Skip to content

fix(gateway): skip duplicate-send diagnostic for interim-only stream consumers (#105341) - #105446

Closed
webtecnica wants to merge 1 commit into
NousResearch:mainfrom
webtecnica:fix/issue105341-dup-send-warn
Closed

webtecnica wants to merge 1 commit into
NousResearch:mainfrom
webtecnica:fix/issue105341-dup-send-warn

Conversation

@webtecnica

Copy link
Copy Markdown

Closes #105341

Problem

With streaming.enabled: false and display.interim_assistant_messages: true, every gateway turn logs a possible duplicate send warning — a guaranteed false positive (321/day on the reporter's install). The GatewayStreamConsumer is still built (interim commentary is relayed through it, gateway/run_turn_runner.py:800), but with text streaming off it is never fed the final reply's deltas. _run_agent_mark_streamed_delivery (gateway/run_turn.py:3595+) could not distinguish "consumer built only for interim messages" from "consumer that streamed but lost its delivery confirmation", so it logged the duplicate-risk diagnostic on every turn.

Change

  • GatewayStreamConsumer gains stream_deltas_enabled = True (default; every construction site that can race the final send — incl. the proxy path, which only builds consumers when streaming is on — keeps the diagnostic).
  • _setup_stream_consumer (run_turn_runner.py) sets stream_consumer.stream_deltas_enabled = want_stream_deltas, marking interim-only consumers (streaming off, interim messages on).
  • The duplicate-risk diagnostic branch in _run_agent_mark_streamed_delivery now requires stream_deltas_enabled — a consumer never fed the final's deltas cannot have raced the normal final send, so the warning is skipped. The wecom ack-timeout case the diagnostic was written for is untouched (that consumer streams).

Tests

  • test_interim_only_consumer_skips_duplicate_warning — interim-only consumer: no warning.
  • test_stream_capable_consumer_still_warns — control: stream-capable consumer keeps the diagnostic.
  • test_consumer_flag_defaults_true_on_real_consumer — real consumer defaults to stream-capable.
  • Ran: python -m pytest tests/gateway/test_interim_only_consumer_warning.py tests/gateway/test_stale_finalize_suppression.py tests/gateway/test_duplicate_reply_suppression.py tests/gateway/test_wecom_double_send.py tests/gateway/test_telegram_final_delivery.py tests/gateway/test_silent_partial_delivery_95382.py tests/gateway/test_stream_consumer.py -q → 123 passed, 1 xfailed.

@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have comp/gateway Gateway runner, session dispatch, delivery area/streaming Streaming responses: gateway delivery, provider wire sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages labels Sep 8, 2026
@Enough1122

Copy link
Copy Markdown

AI code review — automated review for reference; please use your judgment.

Summary

Fixes #105341: the duplicate-send diagnostic in _run_agent_mark_streamed_delivery no longer fires for interim-only stream consumers. A consumer built solely to relay interim commentary (text streaming off, display.interim_assistant_messages on) is never fed the final reply's deltas, so it could never have raced the normal final send — every such turn logged a guaranteed-false-positive "possible duplicate send" warning. New stream_deltas_enabled flag (default True, set from want_stream_deltas at the one interim-capable construction site) gates the warning; getattr(..., True) keeps foreign/Mock consumers on the warning path.

Findings

  • Non-blocking, note: gateway/run_turn.py:12 — the getattr(_sc, "stream_deltas_enabled", True) default-True means any consumer that doesn't set the flag keeps warning; fail-loud direction is correct for a diagnostic (missed true positives are worse than the fixed false positives).
  • Non-blocking, note: gateway/run_turn_runner.py:31 — the flag assignment sits inside the existing try/except that downgrades setup failures to debug; if the assignment itself ever threw, the consumer would silently keep the default True and warn — again the loud direction. Fine.
  • Positive: the regression test (tests/gateway/test_interim_only_consumer_warning.py) pins both directions — stream-capable consumers still warn (the wecom ack-timeout case the diagnostic was written for), interim-only stays silent, and the real consumer defaults to True.

Verdict

Looks good. Minimal flag with fail-loud defaults and control-plus-regression tests. Safe to merge.

@webtecnica

Copy link
Copy Markdown
Author

Heads-up for anyone landing here: this fix was salvaged upstream in #111467 by @teknium1 — the PR body credits it as Salvages #105446 by @webtecnica (cherry-picked as-is; the third change-detector test was trimmed), alongside #111281, #110590 and #105341.

Leaving this one open for now, since that salvage has not merged yet; no need to re-review the diff here in the meantime.

@webtecnica

Copy link
Copy Markdown
Author

Superseded by #111467, which salvages this PR cherry-picked as-is — closing in favour of it, thanks @teknium1. The trimmed third change-detector test is fine by me; the invariant that carries the fix (an interim-only consumer no longer warns) is covered there.

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 P3 Low — cosmetic, nice to have 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.

[Bug]: "possible duplicate send" warning fires every turn when streaming is off and interim_assistant_messages is on

3 participants