fix(harness): tmpfs cleanup removes read-only artifact trees - #4018
Merged
Conversation
Seeded-archive artifacts chmod their directories non-writable; when a test builds a cache under the tmpfs basetemp (query_cardinality_archive does), shutil.rmtree(ignore_errors=True) silently leaves the tree behind, cleanup_managed_tmpfs_path returns False, and devtools verify withholds release_baseline_allowed on an otherwise-green receipt — this blocked two merges on 2026-08-19 and became systematic once #4006 unblocked the query-cardinality fixture. rmtree now carries an onexc handler that restores owner-write on the parent+target and retries. Red twin: a planted read-only artifact subtree fails cleanup on the old code, removed by new. Ref polylogue-b9yw7 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HWcPJJJvuF25CqVwTFgSQC
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Sinity
added a commit
that referenced
this pull request
Aug 19, 2026
…#4012) ## Summary The warm `devtools verify` gate spends most of its wall clock in one place: the `load_sensitive` lane, 80.71s of a 110.67s run. This bounds that lane's concurrency instead of pinning it to a single process, and fixes a startup race in the two tests that made the lane flaky whether or not it ran concurrently. On a quiet host the lane goes from ~72s to ~35s; under fleet load, measured head-to-head, from 91-131s to 39-41s. ## Problem Receipt `20260818T184401Z-full-1494889-23438ba4` puts the lane at 80.71s against 8.76s for the parallel lane and ~22s for every static gate combined. All seven members are in `tests/integration/test_daemon_resilience.py`, each driving a real `polylogued` subprocess with its own archive root (`workspace_env`) and its own loopback port — they share no global resource. `load_sensitive` records a real observation: the *parallel* lane's full worker count starves daemon startup and flakes these tests. It does not follow that the members contend with each other, and the lane was reading that marker as "run one at a time" rather than "do not run at the parallel lane's width". While measuring, the more interesting problem surfaced. Repeating the two SIGTERM tests **strictly serially** under host load produced a failure in 1 of 3 runs (102.4s, `subprocess.TimeoutExpired` after 90s), and again in a 5-run capture. The lane is flaky on master today, and lane concurrency is not what causes it: `_wait_for_lifecycle_start` returns as soon as `DaemonLifecycle.start` persists its row. In `polylogue/daemon/cli.py` that happens early — `install_signal_handlers` runs immediately after it, and the startup lifecycle event, source-root creation and the maintenance loops all run later and all write the ops tier. Acting on the lifecycle row alone races the rest of startup, and the race widens exactly when the host is busy. `test_sigterm_with_locked_ops_exits_without_normal_sqlite_wait` then takes `BEGIN EXCLUSIVE` on `ops.db` inside that window, parking a daemon startup write inside a blocking `sqlite3_step`, where CPython cannot run the Python SIGTERM handler until the busy timeout expires — past the test's 90s bound. ## Solution **Bounded lane** (`devtools/verify.py`). The `load_sensitive` lane now runs `--dist=loadgroup -n min(adaptive, SERIAL_LANE_MAX_WORKERS)` instead of `-n 0`. The cap is 4, with the measurement that produced it recorded at the constant. `_native_pytest_steps` takes `serial_worker_args`; the closed-world reconstruction in `_native_pytest_command_is_closed_world` passes the observed worker request for both lanes, which is exact for the lane under test and irrelevant for its sibling. Nothing here is collection-affecting, so `devtools/pytest_collection_contract.py` and the testmon environment digest are untouched — no graph invalidation. **Bin packing** (`tests/integration/test_daemon_resilience.py`). Under dynamic scheduling the longest member (`test_large_session_file`, ~23s of real 50K-message ingest) is declared sixth of seven, so it starts last and the makespan became "when the longest test happened to begin" — 35.1s against a 23.2s floor. Four `xdist_group` bins pack the members longest-first so the makespan is bounded by the largest bin. The bins are a scheduling hint, not a correctness contract: the tests are independent, so a wrong or missing group costs wall clock, never a false result. **Readiness gate** (same file). `_wait_for_api_ready` polls the daemon's API port, which only answers after the startup sequence completes, and both SIGTERM tests now wait on it before signalling or locking. This removes an unintended precondition rather than relaxing an assertion — every existing assert, including the `< 25.0` bound and the thread-dump log checks, is unchanged. **Lane interpreter pin — found here, shipped in #4017, dropped from this PR.** Provisioning this lane produced a venv on uv's CPython 3.14.5 while the checkout runs the flake's free-threaded 3.14.4. That breaks a lane two ways: the interpreter is a testmon digest input, so the lane can never match the graph `lane-init` just seeded for it and every run bootstraps while printing "lane verifies start warm"; and `import hypothesis` fails outright (`sysconfig` raising `AttributeError: installed_base`), so the lane cannot collect the corpus at all. This PR's first measurement attempt died on it. lane-a1 found the same root cause independently and fixed it more completely in #4017, so my commits are dropped rather than duplicated. Theirs resolves the interpreter via `sys._base_executable` (the build a venv was created *from*) where mine parsed `pyvenv.cfg`'s `home` and guessed among likely binary names — mine breaks on a relocated venv. Theirs also scrubs `_PYTHON_SYSCONFIGDATA_NAME` / `_PYTHON_HOST_PLATFORM` / `PYTHONPYCACHEPREFIX`, which is the actual mechanism behind the `installed_base` failure I hit and worked around without diagnosing. No delta from mine was worth keeping. **Loopback-socket probe routed into the lane** (`tests/unit/daemon/test_web_reader.py`). `test_socket_peer_disconnected_detects_closed_loopback_peer` asserts that `_socket_peer_disconnected` observes the peer's FIN within the call, with no retry or grace window — the case pyproject's marker text names — and was the one function in that file the marker did not cover. It passed in 11 retained receipts, then failed once (`assert False is True`) in a 1990-test batch under load on 2026-08-19 and passed standalone at 0.68s immediately after. Evidence and recommendation: `.agent/scratch/gate-only-flakes-evidence-2026-08-19.md`. **An incomplete tmpfs cleanup now explains itself** (`devtools/pytest_supervisor.py`, `devtools/verify_runs.py`) — see the b9yw7 section below. Rejected: reducing `test_large_session_file`'s 50K-message scale. Its assertions are about behaviour *at* that scale (2 GB RSS bound, FTS population), so shrinking it would weaken what it asserts. Its wall time is genuine ingest — the JSONL writer accounts for 0.28s of it. ## Verification All commands run in `/realm/worktrees/a4-serial-lane` on the lane's own venv. **A correction to the baseline's framing.** "7 `load_sensitive` tests / 80.7s" was testmon's *selection* in that receipt, not the lane's membership. Full membership was already 13 — the seven daemon-resilience tests plus a six-test `load_sensitive` class in `tests/unit/daemon/test_web_reader.py` — and this PR makes it 14 (see the marker addition below). Both figures below are the full 14-member lane. Head-to-head, same host, load average 8-21 (concurrent fleet lanes), `devtools test ... -m load_sensitive`: | lane shape | runs | result | | --- | --- | --- | | `-n 0` (master's shape) | 6 | 98.00s, 99.01s, 105.03s, 106.88s, 120.13s, and one **103.46s with 1 failed** | | `-n 4 --dist=loadgroup` (this PR) | 4 | 14 passed in **34.87s, 39.62s, 40.72s, 50.84s** | On the seven daemon-resilience members alone, the same comparison was 91.18s / 131.10s serial against 39.00s, 40.42s, 41.46s, 41.41s bounded; on a quieter host earlier in the session, 71.95s serial against 35.08s at four workers. The concurrency cliff that sets the cap, same corpus and containment: ``` -n 0 71.95s green -n 4 35.08s green -n 5 107.76s 1 failed (SIGTERM deadline starved) -n 7 95.20s 2 failed (both SIGTERM tests, 90s subprocess timeout blown) ``` The readiness gate, five consecutive repeats of the SIGTERM pair under load — `12.43s, 12.14s, 12.24s, 12.64s, 12.61s`, all green. Before the fix the same command gave `9.36s / 102.40s (1 failed) / 10.65s`, so the variance collapse is the evidence, not just the greens. Full gate, `devtools verify` on this branch: ``` "terminal_green": true, "complete_corpus_covered": true, "non_green_count": 0, "attested_unchanged_count": 20507 ``` Receipts: **before** `20260818T184401Z-full-1494889-23438ba4` (serial lane 80.71s, 7 tests, workers=9 available); **after** `20260819T005619Z-full-3842391-dfb668e3` (terminal green, wall 81.68s) and `20260819T004849Z-full-3759625-54e8f8d8`. Two honest caveats about the receipt pair. The lane selected 0 tests in the green "after" run — testmon had already attested those seven from the immediately preceding execution — so its 5.41s lane step is not a like-for-like measurement; the `devtools test` table above is. And in `20260819T004849Z` the lane took 114.70s at **workers=1**: under load average 22 `adaptive_pytest_worker_count` collapsed to a single worker, so that receipt measures the fallback path, not this change. That collapse is the intended safety property — the cap only raises a ceiling — but it does mean the win lands on a quiet host, which is where the 80.71s baseline was measured. Re-verified after rebasing onto `a9bf50133` (21 PRs merged in between): `devtools verify --quick` success, `mypy` clean across 2607 files, and a focused run over every touched module **432 passed in 149.60s**. Merge-gate receipt at head `27dcee229`: **exit=0, 151.55s**. Anti-vacuity re-checked *after* the rebase rather than trusting the pre-rebase result: disabling `_restore_owner_write` turns all three read-only tests red together (`..._removes_read_only_artifact_trees`, `..._removes_a_read_only_seeded_cache`, `..._sweep_reclaims_a_dead_runs_read_only_tree...`) and green again on restore, so the sweep genuinely depends on #4018's repair rather than carrying its own copy. **No further full `devtools verify` was run on this branch, deliberately.** `devtools/pytest_supervisor.py` is a testmon environment-digest input (`devtools/pytest*.py`, `testmon_bootstrap.py:327`), so the b9yw7 fix changes the digest: this tree computes `polylogue-5fe2342e...` against four recorded graphs that do not include it, i.e. `absent` → a full bootstrap. Per tonight's gate policy that run belongs on the main checkout, scheduled centrally. **Consequence worth planning for: once this merges, every checkout's digest changes and the fleet pays one bootstrap** — worth batching with other digest-touching work. Not addressed: the ≤30s target was not reached. `test_large_session_file` alone is ~23s of real ingest on a quiet host and sets the floor; getting under it means changing what that test covers. ## Repaired in passing while rebasing (both pre-existing on master, neither mine) - **`test_quick_verify_omits_pytest` was failing on master.** #4026 added the `lab policy oracle-integrity` step to `build_verify_steps` without updating the test that pins the quick lane's label list, so master emitted 11 labels against an assertion expecting 10. Verified pre-existing: my branch's diff touches neither the builder region nor that list. One line added to the expected list. - **Two `oracle-integrity` baseline entries reanchored.** That gate keys findings by line number, and this branch's additions shifted two pre-existing entries (`test_daemon_resilience.py` 136→165, `test_web_reader.py` 1456→1464). Hand-edited those two rather than regenerating the baseline, so nothing unrelated could be silently blessed — entry count is 29 before and after, and the diff is exactly two line numbers. ## Findings for the coordinator (bd is read-only here) - **polylogue-b9yw7 (`cleanup.complete=false`): ROOT-CAUSED AND FIXED.** `tests/infra/workload_artifacts._make_read_only` strips write bits from the seeded-archive tree it publishes — from the **directories** as well as the files — so the cache is immutable once built. A directory without its write bit cannot have entries unlinked from it, so any test that builds such a cache under the run's basetemp (`query_cardinality_archive` puts one at `work/"seeded-cache"`) left a tree `shutil.rmtree` simply could not remove. `cleanup_managed_tmpfs_path` returned False, and because `_release_baseline_allowed` requires `cleanup.complete is True`, a green complete-corpus run lost release authority. Confirmed live 2026-08-19: 16 leaked `/dev/shm/pytest-polylogue-*` trees, `rm` refusing with Permission denied on `.../seeded-cache/artifacts/*/wire/*.jsonl`, `chmod -R u+w` clearing them, and the next receipt going 39.5s OK. Receipts: `20260819T003921Z` (wsb worktree) and `20260819T010648Z` (main), both `cleanup:false` on the s11 lane. It blocked two merges before diagnosis. **Correction to my earlier comment on this PR.** I previously proposed an unsynchronized-reclaimer race between `cleanup_managed_pytest_basetemp` and the supervisor's exit pass. That was wrong. The two reclaimers are real, but they are not why cleanup failed — a permission bit was, and the mechanism above reproduces deterministically where my race never did. Recording it because the wrong theory is on this PR already. Three changes, per the operator directive that tmpfs must clean up on its own, never leak, and clean ASAP: 1. **Permission-repairing removal — the repair itself is #4018's, already on master.** This PR keeps exactly one implementation of it (`_restore_owner_write`) and adds `force_rmtree` as the shared thin entry point calling it, because #4018 patched only the supervisor's exit pass while `verify_runs.cleanup_managed_pytest_basetemp` and the sweep have the same defect and the same need. `describe_managed_tmpfs_cleanup` routes through it too, so there is one removal sequence rather than two. My own duplicate implementation was dropped on rebase, including its `S_IXUSR` addition — `_make_read_only` strips only write bits, so #4018's `S_IWUSR`-only repair is correct and mine was over-broad. 2. **Eager, self-healing sweep.** Cleanup was purely trailing, so a tree that failed to unlink stayed until a human noticed. `sweep_stale_managed_basetemps` now runs at verify/test preflight, applying the same conservative ownership test as the exit path: reclaim only when the claim lock is free AND the recorded owner is positively dead; an unknown or live owner is left alone, and the shared `-seeded-` corpus cache is never a candidate. It is placed immediately before basetemp admission, so the space it returns is headroom the admission policy can then admit against. 3. **Red twin** (kept alongside #4018's, which covers a different surface — that one asserts the boolean `cleanup_managed_tmpfs_path` API, this one the `(complete, reason, residual)` tuple and the precondition that a plain rmtree raises), verified to actually go red: it plants a read-only `work/seeded-cache/artifacts/*/wire/*.jsonl` subtree in a managed path, asserts a plain `rmtree` raises, then asserts cleanup returns True and the tree is gone. With the fix reverted it fails `assert (False, ['wor...sions.jsonl']) == (True, [])` — and note the residual list names the exact live evidence path, so the diagnostic added earlier in this PR would have identified this in one receipt. A sweep test covers the three-way discrimination (dead owner reclaimed, live owner spared, shared cache spared). **Disposition: I believe this closes b9yw7**, with one honest residual — a basetemp with *no* claim file and no live owner is still not reclaimable, because "unclaimed" cannot be distinguished from "claimed a moment from now" without inventing an age threshold. Real managed runs always write a claim, so this affects hand-made trees rather than run debt. Your call whether that residual warrants keeping b9yw7 open. Found and fixed in passing: my own survivor test leaked its `/dev/shm` fixture every run (monkeypatch is still active inside the `finally`, so the cleanup `rmtree` was the patched no-op) — six leaked dirs, now `monkeypatch.undo()` first. Removed the strays by hand. - **Design question for the coordinator, not changed here**: `_release_baseline_allowed` requires `cleanup.complete is True`, so release-baseline authority on a complete, green corpus is gated on whether a disposable scratch directory unlinked. The quiescence signal that would justify withholding trust (`controller_group_alive`) is already checked separately, upstream of this flag. Loosening a merge-authority gate is your call, so I left it alone. - **Residual flake, unresolved.** `test_sigterm_read_only_daemon_records_forensics` fails roughly 1 in 12 runs with a *different* mode than the one this PR fixed: the daemon exits cleanly on SIGTERM (`128+SIGTERM`, lifecycle row correct) and its log carries the `received SIGTERM; dumping all thread stacks` header, but the dump body (`Current thread`) is absent. So the forensic dump the daemon promises is sometimes not produced. Bounded, not root-caused: 8/8 clean in a standalone subprocess probe *both* with and without this PR's readiness gate; 10/10 clean under bare pytest; ~1/12 only under the managed `devtools test` harness. **Falsified**: that `logger.error` inside the signal handler (`polylogue/daemon/lifecycle.py:247`) deadlocks on the logging lock — a minimal reproduction of exactly that shape deadlocked 0/12 times. I had read a single instrumented failure as confirming it; it did not, and I am flagging that rather than shipping the theory. Note `faulthandler.dump_traceback` there sits under `contextlib.suppress(Exception)`, so if it is raising, nothing records why. This is a pre-existing daemon forensics gap, not something this PR introduces — but the readiness gate changes which mode dominates, so it is now the visible one. I did not weaken the assertion, retry it, or skip the test. **Your call whether this blocks the merge**; the rest of the PR is independent of it. - **New, unfiled**: a daemon whose ops tier is locked by another connection *during startup* can be held well past its shutdown contract — over 90s against a documented "well under the normal 30s SQLite wait" — because the blocking SQLite wait is not signal-interruptible. This PR makes the test stop entering that window; it does not change the daemon. Worth its own bead against `polylogue/daemon/cli.py`, and worth deciding whether the contract is meant to hold during startup at all. - **8226a-C**: the serial lane's cost is now bounded rather than serialized; residual cost is the 23s ingest floor named above. <!-- polylogue-pr-scope:v2 { "assigned_beads": [], "dispositions": [], "mutated_beads": [], "scope_digest": "79a7984a9ec80a157c96dc8d28561159c642c15de3793837eff751fa7eac34a1", "scope_kind": "self_contained", "version": 2 } -->
Sinity
added a commit
that referenced
this pull request
Aug 19, 2026
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HWcPJJJvuF25CqVwTFgSQC
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
cleanup_managed_tmpfs_pathnow removes read-only artifact trees: plainrmtree first, then a chmod-owner-write walk + retry if anything survived.
Problem
Seeded-archive artifacts chmod their directories non-writable. When a test
builds a cache under the tmpfs basetemp (
query_cardinality_archivedoes),shutil.rmtree(ignore_errors=True)silently leaves the tree behind, thesupervisor reports
tmpfs_cleanup_complete: false, anddevtools verifywithholds
release_baseline_allowedon an otherwise-green receipt. Thisblocked the #4006 merge twice on 2026-08-19 (runs 20260819T003921Z,
20260819T010648Z; 16 leaked /dev/shm dirs confirmed) and became SYSTEMATIC
once #4006 unblocked the query-cardinality fixture — every full-mode receipt
now builds the read-only cache. Ref polylogue-b9yw7 (root cause recorded
there); the eager stale-dir sweep half of the operator's never-leak mandate
stays with the A4 harness lane.
Solution
Two-pass removal in
cleanup_managed_tmpfs_path(ownership checksunchanged): rmtree, then
_restore_owner_write(chmod u+w over root+rglob)and rmtree again only if residue survived. No behavior change for the
already-clean path.
Verification
devtools test tests/unit/devtools/test_pytest_supervisor.py -k cleanup→2 passed. Red twin demonstrated: with the fix stashed, the new
test_cleanup_managed_tmpfs_path_removes_read_only_artifact_treesfails(read-only subtree survives); with the fix, removed.
devtools verify --quick→ success (mypy clean after replacing the onexc handler with thetwo-pass shape — the pinned stubs reject
onexc).Bead disposition
Self-contained; Ref polylogue-b9yw7 (narrows it; closure judgment stays with
the A4 lane's fuller investigation).