From b8a296f0a3a002703dba68cb90eb231bd99f5c20 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Thu, 9 Jul 2026 11:08:33 -0400 Subject: [PATCH] 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) --- builtin/runtime/autosave.lua | 12 ++-- docs/autosave-recovery-framing.md | 44 +++++++++--- src/autosave.rs | 99 +++++++++++++++++---------- src/lua_bindings/mod.rs | 10 +-- tests/autosave_acceptance.rs | 108 +++++++++++++++++++++++++++++- 5 files changed, 217 insertions(+), 56 deletions(-) diff --git a/builtin/runtime/autosave.lua b/builtin/runtime/autosave.lua index d66e15d..48bfff6 100644 --- a/builtin/runtime/autosave.lua +++ b/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, } diff --git a/docs/autosave-recovery-framing.md b/docs/autosave-recovery-framing.md index aaec4e6..2f0c879 100644 --- a/docs/autosave-recovery-framing.md +++ b/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). diff --git a/src/autosave.rs b/src/autosave.rs index 6c8e37e..205eee2 100644 --- a/src/autosave.rs +++ b/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::() 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::() { - 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::() { - 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::() { + 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 = 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::() && 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::() 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::() { - 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); - } } } diff --git a/src/lua_bindings/mod.rs b/src/lua_bindings/mod.rs index 25dbfb1..9625fdd 100644 --- a/src/lua_bindings/mod.rs +++ b/src/lua_bindings/mod.rs @@ -1988,13 +1988,13 @@ fn install_autosave_module(lua: &Lua) -> mlua::Result { 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(()) })?, )?; diff --git a/tests/autosave_acceptance.rs b/tests/autosave_acceptance.rs index 16f1d82..1177a34 100644 --- a/tests/autosave_acceptance.rs +++ b/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(); }