Skip to content

Commit e6b2b73

Browse files
PlaneInABottleclaudetest
authored
fix(execution): report cancellation durability truthfully (#3550)
* fix(cancel): report cancellation durability truthfully Return explicit durability results for execution cancellation so success only reflects persisted cancellation state instead of best-effort Redis availability. * fix: hoist cancellation test mocks Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> * fix(sim): harden execution cancel durability Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> * fix(sim): fallback manual cancel without redis Abort active manual SSE executions locally when Redis cannot durably record the cancellation marker so the run still finalizes as cancelled instead of completing normally. * test: mock AuthType in async execute route Keep the rebased async execute route test aligned with the current hybrid auth module exports so it exercises the queueing path instead of failing at import time. --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> Co-authored-by: test <test@example.com>
1 parent 9ae656c commit e6b2b73

File tree

7 files changed

+295
-10
lines changed

7 files changed

+295
-10
lines changed

apps/sim/app/api/workflows/[id]/execute/route.async.test.ts

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,11 @@ const {
1818
}))
1919

2020
vi.mock('@/lib/auth/hybrid', () => ({
21+
AuthType: {
22+
SESSION: 'session',
23+
API_KEY: 'api_key',
24+
INTERNAL_JWT: 'internal_jwt',
25+
},
2126
checkHybridAuth: mockCheckHybridAuth,
2227
AuthType: {
2328
SESSION: 'session',

apps/sim/app/api/workflows/[id]/execute/route.ts

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,10 @@ import {
2020
} from '@/lib/execution/call-chain'
2121
import { createExecutionEventWriter, setExecutionMeta } from '@/lib/execution/event-buffer'
2222
import { processInputFileFields } from '@/lib/execution/files'
23+
import {
24+
registerManualExecutionAborter,
25+
unregisterManualExecutionAborter,
26+
} from '@/lib/execution/manual-cancellation'
2327
import { preprocessExecution } from '@/lib/execution/preprocessing'
2428
import { LoggingSession } from '@/lib/logs/execution/logging-session'
2529
import {
@@ -845,6 +849,7 @@ export async function POST(req: NextRequest, { params }: { params: Promise<{ id:
845849
const encoder = new TextEncoder()
846850
const timeoutController = createTimeoutAbortController(preprocessResult.executionTimeout?.sync)
847851
let isStreamClosed = false
852+
let isManualAbortRegistered = false
848853

849854
const eventWriter = createExecutionEventWriter(executionId)
850855
setExecutionMeta(executionId, {
@@ -857,6 +862,9 @@ export async function POST(req: NextRequest, { params }: { params: Promise<{ id:
857862
async start(controller) {
858863
let finalMetaStatus: 'complete' | 'error' | 'cancelled' | null = null
859864

865+
registerManualExecutionAborter(executionId, timeoutController.abort)
866+
isManualAbortRegistered = true
867+
860868
const sendEvent = (event: ExecutionEvent) => {
861869
if (!isStreamClosed) {
862870
try {
@@ -1224,6 +1232,10 @@ export async function POST(req: NextRequest, { params }: { params: Promise<{ id:
12241232
})
12251233
finalMetaStatus = 'error'
12261234
} finally {
1235+
if (isManualAbortRegistered) {
1236+
unregisterManualExecutionAborter(executionId)
1237+
isManualAbortRegistered = false
1238+
}
12271239
try {
12281240
await eventWriter.close()
12291241
} catch (closeError) {
Lines changed: 148 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,148 @@
1+
/**
2+
* @vitest-environment node
3+
*/
4+
5+
import { NextRequest } from 'next/server'
6+
import { beforeEach, describe, expect, it, vi } from 'vitest'
7+
8+
const mockCheckHybridAuth = vi.fn()
9+
const mockAuthorizeWorkflowByWorkspacePermission = vi.fn()
10+
const mockMarkExecutionCancelled = vi.fn()
11+
const mockAbortManualExecution = vi.fn()
12+
13+
vi.mock('@sim/logger', () => ({
14+
createLogger: () => ({ info: vi.fn(), warn: vi.fn(), error: vi.fn() }),
15+
}))
16+
17+
vi.mock('@/lib/auth/hybrid', () => ({
18+
checkHybridAuth: (...args: unknown[]) => mockCheckHybridAuth(...args),
19+
}))
20+
21+
vi.mock('@/lib/execution/cancellation', () => ({
22+
markExecutionCancelled: (...args: unknown[]) => mockMarkExecutionCancelled(...args),
23+
}))
24+
25+
vi.mock('@/lib/execution/manual-cancellation', () => ({
26+
abortManualExecution: (...args: unknown[]) => mockAbortManualExecution(...args),
27+
}))
28+
29+
vi.mock('@/lib/workflows/utils', () => ({
30+
authorizeWorkflowByWorkspacePermission: (params: unknown) =>
31+
mockAuthorizeWorkflowByWorkspacePermission(params),
32+
}))
33+
34+
import { POST } from './route'
35+
36+
describe('POST /api/workflows/[id]/executions/[executionId]/cancel', () => {
37+
beforeEach(() => {
38+
vi.clearAllMocks()
39+
mockCheckHybridAuth.mockResolvedValue({ success: true, userId: 'user-1' })
40+
mockAuthorizeWorkflowByWorkspacePermission.mockResolvedValue({ allowed: true })
41+
mockAbortManualExecution.mockReturnValue(false)
42+
})
43+
44+
it('returns success when cancellation was durably recorded', async () => {
45+
mockMarkExecutionCancelled.mockResolvedValue({
46+
durablyRecorded: true,
47+
reason: 'recorded',
48+
})
49+
50+
const response = await POST(
51+
new NextRequest('http://localhost/api/workflows/wf-1/executions/ex-1/cancel', {
52+
method: 'POST',
53+
}),
54+
{
55+
params: Promise.resolve({ id: 'wf-1', executionId: 'ex-1' }),
56+
}
57+
)
58+
59+
expect(response.status).toBe(200)
60+
await expect(response.json()).resolves.toEqual({
61+
success: true,
62+
executionId: 'ex-1',
63+
redisAvailable: true,
64+
durablyRecorded: true,
65+
locallyAborted: false,
66+
reason: 'recorded',
67+
})
68+
})
69+
70+
it('returns unsuccessful response when Redis is unavailable', async () => {
71+
mockMarkExecutionCancelled.mockResolvedValue({
72+
durablyRecorded: false,
73+
reason: 'redis_unavailable',
74+
})
75+
76+
const response = await POST(
77+
new NextRequest('http://localhost/api/workflows/wf-1/executions/ex-1/cancel', {
78+
method: 'POST',
79+
}),
80+
{
81+
params: Promise.resolve({ id: 'wf-1', executionId: 'ex-1' }),
82+
}
83+
)
84+
85+
expect(response.status).toBe(200)
86+
await expect(response.json()).resolves.toEqual({
87+
success: false,
88+
executionId: 'ex-1',
89+
redisAvailable: false,
90+
durablyRecorded: false,
91+
locallyAborted: false,
92+
reason: 'redis_unavailable',
93+
})
94+
})
95+
96+
it('returns unsuccessful response when Redis persistence fails', async () => {
97+
mockMarkExecutionCancelled.mockResolvedValue({
98+
durablyRecorded: false,
99+
reason: 'redis_write_failed',
100+
})
101+
102+
const response = await POST(
103+
new NextRequest('http://localhost/api/workflows/wf-1/executions/ex-1/cancel', {
104+
method: 'POST',
105+
}),
106+
{
107+
params: Promise.resolve({ id: 'wf-1', executionId: 'ex-1' }),
108+
}
109+
)
110+
111+
expect(response.status).toBe(200)
112+
await expect(response.json()).resolves.toEqual({
113+
success: false,
114+
executionId: 'ex-1',
115+
redisAvailable: true,
116+
durablyRecorded: false,
117+
locallyAborted: false,
118+
reason: 'redis_write_failed',
119+
})
120+
})
121+
122+
it('returns success when local fallback aborts execution without Redis durability', async () => {
123+
mockMarkExecutionCancelled.mockResolvedValue({
124+
durablyRecorded: false,
125+
reason: 'redis_unavailable',
126+
})
127+
mockAbortManualExecution.mockReturnValue(true)
128+
129+
const response = await POST(
130+
new NextRequest('http://localhost/api/workflows/wf-1/executions/ex-1/cancel', {
131+
method: 'POST',
132+
}),
133+
{
134+
params: Promise.resolve({ id: 'wf-1', executionId: 'ex-1' }),
135+
}
136+
)
137+
138+
expect(response.status).toBe(200)
139+
await expect(response.json()).resolves.toEqual({
140+
success: true,
141+
executionId: 'ex-1',
142+
redisAvailable: false,
143+
durablyRecorded: false,
144+
locallyAborted: true,
145+
reason: 'redis_unavailable',
146+
})
147+
})
148+
})

apps/sim/app/api/workflows/[id]/executions/[executionId]/cancel/route.ts

Lines changed: 13 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ import { createLogger } from '@sim/logger'
22
import { type NextRequest, NextResponse } from 'next/server'
33
import { checkHybridAuth } from '@/lib/auth/hybrid'
44
import { markExecutionCancelled } from '@/lib/execution/cancellation'
5+
import { abortManualExecution } from '@/lib/execution/manual-cancellation'
56
import { authorizeWorkflowByWorkspacePermission } from '@/lib/workflows/utils'
67

78
const logger = createLogger('CancelExecutionAPI')
@@ -45,20 +46,27 @@ export async function POST(
4546

4647
logger.info('Cancel execution requested', { workflowId, executionId, userId: auth.userId })
4748

48-
const marked = await markExecutionCancelled(executionId)
49+
const cancellation = await markExecutionCancelled(executionId)
50+
const locallyAborted = abortManualExecution(executionId)
4951

50-
if (marked) {
52+
if (cancellation.durablyRecorded) {
5153
logger.info('Execution marked as cancelled in Redis', { executionId })
54+
} else if (locallyAborted) {
55+
logger.info('Execution cancelled via local in-process fallback', { executionId })
5256
} else {
53-
logger.info('Redis not available, cancellation will rely on connection close', {
57+
logger.warn('Execution cancellation was not durably recorded', {
5458
executionId,
59+
reason: cancellation.reason,
5560
})
5661
}
5762

5863
return NextResponse.json({
59-
success: true,
64+
success: cancellation.durablyRecorded || locallyAborted,
6065
executionId,
61-
redisAvailable: marked,
66+
redisAvailable: cancellation.reason !== 'redis_unavailable',
67+
durablyRecorded: cancellation.durablyRecorded,
68+
locallyAborted,
69+
reason: cancellation.reason,
6270
})
6371
} catch (error: any) {
6472
logger.error('Failed to cancel execution', { workflowId, executionId, error: error.message })
Lines changed: 84 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,84 @@
1+
import { loggerMock } from '@sim/testing'
2+
import { beforeEach, describe, expect, it, vi } from 'vitest'
3+
4+
const { mockGetRedisClient, mockRedisSet } = vi.hoisted(() => ({
5+
mockGetRedisClient: vi.fn(),
6+
mockRedisSet: vi.fn(),
7+
}))
8+
9+
vi.mock('@sim/logger', () => loggerMock)
10+
11+
vi.mock('@/lib/core/config/redis', () => ({
12+
getRedisClient: mockGetRedisClient,
13+
}))
14+
15+
import { markExecutionCancelled } from './cancellation'
16+
import {
17+
abortManualExecution,
18+
registerManualExecutionAborter,
19+
unregisterManualExecutionAborter,
20+
} from './manual-cancellation'
21+
22+
describe('markExecutionCancelled', () => {
23+
beforeEach(() => {
24+
vi.clearAllMocks()
25+
})
26+
27+
it('returns redis_unavailable when no Redis client exists', async () => {
28+
mockGetRedisClient.mockReturnValue(null)
29+
30+
await expect(markExecutionCancelled('execution-1')).resolves.toEqual({
31+
durablyRecorded: false,
32+
reason: 'redis_unavailable',
33+
})
34+
})
35+
36+
it('returns recorded when Redis write succeeds', async () => {
37+
mockRedisSet.mockResolvedValue('OK')
38+
mockGetRedisClient.mockReturnValue({ set: mockRedisSet })
39+
40+
await expect(markExecutionCancelled('execution-1')).resolves.toEqual({
41+
durablyRecorded: true,
42+
reason: 'recorded',
43+
})
44+
})
45+
46+
it('returns redis_write_failed when Redis write throws', async () => {
47+
mockRedisSet.mockRejectedValue(new Error('set failed'))
48+
mockGetRedisClient.mockReturnValue({ set: mockRedisSet })
49+
50+
await expect(markExecutionCancelled('execution-1')).resolves.toEqual({
51+
durablyRecorded: false,
52+
reason: 'redis_write_failed',
53+
})
54+
})
55+
})
56+
57+
describe('manual execution cancellation registry', () => {
58+
beforeEach(() => {
59+
unregisterManualExecutionAborter('execution-1')
60+
})
61+
62+
it('aborts registered executions', () => {
63+
const abort = vi.fn()
64+
65+
registerManualExecutionAborter('execution-1', abort)
66+
67+
expect(abortManualExecution('execution-1')).toBe(true)
68+
expect(abort).toHaveBeenCalledTimes(1)
69+
})
70+
71+
it('returns false when no execution is registered', () => {
72+
expect(abortManualExecution('execution-missing')).toBe(false)
73+
})
74+
75+
it('unregisters executions', () => {
76+
const abort = vi.fn()
77+
78+
registerManualExecutionAborter('execution-1', abort)
79+
unregisterManualExecutionAborter('execution-1')
80+
81+
expect(abortManualExecution('execution-1')).toBe(false)
82+
expect(abort).not.toHaveBeenCalled()
83+
})
84+
})

apps/sim/lib/execution/cancellation.ts

Lines changed: 14 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -6,27 +6,36 @@ const logger = createLogger('ExecutionCancellation')
66
const EXECUTION_CANCEL_PREFIX = 'execution:cancel:'
77
const EXECUTION_CANCEL_EXPIRY = 60 * 60
88

9+
export type ExecutionCancellationRecordResult =
10+
| { durablyRecorded: true; reason: 'recorded' }
11+
| {
12+
durablyRecorded: false
13+
reason: 'redis_unavailable' | 'redis_write_failed'
14+
}
15+
916
export function isRedisCancellationEnabled(): boolean {
1017
return getRedisClient() !== null
1118
}
1219

1320
/**
1421
* Mark an execution as cancelled in Redis.
15-
* Returns true if Redis is available and the flag was set, false otherwise.
22+
* Returns whether the cancellation was durably recorded.
1623
*/
17-
export async function markExecutionCancelled(executionId: string): Promise<boolean> {
24+
export async function markExecutionCancelled(
25+
executionId: string
26+
): Promise<ExecutionCancellationRecordResult> {
1827
const redis = getRedisClient()
1928
if (!redis) {
20-
return false
29+
return { durablyRecorded: false, reason: 'redis_unavailable' }
2130
}
2231

2332
try {
2433
await redis.set(`${EXECUTION_CANCEL_PREFIX}${executionId}`, '1', 'EX', EXECUTION_CANCEL_EXPIRY)
2534
logger.info('Marked execution as cancelled', { executionId })
26-
return true
35+
return { durablyRecorded: true, reason: 'recorded' }
2736
} catch (error) {
2837
logger.error('Failed to mark execution as cancelled', { executionId, error })
29-
return false
38+
return { durablyRecorded: false, reason: 'redis_write_failed' }
3039
}
3140
}
3241

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
const activeExecutionAborters = new Map<string, () => void>()
2+
3+
export function registerManualExecutionAborter(executionId: string, abort: () => void): void {
4+
activeExecutionAborters.set(executionId, abort)
5+
}
6+
7+
export function unregisterManualExecutionAborter(executionId: string): void {
8+
activeExecutionAborters.delete(executionId)
9+
}
10+
11+
export function abortManualExecution(executionId: string): boolean {
12+
const abort = activeExecutionAborters.get(executionId)
13+
if (!abort) {
14+
return false
15+
}
16+
17+
abort()
18+
return true
19+
}

0 commit comments

Comments
 (0)