1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138
139
140
141
142
143
144
145
146
147
148
149
150
151
152
153
154
155
156
157
158
159
160
161
162
163
164
165
166
167
168
169
170
171
172
173
174
175
176
177
178
179
180
181
182
183
184
185
186
187
188
189
190
191
192
193
194
195
196
197
198
199
200
201
202
203
204
205
206
207
208
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.
|
