fix(desktop): fix terminal daemon stuck state and add restart button #847
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis change introduces a complete terminal daemon restart capability, including a new Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant UI as Settings UI
participant Router as Terminal Router
participant Client as Terminal Host Client
participant Manager as Daemon Manager
participant Host as Terminal Host
User->>UI: Click "Restart daemon"
UI->>UI: Show confirmation dialog
User->>UI: Confirm restart
UI->>Router: Call restartDaemon mutation
Router->>Client: Connect and authenticate
Client-->>Router: Connected
Router->>Client: List sessions
Client-->>Router: Sessions list
Router->>Client: Shutdown daemon (killSessions: true)
Client->>Host: Kill all sessions
Host-->>Client: Shutdown
Router->>Manager: Reset daemon manager
Manager->>Manager: Clear timers & reset state
Manager->>Manager: Reinitialize client
Router-->>UI: success: true
UI->>UI: Show success notification
UI->>UI: Invalidate session list
UI->>UI: Update tray menu
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
🚀 Preview Deployment🔗 Preview Links
Preview updates automatically with new commits |
Apply clean code principles - remove verbose comments where code is self-explanatory, consolidate debug logging behind DEBUG flags, and simplify function bodies.
- Add reset() method to PrioritySemaphore to clear stuck permits - Call limiter reset in DaemonTerminalManager.reset() - Move markTerminalKilledByUser to onSuccess callback to avoid stale markers if restart fails
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/lib/trpc/routers/terminal/terminal.ts (1)
294-332: Align restart/kill-all log prefixes and hoist polling constants.New log tags like
[killAllDaemonSessions]/[restartDaemon]don’t match the[domain/operation]convention, and the retry constants are defined inside the mutation. Consider hoisting to module-level constants and updating log prefixes for consistency. As per coding guidelines, use the[domain/operation]log pattern and avoid inline magic numbers.♻️ Suggested adjustment
const DEBUG_TERMINAL = process.env.SUPERSET_TERMINAL_DEBUG === "1"; let createOrAttachCallCounter = 0; +const KILL_ALL_MAX_RETRIES = 10; +const KILL_ALL_RETRY_DELAY_MS = 100; @@ - console.log( - "[killAllDaemonSessions] Before kill:", + console.log( + "[terminal/killAllDaemonSessions] Before kill:", beforeIds.length, "sessions", beforeIds, ); - // Poll until sessions are actually dead - const MAX_RETRIES = 10; - const RETRY_DELAY_MS = 100; + // Poll until sessions are actually dead let remainingCount = before.sessions.length; let afterIds: string[] = []; - for (let i = 0; i < MAX_RETRIES && remainingCount > 0; i++) { - await new Promise((resolve) => setTimeout(resolve, RETRY_DELAY_MS)); + for (let i = 0; i < KILL_ALL_MAX_RETRIES && remainingCount > 0; i++) { + await new Promise((resolve) => + setTimeout(resolve, KILL_ALL_RETRY_DELAY_MS), + ); const after = await client.listSessions(); @@ - console.log( - `[killAllDaemonSessions] Retry ${i + 1}/${MAX_RETRIES}: ${remainingCount} sessions still alive`, + console.log( + `[terminal/killAllDaemonSessions] Retry ${i + 1}/${KILL_ALL_MAX_RETRIES}: ${remainingCount} sessions still alive`, afterIds, ); } } - console.log( - "[killAllDaemonSessions] Complete:", + console.log( + "[terminal/killAllDaemonSessions] Complete:", killedCount, "killed,", remainingCount, "remaining", remainingCount > 0 ? afterIds : [], ); @@ - console.log("[restartDaemon] Starting daemon restart..."); + console.log("[terminal/restartDaemon] Starting daemon restart..."); @@ - console.log( - `[restartDaemon] Shutting down daemon with ${aliveCount} alive sessions`, + console.log( + `[terminal/restartDaemon] Shutting down daemon with ${aliveCount} alive sessions`, ); @@ - console.log("[restartDaemon] Daemon was not running"); + console.log("[terminal/restartDaemon] Daemon was not running"); @@ - console.warn( - "[restartDaemon] Error during shutdown (continuing):", + console.warn( + "[terminal/restartDaemon] Error during shutdown (continuing):", error, ); @@ - console.log("[restartDaemon] Complete"); + console.log("[terminal/restartDaemon] Complete");Also applies to: 368-399
🤖 Fix all issues with AI agents
In `@apps/desktop/src/main/lib/terminal/daemon-manager.ts`:
- Around line 1226-1242: In reset(), before clearing this.historyWriters,
perform a best-effort close of each writer like cleanup()/forceKillAll() do:
iterate this.historyWriters.values(), and for each writer call its
close/end/destroy API inside a try/catch (and await if the writer exposes a
promise-based close), flush or write an endedAt timestamp if the writer API used
elsewhere does that, swallow errors and then clear this.historyWriters; this
prevents FD leaks and ensures endedAt is recorded.
- Around line 1292-1295: PrioritySemaphore.reset() currently clears this.queue
allowing pending acquire() promises to hang; change the queue to store both
resolve and reject callbacks and in reset() iterate the queued waiters to call
their reject(...) (e.g., with an Error like "semaphore reset") before clearing
the queue and resetting inUse, so callers awaiting acquire() (such as
createOrAttach()/releaseCreateOrAttach()) receive an immediate rejection instead
of hanging.
🧹 Nitpick comments (1)
apps/desktop/src/renderer/routes/_authenticated/settings/terminal/components/TerminalSettings/TerminalSettings.tsx (1)
935-943: Good pattern: marking sessions only on success.Placing
markTerminalKilledByUserin theonSuccesscallback is the correct approach—if the restart fails, sessions won't be incorrectly marked as user-killed. This aligns with the commit message about preventing stale state on failure.Consider applying the same pattern to
killAllDaemonSessions(lines 790-795) for consistency, where sessions are currently marked before callingmutate().
Kitenite
force-pushed
the
session-not-attachable
branch
2 times, most recently
from
7fe7911 to
92a4b80
Compare
Store reject callback in PrioritySemaphore queue so reset() can reject pending acquire() promises instead of leaving them hanging.
Kitenite
force-pushed
the
session-not-attachable
branch
from
92a4b80 to
1810887
Compare
Summary
killAllbug that could leave sessions stuck in "terminating" stateisAlivecheck after spawn to detect sessions that die immediatelyDaemonTerminalManager.reset()method to properly clear state on restartProblem
Terminals could get into a "Session not attachable" state where all write operations failed. This happened because:
killAllcalledsession.kill()directly without fail-safe timers, leaving sessions stuckSolution
Defense-in-depth approach:
killAllto use proper kill method with fail-safe timersTest plan
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.