Skip to content

Commit 0ffd195

Browse files
mchwangclaude
andauthored
Remove test objects made without a runner label however the test ends (#189) (#191)
* Remove test objects made without a runner label however the test ends (#189) `withoutRunnerLabel` now labels each object it strips with a per-call test-run ID and removes everything with that ID when the test finishes, passed, failed or timed out. The owner sweep also removes containers that hold a swept volume. In agent-network, the stand-in proxy and the blocker are removed in `finally`. A pair of tests checks that a keeper left by a test that stopped early is gone. Closes #189 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Give the test-run sweep its own budget and an afterAll pass (#189) Review round 1: the onTestFinished removal gets 3 minutes, not the 10 s hook default; afterAll sweeps every test-run ID again, for a create that lands after a timed-out test's removal; the regression pair records the storage only after its checks and takes it out of afterAll's list first; one ps call finds every holder. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Remove containers that hold a swept network; record the stand-in first (#189) Review round 2: a network made without the runner label stayed when a runner-labelled proxy held it, so the sweep now removes the containers on a swept network too (looked up by name, which also finds a created container). The hook budget covers the sweep's ten calls, and the impostor's name is recorded before the stand-in exists. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Remove only exact holders of a swept network; test the holder sweep (#189) Review round 3: Docker's network filter also matches the start of another network's ID, so holders are checked by network name; a real-Docker test covers created and running holders and bystanders; hook budgets cover the sweep; label-less creates in agent-container and agent-supervisor are tracked before they are made. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Close the remaining label-less leaks the review found (#189) Review round 4: the rogue volume is made inside its try and removed after its container; the foreign network is listed again in finally; the holder test records each object before it exists; the sweep reads a partial inspect; the hook budgets cover eleven calls. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Record label-less objects first in agent-network; remove networks by ID (#189) Review round 5: the network and proxy shims record a name before they make it, and the tests remove it in finally; the sweep removes networks by full ID, reads output only from a call that exited by itself, and the holder test runs its inspect with a missing ID, as the sweep meets it; the supervisor's finally removals never throw. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Test removal by full network ID; say what the runner sweep removes (#189) Review round 6. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Remove swept containers by full ID; track the blocker before it is made (#189) Review round 7: Docker resolves a container name before an ID prefix, so the sweep lists containers and holders with --no-trunc, and the holder test keeps a container named like a holder's short ID; the blocker is tracked before its run; comment wording. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Remove the foreign-runner storage in its test; clean every storage in afterAll (#189) Review round 8: the storage made under another runner is removed in finally; afterAll removes every storage even when one fails; the killed-create network is listed by full ID; afterAll budgets have a minute over the sweep. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Read network names as JSON; name the seccomp probe; test --no-trunc (#189) Review round 9: a network name may hold spaces, so the sweep reads names and holders' networks as JSON; the label-less seccomp probe is named and tracked before it runs; the holder test keeps a container named like a swept container's short ID; budget comments state the real margin. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Never throw on output the sweep cannot parse; foreign creates inside try (#189) Review round 10: a line that is not JSON is reported and skipped; the foreign-runner network and storage are made inside their try and removed by allocation; the recovery-handle test removes by full ID through the sweep; doc wording. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Sweep the failed-allocation test by allocation; guard unreadable lines (#189) Review round 11: the foreign-runner failed-allocation test removes its objects through the sweep, by full ID and never throwing; network lines that are not a network are skipped, with a unit test; in-test sweeps have budgets for their worst case and their own warning prefix. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Budget the in-test sweeps; exact wording (#189) Review round 12. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Sweep both runners of the recovery test; keep the first failure (#189) Review round 13: the recovery test sweeps its two random runner owners after its undo steps, so what a step could not remove or never recorded goes too; the foreign-storage test's cleanup no longer replaces the body's failure; doc wording. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Check the traps are live in the holder test; report every cleanup failure (#189) Review round 14: the holder test first asserts Docker's prefix match and its name-before-prefix resolution, so its bystander checks test something; afterAll collects every cleanup step's failure; comment states which ending the it.fails pair tests. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Keep the first failure in the replaced-keeper test; exact sweep wording (#189) Review round 15 nits. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Check the partial-inspect answer; sweep the replaced keeper; budget margin (#189) Review round 16. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Budget the replaced-keeper sweep; say what the holder test's budget covers (#189) Review round 17. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * One per-file sweep in afterAll; real docker for the sweep; timed-out leftovers (#189) Review round 18: objects made without the runner label also carry this file's run ID, so afterAll removes them in one sweep within its budget; the sweep runs the real docker found at load, never a shim left on PATH; agent-network's label-less objects and the rogue volume are also removed in afterAll should their test time out; the volume test checks the wrapper's labels; cleanup warnings are logged; the fake's note names its answers. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Track the race and legacy volumes; cleanup loops inside try with real docker (#189) Review round 19. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Sweep other runners' objects when the test finishes, timeouts included (#189) Review round 20: async tests that make objects under another runner's owner or their own random one register their sweep with onTestFinished, which runs after a timeout too, so test budgets return to their old values; the shims and afterAll use the real docker with a timeout; comment wording. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Real docker in every shim; foreign-name containers swept on finish; killed-call test (#189) Review round 21: every shim calls the real docker found at load and refuses when there is none; agent-supervisor's two foreign-name containers are removed by name when their test finishes, timeouts included; a unit test shows a killed call's output is not read; one more race volume tracked; comments name every removal afterAll makes and what the finish-time sweep cannot cover. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Budget the supervisor finish hooks; name a signal kill; exact comments (#189) Review round 22. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Register late-landing agent-network objects by name; exact comments (#189) Review round 23: the three agent-network tests whose foreign object may land after their own removal register it by name for afterAll, since the record goes with the shim; the late-network test says why its network is swept; comment wording. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Track the holder test's objects for afterAll, removed by exact name (#189) Review round 24: the holder test's unlabelled objects and the swapped keeper are tracked for afterAll; afterAll removes tracked names only when an object of exactly that name exists, by its full ID, since some are named like ID prefixes; withoutRunnerLabel uses sweepWhenFinished; the finish-time sweep states what lands too late for it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Report removeByName failures; records keep every name; shim scoped (#189) Review round 25: removeByName reads JSON (names with spaces), reports a failed call other than no such object; agent-network's shims append one name per line and every name is removed; the foreign-label shim rewrites only its own allocation's create; the holder test checks the prefix-named network goes; the killed-call unit test first checks its fake and asserts the warning; comments name the networks set. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Test removeByName's exact-name guard and reports; records outside shims (#189) Review round 26: the holder test checks removeByName leaves an object whose ID starts with a now-missing tracked name; a unit test covers its no-such answer, an ID-prefix match, a refused removal and a killed inspect; only a container's name loses its leading slash; agent-network keeps its records outside the shims so afterAll reads names a late client adds; sweepWhenFinished's doc covers withoutRunnerLabel. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * removeByName treats only Docker's own missing answers as missing; tests every kind (#189) Review round 27: a failure before the daemon (an unknown context) is reported; the fake covers containers, networks and volumes, found, missing, under another name and refused; the unlabelled-create shim acts only on its own container; agent-supervisor's finish-time removals use removeByName; doc and import fixes. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Real-Docker guard check on a free network; real error text in the fake; budgets (#189) Review round 28: the holder test's removeByName check uses a network with no endpoints, so a broken guard would remove it; the fake's stderr is Docker 27.4's own; the supervisor's finish-time removals get 90 s for two 30 s calls; agent-network makes its records directory on first use; comment fixes. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Holder test checks removeByName's exact removal and silence on a gone name (#189) Review round 29: one free object of each kind is removed by exact name and asserted gone, and a second removal of the gone names reports nothing; the ID-prefix check's comment says what the names are; the test's finally removes by exact name; the unit file's header names what the daemon test covers. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * The stand-in shim acts only on an exec in a proxy (#189) Review round 30: any other exec passes through, so the shim can never run a stand-in with an empty name, which Docker would give a random, unrecorded one. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Doc: a test may sweep its own runner owner with removeLabelledObjects (#189) Review round 31. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Say what the killed-call fake leaves out (#189) Review round 32. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Sweeps keep the Docker daemon set when they loaded (#189) Review round 33: a test that times out may leave DOCKER_HOST changed (agent-supervisor points it at a missing socket until its handle settles); every call in docker-sweep.ts now restores DOCKER_HOST and DOCKER_CONTEXT as they were at load, with a unit test that fails without it; the context fake says its path is shortened. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Comment and naming fixes (#189) Review round 34: what the loaded Docker address means for each call, which stdout a failed call leaves unread, who adds the test-run label; the records directory variable is lower case, as it changes. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * The racer shim acts only on its own allocation's create (#189) Review round 35: should a race test time out with the shim on PATH, other tests' creates pass through; withoutRunnerLabel's doc says what a wrapper left on PATH does to a later test. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * removeByName's reports name the stage and the command run (#189) Review round 36: removeByName takes a when prefix, as removeLabelledObjects does, and names the command it ran (docker rm, docker container inspect). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Sweep reports name their stage; a gone holder is not a failure; loaded daemon tested (#189) Review round 37: removeLabelledObjects names the stage of each failed call and says nothing of a holder gone since the listing (Docker 27.4's No such container for it), still reporting any other failure; a test loads a fresh copy of the module with DOCKER_HOST and DOCKER_CONTEXT set and shows the sweep keeps them; unit tests cover both holder inspect answers. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Track the never-started unlabelled agent container by name; comment fixes (#189) Review round 38: Docker 27.4 removes a network a created, never started container still names, so that test's container is tracked for afterAll; the docs say which holder reads stay quiet, that the sweep reads stderr for them, and that a wrapper can stay on PATH for the rest of the file; the loaded-daemon fake says what it answers. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Label agent-network's unnamed helper containers for afterAll; track the rogue endpoint (#189) Review round 39: a helper run killed between create and start is never removed by --rm, and is no holder once its vendor network is gone, so the curl, DNS and concurrent-network helpers carry this file's run ID and afterAll sweeps it (13-minute hook for two sweeps); agent-container's rogue endpoint is tracked by name; the holder fake says the removals' stdout is not read. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Track the peer, stand-in proxy and foreign keeper by name; holder test asserts the quiet sweep on the daemon (#189) Review round 40: three label-less or foreign containers made with a 60 s client could land after their test's finally, and once their network is gone none is a holder, so each is tracked by name for afterAll; the holder test checks the daemon's own answer for a gone container and that the sweep reports no failure for it; the holder fake says what it leaves out; the unit file unstubs env after each test; agent-network removes its records before the sweeps. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Track the rogue-volume container before its create (#189) Review round 41: should the create land after its client is killed, afterAll removes the container by name before the label-less volume it mounts. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Read the holder test's sweep warnings before restoring the spy (#189) Review round 42: mockRestore clears the recorded calls, so the check that the sweep says nothing of a gone holder read an empty list and could not fail; the calls are copied first, and the test also checks the spy saw the sweep's own lines. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Storage release after a pass fails the test again; mark the invented fake answer (#189) Review round 43: in the two tests whose cleanup now warns instead of throwing, a release that fails after the test's own checks passed fails the test, as on main, while a failure above still stays the one reported; the second holder fake case says its answer is invented. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * removeByName: tested on unreadable output, quiet on a network gone before its removal (#189) Review round 44: the fake gives an inspect answer that is not JSON (reported, nothing removed, never thrown) and a network gone between inspect and removal, which Docker 27.4 answers 'network <ID> not found' and removeByName now treats as gone; the keeper comment says afterAll also removes it by name; a comment line is rewrapped. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Restore two failures main's cleanups had; report an inspect answer with no ID and name (#189) Review round 45: the foreign-name supervisor test fails again when, after its checks pass, the container's removal fails; the unsettled-allocation test checks after its sweep that nothing of the allocation is left, as main's listing did; removeByName reports a parsed answer without an ID and a name, and its doc lists what it stays quiet about; comment blocks rewrapped. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Comment fixes: what main's check was; what removeByName reads (#189) Review round 46. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Test that the real docker is found once at load and that shims refuse without one (#189) Review round 47: a unit test loads the fixture with docker on PATH, then puts another first, and checks REAL_DOCKER keeps the first; loaded with no docker, requireRealDocker refuses. A docker found through a relative PATH entry is resolved to an absolute path. The never-started agent container's comment gives the real reason it is tracked. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Test the load-time docker as the default and a relative PATH entry; client limit; messages (#189) Review round 48: the finds-docker test loads the fixture through a relative PATH entry, then puts another docker first and calls both removals with no docker given, checking only the docker found at load ran; the network listing in the killed-create test gets its 60 s client limit back; the supervisor's removal message handles a docker that could not start; the fake says what it answers. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Every Docker client call the branch adds or moves in a test has a time limit (#189) Review round 49: synchronous calls cannot be stopped by a Vitest timer, so each one the branch added, moved or rewrote (removals, inspects, the foreign creates and the ack-script run) takes CLIENT_LIMIT, 60 s with SIGKILL, as main's docker() helper did; the supervisor's removal message names a kill by signal. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Track the ack-script container by name; time limits on the moved and attached calls (#189) Review round 50: agent-supervisor's label-less ack-script container is tracked by name before its create and removed by afterAll; runner-workspace's docker helper, which now runs calls this branch moved out of a try, takes CLIENT_LIMIT; the seccomp probe and agent-network's attached helper runs are killed with SIGKILL at their limit, as an attached run forwards SIGTERM; the killed-create test's listing reports its own failure. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Name a failed foreign-keeper removal after a pass (#189) Review round 51: as on main, a docker rm of the foreign keeper that fails after the test's checks pass fails the test with Docker's own error, before the storage release's. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Workflow limits for the longer teardown; full test of the loaded daemon; keeper warning (#189) Review round 52: the Live probes job limit rises to 90 minutes, as the teardown hooks it covers now allow 24 (vitest-config pins it); the Agent isolation comment says its 45 minutes cover ordinary runs; the loaded-daemon test answers one network with a holder and one named container, so every kind of call is checked; a failed foreign-keeper removal is warned of when the body failed. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Live probes limit counts agent-planning's probe and teardown; clearer missing-record failures (#189) Review round 53: the comment counted four probes, before #193 added agent-planning's; with its 11-minute probe and 2-minute teardown the limits total 90 minutes plus setup, so the job limit rises to 100 (pinned in vitest-config); the refused-network and refused-proxy tests say so when their shim recorded nothing. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Sweep quiet on a network gone since its listing; full loaded-daemon test; hook budget (#189) Review round 54: the sweep treats Docker 27.4's 'network <ID> not found' on its network removal as gone, as removeByName does, with a test that still reports any other failure; the loaded-daemon test now lists a container, a network and a volume, each with a holder, and checks every command line; the after-sweep check's budget covers its three listings; the Agent isolation comment names the shared sweep. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Holder test checks its gone-holder shim fired; header and workflow comments (#189) Review round 55: the holder test's shim leaves a marker, and the test checks it before trusting the quiet sweep; the unit file's header says the sweep reads a network removal's stderr too; the Agent isolation comment names everything its 45 minutes cover. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Rewrap two comment blocks (#189) Review round 56. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * The loaded-daemon fake gives the volume its own holder (#189) Review round 57: the stateless fake listed the network's holder again for the volume, after the sweep had removed it, which Docker would not; the volume now has its own holder, and the test checks its removal. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Track the supervisor's foreign-name containers for afterAll; stage in unreadable-line reports (#189) Review round 58: the two foreign-name containers are tracked by name for afterAll too, should a create land after their finish-time removal; the sweep's report of a line that is not JSON names its stage. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Failure messages name the docker command; record checks first (#189) Review round 59: the killed-create listing and the supervisor's after-pass removal name the command that failed, as main's helper did; the refused-network and refused-proxy tests check their shim's record before the error, so a shim that no longer matches is reported as itself. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Exact command in the listing's failure; holder test checks every sweep line (#189) Review round 60: the killed-create listing's failure message is built from the arguments it ran; the holder test requires the gone-holder inspect's exit 1, and accepts only the sweep's own removal lines, including the holders line, so any failure or unreadable answer fails it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Report sweep lines of the wrong shape; exact commands in removeByName's reports (#189) Review round 61: the sweep reports a network listing line that is not a network and a holder line that is not an ID and networks, as removeByName reports an answer that is not an ID and a name, with tests; removeByName's failure reports name the exact command and ID. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Gone checks need Docker's missing answer; sweep checks listed IDs and names; full commands (#189) Review round 62: objectExists counts only Docker 27.4's missing answer as gone and throws on any other failure, so a killed inspect no longer passes a removal check (the export, follow-up and holder tests; runner-workspace's volume check wants 'no such volume'); the sweep names whole commands in its reports, reports and skips listed lines that are not a container ID or a volume name, and reports a holder line with no space once; agent-network's records removal cannot stop the sweeps; the keeper test's finally sweep says finally. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * agent-network's survival checks use objectExists (#189) Review round 63: a killed or failed inspect in the finally no longer reads as the foreign object being removed; Docker's missing answer counts as gone, and any other failure is rethrown, naming the command, after the body's own checks. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Every ID passed on as an argument has a checked shape (#189) Review round 64: network IDs from the listing, holder IDs and removeByName's inspected IDs must be full IDs, or are reported and skipped, so none reads as an option; an inspect line with no space is reported instead of parsed twice; fake cases for each. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * removeByName refuses a name that could read as an option; fake notes (#189) Review round 65: removeByName checks the name it passes on (a volume's against Docker's allowed shape, others for a leading dash) and reports a refusal, with fake cases; the volume-name shape needs two characters, as Docker 27.4 does; agent-network's two rewritten shims say which answers are Docker's and which are invented. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Test objectExists; gone cases reach their inspect; per-record errors in afterAll (#189) Review round 66: a unit test loads the fixture with a stand-in docker and checks objectExists is true, false only on each kind's missing answer, and throws naming the command on a killed call or an unknown context, at the loaded daemon; removeByName's gone cases assert their inspect ran; the holder fake's missing answer names the full ID; agent-network's afterAll reports a record it cannot read and goes on. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Gone checks in the touched tests use objectExists; fake and comment fixes (#189) Review round 67: the keeper-replaced, foreign-name and unlabelled-agent checks use objectExists, and the holder test calls it directly; the PR says checks in untouched tests keep main's form; the objectExists fake prints the unknown context's quotes as Docker does and says its path is shortened; the supervisor's tracked containers are described as lacking the runner label. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Every gone check in a changed test uses objectExists (#189) Review round 68: the by-ID removal and start tests, the rogue-endpoint test, the recovery test's exists helper and agent-network's late-landing check use objectExists; removeByName's refusal says the name could read as an option; the objectExists fake marks its stdout as invented. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * The proxy-start test's gone check uses objectExists; assert the raced removal ran (#189) Review round 69: the last gone check in a changed test uses objectExists; the gone-network sweep test checks its removal ran before trusting the silence; the stand-in shim's note says where stderr goes. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Shape checks before the gone checks; objectExists refuses an option-like ref (#189) Review round 70: the late-landing network and proxy-start tests check the value their shim recorded before inspecting it, as the record tests do, so a shim that recorded something else is reported as itself; objectExists refuses an empty or dash-led ref, naming it; the proxy-start test's removal takes CLIENT_LIMIT. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Trim the listing's stderr in its failure message (#189) Review round 71. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Pin every docker call's time limit; volume refusal reason; guarded record reads (#189) Review round 72: a unit test loads the fixture with spawnSync observed and checks every removal call is given 30 s and SIGKILL, and objectExists 60 s and SIGKILL, so the budgets built on them cannot lose their limit unseen; removeByName says a refused volume name is not one Docker allows; agent-network's finally blocks report a record they cannot read instead of throwing over the body's failure; the PR says what every changed gone check requires. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Time-limit test runs all thirteen calls; holder search and no-space lines tested (#189) Review round 73: the time-limit test answers so both removals make every call (13), and checks each; a holder search line that is not a container ID is reported and skipped, with a test; removeByName reports an inspect line with no space as no ID and name; the holder read's no-space line is tested. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Test the fixture's remaining guards; exact reasons in refusals and reports (#189) Review round 74: tests cover a docker that cannot start (both functions report and never throw), Docker's missing text with another exit status in objectExists, an empty name in removeByName, and a holder line that is not JSON, reported once; refusals of an empty value say so; an inspected ID that is not a full ID is reported as that; the two fakes that answer removals say their stdout is not read. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
1 parent 79deeec commit 0ffd195

10 files changed

Lines changed: 1181 additions & 227 deletions

‎.github/workflows/agent-isolation.yml‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,9 @@ permissions:
99
jobs:
1010
real-docker:
1111
runs-on: ubuntu-latest
12-
# Covers the 10 min image build, 2 min teardown and the per-test Docker budgets.
12+
# Covers the typecheck, the ordinary Vitest project, the 10 min image build and ordinary run times of the
13+
# Docker-backed files. It does not cover every test and hook at its own Vitest limit: the five afterAll hooks that
14+
# call the shared sweep allow 46 min between them (#189), which only a stalled daemon reaches.
1315
timeout-minutes: 45
1416
steps:
1517
- uses: actions/checkout@v4

‎.github/workflows/live-probes.yml‎

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -13,13 +13,14 @@ concurrency:
1313
jobs:
1414
live-probes:
1515
runs-on: ubuntu-latest
16-
# The probes' own time limits total 33 min (6, 8, 8 and 11). agent-container and agent-supervisor may each spend
17-
# up to 10 min building the image before their probes; agent-question builds inside its probe's own 11 min. The
18-
# teardown hooks have Vitest limits that total 9 min: agent-container 2 after each test and 2 at the end,
19-
# agent-supervisor 3 and agent-question 2. That is 62 min, plus setup. These are only Vitest's limits: the afterAll
20-
# hooks in agent-container and agent-question call docker through spawnSync with no timeout of their own, and
21-
# Vitest's hook timer cannot fire while such a call blocks. The 75 min job limit caps the run.
22-
timeout-minutes: 75
16+
# The probes' own time limits total 44 min (6, 8, 8, 11 and 11). agent-container and agent-supervisor may each spend
17+
# up to 10 min building the image before their probes; agent-question and agent-planning build inside their probes'
18+
# own 11 min. The teardown hooks have Vitest limits that total 26 min: agent-container 2 after each test and 13 at
19+
# the end (two sweeps of up to 5.5 min each, #189), agent-supervisor 7, agent-question 2 and agent-planning 2. That
20+
# is 90 min, plus setup. These are only Vitest's limits: Vitest's hook timer cannot fire while a synchronous docker
21+
# call blocks, and some calls in these hooks have no timeout of their own (the sweeps' calls have 30 s each). The
22+
# 100 min job limit caps the run.
23+
timeout-minutes: 100
2324
steps:
2425
# Fail at once when the secret is missing, so a missing secret never looks like a passing run.
2526
- name: Require the Claude token

‎test/agent-container.test.ts‎

Lines changed: 359 additions & 110 deletions
Large diffs are not rendered by default.

‎test/agent-gate.test.ts‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -51,8 +51,9 @@ const writableMetadata = tmpfs('/work/.git', 'size=16m,nr_inodes=512');
5151
const boundedScratch = tmpfs('/tmp', 'size=32m,nr_inodes=4096');
5252

5353
beforeAll(() => { imageId = buildAgentImage(); }, 10 * 60_000);
54-
// The file has no other cleanup: a probe container whose client timed out may still be running.
55-
afterAll(() => removeRunnerObjects(TEST_RUNNER_OWNER), 4 * 60_000);
54+
// The file has no other cleanup: a probe container whose client timed out may still be running. The budget is the
55+
// sweep's worst case (eleven calls of 30 s) and half a minute.
56+
afterAll(() => removeRunnerObjects(TEST_RUNNER_OWNER), 6 * 60_000);
5657

5758
describe('isolation gate detects breaches', () => {
5859
it.each([

‎test/agent-network.test.ts‎

Lines changed: 133 additions & 47 deletions
Large diffs are not rendered by default.

‎test/agent-supervisor.test.ts‎

Lines changed: 40 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,11 @@
11
import { randomBytes, randomUUID } from 'node:crypto';
22
import { execFileSync, spawnSync } from 'node:child_process';
3-
import { removeRunnerObjects } from './fixtures/docker-sweep.ts';
3+
import { CLIENT_LIMIT, objectExists, removeByName, removeRunnerObjects, requireRealDocker } from './fixtures/docker-sweep.ts';
44
import { fixtureGit } from './fixtures/git.ts';
55
import { chmodSync, mkdtempSync, mkdirSync, rmSync, writeFileSync } from 'node:fs';
66
import { tmpdir } from 'node:os';
77
import { dirname, join } from 'node:path';
8-
import { afterAll, beforeAll, describe, expect, it } from 'vitest';
8+
import { afterAll, beforeAll, describe, expect, it, onTestFinished } from 'vitest';
99
import { startClaudeInvocation } from '../agents/adapters/claude.ts';
1010
import { readCodexOutput } from '../agents/adapters/codex.ts';
1111
import { ACKNOWLEDGEMENT_SCRIPT, isInvocationActive, readBoundedContainerFile, retainSetupCleanup,
@@ -30,6 +30,11 @@ const testOwner = (attemptId = 'fixture') => ({ runnerOwner: TEST_RUNNER_OWNER,
3030

3131
const roots: string[] = [], profiles: ContainerProfile[] = [];
3232
const allocations: ReturnType<typeof prepareTaskFilesystems>[] = [];
33+
/**
34+
* Containers without this run's runner label, by name, removed in afterAll should their create land after their test's
35+
* finally (#189).
36+
*/
37+
const unlabelled = new Set<string>();
3338
let imageId = '';
3439
const git = fixtureGit;
3540

@@ -70,6 +75,7 @@ async function profile(data: ReturnType<typeof fixture>, probe: IsolationProbe,
7075
beforeAll(async () => { imageId = buildAgentImage(); }, 10 * 60_000);
7176
afterAll(async () => {
7277
try {
78+
for (const name of unlabelled) removeByName('container', name, 'afterAll');
7379
for (const profile of profiles) await disposeContainerProfile(profile);
7480
for (const allocation of allocations.reverse()) removeTaskFilesystems(allocation);
7581
for (const root of roots.reverse()) {
@@ -79,9 +85,11 @@ afterAll(async () => {
7985
} finally {
8086
// Then remove whatever this run still owns, even when a test failed or the cleanup above threw: an agent that
8187
// ignores SIGTERM, or storage whose removal did not settle. It may not run before a hook timeout.
88+
// The hook's 7 minutes leave a minute and a half over this sweep's worst case (eleven calls of 30 s), for the
89+
// cleanup before it.
8290
removeRunnerObjects(TEST_RUNNER_OWNER);
8391
}
84-
}, 3 * 60_000);
92+
}, 7 * 60_000);
8593

8694
describe('container invocation supervisor', () => {
8795
it('preserves the first cancellation reason while retained setup cleanup settles', async () => {
@@ -330,7 +338,7 @@ describe('container invocation supervisor', () => {
330338
const hangingDocker = (match: string) => {
331339
const shim = mkdtempSync(join(tmpdir(), 'codeboost-slow-docker-')); roots.push(shim);
332340
mkdirSync(join(shim, 'input')); // afterAll resets this path's mode
333-
const realDocker = execFileSync('sh', ['-c', 'command -v docker'], { encoding: 'utf8' }).trim();
341+
const realDocker = requireRealDocker();
334342
writeFileSync(join(shim, 'docker'), ['#!/bin/sh', `case "$*" in ${match}) exec sleep 60;; esac`,
335343
`exec '${realDocker}' "$@"`].join('\n'), { mode: 0o755 });
336344
return `${shim}:${process.env.PATH}`;
@@ -385,20 +393,30 @@ describe('container invocation supervisor', () => {
385393

386394
it('releases its own resources, and leaves a same-named container another invocation owns', async () => {
387395
const current = await profile(fixture(), 'finite-output', 'foreign-name');
388-
execFileSync('docker', ['create', '--name', current.name, '--label', 'io.codeboost.invocation=foreign',
389-
'--entrypoint', 'true', imageId], { stdio: 'ignore' });
396+
// Removed by name when the test finishes, timeouts included: its label value is shared across runs (#189). Tracked
397+
// for afterAll too, should its create land after that.
398+
unlabelled.add(current.name);
399+
onTestFinished(() => removeByName('container', current.name, 'onTestFinished'), 90_000); // two calls of up to 30 s
400+
let passed = false;
390401
try {
402+
// Inside the try, so its removal runs even if the create's client fails after the daemon made it (#189).
403+
execFileSync('docker', ['create', '--name', current.name, '--label', 'io.codeboost.invocation=foreign',
404+
'--entrypoint', 'true', imageId], { stdio: 'ignore', ...CLIENT_LIMIT });
391405
const result = await startProfileInvocation(current).settled;
392406
expect(result.stopReason).toBe('capture-failure');
393407
expect(result.stderr).toMatch(/already in use|Conflict/);
394408
expect(result.unreleased).toBeUndefined();
395409
expect(isInvocationActive(attempt('foreign-name'))).toBe(false);
396410
expect(isContainerProfileAuthentic(current)).toBe(false);
397-
expect(spawnSync('docker', ['network', 'inspect', current.network.name], { stdio: 'ignore' }).status).not.toBe(0);
411+
expect(objectExists('network', current.network.name)).toBe(false);
398412
expect(execFileSync('docker', ['container', 'inspect', '--format',
399413
'{{index .Config.Labels "io.codeboost.invocation"}}', current.name], { encoding: 'utf8' }).trim()).toBe('foreign');
414+
passed = true;
400415
} finally {
401-
execFileSync('docker', ['rm', '--force', current.name], { stdio: 'ignore' });
416+
// A failure above stays the one reported, whatever exit status this Docker version gives for a missing name. After
417+
// a pass the container exists, and its removal must succeed, as on main.
418+
const removed = spawnSync('docker', ['rm', '--force', current.name], { encoding: 'utf8', ...CLIENT_LIMIT });
419+
if (passed && removed.status !== 0) throw new Error(`docker rm --force ${current.name} failed: ${removed.error?.message ?? (removed.signal ? `killed by ${removed.signal}` : removed.stderr?.trim())}`);
402420
}
403421
}, 3 * 60_000);
404422

@@ -436,9 +454,13 @@ describe('container invocation supervisor', () => {
436454
it('releases only the profile when rejecting before creation, even if its name is held elsewhere', async () => {
437455
const current = await profile(fixture(), 'noop');
438456
// A foreign container occupies the deterministic name, so container-level cleanup could never settle.
439-
execFileSync('docker', ['create', '--name', current.name, '--label', 'io.codeboost.invocation=someone-else',
440-
'--entrypoint', 'true', imageId], { stdio: 'ignore' });
457+
// Removed by name when the test finishes, timeouts included: its label value is shared across runs (#189). Tracked
458+
// for afterAll too, should its create land after that.
459+
unlabelled.add(current.name);
460+
onTestFinished(() => removeByName('container', current.name, 'onTestFinished'), 90_000); // two calls of up to 30 s
441461
try {
462+
execFileSync('docker', ['create', '--name', current.name, '--label', 'io.codeboost.invocation=someone-else',
463+
'--entrypoint', 'true', imageId], { stdio: 'ignore', ...CLIENT_LIMIT });
442464
await expectRejected(startProfileInvocation(current, { timeoutMs: 10 * 60_000 + 1 }), 'ceiling');
443465
expect(isContainerProfileAuthentic(current)).toBe(false);
444466
expect(spawnSync('docker', ['container', 'inspect', current.name], { stdio: 'ignore' }).status).toBe(0);
@@ -641,11 +663,15 @@ describe('container invocation supervisor', () => {
641663
it('writes the deferred output acknowledgement idempotently, repairing a partial one', () => {
642664
const token = randomUUID(), file = `/run/codeboost-control/collected-${token}`;
643665
const name = `codeboost-ack-script-${randomUUID()}`;
644-
execFileSync('docker', ['run', '-d', '--rm', '--name', name, '--network', 'none', '--cap-drop=ALL',
645-
'--security-opt=no-new-privileges', '--user', '10001:10001', '--entrypoint', 'sleep',
646-
'--tmpfs', '/run/codeboost-control:rw,nosuid,nodev,noexec,size=65536,nr_inodes=16,uid=0,gid=0,mode=0711',
647-
imageId, '120'], { stdio: 'ignore' });
666+
// Tracked before it is made: it has no labels, and a create that lands after its client was killed is never started,
667+
// so `--rm` does not apply and only removal by name finds it (#189).
668+
unlabelled.add(name);
648669
try {
670+
// Inside the try: the container has no labels, so the finally below removes it, and afterAll by name (#189).
671+
execFileSync('docker', ['run', '-d', '--rm', '--name', name, '--network', 'none', '--cap-drop=ALL',
672+
'--security-opt=no-new-privileges', '--user', '10001:10001', '--entrypoint', 'sleep',
673+
'--tmpfs', '/run/codeboost-control:rw,nosuid,nodev,noexec,size=65536,nr_inodes=16,uid=0,gid=0,mode=0711',
674+
imageId, '120'], { stdio: 'ignore', ...CLIENT_LIMIT });
649675
const acknowledge = () => spawnSync('docker', ['exec', '--user', '0', name, 'node', '-e', ACKNOWLEDGEMENT_SCRIPT,
650676
token], { encoding: 'utf8' });
651677
const content = () => execFileSync('docker', ['exec', name, 'cat', file], { encoding: 'utf8' });

0 commit comments

Comments
 (0)