From 2c58874c52d830f03af6c21927103b7119a4bee0 Mon Sep 17 00:00:00 2001 From: Bobby Noelte Date: Fri, 6 Nov 2026 12:15:48 +0100 Subject: [PATCH] fix(cache): use a reproducible function hash for cache_in_file keys Build the cache_in_file key from an MD5 hash of the wrapped function instead of inspect.getsource(). Reading the source fails for functions whose source is not on disk, and the key could go stale when the file changes after import. The hash covers what decides the result besides the arguments: the qualified name, the bytecode with the names of called functions, attributes and globals, the constants including nested functions, the defaults and the values captured from an enclosing function. Wrappers made with functools.wraps are unwrapped. Code objects, functions and sets are hashed without memory addresses or hash-seed dependent order, so the key stays the same across restarts and the file cache keeps hitting. Additional fixes - Build the key from the arguments bound to the signature, so *args, **kwargs and the arguments of wrapped functions are part of it. - Apply control parameters given at call time to that call only; they no longer change the decorator settings for later calls. - Apply all control parameters of a call, so until_date is no longer dropped when force_update is also passed; keep the precedence until_datetime > with_ttl > until_date. - Recreate an unreadable cache file in that call only, instead of disabling the cache for the rest of the process. - Pass force_update in two provider tests that relied on a force_update left over from an earlier test. - Fix "Calback" typo in cache_energy_management_store_callback docstring. Signed-off-by: Bobby Noelte Co-Authored-By: Claude Opus 5.5 --- src/akkudoktoreos/core/cache.py | 242 +++++++++++++++++------- tests/test_cache.py | 303 ++++++++++++++++++++++++++++++ tests/test_elecpriceakkudoktor.py | 5 +- tests/test_weatheropenmeteo.py | 4 +- 4 files changed, 482 insertions(+), 72 deletions(-) diff --git a/src/akkudoktoreos/core/cache.py b/src/akkudoktoreos/core/cache.py index 073866b1..108bc9d6 100644 --- a/src/akkudoktoreos/core/cache.py +++ b/src/akkudoktoreos/core/cache.py @@ -13,6 +13,7 @@ import os import pickle import tempfile import threading +import types from pathlib import Path from typing import ( IO, @@ -51,8 +52,91 @@ Param = ParamSpec("Param") RetType = TypeVar("RetType") +def _get_func_code_hash(func: Callable[..., Any]) -> str: + """Return a hash that identifies a function's behaviour for the file cache. + + Covers what decides the result besides the call arguments: the function's + qualified name, its compiled code (bytecode, referenced names and + constants, including nested functions), its default values and the values + it captures from an enclosing function. Wrappers made with + ``functools.wraps`` are unwrapped first. + + The hash is the same in every process as long as the code is unchanged, so + the file cache keeps hitting across restarts. It changes when the Python + minor version changes the bytecode, which invalidates cache entries once. + Values whose ``repr()`` contains a memory address (e.g. an object without a + custom ``__repr__`` used as a default) make the hash process specific. + + Args: + func: The decorated function. + + Returns: + The hexadecimal MD5 digest. + """ + func = inspect.unwrap(func) + digest = hashlib.md5(usedforsecurity=False) + digest.update(f"{func.__module__}.{func.__qualname__}".encode("utf-8")) + _update_value_digest(digest, func.__defaults__) + _update_value_digest(digest, func.__kwdefaults__) + for cell in func.__closure__ or (): + try: + value = cell.cell_contents + except ValueError: + # Cell of a variable that is not assigned yet. + value = "" + _update_value_digest(digest, value) + _update_code_digest(digest, func.__code__) + return digest.hexdigest() + + +def _update_code_digest(digest: Any, code: types.CodeType) -> None: + """Feed a code object into ``digest`` without process-specific parts. + + Args: + digest: A ``hashlib`` hash object. + code: The code object. + """ + digest.update(code.co_code) + # Names of called functions, attributes and globals; co_code only indexes them. + digest.update(repr(code.co_names).encode("utf-8")) + for const in code.co_consts: + _update_value_digest(digest, const) + + +def _update_value_digest(digest: Any, value: Any) -> None: + """Feed a constant, default or captured value into ``digest`` reproducibly. + + Args: + digest: A ``hashlib`` hash object. + value: The value. + """ + if isinstance(value, types.CodeType): + # repr() of a code object contains its memory address. + _update_code_digest(digest, value) + elif isinstance(value, types.FunctionType): + # E.g. a function captured from the enclosing scope; its repr() has an address. + digest.update(f"{value.__module__}.{value.__qualname__}".encode("utf-8")) + _update_code_digest(digest, value.__code__) + elif isinstance(value, (frozenset, set)): + # Iteration order of a set of str depends on the hash seed. + digest.update(repr(sorted(map(repr, value))).encode("utf-8")) + elif isinstance(value, tuple): + digest.update(b"(") + for item in value: + _update_value_digest(digest, item) + digest.update(b")") + elif isinstance(value, dict): + digest.update(b"{") + for key in sorted(value, key=repr): + digest.update(repr(key).encode("utf-8")) + _update_value_digest(digest, value[key]) + digest.update(b"}") + else: + digest.update(repr(value).encode("utf-8")) + + def cache_energy_management_store_callback(event: int, key: Any, value: Any) -> None: - """Calback function for CacheEnergyManagementStore.""" + """Callback function for CacheEnergyManagementStore.""" CacheEnergyManagementStore.last_event = event CacheEnergyManagementStore.last_key = key CacheEnergyManagementStore.last_value = value @@ -899,8 +983,6 @@ def cache_in_file( The cache file is created using `CacheFileStore` and stored with the generated key. If a valid cache file exists, it is returned instead of recomputing the result. The cache expiration is controlled by the `until_date`, `until_datetime`, `with_ttl`, or `force_update` arguments. - If these arguments are present in the function call, their values override those specified in - the decorator. By default, cache files are pickled to save storage space unless a `suffix` is provided. The `mode` parameter allows specifying file modes for reading and writing, and the `delete` @@ -946,76 +1028,61 @@ def cache_in_file( result = expensive_computation(until_date=date.today()) Notes: - - The cache key is based on the function arguments after excluding those in `ignore_params`. - - If conflicting expiration parameters are provided (`until_date`, `until_datetime`, - `with_ttl`), the one in the function call takes precedence. + - The cache key is based on the function arguments after excluding those in + `ignore_params`, and on a hash of the function's code, defaults and captured + values (see `_get_func_code_hash`). + - The control parameters (`force_update`, `until_date`, `until_datetime`, + `with_ttl`) are consumed by the wrapper and are not forwarded to the wrapped + function. + - Control parameters given at call time apply to that call only. Expiration + parameters given at call time replace those of the decorator. + - If several expiration parameters are given, `until_datetime` takes + precedence over `with_ttl`, which takes precedence over `until_date`. """ def decorator(func: Callable[Param, RetType]) -> Callable[Param, RetType]: - nonlocal \ - ignore_params, \ - force_update, \ - until_date, \ - until_datetime, \ - with_ttl, \ - mode, \ - delete, \ - suffix - func_source_code = inspect.getsource(func) + func_code_hash = _get_func_code_hash(func) + # Follows functools.wraps wrappers to the signature of the wrapped function. + func_signature = inspect.signature(func) def wrapper(*args: Param.args, **kwargs: Param.kwargs) -> RetType: - nonlocal \ - ignore_params, \ - force_update, \ - until_date, \ - until_datetime, \ - with_ttl, \ - mode, \ - delete, \ - suffix - # Convert args to a dictionary based on the function's signature - args_names = func.__code__.co_varnames[: func.__code__.co_argcount] - args_dict = dict(zip(args_names, args)) + # Control parameters given at call time apply to this call only. + call_force_update = kwargs.pop("force_update", force_update) + # In order of precedence: until_datetime > with_ttl > until_date. + call_expiry = [ + (param, kwargs.pop(param)) + for param in ("until_datetime", "with_ttl", "until_date") + if param in kwargs + ] + if call_expiry: + # The call's expiry replaces the decorator's; the first given one wins. + expiry: dict[str, Any] = { + "until_datetime": None, + "with_ttl": None, + "until_date": None, + } + param, value = call_expiry[0] + expiry[param] = value + else: + expiry = { + "until_datetime": until_datetime, + "with_ttl": with_ttl, + "until_date": until_date, + } - # Search for caching parameters of function and remove - for param in ["force_update", "until_datetime", "with_ttl", "until_date"]: - if param in kwargs: - if param == "force_update": - force_update = kwargs[param] # type: ignore[assignment] - kwargs.pop("force_update") - if param == "until_datetime": - until_datetime = kwargs[param] - until_date = None - with_ttl = None - elif param == "with_ttl": - until_datetime = None - until_date = None - with_ttl = kwargs[param] - elif param == "until_date": - until_datetime = None - until_date = kwargs[param] - with_ttl = None - kwargs.pop("force_update", None) - kwargs.pop("until_datetime", None) - kwargs.pop("until_date", None) - kwargs.pop("with_ttl", None) - break - - # Remove ignored params - kwargs_clone = kwargs.copy() - for param in ignore_params: - args_dict.pop(param, None) - kwargs_clone.pop(param, None) - - # Create key based on argument names, argument values, and function source code - key = str(args_dict) + str(kwargs_clone) + str(func_source_code) + # Create key based on argument names, argument values, and function code + key = str(_cache_key_arguments(func_signature, args, kwargs, ignore_params)) + key += func_code_hash result: Optional[RetType | bytes] = None # Get cache file that is currently valid cache_file = CacheFileStore().get( - key, until_date=until_date, until_datetime=until_datetime, ttl_duration=with_ttl + key, + until_date=expiry["until_date"], + until_datetime=expiry["until_datetime"], + ttl_duration=expiry["with_ttl"], ) - if not force_update and cache_file is not None: + if not call_force_update and cache_file is not None: # cache file is available try: logger.debug("Used cache file for function: " + func.__name__) @@ -1026,9 +1093,9 @@ def cache_in_file( result = cache_file.read() except Exception as e: logger.info(f"Read failed: {e}") - # Fail gracefully - force creation - force_update = True - if force_update or cache_file is None: + # Fail gracefully - recreate the cache file in this call + call_force_update = True + if call_force_update or cache_file is None: # Otherwise, call the function and save its result to the cache logger.debug("Created cache file for function: " + func.__name__) cache_file = CacheFileStore().create( @@ -1036,9 +1103,9 @@ def cache_in_file( mode=mode, delete=delete, suffix=suffix, - until_datetime=until_datetime, - until_date=until_date, - with_ttl=with_ttl, + until_datetime=expiry["until_datetime"], + until_date=expiry["until_date"], + with_ttl=expiry["with_ttl"], ) result = func(*args, **kwargs) try: @@ -1056,3 +1123,42 @@ def cache_in_file( return wrapper return decorator + + +def _cache_key_arguments( + signature: inspect.Signature, + args: tuple[Any, ...], + kwargs: dict[str, Any], + ignore_params: List[str], +) -> dict[str, Any]: + """Return the call arguments that identify a cache entry. + + Binds the arguments to the function's signature, so positional and keyword + passing give the same key and ``*args``/``**kwargs`` are included. Defaults + are not filled in; they are part of the function hash. + + Args: + signature: Signature of the decorated (unwrapped) function. + args: Positional call arguments. + kwargs: Keyword call arguments, without the cache control parameters. + ignore_params: Parameter names to leave out of the key. + + Returns: + The arguments by parameter name. + """ + try: + arguments = dict(signature.bind_partial(*args, **kwargs).arguments) + except TypeError: + # Arguments that do not fit the signature (e.g. a wrapper that adds parameters). + arguments = {"*args": args, **kwargs} + var_keyword = next( + (p.name for p in signature.parameters.values() if p.kind is p.VAR_KEYWORD), None + ) + if var_keyword in arguments: + # Copy: the dict collected by **kwargs is the one passed to the function. + arguments[var_keyword] = dict(arguments[var_keyword]) + for param in ignore_params: + arguments.pop(param, None) + if var_keyword in arguments: + arguments[var_keyword].pop(param, None) + return arguments diff --git a/tests/test_cache.py b/tests/test_cache.py index 1e91c0c6..0ef2c645 100644 --- a/tests/test_cache.py +++ b/tests/test_cache.py @@ -1,7 +1,12 @@ +import functools import io import json +import os import pickle +import subprocess +import sys import tempfile +import textwrap from datetime import date, datetime, timedelta from pathlib import Path from time import sleep @@ -14,6 +19,7 @@ from akkudoktoreos.core.cache import ( CacheEnergyManagementStore, CacheFileRecord, CacheFileStore, + _get_func_code_hash, cache_energy_management, cache_in_file, ) @@ -795,3 +801,300 @@ class TestCacheFileDecorators: assert len(cache_file_store._store) == 1 assert cache_file_store._store[key].cache_file is not None assert result1 == result + + def test_cache_in_file_distinguishes_different_function_bodies(self, cache_file_store): + """Cache key must change when the function's code changes.""" + cache_file_store.clear(clear_all=True) + + @cache_in_file(mode="w+") + def f(x=None, until_date=None): + return x + 100 + + @cache_in_file(mode="w+") + def g(x=None, until_date=None): + return x + 200 + + until = to_datetime().add(days=1) + assert f(x=1, until_date=until) == 101 + assert g(x=1, until_date=until) == 201 + assert len(cache_file_store._store) == 2 + + def test_force_update_combined_with_until_date(self, cache_file_store): + cache_file_store.clear(clear_all=True) + + @cache_in_file(mode="w+") + def my_function(until_date=None): + return "New result" + + until = to_datetime().add(days=1).date() + my_function(until_date=until) + + key = next(iter(cache_file_store._store)) + cf = cache_file_store._store[key].cache_file + cf.seek(0) + cf.write("Cached result") + + # force_update must win even though until_date comes first in the call. + assert my_function(until_date=until, force_update=True) == "New result" # type: ignore[call-arg] + + + def test_call_time_control_parameters_apply_to_the_call_only(self, cache_file_store): + """A force_update given once does not stick to later calls.""" + cache_file_store.clear(clear_all=True) + calls: list[int] = [] + + @cache_in_file() + def f(): + calls.append(1) + return len(calls) + + f() + f(force_update=True) # type: ignore[call-arg] + f() + f() + + assert len(calls) == 2 + + def test_call_time_expiry_does_not_replace_the_decorator_expiry(self, cache_file_store): + cache_file_store.clear(clear_all=True) + + @cache_in_file(with_ttl="1 hour") + def f(): + return "result" + + f(until_datetime=to_datetime().add(days=2)) # type: ignore[call-arg] + cache_file_store.clear(clear_all=True) + f() + + (record,) = cache_file_store._store.values() + assert record.ttl_duration == to_duration("1 hour") + + def test_call_time_expiry_precedence(self, cache_file_store): + """until_datetime takes precedence over with_ttl and until_date.""" + cache_file_store.clear(clear_all=True) + + @cache_in_file() + def f(): + return "result" + + until = to_datetime().add(days=3) + f(until_date=to_datetime().add(days=1).date(), with_ttl="1 hour", until_datetime=until) # type: ignore[call-arg] + + (record,) = cache_file_store._store.values() + assert compare_datetimes(record.until_datetime, until).equal + assert record.ttl_duration is None + + def test_read_failure_recreates_the_cache_file_once(self, cache_file_store): + """A broken cache file is recreated, and caching works again afterwards.""" + cache_file_store.clear(clear_all=True) + calls: list[int] = [] + + @cache_in_file() + def f(): + calls.append(1) + return "result" + + f() + (record,) = cache_file_store._store.values() + record.cache_file.seek(0) + record.cache_file.truncate() + record.cache_file.write(b"not a pickle") + + assert f() == "result" + assert f() == "result" + assert len(calls) == 2 + + def test_var_positional_arguments_are_part_of_the_key(self, cache_file_store): + cache_file_store.clear(clear_all=True) + + @cache_in_file() + def f(*items): + return sum(items) + + assert f(1) == 1 + assert f(2) == 2 + + def test_arguments_of_a_wrapped_function_are_part_of_the_key(self, cache_file_store): + cache_file_store.clear(clear_all=True) + + def passthrough(func): + @functools.wraps(func) + def inner(*args, **kwargs): + return func(*args, **kwargs) + + return inner + + @cache_in_file() + @passthrough + def f(x): + return x * 10 + + assert f(1) == 10 + assert f(2) == 20 + + def test_positional_and_keyword_arguments_give_the_same_key(self, cache_file_store): + cache_file_store.clear(clear_all=True) + calls: list[int] = [] + + @cache_in_file() + def f(x): + calls.append(x) + return x + + f(1) + f(x=1) + + assert calls == [1] + + def test_ignored_keyword_arguments_still_reach_the_function(self, cache_file_store): + cache_file_store.clear(clear_all=True) + received: list[dict] = [] + + @cache_in_file(ignore_params=["token"]) + def f(**options): + received.append(options) + return options["a"] + + f(a=1, token="secret-1") + f(a=1, token="secret-2") # same key: token is ignored + + assert received == [{"a": 1, "token": "secret-1"}] + + +class TestFuncCodeHash: + def test_hash_is_stable(self): + from akkudoktoreos.core.cache import _get_func_code_hash + + def f(x): + return x + 1 + + assert _get_func_code_hash(f) == _get_func_code_hash(f) + + def test_hash_changes_with_logic(self): + from akkudoktoreos.core.cache import _get_func_code_hash + + def f(x): + return x + 1 + + def g(x): + return x + 2 + + assert _get_func_code_hash(f) != _get_func_code_hash(g) + + def test_hash_changes_with_constant(self): + from akkudoktoreos.core.cache import _get_func_code_hash + + def f(x): + return x + 100 + + def g(x): + return x + 200 + + assert _get_func_code_hash(f) != _get_func_code_hash(g) + + + @staticmethod + def compile_function(source: str): + """Compile ``def f`` from source, so compared functions share the qualified name.""" + namespace: dict = {"__name__": "cache_probe"} + exec(textwrap.dedent(source), namespace) # noqa: S102 + return namespace["f"] + + @pytest.mark.parametrize( + ("source_a", "source_b"), + [ + # Called method (co_names) + ("def f(s): return s.upper()", "def f(s): return s.lower()"), + # Global function (co_names) + ("def f(x): return str(x)", "def f(x): return hex(x)"), + # Positional default + ("def f(x, factor=5): return x * factor", "def f(x, factor=6): return x * factor"), + # Keyword-only default + ("def f(x, *, factor=5): return x * factor", "def f(x, *, factor=6): return x * factor"), + # Constant in a nested function + ("def f(x): return list(map(lambda v: v + 1, x))", "def f(x): return list(map(lambda v: v + 2, x))"), + ], + ) + def test_hash_distinguishes_functions(self, source_a, source_b): + assert _get_func_code_hash(self.compile_function(source_a)) != _get_func_code_hash( + self.compile_function(source_b) + ) + + def test_hash_covers_captured_values(self): + def make(n): + def inner(x): + return x + n + + return inner + + assert _get_func_code_hash(make(1)) != _get_func_code_hash(make(2)) + assert _get_func_code_hash(make(1)) == _get_func_code_hash(make(1)) + + def test_hash_unwraps_functools_wraps(self): + def passthrough(func): + @functools.wraps(func) + def inner(*args, **kwargs): + return func(*args, **kwargs) + + return inner + + def plain(x): + return 1 + + wrapped_1 = passthrough(plain) + wrapped_2 = passthrough(self.compile_function("def f(x): return 2")) + + assert _get_func_code_hash(wrapped_1) == _get_func_code_hash(plain) + assert _get_func_code_hash(wrapped_1) != _get_func_code_hash(wrapped_2) + + def test_hash_is_the_same_in_every_process(self, tmp_path): + """The file cache outlives the process: the hash must not depend on addresses or the hash seed.""" + (tmp_path / "cacheprobe.py").write_text( + textwrap.dedent( + """ + def with_lambda(x): + return sorted(x, key=lambda v: -v) + + def with_genexpr(x): + return sum(v * v for v in x) + + def with_set_literal(x): + return x in {"alpha", "beta", "gamma", "delta", "epsilon"} + + def with_set_default(x, allowed=frozenset({"alpha", "beta", "gamma"})): + return x in allowed + + def make(n): + def inner(x): + return x in {"alpha", "beta"} and n + return inner + + with_closure = make(3) + """ + ), + encoding="utf-8", + ) + script = ( + "import cacheprobe as m\n" + "from akkudoktoreos.core.cache import _get_func_code_hash as h\n" + "for name in ('with_lambda', 'with_genexpr', 'with_set_literal',\n" + " 'with_set_default', 'with_closure'):\n" + " print(name, h(getattr(m, name)))\n" + ) + + def hashes(seed: str) -> str: + env = {**os.environ, "PYTHONHASHSEED": seed, "PYTHONPATH": str(tmp_path)} + result = subprocess.run( + [sys.executable, "-c", script], + env=env, + cwd=tmp_path, + capture_output=True, + text=True, + check=True, + timeout=120, + ) + return result.stdout + + first = hashes("1") + assert len(set(first.split()[1::2])) == 5 + assert hashes("2") == first diff --git a/tests/test_elecpriceakkudoktor.py b/tests/test_elecpriceakkudoktor.py index b9fb4c11..5f4f5ec3 100644 --- a/tests/test_elecpriceakkudoktor.py +++ b/tests/test_elecpriceakkudoktor.py @@ -178,11 +178,12 @@ class TestElecPriceAkkudokor: requests.exceptions.HTTPError if exception else None ) mock_get.return_value = mock_response + # force_update: bypass the file cache, the request must be made in every case. if exception: with pytest.raises(exception): - provider._request_forecast() + provider._request_forecast(force_update=True) else: - provider._request_forecast() + provider._request_forecast(force_update=True) @pytest.mark.asyncio diff --git a/tests/test_weatheropenmeteo.py b/tests/test_weatheropenmeteo.py index 9cb55799..930531e6 100644 --- a/tests/test_weatheropenmeteo.py +++ b/tests/test_weatheropenmeteo.py @@ -273,8 +273,8 @@ def test_openmeteo_request_mode_selection( ems_eos = get_ems() ems_eos.set_start_datetime(start) - # Execute - provider._request_forecast() + # Execute; force_update bypasses the file cache, the request must be made. + provider._request_forecast(force_update=True) # Inspect request params params = mock_get.call_args[1]["params"]