Repository navigation
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Linked Issues check |  |
Issue #29133 requires a private hash-pinned Node 24.22.1 macOS runtime with bounded Darwin worker waits, verification for both target runtimes in Mac packaging workflows, and automated runtime recover… |
Implement the #29133 runtime and packaging requirements in this pull request, including the bounded Darwin worker-wait patch, hash-pinned Node 24.22.1 build, target-runtime verification, and automated recovery tests. Otherwise, remove #2913… |
| Docstring Coverage | |
Docstring coverage is 10.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 20 functions across 27 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Title check The title clearly and concisely identifies the primary change: detecting stalled owners of preserved terminal daemons. Out of Scope Changes check The owner health checks, targeted review and recovery, shutdown fencing, IPC APIs, UI confirmation, and related tests implement the preserved-owner detection and recovery work. Issue #29133identifies…Description check The description is comprehensive and follows the required template. It explains the change, motivation, testing, compatibility considerations, review scope, and checklist status. The Visual Proof sect… Full details: Out of Scope Changes check
-
Explanation
The owner health checks, targeted review and recovery, shutdown fencing, IPC APIs, UI confirmation, and related tests implement the preserved-owner detection and recovery work. Issue
#29133identifies that work as related preserved-owner work. The changes do not show an unrelated product area.
Full details: Description check
-
Explanation
The description is comprehensive and follows the required template. It explains the change, motivation, testing, compatibility considerations, review scope, and checklist status. The Visual Proof section documents extensive behavioral validation, but GitHub screenshot attachments are still pending. The linked issue is identified as
#29133, although the standard “Fixes #” format is not used.
-
Full details: Linked Issues check
-
Explanation
Issue
#29133requires a private hash-pinned Node 24.22.1 macOS runtime with bounded Darwin worker waits, verification for both target runtimes in Mac packaging workflows, and automated runtime recovery tests. The reviewed changes implement preserved-daemon owner health checks and targeted recovery. They add no runtime build, Darwin worker patch, packaging verifier, or runtime qualification tests.Resolution
Implement the
#29133runtime and packaging requirements in this pull request, including the bounded Darwin worker-wait patch, hash-pinned Node 24.22.1 build, target-runtime verification, and automated recovery tests. Otherwise, remove#29133as a direct link and link the preserved-owner work to its appropriate issue.
✨ Finishing Touches: 💡 1
-
📝 Generate docstrings: 💡
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 @coderabbitai help to get the list of available commands.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments: 1
src/main/ipc/pty-management-async-io-health.test.ts: 1
69-75: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReturn a fresh identity copy from the fixture
getDaemonIdentity.
mockReturnValuereturns the same object on every call. The realDaemonClient.getDaemonIdentity()returns a new copy each time. This mock hides the reference-equality defect inreadDaemonProviderAsyncIoHealth. UsemockImplementation(() => ({ ... }))here and for the mainadapterfixture. Then add a test that proves the health result survives the identity re-check.
ℹ️ Review info
⚙️ Run configuration
- Source mode: Auto
- Repository: Unavailable
- Branch: Unavailable
- File: No configuration file from this repository applied
- Commit: Unavailable
- Layers: Repository UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
b05284c2-c7be-4a3c-b3c3-17d0f4f27129
📒 Files selected for processing: 30
config/reliability-gates.jsoncdocs/bug-reproductions/daemon-async-io-stall.mdsrc/main/daemon/daemon-async-io-probe.test.tssrc/main/daemon/daemon-async-io-probe.tssrc/main/daemon/daemon-async-io-spawn-health.test.tssrc/main/daemon/daemon-health-requests.tssrc/main/daemon/daemon-provider-async-io-health.tssrc/main/daemon/daemon-provider-restart.tssrc/main/daemon/daemon-pty-adapter-stalled-owner-shutdown.test.tssrc/main/daemon/daemon-pty-adapter.tssrc/main/daemon/daemon-pty-router.tssrc/main/daemon/daemon-request-router.tssrc/main/daemon/daemon-server.tssrc/main/daemon/daemon-session-owner-resolution.tssrc/main/daemon/daemon-stalled-owner-fencing.test.tssrc/main/daemon/daemon-stalled-owner-recovery.test.tssrc/main/daemon/daemon-stalled-owner-recovery.tssrc/main/daemon/daemon-stalled-owner-shutdown.tssrc/main/daemon/degraded-daemon-owner-recovery.tssrc/main/daemon/degraded-daemon-pty-provider.tssrc/main/daemon/types.tssrc/main/ipc/pty-management-async-io-health.test.tssrc/main/ipc/pty-management.tssrc/preload/api/pty-bridge-stream-and-serialization.tssrc/preload/api/pty-management-api.tssrc/renderer/src/components/sidebar/StalledTerminalDaemonBanner.test.tsxsrc/renderer/src/components/sidebar/StalledTerminalDaemonBanner.tsxsrc/renderer/src/i18n/locales/en.jsonsrc/renderer/src/web/preload-api/web-terminal-api.tssrc/shared/daemon-async-io-health.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 0 remain after this review.
| const health = getAllDaemonAdapters(provider).map((adapter): DaemonAsyncIoStatus => { | ||
| const result = results.get(adapter) | ||
| // Removed owners cannot warn; new or replaced owners cannot inherit health evidence. | ||
| return result?.identity && result.identity === adapter.getDaemonIdentity() |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
The identity check always fails with the real DaemonClient. Health always becomes unverifiable.
Line 28 compares result.identity === adapter.getDaemonIdentity() by reference. DaemonPtyAdapter.getDaemonIdentity() delegates to DaemonClient.getDaemonIdentity(). That method returns { ...this.daemonIdentity }, which is a new object on every call. The two values are never === when the identity is non-null.
Every adapter then maps to { state: 'unverifiable' }, so this reader never reports stalled or healthy. The stalled banner and the owner-targeted recovery flow never activate.
The unit tests do not catch this. The mocks use vi.fn().mockReturnValue(obj), which returns the same object each time.
Compare the identity fields instead. Update the test fixtures to return a fresh copy on each call.
🐛 Proposed fix
-
--- "a/src/main/daemon/daemon-provider-async-io-health.ts" +++ "b/src/main/daemon/daemon-provider-async-io-health.ts" @@ -25,8 +25,13 @@ const health = getAllDaemonAdapters(provider).map((adapter): DaemonAsyncIoStatus => { const result = results.get(adapter) // Removed owners cannot warn; new or replaced owners cannot inherit health evidence. - return result?.identity && result.identity === adapter.getDaemonIdentity() + const currentIdentity = adapter.getDaemonIdentity() + return result?.identity && + currentIdentity && + result.identity.pid === currentIdentity.pid && + result.identity.startedAtMs === currentIdentity.startedAtMs && + result.identity.launchNonce === currentIdentity.launchNonce ? { ...result.health, ...(result.health.state === 'stalled' && provider instanceof DegradedDaemonPtyProvider &&
The
{ ...result.health, ... }branch can stay unchanged. Alternatively, reuse thesameDaemonIdentityhelper thatsrc/main/daemon/client.tsalready uses.
📝 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 changereturn result?.identity && result.identity === adapter.getDaemonIdentity() const currentIdentity = adapter.getDaemonIdentity() return result?.identity && currentIdentity && result.identity.pid === currentIdentity.pid && result.identity.startedAtMs === currentIdentity.startedAtMs && result.identity.launchNonce === currentIdentity.launchNonce
|
Hey Neil, #29133 merged with a protocol bump to v52, so after the next release new terminals start on the patched daemon right away. I looked at this PR again in that light, and I think we should close it or park it as a draft. The recovery path needs the new If old-daemon stalls show up at a meaningful rate in the new stall telemetry after the release, we can do a small detection-only change instead: the banner would check every attached daemon and tell the user to close that daemon's terminals or restart. Your Oct 24 write-up was really useful here; it's what convinced us to do the bump. Thoughts? |
ELI5
An older terminal service can freeze while the newest service stays healthy. Check every adopted local service, and let a user review and end only the affected service's terminals when that service supports safe recovery.
What Changed
Health checks run concurrently against existing adapters and reject stale identity/provider evidence. A verified stall now carries the affected owner's protocol, PID, start time and launch nonce. The sidebar opens a dedicated review listing that owner's complete live session inventory.
Confirmation carries session incarnation IDs. The daemon rechecks its identity, stalled evidence and exact inventory, refuses pending terminal creation, and closes admission before disposal. A successful legacy recovery retires only that adapter; current replacement waits for dead-endpoint evidence under the existing restart barrier. Lost replies, failed disposal and changed inventories never trigger a broad fallback restart. Only successful verified stalled-owner teardown uses a self-directed hard exit, avoiding native worker joins that can hang ordinary process exit.
Why
The previous restart action replaced only the current daemon. Pointing an older-owner stall at that action could end healthy terminals and leave the affected owner running. Owner-specific recovery closes that mismatch.
Linked Issue
Maintainer-reported October 23 incident; related runtime prevention fix: #29133. Evidence:
docs/bug-reproductions/daemon-async-io-stall.md.Visual Proof
Hidden Electron CDP checks verified warning, exact-session review, Cancel, refusal/re-review and unsupported-peer states. A second full test used production preload/IPC and a real isolated daemon whose filesystem workers were deliberately blocked: Cancel preserved its real PTY, confirmation killed that reviewed PTY and retired its endpoint, the replacement returned healthy, and a fresh PTY spawned. That exercise exposed a native shutdown hang: endpoint retirement alone could leave the old process alive. The follow-up limits self SIGKILL to successful strictly reviewed stalled-owner shutdown, after final resource teardown and reply handling. A rebuilt full rendered test then proved the old daemon PID and reviewed PTY disappeared while FIFO workers remained blocked; replacement health and fresh PTY command output passed. All validation processes were cleaned up. A separate two-owner full rendered test adopted a prior-protocol endpoint, stalled only that owner, and confirmed recovery ended only its PTY. The healthy current daemon retained its PID/nonce, its PTY retained its incarnation, and fresh command output worked afterward. This fixture uses current daemon code advertising protocol 50; it proves legacy routing/targeting, not historical shipped-binary compatibility. All isolated processes were cleaned up.
This genuine path caught and fixed an identity-reference comparison that rejected copied client snapshots; fresh-snapshot and changed-owner regressions now pass. Screenshots were inspected locally; GitHub attachment upload remains pending because
gh imagehas no authenticated browser session token.Testing
Final head
496fba759b4481104d2938c961b67f63fa6100d9: 48 exit-focused tests and 32 final recovery/concurrency tests pass. Coverage includes normal shutdown, failed native/final resource disposal, completion ordering, competing ordinary shutdown, and shutdown during the strict health check. The real two-client regression fails against the prior implementation and passes with this fix. Node typechecking, changed-code quality, formatting and the 150-gate reliability manifest pass. Isolated actual exported-function FIFO proof and rebuilt hidden Electron rendered recovery prove old-process death without releasing blocked workers. Fresh final-head CI has passed all five Node unit shards, static/type checks, Windows packaging, relay integration and all three WSL2 distro checks (Ubuntu, Debian and Fedora). The full final-head PR workflow39950232205, including its explicit PR checks passed gate, succeeded. The separate headless-server workflow39950232258 also succeeded, including mobile bundle, glibc/musl compatibility and persistence on ARM/Intel Mac, Linux, Windows x64 and Windows ARM. Final aggregate: 38 successful checks, 16 intentional skips, no pending/failing checks.Automated probe, owner-generation, recovery, adapter and rendered-notice regressions added/updated.
All four
pnpm tcprojects pass; changed-code quality and reliability-manifest gates pass.Real isolated daemon/client tests cover stale owners, reused session IDs, added sessions, recovered health and failed native disposal.
Direct adapter tests cover unknown RPC, lost reply, refusal and admission-fence preservation.
Both real-PTY shell suites pass under Node (36 tests).
Actual final-source shutdown function exercised on all three platforms using Node24 (Linux24.21.0; macOS/Windows24.22.1) and disposable owned children: normal shutdown exits0; verified recovery terminates with SIGKILL on Linux/macOS and exit1 on Windows x64. Platform qualification39953504578 passed all jobs. This checks termination mechanics, not worker starvation or the complete recovery lifecycle.
The broader Bun run produced 3,057 passes and 20 failures: 19 real-PTY failures reproduced exactly on unchanged
origin/main, plus an intermittent filesystem-mock escape (mkdir /fake) before the startup assertion. Focused startup suites pass 103/103 on both branch and base. The full affected Node run finished with 3,080 passes and one provider-installation mock assertion failure. Unchanged baseline finished with 3,039 passes and three failures, including the exact same provider assertion at line 66 (expected one call, received zero); focused provider reruns pass 16/16 on both. Baseline also reproduced the startup filesystem-mock escape and a history-manager timeout. All 14 new English dialog strings pass catalog, extraction and coverage checks. Fresh CI has passed all five Node test shards, both packaging jobs, static/type checks, and Mac/Linux native smoke. Windows native smoke initially timed out rebuilding node-pty before tests; its failed-only retry passed. Fresh aggregate CI is complete at final496fba759b4481104d2938c961b67f63fa6100d9: 38 successful checks and 16 intentional skips, zero pending/failing checks.Review
Detection never restarts processes or replays input. Confirmed recovery is destructive only for the explicitly reviewed owner. Health-capable older peers lacking the new fenced-shutdown RPC decline recovery; updating a client cannot retrofit this capability into an already-running daemon. A degraded current owner also declines recovery so fallback terminals cannot be ended accidentally.
Agent skill upstream boundary
Notes
Local folder workspaces and worktrees use the same owner check. Linux/Windows use platform-independent code, but physical journeys remain untested. SSH/mobile/paired-web do not inspect or restart local owners; web fallback explicitly declines recovery. No new background inventory scan or connection is added; session enumeration occurs only when the user opens the review.
Checklist
496fba759b44; separate headless-server platform workflow also succeeds (38 successful checks, 16 intentional skips, zero pending/failing).