fix(pr): prevent same-PR operations from overlapping#103669
Conversation
openclaw-barnacle
Bot
added
scripts
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8e90eaeebb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const postExitGroupStatus = child.pid ? processGroupStatus(child.pid) : "dead"; | ||
| if (postExitGroupStatus === "indeterminate") { | ||
| notificationFailure ??= new Error("scripts/pr process-group state became indeterminate"); | ||
| } else if (postExitGroupStatus === "live") { | ||
| // A wrapper exit does not end same-group background work. Bound and drain | ||
| // forgotten jobs, but keep the lock because their terminal state is unknown. | ||
| hadLingeringGroup = true; | ||
| notificationFailure ??= new Error("scripts/pr process group remained active after wrapper exit"); | ||
| signalProcessGroup("SIGTERM"); |
There was a problem hiding this comment.
When the PR command's process group has already exited, a high-PID-churn host can reuse child.pid as a new process-group id before this post-exit probe runs. In that case processGroupStatus(child.pid) reports live, and the branch below sends SIGTERM/SIGKILL to that unrelated process group and retains the PR lock even though the operation completed; the shell lock owner checks supervisor birth to avoid this kind of reuse, but the supervisor's own group-drain path does not.
Useful? React with 👍 / 👎.
|
Land-ready proof for exact head
Known proof gaps: none. |
|
Merged via squash.
|
* fix(pr): serialize operations per pull request * fix(pr): tighten supervisor error handling
1 participant
Closes #103666
What Problem This Solves
Fixes an issue where maintainers running
scripts/prcommands for the same pull request could overlap and concurrently mutate the shared worktree and review, prepare, or merge evidence.Why This Change Was Made
State-changing review, prepare, and merge commands now serialize through a per-PR Git ref shared by every linked worktree. A process-group supervisor releases the exact owner only after clean synchronous completion; interrupted, failed, controller-lost, or otherwise indeterminate operations stay locked for explicit exact-OID recovery, and garbage collection skips active or uncertain owners.
User Impact
Maintainers can run operations for different pull requests concurrently while same-PR operations queue safely. A crashed operation no longer silently exposes its worktree to another landing command or garbage collection.
Evidence
tbx_01kx5xbmvm9hb939tx3hcd8hrc:node scripts/run-vitest.mjs test/scripts/pr-operation-lock.test.ts test/scripts/test-projects.test.ts test/scripts/pr-wrappers.test.ts— 3 files, 241 tests passed.bash -nfor the touched shell entry points,node --check scripts/pr-lib/process-group-runner.mjs,git diff --check, and the exact scripts oxlint lane passed with zero warnings/errors./varcanonicalization, symlink aliases, and GC cleanup postconditions; no remaining findings.8e90eaeebb7reported no accepted/actionable findings (0.91 confidence); a second fresh review of follow-upb16d4d41df0was also clean.AI-assisted: implemented and reviewed with Codex. I understand the locking, supervision, recovery, and cleanup paths in this change.