Skip to content

feat(maintenance): self-heal per-bank vector indexes at startup + on schedule (#2645) - #2803

Closed
ijevin wants to merge 4 commits into
vectorize-io:fix/backfill-vector-indexes-2645from
ijevin:fix/vector-index-self-heal-2645-stacked
Closed

feat(maintenance): self-heal per-bank vector indexes at startup + on schedule (#2645)#2803
ijevin wants to merge 4 commits into
vectorize-io:fix/backfill-vector-indexes-2645from
ijevin:fix/vector-index-self-heal-2645-stacked

Conversation

@ijevin

@ijevin ijevin commented Jul 18, 2026

Copy link
Copy Markdown
Contributor

Summary

Stacked follow-up to #2712: turn the one-shot backfill-vector-indexes
CLI into a real self-heal. After this PR, the API:

  • Runs the same reconciliation at startup and on a configurable schedule
    (HINDSIGHT_API_VECTOR_INDEX_RECONCILE_INTERVAL_SECONDS, default 3600s,
    0 disables).
  • Reuses the per-bank partial-index catalog check and reconcile_vector_indexes()
    helper across CLI / boot / periodic paths, so a name-only false positive
    never silently satisfies reconciliation.
  • Defers repair to the background loop from the hot retain path using a TTL
    cache (default 60s) and only signals request_vector_index_reconcile()
    no DDL on the request connection.
  • Holds a session-level pg_try_advisory_lock so two API replicas cannot
    rebuild the same index in parallel.
  • Detects Postgres transaction-pool URLs (PgBouncer port 6543 /
    pool_mode=transaction) and refuses to reconcile unless the operator
    sets HINDSIGHT_API_MIGRATION_DATABASE_URL to a direct libpq URL —
    session-level advisory locks and CREATE/DROP INDEX CONCURRENTLY
    require a single backend session.
  • Uses server-side quote_literal() for the bank_id predicate literal
    so escaping does not depend on standard_conforming_strings.
  • Wraps each tenant schema in a try/except so a single bad schema cannot
    abort the rest of the sweep.
  • Surfaces advisory-lock contention to the CLI as exit code 75
    (EX_TEMPFAIL) with a retry message rather than false-positive success.

The stacked commits, in order, are:

  1. c0def121feat(maintenance): self-heal per-bank vector indexes (#2645)
  2. f279ac4bfix(reconcile): harden vector index self-heal per review (#2645)
  3. 1d49aa73fix(reconcile): match pg_get_indexdef predicate shape and default new field (#2645)
  4. a71c27e7fix(config): make new reconcile interval field keyword-only

Base

This PR is stacked on top of #2712 (8d45a64b,
fix/backfill-vector-indexes-2645) and is not intended to merge
until #2712 lands. Reviewing the four commits in order diff-by-diff against
8d45a64b will surface only the part-2 change.

Test plan

  • uv run pytest tests/test_vector_index_reconcile.py tests/test_vector_index_retain_safety.py tests/test_maintenance_loop.py -k 'not (reconcile_submits_eligible or purge_expired)'
  • uv run pytest tests/test_config_validation.py tests/test_main_module.py
  • uv run ruff check ... and uv run ruff format --check ...
  • uv run ty check hindsight_api/admin/cli.py hindsight_api/config.py hindsight_api/engine/maintenance.py hindsight_api/engine/memory_engine.py hindsight_api/engine/vector_index_reconcile.py
  • Local root user cannot run pg0 initdb; the backfill integration tests
    (tests/test_backfill_vector_indexes.py) must be verified on CI or under
    a non-root user.

Notes for reviewers

  • The retain-side _maybe_request_vector_index_reconcile() TTLs catalog
    checks per (schema, bank_id) using vector.monotonic() and never
    executes DDL on the request connection.
  • bank_vector_indexes_healthy() and the full reconcile query reject
    indexes that are merely name-matching the expected relname; the check
    also requires valid/ready, memory_units as the indexed table, a
    recognised access method, and the canonical
    pg_get_indexdef partial-predicate shape
    (' WHERE ((fact_type = ').
  • The kw_only field added in a71c27e7
    (vector_index_reconcile_interval_seconds) preserves all positional
    callers; the existing from_env() defaults (300 / 60 /
    ['consolidation.completed'] / 30) are unchanged.

ijevin added 4 commits July 18, 2026 15:01
…o#2645)

Address the issues the independent review flagged on the self-heal follow-up:

- Use Postgres-side quote_literal() for the bank_id predicate literal so
  escaping does not depend on standard_conforming_strings and bank_id
  values cannot smuggle DDL.
- Health checks now require the index to be valid, ready, defined over
  the expected memory_units table, use a supported access method, and
  carry the expected partial predicate. A name match alone no longer
  marks a bank healthy, so backend switches and stale same-name indexes
  correctly trigger repair.
- Wrap each per-schema reconcile in a try/except so a failure in one
  tenant schema no longer aborts the global sweep.
- Refuse to reconcile when DATABASE_URL points at a transaction pooler
  (PgBouncer) unless MIGRATION_DATABASE_URL is explicitly set to a
  direct libpq URL. Session-level advisory locks and CONCURRENTLY DDL
  require a single PostgreSQL session.
- Default newly added dataclass fields so direct HindsightConfig(...)
  callers keep working.
- Surface advisory-lock contention to the CLI as exit code 75 with a
  retry message instead of claiming success.
… field (vectorize-io#2645)

Address the residual review findings on the hardened stacked commits:

- Replace the single-paren predicate substring with the canonical Postgres
  rendering '`WHERE ((fact_type = `'. pg_get_indexdef emits a parenthesized
  comparison operand and an explicit ::text cast, so the previous pattern
  matched no healthy per-bank indexdef and would have driven an
  unconditional rebuild loop every sweep.

- Give vector_index_reconcile_interval_seconds the documented default
  (0 = disabled). Without the default the dataclass breaks any external
  caller that constructs HindsightConfig() positionally/keyword-style, the
  claim in the previous commit message was unsupported.
The previous commit added a default to vector_index_reconcile_interval_seconds
inline, which silently shifted existing positional callers onto the wrong
field and rewrote the established defaults for consolidation_reconcile_interval_seconds,
mental_model_refresh_tick_seconds, webhook_event_types, and
webhook_delivery_poll_interval_seconds.

Make only the new field kw_only with its own default, and restore the
original fields to required fields so they keep loading through
HindsightConfig.from_env() with the documented defaults (300, 60,
['consolidation.completed'], 30).
@nicoloboschi

Copy link
Copy Markdown
Collaborator

Closing in favor of #2872 (merged as bd853be).

Thank you @ijevin — the health-check logic in #2872 is directly the correct detection approach from this PR (valid/ready + memory_units + supported access method + pg_get_indexdef partial-predicate shape), rather than the name-only check it replaced. That part was the load-bearing insight and it carried over.

#2872 is an intentionally smaller scope than this PR: it keeps the re-runnable command + the import-bank leak fix, but drops the boot/periodic background reconcile and the retain-path self-heal hook. Two reasons:

  1. Project rule: no advisory locks. The repo forbids pg_advisory_lock/pg_try_advisory_lock (unreliable behind connection poolers — see the Database Locking standard). The session-level advisory lock this PR uses to serialize the reconcile pass isn't allowed, and the background reconcile leans on it. fix(vector-index): repair per-bank index coverage after restore/upgrade (#2645) #2872 handles concurrency by idempotency instead (CREATE INDEX CONCURRENTLY IF NOT EXISTS gated by the health check).
  2. Scope: we opted to ship the leak fix + operator command first and treat always-on convergence as a separate decision.

If we later want zero-touch convergence for restored-then-read-only banks, the boot-reconcile can come back as a follow-up — but it'll need a lock-free coordination approach (per-process idempotency / row constraints) rather than the advisory lock. Really appreciate the thoroughness here; it shaped the merged result.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants