The maintenance tasks will never run if maintenance.lock is accidentally not deleted

In my work, I found that the prefetch task of maintenance often failed, causing the fetch command to take a long time to execute in monorepo.

After investigation, it was found that the maintenance.lock file was not deleted correctly for various reasons, resulting in the inability to trigger subsequent maintenance tasks.

The maintenance task needs to be executed in the background for a long time. Therefore, due to various reasons, the maintenance.lock file is easily not deleted when the process exits abnormally. Unlike ref.lock, which will have an error prompt in the fetch command, maintenance will only fail silently, which makes it difficult for most people to find out where the problem is.

So is it recommended to add some mechanism to ensure that maintenance.lock can be correctly restored when it is not deleted? For example, add pid information to maintenance.lock, or add a lock timeout mechanism.

I'm not sure if I missed any information, but if this is feasible, I would be happy to contribute such a patch.

Thanks. liuzhongbo

On 9/19/24 8:40 AM, 刘钟博 wrote:

This is unfortunately a common occurrence. It seems to be related to the Git background processes being killed in such a way that does not allow the standard lock cleanup mechanism to kick in.

At least, I haven't been able to find a reason why Git would be failing with something like a segfault which would also cause leftover .lock files.

I can speak from experience of previously having a lock timeout that this could cause problems where maintenance processes start running on the same repo concurrently. The reason for this in the past was due to being blocked on credential manager prompts.

I was vaguely remembering fixing that issue with credential prompts, but then realized the change was only made to microsoft/git [1]. That same change reverted the removal of "stale" .lock files.

I should put this together for an upstream patch series, finally.

[1] https://github.com/microsoft/git/pull/598

I will CC you on my submission for the credential changes, as a way to help introduce you to the code in this area.

Thanks, -Stolee

Thank you for your response. On Thu, Sep 19, 2024 at 8:56 PM Derrick Stolee <stolee@gmail.com> wrote:

Yes, it is not necessarily a problem caused by git failure. I think it is a natural shortcoming of file existence lock, which cannot guarantee that the lock will be released when the process exits abnormally.

I read your commit and explored more. Perhaps the file locks provided by the systems are a better choice, such as fcntl() on POSIX and LockFileEx() on Windows. They can be automatically released when the process exits abnormally. If there are no objections, I'll give it a try and send a patch in a few days.

Thanks Liuzhongbo

On 9/23/24 6:26 AM, 刘钟博 wrote:
 > Thank you for your response.
 > On Thu, Sep 19, 2024 at 8:56 PM Derrick Stolee <stolee@gmail.com> wrote:
 >> At least, I haven't been able to find a reason why Git would be
 >> failing with something like a segfault which would also cause leftover
 >> .lock files.
 > Yes, it is not necessarily a problem caused by git failure. I think it
 > is a natural
 > shortcoming of file existence lock, which cannot guarantee that the lock will be
 > released when the process exits abnormally.
 >
 >> I can speak from experience of previously having a lock timeout
 >> that this could cause problems where maintenance processes start
 >> running on the same repo concurrently.
 >> [1] https://github.com/microsoft/git/pull/598
 > I read your commit and explored more. Perhaps the file locks provided by the
 > systems are a better choice, such as fcntl() on POSIX and LockFileEx()
 > on Windows.
 > They can be automatically released when the process exits abnormally.
 > If there are
 > no objections, I'll give it a try and send a patch in a few days.

I'm curious to see how this would be implemented across platforms.

I think the maintenance.lock file is a particular case where such a change would be welcome, because we are not writing to it and then doing a rename over the non-.lock version (like we do with many other files).

For that reason, you would be creating a new API construct, not replacing the existing lockfile API.

Thanks, -Stolee