fix(channels/telegram): observability + regression coverage on sidecar reconnect loop (closes #5111)#5321
Conversation
…closes #5111) The pre-migration in-process librefang-channels::telegram adapter exited its polling task on a DNS resolution failure or transient network error and the bridge stayed dead until the daemon was restarted — the regression #5111 reports. The post-#5241 sidecar already wraps _poll_once in a 'while True / except Exception → backoff' loop, so the bridge does retry; this change makes the loop's behaviour explicit and locks it in. - Backoff cap moved from a hardcoded 120.0 to a new MAX_BACKOFF_SECS = 60.0 module constant, matching every sibling polling sidecar (bluesky / discord / line / mastodon / mattermost / nextcloud / ntfy / reddit / rocketchat / twitch). - The WARN line on each backoff now reports retries=<consecutive- failure-count> alongside error + delay, so operators can read 'how long have we been degraded' off a single log line. - On the first successful poll after at least one retry, the loop emits an INFO 'telegram poll recovered retries=N last_backoff=…' so the operator's log timeline shows the reconnect (the issue's 'restored DNS — bridge does NOT recover' complaint was that recovery happened but was invisible). - TimeoutError (the LONGPOLL_SERVER_SECS server-side block expiring with no updates — normal protocol behaviour) explicitly resets both backoff and retries_in_a_row, so an idle channel never drifts toward MAX. Three new pytest cases assert the contract end-to-end: - test_produce_recovers_after_startup_network_failure - test_produce_backoff_is_capped_at_max - test_produce_treats_longpoll_timeout_as_normal All three monkeypatch tg.asyncio.sleep against a saved real-sleep closure to avoid the infinite-recursion footgun (tg.asyncio and the test file's asyncio import point at the same module object). Verified: pytest tests/test_telegram_adapter.py — 26 passed (3 new + 23 existing, all green).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 458f25c8c8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| except TimeoutError: | ||
| backoff = 1.0 | ||
| retries_in_a_row = 0 | ||
| continue |
There was a problem hiding this comment.
Keep transport timeouts on backoff path
TimeoutError is now treated as a normal long-poll idle case, but in this adapter _api_get also raises TimeoutError for real socket/network timeouts (URLError with socket.timeout). With this block, those transport failures no longer emit a warning and no longer go through exponential backoff, so prolonged network degradation can become silent and repeatedly retried without the intended throttle behavior. Please only bypass sleep for true “no updates” responses, and keep network timeouts in the generic error/backoff branch.
Useful? React with 👍 / 👎.
PR review on #5321 caught the entry saying 'bridge stayed dead until daemon restarted — the regression the issue reports' immediately followed by 'The post-#5241 sidecar already wraps...'. The original bug WAS in the Rust adapter and WAS fixed by the #5241 sidecar migration; this PR is the observability + cap-alignment + regression coverage that should have shipped alongside #5241, not the bug fix. Rewrite is explicit on this point and also documents the deliberate deviation from the issue's 'ERROR only after N consecutive failures' suggestion: every sibling polling sidecar reserves log.error for fatal startup-config issues, none escalate during steady-state backoff; adding it here alone would diverge from the family.
Summary
Closes #5111. Important framing: #5111 was filed against
v2026.5.12-beta.11, when telegram was the in-process Rustlibrefang-channels::telegramadapter — that adapter exited itspolling task on a transient network failure and the bridge stayed
dead until the daemon was restarted, exactly as the issue describes.
The #5241 sidecar migration replaced it with the Python sidecar at
sdk/python/librefang/sidecar/adapters/telegram.py, which alreadywraps
_poll_oncein awhile True / except Exception → backoffloop. The reported symptom is therefore already fixed in
main.This PR is not the bug fix — that was #5241. It is the
observability + cap-alignment + regression coverage that should have
shipped alongside #5241 so a future refactor can't reintroduce the
original failure mode invisibly.
Changes
sdk/python/librefang/sidecar/adapters/telegram.py120.0→ newMAX_BACKOFF_SECS = 60.0module constant, matching every siblingpolling sidecar (bluesky, discord, line, mastodon, mattermost,
nextcloud, ntfy, reddit, rocketchat, twitch). Behavioural change
— persistent-outage retries now cap at one-minute intervals
instead of two-minute ones.
retries=<consecutive-failure-count>so degraded duration isreadable off a single log line.
telegram poll recovered retries=N last_backoff=…. Recoveryalready worked; this makes it visible (closes the issue's
"restored DNS — bridge does NOT recover" symptom, which was
actually "recovers silently").
expiring (normal protocol behaviour) explicitly resets both
backoffandretries_in_a_rowandcontinues withoutconsuming the sleep budget — an idle channel never drifts toward
MAX.
Deliberately NOT done
The issue suggested "Log WARN on each retry attempt, ERROR only after
N consecutive failures". Skipped because:
log.errorfor fatalstartup-config issues (
*_required env vars missing,token validation failed,webhook bind failed). None escalateduring steady-state backoff.
librefang.sidecar.runtimealreadyemits
log.error("producer crashed", …)if an exception ESCAPESproduce(); the backoff loop is the layer that prevents thatescape — escalating from inside it would muddy the signal.
provide the "how long degraded / when restored" signals operators
need.
If a maintainer wants ERROR escalation across the whole sidecar
family for parity, that's a separate change to be applied
consistently to all 11 polling sidecars.
Regression guards (3 new pytest cases)
test_produce_recovers_after_startup_network_failure— URLErroron first poll → warn + sleep + retry → success + INFO recovered
test_produce_backoff_is_capped_at_max— consecutive failuresproduce delays =
[1, 2, 4, 8, 16, 32, 60, 60, …], max neverexceeds
MAX_BACKOFF_SECS, the cap is actually reachedtest_produce_treats_longpoll_timeout_as_normal— TimeoutErroralternating with success → loop re-enters without sleeping, no
backoff growth
All three monkeypatch
tg.asyncio.sleepagainst a saved real-sleepclosure to avoid the infinite-recursion footgun (
tg.asyncioand thetest file's
asyncioimport point at the same module object).Verification
Test plan
interval with rising
retries=. Restore the network, observe oneINFO
telegram poll recovered retries=N last_backoff=…. Bridgecontinues operating without daemon restart.