fix(controller): drain process output to avoid deadlock/silent failures (D12)#16
Merged
Merged
Conversation
…es (D12)
Several Runtime.exec(...).waitFor() calls in DeployFragment never read the
child's stdout/stderr. Once the ~64 KB OS pipe buffer fills the child blocks
writing while the parent blocks in waitFor() -- a deadlock, most plausibly on
the backup `tar | gzip` pipe when tar emits many warnings on a large rootfs.
Some calls also swallowed failures in empty `catch (Exception ignored) {}`
(tech-debt D12).
- New shared util/ProcessRunner.run(cmd): redirectErrorStream(true) so stderr is
merged into stdout, drains it fully, waits, and returns {exitCode, output}.
A single drain cannot deadlock, and callers get the exit code instead of
ignoring it.
- Migrated the raw exec().waitFor() sites in DeployFragment (backup pipe,
chmod -R, and the three rm -rf wipes) to ProcessRunner; the empty catches now
log the failure.
- Left the extraction path (already drains stderr and checks the exit code) and
the getprop read (reads stdout) unchanged.
No new unit test: ProcessRunner is process glue, not pure logic, and running a
shell from a unit test would be fragile on a non-Linux dev box (consistent with
M4/S3/D6). Verified by inspection + the CI compile/test gate.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Phase 1 (security hardening). Several
Runtime.exec(...).waitFor()calls inDeployFragmentnever read the child's stdout/stderr. Once the ~64 KB OS pipe buffer fills, the child blocks writing while the parent blocks inwaitFor()— a deadlock, most plausibly on the backuptar | gzippipe whentaremits many warnings on a large rootfs. Some calls also swallowed failures in emptycatch (Exception ignored) {}. Tech-debt item D12.Changes
util/ProcessRunner.run(cmd)(new, shared):redirectErrorStream(true)(merge stderr into stdout) + full drain + returns{exitCode, output}. A single read cannot deadlock, and callers get the exit code instead of ignoring it. Sameutil/pattern asByteFormatter/LocalVarsYamlParser.exec().waitFor()sites inDeployFragment— the backup pipe,chmod -R, and the threerm -rfwipes — toProcessRunner; the empty catches now log the failure.getpropread (reads stdout).Why no new test
ProcessRunneris process glue, not pure logic, and running a shell from a unit test would be fragile on a non-Linux dev box (consistent with M4/S3/D6). The checked exceptions it throws are exactly thoseexec()/waitFor()already threw, so the existing try/catch blocks cover them. Verified by inspection + the CI compile/test gate.How to identify it in the build / behaviour
No user-visible change on success. A failing
chmod/rm/backup now logs… failed (exit N): …instead of failing silently, and a backup with verbosetarstderr no longer risks hanging. Independent of the in-flight D11 PR (touches the same backup block region, so a trivial rebase may be needed when one merges).