fix(run-engine,webapp): guard run finalization against lost resume signals - #4
Conversation
…gnals Source PR: triggerdotdev#4849 Source head: 933a6a6
⛔ Shipwright · BlockedRecommendation: do not merge PR #4 · Tier
Findings (7)
Fireworks usage: 30,893 input · 925 output · 31,818 total tokens · $0.0074 · 17s · 0 fix iteration(s) Open the Shipwright check for full evidence and the audit bundle. Use |
![shipwright-agent[bot]](https://avatars.githubusercontent.com/u/140162738?s=60&v=4){kind=small}
| environment: { id: run.runtimeEnvironmentId }, | ||
| }); | ||
|
|
||
| /** |
There was a problem hiding this comment.
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.
| raw: `Run expired because the TTL (${run.ttl}) was reached`, | ||
| }; | ||
|
|
||
| await this.#scheduleFinalizationGuard(runId); |
There was a problem hiding this comment.
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.
| environment: { id: run.runtimeEnvironmentId }, | ||
| }); | ||
|
|
||
| /** |
There was a problem hiding this comment.
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.
| this.batchSystem = options.batchSystem; | ||
| this.waitpointSystem = options.waitpointSystem; | ||
| this.delayedRunSystem = options.delayedRunSystem; | ||
| this.finalizationGuardDelayMs = options.finalizationGuardDelayMs ?? 60_000; |
There was a problem hiding this comment.
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.
| return startSpan(this.$.tracer, "ensureRunFinalized", async (span) => { | ||
| span.setAttribute("runId", runId); | ||
|
|
||
| const run = await this.$.runStore.findRun( |
There was a problem hiding this comment.
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.
| * silently forever. | ||
| */ | ||
| ensureRunFinalized: { | ||
| schema: z.object({ |
There was a problem hiding this comment.
Shipwright · HIGH
The ensureRunFinalized worker job accepts a runId and deferCount with no authentication or authorization boundary.
Impact: The ensureRunFinalized worker job accepts a runId and deferCount with no authentication or authorization boundary. Any caller that can enqueue to the worker catalog can force re-delivery of finalization side effects for arbitrary run IDs, including queue acks, waitpoint completion, and batch scheduling. This is a new internal attack surface that should be restricted to the run attempt and TTL systems.
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| raw: `Run expired because the TTL (${run.ttl}) was reached`, | ||
| }; | ||
|
|
||
| await this.#scheduleFinalizationGuard(runId); |
There was a problem hiding this comment.
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.
1 participant
Summary
A run's finish commit and its follow-up side effects (completing the associated waitpoint, waking blocked parents, releasing the queue slot, nudging batch completion) are separate writes across Postgres and Redis. If a database error landed between them, the child run was already finished, so the runner's retries hit the "Run is already finished" guard and the completion signal was lost for good. A parent blocked on
triggerAndWaitorbatchTriggerAndWaitthen stayed waiting forever. TTL expiry had the same shape: its worker retry returned early on a non-pending run, and the batch expiry path swallowed a failed waitpoint-job enqueue.Fix
Every finalizing path (attempt success, permanent failure, cancellation, TTL expiry) now enqueues a durable
ensureRunFinalizedjob before the finish commit, and acks it once the inline side effects all succeed. In steady state the guard never executes; the cost is one Redis enqueue and ack per completion.When the inline path dies in between, the guard fires after a short delay and re-derives everything from current state: it releases the run's queue message and concurrency slot, completes a still-pending associated waitpoint from the run row's output or error, re-runs the blocked-run fan-out (covering a lost unblock enqueue even after the waitpoint committed), and re-schedules the batch completion check. Every leg is idempotent, so racing the inline path is a no-op. The job retries with a capped backoff for roughly five weeks before dead-lettering, so it outlives any database outage while a genuinely poisoned item still becomes visible.
Cancellation gets special handling: CANCELED is the only terminal run status where execution can still be in flight, so the guard only re-delivers for a canceled run once its execution snapshot is FINISHED, re-arming itself until then rather than resuming the parent while the child is still winding down.
A
finalization_rederivationscounter increments whenever the guard actually re-delivers a lost signal; it should stay at zero in a healthy system.Tests cover six shapes: waitpoint completion lost after the finish commit, unblock fan-out lost after the waitpoint completed, a failed guard enqueue failing the completion request with nothing committed, a stale guard held back during an in-flight cancellation, waitpoint completion lost during TTL expiry, and the happy path where the guard is acked and never runs.
Known accepted edge: a guard re-run after a partial inline completion can re-emit a cached-run completion event for the same span; this only happens during failure recovery and is bounded to duplicate trace events.
Source merge-base:
1d55693c0fc76279e7e8275fe41f959b65d5ea99Source head:
933a6a64404f7b9d22a0e5d9e1f133bb304d38e8