Skip to content

Commit aa8bc7e

Browse files
committed
fix(settings): harden async lifecycle consistency
1 parent 51433fc commit aa8bc7e

12 files changed

Lines changed: 387 additions & 42 deletions

File tree

apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/deploy/hooks/use-deploy-readiness.test.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@ vi.mock('@/stores/operation-queue/store', () => ({
1010
getState: () => ({
1111
hasOperationError: false,
1212
hasPendingOperations: () => false,
13-
waitForWorkflowOperations: () => Promise.resolve(true),
13+
waitForWorkflowOperations: () => Promise.resolve('drained'),
1414
}),
1515
}
1616
),

apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/deploy/hooks/use-deploy-readiness.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -131,8 +131,8 @@ export function useDeployReadiness(workflowId: string | null): DeployReadiness {
131131
const queue = useOperationQueueStore.getState()
132132
if (queue.hasOperationError) return false
133133

134-
const drained = await queue.waitForWorkflowOperations(workflowId)
135-
if (!drained) return false
134+
const drainResult = await queue.waitForWorkflowOperations(workflowId)
135+
if (drainResult !== 'drained') return false
136136

137137
const latestQueue = useOperationQueueStore.getState()
138138
const diff = useWorkflowDiffStore.getState()

apps/sim/app/workspace/[workspaceId]/w/[workflowId]/hooks/use-workflow-execution.test.tsx

Lines changed: 57 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -9,13 +9,16 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
99
const {
1010
executionStoreState,
1111
mockCancel,
12+
mockAdoptScopedExecution,
13+
mockBeginScopedExecution,
14+
mockEndScopedExecution,
1215
mockExecute,
1316
mockExecuteFromBlock,
1417
mockFetch,
1518
mockHandleExecutionCancelledConsole,
1619
mockHandleExecutionErrorConsole,
17-
mockPersistenceExecutionEnded,
18-
mockPersistenceExecutionStarted,
20+
mockLoadExecutionPointer,
21+
mockReconnect,
1922
mockRequestJson,
2023
mockResolveStartCandidates,
2124
mockSelectBestTrigger,
@@ -86,13 +89,16 @@ const {
8689
return {
8790
executionStoreState,
8891
mockCancel: vi.fn(),
92+
mockAdoptScopedExecution: vi.fn(),
93+
mockBeginScopedExecution: vi.fn(() => ({})),
94+
mockEndScopedExecution: vi.fn(() => true),
8995
mockExecute: vi.fn(),
9096
mockExecuteFromBlock: vi.fn(),
9197
mockFetch: vi.fn(),
9298
mockHandleExecutionCancelledConsole: vi.fn(),
9399
mockHandleExecutionErrorConsole: vi.fn(),
94-
mockPersistenceExecutionEnded: vi.fn(),
95-
mockPersistenceExecutionStarted: vi.fn(() => ({})),
100+
mockLoadExecutionPointer: vi.fn(),
101+
mockReconnect: vi.fn(),
96102
mockRequestJson: vi.fn(),
97103
mockResolveStartCandidates: vi.fn(),
98104
mockSelectBestTrigger: vi.fn(),
@@ -210,7 +216,7 @@ vi.mock('@/hooks/use-execution-stream', () => {
210216
useExecutionStream: () => ({
211217
execute: mockExecute,
212218
executeFromBlock: mockExecuteFromBlock,
213-
reconnect: vi.fn(),
219+
reconnect: mockReconnect,
214220
cancel: mockCancel,
215221
cancelExecute: vi.fn(),
216222
cancelReconnect: vi.fn(),
@@ -241,11 +247,12 @@ vi.mock('@/stores/execution', () => ({
241247
vi.mock('@/stores/terminal', () => ({
242248
clearExecutionPointer: vi.fn(),
243249
consolePersistence: {
244-
executionStarted: mockPersistenceExecutionStarted,
245-
executionEnded: mockPersistenceExecutionEnded,
250+
adoptScopedExecution: mockAdoptScopedExecution,
251+
beginScopedExecution: mockBeginScopedExecution,
252+
endScopedExecution: mockEndScopedExecution,
246253
persist: vi.fn(),
247254
},
248-
loadExecutionPointer: vi.fn(),
255+
loadExecutionPointer: mockLoadExecutionPointer,
249256
saveExecutionPointer: vi.fn(),
250257
useTerminalConsoleStore: Object.assign(
251258
(selector: (state: typeof terminalStoreState) => unknown) => selector(terminalStoreState),
@@ -406,10 +413,14 @@ describe('useWorkflowExecution cancellation', () => {
406413
describe('useWorkflowExecution attachment uploads', () => {
407414
beforeEach(() => {
408415
vi.clearAllMocks()
416+
terminalStoreState._hasHydrated = false
409417
executionStoreState.getWorkflowExecution.mockReturnValue(
410418
executionStoreState.workflowExecutions.get('workflow-1')!
411419
)
412420
executionStoreState.getCurrentExecutionId.mockReturnValue(null)
421+
mockAdoptScopedExecution.mockReturnValue(undefined)
422+
mockLoadExecutionPointer.mockResolvedValue(null)
423+
mockReconnect.mockResolvedValue(undefined)
413424
mockResolveStartCandidates.mockReturnValue([])
414425
mockSelectBestTrigger.mockReturnValue([])
415426
vi.stubGlobal('fetch', mockFetch)
@@ -548,7 +559,7 @@ describe('useWorkflowExecution attachment uploads', () => {
548559
it('does not let an overlapping run without lifecycle ownership end the active run', async () => {
549560
const persistenceExecution = {}
550561
let resolveActiveRun: (() => void) | undefined
551-
mockPersistenceExecutionStarted.mockReturnValueOnce(persistenceExecution)
562+
mockBeginScopedExecution.mockReturnValueOnce(persistenceExecution)
552563
mockExecute.mockImplementationOnce(
553564
() =>
554565
new Promise<void>((resolve) => {
@@ -571,16 +582,49 @@ describe('useWorkflowExecution attachment uploads', () => {
571582
await result().handleRunWorkflow()
572583
})
573584

574-
expect(mockPersistenceExecutionStarted).toHaveBeenCalledTimes(1)
575-
expect(mockPersistenceExecutionEnded).not.toHaveBeenCalled()
585+
expect(mockBeginScopedExecution).toHaveBeenCalledTimes(1)
586+
expect(mockEndScopedExecution).not.toHaveBeenCalled()
576587

577588
await act(async () => {
578589
resolveActiveRun?.()
579590
await drainStream(activeRun)
580591
})
581592

582-
expect(mockPersistenceExecutionEnded).toHaveBeenCalledOnce()
583-
expect(mockPersistenceExecutionEnded).toHaveBeenCalledWith(persistenceExecution)
593+
expect(mockEndScopedExecution).toHaveBeenCalledOnce()
594+
expect(mockEndScopedExecution).toHaveBeenCalledWith('workflow-1', persistenceExecution)
595+
596+
unmount()
597+
})
598+
599+
it('adopts and finishes persistence ownership created before the hook mounted', async () => {
600+
const persistenceExecution = {}
601+
terminalStoreState._hasHydrated = true
602+
executionStoreState.getWorkflowExecution.mockReturnValue({
603+
...executionStoreState.getWorkflowExecution(),
604+
status: 'running',
605+
isExecuting: true,
606+
currentExecutionId: 'execution-1',
607+
})
608+
executionStoreState.getCurrentExecutionId.mockReturnValue('execution-1')
609+
mockLoadExecutionPointer.mockResolvedValue({
610+
workflowId: 'workflow-1',
611+
executionId: 'execution-1',
612+
lastEventId: 0,
613+
})
614+
mockAdoptScopedExecution.mockReturnValue(persistenceExecution)
615+
mockReconnect.mockImplementationOnce(async ({ callbacks }) => {
616+
callbacks.onExecutionCompleted({ finalBlockLogs: [] })
617+
})
618+
619+
const { unmount } = renderWorkflowExecutionHook()
620+
await act(async () => {
621+
await Promise.resolve()
622+
await Promise.resolve()
623+
})
624+
625+
expect(mockBeginScopedExecution).not.toHaveBeenCalled()
626+
expect(mockAdoptScopedExecution).toHaveBeenCalledWith('workflow-1')
627+
expect(mockEndScopedExecution).toHaveBeenCalledWith('workflow-1', persistenceExecution)
584628

585629
unmount()
586630
})

apps/sim/app/workspace/[workspaceId]/w/[workflowId]/hooks/use-workflow-execution.ts

Lines changed: 6 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -432,22 +432,19 @@ export function useWorkflowExecution() {
432432
const setCurrentExecutionId = useExecutionStore((s) => s.setCurrentExecutionId)
433433
const getCurrentExecutionId = useExecutionStore((s) => s.getCurrentExecutionId)
434434
const rawSetIsExecuting = useExecutionStore((s) => s.setIsExecuting)
435-
const persistenceExecutionsRef = useRef(new Map<string, ConsolePersistenceExecution>())
436435

437436
const endPersistenceExecution = useCallback((workflowId: string) => {
438-
const persistenceExecution = persistenceExecutionsRef.current.get(workflowId)
437+
const persistenceExecution = consolePersistence.adoptScopedExecution(workflowId)
439438
if (!persistenceExecution) return
440-
persistenceExecutionsRef.current.delete(workflowId)
441-
consolePersistence.executionEnded(persistenceExecution)
439+
consolePersistence.endScopedExecution(workflowId, persistenceExecution)
442440
}, [])
443441

444442
const setIsExecuting = useCallback(
445443
(workflowId: string, executing: boolean): ConsolePersistenceExecution | undefined => {
446444
const wasExecuting = useExecutionStore.getState().getWorkflowExecution(workflowId).isExecuting
447445
if (executing) {
448446
if (!wasExecuting) {
449-
const startedExecution = consolePersistence.executionStarted()
450-
persistenceExecutionsRef.current.set(workflowId, startedExecution)
447+
const startedExecution = consolePersistence.beginScopedExecution(workflowId)
451448
rawSetIsExecuting(workflowId, true)
452449
return startedExecution
453450
}
@@ -465,12 +462,11 @@ export function useWorkflowExecution() {
465462
const finishOwnedExecution = useCallback(
466463
(workflowId: string, persistenceExecution: ConsolePersistenceExecution | undefined) => {
467464
if (!persistenceExecution) return
468-
if (persistenceExecutionsRef.current.get(workflowId) !== persistenceExecution) return
469-
endPersistenceExecution(workflowId)
465+
if (!consolePersistence.endScopedExecution(workflowId, persistenceExecution)) return
470466
clearExecutionPointer(workflowId)
471467
rawSetIsExecuting(workflowId, false)
472468
},
473-
[endPersistenceExecution, rawSetIsExecuting]
469+
[rawSetIsExecuting]
474470
)
475471
const setIsDebugging = useExecutionStore((s) => s.setIsDebugging)
476472
const setPendingBlocks = useExecutionStore((s) => s.setPendingBlocks)
@@ -2490,7 +2486,7 @@ export function useWorkflowExecution() {
24902486
setCurrentExecutionId(reconnectWorkflowId, capturedExecutionId)
24912487
reconnectPersistenceExecution =
24922488
setIsExecuting(reconnectWorkflowId, true) ??
2493-
persistenceExecutionsRef.current.get(reconnectWorkflowId)
2489+
consolePersistence.adoptScopedExecution(reconnectWorkflowId)
24942490
activationOwnsPersistence = Boolean(reconnectPersistenceExecution)
24952491
if (fromEventId === 0) {
24962492
clearExecutionEntries(capturedExecutionId)

apps/sim/hooks/use-collaborative-workflow.ts

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -865,8 +865,13 @@ export function useCollaborativeWorkflow() {
865865
workflowId,
866866
})
867867
diffStore.markExternalUpdatePending(workflowId)
868-
void operationQueue.waitForWorkflowOperations(workflowId).then((ready) => {
869-
if (!ready) {
868+
void operationQueue.waitForWorkflowOperations(workflowId).then((result) => {
869+
if (result === 'cancelled') {
870+
useWorkflowDiffStore.getState().clearExternalUpdatePending(workflowId)
871+
return
872+
}
873+
874+
if (result === 'failed') {
870875
const latestQueue = useOperationQueueStore.getState()
871876
if (latestQueue.hasPendingOperations(workflowId) && !latestQueue.hasOperationError) {
872877
return
@@ -879,6 +884,7 @@ export function useCollaborativeWorkflow() {
879884
)
880885
return
881886
}
887+
882888
void replayPendingExternalUpdate(workflowId, 'deferred external update after local save')
883889
})
884890
return

0 commit comments

Comments
 (0)