test: stabilize stopped sandbox websocket test#585
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a test helper ChangesTest Race Condition Fixes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
Terraform Validation Results
Pushed by: @ColeMurray, Action: |
![open-inspect[bot]](https://avatars.githubusercontent.com/u/2492022?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary
This PR refactors the websocket sandbox integration test to wait for the init-triggered warm-up attempt before asserting the stopped/ready paths. The intent is good, but the new shared polling helper still lets the test proceed when the expected status never arrives, so the flake can remain hidden instead of failing deterministically.
PR Title and number: test: stabilize stopped sandbox websocket test (#585)
Author: @ColeMurray
Files changed: 1
Additions/deletions: +19 / -10
Critical Issues
- [Functionality & Correctness]
packages/control-plane/test/integration/websocket-sandbox.test.ts:17-waitForSandboxStatus()exits silently on timeout. If CI is slow and the warm-up has not yet transitioned tofailedwithin 3s, the stopped-sandbox test still continues and can race with the in-flight warm-up, which is the exact flake this PR is trying to remove. Please fail explicitly after the polling loop (for example by throwing with the last observed status) so unmet preconditions are surfaced deterministically.
Suggestions
- [Code Quality & Readability]
packages/control-plane/test/integration/websocket-sandbox.test.ts:7- Once the helper throws on timeout, consider including the target status and last observed status in the error message to make future CI failures easier to diagnose.
Nitpicks
- None.
Positive Feedback
- The extraction of the polling logic removes duplication between the stopped and ready websocket tests.
- Waiting for the init-triggered warm-up to settle is the right direction for addressing the race behind the flaky integration test.
- The PR stays narrowly scoped to the affected test file, which keeps the intent easy to review.
Questions
- None.
Verdict
Request Changes
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/control-plane/test/integration/websocket-sandbox.test.ts (1)
85-103:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReplace the fixed 100ms sleep with the same status poll.
The new helper is the right primitive here, but the post-
accept()setTimeout(100)still leaves a timing window on slower CI workers. Poll for"ready"instead of sleeping so the test only proceeds once the DO has actually processed the connect handler.🛠️ Proposed fix
expect(ws).not.toBeNull(); ws!.accept(); - - // Small delay to ensure DO has processed the connect handler fully - await new Promise((r) => setTimeout(r, 100)); + await waitForSandboxStatus(stub, "ready"); const stateRes = await stub.fetch("http://internal/internal/state"); const state = await stateRes.json<{ sandbox: { status: string } }>();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/control-plane/test/integration/websocket-sandbox.test.ts` around lines 85 - 103, After calling ws!.accept() in the "sandbox connect sets status to ready" test, remove the fixed sleep (the setTimeout(100) promise) and instead poll for the sandbox to reach the "ready" status using the existing helper: call await waitForSandboxStatus(stub, "ready") so the test waits deterministically for the DO connect handler to complete; keep the rest of the test (initNamedSession, seedSandboxAuth, openSandboxWs, ws!.accept()) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/control-plane/test/integration/websocket-sandbox.test.ts`:
- Around line 7-18: Hoist the default timeout into a single named constant
(e.g., DEFAULT_WAIT_FOR_SANDBOX_MS) and use it as the default value for
waitForSandboxStatus(timeoutMs = DEFAULT_WAIT_FOR_SANDBOX_MS); inside
waitForSandboxStatus (which takes stub: DurableObjectStub and uses queryDO<{
status: string }>(stub, "SELECT status FROM sandbox")), when the loop exits
without seeing the expected status throw an Error that includes the expected
status and the timeout so the test will fail instead of silently continuing.
---
Outside diff comments:
In `@packages/control-plane/test/integration/websocket-sandbox.test.ts`:
- Around line 85-103: After calling ws!.accept() in the "sandbox connect sets
status to ready" test, remove the fixed sleep (the setTimeout(100) promise) and
instead poll for the sandbox to reach the "ready" status using the existing
helper: call await waitForSandboxStatus(stub, "ready") so the test waits
deterministically for the DO connect handler to complete; keep the rest of the
test (initNamedSession, seedSandboxAuth, openSandboxWs, ws!.accept()) unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b7a8cce9-d0db-47b2-9907-65497104eccb
📒 Files selected for processing (1)
packages/control-plane/test/integration/websocket-sandbox.test.ts
Terraform Validation Results
Pushed by: @open-inspect[bot], Action: |
1 participant
Summary
stoppedstate.Testing
npm run test:integration -w @open-inspect/control-plane -- websocket-sandbox.test.tsCreated with Open-Inspect
Summary by CodeRabbit