Skip to content

Commit b9a20dc

Browse files
littlebenpenggaolaiTakhoffman
authored
fix(slack): preserve dedupe while recovering dropped app_mention (openclaw#34937)
This PR fixes Slack mention loss without reintroducing duplicate dispatches. - Preserve seen-message dedupe at ingress to prevent duplicate processing. - Allow a one-time app_mention retry only when the paired message event was previously dropped before dispatch. - Add targeted race tests for both recovery and duplicate-prevention paths. Co-authored-by: littleben <[email protected]> Co-authored-by: OpenClaw Agent <[email protected]> Co-authored-by: Tak Hoffman <[email protected]>
1 parent 7830366 commit b9a20dc

File tree

3 files changed

+208
-2
lines changed

3 files changed

+208
-2
lines changed

‎CHANGELOG.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -256,6 +256,7 @@ Docs: https://docs.openclaw.ai
256256
- Synology Chat/reply delivery: resolve webhook usernames to Chat API `user_id` values for outbound chatbot replies, avoiding mismatches between webhook user IDs and `method=chatbot` recipient IDs in multi-account setups. (#23709) Thanks @druide67.
257257
- Slack/thread context payloads: only inject thread starter/history text on first thread turn for new sessions while preserving thread metadata, reducing repeated context-token bloat on long-lived thread sessions. (#32133) Thanks @sourman.
258258
- Slack/session routing: keep top-level channel messages in one shared session when `replyToMode=off`, while preserving thread-scoped keys for true thread replies and non-off modes. (#32193) Thanks @bmendonca3.
259+
- Slack/app_mention dedupe race handling: keep seen-message dedupe to prevent duplicate replies while allowing a one-time app_mention retry when the paired message event was dropped pre-dispatch, so requireMention channels do not lose mentions under Slack event reordering. (#34937) Thanks @littleben.
259260
- Voice-call/webhook routing: require exact webhook path matches (instead of prefix matches) so lookalike paths cannot reach provider verification/dispatch logic. (#31930) Thanks @afurm.
260261
- Zalo/Pairing auth tests: add webhook regression coverage asserting DM pairing-store reads/writes remain account-scoped, preventing cross-account authorization bleed in multi-account setups. (#26121) Thanks @bmendonca3.
261262
- Zalouser/Pairing auth tests: add account-scoped DM pairing-store regression coverage (`monitor.account-scope.test.ts`) to prevent cross-account allowlist bleed in multi-account setups. (#26672) Thanks @bmendonca3.

‎src/slack/monitor/message-handler.app-mention-race.test.ts‎

Lines changed: 157 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,157 @@
1+
import { beforeEach, describe, expect, it, vi } from "vitest";
2+
3+
const prepareSlackMessageMock =
4+
vi.fn<
5+
(params: {
6+
opts: { source: "message" | "app_mention"; wasMentioned?: boolean };
7+
}) => Promise<unknown>
8+
>();
9+
const dispatchPreparedSlackMessageMock = vi.fn<(prepared: unknown) => Promise<void>>();
10+
11+
vi.mock("../../channels/inbound-debounce-policy.js", () => ({
12+
shouldDebounceTextInbound: () => false,
13+
createChannelInboundDebouncer: (params: {
14+
onFlush: (
15+
entries: Array<{
16+
message: Record<string, unknown>;
17+
opts: { source: "message" | "app_mention"; wasMentioned?: boolean };
18+
}>,
19+
) => Promise<void>;
20+
}) => ({
21+
debounceMs: 0,
22+
debouncer: {
23+
enqueue: async (entry: {
24+
message: Record<string, unknown>;
25+
opts: { source: "message" | "app_mention"; wasMentioned?: boolean };
26+
}) => {
27+
await params.onFlush([entry]);
28+
},
29+
flushKey: async (_key: string) => {},
30+
},
31+
}),
32+
}));
33+
34+
vi.mock("./thread-resolution.js", () => ({
35+
createSlackThreadTsResolver: () => ({
36+
resolve: async ({ message }: { message: Record<string, unknown> }) => message,
37+
}),
38+
}));
39+
40+
vi.mock("./message-handler/prepare.js", () => ({
41+
prepareSlackMessage: (
42+
params: Parameters<typeof prepareSlackMessageMock>[0],
43+
): ReturnType<typeof prepareSlackMessageMock> => prepareSlackMessageMock(params),
44+
}));
45+
46+
vi.mock("./message-handler/dispatch.js", () => ({
47+
dispatchPreparedSlackMessage: (
48+
prepared: Parameters<typeof dispatchPreparedSlackMessageMock>[0],
49+
): ReturnType<typeof dispatchPreparedSlackMessageMock> =>
50+
dispatchPreparedSlackMessageMock(prepared),
51+
}));
52+
53+
import { createSlackMessageHandler } from "./message-handler.js";
54+
55+
function createMarkMessageSeen() {
56+
const seen = new Set<string>();
57+
return (channel: string | undefined, ts: string | undefined) => {
58+
if (!channel || !ts) {
59+
return false;
60+
}
61+
const key = `${channel}:${ts}`;
62+
if (seen.has(key)) {
63+
return true;
64+
}
65+
seen.add(key);
66+
return false;
67+
};
68+
}
69+
70+
describe("createSlackMessageHandler app_mention race handling", () => {
71+
beforeEach(() => {
72+
prepareSlackMessageMock.mockReset();
73+
dispatchPreparedSlackMessageMock.mockReset();
74+
});
75+
76+
it("allows a single app_mention retry when message event was dropped before dispatch", async () => {
77+
prepareSlackMessageMock.mockImplementation(async ({ opts }) => {
78+
if (opts.source === "message") {
79+
return null;
80+
}
81+
return { ctxPayload: {} };
82+
});
83+
84+
const handler = createSlackMessageHandler({
85+
ctx: {
86+
cfg: {},
87+
accountId: "default",
88+
app: { client: {} },
89+
runtime: {},
90+
markMessageSeen: createMarkMessageSeen(),
91+
} as Parameters<typeof createSlackMessageHandler>[0]["ctx"],
92+
account: { accountId: "default" } as Parameters<
93+
typeof createSlackMessageHandler
94+
>[0]["account"],
95+
});
96+
97+
await handler(
98+
{ type: "message", channel: "C1", ts: "1700000000.000100", text: "hello" } as never,
99+
{ source: "message" },
100+
);
101+
await handler(
102+
{
103+
type: "app_mention",
104+
channel: "C1",
105+
ts: "1700000000.000100",
106+
text: "<@U_BOT> hello",
107+
} as never,
108+
{ source: "app_mention", wasMentioned: true },
109+
);
110+
await handler(
111+
{
112+
type: "app_mention",
113+
channel: "C1",
114+
ts: "1700000000.000100",
115+
text: "<@U_BOT> hello",
116+
} as never,
117+
{ source: "app_mention", wasMentioned: true },
118+
);
119+
120+
expect(prepareSlackMessageMock).toHaveBeenCalledTimes(2);
121+
expect(dispatchPreparedSlackMessageMock).toHaveBeenCalledTimes(1);
122+
});
123+
124+
it("keeps app_mention deduped when message event already dispatched", async () => {
125+
prepareSlackMessageMock.mockResolvedValue({ ctxPayload: {} });
126+
127+
const handler = createSlackMessageHandler({
128+
ctx: {
129+
cfg: {},
130+
accountId: "default",
131+
app: { client: {} },
132+
runtime: {},
133+
markMessageSeen: createMarkMessageSeen(),
134+
} as Parameters<typeof createSlackMessageHandler>[0]["ctx"],
135+
account: { accountId: "default" } as Parameters<
136+
typeof createSlackMessageHandler
137+
>[0]["account"],
138+
});
139+
140+
await handler(
141+
{ type: "message", channel: "C1", ts: "1700000000.000200", text: "hello" } as never,
142+
{ source: "message" },
143+
);
144+
await handler(
145+
{
146+
type: "app_mention",
147+
channel: "C1",
148+
ts: "1700000000.000200",
149+
text: "<@U_BOT> hello",
150+
} as never,
151+
{ source: "app_mention", wasMentioned: true },
152+
);
153+
154+
expect(prepareSlackMessageMock).toHaveBeenCalledTimes(1);
155+
expect(dispatchPreparedSlackMessageMock).toHaveBeenCalledTimes(1);
156+
});
157+
});

‎src/slack/monitor/message-handler.ts‎

Lines changed: 50 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,8 @@ export type SlackMessageHandler = (
1515
opts: { source: "message" | "app_mention"; wasMentioned?: boolean },
1616
) => Promise<void>;
1717

18+
const APP_MENTION_RETRY_TTL_MS = 60_000;
19+
1820
function resolveSlackSenderId(message: SlackMessageEvent): string | null {
1921
return message.user ?? message.bot_id ?? null;
2022
}
@@ -51,6 +53,13 @@ function shouldDebounceSlackMessage(message: SlackMessageEvent, cfg: SlackMonito
5153
});
5254
}
5355

56+
function buildSeenMessageKey(channelId: string | undefined, ts: string | undefined): string | null {
57+
if (!channelId || !ts) {
58+
return null;
59+
}
60+
return `${channelId}:${ts}`;
61+
}
62+
5463
/**
5564
* Build a debounce key that isolates messages by thread (or by message timestamp
5665
* for top-level non-DM channel messages). Without per-message scoping, concurrent
@@ -133,9 +142,18 @@ export function createSlackMessageHandler(params: {
133142
wasMentioned: combinedMentioned || last.opts.wasMentioned,
134143
},
135144
});
145+
const seenMessageKey = buildSeenMessageKey(last.message.channel, last.message.ts);
136146
if (!prepared) {
147+
const hasMessageSource = entries.some((entry) => entry.opts.source === "message");
148+
const hasAppMentionSource = entries.some((entry) => entry.opts.source === "app_mention");
149+
if (seenMessageKey && hasMessageSource && !hasAppMentionSource) {
150+
rememberAppMentionRetryKey(seenMessageKey);
151+
}
137152
return;
138153
}
154+
if (seenMessageKey) {
155+
appMentionRetryKeys.delete(seenMessageKey);
156+
}
139157
if (entries.length > 1) {
140158
const ids = entries.map((entry) => entry.message.ts).filter(Boolean) as string[];
141159
if (ids.length > 0) {
@@ -152,6 +170,31 @@ export function createSlackMessageHandler(params: {
152170
});
153171
const threadTsResolver = createSlackThreadTsResolver({ client: ctx.app.client });
154172
const pendingTopLevelDebounceKeys = new Map<string, Set<string>>();
173+
const appMentionRetryKeys = new Map<string, number>();
174+
175+
const pruneAppMentionRetryKeys = (now: number) => {
176+
for (const [key, expiresAt] of appMentionRetryKeys) {
177+
if (expiresAt <= now) {
178+
appMentionRetryKeys.delete(key);
179+
}
180+
}
181+
};
182+
183+
const rememberAppMentionRetryKey = (key: string) => {
184+
const now = Date.now();
185+
pruneAppMentionRetryKeys(now);
186+
appMentionRetryKeys.set(key, now + APP_MENTION_RETRY_TTL_MS);
187+
};
188+
189+
const consumeAppMentionRetryKey = (key: string) => {
190+
const now = Date.now();
191+
pruneAppMentionRetryKeys(now);
192+
if (!appMentionRetryKeys.has(key)) {
193+
return false;
194+
}
195+
appMentionRetryKeys.delete(key);
196+
return true;
197+
};
155198

156199
return async (message, opts) => {
157200
if (opts.source === "message" && message.type !== "message") {
@@ -165,8 +208,13 @@ export function createSlackMessageHandler(params: {
165208
) {
166209
return;
167210
}
168-
if (ctx.markMessageSeen(message.channel, message.ts)) {
169-
return;
211+
const seenMessageKey = buildSeenMessageKey(message.channel, message.ts);
212+
if (seenMessageKey && ctx.markMessageSeen(message.channel, message.ts)) {
213+
// Allow exactly one app_mention retry if the same ts was previously dropped
214+
// from the message stream before it reached dispatch.
215+
if (opts.source !== "app_mention" || !consumeAppMentionRetryKey(seenMessageKey)) {
216+
return;
217+
}
170218
}
171219
trackEvent?.();
172220
const resolvedMessage = await threadTsResolver.resolve({ message, source: opts.source });

0 commit comments

Comments
 (0)