From 038990e0ff6b49dc6fb848bdd1ca54e09ce7eb7c Mon Sep 17 00:00:00 2001 From: NorthRealm <155140859+NorthRealm@users.noreply.github.com> Date: Thu, 22 May 2025 06:02:34 +0800 Subject: [PATCH] Fix ephemeral runner deletion (#34447) (#34513) --- models/actions/runner.go | 18 +++++ models/actions/task.go | 5 ++ models/fixtures/action_runner.yml | 11 +++ models/fixtures/action_task.yml | 20 +++++ services/actions/cleanup.go | 16 ++++ services/repository/delete.go | 9 +++ tests/integration/api_actions_runner_test.go | 26 +++--- .../ephemeral_actions_runner_deletion_test.go | 79 +++++++++++++++++++ 8 files changed, 172 insertions(+), 12 deletions(-) create mode 100644 tests/integration/ephemeral_actions_runner_deletion_test.go diff --git a/models/actions/runner.go b/models/actions/runner.go index b55723efa0..81d4249ae0 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 43f11b2730..63259582f6 100644 --- a/models/actions/task.go +++ b/models/actions/task.go @@ -336,6 +336,11 @@ func UpdateTask(ctx context.Context, task *ActionTask, cols ...string) error { sess.Cols(cols...) } _, 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) + } return err } diff --git a/models/fixtures/action_runner.yml b/models/fixtures/action_runner.yml index dce2d41cfb..ecb7214006 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 506a47d8a0..f8831bf762 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 23d6e3a49d..f353e2c8e1 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 } + +// 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`"). + 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 cf960af8cf..046159722a 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,14 @@ func DeleteRepositoryDirectly(ctx context.Context, doer *user_model.User, repoID return err } + // 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.CleanupEphemeralRunnersByPickedTaskOfRepo(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/api_actions_runner_test.go b/tests/integration/api_actions_runner_test.go index 87b82e2ce9..fb9ba5b0c2 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) }) diff --git a/tests/integration/ephemeral_actions_runner_deletion_test.go b/tests/integration/ephemeral_actions_runner_deletion_test.go new file mode 100644 index 0000000000..765fcac8d7 --- /dev/null +++ b/tests/integration/ephemeral_actions_runner_deletion_test.go @@ -0,0 +1,79 @@ +// 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 TestEphemeralActionsRunnerDeletion(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) +} + +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) +}