Repository navigation
fix(net): publish the send estimate before the connect - #6652
Conversation
Quest-Claim: e4e4180a3ab25ac3
A client could observe a connect before the session's send estimate reached the persistent allocator, so reservation grants read None right after connecting. Seed the session's estimate producer with its first sample, and seed the connection's persistent estimate before publishing the connect. 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 13 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. |
|
Landing this at
(Written by Claude Opus 5.5) |
# Conflicts: # quest/m1/test-flakes-2/README.md
|
Merged (Written by Claude Opus 5.5) |
Summary
TestReservationUpdateReleasesSubscribedTrack(go/wrapper/bandwidth_test.go) failed with "grant unavailable" because a client could observe a connect before the session's send estimate reached the persistent allocator. Two hops raced theconnectscounter:moq_net::Sessioncreated its send-estimate producer empty and only filled it once the driver noticed a consumer and sampled.moq-tokio's reconnect loop publishedConnected(bumpingconnects) beforerun_sessionforwarded that estimate into the connection's persistentsend_bandwidth.Fixed at the source, so no wait is needed in the test at all:
moq-net: the send-estimate producer is seeded with the session's first sample (already taken at construction), soSession::send_bandwidth()reads an estimate from the start whenever the backend has one.moq-tokio:Shared::connectedseeds the persistent estimate before publishing the state, so a watcher that sees the connect already reads the estimate and the reservation grants that divide it.moq-ffi(wasm dial): same ordering, the estimate is seeded before the session is published.The Go test is unchanged: after
waitConnects,Reservation.Grant()is now deterministically non-nil.Quest
quest/m1/test-flakes-2/go-reservation-grant.mdis deleted along with its link in the epic README.Reproduction
Go test,
-race, 32 extra CPU burners on a 24-core host already at load ~30:main(unloaded)main(loaded)Decisions made
Reservationexposes no change signal (only theGrant()snapshot), so a wait would have meant either a polling retry or a new publicGrantChangedacross every binding. Making "connected implies the estimate is readable" true removes the race for every consumer, not just this test.send_bandwidth_samples_while_the_driver_runsinmoq-netto assert the estimate is readable before the driver first runs, rather than adding a new test. No new moq-tokio test: the Go test now exercises the ordering end to end deterministically.Public API and wire impact
Public API: none (no signatures change). Behavior:
moq_net::Session::send_bandwidth()andmoq_tokio::Connection::send_bandwidth()now hold an estimate as soon as a connect is observable, instead ofNoneuntil the first sample was forwarded. Docs on both say so.Wire: none.
Checks
just go check(vet, build,go test -race ./...)nix develop --command just check: every selected module passed, including Rust nextest (8775 passed) and the wasm32 clippy ofmoq-ffinix develop --command just test interop --all: 172/172 passed with the host at load ~33. Two earlier runs at load ~87 failed the browserjspublisher's tone checks,rust-swapped -> js-native-node(timestamp gap), and therust-deniedgrant/Unauthorized check.mainpassed 172/172 at load ~41 between them, and this branch then passed at ~33, so I read those as load flakes rather than this change. They were never compared at equal high load.Follow-ups
rust-deniedpublisher over the WebSocket fallback loggedauth token ended err=no stream creditand did not exit with Unauthorized, and one subscriber reported grantnothing. Worth a quest if it shows up again.Reservation.GrantChangedin moq-ffi and the bindings would let non-Rust senders follow their grant without polling, if a consumer needs it.🤖 Generated with Claude Code
(Written by Claude Opus 5.5)