Skip to content

Commit d246cc3

Browse files
committed
docs(sqs): address thirteenth-round Claude lows (dead-code clarification, PR 4 hook scope)
Two low-severity findings from the thirteenth-round Claude review on PR #664: 3.D split-queue FIFO §3.2 -- cross-attribute rule at SetQueueAttributes is dead code (Claude low): the prior wording said the rule applies at both CreateQueue and SetQueueAttributes, but immutability fires first on SetQueueAttributes so the {PartitionCount > 1, DeduplicationScope = queue} combination can never reach the cross-attribute check on that path. Added a clarifying paragraph so PR 2 author knows the SetQueueAttributes side is unreachable rather than missing test coverage. 3.D split-queue FIFO §11 PR 4 -- "Mixed-version gate" expanded (Claude low): the prior label could be read as covering only the §8.5 capability advertisement (/sqs_health + catalog polling). The §8 leadership-refusal hook in kv/lease_state.go is a distinct implementation concern and §8 explicitly says it must land before PR 4 marks the binary htfifo-eligible. Expanded the parenthetical to enumerate both components so the PR 4 author cannot miss the runtime hook.
1 parent fc7c3cb commit d246cc3

1 file changed

Lines changed: 3 additions & 1 deletion

File tree

docs/design/2026_04_26_proposed_sqs_split_queue_fifo.md

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -114,6 +114,8 @@ The pattern is the same: each attribute participates in a routing or dedup decis
114114

115115
**Cross-attribute validation at `CreateQueue` and `SetQueueAttributes`** (Codex P1 on PR #664 tenth-round Codex review): the same validator that enforces immutability also rejects incoherent attribute combinations *before* the queue is created. Specifically, `{PartitionCount > 1, DeduplicationScope = "queue"}` is rejected with `InvalidParameterValue` ("queue-scoped deduplication is incompatible with multi-partition FIFO because the dedup key cannot be globally unique across partitions without a cross-partition OCC transaction"). Without this control-plane gate, an operator who mis-configures the combination at `CreateQueue` gets a successful response and only discovers the problem when every subsequent `SendMessage` fails — a created-but-unserviceable queue with no recovery short of `DeleteQueue`+`CreateQueue`. Any other invalid combinations the implementation discovers during PR 2 should land in the same validator (and the §4.x rejection paragraphs that mention them should be reframed as "cannot reach this code" notes, not runtime rejections).
116116

117+
In practice this rule is only reachable at `CreateQueue` time. `SetQueueAttributes` cannot present the `{PartitionCount > 1, DeduplicationScope = "queue"}` combination because both `PartitionCount` and `DeduplicationScope` are immutable: any change to either is caught by the immutability check (described in the next paragraph) before the cross-attribute check has the chance to run. Implementing the cross-attribute rule on the `SetQueueAttributes` path would therefore be dead code — it is harmless to add for symmetry, but PR 2's author should know it is unreachable rather than spend time hunting for the test case that hits it.
118+
117119
**Enforcement (gate is `CreateQueue`, not first send)**: when `SetQueueAttributes` is called, the validator loads the current `sqsQueueMeta` (one Raft-consistent point read against the catalog — already required for the OCC compare-and-set) and rejects with `InvalidAttributeValue` if any of the three attributes in the request differs from the value already on the meta. **`SetQueueAttributes` is all-or-nothing**: if any immutable attribute in the request carries a differing value, the entire request is rejected before any attribute is persisted — including the *mutable* attributes in the same call (e.g. `VisibilityTimeout` paired with an attempted `PartitionCount` change is rejected as a whole; the `VisibilityTimeout` change does not commit on its own). The §9 immutability test pins this rule. No range scan over the message keyspace is required; no `firstSendAt` timestamp needs to be added; no concurrent-send race exists, because the meta value is set once at `CreateQueue` commit and never changes thereafter. This matches AWS's published behaviour ("you can't change the queue type after you create it" extends to `FifoThroughputLimit` and `DeduplicationScope` in HT-FIFO queues), so SDK clients see the same rejection envelope on the same request as on AWS proper. Picking the create-time gate over a first-send gate is also defensible from a correctness lens: the corner case "operator creates a queue with `PartitionCount=8` and changes their mind to `PartitionCount=4` before any producer connects" is rare and can be solved by `DeleteQueue`+`CreateQueue` (which the operator can also do post-first-send for any other reason). The simplicity of a stateless validator is worth more than the vanishingly small set of operators who would benefit from a brief mutability window.
118120

119121
### 3.3 Routing
@@ -370,7 +372,7 @@ This is out of scope here.
370372
| 1 | This proposal doc lands. Operators have time to flag concerns. | Yes |
371373
| 2 | Schema: `sqsQueueMeta.PartitionCount`, `DeduplicationScope`, `FifoThroughputLimit`. Routing function `partitionFor`. CreateQueue / SetQueueAttributes validation including the §3.2 cross-attribute rules. **Temporary feature gate** (see below): `CreateQueue` rejects `PartitionCount > 1` with `InvalidAttributeValue` ("PartitionCount > 1 requires HT-FIFO data plane — not yet enabled") so the schema field exists in the meta type but cannot land in production data. | Yes (catalog only) |
372374
| 3 | Keyspace: thread `partitionIndex` through every `sqsMsg*Key` constructor, defaulting to 0 so existing queues stay byte-identical. Gate from PR 2 still in place — `PartitionCount > 1` remains rejected. | Yes (mechanical) |
373-
| 4 | Routing layer: `kv/shard_router.go` accepts the `(queue, partition)` key. New `--sqsFifoPartitionMap` flag (separate from the existing `--raftSqsMap` endpoint-mapping flag). Mixed-version gate. PR 2's temporary `PartitionCount > 1` rejection still in place. | Yes (operator-config) |
375+
| 4 | Routing layer: `kv/shard_router.go` accepts the `(queue, partition)` key. New `--sqsFifoPartitionMap` flag (separate from the existing `--raftSqsMap` endpoint-mapping flag). Mixed-version gate (§8.5 capability advertisement via `/sqs_health` + catalog polling for `CreateQueue` gating, **and** the §8 leadership-refusal hook in `kv/lease_state.go` that calls `TransferLeadership` when a non-`htfifo` binary discovers a partitioned queue in its shard on startup or leadership acquisition — both components are required before the binary is marked `htfifo`-eligible). PR 2's temporary `PartitionCount > 1` rejection still in place. | Yes (operator-config) |
374376
| 5 | Send / Receive partition fanout. Receipt-handle v2 codec. **Removes the PR 2 `PartitionCount > 1` rejection** in the same commit that wires the data-plane fanout — the gate and its lift land atomically so a half-deployed cluster can never accept a partitioned queue without the data plane to serve it. | Yes (data-plane) |
375377
| 6 | PurgeQueue / DeleteQueue partition iteration. Tombstone schema update. Reaper update. | Yes (control-plane) |
376378
| 7 | Jepsen HT-FIFO workload. Metrics. | Yes (testing) |

0 commit comments

Comments
 (0)