Skip to content

Commit 90fbfcd

Browse files
codexByron
authored andcommitted
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 087e322 commit 90fbfcd

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
@@ -1195,17 +1195,19 @@ def fetch_remotes(module_repo: "Repo") -> None:
11951195
)
11961196
# END update to new commit only if needed
11971197
except Exception as err:
1198+
if mrepo is not None:
1199+
mrepo.close()
11981200
if not keep_going:
11991201
raise
12001202
_logger.error(str(err))
12011203
# END handle keep_going
12021204

12031205
# HANDLE RECURSION
12041206
##################
1205-
if recursive:
1207+
try:
12061208
# In dry_run mode, the module might not exist.
1207-
if mrepo is not None:
1208-
for submodule in self.iter_items(self.module()):
1209+
if recursive and mrepo is not None:
1210+
for submodule in self.iter_items(mrepo):
12091211
submodule.update(
12101212
recursive,
12111213
init,
@@ -1218,7 +1220,11 @@ def fetch_remotes(module_repo: "Repo") -> None:
12181220
)
12191221
# END handle recursive update
12201222
# END handle dry run
1221-
# END for each submodule
1223+
finally:
1224+
# Log records and progress callbacks can retain refs to this repository.
1225+
# Its cat-file processes must not keep the checkout open on Windows.
1226+
if mrepo is not None:
1227+
mrepo.close()
12221228

12231229
return self
12241230

‎test/test_submodule.py‎

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

660660

661+
@pytest.mark.parametrize("operation", ["update", "error", "recursive", "keep-going"])
662+
def test_update_closes_checkout_processes_retained_by_logging(movable_submodule, monkeypatch, caplog, operation):
663+
sm = movable_submodule
664+
recursive = operation in ("recursive", "keep-going")
665+
with sm.module() as module:
666+
module.head.reference.set_tracking_branch(None)
667+
if recursive:
668+
child = module.create_submodule("child", "child", sm.url)
669+
module.index.commit("Add child")
670+
sm.binsha = module.head.commit.binsha
671+
with child.module() as nested:
672+
nested.head.reference.set_tracking_branch(None)
673+
if operation in ("error", "keep-going"):
674+
sm.binsha = b"\x01" * len(sm.binsha)
675+
676+
checkout = Path(sm.abspath).resolve()
677+
execute = Git.execute
678+
processes = []
679+
680+
def capture_process(self, command, *args, **kwargs):
681+
result = execute(self, command, *args, **kwargs)
682+
if kwargs.get("as_process") and "cat-file" in command:
683+
working_dir = Path(self.working_dir).resolve()
684+
if working_dir == checkout or checkout in working_dir.parents:
685+
processes.append(result.proc)
686+
return result
687+
688+
monkeypatch.setattr(Git, "execute", capture_process)
689+
try:
690+
if operation == "error":
691+
with pytest.raises(GitCommandError):
692+
sm.update(to_latest_revision=True, no_fetch=True)
693+
else:
694+
sm.update(to_latest_revision=True, no_fetch=True, recursive=recursive, keep_going=operation == "keep-going")
695+
# These records retain Head arguments and their internally opened Repos.
696+
# Collection cannot release the processes while the records remain alive.
697+
records = [record for record in caplog.records if "a tracking branch was not set" in record.msg]
698+
assert len(records) == (2 if recursive else 1)
699+
assert processes
700+
assert all(process.poll() is not None for process in processes)
701+
finally:
702+
for process in processes:
703+
if process.poll() is None:
704+
process.terminate()
705+
process.wait()
706+
707+
661708
@pytest.fixture
662709
def windows_directory_symlink_removal(monkeypatch):
663710
"""Exercise Windows rmdir semantics on POSIX, where rmdir rejects symlinks."""
@@ -1927,13 +1974,14 @@ def assert_exists(sm, value=True):
19271974
assert_exists(sm)
19281975

19291976
# Add additional submodule level.
1930-
csm = sm.module().create_submodule(
1931-
"nested-submodule",
1932-
join_path_native("nested-submodule", "working-tree"),
1933-
url=self._small_repo_url(),
1934-
)
1935-
sm.module().index.commit("added nested submodule")
1936-
sm_head_commit = sm.module().commit()
1977+
with sm.module() as module:
1978+
csm = module.create_submodule(
1979+
"nested-submodule",
1980+
join_path_native("nested-submodule", "working-tree"),
1981+
url=self._small_repo_url(),
1982+
)
1983+
module.index.commit("added nested submodule")
1984+
sm_head_commit = module.commit()
19371985
assert_exists(csm)
19381986

19391987
# Fails because there are new commits, compared to the remote we cloned from.
@@ -1963,6 +2011,7 @@ def assert_exists(sm, value=True):
19632011

19642012
# remove
19652013
sm_module_path = sm.module().git_dir
2014+
csm.repo.close()
19662015

19672016
for dry_run in (True, False):
19682017
sm.remove(dry_run=dry_run, force=True)

0 commit comments

Comments
 (0)