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>
This commit is contained in:
parent
cd84d94ee4
commit
67f42150b4
|
|
@ -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
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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")
|
||||
}
|
||||
}
|
||||
Loading…
Reference in New Issue