Skip to content

fix: handle subsequent join calls and clientid/websocket client mismatches #1376

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ndorin merged 1 commit into from
Jan 21, 2026
Merged

fix: handle subsequent join calls and clientid/websocket client mismatches #1376

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
108 changes: 85 additions & 23 deletions src/PepperDash.Essentials.MobileControl/WebSocketServer/MobileControlWebsocketServer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,10 +69,11 @@ public class MobileControlWebsocketServer : EssentialsDevice
private readonly ConcurrentDictionary<string, string> pendingClientRegistrations = new ConcurrentDictionary<string, string>();

/// <summary>
/// Stores queues of pending client IDs per token for legacy clients (FIFO)
/// This ensures thread-safety when multiple legacy clients use the same token
/// Stores pending client registrations with timestamp for legacy clients
/// Key is token, Value is list of (clientId, timestamp) tuples
/// Most recent registration is used to handle duplicate join requests
Comment on lines +72 to +74
Copy link

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

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

The comment states "Most recent registration is used to handle duplicate join requests" but doesn't explain why the most recent is preferred over FIFO ordering, or what problem this solves. Since this is a significant change from the previous FIFO queue behavior, the comment should clarify the rationale for using most-recent-first ordering instead of first-in-first-out.

Suggested change
/// Stores pending client registrations with timestamp for legacy clients
/// Key is token, Value is list of (clientId, timestamp) tuples
/// Most recent registration is used to handle duplicate join requests
/// Stores pending client registrations with timestamp for legacy clients.
/// Key is token, Value is list of (clientId, timestamp) tuples.
/// Most recent registration is used to handle duplicate join requests so that the
/// newest client instance "wins" when multiple connections race for the same token.
/// This intentionally differs from FIFO behavior to avoid binding a token to a stale
/// or abandoned connection when a user reconnects or reloads the UI.

Copilot uses AI. Check for mistakes.
/// </summary>
private readonly ConcurrentDictionary<string, ConcurrentQueue<string>> legacyClientIdQueues = new ConcurrentDictionary<string, ConcurrentQueue<string>>();
private readonly ConcurrentDictionary<string, ConcurrentBag<(string clientId, DateTime timestamp)>> legacyClientRegistrations = new ConcurrentDictionary<string, ConcurrentBag<(string, DateTime)>>();

/// <summary>
/// Gets the collection of UI clients
Expand Down Expand Up @@ -736,15 +737,23 @@ private void GenerateClientTokenFromConsole(string s)

private UiClient BuildUiClient(string roomKey, JoinToken token, string key)
{
// Dequeue the next clientId for legacy client support (FIFO per token)
// Get the most recent unused clientId for this token (legacy support)
// New clients will override this ID in OnOpen with the validated query parameter value
var clientId = "pending";
if (legacyClientIdQueues.TryGetValue(key, out var queue) && queue.TryDequeue(out var dequeuedId))
if (legacyClientRegistrations.TryGetValue(key, out var registrations))
{
clientId = dequeuedId;
this.LogVerbose("Dequeued legacy clientId {clientId} for token {token}", clientId, key);
// Get most recent registration
var sorted = registrations.OrderByDescending(r => r.timestamp).ToList();
if (sorted.Any())
{
clientId = sorted.First().clientId;
// Remove it from the bag
var newBag = new ConcurrentBag<(string, DateTime)>(sorted.Skip(1));
Comment on lines +745 to +751
Copy link

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

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

Using ConcurrentBag with ToList() and OrderByDescending creates a full snapshot of the bag on every call. ConcurrentBag doesn't provide efficient sorted access, and this approach has O(n log n) complexity for sorting plus memory allocation for the list. If multiple join requests occur for the same token, this could become a performance bottleneck.

Consider using a more appropriate data structure such as ConcurrentQueue (with a different approach to handle duplicates) or implementing a custom thread-safe priority collection that maintains order efficiently.

Suggested change
// Get most recent registration
var sorted = registrations.OrderByDescending(r => r.timestamp).ToList();
if (sorted.Any())
{
clientId = sorted.First().clientId;
// Remove it from the bag
var newBag = new ConcurrentBag<(string, DateTime)>(sorted.Skip(1));
// Find the most recent registration in a single pass
(string clientId, DateTime timestamp) latest = default;
var hasLatest = false;
var remaining = new List<(string clientId, DateTime timestamp)>();
foreach (var registration in registrations)
{
if (!hasLatest || registration.timestamp > latest.timestamp)
{
if (hasLatest)
{
// Previous latest becomes part of the remaining registrations
remaining.Add(latest);
}
latest = registration;
hasLatest = true;
}
else
{
remaining.Add(registration);
}
}
if (hasLatest)
{
clientId = latest.clientId;
// Remove it from the bag by rebuilding without the latest registration
var newBag = new ConcurrentBag<(string, DateTime)>(remaining);

Copilot uses AI. Check for mistakes.
legacyClientRegistrations.TryUpdate(key, newBag, registrations);
this.LogVerbose("Assigned most recent legacy clientId {clientId} for token {token}", clientId, key);
Comment on lines +745 to +753
Copy link

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

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

There is a race condition in this logic. Between TryGetValue (line 743) and TryUpdate (line 752), another thread could modify the registrations bag, causing TryUpdate to fail silently. When TryUpdate fails, the clientId is used but not removed from the bag, leading to potential reuse of the same clientId.

Consider using a lock or redesigning this logic to handle the case where TryUpdate returns false. If TryUpdate fails, you should retry the operation or log an error, rather than continuing with a clientId that wasn't successfully removed from the bag.

Suggested change
// Get most recent registration
var sorted = registrations.OrderByDescending(r => r.timestamp).ToList();
if (sorted.Any())
{
clientId = sorted.First().clientId;
// Remove it from the bag
var newBag = new ConcurrentBag<(string, DateTime)>(sorted.Skip(1));
legacyClientRegistrations.TryUpdate(key, newBag, registrations);
this.LogVerbose("Assigned most recent legacy clientId {clientId} for token {token}", clientId, key);
// Attempt to atomically take the most recent registration for this key.
// Use a small retry loop to handle concurrent modifications.
const int maxAttempts = 3;
for (var attempt = 0; attempt < maxAttempts; attempt++)
{
// Snapshot current registrations
var snapshot = registrations.ToList();
if (!snapshot.Any())
{
break;
}
// Get most recent registration
var sorted = snapshot.OrderByDescending(r => r.timestamp).ToList();
var latest = sorted.First();
var newBag = new ConcurrentBag<(string, DateTime)>(sorted.Skip(1));
// Try to update the bag; if we succeed, we have exclusively consumed this clientId
if (legacyClientRegistrations.TryUpdate(key, newBag, registrations))
{
clientId = latest.clientId;
this.LogVerbose("Assigned most recent legacy clientId {clientId} for token {token}", clientId, key);
break;
}
// TryUpdate failed because another thread modified the value; re-read and retry
this.LogWarning("Failed to consume legacy clientId for token {token} on attempt {attempt}", key, attempt + 1);
if (!legacyClientRegistrations.TryGetValue(key, out registrations))
{
// Key was removed; nothing more to do
break;
}

Copilot uses AI. Check for mistakes.
}
}

var c = new UiClient($"uiclient-{key}-{roomKey}-{clientId}", clientId, token.Token, token.TouchpanelKey);
this.LogInformation("Constructing UiClient with key {key} and temporary ID (will be set from query param)", key);
c.Controller = _parent;
Expand All @@ -753,8 +762,8 @@ private UiClient BuildUiClient(string roomKey, JoinToken token, string key)
c.Server = this; // Give UiClient access to server for ID registration

// Don't add to uiClients yet - will be added in OnOpen after ID is set from query param
c.ConnectionClosed += (o, a) =>

c.ConnectionClosed += (o, a) =>
{
uiClients.TryRemove(a.ClientId, out _);
// Clean up any pending registrations for this token
Expand All @@ -765,11 +774,11 @@ private UiClient BuildUiClient(string roomKey, JoinToken token, string key)
{
pendingClientRegistrations.TryRemove(k, out _);
}
// Clean up legacy queue if empty
if (legacyClientIdQueues.TryGetValue(key, out var legacyQueue) && legacyQueue.IsEmpty)

// Clean up legacy registrations if empty
if (legacyClientRegistrations.TryGetValue(key, out var legacyBag) && legacyBag.IsEmpty)
Copy link

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

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

The variable name "legacyBag" is ambiguous as it could refer to the ConcurrentBag retrieved or a new bag. Consider renaming to "registrationsBag" or "currentRegistrations" to clarify that this refers to the existing collection retrieved from the dictionary.

Suggested change
if (legacyClientRegistrations.TryGetValue(key, out var legacyBag) && legacyBag.IsEmpty)
if (legacyClientRegistrations.TryGetValue(key, out var registrationsBag) && registrationsBag.IsEmpty)

Copilot uses AI. Check for mistakes.
{
legacyClientIdQueues.TryRemove(key, out _);
legacyClientRegistrations.TryRemove(key, out _);
}
};
return c;
Expand All @@ -785,7 +794,7 @@ private UiClient BuildUiClient(string roomKey, JoinToken token, string key)
public bool RegisterUiClient(UiClient client, string clientId, string tokenKey)
{
var registrationKey = $"{tokenKey}-{clientId}";

// Verify this clientId was generated during a join request for this token
if (!pendingClientRegistrations.TryRemove(registrationKey, out _))
{
Expand All @@ -799,11 +808,63 @@ public bool RegisterUiClient(UiClient client, string clientId, string tokenKey)
this.LogWarning("Replacing existing client with duplicate id {id}", id);
return client;
});

this.LogInformation("Successfully registered UiClient with ID {clientId} for token {token}", clientId, tokenKey);
return true;
}

/// <summary>
/// Updates a client's ID when a mismatch is detected between stored ID and message ID
/// </summary>
/// <param name="oldClientId">The current/old client ID</param>
/// <param name="newClientId">The new client ID from the message</param>
/// <param name="tokenKey">The token key for validation</param>
/// <returns>True if update successful, false otherwise</returns>
Comment on lines +817 to +822
Copy link

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

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

The documentation doesn't explain when or why a client ID mismatch would occur, or under what circumstances this method would be called. Since this is a new public API method, the documentation should provide context about the scenario this handles (e.g., "Called when a client reconnects with a different ID after a subsequent join request").

Suggested change
/// Updates a client's ID when a mismatch is detected between stored ID and message ID
/// </summary>
/// <param name="oldClientId">The current/old client ID</param>
/// <param name="newClientId">The new client ID from the message</param>
/// <param name="tokenKey">The token key for validation</param>
/// <returns>True if update successful, false otherwise</returns>
/// Updates a UI client's ID when a mismatch is detected between the server's stored ID and
/// the client ID presented in an incoming message.
/// </summary>
/// <param name="oldClientId">
/// The current/old client ID that the server has associated with the UI client connection.
/// This is typically the ID that was originally registered via <see cref="RegisterUiClient"/>.
/// </param>
/// <param name="newClientId">
/// The new client ID presented by the UI client in a message after a subsequent join request
/// (for example, when the client reconnects or refreshes and is issued a new ID).
/// This ID must have been previously generated and registered for the same <paramref name="tokenKey"/>.
/// </param>
/// <param name="tokenKey">
/// The token key used to validate that <paramref name="newClientId"/> corresponds to a pending
/// registration for this session.
/// </param>
/// <returns>
/// True if the client ID was successfully updated from <paramref name="oldClientId"/> to
/// <paramref name="newClientId"/>, or if both IDs are the same; false if validation fails or
/// the update cannot be performed.
/// </returns>
/// <remarks>
/// This method is intended to be called by higher-level message handling logic when a UI client
/// sends a message with a different client ID than the one currently tracked by the server, but
/// where that new ID has been issued as part of a subsequent join/registration flow for the same
/// token. It reconciles the server's state by moving the existing client instance to the new ID
/// after validating the new ID against <c>pendingClientRegistrations</c>.
/// </remarks>

Copilot uses AI. Check for mistakes.
public bool UpdateClientId(string oldClientId, string newClientId, string tokenKey)
{
if (string.IsNullOrEmpty(oldClientId) || string.IsNullOrEmpty(newClientId))
{
this.LogWarning("Cannot update client ID with null or empty values");
return false;
}

Copy link

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

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

The method validates that oldClientId and newClientId are not null or empty, but doesn't validate the tokenKey parameter. If tokenKey is null or empty, the registrationKey will still be constructed and used, potentially leading to unexpected behavior. Consider adding validation for tokenKey as well.

Suggested change
if (string.IsNullOrEmpty(tokenKey))
{
this.LogWarning("Cannot update client ID with null or empty token key");
return false;
}

Copilot uses AI. Check for mistakes.
if (oldClientId == newClientId)
{
return true; // No update needed
}

// Verify the new clientId was registered for this token
var registrationKey = $"{tokenKey}-{newClientId}";
if (!pendingClientRegistrations.TryRemove(registrationKey, out _))
{
this.LogWarning("Cannot update to unregistered clientId {newClientId} for token {token}", newClientId, tokenKey);
return false;
}

// Get the existing client
if (!uiClients.TryRemove(oldClientId, out var client))
{
this.LogWarning("Cannot find client with old ID {oldClientId}", oldClientId);
return false;
}

// Update the client's ID
client.UpdateId(newClientId);

// Re-add with new ID
if (!uiClients.TryAdd(newClientId, client))
{
// If add fails, try to restore old entry
uiClients.TryAdd(oldClientId, client);
client.UpdateId(oldClientId);
this.LogError("Failed to update client ID from {oldClientId} to {newClientId}", oldClientId, newClientId);
Comment on lines +857 to +860
Copy link

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

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

There is a potential race condition in the rollback logic. If TryAdd fails at line 858 (meaning another client with oldClientId was already added by another thread), the rollback call to UpdateId at line 859 will change the client's ID back to oldClientId even though this client instance is no longer in the uiClients dictionary with that ID. This creates an inconsistent state where the client object's internal ID doesn't match its key in the dictionary.

Consider whether rollback is appropriate here, or if the error handling should simply log the failure without attempting to restore the old state.

Suggested change
// If add fails, try to restore old entry
uiClients.TryAdd(oldClientId, client);
client.UpdateId(oldClientId);
this.LogError("Failed to update client ID from {oldClientId} to {newClientId}", oldClientId, newClientId);
// Failed to add under new ID; attempt best-effort rollback
this.LogError("Failed to update client ID from {oldClientId} to {newClientId}", oldClientId, newClientId);
// Only revert the client's ID if we successfully restore the old dictionary entry
if (uiClients.TryAdd(oldClientId, client))
{
client.UpdateId(oldClientId);
this.LogWarning("Reverted client ID from {newClientId} back to {oldClientId} after failed update", newClientId, oldClientId);
}
else
{
this.LogWarning("Could not restore client with ID {oldClientId} after failed update; the ID may have been claimed by another client", oldClientId);
}

Copilot uses AI. Check for mistakes.
return false;
}

this.LogInformation("Successfully updated client ID from {oldClientId} to {newClientId}", oldClientId, newClientId);
return true;
}

/// <summary>
/// Registers a UiClient using legacy flow (for backwards compatibility with older clients)
/// </summary>
Expand All @@ -821,7 +882,7 @@ public void RegisterLegacyUiClient(UiClient client)
this.LogWarning("Replacing existing client with duplicate id {id} (legacy flow)", id);
return client;
});

this.LogInformation("Successfully registered UiClient with ID {clientId} using legacy flow", client.Id);
}

Expand Down Expand Up @@ -1133,16 +1194,17 @@ private void HandleJoinRequest(HttpListenerRequest req, HttpListenerResponse res

// Generate a client ID for this join request
var clientId = $"{Utilities.GetNextClientId()}";

var now = DateTime.UtcNow;

// Store in pending registrations for new clients that send clientId via query param
var registrationKey = $"{token}-{clientId}";
pendingClientRegistrations.TryAdd(registrationKey, clientId);

// Also enqueue for legacy clients (thread-safe FIFO per token)
var queue = legacyClientIdQueues.GetOrAdd(token, _ => new ConcurrentQueue<string>());
queue.Enqueue(clientId);

this.LogVerbose("Assigning ClientId: {clientId} for token: {token}", clientId, token);
// For legacy clients, store with timestamp instead of FIFO queue
var legacyBag = legacyClientRegistrations.GetOrAdd(token, _ => new ConcurrentBag<(string, DateTime)>());
legacyBag.Add((clientId, now));

this.LogVerbose("Assigning ClientId: {clientId} for token: {token} at {timestamp}", clientId, token, now);

// Construct WebSocket URL with clientId query parameter
var wsProtocol = "ws";
Expand Down
9 changes: 9 additions & 0 deletions src/PepperDash.Essentials.MobileControl/WebSocketServer/UiClient.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,15 @@ public class UiClient : WebSocketBehavior, IKeyed
/// </summary>
public string Id { get; private set; }

/// <summary>
/// Updates the client ID - only accessible from within the assembly (e.g., by the server)
/// </summary>
/// <param name="newId">The new client ID</param>
internal void UpdateId(string newId)
{
Id = newId;
}

/// <summary>
/// Token associated with this client
/// </summary>
Expand Down
Loading