api: projects: long running job cleanup (draft) #2242
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| @@ -177,4 +181,115 @@ | |||
| res.end(); | |||
| }); | |||
|
|
|||
| app.post( | |||
| "/job/projects-cleanup", | |||
| authorizer({ anyAdmin: true }), | |||
Check failure
Code scanning / CodeQL
Missing rate limiting High
| app.post( | ||
| "/job/projects-cleanup", | ||
| authorizer({ anyAdmin: true }), |
There was a problem hiding this comment.
Btw if you're creating this only because the other jobs have sth like it, it's unnecessary! The only reason they're like that is tech debt: we already had such APIs that were called from GitHub actions.
The cleanest way for a new job IMO will be to implement everything on the job. Then if later we want the API to trigger it manually, we can make an API that calls the job function (not the other way around).
Your call though, especially if you're already too far into having this API that it would be a big refactor to get rid of it now.
| }); | ||
|
|
||
| for (const asset of assets) { | ||
| await req.taskScheduler.deleteAsset(asset.id); |
There was a problem hiding this comment.
Make sure this doesn't send webhooks I guess? Or do we want webhooks?
| } | ||
|
|
||
| for (const stream of streams) { | ||
| await db.stream.update(stream.id, { |
There was a problem hiding this comment.
FWIW there's a markDeleted function on the tables which is generally used for the soft deletion
| for (const signingKey of signingKeys) { | ||
| await db.signingKey.update(signingKey.id, { | ||
| deleted: true, | ||
| disabled: true, |
There was a problem hiding this comment.
Do we need to disable them as well? Does the code that check disabled not check deleted? If so, that's the thing that needs to be fixed instead imo.
My reasoning is that if we ever want to "undelete" a project then we would need to figure out which signing keys were already disabled and which ones we disabled automatically here.
| const triggerSpy = jest | ||
| .spyOn(projectsController, "triggerCleanUpProjectsJob") | ||
| .mockImplementation(() => [[], Promise.resolve()]); |
There was a problem hiding this comment.
Same regarding the "inverted" nature of the current jobs implementation. Let's make sure the "real" tests are implemented here, which check the effects of the deletion job. The API that should be the "side-kick" which calls the job, but the core logic of the job should be the job itself
| let [projects] = await jobsDb.stream.find([sql`data->>'deleted' = 'true'`], { | ||
| limit, | ||
| order: "data->>'lastSeen' DESC", | ||
| }); |
There was a problem hiding this comment.
We probably need sth like a "deleting phase" on the projects too, otherwise this will keep listing deleted files that have already been cleaned-up ad infinitum. Maybe a simple cleanedUp boolean in the project that gets set by this job?
|
|
||
| let [projects] = await jobsDb.stream.find([sql`data->>'deleted' = 'true'`], { | ||
| limit, | ||
| order: "data->>'lastSeen' DESC", |
There was a problem hiding this comment.
Does this exist? I thought it was an user/api-key/stream thing only
What does this pull request do? Explain your changes. (required)
Specific updates (required)
How did you test each of these updates (required)
Does this pull request close any open issues?
Screenshots (optional)
Checklist