fix(ci): retry transient E2E build artifact upload failures once (#11364) (#11375)

* fix(ci): retry transient E2E build artifact upload failures once (#11364)

* test(ci): harden the e2e-build upload retry contract pins (#11364)

Address review findings on the retry helper suite: scope the two-attempt
assertions to the archive instead of every upload-artifact step and bind
the attempts by step name, pin the pack-to-upload handoff (archive name
and step order), pin the build job's absent job-level continue-on-error,
pin the retry gate by whole expression so a failure() conjunct cannot
silently disable it, compare the two with: blocks wholesale (minus the
intentional overwrite asymmetry) plus the action pin, and compare
downloaded names against every workflow upload instead of restating the
four known legs. Also add the suite to the yamlSuites roster in
ci-platform-lanes.test.js so it can never join the dependency-free fast
lane undetected.

* fix(ci): correct the upload-retry rationale and name-key its contract (#11364)

Real-runner probes on the PR showed the comment's two reasons for the
retry's overwrite do not reproduce: a stalled upload aborts before
finalize and reserves nothing, and name uniqueness is scoped per run
attempt so a manual re-run does not 409 either. Reword both comments to
the window overwrite actually covers — a first attempt that finalized
e2e-build and only then reported failure.

The contract test picked "the archive" by position (uploads[0]),
contradicting its own comment: an unrelated upload step placed before
the archive upload reddened the suite with misleading messages. Key on
the literal e2e-build name instead — already the pinned contract in
scripts/tests/e2e-workflow.test.js — in both the filter and the
download-leg assertion. Mutation probes: a decoy upload step before the
archive now stays green, while decoy+retry-deleted, retry-deleted,
retry-renamed, and overwrite-dropped all still fail.

* test(ci): pin the pack-to-upload directory in the e2e retry contract (#11364)

Co-authored-by: Qwen-Coder <[email protected]>

* test(ci): pin the exact upload path in the e2e retry contract (#11364)

Round 3's handoff pin closed the pack side but left the upload side
pinned to a directory prefix, so moving only the two upload with.path
values into a subdirectory of runner.temp kept the suite green while
the runtime upload found no file and redded the build job. Compare
first.with.path to the exact '${{ runner.temp }}/' + archive literal,
reusing the derived archive binding rather than restating the name.
Mutation probes: the upload-side drift now fails on the new assertion
(16/1, expected-vs-actual), the workspace-escape and pack-side drift
mutations stay red on their existing assertions, and the same
upload-side mutation under the old prefix assertion stays green —
the exact pin is the load-bearing one.

Also reword the overwrite rationale in both the workflow comment and
the test comment to the form the shipped action source supports: the
record is created before any bytes move, so a failed first attempt can
leave the e2e-build name taken, and overwrite is delete-then-upload
and a no-op when nothing exists. The previous wording asserted a
stall reserves nothing, which the create-then-upload lifecycle
contradicts. And name the retry contract's actual ungated siblings
(run-e2e-tests.sh's sandbox-image build retry,
build-and-publish-image.yml's image build retry) instead of the
budget-gated sandbox:none shard retry the comment over-claimed parity
with.

* fix(ci): pin the unpack archive name and restore the overwrite rationale (#11364)

Round 4's handoff pin derives the archive name from the upload
with.path, so renaming the tarball at the pack step and both upload
paths while the four legs' unpack arguments stay put kept the suite
green while every leg would fail closed on main (review R1-3). Pin
each 'Unpack build artifact' run's trailing argument against the
derived name; the legs download into runner.temp/e2e-build/, so the
comparison is a suffix match, not the upload's full path. Mutation
probe: a producer-only rename now fails the contract with 'a leg
unpacks a different archive than the build job uploads' (16 pass / 1
fail), the unmutated workflow stays green (17/17), and the same
mutation was green before this pin (17/17).

Also restore the overwrite rationale that 03b921ba landed and 09deb2b
rewrote: the real-runner harness on this PR showed a stalled attempt
aborts before finalize and reserves nothing, and overwrite
(ListArtifacts -> DeleteArtifact) cannot rescue a name an un-finalized
attempt reserved, so the comments once again name the window overwrite
actually covers — an attempt that finalized e2e-build and only then
reported failure.

* fix(ci): announce absorbed e2e-build upload stalls so recurrence stays countable (#11364)

The upload retry absorbs a stalled first attempt behind
continue-on-error, so the build job concludes success and the main-CI
failure tracker — gated on workflow_run.conclusion == "failure" and
selecting jobs with conclusion == "failure" — never records the stall
the retry recovered from, freezing the recurrence count the #11364
ruling conditions on at one. Add a plain run step after the retry,
gated on steps.upload-build.outcome == 'failure', that emits a
::warning:: annotation: the same check-run surface that named run
34208365262's "Upload progress stalled.", so the absorbed stall stays
queryable without reddening the job or the four legs behind
needs: ['build']. The implicit success() gate from its placement after
the retry scopes it to the absorbed case; a double failure reddens the
job directly and the tracker already sees that.

Pin the step in the retry-contract test per the round-7 review's
acceptance criterion (R4-1): exact gate expression, ::warning::
emission, no action and no failing directive, placed after the retry.
Mutation probes: deleting the step, stripping ::warning::, negating
the gate, adding exit 1, moving the step before the retry, and giving
it a uses: action each turn the suite red (16 pass / 1 fail); the
unmutated workflow is green (17/17).

---------

Co-authored-by: Shaojin Wen <[email protected]>
Co-authored-by: qwen-code-dev-bot <[email protected]>
Co-authored-by: Qwen-Coder <[email protected]>

.github/scripts/e2e-build.test.mjs

@@ -14,9 +14,13 @@ import { tmpdir } from 'node:os';
 import { join } from 'node:path';
 import { after, before, describe, it } from 'node:test';
 import { fileURLToPath } from 'node:url';
+import { parse } from 'yaml';
 
 const PACK = fileURLToPath(new URL('./e2e-build-pack.sh', import.meta.url));
 const UNPACK = fileURLToPath(new URL('./e2e-build-unpack.sh', import.meta.url));
+const E2E_WORKFLOW = fileURLToPath(
+  new URL('../workflows/e2e.yml', import.meta.url),
+);
 const SHA = 'a'.repeat(40);
 // Three roots, two entries sharing one of them, one negated entry: the
 // script must scan every non-negated root once, not a list of its own.
@@ -424,3 +428,189 @@ describe('e2e build archive', () => {
     assert.ok(!existsSync(join(leg, 'dist/cli.js')));
   });
 });
+
+describe('e2e build artifact upload retry (e2e.yml build job)', () => {
+  // Run 34208365262 died with "Upload progress stalled." after ten minutes
+  // and skipped every leg: the upload is the single point that feeds all
+  // download-artifact consumers. The workflow's answer is one bounded
+  // retry, and its semantics live entirely in step properties nothing
+  // else asserts on. A regression here — the retry dropped, its trigger
+  // or overwrite removed, the names drifting apart — is silent until the
+  // next transient stall reddens a main run again, so pin the contract.
+  const doc = parse(readFileSync(E2E_WORKFLOW, 'utf8'));
+  const buildSteps = doc.jobs.build.steps;
+  const uploads = buildSteps.filter((s) =>
+    String(s.uses || '').startsWith('actions/upload-artifact@'),
+  );
+  const downloads = Object.values(doc.jobs).flatMap((job) =>
+    (job.steps ?? []).filter((s) =>
+      String(s.uses || '').startsWith('actions/download-artifact@'),
+    ),
+  );
+
+  it('keeps the two-attempt shape with the retry gated on the first outcome', () => {
+    // Scope to the archive, not the action: an unrelated second artifact
+    // in the build job must not redden the retry contract, and the two
+    // attempts are bound by step name, never by position.
+    const archiveUploads = uploads.filter((s) => s.with?.name === 'e2e-build');
+    assert.equal(
+      archiveUploads.length,
+      2,
+      'the e2e-build archive must be uploaded exactly twice: first attempt plus one bounded retry',
+    );
+    const first = archiveUploads.find(
+      (s) => s.name === 'Upload build artifact',
+    );
+    const retry = archiveUploads.find(
+      (s) => s.name === 'Upload build artifact (retry)',
+    );
+    assert.ok(first, "the build job must have an 'Upload build artifact' step");
+    assert.ok(
+      retry,
+      "the build job must have an 'Upload build artifact (retry)' step",
+    );
+    // The first attempt's failure must not red the job before the retry
+    // runs; the retry carries no continue-on-error, so a double failure
+    // still fails the job and a deterministic failure (missing archive)
+    // stays red through both attempts.
+    assert.equal(first.id, 'upload-build');
+    assert.equal(first['continue-on-error'], true);
+    assert.equal(retry['continue-on-error'], undefined);
+    // A job-level key computes the build conclusion green whatever either
+    // attempt exits, and every leg behind needs: ['build'] then runs against
+    // a missing artifact. isolated-nightly's deliberate job-level key is a
+    // different job — this pins build only.
+    assert.equal(doc.jobs.build['continue-on-error'], undefined);
+    // The whole expression, not a substring: a prepended failure() conjunct
+    // is false once the first attempt's continue-on-error absorbs the stall
+    // (its conclusion is success; only its outcome is failure), so the retry
+    // would never run while a substring pin still reads green.
+    assert.equal(retry.if, "${{ steps.upload-build.outcome == 'failure' }}");
+    // v4+ 409s a same-name upload only against an artifact finalized in
+    // this run attempt — a stall aborts before finalize and reserves
+    // nothing, so overwrite guards the finalize-then-fail window: an
+    // attempt that finalized e2e-build and only then reported failure.
+    assert.equal(retry.with.overwrite, true);
+    // Both attempts publish the same payload under the same name; the
+    // missing-archive guard rides on both so a pack regression fails
+    // fast in either attempt. overwrite is the one intentional asymmetry,
+    // so compare the with: blocks rather than a hand-picked key subset.
+    assert.equal(first.with['if-no-files-found'], 'error');
+    const retryWith = { ...retry.with };
+    delete retryWith.overwrite;
+    assert.deepEqual(
+      retryWith,
+      first.with,
+      'retry must publish the same payload as the first attempt',
+    );
+    assert.equal(
+      retry.uses,
+      first.uses,
+      'both attempts must run the same action pin',
+    );
+    // Pin the handoff, not just the agreement: the deepEqual above only
+    // compares the two upload with: blocks to each other, so a directory
+    // drift moving both together would stay green. The two sides spell
+    // one directory differently — the shell ${RUNNER_TEMP} in the pack
+    // run vs. the ${{ runner.temp }} expression in the upload with: — so
+    // pin each literal separately rather than comparing them as one.
+    const pack = buildSteps.find((s) => s.name === 'Pack build outputs');
+    assert.ok(pack, "the build job must have a 'Pack build outputs' step");
+    const archive = first.with.path.split('/').pop();
+    assert.ok(
+      pack.run.includes('"${RUNNER_TEMP}/' + archive + '"'),
+      'pack step must write the archive the upload publishes',
+    );
+    assert.equal(
+      first.with.path,
+      '${{ runner.temp }}/' + archive,
+      'the upload must publish the exact file the pack step writes',
+    );
+    assert.ok(
+      buildSteps.indexOf(pack) < buildSteps.indexOf(first) &&
+        buildSteps.indexOf(first) < buildSteps.indexOf(retry),
+      'pack must run before the first attempt, which must run before the retry',
+    );
+
+    // An absorbed first attempt leaves the job green, so the main-CI
+    // failure tracker — gated on conclusion == "failure" — never records
+    // the stall the retry recovered from. The announce step keeps it
+    // countable as a warning annotation (the same surface that named run
+    // 34208365262's "Upload progress stalled.") without reddening the job;
+    // after the retry, the implicit success() gate scopes it to the
+    // absorbed case — a double failure reddens the job directly.
+    const announce = buildSteps.find(
+      (s) => s.name === 'Announce absorbed upload failure',
+    );
+    assert.ok(
+      announce,
+      "the build job must have an 'Announce absorbed upload failure' step",
+    );
+    assert.equal(
+      announce.if,
+      "${{ steps.upload-build.outcome == 'failure' }}",
+      'the announce step must be gated on the first attempt outcome, exactly like the retry',
+    );
+    assert.ok(
+      !announce.uses,
+      'the announce step must be a plain run step — an action would carry its own failure modes',
+    );
+    assert.match(
+      announce.run,
+      /::warning::/,
+      'the announce step must emit a warning annotation the check-run annotations API keeps queryable',
+    );
+    assert.doesNotMatch(
+      announce.run,
+      /::error::|exit\s+[1-9]/,
+      'the announce step must not be able to turn the build job red',
+    );
+    assert.ok(
+      buildSteps.indexOf(retry) < buildSteps.indexOf(announce),
+      'the announce step must run after the retry so it fires only for the absorbed failure',
+    );
+    // The archive name above is derived from the upload side, so the
+    // consumer side must be pinned against it too: a rename moving the
+    // pack step and both upload paths together re-derives `archive` and
+    // stays green here while every leg still unpacks the old name. The
+    // legs download into runner.temp/e2e-build/ and unpack from there, so
+    // assert the run's trailing argument, not the upload's full path.
+    const unpacks = Object.values(doc.jobs).flatMap((job) =>
+      (job.steps ?? []).filter((s) => s.name === 'Unpack build artifact'),
+    );
+    assert.equal(unpacks.length, downloads.length);
+    for (const unpack of unpacks) {
+      assert.ok(
+        unpack.run.endsWith('/' + archive + '"'),
+        'a leg unpacks a different archive than the build job uploads',
+      );
+    }
+  });
+
+  it('feeds every download leg a name the workflow actually uploads', () => {
+    // Compare against every name the workflow uploads: a consumed name no
+    // job uploads still fails, while a second, correctly-uploaded artifact
+    // does not. The four known legs' agreement with the first attempt's
+    // name is already pinned in scripts/tests/e2e-workflow.test.js.
+    const consumed = new Set(downloads.map((s) => s.with?.name));
+    const uploaded = new Set(
+      Object.values(doc.jobs).flatMap((job) =>
+        (job.steps ?? [])
+          .filter((s) =>
+            String(s.uses || '').startsWith('actions/upload-artifact@'),
+          )
+          .map((s) => s.with?.name),
+      ),
+    );
+    for (const name of consumed) {
+      assert.ok(
+        uploaded.has(name),
+        `a leg downloads '${name}', which no job uploads`,
+      );
+    }
+    assert.ok(
+      consumed.has('e2e-build'),
+      'no leg downloads the artifact the build job publishes',
+    );
+  });
+});

.github/workflows/e2e.yml

@@ -87,7 +87,35 @@ jobs:
       - name: 'Pack build outputs'
         run: 'bash .github/scripts/e2e-build-pack.sh "${RUNNER_TEMP}/e2e-build.tar.gz"'
 
+      # One bounded retry, same ungated contract as the sandbox-image
+      # build retry (run-e2e-tests.sh) and the image build retry
+      # (build-and-publish-image.yml): upload-artifact dying with
+      # "Upload progress stalled." is a transient runner-to-blobstore
+      # network class — run 34208365262 stalled ten minutes on the ~100MB
+      # archive and reded the whole run before any leg started. A
+      # deterministic failure (missing archive, via if-no-files-found:
+      # error) fails both attempts and keeps the job red. The first
+      # attempt's continue-on-error keeps that failure from failing the
+      # job before the retry runs; the retry's overwrite covers the
+      # finalize-then-fail window: v4+ 409s a same-name upload only
+      # against an artifact already finalized in this run attempt, and
+      # a stall aborts before finalize and reserves nothing — but an
+      # attempt that finalized e2e-build and only then reported failure
+      # would 409 the retry without it.
       - name: 'Upload build artifact'
+        id: 'upload-build'
+        uses: 'actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a' # v7.0.1
+        continue-on-error: true
+        with:
+          name: 'e2e-build'
+          path: '${{ runner.temp }}/e2e-build.tar.gz'
+          if-no-files-found: 'error'
+          # Already gzip-compressed.
+          compression-level: 0
+          retention-days: 1
+
+      - name: 'Upload build artifact (retry)'
+        if: "${{ steps.upload-build.outcome == 'failure' }}"
         uses: 'actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a' # v7.0.1
         with:
           name: 'e2e-build'
@@ -96,6 +124,18 @@ jobs:
           # Already gzip-compressed.
           compression-level: 0
           retention-days: 1
+          overwrite: true
+
+      # An absorbed stall must stay countable: a recovered upload leaves
+      # this job's conclusion green, so the main-CI failure tracker (which
+      # counts conclusion == "failure" jobs) never records it. The warning
+      # annotation keeps it on the check-run record without reddening the
+      # job; placed after the retry, the implicit success() gate scopes it
+      # to the absorbed case — a double failure reddens the job directly.
+      - name: 'Announce absorbed upload failure'
+        if: "${{ steps.upload-build.outcome == 'failure' }}"
+        run: |-
+          echo "::warning::Upload build artifact failed its first attempt and the retry absorbed it — count this toward the #11364 upload-stall recurrence threshold."
 
   e2e-test-linux:
     name: 'E2E Test (Linux) - ${{ matrix.sandbox }} - shard ${{ matrix.shard }}'

scripts/tests/ci-platform-lanes.test.js

@@ -661,7 +661,7 @@ describe('GitHub helper tests', () => {
   it('keeps the dependency-free fast lane off npm-package suites', () => {
     // The github_ci_only helper step runs before ANY dependency install (the
     // setup-node and `npm ci` steps are gated on the full profile), so every
-    // suite it lists must import node: builtins only. These 9 suites import
+    // suite it lists must import node: builtins only. These 10 suites import
     // the `yaml` npm package; letting the fast lane run the full list made an
     // ECS-updater-only fork PR fail closed with ERR_MODULE_NOT_FOUND on a
     // fresh hosted runner (#10548 review R6-1). The full-profile helper step
@@ -685,6 +685,7 @@ describe('GitHub helper tests', () => {
       '.github/scripts/ci-runner-routing.test.mjs',
       '.github/scripts/assign-pr-owner.test.mjs',
       '.github/scripts/ci-disk-pressure.test.mjs',
+      '.github/scripts/e2e-build.test.mjs',
     ];
     for (const suite of yamlSuites) {
       expect(depFreeSuites, suite).not.toContain(suite);