From 42d0365b5c8681cc25f4ca3a056aacc0211be104 Mon Sep 17 00:00:00 2001 From: joaovictor91123 Date: Fri, 24 Jul 2026 09:57:56 -0300 Subject: [PATCH 1/2] fix(orb): guard finalizeRelayFailureRetryRow's DB writes against duplicate redelivery (#8332) --- src/orb/relay.ts | 28 +++++++++++++---- test/integration/orb-relay.test.ts | 50 ++++++++++++++++++++++++++++++ 2 files changed, 72 insertions(+), 6 deletions(-) diff --git a/src/orb/relay.ts b/src/orb/relay.ts index e7c035671..924b63e7f 100644 --- a/src/orb/relay.ts +++ b/src/orb/relay.ts @@ -106,14 +106,30 @@ async function finalizeRelayFailureRetryRow( row: { delivery_id: string; event_name: string; installation_id: number }, outcome: RelayForwardOutcome, ): Promise { - if (isRelayFailureRetryTerminal(outcome, row.event_name)) { - await env.DB.prepare("DELETE FROM orb_relay_failures WHERE delivery_id = ?").bind(row.delivery_id).run(); + try { + if (isRelayFailureRetryTerminal(outcome, row.event_name)) { + await env.DB.prepare("DELETE FROM orb_relay_failures WHERE delivery_id = ?").bind(row.delivery_id).run(); + return; + } + await env.DB + .prepare("UPDATE orb_relay_failures SET attempts = attempts + 1, last_attempt_at = datetime('now') WHERE delivery_id = ?") + .bind(row.delivery_id) + .run(); + } catch (error) { + // The forward already succeeded (outcome is known) before this write failed -- the row stays pending retry, so + // the SAME event risks redelivery to the container on the next retry tick. Never throw (retryFailedRelays's + // "Never throws" contract): log with enough context to spot the duplicate-forward risk from the log alone. + console.error(JSON.stringify({ + level: "error", + event: "orb_relay_failure_finalize_write_failed", + message: `finalizeRelayFailureRetryRow DB write failed after a successful forward -- duplicate redelivery risk for ${row.delivery_id}`, + deliveryId: row.delivery_id, + eventName: row.event_name, + outcome, + error: error instanceof Error ? error.message : String(error), + })); return; } - await env.DB - .prepare("UPDATE orb_relay_failures SET attempts = attempts + 1, last_attempt_at = datetime('now') WHERE delivery_id = ?") - .bind(row.delivery_id) - .run(); if (outcome === "skipped") { logRelayTransientSkip({ deliveryId: row.delivery_id, diff --git a/test/integration/orb-relay.test.ts b/test/integration/orb-relay.test.ts index 9b6d8f9dd..8afc726af 100644 --- a/test/integration/orb-relay.test.ts +++ b/test/integration/orb-relay.test.ts @@ -402,6 +402,56 @@ describe("retryFailedRelays", () => { expect(row ?? null).toBeNull(); // row removed on success }); + it("does not throw when the finalize DELETE fails right after a successful forward (#8332 duplicate-redelivery guard)", async () => { + const e = brokeredEnv(); + const secret = await enroll(e, 9700); + await registerOrbRelay(e, secret, "https://c.example/v1/orb/relay"); + await storeRelayFailure(e, { deliveryId: "finalize-delete-fail", eventName: "pull_request", installationId: 9700, rawBody: "{}" }); + const realPrepare = db(e).prepare.bind(db(e)); + const errorLog = vi.spyOn(console, "error").mockImplementation(() => undefined); + e.DB = { + prepare: (sql: string) => + sql === "DELETE FROM orb_relay_failures WHERE delivery_id = ?" + ? { bind: () => ({ run: async () => { throw new Error("simulated D1 write failure"); } }) } + : realPrepare(sql), + } as unknown as Env["DB"]; + const fetchOk = (() => Promise.resolve(new Response("ok", { status: 200 }))) as typeof fetch; + await expect(retryFailedRelays(e, { fetchImpl: fetchOk })).resolves.toBeUndefined(); // "Never throws" contract holds + expect( + errorLog.mock.calls.some( + ([line]) => + String(line).includes("orb_relay_failure_finalize_write_failed") && + String(line).includes("finalize-delete-fail") && + String(line).includes("forwarded"), + ), + ).toBe(true); + }); + + it("does not throw when the finalize UPDATE fails right after a non-terminal forward (#8332 duplicate-redelivery guard)", async () => { + const e = brokeredEnv(); + const secret = await enroll(e, 9701); + await registerOrbRelay(e, secret, "https://c.example/v1/orb/relay"); + await storeRelayFailure(e, { deliveryId: "finalize-update-fail", eventName: "pull_request", installationId: 9701, rawBody: "{}" }); + const realPrepare = db(e).prepare.bind(db(e)); + const errorLog = vi.spyOn(console, "error").mockImplementation(() => undefined); + e.DB = { + prepare: (sql: string) => + sql === "UPDATE orb_relay_failures SET attempts = attempts + 1, last_attempt_at = datetime('now') WHERE delivery_id = ?" + ? { bind: () => ({ run: async () => { throw new Error("simulated D1 write failure"); } }) } + : realPrepare(sql), + } as unknown as Env["DB"]; + const fetchFail = (() => Promise.resolve(new Response("bad", { status: 503 }))) as typeof fetch; + await expect(retryFailedRelays(e, { fetchImpl: fetchFail })).resolves.toBeUndefined(); // "Never throws" contract holds + expect( + errorLog.mock.calls.some( + ([line]) => + String(line).includes("orb_relay_failure_finalize_write_failed") && + String(line).includes("finalize-update-fail") && + String(line).includes("failed"), + ), + ).toBe(true); + }); + it("INCREMENTS attempts when forwardOrbEvent still fails", async () => { const e = brokeredEnv(); const secret = await enroll(e, 9101); From 28abef7ca198ae7d5038683f65839167379953f9 Mon Sep 17 00:00:00 2001 From: joaovictor91123 Date: Fri, 24 Jul 2026 11:02:03 -0300 Subject: [PATCH 2/2] test(orb): cover the String(error) fallback branch in finalizeRelayFailureRetryRow's log (#8332) --- test/integration/orb-relay.test.ts | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/test/integration/orb-relay.test.ts b/test/integration/orb-relay.test.ts index 8afc726af..f7e1c0d02 100644 --- a/test/integration/orb-relay.test.ts +++ b/test/integration/orb-relay.test.ts @@ -410,9 +410,11 @@ describe("retryFailedRelays", () => { const realPrepare = db(e).prepare.bind(db(e)); const errorLog = vi.spyOn(console, "error").mockImplementation(() => undefined); e.DB = { + // Deliberately a non-Error throw here, to cover the `String(error)` fallback branch of the ternary + // (the DELETE-path regression test below covers the `error instanceof Error` branch instead). prepare: (sql: string) => sql === "DELETE FROM orb_relay_failures WHERE delivery_id = ?" - ? { bind: () => ({ run: async () => { throw new Error("simulated D1 write failure"); } }) } + ? { bind: () => ({ run: () => Promise.reject("a bare string rejection") }) } : realPrepare(sql), } as unknown as Env["DB"]; const fetchOk = (() => Promise.resolve(new Response("ok", { status: 200 }))) as typeof fetch; @@ -422,7 +424,8 @@ describe("retryFailedRelays", () => { ([line]) => String(line).includes("orb_relay_failure_finalize_write_failed") && String(line).includes("finalize-delete-fail") && - String(line).includes("forwarded"), + String(line).includes("forwarded") && + String(line).includes('"error":"a bare string rejection"'), ), ).toBe(true); }); @@ -435,6 +438,8 @@ describe("retryFailedRelays", () => { const realPrepare = db(e).prepare.bind(db(e)); const errorLog = vi.spyOn(console, "error").mockImplementation(() => undefined); e.DB = { + // A real Error instance here, to cover the `error instanceof Error` (true) branch of the ternary + // (the DELETE-path test above covers the non-Error `String(error)` fallback branch instead). prepare: (sql: string) => sql === "UPDATE orb_relay_failures SET attempts = attempts + 1, last_attempt_at = datetime('now') WHERE delivery_id = ?" ? { bind: () => ({ run: async () => { throw new Error("simulated D1 write failure"); } }) } @@ -447,7 +452,8 @@ describe("retryFailedRelays", () => { ([line]) => String(line).includes("orb_relay_failure_finalize_write_failed") && String(line).includes("finalize-update-fail") && - String(line).includes("failed"), + String(line).includes("failed") && + String(line).includes('"error":"simulated D1 write failure"'), ), ).toBe(true); });