Cache resolved LLM provider credentials per agent build

[JSv4]

What & why

Closes #1921.

opencontractserver/llms/model_factory.py::_get_db_credentials runs on every agent build — every chat message, every structured-output call, every memory-curation task (and the corpus-logo path via aget_provider_credentials). It resolves the provider's secret through PipelineSettings.get_full_component_settings, which reads the encrypted secret store twice (merged settings + secrets overlay). Each read decrypts the Fernet blob, and that decryption derives its key with the deliberately expensive PBKDF2 KDF (PIPELINE_SETTINGS_ENCRYPTION_ITERATIONS, default 480 000 HMAC iterations — see PipelineSettings._derive_key). So once any provider key was configured, two PBKDF2 passes ran on every single build. PipelineSettings.get_instance() is already cache-backed, so this was never a per-call DB round-trip, but the repeated decryption is pure wasted work.

How

Memoize the resolved per-provider credentials in-process, keyed on (class_path, PipelineSettings.modified) — the same cache key the reranker and embedder instance caches already use (opencontractserver/pipeline/utils.py). This is the codebase's established pattern for "expensive-to-resolve, lives on the PipelineSettings singleton, must stay live-configurable."

• Fast path is lock-free; a cold key takes a threading.Lock with the standard double-checked-locking guard so concurrent builds decrypt at most once. Returned dicts are copies, so a caller can never poison the cache.
• Live rotation is preserved (the Register LLM providers as DB-singleton pipeline components with live credentials & endpoints #1897 guarantee). Every credential write in production flows through PipelineSettings.save(), which bumps modified (auto_now) and clears the singleton cache. The next build misses the memo on the new key and re-decrypts — a rotated/cleared key takes effect with no redeploy, and the memo adds no staleness beyond get_instance()'s own 5-minute TTL. DB-wins / env-fallback precedence is unchanged.
• New invalidate_credential_cache() mirrors invalidate_embedder_cache / invalidate_reranker_cache; it's wired into conftest.py for per-test isolation and is the escape hatch for any out-of-band singleton write that bypasses save() (QuerySet.update, bulk_update, raw migrations — the same issue Extract Pipeline Cleanup #1410 caveat the sibling caches carry).

This implements the issue's first suggested direction (memoization keyed on modified) rather than restructuring the five make_pydantic_ai_agent call sites, so it covers every credential-resolution path — chat and image generation — at the single chokepoint, with no agent-construction churn.

Tests

opencontractserver/tests/test_llm_model_factory.py::TestCredentialCache:

• test_resolution_memoized_across_builds — two builds, secret store decrypted once (get_secrets called exactly twice = one get_full_component_settings).
• test_rotated_key_takes_effect_without_redeploy — rotate via save() mid-test; the second build picks up the new key (regression guard for the Register LLM providers as DB-singleton pipeline components with live credentials & endpoints #1897 live-config guarantee).
• test_invalidate_credential_cache_forces_redecrypt — explicit purge re-decrypts (out-of-band path).

black, isort (profile black), and flake8 are clean on the changed files; the changelog fragment passes scripts/collate_changelog.py --check. The Dockerized backend suite (which imports pydantic-ai) couldn't run in this environment, but the cache control-flow (memoization, copy-on-return, rotation invalidation, explicit invalidation, empty-creds caching, 16-thread single-decrypt) and the autospec+side_effect spy semantics the assertions rely on were both validated with standalone harnesses.

Notes

• No migration, no API change. Process-local cache; in multi-worker production each worker converges to a rotated key within the shared-Redis PipelineSettings TTL, exactly like the reranker/embedder caches.
• Docs: docs/architecture/llms/README.md API-keys section gains a paragraph on the memo + invalidation contract.

---

Generated by Claude Code

[claude]

Code Review

Overview

This PR memoizes resolved LLM provider credentials in _get_db_credentials() to avoid repeated Fernet/PBKDF2 decryption on every agent build. The fix is well-scoped: a single process-local cache keyed on (class_path, PipelineSettings.modified) intercepts the hot path, while live key rotation is preserved because save() bumps modified and clears the singleton cache.

---

Strengths

• Correct double-checked locking. The fast path reads outside the lock (safe under CPython's GIL for dict operations), the slow path holds _CREDENTIAL_CACHE_LOCK with a proper double-check. This matches the identical pattern in pipeline/utils.py for the reranker/embedder caches.
• Copy-on-return prevents cache poisoning. Both the fast path and the post-lock path return dict(cached) / dict(creds). Unlike the reranker/embedder caches (which return object instances), credential dicts are mutable, so this is a necessary and correct addition.
• Empty-creds path is cached. Providers with no configured credentials return {} (not None), which is correctly stored and served from cache — no repeat decryption attempts for unconfigured providers.
• Exception safety under lock. If get_full_component_settings raises a _DB_READ_RECOVERABLE_ERRORS exception while the lock is held, Python's with block releases the lock before the exception propagates to the except handler. The cache is left unpopulated and the next call retries — correct fallback semantics.
• Test coverage is solid. Three focused tests cover memoization, live rotation (the Register LLM providers as DB-singleton pipeline components with live credentials & endpoints #1897 regression guard), and explicit invalidation. setUp/addCleanup are used correctly to isolate each test.

---

Issues

Minor — conftest.py uses the deprecated _invalidate_cache() alias (not introduced by this PR)

opencontractserver/conftest.py:63 still calls PipelineSettings._invalidate_cache(), which is a deprecated backwards-compat alias for the public clear_cache() (see documents/models.py:1327-1334). The new test's setUp correctly uses PipelineSettings.clear_cache(). The alias works fine, but now that clear_cache exists as the canonical name it would be worth updating the conftest on a future pass (not a blocker here since the alias delegates to the public method).

Minor — _get_pipeline_cache_key() helper not reused

pipeline/utils.py:639 defines _get_pipeline_cache_key(class_path) as a shared helper for exactly this (class_path, PipelineSettings.get_instance().modified) computation; both the reranker and embedder caches use it. The credential cache inlines the same logic. This is actually the right call here — model_factory.py needs the instance reference anyway (for the subsequent get_full_component_settings call), so sharing the helper would require an extra get_instance() round-trip. But worth a code comment explaining why the inline form was preferred over the sibling helper, so future readers don't reach for it.

Low — stale entry accumulation between explicit invalidations

Each PipelineSettings.save() produces a new modified timestamp and thus a new cache key, but the old (class_path, old_modified) → creds entry is never evicted until invalidate_credential_cache() is called or the process restarts. In practice, each entry is a tiny dict with at most two short strings, and settings are saved rarely, so this is not a real concern. It is consistent with the same behaviour in the reranker/embedder caches. Worth a one-liner caveat in the _CREDENTIAL_CACHE comment block for future readers who wonder why the dict can have more than one entry per class path.

---

Nit

# model_factory.py
_CREDENTIAL_CACHE: dict[tuple[str, Any], dict[str, str]] = {}
# Guards against two concurrent builds paying the decryption cost twice.
_CREDENTIAL_CACHE_LOCK = threading.Lock()

"two concurrent builds" → more precisely "concurrent builds on the same key". The lock serialises any number of threads racing on a cold entry, not just two. Minor phrasing nit; the intent is clear from context.

---

Summary

The implementation is correct, follows the established caching pattern faithfully, and the tests cover the scenarios that matter (memoization, live rotation, explicit purge). No blocking issues. The three items above are all low-priority and two of them (conftest alias, stale-entry note) are pre-existing or cosmetic. Good to merge after a quick look at whether the inline cache-key logic deserves a comment.