diff --git a/python/arcticdb/flattener.py b/python/arcticdb/flattener.py index cfe6efaa8ce..bf83fd91197 100644 --- a/python/arcticdb/flattener.py +++ b/python/arcticdb/flattener.py @@ -14,11 +14,11 @@ from arcticdb.exceptions import DataTooNestedException, UnsupportedKeyInDictionary try: - from msgpack.fallback import DEFAULT_RECURSE_LIMIT + from msgpack.fallback import DEFAULT_RECURSE_LIMIT as _DEFAULT_RECURSE_LIMIT except ImportError: # The default as of msgpack 1.1.0 - handle the import error in case the msgpack wheel stops exporting this constant. # Want to keep compatibility with a wide range of msgpack versions. - DEFAULT_RECURSE_LIMIT = 511 + _DEFAULT_RECURSE_LIMIT = 511 from arcticdb import _msgpack_compat from arcticdb.log import version as log @@ -27,6 +27,19 @@ from arcticdb.preconditions import check from arcticdb_ext import get_config_int +# Two separate limits govern maximum nesting depth: +# Write (msgpack packer): uses ~2 Python stack frames per level; capped at DEFAULT_RECURSE_LIMIT // 2. +# Read (create_original_obj_from_metastruct): uses ~3 Python stack frames per level, so reads +# fail above sys.getrecursionlimit() // 3 ≈ 333 with Python's default limit of 1000. +# +# The read path is tighter, so we pin msgpack's DEFAULT_RECURSE_LIMIT at 511, giving a write +# cap of 511 // 2 == 255 — safely below the ~333 read ceiling. msgpack 1.2.0 raised its own +# constant DEFAULT_RECURSE_LIMIT to 1024, which silently allowed writes that the reader could not +# reconstruct. We keep 511 regardless of what msgpack exports. +# TODO: once the metastruct reader is non-recursive, raise to (sys.getrecursionlimit() - 10) // 2. +# https://man312219.monday.com/boards/7852509418/pulses/12254825163 +DEFAULT_RECURSE_LIMIT = min(511, _DEFAULT_RECURSE_LIMIT) + class Flattener: # Probably a bad idea given the dict key could have this, fine for now as write does not allow this symbol anyways. diff --git a/python/tests/unit/arcticdb/version_store/test_recursive_normalizers.py b/python/tests/unit/arcticdb/version_store/test_recursive_normalizers.py index ffe55218090..e89514e89df 100644 --- a/python/tests/unit/arcticdb/version_store/test_recursive_normalizers.py +++ b/python/tests/unit/arcticdb/version_store/test_recursive_normalizers.py @@ -8,7 +8,7 @@ import numpy as np import arcticdb from arcticdb import QueryBuilder, LibraryOptions -from arcticdb.flattener import Flattener +from arcticdb.flattener import Flattener, DEFAULT_RECURSE_LIMIT from arcticdb.version_store._custom_normalizers import ( CustomNormalizer, register_normalizer, @@ -392,12 +392,18 @@ def test_deep_nesting_metastruct_size_over_limit(lmdb_version_store_v1, all_recu key = "reasonable_length_key" data = {key: pd.DataFrame({"col": [0]})} - nesting_levels = 256 + # Mirror the guard in flattener._create_meta_structure (raises above DEFAULT_RECURSE_LIMIT // 2). + # flattener clamps DEFAULT_RECURSE_LIMIT to 511, so this is 255 - see the note there for why. + nesting_limit = DEFAULT_RECURSE_LIMIT // 2 + nesting_levels = nesting_limit + 1 for i in range(nesting_levels - 1): data[key] = {key: data[key]} # When & Then - with pytest.raises(DataTooNestedException, match=r"^Symbol sym cannot be recursively normalized.*255 levels.*"): + with pytest.raises( + DataTooNestedException, + match=rf"^Symbol sym cannot be recursively normalized.*{nesting_limit} levels.*", + ): lib.write(sym, data, recursive_normalizers=True) @@ -408,7 +414,7 @@ def test_deep_nesting_metastruct_size_under_limit(lmdb_version_store_v1, all_rec key = "reasonable_length_key" data = {key: pd.DataFrame({"col": [0]})} - nesting_levels = 255 + nesting_levels = (DEFAULT_RECURSE_LIMIT - 1) // 2 for i in range(nesting_levels - 1): data[key] = {key: data[key]}