[kixelated]

Problem

Cached-group joins and arrivals into unread backlogs sorted each subscriber's entire buffer after every mirror. Joins repeatedly sorted buffers of sizes 1 through the retained group count.

Finishes Cached JS group joins.

Approach

• Append in sequence order, otherwise binary-search the insertion point. Equal sequences stay in their previous stable-sort order. Joins, live arrivals, and re-offers keep one insertion path.

• Cover sparse reverse and mixed arrivals for a live subscriber and a late joiner. Sweep retained groups and backlog depth against subscribers in the replay benchmark, checking every delivered sequence and payload and recording sort work separately from timings.

• Keep replay and resume-history in the shared browser harness and smoke lane. Share trace parsing with the browser driver, use cross-origin isolated pages, and record an untimed traced pass separately from timed samples for replay and resume-history only. frames, drift, and ledger keep their existing warmup and measured passes.

• Merge current main without dropping benchmark suites or controls. PR 7521 landed during local review; the final merge preserves bandwidth, replay, and resume-history registrations, documentation, nightly selection, and the six-fixture browser smoke lane. It does not duplicate the bandwidth implementation. PR 7521 and the earlier main had the same resume-history fixture; its passing run did not exercise the tracing introduced here.

• Fix resume-history's traced-run false retention at its source: V8's synchronous gc() conservatively scans stack slots even from a fresh host task. Await asynchronous major collections, which run with StackState::kNoHeapPointers. Keep the collection count and every serving, concluded, and retired-route assertion. Save traced rows and check their retired counts too. See V8's GC extension.

• Isolate each replay shape on its own frozen clock, then close its readers and track and advance past cache expiry outside timing. This releases deferred callbacks and prune timers before the next shape. The original whole-run GC totals are withdrawn.

Impact

• Public API: none.
• Wire: none.
• Benchmark JSON additionally records traced rows and renderer GC summaries. Timing remains observational.

Alternatives

Batching and sorting once per join would leave a separate path from live insertion. Ordered insertion handles both, retaining the append fast path. Increasing GC passes or retrying a failed control would leave conservative stack roots in place; asynchronous collection removes those roots.

Validation

• Reproduced the traced resume-history failure before the collector change. Heap snapshots collected the reported routes, and V8's source confirms the synchronous/asynchronous stack-state difference.
• Chromium 156.0.8078.4: frames (26 rows), drift (36), ledger (12), bandwidth (44), replay (72), and resume-history (72), one warmup and one measured round for current and identical A/A bundles. Replay and resume-history also pass their traced runs. All passed. Final replay and resume-history artifacts retain all timed and traced rows; every retired-route count is zero.
• Deliberately retaining a route in scratch probes fails the concluded control. Retaining it only in the traced pass, then releasing it at conclusion, fails the retired-route check after saving its rows. Neither probe is committed.
• Final gate: 2,697 net tests and 10 browser-harness tests passed. Both benchmark TypeScript checks and quest check --base origin/main --branch quest/m2/performance-survey/js-cached-groups passed.
• Full gate: passed on final merge head 0c48f1dc2c2aecac13a7df292460ee51fbe714e5 (exit 0), including the PR 7521 integration. nix develop --command python3 .tmp/run-ci.py runs quest gate just ci after removing BASH_ENV and the mbx/mise proxy directories from the process PATH. It changes no repository or host configuration.

The corrected fixture was rerun with three alternating rounds against main f882be3ded89c8861bdc5f643f49128571a2cafd, plus current and identical A/A bundles, in Chromium 156.0.8078.4. All rows still verify every delivered payload and sequence. Raw timing and trace rows remain in .tmp/replay-paired.json. All 36 timing-shape medians improved; current and A/A counted shapes all report 0 sorts, sorted elements, and comparator visits. A reduced weak-reference probe retains 12 completed tracks with the old fixture and zero with the repaired fixture after warmup, measurement, and tracing. The host was shared and A/A samples show substantial scheduling noise; the deterministic counted pass remains the complexity evidence. Original GC totals from the fixture that retained completed shapes are withdrawn.

Choices

• Preserve ordered insertion and all benchmark suites and controls as requested.
• Expire each replay shape outside timing and limit GC tracing to replay and resume-history, fixing local review findings without adding a new option.
• Require Chromium's asynchronous major GC explicitly, with an error if it is unavailable. No fallback, retries, sleeps, or relaxed assertions.
• Store traced rows separately so their timings do not enter the measured rounds.

Follow-ups

• Foreground /quest-plan: the unchanged announcement test a version without a boundary syncs once the request is out asserts synced after yieldNow(). It failed in a full gate here, and untouched main reproduced 12 failures in 100 diagnostic runs. Wait on the actual request/sync event rather than treating one task yield as completion. No existing open PR was found for this exact failure. Subscriber production code and this test have no diff in this PR.
• Existing suggestions: measure backlog-drain front-splice cost and remaining per-mirror join allocation before changing either.
• The host's stale Codex BASH_ENV restores an oversized PATH and triggers mbx E2BIG. Gate commands below clear that hook and resolve Nix Cargo directly; moq-dev/moq#7594 already owns shell PATH deduplication. No host configuration is changed here.

Review

• Claude Code via quest review, claude-opus-5-5, medium effort, session c5c5ec84-194b-4270-a6df-958247636b21.
• Round 3 reviewed final head 0c48f1dc2c2aecac13a7df292460ee51fbe714e5 against main dde50c699f69507ced243754c2f700a6184d1576: no findings. Round 2 also reported no findings before the mechanical integration of PR 7521.
• Round 1: fixed replay tracks retained through the frozen clock, verified by weak-reference probes and fresh paired measurements; limited unused tracing to the two fixtures that consume it. Refuted the request to sort the replay cache per join: the quest explicitly forbids unnecessary whole-backlog sorting, and the deterministic test requires no sort. Ordered insertion preserves the requested semantics; reverse/mixed splice work remains measured rather than claimed linear.
• Existing online wording finding was fixed and resolved before this iteration. No online review was requested or awaited.

(Written by GPT-6.1 Sol)

[kixelated]

Automated review by review (OpenAI)

Reviewed commit: b8eeb32.

No actionable bugs found. The direction is sound: a small append-fast-path insertion helper removes repeated whole-buffer sorting while retaining the existing mirror, replay and eviction paths. Keeping buffer insertion separate from timeline insertion is justified because their equal-sequence behavior differs; batching replay separately would add a second path without fixing live backlog arrivals. No public API or wire change identified.

Checked the insertion/search and delivery context, sparse-arrival regression test, replay benchmark, shared trace extraction and nightly/browser wiring. Verification was static through GitHub only; I did not run tests, browser benchmarks or independently reproduce the reported performance/GC measurements.

(Written by OpenAI)

[coderabbitai]

No actionable comments were generated in the recent review.

Recent review info

Run configuration

• Source mode: Auto
• Repository: Unavailable
• Branch: Unavailable
• File: No configuration file from this repository applied
• Commit: Unavailable
• Layers: Organization UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: 9086449a-d0b6-429b-93c1-30111465c80d

Commits

Reviewing files that changed from the base of the PR and between 17e00c8 and b8eeb32.

Files selected for processing: 12

• bench/README.md
• bench/browser/driver.ts
• bench/browser/micro.ts
• bench/browser/run.sh
• bench/browser/trace.ts
• js/justfile
• js/net/bench/replay.ts
• js/net/src/track.test.ts
• js/net/src/track.ts
• quest/m2/performance-survey/README.md
• quest/m2/performance-survey/js-cached-groups.md
• sh/gh/nightly.sh

Files with no reviewable changes: 1

• quest/m2/performance-survey/js-cached-groups.md

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 0 remain after this review.

Walkthrough

Mirrored groups are now inserted into unread buffers in sequence order, with a test for sparse, out-of-order arrivals. A replay benchmark measures joins and unread-backlog arrivals. Browser benchmark tooling adds replay support, cross-origin isolation checks, and renderer GC data. Benchmark documentation, nightly coverage, and the performance survey are updated.

Merge Risk: Minimal · up to b8eeb

The new replay benchmark has a working browser entrypoint and nightly reporting path. No issue requiring a fix before merge was identified.

Pre-merge checks: 4 · 1

Failed checks: 1 warning

Check name	Status	Explanation	Resolution
Docstring Coverage		Docstring coverage is 77.78% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documenta…	Write docstrings for the functions missing them to satisfy the coverage threshold.

Passed checks: 4 passed

Check name	Status	Explanation
Title check		The title clearly and concisely describes the main change: replacing re-sorting with ordered insertion for replayed JavaScript groups.
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.
Description check		The description directly explains the ordered-insertion change, replay and browser benchmark updates, tracing work, validation results, and their impact. It is clearly related to the changeset.

Full details: Docstring Coverage

Explanation

Docstring coverage is 77.78% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 9 functions across 8 files. (3 skipped: 3 unsupported.)

Resolution

Write docstrings for the functions missing them to satisfy the coverage threshold.

Finishing Touches

Simplify code

• Commit to this branch
• Create a new PR

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

Share

• X
• Mastodon
• Reddit
• LinkedIn

[pullfrog]

ℹ️ No critical issues — one doc-wording suggestion inline.

Reviewed changes

• js/net/src/track.ts: #mirror now inserts a replayed or live group with bufferInsert (append when the buffer's last sequence is <= the new one, else a lower-bound splice that lands after any equal sequence) instead of push + stable sort, removing an O(n log n) sort per arrival.
• js/net/src/track.test.ts: a deterministic test drives sparse reverse and mixed arrivals into a live backlog and a late joiner, asserting ascending delivery with the right payloads and that Array.prototype.sort is never called. It fails on main.
• js/net/bench/replay.ts plus bench/browser/*: a new replay benchmark (retained groups × joining subscribers, and unread backlog × active subscribers, across monotonic/reverse/mixed), trace parsing shared into trace.ts, cross-origin isolation for finer performance.now(), and an untimed GC-traced pass.
• Nightly/quest wiring: replay added to the JS nightly leg and the browser smoke loop; the child quest is closed and its result recorded.

I verified the ordering invariant holds across every mutation of TrackState.groups in track.ts — all insertions route through bufferInsert, the rest are removals or a clear on close, so the append fast path never sees an unsorted buffer, equal-sequence placement matches the old stable sort, and the consumers that assume sorted order (#tryRecvGroup, #tryNextBuffered, #peekGroup, #bufferedLargest) are unaffected. I also confirmed the benchmark prints exactly the 72 data rows ROWS.replay expects, that bench-report.py skips the counts table because its columns are not measurement units, and that the append shape's two publish sets partition 0..total-1 so the drain check passes for all three orders.

  ^{｜ Fix all ➔ ｜ Fix 👍s ➔ ｜ View workflow run ｜ Using deepseek-v4.1-flash (free via Pullfrog for OSS) ｜ 𝕏}

[moq-bot]

codex / Codex land fixer note:

The batch failure log, line 1, says:

Conflicts with the base: bench/README.md, bench/browser/micro.ts, bench/browser/run.sh

Merged base and resolved these conflicts in commit ac4fded4ee8df35a262cdfb328e5cfe4f33ce1ac on pr-7516, retaining replay and resume-history support, GC tracing, and retention checks. No protected paths were manually edited.

Validation passed: bun run --cwd js/net build, bun run --cwd js/net check, bun test --cwd js/net --only-failures (2684 tests), bun run --cwd bench/browser check, bun test --cwd bench/browser --only-failures (10 tests), bun js/net/bench/replay.ts, Biome on changed TypeScript files, shell syntax checks, and python3 sh/gh/nightly.test.py (14 tests). Chromium microbenchmark runs for frames, drift, ledger, and replay passed with one measured round and both current and aa variants, including traces.

The resume-history command below failed twice during the final traced aa run, after both measured variants and the current trace had passed:

BROWSER_BIN=/nix/store/1sda328rshrqarp3jsz1rd1h3bjj3fy7-chromium-155.0.8059.84/bin/chromium bun bench/browser/micro.ts --bench resume-history --rounds 1 --out /tmp/quest-resume-history-retry.json current=. aa=.

First failure:

error: evaluate: Error: 8 fronts, 1 names, 32 routes, churn, subscribe: the control kept {"sessions":32,"transports":32,"broadcasts":256,"bytes":12297} after concluding

Retry:

error: evaluate: Error: 8 fronts, 8 names, 2 routes, churn, fetch: the control kept {"sessions":1,"transports":1,"broadcasts":8,"bytes":4099} after concluding

The merge conflicts are mechanically resolved, but validation remains failing. Review the interaction between reachability controls and GC tracing before changing either behavior. No checks were disabled.

(written by codex / Codex)

[moq-bot]

codex / Codex land fixer: After mechanical conflict resolution, `BROWSER_BIN=/nix/store/1sda328rshrqarp3jsz1rd1h3bjj3fy7-chromium-155.0.8059.84/bin/chromium bun bench/browser/micro.ts --bench resume-history --rounds 1 --out /t. Recommendation: Review resume-history reachability controls under GC tracing and re·solve the repeated validation failure before requeueing; the mechanical base merge is committed on pr-7516.

Conflicts with the base: bench/README.md, bench/browser/micro.ts, bench/browser/run.sh

(written by codex / Codex)

[kixelated]

Automated review by review (OpenAI)

Reviewed commit: 0c48f1d.

No actionable bugs found in this update. Compared with my previous review at b8eeb32, separating upstream merges from the benchmark changes, I checked per-shape replay teardown and clock advancement, asynchronous major collection, saved traced rows and their retention checks, and preservation of the six browser fixtures and nightly registrations.

The direction is sound: expiring each shape outside timing addresses retained prune callbacks rather than masking them with more GC; asynchronous collection retains the serving/concluded controls rather than weakening assertions. Keeping traced samples separate avoids contaminating timing rounds, and restricting tracing to its two consumers avoids unnecessary work. These focused harness corrections justify the added complexity without changing public API or wire behavior.

My previous review had no open findings. The independent documentation wording suggestion is also fixed; no duplicate inline comment is needed.

Verification was static through GitHub only. I did not run tests or browser benchmarks, reproduce the GC probes, or independently verify the reported timing improvements.

(Written by OpenAI)

[pullfrog]

✅ No new issues found.

Reviewed changes

• Gave each replay shape its own Manual clock and advanced it past cache expiry after teardown, outside timing, so completed shapes cannot accumulate through prune timers.
• Switched resume-history's reachability collection to Chromium's asynchronous major GC (StackState::kNoHeapPointers), dropped the task yield, and fail loudly when async GC is unavailable.
• Recorded traced rows and per-renderer GC summaries in the browser benchmark JSON, restricted tracing to replay and resume-history, and extended the retired-route control to traced rows.
• Merged current main's bandwidth and resume-history benchmark registrations, keeping all six smoke fixtures, nightly selections, and documentation.

The production ordering change in js/net/src/track.ts is untouched since the prior review. I verified the async-GC contract against V8's gc-extension.cc (execution: "async" returns a Promise and collects with kNoHeapPointers), the per-shape Manual.install/close lifetimes against js/clock, the replay/resume-history row counts against ROWS (72 each, bandwidth 44), and that the retired-route slice (slice(5, 9)) still lines up with the resume-history header. Public API and wire impact remain none.

  ^{｜ View workflow run ｜ Using deepseek-v4.1-flash (free via Pullfrog for OSS) ｜ 𝕏}