Repository navigation
fix(search): route rtk grep/rg through the already-bounded capture path - #3134
Closed
albatrossflyon-coder wants to merge 1 commit into
Closed
albatrossflyon-coder wants to merge 1 commit into
albatrossflyon-coder wants to merge 1 commit into
Conversation
Fixes rtk-ai#3106. engine_capture() and passthrough() in search.rs called exec_capture_stdin, which wraps Command::output() and buffers the entire child process's stdout/stderr in memory with no limit — on a large search corpus this could consume 50+ GB before the 200-result cap in search.rs ever got a chance to apply. rtk already has a memory-safe capture path for exactly this: run_streaming with FilterMode::CaptureOnly reads line-by-line through the existing RAW_CAP (10 MiB), already covered by test_run_streaming_raw_cap_at_10mb in stream.rs. It just wasn't being used by grep/rg. Added exec_capture_stdin_bounded as a thin CaptureResult-shaped wrapper around run_streaming so both call sites in search.rs needed a one-line swap; no changes to search.rs's grouping/capping logic. Removed the now-unused exec_capture_stdin (the unbounded function my change replaced). Testing: - Full suite: 2457 passed, 8 ignored (unchanged from before this fix — the bounded-capture behavior itself is already covered by test_run_streaming_raw_cap_at_10mb). - Manual verification: `rtk grep` against a real 36MB file where every line matches. Completes cleanly (exit 0) and stderr shows "[rtk] warning: output exceeds 10 MiB — filter input truncated" — proof the cap is genuinely active now (this warning could not have fired through the old unbounded path). - cargo fmt --check and clippy (-D warnings): both clean.
albatrossflyon-coder
force-pushed
the
fix/rg-stream-memory-bound
branch
from
August 27, 2026 23:15
cbc76c1 to
a91df0e
Compare
Collaborator
|
Closing in favour of #3849, which bounds the same unbounded search capture and goes further: it caps per line as well as per stream, reaps the child on a broken pipe, and adds regression tests for both cases. Please take any further discussion of the capture path to that PR. |
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.
Fixes #3106.
The bug
engine_capture()andpassthrough()insearch.rscalledexec_capture_stdin, which wrapsCommand::output()and buffers the entire child process's stdout/stderr in memory with no limit — on a large search corpus this could consume 50+ GB before the 200-result cap insearch.rsever gets a chance to apply.The fix
rtk already has a memory-safe capture path for exactly this:
run_streamingwithFilterMode::CaptureOnlyreads line-by-line through the existingRAW_CAP(10 MiB), already covered bytest_run_streaming_raw_cap_at_10mbinstream.rs. It just wasn't being used bygrep/rg.Added
exec_capture_stdin_boundedas a thinCaptureResult-shaped wrapper aroundrun_streaming, so both call sites insearch.rsneeded a one-line swap — no changes tosearch.rs's grouping/capping logic at all. Removed the now-unusedexec_capture_stdin(the unbounded function this replaces).Testing
test_run_streaming_raw_cap_at_10mb, so this PR doesn't need new unit tests to prove that mechanism works; it only needed to provesearch.rsis actually using it).rtk grepagainst a real 36MB file where every line matches. Completes cleanly (exit 0) and stderr shows[rtk] warning: output exceeds 10 MiB — filter input truncated— proof the cap is genuinely active now (this warning could not have fired through the old unbounded path).cargo fmt --checkandcargo clippy --bin rtk -- -D warnings: both clean.