From 82338895fcf9c7c7ed7aa5f1318753553dc8a42a Mon Sep 17 00:00:00 2001 From: Bernd Verst Date: Sun, 26 Jul 2026 23:11:16 -0700 Subject: [PATCH 1/6] Serialize history events in one pass HistoryEvent.to_dict() called _to_serializable(asdict(self)). asdict() deep-copies the whole event graph into a throwaway structure, which _to_serializable() then walked again. That is two full traversals plus a discarded allocation per event, multiplied across every event in a history export. Walk the dataclass fields directly instead: _to_serializable() now converts dataclass instances itself, field-name tuples are cached per type, and exactly-JSON-native scalars take a fast path. The dataclass check mirrors the dataclasses._is_dataclass_instance predicate asdict() used, so dataclass types stay leaves and only instances recurse. Output is unchanged: same keys, ordering, nesting, isoformat timestamps, and handling of None/enums/nested dataclasses/lists/dicts. Add an equivalence suite pinning to_dict() against the old two-pass form across every HistoryEvent subclass. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 6322dc7f-81ee-4d42-be2d-cef84cf62d15 --- durabletask/history.py | 47 ++- .../durabletask/test_history_serialization.py | 394 ++++++++++++++++++ 2 files changed, 439 insertions(+), 2 deletions(-) create mode 100644 tests/durabletask/test_history_serialization.py diff --git a/durabletask/history.py b/durabletask/history.py index c72a3769..a014f0a5 100644 --- a/durabletask/history.py +++ b/durabletask/history.py @@ -4,7 +4,7 @@ from __future__ import annotations from collections.abc import Callable -from dataclasses import asdict, dataclass +from dataclasses import dataclass, fields from datetime import datetime, timezone from typing import Any, cast @@ -42,7 +42,10 @@ class HistoryEvent: timestamp: datetime def to_dict(self) -> dict[str, Any]: - return _to_serializable(asdict(self)) + return { + name: _to_serializable(getattr(self, name)) + for name in _field_names(type(self)) + } @dataclass(slots=True) @@ -326,7 +329,47 @@ def _message_to_dict(msg: Message) -> dict[str, Any]: return json_format.MessageToDict(msg, preserving_proto_field_name=True) +# Field names are looked up once per dataclass type. History export walks +# many events of the same handful of types, so caching avoids repeatedly +# rebuilding the tuple returned by ``dataclasses.fields``. +_FIELD_NAMES: dict[type[Any], tuple[str, ...]] = {} + +# Values of these exact types are already JSON-native and need no +# conversion. Checking ``type(value)`` against a set is a single hash +# lookup, which short-circuits the common case (most event fields are +# strings, ints, or ``None``) before the ``isinstance`` chain below. +_JSON_NATIVE_TYPES: frozenset[type[Any]] = frozenset({bool, float, int, str, type(None)}) + + +def _field_names(cls: type[Any]) -> tuple[str, ...]: + names = _FIELD_NAMES.get(cls) + if names is None: + names = tuple(field.name for field in fields(cast(Any, cls))) + _FIELD_NAMES[cls] = names + return names + + def _to_serializable(value: Any) -> Any: + """Recursively convert *value* into a JSON-safe structure. + + This walks dataclass instances directly instead of going through + ``dataclasses.asdict``, which would deep-copy the whole event graph + into a throwaway intermediate structure that then has to be walked a + second time. The output is identical to that two-pass form: nested + dataclasses become dicts in field order, datetimes become ISO 8601 + strings, lists and dicts are rebuilt, and every other value is + passed through unchanged. + """ + value_type = cast('type[Any]', type(value)) + if value_type in _JSON_NATIVE_TYPES: + return value + # Mirrors the private ``dataclasses._is_dataclass_instance`` check that + # ``asdict`` used: dataclass *types* are leaves, only instances recurse. + if hasattr(value_type, '__dataclass_fields__'): + return { + name: _to_serializable(getattr(value, name)) + for name in _field_names(value_type) + } if isinstance(value, datetime): return value.isoformat() if isinstance(value, list): diff --git a/tests/durabletask/test_history_serialization.py b/tests/durabletask/test_history_serialization.py new file mode 100644 index 00000000..f1475ff9 --- /dev/null +++ b/tests/durabletask/test_history_serialization.py @@ -0,0 +1,394 @@ +# Copyright (c) Microsoft Corporation. +# Licensed under the MIT License. + +"""Tests for :meth:`durabletask.history.HistoryEvent.to_dict`. + +``to_dict`` walks the dataclass graph in a single pass. It used to be +implemented as ``_to_serializable(asdict(self))``, which deep-copied the +whole event into a throwaway structure and then walked that copy again. +The tests below pin the output of the current implementation to that +original two-pass form so the optimization stays behavior-preserving. +""" + +from __future__ import annotations + +import json +from dataclasses import asdict +from datetime import datetime, timezone +from typing import Any, cast + +import pytest + +from durabletask import history, task + +_TS = datetime(2025, 1, 2, 3, 4, 5, 678901, tzinfo=timezone.utc) +_NAIVE_TS = datetime(2024, 12, 31, 23, 59, 58) +_FIRE_AT = datetime(2025, 6, 7, 8, 9, 10, tzinfo=timezone.utc) + + +def _legacy_to_serializable(value: Any) -> Any: + """The pre-optimization value walker, kept verbatim as a test oracle.""" + if isinstance(value, datetime): + return value.isoformat() + if isinstance(value, list): + return [_legacy_to_serializable(item) for item in cast(list[Any], value)] + if isinstance(value, dict): + return { + key: _legacy_to_serializable(item) + for key, item in cast(dict[Any, Any], value).items() + } + return value + + +def _legacy_to_dict(event: history.HistoryEvent) -> dict[str, Any]: + """The pre-optimization ``to_dict`` body, kept verbatim as a test oracle.""" + return _legacy_to_serializable(asdict(event)) + + +def _failure(message: str = 'boom') -> task.FailureDetails: + return task.FailureDetails(message, 'RuntimeError', 'Traceback...') + + +def _trace_context() -> history.TraceContext: + return history.TraceContext( + trace_parent='00-trace-span-01', + span_id='span-id', + trace_state='state=1', + ) + + +def _parent_instance() -> history.ParentInstanceInfo: + return history.ParentInstanceInfo( + task_scheduled_id=7, + name='ParentOrch', + version='2.0', + orchestration_instance=history.OrchestrationInstance( + instance_id='parent-instance', + execution_id='parent-execution', + ), + ) + + +def _orchestration_state() -> dict[str, Any]: + """A deeply nested ``dict[str, Any]`` payload, as produced by MessageToDict.""" + return { + 'instanceId': 'abc', + 'orchestrationStatus': 'ORCHESTRATION_STATUS_RUNNING', + 'createdTimestamp': _TS, + 'retryCount': 3, + 'completed': False, + 'progress': 0.75, + 'output': None, + 'tags': {'a': '1', 'b': '2'}, + 'nested': { + 'level2': { + 'level3': ['x', 'y', {'level4': _TS}], + }, + 'failure': _failure('nested failure'), + }, + 'history': [ + {'eventId': 1, 'timestamp': _TS}, + {'eventId': 2, 'timestamp': _NAIVE_TS}, + [_failure('in a list'), _TS, None, True, 1.5], + ], + } + + +def _sample_events() -> list[history.HistoryEvent]: + """One fully-populated and one minimally-populated instance per event type.""" + return [ + history.HistoryEvent(event_id=0, timestamp=_TS), + history.ExecutionStartedEvent( + event_id=-1, + timestamp=_TS, + name='MyOrch', + version='1.0', + input='"hello"', + orchestration_instance=history.OrchestrationInstance( + instance_id='inst', execution_id='exec', + ), + parent_instance=_parent_instance(), + scheduled_start_timestamp=_NAIVE_TS, + parent_trace_context=_trace_context(), + orchestration_span_id='orch-span', + tags={'k': 'v', 'k2': 'v2'}, + ), + history.ExecutionStartedEvent(event_id=-1, timestamp=_TS, name='Bare'), + history.ExecutionCompletedEvent( + event_id=-1, + timestamp=_TS, + orchestration_status=3, + result='"done"', + failure_details=_failure(), + ), + history.ExecutionCompletedEvent( + event_id=-1, timestamp=_TS, orchestration_status=1, + ), + history.ExecutionTerminatedEvent( + event_id=-1, timestamp=_TS, input='"bye"', recurse=True, + ), + history.ExecutionTerminatedEvent(event_id=-1, timestamp=_TS), + history.TaskScheduledEvent( + event_id=1, + timestamp=_TS, + name='MyActivity', + version='1.2.3', + input='42', + parent_trace_context=_trace_context(), + tags={'t': 'v'}, + ), + history.TaskScheduledEvent(event_id=1, timestamp=_TS, name='Bare'), + history.TaskCompletedEvent( + event_id=-1, timestamp=_TS, task_scheduled_id=1, result='43', + ), + history.TaskFailedEvent( + event_id=-1, timestamp=_TS, task_scheduled_id=1, + failure_details=_failure(), + ), + history.TaskFailedEvent(event_id=-1, timestamp=_TS, task_scheduled_id=1), + history.SubOrchestrationInstanceCreatedEvent( + event_id=2, + timestamp=_TS, + instance_id='child', + name='ChildOrch', + version='9', + input='null', + parent_trace_context=_trace_context(), + tags={'x': 'y'}, + ), + history.SubOrchestrationInstanceCompletedEvent( + event_id=-1, timestamp=_TS, task_scheduled_id=2, result='"ok"', + ), + history.SubOrchestrationInstanceFailedEvent( + event_id=-1, timestamp=_TS, task_scheduled_id=2, + failure_details=_failure(), + ), + history.TimerCreatedEvent(event_id=3, timestamp=_TS, fire_at=_FIRE_AT), + history.TimerFiredEvent( + event_id=-1, timestamp=_TS, fire_at=_FIRE_AT, timer_id=3, + ), + history.OrchestratorStartedEvent(event_id=-1, timestamp=_TS), + history.OrchestratorCompletedEvent(event_id=-1, timestamp=_TS), + history.EventSentEvent( + event_id=4, timestamp=_TS, instance_id='other', name='Ping', + input='"data"', + ), + history.EventRaisedEvent( + event_id=-1, timestamp=_TS, name='Pong', input='"data"', + ), + history.EventRaisedEvent(event_id=-1, timestamp=_TS, name='Pong'), + history.GenericEvent(event_id=-1, timestamp=_TS, data='some data'), + history.HistoryStateEvent( + event_id=-1, timestamp=_TS, orchestration_state=_orchestration_state(), + ), + history.HistoryStateEvent( + event_id=-1, timestamp=_TS, orchestration_state={}, + ), + history.ContinueAsNewEvent(event_id=-1, timestamp=_TS, input='"again"'), + history.ExecutionSuspendedEvent(event_id=-1, timestamp=_TS, input='"why"'), + history.ExecutionResumedEvent(event_id=-1, timestamp=_TS, input=None), + history.EntityOperationSignaledEvent( + event_id=-1, + timestamp=_TS, + request_id='req-1', + operation='op', + scheduled_time=_FIRE_AT, + input='"payload"', + target_instance_id='@entity@key', + ), + history.EntityOperationSignaledEvent( + event_id=-1, timestamp=_TS, request_id='req-1', operation='op', + ), + history.EntityOperationCalledEvent( + event_id=-1, + timestamp=_TS, + request_id='req-2', + operation='op', + scheduled_time=_FIRE_AT, + input='"payload"', + parent_instance_id='parent', + parent_execution_id='exec', + target_instance_id='@entity@key', + ), + history.EntityOperationCompletedEvent( + event_id=-1, timestamp=_TS, request_id='req-2', output='"result"', + ), + history.EntityOperationFailedEvent( + event_id=-1, timestamp=_TS, request_id='req-2', + failure_details=_failure(), + ), + history.EntityLockRequestedEvent( + event_id=-1, + timestamp=_TS, + critical_section_id='cs-1', + lock_set=['@a@1', '@b@2'], + position=0, + parent_instance_id='parent', + ), + history.EntityLockRequestedEvent( + event_id=-1, timestamp=_TS, critical_section_id='cs-1', + lock_set=[], position=1, + ), + history.EntityLockGrantedEvent( + event_id=-1, timestamp=_TS, critical_section_id='cs-1', + ), + history.EntityUnlockSentEvent( + event_id=-1, + timestamp=_TS, + critical_section_id='cs-1', + parent_instance_id='parent', + target_instance_id='@entity@key', + ), + history.ExecutionRewoundEvent( + event_id=-1, + timestamp=_TS, + reason='manual', + parent_execution_id='exec', + instance_id='inst', + parent_trace_context=_trace_context(), + name='MyOrch', + version='1.0', + input='"hello"', + parent_instance=_parent_instance(), + tags={'r': 'w'}, + ), + history.ExecutionRewoundEvent(event_id=-1, timestamp=_TS), + ] + + +def _event_ids(events: list[history.HistoryEvent]) -> list[str]: + seen: dict[str, int] = {} + ids: list[str] = [] + for event in events: + name = type(event).__name__ + seen[name] = seen.get(name, 0) + 1 + ids.append(f'{name}-{seen[name]}') + return ids + + +_SAMPLE_EVENTS = _sample_events() + + +class TestToDictEquivalence: + """``to_dict`` must produce exactly what the old two-pass form produced.""" + + @pytest.mark.parametrize( + 'event', _SAMPLE_EVENTS, ids=_event_ids(_SAMPLE_EVENTS), + ) + def test_matches_legacy_two_pass_output( + self, event: history.HistoryEvent, + ) -> None: + actual = event.to_dict() + expected = _legacy_to_dict(event) + assert actual == expected + # ``repr`` pins key ordering and value types, not just equality. + assert repr(actual) == repr(expected) + + def test_covers_every_history_event_type(self) -> None: + exported = { + getattr(history, name) + for name in history.__all__ + if isinstance(getattr(history, name), type) + } + declared = { + cls for cls in exported + if issubclass(cls, history.HistoryEvent) + } + covered = {type(event) for event in _SAMPLE_EVENTS} + assert declared - covered == set() + + def test_module_level_to_dict_helper_matches(self) -> None: + for event in _SAMPLE_EVENTS: + assert history.to_dict(event) == _legacy_to_dict(event) + + +class TestToDictOutputShape: + def test_timestamps_use_isoformat(self) -> None: + event = history.TimerFiredEvent( + event_id=5, timestamp=_TS, fire_at=_FIRE_AT, timer_id=3, + ) + assert event.to_dict() == { + 'event_id': 5, + 'timestamp': _TS.isoformat(), + 'fire_at': _FIRE_AT.isoformat(), + 'timer_id': 3, + } + + def test_field_order_follows_dataclass_declaration(self) -> None: + event = history.TaskScheduledEvent( + event_id=1, timestamp=_TS, name='A', version='1', input='2', + parent_trace_context=_trace_context(), tags={'k': 'v'}, + ) + assert list(event.to_dict()) == [ + 'event_id', 'timestamp', 'name', 'version', 'input', + 'parent_trace_context', 'tags', + ] + + def test_nested_dataclasses_become_dicts(self) -> None: + event = history.ExecutionStartedEvent( + event_id=-1, + timestamp=_TS, + name='MyOrch', + parent_instance=_parent_instance(), + ) + assert event.to_dict()['parent_instance'] == { + 'task_scheduled_id': 7, + 'name': 'ParentOrch', + 'version': '2.0', + 'orchestration_instance': { + 'instance_id': 'parent-instance', + 'execution_id': 'parent-execution', + }, + } + + def test_none_values_are_preserved(self) -> None: + event = history.ExecutionStartedEvent( + event_id=-1, timestamp=_TS, name='MyOrch', + ) + payload = event.to_dict() + assert payload['version'] is None + assert payload['parent_instance'] is None + assert payload['tags'] is None + + @pytest.mark.parametrize( + 'event', _SAMPLE_EVENTS, ids=_event_ids(_SAMPLE_EVENTS), + ) + def test_output_is_json_serializable( + self, event: history.HistoryEvent, + ) -> None: + json.dumps(event.to_dict(), sort_keys=True) + + +class TestToDictIsolation: + """The returned structure must not alias the event's own containers.""" + + def test_mutating_result_does_not_affect_event(self) -> None: + event = history.ExecutionStartedEvent( + event_id=-1, + timestamp=_TS, + name='MyOrch', + tags={'k': 'v'}, + ) + payload = event.to_dict() + payload['tags']['k'] = 'mutated' + assert event.tags == {'k': 'v'} + + def test_nested_containers_are_rebuilt(self) -> None: + state = _orchestration_state() + event = history.HistoryStateEvent( + event_id=-1, timestamp=_TS, orchestration_state=state, + ) + payload = event.to_dict() + assert payload['orchestration_state'] is not state + assert payload['orchestration_state']['tags'] is not state['tags'] + assert payload['orchestration_state']['history'] is not state['history'] + + def test_lock_set_list_is_rebuilt(self) -> None: + lock_set = ['@a@1', '@b@2'] + event = history.EntityLockRequestedEvent( + event_id=-1, timestamp=_TS, critical_section_id='cs', + lock_set=lock_set, position=0, + ) + payload = event.to_dict() + assert payload['lock_set'] == lock_set + assert payload['lock_set'] is not lock_set From fa764226c7260100aec563d477bff51a7ea3e0b0 Mon Sep 17 00:00:00 2001 From: Bernd Verst Date: Mon, 27 Jul 2026 09:44:30 -0700 Subject: [PATCH 2/6] Fix tuple recursion and leaf aliasing in single-pass serializer Review of the single-pass walker surfaced two defects against the ``dataclasses.asdict`` behavior it replaced. Aliasing: ``asdict`` ended its recursion with ``copy.deepcopy``, so the dict it returned never shared mutable state with the event. The walker fell through with a bare ``return value``, letting sets, bytearrays and custom objects be handed out by reference. A caller mutating the exported structure could reach back into the live event. Tuple recursion: ``asdict`` rebuilt tuples, converting any nested dataclass into a dict. The walker had no tuple branch, so a tuple holding a dataclass came back holding the raw instance -- a different value, and one json.dumps cannot encode. Tuples now recurse exactly like lists, with namedtuples rebuilt through their positional constructor so the concrete type survives. This deliberately diverges from the old two-pass form, which left datetimes inside tuples unconverted and so produced output that was not JSON encodable; that quirk is not reproduced. Both branches are unreachable for SDK-produced events. The only ``dict[str, Any]`` field is ``HistoryStateEvent.orchestration_state``, populated solely by ``json_format.MessageToDict``, which yields JSON-native values. The fix measures at +0.7% over the corpus, within run-to-run noise, and the walker stays 5.8x faster than the original. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 6322dc7f-81ee-4d42-be2d-cef84cf62d15 --- durabletask/history.py | 21 +- .../durabletask/test_history_serialization.py | 188 +++++++++++++++++- 2 files changed, 201 insertions(+), 8 deletions(-) diff --git a/durabletask/history.py b/durabletask/history.py index a014f0a5..ce37b35a 100644 --- a/durabletask/history.py +++ b/durabletask/history.py @@ -3,6 +3,7 @@ from __future__ import annotations +import copy from collections.abc import Callable from dataclasses import dataclass, fields from datetime import datetime, timezone @@ -355,10 +356,10 @@ def _to_serializable(value: Any) -> Any: This walks dataclass instances directly instead of going through ``dataclasses.asdict``, which would deep-copy the whole event graph into a throwaway intermediate structure that then has to be walked a - second time. The output is identical to that two-pass form: nested - dataclasses become dicts in field order, datetimes become ISO 8601 - strings, lists and dicts are rebuilt, and every other value is - passed through unchanged. + second time. Nested dataclasses become dicts in field order, + datetimes become ISO 8601 strings, lists, tuples and dicts are + rebuilt, and anything else is deep-copied so the result never shares + mutable state with the event it came from. """ value_type = cast('type[Any]', type(value)) if value_type in _JSON_NATIVE_TYPES: @@ -374,12 +375,22 @@ def _to_serializable(value: Any) -> Any: return value.isoformat() if isinstance(value, list): return [_to_serializable(item) for item in cast(list[Any], value)] + if isinstance(value, tuple): + items = [_to_serializable(item) for item in cast(tuple[Any, ...], value)] + # Namedtuples take their fields as positional arguments rather than + # a single iterable, so rebuilding them needs the unpacked form. + if hasattr(value_type, '_fields'): + return value_type(*items) + return value_type(items) if isinstance(value, dict): return { key: _to_serializable(item) for key, item in cast(dict[Any, Any], value).items() } - return value + # ``asdict`` ended its recursion with ``copy.deepcopy``. Keeping that + # behavior means callers can freely mutate the exported structure + # without reaching back into the live event. + return copy.deepcopy(value) _EVENT_CONVERTERS: dict[str, Callable[[pb.HistoryEvent], HistoryEvent]] = { diff --git a/tests/durabletask/test_history_serialization.py b/tests/durabletask/test_history_serialization.py index f1475ff9..dc4cb440 100644 --- a/tests/durabletask/test_history_serialization.py +++ b/tests/durabletask/test_history_serialization.py @@ -13,9 +13,9 @@ from __future__ import annotations import json -from dataclasses import asdict +from dataclasses import asdict, dataclass from datetime import datetime, timezone -from typing import Any, cast +from typing import Any, NamedTuple, cast import pytest @@ -49,6 +49,41 @@ def _failure(message: str = 'boom') -> task.FailureDetails: return task.FailureDetails(message, 'RuntimeError', 'Traceback...') +@dataclass +class _Inner: + """A dataclass the walker should convert wherever it is nested.""" + + label: str + when: datetime + + +class _Point(NamedTuple): + """A namedtuple, which rebuilds differently from a plain tuple.""" + + x: Any + y: Any + + +class _MutableLeaf: + """A value the walker cannot convert, so it must be copied out.""" + + def __init__(self, n: int) -> None: + self.n = n + + def __eq__(self, other: object) -> bool: + return isinstance(other, _MutableLeaf) and other.n == self.n + + def __repr__(self) -> str: + return f'_MutableLeaf({self.n})' + + +def _state_event(value: Any) -> history.HistoryStateEvent: + """Wrap ``value`` in the only ``dict[str, Any]`` field on any event.""" + return history.HistoryStateEvent( + event_id=-1, timestamp=_TS, orchestration_state={'value': value}, + ) + + def _trace_context() -> history.TraceContext: return history.TraceContext( trace_parent='00-trace-span-01', @@ -270,7 +305,12 @@ def _event_ids(events: list[history.HistoryEvent]) -> list[str]: class TestToDictEquivalence: - """``to_dict`` must produce exactly what the old two-pass form produced.""" + """``to_dict`` must produce exactly what the old two-pass form produced. + + Tuple-bearing payloads are deliberately excluded from this suite and + covered by :class:`TestTupleHandling` instead: the old two-pass form + mishandled tuples, so matching it there would mean reproducing a bug. + """ @pytest.mark.parametrize( 'event', _SAMPLE_EVENTS, ids=_event_ids(_SAMPLE_EVENTS), @@ -392,3 +432,145 @@ def test_lock_set_list_is_rebuilt(self) -> None: payload = event.to_dict() assert payload['lock_set'] == lock_set assert payload['lock_set'] is not lock_set + + @pytest.mark.parametrize( + 'leaf', + [ + pytest.param(_MutableLeaf(1), id='custom-object'), + pytest.param(bytearray(b'abc'), id='bytearray'), + pytest.param({1, 2, 3}, id='set'), + pytest.param((1, 'x'), id='tuple'), + pytest.param(_Point(1, 2), id='namedtuple'), + ], + ) + def test_mutable_leaf_is_not_aliased(self, leaf: Any) -> None: + """Values the walker does not recognize must still be copied out. + + ``dataclasses.asdict`` ended its recursion with ``copy.deepcopy``, + so the dict it produced never shared mutable state with the event. + The single-pass walker has to preserve that isolation, otherwise a + caller mutating the exported dict would corrupt the live event. + """ + event = _state_event(leaf) + exported = event.to_dict()['orchestration_state']['value'] + assert exported == leaf + assert exported is not leaf + + def test_mutating_exported_leaf_does_not_affect_event(self) -> None: + leaf = _MutableLeaf(1) + event = _state_event(leaf) + + exported = event.to_dict()['orchestration_state']['value'] + exported.n = 999 + + assert leaf.n == 1 + assert event.orchestration_state['value'].n == 1 + + def test_mutating_exported_bytearray_does_not_affect_event(self) -> None: + leaf = bytearray(b'abc') + event = _state_event(leaf) + + exported = event.to_dict()['orchestration_state']['value'] + exported.extend(b'def') + + assert leaf == bytearray(b'abc') + + def test_nested_mutable_leaf_is_not_aliased(self) -> None: + """Isolation must hold for leaves buried inside lists and dicts.""" + leaf = _MutableLeaf(7) + event = _state_event({'deep': [leaf]}) + + exported = event.to_dict()['orchestration_state']['value']['deep'][0] + assert exported == leaf + assert exported is not leaf + + +class TestTupleHandling: + """Tuples must recurse the same way lists do. + + The old two-pass form got this wrong in a way worth spelling out. + ``dataclasses.asdict`` *did* rebuild tuples, converting any nested + dataclass into a dict, but the ``_to_serializable`` pass that ran + afterwards had no tuple branch, so datetimes sitting inside a tuple + were never converted. The result was a value that could not be JSON + encoded. The single-pass walker handles tuples exactly like lists, + which diverges from the old output on purpose. + """ + + def test_tuple_containing_dataclass_is_converted(self) -> None: + event = _state_event((_Inner('a', _TS),)) + exported = event.to_dict()['orchestration_state']['value'] + assert exported == ({'label': 'a', 'when': _TS.isoformat()},) + + def test_datetime_inside_tuple_is_converted(self) -> None: + event = _state_event((_TS,)) + exported = event.to_dict()['orchestration_state']['value'] + assert exported == (_TS.isoformat(),) + + def test_tuple_type_is_preserved(self) -> None: + event = _state_event((1, 'x')) + exported = event.to_dict()['orchestration_state']['value'] + assert isinstance(exported, tuple) + + def test_namedtuple_type_is_preserved(self) -> None: + """Rebuilding a tuple must not flatten a namedtuple into a plain one.""" + event = _state_event(_Point(1, 2)) + exported = event.to_dict()['orchestration_state']['value'] + assert isinstance(exported, _Point) + assert exported == _Point(1, 2) + + def test_namedtuple_contents_are_converted(self) -> None: + event = _state_event(_Point(_TS, _Inner('b', _TS))) + exported = event.to_dict()['orchestration_state']['value'] + assert exported == _Point( + _TS.isoformat(), {'label': 'b', 'when': _TS.isoformat()}, + ) + + def test_tuple_nested_in_dict_and_list_is_converted(self) -> None: + event = _state_event({'k': [(_Inner('c', _TS),)]}) + exported = event.to_dict()['orchestration_state']['value'] + assert exported == {'k': [({'label': 'c', 'when': _TS.isoformat()},)]} + + def test_empty_tuple_round_trips(self) -> None: + event = _state_event(()) + assert event.to_dict()['orchestration_state']['value'] == () + + @pytest.mark.parametrize( + 'payload', + [ + pytest.param((_Inner('a', _TS),), id='tuple-of-dataclass'), + pytest.param((_TS,), id='tuple-of-datetime'), + pytest.param(_Point(_TS, 2), id='namedtuple-of-datetime'), + pytest.param({'k': [(_TS,)]}, id='tuple-nested-in-dict-and-list'), + ], + ) + def test_tuple_payloads_are_json_serializable(self, payload: Any) -> None: + """The old two-pass form produced tuples json.dumps could not encode.""" + event = _state_event(payload) + json.dumps(event.to_dict()) + + @pytest.mark.parametrize( + 'payload', + [ + pytest.param((_Inner('a', _TS),), id='tuple-of-dataclass'), + pytest.param((_TS,), id='tuple-of-datetime'), + ], + ) + def test_tuple_output_deliberately_diverges_from_legacy( + self, payload: Any, + ) -> None: + """Pin the divergence so it stays intentional rather than accidental. + + The legacy output is not JSON encodable for these payloads; the + new output is. This test fails loudly if anyone ever "restores" + bug-for-bug parity with the old two-pass form. + """ + event = _state_event(payload) + + legacy = _legacy_to_dict(event) + with pytest.raises(TypeError): + json.dumps(legacy) + + actual = event.to_dict() + assert actual != legacy + json.dumps(actual) From e969d30376e57fb308a4d79446aae1cf4b1f5428 Mon Sep 17 00:00:00 2001 From: Bernd Verst Date: Mon, 27 Jul 2026 13:39:10 -0700 Subject: [PATCH 3/6] Preserve legacy asdict semantics for values outside the fast paths The single-pass walker replaced dataclasses.asdict, but a lot of compatibility behavior lived inside asdict rather than in the conversion pass that followed it: container subclasses were rebuilt through their own constructors, mapping keys were recursed, namedtuples and defaultdict were special-cased, and every leaf was deep-copied. Walking values in place silently dropped all of that. Restrict the inline fast paths to exact built-in types, where walking in place is provably identical to what asdict produced, and hand every other value back to the real asdict via a throwaway box dataclass. Delegating rather than reimplementing matters because those internals have shifted between releases and this package supports 3.10 through 3.14. Also remove the tuple branch added earlier. Normalizing datetimes inside tuples is a real bug fix, but a user-visible one, so it does not belong in a performance change. Tuples now route to the compatibility path and come back exactly as they did before, quirk included. Bound the field-name cache with functools.lru_cache so dynamically created dataclass types cannot pin an unbounded number of entries. It measures identically to the unbounded dict (0.0204s vs 0.0206s per 200k lookups) while dropping the cache entirely is 9.4x slower, so the cache stays but is now capped. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 6322dc7f-81ee-4d42-be2d-cef84cf62d15 --- durabletask/history.py | 95 +++-- .../durabletask/test_history_serialization.py | 329 +++++++++++++++--- 2 files changed, 341 insertions(+), 83 deletions(-) diff --git a/durabletask/history.py b/durabletask/history.py index ce37b35a..e093dc2c 100644 --- a/durabletask/history.py +++ b/durabletask/history.py @@ -3,9 +3,9 @@ from __future__ import annotations -import copy +import functools from collections.abc import Callable -from dataclasses import dataclass, fields +from dataclasses import asdict, dataclass, fields from datetime import datetime, timezone from typing import Any, cast @@ -332,22 +332,60 @@ def _message_to_dict(msg: Message) -> dict[str, Any]: # Field names are looked up once per dataclass type. History export walks # many events of the same handful of types, so caching avoids repeatedly -# rebuilding the tuple returned by ``dataclasses.fields``. -_FIELD_NAMES: dict[type[Any], tuple[str, ...]] = {} +# rebuilding the tuple returned by ``dataclasses.fields``. The cache is +# bounded so that dynamically created dataclass types cannot pin an +# unbounded number of entries (and the classes they reference) in memory. +_FIELD_NAMES_CACHE_SIZE = 256 # Values of these exact types are already JSON-native and need no # conversion. Checking ``type(value)`` against a set is a single hash # lookup, which short-circuits the common case (most event fields are -# strings, ints, or ``None``) before the ``isinstance`` chain below. +# strings, ints, or ``None``) before the type checks below. _JSON_NATIVE_TYPES: frozenset[type[Any]] = frozenset({bool, float, int, str, type(None)}) +@functools.lru_cache(maxsize=_FIELD_NAMES_CACHE_SIZE) def _field_names(cls: type[Any]) -> tuple[str, ...]: - names = _FIELD_NAMES.get(cls) - if names is None: - names = tuple(field.name for field in fields(cast(Any, cls))) - _FIELD_NAMES[cls] = names - return names + return tuple(field.name for field in fields(cast(Any, cls))) + + +@dataclass +class _LegacyBox: + """Carrier used to re-enter ``dataclasses.asdict`` for one value.""" + + value: Any + + +def _asdict_only(value: Any) -> Any: + """Apply ``dataclasses.asdict`` recursion to *value* and nothing else. + + Boxing the value in a throwaway dataclass lets the interpreter's own + ``asdict`` implementation handle it, so container subclasses, + namedtuples, ``defaultdict`` and the deep-copy of leaf values all behave + exactly as they did before this module walked events itself. Delegating + rather than reimplementing matters because those details have changed + between Python releases and this package supports several of them. + """ + return asdict(_LegacyBox(value))['value'] + + +def _legacy_walk(value: Any) -> Any: + """The pre-optimization conversion pass, applied to an ``asdict`` result.""" + if isinstance(value, datetime): + return value.isoformat() + if isinstance(value, list): + return [_legacy_walk(item) for item in cast(list[Any], value)] + if isinstance(value, dict): + return { + key: _legacy_walk(item) + for key, item in cast(dict[Any, Any], value).items() + } + return value + + +def _legacy_compat(value: Any) -> Any: + """Reproduce the original ``asdict`` + walk pipeline for one value.""" + return _legacy_walk(_asdict_only(value)) def _to_serializable(value: Any) -> Any: @@ -356,10 +394,15 @@ def _to_serializable(value: Any) -> Any: This walks dataclass instances directly instead of going through ``dataclasses.asdict``, which would deep-copy the whole event graph into a throwaway intermediate structure that then has to be walked a - second time. Nested dataclasses become dicts in field order, - datetimes become ISO 8601 strings, lists, tuples and dicts are - rebuilt, and anything else is deep-copied so the result never shares - mutable state with the event it came from. + second time. + + The type checks below are deliberately exact rather than + ``isinstance``. Only the built-in types are handled inline, because + only for those is walking in place provably identical to what + ``asdict`` produced. Subclasses, tuples and every other value are + routed to :func:`_legacy_compat`, which re-enters the real ``asdict`` + so their original semantics -- constructor round-trips, key + recursion and deep-copied leaves -- are preserved exactly. """ value_type = cast('type[Any]', type(value)) if value_type in _JSON_NATIVE_TYPES: @@ -371,26 +414,20 @@ def _to_serializable(value: Any) -> Any: name: _to_serializable(getattr(value, name)) for name in _field_names(value_type) } - if isinstance(value, datetime): + if value_type is datetime: return value.isoformat() - if isinstance(value, list): + if value_type is list: return [_to_serializable(item) for item in cast(list[Any], value)] - if isinstance(value, tuple): - items = [_to_serializable(item) for item in cast(tuple[Any, ...], value)] - # Namedtuples take their fields as positional arguments rather than - # a single iterable, so rebuilding them needs the unpacked form. - if hasattr(value_type, '_fields'): - return value_type(*items) - return value_type(items) - if isinstance(value, dict): + if value_type is dict: + # ``asdict`` recursed into keys but the conversion pass that followed + # it did not, so keys get ``asdict`` semantics only. Native keys are + # returned as-is because ``asdict`` leaves those untouched too. return { - key: _to_serializable(item) + (key if type(key) in _JSON_NATIVE_TYPES else _asdict_only(key)): + _to_serializable(item) for key, item in cast(dict[Any, Any], value).items() } - # ``asdict`` ended its recursion with ``copy.deepcopy``. Keeping that - # behavior means callers can freely mutate the exported structure - # without reaching back into the live event. - return copy.deepcopy(value) + return _legacy_compat(value) _EVENT_CONVERTERS: dict[str, Callable[[pb.HistoryEvent], HistoryEvent]] = { diff --git a/tests/durabletask/test_history_serialization.py b/tests/durabletask/test_history_serialization.py index dc4cb440..dbf7cfa5 100644 --- a/tests/durabletask/test_history_serialization.py +++ b/tests/durabletask/test_history_serialization.py @@ -13,6 +13,7 @@ from __future__ import annotations import json +from collections import defaultdict from dataclasses import asdict, dataclass from datetime import datetime, timezone from typing import Any, NamedTuple, cast @@ -64,6 +65,17 @@ class _Point(NamedTuple): y: Any +@dataclass(frozen=True) +class _FrozenKey: + """A hashable dataclass, usable as a mapping key until ``asdict`` runs. + + ``asdict`` recurses into keys, turning this into an unhashable dict, + so using one as a key has always raised. That failure must survive. + """ + + name: str + + class _MutableLeaf: """A value the walker cannot convert, so it must be copied out.""" @@ -485,92 +497,301 @@ def test_nested_mutable_leaf_is_not_aliased(self) -> None: assert exported is not leaf -class TestTupleHandling: - """Tuples must recurse the same way lists do. - - The old two-pass form got this wrong in a way worth spelling out. - ``dataclasses.asdict`` *did* rebuild tuples, converting any nested - dataclass into a dict, but the ``_to_serializable`` pass that ran - afterwards had no tuple branch, so datetimes sitting inside a tuple - were never converted. The result was a value that could not be JSON - encoded. The single-pass walker handles tuples exactly like lists, - which diverges from the old output on purpose. +class _OddIsoDatetime(datetime): + """A datetime subclass that overrides the method the walker calls.""" + + def isoformat(self, sep: str = 'T', timespec: str = 'auto') -> str: + return 'CUSTOM:' + super().isoformat(sep, timespec) + + +class _OddCopyDatetime(datetime): + """A datetime subclass that changes when it is deep-copied. + + ``asdict`` deep-copied every leaf, so the conversion pass that ran + afterwards called ``isoformat`` on the *copy*, not the original. That + is only observable when copying is not the identity, which is exactly + what this fixture makes it. """ - def test_tuple_containing_dataclass_is_converted(self) -> None: - event = _state_event((_Inner('a', _TS),)) + def __deepcopy__(self, memo: dict[int, Any]) -> datetime: + return datetime(1999, 9, 9, 9, 9, 9) + + +class _DedupeList(list[Any]): + """A list subclass whose constructor changes the contents. + + The constructor deliberately is *not* idempotent: it appends a marker + every time it runs. ``asdict`` rebuilt list subclasses through their + own constructor, so the marker appears exactly once in the legacy + output. Walking the original in place instead would skip it, so an + idempotent constructor here would let that regression pass unnoticed. + """ + + def __init__(self, items: Any = ()) -> None: + seen: list[Any] = [] + for item in items: + if item not in seen: + seen.append(item) + seen.append('ctor-ran') + super().__init__(seen) + + +class _UpperDict(dict[Any, Any]): + """A dict subclass whose constructor changes the mapping. + + Like :class:`_DedupeList`, this records that it ran so that skipping + the constructor is detectable rather than silently equivalent. + """ + + def __init__(self, items: Any = ()) -> None: + rebuilt = { + key.upper() if isinstance(key, str) else key: value + for key, value in dict(items).items() + } + rebuilt['ctor-ran'] = True + super().__init__(rebuilt) + + +class _IterOnlyTuple(tuple[Any, ...]): + """A tuple subclass that only accepts an iterator, never a sequence. + + ``asdict`` rebuilds tuple subclasses from a generator expression, so a + constructor like this one round-trips fine. Passing an already-built + list instead would raise, which is what makes this a regression guard. + """ + + def __new__(cls, items: Any = ()) -> '_IterOnlyTuple': + if isinstance(items, (list, tuple)): + raise TypeError('iterator-only constructor') + return super().__new__(cls, items) + + +class _DeepCopyKey: + """A mapping key that turns into a plain string when deep-copied.""" + + def __init__(self, name: str) -> None: + self.name = name + + def __deepcopy__(self, memo: dict[int, Any]) -> str: + return 'deepcopied:' + self.name + + def __hash__(self) -> int: + return hash(self.name) + + def __eq__(self, other: object) -> bool: + return isinstance(other, _DeepCopyKey) and other.name == self.name + + +_EXOTIC_PAYLOADS = [ + pytest.param(_OddIsoDatetime(2024, 1, 2, 3, 4, 5), id='datetime-subclass'), + pytest.param( + _OddCopyDatetime(2024, 1, 2, 3, 4, 5), id='datetime-subclass-deepcopy', + ), + pytest.param(_DedupeList([1, 2, 2, 3]), id='list-subclass'), + pytest.param(_DedupeList([_TS, _TS]), id='list-subclass-of-datetime'), + pytest.param(_UpperDict({'a': 1, 'b': 2}), id='dict-subclass'), + pytest.param(defaultdict(int, {'a': 1}), id='defaultdict'), + pytest.param(_IterOnlyTuple(iter([1, 2])), id='tuple-subclass-iterator-only'), + pytest.param((1, 'x'), id='plain-tuple'), + pytest.param((), id='empty-tuple'), + pytest.param((_TS,), id='tuple-of-datetime'), + pytest.param((_Inner('a', _TS),), id='tuple-of-dataclass'), + pytest.param(_Point(1, 2), id='namedtuple'), + pytest.param(_Point(_TS, _Inner('b', _TS)), id='namedtuple-of-datetime'), + pytest.param({'k': [(_Inner('c', _TS),)]}, id='tuple-nested-in-dict-and-list'), + pytest.param({_DeepCopyKey('k'): 1}, id='dict-with-deepcopy-key'), + pytest.param({_TS: 'v'}, id='dict-with-datetime-key'), + pytest.param({(_TS, 1): 'v'}, id='dict-with-tuple-key'), + pytest.param({'plain': 1}, id='plain-dict'), + pytest.param([1, 'x', None], id='plain-list'), + pytest.param({1, 2, 3}, id='set'), + pytest.param(bytearray(b'abc'), id='bytearray'), + pytest.param(b'abc', id='bytes'), + pytest.param(_MutableLeaf(1), id='custom-object'), +] + + +class TestLegacyCompatibility: + """Values the fast paths do not cover must behave exactly as before. + + The optimization only walks a value in place when doing so is provably + identical to what ``dataclasses.asdict`` produced, which is true for + the exact built-in types and for dataclass instances. Everything else + -- container *subclasses*, tuples, namedtuples, custom objects and + exotic mapping keys -- carries semantics that lived inside ``asdict`` + itself: constructor round-trips, key recursion and deep-copied leaves. + Those values are handed back to the real ``asdict``, so this suite + pins that routing. + + The oracle here is ``asdict`` applied to the whole event, not to an + individual value, so it never re-enters the implementation being + tested. + """ + + @pytest.mark.parametrize('payload', _EXOTIC_PAYLOADS) + def test_exotic_payload_matches_legacy_two_pass_output( + self, payload: Any, + ) -> None: + event = _state_event(payload) + + actual = event.to_dict() + expected = _legacy_to_dict(event) + + assert actual == expected + # ``==`` alone would let a namedtuple compare equal to a plain + # tuple, so pin the exact repr as well. + assert repr(actual) == repr(expected) + + @pytest.mark.parametrize('payload', _EXOTIC_PAYLOADS) + def test_exotic_payload_raises_the_same_way_as_legacy( + self, payload: Any, + ) -> None: + """Inputs that used to fail must still fail identically.""" + event = _state_event(payload) + + try: + expected: Any = _legacy_to_dict(event) + except Exception as exc: # noqa: BLE001 - mirroring legacy behavior + with pytest.raises(type(exc)): + event.to_dict() + else: + assert event.to_dict() == expected + + def test_dict_key_that_cannot_be_rebuilt_still_raises(self) -> None: + """``asdict`` turned dataclass keys into unhashable dicts; keep that.""" + event = _state_event({_FrozenKey('z'): 1}) + + with pytest.raises(TypeError): + _legacy_to_dict(event) + with pytest.raises(TypeError): + event.to_dict() + + def test_datetime_subclass_conversion_runs_on_a_copy(self) -> None: + """``asdict`` deep-copied before the walker called ``isoformat``.""" + event = _state_event(_OddIsoDatetime(2024, 1, 2, 3, 4, 5)) + exported = event.to_dict()['orchestration_state']['value'] - assert exported == ({'label': 'a', 'when': _TS.isoformat()},) - def test_datetime_inside_tuple_is_converted(self) -> None: - event = _state_event((_TS,)) + assert exported == 'CUSTOM:2024-01-02T03:04:05' + + def test_datetime_subclass_deepcopy_substitution_is_honored(self) -> None: + """A subclass whose copy differs must be converted via that copy.""" + event = _state_event(_OddCopyDatetime(2024, 1, 2, 3, 4, 5)) + exported = event.to_dict()['orchestration_state']['value'] - assert exported == (_TS.isoformat(),) - def test_tuple_type_is_preserved(self) -> None: - event = _state_event((1, 'x')) + assert exported == '1999-09-09T09:09:09' + + def test_list_subclass_constructor_still_runs(self) -> None: + """The second marker only appears if the constructor re-ran. + + The instance already carries one marker from being built. The + legacy pipeline rebuilt it through its own constructor, adding a + second. Walking the original in place would emit only the first. + """ + event = _state_event(_DedupeList([1, 2, 2, 3])) + + exported = event.to_dict()['orchestration_state']['value'] + + assert exported == [1, 2, 3, 'ctor-ran', 'ctor-ran'] + + def test_dict_subclass_constructor_still_runs(self) -> None: + """``CTOR-RAN`` is the first marker, re-keyed by the second run.""" + event = _state_event(_UpperDict({'a': 1, 'b': 2})) + exported = event.to_dict()['orchestration_state']['value'] - assert isinstance(exported, tuple) + + assert exported == { + 'A': 1, 'B': 2, 'CTOR-RAN': True, 'ctor-ran': True, + } + + def test_tuple_subclass_is_rebuilt_from_an_iterator(self) -> None: + """Handing the constructor a pre-built list instead would raise.""" + event = _state_event(_IterOnlyTuple(iter([1, 2]))) + + exported = event.to_dict()['orchestration_state']['value'] + + assert tuple(exported) == (1, 2) def test_namedtuple_type_is_preserved(self) -> None: - """Rebuilding a tuple must not flatten a namedtuple into a plain one.""" event = _state_event(_Point(1, 2)) + exported = event.to_dict()['orchestration_state']['value'] + assert isinstance(exported, _Point) - assert exported == _Point(1, 2) + assert repr(exported) == repr(_Point(1, 2)) + + def test_mapping_key_is_deep_copied_like_asdict(self) -> None: + event = _state_event({_DeepCopyKey('k'): 1}) - def test_namedtuple_contents_are_converted(self) -> None: - event = _state_event(_Point(_TS, _Inner('b', _TS))) exported = event.to_dict()['orchestration_state']['value'] - assert exported == _Point( - _TS.isoformat(), {'label': 'b', 'when': _TS.isoformat()}, - ) - def test_tuple_nested_in_dict_and_list_is_converted(self) -> None: - event = _state_event({'k': [(_Inner('c', _TS),)]}) + assert exported == {'deepcopied:k': 1} + + def test_mapping_key_is_not_converted_by_the_value_walker(self) -> None: + """``asdict`` recursed into keys, but the walker after it did not. + + A datetime key therefore stayed a datetime rather than becoming an + ISO string, unlike a datetime *value*. + """ + event = _state_event({_TS: 'v'}) + exported = event.to_dict()['orchestration_state']['value'] - assert exported == {'k': [({'label': 'c', 'when': _TS.isoformat()},)]} + + assert list(exported) == [_TS] + + +class TestTupleHandling: + """Tuples keep their original ``asdict`` semantics. + + An earlier revision of this change added a dedicated tuple branch that + normalized datetimes inside tuples. That was a behavior change rather + than a performance change, so it was removed: tuples now route to the + compatibility path and come back exactly as they did before. The + quirk that a datetime nested in a tuple is left unconverted is + therefore preserved, quirk and all. + """ + + def test_tuple_type_is_preserved(self) -> None: + event = _state_event((1, 'x')) + exported = event.to_dict()['orchestration_state']['value'] + assert isinstance(exported, tuple) def test_empty_tuple_round_trips(self) -> None: event = _state_event(()) assert event.to_dict()['orchestration_state']['value'] == () - @pytest.mark.parametrize( - 'payload', - [ - pytest.param((_Inner('a', _TS),), id='tuple-of-dataclass'), - pytest.param((_TS,), id='tuple-of-datetime'), - pytest.param(_Point(_TS, 2), id='namedtuple-of-datetime'), - pytest.param({'k': [(_TS,)]}, id='tuple-nested-in-dict-and-list'), - ], - ) - def test_tuple_payloads_are_json_serializable(self, payload: Any) -> None: - """The old two-pass form produced tuples json.dumps could not encode.""" - event = _state_event(payload) - json.dumps(event.to_dict()) + def test_dataclass_inside_tuple_becomes_a_dict(self) -> None: + """``asdict`` recursed into tuples, so this part always worked.""" + event = _state_event((_Inner('a', _TS),)) + exported = event.to_dict()['orchestration_state']['value'] + assert exported == ({'label': 'a', 'when': _TS},) + + def test_datetime_inside_tuple_is_left_unconverted(self) -> None: + """Pins the pre-existing quirk this change must not silently fix.""" + event = _state_event((_TS,)) + exported = event.to_dict()['orchestration_state']['value'] + assert exported == (_TS,) @pytest.mark.parametrize( 'payload', [ - pytest.param((_Inner('a', _TS),), id='tuple-of-dataclass'), pytest.param((_TS,), id='tuple-of-datetime'), + pytest.param((_Inner('a', _TS),), id='tuple-of-dataclass'), ], ) - def test_tuple_output_deliberately_diverges_from_legacy( + def test_tuple_datetimes_remain_unencodable_like_legacy( self, payload: Any, ) -> None: - """Pin the divergence so it stays intentional rather than accidental. + """The quirk makes these payloads unencodable; legacy did the same. - The legacy output is not JSON encodable for these payloads; the - new output is. This test fails loudly if anyone ever "restores" - bug-for-bug parity with the old two-pass form. + Fixing that is a genuine bug fix, but a user-visible one, so it + belongs in its own change rather than riding along with a + performance optimization. """ event = _state_event(payload) - legacy = _legacy_to_dict(event) with pytest.raises(TypeError): - json.dumps(legacy) - - actual = event.to_dict() - assert actual != legacy - json.dumps(actual) + json.dumps(_legacy_to_dict(event)) + with pytest.raises(TypeError): + json.dumps(event.to_dict()) From e31bb48675a8084a62dd9f97f9d9a25f1ee11d03 Mon Sep 17 00:00:00 2001 From: Bernd Verst Date: Mon, 27 Jul 2026 13:55:59 -0700 Subject: [PATCH 4/6] Handle asdict's version-specific defaultdict behavior in tests `dataclasses.asdict` rebuilt every dict subclass with `type(obj)()` until 3.12, which `defaultdict` rejects with a TypeError; 3.12 added a branch forwarding `default_factory`. The legacy pipeline therefore raises for a `defaultdict` on 3.10 and 3.11 and succeeds from 3.12 on. The walker delegates to the running interpreter's `asdict`, so it already reproduces both behaviors exactly -- the parity test asserting they fail the same way passes on every version. Only the strict-equality test was wrong: it assumed the legacy pipeline always produces a value to compare against. It now skips when legacy cannot serialize the payload at all, leaving that case to the parity test. Adds a named regression test for the split, since hand-rolling `_asdict_inner`'s branches would succeed on 3.10 and 3.11 where the legacy pipeline raises, and that divergence would only surface on one CI matrix leg. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 6322dc7f-81ee-4d42-be2d-cef84cf62d15 --- .../durabletask/test_history_serialization.py | 32 ++++++++++++++++++- 1 file changed, 31 insertions(+), 1 deletion(-) diff --git a/tests/durabletask/test_history_serialization.py b/tests/durabletask/test_history_serialization.py index dbf7cfa5..92a2afc2 100644 --- a/tests/durabletask/test_history_serialization.py +++ b/tests/durabletask/test_history_serialization.py @@ -634,8 +634,17 @@ def test_exotic_payload_matches_legacy_two_pass_output( ) -> None: event = _state_event(payload) + try: + expected = _legacy_to_dict(event) + except Exception: # noqa: BLE001 - see the sibling parity test + # A few payloads cannot be serialized by ``asdict`` at all on + # some interpreters -- ``defaultdict`` before 3.12, say. There + # is no legacy value to compare against, so parity for those is + # asserted by ``test_exotic_payload_raises_the_same_way_as_legacy`` + # instead of being silently weakened here. + pytest.skip('legacy raises for this payload on this interpreter') + actual = event.to_dict() - expected = _legacy_to_dict(event) assert actual == expected # ``==`` alone would let a namedtuple compare equal to a plain @@ -657,6 +666,27 @@ def test_exotic_payload_raises_the_same_way_as_legacy( else: assert event.to_dict() == expected + def test_defaultdict_follows_the_running_interpreter(self) -> None: + """``asdict`` changed how it rebuilds ``defaultdict`` in 3.12. + + Earlier versions rebuilt every ``dict`` subclass with + ``type(obj)()``, which ``defaultdict`` rejects with a + ``TypeError``; 3.12 added a branch that forwards ``default_factory``. + Delegating to the running interpreter's ``asdict`` inherits whichever + applies, so this asserts the two agree rather than pinning one + version's answer -- hand-rolling ``_asdict_inner``'s branches would + succeed on 3.10 and 3.11 where the legacy pipeline raises. + """ + event = _state_event(defaultdict(int, {'a': 1})) + + try: + expected: Any = _legacy_to_dict(event) + except TypeError: + with pytest.raises(TypeError): + event.to_dict() + else: + assert event.to_dict() == expected + def test_dict_key_that_cannot_be_rebuilt_still_raises(self) -> None: """``asdict`` turned dataclass keys into unhashable dicts; keep that.""" event = _state_event({_FrozenKey('z'): 1}) From 15c29eb4bf1a9cb003eb7565b248e1d46b59a371 Mon Sep 17 00:00:00 2001 From: Bernd Verst Date: Mon, 27 Jul 2026 14:23:59 -0700 Subject: [PATCH 5/6] Tighten _to_serializable docs and drop casts that were not load-bearing The docstring promised a "JSON-safe structure", which the function does not guarantee: values reached through dict[str, Any] fields keep their original behavior, and a few shapes -- a tuple holding a datetime, for one -- were never JSON-encodable. Say that plainly rather than implying every output can be encoded. The list and dict branches test value_type rather than the value, so the value is never narrowed and iterating it directly is already clean under strict type checking. The casts there did nothing but call typing.cast on every container. The cast on the type lookup is load-bearing and now says so. type(value) is type[Unknown] to a checker because value is Any. Two details that look like noise are deliberate: the quoted annotation avoids building a throwaway GenericAlias per call, and the lookup stays type(value) rather than the cheaper value.__class__ because asdict dispatched on type(obj). Dispatching on __class__ would let an object claiming to be a str take the JSON-native fast path and be returned by reference, where the original pipeline deep-copied it. A regression test pins that, and fails if the swap is made. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 6322dc7f-81ee-4d42-be2d-cef84cf62d15 --- durabletask/history.py | 20 ++++++-- .../durabletask/test_history_serialization.py | 49 +++++++++++++++++++ 2 files changed, 66 insertions(+), 3 deletions(-) diff --git a/durabletask/history.py b/durabletask/history.py index e093dc2c..0001c644 100644 --- a/durabletask/history.py +++ b/durabletask/history.py @@ -389,7 +389,13 @@ def _legacy_compat(value: Any) -> Any: def _to_serializable(value: Any) -> Any: - """Recursively convert *value* into a JSON-safe structure. + """Recursively convert *value* into the form history export writes. + + Values the SDK itself produces convert to JSON-native types. Arbitrary + values can also arrive through ``dict[str, Any]`` fields, and those keep + whatever the original pipeline did with them -- which for a few shapes, + such as a tuple holding a ``datetime``, is not JSON-encodable. That is + preserved on purpose rather than fixed here; see :func:`_legacy_compat`. This walks dataclass instances directly instead of going through ``dataclasses.asdict``, which would deep-copy the whole event graph @@ -404,6 +410,14 @@ def _to_serializable(value: Any) -> Any: so their original semantics -- constructor round-trips, key recursion and deep-copied leaves -- are preserved exactly. """ + # ``type(value)`` is ``type[Unknown]`` to a type checker because *value* + # is ``Any``, so the cast is what keeps this module clean under strict + # checking. Two details are deliberate: the annotation is quoted, since + # an unquoted ``type[Any]`` is evaluated on every call and builds a + # throwaway ``types.GenericAlias``; and the lookup is ``type(value)`` + # rather than the cheaper ``value.__class__``, because ``asdict`` used + # ``type(obj)`` and an object overriding ``__class__`` would otherwise + # be dispatched differently than it was before. value_type = cast('type[Any]', type(value)) if value_type in _JSON_NATIVE_TYPES: return value @@ -417,7 +431,7 @@ def _to_serializable(value: Any) -> Any: if value_type is datetime: return value.isoformat() if value_type is list: - return [_to_serializable(item) for item in cast(list[Any], value)] + return [_to_serializable(item) for item in value] if value_type is dict: # ``asdict`` recursed into keys but the conversion pass that followed # it did not, so keys get ``asdict`` semantics only. Native keys are @@ -425,7 +439,7 @@ def _to_serializable(value: Any) -> Any: return { (key if type(key) in _JSON_NATIVE_TYPES else _asdict_only(key)): _to_serializable(item) - for key, item in cast(dict[Any, Any], value).items() + for key, item in value.items() } return _legacy_compat(value) diff --git a/tests/durabletask/test_history_serialization.py b/tests/durabletask/test_history_serialization.py index 92a2afc2..169968eb 100644 --- a/tests/durabletask/test_history_serialization.py +++ b/tests/durabletask/test_history_serialization.py @@ -582,6 +582,34 @@ def __eq__(self, other: object) -> bool: return isinstance(other, _DeepCopyKey) and other.name == self.name +class _ClassSpoofer: + """Reports a JSON-native ``__class__`` while being no such thing. + + ``asdict`` dispatched on ``type(obj)``, which cannot be overridden, so + this reached the deep-copy fallback. Anything that dispatches on + ``__class__`` instead would mistake it for a ``str`` and hand back the + original object untouched. + """ + + def __init__(self, tag: str) -> None: + self.tag = tag + + @property + def __class__(self) -> Any: # type: ignore[override] + return str + + def __eq__(self, other: object) -> bool: + # Compares on ``type`` rather than ``isinstance`` because the + # spoofed ``__class__`` makes ``isinstance`` checks ambiguous here. + return type(other) is _ClassSpoofer and other.tag == self.tag + + def __hash__(self) -> int: + return hash(self.tag) + + def __repr__(self) -> str: + return f'_ClassSpoofer({self.tag!r})' + + _EXOTIC_PAYLOADS = [ pytest.param(_OddIsoDatetime(2024, 1, 2, 3, 4, 5), id='datetime-subclass'), pytest.param( @@ -608,6 +636,7 @@ def __eq__(self, other: object) -> bool: pytest.param(bytearray(b'abc'), id='bytearray'), pytest.param(b'abc', id='bytes'), pytest.param(_MutableLeaf(1), id='custom-object'), + pytest.param(_ClassSpoofer('t'), id='class-spoofing-object'), ] @@ -770,6 +799,26 @@ def test_mapping_key_is_not_converted_by_the_value_walker(self) -> None: assert list(exported) == [_TS] + def test_dispatch_uses_type_not_the_overridable_class_attribute(self) -> None: + """Guards a tempting micro-optimization that would change behavior. + + ``value.__class__`` is measurably cheaper than ``type(value)`` and + satisfies a type checker without a cast, so it is an easy swap to + make. It is also wrong here: ``__class__`` can be overridden, and a + value claiming to be a ``str`` would take the JSON-native fast path + and be returned by reference. ``asdict`` dispatched on + ``type(obj)``, so it deep-copied this instead. + """ + spoofer = _ClassSpoofer('t') + event = _state_event(spoofer) + + exported = event.to_dict()['orchestration_state']['value'] + + assert exported is not spoofer, 'fast path taken on a spoofed __class__' + assert type(exported) is _ClassSpoofer + assert exported.tag == 't' + assert _legacy_to_dict(event)['orchestration_state']['value'] is not spoofer + class TestTupleHandling: """Tuples keep their original ``asdict`` semantics. From 47e0f72081646f24633f535a236e05f7383ee613 Mon Sep 17 00:00:00 2001 From: Bernd Verst Date: Mon, 27 Jul 2026 14:30:39 -0700 Subject: [PATCH 6/6] Rebuild mapping keys before converting any value asdict rebuilt a mapping's keys and collapsed any that compared equal afterwards, and only then did the conversion pass run. An entry that lost such a collision was therefore discarded before its value was ever converted. The single-pass walker fused those two phases, converting each value as it iterated, so it visited entries the original pipeline had already dropped. That is observable whenever converting the discarded value raises or has a side effect: with two keys that deep-copy to the same string and a datetime subclass whose isoformat raises, the original returns {'same-key': 'survivor'} while the walker propagated the exception. Only a key that asdict would rebuild can collide -- native keys pass through untouched and a mapping's keys are already distinct -- so the presence of one non-native key hands the whole mapping to the legacy path. The check completes before any value is converted, because bailing out partway would already have visited earlier entries. Mappings with native keys, which is every mapping the SDK produces, stay on the fast path; the speedup is unchanged at 6.2x. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 6322dc7f-81ee-4d42-be2d-cef84cf62d15 --- durabletask/history.py | 22 +++--- .../durabletask/test_history_serialization.py | 70 +++++++++++++++++++ 2 files changed, 84 insertions(+), 8 deletions(-) diff --git a/durabletask/history.py b/durabletask/history.py index 0001c644..a147bd0f 100644 --- a/durabletask/history.py +++ b/durabletask/history.py @@ -433,14 +433,20 @@ def _to_serializable(value: Any) -> Any: if value_type is list: return [_to_serializable(item) for item in value] if value_type is dict: - # ``asdict`` recursed into keys but the conversion pass that followed - # it did not, so keys get ``asdict`` semantics only. Native keys are - # returned as-is because ``asdict`` leaves those untouched too. - return { - (key if type(key) in _JSON_NATIVE_TYPES else _asdict_only(key)): - _to_serializable(item) - for key, item in value.items() - } + # ``asdict`` rebuilt keys and collapsed any that compared equal + # afterwards, and only then did the conversion pass run, so a value + # whose entry lost a collision was never converted. Converting + # inline would visit those dropped entries, which is observable if + # conversion raises or has side effects. Only a key that ``asdict`` + # would rebuild can collide -- native keys pass through untouched + # and a mapping's keys are already distinct -- so the presence of + # one is the signal to hand the whole mapping to the legacy path. + # This is checked before any value is converted, because bailing + # out partway would already have visited earlier entries. + for key in value: + if type(key) not in _JSON_NATIVE_TYPES: + return _legacy_compat(value) + return {key: _to_serializable(item) for key, item in value.items()} return _legacy_compat(value) diff --git a/tests/durabletask/test_history_serialization.py b/tests/durabletask/test_history_serialization.py index 169968eb..82c8a753 100644 --- a/tests/durabletask/test_history_serialization.py +++ b/tests/durabletask/test_history_serialization.py @@ -610,6 +610,34 @@ def __repr__(self) -> str: return f'_ClassSpoofer({self.tag!r})' +class _CollidingKey: + """Distinct keys that all deep-copy to the same value. + + ``asdict`` rebuilt keys before anything was converted, so entries that + collided after reconstruction were dropped and their values never + reached the conversion pass. + """ + + def __init__(self, name: str) -> None: + self.name = name + + def __deepcopy__(self, memo: dict[int, Any]) -> str: + return 'same-key' + + def __hash__(self) -> int: + return hash(self.name) + + def __eq__(self, other: object) -> bool: + return isinstance(other, _CollidingKey) and other.name == self.name + + +class _ThrowingIsoDatetime(datetime): + """A value that can be deep-copied but cannot be converted.""" + + def isoformat(self, *args: Any, **kwargs: Any) -> str: + raise ValueError('isoformat exploded') + + _EXOTIC_PAYLOADS = [ pytest.param(_OddIsoDatetime(2024, 1, 2, 3, 4, 5), id='datetime-subclass'), pytest.param( @@ -637,6 +665,17 @@ def __repr__(self) -> str: pytest.param(b'abc', id='bytes'), pytest.param(_MutableLeaf(1), id='custom-object'), pytest.param(_ClassSpoofer('t'), id='class-spoofing-object'), + pytest.param( + { + _CollidingKey('first'): _ThrowingIsoDatetime(2024, 1, 2, 3, 4, 5), + _CollidingKey('second'): 'survivor', + }, + id='colliding-keys-discard-a-throwing-value', + ), + pytest.param( + {_CollidingKey('a'): 1, _CollidingKey('b'): 2}, + id='colliding-keys-last-entry-wins', + ), ] @@ -819,6 +858,37 @@ def test_dispatch_uses_type_not_the_overridable_class_attribute(self) -> None: assert exported.tag == 't' assert _legacy_to_dict(event)['orchestration_state']['value'] is not spoofer + def test_colliding_keys_discard_their_value_before_conversion(self) -> None: + """Key reconstruction must finish before any value is converted. + + ``asdict`` rebuilt the mapping first, so the entry that lost the + collision disappeared and its value was never handed to the + conversion pass. Converting values inline, as they are iterated, + visits that discarded value instead -- which is observable here + because converting it raises. + """ + discarded = _ThrowingIsoDatetime(2024, 1, 2, 3, 4, 5) + event = _state_event({ + _CollidingKey('first'): discarded, + _CollidingKey('second'): 'survivor', + }) + + expected = _legacy_to_dict(event)['orchestration_state']['value'] + assert expected == {'same-key': 'survivor'} + + exported = event.to_dict()['orchestration_state']['value'] + + assert exported == expected + + def test_colliding_keys_keep_the_last_entry(self) -> None: + """The surviving value is the last one, as ``asdict`` left it.""" + event = _state_event({_CollidingKey('a'): 1, _CollidingKey('b'): 2}) + + exported = event.to_dict()['orchestration_state']['value'] + + assert exported == {'same-key': 2} + assert exported == _legacy_to_dict(event)['orchestration_state']['value'] + class TestTupleHandling: """Tuples keep their original ``asdict`` semantics.