Defer irreversible cleanup until the removal commits

.changeset/defer-irreversible-cleanup.md

@@ -0,0 +1,15 @@
+---
+"executor": minor
+---
+
+**Irreversible cleanup now waits for the transaction to commit, and plugins can do the same**
+
+`oauth.removeClient` deleted the client row and then deleted the client secret from the credential provider. The provider does not enlist in the caller's transaction and does not roll back with it, so an abort restored the client row while its secret stayed destroyed — a client that looks configured and can never authenticate again. The deletion now waits until the removal is durable and is discarded if the removal rolls back. With no transaction active it runs immediately, exactly as before.
+
+Deferring the deletion is not enough on its own. The secret is stored under a key derived from the app's `(owner, slug)` identity alone, so the key outlives the row it belonged to: whoever holds that identity when the deletion finally runs owns the key. A slug registered again before the removal committed would lose the new app's secret to the old app's queued deletion — the same unauthenticatable client, reached the other way round. The deferred deletion now re-checks that the app is still gone and stands down when it is not. A removal that matched no row also no longer queues a deletion at all: it removed nothing, so it has no claim on the key, which may well hold another subject's live secret.
+
+The same trap was reachable by plugins and they had no way out of it. `removeConnection` and `removeIntegration` run inside core's removal transaction — deliberately, so a plugin's own rows die atomically with the connection — which makes them exactly the wrong place to revoke a token at the provider's API, delete a remote object, or notify a third party. Nothing in the hooks' documentation said so, and `PluginCtx` exposed `transaction` but nothing to defer past it.
+
+`PluginCtx` gains `afterCommit`. It runs the effect once the outermost transaction commits, discards it if that transaction rolls back, and runs it immediately when no transaction is active. The lifecycle hooks now document that they run inside core's transaction and that outside-world work belongs in `afterCommit`.
+
+Sequencing work after your own `transaction(...)` call is not equivalent, and the documentation says so explicitly: `transaction` nests by pass-through, so inside an active transaction the inner call simply runs its effect and "afterwards" is still before any commit.

packages/core/sdk/src/executor.ts

@@ -5141,6 +5141,7 @@ export const createExecutor = <const TPlugins extends readonly AnyPlugin[] = rea
         oauth,
         execute: (address, args, options) => execute(address, args, options),
         transaction: <A, E>(effect: Effect.Effect<A, E>) => transaction(effect),
+        afterCommit: (effect: Effect.Effect<void>) => afterCommit(effect),
       };
 
       if (plugin.toolPolicyProvider) {

packages/core/sdk/src/oauth-remove-client.test.ts

@@ -3,10 +3,21 @@ import { tmpdir } from "node:os";
 import { join } from "node:path";
 
 import { describe, expect, it } from "@effect/vitest";
-import { Effect } from "effect";
-
-import { OAuthClientSlug, ToolAddress } from "./ids";
+import { Effect, Exit } from "effect";
+
+import {
+  AuthTemplateSlug,
+  ConnectionName,
+  IntegrationSlug,
+  OAuthClientSlug,
+  ProviderItemId,
+  ProviderKey,
+  ToolAddress,
+  ToolName,
+} from "./ids";
+import { definePlugin } from "./plugin";
 import { makeTestWorkspaceHarness, memoryCredentialsPlugin } from "./test-config";
+import { serveOAuthTestServer } from "./testing/oauth-test-server";
 
 // removeClient permanently deletes an owner-scoped oauth_client row, keyed by
 // (owner, slug). The owner policy on `oauth_client` prevents removing another
@@ -241,3 +252,262 @@ describe("oauth.removeClient", () => {
     ),
   );
 });
+
+// Removing a client deletes its secret from the provider, and that reaches a
+// store which does not roll back with a transaction. `removeClient` opens none
+// itself, but a caller can wrap it — and an abort would then restore the client
+// row while its secret stayed destroyed, leaving a client that looks configured
+// and can never authenticate again.
+const txPlugin = (store: Map<string, string>) =>
+  definePlugin(() => ({
+    id: "demo" as const,
+    storage: () => ({}),
+    credentialProviders: [
+      {
+        key: ProviderKey.make("memory"),
+        writable: true as const,
+        get: (id: ProviderItemId) => Effect.sync(() => store.get(String(id)) ?? null),
+        set: (id: ProviderItemId, value: string) =>
+          Effect.sync(() => {
+            store.set(String(id), value);
+          }),
+        delete: (id: ProviderItemId) =>
+          Effect.sync(() => {
+            store.delete(String(id));
+          }),
+      },
+    ],
+    extension: (ctx) => ({
+      inTransaction: <A, E>(effect: Effect.Effect<A, E>) => ctx.transaction(effect),
+    }),
+  }))();
+
+// The client secret is keyed by (owner, slug) ALONE — the key outlives the row
+// it belonged to, which is what makes the deferred delete a claim that has to be
+// re-checked rather than replayed.
+const SECRET_ITEM = "oauth-client:user:acme-user:secret";
+
+/** The registration used by the secret-lifecycle tests below, parameterised only
+ *  by the secret so each one can prove which incarnation's secret survived. */
+const userClient = (clientSecret: string) =>
+  ({
+    owner: "user",
+    slug: USER_CLIENT,
+    authorizationUrl: "https://acme.test/authorize",
+    tokenUrl: "https://acme.test/token",
+    grant: "authorization_code",
+    clientId: "user-client-id",
+    clientSecret,
+  }) as const;
+
+describe("removing a client defers the secret deletion to the outermost commit", () => {
+  it.effect("a rolled-back removal leaves the client secret intact", () =>
+    Effect.scoped(
+      Effect.gen(function* () {
+        const store = new Map<string, string>();
+        const { executor } = yield* makeTestWorkspaceHarness({
+          plugins: [txPlugin(store)] as const,
+        });
+        yield* executor.oauth.createClient(userClient("user-secret"));
+        expect(store.get(SECRET_ITEM)).toBe("user-secret");
+
+        // A caller wraps the removal in its own transaction, then fails.
+        const outcome = yield* Effect.exit(
+          executor.demo.inTransaction(
+            Effect.gen(function* () {
+              yield* executor.oauth.removeClient("user", USER_CLIENT);
+              return yield* Effect.fail("rollback" as const);
+            }),
+          ),
+        );
+        expect(Exit.isFailure(outcome)).toBe(true);
+
+        // The client came back...
+        const after = yield* executor.oauth.listClients();
+        expect(after.map((client) => String(client.slug))).toContain(String(USER_CLIENT));
+        // ...so its secret must still be there, or it can never authenticate again.
+        expect(store.get(SECRET_ITEM)).toBe("user-secret");
+      }),
+    ),
+  );
+
+  // Deferring must not mean dropping. Without this, removing the deferred
+  // cleanup entirely still passes the rollback test above — the secret would
+  // simply leak on every successful removal, unobserved.
+  it.effect("a committed removal deletes the client secret", () =>
+    Effect.scoped(
+      Effect.gen(function* () {
+        const store = new Map<string, string>();
+        const { executor } = yield* makeTestWorkspaceHarness({
+          plugins: [txPlugin(store)] as const,
+        });
+        yield* executor.oauth.createClient(userClient("user-secret"));
+        expect(store.get(SECRET_ITEM)).toBe("user-secret");
+
+        yield* executor.demo.inTransaction(executor.oauth.removeClient("user", USER_CLIENT));
+
+        expect(yield* executor.oauth.listClients()).toEqual([]);
+        expect(store.has(SECRET_ITEM)).toBe(false);
+      }),
+    ),
+  );
+
+  // With no transaction active `afterCommit` runs its effect inline, so the
+  // ordinary removal path stays synchronous rather than waiting for a commit
+  // that will never come.
+  it.effect("a removal outside any transaction deletes the secret immediately", () =>
+    Effect.scoped(
+      Effect.gen(function* () {
+        const store = new Map<string, string>();
+        const { executor } = yield* makeTestWorkspaceHarness({
+          plugins: [txPlugin(store)] as const,
+        });
+        yield* executor.oauth.createClient(userClient("user-secret"));
+        expect(store.get(SECRET_ITEM)).toBe("user-secret");
+
+        yield* executor.oauth.removeClient("user", USER_CLIENT);
+
+        expect(store.has(SECRET_ITEM)).toBe(false);
+      }),
+    ),
+  );
+
+  // A removal that matched no row removed nothing, so it has no claim on the
+  // key — and the key is not private to the caller. Here B's removal is scoped
+  // away by the owner policy and touches no row at all, while the secret it
+  // would have deleted is A's live one.
+  it.effect("a removal that matched no row leaves the key alone", () =>
+    Effect.scoped(
+      Effect.gen(function* () {
+        const store = new Map<string, string>();
+        const plugins = [txPlugin(store)] as const;
+        const dataDir = mkdtempSync(join(tmpdir(), "oauth-remove-client-secret-"));
+        const tenant = "shared-tenant";
+
+        const a = yield* makeTestWorkspaceHarness({
+          plugins,
+          tenant,
+          subject: "subject-a",
+          dataDir,
+        });
+        yield* a.executor.oauth.createClient(userClient("a-secret"));
+        expect(store.get(SECRET_ITEM)).toBe("a-secret");
+
+        const b = yield* makeTestWorkspaceHarness({
+          plugins,
+          tenant,
+          subject: "subject-b",
+          dataDir,
+        });
+        yield* b.executor.oauth.removeClient("user", USER_CLIENT);
+
+        // A's client survived the owner-scoped delete...
+        const clientsA = yield* a.executor.oauth.listClients();
+        expect(clientsA.map((client) => String(client.slug))).toContain(String(USER_CLIENT));
+        // ...and so must its secret, which B never owned.
+        expect(store.get(SECRET_ITEM)).toBe("a-secret");
+      }),
+    ),
+  );
+});
+
+// An integration to hang an OAuth connection off, so the recreated client's
+// secret can be proven to still WORK — the test authorization server rejects a
+// token request that presents the wrong secret (or none).
+const INTEG = IntegrationSlug.make("acme");
+const TEMPLATE = AuthTemplateSlug.make("oauth");
+
+const integrationPlugin = definePlugin(() => ({
+  id: "acme" as const,
+  storage: () => ({}),
+  resolveTools: () =>
+    Effect.succeed({ tools: [{ name: ToolName.make("whoami"), description: "whoami" }] }),
+  describeAuthMethods: () => [
+    {
+      id: "oauth",
+      label: "OAuth2",
+      kind: "oauth" as const,
+      template: String(TEMPLATE),
+      oauth: { scopes: [] },
+    },
+  ],
+  invokeTool: ({ credential }) => Effect.succeed({ token: credential.value }),
+  extension: (ctx) => ({
+    seed: () => ctx.core.integrations.register({ slug: INTEG, description: "Acme", config: {} }),
+  }),
+}))();
+
+// Deferring the delete to the commit is only half the answer. The provider key
+// is derived from (owner, slug) and nothing else, so it is not owned by the row
+// that was removed — it is owned by whichever row holds that identity when the
+// deletion finally runs. A slug recreated inside the removal's own transaction
+// commits together with the removal, and the queued delete then fires against
+// the NEW app's secret: a client that looks configured and can never
+// authenticate, the exact state the rollback path above prevents.
+describe("removing a client does not delete a recreated client's secret", () => {
+  it.effect("a slug recreated before the commit keeps its own working secret", () =>
+    Effect.scoped(
+      Effect.gen(function* () {
+        const server = yield* serveOAuthTestServer({
+          scopes: ["read"],
+          clients: { "recreated-client": "second-secret" },
+        });
+        const store = new Map<string, string>();
+        const { executor } = yield* makeTestWorkspaceHarness({
+          plugins: [txPlugin(store), integrationPlugin] as const,
+          redirectUri: null,
+        });
+        yield* executor.acme.seed();
+
+        // The first incarnation, registered against a client id the server does
+        // not know: only the second one can ever mint a token.
+        yield* executor.oauth.createClient({
+          owner: "user",
+          slug: USER_CLIENT,
+          authorizationUrl: server.authorizationEndpoint,
+          tokenUrl: server.tokenEndpoint,
+          grant: "client_credentials",
+          clientId: "first-client",
+          clientSecret: "first-secret",
+        });
+        expect(store.get(SECRET_ITEM)).toBe("first-secret");
+
+        // The race, made deterministic: the removal and the re-registration of
+        // the same slug commit together, so the deferred delete runs against a
+        // live client.
+        yield* executor.demo.inTransaction(
+          Effect.gen(function* () {
+            yield* executor.oauth.removeClient("user", USER_CLIENT);
+            yield* executor.oauth.createClient({
+              owner: "user",
+              slug: USER_CLIENT,
+              authorizationUrl: server.authorizationEndpoint,
+              tokenUrl: server.tokenEndpoint,
+              grant: "client_credentials",
+              clientId: "recreated-client",
+              clientSecret: "second-secret",
+            });
+          }),
+        );
+
+        // The new incarnation is listed, and its secret survived...
+        const after = yield* executor.oauth.listClients();
+        expect(after.map((client) => String(client.slug))).toContain(String(USER_CLIENT));
+        expect(store.get(SECRET_ITEM)).toBe("second-secret");
+
+        // ...and still authenticates: the server refuses a token request that
+        // presents the wrong secret or none, so a connection can only be minted
+        // with the intact one.
+        const started = yield* executor.oauth.start({
+          owner: "user",
+          client: USER_CLIENT,
+          clientOwner: "user",
+          name: ConnectionName.make("cc"),
+          integration: INTEG,
+          template: TEMPLATE,
+        });
+        expect(started.status).toBe("connected");
+      }),
+    ),
+  );
+});

packages/core/sdk/src/oauth-service.ts

@@ -20,7 +20,7 @@ import { FetchHttpClient, type HttpClient } from "effect/unstable/http";
 import { connectionIdentifier } from "./connection-name-identifier";
 import type { Connection } from "./connection";
 import type { IFumaClient, StorageFailure } from "./fuma-runtime";
-import { StorageError } from "./fuma-runtime";
+import { afterCommit, StorageError } from "./fuma-runtime";
 import {
   AuthTemplateSlug,
   ConnectionName,
@@ -905,19 +905,59 @@ export const makeOAuthService = (deps: OAuthServiceDeps): OAuthService => {
           cause: undefined,
         });
       }
+      // "Is there an app at (owner, slug) right now?" — asked twice, for two
+      // different reasons. Before the delete it says whether this call removes
+      // anything at all; after the commit it says whether the secret key still
+      // belongs to the app this call removed.
+      const findClientRow = deps.fuma.use("oauth_client.findFirst", (db) =>
+        looseDb(db).findFirst("oauth_client", {
+          where: (b: any) => b.and(b("owner", "=", owner), b("slug", "=", String(slug))),
+        }),
+      );
+
+      const removedRow = yield* findClientRow;
       yield* deps.fuma
         .use("oauth_client.delete", (db) =>
           looseDb(db).deleteMany("oauth_client", {
             where: (b: any) => b.and(b("owner", "=", owner), b("slug", "=", String(slug))),
           }),
         )
         .pipe(Effect.asVoid);
+      // Nothing matched, so this call removed nothing and owns no secret. The
+      // idempotent no-op and the cross-subject miss both land here, and both
+      // used to queue a delete of a key they never had a claim on.
+      if (!removedRow) return;
+
       // Best-effort: drop the secret from the provider so it isn't orphaned.
+      //
+      // Deferred to the outermost commit. This function opens no transaction of
+      // its own, but a caller can wrap it in one — and `provider.delete` reaches
+      // a store that does not roll back with it. An abort would then restore the
+      // client row while its secret stayed destroyed, leaving a client that
+      // looks configured and can never authenticate again. Orphaning a secret is
+      // recoverable; deleting one that is still referenced is not, so the
+      // deletion waits until the row's removal is durable. With no transaction
+      // active `afterCommit` runs it immediately, which is the behaviour this
+      // path already had.
       const provider = deps.defaultWritableProvider();
-      if (provider?.delete) {
-        yield* provider
-          .delete(ProviderItemId.make(clientSecretItemId(owner, slug)))
-          .pipe(Effect.catch(() => Effect.void));
+      const dropSecret = provider?.delete;
+      if (provider && dropSecret) {
+        yield* afterCommit(
+          Effect.gen(function* () {
+            // Deferral alone is not enough: the secret is keyed by (owner, slug)
+            // ALONE, so the key outlives the row it belonged to. If the same
+            // slug is registered again before this hook runs, the key now holds
+            // the NEW app's secret, and deleting it recreates exactly the state
+            // the deferral exists to prevent — a client that looks configured
+            // and can never authenticate. Re-check that the app is still gone
+            // and stand down when it is not. A re-check that FAILS is caught
+            // below and also stands down, which is the deliberate direction:
+            // an orphaned secret is recoverable, a destroyed live one is not.
+            const recreated = yield* findClientRow;
+            if (recreated) return;
+            yield* dropSecret.call(provider, ProviderItemId.make(clientSecretItemId(owner, slug)));
+          }).pipe(Effect.catch(() => Effect.void)),
+        );
       }
     });