Skip to content

Harden build-failure-analysis Azure DevOps fetch (raise artifact cap, fail closed, timeouts) - #55985

Merged
YuliiaKovalova merged 6 commits into
dotnet:mainfrom
YuliiaKovalova:fix-build-failure-analysis-robustness
Aug 28, 2026
Merged

Harden build-failure-analysis Azure DevOps fetch (raise artifact cap, fail closed, timeouts)#55985
YuliiaKovalova merged 6 commits into
dotnet:mainfrom
YuliiaKovalova:fix-build-failure-analysis-robustness

Conversation

@YuliiaKovalova

@YuliiaKovalova YuliiaKovalova commented Aug 28, 2026

Copy link
Copy Markdown
Member

Robustness fixes to the shared Azure DevOps fetch step, all found while validating the equivalent workflow on dotnet/roslyn#85046 end to end against real failing builds. The same fixes are going to microsoft/testfx (microsoft/testfx#10835) and dotnet/arcade, which carry sibling copies of this workflow.

1. Raise the per-artifact download cap: 500 MB -> 2 GB

MAX_ZIP_BYTES is a download guard, not a size expectation. An artifact that exceeds it is silently dropped from the analysis rather than reported, so a too-tight value hides the exact failure the workflow exists to explain.

On roslyn this fired for real: a 636 MB analyzer-logs artifact tripped the 500 MB cap and the run completed "successfully" having analysed nothing (run 33161086176). After raising the cap the same build produced a correct root cause and an inline fix suggestion (run 33161255171).

This repo has more headroom than the others -- the largest *_Logs_Attempt* artifact on definition 101 currently measures ~153 MB -- so this is preventative here rather than urgent. The cumulative budgets (MAX_TOTAL_ZIP_BYTES 3 GB compressed, MAX_TOTAL_BYTES 4 GB uncompressed) and MAX_UNZIP_BYTES are unchanged and are what actually bound the runner's disk and network, so this raises no real resource ceiling.

2. Fail closed on Azure DevOps fetch / non-JSON responses

A failed curl or an HTML error page produced an empty .value/.records, which is indistinguishable from a healthy build -- the step reported "no failed jobs" and the agent stayed inert. A transient ADO outage therefore looked green.

All metadata fetches now go through an ado_get helper that checks the curl exit status and validates the body parses as JSON, reporting a data-resolution failure instead. It returns a status code rather than calling emit_none directly, because emit_none inside a command substitution would only exit the subshell. The timeline fetch already had equivalent fail-closed handling and is left as is.

3. Bound Azure DevOps request time, and make the download deadline hard

Retries without timeouts meant a stalled endpoint hung the step until the whole job timed out. Two rounds of review sharpened this considerably:

  • Metadata requests use --connect-timeout 10 --max-time 20 --retry-max-time 40. These are small JSON documents from an endpoint that normally answers in well under a second, and there are three of them before any download begins, so their combined retry windows must not eat into the job on their own.
  • --max-time is per attempt, so --retry 3 alone would have permitted four full transfers plus backoff — roughly 20 minutes against timeout-minutes: 15. The download loop now has a wall-clock deadline (DOWNLOAD_BUDGET=420, MAX_ATTEMPT_SECONDS=120) and every transfer derives both --max-time and --retry-max-time from the time actually left.
  • --retry-max-time only gates whether a new retry may start, so the whole invocation is additionally wrapped in timeout "${TIME_LEFT}". That is what makes the deadline real; without it a retry begun just inside the window could overshoot by a further attempt.

A transfer killed at the deadline is treated like any other failed download: the leg is reported missing and the analysis is disabled rather than run on a partial picture.

5. Enforce the download budgets, and never analyze a stale binlog

  • The compressed-budget and time-budget guards now actually break out of the loop. ulimit -f takes 512-byte blocks and the block count is rounded up, so a small remaining allowance still buys at least one block instead of flooring to zero and failing every write. (This is a budget, not a byte-exact ceiling: a transfer can overshoot its cap by under 512 bytes before the size check rejects it.)
  • The binlog directory is cleared immediately after it is created. The extract and upload steps glob the whole directory, so anything left by an earlier run on the same runner would otherwise be attributed to this build.

4. Validate GITHUB_OUTPUT once up front

Previously unchecked; the success path writes six outputs. The guard now proves the path is writable with a zero-byte append, not merely non-empty — a set-but-unwritable path passed the old check and then failed every append, emitting no outputs at all. Both the no-op and success paths fail loudly rather than writing into nothing.

Validation

  • gh aw compile build-failure-analysis build-failure-analysis-command --strict — 2 succeeded, 0 warnings; both .lock.yml files regenerated and committed. The diff contains no uses: changes.
  • shellcheck and bash -n over the fetch script extracted from every generated .lock.yml — clean.
  • A checker parses the generated workflows and asserts the invariants directly, so they cannot silently rot: the worst-case download time fits the job's timeout-minutes; every retrying curl bounds its whole retry window; the download is wrapped in timeout; every budget guard actually leaves the loop; and the binlog directory is cleared. It reproduced a real regression during this work — a bad scripted substitution had replaced a break with a literal None, which both bash -n and shellcheck happily accept.
  • End to end on a fork against real failing Azure DevOps builds, most recently run 33172351332: all seven jobs green, the large log artifact downloaded with no cap or budget warnings, and a correct root cause with an inline fix suggestion.

The same fixes are applied to the sibling copies of this workflow in dotnet/roslyn, dotnet/runtime, dotnet/sdk and dotnet/arcade.

Four robustness fixes found while validating the equivalent workflow on
dotnet/roslyn (dotnet/roslyn#85046) end to end against real failing builds:

- Raise the per-artifact download cap from 500 MB to 2 GB. This cap is a
  download guard, not a size expectation, and an artifact that exceeds it
  is silently dropped rather than reported -- so a too-tight value hides
  the exact failure the workflow exists to explain. On roslyn a 636 MB
  analyzer-logs artifact tripped the 500 MB cap and the run produced no
  analysis at all. The cumulative compressed and uncompressed budgets
  still bound the runner's disk and network.
- Fail closed when an Azure DevOps metadata request fails or returns a
  non-JSON body. Previously a transient outage produced an empty
  '.value'/'.records' and was reported as "no failed jobs", which is
  indistinguishable from a healthy build. Routed through a new ado_get
  helper that checks the curl exit status and validates the payload.
- Bound Azure DevOps request time (--connect-timeout 15 --max-time 60 on
  metadata, --connect-timeout 15 on the artifact download); previously a
  stalled endpoint hung the step until the whole job timed out.
- Validate GITHUB_OUTPUT once up front, so both the no-op path and the
  success path fail loudly instead of writing outputs into nothing.

The regenerated lock files also pick up the pending compiler/action-pin
refresh: these two workflows were last compiled with gh aw v0.82.9 while
.github/aw/actions-lock.json already records v0.86.2, so recompiling
brings them in line with the pins the repo has already adopted.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI lite review requested due to automatic review settings August 28, 2026 10:22
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 1 pipeline(s).
2 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR hardens the “build-failure-analysis” workflow’s Azure DevOps fetch logic to avoid silent no-op analyses and reduce hang risk when Azure DevOps is slow/unavailable. It updates the shared fetch step and then regenerates the compiled workflow lock files so the runtime workflows reflect those robustness changes.

Changes:

  • Adds an ado_get helper to validate Azure DevOps metadata fetches (curl status + JSON parse) and applies timeouts to metadata requests.
  • Raises the per-artifact compressed download guard (MAX_ZIP_BYTES) from 500 MB to 2 GB and adds a connect-timeout to key requests.
  • Regenerates *.lock.yml workflows to incorporate the updated shared fetch logic and newer gh-aw compiler output.

Reviewed changes

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

File Description
.github/workflows/shared/build-failure-analysis-fetch.md Implements the hardened ADO fetch helper, timeouts, and increases per-artifact download cap; adds a GITHUB_OUTPUT check.
.github/workflows/build-failure-analysis.lock.yml Regenerated compiled workflow reflecting the shared fetch updates (plus compiler/version refresh).
.github/workflows/build-failure-analysis-command.lock.yml Regenerated compiled slash-command workflow reflecting the shared fetch updates (plus compiler/version refresh).

Comment thread .github/workflows/shared/build-failure-analysis-fetch.md Outdated
Addresses review feedback on this PR:

- `--max-time 1200` exceeded this job's own `timeout-minutes: 15`, so a
  transfer that stalled after connecting would consume the whole job and be
  killed before the script could emit its controlled no-op -- the exact
  failure mode the timeouts were added to prevent. Restored a 5-minute
  per-artifact budget, which is still very generous: the full artifact set
  for the equivalent roslyn build downloads in ~30 seconds.

- The cumulative compressed budget was only checked after a transfer
  finished, so a download could start just below MAX_TOTAL_ZIP_BYTES and
  still pull a further MAX_ZIP_BYTES, making the real ceiling
  MAX_TOTAL_ZIP_BYTES + MAX_ZIP_BYTES. Each transfer is now bounded by
  min(MAX_ZIP_BYTES, remaining budget) before it starts.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Two review findings that turned out to apply to every repo carrying this
workflow rather than only where they were reported.

The GITHUB_OUTPUT guard only checked that the variable was non-empty. A set
but unwritable path still passed and then failed on every append, so the step
produced no outputs at all instead of the intended controlled no-op. The guard
now probes the path with a zero-byte append, which verifies writability
without adding content.

The cumulative compressed budget rejected only a non-positive remaining
allowance. A positive remainder below 512 bytes still started a transfer, but
ulimit -f counts 512-byte blocks, so the file limit floored to 0 and every
write failed - the artifact was guaranteed to be discarded after paying for
the request. A new MIN_ZIP_BYTES (1 MB) floor treats such a remainder as
exhausted, which also makes a zero-block ulimit unreachable by construction.

Validated in the roslyn reference implementation by test-fetch-guards.sh
(13/13) and by fork E2E run 33167910190, whose fetch job downloaded the full
artifact set with no cap or budget warnings.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@YuliiaKovalova

Copy link
Copy Markdown
Member Author

Follow-up in b0c2a7e796: two further defects found while reviewing the sibling
backports apply here too, so they're fixed in this PR as well.

  1. The GITHUB_OUTPUT guard only checked the variable was non-empty; a
    set-but-unwritable path passed it and then failed on every append, leaving
    the step with no outputs instead of the intended controlled no-op. It now
    probes the path with a zero-byte append.
  2. The cumulative compressed budget rejected only a non-positive remaining
    allowance, so a tiny positive remainder still started a transfer that could
    never yield a usable archive. A new MIN_ZIP_BYTES (1 MB) floor treats such
    a remainder as exhausted.

Validated in the reference implementation by a new test-fetch-guards.sh
(13/13) and by fork E2E run
33167910190,
which is green end to end and downloaded the full artifact set with no cap or
budget warnings.

Note

This comment was generated with GitHub Copilot.

YuliiaKovalova and others added 3 commits August 28, 2026 14:37
`--max-time` applies per attempt, so `--retry 3` permitted four full
transfers plus backoff and could outlive the job's `timeout-minutes`.
Give the download loop a wall-clock deadline (DOWNLOAD_BUDGET=420s),
cap each attempt at 120s, and derive both `--max-time` and
`--retry-max-time` from the time actually left. Bound the metadata
calls the same way. Worst case is now 720s against a 1800s job timeout.

Also revert the 1 MB MIN_ZIP_BYTES floor: it dropped legs whose archive
was legitimately smaller than the floor. Round the `ulimit -f` block
count up instead, which guarantees at least one block for any positive
cap without rejecting small artifacts.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The extract loop globs the whole directory, so any *.binlog left behind by
an earlier run on the same runner would be uploaded and analyzed as if it
belonged to this build. Remove pre-existing binlogs right after the
directory is created, before anything is written into it.

Also correct a test header that still described the reverted MIN_ZIP_BYTES
clamp instead of the guards actually in the script.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
`--retry-max-time` only decides whether curl may start another retry; a
retry begun just inside it still runs a further `--max-time`, so a
transfer could overshoot DOWNLOAD_DEADLINE by up to MAX_ATTEMPT_SECONDS
and the advertised wall-clock budget was not actually enforced. Wrap the
whole invocation in `timeout "${TIME_LEFT}"`, keeping the curl limits, so
the phase really is bounded by DOWNLOAD_BUDGET. A killed transfer is
treated like any other failed one: the leg is reported missing and the
analysis is disabled, which fails closed.

Also state plainly that the compressed budget is a budget rather than a
byte-exact ceiling: `ulimit -f` rounds up to 512-byte blocks, so a
transfer can overshoot its cap by under 512 bytes before the size check
rejects it, bounded overall by MAX_ARTIFACTS.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@YuliiaKovalova
YuliiaKovalova merged commit eec7f18 into dotnet:main Aug 28, 2026
11 checks passed
@dotnet-milestone-bot dotnet-milestone-bot Bot added this to the 12.0-preview1 milestone Aug 29, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants