fix: Properly terminate PTY processes when closing tabs

[kshivang]

Summary

• Fixes orphaned processes when closing terminal tabs
• The kill() method now properly terminates PTY processes by:
  1. Closing streams first to signal EOF to child process
  2. Calling destroy() for graceful termination
  3. Waiting up to 2 seconds for process to exit
  4. Falling back to destroyForcibly() if still running

Root Cause

PtyProcessHandle.kill() only called process.destroy() without closing streams or waiting for termination. The pty4j library's destroy() doesn't reliably terminate processes without explicit stream closure.

Test plan

• Start BossTerm, open new tab
• Run long-running task (e.g., sleep 300)
• Close the tab
• Verify process is terminated (ps aux | grep sleep)

🤖 Generated with Claude Code

[claude]

Code Review: PR #181 - Fix PTY Process Termination

Summary

This PR addresses orphaned processes when closing terminal tabs by improving the PtyProcessHandle.kill() method. The changes add proper stream closure, graceful termination waiting, and fallback to force-kill.

---

✅ Strengths

1. Correct Problem Diagnosis: The PR correctly identifies that pty4j's destroy() doesn't reliably terminate processes without explicit stream closure.

2. Defense in Depth: The implementation has multiple layers of termination:

   • Stream closure (signals EOF)
   • Graceful destroy()
   • Wait with timeout (2 seconds)
   • Force kill fallback
   • Exception handling with last-resort force kill

3. Proper Exception Handling: All potentially failing operations are wrapped in try-catch blocks to prevent exceptions from blocking cleanup.

4. Reasonable Timeout: 2-second timeout is appropriate for graceful termination without making tab closure feel sluggish.

---

🔍 Potential Issues & Improvements

1. Race Condition with Concurrent Read Operations

Severity: Medium

The kill() method closes streams while another coroutine may be actively reading from them (TabController.kt:869-870, 1008-1009):

// Output reading coroutine (Dispatchers.IO)
while (handle.isAlive()) {
    val output = handle.read()  // May be blocked here
    // ...
}

Issue: When kill() closes inputStream, the concurrent read() operation may:

• Throw an exception (caught and returns null)
• Return partial data
• Enter an undefined state

Recommendation: Add synchronization or flag to coordinate shutdown:

private val isShuttingDown = AtomicBoolean(false)

override suspend fun read(): String? {
    if (isShuttingDown.get()) return null
    return try {
        // existing read logic
    } catch (e: Exception) {
        if (isShuttingDown.get()) null else throw e
    }
}

override suspend fun kill() {
    isShuttingDown.set(true)
    // existing kill logic
}

2. Blocking Wait on Potentially Coroutine-Unsafe Thread

Severity: Low

The kill() method is marked suspend and uses withTimeoutOrNull with delay(50), which is correct. However, consider whether callers are on the right dispatcher:

// TabController.kt:1143
GlobalScope.launch(Dispatchers.IO) {
    processToKill.kill()
}

Analysis: This is actually fine - using Dispatchers.IO is appropriate for potentially blocking operations. The code is correct as-is.

3. Silent Exception Swallowing

Severity: Low

The catch blocks use catch (_: Exception) which suppresses all exception information:

try { outputStream.close() } catch (_: Exception) {}

Recommendation: Consider logging exceptions for debugging:

try { 
    outputStream.close() 
} catch (e: Exception) { 
    println("WARN: Error closing output stream: \${e.message}")
}

This would help diagnose issues in production without affecting functionality.

4. Potential Double Force-Kill

Severity: Negligible

If destroyForcibly() throws an exception on line 226, the outer catch block will call it again on line 230. This is harmless but slightly redundant.

---

🧪 Test Coverage

Missing: No automated tests verify:

• Process termination succeeds within timeout
• Force-kill fallback works when graceful termination fails
• No exceptions escape kill() method
• Concurrent read operations handle stream closure gracefully

Recommendation: Add integration test:

@Test
fun testProcessTermination() = runBlocking {
    val config = ProcessConfig(...)
    val handle = processService.spawnProcess(config)
    
    // Verify process starts
    assertTrue(handle.isAlive())
    
    // Kill and verify termination
    handle.kill()
    assertFalse(handle.isAlive())
    
    // Verify read returns null after kill
    assertNull(handle.read())
}

---

🔒 Security Considerations

No security issues identified. The PR:

• Doesn't introduce command injection risks
• Properly cleans up resources
• Doesn't expose sensitive information
• Uses appropriate privilege levels

---

📊 Performance Considerations

Impact: Minimal - adds 2-second maximum delay to tab closure, but only when processes don't terminate gracefully.

Optimization opportunity: The 50ms polling interval in the wait loop could be increased to 100ms to reduce CPU overhead without noticeable UX impact:

kotlinx.coroutines.delay(100)  // Instead of 50

---

📝 Code Quality

✅ Follows Kotlin conventions
✅ Clear comments explaining the termination sequence
✅ Appropriate use of coroutines
✅ Consistent with CLAUDE.md guidelines (no backward-compatibility hacks, straightforward implementation)

⚠️ Minor: Consider extracting magic numbers to constants:

private const val GRACEFUL_TERMINATION_TIMEOUT_MS = 2000L
private const val TERMINATION_CHECK_INTERVAL_MS = 50L

---

🎯 Verdict

Recommendation: Approve with minor suggestions

The PR successfully solves the orphaned process problem and uses a sound multi-layered approach. The race condition concern is real but likely low-impact in practice since:

1. The read operation already handles exceptions and returns null
2. The isAlive() check in the while loop will exit after kill
3. Stream closure naturally signals EOF

Optional improvements (not blockers):

1. Add synchronization flag to coordinate shutdown
2. Add logging for debugging
3. Extract magic numbers to constants
4. Add integration tests

The current implementation is production-ready and significantly better than the previous single destroy() call.

---

🤖 Generated with Claude Code

[claude]

Code Review - PR #181: Fix PTY Process Termination

Overview

This PR addresses a critical issue where terminal tabs left orphaned processes after closing. The fix improves the PtyProcessHandle.kill() method to properly terminate PTY processes through a multi-step shutdown sequence.

---

✅ Strengths

1. Comprehensive Shutdown Sequence: The implementation follows best practices for process termination:

   • Closes streams first (signals EOF)
   • Attempts graceful termination with destroy()
   • Waits with timeout (2 seconds)
   • Falls back to destroyForcibly() if needed

2. Race Condition Mitigation: The isShuttingDown flag elegantly coordinates between kill() and concurrent read() operations, preventing exceptions during shutdown.

3. Error Resilience: Multiple layers of exception handling ensure the process terminates even if individual steps fail.

4. Clear Documentation: Well-commented code explains the purpose of each step.

5. Suspend Function: Properly uses suspend for the async operations, maintaining the coroutine contract.

---

🔍 Observations & Suggestions

1. Potential Deadlock Scenario (Minor)

Location: Lines 234-239

val exited = kotlinx.coroutines.withTimeoutOrNull(2000) {
    while (process.isAlive) {
        kotlinx.coroutines.delay(50)
    }
    true
}

Issue: If the read() method is blocked on inputStream.read(buffer) when kill() is called, closing the streams (line 228) should cause read() to throw an exception and return null. However, there's a theoretical race where:

• read() checks isShuttingDown.get() (line 129) → false
• kill() sets isShuttingDown to true (line 223)
• read() enters inputStream.read(buffer) and blocks (line 133)
• kill() closes streams (line 228)
• Stream closure should unblock read(), but timing is OS-dependent

Suggestion: This is likely fine in practice since stream closure should interrupt the read, but you could add a comment noting this dependency:

// Wait briefly for graceful termination (up to 2 seconds)
// Stream closure should unblock any concurrent read() operations
val exited = kotlinx.coroutines.withTimeoutOrNull(2000) { ...

2. Dispatcher Context (Minor)

Location: Line 221

The kill() method doesn't specify a dispatcher. Since it performs I/O operations (closing streams, waiting for process), consider:

override suspend fun kill() = kotlinx.coroutines.withContext(kotlinx.coroutines.Dispatchers.IO) {
    // ... existing code
}

However: Looking at the call sites (TabController.kt:1145, 1237), they're already launched with Dispatchers.IO, so this is already handled. The current approach is fine.

3. UTF-8 Buffer Handling During Shutdown (Minor)

Location: Line 114 (incompleteUtf8Buffer)

When kill() is called, any incomplete UTF-8 bytes in incompleteUtf8Buffer are abandoned. This is acceptable since the process is terminating, but worth noting for completeness.

Status: No action needed - this is the correct behavior.

4. Test Coverage (Suggestion)

The PR mentions manual testing, which is appropriate for this type of change. However, consider adding an automated test:

@Test
fun testProcessKillTerminatesOrphanedProcesses() = runBlocking {
    val processHandle = DesktopProcessService().spawnProcess(
        ProcessConfig(command = "sleep", arguments = listOf("300"))
    )
    assertNotNull(processHandle)
    assertTrue(processHandle!!.isAlive())
    
    val pid = processHandle.getPid()
    processHandle.kill()
    
    delay(2500) // Wait for kill timeout + buffer
    assertFalse(processHandle.isAlive())
    
    // Verify process is actually dead at OS level
    if (pid != null) {
        val osProcess = Runtime.getRuntime().exec("ps -p $pid")
        val exitCode = osProcess.waitFor()
        assertEquals(1, exitCode) // ps returns 1 if process not found
    }
}

---

🔒 Security Considerations

✅ No security issues identified. The change doesn't introduce:

• Command injection vulnerabilities
• Resource leaks (properly handles exceptions)
• Privilege escalation issues

---

⚡ Performance Considerations

✅ Minimal impact:

• Added 2-second timeout is acceptable for cleanup operations
• AtomicBoolean has negligible overhead
• Stream closure and process termination are already I/O-bound

---

📊 Alignment with CLAUDE.md Guidelines

✅ Code Quality: Clear comments, no unnecessary complexity
✅ Error Handling: Multiple fallback layers
✅ No Backwards-Compatibility Hacks: Clean, straightforward implementation
✅ Coroutine Best Practices: Proper use of suspend, timeouts, and delays

---

🎯 Verdict

LGTM with minor suggestions!

This is a well-thought-out fix that addresses the root cause of orphaned processes. The implementation is robust, well-documented, and follows Kotlin/coroutine best practices. The suggestions above are optional refinements—the PR is ready to merge as-is.

Recommendation: ✅ Approve and merge

---

📝 Suggested Commit Message (Already Good!)

The commit message is excellent—it clearly explains the problem, root cause, and solution. No changes needed.

🤖 Review generated with Claude Code

[claude]

Code Review - PR #181: Properly terminate PTY processes when closing tabs

Summary

This PR addresses a critical resource leak where PTY processes were left orphaned when tabs closed. The fix implements a proper shutdown sequence with stream closure, graceful termination, and fallback to forced kill.

---

✅ Strengths

1. Robust Process Termination Strategy

The new kill() method implements a defensive multi-layer approach:

• Stream closure first - Signals EOF to child process
• Graceful termination - Calls destroy() and waits up to 2 seconds
• Force kill fallback - Uses destroyForcibly() if process is still alive
• Exception safety - Triple try-catch ensures cleanup even on errors

This is excellent defensive programming for process management.

2. Race Condition Prevention

The AtomicBoolean isShuttingDown flag elegantly prevents race conditions between:

• read() operations blocked on inputStream.read()
• kill() closing streams

The early exit in read() (line 129) prevents exceptions from closed streams during shutdown.

3. Split Pane Process Cleanup

The TabbedTerminal.kt changes properly handle split panes:

• Identifies orphaned split states when tabs close
• Extracts process references before disposal
• Disposes split state (cancels coroutines, closes channels)
• Kills all split processes in correct coroutine context

This prevents resource leaks for advanced users with split panes.

4. Consistent with Existing Patterns

The implementation mirrors TabController.closeTab() and disposeAll() patterns:

• Holds process references before disposal (prevents GC issues)
• Uses Dispatchers.IO for blocking operations
• Includes error handling with warning messages

---

⚠️ Concerns & Suggestions

1. Silent Exception Swallowing in read() (Medium Priority)

Location: PlatformServices.desktop.kt:167-174

} catch (e: Exception) {
    // During shutdown, exceptions are expected - return null silently
    // Otherwise, this might be a real error worth noting
    if (!isShuttingDown.get()) {
        // Could log here if debugging is needed
    }
    null
}

Issue: Non-shutdown exceptions are silently swallowed without logging. This could hide legitimate errors like:

• I/O errors from the PTY
• UTF-8 decoding failures
• Network issues (if PTY is remote)

Recommendation: Add minimal logging for non-shutdown errors:

if (!isShuttingDown.get()) {
    println("WARN: Error reading from PTY: \${e.message}")
}

2. Potential Deadlock in kill() (Low-Medium Priority)

Location: PlatformServices.desktop.kt:234-239

val exited = kotlinx.coroutines.withTimeoutOrNull(2000) {
    while (process.isAlive) {
        kotlinx.coroutines.delay(50)
    }
    true
}

Issue: This polling loop runs on the calling coroutine context (likely Dispatchers.IO from callers). However:

• If called from a different dispatcher (e.g., Dispatchers.Default), the delay() might not behave as expected
• The 50ms polling interval wastes CPU cycles

Recommendation: Use Process.waitFor(timeout) for more efficient blocking:

val exited = kotlinx.coroutines.withTimeoutOrNull(2000) {
    kotlinx.coroutines.withContext(kotlinx.coroutines.Dispatchers.IO) {
        process.waitFor()
    }
    true
}

This blocks the thread efficiently instead of polling.

3. Missing Resource Cleanup Order (Low Priority)

Location: TabbedTerminal.kt:247-262

Issue: The cleanup sequence is:

1. Dispose split state (cancels coroutines, closes channels)
2. Kill processes

However, disposing might trigger additional writes to closed processes. Better order:

1. Kill processes first (stop new I/O)
2. Then dispose split state (clean up coroutines/channels)

Recommendation: Reverse the order:

// Kill all processes first
for (process in processes) {
    try {
        kotlinx.coroutines.withContext(kotlinx.coroutines.Dispatchers.IO) {
            process.kill()
        }
    } catch (e: Exception) {
        println("WARN: Error killing split process: \${e.message}")
    }
}
// Then dispose the split state
splitState.dispose()

4. Inconsistent Dispatcher Usage (Low Priority)

Location: TabbedTerminal.kt:255

kotlinx.coroutines.withContext(kotlinx.coroutines.Dispatchers.IO) {
    process.kill()
}

Issue: The LaunchedEffect already runs in a coroutine context, but the dispatcher isn't explicit. Since kill() does blocking I/O (stream closure, process wait), explicitly using Dispatchers.IO is correct, but consider documenting why.

Recommendation: Add a comment explaining the dispatcher choice:

// Use Dispatchers.IO for blocking process termination
kotlinx.coroutines.withContext(kotlinx.coroutines.Dispatchers.IO) {
    process.kill()
}

---

🔒 Security Considerations

✅ No Security Issues Found

• Stream closure before process destruction prevents potential data leakage
• Forced termination ensures processes can't escape cleanup
• AtomicBoolean provides thread-safe shutdown signaling
• No command injection or unsafe process spawning introduced

---

🧪 Testing Recommendations

Manual Tests (As specified in PR)

✅ The PR includes a solid manual test plan:

• Start BossTerm, open new tab
• Run long-running task (sleep 300)
• Close the tab
• Verify process is terminated (ps aux | grep sleep)

Additional Test Cases

1. Rapid tab closure: Open 10 tabs, close all quickly - verify no orphaned processes
2. Split panes: Create split pane, run sleep 300 in each, close tab - verify all processes killed
3. Force kill scenario: Run sleep 300 & sleep 300 & sleep 300 (background jobs), close tab - verify all die within 2 seconds
4. Graceful shutdown: Run vim, close tab - verify vim exits gracefully (saves swap files)
5. Process tree: Run bash -c 'sleep 300', close tab - verify child sleep is also killed

Automated Test Suggestions (Future Work)

@Test
fun testProcessKilledOnTabClose() {
    val tab = createTab(command = "sleep 300")
    val pid = tab.processHandle.value?.getPid()
    assertNotNull(pid)
    assertTrue(isProcessAlive(pid))
    
    tab.dispose()
    tab.processHandle.value?.kill()
    
    // Wait for termination
    Thread.sleep(2500)
    assertFalse(isProcessAlive(pid))
}

---

📊 Performance Impact

Positive

• 2-second timeout is reasonable (not too aggressive, not too lenient)
• Early exit in read() prevents wasted I/O operations during shutdown
• Stream closure signals EOF efficiently to child process

Neutral

• 50ms polling adds ~40 cycles max (2000ms / 50ms), acceptable overhead
• AtomicBoolean has negligible performance cost

---

🎯 Final Recommendation

APPROVE with minor suggestions

This is a high-quality fix that addresses a critical resource leak. The implementation is defensive, well-documented, and follows existing patterns in the codebase.

Must Fix (Before Merge)

• None - the implementation is production-ready

Should Fix (Post-Merge)

1. Add error logging for non-shutdown exceptions in read()
2. Replace polling loop with Process.waitFor() for efficiency
3. Reverse cleanup order in TabbedTerminal.kt (kill processes → dispose state)

Nice to Have

• Add automated tests for process lifecycle
• Document dispatcher usage in TabbedTerminal.kt

---

Great work on this fix! The orphaned process issue was a significant UX problem, and this PR resolves it comprehensively. 🎉

---

Reviewed by: Claude Code (Sonnet 4.5)
Review Date: 2025-12-28

[claude]

Code Review - PR #181: Fix PTY Process Termination

Summary

This PR addresses a critical issue where PTY processes were not properly terminated when closing terminal tabs, leading to orphaned processes. The solution implements a proper shutdown sequence with graceful termination and fallback to force kill.

---

✅ Strengths

1. Robust Process Termination Logic

The new kill() implementation follows best practices:

• Closes I/O streams first to signal EOF to child processes
• Attempts graceful shutdown with destroy()
• Waits up to 2 seconds for process exit
• Falls back to destroyForcibly() if still running
• Multiple exception handlers ensure cleanup always completes

2. Race Condition Prevention

• AtomicBoolean isShuttingDown flag coordinates between kill() and read()
• read() early exit prevents blocked I/O during shutdown
• Silent exception handling during shutdown avoids noisy error logs

3. Split Pane Process Cleanup

The TabbedTerminal.kt changes properly handle split panes:

• Kills all processes in split sessions before disposing split state
• Uses Dispatchers.IO for blocking process termination
• Prevents new I/O after kill() is called

---

🔍 Potential Issues & Suggestions

Issue 1: Nested withContext in kill() method ⚠️

Location: PlatformServices.desktop.kt:234-237

val exited = kotlinx.coroutines.withTimeoutOrNull(2000) {
    kotlinx.coroutines.withContext(kotlinx.coroutines.Dispatchers.IO) {
        process.waitFor(2, java.util.concurrent.TimeUnit.SECONDS)
    }
}

Problem: process.waitFor() is already blocking, so withContext(Dispatchers.IO) is redundant and adds overhead. The withTimeoutOrNull already handles the timeout.

Recommendation: Simplify to:

val exited = kotlinx.coroutines.withTimeoutOrNull(2000) {
    kotlinx.coroutines.Dispatchers.IO.run {
        process.waitFor(2, java.util.concurrent.TimeUnit.SECONDS)
    }
}

Or even simpler, since waitFor() with timeout is non-blocking:

// waitFor() already has built-in timeout, just use it directly
val exited = process.waitFor(2, java.util.concurrent.TimeUnit.SECONDS)

Issue 2: Potential Resource Leak in TabbedTerminal.kt ⚠️

Location: TabbedTerminal.kt:249-262

for (process in processes) {
    try {
        kotlinx.coroutines.withContext(kotlinx.coroutines.Dispatchers.IO) {
            process.kill()
        }
    } catch (e: Exception) {
        println("WARN: Error killing split process: ${e.message}")
    }
}
splitState.dispose()

Problem: If multiple processes exist and one throws an exception, the loop continues but splitState.dispose() may be called before all kill operations complete. Since kill() is now suspend, concurrent kills may not finish.

Recommendation: Use supervisorScope or coroutineScope with async to wait for all kills:

// Kill all processes concurrently, wait for all to complete
coroutineScope {
    processes.map { process ->
        async(Dispatchers.IO) {
            try {
                process.kill()
            } catch (e: Exception) {
                println("WARN: Error killing split process: ${e.message}")
            }
        }
    }.awaitAll()
}
// Now safe to dispose
splitState.dispose()

Issue 3: Stream Closure Before Read Completion ℹ️

Location: PlatformServices.desktop.kt:226-227

try { outputStream.close() } catch (_: Exception) {}
try { inputStream.close() } catch (_: Exception) {}

Observation: Closing inputStream while read() is blocked will cause an exception. The isShuttingDown flag handles this gracefully, but there's a tiny race condition window:

1. read() checks isShuttingDown (false) and proceeds
2. kill() sets isShuttingDown to true
3. kill() closes streams
4. read() blocks on inputStream.read() and throws exception

Impact: Low - exception is caught and handled correctly, but logged as "WARN: Error reading from PTY"

Optional Enhancement: Add a second check after the read:

override suspend fun read(): String? {
    if (isShuttingDown.get()) return null
    
    return try {
        val buffer = ByteArray(8192)
        val len = inputStream.read(buffer)
        if (len <= 0 || isShuttingDown.get()) return null  // <-- Add check here
        // ... rest of processing
    } catch (e: Exception) {
        if (!isShuttingDown.get()) {
            println("WARN: Error reading from PTY: ${e.message}")
        }
        null
    }
}

---

🔒 Security Considerations

✅ No security concerns identified:

• Process termination is properly sandboxed
• No command injection risks
• Stream closure is exception-safe

---

🧪 Test Coverage

Test Plan (from PR description):

• ✅ Manual testing described for long-running tasks
• ✅ Process verification with ps aux | grep sleep

Suggestion: Consider adding:

1. Unit test: Mock PtyProcess and verify kill sequence
2. Integration test: Create tab with process, close, verify no orphans
3. Split pane test: Open split with multiple processes, close tab, verify cleanup

---

📊 Performance Considerations

✅ Good:

• 2-second timeout prevents indefinite blocking
• Efficient blocking with process.waitFor() instead of polling
• Proper dispatcher usage (Dispatchers.IO) for blocking operations

⚠️ Minor concern:

• Nested coroutine contexts add small overhead (see Issue 1)

---

🎯 Recommendations Summary

Must Fix:

None - code is functional as-is

Should Consider:

1. Simplify nested withContext in kill() method (Issue 1)
2. Ensure all split pane processes are killed concurrently and awaited (Issue 2)

Nice to Have:

1. Add second isShuttingDown check after inputStream.read() (Issue 3)
2. Add automated tests for process cleanup

---

✅ Overall Assessment

Quality: ⭐⭐⭐⭐ (4/5)

• Solid solution to a real problem
• Handles edge cases well
• Minor optimization opportunities

Approval: ✅ Approved with suggestions

This is a well-thought-out fix that addresses the root cause. The suggested improvements are optimizations rather than blockers. Great work! 🚀

🤖 Generated with Claude Code

[claude]

Code Review: PR #181 - Fix Orphaned PTY Processes

Overview

This PR addresses a critical resource leak where PTY processes were not being properly terminated when closing terminal tabs. The solution implements a robust shutdown sequence with proper stream closure, graceful termination, and fallback force-kill.

---

✅ Strengths

1. Proper Shutdown Sequence (PlatformServices.desktop.kt:220-244)

   • Closes streams first to signal EOF (critical for pty4j)
   • Attempts graceful termination with destroy()
   • Waits up to 2 seconds for process exit
   • Falls back to destroyForcibly() if still running
   • Multiple exception handling layers ensure cleanup always occurs

2. Race Condition Prevention (PlatformServices.desktop.kt:120, 129)

   • isShuttingDown AtomicBoolean coordinates between kill() and read()
   • Early exit in read() prevents blocked I/O operations during shutdown
   • Silences expected exceptions during shutdown (lines 168-172)

3. Split Pane Process Cleanup (TabbedTerminal.kt:245-262)

   • Proactively kills all split pane processes before disposing state
   • Prevents orphaned processes in complex split layouts
   • Uses withContext(Dispatchers.IO) for proper coroutine context

4. Backwards Compatibility

   • No breaking API changes
   • Existing callers automatically benefit from improved behavior

---

🔍 Potential Issues & Recommendations

1. Blocking Call on Main/Composition Thread (TabbedTerminal.kt:253-254)

Severity: Medium

kotlinx.coroutines.withContext(kotlinx.coroutines.Dispatchers.IO) {
    process.kill()
}

This blocks the composition coroutine for up to 2 seconds per process (from waitFor(2, TimeUnit.SECONDS) in kill()). With multiple split panes, this could freeze the UI.

Recommendation:

// Launch kills concurrently instead of sequentially
val killJobs = processes.map { process ->
    scope.launch(Dispatchers.IO) {
        try {
            process.kill()
        } catch (e: Exception) {
            println("WARN: Error killing split process: $e")
        }
    }
}
killJobs.joinAll() // Wait for all kills to complete

2. Redundant null-check (TabbedTerminal.kt:249)

Severity: Minor

val processes = splitState.getAllSessions().mapNotNull { it.processHandle?.value }

The mapNotNull handles the null case, making the explicit null continuation in the loop unnecessary. This is fine but could be simplified for clarity.

3. Missing Timeout on Split Process Kill (TabbedTerminal.kt:245-262)

Severity: Low

The entire cleanup loop has no overall timeout. If multiple processes hang during kill (despite the 2-second per-process timeout), the UI could freeze for an extended period.

Recommendation:

withTimeout(5000L) { // 5 second total timeout
    val killJobs = processes.map { /* concurrent kills */ }
    killJobs.joinAll()
}

4. Stream Close Order (PlatformServices.desktop.kt:225-227)

Severity: Low (theoretical)

The code closes outputStream before inputStream. While this works, the standard pattern is to close output first (to signal no more input will come), then input. However, since both use swallowed exceptions, this is not a practical issue.

Current (fine):

try { outputStream.close() } catch (_: Exception) {}
try { inputStream.close() } catch (_: Exception) {}

Slight improvement (explicit order documentation):

// Close output first to signal EOF to child process
try { outputStream.close() } catch (_: Exception) {}
// Then close input stream
try { inputStream.close() } catch (_: Exception) {}

---

🔒 Security Considerations

✅ No security concerns identified

• Process kill operations are properly scoped to owned processes
• No user input affects the kill logic
• Exception handling prevents information leakage

---

🧪 Test Coverage

Strengths:

• Manual test plan covers the basic scenario (single tab, long-running process)

Recommendations:
Consider testing these edge cases:

1. Multiple split panes - Verify all processes terminate
2. Rapid tab creation/closure - Stress test for race conditions
3. Unresponsive processes - Test force-kill fallback (e.g., trap '' SIGTERM; sleep 300)
4. Concurrent operations - Close tab while actively reading/writing data

---

📊 Performance Impact

Positive:

• Eliminates resource leak (orphaned processes)
• 2-second graceful shutdown timeout is reasonable

Consideration:

• Sequential kills in split pane cleanup could delay UI responsiveness (see Issue feat: Terminal rendering improvements + 4 high-priority features (#2, #3, #4, #5) #1 above)

---

🎯 Recommendation: APPROVE with Minor Improvements

This PR solves a critical bug and implements a robust solution. The core logic is sound. I recommend:

1. Must fix (before merge): Make split process kills concurrent (Issue feat: Terminal rendering improvements + 4 high-priority features (#2, #3, #4, #5) #1) to avoid UI freezes
2. Nice to have: Add overall timeout to split cleanup loop (Issue 🔗 Implement hyperlink detection and click handling #3)
3. Optional: Add comment clarifying stream close order (Issue ⚙️ Implement user settings and configuration system #4)

Great work on the thorough shutdown sequence and race condition handling!

---

📝 Code Quality

• ✅ Clear comments explaining shutdown coordination
• ✅ Proper exception handling at multiple layers
• ✅ Follows Kotlin conventions
• ✅ No backwards-compatibility hacks (matches CLAUDE.md guidelines)

Files: PlatformServices.desktop.kt:105-244, TabbedTerminal.kt:241-263