Skip to content

Commit d3ac06e

Browse files
odgrimclaude
andauthored
fix: block stuck on "running" (and a dead Stop button) when another block interrupts it (#192)
* fix: don't leave a block stuck on "running" when another block interrupts it Starting a block while another is still running left the first one spinning forever, with a Stop button that did nothing. The main process aborts every in-flight execution before starting a new one (electron/main/ipc/exec.ts), which kills the first block's process group and resolves its `exec:run` invoke as `{ status: null, cancelled: true }`. The renderer had no branch for that result: `exec:status` never arrives (main stops sending after the abort, and the listeners are already ignoring events now that a newer run owns `activeExecId`), and the invoke-result reconciliation only applied when a status was present. So the block held `running` over a child process that was already dead. Stop was dead in the same window because `runningExecIdRef` is cleared the moment the invoke settles, and `cancel()` bailed out entirely on a null id — no IPC, no log, no state change. - Treat a statusless result as terminal: back to `pending`, with a log line explaining that another block interrupted the run (skipped when the user cancelled it themselves, which `cancel()` already reports). - Keep the run's id in `lastExecIdRef` after the invoke settles so Stop still targets *this* hook's run. It names its own execution rather than falling back to "whatever ran last", so it can't reach into another block's script; cancelling an already-finished run is a no-op in main. - Key the cancellation log on the rendered status instead of id bookkeeping, so a stuck-looking block reports the stop and an idle one stays quiet. Tests cover the interleaved-run case with two hook instances, the single-run aborted result, no double-reporting on user cancel, and Stop after the invoke has resolved. Three of the four fail against the unfixed hook. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * chore: add "refreshable" to the docs dictionary The Documentation Tests job fails on every PR because GoogleAuth.mdx uses "refreshable-credential code path" and cspell doesn't know the word. Pre-existing on main, unrelated to this branch's fix, but it gates the checks — and the dictionary already carries the same kind of coinage (parallelizable, pasteable, sandboxed), so the word belongs there rather than the prose being reworded. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent c46ca64 commit d3ac06e

3 files changed

Lines changed: 157 additions & 9 deletions

File tree

docs/cspell.json

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,7 @@
3434
"prek",
3535
"prereq",
3636
"pyenv",
37+
"refreshable",
3738
"resourcemanager",
3839
"runbook",
3940
"runbooks",

web/src/hooks/useApiExec.test.ts

Lines changed: 101 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,10 @@ function createMockWindowApi() {
2525
const listeners = new Map<string, Set<EventCallback>>()
2626
let invokeResolve: ((value?: unknown) => void) | null = null
2727
let invokeReject: ((err: Error) => void) | null = null
28+
// Every pending exec:run resolver, in call order. Interleaved-run tests need
29+
// to settle an *earlier* block's invoke after a later one has started, which
30+
// the single `invokeResolve` slot above can't express.
31+
const invokeResolvers: Array<(value?: unknown) => void> = []
2832

2933
const api = {
3034
invoke: vi.fn((channel: string, ..._args: unknown[]) => {
@@ -36,6 +40,7 @@ function createMockWindowApi() {
3640
return new Promise<unknown>((resolve, reject) => {
3741
invokeResolve = resolve
3842
invokeReject = reject
43+
invokeResolvers.push(resolve)
3944
})
4045
}),
4146
on: vi.fn((channel: string, callback: EventCallback) => {
@@ -67,6 +72,10 @@ function createMockWindowApi() {
6772
rejectInvoke(err: Error) {
6873
invokeReject?.(err)
6974
},
75+
/** Resolve the nth invoke('exec:run') call (0-based, in call order) */
76+
resolveInvokeNth(index: number, value?: unknown) {
77+
invokeResolvers[index]?.(value)
78+
},
7079
}
7180
}
7281

@@ -226,6 +235,98 @@ describe('useApiExec state machine', () => {
226235
expect(result.current.state.exitCode).toBe(0)
227236
})
228237

238+
// ---------------------------------------------------------------------------
239+
// Interrupted runs (main aborts every in-flight execution when a new one
240+
// starts, resolving the aborted invoke as { status: null, cancelled: true }).
241+
// ---------------------------------------------------------------------------
242+
243+
it('an aborted run leaves "running" instead of spinning forever', async () => {
244+
const { result } = renderHook(() => useApiExec())
245+
246+
act(() => {
247+
result.current.execute('long-running-script')
248+
})
249+
expect(result.current.state.status).toBe('running')
250+
251+
await act(async () => {
252+
mock.resolveInvoke({ status: null, cancelled: true })
253+
})
254+
255+
await waitFor(() => expect(result.current.state.status).toBe('pending'))
256+
const lastLog = result.current.state.logs[result.current.state.logs.length - 1]
257+
expect(lastLog.line).toContain('another block was run')
258+
})
259+
260+
it('a block interrupted by a second block does not stay stuck on running', async () => {
261+
const first = renderHook(() => useApiExec())
262+
const second = renderHook(() => useApiExec())
263+
264+
act(() => {
265+
first.result.current.execute('slow-script')
266+
})
267+
expect(first.result.current.state.status).toBe('running')
268+
269+
// Second block starts before the first finishes. The main process aborts
270+
// the first run and kills its process group, then resolves its invoke with
271+
// no status — and the first block's listeners are already ignoring events
272+
// because the newer run owns activeExecId.
273+
act(() => {
274+
second.result.current.execute('other-script')
275+
})
276+
expect(second.result.current.state.status).toBe('running')
277+
278+
await act(async () => {
279+
mock.resolveInvokeNth(0, { status: null, cancelled: true })
280+
})
281+
282+
await waitFor(() => expect(first.result.current.state.status).toBe('pending'))
283+
expect(second.result.current.state.status).toBe('running')
284+
})
285+
286+
it('does not explain the stop twice when the user cancelled it', async () => {
287+
const { result } = renderHook(() => useApiExec())
288+
289+
act(() => {
290+
result.current.execute('long-running-script')
291+
})
292+
act(() => {
293+
result.current.cancel()
294+
})
295+
296+
await act(async () => {
297+
mock.resolveInvoke({ status: null, cancelled: true })
298+
})
299+
300+
const lines = result.current.state.logs.map((l) => l.line)
301+
expect(lines.filter((l) => l.includes('cancelled by user'))).toHaveLength(1)
302+
expect(lines.some((l) => l.includes('another block was run'))).toBe(false)
303+
})
304+
305+
it('cancel still targets this run after its invoke has already resolved', async () => {
306+
const { result } = renderHook(() => useApiExec())
307+
308+
act(() => {
309+
result.current.execute('long-running-script')
310+
})
311+
const runCalls = vi
312+
.mocked(mock.api.invoke)
313+
.mock.calls.filter(([channel]) => channel === 'exec:run')
314+
expect(runCalls).toHaveLength(1)
315+
const { executionId } = runCalls[0][1] as { executionId: string }
316+
317+
// The invoke settles (aborted by a newer run) — which used to clear the id
318+
// Stop depends on, leaving the button wired to nothing.
319+
await act(async () => {
320+
mock.resolveInvoke({ status: null, cancelled: true })
321+
})
322+
323+
act(() => {
324+
result.current.cancel()
325+
})
326+
327+
expect(mock.api.invoke).toHaveBeenCalledWith('exec:cancel', { executionId })
328+
})
329+
229330
it('reset: clears all state back to initial', async () => {
230331
const { result } = renderHook(() => useApiExec())
231332

web/src/hooks/useApiExec.ts

Lines changed: 55 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -110,11 +110,25 @@ export function useApiExec(options?: UseApiExecOptions): UseApiExecReturn {
110110
// still cancel a run whose listeners have already been detached — and so the
111111
// decision to cancel doesn't depend on listener lifecycle timing.
112112
const runningExecIdRef = useRef<string | null>(null)
113+
// The last execution id this hook started, kept even after `exec:run`
114+
// resolves. `runningExecIdRef` is cleared the moment the invoke settles, but
115+
// a run aborted by the main process settles while its child may still be
116+
// winding down — and while the UI still shows "running". Falling back to this
117+
// id keeps Stop working in that window. It always names THIS hook's own run,
118+
// never "whatever ran last", so Stop can't reach into another block's script.
119+
const lastExecIdRef = useRef<string | null>(null)
120+
// Set when this hook asked for the cancellation, so the completion handler
121+
// doesn't explain the stop a second time (cancel() already logged it).
122+
const selfCancelledRef = useRef(false)
113123

114124
const cancel = useCallback(() => {
115-
const execId = runningExecIdRef.current
125+
const execId = runningExecIdRef.current ?? lastExecIdRef.current
126+
selfCancelledRef.current = true
116127

117128
// Signal the backend to interrupt + kill *this* run's child process group.
129+
// Cancelling a run the main process has already finished with is a no-op
130+
// there (the id is dropped when the handler returns), so it's safe to send
131+
// whenever we have one.
118132
if (execId !== null) {
119133
window.api.invoke('exec:cancel', { executionId: execId }).catch(() => {})
120134
runningExecIdRef.current = null
@@ -126,14 +140,18 @@ export function useApiExec(options?: UseApiExecOptions): UseApiExecReturn {
126140
cleanupRef.current = null
127141
}
128142

129-
// Only add cancellation log and update state if there was actually an active execution
130-
if (execId !== null) {
131-
setState((prev) => ({
132-
...prev,
133-
status: 'pending',
134-
logs: [...prev.logs, createLogEntry('Execution cancelled by user')],
135-
}))
136-
}
143+
// Log the cancellation only when the block was actually showing a run in
144+
// progress. Keyed on the rendered status rather than the id bookkeeping,
145+
// so a stuck-looking block reports the stop and an idle one stays quiet.
146+
setState((prev) =>
147+
prev.status === 'running'
148+
? {
149+
...prev,
150+
status: 'pending',
151+
logs: [...prev.logs, createLogEntry('Execution cancelled by user')],
152+
}
153+
: prev,
154+
)
137155
}, [])
138156

139157
const reset = useCallback(() => {
@@ -170,6 +188,8 @@ export function useApiExec(options?: UseApiExecOptions): UseApiExecReturn {
170188
// can target this specific execution. Held in a ref for cancel() to read.
171189
const executionId = String(execId)
172190
runningExecIdRef.current = executionId
191+
lastExecIdRef.current = executionId
192+
selfCancelledRef.current = false
173193

174194
// Reset state for new execution
175195
setState({
@@ -261,6 +281,32 @@ export function useApiExec(options?: UseApiExecOptions): UseApiExecReturn {
261281
? { ...prev, status: finalStatus.status as ExecState['status'], exitCode: finalStatus.exitCode }
262282
: prev,
263283
)
284+
} else {
285+
// No status means the run was interrupted before it could report one:
286+
// the main process aborts every in-flight execution when a new one
287+
// starts, and an aborted run resolves as { status: null, cancelled:
288+
// true }. Its `exec:status` event never arrives either — main stops
289+
// sending after the abort, and these listeners are already ignoring
290+
// events now that a newer run owns `activeExecId`. Without this the
291+
// block spins on "running" forever, over a child process that was
292+
// killed. Self-cancellation is already reported by cancel().
293+
const explain = !selfCancelledRef.current
294+
setState((prev) =>
295+
prev.status === 'running' || prev.status === 'pending'
296+
? {
297+
...prev,
298+
status: 'pending',
299+
logs: explain
300+
? [
301+
...prev.logs,
302+
createLogEntry(
303+
'Execution stopped: another block was run before this one finished.',
304+
),
305+
]
306+
: prev.logs,
307+
}
308+
: prev,
309+
)
264310
}
265311
}
266312
// Schedule listener cleanup on the next macrotask so any IPC events

0 commit comments

Comments
 (0)