From ca34faee510d6b6d53aa40b806538b74e72c5ab0 Mon Sep 17 00:00:00 2001 From: Zach Handley Date: Wed, 27 May 2026 12:19:14 -0700 Subject: [PATCH] Drop run-level ?attempt from run-logs ZIP endpoint (run has no unified attempt; jobs re-run independently and diverge), keep ?attempt on the per-job endpoint where it's well-defined, simplify WriteRunLogsZip to always use job.Attempt for entry names, remove attempt=1/attempt=999 run-logs subtests, regen swagger --- routers/api/v1/repo/action.go | 26 +++----- services/actions/job_logs.go | 55 +++++----------- templates/swagger/v1_json.tmpl | 9 +-- .../integration/api_actions_run_logs_test.go | 62 ------------------- 4 files changed, 26 insertions(+), 126 deletions(-) diff --git a/routers/api/v1/repo/action.go b/routers/api/v1/repo/action.go index b858c17890..d01d6e6043 100644 --- a/routers/api/v1/repo/action.go +++ b/routers/api/v1/repo/action.go @@ -1592,18 +1592,16 @@ func GetActionRunLogs(ctx *context.APIContext) { // - name: run_id // in: path // description: > - // ID of the workflow run. The ZIP contains logs for the latest attempt - // of each job in the run, with each entry named `{job-name}-{job-id}.log` - // (the job ID prevents collisions when two jobs share a name). + // ID of the workflow run. The ZIP contains the latest attempt of each + // job in the run, with each entry named + // `{job-name}-{job-id}-attempt-{N}.log` (the job ID prevents collisions + // when two jobs share a name; the attempt number records which run the + // log came from). The run itself has no attempt number — jobs are + // re-run independently, so use the per-job logs endpoint with `?attempt` + // to fetch a specific historical attempt of one job. // 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 @@ -1634,20 +1632,12 @@ func GetActionRunLogs(ctx *context.APIContext) { 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=\"%s\"", zipFilename)) - if err := actions_service.WriteRunLogsZip(ctx, ctx.Resp, run, attempt); err != nil { + if err := actions_service.WriteRunLogsZip(ctx, ctx.Resp, run); err != nil { ctx.Error(http.StatusInternalServerError, "WriteRunLogsZip", err) return } diff --git a/services/actions/job_logs.go b/services/actions/job_logs.go index 4fc2cf8ac7..f47bb4c3fb 100644 --- a/services/actions/job_logs.go +++ b/services/actions/job_logs.go @@ -153,20 +153,19 @@ 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 { +// WriteRunLogsZip writes a ZIP of the latest per-job logs for the run to w. +// Each entry is named {job-name}-{job-id}-attempt-{N}.log, where N is that +// job's current attempt — the run itself has no attempt number, so jobs that +// were re-run independently show different attempts here. Jobs that haven't +// run, can't be looked up, or have expired logs get a .MISSING marker; 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) 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() @@ -185,15 +184,8 @@ func WriteRunLogsZip(ctx context.Context, w io.Writer, run *actions_model.Action return cleaned } - attemptFor := func(job *actions_model.ActionRunJob) int64 { - if hasAttempt { - return attemptVal - } - return job.Attempt - } - entryName := func(job *actions_model.ActionRunJob, suffix string) string { - return fmt.Sprintf("%s-%d-attempt-%d.%s", sanitize(job.Name), job.ID, attemptFor(job), suffix) + return fmt.Sprintf("%s-%d-attempt-%d.%s", sanitize(job.Name), job.ID, job.Attempt, suffix) } writeMarker := func(job *actions_model.ActionRunJob, suffix, msg string) { @@ -204,27 +196,14 @@ func WriteRunLogsZip(ctx context.Context, w io.Writer, run *actions_model.Action } 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 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 { diff --git a/templates/swagger/v1_json.tmpl b/templates/swagger/v1_json.tmpl index ccd3087473..728d8338b2 100644 --- a/templates/swagger/v1_json.tmpl +++ b/templates/swagger/v1_json.tmpl @@ -6506,17 +6506,10 @@ { "type": "integer", "format": "int64", - "description": "ID of the workflow run. The ZIP contains logs for the latest attempt of each job in the run, with each entry named `{job-name}-{job-id}.log` (the job ID prevents collisions when two jobs share a name).\n", + "description": "ID of the workflow run. The ZIP contains the latest attempt of each job in the run, with each entry named `{job-name}-{job-id}-attempt-{N}.log` (the job ID prevents collisions when two jobs share a name; the attempt number records which run the log came from). The run itself has no attempt number — jobs are re-run independently, so use the per-job logs endpoint with `?attempt` to fetch a specific historical attempt of one job.\n", "name": "run_id", "in": "path", "required": true - }, - { - "type": "integer", - "format": "int64", - "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`.", - "name": "attempt", - "in": "query" } ], "responses": { diff --git a/tests/integration/api_actions_run_logs_test.go b/tests/integration/api_actions_run_logs_test.go index e2a70d08db..f7810ff47b 100644 --- a/tests/integration/api_actions_run_logs_test.go +++ b/tests/integration/api_actions_run_logs_test.go @@ -10,7 +10,6 @@ import ( "io" "net/http" "net/url" - "strings" "testing" "time" @@ -192,67 +191,6 @@ 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,