fix(orchestrator): make copy-build handle filesystem-only snapshots#3299
fix(orchestrator): make copy-build handle filesystem-only snapshots#3299arkamar wants to merge 1 commit into
Conversation
copy-build unconditionally read memfile.header, so copying a filesystem-only snapshot (which uploads no memory snapshot: no memfile.header, no memfile data, no snapfile) died with: failed to create header from object: ... failed to create reader for "<build-id>/memfile.header": object does not exist Read metadata.json first and branch on its filesystem_only flag - the resume path's source of truth - instead of probing for memfile.header. This keeps a missing memfile.header on a full snapshot a hard error (incomplete build) rather than silently degrading it to an fs-only copy, and skips memfile and snapfile only for genuine fs-only builds. Rootfs artifacts and metadata.json are copied as before.
PR SummaryLow Risk Overview Reviewed by Cursor Bugbot for commit a7ef94e. Bugbot is set up for automated code reviews on this repo. Configure here. |
❌ 1 Tests Failed:
View the top 1 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
There was a problem hiding this comment.
Code Review
This pull request updates the build copying process in packages/orchestrator/cmd/copy-build/main.go to conditionally skip copying the memfile and snapfile if the build is a filesystem-only snapshot, determined by loading the build metadata early in the execution. Feedback suggests simplifying the metadata loading logic to eliminate redundant variables and unify error handling. Additionally, the reviewer notes that loading this metadata early makes a later block of code that re-reads the same metadata file redundant, presenting an opportunity to clean up unnecessary I/O.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
There was a problem hiding this comment.
LGTM — straightforward fix that reorders logic to check filesystem_only from metadata.json before deciding whether to read memfile/snapfile artifacts, correctly using the existing tested metadata.IsFilesystemOnly() helper.
What was reviewed: the branching logic for filesystem-only vs full snapshots, confirmed metadata.FromBuildID/FromFile/IsFilesystemOnly are existing, tested functions used correctly, and confirmed rootfs/metadata copying is unchanged. One nit-level issue was found (inline comment) around cross-compilation of this dev CLI on non-Linux hosts, but it doesn't affect the core fix or any CI/production path.
Extended reasoning...
Overview
Single-file change to packages/orchestrator/cmd/copy-build/main.go, a standalone dev/self-host CLI tool for copying template builds between storage backends. The fix reorders the copy logic to read metadata.json first and branch on FilesystemOnly, skipping memfile/snapfile reads for filesystem-only snapshots (which never uploaded those artifacts) while preserving the existing hard-failure behavior for genuine full snapshots missing a memfile header. Rootfs and metadata copying logic is unchanged aside from being pulled out of the removed inline snapfile-append block.
Security risks
None. This is an offline CLI operator tool that reads/writes GCS objects or local files based on operator-supplied flags; no new attack surface, auth, or trust boundary changes are introduced.
Level of scrutiny
Low-to-moderate is appropriate: this is a narrow, mechanical fix to non-production-critical tooling (not part of the sandbox creation/runtime hot path), confined to one file, and the new logic correctly reuses existing, unit-tested metadata helpers (IsFilesystemOnly, FromBuildID, FromFile) rather than introducing new parsing logic.
Other factors
The bug-hunting pass surfaced one real but nit-severity regression: importing pkg/template/metadata (which carries a transitive //go:build linux constraint) breaks cross-compilation of this CLI for non-Linux GOOS targets. This doesn't affect any CI or production build path (all of which target Linux) and is confined to a niche, unautomated local dev-tooling scenario, so it doesn't rise to blocking severity for this PR.
| "github.com/google/uuid" | ||
| "golang.org/x/sync/errgroup" | ||
|
|
||
| "github.com/e2b-dev/infra/packages/orchestrator/pkg/template/metadata" |
There was a problem hiding this comment.
🟡 This PR adds an import of pkg/template/metadata, but every file in that package carries a //go:build linux constraint, so copy-build (which itself has no build tag) no longer cross-compiles: GOOS=darwin GOARCH=arm64 go build ./cmd/copy-build/... fails with "build constraints exclude all Go files in .../pkg/template/metadata", whereas it built fine before this PR. This silently breaks building/running this documented CLI from a non-linux dev machine (e.g. a self-hoster or engineer on macOS); the -team flag path already shows the fix in-file — parse metadata.json via a plain inline struct instead of importing the metadata package.
Extended reasoning...
The bug: copy-build/main.go now imports github.com/e2b-dev/infra/packages/orchestrator/pkg/template/metadata to read the filesystem_only flag from a build's metadata. Every file in that package — template_metadata.go, prefetch.go, and both test files — carries a //go:build linux constraint (the tag is transitive: prefetch.go imports pkg/sandbox/block, which is linux-only, and it shares the package with the otherwise-portable JSON-parsing code in template_metadata.go). copy-build/main.go itself carries no build tag, so the Go toolchain now excludes the entire metadata package for any non-linux GOOS, and the build fails outright.\n\nProof: On the PR branch, GOOS=darwin GOARCH=arm64 go build ./cmd/copy-build/... fails with:\n\nChecking out the parent commit (before this PR) and running the identical command succeeds (exit 0). GOOS=linux go build ./cmd/copy-build/... succeeds on both, confirming the regression is specific to cross-compilation, not a general breakage.\n\nWhy existing code didn't prevent this: Before this PR, copy-build only imported packages/shared/pkg/storage, packages/shared/pkg/storage/header, and packages/shared/pkg/id — none of which are linux-constrained — so the CLI was genuinely portable. There is no CI job or Makefile target that builds copy-build for multiple platforms, so nothing caught this at compile time; it only surfaces when a developer actually tries to build/run the tool on a non-linux host.\n\nImpact: copy-build is documented in packages/orchestrator/README.md as a plain go run ./cmd/copy-build -build <uuid> -from ... -to gs://bucket utility with no sudo requirement, unlike the Firecracker/VM tools that inherently need linux. It shells out to rsync/gcloud and has no intrinsic OS dependency, so it's plausible a self-hoster or engineer on macOS/Windows uses it to shuffle template builds between GCS buckets or local paths. That workflow now hard-fails at compile time with an unhelpful "build constraints exclude all Go files" error rather than a clear message about the actual problem (a linux-only dependency).\n\nFix: The file already demonstrates the right pattern for this exact data — the -team flag path near the end of main() decodes metadata.json into a small inline anonymous struct (KernelVersion/FirecrackerVersion) rather than using the metadata package. The same approach works for filesystem_only: read and unmarshal the relevant field directly (or extract just the metadata-JSON-reading logic in pkg/template/metadata into a build-tag-free file/package), avoiding the transitive dependency on the linux-only prefetch.go.\n\nSeverity: All three independent verifiers confirmed the regression is real and reproducible, but split on severity (two called it nit, one called it a borderline nit/normal). None of E2B's CI or deployment pipelines build copy-build for non-linux targets — production and CI always target GOOS=linux — so nothing in an automated or production path breaks. The impact is confined to a niche, unautomated dev-tooling workflow that is trivially worked around by building on a linux host or in a container. Given that and the absence of any evidence this specific cross-platform usage is currently relied upon, I'm filing this as nit: a real, worth-fixing regression, but not one that should block this PR.
copy-build unconditionally read memfile.header, so copying a filesystem-only snapshot (which uploads no memory snapshot: no memfile.header, no memfile data, no snapfile) died with:
failed to create header from object: ... failed to create reader for "/memfile.header": object does not exist
Read metadata.json first and branch on its filesystem_only flag - the resume path's source of truth - instead of probing for memfile.header. This keeps a missing memfile.header on a full snapshot a hard error (incomplete build) rather than silently degrading it to an fs-only copy, and skips memfile and snapfile only for genuine fs-only builds. Rootfs artifacts and metadata.json are copied as before.