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

@@ -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
 	}
 }

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
+}

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,