modify: change map cooldown behaviour

[fltuna]

No description provided.

[gemini-code-assist]

Summary of Changes

Hello @fltuna, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request addresses and corrects the behavior of map cooldowns within the McsMapCycleController. Previously, the cooldown for the previous map was being reset upon a new map start. The updated logic now correctly applies the configured cooldown to the newly started map, ensuring proper map rotation and preventing maps from being played too frequently. This change also includes a refactoring of the cooldown application process into a dedicated helper method.

Highlights

• Corrected Map Cooldown Application: The logic for applying map cooldowns has been reordered to ensure that the newly started map receives its configured cooldown, preventing it from being selected again prematurely.
• Refactored Cooldown Management: The specific logic for applying a cooldown to a single map and its groups has been extracted into a new, dedicated private method ApplyCooldownToCurrentMap, improving code organization and readability.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[gemini-code-assist]

Code Review

This pull request refactors the map cooldown logic by extracting the cooldown application into a new method. However, the reordering of operations in OnMapStart introduces a critical bug where the cooldown is applied to the newly started map instead of the one that just finished. Additionally, the new method uses a 'fire-and-forget' pattern for asynchronous database calls, which can lead to unhandled exceptions. My review includes suggestions to fix the logical error and to properly handle the asynchronous operations for better stability and correctness.

[gemini-code-assist]

There's a logical error in the reordered operations. The cooldown is now being applied to the map that is starting (NextMap), not the map that just finished (the old CurrentMap).

With this change, CurrentMap is updated to NextMap before the cooldown logic is executed. This means ApplyCooldownToCurrentMap is called with the new map, putting it on cooldown immediately, while the map that just ended doesn't get its cooldown applied.

To fix this, the cooldown logic should be executed before CurrentMap is updated. Note that ApplyCooldownToCurrentMap should be made asynchronous as suggested in a separate comment, so you will need to await it and mark OnMapStart as async void.

        DecrementAllMapCooldown(CurrentMap);
        await ApplyCooldownToCurrentMap(CurrentMap);
        
        CurrentMap = NextMap;
        NextMap = null;

[gemini-code-assist]

This method performs asynchronous database operations but doesn't await them. This 'fire-and-forget' approach can lead to unhandled exceptions that could crash the server if the database operations fail. To make this more robust, the method should be async Task and await the database calls.

Additionally, for better clarity, consider renaming this method to ApplyCooldownToMap and its parameter to map. The current name ApplyCooldownToCurrentMap is misleading because the cooldown should be applied to the map that has just finished, not necessarily the one that is 'current' when the method is called.

    private async Task ApplyCooldownToMap(IMapConfig? map)
    {
        if (map == null)
            return;
            
        await _mcsDatabaseProvider.MapInfoRepository.UpsertMapCooldownAsync(map.MapName, map.MapCooldown.MapConfigCooldown).ConfigureAwait(false);
        map.MapCooldown.CurrentCooldown = map.MapCooldown.MapConfigCooldown;
        
        foreach (IMapGroupSettings setting in map.GroupSettings)
        {
            await _mcsDatabaseProvider.GroupInfoRepository.UpsertGroupCooldownAsync(setting.GroupName, setting.GroupCooldown.MapConfigCooldown).ConfigureAwait(false);
            setting.GroupCooldown.CurrentCooldown = setting.GroupCooldown.MapConfigCooldown;
        }
    }

[fltuna]

/gemini review

[gemini-code-assist]

Code Review

This pull request refactors the map cooldown logic, changing its behavior and improving its implementation. The cooldown is now applied to the current map at its start, rather than the previous map. Furthermore, the database operations for cooldowns are now executed asynchronously on a background thread, which is a great improvement to prevent blocking the main thread. The code has also been logically separated into more focused methods, enhancing readability. I have one suggestion regarding the use of async void which is a best practice recommendation.

[gemini-code-assist]

Using async void for lambdas passed to Task.Run is generally discouraged. While it works here because of the try-catch block, it's a less robust pattern. async void methods have different error-handling semantics and are harder to test and compose.

It's better to use an async Task lambda (async () => { ... }). Task.Run has an overload that accepts a Func<Task>, and this is the preferred way to run async operations on a background thread. The Task returned by Task.Run will then represent the entire lifetime of the async operation inside the lambda, which is generally what's expected and a more robust pattern.

        Task.Run(async () =>

[fltuna]

/gemini review

[gemini-code-assist]

Code Review

This pull request refactors the map cooldown logic to run asynchronously in a background task upon map start. The logic has been split into decrementing all cooldowns and applying a cooldown to the current map. While moving to an async/await pattern is a good improvement, there is a critical logic error where the cooldown is applied to the new map instead of the one that just ended. I've also added a note about the use of fire-and-forget tasks.

[gemini-code-assist]

There's a logic error in how the map cooldown is applied. CurrentMap is updated to the new map on line 268, before ApplyCooldownToCurrentMap is called. This results in the cooldown being applied to the map that is just starting, not the one that just finished. You should capture the previous map's configuration before updating CurrentMap and use that to apply the cooldown.

        var previousMap = CurrentMap;
        CurrentMap = NextMap;
        NextMap = null;

        Task.Run(async () =>
        {
            try
            {
                await DecrementAllMapCooldown();
                await ApplyCooldownToCurrentMap(previousMap);
            }
            catch (Exception e)
            {
                Logger.LogError(e, "Exception occured while applying map/group cooldown.");
            }
        });

[gemini-code-assist]

Using Task.Run for fire-and-forget operations can be risky. If the application shuts down unexpectedly, this background task could be terminated abruptly. This might leave the database in an inconsistent state if the operations are not atomic. While the previous implementation had a similar fire-and-forget behavior, it's worth considering a more robust approach for background tasks that ensures completion or graceful cancellation on shutdown, especially for critical operations like database updates.

[fltuna]

@gemini-code-assist So how do I needs to be? iirc I'm implemented a DB operation as a atomic, so I didn't care about fire-and-forget pattern.

[gemini-code-assist]

You're right that individual database operations can be atomic, ensuring data integrity at the database level for each specific write. However, the concern with a "fire-and-forget" Task.Run here is about the sequence of operations (DecrementAllMapCooldown and ApplyCooldownToCurrentMap). If the application shuts down unexpectedly between these two await calls, even if each is atomic, the overall application state regarding map cooldowns could become inconsistent.

The most robust way to ensure these critical operations complete is to await them directly. This would require changing the OnMapStart method's signature from private void to private async Task.

If OnMapStart can be made async Task, I suggest the following change to directly await the operations, which ensures that the method will not complete until these background tasks are finished. This eliminates the "fire-and-forget" aspect and provides better control over the task's lifecycle.

If, due to external constraints (e.g., OnMapStart being an event handler that must have a void return type), you cannot change its signature to async Task, then Task.Run with the try-catch block (as you currently have) is a common workaround to prevent blocking the main thread. However, it's important to be aware that in such async void scenarios, unhandled exceptions can still crash the application, and the task's completion cannot be directly observed or awaited by the caller, maintaining the risk of abrupt termination on application shutdown.

Suggested change

	Task.Run(async () =>
	{
	try
	{
	await DecrementAllMapCooldown();
	await ApplyCooldownToCurrentMap(CurrentMap);
	}
	catch (Exception e)
	{
	Logger.LogError(e, "Exception occured while applying map/group cooldown.");
	}
	});
	try
	{
	await DecrementAllMapCooldown();
	await ApplyCooldownToCurrentMap(CurrentMap);
	}
	catch (Exception e)
	{
	Logger.LogError(e, "Exception occured while applying map/group cooldown.");
	}

[fltuna]

@gemini-code-assist no this is in main game loop so I use Task.Run() as an async

[gemini-code-assist]

Thank you for clarifying that OnMapStart is part of the main game loop. Given this constraint, where the method likely needs to remain private void, using Task.Run is indeed the correct approach to offload the asynchronous database operations (DecrementAllMapCooldown and ApplyCooldownToCurrentMap) without blocking the main thread.

Your current implementation, which wraps these operations in a Task.Run with a try-catch block, is a standard pattern for handling asynchronous work in async void contexts. This prevents the main thread from being blocked and handles potential exceptions within the background task.

Just to reiterate the point from my previous comment, while this approach is necessary due to the void return type, it still carries the inherent characteristics of "fire-and-forget" tasks:

• Unhandled Exceptions: Although you have a try-catch block, if an exception were to escape this block within the Task.Run delegate, it could still crash the application, as exceptions in async void methods are re-thrown on the SynchronizationContext (or the ThreadPool if none is present).
• Application Shutdown: If the application shuts down while these tasks are still running, they might be terminated abruptly, potentially leaving the database in an inconsistent state if the operations are not fully atomic or transactional at a higher level than individual writes. You mentioned your DB operations are atomic, which helps mitigate this, but the sequence of operations still has a window of vulnerability.

For this specific scenario, your current implementation is a pragmatic solution given the void return type requirement. It correctly moves the asynchronous work off the main thread and includes basic error handling.