-
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 all 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 |
|---|---|---|
|
|
@@ -151,6 +151,11 @@ public EvalContext(BuildEventContext context) | |
| /// </summary> | ||
| private ProjectContext? _restoreContext; | ||
|
|
||
| /// <summary> | ||
| /// True if we're replaying a binary log. In this mode, we may encounter NodeIds higher than the initial node count. | ||
| /// </summary> | ||
| private bool _isReplayMode = false; | ||
|
|
||
| /// <summary> | ||
| /// The thread that performs periodic refresh of the console output. | ||
| /// </summary> | ||
|
|
@@ -432,6 +437,9 @@ public void Initialize(IEventSource eventSource) | |
| { | ||
| ParseParameters(); | ||
|
|
||
| // Detect if we're in replay mode | ||
| _isReplayMode = eventSource is IBinaryLogReplaySource; | ||
|
|
||
| eventSource.BuildStarted += BuildStarted; | ||
| eventSource.BuildFinished += BuildFinished; | ||
| eventSource.ProjectStarted += ProjectStarted; | ||
|
|
@@ -732,6 +740,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 +1056,31 @@ 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) | ||
| { | ||
| // Only resize in replay mode - during normal builds, the node count is fixed | ||
| if (_isReplayMode && nodeIndex >= _nodes.Length) | ||
| { | ||
| // Resize to accommodate the new index plus some extra capacity | ||
| lock (_lock) | ||
| { | ||
| if (nodeIndex >= _nodes.Length) | ||
| { | ||
| int newSize = Math.Max(nodeIndex + 1, _nodes.Length * 2); | ||
| Array.Resize(ref _nodes, newSize); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// The <see cref="IEventSource.TargetFinished"/> callback. Unused. | ||
| /// </summary> | ||
|
|
@@ -1185,7 +1216,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.