From 480d8ae60e40e864df7e8c32a4d6e2df36572346 Mon Sep 17 00:00:00 2001 From: Benedikt Bartscher Date: Tue, 21 Jul 2026 12:58:32 +0200 Subject: [PATCH 1/7] improve compile perf --- .../src/reflex_base/components/component.py | 133 ++++++++++++++---- 1 file changed, 103 insertions(+), 30 deletions(-) diff --git a/packages/reflex-base/src/reflex_base/components/component.py b/packages/reflex-base/src/reflex_base/components/component.py index f0f7917eb59..6899b652cfb 100644 --- a/packages/reflex-base/src/reflex_base/components/component.py +++ b/packages/reflex-base/src/reflex_base/components/component.py @@ -610,59 +610,116 @@ def _hash_str(value: str) -> str: return md5(f'"{value}"'.encode(), usedforsecurity=False).hexdigest() -def _update_deterministic_hash(hasher: Any, value: object) -> None: - """Feed ``value`` into ``hasher`` using a self-delimiting, type-tagged encoding. +@functools.cache +def _deterministic_hash_dataclass_fields(cls: type) -> tuple[tuple[str, bytes], ...]: + """Per-class cache of dataclass field names and their encoded bytes. + + ``dataclasses.fields`` rebuilds its result tuple on every call; hashing a + large app calls it millions of times (mostly for ``VarData``), so cache + the derived (name, encoded name) pairs per class. + + Args: + cls: The dataclass type to introspect. + + Returns: + The (field name, encoded field name) pairs in definition order. + """ + return tuple((f.name, f.name.encode()) for f in dataclasses.fields(cls)) + + +def _encode_deterministic(buf: bytearray, value: object) -> None: + """Append ``value`` to ``buf`` using a self-delimiting, type-tagged encoding. Each branch writes a distinct type tag plus length-prefixed payload, which keeps the encoding injective without building intermediate strings — the nested ``str([...])`` approach this replaces was the dominant cost of ``_deterministic_hash`` (~4x speedup on synthetic, ~2x on real renders). + Exact-type checks front-run the isinstance ladder: auto-memoization hashes + hundreds of millions of values per compile, nearly all of them plain + ``str``/``dict``/``list``/``tuple`` nodes from rendered component dicts, + and the ladder's isinstance calls dominated the compile profile. Subclasses + fall through to the ladder, which keeps the original branch order so the + encoding is byte-identical to the pre-dispatch implementation. + Args: - hasher: A ``hashlib`` hasher (must accept ``.update(bytes)``). - value: The value to fold into the hasher. + buf: The output buffer to append to. + value: The value to fold into the buffer. Raises: TypeError: If the value is not hashable. """ + if type(value) is str: + encoded = value.encode() + buf += b"s" + buf += len(encoded).to_bytes(8, "little") + buf += encoded + return + if type(value) is dict: + items = sorted(value.items(), key=operator.itemgetter(0)) + buf += b"d" + buf += len(items).to_bytes(8, "little") + for k, v in items: + _encode_deterministic(buf, k) + _encode_deterministic(buf, v) + return + if type(value) is list or type(value) is tuple: + buf += b"l" + buf += len(value).to_bytes(8, "little") + for item in value: + _encode_deterministic(buf, item) + return if value is None: - hasher.update(b"N") - elif isinstance(value, bool): - hasher.update(b"T" if value else b"F") + buf += b"N" + return + if type(value) is bool: + buf += b"T" if value else b"F" + return + if type(value) is int or type(value) is float: + buf += b"n" + buf += str(value).encode() + return + # Slow path for subclasses and structured types, in the original ladder + # order so subclass encodings stay identical (e.g. ``IntEnum`` must hit + # the numeric branch before the dataclass branch would see it). + if isinstance(value, bool): + buf += b"T" if value else b"F" elif isinstance(value, (int, float, enum.Enum)): - hasher.update(b"n") - hasher.update(str(value).encode()) + buf += b"n" + buf += str(value).encode() elif isinstance(value, str): encoded = value.encode() - hasher.update(b"s") - hasher.update(len(encoded).to_bytes(8, "little")) - hasher.update(encoded) + buf += b"s" + buf += len(encoded).to_bytes(8, "little") + buf += encoded elif isinstance(value, dict): items = sorted(value.items(), key=operator.itemgetter(0)) - hasher.update(b"d") - hasher.update(len(items).to_bytes(8, "little")) + buf += b"d" + buf += len(items).to_bytes(8, "little") for k, v in items: - _update_deterministic_hash(hasher, k) - _update_deterministic_hash(hasher, v) + _encode_deterministic(buf, k) + _encode_deterministic(buf, v) elif isinstance(value, (tuple, list)): - hasher.update(b"l") - hasher.update(len(value).to_bytes(8, "little")) + buf += b"l" + buf += len(value).to_bytes(8, "little") for item in value: - _update_deterministic_hash(hasher, item) + _encode_deterministic(buf, item) elif isinstance(value, Var): - hasher.update(b"v") - _update_deterministic_hash(hasher, value._js_expr) - _update_deterministic_hash(hasher, value._get_all_var_data()) + buf += b"v" + _encode_deterministic(buf, value._js_expr) + _encode_deterministic(buf, value._get_all_var_data()) elif dataclasses.is_dataclass(value): - fields = dataclasses.fields(value) - hasher.update(b"D") - hasher.update(len(fields).to_bytes(8, "little")) - for field in fields: - hasher.update(field.name.encode()) - _update_deterministic_hash(hasher, getattr(value, field.name)) + fields = _deterministic_hash_dataclass_fields( + value if isinstance(value, type) else type(value) + ) + buf += b"D" + buf += len(fields).to_bytes(8, "little") + for field_name, encoded_field_name in fields: + buf += encoded_field_name + _encode_deterministic(buf, getattr(value, field_name)) elif isinstance(value, BaseComponent): - hasher.update(b"C") - _update_deterministic_hash(hasher, value.render()) + buf += b"C" + _encode_deterministic(buf, value.render()) else: msg = ( f"Cannot hash value `{value}` of type `{type(value).__name__}`. " @@ -671,6 +728,22 @@ def _update_deterministic_hash(hasher: Any, value: object) -> None: raise TypeError(msg) +def _update_deterministic_hash(hasher: Any, value: object) -> None: + """Feed ``value`` into ``hasher`` via :func:`_encode_deterministic`. + + Buffering the whole encoding and updating the hasher once replaces the + per-node ``hasher.update`` calls (hundreds of millions per compile) with + cheap bytearray appends. + + Args: + hasher: A ``hashlib`` hasher (must accept ``.update(bytes)``). + value: The value to fold into the hasher. + """ + buf = bytearray() + _encode_deterministic(buf, value) + hasher.update(buf) + + def _deterministic_hash(value: object) -> str: """Hash a rendered dictionary. From 77a5ad28ee50861929b9e16c124cd13cea31840a Mon Sep 17 00:00:00 2001 From: Benedikt Bartscher Date: Tue, 4 Aug 2026 20:32:28 +0200 Subject: [PATCH 2/7] refactor this. --- .../src/reflex_base/components/component.py | 247 +++++++++--------- tests/units/components/test_component.py | 107 +++++++- 2 files changed, 225 insertions(+), 129 deletions(-) diff --git a/packages/reflex-base/src/reflex_base/components/component.py b/packages/reflex-base/src/reflex_base/components/component.py index 6899b652cfb..6b23cb3963a 100644 --- a/packages/reflex-base/src/reflex_base/components/component.py +++ b/packages/reflex-base/src/reflex_base/components/component.py @@ -610,37 +610,99 @@ def _hash_str(value: str) -> str: return md5(f'"{value}"'.encode(), usedforsecurity=False).hexdigest() -@functools.cache -def _deterministic_hash_dataclass_fields(cls: type) -> tuple[tuple[str, bytes], ...]: - """Per-class cache of dataclass field names and their encoded bytes. +def _encode_str(buf: bytearray, value: str) -> None: + encoded = value.encode() + buf += b"s" + buf += len(encoded).to_bytes(8, "little") + buf += encoded - ``dataclasses.fields`` rebuilds its result tuple on every call; hashing a - large app calls it millions of times (mostly for ``VarData``), so cache - the derived (name, encoded name) pairs per class. - Args: - cls: The dataclass type to introspect. +def _encode_number(buf: bytearray, value: int | float | enum.Enum) -> None: + buf += b"n" + buf += str(value).encode() - Returns: - The (field name, encoded field name) pairs in definition order. - """ + +def _encode_dict(buf: bytearray, value: Mapping[Any, Any]) -> None: + items = sorted(value.items(), key=operator.itemgetter(0)) + buf += b"d" + buf += len(items).to_bytes(8, "little") + for k, v in items: + _encode_deterministic(buf, k) + _encode_deterministic(buf, v) + + +def _encode_sequence(buf: bytearray, value: Sequence[Any]) -> None: + buf += b"l" + buf += len(value).to_bytes(8, "little") + for item in value: + _encode_deterministic(buf, item) + + +def _encode_var(buf: bytearray, value: Var) -> None: + buf += b"v" + _encode_deterministic(buf, value._js_expr) + _encode_deterministic(buf, value._get_all_var_data()) + + +@functools.cache +def _dataclass_fields_to_encode(cls: type) -> tuple[tuple[str, bytes], ...]: + # dataclasses.fields rebuilds its result tuple on every call; hashing a + # large app calls it millions of times for a handful of classes. return tuple((f.name, f.name.encode()) for f in dataclasses.fields(cls)) -def _encode_deterministic(buf: bytearray, value: object) -> None: - """Append ``value`` to ``buf`` using a self-delimiting, type-tagged encoding. +def _encode_dataclass(buf: bytearray, value: Any) -> None: + fields = _dataclass_fields_to_encode( + value if isinstance(value, type) else type(value) + ) + buf += b"D" + buf += len(fields).to_bytes(8, "little") + for field_name, encoded_field_name in fields: + buf += encoded_field_name + _encode_deterministic(buf, getattr(value, field_name)) + + +def _encode_component(buf: bytearray, value: BaseComponent) -> None: + buf += b"C" + _encode_deterministic(buf, value.render()) + + +_ENCODERS: dict[type, Callable[[bytearray, Any], None]] = { + dict: _encode_dict, + list: _encode_sequence, + tuple: _encode_sequence, + int: _encode_number, + float: _encode_number, +} + - Each branch writes a distinct type tag plus length-prefixed payload, which - keeps the encoding injective without building intermediate strings — the - nested ``str([...])`` approach this replaces was the dominant cost of - ``_deterministic_hash`` (~4x speedup on synthetic, ~2x on real renders). +def _resolve_encoder(value: object) -> Callable[[bytearray, Any], None] | None: + # Branch order decides the encoding of values matching several branches + # (e.g. an IntEnum encodes as a number, not as a dataclass). + if isinstance(value, (int, float, enum.Enum)): + return _encode_number + if isinstance(value, str): + return _encode_str + if isinstance(value, dict): + return _encode_dict + if isinstance(value, (tuple, list)): + return _encode_sequence + if isinstance(value, Var): + return _encode_var + if dataclasses.is_dataclass(value): + return _encode_dataclass + if isinstance(value, BaseComponent): + return _encode_component + return None - Exact-type checks front-run the isinstance ladder: auto-memoization hashes - hundreds of millions of values per compile, nearly all of them plain - ``str``/``dict``/``list``/``tuple`` nodes from rendered component dicts, - and the ladder's isinstance calls dominated the compile profile. Subclasses - fall through to the ladder, which keeps the original branch order so the - encoding is byte-identical to the pre-dispatch implementation. + +def _encode_deterministic(buf: bytearray, value: object) -> None: + """Append ``value`` to ``buf`` in a self-delimiting, type-tagged encoding. + + Every type writes a distinct tag plus a length-prefixed payload, keeping the + encoding injective without building intermediate strings. Encoders are looked + up by exact type and memoized per type, since auto-memoization encodes + hundreds of millions of values per compile. Args: buf: The output buffer to append to. @@ -649,99 +711,33 @@ def _encode_deterministic(buf: bytearray, value: object) -> None: Raises: TypeError: If the value is not hashable. """ + # str, bool and None are the most common leaves by far, so they skip the + # table lookup (str inlines _encode_str). bool must come first because it + # would otherwise resolve to the numeric encoding. if type(value) is str: encoded = value.encode() buf += b"s" buf += len(encoded).to_bytes(8, "little") buf += encoded return - if type(value) is dict: - items = sorted(value.items(), key=operator.itemgetter(0)) - buf += b"d" - buf += len(items).to_bytes(8, "little") - for k, v in items: - _encode_deterministic(buf, k) - _encode_deterministic(buf, v) - return - if type(value) is list or type(value) is tuple: - buf += b"l" - buf += len(value).to_bytes(8, "little") - for item in value: - _encode_deterministic(buf, item) + value_type = type(value) + if value_type is bool: + buf += b"T" if value else b"F" return if value is None: buf += b"N" return - if type(value) is bool: - buf += b"T" if value else b"F" - return - if type(value) is int or type(value) is float: - buf += b"n" - buf += str(value).encode() - return - # Slow path for subclasses and structured types, in the original ladder - # order so subclass encodings stay identical (e.g. ``IntEnum`` must hit - # the numeric branch before the dataclass branch would see it). - if isinstance(value, bool): - buf += b"T" if value else b"F" - elif isinstance(value, (int, float, enum.Enum)): - buf += b"n" - buf += str(value).encode() - elif isinstance(value, str): - encoded = value.encode() - buf += b"s" - buf += len(encoded).to_bytes(8, "little") - buf += encoded - elif isinstance(value, dict): - items = sorted(value.items(), key=operator.itemgetter(0)) - buf += b"d" - buf += len(items).to_bytes(8, "little") - for k, v in items: - _encode_deterministic(buf, k) - _encode_deterministic(buf, v) - elif isinstance(value, (tuple, list)): - buf += b"l" - buf += len(value).to_bytes(8, "little") - for item in value: - _encode_deterministic(buf, item) - elif isinstance(value, Var): - buf += b"v" - _encode_deterministic(buf, value._js_expr) - _encode_deterministic(buf, value._get_all_var_data()) - elif dataclasses.is_dataclass(value): - fields = _deterministic_hash_dataclass_fields( - value if isinstance(value, type) else type(value) - ) - buf += b"D" - buf += len(fields).to_bytes(8, "little") - for field_name, encoded_field_name in fields: - buf += encoded_field_name - _encode_deterministic(buf, getattr(value, field_name)) - elif isinstance(value, BaseComponent): - buf += b"C" - _encode_deterministic(buf, value.render()) - else: - msg = ( - f"Cannot hash value `{value}` of type `{type(value).__name__}`. " - "Only BaseComponent, Var, VarData, dict, str, tuple, and enum.Enum are supported." - ) - raise TypeError(msg) - - -def _update_deterministic_hash(hasher: Any, value: object) -> None: - """Feed ``value`` into ``hasher`` via :func:`_encode_deterministic`. - - Buffering the whole encoding and updating the hasher once replaces the - per-node ``hasher.update`` calls (hundreds of millions per compile) with - cheap bytearray appends. - - Args: - hasher: A ``hashlib`` hasher (must accept ``.update(bytes)``). - value: The value to fold into the hasher. - """ - buf = bytearray() - _encode_deterministic(buf, value) - hasher.update(buf) + encoder = _ENCODERS.get(value_type) + if encoder is None: + encoder = _resolve_encoder(value) + if encoder is None: + msg = ( + f"Cannot hash value `{value}` of type `{value_type.__name__}`. " + "Only BaseComponent, Var, VarData, dict, str, tuple, and enum.Enum are supported." + ) + raise TypeError(msg) + _ENCODERS[value_type] = encoder + encoder(buf, value) def _deterministic_hash(value: object) -> str: @@ -752,13 +748,10 @@ def _deterministic_hash(value: object) -> str: Returns: The hash of the dictionary. - - Raises: - TypeError: If the value is not hashable. """ - hasher = md5(usedforsecurity=False) - _update_deterministic_hash(hasher, value) - return hasher.hexdigest() + buf = bytearray() + _encode_deterministic(buf, value) + return md5(buf, usedforsecurity=False).hexdigest() @dataclasses.dataclass(kw_only=True, frozen=True, slots=True) @@ -1583,25 +1576,23 @@ def _get_component_hash(self, shallow: bool = False) -> str: Returns: The hex digest content hash. """ - hasher = md5(usedforsecurity=False) - _update_deterministic_hash(hasher, self.render()) + buf = bytearray() + _encode_deterministic(buf, self.render()) if shallow: # For non-snapshot strategies, we only hash the component's own hooks, imports, custom code, and app-wrap components - _update_deterministic_hash(hasher, dict(self._get_imports())) - _update_deterministic_hash(hasher, dict(self._get_hooks_internal())) - _update_deterministic_hash(hasher, dict(self._get_added_hooks())) - _update_deterministic_hash(hasher, self._get_hooks()) - _update_deterministic_hash(hasher, self._get_custom_code()) - _update_deterministic_hash(hasher, dict(self._get_app_wrap_components())) + _encode_deterministic(buf, dict(self._get_imports())) + _encode_deterministic(buf, dict(self._get_hooks_internal())) + _encode_deterministic(buf, dict(self._get_added_hooks())) + _encode_deterministic(buf, self._get_hooks()) + _encode_deterministic(buf, self._get_custom_code()) + _encode_deterministic(buf, dict(self._get_app_wrap_components())) else: - _update_deterministic_hash(hasher, dict(self._get_all_imports())) - _update_deterministic_hash(hasher, dict(self._get_all_hooks_internal())) - _update_deterministic_hash(hasher, dict(self._get_all_hooks())) - _update_deterministic_hash(hasher, dict(self._get_all_custom_code())) - _update_deterministic_hash( - hasher, dict(self._get_all_app_wrap_components()) - ) - return hasher.hexdigest() + _encode_deterministic(buf, dict(self._get_all_imports())) + _encode_deterministic(buf, dict(self._get_all_hooks_internal())) + _encode_deterministic(buf, dict(self._get_all_hooks())) + _encode_deterministic(buf, dict(self._get_all_custom_code())) + _encode_deterministic(buf, dict(self._get_all_app_wrap_components())) + return md5(buf, usedforsecurity=False).hexdigest() def _compute_memo_tag(self) -> str: """Compute a stable tag name for memoizing this component. diff --git a/tests/units/components/test_component.py b/tests/units/components/test_component.py index 3325e11ac4f..248ddd41054 100644 --- a/tests/units/components/test_component.py +++ b/tests/units/components/test_component.py @@ -1,10 +1,12 @@ import copy +import enum +from collections import namedtuple from contextlib import nullcontext from dataclasses import dataclass from typing import Any, ClassVar, TypedDict import pytest -from reflex_base.components.component import Component, field +from reflex_base.components.component import Component, _deterministic_hash, field from reflex_base.constants import EventTriggers from reflex_base.constants.state import FIELD_MARKER from reflex_base.event import ( @@ -2341,3 +2343,106 @@ def test_get_all_hooks_internal_does_not_mutate_hooks_cache(): assert dict(parent._get_hooks_internal()) == parent_own_hooks # And repeated collection yields the same result. assert parent._get_all_hooks_internal() == combined + + +class _HashColor(enum.Enum): + RED = "red" + + +class _HashStr(str): + pass + + +class _HashDict(dict): + pass + + +class _HashList(list): + pass + + +_HashPoint = namedtuple("_HashPoint", ["x", "y"]) + + +@dataclass +class _HashDefaults: + a: int = 1 + + +def test_deterministic_hash_distinguishes_values(): + """Structurally different values must not collide.""" + values = [ + None, + True, + False, + 0, + 1, + 1.5, + "", + "1", + "true", + _HashColor.RED, + [], + [1], + [1, 2], + {}, + {"a": 1}, + {"a": "1"}, + {"a": {"b": 1}}, + ImportVar(tag="Foo"), + ImportVar(tag="Foo", is_default=True), + VarData(imports={"react": [ImportVar(tag="useState")]}), + Bare.create(contents="a"), + ] + hashes = [_deterministic_hash(value) for value in values] + assert len(set(hashes)) == len(values) + + +def test_deterministic_hash_ignores_dict_order(): + """Dicts with the same items hash the same regardless of insertion order.""" + assert _deterministic_hash({"a": 1, "b": [2, "3"]}) == _deterministic_hash({ + "b": [2, "3"], + "a": 1, + }) + + +def test_deterministic_hash_normalizes_subclasses(): + """Subclasses hash like the built-in type they encode as.""" + assert _deterministic_hash(_HashStr("x")) == _deterministic_hash("x") + assert _deterministic_hash(_HashDict({"a": 1})) == _deterministic_hash({"a": 1}) + assert _deterministic_hash(_HashList([1, 2])) == _deterministic_hash([1, 2]) + assert _deterministic_hash(_HashPoint(1, 2)) == _deterministic_hash((1, 2)) + # A bool is an int subclass, but must not encode as a number. + assert _deterministic_hash(True) != _deterministic_hash(1) + + +def test_deterministic_hash_vars_include_var_data(): + """Vars with the same JS expression but different data hash differently.""" + plain = Var(_js_expr="foo") + with_data = Var( + _js_expr="foo", + _var_data=VarData(imports={"react": [ImportVar(tag="useState")]}), + ) + assert _deterministic_hash(plain) != _deterministic_hash(with_data) + + +def test_deterministic_hash_rejects_unsupported_values(): + """Unsupported values raise, and the failure is not cached for other types.""" + with pytest.raises(TypeError): + _deterministic_hash(object()) + # Dataclass types are supported (their fields are read off the class). + assert _deterministic_hash(_HashDefaults) == _deterministic_hash(_HashDefaults) + with pytest.raises(TypeError): + _deterministic_hash(_HashStr) + + +def test_component_hash_includes_lifecycle_hooks(): + """Components differing only in on_mount must not share a hash.""" + plain = Box.create(id="hash_box") + with_mount = Box.create(id="hash_box", on_mount=rx.console_log("mounted")) + + assert plain.render() == with_mount.render() + assert plain._get_component_hash() != with_mount._get_component_hash() + assert plain._get_component_hash(shallow=True) != with_mount._get_component_hash( + shallow=True + ) From 104773cd66be09dc35cb2deaae89ffb7fde3ca06 Mon Sep 17 00:00:00 2001 From: Benedikt Bartscher Date: Tue, 4 Aug 2026 21:16:06 +0200 Subject: [PATCH 3/7] be fast --- .../src/reflex_base/components/component.py | 86 ++++++++++++++----- tests/units/components/test_component.py | 60 +++++++++++++ 2 files changed, 125 insertions(+), 21 deletions(-) diff --git a/packages/reflex-base/src/reflex_base/components/component.py b/packages/reflex-base/src/reflex_base/components/component.py index 6b23cb3963a..fa426bc86fc 100644 --- a/packages/reflex-base/src/reflex_base/components/component.py +++ b/packages/reflex-base/src/reflex_base/components/component.py @@ -662,6 +662,36 @@ def _encode_dataclass(buf: bytearray, value: Any) -> None: _encode_deterministic(buf, getattr(value, field_name)) +_IMMUTABLE_FIELD_TYPES = (str, bool, int, float, type(None)) +_MAX_ENCODED_DATACLASSES = 8192 +# Encodings of frozen dataclass instances whose fields are all immutable +# scalars, keyed by ``id``. Each entry keeps the instance alive, so its id +# cannot be reused while cached and a lookup hit is always the same object, +# whose encoding can never have changed. +_ENCODED_DATACLASSES: dict[int, tuple[object, bytes]] = {} + + +def _encode_frozen_dataclass(buf: bytearray, value: Any) -> None: + entry = _ENCODED_DATACLASSES.get(id(value)) + if entry is not None: + buf += entry[1] + return + start = len(buf) + _encode_dataclass(buf, value) + value_type = type(value) + if all( + type(getattr(value, field_name)) in _IMMUTABLE_FIELD_TYPES + for field_name, _ in _dataclass_fields_to_encode(value_type) + ): + if len(_ENCODED_DATACLASSES) >= _MAX_ENCODED_DATACLASSES: + _ENCODED_DATACLASSES.clear() + _ENCODED_DATACLASSES[id(value)] = (value, bytes(buf[start:])) + else: + # A field holds something mutable (or a Var, dict, component, ...), so + # this class is never cacheable: stop paying for the check. + _ENCODERS[value_type] = _encode_dataclass + + def _encode_component(buf: bytearray, value: BaseComponent) -> None: buf += b"C" _encode_deterministic(buf, value.render()) @@ -690,6 +720,8 @@ def _resolve_encoder(value: object) -> Callable[[bytearray, Any], None] | None: if isinstance(value, Var): return _encode_var if dataclasses.is_dataclass(value): + if not isinstance(value, type) and type(value).__dataclass_params__.frozen: # pyright: ignore[reportAttributeAccessIssue] + return _encode_frozen_dataclass return _encode_dataclass if isinstance(value, BaseComponent): return _encode_component @@ -740,18 +772,28 @@ def _encode_deterministic(buf: bytearray, value: object) -> None: encoder(buf, value) -def _deterministic_hash(value: object) -> str: - """Hash a rendered dictionary. +def _deterministic_hash(*values: object) -> str: + """Hash values into a single digest, in the order given. + + Encoding into a buffer instead of feeding the hasher node by node is what + makes hashing cheap, at the cost of holding one value's encoding in memory + (a few MB for a large page). Each value is flushed into the hasher before + the next one is encoded, so peak memory stays at the largest single value + rather than their sum. Args: - value: The dictionary to hash. + *values: The values to hash. Returns: - The hash of the dictionary. + The hex digest over all values. """ + hasher = md5(usedforsecurity=False) buf = bytearray() - _encode_deterministic(buf, value) - return md5(buf, usedforsecurity=False).hexdigest() + for value in values: + _encode_deterministic(buf, value) + hasher.update(buf) + buf.clear() + return hasher.hexdigest() @dataclasses.dataclass(kw_only=True, frozen=True, slots=True) @@ -1576,23 +1618,25 @@ def _get_component_hash(self, shallow: bool = False) -> str: Returns: The hex digest content hash. """ - buf = bytearray() - _encode_deterministic(buf, self.render()) if shallow: # For non-snapshot strategies, we only hash the component's own hooks, imports, custom code, and app-wrap components - _encode_deterministic(buf, dict(self._get_imports())) - _encode_deterministic(buf, dict(self._get_hooks_internal())) - _encode_deterministic(buf, dict(self._get_added_hooks())) - _encode_deterministic(buf, self._get_hooks()) - _encode_deterministic(buf, self._get_custom_code()) - _encode_deterministic(buf, dict(self._get_app_wrap_components())) - else: - _encode_deterministic(buf, dict(self._get_all_imports())) - _encode_deterministic(buf, dict(self._get_all_hooks_internal())) - _encode_deterministic(buf, dict(self._get_all_hooks())) - _encode_deterministic(buf, dict(self._get_all_custom_code())) - _encode_deterministic(buf, dict(self._get_all_app_wrap_components())) - return md5(buf, usedforsecurity=False).hexdigest() + return _deterministic_hash( + self.render(), + dict(self._get_imports()), + dict(self._get_hooks_internal()), + dict(self._get_added_hooks()), + self._get_hooks(), + self._get_custom_code(), + dict(self._get_app_wrap_components()), + ) + return _deterministic_hash( + self.render(), + dict(self._get_all_imports()), + dict(self._get_all_hooks_internal()), + dict(self._get_all_hooks()), + dict(self._get_all_custom_code()), + dict(self._get_all_app_wrap_components()), + ) def _compute_memo_tag(self) -> str: """Compute a stable tag name for memoizing this component. diff --git a/tests/units/components/test_component.py b/tests/units/components/test_component.py index 248ddd41054..3e62bdbf531 100644 --- a/tests/units/components/test_component.py +++ b/tests/units/components/test_component.py @@ -6,6 +6,7 @@ from typing import Any, ClassVar, TypedDict import pytest +from reflex_base.components import component from reflex_base.components.component import Component, _deterministic_hash, field from reflex_base.constants import EventTriggers from reflex_base.constants.state import FIELD_MARKER @@ -2446,3 +2447,62 @@ def test_component_hash_includes_lifecycle_hooks(): assert plain._get_component_hash(shallow=True) != with_mount._get_component_hash( shallow=True ) + + +@dataclass(frozen=True) +class _HashFrozenScalars: + a: int | bool + b: str = "x" + + +@dataclass(frozen=True) +class _HashFrozenContainer: + items: list[int] + + +@dataclass +class _HashMutable: + a: int + + +def test_deterministic_hash_caches_frozen_dataclasses_by_identity(): + """Cached and freshly encoded instances of the same content hash the same.""" + shared = ImportVar(tag="Shared") + equal = ImportVar(tag="Shared") + + # First encode populates the cache, the second must reuse it, and an equal + # but distinct instance must encode to the same bytes either way. + assert _deterministic_hash([shared, shared]) == _deterministic_hash([shared, equal]) + assert _deterministic_hash([equal, shared]) == _deterministic_hash([shared, shared]) + + +def test_deterministic_hash_never_conflates_equal_but_differently_typed_fields(): + """``True`` and ``1`` compare equal but must never share an encoding.""" + assert _deterministic_hash(_HashFrozenScalars(a=True)) != _deterministic_hash( + _HashFrozenScalars(a=1) + ) + + +def test_deterministic_hash_tracks_mutation_of_uncacheable_dataclasses(): + """Dataclasses that can still change must be re-encoded every time.""" + mutable = _HashMutable(a=1) + before = _deterministic_hash(mutable) + mutable.a = 2 + assert _deterministic_hash(mutable) != before + + # Frozen, but a field holds a mutable container. + container = _HashFrozenContainer(items=[1]) + before = _deterministic_hash(container) + container.items.append(2) + assert _deterministic_hash(container) != before + + +def test_deterministic_hash_survives_encoding_cache_eviction(monkeypatch): + """Evicting the encoding cache must not change any digest.""" + values = [ImportVar(tag=f"Evict{index}") for index in range(32)] + expected = [_deterministic_hash(value) for value in values] + + monkeypatch.setattr(component, "_MAX_ENCODED_DATACLASSES", 4) + component._ENCODED_DATACLASSES.clear() + assert [_deterministic_hash(value) for value in values] == expected + assert len(component._ENCODED_DATACLASSES) <= 4 From 19861aa87280186c057413e2f4f4e8f56c580acc Mon Sep 17 00:00:00 2001 From: Benedikt Bartscher Date: Tue, 4 Aug 2026 21:58:45 +0200 Subject: [PATCH 4/7] review and changelog --- packages/reflex-base/news/6804.performance.md | 1 + .../src/reflex_base/components/component.py | 18 +++++--- tests/units/components/test_component.py | 45 +++++++++++++++++-- 3 files changed, 54 insertions(+), 10 deletions(-) create mode 100644 packages/reflex-base/news/6804.performance.md diff --git a/packages/reflex-base/news/6804.performance.md b/packages/reflex-base/news/6804.performance.md new file mode 100644 index 00000000000..0a11cd4e84c --- /dev/null +++ b/packages/reflex-base/news/6804.performance.md @@ -0,0 +1 @@ +Speed up the content hash behind compiler auto-memoization: values are encoded into a buffer through a per-type encoder table resolved once per type, and the encodings of frozen dataclasses with immutable fields (such as `ImportVar`) are reused by object identity. Hashing a large page's component tree is roughly 3.7x faster, and the resulting hashes are unchanged. diff --git a/packages/reflex-base/src/reflex_base/components/component.py b/packages/reflex-base/src/reflex_base/components/component.py index fa426bc86fc..f5e457ab0fa 100644 --- a/packages/reflex-base/src/reflex_base/components/component.py +++ b/packages/reflex-base/src/reflex_base/components/component.py @@ -10,6 +10,7 @@ import operator import typing from abc import ABC, ABCMeta, abstractmethod +from collections import OrderedDict from collections.abc import Callable, Iterable, Iterator, Mapping, Sequence from dataclasses import _MISSING_TYPE, MISSING from hashlib import md5 @@ -664,11 +665,15 @@ def _encode_dataclass(buf: bytearray, value: Any) -> None: _IMMUTABLE_FIELD_TYPES = (str, bool, int, float, type(None)) _MAX_ENCODED_DATACLASSES = 8192 +_MAX_ENCODED_DATACLASS_SIZE = 512 # Encodings of frozen dataclass instances whose fields are all immutable -# scalars, keyed by ``id``. Each entry keeps the instance alive, so its id +# scalars, keyed by ``id``. Each entry keeps its instance alive, so an id # cannot be reused while cached and a lookup hit is always the same object, -# whose encoding can never have changed. -_ENCODED_DATACLASSES: dict[int, tuple[object, bytes]] = {} +# whose encoding can never have changed. Retention is bounded to +# _MAX_ENCODED_DATACLASSES entries of at most _MAX_ENCODED_DATACLASS_SIZE +# bytes each, evicted oldest-first so a working set past the cap degrades +# entry by entry instead of being dropped wholesale. +_ENCODED_DATACLASSES: OrderedDict[int, tuple[object, bytes]] = OrderedDict() def _encode_frozen_dataclass(buf: bytearray, value: Any) -> None: @@ -683,9 +688,10 @@ def _encode_frozen_dataclass(buf: bytearray, value: Any) -> None: type(getattr(value, field_name)) in _IMMUTABLE_FIELD_TYPES for field_name, _ in _dataclass_fields_to_encode(value_type) ): - if len(_ENCODED_DATACLASSES) >= _MAX_ENCODED_DATACLASSES: - _ENCODED_DATACLASSES.clear() - _ENCODED_DATACLASSES[id(value)] = (value, bytes(buf[start:])) + if len(buf) - start <= _MAX_ENCODED_DATACLASS_SIZE: + if len(_ENCODED_DATACLASSES) >= _MAX_ENCODED_DATACLASSES: + _ENCODED_DATACLASSES.popitem(last=False) + _ENCODED_DATACLASSES[id(value)] = (value, bytes(buf[start:])) else: # A field holds something mutable (or a Var, dict, component, ...), so # this class is never cacheable: stop paying for the check. diff --git a/tests/units/components/test_component.py b/tests/units/components/test_component.py index 3e62bdbf531..3a2225a54a3 100644 --- a/tests/units/components/test_component.py +++ b/tests/units/components/test_component.py @@ -2465,17 +2465,54 @@ class _HashMutable: a: int -def test_deterministic_hash_caches_frozen_dataclasses_by_identity(): - """Cached and freshly encoded instances of the same content hash the same.""" +def test_deterministic_hash_reuses_frozen_dataclass_encoding(monkeypatch): + """A frozen scalar dataclass is encoded once and then reused by identity.""" + shared = ImportVar(tag="Shared") + component._ENCODED_DATACLASSES.clear() + + digest = _deterministic_hash(shared) + assert id(shared) in component._ENCODED_DATACLASSES + + def unreachable(buf: bytearray, value: object) -> None: + pytest.fail("cached encoding was re-encoded instead of reused") + + monkeypatch.setattr(component, "_encode_dataclass", unreachable) + assert _deterministic_hash(shared) == digest + + +def test_deterministic_hash_matches_uncached_encoding(): + """A cached encoding must equal a freshly encoded, equal instance.""" shared = ImportVar(tag="Shared") equal = ImportVar(tag="Shared") - # First encode populates the cache, the second must reuse it, and an equal - # but distinct instance must encode to the same bytes either way. assert _deterministic_hash([shared, shared]) == _deterministic_hash([shared, equal]) assert _deterministic_hash([equal, shared]) == _deterministic_hash([shared, shared]) +def test_encoding_cache_evicts_only_the_oldest_entry(monkeypatch): + """Passing the cap drops the oldest entry, not the whole working set.""" + monkeypatch.setattr(component, "_MAX_ENCODED_DATACLASSES", 2) + component._ENCODED_DATACLASSES.clear() + values = [ImportVar(tag=f"Evict{index}") for index in range(3)] + digests = [_deterministic_hash(value) for value in values] + + assert len(component._ENCODED_DATACLASSES) == 2 + assert id(values[0]) not in component._ENCODED_DATACLASSES + assert id(values[1]) in component._ENCODED_DATACLASSES + assert id(values[2]) in component._ENCODED_DATACLASSES + assert [_deterministic_hash(value) for value in values] == digests + + +def test_encoding_cache_skips_oversized_encodings(): + """Outsized encodings are not retained, keeping the cache's memory bounded.""" + component._ENCODED_DATACLASSES.clear() + oversized = ImportVar(tag="x" * (component._MAX_ENCODED_DATACLASS_SIZE + 1)) + + digest = _deterministic_hash(oversized) + assert id(oversized) not in component._ENCODED_DATACLASSES + assert _deterministic_hash(oversized) == digest + + def test_deterministic_hash_never_conflates_equal_but_differently_typed_fields(): """``True`` and ``1`` compare equal but must never share an encoding.""" assert _deterministic_hash(_HashFrozenScalars(a=True)) != _deterministic_hash( From 39d590cd49ee093644f38bf68f7845c18dd4cfc2 Mon Sep 17 00:00:00 2001 From: Benedikt Bartscher Date: Fri, 21 Aug 2026 00:18:09 +0200 Subject: [PATCH 5/7] review --- packages/reflex-base/news/6804.performance.md | 2 +- .../src/reflex_base/components/component.py | 24 +++++-- tests/units/components/test_component.py | 67 ++++++++++++++++--- 3 files changed, 74 insertions(+), 19 deletions(-) diff --git a/packages/reflex-base/news/6804.performance.md b/packages/reflex-base/news/6804.performance.md index 0a11cd4e84c..34206f27b78 100644 --- a/packages/reflex-base/news/6804.performance.md +++ b/packages/reflex-base/news/6804.performance.md @@ -1 +1 @@ -Speed up the content hash behind compiler auto-memoization: values are encoded into a buffer through a per-type encoder table resolved once per type, and the encodings of frozen dataclasses with immutable fields (such as `ImportVar`) are reused by object identity. Hashing a large page's component tree is roughly 3.7x faster, and the resulting hashes are unchanged. +Speed up the content hash behind compiler auto-memoization: values are encoded into a buffer through a per-type encoder table resolved once per type, and the encodings of frozen dataclasses with immutable fields (such as `ImportVar`) are reused by object identity. Hashing a large page's component tree is roughly 3.7x faster, and the resulting hashes are unchanged. Components that also inherit a dataclass (`rx.text` and friends, via `MarkdownComponentMap`) are now hashed by their rendered content instead of collapsing to their empty field list. diff --git a/packages/reflex-base/src/reflex_base/components/component.py b/packages/reflex-base/src/reflex_base/components/component.py index 9a74c98fb6e..23edf6284e5 100644 --- a/packages/reflex-base/src/reflex_base/components/component.py +++ b/packages/reflex-base/src/reflex_base/components/component.py @@ -717,8 +717,10 @@ def _encode_component(buf: bytearray, value: BaseComponent) -> None: def _resolve_encoder(value: object) -> Callable[[bytearray, Any], None] | None: - # Branch order decides the encoding of values matching several branches - # (e.g. an IntEnum encodes as a number, not as a dataclass). + # Branch order decides the encoding of values matching several branches: an + # IntEnum encodes as a number rather than a dataclass, and a component that + # also inherits a dataclass (MarkdownComponentMap) encodes as a component + # rather than as its own, usually empty, field list. if isinstance(value, (int, float, enum.Enum)): return _encode_number if isinstance(value, str): @@ -729,12 +731,17 @@ def _resolve_encoder(value: object) -> Callable[[bytearray, Any], None] | None: return _encode_sequence if isinstance(value, Var): return _encode_var - if dataclasses.is_dataclass(value): - if not isinstance(value, type) and type(value).__dataclass_params__.frozen: # pyright: ignore[reportAttributeAccessIssue] - return _encode_frozen_dataclass - return _encode_dataclass if isinstance(value, BaseComponent): return _encode_component + if dataclasses.is_dataclass(value): + if not isinstance(value, type): + # is_dataclass only tests for __dataclass_fields__, which classes + # synthesized at runtime (MutableProxy) copy over without the + # decorator's params. + params = getattr(type(value), "__dataclass_params__", None) + if params is not None and params.frozen: + return _encode_frozen_dataclass + return _encode_dataclass return None @@ -778,7 +785,10 @@ def _encode_deterministic(buf: bytearray, value: object) -> None: "Only BaseComponent, Var, VarData, dict, str, tuple, and enum.Enum are supported." ) raise TypeError(msg) - _ENCODERS[value_type] = encoder + if not isinstance(value, type): + # value_type is the metaclass when the value is itself a class, so + # memoizing would route every other class through this encoder. + _ENCODERS[value_type] = encoder encoder(buf, value) diff --git a/tests/units/components/test_component.py b/tests/units/components/test_component.py index 3a2225a54a3..f23ca94e47a 100644 --- a/tests/units/components/test_component.py +++ b/tests/units/components/test_component.py @@ -2370,6 +2370,23 @@ class _HashDefaults: a: int = 1 +@pytest.fixture +def encoding_caches(): + """Isolate the module-level encoder tables from the rest of the suite. + + Yields: + None, once the encoding caches are empty. + """ + encoders = component._ENCODERS.copy() + encoded = component._ENCODED_DATACLASSES.copy() + component._ENCODED_DATACLASSES.clear() + yield + component._ENCODERS.clear() + component._ENCODERS.update(encoders) + component._ENCODED_DATACLASSES.clear() + component._ENCODED_DATACLASSES.update(encoded) + + def test_deterministic_hash_distinguishes_values(): """Structurally different values must not collide.""" values = [ @@ -2427,16 +2444,43 @@ def test_deterministic_hash_vars_include_var_data(): assert _deterministic_hash(plain) != _deterministic_hash(with_data) -def test_deterministic_hash_rejects_unsupported_values(): +def test_deterministic_hash_rejects_unsupported_values(encoding_caches): """Unsupported values raise, and the failure is not cached for other types.""" - with pytest.raises(TypeError): + with pytest.raises(TypeError, match="Cannot hash value"): _deterministic_hash(object()) - # Dataclass types are supported (their fields are read off the class). + # Dataclass types are supported (their fields are read off the class), and + # must not install an encoder under their shared metaclass. assert _deterministic_hash(_HashDefaults) == _deterministic_hash(_HashDefaults) - with pytest.raises(TypeError): + with pytest.raises(TypeError, match="Cannot hash value"): _deterministic_hash(_HashStr) +def test_deterministic_hash_encodes_dataclass_components_as_components(): + """Components inheriting a dataclass encode their render, not their fields.""" + # rx.text inherits MarkdownComponentMap, a dataclass with no encodable + # fields, so a dataclass-first branch order collapses every instance to the + # same bytes. + assert _deterministic_hash(rx.text("a")) != _deterministic_hash(rx.text("b")) + + +def test_deterministic_hash_handles_dataclasses_without_params(encoding_caches): + """Runtime-synthesized dataclass subclasses hash like the class they copy.""" + # MutableProxy builds classes exactly this way: dataclasses.is_dataclass is + # true, but __dataclass_params__ never gets copied along. + synthesized = type( + "_HashSynthesized", + (), + { + "__dataclass_fields__": _HashFrozenScalars.__dataclass_fields__, + "a": 1, + "b": "x", + }, + ) + assert _deterministic_hash(synthesized()) == _deterministic_hash( + _HashFrozenScalars(a=1) + ) + + def test_component_hash_includes_lifecycle_hooks(): """Components differing only in on_mount must not share a hash.""" plain = Box.create(id="hash_box") @@ -2465,10 +2509,11 @@ class _HashMutable: a: int -def test_deterministic_hash_reuses_frozen_dataclass_encoding(monkeypatch): +def test_deterministic_hash_reuses_frozen_dataclass_encoding( + monkeypatch, encoding_caches +): """A frozen scalar dataclass is encoded once and then reused by identity.""" shared = ImportVar(tag="Shared") - component._ENCODED_DATACLASSES.clear() digest = _deterministic_hash(shared) assert id(shared) in component._ENCODED_DATACLASSES @@ -2489,10 +2534,9 @@ def test_deterministic_hash_matches_uncached_encoding(): assert _deterministic_hash([equal, shared]) == _deterministic_hash([shared, shared]) -def test_encoding_cache_evicts_only_the_oldest_entry(monkeypatch): +def test_encoding_cache_evicts_only_the_oldest_entry(monkeypatch, encoding_caches): """Passing the cap drops the oldest entry, not the whole working set.""" monkeypatch.setattr(component, "_MAX_ENCODED_DATACLASSES", 2) - component._ENCODED_DATACLASSES.clear() values = [ImportVar(tag=f"Evict{index}") for index in range(3)] digests = [_deterministic_hash(value) for value in values] @@ -2503,9 +2547,8 @@ def test_encoding_cache_evicts_only_the_oldest_entry(monkeypatch): assert [_deterministic_hash(value) for value in values] == digests -def test_encoding_cache_skips_oversized_encodings(): +def test_encoding_cache_skips_oversized_encodings(encoding_caches): """Outsized encodings are not retained, keeping the cache's memory bounded.""" - component._ENCODED_DATACLASSES.clear() oversized = ImportVar(tag="x" * (component._MAX_ENCODED_DATACLASS_SIZE + 1)) digest = _deterministic_hash(oversized) @@ -2534,7 +2577,9 @@ def test_deterministic_hash_tracks_mutation_of_uncacheable_dataclasses(): assert _deterministic_hash(container) != before -def test_deterministic_hash_survives_encoding_cache_eviction(monkeypatch): +def test_deterministic_hash_survives_encoding_cache_eviction( + monkeypatch, encoding_caches +): """Evicting the encoding cache must not change any digest.""" values = [ImportVar(tag=f"Evict{index}") for index in range(32)] expected = [_deterministic_hash(value) for value in values] From 60d48a3a7938364ca39405a3cab6b08b58dbeea3 Mon Sep 17 00:00:00 2001 From: Benedikt Bartscher Date: Fri, 21 Aug 2026 00:38:00 +0200 Subject: [PATCH 6/7] cleanup --- tests/units/components/test_component.py | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/tests/units/components/test_component.py b/tests/units/components/test_component.py index f23ca94e47a..c9197e45c6a 100644 --- a/tests/units/components/test_component.py +++ b/tests/units/components/test_component.py @@ -2372,13 +2372,10 @@ class _HashDefaults: @pytest.fixture def encoding_caches(): - """Isolate the module-level encoder tables from the rest of the suite. - - Yields: - None, once the encoding caches are empty. - """ + # Isolate the module-level encoder tables from the rest of the suite. encoders = component._ENCODERS.copy() encoded = component._ENCODED_DATACLASSES.copy() + component._ENCODERS.clear() component._ENCODED_DATACLASSES.clear() yield component._ENCODERS.clear() From b51acfcdacc42f60656f36127f86a722be17a819 Mon Sep 17 00:00:00 2001 From: Khaleel Al-Adhami Date: Wed, 26 Aug 2026 13:42:25 -0700 Subject: [PATCH 7/7] ENG-11237 feat(hosting-cli): report why a deploy failed, not just that it did (#6948) * ENG-11237 feat(hosting-cli): report why a deploy failed, not just that it did The watch loop decided everything by substring against a bare status string, and a build failure printed two warnings: the raw status, and an unconditional pointer at `reflex cloud apps build-logs`. A generic failure printed the status alone and nothing else. That pointer was unconditional because there was nothing to condition it on. The server classifies every failure as the app's, the platform's, or transient, but that classification never reached a client -- so a failure in the build pipeline arrived dressed as a build failure and sent people looking for a bug in an app that did not have one. The failure arms now fetch GET /deployments/{id}/failure and print the recorded reason, the guidance for that fault, and the end of the build log when the code is one the log explains. The excerpt goes through console.print(markup=False): it is raw build output, and rich would read its paths and version specifiers as markup. Every way of not getting an answer is one case -- a server predating the endpoint 404s, an older self-hosted one may not route it, the network may be down -- and all three fall back to exactly what the arm printed before, so a new CLI against an older server is unchanged. * Strip terminal controls from the excerpt, and file the news fragment per package Two fixes from review. The excerpt is raw build output -- the user's own dependencies and build scripts -- and it is now printed without anyone asking, on any failed deploy, where before it took an explicit `reflex cloud apps build-logs`. markup=False stops rich reading the text as its own markup and does nothing about escape sequences, so OSC 52 could write the reader's clipboard, OSC 8 could render one destination and link to another, and CSI could erase the lines above it and leave "build succeeded" on screen. Colour is not worth carrying for output shown unsolicited. The changelog job runs towncrier per affected package, so a fragment for a change under packages/reflex-hosting-cli/src has to live in that package's own news directory, not the repository root's. * Widen the escape class, and let a malformed answer fall back like any other Two review findings, both narrow and both real. The two-character escape class covered ESC + 0x40-0x5F, so a sequence whose final byte falls outside it -- `\x1b7` (DECSC), `\x1bc` (a full terminal reset) -- had its ESC removed by the bare-control catch-all and printed the final byte as a stray character. Inert, since the ESC is what drives the terminal, but it is garbage in an excerpt whose whole job is to be read. The general ECMA-48 shape covers them. `response.json()` raises UnicodeDecodeError on a 2xx body in an encoding httpx cannot decode, and that is a ValueError rather than a JSONDecodeError, so it escaped the fallback and would have ended the watch over a malformed answer to a request whose contract is that not getting one costs nothing. The excerpt's type is checked for the same reason: the CLI ships apart from the control plane and talks to self-hosted ones. * Report a build log the server could not read, rather than passing over it The failure endpoint now separates an unreadable log from a build that stored none. Collapsing the two tells somebody their build produced no log when the store was simply down, so the two get different answers here. * Assert the no-log path offers no build log, not just no outage message The test claimed the reason stands alone and only checked the outage wording. Offering the command is what separates this path from the unreadable one, so that is what has to be absent. Verified by mutation: forcing the offer fails this test and nothing else. * Fall back however the failure body is malformed RecursionError is a RuntimeError, so a deeply nested document escaped the ValueError catch and aborted the deploy watch -- over an answer this function is contracted to treat as no answer at all. Parametrized with the UnicodeDecodeError case, since they are one rule. --- .../reflex-hosting-cli/news/6948.feature.md | 1 + .../src/reflex_cli/utils/hosting.py | 150 ++++++- tests/units/reflex_cli/utils/test_hosting.py | 387 ++++++++++++++++++ 3 files changed, 534 insertions(+), 4 deletions(-) create mode 100644 packages/reflex-hosting-cli/news/6948.feature.md diff --git a/packages/reflex-hosting-cli/news/6948.feature.md b/packages/reflex-hosting-cli/news/6948.feature.md new file mode 100644 index 00000000000..b7f4da88e7a --- /dev/null +++ b/packages/reflex-hosting-cli/news/6948.feature.md @@ -0,0 +1 @@ +`reflex cloud deploy` now reports why a deploy failed instead of exiting on a status string: the recorded reason, whether the failure was in your app or on Reflex's side, and the end of the build log when that is what explains it. diff --git a/packages/reflex-hosting-cli/src/reflex_cli/utils/hosting.py b/packages/reflex-hosting-cli/src/reflex_cli/utils/hosting.py index 6f898dae26c..6663a03e952 100644 --- a/packages/reflex-hosting-cli/src/reflex_cli/utils/hosting.py +++ b/packages/reflex-hosting-cli/src/reflex_cli/utils/hosting.py @@ -2867,6 +2867,145 @@ def _get_deployment_status(deployment_id: str, token: str) -> str: return response.json() +# Terminal control sequences, which a build log is not entitled to emit into +# somebody's terminal. Ordered so a full sequence is consumed before the bare +# ESC that starts it: OSC first (it runs until its own terminator and is the +# one that writes the clipboard and forges hyperlinks), then CSI, then the +# two-character escapes, then anything left over. +_TERMINAL_CONTROL_RE = re.compile( + r"\x1b\][^\x07\x1b]*(?:\x07|\x1b\\)" # OSC 8 hyperlinks, OSC 52 clipboard + r"|\x1b\[[0-?]*[ -/]*[@-~]" # CSI: colour, cursor moves, line erases + # Every other escape sequence, in the general ECMA-48 shape: optional + # intermediates then one final byte in 0x30-0x7E. Narrower classes leave + # the final byte behind once the catch-all below eats the ESC -- `\x1b7` + # (DECSC) printing a stray "7", `\x1bc` (full terminal reset) a stray "c". + r"|\x1b[ -/]*[0-~]" + r"|[\x00-\x08\x0b-\x1f\x7f-\x9f]" # bare controls, keeping tab and newline +) + + +def _strip_terminal_controls(text: str) -> str: + """*text* with terminal control sequences removed. + + A build log is the output of building the user's own app, dependencies + included, and this excerpt is printed without anyone asking for it -- on + any failed deploy, rather than only when `reflex cloud apps build-logs` is + run. Colour is not worth carrying for that: the same sequences let the + output erase the lines above it, forge a hyperlink, or write the + clipboard, and none of that should be reachable from a dependency's build + script. `markup=False` stops rich reading the text as its own markup and + does nothing about escape sequences. + + Args: + text: The text to strip. + + Returns: + The text with terminal control sequences removed. + + """ + return _TERMINAL_CONTROL_RE.sub("", text) + + +def _get_deployment_failure(deployment_id: str, token: str) -> dict | None: + """Why a deployment failed, in fields, or None when that cannot be had. + + None covers every way of not getting an answer, and they are one case to + the caller: a control plane predating this endpoint 404s, an older + self-hosted one may not route it at all, and the network may simply be + down. All three mean the same thing here -- report the failure the way the + CLI always has, from the status string. + + Args: + deployment_id: The ID of the deployment. + token: The authentication token. + + Returns: + The failure report, or None if it could not be read. + + """ + import httpx + + try: + response = httpx.get( + urljoin( + constants.Hosting.HOSTING_SERVICE, + f"/api/v1/deployments/{deployment_id}/failure", + ), + headers=authorization_header(token), + timeout=constants.Hosting.TIMEOUT, + ) + response.raise_for_status() + report = response.json() + # Wider than json.JSONDecodeError, because a malformed body has more than + # one way to fail: an undecodable encoding raises UnicodeDecodeError (a + # ValueError) and a deeply nested document raises RecursionError (a + # RuntimeError). Either escaping would abort the watch over an answer this + # function is contracted to treat as no answer at all. + except (httpx.RequestError, httpx.HTTPStatusError, ValueError, RecursionError): + return None + return report if isinstance(report, dict) else None + + +def _report_deployment_failure( + deployment_id: str, + token: str, + status: str, + *, + offer_build_logs: bool, +) -> None: + """Tell the user why their deploy failed and what to do about it. + + The build log is offered only where the control plane says it is the + answer. A failure in our pipeline reported as a build failure sends + somebody hunting for a bug in an app that does not have one, which is the + more expensive of the two mistakes and the reason the fault is asked about + at all. + + Args: + deployment_id: The ID of the deployment. + token: The authentication token. + status: The status string the watch loop ended on. + offer_build_logs: Whether to point at the build log when no structured + report can be read, preserving what this arm printed before. + + """ + report = _get_deployment_failure(deployment_id, token) + if report is None: + logger.warning(status) + if offer_build_logs: + logger.warning( + f"to see the build logs:\n reflex cloud apps build-logs {deployment_id}" + ) + return + + logger.error(report.get("reason") or status) + if guidance := report.get("guidance"): + logger.warning(guidance) + + excerpt = report.get("build_log_excerpt") + # Typed as a string by the endpoint, checked because this one is not ours: + # the CLI is versioned apart from the control plane and talks to + # self-hosted ones, so a non-string here would raise in the sanitiser and + # take down a report that had already read fine. + if not excerpt or not isinstance(excerpt, str): + # A log the server holds but could not read is not a build that + # produced none, and saying nothing here reads as the latter. + if report.get("build_log_unreadable"): + logger.warning( + "the build log could not be read right now; try again with:\n" + f" reflex cloud apps build-logs {deployment_id}" + ) + return + # Raw build output: paths, versions and tracebacks, all of which rich would + # read as markup given the chance, plus whatever escape sequences the + # build printed. + console.print("\nthe end of the build log:") + console.print(_strip_terminal_controls(excerpt), markup=False) + console.print( + f"\nfor the whole log:\n reflex cloud apps build-logs {deployment_id}" + ) + + def watch_deployment_status(deployment_id: str, client: AuthenticatedClient) -> bool: """Continuously watch the status of a specific deployment. @@ -2900,16 +3039,19 @@ def watch_deployment_status(deployment_id: str, client: AuthenticatedClient) -> ) break if "build error" in status: - logger.warning(status) - logger.warning( - f"to see the build logs:\n reflex cloud apps build-logs {deployment_id}" + _report_deployment_failure( + deployment_id, client.token, status, offer_build_logs=True ) return False if "unable to find status for given id" in status: + # Not a failed deployment but an id that resolves to nothing, + # so there is no row to report on and nothing to ask for. logger.error(status) return False if "error" in status: - logger.warning(status) + _report_deployment_failure( + deployment_id, client.token, status, offer_build_logs=False + ) return False if "bad response" in status: logger.warning(status) diff --git a/tests/units/reflex_cli/utils/test_hosting.py b/tests/units/reflex_cli/utils/test_hosting.py index 7aaaaf9054a..5633b878045 100644 --- a/tests/units/reflex_cli/utils/test_hosting.py +++ b/tests/units/reflex_cli/utils/test_hosting.py @@ -21,6 +21,8 @@ SecurityReviewError, TokenSource, _archive_chunks, + _report_deployment_failure, + _strip_terminal_controls, _UploadAbandonedError, authenticated_token, create_app, @@ -1347,3 +1349,388 @@ def test_validate_token_failure_carries_request_id_on_exception(mocker: MockerFi validate_token("some-token") assert exc_info.value.request_id == get_auth_request_id() != "" + + +def _log_messages(caplog: pytest.LogCaptureFixture, level: int) -> list[str]: + """Return the captured log messages emitted at the given level. + + Args: + caplog: The pytest log capture fixture. + level: The numeric log level to filter records by. + + Returns: + The formatted messages of the matching records. + """ + return [r.getMessage() for r in caplog.records if r.levelno == level] + + +def _failure_report(mocker: MockerFixture, **fields: object): + """A mock 2xx /failure response carrying the given report fields. + + Args: + mocker: Pytest mocker fixture. + **fields: Failure-report fields to override on the default report. + + Returns: + A mocked successful HTTP response containing the failure report. + """ + report = { + "status": "Failed", + "code": None, + "fault": None, + "reason": "", + "guidance": "", + "build_log_excerpt": None, + } + report.update(fields) + return _ok(mocker, report) + + +def test_failure_report_prints_reason_and_build_log( + mocker: MockerFixture, caplog: pytest.LogCaptureFixture, capsys +): + """A build failure shows its reason, its guidance and the log's tail. + + Args: + mocker: Pytest mocker fixture. + caplog: Pytest log capture fixture. + capsys: Pytest stdout capture fixture. + """ + mocker.patch( + "httpx.get", + return_value=_failure_report( + mocker, + code="build_failed", + fault="customer", + reason="Deployment error: the build failed", + guidance="Your app failed to build.", + build_log_excerpt="ERROR: no matching distribution for pandas==9.9", + ), + ) + + _report_deployment_failure( + "dep-1", "fake-token", "build error", offer_build_logs=True + ) + + assert "Deployment error: the build failed" in _log_messages(caplog, logging.ERROR) + assert "Your app failed to build." in _log_messages(caplog, logging.WARNING) + printed = capsys.readouterr().out + assert "no matching distribution for pandas==9.9" in printed + assert "reflex cloud apps build-logs dep-1" in printed + + +def test_failure_report_withholds_build_log_when_the_fault_is_ours( + mocker: MockerFixture, caplog: pytest.LogCaptureFixture, capsys +): + """A platform failure says so and never sends the reader to their build. + + Args: + mocker: Pytest mocker fixture. + caplog: Pytest log capture fixture. + capsys: Pytest stdout capture fixture. + """ + mocker.patch( + "httpx.get", + return_value=_failure_report( + mocker, + code="image_push_failed", + fault="platform", + reason="Deployment error: could not push the image", + guidance="This failure is on Reflex's side, not in your app.", + ), + ) + + _report_deployment_failure( + "dep-1", "fake-token", "deployment error", offer_build_logs=False + ) + + assert "Deployment error: could not push the image" in _log_messages( + caplog, logging.ERROR + ) + assert "not in your app" in " ".join(_log_messages(caplog, logging.WARNING)) + assert "build-logs" not in capsys.readouterr().out + + +def test_failure_report_falls_back_when_the_endpoint_is_absent( + mocker: MockerFixture, caplog: pytest.LogCaptureFixture +): + """An older control plane 404s, and the status string is reported as before. + + Args: + mocker: Pytest mocker fixture. + caplog: Pytest log capture fixture. + """ + mocker.patch("httpx.get", return_value=_error(mocker, 404, "Not Found")) + + _report_deployment_failure( + "dep-1", "fake-token", "build error: something broke", offer_build_logs=True + ) + + warnings = _log_messages(caplog, logging.WARNING) + assert "build error: something broke" in warnings + assert any("reflex cloud apps build-logs dep-1" in w for w in warnings) + + +def test_failure_report_fallback_respects_the_arm_that_asked( + mocker: MockerFixture, caplog: pytest.LogCaptureFixture +): + """With no report, a generic failure offers no build log, as before. + + Args: + mocker: Pytest mocker fixture. + caplog: Pytest log capture fixture. + """ + mocker.patch("httpx.get", side_effect=httpx.RequestError("down")) + + _report_deployment_failure( + "dep-1", "fake-token", "deployment error", offer_build_logs=False + ) + + warnings = _log_messages(caplog, logging.WARNING) + assert warnings == ["deployment error"] + + +def test_failure_report_falls_back_to_the_status_when_no_reason_was_recorded( + mocker: MockerFixture, caplog: pytest.LogCaptureFixture +): + """A row that recorded no reason still reports something. + + Args: + mocker: Pytest mocker fixture. + caplog: Pytest log capture fixture. + """ + mocker.patch("httpx.get", return_value=_failure_report(mocker)) + + _report_deployment_failure( + "dep-1", "fake-token", "deployment error", offer_build_logs=False + ) + + assert "deployment error" in _log_messages(caplog, logging.ERROR) + + +@pytest.mark.parametrize( + "hostile, banned", + [ + # OSC 52: writes the reader's clipboard. + ("\x1b]52;c;bWFsaWNpb3Vz\x07error: build failed", "\x1b]52"), + # OSC 8: renders as one destination and links to another. + ("\x1b]8;;https://evil.example\x07docs\x1b]8;;\x07", "\x1b]8"), + # CSI: erases the lines above it, hiding what really happened. + ("done\x1b[2J\x1b[1;1Hbuild succeeded", "\x1b["), + # A carriage return overwrites the line in place. + ("real error\rbuild succeeded", "\r"), + ], +) +def test_a_build_log_cannot_drive_the_terminal(hostile: str, banned: str): + """Build output is the app's own dependencies, printed without being asked for. + + Args: + hostile: Build output carrying a terminal control sequence. + banned: The sequence that must not survive. + """ + cleaned = _strip_terminal_controls(hostile) + + assert banned not in cleaned + assert "\x1b" not in cleaned + + +def test_stripping_keeps_the_text_worth_reading(): + """Colour is dropped; the words, newlines and tabs that carry the answer stay.""" + log = "\x1b[31mERROR\x1b[0m: no matching distribution\n\tfor pandas==9.9\n" + + assert ( + _strip_terminal_controls(log) + == "ERROR: no matching distribution\n\tfor pandas==9.9\n" + ) + + +def test_the_printed_excerpt_is_stripped( + mocker: MockerFixture, caplog: pytest.LogCaptureFixture, capsys +): + """The sanitiser is actually on the path the excerpt takes to the terminal. + + Args: + mocker: Pytest mocker fixture. + caplog: Pytest log capture fixture. + capsys: Pytest stdout capture fixture. + """ + mocker.patch( + "httpx.get", + return_value=_failure_report( + mocker, + code="build_failed", + reason="Deployment error: the build failed", + build_log_excerpt="\x1b]52;c;cHduZWQ=\x07ERROR: \x1b[31mno such package\x1b[0m", + ), + ) + + _report_deployment_failure( + "dep-1", "fake-token", "build error", offer_build_logs=True + ) + + printed = capsys.readouterr().out + assert "ERROR: no such package" in printed + assert "\x1b" not in printed + + +@pytest.mark.parametrize( + "hostile", + [ + "A\x1b7B", # DECSC: final byte 0x37, outside the CSI and OSC shapes + "A\x1b=B", # DECKPAM + "A\x1bcB", # RIS: a full terminal reset + "A\x1b(0B", # a designator with an intermediate byte + ], +) +def test_a_two_character_escape_leaves_no_stray_byte(hostile: str): + """The final byte goes with the ESC, rather than printing as garbage. + + Args: + hostile: Build output carrying a non-CSI escape sequence. + """ + cleaned = _strip_terminal_controls(hostile) + + assert cleaned == "AB" + + +def test_an_undecodable_body_falls_back_rather_than_raising( + mocker: MockerFixture, caplog: pytest.LogCaptureFixture +): + """A 2xx body httpx cannot decode is not an answer, and must not end the watch. + + Args: + mocker: Pytest mocker fixture. + caplog: Pytest log capture fixture. + """ + response = mocker.Mock() + response.raise_for_status.return_value = None + response.json.side_effect = UnicodeDecodeError("utf-8", b"\xff", 0, 1, "invalid") + mocker.patch("httpx.get", return_value=response) + + _report_deployment_failure( + "dep-1", "fake-token", "build error", offer_build_logs=True + ) + + assert "build error" in _log_messages(caplog, logging.WARNING) + + +def test_a_non_string_excerpt_is_ignored( + mocker: MockerFixture, caplog: pytest.LogCaptureFixture, capsys +): + """The CLI ships apart from the server, so the excerpt's type is not a given. + + Args: + mocker: Pytest mocker fixture. + caplog: Pytest log capture fixture. + capsys: Pytest stdout capture fixture. + """ + mocker.patch( + "httpx.get", + return_value=_failure_report( + mocker, + code="build_failed", + reason="Deployment error: the build failed", + build_log_excerpt={"unexpected": "shape"}, + ), + ) + + _report_deployment_failure( + "dep-1", "fake-token", "build error", offer_build_logs=True + ) + + assert "Deployment error: the build failed" in _log_messages(caplog, logging.ERROR) + assert "the end of the build log" not in capsys.readouterr().out + + +def test_an_unreadable_log_is_reported_rather_than_passed_over( + mocker: MockerFixture, caplog: pytest.LogCaptureFixture +): + """A log the server could not read is not a build that produced none. + + Args: + mocker: Pytest mocker fixture. + caplog: Pytest log capture fixture. + """ + mocker.patch( + "httpx.get", + return_value=_failure_report( + mocker, + code="build_failed", + reason="Deployment error: the build failed", + build_log_excerpt=None, + build_log_unreadable=True, + ), + ) + + _report_deployment_failure( + "dep-1", "fake-token", "build error", offer_build_logs=True + ) + + warnings = " ".join(_log_messages(caplog, logging.WARNING)) + assert "could not be read" in warnings + assert "reflex cloud apps build-logs dep-1" in warnings + + +def test_a_build_that_stored_no_log_says_nothing_extra( + mocker: MockerFixture, caplog: pytest.LogCaptureFixture, capsys +): + """Absence is not an outage, and there is nothing to send the reader to. + + The reason stands alone: no excerpt, no header over one, and no command to + go and fetch a log that was never stored. The command is what separates + this path from the unreadable one, which does offer it. + + Args: + mocker: Pytest mocker fixture. + caplog: Pytest log capture fixture. + capsys: Pytest stdout capture fixture. + """ + mocker.patch( + "httpx.get", + return_value=_failure_report( + mocker, + code="build_failed", + reason="Deployment error: the build failed", + build_log_excerpt=None, + ), + ) + + _report_deployment_failure( + "dep-1", "fake-token", "build error", offer_build_logs=True + ) + + assert "Deployment error: the build failed" in _log_messages(caplog, logging.ERROR) + said = " ".join(_log_messages(caplog, logging.WARNING)) + capsys.readouterr().out + assert "could not be read" not in said + assert "the end of the build log" not in said + assert "reflex cloud apps build-logs" not in said + + +@pytest.mark.parametrize( + "failure", + [ + UnicodeDecodeError("utf-8", b"\xff", 0, 1, "invalid"), + # A deeply nested document: a RuntimeError, so a ValueError catch misses it. + RecursionError("maximum recursion depth exceeded"), + ], +) +def test_a_malformed_body_falls_back_however_it_is_malformed( + mocker: MockerFixture, caplog: pytest.LogCaptureFixture, failure: Exception +): + """Not getting an answer costs nothing, whichever way the answer is broken. + + Args: + mocker: Pytest mocker fixture. + caplog: Pytest log capture fixture. + failure: What `.json()` raises on this body. + """ + response = mocker.Mock() + response.raise_for_status.return_value = None + response.json.side_effect = failure + mocker.patch("httpx.get", return_value=response) + + _report_deployment_failure( + "dep-1", "fake-token", "build error", offer_build_logs=True + ) + + assert "build error" in _log_messages(caplog, logging.WARNING)