fix(slack): debounce app_mention edit re-fires to one turn per message#1104
fix(slack): debounce app_mention edit re-fires to one turn per message#1104dogzzdogzz wants to merge 2 commits into
Conversation
Slack re-fires `app_mention` for every edit state of a mention-bearing message, and native streaming (chat.startStream -> N x appendStream -> chat.stopStream -> final chat.update) makes a single reply produce several such states. Each state previously dispatched a full agent turn, producing phantom turns (duplicate agent invocations) and forced no-op replies in the thread. Add MentionDebouncer keyed on (channel, message ts): hold only the latest event state, push a 4s quiet deadline on each new state, then dispatch once on quiet -- only if the state's fingerprint (text + sorted attached file IDs) differs from what was last dispatched for that message. The deadline check and drain happen under one lock (no TOCTOU leak of an unsettled state); identical re-fires drop, material edits re-dispatch (fail-open, so a late final state is never lost). Dispatch history is FIFO-bounded. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
All PRs must reference a prior Discord discussion to ensure community alignment before implementation. Please edit the PR description to include a link like: This PR will be automatically closed in 24 hours if the link is not added. |
Expose the previously-hardcoded 4s mention debounce window as `[slack] mention_debounce_seconds` (Helm: `slack.mentionDebounceSeconds`), default 4. Deployments can tune the quiet window, or set 0 to dispatch on the first event (disabling debounce). `MentionDebouncer` now holds the window as a field (Default stays 4s via `MENTION_DEBOUNCE`); `run_slack_adapter` threads the configured value through from `SlackConfig`. Helm chart renders `mention_debounce_seconds` only when the value is set, so the Rust default applies otherwise. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Closing in favor of a simpler, source-level fix. Rather than debouncing the The phantom turns originate in multi-agent threads, where a streamed reply that @-mentions another bot re-delivers that mention across the message's in-progress states; send-once collapses that to one delivery with none of the debounce timing/fingerprint/race machinery. Trade-off: no live typewriter streaming. Thanks! |
Debounce Slack
app_mentionso one user message produces one agent turn.What problem does this solve?
src/slack.rsspawns a fullhandle_message(→ one agent turn) for everyapp_mentionevent, with no dedup by messagets. But a single logical mention arrives as several events: Slack re-deliversapp_mentionas a mention-bearing message moves through edit states, and openab's own native streaming drives a reply through several message states (chat.startStream→ N×appendStream→chat.stopStream→ finalchat.update). One observed multi-agent thread produced 5 agent turns for 2 real messages — duplicate LLM invocations, each forcing a no-op reply into the thread, and one dispatch even fired on a mid-stream fragment whose text was just<@U…> -.Closes #1103
At a Glance
Prior Art & Industry Research
src/slack.rs), upstream of any agent backend and independent of which ACP agent runs.docs/adr/turn-boundary-batching.md, the Dispatcher): the closest in-repo prior art. It coalesces distinct messages that arrive during an in-flight turn into one batch. This change is complementary and orthogonal — it dedupes repeated edit states of the same message at the adapter ingress, before a turn is ever dispatched, which the batching dispatcher does not cover.debounceSecs): precedent for a per-adapter, seconds-valued "debounce quiet period before flushing" config — the naming/units ofmention_debounce_secondsfollow it.app_mentiondelivery is at-least-once and re-fires as a message is edited (message_changed). Trailing-edge debounce keyed on(channel, ts)is the standard way to collapse such an event storm down to the message's settled state.Proposed Solution
Add
MentionDebouncer, keyed on(channel, ts):text+ sorted attached file IDs) differs from what was last dispatched for that message — so an attachment-only edit is still material, while an identical re-fire drops.try_take), closing the race where a new edit arriving mid-check would otherwise leak an unsettled state through.MENTION_HISTORY_CAP).The quiet window is configurable via
[slack] mention_debounce_seconds(Helm:slack.mentionDebounceSeconds), default 4. Set it lower to trade robustness for latency, or0to dispatch on the first event (effectively disabling debounce). The window defaults toMENTION_DEBOUNCE(4s) when unset.Trailing-edge is deliberate: the first event state of a streamed/edited message is often a mid-stream fragment, and responding to fragments is worse than a few seconds of latency on turns that already take tens of seconds.
Why this approach?
The bug is at the adapter ingress (one Slack message → many events), so the fix belongs there, before dispatch — not in the cross-platform batching Dispatcher, whose job (coalescing distinct messages at turn boundaries) is a different concern. Keying on
(channel, ts)matches Slack's own identity for "the same message across its edit states". The change is contained to the Slack adapter plus a single, default-preserving config knob.Alternatives Considered
<@U…> -); responding to fragments is worse than trailing latency.tsedit states are not its model, and pushing per-message edit dedup down there would entangle a Slack-specific event quirk with the platform-agnostic batching logic.event_id/client_msg_id: wouldn't collapse genuine edit states (each edit is a fresh delivery of the same logical message);(channel, ts)+ content fingerprint is the right grain.mention_debounce_ms(millisecond units): rejected for_seconds— this is a coarse, human-tuned window (3–5s), matching the repo's seconds-valued duration config (timeout_seconds,*_secs, WeComdebounceSecs) rather than the sub-secondReactionTiming_msknobs.Validation
Built and tested locally against this branch:
cargo test --bin openab: 501 passed, 1 failed. The one failure (secrets::tests::resolve_exec_nonzero_exit) reproduces on a clean tree in this environment (asserts a shell error-message string) and is unrelated to this change —src/secrets.rsis untouched here.mention_debounce_latest_state_wins_and_refires_drop,mention_debounce_try_take_respects_extended_deadline,mention_debounce_history_is_bounded(latest-state-wins, identical-re-fire-drop, attachment-only-edit-is-material, the mid-check deadline race, and FIFO-bounded history).cargo clippy --all-targets -- -D warnings: clean on all changed files. (Two pre-existingclippy::bool_assert_comparisonfindings in the untouchedsrc/cron.rsare surfaced only by a newer local clippy; not introduced here.)helm template:slack.mentionDebounceSeconds=Nrendersmention_debounce_seconds = Nunder[slack]; omitted when unset (Rust default applies).Please re-verify via CI.