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 <limiting-factor@noreply.codeberg.org>
Reviewed-by: elle <0xllx0@noreply.codeberg.org>

.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

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

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)

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",
+	)
+}

tests/integration/fixtures/TestCreateTaskForRunner/action_run.yml

@@ -0,0 +1,6 @@
+-
+  id: 900
+  index: 1
+  status: 5 # StatusWaiting
+  repo_id: 4
+  owner_id: 1

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

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"]'