From 231403c1d5443beef2a339aa117f69a37d94e88d Mon Sep 17 00:00:00 2001 From: AJ Date: Thu, 2 Jul 2026 01:05:34 -0400 Subject: [PATCH] fix: spawn-test stderr persistence and drain-cap deadlock (PR #11 review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two bugs caught in supervisor review: 1. Stderr was silently dropped when the Details panel was closed during a crash. _on_spawn_done appended to diag_text only when the panel was already open, and refresh_dependency() clobbered that text on the next field change anyway. Fix: stash the result in self._last_spawn; _full_diag_text() appends the stderr section whenever diag text is generated; _on_spawn_done auto-opens the panel on non-ok outcomes (same pattern as the existing auto_open for missing commands). 2. _drain stopped reading once _STDERR_CAP (4 KB) was reached. A process that writes more than 4 KB then blocked on a full pipe buffer, never exited, and was misclassified as "ok" instead of "crashed". Fix: drain to EOF unconditionally; keep only the first _STDERR_CAP bytes. Regression test: 64 KB stderr + exit(3) → outcome "crashed". Co-Authored-By: Claude Sonnet 4.6 --- bcc.py | 24 ++++++++++++++++-------- bcc_core.py | 10 ++++++++-- tests/test_core.py | 15 +++++++++++++++ 3 files changed, 39 insertions(+), 10 deletions(-) diff --git a/bcc.py b/bcc.py index 3d46b15..187767d 100644 --- a/bcc.py +++ b/bcc.py @@ -321,6 +321,7 @@ class ServerEditor(QFrame): self._before_change = before_change # called before any row add/remove in tables self._extra = {} # preserve unknown server fields verbatim self._loading = False + self._last_spawn: dict | None = None # most recent spawn_test result outer = QVBoxLayout(self) outer.setContentsMargins(16, 16, 16, 16) @@ -488,6 +489,7 @@ class ServerEditor(QFrame): self.headers.load({}) self.details_btn.setChecked(False) self.diag_text.clear() + self._last_spawn = None self._set_dep({"status": "unknown", "label": "—"}) self.args_warn.hide() self.args_fix_btn.hide() @@ -610,7 +612,7 @@ class ServerEditor(QFrame): if auto_open and problem and not self.details_btn.isChecked(): self.details_btn.setChecked(True) # opens panel (fills text via _toggle_diag) if self.diag_card.isVisible(): - self.diag_text.setPlainText(core.diagnostics_text(self.current_name(), data)) + self.diag_text.setPlainText(self._full_diag_text()) def _set_dep(self, res: dict): status = res.get("status", "unknown") @@ -620,13 +622,17 @@ class ServerEditor(QFrame): color = STATUS_COLORS.get(status, MUTED) if status in ("missing", "warn") else MUTED self.dep_label.setStyleSheet(f"color: {color};") + def _full_diag_text(self) -> str: + text = core.diagnostics_text(self.current_name(), self.dump_data()) + if self._last_spawn and self._last_spawn.get("stderr", "").strip(): + text += "\n\n── stderr from last Test launch ──\n" + self._last_spawn["stderr"].strip() + return text + def _toggle_diag(self, on): self.diag_card.setVisible(on) self.details_btn.setText("Details ▾" if on else "Details ▸") if on and self.isEnabled(): - self.diag_text.setPlainText( - core.diagnostics_text(self.current_name(), self.dump_data()) - ) + self.diag_text.setPlainText(self._full_diag_text()) def _recheck(self): core.refresh_path_cache() # re-scan PATH, e.g. after installing a runtime @@ -684,9 +690,9 @@ class ServerEditor(QFrame): def _on_spawn_done(self, result: dict): self.spawn_btn.setEnabled(True) self.spawn_btn.setText("Test launch") + self._last_spawn = result outcome = result.get("outcome", "") detail = result.get("detail", "") - stderr = result.get("stderr", "").strip() if outcome == "ok": color = GOOD label = f"started · {detail}" @@ -700,9 +706,11 @@ class ServerEditor(QFrame): self.dep_dot.setStyleSheet(f"color: {color}; font-size: 14px;") self.dep_label.setText(label) self.dep_label.setStyleSheet(f"color: {color};") - if stderr and self.diag_card.isVisible(): - current = self.diag_text.toPlainText() - self.diag_text.setPlainText(current + f"\n\n── stderr from Test launch ──\n{stderr}") + # Auto-open the details panel on non-ok outcomes so stderr is visible. + if outcome != "ok" and not self.details_btn.isChecked(): + self.details_btn.setChecked(True) # triggers _toggle_diag → fills diag_text + elif self.diag_card.isVisible(): + self.diag_text.setPlainText(self._full_diag_text()) # --------------------------------------------------------------------------- # diff --git a/bcc_core.py b/bcc_core.py index 218a6d0..927681b 100644 --- a/bcc_core.py +++ b/bcc_core.py @@ -1114,12 +1114,18 @@ def spawn_test(data: dict, timeout: float = 3.0) -> dict: stderr_chunks: list[bytes] = [] def _drain(pipe) -> None: + # Read until EOF so the pipe buffer never fills and blocks the subprocess. + # Only keep the first _STDERR_CAP bytes; the rest is discarded. + captured = 0 try: - while sum(len(c) for c in stderr_chunks) < _STDERR_CAP: + while True: chunk = pipe.read(1024) if not chunk: break - stderr_chunks.append(chunk) + if captured < _STDERR_CAP: + keep = min(len(chunk), _STDERR_CAP - captured) + stderr_chunks.append(chunk[:keep]) + captured += keep except Exception: pass diff --git a/tests/test_core.py b/tests/test_core.py index 7d15a58..253f1fd 100644 --- a/tests/test_core.py +++ b/tests/test_core.py @@ -303,3 +303,18 @@ def test_spawn_test_stdin_not_closed(): timeout=0.3, ) assert r["outcome"] == "ok", f"expected ok (blocking on stdin), got {r['outcome']}" + + +def test_spawn_test_large_stderr_does_not_deadlock(): + # A process that writes >4 KB (pipe buffer territory) to stderr then exits non-zero. + # Without EOF-draining, the subprocess blocks on a full pipe and is wrongly + # classified as "ok" (still running) instead of "crashed". + script = "import sys; sys.stderr.write('x' * 65536); sys.stderr.flush(); sys.exit(3)" + r = c.spawn_test( + {"command": sys.executable, "args": ["-c", script]}, + timeout=2.0, + ) + assert r["outcome"] == "crashed", f"expected crashed, got {r['outcome']}" + assert r["returncode"] == 3 + # stderr is capped, not the full 64 KB + assert len(r["stderr"]) <= c._STDERR_CAP