From 0d5ab3f50ef76b47ee73fac8787fadd028fec1f3 Mon Sep 17 00:00:00 2001 From: Jinwoo-H Date: Wed, 9 Sep 2026 19:33:37 -0400 Subject: [PATCH] ci: give each e2e.yml caller its own artifact namespace pr.yml now calls the reusable e2e.yml twice in one workflow run: the advisory `e2e` job and the blocking `orchestration_e2e` job. Both start e2e.yml's `build` job, and both would upload `e2e-build-out`. upload-artifact v4+ fails outright when two jobs in one run upload the same name, so a PR touching orchestration source AND any other routed source would kill whichever build finished second -- taking the new required check red for a reason that has nothing to do with the PR. `playwright-traces-changed` had the same collision on failure in both lanes. Add an optional `artifact_suffix` workflow_call input, default '', and append it to every artifact name e2e.yml uploads or downloads -- not just the two that can collide today, because a bare name left behind would fail only when a spec fails in both lanes, hiding the signal exactly when it matters. The advisory caller passes nothing; schedule and dispatch runs pass no inputs at all, so both keep the names they have always used. `orchestration_e2e` passes `-orchestration`. release-e2e-dispatch-contract's "hands the built relay artifact to every E2E run command" pinned the literal `e2e-build-out` on the upload and both downloads. It now compares the downloads against the upload's own name, which is what the test was really guarding: upload and download must agree. A mutant that renames one download still fails it. Verified: actionlint resolves the caller/callee input contract (a deliberately typo'd input name is reported), and both new assertions were mutation-checked. --- .github/workflows/e2e.yml | 26 +++++++---- .github/workflows/pr.yml | 5 +++ .../pr-e2e-orchestration-routing.test.mjs | 45 +++++++++++++++++++ .../release-e2e-dispatch-contract.test.mjs | 8 +++- 4 files changed, 73 insertions(+), 11 deletions(-) diff --git a/.github/workflows/e2e.yml b/.github/workflows/e2e.yml index 942f0f34a56..36e39df10ed 100644 --- a/.github/workflows/e2e.yml +++ b/.github/workflows/e2e.yml @@ -21,6 +21,14 @@ on: description: '"true" when the PR touches SSH execution source; gates the Docker-SSH lane' required: false type: string + artifact_suffix: + description: >- + Appended to every artifact name this workflow uploads or downloads. A caller that + invokes this workflow twice in one run must give the second call a distinct value: + upload-artifact fails outright when two jobs in the same run upload one name. + required: false + default: '' + type: string workflow_dispatch: inputs: ref: @@ -70,7 +78,7 @@ jobs: - name: Upload E2E build output uses: actions/upload-artifact@v7 with: - name: e2e-build-out + name: e2e-build-out${{ inputs.artifact_suffix }} path: out/ # Why: build-relay.mjs writes each relay's marker as `out/relay//.version`, # and upload-artifact drops dotfiles by default — consumers then fail SSH specs with @@ -159,7 +167,7 @@ jobs: - name: Download E2E build output uses: actions/download-artifact@v8 with: - name: e2e-build-out + name: e2e-build-out${{ inputs.artifact_suffix }} path: out/ # Why: the Electron suite is wall-clock constrained on OSS runners, but @@ -180,7 +188,7 @@ jobs: if: failure() uses: actions/upload-artifact@v7 with: - name: playwright-traces-${{ matrix.shard_name }} + name: playwright-traces-${{ matrix.shard_name }}${{ inputs.artifact_suffix }} path: test-results/ retention-days: 7 if-no-files-found: ignore @@ -214,7 +222,7 @@ jobs: - name: Download E2E build output uses: actions/download-artifact@v8 with: - name: e2e-build-out + name: e2e-build-out${{ inputs.artifact_suffix }} path: out/ - name: Run changed E2E specs @@ -258,7 +266,7 @@ jobs: if: failure() uses: actions/upload-artifact@v7 with: - name: playwright-traces-changed + name: playwright-traces-changed${{ inputs.artifact_suffix }} path: test-results/ retention-days: 7 if-no-files-found: ignore @@ -301,7 +309,7 @@ jobs: - name: Download E2E build output uses: actions/download-artifact@v8 with: - name: e2e-build-out + name: e2e-build-out${{ inputs.artifact_suffix }} path: out/ # Why: this is the release-path proof that the deployed Linux relay keeps @@ -354,7 +362,7 @@ jobs: if: failure() uses: actions/upload-artifact@v7 with: - name: playwright-traces-ssh-docker-watcher-isolation + name: playwright-traces-ssh-docker-watcher-isolation${{ inputs.artifact_suffix }} path: e2e-traces/ retention-days: 7 if-no-files-found: ignore @@ -396,7 +404,7 @@ jobs: native-runtime: electron - uses: actions/download-artifact@v8 with: - name: e2e-build-out + name: e2e-build-out${{ inputs.artifact_suffix }} path: out/ - name: Start isolated localhost SSH server shell: bash @@ -438,7 +446,7 @@ jobs: - uses: actions/upload-artifact@v7 if: failure() with: - name: localhost-ssh-traces + name: localhost-ssh-traces${{ inputs.artifact_suffix }} path: test-results/ retention-days: 7 if-no-files-found: ignore diff --git a/.github/workflows/pr.yml b/.github/workflows/pr.yml index ec3484bb938..eb9baeb298d 100644 --- a/.github/workflows/pr.yml +++ b/.github/workflows/pr.yml @@ -962,6 +962,11 @@ jobs: with: ref: ${{ github.event.pull_request.head.sha }} test_files: ${{ needs.code_paths.outputs.orchestration_test_files }} + # Why: a PR touching orchestration source AND another routed source runs both callers of + # e2e.yml in one workflow run, and upload-artifact fails when two jobs upload one name. + # Without this the second `build` to finish dies on e2e-build-out and takes a required + # check red for a reason unrelated to the PR. + artifact_suffix: -orchestration # Why this is not in verify's needs: it is the first PR-gate run of a harness whose reliability # is only known from nightly main runs (20/20 green, 2026-08-09..2026-08-29, p50 3m25s). It diff --git a/config/scripts/pr-e2e-orchestration-routing.test.mjs b/config/scripts/pr-e2e-orchestration-routing.test.mjs index 5e9398e77e2..2e07c2c7e35 100644 --- a/config/scripts/pr-e2e-orchestration-routing.test.mjs +++ b/config/scripts/pr-e2e-orchestration-routing.test.mjs @@ -246,4 +246,49 @@ describe('orchestration PR E2E routing', () => { } expect(e2eWorkflow.jobs['changed-e2e'].if).toBe("inputs.test_files != ''") }) + + it('gives each caller of e2e.yml its own artifact namespace', () => { + // Why: pr.yml now calls e2e.yml twice in one run. upload-artifact fails outright when two + // jobs in a run upload one name, so without distinct suffixes a PR touching orchestration + // source AND another routed source kills whichever `build` uploads e2e-build-out second -- + // taking a required check red for a reason that has nothing to do with the PR. + const callers = Object.entries(prWorkflow.jobs).filter( + ([, job]) => job.uses === './.github/workflows/e2e.yml' + ) + expect(callers.length).toBeGreaterThan(1) + const suffixes = callers.map(([, job]) => String(job.with?.artifact_suffix ?? '')) + expect(new Set(suffixes).size, `duplicate artifact_suffix among ${suffixes.join(', ')}`).toBe( + callers.length + ) + expect(prWorkflow.jobs.orchestration_e2e.with.artifact_suffix).toBe('-orchestration') + // The advisory caller keeps the empty default so schedule and dispatch runs, which pass no + // inputs at all, land on the same artifact names they always have. + expect(prWorkflow.jobs.e2e.with.artifact_suffix).toBeUndefined() + expect(e2eWorkflow.on.workflow_call.inputs.artifact_suffix).toMatchObject({ + required: false, + default: '', + type: 'string' + }) + }) + + it('suffixes every artifact e2e.yml uploads or downloads', () => { + // Why every one, not just e2e-build-out: a trace upload that kept a bare name would fail the + // job only when a spec fails in both lanes -- a red that appears exactly when the suite is + // already telling you something, and hides it. + const artifactSteps = Object.values(e2eWorkflow.jobs) + .flatMap((job) => job.steps ?? []) + .filter((step) => /^actions\/(?:upload|download)-artifact@/.test(step.uses ?? '')) + expect(artifactSteps.length).toBeGreaterThan(0) + for (const step of artifactSteps) { + expect(step.with?.name, JSON.stringify(step.with)).toMatch( + /\$\{\{ inputs\.artifact_suffix \}\}$/ + ) + } + // A concurrency group would serialize or cancel one call against the other; e2e.yml has none + // and must not grow one without keying it the same way. + expect(e2eWorkflow.concurrency).toBeUndefined() + for (const job of Object.values(e2eWorkflow.jobs)) { + expect(job.concurrency).toBeUndefined() + } + }) }) diff --git a/config/scripts/release-e2e-dispatch-contract.test.mjs b/config/scripts/release-e2e-dispatch-contract.test.mjs index 5fd4503d4e3..21a26d7f1f5 100644 --- a/config/scripts/release-e2e-dispatch-contract.test.mjs +++ b/config/scripts/release-e2e-dispatch-contract.test.mjs @@ -82,7 +82,11 @@ describe('release E2E dispatch contract', () => { (step) => step.name === 'Upload E2E build output' ) - expect(uploadStep.with.name).toBe('e2e-build-out') + // Compared against the upload rather than a literal: the name carries a per-caller suffix so + // two calls of this workflow in one run cannot collide, and what has to hold is that every + // download still asks for the artifact this job produced. + const buildArtifactName = uploadStep.with.name + expect(buildArtifactName).toContain('e2e-build-out') expect(uploadStep.with.path).toBe('out/') for (const [jobName, runStepName] of [ @@ -94,7 +98,7 @@ describe('release E2E dispatch contract', () => { const runStep = job.steps.find((step) => step.name === runStepName) expect(job.needs).toEqual(['build', 'prepare-native-cache']) - expect(downloadStep.with.name).toBe('e2e-build-out') + expect(downloadStep.with.name).toBe(buildArtifactName) expect(downloadStep.with.path).toBe('out/') expect(runStep.run).toContain('ORCA_RELAY_PATH="$GITHUB_WORKSPACE/out/relay"') }