From 6bac9e29e78feea37f2bf3db2e7b0cbdf8a85cd4 Mon Sep 17 00:00:00 2001 From: Mathieu Fenniak Date: Mon, 2 Mar 2026 15:34:09 +0100 Subject: [PATCH] Revert "fix: ensure actions logs are transferred when a task is done (#10008)" (#11462) This reverts commit d4951968f0abbee8c614acec648240c4b25e7ec7, #10008. When Forgejo cancels a job server-side, for example due to an additional push to an open PR, it immediately archives the logs from DBFS to disk due to the changes in #10008. Then, the runner recognizes that the job status is cancelled and it attempts to flush its pending logs to Forgejo, resulting in warnings being logged: ``` forgejo-runner.log:time="2026-02-23T01:32:11+01:00" level=warning msg="uploading final logs failed, but will be retried: already_exists: log file has been archived" task_id=51 forgejo-runner.log:time="2026-02-23T01:32:11+01:00" level=warning msg="uploading final logs failed, but will be retried: already_exists: log file has been archived" task_id=51 forgejo-runner.log:time="2026-02-23T01:32:11+01:00" level=warning msg="uploading final logs failed, but will be retried: already_exists: log file has been archived" task_id=51 forgejo-runner.log:time="2026-02-23T01:32:12+01:00" level=warning msg="uploading final logs failed, but will be retried: already_exists: log file has been archived" task_id=51 forgejo-runner.log:time="2026-02-23T01:32:13+01:00" level=warning msg="uploading final logs failed, but will be retried: already_exists: log file has been archived" task_id=51 forgejo-runner.log:time="2026-02-23T01:32:14+01:00" level=warning msg="uploading final logs failed, but will be retried: already_exists: log file has been archived" task_id=51 forgejo-runner.log:time="2026-02-23T01:32:16+01:00" level=info msg="runner: received shutdown signal" forgejo-runner.log:time="2026-02-23T01:32:16+01:00" level=info msg="runner: shutdown initiated, waiting [runner].shutdown_timeout=0s for running jobs to complete before shutting down" forgejo-runner.log:time="2026-02-23T01:32:16+01:00" level=info msg="[poller] shutdown begin, 1 tasks currently running" forgejo-runner.log:time="2026-02-23T01:32:16+01:00" level=info msg="forcing the jobs to shutdown" forgejo-runner.log:time="2026-02-23T01:32:18+01:00" level=warning msg="uploading final logs failed, but will be retried: already_exists: log file has been archived" task_id=51 forgejo-runner.log:time="2026-02-23T01:32:24+01:00" level=warning msg="uploading final logs failed, but will be retried: already_exists: log file has been archived" task_id=51 ``` This appears to be the cause of the `push-cancel` end-to-end test failing since #10008 was merged. https://code.forgejo.org/forgejo/end-to-end/actions/runs/4985/jobs/8/attempt/1 The `push-cancel` test case itself seems to succeed, but then the test process aborts with `return 1`. Doesn't reproduce locally. Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/11462 Reviewed-by: Michael Kriese Co-authored-by: Mathieu Fenniak Co-committed-by: Mathieu Fenniak --- routers/api/actions/runner/runner.go | 14 +++++++++++++- services/actions/clear_tasks.go | 13 +++++++++++++ services/actions/task.go | 8 +------- 3 files changed, 27 insertions(+), 8 deletions(-) diff --git a/routers/api/actions/runner/runner.go b/routers/api/actions/runner/runner.go index e079aa8de8..5a29dd7ab7 100644 --- a/routers/api/actions/runner/runner.go +++ b/routers/api/actions/runner/runner.go @@ -336,9 +336,21 @@ func (s *Service) UpdateLog( res.Msg.AckIndex = task.LogLength - if err := actions_model.UpdateTask(ctx, task, "log_indexes", "log_length", "log_size"); err != nil { + var remove func() + if req.Msg.NoMore { + task.LogInStorage = true + remove, err = actions.TransferLogs(ctx, task.LogFilename) + if err != nil { + return nil, connect.NewError(connect.CodeInternal, fmt.Errorf("transfer logs: %w", err)) + } + } + + if err := actions_model.UpdateTask(ctx, task, "log_indexes", "log_length", "log_size", "log_in_storage"); err != nil { return nil, connect.NewError(connect.CodeInternal, fmt.Errorf("update task: %w", err)) } + if remove != nil { + remove() + } return res, nil } diff --git a/services/actions/clear_tasks.go b/services/actions/clear_tasks.go index d597ad313b..507659d085 100644 --- a/services/actions/clear_tasks.go +++ b/services/actions/clear_tasks.go @@ -10,6 +10,7 @@ import ( actions_model "forgejo.org/models/actions" "forgejo.org/models/db" + "forgejo.org/modules/actions" "forgejo.org/modules/log" "forgejo.org/modules/optional" "forgejo.org/modules/setting" @@ -53,6 +54,18 @@ func stopTasks(ctx context.Context, opts actions_model.FindTaskOptions) error { log.Warn("Cannot stop task %v: %v", task.ID, err) continue } + + remove, err := actions.TransferLogs(ctx, task.LogFilename) + if err != nil { + log.Warn("Cannot transfer logs of task %v: %v", task.ID, err) + continue + } + task.LogInStorage = true + if err := actions_model.UpdateTask(ctx, task, "log_in_storage"); err != nil { + log.Warn("Cannot update task %v: %v", task.ID, err) + continue + } + remove() } CreateCommitStatus(ctx, jobs...) diff --git a/services/actions/task.go b/services/actions/task.go index 141678549c..4f78092bdb 100644 --- a/services/actions/task.go +++ b/services/actions/task.go @@ -263,7 +263,7 @@ func StopTask(ctx context.Context, taskID int64, status actions_model.Status) er } } - return TransferLogsAndUpdateLogInStorage(ctx, task) + return nil } // UpdateTaskByState updates the task by the state. @@ -319,12 +319,6 @@ func UpdateTaskByState(ctx context.Context, runnerID int64, state *runnerv1.Task } } - if task.Status.IsDone() { - if err := TransferLogsAndUpdateLogInStorage(ctx, task); err != nil { - return nil, err - } - } - if err := task.LoadAttributes(ctx); err != nil { return nil, err }