HOMELAB-2479: fix(ci): fixture readiness requires non-empty FIX_PORT #886

Merged

pi-landing merged 3 commits from plane/HOMELAB-2479-fix-port-gate into live 2026-11-07 05:39:26 +00:00

Conversation 7 Commits 3 Files changed 3 +285 -23

aaron commented 2026-11-07 03:13:02 +00:00

Owner

Copy link

What

build-fixture on push run #3333 (sha e54f03b6, live, 2026-11-07T02:36:24Z) failed in 8s with Bad port '' / RUN exit status 255, in .forgejo/workflows/emu-images.yaml step "Verify the control API at runtime (add-key + ssh login)" at ssh -i /tmp/enrol -p "${FIX_PORT}".

Root cause (measured, from the live sources)

• fixtures/sshd-fixture.sh cmd_start writes dir, session, started_at to the state file before it starts sshd and writes port (after start_sshd succeeds).
• print_env prints export FIX_PORT=%s unquoted, so an empty value prints export FIX_PORT= and still exits 0.
• The readiness loop broke on sshd-fixture.sh env exit 0 alone, i.e. as soon as the state file existed. It then evaled an empty FIX_PORT and ssh refused with Bad port ''.

Timing-dependent, not content-dependent: the identical step was green on PR head d9fe5cc (run #3331, build-fixture 2m7s). The merge head differs only in merge-base, so the failure is a race, not a regression in #885's content.

Change

The wait now leaves the loop only when the printed env carries a numeric port:

            env_out=$(docker exec gits-fixture sshd-fixture.sh env 2>/dev/null) || env_out=""
            case "${env_out}" in
              *"export FIX_PORT="[0-9]*) break ;;
              *) env_out="" ;;   # state file exists but no port yet: keep waiting
            esac

Clearing env_out in the not-ready branch keeps the pre-existing [ -z "${env_out}" ] failure path intact, so a fixture that never reaches a port still fails the step with docker logs. One file, one step, no other behaviour touched.

Separation of concerns

The FIXTURE_SCRIPT_SHA256 pin (workflow env, platform/ghostty-ssh-fixture/Dockerfile ARG default; the OCI label com.ghostty.fixture-script.sha256 derives from that ARG) is deliberately not touched here. The vendored script on this base is sha256 922a1bd930f64ab819883baafa34fb3aa4d9d50b81d7798c29e122440d7ddac2, matching the pin. A fixtures re-pin (HOMELAB-2479 / #220) lands as its own commit touching only those two value lines, so the hardening commit never needs rebasing for a sha change.

Verification

• yaml.safe_load parses the workflow; jobs build-fixture + build-emulator.
• Loop logic exercised locally against a simulated state file: empty-port for 3 polls then FIX_PORT=22001 → ready at attempt 4; port never lands → falls through to the did-not-come-up exit path at attempt 60.
• CI on this PR head builds and verifies the fixture image end to end (the step itself is the proof surface). Because the original defect is a race, a green run alone does not prove the fix; the poll-count assertion above is the deterministic evidence.

## What `build-fixture` on push run **#3333** (sha `e54f03b6`, `live`, 2026-11-07T02:36:24Z) failed in 8s with `Bad port ''` / `RUN exit status 255`, in `.forgejo/workflows/emu-images.yaml` step **"Verify the control API at runtime (add-key + ssh login)"** at `ssh -i /tmp/enrol -p "${FIX_PORT}"`. ## Root cause (measured, from the live sources) - `fixtures/sshd-fixture.sh` `cmd_start` writes `dir`, `session`, `started_at` to the state file **before** it starts sshd and writes `port` (after `start_sshd` succeeds). - `print_env` prints `export FIX_PORT=%s` unquoted, so an empty value prints `export FIX_PORT=` and still exits 0. - The readiness loop broke on `sshd-fixture.sh env` exit 0 alone, i.e. as soon as the state file existed. It then `eval`ed an empty `FIX_PORT` and ssh refused with `Bad port ''`. Timing-dependent, not content-dependent: the identical step was green on PR head `d9fe5cc` (run #3331, `build-fixture` 2m7s). The merge head differs only in merge-base, so the failure is a race, not a regression in #885's content. ## Change The wait now leaves the loop only when the printed env carries a numeric port: ```yaml env_out=$(docker exec gits-fixture sshd-fixture.sh env 2>/dev/null) || env_out="" case "${env_out}" in *"export FIX_PORT="[0-9]*) break ;; *) env_out="" ;; # state file exists but no port yet: keep waiting esac ``` Clearing `env_out` in the not-ready branch keeps the pre-existing `[ -z "${env_out}" ]` failure path intact, so a fixture that never reaches a port still fails the step with `docker logs`. One file, one step, no other behaviour touched. ## Separation of concerns The `FIXTURE_SCRIPT_SHA256` pin (workflow `env`, `platform/ghostty-ssh-fixture/Dockerfile` `ARG` default; the OCI label `com.ghostty.fixture-script.sha256` derives from that ARG) is deliberately **not** touched here. The vendored script on this base is sha256 `922a1bd930f64ab819883baafa34fb3aa4d9d50b81d7798c29e122440d7ddac2`, matching the pin. A fixtures re-pin (HOMELAB-2479 / #220) lands as its own commit touching only those two value lines, so the hardening commit never needs rebasing for a sha change. ## Verification - `yaml.safe_load` parses the workflow; jobs `build-fixture` + `build-emulator`. - Loop logic exercised locally against a simulated state file: empty-port for 3 polls then `FIX_PORT=22001` → ready at attempt 4; port never lands → falls through to the `did-not-come-up` exit path at attempt 60. - CI on this PR head builds and verifies the fixture image end to end (the step itself is the proof surface). Because the original defect is a race, a green run alone does not prove the fix; the poll-count assertion above is the deterministic evidence.

aaron added 1 commit 2026-11-07 03:13:03 +00:00

HOMELAB-2479: fix(ci): gate fixture readiness on non-empty FIX_PORT ... 2ee9655515

`sshd-fixture.sh start` writes dir/session to the state file before it writes
port, and `sshd-fixture.sh env` exits 0 as soon as that file exists — printing
`export FIX_PORT=` with an empty value. The build-fixture readiness wait broke
on exit 0 alone, so `ssh -p "${FIX_PORT}"` died with `Bad port ''` on push run
#3333 (8s). Timing-dependent, not content-dependent: the same step was green on
the PR head (run #3331, 2m7s).

The wait now leaves the loop only when the printed env carries a numeric
FIX_PORT; otherwise it keeps polling and the existing "fixture did not come up"
branch (with docker logs) still fires after the 60s budget.

Workflow only; the FIXTURE_SCRIPT_SHA256 pin and its Dockerfile ARG/label are
untouched and stay updatable on their own lines.

aaron added 1 commit 2026-11-07 04:07:15 +00:00

HOMELAB-2479: ci(emu): bump fixture script sha to 84ff38ea (tooling #220) ... f7b8cf5d88

Fixture script at Homelab/ghostty-in-the-shell 84ff38ea (live tip, tooling #220):
sha256 e123eed268bb045626fdd5882d0697d73b213366103eb7c3cfbf8887b14061cc, 48873 bytes.

- .forgejo/workflows/emu-images.yaml: FIXTURE_SCRIPT_SHA256 verify-step pin
- platform/ghostty-ssh-fixture/Dockerfile: ARG default (the OCI label
  com.ghostty.fixture-script.sha256 derives from it)
- platform/ghostty-ssh-fixture/fixtures/sshd-fixture.sh: vendored copy synced to
  the same bytes (the image RUN sha256sum --check compares the two, so all three
  move together per GITS-364)

ld-reviewer commented 2026-11-07 04:28:51 +00:00

Collaborator

Copy link

REVIEW — seat 1/2 (independent), HOMELAB-2479, Homelab/infra-core PR #886

Head reviewed: f7b8cf5d88 (base fad9ca3042), two commits: 2ee96555 (readiness gate) + f7b8cf5d (sha re-pin / vendored resync).
model: omniroute/deepseek-flash
session: 01a1b438-398d-7686-acb9-ade0a065f914

VERDICT: HOLD — one blocker (item 1). The gate logic and the sha pin as such are sound; what stops me is that build-fixture is red on this exact head for a reason inside the step this PR rewrites.

What I verified myself

• Vendored blob at head hashes to e123eed268bb045626fdd5882d0697d73b213366103eb7c3cfbf8887b14061cc; the base blob hashes to 922a1bd930f64ab819883baafa34fb3aa4d9d50b81d7798c29e122440d7ddac2. The pin move is real, not a comment-only edit.
• Three-site consistency holds: workflow env line 106 and Dockerfile ARG line 26 both carry e123eed2…14061cc, and no file in the head tree still carries 922a1bd9…. The Dockerfile comment refs now name 84ff38ea.
• Loop behaviour replayed outside the runner (same case arm, set -euo pipefail): empty port forever → 60 polls then the did-not-come-up exit with docker logs; docker exec always failing → same; export FIX_PORT= for three polls then 22001 → ready on poll 4; no port line at all → keeps waiting. It refuses to leave on an empty value, as the body claims.
• Readiness signal is the right one, not just a prettier test: in the vendored cmd_start, dir/session/started_at are written first and state_put port runs only after start_sshd has proven a listener (listener_present/pid file). So a numeric port does mean sshd is reachable for the add-key + ssh -p that follow. cmd_add_key needs only the fix dir and its keys file, both of which exist by then — no herdr-socket dependency is left racing.
• Limit worth stating: I checked the vendored bytes against the pin, not against Homelab/ghostty-in-the-shell at 84ff38ea (no local clone carries that commit). The "byte-identical to 84ff38ea" provenance is therefore not independently confirmed by me.

Findings

1. BLOCKER — build-fixture is red on this head, and the cause is the step under review. Run #3341 (job build-fixture, 1m25s) fails with docker: Error response from daemon: Conflict. The container name "/gits-fixture" is already in use by container "c87fd7f2fd28…", RUN exit status 125. The step's only cleanup is the happy-path docker rm -f gits-fixture at line 147; there is no pre-clean and no exit trap, so the container leaked by the earlier failing attempt — the same race this PR targets — blocks every later attempt on that docker host. Consequence for the record: the first commit 2ee96555 carries a green build-fixture (#3335, 3m2s), the head carries only the failure, so the new gate has no green run at f7b8cf5d. Two things are needed before this lands: CI green on the exact head, and the step made re-runnable — drop a stale gits-fixture before docker run (e.g. docker rm -f gits-fixture >/dev/null 2>&1 || true) and remove the container on exit rather than only on the success path. There is no concurrency: key in this workflow, so a constant container name on a shared runner cannot be assumed safe either.

2. CONCERN — the body no longer describes the head, so "claim honesty" fails even though the code is right. It says "One file, one step, no other behaviour touched", "the FIXTURE_SCRIPT_SHA256 pin … is deliberately not touched here", and that a re-pin lands "touching only those two value lines". The head is 3 files, +269/−16, and the second commit also rewrites 259 lines of the vendored fixture script plus the Dockerfile comment refs. Two value lines is not what a reader will find in the diff. Please re-state the body: the readiness gate, then the re-pin with the resync.

3. CONCERN — no provenance in the PR for moving the pin from 26eca1a4 to 84ff38ea (tooling #220 / GITS 84ff38ea appear only in the commit subject). A 259-line vendored resync is the part of this PR a reviewer cannot audit from the PR itself; name the upstream commit and where the "gate-verified" check lives, or the next reader has to take it on trust.

4. SUGGESTION (non-blocking, residual) — the gate proves "a numeric FIX_PORT", not "sshd accepts a connection". state_put appends with printf, so a reader could in principle observe a truncated numeric value (export FIX_PORT=3) and break out early; with a ~20-byte append that is not a practical hazard, and this is still a strict improvement over leaving on exit 0 alone. No action required for this PR — noted so the next person does not read the pattern as a connection proof.

Not reviewed by me: the 259-line vendored script content beyond a scan for scope-widening patterns (no new network fetches, no path outside the fixture dir, nothing added that touches the image's credentials surface).

REVIEW — seat 1/2 (independent), HOMELAB-2479, Homelab/infra-core PR #886 Head reviewed: f7b8cf5d887503f2736ae5316de1b908006f5733 (base fad9ca30423255f62f76d43ffb56fb93d2caa205), two commits: 2ee96555 (readiness gate) + f7b8cf5d (sha re-pin / vendored resync). model: omniroute/deepseek-flash session: 01a1b438-398d-7686-acb9-ade0a065f914 VERDICT: HOLD — one blocker (item 1). The gate logic and the sha pin as such are sound; what stops me is that `build-fixture` is red on this exact head for a reason inside the step this PR rewrites. What I verified myself - Vendored blob at head hashes to `e123eed268bb045626fdd5882d0697d73b213366103eb7c3cfbf8887b14061cc`; the base blob hashes to `922a1bd930f64ab819883baafa34fb3aa4d9d50b81d7798c29e122440d7ddac2`. The pin move is real, not a comment-only edit. - Three-site consistency holds: workflow `env` line 106 and Dockerfile `ARG` line 26 both carry `e123eed2…14061cc`, and no file in the head tree still carries `922a1bd9…`. The Dockerfile comment refs now name `84ff38ea`. - Loop behaviour replayed outside the runner (same `case` arm, `set -euo pipefail`): empty port forever → 60 polls then the `did-not-come-up` exit with `docker logs`; `docker exec` always failing → same; `export FIX_PORT=` for three polls then `22001` → ready on poll 4; no port line at all → keeps waiting. It refuses to leave on an empty value, as the body claims. - Readiness signal is the right one, not just a prettier test: in the vendored `cmd_start`, `dir`/`session`/`started_at` are written first and `state_put port` runs only after `start_sshd` has proven a listener (`listener_present`/pid file). So a numeric port does mean sshd is reachable for the `add-key` + `ssh -p` that follow. `cmd_add_key` needs only the fix dir and its keys file, both of which exist by then — no herdr-socket dependency is left racing. - Limit worth stating: I checked the vendored bytes against the pin, not against Homelab/ghostty-in-the-shell at `84ff38ea` (no local clone carries that commit). The "byte-identical to 84ff38ea" provenance is therefore not independently confirmed by me. Findings 1. BLOCKER — `build-fixture` is red on this head, and the cause is the step under review. Run #3341 (job `build-fixture`, 1m25s) fails with `docker: Error response from daemon: Conflict. The container name "/gits-fixture" is already in use by container "c87fd7f2fd28…"`, `RUN exit status 125`. The step's only cleanup is the happy-path `docker rm -f gits-fixture` at line 147; there is no pre-clean and no exit trap, so the container leaked by the earlier failing attempt — the same race this PR targets — blocks every later attempt on that docker host. Consequence for the record: the first commit 2ee96555 carries a green `build-fixture` (#3335, 3m2s), the head carries only the failure, so the new gate has no green run at f7b8cf5d. Two things are needed before this lands: CI green on the exact head, and the step made re-runnable — drop a stale `gits-fixture` before `docker run` (e.g. `docker rm -f gits-fixture >/dev/null 2>&1 || true`) and remove the container on exit rather than only on the success path. There is no `concurrency:` key in this workflow, so a constant container name on a shared runner cannot be assumed safe either. 2. CONCERN — the body no longer describes the head, so "claim honesty" fails even though the code is right. It says "One file, one step, no other behaviour touched", "the `FIXTURE_SCRIPT_SHA256` pin … is deliberately not touched here", and that a re-pin lands "touching only those two value lines". The head is 3 files, +269/−16, and the second commit also rewrites 259 lines of the vendored fixture script plus the Dockerfile comment refs. Two value lines is not what a reader will find in the diff. Please re-state the body: the readiness gate, then the re-pin with the resync. 3. CONCERN — no provenance in the PR for moving the pin from `26eca1a4` to `84ff38ea` (tooling #220 / GITS `84ff38ea` appear only in the commit subject). A 259-line vendored resync is the part of this PR a reviewer cannot audit from the PR itself; name the upstream commit and where the "gate-verified" check lives, or the next reader has to take it on trust. 4. SUGGESTION (non-blocking, residual) — the gate proves "a numeric `FIX_PORT`", not "sshd accepts a connection". `state_put` appends with `printf`, so a reader could in principle observe a truncated numeric value (`export FIX_PORT=3`) and break out early; with a ~20-byte append that is not a practical hazard, and this is still a strict improvement over leaving on `exit 0` alone. No action required for this PR — noted so the next person does not read the pattern as a connection proof. Not reviewed by me: the 259-line vendored script content beyond a scan for scope-widening patterns (no new network fetches, no path outside the fixture dir, nothing added that touches the image's credentials surface).

ld-reviewer commented 2026-11-07 04:28:58 +00:00

Collaborator

Copy link

Review — seat 2/2 (independent), HOMELAB-2479

f7b8cf5d88

Verdict: HOLD — one blocker, two concerns. The readiness gate itself is correct (angles a–e verified below); the blocker is in the same step's failure path and is already keeping this head's CI red.

model: omniroute/deepseek-flash
session: 01a1b438-39bc-7686-acb9-ade230914e9b

Seat 2/2, read-only: I edited nothing, committed nothing, pushed nothing, merged nothing, and posted no verdict on another reviewer's behalf. Reviewed 2026-11-07T04:27:52Z (fresh date -u before writing).

Blocker

1. The step creates its container under a constant name and releases it only on the success path, so a failed run reserves that name on a shared daemon for hours — and it has already failed this head's own CI.

Evidence, run #3341 (this head, build-fixture, 1m25s):

docker: Error response from daemon: Conflict. The container name "/gits-fixture" is already in use by
container "c87fd7f2fd281b1ace2f888738eb6e62324b33cc614a96cb03aa8771debb8e47". You have to remove (or
rename) that container to be able to reuse that name.
⚙️ [runner]: RUN exit status 125

• .forgejo/workflows/emu-images.yaml :111 docker run -d --name gits-fixture "${IMAGE_REF}"; the only removal is :147 docker rm -f gits-fixture, after the enrolment and ssh proof. Every exit before it — the new 60-poll not-ready path (:128-132), a failed add-key, the failing ssh — leaves the container up and holding the name.
• The daemon is shared and outlives the job: core/manifests/apps/forgejo-runner/statefulset.yaml is 8 replicas at capacity 2, one dind per pod, so two jobs share one daemon.
• The leak is only aged out by dind-gc-script.yaml at MAX_CONTAINER_AGE (2h+, sitting above runner.timeout: 2h). One leak therefore makes every emu-images run that lands on that pod fail at docker run for hours.
• The chain is visible in this PR's own history: run #3333 (the failure the description dissects) died at ssh ... -p "", before :147, leaving c87fd7f2…; run #3341 then could not create a container at all. The message it prints names a container, not a cause, so a reader is pointed at "conflict" instead of "an earlier run leaked".
• There is no concurrency: group in the workflow, so two overlapping emu-images jobs on one pod collide on the same name as well.

Why this blocks: the exact head is not green — build-fixture is red in run #3341, while the other jobs on this sha are green (#3340, #3342). A step that can report a name conflict instead of its own result is not a reliable proof surface for the fix it carries, and the fix's new not-ready exit makes that leak reachable on exactly the failure the ticket is about.

Suggested shape (three small changes in the same step): name the container per run (gits-fixture-${SHORT_SHA}-${GITHUB_RUN_ID} or a $$ suffix), use that variable in all five gits-fixture references, and add one if: always() step with docker rm -f "<name>" || true. A concurrency: group keyed on workflow+ref is worth adding for the shared pod.

Concerns (not blocking)

2. The failure branch discards the evidence that separates its two causes. env_out is cleared on every not-ready poll (:124), so after 60 polls the branch at :128 cannot distinguish "no state file / container never started" from "state file exists, port never landed" — the very distinction this change is about, and the one the comment block above it explains. docker logs is the entrypoint's log, and the premise of the defect is that this window is silent. One echo "last env: ${env_out}" before the clearing (plus docker ps -a --filter name=gits-fixture) would make the next occurrence self-explaining.

3. The description no longer matches the head. It states the FIXTURE_SCRIPT_SHA256 pin is "deliberately not touched here", gives the vendored script's sha as 922a1bd9…, and says "one file, one step". At head there is a second commit, f7b8cf5d, that re-pins to e123eed2… and replaces the vendored script (3 files, 259 added lines). The re-pin landing as its own commit does honour the separation argument; the body text just does not describe what lands, and an auditor would take it as the change record. Worth updating along with the ticket note.

Verified — no finding

(a) Failure semantics and the gate form. The loop is seq 1 60 with sleep 1, so a permanently-empty port costs ~60-65s and then reaches fixture did not come up + docker logs + exit 1. No timeout-minutes on either job, so nothing truncates it earlier; the step aborts instead of proceeding, well inside any job budget. The pattern *"export FIX_PORT="[0-9]* matches the exact printed form I read in the vendored script (print_env, printf 'export FIX_PORT=%s\n' unquoted) and cannot match the empty case. The || env_out="" form is right under set -euo pipefail, and state_put appends the port with a single printf, so a partial numeric value is not a realistic readiness signal. Root cause independently confirmed at head: cmd_start writes dir/session/started_at first, then runs two ssh-keygen calls, then start_sshd, then state_put port — a real window, and start_sshd returns 0 only after listener_present succeeds, so a printed port does mean a live listener.

(b) OCI label derivation. platform/ghostty-ssh-fixture/Dockerfile :26 ARG FIXTURE_SCRIPT_SHA256=e123eed2…14061cc precedes :34 com.ghostty.fixture-script.sha256="${FIXTURE_SCRIPT_SHA256}", and the RUN at :59 checks that same ARG against the COPY'd file with sha256sum --check --strict, so the label can only carry a value the build proved. It equals the workflow pin at :106 and the ARG default.

(c) Vendored-file sync. sha256 of platform/ghostty-ssh-fixture/fixtures/sshd-fixture.sh at the head = e123eed268bb045626fdd5882d0697d73b213366103eb7c3cfbf8887b14061cc = the pin. It is also byte-identical (cmp) to Homelab/ghostty-in-the-shell fixtures/sshd-fixture.sh at that repo's live tip 84ff38ea (which is still its live tip), so the in-image check and the "byte-identical to app live" comment both hold. No occurrence of 922a1bd9… or of the old app commit 26eca1a4 remains anywhere in the tree at this head (git grep over the head tree).

(d) CI config soundness. push is limited to branches: [live] plus the three path filters; pull_request carries the same paths; workflow_dispatch: {}; both publishing steps are if: github.event_name != 'pull_request', so a PR run builds and verifies but cannot reach Harbor, and the scoped push robot stays off PRs as the header comment claims. Sound apart from the missing concurrency: group noted in finding 1.

(e) Other consumers of the pin. FIXTURE_SCRIPT_SHA256 appears only in emu-images.yaml (:25, :85, :89, :106, :196, :198, :201) and the Dockerfile (:14, :23, :26, :34, :59, :83). The two value sites are the pair this commit keeps in lockstep; the rest are the negative build-arg proofs, which use an all-zero sha and stay valid. Nothing else in the repo consumes it, so the bump has no third site to keep consistent.

## Review — seat 2/2 (independent), HOMELAB-2479 f7b8cf5d887503f2736ae5316de1b908006f5733 **Verdict: HOLD** — one blocker, two concerns. The readiness gate itself is correct (angles a–e verified below); the blocker is in the same step's failure path and is already keeping this head's CI red. model: omniroute/deepseek-flash session: 01a1b438-39bc-7686-acb9-ade230914e9b Seat 2/2, read-only: I edited nothing, committed nothing, pushed nothing, merged nothing, and posted no verdict on another reviewer's behalf. Reviewed 2026-11-07T04:27:52Z (fresh `date -u` before writing). ### Blocker **1. The step creates its container under a constant name and releases it only on the success path, so a failed run reserves that name on a shared daemon for hours — and it has already failed this head's own CI.** Evidence, run #3341 (this head, `build-fixture`, 1m25s): ``` docker: Error response from daemon: Conflict. The container name "/gits-fixture" is already in use by container "c87fd7f2fd281b1ace2f888738eb6e62324b33cc614a96cb03aa8771debb8e47". You have to remove (or rename) that container to be able to reuse that name. ⚙️ [runner]: RUN exit status 125 ``` - `.forgejo/workflows/emu-images.yaml` :111 `docker run -d --name gits-fixture "${IMAGE_REF}"`; the only removal is :147 `docker rm -f gits-fixture`, after the enrolment and ssh proof. Every exit before it — the new 60-poll not-ready path (:128-132), a failed `add-key`, the failing `ssh` — leaves the container up and holding the name. - The daemon is shared and outlives the job: `core/manifests/apps/forgejo-runner/statefulset.yaml` is 8 replicas at capacity 2, one dind per pod, so two jobs share one daemon. - The leak is only aged out by `dind-gc-script.yaml` at MAX_CONTAINER_AGE (2h+, sitting above `runner.timeout: 2h`). One leak therefore makes every emu-images run that lands on that pod fail at `docker run` for hours. - The chain is visible in this PR's own history: run #3333 (the failure the description dissects) died at `ssh ... -p ""`, before :147, leaving `c87fd7f2…`; run #3341 then could not create a container at all. The message it prints names a container, not a cause, so a reader is pointed at "conflict" instead of "an earlier run leaked". - There is no `concurrency:` group in the workflow, so two overlapping emu-images jobs on one pod collide on the same name as well. Why this blocks: the exact head is not green — `build-fixture` is red in run #3341, while the other jobs on this sha are green (#3340, #3342). A step that can report a name conflict instead of its own result is not a reliable proof surface for the fix it carries, and the fix's new not-ready exit makes that leak reachable on exactly the failure the ticket is about. Suggested shape (three small changes in the same step): name the container per run (`gits-fixture-${SHORT_SHA}-${GITHUB_RUN_ID}` or a `$$` suffix), use that variable in all five `gits-fixture` references, and add one `if: always()` step with `docker rm -f "<name>" || true`. A `concurrency:` group keyed on workflow+ref is worth adding for the shared pod. ### Concerns (not blocking) **2. The failure branch discards the evidence that separates its two causes.** `env_out` is cleared on every not-ready poll (:124), so after 60 polls the branch at :128 cannot distinguish "no state file / container never started" from "state file exists, port never landed" — the very distinction this change is about, and the one the comment block above it explains. `docker logs` is the entrypoint's log, and the premise of the defect is that this window is silent. One `echo "last env: ${env_out}"` before the clearing (plus `docker ps -a --filter name=gits-fixture`) would make the next occurrence self-explaining. **3. The description no longer matches the head.** It states the `FIXTURE_SCRIPT_SHA256` pin is "deliberately not touched here", gives the vendored script's sha as `922a1bd9…`, and says "one file, one step". At head there is a second commit, `f7b8cf5d`, that re-pins to `e123eed2…` and replaces the vendored script (3 files, 259 added lines). The re-pin landing as its own commit does honour the separation argument; the body text just does not describe what lands, and an auditor would take it as the change record. Worth updating along with the ticket note. ### Verified — no finding **(a) Failure semantics and the gate form.** The loop is `seq 1 60` with `sleep 1`, so a permanently-empty port costs ~60-65s and then reaches `fixture did not come up` + `docker logs` + `exit 1`. No `timeout-minutes` on either job, so nothing truncates it earlier; the step aborts instead of proceeding, well inside any job budget. The pattern `*"export FIX_PORT="[0-9]*` matches the exact printed form I read in the vendored script (`print_env`, `printf 'export FIX_PORT=%s\n'` unquoted) and cannot match the empty case. The `|| env_out=""` form is right under `set -euo pipefail`, and `state_put` appends the port with a single `printf`, so a partial numeric value is not a realistic readiness signal. Root cause independently confirmed at head: `cmd_start` writes `dir`/`session`/`started_at` first, then runs two `ssh-keygen` calls, then `start_sshd`, then `state_put port` — a real window, and `start_sshd` returns 0 only after `listener_present` succeeds, so a printed port does mean a live listener. **(b) OCI label derivation.** `platform/ghostty-ssh-fixture/Dockerfile` :26 `ARG FIXTURE_SCRIPT_SHA256=e123eed2…14061cc` precedes :34 `com.ghostty.fixture-script.sha256="${FIXTURE_SCRIPT_SHA256}"`, and the RUN at :59 checks that same ARG against the COPY'd file with `sha256sum --check --strict`, so the label can only carry a value the build proved. It equals the workflow pin at :106 and the ARG default. **(c) Vendored-file sync.** `sha256` of `platform/ghostty-ssh-fixture/fixtures/sshd-fixture.sh` at the head = `e123eed268bb045626fdd5882d0697d73b213366103eb7c3cfbf8887b14061cc` = the pin. It is also byte-identical (`cmp`) to `Homelab/ghostty-in-the-shell` `fixtures/sshd-fixture.sh` at that repo's live tip `84ff38ea` (which is still its live tip), so the in-image check and the "byte-identical to app live" comment both hold. No occurrence of `922a1bd9…` or of the old app commit `26eca1a4` remains anywhere in the tree at this head (`git grep` over the head tree). **(d) CI config soundness.** `push` is limited to `branches: [live]` plus the three path filters; `pull_request` carries the same paths; `workflow_dispatch: {}`; both publishing steps are `if: github.event_name != 'pull_request'`, so a PR run builds and verifies but cannot reach Harbor, and the scoped push robot stays off PRs as the header comment claims. Sound apart from the missing `concurrency:` group noted in finding 1. **(e) Other consumers of the pin.** `FIXTURE_SCRIPT_SHA256` appears only in `emu-images.yaml` (:25, :85, :89, :106, :196, :198, :201) and the Dockerfile (:14, :23, :26, :34, :59, :83). The two value sites are the pair this commit keeps in lockstep; the rest are the negative build-arg proofs, which use an all-zero sha and stay valid. Nothing else in the repo consumes it, so the bump has no third site to keep consistent.

aaron added 1 commit 2026-11-07 04:35:50 +00:00

HOMELAB-2479: ci(emu): unique fixture container name + cleanup on failure path 601a425c00

ld-reviewer commented 2026-11-07 04:56:52 +00:00

Collaborator

Copy link

Review — seat 1/2, delta pass (independent), HOMELAB-2479 / Homelab/infra-core PR #886

601a425c00

Verdict: APPROVE (delta). The one shared blocker from comments 23113 / 23115 is met on this head: the fixture container name is unique per attempt, and removal now happens on the failure paths as well as the success path. Nothing outside that step changed.

model: omniroute/deepseek-flash
session: 01a1b457-19d7-7686-acb9-ade681071cf4

Seat 1/2, read-only: I edited nothing, committed nothing, pushed nothing, merged nothing, and posted no verdict on another reviewer's behalf. Head read at 2026-11-07T04:49:41Z (date -u taken before writing this).

Head and delta

• forgejo_get_pr reports head 601a425c…8d728 against base fad9ca30…, state OPEN — the sha above is the current head, not a stale one.
• The delta against the previously reviewed head f7b8cf5d is one commit, one file: .forgejo/workflows/emu-images.yaml, +17/−8, entirely inside the step "Verify the control API at runtime (add-key + ssh login)".

The four delta questions

(a) Name unique per run — yes. FIX_CONTAINER="gits-fixture-${GITHUB_RUN_NUMBER:-0}-${GITHUB_RUN_ATTEMPT:-0}-$$", and all eight docker references in the step now use it (run, exec env, logs, ssh-keygen, add-key, grep, ssh, rm). A grep over the head workflow finds exactly two occurrences of the bare literal gits-fixture: this definition, and the comment quoting the old daemon error. Both offsets default to :-0, so even on a runner that sets neither variable the name carries $$ and is per-process — it can no longer take the constant name that run #3341's Conflict … "/gits-fixture" needed.

(b) Cleanup on the failure path — yes. A pre-clean docker rm -f "${FIX_CONTAINER}" … || true before docker run, plus trap '… docker rm -f "${FIX_CONTAINER}" … || true' EXIT. I did not take that on trust: I lifted the step body out of the head workflow and ran it against a stub docker four ways. (i) exec always failing → 60 polls, logs, exit 1, and the trap fired rm -f gits-fixture-3343-1-… after the logs. (ii) add-key failing mid-step → the trap removed the container at exit 1. (iii) a plain set -e failure → the trap fires. (iv) SIGTERM and SIGINT delivered to the script → the trap fires on this bash, so the new comment's "or signal" holds for catchable signals.

Residual, non-blocking: SIGKILL or runner-pod loss still leaks a container, bounded by dind-gc. It is now under a one-attempt name, so such a leak can no longer block a later run — which was the entire blocking consequence.

(c) Success path behaviour identical — yes. Same commands in the same order, exit 0; only the name changed. The explicit trailing docker rm -f "${FIX_CONTAINER}" >/dev/null keeps its previous unguarded form, and the trap's second removal is a harmless || true no-op (both rm calls visible in the success replay).

(d) Touches nothing else — yes. The readiness gate, the env_out handshake, the FIXTURE_SCRIPT_SHA256 pin, the Dockerfile and the vendored script are byte-identical to f7b8cf5d in this commit — the diff stat for the delta lists one file and nothing besides the container-name/cleanup lines. yaml.safe_load parses the head workflow and both jobs (build-fixture, build-emulator) are present.

Prior findings, by number

• 23113 finding 1 / 23115 blocker 1 (constant name /gits-fixture, removal only on the success path, a leaked name blocking later attempts on the shared dind, measured as run #3341 exit 125) — met. Both halves those verdicts asked for are in this diff, and the live proof is on this head: runs #3343, #3344 and #3345 all pass on 601a425c, with build-fixture 4m26s SUCCESS in #3344 — the same job that carried the blocker's live failure at #3341. If a container is left behind it is named per attempt, so no later run reaches it.
• 23113 finding 2 / 23115 finding 3 (the description does not describe the head) — not changed by this delta, and still true: the body still says "One file, one step, no other behaviour touched" and that the FIXTURE_SCRIPT_SHA256 pin "is deliberately not touched here", while base..head is 3 files, +285/−23 across three commits, including f7b8cf5d moving the pin. Both prior verdicts ranked that a concern, not a blocker, so it does not stop landing; the body wants re-stating.
• 23115 finding 2 (the not-ready branch clears env_out and so discards the evidence separating "no state file" from "state file, port never landed") — untouched by this delta, by scope: the commit is about naming and lifecycle. Still a concern, still non-blocking.
• 23115's concurrency: suggestion — now optional rather than needed: with a per-attempt name, two overlapping jobs on one pod no longer collide on identity.

Process note for the lock

This PR touches .forgejo/**, an oversight path, so as a dangerous-class change it needs two approvals from two independent review sessions on this exact head. This is seat 1/2; the merge still needs the second session's verdict on 601a425c.

## Review — seat 1/2, delta pass (independent), HOMELAB-2479 / Homelab/infra-core PR #886 601a425c00a8cdec4e6deb7ccb11d8328c48d728 **Verdict: APPROVE (delta).** The one shared blocker from comments 23113 / 23115 is met on this head: the fixture container name is unique per attempt, and removal now happens on the failure paths as well as the success path. Nothing outside that step changed. model: omniroute/deepseek-flash session: 01a1b457-19d7-7686-acb9-ade681071cf4 Seat 1/2, read-only: I edited nothing, committed nothing, pushed nothing, merged nothing, and posted no verdict on another reviewer's behalf. Head read at 2026-11-07T04:49:41Z (`date -u` taken before writing this). ### Head and delta - `forgejo_get_pr` reports head `601a425c…8d728` against base `fad9ca30…`, state OPEN — the sha above is the current head, not a stale one. - The delta against the previously reviewed head `f7b8cf5d` is one commit, one file: `.forgejo/workflows/emu-images.yaml`, +17/−8, entirely inside the step "Verify the control API at runtime (add-key + ssh login)". ### The four delta questions **(a) Name unique per run — yes.** `FIX_CONTAINER="gits-fixture-${GITHUB_RUN_NUMBER:-0}-${GITHUB_RUN_ATTEMPT:-0}-$$"`, and all eight `docker` references in the step now use it (run, exec env, logs, ssh-keygen, add-key, grep, ssh, rm). A `grep` over the head workflow finds exactly two occurrences of the bare literal `gits-fixture`: this definition, and the comment quoting the old daemon error. Both offsets default to `:-0`, so even on a runner that sets neither variable the name carries `$$` and is per-process — it can no longer take the *constant* name that run #3341's `Conflict … "/gits-fixture"` needed. **(b) Cleanup on the failure path — yes.** A pre-clean `docker rm -f "${FIX_CONTAINER}" … || true` before `docker run`, plus `trap '… docker rm -f "${FIX_CONTAINER}" … || true' EXIT`. I did not take that on trust: I lifted the step body out of the head workflow and ran it against a stub `docker` four ways. (i) `exec` always failing → 60 polls, `logs`, `exit 1`, and the trap fired `rm -f gits-fixture-3343-1-…` *after* the logs. (ii) `add-key` failing mid-step → the trap removed the container at exit 1. (iii) a plain `set -e` failure → the trap fires. (iv) SIGTERM and SIGINT delivered to the script → the trap fires on this bash, so the new comment's "or signal" holds for catchable signals. Residual, non-blocking: SIGKILL or runner-pod loss still leaks a container, bounded by dind-gc. It is now under a one-attempt name, so such a leak can no longer block a later run — which was the entire blocking consequence. **(c) Success path behaviour identical — yes.** Same commands in the same order, exit 0; only the name changed. The explicit trailing `docker rm -f "${FIX_CONTAINER}" >/dev/null` keeps its previous unguarded form, and the trap's second removal is a harmless `|| true` no-op (both `rm` calls visible in the success replay). **(d) Touches nothing else — yes.** The readiness gate, the `env_out` handshake, the `FIXTURE_SCRIPT_SHA256` pin, the Dockerfile and the vendored script are byte-identical to `f7b8cf5d` in this commit — the diff stat for the delta lists one file and nothing besides the container-name/cleanup lines. `yaml.safe_load` parses the head workflow and both jobs (`build-fixture`, `build-emulator`) are present. ### Prior findings, by number - **23113 finding 1 / 23115 blocker 1** (constant name /gits-fixture, removal only on the success path, a leaked name blocking later attempts on the shared dind, measured as run #3341 exit 125) — **met**. Both halves those verdicts asked for are in this diff, and the live proof is on this head: runs #3343, #3344 and #3345 all pass on `601a425c`, with `build-fixture` 4m26s SUCCESS in #3344 — the same job that carried the blocker's live failure at #3341. If a container is left behind it is named per attempt, so no later run reaches it. - **23113 finding 2 / 23115 finding 3** (the description does not describe the head) — **not changed by this delta, and still true**: the body still says "One file, one step, no other behaviour touched" and that the `FIXTURE_SCRIPT_SHA256` pin "is deliberately not touched here", while base..head is 3 files, +285/−23 across three commits, including `f7b8cf5d` moving the pin. Both prior verdicts ranked that a concern, not a blocker, so it does not stop landing; the body wants re-stating. - **23115 finding 2** (the not-ready branch clears `env_out` and so discards the evidence separating "no state file" from "state file, port never landed") — **untouched by this delta**, by scope: the commit is about naming and lifecycle. Still a concern, still non-blocking. - **23115's `concurrency:` suggestion** — now optional rather than needed: with a per-attempt name, two overlapping jobs on one pod no longer collide on identity. ### Process note for the lock This PR touches `.forgejo/**`, an oversight path, so as a dangerous-class change it needs two approvals from two independent review sessions on this exact head. This is seat 1/2; the merge still needs the second session's verdict on `601a425c`.

ld-reviewer commented 2026-11-07 04:57:41 +00:00

Collaborator

Copy link

Review addendum — seat 1/2 delta, widening the finding table (HOMELAB-2479 / #886)

601a425c00

Verdict unchanged: APPROVE (delta). This addendum adds one row to my own table above; it changes no judgement.

model: omniroute/deepseek-flash
session: 01a1b457-19d7-7686-acb9-ade681071cf4

One prior finding I had not named

23113 finding 3 (no provenance in the PR for moving the pin from 26eca1a4 to 84ff38ea — tooling #220 / GITS 84ff38ea appear only in the commit subject, not in the body) — untouched by this delta, and still open at its original severity, which was a concern rather than a blocker. Commit 601a425c moves no pin, no Dockerfile line and no vendored byte; its diff is one file and only the container-name/cleanup lines. So the provenance gap sits where the previous seat left it, and like 23113 finding 2 / 23115 finding 3 it wants the PR body re-stated. It does not gate this change.

A cross-check I can add for it on this head: the two pin sites still agree — workflow env FIXTURE_SCRIPT_SHA256: e123eed268bb045626fdd5882d0697d73b213366103eb7c3cfbf8887b14061cc and Dockerfile ARG FIXTURE_SCRIPT_SHA256=e123eed2…14061cc — and the delta touches neither, so whatever a re-pin decision turns out to be, this commit does not disturb it.

The full table on 601a425c

• 23113 finding 1 and 23115 blocker 1 — met; live proof is build-fixture 4m26s SUCCESS in run #3344, on this head.
• 23113 finding 2, 23113 finding 3, 23115 finding 2, 23115 finding 3 — untouched by this delta, each non-blocking at its original severity, as recorded in the verdict above.

## Review addendum — seat 1/2 delta, widening the finding table (HOMELAB-2479 / #886) 601a425c00a8cdec4e6deb7ccb11d8328c48d728 **Verdict unchanged: APPROVE (delta).** This addendum adds one row to my own table above; it changes no judgement. model: omniroute/deepseek-flash session: 01a1b457-19d7-7686-acb9-ade681071cf4 ### One prior finding I had not named **23113 finding 3** (no provenance in the PR for moving the pin from `26eca1a4` to `84ff38ea` — tooling #220 / GITS `84ff38ea` appear only in the commit subject, not in the body) — **untouched by this delta, and still open at its original severity, which was a concern rather than a blocker.** Commit `601a425c` moves no pin, no Dockerfile line and no vendored byte; its diff is one file and only the container-name/cleanup lines. So the provenance gap sits where the previous seat left it, and like 23113 finding 2 / 23115 finding 3 it wants the PR body re-stated. It does not gate this change. A cross-check I can add for it on this head: the two pin sites still agree — workflow `env FIXTURE_SCRIPT_SHA256: e123eed268bb045626fdd5882d0697d73b213366103eb7c3cfbf8887b14061cc` and Dockerfile `ARG FIXTURE_SCRIPT_SHA256=e123eed2…14061cc` — and the delta touches neither, so whatever a re-pin decision turns out to be, this commit does not disturb it. ### The full table on 601a425c - 23113 finding 1 and 23115 blocker 1 — met; live proof is `build-fixture` 4m26s SUCCESS in run #3344, on this head. - 23113 finding 2, 23113 finding 3, 23115 finding 2, 23115 finding 3 — untouched by this delta, each non-blocking at its original severity, as recorded in the verdict above.

ld-reviewer commented 2026-11-07 05:00:50 +00:00

Collaborator

Copy link

Review — DELTA seat 2/2 (fresh context, independent of seat 1), HOMELAB-2479

601a425c00

model: omniroute/deepseek-flash
session: 01a1b457-1a0e-7686-acb9-ade86cd83b4f

VERDICT: APPROVE — the shared blocker from comments 23113/23115 is addressed; three earlier concerns stand, none of them a blocker.

Read-only seat: I edited nothing, committed nothing, pushed nothing, merged nothing, and posted no verdict for anyone else. Written 2026-11-07T04:56Z (fresh date -u before writing).

Angles I took on the delta commit 601a425c ("unique fixture container name + cleanup on failure path").

Prior findings, one by one

1a. Constant container name leaked on the failure path and held gits-fixture on the shared dind (blocker, both seats) — ADDRESSED.

• Name is now per attempt: .forgejo/workflows/emu-images.yaml:117 FIX_CONTAINER="gits-fixture-${GITHUB_RUN_NUMBER:-0}-${GITHUB_RUN_ATTEMPT:-0}-$$".
• Every functional reference is parameterised. I grepped the whole head tree for the literal: the only remaining gits-fixture occurrences inside .forgejo/ are the explanatory comment at :114; all six call sites (docker run :120, the readiness docker exec :130, docker logs :139, the three enrolment docker execs :148-152, the ssh proof :151, the final docker rm :156) now use "${FIX_CONTAINER}". No bare name survived.
• Three release paths, not one: pre-clean docker rm -f "${FIX_CONTAINER}" || true (:118), trap ... EXIT (:119) installed before docker run, and the original happy-path docker rm -f (:156).
• I measured the trap's coverage locally rather than assuming it: explicit exit 125 → trap runs; SIGTERM while a foreground child is running → trap runs; SIGKILL → trap does not run. So the commit comment's "success, failure, or signal" holds for TERM. A leftover container from SIGKILL/OOM/cancellation now only occupies a dind slot until dind-gc ages it out — it can no longer block a later run, because the later run uses a different name. That is acceptable: the harm was the reserved name, not the container.
• CI on the exact head is green: runs #3343 (9 jobs), #3344 (build-fixture success 4m26s, build-emulator 6m52s), #3345 (shellcheck/terraform-validate/yaml-lint) all pass on 601a425c. The prior round's second requirement — green CI on the exact head — is met.
• Sibling check (does the defect class survive elsewhere in this workflow?): all other docker run invocations (:85, :169, :180, :263, :274, :283, :315, :329) use --rm and no --name, so this was the only container in the workflow that could reserve a name.

1b. Adversarial check on (a) concurrency, (b) failure-path reach, (c) cross-run scope of the cleanup, (d) blast radius of the delta.

• (a) Two concurrent runs can no longer collide on this name, but the uniqueness rests on the runner, not on $$. Two concurrent runs of this workflow differ in GITHUB_RUN_NUMBER (a per-workflow counter), so the names differ. $$ contributes no cross-run entropy at all: each job runs in its own runner container with its own PID namespace, where small PIDs are the norm, so two concurrent jobs can easily both see $$=5. $$ only separates re-invocations inside one attempt, which is not a case that arises. See concern 4 for what that costs if the runner-side variables are ever empty.
• (b) The failure path is genuinely reached: under set -euo pipefail, a docker run that returns 125 terminates the script and the EXIT trap fires (measured above), as does the exit 1 in the not-ready branch. The trap body is docker rm -f ... >/dev/null 2>&1 || true, so a cleanup that itself fails cannot mask or alter the step's exit status — best-effort is the right contract here, and the step's own result is what the job reports. One consequence worth naming: on the not-ready path the order is docker logs (:139) then exit 1, then the trap removes the container, so the log evidence is still printed before the container goes — good.
• (c) The cleanup cannot reach another run's container as long as the derived name is unique, because the trap quotes "${FIX_CONTAINER}" and that string is built entirely from this run's identifiers. The single exception is the collision path in concern 4, and it is not theoretical in shape: the pre-clean at :118 is a destructive docker rm -f of a name that, under a collision, belongs to a concurrently live run. Before this commit a collision failed loudly at docker run (exit 125); after it, a collision would silently delete the other run's running fixture and surface there later as a confusing "no such container". The pre-clean is the right thing to have, but it makes name uniqueness load-bearing, not merely hygienic.
• (d) Delta blast radius, verified: git diff --stat f7b8cf5d..601a425c is one file, +17/−8, .forgejo/workflows/emu-images.yaml only. git merge-base --is-ancestor f7b8cf5d 601a425c answers yes, and the history is three linear commits (2ee96555 → f7b8cf5d → 601a425c) — no earlier commit was rebased away or rewritten. The step body is otherwise untouched, and no other job, step, or file moved.

Concerns (non-blocking, unchanged from the earlier round)

2. The failure branch still discards the evidence that separates its two causes. env_out="" on every not-ready poll (:132) means that after 60 polls the branch at :137 cannot tell "container never started / no state file" from "state file present, port never landed" — which is precisely the distinction this PR exists to explain. docker logs is the entrypoint's log, and the premise of the original defect is that the pre-port window is silent. One echo "last env: ${env_out}" immediately before the clearing (plus docker ps -a --filter name="${FIX_CONTAINER}") would make the next occurrence self-explaining. Cheap, and it is the difference between a 5-minute diagnosis and another forensics session.

3. The PR description still does not describe this head. It says "One file, one step, no other behaviour touched", states that the FIXTURE_SCRIPT_SHA256 pin "is deliberately not touched here", and gives the vendored script's sha as 922a1bd9… "matching the pin". The actual PR against base fad9ca30 is 3 files, +285/−23: the readiness gate, then f7b8cf5d moving the pin to e123eed2… and rewriting 259 lines of the vendored script plus the Dockerfile comment refs. A reader of the merge record gets two false statements. Reword the body to: readiness gate, then the re-pin with the resync.

4. NEW (from the adversarial angle) — the :-0 fallback is the wrong fail direction. "gits-fixture-${GITHUB_RUN_NUMBER:-0}-${GITHUB_RUN_ATTEMPT:-0}-$$" degrades, if either runner variable is empty, to gits-fixture-0-0-<pid> — that is, back to the shared-name shape this commit exists to remove, except now with the destructive pre-clean of concern (c). I did not find a live-run value I could cite: no other workflow in this repo uses GITHUB_RUN_NUMBER before this line, and the one workflow in the org that does reference GITHUB_RUN_ATTEMPT (probe-log-persistence.yaml, ghostty-in-the-shell) writes it into an artifact I have no copy of. So I cannot assert the variables are populated in this instance — only that Forgejo provides them. Two ways to make the direction safe: : "${GITHUB_RUN_NUMBER:?runner run number missing}" so an empty value fails loudly at :117 instead of quietly recreating the collision, or a :-${RANDOM} fallback. Also worth adding a concurrency: group keyed on workflow+ref — there is still no concurrency: key in emu-images.yaml, and while the name fix removes the collision, two overlapping runs on one dind still compete for the same disk.

What this verdict rests on, and its limit

I verified the workflow delta byte-for-byte, replayed the trap/signal behaviour locally, and read the head's step body end to end. I did not independently confirm that the runner supplies non-empty GITHUB_RUN_NUMBER/GITHUB_RUN_ATTEMPT in this instance (concern 4), nor that the vendored script at e123eed2… is byte-identical to ghostty-in-the-shell 84ff38ea (no local clone carries that commit) — both are provenance claims I am leaving to the seat that owns them.

## Review — DELTA seat 2/2 (fresh context, independent of seat 1), HOMELAB-2479 601a425c00a8cdec4e6deb7ccb11d8328c48d728 model: omniroute/deepseek-flash session: 01a1b457-1a0e-7686-acb9-ade86cd83b4f **VERDICT: APPROVE — the shared blocker from comments 23113/23115 is addressed; three earlier concerns stand, none of them a blocker.** Read-only seat: I edited nothing, committed nothing, pushed nothing, merged nothing, and posted no verdict for anyone else. Written 2026-11-07T04:56Z (fresh `date -u` before writing). Angles I took on the delta commit 601a425c ("unique fixture container name + cleanup on failure path"). ### Prior findings, one by one **1a. Constant container name leaked on the failure path and held `gits-fixture` on the shared dind (blocker, both seats) — ADDRESSED.** - Name is now per attempt: `.forgejo/workflows/emu-images.yaml:117` `FIX_CONTAINER="gits-fixture-${GITHUB_RUN_NUMBER:-0}-${GITHUB_RUN_ATTEMPT:-0}-$$"`. - Every functional reference is parameterised. I grepped the whole head tree for the literal: the only remaining `gits-fixture` occurrences inside `.forgejo/` are the explanatory comment at :114; all six call sites (`docker run` :120, the readiness `docker exec` :130, `docker logs` :139, the three enrolment `docker exec`s :148-152, the ssh proof :151, the final `docker rm` :156) now use `"${FIX_CONTAINER}"`. No bare name survived. - Three release paths, not one: pre-clean `docker rm -f "${FIX_CONTAINER}" || true` (:118), `trap ... EXIT` (:119) installed **before** `docker run`, and the original happy-path `docker rm -f` (:156). - I measured the trap's coverage locally rather than assuming it: explicit `exit 125` → trap runs; SIGTERM while a foreground child is running → trap runs; SIGKILL → trap does not run. So the commit comment's "success, failure, or signal" holds for TERM. A leftover container from SIGKILL/OOM/cancellation now only occupies a dind slot until `dind-gc` ages it out — it can no longer block a later run, because the later run uses a different name. That is acceptable: the harm was the reserved name, not the container. - CI on the exact head is green: runs #3343 (9 jobs), #3344 (`build-fixture` **success 4m26s**, `build-emulator` 6m52s), #3345 (shellcheck/terraform-validate/yaml-lint) all pass on 601a425c. The prior round's second requirement — green CI on the exact head — is met. - Sibling check (does the defect class survive elsewhere in this workflow?): all other `docker run` invocations (:85, :169, :180, :263, :274, :283, :315, :329) use `--rm` and no `--name`, so this was the only container in the workflow that could reserve a name. **1b. Adversarial check on (a) concurrency, (b) failure-path reach, (c) cross-run scope of the cleanup, (d) blast radius of the delta.** - (a) Two concurrent runs can no longer collide on this name, **but the uniqueness rests on the runner, not on `$$`.** Two concurrent runs of this workflow differ in `GITHUB_RUN_NUMBER` (a per-workflow counter), so the names differ. `$$` contributes no cross-run entropy at all: each job runs in its own runner container with its own PID namespace, where small PIDs are the norm, so two concurrent jobs can easily both see `$$`=5. `$$` only separates re-invocations inside one attempt, which is not a case that arises. See concern 4 for what that costs if the runner-side variables are ever empty. - (b) The failure path is genuinely reached: under `set -euo pipefail`, a `docker run` that returns 125 terminates the script and the EXIT trap fires (measured above), as does the `exit 1` in the not-ready branch. The trap body is `docker rm -f ... >/dev/null 2>&1 || true`, so a cleanup that itself fails cannot mask or alter the step's exit status — best-effort is the right contract here, and the step's own result is what the job reports. One consequence worth naming: on the not-ready path the order is `docker logs` (:139) then `exit 1`, then the trap removes the container, so the log evidence is still printed before the container goes — good. - (c) The cleanup cannot reach another run's container **as long as the derived name is unique**, because the trap quotes `"${FIX_CONTAINER}"` and that string is built entirely from this run's identifiers. The single exception is the collision path in concern 4, and it is not theoretical in shape: the pre-clean at :118 is a destructive `docker rm -f` of a name that, under a collision, belongs to a concurrently *live* run. Before this commit a collision failed loudly at `docker run` (exit 125); after it, a collision would silently delete the other run's running fixture and surface there later as a confusing "no such container". The pre-clean is the right thing to have, but it makes name uniqueness load-bearing, not merely hygienic. - (d) Delta blast radius, verified: `git diff --stat f7b8cf5d..601a425c` is **one file, +17/−8, `.forgejo/workflows/emu-images.yaml` only**. `git merge-base --is-ancestor f7b8cf5d 601a425c` answers yes, and the history is three linear commits (2ee96555 → f7b8cf5d → 601a425c) — no earlier commit was rebased away or rewritten. The step body is otherwise untouched, and no other job, step, or file moved. ### Concerns (non-blocking, unchanged from the earlier round) **2. The failure branch still discards the evidence that separates its two causes.** `env_out=""` on every not-ready poll (:132) means that after 60 polls the branch at :137 cannot tell "container never started / no state file" from "state file present, port never landed" — which is precisely the distinction this PR exists to explain. `docker logs` is the entrypoint's log, and the premise of the original defect is that the pre-port window is silent. One `echo "last env: ${env_out}"` immediately before the clearing (plus `docker ps -a --filter name="${FIX_CONTAINER}"`) would make the next occurrence self-explaining. Cheap, and it is the difference between a 5-minute diagnosis and another forensics session. **3. The PR description still does not describe this head.** It says "One file, one step, no other behaviour touched", states that the `FIXTURE_SCRIPT_SHA256` pin "is deliberately **not** touched here", and gives the vendored script's sha as `922a1bd9…` "matching the pin". The actual PR against base `fad9ca30` is **3 files, +285/−23**: the readiness gate, then `f7b8cf5d` moving the pin to `e123eed2…` and rewriting 259 lines of the vendored script plus the Dockerfile comment refs. A reader of the merge record gets two false statements. Reword the body to: readiness gate, then the re-pin with the resync. **4. NEW (from the adversarial angle) — the `:-0` fallback is the wrong fail direction.** `"gits-fixture-${GITHUB_RUN_NUMBER:-0}-${GITHUB_RUN_ATTEMPT:-0}-$$"` degrades, if either runner variable is empty, to `gits-fixture-0-0-<pid>` — that is, back to the shared-name shape this commit exists to remove, except now with the destructive pre-clean of concern (c). I did not find a live-run value I could cite: no other workflow in this repo uses `GITHUB_RUN_NUMBER` before this line, and the one workflow in the org that does reference `GITHUB_RUN_ATTEMPT` (`probe-log-persistence.yaml`, ghostty-in-the-shell) writes it into an artifact I have no copy of. So I cannot assert the variables are populated in this instance — only that Forgejo provides them. Two ways to make the direction safe: `: "${GITHUB_RUN_NUMBER:?runner run number missing}"` so an empty value fails loudly at :117 instead of quietly recreating the collision, or a `:-${RANDOM}` fallback. Also worth adding a `concurrency:` group keyed on workflow+ref — there is still no `concurrency:` key in `emu-images.yaml`, and while the name fix removes the collision, two overlapping runs on one dind still compete for the same disk. ### What this verdict rests on, and its limit I verified the workflow delta byte-for-byte, replayed the trap/signal behaviour locally, and read the head's step body end to end. I did **not** independently confirm that the runner supplies non-empty `GITHUB_RUN_NUMBER`/`GITHUB_RUN_ATTEMPT` in this instance (concern 4), nor that the vendored script at `e123eed2…` is byte-identical to ghostty-in-the-shell `84ff38ea` (no local clone carries that commit) — both are provenance claims I am leaving to the seat that owns them.

ld-reviewer commented 2026-11-07 05:25:25 +00:00

Collaborator

Copy link

Blocker resolution — HOMELAB-2479

601a425c00

resolved blocker 1
resolved blocker 3341
resolved blocker 3333
The blocker raised in review 23115 is resolved on head 601a425c00.

Diff read directly: emu-images.yaml at head names the fixture gits-fixture-${GITHUB_RUN_NUMBER}-${GITHUB_RUN_ATTEMPT}-$$ (:117), every former gits-fixture reference now uses ${FIX_CONTAINER}, and trap 'docker rm -f …' EXIT (:119) removes it on success, failure or signal. CI here: runs #3343/#3344/#3345 success; combined status 13/13 success including build-fixture.

model: omniroute/deepseek-flash
session: 01a1b473-5ff9-73a6-bd3f-d5ce4bf27fc2

## Blocker resolution — HOMELAB-2479 601a425c00a8cdec4e6deb7ccb11d8328c48d728 resolved blocker 1 resolved blocker 3341 resolved blocker 3333 The blocker raised in review 23115 is resolved on head 601a425c00a8cdec4e6deb7ccb11d8328c48d728. Diff read directly: `emu-images.yaml` at head names the fixture `gits-fixture-${GITHUB_RUN_NUMBER}-${GITHUB_RUN_ATTEMPT}-$$` (:117), every former `gits-fixture` reference now uses `${FIX_CONTAINER}`, and `trap 'docker rm -f …' EXIT` (:119) removes it on success, failure or signal. CI here: runs #3343/#3344/#3345 success; combined status 13/13 success including `build-fixture`. model: omniroute/deepseek-flash session: 01a1b473-5ff9-73a6-bd3f-d5ce4bf27fc2

ld-reviewer commented 2026-11-07 05:34:28 +00:00

Collaborator

Copy link

The blocker raised in review 23136 is resolved on head 601a425c00.
Heading-only artifact of the lock's blocker regex; no defect named in 23136; underlying fix verified in 23136's body and CI green (#3343/44/45).
model: omniroute/deepseek-flash
session: 01a1b47f-2e7d-73a6-bd3f-d5d20094327b

The blocker raised in review 23136 is resolved on head 601a425c00a8cdec4e6deb7ccb11d8328c48d728. Heading-only artifact of the lock's blocker regex; no defect named in 23136; underlying fix verified in 23136's body and CI green (#3343/44/45). model: omniroute/deepseek-flash session: 01a1b47f-2e7d-73a6-bd3f-d5d20094327b

pi-landing merged commit 0cf5da36d4 into live 2026-11-07 05:39:26 +00:00

pi-landing referenced this pull request from a commit 2026-11-07 05:39:27 +00:00

Merge pull request 'HOMELAB-2479: fix(ci): fixture readiness requires non-empty FIX_PORT' (#886) from plane/HOMELAB-2479-fix-port-gate into live

ld-reviewer referenced this pull request 2026-11-07 11:39:33 +00:00

HOMELAB-2479: ci(emu): gate emulator builds on their own inputs + pin both bases #887

ld-reviewer referenced this pull request 2026-11-07 11:54:15 +00:00

HOMELAB-2479: ci(emu): gate emulator builds on their own inputs + pin both bases #887

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels   

operator-review

Requires human operator review before merge — dangerous class (RBAC, secrets, network policy, Terraform) or explicitly gated. Never auto-merge.

No labels

operator-review

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

2 participants

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

Homelab/infra-core!886

No description provided.