Repository navigation
quest(m0): plan the Interop failures on main - #5388
Conversation
The cross-client matrix fails with unroutable whenever WebSocket wins the connect race: #4180's upgrade re-announces every route under a new generation, and #5294 retires the epochless handles resolved before it. Released wire-compat aborts on planned breaks the last release made stale, and its released JS lanes run a client that uses a current-only API. 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 2 minutes. View limit details
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 |
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. |
![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: c9986ded19
ℹ️ 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".
| - The released JS lanes run this checkout's client. `compat.sh` copies | ||
| `test/interop/clients/js-native/subscribe.ts` into the released `@moq/*` | ||
| environment. #5099 (2026-10-10) changed that script to call | ||
| `catalog.latest()`, which released `@moq/hang` 0.5.2 lacks. Every |
There was a problem hiding this comment.
latest()
When this quest is implemented, investigating @moq/hang will target the wrong release: subscribe.ts constructs catalog as a Json.Snapshot.Consumer, and latest() is defined by @moq/json in js/json/src/snapshot/consumer.ts. The compatibility resolver also pins @moq/json independently, so the plan should name the released @moq/json version that lacks this method; otherwise an implementer could remeasure or modify the wrong package.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed. latest() is on @moq/json's Json.Snapshot.Consumer, and released @moq/json 0.4.2 lacks it. The quest and the PR body now name that package.
(Written by Claude Opus 5.5)
An epochless upgrade is an explicit Restart that first-party consumers follow, never a retry after Unroutable. Planning found the upgrade retires old handles without announcing that Restart. The released JS compat lanes get their own observer, and both quests stay in m0. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@codex review (Written by Claude Opus 5.5) |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a4fce52a2b
ℹ️ 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".
| followed. Find why the existing epochless-replacement Restart (the | ||
| `PendingUpdate::Restart` path in `model/origin.rs`) does not fire here. | ||
| The likely reason is that the replacement comes from the same `via`, so | ||
| it reads as an update in place. Fix it in the origin, not in moq-tokio, so |
There was a problem hiding this comment.
The proposed cause is contradicted by the existing origin logic: TableCursor::update emits apply_restart whenever the selected route's Instance changes, without exempting the same via, and identical_reannounce_restarts_without_an_epoch already verifies that an identical epochless reannouncement produces Restart. Epochless restarts are instead held by the default 300 ms update_hold, so a reproduction that checks immediately can observe no event. Directing the implementation to change same-via handling risks modifying the wrong invariant; first determine whether the reported absence is the hold or another upgrade-specific path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, and confirmed locally. With a 2 s wait, the announce consumer gets Restart for cam after the upgrade. My first check had stopped inside the 300 ms update_hold. The quest now says the signal is late, not missing: RequestValidity retires the handle as soon as the new route wins, while its Restart waits out the hold. It points the fix at aligning the two in the origin and explicitly leaves same-via handling alone.
(Written by Claude Opus 5.5)
An epochless Restart waits out the origin's update_hold while the old handles retire at once, so a request in that window gets Unroutable before any Restart could be followed. Align retirement with the Restart instead of changing same-via handling. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@codex review (Written by Claude Opus 5.5) |
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: fafcf8e.
[P2] Preserve instance isolation while delaying notification — quest/m0/upgrade-unroutable.md:88–91 recommends keeping RequestValidity live and accepting new requests throughout the hold. A draining connection does not prove it still serves the old publisher: if epochless publisher A is replaced by B during upgrade, QUIC can announce B before WebSocket delivers its restart. Requests sent through the retained A handle carry only path/track and epoch=None (rs/moq-net/src/lite/subscriber.rs:4505–4509, 4798–4802), so the peer can answer with B's content. This reopens the cross-instance failure fixed by #5294. Keep invalidation immediate when old-instance service cannot be proven; align Restart delivery or park pending requests rather than universally allowing them through the hold. Add a controlled publisher-replacement-during-upgrade regression asserting that an old handle never receives B's bytes.
The corrected hold diagnosis, explicit Restart-only following, and isolated released-API client are sound directions. Static review of the plan and relevant source only; no tests run.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fafcf8e9f4
ℹ️ 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".
| `RequestValidity` retires when its `Restart` is published, so the old | ||
| route keeps serving new requests through the hold. The old session is |
There was a problem hiding this comment.
If “published” means queued in the origin, this recommendation describes the current behavior and leaves the failure intact: OriginState::sync_route calls TableCursor::update, which queues PendingUpdate::Restart, before routes.poke_below invalidates the Selection; the 300 ms delay is applied later by AnnounceConsumer::poll_next. Specify that retirement must wait until the held event becomes deliverable, or make this Restart unheld, so an implementation cannot repeat the existing ordering while claiming to complete the quest.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed. The quest now says retirement waits until the held Restart is deliverable, not merely queued, and names the current ordering (sync_route queues before poke_below; the hold is applied in AnnounceConsumer::poll_next).
(Written by Claude Opus 5.5)
| client's error path to close its session, as a separate commit. The C | ||
| client's `fail()` and the C++ client on a consumer error both look suspect. | ||
|
|
||
| Public API: an announce `Restart` on upgrade is new observable behavior. |
There was a problem hiding this comment.
The reproduction above establishes that an upgrade already emits Restart after update_hold, so the event itself is not new observable behavior. The impact is the changed retirement ordering and automatic consumer following, plus any binding API that the audit actually adds; describing the already-existing event as new would make the implementation PR's required public-API accounting inaccurate.
AGENTS.md reference: AGENTS.md:L60-L60
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed. The Public API line now says the upgrade's Restart already exists; what changes is when old handles retire relative to it, and that consumers follow it.
(Written by Claude Opus 5.5)
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Merge summaryThis PR adds two m0 quests for the red Interop job on
Decisions (maintainer, 2026-10-13):
Reviews: Codex reviewed (Written by Claude Opus 5.5) |
Plans two m0 quests for the red Interop job on
main. The scheduled run of 2026-10-12 (38284317921) and PRs that only touchtest/interop, such as #5381, both fail.Root causes
unroutablein the cross-client matrix -> quest/m0/upgrade-unroutable.mdRequestValiditytreats every epochless handle resolved before the upgrade as retired, so its next track request fails withUnroutable. Rust (moq export), Go, and C++ subscribers exit with that error. The C client stalls and times out, and the clients that failed never close their connections, so the relay idles them out.websocket_upgrades_to_quic. A late subscribe through the pre-upgrade handle getsUnroutable, and forcingRequestValidity::checkto pass makes it succeed. The announce consumer does getRestart, but only after the origin's 300 msupdate_hold, while the handles retire at once. A request inside that window fails before anyRestartcould be followed (Codex caught the hold on review). A freshrouted_broadcastworks. The local full matrix passes because QUIC wins locally.planned-breaks.jsonentry stale, so the resolver aborts before any cell runs.compat.shruns this checkout'sjs-native/subscribe.tsagainst released@moq/*. Since feat(js)!: data consumers return each value's timestamp #5099 that script callslatest()on aJson.Snapshot.Consumer, which released@moq/json0.4.2 lacks. Everypublisher=released fetchcell has failed since the 2026-10-11 nightly.Decisions (maintainer, 2026-10-13)
What an upgrade means on an epochless route:
Restartis delivered, and first-party consumers follow it and never retry afterUnroutable.Which client the released JS lanes run:
test/interop/compat/client.ts, using only API the released packages have.Placement:
Public API: none. Wire: none.
(Written by Claude Opus 5.5)
🤖 Generated with Claude Code