Repository navigation
fix(engine): bound idle git work by checkout, not by subscriber - #27
Merged
Merged
Conversation
PR-status watches re-ran the ~8-process checkout inspection once per subscription every 15 s, so cost grew with chats x clients (a paired phone watches up to 40 targets regardless of the desktop's hidden-badge setting). Workspace-file authorization ran the full refs() (~5 processes) on every request. - Share one single-flight inspection per checkout and skip it while a stamp of HEAD/config/packed-refs is unchanged (5 min max age). The locked section runs detached so an unpolled subscriber cannot strand the lock. - Authorize checkouts from one cached `git worktree list`, revalidated against the mtime of .git/worktrees (10 s TTL otherwise). - Desktop: watch side chats only while they or their parent are selected; reconcile on selection.
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.
Problem
While idle, the engine spawned ~20–60
git.exeprocesses per second (measured on v0.10.1: 190–590 new children per 10 s, 1,118 in one 30 s window). This starved the engine: lock-free RPCs took up to 918 ms,list_chats6.7 s, and a rename took up to 9 s to reach the chat list.Root causes
WatchCheckoutChangeRequeststream re-raninspect_checkout(~8 git processes:rev-parse --show-toplevel,branch --show-current,@{upstream},branch.<x>.remote,remote.pushDefault,remote get-url --push,symbolic-ref --quiet …/HEAD, …) every 15 s, even though it already shared theghlookup. Cost therefore grew with the number of subscribers (chats × clients) rather than the number of checkouts, and orchestrators keep adding side chats.refs()on every request.Repos::workspace_checkoutranbranch -a+symbolic-ref+branch --show-current+worktree list(~5 processes) only to check that a folder is the repo or one of its worktrees. It runs on every workspace-file request (list/read/search/write/git-status, mention search, command catalogs), on everychange_request_rootspace scan, and on project actions.The open puzzle: why did watches run with
sidebarShowPullRequest: false?The desktop wasn't the subscriber; the paired Android phone was.
Shell::newapplies it synchronously in the same app-init closure that startsbootstrap, soreconcile_change_request_watchesnever runs with thetruedefault once the registry attaches. Dropping a watch task drops itsRpcSubscription, which sends a cancel frame, and the server aborts the stream task (rpc/src/server.rs). The new UI test runs that path (setting off before the registry attaches, chats arriving, selection changes) and holds zero watch tasks. It passes on both the old and new code, which rules the desktop out.GetRemoteAccesslisted a paired session "S23 Ultra", last seen 06:32 local, inside the measurement window.Get-NetTCPConnectionshowed 5 established connections fromroboco-tailcat.exeon the remote-access listener (the Android path), next to 4roboco mcpclients.crates/mobile-client/src/workspace/change_requests.rs) subscribesWatchCheckoutChangeRequestfor up to 40 (repo root, branch) targets per engine: every non-archived top-level chat, idle ones included, sorted by recency. It never sees the desktop's sidebar setting, which is per-viewport UI state; the engine doesn't know about it. That matches the signature: inspections for idle chats' branches such asimplement-core-remainingandfeat/orchestration-v2.So the fix has to live in the engine. Engine cost must be bounded by checkouts, not by subscriber count or client type. That is what (1) below does.
After the app restarted at 07:12 (not by me), a fresh 30 s sample showed 840 git children, dominated by the
refs()signature (~130 sets) with only ~7 PR inspections. That points at (2), workspace-file authorization, which the cache below removes.Changes
Engine:
change_requests.rsCheckoutInspectionper checkout id, shared by every subscription on that checkout whatever branch it asks about (weak map; subscriptions hold the strong refs). It runs single-flight, at most once per poll interval.HEADand the commonconfig/packed-refsis unchanged, up to a 5-minute max age. The stamp is read before inspecting, so a change racing the inspection triggers another. A missing or unreadable stamp (2 s timeout for dead mounts) falls back to the 15 s interval. Failed inspections are never extended by the stamp.unfoldfuture, and with it the lock every other subscriber on the checkout waits for. The existingfailure_keeps_the_last_successtest hung on exactly this before I made the change.Engine:
repos.rsworkspace_checkoutreads linked worktrees from a singlegit worktree list --porcelain. The listing is cached per repository and revalidated against the mtime of.git/worktrees(worktree add/remove/pruneshow up immediately), with a 10 s TTL forworktree moveand repos without a stampable.gitdir. Misses are cached too, which matters forchange_request_root, since it scans every space.git_runscounter onRepos::git(the seam for counting).UI:
change_requests.rs,state.rsfiles/sections.rs) and in their own composer footer. So they're kept for those surfaces instead of being dropped outright.select_chatnow reconciles watches.Git-invocation counts (tests written first; they fail on
main)mainwatchers_on_one_checkout_share_one_inspection_per_interval: 8 watchers, 10 × 15 s intervals, unstampable git dirunchanged_git_metadata_skips_reinspection_until_head_moves: 4 watchers on an idle checkout, 10 intervalsHEADmoves (seen within one interval)workspace_checkout_lists_worktrees_once_until_they_change: 20 authorizations (10 hits + 10 misses)change_request_watches_follow_visibility_and_badge_surfaces(UI)[lead, lead-side, other, other-side][lead, other]; selection-scoped side chats; zero tasks with visibility off (passes onmaintoo, so the desktop isn't the leaker)Verification (Windows):
cargo test -p roboco-engine --lib: 344 passed, 1 ignored.cargo test -p roboco-ui --lib: 1483 passed, 5 ignored.cargo test -p roboco-engine --test workspace_files: 6 passed, 1 failed.write_rejects_changed_checkout_even_when_contents_matchfails identically onmain: the worktree checkout's README hash differs undercore.autocrlf=true. It's unrelated to this change.cargo clippy -p roboco-engine -p roboco-ui --lib --tests: no warnings in the touched code. The engine test target needs-A clippy::unused_io_amountto get past a pre-existing deny-level lint atrpc.rs:3232.cargo fmtisn't clean onmain(it rewrites ~80 files), so the diff only formats the lines it touches.Deliberately left out
gh pr listcadence. The provider lookup is still per (checkout, branch): 45 s without a PR, 2 min with one.ghspawns its owngitchildren (grandchildren of Roboco, not in the counts above). With ~27 idle branches that is about 0.6gh/s. Worth a longer no-PR TTL, or skipping the lookup for branches without an upstream, in a follow-up.checkout_identity(2 git per successful workspace-file resolution) is not cached; the live sample showed it at ~1/s.workspace_checkout. The old code only counted worktrees with abranchline, so a worktree mid-rebase was rejected. They're the repo's own checkouts, so I treated the old behaviour as a bug rather than a rule.