From 9affeabe749e6a153ba3e4d61412e91a7bb15991 Mon Sep 17 00:00:00 2001 From: Pringled Date: Tue, 23 Jun 2026 09:46:52 +0200 Subject: [PATCH 1/5] Fix MCP caching --- src/semble/mcp.py | 24 ++++++++++++++++++++++-- tests/test_mcp.py | 45 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 67 insertions(+), 2 deletions(-) diff --git a/src/semble/mcp.py b/src/semble/mcp.py index 6d4b426..c88ea76 100644 --- a/src/semble/mcp.py +++ b/src/semble/mcp.py @@ -11,7 +11,7 @@ from mcp.server.fastmcp import FastMCP from pydantic import Field -from semble.cache import save_index_to_cache +from semble.cache import get_validated_cache, save_index_to_cache from semble.index import SembleIndex from semble.index.dense import load_model from semble.types import ContentType @@ -199,9 +199,29 @@ def _build_and_cache_index(self, source: str, ref: str | None, model_path: str, def evict(self, source: str) -> None: self._tasks.pop(self._compute_cache_key(source), None) + async def _evict_if_stale(self, source: str, cache_key: str) -> None: + """Evict a cached local-path entry whose on-disk cache no longer matches its files.""" + cached = self._tasks.get(cache_key) + if ( + cached is None + or is_git_url(source) + or not cached.done() + or cached.cancelled() + or cached.exception() is not None + ): + return + validated = await asyncio.to_thread(get_validated_cache, cache_key, self._model_path, self._content) + if validated is None: + self.evict(source) + async def get(self, source: str, ref: str | None = None) -> SembleIndex: - """Return an index for the requested source, building and caching it on first access.""" + """Return an index for the requested source, building and caching it on first access. + + Local paths are revalidated against the on-disk cache on every call (the same + file-mtime check the CLI uses), so an entry is rebuilt once its files change. + """ cache_key = self._compute_cache_key(source, ref) + await self._evict_if_stale(source, cache_key) if cache_key not in self._tasks: model_path = await self._await_model() diff --git a/tests/test_mcp.py b/tests/test_mcp.py index b4e8afb..81aa903 100644 --- a/tests/test_mcp.py +++ b/tests/test_mcp.py @@ -135,6 +135,7 @@ async def test_index_cache_builds_and_caches( with ( patch(f"semble.mcp.SembleIndex.{patch_target}", return_value=fake_index) as mock_build, patch("semble.mcp.save_index_to_cache") as mock_save, + patch("semble.mcp.get_validated_cache", return_value=Path("/fake/cache")), ): first = await cache.get(resolved_source) second = await cache.get(resolved_source) @@ -144,6 +145,50 @@ async def test_index_cache_builds_and_caches( mock_save.assert_called_once_with(fake_index, cache._compute_cache_key(resolved_source)) +@pytest.mark.anyio +@pytest.mark.parametrize( + ("source", "patch_target", "expected_build_calls", "validate_called"), + [ + ("local_tmp_path", "from_path", 2, True), + ("https://github.com/org/repo", "from_git", 1, False), + ], + ids=["local_path_rebuilds_when_stale", "git_url_skips_revalidation"], +) +async def test_index_cache_staleness_check_scope( + cache: _IndexCache, + tmp_path: Path, + source: str, + patch_target: str, + expected_build_calls: int, + validate_called: bool, +) -> None: + """Local paths are revalidated (and rebuilt when stale) on every get(); git URLs never are.""" + resolved_source = str(tmp_path) if source == "local_tmp_path" else source + with ( + patch(f"semble.mcp.SembleIndex.{patch_target}", return_value=MagicMock()) as mock_build, + patch("semble.mcp.save_index_to_cache"), + patch("semble.mcp.get_validated_cache", return_value=None) as mock_validate, + ): + await cache.get(resolved_source) + await cache.get(resolved_source) + assert mock_build.call_count == expected_build_calls + assert mock_validate.called is validate_called + + +@pytest.mark.anyio +async def test_index_cache_skips_staleness_check_for_failed_task(cache: _IndexCache, tmp_path: Path) -> None: + """A cached entry that finished with an exception is not revalidated; it is left for the normal retry path.""" + + async def _raise() -> MagicMock: + raise RuntimeError("boom") + + cache._tasks[str(tmp_path.resolve())] = asyncio.create_task(_raise()) + await asyncio.sleep(0) # let the task finish + with patch("semble.mcp.get_validated_cache") as mock_validate: + await cache._evict_if_stale(str(tmp_path), str(tmp_path.resolve())) + mock_validate.assert_not_called() + + @pytest.mark.anyio async def test_index_cache_evicts_on_failure(cache: _IndexCache, tmp_path: Path) -> None: """A failed build evicts the entry so the next call can retry.""" From f1b68e96099d46bfcb2b8e31454ce6803b38a4d9 Mon Sep 17 00:00:00 2001 From: Pringled Date: Tue, 23 Jun 2026 09:47:36 +0200 Subject: [PATCH 2/5] Fix MCP caching --- src/semble/mcp.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/semble/mcp.py b/src/semble/mcp.py index c88ea76..87ad4c4 100644 --- a/src/semble/mcp.py +++ b/src/semble/mcp.py @@ -217,8 +217,8 @@ async def _evict_if_stale(self, source: str, cache_key: str) -> None: async def get(self, source: str, ref: str | None = None) -> SembleIndex: """Return an index for the requested source, building and caching it on first access. - Local paths are revalidated against the on-disk cache on every call (the same - file-mtime check the CLI uses), so an entry is rebuilt once its files change. + Local paths are revalidated against the on-disk cache on every call, + so an entry is rebuilt once its files change. """ cache_key = self._compute_cache_key(source, ref) await self._evict_if_stale(source, cache_key) From e4aaa4d2a0a4366194f1617e90ac77aaff67bf4b Mon Sep 17 00:00:00 2001 From: Pringled Date: Tue, 23 Jun 2026 09:54:23 +0200 Subject: [PATCH 3/5] Add minimum time --- src/semble/mcp.py | 26 +++++++++++++++++++++----- tests/test_mcp.py | 17 +++++++++++++++++ uv.lock | 2 +- 3 files changed, 39 insertions(+), 6 deletions(-) diff --git a/src/semble/mcp.py b/src/semble/mcp.py index 87ad4c4..2694765 100644 --- a/src/semble/mcp.py +++ b/src/semble/mcp.py @@ -3,6 +3,7 @@ import asyncio import json import logging +import time from collections import OrderedDict from collections.abc import Sequence from pathlib import Path @@ -25,6 +26,7 @@ ) _CACHE_MAX_SIZE = 10 # Max number of cached indexes to keep in memory +_MIN_REVALIDATE_FACTOR = 3 # Don't recheck staleness sooner than this many times the last build's duration async def _get_index( @@ -169,6 +171,7 @@ def __init__(self, content: Sequence[ContentType] = (ContentType.CODE,)) -> None self._model_ready = asyncio.Event() self._content = content self._tasks: OrderedDict[str, asyncio.Task[SembleIndex]] = OrderedDict() # ordered for LRU eviction + self._build_times: dict[str, tuple[float, float]] = {} # cache_key -> (finished_at, duration), in seconds async def _await_model(self) -> str: """Block until the model is installed; re-raise the load error if it failed.""" @@ -185,11 +188,14 @@ def _compute_cache_key(self, source: str, ref: str | None = None) -> str: def _build_and_cache_index(self, source: str, ref: str | None, model_path: str, cache_key: str) -> SembleIndex: """Build an index for the given source and cache it.""" + start = time.monotonic() index = ( SembleIndex.from_git(source, ref=ref, model_path=model_path, content=self._content) if is_git_url(source) else SembleIndex.from_path(cache_key, model_path=model_path, content=self._content) ) + finished = time.monotonic() + self._build_times[cache_key] = (finished, finished - start) try: save_index_to_cache(index, cache_key) except Exception: @@ -197,10 +203,16 @@ def _build_and_cache_index(self, source: str, ref: str | None, model_path: str, return index def evict(self, source: str) -> None: - self._tasks.pop(self._compute_cache_key(source), None) + cache_key = self._compute_cache_key(source) + self._tasks.pop(cache_key, None) + self._build_times.pop(cache_key, None) async def _evict_if_stale(self, source: str, cache_key: str) -> None: - """Evict a cached local-path entry whose on-disk cache no longer matches its files.""" + """Evict a cached local-path entry whose on-disk cache no longer matches its files. + + Skipped while inside the cooldown window so repos that are slow to build aren't + rebuilt faster than they can be served. + """ cached = self._tasks.get(cache_key) if ( cached is None @@ -210,6 +222,9 @@ async def _evict_if_stale(self, source: str, cache_key: str) -> None: or cached.exception() is not None ): return + finished_at, duration = self._build_times.get(cache_key, (0.0, 0.0)) + if time.monotonic() - finished_at < duration * _MIN_REVALIDATE_FACTOR: + return validated = await asyncio.to_thread(get_validated_cache, cache_key, self._model_path, self._content) if validated is None: self.evict(source) @@ -217,8 +232,8 @@ async def _evict_if_stale(self, source: str, cache_key: str) -> None: async def get(self, source: str, ref: str | None = None) -> SembleIndex: """Return an index for the requested source, building and caching it on first access. - Local paths are revalidated against the on-disk cache on every call, - so an entry is rebuilt once its files change. + Local paths are revalidated against the on-disk cache on every call (subject to a + cooldown scaled by build time), so an entry is rebuilt once its files change. """ cache_key = self._compute_cache_key(source, ref) await self._evict_if_stale(source, cache_key) @@ -228,7 +243,8 @@ async def get(self, source: str, ref: str | None = None) -> SembleIndex: # Re-check after the await: another caller may have populated the entry. if cache_key not in self._tasks: if len(self._tasks) >= _CACHE_MAX_SIZE: - self._tasks.popitem(last=False) + evicted_key, _ = self._tasks.popitem(last=False) + self._build_times.pop(evicted_key, None) self._tasks[cache_key] = asyncio.create_task( asyncio.to_thread(self._build_and_cache_index, source, ref, model_path, cache_key) ) diff --git a/tests/test_mcp.py b/tests/test_mcp.py index 81aa903..e16cb28 100644 --- a/tests/test_mcp.py +++ b/tests/test_mcp.py @@ -1,5 +1,6 @@ import asyncio import threading +import time from pathlib import Path from typing import Any from unittest.mock import MagicMock, patch @@ -175,6 +176,22 @@ async def test_index_cache_staleness_check_scope( assert mock_validate.called is validate_called +@pytest.mark.anyio +async def test_index_cache_skips_staleness_check_during_cooldown(cache: _IndexCache, tmp_path: Path) -> None: + """A slow-to-build local path is not revalidated again until its cooldown elapses.""" + cache_key = str(tmp_path.resolve()) + cache._tasks[cache_key] = asyncio.create_task(_succeed()) + await asyncio.sleep(0) # let the task finish + cache._build_times[cache_key] = (time.monotonic(), 10.0) # a build that took 10s, just finished + with patch("semble.mcp.get_validated_cache") as mock_validate: + await cache._evict_if_stale(str(tmp_path), cache_key) + mock_validate.assert_not_called() + + +async def _succeed() -> MagicMock: + return MagicMock() + + @pytest.mark.anyio async def test_index_cache_skips_staleness_check_for_failed_task(cache: _IndexCache, tmp_path: Path) -> None: """A cached entry that finished with an exception is not revalidated; it is left for the normal retry path.""" diff --git a/uv.lock b/uv.lock index 12994ca..f04d43a 100644 --- a/uv.lock +++ b/uv.lock @@ -10,7 +10,7 @@ resolution-markers = [ [options] exclude-newer = "0001-01-01T00:00:00Z" # This has no effect and is included for backwards compatibility when using relative exclude-newer values. -exclude-newer-span = "P3D" +exclude-newer-span = "P1W" [[package]] name = "annotated-doc" From 05a6ca920754900ff7ca19aa3cb6fb61e0fc419b Mon Sep 17 00:00:00 2001 From: Pringled Date: Tue, 23 Jun 2026 10:25:16 +0200 Subject: [PATCH 4/5] Improvements and version bump --- src/semble/mcp.py | 3 ++- src/semble/version.py | 2 +- tests/test_mcp.py | 20 ++++++++++++++++++++ 3 files changed, 23 insertions(+), 2 deletions(-) diff --git a/src/semble/mcp.py b/src/semble/mcp.py index 2694765..2d15abe 100644 --- a/src/semble/mcp.py +++ b/src/semble/mcp.py @@ -226,7 +226,8 @@ async def _evict_if_stale(self, source: str, cache_key: str) -> None: if time.monotonic() - finished_at < duration * _MIN_REVALIDATE_FACTOR: return validated = await asyncio.to_thread(get_validated_cache, cache_key, self._model_path, self._content) - if validated is None: + # Only evict if this entry hasn't already been replaced by a concurrent caller. + if validated is None and self._tasks.get(cache_key) is cached: self.evict(source) async def get(self, source: str, ref: str | None = None) -> SembleIndex: diff --git a/src/semble/version.py b/src/semble/version.py index 2310b95..f531d78 100644 --- a/src/semble/version.py +++ b/src/semble/version.py @@ -1,2 +1,2 @@ -__version_triple__ = (0, 4, 0) +__version_triple__ = (0, 4, 1) __version__ = ".".join(map(str, __version_triple__)) diff --git a/tests/test_mcp.py b/tests/test_mcp.py index e16cb28..fccfefb 100644 --- a/tests/test_mcp.py +++ b/tests/test_mcp.py @@ -206,6 +206,26 @@ async def _raise() -> MagicMock: mock_validate.assert_not_called() +@pytest.mark.anyio +async def test_index_cache_does_not_evict_entry_replaced_during_validation(cache: _IndexCache, tmp_path: Path) -> None: + """If a concurrent caller already replaced a stale entry, _evict_if_stale must not evict the new one.""" + cache_key = str(tmp_path.resolve()) + cache._tasks[cache_key] = asyncio.create_task(_succeed()) + await asyncio.sleep(0) + cache._build_times[cache_key] = (0.0, 0.0) # cooldown already elapsed + + replacement_task = object() + + def _replace_entry_then_report_stale(*args: object, **kwargs: object) -> None: + # Simulate a concurrent get() winning the race and installing a fresh task first. + cache._tasks[cache_key] = replacement_task # type: ignore[assignment] + return None + + with patch("semble.mcp.get_validated_cache", side_effect=_replace_entry_then_report_stale): + await cache._evict_if_stale(str(tmp_path), cache_key) + assert cache._tasks.get(cache_key) is replacement_task + + @pytest.mark.anyio async def test_index_cache_evicts_on_failure(cache: _IndexCache, tmp_path: Path) -> None: """A failed build evicts the entry so the next call can retry.""" From 394a6ee5cda63ead6c72f06128284b472ba075d9 Mon Sep 17 00:00:00 2001 From: Pringled Date: Tue, 23 Jun 2026 10:47:06 +0200 Subject: [PATCH 5/5] Change timing behavior, fix greptile feedback --- src/semble/mcp.py | 29 ++++++++++++++++++----------- tests/test_mcp.py | 7 +++++-- 2 files changed, 23 insertions(+), 13 deletions(-) diff --git a/src/semble/mcp.py b/src/semble/mcp.py index 2d15abe..1274b35 100644 --- a/src/semble/mcp.py +++ b/src/semble/mcp.py @@ -171,7 +171,7 @@ def __init__(self, content: Sequence[ContentType] = (ContentType.CODE,)) -> None self._model_ready = asyncio.Event() self._content = content self._tasks: OrderedDict[str, asyncio.Task[SembleIndex]] = OrderedDict() # ordered for LRU eviction - self._build_times: dict[str, tuple[float, float]] = {} # cache_key -> (finished_at, duration), in seconds + self._revalidate_after: dict[str, float] = {} # cache_key -> monotonic time, staleness check is gated until async def _await_model(self) -> str: """Block until the model is installed; re-raise the load error if it failed.""" @@ -188,24 +188,34 @@ def _compute_cache_key(self, source: str, ref: str | None = None) -> str: def _build_and_cache_index(self, source: str, ref: str | None, model_path: str, cache_key: str) -> SembleIndex: """Build an index for the given source and cache it.""" - start = time.monotonic() index = ( SembleIndex.from_git(source, ref=ref, model_path=model_path, content=self._content) if is_git_url(source) else SembleIndex.from_path(cache_key, model_path=model_path, content=self._content) ) - finished = time.monotonic() - self._build_times[cache_key] = (finished, finished - start) try: save_index_to_cache(index, cache_key) except Exception: logger.warning("Failed to save index cache for %r", cache_key, exc_info=True) return index + async def _build_and_track(self, source: str, ref: str | None, model_path: str, cache_key: str) -> SembleIndex: + """Build an index and, for local paths, record when its staleness cooldown ends. + + The cooldown write happens after the await, i.e. back on the event loop thread, + regardless of which thread `_build_and_cache_index` itself ran on. + """ + start = time.monotonic() + index = await asyncio.to_thread(self._build_and_cache_index, source, ref, model_path, cache_key) + if not is_git_url(source): + finished = time.monotonic() + self._revalidate_after[cache_key] = finished + (finished - start) * _MIN_REVALIDATE_FACTOR + return index + def evict(self, source: str) -> None: cache_key = self._compute_cache_key(source) self._tasks.pop(cache_key, None) - self._build_times.pop(cache_key, None) + self._revalidate_after.pop(cache_key, None) async def _evict_if_stale(self, source: str, cache_key: str) -> None: """Evict a cached local-path entry whose on-disk cache no longer matches its files. @@ -222,8 +232,7 @@ async def _evict_if_stale(self, source: str, cache_key: str) -> None: or cached.exception() is not None ): return - finished_at, duration = self._build_times.get(cache_key, (0.0, 0.0)) - if time.monotonic() - finished_at < duration * _MIN_REVALIDATE_FACTOR: + if time.monotonic() < self._revalidate_after.get(cache_key, 0.0): return validated = await asyncio.to_thread(get_validated_cache, cache_key, self._model_path, self._content) # Only evict if this entry hasn't already been replaced by a concurrent caller. @@ -245,10 +254,8 @@ async def get(self, source: str, ref: str | None = None) -> SembleIndex: if cache_key not in self._tasks: if len(self._tasks) >= _CACHE_MAX_SIZE: evicted_key, _ = self._tasks.popitem(last=False) - self._build_times.pop(evicted_key, None) - self._tasks[cache_key] = asyncio.create_task( - asyncio.to_thread(self._build_and_cache_index, source, ref, model_path, cache_key) - ) + self._revalidate_after.pop(evicted_key, None) + self._tasks[cache_key] = asyncio.create_task(self._build_and_track(source, ref, model_path, cache_key)) self._tasks.move_to_end(cache_key) task = self._tasks[cache_key] try: diff --git a/tests/test_mcp.py b/tests/test_mcp.py index fccfefb..77a4604 100644 --- a/tests/test_mcp.py +++ b/tests/test_mcp.py @@ -169,6 +169,9 @@ async def test_index_cache_staleness_check_scope( patch(f"semble.mcp.SembleIndex.{patch_target}", return_value=MagicMock()) as mock_build, patch("semble.mcp.save_index_to_cache"), patch("semble.mcp.get_validated_cache", return_value=None) as mock_validate, + # Disable the cooldown: real build duration (here, just thread-dispatch overhead) would + # otherwise sometimes exceed the gap between the two get() calls below, flaking the test. + patch("semble.mcp._MIN_REVALIDATE_FACTOR", 0), ): await cache.get(resolved_source) await cache.get(resolved_source) @@ -182,7 +185,7 @@ async def test_index_cache_skips_staleness_check_during_cooldown(cache: _IndexCa cache_key = str(tmp_path.resolve()) cache._tasks[cache_key] = asyncio.create_task(_succeed()) await asyncio.sleep(0) # let the task finish - cache._build_times[cache_key] = (time.monotonic(), 10.0) # a build that took 10s, just finished + cache._revalidate_after[cache_key] = time.monotonic() + 30.0 # a build that took 10s, just finished with patch("semble.mcp.get_validated_cache") as mock_validate: await cache._evict_if_stale(str(tmp_path), cache_key) mock_validate.assert_not_called() @@ -212,7 +215,7 @@ async def test_index_cache_does_not_evict_entry_replaced_during_validation(cache cache_key = str(tmp_path.resolve()) cache._tasks[cache_key] = asyncio.create_task(_succeed()) await asyncio.sleep(0) - cache._build_times[cache_key] = (0.0, 0.0) # cooldown already elapsed + cache._revalidate_after[cache_key] = 0.0 # cooldown already elapsed replacement_task = object()