fix(gateway): judge delivery success against final content, not flag trust (#95382, #98552 class) - #100533
Conversation
…trust (#95382, #98552) A record-less delivery flag (final_response_sent / final_content_delivered set with no recorded turn-final payload) was trusted blindly by delivered_final_matches (None -> legacy trust), so a first-edit prefix or a truncated finalize suppressed the gateway's corrective send — silent partial delivery. - delivered_final_matches: record-less flags are now reconciled against the FINAL content via has_delivered_text; only the explicitly-marked ambiguous-timeout path (_delivery_ambiguous) keeps legacy trust. - _try_fresh_final and the native-streaming optimistic finalize now record their delivered payload (the last record-less flag setters); the optimistic record rolls back on definitive dispatch failure. - Discord adapter: dead-transport send failures (client gone, WS closed/reset) are classified as send_path_degraded (retryable) so the delivery-obligation ledger's reconnect sweep replays the stranded final response instead of losing it until a process restart. Fixes #95382; closes the #98552 false-positive class.
૮ >ﻌ< ა ci reviewran on 84b5ee6 — fix(gateway): gate record-less visible-text match on _alread
|
Draft frames set _last_sent_text for dedupe without setting _already_sent (they are ephemeral); an ungated has_delivered_text match let a draft-only preview count as durable delivery and regressed test_relay_seal_failure's dead-transport guarantee on CI.
alt-glitch
added
type/bug
|
Thanks for the explicit overlap note and for leaving the call open. For whoever makes it: #100228 is a strict subset of this PR — the If you land #100228 first: I'll rebase it against this branch and drop my 6-test module in favor of your 17-test one so there's no duplication — the recording change itself is identical. One mechanical note from the #100450 salvage: my authorship survived the cherry-pick but was erased by the squash merge, which is why If you supersede: equally fine. The class fix is the better artifact — the Discord Either way your 720-test run covers the 47 existing streaming tests my branch was green against, so nothing is lost in the supersede path. |
3 participants
Infographic
Summary
Fixes #95382 (Discord: silent partial delivery — first stream edit sets
final_response_sent, gateway believes delivery succeeded, message truncated with no re-send) and closes the same false-positive class as #98552 (Telegram:content_delivered=Trueon a 624-char message truncated at 333 chars).Reporter @Lenglemetz confirms #95382 is 100% reproducible post-campaign: the WebSocket drops after the first streaming edit (prefix only), the consumer's delivery flags suppress the gateway's normal final send, and the failed normal send is recorded with a non-retryable error — so the delivery-obligation ledger never replays it either. The full 2668-char response sits correctly in state.db while the user sees a truncated prefix and total silence.
Root cause
Two stacked over-trust gaps:
gateway/stream_consumer.py::delivered_final_matches(~L654) — a delivery flag with no recorded turn-final payload returnedNone(= legacy trust)._stream_confirmed_final_delivery(gateway/run.py~L31036) and the suppression site (~L31859) both treat anything but an explicitFalseas "delivered", so a record-less flag set after a partial delivery suppressed the corrective send. Two flag-setting sites still recorded nothing:_try_fresh_final(~L2936) and the native-streaming optimistic finalize (~L3164).plugins/platforms/discord/adapter.py::send(~L3453) — dead-transport failures returnederror="Not connected"/ raw exception strings. The ledger's runtime reconnect sweep (sweep_failed_for_runtime,gateway/delivery_ledger.pyL394) only replays rows whose error is in_RUNTIME_RETRYABLE_ERRORS = {"send_path_degraded"}— so a Discord final send that failed on a dropped WS was stranded asfaileduntil a full process restart. Telegram already classifies these correctly (adapter.pyL5417).The class fix (delivery judged against final content)
has_delivered_text; only the explicitly-marked ambiguous-timeout path (_delivery_ambiguous, set by_send_empty_fallback_final→"ambiguous") keeps legacy trustgateway/stream_consumer.py_try_fresh_finalrecords its delivered payloadgateway/stream_consumer.pygateway/stream_consumer.pysend_path_degraded(retryable):_client is None+ connection-shaped exceptions (_is_discord_transport_error; timeouts excluded — ambiguous)plugins/platforms/discord/adapter.pyAdapter class table
send_path_degradedclassification (this PR)prefers_fresh_final_streaming/ time-threshold)_try_fresh_finalwas record-less)Falseon payload-less split preserved; regression suite greenLive repro: deterministic before/after harness (
/tmp/repro95382.py, tempHERMES_HOME, realGatewayStreamConsumer, realDiscordAdapter.send, realdelivery_ledgerSQLite):Tests
New
tests/gateway/test_silent_partial_delivery_95382.py(17 tests):GatewayRunner._run_agent+ live consumer, pattern fromtest_stale_finalize_suppression.py): record-less flags must not swallow the reply; dead-transport variant must leavealready_sentunset for the normal send / ledger; honest-streaming control still suppresses exactly once.False; record-less + equal visible →True; ambiguous timeout →None; Suppressing normal final send swallows complete replies on Telegram group/forum sessions (payload-less split-delivery flags final_content_delivered) #78541 split behavior preserved._try_fresh_finalrecords; recorded prefix reads as mismatch._client=None→send_path_degraded; transport vs HTTP/timeout exception classification; real ledger sweep claims the degraded row and leaves a generic"Not connected"row alone.Updated
test_stale_finalize_suppression.py: the oldtest_no_record_returns_nonepinned the exact over-trust this PR removes — replaced with the three-way contract (False / visible-match True / ambiguous None).Sabotage-verified: reverting the matcher branch to
return Nonefails 5 of the new tests; restoring goes green.Targeted suites green (capped
systemd-run --user --scope -p MemoryMax=8G): the two regression modules (34 passed) + delivery ledger/producer/delivery/telegram-final/wecom-double-send/progress-topics (124 passed, 1 xfailed) +tests/gateway -k 'stream or finalize or suppression or fresh_final or split or fallback'(720 passed; singletest_approve_deny_commandsfailure is a pre-existing test-isolation flake — passes in its own module on this branch and fails identically on stashed main in the same batch).Related: #71643 (stale successful finalize — the reconciliation contract this extends), #78541 (split-delivery record), #10748 (mirror case, same over-trust family).
Overlap note: open external PR #100228 (@salch-cred) implements the
_try_fresh_finalpayload-recording sub-fix only (1 of the 4 changes here); maintainer call on whether to land it first for credit and rebase this, or supersede.