Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Commit75e7a93

Browse files
fix: stop tearing down non-TTY processes on SSH session end (cherry-pick#18673) (#18677)
Cherry-picked fix: stop tearing down non-TTY processes on SSH sessionend (#18673)(possibly temporary) fix for#18519Matches OpenSSH for non-tty sessions, where we don't actively terminatethe process.Adds explicit tracking to the SSH server for these processes so that ifwe are shutting down we terminate them: this ensures that we can shutdown quickly to allow shutdown scripts to run. It also ensures our testsdon't leak system resources.Co-authored-by: Spike Curtis <spike@coder.com>
1 parent8e8dd58 commit75e7a93

File tree

3 files changed

+54
-18
lines changed

3 files changed

+54
-18
lines changed

‎agent/agentssh/agentssh.go‎

Lines changed: 41 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -124,6 +124,7 @@ type Server struct {
124124
listenersmap[net.Listener]struct{}
125125
connsmap[net.Conn]struct{}
126126
sessionsmap[ssh.Session]struct{}
127+
processesmap[*os.Process]struct{}
127128
closingchanstruct{}
128129
// Wait for goroutines to exit, waited without
129130
// a lock on mu but protected by closing.
@@ -182,6 +183,7 @@ func NewServer(ctx context.Context, logger slog.Logger, prometheusRegistry *prom
182183
fs:fs,
183184
conns:make(map[net.Conn]struct{}),
184185
sessions:make(map[ssh.Session]struct{}),
186+
processes:make(map[*os.Process]struct{}),
185187
logger:logger,
186188

187189
config:config,
@@ -586,7 +588,10 @@ func (s *Server) startNonPTYSession(logger slog.Logger, session ssh.Session, mag
586588
// otherwise context cancellation will not propagate properly
587589
// and SSH server close may be delayed.
588590
cmd.SysProcAttr=cmdSysProcAttr()
589-
cmd.Cancel=cmdCancel(session.Context(),logger,cmd)
591+
592+
// to match OpenSSH, we don't actually tear a non-TTY command down, even if the session ends.
593+
// c.f. https://github.com/coder/coder/issues/18519#issuecomment-3019118271
594+
cmd.Cancel=nil
590595

591596
cmd.Stdout=session
592597
cmd.Stderr=session.Stderr()
@@ -609,6 +614,16 @@ func (s *Server) startNonPTYSession(logger slog.Logger, session ssh.Session, mag
609614
s.metrics.sessionErrors.WithLabelValues(magicTypeLabel,"no","start_command").Add(1)
610615
returnxerrors.Errorf("start: %w",err)
611616
}
617+
618+
// Since we don't cancel the process when the session stops, we still need to tear it down if we are closing. So
619+
// track it here.
620+
if!s.trackProcess(cmd.Process,true) {
621+
// must be closing
622+
err=cmdCancel(logger,cmd.Process)
623+
returnxerrors.Errorf("failed to track process: %w",err)
624+
}
625+
defers.trackProcess(cmd.Process,false)
626+
612627
sigs:=make(chan ssh.Signal,1)
613628
session.Signals(sigs)
614629
deferfunc() {
@@ -1052,6 +1067,27 @@ func (s *Server) trackSession(ss ssh.Session, add bool) (ok bool) {
10521067
returntrue
10531068
}
10541069

1070+
// trackCommand registers the process with the server. If the server is
1071+
// closing, the process is not registered and should be closed.
1072+
//
1073+
//nolint:revive
1074+
func (s*Server)trackProcess(p*os.Process,addbool) (okbool) {
1075+
s.mu.Lock()
1076+
defers.mu.Unlock()
1077+
ifadd {
1078+
ifs.closing!=nil {
1079+
// Server closed.
1080+
returnfalse
1081+
}
1082+
s.wg.Add(1)
1083+
s.processes[p]=struct{}{}
1084+
returntrue
1085+
}
1086+
s.wg.Done()
1087+
delete(s.processes,p)
1088+
returntrue
1089+
}
1090+
10551091
// Close the server and all active connections. Server can be re-used
10561092
// after Close is done.
10571093
func (s*Server)Close()error {
@@ -1091,6 +1127,10 @@ func (s *Server) Close() error {
10911127
_=c.Close()
10921128
}
10931129

1130+
forp:=ranges.processes {
1131+
_=cmdCancel(s.logger,p)
1132+
}
1133+
10941134
s.logger.Debug(ctx,"closing SSH server")
10951135
err:=s.srv.Close()
10961136

‎agent/agentssh/exec_other.go‎

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ package agentssh
44

55
import (
66
"context"
7-
"os/exec"
7+
"os"
88
"syscall"
99

1010
"cdr.dev/slog"
@@ -16,9 +16,7 @@ func cmdSysProcAttr() *syscall.SysProcAttr {
1616
}
1717
}
1818

19-
funccmdCancel(ctx context.Context,logger slog.Logger,cmd*exec.Cmd)func()error {
20-
returnfunc()error {
21-
logger.Debug(ctx,"cmdCancel: sending SIGHUP to process and children",slog.F("pid",cmd.Process.Pid))
22-
returnsyscall.Kill(-cmd.Process.Pid,syscall.SIGHUP)
23-
}
19+
funccmdCancel(logger slog.Logger,p*os.Process)error {
20+
logger.Debug(context.Background(),"cmdCancel: sending SIGHUP to process and children",slog.F("pid",p.Pid))
21+
returnsyscall.Kill(-p.Pid,syscall.SIGHUP)
2422
}

‎agent/agentssh/exec_windows.go‎

Lines changed: 9 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@ package agentssh
22

33
import (
44
"context"
5-
"os/exec"
5+
"os"
66
"syscall"
77

88
"cdr.dev/slog"
@@ -12,14 +12,12 @@ func cmdSysProcAttr() *syscall.SysProcAttr {
1212
return&syscall.SysProcAttr{}
1313
}
1414

15-
funccmdCancel(ctx context.Context,logger slog.Logger,cmd*exec.Cmd)func()error {
16-
returnfunc()error {
17-
logger.Debug(ctx,"cmdCancel: killing process",slog.F("pid",cmd.Process.Pid))
18-
// Windows doesn't support sending signals to process groups, so we
19-
// have to kill the process directly. In the future, we may want to
20-
// implement a more sophisticated solution for process groups on
21-
// Windows, but for now, this is a simple way to ensure that the
22-
// process is terminated when the context is cancelled.
23-
returncmd.Process.Kill()
24-
}
15+
funccmdCancel(logger slog.Logger,p*os.Process)error {
16+
logger.Debug(context.Background(),"cmdCancel: killing process",slog.F("pid",p.Pid))
17+
// Windows doesn't support sending signals to process groups, so we
18+
// have to kill the process directly. In the future, we may want to
19+
// implement a more sophisticated solution for process groups on
20+
// Windows, but for now, this is a simple way to ensure that the
21+
// process is terminated when the context is cancelled.
22+
returnp.Kill()
2523
}

0 commit comments

Comments
 (0)
[8]ページ先頭
©2009-2025 Movatter.jp