ADFA-3945: Handle reader-thread IOException on tar.xz extraction timeout#1412
ADFA-3945: Handle reader-thread IOException on tar.xz extraction timeout#1412fryanpan wants to merge 2 commits into
Conversation
9f4d410 to
1d4c800
Compare
|
This PR previously caught the reader-thread Rejected Option A (close-then-destroy): on Android, closing a stream under a blocked read is precisely what throws Branch was rewritten to a single clean commit ( |
…thread race) The Termux tar extraction drained the child's stdout on a separate reader thread. On a 2-minute timeout the code calls destroyForcibly(), which closes the pipe; the reader thread's in-flight read() then throws InterruptedIOException on a bare thread with no handler, crashing the app (Sentry APPDEVFORALL-V0). Fix (root cause, not the symptom): redirect the child's combined stdout/stderr to a temp file via ProcessBuilder.redirectOutput(), drop the reader thread entirely, and read the log back after waitFor(). With OS-side redirection there is no concurrent read to interrupt when the pipe is closed, and the process also can't deadlock on a full pipe buffer. destroyForcibly() on timeout is retained; the temp log is deleted after use. This eliminates the entire bug class rather than catching the exception, and makes a future timeout/cancellation change safe (no race to reintroduce). Per Hal's suggestion on ADFA-3945. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1d4c800 to
847c79e
Compare
📝 Walkthrough
Walkthrough
ChangesTermux tar extraction output redirection
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@plugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/services/IdeArchiveServiceImpl.kt`:
- Around line 293-295: The process lifecycle in IdeArchiveServiceImpl’s tar
execution block is incomplete: destroyForcibly() is asynchronous, and
InterruptedException from waitFor(...) can leave the spawned process running.
Update the tar handling around process.waitFor(...) so that when completion
times out or an interruption occurs, the process is forcibly terminated and then
waited on before continuing, and make sure the interruption path in this method
cleans up the process and preserves the interrupted state.
- Around line 312-313: The temporary extraction log cleanup in
IdeArchiveServiceImpl is ignoring deletion failures, so the log can remain
behind without notice. Update the cleanup path around the .also block after
extraction to check the result of logFile.delete(), and if it fails, record a
warning or error with the existing logger so the failure is visible. Use the
logFile cleanup point in this extraction flow to keep the behavior clear and
localized.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 93a1874f-bc7d-4869-b931-45ba99a7294c
📒 Files selected for processing (1)
plugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/services/IdeArchiveServiceImpl.kt
| val completed = process.waitFor(2, TimeUnit.MINUTES) | ||
| if (!completed) process.destroyForcibly() | ||
| reader.join() | ||
| exitCode = if (completed) process.exitValue() else -1 |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Wait for the forcibly killed tar process and clean it up on interruption.
destroyForcibly() is asynchronous, so the method can return/read/delete the log while tar is still exiting. Also, if waitFor(...) throws InterruptedException, the started process is never destroyed.
Suggested lifecycle handling
- val completed = process.waitFor(2, TimeUnit.MINUTES)
- if (!completed) process.destroyForcibly()
- exitCode = if (completed) process.exitValue() else -1
+ try {
+ val completed = process.waitFor(2, TimeUnit.MINUTES)
+ if (completed) {
+ exitCode = process.exitValue()
+ } else {
+ process.destroyForcibly()
+ process.waitFor(10, TimeUnit.SECONDS)
+ exitCode = -1
+ }
+ } catch (e: InterruptedException) {
+ process.destroyForcibly()
+ Thread.currentThread().interrupt()
+ throw e
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| val completed = process.waitFor(2, TimeUnit.MINUTES) | |
| if (!completed) process.destroyForcibly() | |
| reader.join() | |
| exitCode = if (completed) process.exitValue() else -1 | |
| try { | |
| val completed = process.waitFor(2, TimeUnit.MINUTES) | |
| if (completed) { | |
| exitCode = process.exitValue() | |
| } else { | |
| process.destroyForcibly() | |
| process.waitFor(10, TimeUnit.SECONDS) | |
| exitCode = -1 | |
| } | |
| } catch (e: InterruptedException) { | |
| process.destroyForcibly() | |
| Thread.currentThread().interrupt() | |
| throw e | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@plugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/services/IdeArchiveServiceImpl.kt`
around lines 293 - 295, The process lifecycle in IdeArchiveServiceImpl’s tar
execution block is incomplete: destroyForcibly() is asynchronous, and
InterruptedException from waitFor(...) can leave the spawned process running.
Update the tar handling around process.waitFor(...) so that when completion
times out or an interruption occurs, the process is forcibly terminated and then
waited on before continuing, and make sure the interruption path in this method
cleans up the process and preserves the interrupted state.
| }.also { | ||
| logFile.delete() |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Handle failed deletion of the temporary extraction log.
The log contains combined process output and currently remains silently if deletion fails.
Suggested cleanup hardening
- }.also {
- logFile.delete()
+ }.also {
+ if (logFile.exists() && !logFile.delete()) {
+ logger.warn("Failed to delete Termux extraction log: ${logFile.absolutePath}")
+ logFile.deleteOnExit()
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| }.also { | |
| logFile.delete() | |
| }.also { | |
| if (logFile.exists() && !logFile.delete()) { | |
| logger.warn("Failed to delete Termux extraction log: ${logFile.absolutePath}") | |
| logFile.deleteOnExit() | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@plugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/services/IdeArchiveServiceImpl.kt`
around lines 312 - 313, The temporary extraction log cleanup in
IdeArchiveServiceImpl is ignoring deletion failures, so the log can remain
behind without notice. Update the cleanup path around the .also block after
extraction to check the result of logFile.delete(), and if it fails, record a
warning or error with the existing logger so the failure is visible. Use the
logFile cleanup point in this extraction flow to keep the behavior clear and
localized.
Jira Ticket: https://appdevforall.atlassian.net/browse/ADFA-3945
Sentry Issue: https://appdevforall-inc-9p.sentry.io/issues/APPDEVFORALL-V0
Reproduction Details
IdeArchiveServiceImpl.extractTarXzViaTermuxran Termuxtarand drained the child's stdout on a separate reader thread (thread { process.inputStream.bufferedReader().useLines { … } }). On a 2-minute timeout the code callsdestroyForcibly(), which closes the pipe; the reader thread's in-flightread()then throwsInterruptedIOExceptionon a bare thread with no handler → uncaught → app crash.Stack Trace
(thread:
tar-xz-extract-output· device.class: high)User Steps
User steps leading up to crash, based on Sentry breadcrumbs:
org.appdevforall.ndkinstaller.cgp). The extraction is force-killed (timeout/cancel) and the reader thread dies mid-read. (The crashing event itself has no UI breadcrumbs — it's a background reader thread.)Was able to reproduce in a unit test?
No.
What remains after this fix is the
ProcessBuilder/Termux path —extractTarXzViaTermuxis aprivate funon afinalclass and execs a hard-coded on-devicetarbinary ($TERMUX_BIN_PATH/tar) absent on any JVM test host, sostart()can't run there. However, Option B removes the concurrency hazard entirely (no reader thread), so the failure class this ticket is about no longer exists to be triggered. Making the remaining exec path regression-testable would need an injectable process/command seam (@VisibleForTesting) — tracked as a follow-up. The change compiles cleanly (:plugin-manager:compileV8DebugKotlinBUILD SUCCESSFUL).What Was Fixed
Root-cause fix (Hal's suggestion, "Option B"), not a symptom catch. Redirect the child's combined stdout/stderr to a temp file via
ProcessBuilder.redirectOutput(logFile)and read it back afterwaitFor(), dropping the reader thread entirely:read()to be interrupted whendestroyForcibly()closes the pipe — theInterruptedIOException-on-a-bare-thread crash cannot occur.destroyForcibly()on timeout is retained; the temp log is deleted after use.(Considered and rejected "Option A" — close stdout then
destroyForcibly(): on Android, closing a stream under a blocked read is exactly what producesread interrupted by close(), so it wouldn't reliably give a clean EOF and would still need the catch.)Testing
:plugin-manager:compileV8DebugKotlin→ BUILD SUCCESSFUL. No automated runtime test (see above — the remaining exec path isn't JVM-testable; the bug class itself is removed by construction). Branch history was rewritten to a single clean commit implementing Option B.Follow-up (separate): make the extraction timeout adaptive for slow devices + add progress/cancel UI + a retry path (the user-facing "extraction actually succeeds" improvement), and add an
@VisibleForTestingprocess seam.Fixes APPDEVFORALL-V0