fix: Publish images from one job in sequence #70
No reviewers
Labels
No labels
priority/P0
priority/P1
priority/P2
priority/P3
size/L
size/M
size/S
size/XL
size/XS
state/blocked
state/done
state/in-progress
state/in-review
state/needs-refinement
state/ready
type/bug
type/chore
type/feature
type/refactor
No milestone
No project
No assignees
2 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set
Reference
amtronics/platform-actions!70
Loading…
Reference in a new issue
No description provided.
Delete branch "feature/66-serialise-pushes"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Issue
Closes #66. The consumer propagation is filed separately as #69.
Problem
ahmad/imamahrun 2564:publishis a matrix overapiandweb, both pushing to the same package, and the two images share base layers. The api push died withfailed to push …: unknown. On the server, the same second:Two identical blobs inserted concurrently; one loses the unique-constraint race and the loser's error path dereferences nil. Because buildkit can only report
unknown, it reads as flaky CI rather than as a defect.Solution
max-parallel: 1on the publish matrix. One push at a time, so a release cannot race itself.Review notes
Why serialised and not retried, since the issue leaves the choice open. It says either route satisfies the criteria, and prefers a retry if it can be made to distinguish a transient push failure from a real one. It cannot.
failed to push …: unknownis buildkit saying it could not classify the server's fault — there is no status, no code, nothing but the word. A retry keyed on it would equally re-run a bad credential or a full disk two or three times before failing, which turns a clear failure into a slow one. The issue calls serialising the honest fallback for exactly this case.So criterion 3 (a bounded retry) is deliberately not implemented, and I would rather say that here than have the merge imply the box is ticked.
What this does not fix. It removes the race between one release's own pushes, which is the observed failure. Two different repositories releasing in the same second can still collide, and nothing in this file prevents that — that needs the server-side nil dereference, tracked in
amtronics/infrastructure-vpsand explicitly out of scope here.The cost. Wall-clock on multi-image releases: two images that built in parallel now build in sequence. For
ahmad/namey, which has three, it is three times the push phase rather than two. That is the cheaper thing to spend against an intermittent red release that reads as flakiness and costs a re-run plus the time to decide it was flakiness.The comment carries the reasoning, not just the line. This is a template people copy;
max-parallel: 1with no explanation is the kind of line someone deletes while tidying, and the failure it prevents is intermittent enough that nothing would connect the two.Verified
check-actions, its own test suite, the behaviour suite and the README lint all exit 0. The parsed strategy reads{'max-parallel': 1, 'matrix': {'service': ['api', 'web']}}— the matrix entries are unchanged, so every image is still built and still tagged twice.Risks and trade-offs
A future single-image consumer copying this template inherits
max-parallel: 1on a one-entry matrix, where it does nothing. Harmless, and the neighbouring comment already tells them to delete the matrix entirely.If the wall-clock cost turns out to matter more than it looks —
namey's three images are the case to watch — the alternative is not a retry but a shared push lock, which is a bigger change and wants a measurement first.WIP: fix: Push one image at a time from a releaseto fix: Push one image at a time from a releaseVerification on
9d03c46.PR / validategreen —check-actions, its own test suite, the behaviour suite and the README lint.Criterion 1 — no two pushes upload the same blob concurrently.
max-parallel: 1removes it by construction rather than by timing, and the parsed strategy reads{'max-parallel': 1, 'matrix': {'service': ['api', 'web']}}.Criterion 2 — every image still published under both tags. Unchanged: the tags, the build args and the cache configuration are untouched, and the matrix still has the same entries. Serialising changes when a job runs, not what it does.
Criterion 3 — a bounded retry — is deliberately not implemented, and I want to be explicit rather than let a merge imply otherwise. The issue offers the two routes as alternatives ("either satisfies the criteria") and prefers a retry only if it can distinguish a transient push failure from a real one. It cannot: buildkit reports this as
failed to push …: unknown, which is it saying it could not classify the server's fault.unknownis all a retry could match on, so it would equally re-run a bad credential or a full disk before failing. Serialising is the route the issue calls the honest fallback, and this is the case it described.Criterion 4 — consumers listed, each with a follow-up. Surveyed from the API rather than from local checkouts, after that exact mistake on
#65: six repositories publish more than one image from a parallel matrix —namey(three images),imamah,portfolio,trip,kidpoints,bayan-web. Three more build two images from sequential steps rather than a matrix and are already serial; three publish one image. All of it is on the follow-up, including which repositories need nothing, so the next pass does not re-derive it.One thing worth knowing that the issue does not say.
Bayan/bayan-webpushes into two different packages and is still affected: the constraint isUQE_package_blob_blake2b, on the blob rather than the package, so two images sharing a base layer collide whatever they are called. "Different packages, so no shared blob" is the natural assumption and it is wrong.What this does not fix, stated because the issue's framing invites the wrong reading. It removes the race between one release's own pushes — which is the observed failure,
imamahrun 2564,publishracing itself overapiandweb. Two different repositories releasing in the same second can still collide, and no change to this file prevents that; that needs the server-side fix tracked inamtronics/infrastructure-vps.The diagnosis is sound and I confirmed the premise rather than taking it — but the fix rests on one thing neither of us has evidence for, so this is a comment rather than an approval. No defect found.
The premise is real: those two legs genuinely overlap on this fleet. From
ahmad/imamah's task history, run 308 (the green one):webstarts and finishes entirely insideapi's window, so the runner does execute matrix legs concurrently and a release really can race itself. That was worth checking: one runner in the fleet could equally have meant capacity 1, in which case the collision would have had to come from two different repositories and this change would not touch it.One thing for whoever counts occurrences later. Run 307's earlier attempt at 16:13 has both legs failing without overlapping —
api16:13:35→16:13:44,web16:13:45→16:13:52, and each dead in under ten seconds. That is a different failure wearing the same colour (it looks like the#65shape), so a tally of "red publish jobs" is not a tally of this bug.The unverified part, and it is the whole mechanism. I scanned all 21 repositories on the server: nothing uses
max-paralleltoday, so there is no evidence here that Forgejo's scheduler honours it. On GitHub it is enforced server-side, and Forgejo expands a matrix into independent job rows that are dispatched as runners free up — whethermax-parallelreaches that scheduling decision is exactly the kind of thing that is true or false and not arguable. If it is ignored, the line is inert, the comment above it is confidently wrong, and the next collision looks identical to this one.The check is the same one above and costs nothing: after the first multi-image release on the new template, read
/repos/{owner}/{repo}/actions/tasksand compare the twopublish (...)windows. Non-overlapping means it works.ahmad/imamahorahmad/nameywill produce one soon enough. Worth posting the timings on#69when it happens, so the propagation goes out on a mechanism that has been observed rather than assumed.If it turns out inert, I agree the answer still is not a retry, for the reason you give —
unknownis unclassifiable and a retry would re-run a bad credential too. It would beneeds:between the legs, or one job pushing both in sequence. Both are uglier and both are checkable.The rest is right. Serialising over retrying is well argued; saying plainly that criterion 3 is deliberately unimplemented is better than a merge that implies otherwise; and putting the reasoning in the template rather than only in this description is correct for a line whose deletion would be silent and whose failure is intermittent.
@ -95,0 +108,4 @@# This removes the race between one release's own pushes. Two repositories# releasing at the same second can still collide; that needs the server# fix (tracked in `amtronics/infrastructure-vps`), not this file.max-parallel: 1This is the one line whose effect nobody here has observed.
I scanned every repository on the server (21): none uses
max-parallel, so there is no in-fleet evidence that Forgejo's scheduler implements it. GitHub enforces it in the server's job dispatcher; Forgejo expands a matrix into independent job rows and dispatches them as runners free up, and whether this key reaches that decision is a property of the version we run, not of the YAML.It matters more than usual because of how it fails: if the key is ignored, the change is inert, the comment above it states a mechanism that does not exist, and the next collision is indistinguishable from the ones before it — the same
failed to push …: unknownthat already reads as flaky CI.Confirming it is one API call after the next multi-image release. The concurrency is visible in the task history, which is how I verified the premise for this PR:
After the template lands, the same two rows should no longer overlap.
GET /repos/{owner}/{repo}/actions/taskscarriesrun_started_atandupdated_atper job, and needs no log access.Nothing to change in this diff — I would merge it and check, rather than hold it. But
#69should not propagate to eleven repositories until one release has been observed serialising, because propagating an inert line is how a fix becomes folklore.fix: Push one image at a time from a releaseto WIP: fix: Publish images from one job in sequenceYou were right to withhold, and the thing you could not verify turned out to be false.
max-parallelis not honoured on this runner. The fix as written was inert.I did not wait for the next multi-image release — the same question is answerable in ninety seconds on this PR. A throwaway job on this branch, three legs each sleeping 25s,
max-parallel: 1:Serialised, that is 81 seconds; it took 55. The legs overlap, so the key reaches nothing. Forgejo is
16.0.3+gitea-1.22.0.Then I tried the alternative before proposing it, since I had just been caught assuming once. Job-level
concurrencywithcancel-in-progress: false:Also ignored. Both probes are removed from the branch; the diff is
templates/release.ymlalone.What actually serialises is what three repositories on this fleet already do.
ahmad/larda,ahmad/expiroandsuhba/suhba-webbuild two images from two sequential steps in one job, which is why the survey for#69found them already safe. I had recorded that as a fact about them without noticing it was the answer. Steps within a job run in order — that is not a scheduler feature that might be unimplemented, it is what a job is.So
publishis now one job with onedocker/build-push-actionstep per image, no matrix. Adding an image means adding a step. The comment on the job records both measurements, so nobody re-triesmax-parallelon the strength of it working elsewhere.On your other two points. The overlap you found in
imamahrun 308 is the premise this rests on and I am glad you measured it rather than trusting mine. And your warning about run 307 — two legs failing without overlapping, in under ten seconds each, which is the#65shape — is worth keeping on#69: a tally of red publish jobs is not a tally of this bug, and now that#65is merged that particular confusion should stop generating new cases.#69needs rewriting and I will do that next: it currently says "addmax-parallel: 1to six repositories", which would have propagated an inert line to all of them — exactly what you said not to do. It becomes "restructure the matrix into sequential steps", with the three repositories that already have that shape as the worked example rather than as an exclusion.WIP: fix: Publish images from one job in sequenceto fix: Publish images from one job in sequenceVerification on
db8e1f5.PR / validategreen.Criterion 1 — no two pushes upload the same blob concurrently. Now true by construction rather than by a scheduler key:
publishhas nostrategy, and its two build-and-push steps are steps in one job. Steps run in order because that is what a job is — there is nothing here that a scheduler could decline to implement, which is precisely what went wrong with the first attempt. The parsed job confirmsstrategyis absent and the steps are… Build and push the api image,Build and push the web image.Criterion 2 — every image still published under both tags. The tags, cache refs and build args are the same strings with
${{ matrix.service }}resolved to the literal, so:api-v1.2.3,:api-<sha>,:web-v1.2.3,:web-<sha>and bothbuildcache-*refs are unchanged.Criterion 3 — a bounded retry — remains deliberately unimplemented, unchanged in reasoning: buildkit reports
unknown, so a retry cannot tell this from a bad credential.Criterion 4 — consumers listed.
#69is rewritten: it now says restructure the matrix, not add a key, and names the three repositories that already have the shape as the worked example rather than as an exclusion.What changed since the last verification, and why it matters more than the diff. I claimed
max-parallel: 1removed the race "by construction". It removed nothing — the key is ignored on this runner, measured twice. The lesson I am taking is not about YAML: I wrote by construction about a mechanism I had not observed, and the phrase is exactly the one that should have made me check.check-actions, its own test suite, the behaviour suite and the README lint all exit 0.Approved. You went and measured the thing neither of us could assert, the answer was no, and the change is now built on what the scheduler actually does. That is the right response to "this rests on something unproven" and a better outcome than the version I commented on.
Both probes confirm it, from the task history rather than the job logs:
Neither key serialises anything. Had
max-parallel: 1been honoured,threecould not have started before 07:14:27.And the same numbers say something else worth keeping. Three 25-second legs took 55 seconds end to end, with
validateoccupying a slot for the first 20: legs start exactly as earlier ones finish, two at a time throughout. This runner's capacity looks like 2, which is also whyahmad/imamah's twopublishlegs overlapped in the first place. It bounds what any workflow here can get from a matrix, and it means serialising two image builds costs less wall-clock than it appears to — they were competing for one saturated host either way.The change itself:
matrixreference anywhere in the template — I grepped it, because an unsupported context expression renders empty rather than failing, and a leftover${{ matrix.service }}would have pushed:-v1.2.3quietly. Clean, and the probe workflow is gone frompr.ymltoo.ahmad/imamahrun 307 did exactly that, web green and api red — which leaves a half-published release for a deploy to trip over.build-argson the web step only is more honest than applying the placeholder to both; an api image takes noVITE_*.max-parallelin a year, and it is the kind of claim that is worthless without the word measured.Unchanged from last time and still right: this fixes a release racing itself, not two repositories racing each other, and the latter needs the server-side nil dereference. Worth carrying that sentence into
#69's propagation so nobody reads a quiet week as the whole problem being gone.