Fix stale execution-lock ownership and serialized verification (BLO-21557) - #1054
Open
kkroo wants to merge 27 commits into
Open
Fix stale execution-lock ownership and serialized verification (BLO-21557)#1054kkroo wants to merge 27 commits into
kkroo wants to merge 27 commits into
Conversation
…BLO-20321)
`queued` and `scheduled_retry` runs are non-terminal, so the terminal-only
staleness test treated a run that had never executed as a live owner. The
assignee's own PATCH/release on its own issue was answered `409 Issue run
ownership conflict`.
That made WIP monotonic. Checkout adds WIP without holding a lock; parking or
closing removes WIP and needs the lock — and the lock was held by the very queue
backlog being drained. The deeper the queue, the slower each issue's run
arrives, so the drain rate falls as the backlog grows. Measured on the CTO
agent: 47 of 59 in_progress issues were immutable to their own assignee.
Adds isReapableHeartbeatRunRow as the single source of truth for "is this lock
owner reapable": terminal OR missing OR (queued/scheduled_retry AND
startedAt IS NULL). Gating on startedAt as well as status means a run that has
actually begun is never reaped, so the race protection the guard exists for is
preserved.
Applied at the two guards that decide the assignee's own write:
- adoptStaleCheckoutRun — runs FIRST when checkoutRunId is set, and a
non-stale verdict there throws the 409 before the other is consulted, so
fixing only the second would have left the fix unreachable for the common
shape (both locks pointing at one never-started run).
- clearStaleExecutionLock — backs assertCheckoutOwner (PATCH) and checkout.
- release's own copy of the test.
Both adoption paths already call cancelStaleIssueContextRuns, so the superseded
run is cancelled by the write and cannot start later against a status the
assignee has since changed.
Scope: the change sits strictly downstream of authorization —
assertAgentIssueMutationAllowed runs decideIssueAccess and the
assignee-identity check before ever calling assertCheckoutOwner — so the authz
boundary is untouched and a non-assignee without a grant is still refused.
checkout's startedAt-preserving adoption branch is deliberately left
terminal-only; see the comment there.
Tests: six cases in issue-stale-execution-lock-routes.test.ts. The four
behavioural ones fail on master with the exact reported 409; the two protection
cases (live `running` owner still 409s, peer still refused) pass on master, so
they are genuine regression guards.
Co-Authored-By: Claude <noreply@anthropic.com>
…20260801 # Conflicts: # server/src/routes/issues.ts
Co-Authored-By: Paperclip <noreply@paperclip.ing>
Co-Authored-By: Paperclip <noreply@paperclip.ing>
Adoption of an issue's checkout lock is authorised by EITHER limb of
if ((!stale && !sameAgentRetry) || !actorLive) return { adopted: null, ... }
but the reap gate this branch added below it (d2a0cf7, 43ecd77) was written
as an unconditional postcondition, which makes it an implicit conjunction over
both limbs:
if (!(await cancelNeverStartedOwnerRun(tx, existingRun, cancellation)) || ...)
cancelNeverStartedOwnerRun returns false for any run that is not reapable. A
same-agent retry adopts from its own parent run, which is still `running` and
therefore never reapable, so the gate returned false and the retry was answered
409 Issue run ownership conflict. The sameAgentRetry limb became dead code,
silently regressing BLO-6869 (0ba33fa).
Gate the reap on `stale` so it is demanded only where it is meaningful.
Staleness-authorised adoption still MUST reap, so a never-started owner cannot
start later against a state the adopter has since changed; retry-authorised
adoption supersedes a live parent, which is exactly what master does today.
Race protection is untouched: a divergent live execution owner is still refused
above by executionOwnerIsAllowed.
Caught by Verify serialized server suites (1/4) — the first execution of that
job on this branch. issues-service.test.ts is only in the serialized shard, and
verify_serialized_server is needs: [policy, general_tests] with no
`if: always()`, so every earlier red general_tests skipped it and the defect sat
across three heads for ~4.5h behind 16 green checks.
Tests: issues-service.test.ts + issue-stale-execution-lock-routes.test.ts,
--no-file-parallelism --maxWorkers=1: 205/205 pass, EXIT=0. The BLO-6869 case
fails without this change with the exact reported 409.
Co-Authored-By: Claude <noreply@anthropic.com>
Co-authored-by: Staff Engineer <staff-engineer@paperclip.local>
…r911-reviewed # Conflicts: # server/src/__tests__/issue-stale-execution-lock-routes.test.ts
|
🔗 Paperclip issue: BLO-21557 |
1 similar comment
|
🔗 Paperclip issue: BLO-21557 |
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
Hey @kkroo! Before this PR can be reviewed, a few things need attention: Missing or incomplete:
Once updated, push a new commit and these checks will re-run automatically. — commitperclip |
![allyblockcast[bot]](https://avatars.githubusercontent.com/u/57363972?s=60&v=4){kind=small}
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: d2a6b82
Critical Issues (0)
Important Issues (1)
- [pr-review-toolkit + native-codex]
server/src/routes/issues.ts:9822— The new execution-state/policy compare-and-swap is applied only whentransition.decisionis present, but this route also derives and writesexecutionStatefrom the earlierexistingsnapshot forreviewRequestupdates. If request A prepares areviewRequestpatch, request B commits the current participant's decision, and then A enterssvc.update, A takes the non-decision branch without either expected-state predicate and can overwrite B's completed/advanced state with the stale pending stage. This can resurrect an already-decided stage or leave status and execution state inconsistent.- Apply the expected execution-state/policy predicates whenever the route writes a derived
executionState, not only when it records a decision, and add a concurrentreviewRequestversus approval regression covering both commit orders.
- Apply the expected execution-state/policy predicates whenever the route writes a derived
Suggestions (1)
- [CI] The
reviewcheck currently fails because the PR description omits the repository's required Thinking Path, What Changed, Verification, Risks, Model Used, and dedup-search sections. Update the description so the quality gate can pass.
Strengths
- Never-started owner cancellation and ownership transfer occur under the same issue/run row locks, making dispatch claim and stale-owner reaping mutually exclusive.
- Divergent live checkout and execution owners remain fail-closed, while independently reapable owners can be recovered transactionally.
- Release validates both ownership columns for agent and board callers and cancels exact queued owners before clearing the issue.
- Decision-bearing updates atomically pin both execution state and policy and persist the decision row in the same transaction.
Recommended Action
- Extend the execution-state CAS to non-decision derived state writes before merge.
- Update the PR description to satisfy the required quality gate.
|
@ally please review exact head |
|
@ally please re-review exact head |
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 69b6818
Prior Findings Dispositioned (1)
- prior:d2a6b82 important 1 — fixed —
server/src/routes/issues.ts:9811— Every derivedexecutionStatewrite now carries the route snapshot's execution-state and execution-policy preconditions;server/src/__tests__/issue-stale-execution-lock-routes.test.ts:1773verifies that either a review request or a decision wins and the stale writer receives 409.
Critical Issues (0)
Important Issues (1)
- [gstack/review + native-codex]
server/src/routes/issues.ts:9811— The new snapshot predicate still omitsstatus, even though the execution-participant exception is authorized only from anin_reviewsnapshot atserver/src/routes/issues.ts:4747. A concurrent release can move the issue totodoand clear ownership without changingexecutionStateorexecutionPolicy; the stale participant decision or review-request write then still satisfies both JSON predicates and can overwrite the newer status or attach review state to a released issue.- Include
expectedCurrentStatus: existing.statuswhenever the route writes a derivedexecutionState, and add release-vs-decision and release-vs-reviewRequest regressions covering both commit orders.
- Include
Strengths
- The prior review-request-versus-decision race is now serialized at the database write, with both commit orders covered.
- Execution state and policy are compared in the final
UPDATEpredicate rather than only before the transaction blocks. - The stale-owner tests exercise divergent owners, live-owner refusal, and never-started-run cancellation across checkout, patch, and release paths.
Recommended Action
- Pin the status snapshot for execution-state-derived writes and cover concurrent release before merge.
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 35ac42f
Prior Findings Dispositioned (1)
- prior:69b6818 important 1 — fixed —
server/src/routes/issues.ts:9814— Every derivedexecutionStatewrite now pins the authorizing status snapshot, andserver/src/__tests__/issue-stale-execution-lock-routes.test.ts:1908verifies that release winning first rejects both stale decision and stale review-request writes.
Critical Issues (0)
Important Issues (2)
- [native-codex]
server/src/services/issues.ts:9634— Stale-lock checkout can overwrite a concurrent reassignment.clearStaleExecutionLock()commits and releases the issue row lock before this retry; if another request assigns the still-eligible issue to agent B in that gap, the retry pins status and run IDs but notassigneeAgentId, then writes agent A over B's newer assignment.- Keep stale-lock clearing and adoption in one transaction, or include the originally observed assignee in the retry CAS predicate. Add a regression that pauses between clear and retry while another writer reassigns the issue.
- [native-codex]
server/src/routes/issues.ts:9811— Run ownership is checked only during route authorization, not at the final write. A request from run A can passisCurrentIssueExecutionRun; after a force-release and checkout transfers both ownership pointers to run B without changing execution state/policy, A's ordinary patch has no snapshot preconditions at all, while an execution-state patch pins only status/state/policy. The superseded run can therefore mutate the newly owned issue.- Carry the authorized
checkoutRunIdandexecutionRunIdsnapshots intosvc.update()and include both in the final SQL predicate, or re-authorize under an issue-row lock. Cover ownership transfer between authorization and write for both ordinary and execution-state patches.
- Carry the authorized
Strengths
- The prior release-versus-derived-state race is now closed with status, execution-state, and execution-policy predicates in the final SQL update.
- Owner run rows are locked in deterministic order, and never-started owner cancellation uses guarded writes.
- The expanded concurrency suite covers divergent live owners, stale decision writers, dispatch races, and both release ordering outcomes.
Recommended Action
- Close the stale-clear/reassignment window before merge.
- Pin authorization-relevant run ownership through the final issue update.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Thinking Path
Linked Issues or Issue Description
stale execution,execution lock, andBLO-21557 OR BLO-20321 OR BLO-20629 OR BLO-21607; related results include fix(issues): a never-started run must not lock out its own assignee (BLO-20321) #911, fix(issues): validate owners on non-active release (BLO-21607) #1005, fix(recovery): stop judging an adopted issue on a foreign run, and re-read lock ids across the handover (BLO-19160) #960, and fix(recovery): re-dispatch infra-class stranded-issue failures to the existing assignee #972. None duplicates this independent-author landing PR.What Changed
kkroo.in_progressrelease ownership validation from fix(issues): validate owners on non-active release (BLO-21607) #1005 /9205faeef39bc4573d117f976a5cfdd9b0e2cc98.reviewRequestwrites so stale requests cannot resurrect a completed stage.69b6818f6731b4491f6c1cc63ceb8831ee02f51d.Verification
pnpm exec vitest run server/src/__tests__/issue-execution-policy-routes.test.ts -t "pins the execution snapshot": 1 passed.pnpm exec vitest run server/src/__tests__/issue-stale-execution-lock-routes.test.ts -t "stale writer" --no-file-parallelism --maxWorkers=1: 2 passed, covering both commit orders.pnpm --filter @paperclipai/server typecheck: passed.e2a06589ab4e80d7f59b439a2fefe33a7192380aand release fix9205faeef39bc4573d117f976a5cfdd9b0e2cc98are ancestors of the PR head.Risks
master; the resolution retained both the reviewed ownership coverage and newer stale-sweeper coverage.Model Used
claude-opus-4-5), 1M context, extended thinking, with tool use.openai/gpt-5.6-sol) with tool use and code execution.Checklist