Skip to content

fix(review): supersede stale exact-review runs#749

Merged
vincentkoc merged 15 commits into
openclaw:mainfrom
snowzlmbot:fix/supersede-stale-exact-reviews
Jul 23, 2026
Merged

fix(review): supersede stale exact-review runs#749
vincentkoc merged 15 commits into
openclaw:mainfrom
snowzlmbot:fix/supersede-stale-exact-reviews

Conversation

@snowzlmbot

@snowzlmbot snowzlmbot commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

Fixes #747.

What Problem This Solves

Resolves a race where a newer pull-request revision or explicit review command could arrive while an older exact review still owned a lease. The stale worker could continue consuming capacity, interfere with the newer review comment, or cause command-specific prompt/status context to be lost.

Why This Change Was Made

The exact-review Durable Object is now the single source-authority owner. Pull-request webhooks reserve a durable monotonic authority sequence before verification; every PR action verifies the live head and retries transient verification failures through an alarm. Newer authoritative revisions revoke stale queue ownership cooperatively, while explicit commands inherit the verified source tuple and queue as follow-up revisions without replacing an active lease. Review-start cleanup is fenced by the full queue tuple and live source revision. Lease loss terminates and drains the complete review process group.

The original contributor direction and authorship are preserved. Maintainer fixes removed unsafe run-id-based cancellation and closed the ordering, redelivery, timestamp, legacy-head, and active-command races found during review.

User Impact

Only the current PR head can publish the authoritative exact review. Rapid synchronize/reopen events and explicit commands are not dropped, stale workers stop promptly, and newer review comments are no longer deleted by older workers. Queue protocol versions and public configuration remain compatible.

Evidence

  • Current head: 9811ed80e370441a06c7f0a323cf9fe54cd02713, rebased on cb5f53f0272eb63c52f15981ffb9139d15afbb46.
  • Focused post-rebase suite: 443/443 passed.
  • Final active-command regression suite: 182/182 passed.
  • Main, repair, and dashboard TypeScript builds passed.
  • Targeted oxfmt, oxlint, and git diff --check passed.
  • Fresh autoreview after the final fix: clean, no accepted/actionable findings.
  • Production dashboard deployment requires CLAWSWEEPER_APP_PRIVATE_KEY; token-only alarm fallback was intentionally not added.

Related incident and state-store follow-up: #738.

@snowzlmbot

Copy link
Copy Markdown
Contributor Author

@clawsweeper review-pr

@clawsweeper

clawsweeper Bot commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

Re-review progress:

@snowzlmbot
snowzlmbot force-pushed the fix/supersede-stale-exact-reviews branch 3 times, most recently from 7f49b84 to 1aecfb4 Compare July 21, 2026 12:13
@snowzlmbot

Copy link
Copy Markdown
Contributor Author

@clawsweeper review-pr

@clawsweeper

clawsweeper Bot commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

Re-review progress:

@snowzlmbot
snowzlmbot force-pushed the fix/supersede-stale-exact-reviews branch 7 times, most recently from bde5b1f to c612047 Compare July 22, 2026 12:22
@snowzlmbot

Copy link
Copy Markdown
Contributor Author

@clawsweeper review-pr

@clawsweeper

clawsweeper Bot commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

Re-review progress:

@vincentkoc vincentkoc left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two P1 blockers:

  1. src/repair/comment-router-core.ts:1956-1998 treats every trusted lease whose SHA differs from the caller SHA as superseded. SHA inequality has no ordering. An older worker that survives best-effort cancellation can reach this cleanup after the newer head has posted its lease, delete the newer lease, break its final in-place update, and allow another stale placeholder. Before deleting a different-head lease, prove the caller still owns the authoritative current queue revision/live PR head. Add the stale-A/newer-B race test.

  2. dashboard/exact-review-queue.ts:842-956,7282-7303 accepts any numeric run_id for the lease capability, persists it, then later obtains an actions:write token and cancels that run without proving it is the matching exact-review sweep.yml run for this item. A misbound or leaked lease can therefore cancel an unrelated workflow run in openclaw/clawsweeper. Fetch and bind the run identity before cancellation (workflow/path, repository-dispatch event, item/head/revision), and test that an unrelated run id is never canceled.

The direction is useful, but cancellation and lease cleanup need authority/ordering proofs before this is safe to merge.

@snowzlmbot
snowzlmbot force-pushed the fix/supersede-stale-exact-reviews branch 2 times, most recently from 6a7a0f0 to 0b45037 Compare July 23, 2026 14:08
@snowzlmbot
snowzlmbot force-pushed the fix/supersede-stale-exact-reviews branch 3 times, most recently from 002dc1c to e2e199e Compare July 23, 2026 14:46
@snowzlmbot

Copy link
Copy Markdown
Contributor Author

@clawsweeper review-pr

@clawsweeper

clawsweeper Bot commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: when the review finishes, ClawSweeper will create the durable review comment if needed or update the existing comment in place.

Re-review progress:

@snowzlmbot
snowzlmbot requested a review from vincentkoc July 23, 2026 15:15
@clawsweeper

clawsweeper Bot commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

ClawSweeper status: review started.

I am starting a fresh review of this pull request: fix(review): supersede stale exact-review runs This is item 1/1 in the current shard. Shard 0/1.

This placeholder means the worker is alive and reading the current context. I will edit this same comment with the actual review when the claws are done clicking.

Crustacean status: shell secured, claws on keyboard, evidence pebbles being sorted.

@vincentkoc
vincentkoc force-pushed the fix/supersede-stale-exact-reviews branch from e2e199e to 9811ed8 Compare July 23, 2026 15:32
@vincentkoc

Copy link
Copy Markdown
Member

Maintainer repair complete on head 9811ed80e370441a06c7f0a323cf9fe54cd02713. The two requested-change blockers are resolved: stale A cannot delete newer B, and run-id-based Actions cancellation was removed in favor of durable queue revocation plus full process-group termination on lease loss.

Additional authority races found during exhaustive review are also covered: delayed/opened ordering, same-timestamp sequencing, legacy missing-head takeover, transient verification retry, delivery redelivery idempotency, explicit commands during pending/parked/dispatching/leased states, and issue webhooks staying outside PR completion.

Proof: 443/443 post-rebase focused tests; 182/182 final authority tests; three TypeScript builds; targeted format/lint/diff checks; fresh autoreview clean. Current CI is running on the exact contributor head.

The remaining token-only alarm concern was rejected because production dashboard deployment requires the GitHub App private key; adding a second unsupported production credential path would widen the contract.

@vincentkoc
vincentkoc dismissed their stale review July 23, 2026 15:33

Resolved on 9811ed8: queue/source authority is durable and run-id cancellation is removed; focused proof and fresh autoreview are clean.

@vincentkoc

Copy link
Copy Markdown
Member

@clawsweeper review-pr

@clawsweeper

clawsweeper Bot commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: when the review finishes, ClawSweeper will create the durable review comment if needed or update the existing comment in place.

Re-review progress:

@clawsweeper

clawsweeper Bot commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

ClawSweeper status: review started.

I am starting a fresh review of this pull request: fix(review): supersede stale exact-review runs This is item 1/1 in the current shard. Shard 0/1.

This placeholder means the worker is alive and reading the current context. I will edit this same comment with the actual review when the claws are done clicking.

Crustacean status: shell secured, claws on keyboard, evidence pebbles being sorted.

@clawsweeper

clawsweeper Bot commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

ClawSweeper status: review started.

I am starting a fresh review of this pull request: fix(review): supersede stale exact-review runs This is item 1/1 in the current shard. Shard 0/1.

This placeholder means the worker is alive and reading the current context. I will edit this same comment with the actual review when the claws are done clicking.

Crustacean status: shell secured, claws on keyboard, evidence pebbles being sorted.

@vincentkoc

Copy link
Copy Markdown
Member

Final-head closeout for 960a2079b945482ed47ae4b074a8b4031cd205a1:

Rank-up handling:

  • The workflow/queue owner contract is confirmed: queue revision, claim generation, source head, run attempt, and lease tuple remain the authority; GitHub Actions cancellation by supplied run id is gone.
  • The requested rapid-revision and explicit-command cases are covered by focused tests, including stale-worker revocation, active command follow-up preservation, and stale cleanup not deleting the newer review lease/comment.
  • A true production Durable Object race trace cannot be generated before deploying this branch. I am explicitly accepting that remaining automation risk as a monitored rollout condition, with immediate post-merge queue/comment verification. This is preferable to inventing a non-production proof or temporarily deploying contributor code outside the normal main deployment path.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

perf(review): superseded exact-review runs keep consuming Actions and leave stale placeholders

2 participants