Skip to content

refactor(selfhost): remove or wire the dead checkAndMarkDelivery helper #2572

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
JSONbored merged 1 commit into from
Jul 2, 2026
Merged

refactor(selfhost): remove or wire the dead checkAndMarkDelivery helper #2572

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 0 additions & 18 deletions src/selfhost/redis-cache.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,21 +27,3 @@ export function createRedisCache(redis: Redis) {
}

export type RedisCache = ReturnType<typeof createRedisCache>;

/**
* Idempotency check for GitHub webhook deliveries. Returns true if the delivery was
* already seen (caller should short-circuit with 204). Marks the delivery as seen
* for `ttlSeconds` (default 5 min — covers GitHub's retry window) on the FIRST call.
* Best-effort: a Redis error is swallowed to avoid blocking webhook processing.
*/
export async function checkAndMarkDelivery(cache: RedisCache, deliveryId: string, ttlSeconds = 300): Promise<boolean> {
try {
const seen = await cache.get(`delivery:${deliveryId}`);
if (seen) return true;
await cache.set(`delivery:${deliveryId}`, "1", ttlSeconds);
return false;
} catch {
// Redis unavailable → treat as first-time (never block processing on cache failure)
return false;
}
}
37 changes: 1 addition & 36 deletions test/unit/selfhost-redis-cache.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import type { Redis } from "ioredis";
import { describe, expect, it } from "vitest";
import { checkAndMarkDelivery, createRedisCache } from "../../src/selfhost/redis-cache";
import { createRedisCache } from "../../src/selfhost/redis-cache";

/** 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
Expand Down Expand Up @@ -64,38 +64,3 @@ describe("createRedisCache (#1216 webhook dedup cache)", () => {
await expect(cache.claim("lock", "1", 60)).rejects.toThrow("connection refused");
});
});

describe("checkAndMarkDelivery (#1216 webhook idempotency)", () => {
it("returns false (first-time) for a new delivery ID and marks it as seen", async () => {
const cache = createRedisCache(fakeRedis());
const result = await checkAndMarkDelivery(cache, "delivery-abc", 300);
expect(result).toBe(false);
// second call with the same ID should be a duplicate
const duplicate = await checkAndMarkDelivery(cache, "delivery-abc", 300);
expect(duplicate).toBe(true);
});

it("returns true (duplicate) for an already-seen delivery ID", async () => {
const r = fakeRedis();
r._store.set("delivery:existing-id", "1");
const cache = createRedisCache(r);
expect(await checkAndMarkDelivery(cache, "existing-id")).toBe(true);
});

it("different delivery IDs are tracked independently", async () => {
const cache = createRedisCache(fakeRedis());
expect(await checkAndMarkDelivery(cache, "id-A")).toBe(false);
expect(await checkAndMarkDelivery(cache, "id-B")).toBe(false); // different ID → first-time
expect(await checkAndMarkDelivery(cache, "id-A")).toBe(true); // id-A seen before
});

it("swallows Redis errors and returns false (never blocks processing)", async () => {
const brokenRedis = {
async get() { throw new Error("connection refused"); },
async set() { throw new Error("connection refused"); },
} as unknown as Redis;
const cache = createRedisCache(brokenRedis);
// Must not throw — error is swallowed, returns false (first-time / let it through)
expect(await checkAndMarkDelivery(cache, "any-id")).toBe(false);
});
});
Loading