oficina / docs Public

<task-notification> <task-id>bhfxsiy7b</task-id> <tool-use-id>toolu_01GRokjACsQjPeQkMrSihtuA</tool-use-id> <output-file>/home/paulca/.cache/tmp/claude-1001/-home-paulca-amadan-net-oficina-docs/4a30c557-129a-4e57-8aa3-0c7518c36cf1/tasks/bhfxsiy7b.output</output-file> <status>completed</status> <summary>Background command "Install fixtures node deps" completed (exit code 0)</summary> </task-notification>

<task-notification> <task-id>bnk410t1x</task-id> <tool-use-id>toolu_01U22q22unGnbSKEw2qe9uoH</tool-use-id> <output-file>/home/paulca/.cache/tmp/claude-1001/-home-paulca-amadan-net-oficina-docs/4a30c557-129a-4e57-8aa3-0c7518c36cf1/tasks/bnk410t1x.output</output-file> <status>completed</status> <summary>Background command "Baseline Go test run" completed (exit code 0)</summary> </task-notification>

<task-notification> <task-id>bmqta3kz5</task-id> <tool-use-id>toolu_01V88aj6FmeRqMBRj4b2MXDs</tool-use-id> <output-file>/home/paulca/.cache/tmp/claude-1001/-home-paulca-amadan-net-oficina-docs/4a30c557-129a-4e57-8aa3-0c7518c36cf1/tasks/bmqta3kz5.output</output-file> <status>completed</status> <summary>Background command "Baseline Go tests with writable cache" completed (exit code 0)</summary> </task-notification>

<task-notification> <task-id>a7c87387c55b3bbdd</task-id> <tool-use-id>toolu_014zexCD1C74RZuFbutGCycM</tool-use-id> <output-file>/home/paulca/.cache/tmp/claude-1001/-home-paulca-amadan-net-oficina-docs/4a30c557-129a-4e57-8aa3-0c7518c36cf1/tasks/a7c87387c55b3bbdd.output</output-file> <status>completed</status> <summary>Agent "Fable review of live-sync design" 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>The review is complete — everything read, nothing in the repo touched. Here is the review. --- # Review: live-sync design (SSE advisory + POST), pre-spec **Verdict: approve with changes.** The transport ruling, the broadcast seam, the client module shape, and all three claims about existing code are correct. Two genuine defects need fixing before this becomes a spec — one silently loses an update (the exact failure class the design says it most fears), one makes the demo scenario fail for a visibly parked tab. The rest is material the spec should state, not things that are wrong. ## Genuinely wrong ### 1. SEVERE — the empty-slice `Docs-Seq` rule is a TOCTOU that skips a row permanently Design §Server-1 (live-sync-design.md:77–78): `Docs-Seq` is "the highest seq included, **or the document's current max when the slice is empty**." That second clause requires a separate `MAX(seq)` query after the slice query, and reads go to the read pool — nothing serializes them against the single-connection write pool's commits. Interleaving: tab A cursor 45, fetches `?since=45`. Handler's slice query runs → empty. Tab B's POST commits row 46 (store.go's transaction) and broadcasts. Handler's `MAX` query runs → 46. Response: empty body, `Docs-Seq: 46`. A advances its cursor to 46 **without ever applying row 46**. The broadcast that would have saved it arrived while A's fetch was in flight; the design's own fetch-serialization ("a full slot means a notification is already pending") coalesces it into a follow-up `?since=46` → empty. Row 46 is invisible in tab A until a full reload. This is exactly the "being wrong loses content silently" case §Server-2 promises its own test for — but the hole is in the GET header rule, not the cursor+1 rule. **Smallest fix:** `Docs-Seq = max(since, highest seq actually returned)` — never a second query. Empty slice echoes `since`; the no-`since` initial GET degenerates to 0 on an empty document, which still seeds the cursor correctly. Client side: cursor updates are always `cursor = max(cursor, header)`, never assignment, so a stale response can never regress it. Both belong in the promised both-sides test. ### 2. MODERATE — a visible, focused, idle tab goes stale with no path back Design §Server-5/Client-1 (live-sync-design.md:130–135): stream closes "after an idle period with no local edits and nothing received — reopening on **visibility, focus, or the next local edit**." A tab that is already visible and already focused while its person reads a long document fires none of those: the idle timer closes the stream, and nothing ever reopens it. Someone edits on their laptop; the desktop tab they are looking at never updates — the literal demo ("type in one, watch the other") fails whenever the watcher has been reading quietly past the idle threshold. Spec §7.3 says "**any activity** reconnects," which is broader than the design's three triggers; the design narrowed it and broke it. **Smallest fix:** count generic interaction — passive `pointermove`/`scroll`/`keydown` listeners — as the activity that both defers idle-close and reopens the stream. No timers, no polling (a slow poll would wake a hibernated instance and be worse than the stream), so the hibernation story is undamaged. ## Correct, but must be stated in the spec (silent otherwise) ### 3. `EventSource` native auto-reconnect must be disabled everywhere, not just for hidden tabs The design notes auto-reconnect "would defeat the TTL" only in the hidden/idle discussion. It defeats *every* policy decision: the browser retries on any server close, including the idle-close the module just chose. This is the one piece of jobs/events.go whose reason does not survive the move — jobs *relies* on native auto-reconnect ("EventSource does so on its own", events.go:16–18) because it has no close policy; this design must never rely on it. State: every close is `es.close()` by the module, every open is a deliberate decision, TTL closes reconnect only while visible-and-active (with jitter is nice, unnecessary at this scale). Miss this and a parked tab's browser reopens the stream forever — §7.3's exact failure. The rest of the recipe survives with its reasons intact: the write deadline (serve.go:275–286's zero-`WriteTimeout` contract, confirmed — `cmd/docs/main.go` goes through `rastrillo.Resolve` with defaults), the heartbeat (load-bearing here: it is what makes the write deadline ever fire on a quiet stream, bounding dead-client discovery to ~45s), the TTL as hard bound, and the hub replacing the 1s tick (the tick existed only because jobs had no broadcast primitive — its own comment says so). I checked the rest of rastrillo v0.22.0: no WebSocket, pubsub, or streaming helper anywhere else (`jobs/events.go` is the only `text/event-stream` producer; `internal/devloop` is unexported dev tooling). The design missed nothing there. ### 4. The committed-row-then-500 path drops a notification and the header `Store.ApplyUpdate` commits the `doc_updates` row, then bumps `updated_at` *outside* the transaction (store.go, the trailing `owned(...).Update`). If the bump fails, ApplyUpdate returns an error over a durable row: the handler 500s, no broadcast, no `Docs-Seq`; the client retries and writes a **duplicate row** (Yjs-idempotent, converges, but costs a `doc_updates` row and a `maxRetainedUpdates` slot), and only the retry broadcasts. Not a data-loss bug — the cursor+1 rule handles it (cursor stays put, next fetch catches up) — but the spec must define the return contract: seq is meaningful only when `err == nil`, broadcast only on nil error, and the duplicate-row-on-retry outcome is accepted. Same applies to a connection dropped after commit. ### 5. Small protocol clarifications the tests should pin - Stream events (`data: &lt;seq&gt;`) are **advisory only** — a fetch trigger, never a cursor assignment. The design implies it; say it. - A stopped `currentLive` instance must drop a late-resolving `?since=` response rather than deliver it (harmless today only because the callback closes over the discarded ydoc — make it a rule, not an accident). - A failed cursor+1 advance while the stream happens to be closed (debounce flush firing after the tab hid) should also mark "behind", healed by the connect-time max event on reopen — which is why that connect-time event is load-bearing and deserves its own test. ## Answers to the six questions **1. Transport — A is right, and "fallback first" is defensible.** B's cost claims all verified: no WS helper in rastrillo v0.22.0; `internal/core` ABI v2 exports no state-vector/diff entry points (an ABI bump D2 deliberately avoided); §7.3's hibernation problem is real. One correction for the amendment: §7.1's fallback describes "a monotonic sequence number **per connection**" with a high-water ack — the design's `Docs-Seq` is the document's global log seq, a different and better protocol. When Paul rules on the open question (correctly left as his call per AGENTS.md), §7.1 should be amended to describe the doc-seq protocol, not annotated as "the fallback shipped" — because what is shipping is not §7.1's fallback as written. **2. Cursor+1 — the rule is correct.** I constructed: two tabs interleaving (loser stays at its cursor, fetch applies both rows, its own row is a Yjs no-op); beacon racing flush *including* the beacon-fires-but-page-survives browser quirk (`pendingBytes()` doesn't clear the queue, so a duplicate row lands; no cursor movement since a beacon reads no response; idempotent); 5xx retry over a committed row (finding 4); two concurrent flushes from one tab with inverted response order (save-queue.mjs has that window — flush1 awaiting `post` while a new timer fires flush2; safe: Yjs parks missing-dep updates pending, and both rows are own bytes); resync mid-flight (old instance's late fetch hits the discarded ydoc). None loses, duplicates-with-effect, or wedges — a non-advanced cursor is always healed by the connect-time event or next fetch. The one silent-loss hole in the protocol is server-side finding 1, not this rule. **3. Broadcast seam — handler is right.** Store stays a persistence type, `doc_updates` stays §4.3's plain append-only table, and the only update writer is `updatesPost` (beacon and keepalive fetch both route through it, so both notify). The seed path (`seedIfEnabled` → `Seed` → `ApplyUpdate`) writes before any editor for that person can exist, so the accepted no-notify consequence has no reachable case today. Fold finding 4's ordering (broadcast on nil error only) into the seam's contract. **4. Hibernation — holds on the server end, holds on the client end only after findings 2 and 3.** Server: no timer exists outside a live connection, TTL is the hard bound, heartbeat+deadline reap wedged clients. A visible active tab reconnecting every 5 minutes pins the instance — by design, per §7.3 (someone is there). The permanently-stale-tab case is finding 2; the pinned-awake case is finding 3's native auto-reconnect. **5. Copied recipe — covered under finding 3.** One thing the dropped tick did that the hub does not: re-check the entity's existence each second (jobs' `gone` event). No delete route exists yet and TTL bounds exposure to 5 minutes; leave a note for the day deletion lands. **6. Yjs semantics — nothing wrong, one omission worth a line.** `applyServerLog`'s shared-decoder `Y.readUpdate` loop is exactly right for the concatenated slice (same framing as the full log). Remote updates onto a doc with local edits: ySyncPlugin mirrors local PM transactions into the Y.Doc synchronously (editor-entry.js's own comment), so there is no uncommitted CRDT state — merging is pure Yjs. Out-of-order and duplicate updates are genuinely safe (idempotent integration; missing-dep updates park in the doc's pending set; the server log is causally adequate since the core applied each row against full replayed history). The caret claim is true — ySyncPlugin maps selection through relative positions across remote transactions. The undo trap: the design never mentions `yUndoPlugin`, and it should — it does the right thing *by default* (its `Y.UndoManager` tracks only the ySyncPlugin's own origin, so `'server'`-origin updates are never captured and Mod-z after a remote edit undoes your own edit, not theirs), and one line in the spec prevents an implementer from "helpfully" adding `'server'` to `trackedOrigins`, which would break it. Two cosmetic notes for the spec: a remote transaction landing mid-IME-composition can cancel composition (known y-prosemirror behavior, not data loss), and undo history resets on resync (whole doc rebuilt — existing behavior). ## The three code claims — all three verified true - **`ApplyUpdate` returning seq:** true. Seq is `maxSeq+1` inside the `Transaction` closure (store.go); hoisting it to a captured variable and returning after commit disturbs neither the single-writer MAX+INSERT guarantee nor the owner re-derivation, which runs before. Contract caveat in finding 4. - **Save queue unchanged:** true. `post: (bytes) =&gt; fetch(...)` in editor-entry.js's `makeSaveQueue` can become an async closure that awaits the fetch, reads `Docs-Seq` **on 2xx only**, and returns the response; save-queue.mjs consumes only `r.ok`/`r.status`. The beforeunload beacon/keepalive path bypasses `post` and reads no header — harmless, analyzed under question 2. - **Echo-loop prevention:** true. `ydoc.on('update')` returns on `origin === 'server'`; `Y.readUpdate(decoder, ydoc, 'server')` tags every server byte; and ySyncPlugin does not re-emit remote-origin changes as new Yjs updates, so there is no ProseMirror-level echo either. Minor: the debounce recommendation (keep 2000ms, define "live" honestly) is right and matches core.go's own `maxRetainedUpdates` arithmetic; the testing section is sound — add finding 1's header rule and cursor monotonicity to it. Key files: `/home/paulca/.claude/jobs/4a30c557/tmp/live-sync-design.md` (under review), `/home/paulca/amadan.net/oficina/docs/internal/docs/store.go` (ApplyUpdate transaction), `/home/paulca/amadan.net/oficina/docs/internal/docs/handlers.go` (updatesGet — where finding 1's fix lands), `/home/paulca/amadan.net/oficina/docs/fixtures/editor-entry.js` and `save-queue.mjs` (claims B/C), `/home/paulca/go/pkg/mod/github.com/carlosframework/rastrillo@v0.22.0/jobs/events.go` (the recipe).</result> <usage><subagent_tokens>120203</subagent_tokens><tool_uses>26</tool_uses><duration_ms>376085</duration_ms></usage> </task-notification>