Conversation
We need to stop possible bloat of databases should users of a seeded data fail to appropriately clean up after themselves. Using the hosted services present in other projects, this adds an alive job and play data delete job to the SeederApi
Development servers are unlikely to be running at midnight UTC, so we need to delete more frequently to ensure data is cleaned up. The Job still deletes things older than a day, it just checks much more frequently, now.
MGibson1
added
the
ai-review-vnext
🤖 Bitwarden Claude Code ReviewOverall Assessment: APPROVE This PR adds a Quartz-based scheduled job ( Code Review DetailsNo findings. The code is clean, follows existing codebase conventions, and correctly implements the Quartz job infrastructure pattern used elsewhere in the repository. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7281 +/- ##
==========================================
+ Coverage 57.74% 58.14% +0.40%
==========================================
Files 2043 2054 +11
Lines 89868 90771 +903
Branches 7991 8059 +68
==========================================
+ Hits 51895 52780 +885
- Misses 36115 36120 +5
- Partials 1858 1871 +13 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Couple thoughts |
|
All data tagged with a PlayId (i.e. PlayData) is by definition opted in to be ephemeral by the client requesting it's creation through the If, for some reason, you want to seed data that is not ephemeral, all you have to do is not include that header data and this job will not touch it. In some respects, we're assuming good actors here, but the endpoints on publicly accessible lower environments are not publicly accessible. |
util/SeederApi/Jobs/AliveJob.cs
Outdated
| public class AliveJob : BaseJob | ||
| { | ||
| private readonly GlobalSettings _globalSettings; | ||
| private HttpClient _httpClient = new HttpClient(); |
There was a problem hiding this comment.
🤔 Noting this as a known anti-pattern that causes socket exhaustion. Did we copy this from the src/Admin job?
There was a problem hiding this comment.
sure did. I don't know what issue, do we have a fix? Is this bump job unnecessary?
There was a problem hiding this comment.
The AliveJob is fine — the issue is private HttpClient _httpClient = new HttpClient(). Directly instantiating HttpClient as a field is a known antipattern that causes socket exhaustion under load because each instance gets its own connection pool and doesn't respect DNS TTLs.
The fix is to inject IHttpClientFactory and call httpClientFactory.CreateClient() in the constructor. We'll also need services.AddHttpClient() in program startup. The job itself can stay as-is.
|
|
New Issues (122)Checkmarx found the following issues in this Pull Request
Fixed Issues (3)Great job! The following issues were fixed in this Pull Request
|
|
|
||
| services.AddControllers(); | ||
|
|
||
| Jobs.JobsHostedService.AddJobsServices(services); |
There was a problem hiding this comment.
🤔 Deleting data with a hosted service always give me a little heartburn. Have we implemented opt-in techniques using environment settings in other places, by chance? I have done that in the past where my teams implemented background services that mutated data and gave us a better feel of control (especially when developing) knowing that one had to explicitly set the setting to ON to mutate/delete data?
There was a problem hiding this comment.
Interesting. Are you concerned with unintentional data loss? To some extent the goal of this job is to enforce that play data is ephemeral.
That can be a server setting if we want, I suppose, but maybe a better tweak would be a lifetime?
There was a problem hiding this comment.
In the ephemeral environments, I am not too concerned because I would not expect an ephemeral environment to live long enough to see the job run. Since the intention is really deleting this ephemeral data from long lived environments then I'm cool with it.
Only real issue is if you're developing tests locally or on one of the QA team VMs and you're expecting data to be there over a couple days (like a weekend) then you'd better be ready for the Seeder to clean itself up.
Did we add a README.md? (Sorry on the GH app so hard to go back n forth).
2 participants
Use Quartz-based hosted service to clear old play data
We need to stop possible bloat of databases should users of a seeded data fail to appropriately clean up after themselves.
Using the hosted services present in other projects, this adds an alive job and play data delete job to the SeederApi