[Bug]: Wait for condition create_wait_strategy failed at runtime

[wangyb-A]

Expected Behavior

create_wait_strategy currently fails at runtime with

AttributeError: 'WaitDecision' object has no attribute 'should_continue'

Python: https://github.com/aws/aws-durable-execution-sdk-python/blob/main/packages/aws-durable-execution-sdk-python/src/aws_durable_execution_sdk_python/waits.py#L81

WaitDecision is dead code should be removed

WaitForConditionConfig= parameters for theoperation(wait_strategy,initial_state,serdes).
WaitStrategyConfig= parameters for the exponential-backoff strategy

Actual Behavior

See above

Steps to Reproduce

SDK Version

1

Python Version

3.14

Is this a regression?

No

Last Working Version

No response

Additional Context

No response

[yaythomas]

Defect 1: type mismatch makes the built-in wait strategy unusable

create_wait_strategy returns a function producing WaitDecision (attribute should_wait). wait_for_condition consumes WaitForConditionDecision (attribute should_continue). Wiring the built-in strategy into WaitForConditionConfig fails on the first poll attempt:

AttributeError: 'WaitDecision' object has no attribute 'should_continue'

Confirmed against the released SDK (1.7.0). JS and Java return the correct decision type.

Python has no working built-in wait strategy for wait_for_condition today.

Defect 2: exhaustion semantics diverge from JS and Java

The max-attempts branch in create_wait_strategy (waits.py L81) returns no_wait(), which wait_for_condition treats as success. JS throws Error and Java throws WaitForConditionFailedException at the same point. Once Defect 1 is fixed, this branch becomes reachable and executions that exhaust polling attempts would complete successfully with partial state. Both defects must be fixed together. Fixing only the type mismatch converts today's loud failure into a silent wrong result.

[AdityaAudi]

@wangyb-A @yaythomas I took a look and I can see both defects on the latest main. Wanted to write up what's going on and float an approach before opening a PR.

On the type mismatch: the built-in strategy from create_wait_strategy hands back a WaitDecision, which exposes should_wait. But wait_for_condition is built around WaitForConditionDecision, which exposes should_continue and that's the attribute the executor actually reads. So the moment you plug the built-in strategy into a WaitForConditionConfig, it blows up on the first poll with the AttributeError you mentioned. What tipped me off to how long this has been latent is that the shipped example doesn't use the built-in helper at all, it hand-rolls its own strategy that returns the right type. Nothing in the tests or examples ever routes create_wait_strategy into a config, so there was no path exercising this. In practice the built-in strategy has just been unusable.

On the exhaustion semantics: you're right that these can't be fixed independently. Right now, when max attempts run out, the strategy returns a "no wait" decision. Today that never matters because Defect 1 crashes first. But the moment we fix the type, that same signal maps cleanly onto the executor's "condition met, we're done" path, so the execution would quietly succeed and return whatever partial state it happened to have. That's worse than the crash, because it's silent. JS and Java both throw here, so lining Python up with them means exhaustion needs to become a failure, not a success.

The approach I'd take:

For the type mismatch, I'd have create_wait_strategy speak the same language as the rest of wait_for_condition and return a WaitForConditionDecision directly, "stop polling" when the condition's satisfied, "keep waiting" with a delay otherwise. The old WaitDecision type only shows up in the waits module and its tests, so it can go away (unless it's being held back for a future step-level wait API, if that's the intent, happy to just deprecate it instead).

For exhaustion, rather than inventing new plumbing, I'd lean on what's already there. The executor already wraps the check in a try/except that records a durable failure and re-raises. So if the strategy simply raises when attempts are exhausted, it flows through that existing path and becomes a proper failure, no changes needed on the executor side. The one open question is the exception itself: there's no wait-for-condition failure type in the codebase today, so we'd be introducing one. I have a sensible default in mind for its name and where it sits in the hierarchy, but since this is really about cross-language parity, I'd rather match whatever convention the maintainers want.

One thing worth calling out: this does change behavior. Any execution that currently exhausts its polling and "succeeds" with partial state will start failing instead. That's the correct outcome and it matches the other SDKs, but it's the kind of thing that deserves a line in the release notes rather than a surprise.

If that direction sounds right, especially the exception type question, I'm happy to put up a PR with tests covering both the type fix and the exhaustion behavior.

[yaythomas]

yes, this is headed for v2 release, so it'll come with release notes detailing breaking changes.

@AdityaAudi thanks for the excellent writeup, and thanks so much for offering to do the PR! Greatly appreciate your help!

yes, have create_wait_strategy return WaitForConditionDecision directly, and raise on exhaustion so it flows through the executor's existing failure path.

To answer your open questions:

1. Exception type: please name it WaitForConditionError, parented under DurableExecutionsError. Reason: this slots into a broader error hierarchy rework planned for v2 (Consolidate error handling #120), where it will be reparented under a new operation-error base — the name is the long-term contract, the parent will move later without affecting anyone catching DurableExecutionsError.

2. WaitDecision: delete it outright, no deprecation needed. This change is headed for the v2 release, so we can take the breaking change cleanly rather than carrying a shim. Same goes for the exhaustion behavior change, silent-success-with-partial-state becoming a failure is the correct outcome and matches JS/Java, and it'll get a line in the v2 migration guide. Please write the commit as a breaking change per conventional commits (fix!: or a BREAKING CHANGE: footer).

A few things to fold into the PR:

a) Keep the check order as it is today: condition first, then exhaustion — so a condition met on the final attempt still succeeds. All three SDKs agree on this and the fix shouldn't disturb it.

b) There's a stale reference to WaitDecision in a log message in operation/wait_for_condition.py ("WaitDecision delay_seconds step for id..."). please tidy that while you're in there, if you dont mind.

c) For tests, please cover both surfaces of the exhaustion failure: the live raise (strategy raises → durable FAIL checkpoint → exception propagates) and the replay path (FAILED checkpoint short-circuits on the next invocation). They surface differently and both matter.

Note the two paths surface different types today: the live raise is WaitForConditionError; the replay path surfaces CallableRuntimeError with error_type == "WaitForConditionError" (typed reconstruction on replay arrives with #120, out of scope here). Please assert accordingly, checking error_type on the replay side keeps the test stable across #120.