fix: spawn-test stderr persistence and drain-cap deadlock (PR #11 review)
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 <noreply@anthropic.com>
This commit is contained in:
@@ -321,6 +321,7 @@ class ServerEditor(QFrame):
|
|||||||
self._before_change = before_change # called before any row add/remove in tables
|
self._before_change = before_change # called before any row add/remove in tables
|
||||||
self._extra = {} # preserve unknown server fields verbatim
|
self._extra = {} # preserve unknown server fields verbatim
|
||||||
self._loading = False
|
self._loading = False
|
||||||
|
self._last_spawn: dict | None = None # most recent spawn_test result
|
||||||
|
|
||||||
outer = QVBoxLayout(self)
|
outer = QVBoxLayout(self)
|
||||||
outer.setContentsMargins(16, 16, 16, 16)
|
outer.setContentsMargins(16, 16, 16, 16)
|
||||||
@@ -488,6 +489,7 @@ class ServerEditor(QFrame):
|
|||||||
self.headers.load({})
|
self.headers.load({})
|
||||||
self.details_btn.setChecked(False)
|
self.details_btn.setChecked(False)
|
||||||
self.diag_text.clear()
|
self.diag_text.clear()
|
||||||
|
self._last_spawn = None
|
||||||
self._set_dep({"status": "unknown", "label": "—"})
|
self._set_dep({"status": "unknown", "label": "—"})
|
||||||
self.args_warn.hide()
|
self.args_warn.hide()
|
||||||
self.args_fix_btn.hide()
|
self.args_fix_btn.hide()
|
||||||
@@ -610,7 +612,7 @@ class ServerEditor(QFrame):
|
|||||||
if auto_open and problem and not self.details_btn.isChecked():
|
if auto_open and problem and not self.details_btn.isChecked():
|
||||||
self.details_btn.setChecked(True) # opens panel (fills text via _toggle_diag)
|
self.details_btn.setChecked(True) # opens panel (fills text via _toggle_diag)
|
||||||
if self.diag_card.isVisible():
|
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):
|
def _set_dep(self, res: dict):
|
||||||
status = res.get("status", "unknown")
|
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
|
color = STATUS_COLORS.get(status, MUTED) if status in ("missing", "warn") else MUTED
|
||||||
self.dep_label.setStyleSheet(f"color: {color};")
|
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):
|
def _toggle_diag(self, on):
|
||||||
self.diag_card.setVisible(on)
|
self.diag_card.setVisible(on)
|
||||||
self.details_btn.setText("Details ▾" if on else "Details ▸")
|
self.details_btn.setText("Details ▾" if on else "Details ▸")
|
||||||
if on and self.isEnabled():
|
if on and self.isEnabled():
|
||||||
self.diag_text.setPlainText(
|
self.diag_text.setPlainText(self._full_diag_text())
|
||||||
core.diagnostics_text(self.current_name(), self.dump_data())
|
|
||||||
)
|
|
||||||
|
|
||||||
def _recheck(self):
|
def _recheck(self):
|
||||||
core.refresh_path_cache() # re-scan PATH, e.g. after installing a runtime
|
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):
|
def _on_spawn_done(self, result: dict):
|
||||||
self.spawn_btn.setEnabled(True)
|
self.spawn_btn.setEnabled(True)
|
||||||
self.spawn_btn.setText("Test launch")
|
self.spawn_btn.setText("Test launch")
|
||||||
|
self._last_spawn = result
|
||||||
outcome = result.get("outcome", "")
|
outcome = result.get("outcome", "")
|
||||||
detail = result.get("detail", "")
|
detail = result.get("detail", "")
|
||||||
stderr = result.get("stderr", "").strip()
|
|
||||||
if outcome == "ok":
|
if outcome == "ok":
|
||||||
color = GOOD
|
color = GOOD
|
||||||
label = f"started · {detail}"
|
label = f"started · {detail}"
|
||||||
@@ -700,9 +706,11 @@ class ServerEditor(QFrame):
|
|||||||
self.dep_dot.setStyleSheet(f"color: {color}; font-size: 14px;")
|
self.dep_dot.setStyleSheet(f"color: {color}; font-size: 14px;")
|
||||||
self.dep_label.setText(label)
|
self.dep_label.setText(label)
|
||||||
self.dep_label.setStyleSheet(f"color: {color};")
|
self.dep_label.setStyleSheet(f"color: {color};")
|
||||||
if stderr and self.diag_card.isVisible():
|
# Auto-open the details panel on non-ok outcomes so stderr is visible.
|
||||||
current = self.diag_text.toPlainText()
|
if outcome != "ok" and not self.details_btn.isChecked():
|
||||||
self.diag_text.setPlainText(current + f"\n\n── stderr from Test launch ──\n{stderr}")
|
self.details_btn.setChecked(True) # triggers _toggle_diag → fills diag_text
|
||||||
|
elif self.diag_card.isVisible():
|
||||||
|
self.diag_text.setPlainText(self._full_diag_text())
|
||||||
|
|
||||||
|
|
||||||
# --------------------------------------------------------------------------- #
|
# --------------------------------------------------------------------------- #
|
||||||
|
|||||||
+8
-2
@@ -1114,12 +1114,18 @@ def spawn_test(data: dict, timeout: float = 3.0) -> dict:
|
|||||||
stderr_chunks: list[bytes] = []
|
stderr_chunks: list[bytes] = []
|
||||||
|
|
||||||
def _drain(pipe) -> None:
|
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:
|
try:
|
||||||
while sum(len(c) for c in stderr_chunks) < _STDERR_CAP:
|
while True:
|
||||||
chunk = pipe.read(1024)
|
chunk = pipe.read(1024)
|
||||||
if not chunk:
|
if not chunk:
|
||||||
break
|
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:
|
except Exception:
|
||||||
pass
|
pass
|
||||||
|
|
||||||
|
|||||||
@@ -303,3 +303,18 @@ def test_spawn_test_stdin_not_closed():
|
|||||||
timeout=0.3,
|
timeout=0.3,
|
||||||
)
|
)
|
||||||
assert r["outcome"] == "ok", f"expected ok (blocking on stdin), got {r['outcome']}"
|
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
|
||||||
|
|||||||
Reference in New Issue
Block a user