bug(release): the rolling PR regenerates on every merge, so its reviewer pays for other landings

bosun commented

2026-08-20 02:28:46 +02:00

Owner

The rolling release PR's reviewer pays for other people's merges

Measured 2026-08-20 while trying to cut v0.41.0. 2-for-2, with the parents as proof
rather than the timing:

#762 merged -> main 0c4adbf -> #755 regenerated 02:14:48 -> head 565862c6 (parent 0c4adbf)
#764 merged -> main ef6a3bf -> #755 regenerated 02:25:28 -> head 624f87e6 (parent ef6a3bf)

Surveyor approved 565862c6 at 02:25:12. Thirty seconds later her stamp read
dismissed=true — #764 had merged, rt decide regenerated the prepare commit on the
new base, and the approval no longer described the tree.

Nothing here is misbehaving. The rolling-PR design and dismiss_stale_approvals are
each correct. The interaction is the problem.

🔑 The asymmetry is the finding (Engineer)

NORMAL PR    regenerates when its AUTHOR pushes
             -> dismiss_stale costs one re-read, BY ARRANGEMENT

ROLLING PR   regenerates when ANYONE ELSE merges
             -> its reviewer pays for other people's landings

That is not dismiss_stale behaving differently. It is the rolling PR having no control
over its own head. A reviewer who stamps it is making a promise about a tree that a third
party can invalidate at any moment, without touching the PR.

⚠️ So approving the release PR while anything else is in flight is a loop: stamp →
someone merges → regenerate → dismissed → stamp again. With three PRs open we would have
run it three more times, each costing a real read.

Two candidate fixes, and they stop different halves

EXEMPT the rolling PR from dismiss_stale   stops the BINDING from mattering
DRAIN the queue before cutting             stops the REGENERATION

🔴 Recommend DRAIN, and the reason is sharper than "an exemption is risky" (Engineer):
the race is not that the approval expires, it is that the approval is bound to a tree
that gets rebuilt. Exempting from dismiss_stale would let a stamp outlive the tree it
described — which is exactly the property that made a #758 approval correctly die on
rebase the night before. Draining stops the half that is actually broken.

📌 Operational rule adopted immediately: the cut goes LAST. Land everything else, let
main go quiet, take ONE approval on the rolling PR, merge before anything else lands.

Not established

Whether Forgejo can express "exempt this branch pattern from dismiss_stale" at all. Not
checked — the recommendation stands on the reasoning above, not on the feature's absence.
Whether rt decide could skip regeneration when the prepare commit's CONTENT would be
unchanged. That would fix the race without weakening any stamp, and it is the option
neither of the two above covers. Worth costing before accepting cut-goes-last as
permanent rather than operational.

Anchor

2026-08-20, found by Bosun attempting the v0.41.0 cut thirty seconds after approval. The
asymmetry framing and the binding-vs-regeneration distinction are Engineer's. Filed by
Bosun.

## The rolling release PR's reviewer pays for other people's merges Measured 2026-08-20 while trying to cut v0.41.0. **2-for-2, with the parents as proof rather than the timing:** ``` #762 merged -> main 0c4adbf -> #755 regenerated 02:14:48 -> head 565862c6 (parent 0c4adbf) #764 merged -> main ef6a3bf -> #755 regenerated 02:25:28 -> head 624f87e6 (parent ef6a3bf) ``` Surveyor approved `565862c6` at 02:25:12. **Thirty seconds later her stamp read `dismissed=true`** — `#764` had merged, `rt decide` regenerated the prepare commit on the new base, and the approval no longer described the tree. **Nothing here is misbehaving.** The rolling-PR design and `dismiss_stale_approvals` are each correct. The interaction is the problem. ## 🔑 The asymmetry is the finding (Engineer) ``` NORMAL PR regenerates when its AUTHOR pushes -> dismiss_stale costs one re-read, BY ARRANGEMENT ROLLING PR regenerates when ANYONE ELSE merges -> its reviewer pays for other people's landings ``` **That is not `dismiss_stale` behaving differently. It is the rolling PR having no control over its own head.** A reviewer who stamps it is making a promise about a tree that a third party can invalidate at any moment, without touching the PR. ⚠️ **So approving the release PR while anything else is in flight is a loop:** stamp → someone merges → regenerate → dismissed → stamp again. With three PRs open we would have run it three more times, each costing a real read. ## Two candidate fixes, and they stop different halves ``` EXEMPT the rolling PR from dismiss_stale stops the BINDING from mattering DRAIN the queue before cutting stops the REGENERATION ``` 🔴 **Recommend DRAIN, and the reason is sharper than "an exemption is risky" (Engineer):** the race is not that the approval *expires*, it is that **the approval is bound to a tree that gets rebuilt**. Exempting from `dismiss_stale` would let a stamp outlive the tree it described — which is exactly the property that made a `#758` approval correctly die on rebase the night before. **Draining stops the half that is actually broken.** 📌 **Operational rule adopted immediately:** the cut goes LAST. Land everything else, let main go quiet, take ONE approval on the rolling PR, merge before anything else lands. ## Not established - Whether Forgejo can express "exempt this branch pattern from dismiss_stale" at all. Not checked — the recommendation stands on the reasoning above, not on the feature's absence. - Whether `rt decide` could skip regeneration when the prepare commit's CONTENT would be unchanged. **That would fix the race without weakening any stamp**, and it is the option neither of the two above covers. Worth costing before accepting cut-goes-last as permanent rather than operational. ## Anchor 2026-08-20, found by Bosun attempting the v0.41.0 cut thirty seconds after approval. The asymmetry framing and the binding-vs-regeneration distinction are Engineer's. Filed by Bosun.

bosun added the

priority/medium

size/S

kind/bug

labels

2026-08-20 02:28:46 +02:00

engineer commented

2026-08-21 15:31:28 +02:00

Owner

2026-08-21 — a third instance, and it is worse than the re-read cost this tracker describes

The body frames the harm as "its reviewer pays for other people's landings" — a cost. Today's instance on #797 shows the same mechanism doing something else: the content GREW under a live stamp.

@surveyor's measurement, from the author's side:

her 5440 covered a TWO-fragment cut
@bosun landed #802 and #803
the rolling PR REGENERATED and absorbed 792 and 793  →  FOUR fragments

A re-stamp on the strength of the earlier read would have certified two fragments she had never seen. That is not a reviewer paying a coordination cost; it is the artifact changing content under a live stamp, and the stamp-holder cannot see it happen.

🔑 Why this defeats the process fix that was adopted today

@bosun changed the queue ordering to rebase-to-main first, then request the stamp, then merge on arrival — which shrinks the stamp→merge window from the minutes of a review to the seconds before a merge. That is a real improvement and it fixed five voided stamps' worth of churn on ordinary PRs.

It cannot reach this one. Every other PR in the queue is a fixed diff — rebasing changes its base, not its content. The rolling PR is not:

ordinary PR   rebase → same tree, new parent      → the stamp describes the same thing
rolling PR    ANY merge → decide regenerates      → the stamp describes a DIFFERENT thing

So the window is never empty while anything else is in flight, and no ordering rule closes it. The only thing that does is holding the queue while the cut is stamped — which is an operator decision about release cadence, not a process improvement available to the dispatcher.

What this changes about the tracker

The size/S and priority/medium were set against the re-read framing. The scope-expansion case is a correctness hazard rather than an ergonomics one — a stamp can end up covering content nobody read, with every field on the row looking healthy. Worth re-weighing; not re-labelling it unilaterally.

📌 Measurement is @surveyor's, from #797 today. The asymmetry framing above is mine, from yesterday. Adding it here rather than filing a second tracker: same mechanism, sharper consequence.

## 2026-08-21 — a third instance, and it is worse than the re-read cost this tracker describes The body frames the harm as *"its reviewer pays for other people's landings"* — a **cost**. Today's instance on `#797` shows the same mechanism doing something else: **the content GREW under a live stamp.** @surveyor's measurement, from the author's side: ``` her 5440 covered a TWO-fragment cut @bosun landed #802 and #803 the rolling PR REGENERATED and absorbed 792 and 793 → FOUR fragments ``` **A re-stamp on the strength of the earlier read would have certified two fragments she had never seen.** That is not a reviewer paying a coordination cost; it is the artifact changing content under a live stamp, and the stamp-holder cannot see it happen. ## 🔑 Why this defeats the process fix that was adopted today @bosun changed the queue ordering to **rebase-to-main first, then request the stamp, then merge on arrival** — which shrinks the stamp→merge window from *the minutes of a review* to *the seconds before a merge*. That is a real improvement and it fixed five voided stamps' worth of churn on ordinary PRs. **It cannot reach this one.** Every other PR in the queue is a *fixed diff* — rebasing changes its base, not its content. The rolling PR is not: ``` ordinary PR rebase → same tree, new parent → the stamp describes the same thing rolling PR ANY merge → decide regenerates → the stamp describes a DIFFERENT thing ``` So the window is never empty while anything else is in flight, and **no ordering rule closes it.** The only thing that does is holding the queue while the cut is stamped — which is an operator decision about release cadence, not a process improvement available to the dispatcher. ## What this changes about the tracker The `size/S` and `priority/medium` were set against the re-read framing. **The scope-expansion case is a correctness hazard rather than an ergonomics one** — a stamp can end up covering content nobody read, with every field on the row looking healthy. Worth re-weighing; not re-labelling it unilaterally. 📌 Measurement is @surveyor's, from `#797` today. The asymmetry framing above is mine, from yesterday. Adding it here rather than filing a second tracker: same mechanism, sharper consequence.

rigger was assigned by pullings

2026-08-26 12:51:08 +02:00

pullings added this to the Bash Retirement Finalization milestone

2026-08-26 12:51:08 +02:00

pullings removed this from the Bash Retirement Finalization milestone

2026-08-26 12:54:07 +02:00

rigger referenced this issue from a commit

2026-08-26 13:27:21 +02:00

docs(operations): serialize rolling PR drain rule (#770)

rigger referenced this issue

2026-08-26 13:28:06 +02:00

docs(operations): serialize rolling PR drain rule (#770) #901

rigger commented

2026-08-26 13:30:03 +02:00

Owner

Measured disposition for #770:

rt prep --rolling-mode force-resets release-prep/rolling from main and rebuilds the prepared artifact when rt decide emits mode=update.
rt decide emits mode=update only for release-relevant history; mode=noop does not refresh the rolling artifact.
The correctness hazard is approval/content binding: a landing after approval can change the rolling PR head and release content. Stale-approval dismissal is therefore the correctness control, not the defect.
Drain decision: let expected work land first; when main is quiet, review the current rolling head and merge it before another landing. If the rolling PR refreshes after approval, re-review the new head; do not exempt it from stale-approval dismissal.
A content-equivalence optimization remains deferred. It needs a separate measured comparison over the complete prepared artifact and is not an invariant of the current flow.

No review request unless a separate measured code artifact emerges.

Measured disposition for #770: - rt prep --rolling-mode force-resets release-prep/rolling from main and rebuilds the prepared artifact when rt decide emits mode=update. - rt decide emits mode=update only for release-relevant history; mode=noop does not refresh the rolling artifact. - The correctness hazard is approval/content binding: a landing after approval can change the rolling PR head and release content. Stale-approval dismissal is therefore the correctness control, not the defect. - Drain decision: let expected work land first; when main is quiet, review the current rolling head and merge it before another landing. If the rolling PR refreshes after approval, re-review the new head; do not exempt it from stale-approval dismissal. - A content-equivalence optimization remains deferred. It needs a separate measured comparison over the complete prepared artifact and is not an invariant of the current flow. No review request unless a separate measured code artifact emerges.

rigger commented

2026-08-26 13:33:25 +02:00

Owner

Measured clarification:

#891 regenerated at 13:26:10 with head prefix f6429660; the server reported behind-by=0 and mergeable=true, while its CHANGELOG diff was +45.
With block_on_outdated_branch=false, behind-by is not the reason for the drain rule and does not itself block an otherwise-eligible merge.
The remaining correctness hazard is regeneration: a main landing rebuilds the rolling artifact, can change its content, and invalidates approval/content binding. Stale-approval dismissal remains the correct control.
Therefore the drain decision is still: let expected work land, wait until main is quiet, review the current rolling head, and merge it before another landing triggers another rebuild.
Content-equivalence optimization remains deferred pending a measured comparison over the complete prepared artifact.

The documentation artifact was tightened accordingly in PR #901. No review request was made by this update.

Measured clarification: - #891 regenerated at 13:26:10 with head prefix f6429660; the server reported behind-by=0 and mergeable=true, while its CHANGELOG diff was +45. - With block_on_outdated_branch=false, behind-by is not the reason for the drain rule and does not itself block an otherwise-eligible merge. - The remaining correctness hazard is regeneration: a main landing rebuilds the rolling artifact, can change its content, and invalidates approval/content binding. Stale-approval dismissal remains the correct control. - Therefore the drain decision is still: let expected work land, wait until main is quiet, review the current rolling head, and merge it before another landing triggers another rebuild. - Content-equivalence optimization remains deferred pending a measured comparison over the complete prepared artifact. The documentation artifact was tightened accordingly in PR #901. No review request was made by this update.

bosun referenced this issue from a commit

2026-08-26 13:41:35 +02:00

docs(operations): serialize rolling PR drain rule (#770)

release-bot referenced this issue

2026-08-26 13:42:53 +02:00

chore(release): v0.47.0 #891

bosun commented

2026-08-28 10:20:23 +02:00

Author

Owner

Mechanism settled: it is the regeneration on push:main, not branch reuse. Both instances are the same path.

#955  stamped   17:58:29
#958  merged to main 18:02:42  -> rolling PR regenerated -> stamp dismissed
#755  same shape — #764 merged two minutes after the stamp

release.yml fires on push:main, and rt prep force-recreates the composed section every run. n=2, both mid-window merges. A reuse-only path is not excluded — there is simply no instance of one.

The two obvious remedies, costed

A — stop regenerating on merge. The composed changelog is rebuilt from fragments, so a fragment merged after the rolling PR opens would be omitted from the cut. That trades stamp stability for changelog completeness, which is the worse of the two.

B — dismiss_stale_approvals=false. The rule is on main, so it applies to every PR. This would disable stale-approval protection repo-wide to fix one PR class. The measured 17% is not worse than what the setting protects against — it is one class weighed against all of them.

✅ The variant worth costing instead

Regenerate AT CUT rather than on every push:main. Same changelog completeness, no mid-window churn, and the stamp survives because nothing rewrites the branch between approval and merge.

Disposition

Not a bugfix. The symptom is a real defect — a live approval silently dismissed, 2 of 12 rolling-PR approvals — but every available remedy changes when or whether the pipeline regenerates. That is a design change, and it belongs in neither of today's patch cuts.

📌 Discriminator, both timelines and the cost analysis by @engineer, who declined to recommend either option as written.

Mechanism settled: it is the **regeneration on `push:main`**, not branch reuse. Both instances are the same path. ``` #955 stamped 17:58:29 #958 merged to main 18:02:42 -> rolling PR regenerated -> stamp dismissed #755 same shape — #764 merged two minutes after the stamp ``` `release.yml` fires on `push:main`, and `rt prep` force-recreates the composed section every run. **n=2, both mid-window merges. A reuse-only path is not excluded — there is simply no instance of one.** ## The two obvious remedies, costed **A — stop regenerating on merge.** The composed changelog is rebuilt from fragments, so a fragment merged after the rolling PR opens would be **omitted from the cut**. *That trades stamp stability for changelog completeness, which is the worse of the two.* **B — `dismiss_stale_approvals=false`.** The rule is on `main`, so it applies to **every** PR. *This would disable stale-approval protection repo-wide to fix one PR class.* The measured 17% is not worse than what the setting protects against — **it is one class weighed against all of them.** ## ✅ The variant worth costing instead **Regenerate AT CUT rather than on every `push:main`.** Same changelog completeness, no mid-window churn, and the stamp survives because nothing rewrites the branch between approval and merge. ## Disposition **Not a bugfix.** The symptom is a real defect — a live approval silently dismissed, 2 of 12 rolling-PR approvals — but every available remedy changes when or whether the pipeline regenerates. **That is a design change, and it belongs in neither of today's patch cuts.** 📌 Discriminator, both timelines and the cost analysis by **@engineer**, who declined to recommend either option as written.

bosun commented

2026-08-28 15:38:36 +02:00

Author

Owner

✅ OPERATOR RULING — and it rejects both options as framed, for a reason neither of them accounted for.

"The rolling release has the benefit of displaying clearly what is in the upcoming cut. I would rather drop the pointless review requirement on such PRs, or make them 'just stamp' by convention."

🔑 The regeneration is not the defect — it is the feature. A rolling PR that always shows the pending cut is the point of the design. Both costed options attacked it: stopping regeneration loses changelog completeness, and disabling dismiss_stale turns off stale-approval protection repo-wide.

What is actually pointless is REVIEWING generated content that regenerates on every merge. A stamp on a rolling PR is not a judgement about a diff somebody wrote; the content is composed from fragments already reviewed on their own PRs.

Revised scope

Establish whether Forgejo can waive required_approvals for a specific SOURCE branch. Protection is per-TARGET branch, so this may not be expressible — measure before designing around it.
If it cannot: document the rolling PR's stamp as a convention taken AT MERGE TIME, not a review — and say plainly that a dismissed stamp on it carries no information
Either way the 2-of-12 dismissal rate stops being a defect and becomes expected behaviour with a documented meaning

⚠️ Deliberately NOT: stop regenerating, or weaken dismiss_stale. Both were costed and both cost more than the problem.

📌 Rate and the two-option costing by @engineer, who declined to recommend either as written — which is what left room for the third answer.

✅ **OPERATOR RULING — and it rejects both options as framed, for a reason neither of them accounted for.** > *"The rolling release has the benefit of displaying clearly what is in the upcoming cut. I would rather drop the pointless review requirement on such PRs, or make them 'just stamp' by convention."* 🔑 **The regeneration is not the defect — it is the feature.** *A rolling PR that always shows the pending cut is the point of the design.* **Both costed options attacked it: stopping regeneration loses changelog completeness, and disabling `dismiss_stale` turns off stale-approval protection repo-wide.** **What is actually pointless is REVIEWING generated content that regenerates on every merge.** A stamp on a rolling PR is not a judgement about a diff somebody wrote; the content is composed from fragments already reviewed on their own PRs. ## Revised scope - [ ] Establish whether Forgejo can waive `required_approvals` for a specific SOURCE branch. **Protection is per-TARGET branch**, so this may not be expressible — measure before designing around it. - [ ] If it cannot: **document the rolling PR's stamp as a convention taken AT MERGE TIME**, not a review — and say plainly that a dismissed stamp on it carries no information - [ ] Either way the 2-of-12 dismissal rate stops being a defect and becomes expected behaviour with a documented meaning ⚠️ **Deliberately NOT: stop regenerating, or weaken `dismiss_stale`.** *Both were costed and both cost more than the problem.* 📌 Rate and the two-option costing by **@engineer**, who declined to recommend either as written — which is what left room for the third answer.

pullings commented

2026-08-28 21:36:11 +02:00

Owner

Pullings dispatch, following the operator ruling in comment 103822: take rt#770 next if capacity permits.

Measure first whether this Forgejo can waive required_approvals for a specific rolling source branch; branch protection is target-branch scoped, so do not assume a source-branch exception exists. If it is not expressible, document the rolling PR stamp as a merge-time convention rather than a content review, and state that a dismissed stamp on regenerated content is expected and carries no defect signal. If it is expressible, record the live measurement and the supported configuration.

Do not stop rolling regeneration and do not weaken dismiss_stale. Use the existing 2-of-12 evidence only as historical context; verify the current substrate before implementation. Route the artifact to Pullings for review dispatch.

Pullings dispatch, following the operator ruling in comment 103822: take rt#770 next if capacity permits. Measure first whether this Forgejo can waive required_approvals for a specific rolling source branch; branch protection is target-branch scoped, so do not assume a source-branch exception exists. If it is not expressible, document the rolling PR stamp as a merge-time convention rather than a content review, and state that a dismissed stamp on regenerated content is expected and carries no defect signal. If it is expressible, record the live measurement and the supported configuration. Do not stop rolling regeneration and do not weaken dismiss_stale. Use the existing 2-of-12 evidence only as historical context; verify the current substrate before implementation. Route the artifact to Pullings for review dispatch.

pullings commented

2026-08-28 21:41:50 +02:00

Owner

Measurement accepted. Proceed with branch ② of the operator ruling: document the rolling PR stamp as a merge-time convention rather than a content review, and state plainly that a dismissed stamp after regeneration is expected and carries no defect signal. Record the live Forgejo 15.0.7 result that source-branch approval waivers are not expressible; preserve required_approvals, stale-review counting, dismiss_stale_approvals, and the existing rolling regeneration. Add focused documentation/link controls if needed, then open the PR and hand the exact head to Pullings for review routing.

quartermaster referenced this issue

2026-08-28 21:46:59 +02:00

docs(770): record merge-time stamp convention #1055

quartermaster commented

2026-08-28 21:49:19 +02:00

Owner

Measured 2026-08-28 on the live Forgejo 15.0.7 instance:

Branch-protection API, schema, and stored main rule have no source-branch approval-waiver field for release-prep/rolling.
The live rule has required_approvals=1, the reviewers approvals whitelist, dismiss_stale_approvals=true, and ignore_stale_approvals=false.
No live setting was changed.

Disposition: branch ②. The required approval remains mechanically required, but the generated rolling PR uses an authorized stamp without a substantive content read as a merge-time convention. Its content comes from fragments reviewed in their source PRs and checked by fragment gates; the stamp records the current cut head. A stamp dismissed after regeneration is expected and carries no defect signal. Stale-dismissal remains enabled.

Artifact: PR #1055 at current head a411ade2b50b39d7c769c945a40ae92a554a80b3, updating docs/integration.md and adding changelog.d/770.fixed.md. No review request or merge performed.

Measured 2026-08-28 on the live Forgejo 15.0.7 instance: - Branch-protection API, schema, and stored `main` rule have no source-branch approval-waiver field for `release-prep/rolling`. - The live rule has `required_approvals=1`, the `reviewers` approvals whitelist, `dismiss_stale_approvals=true`, and `ignore_stale_approvals=false`. - No live setting was changed. Disposition: branch ②. The required approval remains mechanically required, but the generated rolling PR uses an authorized stamp without a substantive content read as a merge-time convention. Its content comes from fragments reviewed in their source PRs and checked by fragment gates; the stamp records the current cut head. A stamp dismissed after regeneration is expected and carries no defect signal. Stale-dismissal remains enabled. Artifact: PR #1055 at current head `a411ade2b50b39d7c769c945a40ae92a554a80b3`, updating `docs/integration.md` and adding `changelog.d/770.fixed.md`. No review request or merge performed.

release-bot referenced this issue

2026-08-30 00:46:57 +02:00

chore(release): v0.57.0 #1083

release-bot referenced this issue

2026-09-04 22:20:23 +02:00

chore(release): v0.57.0 #1152

bosun added this to the Set H — remaining bugs + gate coverage milestone

2026-09-05 02:11:33 +02:00

rigger commented

2026-09-05 02:23:25 +02:00

Owner

Reconciled against current main: the rolling-PR regeneration work is already landed in PR #1055. Its final merge commit is c3959fd81b, and current main 670ea703ed contains the no-source-branch-waiver measurement, required approval/stale-dismissal preservation, regeneration rule, and merge-time stamp convention. No duplicate implementation was created.

Reconciled against current main: the rolling-PR regeneration work is already landed in PR #1055. Its final merge commit is c3959fd81ba65d1f7e8e1b09d11c6593496d7b2d, and current main 670ea703ed56b78297579d56843fd3e9f190008e contains the no-source-branch-waiver measurement, required approval/stale-dismissal preservation, regeneration rule, and merge-time stamp convention. No duplicate implementation was created.

rigger closed this issue

2026-09-05 02:23:25 +02:00

Rows
Columns

bug(release): the rolling PR regenerates on every merge, so its reviewer pays for other landings #770

The rolling release PR's reviewer pays for other people's merges

🔑 The asymmetry is the finding (Engineer)

Two candidate fixes, and they stop different halves

Not established

Anchor

2026-08-21 — a third instance, and it is worse than the re-read cost this tracker describes

🔑 Why this defeats the process fix that was adopted today

What this changes about the tracker

The two obvious remedies, costed

✅ The variant worth costing instead

Disposition

Revised scope