fix(mcp): give the over-budget error a retry value, and say the peak is not the requirement - #2201
Open
DeusData wants to merge 1 commit into
Open
fix(mcp): give the over-budget error a retry value, and say the peak is not the requirement#2201DeusData wants to merge 1 commit into
DeusData wants to merge 1 commit into
Conversation
…is not the requirement v0.10.9 introduces Decision A: an over-budget index attempt fails whole with a named error instead of thrashing. The error told the caller to "Raise CBM_MEM_BUDGET_MB" but not to what, so the obvious next move is to read peak_rss_mb and retry just above it. That move fails, every time, by construction. The abort fires WHEN resident memory crosses the budget, so peak_rss_mb is pinned just above the budget -- it is where indexing was stopped, not what the repository needs. Measured on the linux kernel (94521 files, 43.1M LOC) while benching v0.10.9: default budget 24576 MB -> aborted, peak_rss_mb 25622 completing the same index actually took 31.75 GB So the reported peak understates the real requirement by 24%, and 1.32x the budget was needed. A caller retrying at peak+10% would have burned another 30 seconds to fail again with a nearly identical message. The response now carries suggested_budget_mb (1.5x the current budget, which would have cleared the kernel case) and the hint says plainly that peak_rss_mb is a floor, not a target. Computed as (3*b+1)/2 rather than b + b/2: integer division makes the latter degenerate to b at b == 1, so the suggestion would have repeated the budget that just failed -- the extended test caught exactly that. Not changed: the abort itself, the backpressure ladder, and worker cleanup. A clean test confirmed workers do NOT survive the abort (process count 2 before, 2 after, both idle at 0% CPU), so there was nothing to fix there. tests/test_mcp.c now pins both halves -- a concrete value strictly greater than the budget, and the hint stating the peak is where indexing STOPPED. Verified against the real pre-fix output captured during the bench, which contains neither. mcp suite 317 passed, 0 failed, 4 skipped; lint-ci clean. Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
DeusData
enabled auto-merge
September 13, 2026 12:52
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
1 participant
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.
Found while benching v0.10.9 for release. Small, but it fixes a message that actively misleads.
The problem
v0.10.9 introduces Decision A: an over-budget index fails whole with a named error instead of thrashing. The error said:
It names the knob but not a value, so the natural next step is to read
peak_rss_mband retry just above it.That retry fails every time, by construction. The abort fires when RSS crosses the budget, so
peak_rss_mbis pinned just above the budget — it is where indexing was stopped, not what the repo needs.Measured on the Linux kernel (94,521 files / 43.1M LOC) during the release bench:
The reported peak understates the requirement by 24%. A caller retrying at peak+10% burns another 30s to fail again with a near-identical message.
The change
suggested_budget_mb— 1.5× the current budget, which would have cleared the kernel case (it needed 1.32×)peak_rss_mbis where indexing STOPPED, not a targetComputed as
(3*b+1)/2, notb + b/2: integer division makes the latter degenerate tobatb == 1, so the "suggestion" would repeat the budget that just failed. The extended test caught exactly that on the first run.Deliberately not changed
The abort, the backpressure ladder, and worker cleanup are untouched.
Worth recording: worker cleanup was suspected broken (a 24.6 GB process appeared to outlive an abort). A clean test disproved it — process count 2 before the abort, 2 after, both idle at 0% CPU. The stray process belonged to an overlapping bench leg of my own. No fix was needed, so none was made.
Verification
tests/test_mcp.cpins both halves: a concrete value strictly greater than the budget, and the hint stating the peak is where indexing stopped. Checked against the real pre-fix output captured during the bench, which contains neither string — so the test is genuinely red without this change.make -f Makefile.cbm lint-ci: cleanRelease context
Benching v0.10.9 against the v0.10.0 baseline on the same host and corpus found no performance regression: cold index 289.6s → 226.7s (−21.7%), nodes stable (+0.02%). Edges fell 4.2%, traced by an A/B edge-set diff to removal of false cross-arch/cross-tooling calls (e.g. 23,745 edges pointing at a Python gdb helper; 10,323 bound to SPARC
writelof which only 15 callers were SPARC) — a graph-quality improvement, not a loss.