From dd0822a16b1cd2780ed49d61512227fcaae5cf3f Mon Sep 17 00:00:00 2001 From: Maximilian Kaske Date: Thu, 15 Oct 2026 20:34:05 +0200 Subject: [PATCH] fix(slack): bound the permalink lookup; address source_ref review - permalinkFor races chat.getPermalink against a 3s timeout so a slow Slack API no longer delays the agent turn; a timeout logs and returns undefined like a failed lookup - comment why appendIncidentEvent uses an explicit-undefined check for sourceRef instead of ?? - backfill script logs progress every 100 rows on --apply --- .../src/routes/slack/service-adapter.test.ts | 30 +++++++++++++++++++ .../src/routes/slack/service-adapter.ts | 21 +++++++++++-- .../backfill-incident-event-source.mts | 1 + packages/services/src/incident/internal.ts | 1 + 4 files changed, 51 insertions(+), 2 deletions(-) create mode 100644 apps/server/src/routes/slack/service-adapter.test.ts diff --git a/apps/server/src/routes/slack/service-adapter.test.ts b/apps/server/src/routes/slack/service-adapter.test.ts new file mode 100644 index 000000000..e28eb9eb0 --- /dev/null +++ b/apps/server/src/routes/slack/service-adapter.test.ts @@ -0,0 +1,30 @@ +import { describe, expect, test } from "@openstatus/test-utils"; + +import { permalinkFor } from "./service-adapter"; + +function slackWith(getPermalink: () => Promise<{ permalink?: string }>) { + return { chat: { getPermalink } } as unknown as Parameters< + typeof permalinkFor + >[0]; +} + +describe("permalinkFor", () => { + test("returns the permalink", async () => { + const slack = slackWith(() => + Promise.resolve({ permalink: "https://slack.test/archives/C1/p1" }), + ); + expect(await permalinkFor(slack, "C1", "1.0")).toBe( + "https://slack.test/archives/C1/p1", + ); + }); + + test("gives up after the timeout instead of holding the caller", async () => { + const slack = slackWith(() => new Promise(() => {})); + expect(await permalinkFor(slack, "C1", "1.0", 10)).toBeUndefined(); + }); + + test("swallows API errors", async () => { + const slack = slackWith(() => Promise.reject(new Error("ratelimited"))); + expect(await permalinkFor(slack, "C1", "1.0")).toBeUndefined(); + }); +}); diff --git a/apps/server/src/routes/slack/service-adapter.ts b/apps/server/src/routes/slack/service-adapter.ts index a46945c8c..f3f7a4979 100644 --- a/apps/server/src/routes/slack/service-adapter.ts +++ b/apps/server/src/routes/slack/service-adapter.ts @@ -12,18 +12,35 @@ import type { SlackActor } from "./require-slack-member"; const logger = getLogger(["api-server", "slack", "service-adapter"]); -/** The message's permalink, or `undefined`: a missing link never fails a write. */ +/** + * The message's permalink, or `undefined`: a missing link never fails a + * write, so a slow Slack API is bounded instead of holding up the turn. + */ export async function permalinkFor( slack: Pick, channel: string, ts: string, + timeoutMs = 3_000, ): Promise { + let timer: ReturnType | undefined; + const timeout = new Promise<"timeout">((resolve) => { + timer = setTimeout(() => resolve("timeout"), timeoutMs); + }); try { - const res = await slack.chat.getPermalink({ channel, message_ts: ts }); + const res = await Promise.race([ + slack.chat.getPermalink({ channel, message_ts: ts }), + timeout, + ]); + if (res === "timeout") { + logger.warn("slack permalink timed out", { channel, timeoutMs }); + return undefined; + } return res.permalink; } catch (error) { logger.warn("slack failed to fetch a permalink", { error, channel }); return undefined; + } finally { + clearTimeout(timer); } } diff --git a/packages/db/scripts/backfill-incident-event-source.mts b/packages/db/scripts/backfill-incident-event-source.mts index 442c4ea6c..b71c113d5 100644 --- a/packages/db/scripts/backfill-incident-event-source.mts +++ b/packages/db/scripts/backfill-incident-event-source.mts @@ -41,6 +41,7 @@ for (const row of rows) { message: row.message.replace(FROM_SLACK, ""), }) .where(eq(incidentEvent.id, row.id)); + if (updated % 100 === 0) console.log(`${updated}/${rows.length}`); } console.log( diff --git a/packages/services/src/incident/internal.ts b/packages/services/src/incident/internal.ts index ee9808230..04df66b83 100644 --- a/packages/services/src/incident/internal.ts +++ b/packages/services/src/incident/internal.ts @@ -119,6 +119,7 @@ export async function appendIncidentEvent( type: args.type, message: args.message ?? null, source: args.source ?? actorSource(ctx.actor), + // not `??`: an explicit null (agent writes) must beat the actor's ref sourceRef: args.sourceRef !== undefined ? args.sourceRef -- 2.54.0