Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 9 additions & 9 deletions backend/modules/chess/src/rating.rs
Original file line number Diff line number Diff line change
Expand Up @@ -64,15 +64,15 @@ impl RatingService {
) -> Result<(i32, i32), ApiError> {
// Start a database transaction to ensure atomicity
let txn = db.begin().await
.map_err(|e| ApiError::DatabaseError(DbErr::Custom(format!("Failed to start transaction: {}", e))))?;
.map_err(ApiError::DatabaseError)?;

let result = Self::update_ratings_in_transaction(&txn, game_id, config).await;

match result {
Ok(ratings) => {
// Commit the transaction if everything succeeded
txn.commit().await
.map_err(|e| ApiError::DatabaseError(DbErr::Custom(format!("Failed to commit transaction: {}", e))))?;
.map_err(ApiError::DatabaseError)?;
Ok(ratings)
}
Err(e) => {
Expand All @@ -93,7 +93,7 @@ impl RatingService {
let game_model = game::Entity::find_by_id(game_id)
.one(txn)
.await
.map_err(|e| ApiError::DatabaseError(DbErr::Custom(format!("Failed to fetch game: {}", e))))?
.map_err(ApiError::DatabaseError)?
.ok_or_else(|| ApiError::NotFound("Game not found".to_string()))?;

// 2. Check if game is completed
Expand All @@ -118,13 +118,13 @@ impl RatingService {
let white_player = player::Entity::find_by_id(game_model.white_player)
.one(txn)
.await
.map_err(|e| ApiError::DatabaseError(DbErr::Custom(format!("Failed to fetch white player: {}", e))))?
.map_err(ApiError::DatabaseError)?
.ok_or_else(|| ApiError::NotFound("White player not found".to_string()))?;

let black_player = player::Entity::find_by_id(game_model.black_player)
.one(txn)
.await
.map_err(|e| ApiError::DatabaseError(DbErr::Custom(format!("Failed to fetch black player: {}", e))))?
.map_err(ApiError::DatabaseError)?
.ok_or_else(|| ApiError::NotFound("Black player not found".to_string()))?;

// 5. Calculate new ratings based on game outcome
Expand All @@ -150,10 +150,10 @@ impl RatingService {

// Execute both updates in the same transaction
white_active_model.update(txn).await
.map_err(|e| ApiError::DatabaseError(DbErr::Custom(format!("Failed to update white player rating: {}", e))))?;
.map_err(ApiError::DatabaseError)?;

black_active_model.update(txn).await
.map_err(|e| ApiError::DatabaseError(DbErr::Custom(format!("Failed to update black player rating: {}", e))))?;
.map_err(ApiError::DatabaseError)?;

Ok((new_white_rating, new_black_rating))
}
Expand Down Expand Up @@ -216,7 +216,7 @@ impl RatingService {
let player = player::Entity::find_by_id(player_id)
.one(db)
.await
.map_err(|e| ApiError::DatabaseError(DbErr::Custom(format!("Failed to fetch player: {}", e))))?
.map_err(ApiError::DatabaseError)?
.ok_or_else(|| ApiError::NotFound("Player not found".to_string()))?;

Ok(player.elo_rating)
Expand All @@ -238,7 +238,7 @@ impl RatingService {
};

active_model.update(db).await
.map_err(|e| ApiError::DatabaseError(DbErr::Custom(format!("Failed to update player rating: {}", e))))?;
.map_err(ApiError::DatabaseError)?;

Ok(())
}
Expand Down
2 changes: 2 additions & 0 deletions backend/src/socket/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ uuid = { version = "1.0", features = ["v4"] }
lazy_static = "1.4"
log = "0.4"
env_logger = "0.11"
redis = { version = "0.25", features = ["tokio-comp", "json"] }
dotenvy = "0.15"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Confirm dotenvy is never invoked in the crate.
rg -nP '\bdotenvy\b|\bdotenv\s*\(' --type=rust

Repository: NOVUS-X/XLMate

Length of output: 382


🏁 Script executed:

# First, find the exact location and contents of the socket module's Cargo.toml
find . -path "*/socket/Cargo.toml" -type f | head -5

Repository: NOVUS-X/XLMate

Length of output: 90


🏁 Script executed:

# Check if dotenvy appears in socket module's Cargo.toml
find . -path "*/socket/Cargo.toml" -type f -exec grep -H "dotenvy" {} \;

Repository: NOVUS-X/XLMate

Length of output: 107


🏁 Script executed:

# Look for main.rs and entrypoints in socket module
find . -path "*/socket/*" -name "main.rs" -o -path "*/socket/*" -name "lib.rs" | head -10

Repository: NOVUS-X/XLMate

Length of output: 115


🏁 Script executed:

# Search specifically in socket module for any dotenv/dotenvy calls
find . -path "*/socket/*.rs" -type f -exec rg -l "dotenvy|dotenv" {} \;

Repository: NOVUS-X/XLMate

Length of output: 40


🏁 Script executed:

# Read main.rs to see the actual code
wc -l ./backend/src/socket/main.rs

Repository: NOVUS-X/XLMate

Length of output: 90


🏁 Script executed:

# Read the main.rs file
cat -n ./backend/src/socket/main.rs

Repository: NOVUS-X/XLMate

Length of output: 1904


🏁 Script executed:

# Check if pubsub.rs exists and read it
find ./backend/src/socket -name "pubsub.rs" -type f

Repository: NOVUS-X/XLMate

Length of output: 89


🏁 Script executed:

# Read pubsub.rs to check for env::var usage
wc -l ./backend/src/socket/pubsub.rs

Repository: NOVUS-X/XLMate

Length of output: 92


🏁 Script executed:

# Read pubsub.rs
cat -n ./backend/src/socket/pubsub.rs

Repository: NOVUS-X/XLMate

Length of output: 2278


🏁 Script executed:

# Check the full Cargo.toml for context
cat -n ./backend/src/socket/Cargo.toml

Repository: NOVUS-X/XLMate

Length of output: 775


dotenvy is added but never initialized — .env will not be loaded.

std::env::var("REDIS_URL") in pubsub.rs (line 14) and std::env::var("BIND_ADDRESS") in main.rs (line 16) are read directly without ever calling dotenvy::dotenv(). As a result, this dependency is dead code and operators who place REDIS_URL or BIND_ADDRESS in a .env file will silently fall back to the hardcoded defaults (redis://127.0.0.1:6379 and 127.0.0.1:8080). Either invoke dotenvy::dotenv().ok(); once at the top of main(), or drop the dependency.

🛠️ Suggested fix in `main.rs`
 #[tokio::main]
 async fn main() -> Result<(), Box<dyn std::error::Error>> {
+    // Load environment variables from .env if present (no-op in production).
+    dotenvy::dotenv().ok();
+
     // Initialize the logger
     env_logger::init_from_env(env_logger::Env::default().default_filter_or("info"));
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@backend/src/socket/Cargo.toml` at line 24, The dotenvy crate is added but
never initialized, so env vars like REDIS_URL (used in pubsub.rs) and
BIND_ADDRESS (used in main.rs) in a .env file are ignored; fix by calling
dotenvy::dotenv().ok(); at the start of your application entry (inside main() in
main.rs) before any std::env::var reads so those variables are loaded, or if you
prefer to not use .env files, remove the dotenvy dependency from Cargo.toml and
rely only on environment variables/hardcoded defaults.


[dev-dependencies]
tokio-test = "0.4"
4 changes: 4 additions & 0 deletions backend/src/socket/game.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,15 +10,19 @@ const LATENCY_BUFFER_MS: u64 = 750;

type MessageSender = broadcast::Sender<ServerMessage>;

use crate::pubsub::RedisPubSub;

pub struct ServerState {
pub rooms: HashMap<String, Room>,
pub message_senders: HashMap<String, MessageSender>,
pub pubsub: Option<RedisPubSub>,
}

lazy_static::lazy_static! {
pub static ref GAME_STATE: Arc<Mutex<ServerState>> = Arc::new(Mutex::new(ServerState {
rooms: HashMap::new(),
message_senders: HashMap::new(),
pubsub: None,
}));
Comment on lines +13 to 26

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Confirm RedisPubSub is never instantiated and publish_move is never called.
rg -nP '\bRedisPubSub::new\b|\.publish_move\s*\(|\.subscribe_to_room\s*\(' --type=rust
# Confirm no path assigns to ServerState.pubsub other than the None init.
rg -nP '\bpubsub\s*[:=]' --type=rust -C2

Repository: NOVUS-X/XLMate

Length of output: 1761


🏁 Script executed:

# Check if RedisPubSub is used anywhere (not just instantiated)
rg -n '\bRedisPubSub\b' --type=rust

# Check if subscribe_to_room is called anywhere
rg -n 'subscribe_to_room' --type=rust

# Check for any mutations or assignments to state.pubsub
rg -n 'state\.pubsub|GAME_STATE.*pubsub' --type=rust

# Check the send_move function implementation
fd 'game\.rs$' --type f | head -1 | xargs wc -l

Repository: NOVUS-X/XLMate

Length of output: 480


🏁 Script executed:

# Read the game.rs file to see send_move implementation
cat -n backend/src/socket/game.rs

# Search for any actual calls to subscribe_to_room
rg -n 'subscribe_to_room\s*\(' --type=rust

# Search for any code that might initialize pubsub or call RedisPubSub methods
rg -n 'publish_move|publish_' --type=rust

# Check main.rs for any initialization of RedisPubSub
fd 'main\.rs$' --type f | xargs cat -n 2>/dev/null | head -50

Repository: NOVUS-X/XLMate

Length of output: 23728


Critical: pubsub is wired into ServerState but never initialized or used — horizontal sync is not actually happening.

GAME_STATE initializes pubsub: None and there is no code path anywhere in the crate that constructs RedisPubSub::new() and assigns it to state.pubsub. Combined with the fact that send_move (lines 227–229), join_room (lines 127–131), leave_room (lines 253–255), accept_takeback (lines 370–372), reject_takeback (lines 403–405), and offer_takeback (lines 314–316) only broadcast on the in-process broadcast::Sender and never call pubsub.publish_move(...), the entire pubsub module is dead code in this PR.

This contradicts the PR objective ("moves on one server instance are published to a distributed channel, keeping room state synchronized across physical nodes") — two server instances running this build will not see each other's moves. Before merging, please:

  1. Initialize RedisPubSub at startup (e.g. in main.rs, behind a feature flag or env check), and store it on GAME_STATE.
  2. Call state.pubsub.as_ref().map(|p| p.publish_move(room_id, &response)) from the broadcasting paths in send_move / leave_room / join_room / accept_takeback / reject_takeback / offer_takeback.
  3. Spawn a subscriber task per room (or a single pattern subscriber on game:*) that re-emits incoming messages onto the local broadcast::Sender so remote moves reach connected websockets.

Without (2) and (3) the new module/dependencies have no runtime effect.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@backend/src/socket/game.rs` around lines 13 - 26, GAME_STATE.ServerState
currently holds pubsub: Option<RedisPubSub> but it’s never initialized or used;
to fix, initialize a RedisPubSub at startup (e.g. in main.rs behind env/feature
flag), store it into GAME_STATE.lock().pubsub, and in each broadcast path
(functions send_move, join_room, leave_room, accept_takeback, reject_takeback,
offer_takeback) add a call like state.pubsub.as_ref().map(|p|
p.publish_move(room_id, &response)) after sending on the local
broadcast::Sender; additionally spawn a background subscriber task (per-room or
a pattern subscriber like "game:*") that listens for Redis messages and re-emits
them onto the matching local broadcast::Sender so remote moves are delivered to
websocket clients.

}

Expand Down
1 change: 1 addition & 0 deletions backend/src/socket/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,3 +3,4 @@ pub mod game;
pub mod handlers;
pub mod models;
pub mod websocket;
pub mod pubsub;
52 changes: 52 additions & 0 deletions backend/src/socket/pubsub.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
use redis::{AsyncCommands, Client};
use std::sync::Arc;
use tokio::sync::Mutex;
use crate::models::ServerMessage;
use lazy_static::lazy_static;

#[derive(Clone)]
pub struct RedisPubSub {
client: Client,
connection: Arc<Mutex<redis::aio::Connection>>,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🌐 Web query:

redis-rs 0.25 MultiplexedConnection vs Connection concurrency and pipelining

💡 Result:

In redis-rs 0.25, MultiplexedConnection (from redis::aio) is the recommended async connection type, replacing the deprecated aio::Connection. It supports concurrency and pipelining on a single underlying TCP socket. MultiplexedConnection is cheaply cloneable and allows sending requests concurrently from multiple clones/tasks without waiting for responses (responses return in order). This enables high concurrency: clone the connection and use it across async tasks/threads for non-blocking commands. It natively supports pipelining via send_packed_commands (used internally by Pipeline::query_async). A bug affecting single-command pipelines on MultiplexedConnection was fixed in 0.25.2 (PR #1079). Regular Connection (sync) or deprecated async Connection lack multiplexing: they process one command at a time sequentially, requiring pools for concurrency. Key limitations: blocking commands (e.g., BLPOP) block the entire multiplexed connection; use separate connections/pools. Default response timeout is short (0.5s); adjust with set_response_timeout for longer blocks. Example usage: let client = redis::Client::open("redis://127.0.0.1/")?; let mut con = client.get_multiplexed_async_connection.await?; let _: = con.set("key", "value").await?; let val: String = con.get("key").await?; Clone con for concurrent use; Pipeline works via AsyncCommands.

Citations:


🏁 Script executed:

fd -e rs -p "pubsub" | head -20

Repository: NOVUS-X/XLMate

Length of output: 87


🏁 Script executed:

cat -n backend/src/socket/pubsub.rs

Repository: NOVUS-X/XLMate

Length of output: 2278


Use MultiplexedConnection instead of Arc<Mutex<Connection>> to eliminate lock contention on publishes.

redis::aio::Connection is single-request-at-a-time, and wrapping it in Arc<Mutex<…>> serializes every publish call across all rooms through the same lock, creating a contention point on the hot path. MultiplexedConnection (returned by client.get_multiplexed_async_connection()) is Clone + Send + Sync and pipelines requests internally, allowing you to clone the connection for concurrent use without the Arc<Mutex> overhead.

♻️ Proposed refactor
-use redis::{AsyncCommands, Client};
-use std::sync::Arc;
-use tokio::sync::Mutex;
+use redis::{aio::MultiplexedConnection, AsyncCommands, Client};
@@
 #[derive(Clone)]
 pub struct RedisPubSub {
     client: Client,
-    connection: Arc<Mutex<redis::aio::Connection>>,
+    connection: MultiplexedConnection,
 }
@@
-    pub async fn new() -> Result<Self, Box<dyn std::error::Error>> {
+    pub async fn new() -> Result<Self, Box<dyn std::error::Error>> {
         let client = Client::open(REDIS_URL.as_str())?;
-        let connection = client.get_async_connection().await?;
+        let connection = client.get_multiplexed_async_connection().await?;
         
-        Ok(Self {
-            client,
-            connection: Arc::new(Mutex::new(connection)),
-        })
+        Ok(Self { client, connection })
     }
@@
-    pub async fn publish_move(&self, room_id: &str, message: &ServerMessage) -> Result<(), Box<dyn std::error::Error>> {
+    pub async fn publish_move(&self, room_id: &str, message: &ServerMessage) -> Result<(), Box<dyn std::error::Error>> {
         let payload = serde_json::to_string(message)?;
-        let mut conn = self.connection.lock().await;
+        let mut conn = self.connection.clone();
         let _: () = conn.publish(format!("game:{}", room_id), payload).await?;
📝 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.

Suggested change
connection: Arc<Mutex<redis::aio::Connection>>,
connection: MultiplexedConnection,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@backend/src/socket/pubsub.rs` at line 10, Replace the shared
Arc<Mutex<redis::aio::Connection>> field with a
redis::aio::MultiplexedConnection to avoid serializing publishes: change the
struct field named connection to type MultiplexedConnection (remove Arc and
Mutex), obtain it via client.get_multiplexed_async_connection() where the
connection is created, update imports, and update all uses (e.g. any publish
calls that did self.connection.lock().await.publish(...)) to call
publish/execute directly on the cloned MultiplexedConnection (clone it where
concurrent use is needed) since it is Clone + Send + Sync and pipelines
internally. Ensure code compiles by removing async lock awaits and adjusting
types in function signatures that referenced Arc<Mutex<...>>.

}

lazy_static! {
static ref REDIS_URL: String = std::env::var("REDIS_URL").unwrap_or_else(|_| "redis://127.0.0.1:6379".to_string());
}
Comment on lines +13 to +15

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

REDIS_URL is captured once at first access and never re-resolves.

Combined with the missing dotenvy::dotenv() call (see Cargo.toml comment), tests or processes that set REDIS_URL after this lazy_static is first touched will silently use the default. Lower priority since env vars are typically set before main() runs, but worth being aware of when adding integration tests that set it programmatically.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@backend/src/socket/pubsub.rs` around lines 13 - 15, The REDIS_URL lazy_static
captures the environment once and won't pick up changes or dotenv-loaded vars;
remove the lazy_static! REDIS_URL and replace it with a small accessor function
(e.g., fn redis_url() -> String) that calls
std::env::var("REDIS_URL").unwrap_or_else(||
"redis://127.0.0.1:6379".to_string()) each time it's needed; also ensure
dotenvy::dotenv().ok() is invoked early in program/test setup (e.g., in main or
test harness) so dotenv files are loaded before calling redis_url().


impl RedisPubSub {
pub async fn new() -> Result<Self, Box<dyn std::error::Error>> {
let client = Client::open(REDIS_URL.as_str())?;
let connection = client.get_async_connection().await?;

Ok(Self {
client,
connection: Arc::new(Mutex::new(connection)),
})
}
Comment on lines +18 to +26

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🌐 Web query:

In Rust, why is Boxproblematic fortokio::spawn, and what is the recommended bound?

💡 Result:

Box is problematic for tokio::spawn because dyn std::error::Error does not implement Send. The tokio::spawn function requires the future passed to it to implement Future + Send + 'static, with its Output also being Send + 'static. When an async function returns Result<T, Box>, the generated future captures this return type. Since dyn Error lacks Send, the future cannot be Send, causing a compiler error like "(dyn std::error::Error + 'static) cannot be sent between threads safely". The recommended bound is Box<dyn std::error::Error + Send>. This ensures the trait object implements Send, making the future Send and compatible with tokio::spawn. Some sources also suggest + Send + Sync for additional safety (e.g., if the error needs to be shared across threads), but + Send is the minimal requirement for spawning.

Citations:


🏁 Script executed:

find . -name "pubsub.rs" -type f

Repository: NOVUS-X/XLMate

Length of output: 89


🏁 Script executed:

find . -name "game.rs" -type f

Repository: NOVUS-X/XLMate

Length of output: 236


🏁 Script executed:

wc -l ./backend/src/socket/pubsub.rs

Repository: NOVUS-X/XLMate

Length of output: 92


🏁 Script executed:

cat -n ./backend/src/socket/pubsub.rs | head -60

Repository: NOVUS-X/XLMate

Length of output: 2278


🏁 Script executed:

cat -n ./backend/src/socket/game.rs | head -100

Repository: NOVUS-X/XLMate

Length of output: 3854


🏁 Script executed:

rg "publish_move|subscribe_to_room" ./backend/src/socket/game.rs -A 3 -B 3

Repository: NOVUS-X/XLMate

Length of output: 40


🏁 Script executed:

rg "publish_move|subscribe_to_room" ./backend/ -r --type rust -A 2

Repository: NOVUS-X/XLMate

Length of output: 765


🏁 Script executed:

rg "publish_move|subscribe_to_room" ./backend/ --type-list | head -20

Repository: NOVUS-X/XLMate

Length of output: 448


🏁 Script executed:

rg "publish_move|subscribe_to_room" ./backend/

Repository: NOVUS-X/XLMate

Length of output: 358


🏁 Script executed:

rg "tokio::spawn|tokio::task::spawn" ./backend/ -A 5

Repository: NOVUS-X/XLMate

Length of output: 1561


Box<dyn std::error::Error> is not Send — will block use from tokio::spawn.

The trait object must be Send to cross async task boundaries (required by tokio::spawn). Without it, the future becomes !Send, causing compile errors when awaited in spawned tasks. Add the bound to all three method signatures.

🛠️ Suggested fix
-    pub async fn new() -> Result<Self, Box<dyn std::error::Error>> {
+    pub async fn new() -> Result<Self, Box<dyn std::error::Error + Send>> {
@@
-    pub async fn publish_move(&self, room_id: &str, message: &ServerMessage) -> Result<(), Box<dyn std::error::Error>> {
+    pub async fn publish_move(&self, room_id: &str, message: &ServerMessage) -> Result<(), Box<dyn std::error::Error + Send>> {
@@
-    pub async fn subscribe_to_room(&self, room_id: &str) -> Result<redis::aio::PubSub, Box<dyn std::error::Error>> {
+    pub async fn subscribe_to_room(&self, room_id: &str) -> Result<redis::aio::PubSub, Box<dyn std::error::Error + Send>> {
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@backend/src/socket/pubsub.rs` around lines 18 - 26, The error type returned
by new uses Box<dyn std::error::Error> which is not Send and will make futures
!Send (breaking tokio::spawn); update the error trait object to be sendable
(e.g. Box<dyn std::error::Error + Send + Sync + 'static>) in the new function
signature and in the other two related methods that return boxed errors so the
returned Result types are Send across async task boundaries (locate the new()
function and any publish/subscribe or similar methods that return Box<dyn
std::error::Error> and add the + Send + Sync + 'static bounds).


/**
* Publishes a message to a regional Redis channel to sync horizontally scaled nodes.
* This ensures that players on different server instances stay in sync.
*/
pub async fn publish_move(&self, room_id: &str, message: &ServerMessage) -> Result<(), Box<dyn std::error::Error>> {
let payload = serde_json::to_string(message)?;
let mut conn = self.connection.lock().await;
let _: () = conn.publish(format!("game:{}", room_id), payload).await?;

log::debug!("[Redis] Published move for room {}", room_id);
Ok(())
}

/**
* Subscribes to a specific room's updates.
* In a production horizontal setup, this would be used by each node to listen for events from others.
*/
pub async fn subscribe_to_room(&self, room_id: &str) -> Result<redis::aio::PubSub, Box<dyn std::error::Error>> {
let mut pubsub = self.client.get_async_pubsub().await?;
pubsub.subscribe(format!("game:{}", room_id)).await?;

log::info!("[Redis] Subscribed to horizontal updates for room {}", room_id);
Ok(pubsub)
}
Comment on lines +45 to +51

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

subscribe_to_room returns a PubSub but no caller consumes it — and per-room subscriptions don't scale.

Two concerns to consider before this is wired up:

  1. No consumer loop. Returning the PubSub to a caller that never reads .on_message() means subscribed messages are queued in the kernel buffer and eventually dropped. The module needs a long-lived task that drains the stream and re-publishes onto the local broadcast::Sender for that room.
  2. Per-room connections won't scale. Each call to subscribe_to_room opens a new TCP connection to Redis. With many concurrent rooms this exhausts file descriptors and Redis client slots. Prefer a single psubscribe("game:*") connection at startup, parse the room_id out of the channel name, and dispatch to the right local sender.

Worth addressing as part of the integration step (which is currently missing — see comment on game.rs).

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@backend/src/socket/pubsub.rs` around lines 45 - 51, subscribe_to_room
currently creates and returns a redis::aio::PubSub via
get_async_pubsub()/subscribe but no caller consumes messages, causing kernel
buffer growth, and creating one connection per room which won't scale; instead
implement a single long‑lived subscriber task (e.g. new
start_global_subscription or start_pubsub_worker) that uses psubscribe("game:*")
once, spawns an async loop that calls .on_message()/get_payload(), parses the
room_id from the channel name (channel format "game:{room_id}"), and republishes
each message onto the corresponding local broadcast::Sender for that room (look
up or create the sender in your room registry), and remove per-room
get_async_pubsub usage from subscribe_to_room (make it register/get a local
Sender only); this ensures messages are drained continuously and avoids one TCP
connection per room.

}
17 changes: 10 additions & 7 deletions frontend/app/layout.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import { MatchmakingProvider } from "@/context/matchmakingContext";
import { ToastProvider } from "@/components/ui/toast";
import { TransactionProvider } from "@/context/transactionContext";
import { ThemeProvider } from "next-themes";
import { WebSocketScalingProvider } from "@/context/webSocketScalingContext";

export const metadata: Metadata = {
title: "XLMate",
Expand All @@ -28,13 +29,15 @@ export default function RootLayout({
</a>
<ThemeProvider attribute="class" defaultTheme="dark" enableSystem disableTransitionOnChange>
<AppProvider>
<MatchmakingProvider>
<ToastProvider>
<TransactionProvider>
<ClientRoot>{children}</ClientRoot>
</TransactionProvider>
</ToastProvider>
</MatchmakingProvider>
<WebSocketScalingProvider>
<MatchmakingProvider>
<ToastProvider>
<TransactionProvider>
<ClientRoot>{children}</ClientRoot>
</TransactionProvider>
</ToastProvider>
</MatchmakingProvider>
</WebSocketScalingProvider>
</AppProvider>
</ThemeProvider>
</body>
Expand Down
92 changes: 92 additions & 0 deletions frontend/context/webSocketScalingContext.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,92 @@
"use client";

import React, { createContext, useContext, useState, useEffect, useCallback, useRef } from "react";

export type WebSocketNode = {
id: string;
url: string;
region: string;
load: number; // 0 to 1
};

type WebSocketScalingContextType = {
currentNode: WebSocketNode | null;
availableNodes: WebSocketNode[];
isScaling: boolean;
getOptimalNode: (gameId: string) => Promise<WebSocketNode>;
reportLoad: (nodeId: string, load: number) => void;
};

const WebSocketScalingContext = createContext<WebSocketScalingContextType | undefined>(undefined);

// Mock discovery service URL - in production this would be a real endpoint
const DISCOVERY_ENDPOINT = process.env.NEXT_PUBLIC_DISCOVERY_URL || "http://localhost:8000/v1/nodes/discovery";

export function WebSocketScalingProvider({ children }: { children: React.ReactNode }) {
const [currentNode, setCurrentNode] = useState<WebSocketNode | null>(null);
const [availableNodes, setAvailableNodes] = useState<WebSocketNode[]>([]);
const [isScaling, setIsScaling] = useState(false);

// Cache for game-to-node mapping to ensure session stickiness within horizontal scaling
const gameNodeMap = useRef<Map<string, WebSocketNode>>(new Map());

/**
* Fetches the most optimal WebSocket node based on regional proximity and server load.
* Implements horizontal load balancing logic on the client side.
*/
const getOptimalNode = useCallback(async (gameId: string): Promise<WebSocketNode> => {
// Check if we already have a sticky session for this game
if (gameNodeMap.current.has(gameId)) {
return gameNodeMap.current.get(gameId)!;
}

setIsScaling(true);
try {
// In a real implementation, we would fetch from DISCOVERY_ENDPOINT
// For now, we simulate a discovery process that returns available horizontal nodes
const mockNodes: WebSocketNode[] = [
{ id: "node-us-east-1", url: "ws://localhost:8080", region: "us-east", load: 0.2 },
{ id: "node-eu-west-1", url: "ws://localhost:8081", region: "eu-west", load: 0.4 },
{ id: "node-ap-south-1", url: "ws://localhost:8082", region: "ap-south", load: 0.1 },
];

setAvailableNodes(mockNodes);

// Sorting logic: prioritize lowest load
const sortedNodes = [...mockNodes].sort((a, b) => a.load - b.load);
const optimal = sortedNodes[0];
Comment on lines +33 to +57

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Docstring claims "regional proximity" but implementation only sorts by load.

Lines 33–35 describe selection "based on regional proximity and server load", but the only criterion at line 56 is a.load - b.load. Either drop the proximity claim from the docstring or factor proximity into the score (e.g., a precomputed region-affinity weight from the user's locale / detected region).

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@frontend/context/webSocketScalingContext.tsx` around lines 33 - 57, The
docstring for getOptimalNode promises selection by "regional proximity and
server load" but the implementation only sorts mockNodes by load; update
getOptimalNode to factor user-region affinity into the scoring (e.g., compute a
score = weightLoad * load + weightRegion * distancePenalty or regionAffinity and
pick min score) or change the comment to remove "regional proximity";
specifically modify getOptimalNode to compute region affinity from a detected
user region (or a passed-in preference) and combine it with load when sorting
the mockNodes, then keep the sticky session behavior using gameNodeMap and still
call setAvailableNodes with the node list.


gameNodeMap.current.set(gameId, optimal);
setCurrentNode(optimal);
return optimal;
} finally {
setIsScaling(false);
}
}, []);
Comment on lines +22 to +65

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Confirm DISCOVERY_ENDPOINT / NEXT_PUBLIC_DISCOVERY_URL really has no other consumer.
rg -nP -C2 'DISCOVERY_ENDPOINT|NEXT_PUBLIC_DISCOVERY_URL'

Repository: NOVUS-X/XLMate

Length of output: 1060


Critical: DISCOVERY_ENDPOINT is unused dead code; production builds will connect to hardcoded localhost WebSocket URLs.

DISCOVERY_ENDPOINT is declared on line 23 but never used — it only appears in comments (lines 22, 45). The discovery flow at lines 47–51 always returns hardcoded ws://localhost:808x mock nodes regardless of environment. In production, this means every client receives localhost URLs that (a) cannot resolve outside the local machine, (b) will be blocked as mixed content from HTTPS origins, and (c) use insecure ws:// instead of wss://. This breaks the PR's stated regional discovery and horizontal scaling objective.

Wrap the mock behind process.env.NODE_ENV !== 'production', and fetch from DISCOVERY_ENDPOINT in production with wss:// URLs.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@frontend/context/webSocketScalingContext.tsx` around lines 22 - 65, The
DISCOVERY_ENDPOINT constant is unused and getOptimalNode always returns
hardcoded mockNodes (ws://localhost...), causing production clients to get
invalid localhost/insecure URLs; update getOptimalNode to only use the hardcoded
mockNodes when process.env.NODE_ENV !== 'production' and in production perform a
fetch to DISCOVERY_ENDPOINT to retrieve real node data, convert/validate
returned node URLs to wss:// if needed, then call setAvailableNodes, update
gameNodeMap.current and setCurrentNode with the fetched/validated nodes, and
keep the existing fallback to mockNodes only for non-production or on fetch
failure.

Comment on lines +30 to +65

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Sticky session cache has no invalidation — failover is silently broken.

gameNodeMap.current.set(gameId, optimal) (line 59) is never cleared. Once a gameId is bound to a node:

  1. If that node becomes unhealthy or its load spikes after binding, every subsequent reconnect via useChessSocket (which calls getOptimalNode(gameId)) keeps returning the same dead node — defeating the PR's stated failover goal.
  2. The map grows for the SPA's lifetime (one entry per game ever observed), leaking memory in long sessions / matchmaking lobbies.
  3. reportLoad updates availableNodes but cannot influence sticky decisions, so live load telemetry has no effect.

Add an eviction path: on WebSocket close-with-error / health-check failure, expose an invalidateNode(gameId) (or releaseGame(gameId)) the hook can call to drop the binding before the next getOptimalNode call. Consider a TTL too.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@frontend/context/webSocketScalingContext.tsx` around lines 30 - 65, The
sticky cache in gameNodeMap used by getOptimalNode never evicts entries, causing
stale/dead node reuse and unbounded growth; add an eviction API (e.g., export
invalidateNode(gameId) or releaseGame(gameId)) that deletes
gameNodeMap.current.delete(gameId) and optionally clears related state
(setCurrentNode if it matched) and call it from the WebSocket teardown path
(close-with-error/health-check failure) in the useChessSocket hook; also augment
getOptimalNode to honor a TTL per entry (store timestamp alongside
WebSocketNode) and to re-evaluate using reportLoad/setAvailableNodes when the
entry is expired or the node is flagged unhealthy so live telemetry can
influence sticky decisions.


const reportLoad = useCallback((nodeId: string, load: number) => {
setAvailableNodes(prev =>
prev.map(node => node.id === nodeId ? { ...node, load } : node)
);
}, []);

return (
<WebSocketScalingContext.Provider value={{
currentNode,
availableNodes,
isScaling,
getOptimalNode,
reportLoad
}}>
{children}
</WebSocketScalingContext.Provider>
);
}

export const useWebSocketScaling = () => {
const context = useContext(WebSocketScalingContext);
if (!context) {
throw new Error("useWebSocketScaling must be used within a WebSocketScalingProvider");
}
return context;
};
42 changes: 24 additions & 18 deletions frontend/hook/useChessSocket.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { useEffect, useRef, useState, useCallback } from "react";
import { useWebSocketScaling } from "@/context/webSocketScalingContext";

export type ChessSocketStatus =
| "idle"
Expand All @@ -23,9 +24,6 @@ interface UseChessSocketReturn {
reconnect: () => void;
}

const API_BASE = process.env.NEXT_PUBLIC_API_URL ?? "http://localhost:8000";
const WS_BASE = API_BASE.replace(/^http/, "ws");

// Exponential backoff configuration
const MAX_RECONNECT_ATTEMPTS = 10;
const INITIAL_RECONNECT_DELAY = 1000; // 1 second
Expand All @@ -35,6 +33,7 @@ const RECONNECT_TIMEOUT = 3000; // 3 seconds timeout for reconnection
/**
* Custom hook for managing WebSocket connection to the chess game server.
* Handles connection, reconnection with exponential backoff, move queuing, and network state detection.
* Now supports horizontal load balancing and regional node discovery.
*
* @param gameId - The unique identifier for the chess game, or null if no game is active
* @returns Object containing WebSocket status, game state, and control functions
Expand All @@ -43,6 +42,8 @@ export function useChessSocket(gameId: string | null): UseChessSocketReturn {
const [status, setStatus] = useState<ChessSocketStatus>("idle");
const [lastOpponentMove, setLastOpponentMove] = useState<ChessMove | null>(null);

const { getOptimalNode } = useWebSocketScaling();

const wsRef = useRef<WebSocket | null>(null);
const reconnectAttemptsRef = useRef(0);
const reconnectTimeoutRef = useRef<NodeJS.Timeout | null>(null);
Expand All @@ -53,7 +54,6 @@ export function useChessSocket(gameId: string | null): UseChessSocketReturn {

/**
* Clears all pending reconnection timeouts and timers.
* Used to prevent multiple reconnection attempts from running simultaneously.
*/
const clearReconnectTimeout = useCallback(() => {
if (reconnectTimeoutRef.current) {
Expand All @@ -68,10 +68,6 @@ export function useChessSocket(gameId: string | null): UseChessSocketReturn {

/**
* Calculates the delay for the next reconnection attempt using exponential backoff with jitter.
* Jitter helps prevent thundering herd problem when multiple clients reconnect simultaneously.
*
* @param attempt - The current reconnection attempt number (0-indexed)
* @returns The delay in milliseconds before the next reconnection attempt
*/
const calculateReconnectDelay = useCallback((attempt: number): number => {
const baseDelay = INITIAL_RECONNECT_DELAY * Math.pow(2, attempt);
Expand All @@ -81,25 +77,27 @@ export function useChessSocket(gameId: string | null): UseChessSocketReturn {
}, []);

/**
* Creates a new WebSocket connection to the game server.
* Sets up event handlers for connection lifecycle, message handling, and error recovery.
* Creates a new WebSocket connection to the optimal game server node.
*
* @param attemptReconnect - Whether this connection attempt is a reconnection
* @returns The created WebSocket instance, or null if connection fails
*/
const createWebSocket = useCallback((attemptReconnect = false): WebSocket | null => {
const createWebSocket = useCallback(async (attemptReconnect = false) => {
if (!gameId) return null;

try {
const ws = new WebSocket(`${WS_BASE}/v1/games/${gameId}/ws`);
// Step 1: Discover the optimal horizontal node (Regional Load Balancing)
const node = await getOptimalNode(gameId);
const wsUrl = `${node.url}/v1/games/${gameId}/ws`;

const ws = new WebSocket(wsUrl);
wsRef.current = ws;

Comment on lines +84 to 94

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Async race: orphaned WebSockets when gameId changes during discovery.

createWebSocket is now async and assigns wsRef.current = ws synchronously after await getOptimalNode(...) resolves (lines 89–93). If the gameId effect (lines 314–336) re-runs while a prior getOptimalNode is still in-flight, the second call can resolve before/after the first and overwrite wsRef.current. The active flag at line 318 only closes the currently resolving ws, but by then wsRef.current may already point at a different, in‑flight connection (or the cleanup at line 327 will close the wrong one). Net effect: leaked/stale WebSocket instances whose onmessage/onclose handlers still mutate this hook's state (e.g., setting lastOpponentMove, scheduling reconnects).

Recommend tracking the in-flight token explicitly and bailing out before assigning wsRef.current / wiring listeners.

♻️ Suggested guard pattern (illustrative)
-  const createWebSocket = useCallback(async (attemptReconnect = false) => {
+  const connectionTokenRef = useRef(0);
+  const createWebSocket = useCallback(async (attemptReconnect = false) => {
     if (!gameId) return null;
+    const token = ++connectionTokenRef.current;

     try {
       const node = await getOptimalNode(gameId);
+      if (token !== connectionTokenRef.current) return null;
       const wsUrl = `${node.url}/v1/games/${gameId}/ws`;

       const ws = new WebSocket(wsUrl);
+      if (token !== connectionTokenRef.current) {
+        ws.close();
+        return null;
+      }
       wsRef.current = ws;

…and bump connectionTokenRef.current in disconnect / the gameId-effect cleanup so any pending discovery resolves into a no‑op.

Also applies to: 314-331

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@frontend/hook/useChessSocket.ts` around lines 84 - 94, The async
createWebSocket has a race where multiple in-flight getOptimalNode calls can
overwrite wsRef.current and leave orphaned sockets; fix by introducing and using
a per-call token (use connectionTokenRef) inside createWebSocket: capture a new
token before awaiting getOptimalNode, store it in a local variable, then after
the await check that connectionTokenRef.current still equals the local token
before assigning wsRef.current, adding event handlers, or scheduling reconnects;
also ensure disconnect (and the gameId effect cleanup) increments/changes
connectionTokenRef.current to invalidate any outstanding tokens so resolved
discoveries bail out and never wire listeners to stale sockets.

if (attemptReconnect) {
setStatus("reconnecting");
console.log(`[WebSocket] Attempting reconnection for game ${gameId}`);
console.log(`[WebSocket] Scaling: Attempting reconnection to ${node.id} (${node.region})`);
} else {
setStatus("connecting");
console.log(`[WebSocket] Connecting to game ${gameId}`);
console.log(`[WebSocket] Scaling: Connecting to ${node.id} for game ${gameId}`);
}

ws.onopen = () => {
Expand Down Expand Up @@ -314,13 +312,21 @@ export function useChessSocket(gameId: string | null): UseChessSocketReturn {

// Initialize WebSocket when gameId changes
useEffect(() => {
let active = true;
if (gameId) {
const ws = createWebSocket();
createWebSocket().then(ws => {
if (!active && ws) {
ws.close();
}
});

return () => {
active = false;
isManualDisconnectRef.current = true;
clearReconnectTimeout();
if (ws) {
ws.close();
if (wsRef.current) {
wsRef.current.close();
wsRef.current = null;
}
};
} else {
Expand Down
Loading