Add transcription timeout and retry diagnostics - #5
Conversation
There was a problem hiding this comment.
The change is well-scoped (one file, logging only, no behavior change, no security concerns), but the two new timing fields don't measure what their names and the PR description claim, so the diagnostics would mislead the engineer reading them.
totalTimeMs never spans retries (TranscriptionService.ts:333, set at :171)
this.retryStartTime = Date.now() lives inside launch(), and launch() is called on every retry from the backoff timer (:366), not just from start() (:158). So by the time the budget is exhausted, retryStartTime was reset at the start of the final attempt. totalTimeMs is the duration of the last attempt alone — it excludes every prior attempt and every backoff delay.
Concretely, with the default config (3 retries, 500ms initial, 2x multiplier): a browser that is offline fails each attempt in ~50ms, so the real wall-clock span from start() to exhaustion is ~50 + 500 + 50 + 1000 + 50 + 2000 + 50 ≈ 3.7s, but the log reports totalTimeMs: ~50. That's the exact number the PR exists to surface ("total retry duration"), and it's off by ~70x.
Fix: capture the start of the retry sequence in start() (alongside this.retryAttempt = 0) rather than in launch(), and reset it there when a healthy session refills the budget at :233 if you want it scoped per-sequence. A separate per-attempt field can stay in launch().
timeoutMs is not a timeout, and can be arbitrarily large (TranscriptionService.ts:219)
This is elapsed time since the last launch(), but the onend auto-restart path at :249 calls recognition.start() directly without going through launch(), so retryStartTime is not refreshed there. A session that runs for 30 minutes across Chrome's ~60s silence auto-restarts and then hits a network error logs timeoutMs: 1800000 — which reads like a 30-minute connection timeout when it's really "the user has had the mic open for half an hour." Either reset the clock on the auto-restart path too, or rename to something like msSinceSessionStart so it isn't read as a timeout value.
Minor: maxRetries in the error context duplicates information already in the message template at :352 and in attemptsMade, though it's harmless.
TranscriptionService.test.ts already exists and has the fake-timer scaffolding to assert these values — a test pinning totalTimeMs across a full exhausted retry sequence would have caught the first issue.
totalTimeMs was reset by every launch(), so on exhaustion it reported the duration of the final attempt alone rather than the whole retry sequence. Track the sequence start in start(), and refill it alongside the retry budget when a healthy session ends. timeoutMs was elapsed-since-launch, which the onend auto-restart path never refreshed, so a long-running session reported its full lifetime as if it were a connection timeout. Reset the per-attempt clock on that path too and rename the field to msSinceAttemptStart. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Both timing claims held up against the code, and both are fixed in 8de9980.
There are now two separate clocks:
Not changed: Regression tests (3 added to
Checks (from |
There was a problem hiding this comment.
Traced both new clocks through the state machine and they measure what their names claim. retrySequenceStartedAt is set in start() and refilled in onend alongside the retry budget, so the totalTimeMs logged on exhaustion spans every attempt plus every backoff gap of the sequence that actually spent the budget — not just the last failing attempt. attemptStartedAt is reset in both places that open a session (launch() and the in-place auto-restart branch of onend), so msSinceAttemptStart on a transient error is scoped to the current session rather than the whole call; the comment correctly notes this is elapsed session time, not a timeout. Both reads are guarded by the existing generation check, no control flow or retry/backoff behavior changed, and the fields are numeric durations plus the already-logged error code, so there is no new secret or user-content exposure. Scope is limited to the service and its tests, and the three added tests cover the sequence-spanning case, the healthy-session refill, and the auto-restart re-scoping.
What
Add elapsed time tracking for timeouts and total retry duration to diagnose transcription failures in production.
Why
Helps engineers quickly identify whether failures are due to service latency, connection timeouts, or other issues by providing detailed timing information in logs.