Skip to content

fix: preserve CPU headroom during background indexing - #2022

Open
mvanhorn wants to merge 1 commit into
DeusData:mainfrom
mvanhorn:fix/1084-background-index-cpu
Open

fix: preserve CPU headroom during background indexing#2022
mvanhorn wants to merge 1 commit into
DeusData:mainfrom
mvanhorn:fix/1084-background-index-cpu

Conversation

@mvanhorn

@mvanhorn mvanhorn commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?

Carry an internal background-execution marker on the index requests created by both session auto-index and watcher re-index paths, preserving it through daemon coordination and supervised-worker serialization. When handle_index_repository builds the pipeline, translate that marker into pipeline execution context rather than changing the public MCP tool schema or adding a user-facing configuration key. Make full and incremental pipeline worker-count selection consult that context and use the existing background/incremental default that leaves CPU headroom; retain the current CBM_WORKERS precedence, single-thread crash-recovery behavior, and all-core default for explicit/manual indexing.

Enabling auto_index has repeatedly caused high CPU usage and severe Windows UI stutter, including a fresh confirmation on v0.10.8. Main already contains the maintainer-identified non-Git auto_index_limit guard, dirty-state watcher deduplication, and subprocess RSS isolation, so reimplementing those fixes would be a no-op. The remaining production path still treats automatic first indexing like a foreground full index: the pipeline selects the initial=true worker policy, whose documented behavior is to use every detected core because “the user is waiting.” Automatic session and watcher jobs run in the background, so they should instead use the repository's existing headroom-preserving worker policy while explicit indexing retains its current throughput.

Fixes #1084

Checklist

  • Every commit is signed off (git commit -s) — required, CI rejects
    Not run: no test command resolved in this workspace, so nothing was executed to pass.
    unsigned commits (DCO, see CONTRIBUTING.md)
  • Tests pass locally (make -f Makefile.cbm test)
    Not run: no test command resolved in this workspace, so nothing was executed to pass.
  • Lint passes (make -f Makefile.cbm lint-ci)
    Not run: no test command resolved in this workspace, so nothing was executed to pass.
  • New behavior is covered by a test (reproduce-first for bug fixes)

@mvanhorn
mvanhorn requested a review from DeusData as a code owner September 3, 2026 07:21
@github-actions

github-actions Bot commented Sep 3, 2026

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

DeusData commented Sep 3, 2026

Copy link
Copy Markdown
Owner

Welcome, and thank you for the PR 👋 — one quick, mechanical thing before review, so you are not left guessing at the red.

dco is failing because your commit is not signed off. c846ed7b ("fix: preserve CPU headroom during background indexing") carries no Signed-off-by trailer, and this repository enforces DCO on push.

The fix is to amend the commit with a sign-off and force-push your branch:

  • git commit --amend -s --no-edit-s appends the trailer using your configured user.name and user.email
  • then force-push your own PR branch (with lease), naming the branch explicitly

If you end up with more than one commit, git rebase --signoff <base> does the whole branch at once.

Two things that will save you time when the rest of CI reports:

  • test-msan is currently failing for everyone — it dies at the Docker image build step, before any test runs. If you see it red, it is ours, not yours.
  • test-windows-guards has a standing red on test_daemon_stability.py this week, likewise unrelated to any diff.

I will review the change itself properly once it is signed off — background-indexing CPU headroom is a good thing to be looking at, and +209/-12 over 8 files is a reviewable size.

@DeusData

DeusData commented Sep 3, 2026

Copy link
Copy Markdown
Owner

Attributed your reds so you are not chasing nine separate things — there are really only two, and one of them is a genuinely interesting test-design point.

1. dco — still unsigned

c846ed7b still has no Signed-off-by trailer. Amend with -s and force-push your own branch (see my earlier note).

2. The sanitizer legs — your own new test, and worth thinking about

The failure is not spread across nine problems. On the macOS TSan leg:

mcp_auto_index_in_process_uses_background_worker_policy
  FAIL tests/test_mcp.c:13035: selected_workers == 2, expected cbm_default_worker_count(false) == 4

That is the test this PR adds, asserting:

ASSERT_EQ(selected_workers, cbm_default_worker_count(false));

against your production change:

workers = cbm_default_worker_count(!p || !p->background);

The assertion re-evaluates the helper rather than naming an expected number, so it silently assumes nothing between cbm_default_worker_count(false) and the value the pipeline actually selects can change the answer. The run says otherwise: the helper returned 4 while the pipeline chose 2. Something downstream — a clamp, a cap, or an environment-sensitive input — is intervening.

That is worth pinning down rather than papering over, because it is the same shape as a class of bug we have been clearing out of this suite all week: assertions whose verdict depends on the machine rather than the code. A test that compares a captured value against a live re-evaluation of a helper will agree on your laptop and disagree on a constrained runner, and neither result tells you whether the policy is right.

Two questions that should settle it:

  • Is 2 the value your change intends on that runner, and the helper's 4 simply not the right comparison? Then assert the policy relationship directly, or assert against whatever the pipeline is actually given.
  • Or is the pipeline clamping below your intended policy? Then the production change is not yet doing what the description says, and the test has correctly caught it.

Either way the test is doing its job — it failed rather than passing vacuously, which is more than a lot of new tests manage.

Not everything red is that

For completeness: the test-unix (ubuntu-latest) leg reports 2216 passed, 0 failed and failed at a later step, so that one is environmental rather than yours. And test-msan fails at the Docker image build with the suite skipped — a DNS resolution failure inside buildkit, entirely ours.

I have not reviewed the change on merit yet — I will once it is signed off and the worker-count question is resolved. Preserving CPU headroom during background indexing is a good thing to be fixing.

@adfjadfj16-a11y

This comment has been minimized.

@DeusData

DeusData commented Sep 3, 2026

Copy link
Copy Markdown
Owner

Maintainer notice: please disregard comments from @adfjadfj16-a11y on this thread

@adfjadfj16-a11y is not a maintainer of this project and does not speak for it. That account has posted replies on 17 threads here written in the project's voice — promising merges, announcing that a case has been "escalated to the development team", asking to close issues, and in some threads replying as though it were the author of someone else's pull request. None of those were maintainer decisions, and none of them carried any weight.

@DeusData is the only account that gives a maintainer response on this repository. If a comment about the fate of your issue or pull request did not come from @DeusData, it is not a decision, however official it reads.

If you were waiting on something because of one of those comments — a promised merge, a review "immediately", a request to close your ticket — I am sorry. That was noise you had no way to identify as noise, and it should not have been on your thread. Your issue or PR is judged on its own merits, and I will answer it here myself.

Nothing in this notice reflects on your contribution. Thank you for your patience, and thank you for the work.

@DeusData DeusData added bug Something isn't working stability/performance Server crashes, OOM, hangs, high CPU/memory priority/high Needs near-term maintainer attention; high-impact bug, regression, safety issue, or release blocker. labels Sep 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working priority/high Needs near-term maintainer attention; high-impact bug, regression, safety issue, or release blocker. stability/performance Server crashes, OOM, hangs, high CPU/memory

Projects

None yet

Development

Successfully merging this pull request may close these issues.

CPU memory usage is too high

3 participants