Skip to content

Commit fedbdcd

Browse files
committed
fix(dynamodb): correct the #1440 guard - explicit block precedence + accurate warning
Review of PR #1443 probed the tip and disproved this PR own claim that the guard keeps an unresolvable value on a path that reaches AWS. The coercion happens upstream, so the value is already gone before the reset loop and nothing is sent for that member either way. The guard removes the DESTRUCTIVE half; what remains is a no-op - and the only output was the immutable-field warning below, which tells the user to RECREATE THE INDEX. The suppression now warns accurately instead, naming the member. Review also caught a source divergence that reintroduced the bug the guard closes: an already-SDK-shaped OnDemandThroughput WINS over the derived members in the translation, so reading the derived spellings reported a member DECLARED that the translation never sends. An explicit read-only block beside a leftover WriteOnDemandThroughputSettings suppressed the write reset and left the old ceiling live. The collector now derives both flags from the explicit block alone when present. Also moves the new declarations above toSdkReplicaGlobalSecondaryIndexes, whose JSDoc they had been inserted underneath, and corrects an inverted rationale comment (a missing entry lets the reset FIRE - the destructive direction, safe only because the translation refuses malformed shapes loudly first). Refs #1440
1 parent f7f94f0 commit fedbdcd

3 files changed

Lines changed: 120 additions & 35 deletions

File tree

docs/changelog-cdkd.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@ The CLAUDE.md `## Known Limitations` section retains the load-bearing summary
1717
---
1818

1919
**Recently Implemented** (2026-08-10):
20-
- ✅ **`AWS::DynamoDB::GlobalTable`: the per-GSI on-demand reset stops firing on a present-but-unresolvable value (issue [#1440](https://github.com/go-to-k/cdkd/issues/1440))** — `src/provisioning/providers/dynamodb-globaltable-provider.ts` + `tests/unit/provisioning/dynamodb-globaltable-provider-gsi-throughput.test.ts`. **The class:** the #1423 per-GSI reset decided "the template DROPPED this member" from the TRANSLATED side, and `toSdkGlobalSecondaryIndexes` runs every value through `toFiniteNumber` — which returns `undefined` for a present-but-unparseable value (an unresolved `{Ref: …}`, `''`, an object). Such a GSI was therefore indistinguishable from a removal and was answered with the `-1` sentinel, **silently CLEARING the ceiling the template was trying to SET**. That is the same silent-wrong-action class the reset exists to remove, and the identical hazard was caught in self-review on the table-level path during #1434's PR (the gate there tests RAW key presence); this closes the per-GSI half, which #1434 deliberately left alone because it needed more than a one-line gate change. **The fix** adds `collectRawOnDemandDeclarations(properties, region)` beside `toSdkGlobalSecondaryIndexes` — a per-`IndexName` map of RAW CFn key presence walking the SAME two sources the translation does (top-level GSI for the write half; the LOCAL replica's index entry for the read half, with the GSI-level spelling as the documented hand-authored fallback). Keeping the two functions adjacent is deliberate: they must agree on where each member lives, and a divergence between them is exactly what would re-open the bug. Malformed shapes report "not declared" rather than throwing — `toSdkGlobalSecondaryIndexes` already refuses a non-array `GlobalSecondaryIndexes` loudly, and this helper is consulted only to SUPPRESS a reset, so the conservative answer leaves the live value alone. **Tests:** 4 units (unresolved intrinsic on the write half; on the read half via the replica entry; the GSI-level read fallback spelling counted as a declaration; and a fence that a GENUINE removal still resets, so the guard narrows the reset rather than disabling it). Revert-proofed — neutering the guard fails exactly the first three while the #1423 fence stays green.
20+
- ✅ **`AWS::DynamoDB::GlobalTable`: the per-GSI on-demand reset stops firing on a present-but-unresolvable value (issue [#1440](https://github.com/go-to-k/cdkd/issues/1440))** — `src/provisioning/providers/dynamodb-globaltable-provider.ts` + `tests/unit/provisioning/dynamodb-globaltable-provider-gsi-throughput.test.ts`. **The class:** the #1423 per-GSI reset decided "the template DROPPED this member" from the TRANSLATED side, and `toSdkGlobalSecondaryIndexes` runs every value through `toFiniteNumber` — which returns `undefined` for a present-but-unparseable value (an unresolved `{Ref: …}`, `''`, an object). Such a GSI was therefore indistinguishable from a removal and was answered with the `-1` sentinel, **silently CLEARING the ceiling the template was trying to SET**. That is the same silent-wrong-action class the reset exists to remove, and the identical hazard was caught in self-review on the table-level path during #1434's PR (the gate there tests RAW key presence); this closes the per-GSI half, which #1434 deliberately left alone because it needed more than a one-line gate change. **The fix** adds `collectRawOnDemandDeclarations(properties, region)` beside `toSdkGlobalSecondaryIndexes` — a per-`IndexName` map of RAW CFn key presence walking the SAME two sources the translation does (top-level GSI for the write half; the LOCAL replica's index entry for the read half, with the GSI-level spelling as the documented hand-authored fallback). Keeping the two functions adjacent is deliberate: they must agree on where each member lives, and a divergence between them is exactly what would re-open the bug. Malformed shapes report "not declared" rather than throwing — `toSdkGlobalSecondaryIndexes` already refuses a non-array `GlobalSecondaryIndexes` loudly, and this helper is consulted only to SUPPRESS a reset, so the conservative answer leaves the live value alone. **Be precise about what the guard buys** — review probed the PR tip and disproved the obvious claim: it does NOT put the value back on a path that reaches AWS. The coercion happens upstream in `toSdkGlobalSecondaryIndexes`, so the unparseable value is already gone before the reset loop and NOTHING is sent for that member either way. What the guard removes is the DESTRUCTIVE half; what remains is a no-op — and a silent no-op is its own trap, since the only output was the immutable-field warning further down, which tells the user to RECREATE THE INDEX (useless and alarming advice for an unresolved intrinsic). The suppression therefore now WARNS, naming the member and saying the limit was left unchanged. Review also caught a **source divergence that reintroduced the very bug**: an already-SDK-shaped `gsi['OnDemandThroughput']` WINS over the derived members in the translation, so reading the derived spellings reported a member DECLARED that the translation never sends — an explicit `{MaxReadRequestUnits: 50}` beside a leftover `WriteOnDemandThroughputSettings` suppressed the write reset and left the old ceiling live, the #1160 silent drop re-created by the guard itself. The collector now derives both flags from the explicit block alone when present. **Tests:** 6 units (unresolved intrinsic on the write half; on the read half via the replica entry; the GSI-level read fallback spelling counted as a declaration; the accurate warning; the explicit-block precedence; and a fence that a GENUINE removal still resets, so the guard narrows the reset rather than disabling it). Revert-proofed — neutering the raw-presence guard fails three, and neutering the explicit-block precedence fails its own test, while the #1423 fence stays green throughout.
2121
- ✅ **`AWS::DynamoDB::GlobalTable`: the TABLE-level on-demand write ceiling resets when the template drops it; the two REPLICA overrides are proven un-resettable and deliberately left alone (issue [#1434](https://github.com/go-to-k/cdkd/issues/1434))** — `src/provisioning/providers/dynamodb-globaltable-provider.ts` + `tests/unit/provisioning/dynamodb-globaltable-provider-gsi-throughput.test.ts`. **The class:** removing `WriteOnDemandThroughputSettings` from a template never set `flatChanged`, so no `UpdateTable` went out and the old ceiling stayed live in AWS while cdkd reported success — the absent-field-reset silent drop (#1160), the table-level sibling of the per-GSI case #1423 closed. **The fix** merges per member rather than branching on "the whole block disappeared" (the shape the #1433 review caught: a block can survive with its member dropped, which is the likelier user edit) and is SUPPRESSED while the billing mode is flipping, since dropping the on-demand block on the way to PROVISIONED is the natural template edit rather than a clear request, and step 4's flip owns that call. The `oldBilling` / `newBilling` pair is now resolved ONCE above step 3 and reused by step 4, so the two sites cannot disagree about what "flipping" means. **The issue asked for three fields; only ONE turned out to be implementable, and the live probes are what settled it** (us-east-1 source + us-west-2 replica, one global table per probe, both torn down — transcripts on the issue). Table-level `-1` behaves exactly as the issue assumed: `{MaxWriteRequestUnits: -1}` was accepted and `DescribeTable` then returned `{MaxReadRequestUnits: 100}`, i.e. the dropped member cleared and the untouched sibling preserved, with the reset reading back as ABSENCE never as -1 — verified independently rather than inherited from #1423, since reset semantics are field-specific. The two REPLICA overrides have NO reset mechanism at all: `OnDemandThroughputOverride: {MaxReadRequestUnits: -1}` is accepted but **stored literally as -1** on readback (writing it would be strictly worse than the current no-op — a nonsense value instead of a stale-but-valid one); the documented "empty override means inherit the source table" form `{}` is accepted and then **hangs the table in `UPDATING` for over an hour** with the override unchanged and every later call returning `ResourceInUseException` (it left a table AWS refused to delete for having "acted as a source region for new replica(s) ... in the last 24 hours" — treat `{}` as hazardous in any future probe); an entry carrying only `IndexName` is rejected outright (`ValidationException: There are no actions specified in the Replica Update Action`). `ProvisionedThroughputOverride` is the same story by construction — the registry schema declares `ReadCapacityUnits` / `MaxReadRequestUnits` `"minimum": 1`, and live `-1` / `0` both fail validation while `{}` returns `InternalServerError`. So no in-band sentinel exists, and shipping any of them would trade a silent no-op for a live defect. **Tests:** 6 units covering the full removal, the PARTIAL removal, value-changed (never the sentinel), stringly-typed coercion, billing-flip suppression, and the no-change redeploy; revert-proofed — neutering the reset fails exactly the first two while the other four stay green, since they pin behavior the change does not alter. **Related finding filed, not fixed here:** the probes surfaced that the table-level on-demand READ ceiling (`Replicas[local].ReadOnDemandThroughputSettings`, i.e. the canonical `Billing.onDemand({maxReadRequestUnits})`) is never wired AT ALL — dropped on the way in rather than merely un-reset, and the repo's own verbatim-`cdk synth` fixture already carried the value with no assertion over it (issue [#1436](https://github.com/go-to-k/cdkd/issues/1436)).
2222

2323
**Recently Implemented** (2026-08-09):

src/provisioning/providers/dynamodb-globaltable-provider.ts

Lines changed: 76 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -1383,27 +1383,49 @@ export class DynamoDBGlobalTableProvider implements ResourceProvider {
13831383
// `toFiniteNumber`, so a present-but-unparseable value (an unresolved
13841384
// `{Ref: …}`) arrives here as `undefined` and is indistinguishable from
13851385
// a removal — which sent `-1` and silently CLEARED the ceiling the
1386-
// template was trying to SET. Gating on raw presence keeps such a value
1387-
// on the path where it reaches AWS and fails loudly instead.
1386+
// template was trying to SET.
1387+
//
1388+
// Be precise about what the guard buys, because the obvious claim is
1389+
// WRONG: it does NOT put the value back on a path that reaches AWS.
1390+
// The coercion happened upstream, so the unparseable value is already
1391+
// gone by the time this loop runs and NOTHING is sent for that member.
1392+
// What the guard removes is the DESTRUCTIVE half — cdkd no longer
1393+
// deletes a ceiling the user was trying to set — and what remains is a
1394+
// no-op. A no-op with no explanation is its own trap, so the
1395+
// suppression is reported: without it the only output was the
1396+
// immutable-field warning below, which tells the user to RECREATE THE
1397+
// INDEX — advice that is both useless and alarming for what is really
1398+
// an unresolved intrinsic in their template.
13881399
const previousOnDemand = previousSdkByName.get(gsi.IndexName)?.OnDemandThroughput;
13891400
const declared = rawOnDemandDeclarations.get(gsi.IndexName);
13901401
const onDemand: OnDemandThroughput = { ...gsi.OnDemandThroughput };
1402+
const unresolvedMembers: string[] = [];
13911403
if (!billingFlipped && previousOnDemand) {
13921404
if (
13931405
previousOnDemand.MaxReadRequestUnits !== undefined &&
1394-
onDemand.MaxReadRequestUnits === undefined &&
1395-
!declared?.readDeclared
1406+
onDemand.MaxReadRequestUnits === undefined
13961407
) {
1397-
onDemand.MaxReadRequestUnits = ON_DEMAND_LIMIT_RESET;
1408+
if (declared?.readDeclared) unresolvedMembers.push('MaxReadRequestUnits');
1409+
else onDemand.MaxReadRequestUnits = ON_DEMAND_LIMIT_RESET;
13981410
}
13991411
if (
14001412
previousOnDemand.MaxWriteRequestUnits !== undefined &&
1401-
onDemand.MaxWriteRequestUnits === undefined &&
1402-
!declared?.writeDeclared
1413+
onDemand.MaxWriteRequestUnits === undefined
14031414
) {
1404-
onDemand.MaxWriteRequestUnits = ON_DEMAND_LIMIT_RESET;
1415+
if (declared?.writeDeclared) unresolvedMembers.push('MaxWriteRequestUnits');
1416+
else onDemand.MaxWriteRequestUnits = ON_DEMAND_LIMIT_RESET;
14051417
}
14061418
}
1419+
if (unresolvedMembers.length > 0) {
1420+
this.logger.warn(
1421+
`GSI '${gsi.IndexName}' on ${physicalId}: the template declares ` +
1422+
`${unresolvedMembers.join(' / ')} but the value did not resolve to a ` +
1423+
`number, so the on-demand limit was left UNCHANGED on AWS rather than ` +
1424+
`applied or cleared. This is usually an unresolved intrinsic in the ` +
1425+
`template; resolve it to a literal to change the ceiling, or remove the ` +
1426+
`property entirely to clear it.`
1427+
);
1428+
}
14071429
if (Object.keys(onDemand).length > 0) update.OnDemandThroughput = onDemand;
14081430
// Provisioned capacity on a flip is step 4's job, so only a real
14091431
// same-billing-mode edit sends it from here.
@@ -3103,26 +3125,6 @@ export function toSdkGlobalSecondaryIndexes(
31033125
return result;
31043126
}
31053127

3106-
/**
3107-
* Translate a CFn `Replicas[].GlobalSecondaryIndexes[]` blob (the
3108-
* `ReplicaGlobalSecondaryIndexSpecification` shape) into the SDK's
3109-
* `ReplicaGlobalSecondaryIndex[]` used by `CreateReplicationGroupMemberAction`
3110-
* / `UpdateReplicationGroupMemberAction` (Issue #1387).
3111-
*
3112-
* Verified mapping:
3113-
*
3114-
* | CFn | SDK |
3115-
* | ---------------------------------------------------------- | ---------------------------------------------- |
3116-
* | `ReadProvisionedThroughputSettings.ReadCapacityUnits` | `ProvisionedThroughputOverride.ReadCapacityUnits` |
3117-
* | `ReadOnDemandThroughputSettings.MaxReadRequestUnits` | `OnDemandThroughputOverride.MaxReadRequestUnits` |
3118-
*
3119-
* DELIBERATELY UNMAPPED: `ContributorInsightsSpecification`. The replica-GSI
3120-
* SDK shape has no member for it — per-index contributor insights are toggled
3121-
* by a separate `UpdateContributorInsights(TableName, IndexName)` call, which
3122-
* this provider does not issue for any index (it only reads the TABLE-level
3123-
* status back in `readCurrentState`). Recorded here rather than silently
3124-
* dropped; tracked with the other GlobalTable nested-key gaps.
3125-
*/
31263128
/** Which on-demand members a GSI's CFn side actually DECLARES (issue #1440). */
31273129
export interface RawOnDemandDeclaration {
31283130
readonly readDeclared: boolean;
@@ -3150,10 +3152,12 @@ export interface RawOnDemandDeclaration {
31503152
* two must agree on where each member lives, and a divergence between them is
31513153
* exactly what would re-open the bug.
31523154
*
3153-
* Malformed shapes report "not declared" rather than throwing:
3154-
* `toSdkGlobalSecondaryIndexes` already refuses a non-array `GlobalSecondaryIndexes`
3155-
* loudly, and this helper is consulted only to SUPPRESS a reset, so the
3156-
* conservative answer is the one that leaves the live value alone.
3155+
* A missing entry means "not declared", which lets the reset FIRE — i.e. the
3156+
* DESTRUCTIVE direction, not a conservative one. That is safe only because
3157+
* `toSdkGlobalSecondaryIndexes` refuses a non-array `GlobalSecondaryIndexes`
3158+
* and a malformed per-entry shape LOUDLY before this map is ever consulted, so
3159+
* a template that reaches the reset has already been validated. Do not relax
3160+
* either of those refusals on the assumption that this helper degrades safely.
31573161
*/
31583162
export function collectRawOnDemandDeclarations(
31593163
properties: Record<string, unknown>,
@@ -3182,9 +3186,27 @@ export function collectRawOnDemandDeclarations(
31823186
const name = gsi?.['IndexName'];
31833187
if (!gsi || typeof name !== 'string') continue;
31843188
const localEntry = localByName.get(name);
3189+
// An already-SDK-shaped `OnDemandThroughput` WINS over the derived members
3190+
// in `toSdkGlobalSecondaryIndexes`, so it has to win here too. Reading the
3191+
// derived spellings anyway would report a member DECLARED that the
3192+
// translation never sends — e.g. an explicit `{MaxReadRequestUnits: 50}`
3193+
// next to a leftover `WriteOnDemandThroughputSettings` — and the
3194+
// suppressed reset would leave the old write ceiling live in AWS: the very
3195+
// #1160 silent drop this reset exists to close, reintroduced by the guard.
3196+
const explicitOnDemand = asRecord(gsi['OnDemandThroughput']);
3197+
if (explicitOnDemand) {
3198+
out.set(name, {
3199+
readDeclared: explicitOnDemand['MaxReadRequestUnits'] !== undefined,
3200+
writeDeclared: explicitOnDemand['MaxWriteRequestUnits'] !== undefined,
3201+
});
3202+
continue;
3203+
}
31853204
out.set(name, {
3186-
// Same precedence as the SDK translation: replica entry first, GSI-level
3187-
// spelling as the hand-authored fallback. Declared in EITHER place counts.
3205+
// Same sources as the SDK translation: replica entry first, GSI-level
3206+
// spelling as the hand-authored fallback. Declared in EITHER place
3207+
// counts — the translation resolves "first PARSEABLE", so an OR here is
3208+
// a safe superset for suppression (it can never report not-declared for
3209+
// a member the translation did resolve).
31883210
readDeclared:
31893211
asRecord(localEntry?.['ReadOnDemandThroughputSettings'])?.['MaxReadRequestUnits'] !==
31903212
undefined ||
@@ -3196,6 +3218,26 @@ export function collectRawOnDemandDeclarations(
31963218
return out;
31973219
}
31983220

3221+
/**
3222+
* Translate a CFn `Replicas[].GlobalSecondaryIndexes[]` blob (the
3223+
* `ReplicaGlobalSecondaryIndexSpecification` shape) into the SDK's
3224+
* `ReplicaGlobalSecondaryIndex[]` used by `CreateReplicationGroupMemberAction`
3225+
* / `UpdateReplicationGroupMemberAction` (Issue #1387).
3226+
*
3227+
* Verified mapping:
3228+
*
3229+
* | CFn | SDK |
3230+
* | ---------------------------------------------------------- | ---------------------------------------------- |
3231+
* | `ReadProvisionedThroughputSettings.ReadCapacityUnits` | `ProvisionedThroughputOverride.ReadCapacityUnits` |
3232+
* | `ReadOnDemandThroughputSettings.MaxReadRequestUnits` | `OnDemandThroughputOverride.MaxReadRequestUnits` |
3233+
*
3234+
* DELIBERATELY UNMAPPED: `ContributorInsightsSpecification`. The replica-GSI
3235+
* SDK shape has no member for it — per-index contributor insights are toggled
3236+
* by a separate `UpdateContributorInsights(TableName, IndexName)` call, which
3237+
* this provider does not issue for any index (it only reads the TABLE-level
3238+
* status back in `readCurrentState`). Recorded here rather than silently
3239+
* dropped; tracked with the other GlobalTable nested-key gaps.
3240+
*/
31993241
export function toSdkReplicaGlobalSecondaryIndexes(
32003242
replicaIndexes: unknown
32013243
): ReplicaGlobalSecondaryIndex[] | undefined {

0 commit comments

Comments
 (0)