Fix #256: close subprocess stdin so reads see EOF (aspect CLI hang)#351
Merged
Conversation
Adds an @ignore'd JUnit reproducer that demonstrates the most likely underlying cause of #256 (bazel-diff freezes when aspect CLI is installed). `process()` calls `ProcessBuilder.start()` without ever redirecting or closing the subprocess's stdin pipe, so any subprocess that reads from stdin (as the aspect CLI v1's interactive first-run path appears to do) blocks forever waiting for input. The test spawns `cat` (which blocks on stdin read), wraps the call in `withTimeoutOrNull(5_000)`, and asserts the result is null — proving the deadlock. Confirmed locally: with @ignore removed, the test runs exactly 5s and passes; with @ignore in place CI stays green. No production code change in this PR. The fix is intentionally out-of-scope; this PR just locks in a regression target for whoever picks up the work on #256. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`process()` started subprocesses via `ProcessBuilder.start()` without redirecting or closing stdin. Java defaults stdin to `Redirect.PIPE`, so the subprocess received an open, never-fed pipe; any subprocess that read from stdin on startup (e.g. aspect CLI's interactive first-run path) blocked forever on `read()`, and `process.waitFor()` blocked too — matching the user-reported `FUTEX_WAIT` strace. Close `process.outputStream` immediately after `start()` so the subprocess sees EOF on stdin, mirroring what `ExternalRepoResolver.runProcessAndCaptureFirstLine` already does. Flip the @ignore'd reproducer into a real regression test and rename it to `ProcessStdinHangRegressionTest` to match the codebase's `*RegressionTest` convention. The test now asserts that `cat` exits cleanly within the 5s timeout — without the fix it hangs and the timeout fires. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
tinder-maxwellelliott
changed the title
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
Fixes #256 (cross-posted at aspect-build/aspect-cli-legacy#41):
bazel-difffroze forever when.bazeliskrcwas set to download aspect CLI, with aFUTEX_WAITon the Java process.Root cause
cli/src/main/kotlin/com/bazel_diff/process/Process.ktbuilt the subprocess withProcessBuilder(*command).start()and never redirected or closed stdin.ProcessBuilderdefaultsredirectInputtoRedirect.PIPE, so the subprocess received an open, never-fed stdin pipe. Any subprocess that calledread(stdin)on startup — which aspect CLI v1's interactive first-run path does — blocked indefinitely, the parentlineFlow-on-stdout never saw EOF, andprocess.waitFor()parked forever.Fix
Close
process.outputStreamimmediately afterstart()so the subprocess sees EOF on stdin. This mirrors whatExternalRepoResolver.runProcessAndCaptureFirstLinealready does, and is preferred overredirectInput(Redirect.from(File("/dev/null")))because it works cross-platform without hard-coding a device path.Regression test
The original
@Ignored reproducer (first commit on this branch) is now flipped into a real regression test and renamed toProcessStdinHangRegressionTestto match the codebase's*RegressionTestconvention (cf.StderrPollutionRegressionTest). It spawnscat(a minimal subprocess that blocks on stdin) and asserts the call completes within 5s withresultCode == 0. Without the fix the timeout fires; with the fix the test passes in ~0.1s.Test plan
bazel test //cli:ProcessStdinHangRegressionTest— passes in ~0.4s with the fix.Process.kthunk locally and re-ran the test — it timed out at the 5s mark andassertNotNullfailed, confirming the test still catches the regression.bazel test //cli/...(excludingE2ETest, which fails locally on this machine with an unrelated JDK sandbox auto-config error both before and after this change) — all 14 unit tests pass, including the new regression test.E2ETestend-to-end.🤖 Generated with Claude Code