feat(cache): expose cache entry ids and targeted invalidation

[nkanu17]

Closes #22

Adds an optional entry_id to CacheEntry, makes store() return the newly written entry identifier, and adds delete_by_id() to the provider contract so a coordinator can retire exactly the entry that was served without a semantic re-search or full clear().

Implementation notes:

• LangCacheProvider surfaces the managed entry_id from acheck() hits, returns the ID from astore(), and deletes via adelete_by_id().
• RedisVLCacheProvider standardizes on the unprefixed RedisVL cache entry ID. check() reads entry_id, store() normalizes the returned full Redis key to the same entry ID, and delete_by_id() passes that ID through SemanticCache.drop(ids=[...]). Identifiers from all three paths are interchangeable.
• The provider contract documents ID availability, single-entry semantics, and the delete_by_id() versus clear() distinction.
• Tests cover the issue acceptance scenario, ABC enforcement, RedisVL ID normalization, and explicit asyncio execution markers.

make check passes fully: format, lint, mypy, 124 passed, 9 skipped.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e68908c6ef

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[Copilot]

Pull request overview

This PR extends the semantic cache provider abstraction to surface stable per-entry identifiers and support targeted invalidation, enabling callers to delete exactly the entry that was served without a semantic re-search or clearing unrelated cache entries.

Changes:

• Adds an optional entry_id to CacheEntry and updates the provider contract so store() returns an optional identifier.
• Introduces delete_by_id() on BaseCacheProvider and implements it in both LangCacheProvider and RedisVLCacheProvider.
• Adds tests and documentation covering entry ID surfacing and single-entry invalidation semantics.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 10 comments.

File	Description
src/adk_redis/cache/_provider.py	Extends the provider contract and implementations to surface entry IDs and support delete_by_id().
tests/cache/test_provider.py	Adds contract enforcement tests and entry-id/invalidation behavior tests for both providers.
docs/concepts/caching.md	Documents entry IDs and targeted invalidation usage and backend-specific identifier semantics.

Suppressed comments (5)

src/adk_redis/cache/_provider.py:270

• RedisVLCacheProvider.delete_by_id() calls SemanticCache.drop(keys=[...]), but SemanticCache.drop is typically keyed by entry_id rather than a keys=[...] kwarg. This risks raising TypeError at runtime and prevents targeted invalidation.

    await asyncio.to_thread(self._cache.drop, keys=[entry_id])

tests/cache/test_provider.py:240

• The mocked SemanticCache.check() return value includes a "key" field and expects the provider to use it. If RedisVL identifiers are entry_id-based, mock only "entry_id" (and optionally "prompt") so the test validates the actual contract.

    mock_cache.check.return_value = [
        {
            "response": "An in-memory data store.",
            "vector_distance": 0.08,
            "entry_id": "abc123",

tests/cache/test_provider.py:248

• If CacheEntry.entry_id is the RedisVL SemanticCache entry ID, the assertion should compare against "abc123" (not a constructed Redis key).

    assert entry.entry_id == "test_cache:abc123"

tests/cache/test_provider.py:255

• This test assumes SemanticCache.store() returns a full Redis key. If it returns an entry_id identifier, the return_value and assertion should be updated so the test validates real behavior.

  async def test_store_returns_redis_key(self, mock_vectorizer):
    """store() returns the Redis key reported by the backend."""
    provider, mock_cache = _make_redisvl_provider(mock_vectorizer)
    mock_cache.store.return_value = "test_cache:abc123"

tests/cache/test_provider.py:264

• If RedisVLCacheProvider.delete_by_id() forwards an entry_id to SemanticCache.drop(entry_id), the test should call delete_by_id("abc123") and assert drop was called with that positional argument, rather than keys=[...].

  async def test_delete_by_id_drops_exact_key(self, mock_vectorizer):
    """delete_by_id() drops exactly the given Redis key."""
    provider, mock_cache = _make_redisvl_provider(mock_vectorizer)

    await provider.delete_by_id("test_cache:abc123")

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Suppressed comments (3)

tests/cache/test_provider.py:233

• These async tests are missing a pytest asyncio marker. This repo typically uses @pytest.mark.asyncio for async tests; without it, pytest may not execute them under an event loop.

class TestRedisVLCacheProviderEntryIds:
  """RedisVLCacheProvider surfaces RedisVL entry IDs."""

  async def test_check_surfaces_entry_id(self, mock_vectorizer):

tests/cache/test_provider.py:134

• These async tests are missing a pytest asyncio marker. This repo marks async tests with @pytest.mark.asyncio, and without it pytest will treat these as regular functions and fail to run them correctly.

This issue also appears on line 230 of the same file.

class TestLangCacheProviderEntryIds:
  """LangCacheProvider surfaces entry IDs and targeted invalidation."""

  async def test_check_surfaces_entry_id(self):

src/adk_redis/cache/_provider.py:258

• PR description says RedisVLCacheProvider "standardizes on the full Redis key as the entry identifier" with "no string munging", but this implementation strips the "{name}:" prefix and returns only the suffix. Please align the implementation/tests/docs (either return the full key everywhere, or update the PR description/contract text to match the unprefixed ID approach).

    key: str = await asyncio.to_thread(
        self._cache.store, prompt=prompt, response=response
    )
    key_prefix = f"{self._config.name}:"
    entry_id = key.removeprefix(key_prefix)

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Suppressed comments (3)

src/adk_redis/cache/_provider.py:260

• RedisVLCacheProvider.store() always returns a string, even if the backend returns an empty key. Since the provider contract is Optional[str], it should normalize empty identifiers to None so callers can reliably treat falsy IDs as unavailable.

    key_prefix = f"{self._config.name}:"
    entry_id = key.removeprefix(key_prefix)
    logger.debug("Stored response for prompt: %s", prompt[:50])
    return entry_id

docs/concepts/caching.md:139

• The example comment "same entry" is not guaranteed when checking a semantically similar but different prompt. Tweaking the comment avoids implying a strict identity match while still demonstrating that entry IDs are surfaced.

    print(hit.entry_id)  # same entry

src/adk_redis/cache/_provider.py:113

• BaseCacheProvider.delete_by_id() docstring promises that deleting a non-existent identifier is a no-op, but the interface does not enforce that behavior (implementations delegate directly to backends). Consider relaxing the contract wording to avoid documenting behavior that may not hold for all backends.

This issue also appears on line 257 of the same file.

    Unlike clear(), which removes every entry in the cache, this targets
    a single entry, identified by the entry_id obtained from check() or
    store(). Deleting an identifier that no longer exists is a no-op.