fix(session-storage): catch a mid-staging resume that only reads the transcript

[chenmingwei23]

Problem / Motivation

move_to_trash's revival guard (added in #7081) decides whether a session was
resumed mid-staging by comparing each source file's mtime against the instant the
reclaim began. That catches a resume that WRITES. It does not catch a resume that
only READS.

The uncovered sequence:

1. A retired session's files are certified reclaimable: unmapped by both index
   reads, untouched for MIN_RECLAIM_AGE_DAYS.
2. The session is resumed. The resume reads the old transcript to rebuild history.
3. The turn that follows is recorded under a newly mapped sid, without rewriting
   that old transcript.
4. Every one of the session's files therefore still carries its days-old mtime,
   all of them pass mtime > validated_at, and the whole session is staged.

The live slot then keeps running with its durable history in the trash.

Why it matters

A user resumes a chat and finds its history gone, with nothing in the transcript
explaining why. #7081 narrowed the window this happens in; the remaining case is
the one where the resume is a pure read, and its window is the whole move loop
rather than microseconds.

It is a continuity failure rather than data loss: restore puts the history back,
and emptying the trash needs a separate deliberate action. Same severity class as
the defect #7081 fixed.

What changed (motivation -> approach -> change)

The index is the only place a read-only resume is visible, because such a
resume is mapped even though it wrote nothing. So refresh is now called before
EVERY session the move loop reaches, not once before the loop. A session the
re-read reports as mapped is left in place and named in TrashBatch.revived, the
same way a write-shaped revival already was. Nothing of it has moved when the
check fires, so there is no rollback. The mtime check stays: it costs no syscall
(the stat is already taken for the manifest) and it catches the write-shaped
resume inside a session's own file walk, which the re-read cannot.

Making that cadence affordable is the whole design problem, and it is measured.
_build_index reads and parses the whole session map, so there is a per-call floor
of about 0.26 ms however small the map, and the selection is capped at
_MAX_SELECTION = 200_000:

map entries	_build_index()	union sets	per unit	x 200,000 units
100	0.260 ms	0.022 ms	0.282 ms	56 s
1,000	2.017 ms	0.238 ms	2.255 ms	451 s
10,000	21.74 ms	4.91 ms	26.65 ms	1.5 h
100,000	306.8 ms	88.0 ms	394.8 ms	21.9 h

os.stat() on the map file, for comparison: 2.0 us. It is not the six-figure
store that makes a naive per-session rebuild expensive, it is the six-figure
selection: 56 s even against a 100-entry map.

So the cheapness is where the knowledge is. refresh may return the SAME
SessionIndex object it returned last time to say "nothing has moved", which the
loop recognises by identity and does not re-derive sets for. The dashboard's
refresher (_MapBackedRefresh) rebuilds only when session_map.json's
(st_ino, st_mtime_ns, st_size) moves. Every mapping write lands through
mkstemp plus os.replace, so the inode changes on every write and an unmoved
token cannot hide one. Per-session cost at the cap: one stat, 0.4 s total. The
token is read BEFORE the rebuild and that one is stored, so a write landing during
a rebuild is seen by the next call rather than stamped as already-included. An
unreadable token is None, which never compares equal, so it costs a real re-read
rather than a skipped one. No signature change, and a caller that returns a fresh
index every call (every existing test) still gets a real re-read per session.

A re-read that fails mid-loop degrades rather than aborting. It is logged once
per batch and the last view read is kept. Every re-read only ever widens the live
sets, so losing one costs the extra protection it would have added, not the
protection already read; abandoning a batch that has already moved files would be
the worse trade. A refresh broken from the start still fails closed, before
anything moves, through the existing pre-loop raise.

Two candidate shapes the issue named were rejected, and why. Shape 1 (naive
per-unit rebuild) is the table above. Shape 2 (one refresh after the loop, then
roll back what is now live) costs one extra map read, but the unwind is the
problem: manifest entries are written as each session lands specifically so an
interruption leaves a manifest describing exactly what moved, so removing an entry
means either rewriting the manifest, which gives that invariant up, or a tombstone
every one of _read_manifest, _summarize_manifest, _restore_locked,
_manifest_rels, _listed_bytes, staged_targets and _empty_trash_locked has
to honour. The shape shipped here keeps the append-as-it-lands invariant untouched.

docs/system-specs/modules/session-storage.md moves with the code: the section is
retitled, the two-signal loop is described, the corrected Known Limitations entry
that #7282 added for this gap is replaced by the bounded residual that survives it,
and the handler section stops saying a read-only resume is staged instead of
refused.

Tests

Targeted files only (test_session_storage.py, test_session_storage_api.py):
284 passed. test_session_map_locking.py (the locking ratchet over this tree): 16
passed. black, ruff and mypy clean on the four changed Python files.

Red on base, with the exact assertions:

• test_a_resume_that_only_reads_the_transcript_is_left_in_place -
  assert () == ('bbbb2222',). The resume writes nothing at all: no append, no
  utime, no recreated origin. Also asserts every half is still in place and the
  batch holds no file of it.
• test_the_index_is_consulted_before_every_session - assert 1 == 4. Pins the
  cadence, which is what a future change could quietly drop.
• test_a_re_read_that_starts_failing_keeps_the_view_it_last_read -
  assert 3 == 2. The last good view still protects the session it named, the rest
  of the batch still moves, and the warning is logged once rather than per session.

test_an_unchanged_index_is_not_re_derived passes on base too, because base calls
refresh once - it is a cost ratchet, not a repro. Mutation-verified it has teeth:
replacing the identity check with an unconditional re-derivation turns it red
(assert ['active_stems', ...] == ['active_stems']).

Four tests cover the refresher itself: same object while the map has not moved
(one build for three calls), a rebuild after a mapping write, a rebuild when the
map is unreadable, and a write landing during a rebuild not being missed.

Manual verification

Not applicable - no user-visible surface changes. The behaviour is a refusal path
inside a reclaim, exercised by the tests above.

Related Issues

Closes #7118
Refs #7081, #7282

Pattern harvest

Rule candidate: review-prompt
Pattern: a guard infers "was this resumed" from a file's mtime, so a resume that
only READS the file is invisible to it.

The mtime proxy is the whole defect. It answers "was this written since we
certified it", which is a strictly narrower question than "is this live", and the
gap is exactly the read-only user. A review prompt can ask it directly: when a
staleness or liveness check reads an mtime, what does a reader of that file look
like to the check? Not a semgrep candidate - mtime > stamp is a correct and
common comparison, and only the surrounding claim makes it wrong here.

Three further observations from the fix, none of them rule-shaped on their own:

• A per-item check that is prohibitive at scale is often prohibitive only in its
  naive form. Measure the floor, not the ceiling: the first row of the table (a
  100-entry map) settles the design, because n is what multiplies it.
• Push the cheapness to whoever owns the change signal. This module already takes
  liveness from its caller by design, so a caller-memoized refresh is idiomatic
  here where a module that secretly stats the caller's backing file would not be.
• A monotonic signal (these sets are only ever unioned) can degrade safely on
  failure. That is what makes "log once and keep the last view" defensible instead
  of a swallowed error.

[github-actions]

Design Review (Fable 5) — ✅ PASS

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

Design-Verdict: PASS

A measured, disclosed-residual fix: read-only resumes get the same per-unit revival treatment writes already had, with cost bounded by a file-identity gate.

The one race left open (the ~50 ms _FLUSH_DEBOUNCE_SECS window where a resume is mapped in-process but not yet flushed) is already named in the diff and the spec, with the shared-lock terminal fix identified — flagging it again would change nothing the author builds. The rejected alternatives (naive per-unit rebuild, post-loop refresh + rollback) are dismissed on concrete grounds — the measured cost table and the append-as-it-lands manifest invariant respectively — and the refresh callback's new identity-based "unchanged" protocol is backward-compatible: a caller returning fresh objects stays correct, only slower, which the existing tests exercise.

[DESIGN-REVIEWED] 16382ec

[github-actions]

First Principles Review (Fable 5) — ✅ PASS

Premise-level review of 16382ecef9f879074057e0cd54ae91c0081809a8 — 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 claims verified: is_live at src/kiro_crew/session_storage.py:1778 is the existing closure the new per-unit check reuses; move_to_trash has exactly 2 callers (both dashboard handlers, both now passing _MapBackedRefresh()); _MAX_SELECTION = 200_000 exists at handlers/session_storage.py:63; _FLUSH_DEBOUNCE_SECS and the mkstemp+os.replace write path exist in session_map.py; no shared file-identity-token helper exists to reuse (each module hand-rolls its own stamp). The removed Known Limitations entry is replaced, not deleted, and the spec moves in the same commit as AGENTS.md mandates.

First-Principles-Verdict: PASS

Closes a named defect (#7118) at the deepest reachable level, with the out-of-scope cause (no shared lock) documented rather than hidden.

What this change ships

Intent: stop a reclaim from trashing the history of a session that was resumed by reading, not writing — a FIX.

1. A read-only resume mid-reclaim is now refused and reported in revived, not staged — justified (reported defect move_to_trash: the revival guard misses a resume that only reads the transcript #7118).
2. The index is re-consulted before every session, not once per batch — justified; the mechanism of the fix, cadence pinned by test.
3. refresh may return the same object to mean "unchanged", skipped by identity — justified; derived from a measured cost (0.26 ms floor × 200k cap).
4. Dashboard refresher rebuilds only when session_map.json's (st_ino, st_mtime_ns, st_size) moves — justified; 2 consumers (cleanup, trash handlers).
5. A mid-loop re-read failure logs once and keeps the last view instead of abandoning a half-moved batch — justified; monotonic sets make the degrade safe, pre-loop failure still fails closed.
6. Known Limitations replaces "not detected at all, window is the whole loop" with a debounce-bounded residual — justified; spec moves with the code (mandated invariant).

Every item is declared in the description, including both residual windows and the two rejected shapes. The reuse of the existing is_live closure (session_storage.py:1778) means the refusal path adds no second spelling. No zero-consumer surface, no inherited requirement found.

[FIRST-PRINCIPLES-REVIEWED] 16382ec

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of 16382ecef9f879074057e0cd54ae91c0081809a8 and found no blocking issues.

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] 16382ec

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

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

Reviewed 16382ecef9f879074057e0cd54ae91c0081809a8 — this comment is updated in place on each push.

Review details

Based on my review, the candidate list contained no candidates from the discovery pass. I independently examined the core logic:

• The is_live closure correctly reads the reassigned live_sids/live_stems cells at call time (standard Python enclosing-scope capture), so per-session widening is visible to the check that follows it.
• The per-session refresh block is correctly ordered: unit is None continue → refresh (widen on new object by identity) → is_live(uid) catch → stage. Sets only ever union (widen), never shrink.
• Mid-loop refresh failure falls through to is_live with the last-good sets, logs once via refresh_failed, and correctly does not abort a batch that has already moved files; a start-of-life failure still fails closed at the pre-loop refresh() which raises.
• _map_token() stats config_dir() / SESSION_MAP_FILENAME, which matches SessionMap._path, and SessionMap._write uses mkstemp + os.replace, so the inode changes on every mapping write — the token cannot hide a write. Token is read before the rebuild and stored, so a write racing the rebuild is re-caught next call.
• _build_index reads a fresh SessionMap() from disk each rebuild; _MapBackedRefresh fails toward rebuilding on an unreadable token (None never compares equal).
• Both handlers construct a fresh _MapBackedRefresh() per request. move_to_trash runs under to_thread, so per-session file reads stay off the event loop. Docs updated in the same commit.

No behavioral defect or AUTOSDE rule violation grounded in the changed lines survived falsification, and I found no new grounded finding at the 80+ bar.

No findings.

[OPUS-REVIEWED] 16382ec

Verdict parsed from the review's SHA-scoped output markers for commit 16382ecef9f879074057e0cd54ae91c0081809a8.

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

[bolichen97]

Tier 1 auto-approve: fix (5 files). Criteria: no conflict, no requested changes, security path denylist clean, design-doc gate clean, SAST annotations clean, security checklist all-NO, AI reviewers green. Category: fix with a clear root cause — catches a mid-staging resume that only reads an old transcript (#7118) so its history is not reclaimed out from under a live slot, via a map-file-identity-gated per-session refresh. Spec files changed as a ride-along (a minority of the diff on both file count and changed lines), not reviewed as a design decision: docs/system-specs/modules/session-storage.md.

[bolichen97]

Reviewed via parallel subagent audit: diff matches description, CI fully green, no blocking findings, no unresolved threads.