Repository navigation
fix: sweep the session network a dead shell leaves behind (#337) - #361
Merged
Merged
Conversation
`openSandbox` creates `pi-sandbox-<id>-net` before the container and removes it in its own `finally`. A terminal closed mid-session never reaches that, and nothing else did either: the boot reaper lists only `pi-job-` networks, and issue #277 withdrew removing one at OPEN time because two opens of the same run overlap and the second stripped the first's proxy. So the leftover survived every restart and every later open of that run refused with `egress-network-exists`, permanently. The sandbox retention reaper takes it. It is the one sweep that already asks docker what is live before it deletes, it runs at boot and on the retention timer, and the directory it sweeps is the signal this needs. #277's argument does not reach it: an open in progress always has its retained directory, because `resolveSandbox` refuses a job whose directory is gone. Three things are load-bearing and each is in a comment beside the code. The keep set is the listing the pass STARTED with, never the survivors. The same pass expires directories, so a sweeper reading the listing afterwards would take the network of a run being opened between `createJobNetwork` and `launch` and kill that open with a 125. An id expired this pass simply waits for the next one. The session-container guard gates the DETACH, not the removal. Measured on docker 27.4.0: a `network rm` of a network a live container is on fails, so docker is already the backstop for removal. What docker will not stop is stripping the proxy off a shell someone is sitting in, which is exactly the #277 harm, so the guard sits on the detach list and the test asserts no `network disconnect` is issued rather than asserting the network survived. The weaker assertion would pass on docker's own refusal alone. The sweeper is injected rather than imported. `sandbox.mjs` imports `readManifest` from `sandbox-store.mjs`, so the other direction is a cycle, and injection keeps `sandbox-store.mjs` docker-free in its own tests. It defaults to a real no-op, so every existing construction is byte-unchanged. Naming follows `OQ-007`'s stated property, that one grep covers boot and every tick. A per-network outcome is `reaped_sandbox_network` or `sandbox_network_not_reaped` with a fixed reason token; the sweep's own fault, including a `network ls` that did not answer, keeps `sandbox_reaper_skipped`. The listing is a substring match and is not the namespace, so candidates are parsed against the shape `networkNameFor` builds. The cost is named rather than hidden, and it narrows the issue's acceptance line on purpose: a leftover on a run that is still re-openable survives until its window closes, and until then that run's next open refuses with the two commands, which is shipped behaviour unchanged. The stricter reading is what reopens #277. Specs: `REQ-RESURRECTABLE-SANDBOX` AMENDED (the window bounds the network too); `INT-SANDBOX-CONTRACT` AMENDED, one clause a correction of "such a network stays until an operator removes it" and one a correction of the #357 bullet written a week ago that said a session network has no sweep at all; `DES-EGRESS-DENY-ON-A-DEDICATED-NETWORK` AMENDED with the fourth namespace and four more rejected alternatives; `DES-SANDBOX-IS-A-FRESH-CONTAINER` AMENDED, two Rejected entries this change tested rather than reversed; `OQ-007` AMENDED. UNCHANGED, checked: `REQ-EGRESS-ALLOWLIST`, `REQ-DEPLOYMENT-BOOTSTRAP`, `REQ-LOCAL-JOB-VISIBILITY`, `INT-EGRESS-POLICY-CONTRACT`, `INT-LIVE-PROBE-CONTRACT`, `DES-RETENTION-SWEEPS-ON-A-TIMER`, `DES-CONCURRENCY-3`, `CONST-ISOLATION-CONTAINER-PER-JOB` (its sandbox clause is about a container NAME and the boot reaper's filter is untouched), `OQ-016`. Docs: `docs/sandbox.md` (both the "nothing removes it for you" claim and the "two things follow" count), `SECURITY.md`'s namespace disclosure, and both `.env.example` copies. Refs #337 Signed-off-by: Rob Boerman <robboerman@live.nl>
…found unpinned (#337) Gate findings on the first commit, all confirmed before fixing. THE DEFECT. The keep set was the listing the pass STARTED with, and that closes only one end of the window. `retainJobDir` creates a retained directory at job END, in this same process, and a pass awaits docker and yields per tree, so a job can finish and its run be opened while the pass is still running. That id is in neither the old listing nor `running`, because the container is not up yet, so the sweep would take a network `createJobNetwork` had just made and kill the open with a 125, or strip the proxy off it. That is the #277 harm reached the long way round, which is exactly what the first commit claimed to have closed. The keep set is now the UNION of the pre-pass listing and a fresh one read immediately before the sweep. Each half covers what the other cannot: the first covers a run this pass expired whose open passed `resolveSandbox` a moment earlier, the second covers a run retained after the snapshot. A re-read that throws skips the sweep rather than sweeping on half the evidence. The price is one extra listing per pass, and one stated consequence: a network outlives its directory by a whole pass, because the pass that deletes a directory still holds that id in `keep`. A CLAIM THAT WAS TOO STRONG. The endpoint guard's comment said it covers a container in `created` state that `listRunningSandboxes` cannot see. It does not: `.Containers` lists RUNNING endpoints only, measured under #357, so a `created` container is invisible to both. The launch window is covered by the directory, and the guard covers a running session whose directory neither set knows about. The comment, `docs/sandbox.md` ("safe twice over") and `SECURITY.md` all said the stronger thing and now say this one. THREE PROPERTIES THE ROUND ASSERTED AND DID NOT PIN. Each mutation below was green before this commit and is red after. - The anchors on `SANDBOX_NETWORK_SHAPE`. The test that looked like the pin built its own regex from the same two constants, so it asserted a property of its own string. It now uses the exported constant, and the sweep's fixtures gained `my-pi-sandbox-notes-net` and `pi-sandbox-x-net-backup`, which the substring filter really does return and which only the anchors keep out. - The yield between networks, pinned the way `retention-sweep.test.mjs` pins its sibling. - The `start.mjs` wiring. Deleting the line left the suite green, so the whole feature could have been unwired in production unnoticed. SPEC CORRECTIONS, one of them of this branch's own first commit. `INT-EGRESS-POLICY-CONTRACT` was marked UNCHANGED by a commit that amended it: the edited "what is left behind and what removes it" bullet is that entry's, not `INT-SANDBOX-CONTRACT`'s. Also: the Acceptance promised a log line for every network the sweep passes over, where only the attached case is said; three places said the network dies with its directory rather than one pass later; and `OQ-007`'s gloss read `sandbox_reaper_skipped` as "this pass established nothing", which the per-entry catch has never meant. Refs #337 Signed-off-by: Rob Boerman <robboerman@live.nl>
…t was guarded (#337) An adversarial pass over the sweep measured a case that falsifies a claim this project shipped eight days ago, and I reproduced it independently before changing anything. On docker 27.4.0, a container CREATED on a network but never started is: - absent from `docker ps`, so `listRunningSandboxes` cannot see it; - absent from `network inspect --format '{{json .Containers}}'`, so the endpoint guard cannot see it; - and the `network rm` SUCCEEDS, after which `docker start` fails with "network not found" and that container can never run. So `egress.mjs`'s docblock, written under #357, is wrong where it generalises "listed and holds the network agree". It holds for a running endpoint and for a stopped one. It does not hold for `created`, and `created` is exactly the state `docker run` leaves a sandbox in for 230 ms on a local image, or for the whole pull when the image is not local. The daemon is a backstop for a RUNNING endpoint and for nothing else, and the sweep's comment told the next reader the opposite. The docblock now says what was measured. The sweep takes a third listing, `docker ps -a --filter status=created --filter name=pi-sandbox-`, read before anything else and failing the whole sweep closed when it does not answer. Nothing else on the daemon reports that state, so sweeping without it is sweeping on evidence known to be incomplete. A leftover container stuck in `created` now holds its network back, which is the right direction: `openSandbox` would refuse that run by name until an operator removes it anyway. Confirmed against real docker rather than only in fakes. Before: the sweeper removed a network whose sandbox was mid-launch, detached the proxy, and `docker start` then failed. After, same setup: the network is untouched and not even inspected, `docker start` succeeds, and the next sweep names it `sandbox-attached`. Two mutations go red: drop the `launching` consult, and let a failed `ps -a` fall through instead of failing closed. The reachability is worth stating plainly rather than overselling the fix. The directory keep set already covers this window on a supported deployment, because an open in flight always has its retained directory, and the adversarial pass could not stage a route to it that was not a contrivance. What this closes is the gap between that and what the code's own comments claimed: three guards were named, one was carrying all of it, and one of the other two was documented backwards. Also filed, pre-existing and out of scope: #362, `--publish` is inert while the egress policy is armed because the sandbox is on an `--internal` network, and the CLI prints `published:` anyway. Specs: `INT-SANDBOX-CONTRACT` and `DES-EGRESS-DENY-ON-A-DEDICATED-NETWORK` gain the third listing and the corrected measurement; `docs/sandbox.md` and `SECURITY.md` follow. Refs #337 Signed-off-by: Rob Boerman <robboerman@live.nl>
…ack (#337) Round two of the gate. The launch-window guard the previous commit added was read at the top of the pass, which is the one placement that cannot work, and a review pass drove it on real docker: an open creates its NETWORK before its CONTAINER, so a snapshot taken before the candidate listing is older than the thing it has to protect. Measured exposure was 486 ms for the first candidate, growing with every network ahead of it in the loop; the network was removed and `docker start` then failed with "network not found". So the two container listings become one, and it is the call immediately before the destructive verb: `docker ps -a --filter name=pi-sandbox-<id>`, asked per candidate, after the endpoint read. The order of the whole function is now candidates first, then every piece of evidence that protects one, freshest last. A check-then-act still has a gap; this one is the width of a single command rather than of the pass. Confirmed against real docker by creating the container mid-pass, right after the sweeper's `network ls`: the network survives, the note names it, and `docker start` succeeds. Only `exited` and `dead` free a network. An allowlist rather than a denylist, so a state a future daemon adds is hands off by default, and unlike the `keep` and `running` skips this one is SAID. That matters more than it looks: a container stuck in `created` never ages out (`--rm` fires on exit, so it never fires on one that never started) and nothing in this project removes a `pi-sandbox-` container, so its network would be held back forever with nothing on the host naming it. It is invisible to `docker ps`, hence to `listRunningSandboxes` and `pi-dispatch sandbox --list`, and `network inspect` does not list it either, so the `egress-network-exists` refusal cannot name it and the commands that refusal prints do not clear it. A silent permanent skip is issue #337 arriving from the other side. Two more holes in the same direction, both from the same pass: - A sandbox root that does not exist made the reaper return before the network sweep, permanently, on exactly the host most likely to be holding leftovers. ENOENT is now an empty listing rather than a failed read, and that is safe as well as useful: with no root, `resolveSandbox` refuses every run, so no open can be in flight. Any other error still skips the pass. - The wiring assertion was vacuous. `typeof x === "function"` cannot tell the sweeper from the factory that makes it, so dropping two characters in `start.mjs` left the feature dead and the suite green. The test now asserts the reaper was handed the factory's result, and the sweeper is injected into the wiring tests rather than constructed for real, since calling the real one would shell out to docker. Also recorded rather than left for the next reader: the corrected `created` measurement reaches `live-probes.mjs`'s and doctor's canary sweeps too, and both are deliberately unguarded, with the reason written at each call site. Each touches only a network whose owning pid is DEAD, and a dead process has no launch in flight. The sandbox sweep, whose owner may be alive, is the one that needs the guard. Prose defects fixed, all of them in text this branch wrote: a bullet that counted three load-bearing properties and listed four; a bullet whose opening still described the first commit's keep set while its middle described the second's; a revision row with the same drift; the removal predicate stated as three conditions in `REQ-RESURRECTABLE-SANDBOX` and `docs/sandbox.md` where the code has four. Refs #337 Signed-off-by: Rob Boerman <robboerman@live.nl>
…fely (#337) Round three of the gate, and the last one this branch takes. What survives is filed as #363 rather than fixed here. THE UNPINNED GUARD. `containerHolds` compares the whole container name the producer builds, and nothing made it stay that way: swapping `===` for `startsWith` left the suite green. The fixture that looked like its test only exercised noise on the LEFT (`my-pi-sandbox-notes`), and the half that matters is noise on the RIGHT, which the filter really returns: `pi-sandbox-abcdef` comes back from a filter on `pi-sandbox-abc`, and `gh-1` beside `gh-12` is that shape in real job ids. Under a prefix compare one run's mid-launch container shields a DIFFERENT run's network from ever being swept, which is this issue arriving from the other side. A fixture closes it. Worth recording while correcting the comment: `--filter name=` is not a substring match at all, it is an unanchored REGEX. Measured: `--filter name=pi-sandbox-a.c` returns `pi-sandbox-abc`, and `sanitizeJobId` permits `.`. Harmless as written, because the compare is against the whole name, and precisely the thing to know before anyone slices a prefix off that listing. TWO DEFAULTS THAT ANSWERED THE WRONG WAY. `retained` defaulted to "nothing is retained", which is the least protective answer available. Its sibling default in the reaper can be a no-op because a missing sweeper means no sweep, and that is safe; a missing directory listing means a sweep that ignores every retained run, which is the #277 harm with no log line. It now throws, and every caller with no listing says so by handing in `() => []`. The container-state allowlist compared case-sensitively while reading another tool's rendering, which is exactly why the sibling `networkAbsentInDaemonWords` is case-insensitive. Docker renders `{{.State}}` lowercase, so this is unreachable today; a runtime that capitalised it would have held every leftover network back forever behind a `sandbox-present` note. A failed per-candidate `docker ps -a` also stops sharing a token with a failed `network inspect`. The two have different causes and different fixes, and an operator grepping `sandbox_network_not_reaped` should not have to guess which read went quiet. It is `containers-unreadable`. PROSE. The rewritten contract bullet had dropped the injection rationale that its own revision row still claimed it carried, so one clause is back. And the ENOENT justification is now scoped to the root THIS WORKER resolves: an opener computes its own from its own environment, which `docs/sandbox.md` already says routinely differs, so a worker whose root is missing while an operator's CLI resolves a populated one sweeps with an empty keep set. Bounded, because the next open just creates the network again and one in flight is still held by the container look. Filed as #363, measured and not fixed here: the check-then-act window is two commands wide rather than one, because `removeNetworkOrSay` detaches before it removes, and it widens by one command per endpoint. Reaching it still needs both deeper guards to miss. The same issue records that a retained directory which cannot be removed holds its network indefinitely with only the directory named, and that Podman's `.State` vocabulary is unmeasured. Refs #337 Signed-off-by: Rob Boerman <robboerman@live.nl>
This was referenced Sep 21, 2026
Merged
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.
Issue #337, item 1. The other three items are the panel's, and land in their own PR; this one stays open until then.
The defect
openSandboxcreatespi-sandbox-<jobId>-netbefore it launches the container and removes it in its ownfinally. A terminal closed mid-session never reaches thatfinally, and nothing else removed it either: the boot reaper lists onlypi-job-networks, and commit5dbcddc(#277) withdrew removing one at OPEN time. So the network survived every restart, and every later open of that run refused withegress-network-existsuntil an operator ran the two commands by hand.Why this is not a reversal of #277
#277 withdrew automatic removal at open time, where it raced a concurrent open: the second open removed the first's network after the first had created it, disconnecting the proxy from a live shell. That decision stands, and the refusal at open time is unchanged.
This is a different mechanism with a different input. A background sweep on the retention reaper is keyed on the retained directories, and an open in progress always has one, because
resolveSandboxrefuses a job whose directory is gone.The rule
A
pi-sandbox-<id>-netis swept only when<id>is absent from the retained directories, is not running, has nopi-sandbox-container attached, and has no container of its own in any state butexitedordead. Endpoints are inspected first, what was seen is detached, and the removal is a plainnetwork rm, never-f.Four properties carry the argument, and each has a comment beside it:
resolveSandboxa moment earlier. A fresh one covers the other end:retainJobDircreates a retained directory at job END, in this same process, so a job can finish and its run be opened while the pass is still running. The fresh half arrives as a closure so the sweeper reads it after its own candidate listing.docker startdead with "network not found".network disconnectis issued rather than asserting the network survived, because the weaker assertion passes on docker's refusal alone.docker ps -afor that id. See the measurement below.The sweeper is injected rather than imported:
sandbox.mjsimportsreadManifestfromsandbox-store.mjs, so the other direction is a cycle, and injection keepssandbox-store.mjsdocker-free in its own tests.A measurement that corrects one this project shipped eight days ago
Under #357,
egress.mjs'snetworkEndpointsdocblock generalised a measurement to "listed and holds the network agree". On docker 27.4.0 that holds for a running endpoint and for a stopped one. It does not hold forcreated:docker runleaves a sandbox in that state for 230 ms with the image local, and for the whole pull when it is not. The docblock now says what was measured, and the sweep asksdocker ps -afor that id as the call immediately before the destructive verb. Both othernetworkEndpointscallers (live-probes.mjs, doctor's canary) carry the residual in a comment and are deliberately unguarded: each touches only a network whose owning pid is DEAD, and a dead process has no launch in flight.Naming, and one silence that is deliberate and one that is not
Per
OQ-007's property that one grep covers boot and every tick, a per-network outcome isreaped_sandbox_networkorsandbox_network_not_reapedwith a fixed reason token (unreadable,containers-unreadable,sandbox-attached,sandbox-present,rm-failed); only the sweep's own fault keepssandbox_reaper_skipped. A network skipped because its run is retained or running is passed over in silence, because a line per retained run per pass is noise. A network held back by a container of its own is SAID, because a container stuck increatednever ages out (--rmfires on exit, so never on one that never started), nothing in this project removes api-sandbox-container, and it is invisible todocker ps, topi-dispatch sandbox --listand tonetwork inspect, so without a line nothing on the host would name it.The cost, which narrows the issue's acceptance line deliberately
#337 asks for "a leftover sandbox network with no sandbox attached" to be swept. This sweeps one with no sandbox attached and no retained directory, so a leftover whose run is still resurrectable survives until its window closes, and then one pass longer, because the pass that deletes a directory still counts that run as retained. Until then the next open of that run refuses with the exact commands, which is today's behaviour unchanged. #337's complaint is unboundedness rather than friction, and the stricter reading is what reintroduces #277's race.
Verification
dated-fixture-check,test-count-checkandtemp-dir-checkgreen.createdto the sweepable states, or invert the allowlist into a denylist; compare states case-sensitively; let a failed listing fall through; unanchor the network shape; add-f; drop the yield; make the ENOENT root skip again; defaultretainedto empty; pass the sweeper factory uncalled.network ls, leaves its network untouched with{reason: "sandbox-present"}anddocker startthen succeeds. With the guard removed, the same run removes the network anddocker startfails. A running session blocks the detach; a foreign network outside the name shape survives; the proxy stays up throughout.Review rounds, and what is filed rather than fixed
Three rounds, three reviewers each, against a merged diff, an executing pass and an adversarial pass. Nineteen confirmed findings, and the number worth recording is that two were in the original code and the rest were in the repairs. The branch's round cap then applies: the last round took one simpler-rule fix and what survived is filed.
#363: the check-then-act window is two commands wide rather than one, because
removeNetworkOrSaydetaches before it removes, and it widens by one command per endpoint. Reaching it still needs both deeper guards to miss. The same issue records that a retained directory which cannot be removed holds its network indefinitely with only the directory named, and that Podman's.Statevocabulary is unmeasured.#362, found in passing and unrelated to this change:
--publishis inert while the egress policy is armed, because the sandbox is on an--internalnetwork, and the CLI printspublished:anyway.Specs and docs
AMENDED:
REQ-RESURRECTABLE-SANDBOX,INT-SANDBOX-CONTRACT(one clause corrects "such a network stays until an operator removes it"; another corrects a bullet written last week under #357),INT-EGRESS-POLICY-CONTRACT,DES-EGRESS-DENY-ON-A-DEDICATED-NETWORK,DES-SANDBOX-IS-A-FRESH-CONTAINER(two Rejected entries this change tested rather than reversed),OQ-007.UNCHANGED, checked:
REQ-EGRESS-ALLOWLIST,REQ-DEPLOYMENT-BOOTSTRAP,REQ-LOCAL-JOB-VISIBILITY,INT-LIVE-PROBE-CONTRACT(behaviour untouched; the corrected measurement was checked against it and its call site says why it needs no guard),DES-RETENTION-SWEEPS-ON-A-TIMER,DES-CONCURRENCY-3,CONST-ISOLATION-CONTAINER-PER-JOB(its sandbox clause is about a container name, and the boot reaper's filter is untouched),OQ-016.Docs:
docs/sandbox.md,SECURITY.md's namespace disclosure, and both.env.examplecopies.Out of scope, re-filed rather than lost
The sandbox launcher is hard-wired to the docker CLI and
sandboxVenueRefusalreopens only thelocaladapter by name. That belongs to #354, and it is re-filed there rather than disappearing when this issue is eventually resolved.Refs #337