Repository navigation
Give the network, gate and workspace Docker tests per-run isolation - #188
Merged
Merged
Conversation
Each file now has its own random runner owner and removes what that owner still holds after its ordinary cleanup, as #185 did for the supervisor and container tests. - runner-workspace: the path identity probe test counted probe containers and probe-option volumes across the whole daemon, so a leftover probe container or another runner's probe failed it. It now follows only this probe's container, by the name a PATH shim records (the shim runs the real docker unchanged), and the volumes Docker's events show mounted in it. - agent-gate: broken-isolation containers carry the runner label, and the client is killed with SIGKILL at its timeout. SIGTERM was passed to the container's shell, which ignores it, so the client waited for the container to exit. - agent-network: network and proxy names are random, so the fixed attempt IDs reach only labels; the per-run owner and sweep are preventive. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A SIGKILLed client has no exit status, which the cases' not.toBe(0) checks accepted, so a probe that hung would have passed. Found in independent review. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
🟢 Approval recommended
No blocking issues were identified in these focused test-isolation and cleanup changes.
No open issues 🎉
What changed in this PR
Extends #185’s per-run Docker test isolation to the network, gate, and workspace suites.
Changes:
- Adds random runner owners and owner-scoped cleanup.
- Replaces daemon-wide workspace counts with probe-specific tracking.
- Bounds gate client timeouts and rejects killed probes.
| File | Description |
|---|---|
| test/runner-workspace.test.ts | Tracks the probe’s own container and volume; adds scoped cleanup. |
| test/agent-network.test.ts | Adds a per-run owner and cleanup sweep. |
| test/agent-gate.test.ts | Labels containers, enforces timeout failures, and sweeps leftovers. |
Review effort: Balanced
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
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
This PR does for three more Docker test files what #185 did for the supervisor and container tests. Each file gets its own runner owner,
randomBytes(16).toString('hex'), in place of the fixed0123456789abcdef0123456789abcdef. After the ordinaryafterAllcleanup, afinallyblock callsremoveRunnerObjects(owner)fromtest/fixtures/docker-sweep.ts.test/runner-workspace.test.tscodeboost-path-identity-*container, and every volume that has the probe's tmpfs options. A container a dead runner left behind fails it, and so does another runner's probe.PATHcaptures the container name the probe chose. The shim runs the real docker with the same arguments, so it is not a fake. Adocker eventsstream started before the probe then gives that container's ID and the volumes mounted in it. Absence is proved bycontainer inspectandvolume inspecton those IDs. Per-run owner and sweep added.test/agent-gate.test.tsspawnSynctimeout did not free a hung probe. The default SIGTERM goes to the container'sshPID 1, which ignores it, so the client waited for the container. The container had no owner label, so no sweep could find it.runBrokenthrows when the client has no exit status. The sweep inafterAllremoves a container left running.test/agent-network.test.tsrandomUUID(). The fixed attempt IDs (network-probeand the others) only appear in theio.codeboost.attemptlabel, where the per-run owner tells runs apart.Answer to "can the daemon-wide volume listing at
runner-workspace.test.ts:155be scoped?" Yes. The probe's volume is anonymous and has no labels, so a label filter can't find it. Instead, the test finds the volume through the probe's container, using Docker's events. The test also got faster: about 18 s instead of about 64 s, because it no longer inspects every volume on the daemon.Evidence
All three files were byte-identical to main when these baselines ran.
Fails on main
codeboost-path-identity-<uuid>that was never started, as a dead runner would leave. Main then failed atrunner-workspace.test.ts:165withexpected 'c8f506cb8573' to be ''.runner-workspace.test.ts:166withexpected [ …(29) ] to deeply equal [].spawnSync('docker', ['run', '--rm', …, 'sh', '-c', 'sleep 300'], { timeout: 3000 })on Docker 27.4.0. The client was still blocked about 2 minutes later. The container was still running, with noio.codeboost.runnerlabel. The client returnedETIMEDOUTonly after I removed the container by hand.Passes on this branch
1 passed.--rmfrom the probe inagents/container/storage.ts. The scoped test failed at thecontainer inspectcheck. Then I reverted the change.killSignal: 'SIGKILL'and the runner label returned after 3006 ms withETIMEDOUT SIGKILL. The labelled container was still running, andremoveRunnerObjects(owner)removed it.docker run did not exit: spawnSync docker ETIMEDOUT, andafterAllremoved the three containers left running. Then I reverted the change.Validation gates (reported separately)
Type check.
npx tsc --noEmit -p .passed on head 4044469.Focused Docker files. I ran
npx vitest run --project docker test/agent-network.test.ts test/agent-gate.test.ts test/runner-workspace.test.tsserially on head 4044469. Result: 3 files passed, 57 tests passed, 374 s.Earlier runs on intermediate checkpoint 7eb2ec8. These ran while another project's containers used about 600% CPU on the same daemon. Two runs of agent-network each had one timing-sensitive test fail, and a different test each time:
Neither test reads the owner value or the sweep. Run alone on the branch, the first test passed twice, in 7–9 s. Main's version of the file passed once under the same load, in 145 s. The branch's file then passed 21/21 in 143 s. I count these as infrastructure load, not product failures.
Full suite and real Docker. Left to CI (
testandreal-docker).Review record
status: null, which the cases'not.toBe(0)checks accept, so a hung probe would pass. Fixed in "Fail a gate case whose probe client is killed at its timeout":runBrokennow throws. The 1 s mutation check above covers it. Minor notes, which I declined because each one can only fail a test, never let a broken feature pass: an unhandled'error'orJSON.parsefailure on the events stream, and the shim recording only the separate--name Xform. I clarified the comment for the second.git diff a0c38a3 51365b6matchesgit diff a56bbdc HEAD).'error'listener on the events child; PATH restore if PATH were unset) can only fail a run, not hide a failure; declined.Review-lesson audit
not.toBe(0)check). Covered by the existing rule that a test must fail when its feature is broken, and by CLAUDE.md "Treat every behavioural claim … trace each claim through every path". It is a one-off application of those rules, so no new rule is needed.Follow-ups (not in this PR)
attempt=fixture, no runner label) has been up for 6 days on this machine's daemon. It comes from a test that creates storage without the runner label. No owner sweep can find it.🤖 Generated with Claude Code