Skip to content

Let a published batch receipt own its storage - #390

Merged
zzet merged 1 commit into
mainfrom
fix/batch-receipt-publish-owns-storage
Jul 28, 2026
Merged

Let a published batch receipt own its storage#390
zzet merged 1 commit into
mainfrom
fix/batch-receipt-publish-owns-storage

Conversation

@zzet

@zzet zzet commented Jul 28, 2026

Copy link
Copy Markdown
Owner

Fixes the data race that turned main red on the macOS runner: TestAtomicBatchConcurrentIdempotencyWritesOnce, batch_transaction.go:501 (write) against batch_transaction.go:82 (read) via batch_transaction_journal.go:84.

Root cause

batchTransactionState.publish stored the receipt struct under the write lock, but Results, Files and Summary are reference types — the stored copy kept pointing at the publisher's backing array. snapshot() deep-copies on the way out, so it looked symmetric, but nothing stopped the publisher from writing into storage it had already handed over.

runBatchTransaction does exactly that:

  1. publishes the receipt as "prepared" (results still "validated"),
  2. commits to disk,
  3. stamps receipt.Results[i].Status = "applied" in place,
  4. republishes as "committed".

Step 3 writes into the array a concurrent snapshot() reads under RLock. The lock protected the slice header; it never protected the elements.

Why it is more than a detector complaint

A concurrent idempotent retry classifies the in-flight transaction by snapshotting its receipt. Between the applied-loop and the "committed" publish sits a manifest write and an fsync, so a reader reliably observes a torn receipt: status "prepared" already carrying applied results. The added test asserts that directly and fails without the fix.

Fix

Copy through a single cloneBatchReceipt helper at both boundaries, so a published receipt is owned solely by the state and publishers stay free to keep mutating their own copy. publish clones before taking the lock — the clone touches only the publisher's own storage. CompletedAt is cloned too, closing the same aliasing hole for the one pointer field.

Verification

  • New TestBatchTransactionPublishCopiesReceiptStorage — deterministic, no race detector needed. Fails 3/3 without the fix on all four fields, in both directions.
  • New TestAtomicBatchSnapshotIsolatedFromCommitInProgress — drives the reported interleaving through the same call path as the CI stack. Without the fix it reports the race and the torn observation; the write override deliberately signals without waiting, since a back-edge to the reader orders the commit loop behind its reads and hides the bug.
  • Originally failing test stressed 30x at -cpu=2,8: clean.
  • Full go test -race ./internal/mcp/: clean. go build ./..., go vet, golangci-lint: clean.

publish() stored the receipt struct under the write lock, but Results,
Files and Summary are reference types: the stored copy kept pointing at
the publisher's backing array. runBatchTransaction publishes "prepared",
then stamps Results[i].Status = "applied" in place during commit, so
those writes landed in memory a concurrent snapshot() was reading under
RLock — the lock protected the header, never the elements.

Concurrent idempotent retries hit this: a caller classifying an in-flight
transaction could observe status "prepared" already carrying applied
results, and the race detector flagged the commit loop against snapshot's
slice copy.

Copy through cloneBatchReceipt at both boundaries so the state owns what
it holds and publishers stay free to keep mutating their own receipt.
@zzet
zzet merged commit f1a74cb into main Jul 28, 2026
17 of 18 checks passed
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.

1 participant