{"thread":{"id":"62251","subject":"v2.47.0-rc1 test failure on cygwin","startedAt":"2024-10-04T01:05:55Z","lastAt":"2024-10-04T22:41:06Z","messageCount":19,"participants":["Ramsay Jones","Patrick Steinhardt","Johannes Schindelin","Junio C Hamano","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"504065","messageId":"b1b5fb40-f6c2-4621-b58c-9b7c8c64cc01@ramsayjones.plus.com","threadId":"62251","inReplyTo":null,"subject":"v2.47.0-rc1 test failure on cygwin","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2024-10-04T01:02:44Z","receivedAt":"2024-10-04T01:05:55Z","isPatch":false,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"Hi Patrick,\n\nJust a quick heads up: t0610-reftable-basics.sh test 47 (ref transaction: many\nconcurrent writers) fails on cygwin. The tail end of the debug output for this\ntest looks like:\n\n..\n\n++ wait\nfatal: update_ref failed for ref 'refs/heads/branch-82': reftable: transaction prepare: I/O error\nfatal: update_ref failed for ref 'refs/heads/branch-2': cannot lock references\nfatal: update_ref failed for ref 'refs/heads/branch-3': cannot lock references\nfatal: update_ref failed for ref 'refs/heads/branch-27': cannot lock references\nfatal: update_ref failed for ref 'refs/heads/branch-10': cannot lock references\nfatal: update_ref failed for ref 'refs/heads/branch-21': cannot lock references\nfatal: update_ref failed for ref 'refs/heads/branch-14': cannot lock references\nfatal: update_ref failed for ref 'refs/heads/branch-9': cannot lock references\nfatal: update_ref failed for ref 'refs/heads/branch-19': cannot lock references\nfatal: update_ref failed for ref 'refs/heads/branch-33': cannot lock references\nfatal: update_ref failed for ref 'refs/heads/branch-28': cannot lock references\nfatal: update_ref failed for ref 'refs/heads/branch-29': cannot lock references\nfatal: update_ref failed for ref 'refs/heads/branch-25': cannot lock references\nfatal: update_ref failed for ref 'refs/heads/branch-53': cannot lock references\nfatal: update_ref failed for ref 'refs/heads/branch-41': cannot lock references\nfatal: update_ref failed for ref 'refs/heads/branch-37': cannot lock references\nfatal: update_ref failed for ref 'refs/heads/branch-36': cannot lock references\nfatal: update_ref failed for ref 'refs/heads/branch-50': cannot lock references\nfatal: update_ref failed for ref 'refs/heads/branch-62': cannot lock references\nfatal: update_ref failed for ref 'refs/heads/branch-48': cannot lock references\nfatal: update_ref failed for ref 'refs/heads/branch-49': cannot lock references\nfatal: update_ref failed for ref 'refs/heads/branch-59': cannot lock references\nfatal: update_ref failed for ref 'refs/heads/branch-67': cannot lock references\nfatal: update_ref failed for ref 'refs/heads/branch-81': cannot lock references\nfatal: update_ref failed for ref 'refs/heads/branch-72': cannot lock references\nfatal: update_ref failed for ref 'refs/heads/branch-70': cannot lock references\nfatal: update_ref failed for ref 'refs/heads/branch-54': cannot lock references\nfatal: update_ref failed for ref 'refs/heads/branch-71': cannot lock references\nfatal: update_ref failed for ref 'refs/heads/branch-74': cannot lock references\nfatal: update_ref failed for ref 'refs/heads/branch-95': cannot lock references\nfatal: update_ref failed for ref 'refs/heads/branch-87': cannot lock references\nfatal: update_ref failed for ref 'refs/heads/branch-92': cannot lock references\nfatal: update_ref failed for ref 'refs/heads/branch-64': cannot lock references\nfatal: update_ref failed for ref 'refs/heads/branch-94': cannot lock references\nfatal: update_ref failed for ref 'refs/heads/branch-100': cannot lock references\n++ git for-each-ref --sort=v:refname\n++ test_cmp expect actual\n++ test 2 -ne 2\n++ eval 'diff -u' '\"$@\"'\n+++ diff -u expect actual\n--- expect\t2024-10-03 23:21:01.284020500 +0000\n+++ actual\t2024-10-03 23:21:19.881283200 +0000\n@@ -1,101 +1,66 @@\n 68d032e9edd3481ac96382786ececc37ec28709e commit\trefs/heads/branch-1\n-68d032e9edd3481ac96382786ececc37ec28709e commit\trefs/heads/branch-2\n-68d032e9edd3481ac96382786ececc37ec28709e commit\trefs/heads/branch-3\n 68d032e9edd3481ac96382786ececc37ec28709e commit\trefs/heads/branch-4\n 68d032e9edd3481ac96382786ececc37ec28709e commit\trefs/heads/branch-5\n 68d032e9edd3481ac96382786ececc37ec28709e commit\trefs/heads/branch-6\n 68d032e9edd3481ac96382786ececc37ec28709e commit\trefs/heads/branch-7\n 68d032e9edd3481ac96382786ececc37ec28709e commit\trefs/heads/branch-8\n-68d032e9edd3481ac96382786ececc37ec28709e commit\trefs/heads/branch-9\n-68d032e9edd3481ac96382786ececc37ec28709e commit\trefs/heads/branch-10\n 68d032e9edd3481ac96382786ececc37ec28709e commit\trefs/heads/branch-11\n 68d032e9edd3481ac96382786ececc37ec28709e commit\trefs/heads/branch-12\n 68d032e9edd3481ac96382786ececc37ec28709e commit\trefs/heads/branch-13\n-68d032e9edd3481ac96382786ececc37ec28709e commit\trefs/heads/branch-14\n 68d032e9edd3481ac96382786ececc37ec28709e commit\trefs/heads/branch-15\n 68d032e9edd3481ac96382786ececc37ec28709e commit\trefs/heads/branch-16\n 68d032e9edd3481ac96382786ececc37ec28709e commit\trefs/heads/branch-17\n 68d032e9edd3481ac96382786ececc37ec28709e commit\trefs/heads/branch-18\n-68d032e9edd3481ac96382786ececc37ec28709e commit\trefs/heads/branch-19\n 68d032e9edd3481ac96382786ececc37ec28709e commit\trefs/heads/branch-20\n-68d032e9edd3481ac96382786ececc37ec28709e commit\trefs/heads/branch-21\n 68d032e9edd3481ac96382786ececc37ec28709e commit\trefs/heads/branch-22\n 68d032e9edd3481ac96382786ececc37ec28709e commit\trefs/heads/branch-23\n 68d032e9edd3481ac96382786ececc37ec28709e commit\trefs/heads/branch-24\n-68d032e9edd3481ac96382786ececc37ec28709e commit\trefs/heads/branch-25\n 68d032e9edd3481ac96382786ececc37ec28709e commit\trefs/heads/branch-26\n-68d032e9edd3481ac96382786ececc37ec28709e commit\trefs/heads/branch-27\n-68d032e9edd3481ac96382786ececc37ec28709e commit\trefs/heads/branch-28\n-68d032e9edd3481ac96382786ececc37ec28709e commit\trefs/heads/branch-29\n 68d032e9edd3481ac96382786ececc37ec28709e commit\trefs/heads/branch-30\n 68d032e9edd3481ac96382786ececc37ec28709e commit\trefs/heads/branch-31\n 68d032e9edd3481ac96382786ececc37ec28709e commit\trefs/heads/branch-32\n-68d032e9edd3481ac96382786ececc37ec28709e commit\trefs/heads/branch-33\n 68d032e9edd3481ac96382786ececc37ec28709e commit\trefs/heads/branch-34\n 68d032e9edd3481ac96382786ececc37ec28709e commit\trefs/heads/branch-35\n-68d032e9edd3481ac96382786ececc37ec28709e commit\trefs/heads/branch-36\n-68d032e9edd3481ac96382786ececc37ec28709e commit\trefs/heads/branch-37\n 68d032e9edd3481ac96382786ececc37ec28709e commit\trefs/heads/branch-38\n 68d032e9edd3481ac96382786ececc37ec28709e commit\trefs/heads/branch-39\n 68d032e9edd3481ac96382786ececc37ec28709e commit\trefs/heads/branch-40\n-68d032e9edd3481ac96382786ececc37ec28709e commit\trefs/heads/branch-41\n 68d032e9edd3481ac96382786ececc37ec28709e commit\trefs/heads/branch-42\n 68d032e9edd3481ac96382786ececc37ec28709e commit\trefs/heads/branch-43\n 68d032e9edd3481ac96382786ececc37ec28709e commit\trefs/heads/branch-44\n 68d032e9edd3481ac96382786ececc37ec28709e commit\trefs/heads/branch-45\n 68d032e9edd3481ac96382786ececc37ec28709e commit\trefs/heads/branch-46\n 68d032e9edd3481ac96382786ececc37ec28709e commit\trefs/heads/branch-47\n-68d032e9edd3481ac96382786ececc37ec28709e commit\trefs/heads/branch-48\n-68d032e9edd3481ac96382786ececc37ec28709e commit\trefs/heads/branch-49\n-68d032e9edd3481ac96382786ececc37ec28709e commit\trefs/heads/branch-50\n 68d032e9edd3481ac96382786ececc37ec28709e commit\trefs/heads/branch-51\n 68d032e9edd3481ac96382786ececc37ec28709e commit\trefs/heads/branch-52\n-68d032e9edd3481ac96382786ececc37ec28709e commit\trefs/heads/branch-53\n-68d032e9edd3481ac96382786ececc37ec28709e commit\trefs/heads/branch-54\n 68d032e9edd3481ac96382786ececc37ec28709e commit\trefs/heads/branch-55\n 68d032e9edd3481ac96382786ececc37ec28709e commit\trefs/heads/branch-56\n 68d032e9edd3481ac96382786ececc37ec28709e commit\trefs/heads/branch-57\n 68d032e9edd3481ac96382786ececc37ec28709e commit\trefs/heads/branch-58\n-68d032e9edd3481ac96382786ececc37ec28709e commit\trefs/heads/branch-59\n 68d032e9edd3481ac96382786ececc37ec28709e commit\trefs/heads/branch-60\n 68d032e9edd3481ac96382786ececc37ec28709e commit\trefs/heads/branch-61\n-68d032e9edd3481ac96382786ececc37ec28709e commit\trefs/heads/branch-62\n 68d032e9edd3481ac96382786ececc37ec28709e commit\trefs/heads/branch-63\n-68d032e9edd3481ac96382786ececc37ec28709e commit\trefs/heads/branch-64\n 68d032e9edd3481ac96382786ececc37ec28709e commit\trefs/heads/branch-65\n 68d032e9edd3481ac96382786ececc37ec28709e commit\trefs/heads/branch-66\n-68d032e9edd3481ac96382786ececc37ec28709e commit\trefs/heads/branch-67\n 68d032e9edd3481ac96382786ececc37ec28709e commit\trefs/heads/branch-68\n 68d032e9edd3481ac96382786ececc37ec28709e commit\trefs/heads/branch-69\n-68d032e9edd3481ac96382786ececc37ec28709e commit\trefs/heads/branch-70\n-68d032e9edd3481ac96382786ececc37ec28709e commit\trefs/heads/branch-71\n-68d032e9edd3481ac96382786ececc37ec28709e commit\trefs/heads/branch-72\n 68d032e9edd3481ac96382786ececc37ec28709e commit\trefs/heads/branch-73\n-68d032e9edd3481ac96382786ececc37ec28709e commit\trefs/heads/branch-74\n 68d032e9edd3481ac96382786ececc37ec28709e commit\trefs/heads/branch-75\n 68d032e9edd3481ac96382786ececc37ec28709e commit\trefs/heads/branch-76\n 68d032e9edd3481ac96382786ececc37ec28709e commit\trefs/heads/branch-77\n 68d032e9edd3481ac96382786ececc37ec28709e commit\trefs/heads/branch-78\n 68d032e9edd3481ac96382786ececc37ec28709e commit\trefs/heads/branch-79\n 68d032e9edd3481ac96382786ececc37ec28709e commit\trefs/heads/branch-80\n-68d032e9edd3481ac96382786ececc37ec28709e commit\trefs/heads/branch-81\n-68d032e9edd3481ac96382786ececc37ec28709e commit\trefs/heads/branch-82\n 68d032e9edd3481ac96382786ececc37ec28709e commit\trefs/heads/branch-83\n 68d032e9edd3481ac96382786ececc37ec28709e commit\trefs/heads/branch-84\n 68d032e9edd3481ac96382786ececc37ec28709e commit\trefs/heads/branch-85\n 68d032e9edd3481ac96382786ececc37ec28709e commit\trefs/heads/branch-86\n-68d032e9edd3481ac96382786ececc37ec28709e commit\trefs/heads/branch-87\n 68d032e9edd3481ac96382786ececc37ec28709e commit\trefs/heads/branch-88\n 68d032e9edd3481ac96382786ececc37ec28709e commit\trefs/heads/branch-89\n 68d032e9edd3481ac96382786ececc37ec28709e commit\trefs/heads/branch-90\n 68d032e9edd3481ac96382786ececc37ec28709e commit\trefs/heads/branch-91\n-68d032e9edd3481ac96382786ececc37ec28709e commit\trefs/heads/branch-92\n 68d032e9edd3481ac96382786ececc37ec28709e commit\trefs/heads/branch-93\n-68d032e9edd3481ac96382786ececc37ec28709e commit\trefs/heads/branch-94\n-68d032e9edd3481ac96382786ececc37ec28709e commit\trefs/heads/branch-95\n 68d032e9edd3481ac96382786ececc37ec28709e commit\trefs/heads/branch-96\n 68d032e9edd3481ac96382786ececc37ec28709e commit\trefs/heads/branch-97\n 68d032e9edd3481ac96382786ececc37ec28709e commit\trefs/heads/branch-98\n 68d032e9edd3481ac96382786ececc37ec28709e commit\trefs/heads/branch-99\n-68d032e9edd3481ac96382786ececc37ec28709e commit\trefs/heads/branch-100\n 68d032e9edd3481ac96382786ececc37ec28709e commit\trefs/heads/main\nerror: last command exited with $?=1\nnot ok 47 - ref transaction: many concurrent writers\n\n..\n\nt0610-reftable-basics.sh passed on 'rc0', but this test (and the timeout facility)\nis new in 'rc1'. I tried simply increasing the timeout (10 fold), but that didn't\nchange the result. (I didn't really expect it to - the 'reftable: transaction\nprepare: I/O error' does not look timing related!).\n\nAgain, just a heads up. (I can't look at it until tomorrow now; any ideas?)\n\nATB,\nRamsay Jones\n\n\n"},{"id":"504068","messageId":"Zv9oIrKveu-JAGQM@pks.im","threadId":"62251","inReplyTo":"b1b5fb40-f6c2-4621-b58c-9b7c8c64cc01@ramsayjones.plus.com","subject":"Re: v2.47.0-rc1 test failure on cygwin","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-10-04T03:59:30Z","receivedAt":"2024-10-04T03:59:39Z","isPatch":false,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Oct 04, 2024 at 02:02:44AM +0100, Ramsay Jones wrote:\n> Hi Patrick,\n> \n> Just a quick heads up: t0610-reftable-basics.sh test 47 (ref transaction: many\n> concurrent writers) fails on cygwin. The tail end of the debug output for this\n> test looks like:\n> \n[snip]\n> \n> t0610-reftable-basics.sh passed on 'rc0', but this test (and the timeout facility)\n> is new in 'rc1'. I tried simply increasing the timeout (10 fold), but that didn't\n> change the result. (I didn't really expect it to - the 'reftable: transaction\n> prepare: I/O error' does not look timing related!).\n> \n> Again, just a heads up. (I can't look at it until tomorrow now; any ideas?)\n\nThis failure is kind of known and discussed in [1]. Just to make it\nexplicit: this test failure doesn't really surface a regression, the\nreftable code already failed for concurrent writes before. I fixed that\nand added the test that is now flaky, as the fix itself is seemingly\nonly sufficient on Linux and macOS.\n\nI didn't yet have the time to look at whether I can fix it, but should\nfinally find the time to do so today.\n\nPatrick\n\n[1]: <20240927040752.GA567671@coredump.intra.peff.net>\n"},{"id":"504074","messageId":"Zv-HbT8qrM6IYKb4@pks.im","threadId":"62251","inReplyTo":"Zv9oIrKveu-JAGQM@pks.im","subject":"Re: v2.47.0-rc1 test failure on cygwin","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-10-04T06:13:05Z","receivedAt":"2024-10-04T06:13:12Z","isPatch":false,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Oct 04, 2024 at 05:59:30AM +0200, Patrick Steinhardt wrote:\n> On Fri, Oct 04, 2024 at 02:02:44AM +0100, Ramsay Jones wrote:\n> > Hi Patrick,\n> > \n> > Just a quick heads up: t0610-reftable-basics.sh test 47 (ref transaction: many\n> > concurrent writers) fails on cygwin. The tail end of the debug output for this\n> > test looks like:\n> > \n> [snip]\n> > \n> > t0610-reftable-basics.sh passed on 'rc0', but this test (and the timeout facility)\n> > is new in 'rc1'. I tried simply increasing the timeout (10 fold), but that didn't\n> > change the result. (I didn't really expect it to - the 'reftable: transaction\n> > prepare: I/O error' does not look timing related!).\n> > \n> > Again, just a heads up. (I can't look at it until tomorrow now; any ideas?)\n> \n> This failure is kind of known and discussed in [1]. Just to make it\n> explicit: this test failure doesn't really surface a regression, the\n> reftable code already failed for concurrent writes before. I fixed that\n> and added the test that is now flaky, as the fix itself is seemingly\n> only sufficient on Linux and macOS.\n> \n> I didn't yet have the time to look at whether I can fix it, but should\n> finally find the time to do so today.\n\nHm, interestingly enough I cannot reproduce the issue on Cygwin myself,\nbut I can reproduce the issue with MinGW. And in fact, the logs you have\nsent all indicate that we cannot acquire the lock, there is no sign of\nI/O errors here. So I guess you're running into timeout issues. Does the\nfollowing patch fix this for you?\n\ndiff --git a/t/t0610-reftable-basics.sh b/t/t0610-reftable-basics.sh\nindex 2d951c8ceb..b5cad805d4 100755\n--- a/t/t0610-reftable-basics.sh\n+++ b/t/t0610-reftable-basics.sh\n@@ -455,10 +455,7 @@ test_expect_success 'ref transaction: many concurrent writers' '\n \tgit init repo &&\n \t(\n \t\tcd repo &&\n-\t\t# Set a high timeout such that a busy CI machine will not abort\n-\t\t# early. 10 seconds should hopefully be ample of time to make\n-\t\t# this non-flaky.\n-\t\tgit config set reftable.lockTimeout 10000 &&\n+\t\tgit config set reftable.lockTimeout -1 &&\n \t\ttest_commit --no-tag initial &&\n \n \t\thead=$(git rev-parse HEAD) &&\n\nThe issue on Win32 is different: we cannot commit the \"tables.list\" lock\nvia rename(3P) because the target file may be open for reading by a\nconcurrent process. I guess that Cygwin has proper POSIX semantics for\nrename(3P) and thus doesn't hit the same issue.\n\nWe already try to emulate POSIX semantics somewhat in `mingw_rename()`\nby using a retry-loop when we hit `ERROR_ACCESS_DENIED`, which is what\nwe get when the target file is open in another process. But that\nseemingly isn't enough when there is a lot of contention around a file.\nSo I'm currently investigating whether we can adopt something similar to\nwhat Cygwin is doing for Win32, as well. I assume that they use\n`FILE_RENAME_INFORMATION_EX` with `FILE_RENAME_POSIX_SEMANTICS`, which\nshould give us what we're looking for.\n\ngh, well. Turns out the implementation of rename(3P) in Cygwin is 500\nlines long. I guess this is a non-trivial problem :) But they of course\nhave to handle a whole lot more cases than we have to. But my guess was\ncorrect: they do use `FILE_RENAME_POSIX_SEMANTICS`. The catch is that\nthis flag only exists in Windows 10 and newer. But that should be a fine\ncompromise.\n\nI'll try to wrap my head around how all of this works.\n\nPatrick\n"},{"id":"504077","messageId":"8718c5c4-1d0a-104b-eb39-6338ae9c5dbf@gmx.de","threadId":"62251","inReplyTo":"Zv-HbT8qrM6IYKb4@pks.im","subject":"Re: v2.47.0-rc1 test failure on cygwin","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2024-10-04T09:13:34Z","receivedAt":"2024-10-04T09:13:43Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Patrick,\n\nOn Fri, 4 Oct 2024, Patrick Steinhardt wrote:\n\n> On Fri, Oct 04, 2024 at 05:59:30AM +0200, Patrick Steinhardt wrote:\n> > On Fri, Oct 04, 2024 at 02:02:44AM +0100, Ramsay Jones wrote:\n> > >\n> > > Just a quick heads up: t0610-reftable-basics.sh test 47 (ref\n> > > transaction: many concurrent writers) fails on cygwin. The tail end\n> > > of the debug output for this test looks like:\n> > >\n> > [snip]\n> > >\n> > > t0610-reftable-basics.sh passed on 'rc0', but this test (and the\n> > > timeout facility) is new in 'rc1'. I tried simply increasing the\n> > > timeout (10 fold), but that didn't change the result. (I didn't\n> > > really expect it to - the 'reftable: transaction prepare: I/O error'\n> > > does not look timing related!).\n> > >\n> > > Again, just a heads up. (I can't look at it until tomorrow now; any\n> > > ideas?)\n> >\n> > This failure is kind of known and discussed in [1]. Just to make it\n> > explicit: this test failure doesn't really surface a regression, the\n> > reftable code already failed for concurrent writes before. I fixed\n> > that and added the test that is now flaky, as the fix itself is\n> > seemingly only sufficient on Linux and macOS.\n> >\n> > I didn't yet have the time to look at whether I can fix it, but should\n> > finally find the time to do so today.\n>\n> Hm, interestingly enough I cannot reproduce the issue on Cygwin myself,\n> but I can reproduce the issue with MinGW. And in fact, the logs you have\n> sent all indicate that we cannot acquire the lock, there is no sign of\n> I/O errors here. So I guess you're running into timeout issues. Does the\n> following patch fix this for you?\n>\n> diff --git a/t/t0610-reftable-basics.sh b/t/t0610-reftable-basics.sh\n> index 2d951c8ceb..b5cad805d4 100755\n> --- a/t/t0610-reftable-basics.sh\n> +++ b/t/t0610-reftable-basics.sh\n> @@ -455,10 +455,7 @@ test_expect_success 'ref transaction: many concurrent writers' '\n> \tgit init repo &&\n> \t(\n> \t\tcd repo &&\n> -\t\t# Set a high timeout such that a busy CI machine will not abort\n> -\t\t# early. 10 seconds should hopefully be ample of time to make\n> -\t\t# this non-flaky.\n> -\t\tgit config set reftable.lockTimeout 10000 &&\n> +\t\tgit config set reftable.lockTimeout -1 &&\n> \t\ttest_commit --no-tag initial &&\n>\n> \t\thead=$(git rev-parse HEAD) &&\n\nThat looks plausible to me, as this test is exercising the POSIX emulation\nlayer of Cygwin much more than Git itself (and therefore I expect this\ntest case to take a really long time on Cygwin). If it turns out that this\nworks around the issue, I would propose to use something like this\ninstead:\n\n\t\t# Cygwin needs a slightly longer timeout\n\t\tif have_prereq !CYGWIN\n\t\tthen\n\t\t\tgit config set reftable.lockTimeout 10000\n\t\telse\n\t\t\tgit config set reftable.lockTimeout 60000\n\t\tfi &&\n\n> The issue on Win32 is different: we cannot commit the \"tables.list\" lock\n> via rename(3P) because the target file may be open for reading by a\n> concurrent process. I guess that Cygwin has proper POSIX semantics for\n> rename(3P) and thus doesn't hit the same issue.\n\nIndeed, this is where the symptom lies. I worked around it in Git for\nWindows v2.47.0-rc1 via this patch:\n\n-- snipsnap --\ndiff --git a/compat/mingw.c b/compat/mingw.c\nindex c1030e40f227..56db193d1360 100644\n--- a/compat/mingw.c\n+++ b/compat/mingw.c\n@@ -784,6 +784,7 @@ int mingw_open (const char *filename, int oflags, ...)\n \tint fd, create = (oflags & (O_CREAT | O_EXCL)) == (O_CREAT | O_EXCL);\n \twchar_t wfilename[MAX_LONG_PATH];\n \topen_fn_t open_fn;\n+\tint tries = 0;\n\n \tva_start(args, oflags);\n \tmode = va_arg(args, int);\n@@ -813,7 +814,11 @@ int mingw_open (const char *filename, int oflags, ...)\n \telse if (xutftowcs_long_path(wfilename, filename) < 0)\n \t\treturn -1;\n\n-\tfd = open_fn(wfilename, oflags, mode);\n+\tdo {\n+\t\tfd = open_fn(wfilename, oflags, mode);\n+\t} while (fd < 0 && GetLastError() == ERROR_SHARING_VIOLATION &&\n+\t\t retry_ask_yes_no(&tries, \"Opening '%s' failed because another \"\n+\t\t\t\"application accessed it. Try again?\", filename));\n\n \tif ((oflags & O_CREAT) && fd >= 0 && are_wsl_compatible_mode_bits_enabled()) {\n \t\t_mode_t wsl_mode = S_IFREG | (mode&0777);\n"},{"id":"504080","messageId":"Zv--68J5qv60IuQz@pks.im","threadId":"62251","inReplyTo":"8718c5c4-1d0a-104b-eb39-6338ae9c5dbf@gmx.de","subject":"Re: v2.47.0-rc1 test failure on cygwin","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-10-04T10:09:54Z","receivedAt":"2024-10-04T10:10:01Z","isPatch":false,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Oct 04, 2024 at 11:13:34AM +0200, Johannes Schindelin wrote:\n> Hi Patrick,\n> \n> On Fri, 4 Oct 2024, Patrick Steinhardt wrote:\n> \n> > On Fri, Oct 04, 2024 at 05:59:30AM +0200, Patrick Steinhardt wrote:\n> > > On Fri, Oct 04, 2024 at 02:02:44AM +0100, Ramsay Jones wrote:\n> > > >\n> > > > Just a quick heads up: t0610-reftable-basics.sh test 47 (ref\n> > > > transaction: many concurrent writers) fails on cygwin. The tail end\n> > > > of the debug output for this test looks like:\n> > > >\n> > > [snip]\n> > > >\n> > > > t0610-reftable-basics.sh passed on 'rc0', but this test (and the\n> > > > timeout facility) is new in 'rc1'. I tried simply increasing the\n> > > > timeout (10 fold), but that didn't change the result. (I didn't\n> > > > really expect it to - the 'reftable: transaction prepare: I/O error'\n> > > > does not look timing related!).\n> > > >\n> > > > Again, just a heads up. (I can't look at it until tomorrow now; any\n> > > > ideas?)\n> > >\n> > > This failure is kind of known and discussed in [1]. Just to make it\n> > > explicit: this test failure doesn't really surface a regression, the\n> > > reftable code already failed for concurrent writes before. I fixed\n> > > that and added the test that is now flaky, as the fix itself is\n> > > seemingly only sufficient on Linux and macOS.\n> > >\n> > > I didn't yet have the time to look at whether I can fix it, but should\n> > > finally find the time to do so today.\n> >\n> > Hm, interestingly enough I cannot reproduce the issue on Cygwin myself,\n> > but I can reproduce the issue with MinGW. And in fact, the logs you have\n> > sent all indicate that we cannot acquire the lock, there is no sign of\n> > I/O errors here. So I guess you're running into timeout issues. Does the\n> > following patch fix this for you?\n> >\n> > diff --git a/t/t0610-reftable-basics.sh b/t/t0610-reftable-basics.sh\n> > index 2d951c8ceb..b5cad805d4 100755\n> > --- a/t/t0610-reftable-basics.sh\n> > +++ b/t/t0610-reftable-basics.sh\n> > @@ -455,10 +455,7 @@ test_expect_success 'ref transaction: many concurrent writers' '\n> > \tgit init repo &&\n> > \t(\n> > \t\tcd repo &&\n> > -\t\t# Set a high timeout such that a busy CI machine will not abort\n> > -\t\t# early. 10 seconds should hopefully be ample of time to make\n> > -\t\t# this non-flaky.\n> > -\t\tgit config set reftable.lockTimeout 10000 &&\n> > +\t\tgit config set reftable.lockTimeout -1 &&\n> > \t\ttest_commit --no-tag initial &&\n> >\n> > \t\thead=$(git rev-parse HEAD) &&\n> \n> That looks plausible to me, as this test is exercising the POSIX emulation\n> layer of Cygwin much more than Git itself (and therefore I expect this\n> test case to take a really long time on Cygwin). If it turns out that this\n> works around the issue, I would propose to use something like this\n> instead:\n> \n> \t\t# Cygwin needs a slightly longer timeout\n> \t\tif have_prereq !CYGWIN\n> \t\tthen\n> \t\t\tgit config set reftable.lockTimeout 10000\n> \t\telse\n> \t\t\tgit config set reftable.lockTimeout 60000\n> \t\tfi &&\n\nWe also saw that this creates an issue when running e.g. with the memory\nand leak sanitizers. So I'd just generally raise the timeout value to\nsomething way higher to avoid flakiness, like 5 minutes.\n\n> > The issue on Win32 is different: we cannot commit the \"tables.list\" lock\n> > via rename(3P) because the target file may be open for reading by a\n> > concurrent process. I guess that Cygwin has proper POSIX semantics for\n> > rename(3P) and thus doesn't hit the same issue.\n> \n> Indeed, this is where the symptom lies. I worked around it in Git for\n> Windows v2.47.0-rc1 via this patch:\n> \n> -- snipsnap --\n> diff --git a/compat/mingw.c b/compat/mingw.c\n> index c1030e40f227..56db193d1360 100644\n> --- a/compat/mingw.c\n> +++ b/compat/mingw.c\n> @@ -784,6 +784,7 @@ int mingw_open (const char *filename, int oflags, ...)\n> \tint fd, create = (oflags & (O_CREAT | O_EXCL)) == (O_CREAT | O_EXCL);\n> \twchar_t wfilename[MAX_LONG_PATH];\n> \topen_fn_t open_fn;\n> +\tint tries = 0;\n> \n> \tva_start(args, oflags);\n> \tmode = va_arg(args, int);\n> @@ -813,7 +814,11 @@ int mingw_open (const char *filename, int oflags, ...)\n> \telse if (xutftowcs_long_path(wfilename, filename) < 0)\n> \t\treturn -1;\n> \n> -\tfd = open_fn(wfilename, oflags, mode);\n> +\tdo {\n> +\t\tfd = open_fn(wfilename, oflags, mode);\n> +\t} while (fd < 0 && GetLastError() == ERROR_SHARING_VIOLATION &&\n> +\t\t retry_ask_yes_no(&tries, \"Opening '%s' failed because another \"\n> +\t\t\t\"application accessed it. Try again?\", filename));\n> \n> \tif ((oflags & O_CREAT) && fd >= 0 && are_wsl_compatible_mode_bits_enabled()) {\n> \t\t_mode_t wsl_mode = S_IFREG | (mode&0777);\n\nWait, this is surprising to me as I saw the error happening when calling\nrename, not open. So why would retries in `open()` fix the symptom? I'm\nprobably missing something.\n\nIn any case I also tried something like the below patch (sorry,\nwhitespace-broken). But unfortunately this still caused permission\nerrors when the new path was held open by another process.\n\nI think for now I'd still lean into the direction of adding the !WINDOWS\nprerequisite to the test and increasing timeouts such that I can\ncontinue to investigate without time pressure.\n\nPatrick\n\n@ -2152,9 +2152,12 @@ int mingw_accept(int sockfd1, struct sockaddr *sa, socklen_t *sz)\n int mingw_rename(const char *pold, const char *pnew)\n {\n DWORD attrs, gle;\n- int tries = 0;\n+ int tries = 0, wpnew_len, wpold_len;\n wchar_t wpold[MAX_PATH], wpnew[MAX_PATH];\n- if (xutftowcs_path(wpold, pold) < 0 || xutftowcs_path(wpnew, pnew) < 0)\n+\n+ wpold_len = xutftowcs_path(wpold, pold);\n+ wpnew_len = xutftowcs_path(wpnew, pnew);\n+ if (wpold_len < 0 || wpnew_len < 0)\n return -1;\n \n /*\n@@ -2166,6 +2169,58 @@ int mingw_rename(const char *pold, const char *pnew)\n if (errno != EEXIST)\n return -1;\n repeat:\n+ {\n+#define FileRenameInfoEx 22\n+ enum {\n+ FILE_RENAME_REPLACE_IF_EXISTS = 0x01,\n+ FILE_RENAME_POSIX_SEMANTICS = 0x02,\n+ FILE_RENAME_IGNORE_READONLY_ATTRIBUTE = 0x40,\n+ };\n+ typedef struct FILE_RENAME_INFORMATION {\n+ union {\n+ BOOLEAN ReplaceIfExists;\n+ ULONG Flags;\n+ };\n+ HANDLE RootDirectory;\n+ ULONG FileNameLength;\n+ WCHAR FileName[1];\n+ } *PFILE_RENAME_INFORMATION;\n+ size_t wpnew_size = wpnew_len * sizeof(wchar_t);\n+ PFILE_RENAME_INFORMATION fri = NULL;\n+ HANDLE handle = NULL;\n+ BOOL success;\n+ int ret;\n+\n+ handle = CreateFileW(wpold, DELETE,\n+ FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE, NULL,\n+ OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL);\n+ if (handle == INVALID_HANDLE_VALUE) {\n+ errno = ENOENT;\n+ ret = -1;\n+ goto out;\n+ }\n+\n+ fri = xcalloc(1, sizeof(*fri) + wpnew_size);\n+ fri->Flags = FILE_RENAME_REPLACE_IF_EXISTS | FILE_RENAME_POSIX_SEMANTICS |\n+ FILE_RENAME_IGNORE_READONLY_ATTRIBUTE;\n+ fri->FileNameLength = wpnew_len;\n+ memcpy(fri->FileName, wpnew, wpnew_size);\n+\n+ success = SetFileInformationByHandle(handle, FileRenameInfoEx,\n+ fri, sizeof(*fri) + wpnew_size);\n+ if (!success) {\n+ errno = err_win_to_posix(GetLastError());\n+ ret = -1;\n+ goto out;\n+ }\n+\n+ ret = 0;\n+out:\n+ CloseHandle(handle);\n+ free(fri);\n+ return ret;\n+ }\n+\n"},{"id":"504082","messageId":"f29241a7-aadd-e824-97f3-a95ac6619951@gmx.de","threadId":"62251","inReplyTo":"Zv--68J5qv60IuQz@pks.im","subject":"Re: v2.47.0-rc1 test failure on cygwin","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2024-10-04T11:11:15Z","receivedAt":"2024-10-04T11:11:24Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Patrick,\n\nOn Fri, 4 Oct 2024, Patrick Steinhardt wrote:\n\n> On Fri, Oct 04, 2024 at 11:13:34AM +0200, Johannes Schindelin wrote:\n> >\n> > On Fri, 4 Oct 2024, Patrick Steinhardt wrote:\n> >\n> > > diff --git a/t/t0610-reftable-basics.sh b/t/t0610-reftable-basics.sh\n> > > index 2d951c8ceb..b5cad805d4 100755\n> > > --- a/t/t0610-reftable-basics.sh\n> > > +++ b/t/t0610-reftable-basics.sh\n> > > @@ -455,10 +455,7 @@ test_expect_success 'ref transaction: many concurrent writers' '\n> > > \tgit init repo &&\n> > > \t(\n> > > \t\tcd repo &&\n> > > -\t\t# Set a high timeout such that a busy CI machine will not abort\n> > > -\t\t# early. 10 seconds should hopefully be ample of time to make\n> > > -\t\t# this non-flaky.\n> > > -\t\tgit config set reftable.lockTimeout 10000 &&\n> > > +\t\tgit config set reftable.lockTimeout -1 &&\n> > > \t\ttest_commit --no-tag initial &&\n> > >\n> > > \t\thead=$(git rev-parse HEAD) &&\n> >\n> > That looks plausible to me, as this test is exercising the POSIX emulation\n> > layer of Cygwin much more than Git itself (and therefore I expect this\n> > test case to take a really long time on Cygwin). If it turns out that this\n> > works around the issue, I would propose to use something like this\n> > instead:\n> >\n> > \t\t# Cygwin needs a slightly longer timeout\n> > \t\tif have_prereq !CYGWIN\n> > \t\tthen\n> > \t\t\tgit config set reftable.lockTimeout 10000\n> > \t\telse\n> > \t\t\tgit config set reftable.lockTimeout 60000\n> > \t\tfi &&\n>\n> We also saw that this creates an issue when running e.g. with the memory\n> and leak sanitizers. So I'd just generally raise the timeout value to\n> something way higher to avoid flakiness, like 5 minutes.\n\nSounds good!\n\n> > > The issue on Win32 is different: we cannot commit the \"tables.list\" lock\n> > > via rename(3P) because the target file may be open for reading by a\n> > > concurrent process. I guess that Cygwin has proper POSIX semantics for\n> > > rename(3P) and thus doesn't hit the same issue.\n> >\n> > Indeed, this is where the symptom lies. I worked around it in Git for\n> > Windows v2.47.0-rc1 via this patch:\n> >\n> > -- snipsnap --\n> > diff --git a/compat/mingw.c b/compat/mingw.c\n> > index c1030e40f227..56db193d1360 100644\n> > --- a/compat/mingw.c\n> > +++ b/compat/mingw.c\n> > @@ -784,6 +784,7 @@ int mingw_open (const char *filename, int oflags, ...)\n> > \tint fd, create = (oflags & (O_CREAT | O_EXCL)) == (O_CREAT | O_EXCL);\n> > \twchar_t wfilename[MAX_LONG_PATH];\n> > \topen_fn_t open_fn;\n> > +\tint tries = 0;\n> >\n> > \tva_start(args, oflags);\n> > \tmode = va_arg(args, int);\n> > @@ -813,7 +814,11 @@ int mingw_open (const char *filename, int oflags, ...)\n> > \telse if (xutftowcs_long_path(wfilename, filename) < 0)\n> > \t\treturn -1;\n> >\n> > -\tfd = open_fn(wfilename, oflags, mode);\n> > +\tdo {\n> > +\t\tfd = open_fn(wfilename, oflags, mode);\n> > +\t} while (fd < 0 && GetLastError() == ERROR_SHARING_VIOLATION &&\n> > +\t\t retry_ask_yes_no(&tries, \"Opening '%s' failed because another \"\n> > +\t\t\t\"application accessed it. Try again?\", filename));\n> >\n> > \tif ((oflags & O_CREAT) && fd >= 0 && are_wsl_compatible_mode_bits_enabled()) {\n> > \t\t_mode_t wsl_mode = S_IFREG | (mode&0777);\n>\n> Wait, this is surprising to me as I saw the error happening when calling\n> rename, not open. So why would retries in `open()` fix the symptom? I'm\n> probably missing something.\n\nI am sorry, I did not really read the bug report in detail, I only meant\nto unblock Git for Windows v2.47.0-rc1 and thought I had a fix in hand. It\ncertainly fixed the failures on my local machine, but it unfortunately did\nnot fix the problems in CI.\n\nI tried to debug in CI a bit, but it is a gnarly bug to investigate, what\nwith plenty of processes the intentionally block each other, and no `gdb`\nto help.\n\n> In any case I also tried something like the below patch (sorry,\n> whitespace-broken).\n\nOh, you reminded me that the `mingw_rename()` function looks\n_substantially_ different in Git for Windows than in Git. I did not have\nthe time (or the strength of mind, more like) to upstream those changes\nyet.\n\n> But unfortunately this still caused permission errors when the new path\n> was held open by another process.\n\nYes, this will _always_ be a problem, I think. The\n`FILE_RENAME_POSIX_SEMANTICS` as per its documentation should help, but if\nit does not in your tests it might actually not quite work as advertised\n(wouldn't be the first time I encounter such an issue).\n\nI tried to read through the code (it's a lot!) to figure out whether there\nis potentially any situation when the `tables.list` file is opened but not\nclosed immediately, but couldn't find any. Do you know off-hand of any\nsuch scenario?\n\n> I think for now I'd still lean into the direction of adding the !WINDOWS\n> prerequisite to the test and increasing timeouts such that I can\n> continue to investigate without time pressure.\n\nLet me bang my head against this problem for a little while longer. You\nmight be right, though, that this is a thing we cannot fix in time for\nv2.47.0, which would be sad.\n\nCiao,\nJohannes\n\n>\n> Patrick\n>\n> @ -2152,9 +2152,12 @@ int mingw_accept(int sockfd1, struct sockaddr *sa, socklen_t *sz)\n> int mingw_rename(const char *pold, const char *pnew)\n> {\n> DWORD attrs, gle;\n> - int tries = 0;\n> + int tries = 0, wpnew_len, wpold_len;\n> wchar_t wpold[MAX_PATH], wpnew[MAX_PATH];\n> - if (xutftowcs_path(wpold, pold) < 0 || xutftowcs_path(wpnew, pnew) < 0)\n> +\n> + wpold_len = xutftowcs_path(wpold, pold);\n> + wpnew_len = xutftowcs_path(wpnew, pnew);\n> + if (wpold_len < 0 || wpnew_len < 0)\n> return -1;\n>\n> /*\n> @@ -2166,6 +2169,58 @@ int mingw_rename(const char *pold, const char *pnew)\n> if (errno != EEXIST)\n> return -1;\n> repeat:\n> + {\n> +#define FileRenameInfoEx 22\n> + enum {\n> + FILE_RENAME_REPLACE_IF_EXISTS = 0x01,\n> + FILE_RENAME_POSIX_SEMANTICS = 0x02,\n> + FILE_RENAME_IGNORE_READONLY_ATTRIBUTE = 0x40,\n> + };\n> + typedef struct FILE_RENAME_INFORMATION {\n> + union {\n> + BOOLEAN ReplaceIfExists;\n> + ULONG Flags;\n> + };\n> + HANDLE RootDirectory;\n> + ULONG FileNameLength;\n> + WCHAR FileName[1];\n> + } *PFILE_RENAME_INFORMATION;\n> + size_t wpnew_size = wpnew_len * sizeof(wchar_t);\n> + PFILE_RENAME_INFORMATION fri = NULL;\n> + HANDLE handle = NULL;\n> + BOOL success;\n> + int ret;\n> +\n> + handle = CreateFileW(wpold, DELETE,\n> + FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE, NULL,\n> + OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL);\n> + if (handle == INVALID_HANDLE_VALUE) {\n> + errno = ENOENT;\n> + ret = -1;\n> + goto out;\n> + }\n> +\n> + fri = xcalloc(1, sizeof(*fri) + wpnew_size);\n> + fri->Flags = FILE_RENAME_REPLACE_IF_EXISTS | FILE_RENAME_POSIX_SEMANTICS |\n> + FILE_RENAME_IGNORE_READONLY_ATTRIBUTE;\n> + fri->FileNameLength = wpnew_len;\n> + memcpy(fri->FileName, wpnew, wpnew_size);\n> +\n> + success = SetFileInformationByHandle(handle, FileRenameInfoEx,\n> + fri, sizeof(*fri) + wpnew_size);\n> + if (!success) {\n> + errno = err_win_to_posix(GetLastError());\n> + ret = -1;\n> + goto out;\n> + }\n> +\n> + ret = 0;\n> +out:\n> + CloseHandle(handle);\n> + free(fri);\n> + return ret;\n> + }\n> +\n>\n>\n"},{"id":"504083","messageId":"Zv_SKtehwx2dsflo@pks.im","threadId":"62251","inReplyTo":"f29241a7-aadd-e824-97f3-a95ac6619951@gmx.de","subject":"Re: v2.47.0-rc1 test failure on cygwin","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-10-04T11:32:01Z","receivedAt":"2024-10-04T11:32:07Z","isPatch":false,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Oct 04, 2024 at 01:11:15PM +0200, Johannes Schindelin wrote:\n> Hi Patrick,\n> > > > The issue on Win32 is different: we cannot commit the \"tables.list\" lock\n> > > > via rename(3P) because the target file may be open for reading by a\n> > > > concurrent process. I guess that Cygwin has proper POSIX semantics for\n> > > > rename(3P) and thus doesn't hit the same issue.\n> > >\n> > > Indeed, this is where the symptom lies. I worked around it in Git for\n> > > Windows v2.47.0-rc1 via this patch:\n> > >\n> > > -- snipsnap --\n> > > diff --git a/compat/mingw.c b/compat/mingw.c\n> > > index c1030e40f227..56db193d1360 100644\n> > > --- a/compat/mingw.c\n> > > +++ b/compat/mingw.c\n> > > @@ -784,6 +784,7 @@ int mingw_open (const char *filename, int oflags, ...)\n> > > \tint fd, create = (oflags & (O_CREAT | O_EXCL)) == (O_CREAT | O_EXCL);\n> > > \twchar_t wfilename[MAX_LONG_PATH];\n> > > \topen_fn_t open_fn;\n> > > +\tint tries = 0;\n> > >\n> > > \tva_start(args, oflags);\n> > > \tmode = va_arg(args, int);\n> > > @@ -813,7 +814,11 @@ int mingw_open (const char *filename, int oflags, ...)\n> > > \telse if (xutftowcs_long_path(wfilename, filename) < 0)\n> > > \t\treturn -1;\n> > >\n> > > -\tfd = open_fn(wfilename, oflags, mode);\n> > > +\tdo {\n> > > +\t\tfd = open_fn(wfilename, oflags, mode);\n> > > +\t} while (fd < 0 && GetLastError() == ERROR_SHARING_VIOLATION &&\n> > > +\t\t retry_ask_yes_no(&tries, \"Opening '%s' failed because another \"\n> > > +\t\t\t\"application accessed it. Try again?\", filename));\n> > >\n> > > \tif ((oflags & O_CREAT) && fd >= 0 && are_wsl_compatible_mode_bits_enabled()) {\n> > > \t\t_mode_t wsl_mode = S_IFREG | (mode&0777);\n> >\n> > Wait, this is surprising to me as I saw the error happening when calling\n> > rename, not open. So why would retries in `open()` fix the symptom? I'm\n> > probably missing something.\n> \n> I am sorry, I did not really read the bug report in detail, I only meant\n> to unblock Git for Windows v2.47.0-rc1 and thought I had a fix in hand. It\n> certainly fixed the failures on my local machine, but it unfortunately did\n> not fix the problems in CI.\n\nNothing to be sorry about. Quite on the contrary, thanks for chiming in\nhere, I was hoping for your input.\n\n> I tried to debug in CI a bit, but it is a gnarly bug to investigate, what\n> with plenty of processes the intentionally block each other, and no `gdb`\n> to help.\n\nYup, indeed it is.\n\n> > In any case I also tried something like the below patch (sorry,\n> > whitespace-broken).\n> \n> Oh, you reminded me that the `mingw_rename()` function looks\n> _substantially_ different in Git for Windows than in Git. I did not have\n> the time (or the strength of mind, more like) to upstream those changes\n> yet.\n> \n> > But unfortunately this still caused permission errors when the new path\n> > was held open by another process.\n> \n> Yes, this will _always_ be a problem, I think. The\n> `FILE_RENAME_POSIX_SEMANTICS` as per its documentation should help, but if\n> it does not in your tests it might actually not quite work as advertised\n> (wouldn't be the first time I encounter such an issue).\n\nIt could of course be that the code I wrote is just plain wrong. I had\nto stub out the struct as well as the constants because those are not\navailable on my Windows system, due to whatever reason. Not even when\nbumping _WIN32_WINNT to 0x0A00 (W10+). Maybe this is because I'm using\nWindows 10, but I thought that thish should have been available starting\nwith Windows 10 RC1.\n\n> I tried to read through the code (it's a lot!) to figure out whether there\n> is potentially any situation when the `tables.list` file is opened but not\n> closed immediately, but couldn't find any. Do you know off-hand of any\n> such scenario?\n\nWe indeed do keep the `tables.list` file descriptor open as part of our\nstat cache. But that only happens on non-Windows system. This all\nhappens in `reftable_stack_reload_maybe_reuse()` and is documented quite\nextensively in there.\n\n> > I think for now I'd still lean into the direction of adding the !WINDOWS\n> > prerequisite to the test and increasing timeouts such that I can\n> > continue to investigate without time pressure.\n> \n> Let me bang my head against this problem for a little while longer. You\n> might be right, though, that this is a thing we cannot fix in time for\n> v2.47.0, which would be sad.\n\nA bit sad, yes, but as I mentioned this is at least not a regression.\nThe test just demonstrates that the improvements I did in this area are\nnot yet sufficient for all platforms and that I need to spend some more\ntime on it. Or that the central \"tables.list\" mechanism is not a good\nfit for Windows, which would be a shame.\n\nI'll send the patch as a reply to this thread so that it can be picked\nup as required.\n\nPatrick\n"},{"id":"504084","messageId":"f83e23f1e76454a80e3e53cd02b3bb5bba6b8da1.1728044178.git.ps@pks.im","threadId":"62251","inReplyTo":"b1b5fb40-f6c2-4621-b58c-9b7c8c64cc01@ramsayjones.plus.com","subject":"[PATCH] t0610: work around flaky test with concurrent writers","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-10-04T12:16:45Z","receivedAt":"2024-10-04T12:16:53Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"In 6241ce2170 (refs/reftable: reload locked stack when preparing\ntransaction, 2024-09-24) we have introduced a new test that exercises\nhow the reftable backend behaves with many concurrent writers all racing\nwith each other. This test was introduced after a couple of fixes in\nthis context that should make concurrent writes behave gracefully. As it\nturns out though, Windows systems do not yet handle concurrent writes\nproperly, as we've got two reports for Cygwin and MinGW failing in this\nnewly added test.\n\nThe root cause of this is how we update the \"tables.list\" file: when\nwriting a new stack of tables we first write the data into a lockfile\nand then rename that file into place. But Windows forbids us from doing\nthat rename when the target path is open for reading by another process.\nAnd as the test races both readers and writers with each other we are\nquite likely to hit this edge case.\n\nNow the two reports are somewhat different from one another:\n\n - On Cygwin we hit timeouts because we fail to lock the \"tables.list\"\n file within 10 seconds. The renames themselves succeed even when the\n target file is open because Cygwin provides extensive compatibility\n logic to make them work even when the target file is open already.\n\n - On MinGW we hit I/O errors on rename. While we do have some retry\n logic in place to make the rename work in some cases, this is\n seemingly not sufficient when there is this much contention around\n the files.\n\nNeither of these cases is a regression: the logic didn't work before the\nmentioned commit, and after the commit it performs well on Linux, macOS\nand in Cygwin, and at least a bit better with MinGW. But the tests show\nthat we need to put more thought into how to make this work properly on\nMinGW systems.\n\nThe fact that Cygwin can work around this issue with better emulation of\nPOSIX-style atomic renames shows that we can in theory make MinGW work\nbetter, as well. But doing so likely requires quite some fiddling with\nWindows internals, and Git v2.47 is about to be released in a couple\ndays. This makes any potential fix quite risky as it would have to\nhappen deep down in our rename(3P) implementation in \"compat/mingw.c\".\n\nLet's instead work around both issues by disabling the test on MinGW\nand by significantly increasing the locking timeout for Cygwin. This\nbumped timeout also helps when running with e.g. the address and memory\nsanitizers, which also tend to significantly extend the runtime of this\ntest.\n\nThis should be revisited after Git v2.47 is out.\n\nSigned-off-by: Patrick Steinhardt \n---\n\nThis fix can be applied to remove some of the stress with the Git v2.47\nrelease pending. If would of course be preferable to find an alternate\nfix that makes MinGW work as required, but if you take the 500 lines of\ncode that is the rename(3P) implemenation of Cygwin as a hint you\nquickly figure out that this is a rather complex problem.\n\nPatrick\n\n t/t0610-reftable-basics.sh | 22 +++++++++++++++++-----\n 1 file changed, 17 insertions(+), 5 deletions(-)\n\ndiff --git a/t/t0610-reftable-basics.sh b/t/t0610-reftable-basics.sh\nindex 2d951c8ceb..86a746aff0 100755\n--- a/t/t0610-reftable-basics.sh\n+++ b/t/t0610-reftable-basics.sh\n@@ -450,15 +450,27 @@ test_expect_success 'ref transaction: retry acquiring tables.list lock' '\n \t)\n '\n \n-test_expect_success 'ref transaction: many concurrent writers' '\n+# This test fails most of the time on Windows systems. The root cause is\n+# that Windows does not allow us to rename the \"tables.list.lock\" file into\n+# place when \"tables.list\" is open for reading by a concurrent process.\n+#\n+# The same issue does not happen on Cygwin because its implementation of\n+# rename(3P) is emulating POSIX-style renames, including renames over files\n+# that are open.\n+test_expect_success !MINGW 'ref transaction: many concurrent writers' '\n \ttest_when_finished \"rm -rf repo\" &&\n \tgit init repo &&\n \t(\n \t\tcd repo &&\n-\t\t# Set a high timeout such that a busy CI machine will not abort\n-\t\t# early. 10 seconds should hopefully be ample of time to make\n-\t\t# this non-flaky.\n-\t\tgit config set reftable.lockTimeout 10000 &&\n+\t\t# Set a high timeout. While a couple of seconds should be\n+\t\t# plenty, using the address sanitizer will significantly slow\n+\t\t# us down here. Furthermore, Cygwin is also way slower due to\n+\t\t# the POSIX-style rename emulation. So we are aiming way higher\n+\t\t# than you would ever think is necessary just to keep us from\n+\t\t# flaking. We could also lock indefinitely by passing -1, but\n+\t\t# that could potentially block CI jobs indefinitely if there\n+\t\t# was a bug here.\n+\t\tgit config set reftable.lockTimeout 300000 &&\n \t\ttest_commit --no-tag initial &&\n \n \t\thead=$(git rev-parse HEAD) &&\n\nbase-commit: 111e864d69c84284441b083966c2065c2e9a4e78\n-- \n2.47.0.rc0.dirty\n\n"},{"id":"504088","messageId":"277c243f-7179-4946-99c8-b19ad5c85412@ramsayjones.plus.com","threadId":"62251","inReplyTo":"f83e23f1e76454a80e3e53cd02b3bb5bba6b8da1.1728044178.git.ps@pks.im","subject":"Re: [PATCH] t0610: work around flaky test with concurrent writers","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2024-10-04T14:47:01Z","receivedAt":"2024-10-04T14:50:12Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"Hi Patrick,\n\nOn 04/10/2024 13:16, Patrick Steinhardt wrote:\n[snip]\n\n> Now the two reports are somewhat different from one another:\n> \n> - On Cygwin we hit timeouts because we fail to lock the \"tables.list\"\n> file within 10 seconds. The renames themselves succeed even when the\n> target file is open because Cygwin provides extensive compatibility\n> logic to make them work even when the target file is open already.\n\nHmm, not so much for me! :(\n\n> \n> - On MinGW we hit I/O errors on rename. While we do have some retry\n> logic in place to make the rename work in some cases, this is\n> seemingly not sufficient when there is this much contention around\n> the files.\n\nI am seeing I/O errors.\n\n> \n> Neither of these cases is a regression: the logic didn't work before the\n> mentioned commit, and after the commit it performs well on Linux, macOS\n> and in Cygwin, and at least a bit better with MinGW. But the tests show\n> that we need to put more thought into how to make this work properly on\n> MinGW systems.\n\nOK\n\n> \n> The fact that Cygwin can work around this issue with better emulation of\n> POSIX-style atomic renames shows that we can in theory make MinGW work\n> better, as well. But doing so likely requires quite some fiddling with\n> Windows internals, and Git v2.47 is about to be released in a couple\n> days. This makes any potential fix quite risky as it would have to\n> happen deep down in our rename(3P) implementation in \"compat/mingw.c\".\n> \n> Let's instead work around both issues by disabling the test on MinGW\n> and by significantly increasing the locking timeout for Cygwin. This\n> bumped timeout also helps when running with e.g. the address and memory\n> sanitizers, which also tend to significantly extend the runtime of this\n> test.\n\nThis doesn't work for me.\n\n> \n> This should be revisited after Git v2.47 is out.\n> \n> Signed-off-by: Patrick Steinhardt \n> ---\n\n[PATCH snipped]\n\nAs I said last night, the first thing I tried was increasing the timeout\nten-fold (ie, to 100000), but that didn't change the outcome.\n\nI am also on windows 10 (on an *old* 4th gen core i5): \n \n $ uname -a\n CYGWIN_NT-10.0-19045 satellite 3.5.4-1.x86_64 2024-08-25 16:52 UTC x86_64 Cygwin\n $ \n\nI don't know why we appear to be seeing different behaviour. Since I had\nalready edited this test to up the timeout, I increased it again to match\nthe value in this (redacted) patch (ie I didn't actually apply your patch):\n\n $ git diff\n diff --git a/t/t0610-reftable-basics.sh b/t/t0610-reftable-basics.sh\n index 2d951c8ceb..7b6ded1c6a 100755\n --- a/t/t0610-reftable-basics.sh\n +++ b/t/t0610-reftable-basics.sh\n @@ -458,7 +458,7 @@ test_expect_success 'ref transaction: many concurrent writers' '\n # Set a high timeout such that a busy CI machine will not abort\n # early. 10 seconds should hopefully be ample of time to make\n # this non-flaky.\n - git config set reftable.lockTimeout 10000 &&\n + git config set reftable.lockTimeout 300000 &&\n test_commit --no-tag initial &&\n \n head=$(git rev-parse HEAD) &&\n $ \n\nHowever, apart from less debugging output, the result was essentially the\nsame:\n\n ...\n \n ++ wait\n ++ git update-ref refs/heads/branch-100 HEAD\n fatal: update_ref failed for ref 'refs/heads/branch-97': reftable: transaction failure: I/O error\n fatal: update_ref failed for ref 'refs/heads/branch-87': reftable: transaction prepare: I/O error\n ++ git for-each-ref --sort=v:refname\n ++ test_cmp expect actual\n ++ test 2 -ne 2\n ++ eval 'diff -u' '\"$@\"'\n +++ diff -u expect actual\n --- expect 2024-10-04 13:31:42.450969800 +0000\n +++ actual 2024-10-04 13:32:07.097150600 +0000\n @@ -84,7 +84,6 @@\n 68d032e9edd3481ac96382786ececc37ec28709e commit refs/heads/branch-84\n 68d032e9edd3481ac96382786ececc37ec28709e commit refs/heads/branch-85\n 68d032e9edd3481ac96382786ececc37ec28709e commit refs/heads/branch-86\n -68d032e9edd3481ac96382786ececc37ec28709e commit refs/heads/branch-87\n 68d032e9edd3481ac96382786ececc37ec28709e commit refs/heads/branch-88\n 68d032e9edd3481ac96382786ececc37ec28709e commit refs/heads/branch-89\n 68d032e9edd3481ac96382786ececc37ec28709e commit refs/heads/branch-90\n error: last command exited with $?=1\n not ok 47 - ref transaction: many concurrent writers\n \n ...\n\nIn an earlier email you suggested a timeout of -1, thus:\n\n $ git diff\n diff --git a/t/t0610-reftable-basics.sh b/t/t0610-reftable-basics.sh\n index 2d951c8ceb..9ab37a8d69 100755\n --- a/t/t0610-reftable-basics.sh\n +++ b/t/t0610-reftable-basics.sh\n @@ -458,7 +458,7 @@ test_expect_success 'ref transaction: many concurrent writers' '\n # Set a high timeout such that a busy CI machine will not abort\n # early. 10 seconds should hopefully be ample of time to make\n # this non-flaky.\n - git config set reftable.lockTimeout 10000 &&\n + git config set reftable.lockTimeout -1 &&\n test_commit --no-tag initial &&\n \n head=$(git rev-parse HEAD) &&\n $ \n\nThe result looks familiar:\n \n ++ wait\n ++ git update-ref refs/heads/branch-100 HEAD\n fatal: update_ref failed for ref 'refs/heads/branch-46': reftable: transaction prepare: I/O error\n ++ git for-each-ref --sort=v:refname\n ++ test_cmp expect actual\n ++ test 2 -ne 2\n ++ eval 'diff -u' '\"$@\"'\n +++ diff -u expect actual\n --- expect\t2024-10-04 14:35:20.159564800 +0000\n +++ actual\t2024-10-04 14:35:43.304762500 +0000\n @@ -43,7 +43,6 @@\n 68d032e9edd3481ac96382786ececc37ec28709e commit\trefs/heads/branch-43\n 68d032e9edd3481ac96382786ececc37ec28709e commit\trefs/heads/branch-44\n 68d032e9edd3481ac96382786ececc37ec28709e commit\trefs/heads/branch-45\n -68d032e9edd3481ac96382786ececc37ec28709e commit\trefs/heads/branch-46\n 68d032e9edd3481ac96382786ececc37ec28709e commit\trefs/heads/branch-47\n 68d032e9edd3481ac96382786ececc37ec28709e commit\trefs/heads/branch-48\n 68d032e9edd3481ac96382786ececc37ec28709e commit\trefs/heads/branch-49\n error: last command exited with $?=1\n not ok 47 - ref transaction: many concurrent writers\n\nCan you think of anything else to try?\n \nI would strongly suggest skipping this test on cygwin as well as MINGW.\n\nATB,\nRamsay Jones\n\n \n"},{"id":"504090","messageId":"ZwAJIRPUrxu5ox3w@pks.im","threadId":"62251","inReplyTo":"277c243f-7179-4946-99c8-b19ad5c85412@ramsayjones.plus.com","subject":"Re: [PATCH] t0610: work around flaky test with concurrent writers","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-10-04T15:26:25Z","receivedAt":"2024-10-04T15:26:31Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Oct 04, 2024 at 03:47:01PM +0100, Ramsay Jones wrote:\n> On 04/10/2024 13:16, Patrick Steinhardt wrote:\n> [snip]\n> \n> > Now the two reports are somewhat different from one another:\n> > \n> > - On Cygwin we hit timeouts because we fail to lock the \"tables.list\"\n> > file within 10 seconds. The renames themselves succeed even when the\n> > target file is open because Cygwin provides extensive compatibility\n> > logic to make them work even when the target file is open already.\n> \n> Hmm, not so much for me! :(\n> \n> > \n> > - On MinGW we hit I/O errors on rename. While we do have some retry\n> > logic in place to make the rename work in some cases, this is\n> > seemingly not sufficient when there is this much contention around\n> > the files.\n> \n> I am seeing I/O errors.\n\nInteresting! I wonder why I don't see them. Maybe it's boiling down to\ntiming again.\n\n[snip]\n> Can you think of anything else to try?\n\nNot really, no. I'd be curious whether Windows 11 has the same failure\nmode for Cygwin, but cannot test it myself.\n\n> I would strongly suggest skipping this test on cygwin as well as MINGW.\n\nYup, I definitely agree. I was operating under the assumption that\nCygwin works alright. Now that we know that it doesn't we should adapt\nthe condition from \"!MINGW\" to \"!WINDOWS\".\n\nPatrick\n"},{"id":"504091","messageId":"f3874c7e923846e453499e89f00dd4f8862e4c40.1728055880.git.ps@pks.im","threadId":"62251","inReplyTo":"b1b5fb40-f6c2-4621-b58c-9b7c8c64cc01@ramsayjones.plus.com","subject":"[PATCH v2] t0610: work around flaky test with concurrent writers","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-10-04T15:32:16Z","receivedAt":"2024-10-04T15:32:28Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"In 6241ce2170 (refs/reftable: reload locked stack when preparing\ntransaction, 2024-09-24) we have introduced a new test that exercises\nhow the reftable backend behaves with many concurrent writers all racing\nwith each other. This test was introduced after a couple of fixes in\nthis context that should make concurrent writes behave gracefully. As it\nturns out though, Windows systems do not yet handle concurrent writes\nproperly, as we've got two reports for Cygwin and MinGW failing in this\nnewly added test.\n\nThe root cause of this is how we update the \"tables.list\" file: when\nwriting a new stack of tables we first write the data into a lockfile\nand then rename that file into place. But Windows forbids us from doing\nthat rename when the target path is open for reading by another process.\nAnd as the test races both readers and writers with each other we are\nquite likely to hit this edge case.\n\nThis is not a regression: the logic didn't work before the mentioned\ncommit, and after the commit it performs well on Linux and macOS, and\nthe situation on Windows should have at least improved a bit. But the\ntest shows that we need to put more thought into how to make this work\nproperly there.\n\nWork around the issue by disabling the test on Windows for now. While at\nit, increase the locking timeout to address reported timeouts when using\neither the address or memory sanitizer, which also tend to significantly\nextend the runtime of this test.\n\nThis should be revisited after Git v2.47 is out.\n\nSigned-off-by: Patrick Steinhardt \n---\n t/t0610-reftable-basics.sh | 17 ++++++++++++-----\n 1 file changed, 12 insertions(+), 5 deletions(-)\n\ndiff --git a/t/t0610-reftable-basics.sh b/t/t0610-reftable-basics.sh\nindex 2d951c8ceb..babec7993e 100755\n--- a/t/t0610-reftable-basics.sh\n+++ b/t/t0610-reftable-basics.sh\n@@ -450,15 +450,22 @@ test_expect_success 'ref transaction: retry acquiring tables.list lock' '\n \t)\n '\n \n-test_expect_success 'ref transaction: many concurrent writers' '\n+# This test fails most of the time on Windows systems. The root cause is\n+# that Windows does not allow us to rename the \"tables.list.lock\" file into\n+# place when \"tables.list\" is open for reading by a concurrent process.\n+test_expect_success !WINDOWS 'ref transaction: many concurrent writers' '\n \ttest_when_finished \"rm -rf repo\" &&\n \tgit init repo &&\n \t(\n \t\tcd repo &&\n-\t\t# Set a high timeout such that a busy CI machine will not abort\n-\t\t# early. 10 seconds should hopefully be ample of time to make\n-\t\t# this non-flaky.\n-\t\tgit config set reftable.lockTimeout 10000 &&\n+\t\t# Set a high timeout. While a couple of seconds should be\n+\t\t# plenty, using the address sanitizer will significantly slow\n+\t\t# us down here. So we are aiming way higher than you would ever\n+\t\t# think is necessary just to keep us from flaking. We could\n+\t\t# also lock indefinitely by passing -1, but that could\n+\t\t# potentially block CI jobs indefinitely if there was a bug\n+\t\t# here.\n+\t\tgit config set reftable.lockTimeout 300000 &&\n \t\ttest_commit --no-tag initial &&\n \n \t\thead=$(git rev-parse HEAD) &&\n\nInterdiff against v1:\n diff --git a/t/t0610-reftable-basics.sh b/t/t0610-reftable-basics.sh\n index 86a746aff0..babec7993e 100755\n --- a/t/t0610-reftable-basics.sh\n +++ b/t/t0610-reftable-basics.sh\n @@ -453,23 +453,18 @@ test_expect_success 'ref transaction: retry acquiring tables.list lock' '\n # This test fails most of the time on Windows systems. The root cause is\n # that Windows does not allow us to rename the \"tables.list.lock\" file into\n # place when \"tables.list\" is open for reading by a concurrent process.\n -#\n -# The same issue does not happen on Cygwin because its implementation of\n -# rename(3P) is emulating POSIX-style renames, including renames over files\n -# that are open.\n -test_expect_success !MINGW 'ref transaction: many concurrent writers' '\n +test_expect_success !WINDOWS 'ref transaction: many concurrent writers' '\n \ttest_when_finished \"rm -rf repo\" &&\n \tgit init repo &&\n \t(\n \t\tcd repo &&\n \t\t# Set a high timeout. While a couple of seconds should be\n \t\t# plenty, using the address sanitizer will significantly slow\n -\t\t# us down here. Furthermore, Cygwin is also way slower due to\n -\t\t# the POSIX-style rename emulation. So we are aiming way higher\n -\t\t# than you would ever think is necessary just to keep us from\n -\t\t# flaking. We could also lock indefinitely by passing -1, but\n -\t\t# that could potentially block CI jobs indefinitely if there\n -\t\t# was a bug here.\n +\t\t# us down here. So we are aiming way higher than you would ever\n +\t\t# think is necessary just to keep us from flaking. We could\n +\t\t# also lock indefinitely by passing -1, but that could\n +\t\t# potentially block CI jobs indefinitely if there was a bug\n +\t\t# here.\n \t\tgit config set reftable.lockTimeout 300000 &&\n \t\ttest_commit --no-tag initial &&\n \n\nbase-commit: 111e864d69c84284441b083966c2065c2e9a4e78\n-- \n2.47.0.rc0.dirty\n\n"},{"id":"504093","messageId":"xmqqbjzzg89u.fsf@gitster.g","threadId":"62251","inReplyTo":"f29241a7-aadd-e824-97f3-a95ac6619951@gmx.de","subject":"Re: v2.47.0-rc1 test failure on cygwin","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-10-04T16:09:01Z","receivedAt":"2024-10-04T16:09:04Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n>> But unfortunately this still caused permission errors when the new path\n>> was held open by another process.\n>\n> Yes, this will _always_ be a problem, I think. The\n> `FILE_RENAME_POSIX_SEMANTICS` as per its documentation should help, but if\n> it does not in your tests it might actually not quite work as advertised\n> (wouldn't be the first time I encounter such an issue).\n>\n> I tried to read through the code (it's a lot!) to figure out whether there\n> is potentially any situation when the `tables.list` file is opened but not\n> closed immediately, but couldn't find any. Do you know off-hand of any\n> such scenario?\n>\n>> I think for now I'd still lean into the direction of adding the !WINDOWS\n>> prerequisite to the test and increasing timeouts such that I can\n>> continue to investigate without time pressure.\n>\n> Let me bang my head against this problem for a little while longer. You\n> might be right, though, that this is a thing we cannot fix in time for\n> v2.47.0, which would be sad.\n\nIf you folks think it would help stabilizing the tentative fix, I am\nopen to the idea of delaying the 2.47 by a few days. Currently the\n2.47-final is scheduled on the 7th (Monday), but we can do 2.47-rc2\non that day instead, and move the final to 10th (Thu) or 11th (Fri)\n[*].\n\nThanks, all, for working together.\n\n\n[Footnote]\n\n * All dates are US/Pacific, 10:00 am\n"},{"id":"504096","messageId":"xmqq7cang7nu.fsf@gitster.g","threadId":"62251","inReplyTo":"f83e23f1e76454a80e3e53cd02b3bb5bba6b8da1.1728044178.git.ps@pks.im","subject":"Re: [PATCH] t0610: work around flaky test with concurrent writers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-10-04T16:22:13Z","receivedAt":"2024-10-04T16:22:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> This fix can be applied to remove some of the stress with the Git v2.47\n> release pending. If would of course be preferable to find an alternate\n> fix that makes MinGW work as required, but if you take the 500 lines of\n> code that is the rename(3P) implemenation of Cygwin as a hint you\n> quickly figure out that this is a rather complex problem.\n\nYup, this is good for 2.47 and I agree it is sensible to punt the\nreal solution after the upcoming release.\n\nThanks.\n"},{"id":"504098","messageId":"xmqqy133esmx.fsf@gitster.g","threadId":"62251","inReplyTo":"277c243f-7179-4946-99c8-b19ad5c85412@ramsayjones.plus.com","subject":"Re: [PATCH] t0610: work around flaky test with concurrent writers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-10-04T16:32:06Z","receivedAt":"2024-10-04T16:32:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramsay Jones writes:\n\n> This doesn't work for me.\n> Can you think of anything else to try?\n> \n> I would strongly suggest skipping this test on cygwin as well as MINGW.\n\nThere was some mention of \"Win 10 or later\" in the thread, IIRC,\nwhile explaining why Cygwin version of rename() works. Is it\npossible that the version of Windows and Cygwin you and Patrick\nused are different enough?\n\nIn the meantime, I'll queue a SQUASH??? on top for preparing today's\nintegration.\n\nThis is a tangent, but there are places that use !MINGW,!CYGWIN and\nthere are places that use !WINDOWS; I think they are equivalent\nprerequisites that we might want to straighten out their uses as a\nclean-up later.\n\n\n\n t/t0610-reftable-basics.sh | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git c/t/t0610-reftable-basics.sh w/t/t0610-reftable-basics.sh\nindex 86a746aff0..e10c894cb0 100755\n--- c/t/t0610-reftable-basics.sh\n+++ w/t/t0610-reftable-basics.sh\n@@ -454,10 +454,10 @@ test_expect_success 'ref transaction: retry acquiring tables.list lock' '\n # that Windows does not allow us to rename the \"tables.list.lock\" file into\n # place when \"tables.list\" is open for reading by a concurrent process.\n #\n-# The same issue does not happen on Cygwin because its implementation of\n+# The same issue may not happen on Cygwin because its implementation of\n # rename(3P) is emulating POSIX-style renames, including renames over files\n-# that are open.\n-test_expect_success !MINGW 'ref transaction: many concurrent writers' '\n+# that are open, but it probably depends on the version.\n+test_expect_success !WINDOWS 'ref transaction: many concurrent writers' '\n \ttest_when_finished \"rm -rf repo\" &&\n \tgit init repo &&\n \t(\n"},{"id":"504099","messageId":"7bf0db35-e8a2-4c06-844b-5b3a1af3c4a4@ramsayjones.plus.com","threadId":"62251","inReplyTo":"f3874c7e923846e453499e89f00dd4f8862e4c40.1728055880.git.ps@pks.im","subject":"Re: [PATCH v2] t0610: work around flaky test with concurrent writers","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2024-10-04T16:32:25Z","receivedAt":"2024-10-04T16:32:35Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"Hi Patrick,\n\nOn 04/10/2024 16:32, Patrick Steinhardt wrote:\n> In 6241ce2170 (refs/reftable: reload locked stack when preparing\n> transaction, 2024-09-24) we have introduced a new test that exercises\n> how the reftable backend behaves with many concurrent writers all racing\n> with each other. This test was introduced after a couple of fixes in\n> this context that should make concurrent writes behave gracefully. As it\n> turns out though, Windows systems do not yet handle concurrent writes\n> properly, as we've got two reports for Cygwin and MinGW failing in this\n> newly added test.\n> \n> The root cause of this is how we update the \"tables.list\" file: when\n> writing a new stack of tables we first write the data into a lockfile\n> and then rename that file into place. But Windows forbids us from doing\n> that rename when the target path is open for reading by another process.\n> And as the test races both readers and writers with each other we are\n> quite likely to hit this edge case.\n> \n> This is not a regression: the logic didn't work before the mentioned\n> commit, and after the commit it performs well on Linux and macOS, and\n> the situation on Windows should have at least improved a bit. But the\n> test shows that we need to put more thought into how to make this work\n> properly there.\n> \n> Work around the issue by disabling the test on Windows for now. While at\n> it, increase the locking timeout to address reported timeouts when using\n> either the address or memory sanitizer, which also tend to significantly\n> extend the runtime of this test.\n> \n> This should be revisited after Git v2.47 is out.\n> \n> Signed-off-by: Patrick Steinhardt \n> ---\n> t/t0610-reftable-basics.sh | 17 ++++++++++++-----\n> 1 file changed, 12 insertions(+), 5 deletions(-)\n> \n> diff --git a/t/t0610-reftable-basics.sh b/t/t0610-reftable-basics.sh\n> index 2d951c8ceb..babec7993e 100755\n> --- a/t/t0610-reftable-basics.sh\n> +++ b/t/t0610-reftable-basics.sh\n> @@ -450,15 +450,22 @@ test_expect_success 'ref transaction: retry acquiring tables.list lock' '\n> \t)\n> '\n> \n> -test_expect_success 'ref transaction: many concurrent writers' '\n> +# This test fails most of the time on Windows systems. The root cause is\n> +# that Windows does not allow us to rename the \"tables.list.lock\" file into\n> +# place when \"tables.list\" is open for reading by a concurrent process.\n> +test_expect_success !WINDOWS 'ref transaction: many concurrent writers' '\n> \ttest_when_finished \"rm -rf repo\" &&\n> \tgit init repo &&\n> \t(\n> \t\tcd repo &&\n> -\t\t# Set a high timeout such that a busy CI machine will not abort\n> -\t\t# early. 10 seconds should hopefully be ample of time to make\n> -\t\t# this non-flaky.\n> -\t\tgit config set reftable.lockTimeout 10000 &&\n> +\t\t# Set a high timeout. While a couple of seconds should be\n> +\t\t# plenty, using the address sanitizer will significantly slow\n> +\t\t# us down here. So we are aiming way higher than you would ever\n> +\t\t# think is necessary just to keep us from flaking. We could\n> +\t\t# also lock indefinitely by passing -1, but that could\n> +\t\t# potentially block CI jobs indefinitely if there was a bug\n> +\t\t# here.\n> +\t\tgit config set reftable.lockTimeout 300000 &&\n> \t\ttest_commit --no-tag initial &&\n> \n> \t\thead=$(git rev-parse HEAD) &&\n\nI did apply this patch and (unsurprising) it now passes just fine! :)\n\nATB,\nRamsay Jones\n\n"},{"id":"504100","messageId":"xmqqttdresgo.fsf@gitster.g","threadId":"62251","inReplyTo":"f3874c7e923846e453499e89f00dd4f8862e4c40.1728055880.git.ps@pks.im","subject":"Re: [PATCH v2] t0610: work around flaky test with concurrent writers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-10-04T16:35:51Z","receivedAt":"2024-10-04T16:35:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> This should be revisited after Git v2.47 is out.\n>\n> Signed-off-by: Patrick Steinhardt \n> ---\n> t/t0610-reftable-basics.sh | 17 ++++++++++++-----\n> 1 file changed, 12 insertions(+), 5 deletions(-)\n\nAh, I spoke before I finished my inbox. Let's use this version.\n\nThanks.\n"},{"id":"504103","messageId":"ZwAiXuNPK_SdnfEz@pks.im","threadId":"62251","inReplyTo":"xmqqbjzzg89u.fsf@gitster.g","subject":"Re: v2.47.0-rc1 test failure on cygwin","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-10-04T17:14:14Z","receivedAt":"2024-10-04T17:14:20Z","isPatch":false,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Oct 04, 2024 at 09:09:01AM -0700, Junio C Hamano wrote:\n> Johannes Schindelin writes:\n> \n> >> But unfortunately this still caused permission errors when the new path\n> >> was held open by another process.\n> >\n> > Yes, this will _always_ be a problem, I think. The\n> > `FILE_RENAME_POSIX_SEMANTICS` as per its documentation should help, but if\n> > it does not in your tests it might actually not quite work as advertised\n> > (wouldn't be the first time I encounter such an issue).\n> >\n> > I tried to read through the code (it's a lot!) to figure out whether there\n> > is potentially any situation when the `tables.list` file is opened but not\n> > closed immediately, but couldn't find any. Do you know off-hand of any\n> > such scenario?\n> >\n> >> I think for now I'd still lean into the direction of adding the !WINDOWS\n> >> prerequisite to the test and increasing timeouts such that I can\n> >> continue to investigate without time pressure.\n> >\n> > Let me bang my head against this problem for a little while longer. You\n> > might be right, though, that this is a thing we cannot fix in time for\n> > v2.47.0, which would be sad.\n> \n> If you folks think it would help stabilizing the tentative fix, I am\n> open to the idea of delaying the 2.47 by a few days. Currently the\n> 2.47-final is scheduled on the 7th (Monday), but we can do 2.47-rc2\n> on that day instead, and move the final to 10th (Thu) or 11th (Fri)\n> [*].\n> \n> Thanks, all, for working together.\n> \n> \n> [Footnote]\n> \n> * All dates are US/Pacific, 10:00 am\n\nRight now I don't yet have a good idea for how to fix the issue. I've\nbeen trying a bunch of different things that, according to Windows docs,\nshould've made the renames work. But they didn't, and I don't really\nhave an alternative right now. So I'll need to keep on thinking about\nthis, and maybe get some more help from people familiar with Windows.\n\nSo deferring Git 2.47 because of it probably does not make much sense\nunless somebody can up with a solution. Also, as noted already, this is\nnot a regression, the behaviour actually improved even on Windows. Not\nto the degree I was hoping for, but at least a bit. So that's another\nreason why I don't think it is worth deferring over this.\n\nThanks!\n\nPatrick\n"},{"id":"504105","messageId":"xmqqed4veosy.fsf@gitster.g","threadId":"62251","inReplyTo":"ZwAiXuNPK_SdnfEz@pks.im","subject":"Re: v2.47.0-rc1 test failure on cygwin","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-10-04T17:54:53Z","receivedAt":"2024-10-04T17:54:56Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> Right now I don't yet have a good idea for how to fix the issue. I've\n> been trying a bunch of different things that, according to Windows docs,\n> should've made the renames work. But they didn't, and I don't really\n> have an alternative right now. So I'll need to keep on thinking about\n> this, and maybe get some more help from people familiar with Windows.\n>\n> So deferring Git 2.47 because of it probably does not make much sense\n> unless somebody can up with a solution. Also, as noted already, this is\n> not a regression, the behaviour actually improved even on Windows. Not\n> to the degree I was hoping for, but at least a bit. So that's another\n> reason why I don't think it is worth deferring over this.\n\nOK. \n\nI was thinking that we may need more time to cook the real fix on\ntarget (read: not Windows) platforms, but if that is not the case,\nthen let's stick to the original schedule.\n\nThanks.\n"},{"id":"504118","messageId":"20241004224105.GB580885@coredump.intra.peff.net","threadId":"62251","inReplyTo":"f3874c7e923846e453499e89f00dd4f8862e4c40.1728055880.git.ps@pks.im","subject":"Re: [PATCH v2] t0610: work around flaky test with concurrent writers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-10-04T22:41:05Z","receivedAt":"2024-10-04T22:41:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Oct 04, 2024 at 05:32:16PM +0200, Patrick Steinhardt wrote:\n\n> Work around the issue by disabling the test on Windows for now. While at\n> it, increase the locking timeout to address reported timeouts when using\n> either the address or memory sanitizer, which also tend to significantly\n> extend the runtime of this test.\n> \n> This should be revisited after Git v2.47 is out.\n\nThis sounds OK to me.\n\nIn general, I worry about papering over bugs with a \"we'll revisit it\nlater\" note. We may forget about things or never prioritize them. And\nthe worst-case scenario here is that we might later decide to promote\nreftable to the default format, forgetting that concurrency has problems\non Windows.\n\nBut given that it's an area you've been actively working in, there's\nalready been some discussion towards the real fix, and we still consider\nreftables somewhat experimental, I feel OK bumping it for now.\n\nThanks for all the digging on this (from you and from other folks).\n\n-Peff\n"}]}