Skip to content

Commit feccbda

Browse files
committed
fix(execution): stop terminal-race resumes before cleanup
1 parent df3a592 commit feccbda

4 files changed

Lines changed: 112 additions & 13 deletions

File tree

apps/sim/lib/execution/cancel-workflow-execution.test.ts

Lines changed: 36 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ const {
1717
mockFinalizePausedCancellationForTerminalRun,
1818
mockGetPausedCancellationStatus,
1919
mockGetActiveResumeCancellationTarget,
20+
mockGetActiveResumeCancellationTargets,
2021
mockRollbackActiveResumeCancellation,
2122
mockFinalizeExecutionStream,
2223
mockReadExecutionMetaState,
@@ -39,6 +40,7 @@ const {
3940
mockFinalizePausedCancellationForTerminalRun: vi.fn(),
4041
mockGetPausedCancellationStatus: vi.fn(),
4142
mockGetActiveResumeCancellationTarget: vi.fn(),
43+
mockGetActiveResumeCancellationTargets: vi.fn(),
4244
mockRollbackActiveResumeCancellation: vi.fn(),
4345
mockFinalizeExecutionStream: vi.fn(),
4446
mockReadExecutionMetaState: vi.fn(),
@@ -82,6 +84,8 @@ vi.mock('@/lib/workflows/executor/human-in-the-loop-manager', () => ({
8284
getPausedCancellationStatus: (...args: unknown[]) => mockGetPausedCancellationStatus(...args),
8385
getActiveResumeCancellationTarget: (...args: unknown[]) =>
8486
mockGetActiveResumeCancellationTarget(...args),
87+
getActiveResumeCancellationTargets: (...args: unknown[]) =>
88+
mockGetActiveResumeCancellationTargets(...args),
8589
rollbackActiveResumeCancellation: (...args: unknown[]) =>
8690
mockRollbackActiveResumeCancellation(...args),
8791
},
@@ -191,6 +195,7 @@ describe('cancelWorkflowExecution', () => {
191195
mockFinalizePausedCancellationForTerminalRun.mockReset().mockResolvedValue(true)
192196
mockGetPausedCancellationStatus.mockReset().mockResolvedValue(null)
193197
mockGetActiveResumeCancellationTarget.mockReset().mockResolvedValue(null)
198+
mockGetActiveResumeCancellationTargets.mockReset().mockResolvedValue([])
194199
mockRollbackActiveResumeCancellation.mockReset().mockResolvedValue(true)
195200
mockFinalizeExecutionStream.mockReset().mockResolvedValue(true)
196201
mockReadExecutionMetaState.mockReset().mockResolvedValue({ status: 'missing' })
@@ -1650,7 +1655,7 @@ describe('cancelWorkflowExecution', () => {
16501655
})
16511656
expect(mockWriteTerminalEvent).not.toHaveBeenCalled()
16521657
expect(mockCompletePausedCancellation).not.toHaveBeenCalled()
1653-
expect(mockFinalizePausedCancellationForTerminalRun).toHaveBeenCalledWith('ex-1', 'wf-1')
1658+
expect(mockFinalizePausedCancellationForTerminalRun).toHaveBeenCalledWith('ex-1', 'wf-1', [])
16541659
})
16551660

16561661
it('retries paused cancellation finalization before returning a terminal-race conflict', async () => {
@@ -1682,6 +1687,7 @@ describe('cancelWorkflowExecution', () => {
16821687
kind: 'active_resume',
16831688
target: ACTIVE_RESUME_TARGET,
16841689
})
1690+
mockGetActiveResumeCancellationTargets.mockResolvedValue([ACTIVE_RESUME_TARGET])
16851691
mockMarkExecutionCancelled.mockResolvedValue({ durablyRecorded: true, reason: 'recorded' })
16861692
const returning = vi.fn().mockResolvedValue([])
16871693
const where = vi.fn(() => ({ returning }))
@@ -1698,12 +1704,40 @@ describe('cancelWorkflowExecution', () => {
16981704
await expect(response.json()).resolves.toEqual({
16991705
error: 'Execution cannot be cancelled while completed',
17001706
})
1701-
expect(mockFinalizePausedCancellationForTerminalRun).toHaveBeenCalledWith('ex-1', 'wf-1')
1707+
expect(mockFinalizePausedCancellationForTerminalRun).toHaveBeenCalledWith('ex-1', 'wf-1', [
1708+
'resume-entry-1',
1709+
])
17021710
expect(mockRollbackActiveResumeCancellation).not.toHaveBeenCalled()
17031711
expect(mockClearPausedCancellationIntent).not.toHaveBeenCalled()
17041712
expect(mockClearExecutionCancellation).not.toHaveBeenCalled()
17051713
})
17061714

1715+
it('does not finalize a claimed resume that cannot be stopped after its parent is terminal', async () => {
1716+
mockGetActiveResumeCancellationTargets.mockResolvedValue([ACTIVE_RESUME_TARGET])
1717+
mockGetActiveResumeCancellationTarget.mockResolvedValue(ACTIVE_RESUME_TARGET)
1718+
dbChainMockFns.limit.mockResolvedValueOnce([
1719+
{
1720+
executionDeadlineAt: null,
1721+
executionOrigin: null,
1722+
status: 'completed',
1723+
workspaceId: 'workspace-1',
1724+
},
1725+
])
1726+
1727+
const response = await POST(makeRequest(), makeParams())
1728+
1729+
expect(response.status).toBe(500)
1730+
await expect(response.json()).resolves.toEqual({
1731+
error: 'Failed to reconcile paused execution after cancellation was rejected',
1732+
})
1733+
expect(mockMarkExecutionCancelled).toHaveBeenCalledTimes(3)
1734+
expect(mockMarkExecutionCancelled).toHaveBeenCalledWith('resume-ex-1', {
1735+
executionDeadlineAt: null,
1736+
})
1737+
expect(mockFinalizePausedCancellationForTerminalRun).not.toHaveBeenCalled()
1738+
expect(mockReleaseExecutionSlot).not.toHaveBeenCalled()
1739+
})
1740+
17071741
it('treats a concurrent cancellation as an idempotent success', async () => {
17081742
mockMarkExecutionCancelled.mockResolvedValue({ durablyRecorded: true, reason: 'recorded' })
17091743
dbChainMockFns.limit

apps/sim/lib/execution/cancel-workflow-execution.ts

Lines changed: 40 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -316,14 +316,45 @@ async function clearPausedCancellationIntentWithRetry(
316316

317317
async function finalizePausedCancellationForTerminalRunWithRetry(
318318
executionId: string,
319-
workflowId: string
319+
workflowId: string,
320+
executionDeadlineAt: Date | null,
321+
stopSummary: ExecutionStopSummary
320322
): Promise<boolean> {
321323
for (let attempt = 1; attempt <= PAUSED_CANCELLATION_DB_ATTEMPTS; attempt++) {
322324
try {
323-
const finalized = await PauseResumeManager.finalizePausedCancellationForTerminalRun(
325+
const activeResumeTargets = await PauseResumeManager.getActiveResumeCancellationTargets(
324326
executionId,
325327
workflowId
326328
)
329+
const stoppedResumeEntryIds: string[] = []
330+
for (const target of activeResumeTargets) {
331+
const stopped = await signalAndRecordActiveResumeStop({
332+
workflowId,
333+
executionId,
334+
executionDeadlineAt,
335+
target,
336+
summary: stopSummary,
337+
})
338+
if (!stopped) break
339+
stoppedResumeEntryIds.push(target.resumeEntryId)
340+
}
341+
342+
if (stoppedResumeEntryIds.length !== activeResumeTargets.length) {
343+
logger.warn('Claimed resume could not be stopped during terminal cleanup', {
344+
executionId,
345+
attempt,
346+
})
347+
if (attempt < PAUSED_CANCELLATION_DB_ATTEMPTS) {
348+
await sleep(PAUSED_CANCELLATION_DB_RETRY_MS)
349+
}
350+
continue
351+
}
352+
353+
const finalized = await PauseResumeManager.finalizePausedCancellationForTerminalRun(
354+
executionId,
355+
workflowId,
356+
stoppedResumeEntryIds
357+
)
327358
if (finalized) return true
328359
logger.warn('Paused cancellation terminal cleanup was rejected', {
329360
executionId,
@@ -771,9 +802,12 @@ export async function cancelWorkflowExecution({
771802
}
772803

773804
if (execution.status !== 'running' && execution.status !== 'pending') {
805+
const stopSummary = createExecutionStopSummary()
774806
const pausedCancellationFinalized = await finalizePausedCancellationForTerminalRunWithRetry(
775807
executionId,
776-
workflowId
808+
workflowId,
809+
execution.executionDeadlineAt,
810+
stopSummary
777811
)
778812
if (!pausedCancellationFinalized) throwPausedCancellationReconciliationFailed()
779813

@@ -975,7 +1009,9 @@ export async function cancelWorkflowExecution({
9751009
} else if (effectivePausedCancellationPath) {
9761010
const pausedCancellationFinalized = await finalizePausedCancellationForTerminalRunWithRetry(
9771011
executionId,
978-
workflowId
1012+
workflowId,
1013+
execution.executionDeadlineAt,
1014+
stopSummary
9791015
)
9801016
if (!pausedCancellationFinalized) throwPausedCancellationReconciliationFailed()
9811017
} else {

apps/sim/lib/workflows/executor/human-in-the-loop-manager.test.ts

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1288,7 +1288,9 @@ describe('PauseResumeManager paused cancellation after pause release', () => {
12881288
queueTableRows(resumeQueue, [{ id: 'resume-entry-1' }])
12891289

12901290
await expect(
1291-
PauseResumeManager.finalizePausedCancellationForTerminalRun('execution-1', 'workflow-1')
1291+
PauseResumeManager.finalizePausedCancellationForTerminalRun('execution-1', 'workflow-1', [
1292+
'resume-entry-1',
1293+
])
12921294
).resolves.toBe(true)
12931295

12941296
expect(dbChainMockFns.set).toHaveBeenNthCalledWith(1, {
@@ -1305,6 +1307,19 @@ describe('PauseResumeManager paused cancellation after pause release', () => {
13051307
expect(mockReleaseExecutionSlot).toHaveBeenCalledWith('resume-entry-1')
13061308
})
13071309

1310+
it('does not finalize or release an unconfirmed claimed resume', async () => {
1311+
queueTableRows(workflowExecutionLogs, [{ status: 'completed' }])
1312+
queueTableRows(pausedExecutions, [{ id: 'paused-exec-1', status: 'cancelling' }])
1313+
queueTableRows(resumeQueue, [{ id: 'resume-entry-1' }])
1314+
1315+
await expect(
1316+
PauseResumeManager.finalizePausedCancellationForTerminalRun('execution-1', 'workflow-1', [])
1317+
).resolves.toBe(false)
1318+
1319+
expect(dbChainMockFns.set).not.toHaveBeenCalled()
1320+
expect(mockReleaseExecutionSlot).not.toHaveBeenCalled()
1321+
})
1322+
13081323
it('restores only cancellation-staged queue entries while the workflow remains active', async () => {
13091324
queueTableRows(workflowExecutionLogs, [{ status: 'running' }])
13101325
queueTableRows(pausedExecutions, [{ id: 'paused-exec-1' }])

apps/sim/lib/workflows/executor/human-in-the-loop-manager.ts

Lines changed: 20 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -2736,10 +2736,12 @@ export class PauseResumeManager {
27362736
* Finalizes only pause and resume state when a non-cancellation terminal
27372737
* transition wins the parent execution race. The parent log is locked and
27382738
* inspected but never mutated, so a late claimed resume cannot revive it.
2739+
* Every claimed resume must be stopped before its queue row is finalized.
27392740
*/
27402741
static async finalizePausedCancellationForTerminalRun(
27412742
executionId: string,
2742-
workflowId: string
2743+
workflowId: string,
2744+
stoppedResumeEntryIds: string[]
27432745
): Promise<boolean> {
27442746
const now = new Date()
27452747

@@ -2791,6 +2793,11 @@ export class PauseResumeManager {
27912793
)
27922794
.for('update')
27932795

2796+
const stoppedResumeEntryIdSet = new Set(stoppedResumeEntryIds)
2797+
if (claimedResumeEntries.some((entry) => !stoppedResumeEntryIdSet.has(entry.id))) {
2798+
return { finalized: false, claimedResumeEntryIds: [] as string[] }
2799+
}
2800+
27942801
if (pausedExecution.status !== 'cancelled') {
27952802
await tx
27962803
.update(pausedExecutions)
@@ -2886,7 +2893,18 @@ export class PauseResumeManager {
28862893
executionId: string,
28872894
workflowId: string
28882895
): Promise<ActiveResumeCancellationTarget | null> {
2889-
const activeResume = await execDb
2896+
const activeResumes = await PauseResumeManager.getActiveResumeCancellationTargets(
2897+
executionId,
2898+
workflowId
2899+
)
2900+
return activeResumes[0] ?? null
2901+
}
2902+
2903+
static async getActiveResumeCancellationTargets(
2904+
executionId: string,
2905+
workflowId: string
2906+
): Promise<ActiveResumeCancellationTarget[]> {
2907+
return await execDb
28902908
.select({
28912909
resumeEntryId: resumeQueue.id,
28922910
pausedExecutionId: resumeQueue.pausedExecutionId,
@@ -2904,10 +2922,6 @@ export class PauseResumeManager {
29042922
)
29052923
)
29062924
.orderBy(desc(resumeQueue.claimedAt))
2907-
.limit(1)
2908-
.then((rows) => rows[0])
2909-
2910-
return activeResume ?? null
29112925
}
29122926

29132927
static async rollbackActiveResumeCancellation(

0 commit comments

Comments
 (0)