From b3b8eb51825eb3bf3dced4e5d5914277a897fe49 Mon Sep 17 00:00:00 2001 From: Christopher Homberger Date: Tue, 13 May 2025 12:10:19 +0200 Subject: [PATCH 01/10] Fix ephemeral runner deletion * repository deletion, deletes active tasks as well * user deletion, implicitly deletes active tasks as well * delete ephemeral runners once status changes to done --- models/actions/runner.go | 18 +++++ models/actions/task.go | 4 + models/fixtures/action_runner.yml | 11 +++ models/fixtures/action_task.yml | 20 +++++ services/actions/cleanup.go | 16 ++++ services/repository/delete.go | 13 +++ .../actions_runner_ephemeral_deletion_test.go | 81 +++++++++++++++++++ 7 files changed, 163 insertions(+) create mode 100644 tests/integration/actions_runner_ephemeral_deletion_test.go diff --git a/models/actions/runner.go b/models/actions/runner.go index b55723efa08fc..81d4249ae0b85 100644 --- a/models/actions/runner.go +++ b/models/actions/runner.go @@ -5,6 +5,7 @@ package actions import ( "context" + "errors" "fmt" "strings" "time" @@ -298,6 +299,23 @@ func DeleteRunner(ctx context.Context, id int64) error { return err } +// DeleteEphemeralRunner deletes a ephemeral runner by given ID. +func DeleteEphemeralRunner(ctx context.Context, id int64) error { + runner, err := GetRunnerByID(ctx, id) + if err != nil { + if errors.Is(err, util.ErrNotExist) { + return nil + } + return err + } + if !runner.Ephemeral { + return nil + } + + _, err = db.DeleteByID[ActionRunner](ctx, id) + return err +} + // CreateRunner creates new runner. func CreateRunner(ctx context.Context, t *ActionRunner) error { if t.OwnerID != 0 && t.RepoID != 0 { diff --git a/models/actions/task.go b/models/actions/task.go index 43f11b273074f..bdd5e5d392ac0 100644 --- a/models/actions/task.go +++ b/models/actions/task.go @@ -336,6 +336,10 @@ func UpdateTask(ctx context.Context, task *ActionTask, cols ...string) error { sess.Cols(cols...) } _, err := sess.Update(task) + + if err == nil && task.Status.IsDone() && util.SliceContainsString(cols, "status") { + return DeleteEphemeralRunner(ctx, task.RunnerID) + } return err } diff --git a/models/fixtures/action_runner.yml b/models/fixtures/action_runner.yml index dce2d41cfb2d6..ecb7214006574 100644 --- a/models/fixtures/action_runner.yml +++ b/models/fixtures/action_runner.yml @@ -38,3 +38,14 @@ repo_id: 0 description: "This runner is going to be deleted" agent_labels: '["runner_to_be_deleted","linux"]' +- + id: 34350 + name: runner_to_be_deleted-org-ephemeral + uuid: 3FF231BD-FBB7-4E4B-9602-E6F28363EF20 + token_hash: 3FF231BD-FBB7-4E4B-9602-E6F28363EF20 + ephemeral: true + version: "1.0.0" + owner_id: 3 + repo_id: 0 + description: "This runner is going to be deleted" + agent_labels: '["runner_to_be_deleted","linux"]' diff --git a/models/fixtures/action_task.yml b/models/fixtures/action_task.yml index 506a47d8a04dd..f8831bf762a01 100644 --- a/models/fixtures/action_task.yml +++ b/models/fixtures/action_task.yml @@ -117,3 +117,23 @@ log_length: 707 log_size: 90179 log_expired: 0 +- + id: 52 + job_id: 196 + attempt: 1 + runner_id: 34350 + status: 6 # running + started: 1683636528 + stopped: 1683636626 + repo_id: 4 + owner_id: 1 + commit_sha: c2d72f548424103f01ee1dc02889c1e2bff816b0 + is_fork_pull_request: 0 + token_hash: f8d3962425466b6709b9ac51446f93260c54afe8e7b6d3686e34f991fb8a8953822b0deed86fe41a103f34bc48dbc4784222 + token_salt: ffffffffff + token_last_eight: ffffffff + log_filename: artifact-test2/2f/47.log + log_in_storage: 1 + log_length: 707 + log_size: 90179 + log_expired: 0 diff --git a/services/actions/cleanup.go b/services/actions/cleanup.go index 23d6e3a49d954..1847625e1311c 100644 --- a/services/actions/cleanup.go +++ b/services/actions/cleanup.go @@ -148,3 +148,19 @@ func CleanupEphemeralRunners(ctx context.Context) error { log.Info("Removed %d runners", affected) return nil } + +// CleanupEphemeralRunners removes used ephemeral runners which are no longer able to process jobs +func CleanupEphemeralRunnersByRepoID(ctx context.Context, repoID int64) error { + subQuery := builder.Select("`action_runner`.id"). + From(builder.Select("*").From("`action_runner`"), "`action_runner`"). // mysql needs this redundant subquery + Join("INNER", "`action_task`", "`action_task`.`runner_id` = `action_runner`.`id`"). + Where(builder.And(builder.Eq{"`action_runner`.`ephemeral`": true}, builder.Eq{"`action_task`.`repo_id`": repoID})) + b := builder.Delete(builder.In("id", subQuery)).From("`action_runner`") + res, err := db.GetEngine(ctx).Exec(b) + if err != nil { + return fmt.Errorf("find runners: %w", err) + } + affected, _ := res.RowsAffected() + log.Info("Removed %d runners", affected) + return nil +} diff --git a/services/repository/delete.go b/services/repository/delete.go index cf960af8cf748..e87f9220cfede 100644 --- a/services/repository/delete.go +++ b/services/repository/delete.go @@ -27,6 +27,7 @@ import ( "code.gitea.io/gitea/modules/lfs" "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/storage" + actions_service "code.gitea.io/gitea/services/actions" asymkey_service "code.gitea.io/gitea/services/asymkey" "xorm.io/builder" @@ -133,6 +134,18 @@ func DeleteRepositoryDirectly(ctx context.Context, doer *user_model.User, repoID return err } + // TODO: Deleting task records could break current ephemeral runner implementation. This is a temporary workaround suggested by ChristopherHX. + // Since you delete potentially the only task an ephemeral act_runner has ever run, please delete the affected runners first. + // one of + // call cleanup ephemeral runners first + // delete affected ephemeral act_runners + // 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 { + return fmt.Errorf("cleanupEphemeralRunners: %w", err) + } + if err := db.DeleteBeans(ctx, &access_model.Access{RepoID: repo.ID}, &activities_model.Action{RepoID: repo.ID}, diff --git a/tests/integration/actions_runner_ephemeral_deletion_test.go b/tests/integration/actions_runner_ephemeral_deletion_test.go new file mode 100644 index 0000000000000..56f9a9ecb19f3 --- /dev/null +++ b/tests/integration/actions_runner_ephemeral_deletion_test.go @@ -0,0 +1,81 @@ +// Copyright 2025 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package integration + +import ( + "testing" + + actions_model "code.gitea.io/gitea/models/actions" + "code.gitea.io/gitea/models/unittest" + user_model "code.gitea.io/gitea/models/user" + "code.gitea.io/gitea/modules/util" + repo_service "code.gitea.io/gitea/services/repository" + user_service "code.gitea.io/gitea/services/user" + "code.gitea.io/gitea/tests" + + "github.com/stretchr/testify/assert" +) + +func TestActionsRunnerEphemeralDeletion(t *testing.T) { + + t.Run("ByTaskCompletion", testEphemeralActionsRunnerDeletionByTaskCompletion) + t.Run("ByRepository", testEphemeralActionsRunnerDeletionByRepository) + t.Run("ByUser", testEphemeralActionsRunnerDeletionByUser) +} + +// Test that the ephemeral runner is deleted when the task is finished +func testEphemeralActionsRunnerDeletionByTaskCompletion(t *testing.T) { + defer tests.PrepareTestEnv(t)() + + _, err := actions_model.GetRunnerByID(t.Context(), 34350) + assert.NoError(t, err) + + task := unittest.AssertExistsAndLoadBean(t, &actions_model.ActionTask{ID: 52}) + assert.Equal(t, actions_model.StatusRunning, task.Status) + + task.Status = actions_model.StatusSuccess + err = actions_model.UpdateTask(t.Context(), task, "status") + assert.NoError(t, err) + + _, err = actions_model.GetRunnerByID(t.Context(), 34350) + assert.ErrorIs(t, err, util.ErrNotExist) +} + +// Test that the ephemeral runner is deleted when a repository is deleted +func testEphemeralActionsRunnerDeletionByRepository(t *testing.T) { + defer tests.PrepareTestEnv(t)() + + _, err := actions_model.GetRunnerByID(t.Context(), 34350) + assert.NoError(t, err) + + task := unittest.AssertExistsAndLoadBean(t, &actions_model.ActionTask{ID: 52}) + assert.Equal(t, actions_model.StatusRunning, task.Status) + + user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) + + err = repo_service.DeleteRepositoryDirectly(t.Context(), user, task.RepoID, true) + assert.NoError(t, err) + + _, err = actions_model.GetRunnerByID(t.Context(), 34350) + assert.ErrorIs(t, err, util.ErrNotExist) +} + +// Test that the ephemeral runner is deleted when a user is deleted +func testEphemeralActionsRunnerDeletionByUser(t *testing.T) { + defer tests.PrepareTestEnv(t)() + + _, err := actions_model.GetRunnerByID(t.Context(), 34350) + assert.NoError(t, err) + + task := unittest.AssertExistsAndLoadBean(t, &actions_model.ActionTask{ID: 52}) + assert.Equal(t, actions_model.StatusRunning, task.Status) + + user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) + + err = user_service.DeleteUser(t.Context(), user, true) + assert.NoError(t, err) + + _, err = actions_model.GetRunnerByID(t.Context(), 34350) + assert.ErrorIs(t, err, util.ErrNotExist) +} From 3adc64bfa0a653c0a82d57dda80c9866467e6f28 Mon Sep 17 00:00:00 2001 From: Christopher Homberger Date: Tue, 13 May 2025 12:11:58 +0200 Subject: [PATCH 02/10] fix fmt --- tests/integration/actions_runner_ephemeral_deletion_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/integration/actions_runner_ephemeral_deletion_test.go b/tests/integration/actions_runner_ephemeral_deletion_test.go index 56f9a9ecb19f3..b23c1f45dc89d 100644 --- a/tests/integration/actions_runner_ephemeral_deletion_test.go +++ b/tests/integration/actions_runner_ephemeral_deletion_test.go @@ -18,7 +18,6 @@ import ( ) func TestActionsRunnerEphemeralDeletion(t *testing.T) { - t.Run("ByTaskCompletion", testEphemeralActionsRunnerDeletionByTaskCompletion) t.Run("ByRepository", testEphemeralActionsRunnerDeletionByRepository) t.Run("ByUser", testEphemeralActionsRunnerDeletionByUser) From 4dcdf9f63f93d2b4b3c7eee2d0ec543787bfda7b Mon Sep 17 00:00:00 2001 From: Christopher Homberger Date: Tue, 13 May 2025 12:29:17 +0200 Subject: [PATCH 03/10] fix typo --- services/actions/cleanup.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/services/actions/cleanup.go b/services/actions/cleanup.go index 1847625e1311c..7fad494709a66 100644 --- a/services/actions/cleanup.go +++ b/services/actions/cleanup.go @@ -149,7 +149,7 @@ func CleanupEphemeralRunners(ctx context.Context) error { return nil } -// CleanupEphemeralRunners removes used ephemeral runners which are no longer able to process jobs +// CleanupEphemeralRunnersByRepoID removes all ephemeral runners that have active tasks on the given repository func CleanupEphemeralRunnersByRepoID(ctx context.Context, repoID int64) error { subQuery := builder.Select("`action_runner`.id"). From(builder.Select("*").From("`action_runner`"), "`action_runner`"). // mysql needs this redundant subquery From d593f3603568b3ca22183a2b9b7eda42ecabf1f0 Mon Sep 17 00:00:00 2001 From: Christopher Homberger Date: Tue, 13 May 2025 13:13:04 +0200 Subject: [PATCH 04/10] remove length checks of some runner tests --- modules/storage/local.go | 6 ++++- tests/integration/api_actions_runner_test.go | 26 +++++++++++--------- 2 files changed, 19 insertions(+), 13 deletions(-) diff --git a/modules/storage/local.go b/modules/storage/local.go index 00c7f668aa2c3..1a766f2dc527d 100644 --- a/modules/storage/local.go +++ b/modules/storage/local.go @@ -121,7 +121,7 @@ func (l *LocalStorage) URL(path, name string, reqParams url.Values) (*url.URL, e // IterateObjects iterates across the objects in the local storage func (l *LocalStorage) IterateObjects(dirName string, fn func(path string, obj Object) error) error { dir := l.buildLocalPath(dirName) - return filepath.WalkDir(dir, func(path string, d os.DirEntry, err error) error { + err := filepath.WalkDir(dir, func(path string, d os.DirEntry, err error) error { if err != nil { return err } @@ -147,6 +147,10 @@ func (l *LocalStorage) IterateObjects(dirName string, fn func(path string, obj O defer obj.Close() return fn(relPath, obj) }) + if os.IsNotExist(err) { + return nil + } + return err } func init() { diff --git a/tests/integration/api_actions_runner_test.go b/tests/integration/api_actions_runner_test.go index ace7aa381a344..314b2b5963a67 100644 --- a/tests/integration/api_actions_runner_test.go +++ b/tests/integration/api_actions_runner_test.go @@ -41,8 +41,6 @@ func testActionsRunnerAdmin(t *testing.T) { runnerList := api.ActionRunnersResponse{} DecodeJSON(t, runnerListResp, &runnerList) - assert.Len(t, runnerList.Entries, 4) - idx := slices.IndexFunc(runnerList.Entries, func(e *api.ActionRunner) bool { return e.ID == 34349 }) require.NotEqual(t, -1, idx) expectedRunner := runnerList.Entries[idx] @@ -160,16 +158,20 @@ func testActionsRunnerOwner(t *testing.T) { runnerList := api.ActionRunnersResponse{} DecodeJSON(t, runnerListResp, &runnerList) - assert.Len(t, runnerList.Entries, 1) - assert.Equal(t, "runner_to_be_deleted-org", runnerList.Entries[0].Name) - assert.Equal(t, int64(34347), runnerList.Entries[0].ID) - assert.False(t, runnerList.Entries[0].Ephemeral) - assert.Len(t, runnerList.Entries[0].Labels, 2) - assert.Equal(t, "runner_to_be_deleted", runnerList.Entries[0].Labels[0].Name) - assert.Equal(t, "linux", runnerList.Entries[0].Labels[1].Name) + idx := slices.IndexFunc(runnerList.Entries, func(e *api.ActionRunner) bool { return e.ID == 34347 }) + require.NotEqual(t, -1, idx) + expectedRunner := runnerList.Entries[idx] + + require.NotNil(t, expectedRunner) + assert.Equal(t, "runner_to_be_deleted-org", expectedRunner.Name) + assert.Equal(t, int64(34347), expectedRunner.ID) + assert.False(t, expectedRunner.Ephemeral) + assert.Len(t, expectedRunner.Labels, 2) + assert.Equal(t, "runner_to_be_deleted", expectedRunner.Labels[0].Name) + assert.Equal(t, "linux", expectedRunner.Labels[1].Name) // Verify get the runner by id - req = NewRequest(t, "GET", fmt.Sprintf("/api/v1/orgs/org3/actions/runners/%d", runnerList.Entries[0].ID)).AddTokenAuth(token) + req = NewRequest(t, "GET", fmt.Sprintf("/api/v1/orgs/org3/actions/runners/%d", expectedRunner.ID)).AddTokenAuth(token) runnerResp := MakeRequest(t, req, http.StatusOK) runner := api.ActionRunner{} @@ -183,11 +185,11 @@ func testActionsRunnerOwner(t *testing.T) { assert.Equal(t, "linux", runner.Labels[1].Name) // Verify delete the runner by id - req = NewRequest(t, "DELETE", fmt.Sprintf("/api/v1/orgs/org3/actions/runners/%d", runnerList.Entries[0].ID)).AddTokenAuth(token) + req = NewRequest(t, "DELETE", fmt.Sprintf("/api/v1/orgs/org3/actions/runners/%d", expectedRunner.ID)).AddTokenAuth(token) MakeRequest(t, req, http.StatusNoContent) // Verify runner deletion - req = NewRequest(t, "GET", fmt.Sprintf("/api/v1/orgs/org3/actions/runners/%d", runnerList.Entries[0].ID)).AddTokenAuth(token) + req = NewRequest(t, "GET", fmt.Sprintf("/api/v1/orgs/org3/actions/runners/%d", expectedRunner.ID)).AddTokenAuth(token) MakeRequest(t, req, http.StatusNotFound) }) From 8e8c1277e37ee094d519002196279bd553d60647 Mon Sep 17 00:00:00 2001 From: Christopher Homberger Date: Tue, 13 May 2025 21:34:07 +0200 Subject: [PATCH 05/10] revert local storage just to be sure --- modules/storage/local.go | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/modules/storage/local.go b/modules/storage/local.go index 1a766f2dc527d..00c7f668aa2c3 100644 --- a/modules/storage/local.go +++ b/modules/storage/local.go @@ -121,7 +121,7 @@ func (l *LocalStorage) URL(path, name string, reqParams url.Values) (*url.URL, e // IterateObjects iterates across the objects in the local storage func (l *LocalStorage) IterateObjects(dirName string, fn func(path string, obj Object) error) error { dir := l.buildLocalPath(dirName) - err := filepath.WalkDir(dir, func(path string, d os.DirEntry, err error) error { + return filepath.WalkDir(dir, func(path string, d os.DirEntry, err error) error { if err != nil { return err } @@ -147,10 +147,6 @@ func (l *LocalStorage) IterateObjects(dirName string, fn func(path string, obj O defer obj.Close() return fn(relPath, obj) }) - if os.IsNotExist(err) { - return nil - } - return err } func init() { From 46e80864537c3a7018e96877400ca22e1c1cb885 Mon Sep 17 00:00:00 2001 From: Christopher Homberger Date: Tue, 13 May 2025 21:55:43 +0200 Subject: [PATCH 06/10] Revert "revert local storage just to be sure" This reverts commit 8e8c1277e37ee094d519002196279bd553d60647. --- modules/storage/local.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/modules/storage/local.go b/modules/storage/local.go index 00c7f668aa2c3..1a766f2dc527d 100644 --- a/modules/storage/local.go +++ b/modules/storage/local.go @@ -121,7 +121,7 @@ func (l *LocalStorage) URL(path, name string, reqParams url.Values) (*url.URL, e // IterateObjects iterates across the objects in the local storage func (l *LocalStorage) IterateObjects(dirName string, fn func(path string, obj Object) error) error { dir := l.buildLocalPath(dirName) - return filepath.WalkDir(dir, func(path string, d os.DirEntry, err error) error { + err := filepath.WalkDir(dir, func(path string, d os.DirEntry, err error) error { if err != nil { return err } @@ -147,6 +147,10 @@ func (l *LocalStorage) IterateObjects(dirName string, fn func(path string, obj O defer obj.Close() return fn(relPath, obj) }) + if os.IsNotExist(err) { + return nil + } + return err } func init() { From ee1058bb5e411fe68cc5cd807c933ca3c9789d6b Mon Sep 17 00:00:00 2001 From: Christopher Homberger Date: Wed, 14 May 2025 11:49:04 +0200 Subject: [PATCH 07/10] fix comment and method name --- services/actions/cleanup.go | 4 ++-- services/repository/delete.go | 14 +++++--------- 2 files changed, 7 insertions(+), 11 deletions(-) diff --git a/services/actions/cleanup.go b/services/actions/cleanup.go index 99dacf0dd03b9..f2634f2921322 100644 --- a/services/actions/cleanup.go +++ b/services/actions/cleanup.go @@ -155,8 +155,8 @@ func CleanupEphemeralRunners(ctx context.Context) error { return nil } -// CleanupEphemeralRunnersByRepoID removes all ephemeral runners that have active tasks on the given repository -func CleanupEphemeralRunnersByRepoID(ctx context.Context, repoID int64) error { +// CleanupEphemeralRunnersByPickedTaskRepoID removes all ephemeral runners that have active/finished tasks on the given repository +func CleanupEphemeralRunnersByPickedTaskRepoID(ctx context.Context, repoID int64) error { subQuery := builder.Select("`action_runner`.id"). From(builder.Select("*").From("`action_runner`"), "`action_runner`"). // mysql needs this redundant subquery Join("INNER", "`action_task`", "`action_task`.`runner_id` = `action_runner`.`id`"). diff --git a/services/repository/delete.go b/services/repository/delete.go index e87f9220cfede..273df090aea8b 100644 --- a/services/repository/delete.go +++ b/services/repository/delete.go @@ -134,15 +134,11 @@ func DeleteRepositoryDirectly(ctx context.Context, doer *user_model.User, repoID return err } - // TODO: Deleting task records could break current ephemeral runner implementation. This is a temporary workaround suggested by ChristopherHX. - // Since you delete potentially the only task an ephemeral act_runner has ever run, please delete the affected runners first. - // one of - // call cleanup ephemeral runners first - // delete affected ephemeral act_runners - // 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 { + // CleanupEphemeralRunnersByPickedTaskRepoID does delete 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 runners + if err := actions_service.CleanupEphemeralRunnersByPickedTaskRepoID(ctx, repoID); err != nil { return fmt.Errorf("cleanupEphemeralRunners: %w", err) } From d344fed61c00bc01289aa24ac1ec6c42aa4c1cb3 Mon Sep 17 00:00:00 2001 From: Christopher Homberger Date: Wed, 14 May 2025 22:58:34 +0200 Subject: [PATCH 08/10] Revert "Revert "revert local storage just to be sure"" This reverts commit 46e80864537c3a7018e96877400ca22e1c1cb885. --- modules/storage/local.go | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/modules/storage/local.go b/modules/storage/local.go index 1a766f2dc527d..00c7f668aa2c3 100644 --- a/modules/storage/local.go +++ b/modules/storage/local.go @@ -121,7 +121,7 @@ func (l *LocalStorage) URL(path, name string, reqParams url.Values) (*url.URL, e // IterateObjects iterates across the objects in the local storage func (l *LocalStorage) IterateObjects(dirName string, fn func(path string, obj Object) error) error { dir := l.buildLocalPath(dirName) - err := filepath.WalkDir(dir, func(path string, d os.DirEntry, err error) error { + return filepath.WalkDir(dir, func(path string, d os.DirEntry, err error) error { if err != nil { return err } @@ -147,10 +147,6 @@ func (l *LocalStorage) IterateObjects(dirName string, fn func(path string, obj O defer obj.Close() return fn(relPath, obj) }) - if os.IsNotExist(err) { - return nil - } - return err } func init() { From d2af3ea701b3249601cf7fdf551df1845ff0b809 Mon Sep 17 00:00:00 2001 From: Christopher Homberger Date: Wed, 14 May 2025 23:01:48 +0200 Subject: [PATCH 09/10] ... --- models/actions/task.go | 1 + services/actions/cleanup.go | 4 ++-- services/repository/delete.go | 4 ++-- .../actions_runner_ephemeral_deletion_test.go | 21 ------------------- 4 files changed, 5 insertions(+), 25 deletions(-) diff --git a/models/actions/task.go b/models/actions/task.go index bdd5e5d392ac0..63259582f6374 100644 --- a/models/actions/task.go +++ b/models/actions/task.go @@ -337,6 +337,7 @@ func UpdateTask(ctx context.Context, task *ActionTask, cols ...string) error { } _, err := sess.Update(task) + // Automatically delete the ephemeral runner if the task is done if err == nil && task.Status.IsDone() && util.SliceContainsString(cols, "status") { return DeleteEphemeralRunner(ctx, task.RunnerID) } diff --git a/services/actions/cleanup.go b/services/actions/cleanup.go index f2634f2921322..d0cc63e538872 100644 --- a/services/actions/cleanup.go +++ b/services/actions/cleanup.go @@ -155,8 +155,8 @@ func CleanupEphemeralRunners(ctx context.Context) error { return nil } -// CleanupEphemeralRunnersByPickedTaskRepoID removes all ephemeral runners that have active/finished tasks on the given repository -func CleanupEphemeralRunnersByPickedTaskRepoID(ctx context.Context, repoID int64) error { +// CleanupEphemeralRunnersByPickedTaskOfRepo removes all ephemeral runners that have active/finished tasks on the given repository +func CleanupEphemeralRunnersByPickedTaskOfRepo(ctx context.Context, repoID int64) error { subQuery := builder.Select("`action_runner`.id"). From(builder.Select("*").From("`action_runner`"), "`action_runner`"). // mysql needs this redundant subquery Join("INNER", "`action_task`", "`action_task`.`runner_id` = `action_runner`.`id`"). diff --git a/services/repository/delete.go b/services/repository/delete.go index 273df090aea8b..046159722a823 100644 --- a/services/repository/delete.go +++ b/services/repository/delete.go @@ -134,11 +134,11 @@ func DeleteRepositoryDirectly(ctx context.Context, doer *user_model.User, repoID return err } - // CleanupEphemeralRunnersByPickedTaskRepoID does delete ephemeral global/org/user that have started any task of this repo + // 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 runners - if err := actions_service.CleanupEphemeralRunnersByPickedTaskRepoID(ctx, repoID); err != nil { + if err := actions_service.CleanupEphemeralRunnersByPickedTaskOfRepo(ctx, repoID); err != nil { return fmt.Errorf("cleanupEphemeralRunners: %w", err) } diff --git a/tests/integration/actions_runner_ephemeral_deletion_test.go b/tests/integration/actions_runner_ephemeral_deletion_test.go index b23c1f45dc89d..acaaf9b65c1ea 100644 --- a/tests/integration/actions_runner_ephemeral_deletion_test.go +++ b/tests/integration/actions_runner_ephemeral_deletion_test.go @@ -10,7 +10,6 @@ import ( "code.gitea.io/gitea/models/unittest" user_model "code.gitea.io/gitea/models/user" "code.gitea.io/gitea/modules/util" - repo_service "code.gitea.io/gitea/services/repository" user_service "code.gitea.io/gitea/services/user" "code.gitea.io/gitea/tests" @@ -19,7 +18,6 @@ import ( func TestActionsRunnerEphemeralDeletion(t *testing.T) { t.Run("ByTaskCompletion", testEphemeralActionsRunnerDeletionByTaskCompletion) - t.Run("ByRepository", testEphemeralActionsRunnerDeletionByRepository) t.Run("ByUser", testEphemeralActionsRunnerDeletionByUser) } @@ -41,25 +39,6 @@ func testEphemeralActionsRunnerDeletionByTaskCompletion(t *testing.T) { assert.ErrorIs(t, err, util.ErrNotExist) } -// Test that the ephemeral runner is deleted when a repository is deleted -func testEphemeralActionsRunnerDeletionByRepository(t *testing.T) { - defer tests.PrepareTestEnv(t)() - - _, err := actions_model.GetRunnerByID(t.Context(), 34350) - assert.NoError(t, err) - - task := unittest.AssertExistsAndLoadBean(t, &actions_model.ActionTask{ID: 52}) - assert.Equal(t, actions_model.StatusRunning, task.Status) - - user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) - - err = repo_service.DeleteRepositoryDirectly(t.Context(), user, task.RepoID, true) - assert.NoError(t, err) - - _, err = actions_model.GetRunnerByID(t.Context(), 34350) - assert.ErrorIs(t, err, util.ErrNotExist) -} - // Test that the ephemeral runner is deleted when a user is deleted func testEphemeralActionsRunnerDeletionByUser(t *testing.T) { defer tests.PrepareTestEnv(t)() From 6da57f5f4f72224930dd1a8bda9d6001134bddea Mon Sep 17 00:00:00 2001 From: Christopher Homberger Date: Wed, 14 May 2025 23:18:54 +0200 Subject: [PATCH 10/10] rename test --- ...ephemeral_actions_runner_deletion_test.go} | 22 ++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-) rename tests/integration/{actions_runner_ephemeral_deletion_test.go => ephemeral_actions_runner_deletion_test.go} (69%) diff --git a/tests/integration/actions_runner_ephemeral_deletion_test.go b/tests/integration/ephemeral_actions_runner_deletion_test.go similarity index 69% rename from tests/integration/actions_runner_ephemeral_deletion_test.go rename to tests/integration/ephemeral_actions_runner_deletion_test.go index acaaf9b65c1ea..765fcac8d7c7a 100644 --- a/tests/integration/actions_runner_ephemeral_deletion_test.go +++ b/tests/integration/ephemeral_actions_runner_deletion_test.go @@ -10,14 +10,16 @@ import ( "code.gitea.io/gitea/models/unittest" user_model "code.gitea.io/gitea/models/user" "code.gitea.io/gitea/modules/util" + repo_service "code.gitea.io/gitea/services/repository" user_service "code.gitea.io/gitea/services/user" "code.gitea.io/gitea/tests" "github.com/stretchr/testify/assert" ) -func TestActionsRunnerEphemeralDeletion(t *testing.T) { +func TestEphemeralActionsRunnerDeletion(t *testing.T) { t.Run("ByTaskCompletion", testEphemeralActionsRunnerDeletionByTaskCompletion) + t.Run("ByRepository", testEphemeralActionsRunnerDeletionByRepository) t.Run("ByUser", testEphemeralActionsRunnerDeletionByUser) } @@ -39,6 +41,24 @@ func testEphemeralActionsRunnerDeletionByTaskCompletion(t *testing.T) { assert.ErrorIs(t, err, util.ErrNotExist) } +func testEphemeralActionsRunnerDeletionByRepository(t *testing.T) { + defer tests.PrepareTestEnv(t)() + + _, err := actions_model.GetRunnerByID(t.Context(), 34350) + assert.NoError(t, err) + + task := unittest.AssertExistsAndLoadBean(t, &actions_model.ActionTask{ID: 52}) + assert.Equal(t, actions_model.StatusRunning, task.Status) + + user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) + + err = repo_service.DeleteRepositoryDirectly(t.Context(), user, task.RepoID, true) + assert.NoError(t, err) + + _, err = actions_model.GetRunnerByID(t.Context(), 34350) + assert.ErrorIs(t, err, util.ErrNotExist) +} + // Test that the ephemeral runner is deleted when a user is deleted func testEphemeralActionsRunnerDeletionByUser(t *testing.T) { defer tests.PrepareTestEnv(t)()