Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Actions FetchTask concurrently picked jobs is an error for the runner #33497

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ChristopherHX
Copy link
Contributor

  • Before this Gitea claimed no job is available to be picked in this case
  • The runner had to wait for an external taskversion increment
  • Now act_runner is notified about an error and retries the request later without updating its taskversion

Closes #33492

* Before this Gitea claimed no job is available to be picked in this case
* The runner had to wait for an external taskversion increment
* Now act_runner is notified about an error and retries the request later without updating its taskversion
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Feb 4, 2025
@pull-request-size pull-request-size bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Feb 4, 2025
@github-actions github-actions bot added the modifies/go Pull requests that update Go code label Feb 4, 2025
@ChristopherHX
Copy link
Contributor Author

This has been tested against my defect gitea docker image on a raspberry pi 4 arm64 by replacing the gitea binary with an custom backport to the 1.23.x release branch.
I didn't want to upgrade my database to the main branch and couldn't reproduce this on my stronger hardware

I wonder what are/were the reasons to not return an error in this situation?
The taskversion might has been an addition at a much later point of time conflicting with the original reason.

Bad aspects are, this logs unknown internal errors now while it appears to work all jobs get picked up now.

This is a draft PR, not 100% sure at this point if this patch should be merged like it is currently.

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Feb 15, 2025

Maybe you can call committer.Close() in that if, then still returns nil, false, nil, then no 500 error?

Hmm, above is not right.

Maybe it needs a special error, then make callers use errors.Is/As to check it/ignore it .......

@ChristopherHX
Copy link
Contributor Author

Maybe you can call committer.Close() in that if, then still returns nil, false, nil, then no 500 error?

Hmm, above is not right.

Maybe it needs a special error, then make callers use errors.Is/As to check it/ignore it .......

Thank you for your idea ❤️

So if we would return a special error, then instead of returning the error to the runner make the caller send "no job available" using errors.Is/As, but set taskversion = latestversion - 1 to force query database again then this might work as well

Both would make the runner query the database gain, but this new idea doesn't cause noise in the client

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. modifies/go Pull requests that update Go code size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Gitea Actions FetchTask not reliable assigning queued jobs to idle runners as long no new jobs are queued
3 participants