diff --git a/src/api/routes.ts b/src/api/routes.ts index 827ed51fdc..951a95d78d 100644 --- a/src/api/routes.ts +++ b/src/api/routes.ts @@ -1986,15 +1986,44 @@ export function createApp() { // longer prunes itself (see its own doc comment) precisely so a 500-installation fan-out below can't turn // into 500 redundant global TTL-prune scans/deletes against the shared orb_relay_pending table. await pruneRelayPending(c.env); - await Promise.all(installationIds.map((installationId) => enqueueConfigPushRelay(c.env, installationId, payload))); + // #8880: isolate each per-installation enqueue. A bare Promise.all let one target's throw abort the whole + // fan-out -- the audit event AND the response for the other ~499 installations were silently dropped, and + // there was no route-level try/catch to salvage them. Catch each target's failure inside its own fan-out + // task (mirroring pollPendingAprRepoTransfers' per-item isolation in src/orb/apr-repo-transfer.ts) so one + // bad row can't sink the batch, audit each failure so it is never silently swallowed, and still report the + // successful targets plus the partial failure to the caller. + const settled = await Promise.all( + installationIds.map(async (installationId): Promise<{ installationId: number; error?: string }> => { + try { + await enqueueConfigPushRelay(c.env, installationId, payload); + return { installationId }; + } catch (error) { + return { installationId, error: errorMessage(error) }; + } + }), + ); + const failedInstallationIds: number[] = []; + for (const result of settled) { + if (result.error !== undefined) { + failedInstallationIds.push(result.installationId); + await recordAuditEvent(c.env, { + eventType: "operator.config_push_target_failed", + actor: identity.actor, + targetKey: `config_push#${parsed.data.pushId}#${result.installationId}`, + outcome: "error", + metadata: { installationId: result.installationId, pushId: parsed.data.pushId, message: result.error }, + }); + } + } + const succeededCount = installationIds.length - failedInstallationIds.length; await recordAuditEvent(c.env, { eventType: "operator.config_push_enqueued", actor: identity.actor, targetKey: `config_push#${parsed.data.pushId}`, - outcome: "completed", - metadata: { installationCount: installationIds.length, capability: payload.capability ?? null }, + outcome: failedInstallationIds.length > 0 ? "error" : "completed", + metadata: { installationCount: installationIds.length, succeededCount, failedCount: failedInstallationIds.length, capability: payload.capability ?? null }, }); - return c.json({ ok: true, pushId: parsed.data.pushId, installationCount: installationIds.length }); + return c.json({ ok: true, pushId: parsed.data.pushId, installationCount: installationIds.length, succeededCount, failedInstallationIds }); }); // #5672 post-merge incident report, internal-operator side: same reporting path as the repo-scoped customer diff --git a/test/unit/routes-config-push.test.ts b/test/unit/routes-config-push.test.ts index 5c61830bac..a285dee854 100644 --- a/test/unit/routes-config-push.test.ts +++ b/test/unit/routes-config-push.test.ts @@ -2,12 +2,12 @@ import { describe, expect, it, vi } from "vitest"; vi.mock("../../src/orb/relay", async (importOriginal) => { const actual = await importOriginal(); - return { ...actual, pruneRelayPending: vi.fn(actual.pruneRelayPending) }; + return { ...actual, pruneRelayPending: vi.fn(actual.pruneRelayPending), enqueueConfigPushRelay: vi.fn(actual.enqueueConfigPushRelay) }; }); import { createApp } from "../../src/api/routes"; import { createSessionForGitHubUser } from "../../src/auth/security"; -import { pruneRelayPending } from "../../src/orb/relay"; +import { enqueueConfigPushRelay, pruneRelayPending } from "../../src/orb/relay"; import { createTestEnv } from "../helpers/d1"; // #7522 (piece 1 of #4902's 3-piece design): the config-push write path. Mirrors routes-kill-switch.test.ts's @@ -63,7 +63,7 @@ describe("config-push operator route (#7522)", () => { env, ); expect(res.status).toBe(200); - await expect(res.json()).resolves.toEqual({ ok: true, pushId: "push-1", installationCount: 2 }); + await expect(res.json()).resolves.toEqual({ ok: true, pushId: "push-1", installationCount: 2, succeededCount: 2, failedInstallationIds: [] }); const rows = await relayRows(env); expect(rows).toHaveLength(2); @@ -164,6 +164,58 @@ describe("config-push operator route (#7522)", () => { .first()) as { actor: string; outcome: string; metadata_json: string } | null; expect(audit?.actor).toBe("jsonbored"); expect(audit?.outcome).toBe("completed"); - expect(JSON.parse(audit?.metadata_json ?? "{}")).toEqual({ installationCount: 1, capability: null }); + expect(JSON.parse(audit?.metadata_json ?? "{}")).toEqual({ installationCount: 1, succeededCount: 1, failedCount: 0, capability: null }); + }); + + it("isolates a per-installation failure (#8880): the other targets still enqueue and the response + audit report the partial failure", async () => { + const app = createApp(); + const env = createTestEnv(); + const relay = await vi.importActual("../../src/orb/relay"); + // One target's DB write throws; every other target must still land, and the failure must be audited, not + // swallowed. Restore the real implementation afterwards so later tests keep exercising the genuine enqueue. + vi.mocked(enqueueConfigPushRelay).mockImplementation(async (e, installationId, p) => { + if (installationId === 222) throw new Error("simulated D1 write failure for 222"); + await relay.enqueueConfigPushRelay(e, installationId, p); + }); + try { + const res = await app.request( + "/v1/app/fleet/config-push", + { + method: "POST", + headers: apiHeaders(env), + body: JSON.stringify({ installationIds: [111, 222, 333], pushId: "push-fail", message: "x", capability: "y" }), + }, + env, + ); + expect(res.status).toBe(200); + await expect(res.json()).resolves.toEqual({ + ok: true, + pushId: "push-fail", + installationCount: 3, + succeededCount: 2, + failedInstallationIds: [222], + }); + + // The two healthy targets still landed their rows -- one bad installation didn't sink the batch. + const rows = await relayRows(env); + expect(rows.map((r) => r.installation_id)).toEqual([111, 333]); + + // The failing target was audited per-installation rather than silently dropped. + const failAudit = (await env.DB + .prepare("select target_key, outcome, metadata_json from audit_events where event_type = 'operator.config_push_target_failed'") + .first()) as { target_key: string; outcome: string; metadata_json: string } | null; + expect(failAudit?.outcome).toBe("error"); + expect(failAudit?.target_key).toBe("config_push#push-fail#222"); + expect(JSON.parse(failAudit?.metadata_json ?? "{}")).toEqual({ installationId: 222, pushId: "push-fail", message: "simulated D1 write failure for 222" }); + + // The whole-push summary audit still fired and reports the partial failure (outcome 'error', counts split). + const summary = (await env.DB + .prepare("select outcome, metadata_json from audit_events where event_type = 'operator.config_push_enqueued'") + .first()) as { outcome: string; metadata_json: string } | null; + expect(summary?.outcome).toBe("error"); + expect(JSON.parse(summary?.metadata_json ?? "{}")).toEqual({ installationCount: 3, succeededCount: 2, failedCount: 1, capability: "y" }); + } finally { + vi.mocked(enqueueConfigPushRelay).mockImplementation(relay.enqueueConfigPushRelay); + } }); });