test(auth): pin the websocket refresh account to stop a cross-platform flake - #3128
Conversation
|
✅ Deterministic PR hygiene checks passed. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
github-actions
Bot
added
the
chore
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Team Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe WebSocket auth-refresh test now routes both requests through the ChangesWebSocket auth-refresh test
Estimated code review effort: 1 (Trivial) | ~3 minutes Suggested reviewers: ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![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: 95144b84cd
ℹ️ 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".
| { id: "main", email: "main@example.test", isMain: true }, | ||
| { id: "pool-a", email: "pool@example.test", isMain: false, chatgptAccountId: "acct-pool-a" }, | ||
| ], | ||
| codexAccountNamespaces: { "ws-refresh": "pool-a" }, |
There was a problem hiding this comment.
When startup work reads the real clock before Date.now is overridden—the scenario this change is intended to stabilize—this namespace does not prevent the early refresh: activeCodexAccountId already selects pool-a, it is the only stored pool credential, and both the fixed and ordinary paths ultimately call getValidCodexToken, whose freshness check still uses Date.now(). The first request can therefore still observe new-access-token; set the clock before startServer and the quota/credential setup rather than changing the routing path under test.
Useful? React with 👍 / 👎.
…l mid-fixture (#3139) * docs(devlog): plan merge train round 3 Roadmap for landing the green PRs, retiring the superseded ones, and rebasing the rest, frozen at dev=132b557ad. Includes the round-1 audit synthesis: three blockers folded (fork PRs are carried by cherry-pick rather than force-pushed, because enforce-pr-target.yml applies the readiness checklist to authors without push permission; #3039's closure withdrawn because #3104 prints the configured budget where #3039 printed the elapsed wait; the src/service.ts overlap is 330470e, not 0ef04e6) and two rebutted with evidence. * docs(devlog): record wp1 — #3114 landed as abcda8e * docs(devlog): record the wp2 security review for #3122 * docs(devlog): record wp3 — #3134 landed, #3128 flake premise corrected * docs(devlog): record wp5 — #3077 closed, #3109/#3112 rebased * docs(devlog): locate the websocket refresh flake, and correct the #3128 premise * docs(devlog): prove the flake mechanism and correct its direction * docs(devlog): mark the superseded flake explanation in the wp5 record * test(auth): install the fake clock and fetch stub before startServer startServer returns synchronously but arms an async pool-quota prime that outlives its return (src/server/index.ts:2054-2064). That prime calls getValidCodexToken, which can rotate the very credential these assertions read, and fetches a real host unless the stub is up. Both fixtures installed Date.now and globalThis.fetch AFTER startServer, leaving a window two dynamic import() resolutions wide where the prime ran against the real clock and real fetch. On a warm local module cache it resolved before the fixture finished; on a loaded CI runner it did not, and seenAuth[0] was already the rotated token. Measured rather than assumed: OPENCODEX_DEBUG_QUOTA=1 prints refreshed=1 on every run of both the fixed and unfixed trees, so the prime always fires. The fix does not suppress it -- it makes it run inside the fixture's controlled world. The thread-affinity test at :2131 had the identical shape and is fixed too.
Five landings, six closures, two rebases, and the flake that held the last PR. Records what the round is evidence of rather than only what it did: three explanations were written for the websocket flake and two were wrong, both plausible enough to justify the same fix -- caught by activation evidence, not review. The '#3128 fixed that flake' citation was repeated across three PRs and taught reviewers to dismiss a red that was real.
1 participant
Summary
Cherry-picks
926a8d8c4out of #3109 onto its own branch. Two lines of test change; no product code.tests/server-auth.test.ts—websocket passthrough refreshes pool auth for each response.create turn— is a real cross-platform flake. It failed three times during the v2.39.0 release train: twice on macOS (preview PR run, preview push run) and once on Linuxtest 3/4(main push run). Always the same assertion at:2288, always the first array element, always green on rerun.Why it races
The credential is stored with
expiresAt: now + 120_000whileREFRESH_SKEW_MSis60_000(account-store.ts:22), and the refresh predicate iscred.expiresAt > Date.now() + REFRESH_SKEW_MS(:717) — only 60 s of clearance.startServer(0)runs at:2245, beforeDate.nowis pinned at:2249, so work in that window reads the real clock. When the first turn lands on the wrong side of the skew boundary the refresh fires early andseenAuth[0]is already the new token. The second element is always correct, which is the signature of an early first refresh rather than a missing second one.Not runner slowness: nothing times out, so the 30 s CI watchdog floor in
tests/helpers/ci-watchdog.tsis not involved.The fix
Pin the account namespace (
codexAccountNamespaces: { "ws-refresh": "pool-a" }) and route both turns throughws-refresh/gpt-test, so the turn resolves to a fixed account instead of racing account selection against the skew window.Why it is being carried here
The commit was authored on
codex/3063-combo-compact-failoverand rides on #3109, which is about routing combo compact requests through the failover path. The two are unrelated, and this flake taxes every release train in the meantime — three reruns and roughly 45 minutes on v2.39.0 alone. #3109 keeps its own commit; this branch carries the same change independently so it can land on its own schedule.Authorship is preserved by cherry-pick.
Verification
git diff origin/dev --statistests/server-auth.test.ts | 5 +++--— one file, three insertions, two deletions. Test-only; nosrc/change, so no product behavior moves.Checklist
Summary by CodeRabbit