fix(control-plane): revoke orphaned secrets when createContainer fails after injectSecrets (#8255)

The #8202 reorder (database -> injectSecrets -> createContainer) opened a
window where injectSecrets succeeds and mints secretRef, then createContainer
fails right after -- since provisionTenant always rethrows rather than
returning on a step failure, secretRef never reached the caller to persist
and later revoke, permanently orphaning a live credential in the broker.
Unreachable before the reorder, since injectSecrets used to be the last step.

provisionTenant now best-effort revokes that secretRef itself before
rethrowing, and the PagerDuty alert carries it too as a fallback for when the
revoke itself fails (e.g. broker unreachable).

Author: JSONbored

control-plane/src/pagerduty-notify.ts

@@ -50,12 +50,17 @@ function warnProvisioningPagerDutyFailed(tenantName: string, error: unknown): vo
 
 /** Build the alert payload for a provisioning or deprovisioning failure (#7667). Pure -- no IO. `error` is
  *  coerced through the same {@link pagerDutyFailMessage} helper the IO path uses for its own failure logging, so
- *  the paged summary and any local warn log agree on the same truncated message. */
+ *  the paged summary and any local warn log agree on the same truncated message. `secretRef` (#8202, optional)
+ *  is included in `customDetails` when the caller already had one at failure time -- provisionTenant's own
+ *  best-effort revoke (provisioning.ts) is the primary defense against a dangling broker secret, but a revoke
+ *  can itself fail (e.g. broker unreachable), so the page still needs to hand an operator something to manually
+ *  revoke by rather than nothing at all. */
 export function buildProvisioningPagerDutyAlert(input: {
   tenantName: string;
   product: string;
   phase: "provision" | "deprovision";
   error: unknown;
+  secretRef?: string;
 }): ProvisioningPagerDutyAlert {
   const message = pagerDutyFailMessage(input.error);
   return {
@@ -65,7 +70,13 @@ export function buildProvisioningPagerDutyAlert(input: {
     summary: `${input.product} tenant ${input.phase} failed for ${input.tenantName}: ${message}`,
     severity: "critical",
     dedupKey: `control_plane_${input.phase}_failed:${input.product}:${input.tenantName}`,
-    customDetails: { tenantName: input.tenantName, product: input.product, phase: input.phase, message },
+    customDetails: {
+      tenantName: input.tenantName,
+      product: input.product,
+      phase: input.phase,
+      message,
+      ...(input.secretRef !== undefined ? { secretRef: input.secretRef } : {}),
+    },
   };
 }
 

control-plane/src/provisioning.ts

@@ -69,8 +69,9 @@ function pageAndRethrow(
   phase: "provision" | "deprovision",
   error: unknown,
   options: ProvisioningPagerDutyOptions,
+  secretRef?: string,
 ): never {
-  const alert = buildProvisioningPagerDutyAlert({ tenantName: tenant.name, product, phase, error });
+  const alert = buildProvisioningPagerDutyAlert({ tenantName: tenant.name, product, phase, error, secretRef });
   const notify = options.notify ?? notifyProvisioningFailure;
   const env = options.env ?? process.env;
   const warnNotifyFailed = (notifyError: unknown): void => {
@@ -93,9 +94,13 @@ function pageAndRethrow(
  *  just the tenant identity every other step operates on. `createContainer` is in turn called with `database`
  *  still attached AND `bootstrapSecret` newly attached (#8202) whenever `injectSecrets` returned one -- a real
  *  container driver delivers it into the container's own cold-boot environment. A step failure pages (#7667) and
- *  always rethrows — provisioning never fails silently. `onFailure` (#7677, optional) runs first in that failure
- *  path — the caller's seam for persisting the `"failed"` lifecycle state — and is best-effort: its own
- *  rejection is swallowed so it can never mask the step error. */
+ *  always rethrows — provisioning never fails silently. If `createContainer` is what failed AFTER `injectSecrets`
+ *  already succeeded (#8202), this function best-effort revokes that just-injected secret itself before
+ *  rethrowing -- since it always throws rather than returning on failure, no caller ever gets a chance to persist
+ *  `secretRef` for a later `deprovisionTenant` otherwise, which would permanently orphan a live credential in the
+ *  broker. `onFailure` (#7677, optional) runs after that in the same failure path — the caller's seam for
+ *  persisting the `"failed"` lifecycle state — and, like the revoke attempt, is best-effort: neither's own
+ *  rejection can mask the step error. */
 export async function provisionTenant(
   tenant: Tenant,
   product: Product,
@@ -112,12 +117,34 @@ export async function provisionTenant(
     secretRef = injected.secretRef;
     await driver.createContainer({ ...request, database, ...(injected.bootstrapSecret !== undefined ? { bootstrapSecret: injected.bootstrapSecret } : {}) });
   } catch (error) {
+    // #8202: injectSecrets can succeed (custodying a real secret + minting secretRef) and createContainer can
+    // still fail right after it (Cloudflare quota, a transient container-API error) -- since this function
+    // always rethrows rather than returning on a step failure, secretRef would otherwise never reach the caller
+    // to persist and later revoke, permanently orphaning a live, exchangeable credential in the broker (this
+    // was unreachable before #8202: injectSecrets used to be the LAST step, so nothing after it could fail once
+    // secretRef was set). Best-effort revoke it here, before rethrowing, so this function cleans up after
+    // itself rather than counting on a caller that has no way to know the secret exists. Swallowed like
+    // onFailure below: a revoke failure (e.g. broker unreachable) must never mask the real provisioning error --
+    // that's exactly why the PagerDuty alert below still carries secretRef, as an operator's last resort.
+    if (secretRef !== undefined) {
+      await driver.revokeSecrets({ ...request, secretRef }).catch((revokeError: unknown) => {
+        console.warn(
+          JSON.stringify({
+            event: "provisioning_orphaned_secret_revoke_failed",
+            tenant: tenant.name,
+            product,
+            secretRef,
+            message: pagerDutyFailMessage(revokeError),
+          }),
+        );
+      });
+    }
     // #7677 (ratified 2026-07-21): give the caller its chance to transition the tenant's registry record to
     // "failed" BEFORE the rethrow, so a customer polling the read path sees a terminal "Setup failed" instead
     // of a record stuck at "provisioning" forever. Best-effort by design: a failure writing the failed state
     // must never mask the provisioning error itself, which still pages and rethrows exactly as before.
     if (onFailure) await onFailure().catch(() => undefined);
-    pageAndRethrow(tenant, product, "provision", error, pagerDuty);
+    pageAndRethrow(tenant, product, "provision", error, pagerDuty, secretRef);
   }
   return { tenant, product, state: "active", database, ...(secretRef !== undefined ? { secretRef } : {}) };
 }
@@ -141,7 +168,9 @@ export async function deprovisionTenant(
     await driver.dropDatabase(request);
     await driver.destroyContainer(request);
   } catch (error) {
-    pageAndRethrow(tenant, product, "deprovision", error, pagerDuty);
+    // secretRef is already known here (the caller's own input, not something this function minted) -- passed
+    // along so an operator paged for a deprovision failure doesn't have to go look it up separately.
+    pageAndRethrow(tenant, product, "deprovision", error, pagerDuty, secretRef);
   }
   return { tenant, product, state: "torn down" };
 }

control-plane/test/pagerduty-notify.test.ts

@@ -43,6 +43,18 @@ test("buildProvisioningPagerDutyAlert: provision failure builds a critical alert
   });
 });
 
+test("buildProvisioningPagerDutyAlert: includes secretRef in customDetails when given (#8202)", () => {
+  const alert = buildProvisioningPagerDutyAlert({
+    tenantName: "acme",
+    product: "orb",
+    phase: "provision",
+    error: new Error("container quota exceeded"),
+    secretRef: "orbenr_abc",
+  });
+
+  assert.equal(alert.customDetails.secretRef, "orbenr_abc");
+});
+
 test("buildProvisioningPagerDutyAlert: deprovision failure coerces a non-Error thrown value (#7667)", () => {
   const alert = buildProvisioningPagerDutyAlert({
     tenantName: "acme",

control-plane/test/provisioning-pagerduty.test.ts

@@ -12,6 +12,7 @@ import {
   type ProvisioningPagerDutyAlert,
   type Tenant,
   type TenantProvisioningDriver,
+  type TenantProvisioningRequest,
 } from "../dist/index.js";
 
 /** A driver where exactly one named step throws `error`; every other step is a no-op success. */
@@ -153,3 +154,122 @@ test("deprovisionTenant defaults to the real notifyProvisioningFailure + process
 
   await assert.rejects(deprovisionTenant(tenant, "ams", driver), /db drop failed/);
 });
+
+// #8202: injectSecrets moved ahead of createContainer, so secretRef can now be minted and THEN orphaned if
+// createContainer fails right after -- provisionTenant always rethrows rather than returning, so no caller ever
+// gets secretRef to persist and revoke later otherwise. These prove the fix: a best-effort self-revoke, safe
+// even when that revoke itself fails, and correctly scoped to only fire once a real secretRef actually exists.
+
+test("#8202: provisionTenant best-effort revokes the just-injected secret when createContainer fails right after, before rethrowing", async () => {
+  const revokeCalls: TenantProvisioningRequest[] = [];
+  const driver: TenantProvisioningDriver = {
+    ...createFakeTenantProvisioningDriver(),
+    injectSecrets: async () => ({ secretRef: "orbenr_abc", bootstrapSecret: "orbsec_xyz" }),
+    createContainer: async () => {
+      throw new Error("container quota exceeded");
+    },
+    revokeSecrets: async (request) => {
+      revokeCalls.push(request);
+    },
+  };
+  const tenant: Tenant = { name: "acme" };
+
+  await assert.rejects(provisionTenant(tenant, "orb", driver), /container quota exceeded/);
+
+  assert.equal(revokeCalls.length, 1);
+  assert.equal(revokeCalls[0]?.secretRef, "orbenr_abc");
+});
+
+test("#8202: a failure in the best-effort revoke itself does not mask the real createContainer error", async () => {
+  const driver: TenantProvisioningDriver = {
+    ...createFakeTenantProvisioningDriver(),
+    injectSecrets: async () => ({ secretRef: "orbenr_abc" }),
+    createContainer: async () => {
+      throw new Error("container quota exceeded");
+    },
+    revokeSecrets: async () => {
+      throw new Error("broker unreachable");
+    },
+  };
+  const tenant: Tenant = { name: "acme" };
+
+  await assert.rejects(provisionTenant(tenant, "orb", driver), /container quota exceeded/);
+});
+
+test("#8202: provisionTenant does NOT attempt a revoke when no secretRef was ever obtained (e.g. provisionDatabase itself failed)", async () => {
+  const revokeCalls: TenantProvisioningRequest[] = [];
+  const driver: TenantProvisioningDriver = {
+    ...driverThatThrowsOn("provisionDatabase", new Error("db provisioning failed")),
+    revokeSecrets: async (request) => {
+      revokeCalls.push(request);
+    },
+  };
+  const tenant: Tenant = { name: "acme" };
+
+  await assert.rejects(provisionTenant(tenant, "orb", driver), /db provisioning failed/);
+
+  assert.equal(revokeCalls.length, 0);
+});
+
+test("#8202: provisionTenant does NOT attempt a revoke when injectSecrets itself is the step that failed", async () => {
+  const revokeCalls: TenantProvisioningRequest[] = [];
+  const driver: TenantProvisioningDriver = {
+    ...driverThatThrowsOn("injectSecrets", new Error("secret injection failed")),
+    revokeSecrets: async (request) => {
+      revokeCalls.push(request);
+    },
+  };
+  const tenant: Tenant = { name: "acme" };
+
+  await assert.rejects(provisionTenant(tenant, "orb", driver), /secret injection failed/);
+
+  assert.equal(revokeCalls.length, 0);
+});
+
+test("#8202: the PagerDuty alert carries secretRef when injectSecrets had already succeeded before the failing step", async () => {
+  const calls: ProvisioningPagerDutyAlert[] = [];
+  const notify: NotifyProvisioningFailure = async (alert) => {
+    calls.push(alert);
+  };
+  const driver: TenantProvisioningDriver = {
+    ...createFakeTenantProvisioningDriver(),
+    injectSecrets: async () => ({ secretRef: "orbenr_abc" }),
+    createContainer: async () => {
+      throw new Error("container quota exceeded");
+    },
+  };
+  const tenant: Tenant = { name: "acme" };
+
+  await assert.rejects(provisionTenant(tenant, "orb", driver, { notify }), /container quota exceeded/);
+  await Promise.resolve();
+
+  assert.equal(calls[0]?.customDetails.secretRef, "orbenr_abc");
+});
+
+test("#8202: the PagerDuty alert omits secretRef entirely when none was ever obtained", async () => {
+  const calls: ProvisioningPagerDutyAlert[] = [];
+  const notify: NotifyProvisioningFailure = async (alert) => {
+    calls.push(alert);
+  };
+  const driver = driverThatThrowsOn("provisionDatabase", new Error("db provisioning failed"));
+  const tenant: Tenant = { name: "acme" };
+
+  await assert.rejects(provisionTenant(tenant, "orb", driver, { notify }), /db provisioning failed/);
+  await Promise.resolve();
+
+  assert.equal("secretRef" in (calls[0]?.customDetails ?? {}), false);
+});
+
+test("#8202: deprovisionTenant's PagerDuty alert carries the secretRef it was given, for operator convenience", async () => {
+  const calls: ProvisioningPagerDutyAlert[] = [];
+  const notify: NotifyProvisioningFailure = async (alert) => {
+    calls.push(alert);
+  };
+  const driver = driverThatThrowsOn("dropDatabase", new Error("db drop failed"));
+  const tenant: Tenant = { name: "acme" };
+
+  await assert.rejects(deprovisionTenant(tenant, "ams", driver, { notify }, "orbenr_abc"), /db drop failed/);
+  await Promise.resolve();
+
+  assert.equal(calls[0]?.customDetails.secretRef, "orbenr_abc");
+});