Skip to content

fix: skip a history save whose session was permanently deleted under lock - #6707

Merged
bolichen97 merged 1 commit into
mainfrom
fix/save-guard-deleted-session-6677
Aug 29, 2026
Merged

fix: skip a history save whose session was permanently deleted under lock#6707
bolichen97 merged 1 commit into
mainfrom
fix/save-guard-deleted-session-6677

Conversation

@chenmingwei23

@chenmingwei23 chenmingwei23 commented Aug 29, 2026

Copy link
Copy Markdown
Contributor

Problem / Motivation

A session the user permanently deleted can reappear on disk in Older Sessions. _save_slot_to_history takes the per-session _locked for its whole read-modify-atomic_write, and delete_session unlinks the file under the same lock -- but the save never re-checked, inside the lock, that the session still existed. save_slot_off_loop routes on-loop callers to a worker thread that takes the PATIENT lock acquire, so a save can legitimately sit waiting while a permanent delete runs to completion ahead of it; when the save finally gets the lock it goes straight to mkdir + atomic_write and silently recreates the session the delete had already reported destroyed. delete_session deliberately leaves no tombstone, so nothing downstream catches it.

Why it matters

Permanent delete is the user's strongest promise in the product: "this conversation is gone." A save losing the lock race breaks that promise silently -- the conversation the user destroyed comes back, with no error anywhere and only the delete's success answer on record. For a user deleting sensitive content, resurrection is a real harm, not a cosmetic bug.

What changed (motivation -> approach -> change)

Symptom: deleted session reappears -> root cause: the save could not tell "never existed here (normal first create)" from "existed and was permanently deleted under this lock", and its content could also escape through copies (fork, transfer). The shipped mechanism, converged over nine review rounds (all dispositions in the PR comments):

  • Identity is the sole delete-evidence. Each slot records _disk_meta_created_at -- the metadata created_at of the transcript it last OBSERVED, written exactly at the hydrate sites (dashboard rehydrate x2, channel surfacing, resume handler) and at each committed save, nowhere else. created_at is carried forward by every save, so it never changes for a continuously-existing file: it is the file's identity. The window counters (_resumed_count etc.) take no part in the evidence -- fork/transfer set them optimistically after a best-effort first save (a transient first-write failure must not read as a deletion and eat the retry), and a restored zero-message session has all-zero counters while its delete must still win.
  • The delete-won guard. Inside _locked, before any mkdir/atomic_write: with a known identity, a missing file (stat FileNotFoundError only) or an on-disk created_at that no longer matches (a fresh incarnation created by a foreign append after the delete) aborts the save cleanly -- no write, no error, WARNING log with the slot key, and the flush loop clears _dirty so the delete's reported success stands. The abort returns False (every other completion True), threaded through save_slot_off_loop, so callers that must confirm durability can tell the skip from a committed write.
  • Unreadable metadata fails CLOSED. The save reads metadata via get_metadata_status; a transiently unreadable line makes the save raise (deferral: _dirty stays armed, the flush retries) instead of blanking the identity comparison and overwriting a replacement session with deleted content.
  • Copies are guarded directly. Fork and transfer export call session_was_deleted(state, slot) at their copy choke points -- the periodic 5s flush can consume the guard's signal first by clearing _dirty, so the copy paths cannot rely on their own flush arms observing it. The probe applies the same identity rule, and unreadable metadata refuses the copy retryably. Fork refuses with 409; transfer raises SnapshotUnstable. Being lock-free, the probe can also have the delete land INSIDE it, between its stat and its metadata read: get_metadata_status reports a vanished file as a genuine ({}, True), so an empty created_at is re-stated before it is trusted -- gone means refuse, still-there means this is legacy metadata and fails open as documented. The save's guard needs no equivalent, because it does both reads inside _locked, the lock delete_session unlinks under.
  • The copy is re-checked at the ACKNOWLEDGMENT boundary. One pre-copy probe is not enough, because writing the copy is itself an await that does not serialise against the source's delete: the transfer re-probes after bundle assembly, and the fork re-probes after its DESTINATION save (which takes the destination's lock, so nothing in it orders against the source's). Acknowledgment is the boundary a handler owns, so a delete committing before it wins: the fork removes the destination transcript it had already written (delete_session on the destination key, off-loop) and pops the never-broadcast slot before answering 409. Rolling the destination back cannot harm the source (different key, different lock), so the fail-closed probe costs at worst a retryable 409 against a still-live source. A delete committing AFTER the copy is acknowledged is deliberately out of scope: a fork acknowledged while its source was alive is its own session and survives the source. If the rollback removal itself fails, the copy stays on disk and is logged at ERROR -- the one case that still needs a human, and no worse than the unconditional persistence this replaces.

Doc sync: docs/system-specs/modules/history.md documents the guard, the identity rule, the fail-closed behavior, the return contract, the acknowledgment boundary and its rollback, and the residuals (fresh-slot adoption; the latched cron-linked-slot case). Three pre-existing tests fabricated a resumed slot with no on-disk file (now indistinguishable from delete-won) and were fixed to persist first with their original contracts preserved; save fakes in the fork/transfer test modules return True per the new contract. session_transfer.py and channel_slots.py were reformatted and graduated from the black baseline: the black gate itself fails with "graduated entr(y/ies) to prune" until the baseline shrinks, so the prune must ride in the same change.

Tests

TestSaveDoesNotResurrectDeletedSession (test_dashboard_chat.py), 16 tests, each guard/probe mechanism mutation-verified red without its fix:

  • save after committed delete does not recreate the file (returns False); the _resumed_count arm; first save of a fresh slot still creates (control).
  • fork aborts 409 when the source was deleted (mid-flush AND after a flush consumed the signal); transfer bundle refuses (pre-read AND a delete landing during assembly).
  • a delete landing INSIDE the fork's destination save is rolled back: 409, the source stays deleted, no new transcript survives, and the destination slot is gone from the live set. Mutation-verified: without the boundary re-check the fork answers 200 and a full copy of the deleted conversation persists under a fresh key.
  • a delete landing inside the probe itself (between its stat and its metadata read) is caught, with a companion control pinning that legacy metadata carrying no created_at still fails open while the file exists.
  • a file recreated by a foreign append after the delete is not merged into (identity mismatch); channel-surfaced slots record the identity and honor it.
  • a failed best-effort first save is not mistaken for deletion; a zero-message resumed session's delete still wins; unreadable metadata defers the save and makes the probe refuse the copy.

Gates: isort/flake8/mypy clean; black gate passes; docs lint passes; 859 tests green across the dashboard-chat/fork/transfer suites, and the full backend suite is green apart from failure families proven pre-existing on this environment by an A/B run with the change stashed (test_artifact_source, test_file_explorer_app, test_design_tweak_backend, test_host_isolation_floor, test_dashboard_peer_auth and siblings -- none in the files this PR touches).

Manual verification

N/A -- the race windows are deterministic to pin in unit tests (a delete committing before the save's lock acquire, and a delete committing inside the fork's destination save, both exercise the identical code paths), and the fork/transfer refusals are pinned at the endpoint level.

Closes #6677

@chenmingwei23
chenmingwei23 requested a review from a team as a code owner August 29, 2026 03:33
@github-actions github-actions Bot added the readiness: checking Automated validation is still running label Aug 29, 2026
@github-actions

github-actions Bot commented Aug 29, 2026

Copy link
Copy Markdown
Contributor

GPT 5.6 Review — ✅ human override accepted

Human judgment by @chenmingwei23 overrides the GPT 5.6 finding for c318c3b89334d5dae11362505ecab7ab0a49b516; the recorded reason is authoritative for this commit.

This comment is updated in place on each push.

The model was not re-run because an authorized human decision supersedes it.

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

@github-actions

github-actions Bot commented Aug 29, 2026

Copy link
Copy Markdown
Contributor

Design Review (Fable 5) — 🟡 CONCERNS

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

Design-Verdict: CONCERNS

Sound identity-based fix for a real privacy break, but the invariant is convention-enforced at scattered call sites and fails open when a site is omitted.

Watch

  • The guard's evidence (slot._disk_meta_created_at = str(meta.get("created_at") or "")) is hand-assigned at four hydrate sites, and republish safety depends on each copy path remembering to call session_was_deleted. A future hydrate or export/copy surface that omits either line silently reintroduces resurrection — the guard treats "no evidence" as a fresh slot and proceeds. Nothing structural (helper, gate, pinning test over call sites) catches the omission.
  • The probe and the in-lock guard apply three subtly different fail rules (guard raises on unreadable metadata, propagates non-ENOENT stat errors; probe fails closed on both; probe alone re-stats an empty created_at). Each divergence is justified and tested today, but the split logic is the part most likely to drift when one side is next edited.

Suggestions

  • Extract identity adoption into one helper (e.g. adopt_disk_identity(slot, meta)) used by all four hydrate sites, so a new restore path gets the guard by construction rather than by copy-paste.

[DESIGN-REVIEWED] c318c3b

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Aug 29, 2026
@github-actions

github-actions Bot commented Aug 29, 2026

Copy link
Copy Markdown
Contributor

First Principles Review (Fable 5) — 🟡 CONCERNS

Premise-level review of c318c3b89334d5dae11362505ecab7ab0a49b516 — 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 counts are in hand. Composing the review.

First-Principles-Verdict: CONCERNS

The bool-return channel is a second spelling: two of its three saved consumers sit lines above a session_was_deleted probe that already refuses the same condition.

What this change ships

Intent: stop a lock-racing save (or a fork/transfer copy) from resurrecting a permanently deleted session — a FIX for #6677.

  1. A save losing the lock race to a permanent delete no longer recreates the file — justified (the fix)
  2. A pending save no longer merges deleted turns into a foreign-recreated file — justified (identity rule, cause-level)
  3. Forking a deleted session answers 409 instead of republishing it — justified
  4. A delete landing during the fork's destination write is rolled back pre-acknowledgment — justified
  5. Transfer refuses a deleted source at pre-read and post-assembly — justified
  6. Unreadable metadata defers the save instead of writing blind — justified (fail-closed, named boundary)
  7. Save paths return committed-vs-skipped (bool) — 3 consumers, 2 subsumed by the probe
  8. session_was_deleted probe + _disk_meta_created_at slot identity — justified (4 probe call sites; all 4 hydrate sites armed, verified)
  9. Two files whole-file reformatted, pruned from the black baseline — rides along, declared; AGENTS.md welcomes it and check_black_formatting.py:243-261 couples the prune to the reformat
  10. history.md section + three fixture repairs — mandated (same-commit spec rule; fixtures fabricated impossible states)

Watch

The description justifies the probe because "the copy paths cannot rely on their own flush arms observing" the False — conceding the probe is the strict superset. I traced both subsumed arms: fork's plain-flush check falls through to the probe's identical 409; transfer's falls through to the probe's identical SnapshotUnstable. Only the pending-rewrite arm is not subsumed (without it the snapshot loop terminates in a misleading retryable 503, never reaching the probe), so the return contract's real consumer count is 1.

Subtractions

  • Drop the saved arm at the fork's plain pre-copy flush (chat_fork.py:557-576) — the session_was_deleted probe ~15 lines below returns the identical 409 for the identical condition.
  • Drop the saved arm at the transfer's pre-bundle flush (session_transfer.py:621-631) — the probe on the same loop attempt raises the same SnapshotUnstable.
  • Keep the pending-rewrite arm; with the other two gone, shrink the return-contract docstrings (and the ~10 return True test-fake edits) to that single consumer.

[FIRST-PRINCIPLES-REVIEWED] c318c3b

@github-actions

github-actions Bot commented Aug 29, 2026

Copy link
Copy Markdown
Contributor

Opus 4.8 Review — ✅ no blocking findings

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

Review details

Both candidates hinge on a transient metadata-read failure (get_metadata returning {} for an existing, live file) — a speculative "might happen" input, not a concrete condition that occurs in practice.

Candidate 1 (spurious mismatch from compaction re-minting created_at): _rewrite_session_locked (history.py:5652) preserves created_at via orig_meta.get("created_at", metadata_now_iso()). For a live session being compacted, orig_meta carries the existing created_at, so the value is stable — the save always carries the on-disk value forward. The only path that mints a fresh created_at is a transient read failure returning {}, which is exactly the "might" input the falsification bar forbids. No non-transient trigger exists. Not grounded.

Candidate 2 (rewind/regenerate save raises and loses state): the raise is the intended fail-closed deferral, and chat_rewind.py:211 sets slot._dirty = True (reinforced by the slot.append at :242) before the save at :246. The raise propagates before any _dirty clear, so the flag survives and the periodic flush retries — the deferral is safe, no observable data loss. Falsified.

No self-originated finding meets the (a)/(b)/(c) bar in the changed lines; the guard's fail-open on legacy/empty created_at, the fresh-slot first-create path, and the fork/transfer rollback are all covered and consistent.

No findings.

[OPUS-REVIEWED] c318c3b

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

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

@chenmingwei23
chenmingwei23 force-pushed the fix/save-guard-deleted-session-6677 branch from 0262a30 to fad578c Compare August 29, 2026 03:50
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Aug 29, 2026
@chenmingwei23

Copy link
Copy Markdown
Contributor Author

GPT round 1 disposition (head was 0262a30, finding: "delete-won state is cleared before copy guards can observe it"):

Fixed in fad578c, at the cause rather than via the prescribed remedy. The finding's bypass chain is real and was reproduced in a test: the periodic 5s flush can hit the delete-won guard first and clear _dirty, after which the fork/transfer flush arms (gated on slot._dirty) never run, the disk read comes back empty, and the fork's all_messages falls back to the in-memory window of the deleted conversation.

The prescribed remedy (re-mark the slot dirty before returning False) was not taken: nothing consumes _dirty as a delete signal, so it would only put the slot into a permanent 5s flush->abort->re-dirty loop while leaving the fork/transfer bypass intact for any ordering where their own flush arm still never runs (a non-dirty slot skips it regardless of who marked what).

Instead, a shared session_was_deleted(state, slot) probe (same evidence rule + stat-ENOENT witness as the guard, answered independently of flush ordering; lock-free is safe because a permanent delete never un-happens) is now called directly at both copy choke points: the fork refuses with 409 before its in-memory fallback can publish, and build_transfer_bundle_async raises SnapshotUnstable before assembling. Two regression tests pin exactly the non-dirty bypass ordering (test_fork_aborts_even_after_a_flush_consumed_the_delete_signal, test_transfer_bundle_refuses_a_deleted_session) and both are mutation-verified red without the probes.

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Aug 29, 2026
@chenmingwei23
chenmingwei23 force-pushed the fix/save-guard-deleted-session-6677 branch from fad578c to 74cee8b Compare August 29, 2026 04:00
@chenmingwei23

Copy link
Copy Markdown
Contributor Author

GPT round 2 disposition (head was fad578c, finding: "deletion during bundle assembly bypasses the guard"):

Fixed in 74cee8b as prescribed. The window is real: the threaded _read_and_assemble is the builder's longest await (redaction regexes over the whole transcript), so a permanent delete can complete inside it after the pre-read probe passed, leaving the destroyed conversation in the bundle about to be returned. The session_was_deleted probe is now repeated in the builder's existing post-await re-check block (alongside _guard_snapshot), and raises SnapshotUnstable -- a refusal, not a retry, since a permanent delete never un-happens. Regression test test_transfer_refuses_a_delete_landing_during_assembly commits the delete inside a wrapped _read_and_assemble and is mutation-verified red without the re-check.

@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Aug 29, 2026
@chenmingwei23
chenmingwei23 force-pushed the fix/save-guard-deleted-session-6677 branch from 74cee8b to db48d9c Compare August 29, 2026 04:14
@chenmingwei23

Copy link
Copy Markdown
Contributor Author

GPT round 3 disposition (head was 74cee8b, finding: "Recreated sessions bypass the delete-won guard"):

Fixed in db48d9c, with a corrected identity anchor. The bypass is real: delete_session leaves no tombstone, so a foreign append_off_loop (channel/cron) landing after the delete creates a FRESH session file, and a pending save that only checked existence would merge the deleted window into that new transcript.

The prescribed comparison target (slot.created_at) was not used as-is: a channel/cron slot legitimately adopts a transcript whose file was CREATED by an append -- its metadata created_at never matches the slot's own construction time, so that comparison would permanently discard every save of such a slot. The guard instead compares against the identity the slot last OBSERVED: a new slot field _disk_meta_created_at, recorded at restore (both rehydrate sites) and at each of the slot's own committed saves. created_at is carried forward by every save, so it never changes for a continuously-existing file -- a known-vs-known mismatch means a different incarnation and the save skips; unknown on either side (fresh slot, legacy meta without created_at) fails open to pre-guard behavior. The session_was_deleted probe used by fork/transfer applies the same rule, so a recreated file also refuses forks and transfers of the deleted slot.

Regression test test_save_does_not_merge_into_a_file_recreated_after_the_delete pins the exact chain (delete -> foreign append recreates -> pending save skips, new file left untouched) and is mutation-verified red with the identity comparison disabled.

@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Aug 29, 2026
@chenmingwei23
chenmingwei23 force-pushed the fix/save-guard-deleted-session-6677 branch from db48d9c to c8f260b Compare August 29, 2026 04:27
@github-actions github-actions Bot removed the readiness: action required A blocking check or review needs attention label Aug 29, 2026
@chenmingwei23

Copy link
Copy Markdown
Contributor Author

GPT round 4 disposition (head was db48d9c, finding: "Channel-restored slots lose delete identity"):

Fixed in c8f260b, again with the observed-identity anchor rather than the prescribed slot.created_at fallback. The finding is real: surface_channel_session (channel_slots.py) and the resume handler (chat_handlers.py) hydrate slots without running the dashboard rehydrate paths, so _disk_meta_created_at stayed empty for those slot classes and the identity arm failed open -- delete -> inbound append recreates -> pending save merges.

The prescribed fallback was not adopted for the same reason as round 3: comparing against slot.created_at only works on paths that happen to copy the file's created_at into it first, and on any path that does not, the slot's own construction time never equals an adopted transcript's created_at, so the fallback would misread every legitimate adoption as delete-won and permanently discard those saves. The durable rule stays "compare what the slot OBSERVED on disk": both hydrate sites now record _disk_meta_created_at from the meta they just read, exactly like the two dashboard rehydrate sites wired in round 3.

Regression test test_channel_surfaced_slot_records_the_disk_identity pins the full channel chain (surface -> delete -> foreign append recreates -> pending save skips, returns False, new file untouched) and is mutation-verified red without the channel_slots wiring. 1024 tests green across the dashboard/channel/restore/transfer/fork suites.

@github-actions github-actions Bot added the readiness: checking Automated validation is still running label Aug 29, 2026
@chenmingwei23

Copy link
Copy Markdown
Contributor Author

GPT round 8 finding ("Delete can commit after the sole probe while the fork is saved"): fixed in 3448f21 -- round 8 implemented as prescribed, superseding the escalation posted above.

The finding is real and the remedy is the right one, for a reason worth stating: this PR had already established the rule on the transfer side. build_transfer_bundle_async re-probes session_was_deleted AFTER its threaded bundle assembly, before the peer send is acknowledged. The fork had only the pre-copy probe, so the two copy paths in the same change disagreed about the same window. This is a consistency fill, not a new contract.

What landed:

  1. Post-save re-probe at the acknowledgment boundary (chat_fork.py). The destination save takes the DESTINATION's history lock, so nothing in it orders against the source's delete; a delete beginning after the pre-copy probe can commit inside that await. The handler now re-probes before answering.
  2. Destination rollback. On a delete-won verdict the handler removes the destination transcript it had already written (delete_session on the destination key, via asyncio.to_thread) and pops the slot, then returns 409 fork_source_deleted. The copy was never acknowledged and never broadcast (push_slots_update runs after this point, and every copied append passes broadcast=False), so nothing outside the handler had observed it.
  3. Rollback failure is loud, not silent. If the removal fails the copy remains on disk and WILL be listed in Older Sessions; that path logs at ERROR with both keys and still answers 409, and the SEL audit line records rollback=removed|failed. Reporting success there would additionally hide it.

On the destructive-rollback concern raised in the escalation: it is bounded in the safe direction. Removing the destination cannot touch the source (different key, different lock), and session_was_deleted fails CLOSED on an unverifiable stat or metadata read -- so a false positive costs a retryable 409 against a source that is still live and still forkable, not data. The asymmetry is the point: fail-closed is cheap when the consequence is "refuse, retry", and that is the only consequence here.

Scope boundary, unchanged and deliberate: a delete that commits AFTER the fork is acknowledged is not a resurrection. That fork is its own session, created by an explicit request that passed its checks while the source lived, and it survives the source. Acknowledgment is the only boundary a handler can own.

Verification: new test test_fork_rolls_back_a_delete_landing_during_the_destination_save pins it -- the source is non-dirty so the handler's flush arms do not run and the destination save is the only save, a wrapper commits the permanent delete inside that save, and the test asserts 409, the source still deleted, no transcript surviving the pre-fork set, and the destination slot gone from _slots. Mutation-verified: with the re-check neutralized the fork answers 200 {"ok": true, ...} with the deleted conversation persisted under a fresh key. TestSaveDoesNotResurrectDeletedSession is 14/14; isort/flake8/mypy/black/docs-lint clean; 857 tests green across the dashboard-chat/fork/transfer modules. The spec paragraph in docs/system-specs/modules/history.md moved with the code.

The full backend suite is green apart from failure families proven pre-existing on this environment by an A/B run with the change stashed (identical failures, none in files this PR touches).

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Aug 29, 2026
…lock (#6677)

_save_slot_to_history now aborts, inside _locked and before any
mkdir/atomic_write, when the session file is gone (stat ENOENT only)
AND the slot has evidence it was previously on disk (_resumed_count,
the disk_older snapshot, or _disk_window_len). A permanent delete that
committed while the save awaited the patient lock acquire is no longer
silently undone; a brand-new slot's first save still creates the file.

The abort returns False (threaded through save_slot_off_loop) so
callers that republish the slot's content elsewhere can distinguish
the skip from a committed write: the fork aborts with 409 and the
transfer export refuses the bundle, instead of resurrecting the
destroyed conversation under a fresh key that carries no delete
evidence.

Closes #6677
@chenmingwei23
chenmingwei23 force-pushed the fix/save-guard-deleted-session-6677 branch from 3448f21 to c318c3b Compare August 29, 2026 15:43
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Aug 29, 2026
@chenmingwei23

Copy link
Copy Markdown
Contributor Author

GPT round 9 finding ("Delete between stat and metadata read is missed", chat_persistence.py session_was_deleted): fixed in c318c3b.

Verified real against the contract rather than by reading the call alone. get_metadata_status's own docstring says readable is False ONLY when the transcript exists but its metadata line could not be read, and that "every other empty result -- no transcript at all, an empty first line, an undecodable one -- is a genuine answer and reports True". So a delete committing after this probe's stat() and before its metadata read returns ({}, True): readable is True, current is empty, the identity comparison was gated on current being non-empty, and the probe answered "not deleted" for a session that is gone. Fork and transfer would then republish it.

The fix re-stats when current is empty, which is what separates the two empties that were being conflated:

  • file gone (FileNotFoundError) -> the delete witness -> refuse the copy;
  • any other stat failure -> existence unverifiable -> refuse, same as the first stat's arms;
  • file still there -> the empty created_at is genuine legacy metadata, which fails OPEN by the documented rule, unchanged.

Scope, and why the sibling guard is untouched: this hole is exclusive to the deliberately lock-free probe. _save_slot_to_history enters _locked at chat_persistence.py:2503 and does BOTH reads inside it -- get_metadata_status at :2511 and path.stat() at :2556 -- and delete_session unlinks under that same lock, so no delete can interleave between them there. Adding a re-stat to the save would be dead code.

Verification, and a note on the mutant: test_probe_catches_a_delete_landing_between_its_stat_and_metadata_read deletes the session from inside a patched get_metadata_status, i.e. exactly in the window, and asserts the probe refuses. A companion control, test_probe_still_fails_open_on_legacy_metadata_without_created_at, pins the other empty so the fix cannot be "refuse on any empty". The first mutant I tried (disabling the new branch) was invalid -- it let the empty current fall into the current != known compare and refuse for the wrong reason, passing the test accidentally. Re-running against the exact pre-round-9 predicate (if current and current != known) isolates it cleanly: 1 failed, 15 passed, the single failure being the new test and the legacy control green either way.

TestSaveDoesNotResurrectDeletedSession is now 16/16; isort/flake8/mypy/black/docs-lint clean; 859 tests green across the dashboard-chat/fork/transfer modules. Spec paragraph in docs/system-specs/modules/history.md updated with the re-stat rule and with why the save needs no equivalent.

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Aug 29, 2026
@chenmingwei23

Copy link
Copy Markdown
Contributor Author

Advisory dispositions for head c318c3b:

Design Review CONCERNS (convention-enforced identity adoption; guard/probe fail-rule divergence) -- accepted-and-deferred. Both points are structural hardening of a shipped-and-tested invariant, not defects on this head: all four hydrate sites are armed (FP verified the count independently), and each fail-rule divergence is deliberate and pinned by a mutation-verified test (save defers so the flush retries; probe refuses so the copy is retried by its caller). The suggested adopt_disk_identity(slot, meta) helper plus a call-site pinning test is the right follow-up shape; recorded alongside the existing deferred simplification so the next change to these paths picks both up together. Not pushed now: the PR has converged over 9 review rounds and a behavior-neutral refactor re-rolls every lane.

First Principles CONCERNS (two of three saved consumers subsumed by the probe) -- accepted-and-deferred, and thanks for the sharpened count: conceding the pending-rewrite arm as the one real consumer improves on the round-7 analysis (previous disposition kept all three on the transient-unreadable distinction; the probe's fail-closed unreadable handling landed since, which is what collapses the other two). Same reasoning as above for not pushing the subtraction into this PR: behavior-identical, ~60-line churn, full lane re-roll. Both deferrals travel with the maintainer's follow-up list.

@chenmingwei23

Copy link
Copy Markdown
Contributor Author

/ai-review override gpt c318c3b: Unreachable trigger: created_at is datetime.now(utc).isoformat() with microsecond resolution (10k consecutive calls = 10k unique stamps), so the recreated file's stamp equals the original's only if creation, the session's whole observed lifetime, the delete, and the recreating append all land in one microsecond. The identity arm is also defense-in-depth behind the missing-file arm. The prescribed revert would reinstate the round-3 always-merge defect this hunk fixes.

@github-actions

Copy link
Copy Markdown
Contributor

Human judgment recorded

@chenmingwei23 marked the gpt AI finding as false positive, not applicable, or explicitly accepted for c318c3b89334d5dae11362505ecab7ab0a49b516.

Unreachable trigger: created_at is datetime.now(utc).isoformat() with microsecond resolution (10k consecutive calls = 10k unique stamps), so the recreated file's stamp equals the original's only if creation, the session's whole observed lifetime, the delete, and the recreating append all land in one microsecond. The identity arm is also defense-in-depth behind the missing-file arm. The prescribed revert would reinstate the round-3 always-merge defect this hunk fixes.

This decision applies only to this commit. A new push requires a new judgment.

@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Aug 29, 2026
@bolichen97
bolichen97 enabled auto-merge (squash) August 29, 2026 16:09

@bolichen97 bolichen97 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Reviewed and approved.

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running and removed readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention labels Aug 29, 2026
@bolichen97
bolichen97 merged commit 2214c7a into main Aug 29, 2026
114 of 118 checks passed
@github-actions github-actions Bot added the readiness: passed Eligible automated validation passed for the current revision label Aug 29, 2026
@bolichen97
bolichen97 deleted the fix/save-guard-deleted-session-6677 branch August 29, 2026 16:31
@github-actions github-actions Bot removed the readiness: passed Eligible automated validation passed for the current revision label Aug 29, 2026
@bolichen97

Copy link
Copy Markdown
Collaborator

Open PR relationship audit

This is a consolidated, point-in-time code-level audit note. It compares complete merge-base diffs and current/merged code; it does not treat a shared topic as duplication or partial coverage as completion.

Relationship findings

  • PR #4623 is PARTIALLY_COVERED relative to this PR. Coverage is explicitly incomplete; this finding is not a completion or closure claim. Recommended action for PR #4623: CONTINUE_DEVELOPMENT. The deferred race is now closed on main for every save path, which removes the only recorded blocker against the residual one-liner; the residual behaviour itself is untouched by 6707. Files: src/kiro_crew/dashboard/chat_persistence.py.

No PR, Issue, label, branch, or review state was changed by the relationship-note portion of this audit.

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.

A save can recreate a session whose permanent delete already committed

2 participants