Rewrite the Helix Job Monitor for scalable result processing

[mmitche]

Summary Completely rewrite the Helix Job Monitor around a globally bounded, non-blocking result-processing pipeline while preserving its existing retry, replay, stage-attempt, upload, and pass/fail semantics. The rewrite is designed for hundreds of Helix jobs, thousands of work items, and millions of test results: - separate polling, status reporting, result processing, and finalization so uploads never block control-plane progress - use shared channel-based parallelism with bounded work-item and finalization queues - stream xUnit, JUnit, and TRX files with forward-only XML readers - lazily batch Azure DevOps requests by the 1,000 top-level-result service limit - coordinate Azure DevOps throttling across all workers - preserve whole-job durable replay through completed test-run tags - emit fixed five-minute status updates throughout normal execution and final drain - report aggregate HTTP, throttling, throughput, backlog, and stage-latency metrics ## Semantic behavior - Terminal Helix work items become upload-eligible incrementally while their job is running; the Azure DevOps test run is finalized only after the job completes and all admitted uploads finish. - A job is durably processed only after every work item succeeds and its Azure DevOps test run is completed and tagged. - Interrupted or failed uploads remain untagged and are replayed in full by a later monitor invocation. - Retry is entry-only and reconciles previous stage attempts into the current attempt without waiting forever on abandoned work. - Newer resubmissions and stage attempts supersede older work-item outcomes. - Independent streams use Azure DevOps phase identity, Helix queue, and logical Helix job identity so same-named work items cannot overwrite failures from sibling jobs. - Upload failures do not alter the test outcome, but Helix exit-code failures and failed uploaded tests do. ## Result processing - Replace the previous publisher with a streaming parser and efficient UTF-8 request writer. - Split oversized nested result hierarchies while allowing up to 1,000 top-level results per request. - Recognize Helix artifacts with a single trailing .txt transport suffix, including *.testResults.xml.txt and .trx.txt. - Warn for recognized result files with unsupported XML roots instead of silently reporting zero results. - Keep attachment handling and failed-test metadata associated with the correct work item and test run. ## Parallelism and performance - Use one global work-item concurrency budget rather than multiplying concurrency by job, work item, and result file. - Keep lightweight completed-job descriptors non-dropping while bounding expensive work. - Make test-run creation single-flight, including synchronous failures. - Avoid retrying ambiguous non-idempotent test-run creation; retry idempotent test-run completion PATCH operations. - Default result upload parallelism to 48 based on full dotnet/runtime validation. Representative full runtime runs: | Build | Parallelism | Jobs | Work items | Results | Final drain | Drain share | | --- | ---: | ---: | ---: | ---: | ---: | ---: | | 1550792 | 64 | 66 | 6,842 | 3,076,533 | 2m29s | 1.81% | | 1551686 | 48 | 66 | 6,842 | 3,077,632 | 1m53s | 1.33% | At 48 workers, server-directed deferrals dropped from 1,459 to 1,080 and shared-gate waits dropped from 2,429 to 2,061 while keeping final drain below 2%. ## Logging and diagnostics - Report immutable aggregate status every five minutes, including during drain. - Keep normal and verbose logging bounded; do not emit per-request or per-result noise. - Include final-poll eligibility, earlier backlog, remaining finalizations, request attempts, retries, payload volume, service guidance, shared-gate waits, throughput, and maximum stage latency. - Mark cancellation metrics as partial rather than presenting incomplete data as a normal completed run. ## Design documentation The new design set lives under src/Microsoft.DotNet.Helix/JobMonitor/Design: - SemanticBehavior.md - Architecture.md - Components/Polling.md - Components/UploadPipeline.md - Components/TestResults.md - Components/StateAndStatus.md - Components/PerformanceMetrics.md - Components/Shutdown.md ## Validation - 280 Microsoft.DotNet.Helix.Sdk.Tests tests pass. - Existing Job Monitor scenarios were audited against the pre-rewrite suite and retained at the behavioral level. - Added regression coverage for replay, cancellation, stage retries, non-idempotent creation failures, partial publication failures, throttling, hierarchy splitting, fixed status cadence, .xml.txt discovery, independent same-queue/phase work-item accounting, build-scoped discovery, and incremental result upload. - Integrated the intent of #17373: every Helix discovery request sends both Source and Properties[BuildId]; source-only historical scans are prohibited, responses are defensively revalidated, and a 1,000-job boundary fails closed rather than truncating silently. | Repository | PR | Build | Status | | --- | --- | --- | --- | | Arcade | #17331 | 1562513 | Passed | | runtime | #131969 | 1562514 | Job Monitor validation passed; build reported two runtime test failures | | aspnetcore | #68706 | 1563698 | Running | | SDK | #55894 | 1563697 | Running | The full runtime run processed 66 jobs, 6,920 work items, and 3,087,694 results with zero speculative resubmissions, zero discovery HTTP 500s, zero untagged uploads, and a 1.19-second final drain. The runtime build retry validated attempt-aware replay: it selected exactly the two failed-AzDO-test work items, created two linked descendants preserving submitter attempt 1 while stamping stage/monitor attempt 2, reused the 64 unaffected tagged jobs without re-uploading their roughly 3.09 million results, suppressed stale parent failures, left zero untagged jobs, and drained in 0.78 seconds.

[Copilot]

Pull request overview

This PR rewrites the Helix Job Monitor to handle very large Helix workloads by decoupling polling/status from result processing and introducing a globally bounded, channel-based upload pipeline with shared Azure DevOps throttling and streaming XML parsing, while preserving the existing retry/replay and pass/fail semantics.

Changes:

• Replace the per-job fire-and-forget upload queue with a globally bounded pipeline (job expansion → work-item processing → finalization) plus shared rate-limit gating and performance metrics.
• Move/reshape Azure DevOps test-result publishing into the Job Monitor (streaming XML reader, aggregation, batching/splitting) and extend result-file discovery to handle Helix’s .xml.txt/.trx.txt transport suffix.
• Add design documentation and update templates/tests to reflect new semantics, defaults (48 upload workers), and logging.

Reviewed changes

Copilot reviewed 40 out of 51 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
src/Microsoft.DotNet.Helix/Sdk/Readme.md	Links to new Job Monitor design docs and updates behavioral notes for the new pipeline/streaming model.
src/Microsoft.DotNet.Helix/Sdk.Tests/Microsoft.DotNet.Helix.Sdk.Tests/ScenarioHelpers/ScenarioHelpers.cs	Extends HelixJobInfo test helper to include logical job name and submitter phase name.
src/Microsoft.DotNet.Helix/Sdk.Tests/Microsoft.DotNet.Helix.Sdk.Tests/Microsoft.DotNet.Helix.Sdk.Tests.csproj	Removes reference to the deleted AzureDevOpsTestPublisher project.
src/Microsoft.DotNet.Helix/Sdk.Tests/Microsoft.DotNet.Helix.Sdk.Tests/LocalTestResultsReaderTests.cs	Adds coverage for .txt-suffixed result artifacts and updates existing test to use .xml.txt.
src/Microsoft.DotNet.Helix/Sdk.Tests/Microsoft.DotNet.Helix.Sdk.Tests/JobMonitorRunnerTests.cs	Updates and expands end-to-end runner tests for pipeline drain/status cadence, bounded concurrency, replay semantics, and new logging.
src/Microsoft.DotNet.Helix/Sdk.Tests/Microsoft.DotNet.Helix.Sdk.Tests/HelixServiceTests.cs	Updates Helix download tests for .xml.txt handling and new per-work-item download API.
src/Microsoft.DotNet.Helix/Sdk.Tests/Microsoft.DotNet.Helix.Sdk.Tests/Fakes/FakeHelixService.cs	Adapts fake Helix service to per-work-item downloads and adds list-work-items call counting for polling optimizations.
src/Microsoft.DotNet.Helix/Sdk.Tests/Microsoft.DotNet.Helix.Sdk.Tests/Fakes/FakeAzureDevOpsService.cs	Adapts fake AzDO service to per-work-item uploads and adds concurrent-upload accounting used by new pipeline tests.
src/Microsoft.DotNet.Helix/Sdk.Tests/Microsoft.DotNet.Helix.Sdk.Tests/AzureDevOpsResultPublisherTests.cs	Adds/updates publisher tests for batching/splitting, throttling delay selection, lazy enumeration behavior, and new default parallelism.
src/Microsoft.DotNet.Helix/JobMonitor/TestResultUploadQueue.cs	Removes old fire-and-forget upload queue implementation.
src/Microsoft.DotNet.Helix/JobMonitor/TestResultUploadPipeline.cs	Introduces the new bounded, multi-stage upload pipeline with per-job single-flight test-run creation and finalization.
src/Microsoft.DotNet.Helix/JobMonitor/TestResults/TestResultUploadSummary.cs	Adds upload summary model now hosted in Job Monitor after publisher project removal.
src/Microsoft.DotNet.Helix/JobMonitor/TestResults/TestNameFormatter.cs	Adds formatting logic for AzDO display titles when fully qualified names are enabled.
src/Microsoft.DotNet.Helix/JobMonitor/TestResults/ResultAggregator.cs	Adds aggregation logic (reruns/data-driven/flaky/FQN grouping) now hosted under Job Monitor.
src/Microsoft.DotNet.Helix/JobMonitor/TestResults/Model/TestResultAttachmentMode.cs	Adds attachment-mode model under new Job Monitor-hosted publisher surface.
src/Microsoft.DotNet.Helix/JobMonitor/TestResults/Model/TestResultAttachment.cs	Adds attachment model under new Job Monitor-hosted publisher surface.
src/Microsoft.DotNet.Helix/JobMonitor/TestResults/Model/TestResult.cs	Adds normalized test-result model including stable FullyQualifiedName identity.
src/Microsoft.DotNet.Helix/JobMonitor/TestResults/Model/TerminalError.cs	Adds internal exception type for terminal publisher failures.
src/Microsoft.DotNet.Helix/JobMonitor/TestResults/Model/PackedTestReport.cs	Adds model for packed report payload representation.
src/Microsoft.DotNet.Helix/JobMonitor/TestResults/Model/AzureDevOpsReportingParameters.cs	Adds consolidated AzDO reporting parameter record used by publisher.
src/Microsoft.DotNet.Helix/JobMonitor/TestResults/Model/AzureDevOpsReportingError.cs	Adds publisher exception type for AzDO reporting failures.
src/Microsoft.DotNet.Helix/JobMonitor/TestResults/LocalTestResultsReader.cs	Adds streaming XML reader that supports xUnit/JUnit/TRX and warns on unsupported roots; strips a single trailing .txt.
src/Microsoft.DotNet.Helix/JobMonitor/TestResults/AzureDevOpsResultPublisher.cs	Reworks publisher to stream/partition requests (1,000 top-level results), split deep hierarchies, and share throttling/metrics.
src/Microsoft.DotNet.Helix/JobMonitor/StatusReporter.cs	Separates status reporting from Helix calls and adds bounded pipeline/metrics reporting.
src/Microsoft.DotNet.Helix/JobMonitor/Services/HelixService.cs	Changes downloads to per-work-item and adds metrics for Helix and blob-download operations; stamps logical job metadata.
src/Microsoft.DotNet.Helix/JobMonitor/Services/AzureDevOpsService.cs	Switches upload API to per-work-item, adds shared rate-limit gating + metrics recording, and reuses a shared HttpClient.
src/Microsoft.DotNet.Helix/JobMonitor/Services/AzureDevOpsRateLimitGate.cs	Adds a shared deferral gate to coordinate AzDO throttling across concurrent workers.
src/Microsoft.DotNet.Helix/JobMonitor/Parallelism/ParallelAsync.cs	Adds helper for bounded parallel snapshot reads (e.g., list-work-items fanout).
src/Microsoft.DotNet.Helix/JobMonitor/Parallelism/ActionQueue.cs	Adds reusable channel-based worker queue with bounded/unbounded modes and queue metrics.
src/Microsoft.DotNet.Helix/JobMonitor/MonitorState.cs	Updates stream identity keying (phase/queue/logical job), adds single-result observation helper, and adjusts lineage root logic.
src/Microsoft.DotNet.Helix/JobMonitor/Models/HelixJobInfo.cs	Adds logical job name + submitter phase name properties and persists them into Helix job properties.
src/Microsoft.DotNet.Helix/JobMonitor/Microsoft.DotNet.Helix.JobMonitor.csproj	Enables implicit usings/nullable annotations and removes AzureDevOpsTestPublisher project reference; keeps InternalsVisibleTo for tests.
src/Microsoft.DotNet.Helix/JobMonitor/JobMonitorRunner.cs	Refactors runner to use cached work-item snapshots, fixed-cadence status task, new pipeline drain/final metrics, and new production dependency wiring.
src/Microsoft.DotNet.Helix/JobMonitor/JobMonitorOptions.cs	Changes default result upload parallelism to 48 and updates CLI defaults accordingly.
src/Microsoft.DotNet.Helix/JobMonitor/JobMonitorMetrics.cs	Adds aggregate request/throughput/latency counters and snapshots used for final status reporting.
src/Microsoft.DotNet.Helix/JobMonitor/Interfaces/IHelixService.cs	Updates Helix service contract to per-work-item result downloads.
src/Microsoft.DotNet.Helix/JobMonitor/Interfaces/IAzureDevOpsService.cs	Updates AzDO service contract to per-work-item uploads.
src/Microsoft.DotNet.Helix/JobMonitor/Design/SemanticBehavior.md	Updates/renames spec to reflect new stream key identity and bounded pipeline semantics.
src/Microsoft.DotNet.Helix/JobMonitor/Design/README.md	Adds top-level index for the new design documentation set.
src/Microsoft.DotNet.Helix/JobMonitor/Design/Components/UploadPipeline.md	Documents the new multi-stage upload pipeline behavior and invariants.
src/Microsoft.DotNet.Helix/JobMonitor/Design/Components/TestResults.md	Documents streaming parsing, .txt suffix handling, aggregation, and AzDO limits/batching.
src/Microsoft.DotNet.Helix/JobMonitor/Design/Components/StateAndStatus.md	Documents state ownership and timer-driven status reporting.
src/Microsoft.DotNet.Helix/JobMonitor/Design/Components/Shutdown.md	Documents drain, cancellation behavior, and crash/replay durability boundary.
src/Microsoft.DotNet.Helix/JobMonitor/Design/Components/Polling.md	Documents polling snapshot reuse and bounded work-item refresh behavior.
src/Microsoft.DotNet.Helix/JobMonitor/Design/Components/PerformanceMetrics.md	Documents aggregate metrics captured without per-request logging noise.
src/Microsoft.DotNet.Helix/JobMonitor/Design/Architecture.md	Documents ownership boundaries, shared parallelism utilities, data flow, and durability boundary.
src/Microsoft.DotNet.Helix/AzureDevOpsTestPublisher/Microsoft.DotNet.Helix.AzureDevOpsTestPublisher.csproj	Removes the standalone publisher project (Job Monitor is now the sole host/consumer).
src/Microsoft.DotNet.Helix/AzureDevOpsTestPublisher/LocalTestResultsReader.cs	Removes the old DOM-based reader in favor of the new streaming reader in Job Monitor.
eng/common/core-templates/job/helix-job-monitor.yml	Adds pipeline-template parameter to flow upload parallelism into the monitor invocation.
Arcade.slnx	Removes the deleted AzureDevOpsTestPublisher project from the solution.
.gitignore	Un-ignores the new JobMonitor/TestResults directory (previously matched by MSTest TestResult*/ ignore).

---

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

[Copilot]

Pull request overview

Copilot reviewed 40 out of 51 changed files in this pull request and generated no new comments.

Suppressed comments (1)

src/Microsoft.DotNet.Helix/JobMonitor/Interfaces/IHelixService.cs:36

• The XML doc comment for DownloadTestResultsAsync still describes returning results for multiple work items and says work items may be omitted. The API now returns a single WorkItemTestResults, so callers will always get an instance and should interpret TestResultFiles being empty as “no recognized results”. Updating the comment avoids misleading consumers and future maintainers.

        /// Work items without recognizable test result files may be omitted from the result.
        /// Individual file download failures should not prevent other result files from being downloaded.
        /// </summary>
        Task<WorkItemTestResults> DownloadTestResultsAsync(
            string jobName,

[Copilot]

Pull request overview

Copilot reviewed 39 out of 51 changed files in this pull request and generated no new comments.

Suppressed comments (1)

src/Microsoft.DotNet.Helix/JobMonitor/Interfaces/IHelixService.cs:34

• The XML doc for DownloadTestResultsAsync still describes returning metadata for multiple work items and says work items without recognizable result files may be omitted. The method now downloads results for a single work item and always returns a WorkItemTestResults (possibly with an empty TestResultFiles list), so the contract comment is misleading for implementers/callers.

        /// Downloads test result files for a completed Helix job's work items
        /// and returns metadata about each work item's results.
        /// Work items without recognizable test result files may be omitted from the result.
        /// Individual file download failures should not prevent other result files from being downloaded.
        /// </summary>

[Copilot]

Pull request overview

Copilot reviewed 39 out of 51 changed files in this pull request and generated no new comments.

Suppressed comments (2)

src/Microsoft.DotNet.Helix/JobMonitor/Parallelism/ActionQueue.cs:88

• EnqueueAsync increments _accepted after awaiting WriteAsync. Since workers can start processing as soon as the write completes, _started can be incremented before _accepted, again allowing transient negative queued counts. Increment _accepted before writing and roll it back if the write fails/cancels.

    public async ValueTask EnqueueAsync(T item, CancellationToken cancellationToken)
    {
        await _channel.Writer.WriteAsync(item, cancellationToken);
        Interlocked.Increment(ref _accepted);
    }

src/Microsoft.DotNet.Helix/JobMonitor/Parallelism/ActionQueue.cs:82

• QueueSnapshot.Queued is computed as Accepted - Started, but TryEnqueue increments _accepted after the item is visible to readers. A worker can read and increment _started before _accepted is updated, briefly producing negative queued counts in snapshots/logging.

This issue also appears on line 84 of the same file.

    public bool TryEnqueue(T item)
    {
        if (!_channel.Writer.TryWrite(item))
        {
            return false;
        }

        Interlocked.Increment(ref _accepted);
        return true;
    }

[Copilot]

Pull request overview

Copilot reviewed 39 out of 51 changed files in this pull request and generated no new comments.

Suppressed comments (2)

src/Microsoft.DotNet.Helix/JobMonitor/Services/AzureDevOpsService.cs:620

• HonorRateLimitAsync both defers the shared rate-limit gate and then awaits it immediately. Because SendForStringAsync already runs inside an ExponentialRetry loop, this can cause rate-limit delay to be paid twice (once here, and again via the retry loop), increasing latency and reducing throughput. Deferring the gate without awaiting it here lets the retry loop’s delay “consume” part/all of the guidance and the next request’s pre-send WaitAsync will only wait any remaining time (effectively max(backoff, guidance) rather than sum).

            if (delayToApply > TimeSpan.Zero)
            {
                _rateLimitGate.Defer(delayToApply);
                _logger.LogDebug(
                    "Azure DevOps rate limit back-off. Delaying next request by {DelaySeconds:0.###}s (request: {RequestUri}).",
                    delayToApply.TotalSeconds,
                    requestUri);
                await _rateLimitGate.WaitAsync(cancellationToken);
            }

src/Microsoft.DotNet.Helix/JobMonitor/Services/HelixService.cs:89

• The Helix client call to ListFilesAsync isn’t passed the method CancellationToken, so cancellation won’t stop this request (it will use the default CancellationToken.None). This can delay shutdown and interfere with the monitor’s timeout/cancel semantics.

            JobResultsUri resultsUri = await RetryAsync(() => _helixApi.Job.ResultsAsync(jobName), cancellationToken);
            IImmutableList<UploadedFile> availableFiles = await RetryAsync(
                () => _helixApi.WorkItem.ListFilesAsync(workItemName, jobName, false),
                cancellationToken);

[mmitche]

Why is necessary to lock here?

[mmitche]

The lock is needed because the snapshot must represent one coherent point across all interdependent collections. Each collection is copied while holding the same lock, and callers enumerate only the immutable copies after the lock is released.

[mmitche]

Is reutrning 1 in a failed parsing attempt the correct wayt og o? In what cases would this fail?

[mmitche]

It may be better to let the monitor throw.

[mmitche]

Missing metadata remains compatible and maps to attempt 1. A present malformed, zero, or negative value now throws InvalidOperationException because it violates the submitter contract. Added focused coverage for both missing and malformed cases.

[mmitche]

Changed accordingly: present malformed/non-positive attempt metadata now throws. Only absent legacy metadata defaults to attempt 1.

[Copilot]

Pull request overview

Copilot reviewed 42 out of 54 changed files in this pull request and generated no new comments.

Suppressed comments (2)

src/Microsoft.DotNet.Helix/JobMonitor/Services/AzureDevOpsService.cs:590

• IsTransientException currently treats any HttpRequestException as transient, which will cause retries (and exponential backoff) even for non-transient 4xx responses that are surfaced as HttpRequestException by ThrowForFailure for control requests. This can add unnecessary latency and request volume for terminal client errors. Consider only treating HttpRequestException as transient when it has no status code (network failure) or when the status is 5xx / 429.

        internal static bool IsTransientException(Exception exception, CancellationToken cancellationToken)
            => !cancellationToken.IsCancellationRequested
                && exception is OperationCanceledException { InnerException: TimeoutException }
                    or HttpRequestException
                or TimeoutException
                or SocketException
                or IOException;

src/Microsoft.DotNet.Helix/Sdk.Tests/Microsoft.DotNet.Helix.Sdk.Tests/AzureDevOpsServiceTests.cs:153

• This test relies on a wall-clock threshold (< 500ms) to prove that CompleteTestRunAsync doesn't honor its own rate-limit delay. On slower/loaded CI agents, that tight bound can lead to intermittent failures unrelated to the behavior being validated. Consider loosening the bound (or using a deterministic signal instead of elapsed time) to reduce flakiness.

[Copilot]

Pull request overview

Copilot reviewed 42 out of 54 changed files in this pull request and generated no new comments.

Suppressed comments (2)

src/Microsoft.DotNet.Helix/JobMonitor/TestResultUploadPipeline.cs:116

• _remainingWorkItemsByPoll/_acceptedWorkItemsByPoll are updated after enqueuing the job expansion. Because ActionQueue workers can start processing immediately, a work item can finish and decrement the counters before the increment runs, which can permanently undercount backlog and final-drain metrics (the decrement path currently adds 0 when the key is missing). Consider updating the per-poll counters before publishing the request into _jobs so the key exists before any worker can run.

        _remainingWorkItemsByPoll.AddOrUpdate(
            discoveryPoll,
            newWorkItems.Count,
            (_, remaining) => remaining + newWorkItems.Count);
        _acceptedWorkItemsByPoll.AddOrUpdate(

src/Microsoft.DotNet.Helix/JobMonitor/Services/AzureDevOpsService.cs:590

• IsTransientException currently treats any HttpRequestException as transient. That includes the HttpRequestException instances you explicitly throw for non-transient 4xx control-plane responses in ThrowForFailure, so those responses will be retried needlessly (and may amplify rate limiting / slow failures). It’s safer to treat HttpRequestException as transient only for transport failures (StatusCode == null) or for the explicit transient-status wrapper exception used for 5xx/429.

        internal static bool IsTransientException(Exception exception, CancellationToken cancellationToken)
            => !cancellationToken.IsCancellationRequested
                && exception is OperationCanceledException { InnerException: TimeoutException }
                    or HttpRequestException
                or TimeoutException

[Copilot]

Pull request overview

Copilot reviewed 42 out of 54 changed files in this pull request and generated 1 comment.

[github-actions]

🔴 Build Failure Analysis

Both build legs fail with the same error (CS0103) in HelixService.cs.

Root Cause

GetJobsForBuildAsync creates a local variable called filterProperties but then references an undefined name properties when calling _helixApi.Job.ListAsync(...):

// Defined:
var filterProperties = new Dictionary<string, string>()
{
    ["BuildId"] = buildId,
}.ToImmutableDictionary();

// Bug — 'properties' is not defined in this scope:
IImmutableList<JobSummary> jobs = await RetryAsync(
    async () => await _helixApi.Job.ListAsync(
        source: source,
        properties: properties,          // ← should be filterProperties
        count: MaximumJobsPerBuildQuery),
    cancellationToken);

Error: CS0103 — The name 'properties' does not exist in the current context
File: src/Microsoft.DotNet.Helix/JobMonitor/Services/HelixService.cs, line 77

Fix

Change properties: properties → properties: filterProperties (see inline suggestion).

🤖 Automated content by GitHub Copilot. Generated by the Build Failure Analysis workflow.{ai_credits_suffix} · [◷]( · ◷)

[github-actions]

🤖 Automated content by GitHub Copilot. Generated by the Build Failure Analysis workflow.{ai_credits_suffix} · ◷

[Copilot]

Pull request overview

Copilot reviewed 42 out of 54 changed files in this pull request and generated no new comments.

Suppressed comments (2)

src/Microsoft.DotNet.Helix/JobMonitor/Services/HelixService.cs:112

• DownloadTestResultsAsync accepts a CancellationToken, but the Helix API calls for job results SAS and file listing currently rely on optional-parameter defaults, so they do not observe the caller token. This can delay cancellation/shutdown, since these requests may continue running even after the monitor is cancelled.

Pass cancellationToken into ResultsAsync and ListFilesAsync so Helix control-plane I/O is promptly cancelable.

            JobResultsUri resultsUri = await RetryAsync(() => _helixApi.Job.ResultsAsync(jobName), cancellationToken);
            IImmutableList<UploadedFile> availableFiles = await RetryAsync(
                () => _helixApi.WorkItem.ListFilesAsync(workItemName, jobName, false),
                cancellationToken);

src/Microsoft.DotNet.Helix/JobMonitor/Services/AzureDevOpsService.cs:590

• IsTransientException treats all HttpRequestException as transient. That includes the HttpRequestException you throw for non-transient 4xx control-plane failures (e.g. a 400/404 from a control request), which will cause unnecessary retries and can mask genuine request problems.

Consider classifying only network/timeout HttpRequestException instances (no StatusCode) as transient, and explicitly treating TransientAzureDevOpsRequestException (5xx/429) as transient instead of the broad HttpRequestException match.

        internal static bool IsTransientException(Exception exception, CancellationToken cancellationToken)
            => !cancellationToken.IsCancellationRequested
                && exception is OperationCanceledException { InnerException: TimeoutException }
                    or HttpRequestException
                or TimeoutException
                or SocketException
                or IOException;

[Copilot]

Pull request overview

Copilot reviewed 42 out of 54 changed files in this pull request and generated no new comments.

Suppressed comments (2)

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

src/Microsoft.DotNet.Helix/JobMonitor/Services/AzureDevOpsService.cs:590

• IsTransientException currently treats all HttpRequestExceptions as transient. After this rewrite, non-transient 4xx control-plane failures are thrown as HttpRequestException (see ThrowForFailure), so they will be retried up to ControlRequestAttemptCount times instead of failing fast. This can hide misconfiguration (e.g., bad request/endpoint) and waste time.

        internal static bool IsTransientException(Exception exception, CancellationToken cancellationToken)
            => !cancellationToken.IsCancellationRequested
                && exception is OperationCanceledException { InnerException: TimeoutException }
                    or HttpRequestException
                or TimeoutException

src/Microsoft.DotNet.Helix/JobMonitor/Services/HelixService.cs:112

• DownloadTestResultsAsync always calls Job.ResultsAsync(jobName) even when the work item has no recognizable result files. Since the SAS from ResultsAsync is only needed for blob downloads, this adds an extra Helix API call per work item that can be skipped when availableFiles is empty (reduces latency/throttling on large runs).

            JobResultsUri resultsUri = await RetryAsync(() => _helixApi.Job.ResultsAsync(jobName), cancellationToken);
            IImmutableList<UploadedFile> availableFiles = await RetryAsync(
                () => _helixApi.WorkItem.ListFilesAsync(workItemName, jobName, false),
                cancellationToken);

[Copilot]

Pull request overview

Copilot reviewed 42 out of 54 changed files in this pull request and generated 1 comment.