diff --git a/CHANGELOG.md b/CHANGELOG.md index a9b5122..ef896fb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,3 +4,4 @@ - Fix ClawHub appeals accepting client-supplied account context, and revalidate new and pending appeals against the authenticated GitHub applicant before unbanning. Thanks @SebTardif for the report and intake fix. - Reject crafted GitHub summary owner/repository paths before authenticated requests while preserving configured installation access. Thanks @SebTardif for the report and path validation fix. +- Avoid duplicate channel webhooks when concurrent automod events reach an empty cache in the same Worker instance. diff --git a/README.md b/README.md index 43aeafe..6240ad6 100644 --- a/README.md +++ b/README.md @@ -140,6 +140,10 @@ The Worker must have the matching public key: bunx wrangler secret put FORWARDER_PUBLIC_KEY ``` +## Automod webhooks + +Concurrent automod events in the same Worker instance share the channel webhook lookup and creation. Failed attempts are cleared so a later event can retry; successful webhooks retain the existing 15-minute cache. Separate Worker instances still manage their own caches. + ## Notes - Answer Overflow base URL is hardcoded to `https://www.answeroverflow.com`. diff --git a/src/utils/channelWebhook.ts b/src/utils/channelWebhook.ts index ec6e4fc..8434eaa 100644 --- a/src/utils/channelWebhook.ts +++ b/src/utils/channelWebhook.ts @@ -7,6 +7,7 @@ import { } from "@buape/carbon" const webhookCache = new Map() +const pendingWebhooks = new Map>() const webhookCacheTtlMs = 15 * 60 * 1000 const cleanupWebhookCache = () => { @@ -42,17 +43,30 @@ export const getOrCreateChannelWebhook = async ( return cached.webhook } - const existingWebhooks = await fetchChannelWebhooks(client, channelId) - const usableWebhook = existingWebhooks.find((webhook) => webhook.token) - const webhookData = usableWebhook ?? (await createChannelWebhook(client, channelId, name)) - - if (!webhookData.token) { - throw new Error("Webhook token missing for channel repost") + const pending = pendingWebhooks.get(channelId) + if (pending) { + return pending } - const webhook = new Webhook({ id: webhookData.id, token: webhookData.token }) - webhookCache.set(channelId, { webhook, fetchedAt: Date.now() }) - return webhook + const request = (async () => { + const existingWebhooks = await fetchChannelWebhooks(client, channelId) + const usableWebhook = existingWebhooks.find((webhook) => webhook.token) + const webhookData = usableWebhook ?? (await createChannelWebhook(client, channelId, name)) + + if (!webhookData.token) { + throw new Error("Webhook token missing for channel repost") + } + + const webhook = new Webhook({ id: webhookData.id, token: webhookData.token }) + webhookCache.set(channelId, { webhook, fetchedAt: Date.now() }) + return webhook + })() + pendingWebhooks.set(channelId, request) + try { + return await request + } finally { + pendingWebhooks.delete(channelId) + } } export const sendWebhookMessage = async ( diff --git a/tests/channelWebhook.test.ts b/tests/channelWebhook.test.ts new file mode 100644 index 0000000..e906030 --- /dev/null +++ b/tests/channelWebhook.test.ts @@ -0,0 +1,72 @@ +import { describe, expect, it, mock } from "bun:test" +import type { Client } from "@buape/carbon" +import { getOrCreateChannelWebhook } from "../src/utils/channelWebhook.js" + +const webhookData = { id: "123", token: "synthetic-webhook-token" } +const makeClient = (get: () => Promise, post = async () => webhookData) => { + const rest = { get: mock(get), post: mock(post) } + return { client: { rest } as unknown as Client, rest } +} + +describe("channel webhook cache", () => { + it("shares lookup and creation among simultaneous callers", async () => { + const { client, rest } = makeClient(async () => []) + const webhooks = await Promise.all(Array.from({ length: 20 }, () => + getOrCreateChannelWebhook(client, "concurrent-create") + )) + expect(rest.get).toHaveBeenCalledTimes(1) + expect(rest.post).toHaveBeenCalledTimes(1) + expect(new Set(webhooks).size).toBe(1) + expect(await getOrCreateChannelWebhook(client, "concurrent-create")).toBe(webhooks[0]) + expect(rest.get).toHaveBeenCalledTimes(1) + }) + + it("shares an existing webhook without creating another", async () => { + const { client, rest } = makeClient(async () => [webhookData]) + const webhooks = await Promise.all([ + getOrCreateChannelWebhook(client, "concurrent-existing"), + getOrCreateChannelWebhook(client, "concurrent-existing") + ]) + expect(rest.get).toHaveBeenCalledTimes(1) + expect(rest.post).not.toHaveBeenCalled() + expect(webhooks[0]).toBe(webhooks[1]) + }) + + for (const stage of ["lookup", "create", "missing-token"] as const) { + it(`releases failed ${stage} attempts so later events can retry`, async () => { + let fail = true + const { client, rest } = makeClient(async () => { + if (fail && stage === "lookup") throw new Error("lookup unavailable") + return [] + }, async () => { + if (fail && stage === "create") throw new Error("create unavailable") + return fail && stage === "missing-token" ? { id: "123", token: "" } : webhookData + }) + const channel = `retry-${stage}` + const results = await Promise.allSettled([ + getOrCreateChannelWebhook(client, channel), + getOrCreateChannelWebhook(client, channel) + ]) + expect(results.map((result) => result.status)).toEqual(["rejected", "rejected"]) + expect(rest.get).toHaveBeenCalledTimes(1) + fail = false + expect((await getOrCreateChannelWebhook(client, channel)).id).toBe(webhookData.id) + expect(rest.get).toHaveBeenCalledTimes(2) + }) + } + + it("does not block another channel behind a pending lookup", async () => { + let release = (_: unknown[]) => {} + const pending = new Promise((resolve) => { release = resolve }) + const slow = makeClient(() => pending) + const fast = makeClient(async () => [webhookData]) + const slowRequest = getOrCreateChannelWebhook(slow.client, "slow-channel") + try { + expect((await getOrCreateChannelWebhook(fast.client, "fast-channel")).id).toBe(webhookData.id) + expect(slow.rest.post).not.toHaveBeenCalled() + } finally { + release([]) + await slowRequest + } + }) +})