diff options
Diffstat (limited to 'internal')
| -rw-r--r-- | internal/admin/api_entity_path_test.go | 33 | ||||
| -rw-r--r-- | internal/admin/api_rooms.go | 47 | ||||
| -rw-r--r-- | internal/admin/api_rooms_create_test.go | 78 | ||||
| -rw-r--r-- | internal/admin/yaml_util.go | 22 | ||||
| -rw-r--r-- | internal/game/casino.go | 37 | ||||
| -rw-r--r-- | internal/game/core_login_account.go | 51 | ||||
| -rw-r--r-- | internal/game/login_race_regression_test.go | 77 | ||||
| -rw-r--r-- | internal/game/payout_evidence_regression_test.go | 76 | ||||
| -rw-r--r-- | internal/net/server.go | 68 | ||||
| -rw-r--r-- | internal/net/transport_failure_test.go | 44 |
10 files changed, 491 insertions, 42 deletions
diff --git a/internal/admin/api_entity_path_test.go b/internal/admin/api_entity_path_test.go index 7abf65f..27ecbe8 100644 --- a/internal/admin/api_entity_path_test.go +++ b/internal/admin/api_entity_path_test.go @@ -8,6 +8,7 @@ import ( "strings" "testing" + "thehouseoficarus/internal/behavior" "thehouseoficarus/internal/config" "thehouseoficarus/internal/player" ) @@ -60,3 +61,35 @@ func TestEntityAPICannotEscapeData(t *testing.T) { t.Fatal("entity API escaped the data directory") } } + +// TestEntityRenameRejectsDuplicateIDInOtherDirectory: entity IDs are global +// across the type's subdirectory tree (the runtime path indexes resolve by +// filename anywhere under data/<subdir>), so renaming into an ID that +// exists in a DIFFERENT directory must be rejected instead of creating a +// duplicate ID shadowed by whichever file the index finds first. +func TestEntityRenameRejectsDuplicateIDInOtherDirectory(t *testing.T) { + dir := t.TempDir() + for _, sub := range []string{"items/a", "items/b"} { + if err := os.MkdirAll(filepath.Join(dir, sub), 0o755); err != nil { + t.Fatal(err) + } + } + if err := os.WriteFile(filepath.Join(dir, "items/a/old.yaml"), []byte("name: Original\n"), 0o644); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(dir, "items/b/taken.yaml"), []byte("name: Existing\n"), 0o644); err != nil { + t.Fatal(err) + } + + if _, _, _, err := renameEntityFile(dir, "items", "old", "taken"); err == nil { + t.Fatal("rename accepted an ID that already exists in another directory") + } + if dups := behavior.CheckDuplicateIDs(filepath.Join(dir, "items")); len(dups) != 0 { + t.Fatalf("rename produced duplicate IDs: %+v", dups) + } + + // Renaming within the same directory to a free name still works. + if _, _, _, err := renameEntityFile(dir, "items", "old", "moved"); err != nil { + t.Fatalf("ordinary rename rejected: %v", err) + } +} diff --git a/internal/admin/api_rooms.go b/internal/admin/api_rooms.go index 16fc7a0..f9508f6 100644 --- a/internal/admin/api_rooms.go +++ b/internal/admin/api_rooms.go @@ -144,11 +144,38 @@ func (s *AdminServer) handleRooms(w http.ResponseWriter, r *http.Request) { var extraFiles []ExtraFile + // Prepare the reciprocal exit in the destination map BEFORE the + // destination file is committed, so the written file is complete. + if linkFrom != nil && linkDir != nil && *linkDir != "" { + if dir := world.ExitDir(strings.ToLower(*linkDir)); dir != "" { + if opp, ok := world.OppositeExit[dir]; ok && !linkOneWay { + exits[string(opp)] = map[string]any{"room": *linkFrom} + } + } + } + + // The destination room is committed FIRST: if it cannot be written, + // nothing else has been mutated yet. The old order wrote the + // source room's new exit first, so a failed destination write left + // a dangling exit pointing at a room that never existed, with no + // undo entry to repair it. + path := filepath.Join(subdir, strconv.Itoa(id)+".yaml") + newContent, writeErr := writeMapAsYAML(path, m) + if writeErr != nil { + failJSON(w, fmt.Sprintf("write room: %v", writeErr)) + return + } + if linkFrom != nil && linkDir != nil && *linkDir != "" { dir := world.ExitDir(strings.ToLower(*linkDir)) - if opp, ok := world.OppositeExit[dir]; ok { + if _, ok := world.OppositeExit[dir]; ok { srcRoom, loadErr := s.world.LoadRoom(*linkFrom) if loadErr != nil { + // Roll back the destination so a failed create leaves + // no half-created room behind. + if rmErr := os.Remove(path); rmErr != nil { + log.Printf("room create rollback: remove %s: %v", path, rmErr) + } failJSON(w, fmt.Sprintf("load link_from room %d: %v", *linkFrom, loadErr)) return } @@ -160,31 +187,23 @@ func (s *AdminServer) handleRooms(w http.ResponseWriter, r *http.Request) { srcPath, srcOk := s.world.GetRoomPath(*linkFrom) if srcOk { oldContent, _ := snapshotFile(srcPath) - newContent, writeErr := writeYAMLFile(srcPath, srcRoom) + srcContent, writeErr := writeYAMLFile(srcPath, srcRoom) if writeErr != nil { + if rmErr := os.Remove(path); rmErr != nil { + log.Printf("room create rollback: remove %s: %v", path, rmErr) + } failJSON(w, fmt.Sprintf("write source room: %v", writeErr)) return } extraFiles = append(extraFiles, ExtraFile{ FilePath: srcPath, OldContent: oldContent, - NewContent: newContent, + NewContent: srcContent, }) } - - if !linkOneWay { - exits[string(opp)] = map[string]any{"room": *linkFrom} - } } } - path := filepath.Join(subdir, strconv.Itoa(id)+".yaml") - newContent, writeErr := writeMapAsYAML(path, m) - if writeErr != nil { - failJSON(w, fmt.Sprintf("write room: %v", writeErr)) - return - } - s.world.AddRoomPath(id, path) s.undoStack.Push(ChangeDesc{ Description: fmt.Sprintf("create room %d", id), diff --git a/internal/admin/api_rooms_create_test.go b/internal/admin/api_rooms_create_test.go new file mode 100644 index 0000000..38d6fae --- /dev/null +++ b/internal/admin/api_rooms_create_test.go @@ -0,0 +1,78 @@ +package admin + +// Regression: a failed room create (POST /api/rooms with link_from) must +// not leave the source room with a dangling exit pointing at a room that +// was never written, and must not push an undo entry for work that never +// committed. The destination is committed before the source exit so the +// failure happens before any source-side mutation. + +import ( + "net/http" + "net/http/httptest" + "os" + "path/filepath" + "strings" + "testing" + + "thehouseoficarus/internal/config" + "thehouseoficarus/internal/world" +) + +func TestFailedRoomCreateRollsBackSourceExit(t *testing.T) { + s := newTestAdminServer(t, map[int]string{1: "id: 1\nname: Origin\n"}) + s.cfg = config.Default() + s.cfg.StartingRoom = 1 + // A directory at the destination path makes the destination file write + // fail deterministically while the source room is still readable. + if err := os.Mkdir(filepath.Join(s.dataDir, "rooms", "2.yaml"), 0o755); err != nil { + t.Fatal(err) + } + + rec := httptest.NewRecorder() + s.handleRooms(rec, httptest.NewRequest(http.MethodPost, "/api/rooms", + strings.NewReader(`{"name":"Next","link_from":1,"link_dir":"north"}`))) + if !strings.Contains(rec.Body.String(), "error") { + t.Fatalf("expected the create to fail; got %s", rec.Body.String()) + } + + src, err := s.world.LoadRoom(1) + if err != nil { + t.Fatal(err) + } + if exit, ok := src.Exits[world.North]; ok { + t.Fatalf("failed create left a dangling exit to nonexistent room %d", exit.Room) + } + if info := s.undoStack.Info(); info.StackSize != 0 { + t.Fatalf("failed create pushed an undo entry: %+v", info) + } +} + +// The reciprocal exit still lands in the written destination file when the +// create succeeds. +func TestSuccessfulRoomCreateWritesReciprocalExit(t *testing.T) { + s := newTestAdminServer(t, map[int]string{1: "id: 1\nname: Origin\n"}) + s.cfg = config.Default() + s.cfg.StartingRoom = 1 + + rec := httptest.NewRecorder() + s.handleRooms(rec, httptest.NewRequest(http.MethodPost, "/api/rooms", + strings.NewReader(`{"name":"Next","link_from":1,"link_dir":"north"}`))) + if strings.Contains(rec.Body.String(), "error") { + t.Fatalf("create failed: %s", rec.Body.String()) + } + + dest, err := s.world.LoadRoom(2) + if err != nil { + t.Fatalf("destination room not loadable: %v", err) + } + if dest.Exits[world.South].Room != 1 { + t.Fatalf("destination room missing its reciprocal exit south: %+v", dest.Exits) + } + src, err := s.world.LoadRoom(1) + if err != nil { + t.Fatal(err) + } + if src.Exits[world.North].Room != 2 { + t.Fatalf("source room missing its north exit: %+v", src.Exits) + } +} diff --git a/internal/admin/yaml_util.go b/internal/admin/yaml_util.go index 6ea6692..c042248 100644 --- a/internal/admin/yaml_util.go +++ b/internal/admin/yaml_util.go @@ -226,8 +226,26 @@ func renameEntityFile(dataDir, subdir, oldID, newID string) (string, string, []b if oldPath == newPath { return "", "", nil, fmt.Errorf("new name is same as current") } - if _, err := os.Stat(newPath); err == nil { - return "", "", nil, fmt.Errorf("entity %s already exists", newID) + // The ID namespace is global across the entity type's subdirectory tree + // (findEntityPath and the runtime path indexes resolve by filename + // anywhere under data/<subdir>), so the collision check must cover the + // whole tree, not just the file's own directory — otherwise renaming + // into an ID that exists in ANOTHER directory creates a duplicate ID + // shadowed by whichever file the index happens to find first. + target := newID + ".yaml" + var clash string + filepath.WalkDir(filepath.Join(dataDir, subdir), func(path string, d os.DirEntry, err error) error { + if err != nil || d.IsDir() { + return nil + } + if d.Name() == target { + clash = path + return filepath.SkipAll + } + return nil + }) + if clash != "" { + return "", "", nil, fmt.Errorf("entity %s already exists at %s", newID, clash) } data, err := os.ReadFile(oldPath) if err != nil { diff --git a/internal/game/casino.go b/internal/game/casino.go index f9eb771..79d324a 100644 --- a/internal/game/casino.go +++ b/internal/game/casino.go @@ -103,16 +103,28 @@ func (w casinoWallet) payCreditsFlagged(amount int, refund bool) error { // resort: pay through the character file only. The save below is // the commit point — the money is durable even without a journal // record, so the settlement may be announced as paid. The stake - // resolution evidence (ResolveStakes) keeps recovery from treating - // the round as unresolved; without a payout record, an open wager - // can no longer be distinguished from a lost one, so the balance - // itself is the only record of what happened. + // resolution evidence (recordStakeEvidence) recorded in that same + // file keeps recovery from treating the round as unresolved; + // without a payout record, an open wager can no longer be + // distinguished from a lost one, so the balance itself is the only + // record of what happened. if capErr != nil { return capErr } w.p.Credits = creditsAfter + // The character file is the only durable record, so it must also + // carry the resolution evidence for this player's open stakes: a + // crash before the games' own ResolveStakes call would otherwise + // leave the wager records open with checkpoint balances that no + // longer match, and recovery would refund chip stakes the paid + // settlement had already consumed. The single save below is the + // commit point for the payout AND the resolution. + resolved := w.recordStakeEvidence() if serr := w.g.AccountStore.SaveCharacter(w.p); serr != nil { w.p.Credits = creditsBefore + if resolved > 0 { + w.p.ResolvedStakes = w.p.ResolvedStakes[:len(w.p.ResolvedStakes)-resolved] + } return casino.ErrWalletPersistence } kind := "settlement" @@ -174,6 +186,23 @@ func (w casinoWallet) ResolveStakes() { } } +// recordStakeEvidence adds every currently open stake of the player to the +// file-side resolution evidence list and returns how many IDs were newly +// recorded. Used when the economy journal is unavailable: the open wager +// record IDs in the character file prove the round resolved even though no +// settled marker (or payout record) could ever be journaled. The caller +// must persist the character file for the evidence to count. +func (w casinoWallet) recordStakeEvidence() int { + added := 0 + for _, rec := range w.g.Economy.openStakes(economyKey(w.p)) { + if !resolvedStakeRecorded(w.p, rec.ID) { + w.p.ResolvedStakes = append(w.p.ResolvedStakes, rec.ID) + added++ + } + } + return added +} + // RefundOutstanding returns every outstanding stake to the player: chips // come back as chips, credits through the durable payout path. The stakes // are resolved only once the return is durable; on failure the chip return diff --git a/internal/game/core_login_account.go b/internal/game/core_login_account.go index 371c287..f6e5631 100644 --- a/internal/game/core_login_account.go +++ b/internal/game/core_login_account.go @@ -19,6 +19,7 @@ func (g *Game) handleAccountName(sess *net.Session, input string) { actualName, err := g.AccountStore.FindAccount(name) if err == nil { sess.Account = &player.Account{Name: actualName} + sess.SetAuthName(actualName) sess.State = net.StatePassword sess.SetEcho(false) sess.Write("Password: ") @@ -29,6 +30,7 @@ func (g *Game) handleAccountName(sess *net.Session, input string) { return } sess.Account = &player.Account{Name: name} + sess.SetAuthName(name) sess.State = net.StateNewAccountPass sess.SetEcho(false) sess.WriteLine("\r\nOh, a new visitor to this rock!") @@ -99,17 +101,9 @@ type passwordAuth struct { // The production entry point; handlePassword stays for direct HandleSession // callers (tests) but hashes on the owner there. func (g *Game) handlePasswordOffOwner(sess *net.Session, input string) { - if sess.Account == nil { - // Defensive: password prompts only follow an account-name step. - g.RunOnOwner(func() { - if g.isShuttingDown() { - return - } - g.handlePassword(sess, input) - }) - return - } if input == "" || len(input) > 128 { + // These checks touch no session state, so they are race-free off + // the owner; they also keep Argon2id off absurd inputs. tooshort := input == "" g.RunOnOwner(func() { if g.isShuttingDown() { @@ -127,8 +121,24 @@ func (g *Game) handlePasswordOffOwner(sess *net.Session, input string) { }) return } + // The account name comes from the race-free authName snapshot (set by + // the owner when the password prompt was offered), NOT from + // sess.Account: the owner goroutine can replace Account at any moment + // (account republish after a rename/purge), and reading a plain field + // the owner concurrently writes is a data race. + name := sess.AuthName() + if name == "" { + // Defensive: password prompts only follow an account-name step. + g.RunOnOwner(func() { + if g.isShuttingDown() { + return + } + g.handlePassword(sess, input) + }) + return + } auth := passwordAuth{} - acc, err := g.AccountStore.LoadAccount(sess.Account.Name) + acc, err := g.AccountStore.LoadAccount(name) switch { case err != nil: auth.loadErr = true @@ -251,20 +261,21 @@ func (g *Game) handleNewAccount(sess *net.Session, input string) { // for the existence re-check and save. func (g *Game) handleNewAccountOffOwner(sess *net.Session, input string) { input = strings.TrimSpace(input) - if input == "" || input != sess.PendingPass { - g.RunOnOwner(func() { - if g.isShuttingDown() { - return - } - g.handleNewAccount(sess, input) - }) - return - } + // Hash unconditionally on the connection goroutine (Argon2id is + // expensive); the owner task below re-checks the empty/mismatch cases + // against owner-owned session state, so PendingPass is never read off + // the owner. A wasted hash only happens on the rare mismatched-confirm + // path, where handleNewAccount's own validation rejects it without a + // second hash. hash, hashErr := player.HashPassword(input) g.RunOnOwner(func() { if g.isShuttingDown() { return } + if input == "" || input != sess.PendingPass { + g.handleNewAccount(sess, input) + return + } g.applyNewAccount(sess, hash, hashErr) }) } diff --git a/internal/game/login_race_regression_test.go b/internal/game/login_race_regression_test.go new file mode 100644 index 0000000..6448cf5 --- /dev/null +++ b/internal/game/login_race_regression_test.go @@ -0,0 +1,77 @@ +package game + +// The off-owner login paths must never read owner-owned session fields: +// the owner goroutine replaces Session.Account at any moment (account +// republish after a rename or purge races any concurrent login), so the +// connection goroutine verifies passwords against the race-free authName +// snapshot instead. + +import ( + "testing" + + "thehouseoficarus/internal/net" +) + +// TestOffOwnerLoginDoesNotRaceAccountPublish drives the real server entry +// point through a login while a republish loop keeps replacing the +// session's cached account on the owner — the exact race a second +// authenticated session (or an admin-triggered account update) creates +// while the first session is typing its password. Run with -race. +func TestOffOwnerLoginDoesNotRaceAccountPublish(t *testing.T) { + h := newHarness(t) + h.seedAccount("alice") + h.seedCharacter("alice", "alice") + + sess := &net.Session{Conn: newChanConn(), State: net.StateAccountName} + sess.StartWriter() + defer sess.Close() + h.hub.Add(sess) + + done := make(chan struct{}) + go func() { + defer close(done) + for i := 0; i < 200; i++ { + h.g.RunOnOwner(func() { h.g.publishAccountUpdate("alice", "alice") }) + } + }() + + h.g.HandleSessionOnOwner(sess, "alice") // owner: offers the password prompt + h.g.HandleSessionOnOwner(sess, "pw") // off-owner: verifies the password + <-done + + if sess.AuthName() != "alice" { + t.Fatalf("password prompt lost its account name: %q", sess.AuthName()) + } + if sess.Account == nil || sess.Account.Name != "alice" { + t.Fatalf("login did not complete: %+v", sess.Account) + } +} + +// TestPasswordPromptRejectsEmptyAndOversizedInput keeps the documented +// fast-path behavior of the off-owner password handler after the +// authName rework: an empty line re-prompts (echo still off) and an +// oversized line is rejected without running Argon2id on it. +func TestPasswordPromptRejectsEmptyAndOversizedInput(t *testing.T) { + h := newHarness(t) + h.seedAccount("alice") + + sess := &net.Session{Conn: newChanConn(), State: net.StateAccountName} + sess.StartWriter() + defer sess.Close() + h.hub.Add(sess) + + h.g.HandleSessionOnOwner(sess, "alice") + if sess.State != net.StatePassword { + t.Fatalf("state after account name = %v, want StatePassword", sess.State) + } + + h.g.HandleSessionOnOwner(sess, "") + if sess.State != net.StatePassword { + t.Fatalf("empty password line must stay in StatePassword, got %v", sess.State) + } + + h.g.HandleSessionOnOwner(sess, string(make([]byte, 200))) + if sess.State != net.StateAccountName { + t.Fatalf("oversized password must return to StateAccountName, got %v", sess.State) + } +} diff --git a/internal/game/payout_evidence_regression_test.go b/internal/game/payout_evidence_regression_test.go new file mode 100644 index 0000000..b58bf9d --- /dev/null +++ b/internal/game/payout_evidence_regression_test.go @@ -0,0 +1,76 @@ +package game + +// Regression: when the economy journal is unavailable, a payout paid +// through the character file must record the round's stake-resolution +// evidence in that same file. Otherwise a crash before the games' own +// ResolveStakes call leaves the wager records open with checkpoint +// balances that no longer match, and recovery refunds chip stakes the +// paid settlement had already consumed — duplicating the round's value. + +import ( + "testing" + + "thehouseoficarus/internal/player" +) + +func TestUnjournalablePayoutCrashDoesNotRefundChips(t *testing.T) { + h := newHarness(t) + h.seedAccount("alice") + p := h.seedCharacter("alice", "alice") + h.g.RunOnOwner(func() { + p.Credits = 100 + p.SetInvSlot(0, &player.InventorySlot{ItemID: "chips", Quantity: 10}) + w := casinoWallet{g: h.g, p: p} + if _, err := w.Wager(10); err != nil { + t.Error(err) + return + } + path := h.g.Economy.path + h.g.Economy.path = h.dir // persistent append failure + if err := w.PayCredits(20); err != nil { + t.Errorf("fallback payout failed: %v", err) + return + } + h.g.Economy.path = path + // Process loss BEFORE the game's own ResolveStakes call: the + // fallback payout itself must be the durable resolution. + h.g.recoverEconomy() + loaded, err := h.g.AccountStore.LoadCharacter("alice") + if err != nil { + t.Error(err) + return + } + if loaded.Credits != 120 || countChips(loaded) != 0 { + t.Errorf("recovered credits=%d chips=%d; want 120 credits and no chips (the paid settlement consumed the stake)", loaded.Credits, countChips(loaded)) + } + if open := h.g.Economy.mustLoadOpen(t); len(open) != 0 { + t.Errorf("records left open after recovery: %+v", open) + } + }) +} + +// The stakes-open count is a proxy for "recovery did not silently refund": +// with the fix the wager record settles through the file-side evidence. +func TestUnjournalablePayoutEvidenceRecorded(t *testing.T) { + h := newHarness(t) + h.seedAccount("alice") + p := h.seedCharacter("alice", "alice") + h.g.RunOnOwner(func() { + p.Credits = 100 + w := casinoWallet{g: h.g, p: p} + if _, err := w.Wager(10); err != nil { + t.Error(err) + return + } + path := h.g.Economy.path + h.g.Economy.path = h.dir + if err := w.PayCredits(20); err != nil { + t.Errorf("fallback payout failed: %v", err) + return + } + h.g.Economy.path = path + if len(p.ResolvedStakes) == 0 { + t.Error("fallback payout recorded no stake-resolution evidence; a crash before ResolveStakes would double-refund chips") + } + }) +} diff --git a/internal/net/server.go b/internal/net/server.go index eed5486..66d31e3 100644 --- a/internal/net/server.go +++ b/internal/net/server.go @@ -81,6 +81,14 @@ type Session struct { disconnecting atomic.Bool disconnectTicks atomic.Int64 + // authName carries the account name for the password-entry state. The + // owner goroutine sets it when offering the password prompt, while the + // connection goroutine reads it for the off-owner password + // verification — so unlike Session.Account (which ONLY the owner may + // touch, and which account republish can replace concurrently), it is + // an atomic: a plain field read off the owner is a data race. + authName atomic.Pointer[string] + // Outbound queueing. Transport sessions (telnet/TLS/websocket) are put // into queued mode by startWriter before their input goroutine begins: // Write* then enqueues into outbox, and the dedicated writer goroutine @@ -108,6 +116,21 @@ type Session struct { seq uint64 } +// SetAuthName records the account name offered for password entry. +// Owner goroutine only. +func (sess *Session) SetAuthName(name string) { + sess.authName.Store(&name) +} + +// AuthName returns the account name recorded for password entry, or "" +// when none was recorded. Safe on the connection goroutine. +func (sess *Session) AuthName() string { + if p := sess.authName.Load(); p != nil { + return *p + } + return "" +} + // sessionSeq is the source of Session.seq values. It is incremented under the // adding Hub's mutex, so no extra atomicity is needed for correctness; atomic // access keeps the package-level default counter safe if Hub.Add is ever @@ -126,7 +149,15 @@ type Server struct { done chan struct{} shutdownOnce sync.Once - listenErr error + // finishOnce guards the single close of done: both the graceful + // shutdown path and a fatal transport failure (a listener that cannot + // bind, an Accept loop that dies) converge on finish, so ListenAndServe + // unblocks either way and done is never closed twice. + finishOnce sync.Once + listenErr error + // stopping flips when Shutdown begins so listener goroutines treat + // their subsequent Accept/serve errors as expected, not fatal. + stopping atomic.Bool } type Hub struct { @@ -511,11 +542,27 @@ func (s *Server) ListenAndServe(handler func(*Session, string)) error { return s.listenErr } +// finish records the terminal transport error (nil for a clean shutdown) +// exactly once and unblocks ListenAndServe. Fatal transport failures — a +// listener that cannot bind or an Accept loop that dies outside shutdown — +// reach it too, so a misconfigured server does not sit running with no way +// in. +func (s *Server) finish(err error) { + s.finishOnce.Do(func() { + s.listenErr = err + close(s.done) + }) +} + // Shutdown stops all transports: HTTP(S) servers drain, listeners close, and // every session is closed (its outbound queue drains first). ListenAndServe // returns after Shutdown completes. It is safe to call more than once. func (s *Server) Shutdown() { s.shutdownOnce.Do(func() { + // Before touching any listener: goroutines serving those listeners + // check this flag so their resulting errors are not reported as + // fatal. + s.stopping.Store(true) if s.httpServer != nil { ctx, cancel := context.WithTimeout(context.Background(), 3*time.Second) defer cancel() @@ -535,7 +582,7 @@ func (s *Server) Shutdown() { for _, sess := range s.hub.AllSessions() { sess.Close() } - close(s.done) + s.finish(nil) }) <-s.done } @@ -544,6 +591,10 @@ func (s *Server) serveTelnet() { for { conn, err := s.telnetLn.Accept() if err != nil { + if !s.stopping.Load() { + log.Printf("telnet listener error: %v", err) + s.finish(fmt.Errorf("telnet listener: %w", err)) + } return } sess := &Session{ @@ -563,6 +614,10 @@ func (s *Server) serveTelnetTLS() { for { conn, err := s.telnetTLSLn.Accept() if err != nil { + if !s.stopping.Load() { + log.Printf("telnet+tls listener error: %v", err) + s.finish(fmt.Errorf("telnet+tls listener: %w", err)) + } return } sess := &Session{ @@ -579,6 +634,9 @@ func (s *Server) serveHTTP() { err := s.httpServer.ListenAndServe() if err != nil && err != http.ErrServerClosed { log.Printf("http server error: %v", err) + if !s.stopping.Load() { + s.finish(fmt.Errorf("http server: %w", err)) + } } } @@ -587,11 +645,17 @@ func (s *Server) serveHTTPS() { ln, err := net.Listen("tcp", addr) if err != nil { log.Printf("https server error: %v", err) + if !s.stopping.Load() { + s.finish(fmt.Errorf("https listen: %w", err)) + } return } tlsLn := tls.NewListener(ln, s.httpsTLSConfig) if err := s.httpsServer.Serve(tlsLn); err != nil && err != http.ErrServerClosed { log.Printf("https server error: %v", err) + if !s.stopping.Load() { + s.finish(fmt.Errorf("https server: %w", err)) + } } } diff --git a/internal/net/transport_failure_test.go b/internal/net/transport_failure_test.go new file mode 100644 index 0000000..ecd41e8 --- /dev/null +++ b/internal/net/transport_failure_test.go @@ -0,0 +1,44 @@ +package net + +// Regression: a fatal transport failure (a listener that cannot bind, an +// Accept loop that dies outside shutdown) must unblock ListenAndServe with +// the error — a server must never sit running with no way in — and a +// failure followed by Shutdown must not panic on a double close of done. + +import ( + "fmt" + "net" + "net/http" + "testing" + "time" +) + +func TestTransportFailureStopsListenAndServe(t *testing.T) { + ln, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + t.Fatal(err) + } + defer ln.Close() + port := fmt.Sprintf("127.0.0.1:%d", ln.Addr().(*net.TCPAddr).Port) + + s := &Server{hub: NewHub(), done: make(chan struct{})} + s.httpServer = &http.Server{Addr: port, Handler: http.NewServeMux()} + + result := make(chan error, 1) + go func() { + go s.serveHTTP() + <-s.done + result <- s.listenErr + }() + + select { + case err := <-result: + if err == nil { + t.Fatal("transport failure unblocked ListenAndServe without an error") + } + // Shutdown after a failure must not double-close done. + s.Shutdown() + case <-time.After(2 * time.Second): + t.Fatal("a failed listener left the server running") + } +} |