test(socket.io): drop 200ms guard timers in connection-state-recovery#35132
Conversation
The connection-state-recovery tests drive the Engine.IO long-polling transport by hand via supertest: the heaviest case performs 8 sequential HTTP round trips. When these were ported from upstream socket.io in fe74c94, each test gained a 200ms setTimeout guard that calls fail() and closes the server. On the debian-13 x64-asan lane (introduced in #34782) the first test consistently runs at ~220ms, so the 200ms guard fires mid-work and fails the test with 'error: timeout'. Seen on builds 77055 (219.84ms) and 77789 (219.96ms). The guard timers are not part of the upstream test (which is plain async and simply calls io.close() at the end). They duplicate the test runner's own timeout and turn a slow-but-correct run into a failure. Drop the timers and align with upstream: plain async tests that await the protocol events, with io.close() in a finally block. Same assertions, same code paths, no wall-clock race.
|
Warning Review limit reached
Next review available in: 21 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
Comment |
|
Updated 9:06 AM PT - Jul 22nd, 2026
❌ @robobun, your commit 70a1c05 has 3 failures in
🧪 To try this PR locally: bunx bun-pr 35132That installs a local version of the PR into your bun-35132 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — mechanical de-flake that removes wall-clock races without weakening any assertion.
What was reviewed:
- All 7 tests: every original
expectis preserved verbatim;io.close()moves fromsuccess()/fail()intofinally, so cleanup still runs on every path. - Last test's negative contract (
persistSession/restoreSessionnot called) is now an explicitexpect(called).toBe("")after all protocol steps — strictly stronger than the oldfail()-in-callback which raced the main flow'sdone(). fail/successutil exports remain live in 9 sibling test files, so no dead code introduced.
Extended reasoning...
Overview
Test-only change to test/js/third_party/socket.io/socket.io-connection-state-recovery.test.ts. Converts 7 tests from done-callback style wrapped in an async IIFE with a 200ms setTimeout guard to plain async tests with try/finally { io.close() }. No runtime code, no assertion values changed.
Security risks
None. Third-party integration test file; no auth, crypto, or production code paths touched.
Level of scrutiny
Low. This is exactly the anti-pattern REVIEW.md calls out — a literal setTimeout guard that races real work against a wall clock. The 200ms was never a behavioral assertion (upstream socket.io has no such timer); the test-runner's own per-test timeout still bounds hangs. The refactor to async + finally matches the repo's stated conventions (cleanup registered before assertions, no throwing inside event callbacks).
Other factors
- Cleanup semantics preserved:
success()/fail()both calledio.close(); nowfinallydoes. Thefinallyplacement means the server is released even if anexpectin thetrythrows — same as before via the oldcatch { fail(...) }. - The DummyAdapter test's negative contract moved from
fail(done, ...)inside override callbacks (which could race the IIFE'ssuccess(done, ...)) to a recorded flag asserted after the protocol completes — the correct shape for "X does not happen" per REVIEW.md. io.once("connection", ...)handlers are still registered before the awaits that trigger them; ordering unchanged.- PR description shows before/after verification on debug+ASAN (0→7 pass) and release (7 pass), which is the right check for a de-flake.
|
The changed file passes on every lane. Build 77814 reds are unrelated:
Self-review: no surviving concerns. Ready for a maintainer. |
2 participants
Fixes
test/js/third_party/socket.io/socket.io-connection-state-recovery.test.tsgoing red on the debian-13 x64-asan lane.Failure
Seen on builds 77055 (219.84ms) and 77789 (219.96ms), both debian-13 x64-asan.
Cause
Each test in this file carries a
setTimeout(() => fail(...), 200)guard that was added when the suite was ported from upstream socket.io in fe74c94. The upstream test has no such timers.These tests drive the Engine.IO long-polling transport by hand via supertest: the first test performs 8 sequential HTTP round trips. On release that is ~50ms; on release+ASAN it is ~220ms. #34782 moved the x64-asan build lane from amazonlinux-2023 to debian-13 on Jul 21, and the failures started immediately after, with the work landing just past the 200ms guard every time.
The guard is not a behavioral assertion. It races real work against a wall clock and turns a slow-but-correct run into a failure. The test runner already applies its own per-test timeout (90s, tripled under ASAN in CI).
Fix
Drop the guard timers and align with the upstream structure: plain
asynctests that await the protocol events, withio.close()in afinallyblock so the server is always released. Every assertion is preserved; the same code paths are exercised.Verification
[stamp-90s] gate passed · iteration 0 · 1 files touched
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file
self-review · no surviving concerns
25 concerns were raised and did not survive verification.
root cause · written by the author bot
The test wrapped its async work in a hardcoded 200ms setTimeout guard that was not a behavioral assertion, and on slow ASAN builds the real protocol steps outran that window, so the guard fired and failed the test even though the code under test behaved correctly. The fix removes the wall-clock guards entirely and converts each test to a plain async function that awaits the protocol steps directly, with server cleanup moved into a try/finally block. Hangs are now bounded by the test runner's per-test timeout rather than an arbitrary 200ms race, and all original assertions are preserved.