feat(index): enforce opt-in storage resource limits - #1725
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. |
bd6ea29 to
f14844b
Compare
f14844b to
cf862a0
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>
cf862a0 to
c5d165b
Compare
|
Reviewed slice 3. I said on #1723 that this is where the configuration surface starts growing and that these slices would get a closer look. It holds up, and the restraint is the first thing worth naming. You added two public keys and deliberately withheld three. The final-database, staging-artifact and task-temp ceilings have no operator-facing keys because, as you put it, they are "only meaningful as part of one composed decision about how much room a publication may take, which no single operator-facing key can express." That is the correct instinct — three more knobs would have been easier to write and much worse to live with, because each one becomes a permanent contract the moment someone sets it. Both details you flagged for attention check out. I verified them rather than taking them, because both are destructive. The busy-versus-corrupt verdict. The comment on the unopenable case is right too — keeping its bytes in the projection rather than blocking repair of a confirmed-corrupt destination is the conservative direction. The task-token cleanup. The prefix is Measuring at both checkpoints — before staging and again before publication — is correct for the stated reason: the interesting moment is the one where both copies exist. And failing before the old database is touched, with atomic publication unchanged, means the worst case is "no new index" rather than "no index". StatusYour only red is not yours. Clearance is Rebase when #1724 lands and this collapses to |
DeusData
added
enhancement
2 participants
Related to #1347.
Problem
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.
What this changes
Two opt-in limits, both defaulting to
off:index_cache_max_mbindex_min_free_disk_mbThey are measured before staging and again before publication, because the interesting moment is the one where both copies exist.
Three internal ceilings — final database, staging artifacts, task temporary directory — are added alongside them. They deliberately have no public keys: they are only meaningful as part of one composed decision about how much room a publication may take, which no single operator-facing key can express.
Crossing any of them fails the attempt before the old database is touched. Atomic publication is unchanged and the previous index keeps serving.
Two details worth review attention:
SQLITE_BUSY. Treating busy as corrupt would discard a healthy index.Probes fail closed: an enabled measurement that cannot be completed fails the attempt rather than being assumed to pass.
Testing
make -f Makefile.cbm testandmake -f Makefile.cbm lint-cion macOS. New coverage: directory-size and free-space measurement on each platform path, admission at both checkpoints, task-token-scoped cleanup leaving unrelated cache files intact, preservation of the old index on rejection, and the busy-versus-corrupt integrity verdict.Stack