feat: TargetArch() utility and arch-aware path resolution#2258
Conversation
- Add TargetArch() to shared/utils that reads TARGET_ARCH env var with
alias normalization (x86_64→amd64, aarch64→arm64), defaulting to
runtime.GOARCH
- Update Firecracker and kernel path resolution to prefer arch-prefixed
layout ({version}/{arch}/binary) with legacy flat fallback
- Change OCI DefaultPlatform from hardcoded amd64 var to function using
TargetArch()
- Add comprehensive tests for path resolution and TargetArch
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
PR SummaryMedium Risk Overview Written by Cursor Bugbot for commit e7543ce. This will update automatically on new commits. Configure here. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 20a4968e92
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
- Use sync.Once for unrecognized TARGET_ARCH warning to prevent stderr flood on repeated calls - Only fall back to legacy path on os.ErrNotExist, not on permission errors or other os.Stat failures Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
|
||
| // TargetArch returns the target architecture for binary paths and OCI platform. | ||
| // If TARGET_ARCH is set, it is normalized to Go convention ("amd64" or "arm64"); | ||
| // otherwise defaults to the host architecture (runtime.GOARCH). | ||
| func TargetArch() string { | ||
| if arch := os.Getenv("TARGET_ARCH"); arch != "" { | ||
| if normalized, ok := archAliases[arch]; ok { | ||
| return normalized | ||
| } | ||
|
|
||
| archWarningOnce.Do(func() { | ||
| fmt.Fprintf(os.Stderr, "WARNING: unrecognized TARGET_ARCH=%q, falling back to %s\n", arch, runtime.GOARCH) | ||
| }) |
There was a problem hiding this comment.
🟡 The archAliases map lookup in TargetArch() is case-sensitive, so TARGET_ARCH=AMD64 or TARGET_ARCH=ARM64 (uppercase, common in some CI/CD environments) fails the lookup, emits a misleading warning, and returns runtime.GOARCH instead of the intended architecture. On a cross-compilation host (e.g., arm64 node with TARGET_ARCH=AMD64), this silently selects the wrong architecture for kernel paths and OCI platform. Fix: normalize with strings.ToLower(arch) before the map lookup in env.go:25.
Extended reasoning...
What the bug is and how it manifests
The TargetArch() function in packages/shared/pkg/utils/env.go reads the TARGET_ARCH environment variable and normalizes common aliases (e.g., x86_64->amd64, aarch64->arm64) via the archAliases map. However, all map keys are lowercase, so any uppercase or mixed-case value (e.g., AMD64, ARM64, X86_64) will fail the lookup.
The specific code path that triggers it
In env.go around line 25:
if normalized, ok := archAliases[arch]; ok {
return normalized
}
// falls through to warning + runtime.GOARCH fallback
fmt.Fprintf(os.Stderr, "WARNING: unrecognized TARGET_ARCH=%q, falling back to %s\n", arch, runtime.GOARCH)
return runtime.GOARCH
When arch is "AMD64", the map lookup misses (key "AMD64" does not exist; only "amd64" does), the warning is printed, and runtime.GOARCH (the host arch) is returned.
Why existing code does not prevent it
The fix needs to be applied before the map lookup: archAliases[strings.ToLower(arch)]. The strings package is already imported in this file (used by RequiredEnv/OptionalEnv), so this is a trivial one-line change.
Impact
On a cross-compilation host -- for example, an arm64 CI runner with TARGET_ARCH=AMD64 to build amd64 binaries -- the function returns "arm64" instead of "amd64". This propagates to: (1) the Firecracker binary path (FirecrackerPath builds an arch-prefixed path using TargetArch()), (2) the kernel path (HostKernelPath), and (3) the OCI DefaultPlatform(). The operator does see a warning, so this is not completely silent -- but a WARNING to stderr is easy to overlook in noisy CI logs, and the downstream behavior (wrong arch binary, wrong OCI platform) is incorrect.
Step-by-step proof
- CI runner is arm64; operator sets TARGET_ARCH=AMD64 (uppercase, intending amd64 cross-compile).
- TargetArch() calls os.Getenv("TARGET_ARCH") and gets "AMD64".
- archAliases["AMD64"] returns ok == false (map has only "amd64").
- Function emits: WARNING: unrecognized TARGET_ARCH="AMD64", falling back to arm64.
- Returns "arm64" (runtime.GOARCH on the arm64 runner) -- the wrong value.
- DefaultPlatform() returns {OS: "linux", Architecture: "arm64"} -- the wrong OCI platform.
- FirecrackerPath checks {version}/arm64/firecracker and falls back to the legacy flat path, potentially using the arm64 binary when amd64 was intended.
How to fix it
Change env.go line 25 from:
if normalized, ok := archAliases[arch]; ok {
to:
if normalized, ok := archAliases[strings.ToLower(arch)]; ok {
This makes TargetArch() case-insensitive and robustly handles the full set of real-world CI inputs.
There was a problem hiding this comment.
Won't fix — Go/Docker/Debian arch conventions are always lowercase (amd64, arm64). Adding strings.ToLower would mask actual typos rather than surfacing them via the warning.
jakubno
left a comment
There was a problem hiding this comment.
IMHO no need to fallback as we have them already long enough
All active Firecracker and kernel versions have been migrated to the
arch-prefixed layout ({version}/{arch}/binary). Remove the fallback to
the legacy flat path ({version}/binary) so that a missing arch directory
fails loudly instead of silently using a potentially wrong-arch binary.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@jakubno agreed, removed all fallback code from the PR |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
jakubno
left a comment
There was a problem hiding this comment.
I found out later that we're missing it for some firecracker versions (v1.10), so we need the fallback. I'm sorry, my overlook
This reverts commit 52e9646.
Syncs all application code from e2b-dev/infra upstream including: - Redis PubSub for state transitions (e2b-dev#2099) - Pluggable egress firewall (e2b-dev#2187) - Firecracker v1.12 upgrade (e2b-dev#2245) - Label-based sandbox scheduler (e2b-dev#2066) - Orchestrator internal/ -> pkg/ migration - Pre-compute cgroup CPU deltas (e2b-dev#2265) - Arch-aware downloads (e2b-dev#2258, e2b-dev#2260) - Customizable pre-warmed NBDs (e2b-dev#2266) - Autoresume improvements (e2b-dev#1969, e2b-dev#2196) - Many bug fixes for race conditions, eviction, error handling Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Summary
TargetArch()helper inpackages/shared/pkg/utils/env.gothat readsTARGET_ARCHenv var with alias normalization (x86_64→amd64,aarch64→arm64), defaulting toruntime.GOARCH{version}/{arch}/binary) with a legacy flat-directory fallbackDefaultPlatformfrom a hardcodedamd64variable to a function backed byTargetArch()Part of #1875 (ARM64 support split). This is the foundation PR that subsequent PRs for runtime guards and create-build depend on.
Test plan
go test ./packages/shared/pkg/utils/...passes (TargetArch unit tests)go test ./packages/orchestrator/pkg/sandbox/fc/...passes (config path resolution tests)go test ./packages/orchestrator/pkg/template/build/core/oci/...passes (OCI DefaultPlatform tests)🤖 Generated with Claude Code