15 Commits

Author	SHA1	Message	Date
xarmian	dc70ff3d7f	fix(server): write first, apply second on the collab applier path (TASK-2989 / BUG-2840 half A) (#1318) ... * test(server): measure BUG-2840 half A's premise before designing a fix Half A's plan makes step one an experiment, not a design: the claim that a refused PATCH still lands its content was a reading of the snapshot branch rather than an observation, and the shape of the fix depends on which half actually bites. Measured, on the applier path with a live room: a PATCH carrying content and a stale expected_updated_at answers 409, leaves items.content untouched, and adds an op-log row that outlives the request. The caller's refusal is true of the row and false of the collaborative document. The first version of this test was CIRCULAR and reported the premise confirmed. It drove a ?source=collab-snapshot PATCH carrying the refused string, which proves only that a snapshot write writes what it is given. The server cannot close that loop at all: collab here is a dumb relay that persists opaque Yjs updates and never parses them, so nothing server-side derives markdown from a room's document — in production that markdown comes from a live tab's Y.Doc. What IS observable server-side is durable collab state created by a request that was refused, which is what this now measures. Two details that make the harness faithful rather than convenient: - The fake applier emits a binary op as well as the ack. A real applier is a browser tab that applies the markdown and broadcasts the resulting update; acking alone would leave no durable trace, so the experiment would have been measuring a peer that does not exist. - Readiness is detected by the observable difference between the two paths — a succeeding probe PATCH that leaves items.content untouched proves the applier answered — because no exported accessor for electable connections exists and the manager's state is not reachable from this package. The test asserts today's behaviour, defect included, so the fix has a baseline to move. It skips with an explicit "premise NOT established" message if the harness ever stops reproducing the applier writing durable state, rather than passing vacuously. Refs: BUG-2840 * feat(server): write first, apply second on the collab applier path (TASK-2989 / BUG-2840 half A) PLAN-2975 decisions 2-4. A refused PATCH no longer changes the item. The applier path used to push content into the live Y.Doc before the row write, so any of the four typed refusals answered 4xx while the collaborative document had already moved and the next collab-snapshot flush carried the refused content into items.content. The reorder is possible because TASK-2987's HasElectableApplier answers which path the request is on without taking it. routeContentUpdate owns the re-decision deliberately: the predecessor retried ErrRoomActiveDuringPrune inside applyContentViaCollab and re-called ApplyExternalContent, which could succeed through a freshly joined applier and return nil, after which the row write still ran last and reproduced the defect. Re-deciding before anything is written makes that impossible rather than unlikely. Two typed 409s join the structured family. content_not_applied answers the hybrid the reorder creates - row write committed, content not in the document - naming the landed fields and the new updated_at so a content-only retry does not trip OCC. room_settling answers the standoff where PruneAndApply blocks on any writer while election also demands unfrozen and replay-done: the predecessor gave up after three tries and wrote past the live peer, losing the write on its next flush. applier_ambiguous is untouched; its outcome is unknown and a claim either way would be false. The measurement harness is inverted rather than deleted: it asserted the defect and would have become a SKIP, which reads as a pass. Claude-Session: https://claude.ai/code/session_01GqaEDuCtRiSJfa7eppWecn * refactor(server): retire the route-flipping helper chain the reorder replaced (TASK-2989) applyContentViaCollab, applyContentViaCollabOnce, directWriteFn, applyContentMaxRetries and isDeterministicWriteFailure are dead once the router owns the decision, and golangci-lint said so. Removing them is the point rather than tidying: that chain retried ErrRoomActiveDuringPrune internally and re-called ApplyExternalContent, which could succeed through a freshly joined applier and let the row write run last after all. Two things ported rather than dropped. isDeterministicWriteFailure's closed-set warning moves onto writeTypedItemRefusal, which inherits the job of recognising every typed permanent refusal. Its regression test is ported too, unchanged in property: a refusal the handler does not recognise is treated as recoverable and the request re-derives it by another route, which BUG-2804 measured as a rename cascade run twice. CONVE-23 sweep: my own comment on HasElectableApplier, merged four hours ago, said the fallback could write content past live peers. This unit made that false. It now states what the sentence was true of and what replaced it, rather than being quietly deleted. The structural guard needed teaching, not weakening: it counts the handler's refusal blocks and failed closed when one moved into a shared function. It now scans both files and says why three is still three. Claude-Session: https://claude.ai/code/session_01GqaEDuCtRiSJfa7eppWecn * test(server): pin the settle budget itself, which every other test bypassed (TASK-2989) Found by mutation: applierSettleBudget = 0 survived the whole suite. The decision tests pass their own budget, so the constant had no coverage at all — and a zero budget makes the retryable refusal the normal answer for any room with a writer still anchoring. The floor is the measurement the constant was sized from rather than a number: 47ms, just above the 46.41ms worst anchoring time measured for this deployment. Claude-Session: https://claude.ai/code/session_01GqaEDuCtRiSJfa7eppWecn * test(server): bound the standoff subtest so a broken deadline fails instead of hanging (TASK-2989) The only exit from the standoff branch is the deadline, so the mutant that makes it unreachable spins and the failure arrives as a package timeout with no --- FAIL line — which a mutation harness reads as 'the package broke' rather than as a detection. Measured: that is exactly what M5 produced. Same shape as the waiter rule: a failure mode indistinguishable from the waiting mode is not a signal. Claude-Session: https://claude.ai/code/session_01GqaEDuCtRiSJfa7eppWecn * fix(server): content_not_applied must not assert a timeout did not land (TASK-2989) Codex round 1, and the sharpest finding in it. ApplyExternalContent returns ErrAllAppliersTimedOut only AFTER an applier_request has gone out on the wire, so the elected peer may have applied the markdown and persisted its ops while the ack was lost or merely late. Answering content_landed:false there states as fact something the server cannot know — the same overclaim the ruling avoided by leaving applier_ambiguous alone, arriving one door over. The discriminator already existed upstream and needed no new machinery: electAndApply returns ErrNoApplierAvailable when anyWriteSucceeded is false (nothing reached a peer) and ErrAllAppliersTimedOut when something did. The envelope now carries content_outcome, and content_landed is ABSENT rather than false when the outcome is unknown, because a caller that reads false may act on a premise nothing supports. Three smaller round-1 items. The settle budget's comment now says it bounds how long the route keeps ASKING, not how long the request takes — the deadline is only consulted between attempts and PruneAndApply can block on the per-item lock. A comment on fullWriteHandled still named applyContentViaCollab, which this unit deleted; my own sweep missed it. The ported classifier test now inspects the recorder rather than only the boolean, since a mutant could return true while writing the wrong status. Verified and NOT changed: nil-ing content on the row write does not newly suppress version bracketing. main already set input.Content = nil on the applier path before its row write, so that behaviour is identical before and after the reorder. Claude-Session: https://claude.ai/code/session_01GqaEDuCtRiSJfa7eppWecn * fix(collab,server): the not-applied claim was still false on two post-wire paths (TASK-2989) Codex round 2, and it refuted the reasoning I gave in round 1's fix rather than just finding another case. I said the discriminator already existed upstream because electAndApply tracks anyWriteSucceeded. It does — PER ELECTION — and two paths escape it: - a restore storm returns ErrNoApplierAvailable after up to applierMaxRestartsAfterRestore elections, each of which may have put an applier_request on the wire, with the per-election flag discarded at every restart; - a registerPendingAck failure on a retry attempt returns a raw error after an earlier attempt had already sent one. Both would have answered content_landed:false about content that may have landed. Same shape as the finding they follow: a reason that was sufficient-sounding and one file short of true. Fixed at the source where the source can know it — ApplyExternalContent now carries sentAny across restarts, so ErrNoApplierAvailable means what its callers read it to mean — and by construction everywhere else: classifyApplyOutcome is a whitelist, so only the two sentinels that mean nothing reached a peer may make the claim and every other error, including ones nobody has written yet, degrades to unknown. Cancellation: the re-decision wait is the only new blocking wait this branch adds, and it now ends when the caller goes away. The rest of the path was context-blind on main and stays that way; threading a context into the store and the applier round-trip is a different change. Not fixed here, deliberately: the ambiguous-commit double-write. Codex confirmed against main that it has the identical shape there, so it is pre-existing and gets filed rather than folded into this unit. Claude-Session: https://claude.ai/code/session_01GqaEDuCtRiSJfa7eppWecn * docs(collab): sweep the prose my own round-2 fix falsified (TASK-2989) Codex round 3, one P3, and it is CONVE-23 arriving for the third time in this unit. Carrying sentAny across the restart loop changed which sentinel a restore storm returns, and left two comments describing the old behaviour: the cap's doc still said exhaustion falls back with ErrNoApplierAvailable, and the sentinel's own doc still said every attempt timed out. Both now say what the sentinel MEANS rather than how it usually arises — bytes reached a peer and the outcome is unknown — because that is the half two callers depend on: the op-log prune stays suppressed, and the PATCH handler reports the content outcome as unknown rather than not-applied. Claude-Session: https://claude.ai/code/session_01GqaEDuCtRiSJfa7eppWecn * fix(server): restore the UNIQUE-constraint 409 the applier path used to inherit (TASK-2989) Codex round 5, and a regression rather than a gap. The ordinary error block maps a UNIQUE-constraint race — two updates that both pass checkUniqueFields and then both hit the partial unique index on invocation_slug — to a 409. Before the reorder the applier path's row write ran through that block and inherited the mapping. Routing it through a helper built from 'the four typed refusals' dropped the arm and turned a benign race into a 500 on that route alone. The irony is the lesson, and it belongs on the record: writeTypedItemRefusal exists BECAUSE this handler's refusal set has been under-counted three times, and I under-counted it again while building the thing meant to stop that — by taking the population from the errors that have a Go type rather than from the block that actually answers them. The new arm's first version panicked on a nil error, since it dereferences where the typed arms use errors.As. The existing nil control leg caught it immediately, which is the entire reason that leg is there. The structural guard now DERIVES its file set — every non-test file in the package that calls UpdateItemWithParentLink — instead of listing two names, so a future block in a third file cannot sit unmapped while the test passes. Claude-Session: https://claude.ai/code/session_01GqaEDuCtRiSJfa7eppWecn * test(server): the guard now requires the fifth arm, scoped to the block's own function (TASK-2989) Codex round 6. Two gaps in the guard as it stood: it verified only the four typed arms, so removing the UNIQUE-constraint mapping from either ordinary block still passed; and its file set matched on the store call text, so a file reaching the store through a wrapper would not be scanned at all. The file set is now the UNION of files calling UpdateItemWithParentLink and files calling any of the arms — a refusal block lives where the arms are called, whatever it calls the store through. The fifth-arm check is scoped to the ENCLOSING FUNCTION, and that is the part worth reading. The first version asked whether a UNIQUE literal appeared between one block's start and the next block's start in token.Pos. Those windows span whole files, so the gap between the last block of one file and the first block of the next swallowed every literal in between — two in handlers_items.go belonging to the create and restore paths, one in handlers_items_bulk.go. All three mutation controls survived it. It asserted nothing, and it passed, which is the only reason I looked. Committed BEFORE the controls run this time. The previous round's controls used git checkout -- internal/ against uncommitted guard work and deleted it; the tree read clean afterwards, which is the ambiguity — clean means the mutation was reverted OR the mutation and my work both were. Claude-Session: https://claude.ai/code/session_01GqaEDuCtRiSJfa7eppWecn * test(server): scope the fifth-arm check to the block statement, not the function (TASK-2989) Per-function was the second wrong containment and the controls said so: handleUpdateItem holds TWO refusal blocks with a UNIQUE arm each, so neutralising either hid behind the other and survived. Only the writeTypedItemRefusal control was detected — the check covered one of the three blocks it claimed to cover. Innermost enclosing BlockStmt is the containment that matches what the sentence means by 'the block's own arm'. Claude-Session: https://claude.ai/code/session_01GqaEDuCtRiSJfa7eppWecn * test(server): the fifth-arm check reads if-conditions, not any literal in the block (TASK-2989) Codex round 7. Scanning the whole BlockStmt for a matching string literal let an unrelated nested closure — or a message string quoting the phrase — satisfy the guard after the real mapping had been deleted. That is the guard passing for a reason unrelated to what it asserts, which is the failure this whole check exists to prevent one level down. It now reads only IfStmt conditions, which is the shape the arm actually has. Claude-Session: https://claude.ai/code/session_01GqaEDuCtRiSJfa7eppWecn	2026-09-10 08:54:41 -04:00
xarmian	cece78a703	feat(collab): applier-availability pre-check on RoomManager (TASK-2987 / PLAN-2975 unit 1) (#1314) ... * feat(collab): applier-availability pre-check on RoomManager (TASK-2987) PLAN-2975 decision 1. HasElectableApplier answers "would an external content update for this item route through a designated applier right now?", so handleUpdateItem can learn it is on the applier path BEFORE it writes the row — today the only way to learn that is to apply, and by then the content is already in the live Y.Doc (BUG-2840 half A). It is a hint, not a lock: it registers nothing, takes no admission, and enters no gate, so a concurrent ApplyExternalContent neither blocks it nor sees it. Eligibility is delegated to pickApplier rather than restated, so the hint and the elector cannot drift. The error direction is asymmetric on purpose. A false negative is the defect (it sends the caller back to apply-then-write); a false positive costs a typed partial answer, which PLAN-2975 decision 2 owes anyway. So an in-progress version restore answers TRUE rather than consulting the conns: ForceRefreshRoom freezes every conn and pickApplier skips frozen conns, yet a restore ROLLBACK unfreezes them and elects an applier — so a bare pickApplier check answers false for exactly the window this is required to compose with. Eight legs, including a negative control: the pickApplier-only formulation must answer false during a restore, or the table would be evidence about neither implementation. Claude-Session: https://claude.ai/code/session_01GqaEDuCtRiSJfa7eppWecn * test(collab): pin the m.closed guard, which Close makes unreachable through the public API (TASK-2987) The manager-closed leg passed with the guard deleted: Close sets m.closed and empties m.rooms in one m.mu critical section, so the room lookup already answers nil. Measured as a surviving mutant, not assumed. The new leg constructs closed-with-rooms-populated — a state Close does not produce — because that is the coupling the guard breaks. Claude-Session: https://claude.ai/code/session_01GqaEDuCtRiSJfa7eppWecn * docs(collab): narrow two claims in HasElectableApplier's contract that the code does not support (TASK-2987) Codex round 2, adversarial on the unit's own claims. 'A false positive costs a partial answer, not corruption' was a claim about the whole path stated as if it were about this return value. The fallback that exists today can write content past live peers: when applyContentViaCollab exhausts its ErrRoomActiveDuringPrune retries the handler falls through to a plain direct write while a live writer may hold a Y.Doc that outvotes it. That predates this function; the comment now says so rather than implying it cannot happen. 'The hint and the elector cannot drift' reads as 'cannot disagree'. They cannot disagree about the RULE, since the predicate is shared. They can disagree about the ANSWER, and the routes are now enumerated: a fresh writer joins, an unanchored conn finishes replay, a view-only conn is promoted by the periodic revalidation, a restore rolls back. Claude-Session: https://claude.ai/code/session_01GqaEDuCtRiSJfa7eppWecn	2026-09-10 00:55:22 -04:00
xarmian	51cd6e84e4	fix(collab): op-id durable fence for restore-rollback vs applier-ack race (BUG-2276 residual 2) ... Closes the restore-rollback vs applier-ack clobber race with a durable operation-id correlation instead of a timing heuristic. The client brackets its setContent with an applier_apply_start{request_id} control frame; the server decides whether the external write persisted by reading the per-conn op-log high-water UNDER the same appendMu that sets the restore freeze (finalize-at-freeze — no drain, so a blocked write can't stall the restore; no timing window). Edges handled: unanchored conns are never elected; gate admission spans registration; legacy (pre-bracket) clients negotiate capability and an unconfirmable legacy round-trip returns a retryable 409 applier_ambiguous (fail-safe, never a clobber); the applier callback is synchronous-by-type so nothing can split the bracket. Normal acks stay on a lock-free, latency-identical fast path. Confirming Codex (high effort): redesigned from a timing grace after review; 3 rounds on the op-id design (2 P1 -> 3 P1+P2 -> CLEAN/converging). E2E + Go(PostgreSQL) green; go test -race clean 8x. Go/Web CI red only on the pre-existing dependency advisories (BUG-2278). https://claude.ai/code/session_01EZ6yr6pAUFb1uffan912ra	2026-07-21 20:51:19 -04:00
xarmian	e601f2b368	fix(collab): reconcile Postgres commit-ack-loss on version restore instead of treating it as rollback (BUG-2276 residual 1) ... On Postgres, a version-restore commit that durably lands but whose ack is lost surfaced as an error and wrongly resumed peers on a stale Y.Doc. ForceRefreshRoom now runs a Postgres-only reconcile after a commit error: two durable signals (content == restored version AND last_restore_seq advanced past a lock-captured baseline) must agree → LANDED (publish fences + reseed, return the restored item + SSE); both false → rolled back (unfreeze); disagree/read-error → UNCERTAIN (invalidate in-memory fences so durable state governs, then plain-close sockets so peers reconnect + re-evaluate). SQLite path unchanged. Confirming Codex (high effort): 3 rounds — false-404, frozen-forever, archive-nil, stale-baseline, stale-in-memory-fence-clobber all closed; real Postgres end-to-end ack-loss + SSE test. make test-pg green. Residual 2 (applier-ack rollback race) follows separately. Go CI red only on the pre-existing govulncheck advisory (BUG-2278). https://claude.ai/code/session_01EZ6yr6pAUFb1uffan912ra	2026-07-21 18:00:49 -04:00
xarmian	40f88052cd	fix(collab): version restore via prune+reseed (BUG-2264) (#990) ... Version restore didn't reconcile the live Y.Doc: peers kept editing a Y.Doc built on pre-restore ops, and their next collab-snapshot flush clobbered the restored items.content. Reworked restore to prune+reseed — the restored content becomes canonical and every peer converges on it (unflushed edits are discarded, which is exactly restore semantics), replacing the earlier applier/epoch/watermark routing. handleRestoreItemVersion drives RoomManager.ForceRefreshRoom under the per-item lock. Hardened across Codex xhigh review rounds: - Atomicity: pre-prune MAX(op-log), the items.content write, the "Restored from…" version, the op-log wipe, AND both durable restore boundaries all run in ONE store transaction. A failed commit rolls back all of it — no divergent state, no fail-open boundary. - Unambiguous commit signal: UpdateItem reads the updated row WITHIN the tx (getItemTx) before commit, so a read failure can't make a committed update look failed and the returned seq is this restore's. - Restore freeze: conns are paused via a dedicated rc.frozen flag (NOT canWrite) so the auth-revalidation loop can't thaw the freeze mid-restore or promote a viewer; pickApplier + the applier-ack handler reject frozen conns so a concurrent external PATCH can't falsely succeed. - Stale-flush boundary: pre-prune MAX+1 fences in-flight snapshot cursors under the same item lock. - force_refresh fan-out deadlock: per-conn timer-close so a wedged writeLoop can't hang the fan-out + item lock. - Stale-SEED clobber: the client announces the item.seq it seeded from (?content_seq=) on every (re)connect; Join force_refreshes any seed that predates the last restore. Residual #1 (restart-durability) CLOSED durably, for BOTH stale vectors — the in-memory fences didn't survive a restart, so a surviving cursor-0 pre-restore browser tab wasn't fenced on reconnect. Two nullable per-item columns (migration 075 SQLite / pg 053), both stamped in the restore's own tx (atomic with the content write + op-log prune): * items.last_restore_seq — the content generation. Join's stale-seed fence reads it (via store.ItemLastRestoreSeq) when the in-memory fast-path misses (after a restart); if that read errors, Join fails CLOSED via a RETRYABLE plain close (not a force_refresh, which would discard the Y.Doc and spin an unbounded refresh loop) so the client reconnects with backoff, Y.Doc intact. * items.restore_boundary_op_id — the op-log-id boundary. The collab-snapshot flush gate reads it (via store.ItemRestoreBoundaryOpID) when the in-memory RestoreBoundary misses (after a restart), failing closed (409) on a read error, so a surviving tab's stale HTTP flush is fenced too. No SCHEMA_VERSION bump — durable columns are not a Y.Doc node-spec change. Deferred to BUG-2276: (a) a Postgres commit whose ack is lost is treated as rolled-back (needs commit-outcome reconciliation; SQLite unaffected); (b) a restore rollback racing an in-flight external-applier ack can drop the ack and retry/fall back (needs the applier flow serialised under itemLock at a 30s-stall cost). NOTE(BUG-2270): ForceVersion can mint same-second version rows; the item_versions ordering tie-breaker is tracked separately. Claude-Session: https://claude.ai/code/session_01EZ6yr6pAUFb1uffan912ra	2026-07-21 15:21:57 -04:00
xarmian	bcef802335	fix(security): gate collab WebSocket writes on editor role (TASK-265) (#938) ... * fix(security): gate collab WebSocket writes on editor role (TASK-265) The collab WebSocket (GET /api/v1/collab/{itemID}) is mounted outside the /{slug} subrouter, so RequireWorkspaceAccess never runs on it. authorizeCollabAccess gated admission on membership + item visibility but NOT edit role, so a plain workspace VIEWER was admitted and could WRITE: every inbound Yjs sync frame persisted to item_yjs_updates (room.go) and got canonicalized into items.content when a co-present editor's authorized flush ran. The REST write path blocks viewers via requireEditPermission; this closes the equivalent gap on the collab relay. Fix — non-editors become READ-ONLY participants (not hard-rejected, so live view + presence stay intact): - authorizeCollabAccess now returns a collabAccess{canWrite} alongside the admission decision. canWrite is computed once via store.ResolveUserPermission (the same predicate requireEditPermission falls back to): owner/editor membership grants write; a viewer/guest gets write only through a collection/item edit grant. - RoomManager.Join takes a canWrite flag stored per-connection as an atomic.Bool. room.go's readLoop drops a read-only conn's inbound sync frames (not persisted via AppendYjsUpdate, not rebroadcast); awareness (presence) frames still relay so the viewer's cursor stays visible, and outbound broadcasts from editors still reach the viewer. - The handler's periodic revalidation pushes mid-session write-permission changes via a new RoomManager.SetConnWritable, so an editor demoted to viewer becomes read-only without a reconnect (complements the existing CloseConn-on-revocation path). No SCHEMA_VERSION / DefaultSchemaVersion bump: this is an authorization / behavioral change, not a ProseMirror/Y.Doc node-spec change, so the op-log must not be pruned. Tests: TestCollabViewerIsReadOnly (viewer admitted 101, receives an editor's broadcast, but its own sync frame is neither persisted nor broadcast while the editor's is) and TestAuthorizeCollabAccessCanWrite (viewer→canWrite=false, editor→canWrite=true). Verified the E2E test fails with the gate removed. Claude-Session: https://claude.ai/code/session_01EZ6yr6pAUFb1uffan912ra * fix(security): close 4 collab read-only gaps from orchestrator review (TASK-265) Independent Codex pass on the collab editor-role gate found four gaps: [P1] Read-only conns were still eligible designated APPLIERS. A viewer (or an editor demoted mid-session) could be elected to apply an external content edit; its resulting sync frames were dropped by the new gate, yet its applier_ack was accepted → ApplyExternalContent reported success → the PATCH handler skipped its direct-write fallback → the external edit was silently lost. Fix: pickApplier now skips non-writers, and handleControlMessage ignores applier_ack from a conn whose canWrite is false (belt-and-suspenders so the fallback fires). [P2] Demotion TOCTOU. readLoop read canWrite=true, then could block on appendMu and persist AFTER SetConnWritable(false) returned. Fix: the canWrite check now runs INSIDE the appendMu critical section, and SetConnWritable stores the flag under the same appendMu — so a frame racing a demotion is either fully persisted before the flip or dropped. [P2] Revalidation could run before the conn was registered. The first jittered tick could fire while Join was still setting up; SetConnWritable would no-op against the unregistered conn and Join then installed the stale canWrite=true until a later tick. Fix: Join takes an onRegistered callback invoked right after addConn; the handler gates the reval loop on it so the first SetConnWritable always finds the conn. [P2] canWrite didn't mirror REST for editors/owners. It was computed purely from ResolveUserPermission, which resolves item/collection GRANTS before membership role — so an editor/owner holding an incidental `view` grant was wrongly made read-only. Fix: mirror requireEditPermission exactly — editor/owner MEMBER short-circuits to canWrite=true BEFORE grant resolution; viewers/guests still fall back to ResolveUserPermission so grants can override an insufficient role. Tests: TestApplyExternalContentSkipsReadOnlyApplier (verified failing without the pickApplier gate), TestHandleControlMessageIgnoresAckFromReadOnlyConn, TestCollabDemotionMakesConnReadOnly (mid-session demotion → read-only without reconnect), and two new TestAuthorizeCollabAccessCanWrite cases (editor+incidental view grant → true; viewer+edit grant → true). Gates: build, gofmt, vet, golangci-lint (0 issues), go test ./internal/server/ ./internal/collab/, and go test -race ./internal/collab/ all pass. Claude-Session: https://claude.ai/code/session_01EZ6yr6pAUFb1uffan912ra * fix(security): safe no-applier direct write for read-only-only collab rooms (TASK-265) Codex round 2 found a P1 introduced by excluding viewers from applier election: in a room whose only peers are read-only, an external content update (PATCH) hits ErrNoApplierAvailable, then PruneAndApply refused to prune because live conns existed (len(r.conns) > 0). After the retry budget the PATCH handler fell through to an UNLOCKED, UN-PRUNED direct write — items.content was updated but the stale op-log survived, so a fresh editor replaying it (or a viewer promoted to editor flushing its stale in-memory Y.Doc) would silently overwrite the external update. Fix: PruneAndApply now blocks only on a live WRITER peer — a read-only peer can never persist, so it doesn't force the unsafe fallback. After the prune + write succeeds it evicts the read-only peers (Room.closeReadOnlyConns: WriteControl close frame + Close, concurrency- safe with writeLoop) so their now-stale Y.Doc can't linger; they reconnect and lazy-seed from the fresh items.content (their old resume cursor is below the pruned op-log's MIN → force_refresh). Mixed rooms (an editor present) are unaffected — the editor is still elected applier and PruneAndApply is never reached. Tests: TestPruneAndApplyEvictsReadOnlyRoom (read-only-only room prunes + evicts, applyFn runs) and TestPruneAndApplyBlockedByLiveWriter (a live writer still yields ErrRoomActiveDuringPrune). Gates: build, gofmt, vet, golangci-lint (0 issues), go test ./internal/server/ ./internal/collab/, and go test -race ./internal/collab/ all pass. Claude-Session: https://claude.ai/code/session_01EZ6yr6pAUFb1uffan912ra * fix(security): fence PruneAndApply read-only eviction under appendMu (TASK-265) Codex round 3 P1: PruneAndApply classified writers, ran applyFn (prune + write), and evicted read-only conns WITHOUT holding room.appendMu. A concurrent viewer→editor revalidation could set canWrite=true after the writer check, append a stale frame during the prune/write, and — now a writer — evade closeReadOnlyConns, racing the prune and leaving a live stale Y.Doc that overwrites the external update. Fix: PruneAndApply now holds room.appendMu across the ENTIRE sequence (writer classification + applyFn + eviction). appendMu is the same lock readLoop takes across its canWrite-check+persist and SetConnWritable takes when flipping canWrite, so a promotion can no longer interleave with the classification/prune. Lock order is itemLock → appendMu → room.mu; no path takes room.mu → appendMu, so no inversion. Also closes the residual "frame already read, blocked on appendMu, then promoted after release" window: roomConn gains a terminal `evicted` atomic flag set by closeReadOnlyConns (under appendMu) and checked in readLoop's persist gate alongside canWrite, so an evicted read-only conn's in-flight frame is dropped even if a racing revalidation promotes it in the same instant. Gates: build, gofmt, vet, golangci-lint (0 issues), go test ./internal/server/ ./internal/collab/, and go test -race ./internal/collab/ all pass. Claude-Session: https://claude.ai/code/session_01EZ6yr6pAUFb1uffan912ra * refactor(collab): descope read-only eviction; keep writer-aware prune guard (TASK-265) Per orchestrator scope decision, remove the read-only EVICTION machinery added during review (over-engineering for TASK-265's security goal): - Room.closeReadOnlyConns and its call in PruneAndApply. - roomConn.evicted flag and its check in readLoop's persist gate. - appendMu held across PruneAndApply + the force-close socket I/O. PruneAndApply reverts to no appendMu / no socket I/O, keeping only the LOAD-BEARING writer-aware guard: it blocks (ErrRoomActiveDuringPrune) only on a live WRITER peer, not any conn. An all-viewer room's external edit therefore still prunes + direct-writes safely (op-log pruned, so a fresh editor lazy-seeds from the new items.content) instead of erroring. The residual — a connected read-only peer keeps a possibly-stale Y.Doc until reconnect/refresh, and a viewer promoted to editor before re-sync could push stale content — is a low-severity lost-update edge (a promoted viewer is a legitimate editor), consistent with the pre-existing direct-write contract. Documented on PruneAndApply and tracked in BUG-2103 (proposed fix: proactive re-seed/refresh of remaining read-only peers). Kept unchanged: the authorizeCollabAccess canWrite editor/owner role short-circuit, dropping read-only inbound sync frames under appendMu + the SetConnWritable demotion fence + registration ordering, and the pickApplier / applier_ack read-only exclusions. Tests: replace TestPruneAndApplyEvictsReadOnlyRoom with TestPruneAndApplyAllowsReadOnlyRoom (read-only-only room -> applyFn runs); keep TestPruneAndApplyBlockedByLiveWriter. Gates: build, gofmt, vet, golangci-lint (0 issues), go test ./internal/server/ ./internal/collab/, and go test -race ./internal/collab/ all pass. Claude-Session: https://claude.ai/code/session_01EZ6yr6pAUFb1uffan912ra * fix(security): enforce token write-scope + fence prune promotion on collab (TASK-265) Two logic gaps from the orchestrator's final pass: [P1] canWrite ignored BEARER-TOKEN SCOPE. The collab upgrade is a GET, so a read-scoped PAT/OAuth token passes TokenAuth's method-keyed tokenScopeAllows check, then rode the user's editor role (or the legacy workspace-token grant) to canWrite=true and could persist Yjs mutations over the socket — a read-only-principal-writes bypass via token scope instead of role. REST DOES enforce write-scope (TokenAuth → tokenScopeAllows blocks read-scoped tokens from PATCH/POST/DELETE); the collab GET simply slips the method gate. Fix mirrors REST: TokenAuth now stashes the token scopes (WithTokenScopes, as MCPBearerAuth already does), and authorizeCollabAccess re-applies the write-capability half — canWrite is downgraded to read-only when the caller's token scope doesn't permit writes (http.MethodPost representative verb). Applied to both the legacy workspace-token path and the member/grant path. Non-token principals (cookie / CLI session, fresh install) carry empty scopes → unrestricted → unaffected. Test: an editor with a read / pad:read token gets canWrite=false; with write / * gets canWrite=true. [P2] PruneAndApply's writer-scan was not serialized with SetConnWritable, so a viewer promoted during applyFn could append a stale frame while the op-log is pruned + content written (persist/prune ordering race, distinct from BUG-2103's async residual). Fix: hold room.appendMu across the writer-scan AND applyFn. Safe now that eviction/socket-I/O is gone — applyFn is a pure store op (PruneYjsUpdatesBefore + UpdateItemWithParentLink, the only caller) that never re-enters itemLock / appendMu / room.mu, so no inversion or re-entrant deadlock. Lock order: itemLock → appendMu → room.mu. Gates: build, gofmt, vet, golangci-lint (0 issues), go test ./internal/server/ ./internal/collab/, and go test -race ./internal/collab/ all pass. Claude-Session: https://claude.ai/code/session_01EZ6yr6pAUFb1uffan912ra * fix(security): honor token write-scope in collab fresh-install branch (TASK-265) The zero-user (pre-bootstrap) branch of authorizeCollabAccess returned canWrite=true unconditionally. A legacy workspace token still carries a scope on a fresh instance, so a read-scoped token could persist Yjs mutations over the collab GET upgrade — inconsistent with REST, whose method gate blocks a read token's mutation. Route the branch through collabTokenWriteScopeAllowed, which returns true for the anonymous (no-token) setup caller (empty scopes = unrestricted) and false for a read-scoped token. Adds TestAuthorizeCollabAccessFreshInstallTokenScope. Found by the orchestrator's independent Codex pass. Claude-Session: https://claude.ai/code/session_01EZ6yr6pAUFb1uffan912ra	2026-07-15 01:30:12 -04:00
xarmian	f29fb65bc0	fix(collab): dedup binary frames by content in replay-cursor test (BUG-1924) (#797) ... TestRoomManagerCursorSuppressedDuringReplay flaked with "binary frames seen: want 4, got 5" because runConn deliberately starts the writer before replayTo, so a live op appended during the replay window can be legitimately delivered twice (once live, once via replay), tolerated by Yjs idempotency. Count distinct op payloads instead of raw frames so the designed duplicate is tolerated while a truly dropped op still fails the assertion.	2026-07-03 18:56:26 -04:00
xarmian	18087463ce	feat(collab): op-log cursor protocol — force-refresh + watermark advance (TASK-1319) (#472) ... * feat(collab): op-log cursor protocol — force-refresh + watermark advance (TASK-1319) Closes both holes left by TASK-1309: 1. Long-disconnected tab + external-write race. A reconnecting client announces its highest applied item_yjs_updates.id via `?since=<id>`. If that id is below MIN(id) for the item, rows it expected to replay have been pruned and the server sends a `force_refresh` control frame and closes the conn. Client recreates the Y.Doc and lazy-seeds from items.content. Without this, Tab A's stale state would silently overwrite an external CLI/MCP write on the next 5s flush. 2. Browser-only-edited items never GC'd. Browser collab-snapshot PATCHes now carry an op_log_cursor body field. The store advances items.content_flushed_op_log_id only when the cursor matches the current MAX(op-log.id) — proving the markdown captures every persisted op. SQL CASE clause re-evaluates MAX at COMMIT time so a peer op landing between client-side cursor capture and the UPDATE leaves the watermark untouched (no over-advancement). Combined cursor mechanism: - Server attaches op_log_cursor JSON control frames after replay, after every successful AppendYjsUpdate (originator), and to every peer's binary fan-out (so all peers stay in lockstep without a round trip). - Client persists per-tab in sessionStorage (NOT localStorage — avoids cross-tab cursor leakage that would force-refresh stable sessions). - Server's MIN(id) check + force_refresh fires only when a non-zero `since` is below MIN; `since=0` is treated as a fresh client. New store methods: MinOpLogID, MaxOpLogID. New ItemUpdate field: OpLogCursor *int64. New control message types: op_log_cursor, force_refresh. New OpEvent.OpLogID for cursor piggyback. Existing collab tests updated to drain TextMessage cursor frames. Tests cover: initial cursor frame after replay (populated + empty op-log), force_refresh fires when since<MIN, delta replay when since>=MIN, cursor broadcast to originator + peers on append, and watermark advancement gated on cursor==MAX. Parent: PLAN-1248. Builds on TASK-1309. * fix(collab): skip stale-Ydoc flush on force_refresh teardown per Codex review (round 1) A force_refresh tear-down means the local Y.Doc cursor is below the server's MIN(item_yjs_updates.id) — its derived markdown is stale. Without this guard the collab $effect cleanup runs flushCollabNow on the way out and silently PATCHes that stale markdown back to items.content, overwriting the canonical content the fresh provider is supposed to lazy-seed from. Per Codex round 1 [P1] of TASK-1319. * fix(collab): force_refresh on empty op-log + cancel pending flush per Codex review (round 2) Two P1 fixes: 1. Manager.Join now force_refreshes when since>0 and the op-log is empty (hasMin==false), not just when since<MIN. After PruneAndApply wipes the entire op-log, MIN is undefined; the original predicate would have admitted the stale tab and let its on-open Y.encodeStateAsUpdate write resurrect the pre-prune document. 2. The +page.svelte onForceRefresh handler now also clears collabFlushTimer. Without this a 5s timer that armed before the force_refresh frame arrived can still fire AFTER the cleanup ran, PATCHing stale Y.Doc-derived markdown to items.content. New test: TestRoomManagerForceRefreshOnEmptyOpLogWithSince covers the empty-op-log branch. Per Codex round 2 [P1] of TASK-1319. * fix(collab): include forceRefreshNonce in Editor key so it remounts on force_refresh per Codex review (round 3) The collab $effect cleanup runs on forceRefreshNonce bump, but the <Editor> {#key} was `${item.id}:true` — itemID doesn't change, so the keyed Editor wasn't unmounting. The Tiptap Collaboration extension only binds in onMount, so the editor stayed wired to the stale (destroyed) Y.Doc while a fresh provider+doc were set up in parallel. Edits would either be unsynced or eventually flush stale markdown again. Adding forceRefreshNonce to the key forces the Editor to remount in lockstep with the doc swap. Per Codex round 3 [P1] of TASK-1319. * fix(collab): refetch item.content before lazy-seed on force_refresh per Codex review (round 4) After force_refresh the collab $effect rebuilds the Y.Doc and the lazy-seed (TASK-1261) seeds it from item.content. But item.content was the cached page-state copy — possibly stale relative to the server (the WS force_refresh can beat the SSE/visibility refresh that would otherwise update it). Lazy-seeding stale content into a fresh op-log re-introduces exactly the staleness force_refresh was supposed to clear: the next 5s flush PATCHes that stale view back to canonical items.content. onForceRefresh now does an api.items.get() before bumping the nonce so the rebuild's lazy seed reads server-fresh content. A failed fetch falls through to the bump anyway (an editor on possibly-stale content is still better than a broken editor). Per Codex round 4 [P1] of TASK-1319. * fix(collab): suppress cursor during replay + move force_refresh check before getOrCreate per Codex review (round 5) Two more findings: 1. [P1] writeLoop sends op_log_cursor frames for live ops broadcast during the replay window. A client disconnecting after one of those cursors lands but BEFORE the rest of replay completes would persist a cursor pointing past unreplayed rows. On reconnect with since=that-cursor, server replays nothing — the client's Y.Doc would be missing causally-required ops. Fix: per-roomConn replayDone atomic.Bool. writeLoop suppresses cursor frames while it's false. runConn flips it after the post-replay initial cursor is on the wire. Live binary frames continue to flow during replay (Yjs CRDT commutativity); only the cursor metadata is gated. 2. [P2] Force-refresh path leaked an empty room. getOrCreate inserted into m.rooms before the force_refresh bail-out left an orphan entry that PruneSweep would later treat as 'active' and skip indefinitely. Fix: schema-rebuild + force_refresh checks now run BEFORE getOrCreate. Both are store-only mutations and the per-item lock is held throughout, so concurrency is unchanged. New test: TestRoomManagerCursorSuppressedDuringReplay regression- guards the cursor-suppression behaviour. Per Codex round 5 [P1+P2] of TASK-1319. * fix(collab): tighten initial cursor + sync-destroy provider on force_refresh per Codex review (round 6) Two more P1 fixes: 1. runConn's empty-replay fallback used MaxOpLogID() to anchor the initial cursor. A live op landing between replayTo returning and the cursor write would be reflected in MAX but its binary frame might not have flowed through this conn's writeLoop yet — the cursor would advertise an id the client hasn't received. Initial cursor is now strictly max(highestReplayed, since); MaxOpLogID is removed from the opLogStore interface. 2. Provider.handleControlMessage's force_refresh branch now calls this.destroy() SYNCHRONOUSLY before invoking the onForceRefresh callback. Previously the consumer's recovery path (async items.get refetch) would race the provider's own onClose-triggered reconnect, which would re-open with since=0 and push Y.encodeStateAsUpdate of the stale Y.Doc — recreating the corruption force_refresh was meant to prevent. destroy() sets destroyed=true so scheduleReconnect short-circuits. Per Codex round 6 [P1] of TASK-1319. * fix(collab): block flush scheduling during force_refresh recovery per Codex review (round 7) Previously, after onForceRefresh fires: 1. Provider is destroyed synchronously. 2. Async items.get refetch is in flight. 3. forceRefreshNonce bumps after refetch resolves. 4. $effect cleanup runs, then rebuild. But during steps 2-3 the editor component is still mounted with the stale Y.Doc, and a local edit fires handleContentUpdate which calls scheduleCollabFlush. clearTimeout earlier in onForceRefresh only canceled the timer at THAT moment; a new edit during the refetch window arms a fresh timer that fires before cleanup. That PATCHes stale Y.Doc-derived markdown back to canonical content, recreating the corruption force_refresh was meant to prevent. Fix: forceRefreshInFlight flag set in onForceRefresh, blocks scheduleCollabFlush, resets after the fresh provider is wired (end of $effect run). Per Codex round 7 [P1]. * fix(collab): gate runCollabFlush itself on force_refresh in-flight per Codex review (round 8) scheduleCollabFlush blocked the 5s timer path, but direct callers of flushCollabNow / runCollabFlush (beforeunload handler, rich-to-raw toggle) bypassed the guard. A page reload or raw toggle DURING the force_refresh recovery window still PATCHed stale Y.Doc-derived markdown to canonical items.content. Pulling the guard into runCollabFlush covers every caller in one spot and returns 'deduped' so the result-shape contract holds. Per Codex round 8 [P1] of TASK-1319. * fix(collab): distinct 'skipped' result for force_refresh path; raw-toggle aborts per Codex review (round 9) runCollabFlush returning 'deduped' on the force_refresh-blocked path was indistinguishable from a legitimate same-content dedupe. The rich→raw toggle treats 'deduped' as 'server already has this markdown' and seeds rawSeedMarkdown from it — letting the user's next raw edit overwrite canonical items.content with content derived from the stale Y.Doc. Add a distinct 'skipped' result for the force_refresh path. Raw toggle aborts on it (with a 'try again in a moment' toast); other callers fall through unchanged because no other call site behaviorally depends on 'deduped' vs 'skipped'. Per Codex round 9 [P1] of TASK-1319. * fix(collab): server-side gate + post-await client guard against stale collab-snapshot per Codex review (round 10) A force_refresh frame can arrive WHILE a collab-snapshot PATCH is already mid-flight to the server. The client-side forceRefreshInFlight check at PATCH-start can't catch this race; the request lands at the server with stale Y.Doc-derived markdown. Two-pronged fix: 1. Server: handler now checks op_log_cursor against MIN(op-log.id) for collab-snapshot PATCHes and returns 409 Conflict when cursor < MIN. Such cursors prove the flushing tab's Y.Doc was built on rows that have been pruned (PruneAndApply, schema rebuild, dormant GC). The markdown is, by construction, stale. 2. Client: post-await check on forceRefreshInFlight returns 'skipped' instead of 'flushed' so saveStatus / lastFlushedContent don't seed from a known-stale base even if the server happened to accept the PATCH (e.g. MIN advanced after handler validation). New tests: TestCollabSnapshotRejectsCursorBelowMin (gate fires), TestCollabSnapshotAcceptsCursorAtOrAboveMin (negative path). Also de-leak an unused slice in the round-5 cursor-suppression test so staticcheck stays clean. Per Codex round 10 [P1] of TASK-1319. * fix(collab): reject collab-snapshot when cursor>0 and op-log empty per Codex review (round 11) The HTTP-layer gate I added in round 10 mirrored only PART of the WS-upgrade force_refresh predicate. Round 5 had already taught us that 'op-log entirely pruned' is a separate stale path from 'cursor below MIN' (PruneAndApply, schema rebuild, dormant GC all leave hasMin=false), and the WS check now uses `since > 0 && (!hasMin || since < minID)`. The HTTP gate had only the second clause. Mirror the WS predicate at the handler so a stale collab-snapshot PATCH against an empty op-log gets a 409 too. New regression: TestCollabSnapshotRejectsCursorOnEmptyOpLog. Per Codex round 11 [P1] of TASK-1319. * fix(collab): reject collab-snapshot cursor=0 on non-empty op-log per Codex review (round 12) Round-11 gate accepted cursor=0 unconditionally. But a stateful tab whose previous session disconnected BEFORE receiving the post-replay cursor frame (network blip during the writeMu burst between replay binaries and the cursor) ends up with sessionStorage cursor=0 + a non-empty Y.Doc populated by prior replay binaries. On reconnect with since=0 the server treats it as fresh, replays nothing if the op-log was meanwhile pruned, and the client's on-open Y.encodeStateAsUpdate resurrects pre-prune ops. The next flush carries cursor=0 + stale-derived markdown. The gate now refuses any incompatible cursor: - cursor>0 + empty op-log (prior rule) - cursor<MIN + non-empty op-log (prior rule, now naturally catches cursor=0 too because 0 < any positive MIN) The WS replay path is unchanged — full replay from since=0 is the recovery for clients that genuinely lost their cursor; the corruption manifested through the flush PATCH which we now gate. New test: TestCollabSnapshotRejectsCursorZeroOnNonEmptyOpLog. Per Codex round 12 [P1] of TASK-1319. * fix(collab): close cursor=0 client/server gaps + lock validation+write atomically per Codex review (round 13) Four P1 issues addressed: 1. Client always sends op_log_cursor (including 0) so the server gate sees the field. Previously cursor=0 was omitted, which silently bypassed the server's stale-snapshot rejection. 2. Provider construction now resets sessionStorage cursor to 0 when the Y.Doc is empty. The Y.Doc isn't persisted across page reload, so a stored cursor=N + fresh empty Y.Doc would announce since=N to the server and miss rows 1..N from replay (server only replays id > N). 3. onOpen skips Y.encodeStateAsUpdate when lastOpLogID === 0. A populated Y.Doc + cursor=0 is the network-blip-during-cursor- write failure mode; pushing that state can resurrect ops the server has pruned. Server replay + lazy-seed handle recovery without our push. 4. Server gate now runs INSIDE the per-item collab setup lock (new RoomManager.UnderItemLock helper) so a concurrent prune (PruneAndApply, schema rebuild, dormant GC) cannot land between the MIN check and the items.content write. Without this, a tight race let stale snapshots overwrite canonical content the prune just installed. Per Codex round 13 [P1] of TASK-1319. * fix(collab): gate handleDocUpdate on cursorAnchored to close stale-Ydoc edit path per Codex review (round 14) Round 13 fix skipped on-open send for lastOpLogID===0, but local edits via handleDocUpdate still propagated. A populated Y.Doc + no-cursor-yet client could type, the edit would land in the op-log with id N, server would send originator cursor=N, and the next 5s flush would carry an 'anchored' cursor that passed the server's MIN check — overwriting items.content with stale- Y.Doc-derived markdown. Add a cursorAnchored boolean. Set on first op_log_cursor frame receipt (including cursor=0 against an empty op-log — that's a legitimate 'server has nothing' signal). handleDocUpdate refuses to send before this. Local edits buffer in the editor; once the cursor arrives (or force_refresh rebuilds the provider), the existing reconnect/edit paths catch them up. Per Codex round 14 [P1] of TASK-1319. * fix(collab): buffer + flush pre-anchor local updates per Codex review (round 15) Round 14 silently dropped local Yjs updates fired before the first op_log_cursor frame anchored the session. Yjs updates are incremental: a dropped keystroke leaves later ops referencing structs no peer can resolve, breaking convergence. Buffer pre-anchor updates in a Uint8Array[] (capped at 1000 to prevent unbounded growth in pathological 'anchor never arrives' scenarios — overflow triggers force_refresh-style recovery). On the first cursor frame, flush the buffer in order so the server gets every causally-required struct before any post- anchor updates land. Per Codex round 15 [P1] of TASK-1319. * fix(collab): destroy provider before force_refresh on pre-anchor buffer overflow per Codex review (round 16) Round 15 overflow path called onForceRefresh but didn't destroy the provider synchronously. A late op_log_cursor arriving before the page-level rebuild (the recovery callback is async — refetches items.content) would flip cursorAnchored=true, the partially- populated buffer would flush, but the DROPPED prefix (the overflowed entries) would leave server-side ops causally incomplete — exactly the bug the buffer was supposed to prevent. destroy() sets destroyed=true, removes message listener, short-circuits scheduleReconnect, closes the socket. Late cursor frames can no longer anchor a doomed provider. Per Codex round 16 [P2] of TASK-1319. * fix(collab): refuse rebuild on refetch fail + broaden on-open gate to cursorAnchored per Codex review (round 17) Two findings: [P1] force_refresh recovery bumped forceRefreshNonce in finally even when the item.content refetch failed. The rebuild then lazy-seeded from the cached (possibly-stale) item.content, and the next flush would PATCH that stale view back to the server. Move the bump into .then() so a failed refetch surfaces a 'please reload' toast and leaves the editor effectively read-only (forceRefreshInFlight stays true, blocking flushes). [P2] Send-on-open gate was lastOpLogID > 0, which silently dropped local edits made during a brief offline window after a legitimate 'cursor=0' anchor (empty op-log session). Switch to cursorAnchored — the boolean specifically distinguishes 'unanchored' (stale Y.Doc + no server confirmation) from 'anchored at cursor=0' (legitimate empty op-log). Per Codex round 17 [P1+P2] of TASK-1319. * fix(collab): force_refresh on cursor=0 against non-empty Y.Doc per Codex review (round 18) cursor=0 means the server's op-log is currently empty. A non-empty Y.Doc at first-cursor receipt implies the ops came from an earlier connection within this provider's life that never reached its post-replay cursor frame, followed by a server-side prune (PruneAndApply, schema rebuild, dormant GC) during our disconnect. Anchoring at cursor=0 in that state would mark a stale Y.Doc as authoritative; the next on-open state push or flush would resurrect pre-prune state and overwrite canonical items.content. Detect the configuration via Y.encodeStateVector length and invoke the same force_refresh-style recovery the explicit server frame triggers: destroy provider, clear sessionStorage, fire onForceRefresh so the page rebuilds from items.content. Per Codex round 18 [P1] of TASK-1319. * fix(collab): gate cursor=0 force_refresh on remoteSyncApplied per Codex review (round 19) Round 18 force_refreshed the provider whenever cursor=0 arrived against a non-empty Y.Doc. But local pre-anchor edits (user typed before the initial cursor=0 of a legitimate empty-op-log session arrived) ALSO populate Y.Doc — yet those edits live in preAnchorUpdates and were supposed to flush on anchor. The predicate spuriously triggered force_refresh, dropping the buffered local edits. Track remoteSyncApplied (set when readSyncMessage applies anything to Y.Doc — replay binary or live peer op). Only force_ refresh on cursor=0 when remoteSyncApplied is true: that's the true 'remote replay landed but server now reports empty op-log => mid-session prune' signature. Per Codex round 19 [P1] of TASK-1319. * fix(collab): repair brace mis-merge in wsProvider cursor=0 guard The round-19 patch overlapped the round-18 inner block, producing an extra brace + over-indented body. Collapsing into a single clean block restores parseability without changing semantics beyond what round 19 already documented. * fix(collab): gate syncStep2 reply on cursorAnchored per Codex review (round 20) readSyncMessage writes an inline syncStep2 reply when it receives a peer's syncStep1. That reply embeds our current Y.Doc state. If a peer's syncStep1 arrives before our first op_log_cursor (pre-anchor window), the reply path bypasses handleDocUpdate's cursorAnchored gate and lets potentially-stale Y.Doc state reach the server before the cursor=0 + remoteSyncApplied force_refresh recovery has a chance to fire. Suppress the reply while unanchored. Peer state propagation still works: the buffered preAnchorUpdates flush on anchor, and the lazy-seed rebuild after a force_refresh seeds canonical content from items.content. Per Codex round 20 [P1] of TASK-1319. * fix(collab): fold mid-replay live op ids into post-replay cursor + remoteSyncApplied only on apply per Codex review (round 21) Two more findings: [P1 server] writeLoop suppresses cursor frames during replay to prevent the client persisting a cursor past unreplayed rows. But binary frames for those live ops still go through (commutativity), so the client APPLIES them to its Y.Doc. The post-replay initial cursor only covered max(highestReplayed, since), leaving the cursor below the highest applied op. On empty-replay sessions this trips the client's 'cursor=0 + remoteSyncApplied' force_refresh path and discards buffered pre-anchor edits. Track maxLiveOpLogIDDuringReplay on the roomConn (atomic compare-and-swap) and fold it into the post-replay cursor. [P1 client] remoteSyncApplied was set on every MESSAGE_SYNC, including syncStep1 (which only carries a state vector — it doesn't apply state). A peer's syncStep1 arriving pre-anchor would falsely flag remote-sync-applied and trip the cursor=0 force_refresh on legitimate empty-op-log sessions. Set the flag only after readSyncMessage returns, and only for syncStep2 / update subtypes. Per Codex round 21 [P1] of TASK-1319. * fix(collab): widen writeMu critical section + drop omitempty on op_log_id per Codex review (round 22) Two more P1s: [P1 server] writeLoop's mid-replay record-max happened OUTSIDE writeMu, so runConn's post-replay read could race the record: runConn loads → writeLoop's atomic store of higher value → runConn sends cursor below the live id. Move the entire per-event sequence (binary write + replayDone observation + record-or-send) inside writeMu, and have runConn acquire writeMu around its read+cursor-write+replayDone-flip. The lock serializes the two paths cleanly: writeLoop events that ran first have already recorded; events that arrive after replayDone flips emit their own cursor frames. [P1 protocol] OpLogID had `omitempty` JSON tag — a legitimate cursor=0 (empty op-log session) serialized as `{"type":"op_log_cursor"}` with no op_log_id field. The client's strict-type check then rejected it as malformed, leaving the session unanchored and local edits buffered forever. Drop omitempty so 0 is wire-visible. Other control types (applier_request/ack) carry an extra op_log_id:0 in their JSON, which their client dispatches ignore. Per Codex round 22 [P1] of TASK-1319. * fix(collab): route originator cursor through writeLoop FIFO per Codex review (round 23) readLoop sent the originator's op_log_cursor directly via sendOpLogCursor right after AppendYjsUpdate, bypassing the bus/ writeLoop ordering. With a peer op already queued in rc.bus, the sequence on the wire could be: 1. originator cursor=N (newer local op) 2. peer binary (older op) 3. peer cursor=M < N (rejected by client's max-take logic) Client persists cursor=N. If the client then disconnects before applying the peer binary, reconnect with since=N replays nothing (server has nothing > N) and the older peer op is lost forever to this client's Y.Doc. Fix: writeLoop now processes self events too — skipping the binary echo (the originator already has Y.Doc state) but routing the cursor frame through the same FIFO bus channel as peer ops. The originator's cursor=N now arrives strictly AFTER all older-id peer events on the same channel. Per Codex round 23 [P1] of TASK-1319.	2026-05-09 21:45:46 -04:00
xarmian	028db39217	feat(collab): periodic op-log GC sweeper for dormant items (TASK-1309) (#471) ... The Yjs collab dumb-relay accumulates op-log rows indefinitely in item_yjs_updates. DOC-1307 surfaced 45-second p50 cold-reconnect latency on a single item with 5000 accumulated rows. Without GC, busy items keep growing. This adds a periodic background sweeper that prunes the entire op-log for items that are both DORMANT (no recent activity) AND FULLY FLUSHED (items.content has captured every op-log row). Whole-log only — Yjs op streams are causally linked, prefix-pruning corrupts replay; future cold connects lazy-seed from items.content. Components: - Store.ListDormantOpLogItemsBefore (joins items, filters watermark) - Store.PruneItemOpLogIfDormantBefore (atomic conditional DELETE) - Store.GetItemContentFlushedOpLogID (per-item watermark getter) - RoomManager.PruneSweep (per-item-locked, active-room-skip) - Server.StartOpLogGC / stopOpLogGC (mirrors orphan_gc.go pattern) - cmd/pad/main.go env vars PAD_OPLOG_GC_INTERVAL / PAD_OPLOG_GC_MIN_AGE - New (item_id, created_at) index for the dormancy query - New items.content_flushed_op_log_id column (id-based watermark, monotonic, no clock-skew or second-granularity false positives) + content_flushed_at (informational timestamp) Watermark policy: - Server-driven full-content writes (CLI / MCP / version restore / PruneAndApply) advance content_flushed_op_log_id to MAX(op-log.id) via subquery, atomic with the content UPDATE - Browser collab-snapshot 5s flushes do NOT advance the watermark — they can't prove their markdown captured every peer's ops, so letting them stamp would risk later GC-pruning unsynced peer edits - Schema-mismatch rebuild (TASK-1268) logs a WARN when it drops unflushed ops (data loss is unavoidable on schema bumps but visible) Stop ordering: collab.Close() now runs BEFORE bg.Wait() so a GC goroutine waiting on an itemLock behind an active Join can drain. Migration backfill: items WITH existing op-log rows keep NULL watermark (don't certify); items WITHOUT op-log rows get a synthetic 0 watermark (vacuous, harmless — no rows to compare against). Tests: - 6 RoomManager.PruneSweep tests (dormant prune / default minAge / empty / bails-on-Close / skips-active-room / skips-row-added-mid- sweep via fakeOpLog hook) - 5 Server.OpLogGC tests (prunes-dormant / start-idempotent / preserves-unflushed / backfill-doesnt-certify-unflushed / no-collab-noop) - TestCollabSnapshotDoesNotAdvanceOpLogWatermark in store - TestCollabSnapshotQueryOverridesBodyVersionSource in server (regression for body-attacker bypass) Seven rounds of Codex review — caught 5 P1s and 4 P2s I would have shipped under self-review: 1. Prefix-prune corrupts Yjs replay 2. Stop ordering deadlock 3. Missing index 4. Best-effort flush ⇒ data loss 5. Backfill over-certifies via metadata-PATCH 6. Second-granularity timestamp comparison 7. Schema-mismatch path drops unflushed silently 8. Browser flush stamps watermark beyond Y.Doc 9. Body version_source bypasses server policy	2026-05-09 18:19:33 -04:00
xarmian	9b46be915a	feat(collab): schema-version handshake + mismatch rebuild (TASK-1268) (#466) ... Adds a client→server schema-version handshake on every WS connect and a per-item op-log rebuild path for the case where the server ships a new SCHEMA_VERSION and finds older rows persisted in the op-log. Client side - New web/src/lib/collab/schemaVersion.ts exporting `SCHEMA_VERSION` (currently '1') with a documented bump rule covering Tiptap extension changes, coordinated multi-package bumps, and Y.Doc fragment-shape changes. - wsProvider's defaultCollabUrl appends ?schema_version=... Server side - handlers_collab.go validates ?schema_version against RoomManager.SchemaVersion() BEFORE upgrading the WS; mismatch returns HTTP 400 with code "schema_mismatch". An empty query is treated as legacy '1' for graceful deploys; once the server bumps past v1, missing query becomes a 400 too. - New RoomManager.SchemaVersion() getter. - RoomManager.Join's setup-phase (under itemLock) now calls maybeRebuildOnSchemaMismatch: if the latest persisted op-log row's schema_version disagrees with the manager's current version, the entire item op-log is pruned via PruneYjsUpdatesBefore. items.content is canonical and untouched, so the lazy-seed path (TASK-1261) re-encodes it into ops at the new schema on the next idle tick. - New store method LatestYjsUpdateSchemaVersion. Tests - internal/collab/manager_test.go: three new tests (mismatch prunes, clean version preserves op-log, post-rebuild connects are clean) + fakeOpLog gets LatestYjsUpdateSchemaVersion + PruneYjsUpdatesBefore. - internal/server/handlers_collab_test.go: rejects-schema-mismatch (400), accepts-explicit-match (101). One round of Codex review (CLEAN with two NITs, both fixed).	2026-05-09 11:06:34 -04:00
xarmian	5dc42b60df	feat(collab): wire Yjs WebSocket provider + Y.Doc lifecycle (TASK-1259) (#457) ... * feat(collab): wire Yjs WebSocket provider + Y.Doc lifecycle (TASK-1259) Adds a thin y-websocket-style provider speaking the binary protocol already implemented server-side in internal/collab/room.go. The provider lives in a Svelte 5 .svelte.ts module so connection state (`connected`, `synced`) can be consumed reactively by upcoming UX tasks (TASK-1264 pending-sync indicator, TASK-1265 mobile reconnect). Wire format mirrors the server's first-byte discriminator: 0x00 → y-protocols/sync (persisted to op-log + broadcast) 0x01 → y-protocols/awareness (broadcast only, ephemeral) Lifecycle is bound to the item-detail page via $effect keyed on `${item.id}:${canEdit}` — same key the <Editor> already re-mounts on, so the Y.Doc and provider tear down in lockstep with the editor. View-only viewers (canEdit === false) keep the legacy non-collab editor; their read-only y-binding is deferred to TASK-1266. Reconnect uses 1s/2s/4s/...30s exponential backoff. Sophisticated mobile reconnect (visibility, network state) is TASK-1265. KNOWN TEMPORARY REGRESSION: existing items with non-empty items.content render an empty editor on first open under collab, because the Y.Doc starts empty and TASK-1259 doesn't seed from markdown. TASK-1261 (next in Phase 2) adds the lazy seed-after- initial-sync path. New items + items already round-tripped through collab are unaffected. Drive-by lint cleanup of dead code that escaped Phase 1's make-install-skips-lint loophole: - gofmt -w on internal/collab/{applier,bus,manager}.go - removed unused test/debug helpers Room.peerCount and Room.applierConnCount (re-add with real callers when needed) Parent: PLAN-1248 * fix(collab): gate Editor mount on ydoc + handle applier_request + catch-up state per Codex review (round 1) Three findings from round 1: 1) [P1] $effect constructs ydoc AFTER Editor's onMount runs, so the first mount on an editable item registered StarterKit history instead of the Collaboration extension. The {#key} excluded ydoc, so the editor never re-mounted when ydoc later became truthy → editable users got a non-collab editor while the provider connected to an unused Y.Doc. Fix: gate the editable Editor mount on `ydoc` being ready (`{#if !canEdit} ... {:else if ydoc} ...`). Adds at most one reactive tick of delay; guarantees the first mount has the binding registered. 2) [P1] Provider dropped non-binary WebSocket frames, but the server sends `applier_request` as TextMessage. With TASK-1259 minting active rooms, every concurrent CLI/MCP/API content PATCH would sit blocked for 30s waiting for an ack, then fall back to a direct write — and the in-memory Y.Doc would still hold stale state and clobber it on the next 5s flush. Silent data loss. Fix: parse TextMessage frames as JSON ControlMessage. On `applier_request`, invoke an `onApplierRequest` callback (the page passes `editor.commands.setContent(markdown)`) and send `applier_ack` on success. The ExpiresAtMillis-driven late-apply guard remains TASK-1262's full scope. 3) [P2] Local Y.Doc updates were silently dropped if the socket was closed when handleDocUpdate fired. On reconnect the dumb-relay server can't reconstruct missing updates from a state vector, so any edits made before the first open or during a disconnect could be lost. Fix: after sending syncStep1 in onOpen, also send the current doc state as a single update via `Y.encodeStateAsUpdate(ydoc)`. CRDT idempotency makes this safe on initial open (server already has these ops via op-log replay → sees a no-op update). Larger docs incur a one-time cost on each connection; TASK-1265's mobile-reconnect work can replace this with a buffered queue. * fix(collab): destroy provider during rawMode + enforce ExpiresAtMillis on applier requests per Codex review (round 2) Two findings from round 2: 1) [P1] collabKey ignored rawMode, leaving the WS provider connected while the user edited via RawMarkdownEditor. Raw saves bypass the y-binding (PATCH writes items.content directly), but the server sees an active room → routes the PATCH through the applier flow → no editor mounted → 30s timeout fallback → direct write. The stale Y.Doc still in memory then overwrote the raw save on the next 5s flush after toggling back. Fix: include rawMode in the collabKey derivation so toggling raw destroys the provider (and the in-memory Y.Doc), and toggling back mints a fresh pair that re-seeds from the op-log + TASK-1261's lazy markdown seed. 2) [P1] Provider passed expires_at_millis to the handler but never gated on it. A backgrounded tab that wakes after the server retried or fell back could still apply setContent and overwrite newer peer edits. Fix: enforce the expiry in CollabProvider — check before invoking the handler AND re-check before acking (handlers are awaited and could span the deadline). Suppress the ack if either gate trips; the server interprets "no ack" as "applier unavailable" and falls back cleanly. * fix(collab): prune op-log on direct-write fallback + pre-mutation expiry check per Codex review (round 3) Two findings from round 3: 1) [P1] rawMode toggle to/from rich left a stale op-log: raw saves wrote items.content directly while the destroyed provider's old op-log persisted. Toggling back minted a fresh Y.Doc that replayed the old log → showed pre-raw content → silently overwrote the raw save on the next 5s flush. Fix server-side: when ApplyExternalContent returns ErrNoActiveRoom (no peers in memory, no in-flight Y.Doc state to corrupt), prune the op-log alongside the direct items.content write so future collab sessions start from a clean slate seeded by items.content (TASK-1261's lazy seed). Pruning is intentionally NOT applied to ErrNoApplierAvailable / ErrAllAppliersTimedOut — those paths may have live peers whose Y.Doc state would diverge. 2) [P2] Provider's post-handler expiry check only suppressed the ack, not the actual setContent mutation owned by the page handler. An async handler that crossed the deadline could still write stale markdown into the Y.Doc. Fix: page handler now does its own pre-mutation expiry check inside onApplierRequest before calling setContent. Documented the contract on ApplierRequestHandler — handlers MUST honour expiresAtMillis BEFORE mutating state. * fix(collab): prune op-log on grace-TTL applier-unavailable + suppress autosave when collab active per Codex review (round 4) Two findings from round 4: 1) [HIGH] op-log pruning still skipped ErrNoApplierAvailable. When raw-mode destroys the in-tab provider, the room remains in its 60s grace TTL with zero conns, so the next direct-write PATCH returns ErrNoApplierAvailable (not ErrNoActiveRoom). Stale op-log rows persisted; toggling back within the grace window resurrected pre-raw-save Y.Doc state. Fix: prune op-log on ErrNoApplierAvailable too — the "no live conns" condition makes pruning safe (no peers to corrupt). ErrAllAppliersTimedOut still preserves op-log because peers may still be alive there. 2) [HIGH] Once the WS provider is active the legacy 1.2s content autosave PATCH gets intercepted by the applier path (handleUpdateItem branch added in TASK-1252). On applier success input.Content is nil'd out, so UpdateItem never writes the markdown snapshot. The page's autosave was the only canonical items.content flush in this diff — search / share-page / API consumers would see stale content forever. Fix: short-circuit handleContentUpdate when collabProvider is set. The Y.Doc + op-log are canonical; items.content stays at its pre-collab snapshot until TASK-1260 introduces the proper 5s idle flush with applier-bypass semantics. This is a known Phase-2-internal regression closed by the very next task in this run. * fix(collab): tighten error classification + per-item lock + raw-mode flush per Codex review (round 5) Three findings from round 5: 1) [HIGH] applier.go could return ErrAllAppliersTimedOut even when no applier_request was ever successfully written (a row of write failures followed by no remaining candidates). The handler-side prune skipped that case, leaving stale op-log rows even though no peer received the request. Fix: track `anyWriteSucceeded` across the attempts and return ErrNoApplierAvailable (which prunes) when the loop exits without ever putting bytes on the wire. 2) [HIGH] Race between ApplyExternalContent's no-room classification and the subsequent Prune/UpdateItem: a fresh Join could mint a room and replay the soon-to-be-pruned op-log into a new client, leaving it with stale Y.Doc state that overwrites the freshly-written items.content on the next idle flush. Fix: introduce per-item setup mutex on RoomManager. Join holds the lock across addConn + replayTo and releases it before the long-lived readLoop. New PruneAndApply method wraps the prune+direct-write in the same per-item lock and re-verifies "no live peers" under it (returns ErrRoomActiveDuringPrune if a peer slipped in, in which case the caller falls through to a plain direct write without pruning). Lock order: per-item lock > m.mu > r.mu — Join and PruneAndApply both follow it. 3) [MEDIUM] Raw-mode 1.2s debounce timer could outlive the toggle to rich mode: the deferred PATCH fired post-collab-mint and got routed through the applier path (potentially overwriting newer peer state). Fix: track the latest pending raw markdown in `rawPendingMarkdown`. The Rich-mode button is now an async onclick that awaits a `flushRawIfPending()` synchronous PATCH before flipping `rawMode = false` (which is what activates the collab provider via the collabKey derivation). * fix(collab): evict broken applier conn + retry on prune-race + retain raw pending on PATCH failure per Codex review (round 6) Three findings from round 6: 1) [HIGH] When applier_request write failed, the broken roomConn stayed in r.conns, defeating PruneAndApply's "no live peers" check (which then returned ErrRoomActiveDuringPrune and the handler skipped pruning). Net effect: the prune-safety classification reverted to the round-5 hazard. Fix: in the applier write-failure branch, force-close the conn and call removeConn before continuing to the next applier. Both are idempotent with the readLoop's natural cleanup path (bus.Unsubscribe, conn map delete, conn.Close all tolerate double-invocation). 2) [HIGH] On ErrRoomActiveDuringPrune the handler fell through to a plain direct-write to items.content, bypassing the now-active peer's applier. The peer's stale Y.Doc could still overwrite items.content on the next idle flush. Fix: surface ErrRoomActiveDuringPrune from applyContentViaCollabOnce so the new applyContentViaCollab wrapper can retry the full ApplyExternalContent flow against the freshly-active room. Capped at applyContentMaxRetries=3 to prevent runaway loops if joins keep landing during prune attempts. After exhaustion, returns the same sentinel — the handler's existing `if err == nil { input.Content = nil }` gate falls through to direct write, which is the correct degraded-mode behavior. 3) [MEDIUM] flushRawIfPending cleared rawPendingMarkdown before the PATCH succeeded and the Rich-mode toggle always set rawMode = false regardless of flush outcome. A failed flush could activate collab with unsaved raw edits. Fix: rework flushRawIfPending to return success bool, retain rawPendingMarkdown on PATCH failure, and gate the Rich-button transition on `ok`. Added a re-entrancy guard (rawFlushInFlight) so a rapid double-click waits for the in-flight flush to settle instead of issuing a duplicate PATCH. * fix(collab): drain-loop flushRawIfPending to handle fast-typist edge per Codex review (round 7) [P1] flushRawIfPending snapshotted rawPendingMarkdown then awaited the PATCH; if the user typed during the await, the equality check preserved the newer edit but the function still returned `true` and the Rich-mode handler flipped collab on. The newly-active provider then raced the un-flushed pending raw save — exactly the hazard the guard is meant to close. Fix: rework flushRawIfPending into a bounded drain loop. Each iteration snapshots-PATCHes-clears (with the equality check). The loop runs up to RAW_FLUSH_DRAIN_CAP=5 iterations, returning `true` ONLY when rawPendingMarkdown is null on exit AND no PATCH failed. A fast typist who keeps the queue non-null across the cap returns `false`, leaving the user in raw mode (next click retries). PATCH failure short-circuits with `false` so the toggle stays in raw mode and the unsaved markdown is preserved for retry. * fix(collab): atomic prune+content-write + preserve newer raw edit on stale PATCH response per Codex review (round 8) Two findings from round 8: 1) [P1] PruneAndApply ran the op-log prune under the per-item lock but the items.content write happened later in the post-loop UpdateItem call, OUTSIDE the lock. A fresh Join landing in that gap could replay the now-empty op-log, mint a peer with stale Y.Doc state, and then overwrite the freshly-written items.content on the next idle flush. Fix: applyContentViaCollab now takes a `directWrite` callback that the caller (handleUpdateItem) implements as a content-only UpdateItem. PruneAndApply's applyFn invokes it AFTER the prune so both run inside the same per-item critical section. The trade-off is two DB round-trips when a PATCH carries content + other fields together (rare): the content-only update happens inside the lock; the rest (title, fields, status) flows through the post-loop UpdateItem with input.Content nil'd to suppress the duplicate write. 2) [P1] In flushRawIfPending's drain loop, `item = updated` assigned the server-side snapshot from the just-PATCHed markdown even when a newer raw edit had landed in the meantime. RawMarkdownEditor mirrors `item.content` into its textarea unconditionally (line 16), so the stale assignment would reset the textarea mid-keystroke and lose the queued edit. Fix: only swap in the full updated snapshot when `rawPendingMarkdown === markdown` (no newer edit). Otherwise keep our local content and adopt only the server-side metadata (timestamps, version, modified_by) via spread. * fix(collab): atomic mixed PATCH + raw autosave stale guard + rich→raw seeding per Codex review (round 9) Three findings from round 9: 1) [P1] Toggling FROM rich+collab TO raw mode seeded RawMarkdownEditor from items.content, which is intentionally stale under collab (handleContentUpdate is suppressed while the provider is connected; TASK-1260 closes that gap with a 5s flush). Saving from raw mode would overwrite the live Y.Doc state with a pre-collab snapshot. Fix: when toggling to raw with a connected provider, capture the editor's current Y.Doc-derived markdown via `editor.storage.markdown.getMarkdown()` into a one-shot `rawSeedMarkdown` slot and pre-populate `rawPendingMarkdown` so the first auto-save persists it. RawMarkdownEditor seeds from `rawSeedMarkdown ?? item.content`. Cleared on rich-mode toggle. 2) [P1] The regular debounced raw autosave still assigned `item = updated` from a stale PATCH response. Same stale-snapshot hazard the Round 8 fix closed in flushRawIfPending. Fix: equality-check `rawPendingMarkdown === toSave` before swapping in the server snapshot. On stale, keep local content and adopt only the server-side metadata via spread. 3) [P2] Round 8 split the items.content write (under per-item lock) from the rest of UpdateItem (post-loop), losing atomicity for mixed PATCHes (content + title) and breaking Store.UpdateItem's content-versioning peek at Title. Fix: directWrite callback now invokes the FULL UpdateItem inside the per-item lock. A `fullWriteHandled` flag tells the handler to skip the post-loop UpdateItem entirely (otherwise we'd duplicate the write and create two version-history rows). Mixed PATCHes are atomic again under the lock. * fix(collab): clear raw seed/pending on item navigation per Codex review (round 10) [P1] Navigating between items left rawSeedMarkdown, rawPendingMarkdown, and the contentDebounceTimer set from the previous item. This caused two concrete hazards: (a) Item B's raw editor mounted with item A's live markdown via `rawSeedMarkdown ?? item.content`. (b) Clicking Rich on item B fired flushRawIfPending which PATCHed A's queued markdown INTO item B (cross-item data bleed). Fix: at the top of loadData(), clear contentDebounceTimer, rawSeedMarkdown, and rawPendingMarkdown so each navigation starts from a clean slate. The collab provider's own lifecycle is already keyed on item.id via $effect cleanup, so it doesn't need the same explicit reset. * fix(collab): item-id race guard on raw PATCH responses per Codex review (round 11) [P1] In-flight raw PATCH responses (debounced autosave AND drain loop) could clobber a newly navigated item. Clearing contentDebounceTimer in loadData only cancels timers that have not fired; an awaiting fetch keeps running and its `.then` / `.catch` would assign back to the new page's `item` state. Fix: mirror the existing TASK-754-style race guard pattern (already used in the SSE / sync handlers above). Capture `reqItemId = item.id` BEFORE the PATCH, then in the response handler bail if `!item || item.id !== reqItemId`. Applied to both handleRawContentUpdate's setTimeout body and flushRawIfPending's drain loop. * fix(collab): reset saveStatus on item navigation per Codex review (round 12) [P2] After Round 11's race guard, a stale raw PATCH response that matched a now-different item.id was correctly discarded — but saveStatus had already been set to 'saving' before the await. With loadData not resetting it, the next item could mount with saveStatus pinned at 'saving' indefinitely, which then suppressed all SSE/sync refreshes via the `if (saveStatus === 'saving')` guards above. Fix: in loadData's per-item state reset, clear saveStatusTimer and reset saveStatus to 'idle' alongside the other transient state. Cheap, scoped, no impact on the in-flight save's eventual discard path.	2026-05-08 20:48:57 -04:00
xarmian	50e0936b34	feat(collab): designated-applier protocol for external content updates (TASK-1257) (#455) ... * feat(collab): designated-applier protocol for external content updates (TASK-1257) The keystone task for CLI / API / MCP integration during co-edit sessions. When a content update arrives via PATCH while at least one browser tab is connected to the item's collab room, the server can't write items.content directly — the connected tabs would silently overwrite it on the next 5s idle flush using their (now stale) Y.Doc state and the caller's update would be lost. Solution: nominate one connected tab as the "designated applier", send it a JSON control message with the new markdown, the browser does editor.commands.setContent(markdown) which the y-tiptap binding translates into Y.Doc updates that propagate via the regular sync path. Items.content gets refreshed via the next 5s flush (TASK-1261). Architecture: internal/collab/applier.go (new): - ControlMessage struct — JSON envelope for applier_request / applier_ack frames. Carried over WebSocket TextMessage, which is unambiguous against y-protocol's BinaryMessage. - ApplyExternalContent(itemID, markdown) — public entry point. Returns nil on ack, ErrNoActiveRoom when there's no room (caller falls back to direct write), ErrNoApplierAvailable when the room has no live conns, ErrAllAppliersTimedOut when every attempt expired. - Election: pickApplier returns the longest-connected roomConn that hasn't already been tried, with deterministic tiebreak on conn id. Stable choice — longest connection has the most authoritative cumulative Y.Doc state, fewer flicker risks. - Retry loop: applierMaxAttempts=2, applierFirstTimeoutVar=30s, applierRetryTimeoutVar=15s. The Var-suffixed names exist so test helpers can shrink to ms without sleeping a real minute. - Pending-ack tracking: per-room map[requestID]*pendingApplierAck pairing the channel a PATCH handler is waiting on with the conn the ack is expected from. expectedConn check prevents an unrelated peer from spoofing acks for someone else's request. internal/collab/room.go (extended): - roomConn gains connectedAt for the election. - readLoop branches TextMessage → handleControlMessage which decodes the JSON and routes applier_ack to the room's pending tracker. Unknown control types and malformed JSON are silently dropped so a bad client can't break the loop. internal/collab/manager.go: - Registers connectedAt on Join. - Initialises room.pendingAcks alongside conns map. internal/server/handlers_items.go (extended): - handleUpdateItem now branches on input.Content != nil + s.collab != nil: routes through s.applyContentViaCollab; on success, zeros input.Content so UpdateItem's direct write is suppressed. - Field-only PATCHes skip this branch entirely — backward-compatible. internal/server/handlers_collab.go: - applyContentViaCollab wraps mgr.ApplyExternalContent with per-error-class slog warnings so operators can see degraded paths (timeouts → warn; no-room / no-applier → quiet, the common case for non-co-edit CLI updates). - actorIDFromRequest helper for log fields. Tests (5 new): - TestApplyExternalContentNoActiveRoom — sentinel error path. - TestApplyExternalContentHappyPath — applier echo acks within ms. - TestApplyExternalContentTimeoutsThenFails — applier never acks; we hit applierFirstTimeoutVar then ErrAllAppliersTimedOut. - TestApplyExternalContentTimeoutThenSecondAcks — first applier silent, retry picks second-longest-connected, succeeds. - TestApplyExternalContentRejectsAckFromUnexpectedConn — defence- in-depth: peer B forges an ack for peer A's request; the room's expectedConn check rejects it; ApplyExternalContent runs to timeout instead of being satisfied by the forgery. All tests pass under -race. Full suite green. Parent: PLAN-1248. Phase 1 — Backend foundation. * fix(collab): clean pendingAcks on success + expires_at on applier_request per Codex review (round 1) P2 #1: ApplyExternalContent retained the per-request pendingAcks entry on success. Each successful external update therefore leaked a request_id + channel + expected-conn pointer for the remainder of the room's lifetime — across long-lived sessions the map would grow without bound. Add cancelPendingAck to the ack-success path so the entry is released as soon as the request completes. Test added (TestApplyExternalContentCleansPending- AcksOnSuccess) drives 5 successful applies and asserts the pendingAcks map is empty afterwards. P2 #2: applier_request had no client-enforceable expiry, so a backgrounded tab could process a stale request 60s later and overwrite newer edits with old markdown after the server had already retried with a different applier (or fallen back to direct write). Add ExpiresAtMillis to the ControlMessage envelope, populated per attempt with `now + timeouts[attempt]`. The browser-side handler (TASK-1263) is responsible for the client-side Now() check before applying — without that check the field is documentation-only. Added test (TestApplyExternalContentSendsExpiresAt) regression-tests the server stamp. Server-side cleanup is also reinforced: cancelPendingAck on timeout (already present) means a late ack from a timed-out applier is rejected at the room layer (entry is gone). The expires_at_millis is the second line of defence for the case where the browser sends the Y.Doc setContent BEFORE the ack — the request must not be applied at all.	2026-05-08 16:30:39 -04:00
xarmian	79eb00d2a1	feat(collab): periodic auth revalidation timer (TASK-1256) (#454) ... * feat(collab): periodic auth revalidation timer (TASK-1256) Catches mid-session revocations on a live collab WebSocket the same way handlers_events.go's sseSubscriberStillHasAccess does for SSE. When the WS handler upgrades, it spawns a goroutine that ticks every collabMembershipRevalInterval (60s, jittered across [0, interval) on first fire to avoid post-deploy reconnect-storm spikes). Each tick re-runs authorizeCollabAccess — the same workspace-access ladder used at upgrade time, including the "fresh-fetch user from store" semantics that make admin-demoted-mid-stream visible without waiting for the next request. On access loss the handler routes through a new RoomManager.CloseConn(itemID, conn, code, reason) method which: - Looks up the roomConn in the manager so the close frame can go out under the per-conn writeMu (no concurrent-write panic against the room's writeLoop or replay path). - Sends a websocket.ClosePolicyViolation frame with a human-readable reason ("Your access to this item was revoked.") so the frontend can stop reconnecting in a tight loop. - Falls back to plain conn.Close when the conn isn't tracked yet (race window between Join's getOrCreate and addConn). The goroutine is bound to the handler's lifetime via a `stop` channel that closes when handleCollab returns; no leaked timers or goroutines after disconnect. Test (TestCollabMembershipRevalidationClosesOnRevoke): - Shrinks the reval interval to 30ms so the test runs in tens of ms rather than 60 seconds. - Bootstraps an admin (so the no-users escape hatch is closed), creates a non-admin member user, mints a session, dials in. - Calls RemoveWorkspaceMember while the WS is open. - Asserts the next read returns an error (close frame or transport failure — both are acceptable signals the server tore the connection down). Parent: PLAN-1248. Phase 1 — Backend foundation. * fix(collab): WriteControl for revoke close + tighten test failure modes per Codex review (round 1) P2 #1: CloseConn used rc.writeMessage which acquires the per-conn writeMu. If the room's writeLoop / replay was mid-WriteMessage to a slow peer, revocation would block behind that writer and never force-close the unauthorized conn. Switch to conn.WriteControl, which gorilla documents as concurrency-safe with normal writes (it bypasses the conn's normal write path) and accepts an explicit deadline so a stuck send can't extend the budget indefinitely. The deadline is 1s — generous for a healthy conn, short enough that a half-broken socket falls through to plain Close quickly. The itemID parameter stays in the API for symmetry / future per-room metrics, but is no longer used for the actual close path now that the writeMu lookup is gone. P2 #2: TestCollabMembershipRevalidationClosesOnRevoke previously treated a read-deadline timeout as a log-only branch — the test could pass after waiting 2s with the WS still open, exactly the bug being regression-tested. Restructure to fail fast on timeout (t.Fatalf isTimeout(err)) and prefer ClosePolicyViolation as the expected close code, falling back to "any non-timeout error" only because the underlying TCP teardown can produce different error shapes depending on timing. The isTimeoutOrEOF helper that masked the failure is replaced with a narrowly-scoped isTimeout. * fix(collab): distinguish access denial from transient errors in reval per Codex review (round 2) P-MEDIUM: the revalidation goroutine treated any non-nil error from authorizeCollabAccess as revocation, including transient store errors (GetUser / GetWorkspaceByID / grant-lookup blips). One DB hiccup would close every active collab WS with ClosePolicyViolation, which is a worse UX than the bug being guarded against. Distinguish via errors.As against *statusError (the typed return from authorizeCollabAccess used for all "we know they don't have access" branches). Plain errors fall through to a warn-level log + timer reset so the next tick retries. Three branches in the revalidation switch now: err == nil still authorised — reset timer. isAccessDenial(err) real revocation — close conn with typed reason. default transient — log warn, keep conn open, reset. * fix(collab): re-fetch item on each reval tick per Codex review (round 3) P2: revalidation re-authorized against the *Item captured at upgrade time, so an item moved to a collection the user can't see — or hard-deleted — would not be caught: authorizeCollabAccess kept checking the stale CollectionID, kept passing, and the WS stayed open against an item the user no longer has access to. Re-fetch via s.store.GetItem(itemID) at the start of each tick: - error → log warn, keep conn open, retry next tick (matches the transient-store-error policy from round 2). - nil → item hard-deleted (or never existed): close with ClosePolicyViolation + "This item is no longer available." - otherwise → authorize against the FRESH item, picking up any collection move automatically. Per-tick GetItem is one indexed lookup per minute per active connection — negligible compared to the auth-cascade GetUser / member / grant queries that already run on the same tick.	2026-05-08 16:07:26 -04:00
xarmian	e7b1c3b5ae	feat(collab): per-item Room manager with op-log replay + grace TTL (TASK-1255) (#453) ... * feat(collab): per-item Room manager with op-log replay + grace TTL (TASK-1255) Wires the OpBus + op-log + WS handler from prior phase-1 PRs into a working dumb-relay collab server. Per-item Room created lazily on first Join, kept alive across transient disconnects via a 60s grace TTL, reclaimed when the grace expires with no fresh subscribers. Components: - internal/collab/room.go — Room struct + lifecycle · roomConn pairs (id, conn, bus channel, write mutex). The id is server-assigned per WS so writeLoop can suppress own-event echoes without decoding the Y.Doc to read the Yjs ClientID. · readLoop discriminates yMessageSync vs yMessageAwareness on byte 0. Sync frames are persisted to the op-log AND broadcast; awareness frames are broadcast only (presence is ephemeral). Persistence happens BEFORE broadcast so a crash mid-publish loses at most a live keystroke that the originator will replay on reconnect anyway. · writeLoop drains the bus subscription and writes non-self events to the WS, gated by a per-conn write mutex (gorilla's "one writer at a time" rule). · removeConn arms a 60s graceTimer when the last conn drops; a fresh addConn cancels the timer. onGraceExpired re-checks len(conns) == 0 under the room mutex and only THEN sets closing=true + calls back to the manager. The race between "manager.getOrCreate found us" and "grace timer fired" is handled by addConn returning errRoomClosing; the manager retries via getOrCreate which mints a fresh Room. - internal/collab/manager.go — RoomManager + RoomManagerConfig · NewRoomManager wires production defaults (DefaultGraceTTL = 60s, DefaultSchemaVersion = "1"). NewRoomManagerWithConfig accepts an explicit config so tests can drop graceTTL to a few ms without sleeping a minute. graceTTL is per-manager, not a package var, so parallel tests with different TTLs don't trip the race detector. · Join is the public entry point: getOrCreate → addConn (with retry on errRoomClosing) → replayTo → spawn writeLoop goroutine → run readLoop inline → wait for writeLoop drain → return. The inline read keeps the HTTP handler in scope so its `defer conn.Close()` doesn't fire until both loops exit. · Close is for graceful server shutdown — closes every active conn under the room mutex, then drains the manager's room map. - internal/collab/manager_test.go — 7 tests covering: lazy create, op-log replay-on-connect (two seed rows arrive in order), sync broadcast + persist (peer B sees A's frame, originator does not echo, op-log gains a row), awareness broadcast WITHOUT persist, cross-item isolation (item-a frames don't leak to item-b subscribers), grace-TTL reclaim with a 50ms config TTL, grace cancel on reconnect within window, manager.Close shuts down every active conn. All tests run with -race; the bus's concurrent-publish test was already covered by TASK-1253. - internal/server/handlers_collab.go — wire to RoomManager · Returns 503 when s.collab is nil (matches the SSE handler's "events bus not configured" 503 — fail loud rather than silently accept the upgrade). · Otherwise hands the upgraded conn to s.collab.Join, which blocks until the WS closes. Unexpected close codes get the same warn-log as before; normal closures stay quiet. - internal/server/server.go — adds *collab.RoomManager field + SetCollabRoomManager setter (nil-safe optional, like SetEventBus). - cmd/pad/main.go — wires NewMemoryOpBus + NewRoomManager into the running server alongside the event-bus wiring. Single-instance only today; multi-replica fanout via Redis is a deferred IDEA per the Plan body. - internal/server/handlers_collab_test.go — adds testServerWithCollab helper (so existing collab tests get a real RoomManager) plus TestCollabUpgradeUnavailableWithoutRoomManager which asserts the 503 path for unwired servers. Parent: PLAN-1248. Phase 1 — Backend foundation. * fix(collab): per-room appendMu + Server.Stop closes RoomManager per Codex review (round 1) P1 — concurrent peers raced AppendYjsUpdate, violating the single-writer-per-item contract documented on the store call. Each peer's readLoop runs in its own goroutine, so two peers in the same room could call AppendYjsUpdate concurrently. On Postgres that risks the BIGSERIAL allocation-vs-commit-order cursor gap that TASK-1252's contract was specifically guarding against. Add an appendMu on Room held across the persist+publish sequence; reads, awareness frames, and OTHER rooms remain unserialised. Regression test (TestRoomManagerSerializesSyncAppends) drives 4 peers × 10 writes concurrently and asserts the op-log gains exactly 40 rows. Without appendMu this would intermittently surface fewer rows or out-of-order ids on Postgres; with it the count is deterministic and the race detector stays clean. P2 — Server.Stop did not close s.collab. Active collab WS goroutines + grace timers could keep using s.store after the server's other cleanup paths winding down. Add s.collab.Close() before rateLimiters.Stop so any Join goroutines holding rate-limiter handles can wind down cleanly. nil-safe via the existing collab optional-attachment pattern. * fix(collab): start writer before replay to avoid bus-overflow drops per Codex review (round 2) P2: a joining peer subscribed to live events BEFORE its writer goroutine started. During a long replay, live sync events would pile up in the 64-event bus channel; once full, MemoryOpBus.Publish silently drops them, leaving the new peer connected but permanently missing those updates. Restructure runConn to spawn the writer goroutine FIRST so it drains the bus subscription concurrently with the replay. Both replay and writer go through rc.writeMessage, which holds the per-conn write mutex, so we never violate gorilla's one-writer-at-a-time rule. Yjs CRDTs are commutative — applying live op 100 before replay op 50 yields the same final Y.Doc as the reverse order — so interleaving is correct. The trade-off is a brief "out of causal order" UX wobble during replay, which is acceptable: the alternative would require either an unbounded queue or losing updates the way the original order did. * fix(pad): call srv.Stop() in serveCmd shutdown so collab sessions close per Codex review (round 3) P2: serveCmd's SIGINT/SIGTERM path called srv.Shutdown but never srv.Stop. http.Server.Shutdown does NOT terminate hijacked connections (WebSockets), so active collab sessions kept running until process exit and could race the deferred store close. The RoomManager.Close path added in round 1 only fires inside Stop, so without this call the production shutdown was effectively bypassing the new cleanup. Add srv.Stop() after srv.Shutdown in the serveCmd shutdown sequence. Stop also runs the existing background-loop teardowns (orphan GC, MCP audit writer, MCP session tracker) which were previously already part of Stop's contract — those will continue to fire as they always have, so this commit's only behavioural change is "now also closes the collab room manager". * fix(collab): WaitGroup drain barrier + bigger bus buffer per Codex review (round 3) P1 — RoomManager.Close was not a true drain barrier. closeAll closed the WebSockets but did NOT wait for the corresponding Join goroutines (running runConn) to exit. Server.Stop returned before in-flight collab work finished, racing the deferred store close on process exit. Fix: track every Join in m.activeJoins (sync.WaitGroup); Close iterates closeAll first (waking up every reader by closing the conn), then activeJoins.Wait — guaranteeing no collab goroutine is still running by the time Close returns. P2 — replay-time bus overflow could still drop sync events on a slow drain (writeLoop blocks on the same writeMu replayTo holds, so a long replay starves the bus drain even with the writer goroutine started before replay). Two-part response: (a) Bump the per-subscriber bus channel buffer from 64 to 256. Sized for a 5x safety margin on a 1k-row replay against a chatty 5-peer room (~50 events/sec during a ~1s replay). (b) The architectural fix — force-close subscribers on overflow, honoring the bus's documented slow-peer recovery contract — is filed as TASK-1273 follow-up. That requires extending the OpBus interface (per-subscriber drop callback or counter) and an active health-check tick in the room manager; both are out of scope for TASK-1255's "lazy room + grace TTL" deliverable. For PLAN-1248's single-instance scope and typical editor load, 256 covers realistic workloads. Pathological / load-test scenarios exposing overflow can recover via Yjs's state-vector negotiation on reconnect, and TASK-1273 will tighten that to an active kick. * fix(collab): closed flag gates Join + Close idempotency per Codex review (round 4) P2: http.Server.Shutdown does NOT wait for hijacked WebSocket handlers, so a Join() call from a freshly-upgraded conn could fire AFTER Close() returned. The previous Add-then-Wait pattern was correct for already-started Joins but couldn't catch a Join that hadn't yet hit Add when Close fired. Race: Close iterates the (empty) rooms map, Wait sees zero waiters, Close returns; THEN Join hits Add and proceeds against a torn-down store. Add a `closed` flag gated by the same mutex that wraps activeJoins.Add. Three orderings, all safe: 1. Add before Close.closed=true → Wait blocks until Done. 2. Close.closed=true before Add → Join sees closed=true under the same lock and returns errManagerClosed without ever incrementing the WaitGroup. 3. Close called twice → second call short-circuits (idempotent). getOrCreate also gets a closed-flag short-circuit so a future caller can't bypass the gate by skipping Join. Test: TestRoomManagerJoinAfterCloseFailsFast asserts post-Close Join returns errManagerClosed, plus a second Close() is a no-op. All 15 collab tests pass under -race.	2026-05-08 15:38:45 -04:00
xarmian	264f5b0041	feat(collab): add OpBus interface + in-process MemoryOpBus (TASK-1253) (#451) ... * feat(collab): add OpBus interface + in-process MemoryOpBus (TASK-1253) New internal/collab package for the dumb-relay collab server in PLAN-1248. Defines the OpBus pub/sub interface and ships MemoryOpBus, the in-process implementation used by every shipping target today (single-binary self-host, single-replica pad-cloud). OpBus shape mirrors internal/events.MemoryBus so a future RedisOpBus is a drop-in for multi-replica deployments — that's filed as a separate IDEA at PLAN-1248 close, since the dumb-relay design intentionally keeps Redis off the self-host dependency surface. OpEvent carries: - ItemID fan-out filter - ClientID Yjs client id, used by designated-applier election (TASK-1257); the bus itself does not interpret it - Type "sync" (Y.Doc binary update — persisted by the room manager) or "awareness" (cursor/presence — broadcast only, never persisted) - Data raw y-protocol message; opaque to the server - Timestamp UnixMilli, auto-stamped on Publish MemoryOpBus semantics: - 64-event buffered subscriber channels (matches internal/events default — sized against keystroke-rate workload). - Non-blocking Publish: a slow subscriber whose channel is full has events DROPPED with a warn log rather than back-pressuring the broadcast loop. The room manager (TASK-1255) is responsible for closing genuinely unhealthy peers; the bus only protects itself. - Idempotent Unsubscribe (no panic on double-unsubscribe). - Close clears the subscriber map and closes every channel under the same write lock that gates Publish, so a final inflight Publish can't race a Close into delivering on a closed channel. Tests cover: subscribe/publish fan-out per item filter, unsubscribe closes the channel, slow-consumer drop without blocking, accurate SubscriberCount across subscribe/unsubscribe, Close cleans up every channel regardless of itemID, and concurrent-publishers race-clean under -race (small drop count tolerated — that's the slow-consumer contract; an exact-delivery test would defeat its own purpose). No external dependencies beyond stdlib + slog. RedisOpBus stub is intentionally NOT included — separate IDEA per the Plan body. Parent: PLAN-1248. Phase 1 — Backend foundation. * fix(collab): clone OpEvent.Data + document recovery contract per Codex review (round 1) P1 — Sync-op drops were undocumented as recoverable. Sync drops ARE recoverable in the dumb-relay design: the room manager (TASK-1255) appends to the op-log BEFORE Publish, so any peer that misses a sync op via channel-full drop can replay since their last cursor on reconnect (TASK-1252's LoadYjsUpdatesSince + Yjs state-vector negotiation). The room manager is responsible for detecting slow channels and force-closing the owning WebSocket, which kicks the peer into a fresh reconnect + replay. The bus does not take that action itself because it has no concept of which peer owns which channel — that mapping is the room manager's domain. Doc comment now spells this out explicitly. P2 — OpEvent.Data is a []byte; the same slice header was queued to every subscriber, so a publisher's later buffer reuse OR any subscriber's mutation could corrupt the bytes other subscribers observe. gorilla/websocket's ReadMessage is allowed to reuse its read buffer between messages, so this hazard is real for the production publisher (the WS handler in TASK-1254). Clone Data once at the publish boundary; document the per-receiver immutability expectation in the same comment block. No behavioral test change — the existing slow-consumer-drop test still passes; the clone path adds one allocation per Publish but nothing observable to callers beyond the immutability guarantee.	2026-05-08 13:56:21 -04:00