perf: download bundletool on demand instead of vendoring it - #6097
Conversation
📝 WalkthroughWalkthroughHTTP downloads can now stream into destinations. Android bundletool resolution now supports overrides, cached downloads, checksum validation, locking, progress reporting, and atomic installation, with tests covering these paths. ChangesBundletool acquisition and HTTP streaming
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant AndroidBundleToolService
participant IFileSystem
participant ILockService
participant HttpClient
AndroidBundleToolService->>IFileSystem: check cached bundletool JAR
AndroidBundleToolService->>ILockService: coordinate download
AndroidBundleToolService->>HttpClient: stream bundletool release
HttpClient->>IFileSystem: write temporary download
AndroidBundleToolService->>IFileSystem: verify checksum and rename JAR
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
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 |
The 32MB bundletool jar shipped with every install, but it is only used to convert and install .aab artifacts on a device - a path most users never hit, since building an aab is gradle's job. Resolve it lazily instead: on first use fetch the pinned release into <profileDir>/bundletool/, verify its sha256, and move it into place atomically under a lock so concurrent CLIs neither race nor download it twice. NS_BUNDLETOOL_PATH overrides the whole flow for offline and air-gapped setups. This needs a streaming download, so httpRequest grows a pipeTo option - it previously ran every response through JSON.stringify, which would corrupt a binary payload. The same call now also sets httpsAgent, since axios selects the agent by protocol and https requests were silently bypassing the configured proxy tunnel. Release package drops from 34.0MB to 3.4MB (42.9MB to 10.3MB unpacked).
3b9492f to
bae97e9
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
lib/common/http-client.ts (2)
41-59: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftRetrying a
pipeTorequest reuses an already-destroyed destination stream.On a stuck request/response,
httpRequestCoreis retried with the sameoptionsobject (Line 49). Whenoptions.pipeTois set, the first attempt's failure runspipeline(result.data, options.pipeTo)(or the request never even reaches that point), and Node'sstream.pipelinedestroys both source and destination streams on error. Retrying with the same destroyed write stream will either throw (ERR_STREAM_DESTROYED) or produce a corrupted/incomplete file, defeating the purpose of the retry.🔧 Suggested fix: don't retry when a destination stream is involved
) { + if (options.pipeTo) { + // the destination stream from the failed attempt is already + // destroyed or partially written; retrying would corrupt it + throw err; + } // Retry the request immediately because there are at least 10 seconds between the two requests. this.$logger.warn( "%s Retrying request to %s...", err.message, options.url || options, ); const retryResult = await this.httpRequestCore(options, proxySettings);This directly affects the new bundletool download path (
downloadBundleToolinlib/services/android/android-bundle-tool-service.ts), which passes afs.createWriteStreamaspipeTo.🤖 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 `@lib/common/http-client.ts` around lines 41 - 59, Update the retry branch in httpRequestCore to skip the immediate retry whenever options.pipeTo is set, allowing the original error to propagate instead of reusing a destroyed destination stream. Preserve the existing retry behavior for requests without a pipeTo destination, including its logging and response handling.
111-140: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winClose the download stream before
deleteFileruns.When
options.pipeTois used,pipeline()is not called in the.catchhandler, so a failed HTTP stream leavesoptions.pipeToopen.downloadBundleToolthen callsfs.unlinkSync(tempPath)on that open file handle, which can fail on Windows and leak the descriptor on all platforms.Suggested fix: destroy the destination stream on any request failure
}).catch((err) => { this.$logger.trace("An error occurred while sending the request:", err); + if (options.pipeTo && typeof options.pipeTo.destroy === "function") { + options.pipeTo.destroy(); + } if (err.response) {🤖 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 `@lib/common/http-client.ts` around lines 111 - 140, Update the request error handler in the axios call to destroy or otherwise close the destination stream referenced by options.pipeTo before rethrowing the error. Ensure this cleanup runs for every failed piped request, while preserving existing error-message handling and behavior for non-stream requests.
🤖 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 `@lib/common/http-client.ts`:
- Around line 44-48: Update the retry warning in the HTTP client’s logger.warn
call to avoid passing the full options object when options.url is falsy. Log
only a safe, bounded request identifier or fallback value that excludes headers,
credentials, and request bodies, while preserving the existing URL logging
behavior when available.
In `@lib/services/android/android-bundle-tool-service.ts`:
- Around line 119-124: Update getBundleToolPath to clear bundleToolPathPromise
when resolveBundleToolPath rejects, while retaining the cached promise after
successful resolution. Ensure subsequent installApks or buildApks calls can
retry resolution after a transient failure.
---
Outside diff comments:
In `@lib/common/http-client.ts`:
- Around line 41-59: Update the retry branch in httpRequestCore to skip the
immediate retry whenever options.pipeTo is set, allowing the original error to
propagate instead of reusing a destroyed destination stream. Preserve the
existing retry behavior for requests without a pipeTo destination, including its
logging and response handling.
- Around line 111-140: Update the request error handler in the axios call to
destroy or otherwise close the destination stream referenced by options.pipeTo
before rethrowing the error. Ensure this cleanup runs for every failed piped
request, while preserving existing error-message handling and behavior for
non-stream requests.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6d982bbf-bd80-4939-af56-08c523f8de76
⛔ Files ignored due to path filters (1)
vendor/aab-tool/bundletool.jaris excluded by!**/*.jar
📒 Files selected for processing (7)
lib/common/http-client.tslib/constants.tslib/services/android/android-bundle-tool-service.tsscripts/copy-assets.jstest/services/android-bundle-tool-service.tsvendor/aab-tool/LICENSEvendor/aab-tool/README.txt
💤 Files with no reviewable changes (3)
- vendor/aab-tool/README.txt
- vendor/aab-tool/LICENSE
- scripts/copy-assets.js
| this.$logger.warn( | ||
| "%s Retrying request to %s...", | ||
| err.message, | ||
| options.url || options | ||
| options.url || options, | ||
| ); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Retry-warning fallback can log the entire options object, including headers/credentials.
options.url || options will log the full options object (potentially containing Authorization headers, proxy credentials, or request bodies) whenever url is falsy. Prefer logging a safe, bounded subset.
🔧 Suggested fix
this.$logger.warn(
"%s Retrying request to %s...",
err.message,
- options.url || options,
+ options.url || options.method || "unknown request",
);📝 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.
| this.$logger.warn( | |
| "%s Retrying request to %s...", | |
| err.message, | |
| options.url || options | |
| options.url || options, | |
| ); | |
| this.$logger.warn( | |
| "%s Retrying request to %s...", | |
| err.message, | |
| options.url || options.method || "unknown request", | |
| ); |
🤖 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 `@lib/common/http-client.ts` around lines 44 - 48, Update the retry warning in
the HTTP client’s logger.warn call to avoid passing the full options object when
options.url is falsy. Log only a safe, bounded request identifier or fallback
value that excludes headers, credentials, and request bodies, while preserving
the existing URL logging behavior when available.
| private getBundleToolPath(): Promise<string> { | ||
| this.bundleToolPathPromise = | ||
| this.bundleToolPathPromise || this.resolveBundleToolPath(); | ||
|
|
||
| return this.bundleToolPathPromise; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
A failed resolution is cached forever for the life of the process.
bundleToolPathPromise is memoized unconditionally. If resolveBundleToolPath() rejects once (transient network blip, momentary checksum mismatch, etc.), every subsequent installApks/buildApks call in the same process will immediately re-reject with the same stale error — there's no way to retry without restarting the CLI process.
🔧 Suggested fix: clear the cached promise on failure
private getBundleToolPath(): Promise<string> {
- this.bundleToolPathPromise =
- this.bundleToolPathPromise || this.resolveBundleToolPath();
+ if (!this.bundleToolPathPromise) {
+ this.bundleToolPathPromise = this.resolveBundleToolPath().catch(
+ (err) => {
+ this.bundleToolPathPromise = null;
+ throw err;
+ },
+ );
+ }
return this.bundleToolPathPromise;
}📝 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.
| private getBundleToolPath(): Promise<string> { | |
| this.bundleToolPathPromise = | |
| this.bundleToolPathPromise || this.resolveBundleToolPath(); | |
| return this.bundleToolPathPromise; | |
| } | |
| private getBundleToolPath(): Promise<string> { | |
| if (!this.bundleToolPathPromise) { | |
| this.bundleToolPathPromise = this.resolveBundleToolPath().catch( | |
| (err) => { | |
| this.bundleToolPathPromise = null; | |
| throw err; | |
| }, | |
| ); | |
| } | |
| return this.bundleToolPathPromise; | |
| } |
🤖 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 `@lib/services/android/android-bundle-tool-service.ts` around lines 119 - 124,
Update getBundleToolPath to clear bundleToolPathPromise when
resolveBundleToolPath rejects, while retaining the cached promise after
successful resolution. Ensure subsequent installApks or buildApks calls can
retry resolution after a transient failure.
2 participants
PR Checklist
What is the current behavior?
vendor/aab-tool/bundletool.jaris checked into the repo and shipped with every install. At 32,515,387 bytes it is the single largest thing in the package — roughly nine tenths of the published tarball.It earns none of that.
AndroidBundleToolServiceis only reached fromAndroidApplicationManager.installApplication()when the artifact being installed is an.aab, i.e.ns run/ns deployagainst a bundle. Producing the.aabis gradle's job and never touches the jar, sons build android --aabdoesn't need it either. Every user pays 32MB of download and disk for a path most of them never take.What is the new behavior?
bundletool is resolved lazily, the first time it is actually invoked, and cached per-user:
NS_BUNDLETOOL_PATH, if set, is used as-is (fails loudly if the file is missing) — the escape hatch for offline, air-gapped or mirrored setups.<profileDir>/bundletool/bundletool-all-<version>.jar, reused when its sha256 matches the pinned value.$lockServiceso two concurrent CLIs neither race nor fetch the same 32MB twice. A tampered or partial cache entry is discarded and re-fetched.The version lives in the filename, so bumping
BUNDLETOOL_VERSIONsimply fetches the new jar — no staleness logic, and two CLI versions can coexist on one machine. A failed download names the URL and points atNS_BUNDLETOOL_PATH.The download reports progress through the existing
$terminalSpinnerService, once, on first use.Supporting changes
httpRequestgained apipeTooption. It previously ran every response throughJSON.stringify, which buffers and corrupts a binary payload — streaming was a prerequisite, not a nicety.The same axios call now also passes
httpsAgent. axios selects the agent by protocol, so withhttpAgentalone everyhttps://request was silently bypassing the configured proxy tunnel. That is a pre-existing bug, but the download depends on the fix, so it is included here.Size
Measured with
npm pack --dry-run ./diston a--releasebuild, before and after:A jar is already-compressed, so the saving survives gzip almost intact: -90% on the published tarball.
vendor/still ships —vendor/gradle-plugin(148KB) remains vendored and unaffected.Trade-off
The first
.aabinstall on a machine now needs network access.NS_BUNDLETOOL_PATHcovers the cases where that is not acceptable. Given the jar is untouched by every other command, taxing every install to pre-stage it was the worse default.Verification
objects.githubusercontent.comfollowed transparently, progress events delivered with a total,bodyleft undefined on the pipe path, and the resulting sha256 equal to the pinned constant — which is also byte-identical to the jar this PR deletes.ns doctorneeds no changes; it has never had any knowledge of bundletool orvendor/.Summary by CodeRabbit
New Features
Bug Fixes