Skip to content

fix(sandbox): escalate kill-all to SIGKILL when a task ignores SIGTERM#4516

Open
pedrofrxncx wants to merge 1 commit into
mainfrom
fix/sandbox-kill-all-sigkill-escalation
Open

fix(sandbox): escalate kill-all to SIGKILL when a task ignores SIGTERM#4516
pedrofrxncx wants to merge 1 commit into
mainfrom
fix/sandbox-kill-all-sigkill-escalation

Conversation

@pedrofrxncx

@pedrofrxncx pedrofrxncx commented Jul 14, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Bug found while reading packages/sandbox/daemon/process/task-manager.ts (recent churn area).
  • TaskManager.kill() and killByLogName() both escalate to SIGKILL after 3s if the task is still running post-SIGTERM, but killAll() only sent a bare SIGTERM with no follow-up. A task whose process ignores/traps TERM (a stuck subprocess, a shell that traps TERM) would survive a POST /_sandbox/tasks/kill-all call forever instead of being force-killed like every other kill path.
  • Payoff: the "kill everything" endpoint is meant to be the reliable last-resort stop; without escalation it silently fails for exactly the processes it exists to handle (ones that don't respond to a polite signal).

Failure scenario + regression test

Added packages/sandbox/daemon/process/task-manager.test.ts: spawns a task running trap '' TERM; while true; do :; done (a busy-loop shell that ignores TERM), calls killAll(), and asserts it ends up killed. On current main this test times out (5s) because the task never dies; with the fix it passes in ~3s (a SIGKILL fires after the 3s grace period).

How to verify

bun test packages/sandbox/daemon/process/task-manager.test.ts

Checks run locally

  • bun test packages/sandbox/daemon/process/task-manager.test.ts — 10 pass
  • cd packages/sandbox && bun run check (tsc --noEmit) — clean
  • bun run fmt — clean

Full CI will run the rest of the suite.


Summary by cubic

Escalates TaskManager.killAll() to send SIGKILL after 3s if a task ignores SIGTERM, matching other kill paths. This makes POST /_sandbox/tasks/kill-all reliably stop stuck or TERM-trapping tasks.

  • Bug Fixes
    • In packages/sandbox/daemon/process/task-manager.ts, follow SIGTERM with SIGKILL after 3s when a task is still running.
    • Added a regression test in packages/sandbox/daemon/process/task-manager.test.ts that spawns a shell loop trapping TERM and asserts killAll() results in killed.

Written for commit 340a877. Summary will update on new commits.

Review in cubic

TaskManager.kill() and killByLogName() both follow up a SIGTERM with a
SIGKILL after 3s if the task is still running, but killAll() sent a bare
SIGTERM with no escalation — a task whose process traps or ignores TERM
(a stuck subprocess, a shell with a TERM trap) would survive a kill-all
call indefinitely.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant