From 8b80bf5032f8768dcc4a6d48805bc2fd39764afd Mon Sep 17 00:00:00 2001 From: Alexander Date: Mon, 27 Jul 2026 11:04:31 -0400 Subject: [PATCH] fix(memory): make degraded a live property, not a boot-time constant (#1301) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reduced onto main after 610150d7 (#1357) landed the boot half of #1301 and all of #1300 independently. Dropped from the original PR #1360 as duplicate: `probe_hindsight` (main has `probe_health`, which additionally sends the Authorization header and takes an injectable transport), the `_build_hindsight_client` rewrite, and the whole `project:` unified-bank scoping — main's version of that is the better one (it treats a doc's `project:` tags as a set with OR semantics, and only stamps the tag in unified mode, so pre-unified multi-bank deployments' tag sets are untouched). What survives is the half 610150d7 does not implement at all: it adds no `degraded` on HindsightProvider, so `getattr(provider, "degraded", False)` stays False for the entire life of the process. A boot probe answers one question, once. A daemon that dies after boot leaves the HindsightProvider in place and the flag starts lying again — `/api/status.memory_degraded` and `hal0 memory status` report healthy while recalls come back empty and retains raise. That is the same false-healthy report #1301 was filed about, just displaced in time by one boot. - `_call` wraps every engine round-trip (retain, recall, list_memories x2, delete_document) and observes reachability instead of assuming it. Exceptions propagate untouched, so the existing fail-soft / 404-sweep handling is unchanged. - 4xx does NOT degrade — the daemon answered, and the delete sweep's routine per-bank 404s would otherwise flap the flag on every scoped delete. 5xx does, matching probe_health so boot and runtime agree on "up". - Not a one-way latch: a restarted daemon clears it. A boot probe structurally cannot do this. - `_mark_engine` is edge-triggered so an outage logs once, not once per call. - health.py + `hal0 memory status` wording corrected: degraded now covers two shapes (boot fallback, post-boot outage) and the old line asserted only the first. tests/memory/test_degrade_live.py covers the runtime half plus one anti-vacuity guard: main's boot tests monkeypatch `probe_health` itself, so they cannot distinguish "the probe ran and failed" from "the probe would never have reached the network" — the exact shape of the original bug. The added test points the factory at a real closed loopback port. Known remaining gap, deliberately not addressed here: the boot decision is still permanent in the other direction. A deployment that starts hal0-api before hindsight-api degrades to the volatile PgVectorProvider at boot and never re-promotes, however healthy the daemon later becomes. Co-Authored-By: Claude Opus 5 (1M context) --- CHANGELOG.md | 4 + src/hal0/api/routes/health.py | 9 +- src/hal0/cli/memory_commands.py | 7 +- src/hal0/memory/hindsight_provider.py | 95 ++++++++++- tests/memory/test_degrade_live.py | 220 ++++++++++++++++++++++++++ 5 files changed, 326 insertions(+), 9 deletions(-) create mode 100644 tests/memory/test_degrade_live.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 5ca6dad0c..5a2ed0a8a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -25,6 +25,10 @@ applying. Add those subsections to a version's section to surface them; see - **Pin the destructive `/api/memory` routes as ADMIN, above the generic prefix rule** (closes the last open slice of #1024). #1024's incident was a single unauthenticated `DELETE /api/memory/banks/{bank_id}` that cascade-deleted ~632 live records; its echoed-`?confirm=` guard (#1028) and audit row (#1030) landed, but the classification itself was only ever ADMIN *by generic prefix* and nothing asserted it. That matters because #1024's own follow-up proposes "keep reads open if desired", and the natural expression of that — a `_prefix("/api/memory")` CLIENT rule — silently takes the bank wipe with it under first-match-wins. Two narrow rules (`any DELETE under /api/memory`, `POST /api/memory/delete`) now sit *above* the generic memory row, and `exposure.DESTRUCTIVE_MEMORY_ROUTES` enumerates the irreversible surface so a new memory delete route, or a reclassification, has to touch the constant in the same diff. `tests/security/test_memory_delete_auth.py` asserts the constant tracks the live route table, that each route resolves via a pinned rule (not the ADMIN fallback), that all of them survive a simulated reads-are-CLIENT widening, and — against the real `AuthEnforcementMiddleware`, armed — that an anonymous call gets `401`, a client/inference key gets `403`, and the operator's admin key clears the gate. +### Fixed + +- **`memory_degraded` now tracks the live engine, not just the boot probe** (#1301, runtime half). The boot `/health` probe added in 1.0.0-rc.1 makes the hindsight→pgvector ladder fire when the daemon is down *at boot*, but it answers one question once and the answer goes stale immediately: a daemon that dies afterwards leaves the `HindsightProvider` in place, and `/api/status.memory_degraded` plus `hal0 memory status` went back to reporting healthy while every recall came back empty and every retain raised. `HindsightProvider.degraded` is now a live property fed by a single `_call` wrapper every engine round-trip funnels through — it flips on an observed transport failure and, unlike a boot probe, clears itself when the daemon comes back. A 4xx does not degrade (the daemon answered; the delete sweep's routine per-bank 404s must not flap it), a 5xx does — the same rule `probe_health` uses, so boot and runtime cannot disagree. The `hal0 memory status` line no longer claims "in-memory fallback" for what may be a failing durable engine. + ## [1.0.0-rc.1] — 2026-07-27 (R5 · the rework release) The R5 rework puts the platform back together as a genuine 1.0: memory and diff --git a/src/hal0/api/routes/health.py b/src/hal0/api/routes/health.py index 68c083aee..96a981a0f 100644 --- a/src/hal0/api/routes/health.py +++ b/src/hal0/api/routes/health.py @@ -55,8 +55,13 @@ def _disk_free_mb(path: Path) -> int: def _memory_degraded(request: Request) -> bool | None: """Return the memory degraded state for /api/status. - True → memory enabled, running on the volatile in-memory fallback. - False → memory enabled, using a real durable provider. + True → memory enabled but NOT healthy. Two shapes, deliberately reported + as one flag: the boot-degrade ladder swapped in the volatile + in-memory fallback (Hindsight was down at boot), OR the live + Hindsight daemon has stopped answering since boot (#1301 — the + provider tracks that itself, so this stays true after the boot + probe's answer goes stale). + False → memory enabled and the engine is answering. None → memory is disabled (no provider wired). """ provider = getattr(request.app.state, "memory_provider", None) diff --git a/src/hal0/cli/memory_commands.py b/src/hal0/cli/memory_commands.py index 67b3be917..176179c7b 100644 --- a/src/hal0/cli/memory_commands.py +++ b/src/hal0/cli/memory_commands.py @@ -115,9 +115,14 @@ def status_cmd( t.add_column("v") t.add_row("State", state) if enabled and degraded is True: + # #1301: ``degraded`` now covers BOTH shapes — the boot fallback (a + # volatile in-memory store) and a Hindsight daemon that died after + # boot (writes error, recalls come back empty). The old wording named + # only the first, so a post-boot outage printed a claim about the + # provider that was simply false. One line that is true of both. t.add_row( "Provider", - "[yellow]in-memory fallback (volatile — Hindsight unreachable)[/yellow]", + "[yellow]degraded — Hindsight unreachable (memory is volatile or failing)[/yellow]", ) elif enabled: t.add_row("Provider", "[green]durable[/green]") diff --git a/src/hal0/memory/hindsight_provider.py b/src/hal0/memory/hindsight_provider.py index 6fef9e8ae..6a35e4dac 100644 --- a/src/hal0/memory/hindsight_provider.py +++ b/src/hal0/memory/hindsight_provider.py @@ -15,6 +15,11 @@ client-orchestrated; we fan out to the caller's allowed banks and merge under one reranked token budget (recall returns NO numeric score, so the union is re-ranked via the :8086 reranker; §4b precedence is the tiebreak). +* **Live ``degraded`` flag** (#1301): reports whether the daemon is answering + *right now*, driven by observed transport failures on every engine call — + not a boot-time constant. The boot probe in ``hal0.memory`` decides which + provider gets built; this decides whether the one that got built is still + working. """ from __future__ import annotations @@ -23,8 +28,12 @@ from datetime import UTC, datetime from typing import Any +import structlog + from hal0.memory.provider import MemoryProvider +log = structlog.get_logger(__name__) + _SHARED = "shared" _PRIVATE = "private:" # The ``agents`` namespace is a federated agent-registry / @@ -185,12 +194,85 @@ def __init__( self._graph_enabled = bool(graph_enabled) self._extraction_slot = extraction_slot self._rerank_enabled = reranker is not None + # Live engine reachability (#1301). The factory only builds this + # provider after a successful boot probe, so False is the correct + # starting point; every engine call updates it (see ``_call``). + self._degraded = False @property def hindsight_client(self) -> Any: """REST client handle for the engine admin surface (memory_admin routes).""" return self._client + # ── Live engine health (#1301) ───────────────────────────────────── + + @property + def degraded(self) -> bool: + """True when the Hindsight daemon is NOT answering. + + The BOOT-time degrade ladder (``hal0.memory.provider_from_config`` + + ``hindsight_client.probe_health``) swaps in :class:`PgVectorProvider` + — whose ``degraded`` is a constant ``True`` — when the daemon is down + at boot. But a daemon that dies *after* boot leaves this provider in + place, and the boot probe has no opinion about that: before this, + ``/api/status.memory_degraded`` and ``hal0 memory status`` kept + reporting healthy while every recall came back empty and every retain + raised. A boot-only probe starts lying the moment the daemon dies. + + This flag tracks what the engine actually did on the last call, so the + reported state matches reality — and it clears itself when the daemon + comes back, which a boot probe cannot do either. + """ + return self._degraded + + @staticmethod + def _engine_answered(exc: Exception) -> bool: + """True when ``exc`` proves the daemon is up (it sent an HTTP response). + + Duck-typed on ``exc.response.status_code`` (via the module's existing + :func:`_http_status`) so the fake clients in tests need no httpx. A 5xx + counts as NOT answering — it matches the boot probe's rule in + ``hindsight_client.probe_health``, and a daemon returning 500s is not + usable memory. 4xx passes: a 404 on one bank during a delete sweep, or + a 401 from a stale key, does not mean the engine is down. + """ + code = _http_status(exc) + return code is not None and code < 500 + + def _mark_engine(self, *, reachable: bool, error: str | None = None) -> None: + """Record the outcome of one engine call; log only on a state change. + + Edge-triggered on purpose: a down daemon is hit on every recall, and an + unconditional log would emit a line per call for as long as the outage + lasts. + """ + if reachable == (not self._degraded): + return + self._degraded = not reachable + if reachable: + log.info("hal0.memory.hindsight_recovered") + else: + log.warning("hal0.memory.hindsight_unreachable_runtime", error=error) + + async def _call(self, method: str, /, **kwargs: Any) -> Any: + """Invoke a client method, updating the live ``degraded`` flag. + + Every engine round-trip funnels through here so reachability is + OBSERVED rather than assumed — that is the whole point, and it is why + this is a wrapper and not a periodic background poll: the signal is + already there in the calls the process is making anyway. + + Exceptions propagate untouched, so callers keep their existing + fail-soft / 404-sweep handling exactly as before. + """ + try: + out = await getattr(self._client, method)(**kwargs) + except Exception as exc: + self._mark_engine(reachable=self._engine_answered(exc), error=str(exc)) + raise + self._mark_engine(reachable=True) + return out + # ── ACL: the caller's allowed namespaces → banks ─────────────────── def _allowed_namespaces(self, requested: str | list[str], client_id: str | None) -> list[str]: @@ -384,7 +466,8 @@ async def add( context = meta.pop("context", None) or meta.get("source") or f"{agent} conversation turn" timestamp = meta.pop("timestamp", None) or _now() - resp = await self._client.retain( + resp = await self._call( + "retain", bank_id=bank, content=text, document_id=resolved_id, @@ -438,7 +521,7 @@ async def list_items( if len(items) >= limit: break try: - resp = await self._client.list_memories(bank_id=bank, limit=limit, offset=0) + resp = await self._call("list_memories", bank_id=bank, limit=limit, offset=0) except Exception: continue # fail-soft per bank for fact in resp.get("items", []): @@ -497,8 +580,8 @@ async def _deletable_ids( offset = 0 while wanted - out: try: - resp = await self._client.list_memories( - bank_id=bank, limit=_DELETE_SCAN_PAGE, offset=offset + resp = await self._call( + "list_memories", bank_id=bank, limit=_DELETE_SCAN_PAGE, offset=offset ) except Exception: break # fail-soft per bank; unresolved ids stay withheld @@ -549,7 +632,7 @@ async def delete( continue # withheld: not visible to this caller (fail-closed) for bank in banks: try: - res = await self._client.delete_document(bank_id=bank, document_id=document_id) + res = await self._call("delete_document", bank_id=bank, document_id=document_id) except Exception as exc: if _http_status(exc) == 404: continue # not in this bank — keep sweeping @@ -601,7 +684,7 @@ async def _one(bank: str) -> list[dict[str, Any]]: # that don't know the param stay compatible. if tags_match is not None: kwargs["tags_match"] = tags_match - resp = await self._client.recall(**kwargs) + resp = await self._call("recall", **kwargs) return [self._fact_to_item(f, bank) for f in resp.get("results", [])] per_bank = await asyncio.gather(*[_one(b) for b in banks]) diff --git a/tests/memory/test_degrade_live.py b/tests/memory/test_degrade_live.py new file mode 100644 index 000000000..58e689f76 --- /dev/null +++ b/tests/memory/test_degrade_live.py @@ -0,0 +1,220 @@ +"""#1301 (runtime half) — ``degraded`` must track the engine's LIVE state. + +``tests/memory/test_hindsight_boot_probe.py`` covers the boot half: a bounded +``/health`` probe in ``_build_hindsight_client`` makes ``provider_from_config``'s +degrade ladder actually fire, so a daemon that is down *at boot* yields a +``PgVectorProvider`` with ``degraded=True`` instead of a live-but-broken +``HindsightProvider`` reporting healthy. + +That is necessary and not sufficient. A boot probe answers one question, once, +and the answer goes stale immediately: a daemon that dies *after* boot leaves +the ``HindsightProvider`` in place, and a boot-only flag starts lying again the +moment it does. ``/api/status.memory_degraded`` and ``hal0 memory status`` then +report healthy while every recall comes back empty and every retain raises. + +So ``HindsightProvider.degraded`` is a live property fed by +:meth:`HindsightProvider._call`, which every engine round-trip funnels through. +This file is the regression guard for that half only. + +Two design rules are asserted here because both are load-bearing: + +* the flag is **not a one-way latch** — a restarted daemon clears it, which a + boot probe structurally cannot do; +* a 4xx does **not** degrade (the daemon answered — a stale key or a 404 on one + bank of a delete sweep is not an outage) while a 5xx does, matching + ``hindsight_client.probe_health``'s rule so boot and runtime cannot disagree. + +The end-to-end ladder test at the bottom deliberately uses a REAL closed +loopback port rather than a patched ``probe_health``: the original bug was that +no I/O happened at all, and a test that stubs the I/O away cannot see that. +""" + +from __future__ import annotations + +import socket +from types import SimpleNamespace + +import pytest + +from hal0.memory import provider_from_config +from hal0.memory.hindsight_provider import HindsightProvider +from hal0.memory.pgvector_provider import PgVectorProvider + + +def _cfg(engine: str = "hindsight") -> SimpleNamespace: + return SimpleNamespace( + memory=SimpleNamespace( + engine=engine, + embedding=SimpleNamespace( + rerank_gateway_url="http://127.0.0.1:8080", + rerank_model="builtin.jina-reranker-v1-tiny-en-q8", + rerank_connect_timeout_s=1.0, + rerank_read_timeout_s=8.0, + ), + graph=SimpleNamespace(enabled=False, extraction_slot="utility"), + ) + ) + + +def _closed_port_url() -> str: + """A loopback URL nothing is listening on (instant ECONNREFUSED).""" + sock = socket.socket() + sock.bind(("127.0.0.1", 0)) + port = sock.getsockname()[1] + sock.close() + return f"http://127.0.0.1:{port}" + + +class _FakeClient: + """Minimal Hindsight client whose outcome the test controls.""" + + def __init__(self, error: Exception | None = None) -> None: + self.error = error + + async def recall(self, **_kwargs: object) -> dict[str, list[object]]: + if self.error is not None: + raise self.error + return {"results": []} + + async def retain(self, **_kwargs: object) -> dict[str, str]: + if self.error is not None: + raise self.error + return {"operation_id": "op-1"} + + +class _HttpError(Exception): + """httpx.HTTPStatusError-shaped: the daemon answered, with a status.""" + + def __init__(self, status: int) -> None: + super().__init__(f"HTTP {status}") + self.response = SimpleNamespace(status_code=status) + + +# ── the flag tracks the live engine state ──────────────────────────────────── + + +@pytest.mark.asyncio +async def test_degraded_flips_true_when_engine_stops_answering() -> None: + """THE regression: a daemon that dies after boot kept reporting healthy.""" + client = _FakeClient() + p = HindsightProvider(client=client, client_id="hermes", unified_bank=True) + + await p.recall("q", dataset="shared", client_id="hermes") + assert p.degraded is False # boot state: engine answering + + client.error = ConnectionError("connection refused") + with pytest.raises(ConnectionError): + await p.recall("q", dataset="shared", client_id="hermes") + + assert p.degraded is True, "degraded still reports healthy for a dead engine" + + +@pytest.mark.asyncio +async def test_degraded_clears_when_engine_recovers() -> None: + """Not a one-way latch: a restarted daemon clears it. + + This is the property a boot probe cannot have at all — it has no later + observation to revise its answer with. + """ + client = _FakeClient(error=ConnectionError("connection refused")) + p = HindsightProvider(client=client, client_id="hermes", unified_bank=True) + + with pytest.raises(ConnectionError): + await p.recall("q", dataset="shared", client_id="hermes") + assert p.degraded is True + + client.error = None + await p.recall("q", dataset="shared", client_id="hermes") + assert p.degraded is False + + +@pytest.mark.asyncio +async def test_degraded_tracks_the_write_path_too() -> None: + """``add`` goes through the same accounting. + + A failed retain is engine state, not merely a caller-visible exception — + and writes are the calls whose silent loss is least recoverable. + """ + client = _FakeClient(error=ConnectionError("connection refused")) + p = HindsightProvider(client=client, client_id="hermes", unified_bank=True) + + with pytest.raises(ConnectionError): + await p.add("x", dataset="shared", client_id="hermes") + assert p.degraded is True + + +@pytest.mark.asyncio +async def test_http_4xx_does_not_mark_the_engine_degraded() -> None: + """NEGATIVE CONTROL — a 404/401 means the daemon IS up. + + Without this the delete sweep's routine per-bank 404s would flap the flag + on every scoped delete. + """ + client = _FakeClient(error=_HttpError(404)) + p = HindsightProvider(client=client, client_id="hermes", unified_bank=True) + + with pytest.raises(_HttpError): + await p.recall("q", dataset="shared", client_id="hermes") + assert p.degraded is False + + +@pytest.mark.asyncio +async def test_http_5xx_marks_the_engine_degraded() -> None: + """A daemon returning 5xx is not serving memory. + + Same rule as ``hindsight_client.probe_health``, so boot and runtime cannot + disagree about what "up" means. + """ + client = _FakeClient(error=_HttpError(503)) + p = HindsightProvider(client=client, client_id="hermes", unified_bank=True) + + with pytest.raises(_HttpError): + await p.recall("q", dataset="shared", client_id="hermes") + assert p.degraded is True + + +@pytest.mark.asyncio +async def test_status_reports_live_degrade() -> None: + """End of the wire: ``/api/status``'s reader sees the live flag. + + ``_memory_degraded`` does ``getattr(provider, "degraded", False)``, so this + is what actually decides the value an operator reads in the dashboard and + in ``hal0 memory status``. + """ + from hal0.api.routes.health import _memory_degraded + + client = _FakeClient() + p = HindsightProvider(client=client, client_id="hermes", unified_bank=True) + request = SimpleNamespace(app=SimpleNamespace(state=SimpleNamespace(memory_provider=p))) + + assert _memory_degraded(request) is False + + client.error = ConnectionError("connection refused") + with pytest.raises(ConnectionError): + await p.recall("q", dataset="shared", client_id="hermes") + + assert _memory_degraded(request) is True + + +# ── anti-vacuity: the boot ladder really does I/O ──────────────────────────── + + +def test_boot_ladder_engages_against_a_real_closed_port( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """Nothing below the factory is patched — including ``probe_health``. + + ``test_hindsight_boot_probe.py`` proves the ladder degrades when + ``probe_health`` raises, by monkeypatching ``probe_health`` to raise. That + is the right unit test, but it cannot distinguish "the probe ran and + failed" from "the probe would never have reached the network anyway" — + which is exactly the shape of the original #1301 bug (``from_env()`` did no + I/O). Point the factory at a really-closed loopback port and require the + real degrade. + """ + monkeypatch.setenv("HAL0_HINDSIGHT_URL", _closed_port_url()) + + provider = provider_from_config(_cfg("hindsight")) + + assert isinstance(provider, PgVectorProvider) + assert provider.degraded is True