Repository navigation
Publish claim answers under their own epochs - #5678
Conversation
Co-Authored-By: GPT-5.4 <noreply@openai.com>
|
Implemented the claim answer migration across Rust, TypeScript, C, UniFFI, and hand-written wrappers. Verified:
The PR remains draft because the full cross-language interop matrix and broader relay restart coverage have not run yet. (written by GPT-5.4) |
…-epochs # Conflicts: # rs/moq-c/src/origin.rs # rs/moq-ffi/src/origin.rs
|
Latest-main merge and follow-up fixes are pushed in 2092e8a. Validation:
The PR remains draft while CI runs. Public API: breaking claim API migration across Rust, JS, FFI, C, and wrappers. Wire impact: none. (written by GPT-5.4) |
Remove the duplicate Dynamic::update shim, panic when a Rust application claim is answered with accept, keep JS test claims originated, add Prepared.reject in JS, cover abandoned, released, and pinned answers, and fix the C++, Python, and FFI CI failures. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…-epochs # Conflicts: # quest/m1/README.md
Move the claim upgrade note to the unreleased section, regenerate the Dart bindings, sort the Python imports, and migrate the claims main added since. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A worker claims pool through the public API and restarts pool/p within the cache window; the returning reader at the edge gets the new output, never the old one, and pool/q keeps its reader. The same test with the old accept path joins the old output. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Python, Go, Swift, Kotlin, and Dart tests still answered claims with accept and passed a route; the Python wrapper still exposed OriginDynamic.update. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…-epochs # Conflicts: # quest/m1/claim-answer-epochs.md
Prepared::publish checked the request, announced, then resolved, rolling back if the request was released in between, so a concurrent release could announce the answer and withdraw it again. Admit the request and land the route under the origin's table lock, which every release takes first. A loom model fails on the old sequence. Default-route claims in benches and the rtc test use the public dynamic(prefix). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…-epochs Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Mirror Rust's hidden update_route in JS as an internal updateRoute, mark the session-reached accept and updateRoute as pending the public answer quest, keep model-internal visibility for the atomic announce, move the upgrade note into each package's MIGRATE.md, and refresh the claim paragraph in the moq-lite concept page. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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 6 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: 275b8d55fc
ℹ️ 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".
| this.#done = true; | ||
| return this.#request.publishPrepared(this.broadcast, this.#epoch); |
There was a problem hiding this comment.
Set #done only after publishPrepared succeeds, or clean up on failure. If the prepared broadcast was closed or superseded while tracks were being populated, answer.announce() throws, but this assignment makes the subsequent reject() a no-op. The claim's request then remains pending indefinitely, so all coalesced requesters hang until the entire claim closes.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in cb893b20c4: a failed Prepared.publish() now closes the broadcast and refuses the request before rethrowing, matching Rust where the dropped Prepared releases it. Regression test a prepared answer that fails to publish refuses the request fails without the fix.
(Written by Claude Opus 5.5)
Matches Rust, where a failed Prepared::publish drops the request and resolves its waiters instead of leaving them pending. 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: 09c208e442
ℹ️ 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".
| // Publish announces the answer and resolves every waiting requester. | ||
| func (p *BroadcastPrepared) Publish() (*BroadcastProducer, error) { | ||
| inner, err := p.inner.Publish() | ||
| if err != nil { | ||
| return nil, err |
There was a problem hiding this comment.
When a Go handler successfully calls Prepare but then fails while populating tracks, it can no longer call BroadcastRequest.Reject, while BroadcastPrepared exposes only Broadcast and Publish and hides the generated handle's Destroy. The Rust request therefore remains pending until Go's nondeterministic finalizer runs, potentially leaving every coalesced requester hanging indefinitely. Add a Close/reject operation that synchronously releases the prepared handle.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in bd965eb665: BroadcastPrepared.Close() destroys the handle synchronously, dropping the Rust Prepared and refusing every waiting requester. The other bindings already had a deterministic path (C moq_broadcast_prepared_free, uniffi close()/destroy() on Kotlin and Dart, ARC in Swift, refcounting in Python); Go was the only one hiding it. Regression test TestClosingAPreparedAnswerRefusesTheRequest times out without the call.
(Written by Claude Opus 5.5)
Prepare consumes the request, so a handler that fails while filling tracks could no longer refuse it; the request stayed pending until Go's finalizer freed the handle. Close releases it now, refusing every waiting requester. 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: bd965eb665
ℹ️ 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 unannounced broadcast to fill before publishing. | ||
| pub fn broadcast(&self) -> &broadcast::Producer { | ||
| &self.broadcast |
There was a problem hiding this comment.
When a handler calls prepared.broadcast().announce(...) before Prepared::publish (the bindings expose the same operation), this accessor returns an unrestricted producer whose public announce bypasses both the request-admission check and the epoch stored by prepare. It can therefore publish the answer under an arbitrary or missing epoch, even after the claim/request was released, so abandoning the Prepared no longer guarantees that the answer stayed hidden and the cache-splicing problem this change addresses can recur. Expose a fill-only handle or withhold the announcer until publish so this misuse is unrepresentable.
AGENTS.md reference: AGENTS.md:L66-L66
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 7fe2441d46 by withholding the announcer, with no new public type: prepare takes the broadcast's announcer slot and Prepared holds it, so prepared.broadcast().announce(..) fails with Error::Closed (JS: throws) and route() stays empty. publish hands the announcer back under the same lock as the admission check, and installs it only if the request is admitted and the broadcast is still open. On a refused publish the announcer is dropped, so a cloned producer can't announce the answer afterwards either. The bindings (moq-ffi, C, and the wrappers) wrap the same Rust producer, so they get this behavior automatically.
Regression tests that fail without the fix: a_prepared_answer_cannot_announce_itself and a_refused_prepared_answer_cannot_announce_later (moq-net), a prepared answer cannot announce itself before publish (js/net), plus an assertion in moq-ffi's dynamic_broadcast_request. Each checks that announcing before publish fails and that publish still announces under the prepared epoch.
(Written by Claude Opus 5.5)
`Prepared::broadcast()` handed out a full producer, so a handler could announce the answer itself under any epoch and skip publish's admission. `prepare` now takes the announcer slot: announcing before `publish` fails with `Closed`, and a refused publish drops the announcer for good. The JS `Prepared` does the same; the bindings inherit it from moq-net. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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: 3d7f99cf12
ℹ️ 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".
| prepare(epoch: Epoch.Valid): Prepared { | ||
| if (this.#done) throw new Error("request is closed"); | ||
| if (!this.publisher) throw new Error("forwarded requests cannot be prepared"); | ||
| this.#done = true; |
There was a problem hiding this comment.
When an exception occurs after prepare() but before an explicit publish() or reject(), such as while populating tracks, this marks the request done, so Dynamic.requested()'s finally block calls current.reject(...) but that method immediately returns. Because Prepared has no Symbol.dispose or other automatic cleanup, the entry remains pending and every coalesced requester waits until the entire claim closes. Give Prepared scoped cleanup or keep the request rejectable until the prepared answer settles.
AGENTS.md reference: AGENTS.md:L66-L66
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Declining, because this reopens a decision the maintainer already made. Decision 5 in the description kept JS abandonment explicit (Prepared.reject(err)) over scoped cleanup (Symbol.dispose), and js/net/MIGRATE.md documents that an abandoned Prepared holds its request open. A handler whose fill can throw should catch the error and call prepared.reject(err).
Having the iterator's finally reject an outstanding Prepared would also break the match with Rust. There, prepare consumes the Request, and the Prepared that results lives on its own, independent of the claim's request stream. A handler that prepares, hands the fill to another task, and then leaves the loop would have that answer refused out from under it. That case shows up only on the JS side.
(Written by Claude Opus 5.5)
Merge summary
(Written by Claude Opus 5.5) |
…im-answer-epochs # Conflicts: # cpp/moq/MIGRATE.md # dart/moq/MIGRATE.md # go/wrapper/MIGRATE.md # kt/moq/MIGRATE.md # py/moq-rs/MIGRATE.md # rs/moq-c/MIGRATE.md # rs/moq-ffi/MIGRATE.md # rs/moq-net/MIGRATE.md # swift/MIGRATE.md
The follow and front-reconcile tests from #5925 used dynamic(prefix, route). Unanswered default-route claims become dynamic(prefix); those that accept a peer's broadcast or carry an epoch keep their scenario on the hidden dynamic_route. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Re-enqueue after the merge-queue conflict
(Written by Claude Opus 5.5) |
…im-answer-epochs # Conflicts: # go/wrapper/MIGRATE.md # rs/moq-c/MIGRATE.md # rs/moq-ffi/MIGRATE.md
Re-enqueue after the second merge-queue conflict
(Written by Claude Opus 5.5) |
Drop claim-answer-epochs (landed in #5678) and point replica-resume at the merged PR. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
main's #5678 makes a dynamic claim answer by publishing, so the tests that need a covering prefix route serving on request use the internal route. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Problem
A prefix claim answered every path under the claim's own instance. A worker restarting one output (closing it and answering the next request with a fresh broadcast) therefore looked like the same instance to every hop, so a returning reader could get the old output's cached groups spliced onto the new one.
Approach
Request::prepare(epoch)returns aPreparedholding an unannounced broadcast at the exact path; fill it, thenPrepared::publish()announces it under that epoch and resolves the waiters. Dropping it unpublished refuses the request (JS:prepared.reject(err)).preparewithholds the broadcast's announcer untilpublish, so the answer cannot announce itself under another epoch or skip admission (decision 9).origin.dynamic(prefix)in Rust, JS, C, UniFFI, and every wrapper.dynamic_route/dynamicRoute), its re-pricing (update_route/updateRoute), andaccept, as#[doc(hidden)] pubin Rust and@internalin JS (decision 1). An application claim refusesaccept(Rust panics, JS throws), so the public API cannot serve two outputs under one instance.accepton an application claim is refused. End to end (tests/integration/restart.rsclaim_restart_*): a worker restartspool/pwithin the cache window behind two relays on lite-05/06/07 and moq-transport 14/16/22; the returning reader gets the new output andpool/qkeeps its reader. Swapping the worker back to the oldacceptpath fails that test with "joined the old output".Decisions
Decided by the maintainer; ✅ marks the choice.
dynamic_route,update_route,accept; JSdynamicRoute,updateRoute,accept): ✅ keep them#[doc(hidden)] pub/@internalas the settled interim / make them private in this PR. Their callers model a peer's route (a covering claim with hops, cost, or an epoch, answered with a broadcast the session already routes), which has no public shape yet. Making them private is recorded in Public routes and answering, which owns that public shape, and the session-reached JS members carry its// pending:mark.cost.Prepared::publishadmits the request and lands the route under the origin's table lock, which every release (a new epoch, a retraction, the teardown) takes first, so an answer is never announced and then withdrawn. The loom modela_refused_answer_is_never_announced(just rs loom, nightly) fails on the old check-announce-resolve sequence with "a refused answer was announced".Prepared.reject(err)/Symbol.dispose.Taken while merging main (the recommended option in each case, no maintainer call needed):
Dynamic.update: it was still public, so an application claim could take an epoch in JS. ✅ rename it to an internalupdateRoute, mirroring Rust's hiddenupdate_route/ leave it public.announced_broadcast_hands_out_the_instance_again_after_an_end): ✅ keep its scenario on the hidden peer route through the inner origin / rewrite it ontoprepare, which publishes a new instance and so no longer tests the same-instance hand-out.MIGRATE.md. ✅ one entry per affected package (moq-net, @moq/net, moq-ffi, moq-c, Python, Go, Swift, Kotlin, Dart, C++) / keep a cross-package line indoc/setup/upgrade.md, which now covers only wire ordering.Decided by the maintainer on review:
Prepared::broadcast()returned a full producer, so a handler couldannounce()the answer under any epoch and skippublish()'s admission. ✅ withhold the announcer:preparetakes the announcer slot, announcing beforepublish()fails withClosed(JS throws), and a refusedpublish()drops the announcer so no clone can announce it later / expose a fill-only handle type. No new public type; the bindings inherit it from the Rust producer.Impact
Public API: breaking.
origin::Producer::dynamic(prefix)takes no route; neworigin::Request::prepare(epoch)andorigin::Prepared::{broadcast, publish};Dynamic::updateandRequest::accepthidden (acceptpanics on an application claim).Prepared::broadcast()'s ownannouncefails withCloseduntilpublish(JS:prepared.broadcast.announce()throws; the bindings'announceerrors withClosed).Producer.dynamic(prefix); newRequest.prepare(epoch)andPrepared { broadcast, publish(), reject(err) };Dynamic.updatebecomes the internalupdateRoute;acceptis internal and throws on an application claim.dynamic(prefix),MoqBroadcastRequest.prepare(epoch),MoqBroadcastPrepared { broadcast, publish }(aliasedBroadcastPreparedin Python, Swift, Kotlin, Dart, Go; generated asBroadcastPreparedin C++);acceptandMoqOriginDynamic.updateremoved.moq_origin_dynamictakes no route;moq_origin_dynamic_updateandmoq_broadcast_request_acceptremoved;moq_broadcast_request_prepare,moq_broadcast_prepared_publish,moq_broadcast_prepared_freeadded, as the C freeze's narrow claim exception.Wire impact: none.
Checks
just check(branch against main, at the final head): passed. One run hit the knownredundant_importers_switch_through_bufferbloat_on_lite_07flake (quest); the rerun passed.just rs loom a_refused_answer(the publish race model, after withholding the announcer): passed.just checkpassed, including Rust, JS, Go, Python, Kotlin, Dart, C++, docs, and the model-marker lint. Swift was skipped (no Swift toolchain on this Linux host).just test interop --all(final head): passed, 74 PASS, 0 FAIL. Earlier round: Rust protocol interop 5/5, the Rust, Python, Go, C++, and JS publisher matrix, browser close code, CLI FETCH close, finite track tails, and the drop cases (74 PASS, 0 FAIL).quest check: passed.Follow-ups
🤖 Generated with Claude Code
(Written by Claude Opus 5.5)