fix: skip a history save whose session was permanently deleted under lock - #6707
Conversation
GPT 5.6 Review — ✅ human override acceptedHuman judgment by @chenmingwei23 overrides the GPT 5.6 finding for 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: |
Design Review (Fable 5) — 🟡 CONCERNSDesign-level review of 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
Suggestions
[DESIGN-REVIEWED] c318c3b |
First Principles Review (Fable 5) — 🟡 CONCERNSPremise-level review of All counts are in hand. Composing the review. First-Principles-Verdict: CONCERNS The bool-return channel is a second spelling: two of its three What this change shipsIntent: stop a lock-racing save (or a fork/transfer copy) from resurrecting a permanently deleted session — a FIX for #6677.
WatchThe description justifies the probe because "the copy paths cannot rely on their own flush arms observing" the Subtractions
[FIRST-PRINCIPLES-REVIEWED] c318c3b |
Opus 4.8 Review — ✅ no blocking findingsReviewed Review detailsBoth candidates hinge on a transient metadata-read failure ( Candidate 1 (spurious mismatch from compaction re-minting Candidate 2 (rewind/regenerate save raises and loses state): the raise is the intended fail-closed deferral, and No self-originated finding meets the (a)/(b)/(c) bar in the changed lines; the guard's fail-open on legacy/empty No findings. [OPUS-REVIEWED] c318c3b Verdict parsed from the review's SHA-scoped output markers for commit False positive or not applicable? A repository writer can comment: |
0262a30 to
fad578c
Compare
|
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 The prescribed remedy (re-mark the slot dirty before returning Instead, a shared |
fad578c to
74cee8b
Compare
|
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 |
74cee8b to
db48d9c
Compare
|
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. |
db48d9c to
c8f260b
Compare
|
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. |
|
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. What landed:
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 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 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). |
…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
3448f21 to
c318c3b
Compare
|
GPT round 9 finding ("Delete between stat and metadata read is missed", Verified real against the contract rather than by reading the call alone. The fix re-stats when
Scope, and why the sibling guard is untouched: this hole is exclusive to the deliberately lock-free probe. Verification, and a note on the mutant:
|
|
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 First Principles CONCERNS (two of three |
|
/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. |
Human judgment recorded@chenmingwei23 marked the gpt AI finding as false positive, not applicable, or explicitly accepted for
This decision applies only to this commit. A new push requires a new judgment. |
bolichen97
left a comment
There was a problem hiding this comment.
Reviewed and approved.
Open PR relationship auditThis 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
No PR, Issue, label, branch, or review state was changed by the relationship-note portion of this audit. |
Problem / Motivation
A session the user permanently deleted can reappear on disk in Older Sessions.
_save_slot_to_historytakes the per-session_lockedfor its whole read-modify-atomic_write, anddelete_sessionunlinks the file under the same lock -- but the save never re-checked, inside the lock, that the session still existed.save_slot_off_looproutes 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 tomkdir+atomic_writeand silently recreates the session the delete had already reported destroyed.delete_sessiondeliberately 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):
_disk_meta_created_at-- the metadatacreated_atof 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_atis carried forward by every save, so it never changes for a continuously-existing file: it is the file's identity. The window counters (_resumed_countetc.) 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._locked, before anymkdir/atomic_write: with a known identity, a missing file (statFileNotFoundErroronly) or an on-diskcreated_atthat 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_dirtyso the delete's reported success stands. The abort returnsFalse(every other completionTrue), threaded throughsave_slot_off_loop, so callers that must confirm durability can tell the skip from a committed write.get_metadata_status; a transiently unreadable line makes the save raise (deferral:_dirtystays armed, the flush retries) instead of blanking the identity comparison and overwriting a replacement session with deleted content.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 raisesSnapshotUnstable. Being lock-free, the probe can also have the delete land INSIDE it, between its stat and its metadata read:get_metadata_statusreports a vanished file as a genuine({}, True), so an emptycreated_atis 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 lockdelete_sessionunlinks under.delete_sessionon 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.mddocuments 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 returnTrueper the new contract.session_transfer.pyandchannel_slots.pywere 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:False); the_resumed_countarm; first save of a fresh slot still creates (control).created_atstill fails open while the file exists.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_authand 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