Skip to content

fix(channels/telegram): observability + regression coverage on sidecar reconnect loop (closes #5111)#5321

Merged
houko merged 2 commits into
mainfrom
fix/5111-telegram-reconnect
May 20, 2026
Merged

fix(channels/telegram): observability + regression coverage on sidecar reconnect loop (closes #5111)#5321
houko merged 2 commits into
mainfrom
fix/5111-telegram-reconnect

Conversation

@houko

@houko houko commented May 20, 2026

Copy link
Copy Markdown
Contributor

Summary

Closes #5111. Important framing: #5111 was filed against
v2026.5.12-beta.11, when telegram was the in-process Rust
librefang-channels::telegram adapter — that adapter exited its
polling 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 already
wraps _poll_once in a while True / except Exception → backoff
loop. 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.py

  1. Backoff cap aligned: hardcoded 120.0 → new
    MAX_BACKOFF_SECS = 60.0 module constant, matching every sibling
    polling 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.
  2. WARN gets retry counter: each backoff WARN now carries
    retries=<consecutive-failure-count> so degraded duration is
    readable off a single log line.
  3. INFO on recovery: first successful poll after ≥1 retry emits
    telegram poll recovered retries=N last_backoff=…. Recovery
    already worked; this makes it visible (closes the issue's
    "restored DNS — bridge does NOT recover" symptom, which was
    actually "recovers silently").
  4. TimeoutError isolated: LONGPOLL_SERVER_SECS server-side block
    expiring (normal protocol behaviour) explicitly resets both
    backoff and retries_in_a_row and continues without
    consuming 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:

  • Every sibling polling sidecar reserves log.error for fatal
    startup-config issues (*_required env vars missing,
    token validation failed, webhook bind failed). None escalate
    during steady-state backoff.
  • The producer-crash path in librefang.sidecar.runtime already
    emits log.error("producer crashed", …) if an exception ESCAPES
    produce(); the backoff loop is the layer that prevents that
    escape — escalating from inside it would muddy the signal.
  • The new WARN-with-retry-counter + INFO-on-recovery already
    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 — URLError
    on first poll → warn + sleep + retry → success + INFO recovered
  • test_produce_backoff_is_capped_at_max — consecutive failures
    produce delays = [1, 2, 4, 8, 16, 32, 60, 60, …], max never
    exceeds MAX_BACKOFF_SECS, the cap is actually reached
  • test_produce_treats_longpoll_timeout_as_normal — TimeoutError
    alternating with success → loop re-enters without sleeping, no
    backoff growth

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).

Verification

pytest sdk/python/tests/test_telegram_adapter.py
# 26 passed (3 new + 23 existing)

Test plan

  • Live: stop the network, observe one WARN every backoff
    interval with rising retries=. Restore the network, observe one
    INFO telegram poll recovered retries=N last_backoff=…. Bridge
    continues operating without daemon restart.
  • CI integration smoke green.

…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).
@github-actions github-actions Bot added the size/M 50-249 lines changed label May 20, 2026

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment on lines 1496 to 1499
except TimeoutError:
backoff = 1.0
retries_in_a_row = 0
continue

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge 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 👍 / 👎.

@github-actions github-actions Bot added area/docs Documentation and guides area/sdk JavaScript and Python SDKs labels May 20, 2026
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.
@houko houko changed the title fix(channels/telegram): bound + log + test the sidecar reconnect loop (closes #5111) fix(channels/telegram): observability + regression coverage on sidecar reconnect loop (closes #5111) May 20, 2026
@houko
houko merged commit 3d23eb3 into main May 20, 2026
29 checks passed
@houko
houko deleted the fix/5111-telegram-reconnect branch May 20, 2026 13:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/docs Documentation and guides area/sdk JavaScript and Python SDKs size/M 50-249 lines changed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug(channels/telegram): bridge does not reconnect after transient network failure

1 participant