Repository navigation
[Fix] PostHog captures one exception per API error - #2330
zoomote2[bot] wants to merge 1 commit into
Conversation
|
Important Review skippedAuto reviews are limited based on label configuration. 🏷️ Required labels (at least one): 1
Please check the settings in the CodeRabbit UI or the
 |
|
2 issues outstanding. See session
Reviewed 2b1b074 |
Review statusThis PR was opened by an automated account. A human maintainer must verify the change intent, provenance, and validation before merging. Current step: Mark the PR ready. Required CI must pass before CodeRabbit starts. Review-state labels are managed by this workflow; do not edit them manually. |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![zoomote2[bot]](https://avatars.githubusercontent.com/u/278826416?s=60&v=4){kind=small}
| TelemetryService.instance.captureException(apiError) | ||
|
|
||
| throw new Error(`OpenRouter API Error ${error?.code}: ${rawErrorMessage}`) | ||
| throw apiError |
There was a problem hiding this comment.
Throwing the captured instance lets the telemetry client rewrite the caller-facing message before the throw runs.
PostHogTelemetryClient.captureException synchronously sets error.message = errorMessage (packages/telemetry/src/PostHogTelemetryClient.ts:120) before forwarding to PostHog, and getErrorMessage takes the isOpenAISdkError branch for this instance because Object.assign gave it a numeric status — it returns error.error?.metadata?.raw || error.error?.message || error.message, i.e. the unprefixed raw text. The async client body executes synchronously up to its first await, so the rewrite lands on this instance before line 223 throws it.
Consequences whenever error?.code is a number:
- With telemetry enabled and the error not 429-filtered, callers see
"Rate limit exceeded"(or themetadata.rawtext) instead of"OpenRouter API Error 429: Rate limit exceeded"— the mid-streamstreaming_failedmessage,api_req_failedprompt, and backoff header all show the changed text. With telemetry disabled, or for filtered 429s, the client returns before the rewrite and the prefix survives — user-visible text now depends on telemetry state. - The PR's payload note ("the PostHog message now carries the
OpenRouter API Error <code>:prefix") doesn't hold in production: the real client strips it again before sending. The updatedopenrouter.spec.tsassertions can't catch this because they run against a mocked@roo-code/telemetry, andTask.spec.tsalso uses a mock client.
Capturing a detached copy keeps the caller-facing message and the production PostHog payload both identical to their pre-PR values (the copy still takes the client's rewrite, the thrown instance is untouched):
| TelemetryService.instance.captureException(apiError) | |
| throw new Error(`OpenRouter API Error ${error?.code}: ${rawErrorMessage}`) | |
| throw apiError | |
| // Capture a detached copy: PostHogTelemetryClient#captureException | |
| // rewrites error.message in place (preferring the upstream raw text), | |
| // and this instance is about to be thrown to callers. | |
| TelemetryService.instance.captureException( | |
| Object.assign( | |
| new ApiProviderError(apiError.message, providerIdentifiers.openrouter, modelId, operation, error?.code), | |
| { status: error?.code, error }, | |
| ), | |
| ) | |
| throw apiError |
An enclosing catch that captures the thrown instance is still deduped by the fingerprint window (same provider/model/operation/code/message, same stack, well inside 30s), so the single-capture policy for Bug 4 holds either way.
| * error keeps the window alive while it keeps happening, so one persistent | ||
| * failure surfaces as one $exception event instead of one per retry. | ||
| */ | ||
| const EXCEPTION_DEDUPE_WINDOW_MS = 30 * 1000 |
There was a problem hiding this comment.
The 30s window is shorter than the retry gaps it needs to cover, so a persistent error still emits more than one $exception.
Task#backoffAndAnnounce (src/core/task/Task.ts:6061-6121) doubles requestDelaySeconds || 5 per attempt up to MAX_EXPONENTIAL_BACKOFF_SECONDS = 600, and a 429 RetryInfo can set the delay directly. The inter-attempt gaps run 5s, 10s, 20s, 40s, 80s, ... — from the 4th retry onward each gap exceeds EXCEPTION_DEDUPE_WINDOW_MS, the window expires between attempts, and every later attempt re-captures. A persistent error under auto-approval therefore produces events at attempts 1, 4, 5, 6, ... (roughly one per backoff interval) instead of one total, and the comment above ("one persistent failure surfaces as one $exception event") overstates the suppression for exactly the scenario issue #513 targets.
Either size the window to cover the backoff schedule (larger false-collapse trade-off to weigh), or keep 30s and document the residual rate (about one event per window expiry for long-stuck retries, which PostHog's server-side burst throttling then caps).
Related GitHub Issue
Closes: #513
Description
Providers caught API errors, sent one
$exceptionevent, and re-threw the same failure. The retry loop inTask#attemptApiRequestre-entered the provider on every attempt. One persistent API error produced three to six$exceptionevents. PR #1702 fixed Bugs 1 and 3 of issue #513. This PR fixes Bug 2 and Bug 4 and adds client-side deduplication.This PR picks Option B from the issue.
TelemetryService#captureExceptionnow enforces one capture per error occurrence with two guards:ApiProviderErrorgains acapturedflag. The first capture sets it. Re-throws of the same instance are no-ops. This guard also covers Bug 4.Providers keep their catch blocks and their captures. No telemetry is removed. The issue asked for an audit of capture paths outside the retry loop before this choice.
completePromptand similar paths capture once at the provider and keep doing so. The policy does not drop that telemetry.Bug 4:
OpenRouterHandler#handleStreamingErrorstays the single capture point for mid-stream OpenRouter errors. It now throws the sameApiProviderErrorinstance it captured, so an enclosing capture of that instance is a no-op through the flag. The caller-facing message text is unchanged. One payload note: the PostHog message now carries theOpenRouter API Error <code>:prefix, because one object is both captured and thrown.Test Procedure
New tests:
packages/telemetry/src/__tests__/TelemetryService.exception-single-capture.test.ts. Pins the flag, the window, window expiry, sliding refresh, distinct errors, plain errors, and the not-ready path. Eight tests.src/core/task/__tests__/Task.spec.tsadds a fault-injection test. It drives the real retry loop inattemptApiRequest. The provider seam captures and throws on attempts one and two and recovers on attempt three. The test asserts exactly one clientcaptureExceptioncall and a successful recovery.Consecutive Mistake Errorevent fires andcaptureExceptionnever runs.ConsecutiveMistakeErrortherefore never reaches$exception.src/api/providers/__tests__/openrouter.spec.tsexpects the captured message with theOpenRouter API Error <code>:prefix on the fourhandleStreamingErrorpaths.Commands and results:
pnpm --filter @roo-code/telemetry test: 59 tests pass.pnpm --dir src exec vitest run api/providers/__tests__/openrouter.spec.ts: 41 tests pass.pnpm --dir src exec vitest run core/task/__tests__/Task.spec.ts: 204 tests pass.pnpm testfrom the repository root: 565 test files pass, zero failures. Two unrelatedextension.spec.tstests flaked once under full-suite parallelism. The same flake reproduces on a cleanmaintree and is not caused by this change.tsc --noEmitpasses insrc,@roo-code/types, and@roo-code/telemetry.--prune-suppressionspasses on every changed file. No suppression count increases.Pre-Submission Checklist
src/test-utils/,@/utils/test-utils) are used only for mechanical duplication; test intent and scenario-specific mocks remain visible.Visual Snapshots
N/A. No UI changes.
Videos (interaction / animation only)
N/A.
Documentation Updates
Additional Notes
completePromptcall site. The retry loop has no bounded final failure under auto-approval. Option B removes no telemetry and keeps every capture site in place.$exceptionper distinct occurrence and follows the PostHog guidance on deduplication in retry loops. An identical error re-captures once a quiet gap exceeds the window.capturedflag reads asundefineduntil the first capture, so untouched error instances stay unchanged.Get in Touch
Discord: 0x7777777_.