feat(catalog): adopt canonical MSF multicast schema#44
Conversation
1 similar comment
|
@ally Please review the catalog wire-contract rewrite, especially strict serde/schema parity, multicast endpoint defaults, repair priority propagation, and regressions in legacy |
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: be919ad
Important Issues (2)
-
[native-codex]
moq-catalog/src/multicast.rs:69-73—MulticastConfig::endpointsandMulticastConfig::network_sourcelost their#[serde(skip_serializing_if = "Option::is_none")]attributes in this rewrite (the old struct had both; every other optional field in this schema still uses the attribute). AMulticastConfigwith only one side populated — which is exactly what themoq-pub-mmtptest helperendpoint()builds (network_source: Nonealways) — now serializes as"networkSource":null/"endpoints":nullinstead of omitting the key. That's a schema-conformance regression on a PR whose whole point is a schema-faithful,deny_unknown_fields-strict catalog. Recommend restoringskip_serializing_if = "Option::is_none"on both fields. -
[native-codex]
moq-pub-mmtp/src/main.rs:159-165and:356-365—build_state_mapandbuild_datagram_statenow hardcodelet history_window_raw = 32;, but the doc comments directly above are unchanged and still claim "Config-or-throw... there is no silent unbounded default" (line 161) and "There is no silent unbounded — or silently lossy — default... the window must be an explicit catalog decision" (build_datagram_state). The removed field wasmulticast.subgroupHistoryGroups, which no longer exists in the canonical schema — yet theNonZeroU64::new(...).ok_or_elseerror text on line 365 still referencesmulticast.subgroupHistoryGroupsverbatim, an error path that can now never fire (32 is never zero) and describes a config key that doesn't exist anymore. This silently drops what was previously a required per-deployment tunable; at minimum the stale comments/error text should be updated to reflect that retention is now a fixed runtime constant, not catalog-driven.
Suggestions (3)
-
[gstack/review]
moq-catalog/src/lib.rs:401-424(validate_multicast) — wheneverRoot.multicastisSome(_),networkSourceis now required to be present and non-empty, even for a plain SSM/ASM endpoint with no AMT/ATSC3 bridging. The removed doc comments describednetworkSourceas OPTIONAL (§4.2.3), and none of the three golden positive fixtures exercise a multicast catalog withoutnetworkSource, so this constraint is untested. Worth confirming against the canonical MSF spec thatnetworkSourceis truly mandatory for every multicast catalog, not just AMT/ATSC3-bridged ones. -
[pr-review-toolkit]
moq-catalog/src/lib.rs:410-422— theendpoints.is_empty()check on line 419 is unreachable:multicast.endpoints.as_ref().filter(|value| !value.is_empty()).ok_or(...)on lines 407-411 already guaranteesendpointsis non-empty before that later check runs. Dead code left over from the merge. -
[pr-review-toolkit]
moq-sub/src/media.rs(catalog validation block inrun) — the previous unconditionalanyhow::ensure!(c.version == 1, ...)was replaced with a check that only runsRoot::validate()(which itself checksversion) whenstreaming_format == "mmtp".moq-pub's catalogs arestreaming_format: "cmaf", somoq-subnow performs zero validation — not even the old version check — on the catalogs it actually consumes frommoq-pub. Likely low-impact for an internal test client, but worth a conscious call rather than an incidental drop.
Strengths
- The golden-fixture round-trip test (
moq-catalog/tests/golden.rs) is a solid regression guard: it pins both "parses + validates + re-serializes losslessly" for positive fixtures and "rejected at parse-or-validate" for negative ones. - Threading the repair track's catalog-declared
priority(240) throughRepairSinkinstead of hardcoding7inpublish.rsis a clean, well-tested change (dispatchtests updated to match). deny_unknown_fields+ taggedNetworkSourceenum + narrowedMulticastProtocol/AmtDiscoveryenums meaningfully tighten the wire format vs. the old stringly-typed fields.
Recommended Action
- Fix the two Important issues before merge — both are concrete, reproducible regressions with clear repros.
- Consider the Suggestions opportunistically; the
networkSourcemandatoriness is worth a quick spec check since it's currently unverified by any fixture.
Co-Authored-By: Paperclip <noreply@paperclip.ing>
be919ad to
5383e61
Compare
|
@ally Please re-review rebased head 5383e61. The two Important findings and all three suggestions are addressed: optional fields omit nulls, retention is a named fixed runtime policy, networkSource is optional with coverage, dead validation is removed, and moq-sub restores unconditional version checking. The rebase also preserves #43 formula/interleave grouping and the relay-required catalog aliases. Local cargo test --workspace, strict changed-target Clippy, cargo fmt, and REUSE lint pass. |
Summary
catalogtrack and priority 240 repair deliveryContract decision
The merged golden fixtures from BLO-16823 are the executable source of truth. They retain
version: 1,streamingFormat: "mmtp", andstreamingFormatVersion; this PR follows that contract rather than the staleversion: "draft-01"wording in the issue description.Verification
cargo test --workspacecargo clippy -p moq-catalog --all-targets -- -D warningscargo clippy -p moq-pub-mmtp --bin moq-pub-mmtp -- -D warningscargo clippy -p moq-pub -p moq-sub --lib --bins -- -D warningsnpm test -- --run src/catalog-schema.test.ts(13 passed)Notes
Full
moq-pub-mmtp --all-targetsclippy still reaches a pre-existingtoo_many_argumentswarning inexamples/mmtp_packet_vectors.rs; the changed binary target is clean.Paperclip: https://paperclip.blockcast.net/BLO/issues/BLO-16831