feat(store): own the analytics rollups in application code - #400
Conversation
Groundwork for the Aurora DSQL migration, split out so the PR that adds the cluster does not also have to change IAM. branch-ci-plan carries AWS-managed ReadOnlyAccess, which does not reliably cover dsql:*. Without AmazonAuroraDSQLReadOnlyAccess, the PR introducing an aws_dsql_cluster fails its own terraform plan. branch-lambda-role gets dsql:DbConnectAdmin because DSQL authenticates with an IAM token rather than a password. The role is shared with every preview lambda, so previews are covered too. Resource is "*" until the cluster exists. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
- Auto-formatted .tf files with terraform fmt - Updated README.md with terraform-docs Co-authored-by: nourshoreibah <[email protected]>
Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Aurora DSQL supports neither triggers nor PL/pgSQL, so the analytics rollups
have to be maintained by the application. This adds the package that owns
that, and the migration that removes the database-side machinery.
The three LANGUAGE sql helpers stay: DSQL supports those and they hold the
arithmetic. Only the nine plpgsql dispatchers move into TypeScript.
@branch/store exports `db` as ReadOnlyDb -- a Kysely handle with every
mutating entry point stripped -- plus one named operation per write. A
controller cannot write without going through an operation that also
maintains the rollup, because the write builders will not typecheck.
project_donations.donor_id and project_memberships.user_id become ON DELETE
RESTRICT. Those two cascades used to fire a row trigger from a lambda that
never names the rollup tables (DELETE /donors/{id}, DELETE /users/{id}), so
in application code they would have gone silently unmaintained. RESTRICT
turns a forgotten cascade into an FK error; the store deletes children first.
testkit gains findRollupDrift/assertRollupsConsistent/reconcileRollups,
recomputing both rollups from the base tables with the same aggregation as
the original backfill. resetData now reconciles after seeding, since nothing
refills the rollups once the triggers are gone.
Verified against postgres: 0 triggers remain, only the 3 sql functions
survive, both FKs are RESTRICT, and drift is detected and repaired.
Verified with @aws/dsql-lint: the migration is DSQL-clean.
Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Deletes the six near-identical db.ts files and points all 20 modules at the shared store. The 21 write sites become named store operations, so rollup maintenance travels with the write instead of being a thing each caller has to remember. Write DTOs (NewExpenditure, ExpenditureEdit, NewDonation, ...) are declared in @branch/types beside the row types and re-exported by the store, so a caller does not need Kysely's generics to write a row and cannot set a generated column by accident. claimUser exists because registration's two updates carry a compound `WHERE user_id = $1 AND cognito_sub IS NULL`. That predicate is what makes a concurrent claim a no-op rather than an overwrite of a working account, so it needed its own operation rather than a generic updateUser. syncMemberships moves from the projects lambda into the store, taking the default role as a parameter so the store needs no lambda-local types. The project create/update transactions move with it, which is what lets the member_count rollup update inside the same transaction as the roster. grep for insertInto/updateTable/deleteFrom/db.transaction across lambda source now returns nothing. All six lambdas typecheck. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Mechanical for the most part: the six db.ts modules are gone, so imports move to @branch/store and db.destroy() becomes closeConnection() (the read-only handle deliberately has no destroy). Mocks that drove the query builder now drive the store operation the code actually calls -- mockDb.insertInto becomes mockRecordExpenditure, and so on. That is a smaller mock in every case. rollup-triggers.e2e.test.ts becomes rollup-store.e2e.test.ts. It drove the row triggers with raw SQL, including shapes no route can produce: a bulk UPDATE with no WHERE, TRUNCATE, moving an expenditure between projects by column. Those tested trigger generality. The replacement exercises every operation the store exposes, and adds the two cascade cases the old file could not reach because the triggers hid them -- deleting a donor and deleting a user. Fixtures that stay on raw SQL now call reconcileRollups afterwards. Raw SQL no longer maintains the rollups, and the dashboard fixture in particular rewrites spent_on, which moves rows between buckets. lambda-deploy.yml learns shared/store in both the push filter and the in-job detector. Without it a store-only change would never deploy. 650 tests pass across all six lambdas. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
This PR contains a database migration
It will be applied to the production database automatically when this PR merges, before the new lambda code is deployed. Please confirm before requesting review:
This PR also changes 80 files outside |
|
Database Types Check Complete The database schema files were modified, but the regenerated TypeScript types are identical to the existing ones. No changes were needed and the type definitions are already up to date. |
Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
|
Database Types Check Complete The database schema files were modified, but the regenerated TypeScript types are identical to the existing ones. No changes were needed and the type definitions are already up to date. |
Four review findings, all real. Rollup delta applied even when the DELETE matched nothing. removeExpenditure, removeReport and removeDonation all read the row, deleted, then backed the amount out unconditionally. Two containers deleting the same id would each decrement, leaving the count permanently low. removeDonor and removeUser already guarded this; these three now do too, and the pre-read takes FOR UPDATE so the loser re-reads and finds the row gone rather than racing. The pre-read was also unlocked in editExpenditure, so `before` could be stale by the time the rollup was adjusted -- it would subtract an amount no longer in the bucket. tx()'s retry set could never catch it because Postgres does not raise 40001 under READ COMMITTED. FOR UPDATE closes it on both engines: it blocks on Postgres, and on DSQL the conflict surfaces as an OCC error the retry already handles. project_rollup_bump was a bare UPDATE returning void, so a project with no project_rollup row swallowed every change silently. projects_rollup_seed used to guarantee that row for any insert path; since 20260906215733 only the store seeds it. The function now returns 1 or NULL and the caller throws, so an unseeded project fails the transaction instead of drifting for ever. removeUser did have an e2e test, but a weak one -- it only audited consistency, which passes even if the delete does nothing. It now asserts the RESTRICT path really cleared the memberships and that member_count reached zero on both affected projects. Note on the concurrency test: the double-decrement race is not reachable in-process. The store pool is max: 1, so two tx() calls serialise on the one connection and never interleave -- a test using Promise.all passes against the unfixed code, which is why there is a sequential test and a comment rather than a green test that proves nothing. Reproducing it needs two pools. 653 tests pass. Both new migrations are DSQL-clean per @aws/dsql-lint. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
|
Pushed 1 — rollup delta applied on a zero-row DELETE. Confirmed. 2 — stale 3 — 4 — coverage. Partly off: One thing worth flagging. I first wrote finding 1's regression test with 653 tests pass. No generated-type drift. |
|
Database Types Check Complete The database schema files were modified, but the regenerated TypeScript types are identical to the existing ones. No changes were needed and the type definitions are already up to date. |
Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
|
Database Types Check Complete The database schema files were modified, but the regenerated TypeScript types are identical to the existing ones. No changes were needed and the type definitions are already up to date. |
The earlier round fixed the single-row deletes but left the same bug in every multi-row path: the delta came from an unlocked SELECT taken before the mutation, so a concurrent write made it stale. syncMemberships computed `members.length - existing.length`. Two roster edits on one project, both reading existing = 2: the first deletes 2, inserts 3 and bumps +1 (count 3, correct); the second deletes 3, inserts 1 and bumps -1, leaving count 2 against one actual row. Under READ COMMITTED the second DELETE re-reads and removes the first's rows, but its delta was already fixed from the stale count. Now counted from the DELETE itself. removeDonor and removeUser bumped from a list read before deleting, so a donation or membership inserted in between was deleted without ever coming off the rollup. Both now use DELETE ... RETURNING and bump from what was actually removed, which also drops the pre-read entirely. RETURNING beats locking here: SELECT ... FOR UPDATE locks the rows it finds, but does not stop a new child row appearing before the DELETE, so it would not have closed either window. Both early `return 0n` guards go: the bumps now correspond exactly to the rows deleted, and FK RESTRICT means a child cannot outlive its parent, so the observable result is unchanged. DELETE ... RETURNING confirmed DSQL-compatible with @aws/dsql-lint. As with the last round the race is not reproducible in-process (store pool is max: 1), so these ship without a regression test rather than with one that passes against the unfixed code. 568 tests pass across the five affected lambdas. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
|
Follow-up on race safety —
Both early Verified Two things that are safe and worth recording, since they're easy to "optimise" into bugs later:
Still not covered by a test. Same reason as last round: the store pool is Also unchanged and worth a decision: retry budget is 568 tests pass across the five affected lambdas. |
|
Database Types Check Complete The database schema files were modified, but the regenerated TypeScript types are identical to the existing ones. No changes were needed and the type definitions are already up to date. |
Terraform Plan 📖
|
|
Database Types Check Complete The database schema files were modified, but the regenerated TypeScript types are identical to the existing ones. No changes were needed and the type definitions are already up to date. |
#400 gained V20260907213524__project_rollup_bump_reports_hit before it merged, so it arrived in the pre-Flyway naming form. Rename it and move the adoption baseline up to it -- production has already applied it, so Flyway must baseline over it rather than re-run it. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Second of four PRs migrating BRANCH from RDS Postgres to Aurora DSQL. Still on RDS — no DSQL anywhere in this diff.
Built on #398, which is already merged. This branch was cut before that merge and #398 was squash-merged, so its three commits still appear in the commit list here — they contribute nothing to the file diff (the
infrastructure/diff againstmainis empty).Why
Aurora DSQL supports neither triggers nor PL/pgSQL.
20260823055243_add_analytics_rollups.sqlmaintainsexpenditure_rollupandproject_rollupwith 9 triggers and 9 plpgsql functions, and four lambdas read those tables — so they can't just be dropped. Doing this move on RDS first means the existing suite is a real oracle; the DSQL cutover then has nothing left to prove about rollups.Verified with
@aws/dsql-lint: the old migration produces 9 plpgsql ERRORs + 9 trigger ERRORs, and the threeLANGUAGE sqlhelpers are not flagged. That's the split this PR is built on — only the dispatch moves.The hazard this is shaped around
The obvious design — a store per lambda — does not work.
DELETE /donors/{id}andDELETE /users/{id}mutate rollup-affecting rows via FK cascade, from lambdas that never nameproject_donationsorproject_memberships. Neither would import a store for a table it doesn't mention, so those two paths would have gone silently unmaintained.So the chokepoint is shared, and it owns those parent deletes.
What's here
shared/store→@branch/storereplaces the six identicaldb.tsfiles. It exportsdbasReadOnlyDb— aKysely<DB>withinsertInto/updateTable/deleteFrom/transaction/schema/withSchema/withstripped — plus one named operation per write. A controller cannot write without going through an operation that also maintains the rollup, because the write builders don't typecheck.grepforinsertInto|updateTable|deleteFrom|db.transactionacross lambda source now returns nothing.Write DTOs (
NewExpenditure,ExpenditureEdit,NewDonation, …) live in@branch/typesbeside the row types, following theauth-types.d.tsprecedent, and are re-exported by the store. Callers don't need Kysely generics, and generated columns are absent so they can't be set by accident.The three
LANGUAGE sqlhelpers stay in the database. They hold the arithmetic, including theON CONFLICTagainst the expression index, and DSQL supports them. Only the 9 plpgsql dispatchers — each literallyremove(OLD); add(NEW)— move to TypeScript.ON DELETE RESTRICTonproject_donations.donor_idandproject_memberships.user_id, so a forgotten cascade becomes an FK error rather than a silently wrong total. The store deletes children first. This also earns its keep on DSQL, where cascaded rows count against the 3,000-row transaction limit.claimUserexists because registration's two updates carryWHERE user_id = $1 AND cognito_sub IS NULL. That predicate is what makes a concurrent claim a no-op instead of overwriting a working account, so it needed its own operation rather than a genericupdateUser.tx()retries serialization failures (40001,40P01, and DSQL'sOC000/OC001). It retries the whole transaction, not just the rollup half — retrying only the rollup would double-count.The safety net
testkitgainsfindRollupDrift/assertRollupsConsistent/reconcileRollups, recomputing both rollups from the base tables with the same aggregation as the original backfill. Verified end to end: a write that skips its rollup is detected, with a legible message, and reconcile repairs it.resetDatanow reconciles after seeding —TRUNCATEempties the rollups andseed.sqlonly writes base rows, so with the triggers gone nothing refilled them. The new assertion caught that immediately.rollup-triggers.e2e.test.ts→rollup-store.e2e.test.ts. The old file drove the triggers with raw SQL, including shapes no route can produce (bulkUPDATEwith noWHERE,TRUNCATE, moving an expenditure between projects by column) — that tested trigger generality. The replacement covers every operation the store exposes, and adds the two cascade cases the triggers previously hid.Verification
sqlfunctions, both FKsRESTRICTnpm run typesproduces no diffdsql-lintreports the new migration fully DSQL-clean (neededDROP TRIGGER IF EXISTSandNOT VALIDon the FK adds — both valid Postgres too, so the file stays cross-engine)Two things reviewers should weigh in on
no-restricted-importsbackstop, and it isn't here. The root.eslintrc.jsonsetsignorePatterns: ["**/*"]and the lambdas have no config of their own, soeslint apps/backendcurrently lints nothing — the rule would be decoration. The type-levelReadOnlyDbis the real guard and it works (there's a type test that was checked to fail when the guarantee is removed). Fixing the eslint setup felt out of scope here; happy to do it as a follow-up.reconcileRollupsexists to close it, but wiring it in as a post-deploy step is not in this PR — worth deciding whether that lands here or as a follow-up.Also fixes
lambda-deploy.yml, which filtered on a hardcoded path list — ashared/store-only change would never have deployed. (shared/rbacandshared/lambda-authare still missing from that list; left alone as pre-existing.)Follow-ups
flyway-database-dsql(still on RDS)🤖 Generated with Claude Code