feat: Consistently Handle FatalDecisionError by natemort · Pull Request #200 · cadence-workflow/cadence-python-client

natemort · 2026-10-19T17:28:40Z

Workflow code can catch FatalDecisionError and often does so accidentally via except Exception. Consistently fail the decision:

Store the FatalDecisionError on DeterministicEventLoop any time workflow internals would like to fail the decision.
Reraise it to the WorkflowEngine for reporting.

We want to collect and report only the first FatalDecisionError as Workflow code might incidentally cause additional errors once that has happened and they're likely red herrings.

What changed?

Fatal errors consistently fail decision tasks.
Workflow code can't swallow FatalDecisionError

Why?

Correctness issues related to workflows
Prevent noisy red-herring issues.

How did you test it?

Unit and integration tests

Potential risks

Release notes

Documentation Changes

codecov · 2026-10-19T17:32:59Z

Codecov Report

❌ Patch coverage is 83.87097% with 10 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
cadence/testing/_workflow_environment.py	73.33%	2 Missing and 2 partials ⚠️
...nce/_internal/workflow/deterministic_event_loop.py	91.30%	1 Missing and 1 partial ⚠️
.../_internal/workflow/statemachine/nondeterminism.py	50.00%	2 Missing ⚠️
...internal/workflow/statemachine/decision_manager.py	87.50%	1 Missing ⚠️
cadence/_internal/workflow/workflow_engine.py	50.00%	1 Missing ⚠️

Files with missing lines	Coverage Δ
cadence/_internal/workflow/context.py	`94.84% <100.00%> (+0.05%)`	⬆️
cadence/_internal/workflow/versioning.py	`94.28% <100.00%> (ø)`
cadence/worker/_base_task_handler.py	`100.00% <100.00%> (ø)`
cadence/worker/_decision_task_handler.py	`89.05% <100.00%> (+0.08%)`	⬆️
...internal/workflow/statemachine/decision_manager.py	`94.94% <87.50%> (+0.03%)`	⬆️
cadence/_internal/workflow/workflow_engine.py	`91.15% <50.00%> (-1.37%)`	⬇️
...nce/_internal/workflow/deterministic_event_loop.py	`80.35% <91.30%> (+6.46%)`	⬆️
.../_internal/workflow/statemachine/nondeterminism.py	`95.21% <50.00%> (-0.10%)`	⬇️
cadence/testing/_workflow_environment.py	`86.49% <73.33%> (-0.26%)`	⬇️

... and 2 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

shijiesheng · 2026-10-20T17:39:12Z

+    The DeterministicEventLoop stops running workflow code and re-raises error,
+    so the decision task fails even if workflow code catches it.
+    """
+    loop = events._get_running_loop()


nit: use public API asyncio.get_running_loop but need to handle RuntimeError if not found. In this case, maybe instead raise FatalDecision("bug: run fail_decision not in deterministic eventloop") from

We use this API elsewhere, it's apparently intended to be used by event loop implementations:
https://github.com/python/cpython/blob/main/Lib/asyncio/events.py#L711 .

shijiesheng · 2026-10-20T17:47:51Z

+        if self._decision_failure is None:
+            self._decision_failure = error
+
+    def abort_if_decision_failed(self) -> None:


Do we really need this level of fail-fast? User really has to catch BaseException and have high CPU workflow code to break the SDK.

It does make the code a little too complicated. But I'm open to keep it but needs some documentation

Workflow code can catch FatalDecisionError and often does so accidentally via `except Exception`. Consistently fail the decision: - Store the FatalDecisionError on DeterministicEventLoop. Reraise it to the WorkflowEngine for reporting. - Change FatalDecisionError to extend BaseException rather than Exception. This is a more correct base class that users are unlikely to accidentally catch. We want to collect and report only the first FatalDecisionError as Workflow code might incidentally cause additional errors once that has happened and they're likely red herrings.

gitar-bot · 2026-10-27T22:27:39Z

Code Review ✅ Approved 6 closed / 6 findings

🟡 Medium risk · Making FatalDecisionError bypass workflow catches and persistently fail decision tasks could halt affected workflows instead of letting them continue.

Adds consistent handling of FatalDecisionError so workflow code cannot swallow it, with the event loop storing the first fatal error and reraising it to the WorkflowEngine, and the engine destroyed after each decision task. Resolved this session: destroy() now runs task coroutines in the task's contextvars Context, tasks created during destroy() are unwound, the DecisionManager test loop issue and the create_task(eager_start=False) TypeError were fixed, the predicate test now reaches the waiter-poll abort in _run_once, and the engine destroy/finally handling promised in the PR description is now implemented. No open issues remain.

✅ 6 closed

✅ Bug: destroy() runs task coroutines outside the task's contextvars Context

📄 cadence/_internal/workflow/deterministic_event_loop.py:108-112 📄 cadence/_internal/workflow/deterministic_event_loop.py:634-648 📄 cadence/testing/_workflow_environment.py:444-447 📄 cadence/testing/_workflow_environment.py:727-728
destroy() unwinds each task by calling coro.throw(...) on the task's coroutine directly. That bypasses Task.__step, which normally runs the coroutine inside the task's own contextvars.Context, so the coroutine runs in whatever context the caller of destroy() has. Two things break as a result. Finally blocks can't see context vars set inside the task. And any var.reset(token) on a token created inside the task raises ValueError: Token was created in a different Context, which replaces the unwind exception.

This is reachable today in TestWorkflowEnvironment: child workflows run through _run_in_fresh_context (asyncio.create_task(..., context=contextvars.Context())) and wrap the child in _activate_workflow_context(child_ctx) / extract_headers(...). If a child is blocked when env.close() runs, its finally blocks see the parent's WorkflowContext, not the child's. Then WorkflowContext._var.reset(token) raises ValueError, which gets logged as "Workflow code raised while being destroyed". In production the same thing happens to any workflow or interceptor code that calls ContextVar.set/reset inside a task.

Fix: run each throw/close inside the task's context, e.g. task.get_context().run(coro.throw, err) on 3.12+, with a fallback for 3.11 where get_context doesn't exist.

✅ Edge Case: Tasks created by finally blocks during destroy() are never unwound

📄 cadence/_internal/workflow/deterministic_event_loop.py:110-120 📄 cadence/_internal/workflow/deterministic_event_loop.py:206-212
destroy() takes a snapshot, list(self._tasks.values()), before unwinding anything. The loop stays open until the finally, so create_task still works while workflow finally blocks run. A finally block that starts a task (asyncio.create_task(...), or an async with TaskGroup() used for cleanup) registers a new coroutine that isn't in the snapshot. That coroutine is never started or closed, and close() then clears its scheduled __step from _ready. It ends up with the garbage collector and triggers the "coroutine ... was never awaited" RuntimeWarning, which is exactly what this PR is meant to remove.

Fix: keep unwinding until no new tasks show up. For example, loop over _tasks until a pass finds nothing new, or reject create_task once destroy has started.

✅ Cross-PR: #199's DecisionManager tests hit AttributeError on a plain asyncio loop

📄 cadence/_internal/workflow/statemachine/decision_manager.py:132 📄 cadence/_internal/workflow/statemachine/decision_manager.py:371 📄 cadence/_internal/workflow/statemachine/decision_manager.py:607 📄 tests/cadence/_internal/workflow/statemachine/test_decision_manager.py:27
Breaks when this PR and #199 both merge into main, in either order, and the test suite runs #199's tests that build DecisionManager on a stock asyncio loop.

How

This PR makes DecisionManager._add_state_machine and _request_cancel call self._event_loop.raise_if_decision_failed() (cadence/_internal/workflow/statemachine/decision_manager.py:371,607). Only DeterministicEventLoop has that method.

Because of that, this PR rewrote every DecisionManager(asyncio.get_event_loop()) in the tests it touches to DecisionManager(DeterministicEventLoop()) (tests/cadence/_internal/workflow/statemachine/test_decision_manager.py:27).

feat!: Return Futures Instead of Coroutines #199 adds or keeps tests that still pass asyncio.get_event_loop() or asyncio.get_running_loop(): test_decision_manager.py (test_activity_dispatch), test_context_child_workflow.py::_make_ctx, and test_context_signal_child_workflow.py. Those tests then call schedule_activity or schedule_child_workflow, which reaches _add_state_machine.

On a plain asyncio loop, that call raises AttributeError: ... has no attribute 'raise_if_decision_failed', so the tests fail instead of exercising the feature. Production isn't affected, because WorkflowEngine always passes a DeterministicEventLoop.

Fix

This PR: Nothing required. Optionally, guard the call with getattr(self._event_loop, "raise_if_decision_failed", None) so loop-agnostic callers keep working.

feat!: Return Futures Instead of Coroutines #199: Build DecisionManager(DeterministicEventLoop()) in test_decision_manager.py, test_context_child_workflow.py::_make_ctx and test_context_signal_child_workflow.py, and resolve the textual conflict with this PR's rewrite of test_decision_manager.py.

Compared dcf79bb with #199 at c301d52

✅ Edge Case: create_task(eager_start=False) now fails with TypeError

📄 cadence/_internal/workflow/deterministic_event_loop.py:204-210 📄 cadence/_internal/workflow/deterministic_event_loop.py:652-662
create_task only rejects eager_start=True and passes every kwarg on to _DeterministicTask(coro, loop=self, **kwargs). The new _DeterministicTask.__init__ only accepts loop, name and context. So a call like loop.create_task(coro, eager_start=False), which the guard is clearly meant to let through, now fails with TypeError: unexpected keyword argument 'eager_start'. Before this change, tasks.Task(coro, loop=self, **kwargs) accepted it, because asyncio.Task.__init__ takes eager_start in Python 3.12+. Asyncio helpers that forward **kwargs hit the same error, for example asyncio.create_task(..., eager_start=False) and TaskGroup.create_task on Python 3.14. To fix it, remove eager_start from kwargs before building the task, or accept it in _DeterministicTask.__init__ and ignore it.

✅ Quality: Predicate test never reaches the new waiter-poll abort in _run_once

📄 tests/cadence/_internal/workflow/test_deterministic_event_loop.py:237-251 📄 cadence/_internal/workflow/deterministic_event_loop.py:220-225 📄 cadence/_internal/workflow/deterministic_event_loop.py:240-245
test_fail_decision_from_wait_condition_predicate is meant to cover the new early return in _run_once's waiter loop (settled = w.poll(); if self._decision_failure is not None: return, lines 243-245). It doesn't reach it. create_waiter() calls waiter.poll() once inside the wait() task. The predicate calls fail_decision on that very first call, so the waiter settles with the exception and is never added to self._waiters. The loop then stops only because of the existing check after handle._run() (line 234), so the test would still pass if the waiter-loop check were deleted.

To exercise that branch, make the predicate return False on its first call and call fail_decision on a later poll, then assert that the next waiter or ready handle does not run.

...and 1 more closed from earlier reviews

Verification Repos 2 inspected · PRs 3 inspected

Functional validation No results

Rules No rules evaluated

Broader impact Repos 2 inspected PRs 3 inspected

Cross-repo 2 repositories inspected 0 open impacts

cadence-workflow/cadence-samples Inspected
cadence-workflow/ai-skills Inspected

Cross-PR 3 PRs inspected 0 open impacts

PR 196: feat!: Standardize Error Handling and Support Custom Error Reasons Inspected
PR 199: feat!: Return Futures Instead of Coroutines Inspected
PR 191: fix(worker)!: Worker.run() now waits on failures or Worker.close() Inspected

_{For more verification details, comment gitar display:verbose.}

Options

Auto-apply is off → Gitar will not commit updates to this branch.
Display: compact → Showing compact verification results.

Comment with these commands to change the behavior for this request:

`Auto-apply`	`Compact`
`gitar auto-apply:on`	`gitar display:verbose`

_{Was this helpful? React with 👍 / 👎 | Gitar}