From 092cb5db2fd0db742f6199aa67d6aaa17a6b185f Mon Sep 17 00:00:00 2001 From: Xore Date: Sun, 27 Sep 2026 12:46:49 +0200 Subject: [PATCH] ci: name every artifact per run instead of overwriting a shared name (#3314) `overwrite: true` was on all nine upload-artifact steps in quality.yml since #3319, on the reading that a name which may already exist needs v4's conflict policy. It does not mean "the newest run wins": it deletes the existing artifact of that name before uploading, so on a name two runs share, the name resolves to whichever run got there last and the earlier run's evidence is gone. Every upload now ends its name in run_id and run_attempt, so a name is claimed exactly once and there is nothing left to overwrite. run_attempt is needed as well as run_id because a re-run of a run keeps its run_id. The same pass found the scripts-and-compose upload never uploaded anything at all: upload-artifact v4.4+ skips hidden paths, and `path: .ci-artifacts/` is a hidden directory, so the action logged "No files were found with the provided path" and, under if-no-files-found: ignore, said so quietly. The ml-worker and auth-events-worker JUnit files that retention was added for have therefore never been retained. Naming the two files fixes that and bounds every artifact's contents to the report its own lane wrote. 7-day retention and the always()/failure() split from #3319 are unchanged, no upload was removed, and the ADVISORY set in the zizmor gate is untouched. --- .github/workflows/quality.yml | 122 ++++++++++++++++++++++++++++------ docs/CI-CD.md | 29 ++++++++ 2 files changed, 129 insertions(+), 22 deletions(-) diff --git a/.github/workflows/quality.yml b/.github/workflows/quality.yml index 7ccf2f9a..3766299b 100644 --- a/.github/workflows/quality.yml +++ b/.github/workflows/quality.yml @@ -570,17 +570,46 @@ jobs: # than of what the exit code happened to be. 7 days matches the issue's # retention ask: long enough to still be there when a red run is # investigated the following week, short enough that a busy main does - # not accumulate them indefinitely. `overwrite` is v4's required - # conflict policy for a name that may already exist on a re-run. + # not accumulate them indefinitely. + # + # `overwrite: true` is gone from every upload in this file, and the + # artifact name now ends in the run's own identity. It was here from + # #3319, on the reading that a name which may already exist needs v4's + # conflict policy. What `overwrite: true` actually does is DELETE an + # existing artifact of that name before uploading, so on a name two + # runs share it is not "the newest run's results win" -- it is "the + # second uploader destroys the first's evidence", and the name then + # resolves to whichever run got there last, whose contents the earlier + # run never verified. A per-(run, attempt) name cannot collide, so there + # is nothing left to overwrite and no name an earlier run on the same + # ref could poison. + # + # run_attempt is in the name as well as run_id because a GitHub re-run + # of a run keeps its run_id: run_id alone still collides on the second + # attempt of the same run, which is the collision #3319 was really + # hitting. With both, a name is unique per (run, attempt), and the step + # that writes it runs once per (run, attempt) -- those two conditions + # together are what make upload-artifact v4's immutability mean + # something here. Every step below sits in a job that either has no + # twin or has one gated on the opposite answer from ci-target, so no two + # steps in a run can claim the same name. Nothing downstream reads these + # by name (there is no actions/download-artifact in .github/), so the + # per-run suffix costs no consumer. - name: Upload unit test results if: always() uses: actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02 # v4.6.2 with: - name: frontend-next-unit-junit + name: frontend-next-unit-junit-${{ github.run_id }}-${{ github.run_attempt }} + # The file is named explicitly rather than uploading its directory: + # upload-artifact v4.4+ skips hidden paths unless told otherwise, and + # `.ci-artifacts/` is hidden, so `path: .ci-artifacts/` uploads + # NOTHING (it reports "No files were found" and, with + # if-no-files-found: ignore, says so quietly). Naming the file is + # also what makes the contents a closed set: only the report the + # lane's own reporter wrote can ever land here. path: .ci-artifacts/frontend-next-unit-junit.xml if-no-files-found: warn retention-days: 7 - overwrite: true # No live-ES smoke suite here on purpose: port-tests/ needs the real # cluster over an SSH tunnel to the homeserver (see its README) -- # not reachable from a GitHub-hosted runner, and not appropriate to @@ -659,15 +688,17 @@ jobs: working-directory: arcane/home/honeypot-dashboard/frontend-next env: CI_ARTIFACTS_DIR: ${{ github.workspace }}/.ci-artifacts + # Same per-(run, attempt) artifact name and the same explicit-file path + # as the homeserver twin -- see that step for why `overwrite: true` is + # gone and why the path is not the `.ci-artifacts/` directory. - name: Upload unit test results if: always() uses: actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02 # v4.6.2 with: - name: frontend-next-unit-junit + name: frontend-next-unit-junit-${{ github.run_id }}-${{ github.run_attempt }} path: .ci-artifacts/frontend-next-unit-junit.xml if-no-files-found: warn retention-days: 7 - overwrite: true # No live-ES smoke suite here on purpose -- see the homeserver twin. - run: npm run build working-directory: arcane/home/honeypot-dashboard/frontend-next @@ -752,15 +783,18 @@ jobs: # error so a lane that never produced a report -- because it failed at # `npm ci`, say -- reports the absence instead of masking the real # failure behind an upload error. + # + # Per-(run, attempt) name, explicit file path, no `overwrite: true` -- + # all three for the reasons spelled out on the frontend-next twin's + # upload above. - name: Upload browser test results if: always() uses: actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02 # v4.6.2 with: - name: frontend-next-browser-junit + name: frontend-next-browser-junit-${{ github.run_id }}-${{ github.run_attempt }} path: .ci-artifacts/playwright-junit.xml if-no-files-found: warn retention-days: 7 - overwrite: true # `failure()`, not `always()`: the HTML report bundles a trace per # failing test and is the one artifact big enough to matter. On a green # run it holds nothing worth storing. @@ -768,13 +802,20 @@ jobs: if: failure() uses: actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02 # v4.6.2 with: - name: frontend-next-browser-report + name: frontend-next-browser-report-${{ github.run_id }}-${{ github.run_attempt }} + # Both directories are named, not the frontend-next tree around + # them. playwright.config.ts's own outputDir is ./test-results and + # the html reporter's is ./playwright-report, so this lane creates + # and owns both in a fresh checkout: the contents are what the + # browser matrix wrote, not whatever happens to be under + # frontend-next/ right now -- which is the property that keeps a + # stray .env or key out of a 7-day artifact anyone with repo read + # access can pull. path: | arcane/home/honeypot-dashboard/frontend-next/playwright-report/ arcane/home/honeypot-dashboard/frontend-next/test-results/ if-no-files-found: warn retention-days: 7 - overwrite: true frontend-next-browser-cloud: name: Dashboard-next browser matrix (GitHub-hosted) @@ -815,26 +856,26 @@ jobs: working-directory: arcane/home/honeypot-dashboard/frontend-next env: CI_ARTIFACTS_DIR: ${{ github.workspace }}/.ci-artifacts + # Twin of the homeserver copy's two uploads: same per-(run, attempt) + # names, same explicit paths, no `overwrite: true`. - name: Upload browser test results if: always() uses: actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02 # v4.6.2 with: - name: frontend-next-browser-junit + name: frontend-next-browser-junit-${{ github.run_id }}-${{ github.run_attempt }} path: .ci-artifacts/playwright-junit.xml if-no-files-found: warn retention-days: 7 - overwrite: true - name: Upload Playwright report and traces if: failure() uses: actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02 # v4.6.2 with: - name: frontend-next-browser-report + name: frontend-next-browser-report-${{ github.run_id }}-${{ github.run_attempt }} path: | arcane/home/honeypot-dashboard/frontend-next/playwright-report/ arcane/home/honeypot-dashboard/frontend-next/test-results/ if-no-files-found: warn retention-days: 7 - overwrite: true # First CI coverage for this crate (#1608's Rust service tier had none # until now -- a broken build or a real regression wouldn't have @@ -915,6 +956,11 @@ jobs: # than a hand-rolled conversion that could misrepresent a result. The # log is what the issue said was the only evidence a red run had; it is # now retained rather than scrolled away. + # + # `mkdir -p` plus a named file, never a directory upload: the log is + # the only thing this job writes under .ci-artifacts/, and naming it + # keeps the artifact's contents a closed set (see the frontend-next + # twin's upload for why the directory form uploads nothing at all). - name: Test (log retained on failure) shell: bash run: | @@ -923,15 +969,16 @@ jobs: PATH="$HOME/.cargo/bin:$PATH" cargo test 2>&1 | tee "$CI_ARTIFACTS_DIR/cargo-test.log" env: CI_ARTIFACTS_DIR: ${{ github.workspace }}/.ci-artifacts + # Per-(run, attempt) name, no `overwrite: true` -- see the frontend-next + # twin's upload for the full argument. - name: Upload test log if: failure() uses: actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02 # v4.6.2 with: - name: backend-service-cargo-test-log + name: backend-service-cargo-test-log-${{ github.run_id }}-${{ github.run_attempt }} path: .ci-artifacts/cargo-test.log if-no-files-found: warn retention-days: 7 - overwrite: true - run: PATH="$HOME/.cargo/bin:$PATH" cargo clippy --all-targets -- -D warnings backend-service-cloud: @@ -969,15 +1016,16 @@ jobs: cargo test 2>&1 | tee "$CI_ARTIFACTS_DIR/cargo-test.log" env: CI_ARTIFACTS_DIR: ${{ github.workspace }}/.ci-artifacts + # Twin of the homeserver copy's upload: same per-(run, attempt) name, + # same named-file path, no `overwrite: true`. - name: Upload test log if: failure() uses: actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02 # v4.6.2 with: - name: backend-service-cargo-test-log + name: backend-service-cargo-test-log-${{ github.run_id }}-${{ github.run_attempt }} path: .ci-artifacts/cargo-test.log if-no-files-found: warn retention-days: 7 - overwrite: true - run: cargo clippy --all-targets -- -D warnings vendored-theme: @@ -2072,20 +2120,50 @@ jobs: run: ${{ matrix.run }} # #3319: `always()` because the pytest rows emit their JUnit XML on the # passing run too, and that is the record of what ran. `ignore` on the - # missing-file case because this job is a ~22-row matrix and most rows + # missing-file case because this job is a ~65-row matrix and most rows # write no report at all -- a warning per row would bury the one that # matters. The matrix row name is in the artifact name so the two # pytest rows don't collide in the run's artifact list (upload-artifact # v4 refuses two uploads sharing a name). + # + # Two things this step had wrong, both fixed here rather than papered + # over with `overwrite: true`: + # + # 1. `path: .ci-artifacts/` uploaded NOTHING. upload-artifact v4.4+ + # skips hidden paths by default, and a directory whose own name + # starts with `.` is hidden, so the action reported "No files were + # found with the provided path: .ci-artifacts/" and, under + # if-no-files-found: ignore, said it quietly. Verified on a green + # run of the ml-worker row: 330 passed, pytest wrote + # .ci-artifacts/ml-worker-junit.xml, and the upload step below it + # still found no files. The two JUnit files this retention was + # added for have therefore never been retained. The path below + # names them explicitly, which both fixes that and makes the + # contents a closed set: those two files, written by the two rows' + # own --junitxml flags, and nothing else that happens to be sitting + # in a workspace-relative directory. + # 2. The name is per-(run, attempt), so no upload here can collide + # and no earlier run on the same ref can poison this one's name. + # See the frontend-next twin's upload for the full argument. + # + # One trap left standing on purpose: GitHub rejects `/` in an artifact + # name, and six row names contain one ("Healthcheck/autoheal ...", + # "Zeek lab/prod ...", "Payload/artifact dedupe ...", "scripts/tests + # suite", and two more). They are harmless today because they write no + # report, and upload-artifact only reaches the name when it has files + # to upload. A row that both writes a report AND has a `/` in its name + # will fail its upload -- and there is no expression-level way to + # sanitise a free-text field, so fixing that means renaming the row. - name: Upload test results if: always() uses: actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02 # v4.6.2 with: - name: scripts-${{ matrix.name }}-results - path: .ci-artifacts/ + name: scripts-${{ matrix.name }}-results-${{ github.run_id }}-${{ github.run_attempt }} + path: | + .ci-artifacts/ml-worker-junit.xml + .ci-artifacts/auth-events-worker-junit.xml if-no-files-found: ignore retention-days: 7 - overwrite: true scripts-and-compose-complete: name: Scripts and Compose diff --git a/docs/CI-CD.md b/docs/CI-CD.md index 20bc06d0..942f4a7f 100644 --- a/docs/CI-CD.md +++ b/docs/CI-CD.md @@ -257,6 +257,35 @@ through the unstable `--format json`. Retaining the full log is honest about that; converting it to XML here would mean a hand-rolled translation that could itself misreport a result. +### Names and paths + +The table above names *files*; the artifact each one lands in is named +`-${{ github.run_id }}-${{ github.run_attempt }}`, and no upload in +`quality.yml` sets `overwrite`. Two rules, both of them learned the hard way: + +- **`overwrite: true` is not "the newest run wins".** It deletes an existing + artifact of that name before uploading the new one, so on a name two runs + share it means "the second uploader destroys the first's evidence", and the + name then resolves to whichever run got there last — not the one a reader + is looking at. A per-`(run, attempt)` name is claimed exactly once, so there + is nothing to overwrite and no name an earlier run on the same ref could + poison. The attempt number is part of the name because a GitHub re-run of a + run keeps its `run_id`; `run_id` alone still collides on the second attempt + of the same run. Nothing in the tree downloads these by name, so the suffix + costs no consumer. +- **Paths name files, never the `.ci-artifacts/` directory.** + `actions/upload-artifact` v4.4+ skips hidden paths unless told otherwise, and + a directory whose own name begins with `.` is hidden — so `path: + .ci-artifacts/` uploads *nothing*, logging "No files were found with the + provided path" and, under `if-no-files-found: ignore`, saying so silently. + That is exactly what the two `scripts-and-compose` pytest rows did from + #3319 until this was fixed: a green run logged 330 ml-worker tests passed + and pytest's own "generated xml file" line, and the upload step directly + below it still found no files. Naming each report also bounds what can be + in it: the contents are the reports the lane's own reporter wrote, not + whatever happens to be sitting in a workspace-relative directory that + anyone with repo read access can download for the next 7 days. + ### The switch Both reporters are opt-in through a single environment variable,