Stop becomes Kill when the job ignores it
A background job that ignores SIGTERM could not be stopped from the UI at all. Stop sends SIGTERM to every pid holding the job's output file, and the second click sent the same signal again — task-restart.ts already said "that is what the second click is for", and it was not. Reported after a job hung in another chat. The button now escalates instead of a second button appearing beside it. Press Stop, and if the job is still running three seconds later the same control becomes Kill in the destructive tone, and sends SIGKILL. Escalating rather than always offering Kill, because a permanent Kill button is a second destructive control competing with the first, and people reach for it by default — losing the graceful shutdown that usually works. The delay is the same argument: a clean exit takes well under a second, so offering Kill at once teaches everyone to press it first. `?force=1` on the existing DELETE rather than a second route: same verb on the same thing, differing only in how hard it asks. The member path escalates too — one `kill -KILL` run as them, since fuser and the kill must both be their own identity. Stated in the button's own title rather than hidden: SIGKILL gives the process no chance to release a port or take its children with it. That is the right trade for a hang and the reason it is not offered first. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
@@ -353,7 +353,10 @@ chatRouter.get('/tasks/:id', async (ctx) => {
|
||||
// say "please stop that" is a turn, and it only works while the agent is responsive.
|
||||
chatRouter.delete('/tasks/:id', async (ctx) => {
|
||||
const who = await chatIdentity(ctx.get('user'));
|
||||
const result = await stopBackgroundTask(who, ctx.req.param('id'));
|
||||
// `?force=1` escalates to SIGKILL. A query parameter rather than a second route: it is the same verb on
|
||||
// the same thing, differing only in how hard it asks.
|
||||
const force = ctx.req.query('force') === '1';
|
||||
const result = await stopBackgroundTask(who, ctx.req.param('id'), force);
|
||||
return ctx.json(result, result.stopped ? 200 : 400);
|
||||
});
|
||||
|
||||
|
||||
@@ -767,8 +767,14 @@ export async function pidsHoldingFile(who: ChatIdentity, path: string): Promise<
|
||||
* id is validated, then matched only under this caller's own directories. An id that resolves to nothing
|
||||
* kills nothing.
|
||||
*
|
||||
* SIGTERM, not SIGKILL. A dev server with children should get the chance to take them down with it;
|
||||
* SIGKILL orphans them and leaves the port bound, which is worse than a job that took a second to stop.
|
||||
* SIGTERM by default, and SIGKILL only when asked. A dev server with children should get the chance to
|
||||
* take them down with it; SIGKILL orphans them and leaves the port bound, which is worse than a job that
|
||||
* took a second to stop.
|
||||
*
|
||||
* `force` exists because that reasoning stops applying the moment a process ignores SIGTERM: there is
|
||||
* then no graceful shutdown to protect, only a job that cannot be stopped from the UI at all. The comment
|
||||
* in task-restart.ts already said "that is what the second click is for" — this is the second click
|
||||
* actually doing something different from the first.
|
||||
*
|
||||
* Only agent-run SHELL jobs can be stopped this way. A subagent writes a `.jsonl` transcript rather than
|
||||
* holding a log open, so there is no fd to trace and this reports that rather than pretending.
|
||||
@@ -776,7 +782,8 @@ export async function pidsHoldingFile(who: ChatIdentity, path: string): Promise<
|
||||
export async function stopBackgroundTask(
|
||||
who: ChatIdentity,
|
||||
taskId: string,
|
||||
): Promise<{ stopped: boolean; reason?: string }> {
|
||||
force = false,
|
||||
): Promise<{ stopped: boolean; reason?: string; signal?: 'TERM' | 'KILL' }> {
|
||||
if (!TASK_ID_RE.test(taskId)) return { stopped: false, reason: 'bad task id' };
|
||||
const found = findTaskOutput(who, taskId);
|
||||
if (!found) return { stopped: false, reason: 'no such task' };
|
||||
|
||||
@@ -1,8 +1,9 @@
|
||||
import type { ReactNode } from 'react';
|
||||
import { useEffect, useRef, useState } from 'react';
|
||||
import { toast } from 'sonner';
|
||||
import { shouldOfferKill } from '../kill-escalation';
|
||||
import { useClient } from 'hooks/useClient';
|
||||
import { Check, ChevronDown, CircleSlash, Loader2, Pause, Pin, Play, RotateCw, Square, X } from 'lucide-react';
|
||||
import { Check, ChevronDown, CircleSlash, Loader2, Pause, Pin, Play, RotateCw, Skull, Square, X } from 'lucide-react';
|
||||
import type { Tone } from '@/components/Data';
|
||||
import { toneText } from '@/components/Data';
|
||||
import type { ChatMessage } from '../types';
|
||||
@@ -56,7 +57,20 @@ export const BackgroundTaskTray = ({ messages, sessionId }: BackgroundTaskTrayPr
|
||||
* that state locally would just be a guess that usually agrees with reality and occasionally does not.
|
||||
* A stop that fails leaves the chip running, which is the truth.
|
||||
*/
|
||||
const stopTask = async (taskId: string) => {
|
||||
/**
|
||||
* When Stop was pressed on a job that is still running.
|
||||
*
|
||||
* SIGTERM is a request, and a process is free to ignore it. Until now the second click sent the same
|
||||
* signal again — `task-restart.ts` says "that is what the second click is for", and it was not. This is
|
||||
* what turns the button into Kill once the polite version has visibly failed.
|
||||
*
|
||||
* Local state rather than `consumeStopRequested`, which clears itself on the next notification because
|
||||
* its job is relabelling a finished task. This one has to survive for as long as the job stubbornly
|
||||
* does not.
|
||||
*/
|
||||
const [stopAttempted, setStopAttempted] = useState<Record<string, number>>({});
|
||||
|
||||
const stopTask = async (taskId: string, force = false) => {
|
||||
// Pressing stop is deliberate and aimed at one job, so its death is worth reacting to even if an
|
||||
// agent started it — the preview may well be pointed at exactly this.
|
||||
markUserBackgroundJob(taskId);
|
||||
@@ -64,13 +78,31 @@ export const BackgroundTaskTray = ({ messages, sessionId }: BackgroundTaskTrayPr
|
||||
// (observed live at exit code 144, read as "failed") — so the client relabels it itself. See
|
||||
// `background-job-origin.ts`.
|
||||
markStopRequested(taskId);
|
||||
setStopAttempted((prev) => ({ ...prev, [taskId]: Date.now() }));
|
||||
try {
|
||||
await client.delete(`/chat/tasks/${taskId}`);
|
||||
await client.delete(`/chat/tasks/${taskId}${force ? '?force=1' : ''}`);
|
||||
} catch (err) {
|
||||
toast.error(err instanceof Error ? err.message : 'Could not stop that job');
|
||||
}
|
||||
};
|
||||
|
||||
/**
|
||||
* Has a stop been asked for, and ignored?
|
||||
*
|
||||
* The delay is the point. A job that exits cleanly does so in well under a second, so offering Kill
|
||||
* immediately would train people to reach for it first — and SIGKILL gives a process no chance to take
|
||||
* its children down or release a port, which is worse than waiting a moment.
|
||||
*/
|
||||
const [now, setNow] = useState(Date.now());
|
||||
useEffect(() => {
|
||||
// Only ticks while something is pending, so an idle tray does no work.
|
||||
if (!Object.keys(stopAttempted).length) return;
|
||||
const id = setInterval(() => setNow(Date.now()), 1000);
|
||||
return () => clearInterval(id);
|
||||
}, [stopAttempted]);
|
||||
|
||||
const killOffered = (taskId: string) => shouldOfferKill(stopAttempted[taskId], now);
|
||||
|
||||
/**
|
||||
* Run this job's command again, without asking the agent.
|
||||
*
|
||||
@@ -126,7 +158,7 @@ export const BackgroundTaskTray = ({ messages, sessionId }: BackgroundTaskTrayPr
|
||||
// Finished only. A running chip is the only handle on work still going on, so closing one would hide
|
||||
// something you cannot get back to — the bulk clear draws the same line.
|
||||
onDismiss: task.status ? () => dismiss(task.taskId) : undefined,
|
||||
onStop: task.status ? undefined : () => void stopTask(task.taskId),
|
||||
onStop: task.status ? undefined : () => void stopTask(task.taskId, killOffered(task.taskId)),
|
||||
});
|
||||
|
||||
return (
|
||||
@@ -135,7 +167,8 @@ export const BackgroundTaskTray = ({ messages, sessionId }: BackgroundTaskTrayPr
|
||||
<TaskPanel
|
||||
task={open}
|
||||
onClose={() => setOpenId(null)}
|
||||
onStop={open.status ? undefined : () => void stopTask(open.taskId)}
|
||||
onStop={open.status ? undefined : (force?: boolean) => void stopTask(open.taskId, force)}
|
||||
killOffered={killOffered(open.taskId)}
|
||||
onPause={(next) => pauseTask(open.taskId, next)}
|
||||
paused={pausedIds.includes(open.taskId)}
|
||||
onRestart={() => void restartTask(open.taskId)}
|
||||
@@ -292,7 +325,10 @@ const TaskChip = ({ task, active, pinned, onClick, onTogglePin, onDismiss, onSto
|
||||
|
||||
type TaskPanelProps = {
|
||||
/** Present only while the task is running — the panel shows a real button rather than a bare glyph. */
|
||||
onStop?: () => void;
|
||||
/** Takes `force`: the same control, asking harder once a polite stop has been ignored. */
|
||||
onStop?: (force?: boolean) => void;
|
||||
/** Stop was pressed and the job is still running, so the button becomes Kill. */
|
||||
killOffered?: boolean;
|
||||
/** Freeze and thaw the process. Present while it is running; the label follows the state. */
|
||||
onPause?: (paused: boolean) => Promise<boolean>;
|
||||
/** Frozen right now — owned by the tray, so closing this panel does not forget it. */
|
||||
@@ -303,7 +339,15 @@ type TaskPanelProps = {
|
||||
onClose: () => void;
|
||||
};
|
||||
|
||||
const TaskPanel = ({ task, onClose, onStop, onPause, onRestart, paused = false }: TaskPanelProps) => {
|
||||
const TaskPanel = ({
|
||||
killOffered = false,
|
||||
task,
|
||||
onClose,
|
||||
onStop,
|
||||
onPause,
|
||||
onRestart,
|
||||
paused = false,
|
||||
}: TaskPanelProps) => {
|
||||
const running = !task.status;
|
||||
const { data, isLoading } = useTaskDetail(task.taskId, running);
|
||||
const bodyRef = useRef<HTMLDivElement | null>(null);
|
||||
@@ -364,12 +408,20 @@ const TaskPanel = ({ task, onClose, onStop, onPause, onRestart, paused = false }
|
||||
{running && onStop && (
|
||||
<button
|
||||
type="button"
|
||||
onClick={onStop}
|
||||
title="Stop this job"
|
||||
className="flex shrink-0 cursor-pointer items-center gap-1 rounded border border-border px-2 py-1 text-xs text-muted-foreground transition-colors hover:border-destructive/40 hover:text-destructive"
|
||||
onClick={() => onStop(killOffered)}
|
||||
title={
|
||||
killOffered
|
||||
? 'It ignored the request to stop. Kill it — no chance to clean up, release a port or take children with it.'
|
||||
: 'Stop this job'
|
||||
}
|
||||
className={`flex shrink-0 cursor-pointer items-center gap-1 rounded border px-2 py-1 text-xs transition-colors ${
|
||||
killOffered
|
||||
? 'border-destructive/40 text-destructive hover:bg-destructive/10'
|
||||
: 'border-border text-muted-foreground hover:border-destructive/40 hover:text-destructive'
|
||||
}`}
|
||||
>
|
||||
<Square className="h-3 w-3" />
|
||||
Stop
|
||||
{killOffered ? <Skull className="h-3 w-3" /> : <Square className="h-3 w-3" />}
|
||||
{killOffered ? 'Kill' : 'Stop'}
|
||||
</button>
|
||||
)}
|
||||
<button
|
||||
|
||||
@@ -0,0 +1,23 @@
|
||||
import { describe, expect, test } from 'bun:test';
|
||||
import { KILL_OFFERED_AFTER_MS, shouldOfferKill } from './kill-escalation';
|
||||
|
||||
describe('offering Kill', () => {
|
||||
test('not before a stop has been asked for at all', () => {
|
||||
expect(shouldOfferKill(undefined, Date.now())).toBe(false);
|
||||
});
|
||||
|
||||
test('not immediately — a clean exit takes well under a second', () => {
|
||||
// The rule worth protecting: offered at once, Kill becomes the button people press first, and SIGKILL
|
||||
// orphans children and leaves ports bound.
|
||||
const t = 1_000_000;
|
||||
expect(shouldOfferKill(t, t)).toBe(false);
|
||||
expect(shouldOfferKill(t, t + 500)).toBe(false);
|
||||
expect(shouldOfferKill(t, t + KILL_OFFERED_AFTER_MS)).toBe(false);
|
||||
});
|
||||
|
||||
test('once the request has plainly been ignored', () => {
|
||||
const t = 1_000_000;
|
||||
expect(shouldOfferKill(t, t + KILL_OFFERED_AFTER_MS + 1)).toBe(true);
|
||||
expect(shouldOfferKill(t, t + 60_000)).toBe(true);
|
||||
});
|
||||
});
|
||||
@@ -0,0 +1,19 @@
|
||||
/**
|
||||
* When to offer Kill instead of Stop.
|
||||
*
|
||||
* Stop sends SIGTERM, which is a request a process is free to ignore. Until this existed the second click
|
||||
* sent the same signal again — `task-restart.ts` said "that is what the second click is for", and it was
|
||||
* not — so a job that ignored SIGTERM could not be stopped from the UI at all.
|
||||
*
|
||||
* THE DELAY IS THE POINT. A job that exits cleanly does so in well under a second, so offering Kill
|
||||
* immediately would train people to reach for it first. SIGKILL gives a process no chance to take its
|
||||
* children down or release a port, which is worse than waiting a moment — so it is offered only once the
|
||||
* polite version has visibly failed.
|
||||
*/
|
||||
export const KILL_OFFERED_AFTER_MS = 3000;
|
||||
|
||||
/** Was a stop asked for long enough ago that it has plainly been ignored? */
|
||||
export function shouldOfferKill(attemptedAt: number | undefined, now: number): boolean {
|
||||
if (attemptedAt === undefined) return false;
|
||||
return now - attemptedAt > KILL_OFFERED_AFTER_MS;
|
||||
}
|
||||
Reference in New Issue
Block a user