From 61881fd0c1a7066293f552be1a747143f70d03ff Mon Sep 17 00:00:00 2001 From: historia <[not public]> Date: Tue, 15 Sep 2026 20:44:31 -0400 Subject: fix: registration overwrite, recovery chip/payout edge cases, bulk-delete rollback --- internal/admin/api_rooms_bulk_delete.go | 22 +++++++ internal/admin/api_rooms_bulk_delete_test.go | 70 ++++++++++++++++++++ .../game/account_registration_regression_test.go | 55 ++++++++++++++++ internal/game/core_login_account.go | 13 ++++ internal/game/economy.go | 71 +++++++++++++++----- internal/game/economy_recovery_regression_test.go | 77 ++++++++++++++++++++++ 6 files changed, 292 insertions(+), 16 deletions(-) create mode 100644 internal/admin/api_rooms_bulk_delete_test.go create mode 100644 internal/game/account_registration_regression_test.go diff --git a/internal/admin/api_rooms_bulk_delete.go b/internal/admin/api_rooms_bulk_delete.go index 18b10a5..70c2eb6 100644 --- a/internal/admin/api_rooms_bulk_delete.go +++ b/internal/admin/api_rooms_bulk_delete.go @@ -4,6 +4,7 @@ import ( "encoding/json" "fmt" "io" + "log" "net/http" "os" @@ -38,6 +39,18 @@ func (s *AdminServer) handleBulkDelete(w http.ResponseWriter, r *http.Request) { return } + // Duplicate IDs in one request would delete the same file twice: the + // second os.Remove fails after the room is already gone, and the + // handler aborts without pushing an undo record. Reject up front. + seenIDs := make(map[int]bool, len(req.IDs)) + for _, id := range req.IDs { + if seenIDs[id] { + writeJSON(w, map[string]any{"error": fmt.Sprintf("duplicate room id %d in request", id)}) + return + } + seenIDs[id] = true + } + type roomSnapshot struct { id int path string @@ -63,6 +76,15 @@ func (s *AdminServer) handleBulkDelete(w http.ResponseWriter, r *http.Request) { for _, snap := range snapshots { s.detachRoomFromCourse(snap.id) if err := os.Remove(snap.path); err != nil { + // All-or-nothing: restore every room deleted so far so a failed + // bulk delete never strands rooms without an undo record. + for _, done := range snapshots { + if deletedSet[done.id] { + if werr := os.WriteFile(done.path, done.oldContent, 0644); werr != nil { + log.Printf("bulk-delete rollback of room %d: %v", done.id, werr) + } + } + } writeJSON(w, map[string]any{"error": fmt.Sprintf("delete room %d: %v", snap.id, err)}) return } diff --git a/internal/admin/api_rooms_bulk_delete_test.go b/internal/admin/api_rooms_bulk_delete_test.go new file mode 100644 index 0000000..083b890 --- /dev/null +++ b/internal/admin/api_rooms_bulk_delete_test.go @@ -0,0 +1,70 @@ +package admin + +// Bulk-delete regression tests: duplicate IDs in one request are rejected +// before any file is touched, and a mid-plan delete failure rolls back +// every room it already removed (no rooms stranded without an undo record). + +import ( + "encoding/json" + "net/http" + "net/http/httptest" + "os" + "path/filepath" + "strings" + "testing" +) + +func postBulkDelete(t *testing.T, s *AdminServer, body string) (map[string]any, int) { + t.Helper() + req := httptest.NewRequest(http.MethodPost, "/api/rooms/bulk-delete", strings.NewReader(body)) + rec := httptest.NewRecorder() + s.handleBulkDelete(rec, req) + var resp map[string]any + _ = json.Unmarshal(rec.Body.Bytes(), &resp) + return resp, rec.Code +} + +func TestBulkDeleteRejectsDuplicateIDs(t *testing.T) { + s := newTestAdminServer(t, map[int]string{1: "id: 1\nname: Room\n"}) + path, _ := s.world.GetRoomPath(1) + resp, _ := postBulkDelete(t, s, `{"ids":[1,1]}`) + if errMsg, _ := resp["error"].(string); !strings.Contains(errMsg, "duplicate room id 1") { + t.Fatalf("duplicate IDs not rejected: %v", resp) + } + if _, err := os.Stat(path); err != nil { + t.Fatalf("rejected bulk-delete removed the room: %v", err) + } +} + +// TestBulkDeleteFailureRollsBackDeletions: a delete failure mid-plan +// restores the rooms already removed, so a failed plan leaves the world +// untouched. The failure is staged at the DELETE phase (not the earlier +// snapshot phase) by making room 3's parent directory unwritable after its +// file snapshot was taken. +func TestBulkDeleteFailureRollsBackDeletions(t *testing.T) { + s := newTestAdminServer(t, map[int]string{1: "id: 1\nname: One\n"}) + sub := filepath.Join(s.dataDir, "rooms", "sub") + if err := os.MkdirAll(sub, 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(sub, "3.yaml"), []byte("id: 3\nname: Three\n"), 0o644); err != nil { + t.Fatal(err) + } + s.world.RebuildRoomIndex(s.dataDir) + + // Room 3's snapshot succeeds (the file is readable), but its removal + // fails because the parent directory is not writable. + if err := os.Chmod(sub, 0o555); err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = os.Chmod(sub, 0o755); os.RemoveAll(sub) }) + + path1, _ := s.world.GetRoomPath(1) + resp, _ := postBulkDelete(t, s, `{"ids":[1,3]}`) + if errMsg, _ := resp["error"].(string); !strings.Contains(errMsg, "delete room 3") { + t.Fatalf("expected room-3 delete failure: %v", resp) + } + if _, err := os.Stat(path1); err != nil { + t.Fatalf("failed bulk delete stranded room 1 without an undo record: %v", err) + } +} diff --git a/internal/game/account_registration_regression_test.go b/internal/game/account_registration_regression_test.go new file mode 100644 index 0000000..5ecd254 --- /dev/null +++ b/internal/game/account_registration_regression_test.go @@ -0,0 +1,55 @@ +package game + +// Registration regression: a pending new-account session whose typed name +// was created in the meantime (by another concurrent registration) must be +// refused at confirm time, never overwrite the existing account's password +// or character list. + +import ( + "testing" + + "thehouseoficarus/internal/net" + "thehouseoficarus/internal/player" +) + +func TestPendingRegistrationCannotOverwriteExistingAccount(t *testing.T) { + h := newHarness(t) + h.seedAccount("alice", "pw", false) + p := h.seedCharacter("alice", "alice") + + a := &net.Session{Conn: newChanConn(), State: net.StateAccountName} + b := &net.Session{Conn: newChanConn(), State: net.StateAccountName} + h.g.RunOnOwner(func() { + // Both sessions type the same name while it does not exist yet; + // both reach the password-confirmation state. + h.g.handleAccountName(a, "wait") + h.g.handleAccountName(b, "wait") + h.g.handleNewPass(a, "firstpassword") + h.g.handleNewPass(b, "otherpassword") + // Session a wins the creation race... + h.g.handleNewAccount(a, "firstpassword") + // ...and a character is attached to the account before b confirms. + acc, err := h.g.AccountStore.LoadAccount("wait") + if err != nil { + t.Error(err) + return + } + acc.Characters = []string{p.Name} + if err := h.g.AccountStore.SaveAccount(acc); err != nil { + t.Error(err) + return + } + // Session b confirms second: this must not clobber the account. + h.g.handleNewAccount(b, "otherpassword") + }) + acc, err := h.g.AccountStore.LoadAccount("wait") + if err != nil { + t.Fatal(err) + } + if !player.CheckPassword("firstpassword", acc.PasswordHash) { + t.Errorf("winner's password was overwritten by the losing registration") + } + if len(acc.Characters) != 1 { + t.Errorf("character list destroyed by the losing registration: %v", acc.Characters) + } +} diff --git a/internal/game/core_login_account.go b/internal/game/core_login_account.go index fbd9e3a..60cca2c 100644 --- a/internal/game/core_login_account.go +++ b/internal/game/core_login_account.go @@ -145,6 +145,19 @@ func (g *Game) handleNewAccount(sess *net.Session, input string) { return } + // Re-check existence as part of the same owner step as the save. The + // name was validated as new when it was typed, but another session (or + // the admin API) may have created it in the meantime; saving without + // this check would overwrite that account's password and characters. + if _, findErr := g.AccountStore.FindAccount(sess.Account.Name); findErr == nil { + sess.Conn.SetEcho(true) + sess.Account = nil + sess.State = net.StateAccountName + sess.WriteLine("That account name was just taken; please sign in instead.") + sess.Write("Account name: ") + return + } + acc := &player.Account{ Name: sess.Account.Name, PasswordHash: hash, diff --git a/internal/game/economy.go b/internal/game/economy.go index fd12d07..04508be 100644 --- a/internal/game/economy.go +++ b/internal/game/economy.go @@ -400,17 +400,26 @@ func (g *Game) recoverEconomy() { switch { case p.Credits == rec.CreditsBefore: // The payout (and every identical sibling) was lost in the - // crash: the player is owed them all. + // crash: the player is owed them all. Only the siblings whose + // re-application actually became durable are settled; a save + // failure (e.g. the credit cap) leaves the unpaid remainder open + // for operator inspection — a liability is never erased. + paid := 0 for m := 0; m < len(group); m++ { if err := g.payCreditsDurable(p, rec.Credit); err != nil { fmt.Fprintf(os.Stderr, "economy recovery: payout save %s: %v\n", p.Name, err) break } + paid++ compensated++ } for _, other := range group { + if paid == 0 { + break + } g.Economy.settle(other) settled[other.ID] = true + paid-- } case p.Credits == rec.CreditsAfter && len(group) == 1: g.Economy.settle(rec) @@ -435,37 +444,61 @@ func (g *Game) recoverEconomy() { if p == nil { continue } + // Checkpoint comparisons use the DURABLE state, captured before any + // in-memory chip restoration below. creditsNow, chipsNow := p.Credits, countChips(p) + // Chips are recovered independently of the credit checkpoints. Any + // state where the chip debit is durable but the chips have not come + // back (post-debit chip balance) still owes the chips, even when a + // later payout record already resolves the credit side. A durable + // chip refund always restores the balance to ChipsBefore, so this + // cannot double-return. + chipsOwed := rec.Chips > 0 && chipsNow == rec.ChipsAfter + if chipsOwed && !g.giveChips(p, rec.Chips) { + // No inventory room for the chip return: keep the record open + // for operator inspection rather than silently dropping the + // chips; a later startup (or operator action) retries. + fmt.Fprintf(os.Stderr, "economy recovery: wager %s for %s: no inventory room for %d chips — kept open for operator inspection\n", + rec.ID, rec.Player, rec.Chips) + continue + } + saveChipReturn := func() error { + if !chipsOwed { + return nil + } + if err := g.AccountStore.SaveCharacter(p); err != nil { + p.RemoveItem("chips", rec.Chips) + fmt.Fprintf(os.Stderr, "economy recovery: chip return save %s: %v\n", p.Name, err) + return err + } + return nil + } switch { case creditsNow == rec.CreditsBefore && chipsNow == rec.ChipsBefore: // The debit never became durable: void it. g.Economy.void(rec) - case creditsNow == rec.CreditsAfter && chipsNow == rec.ChipsAfter: + case creditsNow == rec.CreditsAfter: if resolvedByPayout { // A later payout record proves the round resolved (the win, - // push, or refund already returned this stake inside that - // payout). Only the resolution marker was lost. - g.Economy.settle(rec) + // push, or refund already returned the credit stake inside + // that payout). Only the resolution marker was lost; the + // chip stake may still be outstanding. + if saveChipReturn() == nil { + g.Economy.settle(rec) + } continue } // The debit is durable but the round was lost in the crash: - // refund the stake (approved refund-on-recovery policy). + // refund the stake (approved refund-on-recovery policy). The + // chips (if owed) are already restored in memory; one durable + // save covers both the chip return and the credit refund. p.Credits += rec.Credit - if rec.Chips > 0 && !g.giveChips(p, rec.Chips) { - // No inventory room for the chip return: revert the credit - // and keep the record open for operator inspection rather - // than silently dropping the chips. - p.Credits = creditsNow - fmt.Fprintf(os.Stderr, "economy recovery: wager %s for %s: no inventory room for %d chips — kept open for operator inspection\n", - rec.ID, rec.Player, rec.Chips) - continue - } if err := g.AccountStore.SaveCharacter(p); err != nil { // Revert the in-memory refund so later records for this // player still compare against the durable balance; the // record stays open and a later startup can retry. p.Credits = creditsNow - if rec.Chips > 0 { + if chipsOwed { p.RemoveItem("chips", rec.Chips) } fmt.Fprintf(os.Stderr, "economy recovery: refund save %s: %v\n", p.Name, err) @@ -474,6 +507,12 @@ func (g *Game) recoverEconomy() { g.Economy.settle(rec) compensated++ default: + // Persist an owed chip return even when the credit side stays + // ambiguous: the record remains open either way, but the chips + // must not be silently lost. A failed save reverts them. + if saveChipReturn() != nil { + continue + } if resolvedByPayout { g.Economy.settle(rec) continue diff --git a/internal/game/economy_recovery_regression_test.go b/internal/game/economy_recovery_regression_test.go index 738dece..12df39b 100644 --- a/internal/game/economy_recovery_regression_test.go +++ b/internal/game/economy_recovery_regression_test.go @@ -282,3 +282,80 @@ func TestRecoveryPreservesAmbiguousPayout(t *testing.T) { t.Fatalf("ambiguous payout must stay open for operator inspection: %+v", open) } } + +// TestRecoveryKeepsUnpaidSiblingsOpenAtCreditCap: two identical open payout +// records where re-applying the second would exceed the economy cap must +// settle only the sibling that was actually paid. The unpaid 20-credit +// liability is never erased: its record stays open for operator inspection. +func TestRecoveryKeepsUnpaidSiblingsOpenAtCreditCap(t *testing.T) { + h := newHarness(t) + h.seedAccount("alice", "pw", false) + p := h.seedCharacter("alice", "alice") + h.g.RunOnOwner(func() { + p.Credits = casino.MaxCredits - 30 + if err := h.g.AccountStore.SaveCharacter(p); err != nil { + t.Error(err) + return + } + for i := 0; i < 2; i++ { + if _, err := h.g.Economy.start("payout", p, 0, 20, p.Credits, p.Credits+20, 0, 0); err != nil { + t.Error(err) + return + } + } + h.g.recoverEconomy() + }) + loaded, err := h.g.AccountStore.LoadCharacter("alice") + if err != nil { + t.Fatal(err) + } + if loaded.Credits != casino.MaxCredits-10 { + t.Fatalf("balance after capped recovery = %d, want %d (one payout applied)", loaded.Credits, casino.MaxCredits-10) + } + if open := h.g.Economy.loadOpen(); len(open) != 1 { + t.Fatalf("unpaid liability must stay open for operator inspection, open records: %+v", open) + } +} + +// TestRecoveryRestoresChipsFromMixedRefundCrash: a mixed chips+credits +// refund whose credit side is resolved by a later payout record but whose +// chip side was never saved must still return the chips. The wager record +// may be settled only once nothing is owed. +func TestRecoveryRestoresChipsFromMixedRefundCrash(t *testing.T) { + h := newHarness(t) + h.seedAccount("alice", "pw", false) + p := h.seedCharacter("alice", "alice") + h.g.RunOnOwner(func() { + p.Credits = 100 + p.SetInvSlot(0, &player.InventorySlot{ItemID: "chips", Quantity: 10}) + if err := h.g.AccountStore.SaveCharacter(p); err != nil { + t.Error(err) + return + } + // Durable 20-credit (10 chips + 10 credits) wager: after the debit + // the file holds 90 credits and no chips. + w := casinoWallet{g: h.g, p: p} + if _, err := w.Wager(20); err != nil { + t.Error(err) + return + } + // Crash during RefundOutstanding, after the credit-side payout was + // journaled but before anything beyond the debit was durable. + if _, err := h.g.Economy.start("payout", p, 0, 10, 90, 100, 10, 10); err != nil { + t.Error(err) + return + } + h.g.recoverEconomy() + }) + loaded, err := h.g.AccountStore.LoadCharacter("alice") + if err != nil { + t.Fatal(err) + } + if loaded.Credits != 100 || countChips(loaded) != 10 { + t.Fatalf("mixed refund after recovery = (%d credits, %d chips), want (100, 10); open records: %+v", + loaded.Credits, countChips(loaded), h.g.Economy.loadOpen()) + } + if open := h.g.Economy.loadOpen(); len(open) != 0 { + t.Fatalf("nothing may stay owed after recovery: %+v", open) + } +} -- cgit v1.2.3