From f94b476c45579f94359139810b1d9167950f0747 Mon Sep 17 00:00:00 2001 From: silverwind Date: Tue, 21 Apr 2026 04:25:36 +0200 Subject: [PATCH] Fix actions concurrency groups cross-branch leak (#37311) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Problem Workflow-level concurrency groups were evaluated — and jobs were parsed — before the run was persisted, so `run.ID` was `0` and `github.run_id` in the expression context resolved to an empty string. Expressions like: ```yaml concurrency: group: ${{ github.workflow }}-${{ github.head_ref || github.run_id }} cancel-in-progress: true ``` collapsed to `-` on every push event (`head_ref` is empty on push), so `cancel-in-progress` cancelled in-progress runs across **unrelated branches**, not just the current one. Reproduced on a 1.26 instance: - push to `master` → `ci` run starts - push to `feature-branch` → the `master` run gets cancelled GitHub Actions' documented semantic: on push events `github.run_id` is unique per run, so the group is unique → no cancellation; on PR events `github.head_ref` is the source branch → cancellation is per-PR. ## Fix Insert the run **before** parsing jobs or evaluating workflow-level concurrency, so `run.ID` is populated in time for every expression that reads `github.run_id` — not just the concurrency group, but also `run-name`, job names, and `runs-on`. `jobparser.Parse` now runs inside the `InsertRun` transaction, after `db.Insert(ctx, run)`. Workflow-level concurrency evaluation runs next and only mutates `run` in memory. All concurrency-derived fields (`raw_concurrency`, `concurrency_group`, `concurrency_cancel`) plus `status` and `title` are persisted in a single final `UpdateRun` at end-of-transaction — one `INSERT` + one `UPDATE` per run in both the concurrency and non-concurrency paths (matches pre-branch parity, one fewer `UpdateRepoRunsNumbers` `COUNT` than the interim state). `GenerateGiteaContext` now sets `run_id` from `run.ID` unconditionally; every caller passes a persisted run. **Verification**: tested end-to-end on a 1.26 deployment. Before the patch, two successive `ci` pushes (one to master, one to a feature branch) cross-cancelled each other. After the patch, the same pushes — in both orders (master→branch, branch→master) — run to completion simultaneously across 15+ runs with zero cancellations. **Regression tests** in `services/actions/context_test.go`: - `TestEvaluateRunConcurrency_RunIDFallback` — unit check that `EvaluateRunConcurrencyFillModel` resolves `github.run_id` from `run.ID`. - `TestPrepareRunAndInsert_ExpressionsSeeRunID` — full-flow check: calls `PrepareRunAndInsert` with `${{ github.run_id }}` in both `run-name` and the concurrency group, then asserts the persisted `Title`, `ConcurrencyGroup`, and `RawConcurrency` contain / survive the run's ID. Re-ordering `db.Insert` relative to either parse or concurrency eval fails this test. ## Relation to #37119 [#37119](https://github.com/go-gitea/gitea/pull/37119) also moves concurrency evaluation into `InsertRun` but keeps it **before** `db.Insert`, then tries to populate `run_id` only when `run.ID > 0` — which is still `0` at that call site, so the cross-branch leak would survive that PR as written. This PR fixes the ordering so that `run.ID` is actually populated at eval time, and broadens it to cover parse-time expression interpolation too. Co-authored-by: Claude (Opus 4.7) --- services/actions/context.go | 2 +- services/actions/context_test.go | 72 ++++++++++++++++++++++++++++++++ services/actions/run.go | 65 ++++++++++++++-------------- 3 files changed, 107 insertions(+), 32 deletions(-) diff --git a/services/actions/context.go b/services/actions/context.go index 626ae6ee6b..69d5937623 100644 --- a/services/actions/context.go +++ b/services/actions/context.go @@ -73,7 +73,7 @@ func GenerateGiteaContext(run *actions_model.ActionRun, job *actions_model.Actio "repository_owner": run.Repo.OwnerName, // string, The repository owner's name. For example, Codertocat. "repositoryUrl": run.Repo.HTMLURL(), // string, The Git URL to the repository. For example, git://github.com/codertocat/hello-world.git. "retention_days": "", // string, The number of days that workflow run logs and artifacts are kept. - "run_id": "", // string, A unique number for each workflow run within a repository. This number does not change if you re-run the workflow run. + "run_id": strconv.FormatInt(run.ID, 10), // string, A unique number for each workflow run within a repository. This number does not change if you re-run the workflow run. "run_number": strconv.FormatInt(run.Index, 10), // string, A unique number for each run of a particular workflow in a repository. This number begins at 1 for the workflow's first run, and increments with each new run. This number does not change if you re-run the workflow run. "run_attempt": "", // string, A unique number for each attempt of a particular workflow run in a repository. This number begins at 1 for the workflow run's first attempt, and increments with each re-run. "secret_source": "Actions", // string, The source of a secret used in a workflow. Possible values are None, Actions, Dependabot, or Codespaces. diff --git a/services/actions/context_test.go b/services/actions/context_test.go index 74ef694021..4ade67111c 100644 --- a/services/actions/context_test.go +++ b/services/actions/context_test.go @@ -4,14 +4,86 @@ package actions import ( + "strconv" "testing" actions_model "code.gitea.io/gitea/models/actions" "code.gitea.io/gitea/models/unittest" + act_model "github.com/nektos/act/pkg/model" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) +func TestEvaluateRunConcurrency_RunIDFallback(t *testing.T) { + // Unit-level check that EvaluateRunConcurrencyFillModel resolves + // github.run_id from run.ID. The full-flow regression — that run.ID is + // non-zero by the time evaluation happens — is in + // TestPrepareRunAndInsert_ExpressionsSeeRunID. + assert.NoError(t, unittest.PrepareTestDatabase()) + ctx := t.Context() + + runA := unittest.AssertExistsAndLoadBean(t, &actions_model.ActionRun{ID: 791}) + runB := unittest.AssertExistsAndLoadBean(t, &actions_model.ActionRun{ID: 792}) + + expr := &act_model.RawConcurrency{ + Group: "${{ github.workflow }}-${{ github.head_ref || github.run_id }}", + CancelInProgress: "true", + } + + assert.NoError(t, EvaluateRunConcurrencyFillModel(ctx, runA, expr, nil, nil)) + assert.NoError(t, EvaluateRunConcurrencyFillModel(ctx, runB, expr, nil, nil)) + + assert.Contains(t, runA.ConcurrencyGroup, "791") + assert.Contains(t, runB.ConcurrencyGroup, "792") + assert.NotEqual(t, runA.ConcurrencyGroup, runB.ConcurrencyGroup) +} + +func TestPrepareRunAndInsert_ExpressionsSeeRunID(t *testing.T) { + // Regression for the cross-branch concurrency leak: github.run_id must + // be available during BOTH jobparser.Parse (run-name) and workflow-level + // concurrency evaluation. Re-ordering db.Insert relative to either step + // would leave run.ID at 0 and break this test. + assert.NoError(t, unittest.PrepareTestDatabase()) + ctx := t.Context() + + content := []byte(`name: cross-branch +run-name: "Run ${{ github.run_id }}" +on: push +concurrency: + group: group-${{ github.run_id }} + cancel-in-progress: true +jobs: + hello: + runs-on: ubuntu-latest + steps: + - run: echo hi +`) + + run := &actions_model.ActionRun{ + Title: "before parse", + RepoID: 4, + OwnerID: 1, + WorkflowID: "expr-runid.yaml", + TriggerUserID: 1, + Ref: "refs/heads/master", + CommitSHA: "c2d72f548424103f01ee1dc02889c1e2bff816b0", + Event: "push", + TriggerEvent: "push", + EventPayload: "{}", + } + require.NoError(t, PrepareRunAndInsert(ctx, content, run, nil)) + require.Positive(t, run.ID) + + persisted := unittest.AssertExistsAndLoadBean(t, &actions_model.ActionRun{ID: run.ID}) + runIDStr := strconv.FormatInt(run.ID, 10) + assert.Equal(t, "Run "+runIDStr, persisted.Title) + assert.Equal(t, "group-"+runIDStr, persisted.ConcurrencyGroup) + // Rerun reads raw_concurrency from the DB to re-evaluate the group; + // see services/actions/rerun.go. Must survive the insert. + assert.NotEmpty(t, persisted.RawConcurrency) +} + func TestFindTaskNeeds(t *testing.T) { assert.NoError(t, unittest.PrepareTestDatabase()) diff --git a/services/actions/run.go b/services/actions/run.go index e9fcdcaf43..432bb19628 100644 --- a/services/actions/run.go +++ b/services/actions/run.go @@ -13,6 +13,7 @@ import ( "code.gitea.io/gitea/modules/util" notify_service "code.gitea.io/gitea/services/notify" + act_model "github.com/nektos/act/pkg/model" "go.yaml.in/yaml/v4" ) @@ -34,25 +35,7 @@ func PrepareRunAndInsert(ctx context.Context, content []byte, run *actions_model return fmt.Errorf("ReadWorkflowRawConcurrency: %w", err) } - if wfRawConcurrency != nil { - err = EvaluateRunConcurrencyFillModel(ctx, run, wfRawConcurrency, vars, inputsWithDefaults) - if err != nil { - return fmt.Errorf("EvaluateRunConcurrencyFillModel: %w", err) - } - } - - giteaCtx := GenerateGiteaContext(run, nil) - - jobs, err := jobparser.Parse(content, jobparser.WithVars(vars), jobparser.WithGitContext(giteaCtx.ToGitHubContext()), jobparser.WithInputs(inputsWithDefaults)) - if err != nil { - return fmt.Errorf("parse workflow: %w", err) - } - - if len(jobs) > 0 && jobs[0].RunName != "" { - run.Title = jobs[0].RunName - } - - if err = InsertRun(ctx, run, jobs, vars, inputsWithDefaults); err != nil { + if err = InsertRun(ctx, run, content, vars, inputsWithDefaults, wfRawConcurrency); err != nil { return fmt.Errorf("InsertRun: %w", err) } @@ -74,7 +57,7 @@ func PrepareRunAndInsert(ctx context.Context, content []byte, run *actions_model // InsertRun inserts a run // The title will be cut off at 255 characters if it's longer than 255 characters. -func InsertRun(ctx context.Context, run *actions_model.ActionRun, jobs []*jobparser.SingleWorkflow, vars map[string]string, inputs map[string]any) error { +func InsertRun(ctx context.Context, run *actions_model.ActionRun, content []byte, vars map[string]string, inputs map[string]any, wfRawConcurrency *act_model.RawConcurrency) error { return db.WithTx(ctx, func(ctx context.Context) error { index, err := db.GetNextResourceIndex(ctx, "action_run_index", run.RepoID) if err != nil { @@ -82,23 +65,36 @@ func InsertRun(ctx context.Context, run *actions_model.ActionRun, jobs []*jobpar } run.Index = index run.Title = util.EllipsisDisplayString(run.Title, 255) + run.Status = actions_model.StatusWaiting - // check run (workflow-level) concurrency - run.Status, err = PrepareToStartRunWithConcurrency(ctx, run) - if err != nil { - return err - } - + // Insert before parsing jobs or evaluating workflow-level concurrency + // so that run.ID is populated. Expressions referencing github.run_id — + // in run-name, job names, runs-on, or a workflow-level concurrency + // group like `${{ github.head_ref || github.run_id }}` — would otherwise + // interpolate to an empty string. if err := db.Insert(ctx, run); err != nil { return err } - if err := run.LoadRepo(ctx); err != nil { - return err + giteaCtx := GenerateGiteaContext(run, nil) + jobs, err := jobparser.Parse(content, jobparser.WithVars(vars), jobparser.WithGitContext(giteaCtx.ToGitHubContext()), jobparser.WithInputs(inputs)) + if err != nil { + return fmt.Errorf("parse workflow: %w", err) } - if err := actions_model.UpdateRepoRunsNumbers(ctx, run.Repo); err != nil { - return err + titleChanged := len(jobs) > 0 && jobs[0].RunName != "" + if titleChanged { + run.Title = util.EllipsisDisplayString(jobs[0].RunName, 255) + } + + if wfRawConcurrency != nil { + if err := EvaluateRunConcurrencyFillModel(ctx, run, wfRawConcurrency, vars, inputs); err != nil { + return fmt.Errorf("EvaluateRunConcurrencyFillModel: %w", err) + } + run.Status, err = PrepareToStartRunWithConcurrency(ctx, run) + if err != nil { + return err + } } runJobs := make([]*actions_model.ActionRunJob, 0, len(jobs)) @@ -168,7 +164,14 @@ func InsertRun(ctx context.Context, run *actions_model.ActionRun, jobs []*jobpar } run.Status = actions_model.AggregateJobStatus(runJobs) - if err := actions_model.UpdateRun(ctx, run, "status"); err != nil { + cols := []string{"status"} + if titleChanged { + cols = append(cols, "title") + } + if wfRawConcurrency != nil { + cols = append(cols, "raw_concurrency", "concurrency_group", "concurrency_cancel") + } + if err := actions_model.UpdateRun(ctx, run, cols...); err != nil { return err }