From babb3f385da4b2b0fc532830fc6ef14152d4bf10 Mon Sep 17 00:00:00 2001 From: clawbot Date: Tue, 20 Oct 2026 16:12:36 +0000 Subject: [PATCH] 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 : 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 | 3 + internal/ircserver/conn.go | 25 +++++-- internal/ircserver/export_test.go | 16 ++++- internal/ircserver/server.go | 11 ++- internal/ircserver/server_test.go | 115 +++++++++++++++++++++++++++++- 5 files changed, 163 insertions(+), 7 deletions(-) diff --git a/README.md b/README.md index cfb9eb1..eda7e7d 100644 --- a/README.md +++ b/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 diff --git a/internal/ircserver/conn.go b/internal/ircserver/conn.go index 8fc9224..bba99ca 100644 --- a/internal/ircserver/conn.go +++ b/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 @@ -214,17 +213,31 @@ 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. -func (c *Conn) serve(ctx context.Context) { +// When nothing has been read for interval the client is sent +// a PING, and when nothing is read within deadline after that +// the connection is closed. +func (c *Conn) serve( + ctx context.Context, + interval, deadline time.Duration, +) { ctx, cancel := context.WithCancel(ctx) defer cancel() defer c.cleanup(ctx) + ping := time.AfterFunc(interval, 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(interval) + _ = c.conn.SetReadDeadline( - time.Now().Add(readTimeout), + time.Now().Add(interval + deadline), ) if !scanner.Scan() { @@ -366,7 +379,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 +465,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) diff --git a/internal/ircserver/export_test.go b/internal/ircserver/export_test.go index 60d039b..24d3007 100644 --- a/internal/ircserver/export_test.go +++ b/internal/ircserver/export_test.go @@ -13,13 +13,16 @@ import ( "sneak.berlin/go/neoirc/internal/service" ) -// NewTestServer creates a Server suitable for testing. +// NewTestServer creates a Server suitable for testing, whose +// connections use interval and deadline in place of +// pingInterval and pongDeadline. // The caller must call Stop() when finished. func NewTestServer( log *slog.Logger, cfg *config.Config, database *db.Database, brk *broker.Broker, + interval, deadline time.Duration, ) *Server { globs := &globals.Globals{ Appname: "neoirc", @@ -38,6 +41,9 @@ func NewTestServer( brk: brk, svc: svc, conns: make(map[*Conn]struct{}), + + pingInterval: interval, + pongDeadline: deadline, } } @@ -64,6 +70,14 @@ const KillWriteWindow = killWriteWindow // output queue when no broker notification arrives. const PollInterval = pollInterval +// PingInterval is how long the server waits with nothing read +// from a connection before it sends a PING. +const PingInterval = pingInterval + +// PongDeadline is how long the server waits after the PING +// for a line before it closes the connection. +const PongDeadline = pongDeadline + // NewTestConn returns a Conn for nick on tcpConn, without a // server, database or service behind it. func NewTestConn( diff --git a/internal/ircserver/server.go b/internal/ircserver/server.go index e3b6d2e..8d5ce86 100644 --- a/internal/ircserver/server.go +++ b/internal/ircserver/server.go @@ -6,6 +6,7 @@ import ( "log/slog" "net" "sync" + "time" "go.uber.org/fx" "sneak.berlin/go/neoirc/internal/broker" @@ -38,6 +39,11 @@ type Server struct { mu sync.Mutex conns map[*Conn]struct{} cancel context.CancelFunc + + // pingInterval and pongDeadline are the constants of the + // same names, except in tests, which shorten them. + pingInterval time.Duration + pongDeadline time.Duration } // New creates a new IRC Server and registers its lifecycle @@ -57,6 +63,9 @@ func New( listener: nil, cancel: nil, mu: sync.Mutex{}, + + pingInterval: pingInterval, + pongDeadline: pongDeadline, } listenAddr := params.Config.IRCListenAddr @@ -153,7 +162,7 @@ func (s *Server) acceptLoop(ctx context.Context) { s.mu.Unlock() }() - client.serve(ctx) + client.serve(ctx, s.pingInterval, s.pongDeadline) }() } } diff --git a/internal/ircserver/server_test.go b/internal/ircserver/server_test.go index 6aba3a8..6f23c9c 100644 --- a/internal/ircserver/server_test.go +++ b/internal/ircserver/server_test.go @@ -9,6 +9,7 @@ import ( "net" "os" "runtime" + "slices" "strings" "testing" "time" @@ -70,6 +71,22 @@ func newTestEnvWithConfig( ) *testEnv { t.Helper() + return newTestEnvWithPingTimes( + t, cfg, ircserver.PingInterval, ircserver.PongDeadline, + ) +} + +// newTestEnvWithPingTimes is newTestEnvWithConfig, except +// that the server sends a connection a PING after interval +// with nothing read, and closes it when nothing is read +// within deadline after that. +func newTestEnvWithPingTimes( + t *testing.T, + cfg *config.Config, + interval, deadline time.Duration, +) *testEnv { + t.Helper() + // A random name, so that no other test environment, not // even an earlier run of the same test, can share it. dsn := fmt.Sprintf( @@ -126,7 +143,9 @@ func newTestEnvWithConfig( &slog.HandlerOptions{Level: slog.LevelError}, //nolint:exhaustruct )) - srv := ircserver.NewTestServer(log, cfg, database, brk) + srv := ircserver.NewTestServer( + log, cfg, database, brk, interval, deadline, + ) err = srv.Start(addr) if err != nil { @@ -174,6 +193,34 @@ func (env *testEnv) dial(t *testing.T) *testClient { } } +// The ping interval and PONG deadline for +// newTestEnvWithShortPings. 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 +) + +// newTestEnvWithShortPings starts an IRC server that uses +// testPingInterval and testPongDeadline in place of +// pingInterval and pongDeadline. +func newTestEnvWithShortPings(t *testing.T) *testEnv { + t.Helper() + + return newTestEnvWithPingTimes( + t, + &config.Config{ServerName: testServerName}, + testPingInterval, testPongDeadline, + ) +} + +// 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 +435,72 @@ 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, whether with a PONG or with another +// line. +func TestQuietClientAnsweringPingStaysConnected(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + answer string + }{ + {name: "PONG", answer: "PONG :" + testServerName}, + {name: "PING of its own", answer: "PING :answer"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + env := newTestEnvWithShortPings(t) + client := env.dial(t) + client.register("answers") + + for range 3 { + client.readUntil(isServerPing) + client.send(tt.answer) + } + + client.sendAndExpect("PING :still here", "still here") + }) + } +} + +// TestQuietClientNotAnsweringPingIsClosed checks that a +// registered client that does not answer the PING is +// disconnected. +func TestQuietClientNotAnsweringPingIsClosed(t *testing.T) { + t.Parallel() + + env := newTestEnvWithShortPings(t) + client := env.dial(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 := newTestEnvWithShortPings(t) + client := env.dial(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.