perf: overlap the OSV query with extraction, and scan files once

[kridaydave]

Measured against a loopback registry, a 200-file review spends 717 ms on
the wall clock and 45 ms of CPU. The gap is two things: a blocking OSV
round trip in the critical path, and a serial evaluation loop in the ci
lane.

1. The advisory query no longer blocks the review. resolve_advisories
   returns Ready for everything answerable without the network (recall
   hit, policy switch, warm cache, an ecosystem with no coverage) and
   Pending with the OSV request already running otherwise, so the
   tarball fetch, extraction and diff all proceed while the network
   works. join collects it at scoring time. The upstream disclosure
   behaviour is preserved verbatim: an Err stays a HIGH finding, an
   unverified report the operator accepted stays a MEDIUM one, and
   fail_closed is still checked before any stale fallback is served.

2. The ci lane reviews packages in parallel. One worker per package,
   capped at eight, each with its own ReviewContext and its own SQLite
   handle, since neither is shareable. Findings are emitted in
   candidate order rather than completion order, because CI output is
   diffed between runs. The budget check still refuses before any work
   starts.

3. Each file is read once through a 64 KiB window instead of being
   buffered whole for a hash, a NUL scan and a classify. The digest is
   unchanged, because it decides whether a file is compared at all, and
   peak memory stops scaling with the largest file in the package. The
   UTF-8 carry across the window boundary completes by the sequence's
   real length, and a carry still open at EOF is judged malformed, so a
   file ending mid-code-point no longer reads as text.

4. The dev profile emits line tables rather than full DWARF, and emits
   none for dependencies. Cargo's default is opt-level 0 with
   debug = 2, which at this crate's size is what accumulates a hundred
   stale incremental caches. Cargo.lock does not move.

PERF.md carries the measurements, the commands that produced them, and
what is still not known. scripts/perf_harness.py reproduces the runtime
numbers and scripts/perf_watchdog.sh bounds the process count of a test
run.

[kridaydave]

Reviewed against head 6141dba, checked out locally (git rev-parse HEAD matches gh pr view --json headRefOid). Single commit, so every finding attributes to it.

Gate: ./scripts/verify.sh → PASS Format,Clippy,Tests,References, exit 0 (792 lib tests). Toolchain caveat: no rustup on this box, so the 1.97.1 pin is inert and this built with Fedora 1.98.1.

Blockers

1. CHANGELOG.md is not updated. The PR touches 12 files; CHANGELOG.md is not among them. AGENTS.md: "After each merged PR, add an entry to CHANGELOG.md under [Unreleased] in the same branch."

2. The digest is load-bearing and has no test — I confirmed a surviving mutant.

scan_file (src/diff.rs:414) is new and replaces fs::read + hash + classify. The PR body states "The digest is unchanged, because it decides whether a file is compared at all" — that decision is src/diff.rs:120, if base_meta.hash != target_meta.hash. A digest that covered fewer bytes than it read would make base and target agree on a changed file and silently skip the comparison; nothing downstream would notice.

I mutated it to confirm the suite is blind:

// src/diff.rs:433, mutated
if len <= SCAN_CHUNK_BYTES as u64 {
    hasher.update(chunk);
}

Result: 792 passed, 0 failed. The whole suite is green with a digest that only covers the first 64 KiB window. Reverted and re-ran to confirm the restore (clean vs HEAD, 792 pass).

chunked_reads_agree_with_a_single_read_on_the_classification (src/diff.rs:676) asserts only kind, never hash; the sole hash token in the test module is a struct literal at src/diff.rs:735. One assertion closes it — on a file spanning several windows:

assert_eq!(scan_file(&p).unwrap().hash, Sha256::digest(&bytes).into());

AGENTS.md: "A patch with no test that fails without it is a guess."

3. PERF.md is newly wrong about the tree it describes. Added whole by this PR (345 lines, no deletions), and:

• cites src/sandbox.rs five times (lines 254, 255, 259, 260, 342) — that file does not exist in this tree;
• asserts "concurrency anywhere in src/ | none, no rayon, no par_iter" (line 90), which this PR contradicts;
• opens with "Nothing here has been fixed" (line 3) while implementing items 1, 2 and 4 of its own §7;
• §7 item 3 promises "Add a [profile.dev] … so old session caches are evicted rather than accumulating. Reclaims most of the 1.9 GB." The Cargo.toml added sets debug only — no incremental = false, no cache bound. Smaller debuginfo shrinks each of the 101 caches; it does not stop them accumulating. The PR body's causal claim ("debug = 2 … is what accumulates a hundred stale incremental caches") does not hold either — unbounded incremental retention is what does that.

scripts/check-references.sh only resolves citations under references/, so none of this is caught by the gate.

Suggestions

4. The CI lane no longer short-circuits on the first package error. evaluate_candidates (src/ci.rs:423) drains slots in candidate order after every worker finishes, so a failure in package 3 does not stop package 97 from being reviewed. Previously the serial ? aborted at the first failure, so later packages were never fetched and never wrote to the store. On a failing run the store now receives record_verified and advisory-cache writes from packages the serial lane never reached. Both are evidence-only (clean = 0, idempotent), so this is low risk — but it silently widens what a failed run mutates in the trust anchor, and that deserves an explicit decision rather than falling out of the threading change.

5. A PendingAdvisory is dropped unjoined on every early-exit path between src/review.rs:130 and src/review.rs:282 — ctx.fetch_tarball(...)? (138), the checksum check (140), extract_for_ecosystem (150), prepare_extracted_root (164), store.record_verified(...)? (178), and baseline resolution. Each fires an OSV query whose answer and cache write are discarded. The doc comment at src/advisory.rs:164 covers thread safety but not the wasted round trip and lost cache fill.

6. The AUR lane is left serial. evaluate_aur_ci_diff (src/ci.rs:492) still loops evaluate_package one at a time, and PERF.md §7 item 2 named both loops. AUR entries are capped at 4096 and each does a git archive, so the worst case is real. The PR body's "The ci lane reviews packages in parallel" reads as unqualified.

7. Eight concurrent registry clients, no rate limiting or jitter. MAX_CI_REVIEW_WORKERS = 8 (src/ci.rs:359) means up to ~24 in-flight requests against the configured base, which --registry allows to be arbitrary. Fine for npm/crates.io; a small self-hosted mirror will see a burst.

8. Two expect() calls in new production code (src/ci.rs:472, src/ci.rs:483). Permitted by the repo's lint config (only unwrap is disallowed), but both assert an invariant that holds only because std::thread::scope re-panics before slots is drained. The message "every index is claimed before the scope ends" is load-bearing; a line on why it cannot fire would save the next reader the trace.

9. Public API break, no in-tree callers. pub fn fetch_advisories is gone, replaced by resolve_advisories returning a new AdvisoryResolution enum. src/lib.rs:7 re-exports pub mod advisory, so any downstream embedder breaks. Grep found no callers outside advisory.rs. Acceptable for a CLI-first crate; worth a CHANGELOG line once #1 is done.

Residual risk I could not close

• No mutation testing. AGENTS.md requires --mutants for work touching extraction, integrity and policy; src/diff.rs and src/advisory.rs both qualify, and blocker #2 is exactly the class of survivor it exists to catch. The Utf8Scan carry (src/diff.rs:344–378) is intricate enough that I would expect more survivors there.
• I traced the Utf8Scan carry by hand and believe it correct — tail from an error_len() == None error is at most 3, so pending[..tail] cannot overrun [u8; 4]; pending[0] is always a valid lead byte because error_len() == None implies a valid prefix; and the len <= MAX_DIFF_FILE_BYTES skip before feed is safe because an over-cap file classifies OpaqueTooLarge regardless. Reasoning, not a test run.
• Toolchain mismatch, as above: 1.98.1 locally vs the pinned 1.97.1 in CI.
• The gate does not run the two dogfood jobs (this repo's own Cargo.lock and package-lock.json); those need the release binary.

What is good

The advisory split is a clean design. Splitting fallback_or_fail from the policy check so fail_closed is captured at resolve_advisories and applied at join keeps the security ordering in one place, and moving the cache write into join genuinely is the right call — the worker never needs the connection, and the write it enables costs less than the reopen it avoids. The coverage_unknown policy-based distinction between "operator chose this" and "coverage hole discovered at runtime" is the right shape and is tested from both sides. The Pending drop-is-safe contract is documented and covered by a test. The windowed scan's NUL/UTF-8 short-circuit past the diff cap is correct for the reason stated in the comment.

[kridaydave]

Review addressed in a17a87c. All three blockers and every suggestion.

Blocker 1 — CHANGELOG. [Unreleased] now carries all four changes: the advisory overlap (including the fetch_advisories → resolve_advisories API break), the CI parallelism (including the drain-all-candidates behaviour change and the AUR half staying serial), the windowed scan, and the profile change. PERF.md and the harness scripts are listed under Added.

Blocker 2 — the digest mutant. Confirmed and killed. the_windowed_digest_equals_the_whole_file_digest (src/diff.rs) pins scan_file().hash against Sha256::digest of the whole buffer at sizes crossing the window boundary at every offset of one, plus zero and an exact multiple. Verified in both directions: your exact mutant (hasher.update guarded by len <= SCAN_CHUNK_BYTES) makes it FAIL, and the real code passes. Gate still green.

Blocker 3 — PERF.md. Fixed on every count: the src/sandbox.rs citations are gone (item 5 now says the file does not exist on this tree and why), the zero-concurrency row is corrected, the header no longer claims nothing is fixed, and item 3 is marked PARTIALLY IMPLEMENTED with an explicit note that debug = line-tables-only shrinks the caches but nothing here evicts them — cargo has no knob for that, so a real fix is periodic eviction. The PR body's causal claim about debug = 2 was wrong too; the commit message no longer makes it.

Suggestions.

• Added contributor ground rules and removed sources section from README #4: the drain-all decision is documented in evaluate_candidates as deliberate, with the evidence (store writes are clean = 0 and idempotent).
• Add install command with safe scripts execution and harden security boundaries #5: the unjoined-Pending cost on early-exit paths is documented on PendingAdvisory.
• Harden security boundaries and enforce safety invariants across extraction, execution, and registry clients #6: the AUR serial lane is documented on evaluate_aur_ci_diff; PERF.md item 2 says IMPLEMENTED for the lockfile half only.
• Hardened release extraction, SSRF checks, heuristics, and executor isolation #7: the registry burst tradeoff is documented on MAX_CI_REVIEW_WORKERS.
• Update conventions in AGENTS.md #8: the expect invariant now says why it cannot fire.
• Fix IPv6 mutation testing by eliminating redundant address checks #9: covered by the CHANGELOG entry.

Residual risk still open: mutation testing was not run on this branch — it was killed mid-run before these fixes and has not been re-run. Your mutant is closed by hand-verification above, but AGENTS.md wants ./scripts/verify.sh --mutants for extraction-boundary work and I have not produced that run. Running it before merge.

[kridaydave]

Verified against a17a87c, checked out locally. Gate: ./scripts/verify.sh → PASS Format,Clippy,Tests,References, exit 0 (793 lib tests). Restored clean vs HEAD before the final run.

Blockers — all three closed

1. CHANGELOG. Present and specific: the [Unreleased] entry names the fetch_advisories → resolve_advisories break and its blast radius, states that every security ordering is unchanged, and covers the drain-all and AUR-serial behaviour changes. This is the right level of detail for a breaking-change entry.

2. Digest mutant — killed, and I re-ran the check myself. the_windowed_digest_equals_the_whole_file_digest (src/diff.rs:722) asserts scan_file().hash against Sha256::digest of the whole buffer across 14 sizes: zero, an exact window multiple, and n, 2n, 2n+1 around each of SCAN_CHUNK_BYTES - 1, SCAN_CHUNK_BYTES, SCAN_CHUNK_BYTES + 1. I re-applied the same mutant and ran the gate:

test diff::tests::the_windowed_digest_equals_the_whole_file_digest ... FAILED
assertion `left == right` failed: digest differs from the whole-buffer hash at 131073 bytes
test result: FAILED. 792 passed; 1 failed

Fails for the stated reason, on a size that crosses the boundary, and the rest of the suite still passes under the mutant — so it is precisely scoped rather than incidentally green. Restored, confirmed clean vs a17a87c, gate green at 793.

3. PERF.md. Fixed on every count I raised. The five src/sandbox.rs citations are gone; item 5 is retitled NOT APPLICABLE TO CURRENT MAIN and says why. The header no longer claims nothing is fixed and scopes itself to 2d7864e, "a tree that predates this file". Every §7 item now carries an IMPLEMENTED / PARTIALLY IMPLEMENTED / NOT APPLICABLE marker, item 2 correctly says the lockfile half only, and item 3 says exactly what the diff does and does not deliver — that debug = line-tables-only shrinks the 101 caches while unbounded incremental retention is what accumulates them, and that cargo has no knob, so a real fix is periodic eviction. That is a more honest answer than the one I asked for.

Suggestions — all six addressed

#4 through #8 are documented where a reader will actually hit them, and each carries its evidence rather than a bare assertion: the drain-all decision states that the writes are clean = 0 and idempotent and that one transport failure does not invalidate the other ninety-nine reviews; the unjoined-Pending cost says why re-querying beats threading a join through every early exit; the registry-burst note names the concrete exposure ("a spike against a small self-hosted mirror, which --registry permits") and gives the remedy; the expect invariant at src/ci.rs now explains why it cannot fire. #9 is covered by the CHANGELOG.

Residual risk — unchanged, and still the reason not to merge yet

Mutation testing has not been run. AGENTS.md requires ./scripts/verify.sh --mutants for work touching extraction, integrity, policy or the heuristic rule engine, and src/diff.rs and src/advisory.rs both qualify. My mutant was closed by hand, but I picked it because I had already read the code — that is the floor, not the evidence. The Utf8Scan carry (src/diff.rs:344–378) is the part I would most expect a survivor in: tail from an error_len() == None error being bounded by 3, and pending[0] always being a valid lead byte, are both invariants no test pins. A mutant that widened the carry to a fixed four bytes, or that dropped the finish() check, would be worth having CI find rather than us.

Also unchanged: this box has no rustup, so the 1.97.1 pin is inert and my runs used Fedora 1.98.1 — the same gap PERF.md §8 admits. And the gate does not cover the two dogfood jobs, which need the release binary.

Verdict

No blockers remain. The three fixes are correct and each was verified rather than accepted on description; the suggestion-level work went in as reasoned documentation, not as rubber stamps. One thing stands out: the item-3 correction in PERF.md is the diff disagreeing with its own PR body in writing, which is the behaviour I would want from a fail-closed tool's docs.

Ready to merge once --mutants produces a clean run. That is the last gate.

[kridaydave]

Heads-up on --mutants, because the first answer would have been wrong: the run did not complete, and it never reached diff.rs. I am not calling this a clean run.

What happened. I started ./scripts/verify.sh --mutants in the background. The debug log shows it was interrupted at ~2024s (~34 min) partway through an advisory.rs mutant — same interruption as before, this run died on its own rather than being killed by me. The mutants.out/ files I first read (caught: 7, missed: 0, timeout: 33) are stale: they belong to a previous partial run, and the 33 "timeout" rows are inconclusive, not passes. Of the mutants this run actually processed, 0 missed, but that covers roughly 52 mutants across src/advisory.rs and src/main.rs only.

What that means for the residual risk. The part the review flagged as most likely to hide a survivor — the Utf8Scan carry in src/diff.rs, where tail being bounded by 3 and pending[0] always being a valid lead byte are both unpinned invariants — was not mutation-tested at all. The digest mutant is still closed, but by the review's own hand-verification and my re-run, not by this run.

So the verdict stands as the review left it: no blockers remain, and the last gate is still open. This is not that gate.

What a real run needs. It takes well over an hour on this box (each mutant rebuilds the lib: ~16s build + test per mutant, against 4300+ mutants in the crate). The practical options:

1. Scope it: cargo mutants --file src/diff.rs --file src/advisory.rs tests only the two extraction/advisory-boundary files the review named, in a fraction of the time.
2. Run the full --mutants with no timeout, backgrounded, and let it finish before merge.
3. Merge now with the residual risk explicitly accepted, on the strength that the digest mutant is closed and the carry logic is reasoned-correct but unpinned by mutation.

I have not restarted it — option 1 is what I would pick if the goal is the review's specific concern (the Utf8Scan carry), and it is fast. Say which you want and I will run it, or leave it as the known-open item.

[kridaydave]

Traced the --mutants failures from CI. Everything else passes (Tests, Clippy, Format, both dogfoods, supply-chain, citations); only the mutation shards were red — 8 survivors, all in the new perf code. Fixed in 17e9bc2.

The 8 survivors and their disposition

Shard	Mutant	Disposition
1	advisory.rs:390 match guard report.hits.is_empty() → true and → false	killed
3	ci.rs:441 replace <= with > in evaluate_candidates	excluded (equivalent)
4	diff.rs:312 replace * with + (window const)	killed
7	diff.rs:322 delete arm 0x00..=0x7f, :323 delete arm 0xc2..=0xdf	killed
8	diff.rs:325 delete arm 0xf0..=0xf4	killed
15	diff.rs:367 replace < with > in Utf8Scan::feed	killed
24	diff.rs:456 replace > with >= in classify_bytes	killed

The reviewer's exact worry was right: the Utf8Scan carry and the seq_len lead-byte table were unpinned. All three seq_len arms, the carry guard, the window constant and the cap boundary each survived because no test drove them.

Killed (7), each verified both directions

New tests, each confirmed to FAIL under its mutant and PASS on the real code:

• seq_len_maps_every_utf8_lead_byte_class — pins all four length classes plus both rejection ranges (continuation bytes 0x80..0xbf, overlong 0xc0/0xc1, 0xf5..0xff).
• a_carry_completes_only_once_the_whole_sequence_arrives — drives a 4-byte sequence one byte at a time into the same scan, so a partial carry meeting a still-too-short follow-up is exercised (that's the case the < guard protects). First draft fed fresh scans and didn't kill the mutant; fixed to drip into one scan.
• the_scan_window_is_sixty_four_kibibytes — pins the memory bound.
• classify_bytes_draws_the_opaque_boundary_above_the_cap_only — a file exactly at the cap is still Text; one byte past is OpaqueTooLarge.
• the_cache_ttl_distinguishes_clean_from_vulnerable_from_failed — the OSV TTL guard split into a pure cache_ttl_secs (so it's testable without a network mock) and pinned for clean 12h / vulnerable 1h / failed 0.

Excluded (1), with a reason in the workflow

replace <= with > in evaluate_candidates is a true equivalent mutant: workers <= 1 takes the serial path, workers > 1 takes the scoped path, and both return items in candidate order — the scoped path handles workers == 1 correctly, so no input produces a different report. The only difference is one extra store.reopen(), invisible in the result. It cannot be killed by any behavioural test, so it's added to BLUELINE_MUTANTS_EXCLUDE (both the PR and push jobs) with the reasoning in a comment, following the existing pattern for the sandbox-spawn exclusions.

Gate re-run green (Format, Clippy, Tests, References). New CI run will confirm the shards.