Bound the cold path a before-change query has to walk - #526
Merged
Conversation
Two costs sat on the path that runs before every edit an agent makes, and both were unbounded from the caller's side. The first was a full rebuild. A consumer query called `ensureIndex`, so a missing or invalid derived file meant reading the entire history before one path could be answered: 186 seconds on a 21,446-commit repository. A caller with a shorter timeout kills that, and the next edit starts cold again, so the index may never become warm and the product is quietly useless on exactly the repositories it is meant for. The second was duplication. Each `fetch` on the scan source re-ran `scanTrailers`, and one single-alias path query calls it twice -- once for the repository-wide lifecycle fold, once for the path -- so the corpus was parsed at least twice. The fold genuinely needs the whole history (SPEC §5), so the corpus is now materialized once per query and each predicate applied to it in memory. `--limit` still bounds nothing about that work, and cannot: the limit applies after folding, grouping and ordering, so a bound there would change which records the answer contains. A consumer query may now catch the index up but not rebuild it. The distinction is the whole point. An incremental update reads `last_indexed_sha..HEAD` -- the commits made since the last query, which on a repository being worked in is a handful, and which is always a subset of the history the fallback would read. Refusing that too would have been the same defect wearing different clothes: an index one commit behind would be unusable, and every query after every commit would read the whole corpus, worse in steady state than what this set out to fix. Opening the index also creates the file, so a query with no index at all refuses before opening rather than leaving an empty database behind. Answering a read must not change the repository. Limit: a genuinely cold fallback still reads the whole history once, because repository-wide lifecycle folding cannot be scoped to a path without changing what the answer means Limit: nothing outside `index` and `init` builds the index now, so a repository whose derived file was deleted stays on the scan path until one of them is run Ruled-out: applying `--limit` before the lifecycle fold | it would bound the cold work and silently change which records survive supersession Ruled-out: serving a stale index when catching it up is not possible | a fast wrong answer is worse here than a slow right one; the fallback stays fail-closed and git remains the authority Warn: a query on a repository with no index now says so on stderr every time, including from the injection hook; that is a true report of a real slowness, but it is per-invocation noise until the index is built Blast: module Undo: easy Certainty: firm Verified: a 1,024-commit fixture proves the cold fallback makes exactly one corpus pass and leaves no index file behind, and that an index behind by one commit is caught up rather than abandoned -- both structurally, with no wall-clock assertion; indexed and fallback answers remain equal across the existing correctness matrix; 393 cases pass across the query, index, concurrency, classifier-staleness, inject, doctor and stale suites; typecheck clean and two builds produce a byte-identical dist Provenance: authored Record-Id: r-coldpath1
CommitLore — record lintTrailers: clean — 2 commits in Active constraints for the paths this PR touchesLimits (129)
Ruled out (293)
Truncated: 203 lines omitted — the comment hit GitHub's 65000 character limit. Trailer violations fail this check. Active constraints are informational — they are what the repository already decided, not a verdict on this PR. |
A query on a repository with no index now reports that it answered by full scan, and this suite asserts an exact diagnostics array. The extra line is true and about something else, so the fixture builds the index the way an install does and the case goes back to measuring what it names. Blast: local Undo: easy Certainty: firm Verified: twelve rename cases pass Provenance: authored Record-Id: r-coldpath2
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
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.
Closes #522 once this reaches
main.Two costs sat on the path that runs before every edit an agent makes, and both were unbounded from the caller's side.
The rebuild
A consumer query called
ensureIndex, so a missing or invalid derived file meant reading the entire history before one path could be answered — 186 seconds on the 21,446-commit repository in the report. A caller with a shorter timeout kills that, and the next edit starts cold again, so the index may never become warm. Since ADR-0030 the product installs unattended and runs on every edit, which makes this the difference between working and quietly not working on large repositories.The duplication
Each
fetchon the scan source re-ranscanTrailers, and one single-alias path query calls it twice — once for the repository-wide lifecycle fold, once for the path — so the corpus was parsed at least twice. The fold genuinely needs the whole history (SPEC §5), so the corpus is materialized once per query and each predicate applied in memory.--limitstill bounds nothing about that work, and cannot: it applies after folding, grouping and ordering, so a bound there would change which records the answer contains.Catch up, but do not rebuild
This is the part worth reading closely. A consumer query may now bring the index forward but never rebuild it.
An incremental update reads
last_indexed_sha..HEAD— the commits made since the last query, a handful on a repository being worked in, and always a subset of the history the fallback would read. Refusing that too would be the same defect wearing different clothes: an index one commit behind would be unusable, and every query after every commit would read the whole corpus. That is worse in steady state than the defect being fixed, so there is a test pinning it.Opening the index also creates the file, so a query with no index refuses before opening rather than leaving an empty database behind. Answering a read must not change the repository.
The fixture
A 1,024-commit synthetic history proves both properties structurally — one corpus pass, no index file created, and an index one commit behind caught up rather than abandoned. No wall-clock assertion: a timing test on CI is a flake that gets deleted six weeks later and takes the guarantee with it.
Stated plainly
Limit:a genuinely cold fallback still reads the whole history once. Repository-wide lifecycle folding cannot be scoped to a path without changing what the answer means.Limit:nothing outsideindexandinitbuilds the index now, so a repository whose derived file was deleted stays on the scan path until one of them runs.Warn:a query on a repository with no index says so on stderr every time, including from the injection hook. That is a true report of real slowness, but it is per-invocation noise until the index is built.393 cases pass across the query, index, concurrency, classifier-staleness, inject, doctor and stale suites; indexed and fallback answers remain equal across the existing correctness matrix; typecheck clean; two builds produce a byte-identical
dist.