Consume the placed source items through one iterator, not a front drain by mysma-9403 · Pull Request #7476 · tinyhumansai/openhuman

mysma-9403 · 2026-10-11T20:28:25Z

Summary

Follow-up to Store memory source items in batches, as backfill already does #7253, which merged before this landed: store_all drains the front of a Vec per chunk, which is O(N² / STORE_CHUNK) of copying over a sync.
Consumed through one iterator instead, so each placed item is moved exactly once.
No behaviour change — same chunks, same order, same batch and retry paths.

Problem

#7253 added batching with:

while !placed.is_empty() {
    let take = placed.len().min(STORE_CHUNK);
    let chunk: Vec<_> = placed.drain(..take).collect();

Vec::drain(..n) removes from the front, so every chunk shifts all still-pending entries left. Over a sync of N items that is O(N² / STORE_CHUNK) of memmove, paid on the tokio worker — and paid precisely on the large syncs the batching exists to speed up, which makes it worst where the change was supposed to help most. Raised by Codex review on #7253 (P2); the PR merged before the fix was pushed, so it is here.

Solution

let mut placed = placed.into_iter();
loop {
    let chunk: Vec<_> = placed.by_ref().take(STORE_CHUNK).collect();
    if chunk.is_empty() {
        break;
    }

Each entry is moved once, into its chunk. chunks() over a slice would avoid the per-chunk Vec as well, but it cannot be used here: a rejected batch hands nothing back, so the item-by-item retry needs owned StoreItems and the chunk has to be owned either way. What this removes is the repeated memmove, which is the part that scaled with N.

The comment stays next to it, because drain(..n) reads as the obvious way to write this loop and the next person needs to know why it is not.

Submission Checklist

Tests added or updated (happy path + at least one failure / edge case) per Testing Strategy — no new test: this is the same traversal with the same chunk boundaries, and store_all_spans_chunk_boundaries_without_losing_items (250 items over three chunks, plus a bad item in the middle chunk) already pins exactly what a mistake here would break — a dropped, duplicated or misordered entry. store_all_skips_a_bad_item_but_fails_when_nothing_stored covers the failure path unchanged.
Diff coverage ≥ 80% — the changed lines are the loop header, executed by every store_all test; cargo test -p openhuman --lib memory::sources:: → 26 passed, 0 failed.
Coverage matrix updated — N/A: no feature row added, removed or renamed.
All affected feature IDs from the matrix are listed in the PR description under ## Related — N/A: as above.
No new external network dependencies introduced — none.
Manual smoke checklist updated if this touches release-cut surfaces — N/A: no release-cut surface and no behaviour change.
Linked issue closed via Closes #NNN in the ## Related section — N/A: follow-up to merged Store memory source items in batches, as backfill already does #7253 (its origin issue Memory source sync stores one item per request while backfill already batches #7252 is closed); filing a fresh issue for a ten-line loop rewrite would be noise. Referenced below instead.

Impact

Platform: core only, one function.
Performance: removes O(N² / STORE_CHUNK) copying from a source sync. No change to request count, which Store memory source items in batches, as backfill already does #7253 already fixed.
Security / Compatibility / Migration: none.

Follow-up to Store memory source items in batches, as backfill already does #7253 (merged), Codex P2 on that PR. Origin issue Memory source sync stores one item per request while backfill already batches #7252.
Not fixed here, filed separately because it is a decision rather than a defect: Batched source sync reports complete before earlier items are recallable #7474 — the same merge changed when a non-final item of a batch becomes recall-settled.
Same domain, separate: Task source fetch opens SQLite twice per task to answer the dedup ledger #7366 / Read the task-source dedup ledger once per fetch, not twice per task #7367.

AI Authored PR Metadata (required for Codex/Linear PRs)

Linear Issue

Key: N/A
URL: N/A

Commit & Branch

Branch: perf/sources-store-single-pass
Commit SHA: see git log upstream/main..HEAD

Validation Run

pnpm --filter openhuman-app format:check — cargo fmt applied; no frontend file changed.
pnpm typecheck — N/A: no TypeScript changed.
Focused tests: cargo test -p openhuman --lib memory::sources:: (26 passed, 0 failed).
Rust fmt/check (if changed): bare cargo fmt, GGML_NATIVE=OFF cargo check -p openhuman, cargo clippy -p openhuman -- -D warnings (exit 0).
Tauri fmt/check (if changed): N/A: crates/openhuman-app/ not touched.

Validation Blocked

command: pnpm rust:clippy, the pre-push hook's clippy gate, so this branch was pushed with --no-verify.
error: clippy::result_large_err at crates/openhuman-rpc/src/server/saas_gateway.rs:102.
impact: pre-existing on main and unrelated — this PR changes one loop in crates/openhuman-core/src/memory/sources/sync.rs. Not a toolchain difference: rust-toolchain.toml pins 1.96.1 and the run used rustc 1.96.1 / clippy 0.1.96. Clippy over the package this touches is clean.

Behavior Changes

Intended behavior change: none. Same chunks, same order, same batch and retry paths; only how the items are moved into each chunk.
User-visible effect: none beyond a large sync no longer spending time shifting a vector.

Parity Contract

Legacy behavior preserved: take(STORE_CHUNK) yields the same chunks in the same order as drain(..take), and the empty-iterator break replaces the empty-vector condition, so an empty input still enters no batch. Everything inside the loop is untouched.
Guard/fallback/dispatch parity checks: the per-item retry, the is_account_wide() early return and the touched / stored / failed accounting are unchanged, and the chunk-boundary test exercises all of them across three chunks.

Duplicate / Superseded PR Handling

Duplicate PR(s): none
Canonical PR: this one

Summary by CodeRabbit

Performance
- Improved the efficiency of storing items in batches. Batch storage, retry behavior, error handling, and storage accounting remain unchanged.

…ont drain Raised in review: `placed.drain(..take)` removes from the front of a `Vec`, so every chunk shifts all still-pending entries left — O(N^2 / STORE_CHUNK) of copying across a sync, paid precisely on the large syncs this batching exists to speed up, which makes it worse than a wash on the case that matters. `into_iter()` with `by_ref().take(STORE_CHUNK)` moves each entry exactly once. The chunk still has to be owned, because the item-by-item retry needs the items a rejected batch does not hand back, so `chunks()` over a slice would not avoid the allocation — only the repeated memmove, which is the part that scaled. The comment stays next to it: `drain(..n)` reads as the obvious way to write this, so the reason it is not needs to be visible.

chatgpt-codex-connector · 2026-10-11T20:28:33Z

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-10-11T20:30:38.154712Z	`e82a239`	PR opened

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

coderabbitai · 2026-10-11T20:29:18Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI
Review profile: CHILL
Plan: Advanced
Run ID: d900175d-f424-42d4-9c18-e592ed5ef0b8

📥 Commits

Reviewing files that changed from the base of the PR and between 22ddfaa and e82a239.

📒 Files selected for processing (1)

crates/openhuman-core/src/memory/sources/sync.rs

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 8 remain after this review.

📝 Walkthrough

Walkthrough

store_all now collects placed items by advancing an iterator, rather than repeatedly draining the front of a vector. The batch limit and storage behavior remain unchanged.

Changes

Memory storage

Layer / File(s)	Summary
Batch collection `crates/openhuman-core/src/memory/sources/sync.rs`	`store_all` collects each batch by advancing an iterator. It retains the existing chunk limit and stops when no items remain.

Priority: ⬇️ Low

Estimated code review effort: 1 (Trivial) | ~5 minutes

Change: Refactor

Suggested reviewers: senamakel

Merge Risk: ⚪ Minimal · up to e82a2

The iterator-based collection preserves the existing batch and retry behavior, with multi-chunk handling covered by the supplied test. No material merge risk is identified.

Pre-merge checks | 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check		Check skipped - CodeRabbit’s high-level summary is enabled.
Title check		The title clearly and concisely describes the main change: replacing repeated front draining with a single iterator for placed source items.
Docstring Coverage		Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 1 files.
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches 💡 1

🛠️ Fix failing CI checks 💡

Commit to this branch
Create a new PR

Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

_{A rabbit checks the batches flow,
Each chunk moves on without a shove.
No front-row shifting slows the way,
The storage steps through, clear and neat.
Then all the placed items hop along.}

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

tinysweeper · 2026-10-12T10:16:24Z

Tiny Sweeper review

This pull request is a single-purpose internal performance refactor in `pub(crate) async fn store_all(`: the per-chunk front drain of `placed` is replaced by consuming the same vector through one iterator in `STORE_CHUNK`-sized batches. Every review lane (critique, security, tests, commits, description) reported no findings. The critique lane verified boundary behaviour (batches of exactly `STORE_CHUNK` while items remain, an empty batch once exhausted, and the `chunk.is_empty()` break covering empty input), and confirmed the chunk is moved into the `touched` loop or the per-item retry loop without re-reading `placed`, so nothing is skipped or double-counted and the early return on account-wide errors preserves pre-existing intent. Security found no new input paths, sinks, or permission changes; commits found nothing sensitive. The description lane states the PR body's problem statement, solution, and no-behaviour-change claim match the change. No findings were raised, softened, or deprioritized. Each lane noted that the code index is behind this pull request (indexed at `98e870e9bdad`), so retrieved context may be out of date.

State: Ready for maintainer review
Priority: none
Reviewed head: e82a239bbcad
Updated: 2026-10-12T10:19:04Z

Review snapshot

Change surface	Files	Review signal
Production	1	Active findings
Tests	0	Noted findings
Documentation	0	Resolved findings
Configuration	0	Pending checks/questions

Completeness: Complete
Test assessment: No supported feature-to-test mapping was available; this does not mean tests are absent or passed.

What changed

`store_all` no longer drains the front of `placed` in a `while !placed.is_empty()` loop. Instead it converts `placed` into a consuming iterator and loops, taking up to `STORE_CHUNK` items per iteration and breaking when the collected chunk is empty. This changes only how pending items are pulled: chunk size, chunk order, the number of batches, the per-chunk clone for the item-by-item retry path, and the accounting of successful versus rejected items are all unchanged. The stated motivation is that draining the front of a `Vec` shifts every still-pending entry left, costing copying work proportional to the pending backlog across the sync, which is exactly the workload the batching is meant to accelerate. Reviewer lanes describe this as a pure performance refactor preserving the same chunk size, retry, and accounting semantics.

Features

Internal refactor — Batch consumption of pending source items in store_all via a single consuming iterator: Replaces the O(N^2 / STORE_CHUNK) front-drain chunking of `placed` with one iterator consumed in `STORE_CHUNK`-sized batches, preserving chunk size, order, empty-input handling, and every downstream retry and accounting path. Reviewer lanes report no behavioural change, no new correctness or security defect, and no new input paths, sinks, or permission changes; the benefit is reduced copying on large syncs. (crates/openhuman-core/src/memory/sources/sync.rs, crates/openhuman-core/src/memory/sources/sync.rs#pub(crate) async fn store_all()

Tests

No supported feature-to-test mapping was produced. Test execution is not inferred.

Findings

No active actionable findings.

Before merge

None.

How this fits together

flowchart LR
  n0["store_all"]:::impacted
  n1["sync_one"]:::impacted
  n2["start_sync"]:::impacted
  n3["reader_entry"]:::impacted
  n4["sync_due"]:::impacted
  n5["...ling_sync_lands_in_error_with_the_message"]:::impacted
  n1 -->|calls| n0
  n1 -->|calls| n3
  n4 -->|calls| n2
  n5 -->|calls| n2
  n5 -->|tests| n2
  classDef changed fill:#0d4429,stroke:#238636,color:#e6edf3
  classDef impacted fill:#161b22,stroke:#6e7681,color:#c9d1d9
  classDef flagged fill:#5a1e02,stroke:#d93f0b,color:#ffffff
  classDef blocking fill:#67060c,stroke:#f85149,color:#ffffff

Agent review details

critique

Conclusion: Success
Scope reviewed: all assigned evidence
Positive: Boundary behaviour of the new loop was checked: `take(STORE_CHUNK).collect()` yields exactly `STORE_CHUNK` items while more remain, an empty vec once the iterator is exhausted, and the `chunk.is_empty()` break correctly handles an empty input list, matching the existing expectation that `store_all` on an empty vec returns 0. The chunk is moved into the `touched` loop on success or the per-item retry loop on rejection, and neither loop re-reads `placed`, so nothing is skipped or double-counted; the early return before consuming the remainder occurs only on account-wide errors, which is the pre-existing intent.
Lane summary: The change replaces the O(N²) `drain(..take)` loop with a consuming iterator in `store_all`. I checked the boundary behaviour: `take(STORE_CHUNK).collect()` yields exactly `STORE_CHUNK` items while more remain, an empty vec once the iterator is exhausted, and the `chunk.is_empty()` break correctly handles an empty input list (matching the existing test that `store_all` on an empty vec returns 0). `chunk` is moved into the `touched` loop on success or the per-item retry loop on rejection, and neither loop re-reads `placed`, so nothing is skipped or double-counted; `store_all` returns before the remainder is consumed only on account-wide errors, which is the pre-existing intent. No findings. _The code index is behind this pull request (indexed at `98e870e9bdad`), so retrieved context may be out of date._

security

Conclusion: Success
Scope reviewed: all assigned evidence
Positive: The refactor preserves the same chunk size, retry, and accounting semantics and introduces no new input paths, sinks, or permission changes; no security or correctness defect is introduced.
Lane summary: The change replaces a `drain(..n)` chunking loop in `store_all` with a consuming iterator, preserving the same chunk size, retry, and accounting semantics; it is a pure performance refactor with no new input paths, sinks, or permission changes. No security or correctness defect is introduced. _The code index is behind this pull request (indexed at `98e870e9bdad`), so retrieved context may be out of date._

tests

Conclusion: Success
Scope reviewed: all assigned evidence
Lane summary: The only behavioural change is replacing `placed.drain(..take)` with an iterator-based chunking loop; the two forms consume the same chunks in the same order, and the existing `store_all_spans_chunk_boundaries_without_losing_items` test in sync_tests.rs already covers the boundary behaviour (count-exactly-once assertion plus a bad item mid-chunk). No test file changed, but the change is a mechanical swap of an equivalent cursor, so no new coverage is owed. I was not able to confirm `STORE_CHUNK` is nonzero/blocksize by reading its definition; if it were zero the new loop would spin forever, but `take(0)` would also have made the old loop a no-op, so it is contiguous either way. _The code index is behind this pull request (indexed at `98e870e9bdad`), so retrieved context may be out of date._

commits

Conclusion: Neutral
Scope reviewed: all assigned evidence
Positive: Nothing sensitive found in what this pull request commits.
Lane summary: Nothing sensitive found in what this pull request commits.

description

Conclusion: Success
Scope reviewed: all assigned evidence
Positive: The PR body's problem statement, solution, and stated no-behaviour-change claim match the diff, which replaces the per-chunk front drain with a single `into_iter()` consumed in `STORE_CHUNK`-sized batches while preserving chunk size, order, empty-input handling, and every downstream retry/accounting path.
Lane summary: The diff replaces the per-chunk `Vec::drain(..take)` front drain with a single `into_iter()` consumed in `STORE_CHUNK`-sized batches, which preserves chunk size, order, empty-input handling and every downstream retry/accounting path. The PR body's problem statement, solution and stated no-behaviour-change claim match the change exactly, so it is safe to merge. _The code index is behind this pull request (indexed at `98e870e9bdad`), so retrieved context may be out of date._

Evidence and run details

Models: ladder/vectors-oai3, deepseek-v4.1-flash, glm-5.3-flash
Spend: $0.001207
Tokens: 98900 input · 5625 output · 9472 cached · 78 embedding

Head	State	Pass summary
`e82a239bbcad`	ready for maintainer review	0 active finding(s), 0 resolved finding(s) (at 2026-10-12T10:19:04Z)

tinysweeper

tinysweeper found nothing blocking. Approving.

             $0.0012 · 98,900 in / 5,625 out · 9,472 cached (10%) · ladder/vectors-oai3, deepseek-v4.1-flash, glm-5.3-flash · 78 embedded
critique:    $0.0004 · 30,151 in / 489 out   · 4,096 cached (14%) · deepseek-v4.1-flash
security:    $0.0000 · 30,315 in / 346 out   · 3,584 cached (12%) · deepseek-v4.1-flash
tests:       $0.0000 · 13,344 in / 171 out   · 1,792 cached (13%) · deepseek-v4.1-flash
description: $0.0006 · 23,241 in / 2,872 out · 0 cached (0%)      · deepseek-v4.1-flash, glm-5.3-flash

coderabbitai Bot approved these changes Oct 11, 2026

View reviewed changes

tinysweeper Bot approved these changes Oct 12, 2026

View reviewed changes

tinysweeper Bot added the priority: p3 Whenever. Cosmetic, a nicety, or a cleanup with no user visible effect. label Oct 12, 2026

Navigation Menu

Repository navigation

Consume the placed source items through one iterator, not a front drain - #7476

Consume the placed source items through one iterator, not a front drain#7476
mysma-9403 wants to merge 1 commit into
tinyhumansai:mainfrom
mysma-9403:perf/sources-store-single-pass

mysma-9403 commented Oct 11, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

chatgpt-codex-connector Bot commented Oct 11, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot commented Oct 11, 2026 •

edited

Loading

Walkthrough

Changes

Uh oh!

tinysweeper Bot commented Oct 12, 2026 •

edited

Loading

critique

security

tests

commits

description

Uh oh!

tinysweeper Bot left a comment

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

mysma-9403 commented Oct 11, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Problem

Solution

Submission Checklist

Impact

Related

AI Authored PR Metadata (required for Codex/Linear PRs)

Linear Issue

Commit & Branch

Validation Run

Validation Blocked

Behavior Changes

Parity Contract

Duplicate / Superseded PR Handling

Summary by CodeRabbit

Uh oh!

chatgpt-codex-connector Bot commented Oct 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codex Review Summary

Uh oh!

coderabbitai Bot commented Oct 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Uh oh!

tinysweeper Bot commented Oct 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Tiny Sweeper review

Review snapshot

What changed

Features

Tests

Findings

Before merge

How this fits together

critique

security

tests

commits

description

Uh oh!

tinysweeper Bot left a comment

Choose a reason for hiding this comment

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

mysma-9403 commented Oct 11, 2026 •

edited by coderabbitai Bot

Loading

chatgpt-codex-connector Bot commented Oct 11, 2026 •

edited

Loading

coderabbitai Bot commented Oct 11, 2026 •

edited

Loading

tinysweeper Bot commented Oct 12, 2026 •

edited

Loading