test(control-plane): stabilize cancel integration test#643
Merged
ColeMurray merged 1 commit intoMay 17, 2026
Merged
Conversation
📝 WalkthroughWalkthroughThe PR enhances the DO internal routes integration test suite by adding fetch mocking and sandbox spawn synchronization. It introduces a ChangesDO Internal Routes Test Infrastructure
Possibly related PRs
Poem
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 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 unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
![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
🧹 Nitpick comments (1)
packages/control-plane/test/integration/do-internal-routes.test.ts (1)
46-46: ⚡ Quick winConsider including actual status in timeout error.
The error message could include the actual sandbox status to aid debugging when the timeout occurs.
🔍 Proposed enhancement to error message
+ const finalStatus = rows[0]?.status ?? "no row"; - throw new Error("Timed out waiting for sandbox spawn"); + throw new Error(`Timed out waiting for sandbox spawn (last status: ${finalStatus})`); }🤖 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 46, The timeout throw currently uses a static message ("Timed out waiting for sandbox spawn"); update the Error construction in do-internal-routes.test.ts to include the actual sandbox status/state when timing out (e.g., append the value of the relevant status variable in scope such as status or sandbox.status) so the thrown Error message reports both the timeout and the current sandbox status for easier debugging.
🤖 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.
Inline comments:
In `@packages/control-plane/test/integration/do-internal-routes.test.ts`:
- Around line 35-47: Extract the magic timeout literals in waitForSandboxSpawn
into named constants (e.g., SANDBOX_SPAWN_TIMEOUT_MS and
SANDBOX_POLL_INTERVAL_MS) defined once at the top of the file (after imports)
with the Ms suffix to indicate milliseconds, then replace the hardcoded 1000 and
10 in waitForSandboxSpawn with those constants; if these timeouts are needed
elsewhere, export the constants for reuse instead of repeating literals.
---
Nitpick comments:
In `@packages/control-plane/test/integration/do-internal-routes.test.ts`:
- Line 46: The timeout throw currently uses a static message ("Timed out waiting
for sandbox spawn"); update the Error construction in do-internal-routes.test.ts
to include the actual sandbox status/state when timing out (e.g., append the
value of the relevant status variable in scope such as status or sandbox.status)
so the thrown Error message reports both the timeout and the current sandbox
status for easier debugging.
🪄 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: cd256119-1d4b-4089-bac5-f4bae9600936
📒 Files selected for processing (1)
packages/control-plane/test/integration/do-internal-routes.test.ts
Comment on lines
+35
to
+47
| async function waitForSandboxSpawn(stub: DurableObjectStub): Promise<void> { | ||
| const deadline = Date.now() + 1000; | ||
|
|
||
| while (Date.now() < deadline) { | ||
| const rows = await queryDO<{ status: string }>(stub, "SELECT status FROM sandbox LIMIT 1"); | ||
| if (rows[0]?.status === "connecting") { | ||
| return; | ||
| } | ||
| await new Promise((resolve) => setTimeout(resolve, 10)); | ||
| } | ||
|
|
||
| throw new Error("Timed out waiting for sandbox spawn"); | ||
| } |
There was a problem hiding this comment.
Extract timeout literals to named constants with unit suffixes.
Lines 36 and 43 use literal timeout values (1000 and 10). As per coding guidelines, TypeScript timeout values should be defined as named constants with Ms suffix and defined once rather than restated as literals.
⏱️ Proposed fix to extract timeout constants
Add constants at the top of the file (after imports):
+const SANDBOX_SPAWN_TIMEOUT_MS = 1000;
+const SANDBOX_SPAWN_POLL_INTERVAL_MS = 10;
+
const originalFetch = globalThis.fetch;Then update the function:
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");
if (rows[0]?.status === "connecting") {
return;
}
- await new Promise((resolve) => setTimeout(resolve, 10));
+ await new Promise((resolve) => setTimeout(resolve, SANDBOX_SPAWN_POLL_INTERVAL_MS));
}
throw new Error("Timed out waiting for sandbox spawn");
}As per coding guidelines: Use seconds for Python timeouts and milliseconds for TypeScript timeouts, encoding the unit in variable names (TypeScript: timeoutMs, TIMEOUT_MS). Define each default timeout value exactly once as a named constant and import it everywhere rather than restating literal values.
🤖 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` around
lines 35 - 47, Extract the magic timeout literals in waitForSandboxSpawn into
named constants (e.g., SANDBOX_SPAWN_TIMEOUT_MS and SANDBOX_POLL_INTERVAL_MS)
defined once at the top of the file (after imports) with the Ms suffix to
indicate milliseconds, then replace the hardcoded 1000 and 10 in
waitForSandboxSpawn with those constants; if these timeouts are needed
elsewhere, export the constants for reuse instead of repeating literals.
5 tasks
ColeMurray
added a commit
that referenced
this pull request
May 17, 2026
## Summary Follow-up to #643. The `waitForSandboxSpawn` helper polls for `status = 'connecting'` every 10ms (1s deadline), but with the fetch mock installed in #643 the `createSandbox` response resolves synchronously — so on a fast runner the `connecting → running` transition 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 `connecting` or `running` removes it — both states indicate the spawn has progressed past init, which is the actual precondition the cancel test needs before driving `status = 'active'`. ```diff - if (rows[0]?.status === "connecting") { + const status = rows[0]?.status; + if (status === "connecting" || status === "running") { return; } ``` ## Test plan - [x] `npm run test:integration -w @open-inspect/control-plane -- test/integration/do-internal-routes.test.ts` passes locally. - [x] Ran the test file 30 times in a loop — 30/30 green. - [x] `npm run typecheck -w @open-inspect/control-plane` clean. - [x] `npm run lint -w @open-inspect/control-plane` clean. - [x] `npx prettier --check` clean. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Tests** * Improved integration test reliability by updating sandbox readiness detection to recognize additional valid states before proceeding with test execution. <!-- review_stack_entry_start --> [](https://app.coderabbit.ai/change-stack/ColeMurray/background-agents/pull/651?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack) <!-- review_stack_entry_end --> <!-- end of auto-generated comment: release notes by coderabbit.ai -->
](https://app.coderabbit.ai/change-stack/ColeMurray/background-agents/pull/651?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack){kind=link}
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The control-plane integration test in
packages/control-plane/test/integration/do-internal-routes.test.tshas been flaky. The cancel-then-assert-status test occasionally fails because the sandbox row reachesfailedbefore the assertion runs, rather thanstoppedas expected.This PR stabilizes the suite with two changes, both confined to that one test file:
fetchlayer. AbeforeEach/afterEachpair installs/removes avi.stubGlobal("fetch", ...)mock that intercepts requests to*.modal.runand returns a syntheticrunningsandbox response. Non-Modalfetchcalls pass through to the realfetch.waitForSandboxSpawnhelper. Polls the DO'ssandboxtable until the row reachesconnecting(1s timeout, 10ms interval), so the test waits for the async spawn to land before driving subsequent state transitions.Why this matters
The integration suite has been making real outbound Modal HTTP calls in CI. Modal returns
404 modal-http: invalid function call(most likely becauseMODAL_WORKSPACEis the dummy test workspace), so no real sandboxes are being created — but thefetchcalls themselves are leaving the workerd runtime.Recent successful upstream
control-planeintegration jobs each contained 132modal-clientrequest log lines:Each shows the same pattern:
These
createSandboxrequests originate from the backgroundctx.waitUntil()callback that session init kicks off. Most tests do not assert on the final sandbox status, so the background 404 has just been noise. The cancel test was one of the few that did assert on status after cancel, which made it race-prone:stoppedand the assertion ran before the background Modal failure wrotefailed.failed.By mocking Modal at the
fetchboundary, the test no longer depends onMODAL_WORKSPACEbeing unset or on Modal's response timing, and the deterministicconnecting → runningpath letswaitForSandboxSpawnsynchronize the assertion against the spawn write.Test plan
npm run test:integration -w @open-inspect/control-planepasses locally and in CI.*.modal.runrequests after the change.do-internal-routes.test.tsonly.Summary by CodeRabbit
Release Notes
Tests
Note: This release contains no user-facing changes. Updates are limited to internal testing infrastructure improvements.