[PR #122] [MERGED] fix(pr-risk): make the label sync a single atomic PUT (BE-6343) #132

New issue

Closed

opened 2026-10-26 22:36:35 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-26 22:36:35 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/Comfy-Org/github-workflows/pull/122
Author: @mattmillerai
Created: 8/4/2026
Status: ✅ Merged
Merged: 8/4/2026
Merged by: @mattmillerai

Base: main ← Head: matt/be-6343-atomic-label-put

📝 Commits (3)

7b510bf fix(pr-risk): make the label sync a single atomic PUT (BE-6343)
c90926d fix(pr-risk): harden the atomic label PUT against case variants and a broken filter (BE-6343)
f0ef9b4 fix(pr-risk): keep label names inside JSON, and refuse to guess on a jq error (BE-6343)

📊 Changes

4 files changed (+537 additions, -136 deletions)

View changed files

📝 .github/workflows/pr-risk.yml (+24 -9)
📝 scripts/pr-risk/README.md (+29 -10)
📝 scripts/pr-risk/apply-risk-label.sh (+171 -54)
📝 scripts/pr-risk/tests/test_apply_risk_label.sh (+313 -63)

📄 Description

ELI-5

Two grading runs could argue over one PR's risk label and both win. The old code did the label update in three steps — read what's there, delete the wrong one, add the right one — and two runs could slip between each other's steps, leaving the PR wearing two contradictory risk:* stickers at once. Now the update is a single request that says "here is the complete final list of stickers", so whoever writes last wins cleanly and the PR always ends up with exactly one.

What changed

scripts/pr-risk/apply-risk-label.sh replaces its read / delete-stale / add-target write sequence with one PUT /repos/{owner}/{repo}/issues/{n}/labels, which replaces the PR's full label set atomically. The desired set is built with jq from the snapshot read: every label the script does not own, carried through verbatim, plus the target.

Why this closes the race: a pr_numbers batch dispatch run and a pull_request event run for one of its numbers sit in different concurrency groups (a run belongs to exactly one group; a batch covers N PRs), so no concurrency key can serialize them. With the old shape, A reads {}, B reads {}, A adds risk:R1, B adds risk:R2 → two contradictory labels, until the next grade. With the PUT there is no delete/add window to interleave into: after any single writer's PUT the PR has exactly one owned label, and two concurrent writers end last-writer-wins with still exactly one (possibly the staler tier — acceptable, the label is advisory and the next grade re-syncs it). Zero-label is impossible because every PUT we build contains the target.

Deliberately not done, per the ticket: no verify-and-retry loop (concurrent writers would just ping-pong, and the invariant already holds by construction), and no POST-when-nothing-is-stale hybrid (two writers that both read an empty owned set would both POST and reproduce the exact bug — every write must be the PUT). Both are called out in an inline comment so a future "optimization" doesn't quietly reopen the race.

The DELETE loop is deleted entirely, including the HTTP-404 tolerance #113 added — there is no DELETE left for it to guard, and its test block is removed with it. Kept unchanged: input validation, LABEL_MAP parsing / OWNED derivation, the DRY_RUN plan, the enc() path helper (still needed by the repo-label probe), the ghq/gherr error plumbing, the paginated read, and the ensure-label-exists pre-create (a PUT creates the association but never a label's color or description, so first-use enrollment still needs the explicit POST repos/{r}/labels).

Docs: the script header's ownership-contract paragraph, the pr-risk.yml "A BATCH CANNOT SERIALIZE PER-PR" block, and the scripts/pr-risk/README.md residual-window section + apply-risk-label.sh file entry. The "prefer pr_number for a serialized re-grade" guidance is kept — still true, still useful for superseding waits via cancel-in-progress.

The new residual — please read, it is a pilot hazard

The PUT is built from a snapshot read, so a non-owned label added by a human between that read and the PUT is silently dropped — risk-dispute included. The window is ~one API round-trip and only opens on a run that actually changes the grade (an in-sync PR writes nothing at all), and it is far narrower than the double-label window it replaces, which lasted until the next grade. But disputes are this pilot's whole calibration dataset, and one lost there vanishes without a trace. Documented in all three places above; re-adding the label restores it.

Verification

bash scripts/pr-risk/tests/test_apply_risk_label.sh → 35 passed, 0 failed (was 31 assertions; the 404-on-stale-DELETE block is gone, seven new cases added). Sibling suites unaffected: test_grade_pr_risk.sh 57, test_grade_targets.sh 91, test_resolve_enabled.sh 21 — all green (the grade-targets suite copies the real labeler into its stub tooldir, so that integration is exercised too).
shellcheck -x clean over all four scripts and all four test files, exactly as test-pr-risk.yml invokes it.
No changes to grade-pr-risk.sh or grade-targets.sh — the batch loop shells out to the labeler per target and inherits the fix. Confirmed nothing greps the labeler's log lines, so the changed added '…'/removed stale '…' → synced to '…' messages break no caller.

New test coverage: PUT-not-DELETE/POST on the stale-present case; in-sync no-op (rc 0, target on stdout); the race-shape case (empty current set still PUTs, never an additive POST — with a comment on why a POST there reintroduces the interleaving); non-owned preservation (risk-dispute + bug survive a tier change); first-use pre-create ordered before the PUT and carrying the tier color; and PUT-failure → rc 4 with gh's stderr, the PR name, and the UNCHANGED claim.

Judgment calls

I mutation-tested my own tests and found a gap, then closed it. Reverting the PUT to a POST failed 12 assertions and dropping the unowned labels failed 2 — but neutering the in-sync check to the old bare has "$TARGET" failed nothing. That mattered: PRs graded before this lands may be carrying two risk:* labels right now, and a bare target-present check would call them in-sync and never heal them. Added a case asserting a double-labeled PR gets squashed to one (35th assertion; it fails under that mutation). This is a healing property the ticket did not ask for but the fix already had — the test just pins it.
Verified the endpoint rather than trusting memory. The whole change rests on PUT .../issues/{n}/labels existing and replacing the set, so I checked GitHub's REST docs: PUT "Removes any previous labels and sets the new labels"; an empty array removes all (the footgun the ticket flagged — structurally absent here since the array always ends with the target); and POST "adds to existing labels", which independently confirms why the POST hybrid would reproduce the bug.
One out-of-scope-adjacent edit, in pr-risk.yml's pull-requests: write rationale. That block says "SCOPE HONESTLY" and claimed the labeler "touches nothing but the mapped label names". A PUT now sends every label, so I narrowed the wording to "the only state it changes is the mapped label names — its atomic PUT re-sends every unowned label unchanged". Comment-only, and my diff is what made the old sentence imprecise.
Pre-existing limitation I did NOT fix, but which this change amplifies. The read pipes through --jq '.[].name' + split("\n"), so a label name containing a newline was already mangled. Before, the consequence was "a stale label doesn't get removed"; now it is "an unowned label could be dropped or split". The ticket explicitly says to keep the read as-is, and fixing it properly means reworking the read into raw JSON — a larger change than this ticket scopes. Flagging rather than silently widening the diff. (GitHub's label API does not appear to permit newlines in names, so I believe this is theoretical.)

No unmet acceptance criteria — all seven implementation points and all seven test points are done.

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/Comfy-Org/github-workflows/pull/122 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 8/4/2026 **Status:** ✅ Merged **Merged:** 8/4/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-6343-atomic-label-put` --- ### 📝 Commits (3) - [`7b510bf`](https://github.com/Comfy-Org/github-workflows/commit/7b510bf9ed6811cb0c5e00a4c7f6f7a3ab5a4664) fix(pr-risk): make the label sync a single atomic PUT (BE-6343) - [`c90926d`](https://github.com/Comfy-Org/github-workflows/commit/c90926d9158e66f4ea4c1486adb3d42c1ae54a38) fix(pr-risk): harden the atomic label PUT against case variants and a broken filter (BE-6343) - [`f0ef9b4`](https://github.com/Comfy-Org/github-workflows/commit/f0ef9b406b5cf1fcd518063efba75ad0c16bc82f) fix(pr-risk): keep label names inside JSON, and refuse to guess on a jq error (BE-6343) ### 📊 Changes **4 files changed** (+537 additions, -136 deletions) <details> <summary>View changed files</summary> 📝 `.github/workflows/pr-risk.yml` (+24 -9) 📝 `scripts/pr-risk/README.md` (+29 -10) 📝 `scripts/pr-risk/apply-risk-label.sh` (+171 -54) 📝 `scripts/pr-risk/tests/test_apply_risk_label.sh` (+313 -63) </details> ### 📄 Description ## ELI-5 Two grading runs could argue over one PR's risk label and both win. The old code did the label update in three steps — read what's there, delete the wrong one, add the right one — and two runs could slip between each other's steps, leaving the PR wearing two contradictory `risk:*` stickers at once. Now the update is a single request that says "here is the complete final list of stickers", so whoever writes last wins cleanly and the PR always ends up with exactly one. ## What changed `scripts/pr-risk/apply-risk-label.sh` replaces its read / delete-stale / add-target write sequence with one `PUT /repos/{owner}/{repo}/issues/{n}/labels`, which replaces the PR's full label set atomically. The desired set is built with `jq` from the snapshot read: every label the script does not own, carried through verbatim, plus the target. Why this closes the race: a `pr_numbers` batch dispatch run and a `pull_request` event run for one of its numbers sit in **different concurrency groups** (a run belongs to exactly one group; a batch covers N PRs), so no concurrency key can serialize them. With the old shape, A reads `{}`, B reads `{}`, A adds `risk:R1`, B adds `risk:R2` → two contradictory labels, until the next grade. With the PUT there is no delete/add window to interleave into: after **any** single writer's PUT the PR has exactly one owned label, and two concurrent writers end last-writer-wins with still exactly one (possibly the staler tier — acceptable, the label is advisory and the next grade re-syncs it). Zero-label is impossible because every PUT we build contains the target. Deliberately **not** done, per the ticket: no verify-and-retry loop (concurrent writers would just ping-pong, and the invariant already holds by construction), and no POST-when-nothing-is-stale hybrid (two writers that both read an empty owned set would both POST and reproduce the exact bug — every write must be the PUT). Both are called out in an inline comment so a future "optimization" doesn't quietly reopen the race. The DELETE loop is deleted entirely, **including the HTTP-404 tolerance #113 added** — there is no DELETE left for it to guard, and its test block is removed with it. Kept unchanged: input validation, `LABEL_MAP` parsing / `OWNED` derivation, the `DRY_RUN` plan, the `enc()` path helper (still needed by the repo-label probe), the `ghq`/`gherr` error plumbing, the paginated read, and the ensure-label-exists pre-create (a PUT creates the association but never a label's color or description, so first-use enrollment still needs the explicit `POST repos/{r}/labels`). Docs: the script header's ownership-contract paragraph, the `pr-risk.yml` "A BATCH CANNOT SERIALIZE PER-PR" block, and the `scripts/pr-risk/README.md` residual-window section + `apply-risk-label.sh` file entry. The "prefer `pr_number` for a serialized re-grade" guidance is kept — still true, still useful for superseding waits via `cancel-in-progress`. ## The new residual — please read, it is a pilot hazard The PUT is built from a **snapshot** read, so a **non-owned** label added by a human between that read and the PUT is silently dropped — **`risk-dispute` included**. The window is ~one API round-trip and only opens on a run that actually changes the grade (an in-sync PR writes nothing at all), and it is far narrower than the double-label window it replaces, which lasted until the next grade. But disputes are this pilot's whole calibration dataset, and one lost there vanishes without a trace. Documented in all three places above; re-adding the label restores it. ## Verification - `bash scripts/pr-risk/tests/test_apply_risk_label.sh` → **35 passed, 0 failed** (was 31 assertions; the 404-on-stale-DELETE block is gone, seven new cases added). Sibling suites unaffected: `test_grade_pr_risk.sh` 57, `test_grade_targets.sh` 91, `test_resolve_enabled.sh` 21 — all green (the grade-targets suite copies the real labeler into its stub tooldir, so that integration is exercised too). - `shellcheck -x` clean over all four scripts and all four test files, exactly as `test-pr-risk.yml` invokes it. - No changes to `grade-pr-risk.sh` or `grade-targets.sh` — the batch loop shells out to the labeler per target and inherits the fix. Confirmed nothing greps the labeler's log lines, so the changed `added '…'`/`removed stale '…'` → `synced to '…'` messages break no caller. New test coverage: PUT-not-DELETE/POST on the stale-present case; in-sync no-op (rc 0, target on stdout); the **race-shape** case (empty current set still PUTs, never an additive POST — with a comment on why a POST there reintroduces the interleaving); non-owned preservation (`risk-dispute` + `bug` survive a tier change); first-use pre-create ordered before the PUT and carrying the tier color; and PUT-failure → rc 4 with gh's stderr, the PR name, and the UNCHANGED claim. ## Judgment calls 1. **I mutation-tested my own tests and found a gap, then closed it.** Reverting the PUT to a POST failed 12 assertions and dropping the unowned labels failed 2 — but neutering the in-sync check to the *old* bare `has "$TARGET"` failed **nothing**. That mattered: PRs graded before this lands may be carrying two `risk:*` labels **right now**, and a bare target-present check would call them in-sync and never heal them. Added a case asserting a double-labeled PR gets squashed to one (35th assertion; it fails under that mutation). This is a healing property the ticket did not ask for but the fix already had — the test just pins it. 2. **Verified the endpoint rather than trusting memory.** The whole change rests on `PUT .../issues/{n}/labels` existing and replacing the set, so I checked GitHub's REST docs: PUT "Removes any previous labels and sets the new labels"; an empty array removes all (the footgun the ticket flagged — structurally absent here since the array always ends with the target); and POST "adds to existing labels", which independently confirms why the POST hybrid would reproduce the bug. 3. **One out-of-scope-adjacent edit, in `pr-risk.yml`'s `pull-requests: write` rationale.** That block says "SCOPE HONESTLY" and claimed the labeler "touches nothing but the mapped label names". A PUT now *sends* every label, so I narrowed the wording to "the only state it changes is the mapped label names — its atomic PUT re-sends every unowned label unchanged". Comment-only, and my diff is what made the old sentence imprecise. 4. **Pre-existing limitation I did NOT fix, but which this change amplifies.** The read pipes through `--jq '.[].name'` + `split("\n")`, so a label name containing a newline was already mangled. Before, the consequence was "a stale label doesn't get removed"; now it is "an unowned label could be dropped or split". The ticket explicitly says to keep the read as-is, and fixing it properly means reworking the read into raw JSON — a larger change than this ticket scopes. Flagging rather than silently widening the diff. (GitHub's label API does not appear to permit newlines in names, so I believe this is theoretical.) No unmet acceptance criteria — all seven implementation points and all seven test points are done. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-26 22:36:35 +00:00