fix(amplify-table): populate per-GSI provisionedThroughput on billing-mode update to prevent null capacity UPDATE_FAILED - #3518
Conversation
…-mode update to prevent null capacity UPDATE_FAILED The Custom::AmplifyDynamoDBTable managed-table handler could emit a GlobalSecondaryIndexUpdates[*].Update.ProvisionedThroughput with absent read/write capacity units, which DynamoDB UpdateTable rejects with: 2 validation errors detected: Value null at 'globalSecondaryIndexUpdates.1.member.update.provisionedThroughput.writeCapacityUnits' failed to satisfy constraint: Member must not be null (and readCapacityUnits) leaving the custom resource in UPDATE_FAILED. Root cause: in getNextGSIUpdate(), the GSI throughput-update path sourced capacity exclusively from the end-state *index* definition (gsiToUpdate.provisionedThroughput?.readCapacityUnits!). When an index inherits the table-level throughput it has no provisionedThroughput of its own, so both units were `undefined` and the non-null assertions hid it at compile time. The predicate that selected the index made the same mistake in reverse: it compared the live index capacity against the *index* end state, so `10 !== undefined` was always true and it kept selecting an index that did not actually need an update. The billing-mode-change path in getNextAtomicUpdate() had the mirror-image gap, sourcing every index's capacity from the table-level end state and thereby clobbering any per-index throughput the customer had configured. The GSI *creation* path already resolved this correctly (index value first, table-level as default). This change extracts that resolution into resolveGsiProvisionedThroughput() and applies it uniformly to the creation, billing-mode-change, and throughput-update paths. The helper returns undefined rather than a partially populated object, so an incomplete ProvisionedThroughput can no longer be serialized, and it returns undefined for PAY_PER_REQUEST so on-demand tables keep omitting the property. The defect has been latent since #1940. The replace_2_gsis_update_attr_* deploy-velocity e2e groups were green on main Jul 10 (758ab96) and began failing deterministically Jul 24+ with the handler code byte-identical across that window, so an external trigger (service-side UpdateTable validation tightening) unmasked it rather than a repo change. Also updates one existing snapshot: a GSI declaring its own 4/4 throughput is no longer overwritten with the table-level 5/5 on a billing-mode change.
CI status clarificationThe tests this PR fixes are passing. All five The two still-red e2e groups are not caused by this PR. This affects It is being fixed separately in #3519 → #3519 The description has been updated with a CI status section reflecting the above. |
| writeCapacityUnits: endState.provisionedThroughput?.writeCapacityUnits, | ||
| }; | ||
| } | ||
| const gsiProvisionThroughput: any = resolveGsiProvisionedThroughput(endState, gsiToAdd); |
There was a problem hiding this comment.
Could we drop the : any here? The helper already returns a well-typed union — keeping it inferrable makes the downstream spread type-safe if the return type ever changes.
| }; | ||
| const gsiToUpdate = endStateGSIs.find(gsiRequiresUpdatePredicate); | ||
| if (gsiToUpdate) { | ||
| const resolvedThroughput = resolveGsiProvisionedThroughput(endState, gsiToUpdate)!; |
There was a problem hiding this comment.
Worth removing the ! here too? The predicate already guarantees it's defined, but given the PR's goal of eliminating non-null assertions, a guard clause (or capturing the resolved value inside the loop) would keep things consistent.
Problem
All five
replace_2_gsis_update_attr_*deploy-velocity CDK e2e groups (empty_table,single_record,1k_records,10k_records,100k_records) fail deterministically at deploy time. TheCustom::AmplifyDynamoDBTableresource goes toUPDATE_FAILEDwith:These tests replace two GSIs while flipping the table to
BillingMode.PROVISIONEDwith a table-level throughput of 10/10 (API_POST_PROCESSOR_SET_PROVISIONED_THROUGHPUT_TWO_GSIS).Root cause
In
amplify-table-manager-handler.ts, the GSI throughput-update path ofgetNextGSIUpdate()sourced capacity units exclusively from the end-state index definition:In the managed-table construct, a GSI only carries its own
provisionedThroughputwhen the construct-level billing mode is alreadyPROVISIONED(amplify-dynamodb-table-construct/index.tsL143-149). In this scenario the indexes inherit the table-level throughput and therefore have noprovisionedThroughputof their own, so both units evaluated toundefined. The non-null assertions (!) suppressed the type error, and because this return value is not passed throughparsePropertiesToDynamoDBInputtheundefinedkeys survived to the SDK call, where DynamoDB saw an emptyprovisionedThroughputobject and reported both members as null.gsiRequiresUpdatePredicatemade the mirror-image mistake, comparing the live index capacity against the index end state:That is
10 !== undefined→ alwaystrue, so the handler kept selecting an index that did not actually need an update, guaranteeing the malformed request was issued on every reconciliation pass.Separately, the billing-mode-change branch of
getNextAtomicUpdate()had the opposite gap: it mapped every current-state GSI to a table-levelendState.provisionedThroughput, clobbering any per-index throughput a customer had configured (and emitting the same empty object whenever the table-level value was absent).The GSI creation path already resolved this correctly — index value first, table-level as the default (previously L677-684). The update paths simply never adopted that logic.
Fix
Extract the creation path's resolution into a single helper and apply it uniformly across all three paths:
undefinedrather than a partially populated object, so aProvisionedThroughputwith null/absent capacity can never be serialized. Indexes with no resolvable throughput are filtered out ofGlobalSecondaryIndexUpdatesinstead of emitting an invalid member.PAY_PER_REQUESTpreserved — returnsundefined, so on-demand tables continue to omit the property entirely.Change is localized to
getNextAtomicUpdate/getNextGSIUpdateplus the new helper. No public API surface is touched (this package has noapi-extractor.json), so noAPI.mdupdate is required.Why now
The defect has been latent since #1940. The affected e2e groups were green on
mainon Jul 10 (758ab96) and began failing deterministically from Jul 24 onward, while the handler code is byte-identical across that window. The trigger is therefore external — service-sideUpdateTablevalidation tightening that stopped tolerating an emptyprovisionedThroughputon anUpdateaction — rather than a repo change. This fix makes the handler correct regardless of the external trigger.Validation
npx tsc --buildonamplify-graphql-model-transformer— clean.amplify-table-manager-lambda.test.tsunderGet billing mode update › per-index provisioned throughput:PROVISIONEDwith two GSIs where only one declares its own throughput → asserts bothUpdate.ProvisionedThroughputentries have non-null numeric read/write capacity (gsi1 keeps 3/4, gsi2 inherits 10/10).UPDATE_FAILEDscenario and now passes.PAY_PER_REQUEST→ProvisionedThroughputomitted.e2e_workflow_cdk.yml, which contains thereplace_2_gsis_update_attr_*groups) pluspr_workflow.ymlwere triggered on this branch and have completed — see CI status below.CI status
The groups this PR targets are PASSING. ✅
replace_2_gsis_update_attr_*groups (empty_table,single_record,1k_records,10k_records,100k_records) are green on the e2e CDK batch, with zero recurrence of theprovisionedThroughputnull-capacityUPDATE_FAILED.3_gsis_*set is green as well — no regression from the per-index throughput resolution change.pr_workflowgate is green.The two remaining red e2e groups are unrelated to this PR
custom_query_mutation_extensionandadmin_rolefail for a different, pre-existing reason that has nothing to do with this change:cdk initCLI toolchain drift in the e2e scaffolder.initCDKProject()pinsaws-cdk-libbut rancdk initwith a floating CLI, and the upstream template changed its synth command fromnpx ts-node --prefer-ts-extstonpx tsc && npx tsx. That turns synth into a whole-project typecheck of every.tscopied into the scratch project — including lambda entry files that are only referenced by esbuild as path strings and never imported.TS7006(authorizer.ts:1:26) andTS2307(apiInvoker.ts:6:51), both failing beforecdk synthruns.mainand every open PR, not just this one. Deterministic — reproduced 6/6 locally. Retrying will never clear it.No file touched by this PR is involved in those two failures.
Notes
Independent of #3517 (
addResourceDependencydeprecation) — branched off latestmain, no shared files.