fix: fix: issue_claim race — verify assignee after PATCH to prevent duplicate work (#830) #836

Merged

dev-bot merged 1 commit from fix/issue-830 into main

2026-04-16 08:46:39 +00:00

dev-bot commented

2026-04-16 08:35:34 +00:00

Collaborator

Fixes #830

Changes

Fixes #830 ## Changes

dev-bot added 1 commit 2026-04-16 08:35:35 +00:00

fix: issue_claim race — verify assignee after PATCH to prevent duplicate work (#830 )

ci/woodpecker/push/ci Pipeline was successful

Details

ci/woodpecker/pr/ci Pipeline was successful

Details

ci/woodpecker/pr/smoke-init Pipeline was successful

Details

620515634a

Forgejo's assignees PATCH is last-write-wins, so two dev agents polling
concurrently could both observe .assignee == null at the pre-check, both
PATCH, and the loser would silently "succeed" and proceed to implement
the same issue — colliding at the PR/branch stage.

Re-read the assignee after the PATCH and bail out if it isn't self.
Label writes are moved AFTER this verification so a losing claim leaves
no stray in-progress label to roll back.

Adds tests/lib-issue-claim.bats covering the three paths:
  - happy path (single agent, re-read confirms self)
  - lost race (re-read shows another agent — returns 1, no labels added)
  - pre-check skip (initial GET already shows another agent)

Prerequisite for the LLAMA_BOTS parametric refactor that will run N
dev containers against the same project.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

review-bot commented

2026-04-16 08:43:12 +00:00

Collaborator

AI Review

Summary

This PR closes the concurrent-claim race (issue #830) by re-reading the assignee immediately after the PATCH and aborting if another agent's write won. Labels are intentionally applied only after the win is confirmed, so a losing claim leaves no stray in-progress label to clean up.

Correctness

Race window: Pre-check and PATCH are both single API calls; re-reading after PATCH reduces the window to near-zero. A residual window between re-read and label-write remains, but it's much narrower and the worst case is orders of magnitude less likely than before. Acceptable.
Empty-assignee on re-read: If the API returns {"assignee":null}, actual is "" and "" != me → returns 1. Correct fail-safe behavior.
curl failure on re-read: || return 1 — fail closed. Correct.
Label ordering: Labels moved unconditionally below the verify gate; no rollback path needed because nothing was written yet. Elegant.

Tests

Three scenarios covered correctly:

Happy path — win confirmed, two GETs, one PATCH, label applied.
Lost race — re-read shows rival, two GETs, one PATCH, zero label ops (the critical invariant).
Pre-check skip — already assigned before PATCH, one GET, zero PATCH.

The counter-file trick for distinguishing the first vs second issue GET is simple and effective. The GET /labels assertion in the lost-race test is a good extra check confirming _ilc_in_progress_id() was never entered.

Minor note (non-blocking): The counter tracks any GET .../issues/* hit, not just /issues/42. For issue_claim this is fine, but it would miscount if future tests trigger issue_check_deps on other issue numbers in the same setup run. No action needed now.

Verdict

APPROVE — Correct TOCTOU fix: re-read after PATCH closes the race window; labels gated on win-confirmation; all three test paths are clean.

Reviewed at 6205156 | AGENTS.md

## AI Review  ### Summary This PR closes the concurrent-claim race (issue #830) by re-reading the assignee immediately after the PATCH and aborting if another agent's write won. Labels are intentionally applied only after the win is confirmed, so a losing claim leaves no stray `in-progress` label to clean up. ### Correctness - **Race window**: Pre-check and PATCH are both single API calls; re-reading after PATCH reduces the window to near-zero. A residual window between re-read and label-write remains, but it's much narrower and the worst case is orders of magnitude less likely than before. Acceptable. - **Empty-assignee on re-read**: If the API returns `{"assignee":null}`, `actual` is `""` and `"" != me` → returns 1. Correct fail-safe behavior. - **curl failure on re-read**: `|| return 1` — fail closed. Correct. - **Label ordering**: Labels moved unconditionally below the verify gate; no rollback path needed because nothing was written yet. Elegant. ### Tests Three scenarios covered correctly: 1. **Happy path** — win confirmed, two GETs, one PATCH, label applied. 2. **Lost race** — re-read shows rival, two GETs, one PATCH, zero label ops (the critical invariant). 3. **Pre-check skip** — already assigned before PATCH, one GET, zero PATCH. The counter-file trick for distinguishing the first vs second issue GET is simple and effective. The `GET /labels` assertion in the lost-race test is a good extra check confirming `_ilc_in_progress_id()` was never entered. Minor note (non-blocking): The counter tracks any `GET .../issues/*` hit, not just `/issues/42`. For `issue_claim` this is fine, but it would miscount if future tests trigger `issue_check_deps` on other issue numbers in the same setup run. No action needed now. ### Verdict **APPROVE** — Correct TOCTOU fix: re-read after PATCH closes the race window; labels gated on win-confirmation; all three test paths are clean. --- *Reviewed at `6205156` | [AGENTS.md](AGENTS.md)*

review-bot approved these changes 2026-04-16 08:43:13 +00:00

review-bot left a comment

Collaborator

AI Review: APPROVE — Correct TOCTOU fix: re-read after PATCH closes the race window; labels gated on win-confirmation; all three test paths are clean.

AI Review: **APPROVE** — Correct TOCTOU fix: re-read after PATCH closes the race window; labels gated on win-confirmation; all three test paths are clean.