feat(trusty-mpm): retire aged-out session records and color tm ls NUM/NAME - #4993
Conversation
…/NAME `tm ls` showed `NUM 107` at the bottom of a 31-row listing. The slot registry (#3034) numbers every record it observes and the default view then hides the terminal ones — 76 of those 107 slots were held by rows nobody could see. Nothing had ever left the store: the auto-prune path decommissions `record_only`, which keeps the record. Freeing numbers therefore needs the records evicted, not a smarter allocator; with the free set permanently empty, lowest-free allocation alone is a provable no-op. Retention: `SessionRecord::terminal_at` records when a record entered `Decommissioned`/`Deleted`, stamped at the single `enter_terminal_state` entry point. `SessionManager::sweep_terminal_records` evicts records past `TERMINAL_RECORD_RETENTION_DAYS` (7) and releases their slots; it runs on the daemon's existing GC tick, ahead of the tmux probe so a machine without tmux still sweeps. Two guards keep it safe: a record whose workspace directory still exists is never evicted (it is what keeps that path in `prune_orphaned_worktrees`'s protected set — dropping the record would collapse both of that sweep's independent reads at once), and a record with no `terminal_at` is dated rather than deleted, because the auto-prune decommissions long-idle records so inferring death from `created_at`/`last_activity_at` would evict the whole legacy backlog with no retention at all. Color: `NUM` renders magenta and `NAME` cyan — distinct hues, because a name's trailing serial comes from `allocate_serial` (per-project, reuses gaps) while `NUM` is a global slot, so any match is coincidence. The gate keys off stdout's TTY-ness, not the picker's stderr gate, so `tm ls | grep` stays clean; padding now measures visible width so ANSI escapes cannot stagger the table. The table renderer moved to `commands::managed_render` — `managed.rs` was 23 SLOC under the cap. Refs #3034 🤖🤖🤖 Generated with trusty-mpm — https://github.com/bobmatnyc/trusty-tools
Verdict: WARNAdversarial review at head Findings
HIGH —
|
… sweep Code-critic WARN on #4993 — 1 HIGH, 2 MEDIUM, 2 LOW. HIGH: `Path::exists()` maps every stat error to `false`, so a permission error on an ancestor, a stale NFS handle, or `EIO` from a departed volume read as "not there" — and "not there" evicts. The `workspace_path` that leaves with the record is what keeps `prune_orphaned_worktrees` from deleting the worktree, so failing open there could hand live work to an unattended sweep. The probe is now `try_exists().unwrap_or(true)` behind `workspace_present`, whose two non-Ok(false) answers both mean KEEP; the doc that claimed the check "can only evict fewer records" now describes what the code does. The probe is injected, so the error branch is tested deterministically rather than provoked with real permissions. Retention also now debounces like the two sibling destructive sweeps in the same orphan-GC tick: a record must be evictable on TWO consecutive sweeps. One observation was not enough because "the workspace is gone" is a live filesystem reading, and an external volume that unmounts between ticks answers Ok(false) truthfully — the error handling above cannot catch that. MEDIUM: `mark_reactivated` is the sole production Decommissioned -> Active transition and assigned `record.state` directly, so a revived record kept its `terminal_at` stamp and the invariant on the field was false. It routes through the setter — renamed `set_lifecycle_state`, since it now governs both directions — and a test drives the real reactivation path instead of the setter alone. MEDIUM: the delete ran against the phase-1 snapshot, which this sweep's own `upsert_many` and any other process's write have already invalidated. Each candidate is now re-read from the store and re-judged with a fresh probe immediately before deletion, following `prune_orphaned_worktrees`'s #1845 item-9 shape. LOW: the changelog fragment was named for #4988, a different open PR. LOW: the comment claimed the sweep's placement prevents silent disablement, but `TRUSTY_MPM_ORPHAN_GC=0` kills the whole loop. Refs #3034 🤖🤖🤖 Generated with trusty-mpm — https://github.com/bobmatnyc/trusty-tools
Critic round 1 — all five addressed (head
|
Verdict: APPROVERe-review at head Findings
1. HIGH — fail-open existence check: RESOLVED
path.is_some_and(|p| probe(p).unwrap_or(true))
The production call site does pass let on_disk = workspace_present(record.workspace_path.as_deref(), |p| p.try_exists());and it is the only path into The doc at what was line 150 now states what the code does ( On the injected probe: the generic parameter exists only for the test, but it is not a test-only path — production reaches the same three-line body through 2. Two-observation gate: sound, and the reasoning is rightThe premise checks out. I attacked the implementation on the four axes you would expect:
One behavior worth naming rather than flagging: if phase 1 errors out ( 3. MEDIUM 2 —
|
| Site | State assigned | Guard |
|---|---|---|
manager.rs:575 stop |
Stopped |
returns InvalidState on is_terminal() (manager.rs:560) |
manager.rs:745 mark_runtime_exited_stopped |
Stopped |
returns InvalidState unless state == Active (manager.rs:707) |
manager.rs:912 resume |
Active |
Stopped | Errored only (manager.rs:829-839) |
manager.rs:1112 mark_errored |
Errored |
none |
manager.rs:1133 set_workspace |
new_state |
none; all 5 production callers pass Active — launch_on_main.rs:176, lifecycle.rs:726, :1030, :1229, :1310 |
reconcile.rs:104 / :109 |
Active / Stopped |
continues on is_terminal() at reconcile.rs:81-99 — confirmed |
adopt.rs:344 |
Stopped |
selector matches Active | Provisioning only (adopt.rs:336-342) |
Every other hit is a test file (agent_reset_workspace.rs:530, managed_routes/reactivate.rs:461, mcp_proxy.rs:167, runtime_reap.rs:451, task_inject.rs:537/:686, resume_workdir.rs:342-422) or a different type entirely (control/actor.rs's SessionState, worktree_reconcile.rs's ReconcileState, core/circuit.rs).
The load-bearing half of the invariant is true: no production site assigns a terminal state outside set_lifecycle_state. That is the half that matters, because a direct terminal assignment is what would leave a stale terminal_at on a tombstone and make it immediately evictable.
The other half is not. mark_errored (manager.rs:1106-1114) and set_workspace (manager.rs:1125-1135) both take a record from get() with no terminality guard and assign a live state directly. If either were ever handed a tombstone, the record would leave the terminal state still carrying its stamp — the exact shape you just fixed in mark_reactivated. In practice neither is reachable that way: mark_errored runs on provision/spawn failure and set_workspace on spawn, both on records that have never been terminal. And it is self-healing even if it happened — a live record is Keeped unconditionally (retention.rs:112), and a later re-terminalization goes through set_lifecycle_state, whose !was_terminal branch restamps fresh (record.rs:462-464).
So: LOW, and the fix is the doc, not the code. record.rs:420-424 should claim what is enforced — every transition INTO a terminal state — rather than a symmetric invariant two unguarded call sites could violate.
mark_reactivated_clears_the_terminal_stamp is the right test: it drives the real path and asserts on the persisted record, not just the in-memory return.
4. MEDIUM 3 — re-validation before delete
The re-read is genuine. SessionManager::get (manager.rs:325-336) takes the store write lock, calls reload_if_changed, then cached_get. reload_if_changed (store.rs:196-210) re-stats and re-reads whenever the (mtime, len) fingerprint differs, treating a None on either side as changed. Same-process writes are already reflected in self.data; cross-process writes trip the fingerprint. Not a cached snapshot.
The window is as small as claimed — the phase-3 loop, then remove_many at retention.rs:301. A record confirmed at the top of the loop and deleted at the bottom is separated by at most the remaining get() calls plus one lock acquisition.
The cwd claim holds. prune_orphaned_worktrees builds its protected set from workspace_path exclusively — prune.rs:883 (let Some(p) = r.workspace_path else) and prune.rs:1096 (.filter_map(|r| r.workspace_path)). cwd appears nowhere in prune.rs. So evicting a record with workspace_path: None removes nothing from that set, and the asymmetry with mark_reactivated's cwd fallback is not a hole. Widening retention to cwd would, as you say, make almost every record permanently un-evictable. Withdrawn.
New MEDIUM — the phase-3 test does not reach phase 3
sweep_revalidates_before_deleting_a_reactivated_record is named in the Test: pointer at retention.rs:237 as covering phase 3. It does not reach it.
mark_reactivated sets the record to Active. On the second sweep, phase 1's retention_verdict short-circuits at retention.rs:112 (!record.state.is_terminal()), so the record is never pushed to candidates, confirm returns empty, and the phase-3 loop body never executes. The record is saved by phase 1 plus the debounce — the same two mechanisms debounce_requires_two_consecutive_observations already covers.
Proven rather than argued. I put eprintln!("CRITIC_PHASE3_REACHED id={id}") at the top of the phase-3 loop in a throwaway worktree and ran the retention module:
test session_manager::retention::tests::debounce_requires_two_consecutive_observations ... CRITIC_PHASE3_REACHED id=b549a47a-…
test session_manager::retention::tests::sweep_evicts_only_records_past_the_window ... CRITIC_PHASE3_REACHED id=6a10120b-…
CRITIC_PHASE3_REACHED id=2a91ffd8-…
test session_manager::retention::tests::sweep_releases_the_evicted_slot ... CRITIC_PHASE3_REACHED id=0df7c920-…
test session_manager::retention::tests::sweep_stamps_legacy_records_instead_of_evicting_them ... CRITIC_PHASE3_REACHED id=ee342254-…
test result: ok. 15 passed; 0 failed; 0 ignored; 0 measured; 4664 filtered out
Four tests reach it; sweep_revalidates_… is not among them. (Instrumentation reverted; git status --porcelain clean.)
So phase 3's Evict arm is covered incidentally by every eviction test, but its two protective arms — Ok(_) => warn!(…leaving it in place) at retention.rs:291 and Err(_) at :296 — are unexercised.
This is structural, not an oversight in the test: because phase 1 re-derives candidates from a fresh snapshot on every sweep, anything that changes between sweeps is caught at phase 1 and never reaches phase 3. Phase 3 only fires for a write landing within one sweep's execution, which has no seam to drive from a test.
Not a defect in the code — phase 3 reads correctly and is the right belt-and-braces behind the debounce. The defect is the pointer claiming coverage the test does not provide, which is the same shape as the enter_terminal_state test you just fixed: a documented guarantee with no caller behind it. Either correct the pointer to say phase 3's protective branches are uncovered and why, or add the seam. The doc fix is the cheaper and, given this repo's ban on test-only production methods, probably the right one.
5. LOW 4/5 — both confirmed
crates/trusty-mpm/changelog.d/4993-tm-ls-retention-and-color.md— renamed, oneChangedcategory, sits directly inchangelog.d/.daemon/mod.rs:598-604— now states the placement keeps a store-only sweep off tmux discoverability, and thatTRUSTY_MPM_ORPHAN_GC=0disables the loop, retention included. Matchesdaemon/mod.rs:133-137.
6. Did the fixes break anything?
The relocation is behavior-preserving. sweep_default_retention (retention.rs:328-335) computes Duration::days(TERMINAL_RECORD_RETENTION_DAYS) and Utc::now() — the same two values the daemon inlined before, in the same order, passed to the same function. The daemon's match arms are unchanged apart from renaming outcome to o.
No coverage removed, ignored, or gated. git diff origin/main...067a362c -- 'crates/trusty-mpm/**' removes five fns, all of them the managed_render.rs moves already cleared in round 1 (render_session_table, truncate, short_timestamp, format_tombstone_row, format_state_column). Zero added #[ignore], zero added #[cfg(not…)].
Gates at 067a362c
cargo fmt --check && cargo clippy -p trusty-mpm --all-targets -- -D warnings && cargo test -p trusty-mpm --no-fail-fast
EXIT=0
test result: ok. 4672 passed; 0 failed; 7 ignored; 0 measured; 0 filtered out; finished in 49.56s
test result: ok. 1485 passed; 0 failed; 1 ignored; 0 measured; 0 filtered out; finished in 4.22s
(+ 43 further binaries, all ok)
The #4931 baseline flake (ensure_managed_config_dir_emits_the_frozen_skill_warning) did not fire on this run.
Every test the fix round added or renamed passed:
test session_manager::retention::tests::workspace_present_treats_an_undetermined_path_as_present ... ok
test session_manager::retention::tests::debounce_requires_two_consecutive_observations ... ok
test session_manager::retention::tests::debounce_disarms_a_candidate_that_lapses ... ok
test session_manager::retention::tests::sweep_revalidates_before_deleting_a_reactivated_record ... ok
test session_manager::record::tests::set_lifecycle_state_stamps_once ... ok
test session_manager::record::tests::set_lifecycle_state_clears_stamp_on_revival ... ok
test session_manager::reactivate_tests::mark_reactivated_clears_the_terminal_stamp ... ok
bash scripts/check_line_cap.sh
LINECAP_EXIT=0
line-cap: measured 3728 tracked .rs file(s) (floor 500); 7 allowlisted, 0 violations — OK.
Notes
Neither finding blocks. The HIGH is closed, the two-observation gate is correct and correctly reasoned, the re-read is real, and the cwd asymmetry is justified by prune.rs's protected-set construction. Both remaining findings are documentation claiming more than the code enforces — worth a two-line correction, not another round.
🤖🤖🤖 Generated with trusty-mpm — https://github.com/bobmatnyc/trusty-tools
…verclaiming it Code-critic round 2, MEDIUM: `sweep_revalidates_before_deleting_a_reactivated_record` did not exercise phase 3. Confirmed with an `eprintln!` marker in the phase-3 loop — four sibling tests printed, that one did not. Structural, not sloppiness: phase 1 re-derives candidates from scratch every sweep, so a record reactivated BETWEEN sweeps is classified `Keep` there and never becomes a candidate. The debounce plus phase 1 is what saved that record; the re-validation arms sat unexercised on an irreversible-deletion path. Phase 3 is now `revalidate_for_eviction`, a named method the tests can reach. Taking the candidate list as a plain `Vec<Id>` is not a test hook — a list computed from an older view of the store IS the stale snapshot the guard exists for, and passing one directly is the only way to observe the two non-evicting arms fire. Three tests: a still-evictable candidate survives re-validation (so the guard cannot degrade into "never delete"), a record reactivated after the snapshot is dropped, and one a concurrent prune already removed is dropped rather than counted as evicted. Mutation-checked: stubbing the guard to return its input turns both "drops" tests red and leaves the first green. The misnamed test is renamed `sweep_spares_a_record_reactivated_between_sweeps` and its doc now names the guard that actually protects it. LOW: the `terminal_at` invariant was stated symmetrically but enforced one way. `mark_errored` and `set_workspace` assign a live state with no terminality guard. The doc now claims only the entering half, names the two exceptions, and says why a stale stamp on a live record is inert. Refs #3034 🤖🤖🤖 Generated with trusty-mpm — https://github.com/bobmatnyc/trusty-tools
Round 2 — both closed (head
|
| test | arm |
|---|---|
revalidate_keeps_a_still_evictable_record |
the passing arm — stops the guard degrading into "never delete" |
revalidate_drops_a_record_reactivated_after_the_snapshot |
Ok(_), via the real mark_reactivated |
revalidate_drops_a_record_a_concurrent_prune_already_removed |
Err(_) |
Mutation-checked rather than assumed. Stubbing the guard to return its input:
test revalidate_keeps_a_still_evictable_record ... ok
test revalidate_drops_a_record_a_concurrent_prune_already_removed ... FAILED
test revalidate_drops_a_record_reactivated_after_the_snapshot ... FAILED
The misnamed test is now sweep_spares_a_record_reactivated_between_sweeps, and its doc names the guard that actually protects it — including the marker experiment, so the next reader does not have to redo it.
On the pattern — three tests tonight that looked like coverage
They share one shape: assert an outcome that more than one mechanism produces, without pinning which one produced it. sweep_spares_… asserted "record survives" — true under the debounce, phase 1, and phase 3, so it passed while testing none of them specifically. enter_terminal_state_clears_stamp_on_revival asserted "stamp cleared" against a setter no caller invoked. reinstall_preserves_a_customized_agent_without_force asserted "file preserved" from a hand-seeded state.
The check that catches all three, and the one I skipped: break the thing the test names and confirm it goes red. That is cheap — one stub, one run — and it is what turned the three new tests above from plausible into proven. I am treating it as required for any test whose subject is a guard on a destructive path, not optional diligence.
LOW — invariant stated symmetrically, enforced one-way
Correct: mark_errored (manager.rs:1106) and set_workspace (manager.rs:1125) assign a live state with no terminality guard. No enforcement added. The doc now claims only the entering half, names both exceptions explicitly, and states why a stale stamp on a live record is inert — retention_verdict short-circuits on !is_terminal(), and the next terminal transition restamps rather than trusting the old value.
Gate — rung 5, at ad0d1063
cargo fmt --check && cargo check -p trusty-mpm && cargo clippy -p trusty-mpm --all-targets -- -D warnings
EXIT=0
cargo test -p trusty-mpm --no-fail-fast
test result: FAILED. 4674 passed; 1 failed; 7 ignored; 0 measured; 0 filtered out; finished in 50.02s
test result: ok. 1485 passed; 0 failed; 1 ignored; 0 measured; 0 filtered out
test result: ok. 1485 passed; 0 failed; 1 ignored; 0 measured; 0 filtered out
(+ 42 further binaries, all ok)
failure: daemon::managed_routes::tests::stale_assets_for_many_reads_shared_agent_dir_once_for_the_whole_fleet (baseline row 38)
cargo test -p trusty-mpm --lib -- --test-threads=1 daemon::managed_routes::tests::stale_assets_for_many
test result: ok. 4 passed; 0 failed; 0 ignored; 0 measured; 4678 filtered out
EXIT=0
An earlier --include-ignored --lib run in this round also hit ensure_managed_config_dir_emits_the_frozen_skill_warning (#4931) and bench_stale_assets_for_many; both pass under --test-threads=1 alongside the above. No coverage deleted, ignored, cfg-gated, or narrowed — the change is net +3 tests.
Nine lint gates, all EXIT=0:
| script | result |
|---|---|
check_line_cap.sh |
3728 files, 0 violations |
check_sld.sh |
0 errors, 0 warnings |
check_doc_numbers.sh |
0 violations |
check_test_pointers.sh |
21958 citations, 0 dangling |
check_capabilities.sh |
up to date |
check_changelog_fragment.sh |
fragment present and valid |
check_generation_artifacts.sh |
0 leaked artifacts |
check_agent_assets.sh |
all match |
check_buildrs_sync.sh |
in sync |
check_test_pointers did fail once on the way: renaming the test left a dangling Test: pointer in sweep_terminal_records's doc. Fixed, and it now points at both the renamed test and the revalidate_* set with a note on which guard each covers.
🤖🤖🤖 Generated with trusty-mpm — https://github.com/bobmatnyc/trusty-tools
Stamping `terminal_at = now` on records that predate the field grandfathered the entire pre-existing backlog for a further seven days — 76 of 116 records on the reporting machine, holding 76 of the slot numbers that put NUM in three digits. The owner would install the fix and still see NUM 116. `inferred_terminal_at` dates such a record from its latest evidence of life: `last_activity_at` when set, else `created_at`, never earlier than `created_at`. A timestamp in the future is not evidence — clock skew or corruption — and falls back to `now`, so such a record gets a full window rather than being deleted on the strength of a value that cannot be true. `RetentionVerdict::Stamp` now carries the inferred time so the one call site that acts on it cannot silently substitute its own. The earlier reasoning against inference was right about the mechanism and wrong about the goal: the auto-prune does decommission long-idle records, so this can date a record earlier than it truly died — but only for records that predate the field, and only toward clearing a backlog already weeks old. Ordering is unchanged: the inferred date is written to disk first and only a LATER sweep can evict, so the value is auditable before anything acts on it and the record still passes the debounce and phase-3 re-validation. Every safety property is untouched. `retention_verdict` still returns Keep on `!is_terminal() || workspace_on_disk` BEFORE reading `terminal_at`, so the workspace guard runs ahead of the backfill; `workspace_present` still resolves an undetermined `try_exists` to present; the two-observation debounce and the phase-3 re-read are unchanged. Measured against the live store (116 records, 40 shown): 27 evicted, 8 spared by the workspace guard, 41 spared as genuinely recent — 36 of those created AND decommissioned inside the last week. Max NUM 116 -> 89 on the first sweep cycle, falling toward the shown-row count as the recent tombstones age out. Refs #3034 🤖🤖🤖 Generated with trusty-mpm — https://github.com/bobmatnyc/trusty-tools
One-time backfill added (head
|
| property | status |
|---|---|
| Workspace-exists guard | Unchanged. retention_verdict returns Keep on !is_terminal() || workspace_on_disk before it reads terminal_at, so the guard runs ahead of the backfill and no inferred date can reach a record with a live directory. Pinned by sweep_backfill_still_spares_a_legacy_record_whose_workspace_exists, which seeds a 400-day legacy record with a real directory and a canary file and asserts three sweeps stamp nothing, evict nothing, and leave the file intact. |
Fail-closed try_exists |
Unchanged. workspace_present still resolves Err to present. Not touched by this commit. |
| Two-observation debounce | Unchanged. A backfilled record is a candidate only on the sweep after it is stamped, then needs one more to confirm. |
| Phase-3 re-validation | Unchanged. Every eviction still re-reads the record and re-probes the filesystem immediately before deleting. |
The change is confined to which timestamp a record with terminal_at == None receives. It adds no path that bypasses a guard, and no new caller of remove_many.
Mutation-checked, both branches
Reverting the inference to the old now behaviour:
inferred_terminal_at_uses_the_latest_evidence_of_life ... FAILED
retention_verdict_stamps_undated_terminal_record ... FAILED
sweep_backfills_an_old_legacy_record_and_evicts_it_without_waiting ... FAILED
sweep_gives_a_recent_legacy_record_its_full_window ... FAILED
test result: FAILED. 15 passed; 4 failed
The guard tests stayed green under it, correctly — they do not depend on the inference.
Removing the future-timestamp fallback:
inferred_terminal_at_falls_back_to_now_for_a_future_timestamp ... FAILED
sweep_stamps_now_when_a_legacy_record_has_no_usable_signal ... FAILED
test result: FAILED. 17 passed; 2 failed
What the owner will actually see — measured, and it is not the whole story
Against the live store as it stands (116 records, 40 rows shown):
| count | |
|---|---|
| evicted by inferred age | 27 |
| spared — workspace still on disk | 8 |
| spared — inferred age under 7 days | 41 |
| live (never in scope) | 40 |
Max NUM: 116 → 89 on the first sweep cycle. Materially better, and it arrives in minutes rather than a week. But it is still 89 against 40 visible rows, so the owner will not see the two-digit-in-a-fifty-item-list result he asked for on day one, and I would rather say that now than have him find it.
The reason is not a defect. Of the 41 records spared as recent, 36 were created AND decommissioned inside the last seven days — they have no last_activity_at at all and a created_at from this week. Those are genuinely fresh tombstones and the window is doing exactly its job. They age out on their own, and NUM lands near the shown-row count about a week from now, permanently.
If the owner wants it tighter on day one, the lever is TERMINAL_RECORD_RETENTION_DAYS — his call, not mine, and I have not touched it.
Gate — rung 5, at ab90cd4c
cargo fmt --check && cargo check -p trusty-mpm && cargo clippy -p trusty-mpm --all-targets -- -D warnings
EXIT=0
cargo test -p trusty-mpm --no-fail-fast
test result: FAILED. 4679 passed; 1 failed; 7 ignored; 0 measured; 0 filtered out; finished in 48.86s
test result: ok. 1485 passed; 0 failed; 1 ignored; 0 measured; 0 filtered out
test result: ok. 1485 passed; 0 failed; 1 ignored; 0 measured; 0 filtered out
(+ 42 further binaries, all ok)
failure: daemon::managed_routes::tests::stale_assets_for_many_reads_shared_agent_dir_once_for_the_whole_fleet (baseline row 38)
cargo test -p trusty-mpm --lib -- --include-ignored
test result: FAILED. 4686 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out; finished in 47.75s
failure: core::managed_config::tests::ensure_managed_config_dir_emits_the_frozen_skill_warning (#4931)
cargo test -p trusty-mpm --lib -- --include-ignored --test-threads=1 core::managed_config::tests::ensure_managed_config_dir_emits_the_frozen_skill_warning
test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 4686 filtered out
EXIT=0
Nine lint gates, all EXIT=0: check_line_cap (3728 files, 0 violations), check_sld, check_doc_numbers, check_test_pointers (21963 citations, 0 dangling), check_capabilities, check_changelog_fragment, check_generation_artifacts, check_agent_assets, check_buildrs_sync.
One thing I caught in myself, worth recording
My first pass at this replaced a region of the test file by text range and silently took four unrelated tests with it — workspace_present_treats_an_undetermined_path_as_present, both debounce_*, and sweep_spares_a_record_reactivated_between_sweeps. cargo test went green on 4675 tests and I nearly pushed it. What caught it was a clippy unused-import error on workspace_present, not the suite, because a deleted test cannot fail.
That is the "never make a red gate green by deleting coverage" rule violated by accident rather than intent, and the accidental version is harder to see. I diffed the test-function list against HEAD before and after, restored all four, and confirmed the only remaining deletion is sweep_stamps_legacy_records_instead_of_evicting_them — whose premise ("an old legacy record is stamped now and NOT evicted") is precisely what this commit reverses, replaced by two tests. Net for this commit: +6 tests. I am adding that before/after function-list diff to how I do range edits on test files.
🤖🤖🤖 Generated with trusty-mpm — https://github.com/bobmatnyc/trusty-tools
…OME mutation CI `Test` failed at ab90cd4 on `isolated_managed_state_guard_panics_on_production_root` — "test did not panic as expected", 27 passed / 1 failed. Not one of the documented baseline flakes. The test reads `dirs::home_dir()` twice: once in its body to build the production path, once inside `with_root_isolated_managed`'s guard. Eight tests in the same binary hold a `HomeGuard` that overrides `$HOME` process-wide, and `#[serial_test::serial]` only serialises those against each OTHER — this test carried no such attribute. When a HomeGuard boundary lands between the two reads they disagree about the production path, `assert_ne!` passes, nothing panics, and the `should_panic` test fails. Proven rather than assumed: widening the gap between the two reads to 1500ms reproduces the CI signature exactly (27 passed, 1 failed, "did not panic as expected"); adding `#[serial]` makes that same widened case pass 28/28. The diagnostic sleep is not part of this commit. Pre-existing, not caused by this branch: `git diff origin/main...HEAD -- crates/trusty-mpm/tests/` is empty, so the test file was byte-identical to main when it failed. Same `$HOME`-mutation-race class as baseline rows 38 and 39, which cover the sibling cases in the lib crate. Fixed rather than re-rolled — a re-run that happened to pass would have proven nothing and left the defect for the next PR. Refs #3034 🤖🤖🤖 Generated with trusty-mpm — https://github.com/bobmatnyc/trusty-tools
CI
|
1 participant
The defect
tm lsprintedNUM 107at the bottom of a 31-row listing. The slot registry (#3034) hands a number to every record it observes; the default view then hidesdecommissioned/deletedrows. Measured on the reporting machine: 111 records, 76 terminal, 35 shown — 76 of the assigned slots were held by rows nobody could see.Nothing had ever left the store. The auto-prune path calls
/decommissionwithrecord_only=true, which leaves the record in place. So lowest-free slot allocation on its own is a provable no-op: the free set is permanently empty. Freeing numbers requires evicting the records behind them.Change 1 — record retention (7 days)
Timestamp: a new
SessionRecord::terminal_at, stamped when the record entersDecommissioned/Deleted. Notcreated_at— a session born in January and decommissioned today is a one-day-old tombstone, and a clock keyed off birth would delete it immediately. It is written at exactly one place,SessionRecord::enter_terminal_state, so a future terminal transition cannot silently skip it. Re-entering the same terminal state does not refresh the stamp (a repeated decommission must not extend retention); moving back to a live state clears it.Trigger: the daemon's existing orphan-GC tick (
daemon/mod.rs), placed ahead of theTmuxDriver::discover()guard thatcontinues the tick — a store-only sweep must not be disabled by a machine having no tmux.AUTO_PRUNE_CAP=5insession_picker_prune.rsis untouched: that is client-side rate limiting on the CLI's decommission path, a different mechanism at a different layer.Window:
TERMINAL_RECORD_RETENTION_DAYS = 7, a named const, pinned byretention_window_is_seven_days.How #3034's tombstone intent survives
#3034 asks that a deleted session's slot stay reserved and render as
-- deleted --rather than being handed to the next session. Three things keep that true:sweep_preserves_the_in_window_tombstone_slot.numbered_snapshotnow walks the registry's held slots instead of1..=max, so a released slot leaves the listing entirely rather than becoming a phantom tombstone at a number no operator ever saw.The guarantee narrows from "forever within a daemon lifetime" to "for the whole retention window", which in practice is the same thing.
Eviction touches the record store and nothing else
sweep_terminal_recordscallsSessionStore::remove_many(an entry insessions.json) andSlotRegistry::release(an in-memory map). It opens no other path for writing. The one filesystem call it makes is an existence check, and that check can only ever make it evict fewer records.That check is load-bearing, not tidiness.
prune_orphaned_worktreesprotects a worktree from deletion by finding its path among theworkspace_paths read from the store — a set deliberately unfiltered by state, because a record carrying a terminal state is routinely a live session with unsaved work (see the measured case documented inprune::reap_orphaned_worktrees). Both of that sweep's two independent reads come from the store, so deleting a record would remove the path from both at once, collapsing a defense-in-depth pair into nothing and handing the worktree to an unattendeddry_run: falsetimer. A record whose workspace directory still exists is therefore never evicted, however old.sweep_never_touches_the_filesystemseeds a 400-day-old terminal record with a real directory and a canary file, and asserts the sweep evicts nothing, leaves the directory standing, and leaves the file's bytes unchanged.Legacy records are dated, not deleted
Every pre-existing record has
terminal_at == None. That means UNKNOWN, not old: it is stamped with the current time and the window starts there. Inferring death fromlast_activity_at/created_atwould be wrong in the common direction — the auto-prune (#4384/#4702) decommissions long-idle records, so a record whose last activity is a month old may have become terminal seconds ago, and 27 of the 76 terminal records here look "7+ days old" by that measure while their actual death dates are unknown. Guessing would evict them on the first sweep with zero retention.Measured before / after
The 8 survivors are the terminal records whose workspace directory still exists (67 have no
workspace_path, 1 points at a path that is gone). One write per sweep, not per record —upsert_many/remove_manybatch the reload and the save.Change 2 — color NUM and NAME
Two distinct hues:
NUMmagenta (35),NAMEcyan (36). Not one hue — a name's trailing serial comes fromallocate_serial(per-project, 01–99, reuses gaps) whileNUMis a global slot, sotm-foo-01atNUM 1is coincidence; 57 currently listed sessions end in-01, and retention makes an accidental match more likely, not less. Both are plain (non-bold, non-bright) mid-tones, legible on light and dark backgrounds —33was avoided because light themes wash it out.table_use_color(stdout_tty), notpicker_use_color. The table prints withprintln!; the picker witheprintln!. Reusing the picker's stderr gate would leak escapes intotm ls | grepwhenever stderr stays a TTY. Both delegate to onecolor_enabledso theNO_COLORrule (any set value, including empty, disables) cannot drift.{:<5}/{:<24}measure the formatted value, so an ANSI-wrapped column came out ~9 chars narrow.pad_visiblecolorizes the text and appends the padding outside the escape.format_tombstone_rowgets the same treatment and itsNUMcolumn still lines up with a live row's.ls_row_plain_when_color_disabledasserts the plain row has no escape at all and that stripping the escapes from the colored row reproduces it exactly.Rendered (
\x1bshown literally):managed.rswas 477/500 SLOC, so the table renderer moved tocommands::managed_render.Gate — rung 5 (record persistence, irreversible deletion)
Nine lint gates, all
EXIT=0:check_line_cap.shcheck_sld.shcheck_doc_numbers.shcheck_test_pointers.shcheck_capabilities.shcheck_changelog_fragment.shcheck_generation_artifacts.shcheck_agent_assets.shcheck_buildrs_sync.shNew coverage:
retention_tests.rs(10 cases — the pure verdict for every guard, eviction, slot release and reuse, the no-duplicate-NUMinvariant across that cycle, the filesystem no-touch assertion, and the legacy stamp-then-evict sequence),slot_registry_release_*,store_{upsert,remove}_many_*,enter_terminal_state_*, and the threels_row_*color/alignment cases.Refs #3034
🤖🤖🤖 Generated with trusty-mpm — https://github.com/bobmatnyc/trusty-tools