Skip to content

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

Open
ertankucukoglu wants to merge 1 commit into
DeusData:mainfrom
ertankucukoglu:fix/search-code-cancellation
Open

fix(mcp): bound and cancel Windows code search#1608
ertankucukoglu wants to merge 1 commit into
DeusData:mainfrom
ertankucukoglu:fix/search-code-cancellation

Conversation

@ertankucukoglu

@ertankucukoglu ertankucukoglu commented Aug 14, 2026

Copy link
Copy Markdown
Contributor

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

  • Every commit is signed off (git commit -s)
  • Focused MCP tests pass locally (make -f Makefile.cbm test-focused TEST_SUITES=mcp CC=clang CXX=clang++)
  • New behavior is covered by regression tests

@github-actions

Copy link
Copy Markdown

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.

@DeusData

Copy link
Copy Markdown
Owner

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.

Signed-off-by: Ertan <ertan.kucukoglu@gmail.com>
@ertankucukoglu
ertankucukoglu force-pushed the fix/search-code-cancellation branch from fb7169c to 9b8d456 Compare August 14, 2026 15:02
@ertankucukoglu ertankucukoglu changed the title fix(mcp): propagate cancellation to Windows code search fix(mcp): bound and cancel Windows code search Aug 14, 2026
@ertankucukoglu

Copy link
Copy Markdown
Contributor Author

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.

@DeusData

Copy link
Copy Markdown
Owner

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants