Fix ephemeral runner deletion #34447
Conversation
- repository deletion, deletes active tasks as well skips regular cleanup
- user deletion, implicitly deletes active tasks as well skips regular cleanup
- delete ephemeral runners once status changes to done
- You no longer see used ephemeral runners after the task is done, if you see one the cron job takes care of it
* repository deletion, deletes active tasks as well * user deletion, implicitly deletes active tasks as well * delete ephemeral runners once status changes to done
|
@NorthRealm Thank you for you workflow run deletion PR, without yours I might have not noticed these defects in my ephemeral runner implementation. I also noticed you can delete running workflows / tasks by deleting a User / Repository. If the ephemeral runner is shared to this repository this leaks.... Since the database is frozen for 1.24, I think this is the best that could be backported to 1.24. |
| assert.Len(t, runnerList.Entries, 4) | ||
|
|
There was a problem hiding this comment.
bad idea from myself for adding more test data, better just assert existence of a specific id. If we actually want to test paging via fixtures this problem reappears here.
| @@ -160,16 +158,20 @@ func testActionsRunnerOwner(t *testing.T) { | |||
| runnerList := api.ActionRunnersResponse{} | |||
| DecodeJSON(t, runnerListResp, &runnerList) | |||
|
|
|||
| assert.Len(t, runnerList.Entries, 1) | |||
There was a problem hiding this comment.
bad idea from myself for adding more test data, better just assert existence of a specific id
…stopherHX/34447
This reverts commit 8e8c127.
services/repository/delete.go
Outdated
| // I would make ephemeral runners fully delete directly before formally finishing the task | ||
| // | ||
| // See also: https://github.com/go-gitea/gitea/pull/34337#issuecomment-2862222788 | ||
| if err := actions_service.CleanupEphemeralRunnersByRepoID(ctx, repoID); err != nil { |
There was a problem hiding this comment.
Line 178, &actions_model.ActionRunner{RepoID: repoID}, all runners will be deleted
There was a problem hiding this comment.
yes all repository runners are going to be deleted by Line 178.
I also didn't notice this problem in my initial ephemeral runner Pull Request.
If I create an ephemeral runner on global/user/org level this scenario is possible
- global ephemeral runner 1 picks task 1 from repo_id 5 (not deleted by line Line 178)
- task 1 from repo_id 5 keeps running
- you trigger repository deletion of repo_id 5 (anybody who can create an account can do)
- task 1 is now deleted
- global ephemeral runner 1 is not deleted
- the ephemeral runner protection to prevent to pick a second task is now broken
- allows picking a second task until it is deleted
There was a problem hiding this comment.
Yes, I mean this addition lines looks duplicated?
There was a problem hiding this comment.
No it is not, this code deletes "global/user/org ephemeral runner 1" that is not part of the repository. I will update the comment.
The added repository deletion test must fail if this is removed
There was a problem hiding this comment.
Renamed method and added correct comment
// CleanupEphemeralRunnersByPickedTaskOfRepo deletes ephemeral global/org/user that have started any task of this repo
// The cannot pick a second task hardening for ephemeral runners expect that task objects remain available until runner deletion
// This method will delete affected ephemeral global/org/user runners
// &actions_model.ActionRunner{RepoID: repoID} does only handle ephemeral repository runnersAny ephemeral runner that has picked any job of the to be deleted repository must be deleted.
Not relevant for normal runners, since they are allowed to pick another job from another repository.
See test testEphemeralActionsRunnerDeletionByRepository, that verifies the runner that previously wasn't deleted is deleted for a running task
This reverts commit 46e8086.