Skip to content

fix(sandbox): reclaim mount-source dirs on a busy host - #8559

Merged
bolichen97 merged 1 commit into
mainfrom
fix/sandbox-mount-source-inode-leak
Sep 5, 2026
Merged

fix(sandbox): reclaim mount-source dirs on a busy host#8559
bolichen97 merged 1 commit into
mainfrom
fix/sandbox-mount-source-inode-leak

Conversation

@bolichen97

@bolichen97 bolichen97 commented Sep 4, 2026

Copy link
Copy Markdown
Collaborator

Problem / Motivation

Every agent spawn on a busy Linux host dies with AcpRuntimeDead: process exited (rc=None); with agent.log_level=WARNING the cause surfaces as Failed to start transient scope unit: No space left on device. /run/user/$UID is out of inodes (3,244,965 / 3,244,965), so systemd-run --user --scope cannot allocate a unit and cgroup_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:

 929540 d
    511 f      # find /run/user/$UID -maxdepth 1 -name 'kirocrew_sb_*' -printf '%y\n' | sort | uniq -c

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-wide scan_complete flag, and that flag drops for reasons that cannot involve a sandbox. 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 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.

pinned, scan_complete = _mount_pinned_source_names(coverage=coverage)
if entry in pinned or not (scan_complete or coverage.covered):

_PinScanCoverage.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 none. Another user's unreadable or departing task lowers complete but not covered; a hidepid procfs, which hides root's tasks, lowers both. Without a uid to compare against (no os.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's task/<tid>/mountinfo read (a thread can unshare(CLONE_FS) + setns into a namespace its leader is not in; /proc lists 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 //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; 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 the g<pgid> name shape — names stay kirocrew_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/$UID only (never /dev/shm or the shared tempdir), 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, unbound per a scan that is complete or read every possible holder (/run/user/$UID is 0o700 and this uid's, so the same covered claim 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), and os.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_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 tmpfs-relative, so the fence matched nothing — and treated every EINVAL as a gap, which 29 zombie leaders on the real host would have made permanently inert). Both scans strip a //deleted suffix.

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.py failures on hosts without systemd — the cgroup-scope tests mock _probe_cgroup_scope but cgroup_scope_argv also needs trusted_system_bin("systemd-run") to resolve. A systemd_run_resolvable fixture 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.
  • TestMountPinnedSourceNames coverage cases: final-pass departure clears covered, 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), the covered gate, 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), and test_bound_scan_keys_on_the_tmpfs_relative_root_field with a synthetic /proc mirroring a real host's mountinfo lines (//deleted included).
  • 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_roots and defaults _bound_source_basenames fail-closed (without it the suite scans the developer's real /run/user/$UID and /dev/shm). Budget tests use a scoped MonkeyPatch: monkeypatch.undo() reverts the floor including the KIROCREW_HOME pin 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_executor now asserts call_count >= 1 (the boot dispatch is a legitimate second pass) and checks the executor offload for every call. test_sandbox_argv.py 189 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/$UID 1% of inodes afterwards, 654 transient scopes started in the following 3 minutes with no further AcpRuntime dead. Verified the legacy bind scan on the same host: complete=False before the delegation fix, complete=True after (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

  • At most two commits (one is the norm), with a Conventional Commits title (feat|fix|docs|refactor|perf|test|chore|ci|build|revert: ...)
  • Existing tests pass and new tests added for new functionality
  • Self-review completed; code follows project style guidelines
  • Documentation updated (if applicable) — N/A, behaviour documented in the sweep's docstrings
  • No secrets, credentials, or internal references in the diff

@bolichen97
bolichen97 requested a review from a team as a code owner September 4, 2026 20:46
@bolichen97
bolichen97 requested a review from buluoray September 4, 2026 20:46
@github-actions github-actions Bot added the readiness: action required A blocking check or review needs attention label Sep 4, 2026
@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

First Principles Review (Fable 5) — ✅ PASS

Premise-level review of 4a5404a54cb2b4e66e77d360d8a2d1cf4e2270e5 — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

All evidence gathered — the contract's mandatory lenses ran against the patch, the intent file, and the surrounding repo (consumer counts for _PinScanCoverage, the matcher parameter, _launcher_tmpfs_roots, and the sweep entry points at src/kiro_crew/sandbox.py:4214 and src/kiro_crew/session_cleanup.py:446). Final review follows.

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 ships

Intent: make agent spawning survive and self-heal a runtime tmpfs its own sandbox launcher filled with mount-source inodes — a FIX.

  1. Stale sandbox dirs reclaim when every possible holder was read, not only on a host-wide-perfect scan — justified (cause level: 929,540 dirs stranded, count quoted from the incident host).
  2. Live leaders' sibling threads are scanned for pins via task/ — justified (soundness of the new claim; 21 pins leaders missed).
  3. A filtered procfs still contributes visible pins instead of an empty set — justified (retention correctness).
  4. A deleted-but-bound source pins by its real name (//deleted strip) — justified, declared.
  5. One-shot reclaim of pre-fix(sandbox): reclaim the launcher's bind-mount source temps, fixes #6263 #6268 unkeyed tmp* residue, fenced six ways, retired by a marker file — justified (1,836,596-entry pile; without it an upgraded host stays dead).
  6. New persisted marker .legacy-mount-source-residue-swept in the config dir — declared, named cause (a completed pass is final).
  7. One reclaim pass dispatched at gateway start instead of waiting the 5-min interval — justified (a stuck host cannot spawn at all).
  8. Both passes bounded by a 10s resumable budget — justified (measured 11s for 939k entries).
  9. Legacy pass says at WARNING when it retains everything — justified (the original failure was invisible at default log level).
  10. Seven cgroup-scope argv tests now pass on hosts without systemd (systemd_run_resolvable) — rides along, declared, unwedges a red local gate; test-only.

The conftest isolation hunk is mandated by the documented host-floor invariant (tests must not reach the developer's real /run/user/$UID) — derived, not relitigated. Grepped for siblings of the root cause (scan_complete gating a removal): the only other match, papyrus/backend/latex.py:670, is an unrelated symlink scan — no unfixed siblings. No existing mechanism does either job; the legacy scan delegates to the existing traversal via matcher (1 consumer, _bound_source_basenames, sandbox.py:4846) rather than adding a second /proc walk, which is the smaller form, not a generalization.

[FIRST-PRINCIPLES-REVIEWED] 4a5404a

@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Opus 4.8 Review — ✅ no blocking findings

Reviewed 4a5404a54cb2b4e66e77d360d8a2d1cf4e2270e5 — this comment is updated in place on each push.

Review details

I'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. _launcher_tmpfs_roots() yields only /run/user/$UID — the owner's own 0700 dir. The realistic os.scandir failure there is ENOENT (no login session / container), and in that case there is no residue at that path to strand (the launcher would have staged in /dev/shm, which this pass deliberately never walks). EACCES or a transient vanish on one's own runtime dir is not a real trigger. So there is no input where the pile is present at the walked root AND scandir raises. The candidate's own confidence is low and it could not confirm a realistic failure mode. Dropped.

Candidate 2 (conftest pin-scan stub doesn't fail coverage closed): This is a latent fidelity gap in a test fixture, not a production defect. The candidate concedes it is "harmless" and "no shipped test appears to depend on the inert-directory contract" — i.e. no observable wrong outcome (c). It also falls in the tests/fixtures category this pipeline owns deterministically. Dropped.

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 4a5404a54cb2b4e66e77d360d8a2d1cf4e2270e5.

False positive or not applicable? A repository writer can comment:
/ai-review override fable 4a5404a54cb2b4e66e77d360d8a2d1cf4e2270e5: <one-sentence reason>

@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of 4a5404a54cb2b4e66e77d360d8a2d1cf4e2270e5 and found no blocking issues.

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] 4a5404a

False positive or not applicable? A repository writer can comment:
/ai-review override gpt 4a5404a54cb2b4e66e77d360d8a2d1cf4e2270e5: <one-sentence reason>

@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Design Review (Fable 5) — ✅ PASS

Design-level review of 4a5404a54cb2b4e66e77d360d8a2d1cf4e2270e5 — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

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

@bolichen97

Copy link
Copy Markdown
Collaborator Author

Field validation of the dir gate, on the host from #8558

Ran 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:

pin scan: pinned=94 complete=False
scanned=939144 removed: dirs=938864 files=87
held_back: pinned=6 group_alive=0
skipped: fresh_and_alive=187 unparseable_name=0 failed=0

complete=False is the defect in one line: on this host the host-wide flag never settles, so the shipped code holds back all 938,806 candidate dirs indefinitely. With the per-entry group probe, every one is reclaimed and the 6 entries a live mount still pins are kept, as are the 187 whose process is alive. /run/user/$UID went from 100% of its 3,244,965 inodes to 1% in 11s, 0 failures, and the gateway kept spawning throughout (654 transient scopes in the following 3 minutes, no further AcpRuntime dead).

Second commit: an update now heals an already-stuck host, within a bounded pass

Reclaiming the dir class fixes the leak going forward but leaves a host that is ALREADY at the ceiling broken, because its pile predates the kirocrew_sb_<pid>_ prefix — 1,836,596 such entries on the same host. So:

  • One-shot legacy reclaim for that pre-prefix residue, from the same entry point the gateway already calls. An unkeyed name cannot be proven ours, so every condition keeps a stranger's entry: launcher-chosen tmpfs roots only (never the shared tempdir), tempfile's exact shape, own uid, exact mkdtemp/mkstemp modes, past the 24h backstop, unbound per a positively-established scan, and os.rmdir's refusal on a populated dir as the emptiness fence (so the legacy SSH shadow dir survives too).
  • Boot dispatch, not awaited. The cleanup loop starts one pass immediately instead of waiting out its 5–10 minute interval, as a task: 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 startup is untouched — the loop is itself a task nothing awaits on the readiness path.
  • A 10s wall-clock budget per pass, keyed and legacy alike. Reclaim is decided per entry, so a truncated pass is progress and the next tick resumes; the legacy marker is stamped only by a pass that reached the end. 939k dirs took ~11s, so a normal backlog clears in one pass and a pathological one spreads over a few instead of holding a maintenance worker.

Two review notes worth flagging:

  • conftest's host-isolation floor now also pins _launcher_tmpfs_roots and defaults _bound_source_basenames fail-closed. The legacy sweep resolves its own narrower root chain, so without that the suite would scan the developer's real /run/user/$UID and /dev/shm. Relatedly, its budget tests use a scoped MonkeyPatch rather than monkeypatch.undo() — undo reverts the floor including the KIROCREW_HOME pin, and the sweep then stamped its marker in my real data home. Observed, not theorised.
  • test_cleanup_loop_runs_sandbox_sweep_via_executor now asserts call_count >= 1 instead of exactly once (the boot dispatch is a legitimate second pass) and checks the executor offload for EVERY recorded call rather than just the first.

pytest test/ -k sandbox → 1386 passed; the 7 failures in test_sandbox_argv.py are pre-existing on macOS (verified identical with the change stashed — they exercise systemd-run/cgroup paths that need Linux).

@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Sep 4, 2026
@bolichen97
bolichen97 force-pushed the fix/sandbox-mount-source-inode-leak branch from 1648ced to 0ab002d Compare September 4, 2026 22:26
@bolichen97

Copy link
Copy Markdown
Collaborator Author

Re-review found a real defect in the legacy fence — fixed, and the branch is now one squashed commit (0ab002d03)

The defect. The legacy-residue pass's bind-scan fence was disarmed twice over, and the unit tests could not see it because they synthesized mountinfo in the shape the code expected rather than the shape the kernel writes:

  1. It filtered on the root field's dirname against the tmpfs mount point. mountinfo field 4 is the source's path within its own filesystem, so a source staged on /run/user/$UID reads as /kirocrew_sb_… / /tmpab12cd34 — dirname /, never /run/user/$UID. bound was always empty. Verified against a live host's mountinfo.
  2. It reported every EINVAL as a coverage gap. A zombie thread-group leader answers EINVAL while sibling threads keep the namespace alive — the keyed scan already handles this via task/. The real host had 29 such zombies, so the legacy pass would have been permanently inert on exactly the host it exists for. Before: complete=False. After: complete=True, 14 legacy-shaped sources still pinned, 0.05s.

The fix. _mount_pinned_source_names gains a matcher predicate and _bound_source_basenames delegates to it, so the legacy fence inherits the zombie-leader / vanish-relisting / foreign-uid handling instead of re-implementing it wrong. Both scans strip //deleted so a source removed while still bound pins by its real name. New test uses a synthetic /proc mirroring the real host's lines.

Pre-existing red fixed too. Seven test_sandbox_argv.py cgroup-scope tests failed on any host without systemd: they mock _probe_cgroup_scope to "available" but cgroup_scope_argv also needs 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 that assert degradation when it is absent patch the resolver to None themselves and are unaffected.

Squash. The three commits are now one (0ab002d03) per the ≤2-commit CI budget; the full history of each round is in the commit body. pytest test/test_sandbox_mount_source_sweep.py test/test_sandbox_argv.py test/test_session.py → 545 passed, 8 skipped, 0 failed.

@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Sep 4, 2026
@bolichen97
bolichen97 force-pushed the fix/sandbox-mount-source-inode-leak branch 2 times, most recently from 5c7474a to 2223441 Compare September 4, 2026 22:45
@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Sep 4, 2026
@bolichen97
bolichen97 force-pushed the fix/sandbox-mount-source-inode-leak branch from 2223441 to 50f2a68 Compare September 4, 2026 22:59
@bolichen97

Copy link
Copy Markdown
Collaborator Author
  • legacy sweep deletes unrelated live temp objects (sandbox.py legacy pass, span=b426d67664d2) — fixed in 50f2a68.

An unkeyed tmp* name has no provenance, so the pass now (a) walks only the session runtime tmpfs /run/user/$UID/dev/shm and the shared tempdir are excluded outright (_launcher_tmpfs_roots), and (b) touches nothing until a root shows the pile this pass exists for: _LEGACY_PILE_THRESHOLD (64) candidates passing every other fence (own uid, exact mkdtemp/mkstemp mode, empty, >24h, unbound per a complete scan). Below that, every candidate is retained and the one-shot pass retires. A stray day-old scratch dir of another same-uid program is therefore kept; the leak class arrives by the hundred thousand. Tests: test_below_the_pile_threshold_everything_is_retained_and_the_pass_retires, test_at_the_pile_threshold_the_buffered_candidates_are_reclaimed_too, test_legacy_roots_exclude_dev_shm.

@bolichen97

Copy link
Copy Markdown
Collaborator Author
  • gone process group does not prove the mount namespace is dead (sandbox.py directory gate, span=b426d67664d2) — fixed in 50f2a68.

Confirmed against the code: on a filtered procfs the pin scan returned (pinned, False) on the pid-1 check BEFORE reading any mountinfo, so the gate's entry in pinned saw an empty set and fell through to the group probe. _mount_pinned_source_names now marks coverage incomplete and keeps reading every pid the filtered listing does show — this uid's own processes, i.e. every sandbox descendant including a setsid() one, stay visible under hidepid — so the descendant's live bind pins its source and the gate retains it regardless of the group answer. Tests: test_filtered_procfs_without_pid_1_reports_incomplete_but_still_pins, test_visible_holder_outranks_a_gone_staging_group_under_hidepid.

@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention merge conflict Branch has merge conflicts with its base — author must resolve before merge and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Sep 4, 2026
@bolichen97
bolichen97 force-pushed the fix/sandbox-mount-source-inode-leak branch from 50f2a68 to d55b585 Compare September 4, 2026 23:10
@github-actions github-actions Bot removed the readiness: action required A blocking check or review needs attention label Sep 4, 2026
@bolichen97
bolichen97 force-pushed the fix/sandbox-mount-source-inode-leak branch from acac209 to 2bdc686 Compare September 5, 2026 01:27
@bolichen97

Copy link
Copy Markdown
Collaborator Author
  • Pile threshold deletes unrelated same-UID tempfiles (_cleanup_legacy_mount_source_residue, span=b426d67664d2) — fixed in 2bdc686 by narrowing, not by adding per-entry provenance (an unkeyed name has none to check).

Agreed on the vector: an empty, day-old mkstemp file another program still holds open loses what it writes next once unlinked, and no fence here can tell it from ours. The legacy pass now reclaims DIRECTORIES only (os.rmdir, empty, 0o700, own uid, tempfile shape, aged, unbound, pile ≥ 64): an empty directory holds no data to lose, and a stranger's mkdtemp dir gets ENOENT on its next create — an error, not silent loss. The old build's file sources are left in place. test_reclaims_the_unkeyed_residue_once now asserts the file survives.

@bolichen97
bolichen97 force-pushed the fix/sandbox-mount-source-inode-leak branch from 2bdc686 to 2d2c3bb Compare September 5, 2026 01:37
@bolichen97

Copy link
Copy Markdown
Collaborator Author
  • Legacy cleanup misses mounts held by nonleader root threads (_mount_pinned_source_names, span=b426d67664d2) — fixed in 2d2c3bb, for both consumers (the keyed dir gate and the legacy pass share the scan).

Agreed: a thread can unshare(CLONE_FS) + setns into a namespace its leader is not in, and /proc lists leaders only. A live leader that could be a holder (this uid, overflow uid, root, or uid unknown) now has every sibling's task/<tid>/mountinfo read; an unreadable sibling makes coverage unproven (sticky), a sibling that departed mid-read has its group re-read on the next pass (a departure on the final pass clears covered), EINVAL on a sibling is a thread mid-exit and holds no namespace. Other users' threads are skipped (they cannot enter a namespace this uid staged). Measured on the incident host: 3.5k sibling reads, scan 0.03s → 0.25s, still complete=True covered=True — and 21 additional pinned names surfaced from threads alone, so the vector is not theoretical there. Tests: test_a_sibling_thread_in_another_namespace_pins, test_a_sibling_thread_departing_on_the_final_pass_clears_coverage, test_a_sibling_thread_departure_is_re_read_on_the_next_pass.

@bolichen97
bolichen97 force-pushed the fix/sandbox-mount-source-inode-leak branch from 2d2c3bb to 052decc Compare September 5, 2026 01:45
@bolichen97

Copy link
Copy Markdown
Collaborator Author
  • Marker write follows a planted symlink (_cleanup_legacy_mount_source_residue stamp, span=b426d67664d2) — fixed in 63733ba: the marker is created with O_WRONLY|O_CREAT|O_EXCL|O_NOFOLLOW (0o600); a link or pre-existing entry at the path fails the stamp and the idempotent pass simply repeats. Test test_a_planted_symlink_at_the_marker_is_not_followed (dangling link to security_policy.json: target not created, link untouched). Not conceding the premise that the agent can write in config_dir() — it sits on _SENSITIVE_HOME_DIRS — but the hardening is two lines and the right default for a gateway-side write.

@bolichen97
bolichen97 force-pushed the fix/sandbox-mount-source-inode-leak branch from 052decc to 63733ba Compare September 5, 2026 01:53
…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.
@bolichen97
bolichen97 force-pushed the fix/sandbox-mount-source-inode-leak branch from 63733ba to 4a5404a Compare September 5, 2026 02:15
@bolichen97

Copy link
Copy Markdown
Collaborator Author
  • Root-held mount namespaces can be corrupted (sandbox.py directory gate, span=b426d67664d2) — fixed in acac209.

Agreed: root can nsenter a sandbox's namespace, so root's tasks are possible holders. _could_be_descendant now also returns True for uid 0, and a filtered procfs (pid 1 hidden ⇒ root's tasks hidden) clears coverage.covered along with complete. Another user's unreadable/departing task remains the only thing that lowers complete without covered. Tests: test_filtered_procfs_clears_coverage, test_root_departure_clears_coverage, test_foreign_uid_departure_keeps_coverage. Re-verified on the incident host: complete=True covered=True.

@bolichen97

Copy link
Copy Markdown
Collaborator Author
  • held-back message names the removed group probe (sandbox.py diagnostic) — fixed in acac209: now "pin scan incomplete and descendant coverage unproven".

@bolichen97

Copy link
Copy Markdown
Collaborator Author
  • covered rests on the NO_NEW_PRIVS / same-uid invariant; pin it where the launcher sets itaddressed in acac209: the launcher's PR_SET_NO_NEW_PRIVS site now carries a comment naming the sweep gate that depends on it and what breaks if a descendant could change uid.

@bolichen97

Copy link
Copy Markdown
Collaborator Author
  • Legacy pass deletes unattributable tmp* entries on statistical provenanceacknowledged, kept; flagged to the PR owner as the human decision it is. The fences (own uid, exact tempfile shape and mode, empty-only via rmdir, session tmpfs only, ≥64-candidate pile, age) bound the harm to an empty day-old scratch dir of this uid on the runtime tmpfs.

@bolichen97

Copy link
Copy Markdown
Collaborator Author
  • Legacy pass gates on host-wide complete alone; reuse coverage.coveredfixed in 2bdc686: _bound_source_basenames(coverage=...) now threads the same _PinScanCoverage, and the pass gates on complete or covered (/run/user/$UID is 0o700 and this uid's, so this uid + overflow + root is every reachable holder). Test test_incomplete_but_covered_scan_still_heals; conftest's isolation floor fails closed on both claims.

@bolichen97

Copy link
Copy Markdown
Collaborator Author
  • Item 9 (cgroup-scope tests on hosts without systemd) is separableacknowledged, kept: without it the suite cannot run green on the incident host, where the rest of this PR was validated.

@bolichen97

Copy link
Copy Markdown
Collaborator Author
  • One-shot marker can retire the legacy pass while a young age-fenced cohort remainsfixed in 2bdc686: the stamp is withheld whenever any legacy-shaped dir was retained for age alone (young_retained); the pass retires on the first complete walk that finds none. Test test_a_cohort_under_the_age_fence_withholds_the_marker walks the full sequence (old reclaimed, marker withheld, cohort ages, reclaimed, then stamped).

@bolichen97

Copy link
Copy Markdown
Collaborator Author
  • Boot-dispatched task and first tick can run _sweep_sandbox_artifacts concurrentlyacknowledged, not changed: per-entry idempotence (rmdir/remove of an already-gone entry is a caught OSError) makes the overlap harmless, and the marker is stamped only after a walk that reached the end, so the overlap cannot retire the pass early either.

@bolichen97

Copy link
Copy Markdown
Collaborator Author
  • Test module docstring still describes the abandoned group-probe design; PR description lists test names that do not existfixed in 052decc (docstring now states the complete-or-covered gate; the hidepid test's rationale no longer mentions a group probe) and the PR description's Tests section now lists the shipped test names.

@bolichen97

Copy link
Copy Markdown
Collaborator Author
  • Legacy mkstemp file residue and the /dev/shm pile stay unfixed by designacknowledged: both are deliberate constraints (open-fd data loss; provenance), recorded in the legacy pass docstring.

@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: passed Eligible automated validation passed for the current revision and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Sep 5, 2026
@bolichen97
bolichen97 merged commit f2d0891 into main Sep 5, 2026
65 checks passed
@bolichen97
bolichen97 deleted the fix/sandbox-mount-source-inode-leak branch September 5, 2026 03:03
@github-actions github-actions Bot removed the readiness: passed Eligible automated validation passed for the current revision label Sep 5, 2026

@buluoray buluoray left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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 to not scan_complete reddens the regression test; dropping entry in pinned reddens four — a live-pinned directory is never removed.
  • coverage.covered is sound, and the invariant is where it is claimed to be. The launcher drops all caps and then sets PR_SET_NO_NEW_PRIVS with exit-on-failure (:3271-3311), so no descendant can regain CAP_SETUID. Every launcher descendant therefore keeps this uid, or the overflow uid in a nested userns, or root via nsenter — exactly the three uids _could_be_descendant accepts (:4383).
  • _could_be_descendant fails OPEN on an unknown own uid, overflow uid, or task uid (:4390); flipping it to return False reddens a test.
  • //deleted stripping (:4366) and the sibling-thread task/ scan (:4533) are both pinned — the latter by three tests, so the unshare(CLONE_FS) + setns holder 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 residual killpg / setsid / process_group hits (: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_NOFOLLOW under config_dir(), which the gateway may write. (My O_NOFOLLOW mutation survived, but correctly so: O_CREAT|O_EXCL already fails EEXIST on 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:

  1. The exact-0o700 mode fence (:4979) is not tested, and the test that looks like it tests it does not. Replacing if not stat.S_ISDIR(info.st_mode) or stat.S_IMODE(info.st_mode) != 0o700: with if False: stays green. The wrong-mode case at test_sandbox_mount_source_sweep.py:1336-1337 builds tmploose5678 at 0o755 — but _LEGACY_MOUNT_SOURCE_RE is ^tmp[a-z0-9_]{8}$ (:4789) and tmploose5678 is tmp plus nine characters, so it is rejected by the name regex at :4958 and never reaches the mode check. The dir-vs-file half is backstopped by os.rmdir refusing 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. A tmpabcdefgh at 0o755 would pin it.
  2. The own-uid fence (:4972) is not tested. Dropping info.st_uid != own_uid stays green — every fixture creates own-uid directories. Backstopped in practice because /run/user/$UID is 0o700 and this uid's, so a foreign entry is close to impossible; still, a foreign-uid fixture would cost one test.
  3. _LEGACY_PILE_THRESHOLD = 64 (:4797) — the value is asserted nowhere. Lowering it to 1 stays green. The mechanism is properly pinned (>= _LEGACY_PILE_THRESHOLD>= 0 reddens 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

3 participants