Re: [RFC PATCH v1 0/6] stash: drop usage of a second index

["SZEDER Gábor" <szeder.dev@gmail.com>]

On Mon, Jun 15, 2020 at 05:27:15PM +0200, SZEDER Gábor wrote:
>       - Should we even allow 'splitIndex.sharedIndexExpire=now'?
>     
>         I believe, though haven't confirmed, that it can cause trouble
>         even without using an alternate index.  Consider the following
>         sequence of events:
>     
>           - Git process A reads '.git/index', finds the 'link' extension,
>             and reads the SHA1 recorded there that determines the filename
>             of its shared index.
>     
>           - The scheduler steps in, and puts process A to sleep.
>     
>           - Git process B updates the index, decides that it's time to
>             write a new shared index, does so, and then because of
>             'splitIndex.sharedIndexExpire=now' it removes all other shared
>             index files.
>     
>           - The scheduler wakes process A, which now tries to open the
>             shared index file it just learned about, but fails because
>             that file has just been removed by process B.

Confirmed.

To help reproduce the issue, this diff adds a strategically-placed
controllable delay between reading '.git/index' and reading its
shared/base index:

diff --git a/read-cache.c b/read-cache.c
index b888c5df44..5a66e9bf4b 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -2319,6 +2319,9 @@ int read_index_from(struct index_state *istate, const char *path,
 	else
 		split_index->base = xcalloc(1, sizeof(*split_index->base));
 
+	if (git_env_bool("GIT_TEST_WAIT", 0))
+		sleep(3);
+
 	base_oid_hex = oid_to_hex(&split_index->base_oid);
 	base_path = xstrfmt("%s/sharedindex.%s", gitdir, base_oid_hex);
 	trace2_region_enter_printf("index", "shared/do_read_index",


Then this test creates the above described sequence of events:

test_expect_failure 'splitIndex.sharedIndexExpire=now can be harmful' '
	>file1 &&
	>file2 &&
	git update-index --split-index --add file1 &&

	{
		sleep 1 &&
		# "process B"
		git -c splitIndex.sharedIndexExpire=now \
		    update-index --split-index --add file2 &
	} &&

	# "process A"
	GIT_TEST_WAIT=1 git diff --cached --name-only
'

... and fails reliably with:

  [...]
  + GIT_TEST_WAIT=1 git diff --cached --name-only
  [ ... trace from background commands removed ...]
  fatal: .git/sharedindex.818f65852e7402f236aeaadd32efdbb62291aa75: index file open failed: No such file or directory


>         This is similar to the issue we have with 'git gc --prune=now',
>         except that 'git gc's documentation explicitly warns about the
>         risks of using '--prune=now', while the description of
>         'splitIndex.sharedIndexExpire' doesn't have any such warning.
>     
>         I think that 'splitIndex.sharedIndexExpire=now' should be allowed,
>         for those who hopefully know what they are doing, just as we allow
>         'git gc --prune=now', but the documentation should clearly warn
>         against its potential pitfalls.