Defer irreversible cleanup until the removal commits

.changeset/defer-irreversible-cleanup.md

@@ -0,0 +1,13 @@
+---
+"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.
+
+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

@@ -4676,6 +4676,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,9 +3,10 @@ import { tmpdir } from "node:os";
 import { join } from "node:path";
 
 import { describe, expect, it } from "@effect/vitest";
-import { Effect } from "effect";
+import { Effect, Exit } from "effect";
 
-import { OAuthClientSlug } from "./ids";
+import { OAuthClientSlug, ProviderItemId, ProviderKey } from "./ids";
+import { definePlugin } from "./plugin";
 import { makeTestWorkspaceHarness, memoryCredentialsPlugin } from "./test-config";
 
 // removeClient permanently deletes an owner-scoped oauth_client row, keyed by
@@ -176,3 +177,73 @@ 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),
+    }),
+  }))();
+
+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({
+          owner: "user",
+          slug: USER_CLIENT,
+          authorizationUrl: "https://acme.test/authorize",
+          tokenUrl: "https://acme.test/token",
+          grant: "authorization_code",
+          clientId: "user-client-id",
+          clientSecret: "user-secret",
+        });
+        const secretItem = "oauth-client:user:acme-user:secret";
+        expect(store.get(secretItem)).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(secretItem)).toBe("user-secret");
+      }),
+    ),
+  );
+});

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,
@@ -690,11 +690,24 @@ export const makeOAuthService = (deps: OAuthServiceDeps): OAuthService => {
         )
         .pipe(Effect.asVoid);
       // 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(
+          dropSecret
+            .call(provider, ProviderItemId.make(clientSecretItemId(owner, slug)))
+            .pipe(Effect.catch(() => Effect.void)),
+        );
       }
     });
 

packages/core/sdk/src/plugin-after-commit.test.ts

@@ -0,0 +1,116 @@
+import { describe, expect, it } from "@effect/vitest";
+import { Effect, Exit } from "effect";
+
+import {
+  AuthTemplateSlug,
+  ConnectionName,
+  IntegrationSlug,
+  ProviderItemId,
+  ProviderKey,
+  ToolName,
+} from "./ids";
+import { definePlugin } from "./plugin";
+import { makeTestExecutor } from "./test-config";
+
+// A plugin's `removeConnection` runs INSIDE core's removal transaction, which is
+// what makes its database work atomic with the row deletions. The same property
+// makes anything reaching outside the database unsafe there: revoking a token at
+// the provider's API cannot be rolled back with the transaction, so an abort
+// leaves the connection restored and the token already dead.
+//
+// `ctx.afterCommit` is the way out, and these pin both directions of its
+// contract — it runs when the removal is durable, and it is discarded when the
+// removal is not.
+
+const INTEG = IntegrationSlug.make("vercel");
+const TEMPLATE = AuthTemplateSlug.make("apiKey");
+
+const revokingPlugin = (revoked: string[]) =>
+  definePlugin(() => {
+    const store = new Map<string, string>();
+    return {
+      id: "demo" as const,
+      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));
+            }),
+        },
+      ],
+      storage: () => ({}),
+      resolveTools: () =>
+        Effect.succeed({ tools: [{ name: ToolName.make("deploy"), description: "deploy" }] }),
+      invokeTool: ({ toolRow }) => Effect.succeed({ ran: toolRow.name }),
+      /** Stands in for "revoke the token at the provider's API" — the archetypal
+       *  irreversible, outside-the-database cleanup. */
+      removeConnection: ({ ctx, connection }) =>
+        ctx.afterCommit(
+          Effect.sync(() => {
+            revoked.push(String(connection.name));
+          }),
+        ),
+      extension: (ctx) => ({
+        seed: () =>
+          ctx.core.integrations.register({ slug: INTEG, description: "Vercel", config: {} }),
+        inTransaction: <A, E>(effect: Effect.Effect<A, E>) => ctx.transaction(effect),
+      }),
+    };
+  })();
+
+const setup = (revoked: string[]) =>
+  makeTestExecutor({ plugins: [revokingPlugin(revoked)] as const }).pipe(
+    Effect.tap((executor) => executor.demo.seed()),
+  );
+
+const REF = {
+  owner: "org",
+  integration: INTEG,
+  name: ConnectionName.make("main"),
+} as const;
+
+describe("ctx.afterCommit inside a lifecycle hook", () => {
+  it.effect("runs the deferred cleanup once the removal is durable", () =>
+    Effect.gen(function* () {
+      const revoked: string[] = [];
+      const executor = yield* setup(revoked);
+      yield* executor.connections.create({ ...REF, template: TEMPLATE, value: "secret-token" });
+
+      yield* executor.connections.remove(REF);
+
+      // Deferring must not mean dropping: an ordinary removal still revokes.
+      expect(revoked).toEqual(["main"]);
+    }),
+  );
+
+  it.effect("discards the deferred cleanup when the removal rolls back", () =>
+    Effect.gen(function* () {
+      const revoked: string[] = [];
+      const executor = yield* setup(revoked);
+      yield* executor.connections.create({ ...REF, template: TEMPLATE, value: "secret-token" });
+
+      const outcome = yield* Effect.exit(
+        executor.demo.inTransaction(
+          Effect.gen(function* () {
+            yield* executor.connections.remove(REF);
+            return yield* Effect.fail("rollback" as const);
+          }),
+        ),
+      );
+      expect(Exit.isFailure(outcome)).toBe(true);
+
+      // The connection survived, so revoking its token would have destroyed a
+      // live credential with nothing left to undo it.
+      const stillThere = yield* executor.connections.get(REF);
+      expect(String(stillThere?.name)).toBe("main");
+      expect(revoked).toEqual([]);
+    }),
+  );
+});

packages/core/sdk/src/plugin.ts

@@ -286,6 +286,25 @@ export interface PluginCtx<TStore = unknown> {
   /** Run `effect` inside a FumaDB transaction (atomic across plugin storage +
    *  core integration/tool writes). */
   readonly transaction: <A, E>(effect: Effect.Effect<A, E>) => Effect.Effect<A, E | StorageFailure>;
+
+  /** Defer `effect` until the OUTERMOST transaction commits; discard it if that
+   *  transaction rolls back. With none active it runs immediately.
+   *
+   *  Use this for anything that reaches OUTSIDE the database — revoking a token
+   *  at the provider's API, deleting a remote object, sending a webhook. Such
+   *  work does not enlist in the transaction and cannot be rolled back with it,
+   *  so performing it inline means a later abort leaves the database restored
+   *  and the outside world already changed. That gap is not theoretical: the
+   *  lifecycle hooks below run inside core's own transaction.
+   *
+   *  Sequencing it after your `transaction(...)` call is NOT the same thing.
+   *  `transaction` nests by pass-through, so inside an active transaction the
+   *  inner call just runs its effect and "afterwards" is still before any
+   *  commit. This is the only construct that waits for the real one.
+   *
+   *  Best-effort by contract: failures and defects are swallowed, so a hook that
+   *  cannot tidy up never fails the operation that triggered it. */
+  readonly afterCommit: (effect: Effect.Effect<void>) => Effect.Effect<void>;
 }
 
 // ---------------------------------------------------------------------------
@@ -706,13 +725,27 @@ export interface PluginSpec<
     readonly toolRows: readonly ToolInvocationRow[];
   }) => Effect.Effect<Record<string, ToolAnnotations>, unknown>;
 
-  /** Plugin-side cleanup when a connection is removed. */
+  /** Plugin-side cleanup when a connection is removed.
+   *
+   *  RUNS INSIDE core's removal transaction, so database work here is atomic
+   *  with the row deletions — which is the point. The consequence is that
+   *  anything reaching outside the database is NOT: revoking the token at the
+   *  provider's API, deleting a remote object, notifying a third party. If the
+   *  transaction later aborts, the connection is restored and that external
+   *  action has already happened, with nothing left to undo it.
+   *
+   *  Wrap such work in `ctx.afterCommit(...)`. It runs once the removal is
+   *  durable and is discarded if the removal rolls back. */
   readonly removeConnection?: (
     input: ConnectionLifecycleInput<TStore>,
   ) => Effect.Effect<void, unknown>;
 
   /** Plugin-side cleanup when a removable integration is removed. Core still
-   *  owns deleting the integration, connection, tool, and definition rows. */
+   *  owns deleting the integration, connection, tool, and definition rows.
+   *
+   *  Runs inside core's removal transaction, with the same consequence as
+   *  `removeConnection` above: defer any work that reaches outside the database
+   *  through `ctx.afterCommit(...)`. */
   readonly removeIntegration?: (
     input: IntegrationLifecycleInput<TStore>,
   ) => Effect.Effect<void, unknown>;