From fcb806c3445ce56f2e57e208de773b4dac957c8d Mon Sep 17 00:00:00 2001 From: historia <[not public]> Date: Tue, 15 Sep 2026 20:35:23 -0400 Subject: fix: owner re-entrancy deadlock, telnet write queue, recovery/refund money safety, admin path traversal --- internal/game/casino.go | 16 ++- internal/game/casino_forfeit_regression_test.go | 23 ++++- internal/game/economy.go | 83 ++++++++++++---- internal/game/economy_recovery_regression_test.go | 115 ++++++++++++++++++++++ internal/game/owner.go | 54 +++++++++- internal/game/owner_disconnect_test.go | 71 +++++++++++++ 6 files changed, 339 insertions(+), 23 deletions(-) create mode 100644 internal/game/owner_disconnect_test.go (limited to 'internal/game') diff --git a/internal/game/casino.go b/internal/game/casino.go index 8ba9097..3f06b41 100644 --- a/internal/game/casino.go +++ b/internal/game/casino.go @@ -2,6 +2,7 @@ package game import ( "fmt" + "os" "strconv" "strings" "unicode/utf8" @@ -56,7 +57,11 @@ func (w casinoWallet) Wager(amount int) (casino.Wager, error) { // Revert the in-memory mutation: the play must not proceed on a // stake that could not be made durable. if chips > 0 { - w.g.giveChips(w.p, chips) + if !w.g.giveChips(w.p, chips) { + // Cannot happen (the debit just freed or reduced the chips + // stack), but surface it instead of silently losing chips. + fmt.Fprintf(os.Stderr, "casino wager revert: no inventory room for %d chips\n", chips) + } } w.p.Credits += credits w.g.Economy.void(rec) @@ -118,7 +123,14 @@ func (w casinoWallet) RefundOutstanding() (chips, credits int, err error) { return 0, 0, nil } if chips > 0 { - w.g.giveChips(w.p, chips) + if !w.g.giveChips(w.p, chips) { + // No inventory room for the chip return: nothing was refunded. + // The stakes stay open so recovery (or a later attempt once the + // player has inventory space) can complete the refund; callers + // report the stake as recorded rather than refunded, never as + // silently dropped. + return 0, 0, casino.ErrWalletPersistence + } } if credits > 0 { if err := w.PayCredits(credits); err != nil { diff --git a/internal/game/casino_forfeit_regression_test.go b/internal/game/casino_forfeit_regression_test.go index 9ad8016..db5ba31 100644 --- a/internal/game/casino_forfeit_regression_test.go +++ b/internal/game/casino_forfeit_regression_test.go @@ -42,10 +42,27 @@ func TestDisconnectForfeitsBlackjackStake(t *testing.T) { h := newHarness(t) h.seedAccount("alice", "pw", false) p := h.seedCharacter("alice", "alice") + // A natural opening hand (player or dealer blackjack) settles the round + // inside Bet() itself, before any disconnect can forfeit anything. Deal + // from deterministic seeds until the opening hand leaves the stake + // outstanding, so the regression exercises the mid-round forfeit path + // on every run instead of failing ~5% of the time on a lucky deal. + committed := false + for seed := 0; seed < 1000 && !committed; seed++ { + h.g.RunOnOwner(func() { + h.g.Casino = casino.NewManager(int64(seed)) + p.Credits = 100 + h.g.Casino.Join(1, casino.TableConfig{ID: "bj", Game: casino.GameBlackjack, MaxPlayers: 2}, "alice") + h.g.Casino.Bet(1, "bj", "alice", "", 10, casinoWallet{g: h.g, p: p}) + committed = len(h.g.Economy.openStakes("alice")) == 1 + }) + h.barrier() + } + if !committed { + t.Fatal("no manager seed left the blackjack stake outstanding; deal or settle logic changed") + } h.g.RunOnOwner(func() { - p.Credits = 100 - h.g.Casino.Join(1, casino.TableConfig{ID: "bj", Game: casino.GameBlackjack, MaxPlayers: 2}, "alice") - h.g.Casino.Bet(1, "bj", "alice", "", 10, casinoWallet{g: h.g, p: p}) + // The disconnect path forfeits the in-flight hand. h.g.Casino.MarkDisconnected("alice") }) h.barrier() diff --git a/internal/game/economy.go b/internal/game/economy.go index fcc0010..fd12d07 100644 --- a/internal/game/economy.go +++ b/internal/game/economy.go @@ -268,19 +268,24 @@ func (j *economyJournal) compact() { } } -// giveChips re-credits chips to a player's inventory (used by recovery -// refunds). Stacks onto an existing chips slot when one exists. -func (g *Game) giveChips(p *player.Player, qty int) { +// giveChips re-credits chips to a player's inventory (used by refund and +// recovery paths). It stacks onto an existing chips slot when one exists. +// It reports false when the chips do NOT fit (no existing stack and no free +// slot): the caller must then treat the chips as undelivered — they are +// never silently dropped; the money record stays open instead. +func (g *Game) giveChips(p *player.Player, qty int) bool { for _, slot := range p.Inventory { if slot != nil && slot.ItemID == "chips" { slot.Quantity += qty - return + return true } } slot := p.FirstFreeSlot() - if slot >= 0 { - p.SetInvSlot(slot, &player.InventorySlot{ItemID: "chips", Quantity: qty}) + if slot < 0 { + return false } + p.SetInvSlot(slot, &player.InventorySlot{ItemID: "chips", Quantity: qty}) + return true } // payCreditsDurable applies a checked credit increase and persists it. @@ -358,27 +363,58 @@ func (g *Game) recoverEconomy() { } compensated := 0 - // Phase 1 — payouts, in append order. + // Phase 1 — payouts, in append order. Records with identical + // checkpoints are decided as one group: k sibling records all journaled + // against balance `before` were all written against the same in-memory + // balance (a failed save reverts it), so either none became durable + // (the file is still at `before`: all k are owed) or at most one did + // (the file is at `after` and k == 1). Re-applying identical siblings + // one at a time would let the second record misread the first + // re-application as its own durability and silently drop the owed + // payout — the group is settled together instead. + type payoutSig struct { + player string + before, after int + credit int + } + siblings := make(map[payoutSig][]*economyRecord) + for i := range folded { + rec := &folded[i].rec + if rec.Kind == "payout" && rec.Status == "open" { + sig := payoutSig{rec.Player, rec.CreditsBefore, rec.CreditsAfter, rec.Credit} + siblings[sig] = append(siblings[sig], rec) + } + } + settled := make(map[string]bool) for i := range folded { rec := &folded[i].rec - if rec.Kind != "payout" || rec.Status != "open" { + if rec.Kind != "payout" || rec.Status != "open" || settled[rec.ID] { continue } p := loadPlayer(rec.Player) if p == nil { continue } + sig := payoutSig{rec.Player, rec.CreditsBefore, rec.CreditsAfter, rec.Credit} + group := siblings[sig] switch { case p.Credits == rec.CreditsBefore: - // The payout was lost in the crash: the player is owed it. - if err := g.payCreditsDurable(p, rec.Credit); err != nil { - fmt.Fprintf(os.Stderr, "economy recovery: payout save %s: %v\n", p.Name, err) - continue + // The payout (and every identical sibling) was lost in the + // crash: the player is owed them all. + 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 + } + compensated++ } + for _, other := range group { + g.Economy.settle(other) + settled[other.ID] = true + } + case p.Credits == rec.CreditsAfter && len(group) == 1: g.Economy.settle(rec) - compensated++ - case p.Credits == rec.CreditsAfter: - g.Economy.settle(rec) + settled[rec.ID] = true default: fmt.Fprintf(os.Stderr, "economy recovery: ambiguous payout %s for %s (credits %d, want before %d or after %d) — kept open for operator inspection\n", rec.ID, rec.Player, p.Credits, rec.CreditsBefore, rec.CreditsAfter) @@ -415,10 +451,23 @@ func (g *Game) recoverEconomy() { // The debit is durable but the round was lost in the crash: // refund the stake (approved refund-on-recovery policy). p.Credits += rec.Credit - if rec.Chips > 0 { - g.giveChips(p, rec.Chips) + 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 { + p.RemoveItem("chips", rec.Chips) + } fmt.Fprintf(os.Stderr, "economy recovery: refund save %s: %v\n", p.Name, err) continue } diff --git a/internal/game/economy_recovery_regression_test.go b/internal/game/economy_recovery_regression_test.go index e085e67..738dece 100644 --- a/internal/game/economy_recovery_regression_test.go +++ b/internal/game/economy_recovery_regression_test.go @@ -137,6 +137,121 @@ func TestRecoveryRefundsSequentialStakes(t *testing.T) { } } +// TestRecoveryReappliesIdenticalFailedPayouts: two won hands whose payout +// saves both failed leave two OPEN payout records with identical +// checkpoints (the in-memory balance reverts on save failure, so both were +// journaled against balance 90). Recovery must re-apply BOTH — settling +// the second as "already durable" because the first re-application moved +// the balance to `after` would silently drop 20 owed credits. +func TestRecoveryReappliesIdenticalFailedPayouts(t *testing.T) { + h := newHarness(t) + h.seedAccount("alice", "pw", false) + p := h.seedCharacter("alice", "alice") + h.g.RunOnOwner(func() { + p.Credits = 90 // post-debit balance: both payouts failed to save + if err := h.g.AccountStore.SaveCharacter(p); err != nil { + t.Error(err) + } + for i := 0; i < 2; i++ { + if _, err := h.g.Economy.start("payout", p, 0, 20, 90, 110, 0, 0); err != nil { + t.Error(err) + } + } + h.g.recoverEconomy() + }) + loaded, err := h.g.AccountStore.LoadCharacter("alice") + if err != nil { + t.Fatal(err) + } + if loaded.Credits != 130 { + t.Fatalf("balance after recovery = %d, want 130 (both identical failed payouts re-applied)", loaded.Credits) + } + if open := h.g.Economy.loadOpen(); len(open) != 0 { + t.Fatalf("records left open after recovery: %+v", open) + } +} + +// TestRefundOutstandingKeepsStakesOpenWithoutInventoryRoom: a chip refund +// into a FULL inventory must be refused (stake records stay open) instead +// of silently dropping the chips and marking the refund complete. Once the +// player frees a slot, the same outstanding stake refunds successfully. +func TestRefundOutstandingKeepsStakesOpenWithoutInventoryRoom(t *testing.T) { + h := newHarness(t) + h.seedAccount("alice", "pw", false) + p := h.seedCharacter("alice", "alice") + h.g.RunOnOwner(func() { + 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 + } + // The debit emptied the chips slot; fill EVERY slot (including the + // one the wager just freed) so the chip return has nowhere to go. + for p.FirstFreeSlot() >= 0 { + p.SetInvSlot(p.FirstFreeSlot(), &player.InventorySlot{ItemID: "filler", Quantity: 1}) + } + chips, credits, err := w.RefundOutstanding() + if err != casino.ErrWalletPersistence { + t.Errorf("refund error = %v, want ErrWalletPersistence (chips=%d credits=%d)", err, chips, credits) + } + if got := countChips(p); got != 0 { + t.Errorf("chips after refused refund = %d, want 0 (nothing delivered; tracked in the open stake)", got) + } + if open := h.g.Economy.openStakes(p.Name); len(open) != 1 { + t.Errorf("stake records after refused refund = %d, want 1 (kept open for a later attempt)", len(open)) + } + + // Free a slot: the SAME outstanding stake now refunds in full. + p.RemoveItem("filler", 1) + chips, credits, err = w.RefundOutstanding() + if err != nil || chips != 10 || credits != 0 { + t.Errorf("retry refund = (%d chips, %d credits, %v), want (10, 0, nil)", chips, credits, err) + } + if got := countChips(p); got != 10 { + t.Errorf("chips after retried refund = %d, want 10", got) + } + if open := h.g.Economy.openStakes(p.Name); len(open) != 0 { + t.Errorf("stake records after successful refund = %d, want 0", len(open)) + } + }) +} + +// TestRecoveryKeepsWagerOpenWhenChipsDoNotFit: recovery must not settle a +// chip stake as refunded when the character's inventory cannot hold the +// chips — the record stays open for operator inspection. +func TestRecoveryKeepsWagerOpenWhenChipsDoNotFit(t *testing.T) { + h := newHarness(t) + h.seedAccount("alice", "pw", false) + p := h.seedCharacter("alice", "alice") + h.g.RunOnOwner(func() { + // Durable state at crash time: the 10-chip debit landed (10 -> 0 + // chips on disk), credits untouched, and the inventory is FULL of + // non-chip filler, so recovery has nowhere to put the refund. + p.Credits = 100 + for i := 0; i < 28; i++ { + p.SetInvSlot(i, &player.InventorySlot{ItemID: "filler", Quantity: 1}) + } + if err := h.g.AccountStore.SaveCharacter(p); err != nil { + t.Error(err) + } + if _, err := h.g.Economy.start("wager", p, 10, 0, 100, 100, 10, 0); err != nil { + t.Error(err) + } + h.g.recoverEconomy() + }) + loaded, err := h.g.AccountStore.LoadCharacter("alice") + if err != nil { + t.Fatal(err) + } + if got := countChips(loaded); got != 0 { + t.Fatalf("chips after recovery = %d, want 0 (no inventory room; nothing delivered)", got) + } + if open := h.g.Economy.loadOpen(); len(open) != 1 { + t.Fatalf("chip wager must stay open for operator inspection, open records: %+v", open) + } +} + // TestRecoveryPreservesAmbiguousPayout: a payout whose checkpoints match // neither balance state stays open and is surfaced for operator inspection — // a liability is never silently dropped. diff --git a/internal/game/owner.go b/internal/game/owner.go index 4a3646e..8546d10 100644 --- a/internal/game/owner.go +++ b/internal/game/owner.go @@ -1,6 +1,12 @@ package game -import "sync" +import ( + "runtime" + "strconv" + "strings" + "sync" + "sync/atomic" +) // owner is the single execution authority for live game state. Every // mutation of players, sessions, the Hub membership, runtime maps, and all @@ -27,6 +33,12 @@ type owner struct { closed bool tasks chan func() done chan struct{} + // goid is the goroutine ID of the owner loop, recorded once the loop + // starts. Re-entrancy checks (Game.executingOnOwner) compare it to the + // CALLER's goroutine ID so only code actually running on the owner + // detects itself — an "owner is busy" flag would wrongly let outside + // callers bypass the queue while a task is in flight. + goid atomic.Uint64 } const ownerTaskQueue = 4096 @@ -42,6 +54,7 @@ func newOwner() *owner { } func (o *owner) loop() { + o.goid.Store(currentGoroutineID()) defer close(o.done) for task := range o.tasks { task() @@ -102,6 +115,16 @@ func (o *owner) stop() { // exists (synthetic Games in unit tests) fn runs inline on the caller's // goroutine; production Games always have one, started by New. func (g *Game) run(fn func()) { + // Already running on the owner goroutine itself (e.g. a tick callback + // re-entering through a Hub removal callback): run inline, exactly like + // owner code calling functions directly. Re-submitting would deadlock + // waiting for the running task to finish — itself. Only true identity + // passes: an outside caller while the owner is merely BUSY still + // queues (and blocks), preserving the single-owner invariant. + if g.executingOnOwner() { + fn() + return + } o := g.owner if o == nil { fn() @@ -165,6 +188,35 @@ func (g *Game) tryRun(fn func()) bool { return o.submit(fn) } +// currentGoroutineID returns the calling goroutine's ID parsed from its +// stack header ("goroutine 123 [running]:"), 0 when unparseable. +func currentGoroutineID() uint64 { + var buf [64]byte + n := runtime.Stack(buf[:], false) + s := strings.TrimPrefix(string(buf[:n]), "goroutine ") + end := strings.IndexByte(s, ' ') + if end <= 0 { + return 0 + } + id, err := strconv.ParseUint(s[:end], 10, 64) + if err != nil { + return 0 + } + return id +} + +// executingOnOwner reports whether the CALLING goroutine is the owner loop +// goroutine. Owner-executed code must invoke callbacks directly instead of +// re-submitting through run(), which would deadlock waiting for itself. +func (g *Game) executingOnOwner() bool { + o := g.owner + if o == nil { + return false + } + id := o.goid.Load() + return id != 0 && id == currentGoroutineID() +} + // stopOwner drains and stops the owner loop. Called once during graceful // shutdown after all entry points have stopped submitting. func (g *Game) stopOwner() { diff --git a/internal/game/owner_disconnect_test.go b/internal/game/owner_disconnect_test.go new file mode 100644 index 0000000..e43a341 --- /dev/null +++ b/internal/game/owner_disconnect_test.go @@ -0,0 +1,71 @@ +package game + +// Disconnect-cleanup regressions: DisconnectTick runs on the owner +// goroutine, so the Hub removal callback it triggers must execute inline +// rather than re-submitting through run() — which would deadlock waiting +// for the running tick to finish (freezing every tick afterward). + +import ( + "testing" + "time" + + "thehouseoficarus/internal/net" +) + +// TestDisconnectTickHardRemoveCompletesOnOwner guards the single-owner +// re-entrancy fix: DisconnectTick (owner) -> Hub.HardRemove -> onRemove +// callback must not block on the owner from the owner. +func TestDisconnectTickHardRemoveCompletesOnOwner(t *testing.T) { + h := newHarness(t) + h.seedAccount("alice", "pw", false) + h.seedCharacter("alice", "alice") + sess, _ := h.connect("alice") + + done := make(chan struct{}) + h.g.RunOnOwner(func() { + sess.Disconnecting = true + sess.DisconnectTicks = 1 + }) + h.barrier() + + go func() { + h.g.RunOnOwner(func() { h.g.DisconnectTick() }) + close(done) + }() + select { + case <-done: + case <-time.After(5 * time.Second): + t.Fatal("DisconnectTick deadlocked: Hub.HardRemove's onRemove callback re-submitted to the owner from the owner") + } + + // The session was hard-removed and its character persisted. + if sessions := h.g.Hub.AllSessions(); len(sessions) != 0 { + t.Fatalf("session still in hub after disconnect tick: %d", len(sessions)) + } + if _, err := h.g.AccountStore.LoadCharacter("alice"); err != nil { + t.Fatalf("character file unreadable after disconnect save: %v", err) + } +} + +// TestHubRemoveNilPlayerOnOwner: DisconnectTick removes sessions without a +// loaded player through Hub.Remove, which runs the same onRemove callback. +func TestHubRemoveNilPlayerOnOwner(t *testing.T) { + h := newHarness(t) + sess := &net.Session{Conn: newChanConn(), State: net.StateAccountName} + sess.StartWriter() + h.hub.Add(sess) + + done := make(chan struct{}) + go func() { + h.g.RunOnOwner(func() { h.g.Hub.Remove(sess) }) + close(done) + }() + select { + case <-done: + case <-time.After(5 * time.Second): + t.Fatal("Hub.Remove with a nil player deadlocked on the owner") + } + if sessions := h.g.Hub.AllSessions(); len(sessions) != 0 { + t.Fatalf("player-less session still in hub: %d", len(sessions)) + } +} -- cgit v1.2.3