pulls savePR CAS-retry can clobber a landed merge outcome #64

New issue

Closed

opened 2026-09-04 22:39:40 +00:00 by crueber · 3 comments

crueber commented

2026-09-04 22:39:40 +00:00

Owner

Follow-up from the backend audit on issue #59 (origin/main @940ca8c).

Evidence: internal/pulls/service.go:131-162. On 412 the retry reloads cur, then does wholesale *cur = *p (lines 149-150) and re-saves. *p is the writer's stale struct. If a merge task lands Merged/MergeCommitSHA/MergedAt/MergedBy between the writer's load and its retry (merge writes via loadPR+savePR at internal/pulls/merge.go:285-286), the retry overwrites those fields back to unmerged.

Impact: pr.json says unmerged although the base ref advanced and the merge task returned ok; thread has the merged event but the sidecar disagrees. Window needs a concurrent UpdatePR body/title write racing a merge; narrow but real and unrecoverable (no repair rewrites it).

Suggested fix: field-group merge on retry (preserve Merged* outcome fields from cur), or make UpdatePR re-apply only its field group onto the fresh doc.

Follow-up from the backend audit on issue #59 (origin/main @940ca8c). Evidence: internal/pulls/service.go:131-162. On 412 the retry reloads cur, then does wholesale *cur = *p (lines 149-150) and re-saves. *p is the writer's stale struct. If a merge task lands Merged/MergeCommitSHA/MergedAt/MergedBy between the writer's load and its retry (merge writes via loadPR+savePR at internal/pulls/merge.go:285-286), the retry overwrites those fields back to unmerged. Impact: pr.json says unmerged although the base ref advanced and the merge task returned ok; thread has the merged event but the sidecar disagrees. Window needs a concurrent UpdatePR body/title write racing a merge; narrow but real and unrecoverable (no repair rewrites it). Suggested fix: field-group merge on retry (preserve Merged* outcome fields from cur), or make UpdatePR re-apply only its field group onto the fresh doc.

crueber referenced this issue from a commit

2026-09-04 23:04:42 +00:00

Fix #64: pr.json CAS retry field-merge, merge outcome write-once

crueber referenced this issue from a pull request that will close it,

2026-09-04 23:04:43 +00:00

Fix #64: pr.json CAS retry field-merge #67

crueber commented

2026-09-04 23:04:46 +00:00

Author

Owner

Fixed by #67 (#67): pr.json CAS retries now field-merge with a write-once guard on the merge outcome, and all three writers re-apply only their owned delta onto the fresh doc. Regression tests fail pre-fix / pass post-fix; pulls suite -race green, cover 97.7%.

Fixed by #67 (https://git.packden.us/crueber/walhub/pulls/67): pr.json CAS retries now field-merge with a write-once guard on the merge outcome, and all three writers re-apply only their owned delta onto the fresh doc. Regression tests fail pre-fix / pass post-fix; pulls suite -race green, cover 97.7%.

crueber commented

2026-09-04 23:08:37 +00:00

Author

Owner

Review of PR #67 (fix/issue-64, pr.json CAS retry field-merge) — data-integrity pass, verified in scratch worktree at origin/fix/issue-64 (main worktree untouched).

OWNED-DELTA DISJOINTNESS — PASS. Post-change pr.json writers enumerated (non-test savePR call sites): (1) Open/Create via direct PutCreate, service.go:~421-451; (2) UpdatePR owns Body only, service.go:949-965 (reloads fresh doc first); (3) refreshHead owns Head.SHA/HeadForcePushedAt only, mergeable.go:193-216 (reloads fresh doc first); (4) runMerge owns the 5 outcome fields only, merge.go:284-294 (reloads fresh doc first); (5) savePR CAS retry delegates to reapplyPR field-merge, service.go:150. No remaining wholesale stale-struct save: the only '*cur = *p' left is inside reapplyPR behind the monotonic guards. HeadPublished/Draft/Fork/Base are Create-only (no post-open writer found) and reapply case-2 preserves them from the fresh doc — safe either way.

WRITE-ONCE GUARD, FIELD-BY-FIELD — PASS. PRDoc outcome group = Merged, MergedAt, MergedBy, MergeCommitSHA, MergeStrategy. reapplyPR case cur.Merged restores all 5 atomically from fresh (service.go:177-179), so merged_at cannot be clobbered while merged stays true — no partial-outcome state reachable. No path unsets Merged: direct writers only set true (merge.go:289) or don't touch it; reapply never clears a landed true. Symmetric case p.Merged keeps fresh Body/Head/HeadForcePushedAt/HeadPublished/Draft/Fork/Base (service.go:183-187). Num/Kind/Version identical or caller-managed.

UNMERGED-vs-UNMERGED LWW — ACCEPTABLE. Bodies are human-rate; head sha self-heals via live drift detection on next read. Documented in code comment + doc.

EXTRA GET ON BODY-EDIT — ACCEPTABLE. One conditional loadPR in UpdatePR (service.go:953), human-rate path, not a hot/budgeted path (push/refs/checkpoint budgets untouched; no LIST added anywhere).

BOUNDED RETRIES — PRESERVED. attempt<5 loop untouched (service.go:138); persistent 412s still surface ErrConflict.

MERGE SUCCESS PATH — UNCHANGED. Existing tests unmodified (diff touches only doc + 3 source files + new prcas_test.go); full suite green (see below). Bonus: merge.go:295 'pr = target' feeds the FRESH body into closing-keyword texts — strictly better.

NEGATIVE CONTROLS — GENUINE (verified on base via checkout of the 3 files in scratch, then restored byte-identical): TestSavePRRetryPreservesLandedMerge FAILS pre-fix with 'merge outcome clobbered ... Merged:false' (stale head-writer retry); TestSavePRMergeRetryPreservesFreshFields FAILS with 'body edit clobbered' (stale merge retry); TestRefreshHeadPreservesLandedMerge FAILS with 'Merged:false' (fresh-version direct write, no 412 at all — the worse variant). TestSavePRRetryLastWriterWinsWhenUnmerged + TestMergeOutcomeMonotonicUnderChurn pass both sides BY DESIGN (LWW preservation + post-merge-fresh writers).

RESULTS (scratch /tmp/pr67 @ ddc130b): gofmt clean; go vet clean; no new imports; go.mod untouched; go test -race ./internal/pulls/... PASS (10/11 full runs; 1 transient TestGetPRHeadDrift stream-assertion failure on first run, unreproducible since — passes isolated, -count=3, 6x consecutive full runs, and on base; stream/forced logic untouched by this diff, looks pre-existing); coverage 97.7% statements (gate >=95%).

DOCS — ACCURATE. §2.3 paragraph + Decisions entry for #64 match the code (disjoint sets named correctly, write-once semantics, rationale outcome-not-rederivable vs head self-heals).

MERGE RECOMMENDATION: ready to merge.

Review of PR #67 (fix/issue-64, pr.json CAS retry field-merge) — data-integrity pass, verified in scratch worktree at origin/fix/issue-64 (main worktree untouched). OWNED-DELTA DISJOINTNESS — PASS. Post-change pr.json writers enumerated (non-test savePR call sites): (1) Open/Create via direct PutCreate, service.go:~421-451; (2) UpdatePR owns Body only, service.go:949-965 (reloads fresh doc first); (3) refreshHead owns Head.SHA/HeadForcePushedAt only, mergeable.go:193-216 (reloads fresh doc first); (4) runMerge owns the 5 outcome fields only, merge.go:284-294 (reloads fresh doc first); (5) savePR CAS retry delegates to reapplyPR field-merge, service.go:150. No remaining wholesale stale-struct save: the only '*cur = *p' left is inside reapplyPR behind the monotonic guards. HeadPublished/Draft/Fork/Base are Create-only (no post-open writer found) and reapply case-2 preserves them from the fresh doc — safe either way. WRITE-ONCE GUARD, FIELD-BY-FIELD — PASS. PRDoc outcome group = Merged, MergedAt, MergedBy, MergeCommitSHA, MergeStrategy. reapplyPR case cur.Merged restores all 5 atomically from fresh (service.go:177-179), so merged_at cannot be clobbered while merged stays true — no partial-outcome state reachable. No path unsets Merged: direct writers only set true (merge.go:289) or don't touch it; reapply never clears a landed true. Symmetric case p.Merged keeps fresh Body/Head/HeadForcePushedAt/HeadPublished/Draft/Fork/Base (service.go:183-187). Num/Kind/Version identical or caller-managed. UNMERGED-vs-UNMERGED LWW — ACCEPTABLE. Bodies are human-rate; head sha self-heals via live drift detection on next read. Documented in code comment + doc. EXTRA GET ON BODY-EDIT — ACCEPTABLE. One conditional loadPR in UpdatePR (service.go:953), human-rate path, not a hot/budgeted path (push/refs/checkpoint budgets untouched; no LIST added anywhere). BOUNDED RETRIES — PRESERVED. attempt<5 loop untouched (service.go:138); persistent 412s still surface ErrConflict. MERGE SUCCESS PATH — UNCHANGED. Existing tests unmodified (diff touches only doc + 3 source files + new prcas_test.go); full suite green (see below). Bonus: merge.go:295 'pr = target' feeds the FRESH body into closing-keyword texts — strictly better. NEGATIVE CONTROLS — GENUINE (verified on base via checkout of the 3 files in scratch, then restored byte-identical): TestSavePRRetryPreservesLandedMerge FAILS pre-fix with 'merge outcome clobbered ... Merged:false' (stale head-writer retry); TestSavePRMergeRetryPreservesFreshFields FAILS with 'body edit clobbered' (stale merge retry); TestRefreshHeadPreservesLandedMerge FAILS with 'Merged:false' (fresh-version direct write, no 412 at all — the worse variant). TestSavePRRetryLastWriterWinsWhenUnmerged + TestMergeOutcomeMonotonicUnderChurn pass both sides BY DESIGN (LWW preservation + post-merge-fresh writers). RESULTS (scratch /tmp/pr67 @ ddc130b): gofmt clean; go vet clean; no new imports; go.mod untouched; go test -race ./internal/pulls/... PASS (10/11 full runs; 1 transient TestGetPRHeadDrift stream-assertion failure on first run, unreproducible since — passes isolated, -count=3, 6x consecutive full runs, and on base; stream/forced logic untouched by this diff, looks pre-existing); coverage 97.7% statements (gate >=95%). DOCS — ACCURATE. §2.3 paragraph + Decisions entry for #64 match the code (disjoint sets named correctly, write-once semantics, rationale outcome-not-rederivable vs head self-heals). MERGE RECOMMENDATION: ready to merge.

crueber closed this issue

2026-09-04 23:08:51 +00:00

crueber referenced this issue from a commit

2026-09-04 23:08:51 +00:00

Merge pull request 'Fix #64: pr.json CAS retry field-merge' (#67) from fix/issue-64 into main

crueber commented

2026-09-04 23:08:52 +00:00

Author

Owner

Fixed by PR #67 (review: owned-delta disjointness + write-once guard verified, negative controls genuine; 97.7% coverage), merged. Closing.

crueber referenced this issue

2026-09-04 23:29:07 +00:00