test(control-plane): accept running status in waitForSandboxSpawn#651
Conversation
The helper polls for status='connecting' every 10ms, but with the fetch mock installed in #643 the createSandbox response now resolves synchronously, so the connecting -> running transition can complete between polls. On a fast runner the loop would never observe 'connecting' and would throw "Timed out waiting for sandbox spawn" after 1s. Broaden the predicate to accept either 'connecting' or 'running' — both indicate the spawn has progressed past init, which is the precondition the cancel test actually needs.
📝 WalkthroughWalkthroughThis PR updates the sandbox spawn detection logic in an integration test. The ChangesSandbox Spawn Detection
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~5 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. 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
PR #651, test(control-plane): accept running status in waitForSandboxSpawn, by @ColeMurray updates one control-plane integration test helper to treat both connecting and running sandbox statuses as sufficient progress. The change is narrowly scoped and correctly removes a polling race introduced by the synchronous fetch mock.
Files changed: 1, additions/deletions: +2/-1.
Critical Issues
None.
Suggestions
None.
Nitpicks
None.
Positive Feedback
- The helper now waits for the actual test precondition: a sandbox spawn that has progressed past initial creation, rather than one transient intermediate status.
- The change is minimal and keeps the polling logic easy to read.
- This should reduce integration test flakiness without weakening the cancel-path assertion.
Questions
None.
Verdict
Approve: Ready to merge.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/control-plane/test/integration/do-internal-routes.test.ts (1)
36-36: ⚡ Quick winDefine timeout values as named constants with unit suffixes.
The timeout values
1000(line 36) and10(line 44) are hardcoded literals. As per coding guidelines, TypeScript timeouts should be defined as named constants with millisecond units in the variable name (e.g.,TIMEOUT_MS), and each default timeout should be defined exactly once rather than restated.♻️ Refactor to use named constants
Add constants at the top of the file or in a shared constants module:
+const SANDBOX_SPAWN_TIMEOUT_MS = 1000; +const SANDBOX_POLL_INTERVAL_MS = 10; + async function waitForSandboxSpawn(stub: DurableObjectStub): Promise<void> { - const deadline = Date.now() + 1000; + const deadline = Date.now() + SANDBOX_SPAWN_TIMEOUT_MS; while (Date.now() < deadline) { const rows = await queryDO<{ status: string }>(stub, "SELECT status FROM sandbox LIMIT 1"); const status = rows[0]?.status; if (status === "connecting" || status === "running") { return; } - await new Promise((resolve) => setTimeout(resolve, 10)); + await new Promise((resolve) => setTimeout(resolve, SANDBOX_POLL_INTERVAL_MS)); }As per coding guidelines: "Use seconds for Python timeouts and milliseconds for TypeScript timeouts, encoding the unit in variable names (Python:
timeout_seconds, TypeScript:timeoutMs,TIMEOUT_MS)" and "Define each default timeout value exactly once as a named constant and import it everywhere rather than restating literal values".Also applies to: 44-44
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/control-plane/test/integration/do-internal-routes.test.ts` at line 36, Replace the hardcoded numeric timeout literals with named millisecond constants: introduce a top-level constant like DEFAULT_TIMEOUT_MS = 1000 (or TIMEOUT_1S_MS) and another constant for the other literal (e.g., SHORT_TIMEOUT_MS = 10_000 if that 10 was seconds or SHORT_TIMEOUT_MS = 10 if it truly is ms) and use those instead of the raw numbers where the test sets deadline (variable deadline) and the other timeout occurrence (the literal 10). Update the test to compute deadline using DEFAULT_TIMEOUT_MS (e.g., const deadline = Date.now() + DEFAULT_TIMEOUT_MS) and replace the other raw 10 with the corresponding named constant so each default timeout value is defined once and reused.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/control-plane/test/integration/do-internal-routes.test.ts`:
- Line 36: Replace the hardcoded numeric timeout literals with named millisecond
constants: introduce a top-level constant like DEFAULT_TIMEOUT_MS = 1000 (or
TIMEOUT_1S_MS) and another constant for the other literal (e.g.,
SHORT_TIMEOUT_MS = 10_000 if that 10 was seconds or SHORT_TIMEOUT_MS = 10 if it
truly is ms) and use those instead of the raw numbers where the test sets
deadline (variable deadline) and the other timeout occurrence (the literal 10).
Update the test to compute deadline using DEFAULT_TIMEOUT_MS (e.g., const
deadline = Date.now() + DEFAULT_TIMEOUT_MS) and replace the other raw 10 with
the corresponding named constant so each default timeout value is defined once
and reused.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 12996bcb-1589-4c98-9dd8-ecb79792d8ba
📒 Files selected for processing (1)
packages/control-plane/test/integration/do-internal-routes.test.ts
1 participant
Summary
Follow-up to #643. The
waitForSandboxSpawnhelper polls forstatus = 'connecting'every 10ms (1s deadline), but with the fetchmock installed in #643 the
createSandboxresponse resolvessynchronously — so on a fast runner the
connecting → runningtransition can complete between two polls. The loop would never
observe
connecting, then throw"Timed out waiting for sandbox spawn".This was the residual race I flagged at review time. Broadening the
predicate to accept either
connectingorrunningremoves it —both states indicate the spawn has progressed past init, which is the
actual precondition the cancel test needs before driving
status = 'active'.Test plan
npm run test:integration -w @open-inspect/control-plane -- test/integration/do-internal-routes.test.tspasses locally.npm run typecheck -w @open-inspect/control-planeclean.npm run lint -w @open-inspect/control-planeclean.npx prettier --checkclean.Summary by CodeRabbit