fix(memory): remove a superseded memory from the search indexes (#2143)

## Description

Superseding a memory leaves the old version live in the search indexes,
so outdated content can still come back from search after supersession.

`HierarchicalMemory.supersede` updates the store and indexes the new
memory, but previously never removed the old entry from the vector/text
indexes. The store sets the old row's `valid_until` and `superseded_by`,
but the indexes keep their own cached metadata copy from first indexing.
Default search filters superseded rows from that cached metadata, so the
old entry could still look live and be returned with stale content.

Concretely: `add("User prefers Python")`, then `supersede(id, "User now
prefers JavaScript frameworks")`, then `search("Python")` could return
the superseded "prefers Python" entry alongside the new one. That
defeats supersession and can recall contradictory facts.

## Fix

After `store.supersede`, remove the old id from the vector and text
indexes, mirroring `delete`. The store keeps the old row for
`get_history`; only the search indexes are corrected. The new memory is
indexed as before.

## 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/core.py`: `supersede` removes the old id from the
vector and text indexes after the store supersession, before indexing
the new memory.
- `tests/test_memory/test_core_operations.py`: adds
`test_superseded_memory_does_not_resurface_in_search`.
- `CHANGELOG.md`: adds 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` in CI on the pre-merge head; fresh CI is
running on the main-merged head)
- [x] Linting passes (`ruff check` focused locally; previous CI `ruff
check` and `ruff format` passed before hitting unrelated mypy)
- [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/memory/core.py tests/test_memory/test_core_operations.py headroom/memory/factory.py
All checks passed!

uvx ruff@0.15.17 format --check headroom/memory/core.py tests/test_memory/test_core_operations.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_memory/test_core_operations.py::TestSupersede::test_superseded_memory_does_not_resurface_in_search -q
# assertion passed; local Windows teardown hit a locked temp SQLite file during fixture cleanup
```

## Real Behavior Proof

- Environment: Windows 11 review worktree, plus GitHub Actions on the
pre-merge head.
- Exact command / steps: ran focused ruff/format/diff checks; ran the
new supersede regression test directly.
- Observed result: focused checks passed; the new test body passed and
confirmed the old memory id does not resurface when searching for old
content while the new memory remains searchable. The local run then
errored during Windows temp SQLite cleanup after the assertion
completed.
- Not tested: full memory suite, because it pulls the ML embedding
stack. CI already passed the broader test matrix on the pre-merge head;
fresh checks are 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

Removing the old entry from indexes is intentionally aligned with
`delete`; the store row remains available for `get_history`.

---------

Co-authored-by: JerrettDavis <mxjerrett@gmail.com>
Co-authored-by: Tejas Chopra <chopratejas@gmail.com>
This commit is contained in:
Abhay Singh 2026-07-14 13:54:00 +05:30 committed by GitHub
parent eca3db62a3
commit fa330f3e2b
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
3 changed files with 76 additions and 1 deletions

View file

@ -114,6 +114,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`.
* **memory:** drop a superseded memory from the search indexes so it stops resurfacing. `supersede` set the old memory's `valid_until` in the store and indexed the new version, but never touched the old entry in the vector/text index. Those indexes keep a cached metadata copy (captured at index time with `valid_until=None`), and default search filters superseded rows off that cached copy — so the superseded, outdated version kept coming back from semantic/text search alongside the new one, injecting contradictory facts into recall. `supersede` now removes the old id from the vector and text indexes (mirroring `delete`); the store still keeps the row for `get_history`.
* **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.

View file

@ -562,6 +562,16 @@ class HierarchicalMemory:
# Perform supersession in store
new_memory = await self._store.supersede(old_memory_id, new_memory, supersede_time)
# Drop the OLD entry from the search indexes. The store keeps its row
# (valid_until is now set) so get_history still works, but the vector and
# text indexes hold a cached metadata copy with valid_until=None, and
# default search filters superseded rows off that cached copy. Without
# this, the superseded (stale) version keeps resurfacing from search
# alongside the new one, so contradictory/outdated facts get recalled
# together. Mirrors delete()'s index removal.
await self._vector_index.remove(old_memory_id)
await self._text_index.remove(old_memory_id)
# Update indexes
if new_memory.embedding is not None:
await self._vector_index.index(new_memory)

View file

@ -20,7 +20,9 @@ import os
os.environ["TOKENIZERS_PARALLELISM"] = "false"
import functools
import gc
import tempfile
import time
from pathlib import Path
import pytest
@ -70,7 +72,15 @@ def temp_db_path():
path = Path(f.name)
yield path
# Cleanup
path.unlink(missing_ok=True)
gc.collect()
for attempt in range(5):
try:
path.unlink(missing_ok=True)
break
except PermissionError:
if attempt == 4:
raise
time.sleep(0.1)
for suffix in ["-shm", "-wal", ".hnsw"]:
Path(str(path) + suffix).unlink(missing_ok=True)
@ -491,6 +501,60 @@ class TestSupersede:
found_ids = [r.memory.id for r in results]
assert new.id in found_ids
@pytest.mark.asyncio
@network_timeout_handler
async def test_superseded_memory_does_not_resurface_in_search(self, memory_system):
"""The superseded (old) version must not keep coming back from search.
The vector/text index cached the old entry's metadata with
valid_until=None; default search filters superseded rows off that cached
copy, so before the fix a search that matched the old content returned
the stale version alongside the new one.
"""
old = await memory_system.add(
content="User prefers Python",
user_id="alice",
)
new = await memory_system.supersede(
old.id,
"User now prefers JavaScript frameworks",
)
# A search matching the OLD content must not resurface the old entry.
results = await memory_system.search("Python", user_id="alice")
found_ids = [r.memory.id for r in results]
assert old.id not in found_ids
# The new version is still searchable.
new_results = await memory_system.search("JavaScript", user_id="alice")
assert new.id in [r.memory.id for r in new_results]
@pytest.mark.asyncio
@network_timeout_handler
async def test_superseded_index_removal_boundary(self, memory_system):
"""Boundary of the supersede index-removal (#2143).
supersede now drops the old id from the vector/text index (not just
flips valid_until on the cached copy), so a search matching the old
content will not surface it even with include_superseded=True the
entry is gone from the search index, not merely filtered. The store
still keeps the row, so get_history stays the source of truth for the
superseded version. This pins that contract so a future change that
relies on include_superseded search hitting the index fails loudly.
"""
old = await memory_system.add(content="User prefers Python", user_id="alice")
new = await memory_system.supersede(
old.id,
"User now prefers JavaScript frameworks",
)
incl = await memory_system.search("Python", user_id="alice", include_superseded=True)
assert old.id not in [r.memory.id for r in incl]
# Retained in the store for history/audit even though it left the index.
history = await memory_system.get_history(new.id)
assert old.id in [m.id for m in history]
# =============================================================================
# History Tests