Skip to content

Add transcription timeout and retry diagnostics - #5

Merged
claude[bot] merged 2 commits into
mainfrom
improve/20260829-190905
Aug 29, 2026
Merged

Add transcription timeout and retry diagnostics#5
claude[bot] merged 2 commits into
mainfrom
improve/20260829-190905

Conversation

@ssevera1

Copy link
Copy Markdown
Owner

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.

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>
@claude

claude Bot commented Aug 29, 2026

Copy link
Copy Markdown

Both timing claims held up against the code, and both are fixed in 8de9980.

totalTimeMs never spanned retries — confirmed, fixed.
retryStartTime was assigned in launch(), and launch() is reached from the backoff timer on every retry, not just from start(). By the time the budget was exhausted the clock had been reset at the top of the final attempt, so the reported figure was that one attempt's duration.

There are now two separate clocks:

  • retrySequenceStartedAt, set in start() and re-set in onend alongside this.retryAttempt = 0 when sessionWasHealthy() refills the budget, so totalTimeMs covers every attempt and every backoff of the sequence that actually spent the budget.
  • attemptStartedAt, set in launch(), for per-attempt reporting.

timeoutMs was not a timeout — confirmed, fixed both ways.
The onend auto-restart calls recognition.start() in place without going through launch(), so the clock survived across Chrome's silence restarts and a 30-minute session logged timeoutMs: 1800000. The auto-restart path now resets attemptStartedAt, and the field is renamed to msSinceAttemptStart so it can't be read as a timeout value even when it is large for a legitimate reason.

Not changed: maxRetries in the exhaustion context. It's derivable from attemptsMade, but that log line's message (Speech recognition failed after N attempts) doesn't carry the limit — only the retry-scheduling message at the other call site does. Keeping it as a structured field makes the exhaustion event filterable on its own. Your note called it harmless; leaving it.

Regression tests (3 added to TranscriptionService.test.ts, Math.random stubbed to 0 so the backoffs are exactly 500/1000/2000). Verified failing against the pre-fix source:

  • totalTimeMs across a fully exhausted sequence — was 0, now 3500.
  • totalTimeMs after a healthy 10-minute session refills the budget — was 0, now 3500 (i.e. scoped to the sequence, not inflated by the good session).
  • msSinceAttemptStart after a 30-minute session auto-restarts then errors — was timeoutMs: 1800100, now msSinceAttemptStart: 100.

Checks (from .github/workflows/ci.yml): npm run build (tsc -b && vite build) clean; npm test 33 passed across 3 files.

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@claude
claude Bot merged commit 77fd708 into main Aug 29, 2026
2 checks passed
@claude
claude Bot deleted the improve/20260829-190905 branch August 29, 2026 19:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant