From e0edf946d1ff8a0ba434baf22a80320bd4b9e33c Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Sat, 8 Aug 2026 15:44:41 -0700 Subject: [PATCH 1/4] Batch telemetry events through a single queued transport Two upcoming event catalogs turn telemetry volume from one event per turn into one per tool call, which the previous transport would have answered with an unbounded set of concurrent per-event POSTs. Queueing behind one request bounds both sockets and memory when the endpoint is unreachable. --- src/telemetry/index.ts | 127 ++++++++++--- tests/unit/telemetry-first-run.test.ts | 2 + tests/unit/telemetry-toggle.test.ts | 11 +- tests/unit/telemetry.test.ts | 240 ++++++++++++++++++++----- 4 files changed, 310 insertions(+), 70 deletions(-) diff --git a/src/telemetry/index.ts b/src/telemetry/index.ts index ba61adcc9..df9e56954 100644 --- a/src/telemetry/index.ts +++ b/src/telemetry/index.ts @@ -20,6 +20,22 @@ export const POSTHOG_API_KEY = process.env[TELEMETRY_KEY_ENV] ?? DEFAULT_POSTHOG // in flight past this is dropped. const FLUSH_DEADLINE_MS = 500; +// Batching defaults. A busy turn can emit an event per tool call, so events +// accumulate until either trigger fires rather than opening a socket each +// time. The queue limit bounds memory when the endpoint is unreachable — +// a captive portal or hung proxy would otherwise grow the queue for the +// whole session behind a single stuck request. +const DEFAULT_BATCH_SIZE = 20; +const DEFAULT_BATCH_INTERVAL_MS = 10_000; +const DEFAULT_QUEUE_LIMIT = 500; +const REQUEST_TIMEOUT_MS = 3000; + +export type BatchTuning = { + size?: number; + intervalMs?: number; + queueLimit?: number; +}; + // Shown once per installation, in whichever surface a new user reaches // first: the onboarding panel on a fresh install (so disclosure accompanies // the very first event), and the TUI banner otherwise. @@ -113,19 +129,26 @@ export type CreateTelemetryOptions = { fetchFn?: typeof fetch; host?: string; apiKey?: string; + batch?: BatchTuning; +}; + +type QueuedEvent = { + event: TelemetryEvent; + properties: Record; + timestamp: string; }; export type Telemetry = { enabled: boolean; capture(event: TelemetryEvent, properties?: Record): void; - // Waits briefly for captures currently in flight to settle, giving up + // Sends whatever is queued and waits briefly for it to settle, giving up // after a short deadline so a slow endpoint can never hold up process // exit. Callers use this to bound exit against dropped fire-and-forget // requests without ever making capture() itself blocking. flush(): Promise; }; -// Fire-and-forget PostHog capture client. Never throws, never blocks the +// Fire-and-forget PostHog batch client. Never throws, never blocks the // caller — errors (including timeouts) are swallowed silently since // telemetry must never affect product behavior. export function createTelemetry(options: CreateTelemetryOptions): Telemetry { @@ -136,18 +159,64 @@ export function createTelemetry(options: CreateTelemetryOptions): Telemetry { const fetchFn = options.fetchFn ?? fetch; const installationId = options.settings?.telemetry?.installationId ?? ""; - // Tracked so flush() can wait for in-flight requests without making - // capture() itself awaitable. - const pending = new Set>(); + const batchSize = options.batch?.size ?? DEFAULT_BATCH_SIZE; + const batchIntervalMs = options.batch?.intervalMs ?? DEFAULT_BATCH_INTERVAL_MS; + const queueLimit = options.batch?.queueLimit ?? DEFAULT_QUEUE_LIMIT; + + const queue: QueuedEvent[] = []; + let inFlight: Promise | null = null; + let timer: ReturnType | null = null; + + function cancelTimer(): void { + if (timer === null) return; + clearTimeout(timer); + timer = null; + } + + async function send(events: QueuedEvent[]): Promise { + const body = { + api_key: apiKey, + batch: events.map((queued) => ({ + event: queued.event, + timestamp: queued.timestamp, + properties: { ...queued.properties, distinct_id: installationId }, + })), + }; + try { + await fetchFn(`${host}/batch/`, { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify(body), + signal: AbortSignal.timeout(REQUEST_TIMEOUT_MS), + }); + } catch { + // Swallow all errors — telemetry must never surface failures. + } + } + + // Returns the existing drain when one is running so at most one request is + // ever open: events captured mid-flight are picked up by that drain's next + // iteration instead of opening a second socket. + function drain(): Promise { + if (inFlight !== null) return inFlight; + const running = (async () => { + while (queue.length > 0) { + await send(queue.splice(0, batchSize)); + } + })().finally(() => { + inFlight = null; + }); + inFlight = running; + return running; + } function capture(event: TelemetryEvent, properties?: Record): void { if (!enabled) return; if (!(event === "cli_start" || event === "session_end" || event === "inference_turn")) return; - const body = { - api_key: apiKey, + queue.push({ event, - distinct_id: installationId, + timestamp: new Date().toISOString(), properties: { ...allowedProperties(event, properties), service_version: pkg.version, @@ -156,31 +225,37 @@ export function createTelemetry(options: CreateTelemetryOptions): Telemetry { schema_version: 1, session_id: SESSION_ID, }, - }; + }); - const request = fetchFn(`${host}/capture/`, { - method: "POST", - headers: { "content-type": "application/json" }, - body: JSON.stringify(body), - signal: AbortSignal.timeout(3000), - }) - .then(() => undefined) - .catch(() => { - // Swallow all errors — telemetry must never surface failures. - }); - pending.add(request); - void request.finally(() => pending.delete(request)); + // Oldest first: a stuck endpoint makes the head of the queue the least + // likely to still be worth reporting, and unbounded growth is never an + // acceptable alternative. + if (queue.length > queueLimit) queue.splice(0, queue.length - queueLimit); + + if (queue.length >= batchSize) { + cancelTimer(); + void drain(); + return; + } + if (timer === null) { + timer = setTimeout(() => { + timer = null; + void drain(); + }, batchIntervalMs); + timer.unref?.(); + } } async function flush(): Promise { - if (pending.size === 0) return; + cancelTimer(); + if (queue.length === 0 && inFlight === null) return; // Race against a short deadline: stragglers are dropped rather than - // allowed to delay exit for the full 3s per-request AbortSignal window. + // allowed to delay exit for the full per-request AbortSignal window. await Promise.race([ - Promise.allSettled(pending), + drain(), new Promise((resolve) => { - const timer = setTimeout(resolve, FLUSH_DEADLINE_MS); - timer.unref?.(); + const deadline = setTimeout(resolve, FLUSH_DEADLINE_MS); + deadline.unref?.(); }), ]); } diff --git a/tests/unit/telemetry-first-run.test.ts b/tests/unit/telemetry-first-run.test.ts index 33784a6e6..92cfac2c0 100644 --- a/tests/unit/telemetry-first-run.test.ts +++ b/tests/unit/telemetry-first-run.test.ts @@ -68,6 +68,7 @@ test("activation stamps the notice, swaps the singleton, and fires cli_start", a await activateHeldTelemetry("/fake/path", () => true, deps); expect(markCalls()).toBe(1); expect(getInstance()?.enabled).toBe(true); + await getInstance()?.flush(); expect(fetchCalls()).toBe(1); }); @@ -99,5 +100,6 @@ test("a failed notice stamp does not block activation", async () => { }); await activateHeldTelemetry("/fake/path", () => true, deps); expect(getInstance()?.enabled).toBe(true); + await getInstance()?.flush(); expect(fetchCalls()).toBe(1); }); diff --git a/tests/unit/telemetry-toggle.test.ts b/tests/unit/telemetry-toggle.test.ts index bd5e95f5a..b5e0926c5 100644 --- a/tests/unit/telemetry-toggle.test.ts +++ b/tests/unit/telemetry-toggle.test.ts @@ -180,7 +180,7 @@ test("toggle on re-enables after settings load/save resolve", async () => { }); test("session_id on captured payloads stays constant across an enable/disable/enable toggle cycle", async () => { - const capturedBodies: { properties: Record }[] = []; + const capturedBodies: { batch: { properties: Record }[] }[] = []; const fetchFn = ((_url: string, init: RequestInit) => { capturedBodies.push(JSON.parse(init.body as string)); return Promise.resolve(new Response("1", { status: 200 })); @@ -197,6 +197,9 @@ test("session_id on captured payloads stays constant across an enable/disable/en handler(true); await new Promise((resolve) => setTimeout(resolve, 10)); getInstance().capture("cli_start"); + // Toggling off discards the outgoing instance and its queue, so anything + // captured before it has to leave the process first. + await getInstance().flush(); handler(false); await new Promise((resolve) => setTimeout(resolve, 10)); @@ -205,12 +208,12 @@ test("session_id on captured payloads stays constant across an enable/disable/en handler(true); await new Promise((resolve) => setTimeout(resolve, 10)); getInstance().capture("cli_start"); + await getInstance().flush(); - await new Promise((resolve) => setTimeout(resolve, 0)); expect(capturedBodies.length).toBe(2); - const sessionId = capturedBodies[0].properties.session_id; + const sessionId = capturedBodies[0].batch[0].properties.session_id; expect(typeof sessionId).toBe("string"); expect((sessionId as string).length).toBeGreaterThan(0); - expect(capturedBodies[1].properties.session_id).toBe(sessionId); + expect(capturedBodies[1].batch[0].properties.session_id).toBe(sessionId); expect(sessionId).toBe(getSessionId()); }); diff --git a/tests/unit/telemetry.test.ts b/tests/unit/telemetry.test.ts index a34bfd48d..7e0ae3344 100644 --- a/tests/unit/telemetry.test.ts +++ b/tests/unit/telemetry.test.ts @@ -30,6 +30,20 @@ function fakeFetch(): { impl: typeof fetch; calls: () => number } { return { impl, calls: () => count }; } +type BatchBody = { + api_key: string; + batch: { event: string; timestamp: string; properties: Record }[]; +}; + +function recordingFetch() { + const bodies: BatchBody[] = []; + const impl = ((_url: string, init: RequestInit) => { + bodies.push(JSON.parse(init.body as string) as BatchBody); + return Promise.resolve(new Response("1", { status: 200 })); + }) as unknown as typeof fetch; + return { impl, bodies, events: () => bodies.flatMap((body) => body.batch) }; +} + test("resolveTelemetryEnabled is false when settings.telemetry.enabled is false", () => { expect(resolveTelemetryEnabled(settingsWith("id", false), {})).toBe(false); }); @@ -104,11 +118,7 @@ test("capture rejects unknown event names", () => { }); test("capture strips properties not in the event's allowlist", async () => { - const calls: unknown[] = []; - const impl = ((_url: string, init: RequestInit) => { - calls.push(JSON.parse(init.body as string)); - return Promise.resolve(new Response("1", { status: 200 })); - }) as unknown as typeof fetch; + const { impl, events } = recordingFetch(); const telemetry = createTelemetry({ settings: settingsWith("id"), env: {}, @@ -123,9 +133,9 @@ test("capture strips properties not in the event's allowlist", async () => { exit_reason: "done", secret_field: "should-not-appear", }); - await new Promise((resolve) => setTimeout(resolve, 0)); - expect(calls.length).toBe(1); - const body = calls[0] as { properties: Record }; + await telemetry.flush(); + expect(events().length).toBe(1); + const body = events()[0]; expect(body.properties.status).toBe("ok"); expect(body.properties.turn_count).toBe(3); expect(body.properties.duration_ms).toBe(100); @@ -135,11 +145,7 @@ test("capture strips properties not in the event's allowlist", async () => { }); test("capture strips properties not in inference_turn's allowlist", async () => { - const calls: unknown[] = []; - const impl = ((_url: string, init: RequestInit) => { - calls.push(JSON.parse(init.body as string)); - return Promise.resolve(new Response("1", { status: 200 })); - }) as unknown as typeof fetch; + const { impl, events } = recordingFetch(); const telemetry = createTelemetry({ settings: settingsWith("id"), env: {}, @@ -157,9 +163,9 @@ test("capture strips properties not in inference_turn's allowlist", async () => duration_ms: 400, prompt: "should-not-appear", }); - await new Promise((resolve) => setTimeout(resolve, 0)); - expect(calls.length).toBe(1); - const body = calls[0] as { event: string; properties: Record }; + await telemetry.flush(); + expect(events().length).toBe(1); + const body = events()[0]; expect(body.event).toBe("inference_turn"); expect(body.properties.provider_id).toBe("anthropic"); expect(body.properties.model_id).toBe("claude-x"); @@ -173,11 +179,7 @@ test("capture strips properties not in inference_turn's allowlist", async () => }); test("capture payload shape includes distinct_id and common props, with no client-side geoip flag", async () => { - const calls: unknown[] = []; - const impl = ((_url: string, init: RequestInit) => { - calls.push(JSON.parse(init.body as string)); - return Promise.resolve(new Response("1", { status: 200 })); - }) as unknown as typeof fetch; + const { impl, bodies, events } = recordingFetch(); const telemetry = createTelemetry({ settings: settingsWith("my-install-id"), env: {}, @@ -185,16 +187,13 @@ test("capture payload shape includes distinct_id and common props, with no clien apiKey: "test-key", }); telemetry.capture("cli_start"); - await new Promise((resolve) => setTimeout(resolve, 0)); - const body = calls[0] as { - api_key: string; - event: string; - distinct_id: string; - properties: Record; - }; - expect(body.api_key).toBe("test-key"); + await telemetry.flush(); + expect(bodies.length).toBe(1); + expect(bodies[0].api_key).toBe("test-key"); + const body = events()[0]; expect(body.event).toBe("cli_start"); - expect(body.distinct_id).toBe("my-install-id"); + expect(body.properties.distinct_id).toBe("my-install-id"); + expect(typeof body.timestamp).toBe("string"); expect(body.properties.$geoip_disable).toBeUndefined(); expect(body.properties.schema_version).toBe(1); expect(typeof body.properties.service_version).toBe("string"); @@ -246,11 +245,7 @@ test("flush resolves immediately when nothing is pending", async () => { }); test("capture attaches the same session_id across multiple events in one process", async () => { - const calls: unknown[] = []; - const impl = ((_url: string, init: RequestInit) => { - calls.push(JSON.parse(init.body as string)); - return Promise.resolve(new Response("1", { status: 200 })); - }) as unknown as typeof fetch; + const { impl, events } = recordingFetch(); const telemetry = createTelemetry({ settings: settingsWith("my-install-id"), env: {}, @@ -259,13 +254,13 @@ test("capture attaches the same session_id across multiple events in one process }); telemetry.capture("cli_start"); telemetry.capture("session_end", { status: "ok" }); - await new Promise((resolve) => setTimeout(resolve, 0)); - expect(calls.length).toBe(2); - const bodies = calls as { properties: Record }[]; - const sessionId = bodies[0].properties.session_id; + await telemetry.flush(); + const captured = events(); + expect(captured.length).toBe(2); + const sessionId = captured[0].properties.session_id; expect(typeof sessionId).toBe("string"); expect((sessionId as string).length).toBeGreaterThan(0); - expect(bodies[1].properties.session_id).toBe(sessionId); + expect(captured[1].properties.session_id).toBe(sessionId); expect(sessionId).toBe(getSessionId()); }); @@ -284,3 +279,168 @@ test("ensureTelemetrySettings called twice keeps installationId and enabled flag await rm(dir, { recursive: true, force: true }); } }); + +function gatedFetch() { + const releases: (() => void)[] = []; + const bodies: BatchBody[] = []; + let concurrent = 0; + let peakConcurrent = 0; + let open = false; + const impl = ((_url: string, init: RequestInit) => { + bodies.push(JSON.parse(init.body as string) as BatchBody); + concurrent++; + peakConcurrent = Math.max(peakConcurrent, concurrent); + return new Promise((resolve) => { + const release = () => { + concurrent--; + resolve(new Response("1", { status: 200 })); + }; + if (open) release(); + else releases.push(release); + }); + }) as unknown as typeof fetch; + return { + impl, + bodies, + openGate: () => { + open = true; + for (const release of releases.splice(0)) release(); + }, + peak: () => peakConcurrent, + }; +} + +const turnCounts = (body: BatchBody) => body.batch.map((entry) => entry.properties.turn_count); + +test("capture posts batches to the /batch/ endpoint", async () => { + const urls: string[] = []; + const impl = ((url: string) => { + urls.push(url); + return Promise.resolve(new Response("1", { status: 200 })); + }) as unknown as typeof fetch; + const telemetry = createTelemetry({ + settings: settingsWith("id"), + env: {}, + fetchFn: impl, + apiKey: "test-key", + host: "https://telemetry.example", + }); + telemetry.capture("cli_start"); + await telemetry.flush(); + expect(urls).toEqual(["https://telemetry.example/batch/"]); +}); + +test("reaching the batch size sends one request holding every queued event", async () => { + const { impl, bodies } = recordingFetch(); + const telemetry = createTelemetry({ + settings: settingsWith("id"), + env: {}, + fetchFn: impl, + apiKey: "test-key", + batch: { size: 3, intervalMs: 60_000 }, + }); + telemetry.capture("session_end", { turn_count: 1 }); + telemetry.capture("session_end", { turn_count: 2 }); + expect(bodies.length).toBe(0); + + telemetry.capture("session_end", { turn_count: 3 }); + await new Promise((resolve) => setTimeout(resolve, 5)); + expect(bodies.length).toBe(1); + expect(turnCounts(bodies[0])).toEqual([1, 2, 3]); +}); + +test("a partial batch is sent once the batch interval elapses", async () => { + const { impl, bodies } = recordingFetch(); + const telemetry = createTelemetry({ + settings: settingsWith("id"), + env: {}, + fetchFn: impl, + apiKey: "test-key", + batch: { size: 100, intervalMs: 10 }, + }); + telemetry.capture("session_end", { turn_count: 1 }); + expect(bodies.length).toBe(0); + + await new Promise((resolve) => setTimeout(resolve, 60)); + expect(bodies.length).toBe(1); + expect(turnCounts(bodies[0])).toEqual([1]); +}); + +test("overflowing the queue drops the oldest events", async () => { + const { impl, bodies } = recordingFetch(); + const telemetry = createTelemetry({ + settings: settingsWith("id"), + env: {}, + fetchFn: impl, + apiKey: "test-key", + batch: { size: 100, intervalMs: 60_000, queueLimit: 3 }, + }); + for (let turn = 1; turn <= 5; turn++) telemetry.capture("session_end", { turn_count: turn }); + await telemetry.flush(); + expect(bodies.length).toBe(1); + expect(turnCounts(bodies[0])).toEqual([3, 4, 5]); +}); + +test("captures during a request queue behind it instead of opening a second one", async () => { + const gate = gatedFetch(); + const telemetry = createTelemetry({ + settings: settingsWith("id"), + env: {}, + fetchFn: gate.impl, + apiKey: "test-key", + batch: { size: 1, intervalMs: 60_000 }, + }); + telemetry.capture("session_end", { turn_count: 1 }); + telemetry.capture("session_end", { turn_count: 2 }); + telemetry.capture("session_end", { turn_count: 3 }); + await new Promise((resolve) => setTimeout(resolve, 5)); + expect(gate.bodies.length).toBe(1); + + gate.openGate(); + await telemetry.flush(); + expect(gate.peak()).toBe(1); + expect(gate.bodies.map(turnCounts)).toEqual([[1], [2], [3]]); +}); + +test("flush drains a partially full queue within its deadline", async () => { + const { impl, bodies } = recordingFetch(); + const telemetry = createTelemetry({ + settings: settingsWith("id"), + env: {}, + fetchFn: impl, + apiKey: "test-key", + batch: { size: 100, intervalMs: 60_000 }, + }); + telemetry.capture("session_end", { turn_count: 1 }); + telemetry.capture("session_end", { turn_count: 2 }); + const start = Date.now(); + await telemetry.flush(); + expect(Date.now() - start).toBeLessThan(500); + expect(bodies.length).toBe(1); + expect(turnCounts(bodies[0])).toEqual([1, 2]); +}); + +test("a hung endpoint caps the queue and never opens a second request", async () => { + const gate = gatedFetch(); + const telemetry = createTelemetry({ + settings: settingsWith("id"), + env: {}, + fetchFn: gate.impl, + apiKey: "test-key", + batch: { size: 2, intervalMs: 60_000, queueLimit: 4 }, + }); + for (let turn = 1; turn <= 20; turn++) { + telemetry.capture("session_end", { turn_count: turn }); + await new Promise((resolve) => setTimeout(resolve, 1)); + } + expect(gate.bodies.length).toBe(1); + expect(gate.peak()).toBe(1); + expect(turnCounts(gate.bodies[0])).toEqual([1, 2]); + + gate.openGate(); + await telemetry.flush(); + expect(gate.peak()).toBe(1); + // Only the newest queueLimit events survived the overflow; everything + // between the in-flight batch and them was shed rather than buffered. + expect(gate.bodies.slice(1).flatMap(turnCounts)).toEqual([17, 18, 19, 20]); +}); From fa1e3f584bfd28e90e7eebbc54d61c90fd12fe46 Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Sat, 8 Aug 2026 15:50:20 -0700 Subject: [PATCH 2/4] Document how telemetry batches and sheds events on the wire --- docs/TELEMETRY.md | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/docs/TELEMETRY.md b/docs/TELEMETRY.md index 9133aaf55..13040c943 100644 --- a/docs/TELEMETRY.md +++ b/docs/TELEMETRY.md @@ -92,6 +92,27 @@ Events are sent to PostHog. PostHog derives an approximate country from the request IP server-side; the client sends no location data itself. No self-hosted or third-party analytics beyond PostHog are used. +## On the wire + +Events are not sent one at a time. Each captured event is stamped with its +capture time and held in an in-memory queue, which is posted to PostHog's +`/batch/` endpoint when it reaches the batch size or when the batch +interval elapses, whichever comes first. At most one request is ever in +flight: events captured while a request is open wait for it rather than +opening another connection. Exit paths flush the queue, bounded by a short +deadline so a slow endpoint cannot delay quitting. + +The queue has a hard depth limit. Once it is full — which in practice means +the endpoint is unreachable, as on a captive portal or behind a hung proxy +— the oldest queued events are dropped to make room for new ones. Telemetry +is therefore lossy by design: it never grows memory without bound, never +retries indefinitely, and never blocks or reports failures to the user. +Nothing is written to disk, so dropped events are gone rather than deferred +to a later run. + +See `src/telemetry/index.ts` for the batch size, interval, and queue limit +in force. + ## Not this document Local performance tracing and optional OpenTelemetry export to an operator-owned From 369f8fb550bbffecbf5e935a7976f2c3ae649b5a Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Sat, 8 Aug 2026 15:56:41 -0700 Subject: [PATCH 3/4] Discard queued telemetry events when the user opts out Dropping the singleton on opt-out left the outgoing instance's batch timer armed, so events captured before the toggle would still reach the network afterwards. Opting out speaks to activity already generated, not only to activity still to come. --- docs/TELEMETRY.md | 6 ++++ src/telemetry/index.ts | 15 +++++++++- src/telemetry/singleton.ts | 7 ++++- src/telemetry/toggle.ts | 12 +++++--- tests/unit/telemetry-toggle.test.ts | 44 ++++++++++++++++++++++++++++- 5 files changed, 77 insertions(+), 7 deletions(-) diff --git a/docs/TELEMETRY.md b/docs/TELEMETRY.md index 13040c943..d23cb9ce5 100644 --- a/docs/TELEMETRY.md +++ b/docs/TELEMETRY.md @@ -46,6 +46,12 @@ Any of the following disables telemetry entirely: - `CORBITS_TELEMETRY` set to any falsy value: `0`, `false`, `off`, `no`, or empty - `DO_NOT_TRACK=1` (the standard [Console Do Not Track](https://consoledonottrack.com/) convention) +Turning telemetry off also discards whatever is still queued and unsent. +Events captured earlier in the session but not yet transmitted are thrown +away at the moment you opt out, not sent on the way out — opting out covers +the activity you have already generated, not just the activity still to +come. + Re-enable from the same Telemetry tab or by removing the env var / settings override. While an env kill is active the Telemetry tab cannot re-enable — the env override always wins, and the attempt is refused rather than diff --git a/src/telemetry/index.ts b/src/telemetry/index.ts index df9e56954..406f9e5d8 100644 --- a/src/telemetry/index.ts +++ b/src/telemetry/index.ts @@ -146,6 +146,12 @@ export type Telemetry = { // exit. Callers use this to bound exit against dropped fire-and-forget // requests without ever making capture() itself blocking. flush(): Promise; + // Throws away everything queued and disarms the batch timer, so nothing + // captured before this call can ever reach the network. Opting out uses + // this: a user who says stop mid-session is saying they do not want the + // activity they have already generated sent, which makes discarding the + // queue the honest reading of that intent and flushing it a betrayal. + discard(): void; }; // Fire-and-forget PostHog batch client. Never throws, never blocks the @@ -260,5 +266,12 @@ export function createTelemetry(options: CreateTelemetryOptions): Telemetry { ]); } - return { enabled, capture, flush }; + // A request already on the wire cannot be unsent, but nothing still held + // in memory follows it. + function discard(): void { + cancelTimer(); + queue.length = 0; + } + + return { enabled, capture, flush, discard }; } diff --git a/src/telemetry/singleton.ts b/src/telemetry/singleton.ts index 740aa3dbc..797c9bb2e 100644 --- a/src/telemetry/singleton.ts +++ b/src/telemetry/singleton.ts @@ -5,7 +5,12 @@ import type { Telemetry } from "./index.js"; // than threading it through every intermediate call site. Defaults to a // disabled no-op so any code path that runs before index.ts sets it (or in // tests) never throws. -let instance: Telemetry = { enabled: false, capture: () => {}, flush: async () => {} }; +let instance: Telemetry = { + enabled: false, + capture: () => {}, + flush: async () => {}, + discard: () => {}, +}; export function setTelemetry(telemetry: Telemetry): void { instance = telemetry; diff --git a/src/telemetry/toggle.ts b/src/telemetry/toggle.ts index 39544f2cb..31961022e 100644 --- a/src/telemetry/toggle.ts +++ b/src/telemetry/toggle.ts @@ -57,10 +57,14 @@ export function createTelemetryToggleHandler( return; } if (!enabled) { - // Opt-out must be immediate and absolute: swap the in-memory singleton - // synchronously, before any await, so no capture in flight during the - // persistence step below can land on a still-enabled instance, and so - // an unhandled rejection from disk I/O can never leave telemetry on. + // Opt-out must be immediate and absolute: discard whatever the outgoing + // instance has queued (dropping the singleton alone would leave its + // batch timer armed to send it anyway), then swap the in-memory + // singleton synchronously, before any await, so no capture in flight + // during the persistence step below can land on a still-enabled + // instance, and so an unhandled rejection from disk I/O can never + // leave telemetry on. + deps.getTelemetry().discard(); deps.setTelemetry( deps.createTelemetry({ settings: { providers: {}, telemetry: { enabled: false } } }), ); diff --git a/tests/unit/telemetry-toggle.test.ts b/tests/unit/telemetry-toggle.test.ts index b5e0926c5..ed9fc641a 100644 --- a/tests/unit/telemetry-toggle.test.ts +++ b/tests/unit/telemetry-toggle.test.ts @@ -56,6 +56,43 @@ test("capture called immediately after toggle-off makes zero fetch calls", () => expect(fetchCalls()).toBe(0); }); +// Opting out is a statement about activity already generated, not only about +// activity to come: events captured before the toggle must never be sent +// afterwards. Dropping the singleton is not enough on its own — the outgoing +// instance's batch timer would still fire and post its queue — so this test +// guards the explicit discard. If a future change makes opt-out flush what it +// was holding, this fails, and that is the point. +test("opting out discards events captured before the toggle instead of sending them", async () => { + let sends = 0; + const fetchFn = (() => { + sends++; + return Promise.resolve(new Response("1", { status: 200 })); + }) as unknown as typeof fetch; + const { deps, getInstance } = fakeDeps({ + createTelemetry: (opts) => + createTelemetry({ + ...opts, + env: opts.env ?? {}, + apiKey: opts.apiKey ?? "test-key", + fetchFn, + // Short enough that an undiscarded queue would reach the network well + // inside this test's wait, rather than passing by outrunning a timer. + batch: { intervalMs: 20 }, + }), + }); + const handler = createTelemetryToggleHandler("/fake/path", deps); + + handler(true); + await new Promise((resolve) => setTimeout(resolve, 10)); + getInstance().capture("cli_start"); + expect(sends).toBe(0); + + handler(false); + await new Promise((resolve) => setTimeout(resolve, 150)); + expect(getInstance().enabled).toBe(false); + expect(sends).toBe(0); +}); + test("save rejection leaves the singleton disabled with no unhandled rejection", async () => { const { deps, getInstance } = fakeDeps({ saveGlobalSettings: async () => { @@ -90,7 +127,12 @@ test("load failure skips persistence entirely and stays disabled in memory", asy test("toggle on while env-killed writes nothing and swaps no instance", async () => { let ensureCalled = false; let saveCalled = false; - const initial: Telemetry = { enabled: false, capture: () => {}, flush: async () => {} }; + const initial: Telemetry = { + enabled: false, + capture: () => {}, + flush: async () => {}, + discard: () => {}, + }; let setInstance: Telemetry | undefined; const { deps } = fakeDeps({ getTelemetry: () => initial, From d2ab861d55e58344afa2568e67c96b258f530738 Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Sat, 8 Aug 2026 16:42:09 -0700 Subject: [PATCH 4/4] Add a real flush deadline regression test and document in-flight batches The only existing deadline test resolved its fetch immediately, so it would have kept passing even with the deadline race deleted. The new test gates the fetch open forever and asserts flush() still returns, proving the race is load-bearing. Also documents that a batch already on the wire when a user opts out cannot be recalled, matching what the code comment already says. --- docs/TELEMETRY.md | 3 ++- src/telemetry/index.ts | 5 +++-- tests/unit/telemetry.test.ts | 17 +++++++++++++++++ 3 files changed, 22 insertions(+), 3 deletions(-) diff --git a/docs/TELEMETRY.md b/docs/TELEMETRY.md index d23cb9ce5..bc57c1480 100644 --- a/docs/TELEMETRY.md +++ b/docs/TELEMETRY.md @@ -50,7 +50,8 @@ Turning telemetry off also discards whatever is still queued and unsent. Events captured earlier in the session but not yet transmitted are thrown away at the moment you opt out, not sent on the way out — opting out covers the activity you have already generated, not just the activity still to -come. +come. A batch already in flight to the server at the moment you opt out is +not recalled; discarding only reaches events still held in memory. Re-enable from the same Telemetry tab or by removing the env var / settings override. While an env kill is active the Telemetry tab cannot re-enable — diff --git a/src/telemetry/index.ts b/src/telemetry/index.ts index 406f9e5d8..1358042fb 100644 --- a/src/telemetry/index.ts +++ b/src/telemetry/index.ts @@ -17,8 +17,9 @@ export const POSTHOG_HOST = process.env[TELEMETRY_HOST_ENV] ?? DEFAULT_POSTHOG_H export const POSTHOG_API_KEY = process.env[TELEMETRY_KEY_ENV] ?? DEFAULT_POSTHOG_API_KEY; // Upper bound on how long flush() may hold up process exit; anything still -// in flight past this is dropped. -const FLUSH_DEADLINE_MS = 500; +// in flight past this is dropped. Exported so tests can assert against the +// deadline itself rather than a duplicated magic number. +export const FLUSH_DEADLINE_MS = 500; // Batching defaults. A busy turn can emit an event per tool call, so events // accumulate until either trigger fires rather than opening a socket each diff --git a/tests/unit/telemetry.test.ts b/tests/unit/telemetry.test.ts index 7e0ae3344..5905ec2e2 100644 --- a/tests/unit/telemetry.test.ts +++ b/tests/unit/telemetry.test.ts @@ -4,6 +4,7 @@ import { tmpdir } from "node:os"; import { join } from "node:path"; import { createTelemetry, + FLUSH_DEADLINE_MS, getSessionId, resolveTelemetryEnabled, telemetryDisabledByEnv, @@ -444,3 +445,19 @@ test("a hung endpoint caps the queue and never opens a second request", async () // between the in-flight batch and them was shed rather than buffered. expect(gate.bodies.slice(1).flatMap(turnCounts)).toEqual([17, 18, 19, 20]); }); + +test("flush gives up within its deadline even when the request never settles", async () => { + const gate = gatedFetch(); + const telemetry = createTelemetry({ + settings: settingsWith("id"), + env: {}, + fetchFn: gate.impl, + apiKey: "test-key", + }); + telemetry.capture("cli_start"); + const start = Date.now(); + // Gate is never opened: this proves flush() returns on its own deadline + // rather than because the request happened to resolve. + await telemetry.flush(); + expect(Date.now() - start).toBeLessThan(2 * FLUSH_DEADLINE_MS); +});