Skip to content

Fix actions concurrency groups cross-branch leak#37311

Merged
silverwind merged 5 commits intogo-gitea:mainfrom
silverwind:branchconc
Apr 21, 2026
Merged

Fix actions concurrency groups cross-branch leak#37311
silverwind merged 5 commits intogo-gitea:mainfrom
silverwind:branchconc

Conversation

@silverwind
Copy link
Copy Markdown
Member

@silverwind silverwind commented Apr 20, 2026

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:

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


This PR was written with the help of Claude Opus 4.7

Workflow-level concurrency groups were evaluated in PrepareRunAndInsert
before the run was inserted, with run.ID still 0 and run.Index still 0.
GenerateGiteaContext hardcoded run_id to "", so expressions like
${{ github.head_ref || github.run_id }} collapsed to the same string on
every push event, causing cancel-in-progress to cancel unrelated runs
across different branches.

Move the workflow-level evaluation into InsertRun after run.Index is
assigned, and fall back to run.Index in the context when run.ID is not
yet set. run.Index is unique per repo, which is enough for concurrency
grouping (groups are scoped per repo).

Co-Authored-By: Claude (Opus 4.7) <noreply@anthropic.com>
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Apr 20, 2026
@silverwind silverwind added type/bug backport/v1.26 This PR should be backported to Gitea 1.26 labels Apr 20, 2026
@silverwind
Copy link
Copy Markdown
Member Author

silverwind commented Apr 20, 2026

This is a critical bug in the concurrency groups feature added in #32751, it causes cross-branch interference. @Zettat123 can you review? It's a partial extract from #37119.

Copy link
Copy Markdown
Contributor

@wxiaoguang wxiaoguang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even if I don't understand "Actions", I clearly know such hacky patch can't be right.

You should never make "run_id" sometimes be "id" sometimes be "index"

@GiteaBot GiteaBot added lgtm/blocked A maintainer has reservations with the PR and thus it cannot be merged and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Apr 20, 2026

This comment was marked as resolved.

@silverwind silverwind changed the title actions: populate github.run_id during pre-insert concurrency eval Actions: Fix concurrency groups cross-cancellation Apr 20, 2026
Address review feedback on PR go-gitea#37311: the previous revision overloaded
github.run_id to fall back to run.Index when run.ID was not yet set,
which was semantically incorrect — run_id should always mean run.ID.

Instead, reorder InsertRun so that db.Insert happens before concurrency
evaluation. This gives the run a real ID at eval time, so
${{ github.head_ref || github.run_id }} resolves to a per-run unique
value and no longer collapses across branches on push events. The
concurrency-derived fields (concurrency_group, concurrency_cancel,
status) are persisted with a follow-up UpdateRun.

Replace the narrow context unit test with a broader regression test
exercising EvaluateRunConcurrencyFillModel against two persisted
fixture runs, asserting distinct ConcurrencyGroup values for the real
head_ref || run_id expression.

Co-Authored-By: Claude (Opus 4.7) <noreply@anthropic.com>
@silverwind
Copy link
Copy Markdown
Member Author

silverwind commented Apr 20, 2026

Better fix pushed in e9498a6, verified and pr description updated.

@silverwind silverwind changed the title Actions: Fix concurrency groups cross-cancellation Fix actions concurrency groups cross-cancellation Apr 20, 2026
@silverwind silverwind changed the title Fix actions concurrency groups cross-cancellation Fix actions concurrency groups cross-branch cancellation Apr 20, 2026
@silverwind silverwind requested a review from Zettat123 April 20, 2026 08:18
@silverwind silverwind changed the title Fix actions concurrency groups cross-branch cancellation Fix actions concurrency groups cross-branch leak Apr 20, 2026
Comment thread services/actions/run.go Outdated
Addresses review feedback on PR go-gitea#37311: jobparser.Parse also needs a
non-zero run.ID so that github.run_id interpolates correctly in
run-name, job names, and runs-on — not just in workflow concurrency
group expressions.

Moves jobparser.Parse inside the InsertRun transaction, after
db.Insert(ctx, run), and consolidates all concurrency field
persistence (raw_concurrency, concurrency_group, concurrency_cancel,
status) into the single final UpdateRun at end-of-transaction. This
also fixes an intermediate regression where raw_concurrency was set in
memory but never written to the DB — services/actions/rerun.go reads
it from the DB on rerun.

Simplifies GenerateGiteaContext to set run_id from run.ID
unconditionally; all callers now pass a persisted run.

Adds TestPrepareRunAndInsert_ExpressionsSeeRunID exercising the full
PrepareRunAndInsert flow with \${{ github.run_id }} in both run-name
and concurrency group; a re-ordering regression would fail both
assertions.

Co-Authored-By: Claude (Opus 4.7) <noreply@anthropic.com>
Comment thread services/actions/run.go Outdated
The default title (commit message) was already persisted by db.Insert;
only include "title" in the final UpdateRun when jobs[0].RunName
actually overrode it.

Co-Authored-By: Claude (Opus 4.7) <noreply@anthropic.com>
@wxiaoguang wxiaoguang dismissed their stale review April 21, 2026 00:33

dismiss

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/blocked A maintainer has reservations with the PR and thus it cannot be merged labels Apr 21, 2026
@silverwind silverwind requested a review from Copilot April 21, 2026 00:50
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread services/actions/context.go
Comment thread services/actions/run.go
Comment thread services/actions/run.go
Comment thread services/actions/context_test.go
@silverwind silverwind added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Apr 21, 2026
@silverwind silverwind enabled auto-merge (squash) April 21, 2026 01:31
@silverwind silverwind merged commit f94b476 into go-gitea:main Apr 21, 2026
26 checks passed
@silverwind silverwind deleted the branchconc branch April 21, 2026 02:25
@GiteaBot GiteaBot added this to the 1.27.0 milestone Apr 21, 2026
@GiteaBot GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Apr 21, 2026
@GiteaBot GiteaBot added the backport/done All backports for this PR have been created label Apr 21, 2026
silverwind added a commit that referenced this pull request Apr 21, 2026
Backport #37311 by @silverwind

## 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](#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.

---
This PR was written with the help of Claude Opus 4.7

Co-authored-by: silverwind <me@silverwind.io>
Co-authored-by: Claude (Opus 4.7) <noreply@anthropic.com>
silverwind added a commit to 6543-forks/gitea that referenced this pull request Apr 21, 2026
…n-better

* origin/main: (645 commits)
  When the requested arch rpm is missing fall back to noarch (go-gitea#37236)
  Fix `relative-time` error and improve global error handler (go-gitea#37241)
  Enhance styling in actions page (go-gitea#37323)
  fix(oauth): Error on auth sources with spaces (go-gitea#37327)
  Fix actions concurrency groups cross-branch leak (go-gitea#37311)
  Fix bug when accessing user badges (go-gitea#37321)
  Fix AppFullLink (go-gitea#37325)
  Update go js dependencies (go-gitea#37312)
  Update GitHub Actions to latest major versions (go-gitea#37313)
  Revert "Add WebKit to e2e test matrix (go-gitea#37298)" (go-gitea#37315)
  Add `form-fetch-action` to some forms, fix "fetch action" resp bug (go-gitea#37305)
  Move heatmap to first-party code (go-gitea#37262)
  Use updated yaml fields for snapcraft (go-gitea#37318)
  Remove dead code identified by `deadcode` tool (go-gitea#37271)
  Enable strict TypeScript, add `errorMessage` helper (go-gitea#37292)
  Fix vite manifest update masking build errors (go-gitea#37279)
  bump snapcraft base (go-gitea#37301)
  Add WebKit to e2e test matrix (go-gitea#37298)
  Don't add useless labels which will bother changelog generation (go-gitea#37267)
  Fix Repository transferring page (go-gitea#37277)
  ...

# Conflicts:
#	options/locale/locale_en-US.ini
#	templates/package/content/debian.tmpl
zjjhot added a commit to zjjhot/gitea that referenced this pull request Apr 22, 2026
* main: (25 commits)
  Add URL to `Learn more about blocking a user` (go-gitea#37355)
  fix: use TriggerEvent instead of Event in workflow runs API response for scheduled runs (go-gitea#37288)
  Add event.schedule context for schedule actions task (go-gitea#37320)
  Fix typos (go-gitea#37346)
  Fix an issue where changing an organization’s visibility caused problems when users had forked its repositories. (go-gitea#37324)
  Fail vite build on rolldown warnings via NODE_ENV=test (go-gitea#37270)
  Use modern "git update-index --cacheinfo" syntax to support more file names (go-gitea#37338)
  Fix URL related escaping for oauth2 (go-gitea#37334)
  When the requested arch rpm is missing fall back to noarch (go-gitea#37236)
  Fix `relative-time` error and improve global error handler (go-gitea#37241)
  Enhance styling in actions page (go-gitea#37323)
  fix(oauth): Error on auth sources with spaces (go-gitea#37327)
  Fix actions concurrency groups cross-branch leak (go-gitea#37311)
  Fix bug when accessing user badges (go-gitea#37321)
  Fix AppFullLink (go-gitea#37325)
  Update go js dependencies (go-gitea#37312)
  Update GitHub Actions to latest major versions (go-gitea#37313)
  Revert "Add WebKit to e2e test matrix (go-gitea#37298)" (go-gitea#37315)
  Add `form-fetch-action` to some forms, fix "fetch action" resp bug (go-gitea#37305)
  Move heatmap to first-party code (go-gitea#37262)
  ...
silverwind added a commit to silverwind/gitea that referenced this pull request Apr 23, 2026
* origin/main: (32 commits)
  fix: commit status reporting (go-gitea#37372)
  Support for Custom URI Schemes in OAuth2 Redirect URIs (go-gitea#37356)
  Fix cmd tests by mocking builtin paths (go-gitea#37369)
  chore: upgrade Go version in devcontainer image to 1.26 (go-gitea#37374)
  Fix button layout shift when collapsing file tree in editor (go-gitea#37363)
  Update `Block a user` form (go-gitea#37359)
  Remove IsValidExternalURL/IsAPIURL and use IsValidURL at call sites (go-gitea#37364)
  Add URL to `Learn more about blocking a user` (go-gitea#37355)
  fix: use TriggerEvent instead of Event in workflow runs API response for scheduled runs (go-gitea#37288)
  Add event.schedule context for schedule actions task (go-gitea#37320)
  Fix typos (go-gitea#37346)
  Fix an issue where changing an organization’s visibility caused problems when users had forked its repositories. (go-gitea#37324)
  Fail vite build on rolldown warnings via NODE_ENV=test (go-gitea#37270)
  Use modern "git update-index --cacheinfo" syntax to support more file names (go-gitea#37338)
  Fix URL related escaping for oauth2 (go-gitea#37334)
  When the requested arch rpm is missing fall back to noarch (go-gitea#37236)
  Fix `relative-time` error and improve global error handler (go-gitea#37241)
  Enhance styling in actions page (go-gitea#37323)
  fix(oauth): Error on auth sources with spaces (go-gitea#37327)
  Fix actions concurrency groups cross-branch leak (go-gitea#37311)
  ...

# Conflicts:
#	services/actions/commit_status.go
silverwind added a commit to silverwind/gitea that referenced this pull request Apr 23, 2026
* origin/main: (204 commits)
  fix: commit status reporting (go-gitea#37372)
  Support for Custom URI Schemes in OAuth2 Redirect URIs (go-gitea#37356)
  Fix cmd tests by mocking builtin paths (go-gitea#37369)
  chore: upgrade Go version in devcontainer image to 1.26 (go-gitea#37374)
  Fix button layout shift when collapsing file tree in editor (go-gitea#37363)
  Update `Block a user` form (go-gitea#37359)
  Remove IsValidExternalURL/IsAPIURL and use IsValidURL at call sites (go-gitea#37364)
  Add URL to `Learn more about blocking a user` (go-gitea#37355)
  fix: use TriggerEvent instead of Event in workflow runs API response for scheduled runs (go-gitea#37288)
  Add event.schedule context for schedule actions task (go-gitea#37320)
  Fix typos (go-gitea#37346)
  Fix an issue where changing an organization’s visibility caused problems when users had forked its repositories. (go-gitea#37324)
  Fail vite build on rolldown warnings via NODE_ENV=test (go-gitea#37270)
  Use modern "git update-index --cacheinfo" syntax to support more file names (go-gitea#37338)
  Fix URL related escaping for oauth2 (go-gitea#37334)
  When the requested arch rpm is missing fall back to noarch (go-gitea#37236)
  Fix `relative-time` error and improve global error handler (go-gitea#37241)
  Enhance styling in actions page (go-gitea#37323)
  fix(oauth): Error on auth sources with spaces (go-gitea#37327)
  Fix actions concurrency groups cross-branch leak (go-gitea#37311)
  ...

# Conflicts:
#	web_src/js/index-domready.ts
#	web_src/js/markup/content.ts
#	web_src/js/markup/refissue.ts
silverwind added a commit to silverwind/gitea that referenced this pull request Apr 23, 2026
* origin/main: (204 commits)
  fix: commit status reporting (go-gitea#37372)
  Support for Custom URI Schemes in OAuth2 Redirect URIs (go-gitea#37356)
  Fix cmd tests by mocking builtin paths (go-gitea#37369)
  chore: upgrade Go version in devcontainer image to 1.26 (go-gitea#37374)
  Fix button layout shift when collapsing file tree in editor (go-gitea#37363)
  Update `Block a user` form (go-gitea#37359)
  Remove IsValidExternalURL/IsAPIURL and use IsValidURL at call sites (go-gitea#37364)
  Add URL to `Learn more about blocking a user` (go-gitea#37355)
  fix: use TriggerEvent instead of Event in workflow runs API response for scheduled runs (go-gitea#37288)
  Add event.schedule context for schedule actions task (go-gitea#37320)
  Fix typos (go-gitea#37346)
  Fix an issue where changing an organization’s visibility caused problems when users had forked its repositories. (go-gitea#37324)
  Fail vite build on rolldown warnings via NODE_ENV=test (go-gitea#37270)
  Use modern "git update-index --cacheinfo" syntax to support more file names (go-gitea#37338)
  Fix URL related escaping for oauth2 (go-gitea#37334)
  When the requested arch rpm is missing fall back to noarch (go-gitea#37236)
  Fix `relative-time` error and improve global error handler (go-gitea#37241)
  Enhance styling in actions page (go-gitea#37323)
  fix(oauth): Error on auth sources with spaces (go-gitea#37327)
  Fix actions concurrency groups cross-branch leak (go-gitea#37311)
  ...

Co-Authored-By: Claude (Opus 4.7) <noreply@anthropic.com>

# Conflicts:
#	web_src/js/index-domready.ts
#	web_src/js/markup/content.ts
#	web_src/js/markup/refissue.ts
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport/done All backports for this PR have been created backport/v1.26 This PR should be backported to Gitea 1.26 lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants