From 67f42150b40cce8710f4f09308b16d6569aaffb4 Mon Sep 17 00:00:00 2001
From: Pritom14 <pritommazumdar1995@gmail.com>
Date: Sun, 31 May 2026 20:46:12 +0530
Subject: [PATCH] fix(terminal): make creackPTY.Close idempotent to avoid
 shutdown deadlock

The session run loop closes the PTY after copyOut returns, and session.close
(via Manager.Close) closes the same PTY again. creackPTY.Close called cmd.Wait
each time, and a second concurrent Wait on the same process blocks forever, so
daemon shutdown deadlocked whenever a terminal was still attached. fakePTY is
idempotent via sync.Once, so the unit suite never exercised this; a real tmux
attach surfaced it.

Guard close+kill+wait with a sync.Once so Wait runs exactly once. Add a
regression test that double-closes a real PTY under a watchdog.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
---
 backend/internal/terminal/pty_unix.go      | 26 +++++++++++------
 backend/internal/terminal/pty_unix_test.go | 33 ++++++++++++++++++++++
 2 files changed, 51 insertions(+), 8 deletions(-)
 create mode 100644 backend/internal/terminal/pty_unix_test.go

diff --git a/backend/internal/terminal/pty_unix.go b/backend/internal/terminal/pty_unix.go
index 4849215be..e5ca6f34b 100644
--- a/backend/internal/terminal/pty_unix.go
+++ b/backend/internal/terminal/pty_unix.go
@@ -7,6 +7,7 @@ import (
 	"errors"
 	"os"
 	"os/exec"
+	"sync"
 
 	"github.com/creack/pty"
 )
@@ -27,8 +28,10 @@ func defaultSpawn(ctx context.Context, argv []string) (ptyProcess, error) {
 }
 
 type creackPTY struct {
-	f   *os.File
-	cmd *exec.Cmd
+	f         *os.File
+	cmd       *exec.Cmd
+	closeOnce sync.Once
+	closeErr  error
 }
 
 func (p *creackPTY) Read(b []byte) (int, error)  { return p.f.Read(b) }
@@ -42,11 +45,18 @@ func (p *creackPTY) Wait() error { return p.cmd.Wait() }
 
 // Close stops the attach process and releases the PTY. tmux attach exits cleanly
 // when the master closes, but kill the process to be sure it does not linger.
+//
+// It is idempotent: both the session run loop (after copyOut returns) and
+// session.close (via Manager.Close) call Close on the same PTY, and cmd.Wait
+// must run exactly once. A second concurrent Wait on the same process blocks
+// forever, deadlocking daemon shutdown when a terminal is still attached.
 func (p *creackPTY) Close() error {
-	closeErr := p.f.Close()
-	if p.cmd.Process != nil {
-		_ = p.cmd.Process.Kill()
-	}
-	_ = p.cmd.Wait()
-	return closeErr
+	p.closeOnce.Do(func() {
+		p.closeErr = p.f.Close()
+		if p.cmd.Process != nil {
+			_ = p.cmd.Process.Kill()
+		}
+		_ = p.cmd.Wait()
+	})
+	return p.closeErr
 }
diff --git a/backend/internal/terminal/pty_unix_test.go b/backend/internal/terminal/pty_unix_test.go
new file mode 100644
index 000000000..7d8c04ff5
--- /dev/null
+++ b/backend/internal/terminal/pty_unix_test.go
@@ -0,0 +1,33 @@
+//go:build !windows
+
+package terminal
+
+import (
+	"context"
+	"testing"
+	"time"
+)
+
+// TestCreackPTYCloseIsIdempotent guards the shutdown deadlock: the session run
+// loop and session.close both call Close on the same PTY, so cmd.Wait must run
+// exactly once. Without the sync.Once a second Wait blocks forever, so this test
+// would hang (caught by the watchdog) rather than fail.
+func TestCreackPTYCloseIsIdempotent(t *testing.T) {
+	p, err := defaultSpawn(context.Background(), []string{"/bin/sh", "-c", "sleep 30"})
+	if err != nil {
+		t.Fatalf("spawn: %v", err)
+	}
+
+	done := make(chan struct{})
+	go func() {
+		_ = p.Close()
+		_ = p.Close() // second close must not block on a second cmd.Wait
+		close(done)
+	}()
+
+	select {
+	case <-done:
+	case <-time.After(5 * time.Second):
+		t.Fatal("creackPTY.Close did not return: double Close deadlocked on cmd.Wait")
+	}
+}