Skip to content

Enhance configuration options with auto-blacklist, delay, and retry settings #126

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Enhance configuration options with auto-blacklist, delay, and retry settings #126

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
c8c4497
Enhance configuration options: Add auto-blacklist, delay, and retry s…
peter9811 Mar 7, 2025
7fcd2c1
Update .gitignore: Add .vscode directory to ignore list
peter9811 Mar 7, 2025
932d78f
Refactor: Improve formatting and structure in various files
peter9811 Mar 7, 2025
f9052a8
Update subproject commit reference in ArchiSteamFarm
peter9811 Mar 7, 2025
5048b73
Update subproject commit reference in ArchiSteamFarm
peter9811 Mar 7, 2025
1ca3e5f
Refactor: Improve code formatting and structure across multiple files
peter9811 Mar 7, 2025
0847a0e
Enhance FreeGamesCommand: Add retryAttempts variable for error handling
peter9811 Mar 7, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
201 changes: 173 additions & 28 deletions ASFFreeGames/Commands/FreeGamesCommand.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,13 @@ public void Dispose() {
return await HandleInternalSaveOptionsCommand(bot, cancellationToken).ConfigureAwait(false);
case CollectInternalCommandString:
return await HandleInternalCollectCommand(bot, args, cancellationToken).ConfigureAwait(false);
case "SHOWBLACKLIST":
if (Options.Blacklist.Count == 0) {
return FormatBotResponse(bot, "Blacklist is empty");
} else {
string blacklistItems = string.Join(", ", Options.Blacklist);
return FormatBotResponse(bot, $"Current blacklist: {blacklistItems}");
}
}
}

Expand Down Expand Up @@ -119,7 +126,36 @@ public void Dispose() {
await SaveOptions(cancellationToken).ConfigureAwait(false);

return FormatBotResponse(bot, $"{ASFFreeGamesPlugin.StaticName} is now skipping dlc");
case "CLEARBLACKLIST":
Options.ClearBlacklist();
await SaveOptions(cancellationToken).ConfigureAwait(false);

return FormatBotResponse(bot, $"{ASFFreeGamesPlugin.StaticName} blacklist has been cleared");
case "REMOVEBLACKLIST":
if (args.Length >= 4) {
string identifier = args[3];
if (GameIdentifier.TryParse(identifier, out GameIdentifier gid)) {
bool removed = Options.RemoveFromBlacklist(in gid);
await SaveOptions(cancellationToken).ConfigureAwait(false);

if (removed) {
return FormatBotResponse(bot, $"{ASFFreeGamesPlugin.StaticName} removed {gid} from blacklist");
} else {
return FormatBotResponse(bot, $"{ASFFreeGamesPlugin.StaticName} could not find {gid} in blacklist");
}
} else {
return FormatBotResponse(bot, $"Invalid game identifier format: {identifier}");
}
} else {
return FormatBotResponse(bot, "Please provide a game identifier to remove from blacklist");
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

This block handles the REMOVEBLACKLIST command. Consider adding a check to ensure the identifier is not null or empty before attempting to parse it. This can prevent unexpected behavior or errors.

					case "REMOVEBLACKLIST":
						if (args.Length >= 4) {
							string identifier = args[3];
							if (string.IsNullOrEmpty(identifier)) {
								return FormatBotResponse(bot, "Please provide a valid game identifier to remove from blacklist");
							}
							if (GameIdentifier.TryParse(identifier, out GameIdentifier gid)) {

case "SHOWBLACKLIST":
if (Options.Blacklist.Count == 0) {
return FormatBotResponse(bot, "Blacklist is empty");
} else {
string blacklistItems = string.Join(", ", Options.Blacklist);
return FormatBotResponse(bot, $"Current blacklist: {blacklistItems}");
}
default:
return FormatBotResponse(bot, $"Unknown \"{args[2]}\" variable to set");
}
Expand Down Expand Up @@ -239,6 +275,9 @@ private async Task<int> CollectGames(IEnumerable<Bot> bots, ECollectGameRequestS
PreviousSucessfulStrategy = PreviousSucessfulStrategy
};

// Cache of known invalid packages to avoid repeated failed attempts within the same collection run
HashSet<string> knownInvalidPackages = new();

try {
#pragma warning disable CA2000
games = await Strategy.GetGames(strategyContext, cancellationToken).ConfigureAwait(false);
Expand Down Expand Up @@ -312,6 +351,14 @@ private async Task<int> CollectGames(IEnumerable<Bot> bots, ECollectGameRequestS
continue;
}

// Skip packages that have already failed in this collection run
if (knownInvalidPackages.Contains(gid.ToString())) {
if (VerboseLog) {
bot.ArchiLogger.LogGenericDebug($"Skipping previously failed package in this run: {gid}", nameof(CollectGames));
}
continue;
}

string? resp;

string cmd = $"ADDLICENSE {bot.BotName} {gid}";
Expand All @@ -320,47 +367,145 @@ private async Task<int> CollectGames(IEnumerable<Bot> bots, ECollectGameRequestS
bot.ArchiLogger.LogGenericDebug($"Trying to perform command \"{cmd}\"", nameof(CollectGames));
}

using (LoggerFilter.DisableLoggingForAddLicenseCommonErrors(_ => !VerboseLog && (requestSource is not ECollectGameRequestSource.RequestedByUser) && context.ShouldHideErrorLogForApp(in gid), bot)) {
resp = await bot.Commands.Response(EAccess.Operator, cmd).ConfigureAwait(false);
}
int retryAttempts = 0;
int maxRetries = Options.MaxRetryAttempts ?? 1;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Consider adding a check to ensure that Options is not null before accessing MaxRetryAttempts. While the null-coalescing operator provides a default value, explicitly checking for null can prevent potential NullReferenceException if Options itself is null.

						int maxRetries = Options?.MaxRetryAttempts ?? 1;

bool isTransientError = false;

bool success = false;
do {
if (retryAttempts > 0) {
// Add delay before retry
int retryDelay = Options.RetryDelayMilliseconds ?? 2000;
await Task.Delay(retryDelay, cancellationToken).ConfigureAwait(false);

if (!string.IsNullOrWhiteSpace(resp)) {
success = resp!.Contains("collected game", StringComparison.InvariantCultureIgnoreCase);
success |= resp!.Contains("OK", StringComparison.InvariantCultureIgnoreCase);

if (success || VerboseLog || requestSource is ECollectGameRequestSource.RequestedByUser || !context.ShouldHideErrorLogForApp(in gid)) {
bot.ArchiLogger.LogGenericInfo($"[FreeGames] {resp}", nameof(CollectGames));
if (VerboseLog || requestSource is ECollectGameRequestSource.RequestedByUser) {
bot.ArchiLogger.LogGenericInfo($"[FreeGames] Retry attempt {retryAttempts} for {gid}", nameof(CollectGames));
}
}
}

if (success) {
lock (context) {
context.RegisterApp(in gid);
using (LoggerFilter.DisableLoggingForAddLicenseCommonErrors(_ => !VerboseLog && (requestSource is not ECollectGameRequestSource.RequestedByUser) && context.ShouldHideErrorLogForApp(in gid), bot)) {
resp = await bot.Commands.Response(EAccess.Operator, cmd).ConfigureAwait(false);
}

save = true;
res++;
}
else {
if ((requestSource != ECollectGameRequestSource.RequestedByUser) && (resp?.Contains("RateLimited", StringComparison.InvariantCultureIgnoreCase) ?? false)) {
if (VerboseLog) {
bot.ArchiLogger.LogGenericWarning("[FreeGames] Rate limit reached ! Skipping remaining games...", nameof(CollectGames));
bool success = false;

if (!string.IsNullOrWhiteSpace(resp)) {
success = resp!.Contains("collected game", StringComparison.InvariantCultureIgnoreCase);
success |= resp!.Contains("OK", StringComparison.InvariantCultureIgnoreCase);

// Check if this is a transient error that should be retried
isTransientError = !success &&
(resp.Contains("timeout", StringComparison.InvariantCultureIgnoreCase) ||
resp.Contains("connection error", StringComparison.InvariantCultureIgnoreCase) ||
resp.Contains("service unavailable", StringComparison.InvariantCultureIgnoreCase));

// Don't retry if we got a clear "Forbidden" or other definitive error
if (resp.Contains("Forbidden", StringComparison.InvariantCultureIgnoreCase) ||
resp.Contains("RateLimited", StringComparison.InvariantCultureIgnoreCase) ||
resp.Contains("no eligible accounts", StringComparison.InvariantCultureIgnoreCase)) {
isTransientError = false;
}

break;
// Log the result regardless of success if it's verbose or user-requested
if (success || (!isTransientError && (VerboseLog || requestSource is ECollectGameRequestSource.RequestedByUser || !context.ShouldHideErrorLogForApp(in gid)))) {
string statusMessage;
if (success) {
statusMessage = "Success";
} else if (resp.Contains("Forbidden", StringComparison.InvariantCultureIgnoreCase)) {
statusMessage = "AccessDenied/InvalidPackage";
} else if (resp.Contains("RateLimited", StringComparison.InvariantCultureIgnoreCase)) {
statusMessage = "RateLimited";
} else if (resp.Contains("timeout", StringComparison.InvariantCultureIgnoreCase)) {
statusMessage = "Timeout";
} else if (resp.Contains("no eligible accounts", StringComparison.InvariantCultureIgnoreCase)) {
statusMessage = "NoEligibleAccounts";
} else {
statusMessage = "Failed";
}

bot.ArchiLogger.LogGenericInfo($"[FreeGames] <{bot.BotName}> ID: {gid} | Status: {statusMessage}{(isTransientError && retryAttempts < maxRetries ? " (Will retry)" : "")}", nameof(CollectGames));
}
}

if (DateTimeOffset.UtcNow.ToUnixTimeSeconds() - time > DayInSeconds) {
lock (context) {
context.AppTickCount(in gid, increment: true);
// If request was successful or this is not a transient error, break the loop
if (success || !isTransientError) {

if (success) {
lock (context) {
context.RegisterApp(in gid);
}

save = true;
res++;
}
else {
// Add the game to the processed list even if it failed with Forbidden to avoid retrying
if (resp?.Contains("Forbidden", StringComparison.InvariantCultureIgnoreCase) ?? false) {
lock (context) {
// Register the app as attempted but failed due to access restrictions
context.RegisterApp(in gid);
}
save = true;

// Add to the known invalid packages for this collection run
knownInvalidPackages.Add(gid.ToString());

// Optionally blacklist this game ID if auto-blacklisting is enabled
if (Options.AutoBlacklistForbiddenPackages ?? true) {
Options.AddToBlacklist(in gid);

if (VerboseLog || requestSource is ECollectGameRequestSource.RequestedByUser) {
bot.ArchiLogger.LogGenericInfo($"[FreeGames] Adding {gid} to blacklist due to Forbidden response", nameof(CollectGames));
}

// Save the updated options to persist the blacklist
_ = Task.Run(async () => {
try {
await SaveOptions(cancellationToken).ConfigureAwait(false);
} catch (Exception ex) {
if (VerboseLog || requestSource is ECollectGameRequestSource.RequestedByUser) {
bot.ArchiLogger.LogGenericWarning($"Failed to save options after blacklisting: {ex.Message}", nameof(CollectGames));
}
}
});
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The code saves options within a Task.Run. While this prevents blocking the main thread, it's generally better to use Task.ConfigureAwait(false) to avoid potential deadlocks or context-switching issues. Also, consider using ArchiSteamFarm.Core.ASF.ArchiLogger.LogGenericError instead of bot.ArchiLogger.LogGenericWarning to be consistent with the logging style.

                                                        _ = Task.Run(async () => {
                                                                try {
                                                                        await SaveOptions(cancellationToken).ConfigureAwait(false);
                                                                } catch (Exception ex) {
                                                                        if (VerboseLog || requestSource is ECollectGameRequestSource.RequestedByUser) {
                                                                                ArchiSteamFarm.Core.ASF.ArchiLogger.LogGenericError($"Failed to save options after blacklisting: {ex.Message}");
                                                                        }
                                                                }
                                                        }).ConfigureAwait(false);


if (VerboseLog || requestSource is ECollectGameRequestSource.RequestedByUser) {
bot.ArchiLogger.LogGenericWarning($"[FreeGames] Access denied for {gid}. The package may no longer be available or there are restrictions.", nameof(CollectGames));
}
}

if ((requestSource != ECollectGameRequestSource.RequestedByUser) && (resp?.Contains("RateLimited", StringComparison.InvariantCultureIgnoreCase) ?? false)) {
if (VerboseLog) {
bot.ArchiLogger.LogGenericWarning("[FreeGames] Rate limit reached ! Skipping remaining games...", nameof(CollectGames));
}

break;
}
}

// Check if we need to update app tick counts or register invalid apps
if ((!success || isTransientError) && resp != null) {
if (DateTimeOffset.UtcNow.ToUnixTimeSeconds() - time > DayInSeconds) {
lock (context) {
context.AppTickCount(in gid, increment: true);
}
}

if (InvalidAppPurchaseRegex.Value.IsMatch(resp)) {
save |= context.RegisterInvalidApp(in gid);
}
}
}

if (InvalidAppPurchaseRegex.Value.IsMatch(resp ?? "")) {
save |= context.RegisterInvalidApp(in gid);
break;
}

retryAttempts++;
} while (isTransientError && retryAttempts <= maxRetries);

// Add a delay between requests to avoid hitting rate limits
int delay = Options.DelayBetweenRequests ?? 500;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Consider adding a check to ensure that delay is not negative to prevent potential issues.

int delay = (Options.DelayBetweenRequests > 0) ? Options.DelayBetweenRequests.Value : 500;

if (delay > 0) {
await Task.Delay(delay, cancellationToken).ConfigureAwait(false);
}
}

Expand Down
41 changes: 41 additions & 0 deletions ASFFreeGames/Configurations/ASFFreeGamesOptions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,18 @@ public class ASFFreeGamesOptions {
[JsonPropertyName("verboseLog")]
public bool? VerboseLog { get; set; }

[JsonPropertyName("autoBlacklistForbiddenPackages")]
public bool? AutoBlacklistForbiddenPackages { get; set; } = true;

[JsonPropertyName("delayBetweenRequests")]
public int? DelayBetweenRequests { get; set; } = 500; // Default 500ms delay between requests

[JsonPropertyName("maxRetryAttempts")]
public int? MaxRetryAttempts { get; set; } = 1; // Default 1 retry attempt for transient errors

[JsonPropertyName("retryDelayMilliseconds")]
public int? RetryDelayMilliseconds { get; set; } = 2000; // Default 2 second delay between retries

#region IsBlacklisted
public bool IsBlacklisted(in GameIdentifier gid) {
if (Blacklist.Count <= 0) {
Expand All @@ -43,6 +55,35 @@ public bool IsBlacklisted(in GameIdentifier gid) {
}

public bool IsBlacklisted(in Bot? bot) => bot is null || ((Blacklist.Count > 0) && Blacklist.Contains($"bot/{bot.BotName}"));

public void AddToBlacklist(in GameIdentifier gid) {
if (Blacklist is HashSet<string> blacklist) {
blacklist.Add(gid.ToString());
} else {
Blacklist = new HashSet<string>(Blacklist) { gid.ToString() };
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The logic for adding to the blacklist checks if Blacklist is a HashSet<string>. If not, it creates a new HashSet<string> from the existing Blacklist. This could be simplified by always using a HashSet<string> for Blacklist and ensuring it's properly initialized.

        public void AddToBlacklist(in GameIdentifier gid) {
            if (Blacklist is not HashSet<string> blacklist) {
                Blacklist = new HashSet<string>(Blacklist);
                blacklist = (HashSet<string>)Blacklist;
            }
            ((HashSet<string>)Blacklist).Add(gid.ToString());
        }

}

public bool RemoveFromBlacklist(in GameIdentifier gid) {
if (Blacklist is HashSet<string> blacklist) {
return blacklist.Remove(gid.ToString()) || blacklist.Remove(gid.Id.ToString(CultureInfo.InvariantCulture));
} else {
HashSet<string> newBlacklist = new(Blacklist);
bool removed = newBlacklist.Remove(gid.ToString()) || newBlacklist.Remove(gid.Id.ToString(CultureInfo.InvariantCulture));
if (removed) {
Blacklist = newBlacklist;
}
return removed;
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Similar to AddToBlacklist, the RemoveFromBlacklist method also checks the type of Blacklist and creates a new HashSet<string> if it's not already one. This logic can be simplified by ensuring Blacklist is always a HashSet<string>. Also, consider using StringComparer.InvariantCultureIgnoreCase for case-insensitive comparisons.

        public bool RemoveFromBlacklist(in GameIdentifier gid) {
            if (Blacklist is not HashSet<string> blacklist) {
                Blacklist = new HashSet<string>(Blacklist);
                blacklist = (HashSet<string>)Blacklist;
            }
            return ((HashSet<string>)Blacklist).Remove(gid.ToString());
        }

}

public void ClearBlacklist() {
if (Blacklist is HashSet<string> blacklist) {
blacklist.Clear();
} else {
Blacklist = new HashSet<string>();
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The ClearBlacklist method also has similar logic to AddToBlacklist and RemoveFromBlacklist. Consider ensuring Blacklist is always a HashSet<string> to simplify this method.

        public void ClearBlacklist() {
            if (Blacklist is not HashSet<string> blacklist) {
                Blacklist = new HashSet<string>();
            }
            ((HashSet<string>)Blacklist).Clear();
        }

}
#endregion

#region proxy
Expand Down
23 changes: 23 additions & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,9 +40,32 @@ The plugin behavior is configurable via command
- `freegames set f2p` to ☑️**allow** the plugin to collect **f2p** (the default)
- `freegames set nodlc` to ⛔**prevent** the plugin from collecting **dlc**
- `freegames set dlc` to ☑️**allow** the plugin to collect **dlc** (the default)
- `freegames set clearblacklist` to 🗑️**clear** all entries from the **blacklist**
- `freegames set removeblacklist s/######` to 🔄**remove** a specific package from the **blacklist**
- `freegames set showblacklist` to 📋**display** all entries in the current **blacklist**

In addition to the commands above, the configuration is stored in a 📖`config/freegames.json.config` JSON file, which one may 🖊 edit using a text editor to suit their needs.

#### Additional Configuration Options

The following options can be set in the `freegames.json.config` file:

```json
{
"autoBlacklistForbiddenPackages": true, // Automatically blacklist packages that return Forbidden errors
"delayBetweenRequests": 500, // Delay in milliseconds between license requests (helps avoid rate limits)
"maxRetryAttempts": 1, // Number of retry attempts for transient errors (like timeouts)
"retryDelayMilliseconds": 2000 // Delay in milliseconds before retrying a failed request
}
```

**Option Descriptions:**

- `autoBlacklistForbiddenPackages`: When true, packages that return "Forbidden" errors are automatically added to the blacklist to prevent future attempts.
- `delayBetweenRequests`: Adds a delay between license requests to reduce the chance of hitting Steam's rate limits.
- `maxRetryAttempts`: Number of times to retry requests that fail due to transient errors (e.g., timeouts).
- `retryDelayMilliseconds`: How long to wait before retrying a failed request.

## Proxy Setup

The plugin can be configured to use a proxy (HTTP(S), SOCKS4, or SOCKS5) for its HTTP requests to Reddit. You can achieve this in two ways:
Expand Down