From a5a3a309ea2c44853be644f4a175fac9d02b8609 Mon Sep 17 00:00:00 2001 From: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com> Date: Wed, 12 Aug 2026 19:29:57 -0700 Subject: [PATCH 1/2] fix: interruptible bundle-prep/update-check with main-thread signal guard MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Extracted from PR #259 (fix/gap-003-020-023-027-021), which bundled this story with four unrelated fixes across 5 files. This isolates just the SIGINT/interruptibility work (GAP-023, GAP-027) together with its direct dependent fix, so a reviewer can hold the whole narrative in their head at once: "make bundle-prep and update-check interruptible, safely, from any thread." **GAP-023: Ctrl+C during the startup update check killed the whole command.** The "Checking for updates..." phase installed no SIGINT handler, so Click's default Aborted! handler killed the entire `amplifier run` invocation over an optional, best-effort check. Added _run_startup_update_check(), which runs the check on its own event loop with a scoped SIGINT handler: Ctrl+C now cancels only that task and prints "Update check skipped (Ctrl+C) -- continuing...", then proceeds to the user's actual command. **GAP-027: resolve_config() (bundle discovery/clone/compose/activate) ran before any SIGINT-aware phase, with no handler of its own.** An interrupt here either hung silently for 60+ seconds or surfaced as a bare KeyboardInterrupt traceback landing wherever timing put it (observed inside pydantic's plugin loader in one run, nowhere near git). Added _resolve_config_interruptibly(), which installs a scoped handler around resolve_config(), prints "Cancelling bundle preparation..." on interrupt, delivers the real signal via signal.default_int_handler so existing unwind/cleanup still runs, and converts the resulting KeyboardInterrupt into one clean message + exit(130) instead of a raw traceback. **The defect this introduced, and its fix.** Both of the above call `signal.signal(signal.SIGINT, ...)` directly. `signal.signal()` is only callable from the main thread of the main interpreter -- on every platform, this is CPython, not a Windows quirk -- so any caller that reaches these phases off the main thread (an embedder driving this code from a worker thread, an HTTP handler, a channel listener) would get a hard `ValueError: signal only works in main thread of the main interpreter` instead of the working, no-handler behavior these phases had before GAP-023 and GAP-027 shipped. That is a real regression these two fixes introduced, labelled "Windows" fixes but with a failure mode specific to no platform at all. It was caught four commits later. `_scoped_sigint_handler()` (a context manager) now replaces all four raw `signal.signal()` call sites. It declines to install a handler when not on the main thread (restoring the pre-fix behavior: no handler, no crash, just no Ctrl+C acknowledgment), and also catches ValueError for the subinterpreter case where `threading.main_thread()` reports one main thread but `signal.signal()` refuses anyway. `tests/test_run_sigint_main_thread_guard.py` exercises the real guard and the real `_run_startup_update_check()` from a worker thread. **What is and isn't known about real exposure.** An earlier review pass found that `amplifierd` does not depend on this package at all, and `amplifier-app-actions` imports only `console`/`session_runner`, never `commands.run` -- so neither known embedder reaches this code off the main thread. Three other embedders (`amplifier-chat`, `amplifier-voice`, `amplifier-app-nanoclaw`) were never examined, and their exposure is unknown either way. The guard is preventive: it restores a "safe to call from any thread" contract these phases already had before GAP-023/GAP-027 narrowed it, at no cost on the main-thread path. It is not proven necessary for a known caller, and the risk it forecloses is not disproven either -- say both, don't overclaim. **Coverage note.** The three tests added/kept here exercise `_scoped_sigint_handler`'s off-main-thread decline, its install/restore behavior on the main thread, and that the real update-check phase survives being run off the main thread. The `resolve_config()` interrupt path's `sys.exit(130)` unwind (GAP-027) is asserted only by code inspection and the docstring's manual-repro notes above -- there is no automated test exercising that exit path in this change. 🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier) Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com> --- amplifier_app_cli/commands/run.py | 227 +++++++++++++++++++-- tests/test_run_sigint_main_thread_guard.py | 128 ++++++++++++ 2 files changed, 341 insertions(+), 14 deletions(-) create mode 100644 tests/test_run_sigint_main_thread_guard.py diff --git a/amplifier_app_cli/commands/run.py b/amplifier_app_cli/commands/run.py index f09af52b..687581f9 100644 --- a/amplifier_app_cli/commands/run.py +++ b/amplifier_app_cli/commands/run.py @@ -3,35 +3,231 @@ from __future__ import annotations import asyncio +import contextlib import logging +import signal import sys +import threading import uuid -from collections.abc import Callable +from collections.abc import Callable, Iterator from typing import Any import click - -from rich.panel import Panel - from amplifier_foundation.exceptions import BundleError, BundleValidationError from amplifier_foundation.modules import ModuleActivationError +from rich.panel import Panel from ..console import console -from ..session_store import extract_session_mode -from ..utils.error_format import escape_markup from ..effective_config import get_effective_config_summary from ..lib.settings import AppSettings from ..paths import create_config_manager from ..runtime.config import resolve_config +from ..session_store import extract_session_mode from ..types import ( ExecuteSingleProtocol, InteractiveChatProtocol, SearchPathProviderProtocol, ) +from ..utils.error_format import escape_markup logger = logging.getLogger(__name__) +@contextlib.contextmanager +def _scoped_sigint_handler(handler: Any) -> Iterator[bool]: + """Install a SIGINT handler for the duration of the block, where possible. + + ``signal.signal()`` is only callable from the main thread of the main + interpreter; anywhere else CPython raises + ``ValueError: signal only works in main thread of the main interpreter``. + + Before GAP-023 and GAP-027, the two interruptible phases in this module + (the startup update check and bundle preparation) installed no handler at + all, so both were safe to call from any thread. Adding an unguarded + ``signal.signal()`` call narrowed that contract: it makes those phases + raise for any caller that does not own the main thread. + + No caller that actually reaches these phases off the main thread has been + identified. Two candidate embedders were checked and neither reaches them: + ``amplifierd`` does not depend on this package at all, and + ``amplifier-app-actions`` imports only ``console`` and ``session_runner``, + not this module. So this guard is preventive, not a fix for an observed + field failure -- it restores the "safe to call from any thread" contract + that GAP-023/GAP-027 silently removed, at no cost. + + Declining to install leaves the caller without the Ctrl+C acknowledgment + it never had before those fixes, and changes nothing else. That is a + deliberate restoration of shipped behavior for a context where signal + handling is structurally unavailable, not an error being swallowed -- so + it is reported at debug level and the block runs either way. + + Yields True if the handler was installed, False if it was declined. + """ + if threading.current_thread() is not threading.main_thread(): + logger.debug( + "SIGINT handler not installed: not on the main thread (%s). " + "Interrupt acknowledgment is unavailable in this context.", + threading.current_thread().name, + ) + yield False + return + + try: + original = signal.signal(signal.SIGINT, handler) + except ValueError as exc: + # Reachable on the main thread of a *subinterpreter*, where + # threading.main_thread() reports that subinterpreter's own main + # thread but signal.signal() still refuses. + logger.debug("SIGINT handler not installed: %s", exc) + yield False + return + + try: + yield True + finally: + signal.signal(signal.SIGINT, original) + + +def _run_startup_update_check() -> None: + """Run the startup update check, but let Ctrl+C skip it immediately and + fall through to the user's actual command instead of the whole + invocation dying (GAP-023). + + Before this fix, the pre-REPL "Checking for updates..." phase (this + function's caller, ``asyncio.run(check_and_notify())``) ran with no + SIGINT handling of its own -- ``_execute_with_interrupt``'s handler + (main.py, installed inside the per-turn wrapper) and the headless goal + path's ``_goal_sigint_handler`` (main.py) both install a handler + *around the operation they guard*; this phase runs earlier than either, + so it had no equivalent. A Ctrl+C here fell through to Click's default + top-level (EOFError, KeyboardInterrupt) handler, which prints + "Aborted!" and kills the ENTIRE ``amplifier run`` invocation -- not + just the optional, best-effort update check. Measured on native + Windows: the interrupt was not merely delayed, it destroyed the user's + whole command over an update check they never asked to wait on. + + Fix: run the check on its own event loop with its own SIGINT handler + (same pattern as the two handlers above), so a Ctrl+C here cancels only + this task -- printing a clear message -- and the caller proceeds to run + the user's actual prompt/command normally. + """ + from ..utils.startup_checker import check_and_notify + + interrupted = False + + def _update_check_sigint_handler(signum, frame): + nonlocal interrupted + interrupted = True + for task in asyncio.all_tasks(loop): + task.cancel() + + loop = asyncio.new_event_loop() + with _scoped_sigint_handler(_update_check_sigint_handler): + try: + asyncio.set_event_loop(loop) + task = loop.create_task(check_and_notify()) + try: + loop.run_until_complete(task) + except asyncio.CancelledError: + pass + finally: + asyncio.set_event_loop(None) + loop.close() + + if interrupted: + console.print("[dim]Update check skipped (Ctrl+C) -- continuing...[/dim]") + + +def _resolve_config_interruptibly( + *, + bundle_name: str | None, + app_settings: AppSettings, + console: Any, +) -> tuple[dict[str, Any], Any]: + """Wrap resolve_config() with a scoped SIGINT handler and a clean + cancellation message (GAP-027). + + resolve_config() -- bundle discovery, git clone/fetch, compose, + activate -- runs EARLIER than every other SIGINT-aware phase in this + file: earlier than _run_startup_update_check() (GAP-023's own scoped + handler, above) and earlier than main.py's per-turn + _execute_with_interrupt() handler. Before this fix it had none of its + own, so a Ctrl+C here fell all the way through to Python's raw + default SIGINT handler with: + + 1. No acknowledgment at all -- every OTHER interruptible phase in + the app prints something ("Update check skipped...", "Stopping + after current operation completes...", etc.) the instant Ctrl+C + is pressed. This phase printed nothing, for however long the + unwind took. + + 2. A non-deterministic landing spot. resolve_config() calls through + many layers of third-party library code (git subprocess calls, + pydantic schema validation, importlib.metadata lookups, etc.) + with no exception handling of its own around the interrupt. + Confirmed on native Windows: the IDENTICAL repro (a bundle + source pointed at a black-holed IP so the git clone blocks + deterministically, Ctrl+C sent ~6s after spawn), run twice back + to back, produced two different failure modes from the same + keystroke: once the process ran on completely unaffected for + 60+ seconds (the interrupt seemingly lost), and once it died in + ~2s but with a BARE, unhandled Python traceback dumped straight + to the user's terminal -- ending in a lone "KeyboardInterrupt" + with zero context, landing inside pydantic's + complete_model_class -> create_schema_validator -> + importlib.metadata.entry_points() chain, nothing to do with git + at all. Neither outcome is acceptable, and which one a user gets + is pure timing luck. + + This is a genuinely different location and cause from GAP-014 + (git.py's unbounded wait), GAP-023 (the update-check phase, which + runs AFTER this one), GAP-025 (git.py's BaseException cleanup), and + GAP-026 (subprocess_runner.py's delegation cancellation) -- none of + those touch this call site, and none of them install any + acknowledgment or containment for an interrupt landing here. + + Fix: same established pattern as _run_startup_update_check() -- a + scoped SIGINT handler installed only for the duration of this call, + printing the same "Cancelling..." convention used everywhere else in + the app the moment Ctrl+C is pressed (fixing the missing-feedback + symptom), while still delivering the real interrupt via + signal.default_int_handler so the existing unwind-and-cleanup + machinery (GAP-014/025's process-tree kill, etc.) runs exactly as it + did before this fix. The KeyboardInterrupt is then caught at THIS + single, deliberate point -- regardless of which arbitrary library + frame it actually surfaces in -- and converted into one clean, + actionable message and a normal process exit, instead of a raw + traceback landing wherever the timing happened to put it. + """ + interrupted = False + + def _bundle_prep_sigint_handler(signum: int, frame: Any) -> None: + nonlocal interrupted + if not interrupted: + interrupted = True + console.print( + "\n[yellow]Cancelling bundle preparation... " + "(this may take a moment to unwind cleanly)[/yellow]" + ) + # Still deliver the real interrupt so every existing unwind path + # (git.py's process-tree kill, etc.) behaves exactly as before -- + # this handler only ADDS an acknowledgment and a clean landing + # spot, it does not change what happens once the exception is + # actually raised. + signal.default_int_handler(signum, frame) + + with _scoped_sigint_handler(_bundle_prep_sigint_handler): + try: + return resolve_config( + bundle_name=bundle_name, + app_settings=app_settings, + console=console, + ) + except KeyboardInterrupt: + console.print("[red]Bundle preparation cancelled.[/red]") + sys.exit(130) + + def register_run_command( cli: click.Group, *, @@ -159,9 +355,12 @@ def run( # Track configuration source for display (always bundle mode now) config_source_name = f"bundle:{bundle}" - # Resolve configuration using unified function (single source of truth) + # Resolve configuration using unified function (single source of truth). + # GAP-027: wrapped for scoped SIGINT handling + a clean cancellation + # message instead of a raw traceback landing wherever an interrupt + # happens to surface (see _resolve_config_interruptibly docstring). try: - config_data, prepared_bundle = resolve_config( + config_data, prepared_bundle = _resolve_config_interruptibly( bundle_name=bundle, app_settings=app_settings, console=console, @@ -238,8 +437,7 @@ def run( target_idx = None for i, entry in enumerate(providers_list): if isinstance(entry, dict) and ( - entry.get("id") == provider - or entry.get("instance_id") == provider + entry.get("id") == provider or entry.get("instance_id") == provider ): target_idx = i break @@ -248,7 +446,10 @@ def run( # Pass 2: fallback — module-type match (original behavior). # Preserves single-instance usage: -p anthropic → provider-anthropic. for i, entry in enumerate(providers_list): - if isinstance(entry, dict) and entry.get("module") == provider_module: + if ( + isinstance(entry, dict) + and entry.get("module") == provider_module + ): target_idx = i break @@ -345,9 +546,7 @@ def run( prepared_bundle.mount_plan["providers"] = updated_providers # Run update check (uses unified startup_checker with settings.yaml) - from ..utils.startup_checker import check_and_notify - - asyncio.run(check_and_notify()) + _run_startup_update_check() if mode == "chat": # Interactive mode - supports optional initial_prompt for auto-execution diff --git a/tests/test_run_sigint_main_thread_guard.py b/tests/test_run_sigint_main_thread_guard.py new file mode 100644 index 00000000..3fbc6252 --- /dev/null +++ b/tests/test_run_sigint_main_thread_guard.py @@ -0,0 +1,128 @@ +"""Tests for the main-thread guard around SIGINT handler installation. + +`signal.signal()` is only callable from the main thread of the main +interpreter. Anywhere else CPython raises: + + ValueError: signal only works in main thread of the main interpreter + +Two interruptible phases in `amplifier_app_cli.commands.run` install SIGINT +handlers: the startup update check (GAP-023) and bundle preparation +(GAP-027). Neither installed a handler before those fixes, so both were safe +to call from any thread. Adding an unguarded `signal.signal()` call narrowed +that contract -- it makes those phases raise for any caller that does not own +the main thread. + +No caller that actually reaches these phases off the main thread has been +identified; two candidate embedders were checked and neither reaches them. +These tests therefore guard a contract rather than reproduce an observed +field failure: they pin "safe to call from any thread" so it cannot be +removed again silently. + +They exercise the real `_scoped_sigint_handler` and the real +`_run_startup_update_check`, not mocks of them, from a worker thread. +""" + +from __future__ import annotations + +import signal +import threading +from typing import Any +from unittest.mock import patch + +from amplifier_app_cli.commands.run import ( + _run_startup_update_check, + _scoped_sigint_handler, +) + + +def _noop_handler(signum: int, frame: Any) -> None: # pragma: no cover - never invoked + """Stand-in SIGINT handler. Installed and removed, never fired.""" + + +def _run_in_worker_thread(fn: Any) -> dict[str, Any]: + """Run `fn` on a non-main thread and capture its outcome. + + Returns a dict with either `result` or `exc`, so the caller can assert on + a raised exception rather than having it vanish into the thread. + """ + captured: dict[str, Any] = {} + + def target() -> None: + assert threading.current_thread() is not threading.main_thread() + try: + captured["result"] = fn() + except BaseException as exc: # noqa: BLE001 - deliberately capturing everything + captured["exc"] = exc + + thread = threading.Thread(target=target, name="sigint-guard-test-worker") + thread.start() + thread.join(timeout=30) + assert not thread.is_alive(), "worker thread did not finish" + return captured + + +def test_scoped_sigint_handler_declines_off_main_thread() -> None: + """Off the main thread the guard declines instead of raising ValueError. + + This is the regression. Without the guard, `signal.signal()` raises + `ValueError: signal only works in main thread of the main interpreter` + and the caller dies. + """ + + def use_guard() -> bool: + with _scoped_sigint_handler(_noop_handler) as installed: + return installed + + captured = _run_in_worker_thread(use_guard) + + assert "exc" not in captured, ( + f"guard raised off the main thread: {captured.get('exc')!r} " + "-- the main-thread guard is missing or ineffective" + ) + assert captured["result"] is False, ( + "guard reported a handler was installed off the main thread; " + "signal.signal() cannot succeed there" + ) + + +def test_scoped_sigint_handler_installs_and_restores_on_main_thread() -> None: + """On the main thread the handler is installed and then restored. + + Guards against a fix that works by simply never installing anything. + """ + assert threading.current_thread() is threading.main_thread() + + before = signal.getsignal(signal.SIGINT) + + with _scoped_sigint_handler(_noop_handler) as installed: + assert installed is True + assert signal.getsignal(signal.SIGINT) is _noop_handler + + assert signal.getsignal(signal.SIGINT) is before + + +def test_startup_update_check_does_not_raise_off_main_thread() -> None: + """The real update-check phase survives being run off the main thread. + + Integration-level counterpart to the unit test above: exercises the + actual function an embedder reaches, with only its network-touching + dependency stubbed out. + """ + + async def _fake_check_and_notify() -> None: + return None + + def run_check() -> str: + with patch( + "amplifier_app_cli.utils.startup_checker.check_and_notify", + _fake_check_and_notify, + ): + _run_startup_update_check() + return "completed" + + captured = _run_in_worker_thread(run_check) + + assert "exc" not in captured, ( + f"_run_startup_update_check raised off the main thread: {captured.get('exc')!r}" + ) + assert captured["result"] == "completed" From 78b98f805d430c052729da199d415d41711e0d23 Mon Sep 17 00:00:00 2001 From: sadlilas <11658960+sadlilas@users.noreply.github.com> Date: Mon, 17 Aug 2026 14:24:55 -0700 Subject: [PATCH 2/2] fix: run git status helpers off the event loop during update check The startup update check installs a scoped SIGINT handler that cancels the running task on Ctrl+C. But _check_file_source() called six synchronous subprocess.run() git helpers directly on the event loop thread, so the cancellation could not be delivered until every one of them returned -- including _count_commits_behind(), which shells out to `git fetch` (a network call, 5s timeout) once per local source. The handler fired, but the interrupt was not honoured until the blocking git work finished, which is the exact behaviour the interruptible update check was meant to fix. Wrap the six helpers in asyncio.to_thread() so the loop stays free and CancelledError lands at the next await. No change to what the helpers do or return. --- amplifier_app_cli/utils/source_status.py | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/amplifier_app_cli/utils/source_status.py b/amplifier_app_cli/utils/source_status.py index 82d747c8..2d2be017 100644 --- a/amplifier_app_cli/utils/source_status.py +++ b/amplifier_app_cli/utils/source_status.py @@ -4,6 +4,7 @@ Uses existing StandardModuleSourceResolver infrastructure. """ +import asyncio import json import logging import re @@ -205,11 +206,14 @@ async def _check_file_source( local_path = source.path - # Get local git info - local_sha = _get_local_sha(local_path) - remote_url = _get_remote_url(local_path) - uncommitted = _has_uncommitted_changes(local_path) - unpushed = _has_unpushed_commits(local_path) + # Get local git info. These helpers shell out to git synchronously; running + # them off-thread keeps the event loop free so a Ctrl+C during the startup + # update check is honoured at the next await instead of waiting for every + # git invocation to finish first. + local_sha = await asyncio.to_thread(_get_local_sha, local_path) + remote_url = await asyncio.to_thread(_get_remote_url, local_path) + uncommitted = await asyncio.to_thread(_has_uncommitted_changes, local_path) + unpushed = await asyncio.to_thread(_has_unpushed_commits, local_path) status = LocalFileStatus( name=name, @@ -226,7 +230,7 @@ async def _check_file_source( if remote_url and local_sha: try: # Get current branch - current_branch = _get_current_branch(local_path) + current_branch = await asyncio.to_thread(_get_current_branch, local_path) if current_branch: remote_sha = await _get_github_commit_sha( client, remote_url, current_branch @@ -234,7 +238,9 @@ async def _check_file_source( if remote_sha != local_sha: status.remote_sha = remote_sha[:7] - status.commits_behind = _count_commits_behind(local_path) + status.commits_behind = await asyncio.to_thread( + _count_commits_behind, local_path + ) except Exception as e: logger.debug(f"Could not check remote for {name}: {e}")