Skip to content

fix(photon): run the Spectrum patch spawn off the gateway event loop - #66940

Closed
Frowtek wants to merge 1 commit into
NousResearch:mainfrom
Frowtek:fix/photon-sidecar-patch-off-event-loop
Closed

fix(photon): run the Spectrum patch spawn off the gateway event loop#66940
Frowtek wants to merge 1 commit into
NousResearch:mainfrom
Frowtek:fix/photon-sidecar-patch-off-event-loop

Conversation

@Frowtek

@Frowtek Frowtek commented Jul 18, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?

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 gateway that is actively serving Discord/Telegram/Slack traffic.

Same off-the-loop class as the inbound-image decision (#66688) and the cron-fire verifier.

Related Issue

No separate issue — an event-loop-blocking gateway path.

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)

Changes Made

  • plugins/platforms/photon/adapter.py — in _start_sidecar, dispatch the Spectrum patch subprocess.run via asyncio.to_thread so the node spawn/wait runs on a worker thread, mirroring the dep-reinstall hop directly above it. Behavior, arguments, timeout, and Windows console-hiding flags are unchanged.
  • tests/plugins/platforms/photon/test_sidecar_lifecycle.py — add test_spectrum_patch_runs_off_the_event_loop, asserting the spawn executes on a worker thread rather than the loop thread.

How to Test

  1. Run:

    pytest tests/plugins/platforms/photon/test_sidecar_lifecycle.py -q
    

    The new test passes with the fix and fails without it (it records threading.current_thread() inside a faked subprocess.run and asserts it is not the loop thread).

  2. Full photon suite is unchanged apart from the added test: baseline 4 failed, 107 passed4 failed, 108 passed (the 4 failures are pre-existing on main in this environment and unrelated to this change).

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(photon):)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix (no unrelated commits)
  • I've run pytest tests/plugins/platforms/photon/ -q and verified no regressions against a clean main baseline
  • I've added tests for my changes
  • I've tested on my platform: Ubuntu 24.04

Documentation & Housekeeping

  • I've updated relevant documentation (inline comments) — the call site now documents why it must stay off the loop
  • I've updated cli-config.yaml.example if I added/changed config keys — N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — N/A
  • I've considered cross-platform impact — windows_hide_flags() is still applied; only the dispatch changes
  • I've updated tool descriptions/schemas if I changed tool behavior — N/A

`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 (NousResearch#66688) and the cron-fire verifier.

Adds a regression test asserting the spawn executes on a worker thread
rather than the loop thread.
@alt-glitch alt-glitch added type/bug Something isn't working comp/plugins Plugin system and bundled plugins P3 Low — cosmetic, nice to have sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages labels Jul 18, 2026

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for targeting a verified blocking path: current plugins/platforms/photon/adapter.py:958-971 calls subprocess.run(..., timeout=10) inline from async _start_sidecar, and the proposed asyncio.to_thread at PR line 964 preserves the existing subprocess behavior.

Problems

  • tests/plugins/platforms/photon/test_sidecar_lifecycle.py:246 awaits startup with an always-live fake process but no mocked httpx.AsyncClient. This reaches the real readiness loop in plugins/platforms/photon/adapter.py:1001-1022, can probe localhost for up to 15 seconds, and its broad except Exception hides the timeout.

Suggested changes

  • Mock the health probe to return 200, following the existing test at tests/plugins/platforms/photon/test_sidecar_lifecycle.py:175-184, and remove the broad exception handler so the test completes deterministically.

Automated hermes-sweeper review.

)

try:
await adapter._start_sidecar()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This awaits the full readiness path with an always-live fake process but no mocked httpx.AsyncClient, so it can probe localhost and sleep through the 15-second deadline before this broad handler hides the timeout. Please mock a 200 health response as the existing lifecycle test does above, then await normally.

@teknium1

Copy link
Copy Markdown
Contributor

Merged via #73563 — cherry-picked with authorship preserved (with #66956; the shared test scaffolding was reconstructed as two independent tests).

@teknium1 teknium1 closed this Jul 29, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/plugins Plugin system and bundled plugins P3 Low — cosmetic, nice to have sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform 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.

3 participants