fix: default an AbortSignal timeout on browser api() - #124
Conversation
Browser login, state refresh, and admin mutations share api(). A stalled Worker left those fetches pending forever. Default AbortSignal.timeout(30s) unless the caller passes signal, matching clickclack applyDefaultFetchTimeout. Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review completeClawSweeper finished reviewing this revision. The review result is being finalized. |
|
Codex review: needs real behavior proof before merge. Reviewed September 5, 2026, 4:01 AM ET / 08:01 UTC. ClawSweeper reviewWhat this changesAdds a default 30-second timeout to Crabfleet’s shared browser request helper, preserves caller-provided signals, and adds tests and a changelog entry. Regression provenancePossible regression — suspected (reviewed change). No predecessor PR is attributed. Merge readiness⛔ Blocked before merge - 5 items remain The timeout remains absent from main and the latest release, so this PR is still useful. The prior slow-mutation blocker remains, and the supplied evidence does not establish real transport recovery. Priority: P2 Review scores
Verification
How this fits togetherCrabfleet’s browser request helper carries login, Fleet polling, and administrative actions to the Cloudflare Worker. Its responses drive browser state, while write requests can also create persistent sessions and provision workspaces. flowchart TD
A[Login and Fleet actions] --> B[Shared browser request helper]
B --> C{Caller signal supplied?}
C -->|Yes| D[Caller lifetime]
C -->|No| E[30 second deadline]
D --> F[Worker request]
E --> F
F --> G[State reads or workspace writes]
F --> H[Browser result or timeout]
Before merge
Findings
Agent review detailsSecurityNone. Review metrics
Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Bound safe reads while preserving long-running writes until their timeout handling can reconcile the actual server outcome. Do we have a high-confidence way to reproduce the issue? Yes, source establishes both paths: main cannot retry an indefinitely pending fetch, and the patch times out session creation before a supported 120-second checkout can finish. Neither path was executed during this read-only review. Is this the best way to solve the issue? No, the shared timeout is appropriate for bounded reads but is too broad for existing synchronous workspace creation; preserve write lifetimes or reconcile uncertain outcomes. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against bdd5083b0d3d. 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 (5 earlier review cycles)
|
What Problem This Solves
Fixes an issue where users signing in, refreshing Fleet state, or running admin actions would leave the Crabfleet web tab pending forever when the Worker or network stalled. Login,
/api/statepolling, card actions, interactive-session mutations, and admin allow/repo/policy writes all share one browser helper,api(), and that helper calledfetchwith noAbortSignal. There was no deadline, so a hung response never failed and never retried.Why This Change Was Made
api()now attachesAbortSignal.timeout(30_000)unless the caller already passedoptions.signal. That is the same default used by clickclack'sapplyDefaultFetchTimeout. Existing callers do not pass a signal, so login, state refresh, and admin actions all pick up the deadline. A caller that needs a different lifetime can still supply its own signal.User Impact
A stalled Worker no longer freezes the Crabfleet web app. After 30 seconds the request aborts, the existing error path shows the timeout, and state polling can retry. Successful JSON responses are unchanged.
Evidence
Live
nodeagainst the production helper (unfixed function copied from current main, then the patchedsrc/app/api.js). The same hungfetchnever settles without a signal. With the patch,AbortSignal.timeoutis requested with 30000 ms and the pending call rejectsTimeoutError.Related prior art in the same org:
src/app/api.jswas added without a deadline in #122 (bdd5083, 2026-08-29), 4 days ago.Real behavior proof
Behavior or issue addressed: Browser
api()left login, state refresh, and admin mutations pending forever when fetch never completed.Real environment tested: macOS 26.6.2, Node v26.7.0, worktree
/tmp/crabfleet-F005onfix/browser-api-timeoutat the patchedsrc/app/api.js.Exact steps or command run after this patch:
Evidence after fix: terminal output from the live node command above. Unfixed
api()stays pending (watchdog at 152 ms,timeoutArgsempty). Patchedapi()requestsAbortSignal.timeout(30000)and rejectsTimeoutErrorat 27 ms.Observed result after fix: Hung
/api/statefetches now abort on the default 30 second deadline instead of remaining pending. Caller-supplied signals are left in place.What was not tested: A live Crabfleet Worker that is actually wedged in production. Browser DevTools on crabfleet.openclaw.ai. Combining a caller signal with the default deadline (
AbortSignal.any).