diff --git a/HANDOFF.md b/HANDOFF.md index 6157998..847413c 100644 --- a/HANDOFF.md +++ b/HANDOFF.md @@ -2563,3 +2563,125 @@ to stop every Mem0 run from this environment. External launch remains **HELD**. --- +## 2026-08-03 — reset() verifies writability instead of hoping (founder-instructed) + +**Invariant 9 note, up front.** This change can convert phantom +`missing_current_fact` FAILs into passes — the guarded direction. It was +**instructed by the founder**, which is the sign-off invariant 9 requires +before merge, and this entry is the record of it. The test of good faith +named in the invariant holds: a genuine finding survives untouched, because +the sentinel only proves the namespace accepts writes again and is deleted +before any scenario value is written. Nothing about retrieval, supersession, +scoping or deletion behaviour is affected. + +### 1. What shipped + +`reset()` no longer treats "the namespace reads empty" as proof the +`delete_all` finished. When it actually deleted, it now proves the namespace +is writable again before returning. + +- Write a sentinel under identifiers no scenario maps onto, poll for it, + delete it, return. +- **Only on the delete path.** The skip path — namespace already empty, the + overwhelmingly common case — is untouched and still costs nothing. +- If the namespace never accepts a write within the timeout, **raise**. A + scenario is not started on a store that would swallow its first facts. +- **No blanket settle was added**, per instruction. + +### 2. Findings + +**One deliberate refinement of the brief: the sentinel retries.** The brief +said write a sentinel, poll, raise if it never lands. Implemented as a bounded +retry loop instead, because a single-shot sentinel is wrong in exactly the +case it exists for: a write swallowed by the still-reaping delete_all **never +becomes retrievable however long it is polled**. Polling harder cannot help; +only writing again can. A single-shot sentinel would therefore abort runs that +a second attempt moments later would have saved — trading a phantom FAIL for a +phantom abort. + +Each attempt gets a short window (`_SENTINEL_ATTEMPT_TIMEOUT` = 5s); the loop +as a whole is bounded by `_CONVERGE_TIMEOUT` = 30s, so it cannot hang. The +attempt count is recorded and logged. Say if you want the strict single-shot +version instead — it is a two-line change. + +**The measurement series, gathered free on every run.** Each reset that +deletes appends to `adapter.reset_convergence`: + +``` +{"app_id": ..., "empty_after_s": ..., "empty_to_writable_s": ..., + "sentinel_attempts": ...} +``` + +and prints to stderr: + +``` + [reset] mc_ns: namespace read empty after 2.5s, writable 1.5s later (2 sentinel attempt(s)) +``` + +`empty_to_writable_s` is the number nobody has had: **how long a delete_all +keeps reaping after it stops being visible to search**. One value is an +anecdote; a run produces one per deleting reset, and the distribution is the +real characterisation. This makes arms (f)/(g) a confirmation rather than the +only source — and it collects data on runs that pass, not just ones that +break. + +**Four tests added, 67 → 71 passing.** They use a `ReapingClient` whose +`delete_all` arms a window of swallowed writes *after* the namespace already +reads empty — the measured 6/14 behaviour: + +- the sentinel absorbs the reaping window, so the scenario's first real write + survives (without it, that write vanishes and is scored as a missing fact); +- no sentinel is left behind; +- the empty-to-writable gap is recorded, and is **not** recorded on the skip + path; +- reset raises rather than starting a scenario against an unwritable + namespace. + +Building those tests caught a defect in my first fake: it armed the reaping +from construction rather than from `delete_all`, which is not the condition — +reaping is a consequence of the delete. Fixed before it could validate the +wrong thing. + +**Sentinel cleanup uses a per-key delete by id, never a `delete_all`**, so the +cleanup cannot restart the condition it just cleared. Arms (a)-(c) measured +per-key delete followed by an immediate write landing at 0s, three for three, +which is the evidence for that choice. + +**Residual risk, stated.** A sentinel that lands *after* cleanup has run would +persist. It sits under a tenant/user pair no scenario maps onto, so no +scenario query can retrieve it, and the next `reset()` of that namespace +removes it. It would, however, make the namespace read non-empty, which turns +a future skip-path reset into a delete-path one. Bounded and self-healing, but +worth knowing. + +### 3. Decisions + +- **Retry rather than single-shot**, for the reason above. Flagged as a + deviation. +- **Verify only when we deleted.** Verifying on the skip path would add a + write and several reads per scenario per seed for a condition that cannot + arise — meaningful SEARCH spend for nothing. +- **Raise rather than continue** when the namespace stays unwritable. The + alternative is scoring the provider on facts our own reset destroyed. + +### 4. FOR STRATEGY + +- **This does not close the `012`/`003` question**, and should not be read as + doing so. It removes one harness-side mechanism by which a run can be + killed or a provider mis-scored. Whether the aborts were that mechanism is + still open — but the next run will now *measure* the gap rather than guess. +- The `003` abort still needs promoting properly, with its results filename + and latency curve. +- Unchanged: pinning `mem0ai` 2.0.14 for the regen, exact pins for extras, + whether #6017 changes the publication plan, `get_all` pagination. + +### 5. Next + +1. Run the regen or (f)/(g) on the Mac — either now yields the + empty-to-writable series as a side effect. +2. Promote the `003` abort with its curve. +3. Founder ruling before any published text changes. + +External launch remains **HELD**. + +--- diff --git a/src/memorycheck/adapters/mem0.py b/src/memorycheck/adapters/mem0.py index 56f8995..7e35c39 100644 --- a/src/memorycheck/adapters/mem0.py +++ b/src/memorycheck/adapters/mem0.py @@ -31,7 +31,9 @@ import os import re +import sys import time +import uuid from ..ledger import Scope from .base import AdapterError, MemoryAdapter, QueryResult, poll_until @@ -48,6 +50,17 @@ _CONVERGE_TIMEOUT = 30.0 _CONVERGE_INTERVAL = 0.5 +# Key/scope fragment for the post-delete_all sentinel. Distinctive so it can +# never collide with a scenario value, and scoped to identifiers no scenario +# maps onto, so a stray sentinel could not be read by a scenario query. +_SENTINEL = "mc_reset_sentinel" + +# Per-attempt window for a sentinel write to become retrievable. Short, because +# a swallowed write never appears however long we wait — the useful move is to +# try again, not to wait longer. The retry loop overall stays bounded by +# _CONVERGE_TIMEOUT. +_SENTINEL_ATTEMPT_TIMEOUT = 5.0 + def _slug(text: str) -> str: """Collapse arbitrary text to a Mem0-safe identifier fragment.""" @@ -90,6 +103,11 @@ def __init__(self) -> None: ) from e self._client = MemoryClient() # reads MEM0_API_KEY from the environment self._namespace = "default" + #: One entry per reset that actually deleted: how long the namespace + #: took to read empty, and how much longer until it accepted a write. + #: Read this after a run — it is the empirical answer to "how long does + #: a delete_all keep reaping", accumulated at no extra cost. + self.reset_convergence: list[dict] = [] # --------------------------------------------------------------- scoping @@ -117,12 +135,20 @@ def reset(self, namespace: str) -> None: So: read the namespace first, and skip the delete entirely when it is empty (the overwhelmingly common case — namespaces are per scenario and seed). When there really is residue, delete it and wait for the - namespace to read empty before returning, so no write can race it. + namespace to read empty. + + Reading empty is necessary but **not sufficient**: the delete can keep + reaping writes after it has stopped being visible to search, which is + how the 6/14 above happened. So when we did delete, we prove the + namespace is writable again by writing a sentinel and polling for it, + then removing it. That is correct at any propagation latency, unlike a + fixed sleep — a sleep is either too short to be safe or too long to be + affordable, and it can never tell you which. """ self._namespace = namespace or "default" app_id = self._app_id() if not self._namespace_rows(app_id): - return # nothing to clear, so nothing to race + return # nothing to clear, so nothing to race, and nothing to prove _call("reset delete_all", self._client.delete_all, app_id=app_id) cleared, waited = poll_until( lambda: not self._namespace_rows(app_id), @@ -135,6 +161,91 @@ def reset(self, namespace: str) -> None: f"{waited:.1f}s — refusing to run, since leftovers would be " "scored against the provider" ) + self._confirm_namespace_writable(app_id, empty_after=waited) + + def _confirm_namespace_writable(self, app_id: str, empty_after: float) -> None: + """Prove a just-wiped namespace accepts writes again, then clean up. + + Called only when `reset()` actually issued a delete_all, so the common + skip path costs nothing. Writes one sentinel, polls for it, deletes it. + + Why a sentinel rather than a settle: the question is not "has enough + time passed" but "does a write survive", and only a write can answer + it. This is correct whether propagation takes 200ms or 20s, and it + reports the number instead of assuming one. + + The sentinel is removed with a per-key delete by id, not a delete_all, + so the cleanup cannot restart the condition it just cleared — arms + (a)-(c) measured per-key delete followed by an immediate write as + landing at 0s, three for three. + """ + sentinel_scope = Scope(tenant_id=f"{_SENTINEL}_t", user_id=f"{_SENTINEL}_u") + uid = self._user_id(sentinel_scope) + started = time.monotonic() + attempts = 0 + landed = False + + # Re-issue the sentinel rather than writing one and hoping. A write + # swallowed by the still-reaping delete_all never becomes retrievable + # no matter how long we poll it, so a single-shot sentinel would abort + # a run that a second attempt moments later would have saved. Each + # attempt gets a short window; the loop as a whole is bounded by + # _CONVERGE_TIMEOUT, so this cannot hang. + while not landed: + attempts += 1 + value = f"{_SENTINEL}-{uuid.uuid4().hex[:12]}" + _call( + "reset sentinel write", + self._client.add, + f"{_SENTINEL}: {value}", + user_id=uid, + app_id=app_id, + metadata={"key": _SENTINEL}, + infer=False, + ) + remaining = _CONVERGE_TIMEOUT - (time.monotonic() - started) + if remaining <= 0: + break + landed, _ = poll_until( + lambda v=value: any( + v in (m.get("memory") or "") + for m in self._scope_memories(sentinel_scope) + ), + timeout=min(_SENTINEL_ATTEMPT_TIMEOUT, remaining), + interval=_CONVERGE_INTERVAL, + ) + if not landed and time.monotonic() - started >= _CONVERGE_TIMEOUT: + break + + took = time.monotonic() - started + if not landed: + raise AdapterError( + f"mem0 reset: namespace {app_id!r} read empty after " + f"{empty_after:.1f}s but was still not accepting writes " + f"{took:.1f}s later — {attempts} sentinel write(s) were " + "acknowledged and never became retrievable, so the delete_all " + "is still reaping. Refusing to start a scenario, because its " + "first writes would be lost and scored against the provider " + "as missing facts." + ) + # Remove the sentinel so it cannot be seen by any scenario query. + for mem in self._scope_memories(sentinel_scope): + if (mem.get("metadata") or {}).get("key") == _SENTINEL and mem.get("id"): + _call("reset sentinel delete", self._client.delete, mem["id"]) + + # The series this produces IS the characterisation of the reset race: + # empty-to-writable, measured on every reset that actually deleted, + # gathered for free on every run. A single number is an anecdote; this + # accumulates the distribution. + self.reset_convergence.append( + {"app_id": app_id, "empty_after_s": round(empty_after, 2), + "empty_to_writable_s": round(took, 2), "sentinel_attempts": attempts} + ) + print( + f" [reset] {app_id}: namespace read empty after {empty_after:.1f}s, " + f"writable {took:.1f}s later ({attempts} sentinel attempt(s))", + file=sys.stderr, + ) def write( self, scope: Scope, key: str, value: str, ttl_steps: int | None = None diff --git a/tests/test_mem0_adapter.py b/tests/test_mem0_adapter.py index e50ae94..520dcbd 100644 --- a/tests/test_mem0_adapter.py +++ b/tests/test_mem0_adapter.py @@ -21,6 +21,7 @@ import pytest +from memorycheck.adapters.base import AdapterError from memorycheck.adapters.mem0 import Mem0Adapter from memorycheck.judge import load_judge from memorycheck.ledger import Scope @@ -95,6 +96,7 @@ def offline_adapter(monkeypatch): adapter = Mem0Adapter.__new__(Mem0Adapter) adapter._client = FakeMem0Client() adapter._namespace = "default" + adapter.reset_convergence = [] return adapter @@ -169,6 +171,96 @@ def test_reset_deletes_when_the_namespace_has_residue(offline_adapter): assert a._client.delete_all_calls == before + 1 +class ReapingClient(FakeMem0Client): + """A client whose delete_all keeps swallowing writes for `reap_writes` + more calls *after* the namespace already reads empty. + + This is the measured Mem0 behaviour the sentinel exists for: 6/14 writes + lost when a delete_all preceded them. Reading empty is not proof the + delete finished. + """ + + def __init__(self, reap_writes: int) -> None: + super().__init__() + self.reap_budget = reap_writes + self._remaining = 0 + self.swallowed = 0 + + def delete_all(self, app_id=None, **kw): + result = super().delete_all(app_id=app_id, **kw) + # The namespace now reads empty, but the delete keeps reaping — which + # is precisely the state that reading empty fails to detect. + self._remaining = self.reap_budget + return result + + def add(self, messages, **kw): + if self._remaining > 0: + self._remaining -= 1 + self.swallowed += 1 + return {"results": []} # acknowledged, never stored + return super().add(messages, **kw) + + +def _adapter_with(client): + a = Mem0Adapter.__new__(Mem0Adapter) + a._client = client + a._namespace = "default" + a.reset_convergence = [] + return a + + +def test_reset_proves_the_namespace_is_writable_again(monkeypatch): + """The sentinel absorbs the reaped writes so the scenario's first real + write survives — the whole point of the fix.""" + monkeypatch.setattr("memorycheck.adapters.mem0.time.sleep", lambda _: None) + a = _adapter_with(ReapingClient(reap_writes=2)) + a.reset("ns") + a.write(ALICE, "plan", "starter-legacy-2024") + a.reset("ns") # now has residue -> issues delete_all -> sentinel path + + # The reaping window was consumed by sentinel attempts, not by the + # scenario's fact. Without the sentinel this write would have vanished + # and been scored as a missing current fact against the provider. + a.write(ALICE, "plan", "growth-2026") + assert "growth-2026" in a.query(ALICE, "which plan?").answer + assert a._client.swallowed >= 1 + + +def test_reset_leaves_no_sentinel_behind(monkeypatch): + monkeypatch.setattr("memorycheck.adapters.mem0.time.sleep", lambda _: None) + a = _adapter_with(FakeMem0Client()) + a.reset("ns") + a.write(ALICE, "plan", "starter-legacy-2024") + a.reset("ns") + assert not [r for r in a._client.rows if "sentinel" in str(r.get("memory", ""))] + + +def test_reset_records_the_empty_to_writable_gap(monkeypatch): + monkeypatch.setattr("memorycheck.adapters.mem0.time.sleep", lambda _: None) + a = _adapter_with(FakeMem0Client()) + a.reset("ns") + a.write(ALICE, "plan", "starter-legacy-2024") + assert a.reset_convergence == [] # no delete yet, nothing to measure + a.reset("ns") + assert len(a.reset_convergence) == 1 + assert "empty_to_writable_s" in a.reset_convergence[0] + + +def test_reset_raises_rather_than_starting_an_unwritable_scenario(monkeypatch): + """If the sentinel never lands, reset must abort. Proceeding would lose + the scenario's first writes and score them against the provider.""" + monkeypatch.setattr("memorycheck.adapters.mem0.time.sleep", lambda _: None) + monkeypatch.setattr("memorycheck.adapters.mem0._CONVERGE_TIMEOUT", 0.05) + a = _adapter_with(ReapingClient(reap_writes=10_000)) # never stops reaping + a.reset("ns") + a._client.rows.append( + {"id": "residue", "memory": "x", "metadata": {}, "user_id": "u", + "app_id": a._app_id()} + ) + with pytest.raises(AdapterError, match="not accepting writes"): + a.reset("ns") + + # ------------------------------------------------------------------ live layer live = pytest.mark.skipif(