Benchmark MSBuildCache phase telemetry

[Jan Provazník (JanProvaznik)]

Builds instrumented MSBuildCache packages from microsoft/MSBuildCache#179 and enables per-node phase timing in the existing seeded and consumer static-graph builds. This draft exists only to collect realistic Azure Pipelines measurements.

[Copilot]

🟡 Changes recommended

Cache matching, failure fallback, and task conditioning issues must be resolved before approval.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Adds temporary MSBuildCache phase telemetry to Azure Pipelines.

Changes:

• Builds pinned, instrumented MSBuildCache packages.
• Enables phase timing in seed and consumer graph builds.

File summaries

File	Description
eng/pipelines/steps/build-msbuildcache-telemetry.yml	Builds telemetry-enabled packages.
azure-pipelines.yml	Uses telemetry packages and enables timing logs.

Review details

• Files reviewed: 2/2 changed files
• Comments generated: 3
• Review effort level: Balanced

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🟡 Changes recommended

The telemetry may measure cold misses, and package preparation can run unnecessarily or prevent the existing fallback.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (4)

azure-pipelines.yml:191

• These package-selection switches become project global properties, but the existing cache entries were seeded from main without either value. MSBuildCache includes non-ignored global properties in each node descriptor, and PR #179 only adds the timing switch—not these two properties—to its ignore list. Since this seed stage only runs for refs/heads/main, this draft cannot populate matching entries before its PR consumer runs, so the collected data will describe cold misses rather than realistic cache-hit latency. Add a telemetry seed path that runs before the consumer (ideally in an isolated cache universe) so both builds use identical node inputs.

          "/p:MSBuildCachePackageVersion=0.1.999-phase-telemetry",
          "/p:RestoreAdditionalProjectSources=$(Agent.TempDirectory)\MSBuildCacheTelemetryPackages",

eng/pipelines/steps/build-msbuildcache-telemetry.yml:19

• Any clone or pack failure here terminates the job before the existing non-blocking cache/fallback logic runs. A transient GitHub outage, deleted/updated source branch, or experimental package build failure will therefore block TestFX validation instead of handing off to the regular Arcade build. Catch preparation failures, publish a readiness variable, and gate the cache build on it so the fallback remains available.

    if ((git -C $source rev-parse HEAD) -ne $commit) {
      throw "Expected MSBuildCache commit $commit."
    }

eng/pipelines/steps/build-msbuildcache-telemetry.yml:28

• This dotnet pack invocation runs MSBuild without the repository-required binary logger, leaving failures in the temporary Azure Pipelines package build without a diagnostic binlog. Add a unique -bl output.

      --configuration Release `
      --output $packages `
      "-p:PackageVersion=$version"
    if ($LASTEXITCODE -ne 0) {

eng/pipelines/steps/build-msbuildcache-telemetry.yml:36

• This second MSBuild-based pack also omits the mandatory binary logger, so a SharedCompilation packaging failure cannot be inspected through a binlog. Add a distinct unique -bl output.

      --configuration Release `
      --output $packages `
      "-p:PackageVersion=$version"
    if ($LASTEXITCODE -ne 0) {

• Files reviewed: 2/2 changed files
• Comments generated: 1
• Review effort level: Balanced

[Copilot]

🟡 Changes recommended

Moderate pipeline reliability and execution-scope issues must be addressed before approval.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (3)

azure-pipelines.yml:459

• This template runs for every consumer Windows matrix job, but the cache build at line 603 is skipped for fork PRs and for non-PR runs outside main. Those jobs now clone and pack the external repository in both Debug and Release without consuming the packages, adding avoidable time and an extra network/build failure point. Parameterize the template's step condition and pass the same eligibility condition used by the cache build; the seed stage can keep its current unconditional behavior.

        - template: /eng/pipelines/steps/build-msbuildcache-telemetry.yml

eng/pipelines/steps/build-msbuildcache-telemetry.yml:22

• UseDotNet@2 sets a job-level DOTNET_ROOT to its shared tools directory, while this assignment only changes the current PowerShell process. The later TestFX build scripts prepend the repo-local CLI but do not restore DOTNET_ROOT, so subsequent dotnet test/apphost launches no longer honor the repo-local runtimes explicitly configured in test-env-vars.yml. Reset the pipeline variable for subsequent tasks while retaining the .NET 10 root in this process.

    $env:DOTNET_ROOT = $dotnetRoot

eng/pipelines/steps/build-msbuildcache-telemetry.yml:15

• A transient failure cloning or packing this experimental dependency now fails the Windows job before the existing MSBuildCache fallback can run. This makes ordinary validation depend on GitHub availability and on the external branch remaining cloneable, despite the surrounding pipeline explicitly routing cache infrastructure failures to the regular Arcade build. Have this template report package preparation success without failing the job, condition the graph build on that result, and otherwise enter the existing fallback path.

    git clone --depth 1 --branch dev/janprovaznik/cache-phase-telemetry https://github.com/JanProvaznik/MSBuildCache $source

• Files reviewed: 2/2 changed files
• Comments generated: 1
• Review effort level: Balanced

[Copilot]

🟡 Changes recommended

The critical SDK/package-build failures and cache measurement correctness issues must be resolved before approval.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (2)

azure-pipelines.yml:513

• The PR consumer cannot reuse the existing main cache with these two command-line properties. MSBuildCache includes every non-ignored ProjectInstance.GlobalProperties entry in its node descriptor, and the instrumented package ignores only MSBuildCacheLogCacheOperationTimings, not MSBuildCachePackageVersion or RestoreAdditionalProjectSources. Because the seed stage runs only on main, PR validation has no entries created with these new properties and will measure cache misses rather than the intended realistic cache-hit latency. Keep the package-selection properties out of the descriptor while preserving the default ignore list, or arrange a seed with identical properties before consuming it.

              "/p:MSBuildCachePackageVersion=0.1.999-phase-telemetry",
              "/p:RestoreAdditionalProjectSources=$(Agent.TempDirectory)\MSBuildCacheTelemetryPackages",

azure-pipelines.yml:459

• This setup runs for every Windows matrix job, while its only consumer is conditionally skipped for fork PRs and non-main non-PR builds at line 603. Those jobs now clone and pack an external repository without using the result, and a transient setup failure prevents the normal fallback build from running because its condition requires succeeded(). Apply the same runtime condition to the telemetry setup steps, for example through a template parameter.

        - template: /eng/pipelines/steps/build-msbuildcache-telemetry.yml

• Files reviewed: 2/2 changed files
• Comments generated: 2
• Review effort level: Balanced

[Copilot]

🟡 Changes recommended

Critical pipeline dependency and SDK setup defects must be resolved before approval.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

azure-pipelines.yml:449

• This package-building template runs unconditionally, while the cache build below excludes fork PRs and non-main branch runs. Those jobs still install an SDK, clone an external repository, and pack twice—and any transient failure blocks the required Windows job even though no cache attempt would run. Apply the same eligibility condition to the template tasks, and route setup failures to the Arcade fallback.

        - template: /eng/pipelines/steps/build-msbuildcache-telemetry.yml

• Files reviewed: 2/2 changed files
• Comments generated: 4
• Review effort level: Balanced

[Copilot]

🟡 Changes recommended

The SDK lookup is broken, and pipeline conditions and fallback behavior can skip or duplicate required builds.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (9)

azure-pipelines.yml:662

• This message and variable do not actually skip the regular Arcade build: the CIBuild.cmd step at line 738 has no condition, so it runs after every successful cache build. This defeats the cache path and contaminates the benchmark with a second full build. Add the same and(succeeded(), ne(variables['MSBuildCacheBuildSucceeded'], 'true')) condition used by the seed-stage fallback.

            Write-Host "##vso[task.setvariable variable=MSBuildCacheBuildSucceeded]true"
            Write-Host "Cached build outputs are restored, signed and packed. The Arcade 'Build' step will be skipped."

azure-pipelines.yml:449

• The instrumented-package template runs unconditionally, but the cache build at line 593 explicitly excludes fork PRs and non-main branch runs. Those jobs still clone, restore, and pack the external project even though the result cannot be used, adding avoidable latency and making otherwise valid builds depend on two external feeds. Apply the same eligibility condition to both steps in the template (for example via a template parameter).

        - template: /eng/pipelines/steps/build-msbuildcache-telemetry.yml

azure-pipelines.yml:122

• This condition skips the new first stage for PR, scheduled, and manual non-main runs. Because the following build stage has no explicit dependsOn/condition, Azure implicitly makes it depend on this stage and its default succeeded() condition is false when the dependency is skipped, so normal PR validation is skipped as well. Give build an explicit condition that accepts Succeeded, SucceededWithIssues, and Skipped seed results while still rejecting failures.

  condition: and(succeeded(), in(variables['Build.Reason'], 'IndividualCI', 'BatchedCI'), eq(variables['Build.SourceBranch'], 'refs/heads/main'))

azure-pipelines.yml:580

• The error text cannot prove that Arcade will reproduce the failure: an executed project may be compiling against a dependency output materialized by the cache, and a stale/incorrect cache hit can surface as an ordinary CS/BC error with none of the cache tokens above. This branch then blocks the PR instead of exercising the stated correctness fallback. During this cache experiment, route every nonzero cache build to the clean Arcade build (or only fail fast after reproducing the error there).

            if ($reportedErrorCount -gt 0 -and $errorLines.Count -gt 0 -and $mayHelpLines.Count -eq 0 -and $fallbackCanDifferLines.Count -eq 0) {

eng/pipelines/steps/build-msbuildcache-telemetry.yml:7

• Failures while installing the SDK, cloning the fork, restoring, or packing happen before either caller's guarded cache-build script. Any transient external failure therefore skips the advertised Arcade fallback and fails required CI outright. Make this preparation report failure through a variable/output and let callers bypass the cache path while still running the regular build.

- pwsh: |

eng/pipelines/steps/build-msbuildcache-telemetry.yml:34

• This MSBuild-based restore does not generate a binary log. Add a unique /bl switch so failures retain the required diagnostic trace.

    <?xml version="1.0" encoding="utf-8"?>
    <configuration>
      <packageSources>

eng/pipelines/steps/build-msbuildcache-telemetry.yml:42

• This MSBuild-based pack does not generate a binary log. Add a unique /bl switch so failures retain the required diagnostic trace.

      </packageSources>
    </configuration>
    '@ | Set-Content $nugetConfig

    $azurePipelinesProject = "$source\src\AzurePipelines\Microsoft.MSBuildCache.AzurePipelines.csproj"

eng/pipelines/steps/build-msbuildcache-telemetry.yml:50

• This MSBuild-based restore does not generate a binary log. Add a unique /bl switch so failures retain the required diagnostic trace.

    & $dotnet pack $azurePipelinesProject `
      --no-restore `
      --configuration Release `

eng/pipelines/steps/build-msbuildcache-telemetry.yml:58

• This MSBuild-based pack does not generate a binary log. Add a unique /bl switch so failures retain the required diagnostic trace.

      throw "Failed to pack Microsoft.MSBuildCache.AzurePipelines."
    }

    $sharedCompilationProject = "$source\src\SharedCompilation\Microsoft.MSBuildCache.SharedCompilation.csproj"
    & $dotnet restore $sharedCompilationProject `

• Files reviewed: 2/2 changed files
• Comments generated: 1
• Review effort level: Balanced

[Copilot]

🟡 Changes recommended

The critical SDK lookup failure and multiple pipeline control-flow issues must be resolved.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (8)

azure-pipelines.yml:122

• This stage is now the implicit dependency of the following build stage. On PR, scheduled, and manual runs where this condition is false, Azure skips this stage and then skips build because its default dependency did not succeed. Give build an explicit condition that accepts a Skipped seed result (while retaining the ordering for main CI).

  condition: and(succeeded(), in(variables['Build.Reason'], 'IndividualCI', 'BatchedCI'), eq(variables['Build.SourceBranch'], 'refs/heads/main'))

azure-pipelines.yml:449

• This template runs unconditionally, while the cache build below excludes fork PRs and non-main branch runs. Those builds still clone, restore, and pack an unused external dependency—and can fail before their normal Arcade build. Apply the cache-build eligibility condition to package preparation as well.

        - template: /eng/pipelines/steps/build-msbuildcache-telemetry.yml

azure-pipelines.yml:662

• Setting this variable does not currently skip the regular CIBuild.cmd step at line 738, which has no condition. Every successful cache hit is therefore followed by a full Arcade build despite this message, doubling the measured job work. Add the same ne(variables['MSBuildCacheBuildSucceeded'], 'true') condition used by the seed fallback at line 265.

            Write-Host "##vso[task.setvariable variable=MSBuildCacheBuildSucceeded]true"
            Write-Host "Cached build outputs are restored, signed and packed. The Arcade 'Build' step will be skipped."

eng/pipelines/steps/build-msbuildcache-telemetry.yml:21

• The telemetry package setup is outside both cache fallback traps. Any GitHub, feed, restore, or pack failure therefore fails the job before the regular Arcade build can run, contradicting the non-blocking rollout design. Route setup failures into the same fallback state instead of terminating the job.

    git clone --depth 1 --branch dev/janprovaznik/cache-phase-telemetry https://github.com/JanProvaznik/MSBuildCache $source

eng/pipelines/steps/build-msbuildcache-telemetry.yml:54

• This new MSBuild restore invocation does not generate a binary log, leaving no diagnostic trace when dependency restoration fails. Add a unique /bl argument.

    $azurePipelinesProject = "$source\src\AzurePipelines\Microsoft.MSBuildCache.AzurePipelines.csproj"

eng/pipelines/steps/build-msbuildcache-telemetry.yml:62

• This new pack invocation omits the required binary log, so package-build failures cannot be inspected without rerunning the external build. Add a unique /bl argument.

      throw "Failed to restore Microsoft.MSBuildCache.AzurePipelines."
    }
    & $dotnet pack $azurePipelinesProject `
      --no-restore `
      --configuration Release `

eng/pipelines/steps/build-msbuildcache-telemetry.yml:69

• This second restore also lacks a binary log, so failures in the SharedCompilation dependency setup lose the MSBuild diagnostic data. Add a unique /bl argument.

    $sharedCompilationProject = "$source\src\SharedCompilation\Microsoft.MSBuildCache.SharedCompilation.csproj"

eng/pipelines/steps/build-msbuildcache-telemetry.yml:77

• The SharedCompilation pack command likewise needs its own binary log so this custom package build is diagnosable on failure. Add a unique /bl argument.

      throw "Failed to restore Microsoft.MSBuildCache.SharedCompilation."
    }
    & $dotnet pack $sharedCompilationProject `
      --no-restore `
      --configuration Release `

• Files reviewed: 2/2 changed files
• Comments generated: 1
• Review effort level: Balanced

[Copilot]

🟡 Changes recommended

Multiple critical and moderate pipeline correctness and credential-exposure issues remain unresolved.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (4)

azure-pipelines.yml:122

• This condition skips the new first stage on PR, scheduled, and manual runs, while the following build stage implicitly depends on it and retains the default succeeded() condition. A skipped dependency therefore skips the entire existing build stage, so the PR consumer build—and normal validation—never runs. Give build a condition that also accepts dependencies.msbuild_cache_seed.result == 'Skipped' (and SucceededWithIssues) while preserving the dependency for main-branch seeding.

- stage: msbuild_cache_seed
  displayName: Seed MSBuildCache and publish coverage
  condition: and(succeeded(), in(variables['Build.Reason'], 'IndividualCI', 'BatchedCI'), eq(variables['Build.SourceBranch'], 'refs/heads/main'))

azure-pipelines.yml:503

• The repository's package source mapping does not include this temporary source, so RestoreAdditionalProjectSources is ignored for the custom Microsoft.MSBuildCache.* package and this consumer restore cannot resolve version 0.1.999-phase-telemetry. Use a generated NuGet config that declares and maps the local source, as for the seed build.

              "/p:MSBuildCachePackageEnabled=true",
              "/p:MSBuildCachePackageVersion=0.1.999-phase-telemetry",
              "/p:RestoreAdditionalProjectSources=$(Agent.TempDirectory)\MSBuildCacheTelemetryPackages",

azure-pipelines.yml:726

• The regular Arcade Build step immediately below this new block has no condition, so it runs even when MSBuildCacheBuildSucceeded remains true. This contradicts the success messages and rebuilds the solution after every successful cache hit, defeating the intended cached consumer path. Gate that step on the cache result, as the seed-stage fallback already does.

          condition: and(always(), or(and(eq(variables['Build.Reason'], 'PullRequest'), ne(variables['System.PullRequest.IsFork'], 'True')), and(ne(variables['Build.Reason'], 'PullRequest'), eq(variables['Build.SourceBranch'], 'refs/heads/main'))))

azure-pipelines.yml:449

• This template runs before the cache consumer's condition at line 593. For fork PRs and non-main manual runs the consumer is skipped, but both matrix legs still install an SDK, clone an external repository, restore, and pack two projects; any failure also breaks validation that was not using the cache. Apply the same trusted-PR/main condition to the package-building steps.

        - template: /eng/pipelines/steps/build-msbuildcache-telemetry.yml

• Files reviewed: 2/2 changed files
• Comments generated: 3
• Review effort level: Balanced

[Copilot]

🟡 Changes recommended

The critical SDK setup defect and multiple moderate pipeline issues must be resolved.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (6)

azure-pipelines.yml:122

• This conditional stage becomes the implicit dependency of the following build stage. On PR, manual, and scheduled runs this condition is false, so the seed is Skipped; the downstream stage's default succeeded() condition then also evaluates false and skips all existing validation. Add an explicit dependency/condition to build that accepts both Succeeded and Skipped seed results (while still waiting for the seed when it runs).

  condition: and(succeeded(), in(variables['Build.Reason'], 'IndividualCI', 'BatchedCI'), eq(variables['Build.SourceBranch'], 'refs/heads/main'))

azure-pipelines.yml:449

• This template runs unconditionally, while the consuming cache step at line 593 excludes fork PRs and non-main manual/scheduled builds. Those jobs therefore clone, restore, and pack an external repository—and can fail—despite never using the resulting packages. Apply the same runtime eligibility condition to the package-build steps, preferably through a template condition parameter.

        - template: /eng/pipelines/steps/build-msbuildcache-telemetry.yml

azure-pipelines.yml:726

• The Arcade Build task immediately after this cleanup still has no condition, so it always executes even when MSBuildCacheBuildSucceeded is true. That discards the cache time saving and contaminates the telemetry run with a second full build. Gate the fallback on the cache result.

          condition: and(always(), or(and(eq(variables['Build.Reason'], 'PullRequest'), ne(variables['System.PullRequest.IsFork'], 'True')), and(ne(variables['Build.Reason'], 'PullRequest'), eq(variables['Build.SourceBranch'], 'refs/heads/main'))))

azure-pipelines.yml:186

• The repository NuGet.config enables Package Source Mapping and maps only its named feeds (NuGet.config:36-58). This dynamically added local source has no mapping, so NuGet will not consider it for Microsoft.MSBuildCache.*; the seed restore falls back before loading the instrumented package. Generate/use a restore config that declares and maps the local source.

          "/p:MSBuildCachePackageEnabled=true",
          "/p:MSBuildCachePackageVersion=0.1.999-phase-telemetry",
          "/p:RestoreAdditionalProjectSources=$(Agent.TempDirectory)\MSBuildCacheTelemetryPackages",

azure-pipelines.yml:503

• This consumer restore has the same unmapped-source problem: Package Source Mapping is active in NuGet.config:36-58, but MSBuildCacheTelemetryPackages is added only as an additional source. NuGet therefore cannot resolve the custom Microsoft.MSBuildCache.* version, so every consumer run takes the fallback and produces no phase telemetry. Use a restore config that declares and maps this local source.

              "/p:MSBuildCachePackageEnabled=true",
              "/p:MSBuildCachePackageVersion=0.1.999-phase-telemetry",
              "/p:RestoreAdditionalProjectSources=$(Agent.TempDirectory)\MSBuildCacheTelemetryPackages",

eng/pipelines/steps/build-msbuildcache-telemetry.yml:58

• A restore failure here terminates the job before the guarded cache build runs. Because the cleanup/fallback steps require a successful prior step, a transient clone, SDK, feed, restore, or pack failure bypasses the advertised Arcade fallback and fails validation outright. Treat telemetry-package setup as cache infrastructure: record setup failure, keep the job green, and condition the cache invocation on setup success so the regular build can run.

    if ($LASTEXITCODE -ne 0) {
      throw "Failed to restore Microsoft.MSBuildCache.AzurePipelines."

• Files reviewed: 2/2 changed files
• Comments generated: 1
• Review effort level: Balanced

[Copilot]

🔵 Needs a closer look

Four moderate pipeline issues must be resolved before approval.

Review details

Suppressed comments (4)

azure-pipelines.yml:304

• This stage implicitly depends on msbuild_cache_seed and retains the default succeeded() condition. When the seed condition is false—as it is for PR, scheduled, and manual runs—the seed is Skipped, so this build stage is skipped as well; the PR cannot collect consumer telemetry and nightly validation no longer runs. Keep the ordering for main CI, but explicitly allow a skipped seed.

- stage: build
  displayName: Build

azure-pipelines.yml:725

• The handoff never suppresses the existing CIBuild.cmd step at lines 738–745: it has no condition, so the full Arcade build runs even when MSBuildCacheBuildSucceeded remains true. Add the same guard used by the seed path; otherwise the cache does not replace the consumer build and the preceding “will be skipped” messages are incorrect.

          displayName: Preserve cache diagnostics and clean failed outputs

azure-pipelines.yml:449

• The instrumented-package template runs before the graph step's eligibility condition. Fork PRs and non-main manual builds therefore clone/restore/build the external repository even though they never use the package, and any failure prevents the normal Arcade build from running. Apply the same eligibility condition to these package-building steps and route package-build failures to the fallback path.

        - template: /eng/pipelines/steps/build-msbuildcache-telemetry.yml

eng/pipelines/steps/build-msbuildcache-telemetry.yml:5

• 10.x installs the latest matching SDK under $(Agent.ToolsDirectory)/dotnet by default, but the script searches immediate children of $(Agent.TempDirectory) for the exact 10.0.400 directory. On a normal agent no candidate is found (and the wildcard may install a newer patch), so this template stops before producing packages. Pin the requested SDK and give the task the same explicit installation directory that the script consumes.

    version: 10.x

• Files reviewed: 2/2 changed files
• Comments generated: 0 new
• Review effort level: Balanced

[Copilot]

🟡 Changes recommended

Critical SDK setup and multiple fallback-flow issues must be resolved before approval.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (3)

azure-pipelines.yml:529

• Setting this variable does not skip the existing Arcade Build task at lines 739-746, because that task has no condition and therefore still runs after every successful cache build. This rebuilds the complete solution after the cached graph and defeats the intended cache-output handoff. Add condition: and(succeeded(), ne(variables['MSBuildCacheBuildSucceeded'], 'true')) to that task.

              Write-Host "##vso[task.setvariable variable=MSBuildCacheBuildSucceeded]true"
              Write-Host "MSBuildCache produced the build outputs. The Arcade 'Build' step will be skipped."

azure-pipelines.yml:588

• This fail-fast classification assumes an executed project cannot depend on incorrect outputs materialized from a cache hit. A downstream compiler can emit an ordinary CSxxxx error against a stale or incomplete cached dependency, while the clean Arcade build would rebuild that dependency and succeed; the diagnostic on line 587 even acknowledges this case. Route every failed cached graph through the uncached fallback and let that build determine whether the sources are broken.

            if ($reportedErrorCount -gt 0 -and $errorLines.Count -gt 0 -and $mayHelpLines.Count -eq 0 -and $fallbackCanDifferLines.Count -eq 0) {
              foreach ($line in @($errorLines | Select-Object -First 20)) {
                Write-Host "##vso[task.logissue type=error]$line"
              }

              Write-Host "The solution does not build. The Arcade 'Build' step would report the same $reportedErrorCount error(s), so the build is failed here instead of repeating it."
              Write-Host "If you believe an error above is an artifact of a cached dependency rather than a source break, the cache diagnostics for this run are published under artifacts/log/$(_BuildConfig)/MSBuildCache."
              exit 1

eng/pipelines/steps/build-msbuildcache-telemetry.yml:23

• This package-preparation script throws on clone, restore, or build failures before either caller reaches its guarded cache step. Such an external GitHub/feed failure therefore leaves the job failed and prevents the normal Arcade fallback from running, despite the rollout comments promising that infrastructure failures fall back. Make package preparation report availability without failing the job, condition the cache attempt on that result, and allow the regular build to proceed.

    git clone --branch dev/janprovaznik/cache-phase-telemetry https://github.com/JanProvaznik/MSBuildCache $source
    if ((git -C $source rev-parse HEAD) -ne $commit) {
      throw "Expected MSBuildCache commit $commit."

• Files reviewed: 2/2 changed files
• Comments generated: 1
• Review effort level: Balanced

[Copilot]

🟡 Changes recommended

Critical SDK setup, token exposure, gating, redundant-build, and fallback issues remain unresolved.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (6)

Previously missed (1) — in code that hasn't changed since the last review.

azure-pipelines.yml:144

• This template is a hard-failing prerequisite outside the seed step's fallback boundary. A transient clone, restore, or package-build failure skips the seed and Arcade fallback, fails this stage, and then prevents the build stage because its condition excludes Failed. Route package-preparation failures to the regular build fallback instead.

azure-pipelines.yml:213

• This MSBuild process writes MSBuildCache.binlog and inherits SYSTEM_ACCESSTOKEN; MSBuild binlogs record environment properties, and lines 296–301 publish that raw binlog. Excluding CacheClient.log does not protect the token. Remove the raw binlog from publication or use cache authentication that is not inherited by the logged MSBuild process.

      env:
        SYSTEM_ACCESSTOKEN: $(System.AccessToken)

azure-pipelines.yml:451

• This template runs on every Windows leg, but the cache graph step is restricted at line 596 to trusted PRs and main. Fork PRs and non-main manual runs therefore clone and build external packages they never consume, and a failure blocks the ordinary Arcade build. Gate package preparation with the same eligibility condition as the cache step.

        - template: /eng/pipelines/steps/build-msbuildcache-telemetry.yml

azure-pipelines.yml:598

• This cache build writes MSBuildCache.binlog under artifacts/log, which the job publishes, while inheriting SYSTEM_ACCESSTOKEN. MSBuild binlogs capture environment properties, so the OAuth token can be exposed through the artifact even though CacheClient.log is excluded. Do not publish the raw binlog while this token is present, or move authentication out of the logged process environment.

          env:
            SYSTEM_ACCESSTOKEN: $(System.AccessToken)

azure-pipelines.yml:665

• Setting this variable to true does not skip the regular CIBuild.cmd step at lines 741–748, because that step has no condition. Every successful cache run therefore rebuilds the solution despite this message, defeating the intended cached-output path. Add the same ne(variables['MSBuildCacheBuildSucceeded'], 'true') condition used by the seed fallback.

            Write-Host "##vso[task.setvariable variable=MSBuildCacheBuildSucceeded]true"
            Write-Host "Cached build outputs are restored, signed and packed. The Arcade 'Build' step will be skipped."

eng/pipelines/steps/build-msbuildcache-telemetry.yml:7

• Any clone, restore, or package-build failure here terminates the job before the cache invocation's Arcade fallback can run. In the seed stage it also makes the later build stage skip because that stage excludes a failed seed. Make package preparation set a success variable, gate the cache invocation on it, and leave the regular build eligible so transient external-repository/feed failures do not suppress validation.

- pwsh: |

• Files reviewed: 2/2 changed files
• Comments generated: 1
• Review effort level: Balanced

[Copilot]

🟡 Changes recommended

Critical setup failure and unresolved authorization, fallback, execution-gating, benchmarking, and secret-exposure issues block approval.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (8)

Previously missed (2) — in code that hasn't changed since the last review.

azure-pipelines.yml:122

• This seed stage never runs for the draft PR described as collecting cache-hit measurements, while the PR consumer is read-only. The custom package version is also a non-ignored global property in MSBuildCache fingerprints, so existing entries produced with another package version cannot supply representative hits. Provide a trusted, isolated seed path for this PR before measuring the consumer.
  azure-pipelines.yml:188
• The Azure Pipelines cache plugin requires the OAuth token to be granted Pipeline Cache scope via EnablePipelineCache=true or an active Cache@2 task. This pipeline defines neither: both test-template call sites set enableAffectedTests: false, so their compile-time Cache@2 branches are omitted. Remote seed/read operations will therefore fail authorization unless the scope is explicitly enabled.

azure-pipelines.yml:664

• The regular CIBuild.cmd step at lines 739–746 has no condition, so it still runs after this variable is set to true. Consequently every successful cache build is immediately followed by a full Arcade build, which defeats the stated skip/fallback behavior and distorts the benchmark. Add condition: and(succeeded(), ne(variables['MSBuildCacheBuildSucceeded'], 'true')) to that regular Build step, as the seed-stage fallback already does.

            Write-Host "##vso[task.setvariable variable=MSBuildCacheBuildSucceeded]true"
            Write-Host "Cached build outputs are restored, signed and packed. The Arcade 'Build' step will be skipped."
            exit 0

azure-pipelines.yml:450

• The telemetry-package template runs unconditionally, while the cache graph step below excludes fork PRs. Fork validation therefore installs another SDK, clones an external repository, and restores/builds two unused packages; any failure also blocks the normal build. Apply the same trusted-run condition to the template's tasks.

        - template: /eng/pipelines/steps/build-msbuildcache-telemetry.yml

azure-pipelines.yml:177

• This binlog is published from artifacts/log while the same MSBuild process receives SYSTEM_ACCESSTOKEN. MSBuild binary logs can capture environment-derived properties in plaintext, and artifact publication is not secret-redacted. Do not publish the raw binlog containing the cache OAuth token; keep it outside the artifact path or remove/sanitize it first.

          "/bl:$binlogDirectory\MSBuildCache.binlog",

azure-pipelines.yml:495

• The consumer binlog is collected by the job's logs artifact while this MSBuild invocation receives SYSTEM_ACCESSTOKEN. Binary logs can persist environment-derived secrets without Azure's console masking, so publishing this file can expose the cache OAuth token. Keep this binlog outside published logs or sanitize it before artifact upload.

              "/bl:$binlogDirectory\MSBuildCache.binlog",

eng/pipelines/steps/build-msbuildcache-telemetry.yml:2

• Both tasks in this template run unconditionally, while the consuming cache step is skipped for fork PRs and non-main branch runs (azure-pipelines.yml:594). Those jobs now install an SDK and clone/build external telemetry packages they never consume, and any failure aborts the normal build before its fallback path. Gate both template tasks with the same trusted-PR/main condition used by the cache step.

- task: UseDotNet@2

eng/pipelines/steps/build-msbuildcache-telemetry.yml:7

• This optional cache prerequisite is outside the fallback handling. A clone, feed, restore, or package-build failure terminates the job before either guarded cache build runs, so the regular Arcade fallback is skipped on both main and PR validation. Route setup failures to the same fallback path rather than making the experimental dependency a hard blocker.

- pwsh: |

• Files reviewed: 2/2 changed files
• Comments generated: 1
• Review effort level: Balanced