Add connection events and socket reconnect mechanism

[tudddorrr]

No description provided.

[claude]

🔍 Code Review: Connection Events and Socket Reconnect

I've reviewed PR #159 and identified several areas that could be improved:

---

🐛 Potential Bugs/Issues

1. Race Condition in OnConnectionRestored (PlayersAPI.cs:22-34)

• The async void event handler can cause fire-and-forget issues. If ResetConnection() or CreateSocketToken() throw exceptions, they won't be caught.
• Suggestion: Consider logging errors or using async Task pattern with proper error handling.

2. Redundant Event Handlers in Talo.cs

• Lines 235-243 define HandleConnectionLost() and HandleConnectionRestored() which duplicate functionality already present in InvokeConnectionLost() (line 17) and InvokeConnectionRestored() (line 22).
• Suggestion: Remove the redundant methods or consolidate them if there's a specific reason for the duplication.

3. Missing Null Check in HandlePostResponseHealthCheck (ContinuityManager.cs:128-154)

• The method doesn't verify that Talo.HealthCheck is initialized before calling GetLastStatus() or Ping().
• Suggestion: Add null checks to prevent potential NullReferenceException.

4. Socket State Synchronization Issue (TaloSocket.cs:131-137)

• The removed identity check in ResetConnection() means the socket will attempt reconnection even when not identified. The new code in PlayersAPI.OnConnectionRestored() calls ResetConnection() but socket state flags (socketAuthenticated, identified) are reset before checking if identity exists.
• Suggestion: Consider maintaining the identity check or ensuring proper state management across the reconnection flow.

---

⚡ Performance Considerations

1. Sequential Health Checks After Every Request (BaseAPI.cs:104)

• HandlePostResponseHealthCheck() is called after every API request, potentially triggering additional health check pings. This creates extra network overhead.
• Suggestion: Implement a debounce mechanism or rate-limiting to avoid excessive health checks.

2. Await in Hot Path (ContinuityManager.cs:143)

• The health check ping in HandlePostResponseHealthCheck() awaits in the request flow, adding latency to every successful/failed request.
• Suggestion: Consider making this non-blocking or using a background task with a state flag.

---

🔐 Security Concerns

1. Socket Token Error Handling (PlayersAPI.cs:236-238)

• When CreateSocketToken() fails, it returns an empty string and only logs a warning. The calling code doesn't check for empty tokens before calling SetSocketToken().
• Suggestion: Validate the token before setting it or handle empty token cases explicitly in the caller.

---

🔄 Backwards Compatibility

1. Public Event Addition (Talo.cs:10-11)

• Adding OnConnectionLost and OnConnectionRestored static events is backwards compatible, but existing subscribers should be aware these events may fire during normal operation (e.g., network fluctuations).
• Suggestion: Consider documenting the event behavior, especially regarding edge cases like rapid connection state changes.

2. HealthCheckAPI.GetLastStatus() State Change

• The lastHealthCheckStatus field introduces state that persists across health checks. Existing code that relies on Ping() behavior might not expect this new state tracking.
• Suggestion: Verify that the new state-based event firing doesn't cause issues with existing health check consumers.

3. Removed Identity Check in ResetConnection()

• This changes the method's preconditions, allowing it to be called without an identified player. Code that previously relied on the identity check behavior may now encounter unexpected states.
• Suggestion: Review all call sites to ensure they handle the new behavior correctly.

---

Overall: The PR adds valuable connection management features, but would benefit from addressing the race conditions, reducing performance overhead, and clarifying the changed behavior in ResetConnection().

[claude]

Pull Request Review: Connection Events and Socket Reconnect

---

🐛 Potential Bugs or Issues

Race Condition in Socket Reconnection

PlayersAPI.cs:22-30 - The OnConnectionRestored handler has a potential race condition. If CreateSocketToken() fails or returns empty, the socket will have been reset but won't be properly authenticated. This could leave the socket in an inconsistent state.

Suggestion: Add error handling to restore the socket to its previous state if token creation fails, or ensure the socket remains closed until authentication succeeds.

Event Subscription Memory Leak

PlayersAPI.cs:17 - The PlayersAPI constructor subscribes to Talo.OnConnectionRestored but never unsubscribes. Since PlayersAPI is created in a static constructor in Talo.cs:172, this may not be an immediate issue, but it violates the subscription/unsubscription pattern.

Suggestion: Consider implementing IDisposable or adding cleanup logic if APIs can be recreated during runtime.

Removed Identity Check Could Break Functionality

TaloSocket.cs:131-136 - The removal of the identity check in ResetConnection() means the socket can be reset even when no player is identified. This could cause issues when PlayersAPI.OnConnectionRestored tries to create a socket token without a valid identity, as CreateSocketToken() calls Talo.IdentityCheck() which will throw an exception.

Suggestion: Either restore the identity check or ensure OnConnectionRestored handlers gracefully handle the case where no player is identified.

Redundant Health Check Calls

ContinuityManager.cs:141-143 & ContinuityManager.cs:149-152 - Every successful request when in failed state, and every failed request when not in failed state, triggers a health check. For high-frequency API calls, this could create a cascade of health checks.

Suggestion: Add debouncing/throttling logic to prevent multiple simultaneous health checks, or track if a health check is already in progress.

---

🏗️ Code Quality and Best Practices

Duplicate Methods in Talo.cs

Talo.cs:17-24 and Talo.cs:235-243 - The methods InvokeConnectionLost(), InvokeConnectionRestored(), HandleConnectionLost(), and HandleConnectionRestored() are duplicated. The Handle* methods are never called and serve the exact same purpose as the Invoke* methods.

Suggestion: Remove the unused Handle* methods to reduce code duplication.

Async Void Event Handler

PlayersAPI.cs:22 - The OnConnectionRestored method is async void, which is generally discouraged except for event handlers. However, since this isn't a true event handler (it's a lambda/method subscription), exceptions thrown here won't be catchable by the caller.

Suggestion: Consider logging exceptions within this method or using a fire-and-forget pattern with proper exception handling.

Magic String in URL Check

ContinuityManager.cs:130 - Using url.Contains("/health-check") is fragile and could match unintended URLs (e.g., a custom endpoint with "health-check" in its name).

Suggestion: Use url.EndsWith("/health-check") or compare against the full path to be more specific.

---

⚡ Performance Considerations

Health Check Called on Every Request

BaseAPI.cs:104 - HandlePostResponseHealthCheck is called after every single API request, regardless of success or failure. This adds overhead to every request, even when the connection state hasn't changed.

Suggestion: Consider implementing a state tracking mechanism to only trigger health checks when transitioning between states, rather than checking on every request.

Sequential Await in Reconnection Flow

PlayersAPI.cs:23-24 - The socket reset and token creation happen sequentially. The socket reset might take time to close and reopen the connection, blocking the token creation unnecessarily.

Suggestion: Evaluate if these operations could be optimized, though the sequential nature may be intentional for correctness.

---

🔒 Security Concerns

Socket Token Error Leakage

PlayersAPI.cs:234 - The exception message is logged with Debug.LogWarning, which could expose internal error details in production builds if debug logging is enabled.

Suggestion: Consider sanitizing error messages in production builds or using more generic error messages.

---

🔄 Backwards Compatibility

Breaking Change: Static Event Addition

Talo.cs:10-11 - Adding static events OnConnectionLost and OnConnectionRestored to the public API is backwards compatible for most use cases. However, if any code was using reflection to inspect the Talo class structure, this could cause issues.

Impact: Low - this is generally a safe addition.

Behavioral Change: TaloSocket.ResetConnection()

TaloSocket.cs:131-136 - Removing the identity check changes the behavior of ResetConnection(). Previously, it was a no-op when not identified; now it always resets. This could break existing code that relied on the old behavior.

Suggestion: Document this behavioral change in release notes, or consider adding a parameter to control whether to enforce the identity check.

---

📝 Summary

The PR implements connection monitoring and automatic reconnection, which is a valuable feature. However, there are several concerns around race conditions, redundant code, and potential cascading health checks that should be addressed. The most critical issue is the race condition in socket reconnection when token creation fails, and the removed identity check that could cause exceptions in the reconnection flow.