[v15.0/forgejo] fix: do not try to remove task logs that don't exist (#13055)

The routines for the removal of `ActionTask` expect that each `ActionTask` has logs attached and that `LogFilename` isn't empty. However, that is not always the case. For example, Forgejo can resolve jobs without dispatching them to a runner. In that case, a placeholder task is created without logs and `LogFilename`. The log removal routines simply concatenate the path of the log storage directory and `LogFilename` and try to delete that without verifying that `LogFilename` is present. Consequently, they try to remove the log storage directory. In most cases, that causes an error because the directory contains some files. To prevent that from happening, the log removal routines no longer allow empty filenames. And it is checked whether a task has logs before invoking them.

### Tests for Go changes

- I added test coverage for Go changes...
  - [x] in their respective `*_test.go` for unit tests.
  - [ ] in the `tests/integration` directory if it involves interactions with a live Forgejo server.
- I ran...
  - [ ] `make pr-go` before pushing

### Documentation

- [ ] I created a pull request [to the documentation](https://codeberg.org/forgejo/docs) to explain to Forgejo users how to use this change.
- [x] I did not document these changes and I do not expect someone else to do it.

### Release notes

- [x] This change will be noticed by a Forgejo user or admin (feature, bug fix, performance, etc.). I suggest to include a release note for this change.
- [ ] This change is not visible to a Forgejo user or admin (refactor, dependency upgrade, etc.). I think there is no need to add a release note for this change.

Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/13040
Reviewed-by: Gusted <gusted@noreply.codeberg.org>
Reviewed-by: Mathieu Fenniak <mfenniak@noreply.codeberg.org>

(cherry picked from commit 7bd33f32e2)

Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/13055
Reviewed-by: Gusted <gusted@noreply.codeberg.org>

models/actions/task.go

@@ -161,6 +161,10 @@ func (task *ActionTask) UpdateToken(ctx context.Context) error {
 	return UpdateTask(ctx, task, "token_hash", "token_salt", "token_last_eight")
 }
 
+func (task *ActionTask) HasLogs() bool {
+	return task.LogFilename != ""
+}
+
 func GetTaskByID(ctx context.Context, id int64) (*ActionTask, error) {
 	var task ActionTask
 	has, err := db.GetEngine(ctx).Where("id=?", id).Get(&task)

modules/actions/log.go

@@ -173,7 +173,12 @@ func TransferLogs(ctx context.Context, filename string) (func(), error) {
 	return remove, nil
 }
 
+// RemoveLogs deletes the log file with the given filename. Returns an error if filename is empty.
 func RemoveLogs(ctx context.Context, inStorage bool, filename string) error {
+	if filename == "" {
+		return errors.New("cannot remove logs because filename is empty")
+	}
+
 	if !inStorage {
 		name := DBFSPrefix + filename
 		err := dbfs.Remove(ctx, name)

modules/actions/log_test.go

@@ -0,0 +1,59 @@
+// Copyright 2026 The Forgejo Authors. All rights reserved.
+// SPDX-License-Identifier: GPL-3.0-or-later
+
+package actions
+
+import (
+	"testing"
+	"time"
+
+	"forgejo.org/models/unittest"
+
+	runnerv1 "code.forgejo.org/forgejo/actions-proto/runner/v1"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"google.golang.org/protobuf/types/known/timestamppb"
+)
+
+func TestRemoveLogs(t *testing.T) {
+	t.Run("Logs removed", func(t *testing.T) {
+		require.NoError(t, unittest.PrepareTestDatabase())
+
+		_, err := WriteLogs(t.Context(), "test.log", 0,
+			[]*runnerv1.LogRow{{Time: timestamppb.New(time.Now()), Content: "Hello world"}})
+		require.NoError(t, err)
+
+		exist, err := ExistsLogs(t.Context(), "test.log")
+		require.NoError(t, err)
+
+		assert.True(t, exist)
+
+		require.NoError(t, RemoveLogs(t.Context(), false, "test.log"))
+
+		exist, err = ExistsLogs(t.Context(), "test.log")
+		require.NoError(t, err)
+
+		assert.False(t, exist)
+	})
+
+	t.Run("Error if filename is empty", func(t *testing.T) {
+		require.NoError(t, unittest.PrepareTestDatabase())
+
+		_, err := WriteLogs(t.Context(), "test.log", 0,
+			[]*runnerv1.LogRow{{Time: timestamppb.New(time.Now()), Content: "Hello world"}})
+		require.NoError(t, err)
+
+		exist, err := ExistsLogs(t.Context(), "test.log")
+		require.NoError(t, err)
+
+		assert.True(t, exist)
+
+		err = RemoveLogs(t.Context(), false, "")
+		require.ErrorContains(t, err, "cannot remove logs because filename is empty")
+
+		exist, err = ExistsLogs(t.Context(), "test.log")
+		require.NoError(t, err)
+
+		assert.True(t, exist)
+	})
+}

modules/actions/main_test.go

@@ -0,0 +1,16 @@
+// Copyright 2026 The Forgejo Authors. All rights reserved.
+// SPDX-License-Identifier: GPL-3.0-or-later
+
+package actions_test
+
+import (
+	"testing"
+
+	"forgejo.org/models/unittest"
+
+	_ "forgejo.org/modules/testimport"
+)
+
+func TestMain(m *testing.M) {
+	unittest.MainTest(m)
+}

services/actions/cleanup.go

@@ -111,6 +111,10 @@ func CleanupLogs(ctx context.Context) error {
 			return fmt.Errorf("find old tasks: %w", err)
 		}
 		for _, task := range tasks {
+			if !task.HasLogs() {
+				continue
+			}
+
 			if err := actions_module.RemoveLogs(ctx, task.LogInStorage, task.LogFilename); err != nil && !errors.Is(err, os.ErrNotExist) {
 				log.Error("Failed to remove log %s (in storage %v) of task %v: %v", task.LogFilename, task.LogInStorage, task.ID, err)
 				// do not return error here, continue to next task

services/actions/cleanup_test.go

@@ -16,9 +16,9 @@ import (
 )
 
 func TestCleanup(t *testing.T) {
-	require.NoError(t, unittest.PrepareTestDatabase())
-
 	t.Run("Deletes no longer existing logs", func(t *testing.T) {
+		require.NoError(t, unittest.PrepareTestDatabase())
+
 		unittest.AssertSuccessfulInsert(t, &actions_model.ActionTask{ID: 1001, LogExpired: false, LogIndexes: []int64{1, 2, 3, 4}, LogFilename: "does-not-exist", Stopped: timeutil.TimeStamp(1)})
 
 		require.NoError(t, CleanupLogs(db.DefaultContext))
@@ -28,6 +28,19 @@ func TestCleanup(t *testing.T) {
 		assert.True(t, task.LogExpired)
 		assert.Nil(t, task.LogIndexes)
 	})
+
+	t.Run("Ignores tasks without logs", func(t *testing.T) {
+		require.NoError(t, unittest.PrepareTestDatabase())
+
+		unittest.AssertSuccessfulInsert(t, &actions_model.ActionTask{ID: 1001, LogExpired: false, LogIndexes: []int64{}, LogFilename: "", Stopped: timeutil.TimeStamp(1)})
+
+		require.NoError(t, CleanupLogs(db.DefaultContext))
+
+		task := unittest.AssertExistsAndLoadBean(t, &actions_model.ActionTask{ID: 1001})
+		assert.Empty(t, task.LogFilename)
+		assert.False(t, task.LogExpired)
+		assert.Nil(t, task.LogIndexes)
+	})
 }
 
 func TestCleanupEphemeralRunners(t *testing.T) {

services/repository/delete.go

@@ -392,6 +392,10 @@ func DeleteRepositoryDirectly(ctx context.Context, repoID int64, opts DeleteRepo
 
 	// Finally, delete action logs after the actions have already been deleted to avoid new log files
 	for _, task := range tasks {
+		if !task.HasLogs() {
+			continue
+		}
+
 		err := actions_module.RemoveLogs(ctx, task.LogInStorage, task.LogFilename)
 		if err != nil {
 			log.Error("remove log file %q: %v", task.LogFilename, err)