Repository navigation
docs(quest): stats per-broadcast tracks become on-demand prefix tracks - #4870
Conversation
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Warning Review limit reachedYou'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. View limit detailsLimit details: You’ve used all 4 included reviews currently available. Review configuration: ⚙️ Run configuration
📒 Files selected for processing (2)
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. Comment |
kixelated
left a comment
There was a problem hiding this comment.
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)
|
Grok review of I checked the code references against Blocking
Non-blocking
Cross-PR: #4871 also edits 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 |
…d merge with per-upstream baselines Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
kixelated
left a comment
There was a problem hiding this comment.
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)
|
Grok follow-up review of The push is a main merge (
Non-blocking
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 |
Merge summaryThis amends
Reviews: I addressed Grok's review of 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) |
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".Path::MAX_PARTSsegments. The relay's--stats-depthonly decides where group broadcasts are announced and doesn't limit how deep a request can go.@<epoch>children, and a channel prefix maps its broadcasts.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. Genericmoq-statsusers get arbitrary-depth rollups too.Decision trail (2026-10-05, ✅ = chosen)
Group header rates after the split
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
/*Plan-only. Implementation follows stats-split.
(Written by Claude Opus 5.5)
🤖 Generated with Claude Code