fix(state): recover non-reconcile canonical tuple 409 conflicts with rebase-or-skip - #965
Merged
Merged
Conversation
…rebase-or-skip Apply-lane and comment-sync publications posted canonical record tuples without any conflict recovery, so the first 409 from a concurrent review worker killed the entire run mid-checkpoint. Non-reconcile conflicts now absorb equivalent concurrent writes, rebase unrelated-section races onto CURRENT with a single deterministic record-tuple-rebase retry, and skip same-section races per item (syncing CURRENT to the working root) while the rest of the plan continues; all-items-conflicted still fails the run and infrastructure errors still throw immediately. Reconcile-lane recovery is unchanged.
clawsweeper
Bot
added
rating: 🦪 silver shellfish
Contributor
|
Codex review: needs real behavior proof before merge. Reviewed July 30, 2026, 12:01 PM ET / 16:01 UTC. ClawSweeper reviewWhat this changesThis PR makes apply and comment-sync publication recover from canonical record-tuple 409 conflicts by accepting equivalent writes, rebasing one unrelated-section race, and skipping same-section conflicts while sibling items continue. Merge readiness⛔ Blocked until real behavior proof from a real setup is added - 5 items remain Keep open: this fresh reliability PR addresses a concrete canonical-state publication failure and is not obsolete on current main. The conflict strategy is narrowly scoped and test-covered, but it needs real after-fix behavior proof before merge and should not modify the release-owned changelog. Priority: P2 Review scores
Verification
How this fits togetherApply and comment-sync workflows publish review records to the Worker-backed canonical state store after GitHub-side work has occurred. This publisher must reconcile concurrent review updates without overwriting authoritative sections or stopping an entire checkpoint for one conflicted item. flowchart LR
A[Apply or comment-sync run] --> B[Canonical tuple publisher]
B --> C[Worker canonical state]
C --> D{409 conflict?}
D -->|No| E[Publish tuple]
D -->|Equivalent target| F[Sync current state]
D -->|Unrelated section| G[Rebase and retry once]
D -->|Same section| H[Skip item and continue]
E --> I[Checkpoint outcome]
F --> I
G --> I
H --> I
Before merge
Findings
Agent review detailsSecurityNone. Review metrics
Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Keep the bounded no-force-write recovery, remove the release-owned changelog edit, and add redacted after-fix proof showing a real apply or comment-sync checkpoint absorbs a tuple conflict while unaffected items complete. Do we have a high-confidence way to reproduce the issue? Yes at source level: the added focused tests construct the current non-reconcile 409 path and cover each intended recovery outcome. A live after-fix apply or comment-sync run has not been supplied, so runtime confirmation remains needed. Is this the best way to solve the issue? Mostly yes: the bounded equivalent/rebase-or-skip path avoids force-writing canonical state and retains an all-skipped failure guard. The release-owned changelog edit is not part of the correct solution and should be removed. Full review comments:
Overall correctness: patch is correct AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against d91cefcabab2. LabelsLabel changes:
Label justifications:
EvidenceWhat I checked:
Likely related people:
Rank-up movesOptional improvements that raise the rating; they are not merge blockers.
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
|
Contributor
|
ClawSweeper status: review started. I am starting a fresh review of this pull request: fix(state): recover non-reconcile canonical tuple 409 conflicts with rebase-or-skip 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. |
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
1 participant
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.
Problem
Comment-sync and apply runs crash with
CanonicalRecordTupleConflictError: POST /internal/state/records/tuples returned 409: canonical_record_tuple_conflict(sample run 30555649490, step "Apply unchanged proposed decisions with checkpoints").publishMainWithStateAppendposts canonical record tuples per item, butpostCanonicalRecordTupleWithRecoveryonly recovers 409 conflicts whenCLAWSWEEPER_CANONICAL_PUBLICATION_KIND === "reconcile"(set only byscripts/apply-workflow-helpers.sh). Apply-lane and comment-sync publications run without that kind, so the first conflict kills the entire run mid-checkpoint. With ~70 concurrent review workers constantly bumping record revisions in the ExactReviewQueue DO, these races are now routine — the lane fails continuously.Fix
Non-reconcile conflicts now go through a dedicated recovery path in
src/repair/publish-main.ts(recoverNonReconcilePublicationConflict) that reuses the existing reconcile helpers and never force-writes:expectedDigestthe mutation asserted.expectedDigest, our content for sections we changed, CURRENT's content for the rest — under a deterministicrecord-tuple-rebase:<run>:<attempt>:<contentHash>delivery id (mirrors therecord-reconcile:retry pattern in a separate non-reconcile namespace). The rebased tuple is validated before posting; an invalid combination skips instead.isCanonicalInfrastructureError) still throw immediately.A skipped item may repeat its GitHub action next cycle since side effects happen before publication (capture-before-mutate); comment idempotency fences and the unchanged-edit dedupe (#857) make that repeat a no-op, and the code comments call out that those fences must not be weakened. Reconcile behavior is preserved exactly — its recovery block is unchanged aside from returning an explicit outcome.
Proof
Four new tests in
test/repair/publish-main.test.ts, each proven red against the origin/main implementation (all four fail pre-fix: the old code rethrows the 409 for every non-reconcile conflict):record-tuple-rebase:delivery idFull gate:
pnpm run build:all, focused suite 15/15,pnpm run format,pnpm run lint,pnpm run checkgreen, autoreview clean.