Skip to content

Commit ae7dc59

Browse files
committed
fix(dynamodb): merge the on-demand reset so a PARTIAL removal is not still dropped
Review blocker on the first version: the reset lived in an else-if, so it only fired when the new side had NO OnDemandThroughput at all. The read limit comes from Replicas[local].GlobalSecondaryIndexes[].ReadOnDemandThroughputSettings and the write limit from GSI.WriteOnDemandThroughputSettings - two INDEPENDENT CDK props - so removing just one left the other live in AWS forever. That is the #1423 bug itself, in the likelier user edit, shipped as fixed. Now merges: start from the desired side and fill -1 for every member the template dropped. Live-probed the MIXED payload before adopting it, since only {-1,-1} had been verified: UpdateTable accepted { MaxReadRequestUnits: 50, MaxWriteRequestUnits: -1 } and DescribeTable read back { MaxReadRequestUnits: 50 } - dropped member cleared, kept member preserved. The integ fixture now drops ONLY the write limit (the harder case) and verify.sh asserts write is absent while read is still 50. It also asserts the index EXISTS first: a '| [0]' query against a missing index also answers None, so the absence check alone would pass if byOwner had vanished. The first integ run of that assertion FAILED because the rewritten queries lost the Table. prefix and length() received null - caught by the integ, fixed, re-run clean (182s, 3 deleted / 0 errors / 0 orphans). Table-level and cross-region-replica on-demand ceilings have the same class and are filed as #1434; each needs its own live probe and the table-level one touches a different code path.
1 parent e8769c3 commit ae7dc59

5 files changed

Lines changed: 101 additions & 49 deletions

File tree

docs/_generated/integ-last-run.tsv

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -77,7 +77,7 @@ drift-revert-arrays 2026-08-01T09:36:05Z PASS 110 verify.sh 0801 regression swee
7777
drift-revert-vpc 2026-07-30T16:18:56Z PASS 480 verify.sh post-rebase final for PR #1307 (#1299/#1300): 6/6 reverted, 21 del 0 err 0 orphans
7878
dsql 2026-07-30T18:11:24Z PASS 540 verify.sh 4-phase incl. destroy --remove-protection CC flip (#1312); orph clean
7979
dynamodb-autoscaling 2026-07-21T14:33:57Z PASS 77 verify.sh rc ok, orph clean
80-
dynamodb-globaltable 2026-08-09T14:14:39Z PASS 279 verify.sh issue #1423 per-GSI on-demand limit RESET asserted live (reads back absent); 3 del/0 err, 0 orphans
80+
dynamodb-globaltable 2026-08-09T14:33:56Z PASS 182 verify.sh #1423 PARTIAL removal asserted live (write reset absent, read kept 50); 3 del/0 err, 0 orphans
8181
dynamodb-gsi-update 2026-07-20T08:09:23Z PASS 579 verify.sh rc ok, orph clean
8282
dynamodb-ondemand 2026-08-01T10:07:21Z PASS 81 verify.sh 0801 regression sweep; ondemand+policy+kinesis backfills ok, 0 orphans
8383
dynamodb-sse 2026-08-01T10:07:21Z PASS 40 verify.sh 0801 regression sweep; SSE mapping ok, 0 orphans

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

Lines changed: 38 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -1298,38 +1298,46 @@ export class DynamoDBGlobalTableProvider implements ResourceProvider {
12981298
// a flip to on-demand — the same silent-drop class #1387 exists to close.
12991299
const billingFlipped = oldBilling !== newBilling;
13001300
const update: UpdateGlobalSecondaryIndexAction = { IndexName: gsi.IndexName };
1301-
if (gsi.OnDemandThroughput) {
1302-
update.OnDemandThroughput = gsi.OnDemandThroughput;
1303-
} else if (!billingFlipped) {
1304-
// REMOVING a per-GSI on-demand limit from the template has to be sent
1305-
// as an explicit reset. Omitting the member leaves the old ceiling
1306-
// live in AWS forever while cdkd reports success — the
1307-
// absent-field-reset silent-drop class (#1160), one level down inside
1308-
// a nested block (#1225). CloudFormation resets it.
1309-
//
1310-
// `-1` is the reset sentinel, and that is LIVE-VERIFIED rather than
1311-
// inferred from the table-level field's docs (issue #1423): a real
1312-
// UpdateTable with `{-1, -1}` on the per-GSI Update action was
1313-
// ACCEPTED, and DescribeTable afterwards reported the member as
1314-
// ABSENT — i.e. genuinely cleared, not stored as -1. Reset semantics
1315-
// are field-specific, so this was probed, never assumed.
1316-
//
1317-
// Only the members that were actually SET before are reset: blanket
1318-
// `{-1, -1}` would clear a sibling limit the template still declares.
1319-
// Skipped on a billing flip, where "no on-demand fields" is just the
1320-
// translation of a PROVISIONED side, not a template removal.
1321-
const previousOnDemand = previousSdkByName.get(gsi.IndexName)?.OnDemandThroughput;
1322-
if (previousOnDemand) {
1323-
const reset: OnDemandThroughput = {};
1324-
if (previousOnDemand.MaxReadRequestUnits !== undefined) {
1325-
reset.MaxReadRequestUnits = ON_DEMAND_LIMIT_RESET;
1326-
}
1327-
if (previousOnDemand.MaxWriteRequestUnits !== undefined) {
1328-
reset.MaxWriteRequestUnits = ON_DEMAND_LIMIT_RESET;
1329-
}
1330-
if (Object.keys(reset).length > 0) update.OnDemandThroughput = reset;
1301+
// On-demand limits: MERGE the desired side with an explicit reset for
1302+
// every member the template DROPPED. Omitting a removed member leaves
1303+
// the old ceiling live in AWS forever while cdkd reports success — the
1304+
// absent-field-reset silent-drop class (#1160), one level down inside a
1305+
// nested block (#1225). CloudFormation resets it.
1306+
//
1307+
// Merging rather than branching is load-bearing: the read limit comes
1308+
// from `Replicas[local].GlobalSecondaryIndexes[].ReadOnDemandThroughput-
1309+
// Settings` and the write limit from `GSI.WriteOnDemandThroughputSettings`
1310+
// — two INDEPENDENT CDK props. An `else if` that only fired when the new
1311+
// side had no on-demand block at all would still silently drop the
1312+
// single-member removal, which is the likelier user edit.
1313+
//
1314+
// `-1` is the reset sentinel, LIVE-VERIFIED rather than inferred from
1315+
// the table-level field's docs (issue #1423). Both payload shapes were
1316+
// probed against real AWS: `{-1, -1}` cleared both members, and the
1317+
// MIXED `{MaxReadRequestUnits: 50, MaxWriteRequestUnits: -1}` was
1318+
// accepted and read back as `{MaxReadRequestUnits: 50}` — the dropped
1319+
// member cleared, the kept one preserved. In both cases the reset reads
1320+
// back as ABSENCE, never as -1, so drift comparisons must expect that.
1321+
//
1322+
// Skipped on a billing flip, where "no on-demand fields" is just the
1323+
// translation of a PROVISIONED side rather than a template removal.
1324+
const previousOnDemand = previousSdkByName.get(gsi.IndexName)?.OnDemandThroughput;
1325+
const onDemand: OnDemandThroughput = { ...gsi.OnDemandThroughput };
1326+
if (!billingFlipped && previousOnDemand) {
1327+
if (
1328+
previousOnDemand.MaxReadRequestUnits !== undefined &&
1329+
onDemand.MaxReadRequestUnits === undefined
1330+
) {
1331+
onDemand.MaxReadRequestUnits = ON_DEMAND_LIMIT_RESET;
1332+
}
1333+
if (
1334+
previousOnDemand.MaxWriteRequestUnits !== undefined &&
1335+
onDemand.MaxWriteRequestUnits === undefined
1336+
) {
1337+
onDemand.MaxWriteRequestUnits = ON_DEMAND_LIMIT_RESET;
13311338
}
13321339
}
1340+
if (Object.keys(onDemand).length > 0) update.OnDemandThroughput = onDemand;
13331341
// Provisioned capacity on a flip is step 4's job, so only a real
13341342
// same-billing-mode edit sends it from here.
13351343
if (!billingFlipped && gsi.ProvisionedThroughput) {

tests/integration/dynamodb-globaltable/lib/dynamodb-globaltable-stack.ts

Lines changed: 11 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -202,19 +202,19 @@ export class DynamoDBGlobalTableStack extends cdk.Stack {
202202
{
203203
indexName: 'byOwner',
204204
partitionKey: { name: 'owner', type: ddb.AttributeType.STRING },
205-
// Issue #1423: REMOVING these from the template must reset the live
206-
// ceiling, not silently no-op. Under `drop-gsi-ondemand-limits` both
207-
// are omitted, and verify.sh asserts DescribeTable reports the
208-
// member ABSENT afterwards (the reset reads back as absence, never
209-
// as -1).
205+
// -> Replicas[local].GlobalSecondaryIndexes[].ReadOnDemandThroughputSettings
206+
maxReadRequestUnits: 50,
207+
// Issue #1423: REMOVING this from the template must RESET the live
208+
// ceiling, not silently no-op. `drop-gsi-ondemand-limits` drops ONLY
209+
// the write limit and keeps the read one, which is the harder case:
210+
// the two are independent CDK props, so a fix that only reset when
211+
// the whole on-demand block disappeared would still drop this edit.
212+
// verify.sh asserts the write member is ABSENT while read is still
213+
// 50 (the reset reads back as absence, never as -1).
214+
// -> GlobalSecondaryIndexes[].WriteOnDemandThroughputSettings
210215
...(updateMode.includes('drop-gsi-ondemand-limits')
211216
? {}
212-
: {
213-
// -> Replicas[local].GlobalSecondaryIndexes[].ReadOnDemandThroughputSettings
214-
maxReadRequestUnits: 50,
215-
// -> GlobalSecondaryIndexes[].WriteOnDemandThroughputSettings
216-
maxWriteRequestUnits: 60,
217-
}),
217+
: { maxWriteRequestUnits: 60 }),
218218
},
219219
],
220220
removalPolicy: cdk.RemovalPolicy.DESTROY,

tests/integration/dynamodb-globaltable/verify.sh

Lines changed: 21 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -411,15 +411,29 @@ echo "[verify] step 13b: cdkd deploy with drop-gsi-ondemand-limits (issue #1423
411411
CDKD_TEST_UPDATE=ttl,tags,drop-gsi-ondemand-limits ${CLI} deploy "${STACK}" --state-bucket "${STATE_BUCKET}" --verbose
412412

413413
# The reset reads back as ABSENCE, never as -1 (live-probed on #1423). Pre-fix
414-
# the template removal emitted nothing at all, so the 50/60 ceiling stayed live
415-
# in AWS forever while cdkd reported success.
416-
OD_AFTER="$(aws dynamodb describe-table --table-name "${GSI_OD_TABLE}" --region "${REGION}" \
417-
--query "Table.GlobalSecondaryIndexes[?IndexName=='byOwner'].OnDemandThroughput | [0]" --output text)"
418-
if [ "${OD_AFTER}" != "None" ]; then
419-
echo "FAIL: issue #1423 — byOwner still carries OnDemandThroughput after the template removed it: ${OD_AFTER}" >&2
414+
# the template removal emitted nothing at all, so the 60 write ceiling stayed
415+
# live in AWS forever while cdkd reported success.
416+
#
417+
# Assert the index still EXISTS first: a `| [0]` query against a MISSING index
418+
# also answers "None", so the absence check alone would pass if `byOwner` had
419+
# vanished entirely.
420+
OD_IDX="$(aws dynamodb describe-table --table-name "${GSI_OD_TABLE}" --region "${REGION}" \
421+
--query "length(Table.GlobalSecondaryIndexes[?IndexName=='byOwner'])" --output text)"
422+
if [ "${OD_IDX}" != "1" ]; then
423+
echo "FAIL: byOwner index missing after the drop-limits update (count=${OD_IDX})" >&2
424+
exit 1
425+
fi
426+
# The DROPPED member must be gone...
427+
OD_WRITE="$(aws dynamodb describe-table --table-name "${GSI_OD_TABLE}" --region "${REGION}" \
428+
--query "Table.GlobalSecondaryIndexes[?IndexName=='byOwner'].OnDemandThroughput.MaxWriteRequestUnits | [0]" --output text)"
429+
# ...while the one the template STILL declares must survive untouched.
430+
OD_READ="$(aws dynamodb describe-table --table-name "${GSI_OD_TABLE}" --region "${REGION}" \
431+
--query "Table.GlobalSecondaryIndexes[?IndexName=='byOwner'].OnDemandThroughput.MaxReadRequestUnits | [0]" --output text)"
432+
if [ "${OD_WRITE}" != "None" ] || [ "${OD_READ}" != "50" ]; then
433+
echo "FAIL: issue #1423 — expected write=None (reset) / read=50 (kept), got write=${OD_WRITE} / read=${OD_READ}" >&2
420434
exit 1
421435
fi
422-
echo " per-GSI on-demand limits reset (OnDemandThroughput absent), issue #1423 closed"
436+
echo " per-GSI write limit reset (absent) and read limit kept at 50, issue #1423 closed"
423437

424438
echo "[verify] step 14a: assert DeletionProtectionEnabled flipped back to false on AWS"
425439
DP_FINAL="$(aws dynamodb describe-table --table-name "${TABLE_NAME}" --region "${REGION}" \

tests/unit/provisioning/dynamodb-globaltable-provider-gsi-throughput.test.ts

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -556,6 +556,36 @@ describe('DynamoDBGlobalTable GSI throughput translation (issue #1387)', () => {
556556
]);
557557
});
558558

559+
it('resets the DROPPED member while KEEPING the one still declared (partial removal)', async () => {
560+
// The likeliest user edit: read and write limits are two independent CDK
561+
// props (read via the local replica, write on the GSI), so removing ONE
562+
// is common. A branch that only reset when the new side had NO on-demand
563+
// block at all would silently leave the other ceiling live — the very
564+
// #1423 bug, shipped as fixed. Live-probed: the MIXED payload
565+
// {MaxReadRequestUnits: 50, MaxWriteRequestUnits: -1} is accepted and
566+
// reads back as {MaxReadRequestUnits: 50}.
567+
const previous = structuredClone(ON_DEMAND_TABLE_PROPS) as Record<string, unknown>;
568+
const next = structuredClone(ON_DEMAND_TABLE_PROPS) as Record<string, unknown>;
569+
// Drop ONLY the write limit; the replica-side read limit (50) stays.
570+
delete (next['GlobalSecondaryIndexes'] as Record<string, unknown>[])[0]![
571+
'WriteOnDemandThroughputSettings'
572+
];
573+
574+
await provider.update('OnDemand', 'od-table', RESOURCE_TYPE, next, previous);
575+
576+
const gsiUpdates = mockSend.mock.calls
577+
.map((c) => c[0])
578+
.filter(
579+
(c): c is UpdateTableCommand =>
580+
c instanceof UpdateTableCommand &&
581+
(c.input.GlobalSecondaryIndexUpdates ?? []).some((u) => u.Update !== undefined)
582+
);
583+
expect(gsiUpdates).toHaveLength(1);
584+
expect(
585+
gsiUpdates[0]!.input.GlobalSecondaryIndexUpdates?.[0]?.Update?.OnDemandThroughput
586+
).toEqual({ MaxReadRequestUnits: 50, MaxWriteRequestUnits: -1 });
587+
});
588+
559589
it('resets ONLY the member that was actually set before', async () => {
560590
// A blanket {-1, -1} would clear a sibling limit the template still
561591
// declares.

0 commit comments

Comments
 (0)