Fix reachable Debug.Assert in StreamPipeReader.AdvanceTo

[Copilot]

Description

Debug.Assert(_bufferedBytes >= 0) in StreamPipeReader.AdvanceTo is reachable from public API when passing a SequencePosition from a different PipeReader. Debug assertions should never be reachable from public APIs, even under misuse.

PipeReader reader1 = PipeReader.Create(new MemoryStream(new byte[10]));
PipeReader reader2 = PipeReader.Create(new MemoryStream(new byte[1000]));

ReadResult result1 = await reader1.ReadAsync();
ReadResult result2 = await reader2.ReadAsync();

// Triggers Debug.Assert in debug builds, silently corrupts state in release
reader1.AdvanceTo(result2.Buffer.End);

Pre-change behavior in release builds: The Debug.Assert was stripped out, allowing the code to continue with corrupted state. _bufferedBytes would become negative after subtracting an invalid consumedBytes value calculated from segments with unrelated RunningIndex values. This led to the reader having an invalid _readHead pointing to another reader's segment, negative _bufferedBytes, and potentially leaked/corrupted buffer segments.

Changes:

• Replace Debug.Assert with validation that throws InvalidOperationException when consumedBytes is negative or exceeds _bufferedBytes
• Add test AdvanceWithPositionFromDifferentReaderThrows covering the reported scenario

Note: This fix validates the symptom (invalid consumedBytes values) rather than explicitly checking segment ownership, which would add O(n) overhead. The Pipe class has a similar Debug.Assert(_unconsumedBytes >= 0) pattern that may warrant similar treatment.

Original prompt

---

This section details on the original issue you should resolve

<issue_title>Reachable Debug.Assert in StreamPipeReader</issue_title>
<issue_description>The following unit test intentionally misuses PipeReader, however it does hit a Debug.Assert.

Debug.Assert is generally meant to hold invariants to be true, meaning misuse or not, they should not be reachable from public APIs.

[Fact]
public async Task PipeReaderPositionMisuseDebugAssert()
{
    PipeReader reader1 = PipeReader.Create(new MemoryStream(new byte[10]));
    PipeReader reader2 = PipeReader.Create(new MemoryStream(new byte[1000]));

    ReadResult result1 = await reader1.ReadAsync();
    ReadResult result2 = await reader2.ReadAsync();

    SequencePosition posFrom2 = result2.Buffer.End;
    reader1.AdvanceTo(posFrom2);
}

Will result in the following test failure:

Stack Trace:
     at Microsoft.VisualStudio.TestPlatform.TestHost.TestHostTraceListener.GetException(String message)
   at Microsoft.VisualStudio.TestPlatform.TestHost.TestHostTraceListener.Fail(String message, String detailMessage)
   at System.Diagnostics.TraceInternal.Fail(String message, String detailMessage) in /Users/vcsjones/Projects/runtime/src/libraries/System.Diagnostics.TraceSource/src/System/Diagnostics/TraceInternal.cs:line 261
   at System.Diagnostics.TraceInternal.TraceProvider.Fail(String message, String detailMessage) in /Users/vcsjones/Projects/runtime/src/libraries/System.Diagnostics.TraceSource/src/System/Diagnostics/TraceInternal.cs:line 17
   at System.Diagnostics.Debug.Fail(String message, String detailMessage) in /Users/vcsjones/Projects/runtime/src/libraries/System.Private.CoreLib/src/System/Diagnostics/Debug.cs:line 135
   at System.Diagnostics.Debug.Assert(Boolean condition, String message, String detailMessage) in /Users/vcsjones/Projects/runtime/src/libraries/System.Private.CoreLib/src/System/Diagnostics/Debug.cs:line 99
   at System.Diagnostics.Debug.Assert(Boolean condition, String message) in /Users/vcsjones/Projects/runtime/src/libraries/System.Private.CoreLib/src/System/Diagnostics/Debug.cs:line 88
   at System.IO.Pipelines.StreamPipeReader.AdvanceTo(BufferSegment consumedSegment, Int32 consumedIndex, BufferSegment examinedSegment, Int32 examinedIndex) in /Users/vcsjones/Projects/runtime/src/libraries/System.IO.Pipelines/src/System/IO/Pipelines/StreamPipeReader.cs:line 111
   at System.IO.Pipelines.StreamPipeReader.AdvanceTo(SequencePosition consumed, SequencePosition examined) in /Users/vcsjones/Projects/runtime/src/libraries/System.IO.Pipelines/src/System/IO/Pipelines/StreamPipeReader.cs:line 89
   at System.IO.Pipelines.StreamPipeReader.AdvanceTo(SequencePosition consumed) in /Users/vcsjones/Projects/runtime/src/libraries/System.IO.Pipelines/src/System/IO/Pipelines/StreamPipeReader.cs:line 70
```</issue_description>

<agent_instructions>Determine whether the Debug.Assert in question is testing the wrong condition, whether it's completely erroneous, whether there's some missing error handling, whether there's a production bug, etc. Then fix it accordingly and ensure there are tests.</agent_instructions>

## Comments on the Issue (you are @copilot in this section)

<comments>
</comments>

• Fixes Reachable Debug.Assert in StreamPipeReader #123809

---

✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

[Copilot]

Pull request overview

This pull request fixes a critical issue where Debug.Assert(_bufferedBytes >= 0) in StreamPipeReader.AdvanceTo was reachable from public API when passing a SequencePosition from a different PipeReader. Debug assertions should never be reachable from public APIs, even under misuse scenarios.

Changes:

• Replace Debug.Assert with runtime validation that throws InvalidOperationException when consumedBytes is invalid
• Add test coverage for the scenario where a position from one reader is passed to another reader's AdvanceTo method
• Move the _bufferedBytes subtraction to after validation to prevent state corruption

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File	Description
src/libraries/System.IO.Pipelines/src/System/IO/Pipelines/StreamPipeReader.cs	Replaces Debug.Assert with proper runtime validation that checks if consumedBytes is negative or exceeds _bufferedBytes, throwing InvalidOperationException for invalid values
src/libraries/System.IO.Pipelines/tests/StreamPipeReaderTests.cs	Adds test case verifying that passing a SequencePosition from a different PipeReader to AdvanceTo correctly throws InvalidOperationException

[stephentoub]

🤖 Copilot Code Review — PR #123810

Holistic Assessment

Motivation: The PR addresses a real issue where a Debug.Assert is reachable from public API when passing a SequencePosition from an unrelated PipeReader. In debug builds this triggers an assertion failure; in release builds the assertion is stripped, leading to silent state corruption (_bufferedBytes becomes negative, _readHead points to another reader's segment). This violates the fail-fast principle for API misuse and should be fixed.

Approach: The fix validates the computed consumedBytes against the known buffered amount before modifying state. The O(1) symptom-based check is appropriate here — explicitly walking the segment linked list to verify ownership would add O(n) overhead to every AdvanceTo call. The same exception helper (ThrowHelper.ThrowInvalidOperationException_AdvanceToInvalidCursor) is already used at line 101 for the _readHead == null case, so this is consistent.

Summary: ✅ LGTM. The runtime check is lightweight and effective. The test correctly reproduces the cross-reader pollution scenario. A minor unused variable in the test is noted below but is not blocking.

---

Detailed Findings

✅ Correctness — Runtime Validation

The added check if (consumedBytes < 0 || consumedBytes > _bufferedBytes) correctly guards against invalid consumedBytes values that result from calculating distances between unrelated segments. This check runs before modifying _bufferedBytes, preventing state corruption. The condition correctly catches:

• Negative values (from segments with unrelated RunningIndex values)
• Values exceeding _bufferedBytes (consuming more than is actually buffered)

✅ Testing — Regression Coverage

The new test AdvanceWithPositionFromDifferentReaderThrows correctly sets up two independent readers with different buffer sizes and attempts to use a position from reader2 with reader1, verifying that InvalidOperationException is thrown. This directly covers the reported scenario.

✅ Consistency — Exception Pattern

The fix reuses the existing ThrowHelper.ThrowInvalidOperationException_AdvanceToInvalidCursor() helper, maintaining consistency with the error handling patterns in this library.

💡 Suggestion — Unused Variable in Test

The result1 variable is assigned but never used:

ReadResult result1 = await reader1.ReadAsync();  // assigned but unused
ReadResult result2 = await reader2.ReadAsync();

While the ReadAsync call is necessary (to put reader1 into a state with buffered data), the result itself isn't needed. Consider using a discard:

_ = await reader1.ReadAsync();  // ensures reader1 has data
ReadResult result2 = await reader2.ReadAsync();

This is purely cosmetic and may generate a compiler warning (CS8600/IDE0059) depending on project settings.

---

Models contributing to this review: Claude Sonnet 4, Gemini 3 Pro (Preview)