From f52fadc2030e3c016035ea06589ee8f82d2aba34 Mon Sep 17 00:00:00 2001 From: Michael D'Angelo Date: Tue, 11 Aug 2026 15:18:10 -0700 Subject: [PATCH] fix(cache): update scan-result entries atomically on hits --- CHANGELOG.md | 1 + modelaudit/cache/scan_results_cache.py | 40 +++++++++---- tests/cache/test_cache_correctness.py | 79 ++++++++++++++++++++++++++ 3 files changed, 108 insertions(+), 12 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a35827d21..8bf79ddca 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Prevent Windows cache identity probes from creating locked temporary files inside scanned directories. - Preserve locked Windows cache probes reached through directory aliases while clearing stale scan results. - Place cross-volume Windows cache identity probes near the volume root instead of the nearest ancestor of the scanned path, so a probe can no longer appear inside a directory tree that a concurrent scan is walking. +- Keep published scan-result cache entries readable when concurrent or interrupted hits update access metadata. ## [0.2.52](https://github.com/promptfoo/modelaudit/compare/v0.2.51...v0.2.52) (2026-07-22) diff --git a/modelaudit/cache/scan_results_cache.py b/modelaudit/cache/scan_results_cache.py index ef4ba3323..ab9632cf4 100644 --- a/modelaudit/cache/scan_results_cache.py +++ b/modelaudit/cache/scan_results_cache.py @@ -589,11 +589,7 @@ def _get_cached_result_with_identity( self._record_cache_miss("invalid") return None, file_identity - cache_entry["cache_metadata"]["access_count"] += 1 - cache_entry["cache_metadata"]["last_access"] = time.time() - - with open(cache_file_path, "w", encoding="utf-8") as f: - json.dump(cache_entry, f, indent=2) + self._update_cached_entry_access(cache_file_path, cache_entry) if not self._file_identity_matches(file_path, file_identity): self._record_cache_miss("changed") @@ -718,13 +714,7 @@ def _get_cached_result_by_key( self._record_cache_miss("invalid") return None - # Update access statistics - cache_entry["cache_metadata"]["access_count"] += 1 - cache_entry["cache_metadata"]["last_access"] = time.time() - - # Write back updated entry (async write would be better but adds complexity) - with open(cache_file_path, "w", encoding="utf-8") as f: - json.dump(cache_entry, f, indent=2) + self._update_cached_entry_access(cache_file_path, cache_entry) if ( file_path is not None @@ -743,6 +733,32 @@ def _get_cached_result_by_key( self._record_cache_miss("error") return None + @staticmethod + def _update_cached_entry_access(cache_file_path: Path, cache_entry: dict[str, Any]) -> None: + cache_entry["cache_metadata"]["access_count"] += 1 + cache_entry["cache_metadata"]["last_access"] = time.time() + temporary_cache_path: Path | None = None + + try: + with tempfile.NamedTemporaryFile( + mode="w", + encoding="utf-8", + dir=cache_file_path.parent, + prefix=f".{cache_file_path.name}.", + suffix=".tmp", + delete=False, + ) as cache_file: + temporary_cache_path = Path(cache_file.name) + json.dump(cache_entry, cache_file, indent=2) + os.replace(temporary_cache_path, cache_file_path) + temporary_cache_path = None + except OSError as error: + logger.debug("Failed to update cache access metadata for %s: %s", cache_file_path.name, error) + finally: + if temporary_cache_path is not None: + with suppress(OSError): + temporary_cache_path.unlink(missing_ok=True) + @staticmethod def _result_from_cache_entry( cache_entry: dict[str, Any], diff --git a/tests/cache/test_cache_correctness.py b/tests/cache/test_cache_correctness.py index 5978d0d1e..aab254267 100644 --- a/tests/cache/test_cache_correctness.py +++ b/tests/cache/test_cache_correctness.py @@ -5392,6 +5392,85 @@ def checked_replace(source: str | os.PathLike[str], destination: str | os.PathLi assert replace_calls[0][1].is_file() +@pytest.mark.parametrize("lookup_kind", ["path", "key"]) +def test_cache_hit_keeps_published_entry_readable_during_access_update( + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, + lookup_kind: str, +) -> None: + file_path = _make_cacheable_file(tmp_path, name="atomic-hit.cache") + cache = ScanResultsCache(str(tmp_path / "scan-cache")) + version_context = build_cache_version_context({"timeout": 30}) + expected = {"checks": [], "issues": [], "metadata": {}, "scanner": "test", "success": True} + + assert cache.store_result( + str(file_path), expected, version_context=version_context, **_identity_kwargs(cache, str(file_path)) + ) + cache_key = cache.generate_cache_key(str(file_path), version_context=version_context) + assert cache_key is not None + cache_file_path = cache._get_cache_file_path(cache_key) + observed_entries: list[dict[str, Any]] = [] + original_dump = json.dump + + def observe_published_entry(value: Any, destination: Any, *args: Any, **kwargs: Any) -> None: + if isinstance(value, dict) and value.get("cache_key") == cache_key: + observed_entries.append(json.loads(cache_file_path.read_text(encoding="utf-8"))) + original_dump(value, destination, *args, **kwargs) + + monkeypatch.setattr(scan_results_cache_module.json, "dump", observe_published_entry) + + if lookup_kind == "path": + result = cache.get_cached_result(str(file_path), version_context=version_context) + else: + result = cache.get_cached_result_by_key(cache_key, file_path=str(file_path), version_context=version_context) + + assert result == expected + assert len(observed_entries) == 1 + assert observed_entries[0]["scan_result"] == expected + assert json.loads(cache_file_path.read_text(encoding="utf-8"))["cache_metadata"]["access_count"] == 2 + + +@pytest.mark.parametrize("lookup_kind", ["path", "key"]) +def test_cache_hit_preserves_published_entry_when_access_update_fails( + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, + lookup_kind: str, +) -> None: + file_path = _make_cacheable_file(tmp_path, name="failed-hit-update.cache") + cache = ScanResultsCache(str(tmp_path / "scan-cache")) + version_context = build_cache_version_context({"timeout": 30}) + expected = {"checks": [], "issues": [], "metadata": {}, "scanner": "test", "success": True} + + assert cache.store_result( + str(file_path), expected, version_context=version_context, **_identity_kwargs(cache, str(file_path)) + ) + cache_key = cache.generate_cache_key(str(file_path), version_context=version_context) + assert cache_key is not None + cache_file_path = cache._get_cache_file_path(cache_key) + original_dump = json.dump + + def interrupt_entry_update(value: Any, destination: Any, *args: Any, **kwargs: Any) -> None: + if isinstance(value, dict) and value.get("cache_key") == cache_key: + raise OSError("simulated interrupted cache access update") + original_dump(value, destination, *args, **kwargs) + + with monkeypatch.context() as patch: + patch.setattr(scan_results_cache_module.json, "dump", interrupt_entry_update) + if lookup_kind == "path": + result = cache.get_cached_result(str(file_path), version_context=version_context) + else: + result = cache.get_cached_result_by_key( + cache_key, + file_path=str(file_path), + version_context=version_context, + ) + + assert result == expected + assert json.loads(cache_file_path.read_text(encoding="utf-8"))["scan_result"] == expected + assert not list(cache_file_path.parent.glob(f".{cache_file_path.name}.*.tmp")) + assert cache.get_cached_result(str(file_path), version_context=version_context) == expected + + def test_store_result_discards_private_entry_when_final_identity_check_fails( tmp_path: Path, monkeypatch: pytest.MonkeyPatch,