fix(persistence): one buffer owns a path's recovery slot (Q#AS13)

Addresses the PR #100 review round 3.

pmacs.buffer.from_file does not dedup, so two buffers can visit one path.
Ownership was tracked as a path-wide `owned: HashSet<path_hash>`, which
made the duplicate case silently corrupting: both dirty buffers queued a
write to autosave/<same hash>, the later write won on disk, and BOTH were
recorded in `written` --- so the loser skipped future sweeps while its
contents were unrecoverable. The path-wide set also let either buffer's
save/kill retire the other's recovery.

A recovery file must stay keyed by path (a later session knows only
paths, never old BufferIds), so two divergent buffers cannot both be
protected under one key. Ownership is now `owner: path_hash -> BufferId`:

- the first modified buffer to reach a free slot claims it, including
  within a single pass (the write loop updates `owner`, so the gather
  loop tracks slots queued this pass --- otherwise two duplicates both
  queue a write);
- any other buffer on that path is counted `conflicted` and reported
  ("autosave paused for N buffer(s): another buffer is visiting the same
  file"), never silently mis-protected. It records no `written` entry, so
  it re-attempts each sweep instead of believing itself saved;
- `discard_buffer` (save/kill) retires ONLY slots this buffer owns, which
  now enforces both invariants at once: an unowned slot is unclaimed
  crash data (Q#AS12), and a slot owned by another buffer is that
  buffer's recovery;
- saving or killing the owner releases the slot; the duplicate claims it
  on the next sweep;
- `recover-file` adopting into a buffer makes that buffer the owner --- the
  file's contents are now its contents, and the previous owner truthfully
  becomes conflicted.

sweep() now returns (written, blocked, conflicted). Its gather phase is
extracted into `gather()` (clippy too-many-lines).

This is honest rather than clever: pmacs cannot protect two divergent
buffers over one file, and now says so instead of pretending.

Tests (autosave_acceptance now 27):
duplicate_buffers_on_one_path_conflict_instead_of_corrupting (owner's
copy on disk; the dup never wins the slot by editing),
a_duplicate_buffers_save_does_not_retire_the_owners_recovery,
killing_the_owner_frees_the_slot_for_the_duplicate.

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

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

builtin/runtime/autosave.lua

@@ -48,19 +48,26 @@ function pmacs.autosave.interval_ms(ms)
   return interval
 end
 
--- sweep() --- force a pass now. Returns (written, blocked). `blocked`
--- counts buffers whose sweep was refused because an unclaimed crash
--- recovery sits at their key: overwriting it would destroy exactly what
--- autosave protects. Recovering or discarding that copy resumes autosave.
+-- sweep() --- force a pass now. Returns (written, blocked, conflicted).
+--   blocked    --- an unclaimed crash recovery sits at the buffer's key.
+--                  Overwriting it would destroy exactly what autosave
+--                  protects; recovering or discarding it resumes autosave.
+--   conflicted --- another buffer already owns that file's recovery slot.
+--                  Recovery files are keyed by path, so when two buffers
+--                  visit one file only the first can be protected.
 function pmacs.autosave.sweep()
-  if not enabled then return 0, 0 end
-  local written, blocked = pmacs.autosave._sweep()
+  if not enabled then return 0, 0, 0 end
+  local written, blocked, conflicted = pmacs.autosave._sweep()
   if blocked and blocked > 0 then
     pmacs.editor.set_status(
       "autosave paused for " .. blocked .. " file(s) with unclaimed recovery"
         .. " --- M-x recover-file or M-x discard-recovery")
+  elseif conflicted and conflicted > 0 then
+    pmacs.editor.set_status(
+      "autosave paused for " .. conflicted .. " buffer(s): another buffer"
+        .. " is visiting the same file")
   end
-  return written, blocked
+  return written, blocked, conflicted
 end
 
 local function basename(path)

docs/autosave-recovery-framing.md

@@ -296,6 +296,37 @@ not autosaved — and the user is told so, every sweep. Losing the new
 edits to a second crash is recoverable by retyping; losing the original
 crash copy is not.
 
+### Q#AS13 — One buffer owns a path's recovery slot
+
+`pmacs.buffer.from_file` does **not** dedup: a second buffer can visit an
+already-open path. The recovery file must stay keyed by path — a later
+session knows only paths, never old `BufferId`s — so two dirty duplicates
+cannot both be protected under one key. The naive behavior (finding) is
+the worst one: both write to the same key, the later write wins on disk,
+and *both* buffers are recorded as protected, so the loser silently skips
+future sweeps while its contents are unrecoverable. Either buffer's
+save/kill could also retire the other's copy.
+
+So ownership is `path_hash → BufferId`, not a path-wide set:
+
+- the **first** modified buffer to reach a free slot claims it (including
+  within a single sweep pass — the write loop updates `owner`, so the
+  gather loop tracks slots queued this pass);
+- any other buffer on that path is counted **`conflicted`** and reported —
+  *"autosave paused for N buffer(s): another buffer is visiting the same
+  file"* — never silently mis-protected. It records no `written` entry, so
+  it re-attempts each sweep rather than believing itself saved;
+- `discard_buffer` (save/kill) retires **only slots this buffer owns**, so
+  a duplicate cannot delete the owner's recovery;
+- when the owner is saved or killed, the slot is released and the
+  duplicate claims it on the next sweep;
+- `recover-file` adopting into a buffer makes *that* buffer the owner —
+  the file's contents are now its contents, and the previous owner
+  truthfully becomes conflicted.
+
+This is honest rather than clever: pmacs cannot protect two divergent
+buffers over one file, and says so.
+
 ### Q#AS7 — Cleanup lifecycle (keyed by buffer, not by a captured path)
 
 - **`buffer.after-save`** → `discard_buffer(active buffer)`.
@@ -468,6 +499,10 @@ commands, cleanup wiring) → tests.
   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.
+- **Two dirty buffers on one path (Q#AS13)** → `(written, blocked,
+  conflicted) == (1, 0, 1)`; the owner's copy is on disk; the duplicate
+  never wins the slot by editing, its save never retires the owner's
+  copy, and killing the owner frees the slot for it.
 - **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

@@ -18,7 +18,7 @@
 //! Framing: docs/autosave-recovery-framing.md.
 
 use std::cell::RefCell;
-use std::collections::{HashMap, HashSet};
+use std::collections::HashMap;
 use std::path::Path;
 
 use mlua::Lua;
@@ -139,16 +139,26 @@ struct CacheInner {
     /// remembers *where a buffer's recovery currently lives*, which is
     /// what makes cleanup work after a rename.
     written: HashMap<BufferId, (String, u64)>,
-    /// Path hashes whose recovery file **this session** wrote or has
-    /// adopted (Q#AS12).
+    /// Which buffer owns each recovery slot: `path_hash → BufferId`
+    /// (Q#AS12, Q#AS13).
     ///
-    /// A recovery file we did *not* write is unclaimed crash data. If the
-    /// user reopens the file and starts editing, sweeping would overwrite
-    /// the crash copy with the current buffer — destroying exactly what
-    /// autosave exists to protect. So an unowned recovery file *blocks*
-    /// the sweep for that buffer until `recover-file` adopts it or
-    /// `discard-recovery` removes it.
-    owned: HashSet<String>,
+    /// Two roles in one map:
+    ///
+    /// * **Absent** = the recovery file at that hash (if any) is
+    ///   *unclaimed crash data* — this session did not write it. Sweeping
+    ///   would overwrite the crash copy with the current buffer,
+    ///   destroying exactly what autosave protects. So it blocks the
+    ///   sweep until `recover-file` adopts it or `discard-recovery`
+    ///   removes it, and neither save nor kill may delete it.
+    /// * **Present** = the slot belongs to exactly *one* buffer. A
+    ///   recovery file is keyed by path (a later session knows only
+    ///   paths, never old `BufferId`s), but `pmacs.buffer.from_file` can
+    ///   open a *second* buffer on the same path. Both cannot be
+    ///   protected under one key: the later write would win on disk while
+    ///   both buffers believed themselves saved. So the first modified
+    ///   buffer claims the slot and any other buffer on that path is
+    ///   reported as conflicted, not silently mis-protected.
+    owner: HashMap<String, BufferId>,
 }
 
 /// Encode a header + contents into the one-file envelope.
@@ -238,26 +248,103 @@ struct Pending {
 /// Unlike desktop-save this is **not** daemon-gated — autosave is
 /// per-buffer, not per-frontend, and a daemon holds the unsaved work.
 ///
-/// Returns `(written, blocked)` — `blocked` counts buffers whose sweep
-/// was refused because an **unclaimed** recovery file already sits at
-/// their key (Q#AS12).
+/// Returns `(written, blocked, conflicted)`:
+///
+/// * `blocked` — an **unclaimed** recovery file already sits at the
+///   buffer's key (Q#AS12): crash data this session did not write.
+/// * `conflicted` — another buffer already owns that path's recovery
+///   slot (Q#AS13): two buffers visit the same file and only one can be
+///   protected under a path-keyed recovery file.
 ///
 /// # Errors
 /// A state-write failure. Individual buffers never abort the pass.
-pub fn sweep(lua: &Lua) -> Result<(usize, usize), String> {
+pub fn sweep(lua: &Lua) -> Result<(usize, usize, usize), String> {
     let Some(base) = base_dir(lua) else {
-        return Ok((0, 0));
+        return Ok((0, 0, 0));
     };
     let core = lua
         .app_data_ref::<SharedCore>()
         .ok_or("no editor core")?
         .clone();
+    let gathered = gather(lua, &core, &base)?;
+    let Gathered {
+        writes,
+        orphans,
+        live,
+        blocked,
+        conflicted,
+    } = gathered;
 
-    // Gather under a borrow; do all IO after releasing it.
+    let mut written = 0usize;
+    {
+        let cache = lua
+            .app_data_ref::<AutosaveCache>()
+            .ok_or("no autosave cache")?;
+        let mut cache = cache.0.borrow_mut();
+        // A buffer whose path moved leaves its old recovery behind.
+        for old in orphans {
+            let _ = crate::state::remove(&base, &format!("autosave/{old}"));
+            cache.owner.remove(&old);
+        }
+        // GC: a buffer that left the registry (killed) takes its recovery
+        // copy with it. This is the backstop that covers `[new file]`
+        // buffers, which fire no `after-load` and so never get a
+        // per-buffer removal callback registered. Only the slot's owner
+        // may retire it.
+        let dead: Vec<(BufferId, String)> = cache
+            .written
+            .iter()
+            .filter(|(id, _)| !live.contains(id))
+            .map(|(id, (hash, _))| (*id, hash.clone()))
+            .collect();
+        for (id, hash) in dead {
+            if cache.owner.get(&hash) == Some(&id) {
+                let _ = crate::state::remove(&base, &format!("autosave/{hash}"));
+                cache.owner.remove(&hash);
+            }
+            cache.written.remove(&id);
+        }
+        for p in writes {
+            let header = Header {
+                version: AUTOSAVE_VERSION,
+                path: p.path,
+                origin: p.origin,
+            };
+            let bytes = encode(&header, &p.contents)?;
+            crate::state::write_private(&base, &format!("autosave/{}", p.path_hash), &bytes)
+                .map_err(|e| e.to_string())?;
+            cache.owner.insert(p.path_hash.clone(), p.id);
+            cache.written.insert(p.id, (p.path_hash, p.revision));
+            written += 1;
+        }
+    }
+    Ok((written, blocked, conflicted))
+}
+
+/// What one pass of the registry decided, before any IO.
+struct Gathered {
+    writes: Vec<Pending>,
+    /// Recovery keys left behind by buffers whose path moved.
+    orphans: Vec<String>,
+    /// Every buffer still in the registry (drives the dead-buffer GC).
+    live: Vec<BufferId>,
+    blocked: usize,
+    conflicted: usize,
+}
+
+/// Walk the registry under a single borrow and decide what to write.
+/// All IO happens in [`sweep`] after this returns, because a recovery
+/// write must not run while the core is borrowed.
+fn gather(lua: &Lua, core: &SharedCore, base: &Path) -> Result<Gathered, String> {
     let mut writes: Vec<Pending> = Vec::new();
     let mut orphans: Vec<String> = Vec::new();
     let mut live: Vec<BufferId> = Vec::new();
     let mut blocked = 0usize;
+    let mut conflicted = 0usize;
+    // Slots claimed earlier in *this* pass. `owner` is only updated in the
+    // write loop, so without this two dirty duplicates of one path would
+    // both queue a write to the same key.
+    let mut queued: HashMap<String, BufferId> = HashMap::new();
     {
         let cache = lua
             .app_data_ref::<AutosaveCache>()
@@ -279,6 +366,31 @@ pub fn sweep(lua: &Lua) -> Result<(usize, usize), String> {
             let path_s = path.display().to_string();
             let path_hash = sha256_hex(&path_s);
             let revision = buf.revision();
+            // Exactly one buffer may own a path's recovery slot (Q#AS13):
+            // the file is keyed by path, so a second buffer on the same
+            // path cannot also be protected — the later write would win on
+            // disk while both believed themselves saved.
+            let slot_owner = cache
+                .owner
+                .get(&path_hash)
+                .or_else(|| queued.get(&path_hash));
+            match slot_owner {
+                Some(&owner_id) if owner_id != id => {
+                    conflicted += 1;
+                    continue;
+                }
+                Some(_) => {} // we already own the slot
+                None => {
+                    // Unowned. Never clobber unclaimed crash data (Q#AS12):
+                    // a recovery file this session did not write is the
+                    // crash copy the user has not recovered yet.
+                    if crate::state::exists(base, &format!("autosave/{path_hash}")).unwrap_or(false)
+                    {
+                        blocked += 1;
+                        continue;
+                    }
+                }
+            }
             if let Some((prev_hash, prev_rev)) = cache.written.get(&id) {
                 if prev_hash == &path_hash && *prev_rev == revision {
                     continue; // unchanged since its last copy
@@ -288,16 +400,7 @@ pub fn sweep(lua: &Lua) -> Result<(usize, usize), String> {
                     orphans.push(prev_hash.clone());
                 }
             }
-            // Never clobber unclaimed crash data (Q#AS12). A recovery file
-            // this session did not write is the crash copy the user has
-            // not recovered yet; overwriting it with the current buffer
-            // would destroy exactly what autosave protects.
-            if !cache.owned.contains(&path_hash)
-                && crate::state::exists(&base, &format!("autosave/{path_hash}")).unwrap_or(false)
-            {
-                blocked += 1;
-                continue;
-            }
+            queued.insert(path_hash.clone(), id);
             let len = buf.len();
             let mut contents = vec![0u8; usize::try_from(len).unwrap_or(0)];
             if len > 0 {
@@ -314,46 +417,13 @@ pub fn sweep(lua: &Lua) -> Result<(usize, usize), String> {
         }
     }
 
-    for old in orphans {
-        let _ = crate::state::remove(&base, &format!("autosave/{old}"));
-    }
-
-    let mut written = 0usize;
-    {
-        let cache = lua
-            .app_data_ref::<AutosaveCache>()
-            .ok_or("no autosave cache")?;
-        let mut cache = cache.0.borrow_mut();
-        // GC: a buffer that left the registry (killed) takes its recovery
-        // copy with it. This is the backstop that covers `[new file]`
-        // buffers, which fire no `after-load` and so never get a
-        // per-buffer removal callback registered.
-        let dead: Vec<(BufferId, String)> = cache
-            .written
-            .iter()
-            .filter(|(id, _)| !live.contains(id))
-            .map(|(id, (hash, _))| (*id, hash.clone()))
-            .collect();
-        for (id, hash) in dead {
-            let _ = crate::state::remove(&base, &format!("autosave/{hash}"));
-            cache.written.remove(&id);
-            cache.owned.remove(&hash);
-        }
-        for p in writes {
-            let header = Header {
-                version: AUTOSAVE_VERSION,
-                path: p.path,
-                origin: p.origin,
-            };
-            let bytes = encode(&header, &p.contents)?;
-            crate::state::write_private(&base, &format!("autosave/{}", p.path_hash), &bytes)
-                .map_err(|e| e.to_string())?;
-            cache.owned.insert(p.path_hash.clone());
-            cache.written.insert(p.id, (p.path_hash, p.revision));
-            written += 1;
-        }
-    }
-    Ok((written, blocked))
+    Ok(Gathered {
+        writes,
+        orphans,
+        live,
+        blocked,
+        conflicted,
+    })
 }
 
 /// Claim `buffer`'s recovery file for this session (Q#AS12). Called by
@@ -381,7 +451,12 @@ pub fn adopt(lua: &Lua, id: BufferId) {
     drop(core);
     if let Some(cache) = lua.app_data_ref::<AutosaveCache>() {
         let mut cache = cache.0.borrow_mut();
-        cache.owned.insert(entry.0.clone());
+        // Recovering into this buffer makes it the slot's owner — its
+        // contents are now what the file holds. Any previous owner of the
+        // slot (a duplicate buffer on the same path) loses the claim and
+        // will report as conflicted on the next sweep, which is truthful:
+        // the file no longer corresponds to it.
+        cache.owner.insert(entry.0.clone(), id);
         cache.written.insert(id, entry);
     }
 }
@@ -401,7 +476,7 @@ pub fn discard_path(lua: &Lua, path: &Path) -> bool {
     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.owner.remove(&hash);
         cache.written.retain(|_, (h, _)| h != &hash);
     }
     discard(&base, path)
@@ -448,18 +523,21 @@ pub fn discard_buffer(lua: &Lua, id: BufferId) {
         return;
     };
     let mut cache = cache.0.borrow_mut();
-    keys.retain(|h| cache.owned.contains(h));
+    // Retire only slots **this buffer** owns. Two guards in one check:
+    //   * an unowned slot is unclaimed crash data — saving or killing the
+    //     buffer you reopened after a crash must not destroy it (Q#AS12);
+    //   * a slot owned by a *different* buffer belongs to that buffer's
+    //     recovery — a duplicate buffer on the same path must not retire
+    //     it (Q#AS13).
+    keys.retain(|h| cache.owner.get(h) == Some(&id));
     for hash in &keys {
         let _ = crate::state::remove(&base, &format!("autosave/{hash}"));
-        cache.owned.remove(hash);
+        cache.owner.remove(hash);
     }
-    // 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() {
+    // This buffer's own bookkeeping goes regardless: it is being saved or
+    // killed, so any skip-cache entry for it is spent.
     cache.written.remove(&id);
 }
-}
 
 /// Every open file buffer that has a recovery file, with its status
 /// (Q#AS6). Enumerating in Rust is what makes this cover argv

tests/autosave_acceptance.rs

@@ -57,6 +57,21 @@ fn sweep2(s: &EditorState) -> (i64, i64) {
     eval(s, "return pmacs.autosave.sweep()")
 }
 
+/// Force a sweep; returns `(written, blocked, conflicted)`.
+fn sweep3(s: &EditorState) -> (i64, i64, i64) {
+    eval(s, "return pmacs.autosave.sweep()")
+}
+
+/// Open `path`, dirty it, then open a SECOND buffer on the same path via
+/// `from_file` (which does not dedup) and dirty that differently.
+/// Returns with the duplicate active.
+fn two_buffers_one_path(s: &EditorState, path: &str) {
+    exec(s, &format!("_G.a = pmacs.buffer.find_or_open({path:?})"));
+    exec(s, "pmacs.window.buffer():insert(0, 'AAA ')");
+    exec(s, &format!("_G.b = pmacs.buffer.from_file({path:?})"));
+    exec(s, "pmacs.window.buffer():insert(0, 'BBB ')");
+}
+
 fn recovered(s: &EditorState, path: &str) -> Vec<u8> {
     let b: mlua::String = eval(
         s,
@@ -290,6 +305,84 @@ fn sweep_never_overwrites_unclaimed_crash_recovery() {
     std::fs::remove_dir_all(&dir).ok();
 }
 
+#[test]
+fn duplicate_buffers_on_one_path_conflict_instead_of_corrupting() {
+    let dir = fresh_state_dir();
+    let s = editor(&dir);
+    let f = write_file(&dir, "a.txt", "on disk\n");
+    two_buffers_one_path(&s, &f);
+
+    // A recovery file is keyed by path, so only ONE of the two dirty
+    // buffers can be protected. The first claims the slot; the other is
+    // reported, never silently mis-protected.
+    let (written, blocked, conflicted) = sweep3(&s);
+    assert_eq!((written, blocked, conflicted), (1, 0, 1));
+    assert_eq!(
+        recovered(&s, &f),
+        b"AAA on disk\n",
+        "the slot's owner is what is on disk"
+    );
+
+    // The loser must NOT be marked protected: it keeps conflicting, and
+    // its contents never silently overwrite the owner's copy.
+    let (written, _, conflicted) = sweep3(&s);
+    assert_eq!(
+        (written, conflicted),
+        (0, 1),
+        "owner unchanged, dup still conflicts"
+    );
+    exec(&s, "pmacs.window.switch_buffer(_G.b)");
+    exec(&s, "pmacs.window.buffer():insert(0, 'more ')");
+    let (written, _, conflicted) = sweep3(&s);
+    assert_eq!(
+        (written, conflicted),
+        (0, 1),
+        "editing the dup does not win the slot"
+    );
+    assert_eq!(recovered(&s, &f), b"AAA on disk\n", "owner's copy intact");
+    std::fs::remove_dir_all(&dir).ok();
+}
+
+#[test]
+fn a_duplicate_buffers_save_does_not_retire_the_owners_recovery() {
+    let dir = fresh_state_dir();
+    let s = editor(&dir);
+    let f = write_file(&dir, "a.txt", "on disk\n");
+    two_buffers_one_path(&s, &f);
+    assert_eq!(sweep3(&s), (1, 0, 1));
+    let owner_copy = recovered(&s, &f);
+
+    // Save the DUPLICATE. Its cleanup must not touch the other buffer's
+    // recovery — that copy is the only record of the owner's unsaved work.
+    exec(&s, "pmacs.window.switch_buffer(_G.b)");
+    exec(&s, "pmacs.command.invoke('buffer.save')");
+    assert_ne!(status(&s, &f), "none", "the owner's recovery survives");
+    assert_eq!(recovered(&s, &f), owner_copy);
+    std::fs::remove_dir_all(&dir).ok();
+}
+
+#[test]
+fn killing_the_owner_frees_the_slot_for_the_duplicate() {
+    let dir = fresh_state_dir();
+    let s = editor(&dir);
+    let f = write_file(&dir, "a.txt", "on disk\n");
+    two_buffers_one_path(&s, &f);
+    assert_eq!(sweep3(&s), (1, 0, 1));
+
+    // Killing the owner retires its copy and releases the slot; the
+    // duplicate can then claim it and finally be protected.
+    exec(&s, "pmacs.buffer.kill(_G.a)");
+    assert_eq!(status(&s, &f), "none", "owner's copy retired with it");
+    let (written, blocked, conflicted) = sweep3(&s);
+    assert_eq!(
+        (written, blocked, conflicted),
+        (1, 0, 0),
+        "dup claims the slot"
+    );
+    assert_eq!(recovered(&s, &f), b"BBB on disk\n");
+    std::fs::remove_dir_all(&dir).ok();
+}
+
 #[test]
 fn saving_without_recovering_preserves_unclaimed_crash_data() {
     let dir = fresh_state_dir();