Uh oh!
There was an error while loading. Please reload this page.
Enforce MaxAttemptsTotal across messages instead of per message - #242
Conversation
The group total was tracked in a dictionary carried on each xchange, so every failing message started from zero and got its own full budget: 4 messages under a total of 10 produced 12 retries. It also always equalled the per-message attempt count, so the cap could never fire above MaxAttemptsPerError. The total now lives in a RetryGroupUsage table keyed by integration + group, and the evaluator claims from it via IRetryGroupBudget. Dry-runs use an in-memory implementation so simulating never spends a real budget. GroupAttemptCounts is dropped from Xchange and DelayedRetry. The total never resets on its own, so /usage reports what each integration has spent and /resetusage clears it. XchangeResult now records why a retry was refused, since a group with an exhausted budget was previously indistinguishable from one that never matched.
📝 WalkthroughWhat changed
Riskrisk:medium Retry scheduling now depends on persistent shared state. Concurrent claims use conditional updates, but database-provider behavior and transaction isolation require validation. Existing callers must update for changed public APIs. Security-sensitive areas
Test coverage impact
Deployment and operational concerns
WalkthroughThe change replaces per-record retry-count dictionaries with persistent, subscription-scoped group usage. Retry evaluation is asynchronous and budget-backed. New usage and reset handlers expose counters. Retry-block reasons are stored and returned in exchange searches. Database migrations support SQL Server, MySQL, and PostgreSQL. ChangesShared retry budget
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@SW.Bitween.Api/Resources/RetryPolicies/ResetUsage.cs`:
- Around line 42-45: Persist the owning policy identifier on each
RetryGroupUsage row and update both handlers to query by that durable policy
scope instead of the current policy.Groups collection. In
SW.Bitween.Api/Resources/RetryPolicies/ResetUsage.cs lines 42-45, use the
persisted policy ownership so reset removes usage for deleted groups; in
SW.Bitween.Api/Resources/RetryPolicies/Usage.cs lines 42-50, report those
historical rows and return a null GroupName when the group no longer exists.
In `@SW.Bitween.Api/Resources/RetryPolicies/Usage.cs`:
- Around line 18-25: Update Usage.Handle to inject RequestContext and call
EnsureAccess with the policy key and permitted AccountRole.Admin and
AccountRole.Member roles before loading or returning retry-policy usage.
Preserve the existing usage response for authorized callers.
In `@SW.Bitween.Api/Services/RetryGroupBudget.cs`:
- Around line 23-45: Make RetryGroupBudget.TryConsume atomic by replacing the
read-modify-write logic with a conditional database increment that succeeds only
when AttemptsUsed is below maxAttemptsTotal, and handle absent rows through a
first-row upsert or duplicate-key retry. Keep the budget mutation and related
DelayedRetry insertion within the same explicit transaction, preserving the
false result when the shared budget is exhausted.
In `@SW.Bitween.Api/Services/XchangeService.cs`:
- Around line 428-435: Update the exception path around TryScheduleAutoRetry in
the XchangeService catch block so retry-scheduling or retry-related persistence
failures cannot replace the original failure or prevent XchangeResult from being
saved. Isolate scheduling and ensure the failure record is persisted through a
separate context or fallback save, while preserving the original exception
details in XchangeResult.
In `@SW.Bitween.IntegrationTests/Tests/RetryPolicyTests.cs`:
- Around line 369-377: The retry budget test currently serializes claims through
one DbContext and cannot expose races in RetryGroupBudget.TryConsume. Implement
atomic claim/concurrency control in RetryGroupBudget, then add a concurrent test
using separate service scopes and DbContext instances that performs 10 claims
and asserts exactly 10 succeed, preserving MaxAttemptsTotal enforcement.
In `@SW.Bitween.MsSql/Migrations/20260811081235_SharedRetryGroupTotals.cs`:
- Around line 14-34: Update the migration around RetryGroupUsages creation to
preserve existing GroupAttemptCounts: create the new table first, then backfill
one row per (SubscriptionId, GroupId) by aggregating active retry-chain counts
from Xchanges and DelayedRetries, including attempts already consumed by each
shared group. Only drop the source GroupAttemptCounts columns after the backfill
completes, and retain the composite primary key and required non-null fields.
In `@SW.Bitween.MySql/Migrations/20260811081221_SharedRetryGroupTotals.cs`:
- Around line 14-20: Update
SW.Bitween.MySql/Migrations/20260811081221_SharedRetryGroupTotals.cs lines 14-20
to create and backfill RetryGroupUsages from the authoritative
GroupAttemptCounts data for each subscription and group before dropping either
column, with explicit deduplication when Xchanges and DelayedRetries represent
the same retry state. Apply the equivalent create-and-backfill sequence in
SW.Bitween.PgSql/Migrations/20260811081200_SharedRetryGroupTotals.cs lines 15-23
for retry_group_usage before dropping group_attempt_counts.
In `@SW.Bitween.PgSql/Migrations/20260811092318_RetryBlockedReason.Designer.cs`:
- Around line 1925-1932: Remove the seeded default API credential object from
the HasData call in the generated model, including the deterministic Key value
and related default record. Provision the bootstrap credential through a secure
mechanism outside source control and rotate the exposed credential.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: simplify9/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 7f2320c9-d1ed-4b0b-b65e-196f5acf5d6e
📒 Files selected for processing (34)
SW.Bitween.Api/Data/BitweenDbContext.csSW.Bitween.Api/Domain/DelayedRetry.csSW.Bitween.Api/Domain/RetryGroupUsage.csSW.Bitween.Api/Domain/Xchange/Xchange.csSW.Bitween.Api/Domain/XchangeResult/XchangeResult.csSW.Bitween.Api/Resources/RetryPolicies/ResetUsage.csSW.Bitween.Api/Resources/RetryPolicies/Test.csSW.Bitween.Api/Resources/RetryPolicies/Usage.csSW.Bitween.Api/Resources/Xchanges/Search.csSW.Bitween.Api/Services/RetryGroupBudget.csSW.Bitween.Api/Services/XchangeService.csSW.Bitween.IntegrationTests/Tests/RetryJobTests.csSW.Bitween.IntegrationTests/Tests/RetryPolicyTests.csSW.Bitween.MsSql/Migrations/20260811081235_SharedRetryGroupTotals.Designer.csSW.Bitween.MsSql/Migrations/20260811081235_SharedRetryGroupTotals.csSW.Bitween.MsSql/Migrations/20260811092327_RetryBlockedReason.Designer.csSW.Bitween.MsSql/Migrations/20260811092327_RetryBlockedReason.csSW.Bitween.MsSql/Migrations/BitweenDbContextModelSnapshot.csSW.Bitween.MySql/Migrations/20260811081221_SharedRetryGroupTotals.Designer.csSW.Bitween.MySql/Migrations/20260811081221_SharedRetryGroupTotals.csSW.Bitween.MySql/Migrations/20260811092323_RetryBlockedReason.Designer.csSW.Bitween.MySql/Migrations/20260811092323_RetryBlockedReason.csSW.Bitween.MySql/Migrations/BitweenDbContextModelSnapshot.csSW.Bitween.PgSql/BitweenDbContext.csSW.Bitween.PgSql/Migrations/20260811081200_SharedRetryGroupTotals.Designer.csSW.Bitween.PgSql/Migrations/20260811081200_SharedRetryGroupTotals.csSW.Bitween.PgSql/Migrations/20260811092318_RetryBlockedReason.Designer.csSW.Bitween.PgSql/Migrations/20260811092318_RetryBlockedReason.csSW.Bitween.PgSql/Migrations/BitweenDbContextModelSnapshot.csSW.Bitween.Sdk/Model/AutoRetry/IRetryGroupBudget.csSW.Bitween.Sdk/Model/AutoRetry/RetryPolicyEvaluator.csSW.Bitween.Sdk/Model/RetryPolicyModel.csSW.Bitween.Sdk/Model/Xchange.csSW.Bitween.UnitTests/RetryPolicyEvaluatorTests.cs
💤 Files with no reviewable changes (1)
- SW.Bitween.Api/Domain/DelayedRetry.cs
📜 Review details
🧰 Additional context used
🪛 Betterleaks (1.7.3)
SW.Bitween.MsSql/Migrations/20260811092327_RetryBlockedReason.Designer.cs
[high] 1679-1679: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
SW.Bitween.MySql/Migrations/20260811081221_SharedRetryGroupTotals.Designer.cs
[high] 1672-1672: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
SW.Bitween.PgSql/Migrations/20260811081200_SharedRetryGroupTotals.Designer.cs
[high] 1925-1925: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
SW.Bitween.MsSql/Migrations/20260811081235_SharedRetryGroupTotals.Designer.cs
[high] 1675-1675: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
SW.Bitween.MySql/Migrations/20260811092323_RetryBlockedReason.Designer.cs
[high] 1676-1676: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
SW.Bitween.PgSql/Migrations/20260811092318_RetryBlockedReason.Designer.cs
[high] 1930-1930: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (14)
SW.Bitween.IntegrationTests/Tests/RetryPolicyTests.cs (2)
2-2: LGTM!
386-429: LGTM!Also applies to: 431-477, 479-508
SW.Bitween.MySql/Migrations/20260811092323_RetryBlockedReason.cs (1)
11-28: LGTM!SW.Bitween.PgSql/Migrations/20260811092318_RetryBlockedReason.cs (1)
11-29: LGTM!SW.Bitween.Sdk/Model/RetryPolicyModel.cs (1)
1-1: LGTM!Also applies to: 21-56
SW.Bitween.Api/Resources/Xchanges/Search.cs (1)
76-77: LGTM!SW.Bitween.Sdk/Model/Xchange.cs (1)
100-102: LGTM!SW.Bitween.Api/Resources/RetryPolicies/Test.cs (1)
24-24: LGTM!Also applies to: 33-40, 57-57
SW.Bitween.Sdk/Model/AutoRetry/IRetryGroupBudget.cs (1)
17-44: LGTM!SW.Bitween.Sdk/Model/AutoRetry/RetryPolicyEvaluator.cs (1)
13-23: LGTM!Also applies to: 43-43, 65-69
SW.Bitween.UnitTests/RetryPolicyEvaluatorTests.cs (1)
61-64: LGTM!Also applies to: 227-344, 378-440, 445-459
SW.Bitween.Api/Services/XchangeService.cs (2)
88-101: LGTM!Also applies to: 150-150
438-472: LGTM!SW.Bitween.IntegrationTests/Tests/RetryJobTests.cs (1)
125-128: LGTM!
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Claim a group slot with one conditional UPDATE instead of read-then-write, so concurrent failures across instances cannot both take the last slot. The first row is inserted on its own context, falling back to the increment if that race is lost. Guard retry evaluation so a throw there can no longer replace the original exception and discard the XchangeResult. Clear a group's usage rows when it is removed from a policy or the policy is deleted, and require Admin or Member to read usage.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
SW.Bitween.Api/Services/XchangeService.cs (1)
482-492: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftMake retry evaluation idempotent before claiming the budget.
Two workers can both observe no
DelayedRetryat Line 471. Both workers then claim a shared budget slot at Line 482 before oneDelayedRetryinsert fails on its primary key. This spends multiple slots for one failedXchangeand can make the group stop retrying early.Use an atomic retry-evaluation reservation keyed by
Xchange.Id, or otherwise serialize the existence check, budget claim, and delayed-retry creation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@SW.Bitween.Api/Services/XchangeService.cs` around lines 482 - 492, The retry flow around CountRetryChainDepth, evaluator.Evaluate, and DelayedRetry creation must atomically reserve evaluation by xchange.Id before claiming RetryGroupBudget. Serialize or otherwise make the existence check, budget claim, and DelayedRetry insertion one idempotent operation, ensuring concurrent workers cannot consume multiple budget slots for the same Xchange.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@SW.Bitween.Api/Resources/RetryPolicies/Delete.cs`:
- Around line 33-34: Update the delete handler’s flow after
FindAsync<RetryPolicy> to detect a null policy and return the handler’s existing
controlled not-found error before accessing policy.Groups. Preserve the current
group ID collection behavior for policies that are found.
In `@SW.Bitween.Api/Resources/RetryPolicies/Update.cs`:
- Around line 40-43: Coordinate removed-group cleanup in Update.cs (lines 40-43)
with the existing budget-claim invalidation mechanism so stale evaluators cannot
recreate usage after cleanup. Apply the same claim invalidation before deleting
policy usage in Delete.cs (lines 38-41); update the relevant retry-policy
cleanup methods while preserving their existing deletion behavior.
In `@SW.Bitween.Api/Services/RetryGroupBudget.cs`:
- Around line 68-70: Update the DbUpdateException handling in TryIncrement so it
retries only when a concurrent insert has created the expected (subscriptionId,
groupId) row; if no such row exists, rethrow the original DbUpdateException
instead of recording “Group total cap reached.”
---
Outside diff comments:
In `@SW.Bitween.Api/Services/XchangeService.cs`:
- Around line 482-492: The retry flow around CountRetryChainDepth,
evaluator.Evaluate, and DelayedRetry creation must atomically reserve evaluation
by xchange.Id before claiming RetryGroupBudget. Serialize or otherwise make the
existence check, budget claim, and DelayedRetry insertion one idempotent
operation, ensuring concurrent workers cannot consume multiple budget slots for
the same Xchange.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: simplify9/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: d2070cad-c581-427e-bdda-1a5c7a80a088
📒 Files selected for processing (6)
SW.Bitween.Api/Resources/RetryPolicies/Delete.csSW.Bitween.Api/Resources/RetryPolicies/Update.csSW.Bitween.Api/Resources/RetryPolicies/Usage.csSW.Bitween.Api/Services/RetryGroupBudget.csSW.Bitween.Api/Services/XchangeService.csSW.Bitween.IntegrationTests/Tests/RetryPolicyTests.cs
| var policy = await _dbContext.FindAsync<RetryPolicy>(key); | ||
| var groupIds = policy.Groups.Select(g => g.Id).ToList(); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Handle a missing policy before reading Groups.
If key does not identify a policy, FindAsync returns null and Line 34 throws NullReferenceException. Return the handler's controlled not-found error before accessing policy.Groups.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@SW.Bitween.Api/Resources/RetryPolicies/Delete.cs` around lines 33 - 34,
Update the delete handler’s flow after FindAsync<RetryPolicy> to detect a null
policy and return the handler’s existing controlled not-found error before
accessing policy.Groups. Preserve the current group ID collection behavior for
policies that are found.
| if (removedGroupIds.Count > 0) | ||
| await _dbContext.Set<RetryGroupUsage>() | ||
| .Where(u => removedGroupIds.Contains(u.GroupId)) | ||
| .ExecuteDeleteAsync(); |
There was a problem hiding this comment.
🗄️ 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.
SW.Bitween.Api/Resources/RetryPolicies/Update.cs#L40-L43: coordinate removed-group cleanup with budget claims so no stale evaluator can recreate usage.SW.Bitween.Api/Resources/RetryPolicies/Delete.cs#L38-L41: use the same claim-invalidation mechanism before deleting policy usage.
📍 Affects 2 files
SW.Bitween.Api/Resources/RetryPolicies/Update.cs#L40-L43(this comment)SW.Bitween.Api/Resources/RetryPolicies/Delete.cs#L38-L41
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@SW.Bitween.Api/Resources/RetryPolicies/Update.cs` around lines 40 - 43,
Coordinate removed-group cleanup in Update.cs (lines 40-43) with the existing
budget-claim invalidation mechanism so stale evaluators cannot recreate usage
after cleanup. Apply the same claim invalidation before deleting policy usage in
Delete.cs (lines 38-41); update the relevant retry-policy cleanup methods while
preserving their existing deletion behavior.
| catch (DbUpdateException) | ||
| { | ||
| return await TryIncrement(dbContext, groupId, maxAttemptsTotal); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bashset -e
file=$(fd -t f 'RetryGroupBudget\.cs$'.| head -n 1)printf'%s\n'"FILE: $file"
wc -l "$file"
sed -n '1,150p'"$file"printf'\nCALL SITES AND RELATED TYPES:\n'
rg -n -C 4 'TryIncrement|Group total cap reached|RetryGroupBudget|DbUpdateException' --glob '*.cs'.Repository: simplify9/Bitween-api
Length of output: 25933
🏁 Script executed:
#!/bin/bashset -e
printf'%s\n''RetryGroupUsage declarations and mappings:'
rg -n -C 8 'class RetryGroupUsage|RetryGroupUsage|HasKey|HasForeignKey|RetryGroupUsage' \
--glob '*.cs' SW.Bitween.Api SW.Bitween.Domain SW.Bitween.Model SW.Bitween.Infrastructure SW.Bitween.Migrations 2>/dev/null ||trueprintf'\nRelevant database migrations/schema references:\n'
rg -n -C 5 'RetryGroupUsage|AttemptsUsed|LastAttemptOn' --glob '*.cs' --glob '*.sql'.printf'\nException handling and scheduling path:\n'
sed -n '450,525p' SW.Bitween.Api/Services/XchangeService.cs
sed -n '55,75p' SW.Bitween.Sdk/Model/AutoRetry/RetryPolicyEvaluator.csRepository: simplify9/Bitween-api
Length of output: 50378
🏁 Script executed:
#!/bin/bashset -e
printf'%s\n''RetryGroupUsage migration definition:'
sed -n '20,75p' SW.Bitween.MsSql/Migrations/20260811081235_SharedRetryGroupTotals.cs
printf'\nRetry scheduling caller:\n'
sed -n '460,515p' SW.Bitween.Api/Services/XchangeService.cs
printf'\nRetry decision mapping:\n'
sed -n '55,75p' SW.Bitween.Sdk/Model/AutoRetry/RetryPolicyEvaluator.cs
printf'\nDatabase context construction and registration:\n'
rg -n -C 4 'AddDbContext|BitweenDbContext\(' --glob '*.cs' SW.Bitween.Api SW.Bitween.IntegrationTests | head -n 160
printf'\nFocused static behavior probe:\n'
python3 - <<'PY'from pathlib import Pathp = Path("SW.Bitween.Api/Services/RetryGroupBudget.cs")s = p.read_text()catch = s[s.index("catch (DbUpdateException)"):s.index("\n }\n }", s.index("catch (DbUpdateException)")) + len("\n }")]print("catch body:")print(catch)print("fallback returns bool:", "return await TryIncrement(dbContext, groupId, maxAttemptsTotal);" in catch)print("fallback can return false:", " > 0;" in s[s.index("private async Task<bool> TryIncrement"):])e = Path("SW.Bitween.Sdk/Model/AutoRetry/RetryPolicyEvaluator.cs").read_text()print("false maps to cap message:", 'Group total cap reached' in e and "if (!await groupBudget.TryConsume" in e)m = Path("SW.Bitween.MsSql/Migrations/20260811081235_SharedRetryGroupTotals.cs").read_text()section = m[m.index("migrationBuilder.CreateTable"):m.index(");", m.index("migrationBuilder.CreateTable"))+2]print("usage table has composite primary key:", "RetryGroupUsages" in section and "primaryKey:" in section)print("usage table has explicit foreign key:", "foreignKey:" in section)PYRepository: simplify9/Bitween-api
Length of output: 6894
Propagate non-duplicate insert failures
If the isolated insert fails and no (subscriptionId, groupId) row exists, rethrow the original DbUpdateException. Only retry TryIncrement when a concurrent insert created that row; otherwise the current code records "Group total cap reached" for a database failure.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@SW.Bitween.Api/Services/RetryGroupBudget.cs` around lines 68 - 70, Update the
DbUpdateException handling in TryIncrement so it retries only when a concurrent
insert has created the expected (subscriptionId, groupId) row; if no such row
exists, rethrow the original DbUpdateException instead of recording “Group total
cap reached.”
Uh oh!
There was an error while loading. Please reload this page.
The group total was tracked in a dictionary carried on each xchange, so every failing message started from zero and got its own full budget: 4 messages under a total of 10 produced 12 retries. It also always equalled the per-message attempt count, so the cap could never fire above MaxAttemptsPerError.
The total now lives in a RetryGroupUsage table keyed by integration + group, and the evaluator claims from it via IRetryGroupBudget. Dry-runs use an in-memory implementation so simulating never spends a real budget. GroupAttemptCounts is dropped from Xchange and DelayedRetry.
The total never resets on its own, so /usage reports what each integration has spent and /resetusage clears it. XchangeResult now records why a retry was refused, since a group with an exhausted budget was previously indistinguishable from one that never matched.