[codex] Fix canary host-service adoption#4499
Conversation
|
Capy auto-review is paused for this organization because the monthly auto-review limit has been reached. Increase the limit or turn it off in billing settings to resume automatic reviews. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe PR changes host-service adoption and development-mode detection: app-version mismatches no longer force termination — the coordinator always health-checks before deciding to kill and respawn; NODE_ENV is explicitly propagated to spawned host-service processes; dev-mode detection is narrowed to NODE_ENV === "development". ChangesService adoption and development mode detection
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
No issues found across 7 files
You’re at about 91% of the monthly review 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.
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
Greptile SummaryThis PR fixes canary host-service adoption by removing the pre-health-check SIGTERM that fired on any app-version mismatch, and by explicitly setting
Confidence Score: 4/5The coordinator and supervisor changes are internally consistent and well-tested; the one gap is a missing test for the pre-upgrade empty-spawnedByAppVersion path whose behavior changed. The core logic is correct — health-check gating replaces the blunt app-version kill, and explicit NODE_ENV propagation closes the packaged-build teardown gap. The pre-upgrade manifest path (spawnedByAppVersion coerced to "") silently switched from kill-immediately to health-check-first without a dedicated test, so a regression there would not be caught by the suite. apps/desktop/src/main/lib/host-service-coordinator.test.ts — the pre-upgrade manifest (empty spawnedByAppVersion) adoption path is untested after the behavioral change.
|
| Filename | Overview |
|---|---|
| apps/desktop/src/main/lib/host-service-coordinator.ts | App-version mismatch now falls through to the existing health check instead of issuing an early SIGTERM; packaged builds explicitly set NODE_ENV=production in the child environment. |
| apps/desktop/src/main/lib/host-service-coordinator.test.ts | Old version-mismatch test replaced with two new tests (healthy adopt + SIGKILL after failed health check); the pre-upgrade empty-spawnedByAppVersion path is not covered. |
| packages/host-service/src/daemon/DaemonSupervisor.ts | shouldKillStaleDaemonForDev and isDev checks flipped from 'not production' to 'explicitly development', so unset NODE_ENV is now treated as production-safe. |
| packages/host-service/src/serve.ts | isDev guard for the SIGTERM/SIGINT daemon-teardown handler changed from !== 'production' to === 'development', preventing packaged builds from killing PTYs on host-service exit. |
| apps/desktop/src/main/lib/host-service-manifest.ts | Comment-only changes clarifying that spawnedByAppVersion is now diagnostic metadata rather than a kill trigger. |
| packages/host-service/src/daemon/DaemonSupervisor.test.ts | Test updated to reflect that shouldKillStaleDaemonForDev({}) now returns false instead of true. |
| packages/host-service/src/trpc/router/host/host.ts | Comment-only change removing reference to the now-removed strict-equality adoption check. |
Sequence Diagram
sequenceDiagram
participant C as HostServiceCoordinator
participant M as ManifestStore
participant HS as Old Host-Service
participant HC as Health Check
Note over C: Canary app-version bump
C->>M: readManifest(orgId)
M-->>C: "manifest (spawnedByAppVersion != currentAppVersion)"
Note over C: OLD: SIGTERM → removeManifest → return null
Note over C: NEW: log version mismatch, fall through
C->>HC: pollHealthCheck(endpoint, 2000ms)
alt Healthy
HC-->>C: true
C->>C: adopt pid, port, secret
Note over HS: PTYs survive ✓
else Unhealthy
HC-->>C: false
C->>HS: SIGKILL(pid)
C->>M: removeManifest(orgId)
C->>C: "spawn fresh host-service (NODE_ENV=production)"
end
Comments Outside Diff (1)
-
apps/desktop/src/main/lib/host-service-coordinator.test.ts, line 191-224 (link)Missing test for pre-upgrade manifest path
Two new tests cover
spawnedByAppVersion = "0.9.0"(explicit old version), but neither coversspawnedByAppVersion = ""(the value coerced for pre-upgrade manifests inreadManifest). The behavior for that case changed: the old code hit the!== currentAppVersionbranch and killed immediately; the new code health-checks the process instead. A test that usesspawnedByAppVersion: ""would confirm the adoption path (health-check proceeds, healthy → adopt, unhealthy → SIGKILL + respawn) and prevent the empty-string coercion path from silently regressing.Prompt To Fix With AI
This is a comment left during a code review. Path: apps/desktop/src/main/lib/host-service-coordinator.test.ts Line: 191-224 Comment: **Missing test for pre-upgrade manifest path** Two new tests cover `spawnedByAppVersion = "0.9.0"` (explicit old version), but neither covers `spawnedByAppVersion = ""` (the value coerced for pre-upgrade manifests in `readManifest`). The behavior for that case changed: the old code hit the `!== currentAppVersion` branch and killed immediately; the new code health-checks the process instead. A test that uses `spawnedByAppVersion: ""` would confirm the adoption path (health-check proceeds, healthy → adopt, unhealthy → SIGKILL + respawn) and prevent the empty-string coercion path from silently regressing. How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix All With AI
Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
apps/desktop/src/main/lib/host-service-coordinator.test.ts:191-224
**Missing test for pre-upgrade manifest path**
Two new tests cover `spawnedByAppVersion = "0.9.0"` (explicit old version), but neither covers `spawnedByAppVersion = ""` (the value coerced for pre-upgrade manifests in `readManifest`). The behavior for that case changed: the old code hit the `!== currentAppVersion` branch and killed immediately; the new code health-checks the process instead. A test that uses `spawnedByAppVersion: ""` would confirm the adoption path (health-check proceeds, healthy → adopt, unhealthy → SIGKILL + respawn) and prevent the empty-string coercion path from silently regressing.
Reviews (1): Last reviewed commit: "fix canary host-service adoption" | Re-trigger Greptile
* fix canary host-service adoption * test pre-upgrade host-service adoption
1 participant
Summary
NODE_ENV=production, and only run host-service/daemon dev teardown behavior whenNODE_ENV === "development".Root Cause
Canary app version bumps changed
app.getVersion()even when the host-service/daemon payload did not require a restart. The desktop coordinator killed the old host-service before health-checking it. In packaged builds the spawned host-service inherited an unsetNODE_ENV, which made the old host-service take the dev SIGTERM path and stop the pty daemon, killing live terminal sessions before the new host-service could adopt.Impact
Canary updates should now reuse a healthy existing host-service across app-version-only bumps, preserving the daemon and PTYs. Unhealthy adopted services still get killed and respawned after the health check.
Validation
bun test apps/desktop/src/main/lib/host-service-coordinator.test.tsbun test packages/host-service/src/daemon/DaemonSupervisor.test.tsbun run --cwd packages/pty-daemon build:daemonnode --experimental-strip-types --test packages/host-service/src/daemon/DaemonSupervisor.node-test.tsbun run lint:fixbun run lintgit diff --checkSummary by cubic
Prevents terminal disruption during canary updates by reusing a healthy host-service on app-version-only bumps and pre-upgrade manifests. Packaged builds now run the host-service in production mode to avoid dev teardown.
spawnedByAppVersionas informational; run one health check, reuse if healthy, else SIGKILL and respawn.NODE_ENV=production; dev teardown and stdio only whenNODE_ENV === "development".SUPERSET_PTY_DAEMON_ADOPT_IN_DEV=1; added tests for pre-upgrade adoption and updated manifest/version comments.Written for commit 69303d6. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Documentation
Tests