Repository navigation
Conversation
A retained orchestration worker's pane re-attaches renderer-only after an app restart, so the daemon adapter's hasPty cache never learns the session. The visibility reconciler tore the tab down on that fabricated false while the daemon still ran the process (#16904 regression: settled workers now enter the retained path). pty:hasPty now asks the provider's readback before answering absent; a cache hit stays a cheap sync answer.
📝 WalkthroughWalkthroughThe change adds daemon-backed PTY presence resolution for cache misses. Disconnected daemon clients reconnect before liveness probes. IPC tests cover cache hits, daemon readback, unreachable daemons, and absent sessions. A new end-to-end test verifies that a settled worker tab and its PTY remain available after an application restart. Merge Risk: 🔵 Low · up to The restart test can pass without confirming that revealing the worker tab restores its terminal binding. Tighten this assertion before merge to preserve regression coverage. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description explains the problem, root cause, fix, scope, and verification in detail. However, it omits required template content, including the Linked Issue entry, Visual Proof or an explicit N/A statement, the Testing checklist, and the remaining checklist sections. Resolution Add the required template sections. Include a valid issue reference after “Fixes #”, provide before-and-after visual proof or write “N/A” with a reason, complete the Testing checklist, and address the Agent skill upstream boundary, Notes, and final Checklist items.
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Team
Run ID: 50deaace-1ee7-49d6-a24b-47d84796ae2b
📒 Files selected for processing (7)
src/main/daemon/daemon-pty-adapter-protocol-compatibility.test.tssrc/main/daemon/daemon-pty-adapter-unattached-presence.test.tssrc/main/daemon/daemon-pty-session-control.tssrc/main/ipc/pty-has-pty-cache-miss-presence.test.tssrc/main/ipc/pty/ipc/inspect.tssrc/main/ipc/pty/provider/liveness.tstests/e2e/settled-worker-tab-survives-restart.spec.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 5 remain after this review.
| expect(transitions.at(-1)?.activeWorktreeId).toBe(targetWorktreeId) | ||
| expect( | ||
| await second.page.evaluate((tabId) => Boolean(window.__paneManagers?.get(tabId)), workerTabId) | ||
| ).toBe(true) | ||
| expect( | ||
| (await listRuntimeTerminals(client)).find((terminal) => terminal.ptyId === workerPtyId) | ||
| ?.connected | ||
| ).toBe(true) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the renderer PTY binding after reveal.
backgroundMountTab mounts workerTabId without changing selection. The pane-manager and runtime-terminal checks do not prove that the reveal selected the worker tab or bound its active pane to workerPtyId. Assert activeTabIdByWorktree[targetWorktreeId] equals workerTabId, then compare await waitForActivePanePtyId(second.page) with workerPtyId.
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
fix(pty): answer pty:hasPty from the daemon readback on a cache miss—pty:hasPtynow resolves presence throughresolveProviderPtyPresence, so a daemon-adapter cache miss consults the authoritativeprobePtyLivenessreadback instead of fabricating afalse; a cache hit stays a sync answer and a provider without a readback keeps its cache authoritative.ensureConnectedguard inDaemonPtyAdapter.probePtyLiveness— a fresh app process connects to the daemon before probing so it reads real liveness for sessions an older process spawned, rather than answeringnullfrom an empty attach cache.- New unit coverage (
daemon-pty-adapter-unattached-presence,pty-has-pty-cache-miss-presence) plus an e2e restart/reveal spec that asserts the settled worker tab never leaves the store.
I verified the boundary contract end to end. The false fallback in resolveProviderPtyPresence (for a provider with hasPty but no probePtyLiveness) is only reachable by LocalPtyProvider (whose cache is the process table, so false is authoritative) and SshPtyProvider (which has no readback and whose behavior is unchanged); the daemon router/adapter both expose probePtyLiveness. The daemon's getSize readback returns null only for a missing session, so the probe cannot report a false-absent for a live PTY. Verdict vocabulary is preserved: false only on an owner readback, null when unverifiable. No daemon wire/protocol change is introduced (getSize/listSessions already exist), so there is no remote-wire compatibility concern.
ℹ️ probePtyLiveness now establishes a connection, not just a read
The new if (!this.client.isConnected()) guard means a "liveness probe" is no longer a pure RPC against an open socket — on a fresh adapter it performs the full connection lifecycle (setupEventRouting, scheduleCheckpointTimer, background resync) via the adapter-level ensureConnected. I traced the other callers (reapRestoredSubagentsWithoutLiveAgent, the runtime controller, and DaemonPtyRouter.ownerResolver.probe) and all are either already connected or safe to connect, and the connect is bounded by LIVENESS_PROBE_TIMEOUT_MS. Noting only because a method named "probe" having connection side effects is worth keeping in mind for future callers; nothing requires a change.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
|
Superseded by #19358, which fixes the same P0+ at the owning layer (main's pty:spawn attach path) instead of guarding pty:hasPty. Branch kept for reference. |
1 participant
Release blocker from the Slack thread "disappearing orchestrator-worker tabs" (P0+). Clicking a completed orchestration worker's tab after an app restart removed the tab while the daemon still ran the process. Regression trigger: #16904, which extended the legacy-worker resume fence to settled workers whose terminal is still
ownedand neither released nor retained.ELI5
After a restart, a finished worker's tab re-attaches to its live shell without telling main's daemon adapter. When the user reveals the tab, the renderer asks main "is this PTY alive?" Main looked only in its own attach cache, found nothing, and said "no" with full confidence. The renderer treats that "no" as a death certificate and closes the tab.
Root cause
pty:hasPtyanswered fromDaemonPtyAdapter.hasPty, an in-process cache of the sessions this process attached.automaticResumeBlockedBy: legacy-orchestration-worker) pane takes the renderer-onlyattachRetainedLegacyPtypath, which never warms that cache.shouldReconcileMissingSessionreconciles on exactlyfalse, so the cache miss became tab teardown on hidden→visible.Before #16904 settled workers never entered the fenced path, so the latent cache/attach mismatch was unreachable for them.
Fix
pty:hasPtynow resolves presence through the provider's authoritative readback (probePtyLiveness) whenever the cache misses. A cache hit stays a sync answer; a provider without a readback (in-processLocalPtyProvider) keeps its cache as the process table; an unreachable daemon answersnull, neverfalse.DaemonPtyAdapter.probePtyLivenessconnects first when the adapter has never connected, otherwise a fresh app process would answernullfor every session an older process spawned.Verdict vocabulary unchanged: only an owner's readback may say
false(docs/reference/ssh-execution-boundary.md).Verification
src/main/ipc/pty-has-pty-cache-miss-presence.test.ts: cache miss → readback true/false/null; cache hit skips the readback; readback-less sole provider stays authoritative.src/main/daemon/daemon-pty-adapter-unattached-presence.test.ts: real daemon over a temp socket, a second adapter that never attached answers live / absent / absent-after-exit.tests/e2e/settled-worker-tab-survives-restart.spec.ts: starts a real worker, settles it withworker_done, verifies the fence stamped, quits and relaunches the app on the same profile, reveals the worker's worktree, and asserts no store transition ever dropped the tab or its leaf binding.origin/main'sinspect.ts: fails on both assertions (hasPty→false, transitions show leaf cleared → tab removed → worktree deselected → tab re-added by graph sync), which matches the videos in the thread.reproduce-findings-tab-disappearance.test.ts) passes when routed through the new helper; it is not committed because it depends ongit showof historical refs.pnpm tc:node,check:code-quality:changed, and 556 test files acrosssrc/main/ipc,src/main/daemon, renderer reconcile, and worker-recovery suites pass.Not changed