Skip to content

feat(index): bound indexing resources - #1348

Closed
liuchong wants to merge 4 commits into
DeusData:mainfrom
liuchong:fix/index-resource-guards
Closed

feat(index): bound indexing resources#1348
liuchong wants to merge 4 commits into
DeusData:mainfrom
liuchong:fix/index-resource-guards

Conversation

@liuchong

@liuchong liuchong commented Jul 30, 2026

Copy link
Copy Markdown

What does this PR do?

Adds one immutable, configurable resource policy for ordinary indexing requests and enforces it across discovery, supervised workers, and persistence.

  • rejects dangerous exact repository roots and supports operator-defined aggregate roots
  • bounds files, directories, entries, depth, source bytes, and discovery time
  • caps worker CPU parallelism, physical concurrency, memory, temporary output, and duration
  • checks database, staging, cache admission, and free disk while preserving the previous published index on failure
  • returns structured resource_limit_exceeded errors
  • prevents callers from weakening the trusted worker policy
  • documents defaults and keeps cross-repo-intelligence on its existing database-only path

Related to #1347

Validation

  • make -f Makefile.cbm test: 6791 passed, 4 skipped
  • Focused CLI suite: 257 passed
  • Focused ASan/UBSan suites: 640 passed, 3 platform skips
  • make -f Makefile.cbm lint-ci: passed
  • Production binary build: passed
  • config --help: verified all resource keys

Checklist

  • Every commit is signed off (git commit -s)
  • Tests pass locally (make -f Makefile.cbm test)
  • Lint passes (make -f Makefile.cbm lint-ci)
  • New behavior is covered by tests

@liuchong
liuchong requested a review from DeusData as a code owner July 30, 2026 00:10
@liuchong
liuchong force-pushed the fix/index-resource-guards branch from 0b33e28 to a42336e Compare July 30, 2026 07:37
liuchong added 3 commits July 30, 2026 17:26
Signed-off-by: 刘冲 <mail@liuchong.dev>
Signed-off-by: 刘冲 <mail@liuchong.dev>
Signed-off-by: 刘冲 <mail@liuchong.dev>
@liuchong
liuchong force-pushed the fix/index-resource-guards branch from 836f376 to 15a98bc Compare July 30, 2026 09:27
Use PROC_PIDTASKALLINFO so process-group membership and current resident memory are sampled together. This avoids zero resident-size observations from proc_pid_rusage on freshly spawned workers.

Refs DeusData#1347

Signed-off-by: 刘冲 <mail@liuchong.dev>
@DeusData

Copy link
Copy Markdown
Owner

Reviewed in full — every limit traced to its enforcement point and the defaults cross-checked against our recorded benchmarks. The mechanics are strong and the concern is entirely about defaults and surface size. Those are separate, and I do not want the second to overshadow the first.

What is genuinely good. The trust boundary is the best part of this PR: caller-supplied _cbm_index_limits is stripped and replaced with the server's own config policy before the worker spawns, a worker refuses a missing or incomplete policy, and there is a dedicated test that passes {"index_max_files":"999999"} and asserts the config limit still wins. test_worker_watchdog.sh then pins the full-policy contract so adding a key without updating the worker contract fails CI. That is how a supervised-worker boundary should be built.

The discovery semantics are also exactly right for this project: a limit violation aborts the request rather than returning a truncated file list — a partial index is never published as a complete one. Graph-quality-first is the rule here, and you landed on the correct side of it without being told.

Where it cannot ship as-is: the defaults contradict this project's own reference workload.

Default Our recorded reality Effect
index_memory_limit_mb 8192, worker killed on breach linux-kernel index peaks 16.4–33.6 GB RSS our flagship benchmark becomes impossible by default
index_cpu_cores 4 src/foundation/system_info.c:300 — "Use all cores for initial indexing — user is waiting" multi-fold index-time regression for everyone with >4 cores
index_max_files 100000 our own bench is ~94,500 files at 1× 1× squeaks through at 94.5%; 2× hard-fails
index_max_file_mb 64 src/foundation/limits.c:11-14 sets a deliberate 512 MiB cap, documented as "generous" an 8× tightening; files that index today start being skipped
index_scan_timeout_seconds 30 no discovery timeout today cold-cache, NFS and monorepo discovery can now fail the whole request

A resource policy that kills our own reference index out of the box is not a shape we can adopt. If the direction is approved, the version I would argue for is unlimited (off) by default, with the policy opt-in — the enforcement machinery keeps all its value and nothing regresses for existing users.

Two more things worth surfacing louder:

  1. cbm_pipeline_new now bakes default limits into every pipeline, so watcher background reindexes are bound too. A repo that grows past a cap after initial indexing has its refreshes fail into logs and index_status only — the user quietly keeps a stale graph without an obvious signal. Worth making noisy, e.g. a stale-index warning on the next MCP response.
  2. 18 permanent config keys is a very large one-way door for one feature. If this is adopted, collapsing to something like one index_resource_profile plus a few targeted overrides would age much better.

Security is clean: no new dependencies, no CI or workflow changes, no prompt-injection surface, and the two raw fopen calls are Linux-only /proc/<pid>/stat reads behind #elif defined(__linux__), which is fine — fixed ASCII procfs paths do not need cbm_fopen. New file writes use cbm_fopen correctly. The root-safety hardening (refusing /, $HOME and the cache dir as exact canonical roots while leaving descendants indexable) is a sensible touch.

Whether this project wants a resource-policy subsystem at all is the maintainer's call, not mine, so I have escalated it with the above attached rather than deciding in review. You will also need a rebase — the PR is CONFLICTING and touches mcp.c, pipeline.c and cli.c, all of which are moving.

Thank you for the care in this one; the parts that are good are good for the right reasons.

@DeusData DeusData added enhancement New feature or request stability/performance Server crashes, OOM, hangs, high CPU/memory labels Aug 3, 2026
@DeusData DeusData added this to the 0.9.2-rc milestone Aug 3, 2026
@DeusData DeusData added ux/behavior Display bugs, docs, adoption UX priority/normal Standard review queue; useful PR with ordinary maintainer urgency. labels Aug 3, 2026
@liuchong

liuchong commented Aug 5, 2026

Copy link
Copy Markdown
Author

Thanks for the thorough review. Agreed on the defaults and surface concerns. I'll hold off on reshaping and rebasing until the policy direction is clear. ❤️

@DeusData

Copy link
Copy Markdown
Owner

Thank you for waiting while we settled the product direction. The concept is worth pursuing, specifically as an opt-in safety and resource-control surface. We are not ready to change defaults in ways that could regress established large-repository workloads.

To make the work reviewable, please split it into atomic slices:

  1. Exact-root safety
  2. Discovery-side limits
  3. Worker watchdog limits
  4. Storage, cache, and free-disk admission

For the public configuration surface, we would prefer a compact profile with a small number of justified overrides rather than eighteen independent keys. Each slice should preserve current defaults, include deterministic tests, and explain the operational failure mode it controls. This approves further work in that opt-in shape, but it is not yet a merge commitment for a particular API. Thank you for the thoughtful trust-boundary analysis and for giving us room to establish a maintainable direction.

@liuchong

liuchong commented Aug 19, 2026

Copy link
Copy Markdown
Author

Thank you for the direction. I have rebuilt the work in the opt-in shape you asked for, as a stack of six small PRs rather than a reshaped version of this branch. Opening them fresh gives each slice its own review thread and a clean diff against current main; this branch has drifted far enough from what I am now proposing that reusing it would make review harder, not easier.

The stack

# PR Slice
1 #1723 Discovery-side limits
2 #1724 Worker watchdog limits
3 #1725 Storage, cache and free-disk admission
4 #1726 index_resource_profile
5 #1727 Attempt record and Git-backed freshness
6 #1728 Stale-index warning on the next MCP response

PRs 1–3 are your slices 2–4. PR 4 is the compact profile you asked for. PRs 5 and 6 answer point 1 of your first review — the watcher rebuild that fails into the logs and leaves the user on a stale graph with no obvious signal. PR 1 is ready for review; the rest are drafts until the one below it is settled, so you are never asked to review two moving diffs at once.

An honest note about the split. You asked for four slices and I am handing you six, plus six review threads instead of one. Splitting also has real costs here: because a fork PR has to target a branch in this repository, each PR's diff carries the commits under it until the one below merges, so the later diffs look larger than the work they add. And six threads means six CI runs and six places for the conversation to fragment.

If you would rather have one PR, say so and I will drop the stack and make the same changes on this branch instead — same content, same defaults, one thread. I split it because you asked for atomic slices and I think each of these is genuinely separately decidable, but the shape is your call, not mine, and I would rather ask than make you review six things you did not want.

Defaults

Everything defaults to off. With no configuration, none of the six PRs changes any behaviour, so the recorded large-repository benchmarks are unaffected. The balanced profile derives its worker memory ceiling from detected host memory rather than a fixed number, precisely because of the 16.4–33.6 GB figure you cited: any round ceiling low enough to look safe would reject an index that succeeds today.

Slice 1, exact-root safety, is not in the stack

src/foundation/workspace.c now classifies and refuses exact roots — home directory, credential directories, shallow top-level trees, Windows system trees — with the cache-containing-root case deliberately left indexable and documented as such. That is the same protection my branch was adding, done better and already on main, so re-landing it would only create a conflict.

Three capabilities from this branch are dropped on purpose

  • index_max_file_mb — an 8× tightening of the documented 512 MiB cap in src/foundation/limits.c. Files that index today would start being skipped, which is a breaking change and belongs in its own issue rather than riding along with an opt-in policy.
  • index_cpu_cores — contradicts the deliberate "use all cores for initial indexing — the user is waiting" decision in src/foundation/system_info.c. Capping parallelism from a config key fights the existing concurrency model instead of extending it.
  • index_low_priority — same conflict, and my original implementation had no test evidence that it changed anything measurable. I would rather drop it than ship an unverified knob.

The public surface is now index_resource_profile plus six individual keys, all off by default, down from eighteen. The remaining dimensions — directory count, entry count, depth, discovery deadline, and the three storage internals — exist only inside a profile, because they are meaningful as part of a composed decision and not as standalone operator settings.

I have not touched this branch or this PR. Close it in favour of the stack, leave it open as context, or tell me to bring the work back here and close the six — whichever costs you least.

@DeusData

Copy link
Copy Markdown
Owner

Disposition: close this one, keep the stack. Thank you for asking rather than guessing — and for rebuilding rather than reshaping, which was the right instinct.

The deciding facts are objective. This branch is CONFLICTING/DIRTY while all six of #1723#1728 are MERGEABLE/CLEAN. Twenty-seven of this PR's thirty-five files are touched again by the stack, and the core module is renamed rather than reused — index_limits.{c,h} here versus index_policy.{c,h} there — so the two cannot coexist in one tree. Reviving this branch would mean re-doing work you have already done in a shape I asked for.

More importantly, the thing I blocked on is fixed. Here, cbm_index_limits_defaults() is called unconditionally in both config load and pipeline construction, so all eighteen limits bind out of the box: a repo over 100,000 files or 4,096 MB of source is rejected, files over 64 MB are skipped (an 8× tightening of our own documented 512 MiB cap), and the worker is capped at four threads against a deliberate use-all-cores decision. You report the stack defaults everything to off. That is the difference between a safety surface and a defaults change, and it is the whole argument.

Your three deliberate drops are all correct, and I want to say so explicitly since dropping your own work is the least rewarding kind of revision: index_max_file_mb contradicted a documented cap, index_cpu_cores fought a decision made on measurements, and index_low_priority had no evidence it changed anything. Deriving the balanced profile's memory ceiling from detected host memory rather than a fixed number is also the right answer to the 16.4–33.6 GB reference workload — a fixed 8,192 MB was never going to fit both a laptop and that.

And your slice-1 supersession call is right — I verified it. src/foundation/workspace.c on main already provides cbm_workspace_classify_root(), refusing home directories, credential directories and top-level trees. Your exact-root check would now duplicate shipped behaviour. Good catch; that is exactly the check I have been asking everyone to make before reviving an old branch.

Six threads is fine — do not collapse them. You flagged the cost honestly and it is real, but six small reviewable diffs is the shape I asked for and cheap review beats cheap bookkeeping. I will take them in order.

Two things before I start.

  1. All six report draft: false, though you describe five of them as drafts pending the one below. If the later ones are not ready, please mark them draft so I do not review against a moving target — or tell me they are all ready and I will treat them that way.

  2. feat(index): record and report the latest index attempt #1727 and feat(mcp): warn on answers served from a failed rebuild #1728 look like scope beyond the four slices. I asked for exact-root safety (now dropped), discovery limits, worker watchdog limits, and storage/cache/free-disk admission — which I read as feat(index): add opt-in discovery resource limits #1723, feat(index): add worker resource watchdogs #1724 and feat(index): enforce opt-in storage resource limits #1725, with feat(index): add opt-in index resource profiles #1726 as the compact profile that replaces the eighteen keys. Recording the latest index attempt and warning when an answer is served from a failed rebuild are both plausibly good, but they are new surface rather than resource bounding. Are they load-bearing for the earlier slices, or independent improvements that arrived alongside? If independent, I would rather judge them on their own merits than inherit them.

For the record, the parts of this branch I praised in the first review are the parts I expect to see again in the stack: stripping the caller-supplied _cbm_index_limits and replacing it from server config before the worker spawns, the worker refusing an incomplete policy, and discovery aborting a request rather than publishing a truncated file list. That trust boundary was the strongest thing here, and it should survive the reshaping intact.

Closing this in favour of the stack. Nothing about the work is being discarded — only this branch.

@DeusData DeusData closed this Aug 20, 2026
@liuchong

Copy link
Copy Markdown
Author

Both answers, and one thing that changed since you wrote.

All six are ready — please treat them that way. My earlier note was about review order, not readiness, and I phrased it badly. Each slice builds, passes its own tests, and is independently revertible. Marking the later ones draft would imply work they do not need. If the stack has to move under review I will rebase and say so, rather than let it drift under you.

#1727 and #1728 are independent — judge them on their own merits. They are not load-bearing: #1723#1726 stand without them, and closing both leaves nothing dangling.

What they answer is the concern from your own #1723 review — that the auto-index path fails closed "where nobody is watching the log". #1727 records the dropped attempt; #1728 stops a stale graph from answering as though it were current. That makes them the visibility half of a fail-closed design rather than more resource bounding, which matches your reading of them as new surface. If you would rather bound the batch at #1726, close them and I will re-propose each with its own justification.

What changed since you wrote. All six are now rebased onto current main and green. They had been 41 commits behind — far enough to predate #1772, so they were re-hitting Windows flake you had already fixed.

That rebase also isolated a separate defect, which I opened as #1796 rather than fold in here. daemon start exits nonzero when the daemon is up but the UI-configuration handshake misses its one-second window; by that point the daemon is running, so the command reports failure for something it achieved. Same Windows timing #1772 addressed, one bounded wait further downstream — and it is what kept reddening test-windows-guards on this stack after the rebase.

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

Labels

enhancement New feature or request priority/normal Standard review queue; useful PR with ordinary maintainer urgency. stability/performance Server crashes, OOM, hangs, high CPU/memory ux/behavior Display bugs, docs, adoption UX

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants