diff --git a/CHANGELOG.md b/CHANGELOG.md index b2ac2e465..bd1beac72 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -125,6 +125,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 * **memory:** size the HNSW `index_batch` resize off the assigned-id high-water mark, not the live entry count. hnswlib never reclaims a slot on `mark_deleted` (used by remove/evict), so its usable capacity is bounded by the number of ids ever assigned (`_next_hnsw_id`). `index_batch` computed `required_capacity = len(self._memory_to_hnsw) + len(new_memories)` — the *live* count — which drops below `_next_hnsw_id` after deletion/eviction churn, so the resize was skipped and `add_items` raised `RuntimeError: number of elements exceeds the specified limit`, crashing the save path on the HNSW backend. It now resizes off `_next_hnsw_id`, matching the single-item `index()` guard. * **memory:** apply the `turn_id` scope filter even when `agent_id` is absent. In `SQLiteMemoryStore._build_query_conditions` the `turn_id` condition was nested inside the `agent_id` block, so a query filtered by `user_id` + `session_id` + `turn_id` (no `agent_id`) dropped the `turn_id` predicate entirely and returned every memory in the session instead of the single turn — an over-broad result that leaks sibling-turn memories into recall (and makes `count()` wrong for that scope). `agent_id` and `turn_id` are now applied independently. * **transforms:** stop the lossless `diff` fold from silently dropping lines out of non-diff content. `ContentRouter._lossless_first` tries every `compact_lossless` fold on all content, but the `diff` kind (`diff_strip_index`) is the only one with no exact-inverse check — it removes any line shaped like `index ..`. Applied to arbitrary text/log/search payloads that happen to contain such a line, that line was deleted with no CCR marker, so it was unrecoverable — a violation of the lossless no-loss contract the method's own docstring promises. The `diff` fold now runs only when the strategy is `DIFF` or the content is diff-shaped (`_looks_like_diff`); genuine diffs still have their `index` bookkeeping folded. +* **proxy:** reject a 0 `rate_limit_requests_per_minute` when rate limiting is enabled, instead of 500-ing every request. The token-bucket wait computation divides by the per-minute rate (`consume_from_bucket`), so a `rate_limit_requests_per_minute` of 0 raised `ZeroDivisionError` on every request that hit the limiter. The CLI already guards this with `IntRange(min=1)`, but the `HEADROOM_PROXY_CONFIG_JSON` / programmatic config paths bypassed it. `ProxyConfig.__post_init__` now validates `rate_limit_requests_per_minute >= 1` when `rate_limit_enabled` (mirroring the existing `retry_max_attempts` check), so a bad value fails fast at construction with a clear message; it stays inert when limiting is disabled. * **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. diff --git a/headroom/proxy/models.py b/headroom/proxy/models.py index 4e959d929..71e460b17 100644 --- a/headroom/proxy/models.py +++ b/headroom/proxy/models.py @@ -440,6 +440,15 @@ class ProxyConfig: def __post_init__(self, smart_routing: bool | None = None) -> None: if self.retry_enabled and self.retry_max_attempts < 1: raise ValueError("retry_max_attempts must be >= 1 when retry_enabled=True") + # A 0 (or negative) requests-per-minute limit divides by zero in the + # token-bucket wait computation (rate_limit_policy.consume_from_bucket), + # 500-ing every request. The CLI already guards this with IntRange(min=1); + # fail fast here too so the JSON/programmatic config paths can't produce a + # limiter that crashes at request time. Only matters when limiting is on. + if self.rate_limit_enabled and self.rate_limit_requests_per_minute < 1: + raise ValueError( + "rate_limit_requests_per_minute must be >= 1 when rate_limit_enabled=True" + ) @property def provider_api_overrides(self) -> ProviderApiOverrides: diff --git a/tests/test_proxy_config_rate_limit.py b/tests/test_proxy_config_rate_limit.py new file mode 100644 index 000000000..090e837c8 --- /dev/null +++ b/tests/test_proxy_config_rate_limit.py @@ -0,0 +1,30 @@ +"""ProxyConfig must reject a 0 requests-per-minute limit when rate limiting is on +(it would divide by zero in the token-bucket wait computation and 500 every +request), while leaving it inert when limiting is off.""" + +from __future__ import annotations + +import pytest + +from headroom.proxy.models import ProxyConfig + + +def test_zero_rpm_with_limiting_enabled_is_rejected(): + with pytest.raises(ValueError, match="rate_limit_requests_per_minute must be >= 1"): + ProxyConfig(rate_limit_enabled=True, rate_limit_requests_per_minute=0) + + +def test_negative_rpm_with_limiting_enabled_is_rejected(): + with pytest.raises(ValueError, match="rate_limit_requests_per_minute must be >= 1"): + ProxyConfig(rate_limit_enabled=True, rate_limit_requests_per_minute=-5) + + +def test_zero_rpm_is_inert_when_limiting_disabled(): + # Limiting off -> the bucket is never consulted, so a 0 limit is harmless. + config = ProxyConfig(rate_limit_enabled=False, rate_limit_requests_per_minute=0) + assert config.rate_limit_requests_per_minute == 0 + + +def test_valid_rpm_is_accepted(): + config = ProxyConfig(rate_limit_enabled=True, rate_limit_requests_per_minute=60) + assert config.rate_limit_requests_per_minute == 60