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

Commit7747924

Browse files
fix: stop tearing down non-TTY processes on SSH session end (cherry-pick#18673) (#18676)
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 parent4a61bbe commit7747924

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
@@ -125,6 +125,7 @@ type Server struct {
125125
listenersmap[net.Listener]struct{}
126126
connsmap[net.Conn]struct{}
127127
sessionsmap[ssh.Session]struct{}
128+
processesmap[*os.Process]struct{}
128129
closingchanstruct{}
129130
// Wait for goroutines to exit, waited without
130131
// a lock on mu but protected by closing.
@@ -183,6 +184,7 @@ func NewServer(ctx context.Context, logger slog.Logger, prometheusRegistry *prom
183184
fs:fs,
184185
conns:make(map[net.Conn]struct{}),
185186
sessions:make(map[ssh.Session]struct{}),
187+
processes:make(map[*os.Process]struct{}),
186188
logger:logger,
187189

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

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

1088+
// trackCommand registers the process with the server. If the server is
1089+
// closing, the process is not registered and should be closed.
1090+
//
1091+
//nolint:revive
1092+
func (s*Server)trackProcess(p*os.Process,addbool) (okbool) {
1093+
s.mu.Lock()
1094+
defers.mu.Unlock()
1095+
ifadd {
1096+
ifs.closing!=nil {
1097+
// Server closed.
1098+
returnfalse
1099+
}
1100+
s.wg.Add(1)
1101+
s.processes[p]=struct{}{}
1102+
returntrue
1103+
}
1104+
s.wg.Done()
1105+
delete(s.processes,p)
1106+
returntrue
1107+
}
1108+
10731109
// Close the server and all active connections. Server can be re-used
10741110
// after Close is done.
10751111
func (s*Server)Close()error {
@@ -1109,6 +1145,10 @@ func (s *Server) Close() error {
11091145
_=c.Close()
11101146
}
11111147

1148+
forp:=ranges.processes {
1149+
_=cmdCancel(s.logger,p)
1150+
}
1151+
11121152
s.logger.Debug(ctx,"closing SSH server")
11131153
err:=s.srv.Close()
11141154

‎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