diff --git a/guides/observability.md b/guides/observability.md index d3f4eea..0e5acca 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 4815f65..e126925 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([ @@ -47,7 +48,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]). @@ -94,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], @@ -333,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 469a890..e3c4ea1 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) 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 @@ -328,6 +333,46 @@ 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 -> + %% 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 + 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 9cedd10..37fef34 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, discard_unoccupied/4]). -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,123 @@ 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: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.) + +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) -> + case asobi_presence:get_status(PlayerId) of + offline -> + %% `[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 + %% plain `join/2` path, and the two rates answer different + %% questions. + asobi_telemetry:matchmaker_dropped(Mode, no_live_session), + offline; + online -> + %% `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 +%% 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. + +**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) -> + logger:notice(#{ + msg => ~"world had no live players to seat; discarding it", + mode => Mode, + world_id => WorldId + }), + asobi_telemetry:matchmaker_dropped(Mode, unoccupied), + _ = 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 @@ -621,25 +764,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 = asobi_world_server:join(WorldPid, PlayerId), - 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/src/world/asobi_world_server.erl b/src/world/asobi_world_server.erl index f5e82d7..80795af 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,8 +377,30 @@ 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 -> + %% 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) -> @@ -845,7 +898,9 @@ handle_join( max_players := Max, game_module := Mod, game_state := GS - } = State + } = State, + + KnownPid ) -> case map_size(Players) >= Max of true -> @@ -878,10 +933,35 @@ 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 -> + %% 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. 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 + %% 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} + ), ?LOG_WARNING(#{ event => join_no_live_session, world_id => WorldId, @@ -1916,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_bot_spawner_tests.erl b/test/asobi_bot_spawner_tests.erl index 410ecc2..7a58eda 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 0be6ef9..2615016 100644 --- a/test/asobi_matchmaker_isolation_tests.erl +++ b/test/asobi_matchmaker_isolation_tests.erl @@ -102,3 +102,150 @@ 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) -> + %% `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_if_session, 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", ~"m1") + ), + ?assertNot(joined(~"gone_p1")) + end). + +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, 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", ~"m1") + end), + receive + {dropped, #{mode := ~"m1", reason := no_live_session}} -> 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", ~"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} + ]}. + +%% 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. + +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() -> + {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 + {'DOWN', Ref, process, InstancePid, _} -> ok + after 5_000 -> + erlang:error(instance_not_terminated) + end + after + exit(Sup, shutdown) + end. + +occupied_world_kept() -> + {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 5a0aa7a..a52e322 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,116 @@ 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. + +%% 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.