diff --git a/adapter/dynamodb_lease.go b/adapter/dynamodb_lease.go index 638b5e7b4..9e377f489 100644 --- a/adapter/dynamodb_lease.go +++ b/adapter/dynamodb_lease.go @@ -297,26 +297,21 @@ func (d *DynamoDBServer) queryLeaseKey(ctx context.Context, in queryInput) ([]by } // queryLeasePrefix resolves the single hash-key prefix a base-table Query -// reads, classifying the read into queryLeaseSingleGroup (resolved prefix), -// queryLeaseAllGroups (whole-table prefix: a valid multi-shard read), or +// reads, classifying the read into queryLeaseSingleGroup (resolved prefix) or // queryLeaseSkip (malformed KeyConditionExpression: a validation error the -// read path rejects identically). The validation error is deliberately not +// read path rejects identically). It is only reached without an IndexName, so +// the key schema is always the primary key; GSI queries take +// gsiQueryLeasePlan instead. The validation error is deliberately not // surfaced — only the routing classification matters here, and the read path // reports the identical error downstream. func queryLeasePrefix(in queryInput, schema *dynamoTableSchema) ([]byte, queryLeasePlan) { - keySchema, cond, err := resolveQueryCondition(in, schema) + _, cond, err := resolveQueryCondition(in, schema) if err != nil { // Malformed/unsupported KeyConditionExpression: a deterministic // ValidationException the read path produces without touching data. return nil, queryLeaseSkip } - // A query whose key schema hash key differs from the primary hash - // key reads the whole-table prefix (see queryScanPrefix), which can - // span multiple shards; let the all-groups check cover them. - if keySchema.HashKey != schema.PrimaryKey.HashKey { - return nil, queryLeaseAllGroups - } - prefix, err := queryScanPrefix(schema, in, keySchema, cond.hashValue) + prefix, err := queryScanPrefix(schema, in, cond.hashValue) if err != nil { // queryScanPrefix only fails on an unparseable hash-key value — a // ValidationException the read path rejects identically. diff --git a/adapter/dynamodb_lease_validation_test.go b/adapter/dynamodb_lease_validation_test.go index 6859fa190..618133a34 100644 --- a/adapter/dynamodb_lease_validation_test.go +++ b/adapter/dynamodb_lease_validation_test.go @@ -545,3 +545,91 @@ func TestDynamoDB_LeaseCheckQuery_InvalidExclusiveStartKeySkipsLease(t *testing. require.Equal(t, 0, coord.allGroupsCalls, "invalid ExclusiveStartKey must not trigger the lease check — the transaction is doomed by validation") } + +// TestQueryLeasePrefix_BaseTableClassification pins the base-table lease plan. +// queryLeasePrefix is reached only for a Query without IndexName, whose key +// schema is always the primary key, so a valid condition resolves to the +// single hash-key prefix and anything else is a validation skip: the +// base-table path never classifies as an all-groups read. +func TestQueryLeasePrefix_BaseTableClassification(t *testing.T) { + t.Parallel() + + schema := leaseGSIFixtureSchema() + pinned := queryInput{ + TableName: "t", + KeyConditionExpression: "pk = :pk", + ExpressionAttributeValues: map[string]attributeValue{":pk": newStringAttributeValue("tenant")}, + } + hashSegment, err := attributeValueAsKeySegment(newStringAttributeValue("tenant")) + require.NoError(t, err) + wantPrefix := dynamoItemHashPrefixForTable(schema.TableName, schema.Generation, hashSegment) + + for _, tc := range []struct { + name string + in queryInput + wantPlan queryLeasePlan + wantPrefix []byte + }{ + {name: "hash key pinned", in: pinned, wantPlan: queryLeaseSingleGroup, wantPrefix: wantPrefix}, + { + name: "condition on a non-key attribute", + in: queryInput{ + TableName: "t", + KeyConditionExpression: "gk = :gk", + ExpressionAttributeValues: map[string]attributeValue{":gk": newStringAttributeValue("x")}, + }, + wantPlan: queryLeaseSkip, + }, + { + name: "malformed condition", + in: queryInput{TableName: "t", KeyConditionExpression: "pk ="}, + wantPlan: queryLeaseSkip, + }, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + prefix, plan := queryLeasePrefix(tc.in, schema) + require.Equal(t, tc.wantPlan, plan) + require.Equal(t, tc.wantPrefix, prefix) + }) + } +} + +// TestQueryScanPrefix_SelectsHashPrefixPerEncoding pins the prefix a Query +// reads for each key encoding: a base-table Query reads its hash-key item +// prefix and a GSI Query reads its index hash prefix. Every caller derives +// the key schema from the same IndexName, so a base-table Query never falls +// back to the whole-table prefix. +func TestQueryScanPrefix_SelectsHashPrefixPerEncoding(t *testing.T) { + t.Parallel() + + hash := newStringAttributeValue("tenant") + segment, err := attributeValueAsKeySegment(hash) + require.NoError(t, err) + legacyKey, err := attributeValueAsKey(hash) + require.NoError(t, err) + + ordered := leaseGSIFixtureSchema() + legacy := leaseGSIFixtureSchema() + legacy.KeyEncodingVersion = 0 + + for _, tc := range []struct { + name string + schema *dynamoTableSchema + index string + want []byte + }{ + {name: "ordered base table", schema: ordered, want: dynamoItemHashPrefixForTable("t", 1, segment)}, + {name: "ordered gsi", schema: ordered, index: "gsi1", want: dynamoGSIHashPrefixForTable("t", 1, "gsi1", segment)}, + {name: "legacy base table", schema: legacy, want: legacyDynamoItemHashPrefixForTable("t", 1, legacyKey)}, + {name: "legacy gsi", schema: legacy, index: "gsi1", want: legacyDynamoGSIHashPrefixForTable("t", 1, "gsi1", legacyKey)}, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + got, err := queryScanPrefix(tc.schema, queryInput{TableName: "t", IndexName: tc.index}, hash) + require.NoError(t, err) + require.Equal(t, tc.want, got) + require.NotEqual(t, dynamoItemPrefixForTable("t", 1), got) + }) + } +} diff --git a/adapter/dynamodb_query_scan.go b/adapter/dynamodb_query_scan.go index 29328f8b1..fa6a6caf4 100644 --- a/adapter/dynamodb_query_scan.go +++ b/adapter/dynamodb_query_scan.go @@ -371,12 +371,12 @@ func (d *DynamoDBServer) loadQueryItemsWithMigration( cond queryCondition, readTS uint64, ) ([]map[string]attributeValue, error) { - items, err := d.queryItemsByKeyCondition(ctx, in, schema, keySchema, cond, readTS) + items, err := d.queryItemsByKeyCondition(ctx, in, schema, cond, readTS) if err != nil { return nil, err } return d.mergeReadItemsFromSourceSchema(schema, keySchema, items, func(sourceSchema *dynamoTableSchema) ([]map[string]attributeValue, error) { - return d.queryItemsByKeyCondition(ctx, in, sourceSchema, keySchema, cond, readTS) + return d.queryItemsByKeyCondition(ctx, in, sourceSchema, cond, readTS) }) } @@ -861,7 +861,7 @@ func resolveQueryReadBounds( if !schema.usesOrderedKeyEncoding() { return dynamoReadBounds{}, false, nil } - basePrefix, err := queryScanPrefix(schema, in, keySchema, cond.hashValue) + basePrefix, err := queryScanPrefix(schema, in, cond.hashValue) if err != nil { return dynamoReadBounds{}, false, err } @@ -1407,14 +1407,13 @@ func (d *DynamoDBServer) queryItemsByKeyCondition( ctx context.Context, in queryInput, schema *dynamoTableSchema, - keySchema dynamoKeySchema, cond queryCondition, readTS uint64, ) ([]map[string]attributeValue, error) { if strings.TrimSpace(in.IndexName) != "" { return d.queryItemsByGSI(ctx, in, schema, cond, readTS) } - scanPrefix, err := queryScanPrefix(schema, in, keySchema, cond.hashValue) + scanPrefix, err := queryScanPrefix(schema, in, cond.hashValue) if err != nil { return nil, err } @@ -1436,11 +1435,10 @@ func (d *DynamoDBServer) queryItemsByGSI( cond queryCondition, readTS uint64, ) ([]map[string]attributeValue, error) { - keySchema, err := schema.keySchemaForQuery(in.IndexName) - if err != nil { + if _, err := schema.keySchemaForQuery(in.IndexName); err != nil { return nil, newDynamoAPIError(http.StatusBadRequest, dynamoErrValidation, err.Error()) } - prefix, err := queryScanPrefix(schema, in, keySchema, cond.hashValue) + prefix, err := queryScanPrefix(schema, in, cond.hashValue) if err != nil { return nil, err } @@ -1691,7 +1689,7 @@ func sendGSIReadResult(ctx context.Context, results chan<- gsiReadResult, result } } -func queryScanPrefix(schema *dynamoTableSchema, in queryInput, keySchema dynamoKeySchema, hashValue attributeValue) ([]byte, error) { +func queryScanPrefix(schema *dynamoTableSchema, in queryInput, hashValue attributeValue) ([]byte, error) { if !schema.usesOrderedKeyEncoding() { hashKey, err := attributeValueAsKey(hashValue) if err != nil { @@ -1700,9 +1698,6 @@ func queryScanPrefix(schema *dynamoTableSchema, in queryInput, keySchema dynamoK if strings.TrimSpace(in.IndexName) != "" { return legacyDynamoGSIHashPrefixForTable(in.TableName, schema.Generation, in.IndexName, hashKey), nil } - if keySchema.HashKey != schema.PrimaryKey.HashKey { - return dynamoItemPrefixForTable(in.TableName, schema.Generation), nil - } return legacyDynamoItemHashPrefixForTable(in.TableName, schema.Generation, hashKey), nil } hashKey, err := attributeValueAsKeySegment(hashValue) @@ -1712,9 +1707,6 @@ func queryScanPrefix(schema *dynamoTableSchema, in queryInput, keySchema dynamoK if strings.TrimSpace(in.IndexName) != "" { return dynamoGSIHashPrefixForTable(in.TableName, schema.Generation, in.IndexName, hashKey), nil } - if keySchema.HashKey != schema.PrimaryKey.HashKey { - return dynamoItemPrefixForTable(in.TableName, schema.Generation), nil - } return dynamoItemHashPrefixForTable(in.TableName, schema.Generation, hashKey), nil } diff --git a/docs/design/2026_10_16_partial_test_suite_cleanup.md b/docs/design/2026_10_16_partial_test_suite_cleanup.md index a555da0c2..e96bf8ed9 100644 --- a/docs/design/2026_10_16_partial_test_suite_cleanup.md +++ b/docs/design/2026_10_16_partial_test_suite_cleanup.md @@ -117,7 +117,7 @@ Behaviour notes for the `LocalCoordinator` move: dispatches are serialized, elem Deviations from the plan, found while implementing: -1. **WP-5 retarget 1 (DynamoDB base-table hash-key mismatch) cannot be written.** `queryLeasePrefix` is only reached from `queryLeaseKey` with an empty `IndexName`, so `keySchemaForQuery` always returns `PrimaryKey` and the `keySchema.HashKey != schema.PrimaryKey.HashKey` branch is unreachable. A base-table key condition that does not pin the hash key fails `buildQueryCondition` and is classified as a skip. The duplicate test was deleted; removing the dead branch is a production change and is listed under Follow-ups. +1. **WP-5 retarget 1 (DynamoDB base-table hash-key mismatch) cannot be written.** `queryLeasePrefix` is only reached from `queryLeaseKey` with an empty `IndexName`, so `keySchemaForQuery` always returns `PrimaryKey` and the `keySchema.HashKey != schema.PrimaryKey.HashKey` branch is unreachable. A base-table key condition that does not pin the hash key fails `buildQueryCondition` and is classified as a skip. The duplicate test was deleted. The dead branch was removed in a follow-up change, with `TestQueryLeasePrefix_BaseTableClassification` pinning that a base-table Query resolves to a single-group prefix or a skip. The twin branches in `queryScanPrefix` were unreachable for the same reason: every caller derives the key schema from the same `IndexName`, and a GSI Query returns the index prefix first. They were removed with the now-unused `keySchema` parameter; `TestQueryScanPrefix_SelectsHashPrefixPerEncoding` pins the base-table and GSI prefix for both key encodings. 2. **WP-6 retarget 3 (pprof/metrics).** The live `PreparePprofServer`/`PrepareMetricsServer` moved to `internal/serverwiring`, whose own tests already pin the address-before-token validation. The root wrappers and their tests were deleted rather than retargeted. 3. **WP-4 retarget 6 (EXEC dispatch timeout).** `redisDispatchTimeout` is a 30 s constant, so a blocking-store test would take 30 s or need a production change. The rewritten test records the deadline on the stream-deletion scan instead. The mutation named in the plan (`context.Background()` for the EXEC dispatch) survives because `txnContext.dispatchContext` bounds the commit context a second time. Removing that second bound (`dispatchContext` returning an unbounded context) fails the test. @@ -628,8 +628,6 @@ Record each mutation in the PR as a row: file and line, the change, the test tha ## Follow-ups (out of scope) -- **Dead DynamoDB lease branch.** Remove the unreachable hash-key-mismatch branch in `queryLeasePrefix` (`adapter/dynamodb_lease.go`), or document why it is kept (deviation 1 above). - - **Product bugs found by the audit.** Each needs its own PR with a failing test first. - SQS FIFO messages in one group sent within the same millisecond can be received out of order. The visibility key orders by `(visibleAtMillis, random messageID)`. `TestSQSServer_FifoGroupLockHoldsAcrossVisibilityExpiry` and `TestSQSServer_FifoSequenceNumberMonotonic` fail roughly 7/30 and 2/30 runs without `-race`. - The Redis TTL-inline migration builds zero scan plans in production. `startupGatedCoordinator` does not forward `LocalLeaderGroupIDs`, and `keyVizLabeledCoordinator` returns nil when the inner value lacks it. Regression since `a36dac7f`.