docs(adr): ADR-032 integration deletion data cleanup - #641
Open
d-klotz wants to merge 7 commits into
Open
Conversation
Deleting an integration removes the Integration row and, on two of three backends, whatever the ORM cascades from it. Entity, Credential, User and Token rows always survive; on DocumentDB the integration's own child rows survive too, because the adapter deletes via $runCommandRaw and bypasses the Prisma query engine entirely. ADR-032 plans an explicit, backend-uniform purge: unconditional deletion of the records an integration owns outright, reference-counted deletion of the records it can share (Entity, Credential), and never the User. Includes the per-record policy table, ordering chosen for idempotent retry, the repository methods to add per backend, failure handling, an orphan sweeper for existing deployments, and the open decisions for maintainers. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M5jrmtqxucVoXrxvw7ph8h
Module.deauthorize() deletes a credential and unsets the link on only the entity it holds in memory, so siblings keep a dangling credentialId — the same reference-count predicate the purge needs. Note it, and place the predicate in one shared helper so that bug is fixed by construction. Also records that nothing in the repo reference-counts these records today (the one 'orphan' hit is a log line), that deleteEntity/deleteCredentialById already swallow P2025 while findEntityById throws, and that the account-erasure primitives are already half-built (deleteUser, deleteUserById, ADR-007's unimplemented route). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M5jrmtqxucVoXrxvw7ph8h
Two source-verified findings changed the design: - Sync.integrationId is never written. SyncManager.createSyncDBObject omits it and this.integration is captured at manager.js:33 and never used, so the schema's cascade has never matched a row and a purge keyed on integrationId alone would delete zero. Now: fix the writer, and match on the integration's entity ids too (scoped to those ids, never 'integrationId is null', which would delete every parentless sync). - The encryption extension's delete hook decrypts the deleted row afterwards with no error handling down to KMS, so a rotated key throws with the row already gone. Affects Credential and IntegrationMapping — exactly what the purge touches. Prefer deleteMany, which skips the crypto path. Also corrects: DataIdentifier is embedded in the Sync document on DocumentDB, so the prescribed explicit delete is a no-op there; the queue-worker guard already exists on the integrationId path but hydrates before checking status and throws on the processId path, so the purge would turn discarded messages into DLQ entries unless both are fixed here; S3 report artifacts are ADR-010, not ADR-022; admin scripts have no repository access, so the sweeper needs two command-layer reads added; the entity ownership predicate is now stated once (strict equality, null owner skips) instead of two contradictory ways. Adds a known-limits section for the non-atomic check-then-act, syncs the entity predicate does not consult, org-linked users that skip rather than resolve, and revocation being foreclosed by deletion. Testing section rewritten against the real conventions: no real-DB tier exists, repository tests assert emitted command shape against a fake prisma client. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M5jrmtqxucVoXrxvw7ph8h
Found by the synthesis pass and confirmed against source: - findIntegrationById throws on all three backends (mongo:195, postgres:239, documentdb:71,75), so the Boom.notFound at delete-integration-for-user.js:38-42 is unreachable and a retried DELETE returns 500, not 404. "Retry is the recovery path" needs that fixed first. The test double returns null, which is why it stayed invisible. - Integration has no `messages` column — only errors/warnings/info/logs — so updateIntegrationMessages reads undefined, gets [], and writes a single-element array, clobbering the column. Every deletion error erases the previous one. The purge's error reporting depends on it appending. - integration-commands.js:315-339 is a second delete path with no ownership check, no IN_DELETION and no ON_DELETE; its !deleted guard is unreachable. Recommendation is to leave it raw and document it, since the sweeper needs an unpurged primitive. create-integration.js:70-78 self-deletes a duplicate row and must stay purge-free — noted so it is not "fixed" later. Descopes the SyncManager repair: upsertSync binds a bare id array to a relation field and _convertFilterToWhere spreads raw $elemMatch/$all into Prisma's where, so the sync write path is likely non-functional and there may be no rows to purge. The purge's sync rules are correct either way via the entity-id arm; coupling the two would block this change on a larger one. Adds an open question on whether lawful erasure may purge UsageCounter rows, since no prune path exists at all today. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M5jrmtqxucVoXrxvw7ph8h
The previous version was 717 lines — 2.5x the longest existing ADR and 5x the median — written as a forensic audit with file:line citations on nearly every claim. That is not what an ADR is for, and not how the other 29 read. Rewritten to 183 lines in plain English, following the ADR-014 conventions: Context / Decision / Consequences (Positive, Negative, Neutral) / Alternatives Considered / Related, with the metadata block in the usual form. Open Questions kept as a section, matching ADR-022. Same decisions, same policy table, same ordering — just stated once and plainly instead of argued at length. Implementation detail moves to prose and a short list of the three things worth writing down (strict ownership check, deleteMany for encrypted rows, counting from the integration side on MongoDB). The four existing bugs this depends on are named in a few lines each rather than reconstructed step by step. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M5jrmtqxucVoXrxvw7ph8h
✅ Deploy Preview for friggframework-org canceled.
|
d-klotz
added
release
Claude
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7fb2f856fe
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The backend table claimed PostgreSQL's cascade removes syncs, while the bug
list four paragraphs later says Sync.integrationId is never written. Both
cannot be true: with the column always null the foreign key matches nothing.
Syncs are not removed on any backend today, so that is now stated once, under
the table, instead of contradicting it.
Fixing that surfaced a gap the simplification introduced. Step 1 said "delete
mappings, processes, syncs and associations" without saying how syncs are
matched, so implementing it as deleteMany({integrationId}) would delete zero
rows — the exact trap the bug list warns about. Step 1 now says to match on
integration id or the integration's entity ids, and to scope the second arm to
those ids rather than to "integrationId is null", which would take every
parentless sync in the database.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01M5jrmtqxucVoXrxvw7ph8h
The ordered steps listed Sync and Association but never their DataIdentifier and AssociationObject children, while the policy table promises explicit, backend-independent deletion of all six. On PostgreSQL and MongoDB the children cascade anyway, so the omission is invisible; on DocumentDB nothing cascades, so following the algorithm as written would orphan exactly those rows. Step 1 now deletes deepest-first — DataIdentifier before Sync, AssociationObject before Association — and notes that DocumentDB stores identifiers as an embedded array inside the sync document, so there is no separate collection to clear there. Also tightens the sync-cascade wording: the foreign key is null for every sync the framework has created, rather than the cascade being broken in principle. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M5jrmtqxucVoXrxvw7ph8h
|
Contributor
Author
|
Base next (77d9d29) |
This PR (7fb2f85) |
|
|---|---|---|
| Test Suites | 32 failed, 1 skipped, 68 passed | 32 failed, 1 skipped, 68 passed |
| Tests | 138 failed, 10 skipped, 1649 passed | 138 failed, 10 skipped, 1649 passed |
| Workspace | @friggframework/devtools |
@friggframework/devtools |
Same counts, same workspace, same command (jest --passWithNoTests). 77d9d29 is the commit this branch was cut from — base run, this PR's run.
Failures are spread across frigg-cli/**, management-ui/** and infrastructure/** — for instance build-prisma-layer.test.js expects a hardcoded /workspace/packages/core/prisma-postgresql/migrations path that doesn't match the runner's checkout. Nothing in packages/core, and nothing related to docs.
Worth noting the wider picture: every one of the last 30 CI runs on next has failed, going back to 2026-06-08. So build (22.x) has not been a meaningful merge gate for over two months.
I've deliberately not touched it. Fixing 138 unrelated devtools tests does not belong in a documentation PR, and no amount of change here will make this check green. It looks worth its own issue.
Generated by Claude Code
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
2 participants
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
Deleting an integration removes one row: the
Integrationrecord. ItsEntityrecords and theCredentialrecords holding live OAuth access and refresh tokens stay in the database forever, with no owner and nothing to clean them up.How much of the integration's own data goes away depends on the backend:
onDelete: Cascadein the schema, but no real foreign keys; Prisma emulates it, anduser-repository-mongo.jsalready warns against relying on that.$runCommandRaw, which never reaches Prisma's query engine, so nothing cascades and every child row is orphaned too.Syncs are the exception on every backend: the cascade is declared, but
SyncManagernever writesSync.integrationId, so for every sync the framework has created that foreign key is null and matches nothing.The schema says one thing and the behaviour is three different things. This ADR proposes a fix and documents the decision.
What's in this PR
Documentation only — one new ADR and its row in the register. No code changes.
docs/architecture-decisions/032-integration-deletion-cleanup.md(new, 197 lines)docs/architecture-decisions/README.md(one row added to the Current ADRs table)The decision, in brief
A
PurgeIntegrationDatause case runs inside the existing delete flow — afterON_DELETE, so provider-side teardown still happens first, and before theIntegrationrow is removed, so theIN_DELETIONstatus keeps queue work away while it runs.IntegrationMapping,Process,Sync,DataIdentifier,Association,AssociationObjectEntityuserIdmatchesCredentialUser,TokenUsageCounterThe rule can't be "delete what the integration touches", because three records are shared on purpose: an entity can belong to several integrations, a credential can back several entities, and a user is one app user who owns many integrations. So it's "delete what it owns, plus what nothing else needs" — which means reference counting in the use case rather than leaning on the ORM.
Deliberately explicit on all three backends rather than relying on cascade behaviour, since it differs per backend and one backend has none. Extra deletes on PostgreSQL are harmless.
Prerequisites the ADR names
Four existing bugs sit on this path, each a real bug on its own merits:
DELETEreturns 500, not 404 —findIntegrationByIdthrows on all three backends, so theBoom.notFoundcheck is unreachable. Retrying is meant to be the recovery path.updateIntegrationMessagesreads amessagescolumn that doesn't exist, so it always reads empty and writes over the top.SyncManagernever writesSync.integrationId, so the cascade the schema declares has never matched a row.Test plan
Documentation only, so there's nothing to run and I haven't claimed otherwise — no code, config or schema is touched, and the working tree has no
node_modulesinstalled.# ADR-NNN: Titleheading, bold Status/Date/Deciders block, and the Context / Decision / Consequences / Alternatives Considered / Related sectionsdocs/architecture-decisions/README.mdCI status
build (22.x)is red, and it is red onnexttoo — see this comment for the side-by-side. Identical signature on both (32 suites / 138 tests failing in@friggframework/devtools), and every one of the last 30 runs onnexthas failed going back to 2026-06-08. Not caused by this PR, and not fixable from it.Everything else is green: SonarCloud (quality gate passed, 0 new issues), CodeSee, GitGuardian. The three Netlify checks are neutral because the deploy self-cancels for a change outside the site path.
Open questions for review
user-repository-mongo.jsand implemented nowhere.UsageCounterand lawful erasure — "never delete" is right for integration deletion, but no prune path exists at all, so an adopter served an erasure request has no lawful way to remove usage rows.Note on labels
Tagged
releaseandprereleaseper the convention inCLAUDE.md. Since this is documentation only, that will cut a prerelease version for no code change — happy to drop both labels if you'd rather not publish for this.