fix(gateway): wait for Telegram adapter degraded state before startup notifications - #66598
kevin-lucifer wants to merge 3 commits into
Conversation
Implements #66589. This bounded pre-dispatch degraded-state wait overlaps the restart-readiness work in #65709 and generic lifecycle retry in #64613, but changes a different notification stage and has broader adapter scope; linked as related rather than duplicate for maintainer selection. |
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Comment
Critical
- None
Warnings
- None
Assessment
Fixes startup notifications being rejected with "send_path_degraded" (#66589) by waiting up to 10s for Telegram's degraded send path to clear before sending startup notifications. The wait loops over adapters with _send_path_degraded attribute, checking every 500ms up to 20 iterations.
Looks Good
- Correct fix: waits for the adapter to finish its first successful
getUpdatesbefore sending notifications - Reasonable timeout (10s) with graceful break
- Targets only adapters that actually use degraded send gate
Reviewed by Hermes Agent
teknium1
left a comment
There was a problem hiding this comment.
Thanks for targeting a real Telegram restart-notification race. The current approach needs strengthening before it reliably closes that race.
Problems
gateway/run.py:7584caps the new wait at 10 seconds, but Telegram only clears the gate after getUpdates progress (plugins/platforms/telegram/adapter.py:2028-2039), and its verifier permits 60 seconds (plugins/platforms/telegram/adapter.py:2623-2639). A longer startup still reaches the failed send path and_send_restart_notification()removes the marker ingateway/run.py:15455.- The diff contains no regression test. Existing notification-helper tests do not exercise
GatewayRunner.start()with delayed adapter readiness.
Suggested changes
- Use readiness handling aligned with the Telegram polling lifecycle, or defer/retry an explicitly retryable lifecycle send while retaining a redelivery path.
- Add a startup-path regression test for readiness arriving after the one-second settle delay and for the bounded-not-ready case.
Automated hermes-sweeper review.
| # rejected with "send_path_degraded" (#66589). | ||
| for platform, adapter in self.adapters.items(): | ||
| if hasattr(adapter, '_send_path_degraded'): | ||
| for _ in range(20): # max 10s |
There was a problem hiding this comment.
This 10-second cap is shorter than Telegram's own 60-second getUpdates-progress verifier (plugins/platforms/telegram/adapter.py:2623-2639). If the flag remains set after this loop, the following lifecycle send still returns send_path_degraded and _send_restart_notification() removes its marker. Please use readiness/retry handling that preserves a delivery path when startup takes longer.
ba63e4c to
fc38137
Compare
|
Review feedback addressed in fc38137fc: 1. Timeout too short (10s vs verifier's 60s) 2. No regression test
Also: the wait now only runs when a lifecycle notification is actually pending ( |
|
Independent production reproduction on current Before patching, all three profiles followed the same sequence after a planned restart: The gateways were otherwise healthy and accepted Telegram messages after polling recovered. This confirms the issue is specifically lifecycle delivery racing the first successful I tested a narrower dispatch-boundary variant locally: retry only when That suite covered One remaining reliability gap in the current PR head ( I suggest pairing the readiness wait with one of these terminal guarantees:
The event-driven wait is useful. The key is not deleting the only durable notification record when its final send was explicitly rejected before transmission. |
|
Follow-up addressed in 5ebb7de1d, adopting @arcabotai's option 1 (retain the marker) paired with a bounded in-process redelivery path: Marker retention on refused sends
Deferred redelivery
Tests — 8 new cases in |
|
Independent reproduction confirming this race and the need for the Environment:
Observed lifecycle:
Source inspection of the installed tree also confirmed that This independently validates that the readiness wait alone only narrows |
… notifications The gateway's startup notification was failing with 'send_path_degraded' after planned restarts on networks where Telegram's first getUpdates takes longer than 1 second (e.g. when fallback IPs are needed). Root cause: The Telegram adapter's _send_path_degraded flag is set during polling initialization and only cleared after the first successful getUpdates. The gateway only waited 1.0s before sending startup notifications, which was insufficient on networks requiring DoH fallback discovery. Fix: When a lifecycle notification is pending, wait for adapters with a degraded-send gate to become ready before sending. The wait is event-driven via the adapter's _polling_progress_event (re-fetched each round in short slices, since Telegram recreates the event every polling generation) with a 0.5s polling fallback, and is bounded at 65s to stay aligned just above the Telegram polling verifier's 60s budget. Normal startup with no pending lifecycle notification pays nothing. Adds regression tests for readiness arriving after the settle window, the bounded not-ready case, and event recreation across polling generations. Fixes NousResearch#66589 Related: NousResearch#65057
…y deferred The startup readiness wait narrows the NousResearch#66589 race but does not close it: after the 65s deadline the notification is sent anyway, and when the adapter still refuses with send_path_degraded the marker was unlinked in the finally block — silently dropping the only durable record of a promised notification. A send_path_degraded rejection means the message never left the process, so treat it differently from a genuine send failure: - _send_restart_notification() keeps .restart_notify.json when the send is refused with send_path_degraded (retain_marker_on_degraded=True). - _send_home_channel_startup_notifications() counts degraded rejections; start() keeps .restart_pending.json when nothing was delivered and at least one send was refused. - After startup, _schedule_deferred_lifecycle_retry() delivers retained markers in the background once the degraded send paths clear (bounded at 120s). The retry is last-chance: markers are cleared afterwards either way so a stale file can't fire a spurious 'gateway restarted' message after a future unrelated restart. Non-degraded failures keep the existing cleanup behavior.
Follow-up to the marker-retention commit, closing the gap triage called out on NousResearch#66589: the planned-restart marker now survives when ANY home-channel send is rejected with send_path_degraded, even if another target succeeded. To make partial retention safe, the deferred retry skips targets already delivered at startup (tracked via _last_home_notify_delivered) instead of re-sending to every home channel. Also guard against a duplicate-send race: when boot-path sends outlive the restore-gate drain timeout (NousResearch#91969) and continue in the background, the deferred lifecycle retry now waits for the boot task before touching markers, and bows out if it is still running after the bounded wait. New tests: - partial degraded refusal retains the planned marker while recording the delivered target - deferred retry skips already-delivered home targets - deferred retry waits for an in-flight boot send and does not double-send
5ebb7de to
b0859f8
Compare
|
Rebased onto current main (was ~8.7k commits behind) and integrated with the Two follow-ups from the triage thread are addressed in the same push: 1. Multi-target marker gap 2. Boot-send race guard Tests: 3 new cases (partial degraded refusal retains marker + records delivered targets; deferred retry skips delivered targets; deferred retry waits out an in-flight boot send). |
What
Fixes a race condition where Telegram startup notifications fail with
send_path_degradedafter planned gateway restarts.Root Cause
The Telegram adapter's
_send_path_degradedflag is set during polling initialization and only cleared after the first successfulgetUpdates. On networks requiring fallback IPs (DoH discovery + retry), this can take >1 second. The gateway only waited1.0sbefore sending startup notifications, so the send was rejected.Fix
After the initial 1.0s settle wait, poll all adapters for the
_send_path_degradedflag and wait until it clears (max 10s) before sending startup notifications.Testing
test_restart_command_uses_detached_without_systemd) is environment-related (systemd detection in containers) and fails both before and after this changeRelated Issues