fix(persistence): only recover/discard may release unclaimed crash data

Addresses the PR #100 review round 2. Q#AS12's ownership rule guarded the
sweep but not the RELEASE paths, so three doors were still open.

The rule is now total: exactly two things may release an unclaimed
recovery file --- recover-file (which adopts it) and discard-recovery
(explicit user intent). Not a sweep, not a save, not a kill.

- HIGH: buffer.after-save called _discard_buffer unconditionally, which
  removed the live buffer's current-path key without checking ownership.
  Repro: session 1 autosaves and crashes; session 2 opens the file, does
  not recover, then saves --- the crash artifact was deleted. Same door
  was open on kill. discard_buffer now removes ONLY keys this session
  owns. The unclaimed copy survives (reported Stale, so never
  auto-offered, but still recoverable/discardable). The on-disk file holds
  the new work; the crash copy holds work never written anywhere, so
  deleting it was the same data loss by a different door.

- MEDIUM/LOW: _adopt only recorded the path in `owned`, not an
  association with the buffer. A removal callback fires after the buffer
  has left the registry, so discard_buffer had no path to read and no
  `written` entry to fall back on --- recover-then-kill leaked the copy
  and it was offered again. adopt now takes the BUFFER and records a
  `written` entry at the revision whose contents the file holds. That is
  correct twice over: the skip cache declines to rewrite an identical
  copy, and a kill can find and retire it.

- LOW: _discard(path) removed the file and unowned the hash but left
  matching `written` entries, so a still-dirty buffer hit the unchanged
  (path_hash, revision) fast path and went unprotected until its next
  edit. discard_path now clears those entries; the next sweep re-protects
  immediately.

Tests (autosave_acceptance now 24):
saving_without_recovering_preserves_unclaimed_crash_data,
killing_without_recovering_preserves_unclaimed_crash_data,
recover_then_kill_retires_the_adopted_recovery,
discard_recovery_lets_the_next_sweep_reprotect_immediately.

Gates: fmt + workspace clippy clean; lib 1499; crdt 1670; autosave 24;
desktop 11; persistence 5; GPU 58; git diff --check clean.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

builtin/runtime/autosave.lua

@@ -185,16 +185,20 @@ pmacs.command.define {
           return
         end
         buf:replace(0, buf:len(), bytes)
+        -- The crash data now lives in the buffer, so the copy is no
+        -- longer irreplaceable: claim it (Q#AS12). Claiming by BUFFER,
+        -- right after the replace, records the recovery under this
+        -- buffer's id at the revision whose contents it holds -- which
+        -- un-blocks autosave for the path AND lets a later kill retire
+        -- the copy (a removal callback runs after the buffer is gone,
+        -- with no path left to read).
+        pmacs.autosave._adopt(buf)
         -- The mutators notify windows and queue CRDT but do NOT fire
         -- `buffer.after-edit` --- that comes from dispatch_key's
         -- post-command revision check, which the minibuffer shadow
         -- returns before. Fire it so LSP didChange and the syntax
         -- reparse see the recovered contents.
         pmacs.hook.run("buffer.after-edit")
-        -- The crash data now lives in the buffer, so the copy is no
-        -- longer irreplaceable: claim it, which un-blocks autosave for
-        -- this path (Q#AS12).
-        pmacs.autosave._adopt(path)
         pmacs.editor.set_status("recovered from autosave --- save to keep it")
       end,
     }

docs/autosave-recovery-framing.md

@@ -262,17 +262,34 @@ the file, and start editing *before* running `recover-file`. The next
 sweep writes the current buffer to the same key — **destroying the crash
 copy**, which is precisely what autosave exists to protect.
 
-So the sweep tracks **ownership**. A per-session `owned` set records
-which path hashes *this session* wrote or adopted. A recovery file
-present at a key we do not own is unclaimed crash data:
+So autosave tracks **ownership**. A per-session `owned` set records which
+path hashes *this session* wrote or adopted. A recovery file at a key we
+do not own is unclaimed crash data, and the rule is total:
 
-- the sweep **refuses to write** that buffer and counts it as `blocked`;
-- `sweep()` surfaces *"autosave paused for N file(s) with unclaimed
-  recovery — M-x recover-file or M-x discard-recovery"*;
-- `recover-file` **adopts** the copy once its contents are installed in
-  the buffer (the crash data now lives in the buffer, so the file is no
-  longer irreplaceable), and `discard-recovery` removes it. Either action
-  resumes normal autosave for that path.
+> **Exactly two things may release an unclaimed recovery file:**
+> `recover-file` (which *adopts* it) and `discard-recovery` (explicit
+> user intent). Nothing else — not a sweep, not a save, not a kill.
+
+Concretely:
+
+- the **sweep refuses to write** that buffer, counts it `blocked`, and
+  surfaces *"autosave paused for N file(s) with unclaimed recovery — M-x
+  recover-file or M-x discard-recovery"*;
+- **`save` and `kill` delete only keys this session owns** (finding). You
+  reopen a crashed file, edit, and save without recovering: the on-disk
+  file now holds your new work, but the crash copy still holds work that
+  was *never written anywhere*. Deleting it would be the same data loss by
+  a different door. It survives — as `Stale`, so it is never auto-offered,
+  but it is still there to recover or discard.
+- `recover-file` **adopts by buffer**, not by path (finding). Adopt
+  records a `written` entry for that `BufferId` at the revision whose
+  contents the file now holds. That makes the skip cache correct *and*
+  lets a later kill retire the copy — a removal callback fires after the
+  buffer has left the registry, when there is no path left to read.
+- `discard-recovery` clears the matching `written` entries too (finding),
+  so a still-dirty buffer is re-protected on the very next sweep instead
+  of hitting the unchanged-`(path_hash, revision)` fast path and going
+  unprotected until its next edit.
 
 The trade is deliberate: while blocked, edits made *after* the reopen are
 not autosaved — and the user is told so, every sweep. Losing the new
@@ -444,6 +461,13 @@ commands, cleanup wiring) → tests.
   crashes with a recovery copy; session 2 reopens, edits, sweeps →
   `(written, blocked) == (0, 1)` and the crash copy is byte-identical.
   `_adopt` (what `recover-file` calls) or `_discard` resumes the sweep.
+- **…nor deleted by a save or a kill**: session 2 reopens, edits, and
+  saves (or kills) without recovering → the crash copy survives
+  byte-identical, now reported `Stale`.
+- **Recover then kill immediately** (before any save or sweep) → the
+  adopted copy *is* retired, not left to be re-offered.
+- **Explicit `discard-recovery` on a still-dirty buffer** → the next
+  sweep re-protects it at once, with no intervening edit.
 - **Path change**: rename a buffer's path (`set_buffer_path`) without
   editing it → next sweep writes the new key **and** removes the old
   recovery file (the `(path_hash, revision)` cache).

src/autosave.rs

@@ -356,55 +356,79 @@ pub fn sweep(lua: &Lua) -> Result<(usize, usize), String> {
     Ok((written, blocked))
 }
 
-/// Claim the recovery file at `path` for this session (Q#AS12), so
-/// subsequent sweeps may overwrite it. Called by `recover-file` once its
-/// contents are installed in the buffer — the crash data now lives in the
-/// buffer, so the copy is no longer irreplaceable.
-pub fn adopt(lua: &Lua, path: &Path) {
+/// Claim `buffer`'s recovery file for this session (Q#AS12). Called by
+/// `recover-file` once the contents are installed in the buffer — the
+/// crash data now lives in the buffer, so the copy is no longer
+/// irreplaceable.
+///
+/// It records a `written` entry as well as the ownership, because after a
+/// recover the file's contents *are* the buffer's contents. That makes
+/// two things right at once: the skip cache correctly declines to rewrite
+/// it, and `discard_buffer` can find and retire it — including from a
+/// removal callback that fires *after* the buffer is gone, when there is
+/// no path left to read (finding).
+pub fn adopt(lua: &Lua, id: BufferId) {
+    let Some(core) = lua.app_data_ref::<SharedCore>() else {
+        return;
+    };
+    let entry = {
+        let c = core.borrow();
+        let reg = c.registry.borrow();
+        let Ok(buf) = reg.get(id) else { return };
+        let Some(p) = buf.file_path() else { return };
+        (sha256_hex(&p.display().to_string()), buf.revision())
+    };
+    drop(core);
     if let Some(cache) = lua.app_data_ref::<AutosaveCache>() {
-        cache
-            .0
-            .borrow_mut()
-            .owned
-            .insert(sha256_hex(&path.display().to_string()));
+        let mut cache = cache.0.borrow_mut();
+        cache.owned.insert(entry.0.clone());
+        cache.written.insert(id, entry);
     }
 }
 
-/// Forget any claim on `path` (so a foreign recovery appearing there
-/// later still blocks a sweep).
-fn unown(lua: &Lua, path: &Path) {
-    if let Some(cache) = lua.app_data_ref::<AutosaveCache>() {
-        cache
-            .0
-            .borrow_mut()
-            .owned
-            .remove(&sha256_hex(&path.display().to_string()));
-    }
-}
-
-/// Delete the recovery file for `path` and drop any claim on it.
+/// Delete the recovery file for `path` and drop every claim and skip-cache
+/// entry pointing at it.
+///
+/// This is the **explicit** release path (`discard-recovery`), so it
+/// ignores ownership — the user asked. Clearing the matching `written`
+/// entries matters (finding): otherwise a still-dirty buffer would hit the
+/// unchanged-`(path_hash, revision)` fast path on the next sweep and go
+/// unprotected until its next edit.
 pub fn discard_path(lua: &Lua, path: &Path) -> bool {
     let Some(base) = base_dir(lua) else {
         return false;
     };
-    unown(lua, path);
+    let hash = sha256_hex(&path.display().to_string());
+    if let Some(cache) = lua.app_data_ref::<AutosaveCache>() {
+        let mut cache = cache.0.borrow_mut();
+        cache.owned.remove(&hash);
+        cache.written.retain(|_, (h, _)| h != &hash);
+    }
     discard(&base, path)
 }
 
 /// Retire the recovery copy of a specific **buffer** (Q#AS12).
 ///
 /// Keyed by `BufferId`, not by the path captured when the buffer loaded:
-/// it removes both the buffer's *current* path key (if it is still live)
-/// and the key its last recovery was actually **written** under. Those
-/// differ after a rename — an LSP `WorkspaceEdit` changes the path while
-/// the `BufferId` stays — and a path-captured callback would leave the
-/// real recovery file behind.
+/// it considers both the buffer's *current* path key (if it is still
+/// live) and the key its last recovery was actually **written** under.
+/// Those differ after a rename — an LSP `WorkspaceEdit` changes the path
+/// while the `BufferId` stays — and a path-captured callback would leave
+/// the real recovery file behind.
+///
+/// **Only keys this session owns are removed** (Q#AS12, finding). Saving
+/// or killing a buffer you reopened after a crash must *not* destroy the
+/// unclaimed recovery copy sitting at its path — you never recovered it.
+/// Only `recover-file` (which adopts) or an explicit `discard-recovery`
+/// releases unclaimed crash data.
 pub fn discard_buffer(lua: &Lua, id: BufferId) {
     let Some(base) = base_dir(lua) else {
         return;
     };
     let mut keys: Vec<String> = Vec::new();
-    // The key the last sweep actually wrote for this buffer.
+    // The key the last sweep (or an adopt) recorded for this buffer. This
+    // is the only source that still works once the buffer is gone — a
+    // removal callback fires after it has left the registry.
     if let Some(cache) = lua.app_data_ref::<AutosaveCache>()
         && let Some((hash, _)) = cache.0.borrow().written.get(&id)
     {
@@ -420,15 +444,20 @@ pub fn discard_buffer(lua: &Lua, id: BufferId) {
             keys.push(sha256_hex(&p.display().to_string()));
         }
     }
+    let Some(cache) = lua.app_data_ref::<AutosaveCache>() else {
+        return;
+    };
+    let mut cache = cache.0.borrow_mut();
+    keys.retain(|h| cache.owned.contains(h));
     for hash in &keys {
         let _ = crate::state::remove(&base, &format!("autosave/{hash}"));
+        cache.owned.remove(hash);
     }
-    if let Some(cache) = lua.app_data_ref::<AutosaveCache>() {
-        let mut cache = cache.0.borrow_mut();
+    // The skip-cache entry only ever names a key we owned, so it goes
+    // whenever we retired that key — and a buffer that owned nothing has
+    // no entry to drop.
+    if !keys.is_empty() {
         cache.written.remove(&id);
-        for hash in &keys {
-            cache.owned.remove(hash);
-        }
     }
 }
 

src/lua_bindings/mod.rs

@@ -1988,13 +1988,13 @@ fn install_autosave_module(lua: &Lua) -> mlua::Result<Table> {
         lua.create_function(|lua, ()| crate::autosave::sweep(lua).map_err(mlua::Error::external))?,
     )?;
 
-    // _adopt(path): claim a recovery file for this session, so later
-    // sweeps may overwrite it. `recover-file` calls this once the
-    // contents are safely installed in the buffer.
+    // _adopt(buf): claim a buffer's recovery file for this session, so
+    // later sweeps may overwrite it and a kill can retire it.
+    // `recover-file` calls this once the contents are installed.
     m.set(
         "_adopt",
-        lua.create_function(|lua, path: String| {
-            crate::autosave::adopt(lua, std::path::Path::new(&path));
+        lua.create_function(|lua, id: BufferIdLua| {
+            crate::autosave::adopt(lua, id.0);
             Ok(())
         })?,
     )?;

tests/autosave_acceptance.rs

@@ -280,10 +280,114 @@ fn sweep_never_overwrites_unclaimed_crash_recovery() {
     assert_eq!(status(&s2, &f), "fresh", "still offered to the user");
 
     // Once recover-file adopts it, autosave resumes for that path.
-    exec(&s2, &format!("pmacs.autosave._adopt({f:?})"));
+    exec(&s2, "pmacs.autosave._adopt(pmacs.window.buffer())");
+    // Adopt records the copy at the buffer's *current* revision, so the
+    // very next sweep sees no change; an edit makes it write again.
+    exec(&s2, "pmacs.window.buffer():insert(0, 'more ')");
     let (written, blocked) = sweep2(&s2);
     assert_eq!((written, blocked), (1, 0), "adopted → sweeps again");
-    assert_eq!(recovered(&s2, &f), b"new edits on disk\n");
+    assert_eq!(recovered(&s2, &f), b"more new edits on disk\n");
+    std::fs::remove_dir_all(&dir).ok();
+}
+
+#[test]
+fn saving_without_recovering_preserves_unclaimed_crash_data() {
+    let dir = fresh_state_dir();
+    // Session 1 crashes with unsaved work.
+    let s1 = editor(&dir);
+    let f = write_file(&dir, "a.txt", "on disk\n");
+    open_and_dirty(&s1, &f, "CRASH WORK ");
+    assert_eq!(sweep(&s1), 1);
+    let crash_copy = recovered(&s1, &f);
+
+    // Session 2 reopens, edits, and SAVES — without ever recovering or
+    // discarding. The save must not destroy the crash copy: only
+    // recover-file (adopt) or discard-recovery may release it.
+    let s2 = editor(&dir);
+    exec(&s2, &format!("pmacs.buffer.find_or_open({f:?})"));
+    exec(&s2, "pmacs.window.buffer():insert(0, 'new ')");
+    exec(&s2, "pmacs.command.invoke('buffer.save')");
+    assert_ne!(
+        status(&s2, &f),
+        "none",
+        "saving must not delete unclaimed crash data"
+    );
+    assert_eq!(
+        recovered(&s2, &f),
+        crash_copy,
+        "the crash recovery survives a save"
+    );
+    // It is now stale (the file changed on disk), so it is never
+    // auto-offered — but it is still there to recover or discard.
+    assert_eq!(status(&s2, &f), "stale");
+    std::fs::remove_dir_all(&dir).ok();
+}
+
+#[test]
+fn killing_without_recovering_preserves_unclaimed_crash_data() {
+    let dir = fresh_state_dir();
+    let s1 = editor(&dir);
+    let f = write_file(&dir, "a.txt", "on disk\n");
+    open_and_dirty(&s1, &f, "CRASH ");
+    assert_eq!(sweep(&s1), 1);
+    let crash_copy = recovered(&s1, &f);
+
+    let s2 = editor(&dir);
+    exec(&s2, &format!("_G.b = pmacs.buffer.find_or_open({f:?})"));
+    exec(&s2, "pmacs.buffer.kill(_G.b)");
+    assert_eq!(recovered(&s2, &f), crash_copy, "kill preserves it too");
+    std::fs::remove_dir_all(&dir).ok();
+}
+
+#[test]
+fn recover_then_kill_retires_the_adopted_recovery() {
+    let dir = fresh_state_dir();
+    let s1 = editor(&dir);
+    let f = write_file(&dir, "a.txt", "on disk\n");
+    open_and_dirty(&s1, &f, "crash ");
+    assert_eq!(sweep(&s1), 1);
+
+    // Reopen, recover, then kill immediately — before any save or sweep.
+    // The removal callback fires after the buffer is gone, so the only
+    // way to find the copy is the entry `_adopt` recorded for its id.
+    let s2 = editor(&dir);
+    exec(&s2, &format!("_G.b = pmacs.buffer.find_or_open({f:?})"));
+    exec(
+        &s2,
+        &format!(
+            "
+            local bytes = pmacs.autosave._recover_bytes({f:?})
+            local b = pmacs.window.buffer()
+            b:replace(0, b:len(), bytes)
+            pmacs.autosave._adopt(b)
+            "
+        ),
+    );
+    exec(&s2, "pmacs.buffer.kill(_G.b)");
+    assert_eq!(
+        status(&s2, &f),
+        "none",
+        "an adopted recovery is retired on kill, not left to be re-offered"
+    );
+    std::fs::remove_dir_all(&dir).ok();
+}
+
+#[test]
+fn discard_recovery_lets_the_next_sweep_reprotect_immediately() {
+    let dir = fresh_state_dir();
+    let s = editor(&dir);
+    let f = write_file(&dir, "a.txt", "body\n");
+    open_and_dirty(&s, &f, "mine ");
+    assert_eq!(sweep(&s), 1);
+    assert_eq!(sweep(&s), 0, "unchanged → skipped");
+
+    // Explicitly discard while the buffer is still dirty. The next sweep
+    // must re-create protection at once: a stale skip-cache entry would
+    // leave the buffer unprotected until its next edit.
+    exec(&s, &format!("pmacs.autosave._discard({f:?})"));
+    assert_eq!(status(&s, &f), "none");
+    assert_eq!(sweep(&s), 1, "protection restored without needing an edit");
+    assert_eq!(recovered(&s, &f), b"mine body\n");
     std::fs::remove_dir_all(&dir).ok();
 }