fix: interruptible bundle-prep/update-check with main-thread signal guard - #265
Draft
Brian Krabach (bkrabach) wants to merge 1 commit into
Draft
fix: interruptible bundle-prep/update-check with main-thread signal guard#265Brian Krabach (bkrabach) wants to merge 1 commit into
Brian Krabach (bkrabach) wants to merge 1 commit into
Conversation
…uard 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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Extracts one story out of #259 (
fix/gap-003-020-023-027-021): making thestartup update check (GAP-023) and bundle preparation (GAP-027)
interruptible via Ctrl+C, plus the main-thread signal-handler guard that is
a direct dependent fix for a bug those two introduced.
Already extracted from #259: GAP-003 → #263, GAP-020 → #264. This is the
next seam.
The story, honestly
GAP-023 — Ctrl+C during "Checking for updates..." killed the whole
amplifier runinvocation (Click's defaultAborted!handler). Fixed with_run_startup_update_check(): runs the check on its own event loop with ascoped SIGINT handler, so Ctrl+C cancels only that task.
GAP-027 —
resolve_config()(bundle discovery/clone/compose/activate)ran before any SIGINT-aware phase. An interrupt there either hung silently
for 60+ seconds or surfaced as a bare
KeyboardInterrupttraceback landingwherever timing put it (observed once inside pydantic's plugin loader, not
git). Fixed with
_resolve_config_interruptibly(): scoped handler prints"Cancelling bundle preparation...", still delivers the real signal via
signal.default_int_handlerso existing unwind/cleanup runs unchanged, andconverts the resulting
KeyboardInterruptinto one message +exit(130).The defect these introduced, and how it was caught. Both call
signal.signal(signal.SIGINT, ...)directly.signal.signal()is onlycallable from the main thread of the main interpreter — on every
platform; this is CPython, not a Windows-specific quirk — so any caller
reaching these phases off the main thread would get a hard
ValueError: signal only works in main thread of the main interpreterinstead of the working, no-handler behavior these phases had before
GAP-023/GAP-027. That's a real embedder-facing crash risk introduced by
commits labelled "Windows" fixes. It was caught four commits later:
_scoped_sigint_handler()now replaces all four rawsignal.signal()call sites, declining to install off the main thread (restoring pre-fix
behavior — no handler, no crash, just no Ctrl+C acknowledgment) and also
catching
ValueErrorfor the subinterpreter edge case.What's known and not known about real exposure. An earlier review pass
found
amplifierddoes not depend on this package at all, andamplifier-app-actionsimports onlyconsole/session_runner, nevercommands.run— so neither known embedder reaches this off the mainthread. Three other embedders (
amplifier-chat,amplifier-voice,amplifier-app-nanoclaw) were never examined — their exposure isunknown. The guard is preventive, restoring a "safe to call from any
thread" contract at no cost on the main-thread path. Not proven necessary
for a known caller; the risk is also not disproven.
Commits carried forward
aa1f3f2— only therun.pyhunks isolated by hand (the commit alsotouched
provider_env_detect.py/init.py/main.pyfor unrelatedstories already extracted in fix: GAP-003 — refuse to silently fall back to Ollama when a credentialed provider's module is missing #263/fix: GAP-020 - bound the first-run confirm prompt to 3 attempts #264, plus a history-race fix later
reverted on the source branch — none of that rides along here).
e2d4133—fix: main-thread signal handler guard for off-thread invocations(clean:run.py+ new test file).431c529—docs: correct docstrings asserting unreachable embedder crash(clean: docstring corrections only).Squashed into a single commit on this branch for review.
Verification
git diff origin/main...HEAD --stat: onlyamplifier_app_cli/commands/run.pyand
tests/test_run_sigint_main_thread_guard.py— nothing fromprovider_env_detect.pyorinit.py's_bounded_confirmrode along.uv run pytest -q: 1304 passed, 1 skipped, 13 deselected, 1 xfailed. Zerofailures.
uv run ruff checkon both changed files: clean._scoped_sigint_handler(reverted it to an unguardedsignal.signal()call) — 2 of 3 tests in
test_run_sigint_main_thread_guard.pyfailedwith the exact
ValueError: signal only works in main thread of the main interpreterthe guard exists to prevent. Restored the guard — all 3pass again.
Coverage gap, stated plainly
The three tests exercise
_scoped_sigint_handler's off-main-threaddecline, 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'ssys.exit(130)unwind (GAP-027) isasserted only by code inspection and the manual-repro notes in the
docstring — there is no automated test exercising that exit path in this
change.
Out of scope, noted for the record
Nothing found beyond what's described above; the three commits were
already fairly clean once the unrelated stories were separated by hand.
🤖 Generated with Amplifier