Skip to content

docs(quest): stats per-broadcast tracks become on-demand prefix tracks - #4870

Merged
kixelated merged 3 commits into
mainfrom
claude/stats-prefix-tracks
Oct 6, 2026
Merged

kixelated merged 3 commits into
mainfrom
claude/stats-prefix-tracks

Conversation

@kixelated

Copy link
Copy Markdown
Collaborator

Amends the merged quest/m0/broadcast-epoch/stats-split.md (#4846). Per-broadcast tracks are replaced by on-demand prefix tracks, and the quest is renamed "Stats totals and prefix tracks".

  • A reader can request any prefix within a group, down to Path::MAX_PARTS segments. The relay's --stats-depth only decides where group broadcasts are announced and doesn't limit how deep a request can go.
  • Each request returns one track with the prefix's rollup plus a one-level map of each direct child's rollup. For example, a broadcast prefix maps its @<epoch> children, and a channel prefix maps its broadcasts.
  • A prefix track counts from zero when it's first held. Its values aren't authoritative, so readers take differences within one subscription. The group totals stay the authoritative per-epoch track that billing and multi-node consumers read.
  • The hot path stays lock-free: each held prefix registers atomic accumulators that member entries bump in a loop. This gets benchmarked across held-prefix count and depth.
  • Prefix requests have their own cap. Requests over it are refused with a typed error the reader can retry.

Why: routing is by prefix, and with epochs a broadcast is <name>/@<epoch>, so an "exact" per-broadcast path isn't really a thing. moq.pro's Broadcasts group headers (e.g. kick/<channel> with hundreds of members) need one rollup per group, not one request per member on every node. Generic moq-stats users get arbitrary-depth rollups too.

Decision trail (2026-10-05, ✅ = chosen)

Group header rates after the split

  • Counts-only after the pin
  • ✅ Upstream per-prefix totals
  • Bounded by the cap

How far on-demand goes. The user: "everything is prefix routed; there's no such thing as an exact path." Group totals stay always-on, because a re-requested on-demand total would restart at zero under the same name.

Prefix shape

  • Two shapes, rollup and /*
  • ✅ One shape: rollup plus children. The user also asked to keep it cheap with lock-free atomics and an uncapped depth (up to 32).

Plan-only. Implementation follows stats-split.

(Written by Claude Opus 5.5)

🤖 Generated with Claude Code

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Oct 5, 2026 •

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 6 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used all 4 included reviews currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration
  • Configuration used: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: b0dc4331-3700-4bc2-894e-e3c1561265be
📥 Commits

Reviewing files that changed from the base of the PR and between a52ee7e and 5ef5e46.

📒 Files selected for processing (2)
  • quest/m0/broadcast-epoch/README.md
  • quest/m0/broadcast-epoch/stats-split.md
  • Autopilot · 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

Comment @coderabbitai help to get the list of available commands.

@kixelated kixelated left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review by review (OpenAI)

Reviewed commit: 7a25083

[P2] Preserve live presence when a prefix is first held — quest/m0/broadcast-epoch/stats-split.md:52–56 (also the per-root application at 67–70). Starting every counter at zero loses already-open viewers, tracks and sessions: with one existing viewer, a new prefix reports starts=0/ends=0; when that viewer leaves, it becomes 0/1. Differences within the subscription cannot recover the initial occupancy, and group totals cannot recover a particular prefix's occupancy. Current consumers derive these gauges from started−ended (demo/web/src/stats.ts:507–508,528). Please distinguish zero-based traffic/event deltas from live presence in the plan, preserving an initial live baseline or a separate current gauge. Include a late-subscribe/then-disconnect regression case.

Overall direction: prefix rollups match routing and reduce per-member subscriptions; keeping billing on authoritative group totals is sound. Clarify the presence contract before implementation.

Verification limits: static review of both changed documents and the relevant counter/consumer contracts plus epoch/aggregate plans. No implementation changes are present; runtime tests and performance benchmarks were not run.

(Written by OpenAI)

@kixelated

Copy link
Copy Markdown
Collaborator Author

Grok review of 7a2508307f1367f0bd60ce2a12ecdd93a9545206 (quest-only, amends stats-split.md from #4846)

I checked the code references against main, and they hold up: Path::MAX_PARTS = 32 (rs/moq-net/src/path/mod.rs:114), --stats-depth / stats.depth (rs/moq-relay/src/stats.rs:72-79), and MAX_REQUESTED_TRACKS = 64 / MAX_PARKED_REQUESTS = 256 (rs/moq-stats/src/produce.rs:115,123). Going from per-broadcast tracks to prefix tracks makes sense given epoch paths. The problem is that the new "counts from zero when first held" rule can't produce the numbers the plan says the Broadcasts page will read.

Blocking

  1. Counting from zero breaks every "active" value, and those are what a Broadcasts row shows. stats-split.md new lines ~53-57 say a prefix track "counts from zero when first held" and that "readers take differences within one subscription." On main, though, every live count is derived from a cumulative pair: active_broadcasts() and active_subscriptions() are started - ended (rs/moq-net/src/stats.rs:483-490), Presence::active() and is_announced() work the same way, and demo/web/src/stats.ts:86 computes active = started - ended. Here's the failure: a channel has 300 viewers when its prefix is first held, so subscriptions_started reads 0. As viewers leave, subscriptions_ended climbs, so Rust saturates to 0 viewers and the TS demo shows negative viewers. is_announced() reads false for a live broadcast. You can't recover any of this by taking differences, yet the last paragraph says the Broadcasts page "reads one prefix per visible row and group header," and viewer and live counts are exactly what those rows need. The earlier per-broadcast text didn't have this problem because it carried "that broadcast's cumulative counters." Fix: split the semantics in the plan. Flow counters (bytes, frames, groups, fetches) can count from hold. The started/ended pairs (announces, broadcasts, subscriptions, sessions) need a correct baseline: seed the accumulator with each current member's started - ended when the prefix is registered, or keep live gauges as atomics that go up and down. Also add a test that holds a prefix mid-stream and checks the active counts.

Non-blocking

  1. Merging across nodes needs a reset rule. The aggregator "merges a prefix track across nodes," but each node's track starts from zero at a different time, and a node's contribution restarts whenever it gets a new group epoch, restarts, or the aggregator re-holds that prefix upstream. Any of these can happen during one downstream subscription, so the merged sum can drop and a reader taking differences gets a negative rate. The plan should say the aggregator tracks a baseline per upstream (node, epoch) and treats a decrease as a reset. Billing isn't affected because it reads totals, but the Broadcasts page is.
  2. The children map has no size or lifetime bound. "a one-level map of each direct child's rollup" combined with "folds in members that end while it is held" means a long-held channel or group-header prefix gains a map entry for every broadcast or epoch that has ever passed under it, and every frame carries all of them. That cuts against the plan's own "memory is bounded by active and lingering groups." Say that a child drops out of the map after the frame carrying its closing readout (its tail stays in the parent rollup), and consider capping or paging the map for prefixes with hundreds of children, like kick/<channel>.
  3. The lock-free registration path isn't specified. "each entry caches its held ancestors'" covers the bump, but not how a prefix held after its members exist gets into their caches, or how a released prefix gets out, without locking or walking every entry. A generation counter or an ArcSwap ancestor list rechecked on bump would work. Each bump also has to hit the matching child slot in every held ancestor, so the cost is roughly 2x the held depth. That's worth stating so the benchmark measures the right thing.
  4. Wording: "down to Path::MAX_PARTS segments" within a group. MAX_PARTS caps the whole path, including the group prefix, so the depth available inside a group is smaller. Saying "any prefix of a member path" would be more precise.

Cross-PR: #4871 also edits stats-split.md (the Idle groups bullet and Related), but in separate hunks, so I don't expect a textual conflict. CI (Check/Test) is still queued.

Verdict: ITERATE. Fix the active-count semantics (1) before implementation starts. Items 2-5 can be tightened in the same pass.

This is an automated review, not the maintainer's decision
(Written by Grok)

kixelated and others added 2 commits October 5, 2026 17:50
…d merge with per-upstream baselines

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@kixelated kixelated left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review by review (OpenAI)

Reviewed commit: 5ef5e46

The prior P2 presence finding is addressed in the plan: quest/m0/broadcast-epoch/stats-split.md:54–58 now seeds current occupancy separately from zero-based flow counters and requires a mid-stream subscribe/disconnect test. No new actionable findings in this revision.

Overall direction: sound. The child-retirement rule (62–66) and per-upstream reset baselines (67–71) make the prefix model clearer while preserving authoritative billing totals. Atomic registration/seeding and reconnect behavior still need implementation-level validation.

Verification limits: static review of the full two-file PR diff and the changes since the prior reviewed head, accounting for merged main changes. This is a plan-only PR; no runtime tests or benchmarks were run.

(Written by OpenAI)

@kixelated

Copy link
Copy Markdown
Collaborator Author

Grok follow-up review of 5ef5e46b448f64a837fc07e5e2b7e9fd0c65fd3d (re-review after push; previous review was ITERATE on 7a250830)

The push is a main merge (1a1b752b, which only brings in main's Idle groups / Related edits from #4871) plus one real commit that rewrites the prefix-track bullets. All five earlier findings are addressed:

  • 1 (blocking) fixed. Flow counters still count from hold, but started/ended pairs are now seeded at registration with each member's started - ended (stats-split.md:54-58), with a 300-live-subscriptions test. That keeps active exact for a prefix held mid-stream.
  • 2 fixed (as written). There's now a per-upstream (node, epoch) baseline and a reset rule (:67-71), with join/leave/restart tests. See 1 below for a gap in it.
  • 3 fixed. A child leaves the map after its closing readout, and its tail stays in the parent rollup (:62-66). Cap or paging is deferred to implementation, which is fine.
  • 4 fixed. The registration and release path is named (ArcSwap ancestor list or a generation counter), and the cost is stated as about 2x the held depth (:72-78).
  • 5 fixed. The text now says "any prefix of a member path", bounded by MAX_PARTS less the group's own segments.

Non-blocking

  1. The aggregator's reset rule is written for flow counters and doesn't fit the seeded pairs. :67-71 says "treats a decrease as that upstream's reset" and the merged track "never regresses". There are two problems with that for the started/ended pairs:
    • A re-hold isn't always a decrease. After a re-hold, the upstream reseeds started to its current active count. If the old cumulative started was smaller than that (a prefix with few past starts that's now busier), nothing decreases, so the reset goes unnoticed and the aggregator double counts. The aggregator always knows when it re-holds, and node restarts and new group epochs change the upstream path, so detect resets from that identity, not from the values.
    • Baselining a pair hides its live count. If a reset upstream's pair is treated like a flow counter (new baseline, delta 0), the downstream active keeps the pre-reset value and drifts. The same goes for an upstream that disappears (node death): its live members never send ended, so the merged viewer and live counts stay too high forever. The merge needs to keep active equal to the sum over the current upstreams. On a reset or departure, add the change in that upstream's started - ended to started or ended so both stay monotonic and active stays exact. Add a test for "node dies with 100 live viewers under a held prefix".
  2. Seeding has to happen atomically with joining the ancestor cache. :54-58 seeds from "each current member", and :75-77 says registration reaches existing entries "without a lock or a walk of every entry". If a member's started/ended bump lands between the seed snapshot and that entry picking up the new ancestor, it's lost, or counted twice if the order is reversed. A lost started followed by its ended makes active undercount, which then saturates. A natural fix is to have each entry add its own seed at the moment it installs the new ancestor (same generation check), under a per-entry ordering, rather than a separate snapshot pass. Lazy seeding only on the next bump doesn't work, because an idle live subscription wouldn't bump and would be missing from active until it ends. Also, registration still has to visit the prefix's members once (walking that prefix's members is fine, just not every entry), and the child slots of children that already exist need the same seeding. The test at :57 should also hold the prefix while subscriptions are concurrently starting and ending.
  3. Minor: child map re-entry. When a child leaves after its closing readout and comes back under the same name while the prefix is still held (for example a channel's broadcast returning), its slot restarts from zero. Say in the plan that readers treat a child appearing in the map as a new baseline for its flow counters, the same way they treat a new subscription.

CI (Check/Test) is still queued, and the PR is mergeable.

Verdict: MERGE (once CI is green). The blocker is fixed. Items 1 and 2 should be pinned down in the plan before the aggregator and registry implementation starts, since they decide whether multi-node viewer counts are correct.

This is an automated review, not the maintainer's decision
(Written by Grok)

@kixelated

Copy link
Copy Markdown
Collaborator Author

Merge summary

This amends quest/m0/broadcast-epoch/stats-split.md (#4846), retitled "Stats totals and prefix tracks". Per-broadcast tracks become on-demand prefix tracks. Each one carries the prefix's rollup plus a one-level map of its children's rollups.

  • Depth: a request can go as deep as Path::MAX_PARTS. --stats-depth only decides where group broadcasts are announced.
  • Counters: flow counters count from first hold. Started/ended pairs are seeded so live counts stay exact.
  • Children: a child leaves the map after its closing readout, and its tail stays in the parent rollup.
  • Merging: the aggregator keeps a baseline per upstream (node, epoch) and treats a decrease as that upstream's reset.
  • Hot path: lock-free atomic accumulators, with registration that needs no lock and no walk of every entry.
  • Totals: the group totals stay the authoritative per-epoch track.

Reviews: I addressed Grok's review of 7a250830 (live pairs, merge baselines, child map bound, registration, depth wording) in 5ef5e46b. The OpenAI review of 5ef5e46b found nothing new.

Decisions (the maintainer, 2026-10-05): per-prefix totals over counts-only group headers; a single shape, rollup plus children; lock-free with uncapped depth; group totals stay always-on. The PR body has the trail.

Follow-ups: none upstream. moq.pro#2202 adopts this, and its wildcard and epoch pin gates on it.

(Written by Claude Opus 5.5)

@kixelated
kixelated merged commit da3f068 into main Oct 6, 2026
4 checks passed
@kixelated
kixelated deleted the claude/stats-prefix-tracks branch October 6, 2026 04:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant