fix(waits): repair built-in strategy, fail on exhaustion#541
Conversation
zhongkechen
left a comment
There was a problem hiding this comment.
Thank you for contributing this fix and for the thorough regression coverage. The implementation looks good.
Before merge, please fix the commit-lint error: the current subject exceeds the repository limit of 50 characters (invalid subject (must be <=50 chars)). Please shorten the PR title/commit subject so the lint-commits check passes.
| condition is met, matching the exhaustion semantics of the JS and Java SDKs. | ||
|
|
||
| #120 will reparent this under a shared operation-error base, so catch on the | ||
| concrete type rather than the parent that's about to move. |
There was a problem hiding this comment.
note (none-blocking): reparenting under an operation-error base won't affect anyone catching DurableExecutionsError (the new base will still sit under it).
Today: WaitForConditionError → DurableExecutionsError
After #120: WaitForConditionError → OperationError → DurableExecutionsError
for future, we have to update this note when #120 lands.
There was a problem hiding this comment.
Good call, I dropped the #120 note entirely so it can't go stale; the base class already documents the catch contract.
2233686 to
7bde6ac
Compare
7bde6ac to
b90fffa
Compare
Fixes Issue #530
Summary
The built-in
wait_for_conditionwait strategy has never been usable, and the bug hiding behind it would silently corrupt results once the surface error was fixed. This fixes both, together.Root cause
create_wait_strategyreturned aWaitDecision(attributeshould_wait), butwait_for_conditionconsumes aWaitForConditionDecision(attributeshould_continue). Passing the built-in strategy into aWaitForConditionConfigfailed on the very first poll:Nothing in the tests or examples routed
create_wait_strategyinto a config, the shipped example hand-wrote its own strategy, so this stayed latent. The JS and Java SDKs return the correct decision type.When polling attempts run out, the strategy returned a "stop" decision. Once defect feat: add initial operations #1 is fixed, that maps to
should_continue=False, which the executor treats as success, completing with whatever partial state it happened to hold. JS throwsErrorand Java throwsWaitForConditionFailedExceptionat this point. Fixing only the type would have converted today's loud crash into a silent wrong result, which is why both are addressed here.Breaking change
Executions that previously exhausted their polling attempts and completed "successfully" with partial state now fail with
WaitForConditionError. This is the correct behavior and matches the other SDKs. Landing in v2; to be documented in the migration guide.Testing
WaitForConditionConfigand returns the consumed type (closes the gap that let defect feat: add initial operations #1 ship).FAILcheckpoint carryingerror_type == "WaitForConditionError"→WaitForConditionErrorpropagates.FAILEDcheckpoint short-circuits the next invocation, surfacing asCallableRuntimeErrorwitherror_type == "WaitForConditionError"(typed reconstruction on replay lands with Consolidate error handling #120; asserting onerror_typekeeps the test stable across that change).ruff+ format clean ·mypyclean.