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
c2ede7b to
8a3b8d4
Compare
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>
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>
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
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