fix(persistence): adopt clears the old owner's skip cache; failing sweeps are loud
Addresses the PR #100 review round 4. - MEDIUM stale skip-cache entry after a slot transfer. adopt() set owner[hash] = new buffer but left the previous owner's `written` entry pointing at the same hash, breaking the invariant `written[id] => owner[hash] == id`. Repro: A and B are duplicate buffers on one path; A owns the slot; B adopts (recover-file); B is killed without saving, which frees the slot and deletes the file. A is still dirty, but its stale written[A] = (hash, revA) makes the next sweep call it "unchanged since its last copy" --- silently unprotected until its next edit. adopt() now drops any other buffer's written entry for that hash. Verified the new test fails without the fix (sweep writes 0). - MEDIUM autosave write failures were swallowed. write_private can fail (ENOSPC, a permission change, a clobbered state dir), but the tick and before-quit paths did `pcall(sweep)` and dropped the error. For a data-protection feature that is the worst failure mode: the user keeps working, believing edits are captured, while nothing is written. Both paths now go through a reporting wrapper --- status line "autosave FAILED: ... --- your work is NOT being protected" on every failing sweep, each distinct fault 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. Tests (autosave_acceptance now 29): adopting_clears_the_previous_owners_stale_skip_cache, a_failing_sweep_is_reported_not_swallowed (plants a regular file where autosave/ must be a directory, standing in for ENOSPC). Gates: fmt + workspace clippy clean; lib 1499; crdt 1670; autosave 29 + 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>
This commit is contained in:
parent
e173f61a61
commit
c80e00e799
|
|
@ -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 {
|
||||
|
|
|
|||
|
|
@ -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).
|
||||
|
|
|
|||
|
|
@ -451,13 +451,25 @@ 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();
|
||||
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));
|
||||
}
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -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();
|
||||
|
|
|
|||
Loading…
Reference in New Issue