Skip to content

fix(desktop): await PTY ready in daemon to fix port detection#782

Merged
Kitenite merged 3 commits into
mainfrom
port-detection
Jan 17, 2026
Merged

fix(desktop): await PTY ready in daemon to fix port detection#782
Kitenite merged 3 commits into
mainfrom
port-detection

Conversation

@Kitenite
Copy link
Copy Markdown
Collaborator

@Kitenite Kitenite commented Jan 17, 2026

Summary

  • Fix port detection not working for terminals when terminal persistence (daemon mode) is enabled
  • The root cause was that createOrAttach returned before the PTY finished spawning, so session.pid was null
  • The null PID was registered with the port manager, and sessions with null PIDs are skipped during port scanning

Changes

  • Await session.waitForReady() before returning from createOrAttach to ensure PID is available
  • Simplify initial commands section since PTY is now guaranteed to be ready after the await

Test plan

  • Enable terminal persistence in settings
  • Open a terminal and run a dev server (e.g., npm run dev or python -m http.server 8000)
  • Verify that the port appears in the ports panel
  • Test that port detection still works when terminal persistence is disabled

Summary by CodeRabbit

  • Bug Fixes
    • Improved terminal startup reliability by awaiting PTY readiness with a timeout and continuing safely when it times out.
    • Added timeout warnings to aid troubleshooting for slow terminal startups.
    • Ensured spawn permits/resources are always released to prevent leaks during launch.
    • Adjusted initial session command execution: commands run only if the session is alive and no extra readiness wait is performed.

✏️ Tip: You can customize this high-level summary in your review settings.

Port detection was not working for terminals when terminal persistence
(daemon mode) was enabled. The root cause was that createOrAttach
returned the response before the PTY finished spawning, so session.pid
was null. The null PID was registered with the port manager, and
sessions with null PIDs are skipped during port scanning.

The fix awaits the PTY ready state before returning, ensuring the PID
is available. This also simplifies the initial commands section since
the PTY is guaranteed to be ready.
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Jan 17, 2026

Caution

Review failed

The pull request is closed.

📝 Walkthrough

Walkthrough

Await PTY readiness with a timeout while holding a spawn permit; log on timeout and proceed (PID may be null). Always release the spawn permit in a finally block. Execute initialCommands after readiness handling only if the session is alive; minor comment updates.

Changes

Cohort / File(s) Summary
PTY readiness & spawn permit
apps/desktop/src/main/terminal-host/terminal-host.ts
Replaced hold-spawn-permit flow with explicit await of PTY readiness (with timeout) and timeout warning; continue on timeout (PID may be null); always release spawn permit in finally; run initialCommands after readiness handling and guard writes with session-alive checks. (+21/-25)

Sequence Diagram

sequenceDiagram
    participant TerminalHost as TerminalHost
    participant SpawnPermit as SpawnPermit
    participant PTY as PTY
    participant Session as Session

    TerminalHost->>SpawnPermit: acquire()
    TerminalHost->>PTY: await readiness (with timeout)
    alt PTY ready
        PTY-->>TerminalHost: ready
    else timeout
        TerminalHost->>TerminalHost: log timeout warning
        TerminalHost-->>TerminalHost: continue (PID may be null)
    end
    TerminalHost->>Session: isAlive?
    alt session alive
        TerminalHost->>Session: execute initialCommands (write)
    end
    TerminalHost->>SpawnPermit: release()
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Poem

🐰 I waited for the PTY, soft paws in line,
A timeout chimed, I logged the sign,
Permit let go with a courteous hop,
Commands sent only when sessions stop not,
Hopping onward — brief, tidy, fine.

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change: awaiting PTY ready in daemon mode to fix port detection, which directly relates to the primary change in the changeset.
Description check ✅ Passed The description covers the required sections including clear summary of changes, test plan, and related issue context, meeting template expectations.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

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

✨ Finishing touches
  • 📝 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.

❤️ Share

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

@Kitenite Kitenite merged commit 4a3f672 into main Jan 17, 2026
3 of 5 checks passed
@Kitenite Kitenite deleted the port-detection branch January 17, 2026 00:51
@github-actions
Copy link
Copy Markdown
Contributor

🧹 Preview Cleanup Complete

The following preview resources have been cleaned up:

  • ⚠️ Neon database branch
  • ⚠️ Electric Fly.io app

Thank you for your contribution! 🎉

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.

1 participant