mirror of
https://github.com/headroomlabs-ai/headroom.git
synced 2026-08-27 14:17:10 -04:00
fix(memory): key the embedder cache on ollama_base_url (#2109)
## Description
The process-wide embedder cache can hand a caller an embedder bound to
the wrong Ollama server.
`_create_embedder` caches by `(backend, model)`:
```python
key = (
config.embedder_backend.value if hasattr(...) else str(...),
config.embedder_model or "",
)
```
But the Ollama branch constructs the embedder with the server URL:
```python
embedder = OllamaEmbedder(base_url=config.ollama_base_url, model_name=config.embedder_model)
```
So two configs in the same process that share a backend and model but
point at different Ollama servers (for example a per-project storage
router, or a fail-over host) collide on the same cache key. The first
call builds and caches an `OllamaEmbedder` bound to server A; the second
call, asking for server B, gets server A's embedder back and silently
embeds against the wrong host.
The code already reasoned about the analogous `openai_api_key` omission
and worked around it with an up-front validation guard (see the comment
above the key), but `ollama_base_url` has no such guard, so it just
resolves to the wrong server.
## Fix
Add `config.ollama_base_url` to the cache key. Same server still hits
the cache (one model load); a different server gets its own embedder.
Non-Ollama backends are unaffected (the URL just becomes an extra,
constant key component).
Closes #
## Type of Change
- [x] Bug fix (non-breaking change that fixes an issue)
- [ ] New feature (non-breaking change that adds functionality)
- [ ] Breaking change (fix or feature that would cause existing
functionality to change)
- [ ] Documentation update
- [ ] Performance improvement
- [ ] Code refactoring (no functional changes)
## Changes Made
- `headroom/memory/factory.py`: include `config.ollama_base_url` in the
embedder cache key, with a comment explaining why.
- `tests/test_memory/test_factory_embedder_cache.py`: new file with
`test_ollama_embedder_cache_keys_on_base_url` (different servers get
different embedders) and
`test_ollama_embedder_cache_reuses_same_base_url` (same server still
caches). Kept out of `test_factory.py` because that module skips
wholesale without `hnswlib`, which these cases don't need.
- `CHANGELOG.md`: Bug Fixes entry.
## Testing
- [ ] Unit tests pass (`pytest`)
- [x] Linting passes (`ruff check .`)
- [ ] Type checking passes (`mypy headroom`)
- [x] New tests added for new functionality
- [ ] Manual testing performed
### Test Output
```text
$ uvx ruff@0.15.17 check headroom/memory/factory.py tests/test_memory/test_factory_embedder_cache.py
All checks passed!
$ python -m py_compile headroom/memory/factory.py tests/test_memory/test_factory_embedder_cache.py
OK
```
## Real Behavior Proof
- Environment: Windows 11, Python 3.12, `uvx ruff@0.15.17`. Importing
`headroom` pulls in the torch/transformers stack and a full `pytest`
gets OOM-killed on this box, so I verified the cache-key behavior with a
dependency-free script that models the `(backend, model)` vs `(backend,
model, base_url)` keys against a simulated cache, and left the full
pytest to CI.
- Exact command / steps: created two configs with the same backend and
model but `ollama_base_url` of `http://gpu1:11434` and
`http://gpu2:11434`, and resolved each through the old key and the new
key against a shared cache.
- Observed result: the old key serves the same embedder object for both,
and the config asking for `gpu2` is handed the `gpu1`-bound embedder;
the new key gives each config its own embedder bound to its own server.
The new tests assert distinct embedders with the right `_base_url` for
different servers, and cache reuse for the same server.
- Not tested: a live Ollama round-trip (`OllamaEmbedder` construction is
offline — it stores the URL and lazily creates its client); full local
`pytest` deferred to CI (OOM, per above).
## Review Readiness
- [x] I have performed a self-review
- [x] This PR is ready for human review
## Checklist
- [x] My code follows the project's style guidelines
- [x] I have performed a self-review of my code
- [x] I have commented my code, particularly in hard-to-understand areas
- [ ] I have made corresponding changes to the documentation
- [x] My changes generate no new warnings
- [x] I have added tests that prove my fix is effective or that my
feature works
- [ ] New and existing unit tests pass locally with my changes
- [x] I have updated the CHANGELOG.md if applicable
## Additional Notes
The "unit tests pass locally" and "type checking" boxes are unchecked
because the full suite imports the ML stack, which I can't run in this
environment; the change adds one component to a cache-key tuple in a
pure function, verified by the standalone proof and the two new tests
for CI. The tests construct only the lightweight (offline) Ollama
embedder, so they don't need a running server or the vector-index deps.
Co-authored-by: Tejas Chopra <chopratejas@gmail.com>
This commit is contained in:
parent
6979b5245e
commit
1725cd1f83
3 changed files with 61 additions and 0 deletions
|
|
@ -102,6 +102,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
|
|||
|
||||
### Bug Fixes
|
||||
|
||||
* **memory:** include the Ollama server URL in the embedder cache key so a second backend can't get an embedder bound to the wrong server. `_create_embedder` cached by `(backend, model)` only, but the Ollama embedder is constructed with `base_url=config.ollama_base_url`. Two configs in the same process that shared a backend and model but pointed at different Ollama servers (e.g. a per-project storage router) collided on one cache slot, so the second silently reused the first's embedder and embedded against the wrong host. The cache key now also includes `ollama_base_url`.
|
||||
* **tokenizers:** use `o200k_base` for the gpt-4.1 / gpt-4.5 / o4 families in `get_encoding_for_model`. `gpt-4.1*` and `gpt-4.5*` matched the broad `gpt-4` prefix and were encoded with `cl100k_base`, and `o4*` matched no prefix and fell through to the `cl100k_base` default — all three use `o200k_base`, so their token counts were computed with the wrong vocabulary. Added explicit `gpt-4.1`/`gpt-4.5` prefixes ahead of `gpt-4` and an `o4` prefix; `gpt-4` and `gpt-3.5` snapshots still resolve to `cl100k_base`.
|
||||
* **cache/ccr:** stop counting a successful eviction as a retrieval in the compression feedback learner, which inverted the learning signal. When an entry is evicted without ever being retrieved, `CompressionStore` emits a synthetic `retrieval_type="eviction_success"` event to mark that the compression was sufficient (the LLM never needed the original). `CompressionFeedback.record_retrieval` had no branch for it, so — because the type is not `"full"` — it was counted as a *search retrieval*, inflating the tool's `retrieval_rate`/`search_rate`. `get_compression_hints` reads a high retrieval rate as "compressing too aggressively" and backs off, so a compression that actually worked pushed the learner toward *less* compression (a standalone repro scores one successful eviction as a 100% retrieval rate). The event is now recognized and left out of the retrieval counters; the compression is still counted by `record_compression` at store time, so a never-retrieved entry correctly yields a low retrieval rate. Genuine retrievals are unaffected.
|
||||
* **proxy/anthropic:** don't launder a non-2xx upstream into HTTP 200 when enterprise security scans the response. On the non-streaming `/v1/messages` path the response-scan branch rebuilt the reply as `httpx.Response(status_code=200)` and returned it without checking the upstream status, so a rate-limit (429), overloaded (529), or other 4xx error whose JSON body was scanned reached the client as an HTTP 200 — the client's retry/backoff never fired and an error looked like success. The branch is now gated on a 200 upstream, matching the sibling CCR/cache/buffered-stream blocks in the same handler; non-2xx responses fall through and keep their real status.
|
||||
|
|
|
|||
|
|
@ -173,6 +173,14 @@ def _create_embedder(config: MemoryConfig) -> Embedder:
|
|||
if hasattr(config.embedder_backend, "value")
|
||||
else str(config.embedder_backend),
|
||||
config.embedder_model or "",
|
||||
# The Ollama backend is built with ``base_url=config.ollama_base_url``,
|
||||
# so two configs that share a backend and model but point at different
|
||||
# Ollama servers must NOT share a cached embedder — otherwise the second
|
||||
# caller silently gets an embedder bound to the first server. (The
|
||||
# ``openai_api_key`` omission is handled by the up-front validation
|
||||
# above; ``ollama_base_url`` has no such guard and would just resolve to
|
||||
# the wrong host.)
|
||||
config.ollama_base_url or "",
|
||||
)
|
||||
|
||||
with _EMBEDDER_CACHE_LOCK:
|
||||
|
|
|
|||
52
tests/test_memory/test_factory_embedder_cache.py
Normal file
52
tests/test_memory/test_factory_embedder_cache.py
Normal file
|
|
@ -0,0 +1,52 @@
|
|||
"""The embedder cache must not serve an embedder bound to the wrong server.
|
||||
|
||||
Kept out of ``test_factory.py`` (which skips wholesale without hnswlib) because
|
||||
these cases only construct the lightweight Ollama embedder and need no vector
|
||||
index.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
from headroom.memory.config import EmbedderBackend, MemoryConfig
|
||||
from headroom.memory.factory import _create_embedder, _reset_embedder_cache_for_tests
|
||||
|
||||
|
||||
def test_ollama_embedder_cache_keys_on_base_url():
|
||||
"""Two configs that share backend + model but differ in ollama_base_url must
|
||||
not share a cached embedder — the second would otherwise get an embedder
|
||||
bound to the first server."""
|
||||
_reset_embedder_cache_for_tests()
|
||||
try:
|
||||
cfg1 = MemoryConfig(
|
||||
embedder_backend=EmbedderBackend.OLLAMA,
|
||||
embedder_model="nomic-embed-text",
|
||||
ollama_base_url="http://gpu1:11434",
|
||||
)
|
||||
cfg2 = MemoryConfig(
|
||||
embedder_backend=EmbedderBackend.OLLAMA,
|
||||
embedder_model="nomic-embed-text",
|
||||
ollama_base_url="http://gpu2:11434",
|
||||
)
|
||||
|
||||
e1 = _create_embedder(cfg1)
|
||||
e2 = _create_embedder(cfg2)
|
||||
|
||||
assert e1 is not e2
|
||||
assert e1._base_url == "http://gpu1:11434"
|
||||
assert e2._base_url == "http://gpu2:11434"
|
||||
finally:
|
||||
_reset_embedder_cache_for_tests()
|
||||
|
||||
|
||||
def test_ollama_embedder_cache_reuses_same_base_url():
|
||||
"""Same backend + model + base_url still hits the cache (one model load)."""
|
||||
_reset_embedder_cache_for_tests()
|
||||
try:
|
||||
cfg = MemoryConfig(
|
||||
embedder_backend=EmbedderBackend.OLLAMA,
|
||||
embedder_model="nomic-embed-text",
|
||||
ollama_base_url="http://gpu1:11434",
|
||||
)
|
||||
assert _create_embedder(cfg) is _create_embedder(cfg)
|
||||
finally:
|
||||
_reset_embedder_cache_for_tests()
|
||||
Loading…
Add table
Add a link
Reference in a new issue