Maintenance locks and interactivity #598

derrickstolee · 2023-08-17T19:53:22Z

This is an early version of work ~~already under review upstream~~.
This change only applies to interactions with Azure DevOps and the
GVFS Protocol.
This change only applies to the virtualization hook and VFS for Git.

This set of patches has a little of both:

We need to revert a custom change around maintenance.lock files that was only in microsoft/git. The "fix" actually caused a worse situation where many background maintenance processes would pile up.
Create the credential.interactive config as an official Git config and use it in Git to prevent any chance of a foreground username/password request. (This is borrowed from GCM, which already respects this.)
To help avoid problems, especially when blocked by credentials, add configuration to the background schedule to avoid interactive prompts.
To make sure that the new config options are set up in the background schedule, update scalar reconfigure to execute git maintenance start.

Points 2-4 could maybe to upstream, but we shouldn't wait for that.

I've tested these features locally on Linux and Windows and double-checked that they will prevent the backup of maintenance processes when the credentials become invalid.

jeffhostetler · 2023-08-17T20:11:21Z

builtin/gc.c

@@ -1678,6 +1657,9 @@ static const char *get_frequency(enum schedule_priority schedule)
 	}
 }

+static const char *custom_background_config =
+	"-c credential.interactive=false -c cred.askPass=false";
+


Just a nit. As an alternative to defining a static string here, you could #define a macro and use that in each of the fprintf() format strings below (like we do for the PRIuMAX or PRItime strings).

jeffhostetler · 2023-08-17T20:32:14Z

credential.c

+
+	if (!git_config_get_int("credential.timeout", &config_seconds) &&
+	    config_seconds >= 0)
+		select_timeout.tv_sec = config_seconds;


Am I reading this right? If credential.timeout is not defined in the config, we'll default to 5 minutes? So to avoid the timeout and get the legacy wait, we need to have credential.timeout=0 in the config. Not questioning, just confirming intent.

Yes, that's the intent.

Just a note for GCM.. this is a setting we should ignore ourselves :)
As in, GCM itself shouldn't have a timeout, since Git is the one doing the timeout and kill.

jeffhostetler · 2023-08-17T20:34:59Z

credential.c

+				return -1;
+			}
+		}
+


I'm assuming that if we get a single byte from GCM, we'll get the full response so we don't need to do something more fancy. GCM will either be stuck or not before the first byte is returned.

That's also my expectation. The response is not provided until after the "blocking" part of GCM is complete. @mjcheetham or @ldennington might be able to confirm for sure.

GCM only writes back to stdout once it has the complete result. If we've written anything back, we will soon exit.

jeffhostetler · 2023-08-17T20:54:47Z

credential.c

+
+			if (!FD_ISSET(helper.out, &readfds)) {
+				/* Timeout complete before helper.out has bytes to read. */
+				kill_child_command(&helper);


This bothers me a bit. I'm wondering if we should just insert a kill() here and then let the existing call to finish_command() at the bottom wait for the child. The existing finish code will end up calling wait_or_whine() which normalizes the child's exit code, so we'll be consistent. And then it'll emit
the child_exit and other cleanup.

You could also add a trace2_data after the kill() to log that we sent the signal, but that is not strictly necessary.

Also, then you won't need the kill_child_command() function at all. That might make upstream integration easier.

Interesting. I hadn't considered that, because I expected the behavior to be "kill the child then leave the method" and not to go to a later finish_command(). I will play with this when I have a better testing environment ready.

mjcheetham · 2023-08-18T18:46:36Z

builtin/gc.c

@@ -1678,6 +1657,9 @@ static const char *get_frequency(enum schedule_priority schedule)
 	}
 }

+static const char *custom_background_config =
+	"-c credential.interactive=false -c cred.askPass=false";


I wonder if it's worth trying to upstream the concept of credential.interactive, and have it mean "helpers should not interact with the user".

Right now, this is a GCM specific thing.

Alternatively, perhaps a core.interactive or core.background setting would make more sense? To indicate that "this instance of Git is running non-interactively or in the background", and that is a signal that helpers can pick up on to mean "no prompt"?

It might be simpler to have core.background as a single value. Then GCM or the ask-pass or whatever code could do the right thing without us having to enumerate the various flags here.

jeffhostetler · 2023-08-21T13:41:04Z

Does gc know enough about it's environment to know whether to set the background bit? That is, if gc is run by maintenance, then yes, but if gc is run by a foreground fetch, then no. Right??

jeffhostetler · 2023-08-21T13:45:35Z

Same question about credential.c WRT the timeout. If I interactively do a push/fetch and get stuck behind a cred prompt -- and have to dig my phone out of my backpack or while I go get coffee, should it always timeout/abort? Or should the timeout only be enabled in the maintenance case?

Do we still need the timeout if GCM respects the suggested core.background flag ??

derrickstolee · 2023-08-21T15:56:13Z

Same question about credential.c WRT the timeout. If I interactively do a push/fetch and get stuck behind a cred prompt -- and have to dig my phone out of my backpack or while I go get coffee, should it always timeout/abort? Or should the timeout only be enabled in the maintenance case?

I could consider leaving it as a non-timeout case for foreground and set the config in the maintenance scheduler (like we are already doing for the interactivity bit).

Do we still need the timeout if GCM respects the suggested core.background flag ??

Is that a thing? One thing I think happens is that background jobs don't have a TTY, so we won't get blocked on Git's request for a username/password (which during my local testing requires setting GIT_TERMINAL_PROMPT=0).

jeffhostetler · 2023-08-21T16:36:12Z

Same question about credential.c WRT the timeout. If I interactively do a push/fetch and get stuck behind a cred prompt -- and have to dig my phone out of my backpack or while I go get coffee, should it always timeout/abort? Or should the timeout only be enabled in the maintenance case?

I could consider leaving it as a non-timeout case for foreground and set the config in the maintenance scheduler (like we are already doing for the interactivity bit).

I'll wait for @mjcheetham opinion here, but I'm wondering if we want to change the foreground behavior here.

Do we still need the timeout if GCM respects the suggested core.background flag ??

Is that a thing? One thing I think happens is that background jobs don't have a TTY, so we won't get blocked on Git's request for a username/password (which during my local testing requires setting GIT_TERMINAL_PROMPT=0).

I'm not sure. There are too many child processes with STDIN/OUT bound to a pipe from the parent process for me to casually trust isatty(fd) ...

derrickstolee · 2023-08-21T17:21:38Z

Do we still need the timeout if GCM respects the suggested core.background flag ??

Is that a thing? One thing I think happens is that background jobs don't have a TTY, so we won't get blocked on Git's request for a username/password (which during my local testing requires setting GIT_TERMINAL_PROMPT=0).

I'm not sure. There are too many child processes with STDIN/OUT bound to a pipe from the parent process for me to casually trust isatty(fd) ...

What's even worse is that the Git prompt for username/password goes through git_terminal_prompt() which pulls the terminal directly from the environment, and ignores something like redirecting /dev/null into stdin.

derrickstolee · 2023-08-21T20:18:09Z

End-to-End Testing Report

After some local testing in Linux helped identify some issues, I generated a Windows installer and ran it on my Windows machine. Along with an earlier version of the installer, I was able to find out this information:

I had made a mistake of using the wrong config key name sometimes when I meant core.askpass. Further, using "false" results in some failures that are not helpful. Instead, using core.askpass=echo makes Git skip the feature and move on completely.
In addition to the core.askpass config key, there is a GIT_ASKPASS environment variable that overrides the config key. It's set by VS Code (at least in Remote SSH connections like I use).
Even when using an alternative core.askpass, Git has an interactive username/password request that goes through the terminal. See git_terminal_prompt() down deep below credential_getpass() for this info. This doesn't block when using a background job, since there is no terminal, but it makes testing challenging.
The previous two bullets might be good reasons to introduce credential.interactive upstream: we can block this behavior when the user is requesting it.
While the select() method works appropriately on Linux, on Windows it seems to be returning immediately as "the readfd is ready" instead of waiting for a byte of data to be sent. Because this isn't helping, I will most likely remove the timeout feature from this pull request.
However I was able to get things to work by using the -c credential.interactive=false -c core.askpass=echo options. These allow fetches to work when the credentials are valid, and the fetches complete (with expected failure) when the credentials are not valid. This will solve our problems with background fetches getting blocked on credentials.
In order to get these custom configs into the background jobs, we actually need the schedule to be updated during upgrade. I added a patch to make scalar reconfigure run git maintenance start in the necessary repos, giving us an updated schedule.

I need to update this branch with the full learnings here, and add some tests now that we understand the code necessary to get the features we need.

jeffhostetler · 2023-08-22T18:54:04Z

nit: typo in commit message of "add new interactive config option": carefult

This change from microsoft#468 is causing multiple maintenance processes to get blocked on credentials instead of only one. The change did more harm than good. This reverts commit 95ed7f6.

jeffhostetler · 2023-08-22T19:06:23Z

i didn't see anything else. thanks!

dscho

I am optimistic that this will address the reported problems.

A couple of feedback comments about the commit messages:

In the second commit's message: "caues" -> "cause", "modifed" -> "modified".
The third commit's message mentions "GCM" without prior explanation of the acronym; I would recommend using "Git Credential Manager" here instead.

dscho · 2023-08-23T06:24:57Z

credential.c

+	char *value;
+	if (!git_config_get_maybe_bool("credential.interactive", &interactive) &&
+	    !interactive)
+		return -1;


Do we want to trace the fact that the interactive credential prompt was skipped?

Interesting idea. I was thinking that the lack of the other region would be enough.

Do you propose using a trace2_printf()? or what kind of indicator? I'm not familiar with an example of this kind of tracing.

you could do something like a trace2_data_intmax(... "credential", "interactive/skipped", 1)

only log the true case.

t/t5551-http-fetch-smart.sh

builtin/gc.c

t/t9210-scalar.sh

When scripts or background maintenance wish to perform HTTP(S) requests, there is a risk that our stored credentials might be invalid. At the moment, this causes the credential helper to ping the user and block the process. Even if the credential helper does not ping the user, Git falls back to the 'askpass' method, which includes a direct ping to the user via the terminal. Even setting the 'core.askPass' config as something like 'echo' will causes Git to fallback to a terminal prompt. It uses git_terminal_prompt(), which finds the terminal from the environment and ignores whether stdin has been redirected. This can also block the process awaiting input. Create a new config option to prevent user interaction, favoring a failure to a blocked process. The chosen name, 'credential.interactive', is taken from the config option used by Git Credential Manager to already avoid user interactivity, so there is already one credential helper that integrates with this option. However, older versions of Git Credential Manager also accepted other string values, including 'auto', 'never', and 'always'. The modern use is to use a boolean value, but we should still be careful that some users could have these non-booleans. Further, we should respect 'never' the same as 'false'. This is respected by the implementation and test, but not mentioned in the documentation. The implementation for the Git interactions takes place within credential_getpass(). The method prototype is modified to return an 'int' instead of 'void'. This allows us to detect that no attempt was made to fill the given credential, changing the single caller slightly. Also, a new trace2 region is added around the interactive portion of the credential request. This provides a way to measure the amount of time spent in that region for commands that _are_ interactive. It also makes a conventient way to test that the config option works with 'test_region'. Signed-off-by: Derrick Stolee <derrickstolee@github.com>

At the moment, some background jobs are getting blocked on credentials during the 'prefetch' task. This leads to other tasks, such as incremental repacks, getting blocked. Further, if a user manages to fix their credentials, then they still need to cancel the background process before their background maintenance can continue working. Update the background schedules for our four scheduler integrations to include these config options via '-c' options: * 'credential.interactive=false' will stop Git and some credential helpers from prompting in the UI (assuming the '-c' parameters are carried through and respected by GCM). * 'core.askPass=true' will replace the text fallback for a username and password into the 'true' command, which will return a success in its exit code, but Git will treat the empty string returned as an invalid password and move on. We can do some testing that the credentials are passed, at least in the systemd case due to writing the service files. Signed-off-by: Derrick Stolee <derrickstolee@github.com>

The 'scalar reconfigure' command is intended to update registered repos with the latest settings available. However, up to now we were not reregistering the repos with background maintenance. In particular, this meant that the background maintenance schedule would not be updated if there are improvements between versions. Be sure to register repos for maintenance during the reconfigure step. Signed-off-by: Derrick Stolee <derrickstolee@github.com>

In this commit, we added the 'credential.interactive=never' option to unattended scalar options. This should be changed to 'false' to match the modern use of this config option. But also, we have a test that requires using askpass to get credentials, but the test is in unattended mode. Fix that test to include 'credential.interactive=true' to bypass this issue.

derrickstolee self-assigned this Aug 17, 2023

jeffhostetler reviewed Aug 17, 2023

View reviewed changes

jeffhostetler approved these changes Aug 17, 2023

View reviewed changes

derrickstolee had a problem deploying to release August 18, 2023 17:17 — with GitHub Actions Failure

derrickstolee had a problem deploying to release August 18, 2023 17:28 — with GitHub Actions Failure

derrickstolee had a problem deploying to release August 18, 2023 17:41 — with GitHub Actions Failure

derrickstolee had a problem deploying to release August 18, 2023 18:06 — with GitHub Actions Failure

derrickstolee had a problem deploying to release August 18, 2023 18:07 — with GitHub Actions Failure

derrickstolee had a problem deploying to release August 18, 2023 18:08 — with GitHub Actions Failure

derrickstolee had a problem deploying to release August 18, 2023 18:10 — with GitHub Actions Failure

derrickstolee temporarily deployed to release August 18, 2023 18:13 — with GitHub Actions Inactive

derrickstolee temporarily deployed to release August 18, 2023 18:21 — with GitHub Actions Inactive

derrickstolee temporarily deployed to release August 18, 2023 18:41 — with GitHub Actions Inactive

mjcheetham reviewed Aug 18, 2023

View reviewed changes

derrickstolee temporarily deployed to release August 18, 2023 18:58 — with GitHub Actions Inactive

derrickstolee temporarily deployed to release August 18, 2023 21:01 — with GitHub Actions Inactive

derrickstolee force-pushed the maintenance-locks-and-interactivity branch from f27b3af to a25660f Compare August 21, 2023 16:00

derrickstolee temporarily deployed to release August 21, 2023 16:00 — with GitHub Actions Inactive

derrickstolee temporarily deployed to release August 21, 2023 16:04 — with GitHub Actions Inactive

derrickstolee temporarily deployed to release August 21, 2023 16:10 — with GitHub Actions Inactive

derrickstolee temporarily deployed to release August 21, 2023 16:25 — with GitHub Actions Inactive

derrickstolee temporarily deployed to release August 21, 2023 16:32 — with GitHub Actions Inactive

derrickstolee temporarily deployed to release August 21, 2023 17:28 — with GitHub Actions Inactive

derrickstolee mentioned this pull request Aug 21, 2023

[DO NOT MERGE BUT PUSH INSTEAD] Rebase to v2.42.0 #596

Merged

derrickstolee force-pushed the maintenance-locks-and-interactivity branch 4 times, most recently from 1ecfce3 to 8676615 Compare August 22, 2023 16:55

derrickstolee marked this pull request as ready for review August 22, 2023 17:23

fixup! maintenance: delete stale lock files

1eae5bd

This change from microsoft#468 is causing multiple maintenance processes to get blocked on credentials instead of only one. The change did more harm than good. This reverts commit 95ed7f6.

derrickstolee force-pushed the maintenance-locks-and-interactivity branch from 8676615 to 840beed Compare August 22, 2023 19:03

dscho reviewed Aug 23, 2023

View reviewed changes

derrickstolee force-pushed the maintenance-locks-and-interactivity branch 2 times, most recently from 73fe2ea to e0b672c Compare August 23, 2023 13:44

derrickstolee added 4 commits August 23, 2023 09:53

derrickstolee force-pushed the maintenance-locks-and-interactivity branch from e0b672c to e5459ea Compare August 23, 2023 13:55

derrickstolee merged commit 908c6e5 into microsoft:vfs-2.41.0.3 Aug 23, 2023
46 checks passed

dscho mentioned this pull request Nov 24, 2023

headless-git maintenance locks repository forever when askPass is triggered git-for-windows/git#4706

Open

1 task

derrickstolee mentioned this pull request Sep 19, 2024

maintenance: configure credentials to be silent gitgitgadget/git#1798

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Maintenance locks and interactivity #598

Maintenance locks and interactivity #598

derrickstolee commented Aug 17, 2023 •

edited

Loading

jeffhostetler Aug 17, 2023

jeffhostetler Aug 17, 2023

derrickstolee Aug 18, 2023

mjcheetham Aug 18, 2023 •

edited

Loading

jeffhostetler Aug 17, 2023

derrickstolee Aug 18, 2023

mjcheetham Aug 18, 2023

jeffhostetler Aug 17, 2023 •

edited

Loading

jeffhostetler Aug 17, 2023

derrickstolee Aug 18, 2023

mjcheetham Aug 18, 2023

jeffhostetler Aug 21, 2023 •

edited

Loading

jeffhostetler commented Aug 21, 2023

jeffhostetler commented Aug 21, 2023

derrickstolee commented Aug 21, 2023

jeffhostetler commented Aug 21, 2023

derrickstolee commented Aug 21, 2023

derrickstolee commented Aug 21, 2023

jeffhostetler commented Aug 22, 2023

jeffhostetler commented Aug 22, 2023

dscho left a comment •

edited

Loading

dscho Aug 23, 2023

derrickstolee Aug 23, 2023

jeffhostetler Aug 23, 2023

Maintenance locks and interactivity #598

Maintenance locks and interactivity #598

Conversation

derrickstolee commented Aug 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjcheetham Aug 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeffhostetler Aug 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeffhostetler Aug 21, 2023 • edited Loading

Choose a reason for hiding this comment

jeffhostetler commented Aug 21, 2023

jeffhostetler commented Aug 21, 2023

derrickstolee commented Aug 21, 2023

jeffhostetler commented Aug 21, 2023

derrickstolee commented Aug 21, 2023

derrickstolee commented Aug 21, 2023

End-to-End Testing Report

jeffhostetler commented Aug 22, 2023

jeffhostetler commented Aug 22, 2023

dscho left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derrickstolee commented Aug 17, 2023 •

edited

Loading

mjcheetham Aug 18, 2023 •

edited

Loading

jeffhostetler Aug 17, 2023 •

edited

Loading

jeffhostetler Aug 21, 2023 •

edited

Loading

dscho left a comment •

edited

Loading