Skip to content

fix: guard AutoInterrupt terminate during interpreter shutdown #2105

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
EliahKagan merged 2 commits into from
Mar 9, 2026
Merged

fix: guard AutoInterrupt terminate during interpreter shutdown #2105

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3cfd229
fix: ignore AutoInterrupt terminate errors during shutdown
lweyrich1 Feb 25, 2026
357aad1
Remove unnecessary `pytest` import in a test module
EliahKagan Mar 9, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions git/cmd.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -368,8 +368,12 @@ def _terminate(self) -> None:
status = proc.wait() # Ensure the process goes away.

self.status = self._status_code_if_terminate or status
except OSError as ex:
_logger.info("Ignored error after process had died: %r", ex)
except (OSError, AttributeError) as ex:
# On interpreter shutdown (notably on Windows), parts of the stdlib used by
# subprocess can already be torn down (e.g. `subprocess._winapi` becomes None),
# which can cause AttributeError during terminate(). In that case, we prefer
# to silently ignore to avoid noisy "Exception ignored in: __del__" messages.
Copy link

Copilot AI Mar 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Catching AttributeError unconditionally here can mask real runtime failures when _terminate() is called outside interpreter shutdown (e.g. the kill_after_timeout path in handle_process_output). Consider only suppressing AttributeError during interpreter finalization (e.g. getattr(sys, "is_finalizing", lambda: False)()), and otherwise re-raising so unexpected terminate/wait breakages don’t silently leave processes running. Also, the comment says “silently ignore” but this logs at INFO—either adjust the wording or log level to match intent.

Suggested change
# to silently ignore to avoid noisy "Exception ignored in: __del__" messages.
# to log and ignore to avoid noisy "Exception ignored in: __del__" messages.
if isinstance(ex, AttributeError) and not getattr(sys, "is_finalizing", lambda: False)():
# Outside interpreter finalization, an AttributeError here likely indicates
# a real problem (e.g. broken subprocess internals), so re-raise it instead
# of silently leaving the process running.
raise

Copilot uses AI. Check for mistakes.
Copy link
Member

@EliahKagan EliahKagan Mar 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There may be some merit to this concern, but I don't think it needs to block the change here, since AttributeError really should not arise here in any other way. I'm also not entirely sure that a change along the lines of what the model suggests here would be robust enough to be worth doing. But probably some future change to make this more robust, or do log in more detail when the capability to do so is present, or both, could be worthwhile. (I reiterate that I don't consider this blocking.)

_logger.info("Ignored error while terminating process: %r", ex)
# END exception handling

def __del__(self) -> None:
Expand Down
33 changes: 33 additions & 0 deletions test/test_autointerrupt.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
from git.cmd import Git


class _DummyProc:
"""Minimal stand-in for subprocess.Popen used to exercise AutoInterrupt.

We deliberately raise AttributeError from terminate() to simulate interpreter
shutdown on Windows where subprocess internals (e.g. subprocess._winapi) may
already be torn down.
"""

stdin = None
stdout = None
stderr = None

def poll(self):
return None

def terminate(self):
raise AttributeError("TerminateProcess")

def wait(self): # pragma: no cover - should not be reached in this test
raise AssertionError("wait() should not be called if terminate() fails")


def test_autointerrupt_terminate_ignores_attributeerror():
ai = Git.AutoInterrupt(_DummyProc(), args=["git", "rev-list"])

# Should not raise, even if terminate() triggers AttributeError.
ai._terminate()

# Ensure the reference is cleared to avoid repeated attempts.
assert ai.proc is None
Loading