Repository navigation
fix: preserve empty callback error history - #870
zhongkechen wants to merge 7 commits into
Conversation
This comment has been minimized.
This comment has been minimized.
zhongkechen
changed the title
Preserve a present empty ErrorObject in callback, step, context, and chained-invoke details by checking presence instead of truthiness. Keep the matching history payload decoder fix and remove the testing library's completion/reconstruction normalization and synthetic payload. Add core decoding and round-trip regressions, and require live callback errors to survive memory and filesystem persistence with the existing default CallbackExternalError message. The focused regressions fail 25 cases on the previous PR revision and pass all 131 with this fix.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
thanks! preserving The preserved empty object is currently treated as if it contains error details. As a result, plugins receive an empty ErrorObject, and step, context, and invoke exceptions have message=None. Passing that empty object to plugins does not match JavaScript. It causes OTel to record an exception with no message or type. Using that empty object to create an exception also gives the exception message=None. Can we keep the current decoder changes, but treat an ErrorObject with no fields like a missing error when building plugin information and exception messages? Concretely, this should only require plugin.py::_extract_error() and state.py::CheckpointedResult.raise_operation_error() to treat not error.to_dict() the same as error is None; the decoder changes can stay unchanged? |
Revert 58567dc to restore the original testing-library solution. Normalize empty callback and reconstructed history errors to absent SDK diagnostics while preserving empty payloads in detailed callback history. Restore the original regression expectations and README. Keep the later main-branch merges and unrelated fixes intact.
zhongkechen
changed the title
Thanks for the clarification. This PR will focus on the OTel behavior fixes: marking failed operations as I’ll retain the existing core SDK error-decoding behavior here and leave changes to those semantics for future PRs. |
Codex AI reviewNo actionable findings across the five changed files or CONTRIBUTING.md rules 1–9. Tests were inspected but not executed. The PR explicitly leaves deployed-service parity for empty callback-failure payloads unverified. Reviewed commit |
| assert reconstructed.callback_details.error == ( | ||
| expected_error if include_data else None | ||
| ) | ||
| assert history.to_dict() == wire |
There was a problem hiding this comment.
wire was built from history.to_dict() a few lines up, so this can't fail. Drop it, or snapshot wire before decoding/reconstructing if the goal is to show those steps don't mutate history.
| assert history.to_dict() == wire | ||
|
|
||
| assert live.callback_details is not None | ||
| assert live.callback_details.error == expected_error |
There was a problem hiding this comment.
Nit: this repeats the loop assertion above (live is already in that tuple).
| expected_wire = history.to_dict() | ||
| if decode_wire: | ||
| history = GetDurableExecutionHistoryResponse.from_dict(expected_wire) | ||
| assert history.to_dict() == expected_wire |
There was a problem hiding this comment.
With decode_wire=False, this compares history.to_dict() with itself, so it can't fail. Same for line 146.
| if decode_wire: | ||
| history = GetDurableExecutionHistoryResponse.from_dict(expected_wire) | ||
| assert history.to_dict() == expected_wire | ||
| expected_error = error if payload else None |
There was a problem hiding this comment.
The previous revision also round-tripped the reconstructed operation through Operation.from_json_dict. That was the only check that a reconstructed op with e.g. {"ErrorMessage": ""} or {"StackTrace": []} survives persistence unchanged. Worth keeping, at least for the non-empty payloads?
| ) | ||
| result = DurableFunctionTestResult.from_execution_history(execution, history) | ||
| result_operation = result.get_operation_by_name("failed-operation") | ||
| assert isinstance(result_operation, (ContextOperation, InvokeOperation)) |
There was a problem hiding this comment.
StepOperation and CallbackOperation both subclass ContextOperation, so this passes even if an event maps to the wrong wrapper. Could we add the expected wrapper class to the parametrize tuple and assert the exact type?
Do we not need the recommended |
Issue #, if available
Fixes #862.
Description of changes
An empty callback failure error was retained in live memory but disappeared after
filesystem serialization, while history parsing dropped a present empty payload.
Normalize empty diagnostics at the actual callback completion boundary, preserve
Error.Payload: {}in detailed callback-failure history, and normalize that emptypayload back to absent diagnostics when reconstructing SDK operation state.
The fix preserves meaningful fields, including explicit empty strings and empty
stack-trace lists. Metadata-only history still omits payloads and uses
Truncated: true; detailed timeout history is unchanged. The handler still seesthe existing
CallbackExternalErrorand its default message when diagnostics areabsent. Callback update IDs, input acknowledgment, replay filtering, and OTel
behavior remain outside this PR.
Design
existing error/state/history APIs.
live memory inconsistent. Normalizing every history payload would lose the
present-empty distinction. Normalize at the callback completion and
history-to-operation boundaries, and keep the empty payload in the detailed
history projection. Reuse one private conversion function for callback, step,
context, and chained-invoke history instead of duplicating its condition.
treated as the same representation; the completion boundary relied on later
serialization to normalize empty errors.
Assumptions
CallbackFailedDetails.Error.Payload: {}forerrorless failure is unverified. This PR implements the local contract in
[Testing]: Preserve empty callback error history without changing runtime errors #862/fix(testing): match replay state to the service #795 and does not claim deployed-service conformance qualification.
CallbackDetails.from_dicttreatsError={}as absent diagnostics. Unitand public-runner tests verify parity before persistence and after memory/
filesystem reload.
Decisions not covered by the issue or the design note
method before the executor persists it, so filesystem decoding cannot hide a
missing boundary normalization.
type is checked on operation details, where the SDK currently preserves it.
execution-history serialization fields are outside this fix.
Validation
d7ae955: 15 failed, 76 passed. The failures include allfour live omitted/empty error cases across memory/filesystem stores; rich-error
controls pass unchanged. With this fix: 91 passed.
decoded history for four operation types, actual public-runner completion,
pre-persistence state, reload, and a saved step executing once across resume.
hatch run dev-testing:typecheck: passed (137 source files).hatch run dev-core:test -q --tb=short: 2,124 passed.hatch run dev-core:typecheck: passed (96 source files).hatch fmt --checkruns andgit diff --check: passed.hatch run dev-testing:test -q --tb=short: 1,839 passed (two existingdeprecation warnings), with loopback access for HTTP integration tests.
hatch run dev-testing:js-examples --js-dir <pinned-checkout> --no-build:123 test files passed, 150 tests passed, zero retries; four selected files
have no cloud tests. Used JS SDK commit
3deb8389531d35642e2d4657a69575522dc7a7ea, the same source validated for [Testing]: Record callback terminal completions as updated operations #860/[Testing]: Acknowledge invocation input without losing pending updates #861.Checklist
By submitting this pull request, I confirm that you can use, modify, copy, and
redistribute this contribution, under the terms of your choice.