fix(server): a failed turn start no longer leaves the thread stuck starting#14424

orchestration-v2/EffectWorker.ts:135

Cause.pretty(cause) puts arbitrary underlying error text into structured log annotations, potentially exposing payloads or credentials; the same applies to failRunCause below. Could you log a bounded category here and retain the original failure only as the error cause?

No longer relevant as of 254e06214a786f8b60afccde5affb896e8804cf7

Fixed in 254e062: both warnings now annotate identifiers only (effectId, effectType, runId) and pass the failure as the log cause instead of Cause.pretty text.

Sorry, I'm unable to act on this request because you do not have permissions within this repository.

orchestration-v2/ProviderTurnStartService.ts:289

missingExecutionState only forwards its argument into a new error, which the error convention disallows. Could you construct ProviderTurnStartError directly at the failure boundary instead? This needs changes at both call sites, so no single-hunk suggestion applies.

No longer relevant as of 254e06214a786f8b60afccde5affb896e8804cf7

Fixed in 254e062: the helper is gone and ProviderTurnStartError is constructed at both failure sites. (It took no argument, but it was still a constructor-only helper.)

🟠 High orchestration-v2/ProviderTurnStartService.ts:243

failStartingRun reports success when the run is already running, so a failed session.startTurn whose terminal-event write also failed is swallowed and leaves the run permanently running with no active provider-event fiber. The failRun path must report whether settleRunBeforeStart committed and propagate the original start error when the run has already advanced, so the worker does not mark the effect complete.

This PR does not change that case. Once the run has left starting, start itself returns early (run.status !== "starting" means the effect is idempotent), so any retry of the effect already completes it in that state. That was true before this PR, and it is still true for retries before the last one. After the run is running, settling it belongs to the run execution path (startRootRun and finalization), not to the turn-start effect. failStartingRun follows the same rule as start on purpose and only settles runs that never reached the provider. A stuck running run after a failed terminal write is worth its own fix in run execution, outside this PR.

Sorry, I'm unable to act on this request because you do not have permissions within this repository.

🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

failStartingRun cannot settle a run whose root node or attempt is missing. The worker then retries the start effect forever.

Line 284 checks for the root node and attempt before the failRun branch. If either record is missing, failStartingRun fails with ProviderTurnStartError. The changed EffectWorker excludes provider-turn.start and provider-turn.restart from the attempt-limit fail branch. So the effect returns to the outbox every 30 seconds without end. The run stays starting, and each retry logs two warnings.

There are two ways to fix this:

• Detect this invariant break in failStartingRun and return a distinct error. The worker can then send that error to outbox.fail.
• Write a run-only terminal event when rootNode or attempt is undefined.

Source: Learnings

✅ Confirmed as addressed by @saphid

Fixed in a81cbcfff9 using your first option. When the root node or attempt is missing, failStartingRun now returns a distinct ProviderTurnStartRunStateMissingError. The executor passes that error through, and the worker sends it to outbox.fail instead of retrying past the budget. Added tests at the service, executor and worker level.

@saphid Thanks for fixing this in a81cbcfff9. The distinct ProviderTurnStartRunStateMissingError and the reported outbox.fail handling address the unbounded retry concern. The service, executor, and worker tests cover the error path across all three layers.

If you found this review helpful, would you consider giving us a shout-out on X?

Thank you for using CodeRabbit!

✅ Review thread resolved.

You are interacting with an AI system.

🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift

Do not fail the restart run while its interrupted provider turn can remain active.

provider-turn.restart uses the same targetRun.id and active attempt as the interrupted provider turn. interruptTurn does not guarantee terminalization, and interruptAndAwaitTerminal can fail while that turn and attempt still report "running". The final-attempt handler then calls failStartingRun, which marks the run and attempt as failed but emits no provider-turn.updated event. This can leave a failed run with its original provider turn still active, so provider work continues after the run reports failure and the existing terminalization path can no longer process it.

Keep retrying this interruption failure, or terminalize the interrupted provider turn before committing failStartingRun.

✅ Addressed in commits 570fc06 to 38ce455

Confirmed and fixed in 570fc06535 (plus 38ce45516c). The restart creates a new attempt and supersedes the old one, but the interrupted provider turn stays running until the provider reports it ended. If interruptAndAwaitTerminal fails through the last attempt, failStartingRun failed the run and left that turn running. ProviderRuntimeRecoveryService only cancels turns of active runs, so nothing would ever end it.

failStartingRun now marks this run's pending or running provider turns interrupted in the same guarded writeIfRunCurrent write that fails the run, so either everything settles or nothing does. That matches what Stop does when no terminal event arrives. Each event is recorded under its turn's own root node. A new test in ProviderTurnStartService.test.ts failed on 1b6fc066ee (the turn stayed running) and passes now.

I chose this over retrying the interrupt forever because some interrupt failures are permanent (for example "recorded provider execution target is no longer valid"). One gap remains: a provider that really is still alive can report running again later. Provider-turn status isn't monotonic anywhere in the projection, so Stop and recovery have the same exposure.

@saphid Thanks for the clarification. I verified the fix in the current code.

failStartingRun selects this run’s pending or running provider turns and marks them interrupted in the same guarded writeIfRunCurrent batch that fails the run. Each provider-turn event uses that turn’s own nodeId. The regression test checks the interrupted status, completion timestamp, and earlier root node. I inspected the test but did not run it.

My original description was incorrect about attempt identity: restart creates a new attempt and supersedes the old attempt.

The later running update is a separate projection-monotonicity concern shared with Stop and recovery. It does not block this fix. Would you like a follow-up issue for that gap?

If you found this review helpful, would you consider giving us a shout-out on X?

Thank you for using CodeRabbit!

✅ Review thread resolved.

You are interacting with an AI system.

External image Major · Stability & Availability · Quick win

Pass the failed attempt identity to failStartingRun.

If the old attempt's guarded settlement fails with a persistence error, the worker retries the start effect and eventually calls failStartingRun with only threadId and runId. If steering commits a replacement before that fallback reads the run, failStartingRun uses the replacement's activeAttemptId and can mark the replacement run, attempt, root node, and error item as failed. Require the failed attempt to remain current before writing the fallback settlement.