fix: atomic saves, casino RNG/config isolation, tick lifecycle

1 files changed, 209 insertions, 0 deletions

diff --git a/building_guide/foundation_review.md b/building_guide/foundation_review.md
new file mode 100644
index 0000000..b0afb88
--- /dev/null
+++ b/building_guide/foundation_review.md
@@ -0,0 +1,209 @@
+# Foundation review
+
+## Verdict
+
+The package boundaries are useful foundations: casino game logic is separate
+from game presentation, multiplayer games share a small `tableGame` interface,
+and tick system order has an explicit testable contract. Keep those boundaries;
+a wholesale rewrite or a larger plugin framework is not needed yet.
+
+However, this is **not yet a safe foundation for expanding persistent casino
+activity or live world editing**. Ownership and transaction boundaries matter
+more than further extraction of command files.
+
+This review inspected session/tick entry points, account lifecycle and storage,
+casino configuration/runtime/wallet integration, shutdown, and room rename/undo.
+It is not a full audit of every combat, crafting, networking, or admin endpoint.
+
+## Fixed in this pass
+
+- **Account/character single-file saves:** replace direct truncating writes with
+  a same-directory temporary file, file sync, atomic rename, and directory sync
+  (`internal/persistence/atomic.go`, `internal/player/store.go`). Existing modes
+  are preserved; new player/account files use 0600. Account/character discovery
+  ignores non-YAML files, including leftover temporary files. This does not make
+  multi-file operations or concurrent read-modify-write cycles transactional.
+- **Save-time live mutation:** god-mode serialization uses a value snapshot
+  instead of temporarily changing the live player's skills and HP. Maps still
+  require caller ownership during serialization; this is not a deep snapshot.
+- **Casino RNG ownership:** the manager creates independent random streams for
+  each game under its own lock. Previously, different table/machine locks
+  protected the same `rand.Rand`, permitting concurrent access to its state.
+- **Slot profile isolation:** `ScaleToRTP` copies the symbols/paytables it changes,
+  so scaling a reused profile no longer retunes other machines or its source.
+- **Stale machine commands:** betting, spinning, or enabling autobet after a
+  disconnect rejects the requesting player without dereferencing a missing
+  occupant or messaging a replacement occupant.
+- **Casino configuration:** validate raw values before defaulting; reject
+  invalid game/variant/ID/range/finite-value configurations; detect room-local
+  duplicate IDs. The room create/update API checks before writes, startup
+  validation reports errors, and runtime lookup rejects invalid definitions.
+  Legacy slots table entries remain supported. See `rooms.md` for bounds.
+- **Tick lifecycle:** repeated `Stop` calls no longer close a closed channel;
+  ticker goroutines retain their own run's resources across quick restarts;
+  concurrent tick executions are serialized. `Stop` still does not wait for an
+  in-flight callback and is explicitly not a final-save barrier.
+
+## Remaining work, in recommended order
+
+### 1. High: make live game/session state single-owner
+
+Evidence: `internal/net/server.go` invokes `Game.HandleSession` from each
+connection goroutine; `internal/engine/tick.go` invokes game callbacks from the
+ticker goroutine. Disconnect callbacks also mutate players/game maps, while
+admin callbacks such as `Game.RewriteRoomIDs` mutate online players directly.
+
+`Hub.AllSessions` copies a list of pointers, not immutable session snapshots.
+Store/map locks similarly do not grant ownership over all pointed-to state.
+The casino wallet mutates player inventory and credits under a casino lock,
+which does not exclude other player commands or saves. Serializing tick
+callbacks alone does not solve these cross-entry-point races.
+
+Recommended design:
+
+1. One owner loop executes session input, connect/disconnect transitions, all
+   tick callbacks (including dynamically registered ones), and admin mutations.
+2. Network/admin goroutines submit bounded requests; they do not mutate live
+   player/session/world objects. Admin reads consume snapshots.
+3. Preserve the documented tick order. Do not wrap only `RunTickSystems` and
+   assume dynamic callbacks or instant commands are covered.
+4. Add bounded outbound queues/write deadlines before concentrating execution
+   in one owner: current session writes perform synchronous network I/O, so one
+   slow client could otherwise stall the entire world.
+5. Put saves behind an explicit owned snapshot boundary. Avoid holding Hub
+   membership locks across filesystem work; `hardRemoveLocked` currently calls
+   the removal callback while holding the Hub mutex.
+
+Acceptance: concurrent login/input/disconnect/admin-mutation integration tests
+under `-race`, plus a slow-client test proving other players keep progressing.
+
+### 2. High: transactional account lifecycle and authoritative membership
+
+The two outstanding account issues are still present:
+
+- `characterOnline` releases `charsMu` before rename/delete/purge touches files;
+  `connectCharacter` can load/register the character during that gap.
+- Account rename changes only the initiating `sess.Account.Name`. Other
+  authenticated sessions retain old account objects. Character lists and
+  pending menu selections can also become stale after rename/delete/create.
+
+This is not just a cosmetic broadcast issue. A stale character name can be
+reused by another account; cached membership must not authorize a later
+connect/rename/delete against that reassigned name.
+
+Use the owner/lifecycle boundary for lookup, canonical-name resolution,
+current membership validation, online check, mutation, and session publication.
+Refresh or invalidate all affected authenticated sessions, including pending
+menu selections. Do not simply broadcast pointer mutations from a connection
+ goroutine. Revalidate membership at operation commit, not only menu selection.
+
+Account creation, character creation, character rename/delete, and purge also
+span multiple files and contain ignored save/remove errors in
+`core_login_account.go` and `core_login_char.go`. Single-file atomic replacement
+cannot prevent orphaned characters, stale ownership lists, or partial purges.
+Choose a journaled lifecycle transaction or transactional player database.
+Stable account/character IDs would decouple identity from renameable filenames.
+
+Acceptance: simultaneous same-account operations, stale menu selections, and
+injected failure at each filesystem step must preserve identity/membership.
+
+### 3. High: durable money and graceful shutdown
+
+Evidence:
+
+- `internal/game/casino.go`: `casinoWallet.Wager` and `PayCredits` mutate balances
+  and ignore `SaveCharacter` errors. A successful wager can therefore exist only
+  in memory, and a reported win can be lost on restart.
+- `internal/casino/types.go`: `Wallet.PayCredits` cannot report failure.
+- Round/wager/free-spin state exists only in memory. A crash after a saved wager
+  debit but before settlement loses the player's stake without recovery.
+- Currency uses machine-sized `int`, unchecked additions/multiplications, and
+  floating-point payout conversion. Unlimited bets remain supported. Large
+  balances or payout multipliers can overflow; finite config validation is not
+  sufficient protection.
+- `cmd/thoi/main.go` exits on signals after requesting tick stop;
+  `internal/game/cmd_shutdown.go` calls `os.Exit` directly. Neither path drains
+  work or saves all online characters. Tick stop does not join running callbacks.
+  The countdown also reads a replaceable `g.shutdownCancel` field from its
+  goroutine; cancellation/restart needs a captured per-run channel.
+
+Recommended design:
+
+- Give each round/wager/settlement a stable transaction ID; debit plus wager
+  record, and payout plus settlement record, must be durable and idempotent.
+  A small transactional player/economy database is worth considering while
+  retaining YAML for authored world content.
+- Define checked currency arithmetic and explicit rounding/balance limits.
+  Distinguish insufficient funds from persistence failure. A failed write must
+  not silently count as successful gameplay.
+- Unify signals and the shutdown command into one shutdown request handled by
+  main. Stop admissions/admin mutations; quiesce input and autobet; drain the
+  owner and callbacks; resolve or persist outstanding wagers; save owned state;
+  then close transports and return. Surface save failures to the operator.
+- Decide server-shutdown/crash policy separately from deliberate player
+  disconnect forfeiture. Do not blindly run disconnect-forfeit callbacks for a
+  server-initiated shutdown.
+
+Acceptance: fail saves during wager/payout; restart at each debit/settlement
+boundary; test overflow boundaries; SIGTERM and scheduled shutdown must produce
+identical recoverable state, without duplicate credits or lost settlements.
+
+### 4. High for live building: complete room-ID migration and undo
+
+`handleRoomRename` in `internal/admin/api_rooms.go` rewrites referencing rooms
+and invokes player migration, but records only the renamed room in `ChangeDesc`.
+Its `NewContent` is the original bytes, not necessarily the rewritten self-loop
+content. Undo/redo therefore cannot restore all references or player state.
+The reference loop also ignores some load/write failures.
+
+`Game.RewriteRoomIDs` changes player fields but does not remap Hub room buckets
+or casino manager room keys. Live room-ID changes can therefore leave gameplay
+and broadcasts attached to old locations. Other runtime room-keyed state must
+be inventoried before declaring migration complete.
+
+`UndoStack` also ignores extra-file operation errors, so it can report success
+with only part of a multi-file change restored. Restoring file snapshots alone
+cannot update live players or runtime registries.
+
+Implement one mutation plan containing all file before/after images and explicit
+runtime mappings, executed under the owner boundary. Undo/redo must use that
+same transaction and propagate failures. A naive inverse ID map is not enough
+when the destination had pre-existing stale references. Avoid live renumbering
+until this is complete; back up player and world data together.
+
+Acceptance: rename/self-loop/external exits/triggers/offline and online player
+state, Hub membership, active casino games, undo, redo, and injected partial
+write failures all agree after execution and restart.
+
+### 5. Medium: explicit casino reconfiguration and profile contracts
+
+Manager `EnsureTable`/`EnsureMachine` returns the existing object for a room/ID.
+Changing valid room config does not replace its live rules. Deleted/renumbered
+rooms can leave cached games behind. Define whether changes take effect only
+when idle, after draining rounds, or on restart; never replace an active game
+and silently discard its wagers.
+
+Before custom slot profiles are exposed, validate their geometry and weights.
+The current implementation has a fixed 3x5 grid despite configurable profile
+fields. Public profile math/validation is not a general slot engine. Fractional
+slot credits are discarded on leaving a seat, and crash recovery does not
+preserve them. Keep these limits explicit rather than promising arbitrary
+profiles or exact realized RTP for short sessions.
+
+## Verification and limits
+
+Added regression coverage for complete-file visibility, failed-rename cleanup,
+permissions, read-only god-mode saves, file discovery, config rejection and API
+non-mutation, independent concurrent casino activity, profile isolation, stale
+machine commands, and repeated/concurrent tick lifecycle operations.
+
+Commands run successfully after the fixes:
+
+- `go test ./...`
+- `go test -race ./...`
+- `go vet ./...`
+
+The original suite also passed race mode before this work. Passing it is not
+proof of safe live state ownership: it did not exercise the competing runtime
+entry points described above. No production login/load test, process-kill
+recovery test, or filesystem power-loss simulation was performed.