Skip to content

Commit 7627f66

Browse files
committed
fix(tui): cancel sibling stream reader when a git reader fails
The concurrent stdout/stderr drain in _run_git used asyncio.gather, which leaves the sibling _read_bounded coroutine running after the first reader raises — only the process was killed, so the other reader could keep draining a closing pipe. Move the concurrent read into _read_streams, which uses explicit tasks and cancels+awaits both readers on any failure (or cancellation) before propagating. Adds a regression test for a reader-exception path.
1 parent 42179a6 commit 7627f66

2 files changed

Lines changed: 61 additions & 4 deletions

File tree

src/pythinker_code/ui/shell/prompting/completion/workspace.py

Lines changed: 19 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -414,10 +414,7 @@ async def _run_git(self, root: HostPath, *args: str) -> str | None:
414414
# Drain stdout and stderr concurrently: reading them
415415
# sequentially can deadlock if git fills its stderr pipe buffer
416416
# (e.g. submodule warnings) while we are still draining stdout.
417-
stdout, _stderr = await asyncio.gather(
418-
self._read_bounded(process.stdout),
419-
self._read_bounded(process.stderr),
420-
)
417+
stdout = await self._read_streams(process)
421418
returncode = await process.wait()
422419
except asyncio.CancelledError:
423420
if process is not None:
@@ -444,6 +441,24 @@ async def _run_git(self, root: HostPath, *args: str) -> str | None:
444441
encoding = "utf-8"
445442
return stdout.decode(encoding=encoding, errors="replace")
446443

444+
async def _read_streams(self, process: HostProcess) -> bytes:
445+
"""Drain stdout and stderr concurrently and return stdout.
446+
447+
Uses explicit tasks so that if either reader raises (or this coroutine
448+
is cancelled), the sibling reader is cancelled and awaited rather than
449+
left running against a closing pipe.
450+
"""
451+
stdout_reader = asyncio.create_task(self._read_bounded(process.stdout))
452+
stderr_reader = asyncio.create_task(self._read_bounded(process.stderr))
453+
try:
454+
stdout, _stderr = await asyncio.gather(stdout_reader, stderr_reader)
455+
except BaseException:
456+
for reader in (stdout_reader, stderr_reader):
457+
reader.cancel()
458+
await asyncio.gather(stdout_reader, stderr_reader, return_exceptions=True)
459+
raise
460+
return stdout
461+
447462
@staticmethod
448463
async def _read_bounded(stream: AsyncReadable) -> bytes:
449464
chunks = bytearray()

tests/ui_and_conv/test_workspace_index.py

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -207,6 +207,48 @@ async def test_git_failure_uses_degraded_fallback() -> None:
207207
await lifecycle.aclose()
208208

209209

210+
@pytest.mark.asyncio
211+
async def test_read_streams_cancels_sibling_when_one_reader_fails() -> None:
212+
# When one stream reader raises, the sibling reader must be cancelled and
213+
# awaited rather than left draining a closing pipe.
214+
host = _FakeHost()
215+
index, lifecycle = _index(host, HostPath("/workspace"))
216+
217+
class _Blocking:
218+
def __init__(self) -> None:
219+
self.started = asyncio.Event()
220+
self.cancelled = False
221+
222+
async def read(self, _n: int = -1) -> bytes:
223+
self.started.set()
224+
try:
225+
await asyncio.Event().wait()
226+
except asyncio.CancelledError:
227+
self.cancelled = True
228+
raise
229+
return b""
230+
231+
class _Raising:
232+
def __init__(self, gate: asyncio.Event) -> None:
233+
self._gate = gate
234+
235+
async def read(self, _n: int = -1) -> bytes:
236+
await self._gate.wait() # ensure the sibling is mid-read first
237+
raise RuntimeError("stdout reader boom")
238+
239+
blocking = _Blocking()
240+
241+
class _Proc:
242+
stdout = _Raising(blocking.started)
243+
stderr = blocking
244+
245+
with pytest.raises(RuntimeError, match="boom"):
246+
await index._read_streams(cast(Any, _Proc()))
247+
248+
assert blocking.cancelled is True
249+
await lifecycle.aclose()
250+
251+
210252
@pytest.mark.asyncio
211253
async def test_results_are_truncated_at_one_thousand_entries() -> None:
212254
host = _FakeHost()

0 commit comments

Comments
 (0)