fix: harden engine across casino, economy, concurrency, security, and content

1 files changed, 56 insertions, 4 deletions

diff --git a/internal/game/owner.go b/internal/game/owner.go
index 9c7be64..d4d3839 100644
--- a/internal/game/owner.go
+++ b/internal/game/owner.go
@@ -1,7 +1,9 @@
 package game
 
 import (
+	"log"
 	"runtime"
+	"runtime/debug"
 	"strconv"
 	"strings"
 	"sync"
@@ -39,6 +41,10 @@ type owner struct {
 	// detects itself — an "owner is busy" flag would wrongly let outside
 	// callers bypass the queue while a task is in flight.
 	goid atomic.Uint64
+	// panics counts the task panics the loop has contained (see runTask).
+	// A nonzero value means a command, tick callback, or disconnect handler
+	// panicked and was logged instead of killing the process.
+	panics atomic.Uint64
 }
 
 const ownerTaskQueue = 4096
@@ -57,13 +63,34 @@ func (o *owner) loop() {
 	o.goid.Store(currentGoroutineID())
 	defer close(o.done)
 	for task := range o.tasks {
-		task()
+		o.runTask(task)
 		// A queue slot freed up; wake one blocked submitter waiting for
-		// room (see enqueue).
+		// room (see enqueue). The signal MUST be sent while holding o.mu:
+		// a submitter can otherwise observe a full queue, watch this loop
+		// free a slot and signal, and only then reach cond.Wait() — the
+		// signal is lost and the submitter sleeps with capacity available.
+		o.mu.Lock()
 		o.cond.Signal()
+		o.mu.Unlock()
 	}
 }
 
+// runTask executes one queued task behind a panic barrier. A panic in a
+// command, tick callback, or disconnect handler must not kill the owner
+// goroutine: that goroutine is the only execution authority for live game
+// state, so its death takes down every session and all unsaved state with
+// it. The panic is logged with a stack trace and counted (see panics) so a
+// storm is visible, and the loop keeps draining the queue.
+func (o *owner) runTask(task func()) {
+	defer func() {
+		if r := recover(); r != nil {
+			o.panics.Add(1)
+			log.Printf("owner: recovered panic in task: %v\n%s", r, debug.Stack())
+		}
+	}()
+	task()
+}
+
 // submit enqueues without blocking. It returns false when the owner has
 // stopped or the queue is full; callers that need guaranteed delivery use
 // enqueue instead.
@@ -132,8 +159,10 @@ func (g *Game) run(fn func()) {
 	}
 	task := make(chan struct{})
 	if !o.enqueue(func() {
+		// Deferred so a panic contained by runTask still releases the
+		// caller blocked on <-task below instead of hanging it forever.
+		defer close(task)
 		fn()
-		close(task)
 	}) {
 		// The owner loop has stopped (graceful shutdown in progress).
 		// Admissions are closed: DROP the task. Executing it on the
@@ -146,10 +175,25 @@ func (g *Game) run(fn func()) {
 	<-task
 }
 
+// adminTaskPanic is the value RunAdminOnOwner re-raises on its caller's
+// goroutine when the owner loop contained a panic in an editor task. It is a
+// distinct value so a caller can tell a contained task panic from the
+// shutdown refusal that a false result means.
+const adminTaskPanic = "owner: admin task panicked; the owner loop contained it, see the log for the stack trace"
+
 // RunAdminOnOwner synchronously executes one complete editor request. Unlike
 // RunOnOwner it rejects work during shutdown and NEVER falls back to executing
 // on the HTTP goroutine when the owner has stopped. Call only from outside the
 // owner; runtime callbacks within fn must call owner-only methods directly.
+//
+// A panic inside fn is contained by the owner loop's panic barrier (runTask
+// logs and counts it and the loop keeps draining), but it is NOT reported as a
+// false result: false means "the owner refused the work", which the admin
+// middleware renders as 503 "game is shutting down" - a wrong diagnosis for a
+// task that panicked, and one that hides a real internal error. Instead the
+// panic is re-raised on the CALLER's goroutine, so the admin HTTP handler's
+// own panic recovery (admin.ownerMiddleware preserves net/http's) answers an
+// internal error while the owner loop stays untouched.
 func (g *Game) RunAdminOnOwner(fn func()) bool {
 	if g.isShuttingDown() {
 		return false
@@ -167,13 +211,21 @@ func (g *Game) RunAdminOnOwner(fn func()) bool {
 		return accepted
 	}
 	done := make(chan struct{})
+	// finished is written on the owner goroutine before close(done), so the
+	// caller reads it race-free after <-done. A deferred close(done) releases
+	// the caller whether the task completed or panicked.
+	finished := false
 	if !g.owner.enqueue(func() {
+		defer close(done)
 		task()
-		close(done)
+		finished = true
 	}) {
 		return false
 	}
 	<-done
+	if !finished {
+		panic(adminTaskPanic)
+	}
 	return accepted
 }