Send all the player options in auto lobby when host receives them #6995
Conversation
📝 WalkthroughWalkthroughA new early synchronization block is added to ProcessAddPlayerMessage in AutolobbyController that immediately broadcasts each player's Team, Army (StartSpot), StartSpot, and Faction to the lobby server by calling SendPlayerOptionToServer for each local PlayerOption when a player is added. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as AutolobbyController (Client)
participant Store as Local PlayerOptions
participant Lobby as Lobby Server
Note over UI,Store: Player added event triggers local sync
UI->>Store: Read all PlayerOptions
loop for each player option
UI->>Lobby: SendPlayerOptionToServer(Team, Army, StartSpot, Faction)
Lobby-->>UI: ACK (optional)
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| -- send all player options to the server, this is necessary to do immediately (for the host) so that the server knows who is (not) connected. | ||
| for slot, playerOptions in self.PlayerOptions do | ||
| local ownerId = playerOptions.OwnerID | ||
| self:SendPlayerOptionToServer(ownerId, 'Team', playerOptions.Team) | ||
| self:SendPlayerOptionToServer(ownerId, 'Army', playerOptions.StartSpot) | ||
| self:SendPlayerOptionToServer(ownerId, 'StartSpot', playerOptions.StartSpot) | ||
| self:SendPlayerOptionToServer(ownerId, 'Faction', playerOptions.Faction) | ||
| end |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's locate and examine the AutolobbyController.lua file
fd -name "AutolobbyController.lua" -type fRepository: FAForever/fa
Length of output: 286
🏁 Script executed:
#!/bin/bash
# Locate the file with correct syntax
fd --name "AutolobbyController.lua" --type fRepository: FAForever/fa
Length of output: 298
🏁 Script executed:
#!/bin/bash
# Find the file with correct fd syntax
fd "AutolobbyController.lua"Repository: FAForever/fa
Length of output: 102
🏁 Script executed:
#!/bin/bash
# Read the section around lines 627-634 with more context
echo "=== Lines 600-650 context ==="
sed -n '600,650p' lua/ui/lobby/autolobby/AutolobbyController.lua | cat -n
echo ""
echo "=== Lines 560-580 for LaunchThread comparison ==="
sed -n '560,580p' lua/ui/lobby/autolobby/AutolobbyController.lua | cat -nRepository: FAForever/fa
Length of output: 4321
🏁 Script executed:
#!/bin/bash
echo "=== Searching for SendPlayerOptionToServer calls with 'Army' ==="
rg "SendPlayerOptionToServer.*Army" -n -B2 -A2 lua/
echo ""
echo "=== Searching for server-side player option handling ==="
rg "set_player_option|_configured_player|Army" -n lua/ui/lobby/autolobby/ | head -40
echo ""
echo "=== Check if there are other places that send all player options ==="
rg "for.*PlayerOptions.*do" -n lua/ui/lobby/autolobby/AutolobbyController.luaRepository: FAForever/fa
Length of output: 2538
🏁 Script executed:
#!/bin/bash
echo "=== Reading AutolobbyServerCommunicationsComponent.lua around SendPlayerOptionToServer ==="
cat -n lua/ui/lobby/autolobby/components/AutolobbyServerCommunicationsComponent.lua | head -150Repository: FAForever/fa
Length of output: 8540
🏁 Script executed:
#!/bin/bash
echo "=== Search for Army-related comments or documentation ==="
rg -i "army" lua/ui/lobby/autolobby/ -B2 -A2 | head -80
echo ""
echo "=== Check git history or comments around the ProcessAddPlayerMessage change ==="
rg -B5 -A5 "ProcessAddPlayerMessage" lua/ui/lobby/autolobby/AutolobbyController.lua | head -60Repository: FAForever/fa
Length of output: 5447
Fix Army value inconsistency: use calculated army index, not StartSpot.
This implementation has critical issues:
-
Army value bug: Line 631 sends
playerOptions.StartSpotas the Army value, but LaunchThread (line 562) explicitly documents that "Army numbers need to be calculated: they are numbered incrementally in slot order" and correctly uses a calculatedarmyIndexat line 574. Sending StartSpot as Army will cause the server to receive inconsistent and incorrect Army values. -
Inefficient synchronization: The loop sends ALL player options every time ANY single player is added, resulting in O(N²) messages for N sequential players (1+2+3+...+N = 36 messages for an 8-player game instead of 8).
-
Variable shadowing: The loop variable
playerOptions(line 628) shadows the function parameterplayerOptions(line 618), reducing clarity.
If the intent is to ensure the server knows about all connected players early, that's reasonable. However, the Army value must be calculated consistently. Consider sending only the newly added player's options, or if all-player sync is necessary, compute Army values the same way as LaunchThread does.
💡 Suggested fix if sending only the new player
- -- send all player options to the server, this is necessary to do immediately (for the host) so that the server knows who is (not) connected.
- for slot, playerOptions in self.PlayerOptions do
- local ownerId = playerOptions.OwnerID
- self:SendPlayerOptionToServer(ownerId, 'Team', playerOptions.Team)
- self:SendPlayerOptionToServer(ownerId, 'Army', playerOptions.StartSpot)
- self:SendPlayerOptionToServer(ownerId, 'StartSpot', playerOptions.StartSpot)
- self:SendPlayerOptionToServer(ownerId, 'Faction', playerOptions.Faction)
- end
+ -- Inform the server about the newly added player's options immediately
+ -- so the server can mark them as configured and avoid incorrect violation bans
+ local ownerId = playerOptions.OwnerID
+ self:SendPlayerOptionToServer(ownerId, 'Team', playerOptions.Team)
+ self:SendPlayerOptionToServer(ownerId, 'Army', playerOptions.StartSpot)
+ self:SendPlayerOptionToServer(ownerId, 'StartSpot', playerOptions.StartSpot)
+ self:SendPlayerOptionToServer(ownerId, 'Faction', playerOptions.Faction)Note: If sending only the new player, you still need to compute the correct Army index. This requires calculating the position of this player in the sorted slot order, similar to LaunchThread lines 563-576.
Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
This is actually a good point 😃 ! How do we know what the actual start spot is for a player? We can't exactly trust on the user input here.
There was a problem hiding this comment.
The suggested fix is wrong, since then the host would never sent their own player options with the current changes.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for StartSpot validation and slot management
echo "=== Looking for StartSpot validation patterns ==="
rg -n "StartSpot" lua/ui/lobby/autolobby/ -B2 -A2 | head -80
echo ""
echo "=== Looking for slot occupancy checks ==="
rg -n "occupied|slot.*check|validate.*slot" lua/ui/lobby/autolobby/ -i
echo ""
echo "=== Looking for PlayerOptions bounds or validation ==="
rg -n "PlayerOptions\[" lua/ui/lobby/autolobby/AutolobbyController.lua -B1 -A1
echo ""
echo "=== Checking PlayerCount usage for bounds ==="
rg -n "PlayerCount" lua/ui/lobby/autolobby/AutolobbyController.lua | head -20
Length of output: 7969
@Garanas, excellent security question! You're absolutely right to be concerned about trusting client-provided StartSpot values. Currently, no validation exists — the code directly uses the client's StartSpot as an array index at line 625, which is unsafe.
Missing Validations
The TODO comment at line 623 flags this, but here's what needs to be validated before line 625:
-- Validate StartSpot before using it
local startSpot = playerOptions.StartSpot
-- 1. Type and existence check
if type(startSpot) ~= 'number' then
self:DebugWarn("Invalid StartSpot type from", data.SenderID)
-- Reject or assign a free slot
return
end
-- 2. Bounds check (must be within valid player slots)
if startSpot < 1 or startSpot > self.PlayerCount then
self:DebugWarn("StartSpot out of bounds from", data.SenderID, startSpot, self.PlayerCount)
-- Reject or assign a free slot
return
end
-- 3. Occupancy check (slot must not be taken)
if self.PlayerOptions[startSpot] ~= nil then
self:DebugWarn("StartSpot already occupied:", startSpot, "by", self.PlayerOptions[startSpot].PlayerName)
-- Reject or assign a free slot
return
endHost Authority Model
Since this is ProcessAddPlayerMessage (which only the host receives), the host should have full authority to:
- Validate client requests
- Assign slots deterministically if the client's request is invalid
- Potentially find the first available slot and assign it
The autolobby appears to be designed for matchmaker-controlled games where slot assignments come from the server via command-line args, but validation is still needed for defensive programming.
The Original Army Value Bug
Separately, the critical issue from my original comment remains: line 631 sends playerOptions.StartSpot as the Army value, but LaunchThread (lines 563-576) correctly calculates Army as an incrementally numbered index in sorted slot order. This inconsistency will cause the server to receive wrong Army values, regardless of whether StartSpot validation is added.
Both issues need fixing:
- Add StartSpot validation (security/correctness)
- Fix Army value to use calculated index, not StartSpot (correctness/consistency)
Would you like me to provide a more complete code suggestion that addresses both concerns?
🧬 Code Graph Analysis Results
engine/User.lua
- Snippet: HasCommandLineArg(option)
- Description: Function signature for checking if a command-line option is present. Relevant to how AutolobbyController.lua reads command-line arguments (e.g., /startspot) to set local player StartSpot.
- Full snippet:
function HasCommandLineArg(option) end- Context: Used by code paths that parse startup arguments to configure local player options.
lua/simInit.lua
- Snippet: WaitSeconds(n)
- Description: Basic timing helper that yields for a number of seconds. Relevant to understanding timing-based behavior in autolobby threads (e.g., rejoin checks, status broadcasting).
- Full snippet:
function WaitSeconds(n) if n <= 0.1 then WaitTicks(1) return end WaitTicks(n * 10 + 1) end- Notes: Used to pause coroutines/threads; informs how long certain launch/retry sequences wait.
lua/userInit.lua
- Snippet: WaitSeconds(n)
- Description: Enhanced timing helper with frame/time measurement to drive waits, including dynamic calculation of elapsed time and FPS. Relevant for understanding timing behavior in autolobby threads and rejoin logic.
- Full snippet:
function WaitSeconds(n) local start = CurrentTime() local elapsed_frames = 0 local elapsed_time = 0 local wait_frames repeat wait_frames = math.ceil(math.max(1, AvgFPS * 0.1, n * AvgFPS)) WaitFrames(wait_frames) elapsed_frames = elapsed_frames + wait_frames elapsed_time = CurrentTime() - start until elapsed_time >= n if elapsed_time >= 3 then AvgFPS = math.max(10, math.min(200, math.ceil(elapsed_frames / elapsed_time))) end end - Snippet: WaitSeconds(n) (alternative minimal definition)
- Description: Another definition of WaitSeconds used in this file; helps contrast simple vs. FPS-aware waiting.
- Full snippet:
function WaitSeconds(n) local start = CurrentTime() local elapsed_time = 0 while elapsed_time < n do local dt = WaitTicks(1) elapsed_time = CurrentTime() - start end end- Notes: Illustrates timing logic that could affect how startup/startspot decisions propagate in autolobby.
There was a problem hiding this comment.
@Garanas You're absolutely right! I apologize for the incorrect suggestion. The host creates their own player options in the Hosting function but never goes through ProcessAddPlayerMessage, so they would indeed never send their options if we only sent the newly added player.
The loop sending all player options makes sense — it ensures the server has complete information about all connected players (including the host) whenever anyone joins.
However, the Army value bug remains critical: Line 631 still sends playerOptions.StartSpot as the Army value, but Army indices must be calculated incrementally in sorted slot order (as LaunchThread correctly does on lines 563-576).
Here's a corrected fix that preserves the all-player synchronization while fixing the Army calculation:
-- send all player options to the server, this is necessary to do immediately (for the host) so that the server knows who is (not) connected.
-- Army numbers need to be calculated: they are numbered incrementally in slot order.
local slots = {}
for slotIndex, _ in pairs(self.PlayerOptions) do
table.insert(slots, slotIndex)
end
table.sort(slots)
for armyIndex, slotIndex in ipairs(slots) do
local playerOpts = self.PlayerOptions[slotIndex]
local ownerId = playerOpts.OwnerID
self:SendPlayerOptionToServer(ownerId, 'Team', playerOpts.Team)
self:SendPlayerOptionToServer(ownerId, 'Army', armyIndex) -- use calculated armyIndex, not StartSpot
self:SendPlayerOptionToServer(ownerId, 'StartSpot', playerOpts.StartSpot)
self:SendPlayerOptionToServer(ownerId, 'Faction', playerOpts.Faction)
endThis also addresses the variable shadowing issue by using playerOpts in the loop instead of reusing playerOptions.
Regarding your earlier question about trusting start spots: that's a good security concern. The current code accepts whatever StartSpot the joining player claims (line 624). Should there be validation to ensure the StartSpot is not already occupied and within valid range [1, PlayerCount]?
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lua/ui/lobby/autolobby/AutolobbyController.lua (1)
623-625: Add validation before using StartSpot as array index.The TODO comment at line 623 correctly identifies the missing validation. Currently, line 625 uses
playerOptions.StartSpotdirectly as an array index without verifying:
- Type (must be a number)
- Bounds (must be within
[1, PlayerCount])- Occupancy (slot must not already be taken)
Using unvalidated client input as an array index is a security and correctness risk that could allow malicious clients to corrupt the PlayerOptions table or cause runtime errors.
🔎 Proposed validation
-- TODO: verify that the StartSpot is not occupied +-- Validate StartSpot before using it as an array index +local startSpot = playerOptions.StartSpot + +-- Type and existence check +if type(startSpot) ~= 'number' then + self:DebugWarn("Invalid StartSpot type from", data.SenderID, "expected number, got", type(startSpot)) + return +end + +-- Bounds check +if startSpot < 1 or startSpot > self.PlayerCount then + self:DebugWarn("StartSpot out of bounds from", data.SenderID, "value:", startSpot, "max:", self.PlayerCount) + return +end + +-- Occupancy check +if self.PlayerOptions[startSpot] ~= nil then + self:DebugWarn("StartSpot already occupied:", startSpot, "by", self.PlayerOptions[startSpot].PlayerName, "rejecting", data.SenderID) + return +end + -- put the player where it belongs self.PlayerOptions[playerOptions.StartSpot] = playerOptionsNote: Based on your earlier question: "How do we know what the actual start spot is for a player? We can't exactly trust on the user input here."
|
I'm fine with it, but out of curiosity: local slots = {}
for slotIndex, _ in pairs(self.PlayerOptions) do
table.insert(slots, slotIndex)
end
table.sort(slots)This may return different values depending on who joined at that stage, or do I miss understand? |
|
Not in the autolobby, since the starting spot is passed by the cmd line argument into the game. Starting spots in the ladder and matchmaker games are even vs odd, so it cant be determined by who joined first (like in custom lobby, where it doesnt matter) |
|
@speed2CZ according to Zulip it only requires one player option of any type. What if we just send the faction, since that is set the moment a player joins. Then proceed to send the rest (+ faction again) when the game actually starts? Would that work for you? I can also encapsulate the data that is sent into a separate function. |
|
And what about sending all the options and then not resending them again when the game is launching. The options do not change in the game. |
|
But we don't know this order until everyone joined: -- Army numbers need to be calculated: they are numbered incrementally in slot order.
local slots = {}
for slotIndex, _ in pairs(self.PlayerOptions) do
table.insert(slots, slotIndex)
end
table.sort(slots)
-- send player options to the server
for armyIndex, slotIndex in ipairs(slots) do -- <-- used as army index here, which we don't know in advance until everyone's there?
local playerOptions = self.PlayerOptions[slotIndex]
local ownerId = playerOptions.OwnerID
self:SendPlayerOptionToServer(ownerId, 'Team', playerOptions.Team)
self:SendPlayerOptionToServer(ownerId, 'Army', armyIndex)
self:SendPlayerOptionToServer(ownerId, 'StartSpot', playerOptions.StartSpot)
self:SendPlayerOptionToServer(ownerId, 'Faction', playerOptions.Faction)
endWhat is hacky about just sending the faction of the player that joined and then sending everything upon launch like it used to be? edit: feel free to make a commit or two to make it work the way you think it should. As long as we sent something the moment a connection is made. |
|
Another thing: why do we even send these options in the autolobby? They are all decided by the server arent they, since they are passed into the game via cmd line arguments. There is a |
I have no idea. I may be wrong, but it's the Java API that determines the match. While it is the Python lobby server that manages the lobby state. I'm not sure if or when they communicate to one another. Either way, I assume the Python lobby server does not distinguish between custom lobbies and matchmaker lobbies. And therefore just requires this data to be sent. |
2 participants
Description of the proposed changes
Closes FAForever/server#1077.
Testing done on the proposed changes
Launched the game through the autolobby using the launch script, observed that the messages are sent in the logs.
Additional context
Discussion on Zulip:
Checklist
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.