feat(mcp): warn on answers served from a failed rebuild - #1728
Open
liuchong wants to merge 6 commits into
Open
Conversation
This was referenced Aug 19, 2026
liuchong
force-pushed
the
feat/index-stale-warning
branch
from
August 19, 2026 07:40
9bae837 to
3d51b65
Compare
|
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. |
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. 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>
liuchong
force-pushed
the
feat/index-stale-warning
branch
2 times, most recently
from
August 19, 2026 18:37
f22b6d0 to
c47efae
Compare
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>
A failed index is currently a log line. A watcher-triggered rebuild that dies on a resource limit leaves the previously published graph in service and nothing durable behind it, so index_status reports a healthy generation and the operator has no way to learn that it is no longer the tree. Record one attempt per project under the cache directory, updated under a file lock across queued, running and terminal states, carrying the origin, timestamps, the effective profile source and the resource failure when there is one. index_status returns it as last_index_attempt, and delete_project removes a matching record when no database was published. Freshness is reported alongside it and is deliberately conservative. A generation is comparable only when matching clean Git snapshots were observed before and after indexing, and it is fresh only while the worktree is still clean at that HEAD. A different clean HEAD is stale. Non-Git roots, dirty trees, a tree that changed during indexing, a failed probe and a corrupt record are all unknown, because a dirty tree is evidence of neither freshness nor staleness. Daemon startup fails attempts whose worker no longer exists. Liveness is judged by process identity rather than the pid alone, so a recycled pid cannot keep a dead attempt alive, and an unavailable start time falls back to plain liveness rather than declaring the worker gone. Projects with no record keep the previous response shape. Signed-off-by: 刘冲 <mail@liuchong.dev>
A recorded attempt is only visible to whoever thinks to ask for it. An agent that calls search_graph after a background rebuild failed gets a confident answer from a graph that no longer matches the tree, and nothing in that response says so. While the recorded attempt is failed or cancelled, attach a warning to the answers served from that project's graph, naming the origin, the recorded finish time and the limit that ended the attempt. A JSON payload gains a stale_index_warning field so a structured reader cannot miss it; text and tree payloads gain a trailing content block. The answer itself keeps its position and its content. index_status and check_index_coverage already report freshness themselves and index_repository is the remedy, so none of them is annotated. A queued or running rebuild is not yet a failure and stays silent, a completed rebuild clears the warning, and every failure inside the warning path is silent: a warning that cannot be produced must never damage an answer that is fine. Signed-off-by: 刘冲 <mail@liuchong.dev>
liuchong
force-pushed
the
feat/index-stale-warning
branch
from
August 19, 2026 20:18
c47efae to
b810789
Compare
liuchong
marked this pull request as ready for review
August 20, 2026 03:38
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This implements the review suggestion on #1348 to surface a stale index on the next MCP response rather than only in
index_status.Related to #1347.
Problem
A recorded attempt is only visible to whoever thinks to ask for it. An agent that calls
search_graphafter a background rebuild failed receives a confident answer from a graph that no longer matches the tree, and nothing in that response says so. Agents do not pollindex_statusbefore every query.What this changes
While the recorded attempt is
failedorcancelled, answers served from that project's graph carry a warning naming the origin, the recorded finish time, and the limit that ended the attempt — enough for the caller to decide whether to trust the answer and what to fix.stale_index_warningfield, so a structured reader cannot miss itcontent[0]is never displaced, becausehook_augmentand the smoke tests read it as the payloadAnnotated:
search_graph,query_graph,trace_path,trace_call_path,get_architecture,get_code_snippet,search_code. Not annotated:index_statusandcheck_index_coveragealready report freshness themselves, andindex_repositoryis the remedy.A queued or running rebuild is not yet a failure and stays silent; a completed rebuild clears the warning. Every failure inside the warning path is silent — a warning that cannot be produced must never damage an answer that is otherwise fine.
Cost is one small record read per answered query. It probes neither Git nor the working tree.
Testing
make -f Makefile.cbm testandmake -f Makefile.cbm lint-cion macOS. New coverage: the warning on both JSON and text payloads with the answer intact, the attributed cause text, silence forindex_status, silence while an attempt is queued or running, and clearing after a completed rebuild.Stack