feat(agents): Ctrl+Shift+K kill-switch shortcut + SIGTERM→SIGKILL grace window#1962
Conversation
… grace window - Add Ctrl+Shift+K system shortcut in desktop SPA that triggers POST /api/agents/bulk/stop to immediately stop all running agents - Enhance bulk_stop_agents and stop_agent backend routes with SIGTERM (incus stop) → 2s grace → SIGKILL (incus stop --force) to guarantee termination of runaway agent containers - Desktop component AgentKillSwitch + TopBar integration already existed; this adds the keyboard shortcut and hardens the backend Fixes jaylfc#132
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
Warning Review limit reached
Next review available in: 16 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
|
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| force_killed = False | ||
| if await container_exists(container_name): | ||
| force_result = await stop_container(container_name, force=True) | ||
| force_killed = force_result.get("success", False) |
There was a problem hiding this comment.
[WARNING]: Force-kill result error/output is discarded
Unlike bulk_stop_agents (which records error/output in force_results), this single-agent path captures only the boolean success. If incus stop --force fails (daemon down, permission error, timeout), the client receives force_killed: false with no reason, making a failed SIGKILL indistinguishable from "container already gone". Consider returning force_result details for diagnosability.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| stop_result = await stop_container(container_name) | ||
|
|
||
| # 2-second grace window, then SIGKILL | ||
| await asyncio.sleep(2) |
There was a problem hiding this comment.
[SUGGESTION]: Unguarded asyncio.sleep(2) blocks the handler with no timeout/cancellation
The grace window uses a bare await asyncio.sleep(2) followed by a stop_container(force=True) call whose underlying incus stop --force can itself hang. If the incus CLI blocks (daemon unresponsive, etc.), the request coroutine is held indefinitely with no cancellation or timeout. Consider wrapping the wait and the force-kill in asyncio.wait_for (or a try/except for asyncio.CancelledError) so a hung incus call cannot pin a worker. (Same pattern applies to the bulk path at line 818.)
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: 1 Issue Found (7 resolved since last review) | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Resolved since previous review
Files Reviewed
Fix these issues in Kilo Cloud Previous Review Summary (commit 48bfdbf)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 48bfdbf)Status: 6 Issues Found (2 resolved since last review) | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
RESOLVED (fixed in incremental diff since 48bfdbf)
SUGGESTION (carried forward — not duplicated)
Incremental review (since 48bfdbf)The incremental diff contains only Files Reviewed (1 file in incremental diff)
Reviewed by hy3:free · Input: 80.1K · Output: 7.3K · Cached: 199.2K |
|
Reviewed at head 82859a2 (verified locally: tsc clean, AgentKillSwitch + shortcut-registry vitest 20/20, pytest stop-route pass, backward-compatible with the new force_kill_results/force_killed keys). The kill path is safe: it goes through stop_container(name, force) -> incus stop [--force] on the named container taos-agent-, no pid/pgid handling, so there is no risk of the escalation touching the controller or an unrelated process group, and _run wraps the subprocess in wait_for(120s) so a hung incus is bounded, not infinite. Good work. Three small things to fold before I merge (house rule: no deferred nits):
The pre-existing F401 (get_agent_summaries unused, agents.py:15) is on master already, leave it. Fold 1-3 and I will merge. |
- stop_agent: wrap force-kill in asyncio.shield() to protect from cancellation during the 2s grace sleep. On CancelledError, attempt the force-kill anyway before re-raising. - stop_agent: log warning + return force_error when force-kill fails (WARNING fix: previously discarded the SIGKILL failure result). - bulk_stop_agents: same cancellation protection and failed-kill logging. Fixes two Kilo bot findings on PR jaylfc#1962: WARNING line 897 — discarded SIGKILL result SUGGESTION line 893 — unguarded asyncio.sleep + no timeout Kilo: 1W+1S
|
Pushed fix for Kilo findings (1W+1S): stop_agent (WARNING fix): Force-kill result no longer discarded — logs warning + returns stop_agent + bulk_stop_agents (SUGGESTION fix): Force-kill wrapped in Commit: 9e04b4b |
| stop_result = await stop_container(container_name) | ||
|
|
||
| # 2-second grace window, then SIGKILL (protected from cancellation) | ||
| await asyncio.sleep(2) |
There was a problem hiding this comment.
[SUGGESTION]: Comment claims the grace window is "protected from cancellation", but only stop_container is shielded — the preceding await asyncio.sleep(2) is not. A request cancelled during the 2-second sleep skips the SIGKILL phase entirely, contradicting the guarantee implied by the comment (same applies to line 817).
To truly protect the grace window, shield the whole sleep+force-kill sequence, e.g. wrap the sleep in a task that is awaited under asyncio.shield as well, or restructure so the force-kill always runs before re-raising.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| "prepare_report": report, | ||
| "stop_result": stop_result, | ||
| "force_killed": force_killed, | ||
| "force_error": force_error, |
There was a problem hiding this comment.
[SUGGESTION]: When asyncio.shield is cancelled, the shielded stop_container task keeps running in the background (orphaned) while a second stop_container is spawned on line 918, issuing a duplicate force-kill. The orphaned task can outlive the request and its result/error is never inspected.
If the re-attempt on cancellation is intentional for guarantee, consider awaiting the original shielded task instead of starting a fresh one, or otherwise track and cancel it to avoid a stray background container op.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Wrap the entire 2-second grace window + SIGKILL sequence in asyncio.shield() for both stop_agent and bulk_stop_agents. Previously only stop_container calls were shielded; the preceding await asyncio.sleep(2) was not, so a request cancelled during the grace window would skip SIGKILL entirely.
|
Thanks, re-checked at head c8fe2cd. Item 2 (record force output/error in stop_agent) is addressed - _grace_kill now returns the output and the response carries force_killed + force_error, good. But 2 of the 3 are still open:
Also, Kilo flagged the new shield: if the asyncio.shield is cancelled, the shielded task orphans in the background (agents.py:923) - worth handling while you are in here. Fold 1 + 3 (and ideally add a test for the run-state gate, since there is no coverage for the changed force-kill path) and I will merge. |
|
Held. No new commit since my last review, so 2 of the 3 asks are still open:
Ask 2 (record force output/error in stop_agent) is folded, good. Please also add a test for the run-state gate. |
… comment, fix shield orphan - Replace container_exists() with list_containers() status=='Running' check in both bulk_stop_agents and stop_agent force-kill paths. container_exists() only resolves project membership — it returns True for Stopped containers, causing misleading force_killed:false reports after incus stop --force failures on already-dead containers. - Add one-line comment noting that the sleep+force sequence inside asyncio.shield is bounded by the 120s _run cap. - Fix asyncio.shield() orphan: create a Task, catch CancelledError, cancel the detached grace-kill task, and await its cleanup before re-raising. - Add TestKillSwitchRunStateGate with 3 tests covering: * bulk_stop skips force-kill for Stopped containers * stop_agent skips force-kill for Stopped containers * bulk_stop still force-kills Running containers
v3 re-review fixes — commit
|
| name = agent["name"] | ||
| container_name = f"taos-agent-{name}" | ||
| try: | ||
| if container_name in running: |
There was a problem hiding this comment.
[WARNING]: Run-state gate uses exact status == "Running", so a container stuck mid-shutdown escapes SIGKILL
This revision replaced container_exists(container_name) with a gate that only force-kills containers whose status is exactly "Running" (see running = {c.name for c in containers if c.status == "Running"} on line 822). Incus reports transitional statuses such as Stopping, Freezing, and Frozen. A container that received SIGTERM but hangs during shutdown will typically report Stopping rather than Running, so it is silently excluded from running and never receives incus stop --force.
That is precisely the hung-shutdown case the 2s grace + SIGKILL was added to guarantee against, so the guarantee is weakened relative to the previous container_exists behavior. Consider inverting the gate (force-kill anything not confirmed Stopped) or explicitly including transitional states, e.g. treat any status other than Stopped as still-alive.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| await asyncio.sleep(2) | ||
| containers = await list_containers(prefix="taos-agent-") | ||
| running = {c.name for c in containers if c.status == "Running"} | ||
| if container_name in running: |
There was a problem hiding this comment.
[WARNING]: Same run-state gate weakens the single-agent SIGKILL guarantee
if container_name in running: (with running built from c.status == "Running" on line 919) skips incus stop --force for any container not reporting exactly Running. A container hung in a transitional shutdown state (Stopping/Freezing/Frozen) after the SIGTERM at line 912 will not match, so force_killed returns False and the container is left alive — the opposite of the documented guarantee ("if the container is still running, sends SIGKILL … to guarantee termination"). The previous container_exists check did not have this gap. Consider gating on "not Stopped" instead of "exactly Running".
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
… on incus-down, rename force_error->force_output
- desktop/App.tsx: check CSRF token before kill-switch POST, surface
missing-token and server errors via notification store instead of
silent catch{}; remove no-op ?. on withCsrf return.
- agents.py bulk_stop: when list_containers returns empty but agents
are configured (incus unhealthy), attempt force-kill on all agents
anyway instead of silently skipping SIGKILL.
- agents.py stop_agent: same incus-down fallback; rename force_error
to force_output (it carries stdout even on success).
…tates Include Stopping, Freezing, and Frozen in the run-state check for force-kill eligibility in both bulk_stop_agents and stop_agent. Previously the gate matched only 'Running', so containers stuck in transitional Incus states (Stopping/Freezing/Frozen) after SIGTERM would not receive SIGKILL and could hang indefinitely. Kilo PR review finding on jaylfc#1962.
…tates Include Stopping, Freezing, and Frozen in the run-state check for force-kill eligibility in both bulk_stop_agents and stop_agent. Previously the gate matched only 'Running', so containers stuck in transitional Incus states (Stopping/Freezing/Frozen) after SIGTERM would not receive SIGKILL and could hang indefinitely. Kilo PR review finding on jaylfc#1962.
… on incus-down, rename force_error->force_output
- desktop/App.tsx: check CSRF token before kill-switch POST, surface
missing-token and server errors via notification store instead of
silent catch{}; use non-null assertion on withCsrf return (tsc).
- agents.py bulk_stop: when list_containers returns empty but agents
are configured (incus unhealthy), attempt force-kill on all agents
anyway instead of silently skipping SIGKILL.
- agents.py stop_agent: same incus-down fallback; rename force_error
to force_output (it carries stdout even on success).
| try: | ||
| force_results = await asyncio.shield(task) | ||
| except asyncio.CancelledError: | ||
| task.cancel() |
There was a problem hiding this comment.
[WARNING]: task.cancel() in the CancelledError handler defeats asyncio.shield, aborting the SIGKILL
The whole point of asyncio.shield(task) (line 856) is that the inner _grace_kill coroutine keeps running even if the outer await is cancelled (e.g. client disconnect). But on except asyncio.CancelledError you then call task.cancel() (line 858) and await task, which actively cancels and kills the shielded task before re-raising. The result: when a request is cancelled during the grace window, the incus stop --force SIGKILL is aborted and the container is left alive — the opposite of the "shielded from cancellation" comment (line 818) and the PR's stated guarantee of guaranteed termination.
If the intent is to let the force-kill finish despite client cancellation, drop the task.cancel()/await task here and just raise (let the detached task complete in the background). If the intent is to abort on cancellation, the shield is pointless and the comment is misleading. The same defect exists in stop_agent at line 955.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| try: | ||
| force_killed, force_output = await asyncio.shield(task) | ||
| except asyncio.CancelledError: | ||
| task.cancel() |
There was a problem hiding this comment.
[WARNING]: task.cancel() in the CancelledError handler defeats asyncio.shield, aborting the SIGKILL
Same defect as bulk_stop_agents: the asyncio.shield(task) on line 953 is meant to keep the force-kill running after a client-side cancellation, but the except asyncio.CancelledError block (lines 954-960) explicitly task.cancel()s the shielded task, aborting the SIGKILL. So the incus stop --force does NOT survive request cancellation, contradicting the "shielded from cancellation" comment (line 925) and the "guarantee termination" docstring. Remove the task.cancel()/await task to let the shield actually protect the kill, or correct the comment/intent.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
…l-switch CancelledError handlers Removed explicit task.cancel() calls from the CancelledError exception handlers in both bulk_stop_agents (line ~858) and stop_agent (line ~955). Calling task.cancel() on a shielded task in its CancelledError handler aborts the shielded operation before it can send SIGKILL — defeating the 'shielded from cancellation' guarantee. Now the handlers simply await the shielded task (which continues running because asyncio.shield protects it), allowing the 2-second grace period and SIGKILL to complete before re-raising the cancellation. Closes Kilo WARNINGs on PR jaylfc#1962.
Mock stop_container, list_containers, start_container, and restart_container in TestBulkOperations to prevent FileNotFoundError when incus binary is not available (e.g. GitHub Actions CI). The kill-switch changes in PR jaylfc#1962 added incus list/incus stop calls to the bulk stop flow, which exposed the missing mocks. Fixes test failures on feat/kill-switch.
Mock stop_container, list_containers, start_container, and restart_container in TestBulkOperations to prevent FileNotFoundError when incus binary is not available (e.g. GitHub Actions CI). The kill-switch changes added incus list/incus stop calls to the bulk stop flow, which exposed the missing mocks. Fixes CI test failures on PR jaylfc#1988.
- Use asyncio.gather for concurrent force-kills in bulk_stop_agents - Start _grace_kill before await for graceful stop (concurrent SIGKILL path) - Remove Frozen/Freezing from run-state gate (only Running/Stopping trigger force) - Fix inaccurate "120s _run cap" comment (no route timeout exists) - Fix test assertion: "test-agent" → "taos-agent-test-agent" in force_calls - Update test_bulk_stop to exercise Running container path instead of empty-list - Add test_stop_agent_force_kills_running_container for single-agent path - App.tsx: parse bulk stop response to detect individual agent failures Targeted tests: 5/5 pass. TypeScript tsc --noEmit: clean.
|
One real correctness bug defeats the whole feature: in stop_agent (routes/agents.py L381-382) and bulk_stop_agents (L308-309), inside except asyncio.CancelledError you call task.cancel() on the very task wrapped by asyncio.shield(task). That defeats the shield - a client-cancelled request aborts the in-flight incus stop --force, so the SIGKILL the kill-switch promises never lands. Drop the task.cancel() (or await the shielded task to completion) so the force-kill survives request cancellation. Also please confirm two items that were on moved lines: the single-agent path discarding force-kill error/output vs bulk's force_results, and the status=='Running' gate letting a mid-shutdown container escape SIGKILL. Fix the shield defeat and I merge. |
Shield-defeat fix — commit
|
…ace-kill timeout
- Run-state gate uses c.status not in ('Stopped',) instead of exact ('Running', 'Stopping')
to catch containers in transitional states (Freezing, Frozen, etc.) per Kilo findings
- Wrap asyncio.shield(task) in asyncio.wait_for(..., timeout=30) with TimeoutError
handling to prevent hung incus calls from pinning workers
- Shield-cancel bug already fixed in prior commits (no task.cancel() remaining)
Mock stop_container, list_containers, start_container, and restart_container in TestBulkOperations to prevent FileNotFoundError when incus binary is not available (e.g. GitHub Actions CI). The kill-switch changes in PR jaylfc#1962 added incus list/incus stop calls to the bulk stop flow, which exposed the missing mocks. Fixes test failures on feat/kill-switch.
Mock stop_container, list_containers, start_container, and restart_container in TestBulkOperations to prevent FileNotFoundError when incus binary is not available (e.g. GitHub Actions CI). The kill-switch changes in PR jaylfc#1962 added incus list/incus stop calls to the bulk stop flow, which exposed the missing mocks. Fixes test failures on feat/kill-switch.
* feat(agents): add Ctrl+Shift+K kill-switch shortcut + SIGTERM→SIGKILL grace window - Add Ctrl+Shift+K system shortcut in desktop SPA that triggers POST /api/agents/bulk/stop to immediately stop all running agents - Enhance bulk_stop_agents and stop_agent backend routes with SIGTERM (incus stop) → 2s grace → SIGKILL (incus stop --force) to guarantee termination of runaway agent containers - Desktop component AgentKillSwitch + TopBar integration already existed; this adds the keyboard shortcut and hardens the backend Fixes #132 * fix(agents): shield sleep+force-kill sequence from cancellation Wrap the entire 2-second grace window + SIGKILL sequence in asyncio.shield() for both stop_agent and bulk_stop_agents. Previously only stop_container calls were shielded; the preceding await asyncio.sleep(2) was not, so a request cancelled during the grace window would skip SIGKILL entirely. * fix(agents): gate force-kill on container run-state, add bounded-hang comment, fix shield orphan - Replace container_exists() with list_containers() status=='Running' check in both bulk_stop_agents and stop_agent force-kill paths. container_exists() only resolves project membership — it returns True for Stopped containers, causing misleading force_killed:false reports after incus stop --force failures on already-dead containers. - Add one-line comment noting that the sleep+force sequence inside asyncio.shield is bounded by the 120s _run cap. - Fix asyncio.shield() orphan: create a Task, catch CancelledError, cancel the detached grace-kill task, and await its cleanup before re-raising. - Add TestKillSwitchRunStateGate with 3 tests covering: * bulk_stop skips force-kill for Stopped containers * stop_agent skips force-kill for Stopped containers * bulk_stop still force-kills Running containers * fix(agents): surface CSRF failure in kill-switch, fallback force-kill on incus-down, rename force_error->force_output - desktop/App.tsx: check CSRF token before kill-switch POST, surface missing-token and server errors via notification store instead of silent catch{}; remove no-op ?. on withCsrf return. - agents.py bulk_stop: when list_containers returns empty but agents are configured (incus unhealthy), attempt force-kill on all agents anyway instead of silently skipping SIGKILL. - agents.py stop_agent: same incus-down fallback; rename force_error to force_output (it carries stdout even on success). * fix(agents): surface CSRF failure in kill-switch, fallback force-kill on incus-down, rename force_error->force_output - desktop/App.tsx: check CSRF token before kill-switch POST, surface missing-token and server errors via notification store instead of silent catch{}; use non-null assertion on withCsrf return (tsc). - agents.py bulk_stop: when list_containers returns empty but agents are configured (incus unhealthy), attempt force-kill on all agents anyway instead of silently skipping SIGKILL. - agents.py stop_agent: same incus-down fallback; rename force_error to force_output (it carries stdout even on success). * fix(tests): mock incus container ops in TestBulkOperations Mock stop_container, list_containers, start_container, and restart_container in TestBulkOperations to prevent FileNotFoundError when incus binary is not available (e.g. GitHub Actions CI). The kill-switch changes in PR #1962 added incus list/incus stop calls to the bulk stop flow, which exposed the missing mocks. Fixes test failures on feat/kill-switch. * fix(agents): distinguish incus-down from no-containers, parallelize force-kill, surface partial failures - containers: list_containers raises RuntimeError on incus failure instead of silently returning [] — callers can now distinguish incus-down from healthy-but-empty (Kilo W1/W2) - agents.py bulk_stop: catch RuntimeError in _grace_kill, set incus_unhealthy only on actual failure; parallelize force-kill with asyncio.gather (CR 5) - agents.py stop_agent: same incus-down distinction in single-agent path - agents.py list_agent_containers: return 503 when incus is unreachable - App.tsx: parse force_kill_results, surface warning for partial failures (CR 3) - test: fix assertion to check exact container name 'taos-agent-test-agent' (CR 4) CR 6 (cancellation defeats shielded task) already tracked by t_ff7cec2b — skipped. * fix(desktop): narrow force-kill failure filter to error-only Kilo review: Filter !r.force_killed || r.error flagged benign non-force-killed agents (clean SIGTERM stop) as failures. Changed to r.error — only surface agents that actually failed with an error. * fix(test): expect RuntimeError from list_containers on incus failure The kill-switch changes made list_containers() raise RuntimeError instead of returning an empty list on incus failure. Update test_handles_incus_failure to use pytest.raises(RuntimeError) to match the new behavior. Fixes CI regression in PR #1996. * test(agents): cover incus-down vs empty-container distinction in kill-switch Add two tests to TestKillSwitchRunStateGate: - test_bulk_stop_skips_force_kill_when_no_containers: empty container list (healthy host) must NOT force-kill - test_bulk_stop_force_kills_when_incus_down: RuntimeError from list_containers (incus down) MUST force-kill Complements the three fixes from e705a2a and 4c040de that Kilo flagged and jaylfc confirmed in #1996. * fix(agents): populate error field on force-kill failure for UI detection The desktop UI (commit 4c040de) filters force_kill_results on r.error, but the backend only set error on generic exceptions, not on force-kill command failures. When stop_container(force=True) returns success=False, the error field was missing, causing the UI to silently ignore genuine force-kill failures. Now populates error from the command output when force-kill fails, so the desktop's r.error filter correctly surfaces partial failures. CodeRabbit finding on PR #1996. * fix(agents): keep shield alive on cancellation — don't cancel force-kill task Both bulk and single-agent stop handlers used asyncio.shield to protect the grace-kill task from outer cancellation, then immediately defeated the shield with task.cancel() in the CancelledError handler. If the client disconnects during the 2-second grace window, the force-kill would be aborted — potentially leaving agent containers running after an emergency stop. Remove the explicit task.cancel() calls. The shield keeps the task alive; the handler now awaits it to completion before re-raising CancelledError, ensuring both SIGTERM and SIGKILL phases complete even on client disconnect. CodeRabbit finding on PR #1996. * fix(desktop): validate force_killed flag in kill-switch results Check !r.force_killed instead of r.error in the emergency kill-switch failure filter. The backend may return force_killed: false without an error field (e.g., when a container was not in the running set), and the previous filter would miss those entries, incorrectly showing a success notification. Fixes CodeRabbit finding: 'Validate force_kill_results before reporting success.' --------- Co-authored-by: Hogne <227774406+hognek@users.noreply.github.com>
…l-switch CancelledError handlers Removed explicit task.cancel() calls from the CancelledError exception handlers in both bulk_stop_agents (line ~858) and stop_agent (line ~955). Calling task.cancel() on a shielded task in its CancelledError handler aborts the shielded operation before it can send SIGKILL — defeating the 'shielded from cancellation' guarantee. Now the handlers simply await the shielded task (which continues running because asyncio.shield protects it), allowing the 2-second grace period and SIGKILL to complete before re-raising the cancellation. Closes Kilo WARNINGs on PR jaylfc#1962.
…l-switch handlers (#1988) * feat(agents): add Ctrl+Shift+K kill-switch shortcut + SIGTERM→SIGKILL grace window - Add Ctrl+Shift+K system shortcut in desktop SPA that triggers POST /api/agents/bulk/stop to immediately stop all running agents - Enhance bulk_stop_agents and stop_agent backend routes with SIGTERM (incus stop) → 2s grace → SIGKILL (incus stop --force) to guarantee termination of runaway agent containers - Desktop component AgentKillSwitch + TopBar integration already existed; this adds the keyboard shortcut and hardens the backend Fixes #132 * fix(agents): shield sleep+force-kill sequence from cancellation Wrap the entire 2-second grace window + SIGKILL sequence in asyncio.shield() for both stop_agent and bulk_stop_agents. Previously only stop_container calls were shielded; the preceding await asyncio.sleep(2) was not, so a request cancelled during the grace window would skip SIGKILL entirely. * fix(agents): gate force-kill on container run-state, add bounded-hang comment, fix shield orphan - Replace container_exists() with list_containers() status=='Running' check in both bulk_stop_agents and stop_agent force-kill paths. container_exists() only resolves project membership — it returns True for Stopped containers, causing misleading force_killed:false reports after incus stop --force failures on already-dead containers. - Add one-line comment noting that the sleep+force sequence inside asyncio.shield is bounded by the 120s _run cap. - Fix asyncio.shield() orphan: create a Task, catch CancelledError, cancel the detached grace-kill task, and await its cleanup before re-raising. - Add TestKillSwitchRunStateGate with 3 tests covering: * bulk_stop skips force-kill for Stopped containers * stop_agent skips force-kill for Stopped containers * bulk_stop still force-kills Running containers * fix(agents): surface CSRF failure in kill-switch, fallback force-kill on incus-down, rename force_error->force_output - desktop/App.tsx: check CSRF token before kill-switch POST, surface missing-token and server errors via notification store instead of silent catch{}; remove no-op ?. on withCsrf return. - agents.py bulk_stop: when list_containers returns empty but agents are configured (incus unhealthy), attempt force-kill on all agents anyway instead of silently skipping SIGKILL. - agents.py stop_agent: same incus-down fallback; rename force_error to force_output (it carries stdout even on success). * fix(agents): surface CSRF failure in kill-switch, fallback force-kill on incus-down, rename force_error->force_output - desktop/App.tsx: check CSRF token before kill-switch POST, surface missing-token and server errors via notification store instead of silent catch{}; use non-null assertion on withCsrf return (tsc). - agents.py bulk_stop: when list_containers returns empty but agents are configured (incus unhealthy), attempt force-kill on all agents anyway instead of silently skipping SIGKILL. - agents.py stop_agent: same incus-down fallback; rename force_error to force_output (it carries stdout even on success). * fix(agents): remove task.cancel() that defeated asyncio.shield in kill-switch CancelledError handlers Removed explicit task.cancel() calls from the CancelledError exception handlers in both bulk_stop_agents (line ~858) and stop_agent (line ~955). Calling task.cancel() on a shielded task in its CancelledError handler aborts the shielded operation before it can send SIGKILL — defeating the 'shielded from cancellation' guarantee. Now the handlers simply await the shielded task (which continues running because asyncio.shield protects it), allowing the 2-second grace period and SIGKILL to complete before re-raising the cancellation. Closes Kilo WARNINGs on PR #1962. * test(agents): add incus container mocks to test_bulk_stop for CI * fix(agents): address Kilo + CodeRabbit findings on kill-switch (#1988) - Start _grace_kill before graceful stop so grace window overlaps with container shutdown, not after (CodeRabbit) - Use asyncio.gather for concurrent force-kills instead of serial loop in bulk_stop_agents (CodeRabbit) - Remove Frozen/Freezing from run-state gate; only Running/Stopping trigger force-kill (Kilo SUGGESTION) - Fix inaccurate '120s _run cap' comment — no route-level timeout exists, only asyncio.shield (Kilo SUGGESTION) - Wrap list_containers in try/except to skip force-kill on transient incus errors, not on empty results (Kilo WARNING) - Parse per-agent failures in desktop kill-switch response before reporting success (CodeRabbit) - Fix test assertion: partial container name → full (CodeRabbit) - Add test_stop_agent_force_kills_running_container (CodeRabbit) - TypeScript: add Record<string,any> cast for filter callbacks * fix(agents): cancel orphaned _grace_kill task when graceful stop raises Wrap the graceful stop await in bulk_stop_agents and stop_agent in a try/except that cancels the grace_task on failure. If _bulk_container_op or stop_container raises, the grace_task was previously orphaned — it would fire a SIGKILL after 2s detached from the handler lifecycle. Addresses Kilo SUGGESTIONs from #1988 review cycle. --------- Co-authored-by: Hogne <227774406+hognek@users.noreply.github.com>
2 participants
What
Completes the agent kill switch feature (closes #132):
Ctrl+Shift+K keyboard shortcut — registered as a system-scope shortcut in the desktop SPA. Calls
POST /api/agents/bulk/stopjust like the existing "Kill all agents" button, then dispatchestaos:agents-changedso UI components pick up the state change.SIGTERM → 2s → SIGKILL grace window — both
bulk_stop_agentsandstop_agentroutes now:orchestrator.prepare()(informs agent frameworks of shutdown)incus stopincus stop --force(SIGKILL)Existing work preserved
The
AgentKillSwitchdropdown component and its TopBar integration were already implemented — this PR adds the keyboard shortcut and hardens the backend termination guarantees.Files changed
desktop/src/App.tsx— addedwithCsrfimport andCtrl+Shift+Kshortcut inSystemShortcutstinyagentos/routes/agents.py— hardenedbulk_stop_agentsandstop_agentwith force-kill after 2s graceTesting
__all__test_bulk_stopassertion (action == "stop") preserved — response is backward-compatible