mirror of
https://github.com/NanmiCoder/claude-code-haha.git
synced 2026-10-10 20:03:13 +08:00
fix(cli): make control-channel task stops idempotent for evicted tasks
Shell tasks are evicted from the CLI registry the turn after they terminate (and a process restart clears it outright), but the desktop Activity panel learns about termination only through forwarded events. When one is missed, the panel keeps showing the task as running and Stop answers with 'No task found with ID' — and for Agent tasks the failure was latched and replayed on every WS reconnect, flooding the chat with the same error. Stopping a task that is already gone is not an error: the goal state (not running) already holds. stopTaskFromControlRequest now answers not_found with an idempotent success carrying a structured reason, and the server converges on it: it untracks the task and synthesizes the terminal task_notification so clients drop the stale running entry instead of reporting background_task_stop_failed. The LLM-facing TaskStop tool keeps erroring so the model learns its handle is stale, and a CLI without the structured response keeps the explicit failure path.
This commit is contained in:
@@ -0,0 +1,111 @@
|
||||
import { afterEach, beforeEach, describe, expect, test } from 'bun:test'
|
||||
import { getIsInteractive, setIsInteractive } from '../bootstrap/state.js'
|
||||
import type { CanUseToolFn } from '../hooks/useCanUseTool.js'
|
||||
import type { AppState } from '../state/AppState.js'
|
||||
import { getDefaultAppState } from '../state/AppStateStore.js'
|
||||
import { Stream } from '../utils/stream.js'
|
||||
import { __runHeadlessStreamingForTests } from './print.js'
|
||||
import { StructuredIO } from './structuredIO.js'
|
||||
|
||||
function startHeadless(input: Stream<string>, tasks: AppState['tasks']) {
|
||||
const io = new StructuredIO(input)
|
||||
let appState = { ...getDefaultAppState(), tasks }
|
||||
const output = __runHeadlessStreamingForTests(
|
||||
io,
|
||||
[],
|
||||
[],
|
||||
[],
|
||||
[],
|
||||
(() => undefined) as unknown as CanUseToolFn,
|
||||
{},
|
||||
() => appState,
|
||||
update => {
|
||||
appState = update(appState)
|
||||
},
|
||||
[],
|
||||
{ outputFormat: 'stream-json' },
|
||||
)
|
||||
return { io, output, getState: () => appState }
|
||||
}
|
||||
|
||||
async function nextControlResponse(output: AsyncIterable<unknown>) {
|
||||
for await (const message of output) {
|
||||
if ((message as { type?: string }).type === 'control_response') return message
|
||||
}
|
||||
throw new Error('Missing control response')
|
||||
}
|
||||
|
||||
describe('stop_task control request', () => {
|
||||
let wasInteractive = true
|
||||
|
||||
beforeEach(() => {
|
||||
wasInteractive = getIsInteractive()
|
||||
setIsInteractive(false)
|
||||
})
|
||||
|
||||
afterEach(() => {
|
||||
setIsInteractive(wasInteractive)
|
||||
})
|
||||
|
||||
test('answers an unknown task id with an idempotent not_found success', async () => {
|
||||
const input = new Stream<string>()
|
||||
const { output } = startHeadless(input, {})
|
||||
input.enqueue(
|
||||
`${JSON.stringify({
|
||||
type: 'control_request',
|
||||
request_id: 'stop-unknown',
|
||||
request: { subtype: 'stop_task', task_id: 'b3e0jw079' },
|
||||
})}\n`,
|
||||
)
|
||||
|
||||
// UI Stop buttons lag the task registry: shell tasks are evicted the turn
|
||||
// after they terminate. not_found means the stop's goal state already
|
||||
// holds, so the wire answer is success — never "No task found with ID".
|
||||
await expect(nextControlResponse(output)).resolves.toMatchObject({
|
||||
type: 'control_response',
|
||||
response: {
|
||||
subtype: 'success',
|
||||
request_id: 'stop-unknown',
|
||||
response: { stopped: false, reason: 'not_found' },
|
||||
},
|
||||
})
|
||||
input.done()
|
||||
})
|
||||
|
||||
test('keeps erroring when the task exists but is not running', async () => {
|
||||
const input = new Stream<string>()
|
||||
const { output } = startHeadless(input, {
|
||||
'finished-task': {
|
||||
id: 'finished-task',
|
||||
type: 'local_bash',
|
||||
status: 'completed',
|
||||
description: 'Watch the release build',
|
||||
toolUseId: 'bash-tool-1',
|
||||
startTime: 1,
|
||||
outputFile: '/tmp/finished-task.output',
|
||||
outputOffset: 0,
|
||||
notified: true,
|
||||
completionStatusSentInAttachment: true,
|
||||
lastReportedTotalLines: 0,
|
||||
isBackgrounded: true,
|
||||
} as unknown as AppState['tasks'][string],
|
||||
})
|
||||
input.enqueue(
|
||||
`${JSON.stringify({
|
||||
type: 'control_request',
|
||||
request_id: 'stop-finished',
|
||||
request: { subtype: 'stop_task', task_id: 'finished-task' },
|
||||
})}\n`,
|
||||
)
|
||||
|
||||
await expect(nextControlResponse(output)).resolves.toMatchObject({
|
||||
type: 'control_response',
|
||||
response: {
|
||||
subtype: 'error',
|
||||
request_id: 'stop-finished',
|
||||
error: 'Task finished-task is not running (status: completed)',
|
||||
},
|
||||
})
|
||||
input.done()
|
||||
})
|
||||
})
|
||||
+16
-9
@@ -365,7 +365,7 @@ import { removeTeammateFromTeamFile } from '../utils/swarm/teamHelpers.js'
|
||||
import { unassignTeammateTasks } from '../utils/tasks.js'
|
||||
import { getRunningTasks } from '../utils/task/framework.js'
|
||||
import { isBackgroundTask } from '../tasks/types.js'
|
||||
import { stopTask } from '../tasks/stopTask.js'
|
||||
import { stopTaskFromControlRequest } from '../tasks/stopTask.js'
|
||||
import {
|
||||
drainSdkEvents,
|
||||
setAgentRunMessageSink,
|
||||
@@ -3941,14 +3941,21 @@ function runHeadlessStreaming(
|
||||
})
|
||||
} else if (message.request.subtype === 'stop_task') {
|
||||
const { task_id: taskId } = message.request
|
||||
try {
|
||||
await stopTask(taskId, {
|
||||
getAppState,
|
||||
setAppState,
|
||||
})
|
||||
sendControlResponseSuccess(message, {})
|
||||
} catch (error) {
|
||||
sendControlResponseError(message, errorMessage(error))
|
||||
const result = await stopTaskFromControlRequest(taskId, {
|
||||
getAppState,
|
||||
setAppState,
|
||||
})
|
||||
if (result.ok) {
|
||||
// alreadyGone: the registry already evicted the task (it
|
||||
// terminated earlier, or the process restarted). Tell the caller
|
||||
// explicitly so it can converge its stale "running" entry instead
|
||||
// of surfacing "No task found with ID" to the user.
|
||||
sendControlResponseSuccess(
|
||||
message,
|
||||
result.alreadyGone ? { stopped: false, reason: 'not_found' } : {},
|
||||
)
|
||||
} else {
|
||||
sendControlResponseError(message, result.message)
|
||||
}
|
||||
} else if (message.request.subtype === 'send_agent_message') {
|
||||
const agentId = message.request.agent_id.trim()
|
||||
|
||||
@@ -25,6 +25,7 @@ import { computerUseApprovalService } from '../services/computerUseApprovalServi
|
||||
import { sessionService } from '../services/sessionService.js'
|
||||
import * as titleService from '../services/titleService.js'
|
||||
import { SettingsService } from '../services/settingsService.js'
|
||||
import { activeBackgroundTaskIds } from '../ws/agentTaskState.js'
|
||||
import * as teleportApi from '../../utils/teleport/api.js'
|
||||
import { resetSettingsCache, setSessionSettingsCache } from '../../utils/settings/settingsCache.js'
|
||||
|
||||
@@ -3862,6 +3863,77 @@ describe('WebSocket handler session isolation', () => {
|
||||
})
|
||||
})
|
||||
|
||||
it('still reports a failure when a legacy CLI rejects with the plain not_found message', async () => {
|
||||
// Only the structured `{ reason: 'not_found' }` success converges. A CLI
|
||||
// without the idempotent stop keeps the explicit failure path — the
|
||||
// server never string-matches error text across the process boundary.
|
||||
const sessionId = `stop-background-legacy-${crypto.randomUUID()}`
|
||||
const ws = makeClientSocket(sessionId)
|
||||
spyOn(conversationService, 'requestControl')
|
||||
.mockRejectedValue(new Error('No task found with ID: bash-task-1'))
|
||||
handleWebSocket.open(ws)
|
||||
|
||||
handleWebSocket.message(ws, JSON.stringify({
|
||||
type: 'stop_background_task',
|
||||
taskId: 'bash-task-1',
|
||||
}))
|
||||
await Promise.resolve()
|
||||
await Promise.resolve()
|
||||
|
||||
expect(ws.sent.map((payload) => JSON.parse(payload))).toContainEqual({
|
||||
type: 'background_task_stop_failed',
|
||||
taskId: 'bash-task-1',
|
||||
message: 'No task found with ID: bash-task-1',
|
||||
})
|
||||
})
|
||||
|
||||
it('converges a stale running entry when the CLI reports the task already gone', async () => {
|
||||
const sessionId = `stop-background-evicted-${crypto.randomUUID()}`
|
||||
const ws = makeClientSocket(sessionId)
|
||||
let outputCallback: ((cliMsg: any) => void) | null = null
|
||||
spyOn(conversationService, 'hasSession').mockReturnValue(true)
|
||||
spyOn(conversationService, 'onOutput').mockImplementation((_sid, callback) => {
|
||||
outputCallback = callback
|
||||
})
|
||||
const requestControl = spyOn(conversationService, 'requestControl')
|
||||
.mockResolvedValue({ stopped: false, reason: 'not_found' })
|
||||
handleWebSocket.open(ws)
|
||||
|
||||
// The client still shows the task as running from an earlier task_started.
|
||||
outputCallback?.({
|
||||
type: 'system',
|
||||
subtype: 'task_started',
|
||||
task_id: 'bash-evicted-1',
|
||||
tool_use_id: 'bash-evicted-tool-1',
|
||||
description: 'Watch the release build',
|
||||
task_type: 'local_bash',
|
||||
})
|
||||
await flushMicrotasks()
|
||||
expect(activeBackgroundTaskIds.get(sessionId)?.has('bash-evicted-1')).toBe(true)
|
||||
ws.sent.length = 0
|
||||
|
||||
handleWebSocket.message(ws, JSON.stringify({
|
||||
type: 'stop_background_task',
|
||||
taskId: 'bash-evicted-1',
|
||||
}))
|
||||
await flushMicrotasks()
|
||||
|
||||
const sent = ws.sent.map((payload) => JSON.parse(payload))
|
||||
expect(sent.some((payload) => payload.type === 'background_task_stop_failed')).toBe(false)
|
||||
expect(sent).toContainEqual(expect.objectContaining({
|
||||
type: 'system_notification',
|
||||
subtype: 'task_notification',
|
||||
data: expect.objectContaining({
|
||||
task_id: 'bash-evicted-1',
|
||||
tool_use_id: 'bash-evicted-tool-1',
|
||||
status: 'stopped',
|
||||
}),
|
||||
}))
|
||||
// The task is untracked server-side, so reconnect snapshots no longer
|
||||
// list it as active and the terminal state survives a refresh.
|
||||
expect(activeBackgroundTaskIds.get(sessionId)?.has('bash-evicted-1') ?? false).toBe(false)
|
||||
})
|
||||
|
||||
it('rejects malformed background task ids without throwing from the async handler', async () => {
|
||||
const ws = makeClientSocket(`stop-background-invalid-${crypto.randomUUID()}`)
|
||||
const requestControl = spyOn(conversationService, 'requestControl').mockResolvedValue({})
|
||||
|
||||
@@ -1867,15 +1867,47 @@ async function requestStopBackgroundTask(
|
||||
}
|
||||
|
||||
try {
|
||||
await conversationService.requestControl(sessionId, {
|
||||
const response = await conversationService.requestControl(sessionId, {
|
||||
subtype: 'stop_task',
|
||||
task_id: taskId,
|
||||
})
|
||||
if (response?.reason === 'not_found') {
|
||||
convergeEvictedBackgroundTaskStop(sessionId, taskId)
|
||||
}
|
||||
} catch (error) {
|
||||
reportBackgroundTaskStopFailure(sessionId, ws, taskId, error)
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* The CLI evicts a shell task the turn after it terminates (and a process
|
||||
* restart clears the registry outright), so a Stop that lands late is
|
||||
* answered with `not_found`. That is the stop's goal state, not a failure:
|
||||
* drop the task from local tracking and send the terminal notification
|
||||
* clients need to converge an entry they still show as running. Reporting
|
||||
* `No task found with ID` here only re-arms the stop button for a task that
|
||||
* can never be stopped again.
|
||||
*/
|
||||
function convergeEvictedBackgroundTaskStop(sessionId: string, taskId: string): void {
|
||||
const tracked = activeNonAgentTasks.get(sessionId)?.get(taskId)
|
||||
untrackCliBackgroundTask(sessionId, taskId)
|
||||
const description = tracked?.description
|
||||
sendToSession(sessionId, {
|
||||
type: 'system_notification',
|
||||
subtype: 'task_notification',
|
||||
message: description ? `${description} stopped` : 'Background task stopped',
|
||||
data: {
|
||||
type: 'system',
|
||||
subtype: 'task_notification',
|
||||
task_id: taskId,
|
||||
tool_use_id: tracked?.toolUseId,
|
||||
status: 'stopped',
|
||||
summary: description ? `${description} stopped` : 'Background task stopped',
|
||||
timestamp: new Date().toISOString(),
|
||||
},
|
||||
})
|
||||
}
|
||||
|
||||
const AGENT_STOP_CONTROL_TIMEOUT_MS = 3_000
|
||||
const AUTHORITATIVE_STOP_PERSIST_ATTEMPTS = 3
|
||||
const AUTHORITATIVE_STOP_PERSIST_TIMEOUT_MS = 1_000
|
||||
|
||||
@@ -7,7 +7,7 @@ import {
|
||||
import type { AppState } from '../state/AppState.js'
|
||||
import type { SessionId } from '../types/ids.js'
|
||||
import { drainSdkEvents } from '../utils/sdkEventQueue.js'
|
||||
import { stopTask } from './stopTask.js'
|
||||
import { stopTask, stopTaskFromControlRequest } from './stopTask.js'
|
||||
|
||||
function makeShellTaskHarness(agentId?: string) {
|
||||
let killed = false
|
||||
@@ -98,3 +98,45 @@ describe('stopTask SDK events', () => {
|
||||
expect(drainSdkEvents()).toEqual([])
|
||||
})
|
||||
})
|
||||
|
||||
describe('stopTaskFromControlRequest', () => {
|
||||
test('reports a successful stop for a running task', async () => {
|
||||
const harness = makeShellTaskHarness()
|
||||
|
||||
const result = await stopTaskFromControlRequest('btask123', {
|
||||
getAppState: () => harness.state,
|
||||
setAppState: harness.setAppState,
|
||||
})
|
||||
|
||||
expect(result).toEqual({ ok: true, alreadyGone: false })
|
||||
expect(harness.killed).toBe(true)
|
||||
})
|
||||
|
||||
test('treats an unknown task id as an already-achieved stop, not an error', async () => {
|
||||
const harness = makeShellTaskHarness()
|
||||
|
||||
const result = await stopTaskFromControlRequest('evicted-task', {
|
||||
getAppState: () => harness.state,
|
||||
setAppState: harness.setAppState,
|
||||
})
|
||||
|
||||
expect(result).toEqual({ ok: true, alreadyGone: true })
|
||||
expect(harness.killed).toBe(false)
|
||||
})
|
||||
|
||||
test('keeps surfacing genuine stop failures', async () => {
|
||||
const harness = makeShellTaskHarness()
|
||||
harness.state.tasks.btask123.status = 'completed'
|
||||
|
||||
const result = await stopTaskFromControlRequest('btask123', {
|
||||
getAppState: () => harness.state,
|
||||
setAppState: harness.setAppState,
|
||||
})
|
||||
|
||||
expect(result).toEqual({
|
||||
ok: false,
|
||||
message: 'Task btask123 is not running (status: completed)',
|
||||
})
|
||||
expect(harness.killed).toBe(false)
|
||||
})
|
||||
})
|
||||
|
||||
@@ -102,3 +102,35 @@ export async function stopTask(
|
||||
|
||||
return { taskId, taskType: task.type, command }
|
||||
}
|
||||
|
||||
export type StopTaskControlResult =
|
||||
| { ok: true; alreadyGone: boolean }
|
||||
| { ok: false; message: string }
|
||||
|
||||
/**
|
||||
* stop_task control-request variant of {@link stopTask}. The LLM-facing
|
||||
* TaskStop tool must keep erroring on unknown ids so the model learns its
|
||||
* handle is stale, but the control channel is driven by UI Stop buttons whose
|
||||
* view of the task list lags the registry: shell tasks are evicted the turn
|
||||
* after they terminate, and a process restart clears the registry entirely.
|
||||
* A not_found there means the stop's goal state — not running — already
|
||||
* holds, so it reports success instead of an error. Callers key off
|
||||
* `alreadyGone` to converge any still-"running" entry of their own.
|
||||
*/
|
||||
export async function stopTaskFromControlRequest(
|
||||
taskId: string,
|
||||
context: StopTaskContext,
|
||||
): Promise<StopTaskControlResult> {
|
||||
try {
|
||||
await stopTask(taskId, context)
|
||||
return { ok: true, alreadyGone: false }
|
||||
} catch (error) {
|
||||
if (error instanceof StopTaskError && error.code === 'not_found') {
|
||||
return { ok: true, alreadyGone: true }
|
||||
}
|
||||
return {
|
||||
ok: false,
|
||||
message: error instanceof Error ? error.message : String(error),
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user