feat(vcluster-release): resume an interrupted cut instead of refusing the re-run - #246
Open
sydorovdmytro wants to merge 12 commits into
Open
feat(vcluster-release): resume an interrupted cut instead of refusing the re-run#246sydorovdmytro wants to merge 12 commits into
sydorovdmytro wants to merge 12 commits into
Conversation
… the re-run The double-cut guard treated any pre-existing tag or release as fatal, so pressing the release button again after a partial failure could not finish the cut. Recovery was a manual, three-branch procedure that involved deleting tags. Each mutating step now reads back per-repo state (absent/tagged/dispatched/ released) and skips what is already done. Only an all-released version is refused, since a published release is the one state that proves the version shipped. A tag is never re-pointed and a build is never dispatched twice.
A missing tag was read as 'nothing happened yet'. If the tag had been deleted while a build was still running against it, the resume re-created it at the current branch head and dispatched a second build, breaking both invariants at once. A run that has not completed now makes that state fatal. Scoped to in-flight runs on purpose: run records outlive the tag, so keying on 'any run ever' would permanently block the documented delete-and-re-cut path. A merged bump PR was trusted as proof the dependency was bumped. It only proves the bump once landed - a revert or force-push leaves the PR merged with the dependency back where it was, and pro would then be tagged against the wrong OSS code. The merged-PR shortcut is now confirmed against the branch's go.mod, and refuses loudly on a mismatch or an unreadable read rather than guessing. Matches the exact tag or a pseudo-version derived from it, since go get resolves to <base>.0.<timestamp>-<sha> when the tag is not yet a resolvable release.
…hind pro Two more resume holes from review, both on the legacy fan-out. Skipping the bump because pro was past 'absent' was an unverified inference. A pro tag from any other source ships whatever OSS version it vendored, and the resume would dispatch it. Under the old code a pre-existing pro tag was a hard stop so the inference was never load-bearing; now it is, so check the tag's own go.mod - the tag is the artifact, not the branch. OSS is always tagged before pro, so OSS can never legitimately lag pro for the same version. When it does, the OSS tag was deleted, and resuming would re-create it at today's branch head - publishing an OSS half built from different source than the pro half that already shipped.
set -e exited before `case $?`, so all four go.mod diagnostics were dead code in production. The bats suite could not see it because `run` disables errexit for the call it wraps; the tri-state calls now use `rc=0; f || rc=$?` and are covered by tests that execute the script in a child bash. The in-flight probe issued two filtered requests, in_progress then queued, which both miss a run that transitions between them and ignore requested/waiting/ pending entirely. One query now answers both questions from each run's own status. The go.mod parser matched the module name anywhere, so a replace block read as the required version and the single-line require form did not parse at all. It is now scoped to require directives. Also waits for a dispatched run to become queryable: gh workflow run returns before that, and the workflow concurrency group ends when the cut exits.
…l cut The barrier polls the runs endpoint in a loop after a dispatch that has already happened. Reusing the loud probe meant a transient blip printed up to twelve ::error:: annotations on a cut that exits 0 - a green release rendered red, and anything paging on ::error:: firing on success. Its wording was wrong there too: the dispatch is not being treated as un-dispatched, it just has not registered yet. Also stop sleeping after the final attempt.
Run records outlive tags. Delete a tag, re-cut it at a new commit, then lose the dispatch, and a count keyed only on the tag NAME sees the old completed run, calls the version dispatched, and silently never builds the new commit. The count is now restricted to the commit the tag points at. Resolving that commit is only attempted when the tag exists - on a 404 gh api prints the raw error body to stdout without applying --jq, so the value has to be shape-checked rather than trusted - and a tag whose commit cannot be resolved is fatal rather than degrading to an unscoped count. Also assert the runs response is actually shaped like a run list, so a truncated or error body cannot render as a plausible "nothing here".
…ier on an unknown sha An empty sha makes the run count unscoped, which counts runs of previous incarnations of a re-cut tag. The barrier treated that as evidence its own dispatch was visible and returned immediately on the strength of an old build - the opposite of what the comment claimed. It now resolves the sha every attempt and keeps waiting while it is unknown. An annotated tag's ref points at a tag object, while a workflow run reports the peeled commit, so an already-dispatched annotated tag looked un-dispatched and would be built twice. tag_sha_of now peels it. The test stub read the API path from $1, but create_tag calls `gh api -X POST <path>`, so its tag-created marker was never written and two barrier tests passed vacuously. It now scans for the path argument.
Scoping the dispatched check to the tag's commit hid a run that is still in flight against a different commit: the state then read as 'tagged' and the cut dispatched, leaving two builds racing to publish one version from two commits. The active count is deliberately unscoped, so it still sees that run.
loft-bot
reviewed
Sep 1, 2026
loft-bot
left a comment
loft-bot
left a comment
There was a problem hiding this comment.
Panel review: 2 blocking, 7 quality notes, 9 lanes.
Blocking concerns
src/vcluster-release.sh:651—bump_pr_probefails open, so a transient API error silently skips the newgo.modconfirmation before pro is tagged.test/vcluster-release.bats:108— theghstub hand-emits parsed output, so neither new--jqfilter is ever executed by a test.
What was checked
Checked: correctness, security, test-quality, operability, reuse, architecture, dead-code, typos, PR metadata. Skipped: the four e2e lanes, gap-analysis, entry-point fidelity, infra, and the layout-coherence pass (this repo has no e2e suite and no Go unit tests, the linked ticket is not a customer bug, no Terraform/Helm/manifest/workflow/Dockerfile changed, and the diff is not a test-tree restructure).
Worth saying explicitly: the resume state machine itself held up. Security cleared the jq splice (head_sha is hex-validated before interpolation), and the fail-closed handling in runs_at_ref, tag_sha_of and release_state_of traced correctly, including the errexit-in-command-substitution re-raise. The two limitations named in DEVOPS-1452 (the go.mod/tag-sha TOCTOU and the 100-run pagination cap) were treated as acknowledged follow-ups and not re-raised.
PR-level notes
- consider — the Test plan says "92 pass (was 84; 8 new resume tests)", but this head carries 121 tests (was 84; 37 net-new
@testblocks). DEVOPS-1452 records the correct 121, so the PR body looks stale. It understates the change's own coverage.
Quality notes (non-blocking)
- consider
src/vcluster-release.sh:472—guard_double_cuttakes bare state strings, so neither refusal error can name which repo the bad state came from; on a legacy cutannounce_state(which does name it) only runs after the guard, so nothing in the log disambiguates. - consider
src/vcluster-release.sh:362— the contract comment lists four states but the body can return six;announce_statehas an arm fordispatched-tag-missingthat can never fire and none fordispatched-other-commit, whileensure_tag/ensure_dispatchabsorb both into*)catch-alls. - consider
src/vcluster-release.sh:831-842— the pro-taggo.modpolicy block and the OSS-behind-pro guard are the only substantive policy left inline incut_legacy; extracting them as named helpers would restore the step sequence and let the suite call them directly. - nit
src/vcluster-release.sh:340— thetag_sha_ofcontract sentence is truncated mid-edit: "...rather than trusted. Callers" runs straight into "Empty means ...". - nit
README.md:135-139— the continuation lines under "A tag is never re-pointed" are not indented, so they render as a run-on paragraph that breaks out of the bullet list.
(+2 more: README.md:110 introduces "Two further states" and then lists three, the third of which is a cross-repo comparison rather than a release_state value; and test/vcluster-release.bats carries four stale symbol names in comments — bump_landed_on_branch at 144, workflow_runs_at_ref at 646, and guard_not_released at 35 and 661, none of which exist after this change.)
The go.mod confirmation hung off the branch where bump_pr_probe reported an already-merged PR. That probe fails open, so a transient API error made it report "no PR", the bump was re-dispatched, wait_for_bump_merge saw the already-merged PR and returned, and pro was tagged with the check never having run. The gate now sits in front of the tag itself, so every path through the bump reaches it. Extract the runs and tag-ref jq filters as constants, the way BUMP_PR_JQ already was, and cover them with tests that run the real expressions against real API response shapes. The gh stub hand-emits parsed output, so neither filter was being executed by the suite at all. Also: guard_double_cut takes repo=state pairs so a refusal names the repo it came from; the OSS-behind-pro rule and the go.mod policy move into named helpers; announce_state drops an arm that guard_double_cut makes unreachable; and the release_state contract lists all six states it can return.
It takes repo=state pairs now, not bare states.
That read fires immediately after the merge poll observes the merge, so it is the read most exposed to contents-API lag, and after moving the gate it now runs on every legacy path rather than only a resume. With no budget of its own a single blip aborted a cut that had already tagged OSS and merged the bump. Matched to the poll it follows. Only the fetch retries. A file that reads cleanly and names a different version is an answer, not a blip, and must not be retried into a different one.
Refusing on the wrong version does not prove the fetch was not retried - an implementation that re-read the same wrong version five times would refuse too. The stub now counts every read, including failing ones, so the transient, sustained and wrong-version cases each pin the exact call count.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes DEVOPS-1452
The double-cut guard treated any pre-existing tag or release as fatal, so after a
partial failure the release button could not be used to finish the cut. Recovery
was a manual procedure in the README involving deleting tags.
Each mutating step now reads back per-repo state (
absent/tagged/dispatched/released) and skips what is already done. Only an all-releasedversion is refused, since a published release is the one state that proves the
version shipped. Two further states are conflicts rather than progress and are
refused outright: a tag deleted under a running build, and a tag re-pointed under
one.
Before pro is tagged, the release branch (or, on a resume, the pro tag itself)
must actually require the OSS version being co-released. That gate sits in front
of the tag rather than on any one path to it, because
bump_pr_probefails open.Two invariants: a tag is never re-pointed, and a build is never dispatched twice.
The run count is scoped to the commit the tag points at, since run records
outlive tags.
Note the token now also needs to read workflow runs.
reposcope covers it on aclassic PAT; a fine-grained PAT needs
Actions: read. Any token that can alreadydispatch workflows has it. Worth one
dry-run: truecut as a preflight, sincefresh cuts now reach that endpoint.
Test plan
make test-vcluster-release— 131 pass, 0 fail (was 84; 47 net-new tests)make test— full repo suite, 0 failuresmake check-docs,shellcheck— cleanv0.34.8-rc.2: skips the existingOSS tag, sees the bump already merged, confirms
go.modrequires it, plans thepro tag + both dispatches
v0.34.8-rc.1andv0.37.0-rc.4(both shipped): refused, exit 1v0.34.9-rc.1andv0.37.9-rc.1(fresh): proceed, exit 0Includes tests that execute the script in a child bash, because bats'
rundisables errexit and hid a
set -ebug that made four diagnostics dead code inproduction.