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.
bootjp
commented
May 1, 2026
@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=1sqsReceiptHandleV1IDOffset=sqsReceiptHandleV1GenOffset+8sqsReceiptHandleV1TokenOffset=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. |
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
Uh oh!
There was an error while loading. Please reload this page.
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.
funcappendU32(dst []byte, vuint32) []byte {
varbuf [4]bytebinary.BigEndian.PutUint32(buf[:], v)
returnappend(dst, buf[:]...)
}
funcencodeReceiptHandleV2(partitionuint32, queueGenuint64, messageIDHexstring, 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.
bootjp
commented
May 1, 2026
@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.
bootjp
commented
May 1, 2026
@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. |
Uh oh!
There was an error while loading. Please reload this page.
….D PR 8) (#739) ## Summary Phase 3.D PR 8 — the doc-lifecycle bump for the split-queue-FIFO design. Renames `2026_04_26_proposed_sqs_split_queue_fifo.md` → `2026_04_26_partial_sqs_split_queue_fifo.md` because every milestone in the rollout plan that produces shippable code has merged on `main` (#681 schema, #703 keyspace, #704 / #708 / #715 / #721 / #723 routing-layer 4-A through 4-B-3b, #724 / #731 / #732 / #734 data-plane fanout 5a through 5b-3, #735 / #736 reaper 6a/6b, #737 / #738 metrics + Jepsen 7a/7b). This is the milestone tracked as PR 8 in §11 of the design doc itself. ## Why "partial" rather than "implemented" Per `docs/design/README.md`: | Value | Meaning | |---|---| | `proposed` | No matching code on `main`, or the doc itself declares itself a proposal. | | `partial` | Some components exist but the full scope described in the doc is not yet merged. | | `implemented` | Concrete Go code exists on `main` that matches the design's central subsystem. | The central subsystem (the partitioned-FIFO data plane, reaper, capability gate, metrics, and Jepsen workload) does exist on `main`, which would suggest `implemented`. I chose `partial` because §10 (Open Questions) and §12 (Alternatives Considered) explicitly note follow-on work that extends the same surface — operator-configurable hash, online resharding, cross-partition transactional admin. Each is out-of-scope for this proposal but each would be an extension of the design. `partial` leaves room for those without forcing them into a separate proposal track. If you'd prefer `implemented`, I can amend the rename — happy to take direction. ## Changes - **`git mv`** preserves history via 97% similarity-based rename detection — `git log --follow` continues to trace prior revisions. - **Status header** updated from `Proposed` to `Partial` to mirror the filename token. - **§11 rollout table** annotated with shipped status anchored to merge PR numbers, plus a "Status as of 2026-05-04" header paragraph explaining the partial-vs-implemented call. - **In-tree source-comment cross-references** updated from the proposed-stage filename to the partial-stage filename: - `main_sqs_leadership_refusal.go` - `shard_config.go` - `adapter/sqs_keys.go` - `adapter/sqs_partitioning.go` - `adapter/sqs_catalog.go` These are doc-only comments; no behaviour change. ## Self-review (5 lenses, abbreviated) 1. **Data loss** — N/A; doc-only and comment-only changes. 2. **Concurrency** — N/A. 3. **Performance** — N/A. 4. **Data consistency** — N/A. 5. **Test coverage** — no tests added; doc-and-comment change only. ## Test plan - [x] `go build ./...` clean. - [x] `golangci-lint --config=.golangci.yaml run` clean (pre-commit hook auto-ran). - [x] No remaining references to the old filename anywhere in the tree (Go / Clojure / Markdown). ## Refs - `docs/design/2026_04_26_partial_sqs_split_queue_fifo.md` §11. - `docs/design/README.md` "Filename convention" + "Moving or renaming". <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Updated design document references across multiple source files from the proposed version to the partial version. * Updated the design document status and rollout plan to reflect the current shipped state. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
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