0
0
mirror of https://github.com/go-gitea/gitea.git synced 2026-05-07 14:43:35 +02:00

Fix actions concurrency groups cross-branch leak (#37311)

## 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 `<workflow>-` 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) <noreply@anthropic.com>
This commit is contained in:
silverwind 2026-04-21 04:25:36 +02:00 committed by GitHub
parent 12733d3624
commit f94b476c45
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
3 changed files with 107 additions and 32 deletions

View File

@ -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.

View File

@ -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())

View File

@ -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
}