Skip to content

fix(slack): await draft stream flush before messageId check#23118

Open
dashed wants to merge 1 commit intoopenclaw:mainfrom
dashed:alberto/draft-stream-race
Open

fix(slack): await draft stream flush before messageId check#23118
dashed wants to merge 1 commit intoopenclaw:mainfrom
dashed:alberto/draft-stream-race

Conversation

@dashed
Copy link
Copy Markdown
Contributor

@dashed dashed commented Feb 22, 2026

Summary

Fixes #19373.

When Slack streaming is enabled, sendMessageSlack() is called fire-and-forget inside the draft stream. The deliver callback in dispatch.ts then reads draftStream.messageId() to decide whether it can finalize via preview-edit — but the send hasn't resolved yet, so messageId() returns undefined. This causes canFinalizeViaPreviewEdit to evaluate false, and the dispatcher falls through to normal delivery, posting a duplicate message.

This PR adds await draftStream?.flush() before the messageId() read so the in-flight send resolves first.

Root Cause

In dispatchPreparedSlackMessage (dispatch.ts), the deliver callback evaluates:

const draftMessageId = draftStream?.messageId();
const canFinalizeViaPreviewEdit = draftMessageId && draftChannelId && ...;

But messageId() is only populated after the draft stream's sendMessageSlack() promise resolves. Since the send is fire-and-forget (kicked off by stream.update()), there's a race: if the deliver callback runs before the send completes, messageId() returns undefined and the message is delivered normally — duplicating the streamed draft.

The Discord handler already does await flushDraft() before reading messageId (at message-handler.process.ts:555), but the Slack handler was missing the equivalent call.

Changes

src/slack/monitor/message-handler/dispatch.ts — Add flush before messageId read:

       }

+      // Flush any in-flight draft send so messageId() is populated.
+      // Without this, a race between the fire-and-forget sendMessageSlack()
+      // and the deliver callback can cause canFinalizeViaPreviewEdit to
+      // evaluate false, resulting in a duplicate Slack message.
+      await draftStream?.flush();
+
       const mediaCount = payload.mediaUrls?.length ?? (payload.mediaUrl ? 1 : 0);
       const draftMessageId = draftStream?.messageId();

src/slack/draft-stream.test.ts — Two new tests:

+  it("flush resolves in-flight send so messageId is available (race condition fix)", async () => {
+    // Creates a deferred send, starts flush, verifies messageId is undefined,
+    // resolves the send, then verifies messageId is populated
+  });
+
+  it("flush is safe to call when no draft has been started", async () => {
+    // Verifies flush is a no-op when no update has been called
+  });

Comparison with #20248 and #20623

Aspect This PR #20248 #20623
Fix location Before messageId() read Before final reply delivery (after messageId() read) Similar to this PR
Root cause addressed Yes — flush ensures messageId() is populated Partially — flushes too late for canFinalizeViaPreviewEdit Yes, but bundled with streaming fix
Scope Minimal (1 line + comment) Minimal (flush + stop) Broader (also fixes recipient_team_id)
Tests 2 new tests None None
Streaming fix bundled No (already merged as #20988) No Yes (redundant with #20988)

The key difference from #20248: that PR flushes before the final deliverReplies call, but the canFinalizeViaPreviewEdit evaluation happens before delivery. If messageId() is still undefined at evaluation time, the code has already decided to skip preview-edit and deliver normally — flushing afterward doesn't prevent the duplicate. This PR flushes before the evaluation, which is the correct fix point.

Test Plan

  • 2 new tests in src/slack/draft-stream.test.ts
  • flush resolves in-flight send so messageId is available — verifies the race condition fix
  • flush is safe to call when no draft has been started — verifies no-op safety

Related

Greptile Summary

Adds await draftStream?.flush() before reading messageId() in the deliver callback to ensure in-flight draft sends complete before the canFinalizeViaPreviewEdit check. Without this flush, a race condition causes messageId() to return undefined, preventing preview-edit finalization and resulting in duplicate Slack messages.

  • Fixes race condition where fire-and-forget sendMessageSlack() hasn't resolved when messageId() is checked
  • Matches Discord handler pattern (line 559 in message-handler.process.ts)
  • Includes two well-structured tests verifying flush behavior and safety

Confidence Score: 5/5

  • This PR is safe to merge with no risk
  • The fix is minimal, well-tested, and follows an established pattern from Discord. The race condition is clearly identified and the solution directly addresses the root cause by ensuring the async send completes before reading the result.
  • No files require special attention

Last reviewed commit: 132c809


Rebase History

Date Base Upstream Commits Notes
2026-03-23 d5917d37c54a (post-v2026.3.23) 929 Rebased cleanly, zero conflicts.
2026-03-13 330631a0eb39 (v2026.3.12) - Clean rebase. Commit: 767f80f1e7e8.
2026-03-08 eb0758e1722c (v2026.3.7) - Clean rebase, no conflicts. All tests pass. Commit: 473fc58c5e96.

@dashed
Copy link
Copy Markdown
Contributor Author

dashed commented Feb 22, 2026

Note on Windows CI failure: The `checks-windows (node, test)` failure is a pre-existing issue on main — not caused by this PR. Fix: #23128

@dashed dashed force-pushed the alberto/draft-stream-race branch 2 times, most recently from 98048c8 to 630f358 Compare February 27, 2026 07:29
@dashed dashed force-pushed the alberto/draft-stream-race branch 3 times, most recently from 215d7ed to 473fc58 Compare March 8, 2026 13:39
@dashed dashed force-pushed the alberto/draft-stream-race branch 2 times, most recently from 767f80f to 02598cf Compare March 13, 2026 22:43
@openclaw-barnacle
Copy link
Copy Markdown

This pull request has been automatically marked as stale due to inactivity.
Please add updates or it will be closed.

@openclaw-barnacle openclaw-barnacle bot added the stale Marked as stale due to inactivity label Mar 20, 2026
@dashed dashed force-pushed the alberto/draft-stream-race branch 2 times, most recently from b4e7594 to 3bbbf78 Compare March 21, 2026 05:49
@openclaw-barnacle openclaw-barnacle bot removed the stale Marked as stale due to inactivity label Mar 23, 2026
@dashed dashed force-pushed the alberto/draft-stream-race branch from 3bbbf78 to 8bd8522 Compare March 24, 2026 02:47
@dashed dashed force-pushed the alberto/draft-stream-race branch from 8bd8522 to 58ad7cd Compare March 28, 2026 00:39
@dashed
Copy link
Copy Markdown
Contributor Author

dashed commented Mar 28, 2026

Rebased to latest main (post-v2026.3.26, 2a98464a). Clean rebase — branch already targets extensions/slack/src/ paths. No changes to patch logic.

@openclaw-barnacle
Copy link
Copy Markdown

This pull request has been automatically marked as stale due to inactivity.
Please add updates or it will be closed.

@openclaw-barnacle openclaw-barnacle bot added the stale Marked as stale due to inactivity label Apr 2, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

channel: slack Channel integration: slack size: XS stale Marked as stale due to inactivity

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Slack Duplicate Reply Message

1 participant