-
Notifications
You must be signed in to change notification settings - Fork 2
Enforce MaxAttemptsTotal across messages instead of per message #242
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.
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
Merged
+13,092
−233
Merged
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,11 +1,9 @@ | ||
| using System; | ||
| using System.Collections.Generic; | ||
| using SW.PrimitiveTypes; | ||
|
|
||
| namespace SW.Bitween.Domain; | ||
| // Id should be the same for xchangeId when retry happens the record is deleted | ||
| public class DelayedRetry : BaseEntity<string> | ||
| { | ||
| public DateTime On { get; set; } | ||
| public Dictionary<string, int> GroupAttemptCounts { get; set; } = new(); | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,27 @@ | ||
| using System; | ||
|
|
||
| namespace SW.Bitween.Domain; | ||
|
|
||
| /// <summary> | ||
| /// Running total of the retries one retry group has spent for one integration, backing | ||
| /// <c>RetryBudget.MaxAttemptsTotal</c>. That cap is shared by every message hitting the | ||
| /// group, so it cannot be tracked on an individual xchange. | ||
| /// </summary> | ||
| /// <remarks> | ||
| /// The total never resets on its own: once <see cref="AttemptsUsed"/> reaches the group's | ||
| /// <c>MaxAttemptsTotal</c> the group stops retrying for that integration until this row is | ||
| /// cleared. | ||
| /// </remarks> | ||
| public class RetryGroupUsage | ||
| { | ||
| /// <summary>The integration whose budget this is. A shared policy gives each one its own total.</summary> | ||
| public int SubscriptionId { get; set; } | ||
|
|
||
| /// <summary><c>RetryGroup.Id</c>, which survives policy edits, so the total does too.</summary> | ||
| public Guid GroupId { get; set; } | ||
|
|
||
| public int AttemptsUsed { get; set; } | ||
|
|
||
| /// <summary>When the last attempt was claimed — the only clue left once a group is exhausted.</summary> | ||
| public DateTime LastAttemptOn { get; set; } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,56 @@ | ||
| using System.Linq; | ||
| using System.Threading.Tasks; | ||
| using Microsoft.EntityFrameworkCore; | ||
| using SW.Bitween.Domain; | ||
| using SW.Bitween.Domain.Accounts; | ||
| using SW.Bitween.Model; | ||
| using SW.PrimitiveTypes; | ||
|
|
||
| namespace SW.Bitween.Resources.RetryPolicies; | ||
|
|
||
| /// <summary> | ||
| /// Clears spent group budget, letting an exhausted group retry again. The total never resets on | ||
| /// its own, so this is the only way back for an integration that has hit its ceiling. | ||
| /// </summary> | ||
| [HandlerName("resetusage")] | ||
| public class ResetUsage : ICommandHandler<int, RetryPolicyResetUsage, object> | ||
| { | ||
| private readonly BitweenDbContext _dbContext; | ||
| private readonly RequestContext _requestContext; | ||
|
|
||
| public ResetUsage(BitweenDbContext dbContext, RequestContext requestContext) | ||
| { | ||
| _dbContext = dbContext; | ||
| _requestContext = requestContext; | ||
| } | ||
|
|
||
| public async Task<object> Handle(int key, RetryPolicyResetUsage request) | ||
| { | ||
| _requestContext.EnsureAccess(AccountRole.Admin, AccountRole.Member); | ||
|
|
||
| var policy = await _dbContext.Set<RetryPolicy>().AsNoTracking() | ||
| .FirstOrDefaultAsync(p => p.Id == key); | ||
| if (policy == null) throw new SWNotFoundException(key.ToString()); | ||
|
|
||
| // Scope the reset to this policy's own integrations and groups, so a policy id in the | ||
| // route can never clear a counter belonging to a different policy. | ||
| var subscriptionIds = await _dbContext.Set<Subscription>() | ||
| .Where(s => s.RetryPolicyId == key) | ||
| .Select(s => s.Id) | ||
| .ToListAsync(); | ||
|
|
||
| var groupIds = policy.Groups.Select(g => g.Id).ToList(); | ||
|
|
||
| var query = _dbContext.Set<RetryGroupUsage>() | ||
| .Where(u => subscriptionIds.Contains(u.SubscriptionId) && groupIds.Contains(u.GroupId)); | ||
|
hamzahalq marked this conversation as resolved.
|
||
|
|
||
| if (request.SubscriptionId.HasValue) | ||
| query = query.Where(u => u.SubscriptionId == request.SubscriptionId.Value); | ||
|
|
||
| if (request.GroupId.HasValue) | ||
| query = query.Where(u => u.GroupId == request.GroupId.Value); | ||
|
|
||
| await query.ExecuteDeleteAsync(); | ||
| return null; | ||
| } | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,6 @@ | ||
| using System.Linq; | ||
| using System.Threading.Tasks; | ||
| using Microsoft.EntityFrameworkCore; | ||
| using SW.Bitween.Domain; | ||
| using SW.Bitween.Domain.Accounts; | ||
| using SW.Bitween.Model; | ||
|
|
@@ -23,9 +25,23 @@ public async Task<object> Handle(int key, RetryPolicyUpdate model) | |
| RetryGroupValidation.EnsureCanFire(model.Groups); | ||
|
|
||
| var entity = await _dbContext.FindAsync<RetryPolicy>(key); | ||
|
|
||
| // Spent budget is keyed by group id, so a group removed here would leave usage rows | ||
| // that no policy claims — invisible to the usage report and beyond the reach of reset. | ||
| var removedGroupIds = entity.Groups | ||
| .Select(g => g.Id) | ||
| .Except((model.Groups ?? []).Select(g => g.Id)) | ||
| .ToList(); | ||
|
|
||
| entity.Name = model.Name; | ||
| entity.Groups = model.Groups ?? []; | ||
| await _dbContext.SaveChangesAsync(); | ||
|
|
||
| if (removedGroupIds.Count > 0) | ||
| await _dbContext.Set<RetryGroupUsage>() | ||
| .Where(u => removedGroupIds.Contains(u.GroupId)) | ||
| .ExecuteDeleteAsync(); | ||
|
Comment on lines
+40
to
+43
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift Prevent claims after policy cleanup. An evaluator that loaded the old policy can commit a usage claim after either cleanup query completes. The new row is then stranded after a group removal or policy deletion.
📍 Affects 2 files
🤖 Prompt for AI Agents |
||
|
|
||
| return null; | ||
| } | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,74 @@ | ||
| using System.Collections.Generic; | ||
| using System.Linq; | ||
| using System.Threading.Tasks; | ||
| using Microsoft.EntityFrameworkCore; | ||
| using SW.Bitween.Domain; | ||
| using SW.Bitween.Domain.Accounts; | ||
| using SW.Bitween.Model; | ||
| using SW.PrimitiveTypes; | ||
|
|
||
| namespace SW.Bitween.Resources.RetryPolicies; | ||
|
|
||
| /// <summary> | ||
| /// Reports how much of each group's total budget the integrations using this policy have spent, | ||
| /// so an exhausted group is visible instead of just silently declining to retry. | ||
| /// </summary> | ||
| [HandlerName("usage")] | ||
| public class Usage : ICommandHandler<int, RetryPolicyUsageRequest, object> | ||
| { | ||
| private readonly BitweenDbContext _dbContext; | ||
| private readonly RequestContext _requestContext; | ||
|
|
||
| public Usage(BitweenDbContext dbContext, RequestContext requestContext) | ||
| { | ||
| _dbContext = dbContext; | ||
| _requestContext = requestContext; | ||
| } | ||
|
|
||
| public async Task<object> Handle(int key, RetryPolicyUsageRequest request) | ||
|
hamzahalq marked this conversation as resolved.
|
||
| { | ||
| _requestContext.EnsureAccess(AccountRole.Admin, AccountRole.Member); | ||
|
|
||
| var policy = await _dbContext.Set<RetryPolicy>().AsNoTracking() | ||
| .FirstOrDefaultAsync(p => p.Id == key); | ||
| if (policy == null) throw new SWNotFoundException(key.ToString()); | ||
|
|
||
| var subscriptions = await _dbContext.Set<Subscription>().AsNoTracking() | ||
| .Where(s => s.RetryPolicyId == key) | ||
| .Select(s => new { s.Id, s.Name }) | ||
| .ToListAsync(); | ||
|
|
||
| var subscriptionIds = subscriptions.Select(s => s.Id).ToList(); | ||
|
|
||
| var usages = await _dbContext.Set<RetryGroupUsage>().AsNoTracking() | ||
| .Where(u => subscriptionIds.Contains(u.SubscriptionId)) | ||
| .ToListAsync(); | ||
|
|
||
| // Only groups that allow retries have a budget to spend. | ||
| var budgets = policy.Groups | ||
| .Where(g => g.Budget != null) | ||
| .ToDictionary(g => g.Id, g => new { g.Name, g.Budget.MaxAttemptsTotal }); | ||
|
|
||
| var names = subscriptions.ToDictionary(s => s.Id, s => s.Name); | ||
|
|
||
| var rows = usages | ||
| .Where(u => budgets.ContainsKey(u.GroupId)) | ||
| .Select(u => new RetryGroupUsageRow | ||
| { | ||
| SubscriptionId = u.SubscriptionId, | ||
| SubscriptionName = names.GetValueOrDefault(u.SubscriptionId), | ||
| GroupId = u.GroupId, | ||
| GroupName = budgets[u.GroupId].Name, | ||
| AttemptsUsed = u.AttemptsUsed, | ||
| MaxAttemptsTotal = budgets[u.GroupId].MaxAttemptsTotal, | ||
| Exhausted = u.AttemptsUsed >= budgets[u.GroupId].MaxAttemptsTotal, | ||
| LastAttemptOn = u.LastAttemptOn | ||
| }) | ||
| // Exhausted integrations first — those are the ones no longer being retried. | ||
| .OrderByDescending(r => r.Exhausted) | ||
| .ThenByDescending(r => r.AttemptsUsed) | ||
| .ToList(); | ||
|
|
||
| return new List<RetryGroupUsageRow>(rows); | ||
| } | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Handle a missing policy before reading
Groups.If
keydoes not identify a policy,FindAsyncreturnsnulland Line 34 throwsNullReferenceException. Return the handler's controlled not-found error before accessingpolicy.Groups.🤖 Prompt for AI Agents