refactor(ci): drop the vestigial /tmp workspace from e2e (both workflows) - #3454

LGTM. Correct root-cause safety fix.

Without the guard, an empty SANDBOX_NAME (e.g. when the setup step never exported it before this if: always() cleanup) turns rm -rf /tmp/$SANDBOX_NAME into rm -rf /tmp/, wiping the entire /tmp of the self-hosted runner (the e2e job runs on a persistent oracle-vm runner, so this affects other/subsequent runs).

The guard is correct in every branch under the default bash -e -o pipefail shell:

• name set, rm ok -> exit 0
• name set, rm fails -> || true swallows it, cleanup step stays green
• name empty -> [ -n ] short-circuits, rm never runs -> exit 0

Bonus: quoting "/tmp/$SANDBOX_NAME" handles names with special characters.

NOT LGTM — the guard is correct as code, but the description's diagnosis is wrong, its impact claim is wrong, and the scope is one step out of fourteen across two workflows. I'd rather see this land as the cleanup it points at than as a patch on one symptom.

What the workflow actually does

Set sandbox ID (590) carries no condition, so it inherits the implicit success() that every unconditional step gets. Any failure ahead of it skips it — Apply patch on a conflicting pr.patch, either download-artifact, Checkout code, the release-path curl — as does a cancellation that reaches the job after it has started. In all of those cases SANDBOX_NAME is never written to $GITHUB_ENV and stays empty for the remainder of the job.

Eight steps then run anyway, because they carry if: always(), and every one of them interpolates that empty value. Remove workspace is the destructive one: rm -rf /tmp/$SANDBOX_NAME expands to rm -rf /tmp/.

So the hazard is real and worth removing. What the description says causes it is not what causes it.

Corrections to the description

"On GitHub's re-run failed jobs, the earlier steps that populate SANDBOX_NAME (Set sandbox ID / Prepare workspace) are skipped." There is no step-level re-run in GitHub Actions — a new job attempt starts at the first step and re-evaluates every if: from scratch. Set sandbox ID is unconditional and lives in the same job, so a re-run runs it. Steps get skipped here for the ordinary reason: something ahead of them failed. The re-run is not the trigger; incomplete initialization is, by any of the routes above. Worth noting that 792951b9c recently moved the job condition to !cancelled(), which makes the cancellation route reachable.

"so 're-run failed jobs' could never go green for E2E and every failure forced a full re-run." An empty SANDBOX_NAME implies initialization already failed or the job was cancelled, so that job's conclusion is already fixed before cleanup runs. The guard turns two red steps into one red step. It cannot make such a job green, and it does not change whether re-runs work in general — they do.

Blast radius. The approving review justifies the change as "wiping the entire /tmp of the self-hosted runner (the e2e job runs on a persistent oracle-vm runner, so this affects other/subsequent runs)". #3268 moved all CI off the persistent self-hosted runner onto the ephemeral pool, and this job runs there (runs-on: oracle-vm-32cpu-128gb-x86-64, 529). The reasoning describes a topology we retired. Still worth fixing — just not for that reason.

Scope

Eight if: always() steps in this job interpolate SANDBOX_NAME with no check that it was ever set:

• Collect chainsaw report (709) — cd /tmp/$SANDBOX_NAME + mkdir -p _out, which on an empty value creates /tmp/_out on the runner
• Collect report (726) and Collect images list (739) — make -C packages/core/testing … invoked from /tmp
• the three Upload … steps (717, 732, 745) — path: /tmp/${{ env.SANDBOX_NAME }}/_out/… → /tmp//_out/…
• Tear down sandbox (802) — docker rm -f "" → container name cannot be empty (harmless, as the description says)
• Remove workspace (806) — the one this PR fixes

.github/workflows/nightly.yaml has the same shape and is untouched by this PR: Set sandbox ID (273), Prepare workspace (277), six if: always() steps (330–364), and the same unguarded removal at 366. It quotes its paths, which does nothing for an empty value, and it has no images collect/upload pair. Eight plus six is fourteen; this PR changes one.

What I think the actual fix is

SANDBOX_NAME started life as a rendezvous key and only later became a workspace directory name too, and the reason for the rendezvous is gone.

Before 505b693c3 ("[ci] Run e2e tests on shared runners") E2E was five state-sharing runs-on: [self-hosted] jobs — prepare_env, install_cozystack, test_apps, collect_debug_information, cleanup — each carrying its own copy of the same Set sandbox ID step, all relying on shared /tmp and dockerd state on the self-hosted runner. That is what sha256(GITHUB_REPOSITORY:GITHUB_WORKFLOW:GITHUB_REF) buys: jobs that cannot pass state to each other can each re-derive the same value, keyed so concurrent branches don't collide and a re-push replaces its predecessor. The /tmp workspace came in alongside it — b2a697f98 introduced mv cozystack /tmp/$SANDBOX_NAME, and b3380d836 changed the mv to cp -r so the move would stop confusing the checkout action.

Today it is one job on the ephemeral pool. No cross-job rendezvous, no sibling jobs sharing /tmp, and the copy is protecting a checkout directory that is no longer reused across runs.

Two ways to go, and I'd take the first:

1. Drop the /tmp workspace. Remove Prepare workspace and Remove workspace, run in $GITHUB_WORKSPACE, drop the cd /tmp/$SANDBOX_NAME prefixes, and change the three upload paths from /tmp/${{ env.SANDBOX_NAME }}/_out/… to _out/…. That deletes the rm rather than guarding it, and it takes the empty-value expansion out of the upload paths and the cds at the same time. Note this does not let us drop Set sandbox ID: the job reads $SANDBOX_NAME directly in the failure diagnostics (619–629, 689–699), the chainsaw docker cp (714), teardown (804), and every make SANDBOX_NAME=… argument. The container still needs a name — it just doesn't need a directory.
2. If you'd rather keep the /tmp copy, gate the block instead of the command: if: always() && env.SANDBOX_NAME != '' on all eight steps here and all six in nightly.yaml. That subsumes the rm guard and covers the stray mkdir and the /tmp//_out upload paths too.

Either way, one detail is worth knowing: SANDBOX_NAME := in packages/core/testing/Makefile is a plain assignment, and a command-line make SANDBOX_NAME= beats it. Passing the variable explicitly does not fall back to the Makefile default when it is empty — it actively blanks it, which is why teardown reaches docker rm -f "" rather than removing a default-named container. The one place a sane default existed, the workflow overrides it with nothing.

While you're in there — other things the single-runner retirement left behind

Small, none urgent, all in packages/core/testing/Makefile:

• apply still does mkdir -p /tmp/${SANDBOX_NAME} and chmod 777 /tmp/${SANDBOX_NAME} (71–72). Those backed a bind mount added in fa6442998 as -v /tmp/${SANDBOX_NAME}:/workspace/hosttmp, widened to -v /tmp:/workspace/hosttmp in 433bfe7b6, and removed entirely in f891d0bee. No e2e-sandbox code references /workspace/hosttmp any more, but the world-writable directory is still created on every apply.
• The same target passes -e SANDBOX_NAME=${SANDBOX_NAME} into the container (77), and nothing under packages/core/testing/images/e2e-sandbox/ reads it.

A process note, and the one occurrence I can find

Everything above is read off the workflow, the Makefile and the history, because the PR gives nothing to read it against: the description asserts the mechanism in prose with no run link, no job link, no log excerpt and no issue reference, and the commit message has none either. I think that is why the wrong cause went unchallenged and why the approval reasoned from a runner topology we no longer have. For a change whose whole argument is "this happens on re-run", a link to a job where it happened would have settled it in one click.

I did go looking, and I want to be careful about how I present what I found, because it is not this PR's motivating case and it should not be read as one. Run 30067779342 attempt 3 exhibits the failure, but it started at 2026-07-27T14:34:02Z — about three and a half hours after this PR was opened at 11:00:03Z. Attempts 1 and 2 of that same run both completed Remove workspace successfully. So it is a later, independent occurrence that happens to demonstrate the mechanism; nobody could have been looking at it when the description was written.

What it shows is that the trigger is not the one described. Download Talos image (regular PR) failed on GetSignedArtifactURL with a 404, which skipped Set sandbox ID along with everything else unconditional, and the eight always() steps then ran on an empty value — Collect chainsaw report created /tmp/_out, the uploads resolved to /tmp//_out/…, teardown ran docker rm -f "", and Remove workspace did this:

rm: cannot remove '/tmp/systemd-private-…-systemd-timedated.service-1oADZP': Operation not permitted
rm: cannot remove '/tmp/.X11-unix': Operation not permitted
##[error]Process completed with exit code 1.

Note the shape of that: the failures are all Operation not permitted on root-owned entries in a sticky /tmp, and the job's workdir is /home/ubuntu/_work/…, so the step ran unprivileged and could not reach anything it did not own. Also worth knowing for anyone chasing the download 404 separately: the artifact listing succeeded and only the signed-URL call failed, ~190 ms in, because download-artifact treats 404 as non-retryable. It is not deterministic — attempt 2 of the same run fetched the same artifact fine — but one 404 establishes nondeterminism, not a root cause. That deserves its own issue rather than being folded in here.

Requesting changes on framing and scope, not on the line itself. Happy to take the cleanup myself if you'd rather not carry it.

LGTM. The rework matches the recommended approach (drop the /tmp workspace entirely rather than guard it), and both workflows (pull-requests.yaml and nightly.yaml) are covered. The corrected description now accurately attributes the empty SANDBOX_NAME to incomplete initialization, and the Makefile dead code (world-writable mkdir + chmod 777, -e SANDBOX_NAME) is removed. Traced path/CWD resolution end-to-end (make targets, docker cp, upload-artifact paths, ROOT_DIR) — all consistent; repo-wide sweep finds no remaining /tmp/$SANDBOX_NAME references.

Two housekeeping notes: the do-not-merge/hold label is still set, and the now-outdated inline thread on pull-requests.yaml should be marked resolved.

LGTM — the rework implements Option 1 exactly as scoped, across both workflows plus the Makefile, and every point I raised is closed with no new failure mode introduced.

Going through my earlier review point by point.

Cause, impact and blast radius — closed. The rewritten description states the trigger as incomplete initialization rather than a step-level re-run, states plainly that the change cannot turn a red job green, and retracts the persistent-/tmp blast-radius reasoning as describing a topology we retired. It also now carries the run link and log excerpt the earlier version lacked, and correctly frames run 30067779342 attempt 3 as a later independent occurrence rather than the motivating case.

The /tmp workspace is gone, not guarded — closed. Prepare workspace and Remove workspace are removed from both jobs; the job now runs in $GITHUB_WORKSPACE. .github/workflows/pull-requests.yaml:589 and .github/workflows/nightly.yaml:273 keep Set sandbox ID, which is right — the container still needs a name for the diagnostics docker execs, the chainsaw docker cp, teardown and every make SANDBOX_NAME=… argument.

Scope — closed, both workflows. The always() step count drops from 8 to 7 in pull-requests.yaml (700, 707, 716, 721, 728, 733, 790) and from 6 to 5 in nightly.yaml (323, 330, 338, 343, 351) — exactly the one destructive step removed from each. The remaining steps still interpolate a possibly-empty SANDBOX_NAME, and that is fine under Option 1 rather than a leftover: with the cds gone every one of them is now inert on an empty value. Collect chainsaw report (pull-requests.yaml:699, nightly.yaml:322) does mkdir -p _out in the workspace instead of /tmp/_out on the runner, and its docker cp is || true; the collect steps are || true; the uploads carry if-no-files-found: ignore/warn (pull-requests.yaml:725 and :737 take the warn default); teardown reaches docker rm -f "" quoted and || true. Nothing left in either workflow interpolates a possibly-empty variable into a destructive command.

Upload paths — closed, and the artifact layout is unchanged. The three PR paths (pull-requests.yaml:711, :725, :737) and the two nightly paths (nightly.yaml:334, :347) are now relative. Neither workflow sets a defaults.run.working-directory, so upload-artifact resolves them against $GITHUB_WORKSPACE, which is where mkdir -p _out and the Makefile's ../../../_out both land. Each path names a single literal file, so its parent directory is the artifact root and the artifact still contains the bare chainsaw-report.xml / cozyreport.tgz / images.txt — same as under the old absolute form.

Path and CWD resolution — verified end to end. ROOT_DIR at packages/core/testing/Makefile:7 derives the repo root from the Makefile's own location, not from the caller's cwd, so dropping the cd prefixes leaves docker cp "${ROOT_DIR}/." …:/workspace (:77) copying the same tree it copied before — the previous /tmp/$SANDBOX_NAME was itself just a cp -r of that workspace. Root prepare-env (Makefile:180) enters packages/core/testing, from which copy-nocloud-image (:34), collect-report (:56-57) and collect-images (:61-62) resolve ../../../_out to the repo-root _out in both the old and the new layout.

Makefile dead code — closed. apply (packages/core/testing/Makefile:70) goes straight from delete to docker run: the world-writable mkdir -p /tmp/${SANDBOX_NAME} + chmod 777 and the -e SANDBOX_NAME=${SANDBOX_NAME} are both gone. A tree-wide sweep confirms nothing reads SANDBOX_NAME inside the container — every remaining occurrence is a host-side workflow line or a make argument — and no /tmp/$SANDBOX_NAME reference survives anywhere. The only remaining hosttmp hits are in the vendored HAMi device-plugin chart, unrelated to the retired bind mount.

In-place workspace mutation does not change behaviour. git apply _out/assets/pr.patch (pull-requests.yaml:573) leaves uncommitted working-tree changes, and Select E2E tests (:637) diffs the commit range origin/${BASE_REF}...HEAD, which those changes cannot affect — and the old /tmp copy carried a byte-identical .git, so the selection is the same either way. Nightly's Stage GHCR install tree (nightly.yaml:251) deliberately overlays packages/ before the sandbox is created, and every later consumer wants that staged tree; no step downstream depends on a pristine checkout.

The nightly comment rewrite is accurate. The old wording justified persist-credentials: false by the /tmp copy, which was never true for the mirror job in the first place. The replacement at nightly.yaml:123-125 holds: registry auth is established before checkout (:62), and the three steps after it only pull, mirror and push registry artifacts (:130, :140, :145) with no git operation — hack/nightly-mirror.sh contains none.

And it is empirically green on the reworked commit. Run 30375236044 on 78fc3f917 completed the E2E job with every step in the reworked path successful — Prepare environment, Install Cozystack, Select E2E tests, both test steps, all three collect/upload pairs and Tear down sandbox — and all three artifacts uploaded non-empty (chainsaw-report 1165 B, cozyreport 384824 B, image-list 18128 B). Since the change touches packages/core/ and the E2E workflow, TIA escalated to the full suite, so the relative upload paths and the no-cd make invocations are exercised rather than argued.

Non-blocking follow-ups

1. The download-artifact 404 on GetSignedArtifactURL that the description cites as the observed trigger still has no issue of its own — a search turns up nothing filed. Worth opening so the non-deterministic 404 is tracked separately, as the description says it should be.
2. nightly.yaml's E2E job is not exercised by PR CI, so its half of this change is verified by inspection only; the first live run is the next nightly. The two jobs are now structurally identical in this area, so the residual risk is low, but it is worth a glance at the next nightly.
3. Optional, pre-existing: pull-requests.yaml:725 and :737 rely on upload-artifact's default if-no-files-found: warn while nightly states it explicitly at :348. Harmless, but stating it in both places would make the intent local.
4. Optional, pre-existing and out of scope here: SANDBOX_NAME := at packages/core/testing/Makefile:5 is still beaten by a command-line make SANDBOX_NAME=, so passing the variable explicitly blanks it rather than falling back to the default. Nothing depends on that default today; noting it only so it is not rediscovered as a bug.