fix(moq-pub-mmtp): preserve framed input across reconnects - #80
Merged
kkroo merged 4 commits intoAug 25, 2026
Merged
Conversation
…26173) Backport of #76 (reconnect loop) + #78's publisher-local backoff reset onto the focused live-edge baseline 2100678, scoped strictly to moq-pub-mmtp/**. Production `blockcastd-moq-pub-mmtp` is digest-pinned to a moq-rs commit that carries only the four-MTU oldest-drop queue bound on the prior protocol baseline, deliberately without the broad moq-rs upgrade (onprem-k8s production-values.yaml:1221, authored by Omar Ramadan 2026-08-06). Gateway main's pin is 165 commits ahead of that and moves moq-transport's request-ID handling and draft-19 wire tests — the exact surface whose mismatch reingests the "session error: invalid request ID" crashloop. So the reconnect fix is backported here rather than shipped by re-pinning. What changed: - The relay session is established inside a retry loop with exponential backoff (--reconnect-backoff-min-ms 500 .. --reconnect-backoff-max-ms 30000) instead of propagating the error out of main and exiting 1. - The input source is opened ONCE and held across every reconnect. For --mmtp-input=udp that keeps the SSM (S,G) join alive: the join is a property of the socket, not of the moq-transport session. - Packet ingest moves from a third spawned task into the session select!, so the source is owned by main across reconnects. Ingest remains a single ordered flow, so datagram.rs's monotonic group_id assignment is unchanged. - A session that outlives the backoff ceiling clears the consecutive-failure streak, so a publisher that has dropped N times over its lifetime does not stall the full ceiling on every later blip (#78). - Clean stdin EOF still exits 0 rather than reconnecting forever. Deliberately NOT backported: - The `moq_pub_session_reconnect_total` counter. `metrics` is not a dependency of moq-pub-mmtp at this baseline and metrics_endpoint.rs does not exist here; adding them pulls in the observability stack this backport exists to avoid. Reconnects are observable via the structured task=/attempt=/backoff_ms= log fields instead, which satisfies BLO-26173's "counter and/or log at info". - #78's moq-transport/src/serve/subgroup.rs binary-search change. It is a pure read-path optimization, unrelated to reconnect, and touching moq-transport is precisely what the pinned pub/relay pair forbids. `git diff 2100678..HEAD -- moq-transport/` is empty by construction. Co-Authored-By: Paperclip <noreply@paperclip.ing>
A PR based on release/pinned-live-edge-21006781 got zero check runs: the pull_request trigger filtered on base == main only. That is the same silent no-CI condition that let 54d855f merge a tree failing `cargo test -p moq-pub-mmtp` (see moq-pub-mmtp/src/metrics_endpoint.rs's note). A pinned-baseline lane is exactly where an unbuilt merge is most dangerous: the whole point is that production runs that ref, not main. Co-Authored-By: Paperclip <noreply@paperclip.ing>
![allyblockcast[bot]](https://avatars.githubusercontent.com/u/57363972?s=60&v=4){kind=small}
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: 7aed580
Critical Issues (0)
Important Issues (1)
- [native-codex]
.github/workflows/pr.yml:14— addingrelease/**activates a workspace-widecargo clippy --no-depscheck that currently fails on the release baseline, so PRs targeting these branches cannot get a green required build check.- The live run for this head passed
cargo test --verboseand the production shred datagram E2E, but failed the clippy step underRUSTFLAGS=-D warningson pre-existingmoq-transportlints (serve/subgroup.rs:120needless_late_initandwatch/queue.rs:81result_unit_err). Either make the release lane’s clippy scope/configuration pass for this baseline or avoid enabling the lane until the baseline is clean; otherwise the workflow change creates a merge-blocking check for the target it is meant to protect.
- The live run for this head passed
Suggestions (0)
Strengths
- The process-lifetime input reader prevents reconnect cancellation from consuming a partial stdin frame.
- The bounded channel preserves backpressure, and the dedicated regression test covers a prefix/body split across reads.
- Reconnect attempts use capped exponential backoff and reset the streak after a healthy session.
Recommended Action
- Address the Important workflow check failure before merge.
- Re-run the full release-targeted CI lane after the clippy baseline is resolved.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
select!cancellation from dropping a partially read stdin frame during relay reconnectstokio::io::duplexFixes the important review finding on #79. UDP remains owned by the reader task, preserving the source-specific multicast join across relay session replacement.
Verification
cargo fmt --all -- --checkcargo test -p moq-pub-mmtp(80 unit tests, 2 packet-vector tests, 4 reassembler-vector tests)git diff --check