Skip to content

fix(oauth): abort hung grant refresh token fetches - #112

Merged
steipete merged 3 commits into
openclaw:mainfrom
SebTardif:fix/refresh-grant-fetch-timeout
Aug 16, 2026
Merged

fix(oauth): abort hung grant refresh token fetches#112
steipete merged 3 commits into
openclaw:mainfrom
SebTardif:fix/refresh-grant-fetch-timeout

Conversation

@SebTardif

Copy link
Copy Markdown
Contributor

What Problem This Solves

refreshGrant in worker/providers.ts posts the OAuth refresh token to tokenUrl with no AbortSignal. If the provider token endpoint accepts the TCP connection and never responds, admin grant refresh and proxy grant selection stay pending. The same hang class is already fixed for dashboard fetches and the OAuth callback token POST in #111. This change covers the remaining grant-refresh POST.

Evidence

Live Node against a TCP server that accepts the connection and never writes an HTTP response. The shared helper aborts in ~80ms when given an 80ms budget. Production refreshStoredGrant() uses the default 30s budget, returns grant_refresh_failed (502), and does not persist a new grant.

$ node --version && uname -srm
v26.7.0
Darwin 25.6.0 arm64

$ node /tmp/oc-clawrouter-refresh-proof.mjs
hung_server=http://127.0.0.1:51374
default_timeout_ms=30000
helper_abort name=TimeoutError elapsed_ms=91
refresh_grant code=grant_refresh_failed status=502 elapsed_ms=30003 timeout_ms=[30000]
proof_ok hung grant refresh aborted

Patched call site on this branch:

$ rg -n "fetchTimeoutSignal|grant_refresh_failed" shared/fetch-timeout.ts worker/providers.ts
shared/fetch-timeout.ts:1:export const DEFAULT_FETCH_TIMEOUT_MS = 30_000;
shared/fetch-timeout.ts:3:export function fetchTimeoutSignal(existing?: AbortSignal | null, timeoutMs = DEFAULT_FETCH_TIMEOUT_MS): AbortSignal {
worker/providers.ts:1:import { fetchTimeoutSignal } from "../shared/fetch-timeout.ts";
worker/providers.ts:391:  try { response = await fetch(config.tokenUrl, { ..., signal: fetchTimeoutSignal() }); }
worker/providers.ts:392:  catch { throw new HttpError(502, "grant_refresh_failed", `provider ${provider.id} rejected the refresh request`); }

A hung or aborted token POST now takes the existing grant_refresh_failed path instead of leaving the Worker fetch pending.

Real behavior proof

  • Behavior or issue addressed: Grant refresh token POSTs now carry a 30s AbortSignal.timeout. A hung tokenUrl finishes as grant_refresh_failed (502) and does not write a new grant to KV.
  • Real environment tested: macOS Darwin 25.6.0 arm64, Node v26.7.0, clawrouter checkout /tmp/oc-impl-clawrouter-refresh on fix/refresh-grant-fetch-timeout. Live node against a local hanging HTTP server (accept, no response body).
  • Exact steps or command run after this patch: Started a node:http server that never writes a response. Called fetchTimeoutSignal(undefined, 80) on POST /oauth/token, then called production refreshStoredGrant() with that hung tokenUrl so the default 30s budget is the one compiled into worker/providers.ts.
  • Evidence after fix: terminal output copied below.
hung_server=http://127.0.0.1:51374
default_timeout_ms=30000
helper_abort name=TimeoutError elapsed_ms=91
refresh_grant code=grant_refresh_failed status=502 elapsed_ms=30003 timeout_ms=[30000]
proof_ok hung grant refresh aborted
  • Observed result after fix: The 80ms helper path aborted with TimeoutError at 91ms. Production refreshStoredGrant() recorded AbortSignal.timeout(30000) and finished at 30003ms with grant_refresh_failed / 502. KV was not updated.
  • What was not tested: Live Cloudflare Worker against a real provider tokenUrl. Dashboard and OAuth callback token POSTs (covered by fix: abort hung dashboard and OAuth token fetches #111). Browser click of the access-console Refresh action.

Summary

refreshGrant posted to tokenUrl with no AbortSignal. A hung provider
left grant refresh and proxy grant selection pending.

Reuse fetchTimeoutSignal (30s) and map abort/timeout to
grant_refresh_failed.

Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>
@clawsweeper

clawsweeper Bot commented Aug 15, 2026

Copy link
Copy Markdown

🦞👀
ClawSweeper picked this up.

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

@clawsweeper clawsweeper Bot added merge-risk: 🚨 availability 🚨 Merging this PR could cause crashes, hangs, restart loops, stalls, or process outages. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. P2 Normal priority bug or improvement with limited blast radius. proof: sufficient Contributor real behavior proof is sufficient. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. labels Aug 15, 2026
@clawsweeper

clawsweeper Bot commented Aug 15, 2026

Copy link
Copy Markdown

Codex review: needs maintainer review before merge. Reviewed August 16, 2026, 7:32 AM ET / 11:32 UTC.

ClawSweeper review

What this changes

This PR adds a 30-second timeout to OAuth grant-refresh token requests so a hung provider endpoint fails cleanly instead of leaving refresh work pending.

Merge readiness

⚠️ Ready for maintainer review - 1 item remains

Keep open: current main still leaves the grant-refresh token POST unbounded, while this focused PR converts a provider stall into the established 502 failure path with credible real-behavior proof.

Priority: P2
Reviewed head: 5e02ba36d6a9940a8ffe8cbec05c9fc5542b1cd3

Review scores

Measure Result What it means
Overall readiness 🐚 platinum hermit (4/6) A focused reliability patch with strong fault-injection proof and targeted regression coverage; the remaining choice is the intentional timeout ceiling.
Proof confidence 🦞 diamond lobster (5/6) Sufficient (terminal): The PR includes an after-fix terminal trace through production refreshStoredGrant and a real local HTTP transport fault, showing timeout recovery, the 502 mapping, and no grant write.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Verified Sufficient (terminal): The PR includes an after-fix terminal trace through production refreshStoredGrant and a real local HTTP transport fault, showing timeout recovery, the 502 mapping, and no grant write.
Evidence reviewed 6 items Current-main defect: The production refresh path posts to the configured token URL without an AbortSignal; the same helper is used by explicit admin refresh and proxy grant selection.
Supported OAuth boundary: The bundled OpenAI manifest is the current refresh-enabled provider and declares its approved token endpoint, so the change stays within the provider-neutral grant-refresh path rather than adding a provider adapter.
Existing public contract: The documented admin refresh endpoint invokes the Worker refresh flow, confirming the affected operator-visible boundary.
Findings None None.
Security None None.

How this fits together

The Worker refreshes stored OAuth grants through provider token endpoints, then saves the updated grant for admin actions and proxy grant selection. This change bounds that outbound request and returns the existing refresh error when the provider does not respond.

flowchart LR
A[Stored OAuth grant] --> B[Grant refresh worker]
B --> C[Provider token request]
C --> D{Response before timeout?}
D -->|Yes| E[Validate and store grant]
D -->|No| F[Grant refresh failure]
F --> G[Admin and proxy receive 502]
Loading

Before merge

  • Resolve merge risk (P1) - A legitimate OpenAI refresh taking longer than 30 seconds will now return grant_refresh_failed (502) rather than continue waiting; this is a deliberate availability-versus-latency ceiling.
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Patch scope 4 files affected: production +10/-1, tests +125 The change is confined to the grant-refresh fetch boundary and targeted timeout/error-path coverage.

Merge-risk options

Maintainer options:

  1. Accept the bounded refresh window (recommended)
    Merge the focused timeout fix, accepting that refreshes exceeding 30 seconds use the existing 502 failure path instead of holding the Worker request open.
  2. Use a different supported ceiling
    If operational evidence shows the configured OAuth provider needs longer than 30 seconds, adjust the shared timeout and rerun the supplied hung-endpoint regression coverage before merging.

Technical review

Best possible solution:

Land the bounded failure behavior with the focused regression coverage, retaining the existing grant_refresh_failed response and no persisted grant update on timeout.

Do we have a high-confidence way to reproduce the issue?

Yes—current main awaits the refresh POST without a signal, and a local HTTP server that accepts a connection without responding exercises that path; the contributor supplied an after-fix production-owner trace using this setup.

Is this the best way to solve the issue?

Yes—the timeout is applied at the one unbounded production refresh boundary and preserves the existing error contract, making it narrower than adding provider-specific behavior.

AGENTS.md: found and applied where relevant.

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

Labels

Label justifications:

  • P2: A provider stall blocks grant refresh for affected admin and proxy operations but the proposed failure is bounded.
  • merge-risk: 🚨 compatibility: The new 30-second ceiling changes the outcome for a provider refresh that legitimately takes longer.
  • merge-risk: 🚨 availability: The patch intentionally changes a potentially indefinite outbound wait into a controlled refresh failure.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🦞 diamond lobster and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (terminal): The PR includes an after-fix terminal trace through production refreshStoredGrant and a real local HTTP transport fault, showing timeout recovery, the 502 mapping, and no grant write.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR includes an after-fix terminal trace through production refreshStoredGrant and a real local HTTP transport fault, showing timeout recovery, the 502 mapping, and no grant write.

Evidence

What I checked:

  • Current-main defect: The production refresh path posts to the configured token URL without an AbortSignal; the same helper is used by explicit admin refresh and proxy grant selection. (worker/providers.ts:389, ca27bc44227f)
  • Supported OAuth boundary: The bundled OpenAI manifest is the current refresh-enabled provider and declares its approved token endpoint, so the change stays within the provider-neutral grant-refresh path rather than adding a provider adapter. (providers/openai.provider.yaml:29, ca27bc44227f)
  • Existing public contract: The documented admin refresh endpoint invokes the Worker refresh flow, confirming the affected operator-visible boundary. (docs/api-reference.md:103, ca27bc44227f)
  • Submitted implementation and proof: The supplied PR patch adds the shared timeout helper, passes it to the refresh POST, maps fetch failures to grant_refresh_failed, and adds tests. Its terminal transcript exercises refreshStoredGrant against a live local HTTP server that accepts but never responds, observing a 502 after the 30-second production timeout with no KV update. (worker/providers.ts:389, 5e02ba36d6a9)
  • Current-area provenance: Current main's most recent local history entry for the provider path is the per-provider budget work, identifying the recent area contributor; the original TypeScript data-plane introduction is documented by the merged related PR. (worker/providers.ts:371, 98e78e8709f3)
  • Inspection limitation: The checkout is a partial clone: attempts to materialize historical or PR blobs required a promisor fetch, but DNS could not resolve github.com. Current-main source and the provided fully hydrated PR metadata were available; no runtime tests were run because this review is read-only.

Likely related people:

  • Peter Steinberger: The most recent locally available main-history entry touching the provider module is Peter Steinberger's per-provider budget work; the PR head's latest test commit is also attributed locally to him. (role: recent area contributor; confidence: medium; commits: 98e78e8709f3, 5e02ba36d6a9; files: worker/providers.ts, worker/test/grant-refresh-fetch.test.mjs)
  • steipete: The provided merged-PR history identifies steipete as author of the TypeScript data-plane change that introduced this grant-refresh path. (role: data-plane introducer; confidence: medium; commits: 61cba0807c1e; files: worker/providers.ts)

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 (1 earlier review cycle)
  • reviewed 2026-08-15T23:38:45.399Z sha cbcf888 :: needs maintainer review before merge. :: none

# Conflicts:
#	shared/fetch-timeout.ts
#	worker/test/fetch-timeout.test.mjs
@steipete
steipete merged commit ac2bc62 into openclaw:main Aug 16, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 availability 🚨 Merging this PR could cause crashes, hangs, restart loops, stalls, or process outages. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. P2 Normal priority bug or improvement with limited blast radius. proof: sufficient Contributor real behavior proof is sufficient. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants