fix(gateway): skip queued-follow-up re-send when stream consumer already delivered - #172
hashbender wants to merge 1 commit into
Conversation
|
Review Complete Files Reviewed: 2 By Severity:
PR adds duplicate-reply suppression to the gateway's queued-follow-up path, but the fallback check uses Files Reviewed (2 files) |
There was a problem hiding this comment.
Risk: 🟠 High (68/100) — 1 high finding · 58 LOC across 2 files
Single High-Severity Finding
This PR modifies gateway/run.py to suppress duplicate reply delivery in the queued-follow-up path by checking whether the stream consumer already sent content. However, the fallback check at line 18067 uses _sc.already_sent rather than _sc.final_content_delivered.
The Bug
_sc.already_sent(defined instream_consumer.pyline 238-240) isTruewhenever any content reached the platform — tool-progress commentary, partial fallback chunks, interim edits. It is deliberately not reset at tool boundaries (line 344-345)._sc.final_content_delivered(line 253-256) is the dedicated flag for "the final response content reached the user, even if cosmetic finalization failed."- The normal delivery path at line 18252-18253 already correctly checks
final_content_delivered. - The queued-follow-up fallback added by this PR checks
already_sentinstead.
Impact
If the stream task is cancelled after sending tool-progress commentary (already_sent=True) but before the final answer is delivered (final_content_delivered=False), the fallback incorrectly skips the re-send and the user receives no answer. This is a real risk because the stream consumer explicitly documents (line 593-600) that already_sent can be True without the final response having been delivered.
Fix
Change the fallback check from already_sent to final_content_delivered, update the surrounding comments, and update the corresponding test at test_duplicate_reply_suppression.py line 387 to set the correct flag.
| if ( | ||
| not _already_streamed | ||
| and _sc is not None | ||
| and getattr(_sc, "already_sent", False) | ||
| ): | ||
| _already_streamed = True | ||
| logger.debug( | ||
| "Queued follow-up for session %s: stream consumer already_sent=True but final_response_sent=False; skipping re-send.", | ||
| session_key or "?", | ||
| ) |
There was a problem hiding this comment.
🟠 Queued-follow-up fallback uses already_sent instead of final_content_delivered, risking dropped final responses (bug)
In gateway/run.py lines 18067–18076, the PR adds a fallback that treats _sc.already_sent=True as confirmation the final response was delivered, skipping the re-send. However, already_sent is set whenever any content (tool-progress commentary, partial fallback chunks, interim edits) reaches the platform — it does NOT guarantee the final answer was delivered. The stream consumer explicitly documents this at line 593-600: '_already_sent may be True from prior tool-progress edits or fallback-mode promotion — that doesn't mean the final answer reached the user.' The _reset_segment_state() method (line 344-345) resets _final_response_sent and _final_content_delivered at tool boundaries but intentionally does NOT reset _already_sent, so the flag can be stale from earlier segments. The flag designed for this exact purpose — final_content_delivered (line 253-256: 'True when the final response content reached the user, even if the subsequent cosmetic edit failed') — is used correctly by the normal delivery path at line 18252-18253 but omitted from this queued-follow-up path. If the stream task is cancelled after sending tool-progress or partial text (already_sent=True) but before the final response is delivered (final_content_delivered=False), the fallback incorrectly suppresses the re-send and the user receives no answer.
💡 Suggestion: Replace the already_sent fallback check with final_content_delivered, which is the stream consumer's dedicated flag for 'final response content reached the user even if finalization/cursor-removal later failed.' This is the flag the normal delivery path already uses at line 18252-18253.
| if ( | |
| not _already_streamed | |
| and _sc is not None | |
| and getattr(_sc, "already_sent", False) | |
| ): | |
| _already_streamed = True | |
| logger.debug( | |
| "Queued follow-up for session %s: stream consumer already_sent=True but final_response_sent=False; skipping re-send.", | |
| session_key or "?", | |
| ) | |
| if ( | |
| not _already_streamed | |
| and _sc is not None | |
| and getattr(_sc, "final_content_delivered", False) | |
| ): | |
| _already_streamed = True | |
| logger.debug( | |
| "Queued follow-up for session %s: stream consumer final_content_delivered=True but final_response_sent=False; skipping re-send.", | |
| session_key or "?", | |
| ) |
📋 Prompt for AI Agents
In gateway/run.py at lines 18067–18076, change the fallback check from getattr(_sc, 'already_sent', False) to getattr(_sc, 'final_content_delivered', False). Also update the comment block at lines 18060–18065 to reference final_content_delivered instead of already_sent, and update the corresponding test at tests/gateway/test_duplicate_reply_suppression.py line 387 to set final_content_delivered=True instead of already_sent=True so the test validates the correct signal.
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 atfinal_response_sent. If the stream task was cancelled after the send (e.g. image delivery or cleanup exceeded the 5-second wait),final_response_sentstaysFalseand the response is re-sent as a duplicate.The fix adds a fallback check: if the stream consumer's
already_sentproperty isTrue, treat delivery as confirmed. This only applies to the queued-follow-up path — the normal send path is unaffected.Related Issue
Fixes NousResearch#55806
Type of Change
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'salready_sentis 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
pytest tests/gateway/test_duplicate_reply_suppression.py -q— all 28 tests should pass (26 existing + 2 new).Checklist
Code
fix(scope):,feat(scope):, etc.)pytest tests/ -qand all tests passDocumentation & Housekeeping
docs/, docstrings) — or N/Acli-config.yaml.exampleif I added/changed config keys — or N/ACONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — or N/AScreenshots / Logs
From the issue reporter's gateway logs:
With the fix, the queued follow-up path sees
already_sent=Trueon the stream consumer and skips the re-send.Mirror-of: NousResearch#56092
NousResearch#56092