test(auth): seed the pool quota after the clock is pinned - #3147
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
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Team Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughThe authentication tests now install the fake clock before seeding pool credentials and quota. This aligns ChangesAuthentication test timing
Estimated code review effort: 1 (Trivial) | ~3 minutes Merge Risk: ⚪ Minimal · up to This test-only change aligns seeded quota timestamps with the simulated clock and does not alter production behavior; no actionable merge-blocking risk remains after normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Docstring CoverageExplanation No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 1 files. ✨ 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: 523efb84e7
ℹ️ 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".
| }); | ||
| updateAccountQuota("pool-a", 10, 5); | ||
|
|
||
| const originalNow = Date.now; |
There was a problem hiding this comment.
The removed call belongs to the preceding expired thread affinity test, not the websocket-refresh test being fixed. Without a cached quota, startServer treats pool-a as stale and its background startup prime issues an additional /backend-api/wham/usage request through redirectCanonicalCodexTo, incrementing upstreamRequests beyond the three requests asserted at lines 2151 and 2199 whenever the prime completes in time. Restore this seed after that test pins Date.now, and instead remove the still-present pre-pin seed from the following websocket-refresh test.
Useful? React with 👍 / 👎.
The websocket refresh test still failed on loaded CI runners after #3139, on both macOS and Linux, and dev's own HEAD fails it too — so it was not something any open branch introduced. Two writes stamp real time when they run before the clock is pinned: updateAccountQuota sets updatedAt: Date.now(), and saveCodexAccountCredential sets replacedAt. Everything after the pin reads the pinned 2027 value, so the gap is about 136 days against a 6-hour freshness window (QUOTA_DISK_MAX_AGE_MS, src/codex/quota.ts:491). The seeded state reads as stale no matter how fast the runner is, the startup pool-quota prime refreshes the credential before the first turn is served, and seenAuth[0] is already the new token — which is why the failure diff was always the first element. #3139 pinned the clock and the fetch stub before startServer, closing the window for the prime's own reads. It could not close a window for timestamps written before either was in place. Both seeds now run after the pin. Timing-dependent by nature: the mismatch does not reproduce locally either before or after, so the evidence is the mechanism rather than a local red-to-green. A 136-day gap against a 6-hour window is arithmetic, not a race. Twelve consecutive local runs are clean.
523efb8 to
ecf51c6
Compare
Ingwannu
left a comment
Ingwannu
left a comment
There was a problem hiding this comment.
Requesting changes on exact head ecf51c67f89b45e29303b9c5be49678733008c2e.
The unresolved Codex P1 on this head is valid. The removed updateAccountQuota("pool-a", 10, 5) belongs to the preceding expired thread affinity returns 409... test. Without that fresh cached quota, startServer can run the asynchronous startup prime, send an extra WHAM request through the redirected canonical upstream, and increment upstreamRequests beyond the asserted three whenever the prime wins the timing window. The current full matrix being green does not close a race whose failure depends on that ordering.
Restore that first test quota seed immediately after Date.now = () => now and before startServer(0). Also update its stale comment, which currently says updateAccountQuota above even though no such write remains. The websocket-refresh test can keep both credential and quota seeding after the pinned clock; that part is compatible with the intended fix.
This is the same missing boundary already handled in #3143. Once the first-test seed is restored, rerun the affected file and exact-head matrix. If the new head remains otherwise equivalent and green, this owner-authored PR can supersede #3143 and I will close my duplicate after it lands.
The previous commit removed `updateAccountQuota("pool-a", 10, 5)` from the
`expired thread affinity` test along with the websocket test's own seeds. That
seed belongs to the affinity test, and its comment kept pointing at a call that
was no longer there.
Restore it on the correct side of the clock pin. Note what the comment now
claims and what it does not: seeding after the pin is what keeps the startup
pool-quota prime quiet, because `primeCodexPoolQuotas` treats a missing entry
as stale exactly like an expired one (src/codex/auth-api.ts:1334). It is not a
race fix for `expect(upstreamRequests).toBe(3)` — `redirectCanonicalCodexTo`
only rewrites `/backend-api/codex`, while the prime's WHAM call goes to
`/backend-api/wham/usage` and never reaches the counted upstream.
Verified with `bun test tests/server-auth.test.ts`: 91 pass, 0 fail.
|
Restored on One correction to the reasoning, because I do not want this stored in the history as a race fix when it is not one. The stated failure mode cannot reach the assertion. The removal was also behaviorally neutral. Where you were right: the prime genuinely does reach So the seed is restored on its own merits — the comment was lying, and seeding after the pin is the first time the prime is actually suppressed rather than merely harmless. Verified with Separately, for the merge train: #2789 will not go green on this fix alone. Its macOS job is 17098 pass / 2 fail — the websocket test plus |
* docs(devlog): remote hub restack roadmap — measured conflict surface and blocker reclassification * docs(devlog): fold audit r1 — 10 blockers closed, 33 review threads ledgered, phase ownership corrected * docs(devlog): fix markdown lint in the restack unit (MD018/MD060/MD040/MD004/MD036) * docs(devlog): record wp1 outcome — design rebased, six contract defects closed across two review rounds * docs(devlog): record wp2 outcome — p1 rebased, catalog contract reconciled with the landed #2979 * docs(devlog): record the wp2 commit map and why the fixups are a separate commit * docs(devlog): record wp3 outcome — p2 rebased, plaintext pairing removed, unauthenticated body bounded * docs(devlog): record the wp3 commit map * docs(devlog): record wp4 outcome — p3 rebased, stranded-connection journal defect fixed * docs(devlog): record the wp4 commit map * docs(devlog): record wp5 outcome — p4 rebased, machine plane declared, relay enabled, D1/D2 client side finished * docs(devlog): record the wp5 commit map * docs(devlog): record wp6 outcome — p5 rebased, three of four reported blockers were inherited staleness * docs(devlog): record the wp6 commit map * docs(devlog): record wp7 outcome — p6 rebased, rotation abort ordering and in-flight backup fixed * docs(devlog): record the rebased stack state across all seven phases * docs(devlog): record the exact-head CI repairs and the dev-side macos flake finding * docs(devlog): record the route-registry gap and the dev flake root cause split to #3147 * docs(devlog): close the remote hub restack unit * docs(devlog): note the split-out dev flake PR in the outcome * docs(devlog): audit remote hub exposure, requests, and rollback for a standalone user * docs(devlog): record the exposure, request, and rollback polish * docs(devlog): record the polish commit map and final chain * docs(devlog): describe the lint suppression without writing the directive * docs(devlog): record which axis closed where * docs(devlog): record the per-axis verification commands * docs(devlog): record why the server axis needed no change * docs(devlog): record why rollback was the heaviest axis * docs(devlog): record the post-polish stack state * docs(devlog): plan the stack merge train and the #3147 prerequisite * docs(devlog): correct the merge train plan after the audit refuted the P1 * docs(devlog): record the #3147 seed restore and what it does not fix * docs(devlog): reverse the merge order so the T20 fix lands before its writeup * docs(devlog): correct the fetch-binding claim, replace snapshot greens with merge results, drop out-of-scope files --------- Co-authored-by: jun <jun@lidge.dev>
2 participants
Summary
The websocket refresh test in
tests/server-auth.test.tsstill fails on loaded CI runners after #3139, on both macOS and Linux.dev's own HEAD fails it, so this is not specific to any open branch.updateAccountQuotastampsupdatedAt: Date.now(). The seed ran before the clock was pinned, so that stamp is real wall-clock time while everything after it reads the pinned 2027 value. The gap is roughly 136 days against a 6-hour freshness window (QUOTA_DISK_MAX_AGE_MS,src/codex/quota.ts:491), so the seeded quota reads as stale no matter how fast the runner is. The startup pool-quota prime then refreshes the credential before the first turn is served, andseenAuth[0]is already the new token — which is why the failure diff was always the first element and never the second.#3139 pinned the clock and the fetch stub before
startServer, which closed the window for the prime's own reads. It could not close a window for a timestamp written before either was in place. Moving the seed after the pin does.Verification
bun test tests/server-auth.test.ts— passes.Checklist
devSummary by CodeRabbit