fix(telemetry): add circuit breaker and shutdown draining to TelemetryService - #1070
Conversation
📝 WalkthroughWalkthrough
ChangesTelemetry resilience
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Extension
participant TelemetryService
participant TelemetryClient
participant TerminalCleanup
Extension->>TelemetryService: await shutdown()
TelemetryService->>TelemetryService: reject new captures
TelemetryService->>TelemetryClient: drain pending captures
TelemetryService->>TelemetryClient: await shutdown()
TelemetryService-->>Extension: complete or report error
Extension->>TerminalCleanup: continue cleanup
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
edelauna
changed the title
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/telemetry/src/__tests__/TelemetryService.shutdown.test.ts (1)
120-144: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding a matching test for
captureExceptionshutdown gating.
captureExceptionhas the sameisShuttingDowngate ascaptureEvent(TelemetryService.ts, lines 165-167), but no test here verifies that acaptureExceptioncall is dropped aftershutdown()has started. Add a test mirroring "stops accepting new captures once shutdown() has started" forcaptureException.As per path instructions, "
**/*.{test,spec}.{ts,tsx,js}: Use package-local unit tests for ... error handling."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/telemetry/src/__tests__/TelemetryService.shutdown.test.ts` around lines 120 - 144, Add a package-local unit test alongside “stops accepting new captures once shutdown() has started” that starts TelemetryService.shutdown(), invokes captureException after shutdown begins, awaits shutdown completion, and asserts the mock client’s captureException was not called. Mirror the existing captureEvent gating test while targeting TelemetryService.captureException.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/telemetry/src/__tests__/TelemetryService.shutdown.test.ts`:
- Around line 120-144: Add a package-local unit test alongside “stops accepting
new captures once shutdown() has started” that starts
TelemetryService.shutdown(), invokes captureException after shutdown begins,
awaits shutdown completion, and asserts the mock client’s captureException was
not called. Mirror the existing captureEvent gating test while targeting
TelemetryService.captureException.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: da2b6bef-3327-452d-b2d0-3addae8e222f
📒 Files selected for processing (4)
packages/telemetry/src/TelemetryService.tspackages/telemetry/src/__tests__/TelemetryService.circuit-breaker.test.tspackages/telemetry/src/__tests__/TelemetryService.shutdown.test.tssrc/extension.ts
github-actions
Bot
added
the
awaiting-review
github-actions
Bot
removed
the
awaiting-review
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/telemetry/src/__tests__/TelemetryService.circuit-breaker.test.ts`:
- Around line 130-140: Update the test case around TelemetryService.captureEvent
to register mockClient after the 50 zero-client calls, then capture one
CODE_INDEX_ERROR and assert that mockClient.capture is called successfully.
Preserve the existing assertion that no capture occurred before registration, so
the test verifies zero-client calls did not consume circuit-breaker capacity.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 59876322-bbe1-4ed6-9fe5-7c012b3e24e2
📒 Files selected for processing (3)
packages/telemetry/src/TelemetryService.tspackages/telemetry/src/__tests__/TelemetryService.circuit-breaker.test.tspackages/telemetry/src/__tests__/TelemetryService.shutdown.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/telemetry/src/TelemetryService.ts
github-actions
Bot
added
the
awaiting-review
github-actions
Bot
removed
the
awaiting-review
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/telemetry/src/__tests__/TelemetryService.shutdown.test.ts (3)
186-216: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAssert that shutdown waits before each timeout.
These tests check settlement only after advancing the clock by 3 seconds. An implementation that skips the pending operation and returns immediately also passes. Advance to 2,999 ms and assert
settledisfalse, then advance the final millisecond and assertsettledistrue.Proposed assertion change
- await vi.advanceTimersByTimeAsync(3000) + await vi.advanceTimersByTimeAsync(2999) + expect(settled).toBe(false) + await vi.advanceTimersByTimeAsync(1)Also applies to: 218-253
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/telemetry/src/__tests__/TelemetryService.shutdown.test.ts` around lines 186 - 216, The shutdown timeout test should verify that shutdown remains pending until the full drain timeout elapses. In the test around TelemetryService.shutdown, advance fake timers by 2,999 ms and assert settled is false, then advance the final millisecond and assert settled is true; apply the same staged assertions to the corresponding test at lines 218–253.
87-158: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftMake the multi-pass test create a later pending call.
Both captures are tracked before
shutdown()starts. A singlePromise.all(Array.from(pendingClientCalls))implementation also waits forsecondCapturePromise, so this test does not detect removal of the later drain pass.Arrange for a new tracked promise to appear after the first snapshot, or test the drain helper through a controlled seam. The current test does not cover the regression described in its comments.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/telemetry/src/__tests__/TelemetryService.shutdown.test.ts` around lines 87 - 158, The multi-pass shutdown test must enqueue a new pending client call after the first drain snapshot, rather than tracking both captures before shutdown begins. Update the test around TelemetryService.shutdown and the mockClient.capture flow so the first capture’s async continuation invokes a capture path that adds a fresh pendingClientCalls entry, then verify shutdown waits for that later call before client shutdown. Preserve the existing ordering assertions and resolve the newly deferred capture explicitly.
255-276: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winAdd a singleton shutdown guard or document the duplicate shutdown contract.
TelemetryService.shutdown()can still invokeclient.shutdown()again if callers overlap with the existing extension lifecycle path. Add a shared in-flight shutdown promise/cooldown here, or explicitly document and cover that duplicate client shutdownes are expected.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/telemetry/src/__tests__/TelemetryService.shutdown.test.ts` around lines 255 - 276, The test currently only documents duplicate sequential shutdowns, while overlapping lifecycle calls can also repeat client shutdown. Update TelemetryService.shutdown and its test to establish the intended duplicate-shutdown contract: preferably reuse a shared in-flight shutdown promise so concurrent or repeated calls invoke each client shutdown once, or explicitly document and cover duplicate calls if that behavior is intentional.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/telemetry/src/__tests__/TelemetryService.shutdown.test.ts`:
- Around line 186-216: The shutdown timeout test should verify that shutdown
remains pending until the full drain timeout elapses. In the test around
TelemetryService.shutdown, advance fake timers by 2,999 ms and assert settled is
false, then advance the final millisecond and assert settled is true; apply the
same staged assertions to the corresponding test at lines 218–253.
- Around line 87-158: The multi-pass shutdown test must enqueue a new pending
client call after the first drain snapshot, rather than tracking both captures
before shutdown begins. Update the test around TelemetryService.shutdown and the
mockClient.capture flow so the first capture’s async continuation invokes a
capture path that adds a fresh pendingClientCalls entry, then verify shutdown
waits for that later call before client shutdown. Preserve the existing ordering
assertions and resolve the newly deferred capture explicitly.
- Around line 255-276: The test currently only documents duplicate sequential
shutdowns, while overlapping lifecycle calls can also repeat client shutdown.
Update TelemetryService.shutdown and its test to establish the intended
duplicate-shutdown contract: preferably reuse a shared in-flight shutdown
promise so concurrent or repeated calls invoke each client shutdown once, or
explicitly document and cover duplicate calls if that behavior is intentional.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 770c5847-985b-4b87-b8d4-c9a0b1af8c0a
📒 Files selected for processing (3)
packages/telemetry/src/TelemetryService.tspackages/telemetry/src/__tests__/TelemetryService.circuit-breaker.test.tspackages/telemetry/src/__tests__/TelemetryService.shutdown.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/telemetry/src/TelemetryService.ts
- packages/telemetry/src/tests/TelemetryService.circuit-breaker.test.ts
github-actions
Bot
added
the
awaiting-review
2 participants
Related GitHub Issue
Refs: #830 (reliability/event-volume-protection half — split from #835; consent/privacy half shipped separately in #1069)
Description
TelemetryServicehad two reliability gaps:CODE_INDEX_ERRORretry loop (e.g. a broken embedder config re-triggering on every file-system event) could flood the PostHog Product Analytics quota with no circuit breaker.deactivate()calledTelemetryService.instance.shutdown()without awaiting it, andshutdown()itself just firedclient.shutdown()on each client without waiting for any in-flightcapture()/captureException()calls to finish first — a capture that started just before shutdown could be silently dropped.This PR adds:
CODE_INDEX_ERROR, tracked independently of all other telemetry: a sliding 10-minute window, trips after 50 occurrences, then suppresses further captures of that event for a 10-minute cooldown before resetting. Unrelated events (TASK_CREATED,TOOL_USED, etc.) are never guarded and can't mask or reset the guarded count.captureEvent/captureExceptionnow track the promise returned by each client'scapture()/captureException()call.shutdown()drains these (looping, not a single snapshot, since draining one call's chain can enqueue more) before callingclient.shutdown(), bounded to a 3-second timeout so a capture stuck on network I/O can't block extension deactivation indefinitely. A rejected capture is caught internally so it never surfaces as an unhandled rejection or blocks the drain.isShuttingDowngate so captures fired aftershutdown()has started are dropped immediately instead of racing the drain loop.src/extension.ts:deactivate()nowawaitsTelemetryService.instance.shutdown(), wrapped in try/catch so a shutdown failure can't skip the remaining terminal cleanup (Terminal.setTerminalProfile,TerminalRegistry.cleanup).Capture call sites (
captureEvent/captureException) remain synchronous/non-blocking for callers — onlyshutdown()awaits the drain.Out of scope
Kept narrow to service-level reliability, per the split from #835:
PRIVACY.md(in fix(telemetry): default telemetry to opt-out with explicit consent UI #1069).Task Completedaggregation (toolsUsed/messageCount),AttemptCompletionToolchanges.modelCache.ts).Test Procedure
pnpm --filter @roo-code/telemetry test: 42/42 passing, including two new files:TelemetryService.circuit-breaker.test.ts— under-threshold pass-through, trip at 50 in-window, cooldown reset, unrelated events don't reset/count toward the guarded total, old occurrences expire out of the window, other event names are never guarded.TelemetryService.shutdown.test.ts— shutdown waits for a pending capture that later resolves, drains a call still in flight across multiple loop passes, stops accepting new captures once shutdown has started, proceeds after the 3s timeout for a never-settling capture, a rejected capture doesn't prevent shutdown from completing.pnpm check-types: clean repo-wide.eslinton all 4 touched files: 0 errors;src/eslint-suppressions.jsonunchanged (no new suppressions).src/__tests__/extension.spec.ts: 4/4 passing, confirming the awaited-shutdown change doesn't break existing deactivation behavior.Pre-Submission Checklist
PRIVACY.mdalready covers the opt-out default via fix(telemetry): default telemetry to opt-out with explicit consent UI #1069).Documentation Updates
Summary by CodeRabbit