| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
|
Thanks for opening this — it has been seen, and it is queued. This note is automated, but it is not a brush-off: it exists so you know where your PR stands instead of having to guess from silence. Current review status: working through a backlog. 0.9.1-rc.1 is out, so the release freeze that held reviews is over — but it left a large queue of open pull requests behind it, and we are reading through them oldest-first. The background is in discussion #1144. What that means for this PR, concretely:
Things that will genuinely speed it up whenever review does happen:
If this fixes a bug, a reproduction we can run is worth more than a description of the symptom. Thanks for contributing, and sorry in advance for the wait. |
Sorry, something went wrong.
|
Maintainer decision: the cancellation fix is wanted, but it needs a bound in the same change. Real cancellation is a genuine improvement — today a cancelled Windows search keeps running, which is a bug. Moving onto the supervised subprocess path so cancel kills the tree and removes the temp file is the right shape. The problem is what it gives up. cbm_popen provides pipe back-pressure: a producer outrunning the consumer blocks. Writing to log_file and parsing afterwards removes that, so the pathological query from #1565 — "the letter a" across 184k files — now materialises its full output into the cache directory before we read a byte of it. Unbounded. That trades a hang for filling the user's disk, which is not obviously the better failure. So a bound has to land with it. A byte ceiling on the temp file is the one to use, not a result cap: it bounds the resource without touching result semantics at all, so it cannot silently drop matches the way a producer-side row cap can (see my note on #1607 — we are not taking that, and search_code is getting generic cross-platform pagination instead). Concretely: cap the temp file, and when the cap is hit, fail loudly rather than parsing a truncated file as if it were complete. A partial scan reported as a complete one is the failure mode we care most about avoiding. Two other things for when you revise: this will need a rebase, since it rewrites the same mcp.c:9803 call site that #1604 renames and #1604 is going in first. And there is an unrelated cbm_regfree leak fix riding along on the POSIX error path — that is a genuine fix and worth its own one-line PR, where it will land immediately instead of waiting on this decision. Also worth saying: you used cbm_fopen correctly for the temp file. That is a rule people miss constantly here, and it matters — raw fopen breaks non-ASCII paths on Windows. |
Sorry, something went wrong.
|
Updated per review: the supervised Windows scan now has a 64 MiB byte ceiling, cancels the contained process tree when exceeded, validates final size before parsing, fails explicitly, and removes the temporary output. The unrelated POSIX regex cleanup is now #1637. Focused MCP suite: 192 passed, 7 platform skips. |
Sorry, something went wrong.
|
This is the right shape, and I checked the part that mattered rather than taking the summary on trust: if (scan_output_exceeded) return search_code_scan_error(...) — it errors out instead of parsing a truncated file. That was the property I actually cared about. A partial scan reported as a complete one is the failure mode this project treats as a correctness bug, and you closed it explicitly. A byte ceiling is also the right kind of bound. It caps the resource without touching result semantics, so unlike a row cap it cannot silently drop matches the caller has no way to know about. 64 MiB is a sensible number — comfortably above any real search, comfortably below filling someone's disk. The test seam for overriding the limit is a nice touch; it makes the exceeded path actually testable rather than theoretical. Cancelling the contained process tree, validating final size before parsing, and removing the temp file on the way out means the failure path is ownership-complete, which is what the original cbm_popen back-pressure was quietly giving you for free. Three pieces of feedback, three clean turnarounds — the debug flag on #1605, the leak split out as #1637, and the bound here. Thank you; that is unusually easy to review. Merging once CI clears. #1604 wants to land first since it renames the same function this touches. |
Sorry, something went wrong.
Signed-off-by: Ertan <ertan.kucukoglu@gmail.com>
Trivial insertion-point collision in tests/test_mcp.c: both this branch and main (DeusData#1704's UTF-8 pin) add a Windows search_code test at the same spot. Both tests kept.
The conflict's shared tail (#endif + closing brace) was emitted once for two adjacent Windows tests; re-add the closer so both tests compile. Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
|
Brought the branch up to date with main for you (merge commit, no history rewrite): main just landed a Windows search_code UTF-8 fix that adds a test at the same insertion point as your two new tests — pure textual collision, all three tests kept. Composition verified locally: mcp+cli suites 484/484 on the merged tree. CI reruns now; merging on green. Thanks again for the careful supervised-scan work! |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
What does this PR do?
Propagates MCP request cancellation to the Windows PowerShell process used by search_code and bounds its temporary output.
The scan now runs through the supervised subprocess path. Cancelling a request terminates the process tree and removes temporary command output files.
A 64 MiB byte ceiling now monitors the command output while the process is running and checks the final size before parsing. Hitting the ceiling cancels the contained process tree and returns an explicit error; truncated output is never parsed or reported as complete.
Regression coverage verifies both cancellation and output-limit failures, including cleanup of .mcp-command-* files. The unrelated POSIX cbm_regfree cleanup was split into #1637 as requested.
Rebased onto current main; the shared command-builder call site will receive the final rename-only rebase after #1604 lands.
Test results: focused MCP suite: 192 passed, 0 failed, 7 skipped.
Part of #1565
Checklist