From 500fa81c9bf70f1ea2caf75f40956f9829877b97 Mon Sep 17 00:00:00 2001 From: Zach Handley Date: Tue, 26 May 2026 10:46:39 -0700 Subject: [PATCH] Extract WriteRunLogsZip to services/actions, thread ?attempt through run-logs endpoint with .MISSING markers for jobs lacking that attempt, replace silent break on zw.Create failure with .MISSING + continue, surface mid-stream io.Copy failures as .PARTIAL markers, add attempt=1/attempt=999 run-logs integration subtests --- routers/api/v1/repo/action.go | 87 ++++----------- services/actions/job_logs.go | 100 ++++++++++++++++++ .../integration/api_actions_run_logs_test.go | 62 +++++++++++ 3 files changed, 180 insertions(+), 69 deletions(-) diff --git a/routers/api/v1/repo/action.go b/routers/api/v1/repo/action.go index 9ee32be1e6..f3ac72591e 100644 --- a/routers/api/v1/repo/action.go +++ b/routers/api/v1/repo/action.go @@ -5,17 +5,13 @@ package repo import ( - "archive/zip" "errors" "fmt" - "io" "net/http" - "strings" actions_model "forgejo.org/models/actions" "forgejo.org/models/db" secret_model "forgejo.org/models/secret" - "forgejo.org/modules/actions" "forgejo.org/modules/optional" api "forgejo.org/modules/structs" "forgejo.org/modules/util" @@ -1602,6 +1598,12 @@ func GetActionRunLogs(ctx *context.APIContext) { // type: integer // format: int64 // required: true + // - name: attempt + // in: query + // description: 1-based attempt number; omit to fetch the latest attempt of each job. Jobs that don't have that attempt recorded contribute a `.MISSING` placeholder entry instead of a `.log`. + // type: integer + // format: int64 + // required: false // responses: // "200": // description: ZIP archive of per-job log files @@ -1632,74 +1634,21 @@ func GetActionRunLogs(ctx *context.APIContext) { return } - jobs, err := actions_model.GetRunJobsByRunID(ctx, run.ID) - if err != nil { - ctx.Error(http.StatusInternalServerError, "GetRunJobsByRunID", err) - return + var attempt optional.Option[int64] + if ctx.FormString("attempt") != "" { + attempt = optional.Some(ctx.FormInt64("attempt")) + } + + zipFilename := fmt.Sprintf("run-%d-logs.zip", run.ID) + if has, v := attempt.Get(); has { + zipFilename = fmt.Sprintf("run-%d-attempt-%d-logs.zip", run.ID, v) } ctx.Resp.Header().Set("Content-Type", "application/zip") - ctx.Resp.Header().Set("Content-Disposition", fmt.Sprintf("attachment; filename=\"run-%d-logs.zip\"", run.ID)) + ctx.Resp.Header().Set("Content-Disposition", fmt.Sprintf("attachment; filename=\"%s\"", zipFilename)) - zw := zip.NewWriter(ctx.Resp) - defer zw.Close() - - // sanitize strips only what's unsafe inside a ZIP entry name: ASCII - // control bytes and path separators. UTF-8 is preserved — APPNOTE 6.3.0 - // has supported it since 2006 and archive/zip sets general-purpose bit - // 11 automatically when needed. Disambiguation is the caller's job (we - // suffix with the unique job ID below). - sanitize := func(name string) string { - cleaned := strings.Map(func(r rune) rune { - if r < 0x20 || r == 0x7f || r == '/' || r == '\\' { - return -1 - } - return r - }, name) - cleaned = strings.TrimSpace(cleaned) - if cleaned == "" { - cleaned = "job" - } - return cleaned - } - - writePlaceholder := func(job *actions_model.ActionRunJob, msg string) { - w, werr := zw.Create(fmt.Sprintf("%s-%d.MISSING", sanitize(job.Name), job.ID)) - if werr == nil { - _, _ = w.Write([]byte(msg)) - } - } - - for _, job := range jobs { - if job.TaskID == 0 { - writePlaceholder(job, "job has not been executed yet\n") - continue - } - - task, err := actions_model.GetTaskByID(ctx, job.TaskID) - if err != nil { - writePlaceholder(job, fmt.Sprintf("task lookup failed: %v\n", err)) - continue - } - - if task.LogExpired { - writePlaceholder(job, "logs have been cleaned up\n") - continue - } - - reader, err := actions.OpenLogs(ctx, task.LogInStorage, task.LogFilename) - if err != nil { - writePlaceholder(job, fmt.Sprintf("log open failed: %v\n", err)) - continue - } - - w, err := zw.Create(fmt.Sprintf("%s-%d.log", sanitize(job.Name), job.ID)) - if err != nil { - reader.Close() - break - } - - _, _ = io.Copy(w, reader) - reader.Close() + if err := actions_service.WriteRunLogsZip(ctx, ctx.Resp, run, attempt); err != nil { + ctx.Error(http.StatusInternalServerError, "WriteRunLogsZip", err) + return } } diff --git a/services/actions/job_logs.go b/services/actions/job_logs.go index f535c3e0f9..f6e0745488 100644 --- a/services/actions/job_logs.go +++ b/services/actions/job_logs.go @@ -4,11 +4,13 @@ package actions import ( + "archive/zip" "bytes" "context" "errors" "fmt" "io" + "strings" "time" actions_model "forgejo.org/models/actions" @@ -159,3 +161,101 @@ func OpenJobLogReader( return nopReadSeekCloser{bytes.NewReader(buf)}, stepFilename, modtime, nil } + +// WriteRunLogsZip writes a ZIP of per-job logs for the run to w. When attempt +// is set, each job's log is taken from that historical attempt (jobs without +// that attempt get a .MISSING placeholder); when unset, the latest attempt is +// used. Jobs that haven't run, can't be looked up, or have expired logs get +// .MISSING; a mid-stream read failure gets a sibling .PARTIAL marker. Caller +// sets Content-Type / Content-Disposition before calling. +func WriteRunLogsZip(ctx context.Context, w io.Writer, run *actions_model.ActionRun, attempt optional.Option[int64]) error { + jobs, err := actions_model.GetRunJobsByRunID(ctx, run.ID) + if err != nil { + return fmt.Errorf("get jobs for run %d: %w", run.ID, err) + } + + hasAttempt, attemptVal := attempt.Get() + + zw := zip.NewWriter(w) + defer zw.Close() + + // strip control bytes and path separators; UTF-8 passes through. + sanitize := func(name string) string { + cleaned := strings.Map(func(r rune) rune { + if r < 0x20 || r == 0x7f || r == '/' || r == '\\' { + return -1 + } + return r + }, name) + cleaned = strings.TrimSpace(cleaned) + if cleaned == "" { + cleaned = "job" + } + return cleaned + } + + entryName := func(job *actions_model.ActionRunJob, suffix string) string { + if hasAttempt { + return fmt.Sprintf("%s-%d-attempt-%d.%s", sanitize(job.Name), job.ID, attemptVal, suffix) + } + return fmt.Sprintf("%s-%d.%s", sanitize(job.Name), job.ID, suffix) + } + + writeMarker := func(job *actions_model.ActionRunJob, suffix, msg string) { + entry, werr := zw.Create(entryName(job, suffix)) + if werr == nil { + _, _ = entry.Write([]byte(msg)) + } + } + + for _, job := range jobs { + var task *actions_model.ActionTask + if hasAttempt { + task, err = actions_model.GetTaskByJobAttempt(ctx, job.ID, attemptVal) + if err != nil { + if errors.Is(err, util.ErrNotExist) { + writeMarker(job, "MISSING", fmt.Sprintf("no attempt %d recorded for this job\n", attemptVal)) + } else { + writeMarker(job, "MISSING", fmt.Sprintf("task lookup failed: %v\n", err)) + } + continue + } + } else { + if job.TaskID == 0 { + writeMarker(job, "MISSING", "job has not been executed yet\n") + continue + } + task, err = actions_model.GetTaskByID(ctx, job.TaskID) + if err != nil { + writeMarker(job, "MISSING", fmt.Sprintf("task lookup failed: %v\n", err)) + continue + } + } + + if task.LogExpired { + writeMarker(job, "MISSING", "logs have been cleaned up\n") + continue + } + + reader, err := actions.OpenLogs(ctx, task.LogInStorage, task.LogFilename) + if err != nil { + writeMarker(job, "MISSING", fmt.Sprintf("log open failed: %v\n", err)) + continue + } + + entry, err := zw.Create(entryName(job, "log")) + if err != nil { + reader.Close() + writeMarker(job, "MISSING", fmt.Sprintf("zip entry create failed: %v\n", err)) + continue + } + + if _, copyErr := io.Copy(entry, reader); copyErr != nil { + reader.Close() + writeMarker(job, "PARTIAL", fmt.Sprintf("log read failed mid-stream: %v\n", copyErr)) + continue + } + reader.Close() + } + return nil +} diff --git a/tests/integration/api_actions_run_logs_test.go b/tests/integration/api_actions_run_logs_test.go index c9dc865e4c..1c7ff6f257 100644 --- a/tests/integration/api_actions_run_logs_test.go +++ b/tests/integration/api_actions_run_logs_test.go @@ -10,6 +10,7 @@ import ( "io" "net/http" "net/url" + "strings" "testing" "time" @@ -191,6 +192,67 @@ jobs: MakeRequest(t, req, http.StatusNotFound) }) + t.Run("attempt=1: 200 same content, attempt-suffixed entry names", func(t *testing.T) { + req := NewRequestf(t, "GET", + "/api/v1/repos/%s/actions/runs/%d/logs?attempt=1", + repoA.FullName(), runID, + ) + req.AddTokenAuth(token) + resp := MakeRequest(t, req, http.StatusOK) + + assert.Contains(t, resp.Header().Get("Content-Disposition"), + fmt.Sprintf("run-%d-attempt-1-logs.zip", runID)) + + r, err := zip.NewReader(bytes.NewReader(resp.Body.Bytes()), int64(resp.Body.Len())) + require.NoError(t, err) + + entries := map[string]string{} + for _, f := range r.File { + fr, err := f.Open() + require.NoError(t, err) + data, err := io.ReadAll(fr) + require.NoError(t, err) + require.NoError(t, fr.Close()) + entries[f.Name] = string(data) + } + + job1Name := fmt.Sprintf("%s-%d-attempt-1.log", actionRunJob1.Name, actionRunJob1.ID) + job2Name := fmt.Sprintf("%s-%d-attempt-1.log", actionRunJob2.Name, actionRunJob2.ID) + utf8Name := fmt.Sprintf("%s-%d-attempt-1.log", actionRunJob3.Name, actionRunJob3.ID) + + require.Len(t, entries, 3) + require.Contains(t, entries, job1Name) + require.Contains(t, entries, job2Name) + require.Contains(t, entries, utf8Name) + assert.Contains(t, entries[job1Name], "job1-output line one") + assert.Contains(t, entries[job2Name], "job2-output line one") + assert.Contains(t, entries[utf8Name], "utf8-output line one") + }) + + t.Run("attempt=999: 200 with all .MISSING entries", func(t *testing.T) { + req := NewRequestf(t, "GET", + "/api/v1/repos/%s/actions/runs/%d/logs?attempt=999", + repoA.FullName(), runID, + ) + req.AddTokenAuth(token) + resp := MakeRequest(t, req, http.StatusOK) + + r, err := zip.NewReader(bytes.NewReader(resp.Body.Bytes()), int64(resp.Body.Len())) + require.NoError(t, err) + + require.Len(t, r.File, 3, "every job should contribute one .MISSING marker") + for _, f := range r.File { + assert.True(t, strings.HasSuffix(f.Name, "-attempt-999.MISSING"), + "every entry should be an attempt-999 marker; got %q", f.Name) + fr, err := f.Open() + require.NoError(t, err) + data, err := io.ReadAll(fr) + require.NoError(t, err) + require.NoError(t, fr.Close()) + assert.Contains(t, string(data), "no attempt 999 recorded") + } + }) + t.Run("wrong scope: 403 without read:repository", func(t *testing.T) { // Token with only user scope, no repository access. weakToken := getTokenForLoggedInUser(t, session,