feat(moq-pub-mmtp): demand-driven SSM membership (subscribe-driven re-join) - #33
feat(moq-pub-mmtp): demand-driven SSM membership (subscribe-driven re-join)#33kkroo wants to merge 2 commits into
Conversation
…-join) The bridge held its SSM multicast membership for the lifetime of the process and joined once at startup. Without an IGMP/MLD querier on the VLAN, the switch's snooping entry for the group ages out (~260s) and forwarding to the pod stops — the stream goes silent with a perfectly valid membership still in the kernel. Drive membership from subscriber demand instead of a timer: - moq-transport `Publisher::announce` gains an optional `on_subscribe: mpsc::Sender<(bool, String)>` that reports each subscribe's lifecycle: (true, track) when it begins, (false, track) when the subscriber drops. - moq-pub-mmtp joins the (S,G) group on the first subscribe, refreshes it (leave+join, forcing a fresh IGMP/MLD report) on each subsequent subscribe — re-arming the snooping entry — and leaves when the last subscriber drops. open_udp_socket now binds only for SSM targets; the join is deferred to run_udp_loop. IPv4 uses socket2's join_ssm_v4/leave_ssm_v4; IPv6 (which socket2 does not expose) uses MCAST_JOIN/LEAVE_SOURCE_GROUP via libc. --mmtp-udp-source is now IpAddr; --mmtp-udp-iface-index selects the v6 interface. 56 crate tests pass; whole workspace compiles. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
reviewed head: afade31
Important Issues (2)
- [native-codex]
moq-pub-mmtp/src/main.rs:486-487—refresh_ssm(...)?in thesub_rx.recv()arm propagates any join failure straight out ofrun_udp_loop, which tears down the wholetokio::select!inmain()(session + announce + publisher loop all get cancelled). Before this PR a failed SSM join only happened once, at startup, before any subscriber existed. Now it can fire on every subsequent subscribe (refresh_ssmleave+joins each time), so one transient join hiccup (e.g. a momentaryENOBUFS/interface blip) when subscriber #2 joins kills the session for the already-connected subscriber #1 too. Contrast withleave_ssm(moq-pub-mmtp/src/udp.rs:214), which is explicitly best-effort and only logs on error. Recommend treatingrefresh_ssmfailures the same way (log + leavejoined/active_subsstate as-is, retry on the next subscribe event) rather than propagating via?, since a demand-driven rejoin failing shouldn't be worse than the timer-based staleness this PR is fixing. - [gstack/review] This PR bundles ~940 lines completely unrelated to the stated SSM-membership change: 8 new files under
.planning/(dev-session planning notes for a different repo's epic,Blockcast/multicastBLO-6020, discussing other agents/tickets) and 5 new files under.playwright-mcp/(browser console logs / page snapshots from an unrelated portal debugging session). None of this belongs inmoq-rshistory — looks like local session artifacts picked up by a broadgit add. Recommend dropping these 13 files from the PR (and adding.planning/and.playwright-mcp/to.gitignoreif they're a recurring local-tooling byproduct) before merge.
Suggestions (2)
- [pr-review-toolkit]
moq-pub-mmtp/src/main.rs:475-479— thelet Some((started, track)) = ev else { continue }arm loops forever without yielding oncesub_rxcloses (mpscrecv()returnsNoneimmediately on a closed+drained channel, not pending). It's harmless today only becausesub_txlives insidepublisher.announce(), which racesrun_udp_loopin the same top-levelselect!inmain()— so the whole future tree gets dropped the momentannounce()finishes, before this arm can spin. Worth a comment noting that invariant (orreturn Ok(())instead ofcontinue) so a future refactor that runsrun_udp_loopoutside that specificselect!doesn't reintroduce a busy loop. - [pr-review-toolkit] No CI is configured on this branch (
gh pr checksreports nothing), so the PR body's "56 crate tests pass;cargo check --workspaceclean" is self-reported and unverified by an automated gate for this diff.
Strengths
ssm_membershipcleanly separates "is this SSM" from socket state, validates source/group family agreement, and has direct unit coverage for both families plus the mismatch-error case.- The IPv6
MCAST_JOIN/LEAVE_SOURCE_GROUPgroup_source_reqlayout is done carefully — therepr(C)padding-for-8-align reasoning is spelled out and matches the kernel struct, and theunsafeblocks are scoped tightly with clear safety comments. - Demand-driven design (bind-only until first subscribe, leave-on-last-unsubscribe) is a real improvement over the timer-aging problem described in the PR, and the subscribe/unsubscribe accounting in
moq-transport/publisher.rscorrectly fires the "end" event on any task completion path (error, close, or normal drop), not just orderly unsubscribes.
Recommended Action
- Fix the two Important issues before merge — the error-propagation one is a real regression risk for existing viewers, and the bundled unrelated files shouldn't ship in this repo's history.
- Consider the Suggestions opportunistically.
…p-alive) The subscribe-driven refresh in the prior commit doesn't hold membership for the case that matters. MoQ subscription is not a recurring event: the relay forwards exactly one upstream subscribe per track on the 0→1 downstream transition and fans the rest out from the existing TrackReader. A stable single viewer therefore produces one subscribe at warm-up and then silence, so nothing re-arms the switch's IGMP snooping entry and forwarding dies at the ~260s age-out — the exact bug this was meant to fix. Membership maintenance is inherently periodic (that's what an IGMP querier normally drives); with no querier on the receive fabric the host must self-drive it. So: demand GATES membership (join on first subscribe 0→1, leave on last 1→0), a periodic timer MAINTAINS it (re-report while held). The interval is a required CLI flag (--mmtp-membership-refresh-secs, no default) — it must sit below the fabric IGMP Query Interval (default 125s); an unset value with SSM enabled is a hard error, not a silent guess. - cli.rs: add --mmtp-membership-refresh-secs (Option<u64>, documented). - main.rs: UdpRecvConfig bundles the udp receive params (clears the too_many_arguments smell); membership_refresh_period() extracts the required-when-SSM decision as a pure, unit-tested fn; run_udp_loop joins on 0→1, refreshes on a tokio interval while joined, leaves on 1→0. The timer arm is inert (never resolves) for ASM/unicast. - 4 new tests for membership_refresh_period; 60 pass, crate is clippy-clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
Design correction (790824d): the per-subscribe refresh in afade31 doesn't hold membership for a stable single viewer — the relay forwards one upstream subscribe per track (0→1) and fans the rest out, so there are no recurring subscribe events to refresh on, and the snoop entry still ages out at ~260s. Switched to the correct model: demand gates (join 0→1 / leave 1→0), a periodic timer maintains ( |
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Important Issues (1)
- [native-codex]
.planning/*.md,.playwright-mcp/*.log,.playwright-mcp/*.yml(23 changed files, 13 of which are these) — this PR adds 13 files that are unrelated to the statedmoq-pub-mmtpSSM membership feature: agent planning notes (.planning/blo-6020-next-phase-plan.mdand 6 siblings, which reference an entirely different repo,Blockcast/multicast, plus internal engineering discussion — RecvArchitecture flip, MulticastEngineer status, Codex pool outage) and Playwright browser console/DOM captures (.playwright-mcp/console-*.log,.playwright-mcp/page-*.yml) from an unrelated UI debugging session. None of this belongs inmoq-rs—.gitignoredoesn't exclude.planning/or.playwright-mcp/, so these look like accidentalgit add -A/ workspace-scratch commits carried over from a different task. Recommend dropping these 13 files from the branch (git rm+ amend, or a follow-up commit) before merge, and adding.planning/and.playwright-mcp/to.gitignoreto prevent recurrence.
Suggestions (2)
- [pr-review-toolkit]
moq-pub-mmtp/src/udp.rs(ssm_v6_source_group) — the hand-rolledgroup_source_reqFFI (rawsetsockoptwithMCAST_JOIN/LEAVE_SOURCE_GROUP, manualsockaddr_in6construction) is exactly the kind of code that benefits from a real integration test, but the PR's IPv6 SSM coverage is unit-level only (ssm_membership_matches_family_and_rejects_mismatch) — there's no test that actually exercisesssm_v6_source_groupagainst a live socket the way the IPv4 SSM path is exercised inopen_udp_socket's multicast tests. Understandable given CI multicast sandboxing constraints (existing tests already tolerateEPERM/ENOMEMgracefully), but worth calling out since this is unsafe, platform-specific code with no CI signal on the actual syscall path. - [pr-review-toolkit] No CI checks are configured/reported on this branch (
gh pr checksreturns "no checks reported") — the PR description's "56 crate tests pass;cargo check --workspaceclean" is asserted from local runs only, not verified by an automated gate.
Strengths
- The demand-driven join/refresh/leave state machine (
run_udp_loop) is well-isolated and cleanly guarded:active_subsusessaturating_subto avoid underflow on unmatched events, join/leave are gated on the 0→1/1→0 transitions (not per-subscribe), and the periodic refresh only firesif joined. Theannounce→on_subscribewiring inmoq-transportcorrectly pairs each subscribe-start with exactly one subscribe-end (emitted when the correspondingsubscribe_tasksfuture completes), so the counter can't drift. membership_refresh_periodfails closed — SSM without an explicit refresh interval is a hard error rather than silently reproducing the age-out bug this PR fixes, and that's covered by dedicated unit tests.- All five existing
Publisher::announcecall sites (moq-clock-ietf,moq-pub,moq-relay-ietf/producer.rs,moq-test-client/scenarios.rs×4) were updated consistently withNone, preserving prior behavior.
Recommended Action
- Drop the 13 unrelated
.planning//.playwright-mcp/files before merge (Important). - Suggestions are opportunistic — not blocking.
|
Closing — the premise behind this PR is disproven. This PR (demand-gated join + periodic SSM re-report) was built to fix a ~260s IGMP snoop-entry age-out on the shred receive fabric. Direct investigation shows that age-out does not exist:
The kernel already re-reports membership on every querier query, so a userspace periodic re-report is pure duplication. The demand-gating also regresses behavior (bridge idle without a subscriber). The real shred-over-MoQ fix is the source-specific join in #32 (already merged). The intermittent 'flat after ~100k pkts' is an AMT relay/gateway-layer stall class (cf. the source_port/GMI-window reconnect bug), not a receiver-side refresh problem — a linux-amt / amt-verify follow-up. |
Problem
The
moq-pub-mmtpbridge joins its SSM multicast group once at startup and holds it for the process lifetime. Without an IGMP/MLD querier on the VLAN, the switch's snooping entry ages out (~260s) and forwarding to the pod stops — the stream goes silent while a valid membership still sits in the kernel.Fix — drive membership from subscriber demand (no timer)
Publisher::announcegains an optionalon_subscribe: mpsc::Sender<(bool, String)>reporting each subscribe's lifecycle —(true, track)on start,(false, track)on end.None= unchanged behavior. All in-repo callers passNone.(S,G)group on the first subscribe, refresh it (leave+join → fresh IGMP/MLD report, re-arming the snooping entry) on each subsequent subscribe, and leave when the last subscriber drops.open_udp_socketnow binds only for SSM targets; the join is deferred torun_udp_loop.IPv4 + IPv6
join_ssm_v4/leave_ssm_v4.MCAST_JOIN/LEAVE_SOURCE_GROUPvialibcwith arepr(C)group_source_req.--mmtp-udp-sourceis nowIpAddr;--mmtp-udp-iface-indexselects the v6 interface.56 crate tests pass;
cargo check --workspaceclean.🤖 Generated with Claude Code