Skip to content

Host service#3157

Merged
Kitenite merged 18 commits into
mainfrom
decorous-salesman
Apr 4, 2026
Merged

Host service#3157
Kitenite merged 18 commits into
mainfrom
decorous-salesman

Conversation

@Kitenite
Copy link
Copy Markdown
Collaborator

@Kitenite Kitenite commented Apr 3, 2026

Description

Related Issues

Type of Change

  • Bug fix
  • New feature
  • Documentation
  • Refactor
  • Other (please describe):

Testing

Screenshots (if applicable)

Additional Notes


Summary by cubic

Adds a durable per‑organization host service that survives UI quits via on‑disk manifests, plus a tray to monitor/manage instances, terminal auto‑reconnect, and version/protocol compatibility so work persists across restarts and updates. Also includes architecture/boundaries/lifecycle docs to make the host service deployable standalone without Electron.

  • New Features

    • Durability/adoption: per‑org manifest (pid/endpoint/auth/serviceVersion/protocolVersion/startedAt/org) with KEEP_ALIVE_AFTER_PARENT; adopted on app start with PID + endpoint health checks; stale/corrupted manifests cleaned; incompatible instances killed and respawned; update‑available vs pending‑restart surfaced.
    • Versioning: desktop owns serviceVersion (app.getVersion()), passes it to the host service and manifests; introduced HOST_SERVICE_PROTOCOL_VERSION with compatibility checks.
    • TRPC: getLocalPort accepts organizationName; added getServiceInfo, restart, and onStatusChange subscription. Health/info includes serviceVersion, protocolVersion, organizationId, and startedAt. Host REST: GET /terminal/sessions and DELETE /terminal/sessions/:terminalId.
    • Tray: per‑org entries show name, status, version, uptime, restarts, and update/pending‑restart hints; actions include Restart/Stop. Top‑level adds “Open Superset,” “Settings,” “Check for Updates,” “Quit (Keep Services Running),” and “Quit & Stop Services.”
    • Terminal: WS transport auto‑reconnects with backoff on unexpected closes and skips reconnect after explicit exit.
    • Docs: added HOST_SERVICE_LIFECYCLE.md, HOST_SERVICE_ARCHITECTURE.md, and HOST_SERVICE_BOUNDARIES.md in apps/desktop.
  • Refactors

    • Lifecycle: “Quit (Keep Services Running)” now destroys windows but keeps the tray alive; macOS hides windows on close and on implicit quit when services are active; Windows/Linux quit the UI while services persist via releaseAll() and are adopted on next launch. Added requestQuit("release"|"stop"), prepareQuit, and exitImmediately; auto‑updater calls prepareQuit("release") and triggers checkAllCompatibility() after download.
    • Host‑service manager: new statuses (starting/running/degraded/restarting/stopped) and status events; adoption from on‑disk manifests with periodic liveness checks of adopted PIDs; exponential‑backoff restarts; discoverAndAdoptAll(), releaseAll(), getServiceInfo(), and checkAllCompatibility() to surface versions/uptime and flag incompatible or restart‑pending instances.

Written for commit 00343d6. Summary will update on new commits.

Summary by CodeRabbit

  • New Features

    • Host service management in the system tray with per-organization status, uptime, Restart and Stop actions
    • Host services now persist/adopt previous runs and surface version/protocol info
    • Local port lookups include organization names; host service health reports include version and start time
  • Bug Fixes

    • Terminal sessions auto-reconnect with exponential backoff after disconnects
    • More reliable shutdown/restart flows and improved host-service recovery and compatibility checks

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 3, 2026

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Adds persistent host-service manifests, a discovery/adoption lifecycle in HostServiceManager (with richer status/compatibility), new tRPC procedures/subscriptions, host-service-aware auto-update checks, quit/window lifecycle refactor, tray integration for host services, terminal transport auto-reconnect, and new REST session endpoints.

Changes

Cohort / File(s) Summary
Host Service Manifest Persistence
apps/desktop/src/main/lib/host-service-manifest.ts
New filesystem-backed manifest API: write/read/list/remove manifests per organization, manifest schema, and process liveness helper.
Host Service Manager Core
apps/desktop/src/main/lib/host-service-manager.ts, apps/desktop/src/main/lib/host-service-manager.test.ts
Converted manager to EventEmitter; added protocol/version exports and compatibility checks; deduplicated concurrent starts; adopt-or-spawn flow from manifests; periodic adopted-liveness polling; new status values (degraded, restarting, stopped); added methods (releaseAll, discoverAndAdoptAll, restart, getServiceInfo, etc.); tests updated/expanded.
Host-Service Entrypoint & Manifest Use
apps/desktop/src/main/host-service/index.ts
Entry reads/passes HOST_SERVICE_VERSION and HOST_SERVICE_PROTOCOL_VERSION, writes manifest on ready (includes startedAt), conditionally removes manifest on shutdown, and supports KEEP_ALIVE_AFTER_PARENT to disable parent-death cleanup.
tRPC Router & Context
apps/desktop/src/lib/trpc/routers/host-service-manager/index.ts, packages/host-service/src/app.ts, packages/host-service/src/types.ts, packages/host-service/src/trpc/router/health/health.ts
Router switched to registry; getLocalPort accepts optional organizationName; added getServiceInfo query, restart mutation, and onStatusChange subscription; app/context and types now carry optional serviceVersion and protocolVersion; health.info returns versions, orgId, and startedAt.
Tray & UI Integration
apps/desktop/src/main/lib/tray/index.ts, apps/desktop/src/renderer/routes/_authenticated/providers/HostServiceProvider/HostServiceProvider.tsx
Replaced terminal-daemon tray model with host-service-driven menu: shows per-org status/uptime, restart/stop actions wired to manager, event-driven refresh, changed quit menu behavior; HostServiceProvider passes organizationName to service queries.
Quit / Window Lifecycle & App API
apps/desktop/src/main/index.ts, apps/desktop/src/main/windows/main.ts, apps/desktop/src/lib/electron-app/factories/app/setup.ts, apps/desktop/src/lib/trpc/routers/settings/index.ts
Introduced QuitMode API (requestQuit, prepareQuit, exitImmediately), replaced prior quit-bypass flows, before-quit consumes pending mode and prevents quit on macOS if services active (hides windows); focusMainWindow now emits activate when no window; macOS close hides window; settings restart uses exitImmediately.
Auto-Updater
apps/desktop/src/main/lib/auto-updater.ts
installUpdate calls prepareQuit("release") before quit/install; on update-downloaded performs best-effort runtime host-service compatibility check (errors suppressed).
Terminal Transport
apps/desktop/src/renderer/lib/terminal/terminal-ws-transport.ts
Added internal reconnect state and exponential-backoff auto-reconnect on WebSocket close; added _exited flag to prevent reconnect after server exit.
Terminal Session REST
packages/host-service/src/terminal/terminal.ts
Added REST endpoints: DELETE /terminal/sessions/:terminalId to dispose session and GET /terminal/sessions to list in-memory sessions.
Miscellaneous Tests / Mocks
apps/desktop/src/main/lib/host-service-manager.test.ts
Extended mocks and tests for new lifecycle behaviors and compatibility checks.

Sequence Diagram(s)

sequenceDiagram
    participant App as Desktop App
    participant Manager as HostServiceManager
    participant Manifest as Manifest Layer
    participant Process as Host Service Process
    participant IPC as IPC Channel

    App->>Manager: discoverAndAdoptAll()
    Manager->>Manifest: listManifests()
    Manifest-->>Manager: [manifests...]

    loop each manifest
        Manager->>Manifest: isProcessAlive(pid)
        Manifest-->>Manager: alive?/dead?
        Manager->>Manager: run health & compatibility checks
        alt alive & compatible
            Manager->>Process: adopt (no spawn)
            Process->>IPC: "ready" (serviceVersion, protocolVersion, startedAt)
            IPC->>Manager: notify ready
            Manager->>Manager: emit "status-changed":"running"
        else dead or incompatible
            Manager->>Manager: mark "degraded" / schedule restart
        end
    end

    Manager-->>App: adoption complete
    App->>App: initialize tray with service statuses
Loading
sequenceDiagram
    participant Tray as Tray Menu
    participant Manager as HostServiceManager
    participant Service as Host Service
    participant App as Desktop App

    Tray->>Manager: getActiveOrganizationIds + getServiceInfo()
    Manager-->>Tray: [{orgId, status, uptime, compatibility, ...}]
    Tray->>Tray: render menu

    Manager->>Tray: emit "status-changed" events
    Tray->>Tray: rebuild menu immediately

    alt User selects "Restart"
        Tray->>Manager: restart(orgId)
        Manager->>Service: stop/kill -> spawn new process
        Service->>Manager: "ready"
        Manager->>Tray: emit "status-changed":"running"
        Tray->>Tray: refresh menu
    else User selects "Stop"
        Tray->>Manager: stop(orgId)
        Manager->>Service: terminate
        Manager->>Tray: emit "status-changed":"stopped"
        Tray->>Tray: refresh menu
    end
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~50 minutes

Poem

🐰 I hop through manifests, noses twitching bright,
I adopt old services that slept through the night,
Status flags flutter — degraded or fine,
I restart, I release, I keep all in line,
A rabbit-approved desktop, humming just right.

🚥 Pre-merge checks | ❌ 3

❌ Failed checks (1 warning, 2 inconclusive)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 20.59% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Title check ❓ Inconclusive The title 'Host service' is vague and overly generic, failing to convey the specific nature or scope of substantial changes across lifecycle management, manifest persistence, tray UI, and terminal reconnection. Expand the title to reflect the main objective, e.g., 'Add durable per-org host services with manifest adoption and tray UI,' or 'Implement host service durability with auto-adoption and tray management.'
Description check ❓ Inconclusive The PR description is mostly empty, with only the template structure and a separate auto-generated summary by 'cubic' providing substantial context about the changes. The author should fill in the required sections: provide a clear description of changes, link related issues, mark the type of change (appears to be 'New feature'), describe testing performed, and any additional notes. The auto-generated summary is helpful but should not replace the author's own description.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch decorous-salesman

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 3, 2026

🚀 Preview Deployment

🔗 Preview Links

Service Status Link
Neon Database (Neon) View Branch
Fly.io Electric (Fly.io) View App
Vercel API (Vercel) Open Preview
Vercel Web (Vercel) Open Preview
Vercel Marketing (Vercel) Open Preview
Vercel Admin (Vercel) Open Preview
Vercel Docs (Vercel) Open Preview

Preview updates automatically with new commits

@greptile-apps
Copy link
Copy Markdown

greptile-apps Bot commented Apr 3, 2026

Greptile Summary

This PR promotes HostServiceManager from a simple process supervisor to a full EventEmitter-based state machine, adding degraded/restarting/stopped statuses, compatibility checks between app and service protocol versions, restart/stop controls, real-time tray menu updates, and terminal WebSocket auto-reconnect. The tray menu is rewritten around host-service status instead of terminal sessions, and new tRPC endpoints (restart, getServiceInfo, onStatusChange) expose the richer state to the renderer.

Key findings:

  • Exponential backoff is broken — the scheduled restart timer deletes the degraded instance from this.instances before calling spawn(). Since spawn() reads restartCount from this.instances.get(organizationId), it always sees undefined and resets the count to 0. A service that keeps crashing will always restart after the base 1 s delay, never backing off to the configured 30 s maximum.
  • _exited flag in TerminalTransport is never reset — once a terminal session sends an exit message, scheduleReconnect is permanently suppressed on that transport object. If the same transport is reused to connect to a new session URL (e.g., after a host-service restart), connection drops on the new session silently skip auto-reconnect.
  • emitStatus(\"starting\", null) in spawn() always reports previousStatus: null, even when the caller knows the real prior state (\"degraded\" for crash recovery, \"restarting\" for explicit restart).

Confidence Score: 3/5

Not yet safe to merge — two concrete logic bugs (broken backoff, stale _exited flag) affect crash-recovery reliability before the primary feature path is stable.

The overall architecture is sound and the new features are well-structured. However, two P1 logic bugs undermine the crash-recovery guarantees central to this PR: the exponential backoff never advances beyond the base delay, and terminal auto-reconnect can silently stop working after a session exits. Both are straightforward to fix but important for the reliability story this PR is building.

apps/desktop/src/main/lib/host-service-manager.ts (broken restartCount propagation in scheduleRestart) and apps/desktop/src/renderer/lib/terminal/terminal-ws-transport.ts (_exited not reset in connect())

Important Files Changed

Filename Overview
apps/desktop/src/main/lib/host-service-manager.ts Major refactor: HostServiceManager now extends EventEmitter, adds status types (degraded/restarting/stopped), restart/compatibility logic, and service info API — but restartCount is always reset to 0 in respawn due to premature instance deletion, breaking exponential backoff.
apps/desktop/src/renderer/lib/terminal/terminal-ws-transport.ts Adds auto-reconnect with exponential backoff for terminal WebSocket drops; _exited flag prevents reconnect after terminal exit but is never reset in connect(), causing missed reconnects when the transport is reused for a new session.
apps/desktop/src/lib/trpc/routers/host-service-manager/index.ts Adds restart, getServiceInfo, and onStatusChange subscription endpoints; correctly wires auth token and cloud API URL in the restart mutation; looks clean.
apps/desktop/src/main/lib/tray/index.ts Replaces terminal-session tray menu with host-service status/controls; event-driven menu rebuilding replaces async polling; Quit now skips the old session-check confirmation dialog — intentional simplification since users can stop the service separately.
apps/desktop/src/lib/electron-app/factories/app/setup.ts window-all-closed now keeps the app alive on non-Mac when host-service instances are running; uses dynamic require to avoid circular dependency.
apps/desktop/src/main/lib/auto-updater.ts Triggers compatibility check across all running host-service instances when an app update is downloaded; safely catches errors if manager isn't initialized.
apps/desktop/src/main/host-service/index.ts Reads serviceVersion and protocolVersion from environment, sends them in the ready IPC message; straightforward and correct.
packages/host-service/src/terminal/terminal.ts Adds REST DELETE and GET endpoints for terminal session management alongside the existing WebSocket handler; clean addition.
packages/host-service/src/trpc/router/health/health.ts Health info endpoint extended with serviceVersion, protocolVersion, organizationId, and startedAt; straightforward and correct.
apps/desktop/src/renderer/routes/_authenticated/providers/HostServiceProvider/HostServiceProvider.tsx Passes organizationName through to getLocalPort so the manager can display it in the tray; correctly looks it up from the organizations query.
packages/host-service/src/app.ts Plumbs serviceVersion and protocolVersion into the app context so they're accessible in route handlers; clean.
packages/host-service/src/types.ts Adds serviceVersion and protocolVersion to HostServiceContext; minimal change, correct.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A([start / restart called]) --> B[spawn: new instance restartCount=previousInstance?.restartCount ?? 0]
    B --> C{async env build still current?}
    C -- no --> D([throw cancelled])
    C -- yes --> E[fork host-service child process]
    E --> F{ready message within 10 s?}
    F -- yes --> G[status: running, restartCount reset to 0, check compatibility]
    F -- timeout --> H[failStartup, status: degraded, scheduleRestart]
    G --> I{process exits?}
    I -- intentional stop --> J([status: stopped])
    I -- unexpected exit --> K[status: degraded, scheduleRestart]
    K --> L[delay = BASE x 2^restartCount, instance.restartCount++]
    L --> M[timer fires: DELETE instance, spawn again - restartCount lost!]
    M --> B

    style M fill:#f88,stroke:#c00
    style B fill:#ffd,stroke:#aa0
Loading

Reviews (1): Last reviewed commit: "Merge remote-tracking branch 'origin' in..." | Re-trigger Greptile

Comment thread apps/desktop/src/renderer/lib/terminal/terminal-ws-transport.ts
Comment thread apps/desktop/src/main/lib/host-service-manager.ts
Comment thread apps/desktop/src/main/lib/host-service-manager.ts
Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

6 issues found across 12 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/lib/auto-updater.ts">

<violation number="1" location="apps/desktop/src/main/lib/auto-updater.ts:276">
P2: Avoid an empty `catch` here; log a warning with context so failures in the host-service compatibility check are observable.

(Based on your team's feedback about handling async/errors without silent catches.) [FEEDBACK_USED]</violation>
</file>

<file name="apps/desktop/src/renderer/lib/terminal/terminal-ws-transport.ts">

<violation number="1" location="apps/desktop/src/renderer/lib/terminal/terminal-ws-transport.ts:105">
P2: `_exited` is never reset on new connections, which can permanently disable auto-reconnect after a prior exit event.</violation>
</file>

<file name="apps/desktop/src/main/lib/host-service-manager.ts">

<violation number="1" location="apps/desktop/src/main/lib/host-service-manager.ts:161">
P1: Exponential backoff is broken: `this.instances.delete(organizationId)` discards the `restartCount` right before `spawn()` tries to read it, so `restartCount` is always 0 and the delay is always 1 second. Remove the delete — `spawn()` already overwrites the entry via `this.instances.set()`.</violation>
</file>

<file name="apps/desktop/src/renderer/routes/_authenticated/providers/HostServiceProvider/HostServiceProvider.tsx">

<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/providers/HostServiceProvider/HostServiceProvider.tsx:59">
P1: Use the same `getLocalPort` input shape for cache writes/queries and cache reads. Adding `organizationName` only to `ensureData/useQuery` can make `getData({ organizationId })` miss cached entries.</violation>
</file>

<file name="apps/desktop/src/main/lib/tray/index.ts">

<violation number="1" location="apps/desktop/src/main/lib/tray/index.ts:296">
P2: The new `status-changed` subscription is never unsubscribed in `disposeTray`, which can leak listeners and duplicate menu updates across tray re-initializations.</violation>
</file>

<file name="apps/desktop/src/lib/electron-app/factories/app/setup.ts">

<violation number="1" location="apps/desktop/src/lib/electron-app/factories/app/setup.ts:61">
P1: On Windows/Linux this change can leave the app running headless with no way to reopen a window, because quit is skipped when host-service is active but non-macOS has no tray/reopen path.</violation>
</file>

Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.

Comment thread apps/desktop/src/main/lib/host-service-manager.ts
Comment thread apps/desktop/src/lib/electron-app/factories/app/setup.ts Outdated
Comment thread apps/desktop/src/main/lib/auto-updater.ts
Comment thread apps/desktop/src/renderer/lib/terminal/terminal-ws-transport.ts
Comment thread apps/desktop/src/main/lib/tray/index.ts
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
apps/desktop/src/renderer/lib/terminal/terminal-ws-transport.ts (1)

166-178: ⚠️ Potential issue | 🟡 Minor

_exited flag is not reset in disconnect(), preventing future reconnection.

When disconnect() is called explicitly (e.g., user closes terminal), _exited remains true from a previous session. If the same transport is reused with a new connect() call, scheduleReconnect will bail out early due to the stale _exited flag.

🛠️ Proposed fix
 export function disconnect(transport: TerminalTransport) {
 	cancelReconnect(transport);
 	if (transport.socket) {
 		transport.socket.close();
 		transport.socket = null;
 	}
 	transport.currentUrl = null;
 	transport._terminal = null;
 	transport._reconnectAttempt = 0;
+	transport._exited = false;
 	setConnectionState(transport, "disconnected");
 	transport.onDataDisposable?.dispose();
 	transport.onDataDisposable = null;
 }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@apps/desktop/src/renderer/lib/terminal/terminal-ws-transport.ts` around lines
166 - 178, The disconnect function leaves the transport._exited flag set, which
prevents future reconnects; update disconnect(transport: TerminalTransport) to
clear the flag (set transport._exited = false) so subsequent
connect()/scheduleReconnect() calls won't bail out due to a stale exit state;
locate the disconnect function and add the reset alongside resetting _terminal
and _reconnectAttempt, ensuring the same TerminalTransport instance can
reconnect after an explicit disconnect.
apps/desktop/src/renderer/routes/_authenticated/providers/HostServiceProvider/HostServiceProvider.tsx (1)

83-122: ⚠️ Potential issue | 🟠 Major

services can go stale after a host-service restart.

In apps/desktop/src/main/lib/host-service-manager.ts, Line 286 generates a new secret on every spawn, and apps/desktop/src/lib/trpc/routers/host-service-manager/index.ts Lines 53-64 return the fresh { port, secret } on restart. This memo only reuses cached getLocalPort data, so after a crash/manual restart it can keep stale credentials, and possibly a stale port, until the provider remounts. Please invalidate/refetch getLocalPort from the new status subscription or update the cache from the restart path.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@apps/desktop/src/renderer/routes/_authenticated/providers/HostServiceProvider/HostServiceProvider.tsx`
around lines 83 - 122, The services Map produced in the useMemo (function addOrg
/ variable services) can keep stale port/secret because it only reads cached
utils.hostServiceManager.getLocalPort.getData; update the code so the provider
listens to the host-service status subscription (or the restart code path that
returns {port, secret}) and triggers a fresh refetch/invalidation of
getLocalPort before building services: on restart/status change call
utils.hostServiceManager.getLocalPort.invalidate or explicit
utils.hostServiceManager.getLocalPort.fetch (or update the cache with the new
{port, secret}), then rebuild the Map (ensuring addOrg calls
setHostServiceSecret and getHostServiceClient with the fresh values) so services
never use stale credentials or ports.
🧹 Nitpick comments (1)
apps/desktop/src/main/lib/auto-updater.ts (1)

270-278: Consider logging unexpected errors for debuggability.

The empty catch {} block silently swallows all errors, not just the expected "not initialized" case. While this doesn't break functionality, logging unexpected errors could help diagnose issues during development.

💡 Optional: Add debug logging for unexpected errors
 		try {
 			const { getHostServiceManager } = require("../host-service-manager");
 			getHostServiceManager().checkAllCompatibility();
-		} catch {
-			// Host service manager may not be initialized yet
+		} catch (error) {
+			// Host service manager may not be initialized yet; log other errors for debugging
+			if (error instanceof Error && !error.message.includes("not initialized")) {
+				console.debug("[auto-updater] checkAllCompatibility error:", error.message);
+			}
 		}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@apps/desktop/src/main/lib/auto-updater.ts` around lines 270 - 278, Replace
the empty catch with a caught-error handler that logs unexpected exceptions when
calling getHostServiceManager().checkAllCompatibility(); e.g., change catch {}
to catch (err) { (processLogger ?? console).error("auto-updater: error checking
host-service compatibility", err); } so you still ignore the expected “not
initialized” case but surface other unexpected errors from
getHostServiceManager()/checkAllCompatibility().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@apps/desktop/src/main/host-service/index.ts`:
- Around line 24-27: The protocolVersion assignment can become NaN when
process.env.HOST_SERVICE_PROTOCOL_VERSION is non-numeric; update the logic
around protocolVersion to parse the env var (e.g., Number or parseInt), check
isNaN on the result, and set protocolVersion to null when invalid so downstream
comparisons don't use NaN; reference the variables serviceVersion and
protocolVersion and the env var HOST_SERVICE_PROTOCOL_VERSION when making this
change.

In `@apps/desktop/src/main/lib/host-service-manager.ts`:
- Around line 218-226: hasActiveInstances() currently only treats "running" and
"starting" as active, causing the app to quit during backoff when services are
"recovering"/"degraded"/"restarting"; update the hasActiveInstances method to
consider those recovery-related statuses (e.g., include "recovering",
"degraded", and "restarting" alongside "running"/"starting") as active so the
method returns true while services are in recovery; keep the existing
pendingStarts.size check as well.
- Around line 243-260: The failing tests show HostServiceManager directly calls
app.getVersion (seen in checkCompatibility and buildHostServiceEnv) causing a
TypeError when Electron's app mock lacks getVersion; refactor by centralizing
version access behind a single helper or injectable accessor (e.g., create a
getAppVersion() helper or a versionProvider passed into HostServiceManager) and
replace direct app.getVersion usages in checkCompatibility and
buildHostServiceEnv with that helper; update tests to stub/mock that single
helper (or inject a mock versionProvider) so all HostServiceManager tests can
control the app version in one place.

In `@apps/desktop/src/main/lib/tray/index.ts`:
- Around line 294-298: The "status-changed" listener added via
getHostServiceManager().on("status-changed", ...) must be unregistered in
disposeTray(); define the listener as a named/const function (e.g.,
onHostServiceStatusChanged or similar) when you register it so you can call
manager.off("status-changed", thatHandler) (or removeListener) inside
disposeTray() using getHostServiceManager() to get the same singleton manager;
ensure disposeTray() removes the exact same handler reference to prevent stacked
handlers when the tray is re-initialized.

In
`@apps/desktop/src/renderer/routes/_authenticated/providers/HostServiceProvider/HostServiceProvider.tsx`:
- Around line 55-60: The cache key for getLocalPort is inconsistent: ensureData
is called with { organizationId, organizationName } but later reads only use {
organizationId }, causing different cache entries; update every read of
utils.hostServiceManager.getLocalPort (including the one that currently passes
only { organizationId } and the occurrences around the block flagged as also
affected) to use the same key shape by passing organizationName (e.g.
organizationName: org?.name ?? undefined) together with organizationId so all
lookups use the identical cache key as ensureData.

---

Outside diff comments:
In `@apps/desktop/src/renderer/lib/terminal/terminal-ws-transport.ts`:
- Around line 166-178: The disconnect function leaves the transport._exited flag
set, which prevents future reconnects; update disconnect(transport:
TerminalTransport) to clear the flag (set transport._exited = false) so
subsequent connect()/scheduleReconnect() calls won't bail out due to a stale
exit state; locate the disconnect function and add the reset alongside resetting
_terminal and _reconnectAttempt, ensuring the same TerminalTransport instance
can reconnect after an explicit disconnect.

In
`@apps/desktop/src/renderer/routes/_authenticated/providers/HostServiceProvider/HostServiceProvider.tsx`:
- Around line 83-122: The services Map produced in the useMemo (function addOrg
/ variable services) can keep stale port/secret because it only reads cached
utils.hostServiceManager.getLocalPort.getData; update the code so the provider
listens to the host-service status subscription (or the restart code path that
returns {port, secret}) and triggers a fresh refetch/invalidation of
getLocalPort before building services: on restart/status change call
utils.hostServiceManager.getLocalPort.invalidate or explicit
utils.hostServiceManager.getLocalPort.fetch (or update the cache with the new
{port, secret}), then rebuild the Map (ensuring addOrg calls
setHostServiceSecret and getHostServiceClient with the fresh values) so services
never use stale credentials or ports.

---

Nitpick comments:
In `@apps/desktop/src/main/lib/auto-updater.ts`:
- Around line 270-278: Replace the empty catch with a caught-error handler that
logs unexpected exceptions when calling
getHostServiceManager().checkAllCompatibility(); e.g., change catch {} to catch
(err) { (processLogger ?? console).error("auto-updater: error checking
host-service compatibility", err); } so you still ignore the expected “not
initialized” case but surface other unexpected errors from
getHostServiceManager()/checkAllCompatibility().
🪄 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: aefb92e7-ecc6-4aa8-a5db-b21b20eb0118

📥 Commits

Reviewing files that changed from the base of the PR and between 7599ace and 026bf9b.

📒 Files selected for processing (12)
  • apps/desktop/src/lib/electron-app/factories/app/setup.ts
  • apps/desktop/src/lib/trpc/routers/host-service-manager/index.ts
  • apps/desktop/src/main/host-service/index.ts
  • apps/desktop/src/main/lib/auto-updater.ts
  • apps/desktop/src/main/lib/host-service-manager.ts
  • apps/desktop/src/main/lib/tray/index.ts
  • apps/desktop/src/renderer/lib/terminal/terminal-ws-transport.ts
  • apps/desktop/src/renderer/routes/_authenticated/providers/HostServiceProvider/HostServiceProvider.tsx
  • packages/host-service/src/app.ts
  • packages/host-service/src/terminal/terminal.ts
  • packages/host-service/src/trpc/router/health/health.ts
  • packages/host-service/src/types.ts

Comment thread apps/desktop/src/main/host-service/index.ts Outdated
Comment thread apps/desktop/src/main/lib/host-service-manager.ts Outdated
Comment thread apps/desktop/src/main/lib/host-service-manager.ts Outdated
Comment thread apps/desktop/src/main/lib/tray/index.ts Outdated
Comment thread apps/desktop/src/main/host-service/index.ts
Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

3 issues found across 9 files (changes from recent commits).

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/lib/host-service-manifest.ts">

<violation number="1" location="apps/desktop/src/main/lib/host-service-manifest.ts:64">
P2: Validate that the manifest’s `organizationId` matches the requested `organizationId` before returning parsed data.</violation>
</file>

<file name="apps/desktop/src/main/host-service/index.ts">

<violation number="1" location="apps/desktop/src/main/host-service/index.ts:77">
P1: Manifest writes `authToken` to disk using default permissions, which can expose the host-service secret to other local users.</violation>

<violation number="2" location="apps/desktop/src/main/host-service/index.ts:94">
P2: Do not silently swallow manifest cleanup failures; log a warning with context.

(Based on your team's feedback about handling async/error paths without silent catches.) [FEEDBACK_USED]</violation>
</file>

Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.

Comment thread apps/desktop/src/main/host-service/index.ts Outdated
Comment thread apps/desktop/src/main/lib/host-service-manifest.ts
Comment thread apps/desktop/src/main/host-service/index.ts Outdated
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

♻️ Duplicate comments (1)
apps/desktop/src/main/lib/host-service-manager.ts (1)

293-300: ⚠️ Potential issue | 🟠 Major

Keep recovering instances counted as active.

hasActiveInstances() drops to false while an org is degraded or restarting. Line 193 in apps/desktop/src/main/index.ts now uses this to decide whether macOS should stay alive, so quitting during the backoff window can tear down the app even though recovery is still in flight.

💡 Suggested fix
 hasActiveInstances(): boolean {
 	for (const instance of this.instances.values()) {
-		if (instance.status === "running" || instance.status === "starting") {
+		if (instance.status !== "stopped") {
 			return true;
 		}
 	}
 	return this.pendingStarts.size > 0;
 }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@apps/desktop/src/main/lib/host-service-manager.ts` around lines 293 - 300,
hasActiveInstances() incorrectly returns false when instances are in
recovery/restarting states; update the check in hasActiveInstances() to treat
additional statuses (e.g., "recovering", "restarting", or any other non-terminal
states your InstanceStatus enum uses) as active by including them in the
conditional that currently tests for "running" or "starting", and ensure
pendingStarts still contributes to the result; modify the loop over
this.instances.values() in hasActiveInstances() to return true for those
recovery-related statuses so the app remains alive while recovery is in flight.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@apps/desktop/src/main/lib/host-service-manager.ts`:
- Around line 508-517: The restart backoff is lost because scheduleRestart()
increments current.restartCount then deletes the instance before spawn()
recreates it with restartCount reset to 0; update spawn() (or the restart flow)
to preserve the incremented value by reading the previous instance's
restartCount (e.g., previousInstance?.restartCount) and using that value when
constructing the new HostServiceProcess, or avoid deleting current before spawn
snapshots it — ensure the restartCount from the existing map entry is carried
into the new instance creation in spawn() so crash loops back off past
BASE_RESTART_DELAY (also apply the same fix for the similar code around
scheduleRestart()/spawn() at the other location mentioned).
- Around line 170-183: The stop() logic currently sends SIGTERM (via
process.kill(instance.adoptedPid) or instance.process?.kill()) then immediately
deletes state (this.instances.delete, removeManifest) and emits "stopped";
instead, after sending SIGTERM wait for the target process to actually exit
before clearing tracking and removing the manifest: after kill, attach/await the
process exit (for adopted PIDs poll/wait for the PID to die or listen to the
ChildProcess 'exit' event for instance.process), with a sensible
timeout/fallback to force-kill, then only call
this.instances.delete(organizationId), removeManifest(organizationId) and
emitStatus(organizationId, "stopped", previousStatus); apply the same pattern to
the other stop/unregister code paths referenced (around lines 224-237 and
377-401) so we never drop tracking while the old PID may still be running.

---

Duplicate comments:
In `@apps/desktop/src/main/lib/host-service-manager.ts`:
- Around line 293-300: hasActiveInstances() incorrectly returns false when
instances are in recovery/restarting states; update the check in
hasActiveInstances() to treat additional statuses (e.g., "recovering",
"restarting", or any other non-terminal states your InstanceStatus enum uses) as
active by including them in the conditional that currently tests for "running"
or "starting", and ensure pendingStarts still contributes to the result; modify
the loop over this.instances.values() in hasActiveInstances() to return true for
those recovery-related statuses so the app remains alive while recovery is in
flight.
🪄 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: e7e5f40f-e958-4afb-95d3-ff942b69c982

📥 Commits

Reviewing files that changed from the base of the PR and between 026bf9b and 6c504af.

📒 Files selected for processing (9)
  • apps/desktop/HOST_SERVICE_LIFECYCLE.md
  • apps/desktop/src/lib/electron-app/factories/app/setup.ts
  • apps/desktop/src/main/host-service/index.ts
  • apps/desktop/src/main/index.ts
  • apps/desktop/src/main/lib/host-service-manager.test.ts
  • apps/desktop/src/main/lib/host-service-manager.ts
  • apps/desktop/src/main/lib/host-service-manifest.ts
  • apps/desktop/src/main/lib/tray/index.ts
  • apps/desktop/src/main/windows/main.ts
✅ Files skipped from review due to trivial changes (2)
  • apps/desktop/HOST_SERVICE_LIFECYCLE.md
  • apps/desktop/src/main/lib/tray/index.ts

Comment thread apps/desktop/src/main/lib/host-service-manager.ts
Comment thread apps/desktop/src/main/lib/host-service-manager.ts
@@ -0,0 +1,40 @@
# Host Service Lifecycle
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@saddlepaddle this will be helpful

@cubic-dev-ai
Copy link
Copy Markdown
Contributor

cubic-dev-ai Bot commented Apr 4, 2026

You're iterating quickly on this pull request. To help protect your rate limits, cubic has paused automatic reviews on new pushes for now—when you're ready for another review, comment @cubic-dev-ai review.

saddlepaddle and others added 5 commits April 4, 2026 12:54
Defines the target architecture for the host service package and Electron
desktop layer — API shapes, ownership boundaries, and what needs to move
to make the host service deployable standalone without Electron awareness.
"Quit (Keep Services Running)" was exiting the process entirely,
destroying the tray. Now it destroys windows but keeps the tray so
users can still monitor and manage running services.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants