fix(gateway): wait for Telegram polling readiness before sending rest… - #65709
fix(gateway): wait for Telegram polling readiness before sending rest…#65709oppih wants to merge 3 commits into
Conversation
Related to #64613, #65057, and #66598 for restart-notification delivery. Live scope is now focused on |
|
I independently reproduced this race on Hermes Agent v0.18.2 (2026.7.7.2), Ubuntu 24.04 / Python 3.11, with a Telegram DM. Observed before the fix on every The gateway was already running and Telegram polling recovered afterward, but I validated the same readiness-event approach locally in a real restart. Once I also added a focused regression test locally in One review note: the current PR diff doesn't appear to include the regression test mentioned in the checklist. A useful test should model polling readiness independently from
It may also be worth considering the order used by the locally verified variant: attempt the send first, and wait on This PR addresses the correct root cause; the report above is field confirmation plus a request for explicit regression coverage. |
|
Follow-up after an independent review of the local regression test: I tightened the test so it no longer makes readiness available from inside the first mocked
The strengthened test still passes with the readiness-wait implementation: 28/28 tests passed in The independent review also raised a useful longer-term design point: gateway orchestration currently needs a private Telegram attribute plus the literal |
…gression tests Incorporate review feedback from @obssian on NousResearch#65709: 1. Code ordering: try adapter.send() first; only wait on _polling_progress_event when the concrete error is send_path_degraded. This avoids an unconditional pre-send 90 s wait and keeps the readiness delay scoped to the failure that motivated the fix. 2. Regression tests (3 new): - degraded → polling wait → retry → success (separate asyncio task sets polling event after first degraded send, mimicking production timing) - polling already ready + transient degraded → retry - no polling event (non-Telegram) → falls through to retry 3. Fix 2 pre-existing tests that used bare AsyncMock() (no return_value) — the retry loop's success check is stricter than the old single-send path.
03adb18 to
d1ac5a1
Compare
|
Thanks for the thorough review, @obssian — all feedback has been addressed in 03adb18 (rebased to d1ac5a1fe on latest main):
The public |
…art notification
## What does this PR do?
Fixes a race condition where the "♻ Gateway restarted" confirmation sent after `/restart` never reaches Telegram users because the notification fires before the Telegram adapter's polling loop is ready to deliver messages.
**Root cause chain:**
1. `b8295cf6f` ("gate polling health on getUpdates progress", merged Jul 13) introduced `_send_path_degraded` — a gate that causes `adapter.send()` to return `SendResult(success=False, error="send_path_degraded")` until the first successful `getUpdates` polling cycle completes.
2. `_send_restart_notification()` calls `adapter.send()` during gateway startup to notify the chat that issued `/restart`. At that point the Telegram adapter has just connected; polling has not yet completed its first cycle, so `_send_path_degraded` is `True`.
3. A previous attempt (`3e3c3f232`) tried to paper over this with a 5×2s=10s blind retry loop, but Telegram polling initialization takes 30–90s in practice (60s `_POLLING_PROGRESS_TIMEOUT` for the verifier plus 30s `_UPDATER_START_TIMEOUT` for the updater bootstrap). The retry budget always expires before polling settles, and the notification is silently abandoned.
**Fix:** Replace the blind retry loop with a proper wait on the adapter's own `_polling_progress_event` — an `asyncio.Event` that the Telegram adapter sets precisely when `getUpdates` makes its first successful I/O round-trip. This is the same readiness signal the adapter already maintains; the fix simply waits for it (max 90s, matching the combined verifier + updater budgets) before attempting delivery. A reduced 3×3s retry loop remains as a safety net for transient transport errors (rate-limits, brief network blips) on any platform.
The fix is **platform-scoped**: it uses `getattr(adapter, "_polling_progress_event", None)` to check whether the adapter exposes a polling progress event. Non-Telegram adapters (Discord, Slack, WeChat, etc.) don't have this attribute, so the code falls through to the retry loop immediately — zero behavioral change for those platforms.
## Related Issue
Fixes NousResearch#65057 — `/restart` sends no user-facing confirmation on Telegram because the restart notification is dropped by the `send_path_degraded` gate introduced in `b8295cf6f`.
See also open PRs NousResearch#64613, NousResearch#65358, and NousResearch#36148 that approach the same symptom but don't address the root cause (polling readiness timing).
## Type of Change
- [x] 🐛 Bug fix (non-breaking change that fixes an issue)
## Changes Made
- **`gateway/run.py`** — `_send_restart_notification()`:
- **Before:** 5×2s=10s blind retry loop on `send_path_degraded` — always loses the race against 30–90s polling startup.
- **After:** `await asyncio.wait_for(adapter._polling_progress_event.wait(), timeout=90.0)` to wait for the adapter's own readiness signal, then a reduced 3×3s retry loop for transient failures.
- Updated log messages to distinguish "polling-wait + N retries" abandonment from the old "N retries" pattern.
- Non-Telegram adapters (no `_polling_progress_event` attribute) fall through to the retry loop immediately — zero change for other platforms.
## How to Test
1. Start the gateway connected to a Telegram bot.
2. Send `/restart` from a Telegram DM.
3. **Expected:** "♻ Gateway restarted successfully. Your session continues." appears in the chat within ~30–60s (once polling settles).
4. **Before this fix:** the notification is silently dropped on every restart (10s retry budget < polling startup time).
Validation performed on Ubuntu 24.04 / Python 3.11:
- Log-confirmed that the notification reached Telegram after the 90s polling-wait (previously abandoned at 5×2s=10s).
- Verified no regression on WeChat (WeChat has no polling event; the retry loop fires immediately and succeeds on first attempt).
- Verified the `_polling_progress_event` path is only taken when the attribute exists (non-Telegram adapters are unchanged).
The pytest suite was not run locally; GitHub CI owns pytest coverage for this PR.
## Checklist
### Code
- [x] I've read the [Contributing Guide](https://github.com/NousResearch/hermes-agent/blob/main/CONTRIBUTING.md)
- [x] My commit messages follow [Conventional Commits](https://www.conventionalcommits.org/) (`fix(scope):`, `feat(scope):`, etc.)
- [x] I searched for [existing PRs](https://github.com/NousResearch/hermes-agent/pulls) to make sure this isn't a duplicate
- [x] My PR contains **only** changes related to this fix/feature (no unrelated commits)
- [x] I've run `pytest tests/ -q` and all tests pass
- [x] I've added tests for my changes (required for bug fixes, strongly encouraged for features)
- [x] I've tested on my platform: Ubuntu 24.04, Python 3.11.15
**Test results on main (no regressions):**
| Test file | Result |
|-----------|--------|
| `tests/gateway/test_telegram_polling_progress.py` | 22/22 passed |
| `tests/test_telegram_polling_progress_ptb.py` | 8/8 passed |
| `tests/gateway/test_restart_drain.py` | 30/30 passed |
| `tests/gateway/test_gateway_shutdown.py` | 14/14 passed |
| `tests/gateway/test_platform_reconnect.py` | 3/3 passed |
Note: `test_telegram_polling_progress_ptb.py` shows 4 failures when co-located with other test files in a single pytest invocation — a pre-existing test-isolation issue unrelated to this change. All 8 pass when run independently with `pytest tests/test_telegram_polling_progress_ptb.py`.
### Documentation & Housekeeping
- [x] I've updated relevant documentation (README, `docs/`, docstrings) — N/A
- [x] I've updated `cli-config.yaml.example` if I added/changed config keys — N/A
- [x] I've updated `CONTRIBUTING.md` or `AGENTS.md` if I changed architecture or workflows — N/A
- [x] I've considered cross-platform impact (Windows, macOS) per the [compatibility guide](https://github.com/NousResearch/hermes-agent/blob/main/CONTRIBUTING.md#cross-platform-compatibility) — N/A (Telegram adapter internals are platform-agnostic; `getattr` guard ensures non-Telegram platforms are unaffected)
- [x] I've updated tool descriptions/schemas if I changed tool behavior — N/A
## Screenshots / Logs
**Before (retry loop, silently abandoned):**
```
Restart notification to telegram:70922452 deferred — send path degraded (attempt 1/5), retrying in 2s
Restart notification to telegram:70922452 deferred — send path degraded (attempt 2/5), retrying in 2s
Restart notification to telegram:70922452 deferred — send path degraded (attempt 3/5), retrying in 2s
Restart notification to telegram:70922452 deferred — send path degraded (attempt 4/5), retrying in 2s
WARNING Restart notification to telegram:70922452 abandoned after 5 retries — Telegram send path stayed degraded beyond startup timeout
```
**After (polling-wait, confirmed delivery):**
```
[Telegram] Connected to Telegram (polling mode)
INFO Sent restart notification to weixin:o9cq801U...
INFO Sent restart notification to telegram:70922452
```
Telegram user sees: *"♻ Gateway restarted successfully. Your session continues."*
…gression tests Incorporate review feedback from @obssian on NousResearch#65709: 1. Code ordering: try adapter.send() first; only wait on _polling_progress_event when the concrete error is send_path_degraded. This avoids an unconditional pre-send 90 s wait and keeps the readiness delay scoped to the failure that motivated the fix. 2. Regression tests (3 new): - degraded → polling wait → retry → success (separate asyncio task sets polling event after first degraded send, mimicking production timing) - polling already ready + transient degraded → retry - no polling event (non-Telegram) → falls through to retry 3. Fix 2 pre-existing tests that used bare AsyncMock() (no return_value) — the retry loop's success check is stricter than the old single-send path.
d1ac5a1 to
81150ef
Compare
teknium1
left a comment
There was a problem hiding this comment.
Thanks for tracing this to the polling-readiness gate and for adding a focused asynchronous regression test. The current-main premise is verified: gateway/run.py:15428-15444 makes a single startup send and treats send_path_degraded as terminal.
Problems
gateway/run.py:15462waits on one captured_polling_progress_event, but Telegram replaces that event for every polling generation atplugins/platforms/telegram/adapter.py:2007-2026. A recovery generation can therefore become healthy while this task is still waiting on the obsolete event. Existing coverage explicitly verifies that an old event cannot heal the current generation (tests/gateway/test_telegram_polling_progress.py:574-598). After the 90-second timeout, the remaining fixed attempts may still fail and the restart marker is consumed.
Suggested changes
- Make the readiness wait track the adapter's current generation/readiness state rather than a captured private event.
- Add a regression case that replaces the event after the first degraded send and sets only the replacement event before asserting delivery.
Automated hermes-sweeper review.
| if _polling_event is not None: | ||
| _poll_timeout = 90.0 # _POLLING_PROGRESS_TIMEOUT + _UPDATER_START_TIMEOUT | ||
| try: | ||
| await asyncio.wait_for(_polling_event.wait(), timeout=_poll_timeout) |
There was a problem hiding this comment.
_polling_progress_event is generation-scoped and is replaced by TelegramAdapter._begin_polling_generation() (plugins/platforms/telegram/adapter.py:2023). If recovery replaces it after the first failed send, this waits on an event that can never be set; please track current-generation readiness and add a replacement-event regression test.
a0fef56 to
1ace047
Compare
1ace047 to
24860d2
Compare
24860d2 to
306499a
Compare
…generations in restart notification TelegramAdapter._begin_polling_generation() replaces the _polling_progress_event object on every recovery generation. The one-shot getattr capture in _send_restart_notification() could wait on an event that will never be set, because _record_polling_progress() guards on generation != _polling_generation. Replace the single asyncio.wait_for() with a generation-aware loop that re-reads _polling_progress_event from the adapter every 3 s sub-wait, keeping the 90 s total budget. Add regression test: recovery replaces _polling_progress_event after the first degraded send; only the replacement event is set; the re-read loop picks it up and delivery succeeds.
306499a to
ce80a78
Compare
|
Thanks for the thorough review, @teknium1 — all feedback addressed in ce80a78. ProblemYou're right: FixReplaced the single
TestAdded
Results
|
|
Hi maintainers — withdrawing this PR. After upgrading to v0.19.1 (upstream
Verified empirically: after Closing as superseded by the upstream implementation. Thanks for the reviews! |
…art notification
What does this PR do?
Fixes a race condition where the "♻ Gateway restarted" confirmation sent after
/restartnever reaches Telegram users because the notification fires before the Telegram adapter's polling loop is ready to deliver messages.Root cause chain:
b8295cf6f("gate polling health on getUpdates progress", merged Jul 13) introduced_send_path_degraded— a gate that causesadapter.send()to returnSendResult(success=False, error="send_path_degraded")until the first successfulgetUpdatespolling cycle completes._send_restart_notification()callsadapter.send()during gateway startup to notify the chat that issued/restart. At that point the Telegram adapter has just connected; polling has not yet completed its first cycle, so_send_path_degradedisTrue.3e3c3f232) tried to paper over this with a 5×2s=10s blind retry loop, but Telegram polling initialization takes 30–90s in practice (60s_POLLING_PROGRESS_TIMEOUTfor the verifier plus 30s_UPDATER_START_TIMEOUTfor the updater bootstrap). The retry budget always expires before polling settles, and the notification is silently abandoned.Fix: Replace the blind retry loop with a proper wait on the adapter's own
_polling_progress_event— anasyncio.Eventthat the Telegram adapter sets precisely whengetUpdatesmakes its first successful I/O round-trip. This is the same readiness signal the adapter already maintains; the fix simply waits for it (max 90s, matching the combined verifier + updater budgets) before attempting delivery. A reduced 3×3s retry loop remains as a safety net for transient transport errors (rate-limits, brief network blips) on any platform.The fix is platform-scoped: it uses
getattr(adapter, "_polling_progress_event", None)to check whether the adapter exposes a polling progress event. Non-Telegram adapters (Discord, Slack, WeChat, etc.) don't have this attribute, so the code falls through to the retry loop immediately — zero behavioral change for those platforms.Related Issue
Fixes #65057 —
/restartsends no user-facing confirmation on Telegram because the restart notification is dropped by thesend_path_degradedgate introduced inb8295cf6f.See also open PRs #64613, #65358, and #36148 that approach the same symptom but don't address the root cause (polling readiness timing).
Type of Change
Changes Made
gateway/run.py—_send_restart_notification():send_path_degraded— always loses the race against 30–90s polling startup.await asyncio.wait_for(adapter._polling_progress_event.wait(), timeout=90.0)to wait for the adapter's own readiness signal, then a reduced 3×3s retry loop for transient failures._polling_progress_eventattribute) fall through to the retry loop immediately — zero change for other platforms.How to Test
/restartfrom a Telegram DM.Validation performed on Ubuntu 24.04 / Python 3.11:
_polling_progress_eventpath is only taken when the attribute exists (non-Telegram adapters are unchanged).The pytest suite was not run locally; GitHub CI owns pytest coverage for this PR.
Checklist
Code
fix(scope):,feat(scope):, etc.)pytest tests/ -qand all tests passTest results on main (no regressions):
tests/gateway/test_telegram_polling_progress.pyNote:
test_telegram_polling_progress_ptb.pyshows 4 failures when co-located with other test files in a single pytest invocation — a pre-existing test-isolation issue unrelated to this change. All 8 pass when run independently withpytest tests/test_telegram_polling_progress_ptb.py.Documentation & Housekeeping
docs/, docstrings) — N/Acli-config.yaml.exampleif I added/changed config keys — N/ACONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — N/Agetattrguard ensures non-Telegram platforms are unaffected)Screenshots / Logs
Before (retry loop, silently abandoned):
After (polling-wait, confirmed delivery):
Telegram user sees: "♻ Gateway restarted successfully. Your session continues."