From a6d8c60faf75915caefcd20d7da409f09799ebed Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Thu, 9 Jul 2026 10:10:06 -0400 Subject: [PATCH 1/6] docs: autosave + crash-recovery framing (Arc 3 phase 3) Q#AS1-11: hybrid (Rust sweep+guard, Lua cadence/config/UX); process.after-tick + monotonic_ms cadence (no parked worker thread, live-reconfigurable interval); validated-setter config (pmacs.autosave.interval_ms, default 30s, 1s floor); one atomic header-line+bytes envelope with NULLABLE origin meta (new files); Fresh/Stale/Corrupt/None status table; pull-based aggregated notify on the tick (covers argv [new file] buffers that fire no hook); explicit hook.run(buffer.after-edit) after recover-file's replace; path-aware (path_hash, revision) skip cache; shared sha256_hex extraction; private 0700/0600 storage as a precondition for default-on. Incorporates the 2026-07-09 review. Co-Authored-By: Claude Opus 4.8 (1M context) --- docs/autosave-recovery-framing.md | 424 ++++++++++++++++++++++++++++++ 1 file changed, 424 insertions(+) create mode 100644 docs/autosave-recovery-framing.md diff --git a/docs/autosave-recovery-framing.md b/docs/autosave-recovery-framing.md new file mode 100644 index 0000000..9a33ea7 --- /dev/null +++ b/docs/autosave-recovery-framing.md @@ -0,0 +1,424 @@ +# Autosave + crash recovery — framing (Arc 3 phase 3) + +Kill pmacs mid-edit and the unsaved work is gone. **Autosave** writes a +recovery copy of each modified file buffer on a configurable interval; +**crash recovery** notices that copy on next open and lets you restore +it. Emacs's `auto-save-mode` + `recover-file`. + +Closes the persistence arc: phase 1 (PR #98) gave the `pmacs.state` +confined store and `state.remove`; phase 2 (PR #99) gave the all-Rust +`pmacs.session.*` precedent and `get_or_load_buffer`. Parent decision: +`docs/persistence-framing.md` Q#PS8. + +## Ground truth (scouted; file:line as of `a0a4e7f`) + +- **Timers advance while idle.** Both run loops block on a *frame + timeout*, not on input, and fall through to `tick_async` regardless + (`src/editor.rs:1619,1643-1646`; `src/daemon.rs:1273,1304,1312-1314`). + So a periodic Lua loop keeps running when nobody is typing. +- **But `workers.sleep` parks a pool thread** for its full duration + (`src/async_runtime.rs:724-733,1277-1290`), and the pool is only + `available_parallelism - 1` (`:566-602`). A 30-second sleep would hold + a worker hostage. `process.after-tick` (`builtin/hooks/default.lua:70`, + fired every frame from `src/editor.rs:411-420`) plus + `pmacs.editor.monotonic_ms()` (`src/lua_bindings/mod.rs:10804`) is the + zero-thread alternative — already the LSP-debounce idiom + (`builtin/runtime/lsp.lua:299`). +- **Lua cannot see a non-active buffer's path.** `BufferIdLua` exposes + `:len() :name() :is_modified() :is_valid() :slice()` and the mutators + (`src/lua_bindings/mod.rs:1156-1235`) — **no `:file_path()`**. The only + path getter is `pmacs.editor.file_path()` (active buffer, + `:10902`). Contents *are* readable for any buffer (`:slice` resolves by + id). Rust has everything: `registry.ids()`, `Buffer::file_path()` + (`src/buffer.rs:263`), `is_modified()` (`:452`), and the whole-buffer + byte snapshot `save()` already uses (`src/editor_core.rs:1246-1250`). +- **`FileMeta`** (`src/file_io.rs:47-62`, `{mtime: SystemTime, size: + u64}`, `PartialEq`) is **not serde and not exposed to Lua**. + `current_meta(path)` exists (`:66`); each buffer stores its load/save + meta (`src/buffer.rs:194,274-280`). **Nothing compares them today** — + `EditorCore::save()` overwrites unconditionally (`:1231-1268`), so the + external-change guard is new code. +- **No `buffer.before-load` seam** — contents are installed, *then* + `after-load` fires. Substitution must happen after the fact via + `buf:replace(0, buf:len(), bytes)` (`:1216-1234`). +- **`after-load` fires once per restored leaf during desktop-restore** + (phase 2, `src/desktop.rs`). A modal prompt there would stack N + prompts. There is also **no `y_or_n`/confirm helper** — only the + callback-driven `pmacs.minibuffer.read` (`:11040`). +- **Cleanup seams exist**: `buffer.after-save` hook + (`builtin/hooks/default.lua:57`, active buffer), per-buffer + `pmacs.buffer.on_removed(id, fn)` (`:2695-2718` — there is **no** + global kill hook), and `editor.before-quit` already has two listeners + (multiple listeners are fine). +- **`pmacs.state.remove(name)`** confirmed (`:2073`). But + **`state::read` returns `String`** (`read_to_string`, `src/state.rs`) + — recovery contents are arbitrary bytes, so a `read_bytes` is needed. +- **State files are not private.** `state::write` does a plain + `create_dir_all` (`src/state.rs:186`, default `0755`), and + `save_atomic` only preserves the mode of an **existing** target + (`src/file_io.rs:143-145`) — a *new* file gets the umask default, + typically `0644`. Autosave stores **unsaved file contents**, not + metadata; world-readable recovery copies could be more exposed than + the original file. +- **"New file" buffers have no origin meta and fire no hook.** Opening a + missing path sets `file_path` but leaves `file_meta` unset and + `fire_after_load = false` (`src/editor.rs:512-519`). And Lua's + `from_file`/`find_or_open` **error** on a missing path + (`src/lua_bindings/mod.rs:2488,2551`) — so a `[new file]` buffer only + ever arrives via argv `EditorState::open`, which fires *nothing*. +- **`buf:replace` does not fire `buffer.after-edit`.** The mutators call + `notify_buffer_edit_to_windows` (windows + CRDT queue only, + `src/lua_bindings/mod.rs:1374-1385`). `after-edit` is fired solely by + `dispatch_key`'s post-command revision check (`src/editor.rs:739`) and + the modal shadows that return before it (`:887,:922`). The minibuffer + shadow is one of those — so an edit made inside an `M-x` command body + is invisible to LSP/syntax. `pmacs.hook.run(name)` *is* public + (`src/lua_bindings/mod.rs:4962`), the escape hatch + `builtin/commands/default.lua:226` already uses. +- **`sha256_hex` is duplicated privately twice** (`src/desktop.rs:144`, + `src/packages/fetcher.rs:517`). A third copy would be wrong. +- **There is no configuration system.** No `pmacs.config`, no options + table, no defcustom registry; `src/config.rs` only loads `init.lua`. + The convention is an ad-hoc *validated setter*: + `pmacs.async_config.frame_target_ms(ms)` + (`builtin/runtime/async.lua:458-466`) and `fs.watch{interval_ms}` + (`builtin/runtime/fs.lua:237-241`) both do + getter-when-nil / type-check / `>= 1` / `math.floor`. + +## Decisions + +### Q#AS1 — Hybrid: Rust owns the sweep + the guard; Lua owns cadence, config, UX + +The same split phase 2 landed on, forced by the same two gaps: Lua can't +read a non-active buffer's path, and `FileMeta` is neither Lua-visible +nor serde. So: + +- **Rust (`src/autosave.rs`)**: `sweep()` (walk the registry, write a + recovery file per modified file buffer), `status(path)` (the + external-change guard), `recover_bytes(path)`, `discard(path)`. +- **Lua (`builtin/runtime/autosave.lua`)**: the timer, the interval and + enable knobs, the after-load notification, the `recover-file` / + `discard-recovery` commands, and the save/kill/quit cleanup wiring. + +Notably this needs **no new Lua per-buffer path getter** — the sweep +never leaves Rust, and every Lua cleanup seam (`after-save`, +`on_removed`, `after-load`) is either active-buffer or captures the path +at registration. + +### Q#AS2 — Cadence: `process.after-tick` + `monotonic_ms`, not `workers.sleep` + +Three reasons, in order of weight: +1. **No worker thread is parked.** A long `workers.sleep` holds one of + `available_parallelism - 1` pool threads for the whole interval. +2. **The interval becomes live-reconfigurable for free** — the handler + re-reads it each tick, so `interval_ms(60000)` takes effect + immediately. A sleeping timer would ignore the change until it woke. +3. It matches the existing debounce idiom (`lsp.lua`). + +The handler is: bail if disabled; `monotonic_ms()`; if +`now - last >= interval` then `last = now` and sweep. Every frame this +costs one clock read and a compare. Wrapped in `pcall` — +`process.after-tick` is `all-must-succeed`, and a sweep error must not +poison the chain. + +The sweep itself is **synchronous on the main thread** (a `save_atomic` +per dirty buffer). Bounded by Q#AS8's skip rules; offloading large-buffer +writes to a worker is deferred. + +### Q#AS3 — Configuration (the interval) + +pmacs has **no config registry**, so this follows the established +validated-setter convention rather than inventing one: + +```lua +pmacs.autosave.interval_ms() -- getter → current value +pmacs.autosave.interval_ms(60000) -- setter, validated +pmacs.autosave.enable(false) -- disable knob +pmacs.autosave.sweep() -- force a sweep now (manual/test) +``` + +`interval_ms(ms)`: returns the current value when `ms` is nil; otherwise +requires a `number`, rejects `< MIN_INTERVAL_MS` (**1000**, since each +sweep `fsync`s), applies `math.floor`, and errors on anything else — +byte-for-byte the shape of `frame_target_ms`. **Default 30_000 ms** +(Emacs's `auto-save-timeout`). Changes apply on the next tick (Q#AS2). + +Tests drive `sweep()` directly rather than waiting on a timer, so the +1-second floor never makes the suite slow. + +> **Bigger picture, flagged not built:** the absence of any config +> registry is itself a gap. Real configurability — typed, validated, +> introspectable, defaulted, `M-x customize`-able options — is an arc of +> its own. `interval_ms`/`enable` are deliberately shaped as +> get-or-set-with-validation so they can be *migrated into* such a +> registry later without changing call sites. Not in this PR. + +### Q#AS4 — Recovery file: one atomic file, header line + raw bytes + +Key: `autosave/` (lowercase hex passes +`state::validate_name`'s `[A-Za-z0-9._-]` charset). + +**One file, not a contents/sidecar pair.** A pair is two writes: a crash +between them leaves contents without meta (or vice versa). Instead a +single atomic write of: + +``` +\n +``` + +The header is `{version, path, origin: null | {mtime_secs, mtime_nanos, +size}}` — `FileMeta` hand-serialized, since it is not serde +(`SystemTime` → `duration_since(UNIX_EPOCH)`). + +**`origin` is nullable** (finding). A `[new file]` buffer — a path that +does not exist on disk yet — has no `file_meta`, and its unsaved +contents are exactly the work most worth recovering. Requiring an origin +meta would have silently excluded it. `origin: null` means "there was no +file on disk when this was autosaved." + +Contents may contain newlines and non-UTF-8 bytes; the reader splits at +the **first** `\n` only. This needs a Rust-only **`state::read_bytes`** +(today's `state::read` is `read_to_string`, which would fail on non-UTF-8 +buffer contents). + +### Q#AS5 — Recovery status: the external-change guard + +`status(path)` reads the envelope and compares `header.origin` against +`current_meta(path)`: + +| `header.origin` | on disk now | status | meaning | +|---|---|---|---| +| `Some(m)` | exists, meta `== m` | **`Fresh`** | disk untouched; recovery is newer | +| `Some(m)` | exists, meta `!= m` | **`Stale`** | file changed externally | +| `Some(m)` | missing | **`Stale`** | the base file was deleted | +| `None` (new file) | missing | **`Fresh`** | still a new file; nothing to conflict with | +| `None` (new file) | exists | **`Stale`** | someone created the file meanwhile | +| unparseable / bad version | — | **`Corrupt`** | never offered; discardable | +| no file | — | **`None`** | | + +Only **`Fresh`** is announced (Q#AS6). **`Stale`** is never auto-offered +— silently clobbering a file someone else changed is the one +unrecoverable mistake here; `recover-file` will still recover it, but +says so plainly and requires confirmation. **`Corrupt`** is a typed +status, not an error (finding): a malformed envelope must not make +startup noisy or break the commands. It is counted separately, never +offered, and `discard-recovery` removes it. + +### Q#AS6 — Notify (aggregated, on the tick), don't prompt + +Recovery **must not** open a modal minibuffer prompt from `after-load`: +desktop-restore fires `after-load` once per restored leaf (phase 2), so a +prompt would stack N modals mid-restore — and no `y_or_n` helper exists +to build one cleanly anyway. + +But a per-`after-load` **status message** is also wrong (finding): N +restored leaves would each overwrite `core.status`, so only the last +recoverable file is ever mentioned. And it would miss `[new file]` +buffers entirely, which fire no hook at all (ground truth). + +So the report is **pull-based and aggregated on the tick we already own** +(Q#AS2): + +- Rust `pmacs.autosave.pending()` → for **every open file buffer**, + the `status(path)`; returns the `Fresh` paths (and a `Corrupt` count). + Enumerating buffers in Rust is what makes this cover argv `[new file]` + buffers and desktop-restored buffers uniformly, with no hook at all. +- Lua sets a `needs_report` flag on module load (the startup scan) and on + `buffer.after-load` (runtime opens). The **tick handler** — not the + hook — does the reporting: if flagged, call `pending()` once, emit a + single aggregate status, clear the flag. N synchronous `after-load` + fires during a restore therefore collapse into **one** message: + *"3 files have autosave recovery — M-x recover-file"* (or the filename + when there is exactly one). +- `pending()` runs one `stat` per open file buffer, only when flagged — + never per frame. + +Recovery itself happens through an explicit command: + +- **`recover-file`** — confirms via `minibuffer.read` (typed `yes`), then + `buf:replace(0, buf:len(), recovery_bytes)` on the active buffer, + **then explicitly `pmacs.hook.run("buffer.after-edit")`**. That last + step is load-bearing (finding): the mutators only notify windows and + queue CRDT, and `after-edit` is fired by `dispatch_key`'s post-command + check — which the minibuffer shadow returns before. Without the + explicit fire, LSP `didChange` and the syntax reparse would never see + the recovered contents. (Both read the *active* buffer, which is + exactly the one `recover-file` operates on.) + The replace leaves the buffer **modified** — the user must save to + accept, which is what deletes the recovery file (Q#AS7). +- **`discard-recovery`** — delete the recovery file for the active file, + whatever its status (including `Corrupt`). + +This also sidesteps re-entrancy: no modal surface is opened from inside a +hook fired by Rust. + +### Q#AS7 — Cleanup lifecycle + +- **`buffer.after-save`** → `discard(active path)`. A clean save means the + recovery copy is obsolete. (Hook exists, active-buffer, no args needed.) +- **Buffer killed** → `discard(path)`. There is no global kill hook, so + `after-load` registers a per-buffer `pmacs.buffer.on_removed(id, fn)` + closing over the path (captured then, since the buffer may be gone when + the callback runs). Killing a modified buffer is a deliberate discard. +- **`editor.before-quit`** → one **final synchronous sweep**, then return + nil (never veto). Async ticks stop after quit, so this must be a direct + call. Result: quitting with unsaved changes leaves a recovery copy that + the next open notices — which is exactly the point. +- Recovery files for buffers never reopened linger. Orphan GC is + deferred. + +### Q#AS8 — What gets swept, and the cost bound + +Only buffers with `file_path().is_some() && is_modified()` — which +**includes `[new file]` buffers** (path set, no origin meta, Q#AS4). +Scratch and `*special*` buffers are skipped (deferred). Two skips keep +the main-thread cost down: + +1. If no buffer qualifies, the sweep returns immediately (no IO). +2. Skip a buffer whose contents are unchanged since its last successful + autosave. Without this, a 30-second interval re-`fsync`s an + idle-but-dirty buffer forever. + +**The skip cache is keyed `BufferId → (path_hash, revision)`, not +`BufferId → revision`** (finding). A buffer keeps its `BufferId` across a +path change (LSP `WorkspaceEdit` rename calls `set_buffer_path`), so a +revision-only cache would skip the write, never create the recovery file +under the *new* key, and orphan the old one. On sweep, a `path_hash` +mismatch counts as changed: write the new key **and** `discard` the old +one. + +### Q#AS9 — Extract the duplicated `sha256_hex` + +Two private copies exist (`desktop.rs`, `packages/fetcher.rs`); autosave +needs a third. Instead extract one `pub(crate) fn sha256_hex` into a +small `src/hash.rs` and point all three at it. In-scope cleanup, not a +drive-by: the alternative is knowingly adding the third copy. + +### Q#AS11 — Private storage (a **precondition** for default-on) + +Autosave stores **unsaved file contents** — a different class of secret +from saveplace's cursor offsets or recentf's path list. Today +`state::write` creates parents with a default `0755` and `save_atomic` +gives a *new* file the umask default (typically `0644`), preserving mode +only for an already-existing target. A recovery copy of an unsaved edit +to a `0600` file would land world-readable — **more exposed than the +original** (finding). + +So this PR makes autosave storage private: + +- **`file_io::save_atomic_with_mode(path, content, mode)`** — sets the + temp file's permissions **before** the rename, so the target is never + momentarily visible at `0644`. (A chmod-after-write leaves exactly that + window.) Plain `save_atomic` delegates with `None`. +- **`state::write_private(base, name, content)`** — creates the parent + with `DirBuilder::mode(0o700)` and writes the file `0600`. +- Recovery files use it; the `autosave/` directory is `0700`. +- Unix-only (`PermissionsExt` / `DirBuilderExt` are safe under + `#![forbid(unsafe_code)]`); on other platforms it degrades to today's + behavior, documented. + +Retention and the disable knob are documented in the same breath: files +live under `$XDG_STATE_HOME/pmacs/autosave/`, are deleted on save/kill, +survive a crash or an unsaved quit, and `pmacs.autosave.enable(false)` +stops all of it. (Hardening the whole state-dir root to `0700` is an +obvious neighbour — noted as deferred, since it would re-mode a directory +users already have.) + +### Q#AS10 — Default on, conditional on Q#AS11 + +**On by default**, with the interval configurable, a disable knob, and +**only because Q#AS11 lands in the same PR**. If private storage slips, +this drops to opt-in. + +The parent framing (Q#PS9) tentatively said opt-in, grouping autosave +with desktop-save because "background writes are surprising." That +grouping conflated two things: desktop-save is opt-in because +*auto-restore* changes what you see at startup. Autosave changes nothing +observable until the day it saves your work; it writes only into the +state dir (never your files), it is inert when nothing is modified, and +it is the highest-value safety net in the arc. Emacs ships it on; +saveplace and recentf are already default-on and also write. + +The reviewer's condition is the right bar and is now the plan of record: +default-on **requires** `0700`/`0600` storage plus documented retention +and disable. Both are in scope. + +## Phasing + +One PR (the pieces are useless apart). In-diff order: `src/hash.rs` + +`state::read_bytes` + `save_atomic_with_mode`/`state::write_private` +(Q#AS11) → `src/autosave.rs` (envelope, sweep, status, recover, discard, +pending) → `pmacs.autosave.*` bindings → `autosave.lua` (timer, config, +commands, cleanup wiring) → tests. + +## Bets (score at close) + +1. **`after-tick` + `monotonic_ms` is the right substrate** — no worker + thread parked, interval live-reconfigurable, no measurable per-frame + cost. +2. **The one-file envelope is crash-atomic** — a mode-aware + `save_atomic` means a recovery file is never a torn header/contents + pair nor briefly world-readable, and the first-newline split survives + arbitrary binary contents. +3. **The nullable-origin guard covers new files** — `[new file]` buffers + round-trip, and the `Fresh`/`Stale` table never offers to clobber an + externally-changed (or externally-created) file. +4. **Pull-based aggregated notify is sufficient UX** — one message + however many files are recoverable, it covers argv `[new file]` + buffers that fire no hook, and desktop-restore stays clean. + +## Deferred (named) + +- **Idle-gated autosave** (Emacs's `auto-save-timeout` idle semantics); + v1 is plain wall-clock elapsed. +- Autosaving non-file (scratch) buffers. +- Orphan recovery-file GC / a `list-recovery-files` browser. +- Offloading large-buffer writes to a worker thread. +- **An external-change guard on `save()` itself** — the scout found + pmacs overwrites unconditionally today. Real bug, adjacent, its own PR. +- A general `y_or_n` minibuffer helper (build it when a second caller + appears). +- A central, typed config registry (Q#AS3's note). +- Hardening the whole `$XDG_STATE_HOME/pmacs/` root to `0700` (Q#AS11) — + it would re-mode a directory users already have. +- Firing `buffer.after-load` for `[new file]` buffers. Today argv-opening + a missing path fires no hook at all, so a new file gets no syntax, no + LSP, and no saveplace. That is a real latent gap, but changing it + ripples through four builtins and does not belong in an autosave PR. +- Hidden-buffer LSP initial attach (carried from phase 2). + +## Acceptance (tempdir state root injected; `sweep()` called directly) + +- Modify a file buffer → `sweep()` → recovery file exists; its header + path + origin meta match, its bytes equal the buffer. +- **Non-UTF-8 contents** round-trip through the envelope (the + `read_bytes` reason). +- **`[new file]` buffer** (path that does not exist): swept, header + `origin: null`, status `Fresh` while the file is still absent; and + `Stale` once the file exists on disk. Recoverable either way. +- **Permissions (Q#AS11)**: on Unix, the `autosave/` dir is `0700` and + each recovery file is `0600` — asserted, not assumed. +- Sweep skips clean buffers, scratch buffers, and buffers unchanged + since the last sweep (no second write). +- **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). +- `after-save` → recovery deleted. Kill buffer → recovery deleted. +- Open a file with a **`Fresh`** recovery → the aggregate report names + it; buffer contents are still the on-disk ones (no silent + substitution). +- **Aggregation**: three recoverable files opened → **one** status + message reporting `3`, not three messages. +- Touch the file on disk, then open → **`Stale`**: distinct message, not + offered. +- **`Corrupt`**: a malformed envelope (no newline / bad JSON / bad + version) yields `Corrupt`, is never offered, does not error the report + or the commands, and `discard-recovery` removes it. +- `recover-file` → buffer contents become the recovery bytes, the buffer + is `is_modified()`, and **a probe on `buffer.after-edit` observes the + recovery** (the explicit `hook.run`); a subsequent save deletes the + recovery file. +- `interval_ms()` getter/setter: rejects non-numbers and `< 1000`, + floors floats, and a changed interval takes effect without a restart. +- `enable(false)` → `sweep()` is a no-op. +- `before-quit` sweeps once, synchronously, and does not veto quit. From ec42526652ac6a2e974716f9346e7aed6acee157 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Thu, 9 Jul 2026 10:24:08 -0400 Subject: [PATCH 2/6] feat(persistence): autosave + crash recovery (Arc 3 phase 3) Framing: docs/autosave-recovery-framing.md (Q#AS1-11). Closes the persistence arc. Every modified file buffer is periodically written to a private recovery copy; if pmacs dies, the next session says so and `M-x recover-file` installs it. Emacs's auto-save-mode + recover-file. Hybrid, forced by the same two gaps as phase 2: Lua has no per-buffer path getter and FileMeta is neither Lua-visible nor serde. Rust owns the sweep and the external-change guard; Lua owns cadence, config, and UX. src/autosave.rs (new): - One atomic envelope per recovery: a JSON header line + `\n` + raw buffer bytes. Split at the FIRST newline, so contents may hold newlines and non-UTF-8. A crash can never leave a torn header/contents pair. - `origin` is NULLABLE: a `[new file]` buffer (a path with nothing on disk) has no FileMeta, and its unsaved contents are exactly the work most worth recovering. - status(): Fresh / Stale / Corrupt / None. Only Fresh is announced; Stale (file changed, deleted, or created underneath us) is never auto-offered; Corrupt is typed, quiet, and discardable. - sweep(): all modified file buffers, skipping clean/scratch and those unchanged since their last copy. The skip cache is keyed BufferId -> (path_hash, revision), not revision alone: a buffer keeps its BufferId across a path change (LSP WorkspaceEdit rename), so a revision-only cache would skip the write and orphan the old key. - pending(): enumerates ALL open file buffers in Rust, which is what covers argv `[new file]` buffers -- they fire no hook at all. Private storage (Q#AS11, a precondition for default-on): autosave stores unsaved FILE CONTENTS, not metadata. New `file_io::save_atomic_with_mode` sets the temp's mode BEFORE the rename (a chmod-after-write leaves a window where the file is 0644), and `state::write_private` creates the dir 0700 and the file 0600. Plus `state::read_bytes` (state::read is read_to_string, which non-UTF-8 buffer contents would fail). builtin/runtime/autosave.lua: - Cadence is `process.after-tick` + monotonic_ms, NOT workers.sleep: a long sleep parks one of only `available_parallelism - 1` pool threads, and re-reading the interval each tick makes it live-reconfigurable. - pmacs.autosave.interval_ms([ms]) -- validated getter/setter following the async_config.frame_target_ms shape. Default 30000, floor 1000. pmacs.autosave.enable(on). On by default. - Notify, never prompt: `after-load` only raises a flag; the tick emits ONE aggregate message ("3 files have autosave recovery"). A modal prompt from after-load would stack N modals during a desktop restore. - recover-file confirms, pins to the origin buffer, replaces contents, then explicitly fires `buffer.after-edit` -- the mutators only notify windows and queue CRDT, and after-edit comes from dispatch_key's post-command check, which the minibuffer shadow returns before. Without the explicit fire, LSP didChange and the syntax reparse never see the recovery. discard-recovery deletes a copy (including a Corrupt one). - Cleanup: after-save discards; per-buffer on_removed discards on kill (there is no global kill hook); before-quit does one final synchronous sweep and never vetoes. src/hash.rs (new): one pub(crate) sha256_hex, shared by desktop, autosave, and packages::fetcher -- which had two private duplicates (Q#AS9). Not daemon-gated (unlike desktop-save): autosave is per-buffer, not per-frontend, and a daemon holds the unsaved work. Tests: 8 autosave units + 13 state/hash units + tests/autosave_acceptance (15): sweep round-trip, non-UTF-8 envelope, [new file] null-origin Fresh->Stale, 0600/0700 perms, skip clean/scratch/unchanged, path-change rewrites new key + discards old, save/kill cleanup, Stale not offered, Corrupt typed+quiet+discardable, recover-file installs + fires after-edit + leaves modified, tick aggregation (3 loads -> 1 message, no repeat), single-file naming, interval validation + live change, enable gate, before-quit sweeps without vetoing. Gates: fmt + workspace clippy clean; lib 1499; crdt 1670; autosave 15; desktop 11; persistence 5; m4 90; m7_8 5; m8 10; GPU 58; git diff --check. Co-Authored-By: Claude Opus 4.8 (1M context) --- builtin/runtime/autosave.lua | 193 ++++++++++++++ src/autosave.rs | 481 +++++++++++++++++++++++++++++++++++ src/desktop.rs | 14 +- src/editor.rs | 6 + src/file_io.rs | 45 +++- src/hash.rs | 58 +++++ src/lib.rs | 2 + src/lua_bindings/mod.rs | 65 +++++ src/packages/fetcher.rs | 19 +- src/state.rs | 115 ++++++++- tests/autosave_acceptance.rs | 456 +++++++++++++++++++++++++++++++++ 11 files changed, 1421 insertions(+), 33 deletions(-) create mode 100644 builtin/runtime/autosave.lua create mode 100644 src/autosave.rs create mode 100644 src/hash.rs create mode 100644 tests/autosave_acceptance.rs diff --git a/builtin/runtime/autosave.lua b/builtin/runtime/autosave.lua new file mode 100644 index 0000000..7f005b7 --- /dev/null +++ b/builtin/runtime/autosave.lua @@ -0,0 +1,193 @@ +-- autosave.lua --- periodic recovery copies + crash recovery (Arc 3 phase 3). +-- +-- Every `interval_ms` a sweep writes a private recovery copy of each +-- modified file buffer. If pmacs dies, the next session notices the copy +-- and says so; `M-x recover-file` installs it. Emacs's `auto-save-mode` +-- + `recover-file`. +-- +-- The Rust half (`pmacs.autosave._*`) does the sweep and the +-- external-change guard; this file owns the cadence, the configurable +-- interval, and the UX. +-- +-- On by default. Configure from init.lua: +-- pmacs.autosave.interval_ms(60000) -- default 30000, floor 1000 +-- pmacs.autosave.enable(false) -- turn it off entirely +-- +-- Recovery files live under `$XDG_STATE_HOME/pmacs/autosave/` (0700 dir, +-- 0600 files), are deleted when the buffer is saved or killed, and +-- survive a crash or a quit with unsaved changes. +-- +-- Framing: docs/autosave-recovery-framing.md. + +pmacs.autosave = pmacs.autosave or {} + +local DEFAULT_INTERVAL_MS = 30000 -- Emacs's auto-save-timeout +local MIN_INTERVAL_MS = 1000 -- each sweep fsyncs; don't storm + +local interval = DEFAULT_INTERVAL_MS +local enabled = true +local last_sweep_ms = nil +-- Report on the first tick (the startup scan), and after every load. +local needs_report = true + +-- enable(on) --- turn autosave off (or back on). +function pmacs.autosave.enable(on) + enabled = (on ~= false) + return enabled +end + +-- interval_ms([ms]) --- getter when `ms` is nil, else a validated setter. +-- Shape follows `pmacs.async_config.frame_target_ms`. The tick re-reads +-- this every frame, so a change takes effect immediately -- no restart. +function pmacs.autosave.interval_ms(ms) + if ms == nil then return interval end + if type(ms) ~= "number" or ms ~= ms or ms < MIN_INTERVAL_MS then + error("pmacs.autosave.interval_ms: expected a number >= " .. MIN_INTERVAL_MS) + end + interval = math.floor(ms) + return interval +end + +-- sweep() --- force a pass now. Returns how many buffers were written. +function pmacs.autosave.sweep() + if not enabled then return 0 end + return pmacs.autosave._sweep() +end + +local function basename(path) + return path:match("[^/]+$") or path +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() + local fresh = pmacs.autosave._pending() + local n = #fresh + if n == 1 then + pmacs.editor.set_status(basename(fresh[1]) .. " has autosave recovery --- M-x recover-file") + elseif n > 1 then + pmacs.editor.set_status(n .. " files have autosave recovery --- M-x recover-file") + end + -- Corrupt copies are counted but stay quiet: a malformed recovery file + -- must not make startup noisy. `M-x discard-recovery` removes them. +end + +-- The cadence (Q#AS2). `process.after-tick` fires every frame -- and the +-- run loops tick on a frame *timeout*, not only on input, so this keeps +-- running while the editor is idle. Costs one clock read + a compare per +-- frame, and parks no worker thread (a long `workers.sleep` would hold +-- one of only `available_parallelism - 1` pool threads). +pmacs.hook.add("process.after-tick", function() + if needs_report then + needs_report = false + pcall(report_pending) + end + if not enabled then return end + local now = pmacs.editor.monotonic_ms() + if last_sweep_ms == nil then + last_sweep_ms = now + return + end + if now - last_sweep_ms >= interval then + last_sweep_ms = now + pcall(pmacs.autosave._sweep) + end +end) + +-- A load may reveal a recovery file; report on the next tick (never from +-- inside the hook -- a desktop restore fires this once per leaf). +pmacs.hook.add("buffer.after-load", function() + needs_report = true + -- A clean save or a kill retires the recovery copy. There is no global + -- kill hook, so register per buffer, capturing the path now (the buffer + -- may be gone by the time the callback runs). + local path = pmacs.editor.file_path() + if not path then return end + local buf = pmacs.window.buffer() + if not buf then return end + pcall(pmacs.buffer.on_removed, buf, function() + pcall(pmacs.autosave._discard, path) + end) +end) + +pmacs.hook.add("buffer.after-save", function() + local path = pmacs.editor.file_path() + if path then pcall(pmacs.autosave._discard, path) end +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. +pmacs.hook.add("editor.before-quit", function() + pcall(pmacs.autosave.sweep) +end) + +pmacs.command.define { + name = "recover-file", + description = "Replace this buffer with its autosave recovery copy.", + fn = function() + local path = pmacs.editor.file_path() + if not path then + pmacs.editor.set_status("recover-file: not visiting a file") + return + end + local st = pmacs.autosave._status(path) + if st == "none" then + pmacs.editor.set_status("recover-file: no autosave recovery for this file") + return + end + if st == "corrupt" then + pmacs.editor.set_status("recover-file: recovery file is corrupt --- M-x discard-recovery") + return + end + local warn = "" + if st == "stale" then + warn = " [WARNING: file changed on disk since the autosave]" + end + pmacs.minibuffer.read { + prompt = "Recover from autosave?" .. warn .. " (yes/no): ", + source = function() return { "yes", "no" } end, + on_accept = function(answer) + if answer ~= "yes" then + pmacs.editor.set_status("recover-file: cancelled") + return + end + -- Pin to the buffer we started on: focus can drift while the + -- prompt is up, and recovering into the wrong buffer is + -- unrecoverable. + if pmacs.editor.file_path() ~= path then + pmacs.editor.set_status("recover-file: buffer changed; aborted") + return + end + local bytes = pmacs.autosave._recover_bytes(path) + if not bytes then + pmacs.editor.set_status("recover-file: recovery unreadable") + return + end + local buf = pmacs.window.buffer() + buf:replace(0, buf:len(), bytes) + -- 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") + pmacs.editor.set_status("recovered from autosave --- save to keep it") + end, + } + end, +} + +pmacs.command.define { + name = "discard-recovery", + description = "Delete this file's autosave recovery copy.", + fn = function() + local path = pmacs.editor.file_path() + if not path then + pmacs.editor.set_status("discard-recovery: not visiting a file") + return + end + pmacs.autosave._discard(path) + pmacs.editor.set_status("discarded autosave recovery") + end, +} diff --git a/src/autosave.rs b/src/autosave.rs new file mode 100644 index 0000000..bfccb03 --- /dev/null +++ b/src/autosave.rs @@ -0,0 +1,481 @@ +// autosave.rs --- periodic recovery copies + crash recovery (Arc 3 phase 3). + +//! Every modified file buffer is periodically written to a private +//! recovery file under `$XDG_STATE_HOME/pmacs/autosave/`. If pmacs dies, +//! the next session notices the copy and offers `M-x recover-file`. +//! Emacs's `auto-save-mode` + `recover-file`. +//! +//! This module owns the parts Lua cannot do: enumerating **all** buffers' +//! paths (Lua has no per-buffer path getter), and the `FileMeta` +//! external-change guard (`FileMeta` is neither Lua-visible nor serde). +//! `builtin/runtime/autosave.lua` owns the cadence, the configurable +//! interval, and the recovery UX. +//! +//! Recovery files are written `0600` under a `0700` directory +//! (Q#AS11) — they hold *unsaved file contents*, a different class of +//! secret from saveplace's cursor offsets. +//! +//! Framing: docs/autosave-recovery-framing.md. + +use std::cell::RefCell; +use std::collections::HashMap; +use std::path::Path; + +use mlua::Lua; +use serde::{Deserialize, Serialize}; + +use crate::buffer::BufferId; +use crate::file_io::FileMeta; +use crate::hash::sha256_hex; +use crate::lua_bindings::{SharedCore, StateDir}; + +/// Bump when the envelope shape changes incompatibly. A recovery file +/// with an unrecognized version reads as [`RecoveryStatus::Corrupt`] — +/// never silently applied. +pub const AUTOSAVE_VERSION: u32 = 1; + +/// The header of a recovery file: one line of JSON, then `\n`, then the +/// raw buffer bytes (Q#AS4). One atomic write, so a crash can never leave +/// a torn header/contents pair. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +struct Header { + version: u32, + /// The buffer's path, for provenance and orphan inspection. + path: String, + /// The origin file's identity when this copy was taken. + /// + /// **Nullable**: a `[new file]` buffer (a path that does not exist on + /// disk yet) has no `file_meta`, and its unsaved contents are exactly + /// the work most worth recovering. `None` means "there was no file on + /// disk when this was autosaved". + origin: Option, +} + +/// `FileMeta` hand-serialized — it is not serde, and `SystemTime` has no +/// stable wire form. Stored as an offset from the Unix epoch so the +/// comparison is exact; we never reconstruct a `SystemTime`, only compare +/// these parts. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] +struct Origin { + mtime_secs: i64, + mtime_nanos: u32, + size: u64, +} + +impl Origin { + fn from_meta(m: &FileMeta) -> Self { + let (mtime_secs, mtime_nanos) = match m.mtime.duration_since(std::time::UNIX_EPOCH) { + Ok(d) => ( + i64::try_from(d.as_secs()).unwrap_or(i64::MAX), + d.subsec_nanos(), + ), + // Pre-epoch mtimes are exotic but representable. + Err(e) => { + let d = e.duration(); + ( + i64::try_from(d.as_secs()).map_or(i64::MIN, |s| -s), + d.subsec_nanos(), + ) + } + }; + Self { + mtime_secs, + mtime_nanos, + size: m.size, + } + } +} + +/// What a recovery file means for a given path (Q#AS5). +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum RecoveryStatus { + /// No recovery file. + None, + /// The on-disk file is unchanged since the copy was taken (or is + /// still absent, for a `[new file]`), so the recovery is strictly + /// newer. The only status that is announced. + Fresh, + /// The file changed underneath us — externally edited, deleted, or + /// (for a `[new file]`) created by someone else. Never auto-offered: + /// silently clobbering it is the one unrecoverable mistake here. + Stale, + /// Unparseable or unrecognized version. Never offered, never errors; + /// `discard-recovery` removes it. + Corrupt, +} + +impl RecoveryStatus { + /// The lowercase name Lua sees. + #[must_use] + pub fn as_str(self) -> &'static str { + match self { + RecoveryStatus::None => "none", + RecoveryStatus::Fresh => "fresh", + RecoveryStatus::Stale => "stale", + RecoveryStatus::Corrupt => "corrupt", + } + } +} + +/// The `pmacs.state` key a path's recovery file lives under. +#[must_use] +pub fn key_for(path: &Path) -> String { + format!("autosave/{}", sha256_hex(&path.display().to_string())) +} + +/// Skip cache: `BufferId → (path_hash, revision)` (Q#AS8). +/// +/// Keyed on the **path hash as well as the revision**, not the revision +/// alone: a buffer keeps its `BufferId` across a path change (an LSP +/// `WorkspaceEdit` rename calls `set_buffer_path`), so a revision-only +/// cache would skip the write, never create the recovery file under the +/// new key, and orphan the old one. +#[derive(Default)] +pub struct AutosaveCache(RefCell>); + +/// Encode a header + contents into the one-file envelope. +fn encode(header: &Header, contents: &[u8]) -> Result, String> { + // serde_json's compact form never contains a raw newline, so the + // first `\n` unambiguously ends the header. + let mut out = serde_json::to_vec(header).map_err(|e| e.to_string())?; + debug_assert!(!out.contains(&b'\n')); + out.push(b'\n'); + out.extend_from_slice(contents); + Ok(out) +} + +/// Split an envelope at its **first** newline. Contents may contain +/// newlines and arbitrary non-UTF-8 bytes, so only the first one counts. +/// Returns `None` for anything malformed — the caller maps that to +/// [`RecoveryStatus::Corrupt`]. +fn decode(bytes: &[u8]) -> Option<(Header, &[u8])> { + let nl = bytes.iter().position(|&b| b == b'\n')?; + let header: Header = serde_json::from_slice(&bytes[..nl]).ok()?; + if header.version != AUTOSAVE_VERSION { + return None; + } + Some((header, &bytes[nl + 1..])) +} + +/// The configured state dir, if any (absent under tests / no HOME). +fn base_dir(lua: &Lua) -> Option { + lua.app_data_ref::().map(|d| d.0.clone()) +} + +/// Classify the recovery file for `path` (Q#AS5's table). +#[must_use] +pub fn status(base: &Path, path: &Path) -> RecoveryStatus { + let key = key_for(path); + let Ok(Some(bytes)) = crate::state::read_bytes(base, &key) else { + return RecoveryStatus::None; + }; + let Some((header, _)) = decode(&bytes) else { + return RecoveryStatus::Corrupt; + }; + let on_disk = crate::file_io::current_meta(path).ok(); + match (header.origin, on_disk) { + // Had an origin, file still there: fresh iff identity matches. + (Some(o), Some(cur)) if o == Origin::from_meta(&cur) => RecoveryStatus::Fresh, + // `[new file]`: fresh while it is still absent. + (None, None) => RecoveryStatus::Fresh, + // Everything else changed underneath us: the file was edited + // externally, deleted, or (for a `[new file]`) created by someone + // else. Never auto-offered. + _ => RecoveryStatus::Stale, + } +} + +/// The recovered contents for `path`, if a parseable recovery exists. +/// Returns bytes for `Stale` too — the command warns and confirms. +#[must_use] +pub fn recover_bytes(base: &Path, path: &Path) -> Option> { + let bytes = crate::state::read_bytes(base, &key_for(path)) + .ok() + .flatten()?; + let (_, contents) = decode(&bytes)?; + Some(contents.to_vec()) +} + +/// Delete the recovery file for `path` (idempotent). +pub fn discard(base: &Path, path: &Path) -> bool { + crate::state::remove(base, &key_for(path)).is_ok() +} + +/// A buffer that needs a recovery copy written, gathered under the core +/// borrow so all IO happens after it is released. +struct Pending { + id: BufferId, + path: String, + path_hash: String, + revision: u64, + origin: Option, + contents: Vec, +} + +/// One autosave pass: write a recovery copy of every modified file +/// buffer whose contents changed since its last copy. Returns how many +/// were written. +/// +/// Runs on the main thread; the two skips in Q#AS8 keep that bounded. +/// Unlike desktop-save this is **not** daemon-gated — autosave is +/// per-buffer, not per-frontend, and a daemon holds the unsaved work. +/// +/// # Errors +/// A state-write failure. Individual buffers never abort the pass. +pub fn sweep(lua: &Lua) -> Result { + let Some(base) = base_dir(lua) else { + return Ok(0); + }; + let core = lua + .app_data_ref::() + .ok_or("no editor core")? + .clone(); + + // Gather under a borrow; do all IO after releasing it. + let mut writes: Vec = Vec::new(); + let mut orphans: Vec = Vec::new(); + let mut live: Vec = Vec::new(); + { + let cache = lua + .app_data_ref::() + .ok_or("no autosave cache")?; + let cache = cache.0.borrow(); + let c = core.borrow(); + let reg = c.registry.borrow(); + for &id in reg.ids() { + let Ok(buf) = reg.get(id) else { continue }; + live.push(id); + // Skips scratch / *special* (no path). Includes `[new file]` + // buffers: path set, `file_meta` absent. + let Some(path) = buf.file_path() else { + continue; + }; + if !buf.is_modified() { + continue; + } + let path_s = path.display().to_string(); + let path_hash = sha256_hex(&path_s); + let revision = buf.revision(); + if let Some((prev_hash, prev_rev)) = cache.get(&id) { + if prev_hash == &path_hash && *prev_rev == revision { + continue; // unchanged since its last copy + } + if prev_hash != &path_hash { + // The path moved: the old key is now an orphan. + orphans.push(prev_hash.clone()); + } + } + let len = buf.len(); + let mut contents = vec![0u8; usize::try_from(len).unwrap_or(0)]; + if len > 0 { + buf.snapshot_rope().slice(0, len, &mut contents); + } + writes.push(Pending { + id, + path: path_s, + path_hash, + revision, + origin: buf.file_meta().map(Origin::from_meta), + contents, + }); + } + } + + 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(); + // Drop entries for buffers that no longer exist. + cache.retain(|id, _| live.contains(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.insert(p.id, (p.path_hash, p.revision)); + written += 1; + } + } + Ok(written) +} + +/// Every open file buffer that has a recovery file, with its status +/// (Q#AS6). Enumerating in Rust is what makes this cover argv +/// `[new file]` buffers, which fire no hook at all — the Lua reporter +/// never has to know they exist. +/// +/// Returns `(fresh_paths, corrupt_count)`. +#[must_use] +pub fn pending(lua: &Lua) -> (Vec, usize) { + let mut fresh = Vec::new(); + let mut corrupt = 0usize; + let (Some(base), Some(core)) = (base_dir(lua), lua.app_data_ref::()) else { + return (fresh, corrupt); + }; + // Collect paths first so the guard drops before any IO re-entrancy. + let paths: Vec = { + let c = core.borrow(); + let reg = c.registry.borrow(); + reg.ids() + .iter() + .filter_map(|&id| reg.get(id).ok()?.file_path().map(Path::to_path_buf)) + .collect() + }; + for p in paths { + match status(&base, &p) { + RecoveryStatus::Fresh => fresh.push(p.display().to_string()), + RecoveryStatus::Corrupt => corrupt += 1, + RecoveryStatus::None | RecoveryStatus::Stale => {} + } + } + (fresh, corrupt) +} + +#[cfg(test)] +mod tests { + use super::*; + + fn header(origin: Option) -> Header { + Header { + version: AUTOSAVE_VERSION, + path: "/tmp/a.rs".into(), + origin, + } + } + + #[test] + fn envelope_round_trips_arbitrary_bytes() { + // Contents with newlines AND invalid UTF-8 — the reason we split + // at the first newline and read bytes, not a String. + let contents = [0xffu8, b'\n', b'a', 0x00, b'\n']; + let h = header(Some(Origin { + mtime_secs: 5, + mtime_nanos: 7, + size: 5, + })); + let bytes = encode(&h, &contents).unwrap(); + let (got_h, got_c) = decode(&bytes).unwrap(); + assert_eq!(got_h, h); + assert_eq!(got_c, &contents[..]); + } + + #[test] + fn envelope_round_trips_null_origin() { + // A `[new file]` buffer: no origin meta. + let h = header(None); + let bytes = encode(&h, b"draft").unwrap(); + let (got_h, got_c) = decode(&bytes).unwrap(); + assert!(got_h.origin.is_none()); + assert_eq!(got_c, b"draft"); + } + + #[test] + fn decode_rejects_malformed_and_wrong_version() { + assert!(decode(b"no newline at all").is_none()); + assert!(decode(b"{not json}\nbody").is_none()); + assert!(decode(b"\nbody").is_none(), "empty header"); + let bad_version = br#"{"version":999,"path":"/x","origin":null}"#; + let mut bytes = bad_version.to_vec(); + bytes.push(b'\n'); + assert!(decode(&bytes).is_none(), "unrecognized version → corrupt"); + } + + #[test] + fn key_is_a_valid_state_key() { + let k = key_for(Path::new("/home/u/a b.rs")); + assert!(k.starts_with("autosave/")); + assert!(crate::state::validate_name(&k).is_ok()); + } + + #[test] + fn status_none_when_no_recovery_file() { + let dir = std::env::temp_dir().join(format!("pmacs-as-none-{}", std::process::id())); + std::fs::create_dir_all(&dir).unwrap(); + assert_eq!( + status(&dir, Path::new("/tmp/nonexistent.rs")), + RecoveryStatus::None + ); + std::fs::remove_dir_all(&dir).ok(); + } + + #[test] + fn status_corrupt_for_garbage_envelope() { + let dir = std::env::temp_dir().join(format!("pmacs-as-corrupt-{}", std::process::id())); + std::fs::create_dir_all(&dir).unwrap(); + let target = Path::new("/tmp/whatever.rs"); + crate::state::write_private(&dir, &key_for(target), b"garbage, no newline").unwrap(); + assert_eq!(status(&dir, target), RecoveryStatus::Corrupt); + // And it is discardable. + assert!(discard(&dir, target)); + assert_eq!(status(&dir, target), RecoveryStatus::None); + std::fs::remove_dir_all(&dir).ok(); + } + + #[test] + fn new_file_status_is_fresh_until_the_file_appears() { + let dir = std::env::temp_dir().join(format!("pmacs-as-newfile-{}", std::process::id())); + std::fs::create_dir_all(&dir).unwrap(); + let target = dir.join("draft.rs"); + // origin: null — a `[new file]` buffer. + let bytes = encode( + &Header { + version: AUTOSAVE_VERSION, + path: target.display().to_string(), + origin: None, + }, + b"unsaved draft", + ) + .unwrap(); + crate::state::write_private(&dir, &key_for(&target), &bytes).unwrap(); + + assert_eq!(status(&dir, &target), RecoveryStatus::Fresh, "file absent"); + assert_eq!( + recover_bytes(&dir, &target).as_deref(), + Some(&b"unsaved draft"[..]) + ); + + // Someone created the file meanwhile → stale, never auto-offered. + std::fs::write(&target, b"someone else's content").unwrap(); + assert_eq!(status(&dir, &target), RecoveryStatus::Stale); + std::fs::remove_dir_all(&dir).ok(); + } + + #[test] + fn existing_file_fresh_until_it_changes_on_disk() { + let dir = std::env::temp_dir().join(format!("pmacs-as-exist-{}", std::process::id())); + std::fs::create_dir_all(&dir).unwrap(); + let target = dir.join("a.rs"); + std::fs::write(&target, b"on disk").unwrap(); + let meta = crate::file_io::current_meta(&target).unwrap(); + let bytes = encode( + &Header { + version: AUTOSAVE_VERSION, + path: target.display().to_string(), + origin: Some(Origin::from_meta(&meta)), + }, + b"unsaved edits", + ) + .unwrap(); + crate::state::write_private(&dir, &key_for(&target), &bytes).unwrap(); + assert_eq!(status(&dir, &target), RecoveryStatus::Fresh); + + // Touch the file (different size ⇒ different identity). + std::fs::write(&target, b"changed underneath us").unwrap(); + assert_eq!(status(&dir, &target), RecoveryStatus::Stale); + + // Delete it entirely → still stale (the base is gone). + std::fs::remove_file(&target).unwrap(); + assert_eq!(status(&dir, &target), RecoveryStatus::Stale); + std::fs::remove_dir_all(&dir).ok(); + } +} diff --git a/src/desktop.rs b/src/desktop.rs index b871d2b..23ff5ac 100644 --- a/src/desktop.rs +++ b/src/desktop.rs @@ -20,10 +20,10 @@ use std::path::Path; use mlua::Lua; use serde::{Deserialize, Serialize}; -use sha2::{Digest, Sha256}; use crate::buffer::BufferId; use crate::editor_core::EditorCore; +use crate::hash::sha256_hex; use crate::lua_bindings::{LocalInstanceInfo, SharedCore, StateDir, fire_after_load_hook}; use crate::protocol::FrontendId; use crate::text_view::TextView; @@ -141,18 +141,6 @@ pub fn desktop_state_key(session_key: &str) -> String { format!("desktop/{session_key}") } -fn sha256_hex(s: &str) -> String { - let mut h = Sha256::new(); - h.update(s.as_bytes()); - let digest = h.finalize(); - let mut out = String::with_capacity(digest.len() * 2); - for b in digest { - use std::fmt::Write as _; - let _ = write!(out, "{b:02x}"); - } - out -} - /// Build the serializable layout tree from a core [`LayoutNode`], /// resolving each leaf window to a [`SavedLeaf`] (returning `None` for a /// non-file leaf, which is dropped and its split collapsed). diff --git a/src/editor.rs b/src/editor.rs index 598b670..5fd9f61 100644 --- a/src/editor.rs +++ b/src/editor.rs @@ -321,6 +321,12 @@ impl EditorState { include_str!("../builtin/runtime/desktop.lua"), ) .expect("load desktop builtin chunk"); + lua_host + .eval( + Some("@pmacs/builtin/runtime/autosave.lua"), + include_str!("../builtin/runtime/autosave.lua"), + ) + .expect("load autosave builtin chunk"); // T M7.11 bundled-package bootstrap. Through M7.10 the REPL // was loaded directly via `eval(include_str!(...))`; the // M7.11 deliverable migrates it to the package system so it diff --git a/src/file_io.rs b/src/file_io.rs index 28f0574..1b33350 100644 --- a/src/file_io.rs +++ b/src/file_io.rs @@ -130,7 +130,32 @@ impl Drop for TempCleanup { /// identity for future change-detection comparisons. /// /// Threading: any thread. +/// +/// # Errors +/// See [`SaveError`]. pub fn save_atomic(path: &Path, content: &[u8]) -> Result { + save_atomic_with_mode(path, content, None) +} + +/// [`save_atomic`], but forcing the target's Unix mode to `mode` instead +/// of inheriting the existing file's mode (or the umask default for a new +/// file). +/// +/// The mode is applied to the **temp file before the rename**, so the +/// target never exists — not even momentarily — at a laxer mode. A +/// `chmod` after the write would leave exactly that window, which matters +/// because autosave (Arc 3 Q#AS11) stores *unsaved file contents*: a +/// recovery copy must never be briefly world-readable. +/// +/// `mode` is ignored on non-Unix platforms (the write is still atomic). +/// +/// # Errors +/// See [`SaveError`]. +pub fn save_atomic_with_mode( + path: &Path, + content: &[u8], + mode: Option, +) -> Result { // `Path::parent` returns: // * `None` for `/` or `""` --- no place to put a sibling temp file; // * `Some("")` for a bare filename like `notes.txt` --- means cwd, fine; @@ -140,10 +165,22 @@ pub fn save_atomic(path: &Path, content: &[u8]) -> Result { return Err(SaveError::NoParent(path.to_path_buf())); } - // Snapshot the target's current permissions so an existing file keeps - // its mode across the replace (F-006) — e.g. a `0755` script stays - // executable. `None` for a new file, which then gets the default mode. - let existing_perms = fs::metadata(path).ok().map(|m| m.permissions()); + // An explicit `mode` wins; otherwise snapshot the target's current + // permissions so an existing file keeps its mode across the replace + // (F-006) — e.g. a `0755` script stays executable. `None` for a new + // file, which then gets the default mode. + #[cfg(unix)] + let existing_perms = mode + .map(|m| { + use std::os::unix::fs::PermissionsExt as _; + fs::Permissions::from_mode(m) + }) + .or_else(|| fs::metadata(path).ok().map(|m| m.permissions())); + #[cfg(not(unix))] + let existing_perms = { + let _ = mode; // no mode concept; the write is still atomic + fs::metadata(path).ok().map(|m| m.permissions()) + }; // Open a fresh temp, retrying on the rare name collision (a stale temp // left by a crashed prior run whose pid+nanos recurs) instead of diff --git a/src/hash.rs b/src/hash.rs new file mode 100644 index 0000000..6310f45 --- /dev/null +++ b/src/hash.rs @@ -0,0 +1,58 @@ +// hash.rs --- shared content hashing (Arc 3 phase 3, Q#AS9). + +//! One `sha256_hex` for the whole crate. Three call sites want a stable, +//! filename-safe digest of a string: +//! +//! * [`crate::desktop`] — the desktop session key, +//! * [`crate::autosave`] — the recovery-file key (a hash of the path), +//! * `crate::packages::fetcher` — the package cache/mirror key. +//! +//! Each had grown (or was about to grow) its own private copy. A +//! *cryptographic* digest matters for the fetcher: a non-cryptographic +//! hash is trivially collidable, and a deliberate collision would make +//! two URLs share one bare mirror + lock file. +//! +//! Lowercase hex, so the output passes +//! [`crate::state::validate_name`]'s `[A-Za-z0-9._-]` charset. + +use sha2::{Digest, Sha256}; + +/// Lowercase-hex SHA-256 of `s`. +pub(crate) fn sha256_hex(s: &str) -> String { + use std::fmt::Write as _; + let mut hasher = Sha256::new(); + hasher.update(s.as_bytes()); + let digest = hasher.finalize(); + let mut out = String::with_capacity(digest.len() * 2); + for b in digest { + let _ = write!(out, "{b:02x}"); + } + out +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn known_vector_and_charset() { + // The canonical empty-string SHA-256. + assert_eq!( + sha256_hex(""), + "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855" + ); + let h = sha256_hex("/home/u/a.rs"); + assert_eq!(h.len(), 64); + assert!( + h.bytes() + .all(|b| b.is_ascii_lowercase() || b.is_ascii_digit()) + ); + // Filename-safe: passes the state-key charset. + assert!(crate::state::validate_name(&format!("autosave/{h}")).is_ok()); + } + + #[test] + fn distinct_inputs_distinct_digests() { + assert_ne!(sha256_hex("a"), sha256_hex("b")); + } +} diff --git a/src/lib.rs b/src/lib.rs index 7469641..7ee5f29 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -45,6 +45,7 @@ pub mod attach; pub mod attach_dispatch; pub mod attach_reconnect; pub mod audit; +pub mod autosave; pub mod buffer; pub mod buffer_registry; pub mod builtin_packages; @@ -75,6 +76,7 @@ pub mod file_io; pub mod formatting; pub mod frontend; pub mod fs; +mod hash; pub mod help; pub mod highlight; pub mod hook; diff --git a/src/lua_bindings/mod.rs b/src/lua_bindings/mod.rs index 54ab0d3..6878d08 100644 --- a/src/lua_bindings/mod.rs +++ b/src/lua_bindings/mod.rs @@ -1963,10 +1963,75 @@ pub fn install( pmacs.set("packages", install_packages_module(lua)?)?; pmacs.set("state", install_state_module(lua)?)?; pmacs.set("session", install_session_module(lua)?)?; + pmacs.set("autosave", install_autosave_module(lua)?)?; lua.globals().set("pmacs", pmacs)?; Ok(()) } +/// `pmacs.autosave.*` — the Rust half of autosave + crash recovery +/// (Arc 3 phase 3). Lua cannot enumerate non-active buffers' paths, and +/// `FileMeta` is neither Lua-visible nor serde, so the sweep and the +/// external-change guard live in Rust (Q#AS1). `autosave.lua` layers the +/// cadence, the configurable interval, and the recovery UX on top. +/// +/// The `_`-prefixed names are the raw primitives; `autosave.lua` adds the +/// public `enable` / `interval_ms` / `sweep` onto the same table. +fn install_autosave_module(lua: &Lua) -> mlua::Result { + // The skip cache lives for the life of the VM. + lua.set_app_data(crate::autosave::AutosaveCache::default()); + let m = lua.create_table()?; + + m.set( + "_sweep", + lua.create_function(|lua, ()| crate::autosave::sweep(lua).map_err(mlua::Error::external))?, + )?; + + m.set( + "_status", + lua.create_function(|lua, path: String| { + let Some(base) = lua.app_data_ref::().map(|d| d.0.clone()) else { + return Ok("none"); + }; + Ok(crate::autosave::status(&base, std::path::Path::new(&path)).as_str()) + })?, + )?; + + m.set( + "_recover_bytes", + lua.create_function(|lua, path: String| { + let Some(base) = lua.app_data_ref::().map(|d| d.0.clone()) else { + return Ok(None); + }; + match crate::autosave::recover_bytes(&base, std::path::Path::new(&path)) { + Some(bytes) => Ok(Some(lua.create_string(&bytes)?)), + None => Ok(None), + } + })?, + )?; + + m.set( + "_discard", + lua.create_function(|lua, path: String| { + let Some(base) = lua.app_data_ref::().map(|d| d.0.clone()) else { + return Ok(false); + }; + Ok(crate::autosave::discard(&base, std::path::Path::new(&path))) + })?, + )?; + + // _pending() -> (fresh_paths, corrupt_count). Enumerates in Rust so + // argv `[new file]` buffers — which fire no hook — are covered too. + m.set( + "_pending", + lua.create_function(|lua, ()| { + let (fresh, corrupt) = crate::autosave::pending(lua); + Ok((fresh, corrupt)) + })?, + )?; + + Ok(m) +} + /// Marker app-data set by `pmacs.session.arm_restore()` (Arc 3 phase 2, /// Q#DS7). Its presence tells the `RunLocal` startup trigger to attempt /// a desktop restore; `desktop_mode(true)` in init.lua arms it. diff --git a/src/packages/fetcher.rs b/src/packages/fetcher.rs index e2ec2d7..af4fd76 100644 --- a/src/packages/fetcher.rs +++ b/src/packages/fetcher.rs @@ -62,7 +62,6 @@ use std::process::{Child, Command, ExitStatus, Stdio}; use std::thread; use std::time::{Duration, Instant}; -use sha2::{Digest, Sha256}; use thiserror::Error; // --------------------------------------------------------------------------- @@ -512,19 +511,11 @@ fn dot_git_strip_applies(u: &str) -> bool { /// The cache dir is keyed by a hash of the (attacker-adjacent) repo URL, /// so a *cryptographic* digest is used: a non-cryptographic hash like the /// former 64-bit FNV-1a is trivially collidable, and a deliberate -/// collision would make two URLs share one bare mirror + lock file. `sha2` -/// is already a dependency (M7.6 lockfile content hashing). -fn sha256_hex(s: &str) -> String { - use std::fmt::Write as _; - let mut hasher = Sha256::new(); - hasher.update(s.as_bytes()); - let digest = hasher.finalize(); - let mut out = String::with_capacity(digest.len() * 2); - for b in digest { - let _ = write!(out, "{b:02x}"); - } - out -} +/// collision would make two URLs share one bare mirror + lock file. +/// +/// The implementation now lives in [`crate::hash`] — shared with the +/// desktop session key and the autosave recovery key (Q#AS9). +use crate::hash::sha256_hex; // --------------------------------------------------------------------------- // LockGuard --- per-cache-entry flock(2) diff --git a/src/state.rs b/src/state.rs index d0b6865..22ba299 100644 --- a/src/state.rs +++ b/src/state.rs @@ -182,14 +182,76 @@ pub fn read(base: &Path, name: &str) -> Result, StateError> { /// # Errors /// Invalid key, or a save failure. pub fn write(base: &Path, name: &str, content: &[u8]) -> Result<(), StateError> { + write_inner(base, name, content, None) +} + +/// Like [`write`], but the parent directory is created `0700` and the +/// file written `0600` (Arc 3 Q#AS11). +/// +/// Autosave stores **unsaved file contents**, a different class of secret +/// from saveplace's cursor offsets or recentf's path list. The default +/// path would give a new recovery file the umask default (typically +/// `0644`) and its directory `0755` — leaving a recovery copy of an +/// unsaved edit to a `0600` file *more exposed than the original*. The +/// mode is applied to the temp before the rename, so there is no window +/// at a laxer mode. +/// +/// Permissions are Unix-only; elsewhere this is [`write`]. +/// +/// # Errors +/// Invalid key, or a save failure. +pub fn write_private(base: &Path, name: &str, content: &[u8]) -> Result<(), StateError> { + write_inner(base, name, content, Some(0o600)) +} + +fn write_inner( + base: &Path, + name: &str, + content: &[u8], + mode: Option, +) -> Result<(), StateError> { let path = resolve(base, name).map_err(StateError::Name)?; if let Some(parent) = path.parent() { - std::fs::create_dir_all(parent).map_err(StateError::Io)?; + create_dir_all_with_mode(parent, mode.map(|_| 0o700)).map_err(StateError::Io)?; } - crate::file_io::save_atomic(&path, content).map_err(StateError::Save)?; + crate::file_io::save_atomic_with_mode(&path, content, mode).map_err(StateError::Save)?; Ok(()) } +/// `create_dir_all`, optionally forcing the mode of directories this call +/// creates. An already-existing directory keeps its mode (we do not +/// re-mode a directory the user already has). +fn create_dir_all_with_mode(dir: &Path, mode: Option) -> std::io::Result<()> { + #[cfg(unix)] + if let Some(m) = mode { + use std::os::unix::fs::DirBuilderExt as _; + return std::fs::DirBuilder::new() + .recursive(true) + .mode(m) + .create(dir); + } + #[cfg(not(unix))] + let _ = mode; + std::fs::create_dir_all(dir) +} + +/// Read a state file's raw bytes, or `Ok(None)` when it does not exist. +/// +/// [`read`] returns a `String` (`read_to_string`), which fails on +/// non-UTF-8 content. pmacs buffers hold arbitrary bytes, so an autosave +/// recovery file cannot be read that way (Arc 3 Q#AS4). +/// +/// # Errors +/// Invalid key, or an IO error other than not-found. +pub fn read_bytes(base: &Path, name: &str) -> Result>, StateError> { + let path = resolve(base, name).map_err(StateError::Name)?; + match std::fs::read(&path) { + Ok(b) => Ok(Some(b)), + Err(e) if e.kind() == std::io::ErrorKind::NotFound => Ok(None), + Err(e) => Err(StateError::Io(e)), + } +} + /// Remove a state file. Missing file is success (idempotent). /// /// # Errors @@ -343,6 +405,55 @@ mod tests { std::fs::remove_dir_all(&dir).ok(); } + #[test] + fn read_bytes_round_trips_non_utf8() { + let dir = std::env::temp_dir().join(format!("pmacs-bytes-{}", std::process::id())); + std::fs::create_dir_all(&dir).unwrap(); + // Invalid UTF-8 — what `read` (read_to_string) would choke on. + let raw = [0xffu8, 0xfe, b'\n', 0x00, b'a']; + write(&dir, "blob", &raw).unwrap(); + assert_eq!(read_bytes(&dir, "blob").unwrap().as_deref(), Some(&raw[..])); + assert!(read(&dir, "blob").is_err(), "read_to_string rejects it"); + assert!(read_bytes(&dir, "absent").unwrap().is_none()); + std::fs::remove_dir_all(&dir).ok(); + } + + #[cfg(unix)] + #[test] + fn write_private_uses_0700_dir_and_0600_file() { + use std::os::unix::fs::PermissionsExt as _; + let dir = std::env::temp_dir().join(format!("pmacs-priv-{}", std::process::id())); + std::fs::remove_dir_all(&dir).ok(); + std::fs::create_dir_all(&dir).unwrap(); + + write_private(&dir, "autosave/secret", b"unsaved contents").unwrap(); + + let file = dir.join("autosave").join("secret"); + let fmode = std::fs::metadata(&file).unwrap().permissions().mode() & 0o777; + assert_eq!(fmode, 0o600, "recovery file is 0600, not umask default"); + let dmode = std::fs::metadata(dir.join("autosave")) + .unwrap() + .permissions() + .mode() + & 0o777; + assert_eq!(dmode, 0o700, "autosave dir is 0700"); + + // Rewriting keeps the private mode (save_atomic inherits it). + write_private(&dir, "autosave/secret", b"more").unwrap(); + let fmode = std::fs::metadata(&file).unwrap().permissions().mode() & 0o777; + assert_eq!(fmode, 0o600); + + // The plain `write` path is unchanged (umask default, not 0600). + write(&dir, "plain", b"x").unwrap(); + let pmode = std::fs::metadata(dir.join("plain")) + .unwrap() + .permissions() + .mode() + & 0o777; + assert_ne!(pmode, 0o600, "plain write keeps existing behavior"); + std::fs::remove_dir_all(&dir).ok(); + } + #[test] fn blank_xdg_falls_through_to_home_not_a_relative_path() { // The Q#PS2 fix: an empty / whitespace XDG_STATE_HOME must NOT diff --git a/tests/autosave_acceptance.rs b/tests/autosave_acceptance.rs new file mode 100644 index 0000000..e420b39 --- /dev/null +++ b/tests/autosave_acceptance.rs @@ -0,0 +1,456 @@ +//! Autosave + crash-recovery acceptance (Arc 3 phase 3). +//! +//! Each test injects a private tempdir `StateDir` (integration tests link +//! the lib without `cfg(test)`), so nothing touches a developer's real +//! state dir. Sweeps are driven directly rather than through the timer, +//! so the 1-second interval floor never slows the suite. +//! +//! Framing: `docs/autosave-recovery-framing.md`. + +use pmacs::editor::EditorState; +use pmacs::lua_bindings::StateDir; +use std::path::PathBuf; +use std::sync::atomic::{AtomicUsize, Ordering}; + +fn fresh_state_dir() -> PathBuf { + static SEQ: AtomicUsize = AtomicUsize::new(0); + let dir = std::env::temp_dir().join(format!( + "pmacs-autosave-{}-{}", + std::process::id(), + SEQ.fetch_add(1, Ordering::Relaxed) + )); + std::fs::create_dir_all(&dir).unwrap(); + dir +} + +fn editor(state_dir: &std::path::Path) -> EditorState { + let s = EditorState::new(); + s.lua_host.lua().remove_app_data::(); + s.lua_host + .lua() + .set_app_data(StateDir(state_dir.to_path_buf())); + s +} + +fn write_file(dir: &std::path::Path, name: &str, body: &str) -> String { + let p = dir.join(name); + std::fs::write(&p, body).unwrap(); + p.display().to_string() +} + +fn exec(s: &EditorState, src: &str) { + s.lua_host.lua().load(src.to_string()).exec().unwrap(); +} + +fn eval(s: &EditorState, src: &str) -> T { + s.lua_host.lua().load(src.to_string()).eval().unwrap() +} + +/// Force a sweep; returns how many buffers were written. +fn sweep(s: &EditorState) -> i64 { + eval(s, "return pmacs.autosave.sweep()") +} + +fn status(s: &EditorState, path: &str) -> String { + eval(s, &format!("return pmacs.autosave._status({path:?})")) +} + +/// Open a file and dirty it by `n` inserted bytes at the front. +fn open_and_dirty(s: &EditorState, path: &str, text: &str) { + exec( + s, + &format!("pmacs.buffer.find_or_open({path:?}); pmacs.window.buffer():insert(0, {text:?})"), + ); +} + +#[test] +fn sweep_writes_recovery_for_a_modified_file_buffer() { + let dir = fresh_state_dir(); + let s = editor(&dir); + let f = write_file(&dir, "a.txt", "on disk\n"); + open_and_dirty(&s, &f, "unsaved "); + assert_eq!(sweep(&s), 1, "one modified file buffer written"); + assert_eq!(status(&s, &f), "fresh"); + + // The recovery contents are the buffer's, not the file's. + let bytes: mlua::String = eval(&s, &format!("return pmacs.autosave._recover_bytes({f:?})")); + assert_eq!(&*bytes.as_bytes(), b"unsaved on disk\n"); + std::fs::remove_dir_all(&dir).ok(); +} + +#[test] +fn envelope_round_trips_non_utf8_contents() { + let dir = fresh_state_dir(); + let s = editor(&dir); + let f = write_file(&dir, "bin.dat", ""); + // 0xff is invalid UTF-8; the envelope reads bytes, not a String. + exec( + &s, + &format!( + "pmacs.buffer.find_or_open({f:?}); pmacs.window.buffer():insert(0, '\\255\\n\\0a')" + ), + ); + assert_eq!(sweep(&s), 1); + let bytes: mlua::String = eval(&s, &format!("return pmacs.autosave._recover_bytes({f:?})")); + assert_eq!(&*bytes.as_bytes(), &[0xffu8, b'\n', 0x00, b'a'][..]); + std::fs::remove_dir_all(&dir).ok(); +} + +#[test] +fn sweep_skips_clean_scratch_and_unchanged_buffers() { + let dir = fresh_state_dir(); + let s = editor(&dir); + let f = write_file(&dir, "a.txt", "hello\n"); + + // A clean file buffer + the scratch buffer: nothing to write. + exec(&s, &format!("pmacs.buffer.find_or_open({f:?})")); + assert_eq!(sweep(&s), 0, "clean buffer and scratch are skipped"); + + // Dirty it → one write. Sweeping again with no further edit → zero + // (the (path_hash, revision) skip cache). + exec(&s, "pmacs.window.buffer():insert(0, 'x')"); + assert_eq!(sweep(&s), 1); + assert_eq!(sweep(&s), 0, "unchanged since last copy → no rewrite"); + + // Another edit → written again. + exec(&s, "pmacs.window.buffer():insert(0, 'y')"); + assert_eq!(sweep(&s), 1); + std::fs::remove_dir_all(&dir).ok(); +} + +#[test] +fn path_change_writes_new_key_and_discards_the_old() { + let dir = fresh_state_dir(); + let s = editor(&dir); + let old = write_file(&dir, "old.txt", "body\n"); + let new = dir.join("new.txt").display().to_string(); + open_and_dirty(&s, &old, "dirty "); + assert_eq!(sweep(&s), 1); + assert_eq!(status(&s, &old), "fresh"); + + // Rename WITHOUT editing the buffer — what an LSP WorkspaceEdit + // rename does: the file moves on disk (preserving mtime/size) and the + // buffer keeps its BufferId *and* its revision, only its path changes. + std::fs::rename(&old, &new).unwrap(); + { + let id = s.core.borrow().active_buffer_id(); + s.core + .borrow_mut() + .set_buffer_path(id, Some(PathBuf::from(&new))); + } + // A revision-only cache would skip this write, never create the + // recovery under the new key, and orphan the old one. + assert_eq!(sweep(&s), 1, "path change forces a rewrite"); + assert_eq!(status(&s, &new), "fresh", "new key written"); + assert_eq!(status(&s, &old), "none", "old key discarded"); + let bytes: mlua::String = eval( + &s, + &format!("return pmacs.autosave._recover_bytes({new:?})"), + ); + assert_eq!(&*bytes.as_bytes(), b"dirty body\n"); + std::fs::remove_dir_all(&dir).ok(); +} + +#[test] +fn new_file_buffer_is_swept_with_null_origin_and_recovers() { + let dir = fresh_state_dir(); + let s = editor(&dir); + // A `[new file]`: a path with no file on disk, so no `file_meta`. + // Lua's find_or_open *errors* on a missing path, so this is built the + // way argv `pmacs draft.txt` does — an empty buffer with a path. + let missing = dir.join("draft.txt"); + exec( + &s, + "_G.nb = pmacs.buffer.create('draft.txt'); pmacs.window.switch_buffer(_G.nb)", + ); + { + let id = s.core.borrow().active_buffer_id(); + s.core + .borrow_mut() + .set_buffer_path(id, Some(missing.clone())); + } + // Typing into it is what makes it modified (and worth recovering). + exec(&s, "pmacs.window.buffer():insert(0, 'unsaved draft')"); + let p = missing.display().to_string(); + assert_eq!(sweep(&s), 1, "a new-file buffer is swept"); + // origin is null → fresh while the file is still absent. + assert_eq!(status(&s, &p), "fresh"); + let bytes: mlua::String = eval(&s, &format!("return pmacs.autosave._recover_bytes({p:?})")); + assert_eq!(&*bytes.as_bytes(), b"unsaved draft"); + + // Someone creates the file meanwhile → stale, never auto-offered. + std::fs::write(&missing, b"theirs").unwrap(); + assert_eq!(status(&s, &p), "stale"); + std::fs::remove_dir_all(&dir).ok(); +} + +#[test] +fn external_change_makes_recovery_stale() { + let dir = fresh_state_dir(); + let s = editor(&dir); + let f = write_file(&dir, "a.txt", "original\n"); + open_and_dirty(&s, &f, "mine "); + assert_eq!(sweep(&s), 1); + assert_eq!(status(&s, &f), "fresh"); + + // Someone else edits the file on disk. + std::fs::write(&f, b"theirs, quite different\n").unwrap(); + assert_eq!(status(&s, &f), "stale", "never auto-offered"); + std::fs::remove_dir_all(&dir).ok(); +} + +#[test] +fn corrupt_recovery_is_typed_quiet_and_discardable() { + let dir = fresh_state_dir(); + let s = editor(&dir); + let f = write_file(&dir, "a.txt", "body\n"); + exec(&s, &format!("pmacs.buffer.find_or_open({f:?})")); + + // Plant a malformed envelope under the right key. + let key = pmacs::autosave::key_for(std::path::Path::new(&f)); + pmacs::state::write_private(&dir, &key, b"garbage without a newline").unwrap(); + assert_eq!(status(&s, &f), "corrupt"); + + // The aggregate report must not error or announce it. + let (fresh, corrupt): (Vec, i64) = eval(&s, "return pmacs.autosave._pending()"); + assert!(fresh.is_empty(), "corrupt is never offered"); + assert_eq!(corrupt, 1, "counted separately"); + + // And it is discardable. + exec(&s, &format!("pmacs.autosave._discard({f:?})")); + assert_eq!(status(&s, &f), "none"); + std::fs::remove_dir_all(&dir).ok(); +} + +#[test] +fn pending_aggregates_and_names_a_single_file() { + let dir = fresh_state_dir(); + let s = editor(&dir); + let mut paths = Vec::new(); + for i in 0..3 { + let f = write_file(&dir, &format!("f{i}.txt"), "body\n"); + open_and_dirty(&s, &f, "x"); + paths.push(f); + } + assert_eq!(sweep(&s), 3); + let (fresh, corrupt): (Vec, i64) = eval(&s, "return pmacs.autosave._pending()"); + assert_eq!(fresh.len(), 3, "all three reported in ONE call"); + assert_eq!(corrupt, 0); + std::fs::remove_dir_all(&dir).ok(); +} + +#[test] +fn tick_reports_recoveries_once_aggregated() { + let dir = fresh_state_dir(); + // Seed three recovery copies, then "crash" and reopen the files. + let s1 = editor(&dir); + let mut paths = Vec::new(); + for i in 0..3 { + let f = write_file(&dir, &format!("f{i}.txt"), "body\n"); + open_and_dirty(&s1, &f, "x"); + paths.push(f); + } + assert_eq!(sweep(&s1), 3); + + let s2 = editor(&dir); + for f in &paths { + exec(&s2, &format!("pmacs.buffer.find_or_open({f:?})")); + } + // Each `after-load` only raises a flag; the tick does the reporting, + // so three loads collapse into ONE aggregate message. + exec(&s2, "pmacs.hook.run('process.after-tick')"); + let status = s2.core.borrow().status.clone(); + assert!( + status.contains("3 files have autosave recovery"), + "one aggregated message, not three: {status:?}" + ); + + // A second tick does not re-report (the flag was cleared). + s2.core.borrow_mut().status.clear(); + exec(&s2, "pmacs.hook.run('process.after-tick')"); + assert!(s2.core.borrow().status.is_empty(), "no repeat report"); + std::fs::remove_dir_all(&dir).ok(); +} + +#[test] +fn tick_names_the_file_when_exactly_one_is_recoverable() { + let dir = fresh_state_dir(); + let s1 = editor(&dir); + let f = write_file(&dir, "solo.txt", "body\n"); + open_and_dirty(&s1, &f, "x"); + assert_eq!(sweep(&s1), 1); + + let s2 = editor(&dir); + exec(&s2, &format!("pmacs.buffer.find_or_open({f:?})")); + exec(&s2, "pmacs.hook.run('process.after-tick')"); + let status = s2.core.borrow().status.clone(); + assert!( + status.contains("solo.txt") && status.contains("recover-file"), + "single recovery names the file: {status:?}" + ); + std::fs::remove_dir_all(&dir).ok(); +} + +#[test] +fn save_and_kill_delete_the_recovery_copy() { + let dir = fresh_state_dir(); + let s = editor(&dir); + + // Clean save deletes it (buffer.after-save). + let f = write_file(&dir, "a.txt", "body\n"); + open_and_dirty(&s, &f, "x"); + assert_eq!(sweep(&s), 1); + exec(&s, "pmacs.command.invoke('buffer.save')"); + assert_eq!(status(&s, &f), "none", "clean save retires the recovery"); + + // Kill deletes it (per-buffer on_removed registered at after-load). + let g = write_file(&dir, "b.txt", "body\n"); + exec(&s, &format!("_G.gb = pmacs.buffer.find_or_open({g:?})")); + exec(&s, "pmacs.window.buffer():insert(0, 'x')"); + assert_eq!(sweep(&s), 1); + assert_eq!(status(&s, &g), "fresh"); + exec(&s, "pmacs.buffer.kill(_G.gb)"); + assert_eq!(status(&s, &g), "none", "kill retires the recovery"); + std::fs::remove_dir_all(&dir).ok(); +} + +#[test] +fn recover_file_installs_contents_fires_after_edit_and_leaves_modified() { + let dir = fresh_state_dir(); + let s = editor(&dir); + let f = write_file(&dir, "a.txt", "on disk\n"); + open_and_dirty(&s, &f, "recovered "); + assert_eq!(sweep(&s), 1); + + // Simulate the crash-then-reopen: a fresh editor over the same store, + // opening the file whose on-disk contents are the OLD ones. + let s2 = editor(&dir); + exec( + &s2, + r#" + _G.after_edit = 0 + pmacs.hook.add("buffer.after-edit", function() _G.after_edit = _G.after_edit + 1 end) + "#, + ); + exec(&s2, &format!("pmacs.buffer.find_or_open({f:?})")); + assert_eq!(status(&s2, &f), "fresh"); + // The buffer still holds the on-disk contents (no silent substitution). + let before: mlua::String = eval( + &s2, + "local b = pmacs.window.buffer(); return b:slice(0, b:len())", + ); + assert_eq!(&*before.as_bytes(), b"on disk\n"); + + // Drive recover-file's accept path directly (the command opens a + // minibuffer; we exercise what its on_accept does). + exec( + &s2, + &format!( + r#" + local bytes = pmacs.autosave._recover_bytes({f:?}) + local b = pmacs.window.buffer() + b:replace(0, b:len(), bytes) + pmacs.hook.run("buffer.after-edit") + "# + ), + ); + let after: mlua::String = eval( + &s2, + "local b = pmacs.window.buffer(); return b:slice(0, b:len())", + ); + assert_eq!(&*after.as_bytes(), b"recovered on disk\n"); + let fired: i64 = eval(&s2, "return _G.after_edit"); + assert!( + fired >= 1, + "after-edit fired so LSP/syntax see the recovery" + ); + let modified: bool = eval(&s2, "return pmacs.window.buffer():is_modified()"); + assert!( + modified, + "recovered buffer is dirty; user must save to keep" + ); + std::fs::remove_dir_all(&dir).ok(); +} + +#[test] +fn recovery_files_are_private_0600_under_a_0700_dir() { + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt as _; + let dir = fresh_state_dir(); + let s = editor(&dir); + let f = write_file(&dir, "secret.txt", ""); + open_and_dirty(&s, &f, "unsaved secret"); + assert_eq!(sweep(&s), 1); + + let key = pmacs::autosave::key_for(std::path::Path::new(&f)); + let file = dir.join(&key); + let fmode = std::fs::metadata(&file).unwrap().permissions().mode() & 0o777; + assert_eq!(fmode, 0o600, "recovery file holds unsaved contents"); + let dmode = std::fs::metadata(dir.join("autosave")) + .unwrap() + .permissions() + .mode() + & 0o777; + assert_eq!(dmode, 0o700); + std::fs::remove_dir_all(&dir).ok(); + } +} + +#[test] +fn interval_is_a_validated_getter_setter_and_enable_gates_the_sweep() { + let dir = fresh_state_dir(); + let s = editor(&dir); + + let default_ms: i64 = eval(&s, "return pmacs.autosave.interval_ms()"); + assert_eq!(default_ms, 30000, "Emacs's auto-save-timeout"); + + let set: i64 = eval(&s, "return pmacs.autosave.interval_ms(60000)"); + assert_eq!(set, 60000); + let read_back: i64 = eval(&s, "return pmacs.autosave.interval_ms()"); + assert_eq!(read_back, 60000, "change takes effect immediately"); + + // Floats floor; bad values error. + let floored: i64 = eval(&s, "return pmacs.autosave.interval_ms(1500.9)"); + assert_eq!(floored, 1500); + for bad in ["'soon'", "0", "999", "-1", "{}"] { + let ok: bool = eval( + &s, + &format!("return (pcall(pmacs.autosave.interval_ms, {bad}))"), + ); + assert!(!ok, "interval_ms({bad}) must be rejected"); + } + // A rejected set leaves the previous value intact. + let still: i64 = eval(&s, "return pmacs.autosave.interval_ms()"); + assert_eq!(still, 1500); + + // enable(false) makes sweep a no-op even with a dirty buffer. + let f = write_file(&dir, "a.txt", "body\n"); + open_and_dirty(&s, &f, "x"); + exec(&s, "pmacs.autosave.enable(false)"); + assert_eq!(sweep(&s), 0, "disabled → no sweep"); + assert_eq!(status(&s, &f), "none"); + exec(&s, "pmacs.autosave.enable(true)"); + assert_eq!(sweep(&s), 1, "re-enabled → sweeps"); + std::fs::remove_dir_all(&dir).ok(); +} + +#[test] +fn before_quit_sweeps_synchronously_without_vetoing() { + let dir = fresh_state_dir(); + let s = editor(&dir); + let f = write_file(&dir, "a.txt", "body\n"); + open_and_dirty(&s, &f, "unsaved "); + // Not swept yet. + assert_eq!(status(&s, &f), "none"); + + // before-quit is short-circuit: a `true` result means "not vetoed". + let not_vetoed: bool = eval(&s, "return pmacs.hook.run('editor.before-quit')"); + assert!(not_vetoed, "autosave must never veto quit"); + assert_eq!( + status(&s, &f), + "fresh", + "quitting with unsaved changes leaves a recovery copy" + ); + std::fs::remove_dir_all(&dir).ok(); +} From 1205329c341d43f597e18792f1bc02447750aa94 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Thu, 9 Jul 2026 10:52:10 -0400 Subject: [PATCH 3/6] fix(persistence): never clobber unclaimed crash data; buffer-keyed cleanup Addresses the PR #100 review. - HIGH data loss: sweep could overwrite an existing crash recovery before the user ran recover-file. Reopen a file after a crash, edit it, and the next autosave wrote the current buffer over the recovery key --- losing exactly what autosave exists to protect. New ownership rule (Q#AS12): 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; the sweep refuses to write that buffer, counts it `blocked`, and says so ("autosave paused for N file(s) with unclaimed recovery"). recover-file ADOPTS the copy once its contents are in the buffer; discard-recovery removes it. Either resumes normal autosave. sweep() now returns (written, blocked). - MEDIUM cleanup missed paths autosave can write. Kill/save cleanup now goes through `discard_buffer(BufferId)`, which removes BOTH the buffer's current-path key and the key its last sweep actually wrote (they differ after a rename --- an LSP WorkspaceEdit changes the path while the BufferId stays; a path-captured callback deleted the wrong key). And a sweep-time GC deletes the recovery of any buffer that left the registry, which is the backstop for argv `[new file]` buffers: they fire no after-load, so no removal callback is ever registered for them. - LOW/MEDIUM recover-file pinned only on the active path. Two buffers can visit one path (pmacs.buffer.from_file does not dedup), so focus drift could recover into the wrong buffer. It now captures and compares the origin buffer handle as well as the path. - LOW write_private left a pre-existing lax autosave/ directory alone. The birth-mode only applies to dirs that call creates, so a 0755 autosave/ from an older run still leaked recovery-file names, sizes, and mtimes despite 0600 contents. It is now tightened to 0700 --- but never `base` itself, which is shared with history/recentf/desktop and may predate us. New `state::exists` (an existence check, no read) backs the ownership gate. Tests (autosave_acceptance now 20): sweep_never_overwrites_unclaimed_ crash_recovery (blocked, crash copy byte-identical, adopt resumes), discarding_an_unclaimed_recovery_unblocks_the_sweep, killing_a_new_file_buffer_gcs_its_recovery, saving_after_a_rename_removes_the_recovery_written_under_the_old_path, a_pre_existing_lax_autosave_dir_is_tightened. Gates: fmt + workspace clippy clean; lib 1499; crdt 1670; autosave 20; desktop 11; persistence 5; m4 90; m7_8 5; GPU 58; git diff --check clean. Co-Authored-By: Claude Opus 4.8 (1M context) --- builtin/runtime/autosave.lua | 59 ++++++++---- docs/autosave-recovery-framing.md | 69 +++++++++++-- src/autosave.rs | 155 +++++++++++++++++++++++++++--- src/lua_bindings/mod.rs | 33 ++++++- src/state.rs | 42 +++++++- tests/autosave_acceptance.rs | 150 +++++++++++++++++++++++++++++ 6 files changed, 457 insertions(+), 51 deletions(-) diff --git a/builtin/runtime/autosave.lua b/builtin/runtime/autosave.lua index 7f005b7..d66e15d 100644 --- a/builtin/runtime/autosave.lua +++ b/builtin/runtime/autosave.lua @@ -48,10 +48,19 @@ function pmacs.autosave.interval_ms(ms) return interval end --- sweep() --- force a pass now. Returns how many buffers were written. +-- 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. function pmacs.autosave.sweep() - if not enabled then return 0 end - return pmacs.autosave._sweep() + if not enabled then return 0, 0 end + local written, blocked = 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") + end + return written, blocked end local function basename(path) @@ -90,7 +99,7 @@ pmacs.hook.add("process.after-tick", function() end if now - last_sweep_ms >= interval then last_sweep_ms = now - pcall(pmacs.autosave._sweep) + pcall(pmacs.autosave.sweep) end end) @@ -98,21 +107,24 @@ end) -- inside the hook -- a desktop restore fires this once per leaf). pmacs.hook.add("buffer.after-load", function() needs_report = true - -- A clean save or a kill retires the recovery copy. There is no global - -- kill hook, so register per buffer, capturing the path now (the buffer - -- may be gone by the time the callback runs). - local path = pmacs.editor.file_path() - if not path then return end + -- A kill retires the recovery copy. There is no global kill hook, so + -- register per buffer. `_discard_buffer` is keyed by BufferId, not by a + -- path captured here: after a rename the buffer's recovery lives under + -- a different key than the path it loaded with. Buffers that fire no + -- after-load (argv `[new file]`) are covered by the sweep-time GC. local buf = pmacs.window.buffer() if not buf then return end - pcall(pmacs.buffer.on_removed, buf, function() - pcall(pmacs.autosave._discard, path) + pcall(pmacs.buffer.on_removed, buf, function(dead) + pcall(pmacs.autosave._discard_buffer, dead or buf) end) end) +-- A clean save retires the recovery copy. Keyed by buffer, so a renamed +-- buffer's real recovery key (written under the *old* path) is removed +-- too, not just the current path's. pmacs.hook.add("buffer.after-save", function() - local path = pmacs.editor.file_path() - if path then pcall(pmacs.autosave._discard, path) end + local buf = pmacs.window.buffer() + if buf then pcall(pmacs.autosave._discard_buffer, buf) end end) -- A final synchronous sweep on quit: async ticks stop after this, so a @@ -144,6 +156,14 @@ pmacs.command.define { if st == "stale" then warn = " [WARNING: file changed on disk since the autosave]" end + -- Pin to the exact BUFFER we started on, not merely its path: two + -- buffers can visit the same path (`pmacs.buffer.from_file` does not + -- dedup), so a path check alone could recover into the wrong one. + local origin_buf = pmacs.window.buffer() + if not origin_buf then + pmacs.editor.set_status("recover-file: no buffer") + return + end pmacs.minibuffer.read { prompt = "Recover from autosave?" .. warn .. " (yes/no): ", source = function() return { "yes", "no" } end, @@ -152,10 +172,10 @@ pmacs.command.define { pmacs.editor.set_status("recover-file: cancelled") return end - -- Pin to the buffer we started on: focus can drift while the - -- prompt is up, and recovering into the wrong buffer is - -- unrecoverable. - if pmacs.editor.file_path() ~= path then + -- Focus can drift while the prompt is up, and recovering into the + -- wrong buffer is unrecoverable. + local buf = pmacs.window.buffer() + if buf ~= origin_buf or pmacs.editor.file_path() ~= path then pmacs.editor.set_status("recover-file: buffer changed; aborted") return end @@ -164,7 +184,6 @@ pmacs.command.define { pmacs.editor.set_status("recover-file: recovery unreadable") return end - local buf = pmacs.window.buffer() buf:replace(0, buf:len(), bytes) -- The mutators notify windows and queue CRDT but do NOT fire -- `buffer.after-edit` --- that comes from dispatch_key's @@ -172,6 +191,10 @@ pmacs.command.define { -- 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 9a33ea7..aaec4e6 100644 --- a/docs/autosave-recovery-framing.md +++ b/docs/autosave-recovery-framing.md @@ -235,8 +235,11 @@ So the report is **pull-based and aggregated on the tick we already own** Recovery itself happens through an explicit command: -- **`recover-file`** — confirms via `minibuffer.read` (typed `yes`), then - `buf:replace(0, buf:len(), recovery_bytes)` on the active buffer, +- **`recover-file`** — confirms via `minibuffer.read` (typed `yes`), + **pins to the origin *buffer handle*, not merely its path** (finding: + `pmacs.buffer.from_file` does not dedup, so two buffers can visit one + path and a path check alone could recover into the wrong one), then + `buf:replace(0, buf:len(), recovery_bytes)`, **then explicitly `pmacs.hook.run("buffer.after-edit")`**. That last step is load-bearing (finding): the mutators only notify windows and queue CRDT, and `after-edit` is fired by `dispatch_key`'s post-command @@ -252,14 +255,45 @@ Recovery itself happens through an explicit command: This also sidesteps re-entrancy: no modal surface is opened from inside a hook fired by Rust. -### Q#AS7 — Cleanup lifecycle +### Q#AS12 — Never overwrite unclaimed crash data (the ownership rule) -- **`buffer.after-save`** → `discard(active path)`. A clean save means the - recovery copy is obsolete. (Hook exists, active-buffer, no args needed.) -- **Buffer killed** → `discard(path)`. There is no global kill hook, so - `after-load` registers a per-buffer `pmacs.buffer.on_removed(id, fn)` - closing over the path (captured then, since the buffer may be gone when - the callback runs). Killing a modified buffer is a deliberate discard. +The failure this closes (finding): you crash with unsaved work, reopen +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: + +- 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. + +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 +edits to a second crash is recoverable by retyping; losing the original +crash copy is not. + +### Q#AS7 — Cleanup lifecycle (keyed by buffer, not by a captured path) + +- **`buffer.after-save`** → `discard_buffer(active buffer)`. +- **Buffer killed** → `discard_buffer(id)`. There is no global kill hook, + so `after-load` registers a per-buffer `pmacs.buffer.on_removed`. +- Both go through **`discard_buffer(BufferId)`**, not a path captured at + load time (finding). It removes *both* the buffer's current-path key + and the key its last sweep actually **wrote** under — which differ + after a rename (an LSP `WorkspaceEdit` changes the path while the + `BufferId` stays). A path-captured callback would delete the wrong key + and leave the real recovery file behind. +- **Sweep-time GC** is the backstop: any cache entry whose `BufferId` has + left the registry has its recovery file deleted. This is what covers + argv **`[new file]`** buffers, which fire no `after-load` and so never + get a removal callback registered (finding). - **`editor.before-quit`** → one **final synchronous sweep**, then return nil (never veto). Async ticks stop after quit, so this must be a direct call. Result: quitting with unsaved changes leaves a recovery copy that @@ -311,7 +345,13 @@ So this PR makes autosave storage private: momentarily visible at `0644`. (A chmod-after-write leaves exactly that window.) Plain `save_atomic` delegates with `None`. - **`state::write_private(base, name, content)`** — creates the parent - with `DirBuilder::mode(0o700)` and writes the file `0600`. + with `DirBuilder::mode(0o700)` and writes the file `0600`. It also + **tightens a pre-existing lax `autosave/`** to `0700` (finding): the + birth-mode only applies to directories *that call* creates, so a + `0755` directory left by an older run would still leak recovery-file + names, sizes, and mtimes despite `0600` contents. It never re-modes + `base` itself — the state root is shared with history/recentf/desktop + and may predate us. - Recovery files use it; the `autosave/` directory is `0700`. - Unix-only (`PermissionsExt` / `DirBuilderExt` are safe under `#![forbid(unsafe_code)]`); on other platforms it degrades to today's @@ -400,10 +440,19 @@ commands, cleanup wiring) → tests. each recovery file is `0600` — asserted, not assumed. - Sweep skips clean buffers, scratch buffers, and buffers unchanged since the last sweep (no second write). +- **Unclaimed crash data is never overwritten (Q#AS12)**: session 1 + 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. - **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). - `after-save` → recovery deleted. Kill buffer → recovery deleted. +- **Rename then save with no intervening sweep** → the recovery written + under the *old* key is removed (buffer-keyed cleanup, Q#AS7). +- **Killing a `[new file]` buffer** → the sweep-time GC removes its + recovery (no `after-load` fired, so no removal callback exists). +- **A pre-existing `0755` `autosave/` dir is tightened to `0700`.** - Open a file with a **`Fresh`** recovery → the aggregate report names it; buffer contents are still the on-disk ones (no silent substitution). diff --git a/src/autosave.rs b/src/autosave.rs index bfccb03..6c8e37e 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; +use std::collections::{HashMap, HashSet}; use std::path::Path; use mlua::Lua; @@ -123,15 +123,33 @@ pub fn key_for(path: &Path) -> String { format!("autosave/{}", sha256_hex(&path.display().to_string())) } -/// Skip cache: `BufferId → (path_hash, revision)` (Q#AS8). -/// -/// Keyed on the **path hash as well as the revision**, not the revision -/// alone: a buffer keeps its `BufferId` across a path change (an LSP -/// `WorkspaceEdit` rename calls `set_buffer_path`), so a revision-only -/// cache would skip the write, never create the recovery file under the -/// new key, and orphan the old one. +/// Per-session autosave bookkeeping. #[derive(Default)] -pub struct AutosaveCache(RefCell>); +pub struct AutosaveCache(RefCell); + +#[derive(Default)] +struct CacheInner { + /// Skip cache: `BufferId → (path_hash, revision)` (Q#AS8). + /// + /// Keyed on the **path hash as well as the revision**, not the + /// revision alone: a buffer keeps its `BufferId` across a path change + /// (an LSP `WorkspaceEdit` rename calls `set_buffer_path`), so a + /// revision-only cache would skip the write, never create the + /// recovery file under the new key, and orphan the old one. It also + /// 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). + /// + /// 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, +} /// Encode a header + contents into the one-file envelope. fn encode(header: &Header, contents: &[u8]) -> Result, String> { @@ -220,11 +238,15 @@ 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). +/// /// # Errors /// A state-write failure. Individual buffers never abort the pass. -pub fn sweep(lua: &Lua) -> Result { +pub fn sweep(lua: &Lua) -> Result<(usize, usize), String> { let Some(base) = base_dir(lua) else { - return Ok(0); + return Ok((0, 0)); }; let core = lua .app_data_ref::() @@ -235,6 +257,7 @@ pub fn sweep(lua: &Lua) -> Result { let mut writes: Vec = Vec::new(); let mut orphans: Vec = Vec::new(); let mut live: Vec = Vec::new(); + let mut blocked = 0usize; { let cache = lua .app_data_ref::() @@ -256,7 +279,7 @@ pub fn sweep(lua: &Lua) -> Result { let path_s = path.display().to_string(); let path_hash = sha256_hex(&path_s); let revision = buf.revision(); - if let Some((prev_hash, prev_rev)) = cache.get(&id) { + 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 } @@ -265,6 +288,16 @@ pub fn sweep(lua: &Lua) -> Result { 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; + } let len = buf.len(); let mut contents = vec![0u8; usize::try_from(len).unwrap_or(0)]; if len > 0 { @@ -291,8 +324,21 @@ pub fn sweep(lua: &Lua) -> Result { .app_data_ref::() .ok_or("no autosave cache")?; let mut cache = cache.0.borrow_mut(); - // Drop entries for buffers that no longer exist. - cache.retain(|id, _| live.contains(id)); + // 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, @@ -302,11 +348,88 @@ pub fn sweep(lua: &Lua) -> Result { let bytes = encode(&header, &p.contents)?; crate::state::write_private(&base, &format!("autosave/{}", p.path_hash), &bytes) .map_err(|e| e.to_string())?; - cache.insert(p.id, (p.path_hash, p.revision)); + cache.owned.insert(p.path_hash.clone()); + cache.written.insert(p.id, (p.path_hash, p.revision)); written += 1; } } - Ok(written) + 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) { + if let Some(cache) = lua.app_data_ref::() { + cache + .0 + .borrow_mut() + .owned + .insert(sha256_hex(&path.display().to_string())); + } +} + +/// 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. +pub fn discard_path(lua: &Lua, path: &Path) -> bool { + let Some(base) = base_dir(lua) else { + return false; + }; + unown(lua, path); + 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. +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. + if let Some(cache) = lua.app_data_ref::() + && let Some((hash, _)) = cache.0.borrow().written.get(&id) + { + keys.push(hash.clone()); + } + // The buffer's current path, which may have moved since that write. + if let Some(core) = lua.app_data_ref::() { + let c = core.borrow(); + let reg = c.registry.borrow(); + if let Ok(buf) = reg.get(id) + && let Some(p) = buf.file_path() + { + keys.push(sha256_hex(&p.display().to_string())); + } + } + for hash in &keys { + let _ = crate::state::remove(&base, &format!("autosave/{hash}")); + } + if let Some(cache) = lua.app_data_ref::() { + let mut cache = cache.0.borrow_mut(); + cache.written.remove(&id); + for hash in &keys { + cache.owned.remove(hash); + } + } } /// Every open file buffer that has a recovery file, with its status diff --git a/src/lua_bindings/mod.rs b/src/lua_bindings/mod.rs index 6878d08..25dbfb1 100644 --- a/src/lua_bindings/mod.rs +++ b/src/lua_bindings/mod.rs @@ -1981,11 +1981,35 @@ fn install_autosave_module(lua: &Lua) -> mlua::Result
{ lua.set_app_data(crate::autosave::AutosaveCache::default()); let m = lua.create_table()?; + // _sweep() -> (written, blocked). `blocked` counts buffers whose + // sweep was refused because unclaimed crash data sits at their key. m.set( "_sweep", 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. + m.set( + "_adopt", + lua.create_function(|lua, path: String| { + crate::autosave::adopt(lua, std::path::Path::new(&path)); + Ok(()) + })?, + )?; + + // _discard_buffer(buf): retire a buffer's recovery copy by BufferId — + // removes both its current-path key and the key its last sweep wrote + // (they differ after a rename). + m.set( + "_discard_buffer", + lua.create_function(|lua, id: BufferIdLua| { + crate::autosave::discard_buffer(lua, id.0); + Ok(()) + })?, + )?; + m.set( "_status", lua.create_function(|lua, path: String| { @@ -2009,13 +2033,14 @@ fn install_autosave_module(lua: &Lua) -> mlua::Result
{ })?, )?; + // _discard(path): delete a recovery file and drop any claim on it. m.set( "_discard", lua.create_function(|lua, path: String| { - let Some(base) = lua.app_data_ref::().map(|d| d.0.clone()) else { - return Ok(false); - }; - Ok(crate::autosave::discard(&base, std::path::Path::new(&path))) + Ok(crate::autosave::discard_path( + lua, + std::path::Path::new(&path), + )) })?, )?; diff --git a/src/state.rs b/src/state.rs index 22ba299..0a783b7 100644 --- a/src/state.rs +++ b/src/state.rs @@ -204,6 +204,15 @@ pub fn write_private(base: &Path, name: &str, content: &[u8]) -> Result<(), Stat write_inner(base, name, content, Some(0o600)) } +/// True when a state file exists (no read, no parse). +/// +/// # Errors +/// Invalid key. +pub fn exists(base: &Path, name: &str) -> Result { + let path = resolve(base, name).map_err(StateError::Name)?; + Ok(path.exists()) +} + fn write_inner( base: &Path, name: &str, @@ -213,14 +222,25 @@ fn write_inner( let path = resolve(base, name).map_err(StateError::Name)?; if let Some(parent) = path.parent() { create_dir_all_with_mode(parent, mode.map(|_| 0o700)).map_err(StateError::Io)?; + // A directory *we* own beneath the state root (e.g. `autosave/`) + // must actually be `0700`, even if a previous run — or a user — + // created it laxer. Otherwise the mode only applies to the dirs + // this call happened to create, and a pre-existing `0755` + // `autosave/` would still leak recovery-file names, sizes, and + // mtimes despite the `0600` contents. + // + // Never re-mode `base` itself: the state root is a directory the + // user may already have, shared with history/recentf/desktop. + if mode.is_some() && parent != base { + enforce_dir_mode(parent, 0o700).map_err(StateError::Io)?; + } } crate::file_io::save_atomic_with_mode(&path, content, mode).map_err(StateError::Save)?; Ok(()) } -/// `create_dir_all`, optionally forcing the mode of directories this call -/// creates. An already-existing directory keeps its mode (we do not -/// re-mode a directory the user already has). +/// `create_dir_all`, birthing any directory this call creates at `mode` +/// (so it is never briefly world-readable). fn create_dir_all_with_mode(dir: &Path, mode: Option) -> std::io::Result<()> { #[cfg(unix)] if let Some(m) = mode { @@ -235,6 +255,22 @@ fn create_dir_all_with_mode(dir: &Path, mode: Option) -> std::io::Result<() std::fs::create_dir_all(dir) } +/// Tighten an existing directory to `mode` if it is laxer. No-op on +/// non-Unix, and cheap when already correct. +fn enforce_dir_mode(dir: &Path, mode: u32) -> std::io::Result<()> { + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt as _; + let current = std::fs::metadata(dir)?.permissions().mode() & 0o777; + if current != mode { + std::fs::set_permissions(dir, std::fs::Permissions::from_mode(mode))?; + } + } + #[cfg(not(unix))] + let _ = (dir, mode); + Ok(()) +} + /// Read a state file's raw bytes, or `Ok(None)` when it does not exist. /// /// [`read`] returns a `String` (`read_to_string`), which fails on diff --git a/tests/autosave_acceptance.rs b/tests/autosave_acceptance.rs index e420b39..16f1d82 100644 --- a/tests/autosave_acceptance.rs +++ b/tests/autosave_acceptance.rs @@ -48,9 +48,23 @@ fn eval(s: &EditorState, src: &str) -> T { /// Force a sweep; returns how many buffers were written. fn sweep(s: &EditorState) -> i64 { + let (written, _blocked): (i64, i64) = eval(s, "return pmacs.autosave.sweep()"); + written +} + +/// Force a sweep; returns `(written, blocked)`. +fn sweep2(s: &EditorState) -> (i64, i64) { eval(s, "return pmacs.autosave.sweep()") } +fn recovered(s: &EditorState, path: &str) -> Vec { + let b: mlua::String = eval( + s, + &format!("return pmacs.autosave._recover_bytes({path:?})"), + ); + b.as_bytes().to_vec() +} + fn status(s: &EditorState, path: &str) -> String { eval(s, &format!("return pmacs.autosave._status({path:?})")) } @@ -239,6 +253,142 @@ fn pending_aggregates_and_names_a_single_file() { std::fs::remove_dir_all(&dir).ok(); } +#[test] +fn sweep_never_overwrites_unclaimed_crash_recovery() { + let dir = fresh_state_dir(); + // Session 1 crashes with unsaved work: a recovery copy is on disk. + 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); + assert_eq!(&crash_copy, b"CRASH WORK on disk\n"); + + // Session 2 reopens the file (on-disk contents) and edits it BEFORE + // running recover-file. Sweeping must NOT clobber the crash copy. + let s2 = editor(&dir); + exec(&s2, &format!("pmacs.buffer.find_or_open({f:?})")); + exec(&s2, "pmacs.window.buffer():insert(0, 'new edits ')"); + let (written, blocked) = sweep2(&s2); + assert_eq!(written, 0, "must not write over unclaimed crash data"); + assert_eq!(blocked, 1, "the sweep is blocked and reported"); + assert_eq!( + recovered(&s2, &f), + crash_copy, + "the crash recovery survives intact" + ); + 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:?})")); + 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"); + std::fs::remove_dir_all(&dir).ok(); +} + +#[test] +fn discarding_an_unclaimed_recovery_unblocks_the_sweep() { + 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 s2 = editor(&dir); + exec(&s2, &format!("pmacs.buffer.find_or_open({f:?})")); + exec(&s2, "pmacs.window.buffer():insert(0, 'mine ')"); + assert_eq!(sweep2(&s2), (0, 1), "blocked"); + + exec(&s2, &format!("pmacs.autosave._discard({f:?})")); + assert_eq!(sweep2(&s2), (1, 0), "discarded → sweeps again"); + assert_eq!(recovered(&s2, &f), b"mine on disk\n"); + std::fs::remove_dir_all(&dir).ok(); +} + +#[test] +fn killing_a_new_file_buffer_gcs_its_recovery() { + let dir = fresh_state_dir(); + let s = editor(&dir); + // A `[new file]` fires no after-load, so no per-buffer removal + // callback is registered — the sweep-time GC is the backstop. + let missing = dir.join("draft.txt"); + exec( + &s, + "_G.nb = pmacs.buffer.create('draft.txt'); pmacs.window.switch_buffer(_G.nb)", + ); + { + let id = s.core.borrow().active_buffer_id(); + s.core + .borrow_mut() + .set_buffer_path(id, Some(missing.clone())); + } + exec(&s, "pmacs.window.buffer():insert(0, 'draft')"); + let p = missing.display().to_string(); + assert_eq!(sweep(&s), 1); + assert_eq!(status(&s, &p), "fresh"); + + exec(&s, "pmacs.buffer.kill(_G.nb)"); + // The next sweep GCs the dead buffer's recovery copy. + sweep(&s); + assert_eq!( + status(&s, &p), + "none", + "killed new-file buffer is cleaned up" + ); + std::fs::remove_dir_all(&dir).ok(); +} + +#[test] +fn saving_after_a_rename_removes_the_recovery_written_under_the_old_path() { + let dir = fresh_state_dir(); + let s = editor(&dir); + let old = write_file(&dir, "old.txt", "body\n"); + let new = dir.join("new.txt").display().to_string(); + open_and_dirty(&s, &old, "dirty "); + assert_eq!(sweep(&s), 1, "recovery written under the OLD key"); + + // Rename, then save — without an intervening sweep. A path-captured + // cleanup would delete the new key and leave the old one behind. + std::fs::rename(&old, &new).unwrap(); + { + let id = s.core.borrow().active_buffer_id(); + s.core + .borrow_mut() + .set_buffer_path(id, Some(PathBuf::from(&new))); + } + exec(&s, "pmacs.command.invoke('buffer.save')"); + assert_eq!(status(&s, &old), "none", "old key removed"); + assert_eq!(status(&s, &new), "none", "new key removed"); + std::fs::remove_dir_all(&dir).ok(); +} + +#[test] +fn a_pre_existing_lax_autosave_dir_is_tightened() { + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt as _; + let dir = fresh_state_dir(); + // Someone (an older pmacs, or the user) left autosave/ at 0755. + let autosave_dir = dir.join("autosave"); + std::fs::create_dir_all(&autosave_dir).unwrap(); + std::fs::set_permissions(&autosave_dir, std::fs::Permissions::from_mode(0o755)).unwrap(); + + let s = editor(&dir); + let f = write_file(&dir, "a.txt", "body\n"); + open_and_dirty(&s, &f, "secret "); + assert_eq!(sweep(&s), 1); + + let dmode = std::fs::metadata(&autosave_dir) + .unwrap() + .permissions() + .mode() + & 0o777; + assert_eq!(dmode, 0o700, "a lax autosave dir is tightened, not left"); + std::fs::remove_dir_all(&dir).ok(); + } +} + #[test] fn tick_reports_recoveries_once_aggregated() { let dir = fresh_state_dir(); From b8a296f0a3a002703dba68cb90eb231bd99f5c20 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Thu, 9 Jul 2026 11:08:33 -0400 Subject: [PATCH 4/6] 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(); } From e173f61a61992245f163925388a8777cdace0e1d Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Thu, 9 Jul 2026 11:36:55 -0400 Subject: [PATCH 5/6] 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(); From c80e00e799817ec858ab02e7cb212faecc41dec3 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Thu, 9 Jul 2026 12:06:49 -0400 Subject: [PATCH 6/6] 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) --- builtin/runtime/autosave.lua | 33 ++++++++++++++--- docs/autosave-recovery-framing.md | 31 ++++++++++++++++ src/autosave.rs | 16 +++++++-- tests/autosave_acceptance.rs | 60 +++++++++++++++++++++++++++++++ 4 files changed, 134 insertions(+), 6 deletions(-) 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();