fix(mcp): close report_duplicate's CAS-race test flake via shared read seam - #332
Merged
Merged
Conversation
…d seam TestReportDuplicate_ReportsDistinctMessage_WhenCASPreconditionFails failed deterministically under GOMAXPROCS=1 (successes=2 instead of 1): reportDuplicate read the backlog item via h.storage.GetBacklogItem directly instead of the overridable h.getBacklogItemFor seam requestReview already uses, so the test had no way to force both racers' pre-transition reads to land before either's write. Under scheduling delay, the loser could observe the winner's already- committed status + VerificationNotes and take the idempotency short-circuit instead of racing the CAS write. This is the identical race shape already found and fixed for request_review in #308 (a4793c1), which added the getBacklogItemFn hook + readBarrier — reportDuplicate was never migrated onto that same seam. Fix: route reportDuplicate through h.getBacklogItemFor and add the matching readBarrier to its test, mirroring TestRequestReview_ReportsDistinctMessage_WhenCASPreconditionFails. Verified: 200 runs at -cpu=1,2,4 -race, previously failing reliably at cpu=1, now pass; server/mcp package clean at -count=10 -race. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W3683CH7Fs9zYR2yP3Dpba
tstapler
marked this pull request as ready for review
August 4, 2026 05:47
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Fixes a deterministic CAS-race test failure in report_duplicate by routing its pre-transition read through the existing overridable getBacklogItemFor seam, enabling the same read-barrier approach already used by request_review.
Changes:
- Route
reportDuplicate’s backlog-item read throughh.getBacklogItemFor(...)instead ofh.storage.GetBacklogItem(...). - Add a
readBarrier-backedgetBacklogItemFninTestReportDuplicate_ReportsDistinctMessage_WhenCASPreconditionFailsto deterministically synchronize competing reads. - Update
getBacklogItemFordocumentation to reflect its usage for both tools.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| server/mcp/tools_backlog_test.go | Adds a read barrier and injects getBacklogItemFn to make the CAS precondition race deterministic in the test. |
| server/mcp/tools_backlog.go | Switches reportDuplicate to the shared getBacklogItemFor seam and updates comments accordingly. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+921
to
+929
| // Routed through the same overridable getBacklogItemFor seam request_review | ||
| // uses (not h.storage.GetBacklogItem directly) so tests can inject a | ||
| // readBarrier to deterministically force two racing report_duplicate calls' | ||
| // pre-transition reads to both land before either's write — see | ||
| // TestReportDuplicate_ReportsDistinctMessage_WhenCASPreconditionFails and its | ||
| // request_review analogue for why this matters: without it, a sufficiently | ||
| // delayed loser can observe the winner's already-committed status+notes and | ||
| // take the idempotency short-circuit above instead of racing the CAS write. | ||
| item, getErr := h.getBacklogItemFor(ctx, itemID) |
| // VerificationNotes and takes reportDuplicate's idempotency short-circuit | ||
| // (same duplicate_ref/reason, same session) instead of racing the CAS | ||
| // write — producing two successes instead of one success + one CAS | ||
| // failure. Mirrors TestRequestReview_ReportsDistinctMessage_WhenCASPreconditionFails's |
Contributor
✅ Registry ValidationTest Coverage: 25/181 features have
|
Contributor
Go Benchmarks (Tier 1) |
Contributor
E2E RPC Latency |
Contributor
Frontend Terminal Throughput |
Contributor
📊 Feature E2E CoverageFeature coverage report unavailable
|
4 tasks
tstapler
added a commit
that referenced
this pull request
Aug 4, 2026
PR #332 merged to main with an independent fix for reportDuplicate's CAS-race test flake (same getBacklogItemFor + readBarrier fix this branch also arrived at independently, since this worktree branched before #332 merged). Resolved by keeping main's version for the reportDuplicate parts (tools_backlog.go's read + doc comment, tools_backlog_test.go's readBarrier + struct field ordering) rather than reintroducing a second, divergent copy of the same fix — this branch's genuinely new work (reportPRCreated's getBacklogItemFor migration, SetBacklogItemPRAndTransition's atomic-UPDATE fix, and BUG-058) is unaffected.
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
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.
Summary
TestReportDuplicate_ReportsDistinctMessage_WhenCASPreconditionFailswas flagged as failing during a full-suitego test -short ./...run today. Reproduced it deterministically (not merely intermittently) underGOMAXPROCS=1/-cpu=1:successes=2instead of the expected1 success + 1 CAS failure.Root cause
reportDuplicate(server/mcp/tools_backlog.go) read the backlog item viah.storage.GetBacklogItemdirectly, instead of the overridableh.getBacklogItemForseamrequestReviewalready uses. That seam exists specifically so tests can inject areadBarrierforcing both racing goroutines' pre-transition reads to land before either's write.Without it, under scheduling delay (exactly what a CPU-contended full
-short ./...run produces), the "loser" goroutine can execute its entire read step after the "winner" has already committed its status transition +VerificationNotes. Because both racers in this test share one session UUID and an identicalduplicate_ref/reason, the loser then matchesreportDuplicate's own idempotency short-circuit and returns a plain success instead of racing the CAS write — producing 2 successes where the test expects exactly 1.This is the identical race shape already discovered and fixed for
request_reviewin #308 (a4793c1d6), which added thegetBacklogItemFnhook +readBarrierto close it.report_duplicatewas added in that same PR but never wired onto the same seam, so its equivalent test was left exposed to the same race.Fix
reportDuplicatenow reads the item viah.getBacklogItemFor(ctx, itemID)instead ofh.storage.GetBacklogItemdirectly (no new interface — reuses the existing seam).TestReportDuplicate_ReportsDistinctMessage_WhenCASPreconditionFailsnow injects the samereadBarrier-backedgetBacklogItemFn, mirroringTestRequestReview_ReportsDistinctMessage_WhenCASPreconditionFails.Test plan
go test ./server/mcp/... -run TestReportDuplicate_ReportsDistinctMessage_WhenCASPreconditionFails -count=200 -cpu=1,2,4 -race— previously failed deterministically at-cpu=1; now 200/200 pass across all threeGOMAXPROCSsettingsgo test ./server/mcp/... -count=10 -race— cleanmake build && make test— only pre-existing failures aresession/tmux'sTestEnsureServerRunning_NoOp/TestKillOrphanedControlModeClients, both explicitly documented in.claude/rules/fix-flaky-tests-dont-defer.mdas known sandboxed-worktree tmux-availability issues, unrelated to this changemake lint— clean🤖 Generated with Claude Code
https://claude.ai/code/session_01W3683CH7Fs9zYR2yP3Dpba