Skip to content

decode partial stdout/stderr in run_path TimeoutExpired branch #64

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
HrachShah wants to merge 1 commit into
base: main
Choose a base branch
from
+31 −1
Open

decode partial stdout/stderr in run_path TimeoutExpired branch #64

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 24 additions & 0 deletions python/tests/test_runner.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,30 @@ def test_exit_code_captured(self, tmp_path):
)
assert result.exit_code == 42

def test_timeout_returns_str_stdout_stderr(self, tmp_path):
"""When the subprocess exceeds the timeout, partial output is
returned as decoded str (matching the success path) rather than
the raw bytes that subprocess.TimeoutExpired carries by default.
"""
result = run_path(
[
sys.executable,
"-u",
"-c",
"import time; print('partial line'); time.sleep(5)",
],
use_stdin=False,
cwd=str(tmp_path),
timeout=1,
)
# exit_code is None on timeout (process was killed)
assert result.exit_code is None
# stdout/stderr must be str, not bytes, regardless of whether
# the subprocess had produced any output before being killed
assert isinstance(result.stdout, str)
assert isinstance(result.stderr, str)

@rchiodo rchiodo Jun 22, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📍 python/tests/test_runner.py:94
Review-rule violation (tests-no-partial-asserts). The subprocess output is deterministic (print('partial line') with -u yields exactly "partial line\n", and universal-newline translation normalizes any \r\n), and none of the rule's non-determinism exceptions (timestamps, random IDs, PIDs) apply. Replace assert "partial line" in result.stdout with assert result.stdout == "partial line\n" for an exact assertion.

[verified]

@rchiodo rchiodo Jul 1, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📍 python/tests/test_runner.py:92
Confirmed violation of review rule tests-no-partial-asserts.md: assert "partial line" in result.stdout is a banned substring check. The output is fully deterministic (print('partial line') always yields "partial line\n"), so an exact assertion is achievable and required: assert result.stdout == "partial line\n". Per the non-downgradeable rule gate this stays issue — the only way to waive it is to amend the rule doc, not this PR.

[verified]

assert "partial line" in result.stdout

@rchiodo rchiodo Jun 30, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📍 python/tests/test_runner.py:73
This test encodes the same false invariant as the fix: on Windows/newer CPython TimeoutExpired.stdout is already str, so the fixed code raises AttributeError before any assertion runs, making the test error (not pass) on Windows CI — the author's "18 passed" is POSIX-only. Make it platform/version-agnostic so it asserts the str contract on both the bytes-carrying (POSIX) and str-carrying (Windows) timeout behaviors, e.g. via monkeypatch/parametrize that forces e.stdout to each type.

[verified]


@rchiodo rchiodo Jun 22, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📍 python/tests/test_runner.py:73
This test only exercises the POSIX path and will itself AttributeError on Windows (where TimeoutExpired.stdout is already str), so a POSIX-only CI gives false confidence about the platform the current patch breaks. Add coverage that injects a fake TimeoutExpired carrying str output (Windows reality) in addition to bytes (POSIX), so both legs of the normalization are pinned. Separately, the genuinely subtle errors="replace" choice for a multibyte sequence truncated at the kill boundary has no test — consider emitting an incomplete multi-byte sequence before the kill to lock in that behavior.

[verified]

def test_with_env(self, tmp_path):
import os

Expand Down
8 changes: 7 additions & 1 deletion python/vscode_common_python_lsp/runner.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,13 @@ def run_path(
timeout=timeout,
)
except subprocess.TimeoutExpired as e:
return RunResult(e.stdout or "", e.stderr or "", None)
# subprocess.run with encoding='utf-8' returns decoded str on the
# normal completion path, but TimeoutExpired still carries the
# raw bytes captured from the pipe before decoding — produce
# str here so RunResult is uniformly typed.
partial_out = e.stdout.decode("utf-8", errors="replace") if e.stdout else ""

@rchiodo rchiodo Jun 22, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📍 python/vscode_common_python_lsp/runner.py:139
Critical, platform-asymmetric regression: this fix crashes on Windows. The Skeptic, Architect, and Advocate all verified by execution on Windows/CPython 3.14 that TimeoutExpired.stdout/.stderr are already str there — CPython's subprocess.run calls process.communicate() after kill on _mswindows, which decodes because encoding="utf-8" is set. So .decode(...) raises AttributeError: 'str' object has no attribute 'decode' whenever there is partial output (the exact case this PR targets). The if e.stdout guard only covers the empty case. Pre-fix Windows behavior was already correct str; this makes it worse. Decode defensively instead, e.g. def _as_str(v): return "" if not v else (v.decode("utf-8", errors="replace") if isinstance(v, bytes) else v) then RunResult(_as_str(e.stdout), _as_str(e.stderr), None). The POSIX half (errors="replace" for a multibyte char severed at the kill boundary) is sound and should be preserved.

[verified]

@rchiodo rchiodo Jun 22, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📍 python/vscode_common_python_lsp/runner.py:135
The comment says "same encoding that was passed to subprocess.run," but the decode hardcodes the "utf-8" literal independently from the encoding="utf-8" argument three lines up — the two would silently desync if either changes or if run_path ever gains an encoding parameter. Prefer deriving the decode encoding from a single source. Also consider applying the same normalization to the use_stdin=True Popen timeout branch so both timeout paths share one rule.

[verified]

partial_err = e.stderr.decode("utf-8", errors="replace") if e.stderr else ""
return RunResult(partial_out, partial_err, None)

@rchiodo rchiodo Jun 30, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📍 python/vscode_common_python_lsp/runner.py:136
The unconditional e.stdout.decode(...) assumes TimeoutExpired always carries bytes, but that is only true on POSIX. The Advocate, Skeptic, and Architect each independently verified (live, Python 3.14.3 / Windows) that subprocess.run(encoding="utf-8") populates TimeoutExpired.stdout as str on Windows, so this branch raises AttributeError: 'str' object has no attribute 'decode' — an uncaught crash thrown from inside run_path on every timeout, strictly worse than the bytes it replaced (which only blew up downstream and only for some callers). Make the conversion type-defensive, e.g. a helper def _as_text(v): return "" if not v else (v if isinstance(v, str) else v.decode("utf-8", errors="replace")) applied to both fields. The errors="replace" guard against a truncated multibyte sequence is correct and should be kept inside that helper.

[verified]

@rchiodo rchiodo Jun 30, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📍 python/vscode_common_python_lsp/runner.py:136
The Skeptic and Architect both observe the sibling use_stdin=True timeout path already yields uniform str by doing process.kill(); process.communicate() (which decodes in text mode). Mirroring that proven kill-then-communicate pattern here would have produced str on all platforms and avoided this bug entirely; worth considering instead of decoding the exception attribute, to keep the two branches consistent.

[verified]

return RunResult(result.stdout, result.stderr, result.returncode)


Expand Down