Skip to content

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.

Sign up for GitHub

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

Open
Copilot wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Fix TerminalLogger IndexOutOfRangeException when replaying binlog with fewer nodes #12809

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e05ae94
Initial plan
Copilot Nov 21, 2025
e55fcaa
Fix TerminalLogger IndexOutOfRangeException during binary log replay …
Copilot Nov 21, 2025
63d3e9b
Merge branch 'main' into copilot/fix-binary-log-replay-error
YuliiaKovalova Dec 1, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 42 additions & 0 deletions src/Build.UnitTests/TerminalLogger_Tests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -942,5 +942,47 @@ public async Task ProjectFinishedReportsTargetFrameworkAndRuntimeIdentifier()
});
await Verify(_outputWriter.ToString(), _settings).UniqueForOSPlatform();
}

[Fact]
public void ReplayBinaryLogWithFewerNodesThanOriginalBuild()
Copy link
Member

@JanProvaznik JanProvaznik Dec 1, 2025

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

{
// This test validates that replaying a binary log with terminal logger
// using fewer nodes than the original build does not cause an IndexOutOfRangeException.
// See issue: https://github.com/dotnet/msbuild/issues/10596

using (TestEnvironment env = TestEnvironment.Create())
{
// Create a simple project
string contents = @"
<Project>
<Target Name='Build'>
<Message Text='Building project' Importance='High' />
</Target>
</Project>";
TransientTestFolder logFolder = env.CreateFolder(createFolder: true);
TransientTestFile projectFile = env.CreateFile(logFolder, "test.proj", contents);

string binlogPath = env.ExpectFile(".binlog").Path;

// Build with multiple nodes to create a binlog with higher node IDs
RunnerUtilities.ExecMSBuild($"{projectFile.Path} /m:4 /bl:{binlogPath}", out bool success, outputHelper: _outputHelper);
success.ShouldBeTrue();

// Replay the binlog with TerminalLogger using only 1 node
// This should NOT throw an IndexOutOfRangeException
var replayEventSource = new BinaryLogReplayEventSource();
using var outputWriter = new StringWriter();
using var mockTerminal = new Terminal(outputWriter);
var terminalLogger = new TerminalLogger(mockTerminal);

// Initialize with only 1 node (fewer than the original build)
terminalLogger.Initialize(replayEventSource, nodeCount: 1);

// This should complete without throwing an exception
Should.NotThrow(() => replayEventSource.Replay(binlogPath));

terminalLogger.Shutdown();
}
Copy link

Copilot AI Dec 1, 2025

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.

Suggested change
}
}
}

Copilot uses AI. Check for mistakes.
}
}
}
Copy link
Member

@JanProvaznik JanProvaznik Dec 1, 2025

Choose a reason for hiding this comment

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

syntax errors

20 changes: 19 additions & 1 deletion src/Build/Logging/TerminalLogger/TerminalLogger.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -665,6 +665,7 @@ private void ProjectStarted(object sender, ProjectStartedEventArgs e)
{
_restoreContext = c;
int nodeIndex = NodeIndexForContext(e.BuildEventContext);
EnsureNodeCapacity(nodeIndex);
Copy link
Member

@JanProvaznik JanProvaznik Dec 1, 2025

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

_nodes[nodeIndex] = new TerminalNodeStatus(e.ProjectFile!, targetFramework, runtimeIdentifier, "Restore", _projects[c].Stopwatch);
}
}
Expand Down Expand Up @@ -981,9 +982,24 @@ private void TargetStarted(object sender, TargetStartedEventArgs e)
private void UpdateNodeStatus(BuildEventContext buildEventContext, TerminalNodeStatus? nodeStatus)
{
int nodeIndex = NodeIndexForContext(buildEventContext);
EnsureNodeCapacity(nodeIndex);
_nodes[nodeIndex] = nodeStatus;
}

/// <summary>
/// Ensures that the <see cref="_nodes"/> array has enough capacity to accommodate the given index.
/// This is necessary for binary log replay scenarios where the replay may use fewer nodes than the original build.
/// </summary>
private void EnsureNodeCapacity(int nodeIndex)
{
if (nodeIndex >= _nodes.Length)
{
// Resize to accommodate the new index plus some extra capacity
int newSize = Math.Max(nodeIndex + 1, _nodes.Length * 2);
Array.Resize(ref _nodes, newSize);
Comment on lines +1064 to +1065
Copy link

Copilot AI Dec 1, 2025

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.

Suggested change
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);
}
}

Copilot uses AI. Check for mistakes.
Copy link
Member

@JanProvaznik JanProvaznik Dec 1, 2025

Choose a reason for hiding this comment

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

valid concern 👍

}
}

/// <summary>
/// The <see cref="IEventSource.TargetFinished"/> callback. Unused.
/// </summary>
Expand Down Expand Up @@ -1100,7 +1116,9 @@ private void MessageRaised(object sender, BuildMessageEventArgs e)

if (hasProject && project!.IsTestProject)
{
TerminalNodeStatus? node = _nodes[NodeIndexForContext(buildEventContext)];
int nodeIndex = NodeIndexForContext(buildEventContext);
EnsureNodeCapacity(nodeIndex);
TerminalNodeStatus? node = _nodes[nodeIndex];

// Consumes test update messages produced by VSTest and MSTest runner.
if (e is IExtendedBuildEventArgs extendedMessage)
Expand Down