Skip to content

[ci-fix] Needs review: Normalize -1 bufferSize in StreamReader/StreamWriter String ctors (refs #128062)#129360

Closed
github-actions[bot] wants to merge 2 commits into
mainfrom
ci-fix/streamreader-pgo-buffersize-128062-b4de83ddb4ac15e0
Closed

[ci-fix] Needs review: Normalize -1 bufferSize in StreamReader/StreamWriter String ctors (refs #128062)#129360
github-actions[bot] wants to merge 2 commits into
mainfrom
ci-fix/streamreader-pgo-buffersize-128062-b4de83ddb4ac15e0

Conversation

@github-actions

Copy link
Copy Markdown
Contributor

Workflow artifact: ci-fix
Artifact kind: help
Linked KBE: #128062

Note

This is an AI/Copilot-generated best-effort fix attempt that I could not fully validate. It is a starting point for a maintainer, not a finished change. Please review the analysis below before merging.

Root cause (best analysis)

StreamReader_StringCtorTests.NegativeOneBufferSize_ShouldNotThrowException and the equivalent StreamWriter test fail under PGO (runtime-coreclr libraries-pgo pipeline) because the JIT, guided by PGO data, optimizes away the if (bufferSize == -1) bufferSize = DefaultBufferSize; branch in the Stream-based constructor when it is inlined into the String-based constructor.

The failing log line:

System.IO.Tests.StreamReader_StringCtorTests.NegativeOneBufferSize_ShouldNotThrowException [FAIL]
System.ArgumentOutOfRangeException : bufferSize ('-1') must be a non-negative and non-zero value.
  at System.IO.StreamReader..ctor(String path, Encoding encoding, Boolean detectEncodingFromByteOrderMarks, Int32 bufferSize)

The String-based constructors (StreamReader(string, Encoding, bool, int) and StreamWriter(string, bool, Encoding, int)) pass their bufferSize parameter (which may be -1) directly to the Stream-based constructor via constructor chaining. The Stream-based constructor handles -1 by converting to DefaultBufferSize before calling ThrowIfNegativeOrZero. Under PGO inlining, this conversion branch is (incorrectly) optimized away.

Attempted fix

Normalize the -1 sentinel to DefaultBufferSize in the String-based constructors before passing to the Stream-based constructor, so the inlined code never sees -1 at the ThrowIfNegativeOrZero call site regardless of PGO optimization decisions. This is a defense-in-depth fix — the Stream-based constructor still handles -1 correctly for direct callers.

Changes:

  • StreamReader.cs line 200: bufferSizebufferSize == -1 ? DefaultBufferSize : bufferSize
  • StreamWriter.cs line 148: same pattern

What is unverified / where I need help

  • Build not validated: The .NET SDK is not available in the CI remediation environment. The change is a single ternary expression addition with no structural risk.
  • Root cause is a JIT/PGO bug: This fix works around the JIT incorrectly eliminating the -1 conversion branch under PGO. The JIT team should investigate why PGO-guided optimization removes this branch — the bug could manifest in other code patterns.
  • Assigned to @EgorBo: The issue is labeled area-CodeGen-coreclr and assigned to EgorBo, suggesting the team views this as a codegen issue. This defensive fix is a product-code hardening that makes the code PGO-safe without touching the JIT.

Validation

  • Command: dotnet build src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.csproj
  • Result: not run (.NET SDK not available in CI remediation environment)
  • Why the failing test validates this fix: NegativeOneBufferSize_ShouldNotThrowException calls new StreamReader(path, Encoding.UTF8, true, -1) which exercises exactly this constructor path. With the fix, -1 is converted to DefaultBufferSize before the inlined Stream ctor sees it, so ThrowIfNegativeOrZero never fires.

Evidence

Help wanted

  • Area owners (area-CodeGen-coreclr): @EgorBo, @dotnet/jit-contrib
  • The underlying JIT/PGO bug should still be investigated separately. The defensive fix prevents the test failure but the JIT eliminating a valid branch is a correctness issue.

Filed by ci-failure-fix. Comment here or on the workflow file to suggest changes; ci-failure-scan-feedback reads in-scope feedback daily and opens (or updates) a PR with prompt edits.

Generated by CI Outer-Loop Failure Fixer · ● 27.1M ·

…r/StreamWriter

Under PGO, the JIT may optimize away the bufferSize == -1 conversion
in the Stream-based constructor when it is inlined into the String-based
constructor. This causes ThrowIfNegativeOrZero to fire for the -1
sentinel value.

Defensively normalize -1 to DefaultBufferSize in the String-based
constructors before passing to the Stream-based constructor, making
the code robust against PGO optimization decisions.

Fixes the NegativeOneBufferSize_ShouldNotThrowException test failure
in the runtime-coreclr libraries-pgo pipeline.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @dotnet/area-system-io
See info in area-owners.md if you want to be subscribed.

@MihaZupan

Copy link
Copy Markdown
Member

I don't see a point in trying to workaround a codegen bug like this

@MihaZupan MihaZupan closed this Jun 14, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant