Skip to content

fix(actions): make SingleWorkflow.Marshal round-trip multi-line run blocks (stop silent job stranding) - #38520

Merged
bircni merged 8 commits into
go-gitea:mainfrom
darklight147:fix/jobparser-run-block-roundtrip-pr
Jul 23, 2026
Merged

fix(actions): make SingleWorkflow.Marshal round-trip multi-line run blocks (stop silent job stranding)#38520
bircni merged 8 commits into
go-gitea:mainfrom
darklight147:fix/jobparser-run-block-roundtrip-pr

Conversation

@darklight147

Copy link
Copy Markdown
Contributor

Problem

Jobs that call a reusable workflow (uses:) whose steps contain a run: | block that starts with blank lines never start — the child jobs stay Blocked forever, the run never finishes, and nothing is shown to the user (the error is only logged at DEBUG). The reusable is valid YAML and worked on 1.26.

Root cause

jobparser serializes each expanded job via SingleWorkflow.SetJob(), which uses yaml.NewEncoder(...).SetIndent(2), but SingleWorkflow.Marshal() used yaml.Marshal, whose default indentation is 4. Re-emitting a multi-line literal block scalar at a different indentation makes the encoder write a wrong explicit indentation indicator (run: |4) whose declared indent doesn't match the actual content indent. The stored workflow_payload is then unparseable:

run: |4


                while ...

jobparser.Parse / model.ReadWorkflow (both go.yaml.in/yaml/v4) reject it: did not find expected key. This surfaces in services/actions/job_emitter.go resolve()updateConcurrencyEvaluationForJobWithNeedsParseJob, where the error is swallowed at log.Debug and the job is left Blocked.

Encoding at indent 4 triggers the bad indicator; indent 2 does not — matching the value already used by SetJob.

Fix

Encode SingleWorkflow.Marshal() with SetIndent(2) so both encoders agree and the serialized single workflow round-trips. Adds a regression test (Parse → Marshal → Parse on a run: block with leading blank lines) that fails before the change with did not find expected key.

Notes

  • This is the correctness fix. Related: Report structurally invalid workflows to users #37116 added ValidateWorkflowContent to report such content for top-level workflows, but the reusable-expansion/concurrency-eval path is not covered and strands silently — this fix removes the failure mode entirely.
  • Consider a follow-up to elevate the swallowed error in job_emitter.resolve() from log.Debug to a user-visible job failure (there is an existing // TODO).

… SetJob

jobparser builds each reusable/expanded job by encoding it with SetIndent(2)
in SetJob, but SingleWorkflow.Marshal used yaml.Marshal, whose default
indentation is 4. Re-emitting a multi-line block scalar (e.g. a `run:` step
that begins with blank lines) at a different indentation makes the encoder
write a wrong explicit indentation indicator (`run: |4`) that no longer
parses. The stored workflow_payload then fails ParseJob during concurrency
evaluation (EvaluateJobConcurrencyFillModel) and the job is left silently
Blocked, stranding the whole run.

Encode Marshal with the same indentation (2) so the serialized single
workflow round-trips. Adds a regression test.

Signed-off-by: quasimodo <mohamed.belkamel@intelcia.com>
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jul 18, 2026
@bircni

bircni commented Jul 18, 2026

Copy link
Copy Markdown
Member

Consider a follow-up to elevate the swallowed error in job_emitter.resolve() from log.Debug to a user-visible job failure (there is an existing // TODO).

why not directly here?

Comment thread modules/actions/jobparser/roundtrip_test.go
@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Jul 18, 2026
@lunny lunny added the backport/v1.27 This PR should be backported to Gitea 1.27 label Jul 19, 2026
@lunny
lunny requested a review from Zettat123 July 20, 2026 17:26
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Jul 23, 2026
@Zettat123
Zettat123 self-requested a review July 23, 2026 03:38
@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. labels Jul 23, 2026
@Zettat123

Copy link
Copy Markdown
Contributor

Consider a follow-up to elevate the swallowed error in job_emitter.resolve() from log.Debug to a user-visible job failure (there is an existing // TODO).

why not directly here?

Making the error user-visible might need extra db columns, which can't be backported. So I think we can do that in a follow-up PR rather than this PR.

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Jul 23, 2026
@lunny lunny added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Jul 23, 2026
@bircni
bircni merged commit c4fc4d3 into go-gitea:main Jul 23, 2026
23 checks passed
@GiteaBot GiteaBot added this to the 28.0.0 milestone Jul 23, 2026
@GiteaBot GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Jul 23, 2026
@GiteaBot GiteaBot added the backport/done All backports for this PR have been created label Jul 23, 2026
silverwind pushed a commit that referenced this pull request Jul 23, 2026
…locks (stop silent job stranding) (#38520) (#38599)

Backport #38520 by @darklight147

## Problem
Jobs that call a reusable workflow (`uses:`) whose steps contain a `run:
|` block that **starts with blank lines** never start — the child jobs
stay `Blocked` forever, the run never finishes, and nothing is shown to
the user (the error is only logged at DEBUG). The reusable is valid YAML
and worked on 1.26.

## Root cause
`jobparser` serializes each expanded job via `SingleWorkflow.SetJob()`,
which uses `yaml.NewEncoder(...).SetIndent(2)`, but
`SingleWorkflow.Marshal()` used `yaml.Marshal`, whose default
indentation is **4**. Re-emitting a multi-line literal block scalar at a
different indentation makes the encoder write a wrong explicit
indentation indicator (`run: |4`) whose declared indent doesn't match
the actual content indent. The stored `workflow_payload` is then
unparseable:

```
run: |4


                while ...
```

`jobparser.Parse` / `model.ReadWorkflow` (both go.yaml.in/yaml/v4)
reject it: `did not find expected key`. This surfaces in
`services/actions/job_emitter.go` `resolve()` →
`updateConcurrencyEvaluationForJobWithNeeds` → `ParseJob`, where the
error is swallowed at `log.Debug` and the job is left `Blocked`.

Encoding at indent 4 triggers the bad indicator; indent 2 does not —
matching the value already used by `SetJob`.

## Fix
Encode `SingleWorkflow.Marshal()` with `SetIndent(2)` so both encoders
agree and the serialized single workflow round-trips. Adds a regression
test (`Parse → Marshal → Parse` on a `run:` block with leading blank
lines) that fails before the change with `did not find expected key`.

## Notes
- This is the correctness fix. Related: #37116 added
`ValidateWorkflowContent` to *report* such content for top-level
workflows, but the reusable-expansion/concurrency-eval path is not
covered and strands silently — this fix removes the failure mode
entirely.
- Consider a follow-up to elevate the swallowed error in
`job_emitter.resolve()` from `log.Debug` to a user-visible job failure
(there is an existing `// TODO`).

Signed-off-by: quasimodo <mohamed.belkamel@intelcia.com>
Co-authored-by: Mohamed Belkamel <39389636+darklight147@users.noreply.github.com>
Co-authored-by: quasimodo <mohamed.belkamel@intelcia.com>
Co-authored-by: Zettat123 <zettat123@gmail.com>
silverwind added a commit to bircni/gitea that referenced this pull request Jul 24, 2026
* origin/main: (21 commits)
  feat(setting): add shared [redis] section as default for redis-backed subsystems (go-gitea#38550)
  fix(webhook): remove slack channel name check (go-gitea#38608)
  ci: derive topic labels from PR title (go-gitea#38595)
  fix: download dropdown menu clipped on the branches page (go-gitea#38604)
  enhance(actions): action view enhancements (go-gitea#38594)
  enhance: keep status check list scrolled on merge box reload (go-gitea#38597)
  fix: make auth source group sync correctly handle team removal (go-gitea#37161)
  fix(oauth2): enforce mandatory 2FA policy on OAuth2 authorize/grant endpoints (go-gitea#38591)
  refactor: hide git repo path details from more packages (go-gitea#38601)
  fix(project): prevent database mutations on invalid MoveIssues payload (go-gitea#38600)
  refactor: retry file remove/rename when a file is busy and clean up os detection (go-gitea#38588)
  fix(actions): make SingleWorkflow.Marshal round-trip multi-line run blocks (stop silent job stranding) (go-gitea#38520)
  fix(issue): display error toast on batch action failures instead of reloading page (go-gitea#38593)
  perf(emoji): optimize FindEmojiSubmatchIndex using slice-based Trie (go-gitea#38573)
  test(e2e): add pull request merge box test, update AGENTS.md (go-gitea#38576)
  fix(api): align Swagger schemas for UserSettings and TopicListResponse (go-gitea#38590)
  fix(file-tree): handle submodule links and missing view container (go-gitea#38033)
  refactor: clean up git repo and model migration packages (go-gitea#38564)
  [skip ci] Updated translations via Crowdin
  fix(actions): fail unexpandable reusable workflow callers and decouple the job emitter's cross-run processing (go-gitea#38565)
  ...

# Conflicts:
#	services/actions/reusable_workflow_test.go
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.27 This PR should be backported to Gitea 1.27 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.

5 participants