Preventing coordinator messages from getting filtered out by terminal logger by VolPlita · Pull Request #14356 · dotnet/msbuild

VolPlita · 2026-07-14T14:32:13Z

Fixes: #14345

Issue

When MSBuild is invoked with the /tl (Terminal Logger) flag, coordinator wait messages were silently dropped, leaving users without visibility into build delays during nested grant scenarios. This made it difficult to diagnose why builds appeared to hang.

Root Cause

Coordinator messages have null BuildEventContext and MessageImportance.High to indicate they are global diagnostics. These were filtered out at two levels:

ForwardingTerminalLogger dropped them before forwarding to the central logger
TerminalLogger had no handler for null-context messages

Changes

ForwardingTerminalLogger.cs: Forward global messages (null context + HIGH importance) before the null-context filter, respecting quiet mode
TerminalLogger.cs: Handle global messages by displaying them directly to terminal without project context

Testing

Added unit tests in ForwardingTerminalLogger_Tests.cs:
- Global HIGH importance messages are forwarded
- Global NORMAL importance messages are not forwarded
- Project-context messages still work correctly
- Multiple coordinator messages are forwarded
- Quiet mode suppresses all messages including globals
Verified existing TerminalLogger suite tests remain unaffected

…ull-context filter

Copilot

Pull request overview

This PR addresses a /tl (Terminal Logger) usability regression where coordinator “waiting/grant” messages (global, BuildEventContext == null) were being dropped, reducing visibility into build delays.

Changes:

Adjusts ForwardingTerminalLogger to forward null-context HIGH importance coordinator/global messages (while still respecting quiet mode).
Adds TerminalLogger handling for null-context HIGH importance messages so they can be rendered to the terminal.
Introduces new unit tests validating ForwardingTerminalLogger forwarding behavior for null-context/global messages.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File	Description
src/Build/Logging/TerminalLogger/TerminalLogger.cs	Adds rendering path for null-context HIGH-importance messages in the terminal logger.
src/Build/Logging/TerminalLogger/ForwardingTerminalLogger.cs	Changes message forwarding order/filters so global coordinator messages aren’t dropped before reaching the central logger.
src/Build.UnitTests/ForwardingTerminalLogger_Tests.cs	Adds regression/unit tests ensuring null-context forwarding behavior and quiet-mode suppression.

+        // For global/coordinator messages with high importance
+        if (buildEventContext is null && message is not null && e.Importance == MessageImportance.High)
+        {


rainersigwald · 2026-07-14T20:47:41Z

        var buildEventContext = e.BuildEventContext;
+        string? message = e.Message;
+        // For global/coordinator messages with high importance
+        if (buildEventContext is null && message is not null && e.Importance == MessageImportance.High)


I was very confused about how the BEC could be null here, since it should be set to Invalid when the coordinator event throws. It looks like in the case I was debugging, the message is originally raised in the NuGet.Build.Tasks.Console.exe helper process, which serializes it back to be logged here.

That assembly compiles against Microsoft.Build.Utilities.v4.0, and the LogMessage there explicitly passes null for BuildEventContext.

Because RestoreTaskEx generally runs on the entrypoint node we might be able to avoid pushing the change to the forwarding logger, but I don't think that's worth changing.

Thanks for the investigation, that makes sense. I think we are fine with defensive guard in the logger, let's keep it as is

rainersigwald · 2026-07-14T21:59:50Z

        var buildEventContext = e.BuildEventContext;
+        string? message = e.Message;
+        // For global/coordinator messages with high importance
+        if (buildEventContext is null && message is not null && e.Importance == MessageImportance.High)


I would ideally like to check more than context-is-null-and-importance-is-high. Should we add an unlocalized keyword to the message so we can scan for it like IsAuthProviderMessage?

That would be more robust, but it depends on scope. If coordinator messages are defined in a few places we control, a keyword marker makes sense. If they come from legacy helpers we don't control, the null-bec+HIGH guard might be the most pragmatic boundary. How many sources emit these messages?

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Recurring reviewer feedback (PRs #14356, #14370, rainersigwald) flagged direct terminal writes for immediate messages instead of using the established RenderImmediateMessage path. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

JanProvaznik · 2026-07-27T09:50:24Z

could you paste here the before and after (either in plaintext or screenshots)?

AlesProkop · 2026-07-27T11:52:29Z

+        string? message = e.Message;
+
+        // For global/coordinator messages with high importance
        if (buildEventContext is null)


The real coordinator message is logged with BuildEventContext.Invalid, not null, so the in-process path is still swallowed.

AlesProkop · 2026-07-27T11:53:15Z

+        // For global/coordinator messages with high importance
        if (buildEventContext is null)
        {
+            if (Verbosity > LoggerVerbosity.Quiet && message is not null && e.Importance == MessageImportance.High)


null context + high importance is too broad a trigger I think. It may render arbitrary third-party messages.

VolPlita · 2026-08-03T10:04:59Z

could you paste here the before and after (either in plaintext or screenshots)?

Before the fix global/coordinator messages (with null BuildEventContext, or BuildEventContext.Invalid from the build coordinator) were silently dropped by TerminalLogger, even at High importance, so we just had:

Build succeeded in 5.0s
(the diagnostic message never appeared it's swallowed by the null-context filter)

After the fix the same messages are now rendered before the build-succeeded summary:

Global diagnostic message.
Build succeeded in 5.0s

Waiting for coordinator to grant build resources...
Build succeeded in 5.0s

JanProvaznik

I think controlling flow by strings is bad for maintainability.
Could we somehow use the type system?
This may require refactoring the coordinator messages to separate type instead of a normal LogComment or a better mechanism for distinguishing events that should be rendered in TL.

@baronfel may have suggestions.

…sts.

… of message-text comparison.

VolPlita · 2026-08-03T13:45:59Z

I think controlling flow by strings is bad for maintainability. Could we somehow use the type system? This may require refactoring the coordinator messages to separate type instead of a normal LogComment or a better mechanism for distinguishing events that should be rendered in TL.

@baronfel may have suggestions.

Good point, and I agree strings aren't great for this. Here's what I did and what a fully-typed fix would probably have to do.

The current commit: the coordinator's "waiting for nodes" message is now tagged with an ExtendedType (a stable internal key), using MSBuild's existing ExtendedBuildMessageEventArgs mechanism. TerminalLogger checks that tag instead of comparing the message text. So wording/localization can change freely without breaking detection. Tbh, it's still a string check under the hood, just a fixed internal key instead of user-facing text. Not fully type-safe, but no compiler typos possible since it's a shared constant, and it's the standard pattern already used elsewhere in the repo.

A fully type-based fix would mean a dedicated event class (e.g. CoordinatorWaitingForNodesEventArgs), checked with is and no strings at all. But that requires plugging it into MSBuild's node/TaskHost message serialization and the binary log format, likely a binlog version bump. Real type safety, but a much bigger change for one narrow diagnostic.

@JanProvaznik @baronfel Do you think I should do that here, file it as a follow-up issue, or is the current fix good enough?

JanProvaznik · 2026-08-03T14:09:33Z

Real type safety, but a much bigger change for one narrow diagnostic

the coding agent can do it 😉 it has precedent so shouldn't be too hard

…rdinatorWaitingForNodesEventArgs`

VolPlita · 2026-08-03T15:08:16Z

Real type safety, but a much bigger change for one narrow diagnostic

the coding agent can do it 😉 it has precedent so shouldn't be too hard

Ok, done, seems fine too me.

JanProvaznik · 2026-08-04T12:15:54Z

+        e is Microsoft.Build.Framework.Coordinator.CoordinatorWaitingForNodesEventArgs ||
+        e is IExtendedBuildEventArgs { ExtendedType: Microsoft.Build.Framework.Coordinator.Constants.WaitingForNodesEventType };


JanProvaznik · 2026-08-04T12:16:34Z

-        if (buildEventContext is null)
+        string? message = e.Message;
+
+        // Null context (e.g. an out-of-process helper) is trusted alone; BuildEventContext.Invalid additionally
+        // requires a recognized coordinator diagnostic, since Invalid can also be (mis-)used by in-process code
+        // that IS associated with the current build.
+        if (buildEventContext is null || buildEventContext == BuildEventContext.Invalid)
        {
+            bool isRecognizedGlobalMessage = buildEventContext is null || IsCoordinatorMessage(e);
+
+            if (Verbosity > LoggerVerbosity.Quiet && message is not null && e.Importance == MessageImportance.High && isRecognizedGlobalMessage)
+            {
+                RenderImmediateMessage(message);
+            }
+


isn't this stale in the updated design with dedicated type?

AlesProkop · 2026-08-11T11:12:14Z

+        // Null context (e.g. an out-of-process helper) is trusted alone; BuildEventContext.Invalid additionally
+        // requires a recognized coordinator diagnostic, since Invalid can also be (mis-)used by in-process code
+        // that IS associated with the current build.
+        if (buildEventContext is null || buildEventContext == BuildEventContext.Invalid)


This suppresses unrelated high-importance messages with BuildEventContext.Invalid at Detailed/Diagnostic verbosity. Could maybe unrecognized Invalid-context messages fall through to the existing verbosity-dependent handling?

AlesProkop · 2026-08-11T11:13:03Z

            {
                return LoggingEventType.ExtendedBuildMessageEvent;
            }
+            else if (eventType == typeof(Microsoft.Build.Framework.Coordinator.CoordinatorWaitingForNodesEventArgs))


Could we add a node-packet round-trip test for this event? The TerminalLogger test manually constructs the generic ExtendedBuildMessageEventArgs, so it does not verify that this mapping preserves the message, importance, context, and ExtendedType across serialization.

…or event internal and add verbosity boundary + tests

Forward global messages (null context + HIGH importance) before the n…

c99c974

…ull-context filter

VolPlita requested a review from Copilot July 14, 2026 14:43

VolPlita force-pushed the fix-14345-unfilter-global-mesages-tl branch from ed81240 to c99c974 Compare July 14, 2026 14:43

VolPlita requested a review from rainersigwald July 14, 2026 14:44

Copilot started reviewing on behalf of VolPlita July 14, 2026 14:44 View session

VolPlita marked this pull request as ready for review July 14, 2026 14:44

Copilot AI reviewed Jul 14, 2026

View reviewed changes

rainersigwald reviewed Jul 14, 2026

View reviewed changes

VolPlita and others added 2 commits July 15, 2026 10:39

Potential fix for pull request finding

771cde4

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

refactor MessageRaised method

327b94a

github-actions Bot mentioned this pull request Jul 15, 2026

[Dreaming] Dreaming: curate learning atoms for week of 2026-07-08 #14368

Open

github-actions Bot mentioned this pull request Jul 15, 2026

[Dreaming] dreaming: clarify RenderImmediateMessage as the correct path for immediate TerminalLogger output #14381

Merged

AlesProkop requested changes Jul 27, 2026

View reviewed changes

VolPlita added 2 commits July 27, 2026 16:34

add regression tests

908cb20

new tests

d2722fa

VolPlita requested a review from JanProvaznik August 3, 2026 10:05

JanProvaznik requested changes Aug 3, 2026

View reviewed changes

VolPlita added 2 commits August 3, 2026 14:49

Add missing Linux/OSX Verify snapshot baselines for TerminalLogger te…

e3dc5e1

…sts.

Refactor coordinator diagnostic detection to use ExtendedType instead…

c74c4ad

… of message-text comparison.

VolPlita requested a review from JanProvaznik August 3, 2026 13:47

VolPlita and others added 2 commits August 3, 2026 16:00

remove unnesessary comments

0ea00e4

Merge branch 'main' into fix-14345-unfilter-global-mesages-tl

2c4ee8c

Dedicated BuildMessageEventArgs subclass. Detection is now `e is Coo…

539dc33

…rdinatorWaitingForNodesEventArgs`

trim comments

155c1cb

trim comments

31c443d

VolPlita enabled auto-merge (squash) August 3, 2026 15:19

JanProvaznik requested changes Aug 4, 2026

View reviewed changes

AlesProkop requested changes Aug 11, 2026

View reviewed changes

Fix terminal logger coordinator message filtering. Keep the coordinat…

24e8852

…or event internal and add verbosity boundary + tests

		e is Microsoft.Build.Framework.Coordinator.CoordinatorWaitingForNodesEventArgs \|\|
		e is IExtendedBuildEventArgs { ExtendedType: Microsoft.Build.Framework.Coordinator.Constants.WaitingForNodesEventType };

Conversation

VolPlita commented Jul 14, 2026

Fixes: #14345

Issue

Root Cause

Changes

Testing

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JanProvaznik commented Jul 27, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

VolPlita commented Aug 3, 2026

Uh oh!

JanProvaznik left a comment

Choose a reason for hiding this comment

Uh oh!

VolPlita commented Aug 3, 2026

Uh oh!

JanProvaznik commented Aug 3, 2026

Uh oh!

VolPlita commented Aug 3, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants