TT-7621 fix: stop the PBT blob-load dead-states and never-ending bootstrap poll - #547
TT-7621 fix: stop the PBT blob-load dead-states and never-ending bootstrap poll#547nabalone wants to merge 3 commits into
Conversation
…oll (red) Failing repros for the defensive-hardening half of the PBT hung-state report. Kept as a separate commit so the fixes that follow are demonstrably what turns them green. - useFetchMediaBlob: a download that is an S3/CDN error page (text/html or application/xml) dispatched neither FETCHED nor ERROR, so blobStat stayed PENDING forever and the reference player's context `loading` never cleared (top player stuck "Loading..."). - useFetchMediaBlob: a persistently-403 object drove an unbounded RESET->PENDING->403 loop, re-issuing a signed-URL request and a blob GET every turn (the network storm in the report) and never reaching a terminal state. - useGuidedPhraseSegments.ensureSegments: when auto-segment finds no boundaries it returned false forever, leaving the 250ms bootstrap poll (and its effect churn) spinning. It should fall back to one full-length segment once audio is loaded, and return false only while the player has no audio yet. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…strap poll Defensive hardening for the PBT hung-state report. Each is an independent way the step could stop settling; none is the core wavesurfer revoke race (that riskier root-cause fix is deferred to after the next release). useFetchMediaBlob: - A download that is an S3/CDN error page (text/html or application/xml) now dispatches ERROR instead of nothing, so blobStat can no longer stay PENDING forever - which stranded the reference player's context `loading` true and the top player on "Loading...". - The 403 -> RESET -> re-request cycle is now capped (MAX_URL_RESETS). A URL that keeps 403ing (a real permission problem, not expiry) surfaces the error instead of re-issuing a signed-URL request and a blob GET on every turn - one of the request storms in the report. useGuidedPhraseSegments.ensureSegments: - When auto-segment finds no boundaries it now falls back to one full-length segment (once audio is loaded) instead of returning false forever, so the 250ms bootstrap poll in PassageDetailGuidedPhraseRecord - and its effect churn - can stop. It still returns false while the player has no audio yet. Greens the repros committed in the previous change. jest: useFetchMediaBlob + useGuidedPhraseSegments 4/4; MediaPlayer + MediaRecord 44 pass / 3 skip; tsc clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR hardens the renderer’s Phrase Back Translate (PBT) media bootstrap paths to prevent “never-settling” states (stuck loading / endless retry loops) by ensuring blob fetches and segment bootstrap converge to a terminal state instead of spinning.
Changes:
- Make
useFetchMediaBlobterminate withERRORwhen the fetched “blob” is actually an S3/CDN error page (HTML/XML), and cap repeated 403 URL-reset retries. - Make
useGuidedPhraseSegments.ensureSegmentsfall back to a single full-length segment when auto-segmentation yields no boundaries (once audio duration is known), allowing the 250ms bootstrap poll to stop. - Add regression tests covering both the blob-loader terminal states and the segment bootstrap fallback behavior.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/renderer/src/crud/useFetchMediaBlob.ts | Adds bounded 403 reset retries and treats HTML/XML blob bodies as terminal errors. |
| src/renderer/src/crud/useFetchMediaBlob.test.ts | Adds TT-7621 regression tests for error-page blobs and capped 403 retry behavior. |
| src/renderer/src/components/PassageDetail/carefulSpeech/useGuidedPhraseSegments.ts | Ensures segment bootstrap converges by falling back to a single segment when auto-segment finds none. |
| src/renderer/src/components/PassageDetail/carefulSpeech/useGuidedPhraseSegments.test.ts | Adds TT-7621 regression tests for the single-segment fallback and “no audio yet” behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| } else { | ||
| // An HTML/XML body is an error page (S3/CDN), not audio. Terminate | ||
| // instead of leaving blobStat PENDING forever, which stranded the | ||
| // reference player on "Loading..." (TT-7621). | ||
| dispatch({ type: BlobStatus.ERROR, payload: urlOrError }); | ||
| } |
There was a problem hiding this comment.
Done in b0fcd8d. The HTML/XML error-page branch now dispatches ERROR with unexpected content type ${blob.type}: ${urlOrError}, so the logged error names the unexpected content type instead of just the URL + id.
| useEffect(() => { | ||
| resetTriesRef.current = 0; | ||
| fetchMediaUrl({ id: mediaId }); | ||
| dispatch({ | ||
| type: BlobStatus.PENDING, |
There was a problem hiding this comment.
Good catch — fixed in b0fcd8d. The [mediaId] effect now dispatches BlobStatus.IDLE (not PENDING) when mediaId is empty, so a consumer reading loading from blobStat === PENDING no longer shows a spurious spinner before the first fetchBlob.
- Stay IDLE (not PENDING) on the initial [mediaId] effect when mediaId is empty, so a consumer reading loading from blobStat === PENDING does not show a spurious spinner before the first fetchBlob. - Name the unexpected content type in the ERROR payload when the downloaded body is an HTML/XML error page, so the logged error is actionable. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
src/renderer/src/crud/useFetchMediaBlob.ts:87
loadBlob()is async and its callback can still fire aftermediaIdhas changed (e.g., rapid step/segment toggles). BecauseresetTriesRefis shared across requests and there’s no “is this still the active request?” guard, a stale callback can increment/reset the retry counter or dispatch RESET/ERROR against the new request, causing incorrect terminal states or prematurely hittingMAX_URL_RESETS. Consider tracking the latest requestedmediaIdin a ref (similar to other hooks that avoid stale closures) and using it to ignore stale responses/retry accounting.
This issue also appears on line 112 of the same file.
const [mediaId, setMediaId] = useState('');
const { fetchMediaUrl, mediaState } = useFetchMediaUrl(reporter);
const [state, dispatch] = useReducer(stateReducer, blobClean);
const resetTriesRef = useRef(0);
src/renderer/src/crud/useFetchMediaBlob.ts:115
- Even with a
mediaIdReftracked, theloadBlobcallback currently has no guard before it mutates state / triggers URL resets. Adding an early-return when the callback is for a stalemediaIdprevents older in-flight downloads from dispatching RESET/ERROR (or affecting retry counts) after the user has already requested a different blob.
if (state.blobStat === BlobStatus.PENDING && mediaState.url) {
try {
loadBlob(mediaState.url, (urlOrError, blob) => {
if (!blob) {
| dispatch({ | ||
| type: BlobStatus.ERROR, | ||
| payload: `unexpected content type ${blob.type}: ${urlOrError}`, | ||
| }); |
What this is
The low-risk, defensive half of TT-7621 (PBT goes into a hung state when navigating between Phrase Back Translate and Phrase BT Transcribe). Fix #530 removed the stuck-
loadingflag but the hang persisted. This PR closes three independent ways the step could stop settling — each defensible on its own. None of them is the core wavesurfer revoke race, which is what actually drives the freeze; that riskier root-cause fix is deliberately deferred to after the next release (see below).Changes
useFetchMediaBlob— a download whose body is an S3/CDN error page (text/html/application/xml) dispatched neither FETCHED nor ERROR, soblobStatstayed PENDING forever and the reference player's contextloadingnever cleared (top player stuck on "Loading…"). Now dispatches ERROR.useFetchMediaBlob— the403 → RESET → re-requestcycle is now capped (MAX_URL_RESETS). A URL that keeps 403ing (a real permission problem, not expiry) surfaces the error instead of re-issuing a signed-URL request + blob GET on every turn (one of the request storms in the report).useGuidedPhraseSegments.ensureSegments— when auto-segment finds no boundaries it now falls back to one full-length segment (once audio is loaded) instead of returningfalseforever, so the 250 ms bootstrap poll (and its effect churn) can stop.Live reproduction — done, and what it shows
Reproduced the full terminal freeze locally (2026-08-27) on current
developwith this PR's hardening reverted, using a take-bearing fixture: project "hung state" / passage Ps 119:1-103 with recorded back-translation takes and the adjacent Phrase Back Translate → Phrase BT Transcribe steps (bothartifactTypeId 2). Driving rapid chip-toggling under 4× CPU throttle fired all three diagnosed signatures at once:Maximum update depth exceeded— theforceRefreshfeedback loop. (This was absent on the earlier QA attempt, which had no takes — the takes are the fuel for theblob:revoke race.)blob: net::ERR_FILE_NOT_FOUNDstorm, re-fetching identical…backtranslation*.oggfileurls — a blob URL revoked while the media element is still fetching it.A/B with this PR applied (same fixture and protocol, branch rebased onto current
develop): the freeze still occurs — 61 s max block / 338 s total from ~8 toggles, blob storm unchanged. TheMaximum update deptherror didn't surface on that run (the bootstrap-poll fix may cut one loop path), but the freeze persists via the remaining media-reload churn, so it is not a win on its own.Deferred (separate branch, after release) — the actual fix
The core race — a
forceRefresh/useOrbitDataidentity-churn feedback loop that re-drives both wavesurfer loads until ablob:URL is revoked mid-fetch andreadynever fires — is documented indocs/adr/0011-pbt-hung-state-forcerefresh-feedback.mdon branchTT-7621_hang-root-cause-feedback, with file:line fix targets, risks, and the validation plan. Not implemented here on purpose: it touches the most heavily-patched part of the step. The repro fixture above is now available to validate it against.Test plan
jest useFetchMediaBlob useGuidedPhraseSegments→ 4/4 green (red before the fix).jest MediaPlayer MediaRecord→ 44 pass / 3 skip (no regression).npm run typecheck→ clean.🤖 Generated with Claude Code