fix: reconcile service_keys.identity_id FK to ON DELETE CASCADE on legacy DBs - #252
fix: reconcile service_keys.identity_id FK to ON DELETE CASCADE on legacy DBs#252safayavatsal wants to merge 2 commits into
Conversation
…gacy DBs
Migration 006 declares service_keys.identity_id REFERENCES
identities(id) ON DELETE CASCADE, but it uses CREATE TABLE IF NOT
EXISTS — a no-op on any deployment where the table already existed
before the cascade was added (dev1/stage1/prod lineage). Those
legacy DBs kept a non-cascading FK while fresh DBs got the cascade,
a silent declared-vs-actual schema drift.
That drift caused highflame-authn#109: hard-deleting an identity
with a service key 500'd on legacy DBs, breaking every agent delete
from Studio's registry. zeroid#187 fixed the user-visible symptom by
switching DELETE handlers to soft delete, sidestepping the FK
entirely. This closes the underlying drift the workaround left
behind: PurgeIdentity (the compensating rollback in
AgentService.RegisterAgent) still does a real hard delete, and only
avoids the FK today by the accident of running before any service
key row is persisted — a future reordering, or any hard-delete path
such as a GDPR-erasure feature, would silently break again on
legacy DBs only.
- migrations/040_service_keys_fk_cascade.{up,down}.sql: drop and
re-add the FK with ON DELETE CASCADE (down flips to NO ACTION).
Idempotent/no-op on fresh DBs that already declare the cascade.
- tests/integration/service_keys_fk_cascade_test.go: registers an
agent (auto-creates a service key), hard-deletes the identity at
the repo layer — the same DELETE FROM identities that
IdentityRepository.Delete / PurgeIdentity issues — and asserts it
succeeds and cascades the key row away.
Verified against a throwaway Postgres container built with the
legacy pre-cascade shape: reproduced the original FK violation
verbatim, confirmed the up migration flips pg_constraint.confdeltype
from 'a' to 'c' and the same hard delete then succeeds and cascades,
and confirmed the down migration round-trips back to 'a' cleanly.
Full test suite (unit + integration) green.
Closes highflame-ai#196
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
![highflame-oracle[bot]](https://avatars.githubusercontent.com/u/141064602?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔮 Oracle Review
🎯 Start Here
tests/integration/service_keys_fk_cascade_test.go — Critical security risk detected in file
Why this first: Thoroughly review for security vulnerabilities
📋 PR Summary
What this PR does: Reconciles schema drift on legacy databases by applying missing ON DELETE CASCADE to the service_keys.identity_id foreign key constraint, ensuring hard-deletes of identities properly cascade to related service key rows.
Key changes:
- Added migration 040_service_keys_fk_cascade.up.sql to drop and re-add the FK constraint with ON DELETE CASCADE
- Added down migration to revert to ON DELETE NO ACTION for safe rollbacks
- Added integration test to verify cascade behavior on identity hard-deletes
Areas affected: Database migrations, Identity deletion/PurgeIdentity path, Integration test suite
Testing notes: Verified against simulated legacy DB pre-migration state, confirming FK violation before fix and successful cascade after. Full test suite passes including new integration test and existing regression tests from #187.
🔍 Code Review
This is a well-researched and carefully executed fix for a latent schema bug. The author demonstrates deep understanding of the historical drift, validates the fix against a simulated legacy environment, and adds appropriate test coverage to prevent future regressions.
What's good:
- ✨ Excellent investigation into root cause and historical context of the schema drift
- ✨ Comprehensive verification including pre/post-migration state on a simulated legacy DB
- ✨ Thoughtful consideration of migration locking behavior (ACCESS EXCLUSIVE scoped to service_keys only)
- ✨ Proactive identification of future risk scenarios like GDPR erasure paths
Review Stats:
Generated by Oracle - Highflame's AI Code Reviewer
|
|
||
| ALTER TABLE service_keys | ||
| DROP CONSTRAINT IF EXISTS service_keys_identity_id_fkey; | ||
|
|
There was a problem hiding this comment.
Missing transaction safety
While both operations target the same table, running DDL statements without an explicit transaction block could leave the table in an inconsistent state if the ADD CONSTRAINT fails after the DROP succeeds (e.g., if identities table is locked or validation finds violations). Wrap in BEGIN; ... COMMIT; to ensure atomicity.
Suggested fix:
| BEGIN; | |
| ALTER TABLE service_keys | |
| DROP CONSTRAINT IF EXISTS service_keys_identity_id_fkey; | |
| ALTER TABLE service_keys | |
| ADD CONSTRAINT service_keys_identity_id_fkey | |
| FOREIGN KEY (identity_id) REFERENCES identities(id) ON DELETE CASCADE; | |
| COMMIT; |
2 participants
Closes #196.
Summary
migrations/006_service_keys.up.sqldeclaresservice_keys.identity_id REFERENCES identities(id) ON DELETE CASCADE, but the migration usesCREATE TABLE IF NOT EXISTS, which is a no-op on any deployment where the table already existed before the cascade was added. Legacy DBs (dev1/stage1/prod lineage) kept a non-cascading FK; fresh DBs got the cascade. Declared schema vs. live state silently diverged.highflame-authn#109: hard-deleting an identity with a service key 500'd on legacy DBs (violates foreign key constraint "service_keys_identity_id_fkey"), breaking every agent delete from Studio's registry.#187fixed the user-visible symptom by switching the DELETE handlers to soft delete (DeactivateIdentity), sidestepping the FK entirely — the right immediate call, no risky migration, audit trail preserved.PurgeIdentity(the compensating rollback inAgentService.RegisterAgent) still does a real hard delete, and it only avoids tripping the FK today because it happens to run before any service-key row is persisted. That's fragile — a future reordering, or any other hard-delete caller (e.g. a GDPR-erasure path), would silently break again, but only on legacy DBs.Changes
migrations/040_service_keys_fk_cascade.up.sql— drops and re-addsservice_keys_identity_id_fkeywithON DELETE CASCADE. No-op on fresh DBs that already declare the cascade.migrations/040_service_keys_fk_cascade.down.sql— reverts toON DELETE NO ACTION.tests/integration/service_keys_fk_cascade_test.go— registers an agent (auto-creates a bootstrap service key), then hard-deletes the identity directly at the repo layer (the sameDELETE FROM identities ...thatIdentityRepository.Delete/PurgeIdentityissues), and asserts the delete succeeds and the service key row cascades away.Verification
Built a throwaway Postgres container replicating the legacy pre-migration-006 shape (
service_keys.identity_idFK with noON DELETEclause) and drove it end to end:ERROR: update or delete on table "identities" violates foreign key constraint "service_keys_identity_id_fkey".040...up.sql:pg_constraint.confdeltypeflips froma(no action) toc(cascade); the same hard delete now succeeds and the referencingservice_keysrow is gone.040...down.sql:confdeltypeflips cleanly back toa— round-trip confirmed.Also ran, all green:
GOEXPERIMENT=jsonv2 go build ./...GOEXPERIMENT=jsonv2 go vet ./...gofmt -l .go test ./... -race -count=1for unit packages;go test ./tests/integration/...for the testcontainers suite, including the new test and the existingTestDeleteAgent_WithServiceKey_SoftDeletes/TestRegisterAgent_ApiKeyPolicyBroaderThanIdentityRejectedregression tests from fix: soft-delete identities/agents instead of hard delete (authn#109) #187)Acceptance (from #196)
ON DELETE CASCADE— verified viapg_constraint.confdeltypeon a simulated legacy DB (see Verification above). Please still confirm against a real dev1/stage1/prod snapshot before merge, per the issue's own caveat.PurgeIdentity's underlying guarantee (a service-key-bearing identity's hard delete cascades) is exercised end-to-end in the new integration test.DELETE /agents/registry/{id}continues to soft-delete viaDeactivateIdentity; existing regression tests for that path still pass unmodified.Locking / safety
As noted in the issue: both
ALTER TABLEstatements takeACCESS EXCLUSIVEonservice_keysonly (notidentities) for the duration of the constraint flip.DROP CONSTRAINTis metadata-only;ADD CONSTRAINTdoes a full-table validation scan againstidentities, butservice_keysis small (one row per issued API key), so this should be sub-second. Would still likemigration-analyzer(or equivalent) to run before merge per the issue's request.Out of scope
Per the issue: Studio UX consolidation and any user-facing hard-delete/GDPR-erasure path are tracked separately.