Skip to content

fix(waits): repair built-in strategy, fail on exhaustion#541

Open
AdityaAudi wants to merge 1 commit into
aws:mainfrom
AdityaAudi:fix/wait-for-condition-strategy-530
Open

fix(waits): repair built-in strategy, fail on exhaustion#541
AdityaAudi wants to merge 1 commit into
aws:mainfrom
AdityaAudi:fix/wait-for-condition-strategy-530

Conversation

@AdityaAudi

Copy link
Copy Markdown
Contributor

Fixes Issue #530

Summary

The built-in wait_for_condition wait 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

  1. Type mismatch — the built-in strategy can't be used at all.
    create_wait_strategy returned a WaitDecision (attribute should_wait), but wait_for_condition consumes a WaitForConditionDecision (attribute should_continue). Passing the built-in strategy into a WaitForConditionConfig failed on the very first poll:
AttributeError: 'WaitDecision' object has no attribute 'should_continue'

Nothing in the tests or examples routed create_wait_strategy into a config, the shipped example hand-wrote its own strategy, so this stayed latent. The JS and Java SDKs return the correct decision type.

  1. Exhaustion silently succeeds.
    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 throws Error and Java throws WaitForConditionFailedException at 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

  • New regression guard: the built-in strategy wires into a WaitForConditionConfig and returns the consumed type (closes the gap that let defect feat: add initial operations #1 ship).
  • Condition met on the final attempt still succeeds.
  • Both exhaustion surfaces covered:
    • Live raise: strategy raises → durable FAIL checkpoint carrying error_type == "WaitForConditionError"WaitForConditionError propagates.
    • Replay: the FAILED checkpoint short-circuits the next invocation, surfacing as CallableRuntimeError with error_type == "WaitForConditionError" (typed reconstruction on replay lands with Consolidate error handling #120; asserting on error_type keeps the test stable across that change).
  • Full suite: 1381 passed · ruff + format clean · mypy clean.

@zhongkechen zhongkechen left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good call, I dropped the #120 note entirely so it can't go stale; the base class already documents the catch contract.

@AdityaAudi
AdityaAudi force-pushed the fix/wait-for-condition-strategy-530 branch from 2233686 to 7bde6ac Compare July 16, 2026 18:46
@AdityaAudi AdityaAudi changed the title fix: repair built-in wait_for_condition strategy and fail on exhaustion fix(waits): repair built-in strategy, fail on exhaustion Jul 16, 2026
@AdityaAudi
AdityaAudi force-pushed the fix/wait-for-condition-strategy-530 branch from 7bde6ac to b90fffa Compare July 16, 2026 18:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants