Repository navigation
perf(host-service): bound large git watcher batches - #5746
Merged
Merged
Conversation
|
Ready to review this PR? Stage has broken it down into 4 individual chapters for you: 
Chapters generated by Stage for commit b4055ee on Jul 19, 2026 4:41pm UTC. |
Contributor
📝 WalkthroughWalkthroughThe PR documents a generic host-service worker pool for Git status computation, updates the profiling harness for worker execution and multi-workspace measurements, and bounds Git watcher path batches to broad invalidation events after 128 paths. ChangesWorker-pool profiling and rollout
Git watcher batch bounding
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Greptile SummaryThis PR adds a design document for a generic
Confidence Score: 4/5Documentation-only change; no production code is modified and merging carries no runtime risk. The document is thorough and grounded in real measurements. Two small inaccuracies exist: the pool-diagram concurrency formula omits the max(1, …) floor that the live limiter already applies, and the ModeTracker audit points to a field declaration at line 271 rather than the actual hot-path call at line 1237. Neither affects correctness of the codebase, but both could mislead a reader navigating the code from this doc. plans/20260717-host-service-git-worker-pool.md — the two line-reference and formula items noted above are worth a quick fix before this doc is widely referenced.
|
| Filename | Overview |
|---|---|
| plans/20260717-host-service-git-worker-pool.md | New design doc for a generic worker_threads pool in host-service; technically accurate overall but contains one misleading line reference (terminal.ts:271 vs. the actual hot-path at line 1237) and a minor formula discrepancy in the pool diagram (N = min(4,cpus−1) vs. max(1, min(4,cpus−1))). |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["tRPC git.getStatus"] --> B["gitStatusRefreshLimiter\n(per-workspace serialize + coalesce)"]
B -->|"run()"| C["hostWorkerPool.runTask\n'git/getStatusSnapshot'"]
C -->|"postMessage"| D["host-worker.js\nworker_threads pool\nN = max(1, min(4, cpus−1))"]
D --> E["Static task registry\ngit/getStatusSnapshot\ngit/rescan (Phase 3)\n…future tenants"]
E --> F["git subprocesses\n(drain + parse off main loop)"]
F -->|"result"| C
C -->|"snapshot"| A
G["GitWatcher rescan\n(Phase 3 → promoted)"] -->|"postMessage"| D
subgraph "Not in pool (contract violations)"
H["better-sqlite3 (shared handle)"]
I["ModeTracker/hint-scan\n(streaming — needs pinned-worker)"]
J["git mutations (ordering)"]
end
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A["tRPC git.getStatus"] --> B["gitStatusRefreshLimiter\n(per-workspace serialize + coalesce)"]
B -->|"run()"| C["hostWorkerPool.runTask\n'git/getStatusSnapshot'"]
C -->|"postMessage"| D["host-worker.js\nworker_threads pool\nN = max(1, min(4, cpus−1))"]
D --> E["Static task registry\ngit/getStatusSnapshot\ngit/rescan (Phase 3)\n…future tenants"]
E --> F["git subprocesses\n(drain + parse off main loop)"]
F -->|"result"| C
C -->|"snapshot"| A
G["GitWatcher rescan\n(Phase 3 → promoted)"] -->|"postMessage"| D
subgraph "Not in pool (contract violations)"
H["better-sqlite3 (shared handle)"]
I["ModeTracker/hint-scan\n(streaming — needs pinned-worker)"]
J["git mutations (ordering)"]
end
Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
plans/20260717-host-service-git-worker-pool.md:131-133
**ModeTracker line reference points to field declaration, not the hot path**
The doc says "`terminal.ts:271` feeds *every PTY output chunk* through `@xterm/headless` `writeSync`," but line 271 is just the `modeTracker: ModeTracker` field declaration inside a session interface. The actual hot-path call — `session.modeTracker.feed(bytes)` — is at line 1237, with the comment "Feed the tracker on every byte." Anyone navigating the code from this doc lands nearly 1000 lines away from the real work. Updating the reference to line 1237 (and optionally citing `terminal-mode-tracker.ts:123` where `writeSync` is invoked) would make the audit result actionable.
### Issue 2 of 2
plans/20260717-host-service-git-worker-pool.md:33
The pool diagram uses `N = min(4, cpus−1)` but the actual `DEFAULT_CONCURRENCY` in `git-status-refresh-limiter.ts` is `Math.max(1, Math.min(4, cpus().length - 1))`. On a single-CPU machine `cpus−1` would be 0, yet the live formula floors at 1. The diagram formula should match to avoid confusion when the "share `DEFAULT_CONCURRENCY`" note in the Scheduling section is read alongside it.
```suggestion
host-worker.js (worker_threads, N = max(1, min(4, cpus−1)))
```
Reviews (1): Last reviewed commit: "docs(host-service): worker pool design +..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+131
to
+133
| - **pty-daemon's own loop** — mostly I/O relay + FIFO writes (no emulator; handoff snapshot is bookkeeping-only). Expected cheap; verify. | ||
|
|
||
| These streaming/per-chunk workloads do **not** fit the request/response pool (contract §4). If measurement justifies it, the follow-up is a **pinned-worker (sharded/actor) mode** on the same worker entry: pin each session's tracker+hint-scan to a worker keyed by sessionId, stream chunks over a MessagePort with transferred ArrayBuffers, request/response only for `buildPreamble()` on attach. Do not build this before the numbers exist. |
Contributor
There was a problem hiding this comment.
The doc says "terminal.ts:271 feeds every PTY output chunk through @xterm/headless writeSync," but line 271 is just the modeTracker: ModeTracker field declaration inside a session interface. The actual hot-path call — session.modeTracker.feed(bytes) — is at line 1237, with the comment "Feed the tracker on every byte." Anyone navigating the code from this doc lands nearly 1000 lines away from the real work. Updating the reference to line 1237 (and optionally citing terminal-mode-tracker.ts:123 where writeSync is invoked) would make the audit result actionable.
Prompt To Fix With AI
This is a comment left during a code review.
Path: plans/20260717-host-service-git-worker-pool.md
Line: 131-133
Comment:
**ModeTracker line reference points to field declaration, not the hot path**
The doc says "`terminal.ts:271` feeds *every PTY output chunk* through `@xterm/headless` `writeSync`," but line 271 is just the `modeTracker: ModeTracker` field declaration inside a session interface. The actual hot-path call — `session.modeTracker.feed(bytes)` — is at line 1237, with the comment "Feed the tracker on every byte." Anyone navigating the code from this doc lands nearly 1000 lines away from the real work. Updating the reference to line 1237 (and optionally citing `terminal-mode-tracker.ts:123` where `writeSync` is invoked) would make the audit result actionable.
How can I resolve this? If you propose a fix, please make it concise.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!
| hostWorkerPool.runTask("git/getStatusSnapshot", …) | ||
| │ postMessage | ||
| ▼ | ||
| host-worker.js (worker_threads, N = min(4, cpus−1)) |
Contributor
There was a problem hiding this comment.
N = min(4, cpus−1) but the actual DEFAULT_CONCURRENCY in git-status-refresh-limiter.ts is Math.max(1, Math.min(4, cpus().length - 1)). On a single-CPU machine cpus−1 would be 0, yet the live formula floors at 1. The diagram formula should match to avoid confusion when the "share DEFAULT_CONCURRENCY" note in the Scheduling section is read alongside it.
Suggested change
| host-worker.js (worker_threads, N = min(4, cpus−1)) | |
| host-worker.js (worker_threads, N = max(1, min(4, cpus−1))) |
Prompt To Fix With AI
This is a comment left during a code review.
Path: plans/20260717-host-service-git-worker-pool.md
Line: 33
Comment:
The pool diagram uses `N = min(4, cpus−1)` but the actual `DEFAULT_CONCURRENCY` in `git-status-refresh-limiter.ts` is `Math.max(1, Math.min(4, cpus().length - 1))`. On a single-CPU machine `cpus−1` would be 0, yet the live formula floors at 1. The diagram formula should match to avoid confusion when the "share `DEFAULT_CONCURRENCY`" note in the Scheduling section is read alongside it.
```suggestion
host-worker.js (worker_threads, N = max(1, min(4, cpus−1)))
```
How can I resolve this? If you propose a fix, please make it concise.![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (2)
plans/20260717-host-service-git-worker-pool.md (2)
119-120: 🚀 Performance & Scalability | 🔵 TrivialQualify the concurrency-safety claim.
Worker isolation removes main-loop parsing contention, but higher concurrency still multiplies Git subprocesses, filesystem reads, worker memory, and clone traffic. Require CPU/I/O load tests and explicit capacity bounds before increasing concurrency above four.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plans/20260717-host-service-git-worker-pool.md` around lines 119 - 120, Qualify the concurrency-safety claim in the worker-pool capacity discussion: state that worker isolation removes main-loop parsing contention but higher concurrency still increases Git subprocesses, filesystem reads, worker memory, and clone traffic. Require CPU/I/O load testing and explicit capacity bounds before raising concurrency above four.
144-154: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winMeasure structured-clone overhead instead of asserting “no regression.”
Worker transfer adds a structured-clone copy before tRPC serialization; existing JSON serialization does not prove equivalent cost. Add size and clone-time measurements for large snapshots, especially the 2,000-file case.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plans/20260717-host-service-git-worker-pool.md` around lines 144 - 154, Update the Testing and Risks plan around worker snapshot transfer to measure structured-clone overhead rather than claiming no regression from existing JSON serialization. Add benchmarks capturing payload size and clone time for large snapshots, with explicit coverage of the 2,000-file case, and compare the results against the current serialization path where relevant.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@plans/20260717-host-service-git-worker-pool.md`:
- Around line 71-79: Align the rollout plan so git-watcher rescan/subscription
work is included in the initial Phase 1 tenant, matching the TL;DR and
recommendation. Remove it from Phase 3’s opportunistic list, or explicitly
document the dependency that keeps it in Phase 1, ensuring the primary 183 ms
loop-stall path cannot be deferred.
- Around line 40-46: Update the Git worker contract around getGitStatusSnapshot
to state that task payloads carry only clone-safe values such as worktreePath
and baseBranch, and that the worker constructs and configures its own SimpleGit
instance before invoking the handler. Keep live SimpleGit objects out of the
structured-clone boundary and preserve the existing Electron-free worker
constraints.
- Around line 57-59: The shared worker pool must prevent direct
getCommitFiles/getDiff and watcher tasks from occupying all workers and starving
limiter-fronted git.getStatus work. Update the pool scheduling design around
GitStatusRefreshLimiter and its pool tasks to add pool-level priority or reserve
capacity for limiter-fronted tasks, while preserving existing limiter
foreground/background ordering and direct-call coalescing.
- Line 121: Correct the latency statement in the terminal I/O findings: report
the baseline echo p95 of 3.7 ms versus the 138 ms post-reset settle value as
approximately 37×, and limit the 3–10× multiplier to churn-8 or describe the
settle spike separately.
- Around line 59-60: Update GitStatusRefreshLimiter.run and the git.getStatus
worker-task path to accept and explicitly propagate the procedure timeoutMs,
including the 15-second value. Ensure queued tasks honor cancellation before
execution, running tasks are cancelled on timeout, and timed-out workers are
recycled as required; do not rely on tRPC metadata being inherited
automatically.
- Around line 54-55: Update the inline fallback crash circuit described in the
worker pool plan to distinguish infrastructure/worker crashes from individual
task failures, and scope its threshold and fallback state per task type or
worker generation. Ensure a broken task type or generation cannot force
unrelated future task types onto the main thread, while preserving the existing
three-crashes-in-60-seconds behavior within the scoped boundary.
- Around line 39-40: Clarify the plan’s fallback architecture by defining an
explicit inline task-handler registry/import path alongside the worker registry.
Ensure in-process fallback can execute the same Git tasks while callers continue
importing only task types and the module-boundary contract remains intact.
- Around line 61-69: The bundling plan must require launchability tests, not
only file-presence checks. Extend the desktop and CLI packaging validation
around electron.vite.config.ts, packages/host-service/build.ts, and
build-dist.ts to execute or load each emitted host-worker artifact and verify it
starts successfully; resolve the ESM extension/package metadata choice (.mjs
versus package configuration) before rollout so failures cannot silently fall
back inline.
- Around line 26-35: Add the `text` language identifier to the fenced code block
containing the ASCII worker-pool diagram, changing the opening fence only and
preserving the diagram content unchanged.
---
Nitpick comments:
In `@plans/20260717-host-service-git-worker-pool.md`:
- Around line 119-120: Qualify the concurrency-safety claim in the worker-pool
capacity discussion: state that worker isolation removes main-loop parsing
contention but higher concurrency still increases Git subprocesses, filesystem
reads, worker memory, and clone traffic. Require CPU/I/O load testing and
explicit capacity bounds before raising concurrency above four.
- Around line 144-154: Update the Testing and Risks plan around worker snapshot
transfer to measure structured-clone overhead rather than claiming no regression
from existing JSON serialization. Add benchmarks capturing payload size and
clone time for large snapshots, with explicit coverage of the 2,000-file case,
and compare the results against the current serialization path where relevant.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a52c1b26-0658-4f32-8937-3b7770b14cf2
📒 Files selected for processing (1)
plans/20260717-host-service-git-worker-pool.md
Comment on lines
+26
to
+35
| ``` | ||
| tRPC git.getStatus ──► gitStatusRefreshLimiter (unchanged front door) | ||
| │ run() | ||
| ▼ | ||
| hostWorkerPool.runTask("git/getStatusSnapshot", …) | ||
| │ postMessage | ||
| ▼ | ||
| host-worker.js (worker_threads, N = min(4, cpus−1)) | ||
| static registry: git/* today, other domains later | ||
| ``` |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Specify a language for the fenced diagram.
Use text for this ASCII diagram so markdownlint passes.
Proposed fix
-```
+```text📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ``` | |
| tRPC git.getStatus ──► gitStatusRefreshLimiter (unchanged front door) | |
| │ run() | |
| ▼ | |
| hostWorkerPool.runTask("git/getStatusSnapshot", …) | |
| │ postMessage | |
| ▼ | |
| host-worker.js (worker_threads, N = min(4, cpus−1)) | |
| static registry: git/* today, other domains later | |
| ``` |
🧰 Tools
🪛 markdownlint-cli2 (0.23.0)
[warning] 26-26: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@plans/20260717-host-service-git-worker-pool.md` around lines 26 - 35, Add the
`text` language identifier to the fenced code block containing the ASCII
worker-pool diagram, changing the opening fence only and preserving the diagram
content unchanged.
Source: Linters/SAST tools
Comment on lines
+39
to
+40
| - **Typed task definitions**: each task module exports `defineWorkerTask<In, Out>({ type, handler })`. The worker entry imports the handlers; callers import only the types, so `runTask` is typed end-to-end and the caller graph never pulls handler code. | ||
| - Git handlers call the existing `getGitStatusSnapshot` / `git-helpers` — Electron-free, and simple-git works in a worker (it just spawns child processes). **The worker spawns the git subprocesses itself** — draining and parsing both leave the main loop. |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Define how inline fallback obtains task handlers.
The plan says callers import only types and handlers live in the worker registry, but fallback must execute the same task in-process. Add an explicit inline registry/import path; otherwise fallback cannot run Git tasks without violating the stated module-boundary contract.
Also applies to: 54-55
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@plans/20260717-host-service-git-worker-pool.md` around lines 39 - 40, Clarify
the plan’s fallback architecture by defining an explicit inline task-handler
registry/import path alongside the worker registry. Ensure in-process fallback
can execute the same Git tasks while callers continue importing only task types
and the module-boundary contract remains intact.
Comment on lines
+40
to
+46
| - Git handlers call the existing `getGitStatusSnapshot` / `git-helpers` — Electron-free, and simple-git works in a worker (it just spawns child processes). **The worker spawns the git subprocesses itself** — draining and parsing both leave the main loop. | ||
|
|
||
| ## Genericity contract (what task modules must satisfy) | ||
|
|
||
| 1. **Static registry** — handlers are bundled into `host-worker.js` at build time; you cannot ship closures from the main thread. Adding a domain = add a task module + import it in the entry. | ||
| 2. **Clone-safe boundary** — payloads/results cross via structured clone: plain data only, no live handles (SimpleGit, DB, sockets, emitters). | ||
| 3. **No shared process state** — handlers get everything via payload. No host-service DB (the better-sqlite3 handle is per-process), no event bus, no config singletons, no native addons. Enforced by a `no-electron-coupling`-style import test on the worker entry graph. |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Construct SimpleGit inside the worker.
getGitStatusSnapshot currently requires a live SimpleGit instance, which cannot cross the structured-clone boundary. Specify that the task payload contains plain data such as worktreePath and baseBranch, and that the worker creates/configures SimpleGit locally.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@plans/20260717-host-service-git-worker-pool.md` around lines 40 - 46, Update
the Git worker contract around getGitStatusSnapshot to state that task payloads
carry only clone-safe values such as worktreePath and baseBranch, and that the
worker constructs and configures its own SimpleGit instance before invoking the
handler. Keep live SimpleGit objects out of the structured-clone boundary and
preserve the existing Electron-free worker constraints.
Comment on lines
+54
to
+55
| 2. **Inline fallback** (new): if the worker script is missing or workers crash ≥3 times in 60s, log once and run tasks in-process (current behavior). Keeps standalone deployments and tests working if a bundle is malformed. Mirrors DaemonSupervisor's crash circuit. | ||
| 3. Everything else stays: fifo/coalesce/latest-wins, per-task timeout + worker recycle, abort. |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Scope the crash circuit to avoid disabling all tenants.
A generic “three worker crashes” circuit can force unrelated future task types onto the main thread after one broken task. Distinguish infrastructure crashes from task failures and scope fallback at least per task type or worker generation.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@plans/20260717-host-service-git-worker-pool.md` around lines 54 - 55, Update
the inline fallback crash circuit described in the worker pool plan to
distinguish infrastructure/worker crashes from individual task failures, and
scope its threshold and fallback state per task type or worker generation.
Ensure a broken task type or generation cannot force unrelated future task types
onto the main thread, while preserving the existing three-crashes-in-60-seconds
behavior within the scoped boundary.
Comment on lines
+57
to
+59
| ## Scheduling interplay | ||
|
|
||
| `gitStatusRefreshLimiter` remains the only queue for `getStatus` (per-workspace serialization, invalidation-coalescing, foreground/background priority). Pool concurrency = limiter concurrency (share `DEFAULT_CONCURRENCY` from `git-status-refresh-limiter.ts`) so pooled tasks never meaningfully queue behind the limiter. Limiter-fronted tasks use `fifo`; direct calls not behind the limiter (`getCommitFiles`, `getDiff`) use `coalesce` with a `workspaceId:proc:input` dedupe key. Procedure `timeoutMs` (15s/30s) carries into the pool task timeout. |
Contributor
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
Prevent direct pool users from starving git.getStatus.
The limiter’s foreground/background priority only applies within GitStatusRefreshLimiter. Direct calls such as getCommitFiles/getDiff and watcher tasks bypass that queue; a shared FIFO worker pool can consume every worker and delay status refreshes. Define pool-level priorities or reserve capacity for limiter-fronted work.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@plans/20260717-host-service-git-worker-pool.md` around lines 57 - 59, The
shared worker pool must prevent direct getCommitFiles/getDiff and watcher tasks
from occupying all workers and starving limiter-fronted git.getStatus work.
Update the pool scheduling design around GitStatusRefreshLimiter and its pool
tasks to add pool-level priority or reserve capacity for limiter-fronted tasks,
while preserving existing limiter foreground/background ordering and direct-call
coalescing.
Comment on lines
+59
to
+60
| `gitStatusRefreshLimiter` remains the only queue for `getStatus` (per-workspace serialization, invalidation-coalescing, foreground/background priority). Pool concurrency = limiter concurrency (share `DEFAULT_CONCURRENCY` from `git-status-refresh-limiter.ts`) so pooled tasks never meaningfully queue behind the limiter. Limiter-fronted tasks use `fifo`; direct calls not behind the limiter (`getCommitFiles`, `getDiff`) use `coalesce` with a `workspaceId:proc:input` dedupe key. Procedure `timeoutMs` (15s/30s) carries into the pool task timeout. | ||
|
|
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Propagate procedure timeouts explicitly.
The current git.getStatus procedure declares timeoutMs: 15_000, but GitStatusRefreshLimiter.run accepts no timeout argument. The worker task will not inherit tRPC metadata automatically; specify the timeout plumbing and cancellation/recycle behavior for queued and running tasks.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@plans/20260717-host-service-git-worker-pool.md` around lines 59 - 60, Update
GitStatusRefreshLimiter.run and the git.getStatus worker-task path to accept and
explicitly propagate the procedure timeoutMs, including the 15-second value.
Ensure queued tasks honor cancellation before execution, running tasks are
cancelled on timeout, and timed-out workers are recycled as required; do not
rely on tRPC metadata being inherited automatically.
Comment on lines
+61
to
+69
| ## Bundling (3 surfaces, same side-by-side pattern as pty-daemon) | ||
|
|
||
| Resolution copies `daemon/singleton.ts`: env override `SUPERSET_HOST_WORKER_SCRIPT_PATH` → `host-worker.js` next to the running bundle → `packages/host-service/dist/host-worker.js` fallback. | ||
|
|
||
| 1. `apps/desktop/electron.vite.config.ts` — add input `"host-worker": packages/host-service/src/workers/host-worker.ts` (emits `dist/main/host-worker.js`, side-by-side with `host-service.js`). | ||
| 2. `packages/host-service/build.ts` — add `src/workers/host-worker.ts` as a second entrypoint. | ||
| 3. `packages/cli/scripts/build-dist.ts` — copy `dist/host-worker.js` into `lib/` next to `host-service.js`. | ||
|
|
||
| Add a bundle-presence guard like `apps/desktop/scripts/check-pty-daemon-bundle.ts`. |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Make worker launchability a required packaging test.
The plan identifies an ESM/path risk for CLI lib/, but only proposes a presence check. A present-but-unloadable worker will silently trigger inline fallback. Add launch tests for both desktop and CLI artifacts and choose .mjs versus package metadata before rollout.
Also applies to: 152-153
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@plans/20260717-host-service-git-worker-pool.md` around lines 61 - 69, The
bundling plan must require launchability tests, not only file-presence checks.
Extend the desktop and CLI packaging validation around electron.vite.config.ts,
packages/host-service/build.ts, and build-dist.ts to execute or load each
emitted host-worker artifact and verify it starts successfully; resolve the ESM
extension/package metadata choice (.mjs versus package configuration) before
rollout so failures cannot silently fall back inline.
Comment on lines
+71
to
+79
| ## What moves | ||
|
|
||
| **Phase 1 — status snapshot** (the win): `getGitStatusSnapshot` and everything inside it — `parseNumstat`, `countUntrackedFileLines` (reads + line-counts untracked files on the loop today), `detectUnstagedRenames`, `getChangedFilesForDiff`. Serves `git.getStatus`, and the git-watcher/PR-runtime refreshes that funnel through the limiter. | ||
|
|
||
| **Phase 2 — read-path procs**: `getCommitFiles`, `listCommits`, `getBranchSyncStatus`, and `getDiff`'s `git.show` content reads (large-file drains + string assembly). | ||
|
|
||
| **Phase 3 — opportunistic**: git-watcher's `ls-files`/rescan subscription work; PR-runtime per-workspace git interrogation, where not already sharing Phase 1 tasks. Future non-git tenants (each is just a new task module): diff/patch text assembly, search/indexing if host-service ever grows one, large JSON parse/serialize hot spots. | ||
|
|
||
| **Not moving**: port-scanner (already subprocess-based, trivial parsing), reaper/cloud-sync/tunnel (I/O-bound), git mutations (`stageAll`, `discardChanges`, … — rare, fast, want the loop's ordering). Future candidate, out of scope: better-sqlite3 queries are synchronous on the loop; if profiling ever shows DB stalls, the same pool pattern applies. |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Align the rollout phases with the measured conclusion.
The TL;DR and recommendation treat watcher rescan as an initial tenant, while Phase 3 still lists it as opportunistic. Interpretation explicitly says to promote it from Phase 3. Move it into Phase 1 or define the dependency clearly; otherwise the primary 183 ms loop-stall path may be deferred accidentally.
Also applies to: 117-120
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@plans/20260717-host-service-git-worker-pool.md` around lines 71 - 79, Align
the rollout plan so git-watcher rescan/subscription work is included in the
initial Phase 1 tenant, matching the TL;DR and recommendation. Remove it from
Phase 3’s opportunistic list, or explicitly document the dependency that keeps
it in Phase 1, ensuring the primary 183 ms loop-stall path cannot be deferred.
| 1. **The loop stalls come from the watcher/event path, not status parsing.** Churn-8 stalls the loop to 183ms and drags query p95 to 127ms; heavy-burst (huge diffs, no fs churn) keeps the loop clean (p95 4.2ms) while statuses queue in the limiter + git subprocesses. GitWatcher event filtering/debounce/rescan is the strongest offload candidate for loop health — promote it from Phase 3. | ||
| 2. **User-visible status staleness under churn is limiter queueing.** Background refreshes take 3.7–14.6s at concurrency 4 with 8 hot workspaces. The worker pool makes raising that concurrency safe (in-process, more concurrent snapshots would multiply loop parse work; in workers it's just more cores). | ||
| 3. Snapshot cost scales with diff size (~1.2s at 2000 modified files) — worth offloading, but it wasn't the loop killer. | ||
| 4. Terminal I/O degrades 3–10× under churn (echo p50 1.8→5.7ms, p95 138ms during settle) — real but not catastrophic; consistent with conclusion 1. |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Correct the reported latency multiplier.
Baseline echo p95 is 3.7 ms and post-reset settle is 138 ms, approximately 37×, not 3–10×. Scope the 3–10× claim to churn-8 or report the settle spike separately.
🧰 Tools
🪛 LanguageTool
[grammar] ~121-~121: Ensure spelling is correct
Context: ...0× under churn (echo p50 1.8→5.7ms, p95 138ms during settle) — real but not catastrop...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@plans/20260717-host-service-git-worker-pool.md` at line 121, Correct the
latency statement in the terminal I/O findings: report the baseline echo p95 of
3.7 ms versus the 138 ms post-reset settle value as approximately 37×, and limit
the 3–10× multiplier to churn-8 or describe the settle spike separately.
Contributor
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
11 issues found across 1 file
You’re at about 92% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="plans/20260717-host-service-git-worker-pool.md">
<violation number="1" location="plans/20260717-host-service-git-worker-pool.md:12">
P2: The proposed GitWatcher target is not a valid pool task as written: moving rescan/subscription work would violate the document's own DB/event-emitter boundary, while leaving those operations on the main thread may leave the measured 183 ms path untouched. Isolate a pure clone-safe scan/filter transform for the pool and keep watcher, DB, and event dispatch lifecycle on the main thread.</violation>
<violation number="2" location="plans/20260717-host-service-git-worker-pool.md:33">
P3: The diagram formula can produce `N = 0` on single-core machines, which diverges from the documented concurrency floor used elsewhere. Align this expression to include a minimum of 1 worker to avoid contradictory sizing guidance.</violation>
<violation number="3" location="plans/20260717-host-service-git-worker-pool.md:53">
P2: Idle reaping is not a shutdown lifecycle: a newly created or active worker can keep host-service tests and graceful process shutdown alive until work completes or the reap timeout expires. Register explicit pool disposal with host-service shutdown and app/test teardown.</violation>
<violation number="4" location="plans/20260717-host-service-git-worker-pool.md:54">
P1: The promised inline fallback cannot execute with the stated import boundary: when the worker script is missing or the crash circuit opens, the main thread has no handler implementation to run. Keep a main-thread task registry (separate from the worker entry) or make the fallback import handlers explicitly.</violation>
<violation number="5" location="plans/20260717-host-service-git-worker-pool.md:59">
P2: Timeout propagation is asserted but not specified. It would help to define how procedure `timeoutMs` is plumbed into queued and running worker tasks, including cancellation/recycle behavior when the timeout is hit.</violation>
<violation number="6" location="plans/20260717-host-service-git-worker-pool.md:59">
P2: Direct pool users bypass `gitStatusRefreshLimiter` priorities here, so a shared worker pool can be saturated by non-limiter jobs and delay status refreshes. Defining pool-level priority or reserved worker capacity for limiter-fronted tasks would protect status freshness under load.</violation>
<violation number="7" location="plans/20260717-host-service-git-worker-pool.md:69">
P3: A bundle-presence guard alone won’t catch “present but unloadable” worker artifacts. Adding a launchability check (for both desktop and CLI outputs) would catch packaging/ESM resolution failures before rollout.</violation>
<violation number="8" location="plans/20260717-host-service-git-worker-pool.md:73">
P2: Moving everything inside `getGitStatusSnapshot` into the pool removes the current common-directory fetch deduplication across workers, so several workspaces in one repository can start concurrent or repeatedly scheduled `git fetch` processes. Keep `scheduleBaseRefFetch` behind a main-thread coordinator or otherwise centralize this shared per-repository state.</violation>
<violation number="9" location="plans/20260717-host-service-git-worker-pool.md:77">
P2: This phase assignment conflicts with the measured conclusion that watcher rescan/filtering is the main loop-stall source and should be promoted. Leaving it in Phase 3 risks delaying the highest-impact offload.</violation>
<violation number="10" location="plans/20260717-host-service-git-worker-pool.md:121">
P3: The multiplier here doesn’t match the listed values: p95 goes from 3.7ms to 138ms (~37×). Consider scoping the 3–10× claim to churn-only metrics and calling out the post-reset settle spike separately.</violation>
<violation number="11" location="plans/20260717-host-service-git-worker-pool.md:127">
P3: This line reference appears to point to a declaration rather than the per-chunk execution path. Updating it to the `session.modeTracker.feed(bytes)` call site (and optionally the `terminal-mode-tracker.ts` `writeSync` line) would make the finding directly actionable.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| ## Pool changes vs the v1 runner | ||
|
|
||
| 1. **Idle reaping** (new): terminate workers idle > 30s. host-service is long-lived and per-org — a 3-org machine must not hold 12 warm workers. v1 never needed this because the pool lived in the single Electron main process. | ||
| 2. **Inline fallback** (new): if the worker script is missing or workers crash ≥3 times in 60s, log once and run tasks in-process (current behavior). Keeps standalone deployments and tests working if a bundle is malformed. Mirrors DaemonSupervisor's crash circuit. |
Contributor
There was a problem hiding this comment.
P1: The promised inline fallback cannot execute with the stated import boundary: when the worker script is missing or the crash circuit opens, the main thread has no handler implementation to run. Keep a main-thread task registry (separate from the worker entry) or make the fallback import handlers explicitly.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At plans/20260717-host-service-git-worker-pool.md, line 54:
<comment>The promised inline fallback cannot execute with the stated import boundary: when the worker script is missing or the crash circuit opens, the main thread has no handler implementation to run. Keep a main-thread task registry (separate from the worker entry) or make the fallback import handlers explicitly.</comment>
<file context>
@@ -0,0 +1,154 @@
+## Pool changes vs the v1 runner
+
+1. **Idle reaping** (new): terminate workers idle > 30s. host-service is long-lived and per-org — a 3-org machine must not hold 12 warm workers. v1 never needed this because the pool lived in the single Electron main process.
+2. **Inline fallback** (new): if the worker script is missing or workers crash ≥3 times in 60s, log once and run tasks in-process (current behavior). Keeps standalone deployments and tests working if a bundle is malformed. Mirrors DaemonSupervisor's crash circuit.
+3. Everything else stays: fifo/coalesce/latest-wins, per-task timeout + worker recycle, abort.
+
</file context>
|
|
||
| ## Pool changes vs the v1 runner | ||
|
|
||
| 1. **Idle reaping** (new): terminate workers idle > 30s. host-service is long-lived and per-org — a 3-org machine must not hold 12 warm workers. v1 never needed this because the pool lived in the single Electron main process. |
Contributor
There was a problem hiding this comment.
P2: Idle reaping is not a shutdown lifecycle: a newly created or active worker can keep host-service tests and graceful process shutdown alive until work completes or the reap timeout expires. Register explicit pool disposal with host-service shutdown and app/test teardown.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At plans/20260717-host-service-git-worker-pool.md, line 53:
<comment>Idle reaping is not a shutdown lifecycle: a newly created or active worker can keep host-service tests and graceful process shutdown alive until work completes or the reap timeout expires. Register explicit pool disposal with host-service shutdown and app/test teardown.</comment>
<file context>
@@ -0,0 +1,154 @@
+
+## Pool changes vs the v1 runner
+
+1. **Idle reaping** (new): terminate workers idle > 30s. host-service is long-lived and per-org — a 3-org machine must not hold 12 warm workers. v1 never needed this because the pool lived in the single Electron main process.
+2. **Inline fallback** (new): if the worker script is missing or workers crash ≥3 times in 60s, log once and run tasks in-process (current behavior). Keeps standalone deployments and tests working if a bundle is malformed. Mirrors DaemonSupervisor's crash circuit.
+3. Everything else stays: fifo/coalesce/latest-wins, per-task timeout + worker recycle, abort.
</file context>
| | # | Finding | Severity | Fix | | ||
| |---|---|---|---| | ||
| | 1 | One flooding terminal destroys the org-wide daemon↔host-service socket (8 MiB cap, `Server.ts:595`); all terminals blip | Correctness bug, likely hitting users today | Per-session shedding in pty-daemon — **not** this pool | | ||
| | 2 | 8-workspace churn stalls the loop (183ms stalls, query p95 127ms) via the **watcher path**, not status parsing | Loop health | Offload GitWatcher rescan/filtering to the pool | |
Contributor
There was a problem hiding this comment.
P2: The proposed GitWatcher target is not a valid pool task as written: moving rescan/subscription work would violate the document's own DB/event-emitter boundary, while leaving those operations on the main thread may leave the measured 183 ms path untouched. Isolate a pure clone-safe scan/filter transform for the pool and keep watcher, DB, and event dispatch lifecycle on the main thread.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At plans/20260717-host-service-git-worker-pool.md, line 12:
<comment>The proposed GitWatcher target is not a valid pool task as written: moving rescan/subscription work would violate the document's own DB/event-emitter boundary, while leaving those operations on the main thread may leave the measured 183 ms path untouched. Isolate a pure clone-safe scan/filter transform for the pool and keep watcher, DB, and event dispatch lifecycle on the main thread.</comment>
<file context>
@@ -0,0 +1,154 @@
+| # | Finding | Severity | Fix |
+|---|---|---|---|
+| 1 | One flooding terminal destroys the org-wide daemon↔host-service socket (8 MiB cap, `Server.ts:595`); all terminals blip | Correctness bug, likely hitting users today | Per-session shedding in pty-daemon — **not** this pool |
+| 2 | 8-workspace churn stalls the loop (183ms stalls, query p95 127ms) via the **watcher path**, not status parsing | Loop health | Offload GitWatcher rescan/filtering to the pool |
+| 3 | Background `getStatus` takes 3.7–14.6s under churn — limiter queueing at concurrency 4 | UX staleness | Pool makes raising limiter concurrency safe |
+| 4 | Big-diff snapshot ≈1.2s (2k files) but does NOT stall the loop | Minor | Phase 1 offload, comes with #2/#3 infra |
</file context>
|
|
||
| ## What moves | ||
|
|
||
| **Phase 1 — status snapshot** (the win): `getGitStatusSnapshot` and everything inside it — `parseNumstat`, `countUntrackedFileLines` (reads + line-counts untracked files on the loop today), `detectUnstagedRenames`, `getChangedFilesForDiff`. Serves `git.getStatus`, and the git-watcher/PR-runtime refreshes that funnel through the limiter. |
Contributor
There was a problem hiding this comment.
P2: Moving everything inside getGitStatusSnapshot into the pool removes the current common-directory fetch deduplication across workers, so several workspaces in one repository can start concurrent or repeatedly scheduled git fetch processes. Keep scheduleBaseRefFetch behind a main-thread coordinator or otherwise centralize this shared per-repository state.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At plans/20260717-host-service-git-worker-pool.md, line 73:
<comment>Moving everything inside `getGitStatusSnapshot` into the pool removes the current common-directory fetch deduplication across workers, so several workspaces in one repository can start concurrent or repeatedly scheduled `git fetch` processes. Keep `scheduleBaseRefFetch` behind a main-thread coordinator or otherwise centralize this shared per-repository state.</comment>
<file context>
@@ -0,0 +1,154 @@
+
+## What moves
+
+**Phase 1 — status snapshot** (the win): `getGitStatusSnapshot` and everything inside it — `parseNumstat`, `countUntrackedFileLines` (reads + line-counts untracked files on the loop today), `detectUnstagedRenames`, `getChangedFilesForDiff`. Serves `git.getStatus`, and the git-watcher/PR-runtime refreshes that funnel through the limiter.
+
+**Phase 2 — read-path procs**: `getCommitFiles`, `listCommits`, `getBranchSyncStatus`, and `getDiff`'s `git.show` content reads (large-file drains + string assembly).
</file context>
|
|
||
| **Phase 2 — read-path procs**: `getCommitFiles`, `listCommits`, `getBranchSyncStatus`, and `getDiff`'s `git.show` content reads (large-file drains + string assembly). | ||
|
|
||
| **Phase 3 — opportunistic**: git-watcher's `ls-files`/rescan subscription work; PR-runtime per-workspace git interrogation, where not already sharing Phase 1 tasks. Future non-git tenants (each is just a new task module): diff/patch text assembly, search/indexing if host-service ever grows one, large JSON parse/serialize hot spots. |
Contributor
There was a problem hiding this comment.
P2: This phase assignment conflicts with the measured conclusion that watcher rescan/filtering is the main loop-stall source and should be promoted. Leaving it in Phase 3 risks delaying the highest-impact offload.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At plans/20260717-host-service-git-worker-pool.md, line 77:
<comment>This phase assignment conflicts with the measured conclusion that watcher rescan/filtering is the main loop-stall source and should be promoted. Leaving it in Phase 3 risks delaying the highest-impact offload.</comment>
<file context>
@@ -0,0 +1,154 @@
+
+**Phase 2 — read-path procs**: `getCommitFiles`, `listCommits`, `getBranchSyncStatus`, and `getDiff`'s `git.show` content reads (large-file drains + string assembly).
+
+**Phase 3 — opportunistic**: git-watcher's `ls-files`/rescan subscription work; PR-runtime per-workspace git interrogation, where not already sharing Phase 1 tasks. Future non-git tenants (each is just a new task module): diff/patch text assembly, search/indexing if host-service ever grows one, large JSON parse/serialize hot spots.
+
+**Not moving**: port-scanner (already subprocess-based, trivial parsing), reaper/cloud-sync/tunnel (I/O-bound), git mutations (`stageAll`, `discardChanges`, … — rare, fast, want the loop's ordering). Future candidate, out of scope: better-sqlite3 queries are synchronous on the loop; if profiling ever shows DB stalls, the same pool pattern applies.
</file context>
|
|
||
| ## Scheduling interplay | ||
|
|
||
| `gitStatusRefreshLimiter` remains the only queue for `getStatus` (per-workspace serialization, invalidation-coalescing, foreground/background priority). Pool concurrency = limiter concurrency (share `DEFAULT_CONCURRENCY` from `git-status-refresh-limiter.ts`) so pooled tasks never meaningfully queue behind the limiter. Limiter-fronted tasks use `fifo`; direct calls not behind the limiter (`getCommitFiles`, `getDiff`) use `coalesce` with a `workspaceId:proc:input` dedupe key. Procedure `timeoutMs` (15s/30s) carries into the pool task timeout. |
Contributor
There was a problem hiding this comment.
P2: Direct pool users bypass gitStatusRefreshLimiter priorities here, so a shared worker pool can be saturated by non-limiter jobs and delay status refreshes. Defining pool-level priority or reserved worker capacity for limiter-fronted tasks would protect status freshness under load.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At plans/20260717-host-service-git-worker-pool.md, line 59:
<comment>Direct pool users bypass `gitStatusRefreshLimiter` priorities here, so a shared worker pool can be saturated by non-limiter jobs and delay status refreshes. Defining pool-level priority or reserved worker capacity for limiter-fronted tasks would protect status freshness under load.</comment>
<file context>
@@ -0,0 +1,154 @@
+
+## Scheduling interplay
+
+`gitStatusRefreshLimiter` remains the only queue for `getStatus` (per-workspace serialization, invalidation-coalescing, foreground/background priority). Pool concurrency = limiter concurrency (share `DEFAULT_CONCURRENCY` from `git-status-refresh-limiter.ts`) so pooled tasks never meaningfully queue behind the limiter. Limiter-fronted tasks use `fifo`; direct calls not behind the limiter (`getCommitFiles`, `getDiff`) use `coalesce` with a `workspaceId:proc:input` dedupe key. Procedure `timeoutMs` (15s/30s) carries into the pool task timeout.
+
+## Bundling (3 surfaces, same side-by-side pattern as pty-daemon)
</file context>
|
|
||
| Known per-chunk work on the host-service loop besides git (found by audit, unmeasured): | ||
|
|
||
| - **ModeTracker headless xterm** — `terminal.ts:271` feeds *every PTY output chunk* through `@xterm/headless` `writeSync` (full VT parse, `terminal-mode-tracker.ts`), per session, synchronously. Under agent-TUI repaint churn × N terminals this is likely the largest steady-state CPU on the loop. |
Contributor
There was a problem hiding this comment.
P3: This line reference appears to point to a declaration rather than the per-chunk execution path. Updating it to the session.modeTracker.feed(bytes) call site (and optionally the terminal-mode-tracker.ts writeSync line) would make the finding directly actionable.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At plans/20260717-host-service-git-worker-pool.md, line 127:
<comment>This line reference appears to point to a declaration rather than the per-chunk execution path. Updating it to the `session.modeTracker.feed(bytes)` call site (and optionally the `terminal-mode-tracker.ts` `writeSync` line) would make the finding directly actionable.</comment>
<file context>
@@ -0,0 +1,154 @@
+
+Known per-chunk work on the host-service loop besides git (found by audit, unmeasured):
+
+- **ModeTracker headless xterm** — `terminal.ts:271` feeds *every PTY output chunk* through `@xterm/headless` `writeSync` (full VT parse, `terminal-mode-tracker.ts`), per session, synchronously. Under agent-TUI repaint churn × N terminals this is likely the largest steady-state CPU on the loop.
+- **Port-hint scanning** — per-chunk `StringDecoder` + text pattern match (`portHintDecoder` → `portManager.checkOutputForHint`).
+- **WS fan-out** — per-chunk broadcast copies to attached sockets.
</file context>
Suggested change
| - **ModeTracker headless xterm** — `terminal.ts:271` feeds *every PTY output chunk* through `@xterm/headless` `writeSync` (full VT parse, `terminal-mode-tracker.ts`), per session, synchronously. Under agent-TUI repaint churn × N terminals this is likely the largest steady-state CPU on the loop. | |
| - **ModeTracker headless xterm** — `terminal.ts:1237` feeds *every PTY output chunk* through `session.modeTracker.feed(bytes)`, which invokes `@xterm/headless` `writeSync` in `terminal-mode-tracker.ts` (e.g., `:123`), per session, synchronously. Under agent-TUI repaint churn × N terminals this is likely the largest steady-state CPU on the loop. |
| 2. `packages/host-service/build.ts` — add `src/workers/host-worker.ts` as a second entrypoint. | ||
| 3. `packages/cli/scripts/build-dist.ts` — copy `dist/host-worker.js` into `lib/` next to `host-service.js`. | ||
|
|
||
| Add a bundle-presence guard like `apps/desktop/scripts/check-pty-daemon-bundle.ts`. |
Contributor
There was a problem hiding this comment.
P3: A bundle-presence guard alone won’t catch “present but unloadable” worker artifacts. Adding a launchability check (for both desktop and CLI outputs) would catch packaging/ESM resolution failures before rollout.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At plans/20260717-host-service-git-worker-pool.md, line 69:
<comment>A bundle-presence guard alone won’t catch “present but unloadable” worker artifacts. Adding a launchability check (for both desktop and CLI outputs) would catch packaging/ESM resolution failures before rollout.</comment>
<file context>
@@ -0,0 +1,154 @@
+2. `packages/host-service/build.ts` — add `src/workers/host-worker.ts` as a second entrypoint.
+3. `packages/cli/scripts/build-dist.ts` — copy `dist/host-worker.js` into `lib/` next to `host-service.js`.
+
+Add a bundle-presence guard like `apps/desktop/scripts/check-pty-daemon-bundle.ts`.
+
+## What moves
</file context>
| hostWorkerPool.runTask("git/getStatusSnapshot", …) | ||
| │ postMessage | ||
| ▼ | ||
| host-worker.js (worker_threads, N = min(4, cpus−1)) |
Contributor
There was a problem hiding this comment.
P3: The diagram formula can produce N = 0 on single-core machines, which diverges from the documented concurrency floor used elsewhere. Align this expression to include a minimum of 1 worker to avoid contradictory sizing guidance.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At plans/20260717-host-service-git-worker-pool.md, line 33:
<comment>The diagram formula can produce `N = 0` on single-core machines, which diverges from the documented concurrency floor used elsewhere. Align this expression to include a minimum of 1 worker to avoid contradictory sizing guidance.</comment>
<file context>
@@ -0,0 +1,154 @@
+ hostWorkerPool.runTask("git/getStatusSnapshot", …)
+ │ postMessage
+ ▼
+ host-worker.js (worker_threads, N = min(4, cpus−1))
+ static registry: git/* today, other domains later
+```
</file context>
Suggested change
| host-worker.js (worker_threads, N = min(4, cpus−1)) | |
| host-worker.js (worker_threads, N = max(1, min(4, cpus−1))) |
| 1. **The loop stalls come from the watcher/event path, not status parsing.** Churn-8 stalls the loop to 183ms and drags query p95 to 127ms; heavy-burst (huge diffs, no fs churn) keeps the loop clean (p95 4.2ms) while statuses queue in the limiter + git subprocesses. GitWatcher event filtering/debounce/rescan is the strongest offload candidate for loop health — promote it from Phase 3. | ||
| 2. **User-visible status staleness under churn is limiter queueing.** Background refreshes take 3.7–14.6s at concurrency 4 with 8 hot workspaces. The worker pool makes raising that concurrency safe (in-process, more concurrent snapshots would multiply loop parse work; in workers it's just more cores). | ||
| 3. Snapshot cost scales with diff size (~1.2s at 2000 modified files) — worth offloading, but it wasn't the loop killer. | ||
| 4. Terminal I/O degrades 3–10× under churn (echo p50 1.8→5.7ms, p95 138ms during settle) — real but not catastrophic; consistent with conclusion 1. |
Contributor
There was a problem hiding this comment.
P3: The multiplier here doesn’t match the listed values: p95 goes from 3.7ms to 138ms (~37×). Consider scoping the 3–10× claim to churn-only metrics and calling out the post-reset settle spike separately.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At plans/20260717-host-service-git-worker-pool.md, line 121:
<comment>The multiplier here doesn’t match the listed values: p95 goes from 3.7ms to 138ms (~37×). Consider scoping the 3–10× claim to churn-only metrics and calling out the post-reset settle spike separately.</comment>
<file context>
@@ -0,0 +1,154 @@
+1. **The loop stalls come from the watcher/event path, not status parsing.** Churn-8 stalls the loop to 183ms and drags query p95 to 127ms; heavy-burst (huge diffs, no fs churn) keeps the loop clean (p95 4.2ms) while statuses queue in the limiter + git subprocesses. GitWatcher event filtering/debounce/rescan is the strongest offload candidate for loop health — promote it from Phase 3.
+2. **User-visible status staleness under churn is limiter queueing.** Background refreshes take 3.7–14.6s at concurrency 4 with 8 hot workspaces. The worker pool makes raising that concurrency safe (in-process, more concurrent snapshots would multiply loop parse work; in workers it's just more cores).
+3. Snapshot cost scales with diff size (~1.2s at 2000 modified files) — worth offloading, but it wasn't the loop killer.
+4. Terminal I/O degrades 3–10× under churn (echo p50 1.8→5.7ms, p95 138ms during settle) — real but not catastrophic; consistent with conclusion 1.
+
+## Measuring what else deserves offload
</file context>
Suggested change
| 4. Terminal I/O degrades 3–10× under churn (echo p50 1.8→5.7ms, p95 138ms during settle) — real but not catastrophic; consistent with conclusion 1. | |
| 4. Terminal I/O degrades ~3× under churn on p50 (echo p50 1.8→5.7ms), with a larger p95 spike during post-reset settle (3.7→138ms, ~37×) — real but not catastrophic; consistent with conclusion 1. |
Kitenite
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/host-service/src/events/git-watcher.ts (1)
387-396: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winRemove the flawed early-break condition to prevent dropped events.
The current break condition checks the total length of
relativePaths, which includes duplicate paths if the filesystem emits multiple events for the same file in a burst. IfrelativePaths.lengthexceeds the limit primarily due to duplicates, the loop breaks early and discards the rest of the batch.If the discarded events contain changes for other files, those changes are silently lost. Because the distinct path count (
batch.paths.size) never exceeded the limit, the batch will not trigger the broad invalidation fallback, resulting in an incompletegit:changedevent.Since
addWorktreePathsalready safely breaks early whenbatch.paths.size > MAX_WORKTREE_PATHS_PER_BATCH, removing this array-length check prevents the data loss.🐛 Proposed fix
const relativePaths: string[] = []; for (const event of next.value.events) { const rel = toRelative(event.absolutePath); if (rel) relativePaths.push(rel); if (event.oldAbsolutePath) { const oldRel = toRelative(event.oldAbsolutePath); if (oldRel) relativePaths.push(oldRel); } - if (relativePaths.length > MAX_WORKTREE_PATHS_PER_BATCH) break; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/host-service/src/events/git-watcher.ts` around lines 387 - 396, Remove the relativePaths.length-based break from the event loop in the git watcher. Continue processing every event in next.value.events, including oldAbsolutePath handling, and rely on addWorktreePaths’ batch.paths size limit for overflow behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/host-service/scripts/git-status-large-repo-profile.ts`:
- Around line 368-369: Update main’s cleanup flow around the shared workerPool
from getHostWorkerPool() to explicitly await workerPool.dispose() in a finally
block before the script exits, ensuring disposal occurs on both success and
failure while preserving the existing execution logic.
---
Outside diff comments:
In `@packages/host-service/src/events/git-watcher.ts`:
- Around line 387-396: Remove the relativePaths.length-based break from the
event loop in the git watcher. Continue processing every event in
next.value.events, including oldAbsolutePath handling, and rely on
addWorktreePaths’ batch.paths size limit for overflow behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 48971efa-a3ff-4586-be1f-2d0fbdb14844
📒 Files selected for processing (4)
packages/host-service/scripts/git-status-large-repo-profile.tspackages/host-service/src/events/git-watcher.test.tspackages/host-service/src/events/git-watcher.tsplans/20260717-host-service-git-worker-pool.md
🚧 Files skipped from review as they are similar to previous changes (1)
- plans/20260717-host-service-git-worker-pool.md
Comment on lines
+368
to
+369
| const workerPool = getHostWorkerPool(); | ||
| const executionMode = workerPool.getMode(); |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect the worker pool for teardown / unref / idle-reaping behavior.
fd -t f 'host-worker-pool.ts' packages/host-service/src/workers --exec ast-grep outline {} --items all
rg -nP 'terminate|unref|close|shutdown|idle|reap' packages/host-service/src/workers/host-worker-pool.ts -C2Repository: superset-sh/superset
Length of output: 2541
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- host-worker-pool.ts outline ---'
ast-grep outline packages/host-service/src/workers/host-worker-pool.ts --items all
echo
echo '--- host-worker-pool.ts dispose / idle handling ---'
sed -n '1,260p' packages/host-service/src/workers/host-worker-pool.ts
echo
echo '--- script tail around main() shutdown path ---'
sed -n '320,560p' packages/host-service/scripts/git-status-large-repo-profile.tsRepository: superset-sh/superset
Length of output: 17070
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- search for pool disposal / teardown ---'
rg -n "getHostWorkerPool\\(|\\.dispose\\(\\)" packages/host-service/scripts packages/host-service/src -g '!**/dist/**'
echo
echo '--- WorkerTaskRunner outline ---'
ast-grep outline packages/host-service/src/workers/WorkerTaskRunner.ts --items all
echo
echo '--- WorkerTaskRunner idle / unref / terminate handling ---'
rg -nP 'unref|terminate|idle|dispose|timer|ref\\(' packages/host-service/src/workers/WorkerTaskRunner.ts -C3
echo
echo '--- relevant slice of WorkerTaskRunner.ts ---'
sed -n '1,320p' packages/host-service/src/workers/WorkerTaskRunner.tsRepository: superset-sh/superset
Length of output: 5920
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- WorkerTaskRunner idle / terminate slice ---'
sed -n '80,260p' packages/host-service/src/workers/WorkerTaskRunner.ts
echo
echo '--- WorkerTaskRunner dispose / idle reaping slice ---'
sed -n '260,420p' packages/host-service/src/workers/WorkerTaskRunner.ts
echo
echo '--- git-status-large-repo-profile.ts ending ---'
tail -n 120 packages/host-service/scripts/git-status-large-repo-profile.tsRepository: superset-sh/superset
Length of output: 12962
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- worker thread ref/unref search ---'
rg -n '\.unref\(|\.ref\(' packages/host-service/src/workers/WorkerTaskRunner.ts packages/host-service/src/workers/host-worker-pool.ts
echo
echo '--- WorkerTaskRunner worker creation / termination context ---'
sed -n '420,620p' packages/host-service/src/workers/WorkerTaskRunner.ts
echo
echo '--- main() / shutdown context in script ---'
rg -n 'async function main|finally|dispose\(' packages/host-service/scripts/git-status-large-repo-profile.ts -C3Repository: superset-sh/superset
Length of output: 197
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- locate main() in script ---'
rg -n '^(async function main|await main\(\))' packages/host-service/scripts/git-status-large-repo-profile.ts -C 8
echo
echo '--- final section of script around main() ---'
sed -n '1,220p' packages/host-service/scripts/git-status-large-repo-profile.ts | tail -n 120
echo
echo '--- WorkerTaskRunner worker creation / termination slice ---'
sed -n '320,520p' packages/host-service/src/workers/WorkerTaskRunner.tsRepository: superset-sh/superset
Length of output: 10448
Dispose the shared worker pool before exit
getHostWorkerPool() leaves Worker threads alive here, and WorkerTaskRunner only unrefs the idle timer. Without an explicit await workerPool.dispose() in main()/finally, the script can stay up until the idle reaper terminates the workers.
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawn } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/host-service/scripts/git-status-large-repo-profile.ts` around lines
368 - 369, Update main’s cleanup flow around the shared workerPool from
getHostWorkerPool() to explicitly await workerPool.dispose() in a finally block
before the script exits, ensuring disposal occurs on both success and failure
while preserving the existing execution logic.
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/host-service/src/events/git-watcher.ts (1)
408-418: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winHandle watcher overflows and avoid broad refreshes for filtered events.
The current implementation in
startWorktreeWatchhas two functional correctness issues that degrade reliability and performance:
- Dropped
overflowevents:FsWatchEvents of kind"overflow"are treated as normal file paths and passed tocollectWorktreeBatchPaths. Because they often lack a valid relative path, they get filtered out, meaning a broad refresh is not triggered when the watcher drops events. This leaves thegit statuscache permanently stale for the lost events.- Expensive fake broad refreshes: The
elseblock schedules a flush even when all events were intentionally filtered out (e.g., rare.git/directory leaks). BecauseGitChangedEventdefaults to a broad refresh whenbatch.paths.size === 0, every ignored leak inadvertently triggers a full worktree invalidation!Handle
"overflow"explicitly by switching to the broad invalidation sentinel (paths = null), and remove theelseblock so filtered events are safely ignored.🐛 Proposed fix
- const relativePaths = collectWorktreeBatchPaths( - next.value.events, - worktreePath, - ); - - if (relativePaths.size > 0) { - this.addWorktreePaths(workspaceId, relativePaths); - } else { - this.getOrCreateBatch(workspaceId); - this.scheduleFlush(workspaceId); - } + if (next.value.events.some((e) => e.kind === "overflow")) { + const batch = this.getOrCreateBatch(workspaceId); + batch.paths = null; + this.scheduleFlush(workspaceId); + continue; + } + + const relativePaths = collectWorktreeBatchPaths( + next.value.events, + worktreePath, + ); + + if (relativePaths.size > 0) { + this.addWorktreePaths(workspaceId, relativePaths); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/host-service/src/events/git-watcher.ts` around lines 408 - 418, Update startWorktreeWatch to detect any FsWatchEvent with kind "overflow" before calling collectWorktreeBatchPaths and use the broad-invalidation sentinel paths = null for that batch. Remove the else branch that calls getOrCreateBatch and scheduleFlush, so batches containing only filtered events are ignored without triggering a broad refresh.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/host-service/src/events/git-watcher.ts`:
- Around line 408-418: Update startWorktreeWatch to detect any FsWatchEvent with
kind "overflow" before calling collectWorktreeBatchPaths and use the
broad-invalidation sentinel paths = null for that batch. Remove the else branch
that calls getOrCreateBatch and scheduleFlush, so batches containing only
filtered events are ignored without triggering a broad refresh.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 23441ab6-bb75-4f1c-9ac3-3468fd0199d1
📒 Files selected for processing (2)
packages/host-service/src/events/git-watcher.test.tspackages/host-service/src/events/git-watcher.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/host-service/src/events/git-watcher.test.ts
This branch was previously deployed
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.
Summary
Why
The worker pool moved Git status parsing off the host-service event loop, but large native watcher batches still performed unbounded path normalization and Set growth on the main thread. Profiling showed that adding another worker hop increased latency because of structured-clone and pool-contention costs. Bounded broad invalidation preserves correctness while removing the sustained watcher-loop pressure.
Impact
In representative eight-worktree profiling, watcher-only p99 event-loop delay dropped from 87.2 ms to 13.6 ms. The full watcher plus worker-backed status run dropped from 43.5 ms to 6.3 ms p99 and from 7.91 s to 5.34 s. Concurrency 6 did not improve completion time and increased active Git processes, so the production limiter remains at 4.
Validation
bun run lint:fixbun run lintbun run --cwd packages/host-service typecheckbun run --cwd packages/host-service build:hostSummary by CodeRabbit
--workspaces,--watcher-only) and expanded worker/inline execution parity and worker behavior coverage.