Skip to content

test(auth): wait for the reconcile's whole batch before draining access events - #373

Merged
mubeda merged 1 commit into
mainfrom
fix/auth-revoke-test-drain-race
Nov 15, 2026
Merged

mubeda merged 1 commit into
mainfrom
fix/auth-revoke-test-drain-race

Conversation

@mubeda

@mubeda mubeda commented Nov 15, 2026

Copy link
Copy Markdown
Owner

Fixes #372

What

One line in the revoke-in-the-gap test: before it drains the access events, it takes the state lock once. A reconcile sends its changes one by one under that lock, and a receiver sees each send at once, so the marker could arrive while the ClientRemoved of the same batch was still unsent.

No product code changes.

Evidence

  • main failed once on this test with "the access stream resurrected the revoked client" (linked in Flaky: the revoke-in-the-gap auth test reports a resurrected client #372).
  • Forced locally with a temporary pause in the watcher and between the sends (not committed): 20 of 20 runs failed with the same assertion. With the fix under the same pause: 0 of 20 failed. Without the pause: 0 of 100 failed.
  • The race was not observed on CI directly; the panic text alone does not name the interleaving.

Validation

  • cargo fmt --all --check
  • cargo clippy -p bibcode-server --lib --tests -- -D warnings
  • the auth module's tests (134 passed)
  • vp check --no-fmt, vp run typecheck
  • Codex reviewed the change: correct for this race, does not weaken the test, no other test in the file has the same defect.

Not run: native Windows and Linux test runs (CI covers them). Testing runbooks reviewed and remain accurate. Profiler coverage: none applies (test only).

🤖 Generated with Claude Code

…ss events

The revoke-in-the-gap test read the marker link's event and then drained the
channel without waiting. A reconcile sends its changes one by one under the
state lock, and a receiver sees each send at once, so the drain could run
before the ClientRemoved of the same batch was sent. The test then reported
a resurrected client that the stream removed a moment later.

Seen once on main in the Test job. With the watcher's batch slowed by a
temporary probe the test failed 20 of 20 times; with the lock wait it passed
20 of 20 under the same probe and 100 of 100 without it.

Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Nov 15, 2026 •

Copy link
Copy Markdown
Codex Review Summary: ✅ Completed

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-11-15T00:31:05.886641Z 241ebf3 PR opened
🔒 Security Review ✅ Completed 2026-11-15T00:31:59.803343Z 241ebf3 PR opened
ℹ️ 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" or "@codex security review".

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 github-actions Bot added vouch:trusted PR author is trusted by repo permissions or the VOUCHED list. size:XS labels Nov 15, 2026
@mubeda
mubeda merged commit 27a32e8 into main Nov 15, 2026
23 checks passed
@mubeda
mubeda deleted the fix/auth-revoke-test-drain-race branch November 15, 2026 01:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:XS vouch:trusted PR author is trusted by repo permissions or the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Flaky: the revoke-in-the-gap auth test reports a resurrected client

1 participant