patch, 2 partsmacos: safeguard git maintenance against highly concurrent operations

When we started porting Scalar to C, one of the first things we did was to run Scalar's quite extensive set of functional tests. And there, we ran into immediate problems in the macOS job because git maintenance was registering a large number of repositories concurrently, and our code could be more robust in such scenarios.

The culprit lies not with Scalar, of course, but with the way git maintenance wants to write the plist for use by launchctl and register it, every time, and if two concurrent processes try to do that, they stumble over each other.

This pair of patches makes git maintenance much less fragile in those situations.

Please note that this patch series conflicts with lh/systemd-timers, although in a trivial way: the latter changes the signature of launchctl_schedule_plist() to lose its cmd parameter. The resolution is to adjust the conflicting code to lose the cmd parameter, and also drop it from launchctl_list_contains_plist() (and define it in the same way as launchctl_boot_plist() does). I assume that lh/systemd-timers will advance to next pretty soon; I plan on rebasing this patch series on top of it at that stage.

Derrick Stolee (1):
  maintenance: skip bootout/bootstrap when plist is registered

Johannes Schindelin (1):
  maintenance: create `launchctl` configuration using a lock file

 builtin/gc.c           | 91 ++++++++++++++++++++++++++++++++----------
 t/t7900-maintenance.sh | 17 ++++++++
 2 files changed, 87 insertions(+), 21 deletions(-)

base-commit: 48bf2fa8bad054d66bd79c6ba903c89c704201f7
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1024%2Fdscho%2Fmaintenance%2Flaunchctl-concurrent-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1024/dscho/maintenance/launchctl-concurrent-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1024

From: Johannes Schindelin <johannes.schindelin@gmx.de>

When two `git maintenance` processes try to write the `.plist` file, we need to help them with serializing their efforts.

The 150ms time-out value was determined from thin air.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/gc.c | 47 ++++++++++++++++++++++++++++++-----------------
 1 file changed, 30 insertions(+), 17 deletions(-)

From: Derrick Stolee <dstolee@microsoft.com>

On macOS, we use launchctl to manage the background maintenance schedule. This uses a set of .plist files to describe the schedule, but these files are also registered with 'launchctl bootstrap'. If multiple 'git maintenance start' commands run concurrently, then they can collide replacing these schedule files and registering them with launchctl.

To avoid extra launchctl commands, do a check for the .plist files on disk and check if they are registered using 'launchctl list <name>'. This command will return with exit code 0 if it exists, or exit code 113 if it does not.

We can test this behavior using the GIT_TEST_MAINT_SCHEDULER environment variable.

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/gc.c           | 54 +++++++++++++++++++++++++++++++++++-------
 t/t7900-maintenance.sh | 17 +++++++++++++
 2 files changed, 62 insertions(+), 9 deletions(-)

When the launchctl_boot_plist() function was added in a16eb6b1ff3 (maintenance: skip bootout/bootstrap when plist is registered, 2021-08-24), an unused call to launchctl_get_uid() was added along with it. That call appears to have been copy/pasted from launchctl_boot_plist().

Since we can remove that, we can also get rid of the "result" variable, whose only purpose was allow for the free() between its assignment and the return. That pattern also appears to have been copy/pasted from launchctl_boot_plist().

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---

I happen to have a local topic that refactored the launchctl_get_uid() function away, that didn't compile with the updated "master", I figured I'd need to add it back since it had a new user, but as it turns out that hopefully won't be needed.

 builtin/gc.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

Hi Ævar,

On Thu, 9 Sep 2021, Ævar Arnfjörð Bjarmason wrote:

I don't find the most crucial information in that commit message: what is the fall-out of the removal of this call?

Such an analysis (_with_ a summary of it in the commit message) is definitely required. And it should not be left as an exercise for the reader.

Ciao, Johannes

On Thu, Sep 09 2021, Johannes Schindelin wrote:

Do you mean an assurance to the reader that the removed code doesn't have any side-effects? E.g. an addition of

    As the patch shows the returned value wasn't used at all in this
    function, the launchctl_get_uid() function itself just calls
    xstrfmt() and getuid(), neither of which have any subtle global
    side-effects, so this removal is safe.

?

Hi Ævar,

On Thu, 9 Sep 2021, Ævar Arnfjörð Bjarmason wrote:

Yes. You want to refrain from forcing every reader to have to go look at the definition of that function at that revision. The accumulated time spent tallies up rather in disfavor of doing the work diligently on the contributor's side and save every reader some time. I mean, you forced me to spend the time, and then to spend more time to point out the missing analysis, and then you provided the paragraph as a question, forcing me to spend even more time on answering. All this time could have been saved in the first place. In this instance, it is too late to do anything about it. But I'm sure you plan on contributing other patches. Hopefully it will be more efficient next time.

Ciao, Johannes

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

While I very much like to see that you subscribe to the principle, and I do wish that the "question" were posed as a comment after the three-dash-line in a rerolled patch that is written under the assumption that the answer to the question is "yes", which would have saved one extra roundtrip, I do not think the question was not something to be scolded like the above.

All writers tend to assume that their readers know more than they do, and that is where the "do not force the reader to know or go look at things if they don't" principle comes from. But you're assuming that your "what's the fallout?" question did not need clarification, but it is understandable if it weren't.

Thanks.

When the launchctl_boot_plist() function was added in a16eb6b1ff3 (maintenance: skip bootout/bootstrap when plist is registered, 2021-08-24), an unused call to launchctl_get_uid() was added along with it. That call appears to have been copy/pasted from launchctl_boot_plist().

Since we can remove that, we can also get rid of the "result" variable, whose only purpose was allow for the free() between its assignment and the return. That pattern also appears to have been copy/pasted from launchctl_boot_plist().

As the patch shows the returned value from launchctl_get_uid() wasn't used at all in this function. The launchctl_get_uid() function itself just calls xstrfmt() and getuid(), neither of which have any subtle global side-effects, so this removal is safe.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---

Addresses a comment about the clarity of the commit message in v1: https://lore.kernel.org/git/87bl52dkv1.fsf@evledraar.gmail.com/

Range-diff against v1:
1:  93adb856b0c ! 1:  794e68e7722 gc: remove unused launchctl_get_uid() call
    @@ Commit message
         assignment and the return. That pattern also appears to have been
         copy/pasted from launchctl_boot_plist().
     
    +    As the patch shows the returned value from launchctl_get_uid() wasn't
    +    used at all in this function. The launchctl_get_uid() function itself
    +    just calls xstrfmt() and getuid(), neither of which have any subtle
    +    global side-effects, so this removal is safe.
    +
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
      ## builtin/gc.c ##

 builtin/gc.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> writes:

Sounds like a plan.

Here is my attempt to merge lh/systemd-timers into the result of applying these two to 'master', with focus on the top part of the launchctl_schedule_plist(). Sanity-checking is appreciated.

diff --cc builtin/gc.c
index 22e670b508,6a57d0fde5..0000000000
--- i/builtin/gc.c
+++ w/builtin/gc.c
@@@ -1593,48 -1678,26 +1678,50 @@@ static int launchctl_remove_plist(enum 
  	return result;
  }
  
- static int launchctl_remove_plists(const char *cmd)
+ static int launchctl_remove_plists(void)
  {
- 	return launchctl_remove_plist(SCHEDULE_HOURLY, cmd) ||
- 		launchctl_remove_plist(SCHEDULE_DAILY, cmd) ||
- 		launchctl_remove_plist(SCHEDULE_WEEKLY, cmd);
+ 	return launchctl_remove_plist(SCHEDULE_HOURLY) ||
+ 	       launchctl_remove_plist(SCHEDULE_DAILY) ||
+ 	       launchctl_remove_plist(SCHEDULE_WEEKLY);
  }
  
 +static int launchctl_list_contains_plist(const char *name, const char *cmd)
 +{
 +	int result;
 +	struct child_process child = CHILD_PROCESS_INIT;
 +	char *uid = launchctl_get_uid();
 +
 +	strvec_split(&child.args, cmd);
 +	strvec_pushl(&child.args, "list", name, NULL);
 +
 +	child.no_stderr = 1;
 +	child.no_stdout = 1;
 +
 +	if (start_command(&child))
 +		die(_("failed to start launchctl"));
 +
 +	result = finish_command(&child);
 +
 +	free(uid);
 +
 +	/* Returns failure if 'name' doesn't exist. */
 +	return !result;
 +}
 +
- static int launchctl_schedule_plist(const char *exec_path, enum schedule_priority schedule, const char *cmd)
+ static int launchctl_schedule_plist(const char *exec_path, enum schedule_priority schedule)
  {
 -	FILE *plist;
 -	int i;
 +	int i, fd;
  	const char *preamble, *repeat;
  	const char *frequency = get_frequency(schedule);
  	char *name = launchctl_service_name(frequency);
  	char *filename = launchctl_service_filename(name);
 +	struct lock_file lk = LOCK_INIT;
 +	static unsigned long lock_file_timeout_ms = ULONG_MAX;
 +	struct strbuf plist = STRBUF_INIT, plist2 = STRBUF_INIT;
 +	struct stat st;
++	const char *cmd = "launchctl";
  
 -	if (safe_create_leading_directories(filename))
 -		die(_("failed to create directories for '%s'"), filename);
 -	plist = xfopen(filename, "w");
 -
++	get_schedule_cmd(&cmd, NULL);
  	preamble = "<?xml version=\"1.0\"?>\n"
  		   "<!DOCTYPE plist PUBLIC \"-//Apple//DTD PLIST 1.0//EN\" \"http://www.apple.com/DTDs/PropertyList-1.0.dtd\">\n"
  		   "<plist version=\"1.0\">"
@@@ -1687,38 -1750,13 +1774,38 @@@
  		/* unreachable */
  		break;
  	}
 -	fprintf(plist, "</array>\n</dict>\n</plist>\n");
 -	fclose(plist);
 +	strbuf_addstr(&plist, "</array>\n</dict>\n</plist>\n");
  
 -	/* bootout might fail if not already running, so ignore */
 -	launchctl_boot_plist(0, filename);
 -	if (launchctl_boot_plist(1, filename))
 -		die(_("failed to bootstrap service %s"), filename);
 +	if (safe_create_leading_directories(filename))
 +		die(_("failed to create directories for '%s'"), filename);
 +
 +	if ((long)lock_file_timeout_ms < 0 &&
 +	    git_config_get_ulong("gc.launchctlplistlocktimeoutms",
 +				 &lock_file_timeout_ms))
 +		lock_file_timeout_ms = 150;
 +
 +	fd = hold_lock_file_for_update_timeout(&lk, filename, LOCK_DIE_ON_ERROR,
 +					       lock_file_timeout_ms);
 +
 +	/*
 +	 * Does this file already exist? With the intended contents? Is it
 +	 * registered already? Then it does not need to be re-registered.
 +	 */
 +	if (!stat(filename, &st) && st.st_size == plist.len &&
 +	    strbuf_read_file(&plist2, filename, plist.len) == plist.len &&
 +	    !strbuf_cmp(&plist, &plist2) &&
 +	    launchctl_list_contains_plist(name, cmd))
 +		rollback_lock_file(&lk);
 +	else {
 +		if (write_in_full(fd, plist.buf, plist.len) < 0 ||
 +		    commit_lock_file(&lk))
 +			die_errno(_("could not write '%s'"), filename);
 +
 +		/* bootout might fail if not already running, so ignore */
- 		launchctl_boot_plist(0, filename, cmd);
- 		if (launchctl_boot_plist(1, filename, cmd))
++		launchctl_boot_plist(0, filename);
++		if (launchctl_boot_plist(1, filename))
 +			die(_("failed to bootstrap service %s"), filename);
 +	}
  
  	free(filename);
  	free(name);

Hi Junio,

On Tue, 24 Aug 2021, Junio C Hamano wrote:

My local version (hence `git reset -hard`'ed away) looked almost precisely like yours, I only added the definition of `cmd` to the top of `launchctl_list_contains_plist()` and removed its `cmd` parameter (and adjusted the callers). But that's the only difference I can spot.

Thanks, Dscho