7991: fix: `Chan::recv_many` must not make assumptions about buffer contents by martin-augment · Pull Request #86 · martin-augment/tokio

martin-augment · 2026-03-29T20:19:12Z

7991: To review by AI

`Chan::recv_many` intends to assert that no slots have been consumed when exiting with `Ready` via the `rx_closed` code path. Instead of asserting no items were added to the buffer, it asserted buffer emptiness, incorrectly making assumptions about the provided buffer. When `recv_many` was called on an empty channel with idle semaphore after the receiver was closed, the method would panic. The branch coverage had been previously missing; the preceding commit added the tests as proof of failure. This changeset corrects the assertion and updates the tests covering the code path. Fixes tokio-rs#7990.

coderabbitai · 2026-03-29T20:19:28Z

Walkthrough

A fix was applied to the Rx::recv_many method in tokio/src/sync/mpsc/chan.rs. The assertion that validates the state when a receiver is closed and the channel semaphore is idle was modified from requiring an empty buffer to requiring the buffer length to match its initial length. This adjustment accounts for scenarios where the buffer may already contain data before the method is called. Two new regression tests were added to tokio/tests/sync_mpsc.rs to verify the method handles non-empty buffers correctly when the receiver is closed and the channel is idle.

✨ Finishing Touches

📝 Generate docstrings

Create stacked PR
Commit on current branch

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch pr-7991-2026-03-29-20-19-09

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

gemini-code-assist

Code Review

This pull request updates the recv_many method in the MPSC implementation to allow calls with a non-empty buffer when the receiver is closed and the channel is idle, preventing a panic. The assertion was updated to ensure the buffer length matches the initial length instead of requiring it to be empty. New tests were added for both bounded and unbounded channels to verify this behavior. The review feedback suggests enhancing these tests by asserting that the method returns zero and the buffer remains unchanged.

gemini-code-assist · 2026-03-29T20:20:46Z

tokio/tests/sync_mpsc.rs

+    rx.close();
+
+    // Does not panic, no longer expecting an empty buffer (#7990).
+    rx.recv_many(&mut buffer, 1).await;


This test correctly verifies that recv_many no longer panics when called with a non-empty buffer on a closed channel. To make the test more comprehensive, you could also assert that recv_many returns 0 and that the buffer's content remains unchanged.

let n = rx.recv_many(&mut buffer, 1).await; assert_eq!(n, 0); assert_eq!(buffer, &[1]);

value:useful; category:bug; feedback: The Gemini AI reviewer is correct! The test just (implicitly) asserts that there is no panic. It would be better to also assert the returned value of the receive operation and the contents of the buffer after it.

gemini-code-assist · 2026-03-29T20:20:46Z

tokio/tests/sync_mpsc.rs

+    rx.close();
+
+    // Does not panic, no longer expecting an empty buffer (#7990).
+    rx.recv_many(&mut buffer, 1).await;


Similar to the bounded channel test, this test can be made more robust by asserting that recv_many returns 0 and that the buffer's contents are unchanged.

let n = rx.recv_many(&mut buffer, 1).await; assert_eq!(n, 0); assert_eq!(buffer, &[1]);

value:useful; category:bug; feedback: The Gemini AI reviewer is correct! The test just (implicitly) asserts that there is no panic. It would be better to also assert the returned value of the receive operation and the contents of the buffer after it.

augmentcode · 2026-03-29T20:20:54Z

🤖 Augment PR Summary

Summary: Fixes Chan::recv_many so it no longer assumes the caller-provided buffer is empty when the receiver is closed and idle.
Changes: Adjusts the invariant check to ensure the buffer length is unchanged and adds regression tests for bounded/unbounded channels with pre-populated buffers.

_{🤖 Was this summary useful? React with 👍 or 👎}

augmentcode

Review completed. 2 suggestions posted.

Comment augment review to trigger a new review at any time.

augmentcode · 2026-03-29T20:20:55Z

tokio/tests/sync_mpsc.rs

+    rx.close();
+
+    // Does not panic, no longer expecting an empty buffer (#7990).
+    rx.recv_many(&mut buffer, 1).await;


This test currently only verifies that recv_many doesn’t panic; could you also assert the returned count and that buffer remains unchanged so it catches behavior regressions that don’t panic? Other locations where this applies: tokio/tests/sync_mpsc.rs:382.

Severity: low

_{🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.}

value:useful; category:bug; feedback: The Augment AI reviewer is correct! The test just (implicitly) asserts that there is no panic. It would be better to also assert the returned value of the receive operation and the contents of the buffer after it.

augmentcode · 2026-03-29T20:20:55Z

tokio/tests/sync_mpsc.rs

+
+    rx.close();
+
+    // Does not panic, no longer expecting an empty buffer (#7990).


The comment references issue/PR #7990, but this PR is labeled 7991; can you double-check the intended cross-reference so it’s easier to trace later? Other locations where this applies: tokio/tests/sync_mpsc.rs:381.

Severity: low

_{🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.}

value:incorrect-but-reasonable; category:bug; feedback: The Augment AI reviewer is not correct! 7900 is the number of the Github Issue that is resolved by Pull Request 7991.

claude · 2026-03-29T20:21:32Z

PR Review

Summary

This PR correctly identifies and fixes a real bug: Chan::recv_many would panic when called on a closed, idle channel with a non-empty buffer. The old assertion (buffer.is_empty()) was wrong — recv_many is explicitly designed to append to a caller-provided buffer, so asserting it must be empty was always incorrect. The fix (buffer.len() == initial_length) properly captures the actual invariant: no items were added during this poll.

Issues

Tests are missing assertions (medium severity)

The two new regression tests call recv_many but do not assert on the return value or buffer state — they only verify that no panic occurs. Tests should also check what the function returns and what state the buffer is left in. Suggested additions:

let result = rx.recv_many(&mut buffer, 1).await;
assert_eq!(0, result);        // channel was empty — no items added
assert_eq!(vec![1], buffer);  // pre-existing buffer contents are preserved

Since the whole bug was about incorrect assumptions around buffer contents, asserting the buffer is unchanged makes the regression test maximally expressive. Without this, a future regression that incorrectly modifies the buffer would go undetected.

Minor Suggestions

Comment on the assertion — assert!(buffer.len() == initial_length) is subtly different from the original assert!(buffer.is_empty()). A short inline comment explaining the invariant (no items were added during this poll) would help future readers.

Style nit — assert_eq! is conventionally preferred over assert!(a == b) in Rust because it shows both values on failure: assert_eq!(buffer.len(), initial_length);

What is Good

The root cause analysis in the commit message is clear and accurate.
Both bounded and unbounded paths are tested (they use different semaphore implementations).
The #[maybe_tokio_test] usage is consistent with the rest of the test file.
Minimal, surgical change with no unrelated modifications.

Verdict

The core bug fix is correct. Before merging, consider strengthening the two new tests with assertions on the return value and buffer state to make the regression coverage explicit.

coderabbitai

🧹 Nitpick comments (1)

tokio/tests/sync_mpsc.rs (1)

373-383: Good regression test for the unbounded channel case.

Consider asserting the return value and buffer state for additional coverage:

🔧 Optional: Add assertions for return value and buffer state

     // Does not panic, no longer expecting an empty buffer (`#7990`).
-    rx.recv_many(&mut buffer, 1).await;
+    let count = rx.recv_many(&mut buffer, 1).await;
+    assert_eq!(count, 0);
+    assert_eq!(buffer, vec![1]); // Buffer unchanged
 }

Similar enhancement could be applied to the bounded test above.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tokio/tests/sync_mpsc.rs` around lines 373 - 383, Add assertions after the
recv_many call to verify it reports zero newly-received items and leaves the
existing buffer intact: call let received = rx.recv_many(&mut buffer, 1).await;
assert_eq!(received, 0); assert_eq!(buffer, vec![1]); This uses the existing
test function recv_many_with_non_empty_buffer_unbounded_rx_closed_and_idle and
the variables rx and buffer to check both the return value and buffer state.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@tokio/tests/sync_mpsc.rs`:
- Around line 373-383: Add assertions after the recv_many call to verify it
reports zero newly-received items and leaves the existing buffer intact: call
let received = rx.recv_many(&mut buffer, 1).await; assert_eq!(received, 0);
assert_eq!(buffer, vec![1]); This uses the existing test function
recv_many_with_non_empty_buffer_unbounded_rx_closed_and_idle and the variables
rx and buffer to check both the return value and buffer state.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: de24c698-f13a-47a3-86b5-5ec9cd45c2d4

📥 Commits

Reviewing files that changed from the base of the PR and between 0f7ef49 and 2222621.

📒 Files selected for processing (2)

tokio/src/sync/mpsc/chan.rs
tokio/tests/sync_mpsc.rs

martin-augment · 2026-03-29T20:28:37Z

373-383: Good regression test for the unbounded channel case.

Consider asserting the return value and buffer state for additional coverage:

value:useful; category:bug; feedback: The CodeRabbit AI reviewer is correct! The test just (implicitly) asserts that there is no panic. It would be better to also assert the returned value of the receive operation and the contents of the buffer after it.

martin-augment · 2026-03-29T20:29:01Z

Tests are missing assertions (medium severity)

The two new regression tests call recv_many but do not assert on the return value or buffer state — they only verify that no panic occurs. Tests should also check what the function returns and what state the buffer is left in. Suggested additions:
let result = rx.recv_many(&mut buffer, 1).await;
assert_eq!(0, result);        // channel was empty — no items added
assert_eq!(vec![1], buffer);  // pre-existing buffer contents are preserved
Since the whole bug was about incorrect assumptions around buffer contents, asserting the buffer is unchanged makes the regression test maximally expressive. Without this, a future regression that incorrectly modifies the buffer would go undetected.

value:useful; category:bug; feedback: The Claude AI reviewer is correct! The test just (implicitly) asserts that there is no panic. It would be better to also assert the returned value of the receive operation and the contents of the buffer after it.

LeoniePhiline added 2 commits March 27, 2026 01:13

test: test Chan::recv_many after Rx closed with non-empty buffer

2ad3b85

Proves tokio-rs#7990.

gemini-code-assist bot reviewed Mar 29, 2026

View reviewed changes

augmentcode bot reviewed Mar 29, 2026

View reviewed changes

coderabbitai bot reviewed Mar 29, 2026

View reviewed changes


		rx.close();

		// Does not panic, no longer expecting an empty buffer (#7990).

Conversation

martin-augment commented Mar 29, 2026

Uh oh!

coderabbitai bot commented Mar 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist bot Mar 29, 2026

Choose a reason for hiding this comment

Uh oh!

martin-augment Mar 29, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Mar 29, 2026

Choose a reason for hiding this comment

Uh oh!

martin-augment Mar 29, 2026

Choose a reason for hiding this comment

Uh oh!

augmentcode bot commented Mar 29, 2026

Uh oh!

augmentcode bot left a comment

Choose a reason for hiding this comment

Uh oh!

augmentcode bot Mar 29, 2026

Choose a reason for hiding this comment

Uh oh!

martin-augment Mar 29, 2026

Choose a reason for hiding this comment

Uh oh!

augmentcode bot Mar 29, 2026

Choose a reason for hiding this comment

Uh oh!

martin-augment Mar 29, 2026

Choose a reason for hiding this comment

Uh oh!

claude bot commented Mar 29, 2026

PR Review

Summary

Issues

Minor Suggestions

What is Good

Verdict

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

martin-augment commented Mar 29, 2026

Uh oh!

martin-augment commented Mar 29, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

coderabbitai bot commented Mar 29, 2026 •

edited

Loading