Skip to content

fix(gateway): classify real token locks and keep lifecycle running - #83183

Open
alexgunsberg wants to merge 3 commits into
NousResearch:mainfrom
alexgunsberg:fix/gateway-platform-isolation-v2
Open

fix(gateway): classify real token locks and keep lifecycle running#83183
alexgunsberg wants to merge 3 commits into
NousResearch:mainfrom
alexgunsberg:fix/gateway-platform-isolation-v2

Conversation

@alexgunsberg

Copy link
Copy Markdown

Bug Description

Optional platform startup auth/config failures (production: Buzz relay_membership_required) previously forced gateway exit 78 and stopped scheduler/Kanban. PR #83161 tried to isolate them but was closed after independent BLOCK: production token-lock conflicts are emitted retryable=True and bypassed the fatal classifier, and gateway_state=degraded is neither busy nor drainable.

Root Cause

  1. BasePlatformAdapter._acquire_platform_lock marks live foreign holders retryable=True (for later reconnect). Startup routed solely on that flag, so pure single-writer conflicts stayed alive / retry-queued instead of exit 78.
  2. Multiplex secondary lock failures were logged and disconnected without feeding the fatal classifier.
  3. Persisting lifecycle degraded breaks derive_gateway_busy / derive_gateway_drainable (only running is drainable).

Fix

  • Add is_global_startup_conflict() and classify lock/polling ownership before generic retry routing.
  • Pure global conflicts at zero-connected startup → exit 78; never retry-queue them at startup.
  • Secondary multiplex lock failures use the same classifier.
  • Platform-local auth/config (Buzz membership, missing credentials) parks fatal per-platform; gateway stays alive for cron/Kanban/healthy peers.
  • Lifecycle stays running; per-platform fatal/retrying holds detail (no non-drainable degraded lifecycle).

How to Verify

uv run --extra dev pytest \
  tests/gateway/test_runner_startup_failures.py \
  tests/gateway/test_stale_platform_lock_retryable.py \
  tests/gateway/test_runner_fatal_adapter.py \
  tests/gateway/test_platform_reconnect.py \
  tests/gateway/test_status.py::TestGatewayBusyDerivation \
  tests/gateway/test_multiplex_adapter_registry.py \
  tests/gateway/test_multiplex_pairing_stores.py -q
# 59 passed
uv run --extra dev ruff check gateway/restart.py gateway/run.py \
  tests/gateway/test_runner_startup_failures.py \
  tests/gateway/test_multiplex_adapter_registry.py \
  tests/gateway/test_multiplex_pairing_stores.py

Test Plan

  • Real foreign lock-holder via production _acquire_platform_lock (retryable=True) exits 78, not retry-queued
  • Secondary multiplex token-lock exits 78
  • Buzz-style platform-local auth parks fatal; lifecycle running; Kanban watchers spawn; busy/drain true with active_agents
  • Healthy peer continues when optional adapter fails
  • NS-609 mixed retryable + local nonretryable stays alive
  • Adapter-level lock remains retryable for post-startup reconnect (Bug: Stale Telegram bot token lock permanently kills platform (retryable=False in _acquire_platform_lock) #54167)
  • Existing fatal/reconnect/lock/status/multiplex tests + Ruff pass

Risk Assessment

Low–medium. Narrow startup classification change; pure lock conflicts and multiplex config errors keep exit-78; optional platforms no longer take down duties; lifecycle busy/drain preserved.

Notes

Salvage optional-platform parking from NousResearch#83161 without its unsafe
assumptions.

- Inspect lock/polling conflict semantics before retry routing so
  production _acquire_platform_lock failures (retryable=True) still exit
  78 on pure single-writer conflicts and are not retry-queued.
- Surface multiplex secondary lock_conflict through the same classifier
  instead of silently disconnecting.
- Keep gateway_state=running with per-platform fatal/retrying detail so
  busy/drain contracts remain valid for active cron/Kanban work.
- Park Buzz-style platform-local auth/config failures without killing
  scheduler/Kanban or writing a non-drainable degraded lifecycle state.
@alt-glitch alt-glitch added type/bug Something isn't working P1 High — major feature broken, no workaround comp/gateway Gateway runner, session dispatch, delivery comp/cron Cron scheduler and job management labels Aug 10, 2026
@alexgunsberg

Copy link
Copy Markdown
Author

Release follow-up at exact head 5a44eb7647d42a11f831203df8e0a0f123346b96: 59/59 focused tests + Ruff pass, and the combined current-main release candidate passes its affected suite. Hosted CI and Docker runs exist but are action_required: CI run 31386317816, Docker run 31386316936. The contributor identity attempted the Actions approval endpoint and received 403 Must have admin rights. Maintainer action needed: approve these exact-head fork workflow runs; no rerun or head change requested.

@alexgunsberg

Copy link
Copy Markdown
Author

Independent exact-head review: CHANGES REQUIRED

Reviewed head: 5a44eb7

Three HIGH findings:

  1. gateway/run.py: secondary retryable startup failures can suppress exit 78 without entering any retry registry, leaving a live but deaf gateway.
  2. gateway/restart.py: telegram_connect_error does not inspect the production 409 polling-conflict message, so a duplicate poller remains on the generic retry path.
  3. Buzz/IRC/LINE adapters treat acquire_scoped_lock()'s (False, existing) tuple as truthy, making lock-conflict branches unreachable.

Do not merge or deploy this head. Repair the three findings, add production-shape regressions, rerun focused gateway/status suites, and request a fresh exact-head review.

Repair three HIGH residuals on the platform-isolation candidate:

- Secondary multiplex retryable startup failures enter the profile-scoped
  reconnect registry after _running=True instead of suppressing exit 78
  with a live-but-deaf gateway.
- Classify Telegram production 409 getUpdates conflicts even when wrapped
  as generic telegram_connect_error so zero-connected startup still fails
  closed with exit 78.
- Unpack acquire_scoped_lock()'s (acquired, existing) tuple in Buzz, IRC,
  and LINE so lock-conflict branches are reachable.

Add production-shape regressions for all three defects.
@alexgunsberg

Copy link
Copy Markdown
Author

HIGH-3 residual repair at exact head

Head: 8983942625ebf6dbaf3ab28c9109c977d4a13944
(previous: 5a44eb7647d42a11f831203df8e0a0f123346b96)

Repairs the three independent-review HIGH findings only:

  1. Secondary multiplex retryable startup failures now enter the profile-scoped reconnect registry after _running=True (no live-but-deaf exit-78 suppression without a retry path).
  2. is_global_startup_conflict recognizes production Telegram 409 getUpdates conflicts even under generic telegram_connect_error.
  3. Buzz/IRC/LINE unpack acquire_scoped_lock()'s (acquired, existing) tuple so lock-conflict branches are reachable.

Verification (isolated HERMES_HOME): 166 passed, 2 skipped across focused gateway startup/status/adapter suites; Ruff + compileall + git diff --check clean on touched paths.

No merge/deploy requested — awaiting independent exact-head review.

@alexgunsberg

Copy link
Copy Markdown
Author

Independent exact-head review: CHANGES REQUIRED

Reviewed head: 8983942625ebf6dbaf3ab28c9109c977d4a13944
Base: 03fa32c92dd445eb64c7f67434dd91b32c40701d

The prior three HIGH findings are repaired in their direct paths, but two additional HIGH lifecycle/ownership defects were reproduced:

  1. Secondary retry ownership bypass (gateway/run.py:11359-11365, 13738-13790, 13855-13878): a transiently failed secondary profile does not reserve its credential/listener claim. Another profile can consume the same token, then the first reconnects without rechecking shared ownership; same-process machine locks permit both. Reproduced two active Telegram profiles on one credential.
  2. Initial secondary connect can escape shutdown (gateway/run.py:13781-13786, teardown 13359-13367): if stop() completes while secondary connect() is blocked, the late-success adapter is published into a detached local map and is never disconnected. Reproduced runner_registry={}, orphan_running=True, disconnect_called=False.

Required regressions:

  • A transiently fails, B has the same token, A retries: exactly one profile may consume it; reconnect must share the startup credential/listener ownership registry.
  • Block secondary initial connect, complete shutdown, release connect: exact adapter is disconnected and absent from runner registries.

Independent evidence: 144 focused tests passed, Ruff passed, diff-check passed; two adversarial probes reproduce the blockers. Do not merge/deploy this head. No repository files were modified by review.

Repair two HIGH lifecycle/ownership defects on the platform-isolation
candidate:

- Transient secondary startup failures now reserve credential/listener
  claims in the shared ownership registry so a peer profile cannot
  consume the same Telegram token before reconnect. Reconnect consults
  that registry before any external connect/publication.
- After awaiting secondary initial connect, fence publication on
  startup/shutdown state; if stop() already drained the registry,
  disconnect the late-success adapter and do not register it.

Add adversarial regressions for both defects. Preserve prior HIGH-3
repairs (409 classification, lock-tuple unpack, secondary retry arming).
@alexgunsberg

Copy link
Copy Markdown
Author

HIGH-2 ownership/shutdown repair at exact head

Head: 13fb83231deccb0795eaa81562bd73d315c2007a
(previous: 8983942625ebf6dbaf3ab28c9109c977d4a13944)

Repairs the two independent-review HIGH findings only:

  1. Secondary retry ownership — transient secondary startup failures now reserve credential/listener claims in the shared ownership registry (_profile_retry_resource_claims + startup claimed map). Reconnect consults that registry before any external connect/publication, so a peer profile presenting the same Telegram token cannot create a duplicate poller. Same-process machine locks alone are not relied on.
  2. Initial secondary startup/shutdown race — after awaiting secondary initial connect, publication is fenced on startup/shutdown state (_startup_should_abort). If stop()/registry teardown already completed, the exact late-success adapter is disconnected and never registered.

Prior HIGH-3 repairs preserved: telegram_connect_error 409 classification, Buzz/IRC/LINE lock tuple unpack, and normal secondary retry scheduling.

Adversarial regressions added

  • test_secondary_retry_holds_credential_against_peer_and_reconnect
  • test_secondary_initial_connect_discarded_after_stop_teardown

Verification (isolated HERMES_HOME / worktree venv)

  • New adversarial + multiplex/startup suites: 33 passed
  • Status isolated: 52 passed, 2 skipped
  • Buzz/IRC/LINE/Telegram/multiplex lifecycle/pairing/stale-lock: 89 passed
  • Ruff on touched files: clean
  • python -m compileall gateway/run.py: clean
  • git diff --check: clean

No merge/deploy requested — awaiting independent exact-head review.

@yuzilongleif-collab

Copy link
Copy Markdown
Contributor

I found one additional reconnect ownership race on exact head 13fb83231deccb0795eaa81562bd73d315c2007a.

_run_secondary_profile_reconnect() enters _profile_runtime_scope(profile_home) before calling _secondary_reconnect_claim_conflict(). _collect_adapter_resource_ownership() then labels self.adapters (the primary adapter registry) with the dynamically active profile from get_active_profile_name().

During an alpha retry, that makes primary/default claims look owned by alpha, so a newly reloaded secondary adapter can reuse the primary token/listener without being rejected.

Minimal probe:

runner = GatewayRunner.__new__(GatewayRunner)
runner.adapters = {Platform.TELEGRAM: TokenAdapter("shared-token")}
runner._failed_platforms = {}
runner._profile_adapters = {}
runner._profile_retry_resource_claims = {}
monkeypatch.setattr(
    "hermes_cli.profiles.get_active_profile_name", lambda: "alpha"
)

owner = runner._secondary_reconnect_claim_conflict(
    "alpha", Platform.TELEGRAM, TokenAdapter("shared-token")
)
assert owner == "default"

Actual: owner is None; the reconnect is allowed and its new claim is recorded for alpha.

The primary registry owner needs to be fixed/captured independently of the current secondary profile scope (for example, an explicit runner primary owner established at startup). A regression should cover: primary holds P, alpha is retrying after A, alpha's config changes to P, and the reconnect is refused before any external connect() call. The same owner relabeling applies to listener claims.

@egilewski

Copy link
Copy Markdown
Contributor

too large to review safely

This PR changes 690 production lines before tests and docs. Please split it or add a focused justification if it should stay together.

Signed: GPT-5.6-luna-high in Codex

@alt-glitch alt-glitch added comp/plugins Plugin system and bundled plugins sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages labels Aug 11, 2026
@teknium1

Copy link
Copy Markdown
Contributor

Maintainer review at exact head 13fb83231deccb0795eaa81562bd73d315c2007aCHANGES REQUIRED, not merging at this head.

The direction is right and wanted: pure single-writer token-lock conflicts at zero-connected startup should exit 78 instead of retry-queueing forever, platform-local auth/config failures (Buzz membership, missing credentials) should park fatal per-platform while the lifecycle stays running for cron/Kanban/healthy peers, and avoiding a non-drainable degraded lifecycle state is the correct call (derive_gateway_busy/derive_gateway_drainable only understand running). The HIGH-3 and HIGH-2 repair rounds addressed their findings on their direct paths.

What blocks the merge:

  1. @yuzilongleif-collab's reconnect ownership race is unaddressed at this head. _run_secondary_profile_reconnect() enters _profile_runtime_scope(profile_home) before _secondary_reconnect_claim_conflict(), so _collect_adapter_resource_ownership() labels the primary adapter registry with the dynamically active profile name — during an alpha retry, primary/default claims look owned by alpha and a reloaded secondary adapter can reuse the primary token/listener without rejection (their minimal probe shows owner is None where "default" is expected). This is the same defect class the HIGH-2 round fixed for the startup path, still open on the reconnect path. Please collect primary-registry ownership outside (or before) the profile scope, or pass the owning profile explicitly, and add their probe as a regression.

  2. Review surface. This is now 690 production lines across gateway/run.py, gateway/restart.py and three adapters, grown through three repair rounds. After fixing (1), please also confirm the focused suites listed in the PR body still pass at the new exact head and post the run — the repair-round history makes it easy for a stale claim to linger.

No split demanded — the classifier + lifecycle + lock-tuple changes are one coherent contract and splitting them would leave intermediate heads in the live-but-deaf state the PR exists to eliminate. But (1) is a real duplicate-poller vector and can't ship.

Once (1) is repaired with a regression and the suites re-run at the new head, ping and we'll re-review promptly — the underlying fix is wanted.

@alt-glitch alt-glitch added comp/plugins Plugin system and bundled plugins and removed comp/plugins Plugin system and bundled plugins labels Aug 15, 2026
@Enough1122

Copy link
Copy Markdown
Contributor

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

fix(gateway): classify real token locks and keep lifecycle running

Well-scoped fix with strong regression coverage (real _acquire_platform_lock path, Telegram 409 envelope, secondary multiplex). Observations:

  1. gateway/restart.pyis_global_startup_conflict() treats any error code ending in _lock as a global single-writer conflict (code.endswith("_lock")). That matches the production {scope}_lock families emitted by _acquire_platform_lock, but it will also catch any future unrelated code like db_lock or socket_lock and force exit-78 on a platform-local error. Consider matching against the known lock scopes, or requiring the message to also carry "already in use"-style semantics, to keep the bucket tight.

  2. gateway/run.py — after start(), a failure to schedule a secondary reconnect is logged at DEBUG (logger.debug("Failed to schedule secondary %s reconnect ...")). If scheduling fails for every retryable secondary, the profile is left "live but deaf" with nothing visible in the logs — the same failure mode this PR fixes at startup. A WARNING (at least on repeated failure) would make that observable.

  3. The Buzz/IRC/LINE acquire_scoped_lock tuple-unpack fixes are correct, but the platform-adapter authoring docs (website/docs/developer-guide/adding-platform-adapters.md, if not acquire_scoped_lock("newplat", self._token):) still show the old truthiness bug — a new adapter copy-pasted from the docs will reintroduce it. Update the doc example to unpack (acquired, existing).

  4. _record_startup_adapter_failure() — a retryable fatal error with queue_retry=True but no platform_config (or a bare-exception path with no adapter) is appended to startup_retryable_errors but never queued, and returns False. That suppresses exit-78 while nothing will ever retry. If that combination is reachable from the primary path, log it loudly or refuse to suppress exit-78.

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

Labels

comp/cron Cron scheduler and job management comp/gateway Gateway runner, session dispatch, delivery comp/plugins Plugin system and bundled plugins P1 High — major feature broken, no workaround 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.

6 participants