frankenbit/release-toolkit

Watch

Fork 0

rt prep --rolling-mode creates a duplicate rolling PR: FindPRByHead sends head=owner:branch, which the forge now honours and which matches nothing #1584

New issue

Closed

opened 2026-10-21 21:35:37 +02:00 by quartermaster · 2 comments

quartermaster commented

2026-10-21 21:35:37 +02:00

Owner

rt prep --rolling-mode has been failing on frankenbit/storey since 2026-10-21T19:15Z — three runs, all fatal, after every piece of real work had already succeeded.

[rt prep] CHANGELOG transitioned + consumed fragments deleted: CHANGELOG.md
[rt prep] version_files: [] in config - tag-is-version mode; no files to bump
Error: forgejo: api error: POST /repos/frankenbit/storey/pulls status 409

The cut is in fact prepared: release-prep/rolling is at b758d7cf, PR #189 carries it, mergeable=true. Only the PR step failed, and it failed because it tried to create a PR that has existed since 2026-10-16.

The cause: a filter the code calls unreliable, sent in a form that cannot match

internal/forgejo/reads.go:486 (unchanged since 11025e9, 2026-07-26, and identical at v0.66.0 and main):

ep := fmt.Sprintf("/repos/%s/%s/pulls?state=open&head=%s:%s", repo.Owner, repo.Name, repo.Owner, headBranch)

Its own doc comment says "its server-side head filter is unreliable (#274), so the match is client-side on head.label OR head.ref exactly". The client-side match is sound. It is being starved of input. Measured against frankenbit/storey on Forgejo 17.0.0:

pulls?state=open                                       rows=2
pulls?state=open&head=release-prep/rolling             rows=1   <- the plain form works
pulls?state=open&head=frankenbit:release-prep/rolling  rows=0   <- what rt sends

The forge honours the filter, and owner:branch matches no head, so the list comes back empty, the client-side matcher has nothing to walk, FindPRByHead returns ErrNotFound, CheckRollingPR reports Found=false, and OpenOrUpdatePR takes the create branch — POST /pulls → 409.

🔑 The defect is relying on a parameter being IGNORED. #274 established the filter was unreliable; the response was to keep sending it and match client-side. That works for exactly as long as the server discards it. It is a latent create-a-duplicate waiting for the day the server starts honouring it, and the failure mode is not a wrong answer — it is a mutating call.

⚠️ And head.label does not carry the owner prefix on this instance — PR #189 reads head.label=release-prep/rolling, same as head.ref. So the label half of the client-side match is also built on an assumption that does not hold here; it only works because head.ref is checked too.

What I did NOT establish

That 17.0.0 is what began honouring it. routers/api/v1/repo/pull.go is byte-identical at v16.0.6 and v17.0.0 for both the list filter (HeadBranch: ctx.FormTrim("head")) and the duplicate check (StatusConflict from GetUnmergedPullRequest) — so the change, if there was one, is below the handler in the issues model, which I did not compare. The circumstantial case is that all three failures follow the 17.0.0 upgrade at 2026-10-21T15:42Z and the 2026-10-20T00:34Z run reached the same point and emitted its outputs without a POST error.

⚠️ Three hypotheses died on the way here and are recorded so nobody re-runs them: the forge's list-filter handler changed (identical source) · POST /pulls used to be idempotent (both tags return 409 from identical code, and the live forge returns 409) · the rt version changed (rt-v0.66.0-linux-amd64 in both runs' cache keys). And two instruments failed: a strings | grep over the forge binaries returned clean zeros for a positive control that must be present, and job-log mtimes are not run times — a log whose mtime reads 2026-10-21T12:59 contains a run from 2026-10-20T00:34, which invalidated a 128-invocation timeline I had built from them.

A direct A/B is available if anyone wants the attribution: rollback-pre-17.0.0/ is a 16.0.6-readable snapshot taken at 2026-10-21T17:42 local containing PR #189, so booting 16.0.6 against a copy and asking it the same two queries would settle it in one run.

The fixes, in order

Stop sending the filter. The client-side match is the authority, so the endpoint should carry no head at all — a parameter that is not sent cannot be honoured wrongly. Sending head=<branch> without the owner prefix also works today, but it keeps the dependency.
Make OpenOrUpdatePR idempotent on 409. A conflict from CreatePR means the PR exists, so re-find and UpdatePR instead of failing. The find→create window is real even with (1) fixed.
Reorder, or stop failing fatally after the mutation. Prep transitions the CHANGELOG, deletes consumed fragments and pushes the branch before the PR step. As it stands a failure there reports a red run over completed, pushed work, which is the worst available shape: a human reads "failure" and the cut is actually prepared. Either do the PR step first or let a 409 be non-fatal with a warning.
Fix the head.label assumption, or document that only the head.ref arm is load-bearing on Forgejo.

Filed from alcatraz. Consequence beyond storey: every repo whose rolling cut runs through --rolling-mode will take the create branch on every refresh once its rolling PR exists, so this is not storey-specific.

`rt prep --rolling-mode` has been failing on `frankenbit/storey` since 2026-10-21T19:15Z — three runs, all fatal, **after** every piece of real work had already succeeded. ``` [rt prep] CHANGELOG transitioned + consumed fragments deleted: CHANGELOG.md [rt prep] version_files: [] in config - tag-is-version mode; no files to bump Error: forgejo: api error: POST /repos/frankenbit/storey/pulls status 409 ``` The cut is in fact prepared: `release-prep/rolling` is at `b758d7cf`, PR **#189** carries it, `mergeable=true`. Only the PR step failed, and it failed because it tried to create a PR that has existed since 2026-10-16. ## The cause: a filter the code calls unreliable, sent in a form that cannot match `internal/forgejo/reads.go:486` (unchanged since `11025e9`, 2026-07-26, and identical at `v0.66.0` and `main`): ```go ep := fmt.Sprintf("/repos/%s/%s/pulls?state=open&head=%s:%s", repo.Owner, repo.Name, repo.Owner, headBranch) ``` Its own doc comment says *"its server-side `head` filter is unreliable (#274), so the match is client-side on head.label OR head.ref exactly"*. **The client-side match is sound. It is being starved of input.** Measured against `frankenbit/storey` on Forgejo 17.0.0: ``` pulls?state=open rows=2 pulls?state=open&head=release-prep/rolling rows=1 <- the plain form works pulls?state=open&head=frankenbit:release-prep/rolling rows=0 <- what rt sends ``` The forge **honours** the filter, and `owner:branch` matches no head, so the list comes back empty, the client-side matcher has nothing to walk, `FindPRByHead` returns `ErrNotFound`, `CheckRollingPR` reports `Found=false`, and `OpenOrUpdatePR` takes the create branch — `POST /pulls` → 409. 🔑 **The defect is relying on a parameter being IGNORED.** `#274` established the filter was unreliable; the response was to keep sending it and match client-side. That works for exactly as long as the server discards it. It is a latent create-a-duplicate waiting for the day the server starts honouring it, and the failure mode is not a wrong answer — it is a **mutating** call. ⚠️ **And `head.label` does not carry the owner prefix on this instance** — PR #189 reads `head.label=release-prep/rolling`, same as `head.ref`. So the `label` half of the client-side match is also built on an assumption that does not hold here; it only works because `head.ref` is checked too. ## What I did NOT establish **That 17.0.0 is what began honouring it.** `routers/api/v1/repo/pull.go` is byte-identical at `v16.0.6` and `v17.0.0` for both the list filter (`HeadBranch: ctx.FormTrim("head")`) and the duplicate check (`StatusConflict` from `GetUnmergedPullRequest`) — so the change, if there was one, is below the handler in the issues model, which I did not compare. The circumstantial case is that all three failures follow the 17.0.0 upgrade at 2026-10-21T15:42Z and the 2026-10-20T00:34Z run reached the same point and emitted its outputs without a POST error. ⚠️ **Three hypotheses died on the way here and are recorded so nobody re-runs them:** the forge's list-filter handler changed (identical source) · `POST /pulls` used to be idempotent (both tags return 409 from identical code, and the live forge returns 409) · the rt version changed (`rt-v0.66.0-linux-amd64` in both runs' cache keys). **And two instruments failed:** a `strings | grep` over the forge binaries returned clean zeros for a positive control that must be present, and job-log **mtimes are not run times** — a log whose mtime reads 2026-10-21T12:59 contains a run from 2026-10-20T00:34, which invalidated a 128-invocation timeline I had built from them. A direct A/B is available if anyone wants the attribution: `rollback-pre-17.0.0/` is a 16.0.6-readable snapshot taken at 2026-10-21T17:42 local containing PR #189, so booting 16.0.6 against a copy and asking it the same two queries would settle it in one run. ## The fixes, in order 1. **Stop sending the filter.** The client-side match is the authority, so the endpoint should carry no `head` at all — a parameter that is not sent cannot be honoured wrongly. Sending `head=<branch>` without the owner prefix also works today, but it keeps the dependency. 2. **Make `OpenOrUpdatePR` idempotent on 409.** A conflict from `CreatePR` means the PR exists, so re-find and `UpdatePR` instead of failing. The find→create window is real even with (1) fixed. 3. **Reorder, or stop failing fatally after the mutation.** Prep transitions the CHANGELOG, deletes consumed fragments and pushes the branch *before* the PR step. As it stands a failure there reports a red run over completed, pushed work, which is the worst available shape: a human reads "failure" and the cut is actually prepared. Either do the PR step first or let a 409 be non-fatal with a warning. 4. Fix the `head.label` assumption, or document that only the `head.ref` arm is load-bearing on Forgejo. Filed from alcatraz. Consequence beyond storey: every repo whose rolling cut runs through `--rolling-mode` will take the create branch on every refresh once its rolling PR exists, so this is not storey-specific.

quartermaster added the

labels

2026-10-21 21:55:49 +02:00

quartermaster referenced this issue from a commit

2026-10-21 22:04:16 +02:00

fix(prep): do not send a head filter the forge honours, and recover from the 409 it caused (#1584)

quartermaster referenced this issue

2026-10-21 22:04:30 +02:00

fix(prep): do not send a head filter the forge honours, and recover from the 409 it caused #1585

quartermaster commented

2026-10-21 22:04:31 +02:00

Author

Owner

Addressed in PR #1585 (head 9ab8282), awaiting review.

Item (4) of this issue is withdrawn — I filed it and it was wrong. I claimed the head.label arm of the client-side match rests on an assumption that does not hold here, because PR #189 reads head.label=release-prep/rolling with no owner prefix. The matcher compares both arms against the bare branch name, so both work; I had conflated the query string's owner: prefix with the comparison. AGENTS.md §head.ref-replaced-on-delete is why the label arm exists: head.ref is replaced with refs/pull/N/head once the branch is deleted, which broke the Layer 2 cut-safeguard in v0.6.0 and was fixed in v0.6.1 by reading head.label. Removing it would reintroduce that, so the PR adds an arm pinning it instead.

Item (3) is answered rather than implemented. With the 409 handled, the observed fail-after-mutation case is gone, and making the remaining PR-step failures non-fatal would be swallowing them — the exit status should stay non-zero when something genuinely fails. What was wrong was legibility, not control flow.

Addressed in PR #1585 (head `9ab8282`), awaiting review. **Item (4) of this issue is withdrawn — I filed it and it was wrong.** I claimed the `head.label` arm of the client-side match rests on an assumption that does not hold here, because PR #189 reads `head.label=release-prep/rolling` with no owner prefix. The matcher compares **both** arms against the bare branch name, so both work; I had conflated the query string's `owner:` prefix with the comparison. AGENTS.md §*head.ref-replaced-on-delete* is why the label arm exists: `head.ref` is replaced with `refs/pull/N/head` once the branch is deleted, which broke the Layer 2 cut-safeguard in v0.6.0 and was fixed in v0.6.1 by reading `head.label`. Removing it would reintroduce that, so the PR adds an arm pinning it instead. **Item (3) is answered rather than implemented.** With the 409 handled, the observed fail-after-mutation case is gone, and making the remaining PR-step failures non-fatal would be swallowing them — the exit status should stay non-zero when something genuinely fails. What was wrong was legibility, not control flow.

quartermaster referenced this issue from a commit

2026-10-21 22:20:57 +02:00

fix(changelog): the fragment failed this repo's own density gate (#1584)

release-bot referenced this issue

2026-10-21 22:28:42 +02:00

chore(release): v0.68.0 #1559

quartermaster commented

2026-10-21 22:29:09 +02:00

Author

Owner

Fixed by PR #1585, merged fast-forward-only at b4758310 on review 8010 (@surveyor, official=true, stale=false, commit_id=b4758310), all 5 required contexts green by complement.

Items (1) and (2) implemented; (3) answered — the reorder is not available, because commit, push and PR-open are fixed in that order and the body is composed from the sealed CHANGELOG, and it would not have prevented this anyway since the find ran before the create and returned a wrong answer rather than failing; (4) withdrawn as my own error, with an arm added to pin what it would have broken.

Storey's rolling cut should refresh cleanly on its next push to main.

Fixed by PR #1585, merged fast-forward-only at `b4758310` on review 8010 (@surveyor, `official=true`, `stale=false`, `commit_id=b4758310`), all 5 required contexts green by complement. Items (1) and (2) implemented; (3) answered — the reorder is not available, because commit, push and PR-open are fixed in that order and the body is composed from the sealed CHANGELOG, and it would not have prevented this anyway since the find ran before the create and returned a wrong answer rather than failing; (4) withdrawn as my own error, with an arm added to pin what it would have broken. Storey's rolling cut should refresh cleanly on its next push to main.

quartermaster closed this issue

2026-10-21 22:29:09 +02:00

quartermaster referenced this issue from a commit

2026-10-21 23:44:21 +02:00

feat(mirror): mirror on `release: published`, and stop reopening a closed tracker (#1586)

quartermaster referenced this issue

2026-10-21 23:44:52 +02:00

feat(mirror): mirror on release: published, and stop reopening a closed tracker #1587