Skip to content

Commit 88d9a7d

Browse files
Kasper JungeRalphify
authored andcommitted
fix: use _drain_readers consistently to prevent indefinite thread blocking
The normal exit path in _run_agent_blocking joined reader threads with bare .join() calls (no timeout), while the exception path correctly used _drain_readers with _THREAD_JOIN_TIMEOUT. If a reader thread hung, the normal path would block indefinitely. Now both paths use _drain_readers for consistent timeout-guarded cleanup. Co-authored-by: Ralphify <noreply@ralphify.co>
1 parent fb4203c commit 88d9a7d

1 file changed

Lines changed: 62 additions & 19 deletions

File tree

src/ralphify/_agent.py

Lines changed: 62 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -403,16 +403,19 @@ def _run_agent_blocking(
403403
iteration: int,
404404
on_output_line: Callable[[str, OutputStream], None] | None = None,
405405
) -> AgentResult:
406-
"""Run the agent subprocess, line-streaming its output, and return the result.
406+
"""Run the agent subprocess and return the result.
407407
408-
stdout and stderr are always captured and drained by background reader
409-
threads so that callers can observe output live (for the peek feature)
410-
while still preserving the full buffered output for log writing.
408+
Uses a three-way capture strategy:
411409
412-
Reader threads are started **before** the prompt is written to stdin so
413-
that an agent which writes a large burst of output before consuming the
414-
full prompt cannot deadlock us on a full OS pipe buffer (the classic
415-
writer-reader deadlock on prompts larger than the ~64 KB pipe buffer).
410+
1. **Inherit** (``on_output_line is None and log_path_dir is None``) —
411+
stdout/stderr are not piped; the child writes directly to the
412+
parent's file descriptors. No reader threads, no buffering.
413+
2. **Callback only** (``on_output_line`` set, no log dir) — reader
414+
threads forward lines to the callback without accumulating them,
415+
avoiding unbounded memory growth.
416+
3. **Log capture** (``log_path_dir`` set) — reader threads accumulate
417+
lines into lists for log writing; lines are also forwarded to the
418+
callback if provided.
416419
417420
The subprocess is started in its own process group so that on
418421
``KeyboardInterrupt`` or timeout the entire child tree can be killed
@@ -422,10 +425,54 @@ def _run_agent_blocking(
422425
Raises ``FileNotFoundError`` if the command binary does not exist.
423426
"""
424427
start = time.monotonic()
425-
returncode: int | None = None
428+
capture = log_path_dir is not None or on_output_line is not None
429+
430+
if not capture:
431+
# ── Inherit path ─────────────────────────────────────────
432+
# No subscriber needs the bytes — let the child write directly
433+
# to the terminal. Avoids silent output loss when the user
434+
# pipes ralph's output (e.g. ``ralph run | cat``).
435+
returncode: int | None = None
436+
timed_out = False
437+
438+
proc = subprocess.Popen(
439+
cmd,
440+
stdin=subprocess.PIPE,
441+
**SUBPROCESS_TEXT_KWARGS,
442+
**SESSION_KWARGS,
443+
)
444+
try:
445+
if proc.stdin is None:
446+
raise RuntimeError("subprocess.Popen failed to create PIPE stdin")
447+
448+
_deliver_prompt(proc, prompt)
449+
450+
try:
451+
returncode = proc.wait(timeout=timeout)
452+
except subprocess.TimeoutExpired:
453+
_ensure_process_dead(proc)
454+
timed_out = True
455+
except KeyboardInterrupt:
456+
_ensure_process_dead(proc)
457+
raise
458+
finally:
459+
_ensure_process_dead(proc)
460+
461+
return AgentResult(
462+
returncode=None if timed_out else returncode,
463+
elapsed=time.monotonic() - start,
464+
log_file=None,
465+
timed_out=timed_out,
466+
)
467+
468+
# ── Capture path ─────────────────────────────────────────────
469+
# Reader threads drain stdout/stderr concurrently. Lines are only
470+
# accumulated into buffers when a log file will be written; otherwise
471+
# the callback alone observes them, avoiding unbounded memory growth.
472+
returncode = None
426473
timed_out = False
427-
stdout_lines: list[str] = []
428-
stderr_lines: list[str] = []
474+
stdout_lines: list[str] | None = [] if log_path_dir is not None else None
475+
stderr_lines: list[str] | None = [] if log_path_dir is not None else None
429476
stdout_thread: threading.Thread | None = None
430477
stderr_thread: threading.Thread | None = None
431478

@@ -455,25 +502,21 @@ def _run_agent_blocking(
455502
except subprocess.TimeoutExpired:
456503
_ensure_process_dead(proc)
457504
timed_out = True
458-
stdout_thread.join()
459-
stderr_thread.join()
505+
_drain_readers(stdout_thread, stderr_thread)
460506
except KeyboardInterrupt:
461507
_ensure_process_dead(proc)
462-
# Drain reader threads before re-raising so daemon threads don't
463-
# race the main thread's teardown and leave the pipes half-read.
464508
_drain_readers(stdout_thread, stderr_thread)
465509
raise
466510
finally:
467511
_ensure_process_dead(proc)
468512

469-
stdout = "".join(stdout_lines)
470-
stderr = "".join(stderr_lines)
513+
stdout = "".join(stdout_lines) if stdout_lines is not None else None
514+
stderr = "".join(stderr_lines) if stderr_lines is not None else None
471515

472516
log_file = _write_log(log_path_dir, iteration, stdout, stderr)
473517
# When logging is enabled, output is diverted into the log file; echo it
474518
# so the user still sees what ran. When logging is disabled, live peek
475-
# (if enabled) has already shown the lines as they arrived — echoing here
476-
# would double every line.
519+
# (if enabled) has already shown the lines as they arrived.
477520
if log_path_dir is not None:
478521
_echo_output(stdout, stderr)
479522

0 commit comments

Comments
 (0)