mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-07-29 18:46:59 +00:00
fix(photon): run the Spectrum patch spawn off the gateway event loop
`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.
This commit is contained in:
parent
a90a2b3c3c
commit
c0ff5e9169
2 changed files with 75 additions and 1 deletions
|
|
@ -1199,7 +1199,14 @@ class PhotonAdapter(BasePlatformAdapter):
|
|||
from hermes_cli._subprocess_compat import windows_hide_flags
|
||||
|
||||
try:
|
||||
patch = subprocess.run( # noqa: S603
|
||||
# Off the event loop, for the same reason the dep reinstall above
|
||||
# hops to a thread: this spawns node and *waits* for it (up to 10s).
|
||||
# Run inline it holds the shared gateway loop for that whole window,
|
||||
# so every other platform's traffic stalls — and _start_sidecar runs
|
||||
# on every reconnect (connect(is_reconnect=True)), not just startup,
|
||||
# so the stall recurs on a live gateway.
|
||||
patch = await asyncio.to_thread(
|
||||
subprocess.run, # noqa: S603
|
||||
[
|
||||
self._node_bin,
|
||||
str(_SIDECAR_DIR / "patch-spectrum-mixed-attachments.mjs"),
|
||||
|
|
|
|||
|
|
@ -253,3 +253,70 @@ async def test_reap_inspects_listeners_off_the_event_loop(
|
|||
"must be dispatched via asyncio.to_thread so a 5s lsof/ps spawn "
|
||||
"can't freeze every other platform on the gateway loop"
|
||||
)
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_spectrum_patch_runs_off_the_event_loop(
|
||||
monkeypatch: pytest.MonkeyPatch,
|
||||
) -> None:
|
||||
"""The node patch run must not block the shared gateway event loop.
|
||||
|
||||
``_start_sidecar`` spawns the Spectrum patch script and *waits* for it
|
||||
(``timeout=10``). Run inline it holds the loop for that whole window, so
|
||||
every other platform's traffic stalls — and ``_start_sidecar`` runs on
|
||||
every reconnect (``connect(is_reconnect=True)``), not just startup, so the
|
||||
stall recurs on a live gateway. The dep reinstall a few lines above already
|
||||
hops to a thread for exactly this reason; the patch run must too.
|
||||
"""
|
||||
import threading
|
||||
|
||||
adapter = _make_adapter(monkeypatch)
|
||||
main_thread = threading.current_thread()
|
||||
seen: Dict[str, Any] = {}
|
||||
|
||||
# node_modules present + deps fresh, so we reach the patch run.
|
||||
monkeypatch.setattr(photon_adapter.Path, "exists", lambda self: True)
|
||||
monkeypatch.setattr(photon_adapter, "_sidecar_deps_stale", lambda: False)
|
||||
|
||||
async def _no_reap() -> None:
|
||||
return None
|
||||
|
||||
monkeypatch.setattr(adapter, "_reap_stale_sidecar", _no_reap)
|
||||
|
||||
def _fake_run(*a: Any, **k: Any) -> Any:
|
||||
seen["thread"] = threading.current_thread()
|
||||
|
||||
class _Done:
|
||||
returncode = 0
|
||||
stdout = ""
|
||||
stderr = ""
|
||||
|
||||
return _Done()
|
||||
|
||||
monkeypatch.setattr(photon_adapter.subprocess, "run", _fake_run)
|
||||
|
||||
class _FakeProc:
|
||||
pid = 4242
|
||||
stdin = None
|
||||
stdout = None
|
||||
|
||||
def poll(self) -> None:
|
||||
return None
|
||||
|
||||
monkeypatch.setattr(
|
||||
photon_adapter.subprocess, "Popen", lambda *a, **k: _FakeProc()
|
||||
)
|
||||
|
||||
try:
|
||||
await adapter._start_sidecar()
|
||||
except Exception:
|
||||
# Readiness/handshake past the patch run may fail under the fakes —
|
||||
# irrelevant here; we only assert where the patch run executed.
|
||||
pass
|
||||
|
||||
assert seen.get("thread") is not None, "patch run never executed"
|
||||
assert seen["thread"] is not main_thread, (
|
||||
"Spectrum patch subprocess ran on the event-loop thread; it must be "
|
||||
"dispatched via asyncio.to_thread so a 10s node spawn can't freeze "
|
||||
"every other platform on the gateway loop"
|
||||
)
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue