Cap the file-history walk, share the lock, and let it be cancelled - #478
Merged
Merged
Conversation
"Can this be cancelled" and "what cancels it" were two answers to one question — a set of cancellable kinds in `repoActivity`, and a hardcoded `cancelNetworkOps()` at each of the two surfaces offering the button. That is free to disagree, and the disagreement's shape is a Cancel that runs and stops nothing. They are one table now (`cancelPath`), and one store action picks the mechanism for both surfaces (`cancelActivity`). `isCancellable` is derived from the table rather than sitting beside it. **Why:** #474 adds a cancellable operation that is NOT a subprocess — a libgit2 revwalk, stopped by setting a flag it polls rather than by signalling a process group. Without this, the command palette would have offered "Cancel network operation" for a history search and called the network path, which reaches nothing. The palette gets its own row for it, with one label rather than two: a walk polls a flag, so asking twice does what asking once did, and borrowing the network row's "Force stop" would promise an escalation that has no counterpart here. Also resets `cancelRequested` in the palette test's `setRepo`. It was left standing between tests, which is how a test that never clicked anything read "Force stop network operation". Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Closes #474. `file_history` walked from HEAD keeping commits whose tree differs from their parent's at the path, and stopped at `limit` MATCHES. A file with fewer changes than the limit had nothing to stop on, so the walk ran to the root of history with a tree comparison at every commit. Measured on torvalds/linux, warm, for one click: 135.6 s and 1,482,923 tree comparisons. That is most files in most large repositories, not a corner case. Three things, matching the issue: * **A visit cap beside the match cap**, defaulting to 50,000 commits, and the result now says which ceiling ended the walk (`HistoryStop`) plus how many commits it examined. The screen says it out loud — "Searched the newest 50,000 commits" — and offers "Search all of history", which is the old unbounded walk asked for deliberately. A cap nobody mentions is the silent wrong answer this area exists to avoid, and the number shown comes off the wire so it cannot drift from the policy applied. * **`with_repo_read`.** This is the longest read in the backend and it held the EXCLUSIVE lock for all of it, so one click queued every other operation on the repository behind a walk that could take minutes — the exact failure #400 set out to remove, surviving in the op least able to afford it. It writes nothing: a revwalk, `find_commit`, `Tree::get_path`, and a tree-to-tree diff in the pathspec case. Notably it never reads the index, so there is no accidental refresh for anything else to depend on. * **Cancellable**, because 18 s is still a wait worth being able to stop. A new `cancel::Scope::Walk` — the first scope with nothing to signal, so the command hands the backend a predicate to poll between commits and the backend knows nothing about cancellation. The screen also cancels on the way out, so leaving does not leave a walk running. **Why the sort order is untouched:** dropping `TOPOLOGICAL` looks like the obvious fix and buys nothing. libgit2's sorted revwalk pre-walks the whole graph before yielding anything — 14.2 s to the first oid, 14.7 s for all 1.48 million, measured within 1% for TIME and TIME|TOPOLOGICAL alike. `Sort::NONE` IS incremental and is unusable here: it yields commits in traversal order, so the first 50,000 are not the newest 50,000, and a capped walk could miss last week's change while reporting one from 2011. The cap has to mean "the newest N commits" or it cannot be said out loud. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The component tests drive `file_history` through a mocked `invoke`, so none of them can see the wire: the command answers with a struct now rather than an array, takes a new `searchAll` argument, and `cancel_walk` is a new command whose rejection the frontend deliberately swallows (a walk that finished first is the ordinary case). A serde rename arriving as `stopped_at`, or an argument Tauri would not convert, breaks none of the 4,178 mocked tests and every real click — the same shape of break as the argument-name break in #435 that only a real-IPC case caught. Two of the four cases therefore ask the commands directly: nothing on screen distinguishes a readable `stoppedAt` from an unreadable one, since both render the same list with no notice. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`performance.md` said file history was "unbounded on a cold path", which is no longer true, and said it without a number because "unbounded is not a number". There are numbers now, taken on the `linux` fixture: 135.6 s uncapped against 18.3 s capped, of which 14.5 s is libgit2's own revwalk preparation and 4.1 s is 50,000 commits of tree comparison. The second bullet is the more generally useful finding, and it rules out the cheap fix the way the commit-graph measurement next to it already does: a sorted libgit2 revwalk pre-walks the whole graph before it yields anything, and the sort ORDER is not why — TIME and TIME|TOPOLOGICAL were within 1% of each other at every size measured. That floor is very probably the one behind `log_page`'s first page too, which is worth knowing before anyone tries to optimise the log by changing its sort. Also: `file_history` joins the shared-read list in `backend.md` with its proof and the rendezvous test that pins it, `cancel.rs`'s section grows the second KIND of cancellable work, `architecture.md` gains the two commands (`test/docs.test.ts` requires it), `frontend.md` gains the notice and the cancel-ordering trap, and `repo_bench.rs`'s `hottest_path` no longer explains itself with a bug that is fixed — it has two reasons that outlived it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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 #474.
file_historywalks from HEAD keeping commits whose tree differs from theirparent's at the path, and stopped at
limitmatches. A file with fewerchanges than the limit has nothing to stop on, so the walk ran to the root of
history with a tree comparison at every commit — and it held the exclusive
repository lock for all of it.
Measured on the
linuxbenchmark fixture (warm, one click on a path with fewerthan 200 changes):
What changed
A visit cap beside the match cap. The result now carries
visitedandstoppedAt(Exhausted/MatchLimit/VisitLimit), because the threeoutcomes look identical on screen — a list that ends — and mean completely
different things. The screen says which one happened, with the number that was
actually searched, and offers Search all of history: the old unbounded walk,
asked for deliberately, the way the blob ceiling's "diff it anyway" works.
with_repo_read. This is the longest read in the backend, and holding theexclusive lock for it queued every other operation on the repository behind a
walk that could take minutes — the failure #400 set out to remove, surviving in
the op least able to afford it. The proof it writes nothing is in
docs/dev/backend.md; notably it never reads the index, so there is noaccidental refresh for anything else to have come to depend on (the
statusregression next door is the cautionary tale).
Cancellable, because 18 s is still a wait worth stopping. A new
cancel::Scope::Walk— the first scope with nothing to signal, so the commandhands the backend a predicate to poll between commits and the backend knows
nothing about cancellation. The screen also cancels on the way out, so leaving
does not leave a walk running.
A bug this surfaced on the way: the command palette's Cancel row was gated
on "is this cancellable" but hardcoded to the network mechanism, so it would
have offered a Cancel for a history search and reached nothing. "Can it be
cancelled" and "what cancels it" are one table now, with one store action both
surfaces call.
The optimisation this deliberately does not make
Dropping
Sort::TOPOLOGICALlooks like the fix and buys nothing. libgit2'ssorted revwalk pre-walks the whole graph before yielding anything — 14.2 s to
the first oid, 14.7 s for all 1.48 million, measured within 1% for
TIMEand
TIME | TOPOLOGICALalike.Sort::NONEis incremental and unusable here:it yields commits in traversal order, so the first 50,000 are not the newest
50,000 and a capped walk could miss last week's change while reporting one from
2011. The cap has to mean "the newest N commits" or it cannot be said out loud.
That fixed floor is very probably the one behind
log_page's first page too,which is worth knowing before anyone optimises the log by changing its sort. It
is written up in
docs/dev/performance.mdbeside the commit-graph measurementthat rules out the other cheap fix.
Verification
All on the final tree.
cargo test— 96 test binaries, no failures. The visit cap, the two ceilingsreported separately, the exactly-on-the-boundary case that would otherwise
make the notice lie about every ordinary repository, cancellation (including
that it stops rather than running on and discarding the answer), and a
rendezvous test that the walk does not exclude another read — the
predicate is called with the lock held, so it is a place to stand still and
check whether a concurrent
statuscan get in. Verified by plantingwith_repoback: that test fails on its bounded wait and only that one.pnpm test— 407 files, 4,178 tests, all passing.pnpm tsc --noEmitandpnpm exec tsc -p e2e/tsconfig.json --noEmitclean.pnpm test:e2e:docker— snapshot rebuilt, thenfile-history(4 passing),cancel(2) andpalette(8), 0 stalled driver scripts. Two of thefile-history cases ask the commands directly: nothing on screen distinguishes
a readable
stoppedAtfrom an unreadablestopped_at, and the frontenddeliberately swallows a
cancel_walkrejection, so both would fail silently.chain fails the ordering test, and the notice tests fail on a hardcoded cap.
🤖 Generated with Claude Code