fix: round-trip serdes result on first run across ops#550
fix: round-trip serdes result on first run across ops#550ayushiahjolia wants to merge 2 commits into
Conversation
6a28b7a to
e1293c7
Compare
ayushiahjolia
changed the title
| # returned as-is. | ||
| if serialized_state is None: | ||
| return None # type: ignore[return-value] | ||
| return deserialize( |
There was a problem hiding this comment.
I think deserialize should happen before we checkpoint SUCCEEDED. If deserialize throws, we checkpointed SUCCEEDED and then checkpoint another FAILED for the same operation. Same issue in child operation.
There was a problem hiding this comment.
+1
just thinking through the various points this applies:
wait_for_condition:
serialize -> deserialize -> wait_strategy(deserialized_state, attempt) -> checkpoint SUCCEED -> return deserialized_state.
This fixes the double-checkpoint and also matches JS, where the wait strategy already sees the round-tripped state.
Note the wait_for_condition reordering fixes a second, separate divergence for free: today Python passes the raw state to wait_strategy while JS passes the round-tripped state, so with a non-identity serdes the same strategy can decide differently across SDKs. Deserializing before the wait_strategy call closes that gap too.
child:
deserialize before the SUCCEED checkpoint, in all three modes. Note this intentionally diverges from JS ordering (JS checkpoints first). JS is only safe there because safeDeserialize terminates the invocation instead of throwing, so it never writes a FAIL after a SUCCEED.
Deserialize-before-checkpoint is prob the correct Python equivalent, and matches how serialize failures already behave (FAIL, no SUCCEED)?
step
same pattern exists there from fix(step): Return round-tripped result on first run #543 (deserialize after SUCCEED, inside the try that routes to the retry handler)
| # returned as-is. | ||
| if serialized_state is None: | ||
| return None # type: ignore[return-value] | ||
| return deserialize( |
There was a problem hiding this comment.
+1
just thinking through the various points this applies:
wait_for_condition:
serialize -> deserialize -> wait_strategy(deserialized_state, attempt) -> checkpoint SUCCEED -> return deserialized_state.
This fixes the double-checkpoint and also matches JS, where the wait strategy already sees the round-tripped state.
Note the wait_for_condition reordering fixes a second, separate divergence for free: today Python passes the raw state to wait_strategy while JS passes the round-tripped state, so with a non-identity serdes the same strategy can decide differently across SDKs. Deserializing before the wait_strategy call closes that gap too.
child:
deserialize before the SUCCEED checkpoint, in all three modes. Note this intentionally diverges from JS ordering (JS checkpoints first). JS is only safe there because safeDeserialize terminates the invocation instead of throwing, so it never writes a FAIL after a SUCCEED.
Deserialize-before-checkpoint is prob the correct Python equivalent, and matches how serialize failures already behave (FAIL, no SUCCEED)?
step
same pattern exists there from fix(step): Return round-tripped result on first run #543 (deserialize after SUCCEED, inside the try that routes to the retry handler)
| def create_wait_for_condition_retry( | ||
| cls, | ||
| identifier: OperationIdentifier, | ||
| payload: str, |
There was a problem hiding this comment.
shouldn't this also be str | None like the others are now?
| # Ready to execute (checkpoint exists or was just created) | ||
| return CheckResult.create_is_ready_to_execute(checkpointed_result) | ||
|
|
||
| def _deserialize_payload(self, serialized: str | None) -> T: |
There was a problem hiding this comment.
nice! keeping all three child return sites identical :-)
3 participants
Issue #, if available:
#406
#544
Description of changes:
Extends the step operation's first-run behavior (#543) to the remaining operations: on the first run, an operation now returns the serdes round-tripped value
deserialize(serialize(result))- instead of the raw in-memory result. This is the same value each operation reconstructson replay, so results stay identical across the first run and every replay when a non-identity custom SerDes is configured.
Covered operations:
wait_for_condition: returns the round-tripped state on success.run_in_child_context: round-trips in every mode - normal, virtual, and large-payload (ReplayChildren). Large payloads still checkpoint only a compact summary; the full result is round-tripped transiently for the return value. This also flows throughmap/parallel, whose items and BatchResult go through the child handler.invokeandcallbackneed no change (invoke always deserializes from the checkpoint; callback returns only an id).Testing:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.