Conversation
4b052c3 to
d8505e6
Compare
sven-n
left a comment
sven-n
left a comment
There was a problem hiding this comment.
Review
What it does: Adds BotNavigator.HasWalkableSpawnGate(map), a statically-cached terrain check, and uses it as an extra filter in TryPickEasierMap and TryPickBetterMapCore so a bot never selects a map whose safezone spawn gate has no walkable tile — avoiding the ClientReadyAfterMapChangeAsync → WarpToSafezoneAsync → OfflineMapChangePlugIn.MapChangeAsync → ClientReadyAfterMapChangeAsync recursion.
The root-cause analysis in the description is accurate — the cycle is verifiable in Player.cs:1185-1187, Player.cs:1106 and Offline/OfflineMapChangePlugIn.cs:29.
Correctness
-
The check inspects the wrong map (main issue). The recovery path is
WarpToSafezoneAsync→GetSpawnGateOfCurrentMapAsync, which resolvesCurrentMap.Definition.SafezoneMap ?? CurrentMap.Definition(Player.cs:2309). Many maps (dungeons, event maps) have a differentSafezoneMap. The PR checksmap.GetSafezoneGate(...)on the destination map itself, so it can both reject a perfectly safe map and — more importantly — fail to reject the actually-crashing case where the destination'sSafezoneMaptarget is the broken one. It should mirror the runtime resolution:var safezoneMap = map.SafezoneMap ?? map; var terrain = new GameMapTerrain(safezoneMap); var spawnGate = safezoneMap.GetSafezoneGate(terrain);
and the cache key must then be that resolved map, not
map.Number. -
The crash is still reachable through unfiltered paths. Candidate selection is only one way a bot changes maps. The escape/home-town gate path in this same file (
BotNavigator.cs:1492,GetSafezoneGate()), death respawn (Player.cs:2064), walking into a portal/exit gate, mini-games and GM/console warps all bypass this filter and still recurse to overflow — a server-killing crash, not a bot glitch. I'd recommend also adding the cheap defensive guard at the recursion site, e.g. inClientReadyAfterMapChangeAsyncskip theWarpToSafezoneAsyncrecovery when we are already at the resolved safezone gate (or gate it behind a re-entrancy/depth flag on the player). That turns a process crash into a stuck bot on every path, and the navigator filter then remains a nice quality improvement on top. -
Cache keying / lifetime.
ConcurrentDictionary<int, bool>keyed onmap.Numberis static for the process lifetime. OpenMU can host several game servers and the admin panel can reload/edit configuration; twoGameConfigurations with the same mapNumberbut differentTerrainDatawill collide, and edited terrain is never re-evaluated. Keying onmap.GetId()fixes collisions; a note in the remarks that config edits need a restart would cover staleness.
Quality / style
GameMapTerrainallocates twobool[256,256], abyte[256,256]and a spawn-point array per construction. Cached, so amortized fine — and when the map is already loaded,GameContext.GetMapAsync(...)exposesTerrainandSafeZoneSpawnGatedirectly (GameMap.cs:52). Not worth forcing a load for an unloaded map, so the current approach is defensible; worth a comment.for (var x = (int)spawnGate.X1; ...)is safe — gate coordinates are bytes, soWalkMap[256, …]can't be hit.- The
ponytail:prefix in the<remarks>block appears nowhere else in the codebase — looks like a stray internal marker. Please drop it. - Doc comments are otherwise clear and explain the why well.
Tests
No tests added. BotNavigator is internal but reachable from MUnique.OpenMU.Tests; a focused test that builds a GameMapDefinition with a spawn gate over blocked terrain and asserts the map is not selected would be valuable, especially for the resolved-safezone-map logic above.
Security / performance
- No security surface. No per-tick terrain parsing; the filter is O(gate area) once per map.
GetOrAdd's factory may run concurrently for the same key — pure and idempotent here, so only a redundant parse.
Summary
Sound diagnosis and a cheap, well-documented mitigation, but two items should be addressed before merge: (1) resolve SafezoneMap before checking the gate — as written the check can miss the exact crash it targets; (2) add a guard at the recursion site so non-navigator warp paths can't still take down the server. Cache key and the ponytail: comment are minor cleanups.
Generated by Claude Code
A player placed on a non-walkable tile is recovered by WarpToSafezoneAsync, which re-enters ClientReadyAfterMapChangeAsync. A player with a game client re-enters later and on a fresh stack, with its F3 12 packet - but a connection-less player (an OfflinePlayer, i.e. a bot) gets it inline from OfflineMapChangePlugIn. Since PlaceAtGateAsync rolls the position within the gate once and never retries, a safezone spawn gate without a single walkable tile recurses until the stack overflows, taking the game server process down. The repeated-packet guard at the top of ClientReadyAfterMapChangeAsync does not stop this: WarpToAsync sets CurrentMap to null before raising the map change, so the nested call walks straight past it. PlayerMapTransitions.RecoverFromBlockedSpawnAsync now warps at most once per re-entry: a nested attempt places the player on a walkable tile of the map it already stands on. That bounds the recursion on every path into it - the bot navigator, escape and home gates, death respawn, portals, mini-games and GM warps alike. It returns whether it warped, so the outer client-ready frame keeps its existing behaviour of not adding the summon a second time. A player with a game client re-enters on a later, fresh stack with the flag already reset, so it is not bounded by this - it can still be warped back and forth between two blocked gates, exactly as before, which is a stuck client rather than a dead server. The tile it falls back to comes from the new GameMapTerrain.GetAnyWalkableCoordinate, which prefers the safezone. RandomWalkableCoordinate would have been wrong here: it samples the monster spawn points, which exclude every safezone tile by construction, so it would drop a player who is being recovered into a hunting ground - and return nothing at all on a map which is only safezone, stranding the player on the very tile it was meant to rescue it from. On top of that, the bot navigator no longer offers a bot a map it cannot stand in, so it does not pick one and get bounced straight back out. This is the idea of MUnique#844, with two corrections: the check resolves SafezoneMap first, mirroring GetSpawnGateOfCurrentMapAsync, because dungeons and event maps (Icarus, Karutan 2, the Chaos Castles, ...) are recovered to a different map than the one being entered; and the cache is keyed by the resolved map's id rather than its number, which is not unique - the Devil Squares all share number 9. The verdict is not cached when the map has no id of its own, because GetId falls back to Guid.Empty and every such map would share one entry. The gate scan lives on GameMapTerrain, shared by both. Its loop counters are ints on purpose: a gate reaching coordinate 255 would make a byte counter wrap around and loop forever. No map in the seeded Season 6 configuration has a blocked safezone gate today (the tightest is Doppelgaenger 4 at 39 of 64 tiles), so this is reachable only through custom maps or edited terrain. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
2 participants
Problem
A bot can crash the whole game server with a stack overflow when it warps to a map whose spawn gate places it on a
blocked (non-walkable) tile.
Root cause
Bots are connection-less (BotPlayer : OfflinePlayer). When a warp lands a player on a blocked tile,
ClientReadyAfterMapChangeAsync recovers by calling WarpToSafezoneAsync:
ClientReadyAfterMapChangeAsync
→ WarpToSafezoneAsync
→ WarpToAsync
→ IMapChangePlugIn.MapChangeAsync
→ (offline bot) OfflineMapChangePlugIn.MapChangeAsync
→ ClientReadyAfterMapChangeAsync // synchronous, inline — no network round-trip
For a real player the map-change plugin sends a packet and returns; the next ClientReadyAfterMapChangeAsync arrives
later on a fresh stack from the client's F3 12 ack — no recursion.
For a bot, OfflineMapChangePlugIn.MapChangeAsync calls ClientReadyAfterMapChangeAsync inline. If the destination's
safezone spawn gate is itself blocked, the recovery re-enters on the same growing stack and recurses until it
overflows — crashing the process.
A reactive "remember broken maps, skip them next tick" fix cannot work here: the overflow kills the process on the
first bad warp, before the bot's next navigator tick ever runs.
Fix (proactive, bot-side only)
A bot is never offered a map it cannot stand in, so the recovery recursion can never start.
BotNavigator.HasWalkableSpawnGate(map) parses the destination map's terrain and checks that its safezone spawn gate
(the gate WarpToSafezoneAsync recovers to) contains at least one walkable tile. The two candidate pickers —
TryPickEasierMap and TryPickBetterMapCore — now drop any map that fails this check, alongside the existing legal-warp
and affordability filters:
if (!candidate.TryGetRequirementError(this._player, out _)
&& this.TryGetLegalWarp(candidate, out var candidateWarp)
&& this.CanAffordWarp(candidateWarp)
&& this.HasWalkableSpawnGate(candidate)) // new
Terrain is static configuration, so the verdict is parsed once per map and cached in a static
ConcurrentDictionary<int, bool> shared across all bots — no per-tick terrain parsing, no per-bot duplication.