Repository navigation
test(parallel): pin cancel-and-drain against the join_all panic - #11532
Conversation
📝 WalkthroughWalkthroughThe change adds asynchronous tests for ChangesParallel task behavior
Estimated code review effort: 2 (Simple) | ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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 |
Greptile SummaryAdds regression tests covering cancellation of unfinished parallel tasks after an error and preservation of input order in parallel results. Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains. Important Files Changed
Reviews (2): Last reviewed commit: "test(parallel): pin cancel-and-drain aga..." | Re-trigger Greptile |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/parallel.rs`:
- Around line 67-89: Ensure the tests around
a_failure_aborts_running_siblings_without_panicking and the related ordering
test run with a concurrency budget of at least two, rather than relying on the
global Settings::get().jobs value. Configure the test settings or inject the
limit so both sibling tasks can be spawned concurrently while preserving the
existing abort and ordering assertions.
- Around line 111-115: Update the completion-order setup in the parallel test
around the parallel invocation to use an explicit synchronization handshake,
such as an atomic flag or notification: have task 1 signal that it has started
or reached the required point, then let task 0 proceed only after observing that
signal. Remove reliance on the fixed 50 ms sleep while preserving the intended
order where item-1 completes before item-0.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 78d85c04-4bc9-4a59-aedf-43ad955c8965
📒 Files selected for processing (1)
src/parallel.rs
`parallel()` aborts its `JoinSet` when a task fails and then drains with `join_next()`, because `join_all()` panics on any task that was cancelled rather than completed (jdx#7280). Nothing pinned that behaviour: the only test was a happy path where no task fails, so restoring `join_all()` would keep CI green while reintroducing the `task N was cancelled` panic reported in jdx#5263, jdx#5369 and jdx#5414. Two tests, no behaviour change: - `a_failure_aborts_unfinished_siblings_without_panicking` — asserts an `Err` is returned rather than a panic, and that the sibling was cancelled rather than allowed to finish. - `results_follow_input_order_not_completion_order` — results are indexed by input position, not by completion order. `src/backend/conda.rs:423` zips its input vector against this function's output and is only correct while that holds; the existing test cannot catch a regression there, since `x * 2` over a sorted input looks the same either way. Neither test depends on the `jobs` setting, which can legitimately be 1: `raw = true` forces exactly that in `Settings::try_get`. Because a permit is taken before `spawn`, a task that waits unconditionally on a sibling would stop that sibling from ever being spawned, so the waits are bounded and the failing task is spawned first. A sibling cancelled before its first poll is still a cancelled task at drain time, which is the condition `join_all()` panics on, so the property holds at any budget. Shared state lives in statics because `F` is bound by `Copy`, which a closure capturing an `Arc` would not satisfy.
6a56c9b to
3714dec
Compare
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
Test-only.
src/parallel.rsaborts itsJoinSetwhen a task fails and then drains withjoin_next(), with a comment saying exactly why:That is the fix from #7280. Nothing pins it. The file's only test is a happy path where no task fails, so anyone who "simplifies" the drain back to
join_all()reintroduces thetask N was cancelledpanic — reported three separate times in #5263, #5369 and #5414 — and CI stays green.The tests
a_failure_aborts_unfinished_siblings_without_panicking— two inputs. Item 0 fails; item 1 parks for 10s, far longer than the test takes, so it is unfinished when the drain reaches it.JoinSet::join_all()panics on any task that was cancelled rather than completed, so that is the whole precondition — the sibling does not have to have been polled. Asserts:Errreturned rather than a panic, and the sibling did not run to completion, i.e. it was cancelled.results_follow_input_order_not_completion_order—parallelpre-fillsVec<Option<U>>and writes by theenumerateindex, andsrc/backend/conda.rs:423zips its input vector against this function's output vector, which is correct only while that holds. The existing test can't detect a regression:x * 2over1..=5is monotonic, so completion order and input order coincide. Here item 1 signals completion and item 0 does not return until it observes that, so item 0 finishes last and the output is still in input order.Why neither test depends on
jobsA permit is taken before
spawn, so a task that waits unconditionally on a sibling stops that sibling from ever being spawned. That matters becausejobs == 1is reachable —Settings::try_getforces it whenraw = true:So: the failing task is item 0, spawned first, never starved of a permit by the sibling; and both handshakes go through a
yield_untilhelper that gives up after a bounded number of turns. Atjobs == 1the first task completes, releases its permit, the second is spawned, andabort_all()cancels it — the same drain path, no hang. At any larger budget both are in flight and the handshake decides the interleaving. Giving up early only weakens the interleaving; it never changes what is asserted.Shared state is
static AtomicBoolrather than a captured handle becauseF: Fn(T) -> Fut + Send + Copy + 'static—Copyrules out a closure holding anArc, which is how the rest of the repo sequences async tests.What I can and cannot claim
I can't demonstrate the test failing against the pre-#7280 code, since that would mean reverting the fix. The argument is structural:
join_all()panics on a cancelled task, and the test guarantees an unfinished-then-cancelled task exists at drain time.Known gap, deliberately left out: the
Err(e) => e.into()JoinErrorarm, which needs a panicking task. There is prior art insrc/cli/plugins/mod.rs, but it dumps a panic backtrace into test output for a branch no bug report has ever pointed at. Happy to add it if you'd rather have the coverage.No local
cargorun —rustfmt --edition 2024only.lint, linuxcargo test,unit-macosandwindows-unitcover compile and behaviour on all three platforms.Summary by CodeRabbit