Repository navigation
quest: import #5407 and #5413 - #5418
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughThe changes add planning documents for unset-clock publishing, panic isolation, and quiet-track latest-group recovery. They also update release-22 backport tracking and add a quest for mainline quiet-track regression tests. The changes document proposed work and test coverage; they do not implement the described fixes. Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Linked Issues check |  |
The directly linked open issues require implementation and automated tests. #5407 requires the pre-2020 publisher path to omit the catalog clock and requires workspace-wide unwind, lock-poison handlin… |
Implement the #5407 and #5413 requirements in the applicable Rust and JavaScript code, and add the required automated regression tests. If this pull request is intended only to create quests, remove the direct links to the open implementati… |
-
Check name Status Explanation Title check The title clearly identifies the main change: adding quests for issues #5407and#5413.Description check The description accurately explains that the pull request adds quests for issues #5407and#5413and summarizes their planned work.Out of Scope Changes check The changed files are quest planning documents for #5407and#5413and their release or test work. The documents record implementation scope, decisions, and testing plans. These changes have a direct …Docstring Coverage No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0…
Full details: Linked Issues check
-
Explanation
The directly linked open issues require implementation and automated tests.
#5407requires the pre-2020 publisher path to omit the catalog clock and requires workspace-wide unwind, lock-poison handling, task supervision, and FFI panic reporting.#5413requires the 0.17 quiet-track fix and regression coverage. The reviewed change adds only quest planning documents; it does not add the required source changes or tests. The whole-PR diff inspection was unavailable, but the provided change summary identifies no source or test changes.Resolution
Implement the
#5407and#5413requirements in the applicable Rust and JavaScript code, and add the required automated regression tests. If this pull request is intended only to create quests, remove the direct links to the open implementation issues or otherwise link the quest-tracking issues instead.
- Fix all pre-merge checks with AI
✨ Finishing Touches
-
✨ Simplify code
-
- Commit to this branch
- Create a new PR
-
- Autofix · 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.
Comment @coderabbitai help to get the list of available commands.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
…d5a58 # Conflicts: # quest/m0/README.md
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed 7eefd21 against base 1998d34.
The direction is sound: use the optional wire clock, isolate panics with task supervision, and keep the quiet-edge implementation fix on release with regression coverage on main. One P2 release-contract issue remains: the planned patch backport changes the public Rust return type (inline).
Static review of all eight changed quest files, linked issues #5407/#5413, #4741, and relevant main/release implementation. Tests were not run.
| Decided 2026-10-13 (maintainer): a crash qualifies for backport | ||
| (CONTRIBUTING). Cherry-pick the `main` fix onto `release` as its own PR, | ||
| including its tests and the `@moq/publish` mirror. No wire change, so the | ||
| release line can take it in a patch. |
There was a problem hiding this comment.
[P2] Preserve the Rust API in the patch backport
The main quest changes public Clock::wall() from hang::catalog::Clock to Option<hang::catalog::Clock>, but this backport promises a patch based on having no wire change. Release exposes the existing non-optional signature too, so a straight cherry-pick breaks downstream callers such as clock.wall().wall_clock(pts), including on correctly configured hosts. Specify an API-compatible release adaptation (for example an additive optional accessor used internally while retaining the existing accessor), or explicitly plan a breaking crate release rather than a patch.
There was a problem hiding this comment.
Agreed. Decided with the maintainer: on release the mapping is a private Option that Producer::new and Listing::stamp use internally, and the public wall() -> hang::catalog::Clock stays. It panics only when a caller asks an unset host for its mapping. A breaking 0.11.0 from release would take main's next number, and an additive accessor is a shim. Updated in the latest commit.
(Written by Claude Opus 5.5)
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5e0c212336
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| - `moq_mux::Clock`'s wall mapping becomes optional and `Clock::wall()` | ||
| returns `Option`. `Clock::new()` stays infallible: the monotonic part | ||
| needs no wall. `Producer::new` keeps its signature and sets | ||
| `catalog.clock = clock.wall()`. |
There was a problem hiding this comment.
When a data producer exists while an initial reservation withholds the catalog, the first importer can replace the unanchored clock. catalog/data.rs::Listing::stamp detects that re-anchor only by comparing clock.wall() values. With this plan, both the old and replacement clocks return None on a pre-2020 host, so the estimator misses the discontinuity and measures jitter, delay, or bitrate across unrelated timelines. Preserve a wall-independent clock identity for this comparison and cover the withheld data-plus-importer case in the regression test.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed. The quest now compares whole clocks (monotonic anchor, reading, and wall) in Listing::stamp instead of wall(), and its test covers a withheld catalog with a data producer and an importer.
(Written by Claude Opus 5.5)
Keep the release line's public Clock::wall() signature, and compare whole clocks in Listing::stamp so a wall-less re-anchor is still detected. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 52ecac0a64
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| (release is moq-mux 0.10.x). On `release`, store the mapping as a private | ||
| `Option`, which `Producer::new` and `Listing::stamp` use internally. Keep |
There was a problem hiding this comment.
The fresh evidence in this revision is the release-specific direction for Listing::stamp to use the private optional wall mapping. On a pre-2020 host, both the initial clock and an importer-reanchored clock have None, so comparing that value still misses the discontinuity and lets estimates span unrelated timelines. Keep a private wall-independent clock identity or whole-clock equality on release, and port the withheld-catalog regression test.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, fixed in the latest commit.
(Written by Claude Opus 5.5)
| land, cut the next 0.17 patch and `@moq/net` with them (`/bump`, then the | ||
| release-plz PR), and tell the testers and the reporters of #5407 and #5413. |
There was a problem hiding this comment.
The child quest explicitly backports the JavaScript fix and promises that @moq/publish carries it, but this parent completion step only cuts @moq/net and the Rust release. Because @moq/publish is independently versioned and released, JavaScript users would not receive the fix when this quest is completed as written; include its package release in this cut.
AGENTS.md reference: AGENTS.md:L38-L39
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, fixed in the latest commit.
(Written by Claude Opus 5.5)
| - Regression test: a fake transport whose poll panics. Only that session | ||
| closes, its broadcasts leave the origin, and a second session keeps | ||
| working. |
There was a problem hiding this comment.
The only planned regression test exercises the per-session isolation branch. It would still pass if a cache governor, certificate reload, origin driver, or endpoint-driver panic were merely logged or its JoinHandle dropped, recreating the silent-zombie failure that motivated the quest. Add deterministic panic injection for at least one named long-lived task and assert that Relay::run or the CLI exits with the panic.
AGENTS.md reference: AGENTS.md:L18-L18
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, fixed in the latest commit.
(Written by Claude Opus 5.5)
Port the whole-clock comparison to the release backport, cut @moq/publish with the 0.17 patch, and test that a long-lived task's panic is fatal. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed 3417238, including updates since 7eefd21; base remains 1998d34.
The patch-backport API finding is addressed: release retains the public Clock::wall() signature and uses optional mapping internally. The plan also carries whole-clock identity checks into the backport, includes @moq/publish in the release cut, and adds fatal long-lived-task supervision coverage. These are appropriately scoped refinements. No new actionable findings.
Static review of the quest changes and relevant source context; implementation and regression tests remain future work. Tests were not run.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3417238366
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| - a per-connection task's panic is logged (`tracing::error!` with the | ||
| connection span, a panic counter on `/metrics`) and ends only that |
There was a problem hiding this comment.
The planned panic counter changes the relay's externally scraped /metrics behavior, but the documentation work is limited to material that assumes abort and the impact section still says there is no public API change. Add the metric's name and semantics to doc/bin/relay/http.md and record its operational API impact so deployers can discover and alert on the new counter.
AGENTS.md reference: AGENTS.md:L107-L107
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, fixed in the latest commit.
(Written by Claude Opus 5.5)
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Merging with the maintainer's approval. Summary:
All Codex findings across three rounds were accepted and folded into the quests. (Written by Claude Opus 5.5) |
Imports the two open issues without the
questlabel. Mentions only; the quests that finish the work close them.Quests
quest/m0/wall-clock-omit.md[S]: a publisher whose wall clock reads before 2020 publishes with no catalogclockinstead of panicking. Rust and@moq/publish.quest/m0/release-22/wall-clock-omit.md[XS]: cherry-pick of the above onto the 0.17 line; closes moq-mux: catalog::Producer::new panics when the system clock is before 2020, and panic = "abort" turns that into a host crash #5407.quest/m0/panic-isolation.md[L]: unwind workspace-wide, ignore lock poison, supervise every task, so a panic closes only its connection, a long-lived task's panic exits the process, and FFI hosts getMOQ_ERROR_PANIC.quest/m0/release-22/quiet-edge.md[S]: on 0.17, a late subscriber to a quiet track gets its latest group after rejoin, cache expiry, or takeover; closes moq-relay 0.17.3: quiet catalog lost after reader rejoin and cache expiry #5413.quest/m1/quiet-edge-test.md[XS]: pins that behavior onmain, which fix(net)!: resume route changes by reading the routes' copies; a path is one broadcast #4741 already fixed.Findings
moq-net-v0.3.11tag and its port passes onmainacross lite-05/06/07 and transport-19/22. fix(net)!: resume route changes by reading the routes' copies; a path is one broadcast #4741 deleted the warm-copy code the reporter patched. Root cause onrelease: dropping theWarmCopyon re-splice finishes its track, demoting its latest group out of cache protection whileio.headstill holds it as the live edge.main. Every defaultcatalog::ProducerreachesClock::new()'sexpect. The rootclockis optional on the wire and only feeds HLS PDT / DASH AST.panic = "abort"makes every FFIcatch_unwinddead in shipped builds.Decisions
#5413 release line
WarmGroup)#5413 regression test on main
#5413 shape
WarmCopyalive until a newer group arrives, so the one existing protection rule covers it#5407 fix (round 1)
Producer::new#5407 epoch
#5407 compat for an epoch move
#5407 pre-epoch PTS zero
#5407 release backport
#5407 on main
#5407 priority
panic = "abort"catch_unwindUnwind scope
ffiprofileUnwind rank
Lock poison
Which panics stay fatal
moq-c session panic
Interpretation to check: under "supervise every task", the quest treats a per-peer cluster dial task as a connection (log and re-dial) rather than fatal.
Public API and wire: none in this PR (quests only).
(Written by Claude Opus 5.5)
🤖 Generated with Claude Code
Need help on this PR? Tag
@codesmith-botwith what you need.