mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-07-29 18:46:59 +00:00
fix(photon): stop sidecar-crash fatal handler from cancelling its own 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).
This commit is contained in:
parent
3c7cff843d
commit
74939d2bfe
2 changed files with 153 additions and 1 deletions
|
|
@ -1336,7 +1336,31 @@ class PhotonAdapter(BasePlatformAdapter):
|
|||
# standalone senders don't chase a dead port/token.
|
||||
_delete_runtime_record()
|
||||
if self._sidecar_supervisor_task is not None:
|
||||
self._sidecar_supervisor_task.cancel()
|
||||
# _stop_sidecar() is called both from external cleanup
|
||||
# (Gateway shutdown, explicit disconnect) AND, indirectly,
|
||||
# from WITHIN the supervisor task's own crash-handling
|
||||
# chain: _supervise_sidecar() detects the sidecar exit,
|
||||
# calls _set_fatal_error() + self._notify_fatal_error(),
|
||||
# which the Gateway's fatal-error handler answers by
|
||||
# calling adapter.disconnect() -> this same _stop_sidecar().
|
||||
# In that second case, self._sidecar_supervisor_task IS the
|
||||
# currently-running task. Cancelling it raises
|
||||
# CancelledError into its own call stack (at the next
|
||||
# await point in _notify_fatal_error() or here), which
|
||||
# aborts the fatal-error handler before the Gateway ever
|
||||
# reaches the "queue for background reconnection" step --
|
||||
# Photon then stays permanently dead until a manual
|
||||
# restart, since asyncio.CancelledError inherits from
|
||||
# BaseException (not Exception) and isn't caught by the
|
||||
# handler's `except Exception` guards (issue #73159).
|
||||
# A task cannot legally cancel itself anyway (the
|
||||
# cancellation would only take effect at its own next
|
||||
# await, which is exactly the corruption described above),
|
||||
# so skip it here and let the task finish exiting on its
|
||||
# own instead.
|
||||
current_task = asyncio.current_task()
|
||||
if self._sidecar_supervisor_task is not current_task:
|
||||
self._sidecar_supervisor_task.cancel()
|
||||
self._sidecar_supervisor_task = None
|
||||
|
||||
# -- Outbound ----------------------------------------------------------
|
||||
|
|
|
|||
|
|
@ -260,3 +260,131 @@ async def _drain_pending_tasks(limit: int = 50) -> None:
|
|||
if not pending:
|
||||
return
|
||||
await asyncio.wait(pending, timeout=1.0)
|
||||
|
||||
|
||||
# -- Gap 5: self-cancellation race in _stop_sidecar() (issue #73159) --------
|
||||
#
|
||||
# The tests above mock out _notify_fatal_error() entirely, so the real
|
||||
# integration chain (supervisor task -> _notify_fatal_error() ->
|
||||
# disconnect() -> _stop_sidecar() -> cancel the supervisor task) is never
|
||||
# exercised end to end. That chain is exactly where the bug lives: when
|
||||
# _notify_fatal_error() is a real callback that calls adapter.disconnect(),
|
||||
# _stop_sidecar() is invoked FROM WITHIN the currently-running supervisor
|
||||
# task, and cancelling self._sidecar_supervisor_task there cancels the very
|
||||
# task executing the fatal-error handler -- aborting it (via CancelledError,
|
||||
# a BaseException the handler's `except Exception` guards don't catch)
|
||||
# before the Gateway's reconnect-queue step ever runs.
|
||||
|
||||
class _FakeStoppedProc:
|
||||
"""Minimal proc stand-in so _stop_sidecar() reaches its finally block
|
||||
(it early-returns entirely when self._sidecar_proc is None) without
|
||||
spawning a real subprocess or doing any real I/O."""
|
||||
|
||||
def __init__(self) -> None:
|
||||
self.stdin = None
|
||||
|
||||
def wait(self, timeout: float | None = None) -> int:
|
||||
return 0
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_supervisor_task_survives_self_triggered_disconnect(
|
||||
monkeypatch: pytest.MonkeyPatch,
|
||||
) -> None:
|
||||
"""The real chain: _supervise_sidecar() (running as the actual
|
||||
self._sidecar_supervisor_task) detects the crash and calls a REAL
|
||||
_notify_fatal_error() that calls adapter.disconnect() -- which reaches
|
||||
_stop_sidecar() from inside the task it's about to try to cancel.
|
||||
|
||||
Before the fix: this raises CancelledError out of _supervise_sidecar(),
|
||||
so the task ends up in the "cancelled" state and reconnect_queued below
|
||||
is never set (mirroring how the real Gateway's fatal-error handler,
|
||||
which runs the reconnect-queue logic AFTER disconnect() returns, never
|
||||
gets there either).
|
||||
|
||||
After the fix: disconnect() completes normally, _supervise_sidecar()
|
||||
returns normally, and the task is NOT cancelled.
|
||||
"""
|
||||
adapter = _make_adapter(monkeypatch)
|
||||
adapter._inbound_running = True
|
||||
# A minimal fake proc so _stop_sidecar() reaches its finally block
|
||||
# (the real process-management behavior is covered separately by
|
||||
# test_sidecar_lifecycle.py) -- this test is purely about the
|
||||
# self-cancellation race.
|
||||
adapter._sidecar_proc = _FakeStoppedProc()
|
||||
|
||||
reconnect_queued: list[bool] = []
|
||||
|
||||
async def _real_notify_fatal_error() -> None:
|
||||
# Stand-in for the Gateway's actual fatal-error handler: it calls
|
||||
# adapter.disconnect() (real chain: disconnect -> _stop_sidecar,
|
||||
# which used to self-cancel), then -- only if that completes
|
||||
# without the CancelledError escaping -- proceeds to queue the
|
||||
# platform for background reconnection.
|
||||
await adapter.disconnect()
|
||||
reconnect_queued.append(True)
|
||||
|
||||
monkeypatch.setattr(adapter, "_notify_fatal_error", _real_notify_fatal_error)
|
||||
|
||||
async def _run_supervisor():
|
||||
await adapter._supervise_sidecar(_DeadProc(exit_code=75))
|
||||
|
||||
task = asyncio.ensure_future(_run_supervisor())
|
||||
adapter._sidecar_supervisor_task = task
|
||||
|
||||
# Must complete cleanly -- must NOT raise CancelledError out to us.
|
||||
await task
|
||||
|
||||
assert task.cancelled() is False, (
|
||||
"The supervisor task must not end up cancelled by its own "
|
||||
"fatal-error handling chain"
|
||||
)
|
||||
assert adapter.has_fatal_error is True
|
||||
assert adapter.fatal_error_code == "SIDECAR_CRASHED"
|
||||
assert reconnect_queued == [True], (
|
||||
"The reconnect-queue step (everything after disconnect() returns "
|
||||
"in the real Gateway handler) must actually run -- this is the "
|
||||
"exact step issue #73159 reports as silently skipped"
|
||||
)
|
||||
# _stop_sidecar() must have cleared the task reference either way.
|
||||
assert adapter._sidecar_supervisor_task is None
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_external_disconnect_still_cancels_supervisor(
|
||||
monkeypatch: pytest.MonkeyPatch,
|
||||
) -> None:
|
||||
"""The OTHER call path -- external cleanup (Gateway shutdown, an
|
||||
explicit /platform disconnect) -- calls _stop_sidecar() from a
|
||||
DIFFERENT task than the supervisor. That legitimate case must still
|
||||
cancel a still-running supervisor task exactly as before."""
|
||||
adapter = _make_adapter(monkeypatch)
|
||||
adapter._sidecar_proc = _FakeStoppedProc()
|
||||
|
||||
supervisor_ran_forever = asyncio.Event()
|
||||
|
||||
async def _hangs_forever():
|
||||
try:
|
||||
supervisor_ran_forever.set()
|
||||
await asyncio.sleep(3600)
|
||||
except asyncio.CancelledError:
|
||||
raise
|
||||
|
||||
task = asyncio.ensure_future(_hangs_forever())
|
||||
adapter._sidecar_supervisor_task = task
|
||||
await supervisor_ran_forever.wait()
|
||||
|
||||
# Called from THIS (different) task -- the external-cleanup case.
|
||||
await adapter._stop_sidecar()
|
||||
|
||||
# cancel() only schedules the CancelledError; await it so the task
|
||||
# actually settles into the cancelled state before asserting.
|
||||
try:
|
||||
await task
|
||||
except asyncio.CancelledError:
|
||||
pass
|
||||
|
||||
assert task.cancelled() is True, (
|
||||
"External cleanup must still cancel a running supervisor task"
|
||||
)
|
||||
assert adapter._sidecar_supervisor_task is None
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue