Skip to content

Commit 42e8f4b

Browse files
committed
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.
1 parent ca6a6d7 commit 42e8f4b

4 files changed

Lines changed: 42 additions & 3 deletions

File tree

src/github/webhook.ts

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -142,15 +142,20 @@ export async function enqueueWebhookByEnv(env: Env, deliveryId: string, eventNam
142142
return "review_unavailable";
143143
}
144144

145+
// #zero-trace-webhook-loss: hash the raw body (independent of whether it parses) BEFORE the parse attempt, so
146+
// an unparseable delivery can still be durably recorded below instead of vanishing with no row anywhere.
147+
const payloadHash = await sha256Hex(rawBody);
145148
let payload: GitHubWebhookPayload;
146149
try {
147150
payload = JSON.parse(rawBody) as GitHubWebhookPayload;
148151
} catch {
152+
// installation/repository/action are unknown pre-parse; deliveryId + eventName + the hash are enough for an
153+
// operator to trace this delivery instead of it being indistinguishable from "GitHub never sent it."
154+
await recordWebhookEvent(env, { deliveryId, eventName, payloadHash, status: "error", errorSummary: "invalid_json" });
149155
recordWebhookEnqueueMetric(eventName, undefined, "invalid_json");
150156
return "invalid_json";
151157
}
152158

153-
const payloadHash = await sha256Hex(rawBody);
154159
const existingEvent = await getWebhookEvent(env, deliveryId);
155160
// Suppress redelivery of an already-processed event (on success its payloadHash is overwritten to a
156161
// "processed" sentinel, so a hash match alone misses it and the event re-runs its side effects) or one

src/orb/broker-client.ts

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,8 @@
77
// The signal is the ENROLLMENT SECRET's presence: a brokered self-host sets ORB_ENROLLMENT_SECRET (issued by the
88
// operator), cloud never does — so this path is inert on cloud and the deploy is byte-identical there.
99

10+
import { incr } from "../selfhost/metrics";
11+
1012
/** The Orb's hosted broker base; override (ORB_BROKER_URL) only to point at a private gittensory deployment. */
1113
const DEFAULT_BROKER_URL = "https://gittensory-api.aethereal.dev";
1214
// 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(
256258
for (const e of body.events ?? []) {
257259
if (typeof e.deliveryId === "string" && typeof e.eventName === "string" && typeof e.rawBody === "string") {
258260
out.push({ deliveryId: e.deliveryId, eventName: e.eventName, rawBody: e.rawBody });
261+
continue;
259262
}
263+
// #zero-trace-webhook-loss: a batch entry missing/mistyping one of the three required fields was
264+
// previously discarded with no record anywhere — indistinguishable from the Orb never having relayed it.
265+
incr("gittensory_orb_relay_malformed_events_total");
266+
console.error(
267+
JSON.stringify({
268+
level: "error",
269+
event: "orb_relay_malformed_event_dropped",
270+
hasDeliveryId: typeof e.deliveryId === "string",
271+
hasEventName: typeof e.eventName === "string",
272+
hasRawBody: typeof e.rawBody === "string",
273+
}),
274+
);
260275
}
261276
return out;
262277
} catch (error) {

test/unit/orb-broker-client.test.ts

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
1-
import { describe, expect, it } from "vitest";
1+
import { describe, expect, it, vi } from "vitest";
2+
import { counterValue, resetMetrics } from "../../src/selfhost/metrics";
23
import {
34
createOrbRelayRegistrationState,
45
drainOrbRelay,
@@ -362,7 +363,9 @@ describe("drainOrbRelay (pull-mode drain)", () => {
362363
expect(await drainOrbRelay({})).toEqual([]);
363364
});
364365

365-
it("POSTs the ack list, parses returned events, and filters malformed ones", async () => {
366+
it("POSTs the ack list, parses returned events, and filters malformed ones (#zero-trace-webhook-loss: logs + counts the drop)", async () => {
367+
resetMetrics();
368+
const errors = vi.spyOn(console, "error").mockImplementation(() => undefined);
366369
const { fetchImpl, calls } = captureFetch(
367370
Response.json({
368371
events: [
@@ -380,6 +383,11 @@ describe("drainOrbRelay (pull-mode drain)", () => {
380383
expect(calls[0]?.url).toBe("https://gittensory-api.aethereal.dev/v1/orb/relay/pull");
381384
expect((calls[0]?.init?.headers as Record<string, string>).authorization).toBe("Bearer s");
382385
expect(JSON.parse(String(calls[0]?.init?.body))).toEqual({ ack: ["prev-1"] });
386+
expect(counterValue("gittensory_orb_relay_malformed_events_total")).toBe(1);
387+
const logged = errors.mock.calls.map((c) => String(c[0])).find((line) => line.includes("orb_relay_malformed_event_dropped"));
388+
expect(logged).toBeDefined();
389+
expect(JSON.parse(logged!)).toMatchObject({ level: "error", event: "orb_relay_malformed_event_dropped", hasDeliveryId: true, hasEventName: true, hasRawBody: false });
390+
errors.mockRestore();
383391
});
384392

385393
it("tolerates a missing events array (?? [] arm)", async () => {

test/unit/webhook.test.ts

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -285,6 +285,17 @@ describe("github webhook queue isolation (#audit-webhook-queue)", () => {
285285
expect(metrics).toContain('gittensory_webhook_enqueue_total{action="other",event="other",result="queued"} 1');
286286
});
287287

288+
it("REGRESSION (#zero-trace-webhook-loss): an unparseable delivery still gets a durable webhook_events row instead of vanishing with no trace", async () => {
289+
const env = createTestEnv();
290+
env.WEBHOOKS = { send: async () => undefined } as unknown as Queue;
291+
292+
await expect(enqueueWebhookByEnv(env, "invalid-json-trace", "pull_request", "{not json")).resolves.toBe("invalid_json");
293+
294+
const event = await getWebhookEvent(env, "invalid-json-trace");
295+
expect(event).toMatchObject({ deliveryId: "invalid-json-trace", status: "error" });
296+
expect(event?.payloadHash).toBeTruthy();
297+
});
298+
288299
it("rejects retired direct review-app webhooks when the self-host review runtime is absent", async () => {
289300
const env = createTestEnv();
290301
delete env.SELFHOST_TRANSIENT_CACHE;

0 commit comments

Comments
 (0)