Conversation
alt-glitch
added
type/bug
|
suggesting changes The hold lifecycle still has two event-loss/orphaning gaps:
The disconnect-to-reconnect positive path, bounded queue, identity dedupe, teardown salvage, and mid-drain disconnect preservation all pass, but the paths above still violate the no-destruction/eventual-redispatch invariant. Security evidence:
Signed: GPT-5.6-sol-xhigh in Codex |
|
Addressed in 221a6dfb5:
Regression tests added for the three residual paths. |
|
looks mergeable The Telegram adapter now preserves authorized text, photo, and media-group events when delayed delivery races a disconnect, drains them only after a live reconnect, bounds retention, and explicitly discards them on permanent fatal states. The changed producers and teardown paths converge on the existing message-delivery sink; no new authorization, credential, prompt-routing, or data-exposure bypass was found. Security evidence:
Not checked:
Signed: GPT-5.6-luna-max in Codex |
…troying them The disconnect drop-guard (NousResearch#55971) correctly prevents dispatch into a torn-down session. Destroying the event was wrong: by enqueue/flush time python-telegram-bot has already acked the update and advanced the polling offset, so Telegram never redelivers. Result: silent permanent loss, no log, no error. Hold inbound events (text/photo/media-group) when the drop-guard fires, salvage pending batch maps on teardown, cancel+await the redispatch task in the delivery cancel map (lifecycle-tracked), and redispatch from _mark_connected after reconnect. Cap the hold queue (default 64), dedupe by object identity, discard on non-retryable fatal. Cancel-after-pop in flush paths also holds. Distinct from NousResearch#72037 (cancel-after-pop during follow-up supersession) and NousResearch#81528 (boundary discard). Tests use delay=0 and entered/release Events — no wall-clock races; includes production terminal-step coverage.
…cted drain Address review on NousResearch#83878: - Permanent fatal fences all hold producers and discards pending maps on teardown instead of re-populating a queue that can never drain. - Any hold created while connected schedules a tracked redispatch (cancel- after-pop no longer orphans until a future reconnect). - Redispatch failures re-hold current + remainder without tight-looping. Regression coverage for the three residual paths, plus the interaction with OOF-156's connect-failure classification: the retryable network path (telegram_connect_error) must NOT clear the hold queue — reconnect is precisely what drains it; only non-retryable fatals discard.
221a6df to
0e1c2e2
Compare
|
Rebased onto current main ( One upstream change since this PR opened is worth calling out: OOF-156 (
Added a regression test pinning both sides of that interaction ( Verification on the rebased branch: Full suite run against a pristine-main baseline in the same environment: zero delta introduced by this branch. Anything else needed to move this forward? |
…cted drain Address review on #83878: - Permanent fatal fences all hold producers and discards pending maps on teardown instead of re-populating a queue that can never drain. - Any hold created while connected schedules a tracked redispatch (cancel- after-pop no longer orphans until a future reconnect). - Redispatch failures re-hold current + remainder without tight-looping. Regression coverage for the three residual paths, plus the interaction with OOF-156's connect-failure classification: the retryable network path (telegram_connect_error) must NOT clear the hold queue — reconnect is precisely what drains it; only non-retryable fatals discard.
Pre-existing inconsistency: _flush_media_group_event used return in CancelledError while _flush_text_batch and _flush_photo_batch used raise. Changed to raise for consistency and to properly propagate task cancellation. Made more visible by the hold-queue changes in #83878.
|
Merged via #86399 — your commits cherry-picked with authorship preserved via rebase-merge. Your fix for the silent message-loss bug is now on main. The disconnect drop-guard (#55971) was correct to prevent dispatch into a torn-down session, but destroying the event was wrong — PTB had already acked the update, so Telegram never redelivered. Your hold-and-redispatch mechanism solves this cleanly. A small follow-up on top: standardized the media-group flush Thanks for the thorough work — 16 tests with no wall-clock races, comprehensive lifecycle coverage including the permanent-fatal path. |
…cted drain Address review on NousResearch#83878: - Permanent fatal fences all hold producers and discards pending maps on teardown instead of re-populating a queue that can never drain. - Any hold created while connected schedules a tracked redispatch (cancel- after-pop no longer orphans until a future reconnect). - Redispatch failures re-hold current + remainder without tight-looping. Regression coverage for the three residual paths, plus the interaction with OOF-156's connect-failure classification: the retryable network path (telegram_connect_error) must NOT clear the hold queue — reconnect is precisely what drains it; only non-retryable fatals discard.
Pre-existing inconsistency: _flush_media_group_event used return in CancelledError while _flush_text_batch and _flush_photo_batch used raise. Changed to raise for consistency and to properly propagate task cancellation. Made more visible by the hold-queue changes in NousResearch#83878.
…cted drain Address review on NousResearch#83878: - Permanent fatal fences all hold producers and discards pending maps on teardown instead of re-populating a queue that can never drain. - Any hold created while connected schedules a tracked redispatch (cancel- after-pop no longer orphans until a future reconnect). - Redispatch failures re-hold current + remainder without tight-looping. Regression coverage for the three residual paths, plus the interaction with OOF-156's connect-failure classification: the retryable network path (telegram_connect_error) must NOT clear the hold queue — reconnect is precisely what drains it; only non-retryable fatals discard.
Pre-existing inconsistency: _flush_media_group_event used return in CancelledError while _flush_text_batch and _flush_photo_batch used raise. Changed to raise for consistency and to properly propagate task cancellation. Made more visible by the hold-queue changes in NousResearch#83878.
…cted drain Address review on NousResearch#83878: - Permanent fatal fences all hold producers and discards pending maps on teardown instead of re-populating a queue that can never drain. - Any hold created while connected schedules a tracked redispatch (cancel- after-pop no longer orphans until a future reconnect). - Redispatch failures re-hold current + remainder without tight-looping. Regression coverage for the three residual paths, plus the interaction with OOF-156's connect-failure classification: the retryable network path (telegram_connect_error) must NOT clear the hold queue — reconnect is precisely what drains it; only non-retryable fatals discard.
Pre-existing inconsistency: _flush_media_group_event used return in CancelledError while _flush_text_batch and _flush_photo_batch used raise. Changed to raise for consistency and to properly propagate task cancellation. Made more visible by the hold-queue changes in NousResearch#83878.
4 participants
Summary
Current main's disconnect drop-guard (#55971) correctly refuses to dispatch buffered Telegram updates into a torn-down session (
_should_drop_delayed_delivery/_drop_delayed_deliveries).The implementation still destroys the inbound event: debug-level
returnafterpop()(or before enqueue) at the text/photo/media-group batch sites, and_cancel_pending_delivery_tasksclears pending maps with no salvage. By then python-telegram-bot has already accepted the update and advanced the polling offset, so Telegram will never redeliver. Result: silent permanent loss — no WARNING, no error, no retry.Operator signature: messages visible in Telegram (later quoted via
reply_to_id) never appear as gateway inbound or instate.dbfor that turn — consistent with drop-on-enqueue while the drop-guard is true during polling recovery.Problem
_enqueue_text_event_flush_text_batchpop→ drop → return (event gone); cancel after pop also lost_enqueue_photo_event_flush_photo_batchpop→ drop → return; cancel after pop lost_queue_media_group_event_flush_media_group_eventpop→ drop → return; cancel after pop lost_cancel_pending_delivery_tasksInvariant preserved: still never call
handle_messageinto a torn-down session.Fix
Hold, don't destroy. Lifecycle-track redispatch. Drain on reconnect.
_hold_inbound_event(event, where=...)— WARNING,HELD_INBOUND_MAX=64, identity dedupCancelledErrorafterpopholds_cancel_pending_delivery_tasks:_held_inbound_redispatch_task(same lifecycle rule as fix(telegram): shield batch flush from follow-up cancel #72037 review: no untracked dispatch after teardown starts)_mark_connectedschedules one tracked_redispatch_held_inbound(prior=...)Interaction with OOF-156 (connect-failure classification)
Rebased onto current main (zero textual conflicts), which adds
retryable=Falsefatals forInvalidToken/Forbiddenat connect time (telegram_auth_error). The two mechanisms compose by design:telegram_connect_error, network)telegram_auth_error, revoked token)Covered by regression tests on both paths.
Scope boundary
popduring normal supersessionHeld queue is not cleared by conversation boundaries; that remains #81371's job if fragments must die on
/new//stop.Test plan
16 tests in
TestHoldInboundAcrossReconnect— no wall-clock races (delay=0 and/or entered/releaseasyncio.Event):_enqueue_text_eventas_handle_text_messageends) holds when disconnectedFull suite verified against a pristine-
mainbaseline in the same environment: zero delta introduced by this branch (remaining failures are identical pre-existing environment failures — daytona/fal/hindsight/acp extras not installed — present on both sides).