Fix bug on downloading job logs #34041

lunny · 2025-03-27T19:32:18Z

ChristopherHX

Confirmed the defect and that this resolves the problem locally.

Could we test this scenario in a unit test as well?

ChristopherHX · 2025-03-27T21:35:27Z

I suggest to update the test to the following, this bails on main and passes here

diff --git a/tests/integration/actions_log_test.go b/tests/integration/actions_log_test.go
index a157a923f6..cd20604b84 100644
--- a/tests/integration/actions_log_test.go
+++ b/tests/integration/actions_log_test.go
@@ -31,7 +31,7 @@ func TestDownloadTaskLogs(t *testing.T) {
 	testCases := []struct {
 		treePath    string
 		fileContent string
-		outcome     *mockTaskOutcome
+		outcome     []*mockTaskOutcome
 		zstdEnabled bool
 	}{
 		{
@@ -46,21 +46,44 @@ jobs:
       runs-on: ubuntu-latest
       steps:
         - run: echo job1 with zstd enabled
+    job2:
+      runs-on: ubuntu-latest
+      steps:
+        - run: echo job2 with zstd enabled
 `,
-			outcome: &mockTaskOutcome{
-				result: runnerv1.Result_RESULT_SUCCESS,
-				logRows: []*runnerv1.LogRow{
-					{
-						Time:    timestamppb.New(now.Add(1 * time.Second)),
-						Content: "  \U0001F433  docker create image",
-					},
-					{
-						Time:    timestamppb.New(now.Add(2 * time.Second)),
-						Content: "job1 zstd enabled",
+			outcome: []*mockTaskOutcome{
+				{
+					result: runnerv1.Result_RESULT_SUCCESS,
+					logRows: []*runnerv1.LogRow{
+						{
+							Time:    timestamppb.New(now.Add(1 * time.Second)),
+							Content: "  \U0001F433  docker create image",
+						},
+						{
+							Time:    timestamppb.New(now.Add(2 * time.Second)),
+							Content: "job1 zstd enabled",
+						},
+						{
+							Time:    timestamppb.New(now.Add(3 * time.Second)),
+							Content: "\U0001F3C1  Job succeeded",
+						},
 					},
-					{
-						Time:    timestamppb.New(now.Add(3 * time.Second)),
-						Content: "\U0001F3C1  Job succeeded",
+				},
+				{
+					result: runnerv1.Result_RESULT_SUCCESS,
+					logRows: []*runnerv1.LogRow{
+						{
+							Time:    timestamppb.New(now.Add(1 * time.Second)),
+							Content: "  \U0001F433  docker create image",
+						},
+						{
+							Time:    timestamppb.New(now.Add(2 * time.Second)),
+							Content: "job2 zstd enabled",
+						},
+						{
+							Time:    timestamppb.New(now.Add(3 * time.Second)),
+							Content: "\U0001F3C1  Job succeeded",
+						},
 					},
 				},
 			},
@@ -78,21 +101,44 @@ jobs:
       runs-on: ubuntu-latest
       steps:
         - run: echo job1 with zstd disabled
+    job2:
+      runs-on: ubuntu-latest
+      steps:
+        - run: echo job2 with zstd disabled
 `,
-			outcome: &mockTaskOutcome{
-				result: runnerv1.Result_RESULT_SUCCESS,
-				logRows: []*runnerv1.LogRow{
-					{
-						Time:    timestamppb.New(now.Add(4 * time.Second)),
-						Content: "  \U0001F433  docker create image",
+			outcome: []*mockTaskOutcome{
+				{
+					result: runnerv1.Result_RESULT_SUCCESS,
+					logRows: []*runnerv1.LogRow{
+						{
+							Time:    timestamppb.New(now.Add(4 * time.Second)),
+							Content: "  \U0001F433  docker create image",
+						},
+						{
+							Time:    timestamppb.New(now.Add(5 * time.Second)),
+							Content: "job1 zstd disabled",
+						},
+						{
+							Time:    timestamppb.New(now.Add(6 * time.Second)),
+							Content: "\U0001F3C1  Job succeeded",
+						},
 					},
-					{
-						Time:    timestamppb.New(now.Add(5 * time.Second)),
-						Content: "job1 zstd disabled",
-					},
-					{
-						Time:    timestamppb.New(now.Add(6 * time.Second)),
-						Content: "\U0001F3C1  Job succeeded",
+				},
+				{
+					result: runnerv1.Result_RESULT_SUCCESS,
+					logRows: []*runnerv1.LogRow{
+						{
+							Time:    timestamppb.New(now.Add(4 * time.Second)),
+							Content: "  \U0001F433  docker create image",
+						},
+						{
+							Time:    timestamppb.New(now.Add(5 * time.Second)),
+							Content: "job2 zstd disabled",
+						},
+						{
+							Time:    timestamppb.New(now.Add(6 * time.Second)),
+							Content: "\U0001F3C1  Job succeeded",
+						},
 					},
 				},
 			},
@@ -124,54 +170,55 @@ jobs:
 				opts := getWorkflowCreateFileOptions(user2, repo.DefaultBranch, fmt.Sprintf("create %s", tc.treePath), tc.fileContent)
 				createWorkflowFile(t, token, user2.Name, repo.Name, tc.treePath, opts)
 
-				// fetch and execute task
-				task := runner.fetchTask(t)
-				runner.execTask(t, task, tc.outcome)
+				// fetch and execute tasks
+				for jobIndex, outcome := range tc.outcome {
+					task := runner.fetchTask(t)
+					runner.execTask(t, task, outcome)
 
-				// check whether the log file exists
-				logFileName := fmt.Sprintf("%s/%02x/%d.log", repo.FullName(), task.Id%256, task.Id)
-				if setting.Actions.LogCompression.IsZstd() {
-					logFileName += ".zst"
-				}
-				_, err := storage.Actions.Stat(logFileName)
-				assert.NoError(t, err)
+					// check whether the log file exists
+					logFileName := fmt.Sprintf("%s/%02x/%d.log", repo.FullName(), task.Id%256, task.Id)
+					if setting.Actions.LogCompression.IsZstd() {
+						logFileName += ".zst"
+					}
+					_, err := storage.Actions.Stat(logFileName)
+					assert.NoError(t, err)
 
-				// download task logs and check content
-				runIndex := task.Context.GetFields()["run_number"].GetStringValue()
-				req := NewRequest(t, "GET", fmt.Sprintf("/%s/%s/actions/runs/%s/jobs/0/logs", user2.Name, repo.Name, runIndex)).
-					AddTokenAuth(token)
-				resp := MakeRequest(t, req, http.StatusOK)
-				logTextLines := strings.Split(strings.TrimSpace(resp.Body.String()), "\n")
-				assert.Len(t, logTextLines, len(tc.outcome.logRows))
-				for idx, lr := range tc.outcome.logRows {
-					assert.Equal(
-						t,
-						fmt.Sprintf("%s %s", lr.Time.AsTime().Format("2006-01-02T15:04:05.0000000Z07:00"), lr.Content),
-						logTextLines[idx],
-					)
-				}
+					// download task logs and check content
+					runIndex := task.Context.GetFields()["run_number"].GetStringValue()
+					req := NewRequest(t, "GET", fmt.Sprintf("/%s/%s/actions/runs/%s/jobs/%d/logs", user2.Name, repo.Name, runIndex, jobIndex)).
+						AddTokenAuth(token)
+					resp := MakeRequest(t, req, http.StatusOK)
+					logTextLines := strings.Split(strings.TrimSpace(resp.Body.String()), "\n")
+					assert.Len(t, logTextLines, len(outcome.logRows))
+					for idx, lr := range outcome.logRows {
+						assert.Equal(
+							t,
+							fmt.Sprintf("%s %s", lr.Time.AsTime().Format("2006-01-02T15:04:05.0000000Z07:00"), lr.Content),
+							logTextLines[idx],
+						)
+					}
 
-				runID, _ := strconv.ParseInt(task.Context.GetFields()["run_id"].GetStringValue(), 10, 64)
+					runID, _ := strconv.ParseInt(task.Context.GetFields()["run_id"].GetStringValue(), 10, 64)
 
-				jobs, err := actions_model.GetRunJobsByRunID(t.Context(), runID)
-				assert.NoError(t, err)
-				assert.Len(t, jobs, 1)
-				jobID := jobs[0].ID
+					jobs, err := actions_model.GetRunJobsByRunID(t.Context(), runID)
+					assert.NoError(t, err)
+					assert.Len(t, jobs, len(tc.outcome))
+					jobID := jobs[jobIndex].ID
 
-				// download task logs from API and check content
-				req = NewRequest(t, "GET", fmt.Sprintf("/api/v1/repos/%s/%s/actions/jobs/%d/logs", user2.Name, repo.Name, jobID)).
-					AddTokenAuth(token)
-				resp = MakeRequest(t, req, http.StatusOK)
-				logTextLines = strings.Split(strings.TrimSpace(resp.Body.String()), "\n")
-				assert.Len(t, logTextLines, len(tc.outcome.logRows))
-				for idx, lr := range tc.outcome.logRows {
-					assert.Equal(
-						t,
-						fmt.Sprintf("%s %s", lr.Time.AsTime().Format("2006-01-02T15:04:05.0000000Z07:00"), lr.Content),
-						logTextLines[idx],
-					)
+					// download task logs from API and check content
+					req = NewRequest(t, "GET", fmt.Sprintf("/api/v1/repos/%s/%s/actions/jobs/%d/logs", user2.Name, repo.Name, jobID)).
+						AddTokenAuth(token)
+					resp = MakeRequest(t, req, http.StatusOK)
+					logTextLines = strings.Split(strings.TrimSpace(resp.Body.String()), "\n")
+					assert.Len(t, logTextLines, len(outcome.logRows))
+					for idx, lr := range outcome.logRows {
+						assert.Equal(
+							t,
+							fmt.Sprintf("%s %s", lr.Time.AsTime().Format("2006-01-02T15:04:05.0000000Z07:00"), lr.Content),
+							logTextLines[idx],
+						)
+					}
 				}
-
 				resetFunc()
 			})
 		}

lunny · 2025-03-27T22:26:00Z

I suggest to update the test to the following, this bails on main and passes here

diff --git a/tests/integration/actions_log_test.go b/tests/integration/actions_log_test.go
index a157a923f6..cd20604b84 100644
--- a/tests/integration/actions_log_test.go
+++ b/tests/integration/actions_log_test.go
@@ -31,7 +31,7 @@ func TestDownloadTaskLogs(t *testing.T) {
 	testCases := []struct {
 		treePath    string
 		fileContent string
-		outcome     *mockTaskOutcome
+		outcome     []*mockTaskOutcome
 		zstdEnabled bool
 	}{
 		{
@@ -46,21 +46,44 @@ jobs:
       runs-on: ubuntu-latest
       steps:
         - run: echo job1 with zstd enabled
+    job2:
+      runs-on: ubuntu-latest
+      steps:
+        - run: echo job2 with zstd enabled
 `,
-			outcome: &mockTaskOutcome{
-				result: runnerv1.Result_RESULT_SUCCESS,
-				logRows: []*runnerv1.LogRow{
-					{
-						Time:    timestamppb.New(now.Add(1 * time.Second)),
-						Content: "  \U0001F433  docker create image",
-					},
-					{
-						Time:    timestamppb.New(now.Add(2 * time.Second)),
-						Content: "job1 zstd enabled",
+			outcome: []*mockTaskOutcome{
+				{
+					result: runnerv1.Result_RESULT_SUCCESS,
+					logRows: []*runnerv1.LogRow{
+						{
+							Time:    timestamppb.New(now.Add(1 * time.Second)),
+							Content: "  \U0001F433  docker create image",
+						},
+						{
+							Time:    timestamppb.New(now.Add(2 * time.Second)),
+							Content: "job1 zstd enabled",
+						},
+						{
+							Time:    timestamppb.New(now.Add(3 * time.Second)),
+							Content: "\U0001F3C1  Job succeeded",
+						},
 					},
-					{
-						Time:    timestamppb.New(now.Add(3 * time.Second)),
-						Content: "\U0001F3C1  Job succeeded",
+				},
+				{
+					result: runnerv1.Result_RESULT_SUCCESS,
+					logRows: []*runnerv1.LogRow{
+						{
+							Time:    timestamppb.New(now.Add(1 * time.Second)),
+							Content: "  \U0001F433  docker create image",
+						},
+						{
+							Time:    timestamppb.New(now.Add(2 * time.Second)),
+							Content: "job2 zstd enabled",
+						},
+						{
+							Time:    timestamppb.New(now.Add(3 * time.Second)),
+							Content: "\U0001F3C1  Job succeeded",
+						},
 					},
 				},
 			},
@@ -78,21 +101,44 @@ jobs:
       runs-on: ubuntu-latest
       steps:
         - run: echo job1 with zstd disabled
+    job2:
+      runs-on: ubuntu-latest
+      steps:
+        - run: echo job2 with zstd disabled
 `,
-			outcome: &mockTaskOutcome{
-				result: runnerv1.Result_RESULT_SUCCESS,
-				logRows: []*runnerv1.LogRow{
-					{
-						Time:    timestamppb.New(now.Add(4 * time.Second)),
-						Content: "  \U0001F433  docker create image",
+			outcome: []*mockTaskOutcome{
+				{
+					result: runnerv1.Result_RESULT_SUCCESS,
+					logRows: []*runnerv1.LogRow{
+						{
+							Time:    timestamppb.New(now.Add(4 * time.Second)),
+							Content: "  \U0001F433  docker create image",
+						},
+						{
+							Time:    timestamppb.New(now.Add(5 * time.Second)),
+							Content: "job1 zstd disabled",
+						},
+						{
+							Time:    timestamppb.New(now.Add(6 * time.Second)),
+							Content: "\U0001F3C1  Job succeeded",
+						},
 					},
-					{
-						Time:    timestamppb.New(now.Add(5 * time.Second)),
-						Content: "job1 zstd disabled",
-					},
-					{
-						Time:    timestamppb.New(now.Add(6 * time.Second)),
-						Content: "\U0001F3C1  Job succeeded",
+				},
+				{
+					result: runnerv1.Result_RESULT_SUCCESS,
+					logRows: []*runnerv1.LogRow{
+						{
+							Time:    timestamppb.New(now.Add(4 * time.Second)),
+							Content: "  \U0001F433  docker create image",
+						},
+						{
+							Time:    timestamppb.New(now.Add(5 * time.Second)),
+							Content: "job2 zstd disabled",
+						},
+						{
+							Time:    timestamppb.New(now.Add(6 * time.Second)),
+							Content: "\U0001F3C1  Job succeeded",
+						},
 					},
 				},
 			},
@@ -124,54 +170,55 @@ jobs:
 				opts := getWorkflowCreateFileOptions(user2, repo.DefaultBranch, fmt.Sprintf("create %s", tc.treePath), tc.fileContent)
 				createWorkflowFile(t, token, user2.Name, repo.Name, tc.treePath, opts)
 
-				// fetch and execute task
-				task := runner.fetchTask(t)
-				runner.execTask(t, task, tc.outcome)
+				// fetch and execute tasks
+				for jobIndex, outcome := range tc.outcome {
+					task := runner.fetchTask(t)
+					runner.execTask(t, task, outcome)
 
-				// check whether the log file exists
-				logFileName := fmt.Sprintf("%s/%02x/%d.log", repo.FullName(), task.Id%256, task.Id)
-				if setting.Actions.LogCompression.IsZstd() {
-					logFileName += ".zst"
-				}
-				_, err := storage.Actions.Stat(logFileName)
-				assert.NoError(t, err)
+					// check whether the log file exists
+					logFileName := fmt.Sprintf("%s/%02x/%d.log", repo.FullName(), task.Id%256, task.Id)
+					if setting.Actions.LogCompression.IsZstd() {
+						logFileName += ".zst"
+					}
+					_, err := storage.Actions.Stat(logFileName)
+					assert.NoError(t, err)
 
-				// download task logs and check content
-				runIndex := task.Context.GetFields()["run_number"].GetStringValue()
-				req := NewRequest(t, "GET", fmt.Sprintf("/%s/%s/actions/runs/%s/jobs/0/logs", user2.Name, repo.Name, runIndex)).
-					AddTokenAuth(token)
-				resp := MakeRequest(t, req, http.StatusOK)
-				logTextLines := strings.Split(strings.TrimSpace(resp.Body.String()), "\n")
-				assert.Len(t, logTextLines, len(tc.outcome.logRows))
-				for idx, lr := range tc.outcome.logRows {
-					assert.Equal(
-						t,
-						fmt.Sprintf("%s %s", lr.Time.AsTime().Format("2006-01-02T15:04:05.0000000Z07:00"), lr.Content),
-						logTextLines[idx],
-					)
-				}
+					// download task logs and check content
+					runIndex := task.Context.GetFields()["run_number"].GetStringValue()
+					req := NewRequest(t, "GET", fmt.Sprintf("/%s/%s/actions/runs/%s/jobs/%d/logs", user2.Name, repo.Name, runIndex, jobIndex)).
+						AddTokenAuth(token)
+					resp := MakeRequest(t, req, http.StatusOK)
+					logTextLines := strings.Split(strings.TrimSpace(resp.Body.String()), "\n")
+					assert.Len(t, logTextLines, len(outcome.logRows))
+					for idx, lr := range outcome.logRows {
+						assert.Equal(
+							t,
+							fmt.Sprintf("%s %s", lr.Time.AsTime().Format("2006-01-02T15:04:05.0000000Z07:00"), lr.Content),
+							logTextLines[idx],
+						)
+					}
 
-				runID, _ := strconv.ParseInt(task.Context.GetFields()["run_id"].GetStringValue(), 10, 64)
+					runID, _ := strconv.ParseInt(task.Context.GetFields()["run_id"].GetStringValue(), 10, 64)
 
-				jobs, err := actions_model.GetRunJobsByRunID(t.Context(), runID)
-				assert.NoError(t, err)
-				assert.Len(t, jobs, 1)
-				jobID := jobs[0].ID
+					jobs, err := actions_model.GetRunJobsByRunID(t.Context(), runID)
+					assert.NoError(t, err)
+					assert.Len(t, jobs, len(tc.outcome))
+					jobID := jobs[jobIndex].ID
 
-				// download task logs from API and check content
-				req = NewRequest(t, "GET", fmt.Sprintf("/api/v1/repos/%s/%s/actions/jobs/%d/logs", user2.Name, repo.Name, jobID)).
-					AddTokenAuth(token)
-				resp = MakeRequest(t, req, http.StatusOK)
-				logTextLines = strings.Split(strings.TrimSpace(resp.Body.String()), "\n")
-				assert.Len(t, logTextLines, len(tc.outcome.logRows))
-				for idx, lr := range tc.outcome.logRows {
-					assert.Equal(
-						t,
-						fmt.Sprintf("%s %s", lr.Time.AsTime().Format("2006-01-02T15:04:05.0000000Z07:00"), lr.Content),
-						logTextLines[idx],
-					)
+					// download task logs from API and check content
+					req = NewRequest(t, "GET", fmt.Sprintf("/api/v1/repos/%s/%s/actions/jobs/%d/logs", user2.Name, repo.Name, jobID)).
+						AddTokenAuth(token)
+					resp = MakeRequest(t, req, http.StatusOK)
+					logTextLines = strings.Split(strings.TrimSpace(resp.Body.String()), "\n")
+					assert.Len(t, logTextLines, len(outcome.logRows))
+					for idx, lr := range outcome.logRows {
+						assert.Equal(
+							t,
+							fmt.Sprintf("%s %s", lr.Time.AsTime().Format("2006-01-02T15:04:05.0000000Z07:00"), lr.Content),
+							logTextLines[idx],
+						)
+					}
 				}
-
 				resetFunc()
 			})
 		}

Could you push to this pull request?

ChristopherHX · 2025-03-27T22:46:37Z

Could you push to this pull request?

I am not a "merger", I can not push to your branch. We had this before in workflow dispatch api PR.

* giteaofficial/main: [skip ci] Updated translations via Crowdin Add toggleClass function in dom.ts (go-gitea#34063) Add a config option to block "expensive" pages for anonymous users (go-gitea#34024) add additional ReplaceAll in pathsep to cater for different pathsep (go-gitea#34061) [skip ci] Updated translations via Crowdin enable staticcheck QFxxxx rules (go-gitea#34064) update to golangci-lint v2 (go-gitea#34054) Add descriptions for private repo public access settings and improve the UI (go-gitea#34057) Add anonymous access support for private/unlisted repositories (go-gitea#34051) Hide activity contributors, recent commits and code frequrency left tabs if there is no code permission (go-gitea#34053) Update action status badge layout (go-gitea#34018) Add anonymous access support for private repositories (backend) (go-gitea#33257) Simplify emoji rendering (go-gitea#34048) Adjust the layout of the toolbar on the Issues/Projects page (go-gitea#33667) Fix bug on downloading job logs (go-gitea#34041) Fix git client accessing renamed repo (go-gitea#34034) Decouple Batch from git.Repository to simplify usage without requiring the creation of a Repository struct. (go-gitea#34001) fix org repo creation being limited by user limits (go-gitea#34030) Fix the issue with error message logging for the `check-attr` command on Windows OS. (go-gitea#34035) Try to fix check-attr bug (go-gitea#34029)

Fix bug on downloading job logs

acbb69c

GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Mar 27, 2025

pull-request-size bot added the size/XS label Mar 27, 2025

github-actions bot added the modifies/go Pull requests that update Go code label Mar 27, 2025

lunny mentioned this pull request Mar 27, 2025

Download actions job logs from API #33858

Merged

lunny added skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. type/bug labels Mar 27, 2025

ChristopherHX approved these changes Mar 27, 2025

View reviewed changes

GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Mar 27, 2025

Apply ChristopherHX's patch

c540222

pull-request-size bot added size/L and removed size/XS labels Mar 27, 2025

wxiaoguang approved these changes Mar 27, 2025

View reviewed changes

GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Mar 27, 2025

lunny added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Mar 27, 2025

Merge branch 'main' into lunny/fix_download_log

643f520

lunny merged commit 321cbcb into go-gitea:main Mar 28, 2025
26 checks passed

GiteaBot added this to the 1.24.0 milestone Mar 28, 2025

lunny deleted the lunny/fix_download_log branch March 28, 2025 00:20

GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Mar 28, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix bug on downloading job logs #34041

Fix bug on downloading job logs #34041

lunny commented Mar 27, 2025 •

edited

Loading

ChristopherHX left a comment

ChristopherHX commented Mar 27, 2025

lunny commented Mar 27, 2025

ChristopherHX commented Mar 27, 2025

Fix bug on downloading job logs #34041

Fix bug on downloading job logs #34041

Conversation

lunny commented Mar 27, 2025 • edited Loading

ChristopherHX left a comment

Choose a reason for hiding this comment

ChristopherHX commented Mar 27, 2025

lunny commented Mar 27, 2025

ChristopherHX commented Mar 27, 2025

lunny commented Mar 27, 2025 •

edited

Loading