fix: owner re-entrancy deadlock, telnet write queue, recovery/refund money safety, admin path traversal

6 files changed, 339 insertions, 23 deletions

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))
+	}
+}