VPR-54 feat(scheduler): Hangfire-backed scheduler with pause/resume API#182
Open
VPR-54 feat(scheduler): Hangfire-backed scheduler with pause/resume API#182
Conversation
Bundle ReportBundle size has no change ✅ |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #182 +/- ##
==========================================
+ Coverage 42.95% 43.33% +0.38%
==========================================
Files 876 897 +21
Lines 51454 52383 +929
Branches 4802 4875 +73
==========================================
+ Hits 22101 22700 +599
- Misses 28829 29141 +312
- Partials 524 542 +18
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Contributor
Author
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughThis PR implements a Hangfire-based scheduler: discovery of IScheduledJob types via attribute, runtime dispatcher, persisted pause markers with optimistic concurrency, pause/resume operator API, startup/hourly reconciler to heal split-brain, Hangfire wiring (conditional on config), health checks, RAPS role-refresh job, DB mapping, UI link, tests, and docs. ChangesHangfire Scheduler Feature
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 23
🤖 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 `@test/Scheduler/JobsControllerTests.cs`:
- Around line 223-235: The test
ResumeJob_Returns404WithMarkerNotFoundCodeWhenMissing currently only asserts the
response type; update it to also assert the NotFound payload contains the
marker_not_found code by casting result.Result to NotFoundObjectResult and
verifying the response value (from _sut.ResumeJob with
JobsController.ResumeRequest) has a code or error field equal to
"marker_not_found" (keeping the existing SchedulerJobNotFoundException setup);
ensure the assertion checks the same shape the controller returns so the test
fails if the marker code is missing.
In `@test/Viper.test.csproj`:
- Line 37: Update the PackageReference for Hangfire.Core in the test project
from Version="1.8.21" to Version="1.8.23" so it matches the production/web
project; locate the PackageReference element for "Hangfire.Core" in
Viper.test.csproj and change the Version attribute to "1.8.23", then
restore/update NuGet packages (dotnet restore or equivalent) to ensure the
lock/assets are regenerated.
In `@web/appsettings.json`:
- Around line 39-43: Remove the standalone // comment block between top-level
JSON properties that breaks Biome: delete the three lines of // commentary so
the "Hangfire" object is immediately contiguous with other properties, and if
documentation is needed move it to the existing .env.local.example or convert it
into an inline trailing comment on the "Enabled" property (e.g., a single-line
comment after "Enabled") rather than a separate JSON line; target the "Hangfire"
object and its "Enabled" property when making this change.
In `@web/Areas/RAPS/Services/RAPSAuditService.cs`:
- Around line 256-261: The ModBy fallback in AuditRoleMemberChange currently
only handles null; change the logic so empty or whitespace modBy values are
treated like null by using string.IsNullOrWhiteSpace(modBy) when assigning
TblLog.ModBy, falling back to UserHelper.GetCurrentUser()?.LoginId (optionally
trimmed) so audit entries never record blank actors; update the assignment in
AuditRoleMemberChange (and any similar assignments for TblLog.ModBy in this
class) to use this check.
In `@web/Areas/RAPS/Services/RoleViews.cs`:
- Around line 42-54: Both UpdateRoles and UpdateRole duplicate the
actor-resolution logic; create a single helper like ResolveActor(string? modBy)
=> string.IsNullOrEmpty(modBy) ? DefaultModBy : modBy and use it from both
UpdateRoles and UpdateRole so the fallback rule is centralized; update both
methods to call ResolveActor(modBy) (or make it static/private as appropriate)
and remove the duplicated conditional logic in each method.
In `@web/Areas/Scheduler/Controllers/JobsController.cs`:
- Around line 89-92: In JobsController catch blocks that handle
SchedulerConcurrencyException (e.g., in PauseJob and ResumeJob), add a warning
log call before returning Conflict to surface the failure to telemetry/logs; log
contextual details such as jobId and user (or requestor) and the exception
message so operators can correlate the 409 response with the warning; use the
controller's ILogger instance (e.g., _logger) and include a clear message like
"Concurrency conflict while attempting to pause/resume job {jobId} for user
{userId}" followed by the exception/info, then return Conflict(new { error =
ConcurrencyErrorCode }).
- Line 19: The ApiController base class lacks the [ApiController] attribute so
derived controllers like JobsController won't get ApiController behavior; open
the ApiController class (the declaration "public class ApiController :
ControllerBase") and add the [ApiController] attribute directly above that class
declaration so the attribute is applied to the base class used by
JobsController.
In `@web/Areas/Scheduler/Models/Entities/SchedulerJobState.cs`:
- Around line 28-29: The XML doc for the PausedAt property incorrectly says
"UTC" while project guidelines use local time for DB timestamps; update the
summary on the SchedulerJobState.PausedAt property to indicate it is a local
timestamp (and ensure code that sets it uses DateTime.Now/DateTimeKind.Local or
DateTimeOffset local semantics). If there is a true spec dependency requiring
UTC (e.g., reconciliation with Hangfire), instead leave UTC and add a clear
inline comment on SchedulerJobState.PausedAt explaining the exception and why
UtcNow is required.
In `@web/Areas/Scheduler/Models/ScheduledJobContext.cs`:
- Around line 11-15: The ScheduledJobContext constructor currently accepts a
null/blank modBy which breaks downstream audit invariants; update the
ScheduledJobContext(TriggerSource triggerSource, string modBy) constructor to
validate modBy (e.g., check for null or whitespace), throw ArgumentNullException
or ArgumentException with a clear message when invalid, and only assign ModBy
when valid (optionally trim the value before assignment) so invalid contexts
cannot be created.
In `@web/Areas/Scheduler/README.md`:
- Around line 172-174: The fenced code block using backticks containing the
scheduler reconciler message lacks a language specifier; update the fence
starting marker (```) to include a language tag such as "text" (e.g., change ```
to ```text) so syntax highlighters/renderers treat the content as plain
text—modify the fenced block around the line that begins "Scheduler reconciler
pass: split-brain healed=N, system markers deleted=N, paused ok=N, active ok=N,
lost registrations healed=N, markers=N, registrations=N".
- Around line 23-41: The README example constructor for
RapsRoleRefreshScheduledJob is missing the ILogger dependency; update the
snippet to include an ILogger<RapsRoleRefreshScheduledJob> parameter in the
constructor signature, add a private readonly field (e.g., _logger) to the class
and assign the injected logger in the constructor (keeping the existing
RAPSContext _rapsContext assignment), and ensure the example includes the using
for Microsoft.Extensions.Logging so the DI signature matches the real
implementation and won't fail at runtime when constructing
RapsRoleRefreshScheduledJob.
In `@web/Areas/Scheduler/Services/ScheduledJobDiscovery.cs`:
- Around line 111-125: The ResolveTimeZone method currently swallows
TimeZoneNotFoundException and InvalidTimeZoneException and silently returns
TimeZoneInfo.Utc; update ResolveTimeZone in ScheduledJobDiscovery to log a
warning (including the supplied id and exception message/stack) when a lookup
fails so ops can detect misconfigured timezones, then continue to return
TimeZoneInfo.Utc; use the existing logger on the class (or inject
ILogger<ScheduledJobDiscovery> if none exists) and include the TimeZoneId and
exception details in the warning log entry.
In `@web/Areas/Scheduler/Services/SchedulerJobReconciler.cs`:
- Around line 71-92: ResolvePacific currently returns TimeZoneInfo.Utc silently
when neither "Pacific Standard Time" nor "America/Los_Angeles" resolve; change
ResolvePacific to log a warning right before returning UTC so operators see the
fallback. Update ResolvePacific to accept or reference the reconciler's ILogger
(or use the existing logger field) and call logger.LogWarning with a message
that includes the attempted ids and that UTC is being used as a fallback (e.g.,
"Neither 'Pacific Standard Time' nor 'America/Los_Angeles' resolved; falling
back to UTC"). Preserve the existing exception handling and still return
TimeZoneInfo.Utc after logging.
- Around line 113-126: StartAsync currently awaits
_reconciler.RunOnceAsync(cancellationToken) which blocks host startup; change it
to launch the reconciler pass in the background instead: kick off a non-blocking
Task (e.g., use Task.Run or Task.Factory.StartNew with the cancellationToken)
that calls _reconciler.RunOnceAsync and catches/logs exceptions via
_logger.LogError(ex, "...") so failures are recorded but do not delay startup;
ensure the fire-and-forget Task is started (optionally await Task.Yield() first)
and that the cancellationToken is honored inside the background delegate.
In `@web/Areas/Scheduler/Services/SchedulerJobsService.cs`:
- Line 134: Change the timestamp used for DB audit fields from UTC to local
time: replace uses of DateTime.UtcNow assigned to the PausedAt property in
SchedulerJobsService (and the other occurrence noted) with DateTime.Now so the
persisted value uses DateTimeKind.Local; update both assignment sites where
PausedAt is set to ensure DB-stored timestamps follow the project's guideline to
default to local time.
- Around line 324-338: Merge the duplicate methods ResolveDeclaredTimeZone and
ResolveTimeZone by keeping one implementation (rename to ResolveTimeZoneSafe or
keep ResolveTimeZone) that returns TimeZoneInfo.FindSystemTimeZoneById(id)
inside try/catch and falls back to TimeZoneInfo.Utc on TimeZoneNotFoundException
or InvalidTimeZoneException, then remove the other method and update all
call-sites that reference ResolveDeclaredTimeZone or ResolveTimeZone to call the
single surviving method name.
- Around line 55-62: Replace the manual foreach/filter/add with a LINQ pipeline:
filter the markers using .Where(m => !seenIds.Contains(m.RecurringJobId)) and
then map with .Select(m => BuildDtoFromMarker(m)), and add the resulting
sequence into results (use results.AddRange or equivalent); refer to the local
symbols markers, seenIds, BuildDtoFromMarker, and results to locate and
implement the change.
- Around line 160-175: The broad catch(Exception) around
_recurringJobManager.RemoveIfExists(id) must be replaced with explicit catches
for the known Hangfire/storage failure types (e.g.,
DistributedLockTimeoutException, BackgroundJobClientException and any observed
SqlException) so unexpected exceptions can propagate; catch those specific
exception types, set deregistrationPending = true and call _logger.LogError(...)
using LogSanitizer.SanitizeId(id) in each specific catch, and remove the generic
catch so service-level logic does not swallow unrelated failures.
- Line 122: hangfireJob.Job can be null so calling
SerializeJobPayload(hangfireJob.Job) causes an NRE when SerializeJobPayload
dereferences job.Method; change the call site in SchedulerJobsService to check
hangfireJob.Job for null and only call SerializeJobPayload when non-null (e.g.,
set payload to null or an appropriate placeholder when hangfireJob.Job is null),
or update SerializeJobPayload to accept a nullable parameter and safely handle a
null job (avoid dereferencing job.Method). Ensure you reference
SerializeJobPayload and hangfireJob.Job when applying the fix.
In `@web/Classes/Scheduler/HangfireDashboardAuthorizationFilter.cs`:
- Around line 41-43: Guard against a null/empty identity name before calling
userHelper.GetByLoginId: check user?.Identity?.Name (or equivalent) and if it's
null or empty return false early; then call userHelper.GetByLoginId(aaudContext,
name) and proceed to evaluate aaudUser != null &&
userHelper.HasPermission(rapsContext, aaudUser, SchedulerPermission). This
prevents passing a null name into GetByLoginId and avoids incorrect/exceptional
behavior.
In `@web/Classes/Scheduler/HangfireExtensions.cs`:
- Around line 67-69: The scheduled-job discovery currently calls
ScheduledJobDiscovery.RegisterScheduledJobs(services, new[] {
Assembly.GetExecutingAssembly() }) which only scans the executing assembly;
change the call so discovery can see sibling/test assemblies—either replace
Assembly.GetExecutingAssembly() with a filtered
AppDomain.CurrentDomain.GetAssemblies() (e.g., filter by your project's
namespace prefix) or update the RegisterScheduledJobs API to accept an
Assembly[] parameter and pass in a merged list
(AppDomain.CurrentDomain.GetAssemblies().Where(...)). Ensure you reference
ScheduledJobDiscovery.RegisterScheduledJobs and the Assembly selection logic so
future assemblies with [ScheduledJob] are discoverable.
In `@web/Classes/Scheduler/SchedulerSchemaInitializer.cs`:
- Around line 52-58: The catch block in SchedulerSchemaInitializer that
currently reads "catch (Exception ex)" should be narrowed to catch SqlException
only: change the exception type in the catch to SqlException (e.g., catch
(SqlException ex)), keep the existing logger.Error(ex, "...") call, and add the
appropriate using/import for the SQL client library used in the project
(System.Data.SqlClient or Microsoft.Data.SqlClient) so the type resolves; do not
alter the logging text or behavior beyond changing the caught exception type.
In `@web/Viper.csproj`:
- Around line 52-53: Update the Hangfire package references in Viper.csproj:
change the Version for both PackageReference entries "Hangfire.AspNetCore" and
"Hangfire.SqlServer" from 1.8.21 to 1.8.23 to pull in fixes for the dashboard
CSS map and the endpoint pipeline InvalidOperationException; ensure the csproj
now uses Version="1.8.23" for both package references and run a restore/build to
verify no compatibility issues.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: c549a843-f260-47eb-97ac-c0f6ff16a579
📒 Files selected for processing (43)
.env.local.exampletest/HealthChecks/HangfireHealthCheckTests.cstest/RAPS/RapsRoleRefreshScheduledJobTests.cstest/Scheduler/HangfireJobLoggingFilterTests.cstest/Scheduler/JobsControllerTests.cstest/Scheduler/ScheduledJobDiscoveryTests.cstest/Scheduler/ScheduledJobRunnerTests.cstest/Scheduler/SchedulerJobsServiceTests.cstest/Viper.test.csprojweb/Areas/RAPS/Jobs/RapsRoleRefreshScheduledJob.csweb/Areas/RAPS/Services/RAPSAuditService.csweb/Areas/RAPS/Services/RoleViews.csweb/Areas/Scheduler/Controllers/.gitkeepweb/Areas/Scheduler/Controllers/JobsController.csweb/Areas/Scheduler/Models/DTOs/Responses/PauseResumeResultDto.csweb/Areas/Scheduler/Models/DTOs/Responses/ReconcilerOutcomeDto.csweb/Areas/Scheduler/Models/DTOs/Responses/SchedulerJobDto.csweb/Areas/Scheduler/Models/Entities/SchedulerJobState.csweb/Areas/Scheduler/Models/ScheduledJobContext.csweb/Areas/Scheduler/Models/TriggerSource.csweb/Areas/Scheduler/README.mdweb/Areas/Scheduler/Services/.gitkeepweb/Areas/Scheduler/Services/IScheduledJob.csweb/Areas/Scheduler/Services/ISchedulerJobsService.csweb/Areas/Scheduler/Services/ScheduledJobAttribute.csweb/Areas/Scheduler/Services/ScheduledJobDiscovery.csweb/Areas/Scheduler/Services/ScheduledJobRegistry.csweb/Areas/Scheduler/Services/ScheduledJobRunner.csweb/Areas/Scheduler/Services/SchedulerExceptions.csweb/Areas/Scheduler/Services/SchedulerJobReconciler.csweb/Areas/Scheduler/Services/SchedulerJobsService.csweb/Classes/ForwardedHeadersExtensions.csweb/Classes/HealthChecks/HangfireHealthCheck.csweb/Classes/SQLContext/SchedulerContext.csweb/Classes/SQLContext/VIPERContext.csweb/Classes/Scheduler/HangfireDashboardAuthorizationFilter.csweb/Classes/Scheduler/HangfireExtensions.csweb/Classes/Scheduler/HangfireJobLoggingFilter.csweb/Classes/Scheduler/SchedulerSchemaInitializer.csweb/Program.csweb/Views/Shared/Components/MainNav/MainNav.csweb/Viper.csprojweb/appsettings.json
There was a problem hiding this comment.
Pull request overview
Introduces a new Hangfire-backed background scheduler subsystem (gated by Hangfire:Enabled) with secured dashboard access, operator pause/resume APIs, reconciliation logic, health checks, and an initial scheduled consumer for nightly RAPS role refresh.
Changes:
- Add Hangfire + SQL Server storage integration, with dashboard endpoint and per-job logging/telemetry.
- Implement scheduler domain model:
[ScheduledJob]discovery/registration, pause/resume marker table, and reconciler (startup + hourly). - Add scheduler admin API (
/api/scheduler/jobs) and first scheduled job (raps:role-refresh), threading explicit audit actor (modBy) through RAPS role updates.
Reviewed changes
Copilot reviewed 41 out of 43 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| web/Viper.csproj | Adds Hangfire package references and Scheduler area folders. |
| web/Views/Shared/Components/MainNav/MainNav.cs | Adds “Scheduler” nav link to the Hangfire dashboard and updates title mapping. |
| web/Program.cs | Wires forwarded-headers via extension and hooks Hangfire DI + middleware behind Hangfire:Enabled. |
| web/Classes/SQLContext/VIPERContext.cs | Adds scheduler model creation hook to EF context. |
| web/Classes/SQLContext/SchedulerContext.cs | Adds EF DbSet + mapping for [HangFire].[SchedulerJobState]. |
| web/Classes/Scheduler/SchedulerSchemaInitializer.cs | Adds idempotent DDL initializer for the scheduler marker table. |
| web/Classes/Scheduler/HangfireJobLoggingFilter.cs | Adds Hangfire server filter for structured logging scopes and sanitized job start/finish/error logs. |
| web/Classes/Scheduler/HangfireExtensions.cs | Adds Hangfire DI registration, dashboard mount, health check registration, and scheduled job registration bootstrap. |
| web/Classes/Scheduler/HangfireDashboardAuthorizationFilter.cs | Adds RAPS-permission authorization for the Hangfire dashboard. |
| web/Classes/HealthChecks/HangfireHealthCheck.cs | Adds Hangfire readiness health check (storage + server heartbeat freshness). |
| web/Classes/ForwardedHeadersExtensions.cs | Moves Cloudflare/F5 forwarded-headers configuration out of Program.cs. |
| web/Areas/Scheduler/Services/SchedulerJobsService.cs | Implements list/get/pause/resume/reconcile logic coordinating Hangfire + marker table. |
| web/Areas/Scheduler/Services/SchedulerJobReconciler.cs | Adds reconciler driver + recurring registration + startup hosted service pass. |
| web/Areas/Scheduler/Services/SchedulerExceptions.cs | Adds typed exceptions for controller translation (403/404/409). |
| web/Areas/Scheduler/Services/ScheduledJobRunner.cs | Adds DI-based dispatcher to execute scheduled jobs via a concrete target for Hangfire. |
| web/Areas/Scheduler/Services/ScheduledJobRegistry.cs | Adds metadata registry abstraction for declared scheduled jobs. |
| web/Areas/Scheduler/Services/ScheduledJobDiscovery.cs | Adds reflection-based discovery/validation/DI registration for [ScheduledJob] implementations. |
| web/Areas/Scheduler/Services/ScheduledJobAttribute.cs | Adds attribute contract for recurring job id/cron/timezone/system-flag. |
| web/Areas/Scheduler/Services/ISchedulerJobsService.cs | Defines scheduler operator service API + reserved id namespace constants. |
| web/Areas/Scheduler/Services/IScheduledJob.cs | Defines scheduled job execution contract with ScheduledJobContext. |
| web/Areas/Scheduler/Services/.gitkeep | Keeps the services folder tracked. |
| web/Areas/Scheduler/README.md | Adds scheduler onboarding/configuration and ops runbook documentation. |
| web/Areas/Scheduler/Models/TriggerSource.cs | Adds enum to distinguish scheduled vs manual runs. |
| web/Areas/Scheduler/Models/ScheduledJobContext.cs | Adds per-run context including trigger source and audit actor (ModBy). |
| web/Areas/Scheduler/Models/Entities/SchedulerJobState.cs | Adds marker entity representing paused job definition + rowversion. |
| web/Areas/Scheduler/Models/DTOs/Responses/SchedulerJobDto.cs | Adds operator-facing job DTO combining Hangfire + paused marker state. |
| web/Areas/Scheduler/Models/DTOs/Responses/ReconcilerOutcomeDto.cs | Adds reconciler outcome counters DTO. |
| web/Areas/Scheduler/Models/DTOs/Responses/PauseResumeResultDto.cs | Adds pause/resume result DTO with RowVersion and pending deregistration flag. |
| web/Areas/Scheduler/Controllers/JobsController.cs | Adds secured scheduler operator API endpoints (list/get/pause/resume). |
| web/Areas/Scheduler/Controllers/.gitkeep | Keeps the controllers folder tracked. |
| web/Areas/RAPS/Services/RoleViews.cs | Threads explicit modBy through role refresh path and preserves legacy default actor. |
| web/Areas/RAPS/Services/RAPSAuditService.cs | Allows explicit modBy override for audit rows (fallbacks to current CAS user). |
| web/Areas/RAPS/Jobs/RapsRoleRefreshScheduledJob.cs | Adds nightly scheduled job wrapping RAPS role refresh with scheduler audit actor. |
| web/appsettings.json | Adds Hangfire:Enabled (default false) configuration block. |
| test/Viper.test.csproj | Adds Hangfire.Core test dependency. |
| test/Scheduler/SchedulerJobsServiceTests.cs | Adds unit tests covering list/pause/resume/reconcile service behavior. |
| test/Scheduler/ScheduledJobRunnerTests.cs | Adds tests for runner job resolution and context stamping. |
| test/Scheduler/ScheduledJobDiscoveryTests.cs | Adds tests validating discovery and system-prefix invariants. |
| test/Scheduler/JobsControllerTests.cs | Adds controller tests for status codes and error payloads. |
| test/Scheduler/HangfireJobLoggingFilterTests.cs | Adds tests asserting logging, scopes, and error/completion behavior. |
| test/RAPS/RapsRoleRefreshScheduledJobTests.cs | Adds tests verifying scheduled job attribute metadata for RAPS job. |
| test/HealthChecks/HangfireHealthCheckTests.cs | Adds tests for healthy/degraded/unhealthy Hangfire health outcomes. |
| .env.local.example | Documents the optional local env var toggle for Hangfire enablement. |
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (1)
web/appsettings.json (1)
39-43:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winBiome still rejects
//comments inside the JSON object — same issue as before.Moving the comments inside
"Hangfire"instead of between top-level keys didn't fix the parse errors; Biome treats the whole file as strict JSON. Consolidate to a single trailing comment on"Enabled"(JSONC-style inline comments are fine in this config reader).🛠️ Proposed fix
"Hangfire": { - // Disabled by default; each environment opts in via Hangfire:Enabled. - // Hangfire and the marker table share ConnectionStrings:VIPER. - "Enabled": false + "Enabled": false // Disabled by default; enable per env. Shares ConnectionStrings:VIPER. }🤖 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 `@web/appsettings.json` around lines 39 - 43, The Hangfire section contains line-comment blocks that break the strict JSON parser; remove the standalone // comments between top-level keys and replace them with a single trailing inline comment on the "Enabled" property (inside the "Hangfire" object) using JSONC-style inline comment syntax so the config reader can parse it (update the "Hangfire" object that contains "Enabled" accordingly).
🤖 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 `@web/Areas/RAPS/Jobs/RapsRoleRefreshScheduledJob.cs`:
- Around line 30-34: RunAsync currently accepts ct but doesn't thread it into
the role refresh, so update the signature of RoleViews.UpdateRoles to accept a
CancellationToken and call it from RunAsync passing ct; likewise add
CancellationToken parameters to RoleViews.UpdateRole, GetRoleMembers, and
GetViewMembers and propagate ct through their call chain. Inside those methods,
pass the token into all EF Core async calls (e.g., .ToListAsync(ct),
FirstOrDefaultAsync(ct), etc.) across the ~30 queries in GetViewMembers, and
change synchronous SaveChanges() usages in AddRoleMember and DeleteRoleMember to
async SaveChangesAsync(ct) and make those methods async (propagate the token up
to callers). Ensure all call sites are updated to await the new async methods
and forward the CancellationToken.
In `@web/Areas/Scheduler/README.md`:
- Around line 188-190: The README incorrectly references a non-existent
ConnectionStrings:Hangfire override; either remove the "(and `:Hangfire` if
overridden)" text from the README to accurately state that Hangfire wiring only
reads ConnectionStrings:VIPER, or implement the actual override by updating
HangfireExtensions.AddViperHangfire and UseViperHangfire so they check for
ConnectionStrings:Hangfire (falling back to ConnectionStrings:VIPER) and use
that value for the HangFire schema login; pick one approach and make the README
and the methods (AddViperHangfire / UseViperHangfire) consistent.
In `@web/Areas/Scheduler/Services/ScheduledJobRegistry.cs`:
- Around line 9-30: ScheduledJobMetadata is an immutable value holder and should
be converted to a positional record to remove constructor/property boilerplate
and gain structural equality; replace the class declaration and its explicit
constructor/properties with a positional record declaration for
ScheduledJobMetadata taking (Type jobType, string id, string cron, string
timeZoneId, bool isSystem) and update any references to use the record's
autogenerated properties (JobType, Id, Cron, TimeZoneId, IsSystem).
In `@web/Areas/Scheduler/Services/SchedulerJobsService.cs`:
- Around line 193-265: ResumeJobAsync currently calls
_recurringJobManager.AddOrUpdate(id, ...) before persisting the marker removal
and only catches DbUpdateConcurrencyException, so non-concurrency
SaveChangesAsync failures leave Hangfire registered while the API returns an
error. Update ResumeJobAsync to either (A) mirror PauseJobAsync by catching
broader persistence failures (catch DbUpdateException and/or TimeoutException)
and log the error while returning PauseResumeResultDto with
DeregistrationPending = true, or (B) perform a compensating removal (call the
appropriate _recurringJobManager.RemoveIfExists or equivalent) if
SaveChangesAsync fails, then rethrow or return a success/failure DTO consistent
with the actual state; reference the ResumeJobAsync method, the
_recurringJobManager.AddOrUpdate call, and the _context.SaveChangesAsync call to
locate where to add the widened catch or compensation logic.
- Around line 193-222: Change ResumeJobAsync to accept a nullable row-version
and guard against null the same way PauseJobAsync does: update the parameter to
byte[]? expectedRowVersion (nullable) and before calling
marker.RowVersion.SequenceEqual(expectedRowVersion) add a check that treats null
as a concurrency mismatch (e.g. if (expectedRowVersion == null ||
!marker.RowVersion.SequenceEqual(expectedRowVersion)) throw new
SchedulerConcurrencyException(id);). Reference ResumeJobAsync and
marker.RowVersion.SequenceEqual(expectedRowVersion) and ensure callers are
adjusted to pass nullable byte[] where needed.
- Around line 267-309: ReconcileAsync mixes EF deletions
(_context.SchedulerJobStates.Remove(marker)) with Hangfire side-effects
(_recurringJobManager.RemoveIfExists), but SaveChangesAsync is only called after
the loop if SystemMarkersDeleted>0, so a failure in RemoveIfExists can prevent
persisting marker deletes; fix by either (A) first collect and persist all
system-marker removals with SaveChangesAsync before performing Hangfire
mutations, or (B) wrap calls to
_recurringJobManager.RemoveIfExists(marker.RecurringJobId) in a try/catch
modeled on the PauseJobAsync error handling (catch, log sanitized id and
continue) so a flaky Hangfire call won’t abort the loop; update ReconcileAsync
accordingly and keep behavior of outcome counters consistent.
In `@web/Classes/ForwardedHeadersExtensions.cs`:
- Line 67: The log call writes an external CIDR value raw; update the warning in
ForwardedHeadersExtensions (the logger.Warn call that currently logs "Skipping
invalid Cloudflare CIDR: {Cidr}" with variable cidr) to sanitize the cidr before
logging by passing it through the LogSanitizer (e.g., replace cidr with
LogSanitizer.Sanitize(cidr) or the project’s equivalent Sanitize method) so the
external input is not written raw to logs while preserving the existing
exception and message.
In `@web/Classes/Scheduler/HangfireJobLoggingFilter.cs`:
- Around line 44-53: The current code in HangfireJobLoggingFilter.cs constructs
and logs raw argument values (using context.BackgroundJob.Job.Args, Truncate,
LogSanitizer) which can leak secrets/PII; change it to log only metadata: build
an argument summary (e.g., argument count and a comma-separated list of argument
type names or simple type names) instead of the values, optionally sanitize type
names with LogSanitizer, and pass that summary into the _logger.LogInformation
call (replace uses of args/Truncate/LogSanitizer on values). Ensure you update
the string interpolation placeholders (JobArgs) to reflect the new metadata and
remove any logging of actual argument values.
---
Duplicate comments:
In `@web/appsettings.json`:
- Around line 39-43: The Hangfire section contains line-comment blocks that
break the strict JSON parser; remove the standalone // comments between
top-level keys and replace them with a single trailing inline comment on the
"Enabled" property (inside the "Hangfire" object) using JSONC-style inline
comment syntax so the config reader can parse it (update the "Hangfire" object
that contains "Enabled" accordingly).
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 7cccd462-5d95-4874-bc77-997d16f21ae2
📒 Files selected for processing (43)
.env.local.exampletest/HealthChecks/HangfireHealthCheckTests.cstest/RAPS/RapsRoleRefreshScheduledJobTests.cstest/Scheduler/HangfireJobLoggingFilterTests.cstest/Scheduler/JobsControllerTests.cstest/Scheduler/ScheduledJobDiscoveryTests.cstest/Scheduler/ScheduledJobRunnerTests.cstest/Scheduler/SchedulerJobsServiceTests.cstest/Viper.test.csprojweb/Areas/RAPS/Jobs/RapsRoleRefreshScheduledJob.csweb/Areas/RAPS/Services/RAPSAuditService.csweb/Areas/RAPS/Services/RoleViews.csweb/Areas/Scheduler/Controllers/.gitkeepweb/Areas/Scheduler/Controllers/JobsController.csweb/Areas/Scheduler/Models/DTOs/Responses/PauseResumeResultDto.csweb/Areas/Scheduler/Models/DTOs/Responses/ReconcilerOutcomeDto.csweb/Areas/Scheduler/Models/DTOs/Responses/SchedulerJobDto.csweb/Areas/Scheduler/Models/Entities/SchedulerJobState.csweb/Areas/Scheduler/Models/ScheduledJobContext.csweb/Areas/Scheduler/Models/TriggerSource.csweb/Areas/Scheduler/README.mdweb/Areas/Scheduler/Services/.gitkeepweb/Areas/Scheduler/Services/IScheduledJob.csweb/Areas/Scheduler/Services/ISchedulerJobsService.csweb/Areas/Scheduler/Services/ScheduledJobAttribute.csweb/Areas/Scheduler/Services/ScheduledJobDiscovery.csweb/Areas/Scheduler/Services/ScheduledJobRegistry.csweb/Areas/Scheduler/Services/ScheduledJobRunner.csweb/Areas/Scheduler/Services/SchedulerExceptions.csweb/Areas/Scheduler/Services/SchedulerJobReconciler.csweb/Areas/Scheduler/Services/SchedulerJobsService.csweb/Classes/ForwardedHeadersExtensions.csweb/Classes/HealthChecks/HangfireHealthCheck.csweb/Classes/SQLContext/SchedulerContext.csweb/Classes/SQLContext/VIPERContext.csweb/Classes/Scheduler/HangfireDashboardAuthorizationFilter.csweb/Classes/Scheduler/HangfireExtensions.csweb/Classes/Scheduler/HangfireJobLoggingFilter.csweb/Classes/Scheduler/SchedulerSchemaInitializer.csweb/Program.csweb/Views/Shared/Components/MainNav/MainNav.csweb/Viper.csprojweb/appsettings.json
- Gated by Hangfire:Enabled (default false); registration is a no-op when the flag is off. - Connection string falls back to ConnectionStrings:VIPER if ConnectionStrings:Hangfire is unset, so the schema can move later. - Registration wrapped in try/catch: failures log and disable Hangfire for the process rather than block app startup.
- Mount the dashboard at /scheduler/dashboard via MapHangfireDashboard, conditional on Hangfire:Enabled. - Reuse the existing SVMSecure.CATS.scheduledJobs RAPS permission so legacy-scheduler admins inherit access without a new grant. - RequireAuthorization() handles the unauth->CAS redirect; the custom filter handles the 403 for authed-but-unauthorized.
…am.cs - Move the Cloudflare + F5 trusted-proxy block into ForwardedHeadersExtensions.AddViperForwardedHeaders, mirroring the AddViperHealthChecks / AddViperHangfire pattern. - Drops Main$ below the CA1502 cyclomatic-complexity and CA1505 maintainability-index thresholds. No behavior change.
- Health check reports storage reachability and server heartbeat freshness; tagged "ready" so it surfaces on /health/detail. Only registered when Hangfire is enabled - IServerFilter opens a structured logging scope per job execution (jobId, recurringJobId, triggerSource) and sanitizes job arguments via LogSanitizer before logging start/complete/error
- New JobsController under SVMSecure.CATS.scheduledJobs gates list, pause, and resume; system jobs in the __scheduler: namespace are rejected with 403 before any write - SchedulerJobState marker table is the source of truth for "is this job paused?". Pause is marker-first with reconcilable deregistration (202 if Hangfire's RemoveIfExists throws); resume is registration- first; both are idempotent and RowVersion-guarded - Startup hosted service plus hourly __scheduler:reconcile job heal split-brain state, delete stray system-namespaced markers, and emit an outcome counter - Idempotent DDL initializer creates [HangFire].[SchedulerJobState] on startup since this project does not use EF migrations
…consumer - [ScheduledJob(id,cron,TimeZoneId,IsSystem)] attribute and IScheduledJob contract; ScheduledJobContext carries TriggerSource and effective ModBy to every run so jobs do not call UserHelper.GetCurrentUser() - Discovery scans the web assembly at startup, validates the __scheduler: prefix invariant against IsSystem, registers each job's type with DI and the runner that Hangfire dispatches against - ScheduledJobRunner resolves the IScheduledJob from a fresh DI scope at each execution, stamping ModBy with the scheduler actor - RoleViews.UpdateRoles takes an explicit modBy parameter and threads it through TblRoleMember writes and TblLog audit rows so scheduler-driven changes are filterable as ModBy = "__scheduler"; existing preview-only controller call (debugOnly: true) still compiles and behaves the same - RapsRoleRefreshScheduledJob is the first IScheduledJob: nightly cron 0 0 * * * Pacific, wraps RoleViews - Reconciler re-registers any [ScheduledJob] declared job that has no Hangfire entry and no marker; tracked via a new LostRegistrationsHealed counter on the outcome
…t stamp - Adds a "Scheduler" tab to MainNav, gated by SVMSecure.CATS.scheduledJobs and pointing to /scheduler/dashboard; selectedTopNav highlights it when the admin is on the dashboard - Adds web/Areas/Scheduler/README.md covering job-onboarding recipe, configuration reference, dashboard URL, permission requirements, and an ops runbook (heartbeat verification, retry, pause/resume expectations, reconciler outcome counters, pre-escalation checklist) - Shortens ISchedulerJobsService.SchedulerActor from "__scheduler" to "__sched" so the audit stamp fits tblRoleMembers.ModBy (varchar(8)); caught while smoke-testing the RAPS role-refresh trigger end-to-end. The "__scheduler:" recurring-job-id prefix is unchanged - that string never lands in narrow legacy ModBy columns
Adds 17 unit tests over JobsController, asserting each action returns the expected ActionResult / status code on success and on every documented error path (system-job protection, marker not found, concurrency conflict, invalid base64 rowversion, missing body, missing HTTP user). Mocks ISchedulerJobsService and IUserHelper so the tests exercise only the controller's translation layer, not the service.
- Flip the base default to Hangfire:Enabled=true so all environments get the scheduler without per-environment opt-in. - Add Hangfire:AutoSchedule (default true). When false, recurring jobs register with Cron.Never so cron never fires; the worker still runs and the dashboard still mounts, so jobs remain visible and operators can fire them via "Trigger now" or BackgroundJob.Enqueue. Local dev opts out so the developer machine doesn't run cron jobs.
- Hangfire.Console (1.4.3) surfaces per-job execution logs inline on each job's detail page. Threaded Hangfire's PerformContext through ScheduledJobRunner into ScheduledJobContext, which exposes a no-op WriteLine helper so jobs can write progress lines without depending on Hangfire.Console directly. RapsRoleRefreshScheduledJob now writes start/per-message/done lines through the helper. - Hangfire.Heartbeat (0.6.0) reports per-server CPU/RAM/uptime on the dashboard's Servers page. UseHeartbeatPage renders the metrics; ProcessMonitor (registered as an IBackgroundProcess in DI so the simple AddHangfireServer overload picks it up) records them on each worker. Both must run for the graph to populate.
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
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.
Summary
Adds a shared, area-agnostic background-job scheduler on top of Hangfire 1.8 + SQL Server. Jobs implement a thin
IScheduledJoband are auto-discovered. A CAS+RAPS-gated Hangfire dashboard and a pause/resume admin API sit on top. The RAPS nightly role-refresh is the first consumer.Hangfire:Enableddefaults totrue; flip tofalsein any environment to unwire everything.What an admin sees
/scheduler/dashboard. Gated bySVMSecure.CATS.scheduledJobs(same permission as the legacy ColdFusion scheduler, so existing admins inherit access). Shows recurring jobs, queues, history, per-job execution console, and per-server CPU/RAM via the bundled Hangfire.Console + Hangfire.Heartbeat plugins./api/scheduler/jobs[/{id}/pause|/resume], same permission. Pause is marker-first (HTTP 202 if Hangfire deregistration trails); resume is registration-first; both are idempotent andRowVersion-guarded (HTTP 409 on conflict).hangfireon/health/detail: storage reachability + heartbeat freshness.What a job author sees
Discovery scans the web assembly at startup, validates a strict
__scheduler:system-prefix invariant, registers with DI, and wires Hangfire via a single dispatcher. Every run gets a fresh DI scope and aScheduledJobContextcarryingTriggerSource(Scheduled/Manual) and the effectiveModBy. Jobs never callUserHelper.GetCurrentUser().What's resilient
__scheduler:reconcile). Heals split-brain (marker + registration → registration removed), deletes stray system markers, and re-registers any[ScheduledJob]that vanished from Hangfire storage with no marker.__scheduler:) are refused by the API with HTTP 403; only the dashboard can touch them.Configuration
Hangfire:EnabledtrueHangfire:AutoScheduletrue(dev:false)Cron.Neverso cron doesn't fire. Worker still runs and "Trigger now" still works. Local dev opts out so developers don't run cron jobs.ConnectionStrings:VIPERWhere to focus review
HangfireExtensions.cs: the wiring spine (DI, server, dashboard, plugins). All flag handling lives here.SchedulerJobsService.cs: pause/resume/reconcile semantics + serialization round-trip.ScheduledJobDiscovery.cs+ScheduledJobRunner.cs: auto-registration + Hangfire dispatch.web/Areas/Scheduler/README.md: onboarding recipe + ops runbook.Decisions worth flagging
HangFireschema. A separate Hangfire DB would split EF queries from DDL and break pause/resume; revisit only with a dedicated scheduler DbContext.__schedaudit stamp (not__scheduler). LegacytblRoleMembers.ModByisvarchar(8), so the canonical 11-char stamp truncated. The recurring-job-id prefix__scheduler:is unchanged because that string never lands in narrow ModBy columns.Hangfire.Common.InvocationDataisn't compile-time accessible from this assembly's context (known Hangfire 1.8 wrinkle); a small record +System.Text.Jsonround-trips the same shape.Hangfire:Enabled=false. Matches the legacy "Computing" link behavior; documented in the README.Test plan
raps:role-refreshtriggered manually surfaces all 48 per-role messages in the dashboard consoleraps:role-refreshsucceeds; pause via API → dashboard showsIsPaused=true; resume → registration restored