Debounce updating saves and players

[tudddorrr]

No description provided.

[claude]

Code Review - PR #166

🔴 Critical Issues

---

1. Null reference exception in debounced save updates

SavesAPI.ExecuteDebouncedOperation() at line 194-206 doesn't check if CurrentSave is null before calling UpdateSave(currentSave.id). Since debounced operations execute up to 1 second after being queued, the save could be unloaded in the meantime.

// Current code - vulnerable to null reference
protected override async Task ExecuteDebouncedOperation(DebouncedOperation operation)
{
    switch (operation)
    {
        case DebouncedOperation.Update:
            var currentSave = savesManager.CurrentSave;
            if (currentSave != null)  // Missing null check
            {
                await UpdateSave(currentSave.id);
            }
            break;
    }
}

Fix: Add null check before accessing currentSave.id:

case DebouncedOperation.Update:
    var currentSave = savesManager.CurrentSave;
    if (currentSave != null)
    {
        await UpdateSave(currentSave.id);
    }
    break;

---

2. Race condition in UpdateCurrentSave()

In SavesAPI.UpdateCurrentSave() (lines 213-232), CurrentSave is accessed twice. Between line 215 (var currentSave = savesManager.CurrentSave;) and line 230 (currentSave.content = contentManager.Content;), the save could be unloaded by another thread/coroutine, making the local reference stale.

Fix: Cache the reference and validate it's still current before mutation:

var currentSave = savesManager.CurrentSave;
if (currentSave == null)
{
    throw new Exception("No save is currently loaded");
}

if (!string.IsNullOrEmpty(newName))
{
    return await UpdateSave(currentSave.id, newName);
}

// Verify save is still current before mutating
if (savesManager.CurrentSave?.id != currentSave.id)
{
    throw new Exception("Save was unloaded during update");
}

currentSave.content = contentManager.Content;
DebounceUpdate();
return currentSave;

🟡 Major Issues

---

3. Async void methods in TaloManager.Update()

ProcessContinuityRequests() and ProcessDebouncedUpdates() (lines 84-112) are async void, which means:

• They can't be awaited
• Multiple instances can run concurrently if called every frame
• This could cause duplicate API calls for debounced operations

Fix: Add a flag to prevent concurrent execution:

private bool isProcessingDebounce = false;

private async void ProcessDebouncedUpdates()
{
    if (isProcessingDebounce) return;
    isProcessingDebounce = true;
    
    try
    {
        // existing implementation
    }
    catch (Exception ex)
    {
        Debug.LogError($"Failed to process debounced updates: {ex}");
    }
    finally
    {
        isProcessingDebounce = false;
    }
}

🔵 Minor Issues

---

4. Pending debounced operations not cancelled on identity clear

When ClearIdentity() is called or a save is unloaded, any pending debounced operations continue to execute. This could cause updates for players/saves that are no longer active.

Suggestion: Clear pending operations in DebouncedAPI when appropriate, or add a method to cancel pending operations that can be called from ClearIdentity() and UnloadCurrentSave().