perf(memory): batch the prior-lifecycle read in ingest, dropping an N+1

[mysma-9403]

What

The memory ingest pipeline reads every staged chunk's prior lifecycle status before the upsert, to decide which chunks are genuinely new (and need an extract_chunk job) versus already past extraction. That read was an N+1: one SELECT … WHERE id = ? per staged chunk, in a loop, inside the write transaction.

src/openhuman/memory/ingest_pipeline.rs:

let mut prior: HashMap<String, Option<String>> = HashMap::new();
for s in &staged_for_store {
    let status = chunk_store::get_chunk_lifecycle_status_tx(&tx, &s.chunk.id)?;   // 1 round-trip / chunk
    prior.insert(s.chunk.id.clone(), status);
}

A single document fans out into many chunks, so a large note/email/Notion page issues dozens–hundreds of single-row queries — each a fresh prepare + B-tree lookup — before the batch even upserts.

Change

Add get_chunk_lifecycle_statuses_tx: one SELECT id, lifecycle_status … WHERE id IN (…) (windowed at MAX_FETCH_BATCH to stay under SQLite's bound-parameter cap), and call it once:

let staged_ids: Vec<String> = staged_for_store.iter().map(|s| s.chunk.id.clone()).collect();
let prior = chunk_store::get_chunk_lifecycle_statuses_tx(&tx, &staged_ids)?;

This is not a new pattern — it mirrors the two batched readers already in the same store (get_chunks_batch, get_chunk_embeddings_for_signature_batch), reusing their windowing/placeholder shape. The now-unused single-id get_chunk_lifecycle_status_tx is removed; the single-id get_chunk_lifecycle_status (used by read RPC) stays.

Is it worth it? (honest take)

SQLite is in-process, so each avoided round-trip is cheap in absolute terms — this is not a dramatic speedup, and I won't oversell it. Two things make it worthwhile anyway:

1. It shortens the write-transaction critical section. The loop runs inside an IMMEDIATE write transaction, and SQLite serialises writers — every extra statement holds the write lock a little longer, blocking other ingest workers. Collapsing N statements into one shortens that hold time, which is the kind of win that actually shows up under concurrent ingest.
2. It scales with document size (statement prepares + lookups go from O(chunks) to O(1) queries) and follows an existing in-repo precedent for exactly this refactor, so it's low-risk and consistent rather than a speculative micro-tweak.

Correctness

Behaviour-preserving: the map is keyed by chunk id, present rows map to their status, missing ids are absent — identical to the old .get(id).flatten() treating a miss as "no prior row" (genuinely new). Ordering is irrelevant (map lookup). The batch reads inside the same transaction, so it sees the same pre-upsert snapshot the loop did.

Tests

New unit tests in store_tests.rs:

• lifecycle_statuses_batch_matches_single_reads — batch result equals looping the single-id read for present ids (explicit admitted/sealed plus the insert default), and a missing id is absent.
• lifecycle_statuses_batch_empty_input_issues_no_query — empty input → empty map, no SQL.

cargo test --lib memory_store::chunks green.

Summary by CodeRabbit

• Bug Fixes

  • Improved chunk processing so lifecycle states are checked more reliably before scheduling extraction work.
  • Reduced missed lookups by using batched status reads, which also handles empty and missing ID cases more consistently.

• Tests

  • Added coverage for batched lifecycle status retrieval across multiple chunks.
  • Added a test confirming empty input returns no results and does not trigger unnecessary work.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: ff20f7e9-54ef-465d-920e-9cce18a13767

📥 Commits

Reviewing files that changed from the base of the PR and between 42ce5c0 and 5a84ae6.

📒 Files selected for processing (3)

• src/openhuman/memory/ingest_pipeline.rs
• src/openhuman/memory_store/chunks/store.rs
• src/openhuman/memory_store/chunks/store_tests.rs

---

📝 Walkthrough

Walkthrough

Replaced the per-chunk get_chunk_lifecycle_status_tx function with a batched get_chunk_lifecycle_statuses_tx/get_chunk_lifecycle_statuses_conn API in the chunk store, updated the ingest pipeline's persist transaction to fetch prior lifecycle statuses in a single batched call, and added corresponding unit tests.

Changes

Batched lifecycle status lookup

Layer / File(s)	Summary
Batched store API and tests src/openhuman/memory_store/chunks/store.rs, src/openhuman/memory_store/chunks/store_tests.rs	Removes single-id get_chunk_lifecycle_status_tx and adds get_chunk_lifecycle_statuses_tx/get_chunk_lifecycle_statuses_conn, which batch IN (...) queries in MAX_FETCH_BATCH windows and return a HashMap omitting null/missing statuses; new tests verify batch results match single reads and empty input returns an empty map.
Persist transaction wiring src/openhuman/memory/ingest_pipeline.rs	Collects staged chunk IDs and fetches prior lifecycle statuses via the batched call, dropping HashMap import in favor of HashSet, and adjusts the match logic to use prior.get(...).cloned() directly when deciding whether to schedule extract jobs.

Estimated code review effort: 2 (Simple) | ~12 minutes

Sequence Diagram(s)

sequenceDiagram
  participant IngestPipeline
  participant ChunksStore
  participant SQLite

  IngestPipeline->>IngestPipeline: collect staged chunk IDs
  IngestPipeline->>ChunksStore: get_chunk_lifecycle_statuses_tx(ids)
  ChunksStore->>SQLite: SELECT id, lifecycle_status WHERE id IN (...)
  SQLite-->>ChunksStore: rows
  ChunksStore-->>IngestPipeline: HashMap of prior statuses
  IngestPipeline->>IngestPipeline: decide CHUNK_STATUS_PENDING_EXTRACTION per chunk

Loading

Poem

A hop, a batch, a single query flight,
No more one-by-one lookups late at night 🌙
HashMaps hum where loops once crept,
Lifecycle statuses, neatly kept.
This rabbit thumps in pure delight! 🐇✨

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly summarizes the main change: batching prior-lifecycle reads in memory ingest to remove an N+1 query pattern.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[senamakel]

Automated technical review: not approved.

Unmet gates:

1. Merge conflict. The PR has merge conflicts (mergeStateStatus: DIRTY, mergeable: CONFLICTING). It must be rebased onto the latest main before it can be merged.

2. CI failures. Three checks fail:

   • Markdown Link Check -- pre-existing issue (broken fragment anchors in translated docs, not related to code changes).
   • Rust Core Coverage (cargo-llvm-cov) -- pre-existing compilation error in the tinyagents dependency (variant AgentEvent::ToolCompletedhas no fieldstarted_at_ms``), not caused by this PR.
   • PR CI Gate -- aggregate failure due to the above.
     The CI must be green (including pre-existing issues resolved on main) before approval.

N+1 claim: The approach is correct. The new get_chunk_lifecycle_statuses_tx function correctly batches the prior-lifecycle read using WHERE id IN (...) windowed at MAX_FETCH_BATCH, mirroring the existing get_chunks_batch / get_chunk_embeddings_for_signature_batch patterns. The test coverage (batch matches single reads for present/missing ids, empty input issues no query) is thorough. The ingest pipeline change (collecting staged ids, calling the batch function, adjusting the prior.get from .cloned().flatten() to .cloned() since the batch returns HashMap<String, String> instead of HashMap<String, Option<String>>) is behavior-preserving.

Once rebased and CI is green, this PR is ready for approval.

[mysma-9403]

Closing this as superseded rather than rebasing — thanks for the review, @senamakel, but the rebase isn't viable: the code this PR optimized no longer exists in the repo.

Since this branch was opened, main refactored the ingest persist path out from under it:

• The persist() function and its per-chunk get_chunk_lifecycle_status_tx loop — the N+1 this PR collapsed — were removed from src/openhuman/memory/ingest_pipeline.rs (that file now holds only the ingest_chat / ingest_email / ingest_document entry points).
• Chunk-store persistence moved into the vendored tinycortex crate; memory_store/chunks/store.rs is now a thin delegator (get_chunk_lifecycle_status_tx → tinycortex::memory::chunks::get_chunk_lifecycle_status_tx).
• memory_store/chunks/store_tests.rs, where this PR added its coverage, was deleted; ingest is now structured under memory/ingestion/, which no longer carries any per-chunk lifecycle/extract-decision loop.

A trial rebase confirms this: a modify/delete conflict on the (now-deleted) test file plus a ~190-line content conflict in ingest_pipeline.rs, because the whole persisted region is gone. Re-homing the batched SELECT … WHERE id IN (…) would now mean changing tinycortex itself (a separate repository), which is out of scope for this PR and may already be handled differently there.

The N+1 finding was validated at the time, but the in-repo target it applied to has been refactored away, so there's nothing left here to land. Closing as superseded.

[mysma-9403]

Superseded by the ingest refactor that moved chunk-store persistence into the tinycortex crate and removed the per-chunk lifecycle loop from ingest_pipeline.rs. Details in the comment above.