diff --git a/builtin/runtime/autosave.lua b/builtin/runtime/autosave.lua index c236aea..1728693 100644 --- a/builtin/runtime/autosave.lua +++ b/builtin/runtime/autosave.lua @@ -74,6 +74,29 @@ local function basename(path) return path:match("[^/]+$") or path end +-- The last sweep failure we reported, so a persistent fault (ENOSPC, a +-- read-only state dir) logs once but keeps warning in the status line. +local last_error = nil + +-- Run a sweep, surfacing any failure. `write_private` can fail --- a full +-- disk, a permission change, a clobbered state dir --- and this is a +-- data-protection feature: silently swallowing the error would leave the +-- user believing their work is safe when nothing is being written. +local function sweep_reporting() + local ok, err = pcall(pmacs.autosave.sweep) + if ok then + last_error = nil + return + end + local msg = "autosave FAILED: " .. tostring(err) + pmacs.editor.set_status(msg .. " --- your work is NOT being protected") + -- Log once per distinct fault; the status line keeps nagging every sweep. + if msg ~= last_error then + last_error = msg + if pmacs.error then pcall(pmacs.error, msg) end + end +end + -- One aggregate message however many files are recoverable. N synchronous -- `after-load` fires (a desktop restore) collapse into a single report. local function report_pending() @@ -106,7 +129,7 @@ pmacs.hook.add("process.after-tick", function() end if now - last_sweep_ms >= interval then last_sweep_ms = now - pcall(pmacs.autosave.sweep) + sweep_reporting() end end) @@ -135,10 +158,12 @@ pmacs.hook.add("buffer.after-save", function() end) -- A final synchronous sweep on quit: async ticks stop after this, so a --- quit with unsaved changes must capture them here. Returns nil -- --- before-quit is short-circuit and this must never veto. +-- quit with unsaved changes must capture them here. A failure here means +-- the quit is about to discard work that was never written anywhere, so +-- it is reported rather than swallowed. Returns nil -- before-quit is +-- short-circuit and this must never veto. pmacs.hook.add("editor.before-quit", function() - pcall(pmacs.autosave.sweep) + sweep_reporting() end) pmacs.command.define { diff --git a/docs/autosave-recovery-framing.md b/docs/autosave-recovery-framing.md index 82a0818..093d37a 100644 --- a/docs/autosave-recovery-framing.md +++ b/docs/autosave-recovery-framing.md @@ -327,6 +327,31 @@ So ownership is `path_hash → BufferId`, not a path-wide set: This is honest rather than clever: pmacs cannot protect two divergent buffers over one file, and says so. +The bookkeeping invariant that makes it safe is +**`written[id] ⟹ owner[hash] == id`**: a skip-cache entry only ever names +a slot its buffer owns. `adopt` is the one operation that transfers a +slot, so it drops the previous owner's entry (finding). Without that: +adopt into B, then kill B without saving — `discard_buffer` frees the slot +and deletes the file, but A's stale `written[A] = (hash, revA)` survives, +so the next sweep sees A dirty at an unchanged revision, calls it +"unchanged since its last copy", and leaves it **unprotected** until its +next edit. + +### Q#AS14 — A failing sweep is loud + +`write_private` can fail: a full disk, a permission change, a clobbered +state dir. Swallowing that (`pcall(...)` and drop the error, finding) +is the worst possible behavior for a data-protection feature — the user +keeps working, believing their edits are being captured, while nothing is +written. + +So both the tick and the `before-quit` sweep go through a reporting +wrapper: the status line says *"autosave FAILED: … — your work is NOT +being protected"* on every failing sweep, and each distinct fault is +logged once via `pmacs.error`. The quit path reports too — a failure +there means the quit is about to discard work that was never written +anywhere — and still never vetoes. + ### Q#AS7 — Cleanup lifecycle (keyed by buffer, not by a captured path) - **`buffer.after-save`** → `discard_buffer(active buffer)`. @@ -503,6 +528,12 @@ commands, cleanup wiring) → tests. 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. +- **Adopt transfers the slot cleanly**: A owns, B adopts, B is killed + unsaved → the freed slot lets the *next* sweep re-protect the still-dirty + A with no intervening edit (the `written ⟹ owner` invariant). +- **A failing sweep is reported (Q#AS14)**: with `autosave/` unwritable, + `sweep()` raises rather than returning `0`, and `before-quit` surfaces + *"autosave FAILED … NOT being protected"* while still not vetoing quit. - **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 39d4299..d55e0b5 100644 --- a/src/autosave.rs +++ b/src/autosave.rs @@ -451,13 +451,25 @@ pub fn adopt(lua: &Lua, id: BufferId) { drop(core); if let Some(cache) = lua.app_data_ref::() { let mut cache = cache.0.borrow_mut(); + let (hash, revision) = entry; // 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); + // + // Dropping the old owner's skip-cache entry maintains the + // invariant `written[id] ⟹ owner[hash] == id` (finding). Without + // it: adopt into B, then kill B without saving. `discard_buffer` + // frees the slot and deletes the file, but A's stale + // `written[A] = (hash, revA)` survives — so the next sweep sees A + // dirty at an unchanged revision, calls it "unchanged since its + // last copy", and leaves it unprotected until its next edit. + cache + .written + .retain(|&other, (h, _)| other == id || h != &hash); + cache.owner.insert(hash.clone(), id); + cache.written.insert(id, (hash, revision)); } } diff --git a/tests/autosave_acceptance.rs b/tests/autosave_acceptance.rs index 7e044bd..4567f96 100644 --- a/tests/autosave_acceptance.rs +++ b/tests/autosave_acceptance.rs @@ -383,6 +383,66 @@ fn killing_the_owner_frees_the_slot_for_the_duplicate() { std::fs::remove_dir_all(&dir).ok(); } +#[test] +fn adopting_clears_the_previous_owners_stale_skip_cache() { + 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 owns the slot; B is the conflicted duplicate. + assert_eq!(sweep3(&s), (1, 0, 1)); + assert_eq!(recovered(&s, &f), b"AAA on disk\n"); + + // B recovers (adopts), stealing the slot. A keeps its dirty contents. + exec(&s, "pmacs.window.switch_buffer(_G.b)"); + exec(&s, "pmacs.autosave._adopt(pmacs.window.buffer())"); + + // Now kill B without saving: the slot is freed and its file deleted. + exec(&s, "pmacs.buffer.kill(_G.b)"); + assert_eq!(status(&s, &f), "none"); + + // A is still dirty and now unprotected. The next sweep must write it. + // A stale `written[A]` (same hash, same revision) would make the skip + // cache call it "unchanged since its last copy" and leave it exposed. + let (written, blocked, conflicted) = sweep3(&s); + assert_eq!( + (written, blocked, conflicted), + (1, 0, 0), + "the old owner is re-protected once the slot frees, without an edit" + ); + assert_eq!(recovered(&s, &f), b"AAA on disk\n"); + std::fs::remove_dir_all(&dir).ok(); +} + +#[test] +fn a_failing_sweep_is_reported_not_swallowed() { + let dir = fresh_state_dir(); + // Plant a regular file where the `autosave/` directory must go, so + // every recovery write fails (stands in for ENOSPC / a read-only + // state dir). + std::fs::write(dir.join("autosave"), b"not a directory").unwrap(); + + let s = editor(&dir); + let f = write_file(&dir, "a.txt", "body\n"); + open_and_dirty(&s, &f, "precious "); + + // The raw sweep surfaces the error rather than returning 0 silently. + let ok: bool = eval(&s, "return (pcall(pmacs.autosave.sweep))"); + assert!(!ok, "a write failure must not look like a successful sweep"); + + // And the quit path reports it instead of swallowing it — a failure + // there means the quit is about to discard unprotected work. + s.core.borrow_mut().status.clear(); + let not_vetoed: bool = eval(&s, "return pmacs.hook.run('editor.before-quit')"); + assert!(not_vetoed, "reporting must still never veto quit"); + let status = s.core.borrow().status.clone(); + assert!( + status.contains("autosave FAILED") && status.contains("NOT being protected"), + "the failure is surfaced: {status:?}" + ); + std::fs::remove_dir_all(&dir).ok(); +} + #[test] fn saving_without_recovering_preserves_unclaimed_crash_data() { let dir = fresh_state_dir();