Skip to content

fix: default an AbortSignal timeout on browser api() - #124

Open
SebTardif wants to merge 1 commit into
openclaw:mainfrom
SebTardif:fix/browser-api-timeout
Open

fix: default an AbortSignal timeout on browser api()#124
SebTardif wants to merge 1 commit into
openclaw:mainfrom
SebTardif:fix/browser-api-timeout

Conversation

@SebTardif

Copy link
Copy Markdown
Contributor

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/state polling, card actions, interactive-session mutations, and admin allow/repo/policy writes all share one browser helper, api(), and that helper called fetch with no AbortSignal. There was no deadline, so a hung response never failed and never retried.

Why This Change Was Made

api() now attaches AbortSignal.timeout(30_000) unless the caller already passed options.signal. That is the same default used by clickclack's applyDefaultFetchTimeout. 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 node against the production helper (unfixed function copied from current main, then the patched src/app/api.js). The same hung fetch never settles without a signal. With the patch, AbortSignal.timeout is requested with 30000 ms and the pending call rejects TimeoutError.

$ node /tmp/proof-f005.mjs
DEFAULT_FETCH_TIMEOUT_MS 30000
{
  "unfixed": {
    "label": "unfixed api()",
    "outcome": "Error: WATCHDOG: hung fetch still pending",
    "timeoutArgs": [],
    "ms": 152
  },
  "fixed": {
    "label": "fixed api()",
    "outcome": "TimeoutError: The operation was aborted due to timeout",
    "timeoutArgs": [
      30000
    ],
    "ms": 27
  }
}

Related prior art in the same org:

  • clickclack#168 (merged): default 30s fetch timeout unless a caller signal is set
  • clawrouter#111 (merged): abort hung dashboard and OAuth token fetches

src/app/api.js was 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-F005 on fix/browser-api-timeout at the patched src/app/api.js.

  • Exact steps or command run after this patch:

    node /tmp/proof-f005.mjs
  • Evidence after fix: terminal output from the live node command above. Unfixed api() stays pending (watchdog at 152 ms, timeoutArgs empty). Patched api() requests AbortSignal.timeout(30000) and rejects TimeoutError at 27 ms.

  • Observed result after fix: Hung /api/state fetches 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).

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

clawsweeper Bot commented Sep 2, 2026

Copy link
Copy Markdown

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

@clawsweeper clawsweeper Bot added P1 Urgent regression or broken agent/channel workflow affecting real users now. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. merge-risk: 🚨 session-state 🚨 Merging this PR could lose, corrupt, stale, or mis-associate session or agent state. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Sep 2, 2026
@clawsweeper

clawsweeper Bot commented Sep 2, 2026

Copy link
Copy Markdown

Codex review: needs real behavior proof before merge. Reviewed September 5, 2026, 4:01 AM ET / 08:01 UTC.

ClawSweeper review

What this changes

Adds a default 30-second timeout to Crabfleet’s shared browser request helper, preserves caller-provided signals, and adds tests and a changelog entry.

Regression provenance

Possible 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
Reviewed head: d6e75cc3ff5dcf26eeea9988fe5958d4dd891c75

Review scores

Measure Result What it means
Overall readiness 🦪 silver shellfish (2/6) The focused implementation addresses a real gap, but slow-write correctness and real transport proof still block readiness.
Proof confidence 🦪 silver shellfish (2/6) Needs stronger real behavior proof before merge: The captured Node trace exercises the helper with an accelerated timeout, but supplies no identifiable real transport exchange or observed browser recovery; the tests mock fetch. Add redacted browser/Worker diagnostics showing timeout recovery and preserved slow-write outcomes. Terminal output, logs, or a recording with diagnostics count. Remove private endpoints, IPs, and credentials before posting; updating the PR body should trigger review, otherwise ask a maintainer to comment @clawsweeper re-review. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Patch quality 🦐 gold shrimp (3/6) 1 actionable review finding remain.

Verification

Check Result Evidence
Real behavior Needs proof Needs stronger real behavior proof before merge: The captured Node trace exercises the helper with an accelerated timeout, but supplies no identifiable real transport exchange or observed browser recovery; the tests mock fetch. Add redacted browser/Worker diagnostics showing timeout recovery and preserved slow-write outcomes. Terminal output, logs, or a recording with diagnostics count. Remove private endpoints, IPs, and credentials before posting; updating the PR body should trigger review, otherwise ask a maintainer to comment @clawsweeper re-review. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Evidence reviewed 11 items Pinned introduced change: The verified head has the fetched main commit as its raw parent. The introduced diff contains only the helper timeout, three tests, and an Unreleased entry.
Main still lacks the deadline: The current-main helper calls fetch without a signal. Polling catches failures and schedules retries, but a pending fetch prevents that path from running.
Latest-release check: The v0.3.1 helper also has no timeout; the annotated tag resolves to the supplied release commit.
Findings 1 actionable finding [P1] Keep timed-out mutations reconcilable
Security None None.

How this fits together

Crabfleet’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]
Loading

Before merge

  • Add real behavior proof - Needs stronger real behavior proof before merge: The captured Node trace exercises the helper with an accelerated timeout, but supplies no identifiable real transport exchange or observed browser recovery; the tests mock fetch. Add redacted browser/Worker diagnostics showing timeout recovery and preserved slow-write outcomes. Terminal output, logs, or a recording with diagnostics count. Remove private endpoints, IPs, and credentials before posting; updating the PR body should trigger review, otherwise ask a maintainer to comment @clawsweeper re-review. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
  • Keep timed-out mutations reconcilable (P1) - This also aborts POST /api/interactive-sessions, whose Worker handler reserves a session and awaits Sandbox setup; repository checkout alone permits 120 seconds. A successful creation taking 45 seconds now rejects at 30 seconds, and createInteractiveSession() marks its LOCAL-* placeholder failed instead of adopting the returned session. Browser requests carry no replay identity, so resubmission can allocate another session. Preserve the existing lifetime for these writes or reconcile their uncertain outcome before applying this deadline. This prior blocker remains unchanged.
  • Resolve merge risk (P1) - Existing slow workspace creation can now display failure after 30 seconds while its server-side outcome remains uncertain; resubmission can create another session.
  • Resolve merge risk (P1) - The supplied evidence does not establish polling recovery or mutation outcomes through a real transport.
  • Complete next step (P2) - Repair slow-write timeout handling and add redacted real transport evidence before merge.

Findings

  • [P1] Keep timed-out mutations reconcilable — src/app/api.js:8
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Request lifetime mismatch 30-second browser deadline; 120-second checkout budget The browser can abandon a supported creation request before its existing setup budget expires.
Production and test delta Production +3/-0; tests +91/-0 The small helper change has broad caller reach; its tests cover signal wiring but not write outcomes.

Merge-risk options

Maintainer options:

  1. Preserve write lifetimes (recommended)
    Limit the automatic deadline to safe reads and retain existing write behavior, with real transport evidence for polling recovery and slow session creation.
  2. Reconcile timed-out writes
    Keep write deadlines only after callers can recover the original operation’s identity and outcome without treating timeout as definitive failure.

Technical review

Best 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:

  • [P1] Keep timed-out mutations reconcilable — src/app/api.js:8
    This also aborts POST /api/interactive-sessions, whose Worker handler reserves a session and awaits Sandbox setup; repository checkout alone permits 120 seconds. A successful creation taking 45 seconds now rejects at 30 seconds, and createInteractiveSession() marks its LOCAL-* placeholder failed instead of adopting the returned session. Browser requests carry no replay identity, so resubmission can allocate another session. Preserve the existing lifetime for these writes or reconcile their uncertain outcome before applying this deadline. This prior blocker remains unchanged.
    Confidence: 0.97

Overall correctness: patch is incorrect
Overall confidence: 0.96

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against bdd5083b0d3d.

Labels

Label justifications:

  • P2: This is a bounded browser reliability improvement without evidence of a current widespread incident.
  • merge-risk: 🚨 compatibility: Previously successful requests lasting over 30 seconds acquire an unconditional failure deadline.
  • merge-risk: 🚨 session-state: Session creation can be reported as failed without adopting the persisted session identity, making resubmission ambiguous.
  • rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🦐 gold shrimp.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs stronger real behavior proof before merge: The captured Node trace exercises the helper with an accelerated timeout, but supplies no identifiable real transport exchange or observed browser recovery; the tests mock fetch. Add redacted browser/Worker diagnostics showing timeout recovery and preserved slow-write outcomes. Terminal output, logs, or a recording with diagnostics count. Remove private endpoints, IPs, and credentials before posting; updating the PR body should trigger review, otherwise ask a maintainer to comment @clawsweeper re-review. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Evidence

What I checked:

  • Pinned introduced change: The verified head has the fetched main commit as its raw parent. The introduced diff contains only the helper timeout, three tests, and an Unreleased entry. (src/app/api.js:8, d6e75cc3ff5d)
  • Main still lacks the deadline: The current-main helper calls fetch without a signal. Polling catches failures and schedules retries, but a pending fetch prevents that path from running. (src/app/api.js:11, bdd5083b0d3d)
  • Latest-release check: The v0.3.1 helper also has no timeout; the annotated tag resolves to the supplied release commit. (src/app/api.js:11, 8cd813822924)
  • Slow creation becomes a reported failure: Session creation supplies no signal. Any rejection marks its LOCAL-* placeholder failed, bypassing the success path that adopts the server session ID and opens it. (src/app/app-mutations.js:372, d6e75cc3ff5d)
  • Creation waits for provisioning: The Worker allocates a session ID, inserts its reservation, and awaits provisioning before returning. Browser creation does not supply the service’s optional replay identity, so resubmission can allocate another session. (src/worker/session-creation.ts:178, d6e75cc3ff5d)
  • Existing work can exceed the new deadline: Sandbox provisioning awaits terminal setup before reporting ready. That setup awaits repository checkout, whose existing execution budget is 120 seconds, exceeding the new browser deadline. (src/worker/sandbox-runtime.ts:276, d6e75cc3ff5d)

Likely related people:

  • Peter Steinberger: Raw commit 3e70548 adds src/app/api.js:1 relative to its recorded parents. This identifies author metadata, not feature responsibility or a PR merger. (role: source-line author; confidence: high; commits: 3e70548b00ba; files: src/app/api.js)

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

  • Preserve long-running write completion or reconcile timed-out operations without creating duplicates.
  • Supply real transport evidence of polling recovery and successful slow-session handling.

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

History

Review history (5 earlier review cycles)
  • reviewed 2026-09-02T17:20:18.331Z sha d6e75cc :: needs real behavior proof before merge. :: [P1] Protect persistent mutations from unknown timeout outcomes
  • reviewed 2026-09-02T21:50:49.879Z sha d6e75cc :: needs real behavior proof before merge. :: [P1] Keep timed-out mutations reconcilable
  • reviewed 2026-09-03T05:51:34.561Z sha d6e75cc :: needs real behavior proof before merge. :: [P1] Do not time out persistent mutations without reconciliation
  • reviewed 2026-09-03T21:56:20.865Z sha d6e75cc :: needs real behavior proof before merge. :: [P1] Keep timed-out mutations reconcilable
  • reviewed 2026-09-04T02:53:08.809Z sha d6e75cc :: needs real behavior proof before merge. :: [P1] Keep timed-out mutations reconcilable

@clawsweeper clawsweeper Bot added P2 Normal priority bug or improvement with limited blast radius. and removed P1 Urgent regression or broken agent/channel workflow affecting real users now. labels Sep 5, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. merge-risk: 🚨 session-state 🚨 Merging this PR could lose, corrupt, stale, or mis-associate session or agent state. P2 Normal priority bug or improvement with limited blast radius. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant