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.

Review — PR #14282 · 2 files, +55/−8

Methodology: portable core + dotnet/msbuild expert-reviewer · 24/24 dimensions evaluated
Mode: Deep (core engine logging / event routing)
Considered 2 existing comments (policy bot + @AlesProkop APPROVED) · 0 open threads · base branch is the stacked branch copilot/fix-forwarding-logger-issue, not main

Verdict: 🟡 COMMENT — address findings — 0 blocking, 0 major, 2 moderate, 2 nit · 20 of 24 evaluated dimensions clean

I verified the central claim by tracing the code at the PR head: RegisterLogger routes the pre-registered ReusableLogger through RegisterDistributedLoggerCore with a built-in CentralForwardingLogger that forwards all node events (in-proc + OOP) to the shared central sink the logger is initialized against. So the "no events lost / no duplicates" claim holds — the central logger keeps receiving every event through that path, and it is not doubly subscribed to the new per-forwarder sink. The findings below are about the second-order semantics and test depth, not correctness of the reported symptom.

⚠️ Needs attention (4)

🟡 MODERATE · Correctness & Edge Cases (logging routing) — when centralLoggerAlreadyInitialized, the paired forwarding logger's forwarded events land in a sink that has no subscriber, so the forwarding logger's filtering no longer feeds the central logger. · src/Build/BackEnd/Components/Logging/LoggingService.cs:~1147

• Scenario: user registers /DistributedLogger:Central,dll*Forwarding,dll where the central logger is also pre-seeded into ProjectCollection (the Report eval errors through cli loggers #12095 flow). The fix skips InitializeLogger(centralLogger, eventSourceSink), but still creates eventSourceSink, points the forwarding logger's EventRedirectorToSink at it, and adds it to _eventSinkDictionary. Nothing consumes that sink — the central logger instead receives all events via the CentralForwardingLogger path. If the user's forwarding logger filters (the entire purpose of a Central/Forwarding pair — e.g. "forward only errors"), the central logger now sees the unfiltered stream, not the forwarded subset.
• Why it matters: for an aggregating central logger (counts/collects only what its forwarder forwarded) this changes results vs. classic 17.x distributed-logger semantics. For the reported "just write a file" case it's harmless, which is why the symptom looks fixed.
• Confidence: Medium · traced RegisterLogger → RegisterDistributedLoggerCore → CentralForwardingLogger at the PR head; the author explicitly acknowledges this trade-off in the PR "Notes". This is a pragmatic, arguably-intended consequence of Report eval errors through cli loggers #12095, not a defect introduced here — but it should be called out and documented as a known limitation.
• Fix: none required if accepted; at minimum add a short comment on the created-but-unconsumed eventSourceSink explaining it is intentionally orphaned in this branch, and note in the issue/PR that forwarding-logger filtering is not honored for the central logger when the central logger is pre-registered.

🟡 MODERATE · Test Coverage & Completeness — the regression test asserts only that the forwarding logger is registered; it does not verify the two behavioral claims the PR rests on (central logger still receives every event; no duplicates). · src/Build.UnitTests/BackEnd/LoggingService_Tests.cs:600

• Scenario: RegisterDistributedLogger_WhenCentralLoggerAlreadyWrappedInReusableLogger_ForwardingLoggerIsRegistered checks result == true, LoggerDescriptions non-empty, and the forwarding type name is present. It never drives events through the service, so it cannot catch a future regression that drops central-logger events or double-delivers them.
• Why it matters: the risky part of this change is event delivery/duplication, and that is asserted only in prose. A refactor that breaks the CentralForwardingLogger path would leave this test green.
• Confidence: High · the test body is fully visible in the diff.
• Fix: extend the test (or add one) to log a small event set after registration and assert the central logger received each event exactly once and the per-forwarder sink received the forwarded subset.

⚪ NIT · Correctness & Edge Cases — splitting the guard removes duplicate-suppression for the ReusableLogger case: a second RegisterDistributedLogger with the same underlying central logger would now register the forwarding logger twice. · src/Build/BackEnd/Components/Logging/LoggingService.cs:~1138

• Scenario: because InitializeLogger(centralLogger, …) is skipped, centralLogger is never added to _loggers, so a repeated call re-satisfies centralLoggerAlreadyInitialized and proceeds again (unlike the direct-duplicate path guarded by _loggers.Contains). Low real-world likelihood — BuildManager registers each pair once — hence NIT.
• Fix: if you want parity with RegisterDuplicateDistributedCentralLogger, also short-circuit when an equivalent forwarding-logger description is already in _loggerDescriptions.

⚪ NIT · Scope & PR Discipline — the PR targets the stacked branch copilot/fix-forwarding-logger-issue, not main. · (design)

• Fix: confirm this is intentional (stacked on the sibling fix) and that the base eventually flows to main; otherwise retarget so the regression fix isn't stranded.

✅ Clean (20)

✅ Backwards Compatibility Vigilance — restores regressed 17.x behavior (forwarding logger Initialize called again); introduces no new warning/error, so no WarnAsError break.
✅ ChangeWave Discipline — a regression fix restoring prior behavior legitimately needs no ChangeWave gate.
✅ Performance & Allocation — runs once per build at logger registration, not a hot path; the added _loggers.Any(...) matches the pre-existing pattern.
✅ Error Message Quality — no user-facing messages added or changed.
✅ Logging & Diagnostics Rigor — event routing verified end-to-end; central logger fed via CentralForwardingLogger, no double subscription.
✅ String Comparison Correctness — logger identity uses reference equality (==/Contains), which is the correct intent here.
✅ API Surface Discipline — change is confined to a private method; no public surface touched.
✅ MSBuild Target Authoring Conventions — not applicable; no targets/tasks.
✅ Design Before Implementation — minimal split of an over-broad guard; preserves #12095's no-double-Initialize intent.
✅ Cross-Platform Correctness — no path/encoding/newline assumptions.
✅ Code Simplification — the two-case split is clearer than the original combined predicate.
✅ Concurrency & Thread Safety — executes under the existing lock (_lockObject); no new shared state.
✅ Naming Precision — centralLoggerAlreadyInitialized accurately names the condition.
✅ Idiomatic C# Patterns — consistent with surrounding code style.
✅ SDK Integration Boundaries — not applicable.
✅ File I/O & Path Handling — ConvertPathsToFullPaths() path unchanged.
✅ Documentation Accuracy — thorough inline rationale comment and an XML-doc'd test explaining the scenario.
✅ Build Infrastructure Care — no build/CI/manifest changes.
✅ Evaluation Model Integrity — no evaluation-order or evaluation-logging code altered.
✅ Security Awareness — no untrusted-input, secret, or trust-boundary surface.

☑️ Fix checklist

• 🟡 Correctness (logging) — document the intentionally-orphaned eventSourceSink and the "forwarding-logger filtering not honored for a pre-registered central logger" limitation (MODERATE)
• 🟡 Test Coverage — add an event-flow assertion (central logger gets each event once; forwarder sink gets the forwarded subset) (MODERATE)
• ⚪ Correctness — optionally guard against double distributed registration in the ReusableLogger case (NIT)
• ⚪ Scope — confirm the stacked base branch copilot/fix-forwarding-logger-issue is the intended merge target (NIT)

_{🤖 Generated with the ultimate-reviewer skill (portable core + dotnet/msbuild expert-reviewer). Advisory only — not a substitute for human sign-off.}

@AlesProkop the logger routing issue brought up by the expert review is definitely a thing that we need to actually fix/address

Expert review — PR #14282 current head 7cb9729

I re-reviewed the latest head after the address copilot comments commit. The previous filtering-routing concern appears addressed: the reusable wrapper is now rerouted to the dedicated distributed sink, and the focused tests *RegisterDistributedLogger_WhenCentralLoggerAlreadyWrapped* pass on net10.0.

Findings

🟠 MAJOR · Logging semantics / duplicate registration — duplicate distributed registrations for a central logger hidden behind ReusableLogger can reroute the wrapper and orphan the first forwarding sink.
src/Build/BackEnd/Components/Logging/LoggingService.cs:1139, src/Build/BackEnd/Components/Logging/LoggingService.cs:1177

• Scenario: a host supplies BuildParameters.Loggers = [new ReusableLogger(central)] and BuildParameters.ForwardingLoggers contains two ForwardingLoggerRecords for the same central instance, or otherwise calls RegisterDistributedLogger(central, ...) twice after the reusable wrapper has been registered. The first distributed registration reroutes the reusable wrapper to sink A. The second registration also succeeds, reroutes the same wrapper to sink B, and leaves sink A with no central subscriber. Events forwarded by the first forwarding logger are then silently dropped.
• Why it matters: direct central-logger registration rejects this duplicate case today (RegisterDuplicateDistributedCentralLogger / RegisterDuplicateCentralLogger assert that the second registration returns false and only one sink is registered). The reusable-wrapper path bypasses that guard because _loggers contains only the wrapper, not the underlying centralLogger, so duplicate behavior changes based on whether the same logger arrived directly or through ReusableLogger.
• Confidence: high. The only early duplicate guard is _loggers.Contains(centralLogger), and the wrapped path always calls existingReusableCentralLogger.RerouteActiveEventSource(eventSourceSink) for the first wrapper whose OriginalLogger == centralLogger. Nothing records that this underlying central logger has already been claimed by a distributed registration.
• Suggested fix: preserve the existing “same central logger can only be registered once” contract for the reusable-wrapper path. One way is to track distributed central logger identities by the unwrapped logger instance and return false on subsequent registrations. Add a regression test mirroring the existing duplicate-central test, but with a pre-registered ReusableLogger, asserting the second distributed registration returns false and the first forwarding sink still delivers events.

⚪ NIT · Scope / build infrastructure — this logger regression PR also changes the MicroBuildToolset feed endpoint.
azure-pipelines/.vsts-dotnet-build-jobs.yml:115

• Scenario: the signing plugin feed source changes from the devdiv.pkgs.visualstudio.com endpoint to the pkgs.dev.azure.com/devdiv endpoint while the rest of the PR is about runtime logger routing.
• Why it matters: the endpoint may be equivalent, but it is unrelated to the forwarding logger fix and adds build-infrastructure review surface to a targeted bug fix. If intentional, it should be called out with validation; otherwise it is safer to split/revert it here.
• Confidence: medium.
• Suggested fix: revert this line from this PR, or split it into a separate infrastructure PR with the reason and validation noted.

Validation I ran locally:

dotnet test src\Build.UnitTests\Microsoft.Build.Engine.UnitTests.csproj -f net10.0 -- --filter-method "*RegisterDistributedLogger_WhenCentralLoggerAlreadyWrapped*"

Result: passed, 2 tests.

Expert review - PR #14282 current head 214b69ff

Verdict: no blocking findings.

I re-reviewed the current diff after the latest fix. The logger-routing issue from the previous review appears addressed: when the central logger is already present through a ReusableLogger, RegisterDistributedLoggerCore now reroutes that wrapper to the distributed logger's dedicated sink, so the central logger receives the forwarding logger's filtered stream rather than the all-events central stream. The added duplicate-registration tracking also preserves the existing direct-registration contract by rejecting a second distributed registration for the same underlying central logger instead of orphaning the first sink.

Focused validation passed:

dotnet test src/Build.UnitTests/Microsoft.Build.Engine.UnitTests.csproj -- --filter-method "*RegisterDistributedLogger*"
net10.0: passed
net472: passed
total: 8 passed, 0 failed

Non-blocking notes:

1. azure-pipelines/.vsts-dotnet-build-jobs.yml still changes the MicroBuild signing plugin feed URL in a PR otherwise scoped to logging behavior. This may be intentional feed canonicalization, but it is unrelated to the forwarding logger regression and should be confirmed or split out if accidental.
2. The unit coverage now exercises the critical LoggingService behavior, including filtering and duplicate registration. There is still no end-to-end CLI/BuildParameters.ForwardingLoggers test for the exact /distributedlogger flow through ProjectCollection; I do not think that blocks this PR, but that path remains the main residual regression risk.

@baronfel Could you re-review please?

@AlesProkop Thanks for the fix, interesting to see the AI integration on a non-trivial issue and repository. Is there a schedule in which release the changes would be shipped?

We're working around the issue by just using a central logger and doing the filtering there, but that has a non-negligible overhead for our large parallel builds.

@AlesProkop Thanks for the fix, interesting to see the AI integration on a non-trivial issue and repository. Is there a schedule in which release the changes would be shipped?

We're working around the issue by just using a central logger and doing the filtering there, but that has a non-negligible overhead for our large parallel builds.

@danstur This should be shipped in the September VS feature update.