fix(server): skip live git in fetch_agent_history placement#1383
fix(server): skip live git in fetch_agent_history placement#1383yinaoxiong wants to merge 1 commit into
Conversation
|
| Filename | Overview |
|---|---|
| packages/server/src/server/session.ts | Two-line targeted fix: adds skipGitPlacement scoped strictly to the fetch_agent_history_request branch and passes { refreshGit: false } to buildProjectPlacementForCwd. Active-list path is unaffected. |
| packages/server/src/server/session.workspaces.test.ts | New regression test wires getCheckout to throw, runs fetch_agent_history_request, and asserts the response contains the expected entry via ports-and-adapters injection. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[listFetchAgentsEntries] --> B{request.type?}
B -->|fetch_agent_history_request| C[skipGitPlacement = true]
B -->|fetch_agents_request| D[skipGitPlacement = false]
C --> E[getPlacement with refreshGit: false]
D --> F[getPlacement with undefined options]
E --> G[resolveWorkspaceDirectory]
F --> G
G --> H{refreshGit === false?}
H -->|Yes - history path| I[peekSnapshot - cached\nno git subprocess]
H -->|No - active path| J[getCheckout - live git]
I --> K[workspace registry lookup]
J --> K
K --> L[ProjectPlacementPayload]
Reviews (2): Last reviewed commit: "fix(server): skip live git in fetch_agen..." | Re-trigger Greptile
| page: { limit: 25 }, | ||
| }); | ||
|
|
||
| expect(getCheckout).not.toHaveBeenCalled(); |
There was a problem hiding this comment.
Mock call-count assertion on the boundary
expect(getCheckout).not.toHaveBeenCalled() asserts implementation internals rather than observable behavior. The test already covers the behavioral goal because getCheckout is wired to throw — if the history path ever calls it, the test fails with an unhandled rejection before reaching the expect(emitted) check. The explicit not-called assertion is therefore redundant and falls into the pattern the project rules flag (asserting spy/mock call counts instead of recording observable state). Consider relying solely on the throw-on-call guard and the emitted-entries assertion.
Rule Used: # Code Review Pattern Reference: Slop, Tests, Feat... (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
d31f79a to
4cf9422
Compare
The history list never renders checkout info (the client requests it with showCheckoutInfo=false and does not read the project payload), yet the daemon resolved each agent's placement via getCheckoutStatus, which spawns a sequence of git subprocesses per cwd. On slow filesystems (network mounts) a per-cwd `git status` takes seconds; summed across many sessions it exceeded the client's 10s RPC timeout, so the Sessions menu hung and showed an empty list. Resolve history placement from cached snapshots + the workspace registry (refreshGit: false) instead — no git spawns on the history path. The fetch_agents (active list) path is unchanged, the wire protocol is unchanged, and no client/CLI changes are needed. Verified on a real ~180-session machine: history placement dropped from ~11s (timeout, blank list) to sub-millisecond.
4cf9422 to
9789b9b
Compare
Linked issue
Closes #1382
Type of change
What does this PR do
The Sessions (history) list resolves every agent's placement through
getCheckoutStatus(cwd), which spawns git subprocesses per cwd. On slow / network filesystems eachgit statustakes seconds, and the sum across all distinct cwds blows past the client's 10sfetch_agent_historyRPC timeout — so the list hangs and renders empty.The history path doesn't use that git data at all: the client requests it with
showCheckoutInfo={false}, never reads theproject/checkout payload, and applies no project filter here. So the git work is pure overhead.This PR makes
fetch_agent_historyresolve placement withrefreshGit: false(cached snapshot + workspace registry lookup) instead of live git. Thefetch_agents(active list) path is unchanged, the wire protocol is unchanged, and no client/CLI changes are needed.How did you verify it
Tested on a real machine with ~180 sessions across 15 distinct cwds, several on network mounts.
Before (current code), from daemon.log:
fetch_agent_history_requestlatency: 10.7s – 11.9s — over the 10s client timeout, list came back empty.Micro-benchmark of the placement step (real
getCheckoutStatusagainst the real cwds, vs the registry lookup the fix uses):End-to-end after the fix: built the server, swapped it into my daemon, restarted, and opened the Sessions list from the app — it now loads sub-second instead of timing out.
Automated test: added a test that mocks
getCheckoutStatusto throw and asserts the history path still returns entries without ever calling it (proves no git is spawned). Fullsession.workspaces.test.tspasses (53 passed, 4 pre-existing skips).Checklist
npm run typecheckpasses (0 errors, all workspaces)npm run lintpasses (oxlint: 0 warnings, 0 errors)npm run formatran