fix(orchestrator): fail builds loudly when memfile.header cannot be written#3251
fix(orchestrator): fail builds loudly when memfile.header cannot be written#3251AdaAibaby wants to merge 4 commits into
Conversation
…ritten memfile.header is written by a goroutine that waits on the DiffHeader SetOnce future. The future can only be nil for filesystem-only snapshots (NoDiff); for full memory snapshots it must always resolve to a real header. If it resolves nil for a non-filesystem-only snapshot the header file is silently skipped and the template is permanently unresumable after its in-memory cache entry is evicted (the reload path falls back to a wrong 2 MiB block size, causing VM restore failures with offset-beyond-end-of-mapping errors). Add an explicit check in runV3 and runV4 so such a build fails loudly with a clear error instead of silently producing a corrupted template. Also upgrade the legacy-fallback log in NewStorage from Debug to Warn so operators can see when the headerless code path is hit; this is symptomatic of the upload bug described in issue e2b-dev#3226 and lets users identify affected templates without waiting for VM restore failures.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds validation checks in both build_upload_v3.go and build_upload_v4.go to return an error if a memfile diff header resolves to nil for non-filesystem-only snapshots, and updates a log level from Debug to Warn in storage.go for legacy headerless fallbacks. The reviewer noted a potential nil pointer dereference panic in other goroutines that wait on these headers and call u.layerSizeMetadata(h) without checking if h is nil, suggesting that similar nil checks should be added there.
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.
- Revert legacy-fallback log level from Warn back to Debug; this code path is expected for old templates and should not page operators - Shorten the log message to match the original terse style - Use errors.New instead of fmt.Errorf for the static error string in runV3 and runV4 (fixes perfsprint lint)
done. |
jakubno
left a comment
jakubno
left a comment
There was a problem hiding this comment.
Checking the code it seems full-snapshot code cannot produce (nil, nil)
pauseProcessMemory resolves DiffHeader as either:
non-nil header, nil error
nil header, non-nil error
This doesn't fix the connected issue
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f557af2271
ℹ️ 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".
jakubno's review correctly identified that the (nil, nil) guard added in the previous commit is dead code: pauseProcessMemory's goroutine always resolves DiffHeader to either (non-nil header, nil error) via ToDiffHeader or (nil, non-nil error) on failure, never (nil, nil) for full-memory snapshots. The real failure mode in issue e2b-dev#3226 was that two independent goroutines uploaded the memfile header and body concurrently. If the body goroutine completed and the header goroutine later failed, the errgroup error triggered cleanup, but a transient cleanup failure left the body in storage without a header. Loading that template after cache eviction produces the synthetic V1 fallback with Size=physicalMemfileSize instead of the virtual RAM size, causing the "mappingEnd vs offset" crash. Fix: merge the per-file header and body goroutines in runV3 into one, writing the header first. If the header write fails the body is never uploaded (clean state). If the body upload fails after a successful header write, cleanup removes an easily-detectable orphaned header rather than a silent orphaned body. runV4's uploadFramed already handles both header and body atomically in a single call (body first is required there since the V4 header includes the body's frame table). Remove the dead guard from its memfile goroutine too. Fixes e2b-dev#3226
AdaAibaby
force-pushed
the
fix/memfile-header-missing-local-storage
branch
from
f557af2 to
b21997a
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b21997a995
ℹ️ 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".
… memfile header - Merge memfile header+body into one goroutine per file type; upload the body first, then write the header to storage. PollRemoteStorageForHeader on peer orchestrators treats the header's appearance as the durability signal for the whole layer, so writing the header first (as in b21997a) lets child builds proceed against a body that has not yet landed. - Return an explicit error when MemorySnapshot.DiffHeader resolves to nil for a non-filesystem snapshot. Previously the header goroutine returned nil silently (if h == nil { return nil }), allowing the body to land in storage without a header and causing the mis-sizing logged in e2b-dev#3226. Filesystem-only snapshots still return nil (intentional NoDiff path). - Add Info-level logs at the two key points in the header-resolution pipeline: when pauseProcessMemory's goroutine calls setHeader (sandbox.go) and when runV3's goroutine receives the resolved value (build_upload_v3.go). Together these logs make the root-cause observable on the next reproduction without requiring a core dump or additional instrumentation.
Agreed — (nil, nil) should be analytically unreachable for full-memory snapshots, Updated in 8a4cf57: the nil case now returns an explicit error for non-filesystem Also added Info-level logs at both ends of the resolution chain |
4 participants
Problem
Issue #3226: user-built templates in Local storage mode never have
memfile.headerwritten to disk. The template works within the first ~3 minutes (served from in-memory cache), but once the cache TTL expires and the template is reloaded from disk,NewStoragefalls back to a fake header witholdMemfileHugePageSize = 2 MiBblock size. This wrong block size causes VM restores to fail with "offset is beyond the end of mapping, but normalize fix is not applied" warnings and ultimately "object does not exist" errors on snapfile access.Root cause (symptomatic)
runV3/runV4goroutine 1 writesmemfile.header. It first waits onMemorySnapshot.DiffHeader(aSetOncefuture). If that future resolves to(nil, nil), the goroutine silently returns nil —memfile.headeris never written, the upload is reported as successful, and the template is permanently broken after cache eviction.A nil
DiffHeaderis only valid for filesystem-only snapshots (which useNewResolvedDiffHeader(nil)). For full memory snapshots it must always resolve to a real header — a nil result indicates a silent failure in the background header goroutine (pauseProcessMemory).Fix
runV3andrunV4: Add an explicit guard beforeif h == nil { return nil }— for non-filesystem-only snapshots, a nilDiffHeaderis now an upload error. Builds fail immediately with a clear message instead of producing a silently corrupted template.NewStoragefallback: Upgrade the log fromDebugtoWarnwhen the headerless legacy fallback is used. This makes the symptom visible in operator logs and helps identify templates affected by the upload bug.Why not a full root-cause fix?
The exact trigger for
DiffHeaderresolving to nil on a full memory snapshot is not yet identified from static analysis alone. The changes in this PR make the failure loud and observable (build error + warning log) rather than silent and delayed (~3 min), enabling faster diagnosis. Closes #3226.Test plan
memfile.headeris present inLOCAL_TEMPLATE_STORAGE_BASE_PATH/{buildId}/after successful buildFilesystemSnapshot == true)