fix(agents): remove task.cancel() that defeated asyncio.shield in kill-switch handlers#1988
Conversation
|
Warning Review limit reached
Next review available in: 27 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)
📝 WalkthroughWalkthroughThe change adds a Ctrl+Shift+K emergency shortcut and extends bulk and single-agent stop routes with delayed, run-state-gated force termination after graceful stopping. ChangesAgent stop flow
Sequence Diagram(s)sequenceDiagram
participant User
participant SystemShortcuts
participant bulk_stop_agents
participant Incus
User->>SystemShortcuts: Press Ctrl+Shift+K
SystemShortcuts->>bulk_stop_agents: POST /api/agents/bulk/stop
bulk_stop_agents->>Incus: Gracefully stop agent containers
bulk_stop_agents->>Incus: List statuses after 2 seconds
bulk_stop_agents->>Incus: Force-stop running containers
bulk_stop_agents-->>SystemShortcuts: Return stop and force-kill results
SystemShortcuts-->>User: Show notification
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
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? |
|
|
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.
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| for c in containers | ||
| if c.status in ("Running", "Stopping", "Freezing", "Frozen") | ||
| } | ||
| incus_unhealthy = not containers and bool(config.agents) |
There was a problem hiding this comment.
WARNING: The incus_unhealthy fallback force-kills EVERY configured agent whenever list_containers returns an empty list — but list_containers returns [] on ANY error (see containers/__init__.py:160-166: non-zero exit, missing incus binary, malformed JSON), NOT only when incus is truly down. A transient incus hiccup, a permissions blip, or a brief window before state propagates will therefore send SIGKILL to all agents even when they are already stopped/healthy. This is far broader than the stated intent. Consider distinguishing "incus unhealthy" from "list returned empty but healthy" (e.g., by raising/propagating an error from list_containers and only falling back when the call itself raises), or at minimum logging at a higher severity and not force-killing already-stopped containers.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| for c in containers | ||
| if c.status in ("Running", "Stopping", "Freezing", "Frozen") | ||
| } | ||
| if container_name in running or not containers: |
There was a problem hiding this comment.
WARNING: Same over-broad fallback as bulk_stop_agents: not containers triggers an unconditional stop_container(force=True) on the agent whenever list_containers returns empty for ANY reason (transient incus error, not just an unhealthy incus). If the container is already stopped, this still issues a SIGKILL that fails and reports force_killed: False. Prefer only force-killing when the list call raises rather than returns empty, or skip force-kill when the container is known stopped.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| running = { | ||
| c.name | ||
| for c in containers | ||
| if c.status in ("Running", "Stopping", "Freezing", "Frozen") |
There was a problem hiding this comment.
SUGGESTION: The run-state gate includes "Frozen" and "Freezing". A Frozen container is already suspended (not running), so force-killing it via SIGKILL is likely not intended — it can lose a deliberately-frozen state and is indistinguishable from a genuine runaway. Freezing is a brief mid-transition state where a force-kill is reasonable. Consider narrowing the gate to ("Running", "Stopping") to avoid SIGKILLing frozen containers.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| return {"action": "stop", "prepare_report": report, "results": results} | ||
|
|
||
| # Phase 2: 2-second grace window, then SIGKILL any stragglers | ||
| # (shielded from cancellation; the sleep+force is bounded by the 120s _run cap) |
There was a problem hiding this comment.
SUGGESTION: The comment claims the sleep+force sequence is "bounded by the 120s _run cap", but no such timeout/cap wraps this route (no asyncio.wait_for/_run exists in this file or around these handlers — confirmed via search). asyncio.shield only protects the task from cancellation; it does not impose a time bound. If the intent is to cap the grace+force window, wrap _grace_kill() in asyncio.wait_for(..., timeout=...). Otherwise the comment is misleading and should be corrected.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| async def fake_stop(name, force=False): | ||
| return {"success": True, "output": ""} | ||
|
|
||
| async def fake_list_containers(prefix="taos-agent-"): |
There was a problem hiding this comment.
SUGGESTION: test_bulk_stop mocks list_containers to return [], which exercises only the incus_unhealthy force-kill fallback path (so force_killed is sent for test-agent unconditionally). It never exercises the intended normal Phase-2 gate (Running vs Stopped). Add a case that returns a Stopped ContainerInfo to verify the normal skip-force-kill branch, and keep a separate case for the empty-list fallback. This strengthens coverage of the exact behavior changed in this PR.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: 2 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL(none) WARNING
Files Reviewed (1 files)
Resolved since last review
Previous Review Summaries (2 snapshots, latest commit b25f750)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit b25f750)Status: 3 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL(none) WARNING
SUGGESTION
Files Reviewed (3 files)
Resolved since last review
Fix these issues in Kilo Cloud Previous review (commit 05e86b8)Status: 5 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL(none) WARNING
SUGGESTION
Files Reviewed (3 files)
Reviewed by hy3:free · Input: 37.1K · Output: 2.4K · Cached: 142.7K |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
tests/test_routes_agents.py (1)
189-224: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the corresponding running-container test for
stop_agent.The single-agent handler has separate force-kill and response logic, but only its stopped branch is exercised. Verify that it calls
force=Trueand returnsforce_killed: true.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_routes_agents.py` around lines 189 - 224, Add a test for the single-agent stop handler, alongside the existing stopped-branch coverage, using a running ContainerInfo and monkeypatched list/stop operations. Invoke stop_agent for that container, assert the stop call receives force=True, and verify the response reports force_killed: true.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@desktop/src/App.tsx`:
- Around line 133-142: Update the bulk stop response handling around the success
notification and the `bulk_stop_agents` flow to inspect per-agent failures in
both `results` and `force_kill_results`, even when the HTTP response is
successful. Only dispatch the agents-changed event and show the “Agents stopped”
success notification when no individual stop failures are reported; otherwise
surface the failure outcome instead of claiming all agents stopped.
In `@tests/test_routes_agents.py`:
- Around line 143-147: Update the stopped-container assertion in the relevant
test to compare force_calls against the actual container name, including its
taos-agent prefix, rather than the partial "test-agent" value. Preserve the
assertion that the stopped container must not receive a force kill.
In `@tinyagentos/routes/agents.py`:
- Around line 833-851: The force-kill loop around stop_container is serial and
delays later agents when Incus is unhealthy. Add a per-agent async helper that
performs the existing force=True call and preserves each agent’s success,
output, warning, and exception handling, then launch all helpers concurrently
with asyncio.gather and map their results back into force_results.
- Around line 814-820: The _grace_kill task currently starts only after the
graceful stop completes, leaving no concurrent shielded SIGKILL path during a
long shutdown or cancellation. In both tinyagentos/routes/agents.py:814-820 and
tinyagentos/routes/agents.py:917-923, create/start _grace_kill before awaiting
_bulk_container_op(config, stop_container), then preserve the existing shielded
grace-and-force-kill behavior and task cleanup.
---
Nitpick comments:
In `@tests/test_routes_agents.py`:
- Around line 189-224: Add a test for the single-agent stop handler, alongside
the existing stopped-branch coverage, using a running ContainerInfo and
monkeypatched list/stop operations. Invoke stop_agent for that container, assert
the stop call receives force=True, and verify the response reports force_killed:
true.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 0fde7133-1f9a-4cc5-8421-c298574e01f8
📒 Files selected for processing (3)
desktop/src/App.tsxtests/test_routes_agents.pytinyagentos/routes/agents.py
Bot-fix: Kilo + CodeRabbit findings addressedBranch: Changes
Test results
Please merge or cherry-pick at your convenience. |
…ce window (#1962) * 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. * 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 added incus list/incus stop calls to the bulk stop flow, which exposed the missing mocks. Fixes CI test failures on PR #1988. * fix(agents): address Kilo+CodeRabbit findings on kill-switch shield PR - 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. * fix(agents): broaden run-state gate to all non-Stopped and add 30s grace-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) --------- Co-authored-by: Hogne <227774406+hognek@users.noreply.github.com>
… 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
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.
… 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
… 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).
… 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).
…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.
05e86b8 to
8b897d5
Compare
…c#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
| task = asyncio.create_task(_grace_kill()) | ||
| grace_task = asyncio.create_task(_grace_kill()) | ||
|
|
||
| # Phase 2: SIGTERM every agent container |
There was a problem hiding this comment.
SUGGESTION: grace_task is created before the graceful stop await; if _bulk_container_op raises, grace_task is never awaited and becomes an orphaned background task.
Here grace_task (the shielded force-kill) is scheduled first, then results = await _bulk_container_op(config, stop_container) runs. If that await raises (e.g. incus error), execution never reaches await asyncio.shield(grace_task), so the task is orphaned: it keeps running detached, later force-kills containers outside the request scope, and can surface "Task exception was never retrieved" / event-loop warnings. Also, because grace_task only sleeps 2s before force-killing while the graceful stop may still be in flight, the two incus stop / incus stop --force calls can race on the same container.
Consider awaiting/shielding grace_task before (or independent of) the graceful stop, and starting the graceful stop before scheduling the force-kill so the operations don't overlap.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| task = asyncio.create_task(_grace_kill()) | ||
| grace_task = asyncio.create_task(_grace_kill()) | ||
|
|
||
| stop_result = await stop_container(container_name) |
There was a problem hiding this comment.
SUGGESTION: Same orphaned-task risk as the bulk path: grace_task is created before stop_result = await stop_container(container_name), so if the graceful stop raises, grace_task is never awaited and runs detached.
If await stop_container(container_name) (the graceful SIGTERM) raises, this route returns the error but grace_task is left orphaned — it will still sleep 2s and then call stop_container(container_name, force=True) on the same container outside the request, and may emit "Task exception was never retrieved" warnings. It also means the force-kill can race with a still-in-flight graceful stop on the same container.
Await/shield grace_task regardless of whether the graceful stop succeeds, or start the graceful stop before scheduling the force-kill.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
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 jaylfc#1988 review cycle.
2 participants
Fixes Kilo WARNINGs on PR #1962 (kill switch).
Problem: Both
bulk_stop_agentsandstop_agentcalltask.cancel()inside theirCancelledErrorhandlers, which defeats theasyncio.shieldthat's meant to keep SIGKILL running after client cancellation. The handler was explicitly canceling the shielded task before re-raising — SIGKILL did NOT survive request cancellation.Fix: Remove
task.cancel()from both handlers. The shielded task continues running (asyncio.shield protects it from the outer cancellation), soawait tasklets the 2-second grace period + SIGKILL complete. Only re-raise after the await.Changes:
bulk_stop_agents: lines 857-862 → nowawait task; raisestop_agent: lines 954-959 → nowawait task; raiseTests: 5/6 kill-switch tests pass (test_bulk_stop_force_kills_running_container times out — pre-existing, requires live incus)
Summary by Gitar
bulk_stop_agentsandstop_agentto implement a 2-second grace period withasyncio.shieldbefore force-killing stragglers.Running,Stopping, orFrozenstates.Ctrl+Shift+Kglobal keyboard shortcut to trigger an emergency bulk stop of all agents.TestKillSwitchRunStateGateto verify correct handling of container states during force-kill operations.This will update automatically on new commits.
Summary by CodeRabbit
New Features
Ctrl+Shift+K) to stop all running agents.Bug Fixes