diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index d32f9f44..91704e43 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -4,6 +4,8 @@ ### Fixed +- **CLI telemetry shutdown no longer leaks a "Timeout while shutting down PostHog" exception.** `@posthog/core` rejects `_shutdown` when the flush exceeds the timeout; the rejection could win the race in `shutdownTelemetryWithTimeout`, becoming an unhandled rejection that exception autocapture re-reported to PostHog daily and that skipped the CLI's final `process.exit`. The rejection is now swallowed — a slow telemetry flush is expected and silent. + - **Share cards no longer 500 on webp images.** Satori (next/og) can't decode webp, so a webp hero image or avatar (even one with a `.jpg` extension) crashed the whole card render in production. `lib/og-safe-image.ts` now fetches images server-side, format-sniffs them via magic bytes, and passes them through as data URIs only when satori supports them (PNG/JPEG/GIF); unsupported or unfetchable images fall back to the existing no-image layout. Applied to all three affected routes — `/post/[id]` (OG + Twitter), `/api/posts/[id]/share-image`, and `/join/[username]`. - **OG image loaders no longer fetch arbitrary user-controlled URLs.** `users.avatar_url` is stored verbatim from `PATCH /api/users/me`, so a server-side fetch of it was an SSRF probe. Both loaders now apply the same storage allowlist `/api/posts/[id]/share-image` already used (`isAllowedAvatarUrl` for avatars, the `post-images` bucket for hero images) and never fetch anything outside it. Fetches are also bounded by a 3 s deadline and a 5 MiB cap so a slow or oversized third-party response can't hold the render open or exhaust function memory. diff --git a/packages/cli/__tests__/telemetry.test.ts b/packages/cli/__tests__/telemetry.test.ts index 5dbba0e9..4995e6f2 100644 --- a/packages/cli/__tests__/telemetry.test.ts +++ b/packages/cli/__tests__/telemetry.test.ts @@ -4,6 +4,7 @@ vi.mock("../src/lib/posthog.js", () => ({ posthog: { capture: vi.fn(), captureException: vi.fn(), + _shutdown: vi.fn(() => Promise.resolve()), }, })); @@ -17,6 +18,7 @@ import { isPushInvocation, reportCliException, reportUsagePushFailed, + shutdownTelemetryWithTimeout, } from "../src/lib/telemetry.js"; const mockCapture = vi.mocked(posthog.capture); @@ -73,4 +75,45 @@ describe("telemetry", () => { { command: "login" }, ); }); + + it("swallows the posthog shutdown-timeout rejection instead of propagating it", async () => { + // @posthog/core rejects _shutdown with this string when flush exceeds the + // timeout. If it propagates, it becomes an unhandled rejection that + // exception autocapture re-reports and that skips the final process.exit. + vi.mocked(posthog._shutdown).mockRejectedValueOnce( + "Timeout while shutting down PostHog. Some events may not have been sent.", + ); + + await expect(shutdownTelemetryWithTimeout(10)).resolves.toBeTypeOf("number"); + }); + + it("stays quiet when the local timer wins and the rejection lands later", async () => { + // posthog's own timeout and ours are both 150 ms, so the rejection can land + // after the local timer already resolved. Promise.race subscribes to both + // inputs, so that late rejection is already handled — this pins that down + // so a refactor away from Promise.race can't silently reintroduce a + // late unhandled rejection. + const unhandled = vi.fn(); + process.on("unhandledRejection", unhandled); + try { + vi.mocked(posthog._shutdown).mockReturnValueOnce( + new Promise((_resolve, reject) => + setTimeout( + () => + reject( + "Timeout while shutting down PostHog. Some events may not have been sent.", + ), + 20, + ), + ), + ); + + await expect(shutdownTelemetryWithTimeout(1)).resolves.toBeTypeOf("number"); + await new Promise((resolve) => setTimeout(resolve, 60)); + + expect(unhandled).not.toHaveBeenCalled(); + } finally { + process.off("unhandledRejection", unhandled); + } + }); }); diff --git a/packages/cli/src/index.ts b/packages/cli/src/index.ts index 0773ed76..1a1450c8 100755 --- a/packages/cli/src/index.ts +++ b/packages/cli/src/index.ts @@ -246,4 +246,11 @@ main() } console.error(`Error: ${errorMessage(err)}`); }) - .finally(() => shutdownTelemetryWithTimeout().then(() => process.exit(exitCode))); + .finally(() => + // Telemetry shutdown must never decide whether the process exits: if it + // rejects, `.then` is skipped and the CLI hangs on the event loop with an + // unhandled rejection instead of returning its exit code. + shutdownTelemetryWithTimeout() + .catch(() => {}) + .then(() => process.exit(exitCode)), + ); diff --git a/packages/cli/src/lib/telemetry.ts b/packages/cli/src/lib/telemetry.ts index b6806fa1..3acc4a3b 100644 --- a/packages/cli/src/lib/telemetry.ts +++ b/packages/cli/src/lib/telemetry.ts @@ -57,7 +57,10 @@ export async function shutdownTelemetryWithTimeout( let timer: NodeJS.Timeout | undefined; try { await Promise.race([ - posthog._shutdown(timeoutMs), + // @posthog/core rejects _shutdown when flush exceeds the timeout. A + // slow/failed telemetry flush must never surface: unhandled, it gets + // re-captured by exception autocapture and skips the final process.exit. + posthog._shutdown(timeoutMs).catch(() => {}), new Promise((resolve) => { timer = setTimeout(resolve, timeoutMs); timer.unref?.();