Conversation
🦋 Changeset detectedLatest commit: 2d4b3f0 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (5)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughThe workflow registry now stores resume data and checkpoint details before resumed execution. Restart restores a valid pending resume checkpoint. Each step receives a stable execution ID, and completion of the resumed step persists a checkpoint and clears the pending resume metadata. ChangesWorkflow Resume Recovery
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix · Severity of issue fixed: Medium Sequence Diagram(s)sequenceDiagram
participant Operator
participant WorkflowRegistry
participant WorkflowState
participant Workflow
participant restartExecution
participant Step
Operator->>WorkflowRegistry: Submit resumeData
WorkflowRegistry->>WorkflowState: Store resume checkpoint metadata
Operator->>Workflow: Restart execution
Workflow->>restartExecution: Call restartExecution
restartExecution->>WorkflowState: Read pending resume checkpoint
restartExecution->>Step: Execute with resumeData and stepExecutionId
Step-->>restartExecution: Complete resumed step
restartExecution->>WorkflowState: Persist checkpoint and clear resume metadata
Merge Risk: ⚪ Minimal · up to No established issue blocks merging. Workflows configured without running checkpoints may replay unpersisted work after a crash, as expected for that setting. Security Architecture ReviewSecurity architecture risk: 🟡 Moderate · up to Normal interrupted recovery now preserves approval input and reuses the step identity. However, a new replay can inherit an earlier execution’s pending approval and reuse it after interruption. Because the replay has a different execution identity, the original idempotency key would not contain repeated side effects. Retained concerns
Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 4 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
3 issues found across 5 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/core/src/workflow/registry.ts">
<violation number="1" location="packages/core/src/workflow/registry.ts:254">
P2: This checkpoint is written while the execution is still `suspended`; a crash before `workflow.run()` changes it to `running` leaves `restart()` rejecting the saved approval. Set `status: "running"` in this update so the checkpoint is restartable.</violation>
</file>
<file name="packages/core/src/workflow/core.ts">
<violation number="1" location="packages/core/src/workflow/core.ts:1492">
P2: When the resumed step falls outside `checkpointInterval`, this forced save replaces the stored event history with the new `executeInternal`'s events, dropping the original run and suspension events. Merge the existing event history with the resumed events before persisting this checkpoint.</violation>
<violation number="2" location="packages/core/src/workflow/core.ts:1520">
P2: The resume checkpoint is cleared only inside `persistRunningCheckpoint`, which early-returns when `disableCheckpointing` is set. With `disableCheckpointing: true`, the completed step's `VOLTAGENT_RESUME_CHECKPOINT_KEY` is never deleted, so it stays in the persisted metadata. A later crash and `restart()` will then see a pending resume checkpoint whose step has already completed and replay that step with the stale `resumeData` (re-applying the same approval / duplicate side effects); the stale key also lingers on the completed execution state. Clear the one-shot resume checkpoint on resumed-step completion independently of the checkpoint-interval/disableCheckpointing logic.</violation>
</file>
Reply to a comment to ask cubic a question or push back. It learns from your replies.
Re-trigger cubic
| // process dies while that step is running, restart() can replay it with | ||
| // the same resume data instead of presenting the approval again. | ||
| await registeredWorkflow.workflow.memory.updateWorkflowState(executionId, { | ||
| metadata: { |
There was a problem hiding this comment.
P2: This checkpoint is written while the execution is still suspended; a crash before workflow.run() changes it to running leaves restart() rejecting the saved approval. Set status: "running" in this update so the checkpoint is restartable.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At packages/core/src/workflow/registry.ts, line 254:
<comment>This checkpoint is written while the execution is still `suspended`; a crash before `workflow.run()` changes it to `running` leaves `restart()` rejecting the saved approval. Set `status: "running"` in this update so the checkpoint is restartable.</comment>
<file context>
@@ -246,6 +246,22 @@ export class WorkflowRegistry extends SimpleEventEmitter {
+ // process dies while that step is running, restart() can replay it with
+ // the same resume data instead of presenting the approval again.
+ await registeredWorkflow.workflow.memory.updateWorkflowState(executionId, {
+ metadata: {
+ ...workflowState.metadata,
+ [VOLTAGENT_RESUME_CHECKPOINT_KEY]: {
</file context>
| metadata: { | |
| status: "running", | |
| metadata: { |
| ...(stateManager.state?.usage ? { usage: stateManager.state.usage } : {}), | ||
| [VOLTAGENT_RESTART_CHECKPOINT_KEY]: restartCheckpoint, | ||
| }); | ||
| if (isCompletingResumedStep) { |
There was a problem hiding this comment.
P2: The resume checkpoint is cleared only inside persistRunningCheckpoint, which early-returns when disableCheckpointing is set. With disableCheckpointing: true, the completed step's VOLTAGENT_RESUME_CHECKPOINT_KEY is never deleted, so it stays in the persisted metadata. A later crash and restart() will then see a pending resume checkpoint whose step has already completed and replay that step with the stale resumeData (re-applying the same approval / duplicate side effects); the stale key also lingers on the completed execution state. Clear the one-shot resume checkpoint on resumed-step completion independently of the checkpoint-interval/disableCheckpointing logic.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At packages/core/src/workflow/core.ts, line 1520:
<comment>The resume checkpoint is cleared only inside `persistRunningCheckpoint`, which early-returns when `disableCheckpointing` is set. With `disableCheckpointing: true`, the completed step's `VOLTAGENT_RESUME_CHECKPOINT_KEY` is never deleted, so it stays in the persisted metadata. A later crash and `restart()` will then see a pending resume checkpoint whose step has already completed and replay that step with the stale `resumeData` (re-applying the same approval / duplicate side effects); the stale key also lingers on the completed execution state. Clear the one-shot resume checkpoint on resumed-step completion independently of the checkpoint-interval/disableCheckpointing logic.</comment>
<file context>
@@ -1509,15 +1513,20 @@ export function createWorkflow<
+ ...(stateManager.state?.usage ? { usage: stateManager.state.usage } : {}),
+ [VOLTAGENT_RESTART_CHECKPOINT_KEY]: restartCheckpoint,
+ });
+ if (isCompletingResumedStep) {
+ delete checkpointMetadata[VOLTAGENT_RESUME_CHECKPOINT_KEY];
+ }
</file context>
| const isCompletingResumedStep = | ||
| options?.resumeFrom?.resumeData !== undefined && | ||
| lastCompletedStepIndex === options.resumeFrom.resumeStepIndex; | ||
| if ((lastCompletedStepIndex + 1) % checkpointInterval !== 0 && !isCompletingResumedStep) { |
There was a problem hiding this comment.
P2: When the resumed step falls outside checkpointInterval, this forced save replaces the stored event history with the new executeInternal's events, dropping the original run and suspension events. Merge the existing event history with the resumed events before persisting this checkpoint.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At packages/core/src/workflow/core.ts, line 1492:
<comment>When the resumed step falls outside `checkpointInterval`, this forced save replaces the stored event history with the new `executeInternal`'s events, dropping the original run and suspension events. Merge the existing event history with the resumed events before persisting this checkpoint.</comment>
<file context>
@@ -1485,7 +1486,10 @@ export function createWorkflow<
+ const isCompletingResumedStep =
+ options?.resumeFrom?.resumeData !== undefined &&
+ lastCompletedStepIndex === options.resumeFrom.resumeStepIndex;
+ if ((lastCompletedStepIndex + 1) % checkpointInterval !== 0 && !isCompletingResumedStep) {
return;
}
</file context>
PR Checklist
What is the current behavior?
If a workflow process crashes after an approval is accepted and a step begins,
workflow.restart(executionId)restores the pre-resume checkpoint. The step loses itsresumeDataand can suspend for the same approval again.What is the new behavior?
Persist the resume data and checkpoint before executing the resumed step, then restore them during restart. Expose a stable
stepExecutionId(executionId:stepId) in the workflow execution context so integrations can use it as an idempotency key for external side effects. Clear the one-shot resume checkpoint after the resumed step is checkpointed.Fixes #1435
Notes for reviewers
Added a regression test covering approval recovery after an interrupted side effect, including reuse of the stable step identity.
Validation:
vitest run --typecheck packages/core/src/workflow/core.spec.ts: 31 tests passed.tsc --noEmit -p packages/core/tsconfig.json: passed.lerna run build --scope @voltagent/core --include-dependencies: passed.@voltagent/coresuite: 1,396 passed, 2 skipped, 1 todo; 5 unrelated workspace sandbox tests failed on Windows withspawn C:Program ENOENT.Summary by cubic
Fixes workflow restart losing approval data after an interrupted approval side effect. When a process crashes after an approval is accepted,
workflow.restart(executionId)now restores the persisted resume data and checkpoint so the step resumes with the same approval instead of suspending again.stepExecutionId(executionId:stepId) in the workflow execution context for use as an idempotency key around external side effects.Written for commit 2d4b3f0. Summary will update on new commits.
Summary by CodeRabbit