fix(adapter/sqs): standard SendMessageBatch double-send under leader churn

[bootjp]

Summary

Fixes a double-send bug in the SQS standard-queue SendMessageBatch path — the same :duplicate-elements class fixed for DynamoDB in PR #920, found by an adversarial S3/SQS audit prompted by the "extend dedup to S3/SQS" follow-up.

The bug

sendMessageBatchWithRetry → trySendMessageBatchOnce → sendBatchStandardOnce runs an in-process OCC retry loop that, on every attempt, re-ran buildBatchSendRecord → buildSendRecord → newMessageIDHex(), minting a fresh random MessageID (and send timestamp) per entry per attempt, and derived the data/vis/byage storage keys from them.

Under a Raft leader-election storm, attempt 1 can commit its writes yet surface a self-inflicted WriteConflict to the adapter (the mechanism PR #920 documented). The retry then mints new MessageIDs and writes a second copy of every entry at new keys:

1. Attempt 1: mints {M1,M2,M3}, commits, but returns WriteConflict under churn.
2. Retry: mints {M1',M2',M3'}, commits cleanly, returns {M1',M2',M3'} + HTTP 200.
3. Queue holds both sets → every batch entry duplicated; the client only learns the second set.

Standard queues carry no dedup identity, and the OCC ReadKeys are only [meta, gen] (the random data keys never self-conflict), so nothing fenced this.

Why only batch: single SendMessage has no in-process retry (a conflict becomes a normal at-least-once SDK retry), and FIFO send is fenced by the MessageDeduplicationId dedup record. The batch path added an in-process retry without either fence.

The fix

Pre-generate one stable sqsSendIdentity (MessageID + receipt token + send timestamp) per entry, before the retry loop, and reuse it on every attempt (buildSendRecordWithIdentity). The storage keys then stay constant across retries, so a committed-but-conflicted attempt is overwritten idempotently instead of duplicated.

No FSM probe / PrevCommitTS / gate is needed — unlike DynamoDB's list_append (which re-read a growing value), the batch write is a whole-value PUT at a now-stable key, so re-applying is a plain idempotent overwrite. The record's QueueGeneration is still re-derived per attempt, so a concurrent DeleteQueue/PurgeQueue generation bump is followed correctly (the stale-gen attempt-1 write is orphaned/unreachable).

S3/SQS audit outcome (recorded in the design doc)

A handler-by-handler adversarial sweep found the original "S3/SQS share the same latent gap" note was over-broad:

• S3 — safe. Whole-value PUT at a stable key; PutObject is OCC-fenced by a caller-supplied StartTS (a leader-move retry that races an interleaved write fails the fence rather than clobbering), and has no in-process re-read-recompute. CompleteMultipartUpload's manifest is re-assembled deterministically from the immutable uploaded parts (computed once before its retry loop), not from a re-read of the object value → no duplicate.
• SQS single SendMessage / FIFO — safe (random-UUID key + no in-process retry; FIFO dedup record).
• SQS standard SendMessageBatch — fixed here.

Tests (adapter/sqs_batch_send_dedup_test.go)

• TestSendMessageBatchStandard_RetryReusesStableKeys — first dispatch commits-then-conflicts; asserts the retry dispatches the same storage keys (fresh MessageIDs would fail it → reproduces the bug pre-fix).
• TestSendMessageBatchStandard_ReturnedMessageIdsStableAcrossRetry — the returned MessageId is the one actually written on both attempts (client id == stored id).

Validation

• go test ./adapter/ -run 'SendMessage|Batch|FIFO|Send' pass; new dedup tests pass.
• golangci-lint run ./adapter/... 0 issues; go build ./... clean.

Self-review (5 passes)

1. Data loss — the fix prevents a double-send (extra messages), and keeps the generation re-derivation so a concurrent queue delete/recreate still routes to the live generation; no committed message is dropped.
2. Concurrency / distributed — stable keys make the leader-churn retry an idempotent overwrite; the [meta,gen] ReadKeys fence against DeleteQueue/PurgeQueue is unchanged.
3. Performance — identities are minted once instead of once-per-attempt (strictly fewer crypto/rand calls on the retry path); hot path (no retry) is one mint per entry, same as before.
4. Data consistency — eliminates the duplicate-element anomaly for standard batch send; FIFO/exactly-once and single-send semantics unchanged.
5. Test coverage — new co-located tests pin key-stability across the retry (the regression), the load-bearing property of the fix.

[coderabbitai]

Warning

Review limit reached

@bootjp, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 4 minutes and 11 seconds. Learn how PR review limits work.

Your organization has run out of usage credits. Purchase more in the billing tab.

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 3b4561b5-2d85-419b-b034-6c118efaf8f4

📥 Commits

Reviewing files that changed from the base of the PR and between d37b0a0 and 65fe7d0.

📒 Files selected for processing (4)

• adapter/sqs_batch_send_dedup_test.go
• adapter/sqs_messages.go
• adapter/sqs_messages_batch.go
• docs/design/2026_06_03_partial_dynamodb_onephase_dedup.md

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch sqs/batch-send-dedup

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[bootjp]

@claude review

[claude]

Claude finished @bootjp's task in 3m 21s —— View job

---

Code Review: fix(adapter/sqs): standard SendMessageBatch double-send under leader churn

• Read changed files
• Review adapter/sqs_messages.go
• Review adapter/sqs_messages_batch.go
• Review adapter/sqs_batch_send_dedup_test.go
• Review docs/design/2026_06_03_partial_dynamodb_onephase_dedup.md
• Post final review

---

Summary

The fix is correct and addresses the root cause cleanly. The pattern — mint a stable identity once before the retry loop, reuse it on every attempt — is exactly the right medicine for this class of bug. The code quality is high, the factoring is clean, and the tests pin the regression. A few minor observations below.

---

Fix correctness

Core mechanism — correct. Pre-generating sqsSendIdentity (MessageID + receipt token + sendTsMillis) before sendMessageBatchWithRetry's retry loop (sqs_messages_batch.go:132–139) means all three storage keys (data, vis, by-age) are content-stable across retries. A committed-but-conflicted attempt is an idempotent overwrite, not a double-send.

QueueGeneration correctly excluded from the identity. buildSendRecordWithIdentity (sqs_messages.go:714) derives QueueGeneration from meta.Generation, which is freshly read per attempt. A concurrent DeleteQueue/PurgeQueue that bumps the generation between attempts therefore routes the retry to the new generation's key space — the correct behaviour. The stale-generation attempt's commit is effectively orphaned at the old generation, not reachable. Well thought out.

buildSendRecord delegation — correct. The single-send path keeps calling buildSendRecord, which now delegates through buildSendRecordWithIdentity. No behavioural change for single sends (they have no in-process retry, so a conflict surfaces to the client as a normal SDK retry). The refactor is clean.

---

Minor observation: vis key can differ between retries when queue-level DelaySeconds changes mid-retry

AvailableAtMillis (embedded in the vis key) is computed as id.sendTsMillis + delay * sqsMillisPerSecond inside buildSendRecordWithIdentity (sqs_messages.go:703). The delay is resolved from freshly-read meta per attempt in buildBatchSendRecord (sqs_messages_batch.go:435):

delay, err := resolveSendDelay(meta, entry.DelaySeconds)

When entry.DelaySeconds != nil (per-message delay), delay is constant → vis key is fully stable. When entry.DelaySeconds == nil, delay comes from meta.DelaySeconds. If a concurrent SetQueueAttributes changes DelaySeconds during the retry window, the vis key would differ between attempts.

In that scenario:

• Attempt 1 commits vis key V1 (data key D is stable by MessageID).
• Attempt 2 writes vis key V2 ≠ V1 and overwrites data key D.
• V1 is now a zombie vis key: it scans in ReceiveMessage but the subsequent OCC rotate fails immediately (data key D has been updated with a new receipt token after attempt 2's write), so no duplicate delivery to consumers.

The consequence is a zombie vis key in the store, not a double-send. The window is also extremely narrow (transactRetryMaxDuration ≈ seconds, and the queue-level delay rarely changes). This is an acceptable residual edge case, not a blocking issue — just worth documenting if the scan/deliver path sees elevated skip-rate metrics in that scenario.

---

Test observations

TestSendMessageBatchStandard_RetryReusesStableKeys — correctly models the committed-but-conflicted scenario: first dispatch returns store.ErrWriteConflict, second succeeds. The require.Equal(t, coord.dispatchKeys[0], coord.dispatchKeys[1]) assertion transitively covers vis key stability (the key bytes embed AvailableAtMillis). This is the exact regression test the CLAUDE.md convention requires for a review-identified defect. ✓

TestSendMessageBatchStandard_ReturnedMessageIdsStableAcrossRetry — reconstructs the expected data key inline using &sqsQueueMeta{Name: "q", Generation: 1}. This routes through the legacy (non-partitioned) path in sqsMsgDataKeyDispatch (PartitionCount: 0 implicit), which matches the production path for standard queues (standard queues cannot have PartitionCount > 1). The assertion is sound. ✓

Gap: neither test covers the delay-instability edge case described above. Given how narrow the window is, this is acceptable — but a comment noting the delay dependency in buildBatchSendRecord would help a future reader who sees the zombie-vis-key scenario.

---

Nit: newSendIdentity uses time.Now() directly

// sqs_messages.go:685
return sqsSendIdentity{messageID: messageID, token: token, sendTsMillis: time.Now().UnixMilli()}, nil

CLAUDE.md forbids time.Now() for ordering-sensitive operations, but sendTsMillis here feeds only the SentTimestamp system attribute reported to consumers (diagnostic) and the vis/by-age key offsets (not MVCC visibility or OCC ordering). The single-send path has always done this too. This is consistent with existing practice and within the spirit of the guideline. Not a concern.

---

Design doc

The S3/SQS audit section in 2026_06_03_partial_dynamodb_onephase_dedup.md is clear and well-structured. The three-way verdict (S3 safe, SQS single/FIFO safe, SQS standard batch fixed) is correctly argued. The S3 CompleteMultipartUpload safety reasoning (manifest re-assembled from immutable parts, not a re-read of the object) is the key insight and is stated precisely.

---

Verdict

Approve. The fix is correct, well-scoped, and properly tested. The vis-key edge case under mid-retry SetQueueAttributes is a residual imperfection (zombie key, not a double-send) and can be addressed separately if it surfaces in practice.

[gemini-code-assist]

Code Review

This pull request fixes a deduplication bug in SQS standard SendMessageBatch where retries would re-mint fresh message IDs and timestamps, potentially causing double-sends under leader churn. This is resolved by pre-generating a stable sqsSendIdentity per entry before the retry loop and reusing it across attempts. The design documentation and tests have been updated accordingly. The review feedback suggests optimizing this implementation by lazily initializing the identities only when entering the standard queue path, preventing unnecessary cryptographic random reads for FIFO queues.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

[gemini-code-assist]

Pre-generating sqsSendIdentity for all entries before the retry loop is inefficient for FIFO queues, as FIFO queues completely ignore these identities (they mint their own per-entry dedup-fenced identity). Since newSendIdentity() calls crypto/rand.Read twice (once for MessageID and once for ReceiptToken), this results in up to 20 unnecessary cryptographic random reads per batch send on FIFO queues, which can be a significant performance bottleneck under high throughput.

We can optimize this by pre-allocating the identities slice with zero values, and then lazily initializing each identity only when we actually enter the standard queue path (sendBatchStandardOnce) and only if it hasn't been initialized yet. Since slices are passed by reference, the initialized identities will be preserved across retries.

Suggested change

	identities := make([]sqsSendIdentity, len(entries))
	for i := range identities {
	id, err := newSendIdentity()
	if err != nil {
	return nil, nil, err
	}
	identities[i] = id
	}
	identities := make([]sqsSendIdentity, len(entries))

[gemini-code-assist]

Lazily initialize the identity on the first attempt if it's empty. This avoids generating unused identities for FIFO queues.

Suggested change

	for i, entry := range entries {
	// identities[i] is the stable per-entry identity minted once before
	// the retry loop; reusing it keeps the storage keys constant across
	// retries so a committed-but-conflicted attempt is overwritten
	// idempotently rather than double-sent.
	rec, recordBytes, apiErr := buildBatchSendRecord(meta, entry, identities[i])
	for i, entry := range entries {
	if identities[i].messageID == "" {
	id, err := newSendIdentity()
	if err != nil {
	return nil, nil, false, err
	}
	identities[i] = id
	}
	// identities[i] is the stable per-entry identity minted once before
	// the retry loop; reusing it keeps the storage keys constant across
	// retries so a committed-but-conflicted attempt is overwritten
	// idempotently rather than double-sent.
	rec, recordBytes, apiErr := buildBatchSendRecord(meta, entry, identities[i])

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 30b902b024

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Keep visibility index keys idempotent across retries

When a standard batch entry omits per-message DelaySeconds, this call rebuilds the record from freshly loaded queue metadata on every retry while reusing only the MessageId/timestamp identity. If the first attempt commits but reports a retryable conflict under leader churn, and SetQueueAttributes changes the queue DelaySeconds before the retry, the retry writes a second vis key for the same MessageId because AvailableAtMillis changes; the old committed vis key is never deleted. The receive path trusts the visibility index entry it scans, so that stale key can make the message visible before the new delay and leave another stale visibility entry that can redeliver the same message again. Either the delay-derived index key also needs to remain stable for the retry series, or retries need to clean up the previous vis/by-age keys for the same identity.

Useful? React with 👍 / 👎.

[bootjp]

Round-1 review fixes pushed

gemini (medium) — wasted identity minting on FIFO batches (FIXED)

Identities are now minted lazily on the first standard-path attempt (into a zero-valued, by-reference slice reused across retries) instead of all upfront. FIFO batches — which mint their own per-entry dedup-fenced identity — pay zero crypto/rand cost for identities they ignore.

codex (P2) — vis key not stable across a mid-retry DelaySeconds change (FIXED)

AvailableAtMillis (which the vis/by-age keys embed) is now captured in the identity at first mint and reused, instead of being recomputed from a re-resolved delay each attempt. So a SetQueueAttributes that changes DelaySeconds between a committed-but-conflicted attempt and its retry no longer shifts the vis key / orphans the first attempt's vis index entry. Validation + delay resolution also run only on first mint, so an already-committed entry is never re-failed against a since-changed meta.

buildSendRecordWithIdentity drops its delay param (AvailableAtMillis lives in the identity); buildSendRecord's external signature is unchanged → single-send and FIFO callers unaffected (audited: only buildSendRecord callers are sqs_messages.go:515 and sqs_fifo.go:208, both pass the same (meta,in,delay)).

New test: TestSendMessageBatchStandard_VisKeyStableAcrossDelayChange (a mid-retry DelaySeconds bump must not move the keys). All batch/send/FIFO tests pass; golangci-lint ./adapter/... 0 issues.

@claude review

[claude]

Claude finished @bootjp's task in 2m 37s —— View job

---

Claude finished @bootjp's task —

---

Code Review: Round-1 fixes (fix(adapter/sqs): standard SendMessageBatch double-send under leader churn)

• Read changed files
• Review lazy identity minting (Gemini feedback fix)
• Review vis key stability across mid-retry DelaySeconds change (Codex feedback fix)
• Review new test TestSendMessageBatchStandard_VisKeyStableAcrossDelayChange
• Audit buildSendRecord callers for regressions

---

Lazy identity minting — correct

The zero-valued identities := make([]sqsSendIdentity, len(entries)) slice is pre-allocated but un-populated at the top of sendMessageBatchWithRetry (sqs_messages_batch.go:134). The buildBatchSendRecord guard (id.messageID == "") mints on first use and reuses on all subsequent attempts. FIFO batches branch into sendBatchFifoEntries at trySendMessageBatchOnce:179 and never reach sendBatchStandardOnce or buildBatchSendRecord at all, so they pay zero crypto/rand cost. Exactly what Gemini asked for.

messageID == "" as sentinel — sound. newMessageIDHex() returns a non-empty hex string; the zero value of the struct field is "", so the sentinel is unambiguous. An entry that fails validation on attempt 1 (e.g., body-too-large) stays at messageID == "", gets re-evaluated on the retry against the current meta — correct, since it was never committed.

Vis key stability — correct

AvailableAtMillis is now captured in sqsSendIdentity.availableAtMillis inside newSendIdentity(delay) (sqs_messages.go:697):

availableAtMillis: now + delay*sqsMillisPerSecond,

buildSendRecordWithIdentity reads id.availableAtMillis directly; it no longer takes a delay parameter. mintBatchSendIdentity calls resolveSendDelay only on the first attempt for an entry; retries skip the mint block entirely. The vis and by-age keys in sendBatchStandardOnce (sqs_messages_batch.go:219-220) embed rec.AvailableAtMillis, which comes from the stable identity — so a concurrent SetQueueAttributes changing DelaySeconds between attempts cannot shift these keys. The zombie-vis-key edge case from the round-0 review is fully closed.

buildSendRecord callers verified. Both callers — sqs_messages.go:515 (single standard send) and sqs_fifo.go:208 (FIFO) — continue to call the unchanged buildSendRecord(meta, in, delay) wrapper, which now creates the identity with newSendIdentity(delay) and delegates to buildSendRecordWithIdentity. No signature breakage.

New test — correct

TestSendMessageBatchStandard_VisKeyStableAcrossDelayChange (sqs_batch_send_dedup_test.go:91-118) is a clean regression test for Codex P2. The beforeDispatch hook writes a new meta with DelaySeconds: 900 before dispatch 2, simulating a SetQueueAttributes race. The require.Equal(t, coord.dispatchKeys[0], coord.dispatchKeys[1]) assertion fails if AvailableAtMillis were recomputed from the changed delay — a direct pin on the invariant. ✓

Nit: stale first line in buildBatchSendRecord's doc comment

sqs_messages_batch.go:406-419 has two sentences that read as independent comments merged together. The first ("runs every per-entry validation a single SendMessage would, but returns *sqsAPIError…") is left over from before the lazy-mint refactor — it's only true on the first attempt, and the rest of the comment accurately describes the new behavior:

// buildBatchSendRecord runs every per-entry validation a single
// SendMessage would, but returns the *sqsAPIError so the batch path
// can drop the entry into Failed[] instead of failing the whole
// request.
// buildBatchSendRecord builds one standard-queue batch entry's record, reusing
// ...

The stale first sentence implies validation always runs, but retries skip it. Minor but can mislead a reader. Suggest collapsing to a single accurate comment. Fix this →

---

Verdict

Approve. Both round-1 changes are correctly implemented and the new test pins the Codex P2 regression. The comment nit above is the only remaining observation — safe to merge as-is or address before merge, at your discretion.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8770f13aa5

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Revalidate cached batch entries after OCC conflicts

When a standard batch attempt loses a normal OCC race to SetQueueAttributes, the first dispatch may not have committed any message, but this path has already populated id.messageID. On the retry, a now-cached entry skips mintBatchSendIdentity, so it also skips metadata-dependent checks such as MaximumMessageSize; if the concurrent attribute update lowered the limit below this body size, the retry can commit a message that the fresh queue metadata should reject. The stable identity should be reused for keys, but the per-attempt validation against the currently loaded meta still needs to run before committing.

Useful? React with 👍 / 👎.

[bootjp]

Round-2 review fix pushed

codex (P2 round-2) — re-validate batch entries against current meta on every attempt (FIXED)

Round-1 moved per-entry validation into the first-mint path, so an entry whose identity was minted by a prior uncommitted attempt (lost a normal OCC race) skipped validation on retry — if a concurrent SetQueueAttributes lowered MaximumMessageSize meanwhile, the retry could commit a message the fresh meta rejects.

Split the two concerns:

• validateBatchEntry runs against the CURRENT meta on EVERY attempt (body required, MaximumMessageSize, attributes, FIFO params, delay bounds).
• The stable identity is still minted once and reused, so keys stay stable across retries (round-1 vis-key stability preserved). AvailableAtMillis is seeded from the first attempt's delay; re-resolving delay per attempt only re-checks bounds, it never shifts the key.

Caller audit: validateBatchEntry ← buildBatchSendRecord ← sendBatchStandardOnce (single callers each); mintBatchSendIdentity removed (no refs); single-send/FIFO unaffected (buildSendRecord unchanged).

New test: TestSendMessageBatchStandard_RevalidatesAgainstCurrentMetaOnRetry — a mid-flight MaximumMessageSize drop makes the retry reject the now-too-big entry. All batch/send/FIFO tests pass; golangci-lint ./adapter/... 0 issues.

@claude review

[claude]

Claude finished @bootjp's task in 3m 10s —— View job

---

Code Review: Round-2 fix (fix(adapter/sqs): standard SendMessageBatch double-send under leader churn)

• Read changed files
• Review validateBatchEntry / identity split in sqs_messages_batch.go
• Review sqs_messages.go for buildSendRecordWithIdentity / newSendIdentity changes
• Review new test TestSendMessageBatchStandard_RevalidatesAgainstCurrentMetaOnRetry
• Audit caller chain for regressions
• Post final review

---

Round-2 fix: validation/identity split — correct

validateBatchEntry runs before the id.messageID == "" guard (sqs_messages_batch.go:432-435), so it fires on every attempt regardless of whether the identity has been minted. A concurrent SetQueueAttributes that tightens MaximumMessageSize between an uncommitted attempt and its retry is therefore honored. The Codex P2 round-2 issue is closed cleanly.

Delay re-resolution on retries doesn't shift keys — correct. validateBatchEntry returns the re-resolved delay at line 432, but on a retry (where id.messageID != ""), that value is never passed to newSendIdentity. buildSendRecordWithIdentity reads id.availableAtMillis directly from the cached identity (sqs_messages.go:725), so the vis/by-age keys are immune to the re-resolved delay — the bounds check runs, the key doesn't move. The invariant from round-1 is preserved.

mintBatchSendIdentity removal — correct. The function no longer exists; its logic is now inline inside the id.messageID == "" block in buildBatchSendRecord. No remaining references.

Caller chain — no regressions. Both buildSendRecord callers (sqs_messages.go:515, sqs_fifo.go:208) continue to call the unchanged buildSendRecord(meta, in, delay) → newSendIdentity(delay) → buildSendRecordWithIdentity path. The sqsSendIdentity.availableAtMillis field addition flows through correctly.

---

New test — correct

TestSendMessageBatchStandard_RevalidatesAgainstCurrentMetaOnRetry (sqs_batch_send_dedup_test.go:126-154) pins the regression cleanly. The beforeDispatch hook fires at n==1 — before the first dispatch records its keys and returns ErrWriteConflict — so when the retry loads meta in trySendMessageBatchOnce, it sees the lowered MaximumMessageSize: 3. validateBatchEntry sees "alpha" (5 bytes) > 3 → entry goes to failed[] and elems is empty, so Dispatch is never called a second time. The assertions require.Empty(t, successful) + require.Len(t, failed, 1) are correct and sufficient for this scenario.

One minor observation on test terminology: the comment says "if a prior attempt minted the identity but did NOT commit (lost a normal OCC race)". Technically, in the test, recordingBatchCoordinator always returns ErrWriteConflict on the first dispatch — the test doesn't distinguish committed-vs-not. That's fine for the regression pin (the invariant holds either way), but the comment's "did NOT commit" is slightly imprecise. Not a correctness issue.

---

Residual edge case: committed attempt-1 + tightened attribute on retry

The round-1 + round-2 fixes compose correctly for the main scenarios. One corner case worth noting for documentation (not a blocker):

If attempt 1 commits (committed-but-conflicted), and a concurrent SetQueueAttributes lowers MaximumMessageSize between attempt 1 and the retry:

• Attempt 1's write is in the queue at the stable keys (round-1 guarantee).
• Retry re-validates → MaximumMessageSize check fails → entry lands in failed[].
• Client sees failed for a message that IS in the queue.

The consequence is an inconsistent client view (message in queue, client reports failure), but no double-send. This is within the SQS standard-queue at-least-once contract. The window is also narrower than round-1's target scenario. No action needed, but worth a sentence in the design doc's "residual edge cases" section if a future reader asks why a retry can return failed for a committed message.

---

Pre-existing nit (from round-1, still present)

The stale first paragraph of buildBatchSendRecord's doc comment (sqs_messages_batch.go:406-409) still reads:

// buildBatchSendRecord runs every per-entry validation a single
// SendMessage would, but returns the *sqsAPIError so the batch path
// can drop the entry into Failed[] instead of failing the whole
// request.

This is doubly wrong: (1) the function now returns error, not *sqsAPIError; (2) the function name is repeated in the second block immediately after. The correct doc comment follows at lines 410-423. Suggest dropping lines 406-409. Fix this →

---

Verdict

Approve. The round-2 fix is correctly implemented: validation runs every attempt against current meta, the stable identity (including availableAtMillis) is minted once and reused, and the new test pins the regression. The pre-existing stale doc comment is the only remaining observation — safe to merge as-is or address before merge.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ac3e12870b

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Fence retry overwrites against consumer mutations

When the first batch dispatch actually commits but returns a retryable conflict, this reused identity makes the retry PUT the same data/vis/by-age keys again, while the transaction still only reads the queue meta/generation keys. If the message is visible and a consumer receives or deletes it during the retry backoff, the second PUT can overwrite the rotated CurrentReceiptToken/VisibleAtMillis or even recreate a deleted record, because receive/delete mutate these same message keys under their own OCC transactions. The idempotent retry needs to read/fence the message keys or otherwise avoid overwriting a record that has changed since the first attempt.

Useful? React with 👍 / 👎.

[bootjp]

Round-3 (docs only) pushed — addresses the two non-blocking nits from the round-2 approval

claude[bot] approved round-2; these close the remaining observations:

• Stale doc comment removed: dropped the obsolete first paragraph of buildBatchSendRecord's doc (it referenced a *sqsAPIError return that no longer exists and duplicated the accurate description below).
• Residual edge documented: added a note to the design doc — if attempt 1 commits and a concurrent SetQueueAttributes tightens a limit before the retry, the retry re-validates and reports the (already-stored) entry as failed — an inconsistent client view within the standard-queue at-least-once contract, never a double-send.

No functional change (comment + markdown only); go build + golangci-lint ./adapter/... clean.

@claude review

[claude]

Claude finished @bootjp's task in 1m 56s —— View job

---

Code Review: Round-3 (docs only)

• Read changed files
• Verify stale doc comment removal in sqs_messages_batch.go
• Verify residual edge case note in design doc
• Check unaddressed codex P2 from round-3 (sqs_messages_batch.go:208)
• Post final review

---

Stale doc comment removal — correct

The four stale lines are gone. What remains (sqs_messages_batch.go:406–419) is an accurate, well-structured doc comment covering both concerns:

• Validation against current meta runs every attempt.
• Stable identity (MessageID + token + AvailableAtMillis) is minted once and reused.

The validateBatchEntry function immediately below (sqs_messages_batch.go:442–448) has its own precise doc comment explaining why delay re-resolution on retries never shifts the keys. No redundancy, no stale references.

---

Design doc update — correct and well-scoped

The updated paragraph (docs/design/…:360–372) now covers all three milestones of the round-1/2 iteration in one place:

• AvailableAtMillis pinned in the identity → vis key stable across retries.
• Per-entry validation re-runs against current meta → tightened attribute honored on retry.
• Residual edge: committed-but-conflicted + mid-retry SetQueueAttributes tightening → retry reports Failed[] for a stored message, inconsistent client view, not a double-send.

The residual edge framing ("distinguishing committed-vs-not would need a dedup probe, which is out of proportion for this corner") is accurate and proportionate.

---

Unaddressed codex P2 from round-3 — note for completeness

The codex review posted at 2026-06-04T13:13:04Z (reviewing the round-2 commit) surfaced a new P2: "Fence retry overwrites against consumer mutations" at sqs_messages_batch.go:208. This was not part of the two nits identified in the round-2 approval, so round-3 understandably didn't address it. Worth noting the analysis for the record:

The concern: if attempt 1 commits but returns a retryable conflict, and during the retry backoff a consumer calls ReceiveMessage (rotating the receipt token) or DeleteMessage (removing the record), the retry's PUT overwrites the mutated/deleted data key. The batch send transaction's OCC read set covers only [meta, gen], not the per-message data key, so the conflict goes undetected.

Assessment: Real but narrow-window and within contract. The conditions require: zero delay (message immediately visible), a consumer calling ReceiveMessage within transactRetryMaxDuration (seconds), and a retry that loses the race. For DeleteMessage the consequence is message resurrection; for ReceiveMessage it's receipt token invalidation (consumer's subsequent DeleteMessage would fail OCC cleanly). Both are within the SQS standard-queue at-least-once contract. Fixing this properly would require adding the message data key to the send transaction's read set — a deliberate scope decision, not a blocking omission for this PR. Worth a sentence in the residual-edge section of the design doc if a future iteration revisits it.

---

Verdict

Approve. The round-3 docs-only changes are correct, precise, and close the two nits from the round-2 approval. The unaddressed codex P2 on consumer-mutation fencing is real but narrow, within the at-least-once contract, and out of scope for this PR's fix. Safe to merge.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 65fe7d0d75

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Preserve committed entries after retry-time revalidation

When the first standard batch attempt actually commits but surfaces a retryable conflict (the leader-churn case this patch is fixing), a concurrent SetQueueAttributes that lowers MaximumMessageSize before the retry makes this validation return an error even though id.messageID is already cached from the committed attempt. If all affected entries fail here, sendBatchStandardOnce returns them in Failed[] without rewriting anything, so the client receives a sender-fault failure for a message that is already visible in the queue. Fresh evidence beyond the earlier revalidation discussion is that validation now runs before checking whether the stable identity was already minted for a prior attempt.

Useful? React with 👍 / 👎.

[bootjp]

codex P2 ×2 → deferred to #931 (decision: merge this fix, follow up on the full dedup)

Both new codex P2 findings on 65fe7d0d stem from the same root: the simple stable-key reuse assumes attempt 1's keys are untouched between the committed-but-conflicted attempt and the retry, which a concurrent consumer (receive/delete) or SetQueueAttributes can violate. Closing them correctly requires the full option-2 dedup (local commitTS + leader-only + PrevCommitTS exact-ts probe + data-key fence + result cache) — the same gated machinery used for DynamoDB in #920, i.e. a substantially larger change than this double-send correctness fix.

Per maintainer decision, this PR ships the double-send fix (the Jepsen-caught :duplicate-elements, which it fully eliminates and which claude approved), and the two P2 edges are tracked in #931 for the gated dedup follow-up:

• P2-1 (retry clobbers a concurrent consumer receive/delete) — narrower window than the original double-send, one message vs every entry; real but deferred.
• P2-2 (committed entry reported Failed[] after retry revalidation) — within the standard-queue at-least-once contract; already documented in the design doc.

Net: strict improvement over main (eliminates the severe, common double-send) with the residual narrow edges tracked. Enabling auto-merge (squash) once required checks pass.