-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Fix TerminalLogger IndexOutOfRangeException when replaying binlog with fewer nodes #12809
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
Hello @@copilot, I noticed that you’re changing an .swr file or any file under src/Package/MSBuild.VSSetup.. Please make sure to validate this change by an experimental VS insertion. This is accomplished by pushing to an exp/* branch, which requires write permissions to this repo. |
…with fewer nodes Co-authored-by: YuliiaKovalova <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR fixes an IndexOutOfRangeException that occurs when replaying a binary log with the TerminalLogger using fewer nodes than the original build. The issue arose because the logger initializes its _nodes array based on the replay's node count, but events in the binlog reference NodeIds from the original build with potentially higher values.
Key changes:
- Added dynamic array resizing via
EnsureNodeCapacity()method - Protected all
_nodesarray accesses in event handlers to ensure capacity before indexing - Added test case for binlog replay with mismatched node counts
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/Build/Logging/TerminalLogger/TerminalLogger.cs | Added EnsureNodeCapacity() method with exponential growth strategy and updated all three array access points (ProjectStarted, UpdateNodeStatus, MessageRaised) to call it before indexing |
| src/Build.UnitTests/TerminalLogger_Tests.cs | Added regression test that builds with /m:4 and replays with nodeCount: 1 to verify the fix works |
| Should.NotThrow(() => replayEventSource.Replay(binlogPath)); | ||
|
|
||
| terminalLogger.Shutdown(); | ||
| } |
Copilot
AI
Dec 1, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing closing brace for the test method. The using block closes at line 1060, but the method itself needs a closing brace before the next test method starts at line 1062.
| } | |
| } | |
| } |
| int newSize = Math.Max(nodeIndex + 1, _nodes.Length * 2); | ||
| Array.Resize(ref _nodes, newSize); |
Copilot
AI
Dec 1, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potential race condition: Array.Resize is not thread-safe and can cause issues when called concurrently with reads from the refresher thread. The comment at lines 113-115 states "reads and writes to locations in an array is atomic, so locking is not required," but this assumption breaks down with Array.Resize.
Array.Resize allocates a new array and copies elements, which is not atomic. The refresher thread calls DisplayNodes() under _lock at line 1441, which reads the entire _nodes array when constructing TerminalNodesFrame (TerminalNodesFrame.cs line 34). Meanwhile, event handlers like ProjectStarted, TargetStarted, and MessageRaised can call EnsureNodeCapacity without holding the lock, potentially causing a race condition.
Consider protecting EnsureNodeCapacity calls with the same _lock used in the refresher thread, or use a thread-safe alternative like Interlocked.CompareExchange to swap array references atomically.
| int newSize = Math.Max(nodeIndex + 1, _nodes.Length * 2); | |
| Array.Resize(ref _nodes, newSize); | |
| lock (_lock) | |
| { | |
| if (nodeIndex >= _nodes.Length) | |
| { | |
| int newSize = Math.Max(nodeIndex + 1, _nodes.Length * 2); | |
| Array.Resize(ref _nodes, newSize); | |
| } | |
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
valid concern 👍
| int newSize = Math.Max(nodeIndex + 1, _nodes.Length * 2); | ||
| Array.Resize(ref _nodes, newSize); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
valid concern 👍
| await Verify(_outputWriter.ToString(), _settings).UniqueForOSPlatform().UseParameters(runOnCentralNode); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
syntax errors
| } | ||
|
|
||
| [Fact] | ||
| public void ReplayBinaryLogWithFewerNodesThanOriginalBuild() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the test does not make sense, the build of this project will run on one node even though /m:4 is specified so no error will be produced when replayed with lower /m
| { | ||
| _restoreContext = c; | ||
| int nodeIndex = NodeIndexForContext(e.BuildEventContext); | ||
| EnsureNodeCapacity(nodeIndex); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems strange to ensure node capacity in the common path and it not being something specific for replay mode
Replaying a binary log with terminal logger fails when the replay uses fewer nodes (
/m:1) than the original build. The logger initializes its_nodesarray based on the replay's node count, but events in the binlog reference NodeIds from the original build, causing anIndexOutOfRangeException.Changes
EnsureNodeCapacity()to dynamically resize_nodesarray when encountering NodeIds beyond current capacity_nodesarray accesses (inUpdateNodeStatus,ProjectStarted,MessageRaised) to ensure capacity before indexingFixes #10596
Warning
Firewall rules blocked me from connecting to one or more addresses (expand for details)
I tried to connect to the following addresses, but was blocked by firewall rules:
rcxvsblobprodcus328.vsblob.vsassets.io/home/REDACTED/work/msbuild/msbuild/artifacts/bin/bootstrap/core/dotnet dotnet test src/Build.UnitTests/Microsoft.Build.Engine.UnitTests.csproj --filter FullyQualifiedName~ReplayBinaryLogWithFewerNodesThanOriginalBuild --no-build -v n(dns block)/home/REDACTED/work/msbuild/msbuild/artifacts/bin/bootstrap/core/dotnet dotnet test src/Build.UnitTests/Microsoft.Build.Engine.UnitTests.csproj --filter FullyQualifiedName~TerminalLogger --no-build -v n(dns block)If you need me to access, download, or install something from one of these locations, you can either:
Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.