GRB-02 백엔드 검증 루프 hardening과 Docker preflight 추가#75
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughDocker 가용성을 확인하는 preflight 검증 시스템을 도입했습니다. 새로운 Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/test/java/com/gitranker/api/testsupport/DockerPreflightCheck.java (2)
95-97:CommandResult.failure()팩토리 메서드에서 exitCode 검증 누락
failure()팩토리 메서드가exitCode == 0을 허용합니다. 의미론적으로 failure는 non-zero exit code를 가져야 하므로, 잘못된 사용을 방지하기 위해 검증을 추가하는 것이 좋습니다.♻️ exitCode 검증 추가
static CommandResult failure(int exitCode, String stdout, String stderr) { + if (exitCode == 0) { + throw new IllegalArgumentException("Failure result must have non-zero exit code"); + } return new CommandResult(exitCode, stdout, stderr); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/com/gitranker/api/testsupport/DockerPreflightCheck.java` around lines 95 - 97, The failure() factory currently allows exitCode == 0; update CommandResult.failure(int exitCode, String stdout, String stderr) to validate that exitCode is non-zero and throw an IllegalArgumentException (or similar) when exitCode == 0 to enforce the semantic that failure must have a non-zero exit code; locate the failure method in DockerPreflightCheck/Test support and add the check at the start of the method (e.g., if (exitCode == 0) throw new IllegalArgumentException("failure exitCode must be non-zero")).
135-140: 스트림 읽기 순서로 인한 잠재적 데드락
process.waitFor()호출 전에stdout과stderr를 순차적으로readAllBytes()로 읽고 있습니다. 프로세스 출력이 OS 파이프 버퍼(보통 64KB)를 초과하면 프로세스가 블록되고, 동시에 Java 코드도 다른 스트림을 기다리며 데드락이 발생할 수 있습니다.
docker version출력은 일반적으로 매우 작아서 실제 문제가 될 가능성은 낮지만, 더 안전한 패턴은 스트림을 별도 스레드에서 drain하거나ProcessBuilder.redirectErrorStream(true)를 사용하는 것입니다.♻️ 더 안전한 스트림 처리 예시 (선택 사항)
`@Override` public CommandResult run(List<String> command) { ProcessBuilder processBuilder = new ProcessBuilder(command); + processBuilder.redirectErrorStream(false); try { Process process = processBuilder.start(); - String stdout = new String(process.getInputStream().readAllBytes(), StandardCharsets.UTF_8); - String stderr = new String(process.getErrorStream().readAllBytes(), StandardCharsets.UTF_8); - int exitCode = process.waitFor(); + CompletableFuture<String> stdoutFuture = CompletableFuture.supplyAsync(() -> readStream(process.getInputStream())); + CompletableFuture<String> stderrFuture = CompletableFuture.supplyAsync(() -> readStream(process.getErrorStream())); + int exitCode = process.waitFor(); + String stdout = stdoutFuture.join(); + String stderr = stderrFuture.join(); return new CommandResult(exitCode, stdout, stderr);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/com/gitranker/api/testsupport/DockerPreflightCheck.java` around lines 135 - 140, The current implementation in DockerPreflightCheck reads process.getInputStream().readAllBytes() and then process.getErrorStream().readAllBytes() before calling process.waitFor(), which can deadlock for large outputs; fix by draining both streams concurrently or by configuring the ProcessBuilder to merge streams. Update the logic around processBuilder.start()/Process usage in the method that returns CommandResult so that stdout and stderr are read asynchronously (e.g., spawn two reader threads/futures to collect streams while the process runs) or set processBuilder.redirectErrorStream(true) and only read the merged stream, then call process.waitFor() and construct the CommandResult from the collected output.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/test/java/com/gitranker/api/testsupport/DockerPreflightCheck.java`:
- Around line 95-97: The failure() factory currently allows exitCode == 0;
update CommandResult.failure(int exitCode, String stdout, String stderr) to
validate that exitCode is non-zero and throw an IllegalArgumentException (or
similar) when exitCode == 0 to enforce the semantic that failure must have a
non-zero exit code; locate the failure method in DockerPreflightCheck/Test
support and add the check at the start of the method (e.g., if (exitCode == 0)
throw new IllegalArgumentException("failure exitCode must be non-zero")).
- Around line 135-140: The current implementation in DockerPreflightCheck reads
process.getInputStream().readAllBytes() and then
process.getErrorStream().readAllBytes() before calling process.waitFor(), which
can deadlock for large outputs; fix by draining both streams concurrently or by
configuring the ProcessBuilder to merge streams. Update the logic around
processBuilder.start()/Process usage in the method that returns CommandResult so
that stdout and stderr are read asynchronously (e.g., spawn two reader
threads/futures to collect streams while the process runs) or set
processBuilder.redirectErrorStream(true) and only read the merged stream, then
call process.waitFor() and construct the CommandResult from the collected
output.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 07eaa1f8-d1bc-4980-a9b5-0b1f623ebf01
📒 Files selected for processing (5)
README.mdbuild.gradlesrc/test/java/com/gitranker/api/testsupport/DockerPreflightCheck.javasrc/test/java/com/gitranker/api/testsupport/DockerPreflightCheckTest.javasrc/test/java/com/gitranker/api/testsupport/DockerPreflightMain.java
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 67234f6124
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| String stdout = new String(process.getInputStream().readAllBytes(), StandardCharsets.UTF_8); | ||
| String stderr = new String(process.getErrorStream().readAllBytes(), StandardCharsets.UTF_8); |
There was a problem hiding this comment.
Reading stdout to EOF and only then reading stderr can deadlock this preflight task when docker writes enough data to stderr before exiting: the child process blocks on a full error pipe, while the parent is still blocked in readAllBytes() on stdout. In that case verifyDockerAvailable (and therefore integrationTest) can hang indefinitely instead of failing fast, especially under verbose Docker/plugin error output.
Useful? React with 👍 / 👎.
|
CodeRabbit nitpick 2건 반영했습니다.
재검증:
후속 커밋: |
1 participant
1) 요약
integrationTest전에verifyDockerAvailablepreflight를 추가해 Docker 환경 문제를 Testcontainers 예외보다 먼저 fail-fast 하도록 정리했습니다.2) 연관 이슈
3) 문제와 목표
./gradlew integrationTest가ContainerFetchException과DockerClientProviderStrategy예외로 바로 실패해 환경 문제와 코드 회귀를 빠르게 구분하기 어려웠습니다.4) 영향 범위
build.gradle,src/test/java/com/gitranker/api/testsupport/*,README.md5) 검증 증거
./gradlew verifyDockerAvailableDocker preflight passed. context=orbstack serverApiVersion=1.51확인./gradlew test --tests "com.gitranker.api.testsupport.DockerPreflightCheckTest"./gradlew integrationTest./gradlew test jacocoTestCoverageVerification추가 확인:
./gradlew verifyDockerAvailable와./gradlew integrationTest에서 preflight fail-fast 메시지 출력 확인6) 관측성 확인
7) AI 리뷰 메모 (선택)
JavaExectask로 환경 문제를 테스트 가능하게 분리함8) 리스크 및 롤백
docker version --format "{{.Server.APIVersion}}"를 지원하지 않는 매우 오래된 Docker CLI 환경에서는 추가 보정이 필요할 수 있습니다.verifyDockerAvailabletask와integrationTestdependency를 되돌리고 README verification 섹션을 제거합니다.9) 체크리스트
Summary by CodeRabbit
릴리스 노트
Documentation
Tests
Chores