fix(host-service): isolate subsystem crashes from main thread#3811
Merged
Conversation
Subsystem throws (timer callbacks, EventEmitter listeners, pty data/exit handlers, harness subscribe callback) used to escape into the process and could take the whole host-service down. Now they're contained per-subsystem and a process-level safety net catches anything that still slips through. - Add safety.ts with installProcessSafetyNet (uncaughtException + unhandledRejection log without exiting) and safeSync/safeAsync helpers for wrapping callbacks. - Wrap hot async entry points in EventBus, GitWatcher, PullRequestRuntimeManager, ChatRuntimeManager, and terminal PTY callbacks. - Surface the previously-silent malformed-message swallow in EventBus.parseClientMessage. Startup exit on init failure is intentionally preserved so the coordinator / external supervisor can restart with a clean process.
Contributor
📝 WalkthroughWalkthroughAdds a process-level safety net and defensive error handling: installs idempotent handlers for uncaught exceptions/unhandled rejections, defers safety-net installation until the HTTP server is listening, and isolates exceptions in event parsing, broadcast fan-out, and git-watcher listeners. Changes
Sequence DiagramsequenceDiagram
participant Desktop as Desktop Main
participant Serve as HTTP Serve
participant Process as Host Process
participant Logger as Console
Desktop->>Serve: start host-service
Serve->>Serve: create & bind HTTP server
Serve->>Serve: server.listen callback
Serve->>Process: installProcessSafetyNet()
Process->>Process: register uncaughtException handler
Process->>Process: register unhandledRejection handler
Note over Process,Logger: runtime continues
alt Exception or Rejection
Process->>Logger: log structured incident
Logger-->>Process: formatted output
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Greptile SummaryThis PR adds crash isolation to the host-service by introducing a Confidence Score: 5/5Safe to merge — all findings are P2 style/observability suggestions with no correctness or reliability impact. The core design is correct: per-subsystem guards prevent throw propagation, the process-level backstop covers anything that slips through, idempotency is handled, and startup exit semantics are intentionally preserved. The three comments are all P2 (misleading variable name, per-iteration wrapper allocation, low-value promise log field) and none affect runtime behavior. No files require special attention beyond the three P2 style notes.
|
| Filename | Overview |
|---|---|
| packages/host-service/src/safety.ts | New file introducing installProcessSafetyNet, safeSync, and safeAsync; idempotency guard and structured logging are correct, minor logging improvement possible for unhandledRejection |
| packages/host-service/src/runtime/pull-requests/pull-requests.ts | Interval callbacks wrapped with safeAsync; local variable named safeSync clashes semantically with the sync utility, potentially confusing future maintainers |
| packages/host-service/src/events/event-bus.ts | Git-changed listener, port add/remove handlers, and per-socket broadcast all wrapped with safeSync; wrapper is recreated on every broadcast iteration (minor allocation concern) |
| packages/host-service/src/events/git-watcher.ts | Rescan interval and debounce-flush timer now catch and log rejections; per-listener invocation isolated so one bad subscriber cannot block others |
| packages/host-service/src/terminal/terminal.ts | pty.onData and pty.onExit wrapped with safeSync; shellReadyPromise.then() now has an explicit .catch() — correct trade-off accepting possible partial state on error over a full process crash |
| packages/host-service/src/runtime/chat/chat.ts | harness.subscribe callback wrapped with safeSync; purely structural, no logic changes |
| packages/host-service/src/serve.ts | installProcessSafetyNet called at the top of main() before any async work; correct placement |
| apps/desktop/src/main/host-service/index.ts | installProcessSafetyNet wired into the desktop entrypoint; idempotency guard ensures only one set of handlers is registered even if both entrypoints run in the same process |
| packages/host-service/src/index.ts | Re-exports installProcessSafetyNet so the desktop package can import it without reaching into internals |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Process start] --> B[installProcessSafetyNet]
B --> C{safetyNetInstalled?}
C -- yes --> D[no-op]
C -- no --> E[Register uncaughtException handler\nlog + stay up]
E --> F[Register unhandledRejection handler\nlog + stay up]
subgraph Subsystems
G[GitWatcher\nrescan interval / debounce flush / listener fan-out]
H[EventBus\ngit:changed / port add-remove / per-socket broadcast]
I[PullRequestRuntimeManager\nbranch-sync / project-refresh intervals]
J[ChatRuntimeManager\nharness.subscribe callback]
K[Terminal PTY\nonData / onExit / shellReady.then]
end
G -- safeSync/catch --> L[log '— contained'\nreturn undefined]
H -- safeSync --> L
I -- safeAsync --> L
J -- safeSync --> L
K -- safeSync/catch --> L
L -.->|escapes all guards| E
Prompt To Fix All With AI
This is a comment left during a code review.
Path: packages/host-service/src/runtime/pull-requests/pull-requests.ts
Line: 212-218
Comment:
**Misleading local variable name shadows the async wrapper**
The local variable is named `safeSync` but it is produced by the `safeAsync` utility and wraps an async function. This directly conflicts with the `safeSync` export from `safety.ts` and will confuse future readers who expect `safeSync` to mean "synchronous, no-await wrapper". Rename it to something unambiguous like `safeBranchSync`.
```suggestion
const safeBranchSync = safeAsync("pull-requests:branch-sync", () =>
this.syncWorkspaceBranches(),
);
const safeRefresh = safeAsync(
"pull-requests:project-refresh",
(force?: boolean) => this.refreshEligibleProjects(force),
);
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: packages/host-service/src/events/event-bus.ts
Line: 149-152
Comment:
**Wrapper recreated on every broadcast call and every socket iteration**
`safeSync("event-bus:send", sendMessage)` constructs a new closure each time `broadcast` is called for every socket in `this.clients`. The wrapped function is stateless and identical on every call — hoist it once (e.g. as a private field or a module constant) to avoid the repeated allocation.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: packages/host-service/src/safety.ts
Line: 27-32
Comment:
**`promise` object has low diagnostic value in the unhandledRejection log**
The `promise` argument in an `unhandledRejection` handler serialises as `Promise { <rejected> }` and carries no information beyond what `reason` already provides. Consider dropping it from the structured log.
```suggestion
process.on("unhandledRejection", (reason) => {
console.error(`[${label}] unhandledRejection — staying up`, {
reason,
});
});
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix(host-service): isolate subsystem cra..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+212
to
+218
| const safeSync = safeAsync("pull-requests:branch-sync", () => | ||
| this.syncWorkspaceBranches(), | ||
| ); | ||
| const safeRefresh = safeAsync( | ||
| "pull-requests:project-refresh", | ||
| (force?: boolean) => this.refreshEligibleProjects(force), | ||
| ); |
There was a problem hiding this comment.
The local variable is named safeSync but it is produced by the safeAsync utility and wraps an async function. This directly conflicts with the safeSync export from safety.ts and will confuse future readers who expect safeSync to mean "synchronous, no-await wrapper". Rename it to something unambiguous like safeBranchSync.
Suggested change
| const safeSync = safeAsync("pull-requests:branch-sync", () => | |
| this.syncWorkspaceBranches(), | |
| ); | |
| const safeRefresh = safeAsync( | |
| "pull-requests:project-refresh", | |
| (force?: boolean) => this.refreshEligibleProjects(force), | |
| ); | |
| const safeBranchSync = safeAsync("pull-requests:branch-sync", () => | |
| this.syncWorkspaceBranches(), | |
| ); | |
| const safeRefresh = safeAsync( | |
| "pull-requests:project-refresh", | |
| (force?: boolean) => this.refreshEligibleProjects(force), | |
| ); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/host-service/src/runtime/pull-requests/pull-requests.ts
Line: 212-218
Comment:
**Misleading local variable name shadows the async wrapper**
The local variable is named `safeSync` but it is produced by the `safeAsync` utility and wraps an async function. This directly conflicts with the `safeSync` export from `safety.ts` and will confuse future readers who expect `safeSync` to mean "synchronous, no-await wrapper". Rename it to something unambiguous like `safeBranchSync`.
```suggestion
const safeBranchSync = safeAsync("pull-requests:branch-sync", () =>
this.syncWorkspaceBranches(),
);
const safeRefresh = safeAsync(
"pull-requests:project-refresh",
(force?: boolean) => this.refreshEligibleProjects(force),
);
```
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!
Comment on lines
+149
to
+152
|
|
||
| private broadcast(message: ServerMessage): void { | ||
| for (const socket of this.clients.keys()) { | ||
| sendMessage(socket, message); | ||
| // One bad socket must not block the rest of the fan-out. |
There was a problem hiding this comment.
safeSync("event-bus:send", sendMessage) constructs a new closure each time broadcast is called for every socket in this.clients. The wrapped function is stateless and identical on every call — hoist it once (e.g. as a private field or a module constant) to avoid the repeated allocation.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/host-service/src/events/event-bus.ts
Line: 149-152
Comment:
**Wrapper recreated on every broadcast call and every socket iteration**
`safeSync("event-bus:send", sendMessage)` constructs a new closure each time `broadcast` is called for every socket in `this.clients`. The wrapped function is stateless and identical on every call — hoist it once (e.g. as a private field or a module constant) to avoid the repeated allocation.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+27
to
+32
| process.on("unhandledRejection", (reason, promise) => { | ||
| console.error(`[${label}] unhandledRejection — staying up`, { | ||
| reason, | ||
| promise, | ||
| }); | ||
| }); |
There was a problem hiding this comment.
promise object has low diagnostic value in the unhandledRejection log
The promise argument in an unhandledRejection handler serialises as Promise { <rejected> } and carries no information beyond what reason already provides. Consider dropping it from the structured log.
Suggested change
| process.on("unhandledRejection", (reason, promise) => { | |
| console.error(`[${label}] unhandledRejection — staying up`, { | |
| reason, | |
| promise, | |
| }); | |
| }); | |
| process.on("unhandledRejection", (reason) => { | |
| console.error(`[${label}] unhandledRejection — staying up`, { | |
| reason, | |
| }); | |
| }); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/host-service/src/safety.ts
Line: 27-32
Comment:
**`promise` object has low diagnostic value in the unhandledRejection log**
The `promise` argument in an `unhandledRejection` handler serialises as `Promise { <rejected> }` and carries no information beyond what `reason` already provides. Consider dropping it from the structured log.
```suggestion
process.on("unhandledRejection", (reason) => {
console.error(`[${label}] unhandledRejection — staying up`, {
reason,
});
});
```
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.
🧹 Nitpick comments (4)
packages/host-service/src/runtime/pull-requests/pull-requests.ts (1)
212-218: Local variable namesafeSyncshadows/confuses with the importedsafeSynchelper.The
safeSyncname is the well-known sync wrapper from../../safetyused elsewhere in this PR (e.g., event-bus.ts, git-watcher.ts), but here it's bound to asafeAsync(...)result that returns a Promise. Anyone scanning this file may misread the call-site at line 220-221 as synchronous. Suggest renaming to reflect what it does (e.g.,safeBranchSync).♻️ Suggested rename
- const safeSync = safeAsync("pull-requests:branch-sync", () => - this.syncWorkspaceBranches(), - ); - const safeRefresh = safeAsync( + const safeBranchSync = safeAsync("pull-requests:branch-sync", () => + this.syncWorkspaceBranches(), + ); + const safeRefresh = safeAsync( "pull-requests:project-refresh", (force?: boolean) => this.refreshEligibleProjects(force), ); this.branchSyncTimer = setInterval(() => { - void safeSync(); + void safeBranchSync(); }, BRANCH_SYNC_INTERVAL_MS); this.projectRefreshTimer = setInterval(() => { void safeRefresh(); }, PROJECT_REFRESH_INTERVAL_MS); - void safeSync(); + void safeBranchSync(); void safeRefresh(true);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/host-service/src/runtime/pull-requests/pull-requests.ts` around lines 212 - 218, The local variable safeSync shadows the imported safeSync helper and is actually a Promise-producing wrapper created via safeAsync; rename the local variable (e.g., safeBranchSync) to avoid confusion and clarify intent where it's used, update its declaration that calls safeAsync("pull-requests:branch-sync", () => this.syncWorkspaceBranches()) and any subsequent references/invocations, and optionally do the same renaming pattern for consistency with safeRefresh to make clear both are async wrappers.packages/host-service/src/events/event-bus.ts (1)
150-155: Hoist thesafeSyncwrapper outside the per-socket loop.
broadcastruns on every fan-out event (git/port/agent/terminal lifecycle), and each iteration allocates a fresh wrapper closure. Construct it once per call:♻️ Proposed change
private broadcast(message: ServerMessage): void { + const safeSend = safeSync("event-bus:send", sendMessage); for (const socket of this.clients.keys()) { // One bad socket must not block the rest of the fan-out. - safeSync("event-bus:send", sendMessage)(socket, message); + safeSend(socket, message); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/host-service/src/events/event-bus.ts` around lines 150 - 155, In broadcast, avoid allocating the safeSync wrapper per socket: inside the broadcast method (which currently calls safeSync("event-bus:send", sendMessage)(socket, message) in the loop) create the wrapped sender once (e.g., const safeSend = safeSync("event-bus:send", sendMessage)) and then call safeSend(socket, message) for each socket in this.clients.keys(); this preserves the same error-isolation behavior while eliminating per-iteration closure allocation.packages/host-service/src/safety.ts (2)
41-74: Optional: consider preserving a referentially stablenameon the wrapped callback.
safeSync/safeAsyncreturn anonymous arrow functions, which means downstream code that introspects.name(e.g., for diagnostics, EventEmitterlistenerCount/removeListenerparity, or pty/harness.subscribeunsubscription that compares function identity) gets back an unnamed wrapper. Identity-based unsubscription still works because callers retain the wrapper reference, but if any subsystem unsubscribes via name/string lookup or logs the listener name, this will read as empty.If that's not a concern in any current call site, feel free to ignore.
♻️ Optional refinement
export function safeSync<Args extends unknown[], R>( label: string, fn: (...args: Args) => R, ): (...args: Args) => R | undefined { - return (...args: Args) => { + const wrapped = (...args: Args) => { try { return fn(...args); } catch (error) { console.error(`[${label}] callback threw — contained`, { error }); return undefined; } }; + Object.defineProperty(wrapped, "name", { value: `safeSync(${label})` }); + return wrapped; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/host-service/src/safety.ts` around lines 41 - 74, safeSync and safeAsync return anonymous wrappers which lose the original callback's .name; update both functions (safeSync and safeAsync) so after creating the wrapper function you set a stable name on it (e.g., derive from the original fn.name and/or label) via Object.defineProperty(wrapper, "name", { value: desiredName, configurable: true }) before returning, preserving referential behavior and ensuring the wrapper still returns the same types and error-handling semantics.
27-32: Minor:unhandledRejectionlog includes thepromiseobject.Logging the
promisereference adds little diagnostic value beyondreasonand, if a JSON-formatting log transport is layered on top ofconsole.errorlater, may produce noisy/circular output. Thereason(typically the rejection error) pluslabel/origin is usually sufficient. Not blocking —console.errorvia Node's defaultutil.inspecthandles this fine today.♻️ Optional simplification
- process.on("unhandledRejection", (reason, promise) => { - console.error(`[${label}] unhandledRejection — staying up`, { - reason, - promise, - }); - }); + process.on("unhandledRejection", (reason) => { + console.error(`[${label}] unhandledRejection — staying up`, { reason }); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/host-service/src/safety.ts` around lines 27 - 32, The unhandledRejection handler currently logs both reason and the promise object via process.on("unhandledRejection", ...) which can produce noisy or circular output; update the handler to stop including the promise in the console.error payload and log only the reason (and label/origin) — e.g. call console.error or your logger with the descriptive message and the rejection reason (use the existing label variable) and remove the promise field from the logged object so only useful diagnostic data is emitted.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/host-service/src/events/event-bus.ts`:
- Around line 150-155: In broadcast, avoid allocating the safeSync wrapper per
socket: inside the broadcast method (which currently calls
safeSync("event-bus:send", sendMessage)(socket, message) in the loop) create the
wrapped sender once (e.g., const safeSend = safeSync("event-bus:send",
sendMessage)) and then call safeSend(socket, message) for each socket in
this.clients.keys(); this preserves the same error-isolation behavior while
eliminating per-iteration closure allocation.
In `@packages/host-service/src/runtime/pull-requests/pull-requests.ts`:
- Around line 212-218: The local variable safeSync shadows the imported safeSync
helper and is actually a Promise-producing wrapper created via safeAsync; rename
the local variable (e.g., safeBranchSync) to avoid confusion and clarify intent
where it's used, update its declaration that calls
safeAsync("pull-requests:branch-sync", () => this.syncWorkspaceBranches()) and
any subsequent references/invocations, and optionally do the same renaming
pattern for consistency with safeRefresh to make clear both are async wrappers.
In `@packages/host-service/src/safety.ts`:
- Around line 41-74: safeSync and safeAsync return anonymous wrappers which lose
the original callback's .name; update both functions (safeSync and safeAsync) so
after creating the wrapper function you set a stable name on it (e.g., derive
from the original fn.name and/or label) via Object.defineProperty(wrapper,
"name", { value: desiredName, configurable: true }) before returning, preserving
referential behavior and ensuring the wrapper still returns the same types and
error-handling semantics.
- Around line 27-32: The unhandledRejection handler currently logs both reason
and the promise object via process.on("unhandledRejection", ...) which can
produce noisy or circular output; update the handler to stop including the
promise in the console.error payload and log only the reason (and label/origin)
— e.g. call console.error or your logger with the descriptive message and the
rejection reason (use the existing label variable) and remove the promise field
from the logged object so only useful diagnostic data is emitted.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 86716a87-d1a9-442b-b99f-b5acfc42e524
📒 Files selected for processing (9)
apps/desktop/src/main/host-service/index.tspackages/host-service/src/events/event-bus.tspackages/host-service/src/events/git-watcher.tspackages/host-service/src/index.tspackages/host-service/src/runtime/chat/chat.tspackages/host-service/src/runtime/pull-requests/pull-requests.tspackages/host-service/src/safety.tspackages/host-service/src/serve.tspackages/host-service/src/terminal/terminal.ts
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.
1 issue found across 9 files
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="apps/desktop/src/main/host-service/index.ts">
<violation number="1" location="apps/desktop/src/main/host-service/index.ts:26">
P1: Installing the process safety net before startup completes changes failure semantics: uncaught startup errors can be absorbed and the process may continue in a bad state. Install this only after the server has successfully started listening.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
- Install process safety net only after server is listening so startup throws still reach main().catch and exit non-zero. - Rename misleading `safeSync` local in pull-requests.ts to `runBranchSync`/`runProjectRefresh` — it wraps an async fn. - Hoist `safeSync(event-bus:send, sendMessage)` to module scope to avoid per-broadcast-per-socket closure allocation. - Use `safeAsync` in git-watcher rescan for consistency with the pull-requests interval pattern. - Drop low-value `promise` field from unhandledRejection log.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/host-service/src/events/git-watcher.ts`:
- Around line 83-87: The initial rescan in GitWatcher.start is wrapped in
safeAsync (runRescan = safeAsync("git-watcher:rescan", () => this.rescan()))
which swallows startup exceptions before installProcessSafetyNet is installed;
change the startup behavior so startup errors propagate: either move the call to
EventBus.start() so it's invoked after installProcessSafetyNet() is applied (so
the existing safeAsync is fine), or call this.rescan() directly (await or return
its Promise) for the first run without wrapping it in safeAsync so exceptions
bubble to main().catch(); keep the repeating setInterval using safeAsync as-is.
🪄 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: 8f862085-766a-440e-98af-7bace077d7d6
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
apps/desktop/src/main/host-service/index.tspackages/host-service/src/events/event-bus.tspackages/host-service/src/events/git-watcher.tspackages/host-service/src/runtime/pull-requests/pull-requests.tspackages/host-service/src/safety.tspackages/host-service/src/serve.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/desktop/src/main/host-service/index.ts
- packages/host-service/src/serve.ts
- packages/host-service/src/runtime/pull-requests/pull-requests.ts
- packages/host-service/src/events/event-bus.ts
…-out guards Drop safeSync/safeAsync wrappers entirely. The Node process-level handlers already catch throws from setInterval/setTimeout bodies, EventEmitter listeners, native pty callbacks, and orphaned promise continuations — so wrapping each one individually was redundant. The two cases that genuinely need inline error handling are loops over multiple subscribers, where a throw skips the rest of the iteration: - EventBus.broadcast — wrap per-socket send and drop dead sockets, matching the opencode pty fan-out pattern. Converts "log forever per broadcast" into "one log + clean state". - GitWatcher.scheduleFlush listener loop — inline try/catch so one bad subscriber can't skip siblings. Aligns with the field norm (opencode, tabby, hyper, mastra-code-ui all rely on process-level handlers + targeted inline guards rather than per-seam wrappers). Net -68 lines.
…nterval - installProcessSafetyNet had a default label arg that both call sites passed identically; drop the param and inline "host-service" in the logs. - git-watcher.ts start() reverts to its original setInterval one-liner shape (the multi-line form was leftover from an earlier refactor).
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/host-service/src/events/git-watcher.ts (1)
81-85:⚠️ Potential issue | 🟠 MajorInitial rescan is still detached from startup failure flow.
At Line 82,
void this.rescan()starts initialization work without awaiting it, so bootstrap cannot deterministically fail on first-scan errors. That keeps this path outside startup error propagation.Suggested fix
- start(): void { - void this.rescan(); + async start(): Promise<void> { + await this.rescan(); this.rescanTimer = setInterval(() => { void this.rescan(); }, RESCAN_INTERVAL_MS); }If you take this route, call sites should
await gitWatcher.start()during startup.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/host-service/src/events/git-watcher.ts` around lines 81 - 85, The start() method currently calls this.rescan() without awaiting it, so initialization errors are detached from startup; change start() to be async (or return a Promise) and await this.rescan() so the first scan errors propagate to bootstrap, then set this.rescanTimer = setInterval(() => void this.rescan().catch(err => /* log/handle */), RESCAN_INTERVAL_MS) so subsequent scans don't crash the process; update any callers to await gitWatcher.start() during startup.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/host-service/src/events/git-watcher.ts`:
- Around line 81-85: The start() method currently calls this.rescan() without
awaiting it, so initialization errors are detached from startup; change start()
to be async (or return a Promise) and await this.rescan() so the first scan
errors propagate to bootstrap, then set this.rescanTimer = setInterval(() =>
void this.rescan().catch(err => /* log/handle */), RESCAN_INTERVAL_MS) so
subsequent scans don't crash the process; update any callers to await
gitWatcher.start() during startup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1f172284-2cc1-497e-856e-720a0aa2c104
📒 Files selected for processing (3)
packages/host-service/src/events/event-bus.tspackages/host-service/src/events/git-watcher.tspackages/host-service/src/safety.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/host-service/src/events/event-bus.ts
This was referenced May 8, 2026
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
1 participant
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
The host-service hosts long-running subsystems (EventBus, GitWatcher, PullRequestRuntimeManager, ChatRuntimeManager, terminal PTYs). A throw inside any of their async entry points —
setIntervalbodies, EventEmitter listeners,pty.onData/onExit, harness subscribe callbacks — used to escape into the process and could take the whole service down.This PR adds a process-level safety net plus targeted inline guards at the two real cascade-failure points (fan-out loops). The pattern matches what equivalent apps do — opencode, tabby, hyper, mastra-code-ui all rely on a process-level handler + minimal inline guards rather than wrapping every callback.
What's added
packages/host-service/src/safety.ts— new module exportinginstallProcessSafetyNet(), which registersuncaughtException+unhandledRejectionhandlers that log and stay up. Idempotent (safe to call from both entry points).packages/host-service/src/serve.ts,apps/desktop/src/main/host-service/index.ts) inside theserve()listening callback — so startup throws still propagate tomain().catch → process.exit(1)and the supervisor can restart with a clean process.EventBus.broadcast— wraps per-socket send intry/catchand drops dead sockets (matching the opencode pty fan-out pattern). Converts "log every broadcast forever" into "one log + clean state."GitWatcherlistener loop — inlinetry/catcharound each listener invocation so one bad subscriber can't skip siblings.EventBus.parseClientMessage— replaces silent malformed-message swallow withconsole.warnso caught errors are visible.Why this approach
Node already routes throws from
setInterval/setTimeoutbodies,EventEmitterlisteners, native callbacks (pty.onData/onExit), and orphaned promise continuations intouncaughtException/unhandledRejection. A single process-level handler covers all of those. The only places where wrapping genuinely earns its keep are loops over multiple subscribers, where a throw skips the rest of the iteration.What's intentionally not changed
Startup
process.exit(1)is preserved. The "never exit" goal applies to runtime — once the server is listening, subsystem crashes must not propagate. Startup exit is the supervisor's job: the desktop coordinator polls health and surfaces failures, and CLI deploys run under an external supervisor (systemd/fly) that restarts with a clean process.Test plan
bun --filter @superset/host-service typecheckbun --filter @superset/desktop typecheckcd packages/host-service && bun test— 180/180 passbun run lint:fix— cleanpty.onData— terminal session continues, other sessions unaffected (relies on process net)