PING a quiet IRC listener client before closing it (closes #155)

serve closed a connection after five minutes with nothing read and never
sent a PING, so a quiet registered client lost its session. It now sends
PING :<server> after pingInterval with nothing read, and closes the
connection when nothing is read within pongDeadline after that. Every
line read starts both waits again. readTimeout is removed.

Unregistered connections are sent the PING too, so a PONG before
registration is now ignored instead of answered with ERR_NOTREGISTERED.

Model: opus-5-5

README.md

@@ -2667,6 +2667,9 @@ IRC_LISTEN_ADDR: ""
   An optional `PASS` before registration sets the session password (minimum 8
   characters). Registering creates a session without a hashcash stamp; closing
   the connection ends the session, as `QUIT` does.
+- **PING**: a connection that sends nothing for 90 seconds is sent a `PING`, and
+  is closed if it sends nothing in the 30 seconds after that; any line, such as
+  the client's `PONG`, starts both waits again.
 - **CAP negotiation**: `CAP LS` gets an empty capability list and `CAP END` is
   ignored, for compatibility with modern clients.
 - **Channel prefixes**: Channels must start with `#`. `JOIN` prepends it if

internal/ircserver/conn.go

@@ -20,7 +20,6 @@ import (
 
 const (
 	maxLineLen      = 512
-	readTimeout     = 5 * time.Minute
 	writeTimeout    = 30 * time.Second
 	killWriteWindow = 2 * time.Second
 	dnsTimeout      = 3 * time.Second
@@ -46,6 +45,11 @@ type Conn struct {
 	serverSfx string
 	commands  map[string]cmdHandler
 
+	// pingInterval and pongDeadline are the constants of the
+	// same names, except in tests, which shorten them.
+	pingInterval time.Duration
+	pongDeadline time.Duration
+
 	// writeMu serializes writes to conn, which come from
 	// serve(), from the relay goroutine and, for KILL, from
 	// Disconnect.
@@ -96,6 +100,9 @@ func newConn(
 		serverSfx: srvName,
 		remoteIP:  host,
 		hostname:  resolveHost(ctx, host),
+
+		pingInterval: pingInterval,
+		pongDeadline: pongDeadline,
 	}
 
 	conn.commands = conn.buildCommandMap()
@@ -214,17 +221,28 @@ func resolveHost(ctx context.Context, addr string) string {
 
 // serve is the main loop for a single IRC client connection.
 // Cancelling ctx when it returns stops the relay goroutine.
+// When nothing has been read for pingInterval the client is
+// sent a PING, and when nothing is read within pongDeadline
+// after that the connection is closed.
 func (c *Conn) serve(ctx context.Context) {
 	ctx, cancel := context.WithCancel(ctx)
 	defer cancel()
 	defer c.cleanup(ctx)
 
+	ping := time.AfterFunc(c.pingInterval, func() {
+		c.send("PING :" + c.serverSfx)
+	})
+	defer ping.Stop()
+
 	scanner := bufio.NewScanner(c.conn)
 	scanner.Buffer(make([]byte, maxLineLen), maxLineLen)
 
 	for {
+		// Every line read starts both waits again.
+		ping.Reset(c.pingInterval)
+
 		_ = c.conn.SetReadDeadline(
-			time.Now().Add(readTimeout),
+			time.Now().Add(c.pingInterval + c.pongDeadline),
 		)
 
 		if !scanner.Scan() {
@@ -366,7 +384,7 @@ func (c *Conn) handleMessage(
 	msg *Message,
 ) {
 	// Before registration, only NICK, USER, PASS, PING,
-	// QUIT, and CAP are accepted.
+	// PONG, QUIT, and CAP are accepted.
 	if !c.registered {
 		c.handlePreRegistration(ctx, msg)
 
@@ -452,6 +470,10 @@ func (c *Conn) handlePreRegistration(
 	case irc.CmdPing:
 		c.handlePing(msg)
 
+		return
+	case irc.CmdPong:
+		// The answer to serve's PING, which unregistered
+		// clients are sent too.
 		return
 	case irc.CmdQuit:
 		c.handleQuit(msg)

internal/ircserver/export_test.go

@@ -64,6 +64,23 @@ const KillWriteWindow = killWriteWindow
 // output queue when no broker notification arrives.
 const PollInterval = pollInterval
 
+// ServeConn serves tcpConn as s serves each connection it
+// accepts, except that interval and deadline replace
+// pingInterval and pongDeadline.
+func (s *Server) ServeConn(
+	tcpConn net.Conn,
+	interval, deadline time.Duration,
+) {
+	client := newConn(
+		context.Background(), tcpConn, s.log,
+		s.database, s.brk, s.cfg, s.svc,
+	)
+	client.pingInterval = interval
+	client.pongDeadline = deadline
+
+	go client.serve(context.Background())
+}
+
 // NewTestConn returns a Conn for nick on tcpConn, without a
 // server, database or service behind it.
 func NewTestConn(

internal/ircserver/server_test.go

@@ -9,6 +9,7 @@ import (
 	"net"
 	"os"
 	"runtime"
+	"slices"
 	"strings"
 	"testing"
 	"time"
@@ -174,6 +175,42 @@ func (env *testEnv) dial(t *testing.T) *testClient {
 	}
 }
 
+// The ping interval and PONG deadline for dialWithPingTimes.
+// They are equal, so that three answered PINGs take longer
+// than a client that answers none stays connected.
+const (
+	testPingInterval = 300 * time.Millisecond
+	testPongDeadline = 300 * time.Millisecond
+)
+
+// dialWithPingTimes connects a client to the test server
+// whose connection is sent a PING after testPingInterval with
+// nothing read, and closed when nothing is read within
+// testPongDeadline after that.
+func (env *testEnv) dialWithPingTimes(t *testing.T) *testClient {
+	t.Helper()
+
+	serverSide, clientSide := net.Pipe()
+
+	t.Cleanup(func() { _ = clientSide.Close() })
+
+	env.srv.ServeConn(
+		serverSide, testPingInterval, testPongDeadline,
+	)
+
+	return &testClient{
+		t:       t,
+		conn:    clientSide,
+		scanner: bufio.NewScanner(clientSide),
+	}
+}
+
+// isServerPing reports whether line is the PING the test
+// server sends a quiet client.
+func isServerPing(line string) bool {
+	return line == "PING :"+testServerName
+}
+
 // testClient wraps a raw TCP connection with helpers.
 type testClient struct {
 	t       *testing.T
@@ -388,6 +425,57 @@ func TestPingPong(t *testing.T) {
 	assertContains(t, lines, "PONG", "PONG response")
 }
 
+// TestQuietClientAnsweringPingStaysConnected checks that a
+// registered client that sends nothing is sent a PING each
+// time the ping interval passes, and stays connected as long
+// as it answers each one.
+func TestQuietClientAnsweringPingStaysConnected(t *testing.T) {
+	t.Parallel()
+
+	env := newTestEnv(t)
+	client := env.dialWithPingTimes(t)
+	client.register("answers")
+
+	for range 3 {
+		client.readUntil(isServerPing)
+		client.send("PONG :" + testServerName)
+	}
+
+	client.sendAndExpect("PING :still here", "PONG")
+}
+
+// TestQuietClientNotAnsweringPingIsClosed checks that a
+// registered client that does not answer the PING is
+// disconnected.
+func TestQuietClientNotAnsweringPingIsClosed(t *testing.T) {
+	t.Parallel()
+
+	env := newTestEnv(t)
+	client := env.dialWithPingTimes(t)
+	client.register("silent")
+
+	lines := client.readUntilClosed()
+	if !slices.ContainsFunc(lines, isServerPing) {
+		t.Errorf("closed without a PING, got lines %q", lines)
+	}
+}
+
+// TestUnregisteredClientAnswersPing checks that a PONG before
+// registration, the answer to the PING unregistered clients
+// are sent too, gets no ERR_NOTREGISTERED.
+func TestUnregisteredClientAnswersPing(t *testing.T) {
+	t.Parallel()
+
+	env := newTestEnv(t)
+	client := env.dialWithPingTimes(t)
+
+	client.readUntil(isServerPing)
+	client.send("PONG :" + testServerName)
+
+	lines := client.sendAndExpect("PING :after", "PONG")
+	assertNotContains(t, lines, " 451 ", "ERR_NOTREGISTERED")
+}
+
 // TestIdleCleanupKeepsActiveUser checks that a user who
 // sends commands is kept by the idle cleanup, and a user who
 // sends nothing is not.