diff --git a/justfile b/justfile index af3866c399..bbba70cb92 100644 --- a/justfile +++ b/justfile @@ -544,6 +544,7 @@ _check $BASE $TEST: just rs media-features just --justfile bench/justfile check quest check + just test drill-sensitivity --apply-only # Not covered by the line above: moq-wasm only exists on the wasm32 target. just rs wasm just py check @@ -570,6 +571,12 @@ _check $BASE $TEST: if echo "$files" | grep -qE '^(quest/|flake\.lock$)'; then quest check fi + # The drill mutations patch Rust source, so a Rust change can move the + # code they target. Only nightly runs the drills against them; this + # catches a stale patch in the PR that moved its code. + if echo "$files" | grep -qE '^(rs/|test/drill/)'; then + just test drill-sensitivity --apply-only + fi just py check "$files" just kt check "$files" just swift check "$files" diff --git a/quest/m1/README.md b/quest/m1/README.md index 674dbee9ae..6e32afe71c 100644 --- a/quest/m1/README.md +++ b/quest/m1/README.md @@ -17,7 +17,6 @@ transport, benchmark tooling); worktrees isolate commits, not semantics. ## Required -- [Drill sensitivity](/quest/m1/drill-sensitivity.md) - the nightly drill-sensitivity job passes: the subscriber-leaks-broadcasts mutation applies to the current lite subscriber again - [Cluster routing](/quest/m1/cluster-routing.md) - an announcement says where a broadcast originates, not how to reach it, and a relay hears only the prefixes its clients asked for - [lite-07 count settle](/quest/m1/lite-count-settle.md) - moq-lite-07 subscribers stop waiting for a subscription's tail once SUBSCRIBE_END's stream count is reached - [Dropped sources](/quest/m1/dropped-sources.md) - track consumers see the producer's real error on every end path, never `Dropped` diff --git a/quest/m1/drill-sensitivity.md b/quest/m1/drill-sensitivity.md deleted file mode 100644 index 5f2232c610..0000000000 --- a/quest/m1/drill-sensitivity.md +++ /dev/null @@ -1,22 +0,0 @@ -# [XS] The subscriber-leaks-broadcasts mutation applies again - -## Goal - -The nightly `Tests (test drill-sensitivity)` job passes. It fails on -[run 36240326747](https://github.com/moq-dev/moq/actions/runs/36240326747) -because `test/drill/mutations/subscriber-leaks-broadcasts.patch` no longer -applies ("1 out of 2 hunks FAILED" on `rs/moq-net/src/lite/subscriber.rs`) -after a later change to the lite subscriber, so the -`cancel_under_backpressure_releases_the_reader` drill proves nothing. - -## Plan - -- Retarget the patch the way - [#3953](https://github.com/moq-dev/moq/pull/3953) did: find where the lite - subscriber now releases the broadcasts a session fed when it ends, and - remove that behavior again. Keep the failure message the patch declares, or - update it if the drill now fails with a different but still correct one. -- If the release moved somewhere a patch cannot remove cleanly, the drill may - be pointing at the wrong layer; say so rather than force a patch. -- Prove it with `just test drill-sensitivity subscriber-leaks-broadcasts`, and - run the other two mutations to confirm they still apply. diff --git a/test/drill/README.md b/test/drill/README.md index db075b8753..bf3600f75d 100644 --- a/test/drill/README.md +++ b/test/drill/README.md @@ -112,7 +112,9 @@ protocol's reaction to loss and delay, not the kernel's rendering of them. ## Sensitivity The Nightly workflow runs all mutations, so patches that stop applying and drills -that stop detecting their recovery failures fail CI. +that stop detecting their recovery failures fail CI. `just check` also runs +`--apply-only` whenever Rust or this directory changes, so a patch that no longer +applies fails the PR that moved its code rather than the next nightly. `sensitivity.sh` removes one recovery behavior at a time and requires the drill covering it to fail. Each mutation is a patch under `mutations/`, applied to a diff --git a/test/drill/mutations/relay-withdraws-lost-publisher.patch b/test/drill/mutations/relay-withdraws-lost-publisher.patch index 1f30d190ee..47ff59c45d 100644 --- a/test/drill/mutations/relay-withdraws-lost-publisher.patch +++ b/test/drill/mutations/relay-withdraws-lost-publisher.patch @@ -6,25 +6,25 @@ # means a route outlives the session that announced it, so a crashed publisher's # name stays announced with nothing behind it. diff --git a/rs/moq-net/src/lite/subscriber.rs b/rs/moq-net/src/lite/subscriber.rs -index 729a23e4b..dac9990c6 100644 +index 8a0259fec..0b6035640 100644 --- a/rs/moq-net/src/lite/subscriber.rs +++ b/rs/moq-net/src/lite/subscriber.rs -@@ -2566,7 +2566,8 @@ struct AnnouncedRoute { +@@ -2842,7 +2842,8 @@ struct AnnouncedRoute { /// without recomputing the chain. route: crate::origin::Route, /// Dropping it retracts the route and rejects its queued requests. - dynamic: crate::origin::Dynamic, + // MUTATION: never dropped, so a route outlives the session that announced it. + dynamic: std::mem::ManuallyDrop, - /// One minted source per requested path, finished on a clean retraction and - /// aborted (via drop) when the session dies. + /// One minted source per requested path, each closed when its guard drops. sources: HashMap, -@@ -2578,7 +2579,7 @@ impl AnnouncedRoute { - fn new(route: crate::origin::Route, dynamic: crate::origin::Dynamic) -> Self { + /// Whether the GOAWAY drain already re-priced this route. +@@ -2858,7 +2859,7 @@ impl AnnouncedRoute { + fn new(route: crate::origin::Route, dynamic: crate::origin::Dynamic, wake: Arc) -> Self { Self { route, - dynamic, + dynamic: std::mem::ManuallyDrop::new(dynamic), sources: HashMap::new(), drained: false, - } + waker: std::task::Waker::from(wake.clone()), diff --git a/test/drill/mutations/subscriber-leaks-broadcasts.patch b/test/drill/mutations/subscriber-leaks-broadcasts.patch index 44bff5a4d2..328b418eb7 100644 --- a/test/drill/mutations/subscriber-leaks-broadcasts.patch +++ b/test/drill/mutations/subscriber-leaks-broadcasts.patch @@ -3,14 +3,14 @@ # # Removes the release a subscribing session performs when it ends: each announce # stream's `Announced` collection owns the routes and source guards for every -# broadcast the session fed, and dropping it aborts them. Wrapping it in -# `ManuallyDrop` keeps every handle downstream of it alive after the session is -# gone, so a cancelled reader's broadcast parks forever instead of closing. +# broadcast the session fed, and dropping it retracts and closes them. Wrapping +# it in `ManuallyDrop` keeps every handle downstream of it alive after the session +# is gone, so a cancelled reader's broadcast parks forever instead of closing. diff --git a/rs/moq-net/src/lite/subscriber.rs b/rs/moq-net/src/lite/subscriber.rs -index 729a23e4b..54aa7a5be 100644 +index 8a0259fec..92febbcdf 100644 --- a/rs/moq-net/src/lite/subscriber.rs +++ b/rs/moq-net/src/lite/subscriber.rs -@@ -1087,7 +1087,8 @@ struct PrefixRun { +@@ -1142,7 +1142,8 @@ struct PrefixRun { /// it comes from the connect config or the peer's SETUP, neither of which /// changes for the life of the session. link_cost: u64, @@ -19,13 +19,11 @@ index 729a23e4b..54aa7a5be 100644 + announced: std::mem::ManuallyDrop, // Lite06+: announce ids. Each received `active` implicitly assigns the next // per-stream ordinal; `ended`/`restart` reference it instead of repeating the - // path. Tracked even for announces we drop locally (reflected loops), since -@@ -1175,7 +1176,7 @@ impl AnnouncePrefix { - let run = PrefixRun { + // path, and lite-07 bases name it too. Tracked even for announces we drop +@@ -1233,5 +1234,5 @@ impl AnnouncePrefix { responder_origin, link_cost, - announced: Announced::default(), + announced: std::mem::ManuallyDrop::new(Announced::default()), - next_announce_id: 0, - announced_by_id: HashMap::new(), + decoder: lite::AnnounceDecoder::default(), }; diff --git a/test/drill/sensitivity.sh b/test/drill/sensitivity.sh index ba4c19d7cf..92ec1b03f9 100755 --- a/test/drill/sensitivity.sh +++ b/test/drill/sensitivity.sh @@ -32,6 +32,7 @@ CARGO=cargo KEEP=0 BASELINE=1 +APPLY_ONLY=0 SELECTED=() dir= log= @@ -68,6 +69,7 @@ Options: --list list the mutations and the drill each one must break --keep keep the mutated snapshots (prints each path) --no-baseline skip the unmutated run of each drill + --apply-only only check that each mutation still applies; builds nothing -h, --help this With no mutation named, every mutation runs. @@ -88,6 +90,10 @@ while [[ $# -gt 0 ]]; do BASELINE=0 shift ;; + --apply-only) + APPLY_ONLY=1 + shift + ;; -h | --help) usage exit 0 @@ -157,6 +163,14 @@ for patch in "$MUTATIONS"/*.patch; do checked=$((checked + 1)) echo "=== $name -> $drill" + if [[ $APPLY_ONLY -eq 1 ]]; then + if ! patch -p1 -d "$WORKSPACE" --dry-run --batch --forward --silent <"$patch"; then + echo " FAIL: '$name' does not apply to this tree" >&2 + failed=$((failed + 1)) + fi + continue + fi + if [[ $BASELINE -eq 1 ]]; then log=$(mktemp "${TMPDIR:-/tmp}/drill-baseline.XXXXXX") status=$(run_drill "$WORKSPACE" "$drill" "$log") @@ -221,6 +235,15 @@ if [[ $checked -eq 0 ]]; then exit 2 fi +if [[ $APPLY_ONLY -eq 1 ]]; then + if [[ $failed -gt 0 ]]; then + echo "$failed of $checked mutations do not apply; retarget them at the current code" >&2 + exit 1 + fi + echo "$checked of $checked mutations apply" + exit 0 +fi + if [[ $failed -gt 0 ]]; then echo "$failed of $checked mutations did not prove sensitivity" >&2 exit 1