From b7b2e7df6f1a0edf9a84c420a35547042c9b29f3 Mon Sep 17 00:00:00 2001 From: ChronoFinale Date: Sun, 12 Jul 2026 19:18:04 -0500 Subject: [PATCH] fix(lobby): pause on opponent disconnect instead of instant win Any opponent departure mid-run -- including a transient network drop -- instantly showed the local player the win screen. The MQTT bridge's PLAYER_LEFT handler called the gamemode's on_player_forfeit as soon as G.STAGE == RUN, with no distinction between a deliberate leave and a dropped connection, and it never subscribed to the API's PLAYER_DISCONNECTED/PLAYER_RECONNECTED events at all -- so the existing pause/countdown machinery (action_enemyDisconnected/action_enemyReconnected, MP.enemy_disconnect_countdown) was dead code in the MQTT flow. The API's lobby event stream also cannot reliably distinguish a deliberate leave from a drop: `player_left` fires both for an explicit lobby:leave() and for an ungraceful connection drop whose retained players//info topic clears, which can arrive before, instead of, or without ever seeing a player_disconnected event first, with no reason field to tell them apart. Given that ambiguity, every mid-run opponent departure now routes into the same pause/grace flow (pvp_api/disconnect_grace.lua's MP.decide_departure_action), and only local grace expiry ends the match -- there is no server anymore to send stopGame on timeout, so the client resolves the forfeit itself, exactly once, through the existing host-authoritative on_player_forfeit path. - pvp_api/disconnect_grace.lua: new pure decision core -- event + state in, ignore/start_grace/cancel_grace out -- plus the single-fire expiry guard. - pvp_api/lobby_bridge.lua: subscribe to PLAYER_DISCONNECTED/PLAYER_RECONNECTED, route all three departure events through the decision core, and add the grace-expiry forfeit shell (MP.resolve_enemy_disconnect_forfeit). - networking/action_handlers.lua: track the departed player_id on the countdown, resolve it locally on expiry instead of waiting on a server stopGame that no longer exists. - tests/test_disconnect_grace.lua: pure-core scenarios, a RED control proving the old routing forfeited instantly on a network drop, and a shell-wiring test against a fake MPAPI lobby. --- networking/action_handlers.lua | 21 ++- pvp_api/disconnect_grace.lua | 47 +++++++ pvp_api/lobby_bridge.lua | 60 ++++++++- tests/test_disconnect_grace.lua | 226 ++++++++++++++++++++++++++++++++ 4 files changed, 345 insertions(+), 9 deletions(-) create mode 100644 pvp_api/disconnect_grace.lua create mode 100644 tests/test_disconnect_grace.lua diff --git a/networking/action_handlers.lua b/networking/action_handlers.lua index 509f4889..db17e444 100644 --- a/networking/action_handlers.lua +++ b/networking/action_handlers.lua @@ -127,10 +127,21 @@ end local _disconnect_gupdate = Game.update function Game:update(dt) if MP.enemy_disconnect_countdown then - local remaining = math.max(0, math.ceil(MP.enemy_disconnect_countdown.end_time - love.timer.getTime())) - MP.enemy_disconnect_countdown.display = remaining .. "s remaining" - -- No client-side timeout needed: the server sends stopGame - -- when the grace period expires, which handles the cleanup + local countdown = MP.enemy_disconnect_countdown + local remaining = math.max(0, math.ceil(countdown.end_time - love.timer.getTime())) + countdown.display = remaining .. "s remaining" + -- There is no server anymore to send stopGame on timeout (MQTT/API + -- framework is a relay only) -- resolve the forfeit locally, exactly + -- once (MP.disconnect_grace_expired is the single-fire guard). + if MP.disconnect_grace_expired(remaining, countdown) then + countdown.resolved = true + local departed_id = countdown.player_id + MP.enemy_disconnect_countdown = nil + G.FUNCS.exit_overlay_menu() + if MP.resolve_enemy_disconnect_forfeit then + MP.resolve_enemy_disconnect_forfeit(departed_id) + end + end end if MP.self_reconnect_countdown then local remaining = math.max(0, math.ceil(MP.self_reconnect_countdown.end_time - love.timer.getTime())) @@ -153,6 +164,8 @@ local function action_enemyDisconnected(p) MP.enemy_disconnect_countdown = { end_time = love.timer.getTime() + timeout, display = timeout .. "s remaining", + player_id = p.player_id, + resolved = false, } MP.UI.UTILS.overlay_message_countdown( diff --git a/pvp_api/disconnect_grace.lua b/pvp_api/disconnect_grace.lua new file mode 100644 index 00000000..1cfbd914 --- /dev/null +++ b/pvp_api/disconnect_grace.lua @@ -0,0 +1,47 @@ +-- Pure decision logic for opponent disconnect/reconnect/leave events mid-run. +-- +-- Why every departure routes through grace: the API's lobby event stream +-- cannot reliably distinguish a deliberate leave from an ungraceful network +-- drop. `player_left` fires both for an explicit `lobby:leave()` call AND +-- for a raw connection drop whose retained `players//info` topic clears +-- (LWT) -- which can arrive before, instead of, or without ever seeing a +-- `player_disconnected` event first (see BalatroMultiplayerAPI's +-- api/lobby/events.lua `handle_player_info`). There is no `reason` field on +-- the event to disambiguate. So rather than guess wrong and instant-win/lose +-- a match on a network blip, every mid-run opponent departure -- however it +-- is reported -- routes into the same pause/grace flow; only local grace +-- EXPIRY (there is no authoritative server anymore to send `stopGame`) turns +-- it into a forfeit. See the PR body for the full writeup of this tradeoff. + +-- state: { in_run: bool, is_opponent: bool, grace_active: bool } +-- event: "player_disconnected" | "player_reconnected" | "player_left" +-- returns "ignore" | "start_grace" | "cancel_grace" +function MP.decide_departure_action(event, state) + if not state or not state.is_opponent or not state.in_run then + return "ignore" + end + if event == "player_reconnected" then + if state.grace_active then + return "cancel_grace" + end + return "ignore" + end + if event == "player_disconnected" or event == "player_left" then + if state.grace_active then + return "ignore" -- already paused; let grace expiry (or reconnect) resolve it + end + return "start_grace" + end + return "ignore" +end + +-- Pure guard for the countdown tick in networking/action_handlers.lua: should +-- this tick resolve the grace period into a forfeit? Single-fire by +-- construction -- once `countdown.resolved` is true (or the countdown itself +-- has been cleared, e.g. by a reconnect), this always answers false. +function MP.disconnect_grace_expired(remaining, countdown) + if not countdown or countdown.resolved then + return false + end + return remaining <= 0 +end diff --git a/pvp_api/lobby_bridge.lua b/pvp_api/lobby_bridge.lua index e06b2158..ecf82f14 100644 --- a/pvp_api/lobby_bridge.lua +++ b/pvp_api/lobby_bridge.lua @@ -133,6 +133,45 @@ local function mirror_players(lobby) end MP.mirror_players = mirror_players +-- Effectful counterpart to MP.decide_departure_action's "start_grace" / +-- "cancel_grace" outcomes, and to the grace-expiry forfeit (there is no +-- server anymore to send `stopGame` on timeout -- see disconnect_grace.lua). +local function handle_departure_event(lobby, event, player_id) + if player_id == nil or player_id == lobby.player_id then + return -- not the opponent (1v1: the only other player_id is the opponent) + end + local state = { + is_opponent = true, + in_run = G.STAGE == G.STAGES.RUN, + grace_active = MP.enemy_disconnect_countdown ~= nil, + } + local action = MP.decide_departure_action(event, state) + if action == "start_grace" then + MP.dispatch_action("enemyDisconnected", { player_id = player_id }) + elseif action == "cancel_grace" then + MP.dispatch_action("enemyReconnected", { player_id = player_id }) + end +end + +-- Called from the grace-countdown expiry tick (networking/action_handlers.lua) +-- once its own single-fire guard (MP.disconnect_grace_expired) says go. +-- Reuses the existing host-authoritative forfeit path (pvp_api/gamemodes.lua +-- on_player_forfeit -> check_single_survivor -> { winner = ... }) so there is +-- exactly one way a departure ever ends a match. on_player_forfeit returns +-- data instead of broadcasting itself (see api/gamemode/winner.lua) -- +-- MPAPI._handle_gamemode_result is what turns a { winner = ... } result into +-- the pvp_player_won broadcast, same as run_actions.lua's pvp_forfeit handler. +function MP.resolve_enemy_disconnect_forfeit(player_id) + if not player_id then + return + end + local lobby = MPAPI.get_current_lobby() + local gm = lobby and lobby.get_gamemode_instance and lobby:get_gamemode_instance() + if gm and gm.on_player_forfeit then + MPAPI._handle_gamemode_result(gm, gm:on_player_forfeit(player_id)) + end +end + MP.setup_lobby_mirror = function(lobby) MP.CURRENT_LOBBY = lobby MP.LOBBY.code = lobby.code @@ -182,14 +221,17 @@ MP.setup_lobby_mirror = function(lobby) MP.lobby.seed_votes:remove(player_id) end mirror_players(lobby) - -- The gamemode's forfeit hook (host-authoritative) handles a mid-match leave. - local gm = lobby.get_gamemode_instance and lobby:get_gamemode_instance() - if gm and gm.on_player_forfeit and G.STAGE == G.STAGES.RUN then - MPAPI._handle_gamemode_result(gm, gm:on_player_forfeit(player_id)) - end + -- Mid-run: never forfeit instantly here -- `player_left` can equally mean a + -- deliberate leave or an ungraceful drop (see disconnect_grace.lua). Pause + -- and wait out the grace period; only its expiry ends the match. + handle_departure_event(lobby, MPAPI.LobbyEvent.PLAYER_LEFT, player_id) refresh() end) + lobby:on(MPAPI.LobbyEvent.PLAYER_DISCONNECTED, function(player_id) + handle_departure_event(lobby, MPAPI.LobbyEvent.PLAYER_DISCONNECTED, player_id) + end) + -- Phase 9: reconnect tail-replay. PLAYER_RECONNECTED fires to every lobby -- member (including the reconnecting player's own client, once it -- re-subscribes to lobby/{code}/events) -- only act when the reconnecting @@ -204,6 +246,14 @@ MP.setup_lobby_mirror = function(lobby) end end) + -- Opponent-side grace: cancels the local disconnect-grace countdown when the + -- opponent (not us) reconnects. Separate handler from the tail-replay one + -- above -- handle_departure_event's own player_id==lobby.player_id guard + -- makes the two mutually exclusive per event, so registering both is safe. + lobby:on(MPAPI.LobbyEvent.PLAYER_RECONNECTED, function(player_id) + handle_departure_event(lobby, MPAPI.LobbyEvent.PLAYER_RECONNECTED, player_id) + end) + lobby:on(MPAPI.LobbyEvent.METADATA_CHANGED, function(metadata) mirror_metadata(lobby) MPAPI.refresh_current_view() diff --git a/tests/test_disconnect_grace.lua b/tests/test_disconnect_grace.lua new file mode 100644 index 00000000..785e49c8 --- /dev/null +++ b/tests/test_disconnect_grace.lua @@ -0,0 +1,226 @@ +--[[ + Disconnect-grace routing test. + + Covers the bug: "opponent leaves -> instant win screen" for ANY departure, + including a transient network drop. Exercises: + + 1. The pure decision core (pvp_api/disconnect_grace.lua): + MP.decide_departure_action(event, state) -> "ignore" | "start_grace" | "cancel_grace" + MP.disconnect_grace_expired(remaining, countdown) -> bool (single-fire guard) + + 2. A RED control: the OLD pre-fix routing (PLAYER_LEFT -> instant + on_player_forfeit whenever G.STAGE == RUN, no grace distinction) run + through the same network-drop scenario, proving it forfeits + immediately -- the exact bug -- while the new decision core does not. + + 3. The shell wiring (pvp_api/lobby_bridge.lua): a fake MPAPI lobby drives + PLAYER_DISCONNECTED / PLAYER_LEFT / PLAYER_RECONNECTED through the real + `lobby:on` handlers registered by MP.setup_lobby_mirror, with a fake + MP.dispatch_action that mimics the real enemyDisconnected/enemyReconnected + handlers just enough to track countdown state -- asserting only ONE + "enemyDisconnected" dispatch happens per outage (idempotent while grace + is active) and that a reconnect cancels it. + + Run from the repo root: + luajit tests/test_disconnect_grace.lua +]] + +local failures = 0 +local function check(name, cond) + if cond then + print("ok - " .. name) + else + failures = failures + 1 + print("FAIL - " .. name) + end +end + +-- ─── 1. Pure decision core ─────────────────────────────────────────────────── + +MP = {} +dofile("pvp_api/disconnect_grace.lua") + +local function state(overrides) + local s = { is_opponent = true, in_run = true, grace_active = false } + for k, v in pairs(overrides or {}) do + s[k] = v + end + return s +end + +check( + "not-opponent events are ignored", + MP.decide_departure_action("player_disconnected", state({ is_opponent = false })) == "ignore" +) +check("out-of-run events are ignored", MP.decide_departure_action("player_disconnected", state({ in_run = false })) == "ignore") +check( + "first player_disconnected starts grace", + MP.decide_departure_action("player_disconnected", state({ grace_active = false })) == "start_grace" +) +check( + "player_disconnected while grace already active is a no-op", + MP.decide_departure_action("player_disconnected", state({ grace_active = true })) == "ignore" +) +check( + "player_reconnected while grace active cancels it", + MP.decide_departure_action("player_reconnected", state({ grace_active = true })) == "cancel_grace" +) +check( + "player_reconnected with no active grace is a no-op", + MP.decide_departure_action("player_reconnected", state({ grace_active = false })) == "ignore" +) +-- The core fix: player_left (which the API can also fire for an ungraceful +-- LWT-driven drop, not just a deliberate leave) starts grace instead of an +-- instant forfeit. +check( + "player_left with no active grace starts grace (never an instant forfeit)", + MP.decide_departure_action("player_left", state({ grace_active = false })) == "start_grace" +) +check( + "player_left while grace already active is a no-op (avoids double-resolution)", + MP.decide_departure_action("player_left", state({ grace_active = true })) == "ignore" +) +check("unknown event is ignored", MP.decide_departure_action("something_else", state()) == "ignore") + +-- Expiry single-fire guard. +check("expired countdown (remaining<=0, unresolved) resolves", MP.disconnect_grace_expired(0, { resolved = false }) == true) +check("not-yet-expired countdown does not resolve", MP.disconnect_grace_expired(5, { resolved = false }) == false) +check("already-resolved countdown never resolves again", MP.disconnect_grace_expired(0, { resolved = true }) == false) +check("nil countdown never resolves", MP.disconnect_grace_expired(0, nil) == false) + +-- ─── 2. RED control: the old pre-fix routing, run through the same scenario ── + +-- This mirrors the exact logic that shipped before this fix +-- (pvp_api/lobby_bridge.lua's PLAYER_LEFT handler, pre-fix): +-- if gm and gm.on_player_forfeit and G.STAGE == G.STAGES.RUN then +-- gm:on_player_forfeit(player_id) +-- end +-- with no distinction between a deliberate leave and a network drop, and no +-- subscription to PLAYER_DISCONNECTED/PLAYER_RECONNECTED at all. +local function old_pre_fix_on_player_left(in_run) + if in_run then + return "forfeit_now" -- <- the bug: instant win/lose on ANY departure + end + return "ignore" +end + +-- Scenario: opponent's connection drops mid-run. The API fires `player_left` +-- (e.g. via the LWT-cleared players//info topic) with no prior +-- `player_disconnected` ever observed -- exactly the ambiguous case described +-- in disconnect_grace.lua's header comment. +local dropped_mid_run = { is_opponent = true, in_run = true, grace_active = false } + +check( + "RED: old pre-fix logic forfeits instantly on a mid-run network drop (the bug)", + old_pre_fix_on_player_left(dropped_mid_run.in_run) == "forfeit_now" +) +check( + "GREEN: new decision core pauses instead of forfeiting on the same drop", + MP.decide_departure_action("player_left", dropped_mid_run) == "start_grace" +) + +-- ─── 3. Shell wiring: pvp_api/lobby_bridge.lua ─────────────────────────────── + +-- Fresh global MP/MPAPI/G stubs for the shell-level test so nothing leaks +-- from the pure-core section above. +local dispatch_log = {} +local exit_overlay_calls = 0 + +G = { STAGE = "RUN", STAGES = { RUN = "RUN", MAIN_MENU = "MAIN_MENU" } } +G.FUNCS = { exit_overlay_menu = function() exit_overlay_calls = exit_overlay_calls + 1 end } + +MP = { + LOBBY = { config = {}, deck = {} }, + reset_game_states = function() end, + enemy_disconnect_countdown = nil, + dispatch_action = function(name, params) + dispatch_log[#dispatch_log + 1] = { name = name, params = params } + -- Mimic just enough of the real handlers (networking/action_handlers.lua) + -- to drive MP.enemy_disconnect_countdown for the idempotency assertions. + if name == "enemyDisconnected" then + MP.enemy_disconnect_countdown = { player_id = params.player_id } + elseif name == "enemyReconnected" then + MP.enemy_disconnect_countdown = nil + end + end, +} + +MPAPI = { + LobbyEvent = { + CONNECTED = "connected", + DISCONNECTED = "disconnected", + ERROR = "error", + PLAYER_JOINED = "player_joined", + PLAYER_LEFT = "player_left", + PLAYER_DISCONNECTED = "player_disconnected", + PLAYER_RECONNECTED = "player_reconnected", + METADATA_CHANGED = "metadata_changed", + HOST_CHANGED = "host_changed", + }, + get_current_lobby = function() return nil end, + create_lobby_ui = function() return {} end, + refresh_current_view = function() end, +} + +-- Minimal fake lobby: records `on(event, cb)` handlers and lets the test fire them. +local function make_fake_lobby() + local handlers = {} + local lobby + lobby = { + code = "TEST", + is_host = true, + player_id = "me", + on = function(_self, event, cb) + handlers[event] = handlers[event] or {} + table.insert(handlers[event], cb) + end, + get_players = function() return { { id = "me" }, { id = "opp" } } end, + get_metadata = function() return {} end, + fire = function(_self, event, ...) + for _, cb in ipairs(handlers[event] or {}) do + cb(...) + end + end, + } + return lobby +end + +dofile("pvp_api/disconnect_grace.lua") -- MP.decide_departure_action / MP.disconnect_grace_expired +dofile("pvp_api/lobby_bridge.lua") + +local lobby = make_fake_lobby() +MP.setup_lobby_mirror(lobby) + +-- Opponent's connection drops mid-run: PLAYER_DISCONNECTED fires. +lobby:fire(MPAPI.LobbyEvent.PLAYER_DISCONNECTED, "opp") +check("PLAYER_DISCONNECTED dispatches enemyDisconnected exactly once", #dispatch_log == 1 and dispatch_log[1].name == "enemyDisconnected") +check("grace countdown is now active", MP.enemy_disconnect_countdown ~= nil) + +-- A duplicate PLAYER_DISCONNECTED (e.g. redelivered) must not re-dispatch or +-- restart the countdown (idempotent while grace is already active). +lobby:fire(MPAPI.LobbyEvent.PLAYER_DISCONNECTED, "opp") +check("duplicate PLAYER_DISCONNECTED does not re-dispatch", #dispatch_log == 1) + +-- Reconnect before expiry cancels the grace period. +lobby:fire(MPAPI.LobbyEvent.PLAYER_RECONNECTED, "opp") +check("PLAYER_RECONNECTED dispatches enemyReconnected", #dispatch_log == 2 and dispatch_log[2].name == "enemyReconnected") +check("grace countdown cleared on reconnect", MP.enemy_disconnect_countdown == nil) + +-- A fresh outage that goes all the way to a `player_left` (no disconnected +-- event ever seen for it) also pauses -- the core fix -- rather than +-- forfeiting on the spot. +lobby:fire(MPAPI.LobbyEvent.PLAYER_LEFT, "opp") +check("PLAYER_LEFT with no prior grace starts a fresh grace period", #dispatch_log == 3 and dispatch_log[3].name == "enemyDisconnected") +check("no forfeit call was made synchronously from the event handler", exit_overlay_calls == 0) + +-- Self departures (player_id == lobby.player_id) must never be routed as an +-- opponent departure. +local dispatch_count_before_self = #dispatch_log +MP.enemy_disconnect_countdown = nil +lobby:fire(MPAPI.LobbyEvent.PLAYER_DISCONNECTED, "me") +check("self player_id is never treated as the opponent departing", #dispatch_log == dispatch_count_before_self) + +if failures > 0 then + error(failures .. " check(s) failed") +end +print("\nAll disconnect-grace checks passed.")