feat: replace ReceivingService/AggregationService with Quartz-backed … - #181
Conversation
…jobs via SW-Scheduler Replace the previous polling services with IScheduledJob<TParam> implementations (ReceivingJob, AggregationJob) driven by SW-Scheduler — a typed Quartz.NET wrapper. Quartz tables are added via EF Core migrations on all three DB providers (PgSql, MySql, MsSql). SchedulerSeedService re-registers active subscriptions idempotently on startup; SubscriptionSchedulerService bridges the Schedule domain entity to IScheduleRepository. Clustering is enabled so only one node fires each trigger in a multi-node deployment. Also adds SW.Bitween.IntegrationTests with Testcontainers-based tests for entity persistence, bus connectivity, receiving jobs, and aggregation jobs. Docs: docs/scheduler.md (new), docs/architecture.md (scheduler section added). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
|---|---|---|---|---|---|
| 20562020 | Triggered | Generic Password | 5f87616 | SW.Bitween.PgSql/BitweenDbContextFactory.cs | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secret safely. Learn here the best practices.
- Revoke and rotate this secret.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplaces polling services with Quartz-backed scheduling, adds provider-specific scheduler wiring and migrations, and introduces an auto-retry system with retry policy models, evaluation logic, delayed execution, API handlers, and test coverage. ChangesQuartz-based scheduling migration
Auto-Retry policy engine
Estimated code review effort: 5 (Critical) | ~120 minutes 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Nitpick comments (1)
SW.Bitween.Api/Services/SchedulerSeedService.cs (1)
31-45: ⚡ Quick winHonor cancellation during startup seeding loop.
The loop should stop promptly when shutdown is requested; otherwise startup seeding can delay host termination.
Suggested patch
foreach (var sub in subscriptions) { + if (stoppingToken.IsCancellationRequested) + break; + try { await subScheduler.ScheduleAll(sub);🤖 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/SchedulerSeedService.cs` around lines 31 - 45, The foreach loop in SchedulerSeedService that iterates through subscriptions and calls ScheduleAll does not check for cancellation requests, which can delay host shutdown. Modify the loop to check for cancellation by calling ThrowIfCancellationRequested on the CancellationToken parameter (ensure the method accepts a CancellationToken parameter if it does not already) and add this check at the beginning of each iteration before calling await subScheduler.ScheduleAll(sub), allowing the seeding process to be interrupted gracefully when shutdown is requested.
🤖 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 `@docs/architecture.md`:
- Around line 89-98: The fenced code block at line 89 in docs/architecture.md is
missing a language identifier on the opening fence, which triggers the MD040
markdown lint rule. Add a language identifier such as `text` to the opening
fence (change ``` to ```text) to specify the code block type and satisfy the
linting requirement.
In `@docs/scheduler.md`:
- Around line 108-111: The fenced code blocks in the scheduler.md file are
missing language identifiers, which violates markdownlint rule MD040. Add the
language identifier `text` to all fenced code blocks that lack one.
Specifically, add `text` to the opening fence for the code block at lines
108-111 that contains the receiver and aggregator subscription ID patterns, and
also add `text` to the opening fence for the code block at lines 141-147 that
contains the startup and SchedulerSeedService tree structure. Change each
opening ``` to ```text.
In `@SW.Bitween.Api/Resources/Subscriptions/Update.cs`:
- Around line 58-63: The scheduler operations (_subScheduler.Sync in Update.cs
at lines 58-63, _subScheduler.RunNow in AggregateNow.cs at lines 29-32, and
_subScheduler.RunNow in ReceiveNow.cs at lines 29-32) execute after database
commit without durability guarantees, causing transient failures to return
errors despite successful database changes. For each of the three affected
locations, refactor the scheduler call to route failures into a durable
reconciliation mechanism (such as a retry queue, outbox pattern, or background
repair job) instead of directly propagating the failure to the request handler.
This ensures that scheduler state can be reconciled asynchronously even if the
immediate scheduler operation fails, maintaining consistency between database
state and runtime scheduling.
In `@SW.Bitween.Api/Services/AggregationJob.cs`:
- Around line 30-37: The eligibility query in AggregationJob.cs uses incorrect
join correlations that risk matching on wrong keys and producing duplicate
xchange.Id candidates. In the xchangeQuery LINQ expression, the joins on lines
32-33 correlate using result.Id and agg.Id directly, but should instead use
proper foreign key relationships (such as result.XchangeId and agg.XchangeId) to
correctly associate XchangeResult and XchangeAggregation records with their
parent Xchange. Additionally, since multiple XchangeResult rows can exist for a
single Xchange, add a Distinct() call on xchange.Id within the query (before or
after the where clause) to eliminate duplicate candidates, or alternatively
refactor the joins to use Any() existence checks instead of left outer joins to
avoid duplication entirely. This ensures accurate aggregation mapping without
missed or duplicated entries.
In `@SW.Bitween.Api/Services/ReceivingJob.cs`:
- Around line 57-74: The receiver finalization is only executed on the success
path, causing resource leaks if exceptions occur during file operations. Wrap
the receiver initialization and the entire file processing loop (from
receiver.Initialize() to the foreach block) in a try block, and place the
receiver.Finalize() call in a finally block to guarantee it always executes.
Apply this same try/finally pattern to both the native adapter branch (starting
with nativeAdapterDiscovery.GetNativeReceiver) and the second branch mentioned
at lines 76-92 to ensure each receiver's finalization method is always called
regardless of success or failure.
In `@SW.Bitween.Api/Services/SubscriptionSchedulerService.cs`:
- Around line 21-26: The Sync method currently unschedules all old schedules
before attempting to schedule new ones. If any Schedule call fails, the
subscription loses trigger coverage even though the domain changes were already
committed. Refactor the logic in the Sync method to reverse the order: first
call Schedule for all new schedules in sub.Schedules, then afterward call
TryUnschedule only for the schedules that existed in oldSchedules but are no
longer in sub.Schedules. This way, existing trigger coverage is preserved if any
new schedule operations fail. Alternatively or additionally, consider wrapping
both the unschedule and schedule operations in a transactional boundary to
ensure that if any operation fails, no triggers are lost.
In `@SW.Bitween.IntegrationTests/Fixtures/BitweenFixture.cs`:
- Around line 46-49: The BitweenFixture class creates NpgsqlDataSource and a
host without storing them or disposing them, causing resource leaks. Store both
the host (created before line 46) and the dataSource variable (from
dataSourceBuilder.Build() at line 46-49) as private fields in the BitweenFixture
class, then ensure both are properly disposed in the fixture's Dispose method or
cleanup logic. Apply the same fix to the similar code at lines 119-124 where
another data source or host is created without being stored and disposed.
In `@SW.Bitween.IntegrationTests/Tests/AggregationTests.cs`:
- Around line 67-69: The test currently uses FirstOrDefaultAsync() to retrieve
the aggregation Xchange and then only asserts it is not null, which allows
duplicate rows to pass since FirstOrDefaultAsync returns the first match even if
multiple exist. Replace the FirstOrDefaultAsync query with CountAsync() to get
the total count of matching Xchange rows where SubscriptionId equals aggSub.Id,
then assert that the count equals exactly 1 using Assert.Equal. This will
enforce that exactly one aggregation Xchange row exists and catch any
duplicate-row regressions.
In `@SW.Bitween.MsSql/BitweenDbContextFactory.cs`:
- Around line 11-13: The connStr assignment in BitweenDbContextFactory.cs
contains a hardcoded privileged sa password in the fallback connection string,
which is a security credential leak. Remove the fallback default connection
string that includes credentials entirely, and instead throw an
InvalidOperationException or similar exception if the
"ConnectionStrings__BitweenDb" environment variable is not set. This ensures
credentials are never embedded in source code and must be provided via
environment configuration in any deployment environment.
In `@SW.Bitween.MsSql/Migrations/20260614184136_Quartz.Designer.cs`:
- Around line 1477-1487: The HasData call in the migration contains a hardcoded
API key "7facc758283844b49cc4ffd26a75b1de" which poses a security risk by
exposing credentials in version control. Remove the hardcoded Key value from the
seed data in the HasData method call, and instead implement environment-based
seeding through a separate configuration file or seeding strategy that is
excluded from version control, ensuring that credentials are loaded from secure
configuration sources only during runtime rather than being embedded in the
migration code itself.
In `@SW.Bitween.MySql/Migrations/20260614184126_Quartz.cs`:
- Around line 224-229: The MySQL migration contains FK constraint names with
invalid `~` character suffixes (e.g.,
FK_QRTZ_blob_triggers_QRTZ_triggers_sched_name_trigger_name_tri~) that are
non-standard in SQL and likely invalid in MySQL, and these explicit names are
not reflected in the model snapshot, causing a schema desync. Choose one
approach: (1) Add explicit HasConstraintName() calls in the model snapshot for
all four QRTZ foreign keys (blob_triggers, cron_triggers, simple_triggers, and
simprop_triggers) using valid constraint names without the `~` suffix and
respecting MySQL's 64-character identifier limit, or (2) Remove the explicit
name: parameters from all four ForeignKey definitions in the migration file and
let EF Core auto-generate consistent constraint names. Either approach will
ensure the migration and snapshot are synchronized with valid constraint names.
In `@SW.Bitween.MySql/Migrations/20260614184126_Quartz.Designer.cs`:
- Around line 1477-1484: The HasData() call in the migration file
(SW.Bitween.MySql/Migrations/20260614184126_Quartz.Designer.cs) contains a
hardcoded API key "7facc758283844b49cc4ffd26a75b1de" for PartnerApiCredentials
seeding, which creates a security vulnerability by making a predictable
credential available wherever migrations are applied. Remove this deterministic
API key from the HasData() configuration and either relocate API credential
provisioning to a separate secure provisioning flow outside of migrations, or
seed a disabled/non-authenticating placeholder credential instead. After
updating the model seed configuration to remove or disable the hardcoded key,
regenerate this migration file and its snapshot from the updated seed data so
the changes are properly reflected in the migration artifacts.
In `@SW.Bitween.PgSql/Migrations/20260614184104_Quartz.Designer.cs`:
- Around line 1685-1692: The HasData call in the migration contains a hardcoded
API key value ("7facc758283844b49cc4ffd26a75b1de") which is a security risk.
Remove the Key property from the hardcoded seed data in the migration file, and
instead implement secure credential provisioning at runtime using
environment-specific secrets or a secure key management system. After making
these changes, regenerate the migration files and snapshots to ensure they
reflect the updated configuration without the hardcoded credential.
In `@SW.Bitween.Web/Startup.cs`:
- Around line 141-165: The scheduler registration methods (AddPgSqlScheduler,
AddSqlServerScheduler, and AddMySqlScheduler) are being called with the
connectionString before the Authentication=Active Directory Default
configuration is appended to it at line 244. To fix this, move the connection
string authentication configuration logic to execute before the scheduler
registration block, ensuring both the Quartz scheduler and Entity Framework
receive the same properly-configured connection string with authentication
details included.
---
Nitpick comments:
In `@SW.Bitween.Api/Services/SchedulerSeedService.cs`:
- Around line 31-45: The foreach loop in SchedulerSeedService that iterates
through subscriptions and calls ScheduleAll does not check for cancellation
requests, which can delay host shutdown. Modify the loop to check for
cancellation by calling ThrowIfCancellationRequested on the CancellationToken
parameter (ensure the method accepts a CancellationToken parameter if it does
not already) and add this check at the beginning of each iteration before
calling await subScheduler.ScheduleAll(sub), allowing the seeding process to be
interrupted gracefully when shutdown is requested.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1d756b2c-0dfc-44c8-a2ae-f5b922240fbe
📒 Files selected for processing (41)
SW.Bitween.Api/Extensions/ScheduleToCronExtension.csSW.Bitween.Api/Resources/Subscriptions/AggregateNow.csSW.Bitween.Api/Resources/Subscriptions/ReceiveNow.csSW.Bitween.Api/Resources/Subscriptions/Update.csSW.Bitween.Api/SW.Bitween.Api.csprojSW.Bitween.Api/Services/AggregationJob.csSW.Bitween.Api/Services/AggregationService.csSW.Bitween.Api/Services/ReceivingJob.csSW.Bitween.Api/Services/ReceivingService.csSW.Bitween.Api/Services/SchedulerSeedService.csSW.Bitween.Api/Services/SubscriptionSchedulerService.csSW.Bitween.IntegrationTests/Adapters/NativeTestReceiver.csSW.Bitween.IntegrationTests/Fixtures/BitweenFixture.csSW.Bitween.IntegrationTests/SW.Bitween.IntegrationTests.csprojSW.Bitween.IntegrationTests/Tests/AggregationTests.csSW.Bitween.IntegrationTests/Tests/BusTests.csSW.Bitween.IntegrationTests/Tests/EntityTests.csSW.Bitween.IntegrationTests/Tests/ReceivingTests.csSW.Bitween.MsSql/BitweenDbContext.csSW.Bitween.MsSql/BitweenDbContextFactory.csSW.Bitween.MsSql/Migrations/20260614184136_Quartz.Designer.csSW.Bitween.MsSql/Migrations/20260614184136_Quartz.csSW.Bitween.MsSql/Migrations/BitweenDbContextModelSnapshot.csSW.Bitween.MsSql/SW.Bitween.MsSql.csprojSW.Bitween.MySql/BitweenDbContext.csSW.Bitween.MySql/BitweenDbContextFactory.csSW.Bitween.MySql/Migrations/20260614184126_Quartz.Designer.csSW.Bitween.MySql/Migrations/20260614184126_Quartz.csSW.Bitween.MySql/Migrations/BitweenDbContextModelSnapshot.csSW.Bitween.MySql/SW.Bitween.MySql.csprojSW.Bitween.PgSql/BitweenDbContext.csSW.Bitween.PgSql/BitweenDbContextFactory.csSW.Bitween.PgSql/Migrations/20260614184104_Quartz.Designer.csSW.Bitween.PgSql/Migrations/20260614184104_Quartz.csSW.Bitween.PgSql/Migrations/BitweenDbContextModelSnapshot.csSW.Bitween.PgSql/SW.Bitween.PgSql.csprojSW.Bitween.Web/SW.Bitween.Web.csprojSW.Bitween.Web/Startup.csSW.Bitween.slndocs/architecture.mddocs/scheduler.md
💤 Files with no reviewable changes (2)
- SW.Bitween.Api/Services/AggregationService.cs
- SW.Bitween.Api/Services/ReceivingService.cs
| ``` | ||
| SchedulerSeedService (startup) | ||
| └── SubscriptionSchedulerService.ScheduleAll(sub) | ||
| └── IScheduleRepository.ScheduleIfNotExists<Job, Param>(param, cron, key) | ||
| └── Quartz persistent job store (qrtz_* tables, same DB as Bitween) | ||
|
|
||
| Quartz fires at scheduled time | ||
| └── ReceivingJob.Execute(ReceivingJobParams) ← polls receiver adapter, creates Xchanges | ||
| └── AggregationJob.Execute(AggregationJobParams) ← batches successful Xchanges into one | ||
| ``` |
There was a problem hiding this comment.
Add a language identifier to the fenced code block.
Line 89 opens a fenced block without a language token, which triggers MD040. Use something like ```text for this diagram block.
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 89-89: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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 `@docs/architecture.md` around lines 89 - 98, The fenced code block at line 89
in docs/architecture.md is missing a language identifier on the opening fence,
which triggers the MD040 markdown lint rule. Add a language identifier such as
`text` to the opening fence (change ``` to ```text) to specify the code block
type and satisfy the linting requirement.
Source: Linters/SAST tools
| ``` | ||
| receiver-{subscriptionId}-{recurrence}-{on.Ticks}-{backwards ? 1 : 0} | ||
| aggregator-{subscriptionId}-{recurrence}-{on.Ticks}-{backwards ? 1 : 0} | ||
| ``` |
There was a problem hiding this comment.
Add language identifiers to fenced code blocks.
These fences are missing language tags, which triggers markdownlint MD040.
Suggested patch
-```
+```text
receiver-{subscriptionId}-{recurrence}-{on.Ticks}-{backwards ? 1 : 0}
aggregator-{subscriptionId}-{recurrence}-{on.Ticks}-{backwards ? 1 : 0}- +text
startup
└── SchedulerSeedService.ExecuteAsync()
├── query all active Receiving + Aggregation subscriptions with at least one Schedule
</details>
Also applies to: 141-147
<details>
<summary>🧰 Tools</summary>
<details>
<summary>🪛 markdownlint-cli2 (0.22.1)</summary>
[warning] 108-108: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
</details>
</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @docs/scheduler.md around lines 108 - 111, The fenced code blocks in the
scheduler.md file are missing language identifiers, which violates markdownlint
rule MD040. Add the language identifier text to all fenced code blocks that
lack one. Specifically, add text to the opening fence for the code block at
lines 108-111 that contains the receiver and aggregator subscription ID
patterns, and also add text to the opening fence for the code block at lines
141-147 that contains the startup and SchedulerSeedService tree structure.
Change each opening totext.
</details>
<!-- fingerprinting:phantom:poseidon:hawk -->
<!-- cr-comment:v1:2c0566807a1294aa279f7270 -->
_Source: Linters/SAST tools_
<!-- This is an auto-generated comment by CodeRabbit -->
| await _dbContext.SaveChangesAsync(); | ||
| await _BitweenCache.BroadcastRevoke(); | ||
|
|
||
| // Sync Quartz: unschedule removed entries, schedule new/kept ones. | ||
| await _subScheduler.Sync(entity, oldSchedules); | ||
|
|
There was a problem hiding this comment.
Post-commit scheduler calls are not durable across command handlers.
All three handlers commit DB state first and then invoke scheduler operations without retry/compensation, so transient scheduler failures can return errors after a successful commit and leave runtime scheduling stale.
SW.Bitween.Api/Resources/Subscriptions/Update.cs#L58-L63: route_subScheduler.Sync(...)failures into a durable reconciliation path (retry queue/outbox/background repair), not immediate request failure only.SW.Bitween.Api/Resources/Subscriptions/AggregateNow.cs#L29-L32: make_subScheduler.RunNow(entity)resilient post-commit (retry/compensation) to avoid failed-response-after-commit behavior.SW.Bitween.Api/Resources/Subscriptions/ReceiveNow.cs#L29-L32: apply the same resilient post-commit scheduling strategy as above for symmetry and consistent semantics.
📍 Affects 3 files
SW.Bitween.Api/Resources/Subscriptions/Update.cs#L58-L63(this comment)SW.Bitween.Api/Resources/Subscriptions/AggregateNow.cs#L29-L32SW.Bitween.Api/Resources/Subscriptions/ReceiveNow.cs#L29-L32
🤖 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/Subscriptions/Update.cs` around lines 58 - 63, The
scheduler operations (_subScheduler.Sync in Update.cs at lines 58-63,
_subScheduler.RunNow in AggregateNow.cs at lines 29-32, and _subScheduler.RunNow
in ReceiveNow.cs at lines 29-32) execute after database commit without
durability guarantees, causing transient failures to return errors despite
successful database changes. For each of the three affected locations, refactor
the scheduler call to route failures into a durable reconciliation mechanism
(such as a retry queue, outbox pattern, or background repair job) instead of
directly propagating the failure to the request handler. This ensures that
scheduler state can be reconciled asynchronously even if the immediate scheduler
operation fails, maintaining consistency between database state and runtime
scheduling.
| var xchangeQuery = | ||
| from xchange in dbContext.Set<Xchange>() | ||
| join result in dbContext.Set<XchangeResult>() on xchange.Id equals result.Id | ||
| join agg in dbContext.Set<XchangeAggregation>() on xchange.Id equals agg.Id into xa | ||
| from agg in xa.DefaultIfEmpty() | ||
| where result.Success == true && agg == null && | ||
| xchange.SubscriptionId == aggSub.AggregationForId && !aggSub.Inactive | ||
| select xchange.Id; |
There was a problem hiding this comment.
Fix eligibility query to correlate by foreign keys and avoid duplicate candidates.
Line 32 and Line 33 correlate using result.Id / agg.Id, which risks matching on the wrong key path. Also, joining directly to XchangeResult can duplicate xchange.Id when multiple successful rows exist. This can cause missed or duplicated aggregation mappings. Use FK-based existence checks (Any) or Distinct on xchange.Id before Take(10000).
🤖 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/AggregationJob.cs` around lines 30 - 37, The
eligibility query in AggregationJob.cs uses incorrect join correlations that
risk matching on wrong keys and producing duplicate xchange.Id candidates. In
the xchangeQuery LINQ expression, the joins on lines 32-33 correlate using
result.Id and agg.Id directly, but should instead use proper foreign key
relationships (such as result.XchangeId and agg.XchangeId) to correctly
associate XchangeResult and XchangeAggregation records with their parent
Xchange. Additionally, since multiple XchangeResult rows can exist for a single
Xchange, add a Distinct() call on xchange.Id within the query (before or after
the where clause) to eliminate duplicate candidates, or alternatively refactor
the joins to use Any() existence checks instead of left outer joins to avoid
duplication entirely. This ensures accurate aggregation mapping without missed
or duplicated entries.
| if (serverlessId.StartsWith(NativeAdapterDiscoveryService.NativePrefix, StringComparison.OrdinalIgnoreCase)) | ||
| { | ||
| var receiver = nativeAdapterDiscovery.GetNativeReceiver(serverlessId, startupParameters); | ||
| await receiver.Initialize(); | ||
| var fileList = (await receiver.ListFiles()).ToList(); | ||
|
|
||
| logger.LogInformation("Subscription '{SubId}' found {Count} items for retrieval.", subId, fileList.Count); | ||
|
|
||
| foreach (var file in fileList) | ||
| { | ||
| var xchangeFile = await receiver.GetFile(file); | ||
| logger.LogInformation("Submitting received file for subscriber: '{SubId}'.", subId); | ||
| await xchangeService.SubmitSubscriptionXchange(subId, xchangeFile); | ||
| await receiver.DeleteFile(file); | ||
| } | ||
|
|
||
| await receiver.Finalize(); | ||
| } |
There was a problem hiding this comment.
Guarantee receiver finalization with try/finally in both execution branches.
Line 73 and Line 91 run only on the success path. If any file operation throws, finalization is skipped and remote/session resources can be left open. Wrap each branch in try/finally and always call the corresponding finalize method.
Also applies to: 76-92
🤖 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/ReceivingJob.cs` around lines 57 - 74, The receiver
finalization is only executed on the success path, causing resource leaks if
exceptions occur during file operations. Wrap the receiver initialization and
the entire file processing loop (from receiver.Initialize() to the foreach
block) in a try block, and place the receiver.Finalize() call in a finally block
to guarantee it always executes. Apply this same try/finally pattern to both the
native adapter branch (starting with nativeAdapterDiscovery.GetNativeReceiver)
and the second branch mentioned at lines 76-92 to ensure each receiver's
finalization method is always called regardless of success or failure.
| var connStr = Environment.GetEnvironmentVariable("ConnectionStrings__BitweenDb") | ||
| ?? "Server=localhost,1433;Database=bitween;User Id=sa;Password=Pass@word123;TrustServerCertificate=True"; | ||
|
|
There was a problem hiding this comment.
Remove hardcoded SQL credentials from the design-time fallback.
Line 12 embeds a privileged sa password in source control. Even for tooling, this is a credential leak risk and can be reused unintentionally in shared environments.
Suggested fix
var connStr = Environment.GetEnvironmentVariable("ConnectionStrings__BitweenDb")
- ?? "Server=localhost,1433;Database=bitween;User Id=sa;Password=Pass@word123;TrustServerCertificate=True";
+ ?? throw new InvalidOperationException(
+ "ConnectionStrings__BitweenDb must be set for design-time migrations.");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var connStr = Environment.GetEnvironmentVariable("ConnectionStrings__BitweenDb") | |
| ?? "Server=localhost,1433;Database=bitween;User Id=sa;Password=Pass@word123;TrustServerCertificate=True"; | |
| var connStr = Environment.GetEnvironmentVariable("ConnectionStrings__BitweenDb") | |
| ?? throw new InvalidOperationException( | |
| "ConnectionStrings__BitweenDb must be set for design-time migrations."); |
🤖 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.MsSql/BitweenDbContextFactory.cs` around lines 11 - 13, The
connStr assignment in BitweenDbContextFactory.cs contains a hardcoded privileged
sa password in the fallback connection string, which is a security credential
leak. Remove the fallback default connection string that includes credentials
entirely, and instead throw an InvalidOperationException or similar exception if
the "ConnectionStrings__BitweenDb" environment variable is not set. This ensures
credentials are never embedded in source code and must be provided via
environment configuration in any deployment environment.
| b1.WithOwner() | ||
| .HasForeignKey("PartnerId"); | ||
|
|
||
| b1.HasData( | ||
| new | ||
| { | ||
| PartnerId = 1, | ||
| Id = 1, | ||
| Key = "7facc758283844b49cc4ffd26a75b1de", | ||
| Name = "default" | ||
| }); |
There was a problem hiding this comment.
Hardcoded API key in seed data is a security hygiene concern.
Line 1482 contains a hardcoded default API key "7facc758283844b49cc4ffd26a75b1de" that is visible in the migration snapshot and version control. Even though this is test/seed data intended for local development, storing credentials in code and checked-in migrations increases exposure risk.
Recommendation: Consider moving seed data (especially credentials) to a separate configuration file or environment-based seeding that is excluded from version control, or document that this key is non-production only.
🧰 Tools
🪛 Betterleaks (1.3.1)
[high] 1485-1485: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🤖 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.MsSql/Migrations/20260614184136_Quartz.Designer.cs` around lines
1477 - 1487, The HasData call in the migration contains a hardcoded API key
"7facc758283844b49cc4ffd26a75b1de" which poses a security risk by exposing
credentials in version control. Remove the hardcoded Key value from the seed
data in the HasData method call, and instead implement environment-based seeding
through a separate configuration file or seeding strategy that is excluded from
version control, ensuring that credentials are loaded from secure configuration
sources only during runtime rather than being embedded in the migration code
itself.
| table.ForeignKey( | ||
| name: "FK_QRTZ_blob_triggers_QRTZ_triggers_sched_name_trigger_name_tri~", | ||
| columns: x => new { x.sched_name, x.trigger_name, x.trigger_group }, | ||
| principalTable: "QRTZ_triggers", | ||
| principalColumns: new[] { "sched_name", "trigger_name", "trigger_group" }, | ||
| onDelete: ReferentialAction.Cascade); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify MySQL constraint name truncation consistency across migration files
# Extract all FK constraint names from the MySQL migration
echo "=== MySQL Migration Constraint Names ==="
rg -n "name:\s*\"FK_" SW.Bitween.MySql/Migrations/20260614184126_Quartz.cs | head -20
# Extract all FK constraint names from the MsSql migration for comparison
echo "=== MsSql Migration Constraint Names (for comparison) ==="
rg -n "name:\s*\"FK_" SW.Bitween.MsSql/Migrations/20260614184136_Quartz.cs | head -20
# Count unique constraint names to check for collisions
echo "=== Checking for potential name collisions in MySQL migration ==="
rg "name:\s*\"FK_" SW.Bitween.MySql/Migrations/20260614184126_Quartz.cs | wc -lRepository: simplify9/Bitween-api
Length of output: 1291
🏁 Script executed:
# Read the exact lines from the migration file to see actual constraint names
sed -n '200,330p' SW.Bitween.MySql/Migrations/20260614184126_Quartz.cs | grep -A 5 "ForeignKey"Repository: simplify9/Bitween-api
Length of output: 2298
🏁 Script executed:
# Search for model snapshots that might contain these constraint names
find . -name "*ModelSnapshot*" -o -name "*snapshot*" | grep -i mysql | head -5Repository: simplify9/Bitween-api
Length of output: 128
🏁 Script executed:
# Check the model snapshot for FK constraint names
rg -n "FK_QRTZ_" ./SW.Bitween.MySql/Migrations/BitweenDbContextModelSnapshot.cs | grep -i constraintRepository: simplify9/Bitween-api
Length of output: 47
🏁 Script executed:
# Get more context around FK constraint definitions in the snapshot
rg -B 2 -A 2 "FK_QRTZ_blob_triggers_QRTZ_triggers" ./SW.Bitween.MySql/Migrations/BitweenDbContextModelSnapshot.cs | head -20Repository: simplify9/Bitween-api
Length of output: 47
🏁 Script executed:
# View the structure of the model snapshot file
head -100 ./SW.Bitween.MySql/Migrations/BitweenDbContextModelSnapshot.csRepository: simplify9/Bitween-api
Length of output: 3769
🏁 Script executed:
# Search for any QRTZ references in the snapshot
rg "QRTZ_" ./SW.Bitween.MySql/Migrations/BitweenDbContextModelSnapshot.cs | head -10Repository: simplify9/Bitween-api
Length of output: 744
🏁 Script executed:
# Search for FK constraint definitions in snapshot
rg "HasForeignKey|ForeignKey.*sched_name" ./SW.Bitween.MySql/Migrations/BitweenDbContextModelSnapshot.cs | head -20Repository: simplify9/Bitween-api
Length of output: 1335
🏁 Script executed:
# Look for constraint name definitions in snapshot
rg "HasConstraintName|HasName.*FK_" ./SW.Bitween.MySql/Migrations/BitweenDbContextModelSnapshot.cs | head -20Repository: simplify9/Bitween-api
Length of output: 208
🏁 Script executed:
# Search for QRTZ FK constraint names with more context
rg -B 2 -A 2 "SchedulerName.*TriggerName.*TriggerGroup" ./SW.Bitween.MySql/Migrations/BitweenDbContextModelSnapshot.csRepository: simplify9/Bitween-api
Length of output: 2536
🏁 Script executed:
# Get more context around QRTZ FK definitions in snapshot
rg -B 5 -A 5 "FK_QRTZ_blob_triggers|blob_triggers.*HasForeignKey" ./SW.Bitween.MySql/Migrations/BitweenDbContextModelSnapshot.cs | head -40Repository: simplify9/Bitween-api
Length of output: 47
🏁 Script executed:
# Check if other migrations also have truncated FK names with ~
find . -path "*/Migrations/*Quartz.cs" -type f | xargs grep -l "FK_QRTZ" | head -10Repository: simplify9/Bitween-api
Length of output: 175
🏁 Script executed:
# Compare MsSql and MySql migrations side by side for constraint name patterns
echo "=== MsSql constraint name count ===" && \
rg "name:\s*\"FK_" ./SW.Bitween.MsSql/Migrations/20260614184136_Quartz.cs | wc -l && \
echo "=== MySql constraint name count ===" && \
rg "name:\s*\"FK_" ./SW.Bitween.MySql/Migrations/20260614184126_Quartz.cs | wc -lRepository: simplify9/Bitween-api
Length of output: 141
🏁 Script executed:
# Look for any database execution or errors related to these constraint names
rg "tri~|trigger_name_" . --type-list | head -10Repository: simplify9/Bitween-api
Length of output: 455
🏁 Script executed:
# Check git history for this migration file to see when it was created and any related commits
git log --oneline -n 5 -- SW.Bitween.MySql/Migrations/20260614184126_Quartz.cs 2>/dev/null | head -10Repository: simplify9/Bitween-api
Length of output: 164
🏁 Script executed:
# Check if there are any comments in the migration about the truncated names
grep -i "truncat\|constraint.*name\|identif" SW.Bitween.MySql/Migrations/20260614184126_Quartz.cs | head -10Repository: simplify9/Bitween-api
Length of output: 47
🏁 Script executed:
# Verify MySQL constraint naming rules - search for any documentation or tests
rg "constraint.*name|identifier.*length" . --type md | head -10Repository: simplify9/Bitween-api
Length of output: 47
Fix MySQL foreign key constraint names—snapshot and migration are out of sync, and constraint names appear invalid.
The MySQL migration file explicitly specifies FK constraint names with ~ character suffixes (e.g., "FK_QRTZ_blob_triggers_QRTZ_triggers_sched_name_trigger_name_tri~"), but the model snapshot does not include corresponding HasConstraintName() calls for these QRTZ FKs. This creates two problems:
-
Invalid Constraint Names: The
~character is non-standard in SQL identifiers and likely invalid in MySQL. MySQL identifier rules typically allow alphanumeric characters, underscores, and dollar signs only. -
Schema Desync: When the migration runs, EF Core will auto-generate constraint names based on the snapshot's
HasForeignKey()definitions (without explicit names), which may differ from the manually truncated names in the migration file. This causes the actual database schema to diverge from the migration's intent.
Required action: Either:
- Add explicit
HasConstraintName()calls to the model snapshot for all QRTZ FK constraints (lines matching blob_triggers, cron_triggers, simple_triggers, simprop_triggers), using valid constraint names (remove the~suffix and properly truncate to MySQL's 64-character identifier limit), OR - Remove explicit
name:parameters from the migration and let EF Core auto-generate consistent names.
Apply to all four affected FKs: blob_triggers (224–229), cron_triggers (251–256), simple_triggers (277–282), simprop_triggers (316–321).
🤖 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.MySql/Migrations/20260614184126_Quartz.cs` around lines 224 - 229,
The MySQL migration contains FK constraint names with invalid `~` character
suffixes (e.g.,
FK_QRTZ_blob_triggers_QRTZ_triggers_sched_name_trigger_name_tri~) that are
non-standard in SQL and likely invalid in MySQL, and these explicit names are
not reflected in the model snapshot, causing a schema desync. Choose one
approach: (1) Add explicit HasConstraintName() calls in the model snapshot for
all four QRTZ foreign keys (blob_triggers, cron_triggers, simple_triggers, and
simprop_triggers) using valid constraint names without the `~` suffix and
respecting MySQL's 64-character identifier limit, or (2) Remove the explicit
name: parameters from all four ForeignKey definitions in the migration file and
let EF Core auto-generate consistent constraint names. Either approach will
ensure the migration and snapshot are synchronized with valid constraint names.
| b1.HasData( | ||
| new | ||
| { | ||
| PartnerId = 1, | ||
| Id = 1, | ||
| Key = "7facc758283844b49cc4ffd26a75b1de", | ||
| Name = "default" | ||
| }); |
There was a problem hiding this comment.
Avoid deterministic seeded API keys in migration artifacts.
Lines 1477-1484 seed a fixed API key into PartnerApiCredentials. That makes a predictable credential available wherever migrations are applied unless manually rotated, which weakens auth guarantees.
Please move this to a secure provisioning flow (or seed a disabled/non-auth credential), then regenerate this migration/snapshot from the updated model seed configuration.
🧰 Tools
🪛 Betterleaks (1.3.1)
[high] 1482-1482: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🤖 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.MySql/Migrations/20260614184126_Quartz.Designer.cs` around lines
1477 - 1484, The HasData() call in the migration file
(SW.Bitween.MySql/Migrations/20260614184126_Quartz.Designer.cs) contains a
hardcoded API key "7facc758283844b49cc4ffd26a75b1de" for PartnerApiCredentials
seeding, which creates a security vulnerability by making a predictable
credential available wherever migrations are applied. Remove this deterministic
API key from the HasData() configuration and either relocate API credential
provisioning to a separate secure provisioning flow outside of migrations, or
seed a disabled/non-authenticating placeholder credential instead. After
updating the model seed configuration to remove or disable the hardcoded key,
regenerate this migration file and its snapshot from the updated seed data so
the changes are properly reflected in the migration artifacts.
Source: Linters/SAST tools
| b1.HasData( | ||
| new | ||
| { | ||
| PartnerId = 1, | ||
| Id = 1, | ||
| Key = "7facc758283844b49cc4ffd26a75b1de", | ||
| Name = "default" | ||
| }); |
There was a problem hiding this comment.
Remove the hardcoded API credential seed value.
Seeding a fixed API key in migration/model artifacts creates a predictable credential shared across deployments. Move to secure runtime provisioning (or generate per-environment secret) and regenerate migrations/snapshots.
🧰 Tools
🪛 Betterleaks (1.3.1)
[high] 1690-1690: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🤖 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.PgSql/Migrations/20260614184104_Quartz.Designer.cs` around lines
1685 - 1692, The HasData call in the migration contains a hardcoded API key
value ("7facc758283844b49cc4ffd26a75b1de") which is a security risk. Remove the
Key property from the hardcoded seed data in the migration file, and instead
implement secure credential provisioning at runtime using environment-specific
secrets or a secure key management system. After making these changes,
regenerate the migration files and snapshots to ensure they reflect the updated
configuration without the hardcoded credential.
Source: Linters/SAST tools
Swap all local ProjectReferences to SW-SimplyScheduler with the published
NuGet packages (SimplyWorks.Scheduler.* v8.1.1). Each project now references
the correct package for its role:
SW.Bitween.Api → SimplyWorks.Scheduler.Sdk (IScheduledJob, IScheduleRepository)
SW.Bitween.PgSql → SimplyWorks.Scheduler.PgSql
SW.Bitween.MySql → SimplyWorks.Scheduler.MySql
SW.Bitween.MsSql → SimplyWorks.Scheduler.SqlServer
SW.Bitween.Web → SimplyWorks.Scheduler.EfCore (provider packages come
transitively through the DB provider projects)
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Replace the package layout table with the actual NuGet package names and versions (SimplyWorks.Scheduler.* v8.1.1) following the project reference → NuGet migration. Clarifies which project references which package and why. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
SW.Bitween.Api/Services/XchangeService.cs (1)
360-401: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAuto-retry trigger misses mapper-only bad-output failures.
TryScheduleAutoRetryonly fires onresponseFile?.BadData == true. Whenxchange.HandlerId == null,RunHandlerreturnsnullunconditionally (see line 225), soresponseFilestaysnulleven thoughoutputFile.BadData(fromRunMapper) may betrue. For subscriptions configured with a Mapper but no Handler, a bad mapper output is recorded inXchangeResult.OutputBadbut never evaluated for auto-retry.🐛 Suggested fix
- if (responseFile?.BadData == true) - await TryScheduleAutoRetry(xchange, XchangeResultType.BadResult, responseFile.Data); + if (responseFile?.BadData == true) + await TryScheduleAutoRetry(xchange, XchangeResultType.BadResult, responseFile.Data); + else if (outputFile?.BadData == true) + await TryScheduleAutoRetry(xchange, XchangeResultType.BadResult, outputFile.Data);🤖 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 360 - 401, The auto-retry check in XchangeService should also cover mapper-only failures, not just responseFile.BadData. Update the flow around RunMapper, RunHandler, and TryScheduleAutoRetry so that when xchange.HandlerId is null and outputFile is the final payload, a BadData output from outputFile triggers the same XchangeResultType.BadResult retry scheduling. Keep the existing responseFile path intact, but add a fallback using outputFile when responseFile is null.SW.Bitween.Api/Domain/Xchange/Xchange.cs (1)
61-74: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
GroupAttemptCountsbreaks the read-only-dictionary convention used elsewhere in this entity.Every other collection property here (
HandlerProperties,MapperProperties,DocumentFilter, etc.) is exposed asIReadOnlyDictionary<...>to prevent external mutation of persisted state.GroupAttemptCountsis a plain mutableDictionary<string, int>, letting any caller mutate retry-attempt bookkeeping in place, bypassing domain invariants and EF change-tracking (noValueCompareris registered for this jsonb column inBitweenDbContext.cs).♻️ Suggested fix
- public Dictionary<string, int> GroupAttemptCounts { get; private set; } + public IReadOnlyDictionary<string, int> GroupAttemptCounts { get; private set; }Adjust the constructor parameter type accordingly, or copy into a new dictionary on assignment.
Also applies to: 77-90, 110-110
🤖 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/Domain/Xchange/Xchange.cs` around lines 61 - 74, `Xchange.GroupAttemptCounts` should follow the same read-only collection pattern as the other entity dictionaries. Update the `Xchange(Xchange xchange, XchangeFile file, IWorkGroup workGroup, ...)` constructor and the `GroupAttemptCounts` property to avoid exposing a mutable `Dictionary<string, int>` directly; either accept/store it as an `IReadOnlyDictionary<string, int>` or copy the incoming dictionary into a new instance on assignment. Keep the fix aligned with the existing `HandlerProperties`/`MapperProperties` patterns so callers can’t mutate persisted retry bookkeeping in place.SW.Bitween.Api/Services/SchedulerSeedService.cs (1)
19-28: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winMake
RetryJobseeding idempotent.SchedulerSeedServicealready treats startup scheduling as restart-safe, but this call still usesSchedule<RetryJob>(...)unconditionally. Switch it to the same “schedule if missing” path used for subscriptions so clustered restarts don’t duplicate or reset the trigger.🤖 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/SchedulerSeedService.cs` around lines 19 - 28, The RetryJob seeding in SchedulerSeedService.ExecuteAsync is still unconditional, so it can duplicate or reset the trigger on restarts. Update the scheduling call to use the same missing-only/idempotent path already used for subscriptions in SubscriptionSchedulerService, instead of always calling Schedule<RetryJob>(options.RetryJobCron), so clustered startup remains restart-safe.
🤖 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/Domain/RetryPolicy.cs`:
- Around line 13-14: Remove the duplicate audit fields from RetryPolicy by
deleting UpdatedAt and UpdatedBy, since SaveChangesAsync already sets
IAudited.ModifiedOn/ModifiedBy and RetryPolicyModel does not expose them. Update
any RetryPolicy usage, mapping, or serialization code to rely on the audited
properties instead, and verify no remaining references to UpdatedAt/UpdatedBy in
the RetryPolicy model or related mapping logic.
In `@SW.Bitween.Api/Resources/RetryPolicies/Update.cs`:
- Around line 20-29: The `Handle` method in `Update` dereferences the result of
`_dbContext.FindAsync<RetryPolicy>(key)` without checking for null, so add a
not-found guard before setting `entity.Name` or `entity.Groups`. If the
`RetryPolicy` is missing, return the appropriate clean error response instead of
continuing, and keep the existing access check and save flow intact. Also add an
integration test in `RetryPolicyTests` for updating a nonexistent id to verify
the not-found path.
In `@SW.Bitween.Api/Resources/Subscriptions/Update.cs`:
- Around line 54-55: The subscription update flow in Update.cs should validate
RetryPolicyId before assigning it on the entity, because an invalid value
currently surfaces later as a DbUpdateException during SaveChangesAsync. Add a
lookup/validation step in the update handler that checks the provided
RetryPolicyId against the available retry policies, and return a clean
validation error if it is invalid; keep the existing CustomRetryPolicy
assignment in Update since XchangeService uses CustomRetryPolicy ?? RetryPolicy.
In `@SW.Bitween.Api/Services/RetryJob.cs`:
- Around line 28-45: The retry loop in RetryJob.Run is doing per-item database
lookups for Xchange and Subscription, causing N+1 roundtrips across each batch.
Refactor the Ready/foreach flow to preload all needed Xchange records and their
Subscription data in batched queries before iterating, then have the loop use
the in-memory results instead of calling FindAsync and FirstOrDefaultAsync per
DelayedRetry.
- Around line 21-56: Batched persistence in RetryJob.Execute can roll back
successful removals and cause duplicate CreateXchange calls after a mid-batch
failure. Update the Execute loop to persist each successfully processed
DelayedRetry removal immediately (or otherwise commit per item), and isolate
failures so one item’s exception does not prevent already-completed items from
being saved. Use the Execute method, CreateXchange call, and
dbContext.Remove/SaveChangesAsync flow as the points to adjust.
In `@SW.Bitween.Api/Services/XchangeService.cs`:
- Around line 95-101: The CreateXchange overload in XchangeService is accepting
a references argument but never uses it, so callers cannot override the xchange
references. Either remove the unused references parameter from CreateXchange if
it is not needed, or thread it through to the Xchange construction path so the
new Xchange instance can use the supplied references instead of always relying
on xchange.References.
In `@SW.Bitween.PgSql/BitweenDbContext.cs`:
- Around line 23-26: The `RetryPolicy.Groups` JSON round-trip currently allows a
`null`/`"null"` payload to deserialize into a null collection even though the
property is modeled as non-null. Update the `BitweenDbContext` deserialization
path that uses `JsonSerializer.Deserialize<List<RetryGroup>>(json,
_polymorphicOpts)` to explicitly fall back to an empty list when the result is
null, or make `RetryPolicy.Groups` nullable if that better matches the persisted
data shape. Use the existing `_polymorphicOpts` and the `RetryPolicy.Groups`
mapping to locate the fix.
In `@SW.Bitween.PgSql/Migrations/BitweenDbContextModelSnapshot.cs`:
- Around line 510-558: RetryPolicy is mapped with two conflicting audit
conventions in the snapshot, so reconcile the domain model and EF mapping before
shipping. Review SW.Bitween.Domain.RetryPolicy and the related configuration to
decide whether it should follow the standard
CreatedBy/CreatedOn/ModifiedBy/ModifiedOn pattern used by Account, ApiGateway,
and SubscriptionCategory, or the separate UpdatedAt/UpdatedBy pair, then remove
the redundant fields and update the model snapshot/migration accordingly so only
one audit scheme remains.
In `@SW.Bitween.Sdk/Model/AutoRetry/Matcher.cs`:
- Around line 178-227: ResolvePath in Matcher currently violates the “never
throws” behavior because JsonNode indexers can throw when a path walks through a
scalar JsonValue instead of an object/array. Update ResolvePath to safely check
the current node type before using current[segment] or the array-index branch,
returning null whenever traversal cannot continue, and keep IsMatch relying on
that null result for non-matching paths.
- Around line 84-107: RegexMatcher.IsMatch can still throw when Pattern is
invalid or Flags is null, violating the never-throws contract. Update
RegexMatcher and its Compiled property to safely handle bad inputs by validating
or catching regex construction failures, and make Flags null-safe before
checking Contains('i'). Ensure IsMatch returns false on invalid Pattern or
missing Flags instead of propagating exceptions.
- Around line 194-201: The Regex branch in Compare currently calls Regex.IsMatch
without a timeout, which leaves retry matching vulnerable to catastrophic
backtracking. Update the JsonPathOp.Regex case in Matcher.Compare to use a
bounded match timeout consistent with RegexMatcher.Compiled (for example, the
same 200ms timeout), and keep the rest of the switch behavior unchanged.
In `@SW.Bitween.Sdk/Model/AutoRetry/RetryPolicyEvaluator.cs`:
- Around line 85-93: The Allow-path in RetryPolicyEvaluator.Evaluate assumes
group.Budget is always present, but that invariant is not enforced and can cause
a NullReferenceException. Update the logic around the existing group.Action
checks so that when Action is Allow you explicitly validate group.Budget before
using it, and return a clear RetryDecision.Block or equivalent guard result when
it is null; keep the behavior in RetryPolicyEvaluator and the Budget access path
consistent with the RetryGroup contract.
In `@SW.Bitween.UnitTests/MatcherPolymorphicJsonTests.cs`:
- Around line 20-34: The polymorphic JSON round-trip test in
MatcherPolymorphicJsonTests only covers ExceptionTypeMatcher and
JsonPathMatcher, so add coverage for the other registered Matcher subtypes as
well. Update the test to include ContainsMatcher and RegexMatcher in the
matchers collection, then verify the serialized JSON still includes the
discriminator and that deserialization returns the correct concrete types for
all four entries.
---
Outside diff comments:
In `@SW.Bitween.Api/Domain/Xchange/Xchange.cs`:
- Around line 61-74: `Xchange.GroupAttemptCounts` should follow the same
read-only collection pattern as the other entity dictionaries. Update the
`Xchange(Xchange xchange, XchangeFile file, IWorkGroup workGroup, ...)`
constructor and the `GroupAttemptCounts` property to avoid exposing a mutable
`Dictionary<string, int>` directly; either accept/store it as an
`IReadOnlyDictionary<string, int>` or copy the incoming dictionary into a new
instance on assignment. Keep the fix aligned with the existing
`HandlerProperties`/`MapperProperties` patterns so callers can’t mutate
persisted retry bookkeeping in place.
In `@SW.Bitween.Api/Services/SchedulerSeedService.cs`:
- Around line 19-28: The RetryJob seeding in SchedulerSeedService.ExecuteAsync
is still unconditional, so it can duplicate or reset the trigger on restarts.
Update the scheduling call to use the same missing-only/idempotent path already
used for subscriptions in SubscriptionSchedulerService, instead of always
calling Schedule<RetryJob>(options.RetryJobCron), so clustered startup remains
restart-safe.
In `@SW.Bitween.Api/Services/XchangeService.cs`:
- Around line 360-401: The auto-retry check in XchangeService should also cover
mapper-only failures, not just responseFile.BadData. Update the flow around
RunMapper, RunHandler, and TryScheduleAutoRetry so that when xchange.HandlerId
is null and outputFile is the final payload, a BadData output from outputFile
triggers the same XchangeResultType.BadResult retry scheduling. Keep the
existing responseFile path intact, but add a fallback using outputFile when
responseFile is null.
🪄 Autofix (Beta)
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
Run ID: d86cd41b-36d2-479d-9a3b-a4f863559289
📒 Files selected for processing (40)
SW.Bitween.Api/Data/BitweenDbContext.csSW.Bitween.Api/Domain/DelayedRetry.csSW.Bitween.Api/Domain/Notifier.csSW.Bitween.Api/Domain/RetryPolicy.csSW.Bitween.Api/Domain/Subscription/Subscription.csSW.Bitween.Api/Domain/Xchange/Xchange.csSW.Bitween.Api/Resources/RetryPolicies/Create.csSW.Bitween.Api/Resources/RetryPolicies/Delete.csSW.Bitween.Api/Resources/RetryPolicies/Get.csSW.Bitween.Api/Resources/RetryPolicies/Search.csSW.Bitween.Api/Resources/RetryPolicies/Update.csSW.Bitween.Api/Resources/Subscriptions/Update.csSW.Bitween.Api/SW.Bitween.Api.csprojSW.Bitween.Api/Services/BitweenOptions.csSW.Bitween.Api/Services/RetryJob.csSW.Bitween.Api/Services/SchedulerSeedService.csSW.Bitween.Api/Services/XchangeService.csSW.Bitween.IntegrationTests/Fixtures/BitweenFixture.csSW.Bitween.IntegrationTests/SW.Bitween.IntegrationTests.csprojSW.Bitween.IntegrationTests/Tests/RetryJobTests.csSW.Bitween.IntegrationTests/Tests/RetryPolicyTests.csSW.Bitween.IntegrationTests/xunit.runner.jsonSW.Bitween.MsSql/SW.Bitween.MsSql.csprojSW.Bitween.MySql/SW.Bitween.MySql.csprojSW.Bitween.PgSql/BitweenDbContext.csSW.Bitween.PgSql/Migrations/20260629104151_AddAutoRetry.Designer.csSW.Bitween.PgSql/Migrations/20260629104151_AddAutoRetry.csSW.Bitween.PgSql/Migrations/BitweenDbContextModelSnapshot.csSW.Bitween.PgSql/SW.Bitween.PgSql.csprojSW.Bitween.Sdk/Model/AutoRetry/DelayStrategy.csSW.Bitween.Sdk/Model/AutoRetry/IRetryPolicy.csSW.Bitween.Sdk/Model/AutoRetry/Matcher.csSW.Bitween.Sdk/Model/AutoRetry/RetryGroup.csSW.Bitween.Sdk/Model/AutoRetry/RetryPolicyEvaluator.csSW.Bitween.Sdk/Model/RetryPolicyModel.csSW.Bitween.Sdk/Model/Subscription.csSW.Bitween.UnitTests/MatcherPolymorphicJsonTests.csSW.Bitween.UnitTests/RetryPolicyEvaluatorTests.csSW.Bitween.Web/SW.Bitween.Web.csprojdocs/scheduler.md
📜 Review details
🧰 Additional context used
🪛 ast-grep (0.44.0)
SW.Bitween.Sdk/Model/AutoRetry/Matcher.cs
[warning] 198-198: A static Regex method (Match/IsMatch/Matches/Replace/Split) is called with a non-literal (variable) pattern and no matchTimeout. The .NET regex engine backtracks, so an attacker-controlled pattern can cause catastrophic backtracking (ReDoS) and hang the thread. Pass a TimeSpan matchTimeout to the overload (e.g. Regex.IsMatch(input, pattern, RegexOptions.None, TimeSpan.FromSeconds(1))), set AppDomain RegexMatchTimeout, or avoid running untrusted patterns.
Context: Regex.IsMatch(actual, expected, RegexOptions.IgnoreCase)
Note: [CWE-1333] Inefficient Regular Expression Complexity.
(redos-regex-static-untrusted-pattern-no-timeout-csharp)
🪛 Betterleaks (1.6.0)
SW.Bitween.PgSql/Migrations/20260629104151_AddAutoRetry.Designer.cs
[high] 1779-1779: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🪛 LanguageTool
docs/scheduler.md
[grammar] ~30-~30: Ensure spelling is correct
Context: ...| SimplyWorks.Scheduler.Sdk | 8.1.1 | SW.Bitween.Api | IScheduledJob<TParam>, `[Sched...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~31-~31: Ensure spelling is correct
Context: ...SimplyWorks.Scheduler.EfCore| 8.1.1 |SW.Bitween.Web|AddSchedulerMonitoring<TDbConte...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~32-~32: Ensure spelling is correct
Context: ...SimplyWorks.Scheduler.PgSql | 8.1.1 | SW.Bitween.PgSql | AddPgSqlScheduler(...), `mod...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~33-~33: Ensure spelling is correct
Context: ...plyWorks.Scheduler.SqlServer| 8.1.1 |SW.Bitween.MsSql|AddSqlServerScheduler(...)`, ...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~34-~34: Ensure spelling is correct
Context: ...SimplyWorks.Scheduler.MySql | 8.1.1 | SW.Bitween.MySql | AddMySqlScheduler(...), `mod...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~36-~36: Ensure spelling is correct
Context: ...vely brings in the full Quartz runtime. SW.Bitween.Web adds `SimplyWorks.Scheduler.EfCore...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🔇 Additional comments (38)
SW.Bitween.IntegrationTests/xunit.runner.json (1)
1-7: LGTM!SW.Bitween.Web/SW.Bitween.Web.csproj (1)
38-38: LGTM!SW.Bitween.Sdk/Model/RetryPolicyModel.cs (1)
13-18: LGTM!SW.Bitween.PgSql/Migrations/20260629104151_AddAutoRetry.cs (1)
1-134: LGTM!SW.Bitween.Api/Resources/RetryPolicies/Get.cs (1)
20-31: LGTM!SW.Bitween.Api/Resources/RetryPolicies/Search.cs (1)
20-42: LGTM!SW.Bitween.Sdk/Model/Subscription.cs (1)
91-93: 🗄️ Data Integrity & Integration
CustomRetryPolicyis already in scopeSW.Bitween.Sdk/Model/AutoRetry/IRetryPolicy.csdefinesCustomRetryPolicyinSW.Bitween.Model, soSubscription.csresolves it without an extra import.> Likely an incorrect or invalid review comment.SW.Bitween.Api/Resources/Subscriptions/Update.cs (1)
62-63: 🩺 Stability & AvailabilityPost-commit scheduler sync is not durable (duplicate of prior finding).
_subScheduler.Sync(entity, oldSchedules)runs after the DB commit with no retry/compensation, matching the durability gap already raised for this exact call site in a previous review.SW.Bitween.Api/Resources/RetryPolicies/Create.cs (1)
1-33: LGTM!SW.Bitween.Api/Resources/RetryPolicies/Delete.cs (1)
22-34: LGTM!SW.Bitween.Sdk/Model/AutoRetry/IRetryPolicy.cs (1)
1-56: LGTM!SW.Bitween.Api/Domain/Notifier.cs (1)
2-42: LGTM!SW.Bitween.IntegrationTests/Tests/RetryPolicyTests.cs (1)
1-336: LGTM!SW.Bitween.Api/Domain/DelayedRetry.cs (1)
1-11: LGTM!SW.Bitween.Api/SW.Bitween.Api.csproj (1)
29-29: LGTM!SW.Bitween.IntegrationTests/Tests/RetryJobTests.cs (1)
1-282: LGTM!SW.Bitween.Api/Services/XchangeService.cs (1)
404-430: 🎯 Functional CorrectnessVerify idempotency for message-bus redelivery.
DelayedRetryis added withId = xchange.Id. If the underlying bus redelivers the same message at-least-once before ack (common with RabbitMQ-style consumers),Process(XchangeMessage)could run twice for the samexchange.Id, and this insert would collide on the primary key duringSaveChangesAsync. The existingXchangeResultinsert has the same pattern, so this may be an accepted risk profile already, but worth confirming the consumer's ack/redelivery semantics guarantee at-most-once processing per xchange before relying on this for retry scheduling.SW.Bitween.PgSql/Migrations/BitweenDbContextModelSnapshot.cs (2)
24-24: 📐 Maintainability & Code Quality
ProductVersionannotation regressed from 8.0.26 to 8.0.23.Worth confirming this reflects an intentional EF tooling/package alignment across dev environments rather than an accidental snapshot regenerated with an older
dotnet-ef/Microsoft.EntityFrameworkCore.Designversion, which can cause churn in future migrations.
1147-1678: LGTM!Also applies to: 1933-1992, 2002-2017
SW.Bitween.PgSql/BitweenDbContext.cs (1)
349-361: LGTM!Also applies to: 362-362
SW.Bitween.Api/Domain/Subscription/Subscription.cs (1)
8-175: LGTM!docs/scheduler.md (3)
108-111: Missing language identifiers on these fenced code blocks (MD040), previously flagged and still unresolved.
141-147: Same missing-language-identifier issue as Lines 108-111.
24-37: LGTM!SW.Bitween.IntegrationTests/SW.Bitween.IntegrationTests.csproj (1)
1-48: LGTM!SW.Bitween.Sdk/Model/AutoRetry/DelayStrategy.cs (1)
1-78: LGTM!SW.Bitween.Api/Services/SchedulerSeedService.cs (1)
29-50: LGTM!SW.Bitween.PgSql/SW.Bitween.PgSql.csproj (1)
11-20: LGTM!SW.Bitween.Sdk/Model/AutoRetry/RetryGroup.cs (1)
1-84: LGTM!SW.Bitween.Sdk/Model/AutoRetry/RetryPolicyEvaluator.cs (1)
1-152: LGTM on the rest of the evaluator logic (priority ordering, budget caps, delay computation).SW.Bitween.UnitTests/RetryPolicyEvaluatorTests.cs (1)
1-410: LGTM! Good coverage of matchers, priority ordering, budget caps, delay strategies, and state persistence.SW.Bitween.IntegrationTests/Fixtures/BitweenFixture.cs (1)
54-56: 🩺 Stability & Availability | ⚡ Quick winDispose fixture-owned
NpgsqlDataSourceandApphost.
dataSource(line 56) is never stored/disposed, andDisposeAsynconly callsApp.StopAsync(), notApp.Dispose(). This can leak pooled Npgsql connections/channels across test runs.🔒️ Proposed fix
public sealed class BitweenFixture : IAsyncLifetime { private readonly PostgreSqlContainer _postgres = new PostgreSqlBuilder().Build(); private readonly RabbitMqContainer _rabbitMq = new RabbitMqBuilder().Build(); + private NpgsqlDataSource? _dataSource; @@ - var dataSource = dataSourceBuilder.Build(); + _dataSource = dataSourceBuilder.Build(); @@ - .UseNpgsql(dataSource, b => + .UseNpgsql(_dataSource, b => @@ public async Task DisposeAsync() { - if (App is not null) - await App.StopAsync(); + if (App is not null) + { + await App.StopAsync(); + App.Dispose(); + } + if (_dataSource is not null) + await _dataSource.DisposeAsync(); await _postgres.DisposeAsync(); await _rabbitMq.DisposeAsync(); }Also applies to: 142-148
SW.Bitween.Api/Services/BitweenOptions.cs (1)
76-82: LGTM!SW.Bitween.MySql/SW.Bitween.MySql.csproj (1)
10-18: LGTM!SW.Bitween.Api/Data/BitweenDbContext.cs (1)
192-199: LGTM!Also applies to: 201-219, 232-232
SW.Bitween.MsSql/SW.Bitween.MsSql.csproj (1)
10-14: LGTM!Also applies to: 17-18
SW.Bitween.PgSql/Migrations/20260629104151_AddAutoRetry.Designer.cs (1)
1774-1782: 🔒 Security & PrivacyHardcoded default API credential flagged by static analysis.
The seeded partner API key (
7facc758283844b49cc4ffd26a75b1de) is a hardcoded secret. This is pre-existing (mirrored fromBitweenDbContext.cs's unchangedHasDataseed), not introduced by this PR, but is worth confirming this is a known/intentional default meant to be rotated post-deployment rather than used as-is in production.Source: Linters/SAST tools
SW.Bitween.Sdk/Model/AutoRetry/Matcher.cs (1)
63-78: LGTM!Also applies to: 118-149
| public async Task<object> Handle(int key, RetryPolicyUpdate model) | ||
| { | ||
| _requestContext.EnsureAccess(AccountRole.Admin, AccountRole.Member); | ||
|
|
||
| var entity = await _dbContext.FindAsync<RetryPolicy>(key); | ||
| entity.Name = model.Name; | ||
| entity.Groups = model.Groups ?? []; | ||
| await _dbContext.SaveChangesAsync(); | ||
| return null; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Missing null check on FindAsync result — NRE on invalid key.
FindAsync<RetryPolicy>(key) returns null for a nonexistent id; the handler immediately dereferences it, crashing with an unhandled NullReferenceException instead of a clean not-found error. Reachable directly via the API with any bad id.
🐛 Proposed fix
var entity = await _dbContext.FindAsync<RetryPolicy>(key);
+if (entity is null)
+ throw new SWException($"Retry policy {key} not found.");
entity.Name = model.Name;
entity.Groups = model.Groups ?? [];Consider adding an integration test for this path alongside the existing CRUD coverage in RetryPolicyTests.cs.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public async Task<object> Handle(int key, RetryPolicyUpdate model) | |
| { | |
| _requestContext.EnsureAccess(AccountRole.Admin, AccountRole.Member); | |
| var entity = await _dbContext.FindAsync<RetryPolicy>(key); | |
| entity.Name = model.Name; | |
| entity.Groups = model.Groups ?? []; | |
| await _dbContext.SaveChangesAsync(); | |
| return null; | |
| } | |
| public async Task<object> Handle(int key, RetryPolicyUpdate model) | |
| { | |
| _requestContext.EnsureAccess(AccountRole.Admin, AccountRole.Member); | |
| var entity = await _dbContext.FindAsync<RetryPolicy>(key); | |
| if (entity is null) | |
| throw new SWException($"Retry policy {key} not found."); | |
| entity.Name = model.Name; | |
| entity.Groups = model.Groups ?? []; | |
| await _dbContext.SaveChangesAsync(); | |
| return null; | |
| } |
🤖 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 20 - 29, The
`Handle` method in `Update` dereferences the result of
`_dbContext.FindAsync<RetryPolicy>(key)` without checking for null, so add a
not-found guard before setting `entity.Name` or `entity.Groups`. If the
`RetryPolicy` is missing, return the appropriate clean error response instead of
continuing, and keep the existing access check and save flow intact. Also add an
integration test in `RetryPolicyTests` for updating a nonexistent id to verify
the not-found path.
| public async Task Execute() | ||
| { | ||
| var ready = await dbContext.Set<DelayedRetry>() | ||
| .Where(r => r.On <= DateTime.UtcNow) | ||
| .Take(BatchSize) | ||
| .ToListAsync(); | ||
|
|
||
| foreach (var delayedRetry in ready) | ||
| { | ||
| var xchange = await dbContext.FindAsync<Xchange>(delayedRetry.Id); | ||
| if (xchange == null) | ||
| { | ||
| dbContext.Remove(delayedRetry); | ||
| continue; | ||
| } | ||
|
|
||
| // Resolve subscription before fetching the file to avoid a cloud round-trip for orphan records. | ||
| var subscription = await dbContext.Set<Subscription>() | ||
| .FirstOrDefaultAsync(s => s.Id == xchange.SubscriptionId); | ||
|
|
||
| if (subscription == null) | ||
| { | ||
| dbContext.Remove(delayedRetry); | ||
| continue; | ||
| } | ||
|
|
||
| var inputFileData = await xchangeService.GetFile(xchange.Id, XchangeFileType.Input); | ||
| var inputFile = new XchangeFile(inputFileData, xchange.InputName); | ||
|
|
||
| await xchangeService.CreateXchange(subscription, xchange, inputFile, | ||
| groupAttemptCounts: delayedRetry.GroupAttemptCounts); | ||
| dbContext.Remove(delayedRetry); | ||
| } | ||
|
|
||
| await dbContext.SaveChangesAsync(); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win
Batched SaveChangesAsync risks duplicate resubmission on partial failure.
SaveChangesAsync is only called once after the loop (Line 55). CreateXchange (Line 50-51) is a non-idempotent external write. If processing throws mid-batch (e.g. on item 50 of 100, due to GetFile/CreateXchange failure), all dbContext.Remove(delayedRetry) calls for the successfully-processed items (1-49) are still uncommitted and roll back. The next job run will re-fetch those same DelayedRetry rows and call CreateXchange again for xchanges that already succeeded, producing duplicates.
Persist the removal (and ideally isolate failures) per item instead of batching all changes until the end.
Proposed fix: save per item and isolate failures
foreach (var delayedRetry in ready)
{
var xchange = await dbContext.FindAsync<Xchange>(delayedRetry.Id);
if (xchange == null)
{
dbContext.Remove(delayedRetry);
+ await dbContext.SaveChangesAsync();
continue;
}
// Resolve subscription before fetching the file to avoid a cloud round-trip for orphan records.
var subscription = await dbContext.Set<Subscription>()
.FirstOrDefaultAsync(s => s.Id == xchange.SubscriptionId);
if (subscription == null)
{
dbContext.Remove(delayedRetry);
+ await dbContext.SaveChangesAsync();
continue;
}
- var inputFileData = await xchangeService.GetFile(xchange.Id, XchangeFileType.Input);
- var inputFile = new XchangeFile(inputFileData, xchange.InputName);
-
- await xchangeService.CreateXchange(subscription, xchange, inputFile,
- groupAttemptCounts: delayedRetry.GroupAttemptCounts);
- dbContext.Remove(delayedRetry);
+ try
+ {
+ var inputFileData = await xchangeService.GetFile(xchange.Id, XchangeFileType.Input);
+ var inputFile = new XchangeFile(inputFileData, xchange.InputName);
+
+ await xchangeService.CreateXchange(subscription, xchange, inputFile,
+ groupAttemptCounts: delayedRetry.GroupAttemptCounts);
+ dbContext.Remove(delayedRetry);
+ await dbContext.SaveChangesAsync();
+ }
+ catch (Exception ex)
+ {
+ // log and continue with remaining items; leave delayedRetry for next run
+ }
}
-
- await dbContext.SaveChangesAsync();
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public async Task Execute() | |
| { | |
| var ready = await dbContext.Set<DelayedRetry>() | |
| .Where(r => r.On <= DateTime.UtcNow) | |
| .Take(BatchSize) | |
| .ToListAsync(); | |
| foreach (var delayedRetry in ready) | |
| { | |
| var xchange = await dbContext.FindAsync<Xchange>(delayedRetry.Id); | |
| if (xchange == null) | |
| { | |
| dbContext.Remove(delayedRetry); | |
| continue; | |
| } | |
| // Resolve subscription before fetching the file to avoid a cloud round-trip for orphan records. | |
| var subscription = await dbContext.Set<Subscription>() | |
| .FirstOrDefaultAsync(s => s.Id == xchange.SubscriptionId); | |
| if (subscription == null) | |
| { | |
| dbContext.Remove(delayedRetry); | |
| continue; | |
| } | |
| var inputFileData = await xchangeService.GetFile(xchange.Id, XchangeFileType.Input); | |
| var inputFile = new XchangeFile(inputFileData, xchange.InputName); | |
| await xchangeService.CreateXchange(subscription, xchange, inputFile, | |
| groupAttemptCounts: delayedRetry.GroupAttemptCounts); | |
| dbContext.Remove(delayedRetry); | |
| } | |
| await dbContext.SaveChangesAsync(); | |
| } | |
| public async Task Execute() | |
| { | |
| var ready = await dbContext.Set<DelayedRetry>() | |
| .Where(r => r.On <= DateTime.UtcNow) | |
| .Take(BatchSize) | |
| .ToListAsync(); | |
| foreach (var delayedRetry in ready) | |
| { | |
| var xchange = await dbContext.FindAsync<Xchange>(delayedRetry.Id); | |
| if (xchange == null) | |
| { | |
| dbContext.Remove(delayedRetry); | |
| await dbContext.SaveChangesAsync(); | |
| continue; | |
| } | |
| // Resolve subscription before fetching the file to avoid a cloud round-trip for orphan records. | |
| var subscription = await dbContext.Set<Subscription>() | |
| .FirstOrDefaultAsync(s => s.Id == xchange.SubscriptionId); | |
| if (subscription == null) | |
| { | |
| dbContext.Remove(delayedRetry); | |
| await dbContext.SaveChangesAsync(); | |
| continue; | |
| } | |
| try | |
| { | |
| var inputFileData = await xchangeService.GetFile(xchange.Id, XchangeFileType.Input); | |
| var inputFile = new XchangeFile(inputFileData, xchange.InputName); | |
| await xchangeService.CreateXchange(subscription, xchange, inputFile, | |
| groupAttemptCounts: delayedRetry.GroupAttemptCounts); | |
| dbContext.Remove(delayedRetry); | |
| await dbContext.SaveChangesAsync(); | |
| } | |
| catch (Exception ex) | |
| { | |
| // log and continue with remaining items; leave delayedRetry for next run | |
| } | |
| } | |
| } |
🤖 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/RetryJob.cs` around lines 21 - 56, Batched
persistence in RetryJob.Execute can roll back successful removals and cause
duplicate CreateXchange calls after a mid-batch failure. Update the Execute loop
to persist each successfully processed DelayedRetry removal immediately (or
otherwise commit per item), and isolate failures so one item’s exception does
not prevent already-completed items from being saved. Use the Execute method,
CreateXchange call, and dbContext.Remove/SaveChangesAsync flow as the points to
adjust.
| foreach (var delayedRetry in ready) | ||
| { | ||
| var xchange = await dbContext.FindAsync<Xchange>(delayedRetry.Id); | ||
| if (xchange == null) | ||
| { | ||
| dbContext.Remove(delayedRetry); | ||
| continue; | ||
| } | ||
|
|
||
| // Resolve subscription before fetching the file to avoid a cloud round-trip for orphan records. | ||
| var subscription = await dbContext.Set<Subscription>() | ||
| .FirstOrDefaultAsync(s => s.Id == xchange.SubscriptionId); | ||
|
|
||
| if (subscription == null) | ||
| { | ||
| dbContext.Remove(delayedRetry); | ||
| continue; | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
N+1 queries per batch item.
FindAsync<Xchange> and the Subscription query run once per DelayedRetry row, resulting in up to 2×BatchSize roundtrips per job invocation. Consider loading Xchange/Subscription in a single batched query (e.g. Where(x => ids.Contains(x.Id))) before the loop.
🤖 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/RetryJob.cs` around lines 28 - 45, The retry loop in
RetryJob.Run is doing per-item database lookups for Xchange and Subscription,
causing N+1 roundtrips across each batch. Refactor the Ready/foreach flow to
preload all needed Xchange records and their Subscription data in batched
queries before iterating, then have the loop use the in-memory results instead
of calling FindAsync and FirstOrDefaultAsync per DelayedRetry.
| public class RegexMatcher : Matcher | ||
| { | ||
| /// <inheritdoc/> | ||
| public override XchangeResultType ResultType => XchangeResultType.Error; | ||
|
|
||
| /// <summary>.NET-compatible regular expression pattern.</summary> | ||
| public required string Pattern { get; init; } | ||
|
|
||
| /// <summary> | ||
| /// Modifier flags. Supported: <c>"i"</c> (case-insensitive). Defaults to <c>"i"</c>. | ||
| /// Pass an empty string for case-sensitive matching. | ||
| /// </summary> | ||
| public string Flags { get; init; } = "i"; | ||
|
|
||
| private Regex? _compiled; | ||
|
|
||
| private Regex Compiled => _compiled ??= new Regex( | ||
| Pattern, | ||
| Flags.Contains('i') ? RegexOptions.IgnoreCase : RegexOptions.None, | ||
| matchTimeout: TimeSpan.FromMilliseconds(200)); | ||
|
|
||
| /// <inheritdoc/> | ||
| public override bool IsMatch(string content) => Compiled.IsMatch(content); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
RegexMatcher.IsMatch can throw for invalid Pattern or null Flags.
Same "never throws" contract violation: an invalid regex Pattern throws on first access to Compiled (new Regex(...)), and Flags.Contains('i') throws NullReferenceException if Flags is null (e.g. via deserialized JSON with "flags": null). Both are plausible given Pattern/Flags come from admin-configured RetryPolicy data, and neither is validated.
🐛 Proposed fix
public override bool IsMatch(string content) => Compiled.IsMatch(content);
+ public override bool IsMatch(string content)
+ {
+ try { return Compiled.IsMatch(content); }
+ catch { return false; }
+ }Also guard the null case:
- private Regex Compiled => _compiled ??= new Regex(
- Pattern,
- Flags.Contains('i') ? RegexOptions.IgnoreCase : RegexOptions.None,
+ private Regex Compiled => _compiled ??= new Regex(
+ Pattern,
+ (Flags ?? string.Empty).Contains('i') ? RegexOptions.IgnoreCase : RegexOptions.None,
matchTimeout: TimeSpan.FromMilliseconds(200));🤖 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.Sdk/Model/AutoRetry/Matcher.cs` around lines 84 - 107,
RegexMatcher.IsMatch can still throw when Pattern is invalid or Flags is null,
violating the never-throws contract. Update RegexMatcher and its Compiled
property to safely handle bad inputs by validating or catching regex
construction failures, and make Flags null-safe before checking Contains('i').
Ensure IsMatch returns false on invalid Pattern or missing Flags instead of
propagating exceptions.
| public override bool IsMatch(string content) | ||
| { | ||
| JsonNode? root; | ||
| try { root = JsonNode.Parse(content); } | ||
| catch { return false; } | ||
|
|
||
| var node = ResolvePath(root, Path); | ||
|
|
||
| return Op switch | ||
| { | ||
| JsonPathOp.Exists => node is not null, | ||
| JsonPathOp.NotExists => node is null, | ||
| _ => node is not null && Compare(node.ToString(), Value ?? "", Op) | ||
| }; | ||
| } | ||
|
|
||
| private static bool Compare(string actual, string expected, JsonPathOp op) => op switch | ||
| { | ||
| JsonPathOp.Eq => actual.Equals(expected, StringComparison.OrdinalIgnoreCase), | ||
| JsonPathOp.Neq => !actual.Equals(expected, StringComparison.OrdinalIgnoreCase), | ||
| JsonPathOp.Contains => actual.Contains(expected, StringComparison.OrdinalIgnoreCase), | ||
| JsonPathOp.Regex => Regex.IsMatch(actual, expected, RegexOptions.IgnoreCase), | ||
| _ => false | ||
| }; | ||
|
|
||
| private static JsonNode? ResolvePath(JsonNode? root, string path) | ||
| { | ||
| var segments = path.TrimStart('$').TrimStart('.') | ||
| .Split('.', StringSplitOptions.RemoveEmptyEntries); | ||
|
|
||
| var current = root; | ||
| foreach (var segment in segments) | ||
| { | ||
| if (current is null) return null; | ||
|
|
||
| var arrayMatch = Regex.Match(segment, @"^(\w+)\[(\d+)\]$"); | ||
| if (arrayMatch.Success) | ||
| { | ||
| current = current[arrayMatch.Groups[1].Value]; | ||
| if (current is JsonArray arr && | ||
| int.TryParse(arrayMatch.Groups[2].Value, out var idx)) | ||
| current = idx < arr.Count ? arr[idx] : null; | ||
| } | ||
| else | ||
| { | ||
| current = current[segment]; | ||
| } | ||
| } | ||
| return current; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
ResolvePath can throw, violating the documented "never throws" contract.
current[segment] (and the array-indexer variant) throws InvalidOperationException when current resolves to a scalar JsonValue rather than a JsonObject/JsonArray mid-path. Only JsonNode.Parse is guarded by try/catch; a well-formed-but-shallow JSON payload (e.g. {"error":"boom"} matched against path $.error.code) will throw uncaught, breaking RetryPolicyEvaluator processing for the whole batch.
🐛 Proposed fix
public override bool IsMatch(string content)
{
JsonNode? root;
try { root = JsonNode.Parse(content); }
catch { return false; }
- var node = ResolvePath(root, Path);
-
- return Op switch
- {
- JsonPathOp.Exists => node is not null,
- JsonPathOp.NotExists => node is null,
- _ => node is not null && Compare(node.ToString(), Value ?? "", Op)
- };
+ try
+ {
+ var node = ResolvePath(root, Path);
+ return Op switch
+ {
+ JsonPathOp.Exists => node is not null,
+ JsonPathOp.NotExists => node is null,
+ _ => node is not null && Compare(node.ToString(), Value ?? "", Op)
+ };
+ }
+ catch
+ {
+ return false;
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public override bool IsMatch(string content) | |
| { | |
| JsonNode? root; | |
| try { root = JsonNode.Parse(content); } | |
| catch { return false; } | |
| var node = ResolvePath(root, Path); | |
| return Op switch | |
| { | |
| JsonPathOp.Exists => node is not null, | |
| JsonPathOp.NotExists => node is null, | |
| _ => node is not null && Compare(node.ToString(), Value ?? "", Op) | |
| }; | |
| } | |
| private static bool Compare(string actual, string expected, JsonPathOp op) => op switch | |
| { | |
| JsonPathOp.Eq => actual.Equals(expected, StringComparison.OrdinalIgnoreCase), | |
| JsonPathOp.Neq => !actual.Equals(expected, StringComparison.OrdinalIgnoreCase), | |
| JsonPathOp.Contains => actual.Contains(expected, StringComparison.OrdinalIgnoreCase), | |
| JsonPathOp.Regex => Regex.IsMatch(actual, expected, RegexOptions.IgnoreCase), | |
| _ => false | |
| }; | |
| private static JsonNode? ResolvePath(JsonNode? root, string path) | |
| { | |
| var segments = path.TrimStart('$').TrimStart('.') | |
| .Split('.', StringSplitOptions.RemoveEmptyEntries); | |
| var current = root; | |
| foreach (var segment in segments) | |
| { | |
| if (current is null) return null; | |
| var arrayMatch = Regex.Match(segment, @"^(\w+)\[(\d+)\]$"); | |
| if (arrayMatch.Success) | |
| { | |
| current = current[arrayMatch.Groups[1].Value]; | |
| if (current is JsonArray arr && | |
| int.TryParse(arrayMatch.Groups[2].Value, out var idx)) | |
| current = idx < arr.Count ? arr[idx] : null; | |
| } | |
| else | |
| { | |
| current = current[segment]; | |
| } | |
| } | |
| return current; | |
| } | |
| public override bool IsMatch(string content) | |
| { | |
| JsonNode? root; | |
| try { root = JsonNode.Parse(content); } | |
| catch { return false; } | |
| try | |
| { | |
| var node = ResolvePath(root, Path); | |
| return Op switch | |
| { | |
| JsonPathOp.Exists => node is not null, | |
| JsonPathOp.NotExists => node is null, | |
| _ => node is not null && Compare(node.ToString(), Value ?? "", Op) | |
| }; | |
| } | |
| catch | |
| { | |
| return false; | |
| } | |
| } | |
| private static bool Compare(string actual, string expected, JsonPathOp op) => op switch | |
| { | |
| JsonPathOp.Eq => actual.Equals(expected, StringComparison.OrdinalIgnoreCase), | |
| JsonPathOp.Neq => !actual.Equals(expected, StringComparison.OrdinalIgnoreCase), | |
| JsonPathOp.Contains => actual.Contains(expected, StringComparison.OrdinalIgnoreCase), | |
| JsonPathOp.Regex => Regex.IsMatch(actual, expected, RegexOptions.IgnoreCase), | |
| _ => false | |
| }; | |
| private static JsonNode? ResolvePath(JsonNode? root, string path) | |
| { | |
| var segments = path.TrimStart('$').TrimStart('.') | |
| .Split('.', StringSplitOptions.RemoveEmptyEntries); | |
| var current = root; | |
| foreach (var segment in segments) | |
| { | |
| if (current is null) return null; | |
| var arrayMatch = Regex.Match(segment, @"^(\w+)\[(\d+)\]$"); | |
| if (arrayMatch.Success) | |
| { | |
| current = current[arrayMatch.Groups[1].Value]; | |
| if (current is JsonArray arr && | |
| int.TryParse(arrayMatch.Groups[2].Value, out var idx)) | |
| current = idx < arr.Count ? arr[idx] : null; | |
| } | |
| else | |
| { | |
| current = current[segment]; | |
| } | |
| } | |
| return current; | |
| } |
🧰 Tools
🪛 ast-grep (0.44.0)
[warning] 198-198: A static Regex method (Match/IsMatch/Matches/Replace/Split) is called with a non-literal (variable) pattern and no matchTimeout. The .NET regex engine backtracks, so an attacker-controlled pattern can cause catastrophic backtracking (ReDoS) and hang the thread. Pass a TimeSpan matchTimeout to the overload (e.g. Regex.IsMatch(input, pattern, RegexOptions.None, TimeSpan.FromSeconds(1))), set AppDomain RegexMatchTimeout, or avoid running untrusted patterns.
Context: Regex.IsMatch(actual, expected, RegexOptions.IgnoreCase)
Note: [CWE-1333] Inefficient Regular Expression Complexity.
(redos-regex-static-untrusted-pattern-no-timeout-csharp)
🤖 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.Sdk/Model/AutoRetry/Matcher.cs` around lines 178 - 227,
ResolvePath in Matcher currently violates the “never throws” behavior because
JsonNode indexers can throw when a path walks through a scalar JsonValue instead
of an object/array. Update ResolvePath to safely check the current node type
before using current[segment] or the array-index branch, returning null whenever
traversal cannot continue, and keep IsMatch relying on that null result for
non-matching paths.
| private static bool Compare(string actual, string expected, JsonPathOp op) => op switch | ||
| { | ||
| JsonPathOp.Eq => actual.Equals(expected, StringComparison.OrdinalIgnoreCase), | ||
| JsonPathOp.Neq => !actual.Equals(expected, StringComparison.OrdinalIgnoreCase), | ||
| JsonPathOp.Contains => actual.Contains(expected, StringComparison.OrdinalIgnoreCase), | ||
| JsonPathOp.Regex => Regex.IsMatch(actual, expected, RegexOptions.IgnoreCase), | ||
| _ => false | ||
| }; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Missing matchTimeout on Regex.IsMatch — ReDoS risk.
The JsonPathOp.Regex branch runs a user-configured pattern against externally-derived content with no timeout, unlike RegexMatcher.Compiled which correctly sets matchTimeout: 200ms. A crafted response body plus a catastrophic-backtracking pattern can hang the retry-processing thread.
🔒️ Proposed fix
- JsonPathOp.Regex => Regex.IsMatch(actual, expected, RegexOptions.IgnoreCase),
+ JsonPathOp.Regex => Regex.IsMatch(actual, expected, RegexOptions.IgnoreCase, TimeSpan.FromMilliseconds(200)),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private static bool Compare(string actual, string expected, JsonPathOp op) => op switch | |
| { | |
| JsonPathOp.Eq => actual.Equals(expected, StringComparison.OrdinalIgnoreCase), | |
| JsonPathOp.Neq => !actual.Equals(expected, StringComparison.OrdinalIgnoreCase), | |
| JsonPathOp.Contains => actual.Contains(expected, StringComparison.OrdinalIgnoreCase), | |
| JsonPathOp.Regex => Regex.IsMatch(actual, expected, RegexOptions.IgnoreCase), | |
| _ => false | |
| }; | |
| private static bool Compare(string actual, string expected, JsonPathOp op) => op switch | |
| { | |
| JsonPathOp.Eq => actual.Equals(expected, StringComparison.OrdinalIgnoreCase), | |
| JsonPathOp.Neq => !actual.Equals(expected, StringComparison.OrdinalIgnoreCase), | |
| JsonPathOp.Contains => actual.Contains(expected, StringComparison.OrdinalIgnoreCase), | |
| JsonPathOp.Regex => Regex.IsMatch(actual, expected, RegexOptions.IgnoreCase, TimeSpan.FromMilliseconds(200)), | |
| _ => false | |
| }; |
🧰 Tools
🪛 ast-grep (0.44.0)
[warning] 198-198: A static Regex method (Match/IsMatch/Matches/Replace/Split) is called with a non-literal (variable) pattern and no matchTimeout. The .NET regex engine backtracks, so an attacker-controlled pattern can cause catastrophic backtracking (ReDoS) and hang the thread. Pass a TimeSpan matchTimeout to the overload (e.g. Regex.IsMatch(input, pattern, RegexOptions.None, TimeSpan.FromSeconds(1))), set AppDomain RegexMatchTimeout, or avoid running untrusted patterns.
Context: Regex.IsMatch(actual, expected, RegexOptions.IgnoreCase)
Note: [CWE-1333] Inefficient Regular Expression Complexity.
(redos-regex-static-untrusted-pattern-no-timeout-csharp)
🤖 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.Sdk/Model/AutoRetry/Matcher.cs` around lines 194 - 201, The Regex
branch in Compare currently calls Regex.IsMatch without a timeout, which leaves
retry matching vulnerable to catastrophic backtracking. Update the
JsonPathOp.Regex case in Matcher.Compare to use a bounded match timeout
consistent with RegexMatcher.Compiled (for example, the same 200ms timeout), and
keep the rest of the switch behavior unchanged.
Source: Linters/SAST tools
| if (group.Action == RetryAction.Block) | ||
| return RetryDecision.Block($"Group '{group.Name}' explicitly blocks this error"); | ||
|
|
||
| var budget = group.Budget!; | ||
|
|
||
| if (attemptIndexForThisMessage >= budget.MaxAttemptsPerError) | ||
| return RetryDecision.Block( | ||
| $"Per-message cap reached ({budget.MaxAttemptsPerError}) in group '{group.Name}'"); | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Unchecked invariant: Budget could be null for Action = Allow.
group.Budget! assumes the "must be non-null when Allow" contract from RetryGroup's doc comment, but nothing enforces it — a policy loaded from storage/API with Action = Allow, Budget = null throws NullReferenceException here instead of a clear block/error.
Suggested fix
- var budget = group.Budget!;
+ if (group.Budget is null)
+ return RetryDecision.Block($"Group '{group.Name}' is misconfigured: Allow action requires a Budget");
+
+ var budget = group.Budget;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (group.Action == RetryAction.Block) | |
| return RetryDecision.Block($"Group '{group.Name}' explicitly blocks this error"); | |
| var budget = group.Budget!; | |
| if (attemptIndexForThisMessage >= budget.MaxAttemptsPerError) | |
| return RetryDecision.Block( | |
| $"Per-message cap reached ({budget.MaxAttemptsPerError}) in group '{group.Name}'"); | |
| if (group.Action == RetryAction.Block) | |
| return RetryDecision.Block($"Group '{group.Name}' explicitly blocks this error"); | |
| if (group.Budget is null) | |
| return RetryDecision.Block($"Group '{group.Name}' is misconfigured: Allow action requires a Budget"); | |
| var budget = group.Budget; | |
| if (attemptIndexForThisMessage >= budget.MaxAttemptsPerError) | |
| return RetryDecision.Block( | |
| $"Per-message cap reached ({budget.MaxAttemptsPerError}) in group '{group.Name}'"); |
🤖 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.Sdk/Model/AutoRetry/RetryPolicyEvaluator.cs` around lines 85 - 93,
The Allow-path in RetryPolicyEvaluator.Evaluate assumes group.Budget is always
present, but that invariant is not enforced and can cause a
NullReferenceException. Update the logic around the existing group.Action checks
so that when Action is Allow you explicitly validate group.Budget before using
it, and return a clear RetryDecision.Block or equivalent guard result when it is
null; keep the behavior in RetryPolicyEvaluator and the Budget access path
consistent with the RetryGroup contract.
| var matchers = new List<Matcher> | ||
| { | ||
| new ExceptionTypeMatcher { Value = "System.TimeoutException" }, | ||
| new JsonPathMatcher { Path = "$.error.code", Op = JsonPathOp.Eq, Value = "500" } | ||
| }; | ||
|
|
||
| var json = JsonSerializer.Serialize(matchers, Opts); | ||
| Assert.IsTrue(json.Contains("\"type\""), string.Format("No discriminator in JSON: {0}", json)); | ||
|
|
||
| var roundTripped = JsonSerializer.Deserialize<List<Matcher>>(json, Opts); | ||
| Assert.IsNotNull(roundTripped); | ||
| Assert.AreEqual(2, roundTripped.Count); | ||
| Assert.IsInstanceOfType(roundTripped[0], typeof(ExceptionTypeMatcher)); | ||
| Assert.IsInstanceOfType(roundTripped[1], typeof(JsonPathMatcher)); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Extend coverage to all Matcher subtypes.
Only ExceptionTypeMatcher and JsonPathMatcher are round-tripped. ContainsMatcher and RegexMatcher are also polymorphic derived types registered via [JsonDerivedType] and should be exercised to catch serialization regressions.
♻️ Suggested addition
var matchers = new List<Matcher>
{
+ new ContainsMatcher { Value = "boom" },
+ new RegexMatcher { Pattern = "^err-\\d+$" },
new ExceptionTypeMatcher { Value = "System.TimeoutException" },
new JsonPathMatcher { Path = "$.error.code", Op = JsonPathOp.Eq, Value = "500" }
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var matchers = new List<Matcher> | |
| { | |
| new ExceptionTypeMatcher { Value = "System.TimeoutException" }, | |
| new JsonPathMatcher { Path = "$.error.code", Op = JsonPathOp.Eq, Value = "500" } | |
| }; | |
| var json = JsonSerializer.Serialize(matchers, Opts); | |
| Assert.IsTrue(json.Contains("\"type\""), string.Format("No discriminator in JSON: {0}", json)); | |
| var roundTripped = JsonSerializer.Deserialize<List<Matcher>>(json, Opts); | |
| Assert.IsNotNull(roundTripped); | |
| Assert.AreEqual(2, roundTripped.Count); | |
| Assert.IsInstanceOfType(roundTripped[0], typeof(ExceptionTypeMatcher)); | |
| Assert.IsInstanceOfType(roundTripped[1], typeof(JsonPathMatcher)); | |
| } | |
| var matchers = new List<Matcher> | |
| { | |
| new ContainsMatcher { Value = "boom" }, | |
| new RegexMatcher { Pattern = "^err-\\d+$" }, | |
| new ExceptionTypeMatcher { Value = "System.TimeoutException" }, | |
| new JsonPathMatcher { Path = "$.error.code", Op = JsonPathOp.Eq, Value = "500" } | |
| }; | |
| var json = JsonSerializer.Serialize(matchers, Opts); | |
| Assert.IsTrue(json.Contains("\"type\""), string.Format("No discriminator in JSON: {0}", json)); | |
| var roundTripped = JsonSerializer.Deserialize<List<Matcher>>(json, Opts); | |
| Assert.IsNotNull(roundTripped); | |
| Assert.AreEqual(2, roundTripped.Count); | |
| Assert.IsInstanceOfType(roundTripped[0], typeof(ExceptionTypeMatcher)); | |
| Assert.IsInstanceOfType(roundTripped[1], typeof(JsonPathMatcher)); | |
| } |
🤖 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.UnitTests/MatcherPolymorphicJsonTests.cs` around lines 20 - 34,
The polymorphic JSON round-trip test in MatcherPolymorphicJsonTests only covers
ExceptionTypeMatcher and JsonPathMatcher, so add coverage for the other
registered Matcher subtypes as well. Update the test to include ContainsMatcher
and RegexMatcher in the matchers collection, then verify the serialized JSON
still includes the discriminator and that deserialization returns the correct
concrete types for all four entries.
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.PgSql/BitweenDbContext.cs (1)
344-352: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winAdd a
ValueComparerforCustomRetryPolicy. This converted mutable reference type is tracked by identity here, so in-place edits toGroupscan be missed and not persisted.🤖 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.PgSql/BitweenDbContext.cs` around lines 344 - 352, The CustomRetryPolicy property on Subscription is being converted from a mutable reference type without a ValueComparer, so in-place changes to its Groups collection may not be detected by EF Core. Update the Subscription mapping in BitweenDbContext’s modelBuilder.Entity<Subscription> configuration to add a ValueComparer for CustomRetryPolicy alongside the existing HasConversion, using comparison and snapshot logic that inspects the policy contents rather than object identity.
🤖 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.MsSql/Migrations/20260706071041_QuartzAndAutoRetry.cs`:
- Around line 172-191: The RetryPolicies table is created without a uniqueness
constraint on Name, so duplicate policy names are currently allowed and can
break lookup/selection flows. Review the CreateTable call for RetryPolicies in
the QuartzAndAutoRetry migration and add a unique index/constraint on Name if
duplicates are not intended; apply the same change in the corresponding MySQL
and PgSql migrations for this PR so all providers stay consistent.
In `@SW.Bitween.PgSql/Migrations/20260706071017_QuartzAndAutoRetry.Designer.cs`:
- Around line 529-531: The `RetryPolicy.Groups` and
`Subscription.CustomRetryPolicy` mappings are using plain `text` instead of the
`jsonb` pattern used throughout `BitweenDbContext`. Update the
migration/designer metadata for these properties so they follow the same typed
JSON mapping as `DocumentFilter`, `HandlerProperties`, and the other structured
collections, and verify the value-converter configuration in
`BitweenDbContext.cs` still points these properties to `jsonb` rather than
string serialization.
In
`@SW.Bitween.SampleConfigurableAdapter/SW.Bitween.SampleConfigurableAdapter.csproj`:
- Line 8: The project reference for SimplyWorks.Serverless.Sdk is out of sync
with the host package version and should be aligned to avoid adapter/runtime
contract drift. Update the PackageReference in the sample configurable adapter
project so SimplyWorks.Serverless.Sdk matches SimplyWorks.Serverless at 8.1.3,
keeping the version consistent across the adapter and host.
---
Outside diff comments:
In `@SW.Bitween.PgSql/BitweenDbContext.cs`:
- Around line 344-352: The CustomRetryPolicy property on Subscription is being
converted from a mutable reference type without a ValueComparer, so in-place
changes to its Groups collection may not be detected by EF Core. Update the
Subscription mapping in BitweenDbContext’s modelBuilder.Entity<Subscription>
configuration to add a ValueComparer for CustomRetryPolicy alongside the
existing HasConversion, using comparison and snapshot logic that inspects the
policy contents rather than object identity.
🪄 Autofix (Beta)
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
Run ID: 04428266-566c-41fe-84b3-68f265d47574
📒 Files selected for processing (18)
SW.Bitween.IntegrationTests/Fixtures/AdapterInstaller.csSW.Bitween.IntegrationTests/Fixtures/BitweenFixture.csSW.Bitween.IntegrationTests/SW.Bitween.IntegrationTests.csprojSW.Bitween.IntegrationTests/Tests/RetryJobTests.csSW.Bitween.IntegrationTests/Tests/ServerlessAdapterTests.csSW.Bitween.MsSql/Migrations/20260706071041_QuartzAndAutoRetry.Designer.csSW.Bitween.MsSql/Migrations/20260706071041_QuartzAndAutoRetry.csSW.Bitween.MsSql/Migrations/BitweenDbContextModelSnapshot.csSW.Bitween.MySql/Migrations/20260706071032_QuartzAndAutoRetry.Designer.csSW.Bitween.MySql/Migrations/20260706071032_QuartzAndAutoRetry.csSW.Bitween.MySql/Migrations/BitweenDbContextModelSnapshot.csSW.Bitween.PgSql/BitweenDbContext.csSW.Bitween.PgSql/Migrations/20260706071017_QuartzAndAutoRetry.Designer.csSW.Bitween.PgSql/Migrations/20260706071017_QuartzAndAutoRetry.csSW.Bitween.PgSql/Migrations/BitweenDbContextModelSnapshot.csSW.Bitween.SampleConfigurableAdapter/Handler.csSW.Bitween.SampleConfigurableAdapter/Program.csSW.Bitween.SampleConfigurableAdapter/SW.Bitween.SampleConfigurableAdapter.csproj
💤 Files with no reviewable changes (1)
- SW.Bitween.PgSql/Migrations/BitweenDbContextModelSnapshot.cs
📜 Review details
🧰 Additional context used
🪛 OpenGrep (1.23.0)
SW.Bitween.IntegrationTests/Fixtures/AdapterInstaller.cs
[WARNING] 32-32: File operation with dynamic path can lead to path traversal. Validate and sanitize file paths against a safe base directory.
(coderabbit.path-traversal.csharp-file-read)
🔇 Additional comments (21)
SW.Bitween.IntegrationTests/Fixtures/BitweenFixture.cs (2)
46-49: 📐 Maintainability & Code QualityData source still not stored/disposed.
Same issue as previously flagged:
dataSource(built here) is never stored on the fixture, so it can't be explicitly disposed inDisposeAsync, leaking pooled Npgsql connections across test runs.
137-146: 🩺 Stability & Availability | ⚡ Quick winContainer disposal is not exception-safe;
Appis never disposed.
App.Dispose()is still missing (duplicate of prior feedback), and there's notry/finallyguarding the container cleanup. IfGetRequiredService<CloudFilesService>()orApp.StopAsync()throws (e.g., init failed partway through, or cleanup service not registered),_postgres.DisposeAsync()/_rabbitMq.DisposeAsync()are skipped entirely, leaking Testcontainers/Docker resources for every such failure — cumulatively problematic in CI.Proposed fix
public async Task DisposeAsync() { - if (App is not null) - { - App.Services.GetRequiredService<CloudFilesService>().Cleanup(); - await App.StopAsync(); - } - await _postgres.DisposeAsync(); - await _rabbitMq.DisposeAsync(); + try + { + if (App is not null) + { + try { App.Services.GetRequiredService<CloudFilesService>().Cleanup(); } catch { /* best effort */ } + await App.StopAsync(); + App.Dispose(); + } + if (_dataSource is not null) + await _dataSource.DisposeAsync(); + } + finally + { + await _postgres.DisposeAsync(); + await _rabbitMq.DisposeAsync(); + } }SW.Bitween.IntegrationTests/Tests/RetryJobTests.cs (3)
31-102: LGTM!
89-89: 🎯 Functional CorrectnessNo overload mismatch here
new Xchange(doc.Id, null, new XchangeFile("{}"))matchesXchange(int documentId, IWorkGroup workGroup, XchangeFile file, ...), andxs.CreateXchange(sub, new XchangeFile("{}"))matchesCreateXchange(Subscription subscription, XchangeFile file, ...).> Likely an incorrect or invalid review comment.
84-102: 🎯 Functional CorrectnessNo PK-collision issue here — these integration tests already use disjoint
DocumentID ranges (5001-8004), and the shared"Bitween"fixture does not reuse IDs across this set.> Likely an incorrect or invalid review comment.SW.Bitween.IntegrationTests/SW.Bitween.IntegrationTests.csproj (1)
1-49: LGTM!SW.Bitween.IntegrationTests/Fixtures/AdapterInstaller.cs (1)
1-52: LGTM!SW.Bitween.IntegrationTests/Tests/ServerlessAdapterTests.cs (1)
1-93: LGTM!SW.Bitween.SampleConfigurableAdapter/Handler.cs (1)
1-34: LGTM!SW.Bitween.SampleConfigurableAdapter/Program.cs (1)
1-10: LGTM!SW.Bitween.PgSql/BitweenDbContext.cs (2)
328-341: Null round-trip forRetryPolicy.Groupsstill unresolved.
JsonSerializer.Deserialize<List<RetryGroup>>(json, _polymorphicOpts)!(deserialize lambda and Snapshot function) forces non-null on a result that can genuinely benullfor anull/"null"payload, whileRetryPolicy.Groupsis modeled as non-nullable (= []). This was already flagged on a prior commit and remains unfixed.♻️ Suggested fix
b.Property(p => p.Groups).HasConversion( groups => JsonSerializer.Serialize(groups, _polymorphicOpts), - json => JsonSerializer.Deserialize<List<RetryGroup>>(json, _polymorphicOpts)!, + json => JsonSerializer.Deserialize<List<RetryGroup>>(json, _polymorphicOpts) ?? new List<RetryGroup>(), new Microsoft.EntityFrameworkCore.ChangeTracking.ValueComparer<List<RetryGroup>>( (a, b) => JsonSerializer.Serialize(a, _polymorphicOpts) == JsonSerializer.Serialize(b, _polymorphicOpts), v => JsonSerializer.Serialize(v, _polymorphicOpts).GetHashCode(), - v => JsonSerializer.Deserialize<List<RetryGroup>>(JsonSerializer.Serialize(v, _polymorphicOpts), _polymorphicOpts)! + v => JsonSerializer.Deserialize<List<RetryGroup>>(JsonSerializer.Serialize(v, _polymorphicOpts), _polymorphicOpts) ?? new List<RetryGroup>() ) );
1-27: LGTM!Also applies to: 354-368
SW.Bitween.MsSql/Migrations/20260706071041_QuartzAndAutoRetry.Designer.cs (1)
15-16: LGTM!Also applies to: 130-148, 426-463, 488-490, 555-557, 587-588, 700-702, 1592-1596, 1628-1631
SW.Bitween.MsSql/Migrations/20260706071041_QuartzAndAutoRetry.cs (1)
9-49: LGTM!Also applies to: 328-337, 434-520
SW.Bitween.MsSql/Migrations/BitweenDbContextModelSnapshot.cs (1)
7-7: LGTM!Also applies to: 20-20, 126-145, 423-461, 485-487, 552-554, 584-585, 697-699, 1589-1593, 1628-1629
SW.Bitween.MySql/Migrations/20260706071032_QuartzAndAutoRetry.Designer.cs (1)
15-16: LGTM!Also applies to: 129-148, 424-462, 486-488, 553-555, 585-586, 697-699, 1586-1590, 1625-1626
SW.Bitween.MySql/Migrations/20260706071032_QuartzAndAutoRetry.cs (2)
203-228: 🗄️ Data Integrity & Integration | ⚡ Quick win
RetryPolicies.UpdatedAt(DateTimeOffset) loses its offset on MySQL.
UpdatedAtis typedDateTimeOffsetbut stored asdatetime(6)(Line 215) with no timezone component. Pomelo will discard the original offset on write and rehydrate with a zero offset on read — differing from SQL Server's nativedatetimeoffset(offset preserved) and effectively matching PostgreSQL's UTC-normalizingtimestamp with time zone. If any code path relies on the original offset (vs. just the UTC instant), results will be inconsistent across providers.Confirm only the instant (not the offset) matters here; if so this is fine, otherwise consider storing
DateTime(UTC) consistently instead ofDateTimeOffset.
10-49: LGTM!Also applies to: 387-485, 487-551
SW.Bitween.MySql/Migrations/BitweenDbContextModelSnapshot.cs (1)
7-7: LGTM!Also applies to: 20-20, 126-145, 421-459, 483-485, 550-552, 582-583, 694-696, 1586-1590, 1625-1626
SW.Bitween.PgSql/Migrations/20260706071017_QuartzAndAutoRetry.Designer.cs (1)
17-18: LGTM!Also applies to: 26-27, 154-176, 512-559, 672-674, 715-716, 1818-1822, 1866-1866
SW.Bitween.PgSql/Migrations/20260706071017_QuartzAndAutoRetry.cs (1)
11-195: LGTM! (see companion comment onBitweenDbContextModelSnapshot/Designer regardingtextvsjsonbforcustom_retry_policy/groups, Lines 23-28 and 182-183 here — same finding, not repeated)Also applies to: 332-343, 441-535
| migrationBuilder.CreateTable( | ||
| name: "RetryPolicies", | ||
| columns: table => new | ||
| { | ||
| Id = table.Column<int>(type: "int", nullable: false) | ||
| .Annotation("SqlServer:Identity", "1, 1"), | ||
| Name = table.Column<string>(type: "nvarchar(200)", maxLength: 200, nullable: false), | ||
| Groups = table.Column<string>(type: "nvarchar(max)", nullable: true), | ||
| UpdatedBy = table.Column<string>(type: "nvarchar(max)", nullable: true), | ||
| UpdatedAt = table.Column<DateTimeOffset>(type: "datetimeoffset", nullable: false), | ||
| CreatedOn = table.Column<DateTime>(type: "datetime2", nullable: false), | ||
| CreatedBy = table.Column<string>(type: "nvarchar(max)", nullable: true), | ||
| ModifiedOn = table.Column<DateTime>(type: "datetime2", nullable: true), | ||
| ModifiedBy = table.Column<string>(type: "nvarchar(max)", nullable: true) | ||
| }, | ||
| constraints: table => | ||
| { | ||
| table.PrimaryKey("PK_RetryPolicies", x => x.Id); | ||
| }); | ||
|
|
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win
Consider a unique index on RetryPolicies.Name.
No uniqueness constraint on Name, unlike Document.Name / SubscriptionCategory.Code elsewhere in this schema. Duplicate policy names would be allowed, which is ambiguous for lookup/selection in CRUD/search flows (RetryPolicies Create/Update/Search resources). Same gap exists in the MySQL and PgSql migrations for this PR.
♻️ Proposed fix
+ migrationBuilder.CreateIndex(
+ name: "IX_RetryPolicies_Name",
+ table: "RetryPolicies",
+ column: "Name",
+ unique: true);Confirm whether duplicate names are intentionally allowed at the app layer before adding this.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| migrationBuilder.CreateTable( | |
| name: "RetryPolicies", | |
| columns: table => new | |
| { | |
| Id = table.Column<int>(type: "int", nullable: false) | |
| .Annotation("SqlServer:Identity", "1, 1"), | |
| Name = table.Column<string>(type: "nvarchar(200)", maxLength: 200, nullable: false), | |
| Groups = table.Column<string>(type: "nvarchar(max)", nullable: true), | |
| UpdatedBy = table.Column<string>(type: "nvarchar(max)", nullable: true), | |
| UpdatedAt = table.Column<DateTimeOffset>(type: "datetimeoffset", nullable: false), | |
| CreatedOn = table.Column<DateTime>(type: "datetime2", nullable: false), | |
| CreatedBy = table.Column<string>(type: "nvarchar(max)", nullable: true), | |
| ModifiedOn = table.Column<DateTime>(type: "datetime2", nullable: true), | |
| ModifiedBy = table.Column<string>(type: "nvarchar(max)", nullable: true) | |
| }, | |
| constraints: table => | |
| { | |
| table.PrimaryKey("PK_RetryPolicies", x => x.Id); | |
| }); | |
| migrationBuilder.CreateTable( | |
| name: "RetryPolicies", | |
| columns: table => new | |
| { | |
| Id = table.Column<int>(type: "int", nullable: false) | |
| .Annotation("SqlServer:Identity", "1, 1"), | |
| Name = table.Column<string>(type: "nvarchar(200)", maxLength: 200, nullable: false), | |
| Groups = table.Column<string>(type: "nvarchar(max)", nullable: true), | |
| UpdatedBy = table.Column<string>(type: "nvarchar(max)", nullable: true), | |
| UpdatedAt = table.Column<DateTimeOffset>(type: "datetimeoffset", nullable: false), | |
| CreatedOn = table.Column<DateTime>(type: "datetime2", nullable: false), | |
| CreatedBy = table.Column<string>(type: "nvarchar(max)", nullable: true), | |
| ModifiedOn = table.Column<DateTime>(type: "datetime2", nullable: true), | |
| ModifiedBy = table.Column<string>(type: "nvarchar(max)", nullable: true) | |
| }, | |
| constraints: table => | |
| { | |
| table.PrimaryKey("PK_RetryPolicies", x => x.Id); | |
| }); | |
| migrationBuilder.CreateIndex( | |
| name: "IX_RetryPolicies_Name", | |
| table: "RetryPolicies", | |
| column: "Name", | |
| unique: true); |
🤖 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.MsSql/Migrations/20260706071041_QuartzAndAutoRetry.cs` around
lines 172 - 191, The RetryPolicies table is created without a uniqueness
constraint on Name, so duplicate policy names are currently allowed and can
break lookup/selection flows. Review the CreateTable call for RetryPolicies in
the QuartzAndAutoRetry migration and add a unique index/constraint on Name if
duplicates are not intended; apply the same change in the corresponding MySQL
and PgSql migrations for this PR so all providers stay consistent.
| b.Property<string>("Groups") | ||
| .HasColumnType("text") | ||
| .HasColumnName("groups"); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win
RetryPolicy.Groups and Subscription.CustomRetryPolicy now stored as plain text, not jsonb.
Every other structured/collection property in this BitweenDbContext (DocumentFilter, HandlerProperties, MapperProperties, ReceiverProperties, ValidatorProperties, AdapterProperties, PromotedProperties, GroupAttemptCounts) is mapped to jsonb with a typed CLR collection. Groups (List<RetryGroup>) and CustomRetryPolicy are instead serialized to a plain string/text column here.
This drops PostgreSQL's JSON validation, indexing, and query-operator support for retry policy data, and breaks the established pattern in this context. If this was done deliberately (e.g., to avoid jsonb's reformatting interfering with polymorphic $type discriminators used by Matcher/DelayStrategy), that's a reasonable tradeoff — jsonb doesn't alter key names/values, only whitespace/ordering, so it shouldn't break polymorphic deserialization. Worth confirming this wasn't an unintentional regression from the value-converter configuration in BitweenDbContext.cs.
Also applies to: 590-592
🤖 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.PgSql/Migrations/20260706071017_QuartzAndAutoRetry.Designer.cs`
around lines 529 - 531, The `RetryPolicy.Groups` and
`Subscription.CustomRetryPolicy` mappings are using plain `text` instead of the
`jsonb` pattern used throughout `BitweenDbContext`. Update the
migration/designer metadata for these properties so they follow the same typed
JSON mapping as `DocumentFilter`, `HandlerProperties`, and the other structured
collections, and verify the value-converter configuration in
`BitweenDbContext.cs` still points these properties to `jsonb` rather than
string serialization.
| <RootNamespace>SW.Bitween.SampleConfigurableAdapter</RootNamespace> | ||
| </PropertyGroup> | ||
| <ItemGroup> | ||
| <PackageReference Include="SimplyWorks.Serverless.Sdk" Version="8.1.1" /> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Compare SDK vs host Serverless package versions used across the solution.
rg -n 'SimplyWorks\.Serverless' --glob '*.csproj'Repository: simplify9/Bitween-api
Length of output: 159
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== csproj files =="
git ls-files '*.csproj' | sed 's#^`#-` #'
echo
echo "== Serverless package references =="
rg -n 'SimplyWorks\.Serverless' --glob '*.csproj' . || true
echo
echo "== SampleConfigurableAdapter csproj =="
sed -n '1,80p' SW.Bitween.SampleConfigurableAdapter/SW.Bitween.SampleConfigurableAdapter.csproj
echo
echo "== Serverless-related source refs =="
rg -n 'AddServerless|InvokeAsync|SimplyWorks\.Serverless|SimplyWorks\.Serverless\.Sdk' . || trueRepository: simplify9/Bitween-api
Length of output: 5173
🌐 Web query:
SimplyWorks.Serverless.Sdk 8.1.1 SimplyWorks.Serverless 8.1.3 compatibility protocol changelog
💡 Result:
The SimplyWorks.Serverless and SimplyWorks.Serverless.Sdk packages (maintained by simplify9) are designed to be used together, and it is standard practice to keep both packages at the same major and minor version to ensure compatibility. As of July 6, 2026, the latest version for both packages is 8.1.3 [1][2]. There is no formal public changelog detailing a breaking protocol change between versions 8.1.1 and 8.1.3. However, using SimplyWorks.Serverless.Sdk 8.1.1 with SimplyWorks.Serverless 8.1.3 is not recommended due to potential inconsistencies in the underlying communication protocols or runtime assumptions. It is advised to upgrade the SDK to 8.1.3 to match the service framework version, as version 8.1.3 is the current stable release for both components [1]. You can perform this update using the.NET CLI: dotnet add package SimplyWorks.Serverless.Sdk --version 8.1.3 [3]
Citations:
- 1: https://nugetprodusnc-northcentralus-01.regional.azure-api.net/profiles/simplify9
- 2: https://www.nuget.org/profiles/simplify9
- 3: https://github.com/simplify9/Serverless
🌐 Web query:
site:nuget.org SimplyWorks.Serverless.Sdk 8.1.1 SimplyWorks.Serverless 8.1.3
💡 Result:
Based on current NuGet package information, SimplyWorks.Serverless and its associated SDK are managed under the simplify9 organization [1][2]. The package SimplyWorks.Serverless version 8.1.4 is available [3], and it maintains a dependency on SimplyWorks.Serverless.Sdk version 8.1.4 or higher [3]. There is no official release listed for SimplyWorks.Serverless.Sdk 8.1.1 or SimplyWorks.Serverless 8.1.3 in the context of the current versioning lineage for these packages [3]. Users looking for these libraries should refer to the latest versions on the NuGet Gallery under the simplify9 profile [1][2].
Citations:
- 1: https://www.nuget.org/profiles/simplify9?page=3
- 2: https://www.nuget.org/profiles/simplify9
- 3: https://www.nuget.org/packages/SimplyWorks.Serverless
Align SimplyWorks.Serverless.Sdk with the host package version
Keep the SDK on 8.1.3 to match SimplyWorks.Serverless; mixing 8.1.1 with 8.1.3 can drift the adapter/runtime contract and cause InvokeAsync failures.
🤖 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.SampleConfigurableAdapter/SW.Bitween.SampleConfigurableAdapter.csproj`
at line 8, The project reference for SimplyWorks.Serverless.Sdk is out of sync
with the host package version and should be aligned to avoid adapter/runtime
contract drift. Update the PackageReference in the sample configurable adapter
project so SimplyWorks.Serverless.Sdk matches SimplyWorks.Serverless at 8.1.3,
keeping the version consistent across the adapter and host.
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
SW.Bitween.PgSql/SW.Bitween.PgSql.csproj (1)
11-14: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlign the EF Core package versions in
SW.Bitween.PgSql.Npgsql.EntityFrameworkCore.PostgreSQLis on 8.0.11 whileMicrosoft.EntityFrameworkCore.Designis on 8.0.12; keep the project on one patch level to avoid version drift.🤖 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.PgSql/SW.Bitween.PgSql.csproj` around lines 11 - 14, The package versions in SW.Bitween.PgSql are mismatched, with Npgsql.EntityFrameworkCore.PostgreSQL and Microsoft.EntityFrameworkCore.Design on different patch levels. Update the EF Core references in the project so they all use the same 8.0.x patch version, and verify the package entries in the csproj stay aligned for Npgsql.EntityFrameworkCore.PostgreSQL and Microsoft.EntityFrameworkCore.Design.
🤖 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/DelayedRetries/RunNow.cs`:
- Around line 28-36: `RunNow` can race `RetryJob` on the same `DelayedRetry`,
allowing both paths to load and process it before either marks it handled.
Update the `RunNow` flow to claim the retry atomically before calling
`_xchangeService.ExecuteDelayedRetry`, using a lock/processed
marker/concurrency-safe update so only one path can proceed. Keep the fix within
the `RunNow` handler and any `DelayedRetry` state update used by
`SaveChangesAsync`.
In `@SW.Bitween.Api/Resources/Xchanges/BulkRetry.cs`:
- Around line 26-32: The BulkRetry flow in the query that builds scheduledIds
and then filters xchanges has a TOCTOU race between two separate reads. Collapse
this into a single query in BulkRetry so the DelayedRetry existence check and
Xchange selection happen together, and reference the same
request.Ids/Xchange/DelayedRetry logic when refactoring. If duplicate scheduling
must be prevented absolutely, add a unique constraint on DelayedRetry.Id or its
FK and handle the insert exception in the creation path.
In `@SW.Bitween.Api/Services/XchangeService.cs`:
- Around line 131-151: ExecuteDelayedRetry in XchangeService currently reads a
DelayedRetry, performs GetFile/CreateXchange, and only removes the row
afterward, which allows concurrent scheduled/RunNow callers to process the same
retry twice. Add an atomic claim step before any external work—such as updating
the DelayedRetry with a lease/status or doing a conditional delete checked by
rows-affected—and have ExecuteDelayedRetry bail out as a no-op/conflict if the
claim fails. Use the ExecuteDelayedRetry flow and DelayedRetry/Xchange
identifiers to keep the fix localized and ensure only one caller can resubmit a
given retry.
In `@SW.Bitween.Sdk/JsonConverters/DelayStrategyJsonConverter.cs`:
- Around line 60-78: The DelayStrategyJsonConverter is silently accepting
malformed delay strategy payloads by defaulting missing properties to zero or
fallback values, which can turn invalid API input into unsafe retry behavior.
Update the ReadJson logic in DelayStrategyJsonConverter so FixedDelayStrategy,
LinearDelayStrategy, and ExponentialDelayStrategy validate required fields like
delayMs, initialDelayMs, incrementMs, multiplier, and maxDelayMs before
constructing the strategy. If any required field is missing or invalid, reject
the payload by throwing an appropriate deserialization exception instead of
applying default values.
In `@SW.Bitween.UnitTests/RetryPolicyJsonConverterTests.cs`:
- Around line 36-155: The current tests in RetryPolicyJsonConverterTests only
validate successful round-trips and miss the null/invalid discriminator paths in
MatcherJsonConverter. Add negative-path tests that serialize a null Matcher
through the existing serializer helpers and verify the null-handling behavior,
and add read tests for unknown and missing type discriminator values that assert
JsonSerializationException is thrown. Keep the new coverage near the existing
round-trip tests and reuse BuildSerializer, RoundTrip, and the matcher converter
types to locate the affected behavior.
In `@SW.Bitween.Web/Startup.cs`:
- Around line 159-162: The scheduler registration still uses the raw connection
string instead of the managed-identity-backed NpgsqlDataSource, which can break
scheduler storage when EF is already using token refresh. Update the
AddPgSqlScheduler(...) call in Startup/BitweenDbContext wiring to accept and
reuse the same data source used for PostgreSQL/EF (the one created for managed
identity), and keep the assemblies/schema arguments unchanged so scheduler and
EF share the same authenticated connection path.
- Around line 151-176: The scheduler registrations in Startup are still
non-clustered, which can let multiple instances execute the same jobs. Update
the AddPgSqlScheduler, AddSqlServerScheduler, and AddMySqlScheduler calls to
pass a configure callback that enables clustering on the scheduler options. Keep
the fix localized to the scheduler setup block in Startup so the
BitweenDbContext-based registrations all share the same clustered behavior.
---
Outside diff comments:
In `@SW.Bitween.PgSql/SW.Bitween.PgSql.csproj`:
- Around line 11-14: The package versions in SW.Bitween.PgSql are mismatched,
with Npgsql.EntityFrameworkCore.PostgreSQL and
Microsoft.EntityFrameworkCore.Design on different patch levels. Update the EF
Core references in the project so they all use the same 8.0.x patch version, and
verify the package entries in the csproj stay aligned for
Npgsql.EntityFrameworkCore.PostgreSQL and Microsoft.EntityFrameworkCore.Design.
🪄 Autofix (Beta)
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
Run ID: 90e30a35-25a0-4863-910d-d7dca081b011
📒 Files selected for processing (22)
SW.Bitween.Api/Resources/DelayedRetries/RunNow.csSW.Bitween.Api/Resources/DelayedRetries/Search.csSW.Bitween.Api/Resources/Xchanges/BulkRetry.csSW.Bitween.Api/Resources/Xchanges/Retry.csSW.Bitween.Api/Resources/Xchanges/Search.csSW.Bitween.Api/SW.Bitween.Api.csprojSW.Bitween.Api/Services/RetryJob.csSW.Bitween.Api/Services/XchangeService.csSW.Bitween.IntegrationTests/Tests/DelayedRetriesTests.csSW.Bitween.MsSql/SW.Bitween.MsSql.csprojSW.Bitween.MySql/SW.Bitween.MySql.csprojSW.Bitween.PgSql/BitweenDbContext.csSW.Bitween.PgSql/SW.Bitween.PgSql.csprojSW.Bitween.Sdk/JsonConverters/DelayStrategyJsonConverter.csSW.Bitween.Sdk/JsonConverters/MatcherJsonConverter.csSW.Bitween.Sdk/Model/DelayedRetryModel.csSW.Bitween.Sdk/Model/Subscription.csSW.Bitween.Sdk/Model/Xchange.csSW.Bitween.UnitTests/RetryPolicyJsonConverterTests.csSW.Bitween.Web/Properties/launchSettings.jsonSW.Bitween.Web/SW.Bitween.Web.csprojSW.Bitween.Web/Startup.cs
📜 Review details
🔇 Additional comments (24)
SW.Bitween.Api/Services/RetryJob.cs (1)
30-33: Duplicate: persist completed retries before continuing the batch.Line 30 still performs external resubmission per item while Line 33 saves only after the loop, so the previously reported partial-failure replay risk remains.
SW.Bitween.PgSql/BitweenDbContext.cs (2)
333-340: Duplicate: preserve non-nullRetryPolicy.Groupson deserialize.Line 335 still null-forgives
JsonSerializer.Deserialize<List<RetryGroup>>, so persistednullcan still materialize a null collection.
1-26: LGTM!Also applies to: 344-367
SW.Bitween.Web/Startup.cs (2)
166-168: Duplicate: normalize SQL managed-identity auth before scheduler registration.Line 166 still registers SQL scheduler storage before Lines 252-255 append
Authentication=Active Directory Default.Also applies to: 252-255
47-54: LGTM!Also applies to: 71-72, 88-89, 125-130
SW.Bitween.Api/Services/XchangeService.cs (2)
95-98: Duplicate:referencesis still accepted but not forwarded.Line 96 keeps the unused
referencesparameter, while Line 98 constructs the retry xchange without it.
88-93: LGTM!Also applies to: 422-479
SW.Bitween.Sdk/JsonConverters/DelayStrategyJsonConverter.cs (1)
8-48: LGTM!SW.Bitween.Api/Resources/Xchanges/Retry.cs (1)
23-25: LGTM!SW.Bitween.Api/SW.Bitween.Api.csproj (1)
29-29: LGTM!SW.Bitween.MySql/SW.Bitween.MySql.csproj (1)
18-18: LGTM!SW.Bitween.Sdk/Model/Subscription.cs (1)
99-101: LGTM!SW.Bitween.Sdk/Model/Xchange.cs (1)
99-99: LGTM!SW.Bitween.Sdk/Model/DelayedRetryModel.cs (1)
1-20: LGTM!SW.Bitween.PgSql/SW.Bitween.PgSql.csproj (1)
18-20: LGTM!SW.Bitween.Web/Properties/launchSettings.json (1)
1-10: LGTM!SW.Bitween.Api/Resources/DelayedRetries/Search.cs (1)
22-53: LGTM!SW.Bitween.IntegrationTests/Tests/DelayedRetriesTests.cs (2)
51-108: LGTM!Also applies to: 112-134, 138-212
31-47: 🗄️ Data Integrity & IntegrationExplicit
Document.Idassignment is supported here.Documentis configured withValueGeneratedNever(), and the PgSql context matches that, so the hard-coded test IDs are fine.> Likely an incorrect or invalid review comment.SW.Bitween.MsSql/SW.Bitween.MsSql.csproj (2)
17-18: LGTM!
10-14: No action needed:Microsoft.EntityFrameworkCore.Designalready includesbuild; buildtransitive, so design-time tooling is not blocked here.> Likely an incorrect or invalid review comment.SW.Bitween.Api/Resources/Xchanges/Search.cs (1)
41-42: LGTM!Also applies to: 75-76
SW.Bitween.Web/SW.Bitween.Web.csproj (1)
23-23: LGTM!Also applies to: 39-39
SW.Bitween.Sdk/JsonConverters/MatcherJsonConverter.cs (1)
10-56: 🩺 Stability & AvailabilityNull handling here is not a bug. Newtonsoft.Json bypasses custom converters for null values, so this
WriteJsonoverride will not receive a nullMatcher.value.GetType()only runs for non-null unknown subtypes.> Likely an incorrect or invalid review comment.
| var delayedRetry = await _dbContext.Set<DelayedRetry>().FirstOrDefaultAsync(d => d.Id == key); | ||
| if (delayedRetry == null) | ||
| throw new SWValidationException("NOT_FOUND", "No auto-retry is currently scheduled for this exchange."); | ||
|
|
||
| if (!await _xchangeService.ExecuteDelayedRetry(delayedRetry)) | ||
| throw new SWValidationException("NOT_FOUND", "The original exchange or its subscription no longer exists."); | ||
|
|
||
| await _dbContext.SaveChangesAsync(); | ||
| return null; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd -a 'XchangeService.cs' -x cat -n {}Repository: simplify9/Bitween-api
Length of output: 32091
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## RunNow.cs\n'
fd -a 'RunNow.cs' -x sh -c 'echo "--- {}"; cat -n "{}"'
printf '\n## DelayedRetry-related files\n'
rg -n --hidden --glob '!**/bin/**' --glob '!**/obj/**' 'class DelayedRetry|DelayedRetry|RetryJob|Quartz' SW.Bitween.Api SW.Bitween* .Repository: simplify9/Bitween-api
Length of output: 50377
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## RetryJob.cs\n'
fd -a 'RetryJob.cs' -x sh -c 'echo "--- {}"; cat -n "{}"'
printf '\n## DelayedRetry.cs / model config\n'
fd -a 'DelayedRetry.cs' -x sh -c 'echo "--- {}"; cat -n "{}"'
fd -a 'BitweenDbContext.cs' -x sh -c 'echo "--- {}"; sed -n "200,250p;340,380p" "{}"'
printf '\n## RetryJob tests\n'
fd -a 'RetryJobTests.cs' -x sh -c 'echo "--- {}"; sed -n "1,280p" "{}"'Repository: simplify9/Bitween-api
Length of output: 20108
🏁 Script executed:
#!/bin/bash
set -euo pipefail
fd -a 'Xchange.cs' -x sh -c 'echo "--- {}"; cat -n "{}"'
printf '\n## DelayedRetry row mapping around key config\n'
sed -n '200,235p' SW.Bitween.Api/Data/BitweenDbContext.cs
printf '\n## Any unique indexes / constraints on Xchange.RetryFor or DelayedRetry.Id\n'
rg -n --hidden --glob '!**/bin/**' --glob '!**/obj/**' 'HasIndex\(.*RetryFor|HasAlternateKey|HasIndex\(.*Id|IsUnique|HasKey\(p => p.Id' SW.Bitween.Api/Data SW.Bitween.PgSql SW.Bitween.MsSql SW.Bitween.MySqlRepository: simplify9/Bitween-api
Length of output: 50378
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '## Xchange.cs\n'
fd -a 'Xchange.cs' -x sh -c 'echo "--- {}"; cat -n "{}"'
printf '\n## Retry-related entity configuration snippets\n'
rg -n -C 3 --hidden --glob '!**/bin/**' --glob '!**/obj/**' 'Entity<DelayedRetry>|Entity<Xchange>|RetryFor|HasKey\(p => p.Id|HasIndex\(i => i.RetryFor|HasIndex\(p => p.On)' SW.Bitween.Api/Data SW.Bitween.PgSql/BitweenDbContext.cs SW.Bitween.MsSql/BitweenDbContext.cs SW.Bitween.MySql/BitweenDbContext.csRepository: simplify9/Bitween-api
Length of output: 14785
Serialize RunNow with the scheduled retry path
RetryJob is single-instance, but this endpoint can still race the same DelayedRetry with the background job. Both paths can load the row before either deletes it, which can resubmit the same exchange twice and leave one save failing with a concurrency exception. Add a claim/lock or idempotent processed marker before calling ExecuteDelayedRetry.
🤖 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/DelayedRetries/RunNow.cs` around lines 28 - 36,
`RunNow` can race `RetryJob` on the same `DelayedRetry`, allowing both paths to
load and process it before either marks it handled. Update the `RunNow` flow to
claim the retry atomically before calling `_xchangeService.ExecuteDelayedRetry`,
using a lock/processed marker/concurrency-safe update so only one path can
proceed. Keep the fix within the `RunNow` handler and any `DelayedRetry` state
update used by `SaveChangesAsync`.
| var scheduledIds = await _dbContext.Set<DelayedRetry>() | ||
| .Where(d => request.Ids.Contains(d.Id)) | ||
| .Select(d => d.Id) | ||
| .ToListAsync(); | ||
|
|
||
| var xchanges = await _dbContext.Set<Xchange>() | ||
| .Where(c => request.Ids.Contains(c.Id) && !scheduledIds.Contains(c.Id)).AsNoTracking() |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
TOCTOU window between the scheduled-ids check and the xchange query.
A DelayedRetry created for one of request.Ids between the two queries won't be reflected in scheduledIds, allowing a duplicate retry to be scheduled for an already-pending exchange. Collapsing into one query narrows the window and avoids the extra round trip.
🔧 Proposed fix
- var scheduledIds = await _dbContext.Set<DelayedRetry>()
- .Where(d => request.Ids.Contains(d.Id))
- .Select(d => d.Id)
- .ToListAsync();
-
- var xchanges = await _dbContext.Set<Xchange>()
- .Where(c => request.Ids.Contains(c.Id) && !scheduledIds.Contains(c.Id)).AsNoTracking()
+ var xchanges = await _dbContext.Set<Xchange>()
+ .Where(c => request.Ids.Contains(c.Id) &&
+ !_dbContext.Set<DelayedRetry>().Any(d => d.Id == c.Id))
+ .AsNoTracking()
.ToListAsync();If a hard guarantee against duplicate scheduling is required, enforce it with a unique constraint on DelayedRetry.Id/FK and handle the resulting exception on creation, since even a single query is a read-then-act check that a concurrent writer can race.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var scheduledIds = await _dbContext.Set<DelayedRetry>() | |
| .Where(d => request.Ids.Contains(d.Id)) | |
| .Select(d => d.Id) | |
| .ToListAsync(); | |
| var xchanges = await _dbContext.Set<Xchange>() | |
| .Where(c => request.Ids.Contains(c.Id) && !scheduledIds.Contains(c.Id)).AsNoTracking() | |
| var xchanges = await _dbContext.Set<Xchange>() | |
| .Where(c => request.Ids.Contains(c.Id) && | |
| !_dbContext.Set<DelayedRetry>().Any(d => d.Id == c.Id)) | |
| .AsNoTracking() | |
| .ToListAsync(); |
🤖 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/Xchanges/BulkRetry.cs` around lines 26 - 32, The
BulkRetry flow in the query that builds scheduledIds and then filters xchanges
has a TOCTOU race between two separate reads. Collapse this into a single query
in BulkRetry so the DelayedRetry existence check and Xchange selection happen
together, and reference the same request.Ids/Xchange/DelayedRetry logic when
refactoring. If duplicate scheduling must be prevented absolutely, add a unique
constraint on DelayedRetry.Id or its FK and handle the insert exception in the
creation path.
| public async Task<bool> ExecuteDelayedRetry(DelayedRetry delayedRetry) | ||
| { | ||
| var xchange = await _dbContext.FindAsync<Xchange>(delayedRetry.Id); | ||
| if (xchange == null) | ||
| { | ||
| _dbContext.Remove(delayedRetry); | ||
| return false; | ||
| } | ||
|
|
||
| var subscription = await _dbContext.Set<Subscription>() | ||
| .FirstOrDefaultAsync(s => s.Id == xchange.SubscriptionId); | ||
| if (subscription == null) | ||
| { | ||
| _dbContext.Remove(delayedRetry); | ||
| return false; | ||
| } | ||
|
|
||
| var inputFileData = await GetFile(xchange.Id, XchangeFileType.Input); | ||
| var inputFile = new XchangeFile(inputFileData, xchange.InputName); | ||
| await CreateXchange(subscription, xchange, inputFile, groupAttemptCounts: delayedRetry.GroupAttemptCounts); | ||
| _dbContext.Remove(delayedRetry); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | 🏗️ Heavy lift
Atomically claim DelayedRetry before resubmitting it.
ExecuteDelayedRetry is shared by scheduled and RunNow flows, but Lines 133-151 load the row, perform GetFile/CreateXchange, then only stage removal. Two callers can process the same delayed retry concurrently and create duplicate xchanges.
Use a claim/lease/status update or conditional delete with rows-affected guard before external work; return no-op/conflict if the claim fails.
🤖 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 131 - 151,
ExecuteDelayedRetry in XchangeService currently reads a DelayedRetry, performs
GetFile/CreateXchange, and only removes the row afterward, which allows
concurrent scheduled/RunNow callers to process the same retry twice. Add an
atomic claim step before any external work—such as updating the DelayedRetry
with a lease/status or doing a conditional delete checked by rows-affected—and
have ExecuteDelayedRetry bail out as a no-op/conflict if the claim fails. Use
the ExecuteDelayedRetry flow and DelayedRetry/Xchange identifiers to keep the
fix localized and ensure only one caller can resubmit a given retry.
| return new FixedDelayStrategy | ||
| { | ||
| DelayMs = jObject.Property("delayMs")?.Value?.ToObject<int>() ?? 0 | ||
| }; | ||
|
|
||
| case "linear": | ||
| return new LinearDelayStrategy | ||
| { | ||
| InitialDelayMs = jObject.Property("initialDelayMs")?.Value?.ToObject<int>() ?? 0, | ||
| IncrementMs = jObject.Property("incrementMs")?.Value?.ToObject<int>() ?? 0 | ||
| }; | ||
|
|
||
| case "exponential": | ||
| return new ExponentialDelayStrategy | ||
| { | ||
| InitialDelayMs = jObject.Property("initialDelayMs")?.Value?.ToObject<int>() ?? 0, | ||
| Multiplier = jObject.Property("multiplier")?.Value?.ToObject<double>() ?? 2.0, | ||
| MaxDelayMs = jObject.Property("maxDelayMs")?.Value?.ToObject<int>() ?? 30_000 | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reject incomplete delay strategies instead of defaulting missing fields.
Line 62, Line 68, Line 69, and Lines 75-77 turn malformed policies into zero/default delays. That can create immediate retry behavior instead of rejecting invalid API input.
Proposed fix
if (jObject is null) return null;
+ T Required<T>(string name)
+ {
+ var token = jObject.Property(name)?.Value;
+ if (token is null || token.Type == JTokenType.Null)
+ throw new JsonSerializationException($"Missing DelayStrategy property '{name}'.");
+ return token.ToObject<T>();
+ }
+
var type = jObject.Property("type")?.Value?.ToString();
switch (type)
{
case "fixed":
return new FixedDelayStrategy
{
- DelayMs = jObject.Property("delayMs")?.Value?.ToObject<int>() ?? 0
+ DelayMs = Required<int>("delayMs")
};
case "linear":
return new LinearDelayStrategy
{
- InitialDelayMs = jObject.Property("initialDelayMs")?.Value?.ToObject<int>() ?? 0,
- IncrementMs = jObject.Property("incrementMs")?.Value?.ToObject<int>() ?? 0
+ InitialDelayMs = Required<int>("initialDelayMs"),
+ IncrementMs = Required<int>("incrementMs")
};
case "exponential":
return new ExponentialDelayStrategy
{
- InitialDelayMs = jObject.Property("initialDelayMs")?.Value?.ToObject<int>() ?? 0,
- Multiplier = jObject.Property("multiplier")?.Value?.ToObject<double>() ?? 2.0,
- MaxDelayMs = jObject.Property("maxDelayMs")?.Value?.ToObject<int>() ?? 30_000
+ InitialDelayMs = Required<int>("initialDelayMs"),
+ Multiplier = Required<double>("multiplier"),
+ MaxDelayMs = Required<int>("maxDelayMs")
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return new FixedDelayStrategy | |
| { | |
| DelayMs = jObject.Property("delayMs")?.Value?.ToObject<int>() ?? 0 | |
| }; | |
| case "linear": | |
| return new LinearDelayStrategy | |
| { | |
| InitialDelayMs = jObject.Property("initialDelayMs")?.Value?.ToObject<int>() ?? 0, | |
| IncrementMs = jObject.Property("incrementMs")?.Value?.ToObject<int>() ?? 0 | |
| }; | |
| case "exponential": | |
| return new ExponentialDelayStrategy | |
| { | |
| InitialDelayMs = jObject.Property("initialDelayMs")?.Value?.ToObject<int>() ?? 0, | |
| Multiplier = jObject.Property("multiplier")?.Value?.ToObject<double>() ?? 2.0, | |
| MaxDelayMs = jObject.Property("maxDelayMs")?.Value?.ToObject<int>() ?? 30_000 | |
| }; | |
| if (jObject is null) return null; | |
| T Required<T>(string name) | |
| { | |
| var token = jObject.Property(name)?.Value; | |
| if (token is null || token.Type == JTokenType.Null) | |
| throw new JsonSerializationException($"Missing DelayStrategy property '{name}'."); | |
| return token.ToObject<T>(); | |
| } | |
| var type = jObject.Property("type")?.Value?.ToString(); | |
| switch (type) | |
| { | |
| case "fixed": | |
| return new FixedDelayStrategy | |
| { | |
| DelayMs = Required<int>("delayMs") | |
| }; | |
| case "linear": | |
| return new LinearDelayStrategy | |
| { | |
| InitialDelayMs = Required<int>("initialDelayMs"), | |
| IncrementMs = Required<int>("incrementMs") | |
| }; | |
| case "exponential": | |
| return new ExponentialDelayStrategy | |
| { | |
| InitialDelayMs = Required<int>("initialDelayMs"), | |
| Multiplier = Required<double>("multiplier"), | |
| MaxDelayMs = Required<int>("maxDelayMs") | |
| }; |
🤖 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.Sdk/JsonConverters/DelayStrategyJsonConverter.cs` around lines 60
- 78, The DelayStrategyJsonConverter is silently accepting malformed delay
strategy payloads by defaulting missing properties to zero or fallback values,
which can turn invalid API input into unsafe retry behavior. Update the ReadJson
logic in DelayStrategyJsonConverter so FixedDelayStrategy, LinearDelayStrategy,
and ExponentialDelayStrategy validate required fields like delayMs,
initialDelayMs, incrementMs, multiplier, and maxDelayMs before constructing the
strategy. If any required field is missing or invalid, reject the payload by
throwing an appropriate deserialization exception instead of applying default
values.
| [TestMethod] | ||
| public void ContainsMatcher_round_trips_through_newtonsoft() | ||
| { | ||
| var serializer = BuildSerializer(); | ||
| Matcher original = new ContainsMatcher { Value = "timeout", CaseSensitive = true }; | ||
|
|
||
| var result = RoundTrip(original, serializer); | ||
|
|
||
| var typed = Assert1<ContainsMatcher>(result); | ||
| Assert.AreEqual("timeout", typed.Value); | ||
| Assert.IsTrue(typed.CaseSensitive); | ||
| } | ||
|
|
||
| [TestMethod] | ||
| public void RegexMatcher_round_trips_through_newtonsoft() | ||
| { | ||
| var serializer = BuildSerializer(); | ||
| Matcher original = new RegexMatcher { Pattern = "connect.*failed", Flags = "" }; | ||
|
|
||
| var result = RoundTrip(original, serializer); | ||
|
|
||
| var typed = Assert1<RegexMatcher>(result); | ||
| Assert.AreEqual("connect.*failed", typed.Pattern); | ||
| Assert.AreEqual("", typed.Flags); | ||
| } | ||
|
|
||
| [TestMethod] | ||
| public void ExceptionTypeMatcher_round_trips_through_newtonsoft() | ||
| { | ||
| var serializer = BuildSerializer(); | ||
| Matcher original = new ExceptionTypeMatcher { Value = "System.TimeoutException", IncludeInner = false }; | ||
|
|
||
| var result = RoundTrip(original, serializer); | ||
|
|
||
| var typed = Assert1<ExceptionTypeMatcher>(result); | ||
| Assert.AreEqual("System.TimeoutException", typed.Value); | ||
| Assert.IsFalse(typed.IncludeInner); | ||
| } | ||
|
|
||
| [TestMethod] | ||
| public void JsonPathMatcher_round_trips_through_newtonsoft() | ||
| { | ||
| var serializer = BuildSerializer(); | ||
| Matcher original = new JsonPathMatcher { Path = "$.error.code", Op = JsonPathOp.Eq, Value = "500" }; | ||
|
|
||
| var result = RoundTrip(original, serializer); | ||
|
|
||
| var typed = Assert1<JsonPathMatcher>(result); | ||
| Assert.AreEqual("$.error.code", typed.Path); | ||
| Assert.AreEqual(JsonPathOp.Eq, typed.Op); | ||
| Assert.AreEqual("500", typed.Value); | ||
| } | ||
|
|
||
| [TestMethod] | ||
| public void FixedDelayStrategy_round_trips_through_newtonsoft() | ||
| { | ||
| var serializer = BuildSerializer(); | ||
| DelayStrategy original = new FixedDelayStrategy { DelayMs = 5000 }; | ||
|
|
||
| var result = RoundTrip(original, serializer); | ||
|
|
||
| var typed = Assert1<FixedDelayStrategy>(result); | ||
| Assert.AreEqual(5000, typed.DelayMs); | ||
| } | ||
|
|
||
| [TestMethod] | ||
| public void LinearDelayStrategy_round_trips_through_newtonsoft() | ||
| { | ||
| var serializer = BuildSerializer(); | ||
| DelayStrategy original = new LinearDelayStrategy { InitialDelayMs = 1000, IncrementMs = 500 }; | ||
|
|
||
| var result = RoundTrip(original, serializer); | ||
|
|
||
| var typed = Assert1<LinearDelayStrategy>(result); | ||
| Assert.AreEqual(1000, typed.InitialDelayMs); | ||
| Assert.AreEqual(500, typed.IncrementMs); | ||
| } | ||
|
|
||
| [TestMethod] | ||
| public void ExponentialDelayStrategy_round_trips_through_newtonsoft() | ||
| { | ||
| var serializer = BuildSerializer(); | ||
| DelayStrategy original = new ExponentialDelayStrategy { InitialDelayMs = 1000, Multiplier = 3.0, MaxDelayMs = 60_000 }; | ||
|
|
||
| var result = RoundTrip(original, serializer); | ||
|
|
||
| var typed = Assert1<ExponentialDelayStrategy>(result); | ||
| Assert.AreEqual(1000, typed.InitialDelayMs); | ||
| Assert.AreEqual(3.0, typed.Multiplier); | ||
| Assert.AreEqual(60_000, typed.MaxDelayMs); | ||
| } | ||
|
|
||
| [TestMethod] | ||
| public void RetryGroup_with_nested_matcher_and_delay_strategy_round_trips() | ||
| { | ||
| var serializer = BuildSerializer(); | ||
| var original = new RetryGroup | ||
| { | ||
| Name = "Timeout Group", | ||
| Priority = 10, | ||
| AppliesTo = [XchangeResultType.Error], | ||
| Matchers = [new ContainsMatcher { Value = "timeout" }], | ||
| Action = RetryAction.Allow, | ||
| Budget = new RetryBudget | ||
| { | ||
| MaxAttemptsPerError = 3, | ||
| MaxAttemptsTotal = 10, | ||
| DelayStrategy = new ExponentialDelayStrategy { InitialDelayMs = 1000, Multiplier = 2, MaxDelayMs = 30_000 } | ||
| } | ||
| }; | ||
|
|
||
| var result = RoundTrip(original, serializer); | ||
|
|
||
| Assert.IsNotNull(result); | ||
| Assert.AreEqual("Timeout Group", result.Name); | ||
| Assert.AreEqual(RetryAction.Allow, result.Action); | ||
| Assert.AreEqual(1, result.Matchers.Count); | ||
| Assert.IsInstanceOfType(result.Matchers[0], typeof(ContainsMatcher)); | ||
| Assert.IsInstanceOfType(result.Budget!.DelayStrategy, typeof(ExponentialDelayStrategy)); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Add negative-path coverage: null Matcher, unknown/missing discriminator.
Current tests only cover successful round-trips. Given the null-handling bug flagged in MatcherJsonConverter.WriteJson, a test serializing a null Matcher would catch regressions; likewise tests for unknown/missing type discriminators on read would lock in the intended JsonSerializationException behavior.
🤖 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.UnitTests/RetryPolicyJsonConverterTests.cs` around lines 36 - 155,
The current tests in RetryPolicyJsonConverterTests only validate successful
round-trips and miss the null/invalid discriminator paths in
MatcherJsonConverter. Add negative-path tests that serialize a null Matcher
through the existing serializer helpers and verify the null-handling behavior,
and add read tests for unknown and missing type discriminator values that assert
JsonSerializationException is thrown. Keep the new coverage near the existing
round-trip tests and reuse BuildSerializer, RoundTrip, and the matcher converter
types to locate the affected behavior.
| // Register the persistent Quartz scheduler using the same DB as Bitween. | ||
| // NOTE: clustering is only guaranteed once SimplyWorks.Scheduler.* is bumped past | ||
| // 8.1.1 (the version pinned in the .csproj files as of this comment) — the fix that | ||
| // makes clustering unconditional (unique auto-generated SchedulerId per instance) | ||
| // hasn't been published yet. Until that bump happens, these packages run | ||
| // NON-clustered (EnableClustering defaulted to false and no longer settable here). | ||
| if (string.Equals(bitweenOptions.DatabaseType, RelationalDbType.PgSql.ToString(), StringComparison.OrdinalIgnoreCase)) | ||
| { | ||
| services.AddPgSqlScheduler( | ||
| connectionString: connectionString, | ||
| schema: PgSql.BitweenDbContext.Schema, | ||
| assemblies: typeof(BitweenDbContext).Assembly); | ||
| } | ||
| else if (string.Equals(bitweenOptions.DatabaseType, RelationalDbType.MsSql.ToString(), StringComparison.OrdinalIgnoreCase)) | ||
| { | ||
| services.AddSqlServerScheduler( | ||
| connectionString: connectionString, | ||
| assemblies: typeof(BitweenDbContext).Assembly); | ||
| } | ||
| else | ||
| { | ||
| // MySql (default) | ||
| services.AddMySqlScheduler( | ||
| connectionString: connectionString, | ||
| assemblies: typeof(BitweenDbContext).Assembly); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify scheduler package versions and whether clustering is configurable/enabled.
rg -n --glob '*.csproj' 'SimplyWorks\.Scheduler\.(Sdk|PgSql|MySql|SqlServer|MsSql)' .
rg -n --type=cs 'Add(PgSql|SqlServer|MySql)Scheduler|EnableClustering|UseScheduler' .Repository: simplify9/Bitween-api
Length of output: 1297
🌐 Web query:
SimplyWorks.Scheduler 8.1.2 clustering fix SchedulerId unconditional unique auto-generated release notes
💡 Result:
There is no official release note or technical documentation for a SimplyWorks.Scheduler 8.1.2 clustering fix regarding an unconditional unique auto-generated SchedulerId [1][2]. The terminology you are referencing relates closely to Quartz.NET—the underlying scheduling library often wrapped by projects like SimplyWorks.Scheduler [2]. In Quartz.NET, clustering requires each instance to have a unique SchedulerId [3][4]. When instances fail to set a unique ID (or use a duplicate one), it leads to common misconfiguration issues such as jobs running multiple times [3]. Recent developments in the Quartz.NET ecosystem (specifically in April 2026) addressed a distinct but related issue in the SchedulerRepository concerning connections to multiple cluster nodes [5][6]. That fix involved updating the internal storage to handle multiple schedulers per name by differentiating them by their instance ID [5]. If you are encountering issues with SchedulerId in a SimplyWorks.Scheduler environment, ensure that you are explicitly setting SchedulerId to "AUTO" or a unique identifier in your configuration, as this is the standard practice for enabling Quartz.NET clustering and preventing collisions across nodes [3][4].
Citations:
- 1: https://nugetprodusnc.azure-api.net/packages/SimplyWorks.Scheduler
- 2: https://github.com/simplify9/SimplyScheduler
- 3: https://stackoverflow.com/questions/79933341/quartz-net-clustering-mode
- 4: https://github.com/BrighterCommand/Docs/blob/master/contents/QuartzScheduler.md
- 5: Fix SchedulerRepository preventing connections to multiple cluster nodes quartznet/quartznet#2991
- 6: SchedulerRepostory prevents connections to multiple cluster nodes quartznet/quartznet#388
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the scheduler registration and package metadata around the referenced lines.
sed -n '130,190p' SW.Bitween.Web/Startup.cs
printf '\n--- csproj refs ---\n'
sed -n '1,80p' SW.Bitween.Web/SW.Bitween.Web.csproj
printf '\n--- scheduler packages ---\n'
sed -n '1,80p' SW.Bitween.Api/SW.Bitween.Api.csproj
sed -n '1,80p' SW.Bitween.PgSql/SW.Bitween.PgSql.csproj
sed -n '1,80p' SW.Bitween.MySql/SW.Bitween.MySql.csproj
sed -n '1,80p' SW.Bitween.MsSql/SW.Bitween.MsSql.csproj
printf '\n--- search for scheduler configuration ---\n'
rg -n 'SchedulerId|Clustering|EnableClustering|UseScheduler(PostgreSql|MySql|SqlServer)|Add(PgSql|MySql|SqlServer)Scheduler|Quartz' SW.Bitween.* -g '!**/bin/**' -g '!**/obj/**'Repository: simplify9/Bitween-api
Length of output: 37046
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Look for the underlying package APIs/docs in the repository to see whether clustering can be configured.
rg -n 'SimplyWorks\.Scheduler|SchedulerId|EnableClustering|AUTO|cluster' . -g '!**/bin/**' -g '!**/obj/**'Repository: simplify9/Bitween-api
Length of output: 5551
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# If a local package cache exists, inspect the package assembly metadata for clustering-related options.
# Read-only; no builds or installs.
python3 - <<'PY'
import os, glob
candidates = []
for root in [os.path.expanduser('~/.nuget/packages'), '/root/.nuget/packages']:
if os.path.isdir(root):
for path in glob.glob(root + '/simplyworks.scheduler.*/*/*.dll'):
candidates.append(path)
print('\n'.join(sorted(candidates)[:50]))
PYRepository: simplify9/Bitween-api
Length of output: 160
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '216,250p' docs/scheduler.md
printf '\n---\n'
sed -n '24,40p' docs/scheduler.md
printf '\n---\n'
sed -n '96,110p' docs/architecture.mdRepository: simplify9/Bitween-api
Length of output: 3695
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n 'AddPgSqlScheduler\(|AddSqlServerScheduler\(|AddMySqlScheduler\(' docs SW.Bitween.* -g '!**/bin/**' -g '!**/obj/**'Repository: simplify9/Bitween-api
Length of output: 1003
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the scheduler docs for the concrete API shape and version table.
nl -ba docs/scheduler.md | sed -n '20,40p;216,250p'Repository: simplify9/Bitween-api
Length of output: 199
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check whether the package README or docs mention the same options in source control.
rg -n 'EnableClustering|SchedulerId|AUTO|cluster-safe|clustered deployment' docs SW.Bitween.* README* -g '!**/bin/**' -g '!**/obj/**'Repository: simplify9/Bitween-api
Length of output: 1357
Enable clustering on the scheduler registrations. AddPgSqlScheduler / AddSqlServerScheduler / AddMySqlScheduler still omit configure: o => o.EnableClustering = true; in a multi-node deployment, each instance can fire the same trigger and duplicate receiver/aggregation/retry work.
🤖 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.Web/Startup.cs` around lines 151 - 176, The scheduler
registrations in Startup are still non-clustered, which can let multiple
instances execute the same jobs. Update the AddPgSqlScheduler,
AddSqlServerScheduler, and AddMySqlScheduler calls to pass a configure callback
that enables clustering on the scheduler options. Keep the fix localized to the
scheduler setup block in Startup so the BitweenDbContext-based registrations all
share the same clustered behavior.
| services.AddPgSqlScheduler( | ||
| connectionString: connectionString, | ||
| schema: PgSql.BitweenDbContext.Schema, | ||
| assemblies: typeof(BitweenDbContext).Assembly); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect whether the PgSql scheduler registration supports a token-aware data source/provider.
rg -n --type=cs 'UseAzureManagedIdentity|AddPgSqlScheduler|NpgsqlDataSourceBuilder|UsePeriodicPasswordProvider' SW.Bitween.Web .
rg -n --glob '*.csproj' 'SimplyWorks\.Scheduler\.PgSql|Npgsql' .Repository: simplify9/Bitween-api
Length of output: 2277
Wire the scheduler storage through the managed-identity data source. AddPgSqlScheduler(...) still receives the raw connection string, while the EF/PostgreSQL path switches to an NpgsqlDataSource with periodic token refresh. In managed-identity deployments, scheduler storage can fail even though EF succeeds.
🤖 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.Web/Startup.cs` around lines 159 - 162, The scheduler registration
still uses the raw connection string instead of the managed-identity-backed
NpgsqlDataSource, which can break scheduler storage when EF is already using
token refresh. Update the AddPgSqlScheduler(...) call in
Startup/BitweenDbContext wiring to accept and reuse the same data source used
for PostgreSQL/EF (the one created for managed identity), and keep the
assemblies/schema arguments unchanged so scheduler and EF share the same
authenticated connection path.
…tz-scheduler-integration # Conflicts: # SW.Bitween.Api/Domain/Subscription/Subscription.cs
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
SW.Bitween.Web/Startup.cs (1)
164-169: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winMsSql scheduler connection string is built before the Managed-Identity
Authentication=clause is appended.
connectionStringis passed by value intoAddSqlServerSchedulerhere. The;Authentication=Active Directory Defaultsuffix is only appended later, at Lines 252-256, and only mutates the local variable used afterward forAddDbContext<...MsSql.BitweenDbContext>. WithUseAzureManagedIdentity=trueandDatabaseType=MsSql, the Quartz scheduler will attempt to connect without the managed-identity auth clause and fail — a distinct instance of the same "scheduler storage not managed-identity aware" defect, but for MsSql specifically.🔧 Proposed fix
+ // Compute the managed-identity-aware connection string before registering the scheduler. + if (bitweenOptions.UseAzureManagedIdentity && + bitweenOptions.DatabaseType.Equals(RelationalDbType.MsSql.ToString(), StringComparison.OrdinalIgnoreCase) && + !connectionString.Contains("Authentication=", StringComparison.OrdinalIgnoreCase)) + { + connectionString += ";Authentication=Active Directory Default"; + } + else if (string.Equals(bitweenOptions.DatabaseType, RelationalDbType.MsSql.ToString(), StringComparison.OrdinalIgnoreCase)) { services.AddSqlServerScheduler( connectionString: connectionString, assemblies: typeof(BitweenDbContext).Assembly); }And remove the now-redundant mutation later in the method:
// Lines 252-256 — remove since the string is now pre-augmented above if (bitweenOptions.UseAzureManagedIdentity && !connectionString.Contains("Authentication=", StringComparison.OrdinalIgnoreCase)) { connectionString += ";Authentication=Active Directory Default"; }🤖 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.Web/Startup.cs` around lines 164 - 169, The MsSql scheduler path in Startup’s AddSqlServerScheduler call is using the base connection string before the managed-identity Authentication clause is applied. Update the connection-string setup so the same pre-augmented value is used for both the Quartz scheduler and the later AddDbContext<...MsSql.BitweenDbContext> configuration when UseAzureManagedIdentity is enabled, and remove the later connectionString mutation that only affects the DbContext path.SW.Bitween.Api/Domain/Subscription/Subscription.cs (1)
71-74: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winNo invariant preventing
RetryPolicyIdandCustomRetryPolicyfrom being set simultaneously.Both are plain public-setter properties, unlike other mutations on this aggregate that go through dedicated
Set*methods enforcing invariants. Nothing here defines precedence if both are populated. Consider aSetRetryPolicy(...)method that enforces mutual exclusivity, and confirm how the retry evaluation logic (not in this batch) resolves the ambiguity today.Also, the
// this should be saved as jsoncomment is stale — already implemented viaStoreAsJson()inBitweenDbContext.cs.🤖 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/Domain/Subscription/Subscription.cs` around lines 71 - 74, The Subscription aggregate currently allows RetryPolicyId and CustomRetryPolicy to be set at the same time because they are exposed as public setters, which bypasses the invariant pattern used elsewhere in Subscription. Introduce a dedicated SetRetryPolicy(...) method on Subscription that enforces mutual exclusivity and updates both properties consistently, and use that method wherever retry policy is assigned. Also remove the stale “saved as json” comment on CustomRetryPolicy since persistence is already handled by StoreAsJson() in BitweenDbContext.SW.Bitween.Api/Data/BitweenDbContext.cs (1)
230-237: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the extra
UpdatedAt/UpdatedByfields fromRetryPolicy.
RetryPolicyalready has the standardCreated*/Modified*audit fields viaIAudited, andUpdated*is not part ofIRetryPolicyor used elsewhere. If these columns are intentional, map and document them explicitly; otherwise they add schema noise.🤖 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/Data/BitweenDbContext.cs` around lines 230 - 237, Remove the extra UpdatedAt/UpdatedBy mapping from the RetryPolicy entity in BitweenDbContext so the EF model matches IRetryPolicy and the existing IAudited fields only. Update the modelBuilder.Entity<RetryPolicy> configuration to keep the core properties (Id, Name, Groups) and either explicitly map/document any intentional updated fields elsewhere or drop them entirely if they are not part of the contract.SW.Bitween.Sdk/Model/Subscription.cs (1)
100-102: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winGuard retry-policy updates in
SW.Bitween.Api/Resources/Subscriptions/Update.cs:42-55.RetryPolicyId/CustomRetryPolicyare assigned directly, so invalid FK values still reachSaveChangesAsync, and a payload that sets both fields relies on implicitCustomRetryPolicyprecedence. The SDK and domain already use the sameSW.Bitween.Model.CustomRetryPolicytype, so no mapping is needed.🤖 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.Sdk/Model/Subscription.cs` around lines 100 - 102, The retry-policy update path in the subscription update flow assigns RetryPolicyId and CustomRetryPolicy directly, so invalid foreign keys can slip through until SaveChangesAsync and payloads that set both fields depend on implicit precedence. Update the subscription update logic in Update to validate the retry policy before assigning it, reject or ignore invalid RetryPolicyId values, and make the precedence between RetryPolicyId and CustomRetryPolicy explicit. Use the existing SW.Bitween.Model.CustomRetryPolicy type directly so no mapping layer is needed, and keep the fix localized to the subscription update handling.SW.Bitween.Api/Domain/Xchange/Xchange.cs (1)
63-112: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
GroupAttemptCountsexposes a mutable dictionary despite the private setter.Every other dictionary-typed property on this entity (
HandlerProperties,MapperProperties) isIReadOnlyDictionary<string, string>.GroupAttemptCountsbreaks that convention with a mutableDictionary<string, int>— callers holding the reference (e.g.evaluator.GetGroupAttemptCounts(),delayedRetry.GroupAttemptCounts) can mutate it after assignment, silently corrupting retry-tracking state that later gets persisted via EF.♻️ Suggested fix
- public Dictionary<string, int> GroupAttemptCounts { get; private set; } + public IReadOnlyDictionary<string, int> GroupAttemptCounts { get; private set; }Update constructor parameters/assignments to accept
IReadOnlyDictionary<string,int>or defensively copy on assignment.🤖 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/Domain/Xchange/Xchange.cs` around lines 63 - 112, `GroupAttemptCounts` in `Xchange` is exposing mutable retry state through a `Dictionary<string, int>` even though the property has a private setter. Update the `Xchange` constructors that accept `groupAttemptCounts` to take `IReadOnlyDictionary<string, int>` (or defensively copy the incoming dictionary) and change the `GroupAttemptCounts` property to an immutable/read-only shape consistent with `HandlerProperties` and `MapperProperties`, so callers like `evaluator.GetGroupAttemptCounts()` and `delayedRetry.GroupAttemptCounts` can’t mutate it after assignment.SW.Bitween.Api/Services/XchangeService.cs (2)
463-478: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
CountRetryChainDepthperforms sequential per-level DB round trips.Each retry evaluation walks the
RetryForchain one row at a time viaawaitinside thewhileloop — O(depth) sequential queries on every message failure. For subscriptions with several configured retry attempts this adds material latency to a hot failure-handling path. Consider a single recursive query, or simpler: persist the attempt index directly onXchange/DelayedRetryat creation time instead of recomputing it by chain traversal.🤖 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 463 - 478, CountRetryChainDepth is doing one awaited database lookup per RetryFor hop, causing sequential round trips on the hot failure path. Refactor XchangeService.CountRetryChainDepth to avoid walking the chain row-by-row: either fetch the retry chain in one query/recursive query or stop recomputing depth here by storing the attempt index on Xchange/DelayedRetry when the retry is created. Keep the logic localized to CountRetryChainDepth and the RetryFor chain handling so the call sites can use the precomputed value.
435-461: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winGuard
DelayedRetrycreation against duplicateIds
DelayedRetry.Idis the PK, so processing the sameXchangeMessagetwice will hitSaveChangesAsync()with a duplicate-key insert and abort the whole save, including theXchangeResult. Use an upsert or handle the existing row before adding a new one.🤖 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 435 - 461, TryScheduleAutoRetry currently always adds a new DelayedRetry with Xchange.Id as the primary key, so duplicate processing can cause SaveChangesAsync to fail. Update TryScheduleAutoRetry to check for an existing DelayedRetry for the same xchange.Id before calling _dbContext.Add, and either reuse/update the existing row or skip inserting a duplicate. Keep the retry scheduling logic in TryScheduleAutoRetry and the DelayedRetry entity handling aligned with the PK constraint.
🤖 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/Data/BitweenDbContext.cs`:
- Around line 118-146: The MatchExpression value-conversion logic is duplicated
between the BusGatewayRoute and Subscription mappings in BitweenDbContext, so
extract the inline HasConversion lambda pair using
MatchSpecValueConverter.SerializeMatchSpec and DeserializeMatchSpec into a
shared converter or reusable configuration helper. Then apply that shared
converter from both entity configurations (BusGatewayRoute and Subscription) to
keep the mapping consistent and avoid drift.
---
Outside diff comments:
In `@SW.Bitween.Api/Data/BitweenDbContext.cs`:
- Around line 230-237: Remove the extra UpdatedAt/UpdatedBy mapping from the
RetryPolicy entity in BitweenDbContext so the EF model matches IRetryPolicy and
the existing IAudited fields only. Update the modelBuilder.Entity<RetryPolicy>
configuration to keep the core properties (Id, Name, Groups) and either
explicitly map/document any intentional updated fields elsewhere or drop them
entirely if they are not part of the contract.
In `@SW.Bitween.Api/Domain/Subscription/Subscription.cs`:
- Around line 71-74: The Subscription aggregate currently allows RetryPolicyId
and CustomRetryPolicy to be set at the same time because they are exposed as
public setters, which bypasses the invariant pattern used elsewhere in
Subscription. Introduce a dedicated SetRetryPolicy(...) method on Subscription
that enforces mutual exclusivity and updates both properties consistently, and
use that method wherever retry policy is assigned. Also remove the stale “saved
as json” comment on CustomRetryPolicy since persistence is already handled by
StoreAsJson() in BitweenDbContext.
In `@SW.Bitween.Api/Domain/Xchange/Xchange.cs`:
- Around line 63-112: `GroupAttemptCounts` in `Xchange` is exposing mutable
retry state through a `Dictionary<string, int>` even though the property has a
private setter. Update the `Xchange` constructors that accept
`groupAttemptCounts` to take `IReadOnlyDictionary<string, int>` (or defensively
copy the incoming dictionary) and change the `GroupAttemptCounts` property to an
immutable/read-only shape consistent with `HandlerProperties` and
`MapperProperties`, so callers like `evaluator.GetGroupAttemptCounts()` and
`delayedRetry.GroupAttemptCounts` can’t mutate it after assignment.
In `@SW.Bitween.Api/Services/XchangeService.cs`:
- Around line 463-478: CountRetryChainDepth is doing one awaited database lookup
per RetryFor hop, causing sequential round trips on the hot failure path.
Refactor XchangeService.CountRetryChainDepth to avoid walking the chain
row-by-row: either fetch the retry chain in one query/recursive query or stop
recomputing depth here by storing the attempt index on Xchange/DelayedRetry when
the retry is created. Keep the logic localized to CountRetryChainDepth and the
RetryFor chain handling so the call sites can use the precomputed value.
- Around line 435-461: TryScheduleAutoRetry currently always adds a new
DelayedRetry with Xchange.Id as the primary key, so duplicate processing can
cause SaveChangesAsync to fail. Update TryScheduleAutoRetry to check for an
existing DelayedRetry for the same xchange.Id before calling _dbContext.Add, and
either reuse/update the existing row or skip inserting a duplicate. Keep the
retry scheduling logic in TryScheduleAutoRetry and the DelayedRetry entity
handling aligned with the PK constraint.
In `@SW.Bitween.Sdk/Model/Subscription.cs`:
- Around line 100-102: The retry-policy update path in the subscription update
flow assigns RetryPolicyId and CustomRetryPolicy directly, so invalid foreign
keys can slip through until SaveChangesAsync and payloads that set both fields
depend on implicit precedence. Update the subscription update logic in Update to
validate the retry policy before assigning it, reject or ignore invalid
RetryPolicyId values, and make the precedence between RetryPolicyId and
CustomRetryPolicy explicit. Use the existing SW.Bitween.Model.CustomRetryPolicy
type directly so no mapping layer is needed, and keep the fix localized to the
subscription update handling.
In `@SW.Bitween.Web/Startup.cs`:
- Around line 164-169: The MsSql scheduler path in Startup’s
AddSqlServerScheduler call is using the base connection string before the
managed-identity Authentication clause is applied. Update the connection-string
setup so the same pre-augmented value is used for both the Quartz scheduler and
the later AddDbContext<...MsSql.BitweenDbContext> configuration when
UseAzureManagedIdentity is enabled, and remove the later connectionString
mutation that only affects the DbContext path.
🪄 Autofix (Beta)
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
Run ID: 4aae606f-98de-42c0-86fe-e338c0634252
📒 Files selected for processing (12)
SW.Bitween.Api/Data/BitweenDbContext.csSW.Bitween.Api/Domain/Subscription/Subscription.csSW.Bitween.Api/Domain/Xchange/Xchange.csSW.Bitween.Api/Resources/Subscriptions/Update.csSW.Bitween.Api/Services/BitweenOptions.csSW.Bitween.Api/Services/XchangeService.csSW.Bitween.MsSql/Migrations/BitweenDbContextModelSnapshot.csSW.Bitween.MySql/Migrations/BitweenDbContextModelSnapshot.csSW.Bitween.PgSql/BitweenDbContext.csSW.Bitween.PgSql/Migrations/BitweenDbContextModelSnapshot.csSW.Bitween.Sdk/Model/Subscription.csSW.Bitween.Web/Startup.cs
📜 Review details
🔇 Additional comments (24)
SW.Bitween.Web/Startup.cs (3)
151-176: 🩺 Stability & AvailabilityClustering still disabled — contradicts PR's stated goal.
The added comment documents that clustering is NOT enabled pending a
SimplyWorks.Schedulerpackage bump, yet the PR description states "Clustering is enabled so that only one node fires each trigger." As shipped, every node in a multi-instance deployment will independently fireReceivingJob/AggregationJob/retry triggers. This is the same concern raised in a prior review and remains unresolved.
159-162: 🩺 Stability & AvailabilityPgSql scheduler storage bypasses managed-identity data source.
AddPgSqlSchedulerstill receives the rawconnectionStringinstead of the token-refreshingNpgsqlDataSourcebuilt further down for EF. Same as prior review feedback; still unresolved — scheduler storage will fail to authenticate whenUseAzureManagedIdentityis enabled.
33-37: LGTM!Also applies to: 47-54, 71-72, 88-89, 125-130, 334-334
SW.Bitween.Api/Services/BitweenOptions.cs (1)
70-74: LGTM!Also applies to: 81-87
SW.Bitween.Api/Data/BitweenDbContext.cs (1)
188-223: LGTM!Also applies to: 239-248, 261-261
SW.Bitween.Sdk/Model/Subscription.cs (1)
7-16: LGTM!SW.Bitween.MySql/Migrations/BitweenDbContextModelSnapshot.cs (1)
126-145: LGTM!Also applies to: 499-537, 561-563, 628-661, 772-774, 1022-1540, 1699-1739, 1801-1855, 1870-1885
SW.Bitween.Api/Domain/Subscription/Subscription.cs (1)
8-63: LGTM!Also applies to: 105-176
SW.Bitween.Api/Resources/Subscriptions/Update.cs (4)
58-64: 🩺 Stability & AvailabilityPost-commit scheduler sync still lacks durability.
_subScheduler.Sync(entity, oldSchedules)runs afterSaveChangesAsync()with no retry/compensation; a transient scheduler failure leaves the subscription committed but Quartz stale, and the request fails despite the successful update. Previously flagged for this same call site.
54-55: 🎯 Functional Correctness
RetryPolicyIdstill unvalidated before assignment.Invalid
RetryPolicyIdsurfaces as aDbUpdateExceptionatSaveChangesAsync()rather than a clean validation error. Previously flagged.
21-40: LGTM!
265-276: LGTM! BusGateway correctly grouped with GatewayApiCall for the PartnerId-must-be-null rule.SW.Bitween.PgSql/BitweenDbContext.cs (3)
357-371: 🗄️ Data Integrity & Integration
RetryPolicy.Groupsdeserialization still yields null on a null payload.
JsonSerializer.Deserialize<List<RetryGroup>>(json, _polymorphicOpts)!doesn't fall back fornull/"null"payloads, butRetryPolicy.Groupsis a non-null list. Previously flagged for this same conversion.
150-178: LGTM! BusGateway/BusGatewayRoute mappings mirror the existing Subscription MatchExpression conversion pattern.
383-394: LGTM!SW.Bitween.Api/Services/XchangeService.cs (4)
95-101: 🎯 Functional Correctness
referencesparameter still silently dropped. Not forwarded intonew Xchange(subscription, xchange, file, groupAttemptCounts); previously flagged.
131-153: 🎯 Functional Correctness | ⚡ Quick winAtomic claim before resubmission still missing (previously flagged — concurrent scheduled/RunNow callers can duplicate the xchange).
Separately, this method no longer honors
subscription.PausedOn:CreateXchangesForHits(Lines 517-520) routes paused subscriptions toCreateOnHoldXchange, butExecuteDelayedRetryunconditionally callsCreateXchange, bypassing the pause/hold mechanism entirely for retries. A subscription paused after the original failure will still get its retry processed.🐛 Suggested fix for the pause bypass
var inputFileData = await GetFile(xchange.Id, XchangeFileType.Input); var inputFile = new XchangeFile(inputFileData, xchange.InputName); - await CreateXchange(subscription, xchange, inputFile, groupAttemptCounts: delayedRetry.GroupAttemptCounts); + if (subscription.PausedOn != null) + await CreateOnHoldXchange(subscription, inputFile); + else + await CreateXchange(subscription, xchange, inputFile, groupAttemptCounts: delayedRetry.GroupAttemptCounts); _dbContext.Remove(delayedRetry);
88-94: LGTM!
481-527: LGTM on the bus-gateway route handling logic (subscription null-check, pause routing, partner resolution).SW.Bitween.MsSql/Migrations/BitweenDbContextModelSnapshot.cs (2)
501-539: 📐 Maintainability & Code Quality
RetryPolicycarries duplicated audit-field conventions here too.Same root cause already flagged for the PgSql snapshot:
RetryPolicymaps both the standardCreatedBy/CreatedOn/ModifiedBy/ModifiedOnconvention and a separateUpdatedAt/UpdatedBypair. Fix at the domain model (RetryPolicy.cs) before shipping — this generated snapshot will follow.
304-381: LGTM! BusGateway/BusGatewayRoute mappings are consistent with the DbContext configuration.Also applies to: 1591-1625, 1864-1867
SW.Bitween.PgSql/Migrations/BitweenDbContextModelSnapshot.cs (2)
609-657: 📐 Maintainability & Code Quality
RetryPolicyduplicated audit-field conventions — still present.Previously flagged for this exact mapping (both
CreatedBy/CreatedOn/ModifiedBy/ModifiedOnandUpdatedAt/UpdatedBy).
368-467: LGTM! BusGateway/BusGatewayRoute mappings match the DbContext configuration and naming conventions.Also applies to: 1830-1868, 2136-2139
SW.Bitween.Api/Domain/Xchange/Xchange.cs (1)
34-35: LGTM!
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
SW.Bitween.Web/Startup.cs (1)
164-169: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winMsSql scheduler connection string is built before the Managed-Identity
Authentication=clause is appended.
connectionStringis passed by value intoAddSqlServerSchedulerhere. The;Authentication=Active Directory Defaultsuffix is only appended later, at Lines 252-256, and only mutates the local variable used afterward forAddDbContext<...MsSql.BitweenDbContext>. WithUseAzureManagedIdentity=trueandDatabaseType=MsSql, the Quartz scheduler will attempt to connect without the managed-identity auth clause and fail — a distinct instance of the same "scheduler storage not managed-identity aware" defect, but for MsSql specifically.🔧 Proposed fix
+ // Compute the managed-identity-aware connection string before registering the scheduler. + if (bitweenOptions.UseAzureManagedIdentity && + bitweenOptions.DatabaseType.Equals(RelationalDbType.MsSql.ToString(), StringComparison.OrdinalIgnoreCase) && + !connectionString.Contains("Authentication=", StringComparison.OrdinalIgnoreCase)) + { + connectionString += ";Authentication=Active Directory Default"; + } + else if (string.Equals(bitweenOptions.DatabaseType, RelationalDbType.MsSql.ToString(), StringComparison.OrdinalIgnoreCase)) { services.AddSqlServerScheduler( connectionString: connectionString, assemblies: typeof(BitweenDbContext).Assembly); }And remove the now-redundant mutation later in the method:
// Lines 252-256 — remove since the string is now pre-augmented above if (bitweenOptions.UseAzureManagedIdentity && !connectionString.Contains("Authentication=", StringComparison.OrdinalIgnoreCase)) { connectionString += ";Authentication=Active Directory Default"; }🤖 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.Web/Startup.cs` around lines 164 - 169, The MsSql scheduler path in Startup’s AddSqlServerScheduler call is using the base connection string before the managed-identity Authentication clause is applied. Update the connection-string setup so the same pre-augmented value is used for both the Quartz scheduler and the later AddDbContext<...MsSql.BitweenDbContext> configuration when UseAzureManagedIdentity is enabled, and remove the later connectionString mutation that only affects the DbContext path.SW.Bitween.Api/Domain/Subscription/Subscription.cs (1)
71-74: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winNo invariant preventing
RetryPolicyIdandCustomRetryPolicyfrom being set simultaneously.Both are plain public-setter properties, unlike other mutations on this aggregate that go through dedicated
Set*methods enforcing invariants. Nothing here defines precedence if both are populated. Consider aSetRetryPolicy(...)method that enforces mutual exclusivity, and confirm how the retry evaluation logic (not in this batch) resolves the ambiguity today.Also, the
// this should be saved as jsoncomment is stale — already implemented viaStoreAsJson()inBitweenDbContext.cs.🤖 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/Domain/Subscription/Subscription.cs` around lines 71 - 74, The Subscription aggregate currently allows RetryPolicyId and CustomRetryPolicy to be set at the same time because they are exposed as public setters, which bypasses the invariant pattern used elsewhere in Subscription. Introduce a dedicated SetRetryPolicy(...) method on Subscription that enforces mutual exclusivity and updates both properties consistently, and use that method wherever retry policy is assigned. Also remove the stale “saved as json” comment on CustomRetryPolicy since persistence is already handled by StoreAsJson() in BitweenDbContext.SW.Bitween.Api/Data/BitweenDbContext.cs (1)
230-237: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the extra
UpdatedAt/UpdatedByfields fromRetryPolicy.
RetryPolicyalready has the standardCreated*/Modified*audit fields viaIAudited, andUpdated*is not part ofIRetryPolicyor used elsewhere. If these columns are intentional, map and document them explicitly; otherwise they add schema noise.🤖 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/Data/BitweenDbContext.cs` around lines 230 - 237, Remove the extra UpdatedAt/UpdatedBy mapping from the RetryPolicy entity in BitweenDbContext so the EF model matches IRetryPolicy and the existing IAudited fields only. Update the modelBuilder.Entity<RetryPolicy> configuration to keep the core properties (Id, Name, Groups) and either explicitly map/document any intentional updated fields elsewhere or drop them entirely if they are not part of the contract.SW.Bitween.Sdk/Model/Subscription.cs (1)
100-102: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winGuard retry-policy updates in
SW.Bitween.Api/Resources/Subscriptions/Update.cs:42-55.RetryPolicyId/CustomRetryPolicyare assigned directly, so invalid FK values still reachSaveChangesAsync, and a payload that sets both fields relies on implicitCustomRetryPolicyprecedence. The SDK and domain already use the sameSW.Bitween.Model.CustomRetryPolicytype, so no mapping is needed.🤖 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.Sdk/Model/Subscription.cs` around lines 100 - 102, The retry-policy update path in the subscription update flow assigns RetryPolicyId and CustomRetryPolicy directly, so invalid foreign keys can slip through until SaveChangesAsync and payloads that set both fields depend on implicit precedence. Update the subscription update logic in Update to validate the retry policy before assigning it, reject or ignore invalid RetryPolicyId values, and make the precedence between RetryPolicyId and CustomRetryPolicy explicit. Use the existing SW.Bitween.Model.CustomRetryPolicy type directly so no mapping layer is needed, and keep the fix localized to the subscription update handling.SW.Bitween.Api/Domain/Xchange/Xchange.cs (1)
63-112: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
GroupAttemptCountsexposes a mutable dictionary despite the private setter.Every other dictionary-typed property on this entity (
HandlerProperties,MapperProperties) isIReadOnlyDictionary<string, string>.GroupAttemptCountsbreaks that convention with a mutableDictionary<string, int>— callers holding the reference (e.g.evaluator.GetGroupAttemptCounts(),delayedRetry.GroupAttemptCounts) can mutate it after assignment, silently corrupting retry-tracking state that later gets persisted via EF.♻️ Suggested fix
- public Dictionary<string, int> GroupAttemptCounts { get; private set; } + public IReadOnlyDictionary<string, int> GroupAttemptCounts { get; private set; }Update constructor parameters/assignments to accept
IReadOnlyDictionary<string,int>or defensively copy on assignment.🤖 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/Domain/Xchange/Xchange.cs` around lines 63 - 112, `GroupAttemptCounts` in `Xchange` is exposing mutable retry state through a `Dictionary<string, int>` even though the property has a private setter. Update the `Xchange` constructors that accept `groupAttemptCounts` to take `IReadOnlyDictionary<string, int>` (or defensively copy the incoming dictionary) and change the `GroupAttemptCounts` property to an immutable/read-only shape consistent with `HandlerProperties` and `MapperProperties`, so callers like `evaluator.GetGroupAttemptCounts()` and `delayedRetry.GroupAttemptCounts` can’t mutate it after assignment.SW.Bitween.Api/Services/XchangeService.cs (2)
463-478: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
CountRetryChainDepthperforms sequential per-level DB round trips.Each retry evaluation walks the
RetryForchain one row at a time viaawaitinside thewhileloop — O(depth) sequential queries on every message failure. For subscriptions with several configured retry attempts this adds material latency to a hot failure-handling path. Consider a single recursive query, or simpler: persist the attempt index directly onXchange/DelayedRetryat creation time instead of recomputing it by chain traversal.🤖 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 463 - 478, CountRetryChainDepth is doing one awaited database lookup per RetryFor hop, causing sequential round trips on the hot failure path. Refactor XchangeService.CountRetryChainDepth to avoid walking the chain row-by-row: either fetch the retry chain in one query/recursive query or stop recomputing depth here by storing the attempt index on Xchange/DelayedRetry when the retry is created. Keep the logic localized to CountRetryChainDepth and the RetryFor chain handling so the call sites can use the precomputed value.
435-461: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winGuard
DelayedRetrycreation against duplicateIds
DelayedRetry.Idis the PK, so processing the sameXchangeMessagetwice will hitSaveChangesAsync()with a duplicate-key insert and abort the whole save, including theXchangeResult. Use an upsert or handle the existing row before adding a new one.🤖 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 435 - 461, TryScheduleAutoRetry currently always adds a new DelayedRetry with Xchange.Id as the primary key, so duplicate processing can cause SaveChangesAsync to fail. Update TryScheduleAutoRetry to check for an existing DelayedRetry for the same xchange.Id before calling _dbContext.Add, and either reuse/update the existing row or skip inserting a duplicate. Keep the retry scheduling logic in TryScheduleAutoRetry and the DelayedRetry entity handling aligned with the PK constraint.
🤖 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/Data/BitweenDbContext.cs`:
- Around line 118-146: The MatchExpression value-conversion logic is duplicated
between the BusGatewayRoute and Subscription mappings in BitweenDbContext, so
extract the inline HasConversion lambda pair using
MatchSpecValueConverter.SerializeMatchSpec and DeserializeMatchSpec into a
shared converter or reusable configuration helper. Then apply that shared
converter from both entity configurations (BusGatewayRoute and Subscription) to
keep the mapping consistent and avoid drift.
---
Outside diff comments:
In `@SW.Bitween.Api/Data/BitweenDbContext.cs`:
- Around line 230-237: Remove the extra UpdatedAt/UpdatedBy mapping from the
RetryPolicy entity in BitweenDbContext so the EF model matches IRetryPolicy and
the existing IAudited fields only. Update the modelBuilder.Entity<RetryPolicy>
configuration to keep the core properties (Id, Name, Groups) and either
explicitly map/document any intentional updated fields elsewhere or drop them
entirely if they are not part of the contract.
In `@SW.Bitween.Api/Domain/Subscription/Subscription.cs`:
- Around line 71-74: The Subscription aggregate currently allows RetryPolicyId
and CustomRetryPolicy to be set at the same time because they are exposed as
public setters, which bypasses the invariant pattern used elsewhere in
Subscription. Introduce a dedicated SetRetryPolicy(...) method on Subscription
that enforces mutual exclusivity and updates both properties consistently, and
use that method wherever retry policy is assigned. Also remove the stale “saved
as json” comment on CustomRetryPolicy since persistence is already handled by
StoreAsJson() in BitweenDbContext.
In `@SW.Bitween.Api/Domain/Xchange/Xchange.cs`:
- Around line 63-112: `GroupAttemptCounts` in `Xchange` is exposing mutable
retry state through a `Dictionary<string, int>` even though the property has a
private setter. Update the `Xchange` constructors that accept
`groupAttemptCounts` to take `IReadOnlyDictionary<string, int>` (or defensively
copy the incoming dictionary) and change the `GroupAttemptCounts` property to an
immutable/read-only shape consistent with `HandlerProperties` and
`MapperProperties`, so callers like `evaluator.GetGroupAttemptCounts()` and
`delayedRetry.GroupAttemptCounts` can’t mutate it after assignment.
In `@SW.Bitween.Api/Services/XchangeService.cs`:
- Around line 463-478: CountRetryChainDepth is doing one awaited database lookup
per RetryFor hop, causing sequential round trips on the hot failure path.
Refactor XchangeService.CountRetryChainDepth to avoid walking the chain
row-by-row: either fetch the retry chain in one query/recursive query or stop
recomputing depth here by storing the attempt index on Xchange/DelayedRetry when
the retry is created. Keep the logic localized to CountRetryChainDepth and the
RetryFor chain handling so the call sites can use the precomputed value.
- Around line 435-461: TryScheduleAutoRetry currently always adds a new
DelayedRetry with Xchange.Id as the primary key, so duplicate processing can
cause SaveChangesAsync to fail. Update TryScheduleAutoRetry to check for an
existing DelayedRetry for the same xchange.Id before calling _dbContext.Add, and
either reuse/update the existing row or skip inserting a duplicate. Keep the
retry scheduling logic in TryScheduleAutoRetry and the DelayedRetry entity
handling aligned with the PK constraint.
In `@SW.Bitween.Sdk/Model/Subscription.cs`:
- Around line 100-102: The retry-policy update path in the subscription update
flow assigns RetryPolicyId and CustomRetryPolicy directly, so invalid foreign
keys can slip through until SaveChangesAsync and payloads that set both fields
depend on implicit precedence. Update the subscription update logic in Update to
validate the retry policy before assigning it, reject or ignore invalid
RetryPolicyId values, and make the precedence between RetryPolicyId and
CustomRetryPolicy explicit. Use the existing SW.Bitween.Model.CustomRetryPolicy
type directly so no mapping layer is needed, and keep the fix localized to the
subscription update handling.
In `@SW.Bitween.Web/Startup.cs`:
- Around line 164-169: The MsSql scheduler path in Startup’s
AddSqlServerScheduler call is using the base connection string before the
managed-identity Authentication clause is applied. Update the connection-string
setup so the same pre-augmented value is used for both the Quartz scheduler and
the later AddDbContext<...MsSql.BitweenDbContext> configuration when
UseAzureManagedIdentity is enabled, and remove the later connectionString
mutation that only affects the DbContext path.
🪄 Autofix (Beta)
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
Run ID: 4aae606f-98de-42c0-86fe-e338c0634252
📒 Files selected for processing (12)
SW.Bitween.Api/Data/BitweenDbContext.csSW.Bitween.Api/Domain/Subscription/Subscription.csSW.Bitween.Api/Domain/Xchange/Xchange.csSW.Bitween.Api/Resources/Subscriptions/Update.csSW.Bitween.Api/Services/BitweenOptions.csSW.Bitween.Api/Services/XchangeService.csSW.Bitween.MsSql/Migrations/BitweenDbContextModelSnapshot.csSW.Bitween.MySql/Migrations/BitweenDbContextModelSnapshot.csSW.Bitween.PgSql/BitweenDbContext.csSW.Bitween.PgSql/Migrations/BitweenDbContextModelSnapshot.csSW.Bitween.Sdk/Model/Subscription.csSW.Bitween.Web/Startup.cs
📜 Review details
🔇 Additional comments (24)
SW.Bitween.Web/Startup.cs (3)
151-176: 🩺 Stability & AvailabilityClustering still disabled — contradicts PR's stated goal.
The added comment documents that clustering is NOT enabled pending a
SimplyWorks.Schedulerpackage bump, yet the PR description states "Clustering is enabled so that only one node fires each trigger." As shipped, every node in a multi-instance deployment will independently fireReceivingJob/AggregationJob/retry triggers. This is the same concern raised in a prior review and remains unresolved.
159-162: 🩺 Stability & AvailabilityPgSql scheduler storage bypasses managed-identity data source.
AddPgSqlSchedulerstill receives the rawconnectionStringinstead of the token-refreshingNpgsqlDataSourcebuilt further down for EF. Same as prior review feedback; still unresolved — scheduler storage will fail to authenticate whenUseAzureManagedIdentityis enabled.
33-37: LGTM!Also applies to: 47-54, 71-72, 88-89, 125-130, 334-334
SW.Bitween.Api/Services/BitweenOptions.cs (1)
70-74: LGTM!Also applies to: 81-87
SW.Bitween.Api/Data/BitweenDbContext.cs (1)
188-223: LGTM!Also applies to: 239-248, 261-261
SW.Bitween.Sdk/Model/Subscription.cs (1)
7-16: LGTM!SW.Bitween.MySql/Migrations/BitweenDbContextModelSnapshot.cs (1)
126-145: LGTM!Also applies to: 499-537, 561-563, 628-661, 772-774, 1022-1540, 1699-1739, 1801-1855, 1870-1885
SW.Bitween.Api/Domain/Subscription/Subscription.cs (1)
8-63: LGTM!Also applies to: 105-176
SW.Bitween.Api/Resources/Subscriptions/Update.cs (4)
58-64: 🩺 Stability & AvailabilityPost-commit scheduler sync still lacks durability.
_subScheduler.Sync(entity, oldSchedules)runs afterSaveChangesAsync()with no retry/compensation; a transient scheduler failure leaves the subscription committed but Quartz stale, and the request fails despite the successful update. Previously flagged for this same call site.
54-55: 🎯 Functional Correctness
RetryPolicyIdstill unvalidated before assignment.Invalid
RetryPolicyIdsurfaces as aDbUpdateExceptionatSaveChangesAsync()rather than a clean validation error. Previously flagged.
21-40: LGTM!
265-276: LGTM! BusGateway correctly grouped with GatewayApiCall for the PartnerId-must-be-null rule.SW.Bitween.PgSql/BitweenDbContext.cs (3)
357-371: 🗄️ Data Integrity & Integration
RetryPolicy.Groupsdeserialization still yields null on a null payload.
JsonSerializer.Deserialize<List<RetryGroup>>(json, _polymorphicOpts)!doesn't fall back fornull/"null"payloads, butRetryPolicy.Groupsis a non-null list. Previously flagged for this same conversion.
150-178: LGTM! BusGateway/BusGatewayRoute mappings mirror the existing Subscription MatchExpression conversion pattern.
383-394: LGTM!SW.Bitween.Api/Services/XchangeService.cs (4)
95-101: 🎯 Functional Correctness
referencesparameter still silently dropped. Not forwarded intonew Xchange(subscription, xchange, file, groupAttemptCounts); previously flagged.
131-153: 🎯 Functional Correctness | ⚡ Quick winAtomic claim before resubmission still missing (previously flagged — concurrent scheduled/RunNow callers can duplicate the xchange).
Separately, this method no longer honors
subscription.PausedOn:CreateXchangesForHits(Lines 517-520) routes paused subscriptions toCreateOnHoldXchange, butExecuteDelayedRetryunconditionally callsCreateXchange, bypassing the pause/hold mechanism entirely for retries. A subscription paused after the original failure will still get its retry processed.🐛 Suggested fix for the pause bypass
var inputFileData = await GetFile(xchange.Id, XchangeFileType.Input); var inputFile = new XchangeFile(inputFileData, xchange.InputName); - await CreateXchange(subscription, xchange, inputFile, groupAttemptCounts: delayedRetry.GroupAttemptCounts); + if (subscription.PausedOn != null) + await CreateOnHoldXchange(subscription, inputFile); + else + await CreateXchange(subscription, xchange, inputFile, groupAttemptCounts: delayedRetry.GroupAttemptCounts); _dbContext.Remove(delayedRetry);
88-94: LGTM!
481-527: LGTM on the bus-gateway route handling logic (subscription null-check, pause routing, partner resolution).SW.Bitween.MsSql/Migrations/BitweenDbContextModelSnapshot.cs (2)
501-539: 📐 Maintainability & Code Quality
RetryPolicycarries duplicated audit-field conventions here too.Same root cause already flagged for the PgSql snapshot:
RetryPolicymaps both the standardCreatedBy/CreatedOn/ModifiedBy/ModifiedOnconvention and a separateUpdatedAt/UpdatedBypair. Fix at the domain model (RetryPolicy.cs) before shipping — this generated snapshot will follow.
304-381: LGTM! BusGateway/BusGatewayRoute mappings are consistent with the DbContext configuration.Also applies to: 1591-1625, 1864-1867
SW.Bitween.PgSql/Migrations/BitweenDbContextModelSnapshot.cs (2)
609-657: 📐 Maintainability & Code Quality
RetryPolicyduplicated audit-field conventions — still present.Previously flagged for this exact mapping (both
CreatedBy/CreatedOn/ModifiedBy/ModifiedOnandUpdatedAt/UpdatedBy).
368-467: LGTM! BusGateway/BusGatewayRoute mappings match the DbContext configuration and naming conventions.Also applies to: 1830-1868, 2136-2139
SW.Bitween.Api/Domain/Xchange/Xchange.cs (1)
34-35: LGTM!
🛑 Comments failed to post (1)
SW.Bitween.Api/Data/BitweenDbContext.cs (1)
118-146: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Duplicate
MatchExpressionvalue-conversion logic.Same inline
HasConversionlambda pair (MatchSpecValueConverter.SerializeMatchSpec/DeserializeMatchSpec) is repeated forSubscription.MatchExpression(Lines 224-227) andBusGatewayRoute.MatchExpression(Lines 141-144). Extract to a shared converter to avoid divergence.♻️ Proposed refactor
+ private static readonly ValueConverter<IPropertyMatchSpecification, string> MatchExpressionConverter = + new( + domainObject => domainObject == null ? null : MatchSpecValueConverter.SerializeMatchSpec(domainObject), + dbString => dbString == null ? null : MatchSpecValueConverter.DeserializeMatchSpec(dbString));Then reuse at both call sites:
- bgr.Property(p => p.MatchExpression).HasConversion( - domainObject => - domainObject == null ? null : MatchSpecValueConverter.SerializeMatchSpec(domainObject), - dbString => dbString == null ? null : MatchSpecValueConverter.DeserializeMatchSpec(dbString)); + bgr.Property(p => p.MatchExpression).HasConversion(MatchExpressionConverter); ... - b.Property(p => p.MatchExpression).HasConversion( - domainObject => - domainObject == null ? null : MatchSpecValueConverter.SerializeMatchSpec(domainObject), - dbString => dbString == null ? null : MatchSpecValueConverter.DeserializeMatchSpec(dbString)); + b.Property(p => p.MatchExpression).HasConversion(MatchExpressionConverter);Also applies to: 224-227
🤖 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/Data/BitweenDbContext.cs` around lines 118 - 146, The MatchExpression value-conversion logic is duplicated between the BusGatewayRoute and Subscription mappings in BitweenDbContext, so extract the inline HasConversion lambda pair using MatchSpecValueConverter.SerializeMatchSpec and DeserializeMatchSpec into a shared converter or reusable configuration helper. Then apply that shared converter from both entity configurations (BusGatewayRoute and Subscription) to keep the mapping consistent and avoid drift.
2 participants
…jobs via SW-Scheduler
Replace the previous polling services with IScheduledJob implementations (ReceivingJob, AggregationJob) driven by SW-Scheduler — a typed Quartz.NET wrapper. Quartz tables are added via EF Core migrations on all three DB providers (PgSql, MySql, MsSql). SchedulerSeedService re-registers active subscriptions idempotently on startup; SubscriptionSchedulerService bridges the Schedule domain entity to IScheduleRepository. Clustering is enabled so only one node fires each trigger in a multi-node deployment.
Also adds SW.Bitween.IntegrationTests with Testcontainers-based tests for entity persistence, bus connectivity, receiving jobs, and aggregation jobs.
Docs: docs/scheduler.md (new), docs/architecture.md (scheduler section added).