perf(hot-path): persistent BM25F index, flat bulk ingest, batched write groups (#1135) - #1140
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @robotrocketscience, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
|
Warning Review limit reached
Next review available in: 43 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (17)
📝 WalkthroughWalkthroughThis change adds persistent BM25F sidecars, durable store-generation invalidation, transactional write batching, outcome-based ingestion, shared prompt store handles, targeted indexes, TOML memoization, and marker-gated origin migration behavior. ChangesRetrieval and ingest hot-path overhaul
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant UserPromptSubmit
participant MemoryStore
participant Retrieval
participant BM25IndexCache
participant FeedbackWriters
UserPromptSubmit->>MemoryStore: open shared store
UserPromptSubmit->>Retrieval: retrieve using shared store
Retrieval->>BM25IndexCache: request BM25F index
BM25IndexCache->>MemoryStore: validate store generation
BM25IndexCache-->>Retrieval: return cached or rebuilt index
Retrieval-->>UserPromptSubmit: return beliefs
UserPromptSubmit->>FeedbackWriters: record prompt side effects
FeedbackWriters->>MemoryStore: commit batched transaction
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
robotrocketscience
added
the
attn:review
PR-size soft capThis PR is over the advisory size threshold:
Bigger PRs collide with more open work, which under the parallel-session workflow tends to produce repeated This is advisory only — nothing is blocked. If the size is intentional (large refactor, module removal, generated code), apply the |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
[claim:review:Gylf:2026-07-21T18:54:19Z] |
|
[claim:review:Kulili:2026-07-21T18:57:52Z] |
|
[release:review:Kulili:2026-07-21T18:57:58Z] |
|
Reviewed the full diff (all 12 commits, ~1.6k lines) against main. Verdict: approve, with one fix pushed to the branch (same-account, so verdict-by-comment; will queue What I verified:
The fix I pushed (febd7b2): the store-scoped cache + sidecar made the index survive across queries, but Two nits, no action needed:
Resolved the five CodeQL threads: the two "unreachable" statements are the rollback tests' post-raise guards (deliberate), the |
robotrocketscience
added
ready-to-merge
|
[release:review:Gylf:2026-07-21T19:07:15Z] |
|
merge-train: blocked branch is not fast-forward on The |
github-actions
Bot
removed
the
ready-to-merge
WAL was set at open but synchronous stayed at the FULL default, paying a full fsync per commit. NORMAL is the documented WAL pairing: fsync at checkpoint, not per commit; app crashes lose nothing. Measured ~2x cheaper per-commit on the ingest write path.
#1135) _BACKFILL_STATEMENTS (two full-table UPDATEs) ran unguarded on every store open, unlike every other one-shot pass. Contemporary writers set origin explicitly, so the flip only matters once per legacy DB. Marker follows the entity-backfill convention; the pass rides the existing open commit.
…an (#1135) Three query shapes ran unindexed on every hot-path invocation: - derivation worker's unstamped ingest_log scan (O(total log) per turn on a monotonically growing table) -> partial index on the NULL stamp - list_locked_beliefs L0 tier (up to 3x per retrieve, full-table scan) -> partial index matching the ORDER BY - has_edge_type() probe (per non-empty query since the #1064 lane flip; miss case scanned all edges) -> index on edges(type) The unstamped scan also names its columns instead of SELECT *.
…et snapshot (#1135) _ingest_turn_ids snapshotted set(store.list_belief_ids()) every turn (full-table scan, linear in corpus size) solely to distinguish new inserts from corroborations — a fact insert_or_corroborate already returns. WorkerResult now carries outcomes: {log_id: (belief_id, was_inserted)}; ingest reads its rows' fate from there, which also removes the per-sentence get_ingest_log_entry re-read. Fallback to the log row covers the sibling-process race (a row stamped elsewhere is not a brand-new insert of ours). Bulk-ingest per-turn cost is now flat w.r.t. corpus size.
…1135) Every mutating method committed per call (~8 commits per ingested turn, ~45-60 per hook prompt with hits). transaction() suppresses the per-call commits via a depth counter and issues one commit at outermost exit; measured 33x cheaper than commit-per-row for a 200-insert group. Invalidation callbacks defer to one post-commit fire so derived caches never read uncommitted state; exceptions roll the whole group back. No call sites wired yet - behavior outside transaction() is byte-identical.
…ns (#1135) Wire store.transaction() into the four commit-per-row hot spots: - ingest turn (record_ingest per sentence + worker insert/stamp per row + edge inserts): one write group per turn, which also makes the turn atomic — a crash mid-turn leaves no unstamped orphans - hook record_retrieval (apply_feedback per hit + stamp_retrieved): one group per retrieval; an un-committable group (closed handle) reports 0 rows written, preserving the non-blocking contract - hook injection_events + belief_touches loops: one commit per batch - deferred-feedback exposure enqueue: one commit per batch transaction()'s rollback path no longer masks the original exception when the rollback itself fails on a dead handle.
…1135) BM25F is default-ON but no caller passed a bm25f_cache, so every retrieve() re-tokenized + Porter-stemmed the whole corpus and rebuilt the CSR matrix (584 ms at 5k beliefs; the UPS hook is a fresh process per prompt, and even the MCP server rebuilt per query while leaking one invalidation callback per call). - MemoryStore gains a durable store_generation counter, bumped inside the same transaction as every belief/edge content mutation (the 11 _fire_invalidation sites, now _commit_mutation). Feedback/touch writes don't bump it - they change ranking inputs, not index content. Seed is read-first so opens stay lock-free. - BM25IndexCache.get() loads <db-path>.bm25f when its generation + scope-id stamp and k1/b/anchor_weight match, else builds and atomically rewrites it (os.replace; fail-soft both ways; :memory: skips persistence). - serialize() v2 widens k1/b/avgdl to float64: a loaded index now scores byte-identically to a fresh build (float32 round-trip perturbed low-order score bits). No v1 blobs exist - nothing called serialize() before this. - retrieve() reuses one store-scoped cache instead of constructing a fresh one per call.
~24 resolver call sites fall through to the TOML rung per retrieve(), each re-reading and re-parsing the file. The directory walk (stat calls) still runs per call so created/deleted config files are honoured, but the read + tomllib parse is cached per file until its mtime_ns/size changes. Malformed-file stderr traces now print once per file version instead of once per resolver call.
The UPS process opened MemoryStore 4-6 times per prompt (relevance sweep, retrieval, injection events, touches, plus session-start on first prompt), each replaying the schema battery (~25-40 ms/prompt of pure re-open). user_prompt_submit now opens one handle after payload parse and threads it through those helpers; each keeps its legacy self-open fallback for direct callers and tests, and the default-off lanes (sentiment, category boost, phantom blocks) are unchanged. A failed shared open degrades to the fallback path, preserving per-helper fail-softness.
…sion (#1135) The catch-up used to be re-applied by the target store on every open; now that it is a marker-gated one-shot, the target's marker is already stamped (on an empty store) before legacy rows land, so migrated locked/correction rows stayed at origin=unknown. Apply the same mapping in _read_legacy_beliefs — migrate output is unchanged from pre-#1135 (regression: test_apply_preserves_origin_from_legacy_row, #224 contract).
…get() (#1135) The in-process invalidation callback only covers own-process mutations, so a resident process (MCP server) holding the store-scoped cache would never observe sibling-process writes (default-on ingest hooks) — a freshness regression vs the pre-#1135 rebuild-per-query behavior. One indexed schema_meta point-read per get(); on mismatch the cache drops the index and reloads the (possibly sibling-refreshed) sidecar before rebuilding.
febd7b2 to
2592c2f
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/aelfrice/ingest.py (1)
318-349: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConfirm the intended failure semantics of moving the optional substrate writers inside the per-turn transaction.
write_semantic_edgesandwrite_temporal_spine(the latter default-ON peris_temporal_spine_write_enabled) now execute insidestore.transaction(). Because the outer block rolls back the whole group on any exception, a failure in either optional writer will now discard this turn's primary belief/log rows as well and propagate to theingest_turncaller. Pre-batching, those primary rows committed per-row before the substrate writers ran, so a substrate bug degraded only the optional edges — not core ingestion.If the intent is that a secondary-substrate failure should not block primary ingest, consider isolating these calls so their failure can't abort the turn:
♻️ Optional: shield primary ingest from optional-writer failures
if inserted: from aelfrice.relationship_detector import ( is_auto_relationship_detection_enabled, write_semantic_edges, ) - - if is_auto_relationship_detection_enabled(): - write_semantic_edges(store, new_belief_ids=inserted) + try: + if is_auto_relationship_detection_enabled(): + write_semantic_edges(store, new_belief_ids=inserted) + except Exception: + traceback.print_exc()If the atomic all-or-nothing turn (relying on idempotent re-ingest to recover) is deliberate, this is fine as-is — please confirm.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/aelfrice/ingest.py` around lines 318 - 349, Decide and document the intended failure semantics for the optional writers in the ingest transaction. If primary ingestion must remain successful when write_semantic_edges or write_temporal_spine fails, isolate each call from the outer store.transaction rollback and preserve the exception handling policy; otherwise, explicitly confirm that ingest_turn intentionally propagates writer failures and rolls back the primary belief and log rows atomically.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/test_bm25_sidecar.py`:
- Around line 123-130: Update the persisted-parameter validation in
BM25IndexCache._load_sidecar to compare k1 and b at full float64 precision
before accepting a sidecar, without narrowing to np.float32. Extend
test_anchor_weight_mismatch_rejects_sidecar coverage to vary each parameter
independently using math.nextafter and assert the sidecar is rejected.
---
Nitpick comments:
In `@src/aelfrice/ingest.py`:
- Around line 318-349: Decide and document the intended failure semantics for
the optional writers in the ingest transaction. If primary ingestion must remain
successful when write_semantic_edges or write_temporal_spine fails, isolate each
call from the outer store.transaction rollback and preserve the exception
handling policy; otherwise, explicitly confirm that ingest_turn intentionally
propagates writer failures and rolls back the primary belief and log rows
atomically.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 659dbfd7-16a5-4a31-91ab-322356ca2088
📒 Files selected for processing (17)
CHANGELOG/v4.mdsrc/aelfrice/bm25.pysrc/aelfrice/deferred_feedback.pysrc/aelfrice/derivation_worker.pysrc/aelfrice/hook.pysrc/aelfrice/hook_search.pysrc/aelfrice/ingest.pysrc/aelfrice/migrate.pysrc/aelfrice/retrieval.pysrc/aelfrice/store.pytests/test_bm25_sidecar.pytests/test_derivation_worker.pytests/test_retrieval_toml_memo.pytests/test_store_hot_path_indexes.pytests/test_store_transaction.pytests/test_v1_to_v1x_migration.pytests/test_worktree_concurrency.py
robotrocketscience
added
the
ready-to-merge
|
merge-train: blocked 1 review thread(s) are unresolved on these files: tests/test_bm25_sidecar.py. Resolve them on the PR (click 'Resolve conversation' on each) and re-add the label. The |
github-actions
Bot
removed
the
ready-to-merge
The v2 sidecar serialises k1/b as float64, so the round-trip is exact and the float32-narrowed comparison could let two configs that differ only below float32 precision share a sidecar. Compare exactly and pin with a nextafter mismatch test for each parameter.
|
Addressed the k1/b precision thread in d603cd2: the v2 sidecar stores k1/b as float64 so the round-trip is exact — the float32-narrowed comparison was legacy from v1 and is now a full-precision compare, pinned by nextafter mismatch tests for both parameters. Will re-add ready-to-merge when CI is green on the new head. |
robotrocketscience
added
the
ready-to-merge
|
merge-train: merged d603cd2 → |
github-actions
Bot
removed
the
ready-to-merge
2 participants
Closes #1135.
Implements the measured perf audit structurally — pure performance, no ranking change; retrieval outputs are byte-identical on a fixed store.
P1
1. BM25F index persisted across processes (was: rebuilt per
retrieve()).store_generationcounter inschema_meta, bumped inside the same transaction as every belief/edge content mutation (exactly the 11_fire_invalidationsites, now_commit_mutation()). Feedback/touch/corroboration writes do not bump it — they change ranking inputs, not index content. Seed is read-first so store opens stay lock-free.BM25IndexCache.get()loads a<db-path>.bm25fsidecar when its generation + scope-id stamp and k1/b/anchor_weight all match; otherwise builds and atomically rewrites it (os.replace, fail-soft both directions,:memory:skips persistence). The scope-id in the stamp catches a different DB swapped in at the same path.serialize()format bumped to v2: k1/b/avgdl widened float32 → float64 so a loaded index scores byte-identically to a built one (the float32 round-trip perturbed low-order score bits, which could flip near-ties). No v1 blobs exist anywhere — nothing calledserialize()before this PR.retrieve()'s fallback cache is now store-scoped and reused: fixes the pre-existing leak of one invalidation-callback subscription per query on long-running processes (MCP server), which also rebuilt per query.<db-path>.bm25fover the issue's<db-dir>/bm25f.idxso two DB files in one directory can't share a sidecar.2. Bulk ingest de-quadratified.
WorkerResultgainsoutcomes: {log_id: (belief_id, was_inserted)};_ingest_turn_idsreads its rows' fate from there instead of snapshottingset(list_belief_ids())per turn and re-reading each log row. Fallback to the log row covers the sibling-process race (a row stamped elsewhere is by definition not a brand-new insert of ours).idx_ingest_log_unstamped ON ingest_log(id) WHERE derived_belief_ids IS NULL— the log grows monotonically while the unstamped set stays tiny.list_unstamped_ingest_logalso names its columns instead ofSELECT *.3. Commit-per-row churn.
MemoryStore.transaction()context manager: per-call commits suppressed via a depth counter, one commit at outermost exit, invalidation callbacks deferred to a single post-commit fire (so derived caches never read uncommitted state), full-group rollback on exception. Reentrant.record_retrieval(apply_feedback per hit + stamp), the injection-events and touches loops, and the deferred-feedback enqueue (~45–60 commits per hook prompt with hits → a handful).PRAGMA synchronous=NORMALset alongside WAL (the documented pairing).P2
list_locked_beliefs(ran up to 3× per retrieve as a full-table scan), matching its ORDER BY; index onedges(type)for thehas_edge_typeprobe that fires per non-empty query since the Temporal spine: ingest-time chronological edges + dedicated retrieval lane (confirmed +14.6pp coverage on LoCoMo) #1064 lane flip. Tests assert the planner actually uses each index (EXPLAIN QUERY PLAN), not just that they exist..aelfrice.tomlparse memoized per (path, mtime_ns, size). The directory walk still runs per call, so config creation/deletion/move is honoured; only the read + tomllib parse is cached. Malformed-file stderr traces now print once per file version instead of ~24× per retrieve._BACKFILL_STATEMENTS, two full-table UPDATEs) is now a schema_meta-gated one-shot. Contemporary writers set origin explicitly (derive() routes, cli lock upgrade), so the flip only ever mattered once per legacy DB.Acceptance
synchronous=NORMALset alongside WALtest_loaded_index_scores_identical_to_built) and the serialize v2 float64 widening exists precisely to keep it exactOut of scope (the issue's "correctness smells" section)
Left for operator disposition per the issue's own framing ("may deserve their own issues") — not touched here:
retrieval.pyint(time.time())insideretrieve_with_tiers(clock seam, same class as bug: test_retrieve_v2_temporal_sort_explicit_half_life_kwarg is a wall-clock time-bomb (main red ~2026-06-22, blocks all PR gates) #982)RetrievalCache.retrieveskipping exposure/feedback side effects on cache hitsNotes for review
test_search_tool_hook_bash5s-timeout pair (BM25 build under load — the exact cost item 1 addresses) and the doctor ambient-store flake. CI is authority.Summary by CodeRabbit
Performance
Reliability
Data Migration