Skip to content

Fix redirect stdout and stderror to the same file overlapping - #41611

Merged
Feng Wang (chemwolf6922) merged 3 commits into
masterfrom
user/chemwolf6922/fix-mreged-stdout-and-stderr-to-a-file-overlaps
Sep 17, 2026
Merged

Fix redirect stdout and stderror to the same file overlapping#41611
Feng Wang (chemwolf6922) merged 3 commits into
masterfrom
user/chemwolf6922/fix-mreged-stdout-and-stderr-to-a-file-overlaps

Conversation

@chemwolf6922

@chemwolf6922 Feng Wang (chemwolf6922) commented Sep 15, 2026

Copy link
Copy Markdown
Contributor

Summary of the Pull Request

Currently the stdout and stderr relay tracks the output offset separately. And when redirected to the same file, the writes could overlap.

This PR uses the append mode for overlap io writes instead of the separate offsets in the relay. So, the file write offset is correctly tracked by the system.

PR Checklist

  • Closes: Link to issue #xxx
  • Communication: I've discussed this with core contributors already. If work hasn't been agreed, this work might be rejected
  • Tests: Added/updated if needed and all pass
  • Localization: All end user facing strings can be localized
  • Dev docs: Added/updated if needed
  • Documentation updated: If checked, please file a pull request on our docs repo and link it here: #xxx

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

Add test:
UnitTests::UnitTests::MergedOutputFileOffsets

@chemwolf6922
Feng Wang (chemwolf6922) requested a review from a team as a code owner September 15, 2026 08:42
Copilot AI lite review requested due to automatic review settings September 15, 2026 08:42
Copilot AI reviewed Sep 15, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The fix and regression coverage address the reported merged-output issue.

Pull request overview

This PR prevents overlapping writes when stdout and stderr are redirected to the same file and adds regression coverage.

Changes:

  • Uses append-mode relay writes.
  • Adds merged-output regression testing.
File summaries
File Description
test/windows/UnitTests.cpp Adds merged stdout/stderr output coverage.
src/windows/common/relay.cpp Prevents overlapping relay writes.
Review details
  • Files reviewed: 2/2 changed files
  • Comments generated: 0
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Copilot AI review requested due to automatic review settings September 15, 2026 09:10
Copilot AI reviewed Sep 15, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The critical relay offset behavior issue must be addressed before approval.

Get a fresh assessment by requesting another Copilot review.

Review details
  • Files reviewed: 2/2 changed files
  • Comments generated: 1
  • Review effort level: Lite

Comment thread src/windows/common/relay.cpp Resolved
@chemwolf6922 Feng Wang (chemwolf6922) changed the title Fix merged stdout and stderror to a file overlapping Fix redirect stdout and stderror to the same file overlapping Sep 15, 2026
@benhillis
Ben Hillis (benhillis) requested a balanced review from Copilot September 15, 2026 16:30
Copilot AI reviewed Sep 15, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

@benhillis
Ben Hillis (benhillis) requested a balanced review from Copilot September 15, 2026 16:54
Copilot AI reviewed Sep 15, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The append-offset fix is focused and covered by an appropriate regression test.

Review details
  • Files reviewed: 2/2 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

OneBlue
Blue (OneBlue) previously approved these changes Sep 15, 2026
View reviewed changes

@OneBlue Blue (OneBlue) left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, minor comment

Comment thread test/windows/UnitTests.cpp Resolved
Comment thread src/windows/common/relay.cpp Resolved
Copilot AI review requested due to automatic review settings September 16, 2026 02:42
@chemwolf6922
Feng Wang (chemwolf6922) requested a review from a team as a code owner September 16, 2026 02:42
Copilot AI reviewed Sep 16, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The append-offset implementation addresses concurrent overlap and is covered by a focused regression test.

Review details
  • Files reviewed: 2/2 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

@chemwolf6922
Feng Wang (chemwolf6922) merged commit a2ff822 into master Sep 17, 2026
14 checks passed
@chemwolf6922
Feng Wang (chemwolf6922) deleted the user/chemwolf6922/fix-mreged-stdout-and-stderr-to-a-file-overlaps branch September 17, 2026 02:15
Blue (OneBlue) added a commit that referenced this pull request Sep 22, 2026
@OneBlue @chemwolf6922
@ggarzia-MSFT @dkbennett @49081 @beena352 @archlinux-github @benhillis Ben Hillis @damanm24 @tega-ajise Tega Ajise @Eamon2009 @kvega005 @dependabot @DesertRatUa @sbradnick @halter73 @JohnMcPMS @florelis @cnihelton @CarlosNihelton @antonkesy @craigloewen-msft @Dev-next-gen @FetoiuCatalin Catalin-Emil Fetoiu