-
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?
Changes from 3 commits
e05ae94
e55fcaa
63d3e9b
2350a36
461f2bf
e73419f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1018,10 +1018,51 @@ | |
| await Verify(_outputWriter.ToString(), _settings).UniqueForOSPlatform(); | ||
| } | ||
|
|
||
| [Fact] | ||
| public void ReplayBinaryLogWithFewerNodesThanOriginalBuild() | ||
| { | ||
| // 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(); | ||
| } | ||
YuliiaKovalova marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| [Theory] | ||
| [InlineData(true)] | ||
| [InlineData(false)] | ||
| public async Task DisplayNodesRestoresStatusAfterMSBuildTaskYields_TestProject(bool runOnCentralNode) | ||
|
Check failure on line 1065 in src/Build.UnitTests/TerminalLogger_Tests.cs
|
||
| { | ||
| // 1. Start Build | ||
| _centralNodeEventSource.InvokeBuildStarted(MakeBuildStartedEventArgs()); | ||
|
|
@@ -1071,4 +1112,4 @@ | |
| await Verify(_outputWriter.ToString(), _settings).UniqueForOSPlatform().UseParameters(runOnCentralNode); | ||
| } | ||
| } | ||
| } | ||
|
Check failure on line 1115 in src/Build.UnitTests/TerminalLogger_Tests.cs
|
||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. syntax errors |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -732,6 +732,7 @@ private void ProjectStarted(object sender, ProjectStartedEventArgs e) | |
| { | ||
| _restoreContext = c; | ||
| int nodeIndex = NodeIndexForContext(e.BuildEventContext); | ||
| EnsureNodeCapacity(nodeIndex); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @copilot limit to replay mode
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in e73419f. Added |
||
| _nodes[nodeIndex] = new TerminalNodeStatus(e.ProjectFile!, targetFramework, runtimeIdentifier, "Restore", _projects[c].Stopwatch); | ||
| } | ||
| } | ||
|
|
@@ -1047,9 +1048,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); | ||
YuliiaKovalova marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| } | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// The <see cref="IEventSource.TargetFinished"/> callback. Unused. | ||
| /// </summary> | ||
|
|
@@ -1185,7 +1201,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) | ||
|
|
||
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
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.
@copilot
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.
Fixed in e73419f. The test now creates three separate projects and builds them in parallel using MSBuild task with BuildInParallel='true', which ensures multiple NodeIds are present in the binlog.