From dbaeb0be2c19e87f6d5e954dcf8d738fad9a36f4 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Mon, 7 Apr 2025 07:17:50 +0000 Subject: [PATCH 1/6] test(agent/agentssh): fix early close of server in test Fixes coder/internal#558 --- agent/agentssh/agentssh_test.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/agent/agentssh/agentssh_test.go b/agent/agentssh/agentssh_test.go index 9a427fdd7d91e..eb28325766721 100644 --- a/agent/agentssh/agentssh_test.go +++ b/agent/agentssh/agentssh_test.go @@ -153,7 +153,9 @@ func TestNewServer_CloseActiveConnections(t *testing.T) { logger := slogtest.Make(t, &slogtest.Options{IgnoreErrors: true}).Leveled(slog.LevelDebug) s, err := agentssh.NewServer(ctx, logger, prometheus.NewRegistry(), afero.NewMemMapFs(), agentexec.DefaultExecer, nil) require.NoError(t, err) - defer s.Close() + t.Cleanup(func() { + _ = s.Close() + }) err = s.UpdateHostSigner(42) assert.NoError(t, err) From fcb04be844057357c8a0e86fd0b5a06faaf0a9c2 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Mon, 7 Apr 2025 07:22:00 +0000 Subject: [PATCH 2/6] add a sleep to avoid testing early --- agent/agentssh/agentssh_test.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/agent/agentssh/agentssh_test.go b/agent/agentssh/agentssh_test.go index eb28325766721..447f4efa71098 100644 --- a/agent/agentssh/agentssh_test.go +++ b/agent/agentssh/agentssh_test.go @@ -13,6 +13,7 @@ import ( "strings" "sync" "testing" + "time" "github.com/prometheus/client_golang/prometheus" "github.com/spf13/afero" @@ -195,7 +196,9 @@ func TestNewServer_CloseActiveConnections(t *testing.T) { err = sess.Start("/bin/bash -c 'trap \"sleep 60\" SIGTERM; sleep 60'") assert.NoError(t, err) + time.Sleep(100 * time.Millisecond) // Give the session time to start. close(ch) + err = sess.Wait() assert.Error(t, err) }(waitConns[i]) From 4a0b61e130cacc2f6f7b267c0ae3ccc39e1ffa47 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Mon, 7 Apr 2025 07:25:06 +0000 Subject: [PATCH 3/6] make it more robust --- agent/agentssh/agentssh_test.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/agent/agentssh/agentssh_test.go b/agent/agentssh/agentssh_test.go index 447f4efa71098..ba0d7594ffdf8 100644 --- a/agent/agentssh/agentssh_test.go +++ b/agent/agentssh/agentssh_test.go @@ -13,7 +13,6 @@ import ( "strings" "sync" "testing" - "time" "github.com/prometheus/client_golang/prometheus" "github.com/spf13/afero" @@ -193,10 +192,10 @@ func TestNewServer_CloseActiveConnections(t *testing.T) { } // The 60 seconds here is intended to be longer than the // test. The shutdown should propagate. - err = sess.Start("/bin/bash -c 'trap \"sleep 60\" SIGTERM; sleep 60'") + err = sess.Start("/bin/bash -c 'trap \"sleep 60\" SIGTERM; echo start\"ed\"; sleep 60'") assert.NoError(t, err) - time.Sleep(100 * time.Millisecond) // Give the session time to start. + pty.ExpectMatchContext(ctx, "started") close(ch) err = sess.Wait() From ea5ab423aece3cfef9adb1b10f08e5738e8c4cec Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Mon, 7 Apr 2025 07:48:55 +0000 Subject: [PATCH 4/6] e tu window? --- agent/agentssh/agentssh_test.go | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/agent/agentssh/agentssh_test.go b/agent/agentssh/agentssh_test.go index ba0d7594ffdf8..69f92e0fd31a0 100644 --- a/agent/agentssh/agentssh_test.go +++ b/agent/agentssh/agentssh_test.go @@ -192,7 +192,12 @@ func TestNewServer_CloseActiveConnections(t *testing.T) { } // The 60 seconds here is intended to be longer than the // test. The shutdown should propagate. - err = sess.Start("/bin/bash -c 'trap \"sleep 60\" SIGTERM; echo start\"ed\"; sleep 60'") + if runtime.GOOS == "windows" { + // Best effort to at least partially test this in Windows. + err = sess.Start("echo start\"ed\" && sleep 60") + } else { + err = sess.Start("/bin/bash -c 'trap \"sleep 60\" SIGTERM; echo start\"ed\"; sleep 60'") + } assert.NoError(t, err) pty.ExpectMatchContext(ctx, "started") From 80bd12cf85f936bbbba0e1642e98f7fd358e7986 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Mon, 7 Apr 2025 08:10:02 +0000 Subject: [PATCH 5/6] e tu window?^2 --- agent/agentssh/exec_windows.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/agent/agentssh/exec_windows.go b/agent/agentssh/exec_windows.go index 0345ddd85e52e..5bb2dd9713e66 100644 --- a/agent/agentssh/exec_windows.go +++ b/agent/agentssh/exec_windows.go @@ -2,7 +2,6 @@ package agentssh import ( "context" - "os" "os/exec" "syscall" @@ -15,7 +14,8 @@ func cmdSysProcAttr() *syscall.SysProcAttr { func cmdCancel(ctx context.Context, logger slog.Logger, cmd *exec.Cmd) func() error { return func() error { - logger.Debug(ctx, "cmdCancel: sending interrupt to process", slog.F("pid", cmd.Process.Pid)) - return cmd.Process.Signal(os.Interrupt) + logger.Debug(ctx, "cmdCancel: killing process", slog.F("pid", cmd.Process.Pid)) + return cmd.Process.Kill() + // return cmd.Process.Signal(os.Interrupt) } } From cbd62e0b14863925720f483fa0bd6fe057feedc9 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Mon, 7 Apr 2025 08:21:29 +0000 Subject: [PATCH 6/6] prevent goleak, docs --- agent/agentssh/agentssh.go | 4 +++- agent/agentssh/exec_windows.go | 6 +++++- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/agent/agentssh/agentssh.go b/agent/agentssh/agentssh.go index f56497d149499..293dd4db169ac 100644 --- a/agent/agentssh/agentssh.go +++ b/agent/agentssh/agentssh.go @@ -1060,8 +1060,10 @@ func (s *Server) Close() error { // Guard against multiple calls to Close and // accepting new connections during close. if s.closing != nil { + closing := s.closing s.mu.Unlock() - return xerrors.New("server is closing") + <-closing + return xerrors.New("server is closed") } s.closing = make(chan struct{}) diff --git a/agent/agentssh/exec_windows.go b/agent/agentssh/exec_windows.go index 5bb2dd9713e66..39f0f97198479 100644 --- a/agent/agentssh/exec_windows.go +++ b/agent/agentssh/exec_windows.go @@ -15,7 +15,11 @@ func cmdSysProcAttr() *syscall.SysProcAttr { func cmdCancel(ctx context.Context, logger slog.Logger, cmd *exec.Cmd) func() error { return func() error { logger.Debug(ctx, "cmdCancel: killing process", slog.F("pid", cmd.Process.Pid)) + // Windows doesn't support sending signals to process groups, so we + // have to kill the process directly. In the future, we may want to + // implement a more sophisticated solution for process groups on + // Windows, but for now, this is a simple way to ensure that the + // process is terminated when the context is cancelled. return cmd.Process.Kill() - // return cmd.Process.Signal(os.Interrupt) } }