From ef11bb3d1ff3df4448117ad5ad0dfa0385280515 Mon Sep 17 00:00:00 2001 From: Daniel Widgren Date: Wed, 26 Aug 2026 13:57:23 +0200 Subject: [PATCH 1/4] fix(matchmaker): stop seating world players that have no session A player who disconnects between taking a ticket and their group forming was joined into the world anyway, and could then never leave. `asobi_world_server:join/2` takes no session pid and resolves one through pg. With nobody registered it seats the player as `session_pid => undefined` with no monitor, and from there: - they get no interest subscriptions, so they see nothing; - `find_player_by_pid/2` matches on `session_pid =:= Pid`, and `undefined` is never a pid, so no DOWN ever resolves to them; - `handle_leave/2` is reachable only from an explicit leave or that DOWN, so `map_size(Players)` never reaches 0, the `finished` transition is unreachable, and the `empty_grace` timer is never even scheduled. The world then ticks forever around a player who is not there. Measured during review at 105 processes and ~1.1MB leaked per occurrence, permanently: the matchmaker's default `grid_size` of 10 leaves `lazy_zones` off, so every such world pre-spawns 100 zones. Queue for a world mode, close the socket, wait for the group to form. World-side twin of widgrensit/asobi#280. Two ways in, both closed: - **A human who disconnects while queued.** `join_if_present/3` screens `asobi_presence:get_status/1` before seating. Checked in the matchmaker rather than refused in `asobi_world_server`, because a session-less join is legitimate for other callers by design (widgrensit/asobi#277 made `place_player/4` degrade rather than crash) and this is the only production path that reaches the state - the WS handler uses `join/4` with its own session pid. A player who disconnects between the check and the join still lands in it; the window is one message rather than the whole queue wait, and `join_no_live_session` telemetry now counts it. - **Every bot on a world-type mode, with no attacker at all.** `scan_groups/2` starts a bot process only for an `{asobi_match_server, _}` group, so a bot queued into a world mode never registers under `{player, BotId}` and is a guaranteed phantom. Filling was never going to work there; it just failed silently and leaked a seat per bot. `fillable/2` refuses it and says so, because a game declaring `game_type = "world"` alongside a `bots` table has no other way to discover the mistake. Also clears three pre-existing eqwalizer errors in asobi_matchmaker, since CI checks every module a PR touches: `max/2` returns `term()` so `waited/2` names the rule instead, `lists:foldl/3` erased the ticket map so `requeue_failed/2` recurses explicitly, and the ETS queue snapshot is narrowed on the way out. Not covered here, and reported separately: the matchmaker calls `asobi_world_instance_sup:start_world/1` directly, so these worlds bypass `asobi_world_lobby:check_world_capacity/1` entirely and are invisible to `global_world_count/0`. eunit 2403/0, CT 390/0, eqwalize-all 194 from 212 on main. --- src/asobi_telemetry.erl | 3 +- src/lua/bots/asobi_bot_spawner.erl | 30 +++++++- src/matches/asobi_matchmaker.erl | 85 ++++++++++++++++++++--- src/world/asobi_world_server.erl | 17 +++++ test/asobi_bot_spawner_tests.erl | 54 ++++++++++++++ test/asobi_matchmaker_isolation_tests.erl | 73 +++++++++++++++++++ 6 files changed, 252 insertions(+), 10 deletions(-) diff --git a/src/asobi_telemetry.erl b/src/asobi_telemetry.erl index 4815f654..d9f6ddd6 100644 --- a/src/asobi_telemetry.erl +++ b/src/asobi_telemetry.erl @@ -47,7 +47,8 @@ | no_effect_handler | bad_effects_return | bad_zone_busy - | bad_zone_dirty. + | bad_zone_dirty + | join_no_live_session. -export_type([game_error_kind/0]). -export([economy_transaction/4, store_purchase/3]). -export([chat_message_sent/2]). diff --git a/src/lua/bots/asobi_bot_spawner.erl b/src/lua/bots/asobi_bot_spawner.erl index 469a890a..c1a54941 100644 --- a/src/lua/bots/asobi_bot_spawner.erl +++ b/src/lua/bots/asobi_bot_spawner.erl @@ -298,7 +298,7 @@ fill_queue_with_bots() -> fill_mode(Mode, Count) when is_binary(Mode), Count > 0 -> ModeConfig = mode_config(Mode), BotConfig = maps:get(bots, ModeConfig, #{}), - case maps:get(enabled, BotConfig, false) of + case maps:get(enabled, BotConfig, false) andalso fillable(Mode, ModeConfig) of true -> MinPlayers = bot_min_players(BotConfig), %% Never fill past max_players: a match_size=2/max_players=2 @@ -328,6 +328,34 @@ fill_mode(Mode, Count) when is_binary(Mode), Count > 0 -> fill_mode(_, _) -> ok. +-doc """ +Bots are match-only, and a world mode that declares them is a config error. + +`scan_groups/2` starts a bot process only for an `{asobi_match_server, _}` pg +group, so a bot queued into a WORLD mode never gets a process and never +registers under `{player, BotId}`. `asobi_world_server` then resolves it to +`undefined`, seats it with no session and no monitor, and nothing can ever +remove it - `find_player_by_pid/2` matches on the session pid, so no `DOWN` +reaches it and `handle_leave/2` is unreachable. The world never empties. + +Filling was therefore never going to work on a world mode; it just failed +silently and leaked a seat per bot. Refused loudly instead, because a game +declaring `game_type = "world"` alongside a `bots` table has made a mistake it +otherwise has no way to discover. +""". +-spec fillable(binary(), map()) -> boolean(). +fillable(Mode, ModeConfig) -> + case maps:get(type, ModeConfig, match) of + world -> + ?LOG_WARNING(#{ + msg => ~"bots are declared on a world-type mode; not filling", + mode => Mode + }), + false; + _ -> + true + end. + clamp_fill_target(Target) when Target > ?MAX_BOT_FILL -> ?LOG_WARNING(#{ msg => ~"bot fill target exceeds ceiling, clamping", diff --git a/src/matches/asobi_matchmaker.erl b/src/matches/asobi_matchmaker.erl index 9cedd10b..3bff7e09 100644 --- a/src/matches/asobi_matchmaker.erl +++ b/src/matches/asobi_matchmaker.erl @@ -12,6 +12,7 @@ spawns a match, and pushes `match.matched` to each player. A single -export([init/1, handle_call/3, handle_cast/2, handle_info/2, terminate/2]). -ifdef(TEST). -export([next_spawn_attempt/1, join_matched_players/3, tally/1, render/3, notify_expired/1]). +-export([join_if_present/3]). -endif. -export_type([snapshot/0, mode_queue/0]). @@ -153,7 +154,7 @@ snapshot() -> Now = erlang:system_time(millisecond), try ets:lookup(?SNAPSHOT_TABLE, queue) of [{queue, SampledAt, Modes}] when is_integer(SampledAt), is_map(Modes) -> - render(SampledAt, Modes, Now); + render(SampledAt, narrow_tallies(Modes), Now); _ -> empty_snapshot() catch @@ -186,7 +187,36 @@ mode_queue(Mode, #{waiting := Waiting, oldest := Oldest, submitted_sum := Sum}, %% Clamped because system_time can step backwards; a negative wait would be %% read as a clock bug in the console rather than on the node. -spec waited(integer(), integer()) -> non_neg_integer(). -waited(At, Now) -> max(0, Now - At). +waited(At, Now) when Now > At -> Now - At; +waited(_At, _Now) -> 0. + +%% `lists:foldl/3` erases the accumulator type, so the ticket map comes back +%% `term()` and every later use of it is an untyped read. Explicit recursion +%% keeps it a map: see docs/eqwalizer-idioms.md. +-spec requeue_failed([map()], map()) -> map(). +requeue_failed([], Tickets) -> + Tickets; +requeue_failed([T | Rest], Tickets) -> + requeue_failed(Rest, Tickets#{maps:get(id, T) => T}). + +%% The queue snapshot comes back out of ETS as `term()`, and `render/3` wants +%% the tally shape. Narrow at the boundary rather than letting `dynamic()` +%% travel: an entry that is not a tally is dropped, because a malformed +%% snapshot row should cost one mode's numbers, not the whole console page. +-spec narrow_tallies(map()) -> #{binary() => mode_tally()}. +narrow_tallies(Modes) -> + maps:fold( + fun + (Mode, #{waiting := W, oldest := O, submitted_sum := S}, Acc) when + is_binary(Mode), is_integer(W), W > 0, is_integer(O), is_integer(S) + -> + Acc#{Mode => #{waiting => W, oldest => O, submitted_sum => S}}; + (_Mode, _Tally, Acc) -> + Acc + end, + #{}, + Modes + ). -spec publish(map()) -> ok. publish(Tickets) -> @@ -345,11 +375,7 @@ handle_info(tick, #{tickets := Tickets, tick_interval := Interval, max_wait := M {Matched, Expired, Remaining} = process_tickets(Tickets, Now, MaxWait), FailedGroups = spawn_matches(Matched), notify_expired(Expired), - Remaining1 = lists:foldl( - fun(T, Acc) when is_map(T), is_map(Acc) -> Acc#{maps:get(id, T) => T} end, - Remaining, - lists:flatten(FailedGroups) - ), + Remaining1 = requeue_failed(lists:flatten(FailedGroups), Remaining), erlang:send_after(Interval, self(), tick), ok = publish(Remaining1), {noreply, State#{tickets => Remaining1}}; @@ -573,6 +599,49 @@ join_matched_players(MatchPid, Mode, PlayerIds) -> notify_matchmaker_failed(PlayerIds, ~"match_start_failed") end. +-doc """ +Seat a matched player in the world, but only while they still have a session. + +`asobi_world_server:join/2` takes no session pid and resolves one through +`pg`. If the player disconnected between taking a ticket and the group being +formed, it seats them anyway with `session_pid => undefined` and no monitor - +and that player can then never leave: + +- they get no interest subscriptions, so they see nothing; +- `find_player_by_pid/2` matches on `session_pid =:= Pid`, and `undefined` is + never a pid, so no `DOWN` ever resolves to them; +- `handle_leave/2` is reachable only from an explicit leave or that `DOWN`, so + `map_size(Players)` never reaches 0 and the `empty_grace` timer is never even + scheduled. + +The world then ticks forever around a player who is not there. This is the +world-side twin of widgrensit/asobi#280, and the same root cause: nothing +checked that the ticket's owner was still connected before forming. + +Checked here rather than refused in `asobi_world_server`, because a +session-less join is legitimate for other callers by design +(widgrensit/asobi#277 made `place_player/4` degrade rather than crash), and +because this is the only production path that can reach the state: the WS +handler uses `join/4` with its own session pid. + +A player who disconnects between this check and the join still lands in it - +the window is one message rather than the whole queue wait, and +`join_no_live_session` telemetry counts it. +""". +-spec join_if_present(pid(), binary(), term()) -> ok | offline | {error, term()}. +join_if_present(WorldPid, PlayerId, WorldId) -> + case asobi_presence:get_status(PlayerId) of + offline -> + asobi_telemetry:game_error(join_no_live_session, #{ + world_id => WorldId, + player_id => PlayerId, + source => matchmaker + }), + offline; + online -> + asobi_world_server:join(WorldPid, PlayerId) + end. + %% Unlike spawn_match, world spawn is detached (spawn/1) to avoid blocking the %% matchmaker, so there is no clean handle to re-queue the group on failure. %% Worlds therefore fail fast: on the first spawn error/crash the players are @@ -623,7 +692,7 @@ spawn_world(Mode, ModeConfig, PlayerIds, Group, Rest, Failed) -> WorldId = maps:get(world_id, WorldInfo, undefined), lists:foreach( fun(PlayerId) when is_binary(PlayerId) -> - JoinResult = asobi_world_server:join(WorldPid, PlayerId), + JoinResult = join_if_present(WorldPid, PlayerId, WorldId), logger:notice(#{ msg => ~"player joined world", player_id => PlayerId, diff --git a/src/world/asobi_world_server.erl b/src/world/asobi_world_server.erl index f5e82d76..c59dbcc4 100644 --- a/src/world/asobi_world_server.erl +++ b/src/world/asobi_world_server.erl @@ -882,6 +882,23 @@ handle_join( MonRef = case PlayerPid of undefined -> + %% Counted, not just logged: a join + %% that yields zero interest + %% subscriptions is the "joined but + %% sees nothing" symptom, and an + %% operator needs a rate for it rather + %% than a line to grep. The matchmaker + %% screens for this before forming + %% (asobi_matchmaker:join_if_present/3); + %% what reaches here is the residual + %% race, so a non-zero rate is worth + %% knowing about. + asobi_telemetry:game_error( + join_no_live_session, #{ + world_id => WorldId, + player_id => PlayerId + } + ), ?LOG_WARNING(#{ event => join_no_live_session, world_id => WorldId, diff --git a/test/asobi_bot_spawner_tests.erl b/test/asobi_bot_spawner_tests.erl index 410ecc2d..7a58eda7 100644 --- a/test/asobi_bot_spawner_tests.erl +++ b/test/asobi_bot_spawner_tests.erl @@ -474,3 +474,57 @@ added_bots() -> asobi_matchmaker ) ]. + +%% widgrensit/asobi#280 (world side): scan_groups/2 starts a bot process only +%% for an {asobi_match_server, _} group, so a bot queued into a WORLD mode +%% never registers under {player, BotId}. asobi_world_server then seats it with +%% no session and no monitor, and nothing can ever remove it - the world never +%% empties. Filling a world mode was never going to work; it leaked a seat per +%% bot instead. +world_mode_is_not_filled_test() -> + Modes = #{ + ~"w" => #{type => world, bots => #{enabled => true, min_players => 4}} + }, + meck:new(asobi_game_config, [passthrough, no_link]), + meck:new(asobi_matchmaker, [passthrough, no_link]), + Self = self(), + try + meck:expect(asobi_game_config, modes, fun() -> Modes end), + meck:expect(asobi_matchmaker, add, fun(BotId, _Params) -> + Self ! {queued, BotId}, + {ok, BotId, #{}} + end), + asobi_bot_spawner:fill_mode(~"w", 1), + receive + {queued, _} -> ?assert(false) + after 100 -> ok + end + after + meck:unload(asobi_matchmaker), + meck:unload(asobi_game_config) + end. + +%% The same config on a match mode still fills, so the gate is on the mode +%% type and not on bots generally. +match_mode_is_still_filled_test() -> + Modes = #{ + ~"m" => #{type => match, max_players => 4, bots => #{enabled => true, min_players => 2}} + }, + meck:new(asobi_game_config, [passthrough, no_link]), + meck:new(asobi_matchmaker, [passthrough, no_link]), + Self = self(), + try + meck:expect(asobi_game_config, modes, fun() -> Modes end), + meck:expect(asobi_matchmaker, add, fun(BotId, _Params) -> + Self ! {queued, BotId}, + {ok, BotId, #{}} + end), + asobi_bot_spawner:fill_mode(~"m", 1), + receive + {queued, _} -> ok + after 500 -> ?assert(false) + end + after + meck:unload(asobi_matchmaker), + meck:unload(asobi_game_config) + end. diff --git a/test/asobi_matchmaker_isolation_tests.erl b/test/asobi_matchmaker_isolation_tests.erl index 0be6ef91..a72411f5 100644 --- a/test/asobi_matchmaker_isolation_tests.erl +++ b/test/asobi_matchmaker_isolation_tests.erl @@ -102,3 +102,76 @@ wait_gone(Pid, N) -> timer:sleep(5), wait_gone(Pid, N - 1) end. + +%% A player who disconnects between taking a ticket and the group forming used +%% to be seated in the world anyway, with `session_pid => undefined` and no +%% monitor - and could then never leave, because `find_player_by_pid/2` matches +%% on `session_pid =:= Pid` and `undefined` is never a pid. No DOWN, so no +%% `handle_leave/2`, so `map_size(Players)` never reached 0 and the world +%% ticked forever around someone who was not there. World-side twin of +%% widgrensit/asobi#280. +phantom_player_test_() -> + {setup, fun setup/0, fun cleanup/1, [ + {"an offline matched player is not seated", fun offline_player_not_seated/0}, + {"an offline matched player is counted", fun offline_player_is_counted/0}, + {"an online matched player is seated", fun online_player_seated/0} + ]}. + +with_world_server(Fun) -> + meck:new(asobi_world_server, [non_strict, no_link]), + Self = self(), + meck:expect(asobi_world_server, join, fun(_WorldPid, PlayerId) -> + Self ! {joined, PlayerId}, + ok + end), + try + Fun() + after + meck:unload(asobi_world_server) + end. + +joined(PlayerId) -> + receive + {joined, PlayerId} -> true + after 100 -> false + end. + +offline_player_not_seated() -> + meck:expect(asobi_presence, get_status, fun(_PlayerId) -> offline end), + with_world_server(fun() -> + ?assertEqual( + offline, + asobi_matchmaker:join_if_present(self(), ~"gone_p1", ~"w1") + ), + ?assertNot(joined(~"gone_p1")) + end). + +offline_player_is_counted() -> + meck:expect(asobi_presence, get_status, fun(_PlayerId) -> offline end), + Handler = ?FUNCTION_NAME, + Self = self(), + ok = telemetry:attach( + Handler, + [asobi, error], + fun(_E, _M, Meta, _C) -> Self ! {game_error, Meta} end, + undefined + ), + try + with_world_server(fun() -> + offline = asobi_matchmaker:join_if_present(self(), ~"gone_p2", ~"w1") + end), + receive + {game_error, #{kind := join_no_live_session, details := #{source := matchmaker}}} -> ok + after 500 -> + ?assert(false) + end + after + telemetry:detach(Handler) + end. + +online_player_seated() -> + meck:expect(asobi_presence, get_status, fun(_PlayerId) -> online end), + with_world_server(fun() -> + ?assertEqual(ok, asobi_matchmaker:join_if_present(self(), ~"live_p1", ~"w1")), + ?assert(joined(~"live_p1")) + end). From 47dc2c75c4098bdcba2275b101cb15b5939e311f Mon Sep 17 00:00:00 2001 From: Daniel Widgren Date: Wed, 26 Aug 2026 14:30:19 +0200 Subject: [PATCH 2/4] fix(matchmaker): discard a world nobody was seated in Working the architecture guardian's findings. The headline one is that the previous commit did not fix the defect it claimed to. `asobi_world_server` decides its roster is empty in exactly one place - `handle_leave/2`, reachable only from an explicit leave or a session DOWN. A world BORN with an empty roster therefore never arms `empty_grace` and never reaches `finished`; it ticks its whole zone grid forever. Screening the departed players out of the join turned one unremovable seat into zero seats, which is the same terminal state. Same ~105 processes, same permanence, same reproduction as the commit message described. `seat_group/6` counts what was actually seated and `discard_unoccupied/4` tears the instance down when nothing was, where the caller still holds the instance pid. Targeted rather than a terminus on entering `running`: every world's roster is empty before its joins arrive, so finishing there would kill all of them at startup. The residual race is unchanged and the comment no longer understates it. A player who dies between the screen and the join is still seated with no session and no monitor, and ONE seated phantom keeps its world alive exactly as surely as a whole group of them. Closing that needs a per-player terminus in `asobi_world_server`, which changes world lifecycle for every caller. Two claims in the previous commit were wrong and are corrected: - asobi#277 did not make a session-less join legitimate by design; it stopped `monitor(process, undefined)` from crashing. The actual reason not to refuse in `join/2` is that around twenty tests and the Erlang embedding path join a session-less player deliberately. - The 100 pre-spawned zones are not down to the matchmaker's `grid_size` default. `lazy_zones` is never forwarded on this path at all - the matchmaker hand-builds an 8-key config where `asobi_game_modes:world_config/1` forwards 14 more. A game declaring `lazy_zones = true` still gets the full grid here. `[asobi, matchmaker, dropped]` replaces `[asobi, error]` for the screened player. Disconnecting while queued is routine on mobile - backgrounding the app or a network handoff - and counting it as a game-code error would make the node's error rate track connection churn. The world server keeps `join_no_live_session` on `[asobi, error]` for the residual race, which is genuinely anomalous now the screen exists, and the two rates answer different questions. Also: the world-mode bot warning is rate-limited (`fill_mode/2` runs every 8s per queued mode, so it was one line every 8 seconds forever - the same shape `log_bot_not_added/3` already solved), the world-server mock in the tests is `passthrough` so it discriminates on signature drift, and `seat_group/6` is explicit recursion because `lists:foldl/3` erases the accumulator that decides whether the world lives. eunit 2405/0, CT 390/0, eqwalize-all clean on every touched module. --- guides/observability.md | 10 +- src/asobi_telemetry.erl | 21 +++- src/lua/bots/asobi_bot_spawner.erl | 20 +++- src/matches/asobi_matchmaker.erl | 121 +++++++++++++++------- test/asobi_matchmaker_isolation_tests.erl | 80 ++++++++++++-- 5 files changed, 203 insertions(+), 49 deletions(-) diff --git a/guides/observability.md b/guides/observability.md index d3f4eeaf..0e5acca3 100644 --- a/guides/observability.md +++ b/guides/observability.md @@ -213,9 +213,17 @@ asobi.match.started asobi.match.finished asobi.match.player_joined asobi.match.player_left asobi.matchmaker.queued asobi.matchmaker.removed asobi.matchmaker.deduped asobi.matchmaker.formed -asobi.matchmaker.failed +asobi.matchmaker.failed asobi.matchmaker.dropped ``` +`dropped{reason=no_live_session}` counts tickets that were matched but could +not be seated because the player had disconnected while queued. It is +deliberately NOT on `[asobi, error]`: on mobile that is the ordinary +consequence of backgrounding the app or moving between networks, and counting +it as an error would make the node's error rate track connection churn. A +rising rate means players are abandoning the queue - look at wait times, not at +the game. + Queue depth is the number worth watching, and in a cluster it is per-node - each node's matchmaker holds its own tickets, so a fleet-wide total is a sum across nodes, not a reading from one. See [Clustering](clustering.md). diff --git a/src/asobi_telemetry.erl b/src/asobi_telemetry.erl index d9f6ddd6..e1269257 100644 --- a/src/asobi_telemetry.erl +++ b/src/asobi_telemetry.erl @@ -13,7 +13,8 @@ matchmaker_deduped/2, matchmaker_removed/2, matchmaker_formed/3, - matchmaker_failed/2 + matchmaker_failed/2, + matchmaker_dropped/2 ]). -export([session_connected/1, session_disconnected/2]). -export([ @@ -95,6 +96,7 @@ events() -> [asobi, matchmaker, removed], [asobi, matchmaker, formed], [asobi, matchmaker, failed], + [asobi, matchmaker, dropped], [asobi, session, connected], [asobi, session, disconnected], [asobi, ws, connected], @@ -334,6 +336,23 @@ matchmaker_removed(PlayerId, Reason) -> player_id => PlayerId, reason => Reason }). +-doc """ +A ticket that was matched but could not be seated. + +Distinct from `[asobi, error]` on purpose: a player disconnecting while queued +is routine, not a game-code fault, and on mobile it is the ordinary consequence +of backgrounding the app or moving between networks. Counting it as an error +would make the node's error rate track connection churn. + +`mode` is label-safe on the same grounds as the other matchmaker events; +`reason` is a bounded atom. +""". +-spec matchmaker_dropped(binary(), atom()) -> ok. +matchmaker_dropped(Mode, Reason) -> + telemetry:execute([asobi, matchmaker, dropped], #{count => 1}, #{ + mode => Mode, reason => Reason + }). + -spec matchmaker_formed(binary(), pos_integer(), pos_integer()) -> ok. matchmaker_formed(Mode, PlayerCount, WaitMs) -> telemetry:execute( diff --git a/src/lua/bots/asobi_bot_spawner.erl b/src/lua/bots/asobi_bot_spawner.erl index c1a54941..87742dea 100644 --- a/src/lua/bots/asobi_bot_spawner.erl +++ b/src/lua/bots/asobi_bot_spawner.erl @@ -347,10 +347,22 @@ otherwise has no way to discover. fillable(Mode, ModeConfig) -> case maps:get(type, ModeConfig, match) of world -> - ?LOG_WARNING(#{ - msg => ~"bots are declared on a world-type mode; not filling", - mode => Mode - }), + %% Rate-limited, keyed on the mode: fill_mode/2 runs every + %% ?CHECK_INTERVAL for every mode with somebody queued, so an + %% unbounded warning here is one line every 8 seconds for as long + %% as the misconfiguration lasts. A log an operator learns to + %% filter has stopped working. Same shape as log_bot_not_added/3. + case asobi_script_log_limiter:allow({?MODULE, bots_on_world_mode, Mode}) of + {true, Dropped} -> + ?LOG_WARNING(#{ + event => bots_on_world_mode, + msg => ~"bots are declared on a world-type mode; not filling", + mode => Mode, + suppressed_since_last => Dropped + }); + false -> + ok + end, false; _ -> true diff --git a/src/matches/asobi_matchmaker.erl b/src/matches/asobi_matchmaker.erl index 3bff7e09..7f689cce 100644 --- a/src/matches/asobi_matchmaker.erl +++ b/src/matches/asobi_matchmaker.erl @@ -12,7 +12,7 @@ spawns a match, and pushes `match.matched` to each player. A single -export([init/1, handle_call/3, handle_cast/2, handle_info/2, terminate/2]). -ifdef(TEST). -export([next_spawn_attempt/1, join_matched_players/3, tally/1, render/3, notify_expired/1]). --export([join_if_present/3]). +-export([join_if_present/3, discard_unoccupied/4]). -endif. -export_type([snapshot/0, mode_queue/0]). @@ -618,30 +618,94 @@ The world then ticks forever around a player who is not there. This is the world-side twin of widgrensit/asobi#280, and the same root cause: nothing checked that the ticket's owner was still connected before forming. -Checked here rather than refused in `asobi_world_server`, because a -session-less join is legitimate for other callers by design -(widgrensit/asobi#277 made `place_player/4` degrade rather than crash), and -because this is the only production path that can reach the state: the WS -handler uses `join/4` with its own session pid. - -A player who disconnects between this check and the join still lands in it - -the window is one message rather than the whole queue wait, and -`join_no_live_session` telemetry counts it. +Checked here rather than refused in `asobi_world_server:join/2`, because +around twenty tests and the Erlang embedding path join a session-less player +deliberately - `join/2` is the API for a host that is not driving a WS session. +(Not, as an earlier draft of this comment claimed, because asobi#277 blessed +the seat: #277 only stopped `monitor(process, undefined)` from crashing. It +avoided a crash; it did not sanction the state.) + +A player who disconnects between this check and the join still lands in it. +The window is one message rather than the whole queue wait, and +`join_no_live_session` counts it - but the consequence is undiminished, because +one seated phantom keeps its world alive forever just as surely as a full group +of them. Closing that needs a per-player terminus in `asobi_world_server`. """. --spec join_if_present(pid(), binary(), term()) -> ok | offline | {error, term()}. -join_if_present(WorldPid, PlayerId, WorldId) -> +-spec join_if_present(pid(), binary(), binary()) -> ok | offline | {error, term()}. +join_if_present(WorldPid, PlayerId, Mode) -> case asobi_presence:get_status(PlayerId) of offline -> - asobi_telemetry:game_error(join_no_live_session, #{ - world_id => WorldId, - player_id => PlayerId, - source => matchmaker - }), + %% `[asobi, matchmaker, dropped]`, not `[asobi, error]`: this fires + %% on the EXPECTED path. A player disconnecting while queued is + %% routine on mobile, and putting it on the error surface would + %% make the node's error rate track connection churn. The world + %% server keeps `join_no_live_session` on `[asobi, error]` for the + %% residual race, which genuinely is anomalous now that the screen + %% exists - and the two rates answer different questions. + asobi_telemetry:matchmaker_dropped(Mode, no_live_session), offline; online -> asobi_world_server:join(WorldPid, PlayerId) end. +%% Explicit recursion: `lists:foldl/3` erases the accumulator type, and the +%% count is what decides whether the world lives. See docs/eqwalizer-idioms.md. +-spec seat_group([term()], pid(), binary(), term(), [binary()], non_neg_integer()) -> + non_neg_integer(). +seat_group([], _WorldPid, _Mode, _WorldId, _Group, Seated) -> + Seated; +seat_group([PlayerId | Rest], WorldPid, Mode, WorldId, Group, Seated) when is_binary(PlayerId) -> + JoinResult = join_if_present(WorldPid, PlayerId, Mode), + logger:notice(#{ + msg => ~"player joined world", + player_id => PlayerId, + result => JoinResult + }), + asobi_presence:send( + PlayerId, + {match_event, matched, #{match_id => WorldId, mode => Mode, player_ids => Group}} + ), + seat_group(Rest, WorldPid, Mode, WorldId, Group, seated(JoinResult, Seated)); +seat_group([_ | Rest], WorldPid, Mode, WorldId, Group, Seated) -> + seat_group(Rest, WorldPid, Mode, WorldId, Group, Seated). + +-spec seated(term(), non_neg_integer()) -> non_neg_integer(). +seated(ok, Seated) -> Seated + 1; +seated(_Other, Seated) -> Seated. + +-doc """ +Stop a world nobody was seated in. + +The world is built BEFORE anyone joins, and `asobi_world_server` decides it is +empty in exactly one place - `handle_leave/2`, reachable only from an explicit +leave or a session `DOWN`. So a world whose roster was never populated never +arms `empty_grace` and never reaches `finished`: it ticks its whole zone grid +forever. Screening the departed players out of the join (`join_if_present/3`) +turns one unremovable seat into zero seats, which is the same terminal state. + +Hence the count. If the whole group went offline while queued, the world has +no reason to exist and is torn down here, where the caller still holds the +instance pid. + +**This does not cover the residual race**: a player who dies between the screen +and the join is still seated with no session and no monitor, and one seated +player is enough to keep the world alive forever. Closing that needs a terminus +in `asobi_world_server` for a roster that empties without a leave, which is a +change to world lifecycle for every caller and is not made here. +""". +-spec discard_unoccupied(non_neg_integer(), pid(), binary(), term()) -> ok. +discard_unoccupied(0, InstancePid, Mode, WorldId) -> + logger:notice(#{ + msg => ~"world had no live players to seat; discarding it", + mode => Mode, + world_id => WorldId + }), + asobi_telemetry:matchmaker_failed(Mode, 0), + _ = supervisor:terminate_child(asobi_world_instance_sup, InstancePid), + ok; +discard_unoccupied(_Seated, _InstancePid, _Mode, _WorldId) -> + ok. + %% Unlike spawn_match, world spawn is detached (spawn/1) to avoid blocking the %% matchmaker, so there is no clean handle to re-queue the group on failure. %% Worlds therefore fail fast: on the first spawn error/crash the players are @@ -690,25 +754,10 @@ spawn_world(Mode, ModeConfig, PlayerIds, Group, Rest, Failed) -> }), WorldInfo = asobi_world_server:get_info(WorldPid), WorldId = maps:get(world_id, WorldInfo, undefined), - lists:foreach( - fun(PlayerId) when is_binary(PlayerId) -> - JoinResult = join_if_present(WorldPid, PlayerId, WorldId), - logger:notice(#{ - msg => ~"player joined world", - player_id => PlayerId, - result => JoinResult - }), - asobi_presence:send( - PlayerId, - {match_event, matched, #{ - match_id => WorldId, - mode => Mode, - player_ids => SpawnPlayerIds - }} - ) - end, - SpawnPlayerIds - ); + Seated = seat_group( + SpawnPlayerIds, WorldPid, Mode, WorldId, SpawnPlayerIds, 0 + ), + discard_unoccupied(Seated, InstancePid, Mode, WorldId); {error, Reason} -> logger:error(#{ msg => ~"world spawn failed", diff --git a/test/asobi_matchmaker_isolation_tests.erl b/test/asobi_matchmaker_isolation_tests.erl index a72411f5..58b34964 100644 --- a/test/asobi_matchmaker_isolation_tests.erl +++ b/test/asobi_matchmaker_isolation_tests.erl @@ -118,7 +118,11 @@ phantom_player_test_() -> ]}. with_world_server(Fun) -> - meck:new(asobi_world_server, [non_strict, no_link]), + %% `passthrough`, not `non_strict`: a non_strict mock accepts an expectation + %% for a function that does not exist, so a rename or an arity change would + %% break join_if_present/3 in production while these tests stayed green + %% against a stub of a function that is gone. + meck:new(asobi_world_server, [passthrough, no_link]), Self = self(), meck:expect(asobi_world_server, join, fun(_WorldPid, PlayerId) -> Self ! {joined, PlayerId}, @@ -141,7 +145,7 @@ offline_player_not_seated() -> with_world_server(fun() -> ?assertEqual( offline, - asobi_matchmaker:join_if_present(self(), ~"gone_p1", ~"w1") + asobi_matchmaker:join_if_present(self(), ~"gone_p1", ~"m1") ), ?assertNot(joined(~"gone_p1")) end). @@ -150,18 +154,21 @@ offline_player_is_counted() -> meck:expect(asobi_presence, get_status, fun(_PlayerId) -> offline end), Handler = ?FUNCTION_NAME, Self = self(), + %% `[asobi, matchmaker, dropped]`, not `[asobi, error]`: disconnecting while + %% queued is routine on mobile, and counting it as an error would make the + %% node's error rate track connection churn. ok = telemetry:attach( Handler, - [asobi, error], - fun(_E, _M, Meta, _C) -> Self ! {game_error, Meta} end, + [asobi, matchmaker, dropped], + fun(_E, _M, Meta, _C) -> Self ! {dropped, Meta} end, undefined ), try with_world_server(fun() -> - offline = asobi_matchmaker:join_if_present(self(), ~"gone_p2", ~"w1") + offline = asobi_matchmaker:join_if_present(self(), ~"gone_p2", ~"m1") end), receive - {game_error, #{kind := join_no_live_session, details := #{source := matchmaker}}} -> ok + {dropped, #{mode := ~"m1", reason := no_live_session}} -> ok after 500 -> ?assert(false) end @@ -172,6 +179,65 @@ offline_player_is_counted() -> online_player_seated() -> meck:expect(asobi_presence, get_status, fun(_PlayerId) -> online end), with_world_server(fun() -> - ?assertEqual(ok, asobi_matchmaker:join_if_present(self(), ~"live_p1", ~"w1")), + ?assertEqual(ok, asobi_matchmaker:join_if_present(self(), ~"live_p1", ~"m1")), ?assert(joined(~"live_p1")) end). + +%% The world is built BEFORE anyone joins, and asobi_world_server decides it is +%% empty in exactly one place - handle_leave/2, reachable only from an explicit +%% leave or a session DOWN. A world whose roster was never populated therefore +%% never arms empty_grace and never reaches `finished`: it ticks its whole zone +%% grid forever. Screening the departed players out of the join turns one +%% unremovable seat into zero seats, which is the same terminal state - so the +%% count has to be acted on. +unoccupied_world_test_() -> + {setup, fun setup/0, fun cleanup/1, [ + {"a world nobody was seated in is discarded", fun unoccupied_world_discarded/0}, + {"a world with someone in it is kept", fun occupied_world_kept/0} + ]}. + +with_instance_sup(Fun) -> + meck:new(supervisor, [unstick, passthrough, no_link]), + Self = self(), + try + meck:expect(supervisor, terminate_child, fun + (asobi_world_instance_sup, Pid) -> + Self ! {terminated, Pid}, + ok; + (Sup, Pid) -> + meck:passthrough([Sup, Pid]) + end), + Fun() + after + meck:unload(supervisor) + end. + +terminated(Pid) -> + receive + {terminated, Pid} -> true + after 200 -> false + end. + +unoccupied_world_discarded() -> + Instance = spawn(fun() -> + receive + stop -> ok + end + end), + with_instance_sup(fun() -> + ok = asobi_matchmaker:discard_unoccupied(0, Instance, ~"m1", ~"w1"), + ?assert(terminated(Instance)) + end), + exit(Instance, kill). + +occupied_world_kept() -> + Instance = spawn(fun() -> + receive + stop -> ok + end + end), + with_instance_sup(fun() -> + ok = asobi_matchmaker:discard_unoccupied(1, Instance, ~"m1", ~"w1"), + ?assertNot(terminated(Instance)) + end), + exit(Instance, kill). From 174f982913e2c170bde711b147d2c61b25d9ef96 Mon Sep 17 00:00:00 2001 From: Daniel Widgren Date: Wed, 26 Aug 2026 16:03:50 +0200 Subject: [PATCH 3/4] fix(matchmaker): close the join window in the world server Working the security review, whose headline is that the screen alone does not close the attack. The residual window is not "one message", as the previous commit's comment claimed. `find_player_pid/1` runs AFTER `asobi_game_join:invoke/4` and `place_player/3`, and every member of a group is joined through the same gen_statem in turn - so for player N the window is N-1 join hooks plus their own, which on a Lua world is tens to hundreds of milliseconds. Worse, `seat_group/6` sends each player's `matched` frame immediately after their join, so an attacker holding two accounts gets a per-player signal for exactly when the next join begins. And only ONE phantom is needed: a single online co-player keeps `Seated >= 1`, so `discard_unoccupied/4` never fires. `asobi_world_server:join_if_session/2` puts the pg read and the seat in the same callback, so there is no window. Refused before the game module's join and `place_player/3`, for the same reason asobi#258 rolls those back together: a late refusal would leave the game hook applied and the entity in the zone. `join/2` is unchanged and still seats a session-less player, deliberately - around twenty tests and the Erlang embedding path rely on it, and that is the real reason not to refuse there. Also from that review: - `[asobi, error]` no longer carries `player_id`. It would have been the first per-player identifier on that stream; three consumers attach to it, one exporting to the SaaS control plane, and ADR 0005 warns that a naive exporter maps `details` to labels. The WARNING beside it still has the id. - The discard reports `matchmaker_dropped(Mode, unoccupied)` rather than the error-shaped `matchmaker_failed(Mode, 0)` - the same argument this PR makes for the screened player applies to a world nobody occupied. - `maps:get(enabled, BotConfig, false) =:= true` is total. A sys.config `game_modes` entry is never shape-validated, and both a bare `andalso` and the `case` it replaced blow up on `enabled => 1`, killing this gen_server every 8 seconds and losing its `known` state each time. (The review called this a regression; it was not - origin/main case_clause'd on the same input. The fix is an improvement on both.) eunit 2408/0, CT 390/0 (serial), eqwalize-all 191, zero in every touched module. --- src/lua/bots/asobi_bot_spawner.erl | 7 ++- src/matches/asobi_matchmaker.erl | 15 ++++- src/world/asobi_world_server.erl | 62 +++++++++++++++++++-- test/asobi_matchmaker_isolation_tests.erl | 2 +- test/asobi_world_server_tests.erl | 67 ++++++++++++++++++++++- 5 files changed, 143 insertions(+), 10 deletions(-) diff --git a/src/lua/bots/asobi_bot_spawner.erl b/src/lua/bots/asobi_bot_spawner.erl index 87742dea..e3c4ea1d 100644 --- a/src/lua/bots/asobi_bot_spawner.erl +++ b/src/lua/bots/asobi_bot_spawner.erl @@ -298,7 +298,12 @@ fill_queue_with_bots() -> fill_mode(Mode, Count) when is_binary(Mode), Count > 0 -> ModeConfig = mode_config(Mode), BotConfig = maps:get(bots, ModeConfig, #{}), - case maps:get(enabled, BotConfig, false) andalso fillable(Mode, ModeConfig) of + %% `=:= true` rather than a bare `andalso`: a sys.config `game_modes` entry + %% is never shape-validated, so `bots => #{enabled => 1}` is reachable. + %% `andalso` raises badarg on it and the old `case` case_clause'd - either + %% way this gen_server dies every ?CHECK_INTERVAL and silently loses its + %% `known` state. Total is the right shape for operator-supplied config. + case maps:get(enabled, BotConfig, false) =:= true andalso fillable(Mode, ModeConfig) of true -> MinPlayers = bot_min_players(BotConfig), %% Never fill past max_players: a match_size=2/max_players=2 diff --git a/src/matches/asobi_matchmaker.erl b/src/matches/asobi_matchmaker.erl index 7f689cce..62c223ef 100644 --- a/src/matches/asobi_matchmaker.erl +++ b/src/matches/asobi_matchmaker.erl @@ -645,7 +645,18 @@ join_if_present(WorldPid, PlayerId, Mode) -> asobi_telemetry:matchmaker_dropped(Mode, no_live_session), offline; online -> - asobi_world_server:join(WorldPid, PlayerId) + %% `require_session` and not `join/2`: the screen above and the + %% seat are two separate pg reads, and everything between them - + %% the game's join hook, place_player/3, every other member's join + %% in this same gen_statem - is inside the window. The world server + %% re-checks in the callback that seats, so there is none. + case asobi_world_server:join_if_session(WorldPid, PlayerId) of + {error, no_live_session} -> + asobi_telemetry:matchmaker_dropped(Mode, no_live_session), + offline; + Other -> + Other + end end. %% Explicit recursion: `lists:foldl/3` erases the accumulator type, and the @@ -700,7 +711,7 @@ discard_unoccupied(0, InstancePid, Mode, WorldId) -> mode => Mode, world_id => WorldId }), - asobi_telemetry:matchmaker_failed(Mode, 0), + asobi_telemetry:matchmaker_dropped(Mode, unoccupied), _ = supervisor:terminate_child(asobi_world_instance_sup, InstancePid), ok; discard_unoccupied(_Seated, _InstancePid, _Mode, _WorldId) -> diff --git a/src/world/asobi_world_server.erl b/src/world/asobi_world_server.erl index c59dbcc4..c3461a0d 100644 --- a/src/world/asobi_world_server.erl +++ b/src/world/asobi_world_server.erl @@ -13,6 +13,7 @@ transient matches use `asobi_match_server` instead. -export([ start_link/1, join/2, + join_if_session/2, join/3, join/4, leave/2, @@ -89,6 +90,36 @@ join(Pid, PlayerId) -> {error, _} = Err -> Err end. +-doc """ +As `join/2`, but refuse a player who has no live session. + +`join/2` resolves the session through `pg` and seats the player either way, +which is deliberate: the Erlang embedding path and around twenty tests join a +session-less player on purpose. But a player seated that way carries +`session_pid => undefined` and no monitor, so `find_player_by_pid/2` - which +matches on the session pid - can never resolve a `DOWN` to them, +`handle_leave/2` is unreachable, and the roster never empties. The world ticks +forever around someone who is not there. + +A caller that screens beforehand cannot close that on its own. The screen and +the seat are two separate `pg` reads with the game's `join` hook, +`place_player/3` and every other member's join in between - on a Lua world that +is tens to hundreds of milliseconds, and the per-player `matched` frames the +matchmaker sends tell an observer exactly when the next join begins. Checking +HERE puts the read and the seat in the same callback, so there is no window at +all. + +Refused before `asobi_game_join:invoke/4` and `place_player/3` for the same +reason asobi#258 rolls those back together: a late refusal would leave the +game module's join applied and the entity in the zone. +""". +-spec join_if_session(pid(), binary()) -> ok | {error, term()}. +join_if_session(Pid, PlayerId) -> + case gen_statem:call(Pid, {join, PlayerId, #{}, require_session}) of + {ok, _ZonePid} -> ok; + {error, _} = Err -> Err + end. + %% Variant of join/2 that also synchronously binds zone_pid into the caller's %% asobi_player_session via SessionPid before returning. Use this from the WS %% handler so an immediately-following world.input isn't dropped while the async @@ -346,6 +377,21 @@ running( asobi_world_ticker:set_zone_manager(TickerPid, ZoneManagerPid, self()), asobi_telemetry:world_started(WorldId, maps:get(mode, State, undefined)), keep_state_and_data; +running({call, From}, {join, PlayerId, Ctx, require_session}, #{world_id := WorldId} = State) when + is_binary(PlayerId) +-> + case find_player_pid(PlayerId) of + undefined -> + asobi_telemetry:game_error(join_no_live_session, #{world_id => WorldId}), + ?LOG_WARNING(#{ + event => join_refused_no_live_session, + world_id => WorldId, + player_id => PlayerId + }), + {keep_state_and_data, [{reply, From, {error, no_live_session}}]}; + _PlayerPid -> + handle_join(From, PlayerId, Ctx, State) + end; running({call, From}, {join, PlayerId, Ctx}, State) -> handle_join(From, PlayerId, Ctx, State); running(cast, {leave, PlayerId}, State) -> @@ -893,11 +939,19 @@ handle_join( %% what reaches here is the residual %% race, so a non-zero rate is worth %% knowing about. + %% No `player_id` in the details. Every + %% other game_error/2 site carries only + %% module/world/coords, three consumers + %% attach to this stream (one exports + %% to the SaaS control plane), and ADR + %% 0005 warns that a naive exporter + %% maps details to labels - which would + %% be per-player cardinality and a + %% pseudonymous id leaving the game + %% environment. The WARNING below has + %% the id for the operator. asobi_telemetry:game_error( - join_no_live_session, #{ - world_id => WorldId, - player_id => PlayerId - } + join_no_live_session, #{world_id => WorldId} ), ?LOG_WARNING(#{ event => join_no_live_session, diff --git a/test/asobi_matchmaker_isolation_tests.erl b/test/asobi_matchmaker_isolation_tests.erl index 58b34964..00ad2d54 100644 --- a/test/asobi_matchmaker_isolation_tests.erl +++ b/test/asobi_matchmaker_isolation_tests.erl @@ -124,7 +124,7 @@ with_world_server(Fun) -> %% against a stub of a function that is gone. meck:new(asobi_world_server, [passthrough, no_link]), Self = self(), - meck:expect(asobi_world_server, join, fun(_WorldPid, PlayerId) -> + meck:expect(asobi_world_server, join_if_session, fun(_WorldPid, PlayerId) -> Self ! {joined, PlayerId}, ok end), diff --git a/test/asobi_world_server_tests.erl b/test/asobi_world_server_tests.erl index 5a0aa7a2..edc27647 100644 --- a/test/asobi_world_server_tests.erl +++ b/test/asobi_world_server_tests.erl @@ -787,7 +787,7 @@ persistent_reaches_the_zone_config_test() -> #{instance_pid := InstancePid} = start_world(#{persistent => true}), try ZoneManager = asobi_world_instance:get_child(InstancePid, asobi_zone_manager), - #{zone_config := ZoneConfig} = sys:get_state(ZoneManager), + ZoneConfig = zone_config(ZoneManager), ?assertEqual(true, maps:get(persistence, ZoneConfig)) after exit(InstancePid, shutdown) @@ -802,7 +802,7 @@ a_world_that_did_not_ask_is_not_persistent_test() -> #{instance_pid := InstancePid} = start_world(), try ZoneManager = asobi_world_instance:get_child(InstancePid, asobi_zone_manager), - #{zone_config := ZoneConfig} = sys:get_state(ZoneManager), + ZoneConfig = zone_config(ZoneManager), ?assertEqual(false, maps:get(persistence, ZoneConfig)) after exit(InstancePid, shutdown) @@ -810,3 +810,66 @@ a_world_that_did_not_ask_is_not_persistent_test() -> after cleanup(ok) end. + +%% widgrensit/asobi#569 security review: a caller that screens for a live +%% session before calling join/2 cannot close the window on its own - the +%% screen and the seat are two separate pg reads with the game's join hook, +%% place_player/3 and every other member's join in between. join/3 puts the +%% read and the seat in the same callback. +join_require_session_refuses_without_one_test() -> + setup(), + try + refuses_without_session() + after + cleanup(ok) + end. + +refuses_without_session() -> + Ctx = #{world_pid := Pid} = start_world(), + try + PlayerId = ~"require_session_absent", + %% Only meaningful while nobody holds this id in the shared scope. + [] = pg:get_members(nova_scope, {player, PlayerId}), + ?assertEqual( + {error, no_live_session}, + asobi_world_server:join_if_session(Pid, PlayerId) + ), + %% Refused before the game module's join and place_player/3, so nothing + %% is left half-applied. + ?assertEqual(0, maps:get(player_count, asobi_world_server:get_info(Pid))) + after + stop_world(Ctx) + end. + +join_require_session_seats_with_one_test() -> + setup(), + try + seats_with_session() + after + cleanup(ok) + end. + +seats_with_session() -> + Ctx = #{world_pid := Pid} = start_world(), + try + PlayerId = ~"require_session_present", + SessionPid = fake_session(PlayerId), + try + ?assertEqual(ok, asobi_world_server:join_if_session(Pid, PlayerId)), + ?assertEqual(1, maps:get(player_count, asobi_world_server:get_info(Pid))) + after + exit(SessionPid, kill) + end + after + stop_world(Ctx) + end. + +%% sys:get_state/1 is term(); narrow before indexing rather than reading a +%% field out of an unknown shape. `pid() | undefined` because the caller hands +%% this `asobi_world_instance:get_child/2`, which can answer either - the guard +%% then makes `undefined` a loud function_clause. +-spec zone_config(pid() | undefined) -> map(). +zone_config(ZoneManager) when is_pid(ZoneManager) -> + case sys:get_state(ZoneManager) of + #{zone_config := Config} when is_map(Config) -> Config + end. From 2d7c7b06b12de03878082a3017a03ab2d769d4b7 Mon Sep 17 00:00:00 2001 From: Daniel Widgren Date: Wed, 26 Aug 2026 16:35:55 +0200 Subject: [PATCH 4/4] fix(matchmaker): monitor the session the join actually checked Working the code review. Its blocker is that the previous commit's central claim was false: `join_if_session/2` did not close the window. The `require_session` clause resolved the session and then DISCARDED it, and `handle_join` resolved it again to pick its monitor - with the game module's join hook and `place_player/3` in between. A session dying inside that hook therefore produced the exact unremovable seat the clause exists to refuse, through the function documented as closing the window. The reduction was real (the other members' joins left the window) but it was a factor of 1/N, not closure. The resolved pid is now carried into `handle_join/5` and monitored. That makes the case self-healing rather than permanent: a monitor on an already-dead pid delivers `DOWN` immediately, `find_player_by_pid/2` resolves it, and `handle_leave/2` runs. Pinned by a test that kills the session INSIDE the join hook; it reports `expected: 0, got: 1` with the fix reverted. `discard_unoccupied/4`'s tests no longer meck OTP's `supervisor` VM-wide. That mock asserted only that `terminate_child/2` was CALLED - it stayed green if the supervisor atom were wrong, if the supervisor were not running, or if the instance survived, which is every way this can actually break. They run against a real `simple_one_for_one` now and wait for the instance's `DOWN`. Doc corrections, all of them claims this branch made and the code does not support: - `join_if_present/3` still carried the "the window is one message" paragraph that the previous commit's own message rejected, eighteen lines above the comment rejecting it. The screen buys one thing - keeping the ordinary case off `[asobi, error]` - and that is now all it says. - `discard_unoccupied/4` claimed the residual race was a player dying between the screen and the join. That mechanism is gone; what remains is that the count covers this group only, while the world is discoverable and quick-play eligible from the moment it starts. - The `join_no_live_session` comment said the matchmaker screens before this point. False for the plain `join/2` path, which is where it is actually reached from. - **A correction to a correction.** The previous commit said the 100 pre-spawned zones were "not down to the matchmaker's `grid_size` default". That was wrong: `asobi_world_server` defaults `lazy_zones` to `GridSize > 100`, so `grid_size => 10` IS what leaves it off. The original diagnosis was right; the new information is only that an explicit `lazy_zones = true` cannot help either, because the matchmaker never forwards it. eunit 2408/0; eqwalize-all 192, zero in every touched module; dialyzer and xref clean. CT is 389/390 on this branch, failing only `asobi_vote_SUITE`'s pre-existing flake, which is fixed separately in #570 and is unrelated to this change. --- src/matches/asobi_matchmaker.erl | 23 +++---- src/world/asobi_world_server.erl | 38 ++++++++--- test/asobi_matchmaker_isolation_tests.erl | 82 +++++++++++++---------- test/asobi_world_server_tests.erl | 50 ++++++++++++++ 4 files changed, 133 insertions(+), 60 deletions(-) diff --git a/src/matches/asobi_matchmaker.erl b/src/matches/asobi_matchmaker.erl index 62c223ef..37fef34f 100644 --- a/src/matches/asobi_matchmaker.erl +++ b/src/matches/asobi_matchmaker.erl @@ -625,11 +625,10 @@ deliberately - `join/2` is the API for a host that is not driving a WS session. the seat: #277 only stopped `monitor(process, undefined)` from crashing. It avoided a crash; it did not sanction the state.) -A player who disconnects between this check and the join still lands in it. -The window is one message rather than the whole queue wait, and -`join_no_live_session` counts it - but the consequence is undiminished, because -one seated phantom keeps its world alive forever just as surely as a full group -of them. Closing that needs a per-player terminus in `asobi_world_server`. +This screen buys exactly one thing now that `join_if_session/2` re-checks in +the callback that seats: it keeps the ORDINARY case - a player who left the +queue - off `[asobi, error]`, where it would make the node's error rate track +connection churn. It is not what closes the window. """. -spec join_if_present(pid(), binary(), binary()) -> ok | offline | {error, term()}. join_if_present(WorldPid, PlayerId, Mode) -> @@ -640,8 +639,8 @@ join_if_present(WorldPid, PlayerId, Mode) -> %% routine on mobile, and putting it on the error surface would %% make the node's error rate track connection churn. The world %% server keeps `join_no_live_session` on `[asobi, error]` for the - %% residual race, which genuinely is anomalous now that the screen - %% exists - and the two rates answer different questions. + %% plain `join/2` path, and the two rates answer different + %% questions. asobi_telemetry:matchmaker_dropped(Mode, no_live_session), offline; online -> @@ -698,11 +697,11 @@ Hence the count. If the whole group went offline while queued, the world has no reason to exist and is torn down here, where the caller still holds the instance pid. -**This does not cover the residual race**: a player who dies between the screen -and the join is still seated with no session and no monitor, and one seated -player is enough to keep the world alive forever. Closing that needs a terminus -in `asobi_world_server` for a roster that empties without a leave, which is a -change to world lifecycle for every caller and is not made here. +**It counts this group only.** The world is discoverable and quick-play +eligible from the moment it starts (`spawn_world/6` sets neither `listed` nor +`quick_play`, and both default to true), so an unrelated player can join it +while the seat loop is still running and is not counted here. Gating on the +world's own roster rather than a local tally is the fix; it is not made here. """. -spec discard_unoccupied(non_neg_integer(), pid(), binary(), term()) -> ok. discard_unoccupied(0, InstancePid, Mode, WorldId) -> diff --git a/src/world/asobi_world_server.erl b/src/world/asobi_world_server.erl index c3461a0d..80795af5 100644 --- a/src/world/asobi_world_server.erl +++ b/src/world/asobi_world_server.erl @@ -389,11 +389,18 @@ running({call, From}, {join, PlayerId, Ctx, require_session}, #{world_id := Worl player_id => PlayerId }), {keep_state_and_data, [{reply, From, {error, no_live_session}}]}; - _PlayerPid -> - handle_join(From, PlayerId, Ctx, State) + PlayerPid -> + %% Carried, not re-resolved. `handle_join/5` used to read the group + %% again to pick its monitor, with the game module's join hook and + %% `place_player/3` in between - so a session dying inside that hook + %% produced the very seat this clause exists to refuse. Monitoring + %% the pid we already checked makes the failure self-healing + %% instead: a monitor on a dead pid delivers `DOWN` immediately, + %% `find_player_by_pid/2` resolves it, and `handle_leave/2` runs. + handle_join(From, PlayerId, Ctx, State, PlayerPid) end; running({call, From}, {join, PlayerId, Ctx}, State) -> - handle_join(From, PlayerId, Ctx, State); + handle_join(From, PlayerId, Ctx, State, resolve); running(cast, {leave, PlayerId}, State) -> handle_leave(PlayerId, State); running(cast, {move_player, PlayerId, NewPos, Entity}, State) -> @@ -891,7 +898,9 @@ handle_join( max_players := Max, game_module := Mod, game_state := GS - } = State + } = State, + + KnownPid ) -> case map_size(Players) >= Max of true -> @@ -924,7 +933,7 @@ handle_join( %% (its subscribe becomes a no-op); do the same %% here rather than crashing on %% monitor(process, undefined). - PlayerPid = find_player_pid(PlayerId), + PlayerPid = known_or_resolved(KnownPid, PlayerId), MonRef = case PlayerPid of undefined -> @@ -933,12 +942,12 @@ handle_join( %% subscriptions is the "joined but %% sees nothing" symptom, and an %% operator needs a rate for it rather - %% than a line to grep. The matchmaker - %% screens for this before forming - %% (asobi_matchmaker:join_if_present/3); - %% what reaches here is the residual - %% race, so a non-zero rate is worth - %% knowing about. + %% than a line to grep. Reached from + %% the plain `join/2`, which seats a + %% session-less player deliberately; + %% `join_if_session/2` refuses before + %% this point instead. + %% %% No `player_id` in the details. Every %% other game_error/2 site carries only %% module/world/coords, three consumers @@ -1987,3 +1996,10 @@ handle_phase_events([{phase_ended, Name} | Rest], Mod, GS) -> handle_phase_events(Rest, Mod, GS1); handle_phase_events([_Event | Rest], Mod, GS) -> handle_phase_events(Rest, Mod, GS). + +%% `resolve` means "look it up now" - the ordinary `join/2` path. A pid means +%% the caller already resolved it and is entitled to have that one monitored, +%% which is what makes `join_if_session/2` free of a second read. +-spec known_or_resolved(pid() | resolve, binary()) -> pid() | undefined. +known_or_resolved(resolve, PlayerId) -> find_player_pid(PlayerId); +known_or_resolved(KnownPid, _PlayerId) when is_pid(KnownPid) -> KnownPid. diff --git a/test/asobi_matchmaker_isolation_tests.erl b/test/asobi_matchmaker_isolation_tests.erl index 00ad2d54..26150169 100644 --- a/test/asobi_matchmaker_isolation_tests.erl +++ b/test/asobi_matchmaker_isolation_tests.erl @@ -196,48 +196,56 @@ unoccupied_world_test_() -> {"a world with someone in it is kept", fun occupied_world_kept/0} ]}. -with_instance_sup(Fun) -> - meck:new(supervisor, [unstick, passthrough, no_link]), - Self = self(), - try - meck:expect(supervisor, terminate_child, fun - (asobi_world_instance_sup, Pid) -> - Self ! {terminated, Pid}, - ok; - (Sup, Pid) -> - meck:passthrough([Sup, Pid]) - end), - Fun() - after - meck:unload(supervisor) +%% A real `simple_one_for_one` supervisor, not a VM-wide meck of OTP's +%% `supervisor`. The mocked version asserted only that `terminate_child/2` was +%% CALLED - it stayed green if the sup atom were wrong, if the sup were not +%% running, or if the instance survived, which is every way this can actually +%% break. `meck:unload(supervisor)` also hard-purges a kernel module the whole +%% eunit VM is using. +ensure_pg() -> + case whereis(nova_scope) of + undefined -> pg:start_link(nova_scope); + _ -> ok end. -terminated(Pid) -> - receive - {terminated, Pid} -> true - after 200 -> false - end. +start_instance(Sup) -> + ensure_pg(), + {ok, InstancePid} = supervisor:start_child(Sup, [ + #{ + game_module => asobi_test_world_game, + grid_size => 1, + zone_size => 100, + tick_rate => 50, + max_players => 4, + view_radius => 1 + } + ]), + InstancePid. unoccupied_world_discarded() -> - Instance = spawn(fun() -> + {ok, Sup} = asobi_world_instance_sup:start_link(), + unlink(Sup), + try + InstancePid = start_instance(Sup), + Ref = monitor(process, InstancePid), + ok = asobi_matchmaker:discard_unoccupied(0, InstancePid, ~"m1", ~"w1"), receive - stop -> ok + {'DOWN', Ref, process, InstancePid, _} -> ok + after 5_000 -> + erlang:error(instance_not_terminated) end - end), - with_instance_sup(fun() -> - ok = asobi_matchmaker:discard_unoccupied(0, Instance, ~"m1", ~"w1"), - ?assert(terminated(Instance)) - end), - exit(Instance, kill). + after + exit(Sup, shutdown) + end. occupied_world_kept() -> - Instance = spawn(fun() -> - receive - stop -> ok - end - end), - with_instance_sup(fun() -> - ok = asobi_matchmaker:discard_unoccupied(1, Instance, ~"m1", ~"w1"), - ?assertNot(terminated(Instance)) - end), - exit(Instance, kill). + {ok, Sup} = asobi_world_instance_sup:start_link(), + unlink(Sup), + try + InstancePid = start_instance(Sup), + ok = asobi_matchmaker:discard_unoccupied(1, InstancePid, ~"m1", ~"w1"), + timer:sleep(200), + ?assert(is_process_alive(InstancePid)) + after + exit(Sup, shutdown) + end. diff --git a/test/asobi_world_server_tests.erl b/test/asobi_world_server_tests.erl index edc27647..a52e3223 100644 --- a/test/asobi_world_server_tests.erl +++ b/test/asobi_world_server_tests.erl @@ -873,3 +873,53 @@ zone_config(ZoneManager) when is_pid(ZoneManager) -> case sys:get_state(ZoneManager) of #{zone_config := Config} when is_map(Config) -> Config end. + +%% widgrensit/asobi#569 code review: `join_if_session/2` resolved the session, +%% then `handle_join/5` resolved it AGAIN to pick its monitor - with the game +%% module's join hook and place_player/3 in between. A session dying inside +%% that hook therefore produced the exact unremovable seat the clause exists to +%% refuse, through the function documented as closing the window. +%% +%% Monitoring the pid already checked makes it self-healing: a monitor on a +%% dead pid delivers DOWN immediately, so the seat does not outlive the session +%% it was granted for. +session_dying_inside_the_join_hook_does_not_strand_a_seat_test() -> + setup(), + try + dies_in_hook() + after + cleanup(ok) + end. + +dies_in_hook() -> + Ctx = #{world_pid := Pid} = start_world(), + try + PlayerId = ~"dies_in_hook", + SessionPid = fake_session(PlayerId), + meck:new(asobi_game_join, [passthrough, no_link]), + try + meck:expect(asobi_game_join, invoke, fun(Mod, P, C, GS) -> + exit(SessionPid, kill), + wait_until_gone(PlayerId, 50), + meck:passthrough([Mod, P, C, GS]) + end), + ?assertEqual(ok, asobi_world_server:join_if_session(Pid, PlayerId)) + after + meck:unload(asobi_game_join) + end, + timer:sleep(150), + ?assertEqual(0, maps:get(player_count, asobi_world_server:get_info(Pid))) + after + stop_world(Ctx) + end. + +wait_until_gone(_PlayerId, 0) -> + ok; +wait_until_gone(PlayerId, N) -> + case pg:get_members(nova_scope, {player, PlayerId}) of + [] -> + ok; + _ -> + timer:sleep(5), + wait_until_gone(PlayerId, N - 1) + end.