fix(sandbox): reclaim mount-source dirs on a busy host - #8559
Conversation
First Principles Review (Fable 5) — ✅ PASSPremise-level review of All evidence gathered — the contract's mandatory lenses ran against the patch, the intent file, and the surrounding repo (consumer counts for First-Principles-Verdict: PASS A measured, self-inflicted spawn DoS gets a cause-level fix: the gate now asks the question the OS constraint (NO_NEW_PRIVS pins uid) actually supports. What this change shipsIntent: make agent spawning survive and self-heal a runtime tmpfs its own sandbox launcher filled with mount-source inodes — a FIX.
The conftest isolation hunk is mandated by the documented host-floor invariant (tests must not reach the developer's real [FIRST-PRINCIPLES-REVIEWED] 4a5404a |
Opus 4.8 Review — ✅ no blocking findingsReviewed Review detailsI've analyzed both candidates against the diff and the surrounding code. Candidate 1 (legacy heal stamps its marker when the runtime root can't be scanned): The stamp-on-scandir-failure asymmetry is real in the code, but it fails the concrete-input/observable-outcome bar. Candidate 2 (conftest pin-scan stub doesn't fail No Step-2 finding survives the same bar: the coverage accounting, budget bounding, marker-stamp fences (age cohort, budget truncation, O_NOFOLLOW), and boot-reclaim wiring are each grounded and tested in the diff. No findings. [OPUS-REVIEWED] 4a5404a Verdict parsed from the review's SHA-scoped output markers for commit False positive or not applicable? A repository writer can comment: |
GPT 5.6 Review — ✅ no blocking findingsGPT 5.6 completed its review of This comment is updated in place on each push. Review detailsNo findings. False positive or not applicable? A repository writer can comment: |
Design Review (Fable 5) — ✅ PASSDesign-level review of Design-Verdict: PASS Root-cause fix (gate asks the derivable claim, not host-wide settle) plus a heavily fenced one-shot heal; every deletion risk is named, bounded, and fails toward retention. [DESIGN-REVIEWED] 4a5404a |
Field validation of the dir gate, on the host from #8558Ran the patched gate against the real 939k-entry pile, reusing the INSTALLED module's own name parsing, pin scan and pid probe so only the gate differed:
Second commit: an update now heals an already-stuck host, within a bounded passReclaiming the dir class fixes the leak going forward but leaves a host that is ALREADY at the ceiling broken, because its pile predates the
Two review notes worth flagging:
|
1648ced to
0ab002d
Compare
Re-review found a real defect in the legacy fence — fixed, and the branch is now one squashed commit (
|
5c7474a to
2223441
Compare
2223441 to
50f2a68
Compare
|
|
50f2a68 to
d55b585
Compare
acac209 to
2bdc686
Compare
|
2bdc686 to
2d2c3bb
Compare
|
2d2c3bb to
052decc
Compare
|
052decc to
63733ba
Compare
…8558 The mount-source sweep (#6268) reclaimed the FILE class and never the DIRECTORY class. Its dir branch gated every removal on the host-wide scan_complete flag from _mount_pinned_source_names(), and that flag drops for reasons that cannot involve a sandbox at all. Measured after ~21h of hourly sweeps: 929,540 dirs retained against 511 files reclaimed, /run/user/$UID at 100% of its 3.24M inodes, every spawn dying with "Failed to start transient scope unit: No space left on device". Reproduced on the affected host with the installed build's own scan: complete=False on every call, because 29 zombie thread-group leaders answer EINVAL for mountinfo (#8090 on main reads their live siblings through task/; the installed build predates it) -- and the same flag also drops for root's or another user's unreadable task and for a hidepid procfs, on hosts #8090 does not help. That flag asks the wrong question. A source is bindable only by a launcher descendant -- which keeps THIS uid (NO_NEW_PRIVS; a nested user namespace stats as the overflow uid) -- or by root, so what the gate needs is narrower than host-wide coverage: was every task of those uids read, and did none depart unaccounted for? 1. The dir gate accepts that narrower claim. _mount_pinned_source_names fills a _PinScanCoverage beside its flag: `covered` holds when every task that could hold a source this uid staged -- this uid's, the overflow uid's, root's (root can nsenter any namespace), or one gone before its uid could be read -- was read, none was unreadable, and none departed between the FINAL pass's listing and its read (a departure on an earlier pass is followed by a re-listing that shows any child it handed the namespace to; the final pass has no such re-listing). Another user's unreadable or departing task lowers `complete` but not `covered`; a hidepid procfs, which hides root's tasks, lowers both. The gate is `entry in pinned or not (scan_complete or covered)` -- a readable pin always wins; without a uid to compare against (no os.getuid; an unreadable overflowuid sysctl) every task counts as a possible holder, so coverage fails closed. On a filtered a LIVE leader that could be a holder also has its sibling threads' mountinfo read (a thread can unshare(CLONE_FS) + setns into a namespace its leader is not in; proc lists leaders only), a departed sibling re-reads its group on the next pass -- measured 3.5k sibling reads in 0.13s here. On a filtered procfs the scan now keeps reading every pid the listing DOES show before reporting incomplete, instead of returning an empty set. Both scans strip a //deleted suffix so a source removed while still bound pins by its real name. No per-entry process-group probe: an earlier cut of this branch keyed sources on a group id and accepted "no such group" as evidence, which review showed unsound three ways (the fork child's pid is not a group id; a setsid() descendant leaves the group; its group cannot be attributed after it departs) -- the coverage claim above subsumes the only case the probe existed for. Field-validated with a read-only dry run of the shipped gate on two hosts carrying inherited piles: 193,203 and 49,231 dirs all reclaimable (scan complete=True, covered=True, 0.05s), 32 and 6 live-pinned kept. 2. One-shot reclaim of the PRE-#6268 residue (_cleanup_legacy_mount_source _residue), so a host that is ALREADY at the ceiling heals on update instead of staying broken with the fix installed (1,836,596 such pid-less entries on the same host). An unkeyed name cannot be proven ours, so every fence keeps a stranger's entry: the session runtime tmpfs only (/run/user/$UID -- never /dev/shm or the shared tempdir, where any same-uid program's tempfile scratch legitimately lives), tempfile's exact shape, own uid, DIRECTORIES only in mkdtemp's exact 0o700 (the old build's mkstemp file sources are left alone: an unlinked file another program still holds open loses what it writes next, which no fence can rule out, while an empty dir holds nothing to lose), past the 24h backstop (every real member is, by construction), unbound per a scan that is complete OR read every possible holder (/run/user/$UID is 0o700 and this uid's, so the same coverage claim the dir gate accepts applies), a PILE threshold (64 candidates passing every other fence before any is touched -- the pile is the only provenance an unkeyed name has, and a stray scratch dir or two is retained outright), and os.rmdir's refusal on a populated dir as the emptiness fence (the legacy SSH shadow dir survives). A marker retires the pass once it has reached the end with nothing retained for age alone -- a host upgrading within a day of its last old-build spawn keeps the pass until that cohort ages out -- or once a root proves it holds no pile. The bind scan delegates to _mount_pinned_source_names via a new matcher predicate rather than re-walking /proc: a first cut filtered on the mountinfo root field's DIRNAME -- that field is the source's path within its OWN filesystem (/tmpab12cd34, never /run/user/$UID/...), so the fence matched nothing -- and reported every EINVAL as a gap, which 29 zombie leaders on the real host would have made permanently inert. Both scans strip a //deleted suffix so a source removed while still bound pins by its real name. 3. Startup and boundedness. The cleanup loop DISPATCHES one reclaim pass at start (a task, not awaited) instead of waiting out its 5-10 minute interval: a stuck host cannot spawn at all, so the fix must apply now, while the loop's other sweeps must not queue behind a pass a pathological pile can make slow. Gateway readiness is untouched -- the loop is itself a task nothing awaits. Both passes honour a 10s wall-clock budget checked per batch; reclaim is decided per entry, so a truncated pass is progress and the next tick resumes, and the legacy marker is stamped only by a pass that reached the end. Tests. New cases pin: the regression (incomplete but covered scan -> stale dirs reclaimed, pinned ones kept), fail-closed when the scan is incomplete AND uncovered, coverage cleared by a final-pass departure and kept by an earlier-pass one, a foreign-uid departure keeping it while a root one and a filtered procfs clear it, a sibling thread's pin counted and its departure re-read, the legacy fences (dirs only; covered gate; age cohort withholds the marker), the mountinfo root-field shape (synthetic /proc mirroring a real host, //deleted included), boot dispatch as a non-awaited task, and budget truncation without marker stamping. conftest's host-isolation floor now also pins _launcher_tmpfs_roots and defaults _bound_source_basenames fail-closed, or the suite would scan the developer's real /run/user/$UID and /dev/shm; the legacy-residue cases are POSIX-only (their fences compare mode bits and st_uid, which Windows cannot satisfy -- CI's Windows shard 3 was red on the earlier heads for that reason); the budget tests use a scoped MonkeyPatch because monkeypatch.undo() reverts that floor including the KIROCREW_HOME pin (observed: the sweep stamped its marker in a real data home). test_cleanup_loop_runs_sandbox_sweep_via_ executor asserts call_count >= 1 (the boot dispatch is a legitimate second pass) and checks the executor offload for every recorded call. Also fixes seven pre-existing test_sandbox_argv.py failures on hosts without systemd: the cgroup-scope tests mock _probe_cgroup_scope to "available" but cgroup_scope_argv also requires trusted_system_bin( "systemd-run") to resolve, which it never does on macOS, so tests about argv SHAPE failed for an unrelated reason. A systemd_run_resolvable fixture fakes only that one name; the tests asserting degradation when it is absent patch the resolver to None themselves and are unaffected.
63733ba to
4a5404a
Compare
|
|
|
|
|
|
|
|
|
|
buluoray
left a comment
There was a problem hiding this comment.
Approving — 0 blocking findings on 4a5404a54cb2b4e66e77d360d8a2d1cf4e2270e5. Three fences on the legacy pass are unpinned by any test, though, and given this is the repo's highest-risk file-deleting path I would close them in a follow-up rather than leave them.
Reviewed in a detached worktree at the PR head, with PYTHONPATH pinned to the worktree so the tests loaded this sandbox.py and not the live checkout. Baseline 74/74 green; 13 mutations run and reverted, final tree clean. I reasoned about deletion safety from the fences up rather than from the description.
The removal decisions are all pinned
- The gate is
entry in pinned or not (scan_complete or coverage.covered)(sandbox.py:4726). Reverting it tonot scan_completereddens the regression test; droppingentry in pinnedreddens four — a live-pinned directory is never removed. coverage.coveredis sound, and the invariant is where it is claimed to be. The launcher drops all caps and then setsPR_SET_NO_NEW_PRIVSwith exit-on-failure (:3271-3311), so no descendant can regainCAP_SETUID. Every launcher descendant therefore keeps this uid, or the overflow uid in a nested userns, or root viansenter— exactly the three uids_could_be_descendantaccepts (:4383)._could_be_descendantfails OPEN on an unknown own uid, overflow uid, or task uid (:4390); flipping it toreturn Falsereddens a test.//deletedstripping (:4366) and the sibling-threadtask/scan (:4533) are both pinned — the latter by three tests, so theunshare(CLONE_FS)+setnsholder really is covered.- Fail-closed on unproven bind coverage (
:4910) and the age fence (:4980) each redden on removal. - The removed mechanism is genuinely gone. No
g<pgid>naming,getpgid,_PinScanGaps, or per-entry process-group probe remains in the sweep; the residualkillpg/setsid/process_grouphits (:3350,:4110,:7418,:7822,:7938,:8164) are the unrelated spawn supervisor. No half-removed mechanism. - The time budget is resumable per entry on both passes, and a truncated legacy pass never stamps the one-shot marker, so there is no half-deleted state and no premature retirement.
- The marker is created
O_CREAT|O_EXCL|O_NOFOLLOWunderconfig_dir(), which the gateway may write. (MyO_NOFOLLOWmutation survived, but correctly so:O_CREAT|O_EXCLalready failsEEXISTon a symlink, dangling or not, so the planted-symlink test cannot distinguish the two. Belt and braces, not a gap.)
Yellow — three fences no test decides
Each has a structural backstop, so none can delete a live mount source or a populated stranger directory on its own. But all three sit on the pass that calls os.rmdir, and a mutation to each stays green:
- The exact-
0o700mode fence (:4979) is not tested, and the test that looks like it tests it does not. Replacingif not stat.S_ISDIR(info.st_mode) or stat.S_IMODE(info.st_mode) != 0o700:withif False:stays green. The wrong-mode case attest_sandbox_mount_source_sweep.py:1336-1337buildstmploose5678at0o755— but_LEGACY_MOUNT_SOURCE_REis^tmp[a-z0-9_]{8}$(:4789) andtmploose5678istmpplus nine characters, so it is rejected by the name regex at:4958and never reaches the mode check. The dir-vs-file half is backstopped byos.rmdirrefusing files and non-empty directories; the mode bit itself is unguarded. This is the one I would fix, because it is the failure mode that rots silently: widen or relax that regex later and the mode fence goes with it, with no test going red. Atmpabcdefghat0o755would pin it. - The own-uid fence (
:4972) is not tested. Droppinginfo.st_uid != own_uidstays green — every fixture creates own-uid directories. Backstopped in practice because/run/user/$UIDis0o700and this uid's, so a foreign entry is close to impossible; still, a foreign-uid fixture would cost one test. _LEGACY_PILE_THRESHOLD = 64(:4797) — the value is asserted nowhere. Lowering it to 1 stays green. The mechanism is properly pinned (>= _LEGACY_PILE_THRESHOLD→>= 0reddens a test), but all three threshold tests monkeypatch the constant to 1 or 4 (:1161,:1361,:1376), so the production 64 — the number that decides whether a stranger's lone scratch directory is safe — is free to drift. Nit-adjacent, but it is the fence the description leans on hardest when arguing the blast radius is bounded.
None of these is a removed guard or a reachable data-loss path on the shipped configuration, so none blocks. Two small tests and one assertion would close all three.
Not verified
The field-validation numbers in the description (939k, 193k, 49k reclaimed on real hosts) — I have no access to those hosts. The logic they exercise is covered by the tests above. One of my mutations (scaling the keyed time budget) was flawed and proves nothing, since the test budget is negative and stays negative when multiplied; the budget mechanism is directly tested regardless.
Problem / Motivation
Every agent spawn on a busy Linux host dies with
AcpRuntimeDead: process exited (rc=None); withagent.log_level=WARNINGthe cause surfaces asFailed to start transient scope unit: No space left on device./run/user/$UIDis out of inodes (3,244,965 / 3,244,965), sosystemd-run --user --scopecannot allocate a unit andcgroup_scope_argv's wrapped spawn exits before the ACP handshake.What filled it is our own sandbox launcher's bind-mount sources. #6268 added a sweep for them, but it reclaims the FILE class only. Measured after ~21h of hourly sweeps on one host:
The same host also carried 1,836,596 pre-#6268
tmp*sources with no pid in the name, which the sweep documents as out of scope.Why it matters
Self-inflicted, total DoS of the agent surface — chat slots, eager spawns, warm pool, subagents — on any Linux host that spawns continuously. Nothing reclaims the tmpfs automatically (only ending the systemd user session does), the fix-forward sweep cannot reason about the legacy pile, and the failure is invisible at the default log level. An operator who updates to a build with only the dir-gate fix stays exactly as broken as before.
What changed (motivation → approach → change)
Root cause. The sweep's dir branch gated every removal on
_mount_pinned_source_names()'s host-widescan_completeflag, and that flag drops for reasons that cannot involve a sandbox. Reproduced on the affected host with the installed build's own scan:complete=Falseon every call, because 29 zombie thread-group leaders answerEINVALformountinfo(#8090 on main reads their live siblings throughtask/; the installed 0.6.0.11 predates it). The same flag also drops for another user's unreadable task, or one departing during the final pass — hosts #8090 does not help. It asks the wrong question: a source is bindable only by a launcher descendant, which keeps this uid (NO_NEW_PRIVS; a nested user namespace stats as the overflow uid), or by root, so the gate needs a narrower claim than host-wide coverage.1. The dir gate accepts a narrower coverage claim.
_PinScanCoverage.coveredholds when every task that could hold a source this uid staged — this uid's, the overflow uid's, root's (root cannsenterany namespace), or one gone before its uid could be read — was read, none was unreadable, and none departed between the final pass's listing and its read; a departure on an earlier pass is followed by a re-listing that shows any child it handed the namespace to, the final pass has none. Another user's unreadable or departing task lowerscompletebut notcovered; ahidepidprocfs, which hides root's tasks, lowers both. Without a uid to compare against (noos.getuid, unreadable overflowuid sysctl) every task counts, so coverage fails closed. A readable pin always wins. A live leader that could be a holder also has every sibling thread'stask/<tid>/mountinforead (a thread canunshare(CLONE_FS)+setnsinto a namespace its leader is not in;/proclists leaders only), a sibling departed mid-read re-reads its group on the next pass — 3.5k sibling reads in 0.25s on the incident host, surfacing 21 pins leaders alone missed. On a filtered procfs the scan now keeps reading every pid the listing does show before reporting incomplete, instead of returning an empty set; both scans strip a//deletedsuffix so a source removed while still bound pins by its real name.No per-entry process-group probe. An earlier cut of this branch keyed sources on a group id and accepted "no such group" as evidence; review showed that unsound three ways (the fork child's pid is not a group id, a
setsid()descendant leaves the group, and its group cannot be attributed once it has departed). The coverage claim above subsumes the only case the probe existed for, so it is gone along with theg<pgid>name shape — names staykirocrew_sb_<pid>_*exactly as on main.Field validation (shipped gate, read-only dry run) on two hosts carrying inherited piles: 193,203 and 49,231 dirs all reclaimable (
complete=True,covered=True; scan 0.25s with sibling threads), 32 and 6 live-pinned kept, 90 fresh-live skipped. The 938,864 figure from an earlier cut used the since-removed group probe and is history, not a claim about this code.2. One-shot reclaim of the pre-#6268 residue (
_cleanup_legacy_mount_source_residue), so an already-stuck host heals on update. An unkeyed name cannot be proven ours, so every fence keeps a stranger's entry: the session runtime tmpfs/run/user/$UIDonly (never/dev/shmor the shared tempdir), tempfile's exact shape, own uid, directories only inmkdtemp's exact 0o700 (the old build'smkstempfile sources are left alone: an unlinked file another program still holds open loses what it writes next, which no fence can rule out, while an empty dir holds nothing to lose), past the 24h backstop, unbound per a scan that is complete or read every possible holder (/run/user/$UIDis 0o700 and this uid's, so the samecoveredclaim as the dir gate applies), a PILE threshold (_LEGACY_PILE_THRESHOLD= 64 candidates passing every other fence before any is touched — the pile is the only provenance an unkeyed name has; a stray scratch dir or two is retained outright), andos.rmdir's refusal on a populated dir as the emptiness fence. A marker retires the pass once it has reached the end with nothing retained for age alone (a host upgrading within a day of its last old-build spawn keeps the pass until that cohort ages out), or once a root proves it holds no pile. When bind coverage cannot be established the pass retains everything and says so at WARNING (the keyed sweep's held-back report already does), instead of going inert silently. The bind scan delegates to_mount_pinned_source_namesvia a newmatcherpredicate rather than re-walking/proc(a first cut filtered on the mountinfo root field's dirname — that field is tmpfs-relative, so the fence matched nothing — and treated everyEINVALas a gap, which 29 zombie leaders on the real host would have made permanently inert). Both scans strip a//deletedsuffix.3. Startup and boundedness. The cleanup loop DISPATCHES one reclaim pass at start (a task, not awaited) instead of waiting out its 5–10 min interval; gateway readiness is untouched. Both passes honour a 10s wall-clock budget per pass — reclaim is per-entry, so a truncated pass is progress and the next tick resumes, and the legacy marker is stamped only by a pass that reached the end.
Also: seven pre-existing
test_sandbox_argv.pyfailures on hosts without systemd — the cgroup-scope tests mock_probe_cgroup_scopebutcgroup_scope_argvalso needstrusted_system_bin("systemd-run")to resolve. Asystemd_run_resolvablefixture fakes only that name; tests asserting degradation when it is absent patch the resolver themselves and are unaffected.Tests
test/test_sandbox_mount_source_sweep.py(73 passed on Linux):test_dirs_are_reclaimed_once_every_own_uid_task_was_read— the regression (incomplete but covered scan: stale dir goes, pinned dir stays);test_incomplete_and_uncovered_pin_scan_blocks_dir_removal— fail-closed when neither claim holds.TestMountPinnedSourceNamescoverage cases: final-pass departure clearscovered, an earlier-pass one is re-listed and keeps it; a foreign-uid departure keeps it while a root one, an unknown overflow uid and a filtered procfs clear it; a sibling thread's pin counts, its mid-read departure re-reads the group.TestLegacyResidueSweep— each fence individually (dirs only; files survive), thecoveredgate, one-shot marker withheld while an age-fenced cohort remains, wiring into the periodic entry point, boot dispatch as a non-awaited task (AST-asserted), andtest_bound_scan_keys_on_the_tmpfs_relative_root_fieldwith a synthetic/procmirroring a real host's mountinfo lines (//deletedincluded).TestSweepTimeBudget— truncation resumes on the next pass and does not stamp the legacy marker.conftest.py: the host-isolation floor now also pins_launcher_tmpfs_rootsand defaults_bound_source_basenamesfail-closed (without it the suite scans the developer's real/run/user/$UIDand/dev/shm). Budget tests use a scopedMonkeyPatch:monkeypatch.undo()reverts the floor including theKIROCREW_HOMEpin and the sweep then stamps its marker in the real data home — observed while writing them.test_session.py::test_cleanup_loop_runs_sandbox_sweep_via_executornow assertscall_count >= 1(the boot dispatch is a legitimate second pass) and checks the executor offload for every call.test_sandbox_argv.py189 passed.Manual verification
Ran the patched dir gate against the real 939k-entry pile on the affected host, reusing the installed module's own name parsing, pin scan and pid probe so only the gate differed:
pin scan complete=False(the defect, live),removed dirs=938864 files=87, held_back pinned=6, fresh_and_alive=187, failed=0,/run/user/$UID1% of inodes afterwards, 654 transient scopes started in the following 3 minutes with no furtherAcpRuntime dead. Verified the legacy bind scan on the same host:complete=Falsebefore the delegation fix,complete=Trueafter (0.05s).Related Issues
Fixes #8558
Pattern harvest
Rule candidate: review-prompt
Pattern: a host-wide / aggregate coverage or completeness flag used to gate a per-entry decision (the flag can never settle on a busy host, so the gate silently retains forever).
Checklist
feat|fix|docs|refactor|perf|test|chore|ci|build|revert: ...)