From 60e491663a594efe07551ab0ad37d6453042a0c9 Mon Sep 17 00:00:00 2001 From: Nicolas Date: Wed, 17 Jun 2026 17:09:48 +0200 Subject: [PATCH] cleanup --- models/actions/run_list.go | 9 +- routers/api/v1/repo/action.go | 17 +- routers/api/v1/repo/actions_run.go | 38 +---- routers/api/v1/shared/action.go | 7 +- routers/common/actions.go | 190 +++++++++++++--------- tests/integration/api_actions_run_test.go | 44 +++-- 6 files changed, 172 insertions(+), 133 deletions(-) diff --git a/models/actions/run_list.go b/models/actions/run_list.go index 88f3d3dd82..0e61279e7d 100644 --- a/models/actions/run_list.go +++ b/models/actions/run_list.go @@ -98,7 +98,14 @@ func (opts FindRunOptions) ToConds() builder.Cond { func (opts FindRunOptions) ToJoins() []db.JoinFunc { if opts.OwnerID > 0 { return []db.JoinFunc{func(sess db.Engine) error { - sess.Join("INNER", "repository", "repository.id = repo_id AND repository.owner_id = ?", opts.OwnerID) + sess.Join("INNER", "repository", "repository.id = action_run.repo_id AND repository.owner_id = ?", opts.OwnerID) + return nil + }} + } + if opts.RepoID == 0 { + // Exclude runs whose repository has been deleted. + return []db.JoinFunc{func(sess db.Engine) error { + sess.Join("INNER", "repository", "repository.id = action_run.repo_id") return nil }} } diff --git a/routers/api/v1/repo/action.go b/routers/api/v1/repo/action.go index 3b920ac551..35c0530029 100644 --- a/routers/api/v1/repo/action.go +++ b/routers/api/v1/repo/action.go @@ -1298,6 +1298,16 @@ func getCurrentRepoActionRunAttemptByNumber(ctx *context.APIContext) (*actions_m return run, attempt } +func respondRepoActionWorkflowRun(ctx *context.APIContext, run *actions_model.ActionRun) { + run.Repo = ctx.Repo.Repository + convertedRun, err := convert.ToActionWorkflowRun(ctx, run, nil, false) + if err != nil { + ctx.APIErrorInternal(err) + return + } + ctx.JSON(http.StatusOK, convertedRun) +} + // GetWorkflowRun Gets a specific workflow run. func GetWorkflowRun(ctx *context.APIContext) { // swagger:operation GET /repos/{owner}/{repo}/actions/runs/{run} repository GetWorkflowRun @@ -1334,12 +1344,7 @@ func GetWorkflowRun(ctx *context.APIContext) { return } - convertedRun, err := convert.ToActionWorkflowRun(ctx, run, nil, false) - if err != nil { - ctx.APIErrorInternal(err) - return - } - ctx.JSON(http.StatusOK, convertedRun) + respondRepoActionWorkflowRun(ctx, run) } // GetWorkflowRunAttempt Gets a specific workflow run attempt. diff --git a/routers/api/v1/repo/actions_run.go b/routers/api/v1/repo/actions_run.go index 49f700b5ff..8e666578d8 100644 --- a/routers/api/v1/repo/actions_run.go +++ b/routers/api/v1/repo/actions_run.go @@ -4,14 +4,10 @@ package repo import ( - "net/http" - actions_model "gitea.dev/models/actions" - "gitea.dev/models/db" "gitea.dev/routers/common" actions_service "gitea.dev/services/actions" "gitea.dev/services/context" - "gitea.dev/services/convert" ) func DownloadActionsRunJobLogs(ctx *context.APIContext) { @@ -50,11 +46,6 @@ func DownloadActionsRunJobLogs(ctx *context.APIContext) { ctx.APIErrorAuto(err) return } - if err = curJob.LoadRepo(ctx); err != nil { - ctx.APIErrorInternal(err) - return - } - err = common.DownloadActionsRunJobLogs(ctx.Base, ctx.Repo.Repository, curJob) if err != nil { ctx.APIErrorAuto(err) @@ -99,18 +90,15 @@ func CancelWorkflowRun(ctx *context.APIContext) { } if err := actions_service.CancelRun(ctx, run, jobs); err != nil { - ctx.APIErrorInternal(err) + ctx.APIErrorAuto(err) return } - updatedRun, has, err := db.GetByID[actions_model.ActionRun](ctx, run.ID) - if err != nil || !has { - ctx.APIErrorInternal(err) + run = getCurrentRepoActionRunByID(ctx) + if ctx.Written() { return } - - updatedRun.Repo = ctx.Repo.Repository - respondActionWorkflowRun(ctx, updatedRun) + respondRepoActionWorkflowRun(ctx, run) } func ApproveWorkflowRun(ctx *context.APIContext) { @@ -152,7 +140,7 @@ func ApproveWorkflowRun(ctx *context.APIContext) { // GitHub-compatible: return 200 if already approved (idempotent) if !run.NeedApproval { - respondActionWorkflowRun(ctx, run) + respondRepoActionWorkflowRun(ctx, run) return } @@ -161,21 +149,11 @@ func ApproveWorkflowRun(ctx *context.APIContext) { return } - // Note: the overall run status is updated asynchronously by the notifier, - // so the status field may still reflect the pre-approval state. - run.NeedApproval = false - run.ApprovedBy = ctx.Doer.ID - respondActionWorkflowRun(ctx, run) -} - -func respondActionWorkflowRun(ctx *context.APIContext, run *actions_model.ActionRun) { - run.Repo = ctx.Repo.Repository - convertedRun, err := convert.ToActionWorkflowRun(ctx, run, nil, false) - if err != nil { - ctx.APIErrorInternal(err) + run = getCurrentRepoActionRunByID(ctx) + if ctx.Written() { return } - ctx.JSON(http.StatusOK, convertedRun) + respondRepoActionWorkflowRun(ctx, run) } func GetWorkflowRunLogs(ctx *context.APIContext) { diff --git a/routers/api/v1/shared/action.go b/routers/api/v1/shared/action.go index ded1bd4948..d62d0d3a22 100644 --- a/routers/api/v1/shared/action.go +++ b/routers/api/v1/shared/action.go @@ -203,18 +203,15 @@ func ListRuns(ctx *context.APIContext, ownerID, repoID int64, workflowID string) return } - res.Entries = make([]*api.ActionWorkflowRun, 0, len(runs)) + res.Entries = make([]*api.ActionWorkflowRun, len(runs)) for i := range runs { - if runs[i].Repo == nil { - continue // skip runs whose repository has been deleted - } // TODO: load run attempts in batch convertedRun, err := convert.ToActionWorkflowRun(ctx, runs[i], nil, excludePullRequests) if err != nil { ctx.APIErrorInternal(err) return } - res.Entries = append(res.Entries, convertedRun) + res.Entries[i] = convertedRun } ctx.SetLinkHeader(total, listOptions.PageSize) ctx.SetTotalCountHeader(total) diff --git a/routers/common/actions.go b/routers/common/actions.go index b7ec8a5a38..02ffc2d129 100644 --- a/routers/common/actions.go +++ b/routers/common/actions.go @@ -5,6 +5,7 @@ package common import ( "archive/zip" + "context" "errors" "fmt" "io" @@ -17,28 +18,99 @@ import ( "gitea.dev/modules/httplib" "gitea.dev/modules/log" "gitea.dev/modules/util" - "gitea.dev/services/context" + context_module "gitea.dev/services/context" ) -func actionsWorkflowBaseName(workflowID string) string { +var ( + workflowNameReplacer = strings.NewReplacer(`"`, "", "\r", "", "\n", "", "/", "-", `\`, "-") + jobNameReplacer = strings.NewReplacer("/", "-", `\`, "-", "..", "__") +) + +func sanitizeWorkflowFileName(workflowID string) string { if p := strings.Index(workflowID, "."); p > 0 { - return workflowID[:p] + workflowID = workflowID[:p] } - return workflowID + return workflowNameReplacer.Replace(workflowID) } -func DownloadActionsRunJobLogsWithID(ctx *context.Base, ctxRepo *repo_model.Repository, runID, jobID int64) error { +func sanitizeJobFileName(name string) string { + return jobNameReplacer.Replace(name) +} + +func jobLogFileName(workflowID, jobName string, taskID int64) string { + return fmt.Sprintf("%s-%s-%d.log", sanitizeWorkflowFileName(workflowID), sanitizeJobFileName(jobName), taskID) +} + +func resolveJobLogTask(ctx context.Context, job *actions_model.ActionRunJob) (*actions_model.ActionTask, error) { + taskID := job.EffectiveTaskID() + if taskID == 0 { + return nil, util.NewNotExistErrorf("job not started") + } + + task, err := actions_model.GetTaskByID(ctx, taskID) + if err != nil { + return nil, fmt.Errorf("GetTaskByID: %w", err) + } + + if task.LogExpired { + return nil, util.NewNotExistErrorf("logs have been cleaned up") + } + if task.LogLength == 0 { + return nil, util.NewNotExistErrorf("logs not found") + } + return task, nil +} + +func openTaskLogs(ctx context.Context, task *actions_model.ActionTask) (io.ReadSeekCloser, error) { + reader, err := actions.OpenLogs(ctx, task.LogInStorage, task.LogFilename) + if err != nil { + if errors.Is(err, fs.ErrNotExist) || errors.Is(err, util.ErrNotExist) { + return nil, util.NewNotExistErrorf("logs not found") + } + return nil, fmt.Errorf("OpenLogs: %w", err) + } + return reader, nil +} + +func openJobTaskLogs(ctx context.Context, job *actions_model.ActionRunJob) (io.ReadSeekCloser, *actions_model.ActionTask, error) { + task, err := resolveJobLogTask(ctx, job) + if err != nil { + return nil, nil, err + } + + reader, err := openTaskLogs(ctx, task) + if err != nil { + return nil, nil, err + } + return reader, task, nil +} + +func appendJobLogToZip(ctx context.Context, zipWriter *zip.Writer, workflowID string, job *actions_model.ActionRunJob, task *actions_model.ActionTask) error { + reader, err := openTaskLogs(ctx, task) + if err != nil { + return err + } + defer reader.Close() + + zipFile, err := zipWriter.Create(jobLogFileName(workflowID, job.Name, task.ID)) + if err != nil { + return fmt.Errorf("Create zip entry for job %d: %w", job.ID, err) + } + if _, err = io.Copy(zipFile, reader); err != nil { + return fmt.Errorf("Write job %d logs to zip: %w", job.ID, err) + } + return nil +} + +func DownloadActionsRunJobLogsWithID(ctx *context_module.Base, ctxRepo *repo_model.Repository, runID, jobID int64) error { job, err := actions_model.GetRunJobByRunAndID(ctx, runID, jobID) if err != nil { return err } - if err := job.LoadRepo(ctx); err != nil { - return fmt.Errorf("LoadRepo: %w", err) - } return DownloadActionsRunJobLogs(ctx, ctxRepo, job) } -func DownloadActionsRunAllJobLogs(ctx *context.Base, ctxRepo *repo_model.Repository, runID int64) error { +func DownloadActionsRunAllJobLogs(ctx *context_module.Base, ctxRepo *repo_model.Repository, runID int64) error { runJobs, err := actions_model.GetLatestAttemptJobsByRepoAndRunID(ctx, ctxRepo.ID, runID) if err != nil { return fmt.Errorf("GetLatestAttemptJobsByRepoAndRunID: %w", err) @@ -48,100 +120,66 @@ func DownloadActionsRunAllJobLogs(ctx *context.Base, ctxRepo *repo_model.Reposit return util.NewNotExistErrorf("no jobs found for run %d", runID) } - // Load run for workflow name if err := runJobs[0].LoadRun(ctx); err != nil { return fmt.Errorf("LoadRun: %w", err) } + workflowID := runJobs[0].Run.WorkflowID - workflowName := actionsWorkflowBaseName(runJobs[0].Run.WorkflowID) - safeWorkflowName := strings.NewReplacer(`"`, "", "\r", "", "\n", "", "/", "-", `\`, "-").Replace(workflowName) + type jobLogEntry struct { + job *actions_model.ActionRunJob + task *actions_model.ActionTask + } + logEntries := make([]jobLogEntry, 0, len(runJobs)) + for _, job := range runJobs { + task, err := resolveJobLogTask(ctx, job) + if err != nil { + if errors.Is(err, util.ErrNotExist) { + continue + } + return err + } + logEntries = append(logEntries, jobLogEntry{job: job, task: task}) + } + if len(logEntries) == 0 { + return util.NewNotExistErrorf("logs not found") + } ctx.Resp.Header().Set("Content-Type", "application/zip") - ctx.Resp.Header().Set("Content-Disposition", httplib.EncodeContentDispositionAttachment(fmt.Sprintf("%s-run-%d-logs.zip", safeWorkflowName, runID))) + ctx.Resp.Header().Set("Content-Disposition", httplib.EncodeContentDispositionAttachment( + fmt.Sprintf("%s-run-%d-logs.zip", sanitizeWorkflowFileName(workflowID), runID), + )) zipWriter := zip.NewWriter(ctx.Resp) defer zipWriter.Close() - jobNameReplacer := strings.NewReplacer("/", "-", `\`, "-", "..", "__") - for _, job := range runJobs { - taskID := job.EffectiveTaskID() - if taskID == 0 { - continue // Skip jobs that haven't started - } - - task, err := actions_model.GetTaskByID(ctx, taskID) - if err != nil { - return fmt.Errorf("GetTaskByID for job %d: %w", job.ID, err) - } - - if task.LogExpired || task.LogLength == 0 { + // Best-effort: the response headers and zip stream are already committed, so a + // failure to read one job's logs must not abort the whole archive. Log and skip. + for _, entry := range logEntries { + if err := appendJobLogToZip(ctx, zipWriter, workflowID, entry.job, entry.task); err != nil { + log.Error("Failed to add logs for job %d to zip: %v", entry.job.ID, err) continue } - - safeJobName := jobNameReplacer.Replace(job.Name) - fileName := fmt.Sprintf("%s-%s-%d.log", safeWorkflowName, safeJobName, task.ID) - - reader, err := actions.OpenLogs(ctx, task.LogInStorage, task.LogFilename) - if err != nil { - log.Error("Failed to open logs for job %d: %v", job.ID, err) - continue - } - - zipFile, err := zipWriter.Create(fileName) - if err != nil { - reader.Close() - log.Error("Failed to add logs for job %d to zip: %v", job.ID, err) - continue - } - - if _, err = io.Copy(zipFile, reader); err != nil { - log.Error("Failed to add logs for job %d to zip: %v", job.ID, err) - } - reader.Close() } - return nil } -func DownloadActionsRunJobLogs(ctx *context.Base, ctxRepo *repo_model.Repository, curJob *actions_model.ActionRunJob) error { - if curJob.Repo.ID != ctxRepo.ID { +func DownloadActionsRunJobLogs(ctx *context_module.Base, ctxRepo *repo_model.Repository, curJob *actions_model.ActionRunJob) error { + if curJob.RepoID != ctxRepo.ID { return util.NewNotExistErrorf("job not found") } - taskID := curJob.EffectiveTaskID() - if taskID == 0 { - return util.NewNotExistErrorf("job not started") - } - if err := curJob.LoadRun(ctx); err != nil { return fmt.Errorf("LoadRun: %w", err) } - task, err := actions_model.GetTaskByID(ctx, taskID) + reader, task, err := openJobTaskLogs(ctx, curJob) if err != nil { - return fmt.Errorf("GetTaskByID: %w", err) - } - - if task.LogExpired { - return util.NewNotExistErrorf("logs have been cleaned up") - } - - if task.LogLength == 0 { - return util.NewNotExistErrorf("logs not found") - } - - reader, err := actions.OpenLogs(ctx, task.LogInStorage, task.LogFilename) - if err != nil { - if errors.Is(err, fs.ErrNotExist) || errors.Is(err, util.ErrNotExist) { - return util.NewNotExistErrorf("logs not found") - } - return fmt.Errorf("OpenLogs: %w", err) + return err } defer reader.Close() - workflowName := actionsWorkflowBaseName(curJob.Run.WorkflowID) - ctx.ServeContent(reader, context.ServeHeaderOptions{ - Filename: fmt.Sprintf("%v-%v-%v.log", workflowName, curJob.Name, task.ID), + ctx.ServeContent(reader, context_module.ServeHeaderOptions{ + Filename: jobLogFileName(curJob.Run.WorkflowID, curJob.Name, task.ID), ContentLength: &task.LogSize, ContentType: "text/plain; charset=utf-8", ContentDisposition: httplib.ContentDispositionAttachment, diff --git a/tests/integration/api_actions_run_test.go b/tests/integration/api_actions_run_test.go index 1a0250dde6..3d7a66ea5b 100644 --- a/tests/integration/api_actions_run_test.go +++ b/tests/integration/api_actions_run_test.go @@ -482,6 +482,30 @@ func TestAPIActionsListUserWorkflows(t *testing.T) { assert.NotEmpty(t, job.HTMLURL, "html_url should be populated via batch-loaded repo") } }) + + t.Run("JobsDefaultOrderAsc", func(t *testing.T) { + req := NewRequest(t, "GET", "/api/v1/user/actions/jobs").AddTokenAuth(token) + resp := MakeRequest(t, req, http.StatusOK) + jobs := DecodeJSON(t, resp, &api.ActionWorkflowJobsResponse{}) + + assert.GreaterOrEqual(t, len(jobs.Entries), 2, "need at least 2 jobs to verify ordering") + for i := 1; i < len(jobs.Entries); i++ { + assert.Less(t, jobs.Entries[i-1].ID, jobs.Entries[i].ID, + "jobs should be ordered by ID ascending by default") + } + }) + + t.Run("JobsOrderedByIDDesc", func(t *testing.T) { + req := NewRequest(t, "GET", "/api/v1/user/actions/jobs?sort=id&order=desc").AddTokenAuth(token) + resp := MakeRequest(t, req, http.StatusOK) + jobs := DecodeJSON(t, resp, &api.ActionWorkflowJobsResponse{}) + + assert.GreaterOrEqual(t, len(jobs.Entries), 2, "need at least 2 jobs to verify ordering") + for i := 1; i < len(jobs.Entries); i++ { + assert.Greater(t, jobs.Entries[i-1].ID, jobs.Entries[i].ID, + "jobs should be ordered by ID descending") + } + }) } func TestAPIActionsListRepoWorkflows(t *testing.T) { @@ -514,31 +538,21 @@ func TestAPIActionsGetWorkflowRunLogs(t *testing.T) { session := loginUser(t, user.Name) token := getTokenForLoggedInUser(t, session, auth_model.AccessTokenScopeWriteRepository) - t.Run("Success", func(t *testing.T) { + t.Run("NoLogs", func(t *testing.T) { + // Run 795 has jobs but fixture tasks have no log output in storage. req := NewRequest(t, "GET", fmt.Sprintf("/api/v1/repos/%s/actions/runs/795/logs", repo.FullName())). AddTokenAuth(token) - resp := MakeRequest(t, req, http.StatusOK) - assert.Equal(t, "application/zip", resp.Header().Get("Content-Type")) - assert.Contains(t, resp.Header().Get("Content-Disposition"), "attachment") - assert.Contains(t, resp.Header().Get("Content-Disposition"), ".zip") - body := resp.Body.Bytes() - require.NotEmpty(t, body) - assert.Equal(t, "PK", string(body[:2]), "response should be a valid zip file") + MakeRequest(t, req, http.StatusNotFound) }) - t.Run("RerunJobLogs", func(t *testing.T) { - // Rerun the workflow so latest-attempt jobs have SourceTaskID instead of TaskID + t.Run("NoLogsAfterRerun", func(t *testing.T) { req := NewRequest(t, "POST", fmt.Sprintf("/api/v1/repos/%s/actions/runs/795/rerun", repo.FullName())). AddTokenAuth(token) MakeRequest(t, req, http.StatusCreated) - // Download logs for the latest attempt — should include rerun job logs via EffectiveTaskID req = NewRequest(t, "GET", fmt.Sprintf("/api/v1/repos/%s/actions/runs/795/logs", repo.FullName())). AddTokenAuth(token) - resp := MakeRequest(t, req, http.StatusOK) - body := resp.Body.Bytes() - require.NotEmpty(t, body) - assert.Equal(t, "PK", string(body[:2]), "response should be a valid zip file") + MakeRequest(t, req, http.StatusNotFound) }) t.Run("NotFound", func(t *testing.T) {