Skip to content

Commit c33f494

Browse files
icecrasher321claude
andcommitted
fix(copilot): report the run from the engine, and never let recovery erase it
Two findings, both real. Firing before `engine.run` was still one step early: the engine's cancellation subscription is fallible and rejects having run nothing, so that failure claimed a run. The signal now fires inside the engine, immediately before the loop that processes blocks and past every startup step that can refuse a request — DAG construction, pipeline assembly and the subscription. The executor no longer guesses at the line from outside; the engine states it. Separately, the copilot catch path could throw while recording the failed crossing or releasing the execution slot. Either one propagated a different error — one the dispatched-run id was never recorded against — so an existing run reported itself as never started and invited the duplicate the id exists to prevent. Both are recovery work and neither may replace the failure it is describing, so both are contained and logged. Contained with try/catch rather than a rejection handler, since a synchronous throw has to be caught too. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent 0fdadad commit c33f494

4 files changed

Lines changed: 49 additions & 13 deletions

File tree

apps/sim/executor/execution/engine.ts

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -107,6 +107,15 @@ export class ExecutionEngine {
107107
this.initializeQueue(triggerBlockId)
108108
await this.subscribeToCancellationSignal()
109109

110+
/**
111+
* Past every fallible startup step — DAG construction, pipeline assembly and the
112+
* cancellation subscription above all reject having run nothing — and immediately
113+
* before the loop that processes blocks. This is the line a caller means by "could a
114+
* side effect have occurred"; anything earlier reports a run for a request that was
115+
* merely refused.
116+
*/
117+
this.context.onBlocksMayRun?.()
118+
110119
while (this.hasWork()) {
111120
if (this.checkCancellation() || this.errorFlag || this.stoppedEarlyFlag) {
112121
break

apps/sim/executor/execution/executor.ts

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -101,7 +101,6 @@ export class DAGExecutor {
101101
this.registerRestoredClonedSubflows(context.subflowParentMap, restoredClonedSubflows)
102102

103103
const engine = this.buildExecutionPipeline(context, dag, state)
104-
this.contextExtensions.onBlocksMayRun?.()
105104
return await engine.run(triggerBlockId)
106105
}
107106

@@ -259,7 +258,6 @@ export class DAGExecutor {
259258
context.subflowParentMap = this.buildSubflowParentMap(dag)
260259

261260
const engine = this.buildExecutionPipeline(context, dag, state, filteredSnapshot)
262-
this.contextExtensions.onBlocksMayRun?.()
263261
const result = await engine.run()
264262
if (result.metadata) {
265263
result.metadata.largeValueKeys = context.largeValueKeys
@@ -417,6 +415,7 @@ export class DAGExecutor {
417415

418416
const context: ExecutionContext = {
419417
workflowId,
418+
onBlocksMayRun: this.contextExtensions.onBlocksMayRun,
420419
workspaceId: this.contextExtensions.workspaceId,
421420
executionId: this.contextExtensions.executionId,
422421
largeValueExecutionIds: this.contextExtensions.largeValueExecutionIds,

apps/sim/executor/types.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -372,6 +372,8 @@ export interface ExecutorDelegationOrigin {
372372
}
373373

374374
export interface ExecutionContext {
375+
/** See {@link ContextExtensions.onBlocksMayRun}. Fired by the engine, once. */
376+
onBlocksMayRun?: () => void
375377
workflowId: string
376378
workspaceId?: string
377379
executionId?: string

apps/sim/lib/workflows/application/run-workflow-from-copilot.ts

Lines changed: 37 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
import { type Principal, requirePrincipalSubjectUserId } from '@sim/auth/principal'
2+
import { createLogger } from '@sim/logger'
23
import { toError } from '@sim/utils/errors'
34
import { generateId } from '@sim/utils/id'
45
import { mergeSubblockStateWithValues } from '@sim/workflow-persistence/subblocks'
@@ -29,6 +30,9 @@ import {
2930
import type { SerializableExecutionState } from '@/executor/execution/types'
3031
import type { ExecutionResult } from '@/executor/types'
3132
import { attachAttemptedExecutionId } from '@/executor/utils/errors'
33+
34+
const logger = createLogger('CopilotWorkflowRun')
35+
3236
import type { ResolvedSecretTraceRegistry } from '@/executor/utils/resolved-secret-trace-registry'
3337

3438
export interface CopilotWorkflowRunLifecycle {
@@ -305,6 +309,12 @@ async function executeCopilotRun(params: {
305309
* what threw and an execution certainly exists.
306310
*/
307311
if (runReturned) attachAttemptedExecutionId(error, childExecutionId)
312+
/**
313+
* Recovery must never replace the failure it is describing. Both steps below run only to
314+
* record and release, and either throwing would propagate a different error — one the
315+
* dispatched-run id was never recorded against — so an existing run would report itself
316+
* as never started and invite the duplicate this id exists to prevent.
317+
*/
308318
if (registry) {
309319
const executionResult =
310320
typeof error === 'object' &&
@@ -313,18 +323,34 @@ async function executeCopilotRun(params: {
313323
typeof error.executionResult === 'object'
314324
? (error.executionResult as ExecutionResult)
315325
: undefined
316-
await registry.importCrossingProvenance(
317-
executionResult?.executionState?.resolvedSecretTraceProvenance,
318-
{
319-
output: executionResult?.output,
320-
logs: executionResult?.logs,
321-
error: executionResult?.error,
322-
thrownMessage: toError(error).message,
323-
},
324-
{ trusted: true, origin: 'copilotWorkflowMutation.failedRunCrossing' }
325-
)
326+
try {
327+
await registry.importCrossingProvenance(
328+
executionResult?.executionState?.resolvedSecretTraceProvenance,
329+
{
330+
output: executionResult?.output,
331+
logs: executionResult?.logs,
332+
error: executionResult?.error,
333+
thrownMessage: toError(error).message,
334+
},
335+
{ trusted: true, origin: 'copilotWorkflowMutation.failedRunCrossing' }
336+
)
337+
} catch (importError) {
338+
logger.error('Failed to record provenance for a failed Copilot run', {
339+
executionId: childExecutionId,
340+
error: toError(importError).message,
341+
})
342+
}
343+
}
344+
if (admission.targetReservation) {
345+
try {
346+
await releaseExecutionSlot(childExecutionId)
347+
} catch (releaseError) {
348+
logger.error('Failed to release the execution slot for a failed Copilot run', {
349+
executionId: childExecutionId,
350+
error: toError(releaseError).message,
351+
})
352+
}
326353
}
327-
if (admission.targetReservation) await releaseExecutionSlot(childExecutionId)
328354
throw error
329355
} finally {
330356
completePendingActivation?.()

0 commit comments

Comments
 (0)