fix(mcp): bound and cancel Windows code search - #1608

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:

• It will not be closed for inactivity. No stale bot touches pull requests here.
• It may still sit a while before a human reads it. That is on us, not on you.
• Older PRs are read first, so a recent one is not being skipped — it is behind a queue.

Things that will genuinely speed it up whenever review does happen:

• Keep it rebased on main — the tree is moving quickly right now, and a conflicting branch cannot be reviewed as the diff you intended.
• Get CI green, or say which failures you believe are pre-existing.
• Keep the change to one claim. Bundled features and refactors get split before they get merged, which costs you a round trip.
• Every commit needs a sign-off (git commit -s) — CI enforces DCO.

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.

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.

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.

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.