Conversation
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.
done. |
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
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".
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".
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 |
5aad415 to
d71980e
Compare
In runV3 the memfile/rootfs body and header were uploaded by separate goroutines racing each other. PollRemoteStorageForHeader treats a header landing in storage as the durability signal for the whole layer, so if the header arrived first a child build could start deduping against a body that had not yet been written. Merge the body goroutine into the header goroutine for both memfile and rootfs so the invariant body fully uploaded before header is visible is enforced by construction rather than by luck. While here: - Return an explicit error when DiffHeader resolves to nil for a non-filesystem snapshot (previously a silent no-op that would leave the header missing on disk). - Add Info-level logging at header resolution and upload boundaries to make timing issues observable in production logs.
8a4cf57 to
52d2579
Compare
Problem
In
runV3, the memfile body and header were uploaded by separate racing goroutines:memfile.headeras soon asDiffHeaderresolvedPollRemoteStorageForHeadertreats a header appearing in storage as the durabilitysignal for the entire layer. When goroutine 1 won the race, child builds could start
deduplicating against a body that had not yet landed — causing data corruption or
"object does not exist" errors. The same race existed for rootfs body/header.
Root cause analysis
The original approach (
if h == nil { return error }) was dead code: as reviewer@jakubno correctly pointed out,
pauseProcessMemorycannot produce(nil, nil)fora full memory snapshot —
ToDiffHeaderalways returns a non-nil header or an error.The actual connected issue is the body-before-header race described above. A secondary
contributing factor was a
SetOnce.WaitWithContextnon-determinism bug (fixed in #3241,now included via rebase): when both the result channel and the cancelled context fired
simultaneously,
selectcould randomly pickctx.Err()instead of the already-setresult, causing intermittent upload failures that left
memfile.headerunwritten on disk.Fix
Body-before-header ordering (
build_upload_v3.go): Merge the memfile body andheader goroutines into one sequence: resolve header → upload body → write header.
Same ordering applied to rootfs.
Explicit error for nil header (
build_upload_v3.go): A nilDiffHeaderfor anon-filesystem snapshot is now an explicit error instead of a silent no-op.
Diagnostic logging (
build_upload_v3.go,sandbox.go): Info-level logs atheader resolution and upload boundaries for production observability.
Rebased on main: Includes the
SetOnce.WaitWithContextfix (fix(shared): make SetOnce.WaitWithContext deterministic once value is set #3241) and theprovisional header system (feat(orch): decouple warm resume from memfile dedup #3166).
Test plan
memfile.headerexists after buildFilesystemSnapshot == trueskips memfile)Closes #3226
/cc @jakubno @dobrac @ValentaTomas @arkamar @tvi Looking forward to your code review.