Export rootfs in the background during pause()/snapshot() - #3320
Conversation
PR SummaryMedium Risk Overview Reviewed by Cursor Bugbot for commit bec1b51. Bugbot is set up for automated code reviews on this repo. Configure here. |
❌ 8 Tests Failed:
View the top 3 failed test(s) by shortest run time
View the full list of 5 ❄️ flaky test(s)
To view more test analytics, go to the Test Analytics Dashboard |
![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 introduces a deferred rootfs export mechanism to move the rootfs diff sealing (reflink copy) off the critical path of the sandbox pause operation, allowing it to return without waiting for the writeback stall. The reviewer feedback focuses on improving robustness and error handling: specifically, implementing a fallback to synchronous export when the rootfs provider (such as DirectProvider) does not support deferred export, defining a sentinel error ErrDeferredExportNotSupported to cleanly trigger this fallback, and preventing a potential resource leak of sealCache if the pause operation fails before the background export starts.
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.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
05b2204 to
44d32a5
Compare
44d32a5 to
05b2204
Compare
|
Thanks for the reviews. Status on each finding: Addressed in the current revision
Fixed in the incoming revision
Intended tradeoff — proposing a follow-up rather than blocking here
|
|
Two more from the latest Cursor pass — both valid, both fixed in the incoming revision:
Build + vet + race tests green. |
05b2204 to
54615d0
Compare
|
Two more from the re-review of the pushed fixes — both addressed in the incoming revision:
Build + vet + race tests (server + sandbox + build) all green. |
54615d0 to
d82260a
Compare
|
Good catch — and this one was introduced by the V3 half of my upload reorder, so it's on me. V3 uploads rootfs header before seal (Cursor high): correct. In Fixed by gating the V3 rootfs header goroutine on the seal as well ( |
d82260a to
e0229fd
Compare
|
Peers see unsealed rootfs as ready (Cursor medium): correct that, with the P2P-chunk-transfer flag and deferred export both on,
The clean fix is to advertise to peers only once the rootfs is sealed. I'm deferring it to a focused follow-up rather than bolting it on here, because doing it correctly has to preserve the |
5aad415 to
d71980e
Compare
c978d51 to
f04a401
Compare
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
de0c279 to
5afb878
Compare
| return s.failRootfsSeal(diffPromise, fmt.Errorf("create rootfs diff file: %w", err)) | ||
| } | ||
|
|
||
| if _, err := sealCache.ExportToDiff(ctx, diffFile.File); err != nil { |
There was a problem hiding this comment.
claude is telling me that DiffMetadata and ExportToDiff both read the tracker to get the bitmap, but at different times, and there is no structural guarantee that they are exactly the same. Can we pass the metadata in here?
func (c *Cache) ExportToDiffWithMetadata(ctx context.Context, out *os.File, meta *header.DiffMetadata) error| // kinds) so the fs-only pause latency can be decomposed into quiesce + rootfs. | ||
| // This is the pause CRITICAL-PATH rootfs cost: the full export for the | ||
| // synchronous path, but only the eject/setup for the deferred path — the | ||
| // background reflink seal (runDeferredRootfsExport) is intentionally excluded. |
There was a problem hiding this comment.
it looks like with excluding the reflink seal, we lose visibility on the reflink latency. Shall we add a new metric for that?
There was a problem hiding this comment.
also that way we should be able to detect errors on the async path, otherwise they're silent?
There was a problem hiding this comment.
Added a metric specifically for the sealing part.
| return c.closed.Load() | ||
| } | ||
|
|
||
| // DiffMetadata returns the dirty/empty diff metadata from the tracker without |
There was a problem hiding this comment.
PR summary: we should make it clearer that the latency isn't eliminated in the case of immediate resume - it migrates to the pause.
There was a problem hiding this comment.
updated the PR summary.
| // Close waits for the seal to resolve and closes the materialized diff. If the | ||
| // seal failed there is nothing to close (the producer cleans up the partial file | ||
| // on error), so only close when the diff actually materialized. | ||
| func (d *deferredDiff) Close() error { |
There was a problem hiding this comment.
this seems to be only called on disk pressure-caused chunk eviction, but isn't on the TTL-caused eviction. Doesn't look material with 25h TTL, but becomes a problem if we choose to shorten TTL for some reason.
There was a problem hiding this comment.
added logic to close an unsealed deferredDiff in a go routine (without cancel) in case of eviction.
There was a problem hiding this comment.
rootfs copy is synchronous today is no longer true.
|
|
||
| // With deferred rootfs export the just-paused snapshot's rootfs diff is | ||
| // sealed (reflinked) in the background, and the throwaway warm resume below | ||
| // reads the rootfs. Wait for the seal to finish here instead of letting the |
There was a problem hiding this comment.
can we not count it into the harvestDurationHistogram and add record this wait as a separate metric?
| // diff in the background, so the pause returns without paying the reflink stall. | ||
| // Only safe on the suspend path, where nothing reads the diff before the seal | ||
| // completes. | ||
| func (s *Sandbox) setupDeferredRootfsExport( |
There was a problem hiding this comment.
would it make sense to add tests to cover failure branches here?
There was a problem hiding this comment.
Added failure branch tests covering the eject-failure early return.
| // TestDeferredDiff_SealedIsNonBlocking verifies sealed() reports readiness | ||
| // without blocking on the promise, so cache eviction can skip an unsealed diff | ||
| // instead of stalling on it. | ||
| func TestDeferredDiff_SealedIsNonBlocking(t *testing.T) { |
There was a problem hiding this comment.
can we have a test for the fix in the commit, ie the diff is skipped when unsealed?
There was a problem hiding this comment.
added a test that ensures that the diff is skipped when unsealed and then later evicted after it was sealed.
| if err := s.sealCacheToDiff(ctx, sealCache, buildID, blockSize, diffPromise); err != nil { | ||
| logger.L().Error(ctx, "deferred rootfs export failed", zap.Error(err)) | ||
| } else { | ||
| telemetry.ReportEvent(ctx, "rootfs diff sealed (deferred)") |
There was a problem hiding this comment.
claude says we don't report 'exported rootfs' event on the deferred path.
There was a problem hiding this comment.
adding missing telemetry events
5afb878 to
48f42c3
Compare
| // The provider (e.g. DirectProvider) can't defer; fall through to the | ||
| // synchronous export below. Safe because PrepareExportDiff returns this | ||
| // sentinel before ejecting/stopping anything. | ||
| pauseOpts.deferRootfsExport = false |
There was a problem hiding this comment.
claude is saying we can never read this false, including the histogram function above (processRootfsDurationHistogram) where it could be helpful.
There was a problem hiding this comment.
like deferRootfsExport could be an attribute to that metric
| // when ejecting the writable cache fails, setup propagates the error and returns | ||
| // no diff/header/seal — so Pause falls back to the synchronous export rather than | ||
| // registering a deferred diff whose seal would never run. | ||
| func TestSetupDeferredRootfsExport_PrepareError(t *testing.T) { |
There was a problem hiding this comment.
claude thinks it would also be nice to test the call order of the cleanups in setupDeferredRootfsExport in the case of a later failure to avoid lockups.
e458828 to
455008e
Compare
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.
Reviewed by Cursor Bugbot for commit 0d28de6. Configure here.
Add the building blocks to seal the rootfs diff off the pause critical path: - block.Cache.DiffMetadata() returns the dirty/empty bitmaps from the frozen tracker without copying, so the diff header + scheduling metadata can be built synchronously while the reflink copy runs in the background. - rootfs.Provider.PrepareExportDiff ejects the writable cache, stops the sandbox and waits for the overlay device to release, then returns the frozen ejected cache WITHOUT reflinking it (shares ejectAndStopSandbox with ExportDiff). DirectProvider stubs it (NBD-only). - build.NewDeferredDiff wraps a promise of a Diff: CacheKey/BlockSize are synchronous (DiffStore.Add + upload compress-config need them up front), while data methods block on the promise until the background sealer resolves it. No behavior change yet — nothing calls these. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Babis Chalios <babis.chalios@e2b.dev>
0d28de6 to
b9742e0
Compare
This comment has been minimized.
This comment has been minimized.
![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: b9742e09ed
ℹ️ 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".
| diff, err := diffFile.CloseToDiff(blockSize) | ||
| if err != nil { | ||
| return s.failRootfsSeal(diffPromise, fmt.Errorf("materialize rootfs diff: %w", err)) |
There was a problem hiding this comment.
When deferred rootfs export reaches this path and CloseToDiff fails after the large diff has already been written (for example an fsync/stat error), the partial cache file is only closed and is never removed or registered with the diff store. Because the upload error is then marked as a permanent deferred-seal failure, repeated failures can leave orphaned rootfs diff files in DefaultCacheDir that disk-pressure eviction cannot reclaim until process restart.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — fixed in d545ded. The leak was in CloseToDiff: on a Sync/Stat/materialization failure it only closed the fd (defer f.File.Close()) without os.Remove, so the partial file orphaned in DefaultCacheDir with nothing registering it in the DiffStore (eviction can't reclaim a file it doesn't know about). Fixed at the CloseToDiff layer so every caller — the deferred seal, the synchronous pause path, and ensurefreedisk/grow — removes its partial file on failure. Added TestLocalDiffFileCloseToDiffRemovesPartialOnError.
| // DeferRootfsExportFlag moves the rootfs diff seal (the reflink, which forces a | ||
| // synchronous host->NVMe writeback) off the pause critical path: pause() ejects | ||
| // the cache and stops the sandbox, then reflinks the diff in the background so | ||
| // the call returns without the writeback stall. Applied only to the suspend | ||
| // (pause) path, where nothing reads the diff until a later resume. Off by | ||
| // default; falls back to the synchronous export when off or on a non-NBD | ||
| // provider. | ||
| DeferRootfsExportFlag = NewBoolFlag("defer-rootfs-export", false) |
There was a problem hiding this comment.
This flag adds a pause mode where the orchestrator returns after ejecting/stopping the sandbox and seals the rootfs diff in the background, so resumes/prefetch may wait on that seal; docs/ARCHITECTURE.md still describes pause as synchronously diffing rootfs before the snapshot is cached/uploaded. Please update the pause/resume flow documentation with this new deferred-seal behavior so operators do not rely on the old ordering.
AGENTS.md reference: AGENTS.md:L7-L9
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Done in d545ded — added a Deferred rootfs export sub-bullet to the Pause section of docs/ARCHITECTURE.md. It documents the flag-gated behavior: the orchestrator ejects the writable COW cache during pause and returns, then seals it into the rootfs diff (reflink) in the background — so the async upload and any origin-node resume/prefetch that reads the rootfs diff wait on the seal, and a seal failure is permanent (the upload fails fast rather than retrying).
Sealing the rootfs diff is a reflink (copy_file_range on XFS) that forces a synchronous host->NVMe writeback of the changed blocks — the stall that makes pause() scale with how much the guest wrote. Move it off the critical path for the suspend path: eject the cache + stop the sandbox, hand back a deferred diff, and reflink it in the background, so pause() returns without the writeback. - WithDeferredRootfsExport pause option + setupDeferredRootfsExport / runDeferredRootfsExport / sealCacheToDiff in Pause: build the header synchronously from the frozen cache's DiffMetadata, resolve the deferred diff from a background reflink. The async upload waits on the deferred diff, so graceful shutdown is gated via the server's upload WaitGroup. - Gated by the defer-rootfs-export flag (default off), and only the Pause (suspend) handler opts in — Checkpoint resumes a fresh sandbox from the new build immediately, so its diff must be materialized synchronously. - Falls back to the synchronous export when off or on a non-NBD provider. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Babis Chalios <babis.chalios@e2b.dev>
The single disk-space eviction goroutine calls FileSize on the oldest evictable cache entry; for a still-sealing deferred rootfs diff that blocks on the background seal (WaitWithContext with the eviction loop's long-lived ctx), stalling all reclamation during exactly the high-disk-pressure window eviction exists to relieve. Add a non-blocking deferredDiff.sealed() check (via the SetOnce Done channel) and skip not-yet-sealed diffs in eviction, like pinned entries — so eviction neither blocks on the seal nor evicts a fresh, in-flight snapshot; the diff becomes evictable once its seal resolves. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Babis Chalios <babis.chalios@e2b.dev>
b9742e0 to
bec1b51
Compare
kalyazin
left a comment
kalyazin
left a comment
There was a problem hiding this comment.
LGTM
The only thing is claude flags the following as a minor finding, and I can't see how realistic it is. Up to you if you find it worth fixing.
Seal goroutine isn't on a shutdown WaitGroup — after drainUploads times out the process can still exit mid-copy_file_range. Now bounded in blast radius by
the CloseToDiff cleanup for in-process failures, but an exit-time partial still orphans.
Adds the overlay primitives the in-place background rootfs seal needs, on top of main's #3320 deferred-export code: - Overlay: atomic writable-cache pointer + a "sealing" slot. SwapCache installs a fresh writable cache and moves the previous one to the sealing slot; ReadAt resolves writable -> sealing -> base. FoldSealing / ReleaseSealing collapse the sealing cache back once its background seal is done. - Cache.FillMissingFrom copies the sealing cache's blocks the writable cache lacks (concurrency-safe writeAtIfAbsent keeps the guest's newer writes), so the writable cache is a complete diff again after a fold. Cache.DiffMetadata already exists on main (from #3320) and is reused. Purely additive; overlay.go was untouched by #3320. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Babis Chalios <babis.chalios@e2b.dev>
Adds the NBD provider capabilities the in-place background seal needs, alongside main's #3320 PrepareExportDiff (destroy-path eject+stop): - ExportDiffInPlace: reflink the overlay's dirty blocks without ejecting the cache, so the sandbox keeps running on it (synchronous in-place fallback). - SwapForBackgroundSeal: flush the device, swap a fresh writable cache onto the live overlay, return the frozen previous cache for background sealing. - FoldSealed: fold the sealing cache back into the writable cache and detach it. DirectProvider stubs all three (NBD-only). Reuses main's ejectAndStopSandbox. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Babis Chalios <babis.chalios@e2b.dev>
Adds the overlay primitives the in-place background rootfs seal needs, on top of main's #3320 deferred-export code: - Overlay: atomic writable-cache pointer + a "sealing" slot. SwapCache installs a fresh writable cache and moves the previous one to the sealing slot; ReadAt resolves writable -> sealing -> base. FoldSealing / ReleaseSealing collapse the sealing cache back once its background seal is done. - Cache.FillMissingFrom copies the sealing cache's blocks the writable cache lacks (concurrency-safe writeAtIfAbsent keeps the guest's newer writes), so the writable cache is a complete diff again after a fold. Cache.DiffMetadata already exists on main (from #3320) and is reused. Purely additive; overlay.go was untouched by #3320. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Babis Chalios <babis.chalios@e2b.dev>
Adds the NBD provider capabilities the in-place background seal needs, alongside main's #3320 PrepareExportDiff (destroy-path eject+stop): - ExportDiffInPlace: reflink the overlay's dirty blocks without ejecting the cache, so the sandbox keeps running on it (synchronous in-place fallback). - SwapForBackgroundSeal: flush the device, swap a fresh writable cache onto the live overlay, return the frozen previous cache for background sealing. - FoldSealed: fold the sealing cache back into the writable cache and detach it. DirectProvider stubs all three (NBD-only). Reuses main's ejectAndStopSandbox. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Babis Chalios <babis.chalios@e2b.dev>
## What When we are snapshotting a sandbox, during pause()/snapshot() we create a snapshot of both memory and rootfs. Exporting the rootfs boils down to reflinking the overlay to the local cache path and then (asynchronously) uploading the cache to GCS. This exporting process happens synchronously. We have seen that, _typically_ reflinking is very fast as it deals with metadata (rather than moving data around). This PR makes the export asynchronous (optional, guarded by a feature flag). ## Why We have seen that, whereas the reflink operation is typically fast, it can experience long tails when there's a lot of dirty data in the page cache that needs to be written to the backing storage. For example, when we are taking a snapshot after writing 2GiB of data in the disk immediately after the write finished (E1) and 30 seconds after the write had completed and: * E1 — pause immediately after write: the ~2 GB NVMe flush burst happens inside the pause window (host Dirty ~2 GB → 0 during the pause). → pause = 6.8 s * E2 — 30 s settle before pause: the flush burst happens during the settle; Dirty is already 0 when the pause fires. → pause = 0.9 s * Same write, same node — only difference is whether the flush overlaps the pause. This nicely scales with amount of time we wait after the write: * settle 0 s → ~4.2 s (6.8 s tail) * settle 5 s → 3.78 s * settle 10 s → 3.71 s * settle 20 s → 1.53 s * settle 30 s → ~0.7–0.9 s --------- Signed-off-by: Babis Chalios <babis.chalios@e2b.dev> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Adds the overlay primitives the in-place background rootfs seal needs, on top of main's #3320 deferred-export code: - Overlay: atomic writable-cache pointer + a "sealing" slot. SwapCache installs a fresh writable cache and moves the previous one to the sealing slot; ReadAt resolves writable -> sealing -> base. FoldSealing / ReleaseSealing collapse the sealing cache back once its background seal is done. - Cache.FillMissingFrom copies the sealing cache's blocks the writable cache lacks (concurrency-safe writeAtIfAbsent keeps the guest's newer writes), so the writable cache is a complete diff again after a fold. Cache.DiffMetadata already exists on main (from #3320) and is reused. Purely additive; overlay.go was untouched by #3320. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Babis Chalios <babis.chalios@e2b.dev>
Adds the NBD provider capabilities the in-place background seal needs, alongside main's #3320 PrepareExportDiff (destroy-path eject+stop): - ExportDiffInPlace: reflink the overlay's dirty blocks without ejecting the cache, so the sandbox keeps running on it (synchronous in-place fallback). - SwapForBackgroundSeal: flush the device, swap a fresh writable cache onto the live overlay, return the frozen previous cache for background sealing. - FoldSealed: fold the sealing cache back into the writable cache and detach it. DirectProvider stubs all three (NBD-only). Reuses main's ejectAndStopSandbox. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Babis Chalios <babis.chalios@e2b.dev>
2 participants
What
When we are snapshotting a sandbox, during pause()/snapshot() we create a snapshot of both memory and rootfs.
Exporting the rootfs boils down to reflinking the overlay to the local cache path and then (asynchronously) uploading the cache to GCS.
This exporting process happens synchronously. We have seen that, typically reflinking is very fast as it deals with metadata (rather than moving data around).
This PR makes the export asynchronous (optional, guarded by a feature flag). In the case of asynchronous export of the rootfs, we don't observe the reflink latency in the pause, but we might observe it during resume time if the resume happens while the asynchronous seal operation is still ongoing.
Why
We have seen that, whereas the reflink operation is typically fast, it can experience long tails when there's a lot of dirty data in the page cache that needs to be written to the backing storage.
For example, when we are taking a snapshot after writing 2GiB of data in the disk immediately after the write finished (E1) and 30 seconds after the write had completed and:
This nicely scales with amount of time we wait after the write: