fix(index): address qodo review findings on batch write and parse progress#11
Merged
Merged
Conversation
…gress - ladybug_store: move single-use row lists into Value::List instead of cloning them per stage; only the small shared path_rows is cloned. Adds a run_stage helper so each stage executes directly. - indexer: correct the manifest-error comment — parse errors now abort the run before the batched write stage (a more atomic failure mode than the old write-as-you-go loop). - indexer: harden the parse progress observer against a termination race — read `done` before the counter and always emit the final count before breaking; strengthen the worker increment to Release for a real happens-before on the final count. - Cargo.lock: bump synapse entry 0.2.1 -> 0.2.2 to match Cargo.toml so --locked CI/release builds pass. Verified: cargo fmt --check, clippy --all-targets, test --locked (47 pass), and three independent re-index runs of a 1,593-file repo produce byte-identical symbol/package/edge output (determinism preserved).
Review Summary by QodoFix batch write clones, progress race, and Cargo.lock version sync
WalkthroughsDescription• Eliminate unnecessary clones in batch write by moving single-use row lists into Value::List instead of cloning per stage; only the small shared path_rows is cloned • Correct manifest-error comment to reflect actual atomic failure mode: parse errors now abort before batched write stage, not during write-as-you-go loop • Harden parse-progress observer race condition by reading done before counter and always emitting final count; strengthen worker increment to Release ordering for happens-before guarantee • Sync Cargo.lock synapse entry from 0.2.1 to 0.2.2 to match Cargo.toml for --locked CI/release builds Diagramflowchart LR
A["Batch write stage<br/>with clones"] -->|"Extract run_stage<br/>helper"| B["Move single-use lists<br/>Clone only path_rows"]
C["Parse progress observer<br/>race condition"] -->|"Reorder reads<br/>Strengthen ordering"| D["Read done first<br/>Emit final count<br/>Release semantics"]
E["Cargo.lock<br/>out of sync"] -->|"cargo update"| F["synapse 0.2.2<br/>matches Cargo.toml"]
B --> G["Determinism verified<br/>3 runs byte-identical"]
D --> G
File Changes1. src/graph/ladybug_store.rs
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Addresses the four qodo PR-review findings.
Changes
1. Avoid large clones in
write_files_batch(src/graph/ladybug_store.rs)The stage loop cloned every row list per stage, including the large single-use
symbol_rows/declares_rows. Replaced it with arun_stagehelper that moves theVec<Value>intoValue::List. Single-use lists move in by value (no clone); only the small sharedpath_rowsis cloned for its first two of three stages. Query semantics unchanged.2. Correct the manifest-error comment (
src/indexer/mod.rs)The comment claimed parse errors abort "exactly as the previous inline
?did". The new pipeline defers writes until after parse/drain, so a parse error now aborts the run before the batched write stage — a more atomic failure mode than the old write-as-you-go loop. Comment rewritten to describe the actual behaviour.3. Harden the parse-progress observer race (
src/indexer/mod.rs)The observer could break on
done == truewithout having observed the finalparsed_count, leaving the last update belowtotal. Reordered to readdonefirst, then load + emit the counter, breaking only after that final emission. Strengthened the worker increment tofetch_add(Release)so — paired with thecollect()join beforedone.store(Release)and theAcquireloads — there's a real happens-before guarantee on the final count.4. Sync
Cargo.lockwith the version bumpThe root
synapseentry was still0.2.1whileCargo.tomlis0.2.2, which fails--lockedCI/release builds. Bumped viacargo update -p synapse.Verification
cargo fmt --check,cargo clippy --all-targets,cargo test --locked(47 pass) — all clean.index --forceruns of a 1,593-file repo (10,935 symbols / 61,547 reference edges). Symbol dump, package list, and depth-2relatededge traversals were byte-identical across all runs (matching SHA-256) — confirming the batch-write reorder and parallel-parse changes preserve determinism. The onlypackdiff was the git commit hash, which is a property of the test setup, not the indexer.