mirror of
https://github.com/PerpetualSoftware/pad.git
synced 2026-09-11 13:28:57 +00:00
xarmian
e7b1c3b5ae
* 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.