fix: decode subprocess output with errors="replace" to keep the reader thread alive

[takumi-koshii]

Fixes: #274

What was the problem/requirement? (What/Why)

LoggingSubprocess decodes the child process's stdout/stderr with subprocess.Popen(..., encoding="utf-8") and no errors argument, i.e. the strict error handler.

When a subprocess writes a byte that is not valid UTF-8 (common for DCC batch renderers that emit locale-encoded text, e.g. cp1252 on Windows), StreamLogger.run's readline() raises UnicodeDecodeError. Since UnicodeDecodeError is a subclass of ValueError and StreamLogger.run re-raises any ValueError other than "I/O operation on closed file", the stdout reader thread (AdaptorRuntimeStdoutLogger) dies. After that the stdout pipe is no longer drained, the subprocess blocks on a full pipe, and the job hangs indefinitely. The failure is silent, which makes it hard to diagnose.

This was observed in production with the AWS Deadline Cloud 3ds Max adaptor: 3dsmaxbatch.exe emits byte 0xc7 on stdout and rendering hangs partway through.

What was the solution? (How)

Pass errors="replace" to the Popen call in _logging_subprocess.py so subprocess output is decoded leniently. Undecodable bytes are replaced with the Unicode replacement character instead of raising, so the reader thread stays alive, the pipe keeps being drained, and no log lines are dropped.

The fix is intentionally minimal and focused on the decode path. Making StreamLogger.run itself more resilient to unexpected read/log errors is discussed in the linked issue and left out of this PR to keep it focused.

What is the impact of this change?

Jobs whose subprocesses emit non-UTF-8 bytes on stdout/stderr no longer risk a silent reader-thread death and deadlock. Log output is preserved, with any undecodable byte shown as the replacement character.

How was this change tested?

• Added a regression integration test (test_non_utf8_output_does_not_kill_reader) that runs a subprocess emitting byte 0xc7 on stdout and asserts that lines after the undecodable byte are still logged (i.e. the reader thread survived). Verified that this test fails on the unmodified code and passes with the fix.

• Updated the existing Popen argument assertions in the unit tests to include errors="replace".

• Ran the unit and integration tests for the process package locally (41 passed, 1 skipped — the skipped test is Windows-only).

• Have you run the unit tests? Yes.

Was this change documented?

• Are relevant docstrings in the code base updated? A code comment explaining why lenient decoding is required was added at the Popen call site. No public docstrings needed changes.

Is this a breaking change?

No. This does not change any public interface. The only behavioral change is that previously-undecodable bytes are now replaced instead of killing the reader thread.

Does this change impact security?

No. It does not create or modify files/directories or change any trust boundary.

---

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

[leongdl]

Thank you for contribution - we are reviewing this and PR and hope to land it as soon as possible.

[leongdl]

Review

Summary

Tiny, well-aimed fix. LoggingSubprocess spawns adaptor child processes with Popen(..., encoding="utf-8") in strict mode; a single non-UTF-8 byte on stdout raises UnicodeDecodeError inside the reader thread, which dies, the pipe stops being drained, and the child deadlocks on a full pipe — silently. The PR adds errors="replace" so undecodable bytes become U+FFFD instead of killing the thread. One source line, a good regression integration test, and three updated Popen-assertion unit tests.

Call-stack trace

Flow: LoggingSubprocess(args=[...]) → child writes 0xc7 to stdout
  1. _logging_subprocess.py  __init__ builds popen_params
                             (stdin/stdout/stderr=PIPE, encoding="utf-8", errors="replace" ← the change)
  2. _logging_subprocess.py  StreamLogger(name="AdaptorRuntimeStdoutLogger",
                             stream=self._process.stdout).start()
  3. _stream_logger.py:run   for line in iter(self._stream.readline, ""):
                                ← TextIOWrapper decodes here; strict mode raised
                                  UnicodeDecodeError (a ValueError subclass)
  4. _stream_logger.py:run   except ValueError: only "I/O operation on closed file"
                                is tolerated → decode error re-raised → thread dies
  5. (before fix) pipe fills → child blocks on write() → job hangs forever
  6. (after fix)  0xc7 → U+FFFD, line logged, reader stays alive

This confirms the PR description's mechanism exactly: StreamLogger.run's except ValueError clause re-raises anything that isn't the closed-file message, so UnicodeDecodeError was fatal to the thread.

Security assessment of errors="replace"

No meaningful security issue is introduced, and an availability one is removed. Vectors checked:

1. Log injection — no new vector. errors="replace" only affects invalid byte sequences, mapping them to the fixed character U+FFFD. It cannot manufacture newlines, ANSI escape sequences, or any control character: \n (0x0A) and ESC (0x1B) are valid UTF-8 and passed through identically before and after this change.
2. Regex-evasion of output handlers — not a trust boundary. A subprocess could garble a token with invalid bytes so a RegexHandler regex misses it — but the subprocess already fully controls its own stdout and could simply not print the token. The old behavior was strictly worse for monitoring integrity: after the first bad byte, all subsequent output (including error/completion markers) became invisible because the reader was dead.
3. The stdin encoder is also affected — Popen's encoding/errors apply to all three pipes, so writes to the child's stdin would now silently replace unencodable characters instead of raising. Nothing in the repo writes to this stdin (_process.stdin is only close()d in wait()), so no impact today. See finding 2.
4. Forensic information loss — U+FFFD discards the original byte value; you can't tell 0xC7 from 0xFF after the fact. See finding 1 for a strictly-better alternative.

Net: this fixes a denial-of-service-shaped bug (any job whose tooling emits one locale-encoded byte hangs forever) at zero trust-boundary cost.

Findings

No correctness or security issues confirmed. Suggestions and cleanup:

1. Consider errors="backslashreplace" instead of "replace" (suggestion). It keeps the reader alive exactly the same way, but renders the offending byte as \xc7 instead of �, preserving the original byte value in logs. Genuinely useful when debugging which codepage a DCC is emitting (the very scenario motivating this PR), and it's what CPython itself uses for its own stderr. Only the handler and the two test assertions would change.
2. The lenient handler silently applies to stdin too (low, note-for-the-future). If a future change or subclass writes text to _process.stdin, unencodable characters will be replaced silently rather than raising, which could corrupt commands sent to the child without any signal. Worth one extra line in the (already good) comment at the Popen call site.
3. test_non_utf8_output_does_not_kill_reader: the explicit p._cleanup_io_threads() is redundant (cleanup). p.wait() on the line above already calls _cleanup_io_threads() on every path (it's the last statement of wait()), so the explicit call joins already-joined threads and re-closes closed streams — harmless but dead, and it reaches into a private method the sibling test_log_levels doesn't need.
4. The root fragility in StreamLogger.run remains, acknowledged (informational). The except ValueError clause that string-sniffs "I/O operation on closed file" and re-raises everything else means any other ValueError from the read/log path still kills the reader silently. Deferring that to #274 is the right call for a minimal fix — just noting the deferral is real, not resolved.

What's good

• The one-line fix is the correct minimal fix at the correct layer (the decoder), rather than a band-aid in StreamLogger.run that would drop the bad line.
• The regression test is exactly right: a line before the bad byte, the bad byte, and a line after — asserting the after line directly proves the reader thread survived. Verified to fail on unmodified code.
• pytest.mark.timeout(10) on a test whose failure mode is a deadlock — good instinct.
• The comment at the Popen call site explains why in terms of the failure mode, not just what.
• All three existing Popen argument assertions were updated, so the unit suite pins the new argument.

Refuted but considered

• "Replacement could create false regex matches" — U+FFFD is a fixed substitution for invalid sequences only; it can't synthesize delimiters or spoof structured tokens any better than the subprocess already could by printing them directly.
• "Chunk-boundary bytes could decode wrongly" — TextIOWrapper uses an incremental decoder; a multi-byte sequence split across reads is handled correctly, and replace only fires on genuinely invalid sequences.
• "Windows universal-newline handling changes" — text mode and newline translation are independent of the errors handler; unaffected.

[leongdl]

Consider errors="backslashreplace" instead of "replace" (suggestion). It keeps the reader alive exactly the same way, but renders the offending byte as \xc7 instead of �, preserving the original byte value in logs. Genuinely useful when debugging which codepage a DCC is emitting (the very scenario motivating this PR), and it's what CPython itself uses for its own stderr. Only the handler and the two test assertions would change.

Would recommend doing this. We can push up a follow up PR :)

[leongdl]

We will merge this - and also will help to fix the Python 3.12 github action test error.