diff --git a/changelog.d/1921-llm-credential-cache.changed.md b/changelog.d/1921-llm-credential-cache.changed.md new file mode 100644 index 000000000..80fe362f8 --- /dev/null +++ b/changelog.d/1921-llm-credential-cache.changed.md @@ -0,0 +1,21 @@ +- **Memoized the per-agent-build LLM credential read (#1921).** + `opencontractserver/llms/model_factory.py::_get_db_credentials` ran on every + agent build — every chat message, structured-output call, and + memory-curation task — and resolved the provider's secret through + `PipelineSettings.get_full_component_settings`, which reads the encrypted + secret store twice and so derived the Fernet key with the deliberately + expensive PBKDF2 KDF (480k HMAC iterations) twice per build whenever any + provider key was configured. The resolved per-provider credentials are now + cached in-process keyed on `(class_path, PipelineSettings.modified)` — the + same key the reranker/embedder instance caches use + (`opencontractserver/pipeline/utils.py`). Repeat builds skip the decryption, + while live rotation is preserved: a superuser key change calls + `PipelineSettings.save()`, which bumps `modified` (auto_now) and clears the + singleton cache, so the next build misses the memo and re-decrypts — no + redeploy, and no staleness beyond the existing 5-minute `PipelineSettings` + cache TTL (the live-configurability guarantee of #1897 is intact, as is the + DB-wins / env-fallback precedence). New `invalidate_credential_cache()` + mirrors the existing `invalidate_embedder_cache` / `invalidate_reranker_cache` + helpers (wired into `conftest.py` for test isolation; call it after any + out-of-band singleton write that bypasses `save()`). Tests: + `opencontractserver/tests/test_llm_model_factory.py::TestCredentialCache`. diff --git a/docs/architecture/llms/README.md b/docs/architecture/llms/README.md index 8d9074714..40e6f6ec9 100644 --- a/docs/architecture/llms/README.md +++ b/docs/architecture/llms/README.md @@ -2868,6 +2868,8 @@ Resolution is **DB-wins / env-fallback**, applied by [`opencontractserver/llms/m Any failure to build a credentialed model degrades to the env-fallback string, so a misconfiguration can never take the chat path down. The factory is invoked at every `make_pydantic_ai_agent` call site (document, corpus, and structured-output agents, plus the memory-curation tasks); it performs ORM access, so async call sites use the `abuild_agent_model()` wrapper. +Because the factory runs on every agent build, the resolved per-provider credentials are memoized in-process keyed on `(class_path, PipelineSettings.modified)` — the same cache key the reranker/embedder instance caches use. This skips the Fernet/PBKDF2 secret decryption on repeat builds while keeping rotation live: a superuser key change calls `PipelineSettings.save()`, which bumps `modified` and clears the singleton cache, so the next build misses the memo and re-decrypts (no redeploy, no staleness beyond the existing 5-minute `PipelineSettings` cache TTL). An out-of-band write that bypasses `save()` (e.g. `QuerySet.update`) should call `invalidate_credential_cache()`. + ### Validation `Corpus.save()` and `AgentConfiguration.save()` both run the resolver's `validate_model_spec()` — a malformed string or a provider with no registered `BaseLLMProvider` subclass raises `ValidationError({"preferred_llm": ...})`. The validator does not gate against `supported_models` so users aren't blocked from passing newly-released model names. Specs are normalised to canonical `"{provider}:{model}"` form on the way into the database. diff --git a/opencontractserver/conftest.py b/opencontractserver/conftest.py index 061eadf79..96ce00605 100644 --- a/opencontractserver/conftest.py +++ b/opencontractserver/conftest.py @@ -43,16 +43,18 @@ def _invalidate_pipeline_settings_singleton_cache(): migration-seeded state on cache miss. We also drop the process-local embedder/reranker *instance* caches - (``opencontractserver/pipeline/utils.py``). Those are keyed by - ``(class_path, PipelineSettings.modified)``; because ``modified`` is the - stable migration-seeded value across every ``TestCase`` test that doesn't - write the singleton, two tests that mock ``get_component_by_name`` to - return *different* classes for the *same* path would otherwise collide on - the cached instance from whichever test ran first. Clearing before each - test restores per-test isolation without every test needing its own - teardown. + (``opencontractserver/pipeline/utils.py``) and the LLM-provider + *credential* cache (``opencontractserver/llms/model_factory.py``). All are + keyed by ``(class_path, PipelineSettings.modified)``; because ``modified`` + is the stable migration-seeded value across every ``TestCase`` test that + doesn't write the singleton, two tests that mock ``get_component_by_name`` + to return *different* classes for the *same* path (or configure *different* + provider credentials) would otherwise collide on the cached value from + whichever test ran first. Clearing before each test restores per-test + isolation without every test needing its own teardown. """ from opencontractserver.documents.models import PipelineSettings + from opencontractserver.llms.model_factory import invalidate_credential_cache from opencontractserver.pipeline.utils import ( invalidate_embedder_cache, invalidate_reranker_cache, @@ -61,6 +63,7 @@ def _invalidate_pipeline_settings_singleton_cache(): PipelineSettings._invalidate_cache() invalidate_embedder_cache() invalidate_reranker_cache() + invalidate_credential_cache() yield diff --git a/opencontractserver/llms/model_factory.py b/opencontractserver/llms/model_factory.py index 2ca93dafd..383120281 100644 --- a/opencontractserver/llms/model_factory.py +++ b/opencontractserver/llms/model_factory.py @@ -31,6 +31,7 @@ from __future__ import annotations import logging +import threading from typing import Any from asgiref.sync import sync_to_async @@ -76,6 +77,49 @@ def _provider_class_path(provider_key: str) -> str | None: return definition.class_name if definition else None +# Process-local cache of resolved per-provider credentials keyed on +# ``(class_path, PipelineSettings.modified)`` — the same key the reranker and +# embedder instance caches use (``opencontractserver/pipeline/utils.py``). +# +# ``_get_db_credentials`` runs on *every* agent build — every chat message, +# every structured-output call, every memory-curation task. ``PipelineSettings`` +# already caches the singleton row (Django cache, 5-min TTL), so this is not a +# DB round-trip per call, but resolving a provider's secret still decrypts the +# Fernet store, and that decryption derives its key with a deliberately +# expensive PBKDF2 KDF (hundreds of thousands of HMAC iterations — see +# ``PipelineSettings._derive_key``). ``get_full_component_settings`` reads the +# secret store twice (merged settings + secrets overlay), so two KDF passes ran +# on every build once any provider key was configured (issue #1921). +# +# Caching the *resolved* creds amortises that across calls. Live-rotation is +# preserved: every credential write in production flows through +# ``PipelineSettings.save()``, which bumps ``modified`` (auto_now) and clears +# the singleton cache, so the next lookup misses on the new key and re-decrypts +# — a rotated or cleared key takes effect within the existing cache-TTL window +# with no redeploy (the live-configurability guarantee of issue #1897), and the +# cache adds no staleness beyond ``get_instance``'s own TTL. The same issue +# #1410 caveat applies: code paths that bypass ``save()`` (``QuerySet.update``, +# ``bulk_update``, raw migrations) won't bump ``modified``; call +# :func:`invalidate_credential_cache` explicitly in those cases. +_CREDENTIAL_CACHE: dict[tuple[str, Any], dict[str, str]] = {} +# Guards against two concurrent builds paying the decryption cost twice. +# Lookups after warm-up are read-only, so no lock is held on the hot path. +_CREDENTIAL_CACHE_LOCK = threading.Lock() + + +def invalidate_credential_cache() -> None: + """Drop cached per-provider credentials. + + In normal operation this is unnecessary — the cache key includes + ``PipelineSettings.modified`` so any settings write naturally invalidates + the cache on the next lookup across all workers. Kept for test isolation + and for callers that mutate the singleton out-of-band (a path that bypasses + ``save()`` and so leaves ``modified`` unchanged). + """ + with _CREDENTIAL_CACHE_LOCK: + _CREDENTIAL_CACHE.clear() + + def _get_db_credentials(provider_key: str) -> dict[str, str]: """Read DB-configured credentials for a provider from ``PipelineSettings``. @@ -83,6 +127,10 @@ def _get_db_credentials(provider_key: str) -> dict[str, str]: present when non-empty). Empty when the provider is unknown or has no credentials configured. Performs ORM access — invoke from a sync context or via :func:`abuild_agent_model`. + + The resolved creds are memoized per ``(class_path, PipelineSettings.modified)`` + so repeat builds skip the Fernet/PBKDF2 decryption; see the + ``_CREDENTIAL_CACHE`` comment for the invalidation contract. """ class_path = _provider_class_path(provider_key) if not class_path: @@ -91,7 +139,29 @@ def _get_db_credentials(provider_key: str) -> dict[str, str]: try: from opencontractserver.documents.models import PipelineSettings - stored = PipelineSettings.get_instance().get_full_component_settings(class_path) + instance = PipelineSettings.get_instance() + cache_key = (class_path, instance.modified) + + # Fast path: a copy so callers can never mutate the cached dict. + cached = _CREDENTIAL_CACHE.get(cache_key) + if cached is not None: + return dict(cached) + + with _CREDENTIAL_CACHE_LOCK: + # Double-check — another thread may have populated it while we waited. + cached = _CREDENTIAL_CACHE.get(cache_key) + if cached is not None: + return dict(cached) + + stored = instance.get_full_component_settings(class_path) + creds: dict[str, str] = {} + for key in ("api_key", "base_url"): + value = (stored or {}).get(key) + if isinstance(value, str) and value.strip(): + creds[key] = value.strip() + + _CREDENTIAL_CACHE[cache_key] = creds + return dict(creds) except _DB_READ_RECOVERABLE_ERRORS: # DB unavailable (migrations / early startup) — fall back to env. logger.debug( @@ -101,13 +171,6 @@ def _get_db_credentials(provider_key: str) -> dict[str, str]: ) return {} - creds: dict[str, str] = {} - for key in ("api_key", "base_url"): - value = (stored or {}).get(key) - if isinstance(value, str) and value.strip(): - creds[key] = value.strip() - return creds - async def aget_provider_credentials(provider_key: str) -> dict[str, str]: """DB-configured ``api_key`` / ``base_url`` for a provider (empty when unset). diff --git a/opencontractserver/tests/test_llm_model_factory.py b/opencontractserver/tests/test_llm_model_factory.py index 3b6a463b7..6d19f46cc 100644 --- a/opencontractserver/tests/test_llm_model_factory.py +++ b/opencontractserver/tests/test_llm_model_factory.py @@ -24,6 +24,7 @@ from opencontractserver.llms.model_factory import ( abuild_agent_model, build_agent_model, + invalidate_credential_cache, ) from opencontractserver.pipeline.base.settings_schema import ( get_secret_settings, @@ -209,3 +210,85 @@ def test_has_value_flips_after_setting_secret(self): self.assertTrue(schema["api_key"]["has_value"]) # The value itself is never exposed. self.assertIsNone(schema["api_key"]["current_value"]) + + +class TestCredentialCache(TestCase): + """Resolved provider creds are memoized per ``PipelineSettings.modified``. + + Regression coverage for issue #1921: the per-build credential read must + skip repeated Fernet/PBKDF2 decryption, yet a live key rotation (which + bumps ``modified`` via ``save()``) must still take effect without a + redeploy — preserving the live-configurability guarantee of issue #1897. + """ + + def setUp(self): + reset_registry() + self.addCleanup(reset_registry) + PipelineSettings.clear_cache() + self.addCleanup(PipelineSettings.clear_cache) + # The memo is process-local and survives TestCase rollback; clear it + # explicitly so the suite is isolated under the unittest runner too + # (the conftest autouse fixture only applies under pytest). + invalidate_credential_cache() + self.addCleanup(invalidate_credential_cache) + openai_defn = get_llm_provider_by_key_cached("openai") + assert openai_defn is not None + self.openai_path = openai_defn.class_name + + def _configure_openai_creds(self, *, api_key): + instance = PipelineSettings.get_instance() + instance.set_secrets({self.openai_path: {"api_key": api_key}}) + # save() bumps ``modified`` (auto_now) and clears the singleton cache. + instance.save() + + def test_resolution_memoized_across_builds(self): + """The second build for a provider reuses creds without re-decrypting.""" + self._configure_openai_creds(api_key="sk-db-key") + with mock.patch.object( + PipelineSettings, + "get_secrets", + autospec=True, + side_effect=PipelineSettings.get_secrets, + ) as get_secrets_spy: + first = build_agent_model("openai:gpt-4o") + second = build_agent_model("openai:gpt-4o") + # The first build decrypts once — get_full_component_settings reads the + # secret store twice (merged settings + secrets overlay) — and the + # second build is served from the memo without touching the store. + self.assertEqual(get_secrets_spy.call_count, 2) + self.assertIsInstance(first, Model) + self.assertIsInstance(second, Model) + + def test_rotated_key_takes_effect_without_redeploy(self): + """A key rotation via save() busts the memo (preserves #1897).""" + self._configure_openai_creds(api_key="sk-old") + seen_keys: list[str | None] = [] + + def _capture(provider_key, model_name, creds): + seen_keys.append(creds.get("api_key")) + return object() + + with mock.patch( + "opencontractserver.llms.model_factory._construct_model", + side_effect=_capture, + ): + build_agent_model("openai:gpt-4o") # warms the memo with sk-old + # Rotate exactly as a superuser would: System Settings → save(). + self._configure_openai_creds(api_key="sk-new") + build_agent_model("openai:gpt-4o") + + self.assertEqual(seen_keys, ["sk-old", "sk-new"]) + + def test_invalidate_credential_cache_forces_redecrypt(self): + """The explicit purge re-decrypts on the next build (out-of-band path).""" + self._configure_openai_creds(api_key="sk-db-key") + with mock.patch.object( + PipelineSettings, + "get_secrets", + autospec=True, + side_effect=PipelineSettings.get_secrets, + ) as get_secrets_spy: + build_agent_model("openai:gpt-4o") # 2 decrypt reads, then cached + invalidate_credential_cache() + build_agent_model("openai:gpt-4o") # memo purged → 2 more reads + self.assertEqual(get_secrets_spy.call_count, 4)