fleet-memory/hindsight-api-slim/tests/conftest.py
Nicolò Boschi 914ba7962c
perf: 3-phase retain pipeline — fix deadlocks, cap temporal links, query-time entity expansion (#722)
* perf: 3-phase retain pipeline — fix deadlocks, cap temporal links, query-time entity expansion

Major retain pipeline overhaul addressing deadlocks, write amplification,
and TimeoutErrors. Restructures retain into three phases:

Phase 1: Entity resolution on separate connection (read-heavy)
Phase 2: Core write transaction (atomic) — facts, unit_entities, links
Phase 3: Best-effort display data (error-isolated) — entity viz links, stats

Key changes:
- Sorted bulk INSERT FROM unnest() prevents deadlocks
- Temporal links capped to top-20 per unit (95% reduction)
- Batched semantic ANN via temp table + LATERAL
- Query-time entity expansion via unit_entities self-join
- Entity viz links moved to Phase 3 (post-transaction)
- HINDSIGHT_API_RETAIN_MAX_CONCURRENT config (default: 32)

* fix: increase semantic link top_k from 5 to 20

The hardcoded top_k=5 was artificially limiting semantic link creation.
Link expansion retrieval can consume up to budget (50-200) semantic
neighbors per seed set, but each fact only had 5 outgoing edges — making
the bidirectional graph very sparse.

Increasing to 20 gives retrieval 4x more edges to work with. The ANN
probe cost is unchanged (same HNSW traversal per fact, just returning
more rows). INSERT cost is negligible (~14k rows via bulk INSERT).

Also: all 18 TimeoutErrors in the latest benchmark (beam-1m-u20) were
from Gemini LLM calls, zero from the database — confirming the entity
resolution split eliminated DB timeouts entirely.

* perf: move semantic ANN search to Phase 1 to avoid transaction timeouts

The batched LATERAL ANN query (700 HNSW probes) was the last remaining
source of DB TimeoutErrors — all 29 in the latest benchmark were from
create_semantic_links_batch inside the Phase 2 write transaction.

Split semantic link creation into three phases:
- Phase 1 (separate conn, autocommit): ANN search via temp table + LATERAL.
  No transaction locks, no contention with concurrent writers.
- Phase 2 (write transaction): within-batch numpy similarities (instant) +
  INSERT of both within-batch and Phase 1 ANN results. No DB reads.
- Phase 3 (flush_pending_stats): future hook point for re-checking ANN
  results after commit to catch links missed by concurrent batches.

Also adds 7 unit tests for compute_semantic_links_within_batch covering
empty input, identical/orthogonal embeddings, threshold filtering, top_k
cap, and tuple structure validation.

* fix: handle placeholder unit_ids in Phase 1 ANN search (not valid UUIDs)

* test: add Phase 1 ANN cross-batch test + configurable test PG port

- New test_semantic_links_phase1_ann_cross_batch verifies that the Phase 1
  ANN search with placeholder unit IDs correctly creates cross-batch
  semantic links after remapping to real IDs.
- Test PG port now configurable via HINDSIGHT_TEST_PG_PORT env var
  (default: 5556) to avoid conflicts with running benchmark daemons.

* perf: remove retry_with_backoff from retain, set semaphore default to 4

Remove retry_with_backoff from _run_db_work and _run_delta_db_work:
- Deadlocks are prevented by sorted bulk INSERT (no need for retry)
- Transient timeouts are handled by the worker poller's task-level retry
  (3 attempts, 60s spacing) which is better than rapid internal retries
  that amplify I/O pressure during contention storms

Set HINDSIGHT_API_RETAIN_MAX_CONCURRENT default from 32 to 4:
- The semaphore gates Phase 1 (ANN + entity resolution) + Phase 2 (writes)
- At 4 concurrent, HNSW index I/O is manageable; at 10+ concurrent the
  probes saturate disk and cause cascading timeouts
- LLM extraction still runs at full parallelism (semaphore acquired after)

* fix: add fact_type filter to Phase 1 ANN query to use per-bank HNSW indexes

The LATERAL ANN query was falling back to sequential scan + sort (90ms/probe)
because the per-bank HNSW indexes are partial indexes filtered on fact_type.
Without fact_type in the WHERE clause, PostgreSQL couldn't use them.

Fix: iterate over ('world', 'experience') and run one HNSW-indexed ANN per
type. EXPLAIN shows 8ms/probe (was 90ms) — 11x faster.

700 probes × 8ms × 2 types = ~11s total (was ~63s via seq scan).

* fix: scope temporal links by fact_type + add integration tests

Temporal links now filter by fact_type in the LATERAL query — world facts
only link to world facts, experience to experience. This matches how
retrieval filters results and avoids wasted cross-type link rows.

New integration tests:
- test_semantic_ann_uses_hnsw_index: verifies Phase 1 ANN creates
  cross-batch semantic links (tests fact_type filter + placeholder remap)
- test_temporal_links_scoped_by_fact_type: verifies world facts get
  temporal links to other world facts but NOT to experience facts

* fix: tolerate individual chunk LLM failures instead of failing entire batch

Changed asyncio.gather(*tasks) to asyncio.gather(*tasks, return_exceptions=True)
in both chunk-level and content-level fact extraction. A single chunk timeout
(e.g., Gemini >90s) no longer discards all other successfully extracted facts.

For a 50MB document with 17k chunks, even a 2% chunk failure rate previously
caused 0 completions (entire batch discarded). Now 16,700 facts are extracted
and only the 300 failed chunks are skipped with a warning log.

* fix: batch temporal LATERAL query for large documents (16k+ chunks)

The LATERAL query for temporal links passed all unit_ids at once into
unnest(), causing PostgreSQL timeouts on documents with 16k+ chunks.
Split into batches of 500 units per query to keep each under the
command_timeout.

Also identified: HNSW index creation on shared pg0 instances with
50k+ existing units exceeds the 60s command_timeout. This is a
test infrastructure issue (shared pg0 accumulates data) but also
affects production when creating new banks on large instances.

* feat: streaming chunk batching for large documents (RETAIN_CHUNK_BATCH_SIZE)

Process chunks in mini-batches of N (default 500), committing each batch
to the DB before starting the next. This prevents OOM kills on large
documents (50MB / 17k+ chunks) by keeping only ~500 facts + embeddings
in memory at a time instead of 50k+.

Each mini-batch goes through the full Phase 1 → 2 → 3 pipeline
independently, sharing the same document_id. On recovery (process dies
mid-way), delta retain detects already-committed chunks via content_hash
and skips them — only remaining chunks get re-extracted.

Config: HINDSIGHT_API_RETAIN_CHUNK_BATCH_SIZE (default: 500, 0 to disable)
Per-bank configurable via the hierarchical config system.

Tests:
- test_streaming_chunk_batching_produces_same_facts
- test_streaming_chunk_batching_recovery (delta retain skips committed chunks)
- test_streaming_disabled_for_small_docs

* perf(retain): producer-consumer pipeline + deferred semantic ANN

Replace the sequential streaming loop with a producer-consumer pipeline:
- LLM producer fires concurrent chunk extractions (semaphore-bounded)
- DB consumer drains queue in batches, runs Phase 1+2+3 per batch
- LLM and DB work overlap instead of running sequentially

Defer semantic links to a single final ANN pass after all batches commit:
- Remove within-batch semantic links from Phase 2 (was 2.6s/batch)
- Run parallel ANN (4 connections) after all facts committed
- top_k reduced from 50 to 20 (recall uses at most 20 neighbors)
- Recovery via operation result_metadata checkpoint

Additional optimizations:
- skip_exists_check on temporal/causal link INSERT (saves ~0.5s/batch)
- WHERE EXISTS guard on semantic link INSERT (handles document upsert)
- timeout=300s on ANN queries and bulk INSERT for large banks
- Demote [ANN] debug logs to logger.debug()
- Fix docstring typos (agent_id → bank_id)
- Fix content_index remapping in producer-consumer batches
- Fix delta retain passing contents vs delta_contents

50MB benchmark (mock LLM): 9.2 min (was 23 min) — 2.5x faster.
BEAM 10m benchmark: zero deadlocks, zero DB errors.

* refactor(retain): remove legacy fallback code paths

- Remove process_entities_batch (legacy single-connection entity processing)
- Remove extract_entities_batch_optimized (only caller was the above)
- Remove fallback entity processing inside Phase 2 transaction
- Remove legacy ANN inline fallback in create_semantic_links_batch
- Remove fallback entity_links direct-insert path in Phase 3
- Make resolved_entity_ids/entity_to_unit/unit_to_entity_ids required params

* refactor(retain): replace tuple returns with dataclasses, remove dead code

- Add EntityResolutionResult and Phase1Result dataclasses in types.py
- Replace 4-tuple return from _pre_resolve_phase1 with Phase1Result
- Remove dead `entity_links = []` variables in retain_batch and _try_delta_retain
- Remove unused `confidence_score` parameter from orchestrator.retain_batch
  and _retain_batch_async_internal (was accepted but never used)

* fix(entity-resolver): remove LIKE full-scan fallbacks, use index-only trigram matching

The entity resolution query had LIKE '%...' substring conditions that bypassed
the GIN trigram index, causing full sequential scans of the entities table.
On banks with 10k+ entities, this caused TimeoutErrors (observed in BEAM 10m).

Changes:
- Remove LIKE fallbacks, use trigram % operator only (GIN index-based)
- Lower similarity threshold from 0.3 to 0.15 to catch substring relationships
- Use LOWER() on both sides for case-insensitive matching
- Migration: recreate GIN trigram index on LOWER(canonical_name)

* fix: remove schema prefix from index names in trigram migration

* fix(delta-retain): use same chunk_size as streaming path (3000 vs 120000)

_chunk_contents_for_delta defaulted to chunk_size=120000 while the streaming
path used 3000. On retry, delta re-chunked the document with different
boundaries, found 0 matching chunks, and fell through to full re-extraction.
This wasted all LLM calls on already-committed chunks.

Fix: use the same default (3000) so chunk hashes match on recovery.

* fix(retain): persist generated document_id in operation metadata for retry recovery

When no document_id is provided, retain generates a UUID. On retry, a new UUID
was generated, making delta retain and streaming chunk-hash recovery unable to
find previously committed chunks. All LLM extraction was wasted on retry.

Fix: resolve document_id early in retain_batch (before delta), persist it to
operation result_metadata, and recover it on retry. Both delta and streaming
paths now see the same document_id across attempts.

* refactor(retain): unify into single streaming pipeline, remove non-streaming path

All retains now go through the producer-consumer streaming pipeline,
regardless of document size. Small documents are processed as a single batch.
This eliminates the maintenance burden of two separate code paths.

Also fix document upsert: compare content hash to distinguish recovery
(same content, partially committed) from update (different content, needs
cascade-delete). Previously, existing chunks always triggered recovery mode.

* refactor(retain): remove dead code, replace raw dicts with Phase3Context dataclass

- Remove dead _handle_zero_facts_documents (no callers after path unification)
- Remove unused imports: defaultdict, EntityLink
- Replace raw dict phase3_context with typed Phase3Context dataclass
- Update _build_and_insert_entity_links_phase3 to use typed parameter
2026-04-01 12:52:49 +02:00

267 lines
9.4 KiB
Python

"""
Pytest configuration and shared fixtures.
"""
import pytest
import pytest_asyncio
import asyncio
import os
import filelock
from pathlib import Path
from dotenv import load_dotenv
from hindsight_api import MemoryEngine, LLMConfig, LocalSTEmbeddings, RequestContext
from hindsight_api.engine.cross_encoder import LocalSTCrossEncoder
from hindsight_api.engine.query_analyzer import DateparserQueryAnalyzer
from hindsight_api.engine.task_backend import SyncTaskBackend
from hindsight_api.pg0 import EmbeddedPostgres
# Default pg0 instance configuration for tests
DEFAULT_PG0_INSTANCE_NAME = "hindsight-test"
DEFAULT_PG0_PORT = int(os.environ.get("HINDSIGHT_TEST_PG_PORT", "5556"))
# Load environment variables from .env at the start of test session
def pytest_configure(config):
"""Load environment variables before running tests."""
# Look for .env in the workspace root (two levels up from tests dir)
env_file = Path(__file__).parent.parent.parent / ".env"
if env_file.exists():
load_dotenv(env_file)
else:
print(f"Warning: {env_file} not found, tests may fail without proper configuration")
@pytest.fixture(scope="session")
def db_url():
"""
Provide a PostgreSQL connection URL for tests.
If HINDSIGHT_API_DATABASE_URL is set, use it directly.
Otherwise, return None to indicate pg0 should be used (managed by pg0_instance fixture).
"""
return os.getenv("HINDSIGHT_API_DATABASE_URL")
@pytest.fixture(scope="session")
def pg0_db_url(db_url, tmp_path_factory, worker_id):
"""
Session-scoped fixture that ensures pg0 is running, migrations are applied,
and returns the database URL.
If HINDSIGHT_API_DATABASE_URL is a plain postgresql:// URL, uses it directly.
If HINDSIGHT_API_DATABASE_URL is a pg0:// URL, resolves it to a real URL first.
Otherwise, starts pg0 once for the entire test session.
Uses filelock to ensure only one pytest-xdist worker starts pg0.
Migrations use PostgreSQL advisory locks internally, so they're safe to call
from multiple workers - only one will actually run migrations.
Note: We don't stop pg0 at the end because pytest-xdist runs workers in separate
processes that share the same pg0 instance. pg0 will persist for the next test run.
"""
from hindsight_api.pg0 import parse_pg0_url as _parse_pg0_url
# Determine pg0 instance name/port from db_url (if it's a pg0:// URL) or use defaults
if db_url and not _parse_pg0_url(db_url)[0]:
# Plain postgresql:// URL - use it directly but still run migrations
from hindsight_api.migrations import run_migrations
run_migrations(db_url)
return db_url
if db_url:
_, pg0_name, pg0_port = _parse_pg0_url(db_url)
pg0_instance_name = pg0_name or DEFAULT_PG0_INSTANCE_NAME
pg0_instance_port = pg0_port or DEFAULT_PG0_PORT
else:
pg0_instance_name = DEFAULT_PG0_INSTANCE_NAME
pg0_instance_port = DEFAULT_PG0_PORT
# Get shared temp dir for coordination between xdist workers
if worker_id == "master":
# Running without xdist (-n 0 or no -n flag)
root_tmp_dir = tmp_path_factory.getbasetemp()
else:
# Running with xdist - use parent dir shared by all workers
root_tmp_dir = tmp_path_factory.getbasetemp().parent
# Use a lock file to ensure only one worker starts pg0
lock_file = root_tmp_dir / f"pg0_setup_{pg0_instance_name}.lock"
url_file = root_tmp_dir / f"pg0_url_{pg0_instance_name}.txt"
with filelock.FileLock(str(lock_file)):
if url_file.exists():
# Another worker already started pg0
url = url_file.read_text().strip()
else:
# First worker - start pg0
pg0 = EmbeddedPostgres(name=pg0_instance_name, port=pg0_instance_port)
# Run ensure_running in a new event loop
loop = asyncio.new_event_loop()
try:
url = loop.run_until_complete(pg0.ensure_running())
finally:
loop.close()
# Save URL for other workers
url_file.write_text(url)
# Run migrations - uses PostgreSQL advisory lock internally,
# so safe to call from multiple workers (only one will actually run migrations)
from hindsight_api.migrations import run_migrations
run_migrations(url)
return url
@pytest.fixture(scope="function")
def request_context():
"""Provide a default RequestContext for tests."""
return RequestContext()
@pytest.fixture(scope="session")
def llm_config():
"""
Provide LLM configuration for tests.
This can be used by tests that need to call LLM directly without memory system.
"""
return LLMConfig.from_env()
@pytest.fixture(scope="session")
def embeddings(tmp_path_factory, worker_id):
"""
Session-scoped embeddings fixture with filelock to prevent race conditions.
When pytest-xdist runs multiple workers in parallel, they all try to load
models from the HuggingFace cache simultaneously, which can cause race
conditions and meta tensor errors. We use a filelock to serialize model
initialization across workers.
"""
# Get shared temp dir for coordination between xdist workers
if worker_id == "master":
root_tmp_dir = tmp_path_factory.getbasetemp()
else:
root_tmp_dir = tmp_path_factory.getbasetemp().parent
lock_file = root_tmp_dir / "embeddings_init.lock"
emb = LocalSTEmbeddings()
# Serialize model initialization across workers
with filelock.FileLock(str(lock_file)):
loop = asyncio.new_event_loop()
try:
loop.run_until_complete(emb.initialize())
finally:
loop.close()
return emb
@pytest.fixture(scope="session")
def cross_encoder(tmp_path_factory, worker_id):
"""
Session-scoped cross-encoder fixture with filelock to prevent race conditions.
When pytest-xdist runs multiple workers in parallel, they all try to load
models from the HuggingFace cache simultaneously, which can cause race
conditions and meta tensor errors. We use a filelock to serialize model
initialization across workers.
"""
# Get shared temp dir for coordination between xdist workers
if worker_id == "master":
root_tmp_dir = tmp_path_factory.getbasetemp()
else:
root_tmp_dir = tmp_path_factory.getbasetemp().parent
lock_file = root_tmp_dir / "cross_encoder_init.lock"
ce = LocalSTCrossEncoder()
# Serialize model initialization across workers
with filelock.FileLock(str(lock_file)):
loop = asyncio.new_event_loop()
try:
loop.run_until_complete(ce.initialize())
finally:
loop.close()
return ce
@pytest.fixture(scope="session")
def query_analyzer():
return DateparserQueryAnalyzer()
@pytest_asyncio.fixture(scope="function")
async def memory(pg0_db_url, embeddings, cross_encoder, query_analyzer):
"""
Provide a MemoryEngine instance for each test.
Must be function-scoped because:
1. pytest-xdist runs tests in separate processes with different event loops
2. asyncpg pools are bound to the event loop that created them
3. Each test needs its own pool in its own event loop
Uses small pool sizes since tests run in parallel.
Uses pg0_db_url (a postgresql:// URL) directly, so MemoryEngine won't try to
manage pg0 lifecycle - that's handled by the session-scoped pg0_db_url fixture.
Migrations are disabled here since they're run once at session scope in pg0_db_url.
Uses SyncTaskBackend so async tasks execute immediately (no worker needed).
"""
mem = MemoryEngine(
db_url=pg0_db_url, # Direct postgresql:// URL, not pg0://
memory_llm_provider=os.getenv("HINDSIGHT_API_LLM_PROVIDER", "groq"),
memory_llm_api_key=os.getenv("HINDSIGHT_API_LLM_API_KEY"),
memory_llm_model=os.getenv("HINDSIGHT_API_LLM_MODEL", "openai/gpt-oss-120b"),
memory_llm_base_url=os.getenv("HINDSIGHT_API_LLM_BASE_URL") or None,
embeddings=embeddings,
cross_encoder=cross_encoder,
query_analyzer=query_analyzer,
pool_min_size=1,
pool_max_size=5,
run_migrations=False, # Migrations already run at session scope
task_backend=SyncTaskBackend(), # Execute tasks immediately in tests
)
await mem.initialize()
yield mem
try:
if mem._pool and not mem._pool._closing:
await mem.close()
except Exception:
pass
@pytest_asyncio.fixture(scope="function")
async def memory_no_llm_verify(pg0_db_url, embeddings, cross_encoder, query_analyzer):
"""
Provide a MemoryEngine instance that skips LLM connection verification.
This fixture is useful for tests that override the LLM configuration
after initialization (e.g., to test specific providers).
"""
mem = MemoryEngine(
db_url=pg0_db_url,
memory_llm_provider="mock", # Use mock provider as placeholder
memory_llm_api_key="",
memory_llm_model="mock",
embeddings=embeddings,
cross_encoder=cross_encoder,
query_analyzer=query_analyzer,
pool_min_size=1,
pool_max_size=5,
run_migrations=False,
task_backend=SyncTaskBackend(),
skip_llm_verification=True, # Skip verification - will be overridden by test
)
await mem.initialize()
yield mem
try:
if mem._pool and not mem._pool._closing:
await mem.close()
except Exception:
pass