From dbdef00e91db0b4396064eea6cafd0dbc49b834b Mon Sep 17 00:00:00 2001 From: user Date: Fri, 4 Sep 2026 05:36:16 +0000 Subject: [PATCH] test: cover user-mode apply atomicity and non-blocking KILL db: TestSetSessionUserModesIsAtomic installs a trigger that rejects the is_oper UPDATE after the is_wallops UPDATE has run, proving the '+w-o' partial-failure the old independent-UPDATE loop exhibited is gone. ircserver: TestDisconnectDoesNotBlockOnUnresponsiveVictim drives Disconnect against a net.Pipe victim that never reads and requires the call to return promptly; verified to fail against the synchronous implementation. Its counterpart asserts the notification is still delivered and the socket still closed. --- internal/db/export_test.go | 16 ++++ internal/db/queries_test.go | 124 +++++++++++++++++++++++++++++ internal/ircserver/conn_test.go | 128 ++++++++++++++++++++++++++++++ internal/ircserver/export_test.go | 18 +++++ 4 files changed, 286 insertions(+) create mode 100644 internal/ircserver/conn_test.go diff --git a/internal/db/export_test.go b/internal/db/export_test.go index 45c0435..fed642d 100644 --- a/internal/db/export_test.go +++ b/internal/db/export_test.go @@ -58,3 +58,19 @@ func (database *Database) Close() error { return nil } + +// ExecForTest runs a raw statement against the test +// database. Tests use it to install SQLite triggers that +// force a specific write to fail, so that the atomicity of +// multi-statement helpers can be exercised. +func (database *Database) ExecForTest( + ctx context.Context, + query string, +) error { + _, err := database.conn.ExecContext(ctx, query) + if err != nil { + return fmt.Errorf("exec for test: %w", err) + } + + return nil +} diff --git a/internal/db/queries_test.go b/internal/db/queries_test.go index 459dc04..4f96f0e 100644 --- a/internal/db/queries_test.go +++ b/internal/db/queries_test.go @@ -1488,3 +1488,127 @@ func TestChannelUserLimit(t *testing.T) { t.Fatalf("expected 0, got %d", limit) } } + +// TestSetSessionUserModesIsAtomic proves that a multi-mode +// change is all-or-nothing. A trigger makes the is_oper +// UPDATE fail after the is_wallops UPDATE has already run, +// which is exactly the "+w-o" partial-failure the previous +// implementation exhibited: it issued the two UPDATEs +// independently, so +w persisted while the caller reported +// total failure. +func TestSetSessionUserModesIsAtomic(t *testing.T) { + t.Parallel() + + database := setupTestDB(t) + ctx := t.Context() + + sessionID, _, _, err := database.CreateSession( + ctx, "alice", "", "", "", + ) + if err != nil { + t.Fatal(err) + } + + err = database.ExecForTest(ctx, + `CREATE TRIGGER reject_oper + BEFORE UPDATE OF is_oper ON sessions + BEGIN SELECT RAISE(ABORT, 'oper write rejected'); + END`, + ) + if err != nil { + t.Fatal(err) + } + + wallops := true + oper := false + + err = database.SetSessionUserModes( + ctx, sessionID, &wallops, &oper, + ) + if err == nil { + t.Fatal("expected the rejected oper write to fail") + } + + gotWallops, err := database.IsSessionWallops( + ctx, sessionID, + ) + if err != nil { + t.Fatal(err) + } + + if gotWallops { + t.Error( + "wallops persisted despite the transaction " + + "failing; the apply stage is not atomic", + ) + } +} + +// TestSetSessionUserModesAppliesBoth is the success-path +// counterpart: when nothing fails, both flags are written, +// and a nil pointer leaves that flag untouched. +func TestSetSessionUserModesAppliesBoth(t *testing.T) { + t.Parallel() + + database := setupTestDB(t) + ctx := t.Context() + + sessionID, _, _, err := database.CreateSession( + ctx, "alice", "", "", "", + ) + if err != nil { + t.Fatal(err) + } + + if err := database.SetSessionOper( + ctx, sessionID, true, + ); err != nil { + t.Fatal(err) + } + + wallops := true + oper := false + + if err := database.SetSessionUserModes( + ctx, sessionID, &wallops, &oper, + ); err != nil { + t.Fatal(err) + } + + gotWallops, err := database.IsSessionWallops( + ctx, sessionID, + ) + if err != nil { + t.Fatal(err) + } + + gotOper, err := database.IsSessionOper(ctx, sessionID) + if err != nil { + t.Fatal(err) + } + + if !gotWallops || gotOper { + t.Errorf( + "want wallops=true oper=false, got %v/%v", + gotWallops, gotOper, + ) + } + + // A nil pointer must leave the stored value alone. + if err := database.SetSessionUserModes( + ctx, sessionID, nil, nil, + ); err != nil { + t.Fatal(err) + } + + gotWallops, err = database.IsSessionWallops( + ctx, sessionID, + ) + if err != nil { + t.Fatal(err) + } + + if !gotWallops { + t.Error("nil pointers must not clear wallops") + } +} diff --git a/internal/ircserver/conn_test.go b/internal/ircserver/conn_test.go new file mode 100644 index 0000000..3347349 --- /dev/null +++ b/internal/ircserver/conn_test.go @@ -0,0 +1,128 @@ +package ircserver_test + +import ( + "bufio" + "log/slog" + "net" + "os" + "strings" + "testing" + "time" + + "sneak.berlin/go/neoirc/internal/config" + "sneak.berlin/go/neoirc/internal/ircserver" +) + +// disconnectBudget is how long Disconnect is allowed to +// take when the victim never reads. It is far below the +// 30s writeTimeout that the old synchronous implementation +// would have burned on each of its two writes. +const disconnectBudget = 2 * time.Second + +// TestDisconnectDoesNotBlockOnUnresponsiveVictim proves +// that an operator KILL cannot be stalled by its target. +// The victim's socket is a net.Pipe, so every write blocks +// until the peer reads and the peer here never does. The +// old implementation performed both notification writes on +// the killer's goroutine, which wedged the operator's own +// serve() loop on the IRC path and the API request on the +// HTTP path for as long as the victim cared to stay silent. +func TestDisconnectDoesNotBlockOnUnresponsiveVictim( + t *testing.T, +) { + t.Parallel() + + serverSide, clientSide := net.Pipe() + + t.Cleanup(func() { + _ = clientSide.Close() + }) + + log := slog.New(slog.NewTextHandler( + os.Stderr, + &slog.HandlerOptions{Level: slog.LevelError}, //nolint:exhaustruct + )) + cfg := &config.Config{ //nolint:exhaustruct + ServerName: "test.irc", + } + + victim := ircserver.NewTestConn( + log, cfg, serverSide, "victim", + ) + + returned := make(chan struct{}) + + go func() { + victim.Disconnect("killed by oper") + close(returned) + }() + + select { + case <-returned: + case <-time.After(disconnectBudget): + t.Fatal( + "Disconnect blocked on the victim's socket; " + + "the killer must not be held hostage", + ) + } +} + +// TestDisconnectNotifiesAndClosesVictim is the other half +// of the contract: moving the notification off the killer's +// goroutine must not lose it. A victim that does read gets +// both the KILL and the ERROR line, and then its socket is +// closed so its read loop unblocks. +func TestDisconnectNotifiesAndClosesVictim(t *testing.T) { + t.Parallel() + + serverSide, clientSide := net.Pipe() + + t.Cleanup(func() { + _ = clientSide.Close() + }) + + log := slog.New(slog.NewTextHandler( + os.Stderr, + &slog.HandlerOptions{Level: slog.LevelError}, //nolint:exhaustruct + )) + cfg := &config.Config{ //nolint:exhaustruct + ServerName: "test.irc", + } + + victim := ircserver.NewTestConn( + log, cfg, serverSide, "victim", + ) + + lines := make(chan []string, 1) + + go func() { + var got []string + + scanner := bufio.NewScanner(clientSide) + for scanner.Scan() { + got = append(got, scanner.Text()) + } + + lines <- got + }() + + victim.Disconnect("killed by oper") + + var got []string + + select { + case got = <-lines: + case <-time.After(5 * time.Second): + t.Fatal("victim socket was never closed") + } + + joined := strings.Join(got, "\n") + + if !strings.Contains(joined, "KILL victim") { + t.Errorf("missing KILL line, got: %q", joined) + } + + if !strings.Contains(joined, "ERROR :Closing Link:") { + t.Errorf("missing ERROR line, got: %q", joined) + } +} diff --git a/internal/ircserver/export_test.go b/internal/ircserver/export_test.go index bc7e6da..5d26699 100644 --- a/internal/ircserver/export_test.go +++ b/internal/ircserver/export_test.go @@ -55,3 +55,21 @@ func (s *Server) Stop() { func (s *Server) Listener() net.Listener { return s.listener } + +// NewTestConn wraps an already-established net.Conn in a +// Conn so tests can drive connection-level behaviour such +// as Disconnect without standing up a whole server. +func NewTestConn( + log *slog.Logger, + cfg *config.Config, + tcpConn net.Conn, + nick string, +) *Conn { + conn := newConn( + context.Background(), tcpConn, log, + nil, nil, cfg, nil, + ) + conn.nick = nick + + return conn +}