From 42e8f4bc7cda03775f3c7fb5a81301276a0b366b Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Mon, 6 Jul 2026 06:59:59 -0700 Subject: [PATCH] fix(observability): close two zero-trace webhook/relay loss points enqueueWebhookByEnv attempted JSON.parse before writing any webhook_events row, so an unparseable delivery left zero durable trace anywhere -- indistinguishable from GitHub never having sent it. Hash the raw body before the parse attempt and record an "error" row on a parse failure so every delivery is traceable. drainOrbRelay's pull-mode batch parser silently filtered out any relayed event missing/mistyping one of its three required fields, with no log and no counter. Log a structured, Sentry-visible event and increment a counter on the drop, naming which field was missing. Part of #3812; the new fast open-PR reconciliation job is tracked separately. --- src/github/webhook.ts | 7 ++++++- src/orb/broker-client.ts | 15 +++++++++++++++ test/unit/orb-broker-client.test.ts | 12 ++++++++++-- test/unit/webhook.test.ts | 11 +++++++++++ 4 files changed, 42 insertions(+), 3 deletions(-) diff --git a/src/github/webhook.ts b/src/github/webhook.ts index ab6eb57b9b..2c124f148d 100644 --- a/src/github/webhook.ts +++ b/src/github/webhook.ts @@ -142,15 +142,20 @@ export async function enqueueWebhookByEnv(env: Env, deliveryId: string, eventNam return "review_unavailable"; } + // #zero-trace-webhook-loss: hash the raw body (independent of whether it parses) BEFORE the parse attempt, so + // an unparseable delivery can still be durably recorded below instead of vanishing with no row anywhere. + const payloadHash = await sha256Hex(rawBody); let payload: GitHubWebhookPayload; try { payload = JSON.parse(rawBody) as GitHubWebhookPayload; } catch { + // installation/repository/action are unknown pre-parse; deliveryId + eventName + the hash are enough for an + // operator to trace this delivery instead of it being indistinguishable from "GitHub never sent it." + await recordWebhookEvent(env, { deliveryId, eventName, payloadHash, status: "error", errorSummary: "invalid_json" }); recordWebhookEnqueueMetric(eventName, undefined, "invalid_json"); return "invalid_json"; } - const payloadHash = await sha256Hex(rawBody); const existingEvent = await getWebhookEvent(env, deliveryId); // Suppress redelivery of an already-processed event (on success its payloadHash is overwritten to a // "processed" sentinel, so a hash match alone misses it and the event re-runs its side effects) or one diff --git a/src/orb/broker-client.ts b/src/orb/broker-client.ts index 8e39d5e8ca..003e599bf6 100644 --- a/src/orb/broker-client.ts +++ b/src/orb/broker-client.ts @@ -7,6 +7,8 @@ // The signal is the ENROLLMENT SECRET's presence: a brokered self-host sets ORB_ENROLLMENT_SECRET (issued by the // operator), cloud never does — so this path is inert on cloud and the deploy is byte-identical there. +import { incr } from "../selfhost/metrics"; + /** The Orb's hosted broker base; override (ORB_BROKER_URL) only to point at a private gittensory deployment. */ const DEFAULT_BROKER_URL = "https://gittensory-api.aethereal.dev"; // The broker's cold token mint can take many seconds when GitHub is throttling the App; allow headroom so the one @@ -256,7 +258,20 @@ export async function drainOrbRelay( for (const e of body.events ?? []) { if (typeof e.deliveryId === "string" && typeof e.eventName === "string" && typeof e.rawBody === "string") { out.push({ deliveryId: e.deliveryId, eventName: e.eventName, rawBody: e.rawBody }); + continue; } + // #zero-trace-webhook-loss: a batch entry missing/mistyping one of the three required fields was + // previously discarded with no record anywhere — indistinguishable from the Orb never having relayed it. + incr("gittensory_orb_relay_malformed_events_total"); + console.error( + JSON.stringify({ + level: "error", + event: "orb_relay_malformed_event_dropped", + hasDeliveryId: typeof e.deliveryId === "string", + hasEventName: typeof e.eventName === "string", + hasRawBody: typeof e.rawBody === "string", + }), + ); } return out; } catch (error) { diff --git a/test/unit/orb-broker-client.test.ts b/test/unit/orb-broker-client.test.ts index 8d33e028de..15207e03c6 100644 --- a/test/unit/orb-broker-client.test.ts +++ b/test/unit/orb-broker-client.test.ts @@ -1,4 +1,5 @@ -import { describe, expect, it } from "vitest"; +import { describe, expect, it, vi } from "vitest"; +import { counterValue, resetMetrics } from "../../src/selfhost/metrics"; import { createOrbRelayRegistrationState, drainOrbRelay, @@ -362,7 +363,9 @@ describe("drainOrbRelay (pull-mode drain)", () => { expect(await drainOrbRelay({})).toEqual([]); }); - it("POSTs the ack list, parses returned events, and filters malformed ones", async () => { + it("POSTs the ack list, parses returned events, and filters malformed ones (#zero-trace-webhook-loss: logs + counts the drop)", async () => { + resetMetrics(); + const errors = vi.spyOn(console, "error").mockImplementation(() => undefined); const { fetchImpl, calls } = captureFetch( Response.json({ events: [ @@ -380,6 +383,11 @@ describe("drainOrbRelay (pull-mode drain)", () => { expect(calls[0]?.url).toBe("https://gittensory-api.aethereal.dev/v1/orb/relay/pull"); expect((calls[0]?.init?.headers as Record).authorization).toBe("Bearer s"); expect(JSON.parse(String(calls[0]?.init?.body))).toEqual({ ack: ["prev-1"] }); + expect(counterValue("gittensory_orb_relay_malformed_events_total")).toBe(1); + const logged = errors.mock.calls.map((c) => String(c[0])).find((line) => line.includes("orb_relay_malformed_event_dropped")); + expect(logged).toBeDefined(); + expect(JSON.parse(logged!)).toMatchObject({ level: "error", event: "orb_relay_malformed_event_dropped", hasDeliveryId: true, hasEventName: true, hasRawBody: false }); + errors.mockRestore(); }); it("tolerates a missing events array (?? [] arm)", async () => { diff --git a/test/unit/webhook.test.ts b/test/unit/webhook.test.ts index 5f2e41d1a9..3438439a7e 100644 --- a/test/unit/webhook.test.ts +++ b/test/unit/webhook.test.ts @@ -285,6 +285,17 @@ describe("github webhook queue isolation (#audit-webhook-queue)", () => { expect(metrics).toContain('gittensory_webhook_enqueue_total{action="other",event="other",result="queued"} 1'); }); + it("REGRESSION (#zero-trace-webhook-loss): an unparseable delivery still gets a durable webhook_events row instead of vanishing with no trace", async () => { + const env = createTestEnv(); + env.WEBHOOKS = { send: async () => undefined } as unknown as Queue; + + await expect(enqueueWebhookByEnv(env, "invalid-json-trace", "pull_request", "{not json")).resolves.toBe("invalid_json"); + + const event = await getWebhookEvent(env, "invalid-json-trace"); + expect(event).toMatchObject({ deliveryId: "invalid-json-trace", status: "error" }); + expect(event?.payloadHash).toBeTruthy(); + }); + it("rejects retired direct review-app webhooks when the self-host review runtime is absent", async () => { const env = createTestEnv(); delete env.SELFHOST_TRANSIENT_CACHE;