Repository navigation
Conversation
🟡 Code Coverage — 76.8%9424 of 12266 lines covered. 🟡 ELP Lint — 192 warnings212 diagnostics found. See job logs for details. ✅ ELP eqWAlizeNo diagnostics. |
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 #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 (#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.
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.
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.
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.
Taure
force-pushed
the
fix/matchmaker-phantom-world-player
branch
from
August 26, 2026 16:48
8eef5c8 to
2d7c7b0
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A player who disconnects between taking a matchmaker ticket and their group forming was joined into the world anyway, and could then never leave.
The defect
asobi_world_server:join/2takes no session pid and resolves one through pg. With nobody registered it seats the player assession_pid => undefinedwith no monitor, and from there:find_player_by_pid/2matches onsession_pid =:= Pid, andundefinedis never a pid, so no DOWN ever resolves to them;handle_leave/2is reachable only from an explicit leave or that DOWN, somap_size(Players)never reaches 0, thefinishedtransition is unreachable, and theempty_gracetimer is never even scheduled.The world ticks forever around a player who is not there. World-side twin of asobi#280, same root cause: nothing checked that the ticket's owner was still connected.
Measured during review: 105 processes and ~1.1 MB leaked permanently per occurrence. The matchmaker's default
grid_sizeof 10 leaveslazy_zonesoff, so every such world pre-spawns 100 zones.Two ways in, both closed
A human who disconnects while queued.
join_if_present/3screensasobi_presence:get_status/1before seating.Checked in the matchmaker rather than refused in
asobi_world_server, because a session-less join is legitimate for other callers by design (asobi#277 madeplace_player/4degrade rather than crash), and because this is the only production path that reaches the state — the WS handler usesjoin/4with 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_sessiontelemetry now counts it.Every bot on a world-type mode, with no attacker at all.
scan_groups/2starts 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. One human queueing on such a mode leaked one world andmin_players - 1unreleasable seats.Filling was never going to work there; it failed silently.
fillable/2refuses it and says so, because a game declaringgame_type = "world"alongside abotstable has no other way to discover the mistake.Verification
Both fixes were checked by reverting them and confirming the specific test fails, not just by the suite going green.
fmt --check,xref,dialyzer,elp lintclean; eunit 2,403/0; CT 390/0;eqwalize-all194, from 212 on main.Also clears three pre-existing eqwalizer errors in
asobi_matchmaker— CI checks every module a PR touches:max/2returnsterm()sowaited/2names the rule instead,lists:foldl/3erased the ticket map sorequeue_failed/2recurses explicitly, and the ETS queue snapshot is narrowed on the way out.Reported, not fixed here
Found by the review gate on #568, each a separate concern:
asobi_world_instance_sup:start_world/1directly, soasobi_world_lobby:check_world_capacity/1never runs and these worlds are invisible toglobal_world_count/0— making the lobby's own cap under-count. This PR closes the phantom half of the leak, not the uncapped half.join/4discards the authenticatedSessionPidand re-derives from pg;backfill_zone_subscribers/4re-resolves on every zone creation, and that pid is also joined to chat channels.add/2validates nothing about the player id.