{"thread":{"id":"62514","subject":"[PATCH] builtin/gc: provide hint when maintenance hits a stale schedule lock","startedAt":"2024-11-19T10:50:32Z","lastAt":"2024-11-25T11:52:48Z","messageCount":5,"participants":["Patrick Steinhardt","Justin Tobler","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"507583","messageId":"20241119-pks-maintenance-hint-with-stale-lock-v1-1-f9f9a98e12a0@pks.im","threadId":"62514","inReplyTo":null,"subject":"[PATCH] builtin/gc: provide hint when maintenance hits a stale schedule lock","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-11-19T10:48:43Z","receivedAt":"2024-11-19T10:50:32Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When running scheduled maintenance via `git maintenance start`, we\nacquire a lockfile to ensure that no other scheduled maintenance task is\nrunning in the repository concurrently. If so, we do provide an error to\nthe user hinting that another process seems to be running in this repo.\n\nThere are two important cases why such a lockfile may exist:\n\n  - An actual git-maintenance(1) process is still running in this\n    repository.\n\n  - An earlier process may have crashed or was interrupted part way\n    through and has left a stale lockfile behind.\n\nIn c95547a394 (builtin/gc: fix crash when running `git maintenance\nstart`, 2024-10-10), we have fixed an issue where git-maintenance(1)\nwould crash with the \"start\" subcommand, and the underlying bug causes\nthe second scenario to trigger quite often now.\n\nMost users don't know how to get out of that situation again though.\nIdeally, we'd be removing the stale lock for our users automatically.\nBut in the context of repository maintenance this is rather risky, as it\ncan easily run for hours or even days. So finding a clear point where we\nknow that the old process has exited is basically impossible.\n\nWe have the same issue in other subsystems, e.g. when locking refs. Our\nlockfile interfaces thus provide the `unable_to_lock_message()` function\nfor exactly this purpose: it provides a nice hint to the user that\nexplains what is going on and how to get out of that situation again by\nmanually removing the file.\n\nAdapt git-maintenance(1) to print a similar hint. While we could use the\nabove function, we can provide a bit more context as we know exactly\nwhat kind of process would create the lockfile.\n\nReported-by: Miguel Rincon Barahona <mrincon@gitlab.com>\nReported-by: Kev Kloss <kkloss@gitlab.com>\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\nHi,\n\nthis issue was reported to me internally at GitLab with the suggestion\nof providing a hint for how to get out of the situation again.\n\nPatrick\n---\n builtin/gc.c           | 11 ++++++++++-\n t/t7900-maintenance.sh |  8 ++++++++\n 2 files changed, 18 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex d52735354c9f87ba4e8acb593dd11aa0482223e1..34848626e47c777112994e5b5c558b65952ac8c2 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -2890,8 +2890,17 @@ static int update_background_schedule(const struct maintenance_start_opts *opts,\n \tchar *lock_path = xstrfmt(\"%s/schedule\", the_repository->objects->odb->path);\n \n \tif (hold_lock_file_for_update(&lk, lock_path, LOCK_NO_DEREF) < 0) {\n+\t\tif (errno == EEXIST)\n+\t\t\terror(_(\"unable to create '%s.lock': %s.\\n\\n\"\n+\t\t\t    \"Another scheduled git-maintenance(1) process seems to be running in this\\n\"\n+\t\t\t    \"repository. Please make sure no other maintenance processes are running and\\n\"\n+\t\t\t    \"then try again. If it still fails, a git-maintenance(1) process may have\\n\"\n+\t\t\t    \"crashed in this repository earlier: remove the file manually to continue.\"),\n+\t\t\t    absolute_path(lock_path), strerror(errno));\n+\t\telse\n+\t\t\terror_errno(_(\"cannot acquire lock for scheduled background maintenance\"));\n \t\tfree(lock_path);\n-\t\treturn error(_(\"another process is scheduling background maintenance\"));\n+\t\treturn -1;\n \t}\n \n \tfor (i = 1; i < ARRAY_SIZE(scheduler_fn); i++) {\ndiff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh\nindex c224c8450c85f567bc29258e18b4a59fe6682f0a..6d6ffaaf376bdbadecdb23a460994af1d218dc19 100755\n--- a/t/t7900-maintenance.sh\n+++ b/t/t7900-maintenance.sh\n@@ -1011,4 +1011,12 @@ test_expect_success 'repacking loose objects is quiet' '\n \t)\n '\n \n+test_expect_success 'maintenance aborts with existing lock file' '\n+\ttest_when_finished \"rm -rf repo\" &&\n+\tgit init repo &&\n+\t: >repo/.git/objects/schedule.lock &&\n+\ttest_must_fail git -C repo maintenance start 2>err &&\n+\ttest_grep \"Another scheduled git-maintenance(1) process seems to be running\" err\n+'\n+\n test_done\n\n---\nbase-commit: 090d24e9af6e9f59c3f7bee97c42bb1ae3c7f559\nchange-id: 20241119-pks-maintenance-hint-with-stale-lock-35e5417d632b\n\n"},{"id":"507634","messageId":"neuwazkp5ca3mumpqezxsgm3ypb4zron3azfyuzwwf4soivrqi@wun5mz4c7vrm","threadId":"62514","inReplyTo":"20241119-pks-maintenance-hint-with-stale-lock-v1-1-f9f9a98e12a0@pks.im","subject":"Re: [PATCH] builtin/gc: provide hint when maintenance hits a stale schedule lock","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2024-11-19T17:17:24Z","receivedAt":"2024-11-19T17:19:14Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"On 24/11/19 11:48AM, Patrick Steinhardt wrote:\n> When running scheduled maintenance via `git maintenance start`, we\n> acquire a lockfile to ensure that no other scheduled maintenance task is\n> running in the repository concurrently. If so, we do provide an error to\n> the user hinting that another process seems to be running in this repo.\n> \n> There are two important cases why such a lockfile may exist:\n> \n>   - An actual git-maintenance(1) process is still running in this\n>     repository.\n> \n>   - An earlier process may have crashed or was interrupted part way\n>     through and has left a stale lockfile behind.\n> \n> In c95547a394 (builtin/gc: fix crash when running `git maintenance\n> start`, 2024-10-10), we have fixed an issue where git-maintenance(1)\n> would crash with the \"start\" subcommand, and the underlying bug causes\n> the second scenario to trigger quite often now.\n> \n> Most users don't know how to get out of that situation again though.\n> Ideally, we'd be removing the stale lock for our users automatically.\n> But in the context of repository maintenance this is rather risky, as it\n> can easily run for hours or even days. So finding a clear point where we\n> know that the old process has exited is basically impossible.\n\nIndeed, providing a hint to help affected users here make sense to me.\n\n> \n> We have the same issue in other subsystems, e.g. when locking refs. Our\n> lockfile interfaces thus provide the `unable_to_lock_message()` function\n> for exactly this purpose: it provides a nice hint to the user that\n> explains what is going on and how to get out of that situation again by\n> manually removing the file.\n> \n> Adapt git-maintenance(1) to print a similar hint. While we could use the\n> above function, we can provide a bit more context as we know exactly\n> what kind of process would create the lockfile.\n\nThe added context provided by having a more specific message seems\nappropriate. Looking at the message provided through\n`unable_to_lock_message()`, I could see the generic example it provides\nbeing a bit confusing here.\n\n> \n> Reported-by: Miguel Rincon Barahona <mrincon@gitlab.com>\n> Reported-by: Kev Kloss <kkloss@gitlab.com>\n> Signed-off-by: Patrick Steinhardt <ps@pks.im>\n> ---\n> Hi,\n> \n> this issue was reported to me internally at GitLab with the suggestion\n> of providing a hint for how to get out of the situation again.\n> \n> Patrick\n> ---\n>  builtin/gc.c           | 11 ++++++++++-\n>  t/t7900-maintenance.sh |  8 ++++++++\n>  2 files changed, 18 insertions(+), 1 deletion(-)\n> \n> diff --git a/builtin/gc.c b/builtin/gc.c\n> index d52735354c9f87ba4e8acb593dd11aa0482223e1..34848626e47c777112994e5b5c558b65952ac8c2 100644\n> --- a/builtin/gc.c\n> +++ b/builtin/gc.c\n> @@ -2890,8 +2890,17 @@ static int update_background_schedule(const struct maintenance_start_opts *opts,\n>  \tchar *lock_path = xstrfmt(\"%s/schedule\", the_repository->objects->odb->path);\n>  \n>  \tif (hold_lock_file_for_update(&lk, lock_path, LOCK_NO_DEREF) < 0) {\n> +\t\tif (errno == EEXIST)\n> +\t\t\terror(_(\"unable to create '%s.lock': %s.\\n\\n\"\n> +\t\t\t    \"Another scheduled git-maintenance(1) process seems to be running in this\\n\"\n> +\t\t\t    \"repository. Please make sure no other maintenance processes are running and\\n\"\n> +\t\t\t    \"then try again. If it still fails, a git-maintenance(1) process may have\\n\"\n> +\t\t\t    \"crashed in this repository earlier: remove the file manually to continue.\"),\n> +\t\t\t    absolute_path(lock_path), strerror(errno));\n> +\t\telse\n> +\t\t\terror_errno(_(\"cannot acquire lock for scheduled background maintenance\"));\n>  \t\tfree(lock_path);\n> -\t\treturn error(_(\"another process is scheduling background maintenance\"));\n> +\t\treturn -1;\n>  \t}\n>  \n>  \tfor (i = 1; i < ARRAY_SIZE(scheduler_fn); i++) {\n> diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh\n> index c224c8450c85f567bc29258e18b4a59fe6682f0a..6d6ffaaf376bdbadecdb23a460994af1d218dc19 100755\n> --- a/t/t7900-maintenance.sh\n> +++ b/t/t7900-maintenance.sh\n> @@ -1011,4 +1011,12 @@ test_expect_success 'repacking loose objects is quiet' '\n>  \t)\n>  '\n>  \n> +test_expect_success 'maintenance aborts with existing lock file' '\n> +\ttest_when_finished \"rm -rf repo\" &&\n> +\tgit init repo &&\n> +\t: >repo/.git/objects/schedule.lock &&\n> +\ttest_must_fail git -C repo maintenance start 2>err &&\n> +\ttest_grep \"Another scheduled git-maintenance(1) process seems to be running\" err\n> +'\n> +\n>  test_done\n> \n> ---\n> base-commit: 090d24e9af6e9f59c3f7bee97c42bb1ae3c7f559\n> change-id: 20241119-pks-maintenance-hint-with-stale-lock-35e5417d632b\n\nThe changes in this patch are straightforward and look good to me :)\n\n-Justin\n"},{"id":"507934","messageId":"20241122153033.GA1737136@coredump.intra.peff.net","threadId":"62514","inReplyTo":"20241119-pks-maintenance-hint-with-stale-lock-v1-1-f9f9a98e12a0@pks.im","subject":"Re: [PATCH] builtin/gc: provide hint when maintenance hits a stale schedule lock","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-11-22T15:30:33Z","receivedAt":"2024-11-22T15:30:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 19, 2024 at 11:48:43AM +0100, Patrick Steinhardt wrote:\n\n> diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh\n> index c224c8450c85f567bc29258e18b4a59fe6682f0a..6d6ffaaf376bdbadecdb23a460994af1d218dc19 100755\n> --- a/t/t7900-maintenance.sh\n> +++ b/t/t7900-maintenance.sh\n> @@ -1011,4 +1011,12 @@ test_expect_success 'repacking loose objects is quiet' '\n>  \t)\n>  '\n>  \n> +test_expect_success 'maintenance aborts with existing lock file' '\n> +\ttest_when_finished \"rm -rf repo\" &&\n> +\tgit init repo &&\n> +\t: >repo/.git/objects/schedule.lock &&\n> +\ttest_must_fail git -C repo maintenance start 2>err &&\n> +\ttest_grep \"Another scheduled git-maintenance(1) process seems to be running\" err\n> +'\n\nThis failed for me in all of the macos jobs of GitHub CI. Looks like the\nerr file contains:\n\n  fatal: launchctl scheduler is not available\n\nSo maybe it's a platform image issue?\n\n-Peff\n"},{"id":"508012","messageId":"eb504a75147d8609be66e131d239a69d192d693d.1732511129.git.ps@pks.im","threadId":"62514","inReplyTo":"20241122153033.GA1737136@coredump.intra.peff.net","subject":"[PATCH] t7900: fix host-dependent behaviour when testing git-maintenance(1)","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-11-25T05:33:41Z","receivedAt":"2024-11-25T05:33:59Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"We have recently added a new test to t7900 that exercises whether\ngit-maintenance(1) fails as expected when the \"schedule.lock\" file\nexists. The test depends on whether or not the host has the required\nexecutables present to schedule maintenance tasks in the first place,\nlike systemd or launchctl -- if not, the test fails with an unrelated\nerror before even checking for the lock file. This fails for example in\nour CI systems, where macOS images do not have launchctl available.\n\nFix this issue by creating a stub systemctl(1) binary and using the\nsystemd scheduler.\n\nReported-by: Jeff King <peff@peff.net>\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n\nOh, true, thanks for reporting. This here should fix it.\n\nPatrick\n\n t/t7900-maintenance.sh | 9 +++++++--\n 1 file changed, 7 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh\nindex 6d6ffaaf37..4bbc171958 100755\n--- a/t/t7900-maintenance.sh\n+++ b/t/t7900-maintenance.sh\n@@ -1012,10 +1012,15 @@ test_expect_success 'repacking loose objects is quiet' '\n '\n \n test_expect_success 'maintenance aborts with existing lock file' '\n-\ttest_when_finished \"rm -rf repo\" &&\n+\ttest_when_finished \"rm -rf repo script\" &&\n+\tmkdir script &&\n+\twrite_script script/systemctl <<-\\EOF &&\n+\ttrue\n+\tEOF\n+\n \tgit init repo &&\n \t: >repo/.git/objects/schedule.lock &&\n-\ttest_must_fail git -C repo maintenance start 2>err &&\n+\ttest_must_fail env PATH=\"$PWD/script:$PATH\" git -C repo maintenance start --scheduler=systemd 2>err &&\n \ttest_grep \"Another scheduled git-maintenance(1) process seems to be running\" err\n '\n \n\nbase-commit: 2f2daa412d02135a1f1f00da3f424217467f8d16\n-- \n2.47.0.274.g962d0b743d.dirty\n\n"},{"id":"508095","messageId":"20241125115247.GA1070119@coredump.intra.peff.net","threadId":"62514","inReplyTo":"eb504a75147d8609be66e131d239a69d192d693d.1732511129.git.ps@pks.im","subject":"Re: [PATCH] t7900: fix host-dependent behaviour when testing git-maintenance(1)","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-11-25T11:52:47Z","receivedAt":"2024-11-25T11:52:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Nov 25, 2024 at 06:33:41AM +0100, Patrick Steinhardt wrote:\n\n> We have recently added a new test to t7900 that exercises whether\n> git-maintenance(1) fails as expected when the \"schedule.lock\" file\n> exists. The test depends on whether or not the host has the required\n> executables present to schedule maintenance tasks in the first place,\n> like systemd or launchctl -- if not, the test fails with an unrelated\n> error before even checking for the lock file. This fails for example in\n> our CI systems, where macOS images do not have launchctl available.\n> \n> Fix this issue by creating a stub systemctl(1) binary and using the\n> systemd scheduler.\n\nThanks, this explanation makes sense and clears up the CI issues I saw.\n\n>  test_expect_success 'maintenance aborts with existing lock file' '\n> -\ttest_when_finished \"rm -rf repo\" &&\n> +\ttest_when_finished \"rm -rf repo script\" &&\n> +\tmkdir script &&\n> +\twrite_script script/systemctl <<-\\EOF &&\n> +\ttrue\n> +\tEOF\n> +\n>  \tgit init repo &&\n>  \t: >repo/.git/objects/schedule.lock &&\n> -\ttest_must_fail git -C repo maintenance start 2>err &&\n> +\ttest_must_fail env PATH=\"$PWD/script:$PATH\" git -C repo maintenance start --scheduler=systemd 2>err &&\n>  \ttest_grep \"Another scheduled git-maintenance(1) process seems to be running\" err\n>  '\n\nAs always, I am never sure whether to use $PWD or $(pwd) for the benefit\nof Windows. But digging up this message:\n\n  https://lore.kernel.org/git/2b69d098-92ef-77b0-367a-516e9edbe257@kdbg.org/\n\nI think $PWD is right here.\n\n-Peff\n"}]}