Move built-in runtime distribution to Cloudflare R2#72
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis pull request adds a runtime manifest and distribution system: prebuilt runtime artifacts (node22, node24, python3.13) via manifests and R2, a Dockerfile template and runtime manifest helpers, build/publish scripts for rootfs artifacts, install.sh support for manifest-driven and local builds, and documentation updates clarifying install behavior. Changes
Sequence DiagramsequenceDiagram
actor User
participant Installer as Install Script
participant Manifest as Manifest Service
participant R2 as R2 Storage
participant Docker as Docker Build
participant LocalFS as Local Filesystem
rect rgba(100, 150, 200, 0.5)
Note over User,LocalFS: Release Install Flow (Prebuilt Artifacts)
User->>Installer: Run install.sh
Installer->>Manifest: Fetch runtime manifest
Manifest-->>Installer: Runtime metadata (url, sha256, bytes, baseImage)
Installer->>R2: Download runtime artifact
R2-->>Installer: Rootfs artifact
Installer->>Installer: Validate checksum & size
Installer->>LocalFS: Store runtime artifact & metadata
Installer-->>User: Install complete
end
rect rgba(150, 200, 100, 0.5)
Note over User,LocalFS: Source Install Flow (Local Build)
User->>Installer: Run install.sh (source mode / FORCE_LOCAL_RUNTIME_BUILD)
Installer->>Docker: Invoke build-runtime-rootfs.sh
Docker->>Docker: Build image from Dockerfile.template
Docker->>Docker: Export filesystem, create ext4, compress
Docker-->>Installer: Built artifact + metadata.json
Installer->>Installer: Validate metadata (sha256, bytes)
Installer->>LocalFS: Store built artifact & metadata
Installer-->>User: Install complete
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
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.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
scripts/publish-runtimes-to-r2.sh (2)
34-42: JSON parsing with regex is fragile.The
json_string_fieldandjson_number_fieldfunctions usegrep -oPwhich can fail on:
- Values containing escaped quotes (
\")- Multi-line values
- Nested objects
This works for the current
metadata.jsonstructure (simple flat object, no special characters in values), but could break if the metadata format evolves. Consider usingjqwhich is commonly available:♻️ Suggested alternative using jq
+need_cmd jq json_string_field() { local key="$1" file="$2" - grep -oP "\"$key\"\\s*:\\s*\"\\K[^\"]+" "$file" 2>/dev/null | head -n1 || true + jq -r --arg k "$key" '.[$k] // empty' "$file" 2>/dev/null || true } json_number_field() { local key="$1" file="$2" - grep -oP "\"$key\"\\s*:\\s*\\K[0-9]+" "$file" 2>/dev/null | head -n1 || true + jq -r --arg k "$key" '.[$k] // empty' "$file" 2>/dev/null || true }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/publish-runtimes-to-r2.sh` around lines 34 - 42, The two helper functions json_string_field and json_number_field use fragile regex-based grep parsing that breaks on escaped quotes, multiline values, or nested objects; replace their implementation to parse metadata.json with jq (e.g., use jq -r ".${key}" for strings and jq -r ".${key}" | tonumber for numbers) to reliably extract fields, and add a graceful fallback to the existing grep approach or emit an error if jq is not installed; update the functions json_string_field and json_number_field to call jq on the provided file, handle missing keys by returning empty string (or nothing) consistent with current behavior, and ensure any shell-escaping around the key is handled (or require dot-notated keys) so callers continue to work.
110-112: Consider adding version format validation.The
--versionargument is used directly in R2 paths. While unlikely to be malicious in a maintainer script, validating the format (e.g., semver pattern) could prevent accidental issues with special characters in paths.♻️ Suggested validation
[ -n "$VERSION" ] || error "--version is required" +[[ "$VERSION" =~ ^[0-9]+\.[0-9]+\.[0-9]+(-[a-zA-Z0-9.]+)?$ ]] || error "Invalid version format: $VERSION (expected semver)" [ "$CHANNEL" = "stable" ] || error "Only --channel stable is supported in this milestone"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/publish-runtimes-to-r2.sh` around lines 110 - 112, Add a semantic-version format check for the VERSION variable before it gets used in R2 paths: validate $VERSION with a regex (e.g., optional leading "v" plus MAJOR.MINOR.PATCH and optional prerelease/build metadata) and call the existing error helper if it fails; update the validation block around the existing checks for VERSION, CHANNEL, and PROMOTE_STABLE so that the script rejects malformed versions early and only proceeds when VERSION matches the semver pattern.docker/runtimes/runtime-manifest.sh (1)
1-6: Consider documenting the cross-language duplication.The runtime manifest centralizes metadata for shell scripts, but
src/commands/create/types.ts(line 4) andsrc/commands/create/environment.ts(lines 47-51) independently define the same runtime names and filename mappings in TypeScript. If the manifest is updated, those TypeScript definitions must be updated manually.Consider adding a comment noting this relationship, or creating a code generation step that produces TypeScript constants from this manifest.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docker/runtimes/runtime-manifest.sh` around lines 1 - 6, Add a short comment above the RUNTIME_NAMES and RUNTIME_ARCHES constants in runtime-manifest.sh explaining these values are duplicated in src/commands/create/types.ts and src/commands/create/environment.ts (the TypeScript runtime name and filename mapping constants) and must be kept in sync, and either (preferred) add a small codegen step to emit TypeScript constants from this manifest (produce types.ts and environment.ts snippets) or (quick) document the duplication and where to update the TypeScript files; reference RUNTIME_NAMES/RUNTIME_ARCHES and the TypeScript files in the comment so future editors know the source of truth and how to regenerate/update the TS constants.scripts/build-runtime-rootfs.sh (1)
29-37: Consider handling additional JSON special characters injson_escape.The function handles backslash, double-quote, newline, carriage return, and tab. However, JSON also requires escaping control characters (U+0000 through U+001F). Characters like form feed (
\f) or other control codes in file paths or versions could produce invalid JSON.For the current use case (runtime names, versions, dates, SHA256 hashes), this is unlikely to be an issue, but consider using
jqif available for robust escaping.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/build-runtime-rootfs.sh` around lines 29 - 37, The json_escape function should also escape JSON control characters U+0000 through U+001F (e.g., backspace \b, form feed \f and any other non-printable control bytes) to avoid producing invalid JSON; update json_escape to replace \b and \f with \\b and \\f and to convert any remaining control bytes (byte values 0x00–0x1F) into \u00NN Unicode escapes, or, if jq is available, delegate escaping to jq for robustness (detect jq and use it as a safer fallback when building JSON strings).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@install.sh`:
- Around line 1064-1069: The install script currently fetches runtimes from
RUNTIME_MANIFEST_URL (stable.json) which can drift from the resolved release
(e.g., LATEST_TAG/npm@latest); update the logic around
RUNTIME_MANIFEST_URL/RUNTIME_MANIFEST_FILE and the install_runtime_from_manifest
loop to instead derive the manifest URL from the resolved release tag (or
download the manifest associated with the resolved release), then validate that
the downloaded manifest.version matches the release tag and abort with a clear
error if it does not; alternatively, if deriving the release-specific manifest
is not possible, fail the install when manifest.version != resolved release
(using the variables/functions RUNTIME_MANIFEST_URL, RUNTIME_MANIFEST_FILE,
LATEST_TAG, BUILTIN_RUNTIMES, and install_runtime_from_manifest) so runtimes are
pinned to the same version as the installed binaries.
- Around line 997-1002: The local-cache skip path for runtime reuse omits CPU
architecture, allowing amd64 builds to be reused on arm64; update the
runtime_metadata_matches checks (the if that currently passes runtime,
distribution local, artifact_version, recipe_version, base_image) to also
compare an arch key (e.g., arch "$arch" or arch "$(uname -m)"), and ensure the
code that writes/persists runtime metadata includes the same arch field so the
cache key records CPU architecture; apply the same change to the other identical
local-skip block that also calls runtime_metadata_matches.
- Around line 163-173: The download() helper currently creates tmp_dest with
mktemp (which defaults to /tmp) causing large downloads to fail on small tmpfs;
change tmp_dest to be created under the target directory by using the target
directory (dirname "$dest") as the temp file location before writing (e.g.,
create a temp file in $(dirname "$dest") or use mktemp -p "$(dirname "$dest")"),
keep the same error handling around curl and rm -f on failure, and then mv the
temp file into "$dest" as the final step—update references to tmp_dest in the
download() function accordingly.
---
Nitpick comments:
In `@docker/runtimes/runtime-manifest.sh`:
- Around line 1-6: Add a short comment above the RUNTIME_NAMES and
RUNTIME_ARCHES constants in runtime-manifest.sh explaining these values are
duplicated in src/commands/create/types.ts and
src/commands/create/environment.ts (the TypeScript runtime name and filename
mapping constants) and must be kept in sync, and either (preferred) add a small
codegen step to emit TypeScript constants from this manifest (produce types.ts
and environment.ts snippets) or (quick) document the duplication and where to
update the TypeScript files; reference RUNTIME_NAMES/RUNTIME_ARCHES and the
TypeScript files in the comment so future editors know the source of truth and
how to regenerate/update the TS constants.
In `@scripts/build-runtime-rootfs.sh`:
- Around line 29-37: The json_escape function should also escape JSON control
characters U+0000 through U+001F (e.g., backspace \b, form feed \f and any other
non-printable control bytes) to avoid producing invalid JSON; update json_escape
to replace \b and \f with \\b and \\f and to convert any remaining control bytes
(byte values 0x00–0x1F) into \u00NN Unicode escapes, or, if jq is available,
delegate escaping to jq for robustness (detect jq and use it as a safer fallback
when building JSON strings).
In `@scripts/publish-runtimes-to-r2.sh`:
- Around line 34-42: The two helper functions json_string_field and
json_number_field use fragile regex-based grep parsing that breaks on escaped
quotes, multiline values, or nested objects; replace their implementation to
parse metadata.json with jq (e.g., use jq -r ".${key}" for strings and jq -r
".${key}" | tonumber for numbers) to reliably extract fields, and add a graceful
fallback to the existing grep approach or emit an error if jq is not installed;
update the functions json_string_field and json_number_field to call jq on the
provided file, handle missing keys by returning empty string (or nothing)
consistent with current behavior, and ensure any shell-escaping around the key
is handled (or require dot-notated keys) so callers continue to work.
- Around line 110-112: Add a semantic-version format check for the VERSION
variable before it gets used in R2 paths: validate $VERSION with a regex (e.g.,
optional leading "v" plus MAJOR.MINOR.PATCH and optional prerelease/build
metadata) and call the existing error helper if it fails; update the validation
block around the existing checks for VERSION, CHANNEL, and PROMOTE_STABLE so
that the script rejects malformed versions early and only proceeds when VERSION
matches the semver pattern.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e9689045-9bad-44a4-accb-b31849720560
📒 Files selected for processing (11)
README.mddocker/runtimes/Dockerfile.templatedocker/runtimes/runtime-manifest.shdocs/content/1.getting-started/2.installation.mddocs/content/2.guide/4.docker-images.mddocs/maintainers/r2-runtime-publish.mdinstall.shscripts/build-runtime-rootfs.shscripts/publish-runtimes-to-r2.shsrc/commands/create/args.tssrc/commands/create/environment.ts
Comment on lines
163
to
173
| download() { | ||
| local url="$1" dest="$2" | ||
| local tmp_dest | ||
| tmp_dest="$(mktemp)" | ||
| info "Downloading $(basename "$dest")..." | ||
| curl -fsSL -o "$dest" "$url" | ||
| if ! curl -fsSL -o "$tmp_dest" "$url"; then | ||
| rm -f "$tmp_dest" | ||
| error "Failed to download $(basename "$dest") from $url" | ||
| fi | ||
| mv "$tmp_dest" "$dest" | ||
| } |
There was a problem hiding this comment.
Don't stage large artifacts in /tmp.
mktemp defaults to /tmp, so this path now requires /tmp to fit the Ubuntu rootfs and runtime artifacts. On many Linux hosts /tmp is a small tmpfs, which makes installs fail even when $VMSAN_DIR has plenty of space. Stage the temporary file under $(dirname "$dest") instead so the download lives on the target filesystem.
Suggested fix
download() {
local url="$1" dest="$2"
- local tmp_dest
- tmp_dest="$(mktemp)"
+ local dest_dir tmp_dest
+ dest_dir="$(dirname "$dest")"
+ mkdir -p "$dest_dir"
+ tmp_dest="$(mktemp "$dest_dir/.tmp.$(basename "$dest").XXXXXX")"
info "Downloading $(basename "$dest")..."
if ! curl -fsSL -o "$tmp_dest" "$url"; then
rm -f "$tmp_dest"
error "Failed to download $(basename "$dest") from $url"
fi
- mv "$tmp_dest" "$dest"
+ mv -f "$tmp_dest" "$dest"
}📝 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.
Suggested change
| download() { | |
| local url="$1" dest="$2" | |
| local tmp_dest | |
| tmp_dest="$(mktemp)" | |
| info "Downloading $(basename "$dest")..." | |
| curl -fsSL -o "$dest" "$url" | |
| if ! curl -fsSL -o "$tmp_dest" "$url"; then | |
| rm -f "$tmp_dest" | |
| error "Failed to download $(basename "$dest") from $url" | |
| fi | |
| mv "$tmp_dest" "$dest" | |
| } | |
| download() { | |
| local url="$1" dest="$2" | |
| local dest_dir tmp_dest | |
| dest_dir="$(dirname "$dest")" | |
| mkdir -p "$dest_dir" | |
| tmp_dest="$(mktemp "$dest_dir/.tmp.$(basename "$dest").XXXXXX")" | |
| info "Downloading $(basename "$dest")..." | |
| if ! curl -fsSL -o "$tmp_dest" "$url"; then | |
| rm -f "$tmp_dest" | |
| error "Failed to download $(basename "$dest") from $url" | |
| fi | |
| mv -f "$tmp_dest" "$dest" | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@install.sh` around lines 163 - 173, The download() helper currently creates
tmp_dest with mktemp (which defaults to /tmp) causing large downloads to fail on
small tmpfs; change tmp_dest to be created under the target directory by using
the target directory (dirname "$dest") as the temp file location before writing
(e.g., create a temp file in $(dirname "$dest") or use mktemp -p "$(dirname
"$dest")"), keep the same error handling around curl and rm -f on failure, and
then mv the temp file into "$dest" as the final step—update references to
tmp_dest in the download() function accordingly.
Comment on lines
+997
to
+1002
| if [ -f "$dest" ] && runtime_metadata_matches \ | ||
| "$runtime" \ | ||
| distribution local \ | ||
| artifact_version "$artifact_version" \ | ||
| recipe_version "$expected_recipe_version" \ | ||
| base_image "$base_image"; then |
There was a problem hiding this comment.
Persist the runtime arch in the local-build cache key.
The local skip path only matches distribution, artifact_version, recipe_version, and base_image. If ~/.vmsan is reused across hosts or restored onto the other CPU architecture, this can incorrectly keep an amd64 runtime on arm64 (or vice versa) because the filename is the same. Record arch in the metadata and compare it before reusing a local build.
Suggested fix
if [ -f "$dest" ] && runtime_metadata_matches \
"$runtime" \
distribution local \
+ arch "$arch" \
artifact_version "$artifact_version" \
recipe_version "$expected_recipe_version" \
base_image "$base_image"; then
RUNTIME_RECIPE_VERSION_INSTALLED="$expected_recipe_version"
success "Runtime ${runtime} already built locally"
@@
write_runtime_metadata \
"$runtime" \
distribution local \
+ arch "$arch" \
artifact_version "$artifact_version" \
recipe_version "$recipe_version" \
sha256 "$artifact_sha256" \Also applies to: 1037-1045
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@install.sh` around lines 997 - 1002, The local-cache skip path for runtime
reuse omits CPU architecture, allowing amd64 builds to be reused on arm64;
update the runtime_metadata_matches checks (the if that currently passes
runtime, distribution local, artifact_version, recipe_version, base_image) to
also compare an arch key (e.g., arch "$arch" or arch "$(uname -m)"), and ensure
the code that writes/persists runtime metadata includes the same arch field so
the cache key records CPU architecture; apply the same change to the other
identical local-skip block that also calls runtime_metadata_matches.
Comment on lines
+1064
to
+1069
| RUNTIME_MANIFEST_FILE="$(mktemp)" | ||
| info "Fetching runtime manifest from $RUNTIME_MANIFEST_URL..." | ||
| download "$RUNTIME_MANIFEST_URL" "$RUNTIME_MANIFEST_FILE" | ||
| for rt in "${BUILTIN_RUNTIMES[@]}"; do | ||
| install_runtime_from_manifest "$rt" "$HOST_RUNTIME_ARCH" "$RUNTIME_MANIFEST_FILE" | ||
| done |
There was a problem hiding this comment.
Pin release runtimes to the same version as the installed binaries.
Release installs resolve LATEST_TAG for the agent/nftables and npm@latest for the CLI, but Line 1065 still pulls runtimes from the moving stable.json channel. The new maintainer flow explicitly allows publishing a release with --promote-stable false, so this can install newer binaries alongside older runtimes during rollout. Either derive the manifest URL from the resolved release or fail if manifest.version does not match the release being installed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@install.sh` around lines 1064 - 1069, The install script currently fetches
runtimes from RUNTIME_MANIFEST_URL (stable.json) which can drift from the
resolved release (e.g., LATEST_TAG/npm@latest); update the logic around
RUNTIME_MANIFEST_URL/RUNTIME_MANIFEST_FILE and the install_runtime_from_manifest
loop to instead derive the manifest URL from the resolved release tag (or
download the manifest associated with the resolved release), then validate that
the downloaded manifest.version matches the release tag and abort with a clear
error if it does not; alternatively, if deriving the release-specific manifest
is not possible, fail the install when manifest.version != resolved release
(using the variables/functions RUNTIME_MANIFEST_URL, RUNTIME_MANIFEST_FILE,
LATEST_TAG, BUILTIN_RUNTIMES, and install_runtime_from_manifest) so runtimes are
pinned to the same version as the installed binaries.
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
1 participant
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.
Summary
artifacts.vmsan.dev--from-imageseparate and Docker-backedTesting
bash -n install.shbash -n scripts/build-runtime-rootfs.shbash -n scripts/publish-runtimes-to-r2.shbun run testbun run typecheckroot@...withVMSAN_DIR=/root/.vmsan-r2testdockerwas absent on the server before and after installinstall.metaand runtime.metafiles showeddistribution=r2andrecipe_version=3VMSAN_DIR=/root/.vmsan-r2test vmsan doctorVMSAN_DIR=/root/.vmsan-r2test vmsan create --runtime node22 --memory 512 --vcpus 1 --jsonSummary by CodeRabbit
New Features
Documentation
Bug Fixes