PR #1909 external review#

Review target: 0ba54cd50bca908348940eb2020ec4f244450dc9. The supplied medium-effort review named four findings, one test gap, and one broader cross-PR observation. This record separates the code fact from the proposed behavioral consequence.

The review identified a real test gap and a redundant guard. Its proposed restriction of the joined-demand gate would introduce a visible anti-join bug. The Effect classifier and split demand bookkeeping remain evidence gaps rather than confirmed product failures.

Follow-up probes#

For R2, a temporary test compared classifyImmediateDelta and classifyHeldDelta at every ordinary ordered Effect flush and failed if their event types differed. It ran 64 deterministic histories of 32 valid source mutations each. The histories crossed insertion, deletion, replacement, filter membership, order changes, and top-two displacement over five keys. After every mutation, the Effect's accumulated public rows matched an independent filter/sort/window recomputation. No classifier divergence occurred. The instrumented ordered-work, Effect, Effect-disposal, and pagination owners also passed 426 tests without a divergent event type. The temporary instrumentation was removed. These are sampled histories, not a proof for every query shape.

For R3, a temporary fixture gave one on-demand root and child source to a live-query Collection and an Effect. It held the child's request while both consumers evaluated a custom-collation none filter. Both stayed empty before release and ended with only the unmatched root. The callback sequence was Collection, Collection, Effect; each callback's own result was correct. A read of both consumers during the first Collection callback saw the Effect's earlier state. Their callback schedules are separate, so that observation alone does not show that either pending-work definition released at an invalid point. The temporary fixture was removed; no product change was justified by it.

Follow-up review of demand scope and wake-up#

The later review was checked against 2a6b119f0dd85370d9f179b8a942fc163f08ea13. It contained two new product failures and several repeat or speculative claims.

The second review supplied the missing unrelated-demand and empty-replay histories. The alleged infinite loop did not survive the no-progress check.

Code-weight refactor#

The existing D2 graph and OrderedSourceLoader now drive ordered continuation on every Collection graph turn, matching the Effect runner's existing schedule. The Collection builder keeps one loader set per sync run and fences queued jobs by sync generation. The transaction scheduler already deduplicates and clears jobs, so the builder's second per-context callback registry and clear listener were removed. Collection and Effect also use one dependency-graph scheduling helper rather than separate transaction scheduling implementations. The source loader's fixed-point latches still bound requests when a graph turn receives no new rows. Effect's immediate and held delta classification now share one classifier; only ordered Effects pass callback-visible row state. A single monotonically increasing demand generation replaces per-plan generation bookkeeping.

Against merge base afbeb44eef48d92b6e30ae5fd2843b938ee9163c, the seven changed production TypeScript files contain 215 added and 331 deleted lines (net −116 raw lines). Counting nonblank, noncomment diff lines gives 197 added and 221 deleted (net −24 executable lines). This measure excludes architecture prose, tests, and review records. It is a line-count check, not a claim that each old state was redundant: the joined-source demand and public publication gates remain necessary for correctness.

The full packages/db/tests run passed 6,603 tests across 186 files after the production refactor. The ordered-work oracle passed 93 cells, including the unrelated-demand and empty-replay repairs. The scheduler test exercised 81 cases, including repeated-alias failure ownership across falsy failures.

Ordered continuation and window follow-up#

Code target: 0cbc7ae0 (production change 450b9d3c). The later CodeRabbit comment at https://github.com/TanStack/db/pull/1909#discussion_r4124008382 identified three additional failure mechanisms. Its suggestion to run the local CodeRabbit CLI is separate from those findings.

The follow-up also simplified Effect source ingestion and Collection graph scheduling. Collection now checks ordered loaders inside one fixed-point loop. A public-query oracle observes a synchronous root refill before its second joined demand. Deleting the post-loader graph step made that oracle fail at the second-demand assertion. This was an assertion failure, not a timeout or setup failure. The step remains in production. The field-lab loss audit records source-supported distinctions that the short refactor summary omitted. It does not treat those omissions as product regressions.

Against merge base afbeb44eef48d92b6e30ae5fd2843b938ee9163c, the seven production TypeScript files have 318 added and 431 deleted lines. Net production size is −113 raw lines. Excluding blank and comment-only diff lines gives 299 added and 303 deleted, or −4 executable lines. The DB oracle campaign passed 2,627 tests across 42 files. The DB runtime suite passed 6,609 tests across 186 files with Vitest typechecking disabled. Package TypeScript, changed-file ESLint, Prettier, and git diff --check passed.

Oracle guide review for this follow-up#

• ORC-001–003: The ordered-work oracle names the ordered-window and publication contract, uses an independent filter/sort/window reference, and keeps its grammar, driver, and public checkpoint visible. The loader-state case is a focused state refinement of explicit retry admission.
• ORC-004: This follow-up adds bounded deterministic cells and makes no new generated-grammar claim. The existing generated grammar is unchanged. The new cells reconstruct the named joined-demand, repair, window-move, failure, retirement, and refill witnesses. Multiple simultaneous joined plans and a public-query failed-retry overlap remain outside this coverage, as the coverage map states.
• ORC-005–006: The driver uses public live-query Collections and Effects. It checks rows, requests, window settlement, and callback batches at named checkpoints. The pre-fix runs failed at those observations. The graph-step mutant failed at a public second-demand assertion.
• ORC-007: The existing important generated property runs fixed and random campaigns with direct seed/path replay. These follow-up cells are bounded deterministic controls, so they add no separate generated campaign.
• ORC-008–009: The reference model gains no state or new vocabulary. Window participants in production remain distinct from reference rows.
• ORC-010: Each new async control resolves its held promises during cleanup. withHistoryCleanup retains the primary assertion and reports cleanup failures separately.
• ORC-011: No new shared semantic fault hypothesis requires a second formulation in this follow-up. The independent filter/sort/window reference remains the primary result model. Remote relation hints remain a separate feature.
• ORC-012: This versioned section records outcomes for ORC-001 through ORC-011 against the code target and names the remaining in-scope witnesses.

The repaired claim covers finite local joined-filter windows, the named joined-demand and window-move histories, Collection and Effect public rows, and the specified settlement checkpoints. It does not claim all legal demand interleavings. The coverage map owns the two remaining witnesses above.

Shipped-byte consolidation#

Code target: b69c189c. Three independent scouts inspected graph scheduling, ordered demand, Effect state, package exports, and dependency drag. They measured proposed cuts in isolated builds before the final edit. The chosen changes remove two unused scheduler extension paths, two redundant Effect fields, and unused Collection scheduler overrides. The core scheduler and D2 graph remain necessary for variable query dependencies and coherent publication.

Scheduler.onClear() had no production caller, and no production dependency implemented hasPendingGraphRun(). Scheduler is absent from the package root exports. The change retires those internal extension contracts. A direct unsupported source import can no longer attach a clear listener or provide an out-of-queue pending dependency. The scheduler test now checks queued-job cancellation, reuse of a cleared context, prerequisite requeue, exact failure identity, and later successful work. Existing Collection and Effect tests still check coherent public batches, rollback, stale settlement, and loader failure. The architecture now says that only queued jobs block scheduler dependencies. The second field-lab loss audit preserves source details omitted by this short reduction.

The byte assay built the same package path with Vite 7.3.2 --minify, then bundled selected ESM exports with esbuild 0.27.7. It used gzip level 9 and Brotli quality 11. Every row compares the identical absolute path and toolchain. Values are bytes; negative deltas save bytes.

The named query imports remain slightly larger than the merge base under some byte measures. The full import has fewer compressed bytes but 93 more minified bytes. These receipts do not establish a universal net-negative shipped size. @tanstack/pacer-lite contributes no bytes to the measured query imports, so the scouts found no query-path dependency drag to remove. The remaining optional scheduler dependency argument saved only 15 gzip bytes in a trial and would retire another internal test contract; it was left intact.

The DB oracle campaign passed 2,627 tests across 42 files with no type errors. The full DB runtime suite passed 6,605 tests across 186 files with Vitest typechecking disabled. Package TypeScript, changed-file ESLint, Prettier, and diff checks passed. Against the merge base, production TypeScript is −156 raw lines and −35 nonblank, noncomment lines. This count includes the scheduler file; it excludes tests and documentation.

Xhigh review of ordered callbacks and code weight#

External review target: 80421c8. Evaluation started at dd10cb4; the verified follow-up is b445bd13 (production fix 1a1b0952). The raw review named two behavior findings, one duplication suggestion, two lower-severity findings, two oracle gaps, and broader preservation/weight claims. Its reported fresh-worktree and architecture-reading checks are provenance, not product evidence. The entries below retain those claims separately.

RED/GREEN and byte evidence#

Before the fix, the new public Effect witness received enter after the held child load resolved. With 1a1b0952, it received no initial callback and did receive a later update. The ready-work witness observed two root-ready graph schedules under a temporary broad-condition mutant; the source-specific guard reduced that to one while the joined-ready transition still scheduled its work. Both mutants failed assertions at the intended checkpoints. The reorder witness failed its proposed update expectation on both dd10cb4 and the reviewed Effect source from 80421c8; this is a refutation of the review's old-versus-new causal claim, not proof of every callback policy.

The final DB oracle campaign passed 2,629 tests across 42 files, with no Vitest type errors. The focused ordered-work, scheduler, and Effect suites passed 250 tests. Package TypeScript, changed-file ESLint, Prettier, and diff checks passed. The test-only cleanup follow-up retained primary assertion failures while releasing held work. The production diff from merge base afbeb44e remains −153 raw TypeScript lines.

In the Vite 7.3.2 and esbuild 0.27.7 same-path trial, the proposed shared helper cost 37 more minified and 20 more gzip bytes in the full index than the selected local reuse. It cost 108/25 more minified/gzip bytes for the Collection import and 38/17 more for the Effect import. It saved 108 Brotli bytes in the full index but added 15 and 9 Brotli bytes in the two named imports. The selected local reuse plus behavior fixes changed the measured consumer imports against dd10cb4 as follows:

These are incremental build differences from dd10cb4, not a claim that every PR entry point is net smaller than the merge base.

Oracle guide review#

• ORC-001–003: The architecture states the skipInitial boundary; the ordered-work oracle keeps the contract, plain-result model, scenario grammar, real Collection/Effect driver, and callback checkpoint visible. The work counter is a partial oracle for scheduling calls, not public row truth.
• ORC-004 and ORC-007: These are deterministic controls. The existing generated grammar and fixed/random campaigns are unchanged and both ran in the 2,629-test oracle campaign.
• ORC-005–006: childLoads > 0 proves the held joined source was reached. The recorded callback list can contain an illegal initial enter. The pre-fix run rejected that event; the broad-condition mutant failed the scheduler count after a real root ready transition.
• ORC-008–009: The reference gains no model state. The new checks use the glossary's Collection, joined demand, readiness, and publication terms.
• ORC-010: withHistoryCleanup releases the held promise and all Collections while preserving the primary failure and separate cleanup diagnostics.
• ORC-011: No plausible shared semantic fault between the existing plain row reference and the skipInitial callback law was named. The independent callback assertion uses the established skipInitial contract.
• ORC-012: This versioned section records the two RED controls, the GREEN run, the exact code target, the bounded claim, and the remaining cells. Multiple simultaneous joined plans during a window move and a public-query failed-retry overlap remain in the coverage map. Nonterminal per-demand recovery is a separate undefined contract.

Final loss audit and reviewer assessment#

The raw review was reread after the fix. Its 13 recorded items reconcile as 3 fixed-now + 4 already-fixed + 1 duplicate + 1 refuted + 3 deferred + 1 design-decision = 13. The only unresolved behavioral evidence gap is the earlier R2 ordinary-classifier concern, now linked from X7; the stated X4 reorder trace does not supply it. X11 remains a future recovery design issue.

The reviewer showed useful depth on X3 and X10, including a concrete async sequence and a work-specific source comparison. Accuracy and prioritization were mixed: X4's confident regression claim failed on its own old path, X9 overstated duplication and byte savings, and X11 correctly identified a latent state mismatch without showing a current product failure. The review has moderate signal-to-noise and good test-gap instincts. Hire for code review with an executable-verification requirement for blocking behavioral claims; do not use this review alone as a merge gate.