test/refactor: add stalled-with-session test, unexport notePeerActivity

- New TestConnectionKeepAliveClosesStalledConnWithOpenSession exercises
  the channel-request path of dead-peer detection. Opens a session and
  has the client drain all request streams without replying; the server
  should still detect the stall via TimeIsUp and tear the transport
  down. Complements the existing no-session variant which exercises the
  global-request path.

- Unexported NotePeerActivity -> notePeerActivity. It is an internal
  hook driven by this package's request loops; external callers have no
  reason to invoke it. Doing this before v1.2.0 is cut avoids a v2
  break later.

Author: skevetter

keepalive.go

@@ -71,13 +71,13 @@ func (ska *SessionKeepAlive) Reset() {
 	}
 }
 
-// NotePeerActivity records inbound traffic from the peer. It bumps
+// notePeerActivity records inbound traffic from the peer. It bumps
 // lastReceived (clearing the dead-peer deadline used by TimeIsUp) and
 // resets the ticker so the next probe fires `interval` after the most
 // recent activity. Matches OpenSSH sshd, which clears keep_alive_timeouts
 // on every successfully-received packet and defers the next probe on
-// inbound traffic.
-func (ska *SessionKeepAlive) NotePeerActivity() {
+// inbound traffic. Internal — driven by the package's request loops.
+func (ska *SessionKeepAlive) notePeerActivity() {
 	ska.m.Lock()
 	defer ska.m.Unlock()
 	if ska.ticker != nil && !ska.closed {

server.go

@@ -485,7 +485,7 @@ func (srv *Server) HandleConn(newConn net.Conn) {
 			onClose:    openChans.remove,
 			notePeerActivity: func() {
 				if ka := ctx.KeepAlive(); ka != nil {
-					ka.NotePeerActivity()
+					ka.notePeerActivity()
 				}
 			},
 			ctx: ctx,
@@ -526,7 +526,7 @@ func (srv *Server) connectionKeepAlive(
 	// Do NOT Close() the SessionKeepAlive when this function returns: other
 	// goroutines (Server.handleRequests, trackingNewChannel.Accept's
 	// forwarder, and any in-flight probe goroutine spawned below) may still
-	// be calling NotePeerActivity / Reset after we return. The ticker is
+	// be calling notePeerActivity / Reset after we return. The ticker is
 	// no longer referenced once HandleConn returns and is GC'd.
 	keepAlive := ctx.KeepAlive()
 
@@ -591,7 +591,7 @@ func (srv *Server) connectionKeepAlive(
 func (srv *Server) handleRequests(ctx Context, in <-chan *gossh.Request) {
 	for req := range in {
 		if ka := ctx.KeepAlive(); ka != nil {
-			ka.NotePeerActivity()
+			ka.notePeerActivity()
 		}
 		handler := srv.RequestHandlers[req.Type]
 		if handler == nil {

server_test.go

@@ -213,6 +213,81 @@ func TestConnectionKeepAliveClosesStalledConn(t *testing.T) {
 	}
 }
 
+// TestConnectionKeepAliveClosesStalledConnWithOpenSession is the counterpart
+// to TestConnectionKeepAliveClosesStalledConn: when a session channel is open
+// (so probes go as channel requests per client_alive_check), and the peer
+// stops replying to those probes, the connection-level keep-alive must still
+// detect the stall and tear the transport down. This exercises the
+// channel-request path of the deadline check; the no-session variant exercises
+// the global-request path.
+func TestConnectionKeepAliveClosesStalledConnWithOpenSession(t *testing.T) {
+	t.Parallel()
+
+	closingFired := make(chan struct{})
+
+	srv := &Server{
+		Handler:             func(s Session) { <-s.Context().Done() },
+		ClientAliveInterval: 100 * time.Millisecond,
+		ClientAliveCountMax: 3,
+		ConnectionClosingCallback: func(_ Context, _ *gossh.ServerConn) {
+			close(closingFired)
+		},
+	}
+
+	l := newLocalTCPListener()
+	defer func() { _ = l.Close() }()
+	go func() { _ = srv.serveOnce(l) }()
+
+	cfg := &gossh.ClientConfig{
+		User:            "testuser",
+		Auth:            []gossh.AuthMethod{gossh.Password("testpass")},
+		HostKeyCallback: gossh.InsecureIgnoreHostKey(), //nolint:gosec // test code
+	}
+	netConn, err := net.Dial("tcp", l.Addr().String())
+	if err != nil {
+		t.Fatalf("dial: %v", err)
+	}
+	sshConn, chans, reqs, err := gossh.NewClientConn(netConn, l.Addr().String(), cfg)
+	if err != nil {
+		t.Fatalf("NewClientConn: %v", err)
+	}
+	defer func() { _ = sshConn.Close() }()
+
+	ch, chReqs, err := sshConn.OpenChannel("session", nil)
+	if err != nil {
+		t.Fatalf("OpenChannel: %v", err)
+	}
+	defer func() { _ = ch.Close() }()
+
+	// Drain all three request streams (global, per-channel, and the
+	// NewChannel stream) WITHOUT replying. The server's channel-typed
+	// keepalive probes go on chReqs and will never get a reply, so
+	// notePeerActivity is never called on the session-request side and
+	// TimeIsUp eventually fires.
+	go func() {
+		for range chans { //nolint:revive // intentional drain
+		}
+	}()
+	go func() {
+		for req := range reqs {
+			_ = req
+		}
+	}()
+	go func() {
+		for req := range chReqs {
+			_ = req
+		}
+	}()
+
+	// 100ms * 3 = 300ms expected; allow generous slack for CI.
+	select {
+	case <-closingFired:
+	case <-time.After(5 * time.Second):
+		t.Fatal("ConnectionClosingCallback did not fire; " +
+			"stalled-with-session client was not torn down by connection-level keep-alive")
+	}
+}
+
 // TestConnectionKeepAliveUsesChannelRequestWhenSessionOpen verifies that the
 // server's connection-level keepalive mirrors OpenSSH's client_alive_check():
 // while at least one channel is open it sends keepalive@openssh.com as a

session.go

@@ -294,7 +294,7 @@ func (sess *session) handleRequests(_ Context, reqs <-chan *gossh.Request) {
 	}()
 	for req := range reqs {
 		if ka := sess.ctx.KeepAlive(); ka != nil {
-			ka.NotePeerActivity()
+			ka.notePeerActivity()
 		}
 		switch req.Type {
 		case "shell", "exec":