From a1807f6165c5d2b90127a6c84ce711b5633be0c4 Mon Sep 17 00:00:00 2001 From: jony376 Date: Sun, 5 Jul 2026 09:32:56 -0700 Subject: [PATCH] feat(metrics): count Redis webhook dedup hits with backend label Expose isWebhookDeliveryDuplicate/rememberWebhookDelivery helpers and wire server dedup through them so redeliveries increment gittensory_webhook_dedup_total{backend="redis"} without shifting selfhost env-reference line numbers. Closes #2075 Co-authored-by: Cursor --- src/selfhost/redis-cache.ts | 32 ++++++++++++++++++++ src/server.ts | 18 +++++------ test/unit/selfhost-redis-cache.test.ts | 41 ++++++++++++++++++++++++-- 3 files changed, 80 insertions(+), 11 deletions(-) diff --git a/src/selfhost/redis-cache.ts b/src/selfhost/redis-cache.ts index 79dc10764b..6040fba15b 100644 --- a/src/selfhost/redis-cache.ts +++ b/src/selfhost/redis-cache.ts @@ -4,6 +4,38 @@ // ID after a successful processing attempt, the server can return 204 immediately on retries // without re-queuing the job. The self-host review runtime requires REDIS_URL. import type { Redis } from "ioredis"; +import { incr } from "./metrics"; + +const WEBHOOK_DELIVERY_CACHE_PREFIX = "delivery:"; + +export function webhookDeliveryCacheKey(deliveryId: string): string { + return `${WEBHOOK_DELIVERY_CACHE_PREFIX}${deliveryId}`; +} + +/** Returns true when this GitHub webhook delivery ID was already processed (Redis dedup hit). + * Increments `gittensory_webhook_dedup_total{backend="redis"}` on a hit. Does NOT mark the + * delivery — the caller marks only after a successful response (#2506 / #2572). */ +export async function isWebhookDeliveryDuplicate(cache: RedisCache, deliveryId: string): Promise { + try { + const seen = await cache.get(webhookDeliveryCacheKey(deliveryId)); + if (seen) { + incr("gittensory_webhook_dedup_total", { backend: "redis" }); + return true; + } + return false; + } catch { + return false; + } +} + +/** Best-effort: record a successfully processed webhook delivery for Redis dedup. */ +export async function rememberWebhookDelivery(cache: RedisCache, deliveryId: string, ttlSeconds = 300): Promise { + try { + await cache.set(webhookDeliveryCacheKey(deliveryId), "1", ttlSeconds); + } catch { + // best-effort — never block the response on a cache write failure + } +} export function createRedisCache(redis: Redis) { return { diff --git a/src/server.ts b/src/server.ts index 9f5a3018bb..7d707e515c 100644 --- a/src/server.ts +++ b/src/server.ts @@ -481,7 +481,7 @@ async function main(): Promise { const { Redis } = await import("ioredis"); const redisClient = new Redis(redisUrl); const { createRedisRateLimiter } = await import("./selfhost/redis-ratelimit"); - const { createRedisCache, assertSelfhostTransientCacheOwnershipRelease } = await import("./selfhost/redis-cache"); + const { createRedisCache, assertSelfhostTransientCacheOwnershipRelease, isWebhookDeliveryDuplicate, rememberWebhookDelivery } = await import("./selfhost/redis-cache"); const rateLimiter = createRedisRateLimiter(redisClient); const webhookCache = createRedisCache(redisClient); assertSelfhostTransientCacheOwnershipRelease(webhookCache); @@ -706,7 +706,7 @@ async function main(): Promise { "gittensory_orb_events_exported_total", "gittensory_orb_export_errors_total", ]) - incr(c, undefined, 0); + incr(c, c === "gittensory_webhook_dedup_total" ? { backend: "redis" } : undefined, 0); // Seed gittensory_http_requests_total per status class so the breakdown panel has every series from the // first scrape (keeping the metric consistently labeled — never mix labeled and unlabeled samples). for (const status of ["2xx", "3xx", "4xx", "5xx"]) @@ -886,18 +886,18 @@ async function main(): Promise { ? request.headers.get("x-github-delivery") : null; if (deliveryId) { - const seen = await webhookCache!.get(`delivery:${deliveryId}`); - if (seen) { - incr("gittensory_webhook_dedup_total"); + // Redis dedup hit — return 204 before enqueue (#1216). + // Metric: gittensory_webhook_dedup_total{backend="redis"} (#2075). + if (await isWebhookDeliveryDuplicate(webhookCache!, deliveryId)) { return finish(new Response(null, { status: 204 })); } } const response = await worker.fetch(request, env, ctx); if (deliveryId && response.ok) { - // Best-effort — never block the response on a cache write failure - void webhookCache! - .set(`delivery:${deliveryId}`, "1", 300) - .catch(() => undefined); + // Best-effort — never block the response on a cache write failure. + void rememberWebhookDelivery(webhookCache!, deliveryId).catch( + () => undefined, + ); } return finish(response); } finally { diff --git a/test/unit/selfhost-redis-cache.test.ts b/test/unit/selfhost-redis-cache.test.ts index d143447771..f15fa1c520 100644 --- a/test/unit/selfhost-redis-cache.test.ts +++ b/test/unit/selfhost-redis-cache.test.ts @@ -1,6 +1,13 @@ import type { Redis } from "ioredis"; -import { describe, expect, it } from "vitest"; -import { assertSelfhostTransientCacheOwnershipRelease, createRedisCache } from "../../src/selfhost/redis-cache"; +import { afterEach, describe, expect, it } from "vitest"; +import { + assertSelfhostTransientCacheOwnershipRelease, + createRedisCache, + isWebhookDeliveryDuplicate, + rememberWebhookDelivery, + webhookDeliveryCacheKey, +} from "../../src/selfhost/redis-cache"; +import { renderMetrics, resetMetrics } from "../../src/selfhost/metrics"; /** Minimal in-memory stand-in for the ioredis methods the cache uses. Emulates real Redis SET NX * semantics (refuse + return null when NX is requested and the key already exists) so a test @@ -97,3 +104,33 @@ describe("createRedisCache (#1216 webhook dedup cache)", () => { expect(() => assertSelfhostTransientCacheOwnershipRelease(createRedisCache(fakeRedis()))).not.toThrow(); }); }); + +describe("isWebhookDeliveryDuplicate (#2075)", () => { + afterEach(() => resetMetrics()); + + it("returns false and does not increment on a first-time delivery", async () => { + const cache = createRedisCache(fakeRedis()); + await expect(isWebhookDeliveryDuplicate(cache, "delivery-1")).resolves.toBe(false); + expect(await renderMetrics()).not.toContain('gittensory_webhook_dedup_total{backend="redis"}'); + }); + + it("returns true and increments gittensory_webhook_dedup_total{backend=\"redis\"} when already seen", async () => { + const cache = createRedisCache(fakeRedis()); + await cache.set(webhookDeliveryCacheKey("delivery-2"), "1", 300); + await expect(isWebhookDeliveryDuplicate(cache, "delivery-2")).resolves.toBe(true); + expect(await renderMetrics()).toContain('gittensory_webhook_dedup_total{backend="redis"} 1'); + }); + + it("returns false without incrementing when Redis get throws", async () => { + const brokenRedis = { async get() { throw new Error("connection refused"); } } as unknown as Redis; + const cache = createRedisCache(brokenRedis); + await expect(isWebhookDeliveryDuplicate(cache, "delivery-3")).resolves.toBe(false); + expect(await renderMetrics()).not.toContain('gittensory_webhook_dedup_total{backend="redis"}'); + }); + + it("rememberWebhookDelivery stores the delivery key for later dedup", async () => { + const cache = createRedisCache(fakeRedis()); + await rememberWebhookDelivery(cache, "delivery-4"); + expect(await cache.get(webhookDeliveryCacheKey("delivery-4"))).toBe("1"); + }); +});