Skip to content

fix(memory/holographic): wire retrieval_count into all retrieval paths via locked store helper - #73901

Open
ar-nim wants to merge 5 commits into
NousResearch:mainfrom
ar-nim:fix/holographic-retrieval-count
Open

fix(memory/holographic): wire retrieval_count into all retrieval paths via locked store helper#73901
ar-nim wants to merge 5 commits into
NousResearch:mainfrom
ar-nim:fix/holographic-retrieval-count

Conversation

@ar-nim

@ar-nim ar-nim commented Jul 29, 2026

Copy link
Copy Markdown

What does this PR do?

Fixes a dead-code bug where retrieval_count was permanently zero for all facts.

The plugin has two FTS5 search paths. They were written in the same commit (924bc67, PR #4623). They were never connected to each other.

  • store.search_facts() has the retrieval_count increment. It has zero callers.
  • FactRetriever.search() uses _fts_candidates(). It is the active path used by prefetch and fact_store(search). It does not call the increment.

retrieval.py was ported from a separate codebase ("KIK memory_agent.py" per its docstring) with its own FTS5 pipeline, duplicating the store-level search without inheriting the counter. The same gap existed in probe(), related(), and reason() — they also returned facts without recording retrievals.

The fix adds MemoryStore.increment_retrieval_count() under the shared self._lock and calls it from all 5 retrieval paths. Writing _conn directly would bypass MemoryStore's shared connection contract (one connection + one RLock for serialized access across coexisting provider instances). The helper centralizes the counter SQL so every caller uses the same serialized path.

After increment, each result dict is bumped by 1 so the response matches the DB (was always one behind — read 0 from DB, incremented DB to 1, returned 0).

Related Issue

N/A — small bug fix with clear root cause documented above.

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)

Changes Made

  • plugins/memory/holographic/store.py — Add MemoryStore.increment_retrieval_count() under the shared re-entrant self._lock. Reuse from search_facts() to eliminate duplicated SQL.
  • plugins/memory/holographic/retrieval.py — Call the locked helper from all 5 paths: search(), probe(), related(), reason(), and _score_facts_by_vector(). Bump each result dict after increment so response matches DB.
  • tests/plugins/memory/test_holographic_retrieval.py — 6 new tests: keyword search, accumulation, structural paths (probe, related, reason) with entity-bound facts, selectivity (non-returned facts stay at 0), and dict-DB consistency.

How to Test

  1. Run the full test suite: pytest tests/plugins/memory/test_holographic_retrieval.py -v
  2. Verify all 16 tests pass

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, test(scope):)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix (no unrelated commits)
  • I've run pytest tests/ -q and all tests pass
  • I've added tests for my changes (6 new tests, 16 total)
  • I've tested on my platform: Linux (openSUSE Leap 16 / Arch Linux)

Documentation & Housekeeping

  • I've updated relevant documentation (docstrings) — or N/A
  • I've updated cli-config.yaml.example — or N/A
  • I've updated CONTRIBUTING.md or AGENTS.md — or N/A
  • I've considered cross-platform impact — or N/A
  • I've updated tool descriptions — or N/A

Screenshots / Logs

pytest tests/plugins/memory/test_holographic_retrieval.py -v
============================== 16 passed in 0.69s ==============================

@ar-nim
ar-nim force-pushed the fix/holographic-retrieval-count branch from fa20625 to cf56f24 Compare July 29, 2026 05:52
…s via locked store helper

Fix a dead-code bug where retrieval_count was permanently zero. The counter
increment lived in store.search_facts() which has zero callers; the actual
retrieval path (FactRetriever.search()) bypassed it via _fts_candidates().

Changes:
- Add MemoryStore.increment_retrieval_count() under the shared re-entrant
  lock so all writes respect the serialized-access contract.
- Call it from every FactRetriever path that returns results: search(),
  probe(), related(), reason(), and _score_facts_by_vector().
- Reuse the helper from store.search_facts() to eliminate duplicated SQL.
- Add tests for keyword search (FTS5) and structural paths (HRR probe,
  related, reason) with entity-bound facts, including selectivity checks
  for non-returned facts.

Closes: NousResearch#17899
@ar-nim
ar-nim force-pushed the fix/holographic-retrieval-count branch from cf56f24 to 6ec315f Compare July 29, 2026 05:55
@ar-nim ar-nim changed the title fix(memory/holographic): wire retrieval_count into search path fix(memory/holographic): wire retrieval_count into all retrieval paths via locked store helper Jul 29, 2026
@alt-glitch alt-glitch added type/bug Something isn't working comp/plugins Plugin system and bundled plugins tool/memory Memory tool and memory providers P3 Low — cosmetic, nice to have labels Jul 29, 2026
ar-nim added 4 commits July 29, 2026 13:59
After increment_retrieval_count, bump each result dict's
retrieval_count by 1 so the response matches what the DB
now holds. Previously the dict contained the pre-increment
value, always one behind.
…rieval-count

# Conflicts:
#	tests/plugins/memory/test_holographic_retrieval.py
…al_count

increment_retrieval_count now returns {fact_id: new_count} via SQLite
RETURNING clause, so the dict sync is atomic — no race window between
UPDATE and read-back even under concurrent multi-session access.

Previously the dict sync used f.get('retrieval_count', 0) + 1 which
assumed no concurrent increment happened between the SELECT and UPDATE.
With multiple sessions reading the same fact, the dict could drift from
the true DB value.
@teknium1

Copy link
Copy Markdown
Contributor

Thanks for tracing this through the active provider paths. On current main, FactRetriever.search() calls _fts_candidates() at plugins/memory/holographic/retrieval.py:65 and returns results at :106-112 without updating retrieval_count; the existing update is confined to the separate MemoryStore.search_facts() implementation at plugins/memory/holographic/store.py:280-287. The live prefetch and fact_store actions call the retriever directly at plugins/memory/holographic/__init__.py:204-208 and :285-319.

The PR's locked helper and coverage for keyword and structural result paths fit the existing shared-connection design. The target files are unchanged on current main since the PR base, and GitHub reports the branch mergeable.

Automated hermes-sweeper review.

@teknium1 teknium1 added sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users area/memory Memory subsystem: store, providers, sync, background reviews labels Jul 30, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/memory Memory subsystem: store, providers, sync, background reviews comp/plugins Plugin system and bundled plugins P3 Low — cosmetic, nice to have sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users tool/memory Memory tool and memory providers type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants