forked from mirrors/forgejo
fix: ensure actions logs are transferred when a task is done (#10008)
Logs moving out of the database to the filesystem (actions_module.TransferLogsAndUpdateLogInStorage) did not happen in the following cases: - the runner does not send an UpdateLog message with NoMore == true - StopTask is called (canceling from the web.UI, canceling a scheduled task) This is fixed by consistently calling actions_service.TransferLogsAndUpdateLogInStorage when a task is completed by: - UpdateTaskByState if it concludes with Status.IsDone - StopTask Test coverage exists at: - TestActionsDownloadTaskLogs will fail if UpdateTaskByState does not call TransferLogsAndUpdateLogInStorage when when task.Status.IsDone() stat .../tests/integration/gitea-integration-sqlite/data/actions_log/user2/actions-download-task-logs/48/72.log.zst: no such file or directory - TestActionNowDoneNotification will fail if StopTask returns on error when calling TransferLogsAndUpdateLogInStorage Error Trace: .../tests/integration/actions_run_now_done_notification_test.go:142 Refs https://codeberg.org/forgejo/forgejo/issues/9999 --- Note on backporting: it cannot be easily backported to v11.0 because it would require a more involved backport to untangle circular dependencies. It is also not essential in the context of https://codeberg.org/forgejo/forgejo/issues/9999 for instances being polluted by logs that stay in the database. The new [cron job](https://codeberg.org/forgejo/forgejo/pulls/10009) that disposes of them will take care of those daily and they will not be growing the database indefinitely. <!--start release-notes-assistant--> ## Release notes <!--URL:https://codeberg.org/forgejo/forgejo--> - Bug fixes - [PR](https://codeberg.org/forgejo/forgejo/pulls/10008): <!--number 10008 --><!--line 0 --><!--description ZW5zdXJlIGFjdGlvbnMgbG9ncyBhcmUgdHJhbnNmZXJyZWQgd2hlbiBhIHRhc2sgaXMgZG9uZQ==-->ensure actions logs are transferred when a task is done<!--description--> <!--end release-notes-assistant--> Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/10008 Reviewed-by: Michael Kriese <michael.kriese@gmx.de> Co-authored-by: Earl Warren <contact@earl-warren.org> Co-committed-by: Earl Warren <contact@earl-warren.org>
This commit is contained in:
parent
8776ee2491
commit
d4951968f0
3 changed files with 8 additions and 27 deletions
|
|
@ -312,21 +312,9 @@ func (s *Service) UpdateLog(
|
|||
|
||||
res.Msg.AckIndex = task.LogLength
|
||||
|
||||
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 {
|
||||
if err := actions_model.UpdateTask(ctx, task, "log_indexes", "log_length", "log_size"); err != nil {
|
||||
return nil, connect.NewError(connect.CodeInternal, fmt.Errorf("update task: %w", err))
|
||||
}
|
||||
if remove != nil {
|
||||
remove()
|
||||
}
|
||||
|
||||
return res, nil
|
||||
}
|
||||
|
|
|
|||
|
|
@ -10,7 +10,6 @@ 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"
|
||||
|
|
@ -54,18 +53,6 @@ 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...)
|
||||
|
|
|
|||
|
|
@ -208,7 +208,7 @@ func StopTask(ctx context.Context, taskID int64, status actions_model.Status) er
|
|||
}
|
||||
}
|
||||
|
||||
return nil
|
||||
return TransferLogsAndUpdateLogInStorage(ctx, task)
|
||||
}
|
||||
|
||||
// UpdateTaskByState updates the task by the state.
|
||||
|
|
@ -264,6 +264,12 @@ 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
|
||||
}
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue