feat(sqs): receipt-handle v2 codec for partitioned FIFO queues (Phase 3.D PR 5a)#724
feat(sqs): receipt-handle v2 codec for partitioned FIFO queues (Phase 3.D PR 5a)#724
Conversation
… 3.D PR 5a) Adds the v2 receipt-handle layout that PR 5b's send / receive fanout will consume. Pure scaffold: the v2 encoder is added but not yet called by any production path (PartitionCount > 1 is still rejected by the §11 PR 2 dormancy gate, so SendMessage never reaches a partitioned queue, so encodeReceiptHandleV2 stays unreachable until PR 5b lifts the gate). Splitting the codec out as a small isolated PR keeps PR 5b's review focused on the gate-and-lift atomic sequence — codec correctness and the gate-lift correctness are independent concerns and a single mega-PR would conflate them. Wire format v1 (legacy, single partition): [ 0 ] byte version = 0x01 [ 1..9 ] uint64 queue_gen (BE) [ 9..25 ] 16 bytes message_id [ 25..41 ] 16 bytes receipt_token Total: 41 bytes v2 (partitioned, PartitionCount > 1): [ 0 ] byte version = 0x02 [ 1..5 ] uint32 partition (BE) ← NEW [ 5..13 ] uint64 queue_gen (BE) [ 13..29 ] 16 bytes message_id [ 29..45 ] 16 bytes receipt_token Total: 45 bytes decodedReceiptHandle gains Version (byte) and Partition (uint32) fields. decodeReceiptHandle dispatches by the version byte; v1 keeps Partition=0 by definition. What does NOT change yet - encodeReceiptHandleV2 is dormant until PR 5b's SendMessage partitioned-fanout dispatch wires it (gated by meta.PartitionCount > 1). Production traffic continues to use the v1 encoder verbatim — no behaviour change. - DeleteMessage / ChangeMessageVisibility still consume decodedReceiptHandle without inspecting Version or Partition. PR 5b adds the cross-version rejection contract (a v1 handle against a partitioned queue → ReceiptHandleIsInvalid, and vice-versa). Tests - TestEncodeReceiptHandleV2_RoundTrip — every (partition, queue_gen, message_id, token) tuple round-trips with Version=v2 and the encoder's partition. - TestEncodeReceiptHandleV1_StillReportsV1 — regression: v1 encoder still produces v1-decodable handles with Partition=0. - TestDecodeReceiptHandle_VersionDispatch — v1 and v2 produce distinct on-wire bytes (different version + 4-byte size delta); decoder picks the right layout for each. - TestDecodeReceiptHandle_RejectsLengthMismatch — 5 cases (v1 byte / v2 length, v2 byte / v1 length, v1 truncated, v2 truncated, empty) all fail with the same opaque error. - TestDecodeReceiptHandle_RejectsUnknownVersion — 0x00, 0x03, 0x42, 0xFF all reject. - TestEncodeReceiptHandleV2_RejectsBadInputs — short / long token, non-hex / short / empty id all surface as encoder errors. - TestReceiptHandleVersionConstants_Distinct — pins the invariants v1 != v2, v1 == 0x01, v2 == 0x02, legacy alias points at v1, on-wire size constants match what encoders write. - TestDecodeReceiptHandle_RejectsBase64Garbage — base64 errors surface at the right step. - Existing TestSQSServer_ReceiptHandleCodecRoundTrip continues to pass unchanged (v1 path). Self-review (per CLAUDE.md) 1. Data loss — codec only; no FSM/Pebble path. No issue. 2. Concurrency — no shared state; pure encode/decode functions. No issue. 3. Performance — encode/decode are O(handle size) ≤ 45 bytes. The version-byte dispatch is one switch. No issue. 4. Data consistency — v1 and v2 are length-distinct, version-byte- distinct, and decoder rejects wrong-length blobs. A v1 client parsing a v2 handle (or vice versa) fails closed at the length check — pinned by the 5-case rejection test. No issue. 5. Test coverage — 8 new tests across the codec contract surface; existing v1 round-trip test continues to pass.
|
@claude review Phase 3.D PR 5a — receipt-handle v2 codec scaffold. Pure-additive: v2 encoder is dormant until PR 5b lifts the dormancy gate. Splitting it out keeps PR 5b's review focused on the gate-and-lift atomic sequence. PR description has the full self-review and test plan. |
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the 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 have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughSQS receipt handle encoding is extended from a single legacy format to a versioned scheme with v1 (single-partition) and v2 (partitioned FIFO) layouts. Decoding now inspects version bytes to dispatch to the appropriate decoder, with decoded handles carrying version and partition metadata. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~35 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Review rate limit: 0/1 reviews remaining, refill in 46 minutes and 24 seconds.Comment |
|
Claude finished @bootjp's task in 3m 54s —— View job Code Review — Phase 3.D PR 5a: v2 Receipt Handle Codec
Overall: The codec is correct, the tests are thorough, and the dormancy boundary is cleanly maintained. A few minor observations below — none are blocking. Wire format correctnessOffsets in both encoders and decoders are verified against the size constants:
The Observations1.
The comment says the alias preserves "existing call sites in this file" — but The alias is harmless but the comment is slightly misleading. Either drop the alias (and the corresponding assertion in 2. Magic offset numbers in the decoders (nit)
The offsets const (
sqsReceiptHandleV1GenOffset = 1
sqsReceiptHandleV1IDOffset = sqsReceiptHandleV1GenOffset + 8
sqsReceiptHandleV1TokenOffset = sqsReceiptHandleV1IDOffset + sqsMessageIDBytes
// ...analogous v2 offsets
)Not required to merge, but makes layout changes self-consistent. 3. Open-ended receipt token slice (nit)
// Current:
ReceiptToken: bytes.Clone(b[9+sqsMessageIDBytes:]),
// More explicit:
ReceiptToken: bytes.Clone(b[9+sqsMessageIDBytes : 9+sqsMessageIDBytes+sqsReceiptTokenBytes]),Functionally identical (the length check above guarantees no extra bytes), but an explicit upper bound makes the intent self-documenting. 4.
The docstring currently says callers "MUST inspect Version before consulting Partition" and that cross-version handles "are rejected with Test coverageAll 8 new tests are well-targeted:
One minor gap: there's no case for a blob with version=v2 and The existing Self-review passes (CLAUDE.md §5 lenses)
Ready to merge. The nits are worth a quick pass but none warrant holding the PR. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@adapter/sqs_messages.go`:
- Around line 324-343: The dispatcher currently accepts v2 handles which
prematurely exposes the new format; update decodeReceiptHandle so it only
dispatches to decodeReceiptHandleV1 for now and treats sqsReceiptHandleVersion2
(and any other non-v1 version) as a length/version mismatch error, keeping
decodeReceiptHandleV2 dormant; additionally add a regression/guard in the
DeleteMessage and ChangeMessageVisibility entry points to enforce that
downstream validation still only accepts v1-derived fields (specifically ensure
QueueGeneration and ReceiptToken checks reject any v2-encoded handles or handles
that would rely on v2-only fields/symbols) so the public API behavior remains
unchanged until PR 5b.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e4d0102b-7d67-4030-ac58-09770f498293
📒 Files selected for processing (2)
adapter/sqs_messages.goadapter/sqs_receipt_handle_v2_test.go
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a versioned SQS receipt handle format to support partitioned FIFO queues. It adds a new v2 handle format incorporating a partition index, updates the decoding logic to handle both legacy and partitioned formats, and provides extensive unit tests. Feedback indicates that the appendU32 helper function used in the new encoding logic is missing, which will cause a compilation error.
| } | ||
| buf := make([]byte, 0, sqsReceiptHandleV2Size) | ||
| buf = append(buf, sqsReceiptHandleVersion2) | ||
| buf = appendU32(buf, partition) |
There was a problem hiding this comment.
The function appendU32 is called here but it is not defined in this file or any other provided file in the package. This will cause a compilation error. You should add a helper function similar to appendU64 for uint32 values. Ensure that such utility functions are centralized if they are intended for use across the repository to maintain consistency and avoid duplication.
func appendU32(dst []byte, v uint32) []byte {
var buf [4]byte
binary.BigEndian.PutUint32(buf[:], v)
return append(dst, buf[:]...)
}
func encodeReceiptHandleV2(partition uint32, queueGen uint64, messageIDHex string, receiptToken []byte) (string, error) {References
- Centralize common logic into shared helper functions to avoid code duplication and improve maintainability.
Round 1 Claude review nits on PR #724: 1) Dropped sqsReceiptHandleVersion alias Round 1 added sqsReceiptHandleVersion as an alias of v1 "for existing call sites". In practice every call site now uses sqsReceiptHandleVersion1 explicitly, so the alias's only reference was a self-test. Dropped both the alias and the test assertion. The two version-byte constants (sqsReceiptHandleVersion1 / sqsReceiptHandleVersion2) are now the single source of truth. 2) Named per-field offset constants decodeReceiptHandleV1 / decodeReceiptHandleV2 used numeric offsets (1, 9, 13, 1+sqsMessageIDBytes, …) derived from layout arithmetic. Replaced with named constants: v1: V1VersionOffset, V1GenOffset, V1IDOffset, V1TokenOffset, V1Size v2: V2VersionOffset, V2PartitionOffset, V2GenOffset, V2IDOffset, V2TokenOffset, V2Size A future field-shape change touches the constants in one place instead of scattering numeric arithmetic through encoders / decoders. 3) Explicit upper bound on token slice bytes.Clone(b[start:]) → bytes.Clone(b[start:end]). The length check above guarantees the open-ended form would have read the right bytes, but the explicit upper bound is self-documenting and protects against a future refactor that forgets the length precondition. 4) Doc note on cross-version rejection decodedReceiptHandle's doc described the cross-version rejection contract ("a v1 handle against a partitioned queue → ReceiptHandleIsInvalid") that this scaffold PR does NOT enforce — that's PR 5b's gate-and-lift. Added an explicit note that enforcement is wired by PR 5b so future review of that PR doesn't go looking for the contract here. 5) New test case: v2 / v1 oversized TestDecodeReceiptHandle_RejectsLengthMismatch grew two cases (v2 + 1 byte, v1 + 1 byte) so the exact-length check on each decoder branch is independently exercised. The previous table covered "byte mismatch" and "truncated" but not "too long for the version". Audit per the lessons-learned discipline The semantic change here is the alias removal. grep -rn sqsReceiptHandleVersion across the repo confirmed zero remaining references after the test assertion was dropped. The named-offset refactor preserves byte-for-byte the same on-wire format; the constants ARE the layout. go test -race ./adapter/... and golangci-lint clean.
|
@claude review Round 2 — addresses round 1's 4 nits + 1 test gap:
Audit per lessons-learned:
|
|
Claude finished @bootjp's task in 2m 45s —— View job Code Review — Phase 3.D PR 5a: v2 Receipt Handle Codec (Round 2)
Overall: All five Round 1 nits are cleanly addressed. The code is correct and the test coverage is solid. Two reviewer concerns from other tools are addressed below. Round 1 nits — status
Gemini's concern: missing
|
| Constant | Value | Check |
|---|---|---|
V1VersionOffset |
0 | ✓ |
V1GenOffset |
1 | ✓ (1 byte for version) |
V1IDOffset |
9 | ✓ (1+8) |
V1TokenOffset |
25 | ✓ (1+8+16) |
V1Size |
41 | ✓ (1+8+16+16) |
V2PartitionOffset |
1 | ✓ |
V2GenOffset |
5 | ✓ (1+4) |
V2IDOffset |
13 | ✓ (1+4+8) |
V2TokenOffset |
29 | ✓ (1+4+8+16) |
V2Size |
45 | ✓ (1+4+8+16+16) |
All slice indices in both decoders align with the constants. Wire format unchanged.
Self-review passes (CLAUDE.md §5 lenses)
- Data loss — codec only; no FSM/Pebble path. ✓
- Concurrency — pure encode/decode, no shared mutable state. ✓
- Performance — O(45 bytes), one switch dispatch. ✓
- Data consistency — v1/v2 length-distinct, version-byte-distinct, decoder fails closed. Dormancy maintained at encoder level. ✓
- Test coverage — 8 tests covering the contract surface, now closed on oversized blobs. ✓
Ready to merge.
CodeRabbit major on PR #724 round 2: the v2 codec is in the binary AND reachable on the public API. A client can re-encode a legitimately-issued v1 handle's (queue_gen, message_id, receipt_token) under the v2 layout, and DeleteMessage / ChangeMessageVisibility will accept it (downstream validation only checks queue_gen + receipt_token). The behaviour is technically correct (the v1 keyspace lookup still finds the message) but it leaks the new wire format before PR 5b lands — breaking the "no behavior change yet" guarantee. Fix decodeClientReceiptHandle wraps decodeReceiptHandle with the dormancy gate: any v2 handle on the public API surfaces as ReceiptHandleIsInvalid until PR 5b's queue-aware version replaces the gate (v1 required on non-partitioned queues, v2 required on partitioned ones). Changed call sites Three public-API decode points switch from decodeReceiptHandle to decodeClientReceiptHandle: - parseQueueAndReceipt (sqs_messages.go) — DeleteMessage and ChangeMessageVisibility entry point. - DeleteMessageBatch entry decode (sqs_messages_batch.go). - ChangeMessageVisibilityBatch entry decode (sqs_messages_batch.go). The low-level decodeReceiptHandle keeps working as a pure codec so the existing v1 + v2 round-trip tests keep passing. Tests - TestDecodeClientReceiptHandle_AcceptsV1: v1 handles flow through unchanged. - TestDecodeClientReceiptHandle_RejectsV2: a v2 handle on the public API surfaces as the dormancy-gate error. The low-level decoder still accepts it (the gate is at the API boundary, not in the codec). - TestDecodeClientReceiptHandle_PassesThroughDecodeErrors: a decode-step error (e.g. base64 garbage) is NOT masked by the dormancy-gate message — operators see the underlying error. Audit per the lessons-learned discipline The semantic change here is the rejection contract for v2 handles on the public API. grep -rn "decodeReceiptHandle" across adapter/ showed 3 production call sites (parseQueueAndReceipt, DeleteMessageBatch, ChangeMessageVisibilityBatch). All 3 updated. The v2 codec round-trip tests intentionally keep using decodeReceiptHandle directly so they exercise the codec, not the public API. Note on Gemini's "appendU32 is undefined" finding (round 2) False positive. appendU32 is defined in adapter/sqs_keys.go:444 and has been since PR #703 merged. Gemini did not search across files in the package; CI build is green and the round-1 v2 round-trip tests exercise the function. No code change needed.
|
@claude review Round 3 — addresses Round 2's two new findings: CodeRabbit major (real, fixed): v2 codec was reachable on the public API. A client re-encoding a legitimately-issued v1 handle's Added
Low-level Gemini high (false positive): "appendU32 is called but not defined" — Audit per lessons-learned:
|
|
Claude finished @bootjp's task in 2m 2s —— View job Code Review — Phase 3.D PR 5a: v2 Receipt Handle Codec (Round 3)
Overall: All Round 2 items remain in place. The CodeRabbit major is fully addressed with a clean wrapper + complete call-site sweep. Ready to merge. CodeRabbit major — verified, fully addressed
Gemini high — confirmed false positive
New tests — statusThree new tests pin the wrapper contract precisely:
The Round 2 nits — still in place
Self-review passes (CLAUDE.md §5 lenses)
Ready to merge. |
1 participant
Summary
Phase 3.D PR 5a — adds the v2 receipt-handle layout that PR 5b's send/receive partitioned fanout will consume. Pure scaffold: the v2 encoder is added but never called by any production path until PR 5b lifts the §11 PR 2 dormancy gate. Splitting the codec out as a small isolated PR keeps PR 5b's review focused on the gate-and-lift atomic sequence — codec correctness and gate-lift correctness are independent concerns.
Wire format
[ 0 ]0x010x02[ 1..5 ]uint32 partition (BE)← NEW[ ?..? ]uint64 queue_gen (BE)uint64 queue_gen (BE)[ ?..? ][ ?..? ]decodedReceiptHandlegainsVersion(byte) andPartition(uint32) fields.decodeReceiptHandledispatches on the version byte; v1 keepsPartition=0by definition.What does NOT change yet
encodeReceiptHandleV2is dormant until PR 5b's SendMessage fanout dispatch wires it (gated bymeta.PartitionCount > 1). Production traffic continues to use the v1 encoder verbatim — no behaviour change.decodedReceiptHandlewithout inspectingVersionorPartition. PR 5b adds the cross-version rejection contract (a v1 handle against a partitioned queue →ReceiptHandleIsInvalid, and vice-versa).Test plan
TestEncodeReceiptHandleV2_RoundTrip— every(partition, queue_gen, message_id, token)tuple round-trips.TestEncodeReceiptHandleV1_StillReportsV1— regression: v1 encoder still produces v1-decodable handles withPartition=0.TestDecodeReceiptHandle_VersionDispatch— v1 and v2 produce distinct on-wire bytes (different version + 4-byte size delta); decoder picks the right layout.TestDecodeReceiptHandle_RejectsLengthMismatch— 5 cases (v1 byte / v2 length, v2 byte / v1 length, v1 truncated, v2 truncated, empty) all fail with the same opaque error.TestDecodeReceiptHandle_RejectsUnknownVersion—0x00,0x03,0x42,0xFFall reject.TestEncodeReceiptHandleV2_RejectsBadInputs— short/long token, non-hex/short/empty id all error.TestReceiptHandleVersionConstants_Distinct— pinsv1 != v2,v1 == 0x01,v2 == 0x02, legacy alias points at v1, on-wire size constants match.TestDecodeReceiptHandle_RejectsBase64Garbage— base64 error surfaces correctly.TestSQSServer_ReceiptHandleCodecRoundTripcontinues to pass (v1 path).go test -race ./adapter/...pass.golangci-lint ./adapter/...clean.Self-review (per CLAUDE.md)
Summary by CodeRabbit
New Features
Tests