Skip to content

Commit acd9fcb

Browse files
committed
fix(submodule): close repositories owned by update operations
The Windows Python 3.15 CI job failed to remove submodule checkouts because persistent `cat-file` processes still used them as working directories. `Submodule.update()` relied on collection of its temporary `Repo`, but captured log records retained a `Head` argument and therefore the repository and its process. Recursive updates also opened an extra unbounded repository. Close the owned repository after updates and on errors, and reuse it for recursion with final cleanup. This preserves `keep_going` behavior while releasing processes even when logs or callbacks retain repository objects. The compatibility test now scopes its own repository and closes it before removal; allocation tracing identified those separate caller-owned handles. Four regressions retain real logging arguments and verify process cleanup for normal, failing, recursive, and recursive `keep_going` updates. Both previously failing tests pass with tracing asserting no live checkout processes at each removal. Ruff, mypy, basedpyright, and `git diff --check` pass locally. Native Windows validation will run in CI.
1 parent e3d7f66 commit acd9fcb

2 files changed

Lines changed: 66 additions & 11 deletions

File tree

‎git/objects/submodule/base.py‎

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1128,17 +1128,19 @@ def fetch_remotes(module_repo: "Repo") -> None:
11281128
)
11291129
# END update to new commit only if needed
11301130
except Exception as err:
1131+
if mrepo is not None:
1132+
mrepo.close()
11311133
if not keep_going:
11321134
raise
11331135
_logger.error(str(err))
11341136
# END handle keep_going
11351137

11361138
# HANDLE RECURSION
11371139
##################
1138-
if recursive:
1140+
try:
11391141
# In dry_run mode, the module might not exist.
1140-
if mrepo is not None:
1141-
for submodule in self.iter_items(self.module()):
1142+
if recursive and mrepo is not None:
1143+
for submodule in self.iter_items(mrepo):
11421144
submodule.update(
11431145
recursive,
11441146
init,
@@ -1151,7 +1153,11 @@ def fetch_remotes(module_repo: "Repo") -> None:
11511153
)
11521154
# END handle recursive update
11531155
# END handle dry run
1154-
# END for each submodule
1156+
finally:
1157+
# Log records and progress callbacks can retain refs to this repository.
1158+
# Its cat-file processes must not keep the checkout open on Windows.
1159+
if mrepo is not None:
1160+
mrepo.close()
11551161

11561162
return self
11571163

‎test/test_submodule.py‎

Lines changed: 56 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -396,6 +396,53 @@ def capture_process(self, command, *args, **kwargs):
396396
process.wait()
397397

398398

399+
@pytest.mark.parametrize("operation", ["update", "error", "recursive", "keep-going"])
400+
def test_update_closes_checkout_processes_retained_by_logging(movable_submodule, monkeypatch, caplog, operation):
401+
sm = movable_submodule
402+
recursive = operation in ("recursive", "keep-going")
403+
with sm.module() as module:
404+
module.head.reference.set_tracking_branch(None)
405+
if recursive:
406+
child = module.create_submodule("child", "child", sm.url)
407+
module.index.commit("Add child")
408+
sm.binsha = module.head.commit.binsha
409+
with child.module() as nested:
410+
nested.head.reference.set_tracking_branch(None)
411+
if operation in ("error", "keep-going"):
412+
sm.binsha = b"\x01" * len(sm.binsha)
413+
414+
checkout = Path(sm.abspath).resolve()
415+
execute = Git.execute
416+
processes = []
417+
418+
def capture_process(self, command, *args, **kwargs):
419+
result = execute(self, command, *args, **kwargs)
420+
if kwargs.get("as_process") and "cat-file" in command:
421+
working_dir = Path(self.working_dir).resolve()
422+
if working_dir == checkout or checkout in working_dir.parents:
423+
processes.append(result.proc)
424+
return result
425+
426+
monkeypatch.setattr(Git, "execute", capture_process)
427+
try:
428+
if operation == "error":
429+
with pytest.raises(GitCommandError):
430+
sm.update(to_latest_revision=True, no_fetch=True)
431+
else:
432+
sm.update(to_latest_revision=True, no_fetch=True, recursive=recursive, keep_going=operation == "keep-going")
433+
# These records retain Head arguments and their internally opened Repos.
434+
# Collection cannot release the processes while the records remain alive.
435+
records = [record for record in caplog.records if "a tracking branch was not set" in record.msg]
436+
assert len(records) == (2 if recursive else 1)
437+
assert processes
438+
assert all(process.poll() is not None for process in processes)
439+
finally:
440+
for process in processes:
441+
if process.poll() is None:
442+
process.terminate()
443+
process.wait()
444+
445+
399446
@pytest.fixture
400447
def windows_directory_symlink_removal(monkeypatch):
401448
"""Exercise Windows rmdir semantics on POSIX, where rmdir rejects symlinks."""
@@ -1664,13 +1711,14 @@ def assert_exists(sm, value=True):
16641711
assert_exists(sm)
16651712

16661713
# Add additional submodule level.
1667-
csm = sm.module().create_submodule(
1668-
"nested-submodule",
1669-
join_path_native("nested-submodule", "working-tree"),
1670-
url=self._small_repo_url(),
1671-
)
1672-
sm.module().index.commit("added nested submodule")
1673-
sm_head_commit = sm.module().commit()
1714+
with sm.module() as module:
1715+
csm = module.create_submodule(
1716+
"nested-submodule",
1717+
join_path_native("nested-submodule", "working-tree"),
1718+
url=self._small_repo_url(),
1719+
)
1720+
module.index.commit("added nested submodule")
1721+
sm_head_commit = module.commit()
16741722
assert_exists(csm)
16751723

16761724
# Fails because there are new commits, compared to the remote we cloned from.
@@ -1700,6 +1748,7 @@ def assert_exists(sm, value=True):
17001748

17011749
# remove
17021750
sm_module_path = sm.module().git_dir
1751+
csm.repo.close()
17031752

17041753
for dry_run in (True, False):
17051754
sm.remove(dry_run=dry_run, force=True)

0 commit comments

Comments
 (0)