From e59dfada80b50f7738a2e02e4d33fbdb6e7c273b Mon Sep 17 00:00:00 2001 From: Graham Barber Date: Wed, 15 Jul 2026 09:13:31 -0700 Subject: [PATCH] propose add-auto-compaction: size and quit triggers for update-log compaction, verified snapshot replacement with .prev fallback, batched import on open --- .../add-auto-compaction/.openspec.yaml | 2 + .../changes/add-auto-compaction/design.md | 178 ++++++++++++++++++ .../changes/add-auto-compaction/proposal.md | 56 ++++++ .../specs/block-graph/spec.md | 69 +++++++ openspec/changes/add-auto-compaction/tasks.md | 32 ++++ 5 files changed, 337 insertions(+) create mode 100644 openspec/changes/add-auto-compaction/.openspec.yaml create mode 100644 openspec/changes/add-auto-compaction/design.md create mode 100644 openspec/changes/add-auto-compaction/proposal.md create mode 100644 openspec/changes/add-auto-compaction/specs/block-graph/spec.md create mode 100644 openspec/changes/add-auto-compaction/tasks.md diff --git a/openspec/changes/add-auto-compaction/.openspec.yaml b/openspec/changes/add-auto-compaction/.openspec.yaml new file mode 100644 index 0000000..4f63482 --- /dev/null +++ b/openspec/changes/add-auto-compaction/.openspec.yaml @@ -0,0 +1,2 @@ +schema: spec-driven +created: 2026-07-15 diff --git a/openspec/changes/add-auto-compaction/design.md b/openspec/changes/add-auto-compaction/design.md new file mode 100644 index 0000000..39563bb --- /dev/null +++ b/openspec/changes/add-auto-compaction/design.md @@ -0,0 +1,178 @@ +# Design: add-auto-compaction + +## Context + +The graph directory holds `snapshot.loro` (last full snapshot) plus +`updates.log` (length-prefixed incremental blobs since that snapshot). Every +acknowledged edit appends to the log via `GraphStorage::persist_update` +(`crates/trawler-core/src/storage.rs:109`). The fold-into-snapshot mechanism +already exists and is crash-safe — `compact()` (`storage.rs:133`) writes the +snapshot via atomic temp-file-then-rename, then deletes the log — but nothing +invokes it outside tests, so the log grows for the life of the graph and cold +start replays all of it. + +Prior art considered: remoras' op-log compaction spec +(`specs/27-op-log-compaction-design.md` in the remoras repo). Its checkpoint +format, frontier machinery, and sync protocol are things Loro already provides +trawler for free; only its *trigger policy* (size/op-count threshold + on-close ++ manual, explicitly no idle timer) transfers, and that is what this change +adopts. + +## Goals / Non-Goals + +**Goals:** + +- Bound `updates.log` so cold-start replay cost stays roughly constant + regardless of graph age. +- Make the next launch after a normal quit load (almost) purely from snapshot. +- Preserve the acknowledged-edit crash-safety contract exactly as specified in + `block-graph` ("Crash safety") — including when the process dies *during* + compaction. +- Keep the policy in `trawler-core` so it applies to every caller and is + testable headless. + +**Non-Goals:** + +- History trimming or content redaction. `ExportMode::Snapshot` retains full + Loro history; bounding *snapshot* growth (shallow snapshots) is a separate + future change. +- Idle/timer-based triggers. The size threshold already bounds replay time and + the quit trigger covers stale sessions; a timer adds a scheduling surface for + no additional bound (same conclusion remoras reached). +- Any UI, settings surface, or env-var configuration. + +## Decisions + +### D1 — The trigger lives inside `persist_update`, after acknowledgement + +After a successful append + fsync (the edit is durable and would be +acknowledged), `persist_update` checks the log file size and calls the +existing `compact()` if it has reached the threshold. + +- *Why in core, not the app layer:* every caller (app, fixtures, future tools) + gets the bound for free; the policy is unit-testable without GPUI. +- *Why after the append, not before:* the crash-safety contract is about + acknowledged edits; durability must never wait on a snapshot write. The + triggering edit is on disk (in the log) before compaction starts, so a crash + mid-compaction cannot lose it. +- *Alternative rejected:* a background thread. `GraphStorage` is single-threaded + by design (`RefCell`, `LoroDoc` on the UI thread); the inline cost is a + snapshot export — around tens of milliseconds at realistic personal-graph + scale, a couple hundred at 100k blocks — paid once per threshold crossing + (megabytes of edits), which is rare enough to accept for the MVP. Revisit if + it ever registers as a felt hitch. + +### D2 — Compaction failure never fails the persist + +If `compact()` errors after a successful append, `persist_update` still returns +`Ok`: the edit is durable, the graph is loadable (worst case: snapshot replaced +but log not yet deleted — see D4), and the next threshold crossing retries. +The error is reported on stderr (`trawler-core` has no logging facility; this +matches the crate's existing plain-`io` style). Callers currently +`.expect("persist …")` — a best-effort compaction must not turn into an app +crash. + +### D3 — Two named thresholds, one shared primitive + +`GraphStorage` gains `compact_if_log_exceeds(min_bytes: u64) -> io::Result` +(returns whether it compacted; no-op when the log is absent or smaller). Both +triggers are thin wrappers over it: + +- `AUTO_COMPACT_THRESHOLD_BYTES = 4 MiB` — checked by `persist_update`. Large + enough that steady typing doesn't thrash snapshot writes (megabytes of update + blobs ≈ thousands of edits), small enough that replaying a full log of this + size on open is far below perception. +- `QUIT_COMPACT_THRESHOLD_BYTES = 64 KiB` — used by the app's quit hook. Lower, + because quit-time compaction is free from a latency standpoint; but not zero, + so a session that made three edits doesn't rewrite a multi-MB snapshot on + every quit. Logs under 64 KiB replay instantly anyway. + +Tests pass explicit values to `compact_if_log_exceeds`, so no test-only +configuration field is needed. This mirrors the repo's "named knobs" style +(cf. the outline-layout constants commit). + +### D4 — Crash-safety ordering is inherited, with one new interleaving to pin + +`compact()` already writes the snapshot atomically (temp + fsync + rename) +*before* removing the log. The one new interleaving auto-compaction makes +likely enough to care about: a kill after the snapshot rename but before the +log deletion. Then `open()` loads the new snapshot and re-imports a log whose +updates are already contained in it — Loro import of already-known updates is +idempotent, so the result is identical. This gets a dedicated test in +`crash_safety.rs` (simulated by performing the snapshot write and skipping the +deletion, then re-opening) rather than being trusted silently. + +### D5 — Quit hook via `cx.on_app_quit` + +`TrawlerApp` owns `GraphStorage` directly (`main.rs:225`). During app setup, +register `cx.on_app_quit` with a weak handle to the `TrawlerApp` entity; the +callback upgrades it and calls `compact_if_log_exceeds(QUIT_COMPACT_THRESHOLD_BYTES)`. +If the entity is gone or compaction fails, quit proceeds regardless — quit-time +compaction is an optimization, never a gate on exiting. Note GPUI quit hooks +are best-effort by nature (a `SIGKILL`/power loss skips them); the size +threshold in D3 is the guarantee, the quit hook is the polish. + +### D7 — Verify the new snapshot before trusting it; retain one prior snapshot + +Adopted from the remoras failure-modes/on-disk-layout specs (17/02): compaction +must never replace the only good copy with an unverified one. + +`compact()` becomes: export snapshot to `snapshot.loro.tmp` + fsync → **verify** +(import the tmp bytes into a scratch `LoroDoc`; decode failure aborts the +compaction, leaving the old snapshot and full log untouched — reported on +stderr per D2, retried at the next trigger) → **copy** `snapshot.loro` to +`snapshot.loro.prev` (copy, not rename — the current snapshot never leaves its +place, so there is no crash window with no snapshot present) + fsync → atomic +rename tmp over `snapshot.loro` → delete `updates.log`. A crash at any point +leaves at worst redundant files, never a missing-or-unverified-only state. + +Retention is exactly one `.prev` (disk bound: ≤2 snapshots). `open()` does +**not** silently fall back to it — a decode failure of `snapshot.loro` returns +an error that names `snapshot.loro.prev` as the manual recovery path. Silent +fallback would quietly load stale state (the `.prev` predates the edits folded +into the failed snapshot; the post-compaction log does not cover that window), +which violates bounded-loss honesty. The `.prev` is a last resort with a +stated, bounded gap — not a transparent mirror. + +Costs: one extra decode per compaction (~tens of ms typical, ~50ms at 100k +blocks) and one file copy — both fine for an operation that runs every few MiB +of edits or at quit. + +### D8 — Batch the update-log import on open + +`open()` currently calls `doc.import(blob)` once per length-prefixed blob +(`storage.rs:71`). Measured on Loro 1.13.6 (remoras spec 22 Appendix B, +Expt 8): import cost is dominated by a fixed per-call overhead that is flat +regardless of delta size — 500 ops in one import cost the same as 1 op. Since +every acknowledged edit appends one blob, an uncompacted log pays that fixed +cost per edit on every cold start; this, not blob *bytes*, is the likely +mechanism behind "much slower uncompacted" loads. + +Change: collect the blobs and import them in one call (`import_batch`, or the +equivalent batch entry point in the pinned loro 1.13 API — verify at +implementation time). Purely an implementation change: identical resulting +doc state (imports are commutative and idempotent), no format or spec impact. +Complementary to the compaction policy — batching fixes per-blob overhead, +compaction bounds blob count; either alone leaves cold start exposed. + +## Risks / Trade-offs + +- [Inline snapshot export can hitch the UI at the threshold crossing, worst + case ~hundreds of ms at 100k blocks] → Accepted for MVP: it happens once per + ~4 MiB of edits; D1 records the background-executor alternative to revisit + if felt. The devtools `dump` command makes the hitch observable if it ever + matters. +- [Full-history snapshots mean `snapshot.loro` itself still grows with edit + history] → Out of scope here (Non-Goal); auto-compaction bounds *replay*, + not history. A future shallow-snapshot change addresses total size if it + becomes real. +- [Swallowed compaction errors (D2) could hide a persistently failing disk] → + The next `persist_update` append would also fail and *that* error is + propagated loudly, so a genuinely broken disk cannot stay silent. +- [The `.prev` fallback can tempt silent recovery that masks data loss] → + Deliberately manual (D7): open fails honestly and names the fallback; the + gap between `.prev` and the failed snapshot is stated, not papered over. +- [Compaction inside `persist_update` changes timing seen by UI tests / + fixtures] → Thresholds are far above anything fixture graphs or UI tests + write in one session, so no test crosses them implicitly; tests that want + compaction call it explicitly. diff --git a/openspec/changes/add-auto-compaction/proposal.md b/openspec/changes/add-auto-compaction/proposal.md new file mode 100644 index 0000000..493dae6 --- /dev/null +++ b/openspec/changes/add-auto-compaction/proposal.md @@ -0,0 +1,56 @@ +# Proposal: add-auto-compaction + +## Why + +`updates.log` grows without bound: `GraphStorage::compact()` exists but nothing +calls it outside tests, so every acknowledged edit appends to the log forever and +cold start pays a replay cost that only ever increases (README "Performance +notes": an uncompacted log makes doc load noticeably slower at scale). The fix is +pure policy — the mechanism is already implemented and crash-safe. + +## What Changes + +- `GraphStorage` gains an automatic compaction policy: after a successful + `persist_update`, if `updates.log` has reached a size threshold, the log is + folded into a fresh snapshot (the existing `compact()` path). +- The app compacts on quit when the log is non-trivial, so the next launch loads + a snapshot with little or no update replay. +- Compaction preserves the existing crash-safety contract: interruption at any + point (including between snapshot replacement and log removal) loses no + acknowledged edit and never produces an unloadable graph. +- No UI, no configuration surface: thresholds are named constants in + `trawler-core` (overridable in tests), consistent with how other tuning knobs + are handled. +- Companion optimization, same goal, no behavior change: `open()` imports the + update log as one batched import instead of one `import` call per blob. + Measured evidence (remoras spec 22 Appendix B Expt 8, Loro 1.13.6): import + cost is a large fixed overhead per *call*, flat regardless of delta size — + so an uncompacted log currently pays that cost once per acknowledged edit on + every cold start. Batching fixes per-blob overhead; compaction bounds blob + count. + +## Capabilities + +### New Capabilities + +_None._ + +### Modified Capabilities + +- `block-graph`: adds a requirement that the update log is bounded by automatic + compaction (size-threshold trigger during a session, quit-time trigger on + shutdown), and that compaction is crash-safe and invisible to correctness — + the Loro document remains the sole source of truth before, during, and after. + +## Impact + +- `crates/trawler-core/src/storage.rs`: threshold check + trigger in + `persist_update`; threshold field/constants; snapshot verification + `.prev` + retention in `compact()` (design D7); batched update import in `open()` + (design D8). +- `crates/trawler/src/main.rs`: register a quit-time compaction hook + (`cx.on_app_quit`) when the app owns a storage handle. +- `crates/trawler-core/tests/crash_safety.rs` (or sibling): interleaving test + for a kill between snapshot replacement and log truncation. +- No file-format change: `snapshot.loro` + `updates.log` semantics are + unchanged; a graph written by an old build opens identically. diff --git a/openspec/changes/add-auto-compaction/specs/block-graph/spec.md b/openspec/changes/add-auto-compaction/specs/block-graph/spec.md new file mode 100644 index 0000000..e39f375 --- /dev/null +++ b/openspec/changes/add-auto-compaction/specs/block-graph/spec.md @@ -0,0 +1,69 @@ +# block-graph Delta: add-auto-compaction + +## ADDED Requirements + +### Requirement: Update log is bounded by automatic compaction +The system SHALL automatically fold the incremental update log into a fresh +snapshot so that the log cannot grow without bound. Compaction SHALL trigger +(a) when, after an edit has been durably persisted, the update log has reached +a size threshold, and (b) at application quit when the update log exceeds a +smaller quit-time threshold. Compaction MUST NOT be required for correctness: +it only trades log-replay time at open for a snapshot write. + +#### Scenario: Size threshold crossed during a session +- **WHEN** an acknowledged edit brings `updates.log` to or past the automatic + compaction threshold +- **THEN** the log is folded into `snapshot.loro` and emptied (or removed), and + a subsequent open of the graph directory yields the identical graph state + while replaying no pre-compaction updates + +#### Scenario: Quit-time compaction +- **WHEN** the application quits normally with a non-trivial `updates.log` + (at or past the quit-time threshold) +- **THEN** on the next launch the graph loads from the snapshot with the + pre-quit edits already folded in + +#### Scenario: Trivial sessions do not rewrite the snapshot +- **WHEN** the application quits after a session whose `updates.log` is below + the quit-time threshold +- **THEN** the snapshot file is not rewritten and the small log is simply + replayed on next open + +### Requirement: Compaction preserves the acknowledged-edit guarantee +Automatic compaction SHALL run only after the triggering edit is durable in the +update log, and a compaction failure or interruption at any point MUST NOT lose +an acknowledged edit, MUST NOT leave the graph directory unloadable, and MUST +NOT surface as a persistence failure for the edit itself. Before the update log +is removed, the newly written snapshot MUST be verified loadable, and the +previous snapshot MUST be retained (as `snapshot.loro.prev`) as a manual +recovery fallback. Opening a graph whose current snapshot fails to decode MUST +fail with an error identifying the retained fallback rather than silently +loading stale state. + +#### Scenario: Kill between snapshot replacement and log removal +- **WHEN** the process is killed after compaction has atomically replaced + `snapshot.loro` but before `updates.log` is removed +- **THEN** the next open loads the graph to the identical state (re-importing + updates already contained in the snapshot is harmless), and a later + compaction removes the stale log + +#### Scenario: Compaction failure is not an edit failure +- **WHEN** the snapshot write fails (e.g. disk error) after the triggering + edit was appended and fsync'd +- **THEN** the edit is still acknowledged as persisted, the previous snapshot + and the update log remain intact and loadable, and compaction is retried at + the next trigger + +#### Scenario: Unverifiable snapshot never replaces a good one +- **WHEN** the newly exported snapshot fails verification (its bytes do not + decode into a loadable document) +- **THEN** compaction aborts before touching the current snapshot or the + update log, both remain intact and loadable, and the failure is reported + without affecting the acknowledged edit + +#### Scenario: Prior snapshot survives as a recovery fallback +- **WHEN** a compaction completes successfully +- **THEN** the pre-compaction snapshot remains on disk as + `snapshot.loro.prev`, and a subsequent open that finds the current snapshot + undecodable fails with an error naming that fallback instead of silently + loading it diff --git a/openspec/changes/add-auto-compaction/tasks.md b/openspec/changes/add-auto-compaction/tasks.md new file mode 100644 index 0000000..acb0682 --- /dev/null +++ b/openspec/changes/add-auto-compaction/tasks.md @@ -0,0 +1,32 @@ +## 1. Core policy (trawler-core) + +- [ ] 1.1 Add `AUTO_COMPACT_THRESHOLD_BYTES` (4 MiB) and `QUIT_COMPACT_THRESHOLD_BYTES` (64 KiB) as documented named constants in `storage.rs` +- [ ] 1.2 Implement `GraphStorage::compact_if_log_exceeds(min_bytes: u64) -> io::Result` — no-op returning `Ok(false)` when `updates.log` is absent or below `min_bytes`, otherwise delegate to `compact()` and return `Ok(true)` +- [ ] 1.3 Call `compact_if_log_exceeds(AUTO_COMPACT_THRESHOLD_BYTES)` from `persist_update` after the successful append + fsync; report a compaction error on stderr and still return `Ok` (design D2) +- [ ] 1.4 Unit tests in `storage.rs`: below-threshold no-op (snapshot untouched, log intact); at-threshold compaction (log gone, reopen yields identical state, no updates replayed); compaction triggered through `persist_update` with a tiny explicit threshold + +## 2. Snapshot verification and retention (trawler-core, design D7) + +- [ ] 2.1 Rework `compact()`/`write_snapshot` to the D7 sequence: export to `.tmp` + fsync → verify (import tmp bytes into a scratch `LoroDoc`; on failure abort, leaving old snapshot + full log untouched) → copy `snapshot.loro` to `snapshot.loro.prev` + fsync → atomic rename tmp over `snapshot.loro` → delete `updates.log` +- [ ] 2.2 Make `open()` on an undecodable `snapshot.loro` return an error that names `snapshot.loro.prev` when it exists (no silent fallback) +- [ ] 2.3 Tests: verification-failure abort (corrupt tmp bytes → compact errors, old snapshot + log intact and loadable); `.prev` exists and equals the pre-compaction snapshot after a successful compact; open error mentions the fallback when the current snapshot is corrupted + +## 3. Batched import on open (trawler-core, design D8) + +- [ ] 3.1 Replace the per-blob `doc.import(blob)` loop in `GraphStorage::open` with one batched import of all `updates.log` blobs (verify the exact loro 1.13 batch entry point, e.g. `import_batch`) +- [ ] 3.2 Test: create a graph, persist many small updates (one per edit), reopen, assert state identical to incremental opens; add an `#[ignore]`d timing comparison (per-blob vs batched at a few thousand blobs) alongside the existing expensive tests + +## 4. Crash-safety interleaving (trawler-core) + +- [ ] 4.1 Add a `crash_safety.rs` test for the kill-between-rename-and-delete window: persist edits, complete the snapshot replacement without removing `updates.log` (simulate via the D7 steps or by restoring the log file after compaction), reopen, assert identical state +- [ ] 4.2 Extend the test to run a subsequent compaction over the stale log and assert it removes the log and state is unchanged + +## 5. Quit hook (trawler app) + +- [ ] 5.1 Register `cx.on_app_quit` during app setup with a weak `TrawlerApp` entity handle; on quit, upgrade and call `compact_if_log_exceeds(QUIT_COMPACT_THRESHOLD_BYTES)`, ignoring failure so quit is never blocked +- [ ] 5.2 Verify manually via dev-loop: run with a scratch graph, make edits, quit, confirm `updates.log` is gone/small, `snapshot.loro.prev` exists, and relaunch shows identical content + +## 6. Verification and docs + +- [ ] 6.1 `cargo clippy --workspace --all-targets -- -D warnings` and `cargo test --workspace` pass +- [ ] 6.2 Update README "Graph directory format": describe the two automatic triggers (replacing the compaction-only-in-tests note) and document `snapshot.loro.prev` as a manual recovery fallback with its bounded-gap caveat -- 2.51.2