🤖 perf: skip git/PR status refresh on unrelated workspace metadata events - #5566
Conversation
GitStatusStore and PRStatusStore refreshed immediately on every workspace metadata event, and AppLoader's re-applied setClient(api) refreshed again. requestImmediate bypasses the debounce and hidden-window checks, so each event spawned a git status (and gh) process. - setClient is a no-op when the client reference is unchanged. - syncWorkspaces refreshes only on reactivation or when a subscribed workspace is added/removed or one of its status inputs changes (shared projection in browser/utils/statusRefreshInputs.ts). Generated with xum Signed-off-by: Thomas Kosiewski <tk@coder.com>
Signed-off-by: Thomas Kosiewski <tk@coder.com>
Rewrite change-narration comments in timeless terms, shorten the GitStatusStore syncWorkspaces comment, and share one clean git-status result helper in the metadata-driven refresh tests. Signed-off-by: Thomas Kosiewski <tk@coder.com>
Add an initialization-state row to the prompt-refresh table so pruning lifecycle fields from the status-input projection fails a test, and extend the churn test past one debounce window so scheduling a debounced refresh on unrelated metadata events fails it. Signed-off-by: Thomas Kosiewski <tk@coder.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7f44d0c09b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
Unrelated metadata events no longer retry PR refreshes, so the PR (5 s) and stack (60 s) cache TTLs could swallow the single refresh a relevant change requests and keep the previous checkout's PR. Mark changed workspaces and probe them regardless of TTL on the next refresh, including when the change lands during an in-flight probe. Drop the no-op removal branch of the helper.
|
@codex review |
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 82369234bf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Unrelated metadata events no longer retry refreshes, so the 3-60 s fetch backoff could swallow the fetch a checkout change needs and leave ahead/behind on stale remote refs. Mark changed subscribed workspaces; the next refresh fetches their key first regardless of backoff (not while in progress), with the changed workspace as representative. Marks are consumed before the fetch starts; its completion requests one refresh, as does an ordinary fetch whose key gained a mark meanwhile.
|
@codex review |
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 38225f61e0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
… after secondary fetches Remember status-input changes of undisplayed workspaces so their first refresh after resubscribing bypasses the PR/stack TTLs and fetch backoff. Request the forced follow-up status read only after the secondary repo fetches settle, so it cannot race them.
|
@codex review |
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a07f0a1320
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…mark Other changed workspaces sharing the fetch key keep their marks, so the follow-up refresh fetches each in its own checkout after its own runtime eligibility check; a stopped runtime registers a retry instead of losing its unique secondary repos.
|
@codex review |
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f58209eb5a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f58209eb5a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Review loop stopped: NOT READY. Round 6 of 6 did not give a clean Codex verdict on What happened:
Current state: all threads are resolved. Required is red only because of the Codex Comments gate on round 6, and every build/test check passes. The core change (0 git/gh spawns under unrelated churn, down from 619/min) is unchanged since round 1. Options for the reviewer:
|
|
Readiness gates on
Test-audit suggestions, not applied because they would move the head and need a 7th Codex round:
Decisions needed from a human: (a) clear the UAT isolation stop, so a short UAT round can run on this head, and (b) grant one more Codex round or waive the clean-verdict gate. If either moves the head, apply items 1-5 in the same push. |
…lacement From the independent test audit: pin that the PR store consumes a change mark after one probe and that a change during a forced fetch is fetched again inside the backoff; fold the multi-project added-repo test into the secondary-fetch ordering test; give the undisplayed-move PR test a real negative wait; move the fetchWorkspace JSDoc back above its method.
|
@codex review |
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6563ce27cb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
refreshAll consumed the mark before the PR and stack probes ran, so a transient executeBash failure left the previous checkout's PR/stack on screen until the next focus (no unrelated metadata event retries anymore). Keep the mark until both probes answer; on failure schedule a debounced retry, at most 3 attempts in total. A newer change replaces the mark, so a change during a probe still gets its own refresh.
|
@codex review |
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1bab0bc3b7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Readiness record: merging at
Generated with |
1 participant
Summary
Unrelated workspace metadata events no longer start
git statusorghcommands.GitStatusStoreandPRStatusStorenow refresh immediately only when a metadata change affects a workspace the UI shows: the workspace was added or removed, or one of its status inputs changed. Under 10 unrelated events/s, git-statusexecuteBashcalls drop from 600/min to 0/min, visible or hidden.Background
Live flight-recorder data showed
workspace.executeBashas the second most frequent RPC. Each metadata event built a new metadata Map. Two calls then firedRefreshController.requestImmediate()once per event:syncWorkspaces(map)in both stores.setClient(api)in both stores. AppLoader re-runs its store-sync effect on every metadata change and passes the sameapieach time.requestImmediate()skips the debounce, the 500 ms minimum gap and the hidden-window check. The result was one backend process spawn per event. On a large backend each spawn blocks the event loop about 5 ms (page-table copy; this is an estimate).Implementation
setClientreturns early when the client reference is unchanged. A first connect or a reconnect (null→ client, or a new client) still refreshes.src/browser/utils/statusRefreshInputs.ts(hasSubscribedStatusInputChange) defines the status-relevant metadata fields once. Each field has a comment that explains why it is there: checkout location (name,projectPath,namedWorkspacePath,subProjectPath,runtimeConfig), fetch/multi-project keys (projectName,projects), and lifecycle states that decide whether a command can run (isInitializing,isRemoving,incompatibleRuntime,transcriptOnly, archive timestamps). Only subscribed workspaces are compared. The helper skips entries that kept their reference, so the cost stays small. It returns subscribed workspaces that just appeared, and any workspace whose inputs changed. A change to an undisplayed workspace is remembered but starts no refresh, so its first refresh after resubscribing bypasses the caches below.syncWorkspacesin both stores keeps all cleanup. It callsrequestImmediate()only on reactivation or on a relevant change. For other changes it does nothing. It does not callschedule(), because continuous churn would then add one refresh per debounce window.PRStatusStoremarks each changed workspace. The next refresh probes its PR and stack regardless of the 5 s and 60 s cache TTLs, so the one metadata-driven refresh cannot be swallowed. This includes a change that lands while a probe is in flight. Before this PR, the next unrelated event retried the refresh. Now no unrelated event retries it.GitStatusStoremarks each changed workspace for a passivegit fetch. The next refresh fetches its key first and ignores the 3-60 s fetch backoff, but not while that key's fetch is in progress. The changed workspace represents its key, so the fetch runs in the new checkout and covers its new secondary repos. Xum consumes the fetched representative's mark before the fetch starts. Other changed workspaces that share the key keep their marks, and each gets its own fetch after its own runtime eligibility check. When the fetch and its secondary-repo fetches finish, Xum requests one refresh, so status reads the new refs and the next marked key gets its turn. A failed fetch is not retried in a loop.Before / after
The bench replays AppLoader's real caller sequence (
setClient(api)+syncWorkspaces(newMap)) with a stubbed client. It runs 60 s of wall time, with 10 title-change events/s on 3 other workspaces and 1 subscribed workspace. Startup calls are excluded (2 in every run). The bench script was temporary and is not committed.5ea096bb28(calls/min)The live dogfood A/B used a real dev server, counted
git status --porcelainprocesses in workspace worktrees, and ran 10 events/s for 60 s. It measured 501 status runs and 25ghruns on base, and 0 / 0 on this branch. Title churn on the open workspace itself for 30 s gave 287 status runs on base and 0 on this branch.Validation
executeBash. Runtime, name, project path and initialization changes refresh promptly. Removal and re-add, reconnect, and an in-flight follow-up still refresh. PR and stack probes run promptly after a checkout change within the cache TTLs, and again after a change made during a held probe. A checkout change within the fetch backoff triggers a passive fetch. This holds for a change during a held fetch, for two changed fetch keys, and for a project added to a multi-project workspace. A failed fetch causes no loop. A file-modification refresh still fires during 10/s churn, and churn adds no extra refreshes. 7 of these tests fail on base, and mutation checks cover thesetClientguards and the helper branches.↑1,*) while other workspaces churned. Backend restart and branch switch also refreshed.Risks
The risk is low and limited to passive git/PR indicators. If a metadata field that affects status were missing from the projection, that workspace's indicator would update on the next focus, file edit or subscription instead of at once. The projection is listed in one place with a rule for when to add fields.
Deferred (not fixed in this PR):
RuntimeStatusStorehas the samesetClient/syncWorkspaceschurn pattern. Tracked in 🤖 perf: stop RuntimeStatusStore refresh churn on unrelated metadata events #5573.main, and churn used to hide it. Tracked in 🤖 perf: re-read git status after a passive fetch settles #5572. Separately,RefreshController.dispose()is permanent, so reactivating a store afterdispose()(Strict Mode, dev only) never refreshes. That behavior already exists onmainand this PR does not change it.📋 Implementation Plan
Plan: stop git/PR status refresh churn on unrelated workspace metadata events
Problem
GitStatusStore.syncWorkspacesandPRStatusStore.syncWorkspacescallrefreshController.requestImmediate()on every metadata Map change.requestImmediate()bypasses the debounce, the 500 ms minimum interval and the hidden-window check, and it clears a pending debounce timer. AppLoader's store-sync effect (src/browser/components/AppLoader/AppLoader.tsx, deps includeworkspaceContext.workspaceMetadataandapi) also callsgitStatusStore.setClient(api)andgetPRStatusStoreInstance().setClient(api)on every metadata change, and bothsetClientimplementations callrequestImmediate()even when the client reference is unchanged. Result: onegit statusexecuteBash (plus PR commands) per metadata event, visible or hidden. EachonMetadataevent replaces exactly one Map entry with a new object (other entries keep their references); snapshot events rebuild the whole map.Fix
setClientidempotent in GitStatusStore and PRStatusStore: return early when the client reference is unchanged. Keep the immediate refresh fornull -> clientand for a different client.Status-relevant inputs = every metadata field the refresh path reads, directly or through helpers, plus fields that change which checkout the backend runs commands in. Verify by reading: GitStatusStore (checkWorkspaceStatus, checkMultiProjectWorkspaceStatus, getBaseRef, getFetchKey, tryFetchWorkspaces, isMultiProject/getProjects helpers, passive runtime eligibility helpers such as onPassiveRuntimeEligible/canProbe*), PRStatusStore (refreshAll and helpers), and the backend
workspace.executeBashworkspace resolution (path derived from name/namedWorkspacePath/projectPath/runtimeConfig). Expected candidates (verify, do not trust blindly): runtimeConfig, projectPath, projectName, projects, name, namedWorkspacePath, isInitializing, transcriptOnly, incompatibleRuntime, isRemoving. Define the projection once, with a comment naming why each field is included, and share it between both stores if both depend on the same inputs (PR store may need a different/superset list; verify). Structural comparison is fine for nested values (runtimeConfig, projects); avoid adding a generic deep-equal library.requestImmediate()only when (a) the store was inactive and is reactivated, or (b) the helper reports a relevant change for a subscribed workspace. Otherwise do nothing (noschedule()). Subscription-triggered immediate refresh (queueImmediateUpdate / subscribeWorkspace microtask), focus/visibility refresh, file-modifyschedule(), invalidateWorkspace, chat-replay gate retries and runtime eligibility retries stay unchanged. Metadata arriving after a subscription counts as "added" for that subscribed id, so it refreshes.Tests (tests first; load
test-auditand answer its authoring questions)In src/browser/stores/GitStatusStore.test.ts and PRStatusStore.test.ts (existing jest-globals style with stubbed client,
bun test <file>), plus a helper test only if it covers branching not already covered through the stores:setClient(sameClient)followed bysyncWorkspaces(newMap)with churn on another workspace -> zero additional executeBash.schedule()refresh is pending -> the refresh still fires within its debounce window (churn cannot starve it).setClient(null)thensetClient(client)-> refresh still happens.Hidden and visible variants where behavior differs. Route any new test file (src/** tests are auto-routed by
bun test src; confirmmake static-checkcheck-test-routing passes). No "exported for tests" comments; no test-only production exports.Before/after measurement (parent runs it after the workflow; workflow agents need not)
Temporary bench (not committed) with stubbed client and fake clock: 60 simulated seconds, one subscribed workspace, 10 unrelated metadata events/s through the real AppLoader caller sequence (setClient(same) + syncWorkspaces(new Map)), document hidden and visible; count git-status executeBash, fetch executeBash and PR commands separately; startup calls recorded separately. Run against base main and the branch.
Acceptance criteria
make static-checkand touched/sibling tests pass. No UI changes.Dogfooding / UAT focus (synthetic)
Start a dev server (
make dev-serverwith an isolated XUM_ROOT) with a project and at least 3 workspaces. Open workspace A. In a loop generate ~10 metadata events/s on other workspaces (e.g. rename titles / update workspace settings via the server API orxum apiCLI). While churn runs: (1) edit a file in A's worktree and commit; verify A's git status indicator (footer/branch selector dirty/ahead counts) updates within the normal refresh window (file-modify debounce ~3 s or on focus); (2) make an uncommitted edit and verify the dirty indicator; (3) confirm backend executeBash/git-status RPC rate stays near idle during churn (e.g. backend debug log or counting requests) compared with base. Capture screenshots of the indicator before/after commit and a short recording. Check UI at 390px width only if UI changed (it should not).Generated with
xum• Model:anthropic:claude-opus-5-5• Thinking:high