feat(index): add opt-in index resource profiles - #1726
Conversation
|
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. What that means for this PR, concretely:
Things that will genuinely speed it up whenever review does happen:
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. |
b323e24 to
c2ede7b
Compare
8a3b8d4 to
7e8b465
Compare
|
Reviewed slice 4. You asked for the host-share derivation to be challenged, so I went at that first — and it survives. The finding I do have is somewhere else. The derivation is right, and our own numbers back it
That is not a hypothesis here, it is measured. A recorded large-repository run in this project peaks around 16.5 GB resident, and a doubled workload OOMed a 36 GB host. Any tabled ceiling comfortable enough to look safe — 4 GB, 8 GB — would refuse an index that works today, and the operator's reward for enabling a safety feature would be a broken setup. The host share is the honest answer, and 80% is a defensible place to put it. The arithmetic is careful too: Withholding individual keys for directory count, entry count, traversal depth and discovery time is the same restraint as #1725's storage internals, and right for the same reason. The finding: the description promises something the code deliberately does not do
That holds for every dimension except one. The final clamp sits outside the override guard: if (policy->max_rss_bytes.enabled && host_cap > 0 &&
policy->max_rss_bytes.value > host_cap) {
policy->max_rss_bytes.value = host_cap;
}So an operator who explicitly sets I want to be clear I do not think this is a bug — Two small things would settle it: Say so in the description. "A profile is a baseline, not a lock" is the sentence an operator will read and remember, and for And say so at runtime when it bites. Right now the clamp is silent. This repo has just spent four pull requests removing exactly this shape — a value quietly replaced by a different one, with nothing telling the caller to look. A log line naming the requested value and the applied one, emitted only when the clamp actually tightens something, would cost almost nothing and keeps this consistent with that work. Also worth considering: the clamp applies whenever StatusClearance is the same inherited set as the slices below; I will handle the marker before merge. Rebase when #1725 lands and this collapses to |
DeusData
added
enhancement
…pelling the policy root The first cut on this branch (c1b9c45) respelled the session root and the allowed root with forward slashes on the way into the session policy (cbm_mcp_server_set_session_context) and at the three application.c sites that canonicalize a root. CI's Windows shard then failed two tests: tests/test_daemon_application.c:2256: ASSERT(sensitive_blocked) tests/test_daemon_application.c:2380: ASSERT(sensitive_blocked) Both set HOME to the cbm_canonical_path spelling of a directory - the platform's native form, backslashes on Windows - and expect a session rooted there to be refused for auto-index and for watch. The sensitive-root and allowed-root containment walk (ws_is_ancestor_or_equal in src/foundation/workspace.c) compares prefixes byte-exact, so a root that now read C:/Users/... no longer matched a HOME of C:\Users\...: the sensitive refusal was lost and $HOME was admitted. Respelling the policy root was the wrong layer. This commit keeps the policy roots in their canonical native spelling (mcp.c and the three application.c sites are back to what main runs) and folds the separator spelling at the one comparison the original defect lives in: application_index_args_equal, where the running job's args are compared with an explicit index_repository request's. The auto-index job spells repo_path the way the session policy holds it; the handler spells it with forward slashes. On Windows the two never matched, and the request was refused as OPTIONS_CONFLICT instead of joining the job already running for its root. The fold acts on the parsed copies only (yyjson_mut_obj_replace with a cbm_normalize_path_sep'd string), so nothing the daemon stores changes spelling and every other option stays exact. Why this is correct on Windows without a local Windows run: the policy spelling is untouched, so the containment path is byte-for-byte what main runs there today; cbm_normalize_path_sep folds backslashes on every platform, so the new fold is exercised by the new test wherever the suite runs. Tests: daemon_session_context_keeps_one_spelling_of_a_root asserted that the policy respells a root with forward slashes. That was the wrong contract: the policy's containment checks compare its spelling byte-exact against HOME and the granted roots, so a respelled root is exactly what stops matching them. It is replaced by daemon_session_context_keeps_the_policy_spelling_of_a_root, which binds the opposite: the policy stores the root as given. The comparison is exposed through application_internal.h as cbm_daemon_application_index_args_equal_for_test and bound by daemon_application_index_args_compare_repo_path_separator_equivalently: a backslash and a forward-slash repo_path compare equal; a different path, a sub-path, and a different mode still do not. Refs #1726 Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
On Windows cbm_canonical_path answers in the platform's native backslash form, and the daemon stored that spelling verbatim for the client's session_root and allowed_root (cbm_mcp_server_set_session_context) and for the auto-index job's repo_path (application.c canonicalizes with the bare call at three sites). Every tool handler, however, normalizes separators after canonicalizing. One directory therefore had two names, and the exact comparison in application_index_args_equal never matched: an explicit index_repository request for the session root was refused as OPTIONS_CONFLICT instead of joining the auto-index job already running for it. Normalize the separator spelling of both session roots in set_session_context, and canonicalize plus normalize at the three application.c sites through one helper, application_canonical_root(). Separators fold on every platform, so the new test daemon_session_context_keeps_one_spelling_of_a_root binds wherever the suite runs; it fails on main with "C:\repos\cbm" != "C:/repos/cbm". Distilled from DeusData#1726 with co-author credit. Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com> Co-authored-by: 刘冲 <mail@liuchong.dev>
Indexing accepts whatever a repository contains. A tree carrying a vendored monorepo, a generated dump, or a runaway build directory is discovered in full, and the first sign of trouble is a host under memory pressure with nothing that attributes it to indexing. Add two opt-in limits evaluated during discovery against accepted source files only: index_max_files and index_max_source_mb. Both default to off, so nothing changes until an operator sets one. Crossing a limit fails the whole attempt with a structured resource_limit_exceeded result naming the resource, the observed value and the limit; no partial graph is published, and an existing serving index keeps answering. Limits are read from the CLI-managed _config.db and are not MCP request arguments. A supervised parent replaces any caller-supplied policy before spawning its worker, and the worker rejects a missing or incomplete contract, so the CLI, the daemon and the supervised worker all enforce the same decision. Both keys reach an operator through the existing config get/set/list/reset with no new subcommand. `set` suppresses its own generic message for them because the policy writer names the precise reason -- so that writer speaks on every failure it can return, including a validated value whose write then fails on a database that cannot be written. Exiting non-zero in silence is not an acceptable answer from a CLI. The two shell regressions that hand-roll the supervisor's worker argv carry that contract as well. Without it the worker exits before either guard can observe anything, and the guard would go quietly vacuous. Signed-off-by: 刘冲 <mail@liuchong.dev>
Discovery limits bound what indexing accepts, not what it then costs. A repository well inside those bounds can still exhaust the host through parser memory, or simply never finish, and a supervised worker that hangs leaves the parent waiting with nothing to report. Add index_max_rss_mb and index_max_duration_seconds, enforced by the parent against the worker process tree rather than the worker process alone, so a runaway child cannot hide behind a small parent. Resident memory is sampled through the platform interface on macOS, Linux and Windows. Crossing a limit terminates the tree and yields one trusted, structured terminal result that attributes the failure to the resource that caused it. Both limits default to off. A measurement that cannot be taken fails the attempt instead of passing it: a watchdog that quietly stops watching is worse than no watchdog at all. The shell fixture that stands in for the supervisor names the two new keys. The worker accepts only a policy that spells out every key it knows, which is what keeps a stale supervisor from starting a worker it cannot bound. Signed-off-by: 刘冲 <mail@liuchong.dev>
fa11ee4 to
f870ce4
Compare
An index that fits in memory and finishes in time can still fill the disk. Publication needs room for the staging artifacts and the final database at the same time, and running out of space during publication is the one failure that can cost a working index. Add index_cache_max_mb and index_min_free_disk_mb, measured before staging and again before publication, together with internal ceilings on the final database, the staging artifacts and the task temporary directory. Those three have no public keys because they are only meaningful as part of one composed decision. Crossing any of them fails the attempt before the old database is touched, so atomic publication is unchanged and the previous index keeps serving. Staging cleanup is scoped by a private per-task token, so a worker removes only the artifacts it created and never a concurrent run's. An old database is treated as replaceable only after an integrity verdict distinguishes real corruption from a transient busy error. A probe that cannot complete fails closed. Both public keys default to off. The shell fixture that stands in for the supervisor names them too, so the worker still recognises the policy it is handed. Signed-off-by: 刘冲 <mail@liuchong.dev>
Six independent keys are an honest interface and a poor default. An operator who only wants indexing to stay within reason has to learn all six, choose a value for each, and keep them consistent as the machine changes. Add index_resource_profile, accepting off, balanced or strict. Balanced guards the host: it lets an index use what the machine can spare and stops a runaway worker before the host is exhausted. Strict holds indexing to the daemon's own budget, so a repository that needs more fails fast and attributed instead of finishing at the host's expense. Any individual key, including an explicit off, replaces that dimension of the selected profile, so a profile is a baseline rather than a lock. The balanced worker ceiling is derived from detected host memory instead of being tabled. Large-repository indexing peaks in the tens of gigabytes, so any round number low enough to feel safe would reject repositories that index successfully today. When host memory cannot be read, balanced falls back to a fixed floor. Profiles also bound directory count, entry count, traversal depth and discovery time. These have no individual keys for the same reason as the storage internals: they are only meaningful inside a composed decision. The resolved profile travels through the same trusted worker contract as the individual limits, so every entry point enforces the same decision. That contract now also carries the resolved profile and the override mask, so the shell fixture standing in for the supervisor sends both. The coverage that proves a forged policy cannot fork a second job asserts that an explicit request joins the auto-index job already running for the same root, and that assertion found the join broken on Windows. A project root reaches the daemon through the platform canonicalizer, which answers in backslash form there, while every tool handler normalizes the separators of the path it canonicalizes. One directory therefore had two spellings, and they met in comparisons that are exact: whether an index request may join the running job, and whether a watch is still live for that root. Both were wrong on Windows, in opposite directions, so aligning either one alone moved the failure rather than removing it. The session context now stores the normalized spelling, and the daemon canonicalizes project roots through a single function that normalizes on the way out. Only the storage side is directly assertable off Windows, since separators are folded on every platform; the watch-liveness side is held by the existing watcher-ownership tests, which is where the second half of this surfaced. Signed-off-by: 刘冲 <mail@liuchong.dev>
f870ce4 to
c263f48
Compare
2 participants
Related to #1347.
Problem
The previous three PRs add six independent keys. That is an honest interface and a poor default: an operator who only wants indexing to stay within reason has to learn all six, choose a value for each, and keep them consistent as the machine changes.
What this changes
One key,
index_resource_profile, acceptingoff(default),balancedorstrict.balanced, raise the individual key, or leave the profileoff.Any individual key, including an explicit
off, replaces that dimension of the selected profile. A profile is a baseline, not a lock.The balanced worker memory ceiling is derived from detected host memory rather than tabled. This is the decision most worth challenging in review. Large-repository indexing peaks in the tens of gigabytes, so any round number low enough to feel safe would reject repositories that index successfully today — a "safety" feature whose main effect is breaking working setups. The host share is the only balanced answer that scales with the machine. When host memory cannot be read, balanced falls back to a fixed floor.
Profiles also bound directory count, entry count, traversal depth and discovery time. These have no individual keys for the same reason as the storage internals in #1725: they are only meaningful inside a composed decision.
The resolved profile travels through the same trusted worker contract as the individual limits, so the CLI, the daemon and the supervised worker enforce the same decision.
Testing
make -f Makefile.cbm testandmake -f Makefile.cbm lint-cion macOS. New coverage: exact profile baselines forbalancedandstrict, the host-share derivation including the unreadable-host fallback, per-key override precedence including explicitoff, the four discovery dimensions with their rejection paths, and profile propagation through the worker contract.Stack