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

Failed to update size for repository by a race condition #22386

Closed
fsologureng opened this issue Jan 9, 2023 · 1 comment · Fixed by #22392
Closed

Failed to update size for repository by a race condition #22386

fsologureng opened this issue Jan 9, 2023 · 1 comment · Fixed by #22392
Labels

Comments

@fsologureng
Copy link
Contributor

Description

2023/01/06 22:05:35 ...s/repository/push.go:100:pushUpdates() [E] [63b89b2f-14] Failed to update size for repository: updateSize: lstat /mnt/ceph-cluster/git/gitea-repositories/<user>/blog.git/refs/heads/<somefile>.lock: no such file or directory

There is a race condition regarding updating the repo size while processing pushes.

It is observed:

  1. there is no protection in func GetDirectorySize regarding the existence of the path. If the path does not exist, a response of size 0 would be a good response so as not to cause an error. For example, this error 500 is thrown due to bad code in the caller.
  2. There is no exclusion for lock files. If concurrency is not even implemented, then a straightforward solution is to exclude lock files (their existence implying a not handled race condition), allowing the wrong size to be fetched, in favour of not failing an important thread just because of the size calculation. filepath.WalkFunc could handle this case by checking the os.Lstat error and the lock suffix of the filename. For example this error handling is done by another very similar code with the suggested protection but even more general than excluding non-existing lock files.
  3. The Go developers suggest using filepath.WalkDir instead of filepath.Walk because *Walk is less efficient than WalkDir, introduced in Go 1.16, which avoids calling os.Lstat on every file or directory visited. The call uses a different walk function, but is very easy to adapt.

Originally reported in https://codeberg.org/forgejo/forgejo/issues/216

Gitea Version

1.18.0

Can you reproduce the bug on the Gitea demo site?

No

Log Gist

No response

Screenshots

No response

Git Version

No response

Operating System

No response

How are you running Gitea?

systemd

Database

None

@lunny
Copy link
Member

lunny commented Jan 10, 2023

Take a look at the lock file comment. https://github.com/git/git/blob/2a97289ad8b103625d3a1a12f66c27f50df822ce/lockfile.h#L9-L16
Because one go routine are calculating the file size and another are writing some files. I think we can simple ignore a *.lock file not exist. And filepath.WalkDir is a better way to do walk. Some code are from old Golang, so they are still using filepath.Walk.

zeripath pushed a commit that referenced this issue Jan 13, 2023
Fix #22386 

`GetDirectorySize` moved as `getDirectorySize` because it becomes a
special function which should not be put in `util`.

Co-authored-by: Jason Song <i@wolfogre.com>
zeripath pushed a commit to zeripath/gitea that referenced this issue Jan 16, 2023
Backport go-gitea#22392

Fix go-gitea#22386

`GetDirectorySize` moved as `getDirectorySize` because it becomes a
special function which should not be put in `util`.

Co-authored-by: Jason Song <i@wolfogre.com>
jolheiser pushed a commit that referenced this issue Jan 16, 2023
Backport #22392

Fix #22386

`GetDirectorySize` moved as `getDirectorySize` because it becomes a
special function which should not be put in `util`.

Co-authored-by: Jason Song <i@wolfogre.com>
@go-gitea go-gitea locked and limited conversation to collaborators May 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants