move websockets to worker #2974
Conversation
WalkthroughA Redis-backed distributed lobby system replaces in-memory WebSocket broadcasting for game scheduling. New RedisClient manages shared state and readiness. Master polls Redis and schedules games via Redis. WorkerLobbyService handles lobby WebSocket upgrades and broadcasts. Client chooses a random worker path. Docker/supervisord and deps updated to run Redis. Changes
Sequence DiagramssequenceDiagram
participant Client
participant Master
participant Redis
participant Worker as Worker (n)
Client->>Master: GET /api/server_config
Master-->>Client: { numWorkers }
Client->>Client: pick random workerPath (e.g., /w2)
Client->>Worker: WS wss://host/w2/lobbies or HTTP /w2/api/public_lobbies
Worker-->>Client: lobbies_update messages
Master->>Redis: GET PUBLIC_LOBBY_IDS
Redis-->>Master: [gameIDs]
Master->>Redis: HSET PENDING_GAMES for worker X (addPendingGame)
Redis-->>Redis: store pending game entry
Worker->>Redis: HGETALL PENDING_GAMES (filter by worker)
Redis-->>Worker: pending games list
Worker->>Worker: create game instances, update lobby info
Worker->>Redis: HSET LOBBY_INFO entries
Worker->>Worker: broadcast lobbies_update to connected clients
sequenceDiagram
participant DevEnv as Dev
participant RedisInit
participant MemRedis as redis-memory-server
participant ProdEnv as Prod
participant LocalRedis as 127.0.0.1:6379
alt Development
DevEnv->>RedisInit: init
RedisInit->>MemRedis: spawn in-memory Redis
MemRedis-->>RedisInit: ready (host/port)
RedisInit-->>DevEnv: redisReady, set REDIS_HOST/PORT
else Production
ProdEnv->>RedisInit: init
RedisInit->>LocalRedis: connect to 127.0.0.1:6379
LocalRedis-->>RedisInit: connected
RedisInit-->>ProdEnv: redisReady
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
evanpelle
force-pushed
the
move-ws-worker
branch
from
e10aa22 to
eed0c4f
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Fix all issues with AI agents
In `@src/client/LobbySocket.ts`:
- Around line 12-15: The getRandomWorkerPath function can produce `/wNaN` or
out-of-range indices when numWorkers is 0 or invalid; update getRandomWorkerPath
to validate numWorkers (ensure it's a finite integer >= 1), default to 1 (or a
safe fallback like 1) when invalid, and clamp/select an index in the range [0,
numWorkers-1] so the returned path is always `/w0`..`/wN`. Locate and update
getRandomWorkerPath and any other place that calls it (the other occurrence
referenced at line 23) to use the same validation/guard logic or call a shared
validated helper.
- Line 1: The call to getServerConfigFromClient inside LobbySocket.start() can
throw and currently aborts before connectWebSocket and polling begin; wrap calls
to getServerConfigFromClient (used in start() and any other locations noted) in
a try/catch, assign a safe default server config or signal fallback mode when an
exception occurs, and ensure connectWebSocket() and the HTTP polling startup
logic still run (or start fallback polling) even on error; update the error
handling in the start() method and the other occurrences (around lines where
getServerConfigFromClient is called) to catch exceptions, log the error, and
proceed with defaultConfig/fallbackPolling so the lobby updates still start.
In `@src/server/RedisClient.ts`:
- Around line 148-169: In getAllLobbyInfo, corrupted JSON values for lobby
entries are left in Redis and repeatedly logged; update the catch block for
JSON.parse failures to remove the bad record by calling await
redis.hdel(LOBBY_INFO_KEY, gameID) (and then log the error with context), so
replace the current log.error-only behavior with deletion of the key using
redis.hdel before or after logging.
- Around line 83-101: In getPendingGamesForWorker, when JSON.parse fails for a
pending-game entry you should delete the corrupted record from Redis to stop
repeated errors; inside the catch in getPendingGamesForWorker (function name)
call redis.hdel(PENDING_GAMES_KEY, gameID) (or the equivalent async delete
method) after logging the parse failure, and handle/delete any error from hdel
gracefully so the function still returns the remaining valid PendingGame[].
- Around line 130-136: Replace the loose unknown type on LobbyInfo.gameConfig
with the concrete GameConfig | undefined union: update the LobbyInfo interface
(symbol: LobbyInfo) to declare gameConfig: GameConfig | undefined and add an
import for GameConfig from src/core/Schemas.ts (or the module that exports it)
so call sites no longer need unsafe assertions and TypeScript can type-check
game configuration fields.
In `@src/server/Worker.ts`:
- Around line 60-75: In server.on("upgrade") add a guard that parses the
incoming pathname for a worker prefix like "/wX" (extract the first path segment
from pathname) and compare it to the current process/instance worker identifier
(e.g., this.workerPrefix / workerId); if the prefix is present and does not
match the local worker, immediately reject the upgrade and destroy the socket
(do not call wss.handleUpgrade or lobbiesWss.handleUpgrade); otherwise continue
routing to lobbiesWss or wss as before. Ensure the check runs before the
existing pathname === "/lobbies" / endsWith("/lobbies") logic so mismatched
worker-prefixed requests can’t be accepted by the wrong WebSocketServer.
- Around line 172-206: startGameInfoPolling is not removing full lobbies from
Redis, causing the master to keep stale public IDs; after obtaining info =
game.gameInfo() check whether the lobby is full (e.g., info.clients?.length >=
info.gameConfig?.maxPlayers or other capacity field on gameConfig) and if so
call removePublicLobbyID(gameID) and removeLobbyInfo(gameID) (same as the
existing removal for started lobbies). Update the polling loop in
startGameInfoPolling / startPolling right after setLobbyInfo to detect full
lobbies and remove their IDs so Redis no longer advertises joinable lobbies from
this worker.
🧹 Nitpick comments (2)
src/server/Master.ts (1)
176-186: Make Redis writes for new lobbies atomic or rollback.If
addPendingGamesucceeds butaddPublicLobbyIDfails, a pending game is left behind and never advertised. Use Redis MULTI or rollback withremovePendingGame.🛠️ Simple rollback approach
import { addPendingGame, addPublicLobbyID, + removePendingGame, getPublicLobbyIDs, } from "./RedisClient"; async function schedulePublicGame() { const gameID = generateID(); try { const gameConfig = await playlist.gameConfig(); await addPendingGame(gameID, gameConfig); - await addPublicLobbyID(gameID); + try { + await addPublicLobbyID(gameID); + } catch (error) { + await removePendingGame(gameID); + throw error; + } log.info(`Scheduled public game ${gameID} via Redis`); } catch (error) { log.error(`Failed to schedule public game ${gameID}:`, error); throw error; } }src/server/RedisClient.ts (1)
7-59: Reduce duplicate Redis config and allow env overrides.Both branches repeat the same options and hardcode prod host/port. A small shared config keeps this clean and more deploy‑friendly.
♻️ Proposed refactor
-const isDevMode = process.env.GAME_ENV === "dev"; +const isDevMode = process.env.GAME_ENV === "dev"; +const redisOptions = { + maxRetriesPerRequest: 3, + retryStrategy(times: number) { + const delay = Math.min(times * 50, 2000); + return delay; + }, +}; +const prodRedisHost = process.env.REDIS_HOST ?? "127.0.0.1"; +const prodRedisPort = Number(process.env.REDIS_PORT ?? 6379); ... - redis = new Redis({ - host, - port, - maxRetriesPerRequest: 3, - retryStrategy(times) { - const delay = Math.min(times * 50, 2000); - return delay; - }, - }); + redis = new Redis({ host, port, ...redisOptions }); ... - redis = new Redis({ - host: "127.0.0.1", - port: 6379, - maxRetriesPerRequest: 3, - retryStrategy(times) { - const delay = Math.min(times * 50, 2000); - return delay; - }, - }); + redis = new Redis({ host: prodRedisHost, port: prodRedisPort, ...redisOptions });
| function getRandomWorkerPath(numWorkers: number): string { | ||
| const workerIndex = Math.floor(Math.random() * numWorkers); | ||
| return `/w${workerIndex}`; | ||
| } |
There was a problem hiding this comment.
Guard against invalid worker count.
If numWorkers is 0/NaN, the path can become /wNaN or point to a non‑existent worker. Add a small guard and fallback.
🛠️ Suggested guard
function getRandomWorkerPath(numWorkers: number): string {
- const workerIndex = Math.floor(Math.random() * numWorkers);
- return `/w${workerIndex}`;
+ if (!Number.isFinite(numWorkers) || numWorkers < 1) {
+ console.warn("Invalid numWorkers, defaulting to /w0");
+ return "/w0";
+ }
+ const workerIndex = Math.floor(Math.random() * numWorkers);
+ return `/w${workerIndex}`;
}Also applies to: 23-23
🤖 Prompt for AI Agents
In `@src/client/LobbySocket.ts` around lines 12 - 15, The getRandomWorkerPath
function can produce `/wNaN` or out-of-range indices when numWorkers is 0 or
invalid; update getRandomWorkerPath to validate numWorkers (ensure it's a finite
integer >= 1), default to 1 (or a safe fallback like 1) when invalid, and
clamp/select an index in the range [0, numWorkers-1] so the returned path is
always `/w0`..`/wN`. Locate and update getRandomWorkerPath and any other place
that calls it (the other occurrence referenced at line 23) to use the same
validation/guard logic or call a shared validated helper.
| export async function getPendingGamesForWorker( | ||
| workerIndex: (gameID: string) => number, | ||
| workerId: number, | ||
| ): Promise<PendingGame[]> { | ||
| await redisReady; | ||
| const allGames = await redis.hgetall(PENDING_GAMES_KEY); | ||
| const games: PendingGame[] = []; | ||
|
|
||
| for (const [gameID, value] of Object.entries(allGames)) { | ||
| if (workerIndex(gameID) === workerId) { | ||
| try { | ||
| games.push(JSON.parse(value) as PendingGame); | ||
| } catch (e) { | ||
| log.error(`Failed to parse pending game ${gameID}:`, e); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return games; |
There was a problem hiding this comment.
Clean up corrupted pending-game records.
If JSON parsing fails, the bad entry stays forever and keeps logging. Remove it on parse failure.
🐛 Suggested fix
if (workerIndex(gameID) === workerId) {
try {
games.push(JSON.parse(value) as PendingGame);
} catch (e) {
log.error(`Failed to parse pending game ${gameID}:`, e);
+ await redis.hdel(PENDING_GAMES_KEY, gameID);
}
}🤖 Prompt for AI Agents
In `@src/server/RedisClient.ts` around lines 83 - 101, In
getPendingGamesForWorker, when JSON.parse fails for a pending-game entry you
should delete the corrupted record from Redis to stop repeated errors; inside
the catch in getPendingGamesForWorker (function name) call
redis.hdel(PENDING_GAMES_KEY, gameID) (or the equivalent async delete method)
after logging the parse failure, and handle/delete any error from hdel
gracefully so the function still returns the remaining valid PendingGame[].
| export interface LobbyInfo { | ||
| gameID: string; | ||
| numClients: number; | ||
| msUntilStart?: number; | ||
| gameConfig?: unknown; | ||
| updatedAt: number; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find the GameConfig type
rg -n --type=ts "type GameConfig|interface GameConfig"Repository: openfrontio/OpenFrontIO
Length of output: 148
🏁 Script executed:
# Let's look at the RedisClient.ts file around those lines
cat -n src/server/RedisClient.ts | head -160 | tail -50Repository: openfrontio/OpenFrontIO
Length of output: 1862
🏁 Script executed:
# Search for LobbyInfo usage
rg -n --type=ts "LobbyInfo" -B 2 -A 2Repository: openfrontio/OpenFrontIO
Length of output: 3494
🏁 Script executed:
# Search for gameConfig usage
rg -n --type=ts "gameConfig" -B 2 -A 2Repository: openfrontio/OpenFrontIO
Length of output: 50379
Use GameConfig | undefined instead of unknown for gameConfig.
The GameConfig type is already well-defined in the codebase (from src/core/Schemas.ts). Using unknown forces type assertions at call sites and hides errors. Replace it with the proper typed union to keep the code safe and clear.
♻️ Possible change
export interface LobbyInfo {
gameID: string;
numClients: number;
msUntilStart?: number;
- gameConfig?: unknown;
+ gameConfig?: GameConfig;
updatedAt: number;
}Remember to import GameConfig from src/core/Schemas.ts if not already present.
🤖 Prompt for AI Agents
In `@src/server/RedisClient.ts` around lines 130 - 136, Replace the loose unknown
type on LobbyInfo.gameConfig with the concrete GameConfig | undefined union:
update the LobbyInfo interface (symbol: LobbyInfo) to declare gameConfig:
GameConfig | undefined and add an import for GameConfig from src/core/Schemas.ts
(or the module that exports it) so call sites no longer need unsafe assertions
and TypeScript can type-check game configuration fields.
| export async function getAllLobbyInfo(): Promise<LobbyInfo[]> { | ||
| await redisReady; | ||
| const allInfo = await redis.hgetall(LOBBY_INFO_KEY); | ||
| const lobbies: LobbyInfo[] = []; | ||
| const now = Date.now(); | ||
|
|
||
| for (const [gameID, value] of Object.entries(allInfo)) { | ||
| try { | ||
| const info = JSON.parse(value) as LobbyInfo; | ||
| // Skip stale entries (older than 5 seconds) | ||
| if (now - info.updatedAt > 5000) { | ||
| await redis.hdel(LOBBY_INFO_KEY, gameID); | ||
| continue; | ||
| } | ||
| lobbies.push(info); | ||
| } catch (e) { | ||
| log.error(`Failed to parse lobby info ${gameID}:`, e); | ||
| } | ||
| } | ||
|
|
||
| return lobbies; | ||
| } |
There was a problem hiding this comment.
Clean up corrupted lobby-info records on parse failure.
Right now bad JSON stays and will keep logging. Delete it on failure.
🐛 Suggested fix
try {
const info = JSON.parse(value) as LobbyInfo;
// Skip stale entries (older than 5 seconds)
if (now - info.updatedAt > 5000) {
await redis.hdel(LOBBY_INFO_KEY, gameID);
continue;
}
lobbies.push(info);
} catch (e) {
log.error(`Failed to parse lobby info ${gameID}:`, e);
+ await redis.hdel(LOBBY_INFO_KEY, gameID);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export async function getAllLobbyInfo(): Promise<LobbyInfo[]> { | |
| await redisReady; | |
| const allInfo = await redis.hgetall(LOBBY_INFO_KEY); | |
| const lobbies: LobbyInfo[] = []; | |
| const now = Date.now(); | |
| for (const [gameID, value] of Object.entries(allInfo)) { | |
| try { | |
| const info = JSON.parse(value) as LobbyInfo; | |
| // Skip stale entries (older than 5 seconds) | |
| if (now - info.updatedAt > 5000) { | |
| await redis.hdel(LOBBY_INFO_KEY, gameID); | |
| continue; | |
| } | |
| lobbies.push(info); | |
| } catch (e) { | |
| log.error(`Failed to parse lobby info ${gameID}:`, e); | |
| } | |
| } | |
| return lobbies; | |
| } | |
| export async function getAllLobbyInfo(): Promise<LobbyInfo[]> { | |
| await redisReady; | |
| const allInfo = await redis.hgetall(LOBBY_INFO_KEY); | |
| const lobbies: LobbyInfo[] = []; | |
| const now = Date.now(); | |
| for (const [gameID, value] of Object.entries(allInfo)) { | |
| try { | |
| const info = JSON.parse(value) as LobbyInfo; | |
| // Skip stale entries (older than 5 seconds) | |
| if (now - info.updatedAt > 5000) { | |
| await redis.hdel(LOBBY_INFO_KEY, gameID); | |
| continue; | |
| } | |
| lobbies.push(info); | |
| } catch (e) { | |
| log.error(`Failed to parse lobby info ${gameID}:`, e); | |
| await redis.hdel(LOBBY_INFO_KEY, gameID); | |
| } | |
| } | |
| return lobbies; | |
| } |
🤖 Prompt for AI Agents
In `@src/server/RedisClient.ts` around lines 148 - 169, In getAllLobbyInfo,
corrupted JSON values for lobby entries are left in Redis and repeatedly logged;
update the catch block for JSON.parse failures to remove the bad record by
calling await redis.hdel(LOBBY_INFO_KEY, gameID) (and then log the error with
context), so replace the current log.error-only behavior with deletion of the
key using redis.hdel before or after logging.
src/server/Worker.ts
Outdated
| const wss = new WebSocketServer({ noServer: true }); | ||
| const lobbiesWss = new WebSocketServer({ noServer: true }); | ||
|
|
||
| // Handle upgrade requests to route to correct WebSocket server | ||
| server.on("upgrade", (request, socket, head) => { | ||
| const pathname = request.url ?? ""; | ||
| if (pathname === "/lobbies" || pathname.endsWith("/lobbies")) { | ||
| lobbiesWss.handleUpgrade(request, socket, head, (ws) => { | ||
| lobbiesWss.emit("connection", ws, request); | ||
| }); | ||
| } else { | ||
| wss.handleUpgrade(request, socket, head, (ws) => { | ||
| wss.emit("connection", ws, request); | ||
| }); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Validate worker prefix on WebSocket upgrades.
Upgrade handling bypasses the /wX middleware, so a mismatched path can be accepted by the wrong worker. Parse /wX and reject mismatches before routing.
🛠️ Suggested guard
server.on("upgrade", (request, socket, head) => {
- const pathname = request.url ?? "";
+ const pathname = request.url ?? "";
+ const match = pathname.match(/^\/w(\d+)(\/.*)?$/);
+ if (match && parseInt(match[1]) !== workerId) {
+ socket.destroy();
+ return;
+ }
if (pathname === "/lobbies" || pathname.endsWith("/lobbies")) {
lobbiesWss.handleUpgrade(request, socket, head, (ws) => {
lobbiesWss.emit("connection", ws, request);
});
} else {
wss.handleUpgrade(request, socket, head, (ws) => {
wss.emit("connection", ws, request);
});
}
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const wss = new WebSocketServer({ noServer: true }); | |
| const lobbiesWss = new WebSocketServer({ noServer: true }); | |
| // Handle upgrade requests to route to correct WebSocket server | |
| server.on("upgrade", (request, socket, head) => { | |
| const pathname = request.url ?? ""; | |
| if (pathname === "/lobbies" || pathname.endsWith("/lobbies")) { | |
| lobbiesWss.handleUpgrade(request, socket, head, (ws) => { | |
| lobbiesWss.emit("connection", ws, request); | |
| }); | |
| } else { | |
| wss.handleUpgrade(request, socket, head, (ws) => { | |
| wss.emit("connection", ws, request); | |
| }); | |
| } | |
| }); | |
| const wss = new WebSocketServer({ noServer: true }); | |
| const lobbiesWss = new WebSocketServer({ noServer: true }); | |
| // Handle upgrade requests to route to correct WebSocket server | |
| server.on("upgrade", (request, socket, head) => { | |
| const pathname = request.url ?? ""; | |
| const match = pathname.match(/^\/w(\d+)(\/.*)?$/); | |
| if (match && parseInt(match[1]) !== workerId) { | |
| socket.destroy(); | |
| return; | |
| } | |
| if (pathname === "/lobbies" || pathname.endsWith("/lobbies")) { | |
| lobbiesWss.handleUpgrade(request, socket, head, (ws) => { | |
| lobbiesWss.emit("connection", ws, request); | |
| }); | |
| } else { | |
| wss.handleUpgrade(request, socket, head, (ws) => { | |
| wss.emit("connection", ws, request); | |
| }); | |
| } | |
| }); |
🤖 Prompt for AI Agents
In `@src/server/Worker.ts` around lines 60 - 75, In server.on("upgrade") add a
guard that parses the incoming pathname for a worker prefix like "/wX" (extract
the first path segment from pathname) and compare it to the current
process/instance worker identifier (e.g., this.workerPrefix / workerId); if the
prefix is present and does not match the local worker, immediately reject the
upgrade and destroy the socket (do not call wss.handleUpgrade or
lobbiesWss.handleUpgrade); otherwise continue routing to lobbiesWss or wss as
before. Ensure the check runs before the existing pathname === "/lobbies" /
endsWith("/lobbies") logic so mismatched worker-prefixed requests can’t be
accepted by the wrong WebSocketServer.
src/server/Worker.ts
Outdated
| // Periodically write this worker's public games to Redis | ||
| startGameInfoPolling(gm); | ||
|
|
||
| async function startGameInfoPolling(gameManager: GameManager) { | ||
| startPolling(async () => { | ||
| try { | ||
| const publicLobbyIDs = await getPublicLobbyIDs(); | ||
|
|
||
| for (const gameID of publicLobbyIDs) { | ||
| // Only update games that belong to this worker | ||
| if (config.workerIndex(gameID) !== workerId) continue; | ||
|
|
||
| const game = gameManager.game(gameID); | ||
| if (game === null) { | ||
| // Game no longer exists, clean up | ||
| await removeLobbyInfo(gameID); | ||
| await removePublicLobbyID(gameID); | ||
| continue; | ||
| } | ||
|
|
||
| const info = game.gameInfo(); | ||
| await setLobbyInfo({ | ||
| gameID: info.gameID, | ||
| numClients: info.clients?.length ?? 0, | ||
| msUntilStart: info.msUntilStart, | ||
| gameConfig: info.gameConfig, | ||
| updatedAt: Date.now(), | ||
| }); | ||
|
|
||
| // Remove from public lobbies if game started or is full | ||
| if (info.msUntilStart !== undefined && info.msUntilStart <= 250) { | ||
| await removePublicLobbyID(gameID); | ||
| await removeLobbyInfo(gameID); | ||
| } | ||
| } |
There was a problem hiding this comment.
Remove full lobbies from Redis set to avoid scheduler stall.
Full lobbies are filtered out for clients, but the public lobby ID remains in Redis, so the master may stop scheduling even when no joinable lobbies exist. Remove IDs when the lobby is full.
🛠️ Suggested fix
const info = game.gameInfo();
- await setLobbyInfo({
- gameID: info.gameID,
- numClients: info.clients?.length ?? 0,
- msUntilStart: info.msUntilStart,
- gameConfig: info.gameConfig,
- updatedAt: Date.now(),
- });
+ const numClients = info.clients?.length ?? 0;
+ const maxPlayers = info.gameConfig?.maxPlayers;
+ const isFull =
+ maxPlayers !== undefined && numClients >= maxPlayers;
+
+ await setLobbyInfo({
+ gameID: info.gameID,
+ numClients,
+ msUntilStart: info.msUntilStart,
+ gameConfig: info.gameConfig,
+ updatedAt: Date.now(),
+ });
// Remove from public lobbies if game started or is full
- if (info.msUntilStart !== undefined && info.msUntilStart <= 250) {
+ if (
+ (info.msUntilStart !== undefined && info.msUntilStart <= 250) ||
+ isFull
+ ) {
await removePublicLobbyID(gameID);
await removeLobbyInfo(gameID);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Periodically write this worker's public games to Redis | |
| startGameInfoPolling(gm); | |
| async function startGameInfoPolling(gameManager: GameManager) { | |
| startPolling(async () => { | |
| try { | |
| const publicLobbyIDs = await getPublicLobbyIDs(); | |
| for (const gameID of publicLobbyIDs) { | |
| // Only update games that belong to this worker | |
| if (config.workerIndex(gameID) !== workerId) continue; | |
| const game = gameManager.game(gameID); | |
| if (game === null) { | |
| // Game no longer exists, clean up | |
| await removeLobbyInfo(gameID); | |
| await removePublicLobbyID(gameID); | |
| continue; | |
| } | |
| const info = game.gameInfo(); | |
| await setLobbyInfo({ | |
| gameID: info.gameID, | |
| numClients: info.clients?.length ?? 0, | |
| msUntilStart: info.msUntilStart, | |
| gameConfig: info.gameConfig, | |
| updatedAt: Date.now(), | |
| }); | |
| // Remove from public lobbies if game started or is full | |
| if (info.msUntilStart !== undefined && info.msUntilStart <= 250) { | |
| await removePublicLobbyID(gameID); | |
| await removeLobbyInfo(gameID); | |
| } | |
| } | |
| // Periodically write this worker's public games to Redis | |
| startGameInfoPolling(gm); | |
| async function startGameInfoPolling(gameManager: GameManager) { | |
| startPolling(async () => { | |
| try { | |
| const publicLobbyIDs = await getPublicLobbyIDs(); | |
| for (const gameID of publicLobbyIDs) { | |
| // Only update games that belong to this worker | |
| if (config.workerIndex(gameID) !== workerId) continue; | |
| const game = gameManager.game(gameID); | |
| if (game === null) { | |
| // Game no longer exists, clean up | |
| await removeLobbyInfo(gameID); | |
| await removePublicLobbyID(gameID); | |
| continue; | |
| } | |
| const info = game.gameInfo(); | |
| const numClients = info.clients?.length ?? 0; | |
| const maxPlayers = info.gameConfig?.maxPlayers; | |
| const isFull = | |
| maxPlayers !== undefined && numClients >= maxPlayers; | |
| await setLobbyInfo({ | |
| gameID: info.gameID, | |
| numClients, | |
| msUntilStart: info.msUntilStart, | |
| gameConfig: info.gameConfig, | |
| updatedAt: Date.now(), | |
| }); | |
| // Remove from public lobbies if game started or is full | |
| if ( | |
| (info.msUntilStart !== undefined && info.msUntilStart <= 250) || | |
| isFull | |
| ) { | |
| await removePublicLobbyID(gameID); | |
| await removeLobbyInfo(gameID); | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@src/server/Worker.ts` around lines 172 - 206, startGameInfoPolling is not
removing full lobbies from Redis, causing the master to keep stale public IDs;
after obtaining info = game.gameInfo() check whether the lobby is full (e.g.,
info.clients?.length >= info.gameConfig?.maxPlayers or other capacity field on
gameConfig) and if so call removePublicLobbyID(gameID) and
removeLobbyInfo(gameID) (same as the existing removal for started lobbies).
Update the polling loop in startGameInfoPolling / startPolling right after
setLobbyInfo to detect full lobbies and remove their IDs so Redis no longer
advertises joinable lobbies from this worker.
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Dockerfile (1)
10-11: Pipeline failure: missing build tools for redis-memory-serverThe
npm cistep fails becauseredis-memory-server(devDependency) compiles Redis from source during postinstall. The build stage lacksmakeand other build tools.Add
build-essentialto the build stage before runningnpm ci:🔧 Proposed fix
# Build stage - install ALL dependencies and build FROM base AS build ENV HUSKY=0 +# Install build tools needed for native dependencies +RUN apt-get update && apt-get install -y build-essential && rm -rf /var/lib/apt/lists/* # Copy package files first for better caching COPY package*.json ./ RUN --mount=type=cache,target=/root/.npm \ npm ci
🤖 Fix all issues with AI agents
In `@package.json`:
- Line 78: The package.json dependency for "redis-memory-server" is set to an
invalid version ^0.15.0; update the entry for "redis-memory-server" to the
latest available 0.14.1 (e.g., change "redis-memory-server": "^0.15.0" to
"redis-memory-server": "^0.14.1") so npm/yarn can resolve it while keeping
"ioredis" ^5.9.2 unchanged.
In `@src/client/LobbySocket.ts`:
- Line 1: The start() method in LobbySocket currently calls
getServerConfigFromClient and lets errors propagate, preventing lobby
connections when /api/env fails; update start() (and similar calls around the
40-45 region) to catch errors from getServerConfigFromClient, log the error via
the existing logger, and fall back to a sensible default config/path (e.g.,
default server path or empty config) so lobby updates still start; ensure the
catch only supplies the fallback and does not suppress other unrelated errors.
- Around line 12-15: getRandomWorkerPath can produce "/wNaN" when numWorkers is
0 or invalid; update the function (getRandomWorkerPath) to validate numWorkers
(e.g., if not a finite positive integer) and use a safe fallback index (such as
0) or normalize with Math.max(1, Math.floor(...)) so the returned path is always
a valid "/w{n}" string; ensure the same guard/normalization is applied to any
other callers that compute workerIndex in this module.
In `@src/server/RedisClient.ts`:
- Around line 39-60: The current production branch sets redisReady =
Promise.resolve() immediately, which can let callers proceed before the Redis
client actually connects; change this to set redisReady to a Promise that
resolves when the Redis client's "connect" event fires and rejects (or rejects
and logs) on an "error" (and optionally "end") event so callers await a real
connection; update the block where redis is created (the Redis(...) constructor
and the redis.on handlers) to create this promise, attach resolve in
redis.on("connect") and reject in redis.on("error"), and ensure existing
log.info/log.error calls remain but no longer prematurely mark readiness.
In `@src/server/Worker.ts`:
- Line 33: The WebSocket upgrade handler currently only checks
pathname.endsWith("/lobbies") and thus skips validating the /w{workerId} prefix
that the HTTP middleware enforces; update the upgrade handling logic (where
pathname is checked, near the code that uses WorkerLobbyService) to parse and
strip a leading /w{workerId} segment and verify it equals this.id (e.g., compare
`/w${this.id}`), returning/ignoring the upgrade if it mismatches, so WebSocket
routing matches the HTTP middleware behavior.
🧹 Nitpick comments (7)
src/server/RedisClient.ts (3)
154-161: Side effect in read operation:getAllLobbyInfodeletes stale entriesThis function both reads and deletes data. This can cause issues:
- Unexpected behavior for callers expecting a pure read
- Multiple concurrent calls may cause race conditions
- Harder to reason about and test
Consider separating cleanup into a dedicated function or polling loop.
♻️ Suggested approach
export async function getAllLobbyInfo(): Promise<LobbyInfo[]> { await redisReady; const allInfo = await redis.hgetall(LOBBY_INFO_KEY); const lobbies: LobbyInfo[] = []; const now = Date.now(); + const staleKeys: string[] = []; for (const [gameID, value] of Object.entries(allInfo)) { try { const info = JSON.parse(value) as LobbyInfo; // Skip stale entries (older than 5 seconds) if (now - info.updatedAt > 5000) { - await redis.hdel(LOBBY_INFO_KEY, gameID); + staleKeys.push(gameID); continue; } lobbies.push(info); } catch (e) { log.error(`Failed to parse lobby info ${gameID}:`, e); + staleKeys.push(gameID); } } + // Clean up stale entries in background (fire-and-forget) + if (staleKeys.length > 0) { + redis.hdel(LOBBY_INFO_KEY, ...staleKeys).catch((err) => { + log.error("Failed to clean up stale lobby entries:", err); + }); + } + return lobbies; }
12-38: Consider extracting shared Redis configuration to reduce duplicationThe Redis client options and event handlers are duplicated between dev and prod modes. Extract common logic:
♻️ Example refactor
function createRedisClient(host: string, port: number): Redis { const client = new Redis({ host, port, maxRetriesPerRequest: 3, retryStrategy(times) { return Math.min(times * 50, 2000); }, }); client.on("error", (err) => { log.error("Redis connection error:", err); }); client.on("connect", () => { log.info(`Connected to Redis at ${host}:${port}`); }); return client; }
130-136: Use typedGameConfiginstead ofunknownforgameConfigThe
LobbyInfointerface usesgameConfig?: unknown, butGameConfigis already imported and used elsewhere. For type safety:export interface LobbyInfo { gameID: string; numClients: number; msUntilStart?: number; - gameConfig?: unknown; + gameConfig?: GameConfig; updatedAt: number; }src/server/Master.ts (1)
176-188: Consider cleanup on schedule failureIf
addPendingGamesucceeds butaddPublicLobbyIDfails, you'll have an orphan pending game in Redis. Consider cleanup:♻️ Suggested fix
async function schedulePublicGame() { const gameID = generateID(); try { const gameConfig = await playlist.gameConfig(); await addPendingGame(gameID, gameConfig); - await addPublicLobbyID(gameID); - log.info(`Scheduled public game ${gameID} via Redis`); + try { + await addPublicLobbyID(gameID); + log.info(`Scheduled public game ${gameID} via Redis`); + } catch (error) { + // Rollback pending game if public lobby ID fails + await removePendingGame(gameID).catch(() => {}); + throw error; + } } catch (error) { log.error(`Failed to schedule public game ${gameID}:`, error); throw error; } }You'll need to import
removePendingGamefrom./RedisClient.src/server/WorkerLobbyService.ts (3)
52-65: URL path matching may be too broadThe check
pathname.endsWith("/lobbies")would match paths like/foo/lobbiesor/api/lobbies. If those paths should go togameWss, this could cause issues.♻️ Suggested fix for stricter matching
private setupUpgradeHandler() { this.server.on("upgrade", (request, socket, head) => { const pathname = request.url ?? ""; - if (pathname === "/lobbies" || pathname.endsWith("/lobbies")) { + // Match exactly "/lobbies" or "/<worker>/lobbies" pattern + if (pathname === "/lobbies" || /^\/\d+\/lobbies$/.test(pathname)) { this.lobbiesWss.handleUpgrade(request, socket, head, (ws) => { this.lobbiesWss.emit("connection", ws, request); }); } else {
144-151: Extract magic number and improve type handlingThe
250ms threshold appears in multiple places. Also, castinginfo.gameConfigto a partial object is fragile.♻️ Suggested improvements
// At module level or in a constants file const GAME_START_THRESHOLD_MS = 250; // In the filter: if (info.msUntilStart !== undefined && info.msUntilStart <= GAME_START_THRESHOLD_MS) return false; // For gameConfig, consider using the GameConfig type with optional fields // or use a type guard function
182-186: Duplicate cleanup logic for lobby removalLobby cleanup happens in two places:
- Lines 182-186: When
game === null- Lines 199-202: When
msUntilStart <= 250Consider extracting to a helper:
♻️ Example helper
private async cleanupLobby(gameID: string): Promise<void> { await removeLobbyInfo(gameID); await removePublicLobbyID(gameID); }Also applies to: 199-202
| "prettier-plugin-organize-imports": "^4.1.0", | ||
| "prettier-plugin-sh": "^0.17.4", | ||
| "protobufjs": "^7.5.3", | ||
| "redis-memory-server": "^0.15.0", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check the package.json file around the lines mentioned
head -120 package.json | tail -50Repository: openfrontio/OpenFrontIO
Length of output: 1694
🌐 Web query:
latest version of ioredis npm package 2025
💡 Result:
The latest ioredis release is v5.9.2 (released January 15, 2026). [1]
Sources:
[1] GitHub releases — redis/ioredis (v5.9.2).
🌐 Web query:
latest version of redis-memory-server npm package 2025
💡 Result:
The latest redis-memory-server npm version is 0.14.1 (as of January 20, 2026). [1][2]
Sources:
- Socket (package info) — shows v0.14.1. [1]
- jsDelivr package page — shows v0.14.1. [2]
Update redis-memory-server version—0.15.0 does not exist.
ioredis ^5.9.2 is correct (latest version). However, redis-memory-server ^0.15.0 is invalid. The latest available version is 0.14.1 (as of January 20, 2026). Change to ^0.14.1 to fix the dependency resolution failure.
Both packages are good choices: ioredis is the standard Node.js Redis client, and redis-memory-server provides isolated Redis instances for development and testing.
🤖 Prompt for AI Agents
In `@package.json` at line 78, The package.json dependency for
"redis-memory-server" is set to an invalid version ^0.15.0; update the entry for
"redis-memory-server" to the latest available 0.14.1 (e.g., change
"redis-memory-server": "^0.15.0" to "redis-memory-server": "^0.14.1") so
npm/yarn can resolve it while keeping "ioredis" ^5.9.2 unchanged.
| function getRandomWorkerPath(numWorkers: number): string { | ||
| const workerIndex = Math.floor(Math.random() * numWorkers); | ||
| return `/w${workerIndex}`; | ||
| } |
There was a problem hiding this comment.
Guard against numWorkers <= 0 to avoid /wNaN.
If numWorkers() is 0 or invalid, the path becomes /wNaN and both WS + HTTP fail. Add a safe fallback.
✅ Suggested fix
function getRandomWorkerPath(numWorkers: number): string {
- const workerIndex = Math.floor(Math.random() * numWorkers);
+ const safeCount =
+ Number.isFinite(numWorkers) && numWorkers > 0 ? Math.floor(numWorkers) : 1;
+ const workerIndex = Math.floor(Math.random() * safeCount);
return `/w${workerIndex}`;
}Also applies to: 23-23
🤖 Prompt for AI Agents
In `@src/client/LobbySocket.ts` around lines 12 - 15, getRandomWorkerPath can
produce "/wNaN" when numWorkers is 0 or invalid; update the function
(getRandomWorkerPath) to validate numWorkers (e.g., if not a finite positive
integer) and use a safe fallback index (such as 0) or normalize with Math.max(1,
Math.floor(...)) so the returned path is always a valid "/w{n}" string; ensure
the same guard/normalization is applied to any other callers that compute
workerIndex in this module.
| } else { | ||
| // In production, connect to real Redis | ||
| redis = new Redis({ | ||
| host: "127.0.0.1", | ||
| port: 6379, | ||
| maxRetriesPerRequest: 3, | ||
| retryStrategy(times) { | ||
| const delay = Math.min(times * 50, 2000); | ||
| return delay; | ||
| }, | ||
| }); | ||
|
|
||
| redis.on("error", (err) => { | ||
| log.error("Redis connection error:", err); | ||
| }); | ||
|
|
||
| redis.on("connect", () => { | ||
| log.info("Connected to Redis"); | ||
| }); | ||
|
|
||
| redisReady = Promise.resolve(); | ||
| } |
There was a problem hiding this comment.
Production Redis: redisReady resolves before connection is established
In production mode, redisReady = Promise.resolve() is set immediately, but the Redis connection happens asynchronously. If any Redis operation is called before the connect event fires, it may fail or behave unexpectedly.
Consider waiting for the actual connection:
🔧 Proposed fix
} else {
// In production, connect to real Redis
- redis = new Redis({
+ const prodRedis = new Redis({
host: "127.0.0.1",
port: 6379,
maxRetriesPerRequest: 3,
retryStrategy(times) {
const delay = Math.min(times * 50, 2000);
return delay;
},
});
+ redis = prodRedis;
- redis.on("error", (err) => {
+ prodRedis.on("error", (err) => {
log.error("Redis connection error:", err);
});
- redis.on("connect", () => {
+ prodRedis.on("connect", () => {
log.info("Connected to Redis");
});
- redisReady = Promise.resolve();
+ redisReady = new Promise<void>((resolve) => {
+ prodRedis.once("ready", () => {
+ resolve();
+ });
+ });
}🤖 Prompt for AI Agents
In `@src/server/RedisClient.ts` around lines 39 - 60, The current production
branch sets redisReady = Promise.resolve() immediately, which can let callers
proceed before the Redis client actually connects; change this to set redisReady
to a Promise that resolves when the Redis client's "connect" event fires and
rejects (or rejects and logs) on an "error" (and optionally "end") event so
callers await a real connection; update the block where redis is created (the
Redis(...) constructor and the redis.on handlers) to create this promise, attach
resolve in redis.on("connect") and reject in redis.on("error"), and ensure
existing log.info/log.error calls remain but no longer prematurely mark
readiness.
| import { startPolling } from "./PollingLoop"; | ||
| import { PrivilegeRefresher } from "./PrivilegeRefresher"; | ||
| import { verifyTurnstileToken } from "./Turnstile"; | ||
| import { WorkerLobbyService } from "./WorkerLobbyService"; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's see the structure and locate the relevant files
git ls-files | grep -E "(Worker|Lobby)" | head -20Repository: openfrontio/OpenFrontIO
Length of output: 538
🏁 Script executed:
#!/bin/bash
# Read Worker.ts to see context around lines 33 and 55-71
wc -l src/server/Worker.ts
head -80 src/server/Worker.ts | cat -nRepository: openfrontio/OpenFrontIO
Length of output: 3108
🏁 Script executed:
#!/bin/bash
# Search for upgrade handlers and WebSocket routing
rg -n "upgrade|wss|WebSocket" src/server/ -A 3Repository: openfrontio/OpenFrontIO
Length of output: 9052
🏁 Script executed:
#!/bin/bash
# Get the full setupUpgradeHandler method and surrounding context
sed -n '52,65p' src/server/WorkerLobbyService.ts | cat -nRepository: openfrontio/OpenFrontIO
Length of output: 682
🏁 Script executed:
#!/bin/bash
# Look for Express middleware that might handle /w{workerId} paths
rg -n "app\.use|router\.|/w" src/server/Worker.ts -B 2 -A 2Repository: openfrontio/OpenFrontIO
Length of output: 1536
🏁 Script executed:
#!/bin/bash
# Check if there's any middleware setup in Worker.ts
rg -n "middleware|use\(" src/server/Worker.ts -A 3Repository: openfrontio/OpenFrontIO
Length of output: 986
🏁 Script executed:
#!/bin/bash
# Get the complete middleware implementation to understand what it does
sed -n '83,106p' src/server/Worker.ts | cat -nRepository: openfrontio/OpenFrontIO
Length of output: 950
WebSocket upgrade handler does not validate worker ID—inconsistent with HTTP middleware.
The Express middleware validates /w{workerId} for HTTP requests (line 12: returns 404 on mismatch), but the WebSocket upgrade handler only checks pathname.endsWith("/lobbies") without validating the worker ID prefix. This means requests like /w2/lobbies will be routed correctly on the current worker even though HTTP would reject them.
Either:
- Strip and validate the
/w{workerId}prefix in the upgrade handler (matching HTTP behavior), or - Document that WebSocket upgrades intentionally accept any worker prefix for load balancing.
🤖 Prompt for AI Agents
In `@src/server/Worker.ts` at line 33, The WebSocket upgrade handler currently
only checks pathname.endsWith("/lobbies") and thus skips validating the
/w{workerId} prefix that the HTTP middleware enforces; update the upgrade
handling logic (where pathname is checked, near the code that uses
WorkerLobbyService) to parse and strip a leading /w{workerId} segment and verify
it equals this.id (e.g., compare `/w${this.id}`), returning/ignoring the upgrade
if it mismatches, so WebSocket routing matches the HTTP middleware behavior.
eed0c4f to
72b7f28
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/server/Master.ts (1)
144-148: Missing Redis connection info in worker restart.Restarted workers don't receive
REDIS_HOSTandREDIS_PORTenvironment variables. In dev mode withredis-memory-server(dynamic port), restarted workers will fail to connect to Redis.🐛 Suggested fix
// Restart the worker with the same ID const newWorker = cluster.fork({ WORKER_ID: workerId, ADMIN_TOKEN, INSTANCE_ID, + REDIS_HOST: process.env.REDIS_HOST, + REDIS_PORT: process.env.REDIS_PORT, });
🤖 Fix all issues with AI agents
In `@src/server/RedisClient.ts`:
- Around line 46-70: redisReady is being resolved immediately for workers
instead of waiting for the actual Redis connection; change the worker branch in
RedisClient.ts to create redisReady as a Promise that resolves when the Redis
client emits a successful connection/ready event (use redis.once("connect" or
"ready") to resolve) and rejects on "error" so callers wait for a real
connection; ensure you remove the immediate Promise.resolve() and tie resolution
to the same event handlers used for logging (redis.on/once) to avoid race
conditions.
In `@src/server/WorkerLobbyService.ts`:
- Around line 52-65: The upgrade handler in setupUpgradeHandler currently routes
solely on the "/lobbies" suffix and ignores a leading worker prefix (e.g.
"/w2"), allowing cross-worker connections; update the server.on("upgrade") logic
in setupUpgradeHandler to parse and validate an optional "/w<N>" prefix from
request.url (extract N with a regex like ^/w(\d+)(/|$)), compare it to this
worker's id (use the existing worker id field, e.g. this.workerId), and if the
prefix exists and does not match return an HTTP 404/close the socket
(destroy/end) instead of proceeding; if it matches or no prefix is present,
strip the prefix before checking for "/lobbies" and continue using
lobbiesWss.handleUpgrade or gameWss.handleUpgrade as before.
♻️ Duplicate comments (7)
src/server/RedisClient.ts (4)
162-168: UseGameConfiginstead ofunknownfor type safety.
GameConfigis already imported at line 3. Usingunknownforces unsafe type assertions at call sites.♻️ Suggested fix
export interface LobbyInfo { gameID: string; numClients: number; msUntilStart?: number; - gameConfig?: unknown; + gameConfig?: GameConfig; updatedAt: number; }
71-92: Production:redisReadyresolves before connection is established.In production mode,
redisReady = Promise.resolve()completes immediately, but Redis connection is async. Operations called before theconnectevent may fail.🛠️ Suggested fix
redis.on("connect", () => { log.info("Connected to Redis"); }); - redisReady = Promise.resolve(); + redisReady = new Promise<void>((resolve) => { + redis.once("ready", () => { + resolve(); + }); + }); }
123-130: Clean up corrupted pending-game records on parse failure.If JSON parsing fails, the bad entry stays in Redis forever and keeps logging errors. Delete it after logging.
🛠️ Suggested fix
try { games.push(JSON.parse(value) as PendingGame); } catch (e) { log.error(`Failed to parse pending game ${gameID}:`, e); + await redis.hdel(PENDING_GAMES_KEY, gameID); }
195-197: Clean up corrupted lobby-info records on parse failure.Stale entries get cleaned up (lines 190-192), but parse failures leave bad JSON in Redis forever.
🛠️ Suggested fix
} catch (e) { log.error(`Failed to parse lobby info ${gameID}:`, e); + await redis.hdel(LOBBY_INFO_KEY, gameID); }src/client/LobbySocket.ts (2)
12-15: Guard against invalidnumWorkersto avoid/wNaNpaths.If
numWorkers()returns 0, NaN, or a negative number,Math.floor(Math.random() * numWorkers)produces NaN or invalid indices. Add a fallback.🛠️ Suggested fix
function getRandomWorkerPath(numWorkers: number): string { + if (!Number.isFinite(numWorkers) || numWorkers < 1) { + console.warn("Invalid numWorkers, defaulting to /w0"); + return "/w0"; + } const workerIndex = Math.floor(Math.random() * numWorkers); return `/w${workerIndex}`; }
40-45: Handle config fetch errors so lobby updates still start.If
/api/envfails,start()throws beforeconnectWebSocket()runs, leaving the lobby completely dead. Wrap in try/catch and fall back to a default worker path.🛠️ Suggested fix
async start() { this.wsConnectionAttempts = 0; - // Get config to determine number of workers, then pick a random one - const config = await getServerConfigFromClient(); - this.workerPath = getRandomWorkerPath(config.numWorkers()); + try { + // Get config to determine number of workers, then pick a random one + const config = await getServerConfigFromClient(); + this.workerPath = getRandomWorkerPath(config.numWorkers()); + } catch (error) { + console.error("Failed to load server config, defaulting to /w0:", error); + this.workerPath = "/w0"; + } this.connectWebSocket(); }src/server/WorkerLobbyService.ts (1)
172-208: Full lobbies are not removed from the public lobby set.The filter in
startLobbyPollinghides full lobbies from clients, butstartGameInfoPollingonly removes lobbies whenmsUntilStart <= 250. Full lobbies remain in Redis, which can cause the master scheduler to think public lobbies exist when none are joinable.This was flagged in a previous review and appears unaddressed.
Suggested fix
const info = game.gameInfo(); + const numClients = info.clients?.length ?? 0; + const maxPlayers = (info.gameConfig as { maxPlayers?: number }) + ?.maxPlayers; + const isFull = + maxPlayers !== undefined && numClients >= maxPlayers; + await setLobbyInfo({ gameID: info.gameID, - numClients: info.clients?.length ?? 0, + numClients, msUntilStart: info.msUntilStart, gameConfig: info.gameConfig, updatedAt: Date.now(), }); // Remove from public lobbies if game started or is full - if (info.msUntilStart !== undefined && info.msUntilStart <= 250) { + if ( + (info.msUntilStart !== undefined && info.msUntilStart <= 250) || + isFull + ) { await removePublicLobbyID(gameID); await removeLobbyInfo(gameID); }
🧹 Nitpick comments (3)
src/server/WorkerLobbyService.ts (1)
136-170: Type assertion may hide mismatches.Line 161 uses
as GameInfowhich bypasses type checking. IfGameInfohas required fields not present in the mapped object, runtime errors may occur silently.Safer approach
.map( (info) => - ({ + { gameID: info.gameID, numClients: info.numClients, gameConfig: info.gameConfig, msUntilStart: info.msUntilStart, - }) as GameInfo, + } satisfies Partial<GameInfo>, );Or define a proper mapping function that returns
GameInfowith all required fields.src/server/Worker.ts (2)
77-82: Positional arguments withundefinedplaceholder reduce clarity.Using
undefinedas a placeholder for therefreshIntervalparameter makes the call harder to read. Named parameters via an options object would be clearer, but this is minor.Consider an options object (future improvement)
If
PrivilegeRefresheris refactored later, using an options object would improve readability:// Future pattern: new PrivilegeRefresher({ endpoint: config.jwtIssuer() + "/cosmetics.json", log, env: config.env(), });
556-558: Suppressing errors in Dev may hide issues during development.Silencing matchmaking poll errors in Dev environment means developers won't see network failures or misconfigurations. Consider logging at a lower level (debug/warn) instead of suppressing entirely.
Alternative: log at debug level in Dev
} catch (error) { - if (config.env() !== GameEnv.Dev) { - log.error(`Error polling matchmaking:`, error); - } + const level = config.env() === GameEnv.Dev ? "debug" : "error"; + log[level](`Error polling matchmaking:`, error); }
| } else { | ||
| // Worker: connect to Redis using env vars set by master | ||
| const host = process.env.REDIS_HOST ?? "127.0.0.1"; | ||
| const port = parseInt(process.env.REDIS_PORT ?? "6379"); | ||
|
|
||
| redis = new Redis({ | ||
| host, | ||
| port, | ||
| maxRetriesPerRequest: 3, | ||
| retryStrategy(times) { | ||
| const delay = Math.min(times * 50, 2000); | ||
| return delay; | ||
| }, | ||
| }); | ||
|
|
||
| redis.on("error", (err) => { | ||
| log.error("Redis connection error:", err); | ||
| }); | ||
|
|
||
| redis.on("connect", () => { | ||
| log.info(`Worker connected to Redis memory server at ${host}:${port}`); | ||
| }); | ||
|
|
||
| redisReady = Promise.resolve(); | ||
| } |
There was a problem hiding this comment.
Dev worker: redisReady also resolves before connection.
Same issue as production - the worker's redisReady resolves immediately at line 69 while the actual Redis connection happens asynchronously.
🛠️ Suggested fix
redis.on("connect", () => {
log.info(`Worker connected to Redis memory server at ${host}:${port}`);
});
- redisReady = Promise.resolve();
+ redisReady = new Promise<void>((resolve) => {
+ redis.once("ready", () => {
+ resolve();
+ });
+ });
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } else { | |
| // Worker: connect to Redis using env vars set by master | |
| const host = process.env.REDIS_HOST ?? "127.0.0.1"; | |
| const port = parseInt(process.env.REDIS_PORT ?? "6379"); | |
| redis = new Redis({ | |
| host, | |
| port, | |
| maxRetriesPerRequest: 3, | |
| retryStrategy(times) { | |
| const delay = Math.min(times * 50, 2000); | |
| return delay; | |
| }, | |
| }); | |
| redis.on("error", (err) => { | |
| log.error("Redis connection error:", err); | |
| }); | |
| redis.on("connect", () => { | |
| log.info(`Worker connected to Redis memory server at ${host}:${port}`); | |
| }); | |
| redisReady = Promise.resolve(); | |
| } | |
| } else { | |
| // Worker: connect to Redis using env vars set by master | |
| const host = process.env.REDIS_HOST ?? "127.0.0.1"; | |
| const port = parseInt(process.env.REDIS_PORT ?? "6379"); | |
| redis = new Redis({ | |
| host, | |
| port, | |
| maxRetriesPerRequest: 3, | |
| retryStrategy(times) { | |
| const delay = Math.min(times * 50, 2000); | |
| return delay; | |
| }, | |
| }); | |
| redis.on("error", (err) => { | |
| log.error("Redis connection error:", err); | |
| }); | |
| redis.on("connect", () => { | |
| log.info(`Worker connected to Redis memory server at ${host}:${port}`); | |
| }); | |
| redisReady = new Promise<void>((resolve) => { | |
| redis.once("ready", () => { | |
| resolve(); | |
| }); | |
| }); | |
| } |
🤖 Prompt for AI Agents
In `@src/server/RedisClient.ts` around lines 46 - 70, redisReady is being resolved
immediately for workers instead of waiting for the actual Redis connection;
change the worker branch in RedisClient.ts to create redisReady as a Promise
that resolves when the Redis client emits a successful connection/ready event
(use redis.once("connect" or "ready") to resolve) and rejects on "error" so
callers wait for a real connection; ensure you remove the immediate
Promise.resolve() and tie resolution to the same event handlers used for logging
(redis.on/once) to avoid race conditions.
| private setupUpgradeHandler() { | ||
| this.server.on("upgrade", (request, socket, head) => { | ||
| const pathname = request.url ?? ""; | ||
| if (pathname === "/lobbies" || pathname.endsWith("/lobbies")) { | ||
| this.lobbiesWss.handleUpgrade(request, socket, head, (ws) => { | ||
| this.lobbiesWss.emit("connection", ws, request); | ||
| }); | ||
| } else { | ||
| this.gameWss.handleUpgrade(request, socket, head, (ws) => { | ||
| this.gameWss.emit("connection", ws, request); | ||
| }); | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
WebSocket upgrade handler lacks worker prefix validation.
The upgrade handler routes based on /lobbies path but does not validate the /wX worker prefix. HTTP middleware in Worker.ts validates worker ID and returns 404 on mismatch, but WebSocket upgrades bypass this. A client connecting to /w2/lobbies will be accepted even if this is worker 0.
Suggested fix
private setupUpgradeHandler() {
this.server.on("upgrade", (request, socket, head) => {
const pathname = request.url ?? "";
+
+ // Validate worker prefix (matches HTTP middleware behavior)
+ const match = pathname.match(/^\/w(\d+)(\/.*)?$/);
+ if (match && parseInt(match[1]) !== this.workerId) {
+ socket.destroy();
+ return;
+ }
+
if (pathname === "/lobbies" || pathname.endsWith("/lobbies")) {
this.lobbiesWss.handleUpgrade(request, socket, head, (ws) => {
this.lobbiesWss.emit("connection", ws, request);
});
} else {
this.gameWss.handleUpgrade(request, socket, head, (ws) => {
this.gameWss.emit("connection", ws, request);
});
}
});
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private setupUpgradeHandler() { | |
| this.server.on("upgrade", (request, socket, head) => { | |
| const pathname = request.url ?? ""; | |
| if (pathname === "/lobbies" || pathname.endsWith("/lobbies")) { | |
| this.lobbiesWss.handleUpgrade(request, socket, head, (ws) => { | |
| this.lobbiesWss.emit("connection", ws, request); | |
| }); | |
| } else { | |
| this.gameWss.handleUpgrade(request, socket, head, (ws) => { | |
| this.gameWss.emit("connection", ws, request); | |
| }); | |
| } | |
| }); | |
| } | |
| private setupUpgradeHandler() { | |
| this.server.on("upgrade", (request, socket, head) => { | |
| const pathname = request.url ?? ""; | |
| // Validate worker prefix (matches HTTP middleware behavior) | |
| const match = pathname.match(/^\/w(\d+)(\/.*)?$/); | |
| if (match && parseInt(match[1]) !== this.workerId) { | |
| socket.destroy(); | |
| return; | |
| } | |
| if (pathname === "/lobbies" || pathname.endsWith("/lobbies")) { | |
| this.lobbiesWss.handleUpgrade(request, socket, head, (ws) => { | |
| this.lobbiesWss.emit("connection", ws, request); | |
| }); | |
| } else { | |
| this.gameWss.handleUpgrade(request, socket, head, (ws) => { | |
| this.gameWss.emit("connection", ws, request); | |
| }); | |
| } | |
| }); | |
| } |
🤖 Prompt for AI Agents
In `@src/server/WorkerLobbyService.ts` around lines 52 - 65, The upgrade handler
in setupUpgradeHandler currently routes solely on the "/lobbies" suffix and
ignores a leading worker prefix (e.g. "/w2"), allowing cross-worker connections;
update the server.on("upgrade") logic in setupUpgradeHandler to parse and
validate an optional "/w<N>" prefix from request.url (extract N with a regex
like ^/w(\d+)(/|$)), compare it to this worker's id (use the existing worker id
field, e.g. this.workerId), and if the prefix exists and does not match return
an HTTP 404/close the socket (destroy/end) instead of proceeding; if it matches
or no prefix is present, strip the prefix before checking for "/lobbies" and
continue using lobbiesWss.handleUpgrade or gameWss.handleUpgrade as before.
evanpelle
force-pushed
the
move-ws-worker
branch
from
72b7f28 to
fbfd09c
Compare
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)
src/server/Master.ts (1)
144-148: Restarted workers miss Redis connection info.When a worker crashes and restarts, the
cluster.fork()call does not passREDIS_HOSTandREDIS_PORT. The new worker will fall back to defaults (127.0.0.1:6379), which breaks in dev mode where Redis memory server runs on a different port.🐛 Suggested fix
// Restart the worker with the same ID const newWorker = cluster.fork({ WORKER_ID: workerId, ADMIN_TOKEN, INSTANCE_ID, + REDIS_HOST: redisInfo.host, + REDIS_PORT: String(redisInfo.port), });Note: You need to capture
redisInfoin a scope accessible to theexithandler, or re-read fromprocess.env.
♻️ Duplicate comments (7)
src/server/RedisClient.ts (5)
162-168: UseGameConfiginstead ofunknownforgameConfig.
GameConfigis already imported at line 3. Usingunknownforces type assertions at call sites and hides type errors.♻️ Suggested fix
export interface LobbyInfo { gameID: string; numClients: number; msUntilStart?: number; - gameConfig?: unknown; + gameConfig?: GameConfig; updatedAt: number; }
46-70: Dev worker:redisReadyresolves before connection is established.At line 69,
redisReady = Promise.resolve()resolves immediately, but Redis connection happens asynchronously. Any Redis operation called before theconnectevent fires may fail.🔧 Suggested fix
redis.on("connect", () => { log.info(`Worker connected to Redis memory server at ${host}:${port}`); }); - redisReady = Promise.resolve(); + redisReady = new Promise<void>((resolve) => { + redis.once("ready", () => { + resolve(); + }); + }); }
71-92: Production Redis:redisReadyresolves before connection is established.Same issue as dev worker. Line 91 resolves immediately, but
redis.on("connect")fires later. Early Redis calls may fail or behave unexpectedly.🔧 Suggested fix
redis.on("connect", () => { log.info("Connected to Redis"); }); - redisReady = Promise.resolve(); + redisReady = new Promise<void>((resolve) => { + redis.once("ready", () => { + resolve(); + }); + }); }
123-131: Clean up corrupted pending-game records on parse failure.If JSON parsing fails, the bad entry stays in Redis forever and keeps logging errors on every poll. Delete it after logging.
🐛 Suggested fix
try { games.push(JSON.parse(value) as PendingGame); } catch (e) { log.error(`Failed to parse pending game ${gameID}:`, e); + await redis.hdel(PENDING_GAMES_KEY, gameID); }
195-197: Clean up corrupted lobby-info records on parse failure.Same issue as pending games. Bad JSON stays in Redis and logs repeatedly. Delete it on failure.
🐛 Suggested fix
} catch (e) { log.error(`Failed to parse lobby info ${gameID}:`, e); + await redis.hdel(LOBBY_INFO_KEY, gameID); }src/server/WorkerLobbyService.ts (2)
52-65: WebSocket upgrade handler lacks worker prefix validation.The upgrade handler routes based on
/lobbiespath but does not validate the/wXworker prefix. HTTP middleware in Worker.ts validates worker ID and returns 404 on mismatch, but WebSocket upgrades bypass this. A client connecting to/w2/lobbieswill be accepted even if this is worker 0.🔧 Suggested fix
private setupUpgradeHandler() { this.server.on("upgrade", (request, socket, head) => { const pathname = request.url ?? ""; + + // Validate worker prefix (matches HTTP middleware behavior) + const match = pathname.match(/^\/w(\d+)(\/.*)?$/); + if (match && parseInt(match[1]) !== this.workerId) { + socket.destroy(); + return; + } + if (pathname === "/lobbies" || pathname.endsWith("/lobbies")) { this.lobbiesWss.handleUpgrade(request, socket, head, (ws) => { this.lobbiesWss.emit("connection", ws, request); }); } else { this.gameWss.handleUpgrade(request, socket, head, (ws) => { this.gameWss.emit("connection", ws, request); }); } }); }
188-202: Full lobbies not removed from Redis, causing scheduler stall.When a lobby reaches max capacity, it's filtered out for clients in
startLobbyPolling(lines 147-151), but its ID stays inpublic_lobby_ids. The master checksgetPublicLobbyIDs()and sees a lobby exists, so it won't schedule new games. Players see no joinable lobbies.🔧 Suggested fix
const info = game.gameInfo(); + const numClients = info.clients?.length ?? 0; + const maxPlayers = (info.gameConfig as { maxPlayers?: number })?.maxPlayers; + const isFull = maxPlayers !== undefined && numClients >= maxPlayers; + await setLobbyInfo({ gameID: info.gameID, - numClients: info.clients?.length ?? 0, + numClients, msUntilStart: info.msUntilStart, gameConfig: info.gameConfig, updatedAt: Date.now(), }); // Remove from public lobbies if game started or is full - if (info.msUntilStart !== undefined && info.msUntilStart <= 250) { + if ( + (info.msUntilStart !== undefined && info.msUntilStart <= 250) || + isFull + ) { await removePublicLobbyID(gameID); await removeLobbyInfo(gameID); }
🧹 Nitpick comments (2)
Dockerfile (1)
46-46: Adding redis-server supports the new distributed lobby system.This is fine for development and single-container deployments. For production at scale, consider using a managed Redis service or a separate Redis container for better reliability and monitoring.
src/server/WorkerLobbyService.ts (1)
143-152: Type cast needed due tounknownin LobbyInfo.The cast
info.gameConfig as { maxPlayers?: number }at line 146 is a workaround forLobbyInfo.gameConfigbeing typed asunknown. Once that interface usesGameConfig, this cast can be removed for better type safety.
If this PR fixes an issue, link it below. If not, delete these two lines.
Resolves #(issue number)
Description:
Describe the PR.
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
DISCORD_USERNAME