From 9085be33e9b6c35ae60c918402569e2fb01540dc Mon Sep 17 00:00:00 2001 From: Dan Draper Date: Tue, 28 Jul 2026 13:54:04 +1000 Subject: [PATCH] fix(cli): stop PostHog SDK printing stack traces on failed telemetry sends MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When the telemetry endpoint is unreachable or returns an error status, @posthog/core's logFlushError writes the full error — nested stack traces included — straight to console.error, from both the capture-triggered background flush and the shutdown flush (two blocks per command). It logs internally before swallowing the rejection, so the CLI's own .catch(() => {}) wrappers never see it. Supply the client a fetch wrapper (silentFetch) that absorbs network errors and HTTP error statuses into a stub 200, so the SDK never observes a failure and never logs. Discarded error responses are drained so undici releases the connection, and the SDK's AbortSignal is passed through so the bounded-flush guarantee holds. Fire-and-forget semantics are unchanged under this client's flushAt: 1 / fetchRetryCount: 0 config — a failed event was already dropped. The traces went to stderr, so stash manifest --json stdout was never corrupted; agent harnesses reporting merged streams read it as stdout. Closes #740 --- .changeset/silent-telemetry-flush.md | 5 ++ .../__tests__/telemetry-lifecycle.test.ts | 4 ++ .../src/telemetry/__tests__/telemetry.test.ts | 72 +++++++++++++++++++ packages/cli/src/telemetry/index.ts | 47 +++++++++++- 4 files changed, 127 insertions(+), 1 deletion(-) create mode 100644 .changeset/silent-telemetry-flush.md diff --git a/.changeset/silent-telemetry-flush.md b/.changeset/silent-telemetry-flush.md new file mode 100644 index 000000000..aea96a71b --- /dev/null +++ b/.changeset/silent-telemetry-flush.md @@ -0,0 +1,5 @@ +--- +'stash': patch +--- + +Fix telemetry stack traces printing to the terminal when the telemetry endpoint is unreachable or returns an error. The PostHog SDK logs flush failures to the console internally (bypassing the CLI's own error swallowing), so a machine with telemetry enabled and a failing network printed two full stack-trace blocks per command. The CLI now supplies the SDK with a fetch wrapper that absorbs network and HTTP errors, so a failed send is silently dropped — matching the documented fire-and-forget behaviour. Command output, including `stash manifest --json` stdout, was never corrupted; the noise went to stderr. diff --git a/packages/cli/src/telemetry/__tests__/telemetry-lifecycle.test.ts b/packages/cli/src/telemetry/__tests__/telemetry-lifecycle.test.ts index ebb14be43..6c5c83ee6 100644 --- a/packages/cli/src/telemetry/__tests__/telemetry-lifecycle.test.ts +++ b/packages/cli/src/telemetry/__tests__/telemetry-lifecycle.test.ts @@ -87,6 +87,10 @@ describe('telemetry lifecycle (emitter + flush)', () => { }) await t.shutdownTelemetry() expect(h.PostHog).toHaveBeenCalledTimes(1) + // Pin the CIP-3587 fix: the client MUST be constructed with silentFetch, + // or a failed send prints the SDK's internal stack traces (logFlushError + // writes to console.error before our .catch() swallows can run). + expect(h.PostHog.mock.calls[0][1].fetch).toBe(t.silentFetch) expect(h.capture).toHaveBeenCalledTimes(1) const arg = h.capture.mock.calls[0][0] expect(arg.distinctId).toBe('anon-x') diff --git a/packages/cli/src/telemetry/__tests__/telemetry.test.ts b/packages/cli/src/telemetry/__tests__/telemetry.test.ts index fd89550c6..7e8320bdb 100644 --- a/packages/cli/src/telemetry/__tests__/telemetry.test.ts +++ b/packages/cli/src/telemetry/__tests__/telemetry.test.ts @@ -5,6 +5,7 @@ import { PLACEHOLDER_KEY, resolveStatus, sanitize, + silentFetch, type TelemetryStatus, } from '../index.js' import type { TelemetryState } from '../state.js' @@ -209,3 +210,74 @@ describe('sanitize (property allowlist)', () => { } }) }) + +describe('silentFetch (CIP-3587: failed sends must never print)', () => { + const request = { + method: 'POST' as const, + headers: { 'Content-Type': 'application/json' }, + body: '{}', + } + + afterEach(() => { + vi.unstubAllGlobals() + }) + + it('passes a successful response through untouched, forwarding url and options verbatim', async () => { + const real = { status: 200, text: async () => 'ok', json: async () => ({}) } + const fetchMock = vi.fn().mockResolvedValue(real) + vi.stubGlobal('fetch', fetchMock) + await expect(silentFetch('https://t.example', request)).resolves.toBe(real) + // The options object (which carries the SDK's AbortSignal) must reach the + // real fetch unmodified — dropping it would unbind requestTimeout and break + // the bounded-flush guarantee the wrapper's doc comment promises. + expect(fetchMock).toHaveBeenCalledWith('https://t.example', request) + }) + + it('swallows a network error into a stub 200 (no throw, nothing for the SDK to log)', async () => { + // The repro: an unreachable endpoint. @posthog/core turns a rejected fetch + // into PostHogFetchNetworkError and console.errors the full stack trace. + vi.stubGlobal( + 'fetch', + vi.fn().mockRejectedValue(new TypeError('fetch failed')), + ) + const res = await silentFetch('https://t.example', request) + expect(res.status).toBe(200) + await expect(res.text()).resolves.toBe('') + }) + + it('swallows a timeout abort into a stub 200', async () => { + // The SDK aborts via AbortController after requestTimeout; fetch rejects + // with an AbortError DOMException, which must be swallowed like any other + // network failure. + vi.stubGlobal( + 'fetch', + vi + .fn() + .mockRejectedValue( + new DOMException('This operation was aborted', 'AbortError'), + ), + ) + const res = await silentFetch('https://t.example', request) + expect(res.status).toBe(200) + }) + + it('swallows an HTTP error status into a stub 200 and drains the real body', async () => { + // status >= 400 makes the SDK throw PostHogFetchHttpError → same log path. + // The discarded response's body must be cancelled so undici releases the + // connection — a held socket outlives the bounded flush and keeps the + // process alive after output. + const cancel = vi.fn(async () => {}) + vi.stubGlobal( + 'fetch', + vi.fn().mockResolvedValue({ + status: 503, + text: async () => '', + json: async () => ({}), + body: { cancel }, + }), + ) + const res = await silentFetch('https://t.example', request) + expect(res.status).toBe(200) + expect(cancel).toHaveBeenCalledTimes(1) + }) +}) diff --git a/packages/cli/src/telemetry/index.ts b/packages/cli/src/telemetry/index.ts index eb87f816b..41af4e6fb 100644 --- a/packages/cli/src/telemetry/index.ts +++ b/packages/cli/src/telemetry/index.ts @@ -1,4 +1,4 @@ -import type { PostHog } from 'posthog-node' +import type { PostHog, PostHogOptions } from 'posthog-node' import { isCiEnvBroad, resolveCaller } from '../config/tty.js' import { messages } from '../messages.js' import { readState, type TelemetryState, writeState } from './state.js' @@ -203,6 +203,49 @@ export function telemetryStatus(): TelemetryStatus { return init().status } +/** + * Fetch wrapper that makes a failed send invisible to the SDK. On any network + * error or HTTP error status, `@posthog/core`'s `logFlushError` writes the full + * error — nested stack traces included — straight to `console.error`, from both + * the capture-triggered background flush and the shutdown flush. That happens + * INSIDE the SDK before it swallows the rejection, so our own `.catch(() => {})` + * swallows never see it (CIP-3587 / #740: an unreachable telemetry endpoint + * printed two stack-trace blocks per command). Returning a stub 200 for every + * failure means the SDK never observes an error, so it never logs. + * The SDK's per-request AbortSignal (requestTimeout) arrives via `options.signal` + * and is passed through, so the bounded-flush guarantee is preserved. + * + * Two deliberate trade-offs, both safe only under this client's config: + * - Faking success defeats the SDK's failure handling (queue retention on + * network error, 413 batch-splitting). With `flushAt: 1` and + * `fetchRetryCount: 0` those branches were already unreachable — the event + * was dropped either way — but revisit this if batching is ever enabled. + * - This client must only ever send capture batches. The stub `{}` reads as a + * valid success payload to the SDK's JSON-parsing endpoints (feature flags, + * remote config), so adding those would return confidently wrong answers on + * any failure. + */ +export const silentFetch: NonNullable = async ( + url, + options, +) => { + try { + const res = await fetch(url, options) + // Mirror the SDK's own success window (it throws outside 200–399). + if (res.status >= 200 && res.status < 400) return res + // Drain the discarded error response so undici releases the connection. + // The SDK's old error path did this via `await err.text`; without it the + // socket outlives the bounded flush and keeps the process alive. + await res.body?.cancel() + } catch { + // Anything that failed to produce a usable response falls through to the + // stub: unreachable endpoint, DNS failure, timeout abort — and also + // request-construction failures (e.g. a malformed STASH_POSTHOG_HOST), + // which are swallowed the same as network-down. + } + return { status: 200, text: async () => '', json: async () => ({}) } +} + async function getClient(): Promise { if (clientPromise === null) { clientPromise = import('posthog-node').then( @@ -222,6 +265,8 @@ async function getClient(): Promise { requestTimeout: FLUSH_TIMEOUT_MS, // Never resolve IP → geo; we don't want or store location. disableGeoip: true, + // Failures must never print — see silentFetch. + fetch: silentFetch, }), ) }