From 15575c158ed6e066e819b30c43818b93a053d9de Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 20 Aug 2026 20:24:11 +0000 Subject: [PATCH 1/7] docs(adr): ADR-032 integration deletion data cleanup 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 Claude-Session: https://claude.ai/code/session_01M5jrmtqxucVoXrxvw7ph8h --- .../032-integration-deletion-cleanup.md | 452 ++++++++++++++++++ docs/architecture-decisions/README.md | 1 + 2 files changed, 453 insertions(+) create mode 100644 docs/architecture-decisions/032-integration-deletion-cleanup.md diff --git a/docs/architecture-decisions/032-integration-deletion-cleanup.md b/docs/architecture-decisions/032-integration-deletion-cleanup.md new file mode 100644 index 000000000..f446d547e --- /dev/null +++ b/docs/architecture-decisions/032-integration-deletion-cleanup.md @@ -0,0 +1,452 @@ +# ADR-032: Integration Deletion Data Cleanup + +**Status**: Proposed +**Date**: 2026-08-20 +**Deciders**: TBD + +## Context + +Deleting an integration removes the `Integration` row and — on two of the three database +backends — whatever the ORM cascades from it. Everything the integration needed in order to +exist (its `Entity` rows, their `Credential` rows) is left in the database forever, and on +DocumentDB even the integration's own child rows survive. Adopters end up with a database +that accumulates dead credentials holding live OAuth refresh tokens. + +### What deletion does today + +`DELETE /api/integrations/:integrationId` (`integration-router.js:315-324`) calls +`DeleteIntegrationForUser.execute(integrationId, userId)` +(`integrations/use-cases/delete-integration-for-user.js:35-116`), which: + +1. Loads the integration and rejects it if `integrationRecord.userId !== userId` (`:50-54`). +2. Best-effort loads a module instance per entity id so provider-side teardown has API + clients; failures are logged and counted, never fatal (`:56-80`). +3. Instantiates the integration class, `initialize()`s it, and sets status `IN_DELETION` + (`:82-96`). +4. Sends `ON_DELETE` (`:98`). The base handler is a no-op — `async onDelete(params) {}` + (`integration-base.js:485`) — so provider-side cleanup (webhooks, subscriptions, + scheduled EventBridge jobs) happens only if the concrete integration overrides it. If it + throws, an error message is appended and the status stays `IN_DELETION` (`:99-113`). +5. Calls `integrationRepository.deleteIntegrationById(integrationId)` (`:115`). + +Step 5 is the only database write, and what it actually removes differs per backend: + +| Backend | Implementation | Cascade behaviour | +|---|---|---| +| PostgreSQL | `prisma.integration.delete` (`integration-repository-postgres.js:178-186`) | Real FK cascades: `IntegrationMapping`, `Sync` (→ `DataIdentifier`), `Association` (→ `AssociationObject`), `Process`, plus the implicit `_EntityToIntegration` join rows (`prisma-postgresql/schema.prisma:196,218,239,261,278,308`) | +| MongoDB | `prisma.integration.delete` (`integration-repository-mongo.js:135-142`) | The same `onDelete: Cascade` declarations exist (`prisma-mongodb/schema.prisma:205,233,270,316`) but there are no DB-level FKs — the actions are **emulated by Prisma Client**, and the repo's own guidance is not to rely on them (see below) | +| DocumentDB | `deleteOne(...)` → `prisma.$runCommandRaw({delete: 'Integration', ...})` (`integration-repository-documentdb.js:52-58`, `database/documentdb-utils.js:91-101`) | **None.** The raw wire-protocol command bypasses the Prisma query engine entirely, so no emulated cascade fires | + +`deleteMappingsByIntegration` exists on all three backends +(`integration-mapping-repository-mongo.js:125`, `-postgres.js:180`, `-documentdb.js:193`) +but has **no caller anywhere in the repo** — mapping cleanup today is purely a side effect +of the ORM cascade. + +The codebase already records that the MongoDB half of that table is not to be trusted. +`user-repository-mongo.js:275-284`: + +> Prisma's `onDelete: Cascade` does NOT work reliably with MongoDB (no database-level +> referential integrity). Integration developers MUST manually cascade delete related +> records before calling this method: 1. Delete integrations … 2. Delete entities … +> 3. Delete credentials … 4. Finally delete user + +So the manual-cascade requirement, and even its ordering, is already written down for the +user path. This ADR applies the same rule to the integration path. + +There is also no re-entrancy guard: the record's existing status is never inspected, so a +retried or concurrent `DELETE` re-runs `persistStatus` and the full `ON_DELETE` teardown. +`onDelete` implementations therefore have to be idempotent already — the change below does +not introduce that requirement, but it does make it worth documenting. + +### What is left behind + +Left behind on **every** backend: + +- `Entity` rows for every entity the integration used. +- `Credential` rows behind those entities — including encrypted `access_token` / + `refresh_token` / `domain` fields (`database/encryption/encryption-schema-registry.js`). + A revoked-but-undeleted credential is the worst of the set: live secrets at rest with no + owner. +- The `User` row and its `Token` rows. + +Left behind on **DocumentDB always** (no cascade of any kind fires), and on **MongoDB +whenever the emulated cascade does not**: `IntegrationMapping`, `Process`, `Sync`, +`DataIdentifier`, `Association`, `AssociationObject`. PostgreSQL is the only backend where +these are reliably removed, because it is the only one with real foreign keys. + +Dangling references: + +- MongoDB `Entity.integrationIds` is a scalar ObjectId list + (`prisma-mongodb/schema.prisma:135-136`). `createIntegration` writes only the + `Integration.entityIds` side (`integration-repository-mongo.js:315-328`), so the entity + side is already never populated. Every `Integration ↔ Entity` read in the codebase goes + through the integration side (`entityIds: { has: entityId }`, + `integration-repository-mongo.js:283-291`), which is why nothing has broken. Cleanup code + must keep using the integration side and must not start trusting `Entity.integrationIds`. +- PostgreSQL join rows are removed by the implicit m2m cascade, so no dangling refs there. +- MongoDB `Process` has no parent/child *relation* — just `childProcesses String[]` / + `parentProcessId String?` (`prisma-mongodb/schema.prisma:247-248`) — whereas PostgreSQL + declares a self-relation with `onDelete: SetNull` + (`prisma-postgresql/schema.prisma:322-324`). Process-hierarchy pointers therefore go stale + on MongoDB and are nulled on PostgreSQL. +- DocumentDB writes `IntegrationMapping.integrationId` as a plain string + (`integration-mapping-repository-documentdb.js:122,282-289`) while the shared mongodb + schema declares it `@db.ObjectId`. Purge filters must match per backend: ObjectId for + MongoDB/Prisma, raw string for DocumentDB. + +Deliberately retained, and must stay retained: + +- `UsageCounter` — "deliberately isolated: no userId and no foreign key to Integration, so + … usage history survives integration deletion" + (`prisma-postgresql/schema.prisma:416-418`, `prisma-mongodb/schema.prisma:434-436`). +- `WebsocketConnection` and `State` — neither model has a user, entity or integration + reference at all (`prisma-postgresql/schema.prisma:343-353`). +- `ScriptSchedule` / `AdminScriptExecution` — app-level admin machinery (ADR-005), never + per-integration. + +Outside the database: + +- EventBridge one-time schedules created via `createSchedulerCommands()` + (`application/commands/scheduler-commands.js:104,188`). Job ids are integration-defined + strings (`'zoho-notif-renewal-abc123'`, `:14`) and the scheduler port exposes only + `scheduleOneTime` / `deleteSchedule` / `getScheduleStatus` + (`infrastructure/scheduler/scheduler-service-interface.js:22,32,42`) — there is no list + operation, so the framework cannot enumerate an integration's schedules. +- In-flight SQS messages naming the deleted integration. +- Provider-side webhooks and subscriptions. + +### Why "delete everything the integration touches" is the wrong predicate + +Three records are shared by design, and deleting them unconditionally is data loss: + +1. **`Entity` is shared across integrations.** `findOrCreateEntity` matches on + `{ externalId, user, moduleName }` and returns the existing row + (`modules/use-cases/process-authorization-callback.js:202-232`); it only creates when no + match exists (`:233-238`). ADR-024 states the goal explicitly — "One Nagaris entity + should serve both Nagaris CRM integration AND Nagaris Analytics integration" + (`docs/MULTI_STEP_AUTH_AND_SHARED_ENTITIES_SPEC.md:11,37-40`). The schema models it as + many-to-many on all backends. +2. **`Credential` is shared across entities.** `Credential.entities` is one-to-many + (`prisma-postgresql/schema.prisma:109`), `upsertCredential` matches an existing + credential by `externalId`, and re-auth explicitly repoints an entity at a different + credential (`process-authorization-callback.js:212-229`). PostgreSQL even declares + `Entity.credential` as `onDelete: SetNull` (`:119`), i.e. the schema's own answer to + "credential deleted" is "orphan the entity", not "delete it". +3. **`User` is per app-user, not per integration.** Users are looked up by `appUserId` / + `appOrgId` on every authenticated request + (`user/use-cases/authenticate-user.js:51-93`, + `get-user-from-x-frigg-headers.js`, `get-user-from-adopter-jwt.js:76-113`), and + `User.integrations` is a list (`prisma-postgresql/schema.prisma:56`). Deleting the user + when one of their integrations is deleted would cascade away every other integration + that user owns (`onDelete: Cascade` on `Integration.user`, `:152`), their credentials + (`:97`), their entities (`:121`) and their sessions (`:82`). It would also break the + next authenticated request for a user the adopter's app still considers active. + +Global entities (ADR-024) sharpen the same point: an app-owner-owned entity is intended to +back many users' integrations. The `isGlobal` field does not exist in the schema or the +code yet (ADR-024 is Proposed and notes the feature is non-functional for exactly that +reason), so it cannot be checked today — but the predicate must be written so that adding +the check later is a one-line change. + +## Decision + +Add an explicit, backend-uniform purge that runs as part of integration deletion. Nothing +relies on ORM cascades any more; the same code path produces the same end state on +MongoDB, PostgreSQL and DocumentDB. Records the integration owns outright are deleted +unconditionally. Records that can be shared are deleted only when the integration being +deleted was their last referent. The `User` row is never touched. + +### 1. Deletion policy + +| Record | Policy | Predicate | Rationale | +|---|---|---|---| +| `Integration` | always delete | — | The thing being deleted. Deleted **last**, so `IN_DELETION` remains a durable checkpoint for the whole purge | +| `IntegrationMapping` | always delete | `integrationId = X` | Owned outright. Explicit call, not cascade — DocumentDB has none | +| `Sync` | always delete | `integrationId = X` | Owned outright | +| `DataIdentifier` | always delete | `syncId ∈ deleted syncs` | Cascades from `Sync` on MongoDB/PostgreSQL; explicit `deleteMany` on DocumentDB | +| `Association` | always delete | `integrationId = X` | Owned outright | +| `AssociationObject` | always delete | `associationId ∈ deleted associations` | Cascades on MongoDB/PostgreSQL; explicit on DocumentDB | +| `Process` | always delete | `integrationId = X` | Owned outright. A whole-integration delete makes the hierarchy moot, so children go with it rather than being re-parented | +| `Entity` | delete if orphaned | `findIntegrationsByEntityId(e).filter(i => i.id !== X).length === 0` **and** `entity.isGlobal !== true` **and** `entity.userId === integration.userId` | Shared by design (see Context). The user check is defensive: an entity owned by another user is never collateral of this delete | +| `Credential` | delete if orphaned | after entity deletion, `findEntities({ credentialId }).length === 0` | One credential can back several entities | +| `User` | **never** | — | Per app-user, reused across integrations, cascades to everything (see Context). Account erasure is a separate operation | +| `Token` | never | — | Session-scoped, user-scoped. Cascades only if a user is deleted | +| `UsageCounter` | never | — | Explicitly designed to survive integration deletion | +| `WebsocketConnection`, `State` | never | — | No integration, entity, credential or user reference exists | +| `ScriptSchedule`, `AdminScriptExecution` | never | — | App-level admin machinery, not per-integration | +| EventBridge schedules | integration's `onDelete` | — | Job ids are integration-defined and the scheduler port has no list operation. Optional follow-up in step 8 | +| In-flight SQS messages | never | — | Cannot be selectively purged. Handlers must tolerate a missing integration (see §4) | +| S3 artifacts (ADR-022) | out of scope | — | Separate lifecycle; call out explicitly rather than half-handle it | + +### 2. Order of operations + +`PurgeIntegrationData.execute({ integrationId, userId, entityIds })`: + +1. `integrationMappingRepository.deleteMappingsByIntegration(integrationId)` +2. `processRepository.deleteByIntegrationId(integrationId)` +3. `syncRepository.deleteSyncsByIntegrationId(integrationId)` +4. `associationRepository.deleteByIntegrationId(integrationId)` +5. Orphan sweep, per entity id captured from the integration record **before** anything was + deleted: + - `others = findIntegrationsByEntityId(entityId).filter(i => String(i.id) !== String(integrationId))` + - skip if `others.length > 0` (shared), if the entity is gone, if `entity.isGlobal === true`, + or if `entity.userId` is set and differs from the integration's `userId`. + Note `findEntityById` **throws** `Entity not found` on all three backends + (`module-repository-mongo.js:92-94`, `-postgres.js:126-128`, `-documentdb.js:35-41`) + rather than returning null, so the "already gone" branch must catch, or the sweep must + read through `findEntities({ id })` instead. Getting this wrong is what would make a + retried purge fail instead of converging + - remember `credentialId = entity.credential?.id`, then `moduleRepository.deleteEntity(entityId)` + - if `credentialId` and `findEntities({ credentialId }).length === 0` → + `credentialRepository.deleteCredentialById(credentialId)` +6. `integrationRepository.deleteIntegrationById(integrationId)` + +Two properties fall out of that ordering, and both are the reason for it: + +- **One durable checkpoint.** The `Integration` row — and therefore its `IN_DELETION` + status and its `errors` messages — survives until the very last step. A crash anywhere in + 1–5 leaves a record an operator (or a retried `DELETE`) can still see and act on. That is + why the reference-count predicate excludes the integration explicitly instead of relying + on the row already being gone. +- **Idempotence, so retry is the recovery story.** Every step is "delete rows matching this + id" or "delete this row if nothing else points at it". Re-running the purge after a + partial failure converges to the same end state. Children are deleted before parents, so + no step can violate a PostgreSQL FK. + +Deleting entities while the `Integration` row still exists is safe: on PostgreSQL the +implicit `_EntityToIntegration` join rows cascade from the entity, and +`DataIdentifier`/`AssociationObject` rows referencing that entity cascade too +(`prisma-postgresql/schema.prisma:241,280`) — all of them already dead by that point. + +### 3. Transactions + +Do not require them. PostgreSQL can wrap steps 1–4 and 6 in `prisma.$transaction`, but +Prisma transactions on MongoDB need a replica set (not guaranteed in local Docker setups), +and the DocumentDB adapter issues one `$runCommandRaw` per operation with no transaction +helper in `documentdb-utils.js` at all. Rather than a capability that silently exists on one +backend, the ordering above is chosen so that a partial failure can only leave *extra* +rows — never a dangling reference — and the retry/sweeper path finishes the job. A +per-backend `runInTransaction(fn)` that PostgreSQL implements and the others no-op is a +reasonable later refinement, not a prerequisite. + +### 4. Code changes + +**New use case** — `packages/core/integrations/use-cases/purge-integration-data.js`, +exported from `use-cases/index.js`. Constructor-injected: +`integrationRepository`, `integrationMappingRepository`, `processRepository`, +`syncRepository`, `associationRepository`, `moduleRepository`, `credentialRepository`, +plus a `policy` object (§5). Returns a summary — counts deleted per model, and a list of +`{ entityId, reason }` skips — so the caller can log it and telemetry can count it. + +**`DeleteIntegrationForUser`** — inject `purgeIntegrationData` and replace the bare +`deleteIntegrationById` call (`:115`) with a purge call that receives the entity ids +snapshotted from the record at `:60`. Keep the existing `ON_DELETE` failure semantics +untouched: provider-side teardown must still succeed before any local data is destroyed. + +**Repository methods to add** (interface + all three adapters + tests): + +| Repository | Method | Notes | +|---|---|---| +| `sync-repository-*` | `deleteSyncsByIntegrationId(integrationId)` | Mongo/Postgres: `prisma.sync.deleteMany`. DocumentDB: `deleteMany` on `Sync`, then `DataIdentifier` by the collected sync ids — no cascade there | +| `process-repository-*` | `deleteByIntegrationId(integrationId)` | Only `findByIntegrationAndType` / `findActiveProcesses` exist today (`process-repository-interface.js:102,112`); `deleteById` is single-row (`:130`) | +| `association-repository-*` | new repository | No association repository exists at all; `Association`/`AssociationObject` are referenced only by the schema-init test (`database/utils/mongodb-schema-init.test.js:9`). Add a minimal repository with `deleteByIntegrationId` so DocumentDB is covered and the model stops being invisible | +| `integration-repository-*` | make `deleteIntegrationById` idempotent | Mongo/Postgres throw Prisma `P2025` on a missing row and do not catch it. Catch it and return `deletedCount: 0` so a retried purge does not 500 — same pattern as `deleteUser` (`user-repository-mongo.js:294-301`) and the mapping repositories | + +Already present and reused unchanged: `findIntegrationsByEntityId` +(`integration-repository-mongo.js:283`, `-postgres.js:464`, `-documentdb.js:43`), +`deleteMappingsByIntegration`, `moduleRepository.findEntityById` / `findEntities` / +`deleteEntity` (`module-repository-mongo.js:385`, `-postgres.js:428`, `-documentdb.js:189`), +`credentialRepository.deleteCredentialById` (`credential-repository-mongo.js:74`, +`-postgres.js:88`, `-documentdb.js:68`). + +**DI wiring** — `createIntegrationRouter()` already builds `moduleRepository`, +`integrationRepository` and `credentialRepository` +(`integration-router.js:73-115`); add the mapping, process, sync and association factories +(`createIntegrationMappingRepository`, `createProcessRepository`, `createSyncRepository` all +exist) and pass the purge use case into `DeleteIntegrationForUser` at `:111`. + +**Queue/worker guard** — an in-flight SQS message for a deleted integration must fail +closed, not loudly: workers that hydrate an integration by id should treat "not found" as a +completed message (ack and drop) rather than retry into the DLQ. + +### 5. Configuration + +Cascade-deleting credentials is the one part of this that an adopter might reasonably want +off (an installation that treats credentials as long-lived connections, reused when the user +re-adds an integration). Add a `deletion` block to the app definition: + +```javascript +// backend/index.js +Definition = { + deletion: { + purgeOrphanedEntities: true, // default true + purgeOrphanedCredentials: true, // default true + }, +}; +``` + +`loadAppDefinition()` destructures a fixed key set and returns a fixed object +(`handlers/app-definition-loader.js:33-38,62`), so the key must be added there and defaulted +to `{}`. Steps 1–4 and 6 are not configurable — those rows are the integration's own and +have no other referent. + +### 6. Failure handling + +- `ON_DELETE` failure keeps today's behaviour exactly: error message appended, status stays + `IN_DELETION`, nothing purged, error rethrown (`:99-113`). Provider-side teardown before + local destruction stays the invariant. +- Purge failure: log with the integration id and the step that failed, append an `errors` + message via `updateIntegrationMessages`, leave the row `IN_DELETION`, rethrow. The client + gets a 500; a retried `DELETE` resumes and converges. +- Skips are logged at `warn` with the reason (`shared`, `global`, `foreign-user`, + `already-gone`) so "why is this entity still here" is answerable from logs. + +### 7. Orphan sweeper + +Existing deployments already hold orphans — every integration ever deleted, plus every +child row on DocumentDB. Ship an admin script (ADR-005 runner) `purge-orphaned-records` +that reports counts by default and deletes with `--apply`: + +- `Entity` rows no integration references, respecting the same skip predicates +- `Credential` rows no entity references +- `IntegrationMapping` / `Sync` / `DataIdentifier` / `Association` / `AssociationObject` / + `Process` rows whose parent id no longer resolves + +It doubles as the recovery path for a purge that failed after the `Integration` row was +already gone. + +### 8. Optional follow-up: scheduled-job sweep + +Add `listSchedules({ namePrefix })` to `scheduler-service-interface.js` plus the EventBridge +and mock adapters (EventBridge Scheduler's `ListSchedules` supports `NamePrefix`), and a +`deleteJobsByPrefix(integrationId)` command. That only helps integrations that include the +integration id in their job ids, so document the convention rather than implying the +framework can clean up arbitrary job names. + +## Implementation plan + +Each step is independently reviewable and independently verifiable. + +1. **This ADR** + a row in `docs/architecture-decisions/README.md`. +2. **Repository primitives.** `deleteSyncsByIntegrationId`, `processRepository.deleteByIntegrationId`, + the new association repository, and `P2025`-tolerant `deleteIntegrationById` — interface + first, then mongo / postgres / documentdb, with a repository test per backend. +3. **`PurgeIntegrationData` use case** + unit tests against repository doubles. Test matrix: + sole-owner entity deleted; shared entity kept; credential with one entity deleted; + credential with two entities kept; foreign-user entity skipped; already-deleted entity + skipped; `isGlobal` entity skipped (forward-compat, asserted against a stub field); + re-running the purge is a no-op. +4. **Wire into `DeleteIntegrationForUser`** + extend + `tests/use-cases/delete-integration-for-user.test.js` and the doubles in + `tests/doubles/` (`test-integration-repository.js` needs the ref-count and bulk-delete + methods). +5. **Router DI** in `createIntegrationRouter()`. +6. **App-definition `deletion` block** in `app-definition-loader.js` + defaults + a loader test. +7. **Orphan sweeper admin script** with dry-run default. +8. **Queue/worker "integration not found → ack" guard.** +9. **Docs**: `packages/core/CLAUDE.md` deletion section, `docs/DANGER_ZONES.md`, the + `onDelete` contract in `integration-base.js` (what the framework cleans up vs. what the + integration still owns — provider webhooks and scheduled jobs — and that `onDelete` must + be idempotent because a retried `DELETE` re-runs it), and the `delete` operation in + `docs/frigg-management-api.yml:312-322`, which currently documents only the 204. +10. **Optional**: scheduler `listSchedules` + prefix sweep (§8). + +No schema migration is required — the change is pure application code. That is deliberate: +it keeps the change deployable to existing installations with no coordination, and it is a +reason to prefer reference counting in the use case over adding schema-level machinery. + +## Testing + +- Use case unit tests: `cd packages/core && npx jest integrations/tests/use-cases/` +- Repository tests per backend: `cd packages/core && npx jest integrations/repositories syncs credential modules` +- Full package: `cd packages/core && npx jest` — and `npm run test:all` from the root before + the PR, per `CLAUDE.md`. +- Real-database verification (PostgreSQL and MongoDB via Docker, no mocks) with the + `frigg-canary-test` harness: create an integration, add a second integration sharing one + entity, delete the first, and assert the shared entity and its credential survive while + the sole-owner entity and credential are gone. DocumentDB has no local emulator; its + adapter is covered by unit tests plus a manual check against a real cluster before + release. + +## Consequences + +### Positive + +- Deleting an integration stops leaving live OAuth refresh tokens at rest. +- DocumentDB stops orphaning the integration's own child rows — currently the worst-affected + backend and the least obvious, because the schema *says* `onDelete: Cascade`. +- One code path, one end state, for all three backends. Deletion behaviour becomes + something you can read in a use case instead of inferring from a schema annotation plus + the ORM's emulation rules plus which adapter you happen to be running. +- Shared entities (ADR-024) get a deletion story before the feature ships, rather than + after the first data-loss report. + +### Negative + +- More repository surface: three new bulk-delete methods across three backends, plus a + repository for a model that had none. +- Deletion is now a multi-step destructive operation without a transaction on two of three + backends. Mitigated by ordering and idempotence, but a partial failure does leave extra + rows until a retry or the sweeper runs. +- Deleting a credential does not revoke it at the provider. The token stops existing locally + and keeps existing upstream until it expires — worth stating plainly rather than implying + full teardown. +- Reference counting costs one query per entity plus one per credential. Negligible at + realistic entity counts, and only on the delete path. + +### Neutral + +- `User`, `Token` and `UsageCounter` behaviour is unchanged. +- Existing orphans are untouched until the sweeper is run deliberately. + +## Alternatives Considered + +**Lean on schema referential actions.** Making the ORM do the work fails on both ends: it +cannot express "delete only if nothing else references this" (which is the entire +shared-entity problem), and it does not run at all on DocumentDB, where the adapter bypasses +the query engine. It also would not fire for the `Entity → Credential` direction, which +PostgreSQL deliberately declares `SetNull`. + +**Soft delete (`deletedAt` everywhere).** Reversible and audit-friendly, but it does not +solve the stated problem — the secrets stay in the database — and every read path in the +codebase would need a `deletedAt: null` filter, which is a large, easy-to-miss change. If +audit history is wanted, the right shape is an append-only deletion log recording what was +purged, not tombstoned rows. + +**Delete the `User` too.** The maintainer's framing named the user record, so it is worth +being explicit: it is not safe. Users are keyed by the adopter's `appUserId`, reused across +integrations, and cascade to every integration, credential, entity and session they own. +"Erase this account and everything in it" is a legitimate operation — as a separate, +explicitly-invoked use case (a GDPR erasure path), not as a side effect of deleting one +integration. + +**Fire-and-forget async purge (queue a cleanup job).** Keeps the API response fast, but +splits the durable state across a queue and hides failures. The synchronous purge with +`IN_DELETION` as the checkpoint keeps one place to look. + +## Open questions for the maintainer + +1. **Credential purge default.** Recommend `purgeOrphanedCredentials: true`, since dead + credentials holding live refresh tokens are the main motivation. Adopters who treat a + credential as a reusable connection may want `false`. +2. **Provider-side revocation.** Should credential deletion attempt an OAuth revoke where + the module supports it? Not currently modelled in `requiredAuthMethods`; would need a new + optional method. +3. **Account erasure.** Should the GDPR-style "delete user and everything they own" use case + land in the same change, or as its own ADR? +4. **Deletion audit log.** Worth recording purge summaries durably (an `AdminScriptExecution` + -shaped row, or telemetry only)? +5. **Adjacent papercut, in or out?** An ownership mismatch throws a plain `Error` + (`delete-integration-for-user.js:51-53`), which the error middleware boomifies to a 500 + `{"error":"Internal Server Error"}` (`handlers/app-handler-helpers.js:26-37`) instead of a + 403/404. One-line fix in the same file this change already touches, but it is a + behaviour change for API clients, so it should be an explicit yes/no rather than a + drive-by. + +## Related + +- ADR-005: Admin Script Runner Service — home for the orphan sweeper +- ADR-011: Integration Telemetry, Eventing & Feature-Usage Tracking — why `UsageCounter` survives +- ADR-022: Artifacts — S3 artifact lifecycle, explicitly out of scope here +- ADR-024: Global Entities — `isGlobal` skip predicate, pending that field existing +- `docs/MULTI_STEP_AUTH_AND_SHARED_ENTITIES_SPEC.md` — shared-entity design intent diff --git a/docs/architecture-decisions/README.md b/docs/architecture-decisions/README.md index 8e9590398..448ffcdf7 100644 --- a/docs/architecture-decisions/README.md +++ b/docs/architecture-decisions/README.md @@ -45,6 +45,7 @@ An ADR documents a significant architectural decision made in the project, inclu | [026](./026-evals.md) | Evals | Proposed | 2026-06-09 | | [027](./027-ssm-parameter-offload-and-env-scoping.md) | SSM Parameter Offload and Per-Function Environment Scoping | Accepted | 2026-07-10 | | [031](./031-concurrent-oauth-credential-refresh.md) | Concurrent OAuth Credential Refresh Across Lambda Invocations | Proposed | 2026-08-11 | +| [032](./032-integration-deletion-cleanup.md) | Integration Deletion Data Cleanup | Proposed | 2026-08-20 | ## Conventions From d9d7daf8b5b4c3f464cf14cb8bb0c8d27fc3f40e Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 20 Aug 2026 20:29:53 +0000 Subject: [PATCH 2/7] docs(adr): fold adjacent missing-predicate findings into ADR-032 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Claude-Session: https://claude.ai/code/session_01M5jrmtqxucVoXrxvw7ph8h --- .../032-integration-deletion-cleanup.md | 42 ++++++++++++++++++- 1 file changed, 40 insertions(+), 2 deletions(-) diff --git a/docs/architecture-decisions/032-integration-deletion-cleanup.md b/docs/architecture-decisions/032-integration-deletion-cleanup.md index f446d547e..bf32a3528 100644 --- a/docs/architecture-decisions/032-integration-deletion-cleanup.md +++ b/docs/architecture-decisions/032-integration-deletion-cleanup.md @@ -142,6 +142,29 @@ Three records are shared by design, and deleting them unconditionally is data lo (`:97`), their entities (`:121`) and their sessions (`:82`). It would also break the next authenticated request for a user the adopter's app still considers active. +### The same missing predicate, elsewhere + +Integration deletion is where the gap is most visible, but nothing in the codebase +reference-counts these records today. A repo-wide search for reference counting or orphan +cleanup of data records finds one hit, and it is a log line rather than logic: +`[Frigg] findIntegrationByEntityExternalId: entity ${entity.id} has no owning integrations (orphan)` +(`integrations/use-cases/find-integration-by-entity-external-id.js:57`). Orphaned entities +are already a condition the framework recognises at runtime and does nothing about. + +Two other paths delete these records with no reference check at all: + +- `Module.deauthorize()` (`modules/module.js:206-227`) deletes the `Credential` row + unconditionally and then unsets `credentialId` on **only the entity it happens to hold in + memory**. Any sibling entity sharing that credential keeps a dangling pointer. Same + predicate, same fix — worth doing with the same helper rather than separately. +- The developer command surface exposes the raw primitives with only a truthy-id check: + `createEntityCommands().deleteEntity` / `deleteEntityById` / `unsetCredential` + (`application/commands/entity-commands.js:273,294,315`) and + `createCredentialCommands().deleteCredential` / `deleteCredentialById` + (`credential-commands.js:203,241`). Those stay as-is — they are the primitives — but the + safe, reference-counted operation should be reachable from the same surface so integration + authors are not obliged to reimplement the predicate. + Global entities (ADR-024) sharpen the same point: an app-owner-owned entity is intended to back many users' integrations. The `isGlobal` field does not exist in the schema or the code yet (ADR-024 is Proposed and notes the feature is non-functional for exactly that @@ -195,7 +218,10 @@ deleted was their last referent. The `User` row is never touched. (`module-repository-mongo.js:92-94`, `-postgres.js:126-128`, `-documentdb.js:35-41`) rather than returning null, so the "already gone" branch must catch, or the sweep must read through `findEntities({ id })` instead. Getting this wrong is what would make a - retried purge fail instead of converging + retried purge fail instead of converging. `deleteEntity` and `deleteCredentialById` + already swallow `P2025` and report "nothing deleted" + (`module-repository-mongo.js:385-398`, `credential-repository-mongo.js:74-86`), so the + write side of a retry is already safe — it is only the read that throws - remember `credentialId = entity.credential?.id`, then `moduleRepository.deleteEntity(entityId)` - if `credentialId` and `findEntities({ credentialId }).length === 0` → `credentialRepository.deleteCredentialById(credentialId)` @@ -252,6 +278,13 @@ untouched: provider-side teardown must still succeed before any local data is de | `association-repository-*` | new repository | No association repository exists at all; `Association`/`AssociationObject` are referenced only by the schema-init test (`database/utils/mongodb-schema-init.test.js:9`). Add a minimal repository with `deleteByIntegrationId` so DocumentDB is covered and the model stops being invisible | | `integration-repository-*` | make `deleteIntegrationById` idempotent | Mongo/Postgres throw Prisma `P2025` on a missing row and do not catch it. Catch it and return `deletedCount: 0` so a retried purge does not 500 — same pattern as `deleteUser` (`user-repository-mongo.js:294-301`) and the mapping repositories | +**Shared orphan-check helper** — the entity and credential predicates should live in one +place the purge and `Module.deauthorize()` both call, so the deauthorize bug +(`modules/module.js:206-227`, credential deleted without checking sibling entities) is fixed +by construction rather than separately. Expose the reference-counted operation on the +developer command surface alongside the existing raw `deleteEntity` / `deleteCredential` +commands. + Already present and reused unchanged: `findIntegrationsByEntityId` (`integration-repository-mongo.js:283`, `-postgres.js:464`, `-documentdb.js:43`), `deleteMappingsByIntegration`, `moduleRepository.findEntityById` / `findEntities` / @@ -433,7 +466,12 @@ splits the durable state across a queue and hides failures. The synchronous purg the module supports it? Not currently modelled in `requiredAuthMethods`; would need a new optional method. 3. **Account erasure.** Should the GDPR-style "delete user and everything they own" use case - land in the same change, or as its own ADR? + land in the same change, or as its own ADR? The pieces are already half-built: `deleteUser` + exists on all three backends (`user-repository-interface.js:195`), it is exposed to + developers as `deleteUserById` (`application/commands/user-commands.js:281`) whose docblock + spells out the manual cascade order but does not perform it, no framework flow calls it, + and ADR-007 sketches a `DELETE /users/:id` route (`:229`) that was never implemented. An + erasure use case would give that command the cascade its own documentation asks for. 4. **Deletion audit log.** Worth recording purge summaries durably (an `AdminScriptExecution` -shaped row, or telemetry only)? 5. **Adjacent papercut, in or out?** An ownership mismatch throws a plain `Error` From ae220b4853677ce2ebad61c44bfce5b5a1b21c44 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 20 Aug 2026 21:21:12 +0000 Subject: [PATCH 3/7] docs(adr): correct ADR-032 after adversarial review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Claude-Session: https://claude.ai/code/session_01M5jrmtqxucVoXrxvw7ph8h --- .../032-integration-deletion-cleanup.md | 254 +++++++++++++++--- 1 file changed, 211 insertions(+), 43 deletions(-) diff --git a/docs/architecture-decisions/032-integration-deletion-cleanup.md b/docs/architecture-decisions/032-integration-deletion-cleanup.md index bf32a3528..6d1e9ae53 100644 --- a/docs/architecture-decisions/032-integration-deletion-cleanup.md +++ b/docs/architecture-decisions/032-integration-deletion-cleanup.md @@ -83,7 +83,14 @@ Dangling references: through the integration side (`entityIds: { has: entityId }`, `integration-repository-mongo.js:283-291`), which is why nothing has broken. Cleanup code must keep using the integration side and must not start trusting `Entity.integrationIds`. -- PostgreSQL join rows are removed by the implicit m2m cascade, so no dangling refs there. + `Entity.syncIds` / `Sync.entityIds` (`:138-139`, `:273-274`) are a second pair with the + same property. Prisma forbids referential actions on implicit many-to-many relations and + does not strip a deleted document's id from the surviving side's scalar list, so any id + written into these arrays would dangle permanently. +- PostgreSQL join rows are removed by the implicit m2m cascade on both sides of + `_EntityToIntegration` and `_EntityToSync` + (`prisma-postgresql/migrations/20250930193005_init/migration.sql:306-315`), so no dangling + refs are possible there. - MongoDB `Process` has no parent/child *relation* — just `childProcesses String[]` / `parentProcessId String?` (`prisma-mongodb/schema.prisma:247-248`) — whereas PostgreSQL declares a self-relation with `onDelete: SetNull` @@ -142,6 +149,32 @@ Three records are shared by design, and deleting them unconditionally is data lo (`:97`), their entities (`:121`) and their sessions (`:82`). It would also break the next authenticated request for a user the adopter's app still considers active. +### Two findings that change the design + +**`Sync` rows are never linked to their integration.** The schema declares +`Sync.integrationId` as optional with `onDelete: Cascade` +(`prisma-postgresql/schema.prisma:216-218`), but the only framework writer never sets it: +`SyncManager.createSyncDBObject` builds `{ name, entities, hash, dataIdentifiers }` +(`syncs/manager.js:330-337`) and the integration is captured into `this.integration` +(`:33`) and never used again. Every sync row the framework writes therefore has +`integrationId = null`, which means the cascade matches nothing *and* a purge keyed on +`integrationId` alone would delete zero rows. Those rows are only reachable through their +entity ids. The fix is two-part: make `SyncManager` persist `integrationId`, and have the +purge match on `integrationId` **or** on the integration's entity ids so pre-existing rows +are covered too. + +**Deleting an encrypted row can fail after the row is gone.** The Prisma encryption +extension's `delete` hook runs the query and *then* decrypts the returned record +(`database/encryption/prisma-encryption-extension.js:171-182`), and `decryptFields` → +`Cryptor.decrypt` → KMS has no error handling +(`database/encryption/field-encryption-service.js:101`, `encrypt/Cryptor.js:92-98`). So a +KMS outage or a rotated-away key throws *after* the row has been removed: the caller sees a +500 and may retry, but the data is already gone. The affected models are exactly the ones +this purge touches — `Credential` and `IntegrationMapping` +(`database/encryption/encryption-schema-registry.js:16-41`). `deleteMany` is a pure +pass-through with no crypto (`prisma-encryption-extension.js:184-186`), so purge paths +should prefer it. + ### The same missing predicate, elsewhere Integration deletion is where the gap is most visible, but nothing in the codebase @@ -185,12 +218,12 @@ deleted was their last referent. The `User` row is never touched. |---|---|---|---| | `Integration` | always delete | — | The thing being deleted. Deleted **last**, so `IN_DELETION` remains a durable checkpoint for the whole purge | | `IntegrationMapping` | always delete | `integrationId = X` | Owned outright. Explicit call, not cascade — DocumentDB has none | -| `Sync` | always delete | `integrationId = X` | Owned outright | -| `DataIdentifier` | always delete | `syncId ∈ deleted syncs` | Cascades from `Sync` on MongoDB/PostgreSQL; explicit `deleteMany` on DocumentDB | -| `Association` | always delete | `integrationId = X` | Owned outright | +| `Sync` | always delete | `integrationId = X` **or** `entityIds ∩ integration.entityIds` | Owned outright, but `integrationId` is never written today (see Context), so the entity-id arm is what actually matches existing rows | +| `DataIdentifier` | always delete | `syncId ∈ deleted syncs` | Cascades from `Sync` on MongoDB/PostgreSQL. On **DocumentDB** there is nothing to delete — identifiers are an embedded array inside the Sync document (`sync-repository-documentdb.js:100-121,201-227`), not a collection, so deleting the sync removes them. The two Mongo-family backends genuinely diverge in storage shape here | +| `Association` | always delete | `integrationId = X` | Owned outright. Note nothing in the codebase reads or writes these models today (`associations/association.js` is an in-memory hashing helper with no repository), so this is future-proofing, not a live orphan source | | `AssociationObject` | always delete | `associationId ∈ deleted associations` | Cascades on MongoDB/PostgreSQL; explicit on DocumentDB | | `Process` | always delete | `integrationId = X` | Owned outright. A whole-integration delete makes the hierarchy moot, so children go with it rather than being re-parented | -| `Entity` | delete if orphaned | `findIntegrationsByEntityId(e).filter(i => i.id !== X).length === 0` **and** `entity.isGlobal !== true` **and** `entity.userId === integration.userId` | Shared by design (see Context). The user check is defensive: an entity owned by another user is never collateral of this delete | +| `Entity` | delete if orphaned | `findIntegrationsByEntityId(e).filter(i => i.id !== X).length === 0` **and** `entity.isGlobal !== true` **and** `String(entity.userId) === String(integration.userId)` | Shared by design (see Context). The ownership check is strict on purpose: `Entity.userId` is nullable (`prisma-postgresql/schema.prisma:120`) and a null owner is the shape ADR-024 gives a global entity, so a null `userId` **skips**, it does not match | | `Credential` | delete if orphaned | after entity deletion, `findEntities({ credentialId }).length === 0` | One credential can back several entities | | `User` | **never** | — | Per app-user, reused across integrations, cascades to everything (see Context). Account erasure is a separate operation | | `Token` | never | — | Session-scoped, user-scoped. Cascades only if a user is deleted | @@ -199,7 +232,7 @@ deleted was their last referent. The `User` row is never touched. | `ScriptSchedule`, `AdminScriptExecution` | never | — | App-level admin machinery, not per-integration | | EventBridge schedules | integration's `onDelete` | — | Job ids are integration-defined and the scheduler port has no list operation. Optional follow-up in step 8 | | In-flight SQS messages | never | — | Cannot be selectively purged. Handlers must tolerate a missing integration (see §4) | -| S3 artifacts (ADR-022) | out of scope | — | Separate lifecycle; call out explicitly rather than half-handle it | +| S3 report artifacts (ADR-010) | out of scope | — | Keys are execution-scoped (`reports/{executionId}/…`), not integration-scoped, so per-integration purge is impossible by key. Separately: the artifact port has no `delete` at all (`artifacts/repositories/artifact-repository-interface.js:6-25`) and the bucket has no lifecycle policy — worth its own issue. ADR-022 "Artifacts" is a different concept (provider-side deployables) | ### 2. Order of operations @@ -207,13 +240,15 @@ deleted was their last referent. The `User` row is never touched. 1. `integrationMappingRepository.deleteMappingsByIntegration(integrationId)` 2. `processRepository.deleteByIntegrationId(integrationId)` -3. `syncRepository.deleteSyncsByIntegrationId(integrationId)` +3. `syncRepository.deleteSyncsForIntegration({ integrationId, entityIds })` — matching either + arm, because `integrationId` is null on every sync the framework has written so far 4. `associationRepository.deleteByIntegrationId(integrationId)` 5. Orphan sweep, per entity id captured from the integration record **before** anything was deleted: - `others = findIntegrationsByEntityId(entityId).filter(i => String(i.id) !== String(integrationId))` - skip if `others.length > 0` (shared), if the entity is gone, if `entity.isGlobal === true`, - or if `entity.userId` is set and differs from the integration's `userId`. + or if `String(entity.userId) !== String(integration.userId)` — which includes a null + owner, per the policy table. Note `findEntityById` **throws** `Entity not found` on all three backends (`module-repository-mongo.js:92-94`, `-postgres.js:126-128`, `-documentdb.js:35-41`) rather than returning null, so the "already gone" branch must catch, or the sweep must @@ -273,11 +308,18 @@ untouched: provider-side teardown must still succeed before any local data is de | Repository | Method | Notes | |---|---|---| -| `sync-repository-*` | `deleteSyncsByIntegrationId(integrationId)` | Mongo/Postgres: `prisma.sync.deleteMany`. DocumentDB: `deleteMany` on `Sync`, then `DataIdentifier` by the collected sync ids — no cascade there | +| `sync-repository-*` | `deleteSyncsForIntegration({ integrationId, entityIds })` | The entity arm must be scoped to *these* entity ids — never "where `integrationId is null`", which would delete every parentless sync in the database. Mongo/Postgres: `prisma.sync.deleteMany`. MongoDB additionally needs its separate `DataIdentifier` rows removed; DocumentDB does not (embedded). Only `deleteSync(id)` exists today (`sync-repository-interface.js:104`) | | `process-repository-*` | `deleteByIntegrationId(integrationId)` | Only `findByIntegrationAndType` / `findActiveProcesses` exist today (`process-repository-interface.js:102,112`); `deleteById` is single-row (`:130`) | | `association-repository-*` | new repository | No association repository exists at all; `Association`/`AssociationObject` are referenced only by the schema-init test (`database/utils/mongodb-schema-init.test.js:9`). Add a minimal repository with `deleteByIntegrationId` so DocumentDB is covered and the model stops being invisible | | `integration-repository-*` | make `deleteIntegrationById` idempotent | Mongo/Postgres throw Prisma `P2025` on a missing row and do not catch it. Catch it and return `deletedCount: 0` so a retried purge does not 500 — same pattern as `deleteUser` (`user-repository-mongo.js:294-301`) and the mapping repositories | +**Prefer `deleteMany` for encrypted models.** `Credential` and `IntegrationMapping` carry +encrypted fields, and the extension's `delete` hook decrypts the deleted row afterwards, so a +KMS failure throws with the row already gone (see Context). `deleteMany({ where: { id } })` +skips the crypto path entirely. That argues for a `deleteCredentialsByIds` bulk method rather +than looping the existing `deleteCredentialById`, which uses `.delete()` +(`credential-repository-mongo.js:74-86`). + **Shared orphan-check helper** — the entity and credential predicates should live in one place the purge and `Module.deauthorize()` both call, so the deauthorize bug (`modules/module.js:206-227`, credential deleted without checking sibling entities) is fixed @@ -298,9 +340,30 @@ Already present and reused unchanged: `findIntegrationsByEntityId` (`createIntegrationMappingRepository`, `createProcessRepository`, `createSyncRepository` all exist) and pass the purge use case into `DeleteIntegrationForUser` at `:111`. -**Queue/worker guard** — an in-flight SQS message for a deleted integration must fail -closed, not loudly: workers that hydrate an integration by id should treat "not found" as a -completed message (ack and drop) rather than retry into the DLQ. +**Queue/worker guard — a regression this change would otherwise introduce.** The guard +already exists on the `integrationId` path: `createQueueWorker` discards when hydration +returns null and when status is `DISABLED` / `ERROR` / `IN_DELETION` +(`handlers/backend-utils.js:202-229`, tested at `backend-utils.test.js:152-161`). The +`processId` path has no such tolerance — `loadIntegrationForProcess` throws +`Process not found: ${processId}` (`:143-153`). Today such a message hydrates fine and is +discarded on status; once the purge deletes `Process` rows, every in-flight process-keyed +message for a deleted integration starts throwing and lands in the DLQ. Per-integration +queues retain messages for 4 days and the shared error queue for 14 +(`devtools/infrastructure/domains/integration/integration-builder.js:462-469,557-574`), so +this is not theoretical. Fixing that one branch is part of this change, not an optional +follow-up. + +There is a second, sharper version of the same problem. Both paths hydrate **before** they +check status, and `GetIntegrationInstance` loops the entity ids calling +`moduleFactory.getModuleInstance` with no try/catch +(`integrations/use-cases/get-integration-instance.js:57-63`) — unlike `DeleteIntegrationForUser`, +which wraps each load. Since `findEntityById` throws when the entity is gone, a worker that +arrives during the purge window — `Integration` row still present as `IN_DELETION`, entities +already deleted — throws inside hydration and never reaches the `IN_DELETION` discard at +`backend-utils.js:220-229`. The fix is to check status before hydrating modules, or to make +that loop tolerant the way the delete path already is. This hazard exists today in a narrower +form (any manually-deleted entity does it); the purge widens the window, so closing it belongs +to this change. ### 5. Configuration @@ -334,6 +397,38 @@ have no other referent. - Skips are logged at `warn` with the reason (`shared`, `global`, `foreign-user`, `already-gone`) so "why is this entity still here" is answerable from logs. +#### Known limits, stated rather than hidden + +- **Check-then-act is not atomic.** Between the reference-count read and the delete, a + concurrent `createIntegration` could link that entity. There is no unique index or lock to + fall back on. The window is small and the operation requires the same user to be adding an + integration while deleting another, but it is real — and it argues for keeping the + `IN_DELETION` status guard (which stops queue work for this integration) rather than + deleting the row early. +- **Syncs are not consulted by the entity predicate.** An entity can be referenced by a + `Sync` even when no integration references it, and on PostgreSQL deleting the entity + cascades that sync's `DataIdentifier` rows away + (`prisma-postgresql/schema.prisma:241`). While `Sync.integrationId` is unwritten the syncs + are unattributable, so the honest position is: after step 2 makes syncs attributable, the + entity predicate should also require that no *surviving* integration's sync references the + entity. Until then, pre-existing parentless syncs are the sweeper's problem, not the + purge's. +- **Org-linked users are skipped, not resolved.** The predicate compares raw ids, while the + rest of the codebase resolves ownership through `User.ownsUserId`, which understands + individual↔organization membership (`modules/use-cases/get-module.js:32-35`, + `test-module-auth.js:37-38`). The purge only has ids, so an entity owned by the individual + while the integration is owned by the org is skipped. That is the safe direction — a skip + leaves an orphan, a false match destroys live data — but it means org-configured + deployments will accumulate some orphans that only the sweeper clears. +- **Deleting a credential forecloses revoking it.** Once the row is gone the refresh token is + gone with it, so provider-side revocation can never happen afterwards. If revocation is + added later it has to run *before* the purge — `ON_DELETE`, which already executes first + with modules loaded, is the natural place. +- **Guard against empty filters on DocumentDB.** `_buildFilter` can drop an unconvertible id + and return a match-all query. For the credential reference count that fails safe (it looks + like "still referenced", so nothing is deleted), but any purge filter must assert it is + non-empty before issuing a `deleteMany`. + ### 7. Orphan sweeper Existing deployments already hold orphans — every integration ever deleted, plus every @@ -348,6 +443,24 @@ that reports counts by default and deletes with `--apply`: It doubles as the recovery path for a purge that failed after the `Integration` row was already gone. +One constraint makes this more than a scripting job: **admin scripts have no repository +access.** They reach the database only through the injected command bundle — +`{ users, credentials, entities, integrations }` +(`admin-scripts/src/application/admin-script-context.js:7-11`, +`src/infrastructure/bootstrap.js:91-96`) — and the two reads the orphan predicate needs are +not on it: + +- `findIntegrationsByEntityId` exists on the repository interface (`:182`) but is not exposed + as an integration command. +- `commands.entities.findEntity` rejects a filter that carries only `credentialId` — it + requires `externalId`, `userId` or `moduleName` (`entity-commands.js:92-104`) — so the + credential reference count is unreachable. + +Both writes are already reachable (`integration-commands.js:315`, +`entity-commands.js:273,294`, `credential-commands.js:203,241`). So the sweeper step includes +adding those two reads to the command layer — which is also what makes the reference-counted +operation available to integration authors. + ### 8. Optional follow-up: scheduled-job sweep Add `listSchedules({ namePrefix })` to `scheduler-service-interface.js` plus the EventBridge @@ -361,28 +474,47 @@ framework can clean up arbitrary job names. Each step is independently reviewable and independently verifiable. 1. **This ADR** + a row in `docs/architecture-decisions/README.md`. -2. **Repository primitives.** `deleteSyncsByIntegrationId`, `processRepository.deleteByIntegrationId`, - the new association repository, and `P2025`-tolerant `deleteIntegrationById` — interface - first, then mongo / postgres / documentdb, with a repository test per backend. -3. **`PurgeIntegrationData` use case** + unit tests against repository doubles. Test matrix: +2. **Fix `SyncManager` to persist `integrationId`** (`syncs/manager.js:330-337`). Without this + the sync arm of the purge is decorative, and the schema's own `onDelete: Cascade` on + `Sync.integrationId` has never matched a row. Backfilling existing rows is the sweeper's + job, not a migration's. +3. **Repository primitives.** `deleteSyncsForIntegration`, + `processRepository.deleteByIntegrationId`, a bulk `deleteCredentialsByIds` that uses + `deleteMany`, the new association repository, and `P2025`-tolerant + `deleteIntegrationById` — interface first, then mongo / postgres / documentdb. +4. **`PurgeIntegrationData` use case** + unit tests against repository doubles. Test matrix: sole-owner entity deleted; shared entity kept; credential with one entity deleted; - credential with two entities kept; foreign-user entity skipped; already-deleted entity - skipped; `isGlobal` entity skipped (forward-compat, asserted against a stub field); - re-running the purge is a no-op. -4. **Wire into `DeleteIntegrationForUser`** + extend - `tests/use-cases/delete-integration-for-user.test.js` and the doubles in - `tests/doubles/` (`test-integration-repository.js` needs the ref-count and bulk-delete - methods). -5. **Router DI** in `createIntegrationRouter()`. -6. **App-definition `deletion` block** in `app-definition-loader.js` + defaults + a loader test. -7. **Orphan sweeper admin script** with dry-run default. -8. **Queue/worker "integration not found → ack" guard.** -9. **Docs**: `packages/core/CLAUDE.md` deletion section, `docs/DANGER_ZONES.md`, the - `onDelete` contract in `integration-base.js` (what the framework cleans up vs. what the - integration still owns — provider webhooks and scheduled jobs — and that `onDelete` must - be idempotent because a retried `DELETE` re-runs it), and the `delete` operation in - `docs/frigg-management-api.yml:312-322`, which currently documents only the 204. -10. **Optional**: scheduler `listSchedules` + prefix sweep (§8). + credential with two entities kept; entity with a foreign `userId` skipped; entity with a + null `userId` skipped; already-deleted entity skipped; `isGlobal` entity skipped + (forward-compat, asserted against a stub field); sync matched by entity id when + `integrationId` is null; re-running the whole purge is a no-op. +5. **Wire into `DeleteIntegrationForUser`** + extend + `tests/use-cases/delete-integration-for-user.test.js`. The existing failure-path tests + (`:132-232`, `:269-320`, including a `throw null` case) lock in current behaviour — do not + regress them. The double `test-integration-repository.js` already implements + `findIntegrationsByEntityId`; what it needs is the bulk deletes, and its + `deleteIntegrationById` aligned to the real `{ acknowledged, deletedCount }` shape rather + than a bare boolean. Note its field is `entitiesIds`, matching the domain mapping. + `DeleteIntegrationForUser` is constructed without `moduleFactory` throughout that suite, so + it must keep tolerating its absence. +6. **Close the queue-worker window** — two small fixes, both regression guards rather than + niceties (see §4): make the `processId` path discard a missing process + (`handlers/backend-utils.js:143-153`) the way the `integrationId` path already discards a + missing integration, and check integration status *before* hydrating modules so a purge in + progress cannot throw a message into the DLQ + (`integrations/use-cases/get-integration-instance.js:57-63`). +7. **Router DI** in `createIntegrationRouter()`. +8. **App-definition `deletion` block** in `app-definition-loader.js` + defaults + a loader test. +9. **Command-layer reads** — expose `findIntegrationsByEntityId`, and let entity lookup filter + on `credentialId` — then the **orphan sweeper admin script**, dry-run by default. +10. **Docs**: `packages/core/CLAUDE.md`, `docs/DANGER_ZONES.md`, the `onDelete` contract in + `integration-base.js`, and the `delete` operation in `docs/frigg-management-api.yml:312-322` + (which documents only the 204 today). Also `packages/core/integrations/EXTENSIONS.md` and + `WEBHOOK-QUICKSTART.md`: both document registering provider-side webhooks and contain no + mention of deletion, teardown or `onDelete` anywhere — an author following either is never + told they own the removal. `EXTENSIONS.md:175` ("Contract enforced by the framework") is the + natural home for the teardown half. +11. **Optional**: scheduler `listSchedules` + prefix sweep (§8). No schema migration is required — the change is pure application code. That is deliberate: it keeps the change deployable to existing installations with no coordination, and it is a @@ -390,16 +522,40 @@ reason to prefer reference counting in the use case over adding schema-level mac ## Testing -- Use case unit tests: `cd packages/core && npx jest integrations/tests/use-cases/` -- Repository tests per backend: `cd packages/core && npx jest integrations/repositories syncs credential modules` -- Full package: `cd packages/core && npx jest` — and `npm run test:all` from the root before - the PR, per `CLAUDE.md`. -- Real-database verification (PostgreSQL and MongoDB via Docker, no mocks) with the - `frigg-canary-test` harness: create an integration, add a second integration sharing one - entity, delete the first, and assert the shared entity and its credential survive while - the sole-owner entity and credential are gone. DocumentDB has no local emulator; its - adapter is covered by unit tests plus a manual check against a real cluster before - release. +The repo has no real-database test tier: there is no `testcontainers` dependency, no +`docker-compose` file, and no per-backend jest project. Repository tests are +*command-generation* tests — instantiate the adapter, replace `repo.prisma` with a hand-rolled +fake, and assert on the emitted command shape (exemplars: +`integration-repository-mongo.test.js:15-25`, +`integration-repository-documentdb.test.js:17-30`). Match that habit rather than inventing a +new tier. Use-case tests use the stateful fake in `integrations/tests/doubles/`, and every core +test file starts by mocking `database/config` so requiring a use case does not pull a real +Prisma client (copy `delete-integration-for-user.test.js:1-6`, adjusting the relative depth). + +```bash +npm install # from the repo root; needed before any run + +cd packages/core +npm test -- integrations/tests/use-cases/ # use-case suites +npm test -- integrations/repositories/ # integration, mapping, process adapters +npm test -- syncs credential modules # sync, credential, entity adapters +npm test # whole core package + +# from the root, what CI runs +npm run test:all +npm run lint:fix --workspaces +``` + +Note every core jest invocation boots an in-memory mongod via `globalSetup` +(`packages/core/jest.config.js:19-22`), so runs are slower than they look and need the +workspace installed. + +Real-database verification is therefore out-of-suite, with the canary harness against +PostgreSQL and MongoDB: create two integrations sharing one entity, delete the first, and +assert the shared entity and its credential survive while the sole-owner pair is gone. +DocumentDB has no local emulator — its adapters are covered by command-shape tests plus a +manual check against a real cluster before release. That gap is worth stating plainly: the +backend with no cascade at all is also the one that cannot be integration-tested locally. ## Consequences @@ -413,6 +569,9 @@ reason to prefer reference counting in the use case over adding schema-level mac the ORM's emulation rules plus which adapter you happen to be running. - Shared entities (ADR-024) get a deletion story before the feature ships, rather than after the first data-loss report. +- Two latent bugs get fixed on the way: `Sync.integrationId` starts being written, so the + cascade the schema has always declared finally matches something; and + `Module.deauthorize()` stops leaving dangling `credentialId` pointers. ### Negative @@ -426,11 +585,20 @@ reason to prefer reference counting in the use case over adding schema-level mac full teardown. - Reference counting costs one query per entity plus one per credential. Negligible at realistic entity counts, and only on the delete path. +- Deleting `Process` rows turns a currently-harmless in-flight SQS message into a DLQ entry + unless the `processId` hydration path is fixed in the same change. That fix is step 6, and + it is the one part of this plan that is a guard against the plan itself. +- The purge is exercised end to end only against PostgreSQL and MongoDB. DocumentDB — the + backend this change helps most — has no local emulator, so its adapters get command-shape + tests and a manual pre-release check. ### Neutral - `User`, `Token` and `UsageCounter` behaviour is unchanged. - Existing orphans are untouched until the sweeper is run deliberately. +- `Association` / `AssociationObject` gain a repository with nothing to clean yet — nothing + in the codebase writes those models today. It is there so DocumentDB is not the odd one out + if an adopter starts. ## Alternatives Considered From b6eadc655481d6cd3abd7636359d3fd594f8083c Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 20 Aug 2026 21:29:17 +0000 Subject: [PATCH 4/7] docs(adr): three more verified defects, and descope the SyncManager fix MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Claude-Session: https://claude.ai/code/session_01M5jrmtqxucVoXrxvw7ph8h --- .../032-integration-deletion-cleanup.md | 103 ++++++++++++++---- 1 file changed, 81 insertions(+), 22 deletions(-) diff --git a/docs/architecture-decisions/032-integration-deletion-cleanup.md b/docs/architecture-decisions/032-integration-deletion-cleanup.md index 6d1e9ae53..9732eddac 100644 --- a/docs/architecture-decisions/032-integration-deletion-cleanup.md +++ b/docs/architecture-decisions/032-integration-deletion-cleanup.md @@ -149,6 +149,40 @@ Three records are shared by design, and deleting them unconditionally is data lo (`:97`), their entities (`:121`) and their sessions (`:82`). It would also break the next authenticated request for a user the adopter's app still considers active. +### Three defects on this path the plan must fix, not inherit + +**A retried `DELETE` returns 500, not 404.** `findIntegrationById` *throws* on all three +backends — `integration-repository-mongo.js:195`, `-postgres.js:239`, +`-documentdb.js:71,75` — so the `Boom.notFound` guard at +`delete-integration-for-user.js:38-42` is unreachable dead code. Retrying a delete against an +already-deleted integration produces a plain `Error`, which the middleware boomifies to a 500. +Any plan that names retry as its recovery path has to make that read tolerant first. Do it in +the use case rather than by changing the repositories: callers elsewhere depend on the throw +and match on its message (`handlers/backend-utils.js:99-103`). The existing test double +returns `null`, which is exactly why this has stayed invisible. + +**The failure breadcrumb overwrites itself.** `updateIntegrationMessages` reads +`integration.messages` (`integration-repository-mongo.js:254`, `-postgres.js:300`, +`-documentdb.js:110`), but neither schema has a `messages` column — there are four separate +Json arrays, `errors` / `warnings` / `info` / `logs` +(`prisma-postgresql/schema.prisma:164-168`). So the array read is always `[]`, and the write +`data: { [messageType]: messageArray }` replaces the column with a single element. Every +deletion error message destroys the previous one, and any pre-existing errors on the +integration. The purge's error reporting depends on this appending, so it needs fixing here. + +**There is a second delete path that skips the lifecycle entirely.** +`application/commands/integration-commands.js:315-339` calls `deleteIntegrationById` raw — no +ownership check, no `IN_DELETION`, no `ON_DELETE` — and its `if (!deleted)` guard at `:325` is +unreachable because every adapter returns a truthy `{ acknowledged, deletedCount }`. Whether +the purge belongs on that path is a decision, not an oversight: it is the developer-facing +primitive, and the sweeper needs an unpurged raw delete. The recommendation is to leave it raw +and document it as such, while exposing the purge as its own command. + +One caller must be left alone: `create-integration.js:70-78` deletes the duplicate row it +just created when it loses a concurrent-create race. That row never had entities set up and +owes no teardown — and the surviving integration holds the same entities — so purging there is +wasted work at best. Excluding it is deliberate, noted so a later reader does not "fix" it. + ### Two findings that change the design **`Sync` rows are never linked to their integration.** The schema declares @@ -159,9 +193,16 @@ Three records are shared by design, and deleting them unconditionally is data lo (`:33`) and never used again. Every sync row the framework writes therefore has `integrationId = null`, which means the cascade matches nothing *and* a purge keyed on `integrationId` alone would delete zero rows. Those rows are only reachable through their -entity ids. The fix is two-part: make `SyncManager` persist `integrationId`, and have the -purge match on `integrationId` **or** on the integration's entity ids so pre-existing rows -are covered too. +entity ids, so the purge must match on `integrationId` **or** on the integration's entity ids. + +The write path looks broken beyond that missing field, which is why fixing it is *not* a +prerequisite here. `upsertSync` passes `syncData` with `entities` as a bare id array into a +Prisma *relation* field (`sync-repository-mongo.js:87-89`), and `_convertFilterToWhere` +spreads the caller's filter straight into `where` (`:225-236`) while `SyncManager` builds that +filter from Mongo operators — `$elemMatch`, `$all` (`manager.js:338-347`). Prisma accepts +neither shape, so there are probably no framework-written `Sync` rows on either Prisma backend +at all. Repairing `SyncManager` is a separate defect with its own test surface; this change +ships the correct purge rules and leaves them accurate whether or not rows exist. **Deleting an encrypted row can fail after the row is gone.** The Prisma encryption extension's `delete` hook runs the query and *then* decrypts the returned record @@ -393,7 +434,10 @@ have no other referent. local destruction stays the invariant. - Purge failure: log with the integration id and the step that failed, append an `errors` message via `updateIntegrationMessages`, leave the row `IN_DELETION`, rethrow. The client - gets a 500; a retried `DELETE` resumes and converges. + gets a 500; a retried `DELETE` resumes and converges. Both halves of that sentence currently + need a fix to be true: `updateIntegrationMessages` clobbers rather than appends, and a retry + against an already-deleted integration 500s instead of returning 404 (see Context). Those are + steps 2 and 3. - Skips are logged at `warn` with the reason (`shared`, `global`, `foreign-user`, `already-gone`) so "why is this entity still here" is answerable from logs. @@ -474,21 +518,29 @@ framework can clean up arbitrary job names. Each step is independently reviewable and independently verifiable. 1. **This ADR** + a row in `docs/architecture-decisions/README.md`. -2. **Fix `SyncManager` to persist `integrationId`** (`syncs/manager.js:330-337`). Without this - the sync arm of the purge is decorative, and the schema's own `onDelete: Cascade` on - `Sync.integrationId` has never matched a row. Backfilling existing rows is the sweeper's - job, not a migration's. -3. **Repository primitives.** `deleteSyncsForIntegration`, +2. **Make the delete path retryable.** Translate a not-found read into `Boom.notFound` inside + `DeleteIntegrationForUser` (`:35-42`), so the existing dead guard becomes live and a retried + `DELETE` returns 404 rather than 500. Do not change the repositories' throw — other callers + match on its message. +3. **Fix `updateIntegrationMessages` to append**, against the real `errors` / `warnings` / + `info` / `logs` columns rather than a `messages` column that does not exist. Without this + every purge failure erases the previous one's breadcrumb. Align the test double's + `deleteIntegrationById` return shape at the same time. +4. **Do not fix `SyncManager` here** — file it separately. The purge's sync rules are written + to be correct either way (entity-id arm included), and the write path needs its own repair + plus its own tests (see Context). Persisting `integrationId` is the smallest part of that + defect, and coupling the two would make this change unshippable until the larger one lands. +5. **Repository primitives.** `deleteSyncsForIntegration`, `processRepository.deleteByIntegrationId`, a bulk `deleteCredentialsByIds` that uses `deleteMany`, the new association repository, and `P2025`-tolerant `deleteIntegrationById` — interface first, then mongo / postgres / documentdb. -4. **`PurgeIntegrationData` use case** + unit tests against repository doubles. Test matrix: +6. **`PurgeIntegrationData` use case** + unit tests against repository doubles. Test matrix: sole-owner entity deleted; shared entity kept; credential with one entity deleted; credential with two entities kept; entity with a foreign `userId` skipped; entity with a null `userId` skipped; already-deleted entity skipped; `isGlobal` entity skipped (forward-compat, asserted against a stub field); sync matched by entity id when `integrationId` is null; re-running the whole purge is a no-op. -5. **Wire into `DeleteIntegrationForUser`** + extend +7. **Wire into `DeleteIntegrationForUser`** + extend `tests/use-cases/delete-integration-for-user.test.js`. The existing failure-path tests (`:132-232`, `:269-320`, including a `throw null` case) lock in current behaviour — do not regress them. The double `test-integration-repository.js` already implements @@ -497,24 +549,24 @@ Each step is independently reviewable and independently verifiable. than a bare boolean. Note its field is `entitiesIds`, matching the domain mapping. `DeleteIntegrationForUser` is constructed without `moduleFactory` throughout that suite, so it must keep tolerating its absence. -6. **Close the queue-worker window** — two small fixes, both regression guards rather than +8. **Close the queue-worker window** — two small fixes, both regression guards rather than niceties (see §4): make the `processId` path discard a missing process (`handlers/backend-utils.js:143-153`) the way the `integrationId` path already discards a missing integration, and check integration status *before* hydrating modules so a purge in progress cannot throw a message into the DLQ (`integrations/use-cases/get-integration-instance.js:57-63`). -7. **Router DI** in `createIntegrationRouter()`. -8. **App-definition `deletion` block** in `app-definition-loader.js` + defaults + a loader test. -9. **Command-layer reads** — expose `findIntegrationsByEntityId`, and let entity lookup filter +9. **Router DI** in `createIntegrationRouter()`. +10. **App-definition `deletion` block** in `app-definition-loader.js` + defaults + a loader test. +11. **Command-layer reads** — expose `findIntegrationsByEntityId`, and let entity lookup filter on `credentialId` — then the **orphan sweeper admin script**, dry-run by default. -10. **Docs**: `packages/core/CLAUDE.md`, `docs/DANGER_ZONES.md`, the `onDelete` contract in +12. **Docs**: `packages/core/CLAUDE.md`, `docs/DANGER_ZONES.md`, the `onDelete` contract in `integration-base.js`, and the `delete` operation in `docs/frigg-management-api.yml:312-322` (which documents only the 204 today). Also `packages/core/integrations/EXTENSIONS.md` and `WEBHOOK-QUICKSTART.md`: both document registering provider-side webhooks and contain no mention of deletion, teardown or `onDelete` anywhere — an author following either is never told they own the removal. `EXTENSIONS.md:175` ("Contract enforced by the framework") is the natural home for the teardown half. -11. **Optional**: scheduler `listSchedules` + prefix sweep (§8). +13. **Optional**: scheduler `listSchedules` + prefix sweep (§8). No schema migration is required — the change is pure application code. That is deliberate: it keeps the change deployable to existing installations with no coordination, and it is a @@ -569,9 +621,10 @@ backend with no cascade at all is also the one that cannot be integration-tested the ORM's emulation rules plus which adapter you happen to be running. - Shared entities (ADR-024) get a deletion story before the feature ships, rather than after the first data-loss report. -- Two latent bugs get fixed on the way: `Sync.integrationId` starts being written, so the - cascade the schema has always declared finally matches something; and - `Module.deauthorize()` stops leaving dangling `credentialId` pointers. +- Four latent bugs get fixed on the way: a retried `DELETE` returns 404 instead of 500; + deletion error messages stop erasing each other; `Module.deauthorize()` stops leaving + dangling `credentialId` pointers; and queue workers stop DLQ-ing on a missing process. Each + is a defect on its own merits — the purge is what surfaced them. ### Negative @@ -640,9 +693,15 @@ splits the durable state across a queue and hides failures. The synchronous purg spells out the manual cascade order but does not perform it, no framework flow calls it, and ADR-007 sketches a `DELETE /users/:id` route (`:229`) that was never implemented. An erasure use case would give that command the cascade its own documentation asks for. -4. **Deletion audit log.** Worth recording purge summaries durably (an `AdminScriptExecution` +4. **`UsageCounter` under a lawful erasure request.** The policy here is "never delete", per + the schema's stated intent, and that is right for integration deletion. But no purge or + prune path exists at all, so an adopter served an erasure request has no lawful way to + remove usage rows. Should the erasure use case (question 3) be allowed to purge them — + making the policy "configurable, default never" — or is aggregation enough to consider them + non-personal? +5. **Deletion audit log.** Worth recording purge summaries durably (an `AdminScriptExecution` -shaped row, or telemetry only)? -5. **Adjacent papercut, in or out?** An ownership mismatch throws a plain `Error` +6. **Adjacent papercut, in or out?** An ownership mismatch throws a plain `Error` (`delete-integration-for-user.js:51-53`), which the error middleware boomifies to a 500 `{"error":"Internal Server Error"}` (`handlers/app-handler-helpers.js:26-37`) instead of a 403/404. One-line fix in the same file this change already touches, but it is a From 7fb2f856fea3b42c714c69078bd1b773ae01ebbc Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 20 Aug 2026 23:09:54 +0000 Subject: [PATCH 5/7] docs(adr): rewrite ADR-032 to match the house pattern MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Claude-Session: https://claude.ai/code/session_01M5jrmtqxucVoXrxvw7ph8h --- .../032-integration-deletion-cleanup.md | 838 ++++-------------- 1 file changed, 152 insertions(+), 686 deletions(-) diff --git a/docs/architecture-decisions/032-integration-deletion-cleanup.md b/docs/architecture-decisions/032-integration-deletion-cleanup.md index 9732eddac..87e71d14c 100644 --- a/docs/architecture-decisions/032-integration-deletion-cleanup.md +++ b/docs/architecture-decisions/032-integration-deletion-cleanup.md @@ -6,712 +6,178 @@ ## Context -Deleting an integration removes the `Integration` row and — on two of the three database -backends — whatever the ORM cascades from it. Everything the integration needed in order to -exist (its `Entity` rows, their `Credential` rows) is left in the database forever, and on -DocumentDB even the integration's own child rows survive. Adopters end up with a database -that accumulates dead credentials holding live OAuth refresh tokens. - -### What deletion does today - -`DELETE /api/integrations/:integrationId` (`integration-router.js:315-324`) calls -`DeleteIntegrationForUser.execute(integrationId, userId)` -(`integrations/use-cases/delete-integration-for-user.js:35-116`), which: - -1. Loads the integration and rejects it if `integrationRecord.userId !== userId` (`:50-54`). -2. Best-effort loads a module instance per entity id so provider-side teardown has API - clients; failures are logged and counted, never fatal (`:56-80`). -3. Instantiates the integration class, `initialize()`s it, and sets status `IN_DELETION` - (`:82-96`). -4. Sends `ON_DELETE` (`:98`). The base handler is a no-op — `async onDelete(params) {}` - (`integration-base.js:485`) — so provider-side cleanup (webhooks, subscriptions, - scheduled EventBridge jobs) happens only if the concrete integration overrides it. If it - throws, an error message is appended and the status stays `IN_DELETION` (`:99-113`). -5. Calls `integrationRepository.deleteIntegrationById(integrationId)` (`:115`). - -Step 5 is the only database write, and what it actually removes differs per backend: - -| Backend | Implementation | Cascade behaviour | +Deleting an integration removes one row: the `Integration` record. Everything that integration +needed in order to work stays in the database — its `Entity` records, and the `Credential` +records holding the OAuth access and refresh tokens. Those tokens are still valid. Nobody owns +them any more, and nothing will ever clean them up. + +How much of the integration's *own* data goes away depends on which database you run: + +| Backend | What happens | Result | |---|---|---| -| PostgreSQL | `prisma.integration.delete` (`integration-repository-postgres.js:178-186`) | Real FK cascades: `IntegrationMapping`, `Sync` (→ `DataIdentifier`), `Association` (→ `AssociationObject`), `Process`, plus the implicit `_EntityToIntegration` join rows (`prisma-postgresql/schema.prisma:196,218,239,261,278,308`) | -| MongoDB | `prisma.integration.delete` (`integration-repository-mongo.js:135-142`) | The same `onDelete: Cascade` declarations exist (`prisma-mongodb/schema.prisma:205,233,270,316`) but there are no DB-level FKs — the actions are **emulated by Prisma Client**, and the repo's own guidance is not to rely on them (see below) | -| DocumentDB | `deleteOne(...)` → `prisma.$runCommandRaw({delete: 'Integration', ...})` (`integration-repository-documentdb.js:52-58`, `database/documentdb-utils.js:91-101`) | **None.** The raw wire-protocol command bypasses the Prisma query engine entirely, so no emulated cascade fires | - -`deleteMappingsByIntegration` exists on all three backends -(`integration-mapping-repository-mongo.js:125`, `-postgres.js:180`, `-documentdb.js:193`) -but has **no caller anywhere in the repo** — mapping cleanup today is purely a side effect -of the ORM cascade. - -The codebase already records that the MongoDB half of that table is not to be trusted. -`user-repository-mongo.js:275-284`: - -> Prisma's `onDelete: Cascade` does NOT work reliably with MongoDB (no database-level -> referential integrity). Integration developers MUST manually cascade delete related -> records before calling this method: 1. Delete integrations … 2. Delete entities … -> 3. Delete credentials … 4. Finally delete user - -So the manual-cascade requirement, and even its ordering, is already written down for the -user path. This ADR applies the same rule to the integration path. - -There is also no re-entrancy guard: the record's existing status is never inspected, so a -retried or concurrent `DELETE` re-runs `persistStatus` and the full `ON_DELETE` teardown. -`onDelete` implementations therefore have to be idempotent already — the change below does -not introduce that requirement, but it does make it worth documenting. - -### What is left behind - -Left behind on **every** backend: - -- `Entity` rows for every entity the integration used. -- `Credential` rows behind those entities — including encrypted `access_token` / - `refresh_token` / `domain` fields (`database/encryption/encryption-schema-registry.js`). - A revoked-but-undeleted credential is the worst of the set: live secrets at rest with no - owner. -- The `User` row and its `Token` rows. - -Left behind on **DocumentDB always** (no cascade of any kind fires), and on **MongoDB -whenever the emulated cascade does not**: `IntegrationMapping`, `Process`, `Sync`, -`DataIdentifier`, `Association`, `AssociationObject`. PostgreSQL is the only backend where -these are reliably removed, because it is the only one with real foreign keys. - -Dangling references: - -- MongoDB `Entity.integrationIds` is a scalar ObjectId list - (`prisma-mongodb/schema.prisma:135-136`). `createIntegration` writes only the - `Integration.entityIds` side (`integration-repository-mongo.js:315-328`), so the entity - side is already never populated. Every `Integration ↔ Entity` read in the codebase goes - through the integration side (`entityIds: { has: entityId }`, - `integration-repository-mongo.js:283-291`), which is why nothing has broken. Cleanup code - must keep using the integration side and must not start trusting `Entity.integrationIds`. - `Entity.syncIds` / `Sync.entityIds` (`:138-139`, `:273-274`) are a second pair with the - same property. Prisma forbids referential actions on implicit many-to-many relations and - does not strip a deleted document's id from the surviving side's scalar list, so any id - written into these arrays would dangle permanently. -- PostgreSQL join rows are removed by the implicit m2m cascade on both sides of - `_EntityToIntegration` and `_EntityToSync` - (`prisma-postgresql/migrations/20250930193005_init/migration.sql:306-315`), so no dangling - refs are possible there. -- MongoDB `Process` has no parent/child *relation* — just `childProcesses String[]` / - `parentProcessId String?` (`prisma-mongodb/schema.prisma:247-248`) — whereas PostgreSQL - declares a self-relation with `onDelete: SetNull` - (`prisma-postgresql/schema.prisma:322-324`). Process-hierarchy pointers therefore go stale - on MongoDB and are nulled on PostgreSQL. -- DocumentDB writes `IntegrationMapping.integrationId` as a plain string - (`integration-mapping-repository-documentdb.js:122,282-289`) while the shared mongodb - schema declares it `@db.ObjectId`. Purge filters must match per backend: ObjectId for - MongoDB/Prisma, raw string for DocumentDB. - -Deliberately retained, and must stay retained: - -- `UsageCounter` — "deliberately isolated: no userId and no foreign key to Integration, so - … usage history survives integration deletion" - (`prisma-postgresql/schema.prisma:416-418`, `prisma-mongodb/schema.prisma:434-436`). -- `WebsocketConnection` and `State` — neither model has a user, entity or integration - reference at all (`prisma-postgresql/schema.prisma:343-353`). -- `ScriptSchedule` / `AdminScriptExecution` — app-level admin machinery (ADR-005), never - per-integration. - -Outside the database: - -- EventBridge one-time schedules created via `createSchedulerCommands()` - (`application/commands/scheduler-commands.js:104,188`). Job ids are integration-defined - strings (`'zoho-notif-renewal-abc123'`, `:14`) and the scheduler port exposes only - `scheduleOneTime` / `deleteSchedule` / `getScheduleStatus` - (`infrastructure/scheduler/scheduler-service-interface.js:22,32,42`) — there is no list - operation, so the framework cannot enumerate an integration's schedules. -- In-flight SQS messages naming the deleted integration. -- Provider-side webhooks and subscriptions. - -### Why "delete everything the integration touches" is the wrong predicate - -Three records are shared by design, and deleting them unconditionally is data loss: - -1. **`Entity` is shared across integrations.** `findOrCreateEntity` matches on - `{ externalId, user, moduleName }` and returns the existing row - (`modules/use-cases/process-authorization-callback.js:202-232`); it only creates when no - match exists (`:233-238`). ADR-024 states the goal explicitly — "One Nagaris entity - should serve both Nagaris CRM integration AND Nagaris Analytics integration" - (`docs/MULTI_STEP_AUTH_AND_SHARED_ENTITIES_SPEC.md:11,37-40`). The schema models it as - many-to-many on all backends. -2. **`Credential` is shared across entities.** `Credential.entities` is one-to-many - (`prisma-postgresql/schema.prisma:109`), `upsertCredential` matches an existing - credential by `externalId`, and re-auth explicitly repoints an entity at a different - credential (`process-authorization-callback.js:212-229`). PostgreSQL even declares - `Entity.credential` as `onDelete: SetNull` (`:119`), i.e. the schema's own answer to - "credential deleted" is "orphan the entity", not "delete it". -3. **`User` is per app-user, not per integration.** Users are looked up by `appUserId` / - `appOrgId` on every authenticated request - (`user/use-cases/authenticate-user.js:51-93`, - `get-user-from-x-frigg-headers.js`, `get-user-from-adopter-jwt.js:76-113`), and - `User.integrations` is a list (`prisma-postgresql/schema.prisma:56`). Deleting the user - when one of their integrations is deleted would cascade away every other integration - that user owns (`onDelete: Cascade` on `Integration.user`, `:152`), their credentials - (`:97`), their entities (`:121`) and their sessions (`:82`). It would also break the - next authenticated request for a user the adopter's app still considers active. - -### Three defects on this path the plan must fix, not inherit - -**A retried `DELETE` returns 500, not 404.** `findIntegrationById` *throws* on all three -backends — `integration-repository-mongo.js:195`, `-postgres.js:239`, -`-documentdb.js:71,75` — so the `Boom.notFound` guard at -`delete-integration-for-user.js:38-42` is unreachable dead code. Retrying a delete against an -already-deleted integration produces a plain `Error`, which the middleware boomifies to a 500. -Any plan that names retry as its recovery path has to make that read tolerant first. Do it in -the use case rather than by changing the repositories: callers elsewhere depend on the throw -and match on its message (`handlers/backend-utils.js:99-103`). The existing test double -returns `null`, which is exactly why this has stayed invisible. - -**The failure breadcrumb overwrites itself.** `updateIntegrationMessages` reads -`integration.messages` (`integration-repository-mongo.js:254`, `-postgres.js:300`, -`-documentdb.js:110`), but neither schema has a `messages` column — there are four separate -Json arrays, `errors` / `warnings` / `info` / `logs` -(`prisma-postgresql/schema.prisma:164-168`). So the array read is always `[]`, and the write -`data: { [messageType]: messageArray }` replaces the column with a single element. Every -deletion error message destroys the previous one, and any pre-existing errors on the -integration. The purge's error reporting depends on this appending, so it needs fixing here. - -**There is a second delete path that skips the lifecycle entirely.** -`application/commands/integration-commands.js:315-339` calls `deleteIntegrationById` raw — no -ownership check, no `IN_DELETION`, no `ON_DELETE` — and its `if (!deleted)` guard at `:325` is -unreachable because every adapter returns a truthy `{ acknowledged, deletedCount }`. Whether -the purge belongs on that path is a decision, not an oversight: it is the developer-facing -primitive, and the sweeper needs an unpurged raw delete. The recommendation is to leave it raw -and document it as such, while exposing the purge as its own command. - -One caller must be left alone: `create-integration.js:70-78` deletes the duplicate row it -just created when it loses a concurrent-create race. That row never had entities set up and -owes no teardown — and the surviving integration holds the same entities — so purging there is -wasted work at best. Excluding it is deliberate, noted so a later reader does not "fix" it. - -### Two findings that change the design - -**`Sync` rows are never linked to their integration.** The schema declares -`Sync.integrationId` as optional with `onDelete: Cascade` -(`prisma-postgresql/schema.prisma:216-218`), but the only framework writer never sets it: -`SyncManager.createSyncDBObject` builds `{ name, entities, hash, dataIdentifiers }` -(`syncs/manager.js:330-337`) and the integration is captured into `this.integration` -(`:33`) and never used again. Every sync row the framework writes therefore has -`integrationId = null`, which means the cascade matches nothing *and* a purge keyed on -`integrationId` alone would delete zero rows. Those rows are only reachable through their -entity ids, so the purge must match on `integrationId` **or** on the integration's entity ids. - -The write path looks broken beyond that missing field, which is why fixing it is *not* a -prerequisite here. `upsertSync` passes `syncData` with `entities` as a bare id array into a -Prisma *relation* field (`sync-repository-mongo.js:87-89`), and `_convertFilterToWhere` -spreads the caller's filter straight into `where` (`:225-236`) while `SyncManager` builds that -filter from Mongo operators — `$elemMatch`, `$all` (`manager.js:338-347`). Prisma accepts -neither shape, so there are probably no framework-written `Sync` rows on either Prisma backend -at all. Repairing `SyncManager` is a separate defect with its own test surface; this change -ships the correct purge rules and leaves them accurate whether or not rows exist. - -**Deleting an encrypted row can fail after the row is gone.** The Prisma encryption -extension's `delete` hook runs the query and *then* decrypts the returned record -(`database/encryption/prisma-encryption-extension.js:171-182`), and `decryptFields` → -`Cryptor.decrypt` → KMS has no error handling -(`database/encryption/field-encryption-service.js:101`, `encrypt/Cryptor.js:92-98`). So a -KMS outage or a rotated-away key throws *after* the row has been removed: the caller sees a -500 and may retry, but the data is already gone. The affected models are exactly the ones -this purge touches — `Credential` and `IntegrationMapping` -(`database/encryption/encryption-schema-registry.js:16-41`). `deleteMany` is a pure -pass-through with no crypto (`prisma-encryption-extension.js:184-186`), so purge paths -should prefer it. - -### The same missing predicate, elsewhere - -Integration deletion is where the gap is most visible, but nothing in the codebase -reference-counts these records today. A repo-wide search for reference counting or orphan -cleanup of data records finds one hit, and it is a log line rather than logic: -`[Frigg] findIntegrationByEntityExternalId: entity ${entity.id} has no owning integrations (orphan)` -(`integrations/use-cases/find-integration-by-entity-external-id.js:57`). Orphaned entities -are already a condition the framework recognises at runtime and does nothing about. - -Two other paths delete these records with no reference check at all: - -- `Module.deauthorize()` (`modules/module.js:206-227`) deletes the `Credential` row - unconditionally and then unsets `credentialId` on **only the entity it happens to hold in - memory**. Any sibling entity sharing that credential keeps a dangling pointer. Same - predicate, same fix — worth doing with the same helper rather than separately. -- The developer command surface exposes the raw primitives with only a truthy-id check: - `createEntityCommands().deleteEntity` / `deleteEntityById` / `unsetCredential` - (`application/commands/entity-commands.js:273,294,315`) and - `createCredentialCommands().deleteCredential` / `deleteCredentialById` - (`credential-commands.js:203,241`). Those stay as-is — they are the primitives — but the - safe, reference-counted operation should be reachable from the same surface so integration - authors are not obliged to reimplement the predicate. - -Global entities (ADR-024) sharpen the same point: an app-owner-owned entity is intended to -back many users' integrations. The `isGlobal` field does not exist in the schema or the -code yet (ADR-024 is Proposed and notes the feature is non-functional for exactly that -reason), so it cannot be checked today — but the predicate must be written so that adding -the check later is a one-line change. +| PostgreSQL | Real foreign keys with `ON DELETE CASCADE` | Mappings, syncs, associations and processes are removed | +| MongoDB | Same `onDelete: Cascade` in the schema, but no real foreign keys — Prisma emulates it | Usually removed, and the repo already warns against relying on it (`user-repository-mongo.js`) | +| DocumentDB | The adapter deletes through `$runCommandRaw`, which bypasses Prisma's query engine | Nothing cascades. Every child row is orphaned too | + +So the schema says one thing and the behaviour is three different things. DocumentDB is the +worst affected and the least obvious, because the schema *does* say `Cascade`. + +### Why we can't just delete everything the integration points at + +Three records are shared on purpose, and deleting them blindly would destroy live data: + +- **An `Entity` can belong to more than one integration.** Entities are found-or-created per + user, module and external id (`process-authorization-callback.js`), so two integrations that + use the same HubSpot account share one entity row. +- **A `Credential` can back more than one entity.** The schema even says what should happen + when a credential goes away: `Entity.credential` is `onDelete: SetNull`, meaning "orphan the + entity", not "delete it". +- **A `User` is one app user, not one integration.** Users are looked up by `appUserId` on every + authenticated request, and one user owns many integrations. Deleting the user would sign them + out of everything. + +So the rule cannot be "delete what it touches". It has to be "delete what it owns, plus what +nothing else needs". + +### Four existing bugs on this path + +Found while working this out. Each is a real bug on its own, and this change depends on all +four being fixed: + +1. **A retried delete returns 500, not 404.** `findIntegrationById` throws on all three + backends, so the `Boom.notFound` check in the delete use case is unreachable. That matters + because retrying is meant to be the recovery path. +2. **Deletion error messages erase each other.** `updateIntegrationMessages` reads a `messages` + column that does not exist — there are four separate arrays (`errors`, `warnings`, `info`, + `logs`) — so it always reads empty and writes a single-element array over the top. +3. **Queue workers will start dead-lettering.** Workers discard messages for a deleted or + `IN_DELETION` integration, but only after hydrating it, and hydration throws if an entity is + missing. The `processId` path throws outright. Once we delete processes and entities, messages + that used to be discarded cleanly will fail instead. +4. **Sync rows are never linked to their integration.** `SyncManager` never writes + `Sync.integrationId`, so the cascade the schema declares has never matched a row. ## Decision -Add an explicit, backend-uniform purge that runs as part of integration deletion. Nothing -relies on ORM cascades any more; the same code path produces the same end state on -MongoDB, PostgreSQL and DocumentDB. Records the integration owns outright are deleted -unconditionally. Records that can be shared are deleted only when the integration being -deleted was their last referent. The `User` row is never touched. - -### 1. Deletion policy - -| Record | Policy | Predicate | Rationale | -|---|---|---|---| -| `Integration` | always delete | — | The thing being deleted. Deleted **last**, so `IN_DELETION` remains a durable checkpoint for the whole purge | -| `IntegrationMapping` | always delete | `integrationId = X` | Owned outright. Explicit call, not cascade — DocumentDB has none | -| `Sync` | always delete | `integrationId = X` **or** `entityIds ∩ integration.entityIds` | Owned outright, but `integrationId` is never written today (see Context), so the entity-id arm is what actually matches existing rows | -| `DataIdentifier` | always delete | `syncId ∈ deleted syncs` | Cascades from `Sync` on MongoDB/PostgreSQL. On **DocumentDB** there is nothing to delete — identifiers are an embedded array inside the Sync document (`sync-repository-documentdb.js:100-121,201-227`), not a collection, so deleting the sync removes them. The two Mongo-family backends genuinely diverge in storage shape here | -| `Association` | always delete | `integrationId = X` | Owned outright. Note nothing in the codebase reads or writes these models today (`associations/association.js` is an in-memory hashing helper with no repository), so this is future-proofing, not a live orphan source | -| `AssociationObject` | always delete | `associationId ∈ deleted associations` | Cascades on MongoDB/PostgreSQL; explicit on DocumentDB | -| `Process` | always delete | `integrationId = X` | Owned outright. A whole-integration delete makes the hierarchy moot, so children go with it rather than being re-parented | -| `Entity` | delete if orphaned | `findIntegrationsByEntityId(e).filter(i => i.id !== X).length === 0` **and** `entity.isGlobal !== true` **and** `String(entity.userId) === String(integration.userId)` | Shared by design (see Context). The ownership check is strict on purpose: `Entity.userId` is nullable (`prisma-postgresql/schema.prisma:120`) and a null owner is the shape ADR-024 gives a global entity, so a null `userId` **skips**, it does not match | -| `Credential` | delete if orphaned | after entity deletion, `findEntities({ credentialId }).length === 0` | One credential can back several entities | -| `User` | **never** | — | Per app-user, reused across integrations, cascades to everything (see Context). Account erasure is a separate operation | -| `Token` | never | — | Session-scoped, user-scoped. Cascades only if a user is deleted | -| `UsageCounter` | never | — | Explicitly designed to survive integration deletion | -| `WebsocketConnection`, `State` | never | — | No integration, entity, credential or user reference exists | -| `ScriptSchedule`, `AdminScriptExecution` | never | — | App-level admin machinery, not per-integration | -| EventBridge schedules | integration's `onDelete` | — | Job ids are integration-defined and the scheduler port has no list operation. Optional follow-up in step 8 | -| In-flight SQS messages | never | — | Cannot be selectively purged. Handlers must tolerate a missing integration (see §4) | -| S3 report artifacts (ADR-010) | out of scope | — | Keys are execution-scoped (`reports/{executionId}/…`), not integration-scoped, so per-integration purge is impossible by key. Separately: the artifact port has no `delete` at all (`artifacts/repositories/artifact-repository-interface.js:6-25`) and the bucket has no lifecycle policy — worth its own issue. ADR-022 "Artifacts" is a different concept (provider-side deployables) | - -### 2. Order of operations - -`PurgeIntegrationData.execute({ integrationId, userId, entityIds })`: - -1. `integrationMappingRepository.deleteMappingsByIntegration(integrationId)` -2. `processRepository.deleteByIntegrationId(integrationId)` -3. `syncRepository.deleteSyncsForIntegration({ integrationId, entityIds })` — matching either - arm, because `integrationId` is null on every sync the framework has written so far -4. `associationRepository.deleteByIntegrationId(integrationId)` -5. Orphan sweep, per entity id captured from the integration record **before** anything was - deleted: - - `others = findIntegrationsByEntityId(entityId).filter(i => String(i.id) !== String(integrationId))` - - skip if `others.length > 0` (shared), if the entity is gone, if `entity.isGlobal === true`, - or if `String(entity.userId) !== String(integration.userId)` — which includes a null - owner, per the policy table. - Note `findEntityById` **throws** `Entity not found` on all three backends - (`module-repository-mongo.js:92-94`, `-postgres.js:126-128`, `-documentdb.js:35-41`) - rather than returning null, so the "already gone" branch must catch, or the sweep must - read through `findEntities({ id })` instead. Getting this wrong is what would make a - retried purge fail instead of converging. `deleteEntity` and `deleteCredentialById` - already swallow `P2025` and report "nothing deleted" - (`module-repository-mongo.js:385-398`, `credential-repository-mongo.js:74-86`), so the - write side of a retry is already safe — it is only the read that throws - - remember `credentialId = entity.credential?.id`, then `moduleRepository.deleteEntity(entityId)` - - if `credentialId` and `findEntities({ credentialId }).length === 0` → - `credentialRepository.deleteCredentialById(credentialId)` -6. `integrationRepository.deleteIntegrationById(integrationId)` - -Two properties fall out of that ordering, and both are the reason for it: - -- **One durable checkpoint.** The `Integration` row — and therefore its `IN_DELETION` - status and its `errors` messages — survives until the very last step. A crash anywhere in - 1–5 leaves a record an operator (or a retried `DELETE`) can still see and act on. That is - why the reference-count predicate excludes the integration explicitly instead of relying - on the row already being gone. -- **Idempotence, so retry is the recovery story.** Every step is "delete rows matching this - id" or "delete this row if nothing else points at it". Re-running the purge after a - partial failure converges to the same end state. Children are deleted before parents, so - no step can violate a PostgreSQL FK. - -Deleting entities while the `Integration` row still exists is safe: on PostgreSQL the -implicit `_EntityToIntegration` join rows cascade from the entity, and -`DataIdentifier`/`AssociationObject` rows referencing that entity cascade too -(`prisma-postgresql/schema.prisma:241,280`) — all of them already dead by that point. - -### 3. Transactions - -Do not require them. PostgreSQL can wrap steps 1–4 and 6 in `prisma.$transaction`, but -Prisma transactions on MongoDB need a replica set (not guaranteed in local Docker setups), -and the DocumentDB adapter issues one `$runCommandRaw` per operation with no transaction -helper in `documentdb-utils.js` at all. Rather than a capability that silently exists on one -backend, the ordering above is chosen so that a partial failure can only leave *extra* -rows — never a dangling reference — and the retry/sweeper path finishes the job. A -per-backend `runInTransaction(fn)` that PostgreSQL implements and the others no-op is a -reasonable later refinement, not a prerequisite. - -### 4. Code changes - -**New use case** — `packages/core/integrations/use-cases/purge-integration-data.js`, -exported from `use-cases/index.js`. Constructor-injected: -`integrationRepository`, `integrationMappingRepository`, `processRepository`, -`syncRepository`, `associationRepository`, `moduleRepository`, `credentialRepository`, -plus a `policy` object (§5). Returns a summary — counts deleted per model, and a list of -`{ entityId, reason }` skips — so the caller can log it and telemetry can count it. - -**`DeleteIntegrationForUser`** — inject `purgeIntegrationData` and replace the bare -`deleteIntegrationById` call (`:115`) with a purge call that receives the entity ids -snapshotted from the record at `:60`. Keep the existing `ON_DELETE` failure semantics -untouched: provider-side teardown must still succeed before any local data is destroyed. - -**Repository methods to add** (interface + all three adapters + tests): - -| Repository | Method | Notes | +Add a `PurgeIntegrationData` use case that runs inside the existing delete flow: after +`ON_DELETE` (so provider-side teardown still happens first), and before the `Integration` row is +removed (so the `IN_DELETION` status keeps queue work away while we work). + +The rules: + +| Record | What we do | When | |---|---|---| -| `sync-repository-*` | `deleteSyncsForIntegration({ integrationId, entityIds })` | The entity arm must be scoped to *these* entity ids — never "where `integrationId is null`", which would delete every parentless sync in the database. Mongo/Postgres: `prisma.sync.deleteMany`. MongoDB additionally needs its separate `DataIdentifier` rows removed; DocumentDB does not (embedded). Only `deleteSync(id)` exists today (`sync-repository-interface.js:104`) | -| `process-repository-*` | `deleteByIntegrationId(integrationId)` | Only `findByIntegrationAndType` / `findActiveProcesses` exist today (`process-repository-interface.js:102,112`); `deleteById` is single-row (`:130`) | -| `association-repository-*` | new repository | No association repository exists at all; `Association`/`AssociationObject` are referenced only by the schema-init test (`database/utils/mongodb-schema-init.test.js:9`). Add a minimal repository with `deleteByIntegrationId` so DocumentDB is covered and the model stops being invisible | -| `integration-repository-*` | make `deleteIntegrationById` idempotent | Mongo/Postgres throw Prisma `P2025` on a missing row and do not catch it. Catch it and return `deletedCount: 0` so a retried purge does not 500 — same pattern as `deleteUser` (`user-repository-mongo.js:294-301`) and the mapping repositories | - -**Prefer `deleteMany` for encrypted models.** `Credential` and `IntegrationMapping` carry -encrypted fields, and the extension's `delete` hook decrypts the deleted row afterwards, so a -KMS failure throws with the row already gone (see Context). `deleteMany({ where: { id } })` -skips the crypto path entirely. That argues for a `deleteCredentialsByIds` bulk method rather -than looping the existing `deleteCredentialById`, which uses `.delete()` -(`credential-repository-mongo.js:74-86`). - -**Shared orphan-check helper** — the entity and credential predicates should live in one -place the purge and `Module.deauthorize()` both call, so the deauthorize bug -(`modules/module.js:206-227`, credential deleted without checking sibling entities) is fixed -by construction rather than separately. Expose the reference-counted operation on the -developer command surface alongside the existing raw `deleteEntity` / `deleteCredential` -commands. - -Already present and reused unchanged: `findIntegrationsByEntityId` -(`integration-repository-mongo.js:283`, `-postgres.js:464`, `-documentdb.js:43`), -`deleteMappingsByIntegration`, `moduleRepository.findEntityById` / `findEntities` / -`deleteEntity` (`module-repository-mongo.js:385`, `-postgres.js:428`, `-documentdb.js:189`), -`credentialRepository.deleteCredentialById` (`credential-repository-mongo.js:74`, -`-postgres.js:88`, `-documentdb.js:68`). - -**DI wiring** — `createIntegrationRouter()` already builds `moduleRepository`, -`integrationRepository` and `credentialRepository` -(`integration-router.js:73-115`); add the mapping, process, sync and association factories -(`createIntegrationMappingRepository`, `createProcessRepository`, `createSyncRepository` all -exist) and pass the purge use case into `DeleteIntegrationForUser` at `:111`. - -**Queue/worker guard — a regression this change would otherwise introduce.** The guard -already exists on the `integrationId` path: `createQueueWorker` discards when hydration -returns null and when status is `DISABLED` / `ERROR` / `IN_DELETION` -(`handlers/backend-utils.js:202-229`, tested at `backend-utils.test.js:152-161`). The -`processId` path has no such tolerance — `loadIntegrationForProcess` throws -`Process not found: ${processId}` (`:143-153`). Today such a message hydrates fine and is -discarded on status; once the purge deletes `Process` rows, every in-flight process-keyed -message for a deleted integration starts throwing and lands in the DLQ. Per-integration -queues retain messages for 4 days and the shared error queue for 14 -(`devtools/infrastructure/domains/integration/integration-builder.js:462-469,557-574`), so -this is not theoretical. Fixing that one branch is part of this change, not an optional -follow-up. - -There is a second, sharper version of the same problem. Both paths hydrate **before** they -check status, and `GetIntegrationInstance` loops the entity ids calling -`moduleFactory.getModuleInstance` with no try/catch -(`integrations/use-cases/get-integration-instance.js:57-63`) — unlike `DeleteIntegrationForUser`, -which wraps each load. Since `findEntityById` throws when the entity is gone, a worker that -arrives during the purge window — `Integration` row still present as `IN_DELETION`, entities -already deleted — throws inside hydration and never reaches the `IN_DELETION` discard at -`backend-utils.js:220-229`. The fix is to check status before hydrating modules, or to make -that loop tolerant the way the delete path already is. This hazard exists today in a narrower -form (any manually-deleted entity does it); the purge widens the window, so closing it belongs -to this change. - -### 5. Configuration - -Cascade-deleting credentials is the one part of this that an adopter might reasonably want -off (an installation that treats credentials as long-lived connections, reused when the user -re-adds an integration). Add a `deletion` block to the app definition: - -```javascript -// backend/index.js -Definition = { - deletion: { - purgeOrphanedEntities: true, // default true - purgeOrphanedCredentials: true, // default true - }, -}; -``` - -`loadAppDefinition()` destructures a fixed key set and returns a fixed object -(`handlers/app-definition-loader.js:33-38,62`), so the key must be added there and defaulted -to `{}`. Steps 1–4 and 6 are not configurable — those rows are the integration's own and -have no other referent. - -### 6. Failure handling - -- `ON_DELETE` failure keeps today's behaviour exactly: error message appended, status stays - `IN_DELETION`, nothing purged, error rethrown (`:99-113`). Provider-side teardown before - local destruction stays the invariant. -- Purge failure: log with the integration id and the step that failed, append an `errors` - message via `updateIntegrationMessages`, leave the row `IN_DELETION`, rethrow. The client - gets a 500; a retried `DELETE` resumes and converges. Both halves of that sentence currently - need a fix to be true: `updateIntegrationMessages` clobbers rather than appends, and a retry - against an already-deleted integration 500s instead of returning 404 (see Context). Those are - steps 2 and 3. -- Skips are logged at `warn` with the reason (`shared`, `global`, `foreign-user`, - `already-gone`) so "why is this entity still here" is answerable from logs. - -#### Known limits, stated rather than hidden - -- **Check-then-act is not atomic.** Between the reference-count read and the delete, a - concurrent `createIntegration` could link that entity. There is no unique index or lock to - fall back on. The window is small and the operation requires the same user to be adding an - integration while deleting another, but it is real — and it argues for keeping the - `IN_DELETION` status guard (which stops queue work for this integration) rather than - deleting the row early. -- **Syncs are not consulted by the entity predicate.** An entity can be referenced by a - `Sync` even when no integration references it, and on PostgreSQL deleting the entity - cascades that sync's `DataIdentifier` rows away - (`prisma-postgresql/schema.prisma:241`). While `Sync.integrationId` is unwritten the syncs - are unattributable, so the honest position is: after step 2 makes syncs attributable, the - entity predicate should also require that no *surviving* integration's sync references the - entity. Until then, pre-existing parentless syncs are the sweeper's problem, not the - purge's. -- **Org-linked users are skipped, not resolved.** The predicate compares raw ids, while the - rest of the codebase resolves ownership through `User.ownsUserId`, which understands - individual↔organization membership (`modules/use-cases/get-module.js:32-35`, - `test-module-auth.js:37-38`). The purge only has ids, so an entity owned by the individual - while the integration is owned by the org is skipped. That is the safe direction — a skip - leaves an orphan, a false match destroys live data — but it means org-configured - deployments will accumulate some orphans that only the sweeper clears. -- **Deleting a credential forecloses revoking it.** Once the row is gone the refresh token is - gone with it, so provider-side revocation can never happen afterwards. If revocation is - added later it has to run *before* the purge — `ON_DELETE`, which already executes first - with modules loaded, is the natural place. -- **Guard against empty filters on DocumentDB.** `_buildFilter` can drop an unconvertible id - and return a match-all query. For the credential reference count that fails safe (it looks - like "still referenced", so nothing is deleted), but any purge filter must assert it is - non-empty before issuing a `deleteMany`. - -### 7. Orphan sweeper - -Existing deployments already hold orphans — every integration ever deleted, plus every -child row on DocumentDB. Ship an admin script (ADR-005 runner) `purge-orphaned-records` -that reports counts by default and deletes with `--apply`: - -- `Entity` rows no integration references, respecting the same skip predicates -- `Credential` rows no entity references -- `IntegrationMapping` / `Sync` / `DataIdentifier` / `Association` / `AssociationObject` / - `Process` rows whose parent id no longer resolves - -It doubles as the recovery path for a purge that failed after the `Integration` row was -already gone. - -One constraint makes this more than a scripting job: **admin scripts have no repository -access.** They reach the database only through the injected command bundle — -`{ users, credentials, entities, integrations }` -(`admin-scripts/src/application/admin-script-context.js:7-11`, -`src/infrastructure/bootstrap.js:91-96`) — and the two reads the orphan predicate needs are -not on it: - -- `findIntegrationsByEntityId` exists on the repository interface (`:182`) but is not exposed - as an integration command. -- `commands.entities.findEntity` rejects a filter that carries only `credentialId` — it - requires `externalId`, `userId` or `moduleName` (`entity-commands.js:92-104`) — so the - credential reference count is unreachable. - -Both writes are already reachable (`integration-commands.js:315`, -`entity-commands.js:273,294`, `credential-commands.js:203,241`). So the sweeper step includes -adding those two reads to the command layer — which is also what makes the reference-counted -operation available to integration authors. - -### 8. Optional follow-up: scheduled-job sweep - -Add `listSchedules({ namePrefix })` to `scheduler-service-interface.js` plus the EventBridge -and mock adapters (EventBridge Scheduler's `ListSchedules` supports `NamePrefix`), and a -`deleteJobsByPrefix(integrationId)` command. That only helps integrations that include the -integration id in their job ids, so document the convention rather than implying the -framework can clean up arbitrary job names. - -## Implementation plan - -Each step is independently reviewable and independently verifiable. - -1. **This ADR** + a row in `docs/architecture-decisions/README.md`. -2. **Make the delete path retryable.** Translate a not-found read into `Boom.notFound` inside - `DeleteIntegrationForUser` (`:35-42`), so the existing dead guard becomes live and a retried - `DELETE` returns 404 rather than 500. Do not change the repositories' throw — other callers - match on its message. -3. **Fix `updateIntegrationMessages` to append**, against the real `errors` / `warnings` / - `info` / `logs` columns rather than a `messages` column that does not exist. Without this - every purge failure erases the previous one's breadcrumb. Align the test double's - `deleteIntegrationById` return shape at the same time. -4. **Do not fix `SyncManager` here** — file it separately. The purge's sync rules are written - to be correct either way (entity-id arm included), and the write path needs its own repair - plus its own tests (see Context). Persisting `integrationId` is the smallest part of that - defect, and coupling the two would make this change unshippable until the larger one lands. -5. **Repository primitives.** `deleteSyncsForIntegration`, - `processRepository.deleteByIntegrationId`, a bulk `deleteCredentialsByIds` that uses - `deleteMany`, the new association repository, and `P2025`-tolerant - `deleteIntegrationById` — interface first, then mongo / postgres / documentdb. -6. **`PurgeIntegrationData` use case** + unit tests against repository doubles. Test matrix: - sole-owner entity deleted; shared entity kept; credential with one entity deleted; - credential with two entities kept; entity with a foreign `userId` skipped; entity with a - null `userId` skipped; already-deleted entity skipped; `isGlobal` entity skipped - (forward-compat, asserted against a stub field); sync matched by entity id when - `integrationId` is null; re-running the whole purge is a no-op. -7. **Wire into `DeleteIntegrationForUser`** + extend - `tests/use-cases/delete-integration-for-user.test.js`. The existing failure-path tests - (`:132-232`, `:269-320`, including a `throw null` case) lock in current behaviour — do not - regress them. The double `test-integration-repository.js` already implements - `findIntegrationsByEntityId`; what it needs is the bulk deletes, and its - `deleteIntegrationById` aligned to the real `{ acknowledged, deletedCount }` shape rather - than a bare boolean. Note its field is `entitiesIds`, matching the domain mapping. - `DeleteIntegrationForUser` is constructed without `moduleFactory` throughout that suite, so - it must keep tolerating its absence. -8. **Close the queue-worker window** — two small fixes, both regression guards rather than - niceties (see §4): make the `processId` path discard a missing process - (`handlers/backend-utils.js:143-153`) the way the `integrationId` path already discards a - missing integration, and check integration status *before* hydrating modules so a purge in - progress cannot throw a message into the DLQ - (`integrations/use-cases/get-integration-instance.js:57-63`). -9. **Router DI** in `createIntegrationRouter()`. -10. **App-definition `deletion` block** in `app-definition-loader.js` + defaults + a loader test. -11. **Command-layer reads** — expose `findIntegrationsByEntityId`, and let entity lookup filter - on `credentialId` — then the **orphan sweeper admin script**, dry-run by default. -12. **Docs**: `packages/core/CLAUDE.md`, `docs/DANGER_ZONES.md`, the `onDelete` contract in - `integration-base.js`, and the `delete` operation in `docs/frigg-management-api.yml:312-322` - (which documents only the 204 today). Also `packages/core/integrations/EXTENSIONS.md` and - `WEBHOOK-QUICKSTART.md`: both document registering provider-side webhooks and contain no - mention of deletion, teardown or `onDelete` anywhere — an author following either is never - told they own the removal. `EXTENSIONS.md:175` ("Contract enforced by the framework") is the - natural home for the teardown half. -13. **Optional**: scheduler `listSchedules` + prefix sweep (§8). - -No schema migration is required — the change is pure application code. That is deliberate: -it keeps the change deployable to existing installations with no coordination, and it is a -reason to prefer reference counting in the use case over adding schema-level machinery. - -## Testing - -The repo has no real-database test tier: there is no `testcontainers` dependency, no -`docker-compose` file, and no per-backend jest project. Repository tests are -*command-generation* tests — instantiate the adapter, replace `repo.prisma` with a hand-rolled -fake, and assert on the emitted command shape (exemplars: -`integration-repository-mongo.test.js:15-25`, -`integration-repository-documentdb.test.js:17-30`). Match that habit rather than inventing a -new tier. Use-case tests use the stateful fake in `integrations/tests/doubles/`, and every core -test file starts by mocking `database/config` so requiring a use case does not pull a real -Prisma client (copy `delete-integration-for-user.test.js:1-6`, adjusting the relative depth). - -```bash -npm install # from the repo root; needed before any run - -cd packages/core -npm test -- integrations/tests/use-cases/ # use-case suites -npm test -- integrations/repositories/ # integration, mapping, process adapters -npm test -- syncs credential modules # sync, credential, entity adapters -npm test # whole core package - -# from the root, what CI runs -npm run test:all -npm run lint:fix --workspaces -``` - -Note every core jest invocation boots an in-memory mongod via `globalSetup` -(`packages/core/jest.config.js:19-22`), so runs are slower than they look and need the -workspace installed. - -Real-database verification is therefore out-of-suite, with the canary harness against -PostgreSQL and MongoDB: create two integrations sharing one entity, delete the first, and -assert the shared entity and its credential survive while the sole-owner pair is gone. -DocumentDB has no local emulator — its adapters are covered by command-shape tests plus a -manual check against a real cluster before release. That gap is worth stating plainly: the -backend with no cascade at all is also the one that cannot be integration-tested locally. +| `IntegrationMapping`, `Process`, `Sync`, `DataIdentifier`, `Association`, `AssociationObject` | Always delete | They belong to this integration and nothing else | +| `Entity` | Delete only if orphaned | No other integration references it, and its `userId` matches the integration's | +| `Credential` | Delete only if orphaned | No entity is left pointing at it | +| `User`, `Token` | Never | One user owns many integrations | +| `UsageCounter` | Never | Deliberately kept so usage history survives deletion | +| `WebsocketConnection`, `State`, `ScriptSchedule`, `AdminScriptExecution` | Never | Not tied to an integration at all | +| EventBridge jobs, provider webhooks | The integration's `onDelete` | The framework cannot list them | +| In-flight SQS messages | Nothing | Cannot be purged selectively; workers discard them instead | + +Order matters, so the steps run children first and the integration row last: + +1. Delete mappings, processes, syncs and associations. +2. For each entity: count the *other* integrations using it. Skip it if any remain, if it is + marked global, or if its `userId` does not match. Otherwise delete it, and remember its + credential id. +3. For each remembered credential: delete it only if no entities are left pointing at it. +4. Delete the `Integration` row. + +Every step is explicit, in the use case, and identical on all three backends. We do not rely on +the ORM's cascade behaviour, because it differs per backend and one backend has none. Extra +deletes on PostgreSQL are harmless — the rows are already gone. + +Three details worth writing down: + +- **Ownership is strict.** `Entity.userId` is nullable, and a null owner is the shape a global + entity takes (ADR-024), so a null `userId` skips rather than matches. Skipping leaves an + orphan; a false match destroys live data. +- **Prefer `deleteMany` for encrypted rows.** The encryption extension's `delete` hook decrypts + the deleted record *after* the delete, with no error handling down to KMS, so a bad key throws + with the row already gone. `deleteMany` skips that path. +- **On MongoDB, count from the integration side.** `Entity.integrationIds` is never written, so + reference counting must query `Integration.entityIds`. + +Credential deletion is behind `deletion.purgeOrphanedCredentials` in the app definition, +defaulting to on. Entities and the integration's own children are not configurable. + +Existing deployments already hold orphans from every integration ever deleted. A +`purge-orphaned-records` admin script reports counts by default and deletes with `--apply`. It +needs two new command-layer reads first, because admin scripts talk to the database only through +commands, and neither "which integrations use this entity" nor "which entities use this +credential" is exposed today. + +No schema migration is required. This is application code, so it deploys to existing +installations with no coordination. ## Consequences ### Positive -- Deleting an integration stops leaving live OAuth refresh tokens at rest. -- DocumentDB stops orphaning the integration's own child rows — currently the worst-affected - backend and the least obvious, because the schema *says* `onDelete: Cascade`. -- One code path, one end state, for all three backends. Deletion behaviour becomes - something you can read in a use case instead of inferring from a schema annotation plus - the ORM's emulation rules plus which adapter you happen to be running. -- Shared entities (ADR-024) get a deletion story before the feature ships, rather than - after the first data-loss report. -- Four latent bugs get fixed on the way: a retried `DELETE` returns 404 instead of 500; - deletion error messages stop erasing each other; `Module.deauthorize()` stops leaving - dangling `credentialId` pointers; and queue workers stop DLQ-ing on a missing process. Each - is a defect on its own merits — the purge is what surfaced them. +- Deleting an integration stops leaving valid OAuth refresh tokens at rest. +- DocumentDB stops orphaning the integration's own child rows. +- Deletion behaviour becomes something you can read in one use case, instead of inferring it + from a schema annotation plus the ORM's emulation rules plus which adapter you happen to run. +- Shared entities (ADR-024) get a deletion story before the feature ships. +- Four latent bugs get fixed on the way. ### Negative -- More repository surface: three new bulk-delete methods across three backends, plus a - repository for a model that had none. -- Deletion is now a multi-step destructive operation without a transaction on two of three - backends. Mitigated by ordering and idempotence, but a partial failure does leave extra - rows until a retry or the sweeper runs. -- Deleting a credential does not revoke it at the provider. The token stops existing locally - and keeps existing upstream until it expires — worth stating plainly rather than implying - full teardown. -- Reference counting costs one query per entity plus one per credential. Negligible at - realistic entity counts, and only on the delete path. -- Deleting `Process` rows turns a currently-harmless in-flight SQS message into a DLQ entry - unless the `processId` hydration path is fixed in the same change. That fix is step 6, and - it is the one part of this plan that is a guard against the plan itself. -- The purge is exercised end to end only against PostgreSQL and MongoDB. DocumentDB — the - backend this change helps most — has no local emulator, so its adapters get command-shape - tests and a manual pre-release check. +- More repository surface: bulk deletes across three backends, plus a repository for a model + that has none today. +- Deletion is a multi-step destructive operation with no transaction on two of three backends. + A partial failure leaves extra rows until a retry or the sweeper runs. +- Deleting a credential does not revoke it upstream. The token stops existing locally and keeps + existing at the provider until it expires. And once the row is gone we can never revoke it, so + if revocation is added later it has to run before the purge. +- Checking then deleting is not atomic. A concurrent create could link an entity in between. + The window is small and there is no lock to fall back on. +- The purge is only tested end to end against PostgreSQL and MongoDB. DocumentDB has no local + emulator, so it gets unit tests and a manual check — and it is the backend this helps most. ### Neutral - `User`, `Token` and `UsageCounter` behaviour is unchanged. -- Existing orphans are untouched until the sweeper is run deliberately. -- `Association` / `AssociationObject` gain a repository with nothing to clean yet — nothing - in the codebase writes those models today. It is there so DocumentDB is not the odd one out - if an adopter starts. +- Existing orphans stay until someone runs the sweeper. +- Org-linked users are skipped rather than resolved. The purge compares ids, while the rest of + the codebase resolves ownership through `User.ownsUserId`, so some org setups will accumulate + orphans that only the sweeper clears. ## Alternatives Considered -**Lean on schema referential actions.** Making the ORM do the work fails on both ends: it -cannot express "delete only if nothing else references this" (which is the entire -shared-entity problem), and it does not run at all on DocumentDB, where the adapter bypasses -the query engine. It also would not fire for the `Entity → Credential` direction, which -PostgreSQL deliberately declares `SetNull`. - -**Soft delete (`deletedAt` everywhere).** Reversible and audit-friendly, but it does not -solve the stated problem — the secrets stay in the database — and every read path in the -codebase would need a `deletedAt: null` filter, which is a large, easy-to-miss change. If -audit history is wanted, the right shape is an append-only deletion log recording what was -purged, not tombstoned rows. - -**Delete the `User` too.** The maintainer's framing named the user record, so it is worth -being explicit: it is not safe. Users are keyed by the adopter's `appUserId`, reused across -integrations, and cascade to every integration, credential, entity and session they own. -"Erase this account and everything in it" is a legitimate operation — as a separate, -explicitly-invoked use case (a GDPR erasure path), not as a side effect of deleting one -integration. - -**Fire-and-forget async purge (queue a cleanup job).** Keeps the API response fast, but -splits the durable state across a queue and hides failures. The synchronous purge with -`IN_DELETION` as the checkpoint keeps one place to look. - -## Open questions for the maintainer - -1. **Credential purge default.** Recommend `purgeOrphanedCredentials: true`, since dead - credentials holding live refresh tokens are the main motivation. Adopters who treat a - credential as a reusable connection may want `false`. -2. **Provider-side revocation.** Should credential deletion attempt an OAuth revoke where - the module supports it? Not currently modelled in `requiredAuthMethods`; would need a new - optional method. -3. **Account erasure.** Should the GDPR-style "delete user and everything they own" use case - land in the same change, or as its own ADR? The pieces are already half-built: `deleteUser` - exists on all three backends (`user-repository-interface.js:195`), it is exposed to - developers as `deleteUserById` (`application/commands/user-commands.js:281`) whose docblock - spells out the manual cascade order but does not perform it, no framework flow calls it, - and ADR-007 sketches a `DELETE /users/:id` route (`:229`) that was never implemented. An - erasure use case would give that command the cascade its own documentation asks for. -4. **`UsageCounter` under a lawful erasure request.** The policy here is "never delete", per - the schema's stated intent, and that is right for integration deletion. But no purge or - prune path exists at all, so an adopter served an erasure request has no lawful way to - remove usage rows. Should the erasure use case (question 3) be allowed to purge them — - making the policy "configurable, default never" — or is aggregation enough to consider them - non-personal? -5. **Deletion audit log.** Worth recording purge summaries durably (an `AdminScriptExecution` - -shaped row, or telemetry only)? -6. **Adjacent papercut, in or out?** An ownership mismatch throws a plain `Error` - (`delete-integration-for-user.js:51-53`), which the error middleware boomifies to a 500 - `{"error":"Internal Server Error"}` (`handlers/app-handler-helpers.js:26-37`) instead of a - 403/404. One-line fix in the same file this change already touches, but it is a - behaviour change for API clients, so it should be an explicit yes/no rather than a - drive-by. +- **Fix the schema and let the database cascade.** Rejected: it cannot express "delete only if + no one else needs this", and it does nothing for DocumentDB, which never reaches the query + engine. +- **Delete the user too.** Rejected: users are per app user and own many integrations. This is a + separate account-erasure operation, not part of deleting one integration. +- **Soft delete (a `deletedAt` column).** Rejected: it does not solve the problem. The point is + to stop storing live credentials, and a soft-deleted credential is still a stored credential. +- **Leave it to each integration's `onDelete`.** Rejected: every integration would reimplement + reference counting, and the default handler is a no-op, so most would simply not do it. +- **Sweep on a schedule instead of on delete.** Rejected as the primary mechanism — credentials + would linger for a whole sweep interval. Kept as the backstop for existing orphans and failed + purges. + +## Open Questions + +1. **Provider-side revocation.** Should deleting a credential try to revoke it upstream where + the module supports it? Best-effort, or fail the delete? +2. **Account erasure.** Should there be a separate "delete this user and everything they own" + use case? The four-step order is already documented in `user-repository-mongo.js` and + implemented nowhere. +3. **`UsageCounter` and lawful erasure.** The policy here is "never delete", which 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. +4. **Audit trail.** Worth recording what each purge deleted, somewhere durable? ## Related -- ADR-005: Admin Script Runner Service — home for the orphan sweeper -- ADR-011: Integration Telemetry, Eventing & Feature-Usage Tracking — why `UsageCounter` survives -- ADR-022: Artifacts — S3 artifact lifecycle, explicitly out of scope here -- ADR-024: Global Entities — `isGlobal` skip predicate, pending that field existing -- `docs/MULTI_STEP_AUTH_AND_SHARED_ENTITIES_SPEC.md` — shared-entity design intent +- [ADR-024: Global Entities](./024-global-entities.md) — the sharing model this must not break +- [ADR-005: Admin Script Runner](./005-admin-script-runner.md) — how the orphan sweeper ships +- [ADR-010: Reporting as an Admin Operation](./010-reporting-as-admin-operation.md) — owns the S3 + report artifacts, which are execution-scoped and out of scope here +- Implementation: `packages/core/integrations/use-cases/delete-integration-for-user.js`, + `packages/core/integrations/repositories/`, `packages/core/modules/repositories/`, + `packages/core/credential/repositories/` From 7fcafe474d3e6a2279c6dc3921ab6260a7ced5b3 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 20 Aug 2026 23:21:02 +0000 Subject: [PATCH 6/7] docs(adr): fix sync contradiction in ADR-032 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Claude-Session: https://claude.ai/code/session_01M5jrmtqxucVoXrxvw7ph8h --- .../032-integration-deletion-cleanup.md | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/docs/architecture-decisions/032-integration-deletion-cleanup.md b/docs/architecture-decisions/032-integration-deletion-cleanup.md index 87e71d14c..f4acbef3a 100644 --- a/docs/architecture-decisions/032-integration-deletion-cleanup.md +++ b/docs/architecture-decisions/032-integration-deletion-cleanup.md @@ -15,10 +15,13 @@ How much of the integration's *own* data goes away depends on which database you | Backend | What happens | Result | |---|---|---| -| PostgreSQL | Real foreign keys with `ON DELETE CASCADE` | Mappings, syncs, associations and processes are removed | -| MongoDB | Same `onDelete: Cascade` in the schema, but no real foreign keys — Prisma emulates it | Usually removed, and the repo already warns against relying on it (`user-repository-mongo.js`) | +| PostgreSQL | Real foreign keys with `ON DELETE CASCADE` | Mappings, associations and processes are removed | +| MongoDB | Same `onDelete: Cascade` in the schema, but no real foreign keys — Prisma emulates it | The same set, usually. The repo already warns against relying on it (`user-repository-mongo.js`) | | DocumentDB | The adapter deletes through `$runCommandRaw`, which bypasses Prisma's query engine | Nothing cascades. Every child row is orphaned too | +Syncs are the exception on every backend. The cascade is declared, but `Sync.integrationId` is +never written, so it has never matched a row — see the fourth bug below. + So the schema says one thing and the behaviour is three different things. DocumentDB is the worst affected and the least obvious, because the schema *does* say `Cascade`. @@ -78,7 +81,10 @@ The rules: Order matters, so the steps run children first and the integration row last: -1. Delete mappings, processes, syncs and associations. +1. Delete mappings, processes and associations by integration id. Delete syncs by integration id + **or** by the integration's entity ids — the second half matters, because matching on + `Sync.integrationId` alone would delete nothing today. Scope it to *those* entity ids, never + to "where `integrationId` is null", which would take every parentless sync in the database. 2. For each entity: count the *other* integrations using it. Skip it if any remain, if it is marked global, or if its `userId` does not match. Otherwise delete it, and remember its credential id. From a865001a644f227a92eb07e342afab86477f790f Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 20 Aug 2026 23:21:51 +0000 Subject: [PATCH 7/7] docs(adr): delete nested records before their parents in ADR-032 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Claude-Session: https://claude.ai/code/session_01M5jrmtqxucVoXrxvw7ph8h --- .../032-integration-deletion-cleanup.md | 20 +++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/docs/architecture-decisions/032-integration-deletion-cleanup.md b/docs/architecture-decisions/032-integration-deletion-cleanup.md index f4acbef3a..6fb09cd73 100644 --- a/docs/architecture-decisions/032-integration-deletion-cleanup.md +++ b/docs/architecture-decisions/032-integration-deletion-cleanup.md @@ -19,8 +19,9 @@ How much of the integration's *own* data goes away depends on which database you | MongoDB | Same `onDelete: Cascade` in the schema, but no real foreign keys — Prisma emulates it | The same set, usually. The repo already warns against relying on it (`user-repository-mongo.js`) | | DocumentDB | The adapter deletes through `$runCommandRaw`, which bypasses Prisma's query engine | Nothing cascades. Every child row is orphaned too | -Syncs are the exception on every backend. The cascade is declared, but `Sync.integrationId` is -never written, so it has never matched a row — see the fourth bug below. +Syncs are the exception on every backend. The cascade is declared, but `SyncManager` never writes +`Sync.integrationId`, so for every sync the framework has created that foreign key is null and +matches nothing — see the fourth bug below. So the schema says one thing and the behaviour is three different things. DocumentDB is the worst affected and the least obvious, because the schema *does* say `Cascade`. @@ -81,10 +82,17 @@ The rules: Order matters, so the steps run children first and the integration row last: -1. Delete mappings, processes and associations by integration id. Delete syncs by integration id - **or** by the integration's entity ids — the second half matters, because matching on - `Sync.integrationId` alone would delete nothing today. Scope it to *those* entity ids, never - to "where `integrationId` is null", which would take every parentless sync in the database. +1. Delete the integration's own children, deepest first, so nothing is left pointing at a parent + that is already gone. DocumentDB cascades nothing, so each level is deleted explicitly rather + than assumed: + - `DataIdentifier`, then `Sync`. Match syncs on integration id **or** on the integration's + entity ids — the second arm matters, because matching on `Sync.integrationId` alone would + delete nothing today. Scope it to *those* entity ids, never to "where `integrationId` is + null", which would take every parentless sync in the database. On DocumentDB the + identifiers are an embedded array inside the sync document, so deleting the sync removes + them and there is no separate collection to clear. + - `AssociationObject`, then `Association`. + - `IntegrationMapping` and `Process`, by integration id. 2. For each entity: count the *other* integrations using it. Skip it if any remain, if it is marked global, or if its `userId` does not match. Otherwise delete it, and remember its credential id.