fix(core): stop FAL polling after timeout - #392
Conversation
|
🦞👀 Pull request received. I will update this pull request when review starts. |
clawsweeper
Bot
added
merge-risk: 🚨 availability
|
Codex review: needs real behavior proof before merge. Reviewed August 26, 2026, 5:04 AM ET / 09:04 UTC. ClawSweeper reviewWhat this changesThe PR clears FAL transcription deadlines and propagates cancellation through queue polling and retry waits. Merge readiness⛔ Blocked until real behavior proof from a real setup is added - 3 items remain The focused cancellation repair has no concrete correctness finding, but it still needs real FAL-provider evidence before merge. Priority: P2 Review scores
Verification
How this fits togetherFAL is a transcription provider in Summarize Core: audio is uploaded, submitted to FAL’s queue, then polled for text. This path controls local timers and fetches when a transcription attempt completes or times out. flowchart LR
A[Audio input] --> B[FAL upload]
B --> C[FAL queue submission]
C --> D[Status polling and retry waits]
D --> E{Result or deadline}
E -->|Completed| F[Extracted transcript]
E -->|Timed out| G[Abort local fetches and timers]
G --> H[Provider workflow continues]
Before merge
Agent review detailsSecurityNone. Review metrics
Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Retain the shared abort path and add a redacted real FAL queue trace showing deadline-triggered local cancellation, stopped polling, and clean process exit. Do we have a high-confidence way to reproduce the issue? Yes—current source and fake-timer coverage define a high-confidence path: settle or time out an FAL subscription and inspect whether local timers and polling/retry waits remain active. Real provider-boundary reproduction is still absent. Is this the best way to solve the issue? Yes—the shared abort signal plus a finally-cleared deadline is the narrowest maintainable repair for lingering timers and local FAL polling. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 861fa4a90ead. LabelsLabel justifications:
EvidenceWhat I checked:
Likely related people:
Rank-up movesOptional improvements that raise the rating; they are not merge blockers.
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (7 earlier review cycles)
|
|
Triage recommendation: LAND. I reproduced the timer leak and validated this patch against real FAL HTTP using a locally generated synthetic speech recording. No source repair was needed. Current main transcribed the synthetic sentence correctly but retained one referenced ten-minute deadline timer. With this PR's two production files compiled into the core package, the same real upload → queue → polling → result path returned the sentence and retained zero deadline timers. The baseline harness explicitly cleared its leaked timer after recording it so the diagnostic run could exit. Additional live checks:
Redacted output: Validation: Node 24.20.0; core build and targeted TypeScript compilation passed; the two focused regression suites passed all 52 tests. Local execution reused the existing installed dependency tree (Vitest 4.1.10); the PR's existing exact-head CI checks are green. Codex autoreview of This proves local cancellation and cleanup, not remote job cancellation; an already-submitted FAL job remains remote, as documented in the PR. No merge performed. Suggested landing changelog: “FAL transcription: clear settled deadlines and stop local queue polling and retries after timeout (#392, thanks @vincent-peng).” Preserve |
|
🦞👀 Pull request received. I will update this pull request when review starts. |
2 participants
Summary
Root cause
transcribeWithFalracedfal.subscribe()against a 10-minute timer without clearing the losing timer. A successful one-shot CLI invocation could therefore remain alive until that timer fired.When the timer won,
Promise.racerejected the caller but did not cancel the losing subscription. The internal FAL client could continue polling while the provider chain moved on to Deepgram.The deadline still begins after upload, matching the existing behavior. Aborting stops local fetches and timers; it does not cancel an already-submitted remote FAL job.
Verification
pnpm -s vitest run tests/transcription.fal-client.test.ts tests/transcription.whisper.test.ts— 2 files, 52 tests passedpnpm -s check— 556 files passed, 29 skipped; 3,018 tests passed, 43 skippedpnpm -s build— passeddone, reported zero activeTimeoutresources, and exited in 0.07 seconds