fix(gui): bound the 401 session re-bootstrap and make token resolution abort-aware - #1855
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (2)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
✅ Deterministic PR hygiene checks passed. |
✅ READY
UI screenshot waived by the Hygiene✅ Deterministic PR hygiene checks passed. |
lidge-jun
added
the
gui-screenshot-waived
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 91e4e8e064
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const stored = storeSession( | ||
| metaContentFromHtml(html, "opencodex-session-token"), | ||
| metaContentFromHtml(html, "opencodex-session-csrf"), | ||
| metaContentFromHtml(html, "opencodex-session-origin"), | ||
| ); |
There was a problem hiding this comment.
When the watchdog wins because a bootstrap fetch ignores its abort signal, reBootstrapSessionToken() continues running outside the race. If a later resolution stores a valid session and the abandoned response subsequently settles—for example, after a proxy restart or delayed fault-injection release—this unconditional storeSession overwrites the newer credentials with the late session, which may already be invalid and can restart the 401 cycle. Return parsed credentials without mutating global state, then store them only after that bootstrap is confirmed as the winning resolution (or guard the mutation with a resolution generation).
Useful? React with 👍 / 👎.
…n abort-aware The shared resolutionInFlight promise awaited reBootstrapSessionToken with no timeout and no caller signal: one stalled /opencodex-session wedged every /api/* fetch for the page lifetime (loopback sessions expire every 5 minutes, so it re-armed periodically). The bootstrap is now bounded (10s) and returns a tri-state minted/unavailable/failed; only a definitive refusal (4xx or meta-less OK) reaches the admin-token prompt, while transient failures (timeout, abort, network, 5xx) fail the wave and re-arm on the next 401. Callers race the shared resolution against their own abort signal without cancelling it for others, and the race listener is always removed. New tests: api-auth-deadline (4 cases). Evidence: devlog/_plan/260816_gui_loading_performance/001 (E2)
…artifact correction Live verification found the residual: a bootstrap fetch that never honors the client abort (debugger-held or quirky network stack) leaves the shared resolution body pending forever even with the 10s fetch bound, re-wedging every /api waiter. A 15s whole-resolution watchdog now unwraps the wave as failed, and the conditional clear lets the next 401 start a fresh resolution (a late zombie settle cannot wipe it). Also corrects 001/E3: the post-heal non-recovery observation was a harness artifact (raw CDP rejects Fetch.disable); with interception properly cleared, all tabs recover automatically without reload.
…rompt Review round r3 (FAIL, one High) caught it: a whole-body watchdog fires 15s into a user-controlled admin-token prompt on non-loopback dashboards, and since promptForAdminToken has no singleton guard each subsequent poll wave stacked another modal, with cancel poisoning promptCancelled. The watchdog now races only the bootstrap call inside the shared body; while the prompt pends, later waves join the same body (single dialog). New regression test: watchdog never bounds the prompt, waves join, single prompt across a >watchdog window.
1ae1011 to
63cd5d2
Compare
91e4e8e to
2511302
Compare
1 participant
Summary
Layer 2 of the loading-resilience stack. Layer 1 bounded each resource fetch; this one removes the app-wide chokepoint that could pin every management request at once.
On any 401 the fetch wrapper joins a single shared token resolution, and that resolution awaited
rawFetch("/opencodex-session")with no timeout and no caller signal. One stalled bootstrap therefore froze every/api/*request for the lifetime of the page — caller aborts did nothing, and the shared promise only cleared in afinallythat never ran. Loopback sessions expire every five minutes, so the exposure re-armed periodically, which is exactly the "refresh fixes it for a while, then it sticks again" symptom.Measured on a sandboxed instance with the bootstrap stalled and the server otherwise healthy:
/api/*traffic dropped to zero for 40s+ while/healthzkept polling, and cold tabs showed permanent skeletons.Three changes:
Design and evidence:
devlog/_plan/260816_gui_loading_performance/020_phase2_auth_unwedge.mdStack (merge bottom-up): #1854 (resource deadline) ← this PR ← hidden pause ← poll consolidation.
Depends on #1854. Review this PR's diff only.
Verification
cd gui && bun test tests→ 922 pass / 0 failcd gui && bun run lint,bun run lint:i18n,bun run build→ greengui/tests/api-auth-deadline.test.ts(6 cases): hung bootstrap fails the wave and a later wave re-bootstraps to success; timeout and 5xx never open the prompt while a 4xx refusal does; a caller abort unwinds only that caller with balanced listener add/remove; no page-lifetime poisoning; the retry carries the caller signal; and a signal-dropping zombie bootstrap is bounded while the prompt is never watchdog-bounded.Audit note: the first review round failed this layer — the watchdog originally raced the whole resolution body, which would have stacked a new admin-token modal every ~15s on non-loopback dashboards. Fixed and re-reviewed before this PR.
No visual change: this is auth/request plumbing.
Checklist