perf(daemon): run the unit suite with a shared module registry - #1076
Conversation
There was a problem hiding this comment.
Approved. I reviewed exact head 7243db153f268018fa09efcdc0ee2a0982799e72 and found no blocking regression.
The new root command is a true complement of the daemon package while still running the repository-level script tests, and the dedicated daemon job invokes the same vitest run behavior with only deterministic, total, disjoint shard selection added. The shared sequencer remains covered by the control-plane unit suite, and the reusable-workflow summary now includes the unit job plus both daemon shard summaries.
Verification: the local synthetic merge has exactly the supplied base/head parents, every changed file matches the trusted head, and the patch has no added trailing whitespace. GitHub CI passed Build, Check, Unit Test, Daemon Unit Test 2/2, Sandbox, and both integration shards. Daemon Unit Test 1/2 currently has one 5-second timeout in the unchanged daemon-agent-mention-routing.test.ts test at line 547; because that test and behavior predate this patch, I do not consider it a review blocker.
sent by review-bot (Codex · gpt-5.6-sol) · open in session
7243db1 to
1414b26
Compare
There was a problem hiding this comment.
Approved for current head 1414b269edf5b7fc0b5ebdbb2a47a0030b60ccd4; I found no blocking regression in this revision.
This synchronized PR diff no longer contains the CI-sharding changes from the earlier head. Against base 385d1f1e1a78ceb3bd7d333403eceb503539a91d, it contains only the WorkspaceManager staging refactor in packages/daemon/src/workspace/workspace-manager.ts. The existing resolver, path clearer, sandbox-mode flag, and clone single-flight now live behind a manager object, while all current module-level call paths continue through a single default instance, preserving current behavior.
Scope note: this commit does not yet provide per-daemon workspace isolation because no WorkspaceManager instance is owned or passed by Daemon; the source explicitly identifies that threading as step 2, consistent with the PR being parked. I treated the pre-existing process-global behavior as non-blocking because this patch does not amplify it.
Verification: the checkout is exactly the trusted head, the changed file matches that commit's raw content, the current PR head/base remain the supplied SHAs, and the patch has no added trailing whitespace. CI was still running at review time.
sent by review-bot (Codex · gpt-5.6-sol) · open in session
…er process Where an agent's git runs, how a path this process cannot see gets emptied, whether workspaces live in sandboxes at all, and the clone single-flight were four module-level bindings in workspace-manager.ts. A process holding two daemons therefore had one plane between them: the second daemon inherited the first's git runner and sandbox mode, so its git could run through the other's sandbox channel, or a cluster clone could land on this disk. Collect the four into a `WorkspaceManager` instance. This step only moves the state and leaves a module-level default instance behind the existing `set*` entry points, so every caller and every behavior is unchanged; the next step threads an explicit instance down from the owning Daemon and removes both. The test suite is the loudest current victim: it holds many daemons per process and only survives because Vitest gives each FILE a fresh module registry, which is exactly what blocks running the daemon suite with `isolate: false`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FtPhRcPGtRX8RZqApUZRGu
…s it Follows the staging commit: the workspace entry points become methods on `WorkspaceManager`, and every caller resolves through the plane of the daemon that owns the agent instead of a module-level default. - `Daemon` holds `readonly workspaces = new WorkspaceManager()` and passes it to the three CP factories (`createWorkspaceGit`, `createWorkspaceReader`, `createLocalSkillsReader`), which now take it as their first argument. - `SessionManager` takes it as a dep, falling back to a private plane so the lightweight host harnesses still cannot share one. - The standalone `chat` CLI owns a plane with nothing registered on it, which is exactly what a local single-agent run means. - The module-level default instance and the four `set*` free functions are gone. Test files construct one plane each, which is the isolation Vitest's per-file module registry was silently providing. That is what unblocks running this suite with `isolate: false`. Verified against the pre-refactor baseline on the same machine: 3760 tests, the same 15 failures in the same 7 files (all pre-existing sandbox-network flakes), zero new. Typecheck, lint and format clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FtPhRcPGtRX8RZqApUZRGu
The suite executed every module once per test FILE — 248 files against a module graph this size, which measured as more CPU than the tests themselves. Share the registry per worker instead: on one machine the full suite goes from 8m55s of CPU (user+sys) to 2m48s, a 69% cut. Wall clock locally only moves 3m28s to 2m47s because much of it is idle waiting on Slack calls, not CPU; CI is CPU-bound and should track the CPU number. `isolate: false` shares module-level state, so the workspace execution plane had to become per-daemon instance state first (the two preceding commits). That left exactly one registry-sensitive construct: `vi.mock`. A mock is registered per file but rewires a module in the shared registry, so it either misses — the real module was already imported by an earlier file — or leaks into a later one. `workspace.test.ts` mocking `simple-git` surfaced as "fatal: not a git repository", because real git ran. Measured directly: excluding the five files that call `vi.mock` leaves ZERO isolation failures across the other 246, and those five are the entire remaining set. So they get a second project that keeps `isolate: true`, and everything else stops paying for isolation it does not use. `test/no-stray-vi-mock.test.ts` fails if that list ever drifts from what the suite actually mocks, in either direction, so a new `vi.mock` breaks at the seam rather than somewhere unrelated depending on which worker paired the two files. Verified against the pre-change baseline on the same machine: 3789 tests, 13 failures in 6 files, all pre-existing sandbox-network flakes, zero new. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FtPhRcPGtRX8RZqApUZRGu
There was a problem hiding this comment.
I found one blocking regression in current head cc52a85e4d8ea2a739fc415e7b38a04b84531e07.
The workspace execution plane is now correctly owned by Daemon and passed to the console workspace/git/skills seams, but it is not passed into the production SessionManager. That manager therefore creates a separate local-mode WorkspaceManager; for Kubernetes git workspaces with an agentDir, session creation/load resolves a pod-side cwd and then tries to canonicalize it on the daemon filesystem when calculating additional workspace directories. Pass the owning manager in this constructor (workspaces: this.workspaces) so all session workspace calculations use the same sandbox-aware plane.
Verification: the checkout is exactly the trusted head and all changed files match that revision; the patch has no added trailing whitespace. GitHub CI passed Build, Check, Sandbox, Daemon Store, Daemon Image, Runtime Sandbox Image, and both integration shards. Unit Test is currently failing under the new shared-registry configuration; GitHub exposed only the step-level exit-code annotation, so I could not attribute that failure to a specific test from the public logs.
sent by review-bot (Codex · gpt-5.6-sol) · open in session
| this.k8sPlane | ||
| ? clusterWorkspaceCwd(agent, this.k8sPlane.workspaceRootFor(agent.id)) | ||
| : resolvePreparedWorkspaceCwd(agent), | ||
| ? this.workspaces.clusterWorkspaceCwd(agent, this.k8sPlane.workspaceRootFor(agent.id)) |
There was a problem hiding this comment.
Pass workspaces: this.workspaces into this SessionManager construction. Its new getter falls back to new WorkspaceManager() when the dependency is omitted, so runtimeWorkspaceDirectories() uses a local-mode plane even though this resolver returns pod coordinates. For a --k8s git workspace with agentDir, additionalWorkspaceDirectories() then takes its local branch and calls realpathSync on paths that exist only in the sandbox, causing session creation/load to fail before the runtime call.
…shim handshake on stop Two defects the shared-registry run surfaced. `SessionManager` took the workspace plane as an optional dep but production never passed it, so it built a local-mode plane of its own. For a `--k8s` git workspace with an `agentDir` that meant `additionalWorkspaceDirectories` took its local branch and called `realpathSync` on paths that exist only in the sandbox, failing session create/load before the runtime call. Caught in review, not by a test. `ShimClient.stop()` left an in-flight handshake timer armed. `stop()` closes the transport and onClose settles the connect promise, but the timer stayed behind and fired `binding timed out` into a promise the supervision loop had stopped awaiting — an unhandled rejection that failed the whole run while all 3741 tests passed. Per-file isolation had been hiding it by tearing the worker down first. `stop()` now cancels that timer and only that: settling there too would reject a promise nobody is attached to, which is the same bug with a different message. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FtPhRcPGtRX8RZqApUZRGu
cc52a85 to
0d1f110
Compare
There was a problem hiding this comment.
Reviewed at 0d1f110ba0d033656cd796a83e9b7a64bad3cccc.
The previous blocking issue is fixed: Daemon now passes its owning WorkspaceManager into SessionManager, so k8s session creation and loading resolve additional workspace directories through the sandbox execution plane instead of a fresh local-mode manager.
I also checked the shared-registry split and its supporting refactor. The non-isolated and isolated projects are disjoint, every current vi.mock caller is routed to the isolated project, and the guard test will catch drift in either direction. Workspace git/read/skills consumers consistently receive the owning manager, and the shim shutdown change clears the in-flight handshake timer without altering normal bind/reconnect handling.
No blocking findings on this revision. I could not rerun the suite in this isolated checkout because dependencies are not installed; the review therefore relies on exact-revision source inspection and the submitted test results.
sent by review-bot (Codex · gpt-5.6-sol) · open in session
Cuts the daemon unit suite's CPU by ~69% by executing each module once per worker instead of once per test file. Replaces the CI-sharding approach that was previously on this branch (that diff is still reachable at
7243db15if we want it later).The measurement
packages/daemonwas ~88% of the Unit Test job's 3m31s step (248 files, 3699 tests, 186.52s against the step's 211s), andimportwas 327.50s of its ~700s of worker time — module re-execution, not tests.On one machine, full suite:
Wall moves less than CPU locally because much of this suite is idle, not busy — see the last section. CI is CPU-bound, so it should track the CPU number.
What had to change first
isolate: falseshares module-level state between files in a worker, so anything the daemon kept at module scope becomes cross-file state. Two commits precede the flip:1.
WorkspaceManager(1414b269,05619dec). Where an agent's git runs, how a path this process cannot see gets emptied, whether workspaces live in sandboxes, and the clone single-flight were four module-level bindings thatDaemon.start()wrote. They are now instance state owned by the daemon, threaded to the three CP factories,SessionManager, and the standalonechatCLI.This is a real production bug independent of tests: a process holding two daemons had one plane between them, so the second inherited the first's git runner and sandbox mode — its git could run through the other's sandbox channel, or a cluster clone could land on this disk. A k8s daemon beside a local one is exactly that shape.
Honest note: I predicted this refactor would unblock
workspace.test.tsandworkspace-git.test.tsunderisolate: false. It did not — they were failing for the reason below. The refactor stands on the production bug, not on the test win I expected from it.2.
vi.mockis the one genuinely registry-sensitive construct. A mock is registered per file but rewires a module in the shared registry, so it either misses (the real module was already imported by an earlier file) or leaks into a later one.workspace.test.tsmockingsimple-gitsurfaced asfatal: not a git repository— real git ran.Measured directly rather than guessed: excluding the five files that call
vi.mockleaves zero isolation failures across the other 246, and those five are the entire remaining set. So they get a second Vitest project that keepsisolate: true, and the other 246 stop paying for isolation they never used.test/no-stray-vi-mock.test.tsfails if that list drifts in either direction, so a newvi.mockbreaks at the seam instead of somewhere unrelated depending on which worker paired the two files.Test plan
tsc -p tsconfig.typecheck.json— 0 errors;eslint,prettier --checkcleanTwo things this does not fix
Sharding. Still available at
7243db15(2 shards, cost-balanced sequencer, reviewed and green). Worth re-deciding once CI shows what this change alone buys.The daemon unit suite reaches slack.com.
Daemon.start()makes a@slack/web-apicall on every boot, and 54 test files construct aDaemon. On CI it is fast because Slack answersinvalid_authover HTTP andp-retrydoes not retry a successful response — but where the request fails at the transport layer the retry ladder costs ~4s per boot, which is why local wall clock here is mostly idle. It also already caused a real CI failure on this branch: the one test usingboot(..., { distinctTokens: true })makes two such calls and timed out at 5s. Worth stubbing at the seam.🤖 Generated with Claude Code
https://claude.ai/code/session_01FtPhRcPGtRX8RZqApUZRGu