Skip to content

Commit 7f3a643

Browse files
committed
fix(sqs): buildSQSPartitionResolver returns interface to avoid typed-nil
Round 1 Claude review on PR #715 caught a typed-nil interface bug: buildSQSPartitionResolver had return type *adapter.SQSPartitionResolver, so on a non-partitioned cluster it returned a typed-nil pointer. When that pointer was passed into kv.ShardRouter.WithPartitionResolver (parameter type kv.PartitionResolver), Go wrapped it into a NON-NIL interface — the resolver-first short-circuit `s.partitionResolver != nil` would always pass on every request, defeating the "non-partitioned cluster keeps engine-only hot path" contract from the PR description. The (*SQSPartitionResolver).ResolveGroup nil-receiver guard kept this functionally safe (correct routing) but not free (extra map lookup per request). Fix - Return type changed from *adapter.SQSPartitionResolver to kv.PartitionResolver. Untyped `nil` returns now propagate as a true nil interface, the short-circuit fires correctly, and the hot path stays engine-only. - Defensive nil guard after NewSQSPartitionResolver in case canonicalisation collapses every entry — the typed pointer from the constructor would otherwise wrap to a non-nil interface even when its underlying pointer is nil. - New main_sqs_resolver_test.go regression test: TestBuildSQSPartitionResolver_NilOnEmpty uses requireNilInterface to force the kv.PartitionResolver conversion at the call boundary (a plain require.Nil on the concrete pointer would pass even with the bug present, since the pointer itself IS nil — only the interface wrap exposes the failure mode). - TestBuildSQSPartitionResolver_NonEmptyReturnsResolver pins the happy path so a future "always return nil" regression is caught.
1 parent 8bbfcb9 commit 7f3a643

2 files changed

Lines changed: 93 additions & 2 deletions

File tree

main.go

Lines changed: 22 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -486,13 +486,24 @@ func buildLeaderSQS(groups []groupSpec, sqsAddr string, raftSqsMap string) (map[
486486
// is a documented no-op, so the request hot path keeps the existing
487487
// engine-only dispatch.
488488
//
489+
// Return type is the kv.PartitionResolver interface, NOT the
490+
// concrete *adapter.SQSPartitionResolver, because Go wraps a typed
491+
// nil pointer into a NON-NIL interface value when the function
492+
// signature is the concrete type. With a concrete return type, a
493+
// non-partitioned cluster would carry a non-nil interface whose
494+
// underlying pointer is nil, the resolver-first short-circuit
495+
// `s.partitionResolver != nil` would always pass, and every request
496+
// would pay an extra ResolveGroup call (which the nil-receiver
497+
// guard makes safe but not free). The interface return type makes
498+
// the untyped `nil` propagate as a true nil interface.
499+
//
489500
// The group-reference parsing here cannot fail in practice because
490501
// parseSQSFifoGroupList already canonicalized each entry as a
491502
// uint64 string at flag-parse time; the conversion is repeated
492503
// defensively so a future caller that bypasses parseSQSFifoGroupList
493504
// (e.g. a test seeding the map programmatically) gets a clear panic
494505
// instead of a silent route-to-group-zero.
495-
func buildSQSPartitionResolver(partitionMap map[string]sqsFifoQueueRouting) *adapter.SQSPartitionResolver {
506+
func buildSQSPartitionResolver(partitionMap map[string]sqsFifoQueueRouting) kv.PartitionResolver {
496507
if len(partitionMap) == 0 {
497508
return nil
498509
}
@@ -514,7 +525,16 @@ func buildSQSPartitionResolver(partitionMap map[string]sqsFifoQueueRouting) *ada
514525
}
515526
flat[queue] = ids
516527
}
517-
return adapter.NewSQSPartitionResolver(flat)
528+
r := adapter.NewSQSPartitionResolver(flat)
529+
if r == nil {
530+
// Defensive: NewSQSPartitionResolver returns nil on an
531+
// empty input. The len-check above already short-circuits
532+
// for empty partitionMap, so reaching this branch means
533+
// the canonicalisation collapsed every entry — surface as
534+
// a true nil interface, not a typed-nil pointer wrapper.
535+
return nil
536+
}
537+
return r
518538
}
519539

520540
// buildSQSFifoPartitionMap parses and validates the

main_sqs_resolver_test.go

Lines changed: 71 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,71 @@
1+
package main
2+
3+
import (
4+
"testing"
5+
6+
"github.com/bootjp/elastickv/kv"
7+
"github.com/stretchr/testify/require"
8+
)
9+
10+
// TestBuildSQSPartitionResolver_NilOnEmpty pins the typed-nil
11+
// interface invariant flagged by the Phase 3.D PR 4-B-2 round-1
12+
// review: buildSQSPartitionResolver MUST return a true-nil
13+
// kv.PartitionResolver interface (not a typed-nil pointer wrapped
14+
// in a non-nil interface) when the partition map is empty or nil.
15+
//
16+
// If the function's return type were the concrete
17+
// *adapter.SQSPartitionResolver and the body returned a nil
18+
// pointer, Go would wrap that pointer into a NON-NIL interface
19+
// when assigned to kv.ShardRouter.partitionResolver. The router's
20+
// resolver-first short-circuit `s.partitionResolver != nil` would
21+
// always pass on a non-partitioned cluster, and every request
22+
// would pay an extra ResolveGroup call (which the nil-receiver
23+
// guard inside (*SQSPartitionResolver).ResolveGroup makes safe
24+
// but not free).
25+
//
26+
// requireNilInterface forces the interface conversion at the call
27+
// boundary, which is what makes this regression observable. A
28+
// plain require.Nil(t, r) would pass even with a typed-nil because
29+
// at that point r still has the concrete pointer type.
30+
func TestBuildSQSPartitionResolver_NilOnEmpty(t *testing.T) {
31+
t.Parallel()
32+
cases := []struct {
33+
name string
34+
in map[string]sqsFifoQueueRouting
35+
}{
36+
{"nil map", nil},
37+
{"empty map", map[string]sqsFifoQueueRouting{}},
38+
}
39+
for _, tc := range cases {
40+
t.Run(tc.name, func(t *testing.T) {
41+
t.Parallel()
42+
requireNilInterface(t, buildSQSPartitionResolver(tc.in),
43+
"buildSQSPartitionResolver("+tc.name+
44+
") must produce a true nil interface, "+
45+
"not a typed-nil pointer wrapper")
46+
})
47+
}
48+
}
49+
50+
// TestBuildSQSPartitionResolver_NonEmptyReturnsResolver pins the
51+
// happy path: a non-empty partition map yields a non-nil resolver
52+
// that dispatches partition keys correctly.
53+
func TestBuildSQSPartitionResolver_NonEmptyReturnsResolver(t *testing.T) {
54+
t.Parallel()
55+
in := map[string]sqsFifoQueueRouting{
56+
"orders.fifo": {partitionCount: 2, groups: []string{"10", "11"}},
57+
}
58+
r := buildSQSPartitionResolver(in)
59+
require.NotNil(t, r,
60+
"non-empty partition map must produce a non-nil resolver")
61+
}
62+
63+
// requireNilInterface accepts a kv.PartitionResolver and asserts
64+
// the interface value (NOT just the underlying pointer) is nil.
65+
// The function-parameter conversion forces a typed-nil pointer to
66+
// be wrapped into a non-nil interface, which is exactly the
67+
// failure mode the regression test guards against.
68+
func requireNilInterface(t *testing.T, r kv.PartitionResolver, msg string) {
69+
t.Helper()
70+
require.Nil(t, r, msg)
71+
}

0 commit comments

Comments
 (0)