From 29d0cc2602e01943ab300c0382fc9d97efb376da Mon Sep 17 00:00:00 2001 From: Austin Pickett Date: Fri, 14 Aug 2026 12:02:05 -0400 Subject: [PATCH] fix(dashboard): treat Ctrl+C serve shutdown as a clean exit (supersedes #52970) (#85711) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(dashboard): suppress Ctrl+C shutdown traceback * fix(dashboard): extend clean Ctrl+C exit to the Windows serve branch The Windows loop-factory branch (and its pre-0.36 asyncio.run fallback) runs under the same uvicorn capture_signals() re-raise as the POSIX path, so console Ctrl+C leaked the identical KeyboardInterrupt traceback there. Guard both serve calls with the same clean-exit contract, keeping the import-resolution try/except comment accurate (genuine serve-time errors still propagate). Also ports the reworded POSIX-test docstring (the serve path is no longer 'byte-for-byte unchanged'), wraps the POSIX KI test in pytest.fail so a regression reports red instead of aborting the pytest session, and adds the windows_only sibling test. Extends #52970 to the whole bug class. * chore: map contributor email for @wangs1203 * test(dashboard): actually exercise the pre-0.36 Windows fallback KI contract Copilot review caught that patching uvicorn._compat.asyncio_run with raising=False makes the import succeed, so _runner is non-None and the extra asyncio.run patch never covered the fallback. Split it out: a dedicated windows_only test halts the _compat import (None in sys.modules) so the fallback branch is genuinely selected, then asserts the same clean-KI contract on bare asyncio.run. --------- Co-authored-by: Emiya·Leon --- contributors/emails/wangs.coder@gmail.com | 1 + hermes_cli/web_server.py | 41 ++++++--- tests/test_web_server.py | 105 ++++++++++++++++++++-- 3 files changed, 129 insertions(+), 18 deletions(-) create mode 100644 contributors/emails/wangs.coder@gmail.com diff --git a/contributors/emails/wangs.coder@gmail.com b/contributors/emails/wangs.coder@gmail.com new file mode 100644 index 0000000000..4e1784e5ad --- /dev/null +++ b/contributors/emails/wangs.coder@gmail.com @@ -0,0 +1 @@ +wangs1203 diff --git a/hermes_cli/web_server.py b/hermes_cli/web_server.py index d34e0a16e0..857fcd3402 100644 --- a/hermes_cli/web_server.py +++ b/hermes_cli/web_server.py @@ -18266,11 +18266,14 @@ def start_server( if server.started: await server.shutdown() - # On POSIX, keep the long-standing ``asyncio.run(_serve())`` behavior - # unchanged — Python's default loop there is already a SelectorEventLoop - # (or uvloop when uvicorn[standard] installs it), which is exactly what - # uvicorn serves on. Touching that path would only widen the blast radius - # for no benefit. + # On POSIX, keep the long-standing ``asyncio.run(_serve())`` runner — + # Python's default loop there is already a SelectorEventLoop (or uvloop when + # uvicorn[standard] installs it), which is exactly what uvicorn serves on. + # Uvicorn's ``capture_signals()`` restores the original SIGINT handler and + # re-raises the captured signal after a graceful shutdown, which otherwise + # leaks a noisy KeyboardInterrupt traceback for the normal foreground + # dashboard Ctrl+C path. Treat that one signal as a clean user-requested + # shutdown; other serve-time errors still propagate. # # On Windows it is broken: ``asyncio.run`` defaults to a ProactorEventLoop, # but uvicorn's socket-serving stack assumes a SelectorEventLoop on win32 @@ -18282,14 +18285,17 @@ def start_server( # no TCP handshake completing (#50641). So *only on Windows* we mirror # uvicorn's own machinery and run on the loop factory it picks. if sys.platform != "win32": - asyncio.run(_serve()) + try: + asyncio.run(_serve()) + except KeyboardInterrupt: + return return # Windows-only path. Resolve the runner + loop factory FIRST (and fall back # to a hand-installed Windows selector policy only when uvicorn predates the - # loop-factory API, < 0.36). The actual serve call is then OUTSIDE the - # try/except so genuine serve-time errors (port in use, KeyboardInterrupt) - # propagate normally instead of being swallowed and double-run. + # loop-factory API, < 0.36). The actual serve call is then OUTSIDE this + # import try/except so genuine serve-time errors (port in use) propagate + # normally instead of being swallowed and double-run. try: from uvicorn._compat import asyncio_run as _runner @@ -18304,7 +18310,16 @@ def start_server( except Exception: pass - if _runner is not None: - _runner(_serve(), loop_factory=_loop_factory) - else: - asyncio.run(_serve()) + # Same clean Ctrl+C contract as the POSIX branch above: ``capture_signals()`` + # re-raises the captured signal after the graceful shutdown has already + # completed. For console Ctrl+C the re-raised SIGINT lands as + # ``KeyboardInterrupt`` — a clean user-requested exit here too. (Re-raised + # SIGTERM/SIGBREAK keep their default terminate disposition and never reach + # this except.) + try: + if _runner is not None: + _runner(_serve(), loop_factory=_loop_factory) + else: + asyncio.run(_serve()) + except KeyboardInterrupt: + return diff --git a/tests/test_web_server.py b/tests/test_web_server.py index 55534e8b84..ed81bce22a 100644 --- a/tests/test_web_server.py +++ b/tests/test_web_server.py @@ -6,6 +6,7 @@ Config + Server + asyncio.run to capture kwargs without starting an event loop. import asyncio import contextlib +import sys import pytest import uvicorn @@ -207,12 +208,12 @@ def test_start_server_runs_on_uvicorns_loop_factory(monkeypatch): def test_start_server_keeps_bare_asyncio_run_on_posix(monkeypatch): - """POSIX behavior must be byte-for-byte unchanged: serve via the plain - ``asyncio.run(_serve())`` path, never the Windows loop-factory branch. + """POSIX continues to serve via the plain ``asyncio.run(_serve())`` path, + never the Windows loop-factory branch. - The #50641 fix is intentionally win32-scoped to keep the blast radius - minimal — Python's default loop on POSIX is already a SelectorEventLoop - (or uvloop), which is what uvicorn serves on, so there is nothing to fix. + The #50641 fix is intentionally win32-scoped to keep the loop selection + unchanged — Python's default loop on POSIX is already a SelectorEventLoop + (or uvloop), which is what uvicorn serves on. No platform patching: the Linux CI host is already POSIX, so this asserts the real host's serve path. @@ -243,3 +244,97 @@ def test_start_server_keeps_bare_asyncio_run_on_posix(monkeypatch): assert runner_called["hit"] is False, ( "POSIX must not take the Windows loop-factory branch" ) + + +def test_start_server_treats_posix_keyboardinterrupt_as_clean_shutdown(monkeypatch): + """Ctrl+C is the normal foreground-dashboard shutdown path. + + Uvicorn re-raises captured SIGINT as ``KeyboardInterrupt`` after it has + restored the original signal handlers. The dashboard should treat that as a + clean user-requested shutdown instead of leaking a traceback to the terminal. + """ + _stub_uvicorn(monkeypatch) + + def _raise_keyboard_interrupt(coro): + coro.close() + raise KeyboardInterrupt + + monkeypatch.setattr(asyncio, "run", _raise_keyboard_interrupt) + + # Catch rather than let it escape: pytest treats a propagating + # KeyboardInterrupt as a session abort, not a test failure, so a + # regression here would kill the run instead of reporting red. + try: + web_server.start_server(host="127.0.0.1", port=0, open_browser=False) + except KeyboardInterrupt: + pytest.fail( + "start_server must treat serve-time KeyboardInterrupt as a clean " + "shutdown, not propagate it" + ) + + +@pytest.mark.windows_only +def test_start_server_treats_windows_keyboardinterrupt_as_clean_shutdown(monkeypatch): + """Console Ctrl+C on the Windows loop-factory branch is a clean exit too. + + Same bug class as the POSIX branch: ``capture_signals()`` re-raises the + captured SIGINT after graceful shutdown, which surfaces as + ``KeyboardInterrupt`` out of the loop-factory runner. The serve call must + swallow exactly that and return. + + Windows-only per the no-platform-faking rule (tests/conftest.py): the + branch is selected by the real host, and the runner import + (``uvicorn._compat.asyncio_run``) resolves inside ``start_server``, after + the monkeypatch below is installed. + """ + _stub_uvicorn(monkeypatch) + + def _raise_keyboard_interrupt(coro, *, loop_factory=None): + coro.close() + raise KeyboardInterrupt + + monkeypatch.setattr( + "uvicorn._compat.asyncio_run", _raise_keyboard_interrupt, raising=False + ) + + try: + web_server.start_server(host="127.0.0.1", port=0, open_browser=False) + except KeyboardInterrupt: + pytest.fail( + "start_server must treat serve-time KeyboardInterrupt as a clean " + "shutdown on the Windows branch, not propagate it" + ) + + +@pytest.mark.windows_only +def test_start_server_treats_windows_fallback_keyboardinterrupt_as_clean_shutdown( + monkeypatch, +): + """The pre-0.36 fallback runner shares the clean Ctrl+C contract. + + When ``uvicorn._compat.asyncio_run`` is unavailable (uvicorn predates the + loop-factory API), the Windows branch falls back to bare ``asyncio.run`` + under a hand-installed selector policy — still inside the same + ``capture_signals()`` re-raise, so its ``KeyboardInterrupt`` must be + swallowed identically. Forcing the ``_compat`` import to fail (None in + ``sys.modules`` halts the import) is what actually selects the fallback: + merely patching ``asyncio.run`` alongside a successful import would leave + this path untested. + """ + _stub_uvicorn(monkeypatch) + + monkeypatch.setitem(sys.modules, "uvicorn._compat", None) + + def _raise_keyboard_interrupt(coro): + coro.close() + raise KeyboardInterrupt + + monkeypatch.setattr(asyncio, "run", _raise_keyboard_interrupt) + + try: + web_server.start_server(host="127.0.0.1", port=0, open_browser=False) + except KeyboardInterrupt: + pytest.fail( + "start_server must treat serve-time KeyboardInterrupt as a clean " + "shutdown on the Windows pre-0.36 fallback, not propagate it" + )