ci: name every artifact per run instead of overwriting a shared name (#3314)

.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,29 +783,39 @@ 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.
       - 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 }}
+          # 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

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
+`<that name>-${{ 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,