Skip to content

Give the network, gate and workspace Docker tests per-run isolation - #188

Merged
mchwang merged 2 commits into
mainfrom
fix/docker-test-isolation-more
Oct 21, 2026
Merged

mchwang merged 2 commits into
mainfrom
fix/docker-test-isolation-more

Conversation

@mchwang

@mchwang mchwang commented Oct 20, 2026

Copy link
Copy Markdown
Contributor

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 fixed 0123456789abcdef0123456789abcdef. After the ordinary afterAll cleanup, a finally block calls removeRunnerObjects(owner) from test/fixtures/docker-sweep.ts.

File Problem on main Change
test/runner-workspace.test.ts The path identity probe test counted across the whole daemon. It counted every codeboost-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. The test now follows only this probe's own objects. A recording shim on PATH captures the container name the probe chose. The shim runs the real docker with the same arguments, so it is not a fake. A docker events stream started before the probe then gives that container's ID and the volumes mounted in it. Absence is proved by container inspect and volume inspect on those IDs. Per-run owner and sweep added.
test/agent-gate.test.ts The spawnSync timeout did not free a hung probe. The default SIGTERM goes to the container's sh PID 1, which ignores it, so the client waited for the container. The container had no owner label, so no sweep could find it. The containers carry the runner label. The client is killed with SIGKILL at the timeout, and runBroken throws when the client has no exit status. The sweep in afterAll removes a container left running.
test/agent-network.test.ts No name collision and no count. Network and proxy names come from randomUUID(). The fixed attempt IDs (network-probe and the others) only appear in the io.codeboost.attempt label, where the per-run owner tells runs apart. Per-run owner and sweep only, as a precaution. Attempt IDs are unchanged.

Answer to "can the daemon-wide volume listing at runner-workspace.test.ts:155 be 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

  • runner-workspace, leftover container. I created a container named codeboost-path-identity-<uuid> that was never started, as a dead runner would leave. Main then failed at runner-workspace.test.ts:165 with expected 'c8f506cb8573' to be ''.
  • runner-workspace, concurrent volumes. A loop created a volume with the probe's tmpfs options every second while the test ran. Main then failed at runner-workspace.test.ts:166 with expected [ …(29) ] to deeply equal [].
  • agent-gate, timeout mechanism. I ran 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 no io.codeboost.runner label. The client returned ETIMEDOUT only after I removed the container by hand.
  • agent-network. I found no failing case on main (see the table).

Passes on this branch

  • runner-workspace, same hostile conditions. I left a leftover probe container in place, and the loop created 10 probe-option volumes during the run. The test passed: 1 passed.
  • runner-workspace, mutation check. I temporarily removed --rm from the probe in agents/container/storage.ts. The scoped test failed at the container inspect check. Then I reverted the change.
  • agent-gate, timeout and sweep. The same command with killSignal: 'SIGKILL' and the runner label returned after 3006 ms with ETIMEDOUT SIGKILL. The labelled container was still running, and removeRunnerObjects(owner) removed it.
  • agent-gate, mutation check. I temporarily set the timeout to 1 s. Three cases failed with docker run did not exit: spawnSync docker ETIMEDOUT, and afterAll removed 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.ts serially 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:

    • "does not delete a same-named stand-in…" timed out at 60 s.
    • "removes a network that lands…" found the late network after its settle window.

    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 (test and real-docker).

Review record

  • Independent review round 1. Fresh general-purpose agent, base a0c38a3 (PR Keep each Docker test run's containers and counts to itself #185's head) to head 7eb2ec8. Finding: with SIGKILL, a timed-out gate probe returned 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": runBroken now 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' or JSON.parse failure on the events stream, and the shim recording only the separate --name X form. I clarified the comment for the second.
  • Independent review round 2. Fresh agent, base a0c38a3 to head 51365b6. No issues.
  • Rebase. Keep each Docker test run's containers and counts to itself #185 squash-merged as a56bbdc. I rebased onto main, and the patch is byte-identical (git diff a0c38a3 51365b6 matches git diff a56bbdc HEAD).
  • Independent review round 3. Fresh agent, base a56bbdc to head 4044469. No issues. Low-severity notes (no '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

  • Round 1 finding (a killed client passes a 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)

  • A keeper container (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.
  • The path identity probe itself carries no owner labels, so a dead runner's probe is found only by name. Recovery is tracked in Sweep leftover path identity probe containers in startup recovery #181.

🤖 Generated with Claude Code

mchwang and others added 2 commits October 20, 2026 15:00
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>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 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

@mchwang
mchwang merged commit c0b8bf0 into main Oct 21, 2026
3 checks passed
@mchwang
mchwang deleted the fix/docker-test-isolation-more branch October 21, 2026 00:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants