Skip to content

test(rooms): keep revocation cases deterministic under load - #2184

Merged
milind-soni merged 2 commits into
milind-soni:mainfrom
pauldupuyjr:test/room-coordination-revocation-gate
Oct 9, 2026
Merged

milind-soni merged 2 commits into
milind-soni:mainfrom
pauldupuyjr:test/room-coordination-revocation-gate

Conversation

@pauldupuyjr

@pauldupuyjr pauldupuyjr commented Oct 2, 2026 •

Copy link
Copy Markdown
Contributor

What changed

server/room-coordination.e2e.test.ts only. The three in-flight revocation cases ("withholds a cross-team result if the owner revokes access while it runs" and both variants of "withholds an in-flight result after team %s and recreation") now hold the delegated child on a gateFile instead of a fixed delayMs window:

  • The child is held after its MCP calls, using the gate support server/testing/room-handoff-agent.ts already has (the same pattern as the existing queued-dispatch and section-change cases).
  • The test lands the access change while the child is running. A new helper, releaseAfterWithhold, waits until the child is marked failed, then releases the gate.
  • The "running" poll budget goes from 15 s to 30 s, and each case's timeout from 45 s to 60 s so the new budget fits.

Every assertion is unchanged. No production code changes, and the scripted agent doesn't change.

Why

The child was scripted to stay "running" for 3 s (5 s in the team cases). The test had to see "running", land the access change, and have the periodic revalidation of the running child withhold the result, all inside that window. Under load the 15 s poll for "running" could time out before the child started (expected undefined to be running). It also happened when two suites shared one machine. A fixed window either expires too soon, or lets the child complete before revalidation runs.

Releasing the gate right after the access change isn't enough either. That variant fails deterministically with expected 'completed' to be 'failed', because the withhold is enforced by revalidation, not at the moment of the PATCH. Holding the gate until the child is failed removes both timing dependencies.

The longer budgets only matter when a case fails. In passing runs each revocation case still takes about 3 s, the same as before.

How it was verified

  • Linux (Docker), full suite twice with the base and branch suites running concurrently, which is the load that produced the flake. Both runs: 11256/11317 passed on the branch, identical to the base at 033fd71. No branch-only failures, and this file passes in both.
  • After factoring the release step into the helper, this file ran 3 times in a row under load: 120/120 passed.
  • pnpm typecheck, pnpm lint and pnpm i18n:check pass, and vitest run server/room-coordination.e2e.test.ts passes on its own.
  • The "release immediately" variant was run and fails as described above. That confirms the poll for failed before release is needed.

Checklist

  • pnpm typecheck and pnpm test pass locally (Linux; the full suite ran twice as above)
  • Server behavior changes come with tests (test-only change)
  • No dist-server/ edits (it's build output)
  • macOS-only code is platform-gated; no shell: true / cmd.exe string-building
  • No secrets in logs, responses, events, or argv

Summary by CodeRabbit

  • Tests
    • Improved automated checks for access changes during in-progress handoffs, including scenarios involving team changes. These checks now better accommodate timing variation and verify that access changes are handled before gated results are released.

…r load

The two in-flight revocation cases raced a 3-5s scripted window and could go red under pair load (pair 20260929-221744). Hold the child on a gate file after its MCP calls, land the access change while it runs, keep it held until the periodic revalidation of the running child enforces the withhold, then release the gate, with a 30s poll budget and a 60s case timeout.

Co-Authored-By: GLM-5.3 <noreply@z.ai>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@vercel

vercel Bot commented Oct 2, 2026

Copy link
Copy Markdown

@pauldupuyjr is attempting to deploy a commit to the SupaMaus Team on Vercel.

A member of the Team first needs to authorize it.

@coderabbitai

coderabbitai Bot commented Oct 2, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 33 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used all 8 included reviews currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration
  • Configuration used: defaults
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 62537e74-5286-4011-b698-6902e9409296

📥 Commits

Reviewing files that changed from the base of the PR and between b75af6f and cb3a66c.


📒 Files selected for processing (1)
  • server/room-coordination.e2e.test.ts


No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: 6db9f5bd-892e-460c-bf3d-97a0e9e65d80


📥 Commits

Reviewing files that changed from the base of the PR and between b7a7035 and b75af6f.


📒 Files selected for processing (1)
  • server/room-coordination.e2e.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 7 remain after this review.



📝 Walkthrough

Walkthrough

The room coordination end-to-end tests now gate recipient results and release them after the child node fails. The affected tests also use longer polling and overall timeout limits.

Changes

Room coordination test synchronization

Layer / File(s) Summary
Gate and release results in coordination tests
server/room-coordination.e2e.test.ts
A shared helper waits for the child node to fail before releasing the gated result. The revocation and team recreation tests use the helper and increase polling and test timeouts.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Other

Suggested reviewers: milind-soni


Merge Risk: ⚪ Minimal · up to b75af

The change is limited to end-to-end test synchronization, and the timeout path has bounded cleanup. The supplied test and check results are green; no material merge-blocking risk remains.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly identifies the test change and its purpose: keeping room revocation cases deterministic under load.
Description check ✅ Passed The description covers the required change, rationale, verification steps, and checklist. It also explains why the gate-based approach is necessary and reports relevant test results.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 1 files.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.


✨ Finishing Touches 💡 1
🛠️ Fix failing CI checks 💡
  • Commit to this branch
  • Create a new PR

🧪 Generate unit tests (beta)
  • Create a new PR


  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Autopilot is currently an internal CodeRabbit preview.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@milind-soni

Copy link
Copy Markdown
Owner

Reviewed the gate against the scripted agent and running-child revocation flow. Merged current main locally: the three targeted cases pass, and the complete room-coordination file passes all 40 tests. Assertions remain intact; only the timing window is replaced with observable state. Ready for a refreshed CI run after shared baseline repair #2528.

@milind-soni
milind-soni enabled auto-merge (squash) October 9, 2026 11:29
@milind-soni
milind-soni merged commit b2b645a into milind-soni:main Oct 9, 2026
23 of 24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants