Release 0.48.0

[tudddorrr]

No description provided.

[claude]

Code Review - Release 0.48.0

---

🐛 Potential Bugs & Issues

Race Conditions in Connection State Management

ContinuityManager.cs:128-154 - HandlePostResponseHealthCheck can trigger concurrent health check pings. If multiple failed requests occur simultaneously, this creates multiple parallel Ping() calls, leading to race conditions when setting lastHealthCheckStatus.

HealthCheckAPI.cs:41 - The check failedLastHealthCheck is read before updating lastHealthCheckStatus, but in concurrent scenarios, multiple threads could read UNKNOWN simultaneously, then all update to OK, missing the state transition detection.

Unsubscribed Event Handler Memory Leak

PlayersAPI.cs:19 - The constructor subscribes to Talo.OnConnectionRestored, but there's no corresponding unsubscribe in a destructor/dispose pattern. Since PlayersAPI instances are static and long-lived, this is less critical but represents poor cleanup hygiene. If the API instances are ever recreated, this creates duplicate subscriptions.

Async Void Anti-Pattern

PlayersAPI.cs:22 - OnConnectionRestored() is async void, which means exceptions thrown here cannot be caught and will crash the application. If ResetConnection() or CreateSocketToken() throw, the exception propagates to the synchronization context.

Silent Failure in Socket Token Creation

PlayersAPI.cs:28-32 - If CreateSocketToken() returns an empty string due to an exception (line 223-239), the socket token is silently not set. The connection restoration appears successful but the socket won't authenticate. Consider logging this scenario more explicitly or propagating the failure.

Duplicate Dead Code

Talo.cs:235-243 - HandleConnectionLost() and HandleConnectionRestored() methods are defined but never called. These appear to be dead code duplicating the functionality of InvokeConnectionLost() and InvokeConnectionRestored() (lines 17-25).

Health Check Recursion Risk

ContinuityManager.cs:143 & 150 - Calling Talo.HealthCheck.Ping() from HandlePostResponseHealthCheck() which is called after every API response creates potential for recursive calls if the health check itself triggers through BaseAPI.Call() (though line 130-132 attempts to prevent this for /health-check specifically).

---

⚡ Performance Considerations

Sequential Task Execution

SavesAPI.cs:76-94 - SyncOfflineSaves() creates parallel tasks but then calls savesManager.DeleteOfflineSaves() synchronously after all tasks complete. Consider batching deletes or making the delete operation async to improve throughput.

Unnecessary String Contains Check

ContinuityManager.cs:130 - Using url.Contains("/health-check") on every single API call is inefficient. Consider checking uri.AbsolutePath or using a more precise string comparison since you already have structured URI data.

Excessive Health Check Triggers

ContinuityManager.cs:128-154 - Every successful or failed API call can potentially trigger a Ping(), which itself makes an API call. This could create significant overhead during periods of intermittent connectivity. Consider adding a debounce/throttle mechanism to limit health check frequency.

No Null Check Before Iteration

SavesAPI.cs:77 - SyncOfflineSaves() checks offlineSaves.Length == 0 but doesn't check if offlineSaves itself is null before calling .Where(). While the diff shows this method is now private and controlled, defensive programming would add resilience.

---

🔒 Security Concerns

Async Void Exception Handling

PlayersAPI.cs:22 - The async void handler makes exception handling difficult, potentially exposing internal error states or stack traces if socket operations fail during connection restoration.

---

🔄 Backwards Compatibility

Breaking API Change in TaloSocket

TaloSocket.cs:131-137 - ResetConnection() previously had a guard clause checking if (!identified) which was removed. This changes the behavior - external code that called ResetConnection() when not identified would previously silently no-op, but now will disconnect and attempt to reconnect. This could cause unexpected WebSocket churn for consumers.

New Required Package Dependency

Packages/manifest.json:5 - Added com.unity.toolchain.macos-arm64-linux-x86_64 as a dependency. Verify this doesn't break builds on unsupported platforms or older Unity versions that don't have this package available.

Unity Version Upgrade

CI workflow & ProjectVersion.txt - Upgrading from Unity 6000.0.28f1 to 6000.0.59f2 may introduce breaking changes for users still on older versions. Ensure this is clearly communicated in release notes.

---

💡 Code Quality & Best Practices

Inconsistent Error Handling

PlayersAPI.cs:236-238 - CreateSocketToken() catches all exceptions and returns an empty string with a warning. This differs from the pattern used elsewhere (like line 79-84) where exceptions are rethrown. Consider consistency.

Missing XML Documentation

New public APIs like StatsAPI.ListPlayerStats() (line 47) and the new HealthCheckStatus enum lack XML documentation comments. This impacts discoverability for API consumers.

Unclear Method Naming

ContinuityManager.HandlePostResponseHealthCheck() - The name suggests it handles checks after responses, but it actually triggers new health check requests. Consider renaming to something like CheckConnectionStatusAfterResponse() for clarity.

---

[claude]

🔍 Code Review - Release 0.48.0

I've reviewed the PR and identified several areas for improvement:

---

🐛 Potential Bugs

1. Memory Leak from Event Subscription (PlayersAPI.cs:19)

• The constructor subscribes to Talo.OnConnectionRestored but never unsubscribes
• Since PlayersAPI is a singleton created in the static constructor of Talo, this shouldn't cause multiple subscriptions in normal use, but it's still a poor pattern
• Consider implementing IDisposable or providing a cleanup method

2. Unhandled Exception in async void (PlayersAPI.cs:22)

• OnConnectionRestored() is async void which means exceptions thrown will crash the application
• If ResetConnection() or CreateSocketToken() throw, the exception won't be caught by the empty catch block in CreateSocketToken()
• Change to async Task and handle exceptions, or wrap the entire method body in try-catch

3. Duplicate Methods in Talo.cs (lines 235-243)

• HandleConnectionLost() and HandleConnectionRestored() are duplicates of InvokeConnectionLost() and InvokeConnectionRestored() (lines 17-25)
• The new methods are never called - only the Invoke*() versions are used
• Remove the unused Handle*() methods

4. Race Condition in HealthCheckAPI.cs (lines 41-58)

• lastHealthCheckStatus is not thread-safe and can be modified concurrently
• Multiple calls to Ping() could result in events firing in the wrong order or multiple times
• Consider using a lock or checking both old and new status atomically

5. Null Check Missing in SavesManager.DeleteOfflineSaves (SavesAPI.cs:102)

• DeleteOfflineSaves() is called but we don't have the implementation visible in this diff
• Based on the similar DeleteOfflineSave() pattern, ensure null checks are present for offlineContent?.saves

---

⚡ Performance Considerations

1. Unnecessary Health Check After Every Request (ContinuityManager.cs:128-154)

• HandlePostResponseHealthCheck() is called after EVERY API response via BaseAPI.cs:104
• This triggers additional health check pings on every success/failure, creating a cascade of requests
• Consider rate-limiting or debouncing these checks, or using a flag instead of calling Ping() directly

2. Sequential Processing in SavesAPI.SyncOfflineSaves (SavesAPI.cs:79-97)

• While tasks are created in parallel, failed requests still consume time and create unnecessary API objects
• Consider adding a circuit breaker pattern if multiple syncs are failing

---

🔒 Security Concerns

1. Empty Exception Handler Swallows Errors (SavesAPI.cs:93-96)

• Failed save syncs fail silently with no logging
• Users won't know if their offline saves failed to sync to the server
• Add at minimum a Debug.LogWarning() to inform users of sync failures

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

• When socket token creation fails, it returns an empty string and only logs a warning
• The caller at line 29 checks !string.IsNullOrEmpty(socketToken) but this might leave the socket in an inconsistent state
• Consider more robust error recovery or retry logic

---

🔄 Backwards Compatibility

1. New Public Events (Talo.cs:10-11)

• OnConnectionLost and OnConnectionRestored are new public static events
• Any user code that references Talo might trigger the static constructor earlier than before
• This should be safe, but document these new events for users

2. HealthCheckStatus Enum (HealthCheckAPI.cs:7-12)

• New public enum could conflict if users have their own HealthCheckStatus in the same namespace
• Consider prefixing as TaloHealthCheckStatus or moving to a nested class

3. Breaking Change in TaloSocket.ResetConnection() (TaloSocket.cs:131-137)

• Previously checked if (!identified) return; before resetting (line 133-136 in old code)
• Now always resets regardless of identification status
• This could break existing code that relied on the early return behavior

---

💡 Code Quality Notes

1. Inconsistent Null Safety

• Some methods use ?.Invoke() pattern (good), others don't check
• Consider adding null checks consistently throughout

2. Magic Numbers

• ProcessRequests() uses hardcoded 10 for queue limit (ContinuityManager.cs:88)
• Consider making this configurable

3. Exception Handling Pattern

• Mix of empty catch blocks, catch with logging, and catch with throw
• Standardize exception handling approach across the codebase

[claude]

Pull Request Review: Release 0.48.0

---

🐛 Code Quality and Best Practices

ContinuityManager.cs:128-154

• The HandlePostResponseHealthCheck method performs health checks on every successful/failed request, which could lead to excessive health check pings. Consider adding debouncing or rate limiting to prevent redundant health checks when multiple requests fail/succeed in quick succession.

PlayersAPI.cs:22-34

• The OnConnectionRestored event handler is registered in the constructor but never unregistered. If PlayersAPI instances are created/destroyed (though unlikely in this architecture), this could lead to memory leaks or duplicate event handlers.

HealthCheckAPI.cs:41

• Variable naming: failedLastHealthCheck is a bit confusing as it represents the past state. Consider wasFailedPreviously or previouslyFailed for clarity.

SavesAPI.cs:72-105

• In SyncOfflineSaves, the method silently swallows all exceptions during individual save sync operations (line 94). This makes debugging difficult. Consider logging failed sync operations even if you don't want to throw.

ListPlayerStats.cs:27-28

• Exception is caught, displayed to user, then re-thrown. Re-throwing after handling defeats the purpose of the try-catch. Either handle it completely or let it propagate - don't do both.

---

🔒 Security Concerns

ContinuityManager.cs:99

• Authorization headers are re-added during request replay using the current accessKey from settings. If the access key has changed between the original request and replay, this could cause authorization mismatches or replay requests with invalid credentials. Consider whether the timestamp header is sufficient for preventing replay attacks, or if additional validation is needed.

BaseAPI.cs:104

• HandlePostResponseHealthCheck is called after every response but before checking if the request was successful. This means the health check logic executes even when requests fail for reasons other than connectivity (e.g., 400 Bad Request). The current implementation might mask legitimate API errors by triggering offline mode unnecessarily.

---

⚡ Performance Considerations

ContinuityManager.cs:128 & BaseAPI.cs:104

• Every single API call now triggers HandlePostResponseHealthCheck, which may trigger additional health check pings. For applications making frequent API calls, this could significantly increase network traffic and latency.

SavesAPI.cs:127-139

• The parallel execution of save syncing creates a task for every online save, even those that don't have offline counterparts. This is wasteful. Consider filtering first: onlineSaves.Where(online => offlineSaves.Any(offline => offline.id == online.id)) before creating tasks.

TaloManager.cs:48-67

• The Update method now calls await Talo.Continuity.ProcessRequests() which is an async operation in a non-async context. While using async void works, it's generally an anti-pattern in Unity's Update loop. If ProcessRequests takes significant time, it could cause frame drops. Consider running this in a background task or coroutine instead.

---

🔄 Backwards Compatibility

HealthCheckAPI.cs:7-12

• New public enum HealthCheckStatus is added. If external code was using string comparisons or custom status tracking, this change is compatible. However, the initial state is UNKNOWN which might cause unexpected behavior if code checks for specific states before any health check has run.

TaloSocket.cs:131-137

• The ResetConnection method previously had an early return if !identified. This check was removed. This means the method now allows resetting connections even when not identified, which could change behavior for existing callers expecting the early exit. The removed lines (130-135 in the diff) fundamentally change the method's contract.

Talo.cs:7-8

• New public static events OnConnectionLost and OnConnectionRestored are added. These are additive changes and shouldn't break existing code, but consumers need to be aware that these events fire based on health check results, which may not perfectly align with actual connectivity issues (e.g., DNS problems vs. service degradation).

---

📦 Additional Notes

SavesManager.cs (referenced but modified)

• The SyncOfflineSaves method was moved from SavesManager to SavesAPI. While this is a refactoring improvement (better separation of concerns), ensure that SavesManager doesn't expose this as a public method anymore, as it could lead to confusion.

Unity Version Update

• CI workflow updates Unity from 6000.0.28f1 to 6000.0.59f2. Verify that all new Unity APIs used are compatible with the minimum supported Unity version for this package.