Skip to content

fix: close FAILED→COMPENSATING race in saga startup#201

Merged
pratyush618 merged 1 commit into
masterfrom
fix/saga-compensation-race
May 27, 2026
Merged

fix: close FAILED→COMPENSATING race in saga startup#201
pratyush618 merged 1 commit into
masterfrom
fix/saga-compensation-race

Conversation

@pratyush618

@pratyush618 pratyush618 commented May 27, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Moves _mark_run_compensating earlier in start_compensation — immediately after the cheap in-memory eligibility check, before the storage-heavy _fetch_compensable_nodes call
  • On Windows with SQLite under contention, the gap between mark_workflow_node_result setting the run to FAILED and the saga transitioning it to COMPENSATING exceeded the 100ms wait() poll interval, causing the poll to see FAILED (terminal) and return early
  • Handles the vacuous-compensation edge case: if no nodes need compensation after early transition, finalizes as COMPENSATED immediately and resolves any pending parent node inline

Fixes CI failure: test_child_saga_propagates_when_parent_node_is_sub_workflow on windows-latest / Python 3.10

Test plan

  • All 3 saga subworkflow tests pass
  • All 98 workflow tests pass
  • Full Python suite: 998 passed, 6 skipped
  • Rust tests pass
  • ruff + mypy clean

Summary by CodeRabbit

  • Bug Fixes

    • Improved workflow compensation handling to ensure proper state transitions and timely detection of child workflow completion during the compensation phase.
  • Performance

    • Optimized compensation logic to reduce latency in workflow execution.

Review Change Stack

Move _mark_run_compensating earlier in start_compensation — right
after the cheap in-memory eligibility check instead of after the
storage-heavy _fetch_compensable_nodes call. On Windows with SQLite
under contention, the storage I/O gap let wait()'s poll see the
transient FAILED state as terminal.
@coderabbitai

coderabbitai Bot commented May 27, 2026

Copy link
Copy Markdown
📝 Walkthrough

Walkthrough

The PR optimizes the saga compensation flow by reordering state transitions in start_compensation to emit the WORKFLOW_COMPENSATING event before fetching compensable nodes, preventing transient state observers from seeing the run as FAILED. It also adds inline detection in _propagate_to_child_saga to resolve parent node outcomes synchronously when child sagas complete immediately.

Changes

Compensation Flow Optimization

Layer / File(s) Summary
Early state transition in compensation start
py_src/taskito/workflows/saga/orchestrator.py
start_compensation marks the run as Compensating and emits WORKFLOW_COMPENSATING before fetching compensable nodes, and finalizes immediately when no compensable nodes or reverse-topological waves exist.
Inline parent node resolution on immediate child completion
py_src/taskito/workflows/saga/orchestrator.py
_propagate_to_child_saga detects when a child saga already finalized (no longer present in waves/inflight) and calls _on_child_saga_terminal inline to resolve the parent node synchronously.

Estimated Code Review Effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Poem

🐰 In compensation's dance, we reorder the beat,
Mark states early so observers greet
The truth before nodes are fetched slow,
And when children finish—resolved inline they go!
Timing and flow, now perfectly neat! 🎭

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title directly and specifically summarizes the main fix: closing a FAILED→COMPENSATING race condition in saga startup by reordering operations.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/saga-compensation-race

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
py_src/taskito/workflows/saga/orchestrator.py (1)

512-521: 💤 Low value

Minor observation on the already_done assumption.

The comment accurately describes the primary case (vacuous compensation), but the check could also trigger if non-vacuous compensation finishes between start_compensation returning and this lock acquisition. In that edge case, passing True would be incorrect if the child saga actually failed.

This race window is extremely narrow (nanoseconds, bounded by GIL), making it practically impossible for asynchronous job execution/completion to occur in that time. The fix correctly addresses the real CI failure and the logic is sound for the documented vacuous case.

No action required—just noting the implicit assumption that already_done=True immediately after start_compensation implies vacuous (always-success) completion.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@py_src/taskito/workflows/saga/orchestrator.py` around lines 512 - 521, The
code assumes already_done implies vacuous (successful) compensation and calls
_on_child_saga_terminal(child_run_id, True, None); instead, avoid passing True
unconditionally—check the child's actual terminal state (e.g., consult a
terminal-status map or failure set such as self._run_results /
self._terminal_status or similar) before deciding the success boolean, and if no
definitive status exists pass None so _on_child_saga_terminal can inspect
authoritative state; update the branch around already_done and calls to
_on_child_saga_terminal (and any helpers that record terminal outcomes) to
derive the success flag from the real child terminal record rather than assuming
success after start_compensation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@py_src/taskito/workflows/saga/orchestrator.py`:
- Around line 512-521: The code assumes already_done implies vacuous
(successful) compensation and calls _on_child_saga_terminal(child_run_id, True,
None); instead, avoid passing True unconditionally—check the child's actual
terminal state (e.g., consult a terminal-status map or failure set such as
self._run_results / self._terminal_status or similar) before deciding the
success boolean, and if no definitive status exists pass None so
_on_child_saga_terminal can inspect authoritative state; update the branch
around already_done and calls to _on_child_saga_terminal (and any helpers that
record terminal outcomes) to derive the success flag from the real child
terminal record rather than assuming success after start_compensation.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: a278b13d-b29b-420e-98b9-9691d1e44549

📥 Commits

Reviewing files that changed from the base of the PR and between 3cafd08 and 0c2ef4c.

📒 Files selected for processing (1)
  • py_src/taskito/workflows/saga/orchestrator.py

@pratyush618
pratyush618 merged commit 514650f into master May 27, 2026
21 checks passed
@pratyush618
pratyush618 deleted the fix/saga-compensation-race branch May 27, 2026 05:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant