fix(flaky): replace sleep-based races in service.waiters and service.auto-accept tests#85
fix(flaky): replace sleep-based races in service.waiters and service.auto-accept tests#85ggonzalez94 wants to merge 1 commit into
Conversation
…vice tests service.waiters.test.ts "clears all pending waiters" and service.auto-accept.test.ts "does not call hook when invite is invalid" used hardcoded `sleep(50)` to wait for async processing. Under CI load 50 ms is not enough for the execution mutex to release, so the tests intermittently fail. Replace both sleeps with condition-based polling: observe the transport's sent-message count (waiter test) or the rejection result delivery (auto-accept test) to make assertions deterministic regardless of scheduler timing. https://claude.ai/code/session_01VKaHZFWnzKZmN1paVHxdQK
![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: c9f2078b53
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| await waitForCondition("rejected connection/result sent", () => | ||
| transport.sentMessages.some((m) => m.message.method === "connection/result"), |
There was a problem hiding this comment.
In this test, the predicate accepts any connection/result, including an accidental accepted result. The accepted path sends the result before it writes the contact and awaits onConnectionEstablished, so this wait can return as soon as FakeTransport.send() records the message and the negative assertion can run before the hook is invoked. In the invalid-invite scenario this means a regression that accepts the invite could pass; check the result params/status is "rejected" before returning from the wait.
Useful? React with 👍 / 👎.
2 participants
Summary
Fixes two known flaky tests documented in PR #79 ("two pre-existing flaky tests:
service.waiters.test.ts,service.auto-accept.test.ts") and PR #81 ("290/291 — one pre-existing migrate-wallet flake").Root cause
Both tests used hardcoded
await sleep(50)to wait for async processing to complete before making assertions. Under CI resource contention (shared runners, parallel test suites), 50ms is insufficient for theexecutionMutex-gatedconnect()flow to register its waiter or for the async connection-request processor to send a rejection response.Changes
service.waiters.test.ts— "clears all pending waiters when service.stop() is called"await sleep(50)— hopes 50ms is enough forconnect()to acquire the mutex, persist the journal entry, register the waiter, and calltransport.send()await waitFor(() => transport.sentMessages.length === 1)— deterministically waits until the connection/request is sent, which happens immediately after waiter registration (the waiter is registered at line 1142 of service.ts, send at line 1150)service.auto-accept.test.ts— "does not call onConnectionEstablished hook when invite is invalid"await sleep(50)— hopes 50ms is enough for the async reject path to completeawait waitForCondition("rejected connection/result sent", ...)— deterministically waits until the rejectionconnection/resultappears intransport.sentMessages, proving the async processing completed before checking the negative assertionNot addressed
The
migrate-wallet.test.tsflake (PR #81) is caused by OWS vault contention from parallel vitest workers sharing a file-backed wallet store. That requires deeper changes to OWS isolation and is out of scope here.Test plan
bun run lintcleanbun run typecheckcleanhttps://claude.ai/code/session_01VKaHZFWnzKZmN1paVHxdQK
Generated by Claude Code