fix(sqs/htfifo): address PR #681 review feedback (Gemini medium x3)

Three medium-priority Gemini findings + one cyclop fix from the
rebase onto the updated PR #679 throttling branch.

(1) sqsErrInvalidParameterValue removed (sqs_catalog.go,
sqs_partitioning.go, sqs_partitioning_test.go): the constant
duplicated sqsErrValidation which already carries
"InvalidParameterValue". Reuses the existing constant; comment on
the validator clarifies the choice.

(2) snapshotImmutableHTFIFO heap allocation gated on
htfifoAttributesPresent (sqs_catalog.go): the snapshot is only
needed when the request actually carries an HT-FIFO attribute. A
SetQueueAttributes that touches only mutable attributes (e.g.
VisibilityTimeout — the common path) now skips the allocation
entirely. The validator short-circuit on preApply == nil keeps
correctness identical: a request with no HT-FIFO attribute can
never change an HT-FIFO field.

(3) partitionFor inlined FNV-1a (sqs_partitioning.go): the
hash/fnv.New64a + h.Write([]byte(messageGroupID)) path heap-
allocates the byte slice and pays the hash.Hash interface dispatch
on every SendMessage. Replaced with a manual FNV-1a loop over the
string — measurably faster on the routing hot path. MessageGroupId
is capped at 128 chars by validation so the loop is bounded.

(4) cyclop (trySetQueueAttributesOnce): the conditional preApply
branch + immutability check + Throttle validator pushed cyclop to
12. Extracted applyAndValidateSetAttributes so the function stays
under the ceiling; the helper is also useful for any future
admin-surface callers that want the same apply+validate flow
without the OCC dispatch.

All tests pass under -race; golangci-lint clean.

Author: bootjp

adapter/sqs_catalog.go

@@ -60,12 +60,6 @@ const (
 	sqsErrQueueDoesNotExist     = "AWS.SimpleQueueService.NonExistentQueue"
 	sqsErrInvalidAttributeName  = "InvalidAttributeName"
 	sqsErrInvalidAttributeValue = "InvalidAttributeValue"
-	// sqsErrInvalidParameterValue is the AWS code for incoherent
-	// parameter combinations (vs malformed individual values, which
-	// use sqsErrInvalidAttributeValue). HT-FIFO uses this for the
-	// {PartitionCount > 1, DeduplicationScope = "queue"} cross-
-	// attribute rejection.
-	sqsErrInvalidParameterValue = "InvalidParameterValue"
 )
 
 var sqsQueueNamePattern = regexp.MustCompile(`^[a-zA-Z0-9_-]{1,80}(\.fifo)?$`)
@@ -1376,43 +1370,56 @@ func (s *SQSServer) setQueueAttributesWithRetry(ctx context.Context, queueName s
 	return newSQSAPIError(http.StatusInternalServerError, sqsErrInternalFailure, "set queue attributes retry attempts exhausted")
 }
 
-// trySetQueueAttributesOnce is one read-validate-commit pass. The first
-// return reports whether the caller should stop retrying (the attrs
-// are now committed); an error means either a non-retryable failure
-// (propagate) or a retryable write conflict (retry after backoff).
-func (s *SQSServer) trySetQueueAttributesOnce(ctx context.Context, queueName string, attrs map[string]string) (bool, error) {
-	readTS := s.nextTxnReadTS(ctx)
-	meta, exists, err := s.loadQueueMetaAt(ctx, queueName, readTS)
-	if err != nil {
-		return false, errors.WithStack(err)
-	}
-	if !exists {
-		return false, newSQSAPIError(http.StatusBadRequest, sqsErrQueueDoesNotExist, "queue does not exist")
+// applyAndValidateSetAttributes runs the apply + cross-validator
+// chain for a SetQueueAttributes request. Extracted from
+// trySetQueueAttributesOnce so that function stays under the cyclop
+// ceiling once HT-FIFO immutability + Throttle validators were
+// added. Returns nil on success; on rejection returns the typed
+// sqsAPIError the caller forwards to writeSQSErrorFromErr.
+//
+// preApply snapshot allocation is gated on htfifoAttributesPresent
+// so the common "mutable-only update" path stays alloc-free per the
+// Gemini medium feedback on PR #681.
+func applyAndValidateSetAttributes(meta *sqsQueueMeta, attrs map[string]string) error {
+	var preApply *sqsQueueMeta
+	if htfifoAttributesPresent(attrs) {
+		preApply = snapshotImmutableHTFIFO(meta)
 	}
-	// Snapshot the on-disk values of the immutable HT-FIFO fields
-	// before applying the request so the immutability check has a
-	// clean before/after pair. SetQueueAttributes is all-or-nothing
-	// per §3.2: if any immutable attribute carries a differing value,
-	// the entire request is rejected before any attribute is
-	// persisted (including mutable attributes in the same call).
-	preApply := snapshotImmutableHTFIFO(meta)
 	if err := applyAttributes(meta, attrs); err != nil {
-		return false, err
+		return err
 	}
-	if err := validatePartitionImmutability(preApply, meta); err != nil {
-		return false, err
+	if preApply != nil {
+		if err := validatePartitionImmutability(preApply, meta); err != nil {
+			return err
+		}
 	}
 	// ContentBasedDeduplication is FIFO-only; a Standard queue
 	// silently accepting it would advertise unsupported behavior to
 	// clients. Same rule enforced on CreateQueue.
 	if meta.ContentBasedDedup && !meta.IsFIFO {
-		return false, newSQSAPIError(http.StatusBadRequest, sqsErrInvalidAttributeValue, "ContentBasedDeduplication is only valid on FIFO queues")
+		return newSQSAPIError(http.StatusBadRequest, sqsErrInvalidAttributeValue, "ContentBasedDeduplication is only valid on FIFO queues")
 	}
 	// HT-FIFO schema validator runs after applyAttributes so the
 	// FIFO-only checks see the post-apply state. IsFIFO comes from
 	// the loaded meta record (immutable from CreateQueue) so the
 	// validator sees the same flag CreateQueue set.
-	if err := validatePartitionConfig(meta); err != nil {
+	return validatePartitionConfig(meta)
+}
+
+// trySetQueueAttributesOnce is one read-validate-commit pass. The first
+// return reports whether the caller should stop retrying (the attrs
+// are now committed); an error means either a non-retryable failure
+// (propagate) or a retryable write conflict (retry after backoff).
+func (s *SQSServer) trySetQueueAttributesOnce(ctx context.Context, queueName string, attrs map[string]string) (bool, error) {
+	readTS := s.nextTxnReadTS(ctx)
+	meta, exists, err := s.loadQueueMetaAt(ctx, queueName, readTS)
+	if err != nil {
+		return false, errors.WithStack(err)
+	}
+	if !exists {
+		return false, newSQSAPIError(http.StatusBadRequest, sqsErrQueueDoesNotExist, "queue does not exist")
+	}
+	if err := applyAndValidateSetAttributes(meta, attrs); err != nil {
 		return false, err
 	}
 	meta.LastModifiedAtMillis = time.Now().UnixMilli()

adapter/sqs_partitioning.go

@@ -1,7 +1,6 @@
 package adapter
 
 import (
-	"hash/fnv"
 	"net/http"
 	"strconv"
 )
@@ -80,14 +79,27 @@ func partitionFor(meta *sqsQueueMeta, messageGroupID string) uint32 {
 	if messageGroupID == "" {
 		return 0
 	}
-	h := fnv.New64a()
-	_, _ = h.Write([]byte(messageGroupID))
+	// Inlined FNV-1a over the string to avoid the []byte allocation
+	// hash/fnv.New64a + h.Write would force (Gemini medium on PR
+	// #681). MessageGroupId is capped at 128 chars by validation, so
+	// this loop bounds at 128 iterations of integer arithmetic per
+	// SendMessage — measurably faster than the hash.Hash interface
+	// path on the routing hot path.
+	const (
+		fnv64Offset uint64 = 14695981039346656037
+		fnv64Prime  uint64 = 1099511628211
+	)
+	hash := fnv64Offset
+	for i := 0; i < len(messageGroupID); i++ {
+		hash ^= uint64(messageGroupID[i])
+		hash *= fnv64Prime
+	}
 	// PartitionCount is a power of two (validator-enforced); mod is
 	// equivalent to mask-AND. The mask is meta.PartitionCount - 1.
 	// Computing the mask in uint64 first then narrowing to uint32 is
 	// safe because htfifoMaxPartitions == 32 fits in uint32 trivially.
 	mask := uint64(meta.PartitionCount - 1)
-	return uint32(h.Sum64() & mask) //nolint:gosec // masked by (PartitionCount - 1) ≤ htfifoMaxPartitions − 1, fits in uint32.
+	return uint32(hash & mask) //nolint:gosec // masked by (PartitionCount - 1) ≤ htfifoMaxPartitions − 1, fits in uint32.
 }
 
 // isPowerOfTwo returns true when n is a positive power of two.
@@ -140,7 +152,10 @@ func validatePartitionConfig(meta *sqsQueueMeta) error {
 		}
 	}
 	if meta.PartitionCount > 1 && meta.DeduplicationScope == htfifoDedupeScopeQueue {
-		return newSQSAPIError(http.StatusBadRequest, sqsErrInvalidParameterValue,
+		// sqsErrValidation is "InvalidParameterValue" (Gemini medium
+		// on PR #681 — uses the existing constant rather than a
+		// duplicate-value alias).
+		return newSQSAPIError(http.StatusBadRequest, sqsErrValidation,
 			"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")
 	}
 	return nil

adapter/sqs_partitioning_test.go

@@ -187,8 +187,8 @@ func TestValidatePartitionConfig_RejectsQueueScopedDedupOnPartitioned(t *testing
 	require.Error(t, err)
 	var apiErr *sqsAPIError
 	require.True(t, errors.As(err, &apiErr), "expected sqsAPIError, got %T", err)
-	require.Equal(t, sqsErrInvalidParameterValue, apiErr.errorType,
-		"the cross-attribute rejection must use InvalidParameterValue (incoherent params), not InvalidAttributeValue (malformed individual value)")
+	require.Equal(t, sqsErrValidation, apiErr.errorType,
+		"the cross-attribute rejection must use InvalidParameterValue (incoherent params, sqsErrValidation), not InvalidAttributeValue (malformed individual value)")
 	// Single-partition + queue-scoped dedup is fine (legacy behaviour).
 	require.NoError(t, validatePartitionConfig(&sqsQueueMeta{
 		IsFIFO:             true,