Skip to content

Commit aacce32

Browse files
Zsailerclaude
Zsailer
and
claude
committed
Add comprehensive timeout handling and process cleanup improvements
- Extract subprocess functions to module level with proper timeout handling - Add timeout parameter to call_subprocess_with_authentication - Implement graceful termination (terminate → wait → kill) for subprocess timeouts - Add comprehensive test suite for timeout scenarios and process cleanup - Improve credential daemon cleanup with proper timeout handling - Ensure proper logging with lazy evaluation for all timeout scenarios 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
1 parent 5039311 commit aacce32

File tree

2 files changed

+463
-90
lines changed

2 files changed

+463
-90
lines changed

‎jupyterlab_git/git.py‎

Lines changed: 106 additions & 90 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010
import shlex
1111
import shutil
1212
import subprocess
13+
import time
1314
import traceback
1415
from enum import Enum, IntEnum
1516
from pathlib import Path
@@ -55,6 +56,104 @@
5556
execution_lock = tornado.locks.Lock()
5657

5758

59+
async def call_subprocess_with_authentication(
60+
cmdline: "List[str]",
61+
username: "str",
62+
password: "str",
63+
cwd: "Optional[str]" = None,
64+
env: "Optional[Dict[str, str]]" = None,
65+
timeout: "float" = 20,
66+
) -> "Tuple[int, str, str]":
67+
"""Call subprocess with authentication handling."""
68+
try:
69+
p = pexpect.spawn(
70+
cmdline[0],
71+
cmdline[1:],
72+
cwd=cwd,
73+
env=env,
74+
encoding="utf-8",
75+
timeout=timeout,
76+
)
77+
78+
# We expect a prompt from git
79+
# In most of cases git will prompt for username and
80+
# then for password
81+
# In some cases (Bitbucket) username is included in
82+
# remote URL, so git will not ask for username
83+
i = await p.expect(["Username for .*: ", "Password for .*:"], async_=True)
84+
if i == 0: # ask for username then password
85+
p.sendline(username)
86+
await p.expect("Password for .*:", async_=True)
87+
p.sendline(password)
88+
elif i == 1: # only ask for password
89+
p.sendline(password)
90+
91+
await p.expect(pexpect.EOF, async_=True)
92+
response = p.before
93+
94+
returncode = p.wait()
95+
p.close()
96+
return returncode, "", response
97+
except pexpect.exceptions.EOF: # In case of pexpect failure
98+
response = p.before
99+
returncode = p.exitstatus
100+
p.close() # close process
101+
return returncode, "", response
102+
except pexpect.exceptions.TIMEOUT: # Handle timeout
103+
p.terminate(force=True)
104+
p.close()
105+
raise TimeoutError(
106+
f"Git authentication timed out after {timeout} seconds: {' '.join(cmdline)}"
107+
)
108+
except Exception as e:
109+
# Ensure process is always closed on any exception
110+
if p and p.isalive():
111+
p.terminate(force=True)
112+
if p:
113+
p.close()
114+
raise e
115+
116+
117+
def call_subprocess(
118+
cmdline: "List[str]",
119+
cwd: "Optional[str]" = None,
120+
env: "Optional[Dict[str, str]]" = None,
121+
timeout: "float" = 20,
122+
is_binary=False,
123+
) -> "Tuple[int, str, str]":
124+
"""Call subprocess with timeout and process cleanup."""
125+
process = subprocess.Popen(
126+
cmdline, stdout=subprocess.PIPE, stderr=subprocess.PIPE, cwd=cwd, env=env
127+
)
128+
try:
129+
output, error = process.communicate(timeout=timeout)
130+
except subprocess.TimeoutExpired:
131+
# Terminate the process gracefully
132+
process.terminate()
133+
try:
134+
process.wait(timeout=5)
135+
except subprocess.TimeoutExpired:
136+
# Force kill if still alive after 5 seconds
137+
process.kill()
138+
process.wait()
139+
get_logger().warning("Git command timed out: %s", cmdline)
140+
raise TimeoutError(
141+
f"Git command timed out after {timeout} seconds: {' '.join(cmdline)}"
142+
)
143+
except OSError as e:
144+
get_logger().warning("Failed to execute git command: %s", cmdline)
145+
raise
146+
147+
if is_binary:
148+
return (
149+
process.returncode,
150+
base64.encodebytes(output).decode("ascii"),
151+
error.decode("utf-8"),
152+
)
153+
else:
154+
return (process.returncode, output.decode("utf-8"), error.decode("utf-8"))
155+
156+
58157
class State(IntEnum):
59158
"""Git repository state."""
60159

@@ -99,94 +198,6 @@ async def execute(
99198
(int, str, str): (return code, stdout, stderr)
100199
"""
101200

102-
async def call_subprocess_with_authentication(
103-
cmdline: "List[str]",
104-
username: "str",
105-
password: "str",
106-
cwd: "Optional[str]" = None,
107-
env: "Optional[Dict[str, str]]" = None,
108-
) -> "Tuple[int, str, str]":
109-
try:
110-
p = pexpect.spawn(
111-
cmdline[0],
112-
cmdline[1:],
113-
cwd=cwd,
114-
env=env,
115-
encoding="utf-8",
116-
timeout=timeout,
117-
)
118-
119-
# We expect a prompt from git
120-
# In most of cases git will prompt for username and
121-
# then for password
122-
# In some cases (Bitbucket) username is included in
123-
# remote URL, so git will not ask for username
124-
i = await p.expect(["Username for .*: ", "Password for .*:"], async_=True)
125-
if i == 0: # ask for username then password
126-
p.sendline(username)
127-
await p.expect("Password for .*:", async_=True)
128-
p.sendline(password)
129-
elif i == 1: # only ask for password
130-
p.sendline(password)
131-
132-
await p.expect(pexpect.EOF, async_=True)
133-
response = p.before
134-
135-
returncode = p.wait()
136-
p.close()
137-
return returncode, "", response
138-
except pexpect.exceptions.EOF: # In case of pexpect failure
139-
response = p.before
140-
returncode = p.exitstatus
141-
p.close() # close process
142-
return returncode, "", response
143-
except pexpect.exceptions.TIMEOUT: # Handle timeout
144-
p.terminate(force=True)
145-
p.close()
146-
raise TimeoutError(
147-
f"Git authentication timed out after {timeout} seconds: {' '.join(cmdline)}"
148-
)
149-
except Exception as e:
150-
# Ensure process is always closed on any exception
151-
if p and p.isalive():
152-
p.terminate(force=True)
153-
if p:
154-
p.close()
155-
raise e
156-
157-
def call_subprocess(
158-
cmdline: "List[str]",
159-
cwd: "Optional[str]" = None,
160-
env: "Optional[Dict[str, str]]" = None,
161-
is_binary=is_binary,
162-
) -> "Tuple[int, str, str]":
163-
process = subprocess.Popen(
164-
cmdline, stdout=subprocess.PIPE, stderr=subprocess.PIPE, cwd=cwd, env=env
165-
)
166-
try:
167-
output, error = process.communicate(timeout=timeout)
168-
except subprocess.TimeoutExpired:
169-
# Terminate the process gracefully
170-
process.terminate()
171-
try:
172-
process.wait(timeout=5)
173-
except subprocess.TimeoutExpired:
174-
# Force kill if still alive after 5 seconds
175-
process.kill()
176-
process.wait()
177-
raise TimeoutError(
178-
f"Git command timed out after {timeout} seconds: {' '.join(cmdline)}"
179-
)
180-
181-
if is_binary:
182-
return (
183-
process.returncode,
184-
base64.encodebytes(output).decode("ascii"),
185-
error.decode("utf-8"),
186-
)
187-
else:
188-
return (process.returncode, output.decode("utf-8"), error.decode("utf-8"))
189-
190201
try:
191202
await execution_lock.acquire(timeout=datetime.timedelta(seconds=timeout))
192203
except tornado.util.TimeoutError:
@@ -210,11 +221,12 @@ def call_subprocess(
210221
password,
211222
cwd,
212223
env,
224+
timeout,
213225
)
214226
else:
215227
current_loop = tornado.ioloop.IOLoop.current()
216228
code, output, error = await current_loop.run_in_executor(
217-
None, call_subprocess, cmdline, cwd, env
229+
None, call_subprocess, cmdline, cwd, env, timeout, is_binary
218230
)
219231
log_output = (
220232
output[:MAX_LOG_OUTPUT] + "..." if len(output) > MAX_LOG_OUTPUT else output
@@ -271,7 +283,11 @@ def _cleanup_processes(self):
271283
self._GIT_CREDENTIAL_CACHE_DAEMON_PROCESS.wait(timeout=5)
272284
except subprocess.TimeoutExpired:
273285
self._GIT_CREDENTIAL_CACHE_DAEMON_PROCESS.kill()
274-
self._GIT_CREDENTIAL_CACHE_DAEMON_PROCESS.wait()
286+
try:
287+
self._GIT_CREDENTIAL_CACHE_DAEMON_PROCESS.wait(timeout=1)
288+
except subprocess.TimeoutExpired:
289+
# If kill() doesn't work within 1 second, just give up
290+
pass
275291
get_logger().debug("Git credential cache daemon process (PID: %s) cleaned up successfully", self._GIT_CREDENTIAL_CACHE_DAEMON_PROCESS.pid)
276292
except Exception as e:
277293
get_logger().warning("Failed to cleanup credential cache daemon: %s", e)

0 commit comments

Comments
 (0)