-
Notifications
You must be signed in to change notification settings - Fork 0
fix(run-engine,webapp): guard run finalization against lost resume signals #4
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: qa/agent-triggerdotdev-trigger-dev/pr-06-4849/base
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| --- | ||
| area: webapp | ||
| type: fix | ||
| --- | ||
|
|
||
| A durable guard improves reliability for runs waiting on triggerAndWait or batchTriggerAndWait if there's a database error that interrupts a child run finishing. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,7 +7,7 @@ import { | |
| RedisCacheStore, | ||
| } from "@internal/cache"; | ||
| import type { RedisOptions } from "@internal/redis"; | ||
| import { startSpan } from "@internal/tracing"; | ||
| import { startSpan, type Counter } from "@internal/tracing"; | ||
| import { tryCatch } from "@trigger.dev/core/utils"; | ||
| import type { | ||
| CompleteRunAttemptResult, | ||
|
|
@@ -41,6 +41,7 @@ import { getMachinePreset, machinePresetFromName } from "../machinePresets.js"; | |
| import { retryOutcomeFromCompletion } from "../retrying.js"; | ||
| import { | ||
| isExecuting, | ||
| isFinalRunStatus, | ||
| isFinishedOrPendingFinished, | ||
| isInitialState, | ||
| isPendingExecuting, | ||
|
|
@@ -67,6 +68,7 @@ export type RunAttemptSystemOptions = { | |
| waitpointSystem: WaitpointSystem; | ||
| delayedRunSystem: DelayedRunSystem; | ||
| retryWarmStartThresholdMs?: number; | ||
| finalizationGuardDelayMs?: number; | ||
| machines: RunEngineOptions["machines"]; | ||
| redisOptions: RedisOptions; | ||
| }; | ||
|
|
@@ -102,12 +104,22 @@ const DEPLOYMENT_STALE_TTL = 60000 * 60 * 24 * 2; // 2 days | |
| const QUEUE_FRESH_TTL = 60000 * 60; // 1 hour | ||
| const QUEUE_STALE_TTL = 60000 * 60 * 2; // 2 hours | ||
|
|
||
| /** | ||
| * How many times the finalization guard defers to an in-flight cancellation before | ||
| * delivering anyway. The worker-owned states normally exit within a heartbeat cycle | ||
| * or two, so exhausting this budget means the heartbeat itself was lost; resuming the | ||
| * parent with the identical cancel error then beats watching forever. | ||
| */ | ||
| const MAX_FINALIZATION_GUARD_DEFERRALS = 10; | ||
|
|
||
| export class RunAttemptSystem { | ||
| private readonly $: SystemResources; | ||
| private readonly executionSnapshotSystem: ExecutionSnapshotSystem; | ||
| private readonly batchSystem: BatchSystem; | ||
| private readonly waitpointSystem: WaitpointSystem; | ||
| private readonly delayedRunSystem: DelayedRunSystem; | ||
| private readonly finalizationGuardDelayMs: number; | ||
| private readonly rederivationsCounter: Counter; | ||
| private readonly cache: UnkeyCache<{ | ||
| tasks: BackwardsCompatibleTaskRunExecution["task"]; | ||
| machinePresets: MachinePreset; | ||
|
|
@@ -123,6 +135,15 @@ export class RunAttemptSystem { | |
| this.batchSystem = options.batchSystem; | ||
| this.waitpointSystem = options.waitpointSystem; | ||
| this.delayedRunSystem = options.delayedRunSystem; | ||
| this.finalizationGuardDelayMs = options.finalizationGuardDelayMs ?? 60_000; | ||
| this.rederivationsCounter = this.$.meter.createCounter( | ||
| "run_attempt_system.finalization_rederivations", | ||
| { | ||
| description: | ||
| "Lost run-finalization side effects re-delivered by the ensureRunFinalized guard", | ||
| unit: "runs", | ||
| } | ||
| ); | ||
|
|
||
| const ctx = new DefaultStatefulContext(); | ||
| const memory = createLRUMemoryStore(5000); | ||
|
|
@@ -757,6 +778,8 @@ export class RunAttemptSystem { | |
| environmentType: latestSnapshot.environmentType, | ||
| }); | ||
|
|
||
| await this.#scheduleFinalizationGuard(runId); | ||
|
|
||
| const run = await this.$.runStore.completeAttemptSuccess( | ||
| runId, | ||
| { | ||
|
|
@@ -1439,6 +1462,8 @@ export class RunAttemptSystem { | |
| }); | ||
| } | ||
|
|
||
| await this.#scheduleFinalizationGuard(runId); | ||
|
|
||
| const run = await this.$.runStore.cancelRun( | ||
| runId, | ||
| { | ||
|
|
@@ -1654,6 +1679,8 @@ export class RunAttemptSystem { | |
| environmentType: latestSnapshot.environmentType, | ||
| }); | ||
|
|
||
| await this.#scheduleFinalizationGuard(runId); | ||
|
|
||
| //run permanently failed | ||
| const run = await this.$.runStore.failRunPermanently( | ||
| runId, | ||
|
|
@@ -1769,6 +1796,167 @@ export class RunAttemptSystem { | |
|
|
||
| //cancel the heartbeats | ||
| await this.$.worker.ack(`heartbeatSnapshot.${id}`); | ||
|
|
||
| await this.$.worker.ack(`ensureRunFinalized:${id}`); | ||
| } | ||
|
|
||
| /** | ||
| * Write-ahead guard for run finalization. Enqueued BEFORE the finish commit (so no | ||
| * finish write can exist without a durable watcher) and acked at the end of | ||
| * {@link #finalizeRun} once every inline side effect succeeded. It only ever | ||
| * executes when the inline path died in between. | ||
| */ | ||
| async #scheduleFinalizationGuard(runId: string, deferCount?: number): Promise<void> { | ||
| await this.$.worker.enqueue({ | ||
| id: `ensureRunFinalized:${runId}`, | ||
| job: "ensureRunFinalized", | ||
| payload: { runId, deferCount }, | ||
| availableAt: new Date(Date.now() + this.finalizationGuardDelayMs), | ||
| }); | ||
| } | ||
|
|
||
| /** | ||
| * Re-delivers a finished run's finalization side effects: the queue ack, the | ||
| * associated waitpoint's completion, the parent unblock fan-out, and the batch | ||
| * completion nudge. Safe to run at-least-once and to race the inline path — every leg | ||
| * is idempotent, and completing an already-completed waitpoint still re-runs the | ||
| * blocked-run fan-out (which covers a lost `continueRunIfUnblocked` enqueue). A | ||
| * non-final run means the finish commit itself never landed; the caller's retry | ||
| * re-runs the whole completion, so there is nothing to re-deliver. A canceled run | ||
| * whose worker is still winding down re-arms the guard and waits: the cancellation | ||
| * finalize path owns that window, and completing early would resume the parent while | ||
| * the child is still running. | ||
| */ | ||
| public async ensureRunFinalized({ | ||
| runId, | ||
| deferCount, | ||
| }: { | ||
| runId: string; | ||
| deferCount?: number; | ||
| }): Promise<void> { | ||
| return startSpan(this.$.tracer, "ensureRunFinalized", async (span) => { | ||
| span.setAttribute("runId", runId); | ||
|
|
||
| const run = await this.$.runStore.findRun( | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shipwright · HIGH The ensureRunFinalized method is public and directly callable from tests and other systems, but its cancellation deferral logic depends on deferCount being passed correctly. Impact: The ensureRunFinalized method is public and directly callable from tests and other systems, but its cancellation deferral logic depends on deferCount being passed correctly. A caller that omits deferCount resets the budget to zero on every invocation, allowing an unbounded number of deferrals if the worker never reaches a finished execution state. Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright. |
||
| { id: runId }, | ||
| { | ||
| select: { | ||
| id: true, | ||
| status: true, | ||
| output: true, | ||
| outputType: true, | ||
| error: true, | ||
| batchId: true, | ||
| runtimeEnvironmentId: true, | ||
| associatedWaitpoint: { | ||
| select: { | ||
| id: true, | ||
| status: true, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| this.$.prisma | ||
| ); | ||
|
|
||
| if (!run) { | ||
| this.$.logger.error("ensureRunFinalized: run not found", { runId }); | ||
| return; | ||
| } | ||
|
|
||
| if (!isFinalRunStatus(run.status)) { | ||
| this.$.logger.debug("ensureRunFinalized: run is not final, nothing to re-deliver", { | ||
| runId, | ||
| status: run.status, | ||
| }); | ||
| return; | ||
| } | ||
|
|
||
| if (run.status === "CANCELED") { | ||
| const latestSnapshot = await getLatestExecutionSnapshot( | ||
| this.$.prisma, | ||
| runId, | ||
| this.$.runStore | ||
| ); | ||
|
|
||
| /** | ||
| * Defer only while a worker still owns the execution: those states carry | ||
| * heartbeats that force the cancellation finalize path, which completes the | ||
| * waitpoint with the run's actual wind-down and acks this guard, so the watch | ||
| * always terminates. In any other snapshot state (queued, delayed, suspended, | ||
| * created) nobody is left to produce a FINISHED snapshot for a canceled run, | ||
| * so the guard must deliver or the parent is stranded. | ||
| */ | ||
| const workerOwnsExecution = | ||
| isExecuting(latestSnapshot.executionStatus) || | ||
| isPendingExecuting(latestSnapshot.executionStatus) || | ||
| latestSnapshot.executionStatus === "PENDING_CANCEL"; | ||
|
|
||
| if (latestSnapshot.executionStatus !== "FINISHED" && workerOwnsExecution) { | ||
| const currentDeferCount = deferCount ?? 0; | ||
|
|
||
| if (currentDeferCount < MAX_FINALIZATION_GUARD_DEFERRALS) { | ||
| this.$.logger.info( | ||
| "ensureRunFinalized: run is canceled but the worker is still winding down, keeping watch until the cancellation finalize path completes it", | ||
| { | ||
| runId, | ||
| executionStatus: latestSnapshot.executionStatus, | ||
| deferCount: currentDeferCount, | ||
| } | ||
| ); | ||
| await this.#scheduleFinalizationGuard(runId, currentDeferCount + 1); | ||
| return; | ||
| } | ||
|
|
||
| this.rederivationsCounter.add(1, { leg: "cancel_deferral_budget" }); | ||
| this.$.logger.warn( | ||
| "ensureRunFinalized: canceled run never reached a finished execution within the deferral budget, delivering anyway", | ||
| { | ||
| runId, | ||
| executionStatus: latestSnapshot.executionStatus, | ||
| deferCount: currentDeferCount, | ||
| } | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| span.setAttribute("runStatus", run.status); | ||
|
|
||
| const env = await this.$.controlPlaneResolver.resolveEnv(run.runtimeEnvironmentId); | ||
|
|
||
| if (env) { | ||
| await this.$.runQueue.acknowledgeMessage(env.organizationId, runId, { | ||
| removeFromWorkerQueue: true, | ||
| }); | ||
| } else { | ||
| this.$.logger.error("ensureRunFinalized: environment not found, skipping queue ack", { | ||
| runId, | ||
| runtimeEnvironmentId: run.runtimeEnvironmentId, | ||
| }); | ||
| } | ||
|
|
||
| if (run.associatedWaitpoint) { | ||
| const wasPending = run.associatedWaitpoint.status === "PENDING"; | ||
|
|
||
| if (wasPending) { | ||
| this.rederivationsCounter.add(1, { leg: "waitpoint" }); | ||
| this.$.logger.warn("ensureRunFinalized: re-deriving lost waitpoint completion", { | ||
| runId, | ||
| runStatus: run.status, | ||
| waitpointId: run.associatedWaitpoint.id, | ||
| }); | ||
| } | ||
|
|
||
| await this.waitpointSystem.completeWaitpoint({ | ||
| id: run.associatedWaitpoint.id, | ||
| output: this.waitpointSystem.buildWaitpointOutputFromRun(run), | ||
| }); | ||
| } | ||
|
|
||
| if (run.batchId) { | ||
| await this.batchSystem.scheduleCompleteBatch({ batchId: run.batchId }); | ||
| } | ||
| }); | ||
| } | ||
|
|
||
| async #resolveTaskRunExecutionTask( | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,15 +12,33 @@ import { boundedIn } from "@trigger.dev/database"; | |
| export type TtlSystemOptions = { | ||
| resources: SystemResources; | ||
| waitpointSystem: WaitpointSystem; | ||
| finalizationGuardDelayMs?: number; | ||
| }; | ||
|
|
||
| export class TtlSystem { | ||
| private readonly $: SystemResources; | ||
| private readonly waitpointSystem: WaitpointSystem; | ||
| private readonly finalizationGuardDelayMs: number; | ||
|
|
||
| constructor(private readonly options: TtlSystemOptions) { | ||
| this.$ = options.resources; | ||
| this.waitpointSystem = options.waitpointSystem; | ||
| this.finalizationGuardDelayMs = options.finalizationGuardDelayMs ?? 60_000; | ||
| } | ||
|
|
||
| /** | ||
| * Write-ahead guard for TTL expiry, mirroring the run attempt system's: enqueued | ||
| * before the EXPIRED commit so a crash or error between that commit and the | ||
| * waitpoint completion cannot strand a waiting parent, and acked once the inline | ||
| * side effects succeed. | ||
| */ | ||
| async #scheduleFinalizationGuard(runId: string): Promise<void> { | ||
| await this.$.worker.enqueue({ | ||
| id: `ensureRunFinalized:${runId}`, | ||
| job: "ensureRunFinalized", | ||
| payload: { runId }, | ||
| availableAt: new Date(Date.now() + this.finalizationGuardDelayMs), | ||
| }); | ||
| } | ||
|
|
||
| async expireRun({ runId, tx }: { runId: string; tx?: PrismaClientOrTransaction }) { | ||
|
|
@@ -62,6 +80,8 @@ export class TtlSystem { | |
| raw: `Run expired because the TTL (${run.ttl}) was reached`, | ||
| }; | ||
|
|
||
| await this.#scheduleFinalizationGuard(runId); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shipwright · CRITICAL In the single-run TTL path, the guard is scheduled before expireRun commits, but the ack only runs after expireRun succeeds. Impact: In the single-run TTL path, the guard is scheduled before expireRun commits, but the ack only runs after expireRun succeeds. If expireRun throws, the guard remains armed and later fires ensureRunFinalized for a run that was never expired. ensureRunFinalized only checks isFinalRunStatus, so a non-final run returns early, but the stale guard can still race a later legitimate finalization and re-deliver side effects fo… Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shipwright · MEDIUM The finalization guard is scheduled before the run is expired, but the ack in the single-run path is only executed inside the transaction callback after expireRun succeeds. Impact: The finalization guard is scheduled before the run is expired, but the ack in the single-run path is only executed inside the transaction callback after expireRun succeeds. If expireRun throws, the guard remains armed and will later fire ensureRunFinalized for a run that was never expired, potentially causing incorrect finalization side effects. Suggested fix: Fix the review finding before release. |
||
|
|
||
| const updatedRun = await this.$.runStore.expireRun( | ||
| runId, | ||
| { | ||
|
|
@@ -121,6 +141,8 @@ export class TtlSystem { | |
| project: { id: snapshot.projectId }, | ||
| environment: { id: snapshot.environmentId }, | ||
| }); | ||
|
|
||
| await this.$.worker.ack(`ensureRunFinalized:${runId}`); | ||
| }); | ||
| } | ||
|
|
||
|
|
@@ -220,6 +242,10 @@ export class TtlSystem { | |
| raw: "Run expired because the TTL was reached", | ||
| }; | ||
|
|
||
| await pMap(runsToExpire, (run) => this.#scheduleFinalizationGuard(run.id), { | ||
| concurrency: 10, | ||
| }); | ||
|
|
||
| await this.$.runStore.expireRunsBatch(runIdsToExpire, { error, now }, this.$.prisma); | ||
|
|
||
| // Process each run: enqueue waitpoint completion jobs and emit events | ||
|
|
@@ -262,6 +288,16 @@ export class TtlSystem { | |
| environment: { id: run.runtimeEnvironmentId }, | ||
| }); | ||
|
|
||
| /** | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shipwright · CRITICAL The batch TTL path acks the finalization guard for runs without an associatedWaitpoint immediately after enqueueing finishWaitpoint, but the comment claims the guard stays armed fo Impact: The batch TTL path acks the finalization guard for runs without an associatedWaitpoint immediately after enqueueing finishWaitpoint, but the comment claims the guard stays armed for runs with a waiting parent. The condition checks associatedWaitpoint, not whether a parent is actually waiting. A run with a waiting parent but no associatedWaitpoint will have its guard released prematurely, re-introducing the exact str… Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shipwright · HIGH In the batch path, the guard is acked for runs without an associatedWaitpoint immediately after enqueueing the finishWaitpoint job. Impact: In the batch path, the guard is acked for runs without an associatedWaitpoint immediately after enqueueing the finishWaitpoint job. However, the comment says the guard stays armed for runs with a waiting parent and verifies the completion landed. The condition checks associatedWaitpoint, not whether the run has a waiting parent, so a run with an associatedWaitpoint that is not actually waiting on a parent will keep… Suggested fix: Fix the review finding before release. |
||
| * Waitpoint completion in this path is delegated to the finishWaitpoint job, | ||
| * which can still exhaust its retries, so the guard stays armed for runs with | ||
| * a waiting parent and verifies the completion landed. Runs with no waitpoint | ||
| * have nothing left to re-deliver, so release their guard now. | ||
| */ | ||
| if (!run.associatedWaitpoint) { | ||
| await this.$.worker.ack(`ensureRunFinalized:${run.id}`); | ||
| } | ||
|
|
||
| expired.push(run.id); | ||
| } catch (e) { | ||
| this.$.logger.error("Failed to process expired run", { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shipwright · HIGH
The finalization guard delay is duplicated as a magic default of 60_000 in both RunAttemptSystem and TtlSystem constructors, with no shared constant.
Impact: The finalization guard delay is duplicated as a magic default of 60_000 in both RunAttemptSystem and TtlSystem constructors, with no shared constant. A future change to one default will silently diverge the two systems and produce inconsistent guard timing.
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.