From 044f3d95b65ccb4b76429094204df80d9ab648e9 Mon Sep 17 00:00:00 2001 From: Clouds Date: Fri, 5 Jun 2026 01:38:23 +0200 Subject: [PATCH] chore: re-enable nilnil lint for `models/actions/task.go` (#12768) I added sentinel error values for jobs not being found and no jobs being updated by the `CreateTaskForRunner` function. This avoids the nilnil antipattern and allows the nilnil lint to be enabled for the `models/actions/task.go` file again. The handling of these new errors was added to the `PickTask` function in `services/actions/task.go`. Related issue: https://codeberg.org/forgejo/forgejo/issues/11261 Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/12768 Reviewed-by: limiting-factor Reviewed-by: elle <0xllx0@noreply.codeberg.org> --- .golangci.yml | 3 - models/actions/task.go | 34 ++++---- services/actions/task.go | 10 ++- .../actions_create_task_for_runner_test.go | 78 +++++++++++++++++++ .../TestCreateTaskForRunner/action_run.yml | 6 ++ .../action_run_job.yml | 18 +++++ .../TestCreateTaskForRunner/action_runner.yml | 13 ++++ 7 files changed, 141 insertions(+), 21 deletions(-) create mode 100644 tests/integration/actions_create_task_for_runner_test.go create mode 100644 tests/integration/fixtures/TestCreateTaskForRunner/action_run.yml create mode 100644 tests/integration/fixtures/TestCreateTaskForRunner/action_run_job.yml create mode 100644 tests/integration/fixtures/TestCreateTaskForRunner/action_runner.yml diff --git a/.golangci.yml b/.golangci.yml index bed9acf98b..2d9ed5e098 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -189,9 +189,6 @@ linters: text: "(ST1005|ST1003|QF1001):" # TODO: eventually remove this section entirely - - path: models/actions/task.go - linters: - - nilnil - path: models/activities/action_list.go linters: - nilnil diff --git a/models/actions/task.go b/models/actions/task.go index d7e2b0ef87..033151dff4 100644 --- a/models/actions/task.go +++ b/models/actions/task.go @@ -6,6 +6,7 @@ package actions import ( "context" "crypto/subtle" + "errors" "fmt" "time" @@ -341,10 +342,15 @@ func GetAvailableJobsForRunner(e db.Engine, runner *ActionRunner) ([]*ActionRunJ return jobs, nil } -func CreateTaskForRunner(ctx context.Context, runner *ActionRunner, requestKey, handle *string) (*ActionTask, bool, error) { +var ( + ErrNoMatchingJobFound = errors.New("no matching job found") + ErrNoJobUpdated = errors.New("no job updated") +) + +func CreateTaskForRunner(ctx context.Context, runner *ActionRunner, requestKey, handle *string) (*ActionTask, error) { ctx, committer, err := db.TxContext(ctx) if err != nil { - return nil, false, err + return nil, err } defer committer.Close() @@ -352,7 +358,7 @@ func CreateTaskForRunner(ctx context.Context, runner *ActionRunner, requestKey, jobs, err := GetAvailableJobsForRunner(e, runner) if err != nil { - return nil, false, err + return nil, err } // TODO: a more efficient way to filter labels @@ -365,10 +371,10 @@ func CreateTaskForRunner(ctx context.Context, runner *ActionRunner, requestKey, } } if job == nil { - return nil, false, nil + return nil, ErrNoMatchingJobFound } if err := job.LoadAttributes(ctx); err != nil { - return nil, false, err + return nil, err } now := timeutil.TimeStampNow() @@ -393,20 +399,20 @@ func CreateTaskForRunner(ctx context.Context, runner *ActionRunner, requestKey, var workflowJob *jobparser.Job if gots, err := jobparser.Parse(job.WorkflowPayload, false); err != nil { - return nil, false, fmt.Errorf("parse workflow of job %d: %w", job.ID, err) + return nil, fmt.Errorf("parse workflow of job %d: %w", job.ID, err) } else if len(gots) != 1 { - return nil, false, fmt.Errorf("workflow of job %d: not single workflow", job.ID) + return nil, fmt.Errorf("workflow of job %d: not single workflow", job.ID) } else { //nolint:revive _, workflowJob = gots[0].Job() } if _, err := e.Insert(task); err != nil { - return nil, false, err + return nil, err } task.LogFilename = logFileName(job.Run.Repo.FullName(), task.ID) if err := UpdateTask(ctx, task, "log_filename"); err != nil { - return nil, false, err + return nil, err } if len(workflowJob.Steps) > 0 { @@ -422,7 +428,7 @@ func CreateTaskForRunner(ctx context.Context, runner *ActionRunner, requestKey, } } if _, err := e.Insert(steps); err != nil { - return nil, false, err + return nil, err } task.Steps = steps } @@ -430,18 +436,18 @@ func CreateTaskForRunner(ctx context.Context, runner *ActionRunner, requestKey, job.TaskID = task.ID // We never have to send a notification here because the job is started with a not done status. if n, err := UpdateRunJobWithoutNotification(ctx, job, builder.Eq{"task_id": 0}); err != nil { - return nil, false, err + return nil, err } else if n != 1 { - return nil, false, nil + return nil, ErrNoJobUpdated } task.Job = job if err := committer.Commit(); err != nil { - return nil, false, err + return nil, err } - return task, true, nil + return task, nil } // Placeholder tasks are created when the status/content of an [ActionRunJob] is resolved by Forgejo without dispatch to diff --git a/services/actions/task.go b/services/actions/task.go index a08403ad08..9fa56b767a 100644 --- a/services/actions/task.go +++ b/services/actions/task.go @@ -42,13 +42,15 @@ func PickTask(ctx context.Context, runner *actions_model.ActionRunner, requestKe } if err := db.WithTx(ctx, func(ctx context.Context) error { - t, ok, err := actions_model.CreateTaskForRunner(ctx, runner, requestKey, handle) + t, err := actions_model.CreateTaskForRunner(ctx, runner, requestKey, handle) if err != nil { + if errors.Is(err, actions_model.ErrNoMatchingJobFound) || + errors.Is(err, actions_model.ErrNoJobUpdated) { + return nil + } + return fmt.Errorf("CreateTaskForRunner: %w", err) } - if !ok { - return nil - } if err := t.LoadAttributes(ctx); err != nil { return fmt.Errorf("task LoadAttributes: %w", err) diff --git a/tests/integration/actions_create_task_for_runner_test.go b/tests/integration/actions_create_task_for_runner_test.go new file mode 100644 index 0000000000..2a9919fde9 --- /dev/null +++ b/tests/integration/actions_create_task_for_runner_test.go @@ -0,0 +1,78 @@ +// Copyright 2026 The Forgejo Authors. All rights reserved. +// SPDX-License-Identifier: GPL-3.0-or-later + +package integration + +import ( + "errors" + "sync" + "testing" + "time" + + actions_model "forgejo.org/models/actions" + "forgejo.org/models/unittest" + "forgejo.org/modules/setting" + "forgejo.org/tests" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestCreateTaskForRunnerNoMatchingJobFound(t *testing.T) { + defer unittest.OverrideFixtures("tests/integration/fixtures/TestCreateTaskForRunner")() + defer tests.PrepareTestEnv(t)() + + runner := unittest.AssertExistsAndLoadBean(t, &actions_model.ActionRunner{ID: 1004}) + + _, err := actions_model.CreateTaskForRunner(t.Context(), runner, nil, nil) + require.ErrorIs(t, err, actions_model.ErrNoMatchingJobFound) +} + +// We need to concurrently choose the same job with two requests to CreateTaskForRunner. The second +// request that tries to update the job in the database (that was already updated by the first +// request) then chokes and returns the error ErrNoJobUpdated. +func TestCreateTaskForRunnerNoJobUpdated(t *testing.T) { + if setting.Database.Type.IsSQLite3() { + // SQLite locks on the transaction and the desired race condition can't be achieved + t.Skip() + } + + defer unittest.OverrideFixtures("tests/integration/fixtures/TestCreateTaskForRunner")() + defer tests.PrepareTestEnv(t)() + + assert.Eventually( + t, + func() bool { + unittest.LoadFixtures() + + runner := unittest.AssertExistsAndLoadBean(t, &actions_model.ActionRunner{ID: 1005}) + + var w sync.WaitGroup + errs := make(chan error, 2) + for range 2 { + w.Go(func() { + _, err := actions_model.CreateTaskForRunner(t.Context(), runner, nil, nil) + errs <- err + }) + } + w.Wait() + close(errs) + + gotNoJobUpdated := false + succeded := false + for err := range errs { + if errors.Is(err, actions_model.ErrNoJobUpdated) { + gotNoJobUpdated = true + } + if err == nil { + succeded = true + } + } + + return succeded && gotNoJobUpdated + }, + 30*time.Second, + 500*time.Microsecond, + "one call to CreateTaskForRunner should succeed and the other one should return the error ErrNoJobUpdated", + ) +} diff --git a/tests/integration/fixtures/TestCreateTaskForRunner/action_run.yml b/tests/integration/fixtures/TestCreateTaskForRunner/action_run.yml new file mode 100644 index 0000000000..20ff848e02 --- /dev/null +++ b/tests/integration/fixtures/TestCreateTaskForRunner/action_run.yml @@ -0,0 +1,6 @@ +- + id: 900 + index: 1 + status: 5 # StatusWaiting + repo_id: 4 + owner_id: 1 diff --git a/tests/integration/fixtures/TestCreateTaskForRunner/action_run_job.yml b/tests/integration/fixtures/TestCreateTaskForRunner/action_run_job.yml new file mode 100644 index 0000000000..1052d648fc --- /dev/null +++ b/tests/integration/fixtures/TestCreateTaskForRunner/action_run_job.yml @@ -0,0 +1,18 @@ +- + id: 1000 + run_id: 900 + repo_id: 4 + owner_id: 1 + name: "crate task - no job updated test job" + job_id: create_task_no_job_updated + task_id: 0 + status: 5 # StatusWaiting + runs_on: '["ubuntu"]' + workflow_payload: | + name: test + on: push + jobs: + concurrent_test_job: + runs-on: ubuntu-latest + steps: + - run: echo hello diff --git a/tests/integration/fixtures/TestCreateTaskForRunner/action_runner.yml b/tests/integration/fixtures/TestCreateTaskForRunner/action_runner.yml new file mode 100644 index 0000000000..66d1937a79 --- /dev/null +++ b/tests/integration/fixtures/TestCreateTaskForRunner/action_runner.yml @@ -0,0 +1,13 @@ +- + id: 1004 + name: "create task - no matching job found test runner" + owner_id: 0 + repo_id: 0 + agent_labels: '["fedora"]' + +- + id: 1005 + name: "create task - no job updated test runner" + owner_id: 0 + repo_id: 0 + agent_labels: '["ubuntu"]'