Conversation
|
Thanks for opening this, but we'd appreciate a little more information. Could you update it with more details? |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (10)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughWalkthroughA new ChangesQuidjibo.SqlServer SQL Server Provider
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 20
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Workers/Support/Quidjibo.SqlServer/Scripts/Schedule/List.sql (1)
1-2: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winAdd the missing
FROMclause.This script is not valid SQL as written, so any list call will fail immediately when SQL Server parses it.
Suggested fix
SELECT TOP(`@Take`) [Quidjibo].[Schedule].* +FROM [Quidjibo].[Schedule]🤖 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 `@Workers/Support/Quidjibo.SqlServer/Scripts/Schedule/List.sql` around lines 1 - 2, The list query is missing its FROM clause, so the SQL in the Schedule listing script is invalid. Update the query built around the SELECT TOP(`@Take`) [Quidjibo].[Schedule].* statement to include the correct source table reference for the Schedule rows, ensuring the list operation parses and executes successfully.
🧹 Nitpick comments (8)
Workers/Support/Quidjibo.SqlServer/Utils/SqlLoader.cs (1)
10-10: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCache dictionary should be
readonly.The static
Scriptsfield is reassignment-safe but not markedreadonly. As per coding guidelines, prefer immutable data where appropriate.♻️ Proposed fix
- private static readonly ConcurrentDictionary<string, string> Scripts = new ConcurrentDictionary<string, string>(); + private static readonly ConcurrentDictionary<string, string> Scripts = new ConcurrentDictionary<string, string>();🤖 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 `@Workers/Support/Quidjibo.SqlServer/Utils/SqlLoader.cs` at line 10, The static Scripts cache in SqlLoader should be declared as readonly to match the coding guideline for immutable references. Update the Scripts field in SqlLoader so the ConcurrentDictionary reference cannot be reassigned, keeping the existing cache behavior intact while making the intent explicit.Workers/Support/Quidjibo.SqlServer/Utils/SqlRunner.cs (1)
20-43: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant nested block inside
usingstatements.Lines 20 and 43 form an unnecessary
{ ... }block inside theusingscope with no functional purpose. Remove to reduce nesting.♻️ Proposed fix
using (var conn = new SqlConnection(connectionString)) using (var cmd = conn.CreateCommand()) { - { await conn.OpenAsync(cancellationToken); ... - } }🤖 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 `@Workers/Support/Quidjibo.SqlServer/Utils/SqlRunner.cs` around lines 20 - 43, Remove the unnecessary inner braces inside the SqlRunner execution flow so the code stays flat within the existing using scopes. In the method that opens the connection and handles the transaction path, keep the await conn.OpenAsync and the inTransaction logic, but eliminate the redundant { ... } block around the transaction handling since it adds nesting without changing behavior.Workers/Support/Quidjibo.SqlServer/Extensions/SqlParameterCollectionExtensions.cs (2)
6-11: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueClass name claims
SqlParameterCollectionbut extendsSqlCommand.The class is named
SqlParameterCollectionExtensionsbut the extension method targetsSqlCommand. Rename toSqlCommandExtensionsor move the method to an existingSqlCommandExtensionsclass to match the target type.🤖 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 `@Workers/Support/Quidjibo.SqlServer/Extensions/SqlParameterCollectionExtensions.cs` around lines 6 - 11, The extension class name does not match the type being extended: SqlParameterCollectionExtensions currently adds AddParameter to SqlCommand. Rename the class to SqlCommandExtensions or move AddParameter into the existing SqlCommandExtensions so the class name aligns with the SqlCommand target and the extension remains easy to find.
8-11: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
AddWithValueinfersSqlDbTypefrom runtime type, risking query plan cache pollution and type mismatches.When
valueis null,DBNull.Valueis passed andAddWithValueinfers a default type (oftenNVarChar). For parameters that should beInt,DateTime, etc., this can cause implicit conversions, index scans, and plan cache bloat. Consider an overload acceptingSqlDbTypeor using explicitSqlParameterconstruction for critical paths.♻️ Proposed fix (add typed overload)
public static SqlParameter AddParameter(this SqlCommand cmd, string name, object value) { return cmd.Parameters.AddWithValue(name, value ?? DBNull.Value); } + public static SqlParameter AddParameter(this SqlCommand cmd, string name, SqlDbType type, object value) + { + return cmd.Parameters.Add(new SqlParameter(name, type) { Value = value ?? DBNull.Value }); + }🤖 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 `@Workers/Support/Quidjibo.SqlServer/Extensions/SqlParameterCollectionExtensions.cs` around lines 8 - 11, The AddParameter extension in SqlParameterCollectionExtensions uses AddWithValue, which infers types from runtime values and can cause mismatches and plan cache issues. Update SqlCommand parameter creation to use explicit SqlParameter construction or add a typed overload that accepts SqlDbType so callers can specify the intended database type, especially when value is null. Keep AddParameter as the location to fix and ensure it no longer relies on AddWithValue for critical parameters.Workers/Support/Quidjibo.SqlServer/Providers/SqlWorkProvider.cs (1)
39-55: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlign provider construction with the repository dependency pattern.
This adds an
ILoggerconstructor dependency to the provider API. Prefer removing it unless needed, and use the repository logging pattern when logging is added. As per coding guidelines, “UseService Locatorpattern viaBootstrapper.GetKernel().Resolve<T>()to resolve dependencies explicitly in constructors, rather than constructor injection” and “UseResgrid.Framework.Loggingstatic methods for logging.”🤖 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 `@Workers/Support/Quidjibo.SqlServer/Providers/SqlWorkProvider.cs` around lines 39 - 55, The SqlWorkProvider constructor currently adds an ILogger dependency that conflicts with the repository’s dependency pattern. Remove the constructor-injected logger from SqlWorkProvider unless it is actually used, and if logging is needed later, follow the existing repository approach by resolving dependencies through Bootstrapper.GetKernel().Resolve<T>() and using Resgrid.Framework.Logging static methods instead of constructor injection.Source: Coding guidelines
Workers/Support/Quidjibo.SqlServer/Providers/SqlScheduleProvider.cs (1)
23-31: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftAlign dependency resolution with the project pattern.
ILoggeris constructor-injected here; the project guideline asks C# constructors to resolve dependencies throughBootstrapper.GetKernel().Resolve<T>()instead. As per coding guidelines,**/*.cs: UseService Locatorpattern viaBootstrapper.GetKernel().Resolve<T>()to resolve dependencies explicitly in constructors, rather than constructor injection.🤖 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 `@Workers/Support/Quidjibo.SqlServer/Providers/SqlScheduleProvider.cs` around lines 23 - 31, The SqlScheduleProvider constructor currently takes ILogger via constructor injection, which does not follow the project’s Service Locator pattern. Update the SqlScheduleProvider constructor to resolve ILogger explicitly through Bootstrapper.GetKernel().Resolve<T>() inside the constructor, while keeping the existing assignment of _connectionString and _queues unchanged. Use the SqlScheduleProvider symbol to locate the constructor and replace the ILogger parameter usage with the resolved dependency.Source: Coding guidelines
Workers/Support/Quidjibo.SqlServer/Factories/SqlScheduleProviderFactory.cs (1)
35-46: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winCache schedule initialization after the first successful setup.
This factory reruns
Schema.SetupandSchedule.Setupon everyCreateAsynccall while holding a static semaphore. That serializes every schedule-provider creation and keeps issuing the same setup work against SQL Server after startup.Proposed refactor
public class SqlScheduleProviderFactory : IScheduleProviderFactory { private static readonly SemaphoreSlim SyncLock = new SemaphoreSlim(1, 1); private readonly string _connectionString; + private bool _initialized; @@ await SyncLock.WaitAsync(cancellationToken); lockTaken = true; - await SqlRunner.ExecuteAsync(async cmd => + if (!_initialized) { - var schemaSetup = await SqlLoader.GetScript("Schema.Setup"); - var scheduleSetup = await SqlLoader.GetScript("Schedule.Setup"); - cmd.CommandText = $"{schemaSetup};\r\n{scheduleSetup}"; - await cmd.ExecuteNonQueryAsync(cancellationToken); - }, _connectionString, false, cancellationToken); + await SqlRunner.ExecuteAsync(async cmd => + { + var schemaSetup = await SqlLoader.GetScript("Schema.Setup"); + var scheduleSetup = await SqlLoader.GetScript("Schedule.Setup"); + cmd.CommandText = $"{schemaSetup};\r\n{scheduleSetup}"; + await cmd.ExecuteNonQueryAsync(cancellationToken); + }, _connectionString, false, cancellationToken); + _initialized = true; + } return await Task.FromResult<IScheduleProvider>(new SqlScheduleProvider( _loggerFactory.CreateLogger<SqlScheduleProvider>(), _connectionString, queues));🤖 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 `@Workers/Support/Quidjibo.SqlServer/Factories/SqlScheduleProviderFactory.cs` around lines 35 - 46, The SqlScheduleProviderFactory.CreateAsync flow is rerunning Schema.Setup and Schedule.Setup on every call and serializing all callers behind SyncLock. Add a one-time initialization cache in SqlScheduleProviderFactory so the setup scripts run only once after the first successful completion, and have subsequent CreateAsync calls skip the SQL setup work while still returning a new SqlScheduleProvider.Workers/Support/Quidjibo.SqlServer/Providers/SqlProgressProvider.cs (1)
17-25: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the injected
ILoggerhere or align it with the repo logging pattern.This class never logs, so carrying a constructor-injected logger adds a dependency without behavior. As per coding guidelines,
Use Service Locator pattern via Bootstrapper.GetKernel().Resolve<T>() to resolve dependencies explicitly in constructors, rather than constructor injectionandUse Resgrid.Framework.Logging static methods (LogException, LogError, LogInfo, LogDebug) for all logging throughout the codebase.🤖 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 `@Workers/Support/Quidjibo.SqlServer/Providers/SqlProgressProvider.cs` around lines 17 - 25, The SqlProgressProvider constructor currently injects ILogger even though the class does not use logging, so remove the unused logger dependency from SqlProgressProvider and its constructor signature, or if logging is actually needed, switch this class to the repo’s standard Resgrid.Framework.Logging static methods and resolve dependencies explicitly via Bootstrapper.GetKernel().Resolve<T>() instead of constructor injection.Source: Coding guidelines
🤖 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 `@Workers/Support/Quidjibo.SqlServer/Extensions/QuidjiboBuilderExtensions.cs`:
- Around line 16-20: The Sql Server builder entry points are accepting
null/blank inputs and deferring failures until connection time; add eager
validation in the public extension methods so startup fails immediately. In
QuidjiboBuilderExtensions, validate the configuration delegate/config object
before using it, and check the connection string for null/whitespace in the
UseSqlServer overloads and any related setup methods referenced by the comment.
Use the existing symbols like UseSqlServer and SqlServerQuidjiboConfiguration to
locate all affected entry points and throw an argument validation failure before
passing values into the factories.
- Around line 68-71: The UseSqlServerForWork method is only replacing the work
provider factory and never applies the provided SqlServerQuidjiboConfiguration
to the QuidjiboBuilder, so caller-supplied builder settings are ignored. Update
UseSqlServerForWork in QuidjiboBuilderExtensions to also bind/apply the passed
configuration the same way the full UseSqlServer overloads do, while still
configuring SqlWorkProviderFactory, so settings like PollingInterval,
WorkPollingInterval, EnableWorker, SingleLoop, and Throttle are honored.
In `@Workers/Support/Quidjibo.SqlServer/Extensions/SqlCommandExtensions.cs`:
- Around line 39-67: The work transition helpers in SqlCommandExtensions
currently rely only on `@Id`, which allows stale workers to update rows after
lease transfer. Update PrepareForRenewAsync, PrepareForCompleteAsync, and
PrepareForFaultAsync to accept and add a lease owner/token or worker-specific
value, then ensure the corresponding SQL scripts use that value in the WHERE
clause so only the current owner can renew, complete, or fault a work item.
In `@Workers/Support/Quidjibo.SqlServer/Factories/SqlProgressProviderFactory.cs`:
- Around line 39-55: The issue is that `SqlProgressProviderFactory` always calls
`SyncLock.Release()` in the `finally` block even when
`WaitAsync(cancellationToken)` never successfully acquired the semaphore, which
can surface as `SemaphoreFullException` and mask cancellation. Update the
`SqlProgressProviderFactory` flow to track whether `SyncLock.WaitAsync`
completed successfully before entering the protected section, and only release
the semaphore in `finally` when acquisition actually happened. Keep the fix
localized around `SyncLock`, `WaitAsync`, and the `SqlRunner.ExecuteAsync` setup
path.
In `@Workers/Support/Quidjibo.SqlServer/Factories/SqlScheduleProviderFactory.cs`:
- Around line 33-50: The SqlScheduleProviderFactory flow currently releases
SyncLock in the finally block even if WaitAsync never acquired it, which can
turn cancellation or an early failure into SemaphoreFullException. Update the
Create method to track ownership with a lockTaken flag around
SyncLock.WaitAsync, set it only after acquisition succeeds, and call
SyncLock.Release() in finally only when lockTaken is true; keep the rest of the
SqlRunner.ExecuteAsync and SqlScheduleProvider creation logic unchanged.
In `@Workers/Support/Quidjibo.SqlServer/Factories/SqlWorkProviderFactory.cs`:
- Around line 34-59: The semaphore handling in
SqlWorkProviderFactory.CreateAsync releases SyncLock unconditionally in the
finally block, which can throw if WaitAsync was canceled before the lock was
acquired. Add a lockTaken flag (or equivalent) around the await on
SyncLock.WaitAsync and only call SyncLock.Release() after acquisition succeeds;
keep the initialization and SqlRunner.ExecuteAsync path unchanged, but ensure
the finally block respects whether the semaphore was actually entered.
In `@Workers/Support/Quidjibo.SqlServer/Providers/SqlScheduleProvider.cs`:
- Around line 144-156: Map the missing VisibleOn field in MapScheduleItem so
schedule items retain the state persisted by CreateAsync and returned by
Receive.sql. Update the ScheduleItem object initializer in
SqlScheduleProvider.MapScheduleItem to read VisibleOn from the SqlDataReader
using the same mapping pattern as the other properties, referencing
ScheduleItem.VisibleOn and the MapScheduleItem helper to keep loaded/received
items consistent.
- Around line 30-56: Handle the empty-queue path in
SqlScheduleProvider.ReceiveAsync before executing the Schedule.Receive script.
When _queues.Length is 0, _receiveSql still contains `@Queue1` but no
`@Queue0/`@Queue1 parameters are added, so adjust the logic in ReceiveAsync (and
the _receiveSql template replacement) to either use a no-queue query variant or
skip the queue placeholder replacement when there are no queues. Ensure the
dynamic parameter binding block in ExecuteAsync only runs when queues exist, so
polling works correctly for the empty _queues case.
In `@Workers/Support/Quidjibo.SqlServer/Providers/SqlWorkProvider.cs`:
- Around line 71-97: Handle the zero-queue case in SqlWorkProvider.Receive
before calling ExecuteAsync: when _queues.Length is 0, ensure the receive SQL is
adjusted so it does not reference `@Queue1`, or skip adding a queue filter
entirely to preserve the default/all-queue behavior. Update the logic around
_receiveSql setup and the dynamic parameter loop in Receive so the query and
parameters stay in sync for both queued and non-queued modes.
In `@Workers/Support/Quidjibo.SqlServer/Scripts/Progress/LoadByCorrelationId.sql`:
- Around line 1-3: Add an explicit ordering to the LoadByCorrelationId.sql query
so progress history is returned deterministically; the current SELECT from
[Quidjibo].[Progress] filtered by `@CorrelationId` has no ORDER BY, so update it
to sort by the [Sequence] column ascending. Keep the existing projection and
READPAST hint, and ensure callers of this progress timeline see rows in the
intended sequence.
In `@Workers/Support/Quidjibo.SqlServer/Scripts/Progress/Setup.sql`:
- Around line 6-12: The table schema allows nullable values for WorkId and
RecordedOn, but SqlProgressProvider.LoadByCorrelationIdAsync expects both to
always be present when rehydrating with Map<Guid> and Map<DateTime>. Update the
Setup.sql definition to make those columns non-nullable so the database contract
matches the provider’s round-trip assumptions, and ensure the schema aligns with
the existing SqlProgressProvider.LoadByCorrelationIdAsync mapping.
- Around line 1-22: The Progress.Setup script creates [Quidjibo].[Progress]
without guaranteeing the [Quidjibo] schema exists, so add schema bootstrapping
before the CREATE TABLE block. Update Setup.sql to create [Quidjibo] if it is
missing, and make sure the initialization flow in
SqlProgressProviderFactory.CreateAsync still works on a fresh database without
relying on Schema/Setup.sql having run first.
In `@Workers/Support/Quidjibo.SqlServer/Scripts/Schedule/Complete.sql`:
- Around line 1-5: Remove READPAST from the UPDATE in Complete.sql so the
completion write cannot silently skip a locked schedule row. Update the
completion path used by SqlScheduleProvider.CompleteAsync to keep the row
targeted even when it is contended, leaving ROWLOCK and UPDLOCK as needed but
not READPAST. This ensures the Schedule row identified by [Id] is actually
updated instead of being skipped and left eligible for reprocessing.
In `@Workers/Support/Quidjibo.SqlServer/Scripts/Work/Complete.sql`:
- Around line 5-6: The Complete.sql update uses READPAST with the Work row
lookup, which can skip a locked row and make SqlWorkProvider.CompleteAsync
appear to succeed even when nothing was updated. Remove READPAST from the
Complete script’s update against Quidjibo.Work, or add an explicit affected-row
validation in CompleteAsync so a zero-row completion is treated as a failure and
does not silently succeed.
In `@Workers/Support/Quidjibo.SqlServer/Scripts/Work/List.sql`:
- Line 1: Work.List is still a placeholder and does not return any rows, so
callers get no result set. Implement the actual SELECT query in the Work.List
script, using the existing Work.List resource as the entry point, and ensure it
returns the expected list of work items in all states before the list path is
wired to it.
In `@Workers/Support/Quidjibo.SqlServer/Scripts/Work/Renew.sql`:
- Around line 1-4: The renewal update currently matches only on `Id`, so a stale
worker can extend a work item it no longer owns. Update the `Renew.sql` logic
used by `PrepareForRenewAsync` to also filter by the current lease state/value
that was originally observed when the work item was locked, not just the
identifier. Make sure the caller passes that expected lease/status alongside
`@Id` and `@VisibleOn`, and treat an update affecting 0 rows as a failed renew
so ownership loss is detected.
In `@Workers/Support/Quidjibo.SqlServer/Scripts/Work/Send.sql`:
- Around line 6-13: The INSERT in Send.sql is binding `@Worker` in
PrepareForSendAsync but not persisting it, so either add the Worker column to
the INSERT/value list or remove the parameter binding entirely. Update the Send
script to keep the column list, parameter list, and WorkItem.Worker mapping in
sync, and check the related insert block that also applies to the other
referenced range.
In `@Workers/Support/Quidjibo.SqlServer/Utils/SqlLoader.cs`:
- Around line 12-29: GetScript in SqlLoader is caching a missing manifest
resource as an empty script because it falls back to a blank stream when
GetManifestResourceStream returns null. Update GetScript to detect the missing
resource for the computed fullName and throw an explicit exception immediately
instead of reading from a default stream, so Scripts only caches valid script
contents and failures are observable.
In `@Workers/Support/Quidjibo.SqlServer/Utils/SqlRunner.cs`:
- Around line 26-37: The transaction rollback path in the SqlRunner command
execution flow is catching exceptions without logging them, so update the catch
block around the tran.Commit/tran.Rollback logic to call
Resgrid.Framework.Logging.LogException before rethrowing. Use the existing
SqlRunner method that wraps func(cmd) and transaction handling, and make sure
the logged exception is the one being caught so the rollback failure is recorded
with caller context automatically.
- Around line 32-36: The catch block in SqlRunner currently calls
tran.Rollback() directly, which can mask the original exception if rollback
fails. Update the rollback path to use a nested try/catch around tran.Rollback()
so the original exception from the surrounding operation is preserved, and only
treat rollback failures as secondary errors. Keep the fix localized to the catch
block in SqlRunner.
---
Outside diff comments:
In `@Workers/Support/Quidjibo.SqlServer/Scripts/Schedule/List.sql`:
- Around line 1-2: The list query is missing its FROM clause, so the SQL in the
Schedule listing script is invalid. Update the query built around the SELECT
TOP(`@Take`) [Quidjibo].[Schedule].* statement to include the correct source table
reference for the Schedule rows, ensuring the list operation parses and executes
successfully.
---
Nitpick comments:
In
`@Workers/Support/Quidjibo.SqlServer/Extensions/SqlParameterCollectionExtensions.cs`:
- Around line 6-11: The extension class name does not match the type being
extended: SqlParameterCollectionExtensions currently adds AddParameter to
SqlCommand. Rename the class to SqlCommandExtensions or move AddParameter into
the existing SqlCommandExtensions so the class name aligns with the SqlCommand
target and the extension remains easy to find.
- Around line 8-11: The AddParameter extension in
SqlParameterCollectionExtensions uses AddWithValue, which infers types from
runtime values and can cause mismatches and plan cache issues. Update SqlCommand
parameter creation to use explicit SqlParameter construction or add a typed
overload that accepts SqlDbType so callers can specify the intended database
type, especially when value is null. Keep AddParameter as the location to fix
and ensure it no longer relies on AddWithValue for critical parameters.
In `@Workers/Support/Quidjibo.SqlServer/Factories/SqlScheduleProviderFactory.cs`:
- Around line 35-46: The SqlScheduleProviderFactory.CreateAsync flow is
rerunning Schema.Setup and Schedule.Setup on every call and serializing all
callers behind SyncLock. Add a one-time initialization cache in
SqlScheduleProviderFactory so the setup scripts run only once after the first
successful completion, and have subsequent CreateAsync calls skip the SQL setup
work while still returning a new SqlScheduleProvider.
In `@Workers/Support/Quidjibo.SqlServer/Providers/SqlProgressProvider.cs`:
- Around line 17-25: The SqlProgressProvider constructor currently injects
ILogger even though the class does not use logging, so remove the unused logger
dependency from SqlProgressProvider and its constructor signature, or if logging
is actually needed, switch this class to the repo’s standard
Resgrid.Framework.Logging static methods and resolve dependencies explicitly via
Bootstrapper.GetKernel().Resolve<T>() instead of constructor injection.
In `@Workers/Support/Quidjibo.SqlServer/Providers/SqlScheduleProvider.cs`:
- Around line 23-31: The SqlScheduleProvider constructor currently takes ILogger
via constructor injection, which does not follow the project’s Service Locator
pattern. Update the SqlScheduleProvider constructor to resolve ILogger
explicitly through Bootstrapper.GetKernel().Resolve<T>() inside the constructor,
while keeping the existing assignment of _connectionString and _queues
unchanged. Use the SqlScheduleProvider symbol to locate the constructor and
replace the ILogger parameter usage with the resolved dependency.
In `@Workers/Support/Quidjibo.SqlServer/Providers/SqlWorkProvider.cs`:
- Around line 39-55: The SqlWorkProvider constructor currently adds an ILogger
dependency that conflicts with the repository’s dependency pattern. Remove the
constructor-injected logger from SqlWorkProvider unless it is actually used, and
if logging is needed later, follow the existing repository approach by resolving
dependencies through Bootstrapper.GetKernel().Resolve<T>() and using
Resgrid.Framework.Logging static methods instead of constructor injection.
In `@Workers/Support/Quidjibo.SqlServer/Utils/SqlLoader.cs`:
- Line 10: The static Scripts cache in SqlLoader should be declared as readonly
to match the coding guideline for immutable references. Update the Scripts field
in SqlLoader so the ConcurrentDictionary reference cannot be reassigned, keeping
the existing cache behavior intact while making the intent explicit.
In `@Workers/Support/Quidjibo.SqlServer/Utils/SqlRunner.cs`:
- Around line 20-43: Remove the unnecessary inner braces inside the SqlRunner
execution flow so the code stays flat within the existing using scopes. In the
method that opens the connection and handles the transaction path, keep the
await conn.OpenAsync and the inTransaction logic, but eliminate the redundant {
... } block around the transaction handling since it adds nesting without
changing behavior.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 417d3371-2a15-4a52-a926-bab9fa4e3417
📒 Files selected for processing (35)
Resgrid.slnWorkers/Resgrid.Workers.Console/Resgrid.Workers.Console.csprojWorkers/Support/Quidjibo.SqlServer/Configurations/SqlServerQuidjiboConfiguration.csWorkers/Support/Quidjibo.SqlServer/Extensions/QuidjiboBuilderExtensions.csWorkers/Support/Quidjibo.SqlServer/Extensions/SqlCommandExtensions.csWorkers/Support/Quidjibo.SqlServer/Extensions/SqlParameterCollectionExtensions.csWorkers/Support/Quidjibo.SqlServer/Factories/SqlProgressProviderFactory.csWorkers/Support/Quidjibo.SqlServer/Factories/SqlScheduleProviderFactory.csWorkers/Support/Quidjibo.SqlServer/Factories/SqlWorkProviderFactory.csWorkers/Support/Quidjibo.SqlServer/Providers/SqlProgressProvider.csWorkers/Support/Quidjibo.SqlServer/Providers/SqlScheduleProvider.csWorkers/Support/Quidjibo.SqlServer/Providers/SqlWorkProvider.csWorkers/Support/Quidjibo.SqlServer/Quidjibo.SqlServer.csprojWorkers/Support/Quidjibo.SqlServer/Scripts/Progress/Create.sqlWorkers/Support/Quidjibo.SqlServer/Scripts/Progress/LoadByCorrelationId.sqlWorkers/Support/Quidjibo.SqlServer/Scripts/Progress/Setup.sqlWorkers/Support/Quidjibo.SqlServer/Scripts/Schedule/Complete.sqlWorkers/Support/Quidjibo.SqlServer/Scripts/Schedule/Create.sqlWorkers/Support/Quidjibo.SqlServer/Scripts/Schedule/Delete.sqlWorkers/Support/Quidjibo.SqlServer/Scripts/Schedule/Exists.sqlWorkers/Support/Quidjibo.SqlServer/Scripts/Schedule/List.sqlWorkers/Support/Quidjibo.SqlServer/Scripts/Schedule/Load.sqlWorkers/Support/Quidjibo.SqlServer/Scripts/Schedule/LoadByName.sqlWorkers/Support/Quidjibo.SqlServer/Scripts/Schedule/Receive.sqlWorkers/Support/Quidjibo.SqlServer/Scripts/Schedule/Setup.sqlWorkers/Support/Quidjibo.SqlServer/Scripts/Schema/Setup.sqlWorkers/Support/Quidjibo.SqlServer/Scripts/Work/Complete.sqlWorkers/Support/Quidjibo.SqlServer/Scripts/Work/Fault.sqlWorkers/Support/Quidjibo.SqlServer/Scripts/Work/List.sqlWorkers/Support/Quidjibo.SqlServer/Scripts/Work/Receive.sqlWorkers/Support/Quidjibo.SqlServer/Scripts/Work/Renew.sqlWorkers/Support/Quidjibo.SqlServer/Scripts/Work/Send.sqlWorkers/Support/Quidjibo.SqlServer/Scripts/Work/Setup.sqlWorkers/Support/Quidjibo.SqlServer/Utils/SqlLoader.csWorkers/Support/Quidjibo.SqlServer/Utils/SqlRunner.cs
Comment on lines
+16
to
+20
| public static QuidjiboBuilder UseSqlServer(this QuidjiboBuilder builder, Action<SqlServerQuidjiboConfiguration> sqlServerQuidjiboConfiguration) | ||
| { | ||
| var config = new SqlServerQuidjiboConfiguration(); | ||
| sqlServerQuidjiboConfiguration(config); | ||
| return builder.UseSqlServer(config); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Fail fast on null configuration and blank connection strings.
These entry points pass configuration straight into the factories, so a missing delegate/config/connection string is only discovered later when the provider first opens SQL connections. Validate the public inputs here so startup fails deterministically.
Proposed fix
public static QuidjiboBuilder UseSqlServer(this QuidjiboBuilder builder, Action<SqlServerQuidjiboConfiguration> sqlServerQuidjiboConfiguration)
{
+ ArgumentNullException.ThrowIfNull(builder);
+ ArgumentNullException.ThrowIfNull(sqlServerQuidjiboConfiguration);
+
var config = new SqlServerQuidjiboConfiguration();
sqlServerQuidjiboConfiguration(config);
return builder.UseSqlServer(config);
}
@@
public static QuidjiboBuilder UseSqlServer(this QuidjiboBuilder builder, SqlServerQuidjiboConfiguration sqlServerQuidjiboConfiguration)
{
+ ArgumentNullException.ThrowIfNull(builder);
+ ArgumentNullException.ThrowIfNull(sqlServerQuidjiboConfiguration);
+ if (string.IsNullOrWhiteSpace(sqlServerQuidjiboConfiguration.ConnectionString))
+ throw new ArgumentException("ConnectionString is required.", nameof(sqlServerQuidjiboConfiguration));
+
return builder.Configure(sqlServerQuidjiboConfiguration)
.ConfigureWorkProviderFactory(new SqlWorkProviderFactory(builder.LoggerFactory, sqlServerQuidjiboConfiguration))
.ConfigureProgressProviderFactory(new SqlProgressProviderFactory(builder.LoggerFactory, sqlServerQuidjiboConfiguration.ConnectionString))
.ConfigureScheduleProviderFactory(new SqlScheduleProviderFactory(builder.LoggerFactory, sqlServerQuidjiboConfiguration.ConnectionString));
}
@@
public static QuidjiboBuilder UseSqlServer(this QuidjiboBuilder builder, string connectionString, params string[] queues)
{
+ ArgumentNullException.ThrowIfNull(builder);
+ if (string.IsNullOrWhiteSpace(connectionString))
+ throw new ArgumentException("Connection string is required.", nameof(connectionString));
+
if (queues == null || queues.Length == 0)
{
queues = Default.Queues;
}
@@
public static QuidjiboBuilder UseSqlServerForWork(this QuidjiboBuilder builder, SqlServerQuidjiboConfiguration sqlServerQuidjiboConfiguration)
{
+ ArgumentNullException.ThrowIfNull(builder);
+ ArgumentNullException.ThrowIfNull(sqlServerQuidjiboConfiguration);
+ if (string.IsNullOrWhiteSpace(sqlServerQuidjiboConfiguration.ConnectionString))
+ throw new ArgumentException("ConnectionString is required.", nameof(sqlServerQuidjiboConfiguration));
+
return builder.ConfigureWorkProviderFactory(new SqlWorkProviderFactory(builder.LoggerFactory, sqlServerQuidjiboConfiguration));
}
@@
public static QuidjiboBuilder UseSqlServerForProgress(this QuidjiboBuilder builder, string connectionString)
{
+ ArgumentNullException.ThrowIfNull(builder);
+ if (string.IsNullOrWhiteSpace(connectionString))
+ throw new ArgumentException("Connection string is required.", nameof(connectionString));
+
return builder.ConfigureProgressProviderFactory(new SqlProgressProviderFactory(builder.LoggerFactory, connectionString));
}
@@
public static QuidjiboBuilder UseSqlServerForSchedule(this QuidjiboBuilder builder, string connectionString)
{
+ ArgumentNullException.ThrowIfNull(builder);
+ if (string.IsNullOrWhiteSpace(connectionString))
+ throw new ArgumentException("Connection string is required.", nameof(connectionString));
+
return builder.ConfigureScheduleProviderFactory(new SqlScheduleProviderFactory(builder.LoggerFactory, connectionString));
}Also applies to: 29-35, 44-60, 68-71, 79-93
🤖 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 `@Workers/Support/Quidjibo.SqlServer/Extensions/QuidjiboBuilderExtensions.cs`
around lines 16 - 20, The Sql Server builder entry points are accepting
null/blank inputs and deferring failures until connection time; add eager
validation in the public extension methods so startup fails immediately. In
QuidjiboBuilderExtensions, validate the configuration delegate/config object
before using it, and check the connection string for null/whitespace in the
UseSqlServer overloads and any related setup methods referenced by the comment.
Use the existing symbols like UseSqlServer and SqlServerQuidjiboConfiguration to
locate all affected entry points and throw an argument validation failure before
passing values into the factories.
Comment on lines
+68
to
+71
| public static QuidjiboBuilder UseSqlServerForWork(this QuidjiboBuilder builder, SqlServerQuidjiboConfiguration sqlServerQuidjiboConfiguration) | ||
| { | ||
| return builder.ConfigureWorkProviderFactory(new SqlWorkProviderFactory(builder.LoggerFactory, sqlServerQuidjiboConfiguration)); | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
UseSqlServerForWork drops the caller's configuration.
Unlike the full UseSqlServer(...) overloads, this method only swaps the work-provider factory. The passed SqlServerQuidjiboConfiguration never gets applied to the builder, so builder-level settings from that object (PollingInterval, WorkPollingInterval, EnableWorker, SingleLoop, Throttle, etc.) are ignored.
Proposed fix
public static QuidjiboBuilder UseSqlServerForWork(this QuidjiboBuilder builder, SqlServerQuidjiboConfiguration sqlServerQuidjiboConfiguration)
{
- return builder.ConfigureWorkProviderFactory(new SqlWorkProviderFactory(builder.LoggerFactory, sqlServerQuidjiboConfiguration));
+ return builder.Configure(sqlServerQuidjiboConfiguration)
+ .ConfigureWorkProviderFactory(new SqlWorkProviderFactory(builder.LoggerFactory, sqlServerQuidjiboConfiguration));
}📝 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.
Suggested change
| public static QuidjiboBuilder UseSqlServerForWork(this QuidjiboBuilder builder, SqlServerQuidjiboConfiguration sqlServerQuidjiboConfiguration) | |
| { | |
| return builder.ConfigureWorkProviderFactory(new SqlWorkProviderFactory(builder.LoggerFactory, sqlServerQuidjiboConfiguration)); | |
| } | |
| public static QuidjiboBuilder UseSqlServerForWork(this QuidjiboBuilder builder, SqlServerQuidjiboConfiguration sqlServerQuidjiboConfiguration) | |
| { | |
| return builder.Configure(sqlServerQuidjiboConfiguration) | |
| .ConfigureWorkProviderFactory(new SqlWorkProviderFactory(builder.LoggerFactory, sqlServerQuidjiboConfiguration)); | |
| } |
🤖 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 `@Workers/Support/Quidjibo.SqlServer/Extensions/QuidjiboBuilderExtensions.cs`
around lines 68 - 71, The UseSqlServerForWork method is only replacing the work
provider factory and never applies the provided SqlServerQuidjiboConfiguration
to the QuidjiboBuilder, so caller-supplied builder settings are ignored. Update
UseSqlServerForWork in QuidjiboBuilderExtensions to also bind/apply the passed
configuration the same way the full UseSqlServer overloads do, while still
configuring SqlWorkProviderFactory, so settings like PollingInterval,
WorkPollingInterval, EnableWorker, SingleLoop, and Throttle are honored.
Comment on lines
+39
to
+67
| public static async Task PrepareForRenewAsync(this SqlCommand cmd, WorkItem item, DateTime lockExpireOn, CancellationToken cancellationToken) | ||
| { | ||
| #pragma warning disable CA2100 // Review SQL queries for security vulnerabilities | ||
| cmd.CommandText = await SqlLoader.GetScript("Work.Renew"); | ||
| #pragma warning restore CA2100 // Review SQL queries for security vulnerabilities | ||
| cmd.AddParameter("@Id", item.Id); | ||
| cmd.AddParameter("@VisibleOn", lockExpireOn); | ||
| } | ||
|
|
||
| public static async Task PrepareForCompleteAsync(this SqlCommand cmd, WorkItem item, CancellationToken cancellationToken) | ||
| { | ||
| #pragma warning disable CA2100 // Review SQL queries for security vulnerabilities | ||
| cmd.CommandText = await SqlLoader.GetScript("Work.Complete"); | ||
| #pragma warning restore CA2100 // Review SQL queries for security vulnerabilities | ||
| cmd.AddParameter("@Id", item.Id); | ||
| cmd.AddParameter("@Complete", SqlWorkProvider.StatusFlags.Complete); | ||
| } | ||
|
|
||
| public static async Task PrepareForFaultAsync(this SqlCommand cmd, WorkItem item, int visibilityTimeout, CancellationToken cancellationToken) | ||
| { | ||
| var faultedOn = DateTime.UtcNow; | ||
|
|
||
| #pragma warning disable CA2100 // Review SQL queries for security vulnerabilities | ||
| cmd.CommandText = await SqlLoader.GetScript("Work.Fault"); | ||
| #pragma warning restore CA2100 // Review SQL queries for security vulnerabilities | ||
| cmd.AddParameter("@Id", item.Id); | ||
| cmd.AddParameter("@VisibleOn", faultedOn.AddSeconds(Math.Max(visibilityTimeout, 30))); | ||
| cmd.AddParameter("@Faulted", SqlWorkProvider.StatusFlags.Faulted); | ||
| } |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Include a lease owner/token in work state transitions.
Renew, complete, and fault currently prepare updates using only @Id. A stale worker can still update a job after its visibility timeout expires and another worker receives the same row. Pass a receiver-specific token/worker value into these commands and have the SQL WHERE clause enforce it.
🤖 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 `@Workers/Support/Quidjibo.SqlServer/Extensions/SqlCommandExtensions.cs` around
lines 39 - 67, The work transition helpers in SqlCommandExtensions currently
rely only on `@Id`, which allows stale workers to update rows after lease
transfer. Update PrepareForRenewAsync, PrepareForCompleteAsync, and
PrepareForFaultAsync to accept and add a lease owner/token or worker-specific
value, then ensure the corresponding SQL scripts use that value in the WHERE
clause so only the current owner can renew, complete, or fault a work item.
Comment on lines
+1
to
+4
| UPDATE wrk | ||
| SET [VisibleOn] = @VisibleOn | ||
| FROM [Quidjibo].[Work] wrk | ||
| WHERE [Id] = @Id No newline at end of file |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Guard renew with the current lease, not just Id.
This update can extend a work item that the caller no longer owns. PrepareForRenewAsync only sends @Id plus the new @VisibleOn, so a worker that renews after its lock expired can overwrite the lease acquired by another worker. Match on the previously observed lease value/status and treat a 0-row update as a failed renew.
🤖 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 `@Workers/Support/Quidjibo.SqlServer/Scripts/Work/Renew.sql` around lines 1 -
4, The renewal update currently matches only on `Id`, so a stale worker can
extend a work item it no longer owns. Update the `Renew.sql` logic used by
`PrepareForRenewAsync` to also filter by the current lease state/value that was
originally observed when the work item was locked, not just the identifier. Make
sure the caller passes that expected lease/status alongside `@Id` and
`@VisibleOn`, and treat an update affecting 0 rows as a failed renew so
ownership loss is detected.
Comment on lines
+6
to
+13
| [Name], | ||
| [Queue], | ||
| [Attempts], | ||
| [CreatedOn], | ||
| [ExpireOn], | ||
| [VisibleOn], | ||
| [Status], | ||
| [Payload] |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Persist the bound @Worker value or stop binding it.
PrepareForSendAsync adds @Worker, but this INSERT omits [Worker], so any supplied WorkItem.Worker is dropped while the parameter remains unused.
Proposed fix
[CorrelationId],
[Name],
+ [Worker],
[Queue],
@@
`@CorrelationId`,
`@Name`,
+ `@Worker`,
`@Queue`,Also applies to: 20-27
🤖 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 `@Workers/Support/Quidjibo.SqlServer/Scripts/Work/Send.sql` around lines 6 -
13, The INSERT in Send.sql is binding `@Worker` in PrepareForSendAsync but not
persisting it, so either add the Worker column to the INSERT/value list or
remove the parameter binding entirely. Update the Send script to keep the column
list, parameter list, and WorkItem.Worker mapping in sync, and check the related
insert block that also applies to the other referenced range.
Comment on lines
+26
to
+37
| try | ||
| { | ||
| cmd.Transaction = tran; | ||
| await func(cmd); | ||
| tran.Commit(); | ||
| } | ||
| catch | ||
| { | ||
| tran.Rollback(); | ||
| throw; | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Missing logging on transaction rollback path.
Per coding guidelines, use Resgrid.Framework.Logging.LogException when catching exceptions. The catch block rolls back but does not log the failure, making production debugging difficult. As per coding guidelines, LogException() automatically captures caller file path, member name, and line number.
🛠️ Proposed fix
catch (Exception ex)
{
+ LogException(ex, "Transaction failed, rolling back");
tran.Rollback();
throw;
}📝 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.
Suggested change
| try | |
| { | |
| cmd.Transaction = tran; | |
| await func(cmd); | |
| tran.Commit(); | |
| } | |
| catch | |
| { | |
| tran.Rollback(); | |
| throw; | |
| } | |
| } | |
| try | |
| { | |
| cmd.Transaction = tran; | |
| await func(cmd); | |
| tran.Commit(); | |
| } | |
| catch (Exception ex) | |
| { | |
| LogException(ex, "Transaction failed, rolling back"); | |
| tran.Rollback(); | |
| throw; | |
| } | |
| } |
🤖 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 `@Workers/Support/Quidjibo.SqlServer/Utils/SqlRunner.cs` around lines 26 - 37,
The transaction rollback path in the SqlRunner command execution flow is
catching exceptions without logging them, so update the catch block around the
tran.Commit/tran.Rollback logic to call Resgrid.Framework.Logging.LogException
before rethrowing. Use the existing SqlRunner method that wraps func(cmd) and
transaction handling, and make sure the logged exception is the one being caught
so the rollback failure is recorded with caller context automatically.
Source: Coding guidelines
Code Review Could Not Complete
|
| Options | Enabled |
|---|---|
| Bug | ✅ |
| Performance | ✅ |
| Security | ✅ |
| Business Logic | ✅ |
Resgrid-Bot
reviewed
Jun 29, 2026
| _queues = queues; | ||
| _visibilityTimeout = visibilityTimeout; | ||
| _batchSize = batchSize; | ||
| _maxAttempts = 10; |
There was a problem hiding this comment.
Magic number violation where the literal 30 appears at lines 88 and 124 and _maxAttempts = 10 is hardcoded at line 52 in SqlWorkProvider.cs, also present in SqlCommandExtensions.cs:65 and SqlScheduleProvider.cs:47-53. Extract these literals into named constants to centralize their definition and localize future changes.
Kody rule violation: Replace magic numbers with named constants
private const int MinimumVisibilityTimeoutSeconds = 30;
private const int DefaultMaxAttempts = 10;
// ...
_maxAttempts = DefaultMaxAttempts;
// ...
cmd.AddParameter("@VisibleOn", receiveOn.AddSeconds(Math.Max(_visibilityTimeout, MinimumVisibilityTimeoutSeconds)));
// ...
var lockExpireOn = (item.VisibleOn ?? DateTime.UtcNow).AddSeconds(Math.Max(_visibilityTimeout, MinimumVisibilityTimeoutSeconds));Prompt for LLM
File Workers/Support/Quidjibo.SqlServer/Providers/SqlWorkProvider.cs:
Line 52:
Violates rule 'Replace magic numbers with named constants': the minimum visibility timeout literal `30` appears at lines 88 and 124 (`Math.Max(_visibilityTimeout, 30)`) and `_maxAttempts = 10` is hardcoded at line 52 instead of deriving from configuration. These meaningful literals should be extracted to named constants so the values are defined once and any future change is localized.
Suggested Code:
private const int MinimumVisibilityTimeoutSeconds = 30;
private const int DefaultMaxAttempts = 10;
// ...
_maxAttempts = DefaultMaxAttempts;
// ...
cmd.AddParameter("@VisibleOn", receiveOn.AddSeconds(Math.Max(_visibilityTimeout, MinimumVisibilityTimeoutSeconds)));
// ...
var lockExpireOn = (item.VisibleOn ?? DateTime.UtcNow).AddSeconds(Math.Max(_visibilityTimeout, MinimumVisibilityTimeoutSeconds));
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
Member
Author
|
Approve |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pull Request Description
This PR fixes a SQL Server compatibility issue with the Resgrid Workers that occurs on .NET 8+ (the project now targets .NET 9.0).
Problem
The external
Quidjibo.SqlServerNuGet package (v0.6.0) uses the legacySystem.Data.SqlClient, which throws aSqlGuidCaster TypeLoadExceptionon .NET 8 and above, breaking the worker's ability to use SQL Server as its background job store.Changes
Vendored the Quidjibo.SqlServer provider locally: Replaced the NuGet package reference with a new in-repository
Quidjibo.SqlServerproject that usesMicrosoft.Data.SqlClientinstead. This project includes:Fixed semaphore lock patterns in the Postgres provider factories: Moved
SyncLock.WaitAsync()calls outside of theirtryblocks in the Progress, Schedule, and Work provider factories. This prevents a scenario where a cancellation or exception duringWaitAsyncwould cause thefinallyblock to incorrectly callRelease(), leading to aSemaphoreFullException.