fix(rust): cut retained runtime-install memory by ~99% - #2676
mohamedmansour wants to merge 21 commits into
Conversation
Compare runtime assets in bounded chunks, stage changed entries without whole-image buffers, and validate the archive before atomic publication. Preserve read-only warm caches, repair unreadable or corrupt files, and reject duplicate normalized destinations. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Exercise the public installer in isolated cold, warm, corrupt and truncated-cache processes. Record matched release-build memory and latency measurements, output identities, separate allocator diagnostics, and native-platform limitations. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
Destination deduplication misses aliases on case-insensitive filesystems, allowing later entries to overwrite required runtime artifacts.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Bounds Rust bundled-runtime installation memory through streamed comparison, staged writes, and atomic per-file publication.
Changes:
- Streams runtime extraction and comparison with bounded buffers.
- Adds extensive installer regression tests.
- Adds a reproducible macOS benchmark and documentation.
File summaries
| File | Description |
|---|---|
rust/src/embeddedcli.rs |
Implements streaming installation and tests. |
rust/README.md |
Documents installer behavior. |
rust/examples/runtime_install.rs |
Adds installer probe. |
rust/Cargo.toml |
Packages benchmark assets. |
rust/benchmarks/runtime_install.py |
Adds measurement harness. |
rust/benchmarks/runtime_install.md |
Documents results and reproduction. |
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Keep the small installer example and runtime behavior documentation, while retaining measurement evidence outside the repository. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Normalize separators and deduplicate case-insensitively before artifact selection. Reject non-portable archive names rather than approximating filesystem-specific Unicode, DOS short-name, or trailing-dot aliases. Cover cold, warm and repair installs through the real installer. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Remove the README additions and standalone measurement example. Preserve reproduction details and measurements in the pull request rather than shipping a benchmark surface. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
mohamedmansour
changed the title
|
There is one P2 behavioral regression that should be fixed before merge. On Unix, The new read-only-cache test only removes write permission from the directories, so it does not exercise this case. Please make warm validation ignore irrelevant write-bit differences (while still requiring the executable bits needed by the wrapper), and extend the test to remove write bits from the installed files as well. I did not find another P0/P1/P2 issue. The streaming design and archive/publish hardening otherwise look directionally sound. Generated by Copilot |
Ignore write-bit differences when validating installed runtime files, while retaining read and execute permission checks. Cover read-only installed files and repair of each missing wrapper execute bit. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
@stephentoub Accepted and fixed in 61cb61c: I reproduced the |
stephentoub
left a comment
stephentoub
left a comment
There was a problem hiding this comment.
Approved with the remaining permission-mode concern noted inline.
Generated by Copilot
Generate per-file SHA256, size and mode alongside the unchanged runtime archive. Verify valid warm caches without reading compressed bytes, and perform cold or repair extraction in one bounded forward traversal with manifest and gzip integrity checks before publication. Use sha2 0.11 runtime-detected acceleration with a software fallback, preserving the existing miniz_oxide decompression backend. Cover generated output identity, zero archive reads, immutable cache reuse and mixed-cache repair failures. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
mohamedmansour
changed the title
Verify readability through bounded hashing and executable access using effective process credentials rather than requiring every permission class to match the archive. Reuse locked rustix for a safe Unix access check and cover owner-only cache modes without re-extraction. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Compare runtime assets in bounded chunks, stage changed entries without whole-image buffers, and validate the archive before atomic publication. Preserve read-only warm caches, repair unreadable or corrupt files, and reject duplicate normalized destinations. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Exercise the public installer in isolated cold, warm, corrupt and truncated-cache processes. Record matched release-build memory and latency measurements, output identities, separate allocator diagnostics, and native-platform limitations. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Keep the small installer example and runtime behavior documentation, while retaining measurement evidence outside the repository. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Normalize separators and deduplicate case-insensitively before artifact selection. Reject non-portable archive names rather than approximating filesystem-specific Unicode, DOS short-name, or trailing-dot aliases. Cover cold, warm and repair installs through the real installer. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Remove the README additions and standalone measurement example. Preserve reproduction details and measurements in the pull request rather than shipping a benchmark surface. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Ignore write-bit differences when validating installed runtime files, while retaining read and execute permission checks. Cover read-only installed files and repair of each missing wrapper execute bit. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Generate per-file SHA256, size and mode alongside the unchanged runtime archive. Verify valid warm caches without reading compressed bytes, and perform cold or repair extraction in one bounded forward traversal with manifest and gzip integrity checks before publication. Use sha2 0.11 runtime-detected acceleration with a software fallback, preserving the existing miniz_oxide decompression backend. Cover generated output identity, zero archive reads, immutable cache reuse and mixed-cache repair failures. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Verify readability through bounded hashing and executable access using effective process credentials rather than requiring every permission class to match the archive. Reuse locked rustix for a safe Unix access check and cover owner-only cache modes without re-extraction. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
4c21738 to
523f7b5
Compare
…ansour-sdk-runtime-memory
mohamedmansour
changed the title
Remove the generated hash manifest and runtime sha2/rustix dependencies. Compare installed files directly against streamed archive entries with fixed-size buffers, and retain the original content-based warm-cache permission behavior. Keep atomic staging, archive integrity checks and regression coverage for immutable native libraries and multi-chunk corruption. Restore Cargo.toml, Cargo.lock and the build generator exactly to the PR base. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
mohamedmansour
changed the title
Check only the runtime wrapper with POSIX faccessat and effective credentials, preserving readable native libraries and owner-only executable caches. Repair inaccessible wrappers through the existing staged publish path and reject persistent execute denial before returning. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
stephentoub
left a comment
stephentoub
left a comment
There was a problem hiding this comment.
The remaining runtime-wrapper execute-access issue is correctly addressed, and I found no new P0/P1/P2 concerns in the update. Approving the code; the current-head workflows still need to be authorized and pass before merge.
Generated by Copilot
Problem: The Rust runtime installer repeatedly loaded entire native files into memory for extraction and comparison, even when the runtime was already installed. The buffers were freed, but the allocator retained the memory after installation. Fixes #2675.
Fix: Stream extraction and compare existing files in fixed-size chunks. Stage only missing or changed files, validate the archive, then replace files atomically. Matching files are reused without requiring a writable cache.
Proof: Matched SDK-only measurements on macOS arm64, runtime 1.0.85, five alternating pairs per case:
All 68 installed files remain byte-identical. Regression tests cover corruption repair, archive integrity, concurrent installation and read-only caches. Native Windows/Linux validation remains outstanding.