From e173f61a61992245f163925388a8777cdace0e1d Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Thu, 9 Jul 2026 11:36:55 -0400 Subject: [PATCH] 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`, which made the duplicate case silently corrupting: both dirty buffers queued a write to autosave/, 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) --- builtin/runtime/autosave.lua | 21 ++- docs/autosave-recovery-framing.md | 35 +++++ src/autosave.rs | 230 ++++++++++++++++++++---------- tests/autosave_acceptance.rs | 93 ++++++++++++ 4 files changed, 296 insertions(+), 83 deletions(-) diff --git a/builtin/runtime/autosave.lua b/builtin/runtime/autosave.lua index 48bfff6..c236aea 100644 --- a/builtin/runtime/autosave.lua +++ b/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) diff --git a/docs/autosave-recovery-framing.md b/docs/autosave-recovery-framing.md index 2f0c879..82a0818 100644 --- a/docs/autosave-recovery-framing.md +++ b/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). diff --git a/src/autosave.rs b/src/autosave.rs index 205eee2..39d4299 100644 --- a/src/autosave.rs +++ b/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, - /// 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, + /// 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, } /// 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::() .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::() + .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, + /// Recovery keys left behind by buffers whose path moved. + orphans: Vec, + /// Every buffer still in the registry (drives the dead-buffer GC). + live: Vec, + 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 { let mut writes: Vec = Vec::new(); let mut orphans: Vec = Vec::new(); let mut live: Vec = 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 = HashMap::new(); { let cache = lua .app_data_ref::() @@ -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::() - .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::() { 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::() { 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,17 +523,20 @@ 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); - } - // 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); + cache.owner.remove(hash); } + // 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 diff --git a/tests/autosave_acceptance.rs b/tests/autosave_acceptance.rs index 1177a34..7e044bd 100644 --- a/tests/autosave_acceptance.rs +++ b/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 { 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();