feat(engine): scoped compensation with per-branch sub-stacks#17
Open
albertgwo wants to merge 2 commits intofeat/engine-pr4-parallel-isolationfrom
Open
feat(engine): scoped compensation with per-branch sub-stacks#17albertgwo wants to merge 2 commits intofeat/engine-pr4-parallel-isolationfrom
albertgwo wants to merge 2 commits intofeat/engine-pr4-parallel-isolationfrom
Conversation
Add per-branch compensation sub-stacks for parallel execution. Each walkBranch now tracks its own CompensationEntry[] and returns it as part of BranchResult. On parallel branch failure, only completed branches' sub-stacks are unwound in reverse order (best-effort). Introduce ExecutionError class thrown by CompiledFlow.execute() that surfaces trace, failedNode, compensated, and compensationErrors for structured error handling by consumers. Export runCompensationStack, CompensationResult, BranchResult, and ExecutionError from the public API.
Test coverage for: - Sequential LIFO compensation order (A -> B -> C fails -> comp B -> comp A) - Best-effort: compensation continues after handler failure - runCompensationStack returns compensated/compensationErrors correctly - Parallel branch compensation: only completed branches are compensated - Downstream failure after parallel: all branch stacks compensated - ExecutionError shape: trace, failedNode, compensated, compensationErrors - walkBranch returns isolated CompensationEntry[] per branch - Edge cases: empty stack, non-Error throws
![baz-reviewer[bot]](https://avatars.githubusercontent.com/in/933528?s=60&v=4){kind=small}
Comment on lines
1
to
+3
| export { FlowprintEngine } from './engine.js' | ||
| export { CompiledFlow } from './compiled-flow.js' | ||
| export { ExecutionError } from './errors.js' |
There was a problem hiding this comment.
Adding ExecutionError to the engine exports is package-impacting — should we add a .changeset/*.md summarizing the new export and bump the package version?
Finding type: AI Coding Guidelines | Severity: 🟠 Medium
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents:
In packages/engine/src/engine/index.ts around lines 1-3 the new line `export {
ExecutionError } from './errors.js'` introduces a package-impacting API change by
exposing ExecutionError publicly. Add a .changeset/*.md file that summarizes the change
(describe that ExecutionError is now exported and any related compensation/behavior
details), follow the repository changeset format, and include the appropriate bump level
(patch/minor/major) per the change. Also ensure the top-level re-export in
packages/engine/src/index.ts and the new class implementation in
packages/engine/src/engine/errors.ts are referenced in the changeset, and update the
package version/release metadata according to the changeset guidelines (CLAUDE.md lines
89-91).
Heads up!
Your free trial ends tomorrow.
To keep getting your PRs reviewed by Baz, update your team's subscription
Comment on lines
+372
to
+375
| const resultWithMeta = Object.assign(parallelResult, { | ||
| __branchCompensationStacks: completedBranchStacks, | ||
| }) | ||
|
|
There was a problem hiding this comment.
onParallel returns __branchCompensationStacks into the merged state, leaking non-serializable handler functions into ExecutionResult.output — should we keep those stacks out of the returned object and store them separately?
Finding type: Breaking Changes | Severity: 🔴 High
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents:
In packages/engine/src/engine/compiled-flow.ts around lines 372 to 375, the onParallel
logic mutates the parallelResult with __branchCompensationStacks and returns it, which
causes internal, non-serializable compensation entries to leak into
ExecutionResult.output. Remove the Object.assign that attaches
__branchCompensationStacks to the returned result and instead keep completedBranchStacks
in an internal, non-returned structure (for example, a Map keyed by the parallel nodeId
or a closure-level array passed into buildCallbacks) that is never merged into
ctx.state. Update the error/compensation handling to read from that internal map when
running or reporting compensations, and ensure the final returned parallelResult
contains only the branch state (no __branchCompensationStacks) so public outputs remain
JSON-serializable.
Heads up!
Your free trial ends tomorrow.
To keep getting your PRs reviewed by Baz, update your team's subscription
Comment on lines
+492
to
+503
| onCompensation: ( | ||
| nodeId: string, | ||
| compensation: { file: string; symbol: string }, | ||
| _result: unknown, | ||
| ): (() => Promise<void>) => { | ||
| // Return a compensation handler. The actual implementation would load | ||
| // the compensation entry point, but for now we use a basic handler | ||
| // that the adapter can override. | ||
| return async () => { | ||
| const handler = resolvedHandlers.get(nodeId) | ||
| if (handler && (handler.type === 'entry_point' || handler.type === 'registered')) { | ||
| // Re-invoke the handler as a compensation (in a real system, |
There was a problem hiding this comment.
Should we load and execute the configured compensation entry or delegate to the adapter instead of returning a no-op from onCompensation, which currently marks compensations done without running them?
Finding type: Logical Bugs | Severity: 🔴 High
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents:
In packages/engine/src/engine/compiled-flow.ts around lines 492 to 511, the
onCompensation implementation returns a placeholder async function that never executes
the configured compensation entry point. Change it so the returned async handler
actually invokes the configured compensation: first attempt to delegate to the adapter
by calling adapter.executeCompensation(nodeId, compensation, ctx) if that method exists;
if not, dynamic-import the module at compensation.file and call the exported function
named by compensation.symbol with a legacy context (use buildLegacyContext) or other
appropriate args. Ensure you call hooks?.onNodeStart and hooks?.onNodeComplete (or
onCompensationStep) to record compensation start/completion, handle and surface errors
(rethrow after recording), and preserve the existing no-op behavior only if no
compensation can be resolved. This will ensure runCompensationStack invokes real
compensation logic instead of a no-op.
Heads up!
Your free trial ends tomorrow.
To keep getting your PRs reviewed by Baz, update your team's subscription
Deploying flowprint with
|
| Latest commit: |
b16622d
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://993bdb6d.flowprint.pages.dev |
| Branch Preview URL: | https://feat-engine-pr5-scoped-compe.flowprint.pages.dev |
Contributor
Author
Code Review FindingsCritical (90-100 confidence)1. Branch compensation stacks never promoted to parent on success (95)
2.
Important (80-89 confidence)3.
4.
5.
6. Test gap: no test for downstream-of-parallel failure expecting branch compensations (80)
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
User description
Summary
Dependency
Base: `feat/engine-pr4-parallel-isolation`
PR 9 of 16 in the execution engine implementation.
Test plan
Generated description
Below is a concise technical summary of the changes proposed in this PR:
Enable scoped compensation handling throughout the execution engine by teaching
walkBranchandwalkGraphto surface per-branch stacks, propagating them throughCompiledFlow’s parallel callback, and ensuring failures unwind only finished branches while attaching metadata so upstream code obtains the trace, failed node, and compensation outcomes.ExecutionErrorfromCompiledFlow.executeso callers receive a structured failure containing the trace, failed node, and compensation successes/errors, wiring the new error class and exports through the engine surface.Modified files (4)
Latest Contributors(1)
ExecutionErrorfor both parallel and sequential failure scenarios.Modified files (1)
Latest Contributors(0)
walkBranchreturn its own stack,walkGraphcapture completed branch stacks and promote them viarunCompensationStack, andCompiledFlowmerge the promoted data while coordinating compensation unwinds with adapters.Modified files (4)
Latest Contributors(1)