STT reliability: timeout, retry, and surfaced failure state (LC-590) #578
Loading…
Reference in a new issue
No description provided.
Delete branch "feat/lc590-stt-reliability"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Closes LC-590. Last of the LC-589 transcription-hardening children I have in flight; LC-592 (cost/load control) is the remaining sibling and is untouched here.
What changed
Timeout.
LETS_CHAT_STT_TIMEOUT_SECS(default 60) replaces the 10s ceiling STT shared with every other outbound HTTP call, applied as a per-request override. Voice messages scale it by the recording's own length (read from the stored waveform blob), capped at 300s to match the recorder'sMAX_SECONDS; an operator who configures a larger base is never clamped below it.Retry. Three attempts with 1s then 4s backoff, transient failures only. This lives in a free function over the
SttClienttrait rather than insideReqwestSttClient, specifically so the policy is exercisable throughMockSttClientinstead of only against a live endpoint. Classifying "transient" needed a real signal, soSttErrorgrew aStatus(u16)variant split out of the stringly-typedBadResponse: 5xx / 408 / 429 are the engine being briefly unwell, every other 4xx is deterministic and retrying it only spends the operator's quota to fail three times instead of once.Failures persist and surface. New
transcript_statuscolumn (pending/done/failed, NULL for never-attempted). On exhaustion the upload is marked failed and the existingVoiceTranscribedevent re-broadcasts, so the message re-renders with a "Transcription failed - Retry" control.POST /api/files/{id}/retranscribere-queues it. The call/audioroute now answers non-2xx instead of 200-with-empty-body, andtranscribe.jsreveals an advisory caption-failed line that the next successful clip clears.Two judgement calls worth a look
Live clips do not inherit the operator's base timeout - they take a short 15s override. Raising the base to 60s for voice messages would otherwise regress the live path badly: against a hung engine (one that accepts the connection and never answers) each clip parks three attempts for the full base, ~3 minutes, while the browser keeps posting a new clip every 5 seconds. Stalled requests stack tens deep. The old 10s ceiling bounded that accidentally, and I did not want this ticket to quietly make load worse than it found it. Bounding the concurrency properly (a worker queue) is LC-592's job; this is just the guard rail. A caption that takes longer than 15s has scrolled out of usefulness anyway.
The retranscribe route is uploader-only, matching the control's
message.can_editvisibility exactly, so nobody is ever offered a button that would 403. It spawns rather than awaits, since a worst-case attempt is far too long to hold a request open; the row is markedpendingand the helper broadcasts on success or failure so the UI always resolves. A silent clip is deliberately not a failure - the engine ran fine and heard nothing - so no Retry control appears for silence.Deviations from the ticket
POST /api/files/{id}/retranscriberather than/room/{id}/message/{msg}/retranscribe, to sit alongside the existing/api/files/{id}/altand reuse itsget_upload+ re-render shape.ReqwestSttClient::transcribe, but AC6 asks for retry tests viaMockSttClient, and a mock cannot exercise a loop buried in the production client. Hence the free function.Testing
just checkclean; full suite green at 184 test binaries.Seven new unit tests (timeout scaling and its cap, transient classification, retry-then-succeed, bounded exhaustion, 4xx not retried, the production policy constants, env fallbacks) and five integration tests through
MockSttClient(retry against the real production backoff, exhaustion marks the row failed, silence does not, the retranscribe uploader gate and its non-transcribable rejection, and the non-2xx call clip).Each fix was verified non-vacuous: I broke all four in one pass and confirmed exactly the four intended tests failed and nothing else. The three
from_envtests now share a mutex, since they mutate process-global environment variables the harness would otherwise interleave across threads.Also folded the transcript block, which was duplicated between the voice-message and video-clip branches of
attachment.html, into one shared partial carrying all three states.🤖 Generated with Claude Code
https://claude.ai/code/session_019xQaQQJyeJX7ZS9aVECXwF
No transcription is silently lost any more. Transient failures are retried, permanent ones are persisted and visible, and STT is no longer bound by the 10s ceiling shared with every other outbound HTTP call. `LETS_CHAT_STT_TIMEOUT_SECS` (default 60) replaces the shared `DEFAULT_TIMEOUT` for STT only, applied as a per-request override. For voice messages it scales by the recording's own length (from the stored waveform blob), capped at 300s to match the recorder's `MAX_SECONDS`; an operator who configures a larger base is never clamped below it. Live call clips deliberately do NOT inherit the base: they take a short 15s override, because a hung engine would otherwise park three attempts per 5-second clip while the browser keeps posting a new one every 5 seconds, stacking stalled requests tens deep. The old 10s ceiling bounded that accidentally, and raising it for voice messages must not un-bound it here. Bounding the concurrency properly is LC-592's job. Retries are a free function over the `SttClient` trait rather than logic inside `ReqwestSttClient`, so the policy is exercisable through `MockSttClient` instead of only against a live endpoint. Three attempts with 1s then 4s backoff, and only for transient failures. That classification needed a real signal, so `SttError` grew a `Status(u16)` variant split out of the stringly-typed `BadResponse`: 5xx / 408 / 429 are the engine being briefly unwell, every other 4xx is deterministic and retrying it only spends the operator's quota to fail three times instead of once. The old `(audio, content_type, language)` argument list became an `SttRequest` struct once the timeout needed the clip duration too, which also lets the retry helper clone and replay a request without threading four positional args through both providers. Failures now persist. A new `transcript_status` column records `pending` / `done` / `failed` (NULL for never-attempted, which is every pre-existing row). On exhaustion the upload is marked `failed` and the existing `VoiceTranscribed` event re-broadcasts, so the message re-renders with a "Transcription failed - Retry" control for its author. `POST /api/files/{id}/retranscribe` re-queues the work: uploader-only, matching the control's `message.can_edit` visibility exactly so nobody is offered a button that would 403. It spawns rather than awaits (a worst-case attempt is far too long to hold a request open), marks the row `pending`, and the transcription helper broadcasts on success OR failure so the UI always resolves. A silent clip is deliberately not a failure: the engine ran fine and heard nothing, so no Retry control appears for it. The call `/audio` route now answers non-2xx once its retries are exhausted. It used to answer 200 with an empty body, which is indistinguishable from "you were silent" - the operator got a log line and the user got nothing - so `transcribe.js` now reveals an advisory caption-failed line in the drawer, cleared by the next clip that succeeds. The transcript block was duplicated between the voice-message and video-clip branches of `attachment.html`; both now include one shared partial that carries all three states. Tests: seven new unit tests (timeout scaling and its cap, transient classification, retry-then-succeed, exhaustion is bounded, 4xx is not retried, the production policy constants, env fallbacks) plus five integration tests through `MockSttClient` (retry against the real backoff, exhaustion marks the row failed, silence does not, the retranscribe route's uploader gate and non-transcribable rejection, and the non-2xx call clip). Each fix was verified non-vacuous by breaking it and confirming only its own test fails. The three `from_env` tests now share a mutex, since they mutate process-global environment variables the harness would otherwise interleave. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019xQaQQJyeJX7ZS9aVECXwF