mirror of
https://github.com/headroomlabs-ai/headroom.git
synced 2026-08-10 14:27:00 -04:00
fix(proxy): fail soft on a bad HEADROOM_QDRANT_PORT during config construction (#2141)
## Description A stray or typo'd `HEADROOM_QDRANT_PORT` can crash proxy startup during `ProxyConfig()` construction, even when memory is disabled. `memory_qdrant_port` is resolved through a dataclass field `default_factory`. That factory runs on every `ProxyConfig()` construction, regardless of whether memory or the `qdrant-neo4j` backend is enabled. The strict qdrant env parser raises on non-integer or out-of-range ports, so inherited values such as `HEADROOM_QDRANT_PORT=0`, `70000`, or `not-a-port` could prevent the proxy from starting for an off-by-default subsystem. ## Fix Use a fail-soft wrapper for the `ProxyConfig.memory_qdrant_port` default factory. Bad env values log a warning and fall back to the default port `6333`; valid env values are still honored. The strict `qdrant_env.qdrant_env_port()` parser remains unchanged for direct memory APIs and explicit tests, and the CLI `--memory-qdrant-port` option still validates `1..65535` before constructing `ProxyConfig`. ## 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/proxy/models.py`: add `_qdrant_env_port_or_default()` and use it for the proxy config qdrant port default. - `tests/test_proxy_config_qdrant_port.py`: cover bad env values falling back, valid env values passing through, and `ProxyConfig()` construction surviving bad env. - `CHANGELOG.md`: add a bug-fix entry. - Merged current `main` to pick up the repository-wide memory factory type annotation fix that was breaking the PR lint job. ## Testing - [x] Unit tests pass (`pytest` focused locally; broader CI passed on the pre-merge head and fresh CI is running on the main-merged head) - [x] Linting passes (`ruff check` focused locally) - [x] Type checking passes for the prior mypy blocker after merging current `main` - [x] New tests added for new functionality - [ ] Manual testing performed ### Test Output ```text uvx ruff@0.15.17 check headroom/proxy/models.py tests/test_proxy_config_qdrant_port.py headroom/memory/factory.py All checks passed! uvx ruff@0.15.17 format --check headroom/proxy/models.py tests/test_proxy_config_qdrant_port.py headroom/memory/factory.py 3 files already formatted git diff --check headroomlabs/main...HEAD # no output uv run --extra dev python -m pytest tests/test_proxy_config_qdrant_port.py -q 5 passed ``` ## Real Behavior Proof - Environment: Windows 11 review worktree, Python 3.13.3. - Exact command / steps: ran focused qdrant proxy config tests and targeted lint/format checks. - Observed result: invalid `HEADROOM_QDRANT_PORT` values fall back to `6333`; valid `6444` is honored; `ProxyConfig()` no longer raises during construction when env is bad. - Not tested: full suite; fresh CI is queued after the main merge. ## 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 This intentionally changes only the unconditional proxy config default path. Direct qdrant env parsing remains strict, and explicit CLI qdrant port input is still range-validated by Click. Co-authored-by: JerrettDavis <mxjerrett@gmail.com>
This commit is contained in:
parent
b6eb7a7613
commit
69aea2fc4f
3 changed files with 60 additions and 1 deletions
|
|
@ -113,6 +113,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
|
|||
* **wrap/doctor:** make the Claude Remote Control gate warning accurate and stop it firing for users who never had the feature ([#1779](https://github.com/headroomlabs-ai/headroom/issues/1779)). Claude Code 2.1.196 added a client-side check that **deterministically** disables first-party Remote Control (`/remote-control` / `/rc`) whenever `ANTHROPIC_BASE_URL` points at a non-`api.anthropic.com` host — which Headroom always does. The old notice hedged ("may hide the Remote Control menu"); it now states the disable as fact, names the `/rc` command, and detects the installed Claude Code version so the wording is exact (`2.1.196` when known, `2.1.196+` when not). The gate is upstream and RC's control-plane talks to `claude.ai` (not the API host), so Headroom cannot restore it — the warning tells you to run Claude without Headroom for RC sessions. The warning is suppressed for auth modes that never had Remote Control (API-key/PAYG via `ANTHROPIC_API_KEY`/`ANTHROPIC_AUTH_TOKEN`, and Bedrock/Vertex/Foundry cloud IAM) and on Claude Code builds older than 2.1.196 where RC is unaffected by a custom base URL. Both the `headroom wrap claude` launch banner and `headroom doctor` co-report the sibling base-URL gates Headroom *does* restore — on-demand tool loading (#746, automatic) and the 1M context window (#1158, via `--1m`) — and the wrap-side co-report is session-accurate: it says "already restored via --1m" when the flag is in effect and reports tool deferral OFF (not falsely "kept on") when the user chose `--tool-search false`/`ENABLE_TOOL_SEARCH=false`; the `ENABLE_TOOL_SEARCH=...` banner line got the same accuracy fix. `is_custom_anthropic_base_url` now recognizes scheme-less values (`myproxy.local:8080`, `127.0.0.1:8787`) as custom hosts and degrades gracefully on malformed URLs instead of crashing `doctor`. `doctor` resolves the Claude Code version lazily, so runs with no custom base URL never pay the `claude --version` subprocess. No request bytes are touched (cache-safe); this is UX/notice-only.
|
||||
* **install:** default the docker image to `ghcr.io/headroomlabs-ai/headroom:latest` instead of the dead `ghcr.io/chopratejas/headroom:latest`. After the repo moved to the `headroomlabs-ai` org, GHCR did not redirect the old package, so `headroom install` / `headroom init` and the install scripts pulled a frozen `0.27.0` image while current releases publish to the new path ([#1867](https://github.com/headroomlabs-ai/headroom/issues/1867)).
|
||||
* **transforms/content-router:** stop a profile-derived `read_protection_window` kwarg from weakening an explicit `--protect-tool-results` guarantee. `ContentRouter.apply()` computes `read_protection_window` from `protect_recent_reads_fraction`, where `0.0` (the sentinel `--protect-tool-results` sets) means "protect all excluded-tool output regardless of conversation depth" per #1374's documented contract — but the method then unconditionally overwrote that window with a `read_protection_window` kwarg whenever one was present. `proxy_pipeline_kwargs()` supplies that kwarg on every request from the active `AgentSavingsProfile.protect_recent` (the default `coding` profile sets `protect_recent=2`), so in practice only the last 2 messages ever kept read-protection and older excluded-tool output silently fell through to lossy compression. The runtime kwarg may now only narrow the window when `protect_recent_reads_fraction > 0`; it can no longer shrink the "protect everything" guarantee set by `--protect-tool-results`.
|
||||
* **proxy:** don't let a stray `HEADROOM_QDRANT_PORT` crash proxy startup. `ProxyConfig.memory_qdrant_port` used `qdrant_env.qdrant_env_port` as its field `default_factory`, and that function raises `ValueError` on a non-integer or out-of-range value. Because a `default_factory` runs on **every** `ProxyConfig()` construction, an inherited or typo'd `HEADROOM_QDRANT_PORT` crashed the proxy before it served a request — even though memory (and the qdrant backend) are off by default and unrelated to core proxying. The field now resolves the port through a fail-soft wrapper that falls back to the default (6333) with a warning; the strict `qdrant_env_port()` is unchanged for explicit qdrant setup.
|
||||
* **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 <hex>..<hex>`. 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.
|
||||
|
|
|
|||
|
|
@ -6,6 +6,7 @@ Extracted from server.py to keep the codebase maintainable.
|
|||
|
||||
from __future__ import annotations
|
||||
|
||||
import logging
|
||||
from dataclasses import InitVar, dataclass, field
|
||||
from datetime import datetime
|
||||
from typing import Any, Literal
|
||||
|
|
@ -13,6 +14,30 @@ from typing import Any, Literal
|
|||
from headroom.memory import qdrant_env
|
||||
from headroom.providers.registry import ProviderApiOverrides
|
||||
|
||||
logger = logging.getLogger(__name__)
|
||||
|
||||
|
||||
def _qdrant_env_port_or_default() -> int:
|
||||
"""Resolve ``HEADROOM_QDRANT_PORT``, falling back to the default on a bad value.
|
||||
|
||||
``qdrant_env.qdrant_env_port`` raises on an invalid port (intended for
|
||||
explicit qdrant setup). As a ``ProxyConfig`` field ``default_factory`` it
|
||||
runs on EVERY ``ProxyConfig()`` construction, regardless of whether
|
||||
memory/qdrant is enabled (both off by default), so a stray or typo'd
|
||||
``HEADROOM_QDRANT_PORT`` would crash proxy startup for an unrelated,
|
||||
off-by-default subsystem. Fail soft here so config construction never raises.
|
||||
"""
|
||||
try:
|
||||
return qdrant_env.qdrant_env_port()
|
||||
except ValueError:
|
||||
logger.warning(
|
||||
"Ignoring invalid HEADROOM_QDRANT_PORT; using default %d. "
|
||||
"Set a valid 1-65535 port to override.",
|
||||
qdrant_env.DEFAULT_QDRANT_PORT,
|
||||
)
|
||||
return qdrant_env.DEFAULT_QDRANT_PORT
|
||||
|
||||
|
||||
# =============================================================================
|
||||
# Data Models
|
||||
# =============================================================================
|
||||
|
|
@ -319,7 +344,7 @@ class ProxyConfig:
|
|||
# Qdrant connection (defaults resolve from HEADROOM_QDRANT_* env vars)
|
||||
memory_qdrant_url: str | None = field(default_factory=qdrant_env.qdrant_env_url)
|
||||
memory_qdrant_host: str = field(default_factory=qdrant_env.qdrant_env_host)
|
||||
memory_qdrant_port: int = field(default_factory=qdrant_env.qdrant_env_port)
|
||||
memory_qdrant_port: int = field(default_factory=_qdrant_env_port_or_default)
|
||||
memory_qdrant_api_key: str | None = field(default_factory=qdrant_env.qdrant_env_api_key)
|
||||
memory_neo4j_uri: str = "neo4j://localhost:7687"
|
||||
memory_neo4j_user: str = "neo4j"
|
||||
|
|
|
|||
33
tests/test_proxy_config_qdrant_port.py
Normal file
33
tests/test_proxy_config_qdrant_port.py
Normal file
|
|
@ -0,0 +1,33 @@
|
|||
"""ProxyConfig construction must survive a bad HEADROOM_QDRANT_PORT.
|
||||
|
||||
The port is resolved by a field default_factory that runs on every ProxyConfig()
|
||||
construction, so a stray/typo'd value must not crash proxy startup for an
|
||||
off-by-default subsystem."""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import pytest
|
||||
|
||||
from headroom.memory import qdrant_env
|
||||
from headroom.proxy.models import ProxyConfig, _qdrant_env_port_or_default
|
||||
|
||||
|
||||
@pytest.mark.parametrize("bad", ["not-a-port", "70000", "0"])
|
||||
def test_bad_qdrant_port_falls_back_to_default(monkeypatch, bad):
|
||||
monkeypatch.setenv("HEADROOM_QDRANT_PORT", bad)
|
||||
assert _qdrant_env_port_or_default() == qdrant_env.DEFAULT_QDRANT_PORT
|
||||
|
||||
|
||||
def test_valid_qdrant_port_is_honored(monkeypatch):
|
||||
monkeypatch.setenv("HEADROOM_QDRANT_PORT", "6444")
|
||||
assert _qdrant_env_port_or_default() == 6444
|
||||
|
||||
|
||||
def test_proxyconfig_construction_survives_bad_qdrant_port(monkeypatch):
|
||||
monkeypatch.setenv("HEADROOM_QDRANT_PORT", "not-a-port")
|
||||
|
||||
# Must not raise even though the port is unparseable (memory is off by
|
||||
# default and unrelated to core proxying).
|
||||
config = ProxyConfig()
|
||||
|
||||
assert config.memory_qdrant_port == qdrant_env.DEFAULT_QDRANT_PORT
|
||||
Loading…
Add table
Add a link
Reference in a new issue