Repository navigation
Conversation
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
f637835 to
0ab747f
Compare
0ab747f to
442fe72
Compare
WorkflowCode can catch FatalDecisionError and often does so accidentally via `except Exception`. Consistently fail the decision: - Store the FatalDecisionError on DeterministicEventLoop and stop processing coroutines once it has failed. 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 WorkflowCode might incidentally cause additional errors once that has happened and they're likely red herrings. Once we've completed a decision, successfully or otherwise, we currently discard the WorkflowEngine and let the garbage collector clean it up. This is problematic for a number of reasons: - Python logs warnings about these unawaited coroutines. - As part of garbage collection Python `cancel`s them, which executes the user code outside of the WorkflowContext and event loop. - We want to consistently run `finally` blocks, even when destroying an incomplete Workflow. This sounds strange, but: - In Go we use goexit which executes `defer` blocks. - In Java we throw DestroyWorkflowThreadError which executes `finally` blocks. As we provide the event loop we're responsible for running their coroutines to completion. They might have an `await` within a `finally` block, especially if using an async context manager, which means we need to repeatedly run and interrupt it. We don't know how many times their coroutine will do this, or if it's doing it infinitely, so we try at most 100 times before giving up on a coroutine and leaving it for the garbage collector. Destroy the WorkflowEngine after each DecisionTask.
442fe72 to
52ac5aa
Compare
Code Review ✅ Approved 5 closed / 5 findings🟡 Medium risk · Changing fatal-decision propagation and destroying workflow coroutines after each task could fail decisions or run user finally blocks unexpectedly. Makes FatalDecisionError consistently fail decision tasks by storing the first occurrence on DeterministicEventLoop, halting coroutine processing, and changing its base class to BaseException so workflow code cannot accidentally swallow it. Also destroys the WorkflowEngine after each DecisionTask, running pending finally blocks with a bounded retry limit. Resolved issues include contextvars handling during destroy(), unwinding of tasks created in finally blocks, DecisionManager test compatibility with plain asyncio loops, eager_start TypeError handling, and waiter-poll abort coverage in _run_once. No issues found. ✅ 5 closed✅ Bug: destroy() runs task coroutines outside the task's contextvars Context
✅ Edge Case: Tasks created by finally blocks during destroy() are never unwound
✅ Cross-PR: #199's DecisionManager tests hit AttributeError on a plain asyncio loop
✅ Edge Case: create_task(eager_start=False) now fails with TypeError
✅ Quality: Predicate test never reaches the new waiter-poll abort in _run_once
Verification Repos 1 inspected · PRs 4 inspectedFunctional validation No results Rules No rules evaluated Broader impact Repos 1 inspected PRs 4 inspectedCross-repo 1 repository inspected 0 open impacts
Cross-PR 4 PRs inspected 0 open impacts
For more verification details, comment OptionsAuto-apply is off → Gitar will not commit updates to this branch. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Gitar |
This branch has not been deployed
1 participant
Workflow code can catch FatalDecisionError and often does so accidentally via
except Exception. Consistently fail the decision: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.
Once we've completed a decision, successfully or otherwise, we currently discard the WorkflowEngine and let the garbage collector clean it up. This is problematic for a number of reasons:
cancels them, which executes the user code outside of the WorkflowContext and event loop.finallyblocks, even when destroying an incomplete Workflow. This sounds strange, but:deferblocks.finallyblocks.As we provide the event loop we're responsible for running their coroutines to completion. They might have an
awaitwithin afinallyblock, especially if using an async context manager, which means we need to repeatedly run and interrupt it. We don't know how many times their coroutine will do this, or if it's doing it infinitely, so we try at most 100 times before giving up on a coroutine and leaving it for the garbage collector.Destroy the WorkflowEngine after each DecisionTask.
What changed?
Why?
How did you test it?
Potential risks
Release notes
Documentation Changes