From e311aa7cae9c377d2fe71c2080b75ea6d3fc4d14 Mon Sep 17 00:00:00 2001
From: Mathieu Fenniak <mathieu@fenniak.net>
Date: Fri, 5 Sep 2025 19:31:22 +0200
Subject: [PATCH] fix: action view 'Re-run all jobs' leaves UI on the last
 attempt, not the new attempt (#9172)

In #9017, the ability to view older logs of an action run was added.  However, the 'Re-run all jobs' button was not updated in any way.  When viewing the logs for attempt 1 and clicking 'Re-run all jobs', the UI would continue to show the logs for attempt 1.  Before #9017 the behavior would have begun to view the logs from the re-run operation.

There are two commits in this PR:
- Update the `Rerun` view handler so that it redirects the user to the next attempt number for the job.
- The next attempt number isn't actually persisted to the DB until the rerun is picked up from a worker.  By pure coincidence, viewing an out-of-range attempt number was fully functional because it also happened to be viewing a job that wasn't picked up by a worker, and fell into those code paths.  However, as there were no automated tests around this codepath and it felt fragile, new tests have been added around the template render, backend data fetch, and frontend UI component, to ensure it continues to work in this corner-case in the future.

## Checklist

The [contributor guide](https://forgejo.org/docs/next/contributor/) contains information that will be helpful to first time contributors. There also are a few [conditions for merging Pull Requests in Forgejo repositories](https://codeberg.org/forgejo/governance/src/branch/main/PullRequestsAgreement.md). You are also welcome to join the [Forgejo development chatroom](https://matrix.to/#/#forgejo-development:matrix.org).

### Tests

- I added test coverage for Go changes...
  - [x] in their respective `*_test.go` for unit tests.
  - [x] in the `tests/integration` directory if it involves interactions with a live Forgejo server.
- I added test coverage for JavaScript changes...
  - [x] in `web_src/js/*.test.js` if it can be unit tested.
  - [ ] in `tests/e2e/*.test.e2e.js` if it requires interactions with a live Forgejo server (see also the [developer guide for JavaScript testing](https://codeberg.org/forgejo/forgejo/src/branch/forgejo/tests/e2e/README.md#end-to-end-tests)).

### Documentation

- [ ] I created a pull request [to the documentation](https://codeberg.org/forgejo/docs) to explain to Forgejo users how to use this change.
- [x] I did not document these changes and I do not expect someone else to do it.

### Release notes

- [x] I do not want this change to show in the release notes. (_Note_: This is a fix for an unreleased regression, no need for release notes.)
- [ ] I want the title to show in the release notes with a link to this pull request.
- [ ] I want the content of the `release-notes/<pull request number>.md` to be be used for the release notes instead of the title.

Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/9172
Reviewed-by: Earl Warren <earl-warren@noreply.codeberg.org>
Co-authored-by: Mathieu Fenniak <mathieu@fenniak.net>
Co-committed-by: Mathieu Fenniak <mathieu@fenniak.net>
---
 models/fixtures/action_run_job.yml           | 15 ++++
 routers/web/repo/actions/view.go             | 48 +++++++++++-
 routers/web/repo/actions/view_test.go        | 80 ++++++++++++++++++++
 tests/integration/actions_view_test.go       | 34 ++++++++-
 web_src/js/components/RepoActionView.test.js | 65 ++++++++++++++++
 5 files changed, 239 insertions(+), 3 deletions(-)

diff --git a/models/fixtures/action_run_job.yml b/models/fixtures/action_run_job.yml
index 9455ac3c41..00f9998ba9 100644
--- a/models/fixtures/action_run_job.yml
+++ b/models/fixtures/action_run_job.yml
@@ -128,3 +128,18 @@
   runs_on: '["fedora"]'
   started: 1683636528
   stopped: 1683636626
+-
+  id: 396
+  run_id: 794
+  repo_id: 4
+  owner_id: 1
+  commit_sha: 985f0301dba5e7b34be866819cd15ad3d8f508ee
+  is_fork_pull_request: 0
+  name: job_2
+  attempt: 1
+  job_id: job_2
+  task_id: null
+  status: 5
+  runs_on: '["fedora"]'
+  started: 1683636528
+  stopped: 1683636626
diff --git a/routers/web/repo/actions/view.go b/routers/web/repo/actions/view.go
index d9255aaa55..b5e73e9d33 100644
--- a/routers/web/repo/actions/view.go
+++ b/routers/web/repo/actions/view.go
@@ -336,6 +336,11 @@ func getViewResponse(ctx *context_module.Context, req *ViewRequest, runIndex, jo
 	}
 
 	var task *actions_model.ActionTask
+	// TaskID will be set only when the ActionRunJob has been picked by a runner, resulting in an ActionTask being
+	// created representing the specific task.  If current.TaskID is not set, then the user is attempting to view a job
+	// that hasn't been picked up by a runner... in this case we're not going to try to fetch the specific attempt.
+	// This helps to support the UI displaying a useful and error-free page when viewing a job that is queued but not
+	// picked, or an attempt that is queued for rerun but not yet picked.
 	if current.TaskID > 0 {
 		var err error
 		task, err = actions_model.GetTaskByJobAttempt(ctx, current.ID, attemptNumber)
@@ -357,6 +362,7 @@ func getViewResponse(ctx *context_module.Context, req *ViewRequest, runIndex, jo
 	}
 	resp.State.CurrentJob.Steps = make([]*ViewJobStep, 0) // marshal to '[]' instead of 'null' in json
 	resp.Logs.StepsLog = make([]*ViewStepLog, 0)          // marshal to '[]' instead of 'null' in json
+	// As noted above with TaskID; task will be nil when the job hasn't be picked yet...
 	if task != nil {
 		taskAttempts, err := task.GetAllAttempts(ctx)
 		if err != nil {
@@ -452,6 +458,12 @@ func getViewResponse(ctx *context_module.Context, req *ViewRequest, runIndex, jo
 	return resp
 }
 
+// When used with the JS `linkAction` handler (typically a <button> with class="link-action" and a data-url), will cause
+// the browser to redirect to the target page.
+type redirectObject struct {
+	Redirect string `json:"redirect"`
+}
+
 // Rerun will rerun jobs in the given run
 // If jobIndexStr is a blank string, it means rerun all jobs
 func Rerun(ctx *context_module.Context) {
@@ -493,6 +505,7 @@ func Rerun(ctx *context_module.Context) {
 	}
 
 	if jobIndexStr == "" { // rerun all jobs
+		var redirectURL string
 		for _, j := range jobs {
 			// if the job has needs, it should be set to "blocked" status to wait for other jobs
 			shouldBlock := len(j.Needs) > 0
@@ -500,13 +513,31 @@ func Rerun(ctx *context_module.Context) {
 				ctx.Error(http.StatusInternalServerError, err.Error())
 				return
 			}
+			if redirectURL == "" {
+				// ActionRunJob's `Attempt` field won't be updated to reflect the rerun until the job is picked by a
+				// runner. But we need to redirect the user somewhere; if they stay on the current attempt then the
+				// rerun's logs won't appear. So, we redirect to the upcoming new attempt and then we'll handle the
+				// weirdness in the UI if the attempt doesn't exist yet.
+				j.Attempt++ // note: this is intentionally not persisted
+				redirectURL, err = j.HTMLURL(ctx)
+				if err != nil {
+					ctx.Error(http.StatusInternalServerError, err.Error())
+					return
+				}
+			}
+		}
+
+		if redirectURL != "" {
+			ctx.JSON(http.StatusOK, &redirectObject{Redirect: redirectURL})
+		} else {
+			ctx.Error(http.StatusInternalServerError, "unable to determine redirectURL for job rerun")
 		}
-		ctx.JSON(http.StatusOK, struct{}{})
 		return
 	}
 
 	rerunJobs := actions_service.GetAllRerunJobs(job, jobs)
 
+	var redirectURL string
 	for _, j := range rerunJobs {
 		// jobs other than the specified one should be set to "blocked" status
 		shouldBlock := j.JobID != job.JobID
@@ -514,9 +545,22 @@ func Rerun(ctx *context_module.Context) {
 			ctx.Error(http.StatusInternalServerError, err.Error())
 			return
 		}
+		if j.JobID == job.JobID {
+			// see earlier comment about redirectURL, applicable here as well
+			j.Attempt++ // note: this is intentionally not persisted
+			redirectURL, err = j.HTMLURL(ctx)
+			if err != nil {
+				ctx.Error(http.StatusInternalServerError, err.Error())
+				return
+			}
+		}
 	}
 
-	ctx.JSON(http.StatusOK, struct{}{})
+	if redirectURL != "" {
+		ctx.JSON(http.StatusOK, &redirectObject{Redirect: redirectURL})
+	} else {
+		ctx.Error(http.StatusInternalServerError, "unable to determine redirectURL for job rerun")
+	}
 }
 
 func rerunJob(ctx *context_module.Context, job *actions_model.ActionRunJob, shouldBlock bool) error {
diff --git a/routers/web/repo/actions/view_test.go b/routers/web/repo/actions/view_test.go
index 68b981211e..b94e57591a 100644
--- a/routers/web/repo/actions/view_test.go
+++ b/routers/web/repo/actions/view_test.go
@@ -304,6 +304,40 @@ func TestActionsViewViewPost(t *testing.T) {
 				resp.State.CurrentJob.Steps[1].Status = "waiting"
 			},
 		},
+		{
+			// This ActionRunJob has TaskID: null, which allows us to access out-of-range attempts without errors and
+			// with just some stub data for the UI to start waiting around on.
+			name:          "attempt out-of-bounds on non-picked task",
+			runIndex:      190,
+			jobIndex:      0,
+			attemptNumber: 100,
+			expected:      baseExpectedViewResponse(),
+			expectedTweaks: func(resp *ViewResponse) {
+				// Variations from runIndex 187 -> runIndex 190 that are not the subject of this test...
+				resp.State.Run.Link = "/user5/repo4/actions/runs/190"
+				resp.State.Run.Title = "job output"
+				resp.State.Run.TitleHTML = "job output"
+				resp.State.Run.Done = false
+				resp.State.Run.Jobs = []*ViewJob{
+					{
+						ID:     396,
+						Name:   "job_2",
+						Status: "waiting",
+					},
+				}
+				resp.State.Run.Commit.Branch = ViewBranch{
+					Name:      "test",
+					Link:      "/user5/repo4/src/branch/test",
+					IsDeleted: true,
+				}
+
+				// Expected blank data in the response because this job isn't picked by a runner yet.  Keep details here
+				// in-sync with the RepoActionView 'view non-picked action run job' test.
+				resp.State.CurrentJob.Detail = "actions.status.waiting"
+				resp.State.CurrentJob.Steps = []*ViewJobStep{}
+				resp.State.CurrentJob.AllAttempts = nil
+			},
+		},
 	}
 
 	for _, tt := range tests {
@@ -410,3 +444,49 @@ func TestActionsViewRedirectToLatestAttempt(t *testing.T) {
 		})
 	}
 }
+
+func TestActionsRerun(t *testing.T) {
+	tests := []struct {
+		name         string
+		runIndex     int64
+		jobIndex     int64
+		expectedCode int
+		expectedURL  string
+	}{
+		{
+			name:        "rerun all",
+			runIndex:    187,
+			jobIndex:    -1,
+			expectedURL: "https://try.gitea.io/user2/repo1/actions/runs/187/jobs/0/attempt/2",
+		},
+		{
+			name:        "rerun job",
+			runIndex:    187,
+			jobIndex:    2,
+			expectedURL: "https://try.gitea.io/user2/repo1/actions/runs/187/jobs/2/attempt/3",
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			ctx, resp := contexttest.MockContext(t, "user2/repo1/actions/runs/187/rerun")
+			contexttest.LoadUser(t, ctx, 2)
+			contexttest.LoadRepo(t, ctx, 1)
+			ctx.SetParams(":run", fmt.Sprintf("%d", tt.runIndex))
+			if tt.jobIndex != -1 {
+				ctx.SetParams(":job", fmt.Sprintf("%d", tt.jobIndex))
+			}
+
+			Rerun(ctx)
+			require.Equal(t, http.StatusOK, resp.Result().StatusCode, "failure in Rerun(): %q", resp.Body.String())
+
+			var actual redirectObject
+			err := json.Unmarshal(resp.Body.Bytes(), &actual)
+			require.NoError(t, err)
+
+			// Note: this test isn't doing any functional testing of the Rerun handler's actual ability to set up a job
+			// rerun.  This test was added when the redirect to the correct `attempt` was added and only covers that
+			// addition at this time.
+			assert.Equal(t, redirectObject{Redirect: tt.expectedURL}, actual)
+		})
+	}
+}
diff --git a/tests/integration/actions_view_test.go b/tests/integration/actions_view_test.go
index 422439b60f..1a0cdc2ec1 100644
--- a/tests/integration/actions_view_test.go
+++ b/tests/integration/actions_view_test.go
@@ -23,7 +23,7 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
-func TestActionsViewArtifactDeletion(t *testing.T) {
+func TestActionViewsArtifactDeletion(t *testing.T) {
 	onGiteaRun(t, func(t *testing.T, u *url.URL) {
 		user2 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2})
 
@@ -156,3 +156,35 @@ func TestActionViewsView(t *testing.T) {
 	})
 	htmlDoc.AssertAttrEqual(t, selector, "data-initial-artifacts-response", "{\"artifacts\":[{\"name\":\"multi-file-download\",\"size\":2048,\"status\":\"completed\"}]}\n")
 }
+
+// Action re-run will redirect the user to an attempt that may not exist in the database yet, since attempts are only
+// updated in the DB when jobs are picked up by runners.  This test is intended to ensure that a "future" attempt number
+// can still be loaded into the repo-action-view, which will handle waiting & polling for it to have data.
+func TestActionViewsViewAttemptOutOfRange(t *testing.T) {
+	defer tests.PrepareTestEnv(t)()
+
+	// For this test to accurately reflect an attempt not yet picked, it needs to be accessing an ActionRunJob with
+	// TaskID: null... otherwise we can't fetch future unpersisted attempts.
+	req := NewRequest(t, "GET", "/user5/repo4/actions/runs/190/jobs/0/attempt/100")
+	resp := MakeRequest(t, req, http.StatusOK)
+
+	htmlDoc := NewHTMLParser(t, resp.Body)
+	selector := "#repo-action-view"
+	// Verify key properties going into the `repo-action-view` to initialize the Vue component.
+	htmlDoc.AssertAttrEqual(t, selector, "data-run-index", "190")
+	htmlDoc.AssertAttrEqual(t, selector, "data-job-index", "0")
+	htmlDoc.AssertAttrEqual(t, selector, "data-attempt-number", "100")
+	htmlDoc.AssertAttrPredicate(t, selector, "data-initial-post-response", func(actual string) bool {
+		// Remove dynamic "duration" fields for comparison.
+		pattern := `"duration":"[^"]*"`
+		re := regexp.MustCompile(pattern)
+		actualClean := re.ReplaceAllString(actual, `"duration":"_duration_"`)
+		// Remove "time_since_started_html" fields for comparison since they're TZ-sensitive in the test
+		pattern = `"time_since_started_html":".*?\\u003c/relative-time\\u003e"`
+		re = regexp.MustCompile(pattern)
+		actualClean = re.ReplaceAllString(actualClean, `"time_since_started_html":"_time_"`)
+
+		return assert.JSONEq(t, "{\"state\":{\"run\":{\"link\":\"/user5/repo4/actions/runs/190\",\"title\":\"job output\",\"titleHTML\":\"job output\",\"status\":\"success\",\"canCancel\":false,\"canApprove\":false,\"canRerun\":false,\"canDeleteArtifact\":false,\"done\":false,\"jobs\":[{\"id\":396,\"name\":\"job_2\",\"status\":\"waiting\",\"canRerun\":false,\"duration\":\"_duration_\"}],\"commit\":{\"localeCommit\":\"Commit\",\"localePushedBy\":\"pushed by\",\"localeWorkflow\":\"Workflow\",\"shortSHA\":\"c2d72f5484\",\"link\":\"/user5/repo4/commit/c2d72f548424103f01ee1dc02889c1e2bff816b0\",\"pusher\":{\"displayName\":\"user1\",\"link\":\"/user1\"},\"branch\":{\"name\":\"test\",\"link\":\"/user5/repo4/src/branch/test\",\"isDeleted\":true}}},\"currentJob\":{\"title\":\"job_2\",\"detail\":\"Waiting\",\"steps\":[],\"allAttempts\":null}},\"logs\":{\"stepsLog\":[]}}\n", actualClean)
+	})
+	htmlDoc.AssertAttrEqual(t, selector, "data-initial-artifacts-response", "{\"artifacts\":[]}\n")
+}
diff --git a/web_src/js/components/RepoActionView.test.js b/web_src/js/components/RepoActionView.test.js
index 1fe7a84a4d..c304e1915d 100644
--- a/web_src/js/components/RepoActionView.test.js
+++ b/web_src/js/components/RepoActionView.test.js
@@ -559,3 +559,68 @@ test('initial load data is used without calling fetch()', async () => {
   await flushPromises();
   expect(fetchSpy).not.toHaveBeenCalled();
 });
+
+test('view non-picked action run job', async () => {
+  Object.defineProperty(document.documentElement, 'lang', {value: 'en'});
+  const wrapper = mount(RepoActionView, {
+    props: {
+      actionsURL: 'https://example.com/example-org/example-repo/actions',
+      runIndex: '10',
+      runID: '1001',
+      jobIndex: '2',
+      attemptNumber: '1',
+      initialJobData: {
+        ...minimalInitialJobData,
+        // Definitions here should match the same type of content as the related backend test,
+        // "TestActionsViewViewPost/attempt out-of-bounds on non-picked task", so that we're sure we can handle in the
+        // UI what the backend will deliver in this case.
+        state: {
+          run: {
+            done: false,
+            status: 'waiting',
+            commit: {
+              pusher: {},
+            },
+            jobs: [
+              {
+                id: 161,
+                name: 'check-1',
+                status: 'waiting',
+                canRerun: false,
+                duration: '0s',
+              },
+              {
+                id: 162,
+                name: 'check-2',
+                status: 'waiting',
+                canRerun: false,
+                duration: '0s',
+              },
+              {
+                id: 163,
+                name: 'check-3',
+                status: 'waiting',
+                canRerun: false,
+                duration: '0s',
+              },
+            ],
+          },
+          currentJob: {
+            title: 'check-1',
+            detail: 'waiting (locale)', // locale-specific, not exact match to backend test
+            steps: [],
+            allAttempts: null,
+          },
+        },
+      },
+      initialArtifactData: minimalInitialArtifactData,
+      locale: testLocale,
+    },
+  });
+  await flushPromises();
+
+  expect(wrapper.get('.job-info-header-detail').text()).toEqual('waiting (locale)');
+  expect(wrapper.get('.job-brief-list .job-brief-item:nth-of-type(1) .job-brief-name').text()).toEqual('check-1');
+  expect(wrapper.get('.job-brief-list .job-brief-item:nth-of-type(2) .job-brief-name').text()).toEqual('check-2');
+  expect(wrapper.get('.job-brief-list .job-brief-item:nth-of-type(3) .job-brief-name').text()).toEqual('check-3');
+});