Repository navigation
fix(memory_tree): code quality improvements on batch tree fetch (PR #2976 follow-up) - #3020
Conversation
`memory_tree::tree::flush::flush_stale_buffers` previously issued `store::get_tree(config, &buf.tree_id)?` once per stale buffer — one SQLite round-trip per row returned by `list_stale_buffers`. On hosts with many stale L0 buffers (e.g. after a long backfill or a cron miss) each flush pass paid N round-trips before the cascade even started. Replace the per-buffer loop with one upfront batched read: - `memory_store::trees::store::get_trees_batch(config, &[ids]) -> Result<HashMap<String, Tree>>` `flush_stale_buffers` now dedups buffer tree_ids into a `HashSet`, issues one `SELECT … WHERE id IN (?,?,…)` (1 round-trip), then per-id HashMap lookup routes each buffer to its own `Tree` for `cascade_all_from`. The L0-only filter in `list_stale_buffers` keeps the same tree_id at most once today, but the dedup is defensive against future widenings of that filter that would otherwise re-pay the cost. ### Defensive chunking — TREES_MAX_FETCH_BATCH = 500 `get_trees_batch` iterates via `chunks(TREES_MAX_FETCH_BATCH)`. - SQLite's compile-time `SQLITE_MAX_VARIABLE_NUMBER` has been ≥ 32 766 since 3.32 → 500 leaves a ~65× safety margin. - The `flush_stale_buffers` call-site passes the deduped distinct tree_id set (typically << 500 in practice) → runs as one query. - Loop is purely defensive: future callers passing larger id lists will not blow up against a host with a lower compile-time SQLite cap. - No volume reduction: all input ids in → all matching rows out; the merged HashMap is byte-identical to one giant query. ### Semantic preservation — no behavioural change - `get_trees_batch` reuses `row_to_tree` so decoded `Tree` values stay bit-identical to the per-row path. - Missing ids stay silently absent from the HashMap, mirroring the per-row `get_tree` `Ok(None)` contract — the existing orphan-buffer warn-and-skip path keeps working unchanged. - Per-buffer iteration order preserved by walking `stale` directly; HashMap lookup keys by `&buf.tree_id`. ### Tests - `get_trees_batch_returns_present_ids_in_map` — both ids present, decoded `Tree` equal to the per-row path bit-for-bit. - `get_trees_batch_empty_input_and_missing_ids` — empty input → empty map (no SQL); missing id silently absent so callers keep their `Ok(None)` skip path. - `flush_seals_multiple_distinct_trees_via_batched_lookup` — two real trees each carry a stale L0 buffer; flush seals both via the batched fetch. Pins the HashMap-keyed-by-id contract: swapping the lookup key to position would route buffers to the wrong `Tree` and mis-write summaries against the wrong `root_id`. ### Companion to tinyhumansai#2927 / tinyhumansai#2941 / tinyhumansai#2948 Same playbook as the topic-rerank batch (tinyhumansai#2927), `fetch_leaves` batch (tinyhumansai#2941), and `hydrate_summary_inputs` batch (tinyhumansai#2948): partition / batch / decorate. Different table, different call-site, same pattern.
- Add SQL-injection-safe comment on format!() in get_trees_batch - Replace raw Vec<&dyn ToSql> with idiomatic params_from_iter - Document that dedup in flush_stale_buffers is for DB efficiency only - Rename and clarify the batched-flush regression test
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughThis pull request adds clarifying inline comments to the ChangesStale buffer flush logic clarification
Possibly related PRs
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
coderabbitai
Bot
added
the
memory
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
oxoxDev
left a comment
oxoxDev
left a comment
There was a problem hiding this comment.
Reviewed the code — the substance is good. get_trees_batch uses positional bind params (?1,?2,…, not interpolated, so not injectable), the 500-row batch cap stays under SQLite's 999-param limit, params_from_iter(window.iter().map(String::as_str)) is the right idiom, and the HashSet dedup → batch fetch correctly replaces the per-buffer get_tree calls. Rust Quality (fmt/clippy) is green.
Two things before merge:
-
PR Submission Checklist is failing. It's the unchecked box on line 14:
- [ ]cargo fmt --check— not runnable in this environment…
Rust Quality already runscargo fmt --checkin CI and it passes here, so either check the box or mark itN/A: covered by Rust Quality CI gate(note:N/Amust start the item text per the checklist parser). -
Overlap with #2976. This PR touches the same two files (
memory_store/trees/store.rs,memory_tree/tree/flush.rs) as the still-open #2976, so they'll conflict. Could you fold this into #2976 (push these review fixes onto that branch) or close #2976 in favour of this one? Right now both are open with the same change. Happy to re-approve once it's a single PR with a green checklist.
# Conflicts: # src/openhuman/memory_store/trees/store.rs # src/openhuman/memory_tree/tree/flush.rs
|
Merge conflicts resolved and Re: the two pre-merge items from @oxoxDev:
|
|
E2E status: Lane 2/4 failure is in |
…inyhumansai#2976 follow-up) (tinyhumansai#3020) Co-authored-by: Maciej Myszkiewicz <mmyszkiewicz@bwcoders.com> Co-authored-by: Steven Enamakel <enamakel@tinyhumans.ai> Co-authored-by: Taimoor <astikkosapparel009@gmail.com>
Summary
Follow-up to PR #2976 (
perf(memory_tree): batch tree fetch in flush_stale_buffers).Net change vs
mainflush_stale_buffers) — document that theHashSet+Vecdedup is purely for DB efficiency (fewer bind params), not for semantic ordering: the per-buffer loop walksstaledirectly andlist_stale_buffersorder is preserved.The other items originally proposed here (SQL safety comment,
params_from_itervs explicitVec<&dyn ToSql>, the regression test) are already present onmainvia the merged #2976, so they are intentionally not re-applied — re-churning approved-and-merged code would add divergence with no functional benefit.Test plan
cargo fmt --check— clean.cargo check(core) — clean.memory_store::trees::andmemory_tree::tree::flush::pass, includingflush_seals_multiple_distinct_trees_via_batched_lookup(38 passed).prettier --check— clean (no TS changes).Summary by CodeRabbit
Release Notes
No user-facing changes. This release includes internal code improvements and clarifications to enhance code maintainability.