Use DB time for background job time limit#5560
Conversation
LTA-Thinking
added
Bug
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5560 +/- ##
=======================================
Coverage ? 77.42%
=======================================
Files ? 993
Lines ? 36411
Branches ? 5515
=======================================
Hits ? 28192
Misses ? 6865
Partials ? 1354 🚀 New features to boost your workflow:
|
rbans96
left a comment
rbans96
left a comment
There was a problem hiding this comment.
Nice fix ? using DB time eliminates the clock skew issue cleanly. The Task.Delay removals in E2E tests confirm it works. A couple things to consider:
| var searchParameters = new List<Tuple<string, string>>() | ||
| { | ||
| new Tuple<string, string>(KnownQueryParameterNames.Summary, "count"), | ||
| new Tuple<string, string>(KnownQueryParameterNames.LastUpdated, $"lt{createDate}"), |
There was a problem hiding this comment.
This uses lt but BulkUpdate's equivalent uses le. The original code used lt for both. Was switching BulkUpdate to le intentional? As-is, a resource written at the exact same instant as CreateDate gets skipped by delete but included by update.
|
|
||
| QueuedTime = Clock.UtcNow; | ||
| Till = till ?? new PartialDateTime(Clock.UtcNow); | ||
| Till = till; |
There was a problem hiding this comment.
Till can now be null between job creation and orchestrator execution (previously it always had a value via the ?? Clock.UtcNow fallback). Any code that reads Till before the orchestrator runs would hit a null. Worth checking nothing does, or keeping a default.
| record.QueuedTime = jobInfo.CreateDate; // get record of truth | ||
|
|
||
| // If Till was not explicitly set by the user, use the job's CreateDate | ||
| if (!record.IsTillExplicit) |
There was a problem hiding this comment.
nit: Same block exists in SqlExportOrchestratorJob.cs. Maybe extract to a shared helper to avoid drift.
3 participants
Description
Some background jobs have a clause to prevent them working on data created after they were queued. This PR changes the source of the time to be the DB set create time instead of it being set by the server when the job is queued. This prevents mismatches between the two sources that could lead to resources being missed if they were created very soon before a job was enqueued.
Related issues
Addresses Bug 191061
Testing
Existing E2E tests
FHIR Team Checklist
Semver Change (docs)
Patch|Skip|Feature|Breaking (reason)