maintenance(geometric): avoid deadlocks on Windows 10#2103
maintenance(geometric): avoid deadlocks on Windows 10#2103dscho wants to merge 2 commits intogitgitgadget:masterfrom
Conversation
At some point between Windows 10 Build 17134.1304 and Build 18363.657, the default behavior of `DeleteFileW()` was changed to use POSIX semantics (https://stackoverflow.com/a/60512798). Under those semantics, a file can be deleted even when another process holds an active `MapViewOfFile` view on it: the directory entry is removed immediately, but the underlying data persists until the last handle is closed. On older Windows versions (and Windows 10 builds before that change), `DeleteFileW()` uses legacy semantics where deletion fails outright if any process holds a file mapping. To allow testing code paths that depend on the legacy behavior, introduce a `GIT_TEST_LEGACY_DELETE` environment variable. When set, `mingw_unlink()` uses `SetFileInformationByHandle()` with `FileDispositionInfo` (the non-POSIX variant) instead of `DeleteFileW()`, forcing legacy delete semantics regardless of the Windows version. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
As is done for all the other maintenance tasks, let's release the ODB also before starting the geometric repacking. That way, the `.idx` files won't be `mmap()`ed when they are to be deleted (which does not work on Windows because you cannot delete files on that platform as long as they are kept open by a process). This regression was introduced by 9bc1518 (builtin/maintenance: introduce "geometric-repack" task, 2025-10-24), but was only noticed once geometric repacking was made the default in 452b12c (builtin/ maintenance: use "geometric" strategy by default, 2026-02-24). The fix recapitulates my work from df76ee7 (run-command: offer to close the object store before running, 2021-09-09) & friends. To guard against future regressions of this kind, add a check to `run_and_verify_geometric_pack()` in `t7900` that detects orphaned `.idx` files left behind after repacking. Contrary to interactive calls, the `git maintenance` call in that test case would _not_ block on Windows, asking whether to retry deleting that file, which is the reason why this bug was not caught earlier. Furthermore, since the default behavior of `DeleteFileW()` was changed at some point between Windows 10 Build 17134.1304 and Build 18363.657 to use POSIX semantics (see https://stackoverflow.com/a/60512798), the added orphaned-`.idx` check would be insufficient to catch this regression on modern Windows without emulating legacy delete semantics via `GIT_TEST_LEGACY_DELETE=1`. This fixes git-for-windows#6210. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
47c2b34 to
12ebd5c
Compare
|
/submit |
|
Submitted as pull.2103.git.1777380768.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
Derrick Stolee wrote on the Git mailing list (how to reply to this email): On 4/28/2026 8:52 AM, Johannes Schindelin via GitGitGadget wrote:
> On Windows, maintenance_task_geometric_repack() opens pack index files via
> pack_geometry_init() (which mmap()s the .idx files), then spawns git repack
> as a child process without setting child.odb_to_close. The parent's mmap()s
> prevent the child from deleting old .idx files.
>
> On Windows 10 builds before the POSIX delete semantics change (between Build
> 17134.1304 and 18363.657, see https://stackoverflow.com/a/60512798), this
> results in Unlink of file '.git/objects/pack/pack-<hash>.idx' failed. Should
> I try again? during fetch-triggered auto-maintenance with the geometric
> strategy.
>
> The fix adds the missing child.odb_to_close = the_repository->objects line,
> matching all other maintenance tasks.
>
> The first commit introduces a GIT_TEST_LEGACY_DELETE environment variable to
> simulate legacy (pre-POSIX) delete semantics on modern Windows, so the
> regression test can verify the fix even on Windows 11.
>
> This fixes https://github.com/git-for-windows/git/issues/6210.
Thanks for these patches. I reviewed their equivalents in the
git-for-windows/git fork so I'll give my LGTM here, too.
Thanks,
-Stolee |
|
User |
This is a companion of gitgitgadget#2103 and of git-for-windows#6215. On Windows, `maintenance_task_geometric_repack()` opens pack index files via `pack_geometry_init()` (which `mmap()`s the `.idx` files), then spawns `git repack` as a child process without setting `child.odb_to_close`. The parent's `mmap()`s prevent the child from deleting old `.idx` files. On Windows 10 builds before the POSIX delete semantics change (between Build 17134.1304 and 18363.657, see https://stackoverflow.com/a/60512798), this results in `Unlink of file '.git/objects/pack/pack-<hash>.idx' failed. Should I try again?` during fetch-triggered auto-maintenance with the geometric strategy. The fix adds the missing `child.odb_to_close = the_repository->objects` line, matching all other maintenance tasks. The first commit introduces a `GIT_TEST_LEGACY_DELETE` environment variable to simulate legacy (pre-POSIX) delete semantics on modern Windows, so the regression test can verify the fix even on Windows 11.
This PR is a companion of gitgitgadget#2103. On Windows, `maintenance_task_geometric_repack()` opens pack index files via `pack_geometry_init()` (which `mmap()`s the `.idx` files), then spawns `git repack` as a child process without setting `child.odb_to_close`. The parent's `mmap()`s prevent the child from deleting old `.idx` files. On Windows 10 builds before the POSIX delete semantics change (between Build 17134.1304 and 18363.657, see https://stackoverflow.com/a/60512798), this results in `Unlink of file '.git/objects/pack/pack-<hash>.idx' failed. Should I try again?` during fetch-triggered auto-maintenance with the geometric strategy. The fix adds the missing `child.odb_to_close = the_repository->objects` line, matching all other maintenance tasks. The first commit introduces a `GIT_TEST_LEGACY_DELETE` environment variable to simulate legacy (pre-POSIX) delete semantics on modern Windows, so the regression test can verify the fix even on Windows 11. This fixes #6210. Tested-by: Patryk Miś <foss@patrykmis.com>
|
This branch is now known as |
|
This patch series was integrated into seen via git@cf5abd7. |
|
There was a status update in the "New Topics" section about the branch To help Windows 10 installations, avoid removing files whose contents are still mmap()'ed. Will merge to 'next'? source: <pull.2103.git.1777380768.gitgitgadget@gmail.com> |
This PR is a companion of gitgitgadget#2103. On Windows, `maintenance_task_geometric_repack()` opens pack index files via `pack_geometry_init()` (which `mmap()`s the `.idx` files), then spawns `git repack` as a child process without setting `child.odb_to_close`. The parent's `mmap()`s prevent the child from deleting old `.idx` files. On Windows 10 builds before the POSIX delete semantics change (between Build 17134.1304 and 18363.657, see https://stackoverflow.com/a/60512798), this results in `Unlink of file '.git/objects/pack/pack-<hash>.idx' failed. Should I try again?` during fetch-triggered auto-maintenance with the geometric strategy. The fix adds the missing `child.odb_to_close = the_repository->objects` line, matching all other maintenance tasks. The first commit introduces a `GIT_TEST_LEGACY_DELETE` environment variable to simulate legacy (pre-POSIX) delete semantics on modern Windows, so the regression test can verify the fix even on Windows 11. This fixes #6210. Tested-by: Patryk Miś <foss@patrykmis.com>
This PR is a companion of gitgitgadget#2103. On Windows, `maintenance_task_geometric_repack()` opens pack index files via `pack_geometry_init()` (which `mmap()`s the `.idx` files), then spawns `git repack` as a child process without setting `child.odb_to_close`. The parent's `mmap()`s prevent the child from deleting old `.idx` files. On Windows 10 builds before the POSIX delete semantics change (between Build 17134.1304 and 18363.657, see https://stackoverflow.com/a/60512798), this results in `Unlink of file '.git/objects/pack/pack-<hash>.idx' failed. Should I try again?` during fetch-triggered auto-maintenance with the geometric strategy. The fix adds the missing `child.odb_to_close = the_repository->objects` line, matching all other maintenance tasks. The first commit introduces a `GIT_TEST_LEGACY_DELETE` environment variable to simulate legacy (pre-POSIX) delete semantics on modern Windows, so the regression test can verify the fix even on Windows 11. This fixes #6210. Tested-by: Patryk Miś <foss@patrykmis.com>
| @@ -449,20 +449,63 @@ static wchar_t *normalize_ntpath(wchar_t *wbuf) | |||
| return wbuf; | |||
There was a problem hiding this comment.
Patrick Steinhardt wrote on the Git mailing list (how to reply to this email):
On Tue, Apr 28, 2026 at 12:52:47PM +0000, Johannes Schindelin via GitGitGadget wrote:
> diff --git a/compat/mingw.c b/compat/mingw.c
> index 2023c16db6..04f9aa3922 100644
> --- a/compat/mingw.c
> +++ b/compat/mingw.c
> @@ -449,20 +449,63 @@ static wchar_t *normalize_ntpath(wchar_t *wbuf)
> return wbuf;
> }
>
> +/*
> + * Use SetFileInformationByHandle(FileDispositionInfo) to force legacy
> + * (non-POSIX) delete semantics. On Windows 11, DeleteFileW() uses POSIX
> + * delete semantics internally, allowing deletion even with active
> + * MapViewOfFile views. This helper simulates Windows 10 behavior where
> + * deletion fails if a file mapping exists.
> + *
> + * Returns nonzero on success (like DeleteFileW), 0 on failure.
> + */
> +static int legacy_delete_file(const wchar_t *wpathname)
> +{
> + FILE_DISPOSITION_INFO fdi = { TRUE };
> + DWORD gle;
> + HANDLE h = CreateFileW(wpathname, DELETE,
> + FILE_SHARE_READ | FILE_SHARE_WRITE |
> + FILE_SHARE_DELETE,
> + NULL, OPEN_EXISTING,
> + FILE_FLAG_OPEN_REPARSE_POINT, NULL);
> + if (h == INVALID_HANDLE_VALUE)
> + return 0;
> +
> + if (SetFileInformationByHandle(h, FileDispositionInfo,
> + &fdi, sizeof(fdi))) {
> + CloseHandle(h);
> + return 1;
> + }
> + gle = GetLastError();
> + CloseHandle(h);
> + SetLastError(gle);
> + return 0;
> +}
> +
> +static int try_delete_file(const wchar_t *wpathname, int use_legacy)
> +{
> + if (use_legacy)
> + return legacy_delete_file(wpathname);
> + return DeleteFileW(wpathname);
> +}
> +
> int mingw_unlink(const char *pathname, int handle_in_use_error)
> {
> + static int use_legacy_delete = -1;
> int tries = 0;
> wchar_t wpathname[MAX_PATH];
> if (xutftowcs_path(wpathname, pathname) < 0)
> return -1;
>
> - if (DeleteFileW(wpathname))
> + if (use_legacy_delete < 0)
> + use_legacy_delete = !!getenv("GIT_TEST_LEGACY_DELETE");
Should this use `git_env_bool()`?
Patrick|
User |
|
Patrick Steinhardt wrote on the Git mailing list (how to reply to this email): On Tue, Apr 28, 2026 at 11:01:34AM -0400, Derrick Stolee wrote:
> On 4/28/2026 8:52 AM, Johannes Schindelin via GitGitGadget wrote:
> > On Windows, maintenance_task_geometric_repack() opens pack index files via
> > pack_geometry_init() (which mmap()s the .idx files), then spawns git repack
> > as a child process without setting child.odb_to_close. The parent's mmap()s
> > prevent the child from deleting old .idx files.
> >
> > On Windows 10 builds before the POSIX delete semantics change (between Build
> > 17134.1304 and 18363.657, see https://stackoverflow.com/a/60512798), this
> > results in Unlink of file '.git/objects/pack/pack-<hash>.idx' failed. Should
> > I try again? during fetch-triggered auto-maintenance with the geometric
> > strategy.
> >
> > The fix adds the missing child.odb_to_close = the_repository->objects line,
> > matching all other maintenance tasks.
> >
> > The first commit introduces a GIT_TEST_LEGACY_DELETE environment variable to
> > simulate legacy (pre-POSIX) delete semantics on modern Windows, so the
> > regression test can verify the fix even on Windows 11.
> >
> > This fixes https://github.com/git-for-windows/git/issues/6210.
>
> Thanks for these patches. I reviewed their equivalents in the
> git-for-windows/git fork so I'll give my LGTM here, too.
I've got a single comment on the first patch, but other than that this
series looks good to me. Thanks!
Patrick |
On Windows,
maintenance_task_geometric_repack()opens pack index files viapack_geometry_init()(whichmmap()s the.idxfiles), then spawnsgit repackas a child process without settingchild.odb_to_close. The parent'smmap()s prevent the child from deleting old.idxfiles.On Windows 10 builds before the POSIX delete semantics change (between Build 17134.1304 and 18363.657, see https://stackoverflow.com/a/60512798), this results in
Unlink of file '.git/objects/pack/pack-<hash>.idx' failed. Should I try again?during fetch-triggered auto-maintenance with the geometric strategy.The fix adds the missing
child.odb_to_close = the_repository->objectsline, matching all other maintenance tasks.The first commit introduces a
GIT_TEST_LEGACY_DELETEenvironment variable to simulate legacy (pre-POSIX) delete semantics on modern Windows, so the regression test can verify the fix even on Windows 11.This fixes git-for-windows#6210.
cc: Derrick Stolee stolee@gmail.com
cc: Patrick Steinhardt ps@pks.im