fix(stream): make the broadcast the only writer of the pool cache

[benitogf]

What & why

List subscriptions receive positional JSON-patch ops computed against a per-key pool cache. Two code paths wrote that cache in opposite orders — live inserts kept it ascending by Created, while Server.fetch re-initialized it descending — and fetch ran on every REST read of a glob key and every new subscriber, silently re-ordering the cache under already-attached subscribers with no version bump and no message. The next positional op then addressed the wrong element: a delete removed the wrong row, a replace overwrote the wrong record's data (duplicating one record, losing another).

Symptom in the field: an operator with a list open sees the wrong row vanish on a delete, or a rename land on the wrong record — but only when a collection read or a second subscriber landed in between (which is why a reload appeared to "fix" it).

This restores one invariant: the pool cache changes only through a broadcast that bumps the version and delivers the matching op or snapshot.

The three parts

• F1 — one canonical order, newest-first. insertSorted now maintains descending Created, matching the pinned REST descending contract (ClientCompatibilityTest/ListSortOrder). Every ascending test fixture adjusted.
• F2 — reads never write the pool cache. REST reads serve straight from storage + read filters (fetchREST), with no pool interaction of any kind. A WS subscriber attaching to an already-initialized pool is served its snapshot from the cache under the pool lock (ServeCache — copy under lock, encode outside); only pool creation loads from storage. Applies to list (glob) and single-object pools alike.
• F3 — resync at the observable desync sites. The only remaining ways cache and storage can diverge are counted events — a dropped watch event (DroppedEvents) and a recovered watch panic (WatchPanics). Both now schedule (never inline — the drop callback runs on the storage sender goroutine under storage's lock) a rebuild of the matching pools from storage plus a version-bumped snapshot broadcast, via a coalescing scheduler (buffered channel + per-key set + dedicated worker started in Start, stopped by a PreShutdown hook before storage close). No standing self-heal anywhere else.

Tests

• The two originally-failing tests pass unmodified: TestClientListDeleteAfterCollectionRead, TestClientListDeleteAfterSecondSubscriber.
• New: TestClientListReplaceAfterCollectionRead (rename-after-read holds), TestWsReconnectGateAfterCollectionRead (the ?v= reconnect gate), TestWsResyncAfterDroppedEventSnapshot (F3, via DelSilent divergence injection), TestWsGlobSnapshotAfterGlobDeleteIsEmptyList (glob-delete snapshot is [], not null).
• Full suite green under go test ./... -race -count=1; baseline TestClientListDelete green at -count=10 -race; benchmarks compile and run.

Reviewed

Hard self-review (truthseeker rigor) verified the F3 concurrency (send-under-lock serialized against close, no missed-wakeup/deadlock/send-on-closed, lock discipline mirrors Broadcast, no inline storage I/O on the drop path, worker stopped before storage close), the F2 invariant (the broadcast + resync are the only writers of an initialized pool's cache), and F1 completeness. One blocker found during review (an emptied glob pool served null instead of []) is fixed and guarded.

Two pre-existing items surfaced and tracked, not fixed here: the close-callback budget can skip the resync stop hook (shared with the existing LimitFilter cleanup hook) — #151; and the first-attach fetch+attach window is non-atomic, which this change narrows to first-attach only — #152.

Notes for downstream

Downstream consumers pin ooo by commit; pin bumps happen after this merges. The ooo-client patch-failure resilience work is a separate, independent PR.

🤖 Generated with Claude Code

[CBosch101]

APPROVE — verified locally at head 0666d0d: go build ./..., go vet ./..., and go test ./... -race -count=1 all green (the six named regression/scenario tests pass under -race).

The invariant is restored cleanly on all three fronts:

• F1 (stream/patch.go) — insertSorted flipped to descending; single caller, comment updated, and the patch_test fixtures were reordered to match. Live inserts and fetch re-init now agree on newest-first ordering, so a positional op can't address a re-ordered element.
• F2 (rest.go → fetchREST) — REST reads no longer touch the pool cache, closing the path where an out-of-band read silently re-ordered the cache under attached subscribers with no version bump.
• F3 (scheduleResync/resyncWorker/Stream.ResyncPools) — the coalescing token+set worker is race-clean: send and close both run under resyncMu behind a resyncClosed guard (no send-on-closed); a dropped non-blocking send can't lose a resync because the key stays in resyncSet and a pending token drains the full set; ResyncPools mirrors Broadcast's pool.mutex → nextVersion → broadcastPool discipline (no lock-order inversion) and never resurrects a Version==0 pool.

Both known edges are disclosed and tracked — #151 (the close-callback budget can skip the resync stop hook, gated on a non-default CloseCallbackBudget>0) and #152 (the first-attach fetch+attachConn window is non-atomic, a pre-existing window this change narrows). Neither is introduced here, so neither blocks.

🤖 reviewed by Claude Code (claude-runner) on behalf of @CBosch101

[mhsantoidnerd]

Approving. Reviewing as the reporter of the downstream symptom (bulk #1178) — the diagnosis here is sharper than mine was, and F2 removes the exact trigger I had.

Verified

• F1's binary-search flip is correct, not just inverted: the new predicate finds the first element with Created <= obj.Created, which is the right insertion point for a descending list.
• Ordering is now consistent across every path that seeds a pool — all three list reads use GetListDescending, InitCacheObjectsWithVersion has exactly one caller (the pool-creation branch of fetch), and live inserts are descending. No ascending seed left to reintroduce the mismatch.
• F2 kills my repro's trigger specifically. Every reproduction I ran read the collection over REST before deleting — so my own measurement was creating the desync. With fetchREST touching no pool state that path is gone, and F1 makes the two orders agree even if a fetch did run.
• Not re-run: ooo's own suite. @CBosch101 verified build/vet/test -race at this head and I didn't duplicate it.

Non-blocking

• #152 is mis-scoped — the window is not first-attach-only. ServeCache releases pool.mutex before returning, then Stream.New runs OnSubscribe and the WebSocket Upgrade handshake before attachConn takes the lock (stream/stream.go:279-298). A broadcast landing in that interval reaches neither the snapshot (copied earlier) nor the conn (not yet in pool.connections), and the conn is stamped with the pre-broadcast version — so a later positional op applies to a stale base on any attach, not only pool creation.
  Nor is the window meaningfully shorter: it still spans a network handshake, which dominates the storage read F2 removed. Worth correcting #152's scope, and noting that only its second suggested direction (resolve the snapshot after registering, under one critical section) covers the non-first-attach case — locking across the storage load fixes just the first.

Downstream heads-up for the pin bump: consumers watching a list get built up incrementally will see it newest-first now, where ascending inserts previously put new records at the wrong end. That is the fix working, but it is a visible ordering change in any UI that renders subscription order directly.

---

🤖 Generated with Claude Code