p1.13: cascade ON DELETE on every legacy FK to users(id) (#413)#425
Merged
Conversation
Pre-fix, 32 of the 39 user-owned tables had a FK to users(id) without ON DELETE CASCADE. The clause wasn't standardised until the newer tables (ai_provider_settings, lifebooks, recovery_codes, model_downloads, connector_health from #377) landed. The practical effect: any "delete my account" code path either had to enumerate every table (fragile, easy to miss a new one) or got blocked by FK violations on the first row in behavioral_patterns / signals / mempalace / sessions. New migration 061-cascade-cleanup.sql does the one-time DDL backfill: for each of the 32 tables, ALTER TABLE <t> DROP CONSTRAINT IF EXISTS <t>_user_id_fkey; ALTER TABLE <t> ADD CONSTRAINT <t>_user_id_fkey FOREIGN KEY (user_id) REFERENCES users(id) ON DELETE CASCADE; The DROP uses IF EXISTS so re-runs no-op; the ADD trips 42710 (duplicate_object) on re-run which the migration runner's IDEMPOTENT_DDL_CODES set absorbs. Constraint names follow CockroachDB's default `<table>_<column>_fkey` convention — verified across the codebase as the universal pattern (no FK to users(id) is hand-named). Safety net: new E2E test packages/db/src/__tests__/cascade-cleanup.e2e.test.ts (gated on E2E=true) queries information_schema.referential_constraints after migrations run and fails if any FK to users(id) is still set to NO ACTION — catches drift if a fork ever adds a hand-named FK that the migration's conventional DROP would miss. Also exercises cascade end-to-end on behavioral_patterns (one of the 32) to prove the FK semantics actually delete child rows, not just the metadata flag. Schema-level prerequisite for #376 (delete-my-data endpoint) — without cascade, that endpoint would have either been a 30-statement manual purge or a runtime FK error on first delete. Migration number 061 leaves 060 to PR #422 (connector_health from #377). Both migrations are independent; either ordering is fine. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR standardizes deletion semantics across the database by backfilling ON DELETE CASCADE on legacy foreign keys that reference users(id), enabling a single DELETE FROM users to reliably remove a user’s dependent rows across the schema.
Changes:
- Adds migration
061-cascade-cleanup.sqlto drop/recreate legacyuser_id → users(id)FKs withON DELETE CASCADEacross the affected tables. - Adds an E2E test (
cascade-cleanup.e2e.test.ts, gated byE2E=true) that verifies all FKs tousers(id)are cascading and validates actual cascade behavior onbehavioral_patterns. - Updates
CHANGELOG.mdto document the cascade backfill and the new safety-net test.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/db/src/migrations/061-cascade-cleanup.sql | Rebuilds legacy user_id foreign keys to users(id) with ON DELETE CASCADE. |
| packages/db/src/tests/cascade-cleanup.e2e.test.ts | E2E validation that all users(id) references cascade + behavioral canary delete check. |
| CHANGELOG.md | Documents the cascade backfill migration and associated E2E safety net. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+12
to
+16
| -- The 7 tables that already had ON DELETE CASCADE | ||
| -- (ai_provider_settings, lifebooks, recovery_codes, model_downloads, | ||
| -- and a handful of newer ones — connector_health from #377 being the | ||
| -- most recent) are skipped here; their FK already cascades and re-running | ||
| -- this migration would be a duplicate. |
|
|
||
| it('every FK that references users(id) has ON DELETE CASCADE', async () => { | ||
| // information_schema.referential_constraints joined to key_column_usage | ||
| // surfaces the delete_rule + the column that referencee — exactly the |
1. Migration 061 comment claimed "7 tables already had ON DELETE CASCADE" but the codebase actually has many more cascading FKs (mcp_*, capability_provenance_nodes, etc. from migration 027+). Reworded to enumerate the categories without an inaccurate count and to cite the grep used to find the exhaustive non-cascade target list. 2. Typo in cascade-cleanup.e2e.test.ts comment: "referencee" → "referencing column". Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Owner
Author
|
Addressed both Copilot doc findings in 676a651: enumerated the cascading-FK categories accurately (mcp_*, capability_provenance_nodes, etc.) and fixed the "referencee" typo. CI should re-run. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Pre-fix, 32 of 39 user-owned tables had a FK to
users(id)withoutON DELETE CASCADE. That meant any "delete my account" code path either had to enumerate every table manually (fragile) or hit FK violations on the first row inbehavioral_patterns/signals/ mempalace /sessions.New migration
061-cascade-cleanup.sqldoes the one-time DDL backfill:```sql
ALTER TABLE DROP CONSTRAINT IF EXISTS _user_id_fkey;
ALTER TABLE ADD CONSTRAINT _user_id_fkey
FOREIGN KEY (user_id) REFERENCES users(id) ON DELETE CASCADE;
```
…for each of the 32 affected tables.
Why CockroachDB's default constraint name works
The convention is
<table>_<column>_fkeyfor any FK declared inline in a column definition without an explicit name. Verified across the codebase as the universal pattern — no FK tousers(id)is hand-named (`grep CONSTRAINT.FOREIGN KEY packages/db/src/migrations/.sql` returns zero matches).Idempotency
DROP CONSTRAINT IF EXISTSno-ops on a re-runADD CONSTRAINTtrips SQLSTATE42710(duplicate_object) which the migration runner'sIDEMPOTENT_DDL_CODESset absorbsSafety net
New E2E test
cascade-cleanup.e2e.test.ts(gated onE2E=true):information_schema.referential_constraintsafter migrations run, fails if any FK tousers(id)is stillNO ACTIONbehavioral_patterns(one of the 32) to prove the FK semantics — not just the metadata flag — actually delete child rowsIf a fork ever adds a hand-named FK that the migration's conventional DROP would miss, the assertion test fires.
Migration ordering
Number 061 leaves 060 to PR #422 (connector_health from #377). Both migrations are independent; either ordering is fine.
Unlocks
Test plan
pnpm --filter @skytwin/db build— cleanpnpm --filter @skytwin/db test— 301 pass, 19 skipped (no regressions)E2E=true pnpm --filter @skytwin/db exec vitest run src/__tests__/cascade-cleanup.e2e.test.tsagainst a CRDB with migrations applied🤖 Generated with Claude Code