Repository navigation
Conversation
📊 Automated PR Analysis
SummaryBatches grouped grep/rg recall writes for SQLite recovery storage into a single transaction, moving hashing/gzip compression before the write lock and consolidating eviction into one final pass. This fixes a cap/retention regression where hints were advertised for rows that didn't survive eviction, causing Review Checklist
Linked issues: #3946 Analyzed automatically by wshm · This is an automated analysis, not a human review. |
yeahjack
force-pushed
the
fix/recall-batch-search-3946
branch
from
October 12, 2026 20:54
49919c9 to
7aa2e47
Compare
yeahjack
marked this pull request as ready for review
October 12, 2026 21:19
1 of 2 tasks
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.
Stack and review scope
This is the final stacked integration PR for #3946. Its
developcomparison intentionally includes the integrated dependency chain below; none of these PRs is merged by this change.Review the batching work separately in the batch-only commit diff. Its exact parent is
e20801b6ae93a7b47d9d51b93b5f899699353363. The subsequent Windows test-stack configuration retains every parser test and assertion.f9bdd36,9514ad5321b646e78d67be2831be58d10e2c03f33e,e20801bSummary
Fixes #3946.
The original cap regression is reproduced on develop: four files with
max_entries = 2produce four hints, but the first two recall commands exit 1. The batch now advertises only surviving captures. Duplicate payloads keep distinct immutable offsets rather than borrowing the most recent capture's offset.Performance
The final comparison uses the exact matched predecessor
e20801b6ae93a7b47d9d51b93b5f899699353363, so both binaries contain all companion features. Rust 1.99.0 release profile, shared Linux x86_64 host; 100 files × 80 matching lines;rtk grep --max 2500 -rn needle corpus; 30 interleaved measured runs after three warm-ups:Both SQLite cases preserve 100 per-file capture hints. Gzip median is ~73% lower against this integrated predecessor, largely from removing repeated per-file capture-pruning work. This is not a 73% improvement against unmodified develop. The initial standalone comparison against current develop
311ebcbshowed ~15% lower median; both experiments are documented separately.At the default 200-result display cap (nine captures), gzip median is 34.31 → 32.40 ms; raw 24.91 → 22.52 ms. The one-file check shows no observed regression (gzip 5.29 → 4.80 ms; disabled control 4.74 → 4.25 ms). Small-case differences are within host variability.
A separate phase benchmark uses a fresh store for each codec so immutable deduplication cannot reuse the other encoding:
The preparation difference isolates roughly 4.7 ms of incremental gzip work from database writes. These are workload-specific observations on a noisy shared host, not timing guarantees or a reuse of the old issue's
ddc21826numbers.Reproduction:
scripts/benchmark-recall-search.pyanddocs/contributing/RECALL_BENCHMARK.md.Test plan
cargo fmt --all -- --check,cargo clippy --all-targets,cargo test --all: 4,459 passed, 9 ignoredcargo doc --no-depsandgit diff --checkcargo test --release --bin rtk test_benchmark_recall_batch_phases -- --ignored --nocapturedddd94247ff595f6b0919668c4d0452b9be80718: Linux/macOS/Windows tests, fmt, clippy, rustdoc, benchmark, test presence, and security checks pass; CLA passes; master-target guard is correctly skipped fordevelopWindows test configuration
The combined debug Clap graph exceeded the Windows libtest worker stack in
hook_event_option_does_not_consume_nested_command_operandsand the existingtest_bun_build_parses_to_arg_vector(process exitSTATUS_STACK_OVERFLOW, not an assertion failure or a timeout). Windows CLI startup already reserves an 8 MiB main-thread stack; test workers independently default to 2 MiB. The build now supplies the same 8 MiB minimum for Cargo-launched Windows test workers, including localcargo test. Cargo also supplies the setting tocargo runand inherited children, preserving a valid larger caller stack setting; a directly launched installed CLI is unaffected. Direct-harness instructions are documented. No tests, parser assertions, or production parsing semantics are removed or relaxed.The hook event option from #4541 is root-only, not a Clap global option replicated across subcommands. This stack config is for the full combined command graph; it does not attribute the threshold to one companion PR. Cargo's runtime environment propagation is documented; Rust documents the separate worker-stack setting.
Known separate limitation
Custom tracking-history erasure through
RTK_DB_PATHor[tracking].database_pathis outside this stack and remains covered by the existing #3044. The forget integration here covers recall/capture storage and legacy tee files; it does not claim that a custom tracking database was erased.