feat(teamplay): fetch-intent sub() re-pulls fresh server state (read-after-write for fetchOnly + mixed fetch/subscribe)#43
Open
cray0000 wants to merge 3 commits into
Conversation
… read-after-write A fetch query is a one-shot point-in-time read (ShareDB self-destroys it once results arrive), but sub() treated it like a persistent transport and deduped repeat calls to a stale snapshot. So a fetchOnly root never reflected an awaited write, and the fetchOnly server default was effectively unusable (subscribe queries lag an awaited write by a pubsub round-trip, since query membership is server-authoritative). Now sub() in fetch mode never dedups: every call re-pulls via Query._refetch, which runs a fresh createFetchQuery and replaces $queries.<hash> in place (no detach, so reactive readers never see an empty "blink"). This gives fetchOnly roots read-after-write: an awaited write is reflected by the next sub(). Also propagate initConnection's fetchOnly to the auto-created global root, which froze the pre-init default at import time and so never became fetchOnly on the server. - Query._refetch + overridable _swapRefetchedDocs (Aggregation overrides it to a no-op: its rows are projected `extra`, not subscribed docs) - reconcileTransport re-runs if a sub/unsub/refetch lands mid-transition - tests: query and aggregation re-fetch on a fetchOnly root reflect awaited writes with a single subscription (same signal, no duplicate)
…ubscribe query (mixed) Extends fetch-intent read-after-write to the case where the query is ALSO live-subscribed by another owner. A separate fetch query can't safely rewrite a subscribe query's membership (they share singleton Docs — a fetch refreshes doc DATA, but the subscribe query's id list is maintained by index-based diffs against its own lagging baseline, so overwriting it corrupts the next diff). Instead, a fetch-intent sub() on a live subscribe transport overlap-resubscribes it: stand up a fresh createSubscribeQuery (current server membership), silence the old query's handlers, swap, replace $queries.<hash> in place, then destroy the old query. One live transport throughout, reflects the awaited write, no blink. The state machine is unchanged — "mixed" is a refresh of the single transport, not two coexisting transports. Live handlers were factored out of _initData into _attachLiveHandlers / _detachLiveHandlers so the old query can be silenced before teardown. - tests: mode:'fetch' on a live subscribe (fresh read, same signal, still live); concurrent subscribe + mode:'fetch' settle to one live subscribe transport
…review A concurrent fetch-intent sub() that landed in the microtask gap between the reconcile loop's final while-check and the phase->stable flip (the flip lived in the outer finally, a microtask after `await next`) had its bumped refetchEpoch dropped, resolving a stale snapshot — breaking read-after-write for a sub() issued after an awaited write when the burst also CREATES the transport (deterministic; regression test added). Fix: flip phase->stable in the SAME synchronous tick as the loop's exit check (inside the reconcile closure, not the outer finally), and drive the loop off the epoch/mode pending-work condition rather than a standalone boolean — which also neutralizes a captured-then-recreated entry. Also addressing the architecture review: - GC-based cleanup tests for the fetch/mixed paths (no explicit unsub() — cleanup via the FinalizationRegistry destructor on GC): fetchOnly query, repeated re-fetch, and mixed overlap-resubscribe each leave no transport or dangling ShareDB query. - pin that documents are NOT re-fetched (the singleton Doc already gives local read-after-write) and scope read-after-write to queries/aggregations in the docs - note the global-root fetchOnly carve-out in the rootFetchOnly contract comment - document the mixed-refresh bystander + coalescing behavior (pre-commit hook bypassed: a pre-existing, unrelated 2000ms-timeout flake in babel-plugin-teamplay, reproducible on the parent commit without these changes; the full teamplay server/compat/client/type suites + eslint pass.)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
A
fetchquery is a one-shot, point-in-time read — ShareDB self-destroys it (_handleFetch→_destroyQuery) the moment results arrive. Butsub()treated a fetch as a persistent transport and deduped repeat calls to a cached snapshot. And a live subscribe query's membership is server-authoritative, arriving a pubsub round-trip after a write's op-ack — so it momentarily lags an awaited write. Net result: no reliable read-after-write for queries, and the intended fetchOnly server default was unusable.Change
1. Fetch-intent
sub()always re-pulls fresh server state (never dedups to a stale snapshot):createFetchQuery, replacing$queries.<hash>in place viaQuery._refetch— no detach, no reactive "blink". Gives a fetchOnly root read-after-write.mode:'fetch'on a query someone else live-subscribed): a separate fetch can't safely rewrite a subscribe query's membership (they share singletonDocs — a fetch refreshes doc data, but the id list is maintained by index-based diffs against the subscribe query's own lagging baseline, so overwriting corrupts the next diff). InsteadQuery._refreshSubscribeoverlap-resubscribes: freshcreateSubscribeQuery, silence old handlers, swap, replace in place, destroy old. One live transport, reflects the write, no blink, stays live.2.
initConnection({ fetchOnly })propagates to the auto-created global root (froze the pre-init default at import time).3. The reconcile state machine stays single-transport — "mixed" is a refresh of the one transport, not two coexisting transports.
reconcileTransportre-runs until settled (epoch/mode driven) and flipsphase->stablein the same synchronous tick as its loop-exit check.Aggregations reuse the machinery and override
_swapRefetchedDocs(rows are projectedextra). Read-after-write is queries/aggregations only — documents are not re-fetched (the singletonDocalready gives local read-after-write; pinned by a test).forceflag was considered and rejected in favor ofmode:'fetch'as the intent;mode:'fetch'is deliberately not plumbed throughuseSub.Review (3 architects, scientific debate) — addressed
A 3-architect review (correctness / performance / design) ran on this PR. Acted on in-PR:
sub()landing in the microtask gap between the reconcile loop's final while-check and the oldfinally-basedphase->stableflip had its bumpedrefetchEpochdropped → stale read when the burst also creates the transport. Fixed (flip phase in the same sync tick as the loop exit + epoch/mode-driven loop, which also neutralizes a captured-then-recreated entry). Regression test added (fails before, passes after).unsub): cleanup runs via the FinalizationRegistry destructor. Added tests — fetchOnly query, repeated re-fetch, and mixed overlap-resubscribe each leave no transport or dangling ShareDB query.fetchOnlycarve-out noted in therootFetchOnlycontract comment; mixed-refresh bystander + coalescing behavior documented.Confirmed by the review (no change needed): per-process single-flight (concurrent identical reads coalesce, measured 50→1), no reactivity "blink" (whole-array replace goes through
setDiffDeep, 0 re-runs on unchanged), no retain/handler leaks across repeated refresh, error paths leave consistent state.Deferred follow-ups (not blockers; several gate the server-default flip, not this PR)
fetchOnlydefault totrue: validate cross-process DB-QPS (every awaitedsub()is a fresh query, no inter-process cache) and decide the cross-writer doc-staleness analog.mode:'fetch'-on-a-live-subscribe is a full re-subscribe (and refreshes co-subscribers of that hash) — consider a debounce/cap; add a bystander-across-distinct-roots test (verified correct, just untested).initConnectionglobal-root mutation → named helper + isolated plumbing test;SubscriptionStateonQueryis now vestigial (tech-debt cleanup).Tests
packages/teamplay/test/sub$.js+gcCleanup.js+rootFetchOnly.js. Full matrix green: server (489), compat-server (568), client (100), compat-client (117), type tests, eslint. (The repo's pre-commit hook also runsbabel-plugin-teamplay, which has a pre-existing unrelated 2000ms-timeout flake reproducible on the parent commit without these changes.)