fix(telemetry): fix circuit breaker off-by-one, allSettled on shutdown

Author: edelauna

packages/telemetry/src/TelemetryService.ts

@@ -129,7 +129,7 @@ export class TelemetryService {
 		occurrences.push(now)
 		this.guardedEventOccurrences.set(eventName, occurrences)
 
-		if (occurrences.length > CIRCUIT_BREAKER_MAX_IN_WINDOW) {
+		if (occurrences.length >= CIRCUIT_BREAKER_MAX_IN_WINDOW) {
 			this.trippedUntil.set(eventName, now + CIRCUIT_BREAKER_COOLDOWN_MS)
 			this.guardedEventOccurrences.delete(eventName)
 			return true
@@ -375,7 +375,8 @@ export class TelemetryService {
 			])
 		}
 
-		await Promise.all(this.clients.map((client) => client.shutdown()))
+		// allSettled, not all: one client rejecting must not stop us from awaiting the others.
+		await Promise.allSettled(this.clients.map((client) => client.shutdown()))
 	}
 
 	private static _instance: TelemetryService | null = null

packages/telemetry/src/__tests__/TelemetryService.circuit-breaker.test.ts

@@ -35,21 +35,21 @@ describe("TelemetryService circuit breaker", () => {
 		expect(mockClient.capture).toHaveBeenCalledTimes(49)
 	})
 
-	it("trips after 50 CODE_INDEX_ERROR captures within the window and drops further ones", () => {
+	it("trips at the 50th CODE_INDEX_ERROR capture within the window and drops further ones", () => {
 		const service = new TelemetryService([mockClient])
 
-		for (let i = 0; i < 50; i++) {
+		for (let i = 0; i < 49; i++) {
 			service.captureEvent(TelemetryEventName.CODE_INDEX_ERROR, { i })
 		}
-		expect(mockClient.capture).toHaveBeenCalledTimes(50)
+		expect(mockClient.capture).toHaveBeenCalledTimes(49)
 
-		// 51st capture should be dropped - breaker has tripped.
-		service.captureEvent(TelemetryEventName.CODE_INDEX_ERROR, { i: 50 })
-		expect(mockClient.capture).toHaveBeenCalledTimes(50)
+		// 50th capture trips the breaker but is itself dropped.
+		service.captureEvent(TelemetryEventName.CODE_INDEX_ERROR, { i: 49 })
+		expect(mockClient.capture).toHaveBeenCalledTimes(49)
 
 		// Keeps dropping while tripped.
-		service.captureEvent(TelemetryEventName.CODE_INDEX_ERROR, { i: 51 })
-		expect(mockClient.capture).toHaveBeenCalledTimes(50)
+		service.captureEvent(TelemetryEventName.CODE_INDEX_ERROR, { i: 50 })
+		expect(mockClient.capture).toHaveBeenCalledTimes(49)
 	})
 
 	it("re-allows captures after the cooldown window elapses", () => {
@@ -58,18 +58,17 @@ describe("TelemetryService circuit breaker", () => {
 		for (let i = 0; i < 50; i++) {
 			service.captureEvent(TelemetryEventName.CODE_INDEX_ERROR, { i })
 		}
-		service.captureEvent(TelemetryEventName.CODE_INDEX_ERROR, { i: 50 })
-		expect(mockClient.capture).toHaveBeenCalledTimes(50)
+		expect(mockClient.capture).toHaveBeenCalledTimes(49)
 
 		// Just under 10 minutes - still tripped.
 		vi.setSystemTime(10 * 60 * 1000 - 1)
-		service.captureEvent(TelemetryEventName.CODE_INDEX_ERROR, { i: 51 })
-		expect(mockClient.capture).toHaveBeenCalledTimes(50)
+		service.captureEvent(TelemetryEventName.CODE_INDEX_ERROR, { i: 50 })
+		expect(mockClient.capture).toHaveBeenCalledTimes(49)
 
 		// Cooldown elapsed - one more error gets through.
 		vi.setSystemTime(10 * 60 * 1000)
-		service.captureEvent(TelemetryEventName.CODE_INDEX_ERROR, { i: 52 })
-		expect(mockClient.capture).toHaveBeenCalledTimes(51)
+		service.captureEvent(TelemetryEventName.CODE_INDEX_ERROR, { i: 51 })
+		expect(mockClient.capture).toHaveBeenCalledTimes(50)
 	})
 
 	it("does not reset the guarded count when unrelated events are interleaved", () => {
@@ -85,17 +84,23 @@ describe("TelemetryService circuit breaker", () => {
 		// 25 CODE_INDEX_ERROR so far - still under the threshold of 50.
 		expect(mockClient.capture).toHaveBeenCalledTimes(25 + 25)
 
-		for (let i = 25; i < 50; i++) {
+		for (let i = 25; i < 49; i++) {
 			service.captureEvent(TelemetryEventName.CODE_INDEX_ERROR, { i })
 			service.captureEvent(TelemetryEventName.TASK_CREATED, { taskId: `task-${i}` })
 		}
-		// 50th CODE_INDEX_ERROR trips the breaker; TASK_CREATED events are never guarded.
-		expect(mockClient.capture).toHaveBeenCalledTimes(50 + 50)
+		// 49 CODE_INDEX_ERROR so far - still under the threshold of 50.
+		expect(mockClient.capture).toHaveBeenCalledTimes(49 + 49)
+
+		// 50th CODE_INDEX_ERROR trips the breaker (and is itself dropped); TASK_CREATED
+		// events are never guarded.
+		service.captureEvent(TelemetryEventName.CODE_INDEX_ERROR, { i: 49 })
+		service.captureEvent(TelemetryEventName.TASK_CREATED, { taskId: "task-49" })
+		expect(mockClient.capture).toHaveBeenCalledTimes(49 + 50)
 
 		// Further CODE_INDEX_ERROR captures are dropped even though unrelated events keep flowing.
 		service.captureEvent(TelemetryEventName.CODE_INDEX_ERROR, { i: 50 })
 		service.captureEvent(TelemetryEventName.TASK_CREATED, { taskId: "task-50" })
-		expect(mockClient.capture).toHaveBeenCalledTimes(50 + 51)
+		expect(mockClient.capture).toHaveBeenCalledTimes(49 + 51)
 	})
 
 	it("expires old occurrences outside the counting window instead of trapping the breaker open forever", () => {
@@ -121,4 +126,16 @@ describe("TelemetryService circuit breaker", () => {
 
 		expect(mockClient.capture).toHaveBeenCalledTimes(200)
 	})
+
+	it("returns early on the not-ready (zero-client) branch without touching circuit breaker state", () => {
+		// captureEvent's !this.isReady check runs before shouldDropForCircuitBreaker, so with
+		// no clients registered, guarded-event bookkeeping should never be reached.
+		const service = new TelemetryService([])
+
+		for (let i = 0; i < 50; i++) {
+			service.captureEvent(TelemetryEventName.CODE_INDEX_ERROR, { i })
+		}
+
+		expect(mockClient.capture).not.toHaveBeenCalled()
+	})
 })

packages/telemetry/src/__tests__/TelemetryService.shutdown.test.ts

@@ -44,6 +44,46 @@ describe("TelemetryService.shutdown draining", () => {
 		expect(captureOrder).toEqual(["captured", "shutdown"])
 	})
 
+	it("awaits in-flight captureException calls before shutting down clients", async () => {
+		let resolveCapture!: () => void
+		const capturePromise = new Promise<void>((resolve) => {
+			resolveCapture = resolve
+		})
+
+		const captureOrder: string[] = []
+
+		const mockClient: TelemetryClient = {
+			setProvider: vi.fn(),
+			capture: vi.fn(),
+			captureException: vi.fn().mockImplementation(async () => {
+				await capturePromise
+				captureOrder.push("captured")
+			}),
+			updateTelemetryState: vi.fn(),
+			isTelemetryEnabled: vi.fn().mockReturnValue(true),
+			shutdown: vi.fn().mockImplementation(async () => {
+				captureOrder.push("shutdown")
+			}),
+		}
+
+		const service = new TelemetryService([mockClient])
+
+		// Fire a captureException whose underlying client.captureException() promise hasn't
+		// resolved yet -- follows the same trackPendingClientCall path as captureEvent.
+		service.captureException(new Error("boom"))
+
+		const shutdownPromise = service.shutdown()
+
+		// Capture is still pending - shutdown must not have run yet.
+		expect(captureOrder).toEqual([])
+
+		resolveCapture()
+		await shutdownPromise
+
+		// The in-flight capture must complete before the client is shut down.
+		expect(captureOrder).toEqual(["captured", "shutdown"])
+	})
+
 	it("drains a call already queued before shutdown() started, even across multiple drain passes", async () => {
 		// Regression test: shutdown() must not take a single Promise.all snapshot of
 		// pendingClientCalls. A promise added to pendingClientCalls *after* Promise.all(set)
@@ -175,6 +215,29 @@ describe("TelemetryService.shutdown draining", () => {
 		}
 	})
 
+	it("calling shutdown() twice shuts down clients twice (no re-entrancy guard)", async () => {
+		// Documents current behavior: shutdown() has no guard against being called more than
+		// once. deactivate() only calls it once, so this isn't a bug to fix here, but a second
+		// call re-running the (now-trivial, since pendingClientCalls is empty) drain loop and
+		// calling client.shutdown() again should be an explicit, intentional outcome rather than
+		// an untested one.
+		const mockClient: TelemetryClient = {
+			setProvider: vi.fn(),
+			capture: vi.fn().mockResolvedValue(undefined),
+			captureException: vi.fn(),
+			updateTelemetryState: vi.fn(),
+			isTelemetryEnabled: vi.fn().mockReturnValue(true),
+			shutdown: vi.fn().mockResolvedValue(undefined),
+		}
+
+		const service = new TelemetryService([mockClient])
+
+		await service.shutdown()
+		await service.shutdown()
+
+		expect(mockClient.shutdown).toHaveBeenCalledTimes(2)
+	})
+
 	it("does not let a rejected capture prevent shutdown from completing", async () => {
 		const mockClient: TelemetryClient = {
 			setProvider: vi.fn(),