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

[andrew-welker]

This pull request updates the handling of legacy client registrations in the MobileControlWebsocketServer to improve robustness and support for duplicate join requests, and introduces the ability to update a client's ID after registration. The main changes include replacing the legacy FIFO queue with a timestamped registration bag, updating the client ID management logic, and adding a method to update a client's ID safely.

Legacy Client Registration Improvements:

• Replaced legacyClientIdQueues (FIFO queue per token) with legacyClientRegistrations, a ConcurrentBag storing (clientId, timestamp) tuples, allowing the server to always use the most recent registration for a given token. [1] [2]
• Updated the legacy client ID assignment logic in BuildUiClient to select and remove the most recent client ID for a token, instead of dequeuing the oldest, improving support for duplicate join requests.
• Adjusted cleanup logic to remove empty legacy registration bags instead of empty queues.

Client ID Management Enhancements:

• Added a new UpdateClientId method to MobileControlWebsocketServer to handle situations where a client's ID needs to be updated after registration, with checks for registration validity and safe updating in the uiClients collection.
• Added an UpdateId internal method to the UiClient class to allow the server to update the client ID in a controlled manner.

[Copilot]

Pull request overview

This PR addresses issues with subsequent join calls and client ID/WebSocket client mismatches in the mobile control WebSocket server. It replaces the FIFO queue-based approach for legacy client registrations with a timestamp-based system and adds functionality to update client IDs when mismatches are detected.

Changes:

• Replaced ConcurrentQueue with ConcurrentBag<(string, DateTime)> for legacy client registrations, using most-recent-first ordering instead of FIFO
• Added UpdateId method to UiClient class to allow internal ID updates
• Added UpdateClientId method to handle client ID mismatches during runtime

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 7 comments.

File	Description
UiClient.cs	Adds internal UpdateId method to enable server to update client IDs
MobileControlWebsocketServer.cs	Refactors legacy client registration from FIFO queue to timestamp-based bag, adds client ID update functionality, and updates cleanup logic

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

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]

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]

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]

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]

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]

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]

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);