Skip to content

fix(orchestrator): make copy-build handle filesystem-only snapshots#3299

Open
arkamar wants to merge 1 commit into
mainfrom
fix/copy-build-fs-only
Open

fix(orchestrator): make copy-build handle filesystem-only snapshots#3299
arkamar wants to merge 1 commit into
mainfrom
fix/copy-build-fs-only

Conversation

@arkamar

@arkamar arkamar commented Jul 16, 2026

Copy link
Copy Markdown
Member

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.

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.
@cursor

cursor Bot commented Jul 16, 2026

Copy link
Copy Markdown

PR Summary

Low Risk
Narrow change to an orchestrator copy CLI using the existing metadata flag; full-snapshot copy behavior is unchanged aside from ordering metadata before memfile reads.

Overview
copy-build now loads build metadata up front and uses filesystem_only to decide whether to copy memory snapshot artifacts. Filesystem-only builds skip memfile headers/data and snapfile while still copying rootfs and metadata; full snapshots keep the previous behavior, so a missing memfile.header on a non–filesystem-only build remains a hard failure instead of being treated as filesystem-only.

Reviewed by Cursor Bugbot for commit a7ef94e. Bugbot is set up for automated code reviews on this repo. Configure here.

@codecov

codecov Bot commented Jul 16, 2026

Copy link
Copy Markdown

❌ 1 Tests Failed:

Tests completed Failed Passed Skipped
3333 1 3332 9
View the top 1 failed test(s) by shortest run time
github.com/e2b-dev/infra/tests/integration/internal/tests/envd::TestCommandKillNextApp
Stack Traces | 220s run time
=== RUN   TestCommandKillNextApp
=== PAUSE TestCommandKillNextApp
=== CONT  TestCommandKillNextApp
Executing command /bin/bash in sandbox icx50nbp5vcn6sdh82hcq
    process_test.go:30: Build failed: {<nil> An internal error occurred. Please try again or contact support with the build ID. <nil>}
--- FAIL: TestCommandKillNextApp (219.65s)

To view more test analytics, go to the Test Analytics Dashboard
📋 Got 3 mins? Take this short survey to help us improve Test Analytics.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread packages/orchestrator/cmd/copy-build/main.go
Comment thread packages/orchestrator/cmd/copy-build/main.go

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant