oficina / docs Public

supress the rule

So I guess this is laggy by about 2 seconds as predicted. I guess it would be better to get it near-realtime? What are our options there? Websockets? And that gives us live presence too? Does our implementation prove it works and now we upgrade to a websocket implementation?

<task-notification> <task-id>aec44d5136d76ab35</task-id> <tool-use-id>toolu_01Cm9gJ7ccCTaFZJgHUQJrSR</tool-use-id> <output-file>/home/paulca/.cache/tmp/claude-1001/-home-paulca-amadan-net-oficina-docs/4a30c557-129a-4e57-8aa3-0c7518c36cf1/tasks/aec44d5136d76ab35.output</output-file> <status>completed</status> <summary>Agent "Fable review of snapshots spec" finished</summary> <note>A task-notification fires each time this agent stops with no live background children of its own. The user can send it another message and resume it, so the same task-id may notify more than once.</note> <result>I have everything I need. I verified the yrs 0.23.5 API surface directly against the crate source on this machine, traced the concurrency and lifecycle paths in the Go host, and checked every claim the spec makes about existing code. --- # Spec review: `docs/superpowers/specs/2026-09-01-snapshots-design.md` **Verdict: approve with changes.** The shape is right — snapshot-as-ordinary-update, never authoritative, no timers, cursor rules untouched — and most of the spec's claims about existing code check out. But there are two genuine correctness defects, both in *where* D3 takes the snapshot from, and both produce exactly the failure the spec says it fears most: a wrong snapshot that verifies clean and silently becomes every reader's document. One of them also defeats the spec's headline motivation for documents already at the cap. Two further defects are omissions an implementer would resolve wrongly by picking the nearest-looking API. Fix those and this is a sound, shippable slice. ## Genuine defects, most severe first ### 1. D3's snapshot can be generated from a state that is missing a committed row — and D5 passes it **Where:** D3, D5; `internal/docs/store.go` `ApplyUpdate`. `ApplyUpdate`'s replay runs *outside* the write transaction; only MAX(seq)+INSERT is serialized by the single-connection write pool. Two concurrent POSTs to one document — the flush-timer + `beforeunload` beacon pair the live-sync spec itself names as real — both replay rows 1..10; A commits seq 11, B commits seq 12. B's in-memory Handle now holds rows 1..10 plus its own update, **not row 11**. If seq 12 is the cadence boundary, D3 writes a snapshot claiming seq 12 that does not contain row 11's content. D5 cannot catch this: it compares the candidate against *the same stale handle* that produced it — they match perfectly. Consequences: every D6 server render (show page, exports) and every D7 snapshot-then-tail client silently loses row 11's content, because rows ≤ 12 are never served raw once the snapshot exists. `Docs-Seq: 12` now genuinely lies about the bytes in hand — this is the one hole through which the D2-forbidden failure (cursor advanced over content never received) re-enters, wearing the snapshot's seq column as a disguise. It self-heals only at the *next* snapshot 256 updates later, and never if the document goes idle. D2's "log remains authoritative" rescues nothing, because nothing detects the mismatch — the fallback triggers only on unreadable rows. **Smallest fix:** take the snapshot from the *replayed* state, at seq = the highest seq the replay actually read, **before** applying the new update. That state is definitionally the complete prefix (seq is gapless by MAX+1 construction, and the replay read every row that existed at read time), so the seq column becomes unconditionally truthful. Alternative that preserves D3's after-success placement: record the replay's max seq and write the snapshot only when `assignedSeq == replayedMax + 1`, skipping otherwise (the next POST retries). Prefer the first — see finding 2. ### 2. A document already at the cap stays read-only forever — the spec's headline case fails **Where:** §1, D3, D4; `internal/core/core.go` `Handle.Apply`, `Store.replay`. The spec's opening argument is "this is the thing that stops a document going read-only," and there may be such documents in production. Trace what actually happens to one with 4096 rows: no snapshot exists, so D6 falls back to full replay, which fills `h.applied` to exactly 4096. The POST's `h.Apply` then returns `ErrUpdateLogFull` **before the update succeeds** — and D3 writes snapshots only *after* a successful update. So the capped document never gets a snapshot, and every POST 400s forever. The cliff is removed for documents approaching it and left permanently in place for documents already over it. **Smallest fix:** the same placement change as finding 1. Snapshotting the replayed state before `Apply` is a read against a fully usable handle: the first POST after ship writes a snapshot at seq 4096, that POST still fails once with 400, the client's retry replays snapshot + empty tail (`h.applied` = 1) and succeeds. One 400, then healed. The spec's own test list would not catch this: its cap regression test applies &gt;4096 updates *through the new path* (never hits the cap, since replay is always snapshot+tail); the production-shaped case is a pre-existing 4096-row log with no snapshot, and that fixture needs to be in §8 explicitly. ### 3. The spec never names the encode call, and two wrong near-neighbours exist in yrs **Where:** §3, D1; verified against `/home/paulca/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/yrs-0.23.5/src/transaction.rs`. The correct call is `ReadTxn::encode_state_as_update_v1(&amp;StateVector::default())` — I confirmed in the 0.23.5 source (transaction.rs:138) that it encodes all blocks, the **entire delete set**, and — via `merge_pending_v1` (transaction.rs:374) — **merges the pending queue** (updates whose dependencies never arrived, which `apply_update` accepts with rc 0 and parks). With that call, §3's central property genuinely holds under this app's constraints: the fixed guid/client id play no role in the encoded state, GC is symmetric (both the browser's `Y.Doc` and the server's `Options::with_guid_and_client_id` doc run gc-on defaults, so tombstone handling matches log replay), and the "prosemirror" root travels inside the update. No clippy.toml rule is threatened — the snapshot is data, not construction. But the spec leaves the choice to the implementer, and the two adjacent APIs are traps: - `encode_diff_v1(&amp;StateVector::default())` looks identical and **silently drops pending updates** (its own doc comment says so). A log whose orphaned update's dependency arrives later heals under full replay but not under a diff-built snapshot — the exact "renders identically today, breaks a later merge" failure this review was told to hunt, and D5's render comparison cannot see it (pending content is invisible in both docs). - yrs's own `Snapshot` type / `encode_state_from_snapshot` is a name collision with this spec's vocabulary and a different feature entirely (historical views, requires `skip_gc`). **Fix:** D1 names the exact function and the empty state vector, and says why the siblings are wrong. ### 4. "Discarded if from an older ABI" is unimplementable against D2's schema **Where:** D2, §6 failure table; `internal/docs/migrations/`. The failure table promises "snapshot row … from an older ABI → discarded", but the specced columns (`doc_id, seq, state, created_at`) record nothing about what produced the row. The implementer must invent: an `abi_version` (or format) column; the migration file (0004); whether `(doc_id, seq)` is UNIQUE — under finding 1's before-apply placement, two concurrent boundary POSTs can legitimately produce a snapshot at the *same* seq, so the spec must choose UNIQUE-with-conflict-ignore or newest-row-wins; and what "newest usable" means (highest seq? highest id?). Small additions, but every one is currently an invention point, and migration 0003's own comment shows this repo treats "what does the migration do to rows that already exist" as spec-level material. ### 5. The corrupt-snapshot fallback exists only on the server; the GET path has none **Where:** D2, D6, D7, §6; `internal/docs/handlers.go` `updatesGet`, `fixtures/editor-entry.js` `applyServerLog`. D2's safety story — "anything unreadable … is discarded and the full log replayed instead" — holds for `Store.replay`, which can observe an apply failure. `updatesGet` cannot: it serves BLOB bytes without a core call, so a snapshot row corrupted on disk *after* D5 verified it goes straight to the browser, where `Y.readUpdate` throws mid-loop and resync refetches the same corrupt bytes forever. Server rendering works; the editor is permanently unloadable. That contradicts both "the worst failure is a performance regression, never wrong content" (D2) and "this needs no client change" (D7). **Fix:** either state the residual honestly (disk corruption of a verified row is out of model and manifests as a broken editor), or spec the small client change: on a decode failure during `applyServerLog`, refetch with a no-snapshot escape hatch (`?raw=1` or similar) so the log's authority is actually reachable from the one reader that matters most. ## Answers to the ordered questions, and things worth stating (not defects) **1. Snapshot correctness (§3):** True, *conditional on finding 3's exact call*. Verified against yrs 0.23.5's own source: full blocks + entire delete set + merged pending, applied to an empty doc, is Yjs's own persistence mechanism and is merge-equivalent to the log — tombstones/GC ranges preserve every clock range, so later concurrent merges integrate identically. The encode must be against the **empty state vector**. Client ids and the "prosemirror" root pose no problem. One footnote: `merge_pending_v1` ends in `.unwrap()` — a panic there is a wasm trap, which the host's existing trap taxonomy already contains (failure table row 5 covers it correctly). **2. D5:** A materially stronger check exists at *lower* cost than the specced one: do the verification **inside the core** with one export — apply the candidate to a fresh internal `Doc` (no second wasm instance, no second Go-side handle) and compare `StateVector` equality and `DeleteSet` equality as direct Rust struct comparisons (no encoding-determinism worries), plus `fragment_json` equality rather than text (text misses formatting-only divergence; JSON is the same cost and strictly stronger). Honest limit, which the spec should state: this is still not a proof — update encoding is not canonical (merge boundaries vary), so no cheap byte-level proof exists; SV+DS equality bounds the residual to "yrs corrupted content while preserving every clock range, every deletion, and the full structural render", which is the right place to stop. Note D5 as written cannot catch finding 1 regardless — that needs the placement fix, not a stronger comparator. **3. D7 / cursor contract:** The distinction the spec draws is **real, not the D2 TOCTOU in a hat** — the D2 failure required the header to promise more than the body carried; a snapshot's seq travels atomically with bytes that contain its whole prefix, so the header can never outrun the bytes in hand… *exactly as long as the snapshot row is truthful*, which is what finding 1 breaks. With that fixed, every interleaving checks out: since behind the snapshot (snapshot covers the gap, integration idempotent), since at/ahead of it (raw rows, unchanged), mixed tabs (converge), the beacon (reads no response, moves no cursor), empty tail (Docs-Seq = snapshot.seq, bytes in hand). One implementation note worth a sentence: select the snapshot row first and the tail as `seq &gt; snapshot.seq`; the reverse order is also loss-free (overlap is idempotent) but the natural order should be stated. **4. D3 synchronous write:** Right call. The boundary POST pays snapshot export + verification + one small INSERT; each guest call is bounded by the 1s timeout, the INSERT is a fast transaction on the single write connection, and — with D6 — the per-POST replay gets *cheaper* overall, so the request path nets out ahead. Outside `ApplyUpdate`'s transaction is correct and the failure table already depends on it (update stands when the snapshot fails). Two things to state: (a) the guest-call timeout now indirectly bounds snapshot size — a document whose state export or snapshot-apply exceeds 1s traps every attempt, never gets a snapshot, and pays a rebuild penalty per cadence attempt (`WithGuestCallTimeout` is the existing knob if that ever bites); (b) once distance ≥ cadence, *every* subsequent POST retries a failing snapshot, not "the next Nth write" — the failure table's wording is slightly off. **5. Cadence 256 / is the cap retired:** The trace works: replay = 1 snapshot-update + ≤ ~257 tail + 1 new, so `h.applied` stays ~16× clear of 4096. But "stops being reachable" is **conditional**: the tail is bounded only while snapshot writes keep succeeding; ~15 consecutive cadence failures (each logged at Error, so not silent) re-reach the cliff, and then the failure table's "correct, possibly slower" bar is no longer met — it ends in `ErrUpdateLogFull` again. Say so. Relatedly, "maxLogBytes stops being a second cliff" is true for *history* but not *size*: a document whose state itself exceeds 8MB still 500s on GET with the snapshot in the body. **6. Claims about existing code:** All materially correct — `maxRetainedUpdates`'s comment does specify this export (it says "ABI v2"; current is v3, spec's v4 is right), the 2h16m arithmetic holds, `ErrUpdateLogFull` does reach the browser as a permanent 400 (`updateIsClientFault`), `applyServerLog` does loop `Y.readUpdate` over a concatenated body (editor-entry.js:99–103), the ABI check is exact-match, and no existing export can be reused — a new len/read pair is genuinely needed (the len-then-read double-encode is safe: same unmutated store between the two calls, and the written-count guard backstops it). Omissions beyond findings 2/4: the spec should say how the cadence's "distance since the last snapshot" is known per POST (D6's replay already loads the newest snapshot row — pass its seq through rather than adding a query), and §8 needs two tests it lacks: the concurrent-boundary-POST race (finding 1) and the pre-existing-capped-log fixture (finding 2). **Scope ruling:** Coherent and shippable. Nothing deferred is load-bearing: cadence-by-count works without coalescing, and the horizon is storage-only. Two honesty amendments to §2/§9's storage paragraph: snapshot rows grow storage **faster than the log** for large documents (a full state copy every 256 updates, all retained under "nothing is deleted", all replicated by Litestream) — and superseded snapshots are *derived* data, reproducible from the log, so deleting them is not deleting history and need not wait for the horizon ruling; the spec should rule on supersession either way. D2's "adds no new argument about sealing" is slightly generous for the same reason — one snapshot row is the entire document in a single plaintext blob — worth one sentence, not a redesign. --- Files load-bearing for this review: `/home/paulca/amadan.net/oficina/docs/.claude/worktrees/live-sync/docs/superpowers/specs/2026-09-01-snapshots-design.md` (the spec), `/home/paulca/amadan.net/oficina/docs/.claude/worktrees/live-sync/internal/docs/store.go` (`ApplyUpdate`'s replay-outside-transaction structure, findings 1–2), `/home/paulca/amadan.net/oficina/docs/.claude/worktrees/live-sync/internal/core/core.go` (`Handle.Apply`'s cap check ordering, finding 2), `/home/paulca/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/yrs-0.23.5/src/transaction.rs` (finding 3's API verification). One housekeeping note: the worktree's checked-out branch is `mobile-margins-squashed` at fecd95c, not the `worktree-snapshots` name the task stated — the spec and all reviewed files were present and reviewed as found.</result> <usage><subagent_tokens>119774</subagent_tokens><tool_uses>20</tool_uses><duration_ms>494918</duration_ms></usage> </task-notification>