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

Adjustments to remove dangling repository locks #32485

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

Conversation

bsofiato
Copy link
Contributor

@bsofiato bsofiato commented Nov 12, 2024

This PR allows Gitea to detect if a repo has any dangling locks (older than a threshold) and remove them if needed. It also detects if a git process has crashed (in that case, it will also remove any locks that remain).

By default, the threshold for a lock to be eligible for removal is 1 hour. This can be changed via the DANGLING_LOCK_THRESHOLD configuration parameter (the PR for the docs is still in the works).

There are still some points that might need clarification.

  1. I still do not know if we need the crontask (as discussed here, the original idea was to have a cronjob that remove the locks that could not be removed by detecting if the git processed had crashed), since we can check for dangling locks before issuing the actual git command (If you think this is subpar, I can remove it);
  2. The code will look for every file within the repo whose extension is ´.lock' and remove it if it is older than the threshold. As pointed out, there might be locks at the refs level so we must search for them by walking the repo's directory structure;
  3. I could not come up with a way of reliably detecting whether a process has crashed when running on Windows. In this case, I assume that the process hasn't and the lock removal process is skipped altogether. The repo will remain locked until its locks are older than the given threshold.

Resolves #22578

P.S. It seems that the unit tests are breaking due to some timing issue (evidence attached)

image

P.S, Below is the description of the PR when it was a draft

This is a work-in-progress PR

Hey @lunny, I've written this PR as a Proof of Concept on removing dangling locks that might be left when a git process crashes.

Right now, it appears functional. There are, however, some points that I'd like your feedback on before I proceed.

When running on Linux, it is possible to detect if a process has been killed (its exit code should be >128). I'm not sure how to do that on other platforms (e.g., windows or macOS). This is critical because if we release the locks every time a git process fails, we might release those of live processes and end up with broken repositories.

Another option is to execute the command and, if there is an error, check for dangling locks older than an specific threshold before retrying. In the PoC I've hard-coded the threshold to 1 hour, but I think the final version should allow the user to configure a threshold that makes more sense. This approach would allows us to skip the need of having a batch job that look for dangling locks.

If you guys are ok with how this is being handled, I can deploy it in my workplace and see how she behaves and then provide you guys feedback.

How does that sound to you guys?

P.S. In the PR, both approaches are combined.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Nov 12, 2024
@pull-request-size pull-request-size bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Nov 12, 2024
@github-actions github-actions bot added the modifies/go Pull requests that update Go code label Nov 12, 2024
@bsofiato bsofiato marked this pull request as draft November 12, 2024 21:22
return nil
} else {
log.Trace("Checking if repository %s is locked [lock threshold is %s]", repoPath, threshold)
return filepath.Walk(repoPath, func(filePath string, fileInfo os.FileInfo, err error) error {
Copy link
Member

Choose a reason for hiding this comment

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

I don't think Walk is necessary since the lock files have a fixed relative path.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lunny, Sadly I think we actually might :(

I've look at the source of gitlab referenced here and it appears that each refs can have its own lock (I've pasted the corresponding code snippet to make it easier for us to discuss)

image

@github-actions github-actions bot added the docs-update-needed The document needs to be updated synchronously label Nov 15, 2024
Signed-off-by: Bruno Sofiato <bruno.sofiato@gmail.com>
@bsofiato bsofiato marked this pull request as ready for review November 15, 2024 17:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs-update-needed The document needs to be updated synchronously lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. modifies/go Pull requests that update Go code size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pull requests stuck in "Merge conflict checking is in progress. Try again in few moments"
3 participants