Skip to content

fix(gateway): skip queued-follow-up re-send when stream consumer already delivered - #56092

Closed
liuhao1024 wants to merge 1 commit into
NousResearch:mainfrom
liuhao1024:liuhao/cron-bugfix-55806-queued-followup-duplicate
Closed

liuhao1024 wants to merge 1 commit into
NousResearch:mainfrom
liuhao1024:liuhao/cron-bugfix-55806-queued-followup-duplicate

Conversation

@liuhao1024

Copy link
Copy Markdown

What does this PR do?

Fixes a race condition where the gateway's queued-follow-up path re-sends a response that was already delivered to the user by the stream consumer.

When a response includes image attachments, the stream consumer sends the text, then _deliver_media_from_response() sends the images separately. If background processes finish around the same time and inject notifications, the queued-follow-up path checks _stream_confirmed_final_delivery() which only looks at final_response_sent. If the stream task was cancelled after the send (e.g. image delivery or cleanup exceeded the 5-second wait), final_response_sent stays False and the response is re-sent as a duplicate.

The fix adds a fallback check: if the stream consumer's already_sent property is True, treat delivery as confirmed. This only applies to the queued-follow-up path — the normal send path is unaffected.

Related Issue

Fixes #55806

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)

Changes Made

  • gateway/run.py: Added fallback check in the queued-follow-up path — when _stream_confirmed_final_delivery() returns False but the stream consumer's already_sent is True, skip the re-send. This prevents duplicate responses when the stream task is cancelled after delivering the message.
  • tests/gateway/test_duplicate_reply_suppression.py: Added 2 regression tests covering the new fallback (consumer already sent → skip, consumer never sent → re-send).

How to Test

  1. Run pytest tests/gateway/test_duplicate_reply_suppression.py -q — all 28 tests should pass (26 existing + 2 new).
  2. The bug requires a specific timing scenario (stream consumer sends text, image delivery takes >5 seconds, background process notification arrives during the gap). The regression tests verify the logic without requiring the timing race.

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run pytest tests/ -q and all tests pass
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform: macOS 26.4.1

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — or N/A
  • I've updated cli-config.yaml.example if I added/changed config keys — or N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — or N/A
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide — or N/A (logic-only change, no platform-specific behavior)
  • I've updated tool descriptions/schemas if I changed tool behavior — or N/A

Screenshots / Logs

From the issue reporter's gateway logs:

17:59:06  [Discord] Sending response (672 chars)    ← stream consumer sent the text
17:59:06  [Discord] Sending 1 image(s)              ← image delivered separately
17:59:11  Process finished — injecting agent notification
17:59:14  Queued follow-up: final stream delivery not confirmed; sending first response  ← BUG: duplicate

With the fix, the queued follow-up path sees already_sent=True on the stream consumer and skips the re-send.

…ady delivered

When the stream consumer sends the response but the stream task is
cancelled before setting _final_response_sent (e.g. image delivery or
cleanup exceeds the 5-second wait), the queued-follow-up path
re-sends the response that is already visible on the chat platform.

Add a fallback check: if the stream consumer's already_sent flag is
True, treat delivery as confirmed even when final_response_sent is
False.  This only applies to the queued-follow-up path — the normal
send path is unaffected.

Regression tests in test_duplicate_reply_suppression.py.

Fixes NousResearch#55806
@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/gateway Gateway runner, session dispatch, delivery platform/discord Discord bot adapter sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages labels Jul 1, 2026
@teknium1

Copy link
Copy Markdown
Collaborator

Thanks for tracing the queued-follow-up race and providing the Discord logs from #55806.

Problems

  • The proposed fallback treats already_sent as proof that the final answer reached the user. That property only means some message was sent or edited (gateway/stream_consumer.py:250-252); it may represent tool-progress or fallback-mode partial output rather than the final answer (gateway/stream_consumer.py:688-695). This would make gateway/run.py's queued path suppress the full-response fallback when it is still required.
  • The added tests duplicate the proposed boolean expression instead of exercising the queued delivery path, so they do not cover the partial-output distinction enforced by current cancellation handling (gateway/stream_consumer.py:861-888).

Suggested changes

  • Track confirmation for the exact completed response on the direct/media delivery path, rather than using already_sent.
  • Add an integration-level queued-follow-up test covering both complete direct delivery and partial streamed output.

Automated hermes-sweeper review.

@kshitijk4poor

Copy link
Copy Markdown

Thanks @liuhao1024 — you were the first to pin the missing confirmation on this path (2026-06-30). The guard landed in a stricter shape than a bare already_sent check: 194bff0 and 80ba7f6 make _run_agent_stream_confirmed_final_delivery (gateway/run_turn.py) accept an exact durable-text match via GatewayStreamConsumer.has_durably_delivered_text, so partial progress text can never suppress the final send (the concern raised on this PR), and the queued lane now reconciles by editing the streamed message first (#85796). A probe on 0a1bc07 with the #55806 image/cancel case produces zero resends. Closing as superseded by what is on main; #55806 is closed with the same evidence.

@kshitijk4poor

Copy link
Copy Markdown

Thanks for tracing the queued-follow-up race and for the Discord logs on #55806 — the flag combination you identified (already_sent=True with final_response_sent=False) is real. Closing this as superseded: on current main the approach is both redundant and, if applied, a regression.

Why not merged

already_sent means "some bytes went out", not "the completed final answer reached the user". The codebase sets it at every partial/progress site, and says so itself:

  • gateway/stream_consumer_fallback.py:113-122 — partial fallback continuation landed: _final_response_sent is deliberately left False because "the gateway must still deliver the full answer".
  • gateway/stream_consumer.py:307 — "_already_sent may be True from prior progress/fallback state (fix(gateway): streaming mode silently drops final response when already_sent is true #10748)".
  • gateway/stream_consumer_transport.py:426-428 — draft frames deliberately do not set _already_sent so the final send still fires.

Tracing every _already_sent site: 7 of them are partial/progress/preview states where the final answer was not delivered, and the remaining sites all set final_response_sent as well — so the fallback would add nothing where it is safe and suppress the answer where it is not.

Reproduced against main (0b9a9a0f5c)

A real-import probe driving GatewayStreamConsumer._send_fallback_final with a flaky adapter (chunk 1 lands, chunk 2 fails) leaves the consumer at already_sent=True, final_response_sent=False, final_content_delivered=False, has_delivered_text=False, and the final answer never reached the chat. main's predicate correctly returns False there (safety resend fires); the fallback in this PR would return True and the user would be left with only the head of the answer. In the queued path that send is the first turn's only delivery (run_turn.py returns through _run_agent_queued_followup before _run_agent_mark_streamed_delivery at run_turn.py:4259), so suppressing it loses the reply outright.

Also

  • The patch no longer applies: the guard moved out of gateway/run.py into gateway/run_turn.py::_run_agent_stream_confirmed_final_delivery (git merge-tree --write-tree origin/main <head> exits 1 on both files).
  • The two added tests never touch production code (they recompute the same boolean against a SimpleNamespace) — they pass on clean main with the change reverted, so they cannot guard the behaviour.
  • main's predicate now reconciles against the exact delivered text (has_durably_delivered_text, landed in 80ba7f6f79), which covers the confirm-after-send case this targeted, without trusting a progress flag.

If you can still reproduce a duplicate on current main, please reopen with the gateway log and I'll land the fix — the right shape confirms by the exact delivered payload, not by already_sent.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/streaming Streaming responses: gateway delivery, provider wire comp/gateway Gateway runner, session dispatch, delivery P2 Medium — degraded but workaround exists platform/discord Discord bot adapter sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Gateway: queued follow-up resends previous response when stream delivery confirmation fails (image + background process race)

4 participants