Skip to content

fix(gateway): stop retrying /update notification for platforms disabled in this profile - #87091

Open
RichardHojunJang wants to merge 1 commit into
NousResearch:mainfrom
RichardHojunJang:snow/pr-update-notify-unconfigured-platform-20260815
Open

RichardHojunJang wants to merge 1 commit into
NousResearch:mainfrom
RichardHojunJang:snow/pr-update-notify-unconfigured-platform-20260815

Conversation

@RichardHojunJang

Copy link
Copy Markdown

Summary

A stale .update_pending.json naming a platform the profile does not run pins a permanent retry loop for the life of the gateway.

_send_update_notification treats "no adapter for the target platform" as one case and always defers, preserving the markers so a reconnecting adapter can still be notified. That is correct — and deliberate — for a platform that is enabled and merely still connecting (the behaviour #39091 added).

But config.platforms is pre-seeded with disabled placeholders for the whole platform catalog, so a marker can name a platform with enabled=False. No adapter will ever appear for it. The watcher polls every 2s, and each poll re-reads the marker, logs, and rewrites it — forever.

This PR splits the two cases.

Reproduction

Found in production on a Slack-only profile carrying an orphan marker that named telegram:

gateway.run: Update notification deferred: telegram adapter not connected yet
Measurement Value
Rate 0.50 lines/sec (~43,200/day)
Share of gateway log 9,972 of 12,699 lines (78%)
Duration ~19 days, until manually cleared

The marker was left by an interrupted update: .update_exit_code was 124 (timeout), so the update was long finished — only the unreachable target remained.

Runtime probe of the affected profile confirms the shape:

total platform entries: 6
ENABLED: ['slack']
telegram entry EXISTS: enabled=False  home=None

telegram is present in config.platforms but disabled — so self.adapters.get(platform) returns None on every single poll, permanently.

The fix

Where main has one branch, there are two distinct situations:

  1. Platform enabled, adapter still reconnecting → defer, preserve markers. Unchanged.
  2. Platform not enabled for this profile → unreachable by construction. Consume the markers, log once at WARNING with the reason.

The new _update_target_platform_is_enabled helper fails open: a missing or unreadable config returns True, so an unexpected config shape keeps the existing retry behaviour and can never discard a deliverable notification. Only a positive, readable enabled=False ends the retry.

Mere key presence is explicitly not trusted — the enabled flag is the load-bearing part. This is the same pre-seeded-placeholder trap already documented on _scale_to_zero_active_messaging_platforms ("the F25 bug"), and the helper's docstring points at it so the next reader does not re-learn it.

Test plan

tests/gateway/test_update_command.py
tests/gateway/test_update_streaming.py
tests/gateway/test_restart_after_turn.py
tests/gateway/test_restart_drain.py
tests/gateway/test_lifecycle_ledger.py
tests/gateway/test_internal_notification_marker_82888.py
→ 58 passed, 2 skipped

Three new tests:

Test Pins
test_disabled_platform_marker_is_discarded_not_retried markers consumed, no retry, nothing misdelivered to the wrong platform
test_enabled_platform_still_defers_while_reconnecting the discard path does not over-reach into the legitimate deferral
test_unreadable_platform_config_fails_open_to_deferral a broken config falls back to the old behaviour

Held-in verification — the regression was confirmed to actually pin this bug, not the surrounding behaviour. Reverting only gateway/run.py while keeping the new tests:

FAILED test_disabled_platform_marker_is_discarded_not_retried
        assert False is True
1 failed, 17 passed

The new test fails on the unpatched tree with the exact expected assertion, and the other 17 tests in the class still pass — so it is not an over-broad change detector.

ruff check passes on both files. (ruff format --check reports these files as unformatted both before and after this change, so it is pre-existing and untouched here.)

Relationship to other open PRs

Checked before writing this; all three touch the same markers but none cover this case:

PR Scope Overlap
#42191 SendResult(success=False) soft-failure none — an adapter exists there
#80172 completion survives the restart it reports none — about marker lifecycle across restart
#33282 invalid platform string + corrupt JSON closest, but its guard is Platform(platform_str) raising. A valid platform that is merely disabled passes that check and still defers (its own line 359-361)

This PR is additive to all three: they make delivery more reliable when a target is reachable; this one ends the retry when the target provably is not.

Sensitive data audit

  • Only gateway/run.py and tests/gateway/test_update_command.py changed.
  • Test chat/user IDs are dummy values (111/222), matching the existing convention in this file.
  • The added diff was scanned for token/key/secret patterns, private key blocks, absolute local paths, and identifying strings — no matches. Log-line counts quoted above are aggregate statistics, not log contents.

A stale `.update_pending.json` naming a platform this profile does not run
pins a permanent retry loop for the life of the gateway.

`_send_update_notification` treats "no adapter for the target platform" as a
single case and always defers, preserving the markers so a reconnecting
adapter can still be notified (the behaviour NousResearch#39091 added, which is correct
for a platform that IS enabled and merely still connecting).

But `config.platforms` is pre-seeded with disabled placeholders for the whole
platform catalog, so a marker can name a platform with `enabled=False`. No
adapter will ever appear for it. The watcher then polls every 2s, and each
poll re-reads the marker, logs, and rewrites it -- forever.

Observed on a Slack-only profile carrying an orphan marker that named
telegram: a steady 0.5 lines/sec, ~43k lines/day, which grew to 78% of the
gateway log (9,972 of 12,699 lines) before it was found.

Split the two cases. When the target platform is provably not enabled for
this profile, the target is unreachable by construction: consume the markers
and log once at WARNING with the reason. When it is enabled, keep deferring
exactly as before.

The enabled-check fails open. A missing or unreadable config returns True, so
an unexpected config shape keeps the existing retry behaviour and can never
discard a deliverable notification; only a positive, readable `enabled=False`
ends the retry.

Tests:
- disabled platform: markers consumed, no retry, nothing misdelivered
- enabled but reconnecting: markers preserved (guards the discard path from
  over-reaching)
- unreadable config: fails open to the old deferral

Verified the first test fails on the unpatched tree (`assert False is True`)
and that the other 17 tests in the class still pass there, so it pins this
bug rather than the surrounding behaviour.
@RichardHojunJang
RichardHojunJang force-pushed the snow/pr-update-notify-unconfigured-platform-20260815 branch from 60fd7e9 to 898fca1 Compare August 15, 2026 15:43
@Enough1122

Copy link
Copy Markdown

AI code review — automated review for reference, author can ignore or act on any point.

fix(gateway): stop retrying /update notification for platforms disabled in this profile

  1. Notification permanently lost on temporary disable: the discard path consumes the markers (return True, gateway/run.py line 23463) whenever the target platform is currently enabled=False in this profile's config. If a user disables a platform temporarily (e.g. pauses Telegram for maintenance) while an update notification is pending, the completion result is dropped forever rather than delivered when the platform returns. The retry-loop cost this fixes is real, but a middle ground — e.g. retaining the marker but with a bounded retry count/expiry — would preserve delivery for the re-enable case. At minimum, the log warning could be surfaced as a one-time user-visible notice.
  2. platform type coupling: _update_target_platform_is_enabled indexes config.platforms[platform] (line 23372) — the test passes Platform.TELEGRAM enum keys, but the marker's platform field is a plain string ("telegram"). If the marker ever carries a string while config.platforms is keyed by enum (or vice versa), the lookup silently misses → fails open → deferral (safe direction, but the discard path never fires). A normalization/Platform(platform) conversion would make the match deterministic; the current test only exercises the enum-keyed path.
  3. Fails-open design is good: only a positive, readable enabled=False discards; missing/unknown/unreadable config keeps the historical deferral. Tests cover all three branches well.

@alt-glitch alt-glitch added type/bug Something isn't working comp/gateway Gateway runner, session dispatch, delivery P2 Medium — degraded but workaround exists sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages labels Aug 15, 2026
@teknium1

Copy link
Copy Markdown
Collaborator

Superseded by the age cap in #118312 (merged 09f847d): a pending post-update notice for a platform that never connects is dropped after 1 h instead of being retried every poll, so the stale-marker log spam and rename churn this issue describes no longer occur.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/gateway Gateway runner, session dispatch, delivery P2 Medium — degraded but workaround exists sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants