fix: round-trip serdes result on first run across ops - #550
Conversation
6a28b7a to
e1293c7
Compare
ayushiahjolia
changed the title
315a90c to
cd7a3c4
Compare
b4f4ddf to
3d1eef0
Compare
GitHub Actions
Failure
GitHub Actions
Failure
249c4e7 to
6a57868
Compare
GitHub Actions
Failure
GitHub Actions
Inactive
This comment has been minimized.
This comment has been minimized.
6a57868 to
7db4a57
Compare
GitHub Actions
Inactive
GitHub Actions
Failure
This comment has been minimized.
This comment has been minimized.
7db4a57 to
782f6db
Compare
GitHub Actions
Inactive
GitHub Actions
Failure
GitHub Actions
Failure
GitHub Actions
Inactive
GitHub Actions
Error
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
This comment has been minimized.
This comment has been minimized.
a3b48c2 to
4ffa22a
Compare
GitHub Actions
Error
4ffa22a to
fd028a9
Compare
GitHub Actions
Error
fd028a9 to
d9b1ff3
Compare
GitHub Actions
Error
d9b1ff3 to
f13ccb6
Compare
GitHub Actions
Error
f13ccb6 to
d23f3f7
Compare
GitHub Actions
Inactive
GitHub Actions
Error
This comment has been minimized.
This comment has been minimized.
| # For errors that carry wire fields (DurableOperationError, SerDesError) | ||
| # also preserve their data and stack_trace so the inner info survives | ||
| # across a single operation boundary. | ||
| wire_type: str = _qualified_error_type(exception) |
There was a problem hiding this comment.
Codex AI review
[P1] Preserve the existing ErrorType wire contract. This changes every non-builtin error from names such as StepError or MyError to a module-qualified name. Besides breaking consumers of invocation results, existing checkpoints no longer match the new fully-qualified-only reconstruction registry. For example, a previously checkpointed wait-for-callback submitter StepError reconstructs as plain DurableOperationError, so it is no longer translated to CallbackSubmitterError. Keep the existing class-name encoding for ordinary errors, or version the new encoding while accepting both legacy and qualified registry keys, with an upgrade replay test.
There was a problem hiding this comment.
This ships in v2 which is a breaking release. There are no v1 checkpoints to maintain backward compatibility with. The qualified wire format is the v2 contract going forward.
| except RetryableSerDesError: | ||
| # Transient serdes failure: fail the invocation for backend retry, | ||
| # bypassing the step retry strategy. This narrow catch relies on the | ||
| # serdes wrappers raising only RetryableSerDesError or SerDesError; | ||
| # any other retryable InvocationError would fall through to the step | ||
| # retry strategy below. | ||
| raise |
There was a problem hiding this comment.
Codex AI review
[P1] Handle retryable serdes failures for AT_MOST_ONCE_PER_RETRY. Such a step has already persisted START before this rethrow. On the backend retry, check_result_status() treats that checkpoint as an interrupted step and invokes the step retry strategy, potentially exhausting it and permanently failing instead of retrying the serdes at invocation level. Persist a resumable serialized payload before propagating retryable deserialization failures, and explicitly handle retryable serialization failures according to at-most-once semantics. Add coverage for both failure phases with AT_MOST_ONCE_PER_RETRY.
| ) | ||
| # A serdes failure surfaces as SerDesError regardless of the operation | ||
| # kind, so it is catchable as itself on both first run and replay. | ||
| if self.type == f"{SerDesError.__module__}.{SerDesError.__qualname__}": |
There was a problem hiding this comment.
Codex AI review
[P2] Recognize subclasses of SerDesError. The serdes wrappers deliberately propagate subclasses via isinstance, but their checkpointed type is the subclass's qualified name, so this exact string comparison misses it. A custom MySerDesError(SerDesError) is consequently surfaced as StepError/ChildContextError and cannot be caught as SerDesError. Normalize all SerDesError instances to a stable serdes discriminator when creating the ErrorObject, or persist a separate serdes category, and test subclass behavior on first run and replay.
| ) | ||
| new_state = wrapped_user_func(current_state, check_context) | ||
|
|
||
| serialized_state = self._serialize(new_state) |
There was a problem hiding this comment.
Codex AI review
[P2] Checkpoint the state after the wait strategy runs. Previously serialization followed wait_strategy, so deterministic mutations it made to mutable state were persisted and returned. Capturing serialized_state here discards those mutations for both retry and success; a strategy that advances polling metadata can therefore see the same state forever. Run the strategy on round_tripped_state, then serialize and validate that post-strategy state for the checkpoint and return value.
There was a problem hiding this comment.
The wait strategy doesn't produce or modify state - it only returns a continue/stop decision.
Codex AI reviewFour correctness and compatibility issues found. Static review only; repository code was not executed as requested. Reviewed commit |
3 participants
Issue #, if available:
#406
#544
Description of changes:
When a step, child context, or wait_for_condition finishes, we now serialize its result and deserialize it back before saving the SUCCEED checkpoint. The value the function returns on the first run is the deserialized one - exactly what it would return on replay.
Before this, the first run returned the raw in-memory result, but replay returned the value rebuilt from the checkpoint. With a custom serdes that changes the value in transit, those two could differ. Running the round-trip up front makes the first run and replay always agree, and guarantees a SUCCEEDED result is always reconstructable.
Serdes failures are now clearly split:
Behavior changes to note
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.