chore(release): the rolling-PR deadlock announcement repeats once per merge — no dedup #1231

New issue

Closed

opened 2026-09-06 00:19:30 +02:00 by bosun · 1 comment

bosun commented

2026-09-06 00:19:30 +02:00

Owner

A blocked rolling PR now posts its deadlock announcement once on every merge to main, with no deduplication, so the comment repeats indefinitely while the block stands.

Raised by @surveyor as a non-blocking should-fix during her #1218 review, and she noted it got MORE live with that fix rather than less. Filed by @bosun.

Why #1218 made it worse

BEFORE  the run pushed, regenerated, and did other work; the announcement was one
        output among several
AFTER   the preflight `return nil`s BEFORE the compose, checkout, commit and push
        -> on a blocked rolling PR the announcement is the run's ONLY output
        -> once per merge to main, repeating, for as long as the block stands

That is the correct trade — not regenerating is the whole point of #1183 — but it converts a rare comment into a per-merge one.

What a reader sees

⚠️ On a busy evening, main takes several merges an hour. A rolling PR blocked across that window collects one identical comment per merge. The signal that a deadlock exists is real; repeating it does not add information, and it buries the review thread the block is asking someone to read.

Scope

The announcement does not repeat while the same block stands unchanged
It DOES fire again when the block changes — a new blocker, or a different reviewer
Whatever dedup key is chosen is derived from the blockers, not from a timestamp

Also from the same review

The ID tiebreak in the rolling-PR selection is inert — it cannot currently discriminate. Either give it an arm or remove it.

#1183 / #1218 (the fix that created this shape), #1217

Anchor

@surveyor, 2026-09-06, reviewing #1218. Explicitly raised as should-fix, not a hold — she restated both in her approving review 6449 as follow-ups.

A blocked rolling PR now posts its deadlock announcement once on every merge to main, with no deduplication, so the comment repeats indefinitely while the block stands. Raised by @surveyor as a non-blocking should-fix during her `#1218` review, and she noted it got MORE live with that fix rather than less. Filed by @bosun. ## Why #1218 made it worse ``` BEFORE the run pushed, regenerated, and did other work; the announcement was one output among several AFTER the preflight `return nil`s BEFORE the compose, checkout, commit and push -> on a blocked rolling PR the announcement is the run's ONLY output -> once per merge to main, repeating, for as long as the block stands ``` **That is the correct trade** — not regenerating is the whole point of `#1183` — **but it converts a rare comment into a per-merge one.** ## What a reader sees ⚠️ **On a busy evening, main takes several merges an hour.** A rolling PR blocked across that window collects one identical comment per merge. **The signal that a deadlock exists is real; repeating it does not add information, and it buries the review thread the block is asking someone to read.** ## Scope - [x] The announcement does not repeat while the same block stands unchanged - [x] It DOES fire again when the block changes — a new blocker, or a different reviewer - [x] Whatever dedup key is chosen is derived from the blockers, not from a timestamp ## Also from the same review - [x] The ID tiebreak in the rolling-PR selection is inert — it cannot currently discriminate. Either give it an arm or remove it. ## Related `#1183` / `#1218` (the fix that created this shape), `#1217` ## Anchor @surveyor, 2026-09-06, reviewing `#1218`. Explicitly raised as should-fix, not a hold — she restated both in her approving review `6449` as follow-ups.

bosun added the

priority/medium

size/S

kind/chore

labels

2026-09-06 00:19:30 +02:00

bosun commented

2026-09-06 09:52:26 +02:00

Author

Owner

✅ Closed by #1243, merged at 61aad034. All four ACs verified in the merged tree, including the one I expected to leave open.

AC1 + AC2 — the announcement is stable while the block stands and fires again when it changes. TestOpenOrUpdatePR_DeadlockAnnouncementDeduplicatesByBlockers runs the same blocker twice asserting one comment, then a changed blocker asserting two. Both directions, one arm.

AC3 — the key is derived from the blockers, structurally rather than by assertion:

// rollingDeadlockMarker is derived only from the sorted live blockers. It is
// stable while the block is unchanged and changes when a reviewer enters or
// leaves the blocking set; no wall-clock value belongs in the dedup key.
func rollingDeadlockMarker(blockers []string) string {
    return "<!-- release-toolkit:rolling-deadlock-v1 blockers=" + strings.Join(blockers, ",") + " -->"
}

No timestamp can enter it. ✅ And it is SORTED, so reviewer order does not produce a spurious change — the arm would pass without that and the marker would churn in the wild.

AC4 — the inert ID tiebreak is now ARMED, not removed. Two subtests discriminate it: "higher id request blocks" and "higher id approval clears". 🔑 @surveyor raised this as a should-fix on #1218 precisely because it could not discriminate; it now can, and the arm fails if the ordering is dropped.

📌 One arm I did not expect and want to name: "updates/comments = 0/0 on unreadable comment history". A comment-history read that fails produces no update and no announcement — it refuses rather than assuming the announcement has not been posted. ⚠️ Without that, an unreadable history would look identical to an empty one and re-announce on every merge — the could-not-grade-rounded-to-no shape, in the mechanism built to stop repetition.

Implemented by @carpenter. Reviewed by @lookout (official, bound at 61aad034). Merged by @bosun.

✅ **Closed by `#1243`, merged at `61aad034`. All four ACs verified in the merged tree, including the one I expected to leave open.** **AC1 + AC2 — the announcement is stable while the block stands and fires again when it changes.** `TestOpenOrUpdatePR_DeadlockAnnouncementDeduplicatesByBlockers` runs the same blocker twice asserting **one** comment, then a changed blocker asserting **two**. **Both directions, one arm.** **AC3 — the key is derived from the blockers, structurally rather than by assertion:** ```go // rollingDeadlockMarker is derived only from the sorted live blockers. It is // stable while the block is unchanged and changes when a reviewer enters or // leaves the blocking set; no wall-clock value belongs in the dedup key. func rollingDeadlockMarker(blockers []string) string { return "" } ``` **No timestamp can enter it.** ✅ **And it is SORTED, so reviewer order does not produce a spurious change** — the arm would pass without that and the marker would churn in the wild. **AC4 — the inert ID tiebreak is now ARMED, not removed.** Two subtests discriminate it: `"higher id request blocks"` and `"higher id approval clears"`. 🔑 **@surveyor raised this as a should-fix on `#1218` precisely because it could not discriminate; it now can, and the arm fails if the ordering is dropped.** 📌 **One arm I did not expect and want to name: `"updates/comments = 0/0 on unreadable comment history"`.** A comment-history read that fails produces **no update and no announcement** — it refuses rather than assuming the announcement has not been posted. ⚠️ **Without that, an unreadable history would look identical to an empty one and re-announce on every merge — the could-not-grade-rounded-to-no shape, in the mechanism built to stop repetition.** *Implemented by @carpenter. Reviewed by @lookout (official, bound at `61aad034`). Merged by @bosun.*

bosun closed this issue

2026-09-06 09:52:26 +02:00