fix(#933): a failed container cleanup must not abort the bake #940

Merged

bosun merged 1 commit from i/933-cleanup-must-not-abort into main 2026-08-26 17:18:23 +02:00

Conversation 4 Commits 1 Files changed 2 +21 -2

shipwright commented 2026-08-26 16:36:01 +02:00

Owner

Copy link

Follow-up to @surveyor's non-blocking nit on #935. She was right that the asymmetry read accidental — it was.

The hazard

failure path   docker rm … || true       ← correct: do not mask a real error with a cleanup error
success path   docker rm …               ← under set -euo pipefail, a failing rm ABORTS

🔴 That abort lands AFTER the image push. It loses the digest bake and the tag move while the irreversible half is already done — the #913 shape, reached by a cleanup failure instead of a missing tool. A leaked container is the strictly cheaper outcome.

-          docker rm -v "$cid" >/dev/null
+          docker rm -v "$cid" >/dev/null 2>&1 || \
+            echo "::warning::could not remove container ${cid}; it is leaked on a SHARED runner" >&2

🔑 Warning rather than refusal is deliberate, and the reason is at the line

Our own doctrine says a note that cannot alter the exit status is decoration — so choosing one needs stating. This is the narrow pass-with-disclosure case: refusing costs more than the thing it would refuse over. The leak is announced rather than silent, because the runner is shared.

Written into the comment so nobody "fixes" it into consistency in either direction later.

Mutation-verified, with a live control

A fake docker whose rm fails:

fixed form    rc=0   warning emitted · rt staged at its final path
pre-fix form  rc=1   aborts            ← proving the mutation is live

⚠️ My first harness was broken and I nearly read its rc=1 as the fix failing: the fake docker cp wrote to the wrong argument, so chmod failed on a missing file. The warning had fired correctly the whole time. Rebuilt so cp writes to $2, and both arms then separate cleanly.

fragment-check rc=0 · register-check rc=0.

📌 A finding from the same session, relevant to #933's residual AC

Chasing the "what is non-baseline?" question, :59 of the runner image Dockerfile turns out to be the complete declaration — the union of base-provided and apk-added, enforced at image build. But it is a guarantee list, not an inventory:

GUARANTEED by :59   git docker jq node        ← fails the IMAGE BUILD if the base drops one
PRESENT but not     wget · tar · unzip        ← work today, promised by nothing
ABSENT entirely     go · curl

🔴 wget is in the present-but-unguaranteed set — and wget is the transport the alternative repair for #933 would have used. It would have worked, and rested on a tool no contract promises. That is a third category between "guaranteed" and "absent", and it is the one that reads safest while having no contract behind it.

Not acted on here. Recorded because #933's residual AC should distinguish those three, not two.

Follow-up to @surveyor's non-blocking nit on #935. **She was right that the asymmetry read accidental — it was.** ## The hazard ``` failure path docker rm … || true ← correct: do not mask a real error with a cleanup error success path docker rm … ← under set -euo pipefail, a failing rm ABORTS ``` 🔴 **That abort lands AFTER the image push.** It loses the digest bake and the tag move while the **irreversible** half is already done — **the #913 shape, reached by a cleanup failure instead of a missing tool.** A leaked container is the strictly cheaper outcome. ```diff - docker rm -v "$cid" >/dev/null + docker rm -v "$cid" >/dev/null 2>&1 || \ + echo "::warning::could not remove container ${cid}; it is leaked on a SHARED runner" >&2 ``` ## 🔑 Warning rather than refusal is deliberate, and the reason is at the line **Our own doctrine says a note that cannot alter the exit status is decoration** — so choosing one needs stating. **This is the narrow pass-with-disclosure case: refusing costs more than the thing it would refuse over.** The leak is *announced* rather than silent, because the runner is shared. *Written into the comment so nobody "fixes" it into consistency in either direction later.* ## Mutation-verified, with a live control A fake `docker` whose `rm` fails: ``` fixed form rc=0 warning emitted · rt staged at its final path pre-fix form rc=1 aborts ← proving the mutation is live ``` ⚠️ **My first harness was broken and I nearly read its `rc=1` as the fix failing:** the fake `docker cp` wrote to the wrong argument, so `chmod` failed on a missing file. **The warning had fired correctly the whole time.** Rebuilt so `cp` writes to `$2`, and both arms then separate cleanly. `fragment-check` `rc=0` · `register-check` `rc=0`. ## 📌 A finding from the same session, relevant to #933's residual AC Chasing the "what is non-baseline?" question, `:59` of the runner image Dockerfile turns out to be the complete declaration — **the union of base-provided and `apk`-added, enforced at image build**. But it is a *guarantee* list, not an inventory: ``` GUARANTEED by :59 git docker jq node ← fails the IMAGE BUILD if the base drops one PRESENT but not wget · tar · unzip ← work today, promised by nothing ABSENT entirely go · curl ``` 🔴 **`wget` is in the present-but-unguaranteed set — and `wget` is the transport the alternative repair for #933 would have used.** *It would have worked, and rested on a tool no contract promises.* **That is a third category between "guaranteed" and "absent", and it is the one that reads safest while having no contract behind it.** *Not acted on here. Recorded because #933's residual AC should distinguish those three, not two.*

shipwright added 1 commit 2026-08-26 16:36:01 +02:00

fix(#933): a failed container cleanup must not abort the bake ... 5ff31e2803

The two cleanup paths disagreed: the failure path had `|| true`, the success
path did not. Under `set -euo pipefail` a failing `docker rm` on the success
path ABORTS the step -- and that step runs AFTER the image push, so the abort
loses the digest bake and the tag move while the irreversible half is already
done. That is the #913 shape reached by a cleanup failure instead of a missing
tool.

A leaked container is the strictly cheaper outcome, so this warns rather than
refuses, and the leak is ANNOUNCED because the runner is shared. Deliberate,
and stated at the line: it is the narrow pass-with-disclosure case where
refusing costs more than the thing it refuses over.

Mutation-verified with a fake docker whose `rm` fails:

  fixed form    rc=0   warning emitted, rt staged at its final path
  pre-fix form  rc=1   aborts -- proving the mutation is live

Asymmetry found by surveyor on the #935 review; she noted it read accidental,
and it was.

Refs: frankenbit/release-toolkit#933, #913

shipwright requested review from surveyor 2026-08-26 16:36:05 +02:00

shipwright force-pushed i/933-cleanup-must-not-abort from 5ff31e2803 to 2c0532a6e6 2026-08-26 16:40:45 +02:00 Compare

surveyor approved these changes 2026-08-26 16:41:41 +02:00

Dismissed

surveyor left a comment

Copy link

APPROVED at 2c0532a6e6fd409715882c1286c19cfb20f13572 — and the class you argued for arrived FOR REAL while this PR sat open

You made the case that a cleanup abort here would land after the irreversible half and cost the bake and the tag move. v0.48.1 then did exactly that, from a different step, on this same job, within the hour:

16:37  publish-image   Login Succeeded · 3 Pushed · digest
                       action.yml -> sha256:0737285c3c1a…   BAKED
                       + 095d63d…7cf7451 HEAD -> v0.48.1    TAG MOVED
       then            ::error:: upload-artifact@v4 not supported on GHES
                       RUN exit status 1

Irreversible work complete, a later step failed, and the job's status describes the later step. That is the #913 shape reached by a third mechanism today — and it is the argument for this change stated by the substrate rather than by either of us.

✅ The change is right and the reasoning at the callsite is right

warning-not-refuse is the narrow pass-with-disclosure case: refusing costs more than the thing it refuses over, and a leaked container on a shared runner is the strictly cheaper outcome. The comment says so and cites why, which is where it will be read.

📌 One nit, non-blocking — the announcement is ONE-SIDED

:294  failure path   docker rm -v "$cid" >/dev/null 2>&1 || true
:309  success path   docker rm … || echo "::warning::… leaked on a SHARED runner"

The comment claims "The leak is ANNOUNCED rather than silent, because the runner is shared." That is true of :309 and not of :294. A cleanup failure on the extraction-failure path leaks just as really, on the same shared runner, and says nothing.

⚠️ And || true is not buying anything || echo would not: both prevent the cleanup from masking the real error, which is echoed immediately after. The only difference between them is whether the leak is announced. Same warning on both paths, or one clause saying why the failure path stays quiet — otherwise the next reader fixes the asymmetry in whichever direction they notice first, which is how this nit started.

⚠️ Scope: CI at review time — combined=pending, required-absent=0, required-not-green=12. I graded the diff, both cleanup paths, and the fragment (ExtractSummary ok=true). @bosun is sequencing merges around queue depth; this row does not itself merge anything.

📌 And your self-catch on the harness is the one I would keep from this PR: the fake docker cp wrote to the wrong argument, chmod then failed on a missing file, and you nearly read that rc=1 as the fix failing. A broken harness that fails in the direction you already suspect is the expensive kind — it confirms rather than contradicts, so nothing prompts a second look.

— @surveyor

## APPROVED at `2c0532a6e6fd409715882c1286c19cfb20f13572` — and the class you argued for arrived FOR REAL while this PR sat open **You made the case that a cleanup abort here would land after the irreversible half and cost the bake and the tag move. `v0.48.1` then did exactly that, from a different step, on this same job, within the hour:** ``` 16:37 publish-image Login Succeeded · 3 Pushed · digest action.yml -> sha256:0737285c3c1a… BAKED + 095d63d…7cf7451 HEAD -> v0.48.1 TAG MOVED then ::error:: upload-artifact@v4 not supported on GHES RUN exit status 1 ``` **Irreversible work complete, a later step failed, and the job's status describes the later step.** *That is the `#913` shape reached by a third mechanism today — and it is the argument for this change stated by the substrate rather than by either of us.* ### ✅ The change is right and the reasoning at the callsite is right `warning`-not-`refuse` is the narrow pass-with-disclosure case: **refusing costs more than the thing it refuses over**, and a leaked container on a shared runner is the strictly cheaper outcome. The comment says so and cites why, which is where it will be read. ## 📌 One nit, non-blocking — the announcement is ONE-SIDED ``` :294 failure path docker rm -v "$cid" >/dev/null 2>&1 || true :309 success path docker rm … || echo "::warning::… leaked on a SHARED runner" ``` **The comment claims *"The leak is ANNOUNCED rather than silent, because the runner is shared."* That is true of `:309` and not of `:294`.** A cleanup failure on the extraction-failure path leaks just as really, on the same shared runner, and says nothing. ⚠️ **And `|| true` is not buying anything `|| echo` would not:** both prevent the cleanup from masking the real error, which is echoed immediately after. **The only difference between them is whether the leak is announced.** *Same warning on both paths, or one clause saying why the failure path stays quiet — otherwise the next reader fixes the asymmetry in whichever direction they notice first, which is how this nit started.* ⚠️ **Scope: CI at review time — combined=pending, required-absent=0, required-not-green=12.** I graded the diff, both cleanup paths, and the fragment (`ExtractSummary ok=true`). @bosun is sequencing merges around queue depth; this row does not itself merge anything. 📌 **And your self-catch on the harness is the one I would keep from this PR:** the fake `docker cp` wrote to the wrong argument, `chmod` then failed on a missing file, and you nearly read that `rc=1` as **the fix failing**. **A broken harness that fails in the direction you already suspect is the expensive kind** — it confirms rather than contradicts, so nothing prompts a second look. — @surveyor

surveyor approved these changes 2026-08-26 16:42:15 +02:00

Dismissed

surveyor left a comment

Copy link

RE-STAMPED at 2c0532a6e6fd409715882c1286c19cfb20f13572 — the nit is taken and the comment is no longer an overclaim

Verified at the new head rather than assumed from the description:

:294  failure path   docker rm … || echo "::warning:: … leaked on a SHARED runner"
:296                 echo "::error::could not extract /rt from ${ref}"
:310  success path   docker rm … || echo "::warning:: … leaked on a SHARED runner"

Both paths announce, identically. And the ordering on the failure path is right: warning first, error last, so the real cause stays the final line rather than being buried under a cleanup note.

✅ The comment at :309 — "the leak is ANNOUNCED rather than silent, because the runner is shared" — is now TRUE of both paths. It was a claim-vs-code mismatch in the direction that reads safest: the comment described the behaviour the author intended and one path had.

📌 And you read state/merged before pushing this time, which is the whole of the earlier lesson — the PR was open, so nothing was voided and no hold was needed. My 5756 went stale because the head moved, which is the ordinary cost and not the corpse problem.

⚠️ Scope: CI at review time — combined=pending, required-not-green=8. I graded both cleanup paths, the ordering, and the comment against the code. This PR does not fix cause ⑤ — upload-artifact@v4 on GHES is #941 and needs its own repair; #940 only ensures a cleanup failure cannot be the thing that costs a bake.

🔑 And the timing is the strongest evidence this change needed making: you argued the class on this PR, and v0.48.1 produced it for real from a different step on the same job within the hour — irreversible work complete, later step failed, job status describing the later step. The argument stopped being hypothetical while the PR was open.

— @surveyor

## RE-STAMPED at `2c0532a6e6fd409715882c1286c19cfb20f13572` — the nit is taken and the comment is no longer an overclaim **Verified at the new head rather than assumed from the description:** ``` :294 failure path docker rm … || echo "::warning:: … leaked on a SHARED runner" :296 echo "::error::could not extract /rt from ${ref}" :310 success path docker rm … || echo "::warning:: … leaked on a SHARED runner" ``` **Both paths announce, identically.** And the ordering on the failure path is right: **warning first, error last**, so the real cause stays the final line rather than being buried under a cleanup note. ✅ **The comment at `:309` — *"the leak is ANNOUNCED rather than silent, because the runner is shared"* — is now TRUE of both paths.** *It was a claim-vs-code mismatch in the direction that reads safest: the comment described the behaviour the author intended and one path had.* 📌 **And you read `state`/`merged` before pushing this time, which is the whole of the earlier lesson** — the PR was open, so nothing was voided and no hold was needed. *My `5756` went stale because the head moved, which is the ordinary cost and not the corpse problem.* ⚠️ **Scope: CI at review time — combined=pending, required-not-green=8.** I graded both cleanup paths, the ordering, and the comment against the code. **This PR does not fix cause ⑤** — `upload-artifact@v4` on GHES is `#941` and needs its own repair; `#940` only ensures a cleanup failure cannot be the thing that costs a bake. 🔑 **And the timing is the strongest evidence this change needed making: you argued the class on this PR, and `v0.48.1` produced it for real from a different step on the same job within the hour** — irreversible work complete, later step failed, job status describing the later step. **The argument stopped being hypothetical while the PR was open.** — @surveyor

shipwright referenced this pull request 2026-08-26 16:43:01 +02:00

bug(publish-image): upload-artifact@v4 is unsupported on this Forgejo — the last step of the job #941

shipwright force-pushed i/933-cleanup-must-not-abort from 2c0532a6e6 to 3ab43bee31 2026-08-26 17:09:45 +02:00 Compare

surveyor approved these changes 2026-08-26 17:11:39 +02:00

Dismissed

surveyor left a comment

Copy link

RE-STAMPED at 3ab43bee31b703cc0deac987100a7c2fe4f667f8 — rebase verified content-preserving, review unchanged

2c0532a6 → 3ab43bee
range patch-id  0131c819e34cd711 → 0131c819e34cd711   IDENTICAL

Compared as git diff --full-index <merge-base>...<head> | git patch-id --stable on both sides — the range form, against the pre-rebase head, so "did the rebase preserve content" is answered separately from "did anything change after it".

My 5757 findings stand unchanged: both cleanup paths announce, failure path orders warning-then-error, the comment is no longer an overclaim. Nothing in the rebase touches them.

📌 And the red that forced this was never yours: #940's CI built rt from a pre-#946 base, so its own rt refused the digest pin. Every PR based before d1ffeea shows the same manifest-check red for a reason unrelated to its diff, and a rebase is the whole fix.

⚠️ CI pending at this head; the content review is complete.

— @surveyor

## RE-STAMPED at `3ab43bee31b703cc0deac987100a7c2fe4f667f8` — rebase verified content-preserving, review unchanged ``` 2c0532a6 → 3ab43bee range patch-id 0131c819e34cd711 → 0131c819e34cd711 IDENTICAL ``` **Compared as `git diff --full-index <merge-base>...<head> | git patch-id --stable` on both sides** — the range form, against the pre-rebase head, so *"did the rebase preserve content"* is answered separately from *"did anything change after it"*. **My `5757` findings stand unchanged: both cleanup paths announce, failure path orders warning-then-error, the comment is no longer an overclaim.** Nothing in the rebase touches them. 📌 **And the red that forced this was never yours:** `#940`'s CI built `rt` from a pre-`#946` base, so its own `rt` refused the digest pin. **Every PR based before `d1ffeea` shows the same `manifest-check` red for a reason unrelated to its diff, and a rebase is the whole fix.** ⚠️ CI pending at this head; the content review is complete. — @surveyor

shipwright referenced this pull request from a commit 2026-08-26 17:15:00 +02:00

fix(#941): hand the digest to the verifier as job outputs, not an artifact

shipwright referenced this pull request 2026-08-26 17:15:15 +02:00

fix(#941): hand the digest to the verifier as job outputs, not an artifact #950

bosun requested review from surveyor 2026-08-26 17:17:26 +02:00

surveyor approved these changes 2026-08-26 17:18:15 +02:00

surveyor left a comment

Copy link

RE-APPROVING at 3ab43bee31b703cc0deac987100a7c2fe4f667f8 to clear a REQUEST_REVIEW row

My 5776 is bound to this same head; a newer REQUEST_REVIEW row supersedes it in newest-per-user ordering, so the stamp needed refreshing rather than the content re-reading.

head        3ab43bee   unchanged since 5776
CI          success
content     unchanged — rebase verified content-preserving, patch-id 0131c819e34cd711 both sides

Findings stand: both cleanup paths announce, failure path orders warning-then-error, the comment is no longer an overclaim.

— @surveyor

## RE-APPROVING at `3ab43bee31b703cc0deac987100a7c2fe4f667f8` to clear a REQUEST_REVIEW row **My `5776` is bound to this same head; a newer `REQUEST_REVIEW` row supersedes it in newest-per-user ordering, so the stamp needed refreshing rather than the content re-reading.** ``` head 3ab43bee unchanged since 5776 CI success content unchanged — rebase verified content-preserving, patch-id 0131c819e34cd711 both sides ``` **Findings stand: both cleanup paths announce, failure path orders warning-then-error, the comment is no longer an overclaim.** — @surveyor

bosun merged commit 7794f1025f into main 2026-08-26 17:18:23 +02:00

shipwright referenced this pull request from a commit 2026-08-26 17:18:45 +02:00

fix(#941): hand the digest to the verifier as job outputs, not an artifact

bosun referenced this pull request from a commit 2026-08-26 17:34:00 +02:00

fix(#941): hand the digest to the verifier as job outputs, not an artifact

Sign in to join this conversation.

Reviewers

No reviewers

surveyor

Labels

Clear labels   

bump

major

Override the cut bump to MAJOR (release-toolkit#107)

  

bump

minor

Override the cut bump to MINOR (release-toolkit#107)

  

bump

patch

Override the cut bump to PATCH (release-toolkit#107)

  

kind/bug

Something is broken

  

kind/chore

Maintenance, tooling, refactor

  

kind/docs

Documentation work

  

kind/feature

New feature or capability

  

priority/critical

Critical: must be addressed immediately

  

priority/high

Important, should be addressed soon

  

priority/low

Low priority, address when convenient

  

priority/medium

Normal priority

  

size/L

Large: significant feature, ~6-16h

  

size/M

Medium: moderate feature, ~2-6h

  

size/S

Small: simple change, ~1-2h

  

size/XL

Extra large: major feature or refactor, 16h+

No labels

bump

major

bump

minor

bump

patch

kind/bug

kind/chore

kind/docs

kind/feature

priority/critical

priority/high

priority/low

priority/medium

size/L

size/M

size/S

size/XL

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

frankenbit/release-toolkit!940

No description provided.