Skip to content

fix(net): backport withdrawing IETF advertisements without send credit to release - #7922

Merged
kixelated merged 2 commits into
releasefrom
backport/withdraw-without-credit
Nov 5, 2026
Merged

kixelated merged 2 commits into
releasefrom
backport/withdraw-without-credit

Conversation

@kixelated

@kixelated kixelated commented Nov 4, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

On release, the unsolicited announce loop awaits every PUBLISH_NAMESPACE and REQUEST_UPDATE write. A peer that withholds send credit on one request stream pins the loop on that write, so withdrawing the namespace, and every other announce, withdrawal, and retry behind it, never runs for the rest of the session.

Reproduced on release (f48874e434) with the new test below: withdrawing a draft-17 advertisement written with zero credit produced no reset at all (moq-transport-17, FirstWrite, partial=0, Local: left: [], right: [1]).

Tracking quest: quest/m0/backport-release-hardening.md, claimed in moq-dev/moq#7596. Backports moq-dev/moq#7414.

Approach

Hand-port of #7414's behavior; release's publisher is async and lacks main's per-request event machinery, so the layout differs.

  • advertise_namespace and update_namespace buffer their message and record it as pending instead of awaiting the write and the answer.
  • The loop's event wait writes and reads each woken request on its own stream, polling only the requests whose stream woke. The answer is read even while the write is stalled, so a refusal or the peer ending the request is seen without credit.
  • The answer deadline (ADVERTISE_TIMEOUT) starts once the message is fully written. Backpressure alone never times a request out.
  • One message in flight per request: route changes meanwhile coalesce, and the answer re-runs the sync. An early REQUEST_OK is held until our write completes, so it cannot admit an update behind the stalled one; a second message meanwhile is UnexpectedMessage.
  • Withdrawing a request whose message is still being written or awaiting its answer resets it in both directions (RESET_STREAM and STOP_SENDING, CANCELLED). An idle accepted request still withdraws gracefully as before (PUBLISH_NAMESPACE_DONE on drafts 14-16, the FIN on 17+). An unanswered message, and a refusal of the initial advertisement or of a partly written update, also end the request in both directions.
  • Drafts 14-16 ride the control stream through the adapter, whose virtual writes never wait for credit and cannot reset one request, so they keep PUBLISH_NAMESPACE_DONE in every case.

Impact

  • Public API: none. sh/api-diff/run.sh --deny rust against origin/release: see Validation.
  • Wire: none in message format. A withdrawal of a request still writing or awaiting its answer is now a reset in both directions instead of waiting for the answer; an unanswered or refused advertisement now also sends STOP_SENDING where it previously only reset.
  • No dependency or version changes, tags, publication, deployment, or merges.

Alternatives

  • Bounding each write with ADVERTISE_TIMEOUT, like the open and the answer read: it still holds the whole loop for the timeout per stalled write, and times out a peer that is merely slow to grant credit. fix(net): withdraw IETF advertisements without send credit #7414 settled that backpressure alone starts no timeout.
  • One exchange in flight across all namespaces: smaller, but a single starved request still blocks advertising every other namespace.

Choices made without asking

  • Kept release's handling of a request stream that the peer FINs or resets before answering: it still ends the announce loop with an error. Main ends only that advertisement, a change that predates fix(net): withdraw IETF advertisements without send credit #7414 there; the new test pins that the stall no longer hides the peer's end.
  • Main's epoch-replacement and credential-retention cases are not ported: release has no advertisement epochs or auth links.
  • Tests cover every request-stream draft release speaks (17 through 22).
  • Test transport: ScriptedSession::with_send_window (each bidi stream sends only its first N bytes, then parks for good) and readers parked on an exhausted script are woken by push and close.

Validation

Head 0fcb088cb93ad7ce138b23e58df60086fa77a597 merges origin/release at 6d6f264492 (after moq-dev/moq#7920) into 55000adf43. The merge was conflict-free and touched only moq-relay and its doc; git diff origin/release 0fcb088cb9 is byte-identical to git diff f48874e434 55000adf43. Each run through quest gate nix develop --command ...:

  • just ci check origin/release: pass.
  • just ci test origin/release: pass. Rust: 5189 passed, 11 skipped; 387 passed, 3 skipped; 133 passed (moq-cli). JS suites pass.
  • API diff (release lacks sh/api-diff/; ran main's sh/api-diff/run.sh --deny against this head): Rust passes, all 30 published crates unchanged, including moq-net 0.3.20. JS reported every @moq/* surface as new, not a break: main's renderer drew no baseline from release, so it compared nothing. The PR touches no JS. FFI could not run: main's ffi.sh builds moq-c, which release names libmoq. The PR touches neither rs/libmoq nor rs/moq-ffi, and the Rust diff shows moq-net's API unchanged.

Earlier, on 55000adf43:

  • Reproduction: with only the new test and the test-transport support on release, stalled_advertisements_observe_local_and_peer_ends fails at its first case, moq-transport-17, FirstWrite, partial=0, Local: no reset (left: [], right: [1]). With the fix it passes for drafts 17 through 22, every stage (initial write, initial answer, update write), zero and one byte of credit, and each end (local withdrawal, refusal, FIN, reset).
  • New tests also cover the answer deadline starting once the message is written (an_unanswered_advertisement_is_given_up_at_its_deadline) and the early-OK hold (an_early_ok_admits_nothing_behind_a_stalled_write). a_silent_answer_still_lets_the_next_namespace_be_advertised now requires both advertisements without advancing time.
  • just rs uring: pass. 333 passed, 2 skipped, plus 78 passed.
  • quest check --base origin/release: pass.

Review

Codex, gpt-6.1-sol, medium effort, round 1 (quest review --base origin/release). Claude Code wrote this change, so Codex is the other vendor. Reviewed head: 55000adf43cf6a4b3ab10639e0c36c1c609e286a. No findings. The current head 0fcb088cb9 adds only a conflict-free merge of origin/release, which leaves the PR's diff identical, so it needs no new review.

Follow-ups

None identified. This PR belongs in the already planned combined patch release; the maintainer chooses versions and cuts it separately.

(Written by Claude Opus 5.5)

🤖 Generated with Claude Code

Backport of #7414's behavior. The unsolicited announce loop awaited each
PUBLISH_NAMESPACE and REQUEST_UPDATE write, so a peer withholding send
credit pinned it, along with every withdrawal queued behind. Buffer the
message instead and write and read it from the loop's own event wait:
withdrawing a request still writing or awaiting its answer resets it in
both directions, and the answer deadline starts once the message is
written. Hold an early REQUEST_OK until the write completes.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@kixelated kixelated left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review by review (OpenAI)

Reviewed commit: 55000ad

No actionable introduced bugs found. Buffering and driving writes from the namespace event loop is a reasonable backport of #7414; the distinct writing and answer states and cancellation handling appear justified by the stalled-credit cases. The change keeps the release API rather than introducing a larger redesign.

Static source review only; I did not run tests. GitHub Check and Release gate were queued when inspected.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes

I read the complete diff and traced the new per-request announce machinery end to end, focusing on the waker/deadline ordering and on whether the recorded announce state can diverge from what the peer actually holds. The release base's peer-FIN/reset-ends-the-loop behavior and the drafts 14-16 control-stream path both hold up.

  • Buffered announcements — advertise_namespace/update_namespace now buffer their message and record Pending::Writing instead of awaiting the write and the answer, so a stalled namespace no longer pins the loop or the withdrawals behind it.
  • Per-request polling — Woken/RequestWaker/poll_answers/poll_request flush and read each woken request on its own stream, with the answer read even while the write is stalled; Framed preserves the [type][size][body] framing and the existing stats accounting.
  • Answer deadline after the write — Pending::Answer(at) is only set once poll_flush completes, tracked in inflight and enforced by the late deadline; backpressure alone never times a request out.
  • One message in flight — the sync_namespace early return coalesces route changes behind the in-flight message, an early RequestOk is held until the write flushes, and answered applies OK/refusal/Late by re-syncing, terminating gracefully, or resetting both directions.
  • Withdrawal — drafts 17+ reset both directions when a message is in flight; drafts 14-16 keep PUBLISH_NAMESPACE_DONE because their virtual writes never wait for credit.
  • Tests — ScriptedSession::with_send_window plus woken scripted readers, the staged withdrawal/refusal/FIN/reset matrix over drafts 17-22, the write-only deadline, and the early-OK hold.

Pullfrog  | View workflow run | Using deepseek-v4.1-flash-1017 (free via Pullfrog for OSS) | 𝕏

@kixelated
kixelated enabled auto-merge November 5, 2026 06:44
@kixelated
kixelated merged commit d6ffbbb into release Nov 5, 2026
34 of 36 checks passed
@kixelated
kixelated deleted the backport/withdraw-without-credit branch November 5, 2026 07:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant