Harden the build failure analysis fetch step

[YuliiaKovalova]

Backports the robustness corrections to both copies of the build failure
analysis fetch step. They came out of review of the same workflow in
dotnet/roslyn#85046 and were
validated end to end there against real Azure DevOps builds. Companion PRs:
dotnet/sdk#55985,
microsoft/testfx#10835.

What changes

1. Fail closed on Azure DevOps fetches. build_json and artifacts_json
   used a bare curl; on failure the empty body fell through jq to an
   empty result and the job reported a misleading "no artifacts" warning
   instead of a fetch failure. A new ado_get helper checks the curl exit
   status and validates the body with jq -e .. It returns a status rather
   than calling emit_none directly, because emit_none inside a command
   substitution would only exit the subshell.
2. Guard GITHUB_OUTPUT once up front, so the success path's writes are
   covered too, not just emit_none.
3. Timeouts on the metadata fetches
   (--connect-timeout 10 --max-time 20 --retry-max-time 40), so a stalled
   endpoint fails in seconds rather than eating the job timeout. --max-time
   is per attempt, so --retry-max-time is what actually bounds a retrying
   call; there are three of these before any download starts, so their
   combined retry windows must not consume the job on their own.
4. Per-artifact compressed cap 500 MB -> 2 GB. This is the high-value fix:
   real log artifacts land close enough to 500 MB that an ordinary build trips
   the cap and the job silently skips exactly the leg it exists to diagnose.
   Only one archive is on disk at a time, so this bounds peak disk use, not the
   sum across artifacts.
5. New MAX_TOTAL_ZIP_BYTES (3 GB), charged before each transfer.
   Arcade had no cumulative compressed budget at all, so raising the
   per-artifact cap would have raised worst-case bytes pulled over the network
   by the same factor. Each transfer is now clamped to
   ZIP_CAP = min(MAX_ZIP_BYTES, MAX_TOTAL_ZIP_BYTES - TOTAL_ZIP_BYTES).
   Charging before rather than after the transfer stops the last artifact from
   starting just under the limit and still pulling a full cap's worth. The
   post-download size guard and the ulimit -f backstop both use ZIP_CAP,
   otherwise the clamp is defeated.
6. The download phase is bounded by a hard wall-clock deadline. A stalled
   transfer must not kill the job before the script can emit its controlled
   no-op. Because --max-time is per attempt, --retry 3 alone permitted four
   full transfers plus backoff — around 20 minutes against timeout-minutes: 15. The loop now has a deadline (DOWNLOAD_BUDGET=420,
   MAX_ATTEMPT_SECONDS=120) and each transfer derives both --max-time and
   --retry-max-time from the time left. Since --retry-max-time only gates
   whether a new retry may start, the whole invocation is also wrapped in
   timeout "${TIME_LEFT}" — that is what makes the deadline real. 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.
7. The budget guards actually stop the loop, and ulimit -f's 512-byte
   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. The
   compressed budget is a budget rather than a byte-exact ceiling: a transfer
   can overshoot its cap by under 512 bytes before the size check rejects it.
8. The binlog directory is cleared before anything is extracted into it.
   The extract and upload steps glob the whole directory, so a binlog left by
   an earlier run on the same runner would otherwise be attributed to this
   build.

On the lock files

The generated .lock.yml files are edited in lockstep with the sources
rather than recompiled, so they stay on the gh aw version they were built
with (v0.77.5) and this diff carries no unrelated toolchain churn — no
uses: bumps and no actions-lock.json changes. The run blocks embedded
in the locks were verified byte-identical to the ones in the sources.

Validation

• shellcheck and bash -n clean on both generated fetch scripts.
• Source/lock equivalence verified programmatically.
• 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 earned its keep by catching a
  real regression during this work — a bad scripted substitution had replaced a
  break with a literal None, which both bash -n and shellcheck accept
  without complaint.
• Exercised end to end in the roslyn 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.

[Copilot]

Pull request overview

This PR backports robustness improvements to the GitHub Actions “build failure analysis” fetch step in Arcade, aiming to make Azure DevOps metadata/artifact retrieval fail safely (and predictably) while bounding download time and resource usage.

Changes:

• Introduces an ado_get helper to validate Azure DevOps fetches (curl status + JSON validation) and avoids misleading “no artifacts” outcomes on fetch failures.
• Adds upfront GITHUB_OUTPUT guarding and adds explicit curl connect/overall timeouts for metadata and artifact downloads.
• Increases per-artifact compressed cap to 2GB and adds a cumulative compressed download budget (3GB) via ZIP_CAP clamping.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.

File	Description
.github/workflows/build-failure-analysis.md	Hardens ADO metadata/artifact fetching and enforces time/budget limits for binlog extraction.
.github/workflows/build-failure-analysis.lock.yml	Lockfile kept in sync with the hardened fetch script logic.
.github/workflows/build-failure-analysis-command.md	Applies the same hardened fetch logic to the slash-command driven workflow variant.
.github/workflows/build-failure-analysis-command.lock.yml	Lockfile kept in sync with the command workflow hardened fetch script logic.

---

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

[YuliiaKovalova]

Good catch, and this was a real defect - fixed in 11144a5a.

The budget check only rejected a non-positive ZIP_CAP. A remainder of, say,
300 bytes was "positive", so the loop went ahead and ran
ulimit -f $((ZIP_CAP / 512)), which floors to 0 blocks - every write then
fails and the artifact is discarded anyway, after paying for the request. Even
above 512 bytes the transfer could never produce a usable archive.

Rather than special-casing the ulimit arithmetic, the floor is now expressed in
terms of what a transfer needs to be worth starting, which makes a zero-block
ulimit unreachable by construction:

MAX_TOTAL_ZIP_BYTES=3221225472  # 3 GB compressed across all artifacts
# A transfer smaller than this can't yield a usable archive, and
# `ulimit -f` works in 512-byte blocks, so a cap under 512 bytes
# would floor to a 0-block file limit and fail every write. Treat a
# remaining allowance below this as exhausted rather than starting a
# transfer that is guaranteed to be discarded.
MIN_ZIP_BYTES=1048576           # 1 MB

if [ "${ZIP_CAP}" -lt "${MIN_ZIP_BYTES}" ]; then
  echo "::warning::Cumulative compressed download budget ${MAX_TOTAL_ZIP_BYTES} is exhausted before ${safe_name}; stopping downloads."
  break
fi

Since you flagged this on four locations, I treated it as a class rather than a
list: it's fixed in both arcade workflows and both generated locks, and also
backported to the same workflow in
dotnet/roslyn#85046,
dotnet/runtime#132609,
dotnet/sdk#55985 and
microsoft/testfx#10835.

A new test-fetch-guards.sh in the roslyn reference covers the clamp across a
fresh, partial, sub-block, exactly-exhausted and over-spent budget, and asserts
that no cap the loop accepts can yield a zero-block ulimit (13/13 pass).

Note

This reply was generated with GitHub Copilot.

[YuliiaKovalova]

Good catch, and this was a real defect - fixed in 11144a5a.

The budget check only rejected a non-positive ZIP_CAP. A remainder of, say,
300 bytes was "positive", so the loop went ahead and ran
ulimit -f $((ZIP_CAP / 512)), which floors to 0 blocks - every write then
fails and the artifact is discarded anyway, after paying for the request. Even
above 512 bytes the transfer could never produce a usable archive.

Rather than special-casing the ulimit arithmetic, the floor is now expressed in
terms of what a transfer needs to be worth starting, which makes a zero-block
ulimit unreachable by construction:

MAX_TOTAL_ZIP_BYTES=3221225472  # 3 GB compressed across all artifacts
# A transfer smaller than this can't yield a usable archive, and
# `ulimit -f` works in 512-byte blocks, so a cap under 512 bytes
# would floor to a 0-block file limit and fail every write. Treat a
# remaining allowance below this as exhausted rather than starting a
# transfer that is guaranteed to be discarded.
MIN_ZIP_BYTES=1048576           # 1 MB

if [ "${ZIP_CAP}" -lt "${MIN_ZIP_BYTES}" ]; then
  echo "::warning::Cumulative compressed download budget ${MAX_TOTAL_ZIP_BYTES} is exhausted before ${safe_name}; stopping downloads."
  break
fi

Since you flagged this on four locations, I treated it as a class rather than a
list: it's fixed in both arcade workflows and both generated locks, and also
backported to the same workflow in
dotnet/roslyn#85046,
dotnet/runtime#132609,
dotnet/sdk#55985 and
microsoft/testfx#10835.

A new test-fetch-guards.sh in the roslyn reference covers the clamp across a
fresh, partial, sub-block, exactly-exhausted and over-spent budget, and asserts
that no cap the loop accepts can yield a zero-block ulimit (13/13 pass).

Note

This reply was generated with GitHub Copilot.

[YuliiaKovalova]

Good catch, and this was a real defect - fixed in 11144a5a.

The budget check only rejected a non-positive ZIP_CAP. A remainder of, say,
300 bytes was "positive", so the loop went ahead and ran
ulimit -f $((ZIP_CAP / 512)), which floors to 0 blocks - every write then
fails and the artifact is discarded anyway, after paying for the request. Even
above 512 bytes the transfer could never produce a usable archive.

Rather than special-casing the ulimit arithmetic, the floor is now expressed in
terms of what a transfer needs to be worth starting, which makes a zero-block
ulimit unreachable by construction:

MAX_TOTAL_ZIP_BYTES=3221225472  # 3 GB compressed across all artifacts
# A transfer smaller than this can't yield a usable archive, and
# `ulimit -f` works in 512-byte blocks, so a cap under 512 bytes
# would floor to a 0-block file limit and fail every write. Treat a
# remaining allowance below this as exhausted rather than starting a
# transfer that is guaranteed to be discarded.
MIN_ZIP_BYTES=1048576           # 1 MB

if [ "${ZIP_CAP}" -lt "${MIN_ZIP_BYTES}" ]; then
  echo "::warning::Cumulative compressed download budget ${MAX_TOTAL_ZIP_BYTES} is exhausted before ${safe_name}; stopping downloads."
  break
fi

Since you flagged this on four locations, I treated it as a class rather than a
list: it's fixed in both arcade workflows and both generated locks, and also
backported to the same workflow in
dotnet/roslyn#85046,
dotnet/runtime#132609,
dotnet/sdk#55985 and
microsoft/testfx#10835.

A new test-fetch-guards.sh in the roslyn reference covers the clamp across a
fresh, partial, sub-block, exactly-exhausted and over-spent budget, and asserts
that no cap the loop accepts can yield a zero-block ulimit (13/13 pass).

Note

This reply was generated with GitHub Copilot.

[YuliiaKovalova]

Good catch, and this was a real defect - fixed in 11144a5a.

The budget check only rejected a non-positive ZIP_CAP. A remainder of, say,
300 bytes was "positive", so the loop went ahead and ran
ulimit -f $((ZIP_CAP / 512)), which floors to 0 blocks - every write then
fails and the artifact is discarded anyway, after paying for the request. Even
above 512 bytes the transfer could never produce a usable archive.

Rather than special-casing the ulimit arithmetic, the floor is now expressed in
terms of what a transfer needs to be worth starting, which makes a zero-block
ulimit unreachable by construction:

MAX_TOTAL_ZIP_BYTES=3221225472  # 3 GB compressed across all artifacts
# A transfer smaller than this can't yield a usable archive, and
# `ulimit -f` works in 512-byte blocks, so a cap under 512 bytes
# would floor to a 0-block file limit and fail every write. Treat a
# remaining allowance below this as exhausted rather than starting a
# transfer that is guaranteed to be discarded.
MIN_ZIP_BYTES=1048576           # 1 MB

if [ "${ZIP_CAP}" -lt "${MIN_ZIP_BYTES}" ]; then
  echo "::warning::Cumulative compressed download budget ${MAX_TOTAL_ZIP_BYTES} is exhausted before ${safe_name}; stopping downloads."
  break
fi

Since you flagged this on four locations, I treated it as a class rather than a
list: it's fixed in both arcade workflows and both generated locks, and also
backported to the same workflow in
dotnet/roslyn#85046,
dotnet/runtime#132609,
dotnet/sdk#55985 and
microsoft/testfx#10835.

A new test-fetch-guards.sh in the roslyn reference covers the clamp across a
fresh, partial, sub-block, exactly-exhausted and over-spent budget, and asserts
that no cap the loop accepts can yield a zero-block ulimit (13/13 pass).

Note

This reply was generated with GitHub Copilot.

[Copilot]

Pull request overview

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

Suppressed comments (4)

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

.github/workflows/build-failure-analysis.md:401

• The comment about the ulimit -f calculation is incorrect: ZIP_CAP / 512 floors, so the ulimit can be up to 511 bytes below ZIP_CAP (not >=). This can cause a capped download to be classified via curl_rc as truncation rather than being caught by the ZIP_BYTES -ge ZIP_CAP guard, and it misdocuments the intent of the guard/ulimit pairing.

            # `ulimit -f` is only a disk backstop for a response that declares
            # no Content-Length; the `-ge ZIP_CAP` guard below is
            # authoritative. Divide by 512 so the cap is >= ZIP_CAP under
            # either block-size reading (bash uses 1024, POSIX says 512).

.github/workflows/build-failure-analysis-command.md:459

• The comment about the ulimit -f calculation is incorrect: ZIP_CAP / 512 floors, so the ulimit can be up to 511 bytes below ZIP_CAP (not >=). This can cause a capped download to be classified via curl_rc as truncation rather than being caught by the ZIP_BYTES -ge ZIP_CAP guard, and it misdocuments the intent of the guard/ulimit pairing.

            # `ulimit -f` is only a disk backstop for a response that declares
            # no Content-Length; the `-ge ZIP_CAP` guard below is
            # authoritative. Divide by 512 so the cap is >= ZIP_CAP under
            # either block-size reading (bash uses 1024, POSIX says 512).

.github/workflows/build-failure-analysis.lock.yml:1678

• The comment about the ulimit -f calculation is incorrect: ZIP_CAP / 512 floors, so the ulimit can be up to 511 bytes below ZIP_CAP (not >=). This can cause a capped download to be classified via curl_rc as truncation rather than being caught by the ZIP_BYTES -ge ZIP_CAP guard, and it misdocuments the intent of the guard/ulimit pairing.

            # `ulimit -f` is only a disk backstop for a response that declares
            # no Content-Length; the `-ge ZIP_CAP` guard below is
            # authoritative. Divide by 512 so the cap is >= ZIP_CAP under
            # either block-size reading (bash uses 1024, POSIX says 512).

.github/workflows/build-failure-analysis-command.lock.yml:1771

• The comment about the ulimit -f calculation is incorrect: ZIP_CAP / 512 floors, so the ulimit can be up to 511 bytes below ZIP_CAP (not >=). This can cause a capped download to be classified via curl_rc as truncation rather than being caught by the ZIP_BYTES -ge ZIP_CAP guard, and it misdocuments the intent of the guard/ulimit pairing.

            # `ulimit -f` is only a disk backstop for a response that declares
            # no Content-Length; the `-ge ZIP_CAP` guard below is
            # authoritative. Divide by 512 so the cap is >= ZIP_CAP under
            # either block-size reading (bash uses 1024, POSIX says 512).

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.

Suppressed comments (4)

.github/workflows/build-failure-analysis.md:399

• If the download wall-clock budget is exhausted, this branch only prints a warning (and appends an unexpected literal None) but then continues, potentially passing TIME_LEFT <= 0 into curl’s --max-time/--retry-max-time.

Break out of the artifact loop when no time remains and remove the stray None suffix.

            if [ "${TIME_LEFT}" -le 0 ]; then
              echo "::warning::Download time budget ${DOWNLOAD_BUDGET}s exhausted before ${safe_name}; stopping downloads."None
            fi

.github/workflows/build-failure-analysis.lock.yml:1676

• If the download wall-clock budget is exhausted, this branch only prints a warning (and appends an unexpected literal None) but then continues, potentially passing TIME_LEFT <= 0 into curl’s --max-time/--retry-max-time.

Break out of the artifact loop when no time remains and remove the stray None suffix.

            if [ "${TIME_LEFT}" -le 0 ]; then
              echo "::warning::Download time budget ${DOWNLOAD_BUDGET}s exhausted before ${safe_name}; stopping downloads."None
            fi

.github/workflows/build-failure-analysis-command.md:457

• If the download wall-clock budget is exhausted, this branch only prints a warning (and appends an unexpected literal None) but then continues, potentially passing TIME_LEFT <= 0 into curl’s --max-time/--retry-max-time.

Break out of the artifact loop when no time remains and remove the stray None suffix.

            if [ "${TIME_LEFT}" -le 0 ]; then
              echo "::warning::Download time budget ${DOWNLOAD_BUDGET}s exhausted before ${safe_name}; stopping downloads."None
            fi

.github/workflows/build-failure-analysis-command.lock.yml:1769

• If the download wall-clock budget is exhausted, this branch only prints a warning (and appends an unexpected literal None) but then continues, potentially passing TIME_LEFT <= 0 into curl’s --max-time/--retry-max-time.

Break out of the artifact loop when no time remains and remove the stray None suffix.

            if [ "${TIME_LEFT}" -le 0 ]; then
              echo "::warning::Download time budget ${DOWNLOAD_BUDGET}s exhausted before ${safe_name}; stopping downloads."None
            fi

[YuliiaKovalova]

Fixed — you're right on both counts, and the missing break was the serious one.

The None was not cosmetic: a scripted substitution I used to apply the retry-window change across the sibling repos had a bug, and a regex group absent in this repo's variant came back as None, which Python stringified into the output in place of the break. So an exhausted compressed or time budget printed its warning and then carried straight on to the next artifact — neither cap was enforced at all, exactly as you describe.

Both guards now read:

if [ "${ZIP_CAP}" -le 0 ]; then
  echo "::warning::Cumulative compressed download budget ${MAX_TOTAL_ZIP_BYTES} is exhausted before ${safe_name}; stopping downloads."
  break
fi

Worth noting why this got through: neither bash -n nor shellcheck objects, because None is simply an extra argument to echo, so the script stays valid while silently losing its loop exit — and it survived a read-through of the diff. I added a check that parses the generated .lock.yml and fails if any stopping downloads. warning is not followed by a break within its guard block. It reproduces the bug on the old files, passes on the new ones, and now runs over the generated workflows in every repo carrying this code.

Note

This reply was drafted with AI assistance and reviewed by me.

[YuliiaKovalova]

Fixed — you're right on both counts, and the missing break was the serious one.

The None was not cosmetic: a scripted substitution I used to apply the retry-window change across the sibling repos had a bug, and a regex group absent in this repo's variant came back as None, which Python stringified into the output in place of the break. So an exhausted compressed or time budget printed its warning and then carried straight on to the next artifact — neither cap was enforced at all, exactly as you describe.

Both guards now read:

if [ "${ZIP_CAP}" -le 0 ]; then
  echo "::warning::Cumulative compressed download budget ${MAX_TOTAL_ZIP_BYTES} is exhausted before ${safe_name}; stopping downloads."
  break
fi

Worth noting why this got through: neither bash -n nor shellcheck objects, because None is simply an extra argument to echo, so the script stays valid while silently losing its loop exit — and it survived a read-through of the diff. I added a check that parses the generated .lock.yml and fails if any stopping downloads. warning is not followed by a break within its guard block. It reproduces the bug on the old files, passes on the new ones, and now runs over the generated workflows in every repo carrying this code.

Note

This reply was drafted with AI assistance and reviewed by me.

[YuliiaKovalova]

Fixed — you're right on both counts, and the missing break was the serious one.

The None was not cosmetic: a scripted substitution I used to apply the retry-window change across the sibling repos had a bug, and a regex group absent in this repo's variant came back as None, which Python stringified into the output in place of the break. So an exhausted compressed or time budget printed its warning and then carried straight on to the next artifact — neither cap was enforced at all, exactly as you describe.

Both guards now read:

if [ "${ZIP_CAP}" -le 0 ]; then
  echo "::warning::Cumulative compressed download budget ${MAX_TOTAL_ZIP_BYTES} is exhausted before ${safe_name}; stopping downloads."
  break
fi

Worth noting why this got through: neither bash -n nor shellcheck objects, because None is simply an extra argument to echo, so the script stays valid while silently losing its loop exit — and it survived a read-through of the diff. I added a check that parses the generated .lock.yml and fails if any stopping downloads. warning is not followed by a break within its guard block. It reproduces the bug on the old files, passes on the new ones, and now runs over the generated workflows in every repo carrying this code.

Note

This reply was drafted with AI assistance and reviewed by me.

[YuliiaKovalova]

Fixed — this one slipped through the first pass.

The ado_get helper was added in this PR and build_json and artifacts_json were converted to use it, but builds_json on the latest resolution path was missed. It kept the failure mode the helper exists to remove: a network error or an HTML error page yields an empty body, jq turns that into an empty .value, and the step reports "No arcade-pr build found for PR #N" — a message that says there is nothing to analyze when what actually happened is we could not find out. A transient ADO outage looks like a healthy PR with no builds.

It now goes through the helper like the others:

ado_get "build list for PR #${PR_NUMBER}" \
  "${ADO_API}/build/builds?definitions=${ADO_BUILD_DEFINITION_ID}&branchName=refs/pull/${PR_NUMBER}/merge&queryOrder=queueTimeDescending&\$top=1&api-version=7.1" || emit_none
builds_json="${ADO_DOC}"

which checks the curl exit status and validates the body with jq -e . before anything downstream reads it.

I also swept for the rest of the class rather than fixing just this one: a check now walks every curl in the generated workflows across all the repos carrying this code and flags any that is not routed through ado_get or explicitly handled. That found one more — the timeline fetch in the automatic workflow — which is fixed in the same commit. The two remaining raw fetches are the completeness-check timelines that deliberately use || true, because there an unreadable timeline must not look like a complete build; they already fail closed.

Note

This reply was drafted with AI assistance and reviewed by me.

[YuliiaKovalova]

Fixed — you're right on both counts, and the missing break was the serious one.

The None was not cosmetic: a scripted substitution I used to apply the retry-window change across the sibling repos had a bug, and a regex group absent in this repo's variant came back as None, which Python stringified into the output in place of the break. So an exhausted compressed or time budget printed its warning and then carried straight on to the next artifact — neither cap was enforced at all, exactly as you describe.

Both guards now read:

if [ "${ZIP_CAP}" -le 0 ]; then
  echo "::warning::Cumulative compressed download budget ${MAX_TOTAL_ZIP_BYTES} is exhausted before ${safe_name}; stopping downloads."
  break
fi

Worth noting why this got through: neither bash -n nor shellcheck objects, because None is simply an extra argument to echo, so the script stays valid while silently losing its loop exit — and it survived a read-through of the diff. I added a check that parses the generated .lock.yml and fails if any stopping downloads. warning is not followed by a break within its guard block. It reproduces the bug on the old files, passes on the new ones, and now runs over the generated workflows in every repo carrying this code.

Note

This reply was drafted with AI assistance and reviewed by me.

[YuliiaKovalova]

Fixed — this one slipped through the first pass.

The ado_get helper was added in this PR and build_json and artifacts_json were converted to use it, but builds_json on the latest resolution path was missed. It kept the failure mode the helper exists to remove: a network error or an HTML error page yields an empty body, jq turns that into an empty .value, and the step reports "No arcade-pr build found for PR #N" — a message that says there is nothing to analyze when what actually happened is we could not find out. A transient ADO outage looks like a healthy PR with no builds.

It now goes through the helper like the others:

ado_get "build list for PR #${PR_NUMBER}" \
  "${ADO_API}/build/builds?definitions=${ADO_BUILD_DEFINITION_ID}&branchName=refs/pull/${PR_NUMBER}/merge&queryOrder=queueTimeDescending&\$top=1&api-version=7.1" || emit_none
builds_json="${ADO_DOC}"

which checks the curl exit status and validates the body with jq -e . before anything downstream reads it.

I also swept for the rest of the class rather than fixing just this one: a check now walks every curl in the generated workflows across all the repos carrying this code and flags any that is not routed through ado_get or explicitly handled. That found one more — the timeline fetch in the automatic workflow — which is fixed in the same commit. The two remaining raw fetches are the completeness-check timelines that deliberately use || true, because there an unreadable timeline must not look like a complete build; they already fail closed.

Note

This reply was drafted with AI assistance and reviewed by me.

[Copilot]

Pull request overview

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

Suppressed comments (8)

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

.github/workflows/build-failure-analysis.md:403

• When the download deadline is reached, the script currently only prints a warning (and includes a stray None) but continues, which can make TIME_LEFT negative and pass an invalid --max-time/--retry-max-time to curl. This should stop downloading further artifacts.

            if [ "${TIME_LEFT}" -le 0 ]; then
              echo "::warning::Download time budget ${DOWNLOAD_BUDGET}s exhausted before ${safe_name}; stopping downloads."None
            fi

.github/workflows/build-failure-analysis.lock.yml:1680

• The time budget exhaustion guard only prints a warning (with a stray None) and then continues, which can make TIME_LEFT negative and lead to invalid curl timeout arguments. This should break out of the artifact loop.

            if [ "${TIME_LEFT}" -le 0 ]; then
              echo "::warning::Download time budget ${DOWNLOAD_BUDGET}s exhausted before ${safe_name}; stopping downloads."None
            fi

.github/workflows/build-failure-analysis-command.md:461

• When TIME_LEFT is exhausted, the script currently warns (and appends a stray None) but continues, which can make ATTEMPT_SECONDS negative and pass invalid timeout values to curl. This should break out of the artifact loop.

            if [ "${TIME_LEFT}" -le 0 ]; then
              echo "::warning::Download time budget ${DOWNLOAD_BUDGET}s exhausted before ${safe_name}; stopping downloads."None
            fi

.github/workflows/build-failure-analysis-command.lock.yml:1773

• The time budget exhaustion check currently only prints a warning (and includes a stray None) but continues, which can result in negative TIME_LEFT and invalid curl timeout arguments. This should stop downloading further artifacts.

            if [ "${TIME_LEFT}" -le 0 ]; then
              echo "::warning::Download time budget ${DOWNLOAD_BUDGET}s exhausted before ${safe_name}; stopping downloads."None
            fi

.github/workflows/build-failure-analysis.md:397

• The budget/time exhaustion guards don't actually stop the loop: they just print a warning (with an extra None token) and then continue, which can lead to invalid ZIP_CAP/negative timeouts being used for ulimit/curl. These branches should break out of the artifact loop once the budget is exhausted, and the stray None should be removed.

            if [ "${ZIP_CAP}" -le 0 ]; then
              echo "::warning::Cumulative compressed download budget ${MAX_TOTAL_ZIP_BYTES} is exhausted before ${safe_name}; stopping downloads."None
            fi

.github/workflows/build-failure-analysis.lock.yml:1674

• The budget exhaustion guard only prints a warning (with an extra None token) and then continues the loop. That can leave ZIP_CAP non-positive and still attempt ulimit/curl. This should break out of the artifact loop and remove the stray None.

            if [ "${ZIP_CAP}" -le 0 ]; then
              echo "::warning::Cumulative compressed download budget ${MAX_TOTAL_ZIP_BYTES} is exhausted before ${safe_name}; stopping downloads."None
            fi

.github/workflows/build-failure-analysis-command.md:455

• The budget exhaustion check prints a warning (and includes a stray None) but does not stop the loop. If ZIP_CAP is non-positive, subsequent ulimit/curl calls can behave unpredictably. This should break out of the artifact loop once the budget is exhausted.

            if [ "${ZIP_CAP}" -le 0 ]; then
              echo "::warning::Cumulative compressed download budget ${MAX_TOTAL_ZIP_BYTES} is exhausted before ${safe_name}; stopping downloads."None
            fi

.github/workflows/build-failure-analysis-command.lock.yml:1767

• The budget exhaustion guard only prints a warning (with an extra None) and then continues. If ZIP_CAP is non-positive, the loop should stop rather than running ulimit/curl with invalid limits.

            if [ "${ZIP_CAP}" -le 0 ]; then
              echo "::warning::Cumulative compressed download budget ${MAX_TOTAL_ZIP_BYTES} is exhausted before ${safe_name}; stopping downloads."None
            fi

[Copilot]

Pull request overview

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

[Copilot]

Pull request overview

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

Suppressed comments (4)

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

.github/workflows/build-failure-analysis.md:398

• ZIP_CAP is meant to clamp downloads to the remaining compressed budget, but ulimit -f $(( (ZIP_CAP + 511) / 512 )) rounds up to 512-byte blocks, so the file-size backstop can exceed ZIP_CAP by up to 511 bytes. When ZIP_CAP comes from a non-512-byte-aligned remaining allowance, a transfer can exceed MAX_TOTAL_ZIP_BYTES even though the code thinks it is clamped. Consider rounding ZIP_CAP down to a 512-byte multiple (or otherwise aligning the cap/ulimit/guards) before starting the transfer.

            ZIP_CAP="${MAX_ZIP_BYTES}"
            ZIP_ALLOWANCE=$((MAX_TOTAL_ZIP_BYTES - TOTAL_ZIP_BYTES))
            [ "${ZIP_ALLOWANCE}" -lt "${ZIP_CAP}" ] && ZIP_CAP="${ZIP_ALLOWANCE}"
            if [ "${ZIP_CAP}" -le 0 ]; then
              echo "::warning::Cumulative compressed download budget ${MAX_TOTAL_ZIP_BYTES} is exhausted before ${safe_name}; stopping downloads."

.github/workflows/build-failure-analysis-command.md:457

• ZIP_CAP is intended to clamp downloads to the remaining compressed budget, but the later ulimit uses 512-byte blocks and is rounded up, which can allow the download to exceed ZIP_CAP (and thus MAX_TOTAL_ZIP_BYTES) by up to 511 bytes when the remaining allowance isn’t block-aligned. Consider rounding ZIP_CAP down to a 512-byte multiple (or aligning the ulimit/guards to the same effective cap) before starting the transfer.

            ZIP_CAP="${MAX_ZIP_BYTES}"
            ZIP_ALLOWANCE=$((MAX_TOTAL_ZIP_BYTES - TOTAL_ZIP_BYTES))
            [ "${ZIP_ALLOWANCE}" -lt "${ZIP_CAP}" ] && ZIP_CAP="${ZIP_ALLOWANCE}"
            if [ "${ZIP_CAP}" -le 0 ]; then
              echo "::warning::Cumulative compressed download budget ${MAX_TOTAL_ZIP_BYTES} is exhausted before ${safe_name}; stopping downloads."

.github/workflows/build-failure-analysis.lock.yml:1675

• ZIP_CAP is clamped to the remaining compressed budget, but the later ulimit is rounded up in 512-byte blocks, which means the download backstop can still allow up to 511 bytes more than ZIP_CAP. If ZIP_CAP comes from a non-block-aligned remaining allowance, this can exceed MAX_TOTAL_ZIP_BYTES despite the clamp. Consider rounding ZIP_CAP down to a 512-byte multiple before starting the transfer.

            ZIP_CAP="${MAX_ZIP_BYTES}"
            ZIP_ALLOWANCE=$((MAX_TOTAL_ZIP_BYTES - TOTAL_ZIP_BYTES))
            [ "${ZIP_ALLOWANCE}" -lt "${ZIP_CAP}" ] && ZIP_CAP="${ZIP_ALLOWANCE}"
            if [ "${ZIP_CAP}" -le 0 ]; then
              echo "::warning::Cumulative compressed download budget ${MAX_TOTAL_ZIP_BYTES} is exhausted before ${safe_name}; stopping downloads."

.github/workflows/build-failure-analysis-command.lock.yml:1769

• ZIP_CAP is clamped to the remaining compressed budget, but the later ulimit is rounded up in 512-byte blocks, which can allow the download to exceed ZIP_CAP (and therefore MAX_TOTAL_ZIP_BYTES) by up to 511 bytes when the remaining allowance isn’t 512-byte aligned. Consider rounding ZIP_CAP down to a 512-byte multiple before starting the transfer.

            ZIP_CAP="${MAX_ZIP_BYTES}"
            ZIP_ALLOWANCE=$((MAX_TOTAL_ZIP_BYTES - TOTAL_ZIP_BYTES))
            [ "${ZIP_ALLOWANCE}" -lt "${ZIP_CAP}" ] && ZIP_CAP="${ZIP_ALLOWANCE}"
            if [ "${ZIP_CAP}" -le 0 ]; then
              echo "::warning::Cumulative compressed download budget ${MAX_TOTAL_ZIP_BYTES} is exhausted before ${safe_name}; stopping downloads."

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.

[YuliiaKovalova]

Fair point, and it applies to more than ado_get — the artifact archive was going to a fixed /tmp/a.zip for the same reason. Both are now mktemp:

tmp=$(mktemp) || {
  echo "::warning::Could not create a temporary file for the ${what}; treating as a data-resolution failure."
  return 1
}

The download allocates one scratch file before the loop and truncates it per artifact (: > "${ZIP_TMP}") instead of removing and recreating it — removing it would hand the name back and undo the point of mktemp. The rm -f "${tmp}" that used to precede the fetch is gone too: mktemp already yields an empty private file and curl truncates before each attempt, so the old line only reintroduced a window at a known path.

These jobs run on ephemeral ubuntu-latest, so I don't think this was reachable today. But the cost of not depending on that is a one-line change, and these workflows get copied into repos that do use self-hosted runners — this one is a backport of exactly that kind.

One deliberate exception: BINLOG_DIR keeps its fixed path. It is an interface rather than scratch space — the workflow sets it, the agent container maps it, and the upload step reads it — and the script creates it with mkdir -p and clears stale binlogs from it rather than writing through a name it does not control. A test now asserts that no curl writes to a fixed /tmp path and that the scratch files come from mktemp, with that exception spelled out.

Validated end to end on a real failed Azure Pipelines build: run 33181881513, all seven jobs green with no warnings.

Note

Parts of this reply were generated with AI assistance.

[YuliiaKovalova]

Fair point, and it applies to more than ado_get — the artifact archive was going to a fixed /tmp/a.zip for the same reason. Both are now mktemp:

tmp=$(mktemp) || {
  echo "::warning::Could not create a temporary file for the ${what}; treating as a data-resolution failure."
  return 1
}

The download allocates one scratch file before the loop and truncates it per artifact (: > "${ZIP_TMP}") instead of removing and recreating it — removing it would hand the name back and undo the point of mktemp. The rm -f "${tmp}" that used to precede the fetch is gone too: mktemp already yields an empty private file and curl truncates before each attempt, so the old line only reintroduced a window at a known path.

These jobs run on ephemeral ubuntu-latest, so I don't think this was reachable today. But the cost of not depending on that is a one-line change, and these workflows get copied into repos that do use self-hosted runners — this one is a backport of exactly that kind.

One deliberate exception: BINLOG_DIR keeps its fixed path. It is an interface rather than scratch space — the workflow sets it, the agent container maps it, and the upload step reads it — and the script creates it with mkdir -p and clears stale binlogs from it rather than writing through a name it does not control. A test now asserts that no curl writes to a fixed /tmp path and that the scratch files come from mktemp, with that exception spelled out.

Validated end to end on a real failed Azure Pipelines build: run 33181881513, all seven jobs green with no warnings.

Note

Parts of this reply were generated with AI assistance.

[YuliiaKovalova]

Fair point, and it applies to more than ado_get — the artifact archive was going to a fixed /tmp/a.zip for the same reason. Both are now mktemp:

tmp=$(mktemp) || {
  echo "::warning::Could not create a temporary file for the ${what}; treating as a data-resolution failure."
  return 1
}

The download allocates one scratch file before the loop and truncates it per artifact (: > "${ZIP_TMP}") instead of removing and recreating it — removing it would hand the name back and undo the point of mktemp. The rm -f "${tmp}" that used to precede the fetch is gone too: mktemp already yields an empty private file and curl truncates before each attempt, so the old line only reintroduced a window at a known path.

These jobs run on ephemeral ubuntu-latest, so I don't think this was reachable today. But the cost of not depending on that is a one-line change, and these workflows get copied into repos that do use self-hosted runners — this one is a backport of exactly that kind.

One deliberate exception: BINLOG_DIR keeps its fixed path. It is an interface rather than scratch space — the workflow sets it, the agent container maps it, and the upload step reads it — and the script creates it with mkdir -p and clears stale binlogs from it rather than writing through a name it does not control. A test now asserts that no curl writes to a fixed /tmp path and that the scratch files come from mktemp, with that exception spelled out.

Validated end to end on a real failed Azure Pipelines build: run 33181881513, all seven jobs green with no warnings.

Note

Parts of this reply were generated with AI assistance.

[YuliiaKovalova]

Fair point, and it applies to more than ado_get — the artifact archive was going to a fixed /tmp/a.zip for the same reason. Both are now mktemp:

tmp=$(mktemp) || {
  echo "::warning::Could not create a temporary file for the ${what}; treating as a data-resolution failure."
  return 1
}

The download allocates one scratch file before the loop and truncates it per artifact (: > "${ZIP_TMP}") instead of removing and recreating it — removing it would hand the name back and undo the point of mktemp. The rm -f "${tmp}" that used to precede the fetch is gone too: mktemp already yields an empty private file and curl truncates before each attempt, so the old line only reintroduced a window at a known path.

These jobs run on ephemeral ubuntu-latest, so I don't think this was reachable today. But the cost of not depending on that is a one-line change, and these workflows get copied into repos that do use self-hosted runners — this one is a backport of exactly that kind.

One deliberate exception: BINLOG_DIR keeps its fixed path. It is an interface rather than scratch space — the workflow sets it, the agent container maps it, and the upload step reads it — and the script creates it with mkdir -p and clears stale binlogs from it rather than writing through a name it does not control. A test now asserts that no curl writes to a fixed /tmp path and that the scratch files come from mktemp, with that exception spelled out.

Validated end to end on a real failed Azure Pipelines build: run 33181881513, all seven jobs green with no warnings.

Note

Parts of this reply were generated with AI assistance.

[Copilot]

Pull request overview

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

[YuliiaKovalova]

You are right about /tmp/ax, and I have taken that half. It is pure scratch, so it is now mktemp -d, emptied per artifact and released after the loop:

AX_DIR=$(mktemp -d) || { echo "::warning::Could not create a temporary directory for extraction."; emit_none; }
...
find "${AX_DIR:?}" -mindepth 1 -delete   # per artifact
...
rm -rf "${AX_DIR:?}" "${ZIP_TMP}"        # after the loop

BINLOG_DIR I have deliberately left alone, and I want to be straight that this is a real limitation rather than a rebuttal. It is not scratch space — it is an interface. The workflow sets it, the agent container maps it to /data/binlogs, and the upload step reads it; that mount is part of the generated lock file, so a mktemp -d path would have to be threaded through the container spec that gh aw emits. That is a change to generated infrastructure, not to this script, and I would rather not make it as a drive-by in a backport.

What that leaves is the residual you identified: two of these jobs on one shared runner could collide in /tmp/binlogs. The rm -f "${BINLOG_DIR}"/*.binlog is there for the sequential case, which is the one that actually bites — a stale binlog from an earlier run being uploaded and attributed to this build, which produces a confidently wrong root cause on someone's PR. For the concurrent case it is not sufficient, and I have not claimed otherwise.

These workflows target ubuntu-latest, where each job gets its own VM, so this is not reachable as configured. If a repo moves them to shared runners, BINLOG_DIR is the thing to revisit, and it is a one-line change at the call site now that nothing else in the script hardcodes a path.

Note

Parts of this reply were generated with AI assistance.

[YuliiaKovalova]

You are right about /tmp/ax, and I have taken that half. It is pure scratch, so it is now mktemp -d, emptied per artifact and released after the loop:

AX_DIR=$(mktemp -d) || { echo "::warning::Could not create a temporary directory for extraction."; emit_none; }
...
find "${AX_DIR:?}" -mindepth 1 -delete   # per artifact
...
rm -rf "${AX_DIR:?}" "${ZIP_TMP}"        # after the loop

BINLOG_DIR I have deliberately left alone, and I want to be straight that this is a real limitation rather than a rebuttal. It is not scratch space — it is an interface. The workflow sets it, the agent container maps it to /data/binlogs, and the upload step reads it; that mount is part of the generated lock file, so a mktemp -d path would have to be threaded through the container spec that gh aw emits. That is a change to generated infrastructure, not to this script, and I would rather not make it as a drive-by in a backport.

What that leaves is the residual you identified: two of these jobs on one shared runner could collide in /tmp/binlogs. The rm -f "${BINLOG_DIR}"/*.binlog is there for the sequential case, which is the one that actually bites — a stale binlog from an earlier run being uploaded and attributed to this build, which produces a confidently wrong root cause on someone's PR. For the concurrent case it is not sufficient, and I have not claimed otherwise.

These workflows target ubuntu-latest, where each job gets its own VM, so this is not reachable as configured. If a repo moves them to shared runners, BINLOG_DIR is the thing to revisit, and it is a one-line change at the call site now that nothing else in the script hardcodes a path.

Note

Parts of this reply were generated with AI assistance.

[Copilot]

Pull request overview

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

[Copilot]

Pull request overview

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

[Copilot]

Pull request overview

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

[Copilot]

Pull request overview

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

Suppressed comments (4)

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

.github/workflows/build-failure-analysis.md:220

• The warning message in ado_get reports "curl exit ${rc}", but rc is the exit status of the timeout ... curl ... wrapper (e.g., 124 on timeout), not always curl’s own exit code. This can be misleading when diagnosing ADO fetch failures.

            if [ "${rc}" -ne 0 ] || [ -z "${ADO_DOC}" ]; then
              echo "::warning::Could not fetch the ${what} from Azure DevOps (curl exit ${rc}); treating as a data-resolution failure."
              return 1
            fi

.github/workflows/build-failure-analysis-command.md:311

• The warning message in ado_get reports "curl exit ${rc}", but rc is the exit status of the timeout ... curl ... wrapper (e.g., 124 on timeout), not always curl’s own exit code. This can be misleading when diagnosing ADO fetch failures.

            if [ "${rc}" -ne 0 ] || [ -z "${ADO_DOC}" ]; then
              echo "::warning::Could not fetch the ${what} from Azure DevOps (curl exit ${rc}); treating as a data-resolution failure."
              return 1
            fi

.github/workflows/build-failure-analysis.lock.yml:1497

• The warning message in ado_get reports "curl exit ${rc}", but rc is the exit status of the timeout ... curl ... wrapper (e.g., 124 on timeout), not always curl’s own exit code. This can be misleading when diagnosing ADO fetch failures.

            if [ "${rc}" -ne 0 ] || [ -z "${ADO_DOC}" ]; then
              echo "::warning::Could not fetch the ${what} from Azure DevOps (curl exit ${rc}); treating as a data-resolution failure."
              return 1
            fi

.github/workflows/build-failure-analysis-command.lock.yml:1623

• The warning message in ado_get reports "curl exit ${rc}", but rc is the exit status of the timeout ... curl ... wrapper (e.g., 124 on timeout), not always curl’s own exit code. This can be misleading when diagnosing ADO fetch failures.

            if [ "${rc}" -ne 0 ] || [ -z "${ADO_DOC}" ]; then
              echo "::warning::Could not fetch the ${what} from Azure DevOps (curl exit ${rc}); treating as a data-resolution failure."
              return 1
            fi