Skip to content

fix(sandbox): purge per-session _cache/<id> at delete time, not only via the reaper - #2414

Merged
eumemic merged 4 commits into
masterfrom
detail/bug-fix/fix-sandbox-purge-per-session-cache-id-at-delete-t-4ca1fd
Sep 8, 2026
Merged

eumemic merged 4 commits into
masterfrom
detail/bug-fix/fix-sandbox-purge-per-session-cache-id-at-delete-t-4ca1fd

Conversation

@detail-app

@detail-app detail-app Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

Detail bug report: View on Detail

Summary

purge_session_directories (the synchronous, delete-time host-dir cleanup run from delete_session) removed every per-session host directory except the per-session /root/.cache bind-mount source. Commit db1ebfa4 (#2347) added <workspace_root>/_cache/<session_id> and its asynchronous reaper but — unlike af52d8ac did for /tmp — never registered session_cache_dir(session_id) in the purge candidate tuple, so a deleted session's _tmp/<id> vanished the moment its row was gone while _cache/<id> lingered until the periodic host_dir_reaper aged it out (and under the documented P1 disk-fill override host_dir_reaper_enabled=false the reaper is a no-op, so _cache had zero cleanup and accumulated indefinitely). This registers the cache dir in purge_session_directories, restoring the symmetry with _tmp.

Substrate state changes

None — code-only change. The _cache/<id> bind mount and its host dir already exist (created by ensure_session_cache_dir at provision time); this only adds the missing delete-time rmtree of that already-existing dir.

Test plan

  • Strengthened tests/unit/test_purge_session_directories_jail.py: _session_owned_dirs now lists all six per-session dirs (it previously omitted both mounts — _tmp and _cache), and the legitimate-purge, per-candidate-reclaim, and shared-root-refusal cases now cover _tmp/_cache with sibling-session bystanders proving per-session scoping. 17/17 pass.
  • Added tests/integration/test_delete_session_purges_cache_dir.py: drives the real delete_session service against a migrated Postgres testcontainer and asserts _cache/<id> (and _tmp/<id>) are gone the moment delete_session returns, with host_dir_reaper_enabled both true and false. 2/2 pass.
  • RED→GREEN verified at each layer: reverting the one-line fix makes the cache-dir assertion fail (and only that assertion — _tmp still passes) across the unit, integration, and a real-Docker e2e smoke; re-applying turns all green.
  • Routine gates green: mypy src tests (strict, 1084 files), ruff check/ruff format --check, full uv run pytest tests/unit -q (6093 passed, 1 unrelated skip, 0 failures).
  • End-to-end (not versioned): ran a real-Docker smoke against a locally-built sandbox image (aios-sandbox:ci) driving a scripted model — provisioned a sandbox, confirmed the host _cache/<id> existed post-provision, deleted the session via the service, and confirmed the host dir was gone immediately, both with the reaper on and off. Excluded from the PR since the integration test already guards the real delete_session call site at far lower maintenance cost.

Risk / rollback

Low. The new candidate is structurally identical to the existing _tmp entry (same _per_session_child helper, same per-session exclusivity proof) and passes the same _purge_target_if_owned ownership guard, so no refusal is weakened — verified by the unchanged jail-suite refusal tests (account root, sibling sessions, shared roots including _cache itself, out-of-jail, symlink-escape). Roll back by reverting the one-line addition; the asynchronous reaper continues to backstop cleanup as it does today.


Automatic Fixes PRs can be configured here.

…via the reaper

Commit db1ebfa (#2347) added the per-session /root/.cache bind-mount source
— <workspace_root>/_cache/<session_id>, built by the same _per_session_child
helper as /tmp — together with the asynchronous _reap_session_cache reaper,
but, unlike af52d8a did for /tmp, omitted the matching
(session_cache_dir(session_id), ...) entry from purge_session_directories.

So a deleted session's _tmp/<id> was removed the moment its row was gone,
while _cache/<id> survived on disk until the periodic host_dir_reaper aged
it out. Under the documented P1 disk-fill override
(host_dir_reaper_enabled=false) the reaper short-circuits to a no-op, and
since the synchronous purge never gated on the kill-switch, _cache had zero
cleanup and accumulated indefinitely — the exact warm-cache workload
db1ebfa names as the regrowth driver.

Register session_cache_dir(session_id) in the purge_session_directories
candidate tuple, restoring the symmetry with session_tmp_dir. The directory
is per-session and never shared (only the owner session mounts it), so it
satisfies the same _purge_target_if_owned proof the _tmp entry already passes;
the ownership guard's refusals (sibling dirs, shared roots, out-of-jail, the
account root) are unchanged.

Tests:
- test_purge_session_directories_jail.py: _session_owned_dirs now lists all
  six per-session dirs (workspace, uploads, attachments, repos, _tmp, _cache)
  — closing a blind spot where the suite omitted BOTH mounts — and the
  legitimate-purge, per-candidate-reclaim, and shared-root-refusal cases now
  cover _tmp and _cache, with sibling-session bystanders proving scoping.
- test_delete_session_purges_cache_dir.py: drives the real delete_session
  service against a migrated Postgres testcontainer; asserts _cache/<id>
  (and _tmp/<id>) are gone the moment delete_session returns, with
  host_dir_reaper_enabled both true and false.

Co-Authored-By: Detail
@detail-app
detail-app Bot requested a review from eumemic September 8, 2026 15:31
@eumemic-bot

eumemic-bot Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

Code review

Verdict: pass — head 6f31bad5a19c94f7113187d0f43ca6d8aa57a5d8, tree confirmed clean at that SHA (git -C /mnt/review rev-parse HEAD matches).

What the change does

src/aios/sandbox/volumes.py:882 adds one candidate to purge_session_directories:

(session_cache_dir(session_id), (session_cache_dir(session_id),)),

The claim in the PR body is accurate and I verified each link in it against the tree:

  • session_cache_dir (volumes.py:566) is built by the same _per_session_child helper as session_tmp_dir, so the target is derived from session_id alone and is a single, non-dotted path component under <workspace_root>/_cache, with an explicit escape re-check. It is therefore exactly as provable as the _tmp entry already registered on the line above.
  • The only mounter of _cache/<id> is the session spec builder (spec.py:1128, inside the session_id.startswith("sess_") branch), so the dir is exclusively the owner session's — no sharing case exists for the ownership guard to have to refuse.
  • Before this change the only reclamation path was _reap_session_cache (host_dir_reaper.py:338), and sweep_host_dirs short-circuits on host_dir_reaper_enabled — so under the documented kill-switch _cache genuinely had zero cleanup, unlike every other per-session tree. The asymmetry with _tmp was real.
  • The new candidate goes through the identical _purge_target_if_owned proof and the same prove-all-then-delete phase ordering; the live_workspace_paths borrowed-workspace check is guarded by candidate == workspace_path and so is untouched by a new non-workspace candidate. No refusal is weakened.

Verification I ran (scoped to the changed behavior)

  • tests/unit/test_purge_session_directories_jail.py — 17 passed on head.
  • RED→GREEN confirmed at the unit layer: with the single added line reverted in the worktree, exactly 2 tests fail (test_purges_canonical_session_directories, test_refusing_one_candidate_still_reclaims_the_session_s_own_dirs); restored the file afterward (worktree clean). This is a genuine failing-on-base test, not a tautology.
  • tests/unit/test_host_dir_reaper.py + tests/unit/sandbox/test_tmp_mount_and_disk_gate.py — 53 passed; the reaper's _cache liveness/fail-closed/unknown-owner behavior is unaffected by the delete-time purge.
  • pytest tests/unit -k "purge or volumes or cache_dir or tmp_dir" — 91 passed.
  • mypy and ruff check/format --check on the three touched files — clean. (Repo-wide suites left to CI per instructions.)

The strengthened unit fixture is a real improvement, not padding: _session_owned_dirs previously omitted both bind-mount sources, so the legitimate-purge assertion could not have detected either omission. The added sibling-session bystanders (_tmp/sess_sibling, _cache/sess_sibling) are the right control for a new candidate — they would catch a future change that widened the target from _cache/<id> to _cache, and the shared-root refusal loop now includes _cache itself.

Not fully evaluated (declared, not treated as green)

tests/integration/test_delete_session_purges_cache_dir.py could not be executed here — no Docker daemon in this sandbox, so the Postgres testcontainer behind migrated_db_url is unavailable. What I could check statically, I did:

  • Both tests collect cleanly (--collect-only → 2 tests) and pass mypy, so this is not a broken-import or signature-drift file.
  • pytestmark = pytest.mark.integration matches the declared marker and the sibling files' convention; create_pool(migrated_db_url, min_size=1, max_size=4) + _reset_db_state + the raw INSERT INTO accounts (...) seed all match the established pattern (e.g. test_abandoned_client_tool_ghost_repair.py, test_workspace_reaper_shared_run_lock.py), including the monkeypatch.setattr(settings, "workspace_root", tmp_path) idiom used by test_workspace_reaper_shared_run_lock.py:108.
  • The seed's assumption is sound: insert_session defaults workspace_volume_path to _default_workspace_path = <workspace_root>/<account>/<session> (db/queries/sessions.py:243), which is what _seed materializes and what delete_session (services/sessions.py:2654) reads back, so the workspace candidate will be proven rather than skipped and the _tmp/_cache assertions exercise the real service call site rather than a mock.

CI's integration job runs this file, and the assertions it makes are the correct ones. I am flagging the non-execution for transparency; I am not treating it as evidence, and I am not blocking on it, because the substantive behavior change is a one-line candidate registration that is already independently proven RED→GREEN at the unit layer against the real purge_session_directories function.

Notes (non-blocking, no action required)

No blocking issues. The diff is minimal, symmetric with the existing _tmp entry, adds no new destructive authority beyond what the ownership guard already proves, and ships a test that fails without it.

@eumemic

eumemic commented Sep 8, 2026

Copy link
Copy Markdown
Owner

@cursor update to latest master

@cursor

cursor Bot commented Sep 8, 2026 •

Copy link
Copy Markdown

This branch is already on latest master. I fetched origin/master and compared it to the PR head.

Nothing new to merge or rebase. The only commits on this branch that are not on master are the cache-purge fix (6f31bad5) and the existing merge commit. CI on #2414 is still running; mergeable_state: blocked is from pending checks / review, not a stale base.

Open in Web Open in Cursor 

@eumemic-bot

eumemic-bot Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

Code review

Verdict: pass — head 82a8a4933d9f135792f6fabed230d6d6fa03a8d5, confirmed checked out (git -C /mnt/review rev-parse HEAD matches; PR head ref fetched first). The head is a merge of master (7271ae68, #2415) into the fix commit 6f31bad5; git diff 6f31bad5 HEAD over the three touched files is empty, i.e. the substantive PR content is unchanged from the previously reviewed tree and the merge brings only the unrelated workflows-sweep change.

What the change does

src/aios/sandbox/volumes.py:882 registers one additional candidate in purge_session_directories:

(session_cache_dir(session_id), (session_cache_dir(session_id),)),

Verified against the tree rather than taken from the PR body:

  • session_cache_dir (volumes.py:566) is built by the same _per_session_child helper (volumes.py:459) as session_tmp_dir — single non-dotted path component, explicit post-resolve escape re-check — so the new target is derived from session_id alone and is exactly as provable as the _tmp entry directly above it. It passes the identical _purge_target_if_owned guard with itself as its sole owned base.
  • _cache/<id> is mounted only by the session spec builder (spec.py:1128, inside the session_id.startswith(f"{SESSION}_") branch), so no run/browser/shared owner exists that the guard would have to refuse. The asymmetry with _tmp was real.
  • The live_workspace_paths borrowed-workspace check is gated on candidate == workspace_path, so a new non-workspace candidate cannot interact with it. Prove-all-then-delete phase ordering is untouched. No refusal is weakened.
  • Before this change the only reclamation path was _reap_session_cache (harness/host_dir_reaper.py:337), and sweep_host_dirs returns 0 outright when host_dir_reaper_enabled is false (host_dir_reaper.py:360) — so under the documented kill-switch _cache genuinely had no cleanup while every sibling tree did.

Verification run (scoped to the changed behavior)

  • tests/unit/test_purge_session_directories_jail.py — 17 passed on head.
  • RED→GREEN reconfirmed on this head: deleting only line 882 in the worktree makes exactly 2 tests fail (test_purges_canonical_session_directories, test_refusing_one_candidate_still_reclaims_the_session_s_own_dirs), 15 pass; restored, worktree clean (git status --porcelain empty). Not a tautological test.
  • tests/unit/test_host_dir_reaper.py + tests/unit/sandbox/test_tmp_mount_and_disk_gate.py — 53 passed; the reaper's _cache liveness / fail-closed / unknown-owner behavior is unaffected.
  • pytest tests/unit -k "purge_session or volumes or session_cache or delete_session" — 69 passed.
  • mypy + ruff check / ruff format --check on the three touched files — clean.
  • The integration file tests/integration/test_delete_session_purges_cache_dir.py could not be executed here (no Docker daemon → no Postgres testcontainer). Rather than leave that as UNKNOWN, I read CI for this exact SHA: the integration check-run (102170317899) completed success at 82a8a493, as did unit, lint, e2e (docker), e2e (non-docker), connectors, and migration-head. So the file did run green against a real DB on this head. Statically I also confirmed it collects (2 tests), type-checks, uses the established migrated_db_url / _reset_db_state / monkeypatch.setattr(settings, "workspace_root", tmp_path) idiom (matching test_workspace_reaper_shared_run_lock.py:108), and that its seed assumption is sound: insert_session defaults workspace_volume_path to _default_workspace_path = <workspace_root>/<account>/<session> (db/queries/sessions.py:243,293), which is what _seed materializes and what delete_session (services/sessions.py:2653) reads back — so the workspace candidate is proven rather than skipped and the _tmp/_cache assertions exercise the real service call site.

Test quality

The strengthened unit fixture is substantive, not padding. _session_owned_dirs previously omitted both bind-mount sources, so the legitimate-purge assertion was structurally incapable of detecting either omission. The added sibling bystanders (_tmp/sess_sibling, _cache/sess_sibling) are the correct control for a newly-authorized destructive target — they would catch a future widening from _cache/<id> to _cache — and the shared-root refusal loop now covers _tmp/_cache themselves.

Notes (non-blocking, no action required)

No blocking issues. Minimal diff, symmetric with the existing _tmp registration, adds no destructive authority the ownership guard does not already prove, and ships tests that are demonstrably red without the fix.

@eumemic-bot

eumemic-bot Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

Code review

Verdict: pass — head 226ce5604fcef6a9e61e58a70a67469587fe1ba0, confirmed checked out (PR head ref fetched first; git -C /mnt/review rev-parse HEAD matches, worktree clean).

This head is a second master merge (base now b3cdc118, #2365) on top of the previously reviewed 82a8a493. The substantive PR content is unchanged: git diff 82a8a493 HEAD -- src/aios/sandbox/volumes.py tests/unit/test_purge_session_directories_jail.py tests/integration/test_delete_session_purges_cache_dir.py is empty; the merge brings only unrelated master work (secret_egress_proxy, wake, _log_redaction, lanes/activate_script + their tests). git diff --stat origin/master...HEAD is exactly the three PR files (+211/−9). I confirmed no master commit between the old and new base touched volumes.py, services/sessions.py, or harness/host_dir_reaper.py, so there is no new interaction with the change under review. Per instructions I did not re-run the expensive checks the earlier eumemic-bot reviews reported on unchanged content; I did re-run the directly affected unit suite and re-confirm the RED→GREEN property on this tree.

What the change does

src/aios/sandbox/volumes.py:882 registers one additional candidate in purge_session_directories:

(session_cache_dir(session_id), (session_cache_dir(session_id),)),

Verified against this tree, not taken from the PR body:

  • session_cache_dir (volumes.py:566) is built by the same _per_session_child helper (volumes.py:459) as session_tmp_dir — session id constrained to one non-dotted path component, plus an explicit post-resolve escape re-check — so the new target is derived from session_id alone and is exactly as provable as the _tmp entry on the line above. It passes the identical _purge_target_if_owned guard with itself as its sole owned base.
  • The prove-all-then-delete phase ordering is untouched: the new candidate is appended to the same tuple consumed by the proving loop, and the destructive phase still runs only after every candidate is proven.
  • The live_workspace_paths borrowed-workspace refusal is gated on candidate == workspace_path (volumes.py:894), so a new non-workspace candidate cannot weaken or bypass it.
  • Call site unchanged: services/sessions.py:2679 invokes the purge inside the same transaction block after queries.delete_session, so the new dir is removed on the real delete path rather than only by the periodic reaper.

Verification run on this head (scoped)

  • tests/unit/test_purge_session_directories_jail.py — 17 passed.
  • RED→GREEN re-confirmed on this exact tree: deleting only line 882 in the worktree makes exactly 2 tests fail (test_purges_canonical_session_directories, test_refusing_one_candidate_still_reclaims_the_session_s_own_dirs), 15 pass; restored, git status --porcelain empty. The test is not tautological.
  • tests/integration/test_delete_session_purges_cache_dir.py collects cleanly (2 tests).
  • Repo-wide lint/type/test suites left to CI as instructed.

Test quality

The strengthened unit fixture is substantive rather than padding: _session_owned_dirs previously omitted both bind-mount sources, so the legitimate-purge assertion was structurally incapable of detecting either omission. The added sibling bystanders (_tmp/sess_sibling, _cache/sess_sibling) are the correct control for a newly-authorized destructive target — they would catch a future widening from _cache/<id> to _cache — and the shared-root refusal loop now covers _tmp/_cache themselves.

Not executed here (declared, not treated as green)

tests/integration/test_delete_session_purges_cache_dir.py could not be run in this sandbox — no Docker daemon, hence no Postgres testcontainer behind migrated_db_url. The integration check-run for this SHA (102194976115) was still in_progress at review time, as were e2e (docker) and e2e (non-docker); unit, lint, connectors, detect, and migration-head had already concluded success. The byte-identical file did complete success in CI on 82a8a493, and the merge changes nothing it touches.

I am recording this as UNKNOWN rather than green. It is not blocking because the consequential behavior change is the single candidate registration, which is independently proven RED→GREEN against the real purge_session_directories at the unit layer on this head; the integration file adds call-site coverage on top of an already-proven change rather than being the sole evidence for it. If the still-running integration/e2e runs fail on this SHA, that is a new fact and this verdict should be revisited.

Notes (non-blocking, no action required)

No blocking issues. Minimal diff, symmetric with the existing _tmp registration, adds no destructive authority the ownership guard does not already prove, and ships tests that are demonstrably red without the fix.

@eumemic
eumemic enabled auto-merge (squash) September 8, 2026 19:14
@eumemic-bot

eumemic-bot Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

Code review

Verdict: pass — head b9723edd5e451539bcde90a95f83ae7601e8dc98, confirmed checked out in /mnt/review (PR head ref fetched first; git rev-parse HEAD matches; worktree clean after all experiments).

This head is a third master merge (base now 34ed30d3, #2377) on top of the previously reviewed 226ce560. The substantive PR content is unchanged: git diff 226ce560 HEAD -- src/aios/sandbox/volumes.py tests/unit/test_purge_session_directories_jail.py tests/integration/test_delete_session_purges_cache_dir.py is empty (0 lines), and git diff --stat origin/master...HEAD is exactly the three PR files (+211/−9). Per instructions I did not re-run the expensive checks the earlier eumemic-bot reviews reported on unchanged content; I did re-verify the affected unit suite and re-establish the RED→GREEN property on this tree, and I specifically examined the newly merged master work for interaction with the change under review.

The merge: what it brought in, and why it does not touch this change

git log 226ce560..HEAD adds b96d03b2 (#2367, dead snapshot-throughput calibration removal) and 34ed30d3 (#2377, clone_session workspace-path realpath normalization). #2377 is the only one that touches a file on this PR's call path (src/aios/services/sessions.py), so I read it rather than assuming:

  • Its change is confined to clone_session (sessions.py:2612-2623): it applies queries.normalized_workspace_path to an explicit workspace_path before storing, so the persisted column matches the workspace reaper's os.path.realpath SQL probe. delete_session (sessions.py:2686-2693) and its purge_session_directories call are untouched.
  • No interaction with this PR's guarantee: purge_session_directories does not trust the stored form — _purge_target_if_owned calls .resolve() on both the candidate and each owned base before comparing, so a literal-vs-realpath discrepancy in the workspace row was already handled on the purge path, and the new _cache candidate is not derived from the stored column at all (it comes from session_cache_dir(session_id)).
  • No master commit in the merge touches sandbox/volumes.py, harness/host_dir_reaper.py, or sandbox/spec.py.

What the change does

src/aios/sandbox/volumes.py:882 registers one additional candidate in purge_session_directories:

(session_cache_dir(session_id), (session_cache_dir(session_id),)),

Re-verified against this tree:

  • session_cache_dir (volumes.py:566) is built by the same _per_session_child helper (volumes.py:459) as session_tmp_dir — the id is constrained to a single non-dotted path component with an explicit post-resolve escape re-check — so the target is derived from session_id alone and is exactly as provable as the _tmp entry on the line above. It passes the identical _purge_target_if_owned guard with itself as its sole owned base, so it can never resolve to _cache itself or to a sibling's dir.
  • Prove-all-then-delete phase ordering is preserved: the candidate is appended to the same tuple the proving loop consumes, and the destructive loop still runs only after every candidate is proven.
  • The live_workspace_paths borrowed-workspace refusal is gated on candidate == workspace_path (volumes.py:894), so a new non-workspace candidate cannot weaken or bypass it.
  • The asymmetry being fixed is real: _cache/<id>'s only prior reclamation path was the periodic host_dir_reaper, which short-circuits entirely under host_dir_reaper_enabled=false, while _tmp/<id> was already purged synchronously at delete time.

Verification run on this head (scoped)

  • tests/unit/test_purge_session_directories_jail.py — 17 passed.
  • RED→GREEN re-confirmed on this exact tree: deleting only line 882 in the worktree makes exactly 2 tests fail (TestPermitsLegitimatePurge::test_purges_canonical_session_directories, TestRefusesUnownedTargets::test_refusing_one_candidate_still_reclaims_the_session_s_own_dirs) and 15 pass; restoring the line returns 17/17 and git status --porcelain is empty. The added assertions are not tautological.
  • tests/integration/test_delete_session_purges_cache_dir.py collects cleanly (2 tests).
  • Repo-wide lint/type/test suites left to CI as instructed.

Test quality

The strengthened unit fixture is substantive, not padding: _session_owned_dirs previously omitted both bind-mount sources, so the legitimate-purge assertion was structurally incapable of detecting either omission. The added sibling bystanders (_tmp/sess_sibling, _cache/sess_sibling) are the right control for a newly-authorized destructive target — they would catch a future widening from _cache/<id> to _cache — and the shared-root refusal loop now covers _tmp/_cache themselves. The integration test drives the real delete_session service against a migrated Postgres and asserts the dir is gone the moment it returns, with the reaper kill-switch both on and off, which is the correct statement of the property (delete-time reclamation, not eventual reclamation).

Not executed locally (declared, resolved via CI on this SHA)

tests/integration/test_delete_session_purges_cache_dir.py could not be run in this sandbox (no Docker daemon → no Postgres testcontainer). Rather than leave it UNKNOWN I read the check-runs for this exact SHA: integration completed success (check-run 102201429309), as did unit, lint, e2e (non-docker), connectors, detect, and migration-head. So the file did run green against a real DB on this head. e2e (docker) was still in_progress at review time — it exercises no code this PR changes, but if it fails on this SHA that is a new fact and this verdict should be revisited.

Notes (non-blocking, no action required)

No blocking issues. Minimal diff, symmetric with the existing _tmp registration, adds no destructive authority the ownership guard does not already prove, and ships tests that are demonstrably red without the fix. The merge brings no change that interacts with it.


Structured result (for the launcher):

{"verdict": "pass", "issues": [], "artifact_posted": true}

@eumemic
eumemic merged commit c242b06 into master Sep 8, 2026
9 checks passed
@eumemic
eumemic deleted the detail/bug-fix/fix-sandbox-purge-per-session-cache-id-at-delete-t-4ca1fd branch September 8, 2026 19:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant