Re: [PATCH v2 2/2] compat/mingw: fix EACCESS when opening files with `O_CREAT | O_EXCL`

> On Thu, 20 Mar 2025, Patrick Steinhardt wrote:
> > In our CI systems we can observe that t0610 fails rather frequently.
> > This testcase races a bunch of git-update-ref(1) processes with one
> > another which are all trying to update a unique reference, where we
> > expect that all processes succeed and end up updating the reftable
> > stack. The error message in this case looks like the following:
> >
> >     fatal: update_ref failed for ref 'refs/heads/branch-88': reftable: transaction prepare: I/O error
> >
> > Instrumenting the code with a couple of calls to `BUG()` in relevant
> > sites where we return `REFTABLE_IO_ERROR` quickly leads one to discover
> > that this error is caused when calling `flock_acquire()`, which is a
> > thin wrapper around our lockfile API. Curiously, the error code we get
> > in such cases is `EACCESS`, indicating that we are not allowed to access
> > the file.
> >
> > The root cause of this is an oddity of `CreateFileW()`, which is what
> > `_wopen()` uses internally. Quoting its documentation [1]:
> >
> >     If you call CreateFile on a file that is pending deletion as a
> >     result of a previous call to DeleteFile, the function fails. The
> >     operating system delays file deletion until all handles to the file
> >     are closed. GetLastError returns ERROR_ACCESS_DENIED.
> >
> > This behaviour is triggered quite often in the above testcase because
> > all the processes race with one another trying to acquire the lock for
> > the "tables.list" file. This is due to how locking works in the reftable
> > library when compacting a stack:
> >
> >     1. Lock the "tables.list" file and reads its contents.
> >
> >     2. Decide which tables to compact.
> >
> >     3. Lock each of the individual tables that we are about to compact.
> >
> >     4. Unlock the "tables.list" file.
> >
> >     5. Compact the individual tables into one large table.
> >
> >     6. Re-lock the "tables.list" file.
> >
> >     7. Write the new list of tables into it.
> >
> >     8. Commit the "tables.list" file.
> >
> > The important step is (4): we don't commit the file directly by renaming
> > it into place, but instead we delete the lockfile so that concurrent
> > processes can continue to append to the reftable stack while we compact
> > the tables. And because we use `DeleteFileW()` to do so, we may now race
> > with another process that wants to acquire that lockfile. So if we are
> > unlucky, we would now see `ERROR_ACCESS_DENIED` instead of the expected
> > `ERROR_FILE_EXISTS`, which the lockfile subsystem isn't prepared to
> > handle and thus it will bail out without retrying to acquire the lock.
> >
> > In theory, the issue is not limited to the reftable library and can be
> > triggered by every other user of the lockfile subsystem, as well. My gut
> > feeling tells me it's rather unlikely to surface elsewhere though.
> >
> > Fix the issue by translating the error to `EEXIST`. This makes the
> > lockfile subsystem handle the error correctly: in case a timeout is set
> > it will now retry acquiring the lockfile until the timeout has expired.
> >
> > With this, t0610 is now always passing on my machine whereas it was
> > previously failing in around 20-30% of all test runs.
> >
> > [1]: https://learn.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-createfilew
> 
> Couldn't we simply handle `EACCES` the same way as `EEXIST` in step 4?
> 
> This suggestion is different from v1, which would have affected all
> callers of `mingw_open()`.

> I know that e.g. PostgreSQL used this undocumented function at least at
> some stage, but SQLite avoided it by introducing a simple poll strategy.
> We could also do that, but if there is already code in the reftable
> library that skips doing things if a `.lock` file exists, then doing the
> same if the `.lock` file cannot be created, too, should be a safe argument
> to make.