Skip to content

Commit 87561fd

Browse files
committed
fix(sqs): RecognisesPartitionedKey checks prefix only, not full parse
Round 5 review on PR #715 raised a nit: RecognisesPartitionedKey delegated to parsePartitionedSQSKey, which fails when the queue segment is malformed base64 or the partition segment is truncated. For such corrupt-shape keys the predicate returned false, so the router fell through to the engine and silently routed via routeKey's !sqs|route|global collapse to the SQS catalog default group — exactly the failure mode the round 5 fail-closed change was introduced to prevent. The fix is the structural-only intent the kv.PartitionResolver contract already documents: "Implementations answer purely on prefix / structural inspection". Make RecognisesPartitionedKey match the intent — accept ANY key that starts with one of the partitioned family prefixes, regardless of subsequent corruption. ResolveGroup still returns (0, false) for malformed keys, and the router pairs that with Recognised=true to fail closed. Tests - TestSQSPartitionResolver_RecognisesMalformedPartitionedKey: three sub-cases pin the new contract — prefix-only, prefix + invalid base64 queue segment, prefix + valid queue + '|' but truncated partition bytes. All assert Recognised=true and ResolveGroup ok=false, which is exactly the fail-closed pairing the router consumes. - Existing TestSQSPartitionResolver_RecognisesPartitionedKey cases stay valid (the well-formed shapes still match).
1 parent b4bf81c commit 87561fd

2 files changed

Lines changed: 60 additions & 8 deletions

File tree

adapter/sqs_partition_resolver.go

Lines changed: 9 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -99,13 +99,14 @@ func (r *SQSPartitionResolver) ResolveGroup(key []byte) (uint64, bool) {
9999
}
100100

101101
// RecognisesPartitionedKey reports whether key has the structural
102-
// shape of a partitioned-SQS key — a partitioned family prefix
103-
// followed by an encoded queue segment, a '|' terminator, and a
104-
// fixed-width uint32 partition index. Implementations of
105-
// kv.PartitionResolver must answer purely on shape so the router
106-
// can use the predicate to decide between fall-through (not
107-
// partitioned) and fail-closed (partitioned but unresolved); see
108-
// kv.PartitionResolver doc and codex P1 round 2 on PR #715.
102+
// shape of a partitioned-SQS key — i.e. starts with one of the
103+
// partitioned family prefixes. The check is PREFIX-ONLY, not a
104+
// full parse: a key with a partitioned prefix followed by a
105+
// malformed queue / partition segment still answers true, so the
106+
// router fails closed via kv.PartitionResolver semantics instead
107+
// of falling through to the engine and silently routing to the
108+
// SQS catalog default group via routeKey's !sqs|route|global
109+
// collapse (round 5 review nit on PR #715).
109110
//
110111
// A nil receiver returns false so kv.ShardRouter's typed-nil case
111112
// (ResolveGroup(nil) == (0, false)) pairs with an honest "I don't
@@ -114,7 +115,7 @@ func (r *SQSPartitionResolver) RecognisesPartitionedKey(key []byte) bool {
114115
if r == nil || len(key) == 0 {
115116
return false
116117
}
117-
_, _, ok := parsePartitionedSQSKey(key)
118+
_, ok := stripPartitionedFamilyPrefix(key)
118119
return ok
119120
}
120121

adapter/sqs_partition_resolver_test.go

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -280,6 +280,57 @@ func TestSQSPartitionResolver_RecognisesPartitionedKey_NilReceiver(t *testing.T)
280280
require.False(t, r.RecognisesPartitionedKey([]byte("any-key")))
281281
}
282282

283+
// TestSQSPartitionResolver_RecognisesMalformedPartitionedKey pins
284+
// the prefix-only check (round 5 nit on PR #715): a key with a
285+
// valid partitioned family prefix but a corrupt / truncated queue
286+
// or partition segment MUST still be recognised so the router
287+
// fails closed rather than falling through to engine routing
288+
// (which would silently mis-route to the SQS catalog default
289+
// group via routeKey's !sqs|route|global collapse).
290+
func TestSQSPartitionResolver_RecognisesMalformedPartitionedKey(t *testing.T) {
291+
t.Parallel()
292+
r := NewSQSPartitionResolver(map[string][]uint64{
293+
"q.fifo": {10, 11},
294+
})
295+
cases := []struct {
296+
name string
297+
key []byte
298+
}{
299+
{
300+
// Prefix only, nothing after — parsePartitionedSQSKey
301+
// would fail on the missing '|' terminator, but the
302+
// shape IS recognised.
303+
name: "prefix only",
304+
key: []byte(SqsPartitionedMsgDataPrefix),
305+
},
306+
{
307+
// Prefix + base64 garbage that decodes invalidly.
308+
// parsePartitionedSQSKey would fail at decodeSQSSegment.
309+
name: "prefix + invalid base64",
310+
key: []byte(SqsPartitionedMsgDataPrefix + "!!!|"),
311+
},
312+
{
313+
// Prefix + valid queue segment + '|' but no partition
314+
// bytes (truncated).
315+
name: "prefix + queue + truncated partition",
316+
key: []byte(SqsPartitionedMsgVisPrefix + encodeSQSSegment("q.fifo") + "|"),
317+
},
318+
}
319+
for _, tc := range cases {
320+
t.Run(tc.name, func(t *testing.T) {
321+
t.Parallel()
322+
require.True(t, r.RecognisesPartitionedKey(tc.key),
323+
"malformed partitioned key MUST be recognised so "+
324+
"the router fails closed rather than mis-routing "+
325+
"through the engine's SQS catalog default")
326+
gid, ok := r.ResolveGroup(tc.key)
327+
require.False(t, ok,
328+
"malformed key cannot resolve")
329+
require.Equal(t, uint64(0), gid)
330+
})
331+
}
332+
}
333+
283334
// TestSQSPartitionResolver_PrefixesAlign pins that the resolver's
284335
// family-prefix list matches the constants in sqs_keys.go exactly.
285336
// A future renamed prefix or added family that touches sqs_keys.go

0 commit comments

Comments
 (0)