Scope Helix Job Monitor to the current stage attempt - #17163
Conversation
The monitor discovered Helix jobs by build + stage across all pipeline attempts. When a stage was retried, each new monitor attempt re-discovered the same never-finishing work (e.g. work items permanently stranded in Waiting) and re-ran its full timeout, so it could never reach a terminal state. Scope discovery to the monitor's own stage attempt using the System.StageAttempt property the Helix submitter already stamps. On a stage retry the new attempt's submitters resubmit all work fresh, and the previous attempt's stranded/failed work is abandoned rather than re-discovered. When the attempt is unknown the monitor falls back to build + stage scope for back-compat. - HelixJobInfo: read/store StageAttempt from System.StageAttempt - JobMonitorOptions: add --stage-attempt (defaults to SYSTEM_STAGEATTEMPT) - JobMonitorRunner.IsInScope: add per-attempt filtering with empty-value fallback - HelixService: carry System.StageAttempt onto resubmitted jobs - helix-job-monitor.yml: pass --stage-attempt '$(System.StageAttempt)' - Design.md: document per-attempt scoping - Tests: previous-attempt job ignored, current tracked, unknown-attempt fallback Contributes to dotnet#17156 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 093b303a-ca5a-4f2d-80e6-6945a95a512b
There was a problem hiding this comment.
Pull request overview
This PR updates the Helix Job Monitor so Helix job discovery and tracking are scoped to the current Azure DevOps stage attempt, preventing a retried stage from re-discovering and re-waiting on never-finishing Helix work from a previous attempt (as described in #17156).
Changes:
- Add
System.StageAttemptplumbing end-to-end (job model, CLI option defaulting fromSYSTEM_STAGEATTEMPT, YAML template arg passing). - Update monitor scoping logic to filter Helix jobs by stage attempt, with a backward-compatible fallback when either side’s attempt is unknown.
- Extend the test suite to validate previous-attempt jobs are ignored, current-attempt jobs are tracked, and unknown-attempt behavior matches historical build+stage scoping.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/Microsoft.DotNet.Helix/Sdk.Tests/Microsoft.DotNet.Helix.Sdk.Tests/ScenarioHelpers/ScenarioHelpers.cs | Extends synthetic Helix job helper to include stageAttempt. |
| src/Microsoft.DotNet.Helix/Sdk.Tests/Microsoft.DotNet.Helix.Sdk.Tests/JobMonitorRunnerTests.cs | Adds coverage for attempt-scoped monitoring behaviors and fallback behavior. |
| src/Microsoft.DotNet.Helix/Sdk.Tests/Microsoft.DotNet.Helix.Sdk.Tests/Fakes/FakeHelixService.cs | Ensures resubmitted synthetic jobs preserve StageAttempt in tests. |
| src/Microsoft.DotNet.Helix/JobMonitor/Services/HelixService.cs | Reads System.StageAttempt and includes it in returned HelixJobInfo for resubmitted jobs. |
| src/Microsoft.DotNet.Helix/JobMonitor/Models/HelixJobInfo.cs | Captures and (for synthetic instances) emits System.StageAttempt in job properties. |
| src/Microsoft.DotNet.Helix/JobMonitor/JobMonitorRunner.Design.md | Documents the updated per-attempt scoping semantics and related behaviors. |
| src/Microsoft.DotNet.Helix/JobMonitor/JobMonitorRunner.cs | Implements stage-attempt scoping in IsInScope with unknown-attempt fallback. |
| src/Microsoft.DotNet.Helix/JobMonitor/JobMonitorOptions.cs | Adds --stage-attempt option, env defaulting, and option plumbing. |
| eng/common/core-templates/job/helix-job-monitor.yml | Passes --stage-attempt '$(System.StageAttempt)' to the monitor tool invocation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
premun
left a comment
There was a problem hiding this comment.
@mmitche this only solves when we re-run the whole stage but it does not solve when we time out and the helix jobs from the same attempt will never be completed, right? For that we need to resubmit all jobs that are incomplete when we're re-running the monitor only.
|
@premun I haven't looked at this yet, so not sure. On my todo list today. |
The first iteration scoped the monitor strictly to the current stage attempt and ignored all previous-attempt jobs. That breaks the "retry failed jobs" gesture: when only the monitor job re-runs (the Helix submitters passed and were not re-run), the current attempt contains no Helix work, so the monitor would exit 0 immediately and silently discard the previous attempt's results and failures. Rework the model to gate completion on the current attempt while reconciling previous-attempt work into it by resubmission, decided per logical work stream (AzDO System.JobName + Helix queue) rather than per attempt: - Retry pass walks the latest incarnation per stream across all attempts: current in-flight -> gate on it; previous in-flight (abandoned) -> resubmit not-yet-passed items; completed-with-failures -> resubmit; passed -> upload and count; un-resubmittable previous work -> record as failed and fail fast. - Resubmissions are stamped with the monitor's own stage attempt (threaded a targetStageAttempt param through IHelixService.ResubmitWorkItemsAsync) so the monitor gates on its own resubmission. - Poll loop separates stageJobs (all attempts, for upload/reconcile) from currentAttemptJobs (completion gating only). - Outcome ordering is attempt-aware so unlinked rerun duplicates on different attempts resolve deterministically (higher attempt wins). Corner cases are pinned by pipeline-emulating tests (AttemptScoped_* in JobMonitorRunnerTests): retry-only-monitor, stranded-Waiting previous work (mirrors dotnet#17156), fast-rerun no-double-submit, unlinked rerun duplicates, and fail-fast on a purged queue. Design.md updated to match. Contributes to dotnet#17156 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 093b303a-ca5a-4f2d-80e6-6945a95a512b
|
Updated this PR after review feedback: the earlier "scope strictly to the current attempt and ignore previous-attempt jobs" design was too naive. Why it was wrong: Azure DevOps'' retry-failed-jobs gesture re-runs only failed jobs. If the Helix submitters passed and only the monitor timed out, the submitters do not re-run, so the current attempt has zero Helix work — the naive monitor would exit 0 immediately and silently discard the previous attempt''s results and failures. There were also several related corner cases (resubmissions stamped with the wrong attempt, cancel-mid-retry, still-running previous incarnations during a fast rerun, unlinked rerun duplicates, and un-resubmittable/purged-queue work). New model: gate completion on the current attempt, but reconcile previous-attempt work into it by resubmission, decided per logical work stream (submitter + queue) rather than per attempt. Each of the six corner cases is now pinned by a pipeline-emulating |
| private bool IsPreviousAttempt(HelixJobInfo job) | ||
| => !string.IsNullOrEmpty(_options.StageAttempt) | ||
| && !string.IsNullOrEmpty(job.StageAttempt) | ||
| && MonitorState.ParseStageAttempt(job.StageAttempt) < MonitorState.ParseStageAttempt(_options.StageAttempt); |
Cover the end-to-end scenario spanning five stage attempts (with attempt 4 being a monitor crash where no invocation runs): each attempt resubmits only the still-unfinished streams, and a stream that has passed in any prior attempt is never resubmitted again. Includes the variant where a previously-Waiting stream has PASSED by the time the monitor is retried, so it is not resubmitted. Contributes to dotnet#17156 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 093b303a-ca5a-4f2d-80e6-6945a95a512b
| _failedWorkItemConsoleInfo[key] = new FailedWorkItemConsoleInfo( | ||
| job.DisplayName, | ||
| wi.Name, | ||
| "Abandoned (could not be resubmitted)", | ||
| job.DetailsUri); |
Review feedback: the Helix submitter (SendHelixJob) already stamps the stage attempt onto each job as the AzDO predefined-variable property System.StageAttempt. Make that a single source of truth (HelixJobInfo.StageAttemptPropertyName) and use it in both the read and the resubmission-write paths so the resubmitted job carries the exact same property name the submitter uses. Also add a test: when the monitor times out with a work item still Waiting and, on retry, that work item has completed, it is not resubmitted and its test results are processed (uploaded). Contributes to dotnet#17156 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 093b303a-ca5a-4f2d-80e6-6945a95a512b
premun
left a comment
There was a problem hiding this comment.
I went through it and I think it does make sense like this
| latest, failedWorkItems, _options.StageAttempt, cancellationToken); | ||
| if (resubmitted is null) | ||
| { | ||
| // Previous-attempt work that can never run again (e.g. its queue was removed) |
There was a problem hiding this comment.
| // Previous-attempt work that can never run again (e.g. its queue was removed) | |
| // Previous-attempt work that can never run again (e.g. its queue was purged) |
Purpose
Update the Helix Job Monitor so it gates completion on the current stage attempt while resubmitting a previous attempt''s unfinished/failed work into the current one, because today it re-discovers a previous attempt''s Helix jobs and can hang forever on work items permanently stranded in
Waiting(e.g. after a queue purge) — never terminating and re-burning its full timeout on every retry (#17156).Scoping strictly to the current attempt is not enough on its own: Azure DevOps'' retry-failed-jobs gesture re-runs only failed jobs, so if the Helix submitters passed and only the monitor timed out, the current attempt has no Helix work — and the monitor must still reconcile (upload results, resubmit failures) the previous attempt''s work rather than exit 0 and discard it.
What changed
--stage-attempt(SYSTEM_STAGEATTEMPT); completion gating considers only current-attempt Helix jobs.AttemptScoped_*tests cover the corner cases; all 179Microsoft.DotNet.Helix.Sdk.Testspass.JobMonitorRunner.Design.mdupdated.Contributes to #17156.