Record what each app answered when one of them refuses an offboarding

CHANGELOG.md

@@ -8,6 +8,16 @@ Newest first. `Unreleased` is what is on `main` and not yet tagged.
 
 ## Unreleased
 
+### An offboarding one app refuses still records the apps that answered
+
+Removing somebody withdraws each brokered account they connected. When the broker refused one of
+those apps, the whole act stopped before anything was recorded: apps already withdrawn at Composio
+kept their `composio_connections` row and left nothing on the trail saying the account had ended,
+and the retry that #574 made the recovery then asked again, was told there was nothing to withdraw,
+and wrote `vendorRevocationRequested: false` about a withdrawal this deployment had asked for and
+got. Each app is now asked, recorded with the answer it actually gave and its row removed, and only
+the apps that were refused are left standing for the retry. The act still fails and still answers
+500, so a refusal is as loud as it was.
 ### Paging the audit trail no longer skips rows written in the same millisecond
 
 `GET /api/admin/audit-events` hands out a `nextCursor` built from the last row's timestamp, which the

server/src/plugins/store.ts

@@ -6691,22 +6691,65 @@ export function createPluginStore(options: PluginStoreOptions) {
        * rather than the call. False where there is no broker at all: a deployment whose key has
        * since been unset can still offboard somebody, and it could not have been calling Composio
        * either way — but nothing was asked there and the row must not claim otherwise.
+       *
+       * ONE APP'S REFUSAL IS ONE APP'S REFUSAL, and until now it was everybody's. A throw out of
+       * `revoke` left this loop before the delete and before the trail, so three accounts already
+       * withdrawn at Composio kept their rows and got no row on the trail. That was survivable while
+       * repeating the act did nothing — and #574 made repeating it the documented recovery, so the
+       * second pass asks again for those three, Composio answers `false` because the accounts are
+       * gone, and each writes `vendorRevocationRequested: false` about a withdrawal this deployment
+       * asked for and got. That field exists to tell an account we acted on from one that outlives
+       * us somewhere else; those three rows say the wrong one.
+       *
+       * So the answer is kept per app and the refusal is held rather than thrown. Every app is still
+       * asked — a later one is not punished for an earlier one — and the first refusal is rethrown
+       * below, so the act still fails loudly and the administrator still gets a 500.
        */
-      const vendorRevocationRequested = new Map<string, boolean>();
+      const withdrawn: { toolkit: string; requested: boolean }[] = [];
+      const refusals: unknown[] = [];
       for (const connection of brokered) {
-        vendorRevocationRequested.set(
-          connection.toolkit,
-          broker
-            ? await broker.revoke({ userId, toolkit: connection.toolkit })
-            : false,
-        );
+        try {
+          withdrawn.push({
+            toolkit: connection.toolkit,
+            requested: broker
+              ? await broker.revoke({ userId, toolkit: connection.toolkit })
+              : false,
+          });
+        } catch (error) {
+          /*
+           * Held, and the row deliberately left standing.
+           *
+           * "An offboarding the vendor refuses leaves the connection standing" is the existing
+           * criterion and it is unchanged: the row is the only thing naming which app this person
+           * connected, repeating the act is the recovery, and repeating it is only possible while
+           * the row is there. What changes is that the rule now applies to the app it is about
+           * rather than to every app in the same act.
+           */
+          refusals.push(error);
+        }
       }
 
-      await database
-        .delete(composioConnections)
-        .where(eq(composioConnections.userId, userId));
+      /*
+       * Only the apps that answered, which is the other half of the same correction.
+       *
+       * Deleting by user id would take the rows of apps that were refused or never reached, and
+       * those are exactly the rows the recovery needs. Deleting none — what a throw used to do —
+       * leaves a row and an open `(toolkit, user_id)` gate for an account that is already gone at
+       * Composio, so the table claims a connection this person does not have.
+       */
+      if (withdrawn.length > 0) {
+        await database.delete(composioConnections).where(
+          and(
+            eq(composioConnections.userId, userId),
+            inArray(
+              composioConnections.toolkit,
+              withdrawn.map((entry) => entry.toolkit),
+            ),
+          ),
+        );
+      }
 
-      for (const connection of brokered) {
+      for (const connection of withdrawn) {
         retired += 1;
         await recordAuditEvent(auditStore, {
           eventType: "mcp.account_disconnected",
@@ -6729,12 +6772,21 @@ export function createPluginStore(options: PluginStoreOptions) {
              * ask. The value of the field is exactly that a reader can tell those apart, so a
              * constant here would be worse than none.
              */
-            vendorRevocationRequested:
-              vendorRevocationRequested.get(connection.toolkit) ?? false,
+            vendorRevocationRequested: connection.requested,
           },
         });
       }
 
+      /*
+       * Loud, after every app has been asked and every answer recorded.
+       *
+       * The first, because the route turns this into a 500 and one sentence is what reaches the
+       * administrator; the rest are the same act failing more than once, and the trail above already
+       * says which apps did not end. Thrown last rather than first so a refusal on one app cannot
+       * cost the record of another — which is the whole of this change.
+       */
+      if (refusals.length > 0) throw refusals[0];
+
       return { retired };
     },
 

server/tests/composio-connections.test.ts

@@ -1304,6 +1304,95 @@ test("an offboarding carries the vendor's answer per app, in a fixed order", asy
   ]);
 });
 
+/**
+ * ONE APP'S REFUSAL IS ONE APP'S REFUSAL, AND THE APPS THAT ANSWERED ARE RECORDED.
+ *
+ * CRITERION. Two apps, the vendor refusing the second: the act still fails, the refused app keeps
+ * its row and gets no trail row, and the app that WAS withdrawn loses its row and is recorded with
+ * `vendorRevocationRequested: true`.
+ *
+ * REASON. A throw out of `revoke` used to leave the loop before the delete and before the trail, so
+ * an account already gone at Composio kept its row and its `(toolkit, user_id)` gate and left
+ * nothing on the trail saying it had ended. That was survivable while repeating the act did
+ * nothing — and #574 made repeating it the documented recovery, so the second pass asks again for
+ * that app, the vendor answers `false` because the account is already gone, and the row that
+ * finally lands says `vendorRevocationRequested: false` about a withdrawal this deployment asked
+ * for and got. `vendorRevocationRequested` exists to tell an account we acted on from one that
+ * outlives us somewhere else; that row says the wrong one, permanently, and no later act corrects it.
+ *
+ * THE REFUSED APP IS UNCHANGED, which is the criterion the single-app test above already states:
+ * the row is the only thing naming which app this person connected, and repeating the act is only
+ * possible while it is there. What is new is that the rule now applies to the app it is about
+ * rather than to every app in the same act.
+ */
+test("an offboarding one app refuses still records the app that answered", async () => {
+  await seedApp({ connect: false });
+  await database
+    .insert(composioConnections)
+    .values({ toolkit, userId: askerId });
+  await database
+    .insert(composioConnections)
+    .values({ toolkit: secondToolkit, userId: askerId });
+  vendorRefuses = ({ toolkit: asked }) => asked === secondToolkit;
+
+  await expect(store.retireConnectionsFor(askerId, admin)).rejects.toThrow(
+    /would not withdraw/i,
+  );
+
+  // Both were asked. A later app is not punished for an earlier one, and on the previous shape the
+  // throw left the loop, so an app after the refused one was never reached at all.
+  expect(asksMade()).toEqual([
+    `revoke:${toolkit}/${askerId}`,
+    `revoke:${secondToolkit}/${askerId}`,
+  ]);
+
+  // The withdrawn app's row is gone; the refused app's stands, so the recovery still has it.
+  expect(await connectedToolkitsFor(askerId)).toEqual([secondToolkit]);
+
+  const disconnected = recordedOfType("mcp.account_disconnected");
+  expect(disconnected).toHaveLength(1);
+  expect(disconnected[0]?.payload).toMatchObject({
+    server: toolkit,
+    owner: askerId,
+    reason: "person_removed",
+    // The answer this app actually got, written while it was still known.
+    vendorRevocationRequested: true,
+  });
+});
+
+/**
+ * AND THE REFUSAL IS STILL THROWN, WHATEVER ORDER IT CAME IN.
+ *
+ * CRITERION. The vendor refusing the FIRST app: the second is still asked and still recorded, and
+ * the act still fails with the refusal rather than reporting a success.
+ *
+ * REASON. Holding a refusal instead of throwing it is only correct if it is still thrown. The risk
+ * this pins is the opposite of the one above — that collecting refusals turns a failed offboarding
+ * into a reported one, which is the state `an offboarding the vendor refuses leaves the connection
+ * standing` exists to forbid.
+ */
+test("a refusal on the first app still fails the act and still asks the second", async () => {
+  await seedApp({ connect: false });
+  await database
+    .insert(composioConnections)
+    .values({ toolkit, userId: askerId });
+  await database
+    .insert(composioConnections)
+    .values({ toolkit: secondToolkit, userId: askerId });
+  vendorRefuses = ({ toolkit: asked }) => asked === toolkit;
+
+  await expect(store.retireConnectionsFor(askerId, admin)).rejects.toThrow(
+    /would not withdraw/i,
+  );
+
+  expect(asksMade()).toEqual([
+    `revoke:${toolkit}/${askerId}`,
+    `revoke:${secondToolkit}/${askerId}`,
+  ]);
+  expect(await connectedToolkitsFor(askerId)).toEqual([toolkit]);
+  expect(recordedOfType("mcp.account_disconnected")).toHaveLength(1);
+});
+
 /**
  * THE ANONYMOUS ACTOR OWNS NOTHING, and `notNull` does not exclude the empty string, so a row at
  * `(toolkit, "")` is legal. Retiring "nobody" must not be what deletes it — that would be an