Repository navigation
Conversation
🦋 Changeset detectedLatest commit: 0863a6f The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
📖 Glossary & notes — scoring, indicators, per-metric thresholds (click to expand)
Results — PR vs
|
| Task | Template | Tests | Judge | Cost | Turns | Score | Stop reason |
|---|---|---|---|---|---|---|---|
| async-word-counter | bare | — | — | ✨ $4.38 (-$1.37) | ✨ 23 (-6) | — | endTurn |
| auth-notes | demo | — | — | ❌ $7.01 (+$2.05) | — | endTurn | |
| cognito-profile | auth-cognito | — | — | ❌ $18.16 (+$8.21) | ❌ 50 (+14) | — | endTurn |
| collab-presence-board | default | — | — | ✅ $2.21 (-$0.08) | ✅ 17 (+1) | — | endTurn |
| email-digest | demo | — | — | ✨ $5.23 (-$0.89) | ✅ 32 (-2) | — | endTurn |
| file-gallery | bare | — | — | ❌ $2.49 (+$0.89) | — | endTurn | |
| kb-chat-agent | demo | — | — | ✅ $24.26 (+$1.44) | ✅ 51 (-2) | — | endTurn |
| observability-api | backend | — | — | ✨ $4.1 (-$1.21) | ✨ 22 (-4) | — | endTurn |
| oidc-dsql-notes | react | — | — | ✨ $4.68 (-$4.84) | ✨ 23 (-12) | — | endTurn |
| sql-kb-catalog | nextjs | — | — | ✅ $7.93 (-$0.43) | ✅ 39 (0) | — | endTurn |
Merge verdict
✅ This bench run does not block the PR.
- ⚪ Absolute floor — BENCH_MIN_SCORE=65 set, but no cell was scored — floor skipped (conservative).
- ⚪ Regression vs
main— BENCH_MAX_REGRESSION=10 set, but no main baseline to diff — regression gate skipped.
The Δ vs base ball above is a fixed ±5-pt display band; the regression gate uses BENCH_MAX_REGRESSION. The ball is cosmetic — the gate threshold is what blocks.
Caveats & exclusions
- 🧰 Excluded (harness_error, not scored) — 10: 10× isolation_unavailable. Cells:
async-word-counter/bare,auth-notes/demo,cognito-profile/auth-cognito,collab-presence-board/default,email-digest/demo,file-gallery/bare,kb-chat-agent/demo,observability-api/backend,oidc-dsql-notes/react,sql-kb-catalog/nextjs
osama-rizk
left a comment
osama-rizk
left a comment
There was a problem hiding this comment.
This fixes a real data-durability bug: a stack delete (or removing a DistributedTable) dropped every GSI from a table CloudFormation then retains. The change is safe to ship on the code path. No blocking items; one should-fix on how a lookup failure during an update cleanup shows up, plus two nits. The dependency and IAM sign-off is a maintainer call, summarised below.
What's good
- The state table in the PR body and DESIGN.md matches the code, and the fail-safe direction (keep on uncertainty) is the right default for a retained table.
- The "keep" verdict travels to isComplete through the Provider framework's
{...request, ...onEventResult}merge (checked in aws-cdk-lib's framework.js), so only the onEvent role gets the new permission. - Putting the statement in the role's default policy, which the function
DependsOn, closes the stack-delete ordering race, and a test pins it. - The tests are strong. With main's
gsi-manager-lambda.tsandindex.cdk.tsunder the new tests, 15 of 176 fail; on this head, 176 of 176 pass. 14 of 16 mutations I tried fail a test (status set, invertedtableStays, failure→drop, isComplete ignoring the flag,TemplateStage, every NotFound path, missing StackId, lookup on Create, IAM*, separate policy, wrong role,GetTemplatedropped). - Offline synth of main against this head: no resources added or removed. The only changes are both GSI Lambdas'
Code(in-place update) and one added statement inBlocksGsiManagerServiceRoleDefaultPolicy:cloudformation:DescribeStacksandcloudformation:GetTemplateonRef AWS::StackId. The custom resource properties are unchanged, so nothing is replaced or re-invoked on upgrade.
What needs work: 0 must-fix · 1 should-fix · 2 nits; details inline.
Dependency and IAM (maintainer decision)
@aws-sdk/client-cloudformation@3.1153.0: Apache-2.0, maintained by AWS.npm lsshows it deduped with@aws-blocks/core's identical range, so installs don't change. Only the Lambda source imports it. esbuild keeps@aws-sdk/*external, so in Lambda the Node 24 runtime's SDK serves it. A devDependency would also build, butbb-auth-oidcalready lists its Lambda-only SDK clients underdependencies, so this follows the repo's convention.- IAM: read-only, the stack's own ARN, the onEvent role only, pinned by tests. To weigh: it is a broadening IAM diff in every indexed-table stack. Blocks deploy and sandbox pass
--require-approval never, but a plaincdk deployat the default level would prompt.GetTemplatealso lets the GSI Lambda read the whole template. - Semver: shipped as a patch. The behaviour change (indexes kept when the table leaves the stack) is a fix, so patch looks defensible; flagging it for the maintainer.
- Conditional exports:
exportsis unchanged, and the new code lives only in the CDK construct and the separately bundled Lambda.parity.testpasses.
Speed / flakiness: The GSI custom resource delete during stack destroy drops from 18–31 s on main (runs 39812188339 and 39821845972) to 6–7 s on this PR (run 39872727575), in both Production and Sandbox. The whole destroy is unchanged (10.5 min here; 10.4 and 13.4 min on main), and the E2E Production job's 50 min sits within main's 43–52 min range. So the run-time saving is real but small, about 12–25 s per destroy, and doesn't change the time to required-green. On flakiness, it removes one DELETE_FAILED mode (table already gone), which can leak a stack. That is inferred; the runs don't show it happening. The E2E Sandbox failure on this head (secure-items GSI query works, 0 rows) writes two items and queries the GSI immediately with no retry. GSI reads are eventually consistent, the query path isn't touched here, and the same test passed on this PR's earlier head c0ecf68. I read it as a pre-existing flake.
E2E integrity: Nothing is skipped, gated, loosened, retried or given a longer timeout. The PR only adds tests: a new unit file wired into npm test, and 5 CDK tests. The program doc's "no PR removes or weakens a test" holds here. One gap: the path that must still destroy (remove indexes, redeploy, indexes dropped) is covered only by unit tests against a faked template. E2E exercises only the stack-delete path.
Open questions
- Is a silent keep on a lookup failure during an update cleanup the intended UX (inline)?
- The PR is still a draft. Is anything pending before it's ready for review?
Cross-cutting
- #827 also touches
bb-distributed-table(index.cdk.tstype exports, its own changeset) and merges cleanly with this PR. No ordering is needed. - Follow-up idea, not for this PR: an E2E step that redeploys a table without
indexesand asserts the GSI is gone would cover the destructive branch end to end. Thesecure-itemsGSI query test would also benefit from a short poll.
| console.log(tableStays ? 'Table stays in the stack' : 'Table is leaving the stack'); | ||
| return !tableStays; | ||
| } catch (e) { | ||
| console.warn('Could not read the stack status or template; leaving the indexes in place:', e); |
There was a problem hiding this comment.
Non-blocking (should fix): during UPDATE_COMPLETE_CLEANUP_IN_PROGRESS, this catch also handles the case where the user removed indexes and redeployed. If GetTemplate (or DescribeStacks) fails there after the 8 attempts (throttling is plausible when several tables clean up at once), the handler keeps the indexes and reports success. The deploy goes green and the custom resource leaves the stack, so no later deploy reconciles that table. Its GSIs stay in place and keep costing storage and write capacity until someone notices. The only signal is this console.warn in the GSI manager's log group.
Keeping the indexes is right while the status is unknown. Once DescribeStacks has returned a cleanup status, though, a GetTemplate failure is just a failed cleanup step. As far as I know, CloudFormation records a failed delete in the cleanup phase as a DELETE_FAILED event but leaves the stack UPDATE_COMPLETE, with no rollback. Throwing there would make the miss visible at no durability cost.
Fix: split the try. Keep returning true when DescribeStacks fails or the status is in STACK_GOING_AWAY. When the status is a cleanup status and GetTemplate fails, throw (or at least log a line naming the table and indexes left behind). Add a test for each, and a README line on how to spot a leftover index. If keep-and-continue is the deliberate choice, a sentence in the README's "Indexes follow the config" note would settle it.
There was a problem hiding this comment.
Fixed in 9b6c3a1b (message refined in 47edae29 and e70a3fe8): the try is split, so a DescribeStacks failure or an empty stack list still keeps the indexes and succeeds, while a GetTemplate failure in either cleanup status keeps them and fails the Delete with the table and up to 5 index names (a stack delete never reaches GetTemplate). Tests cover each case, and the README's "Indexes follow the config" note now says how to spot and remove a leftover index; unit job: Build, Unit Tests, E2E Local.
| return { IsComplete: true }; | ||
| } | ||
|
|
||
| const table = event.RequestType === 'Delete' ? await describeTableIfExists(TableName) : await describeTable(TableName); |
There was a problem hiding this comment.
Non-blocking (nit): the Delete-tolerant pattern is written twice in each handler. The RequestType === 'Delete' ? describeTableIfExists : describeTable pair is here and at line 246, and the initiateNextChange try/catch on isTableNotFound is at lines 262-270 and 308-313. Four copies of one rule ("on Delete, a missing table is success") will drift if one changes.
Fix: fold it into two small helpers, e.g. describeForRequest(requestType, tableName) and initiateNextChangeTolerant(requestType, ...) that returns false when the table is gone, and call them from both handlers.
|
|
||
| // --- Lookup failures keep the indexes (non-destructive) --- | ||
|
|
||
| test('GSI manager: if the stack status cannot be read, Delete keeps the indexes', async () => { |
There was a problem hiding this comment.
Non-blocking (nit): nothing covers the status === undefined keep branch at gsi-manager-lambda.ts:93 (DescribeStacks returning no stacks). I changed it to status !== undefined && STACK_GOING_AWAY.has(status) and all tests still pass. The handler then falls through to GetTemplate and drops or keeps based on the template.
Fix: add a case next to this one where the fake returns { Stacks: [] } and assert updateTableCalls is empty and cfnCalls is ['DescribeStacks'].
There was a problem hiding this comment.
Added in 9b6c3a1b: the fake returns { Stacks: [] } and the test asserts no UpdateTable and cfnCalls equal to ['DescribeStacks']; your status !== undefined && ... mutant now fails it.
fa0639d to
e70a3fe
Compare
|
On the review body: the |
osama-rizk
left a comment
osama-rizk
left a comment
There was a problem hiding this comment.
Since the last review: 3 resolved · 0 new · 0 still open
- Resolved: a
GetTemplatefailure inUPDATE_COMPLETE_CLEANUP_IN_PROGRESSorUPDATE_ROLLBACK_COMPLETE_CLEANUP_IN_PROGRESSnow keeps the indexes and fails the Delete, naming the table and up to 5 indexes (gsi-manager-lambda.ts:122). ADescribeStacksfailure or an empty stack list still keeps them and succeeds. - Resolved: the Delete-tolerant describe and
UpdateTablehandling now live indescribeForRequestandinitiateNextChangeTolerant, used by both handlers, and a new test pins that Create still fails when the table disappears beforeUpdateTable. - Resolved: the
{ Stacks: [] }case is tested, and thestatus !== undefined && ...mutant now fails it.
Approving. The fix keeps the indexes in place when the stack is being deleted or the table is leaving it, and a cleanup that cannot read the template now shows up as a DELETE_FAILED event in the deploy output instead of passing silently.
What I verified on e70a3fe (the branch was rebased onto main; the first commit has the same patch as fa0639d, so I reviewed d5851da..e70a3fe as the new work):
- Build is clean and the package tests pass, 183/183. With main's
gsi-manager-lambda.tsandindex.cdk.ts, 21 of them fail. With the previous head's handler and the new tests, exactly the 4 new cleanup-failure tests fail, so they pin the new behavior. - A stack delete can't be blocked by the new failure.
DELETE_IN_PROGRESSandROLLBACK_IN_PROGRESSreturn beforeGetTemplate(the stack-delete test assertscfnCallsis['DescribeStacks']), and making the throw apply to every status fails the "any other stack status" test. - A failed Delete in the cleanup phase is reported as a
DELETE_FAILEDevent, and the stack still endsUPDATE_COMPLETE/UPDATE_ROLLBACK_COMPLETE. The custom resource leaves the stack, so nothing stays stuck. The README and changeset say this accurately. - Mutation testing: I ran 29 mutants against the handler and CDK wiring, and 28 are caught. They cover dropping the rollback-cleanup status, removing the throw, throwing in every status, removing the 5-index cap, swapping the per-status hint, dropping the cause, and both new helpers in each direction. The only survivor is skipping the early return in onEvent when
UpdateTablereports the table gone, and the outcome is the same there: isComplete finds the table gone and completes. - No change to
index.cdk.ts, the IAM (Identity and Access Management) statement or dependencies since the last review, so the earlier synth diff still holds. - The GSI (global secondary index) query E2E (end-to-end) test now polls for up to about 10 s with the same assertions, and it passed in E2E Sandbox on this head. All checks are green.
Follow-ups (not blocking):
- A redeploy-without-
indexesE2E test, which you already list in the description. The "indexes removed, table stays, indexes dropped" path is covered by unit tests only. - The dependency, the IAM statement and patch vs minor still need a maintainer decision before this leaves draft.
…aves the stack On Delete the GSI manager now reads its own stack's status and current template. It leaves the indexes in place when the stack is being deleted, its first create is rolling back, or the table is no longer in the template (the DistributedTable was removed from the app, or a failed update that added it is rolled back). Removing indexes from the config still drops them. A failed lookup keeps the indexes, and a Delete whose table no longer exists now succeeds. Grants cloudformation:DescribeStacks and cloudformation:GetTemplate on the stack itself to the GSI onEvent role, in the role's default policy so a stack delete cannot remove it before the custom resource runs.
…stack template During an update's cleanup phase, a GetTemplate failure on Delete kept the table's indexes and reported success, so an index removed from the config could stay on the table with no visible signal. DescribeStacks and GetTemplate now have separate handling: a status that cannot be read still keeps the indexes and succeeds, while a template that cannot be read in an update cleanup keeps them and fails the Delete with a message naming the table and its indexes. A failed delete in the cleanup phase does not roll the update back, and a stack delete never reads the template. Also covers DescribeStacks returning no stack, folds the four missing-table-on-Delete checks into two helpers, and polls briefly in the secure-items GSI e2e query (GSI reads are eventually consistent).
… more cases The failure reported when the stack template cannot be read in a cleanup phase now says, after a rolled-back update, that a leftover index is reused by a later deploy, and lists at most 10 index names so the reason stays well inside the custom resource response limit. The README says the event can come from an update or a rolled-back update, names the logical ID to look for, and notes that DynamoDB takes one index change per call. New tests: Create still fails when the table disappears before UpdateTable, only a DynamoDB table resource counts as the table staying, and the index list is capped.
Keeps the failure reason well inside the custom resource response limit even with long index names, and says that a leftover index from a rolled-back update is reused only if a later deploy adds it again with the same keys.
…r version The fix adds a runtime dependency and a read-only IAM statement to every stack with an indexed table, so it ships as a minor bump. The changeset now names the new dependency.
…r changeset @aws-blocks/blocks re-exports DistributedTable, so its changelog carries the change too. The dependency note no longer claims an identical range with the published core release.
For reviewers: new runtime dependency and new role permissions
@aws-blocks/bb-distributed-table:@aws-sdk/client-cloudformation(^3.1153.0). It is already a runtime dependency of@aws-blocks/core, so it is normally installed already. The GSI Lambda bundle keeps@aws-sdk/*external, so the Lambda runtime provides it.cloudformation:DescribeStacksandcloudformation:GetTemplate, read-only, with the resource limited to the stack's own ARN (Ref AWS::StackId). The isComplete role and the application role are unchanged.indexes, including every app using Realtime.@aws-blocks/bb-distributed-tableand@aws-blocks/blocksbecause of the new dependency and the IAM diff.Problem
The DistributedTable GSI custom resource depends on the table, so CloudFormation deletes it first whenever the table leaves the stack. Its Delete handler always treated the desired index set as empty and dropped every GSI, one at a time. That is right when
indexesis removed from the config, but with the production default (RemovalPolicy.RETAIN,protection: 'locked'/'retained') the table survives a stack delete, or the removal of the DistributedTable from the app, without any of its indexes. Rebuilding them on a large table takes hours. A Delete also failed (DELETE_FAILED) when the table no longer existed.Observed on main in CI: during the E2E Production stack delete of a baseline run, CloudTrail shows the GSI manager roles issuing four
UpdateTableGSI deletes across three tables.Fix
On Delete,
onEventreads its own stack:DELETE_IN_PROGRESS(stack delete, retried delete, nested stack removed)ROLLBACK_IN_PROGRESS(first create failed)UPDATE_COMPLETE_CLEANUP_IN_PROGRESSindexesremoved)UPDATE_COMPLETE_CLEANUP_IN_PROGRESSUPDATE_ROLLBACK_COMPLETE_CLEANUP_IN_PROGRESSindexes)UPDATE_ROLLBACK_COMPLETE_CLEANUP_IN_PROGRESSDescribeStacksfails (or returns no stack), the Delete keeps the indexes and succeeds. IfGetTemplatefails in one of the two cleanup statuses, the Delete keeps the indexes and fails with a message naming the table and its indexes (at most 5). A failed delete in the cleanup phase shows up as aDELETE_FAILEDevent, and the update is not rolled back, so the leftover is visible instead of silent. A stack delete never reachesGetTemplate, so it cannot be blocked. In any other status aGetTemplatefailure keeps the indexes and succeeds.isCompleteHandlerthrough the Provider framework's onEvent-result passthrough, so only the onEvent role gets the permission.ResourceNotFoundExceptionon Delete (DescribeTable or UpdateTable) is success. Create and Update still fail on a missing table. The four copies of that rule are now two helpers,describeForRequestandinitiateNextChangeTolerant.IAM and upgrade impact
cloudformation:DescribeStacks,cloudformation:GetTemplateonRef AWS::StackId(read-only, own stack). It is added to the role's default policy, which the functionDependsOn.addToRolePolicywould emit a separate policy with no ordering edge, which a stack delete could remove before the custom resource runs.@aws-sdk/client-cloudformationbecomes a runtime dependency of bb-distributed-table. It is already a runtime dependency of@aws-blocks/core, so it is normally installed already. The Lambda bundle keeps@aws-sdk/*external.indexes(including every app using Realtime) gets a new code asset for both GSI Lambdas plus that one statement. Custom resource properties are unchanged, so nothing is re-invoked on upgrade. A synth test pins this.Tests
src/gsi-manager-lambda.test.ts(new) drives the real handlers the way the Provider framework does, with the SDK clients faked. Against origin/main's handler and CDK source, 15 of the 56 package tests fail; all pass with the fix.src/index.cdk.test.tspins the IAM statement, its placement, the unchanged custom resource properties, the literal table name the check relies on, and no change for tables without indexes.npm run build,lint:depsandlintpass. The only local unit/e2e failures are the bb-distributed-data DSQL tests, whose native linter binary needs a newer glibc than the build host has. That package is unrelated to this change.DescribeStacksfails or returns no stack (kept, success, template not read),GetTemplatefails or returns an unparsable template in each cleanup status (kept, Delete fails naming the table and indexes),GetTemplatefails in another status (kept, success). Also: Create still fails when the table disappears beforeUpdateTable, and only a DynamoDB table resource with the same name counts as the table staying.test-apps/comprehensivesecure-items GSI query worksnow polls for up to about 10 s, because GSI reads are eventually consistent. It still asserts exactly the 2 rows and their data.DELETE_FAILEDin the cleanup phase leaves the stackUPDATE_COMPLETE/UPDATE_ROLLBACK_COMPLETE, andGetTemplateduring rollback cleanup returns the restored template.indexesand asserts the index is gone.Status (head 0863a6f, rebased on main after #952 and others merged)
packages/bb-distributed-table/package.jsonscripts (main added--conditions=aws-runtimetobuild:lambda); both changes kept.Not addressed here: the sandbox-only recreate fast path rebuilds the table without SSE, deletion protection, PITR, TTL, stream or tags.