fix(photon): sidecar lifecycle cluster — crash recovery, event-loop, structured errors (salvage x7) - #73563
Merged
Conversation
alt-glitch
added
type/bug
teknium1
force-pushed
the
photon/lifecycle-cluster
branch
from
July 28, 2026 19:24
f8c1a26 to
b23852e
Compare
Contributor
૮ >ﻌ< ა ci reviewran on 8036c2f ℹ️ InfoDesktop E2E visual evidence · View test artifacts · View job1 visual diff. inline evidence upload failed. Failed to upload diff-665a0833239e-onboarding-overlay-diff.png with gh image (exit code 1): Error uploading /home/runner/work/_temp/e2e-evidence/diff-665a0833239e-onboarding-overlay-diff.png: step 0 (get upload token): uploadToken not found on repo page — do you have write access to NousResearch/hermes-agent? (or, if NousResearch enforces SAML SSO, authorize at https://github.com/orgs/NousResearch/sso) |
teknium1
force-pushed
the
photon/lifecycle-cluster
branch
2 times, most recently
from
July 29, 2026 01:23
f7aea21 to
8d0b4ec
Compare
Follow-up to #69112. That PR hardened the shared gateway dispatch path in gateway/run.py against the *caller* being cancelled. A second, self-referential cancellation specific to PhotonAdapter sits one layer underneath it and survived that fix. PhotonAdapter is the only platform adapter that awaits _notify_fatal_error() inline, on the same task that detected the fault. Both _monitor_sidecar_health and _supervise_sidecar run as self._sidecar_health_task / self._sidecar_supervisor_task, and the notification routes into GatewayRunner._handle_adapter_fatal_error_impl, which tears the adapter down via _safe_adapter_disconnect -> disconnect(). disconnect() then cancels self._sidecar_health_task and awaits it -- which, when the health task is what raised the notification, means disconnect() cancels its own caller several plain-await frames up. disconnect()'s `task is not asyncio.current_task()` guard does not catch this. The current task where that guard evaluates is the wrapper _await_adapter_cleanup_with_timeout creates around disconnect() via asyncio.ensure_future, not the health task further up the chain, so the guard passes and the cancel lands. CancelledError stopped subclassing Exception in Python 3.8, so the `except Exception` that wrapped the inline notify call never saw it. The health task died silently mid-handoff: no log line, no "exception never retrieved" warning (cancellation is normal asyncio), and no retry. The platform stayed stranded until the gateway was restarted by hand. Fix: dispatch the notification onto a new task, the same pattern DiscordAdapter._handle_bot_task_done already uses for this reason. disconnect() can then cancel the health/supervisor task freely without that cancellation reaching the code still running the handoff, so the handoff always reaches the reconnect queue. Both Photon fatal call sites are converted: the health-poll path (observed wedging) and the sidecar-crash path (same shape, not yet observed). gateway/run.py is untouched. Observed twice on a self-hosted gateway, ~4h38m and ~52min of silent inbound outage, both cleared only by a manual restart, both post-dating #69112's merge. In each case the fatal log line appears and `queued for background reconnection` never does. Tests: new tests/plugins/platforms/photon/test_fatal_notify_self_cancel.py covers the self-cancellation (fails with CancelledError without this change), that the dispatch does not block its caller, that a failing notification warns rather than raising, and a source guard against reintroducing either inline await. Two assertions in test_overflow_recovery.py that drove these coroutines directly now drain pending tasks before asserting delivery, since the notification is deliberately no longer awaited inline. Prepared with agent assistance and reviewed before submission. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… supervisor task Fixes #73159. When the Photon sidecar exits unexpectedly, _supervise_sidecar() (running as self._sidecar_supervisor_task) correctly detects SIDECAR_CRASHED and calls self._notify_fatal_error(). The Gateway's fatal-error handler answers that by calling adapter.disconnect(), which calls _stop_sidecar() -- from INSIDE the very task that's currently executing this whole chain. _stop_sidecar()'s cleanup unconditionally cancelled self._sidecar_supervisor_task. Cancelling the currently-running task raises CancelledError at its own next await point (inside _notify_fatal_error() or _stop_sidecar() itself). Since asyncio.CancelledError inherits from BaseException (not Exception), the Gateway's `except Exception` guards around the fatal-error handler don't catch it -- the handler aborts before ever reaching the "queue platform for background reconnection" step. Photon then stays permanently in `retrying` state until the whole process is manually restarted, even though detection worked correctly and the underlying transient upstream outage had long since recovered. Fix: in _stop_sidecar()'s cleanup, check whether self._sidecar_supervisor_task is asyncio.current_task() before cancelling it. A task cannot legally cancel itself in any useful way anyway (the cancellation only takes effect at its own next await, which is exactly the corruption described above) -- when we're inside the supervisor's own call stack, it's already in the process of finishing on its own once _notify_fatal_error() returns, so skip the cancel and just clear the reference. This mirrors an existing precedent in the same file: disconnect() already guards its OTHER task-cancellation (self._sidecar_health_task) the same way (`if task is not asyncio.current_task()`), just not the supervisor task in _stop_sidecar(). Per the issue's own note that existing tests mock out _notify_fatal_error() entirely (so this integration chain was never exercised), added two tests that drive the REAL chain: one runs _supervise_sidecar() as an actual asyncio task with a real _notify_fatal_error() that calls the real disconnect() -> _stop_sidecar(), confirming the task completes without CancelledError and that the post-disconnect reconnect-queue step actually executes; a second confirms the OTHER call path (external cleanup, a different task) still correctly cancels a running supervisor exactly as before. Reverting only the adapter.py fix (keeping the new test) reproduces the exact CancelledError from the bug report, confirming this is a genuine regression test. 2 new tests pass; 113/113 in the full tests/plugins/platforms/photon/ directory (no regression to sidecar lifecycle, overflow recovery, or health-monitoring behavior).
…isconnect() Follow-up widening of the #73170 pattern: apply the same asyncio.current_task() guard used for the health task (and now the supervisor task in _stop_sidecar) to the inbound-task cancel path in disconnect(), so an inbound task that triggers disconnect() cannot cancel-and-await itself.
`_reap_stale_sidecar` is `async`, but it identified the processes holding the sidecar port with two blocking helpers called inline: * `_find_listener_pids` -> `subprocess.run(["lsof", ...], timeout=5.0)` * `_pid_is_sidecar` -> `subprocess.run(["ps", ...], timeout=5.0)`, once per candidate pid so the inspection can hold the shared gateway loop for 5 + 5·N seconds while nothing else on it is serviced. It only runs once the /healthz probe finds something already listening — the orphaned-sidecar recovery path — and `_reap_stale_sidecar` is awaited from `_start_sidecar`, which runs on every reconnect (`connect(is_reconnect=True)`). The stall therefore lands on a live gateway that is still serving every other platform, right when a crashed sidecar has already left an orphan behind. Move the whole inspection to one `asyncio.to_thread` hop (one hop rather than N+1 round trips). The reaping semantics are untouched: SIGTERM for verified orphans, SIGKILL escalation, and both foreign-listener RuntimeErrors behave exactly as before. Same off-the-loop class as the inbound-image decision (#66688) and the cron-fire verifier. Adds a regression test asserting both the lsof lookup and the per-pid ps check execute on a worker thread rather than the loop thread.
`PhotonAdapter._start_sidecar` is `async`, but it ran the Spectrum
mixed-attachment patch script with a bare `subprocess.run(...)`: it spawns
node and *waits* for it, with `timeout=10`. Executed inline that holds the
shared gateway event loop for the whole window, so no other platform's
messages, heartbeats, or sessions are serviced until it returns.
The same function already establishes this exact invariant twenty lines
above, where the stale-dependency reinstall hops to a worker thread:
# Runs off the event loop so a cold install can't freeze every other
# platform's traffic.
if _sidecar_deps_stale():
await asyncio.to_thread(_reinstall_sidecar_deps)
The patch spawn never got the same treatment. It is not startup-only
either — `_start_sidecar` is called from `connect()`, which takes
`is_reconnect`, so an ordinary Photon reconnect (network blip, sidecar
death) re-runs it and stalls a live gateway that is actively serving
Discord/Telegram/Slack traffic.
Dispatch it via `asyncio.to_thread` like its sibling. Same off-the-loop
class as the inbound-image decision (#66688) and the cron-fire verifier.
Adds a regression test asserting the spawn executes on a worker thread
rather than the loop thread.
Photon's Node sidecar intentionally hides raw handler exceptions, but the Python adapter still needs a safe failure class and retryability bit so delivery retries do not collapse into an opaque generic 500. Constraint: Sidecar responses must not leak raw stack traces or private exception text Rejected: Retry every internal sidecar error | masks permanent auth/config failures Confidence: high Scope-risk: narrow Directive: Keep sidecar error text generic; extend safe error classes instead of exposing raw SDK failures Tested: uv run --with pytest-timeout pytest tests/plugins/platforms/photon/test_overflow_recovery.py -q Tested: uv run --with pytest-timeout pytest tests/plugins/platforms/photon -q Tested: uv run ruff check plugins/platforms/photon/adapter.py tests/plugins/platforms/photon/test_overflow_recovery.py Tested: python3 -m py_compile plugins/platforms/photon/adapter.py tests/plugins/platforms/photon/test_overflow_recovery.py Tested: node --check plugins/platforms/photon/sidecar/index.mjs Tested: git diff --check Tested: python3 scripts/check-windows-footguns.py --diff origin/main Not-tested: Live Photon/Spectrum delivery against a real iMessage account Related: #50971
…owed Maintainer follow-up to the #51193 salvage: - _send_with_retry: permanent classes (auth_or_config, target_not_allowed) now short-circuit BEFORE the unconditional plain-text fallback resend, including when a retry attempt surfaces one — no more double-sends of permanently-failing requests. - sidecar classifySidecarError: new structured code target_not_allowed for Spectrum's 'Target not allowed for this project' AuthenticationError (shared/free-tier lines cannot initiate outbound sends to new targets). Classification applies to every handler sharing the catch-all serverError path (/send, /send-attachment, /react, /typing, ...). - _standalone_send now parses the structured error body too (it reads sidecar responses independently of _sidecar_call) and returns error_class/retryable alongside the message. - target_not_allowed maps to a canonical user-facing message in both paths; raw upstream error text never leaks through the structured code. Closes the actionable halves of #50971, #51897, #52794.
teknium1
force-pushed
the
photon/lifecycle-cluster
branch
from
July 29, 2026 04:30
8d0b4ec to
8036c2f
Compare
This was referenced Jul 29, 2026
This was referenced Jul 29, 2026
Closed
Closed
Closed
This was referenced Jul 29, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Consolidated Photon sidecar lifecycle salvage — the crash-recovery cluster. Sidecar failures now recover instead of stranding the platform: no more self-cancelled supervisors, fatal notifications that die with their caller, patch-failure boot aborts, event-loop stalls during spawn/reap, multi-profile port storms, or permanent auth failures retried (and double-sent) as generic 500s.
Salvages seven contributor PRs onto current main with authorship preserved: #61868 (@s00rz), #58047 (@tianma-if), #72475 (@tom-channel), #73170 (@ygd58), #66956 + #66940 (@Frowtek), #51193 (@westkite1201) — plus maintainer widening commits.
Changes
_handle_bot_task_donepattern) — reconnect handoff can't be cancelled mid-flight.asyncio.current_task()guard so_stop_sidecarcan't cancel its own supervisor. Maintainer widening: same guard on the inbound-task cancel indisconnect()(same class).subprocess.runmoved off the gateway event loop.internal sidecar error;auth_or_configreturns before the fallback send (no more double-send on permanent failures); classification widened to/send-attachment,/react,/typingand_standalone_send; upstreamTarget not allowedmapped to a structured non-retryabletarget_not_allowedwith a clear shared-line message (no raw provider text leaked).Validation
Fixes #73159. Fixes #50971. Fixes #52794 (outbound half; Desktop-stale half tracked separately).
Infographic