Remove two redundant copies from the bulk-scan write path (#182)

[ekalb81]

Summary

Fixes two concrete, standalone inefficiencies in the bulk-scan write path that issue #182 calls out, independent of that issue's still-undetermined memory-ceiling attribution.

• Fix 1 — commands.rs's on_session_batch closure called batch.to_vec(), a full deep clone of up to SCAN_WRITE_BATCH_SIZE (64) owned Sessions, before the write lock was even taken. scanner.rs drops its own batch immediately after invoking the callback, so nothing needed the clone. scan_all's on_session_batch callback now takes the batch by value (Vec<(PathBuf, Session)> instead of a borrowed slice) and commands.rs passes it straight through — no clone.
• Fix 2 — observe_bulk_batch's read-back looped over load_one, which opens a fresh Connection per call (up to 2x items.len() opens per batch), each also materializing another full Session copy, all while the durable-write mutex's hot path is right behind it. The read-back is now split into HistoryStore::load_batch_outcomes, which opens one reader connection via open_reader() and reuses it across every key in the batch.

Why fix 2 doesn't just call load_many

The issue suggested reusing load_many (added for #139) outright since it already does the same connection reuse. It has different semantics on purpose: load_many is best-effort per key — a missing row is skipped with a warning, so its output can be shorter than its input. Every caller of observe_bulk_batch's read-back zips the result 1:1 against items/written by position, so a silently shortened output would misalign every outcome after the missing key, not just drop the missing one — a much worse bug than the connection-open cost being fixed. load_batch_outcomes instead preserves load_one's original "any missing/errored key fails the whole call" semantics, keeping the caller's existing per-batch retry fallback (observe_bulk, one session at a time) triggering exactly when it did before.

A new test, observe_bulk_batch_read_back_matches_per_key_load_one_including_a_missing_key, calls the real load_batch_outcomes (not a hand-written stand-in) and a load_one-loop replica with identical keys, including a missing one, and asserts identical results and identical fail-the-whole-call behavior. I verified this test actually fails against a load_many-based implementation before restoring the real fix (see commit message for detail).

Measurement

examples/hydration_memory.rs measures a different phase (stream_sessions, the post-scan startup hydration read) and never touches scan_all, reconcile_scanned_batch_if_current, or observe_bulk_batch — it does not cover this write path. I wrote a new harness, examples/scan_write_batch_memory.rs, to measure it directly (never touches the real ledger; builds a disposable store under a temp dir on C:).

Fix 1 (isolated clone cost, 200 reps on a 64-session ~412 KB/session batch): ~19–29 ms wall time and ~21.3 MB peak allocator heap per clone call, both eliminated entirely. Extrapolated to field scan cardinality (~73 batches): ~1.4–2.1 s of pure clone time and one avoided ~25 MB peak allocation per in-flight batch.

Fix 2 (73 observe_bulk_batch calls, 4,672 fresh sessions, 2 runs each before/after by temporarily reverting just this change and rebuilding --release):

• Connection-open count drops exactly 64x (4,672 → 73) for the read-back half, by construction — not a measurement, a structural fact.
• Rust allocator heap peak was bit-for-bit identical before and after in every run. This is expected, not a bug: SQLite's connection opens allocate via the bundled C library's malloc, outside the crate's TrackingAllocator, so this fix's effect is invisible to the exact "Rust heap" dimension issue Startup ceiling is now bulk_scan at 1,491 MB, and it is Rust heap (85% of RSS) — predates v0.8.17 #182 attributes its 1,491 MB ceiling to (85% of RSS, confirmed Rust heap).
• Wall time was modestly and consistently lower after the fix (mean 38.7 s vs 41.6 s across 2 runs each, ~7%), but the individual runs overlap enough that this is suggestive, not conclusive, with only 2 samples per side.
• Process RSS/private-bytes deltas varied far more between repeated runs of the same binary (6.9 MB to 77.8 MB) than between before/after — noise at this harness's resolution dwarfs any signal, so no memory claim is made from those figures.

Not claimed: any effect on the 1,491 MB ceiling itself. Issue #182 is explicit that instrumentation must land before attribution, and nothing here adds that instrumentation. Both fixes are removals of redundant work on their own terms, reported as such — not a fix for the ceiling.

Validation

All six AGENTS.md commands pass: npm run check, npm test (174 vitest + 31 node tests), npm run build, cargo fmt --check, cargo clippy --all-targets --locked -- -D warnings (zero warnings), cargo test --locked (373 lib tests + parser/provider/scan-cache/sync-design integration suites, 0 failed, 17 ignored — Windows runs fewer than CI per AGENTS.md's store.rs note). Per AGENTS.md, green here is necessary but not sufficient; CI's visual-regression job still needs to run, though this PR is Rust-only and should be skipped by scripts/visual-impact.mjs.

Refs #182. Not merging — for review.

🤖 Generated with Claude Code

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: dc1fb0e31c

ℹ️ 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".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Remove the hard-coded developer directory

When this example is run, the value embeds a real developer-specific local path and is passed directly to remove_dir_all at both startup and cleanup, so it can recursively delete unrelated contents on the named machine and will not provide a reliable writable location for other users. Use a disposable tempfile::TempDir or require an explicit benchmark directory with a safety guard instead.

AGENTS.md reference: AGENTS.md:L13-L16

Useful? React with 👍 / 👎.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!