fix: Retry the release POST instead of stranding the tag #44
No reviewers
Labels
No labels
priority/P0
priority/P1
priority/P2
priority/P3
size/L
size/M
size/S
size/XL
size/XS
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!44
Loading…
Reference in a new issue
No description provided.
Delete branch "feature/40-retry-release-post"
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 #40
Problem
release/action.ymlpushes the tag and thenPOSTs the release that references it. When Forgejo has not finished indexing that tag the POST returns 500,curl -fsSexits 22, and the job dies after the tag exists — leaving a version with a release missing,publishanddeployskipped, and a re-dispatch that derives nothing because the tag is already there. The images can never be built for that version.Observed on
ahmad/imamahv0.5.0. Replaying the identical call by hand minutes later returned 201.Solution
The POST retries with exponential backoff (2s, 4s, 8s, 16s; five attempts) on a server fault or a lost connection, and reports the status and the response body when it gives up.
curl -fis dropped deliberately — it exits 22 and discards the body, which is why the original incident needed a dig through the job log to discover a plain 500.Review notes
Two judgement calls the issue does not name, both forced by adding a retry rather than optional:
409is treated as published, not as a failure. A retry only exists because a request can be lost, and the case where the response is lost rather than the request is exactly when the second attempt finds the release already created. Failing there would make the retry able to undo itself — turning a survivable blip into the stranded tag this issue is about.5xxand000retry.401,403and422fail immediately with the body. Retrying a bad token five times over thirty seconds adds delay and tells nobody anything.Verified by extracting the loop and driving a stub
curlthrough status sequences, withsleepstubbed to record the backoff rather than spend it:That exercise found a real bug in my first version, which is the part worth reading. I had written
curl … || echo 000, not noticing thatcurl -w '%{http_code}'already prints000when it never got a status. The two concatenated to000000, which matched neither5*nor000, fell through to the fail-fast branch, and meant a dropped connection — the case most deserving a retry — would never have been retried. It is now|| true, which only stopsset -eacting on curl's non-zero exit, with a comment saying why anechothere is wrong.The whole point of this change is surviving a transient failure, so shipping it unable to survive the most transient failure of all would have been quietly useless. It passed every other case before I ran that one.
Risks and trade-offs
A genuinely unreachable Forgejo now costs 30 seconds before the job fails rather than failing at once. That is the trade the issue asks for.
This does not address the ordering — the tag is still pushed before the release is known to be creatable, so any failure after that point still strands a version. A retry covers a transient 500; it does nothing for a runner killed mid-step. #40's notes make that case and it is deliberately left out of scope here, but it is the larger fix and this should not be read as closing it.
validategreen onb7d8fda. Verification against each acceptance criterion.The retry loop was extracted from the action and driven through status sequences with a stub
curl, andsleepstubbed to record the backoff rather than spend it.publishgates onneeds.version.outputs.released, which is written before this step, so an exit 0 here is exactly what lets it run — unchanged from before.Beyond the criteria, because adding a retry forces the questions:
The exercise found a real bug in my first version. I wrote
curl … || echo 000without noticing thatcurl -w '%{http_code}'already prints000when it never received a status. The two concatenated to000000, matched neither5*nor000, and fell through to fail-fast — so a dropped connection, the most transient failure there is, would never have been retried. Now|| true, with a comment saying why anechothere is wrong. Every other case passed before I ran that one, which is the argument for running the awkward ones.Worth recording against this PR: it would not have prevented today's third stranded version.
imamahv0.5.2 was stranded an hour ago because the runner reported a job that had succeeded — its log endsPublished release v0.5.2.thenJob succeeded— asfailure, blockingpublishanddeploy. Nothing failed for a retry to catch. This change covers a transient 500 and a lost connection; it does not cover a phantom failure, a killed runner, or anything else after the tag is pushed. Only reordering the tag behind the artefacts covers the class, which #40's notes argue and this PR deliberately does not attempt.WIP: fix: Retry the release POST instead of stranding the tagto fix: Retry the release POST instead of stranding the tagApproving. All four criteria hold, and I reproduced your table row for row against a stub
curlwithsleeprecording the backoff rather than spending it:Five attempts, four sleeps, doubling from 2s — the arithmetic matches what the loop claims.
The bug you found is real, and I confirmed the mechanism against a live
curlrather than taking it on trust:So the first version would have failed immediately on a dropped connection — the single most transient failure there is, and the one a retry exists for. Finding that by running the case rather than reading the branch is the difference between a retry that works and one that reads as though it does; it would have passed every other row.
Both judgement calls are right, and neither is optional once you add a retry. Treating
409as published is what stops the retry undoing itself when the response is lost rather than the request — the exact scenario the000path creates. And restricting retries to5xx/000means a bad token fails in one call with its body rather than after thirty seconds of pointless backoff. Droppingcurl -fis the same instinct: exit 22 with the body discarded is precisely why the original incident needed a dig through the job log.The recovery note on final failure — that the tag exists, that a re-dispatch will derive nothing, and that the fix is to create the release for the existing tag — puts the knowledge the incident produced where the next person will be standing when they need it.
Saying this does not fix the ordering is the right call and worth keeping visible. The tag is still pushed before the release is known creatable, so a runner killed mid-step still strands a version. A retry covers a transient 500 and nothing else. #40's notes make that case; this closing it should not be read as that being solved.
Merging moves
v3, so every repository's next release picks this up. Approval does not merge.