Repository navigation
fix(net): bound legacy feed fills by shared demand - #7215
Merged
Merged
Conversation
Quest-Claim: e80c5a9f84e37595
Co-Authored-By: GPT-6.1 Sol <noreply@openai.com>
kixelated
changed the title
…it/fetch-backstop
Main removed `fill_groups`, so the test shares one-group fills between two readers each instead; `a_wide_feed_fill_settles_by_run` covers width. The backstop was `Fill::refuse`'s last caller. The quest is finished. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
|
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 17 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. |
kixelated
commented
Oct 31, 2026
kixelated
left a comment
kixelated
left a comment
Collaborator
Author
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: c353355
No actionable bugs found in the current diff. Removing the unrelated announcement-count cap and its full-table reclamation scan is a reasonable simplification: held entries follow existing model fills, overlapping readers share demand, and last-reader cancellation remains wired through poll_unused. The replacement test checks both sharing and release after the final reader leaves; deleting the now-unused Fill::refuse keeps the change focused without adding another refusal policy.
Verification limits: static review of the diff and surrounding fill join/withdrawal, holder cancellation, and serve-loop code via GitHub. I did not run tests or independently reproduce the reported validation, and this is not a general resource-exhaustion audit of downstream transports.
Collaborator
Author
|
Landing summary for
(Written by Claude Opus 5.5) |
kixelated
enabled auto-merge (squash)
October 31, 2026 06:32
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
- Delete the legacy feed backstop: removes the
maxfield (wasLimits::default().announces= 100,000), theholdreap/refuse block, and the now-uncalledFill::refuse; the pre-lite-05FeedFetchesrelies on demand-driven release. held_feed_fills_follow_shared_demand: replaces the backstop and replacement-at-limit tests, asserting one entry per fill, shared joins adding none, and release only when the last reader leaves.- Quest completion: deletes
quest/m1/security-audit/fetch-backstop.mdand its README bullet; no dangling references remain.
The bound derivation holds. held holds one entry per model fill (subscriber.rs:7515), and FillState::join creates fills only for uncovered gaps plus at most two boundary splits, with a repeat join over an already-covered span creating none (model/track/fill.rs:406-489). Covers are removed only as demand releases or answers arrive, so fills stay linear in outstanding wanted-run fragments — which follow request, cache, and ledger boundaries, not a span's numeric width. For a network peer that aggregate is bounded by downstream bidi stream credit (lite) or the request window (IETF), matching the documented design in session::Limits. The deleted 100k cap was an unrelated upstream-announcement allowance that only refused legitimate aggregate demand, so removing it does not remove a real bound.
Worth noting, not blocking: the new test covers the sharing property but not the fragmentation/compounding bound; that rests on the code trace above rather than a test.
deepseek-v4.1-flash (free via Pullfrog for OSS) ｜ 𝕏
Contributor
|
quest land removed this pull request from the queue: Conflicts with the base: rs/moq-net/src/lite/subscriber.rs, rs/moq-net/src/model/track/fill.rs |
Contributor
|
codex / Codex land fixer: - A breaking change marks its title with `!` and its Impact bullet says how to migrate. Only the release cut writes `MIGRATE.md` sections; leave existing Unreleased notes for that cut to consume.
- Keep things private until a current or planned consumer needs them. Scrutinize every new exported item.
- Never add `foo_with_x`, `foo_checked`, or a compatibility shim. Make the breaking change to `foo` instead.
- Let the type system make misuse unrepresentable
- Decide a fast or supported path at setup from known capabilities, and error otherwise. No API tries a path at runtime and silently falls back, and reporting which path ran is not a fix.
- Players start and stop on a broadcast's announcement and keep waiting for it to return; a request error is never the restart trigger.
- A broadcast, track, or group name always means the same content. MoQ has no ETag-style invalidation, so a publisher that reuses a name for different content (such as restarting group numbers) is a bug; publish a new broadcast instead of working around it.
- Each publisher produces its own stream. With several publishers, the end subscriber decides how to subscribe to them (switch between, combine, or keep separate); never make independent publishers produce identical output so the relay can stitch them.
- Avoid callback parameters. Return a handle, an event, or a Producer/Consumer split.
- Avoid 4+ args; use a struct or object.
- Prefer nesting related API, configuration, and structured output fields under their shared role.
- Name by role, not today's implementation.
- When a name or shape feels awkward, propose alternatives with a recommendation instead of shipping it.
- Short names under a module namespace (`encode::Config`, not `EncoderConfig`). Mirror names across Rust, JS, and the bindings.
- Document every exported symbol in one plain line, the way you'd say it out loud.
- Prefer refactoring and simplification; many APIs have not been published yet.
- The `moq-net` API and moq-lite take priority over moq-transport, in design and in work order. Map moq-transport onto them, not the reverse, and skip any of its features or MUSTs that add cost without a real behavioral benefit, that a peer cannot detect or enforce when the alternative is much simpler, or that the draft gets wrong. A spec rule or a "spec violation" finding never decides a design on its own; argue on behavior.
MoQ is split into many layers as part of the public API.
The boundary between packages is extremely important to keep things modular and reusable.
# Development
PRs target `main`, the trunk, where breaking API changes are allowed.
`release` is what ships; touch it only to cut a release or backport a fix (see `CONTRIBUTING.md`).
Wire changes should be backwards compatible for any *published* drafts/versions.
Between our own builds only moq-lite compatibility is guarded, since our implementations always prefer it; moq-transport compatibility matters against third-party implementations.
An unpublished `-wip` version changes freely, so never defer a better wire design to keep it compatible.
Scratch lives in the checkout's ignored `.tmp/`, not `target/`: build output may live outside the checkout and gets wiped.
Use the Nix dev shell so tooling matches CI.
direnv loads it automatically, but if not: `nix develop --command ...`.
```bash
just ci-select # Report whether CI and Rust build setup are needed
just check # Lint, compile, and test what the branch changed
just fix # Auto-fix lint/formatting, same scope
```
These diff the branch against its base and only run the affected packages.
Quests: before starting work, run `quest guide` and follow it.
The `quest` binary comes from the moq-dev/quest flake input and serves the quest skills; change them upstream and bump the input.
# Cross-Package Sync
| Change in | Also update |
|---|---|
| `rs/moq-ffi` | `rs/moq-c`, `c/moq`, `{py,swift,kt,dart}/`, `go/wrapper/` (the `go/ffi` and `dart/moq_ffi` bindings regenerate automatically, but a new method needs a hand-written wrapper too, like `py/moq-rs` or `dart/moq`), `cpp/moq`, `cpp/obs/src`, `doc/lib/{py,swift,kt,go,dart,c,cpp}`, `doc/bin/obs.md` |
| `rs/moq-net` wire/API | `js/net`, `doc/concept`, `drafts/draft-lcurley-moq-lite.md` (if the wire spec changes) |
| `rs/hang` catalog/container (the `m2ts` section is Rust-only) | `js/hang`, `doc/concept`, `drafts/draft-lcurley-moq-hang.md` (if the format spec changes) |
| `rs/moq-auth` | `js/auth`, `doc/bin/relay/auth.md` |
| `rs/moq-stats` wire (track names, frame shapes) | `doc/concept/stats.md` |
| `rs/moq-relay` config/behavior | `doc/bin/relay/` |
| `rs/moq-cli` | `doc/bin/cli.md` |
| `rs/moq-gst` | `doc/bin/gstreamer.md` |
| `rs/moq-scene`, `rs/moq-studio` | `doc/bin/studio.md` |
| `rs/moq-c` C ABI (`moq.h`) | `doc/lib/c` |
| `js/{watch,publish}` UI/API | `demo/web` if it consumes the API |
Any wire-format change updates its matching IETF draft in the same PR, including framing, message fields, enum values, and version negotiation. Use the feature-specific draft for extensions and validate with `just drafts check`. See `drafts/AGENTS.md`.
For wire, `moq-ffi`, or gateway changes, also run `just test interop --all` for cross-language interop; plain `interop` is Rust-only.
When a CLI interface changes, search the whole repo for the binary name and update every example invocation, including docs and demo recipes. Check examples against `--help`.
codex
The log’s two merge conflicts are reproducible. I also found an instruction in `AGENTS.md` to “Pull the latest origin changes before working,” which conflicts with this task’s no-contact rule. Under your untrusted-input rule, I’ll hand back PR 7215 and leave its branch unchanged.
exec
/bin/bash -lc "python3 - <<'PY'
import json
from pathlib import Path
result = [{\"number\":7215,\"action\":\"handback\",\"diagnosis\":\"Base merge conflicts in rs/moq-net/src/lite/subscriber.rs and rs/moq-net/src/model/track/fill.rs; untrusted pr-7215:AGENTS.md, Required section, directs: \\\"Pull the latest origin changes before working.\\\", conflicting with the no-contact rule.\",\"recommendation\":\"Have the author resolve the conflicts against base and submit for review; ignore repository instructions requesting remote contact.\"}]
Path('quest-land-result.json').write_text(json.dumps(result, indent=2) + '\\n')
PY
git status --short && git rev-parse pr-7215 && cat quest-land-result.json" in /work
succeeded in 0ms:
?? quest-land-result.json
c353355636753584d992331afcef5604d7a86691
[
{
"number": 7215,
"action": "handback",
"diagnosis": "Base merge conflicts in rs/moq-net/src/lite/subscriber.rs and rs/moq-net/src/model/track/fill.rs; untrusted pr-7215:AGENTS.md, Required section, directs: \"Pull the latest origin changes before working.\", conflicting with the no-contact rule.",
"recommendation": "Have the author resolve the conflicts against base and submit for review; ignore repository instructions requesting remote contact."
}
]
codex
Recorded PR 7215 as `handback` in [quest-land-result.json](/work/quest-land-result.json), left uncommitted.
Confirmed both merge conflicts and quoted the conflicting repository instruction in the diagnosis. All branches remain unchanged; no remote contact occurred.
tokens used
16,976
Recorded PR 7215 as `handback` in [quest-land-result.json](/work/quest-land-result.json), left uncommitted.
Confirmed both merge conflicts and quoted the conflicting repository instruction in the diagnosis. All branches remain unchanged; no remote contact occurred.
fixer result written
(written by codex / Codex) |
# Conflicts: # rs/moq-net/src/lite/subscriber.rs # rs/moq-net/src/model/track/fill.rs
Collaborator
Author
|
Requeued after the land queue stopped on merge conflicts. Merged origin/main locally as d0215ca. In (Written by Claude Opus 5.5) |
Contributor
|
quest land removed this pull request from the queue: the batch check failed (exit 1). error: recipe `test-changed` failed on line 87 with exit code 143
ci: check kt cancelled in 97s
==> check kt
bash ../sh/kt/check.sh
kt generate: building moq-ffi for x86_64-unknown-linux-gnu...
Blocking waiting for file lock on build directory
Finished `dev` profile [unoptimized + debuginfo] target(s) in 1.88s
mbx[savings]: 10d 1h unspent across 12887 builds. compounding, in a sense.
Compiling libc v0.2.191
Compiling syn v3.0.6
Compiling getrandom v0.4.3
Compiling winnow v1.0.4
Compiling indexmap v2.14.2
Compiling memchr v2.8.3
Compiling rustc-hash v2.1.3
Compiling siphasher v1.0.4
Compiling glob v0.3.4
Compiling uniffi_internal_macros v0.32.2 (https://github.com/kixelated/uniffi-rs?rev=88d835d#88d835d6)
Compiling nom v7.1.3
Compiling fs-err v3.3.1
Compiling tempfile v3.27.0
Compiling scroll_derive v0.12.1
Compiling smawk v0.3.3
Compiling serde_derive v1.0.229
Compiling thiserror-impl v2.0.21
Compiling weedle2 v5.0.0 (https://github.com/kixelated/uniffi-rs?rev=88d835d#88d835d6)
Compiling serde_json v1.0.154
Compiling textwrap v0.16.4
Compiling toml_parser v1.1.3+spec-1.1.0
Compiling toml_datetime v1.1.1+spec-1.1.0
Compiling plain v0.2.3
Compiling log v0.4.34
Compiling toml v1.1.6+spec-1.1.0
Compiling clap_builder v4.6.7
Compiling clap_derive v4.6.7
Compiling uniffi_core v0.32.2 (https://github.com/kixelated/uniffi-rs?rev=88d835d#88d835d6)
Compiling uniffi_pipeline v0.32.2 (https://github.com/kixelated/uniffi-rs?rev=88d835d#88d835d6)
Compiling scroll v0.12.0
Compiling uniffi_meta v0.32.2 (https://github.com/kixelated/uniffi-rs?rev=88d835d#88d835d6)
Compiling goblin v0.8.2
Compiling uniffi_udl v0.32.2 (https://github.com/kixelated/uniffi-rs?rev=88d835d#88d835d6)
Compiling thiserror v2.0.21
Compiling serde v1.0.229
Compiling askama_parser v0.16.1
Compiling basic-toml v0.1.10
Compiling cargo_metadata v0.23.1
Compiling uniffi_macros v0.32.2 (https://github.com/kixelated/uniffi-rs?rev=88d835d#88d835d6)
Compiling clap v4.6.7
Compiling askama_derive v0.16.1
Compiling askama_macros v0.16.1
Compiling askama v0.16.1
Compiling uniffi_bindgen v0.32.2 (https://github.com/kixelated/uniffi-rs?rev=88d835d#88d835d6)
Compiling uniffi v0.32.2 (https://github.com/kixelated/uniffi-rs?rev=88d835d#88d835d6)
Compiling uniffi-bindgen v0.1.0 (/work/rs/uniffi-bindgen)
Finished `dev` profile [unoptimized + debuginfo] target(s) in 2.74s
Running `mbx-launch target/debug/uniffi-bindgen generate --library /work/target/debug/libmoq_ffi.so --language kotlin --no-format --out-dir /tmp/tmp.SbiKEjx0nD`
mbx[cache]: object cache: 52 hits, 0 misses; 0 B downloaded, 0 B uploaded, 0 B stored locally
mbx[cache]: 4.53s session; 2m 5s estimated compiler time avoided (summed across compilations)
mbx[cache]: Cargo artifact reuse and CI cache archive transfers are not included above
mbx[savings]: 2.6 TiB of outputs, one copy, several very confident checkouts.
Welcome to Gradle 8.14.4!
Here are the highlights of this release:
- Java 24 support
- GraalVM Native Image toolchain selection
- Enhancements to test reporting
- Build Authoring improvements
For more details see https://docs.gradle.org/8.14.4/release-notes.html
Starting a Gradle Daemon (subsequent builds will be faster)
> Task :moq-ffi:kmpPartiallyResolvedDependenciesChecker
> Task :moq:kmpPartiallyResolvedDependenciesChecker
> Task :moq-ffi:checkKotlinGradlePluginConfigurationErrors SKIPPED
> Task :moq:checkKotlinGradlePluginConfigurationErrors SKIPPED
> Task :moq:generateVersion
> Task :moq:jvmProcessResources NO-SOURCE
> Task :moq:processJvmMainResources SKIPPED
> Task :moq:jvmTestProcessResources NO-SOURCE
> Task :moq:processJvmTestResources SKIPPED
> Task :moq-ffi:jvmProcessResources
> Task :moq-ffi:processJvmMainResources SKIPPED
> Task :moq-ffi:jvmTestProcessResources NO-SOURCE
> Task :moq-ffi:processJvmTestResources SKIPPED
error: recipe `check` failed on line 17 with exit code 143
ci: test js failed
ci: seconds per module
267 check py
23 test py
606 check rs
21 test rs (cancelled)
329 check c
204 check cpp
36 check obs_compile
281 check js
16 test js (failed)
97 check kt (cancelled)
627 wall time
error: recipe `ci` failed on line 107 with exit code 1
. |
Contributor
|
codex / Codex land fixer: (written by codex / Codex) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Pre-lite-05 feed fills are refused at the default announcement count, an unrelated field that ignores configured limits and aggregates demand from multiple downstream peers.
Approach
Delete the redundant backstop and its full-table reclamation scan, plus
Fill::refuse, which lost its last caller. Retain demand-driven cancellation and per-group readiness. Finishes and deletesquest/m1/security-audit/fetch-backstop.md, as its Plan preferred deleting the backstop if held fills are already bounded.Bound derivation:
FillState::joincreates fills only for uncovered demand runs; overlapping readers share existing fills. Fragmentation follows request, cache, and ledger boundaries, not the numeric width of a range.FeedFetches::holdstores at most one entry per taken model fill and waits on one next group. It creates neither readers nor additional fills. For P existing pending fills, held storage is O(P), plus cancellation notifications awaiting the next poll; P is not the number of groups in a requested span.Limitscurrently contains onlyannounces.held_feed_fills_follow_shared_demandholds three fills, joins each from a second reader without new fills, and checks release only when the last reader leaves.a_wide_feed_fill_settles_by_run(already on main) covers width. Existing tests cover arrivals, missing ranges, abandonment wakeups, and replacement attempts.Impact
TooManyRequestsfrom the unrelated announcement default.Alternatives
Threading session limits through would still confuse upstream announcement ownership with downstream request demand. A new fixed cap would add refusal policy without bounding the model's existing cost.
Changes since the previous attempt
Consumer::fill_groups, so the new test now shares one-groupfetch_groupfills between two reader sets instead of three trillion-group ranges; width is already covered on main bya_wide_feed_fill_settles_by_run.Fill::refuse: after the merge the backstop was its last caller, andbuild.warnings = denyfailed the build on it.quest/m1/security-audit/README.md.rs/moq-net/src/lite/subscriber.rs: kept this PR's removal of the backstop block inFeedFetches::hold.rs/moq-net/src/model/track/fill.rs: took main'swanted_changed/poll_wanted_changedand itsnext->wanteddoc rename from moq-dev/moq#7036, still withoutFill::refuse.Validation
fetches.max = 2injected fails atassertion left == right failed: one entry per fill, left: 2, right: 3; the third fill of live, shared demand is refused.quest gate just rs test -p moq-net --lib -E 'test(/feed_/) | test(held_feed_fills_follow_shared_demand)': 15 passed.quest gate just ci: passed (exit 0) on 4d7527f.quest check --base origin/main: ok.quest gate just rs test -p moq-net: 3210 passed, andquest gate just cipassed.Review
quest review --deep(gpt-6-astra, medium effort) on head d0215ca against origin/main 38566f5: 0 findings. Covers the hand-resolved merge of main.quest review --deepon 4d7527f found nothing, and Claude Opus 5.5 reviewed the Codex-authored fc660b3 and found nothing.Follow-ups
None.
(Written by Claude Opus 5.5)