perf: download bundletool on demand instead of vendoring it - #6097
Conversation
Sorry, something went wrong.
WalkthroughHTTP 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: nathanwalker Poem 🚥 Pre-merge checks | ✅ 5 ✅ Passed checks (5 passed)
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. ❤️ ShareComment @coderabbitai help to get the list of available commands. |
Sorry, something went wrong.
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).
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 lift
Retrying a pipeTo request reuses an already-destroyed destination stream.
On a stuck request/response, httpRequestCore is retried with the same options object (Line 49). When options.pipeTo is set, the first attempt's failure runs pipeline(result.data, options.pipeTo) (or the request never even reaches that point), and Node's stream.pipeline destroys 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 (downloadBundleTool in lib/services/android/android-bundle-tool-service.ts), which passes a fs.createWriteStream as pipeTo.
🤖 Prompt for AI AgentsVerify 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 win
Close the download stream before deleteFile runs.
When options.pipeTo is used, pipeline() is not called in the .catch handler, so a failed HTTP stream leaves options.pipeTo open. downloadBundleTool then calls fs.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🤖 Prompt for AI Agents}).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) {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.
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.
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
📥 CommitsReviewing files that changed from the base of the PR and between ddd39b8 and bae97e9.
⛔ Files ignored due to path filters (1)- vendor/aab-tool/bundletool.jar is excluded by !**/*.jar
- lib/common/http-client.ts
- lib/constants.ts
- lib/services/android/android-bundle-tool-service.ts
- scripts/copy-assets.js
- test/services/android-bundle-tool-service.ts
- vendor/aab-tool/LICENSE
- vendor/aab-tool/README.txt
- vendor/aab-tool/README.txt
- vendor/aab-tool/LICENSE
- scripts/copy-assets.js
Sorry, something went wrong.
| 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",
);‼️ 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", | |
| ); |
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.
Sorry, something went wrong.
| 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;
}‼️ 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; | |
| } |
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.
Sorry, something went wrong.
PR Checklist
What is the current behavior?
vendor/aab-tool/bundletool.jar is 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. AndroidBundleToolService is only reached from AndroidApplicationManager.installApplication() when the artifact being installed is an .aab, i.e. ns run/ns deploy against a bundle. Producing the .aab is gradle's job and never touches the jar, so ns build android --aab doesn'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:
The version lives in the filename, so bumping BUNDLETOOL_VERSION simply fetches the new jar — no staleness logic, and two CLI versions can coexist on one machine. A failed download names the URL and points at NS_BUNDLETOOL_PATH.
The download reports progress through the existing $terminalSpinnerService, once, on first use.
Supporting changes
httpRequest gained a pipeTo option. It previously ran every response through JSON.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 with httpAgent alone every https:// 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 ./dist on a --release build, 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 .aab install on a machine now needs network access. NS_BUNDLETOOL_PATH covers 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
ns doctor needs no changes; it has never had any knowledge of bundletool or vendor/.
Summary by CodeRabbit
New Features
Bug Fixes