Skip to content

feat(orch): last-cycle memory prefetch on resume#3258

Open
kalyazin wants to merge 5 commits into
mainfrom
kalyazin/resume-fulllife-prefetch
Open

feat(orch): last-cycle memory prefetch on resume#3258
kalyazin wants to merge 5 commits into
mainfrom
kalyazin/resume-fulllife-prefetch

Conversation

@kalyazin

@kalyazin kalyazin commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

Why

Every prefetch mechanism we have today accelerates only the resume → envd-init prefix of a sandbox's life:

  • Build-time optimize intersects two finalize resumes but runs only to WaitForEnvd.
  • The throwaway pause-resume harvest resumes a network-isolated copy that has no workload to replay, so it structurally cannot see the post-init working set.

So the workload's hot pages are never prefetched, and every resume of a paused sandbox demand-faults them cold from object storage through UFFD — the dominant resume-latency / time-to-responsive cost. Meanwhile the live sandbox's PrefetchTracker accumulates the whole resume→pause cycle's fault set for free and it is discarded when the uffd is torn down.

This PR replays the last cycle's working set on the next resume. Those pages are already recorded: they are the snapshot's own pause diff — the merged memfile.header BuildMap entries whose BuildId is the header's own build ID — so the set is derived from the header at resume with no pause-time capture, no new metadata, and no extra I/O. Off by default and strictly additive.

What

Last-cycle prefetch on resume, selected by a flag

buildDiffMemoryPrefetchMapping(header) builds the mapping from the snapshot's own-BuildId blocks — deduped by block index (a merged/dedup header repeats an offset across layers) and offset-sorted — i.e. the last cycle's writes, read from the memfile header at resume. ResumeSandbox picks what to replay via the new resume-prefetch-source flag (selectResumePrefetch): init (default, today's behavior), last-cycle, both, or off.

Fetch-only replay, behind a barrier

The prefetcher gains an exported Prefault field (default true, so existing callers are unchanged); last-cycle sets it false, warming the shared chunk cache and letting the guest fault the now-warm pages instead of UFFDIO_COPY-ing them in. When an init trace is also present it runs first (prefaulted) and its Start() blocks on the fetch+copy waitgroups, so last-cycle begins only afterward.

Contiguous-fetch coalescing

coalesceIndices merges runs of contiguous block indices into one larger source.Slice, capped by the new memory-prefetch-coalesce-max-mb int flag (read at Start() like the existing worker-count flags; default 0 = off). The copy phase stays strictly per-page — a coalesced extent is split back into page-sized sub-slices before Prefault, since UFFDIO_COPY requires page-sized data. The flag is read for every prefetcher, but only the offset-sorted last-cycle mapping coalesces meaningfully; the fault-ordered init trace stays one extent per block (coalesceIndices never reorders).

Volume cap

resume-last-cycle-prefetch-max-mib (default -1 = uncapped) bounds how much of the last-cycle diff a resume prefetches; capResumePrefetch keeps the first N MiB of blocks in offset order and leaves the rest to demand-fault.

Observability (for rollout)

Two signals ship with the feature so a cohort can be measured without a follow-up:

  • orchestrator.sandbox.uffd.prefetch.mapping_blocks — a histogram of the replayed mapping size in blocks, tagged mode=prefault (init trace) vs mode=fetch (last-cycle diff). The fetch series is the recorded-working-set distribution that sizes the rollout; its bottom bucket flags idle-at-pause sandboxes. A live metric, so it's dashboard-able without Tempo sampling.
  • Resume span attributes resume.prefetch.source / resume.prefetch.init_blocks / resume.prefetch.last_cycle_blocks — cohort a resume by the chosen source (guards against flag misconfiguration) and see the per-resume set sizes.

Existing prefetcher metrics (…prefetch.pages by stage, …prefetch.duration by phase, and the per-page …prefault metric) already cover fetched/copied/skipped counts and how late the prefetch ran.

New feature flags (all default to today's behavior):

Flag Type Default Effect
resume-prefetch-source enum off|init|last-cycle|both init which trace resume replays; init is a no-op-equivalent
memory-prefetch-coalesce-max-mb int (MiB) 0 coalesce contiguous last-cycle blocks into ≤N MiB fetches; 0 = off
resume-last-cycle-prefetch-max-mib int (MiB) -1 cap last-cycle replay volume; -1 = uncapped

Key decisions

  • Last-cycle set is header-derived, not captured at pause. The pause diff already encodes the last cycle's writes, so it is read from the memfile header at resume — no Prefetch.LastCycle metadata field and no pause-path work. Persisting an explicit trace (and its harvest flag) is deferred as unnecessary.
  • Last-cycle replays fetch-only, not prefault. Prefaulting the multi-GiB diff costs ~2.5 s of cold resume for no workload gain and regresses warm resumes; fetch-only warms the cache and lets the guest fault.
  • Init and last-cycle are sequenced behind a barrier, not unioned. Init (prefaulted) lands first and last-cycle follows it, keeping the big last-cycle fetch off the resume-critical path; a concurrent union raised resume latency for no workload gain.
  • Coalescing is opt-in (default off). The measured win saturates by ~8 MiB, so a small cap captures it; larger caps buy nothing.
  • The cap applies to last-cycle only (the init trace is tiny); default uncapped.
  • Backwards-compatible by construction: default resume-prefetch-source=init is today's behavior, and prefetch.New's signature is unchanged (fetch-only is opted into via a new field defaulting to prefault). No Firecracker-version coupling — capture and replay are orchestrator-side and reuse the in-prod prefetcher.

Validation

  • Unit: go test ./pkg/sandbox/ ./pkg/sandbox/uffd/prefetch/ passes — table-driven tests for the mapping builder, source selection, cap, and coalescing; a test that a coalesced multi-block fetch still yields one page-sized copy per block; and a Start()-level test that a fetch-only run never calls Prefault while still fetching every block. Each commit builds + tests standalone; golangci-lint clean.
  • Dev cluster, cold GCS resume, 8 GiB template (workload reads a 3.5 GiB working set that would otherwise demand-fault cold; mem_seq on the first cold resume is the reliable metric):
    • no workload prefetch ≈ 173 s; last-cycle ≈ 14.9 s (~12×); with coalescing 7.65 s (~24×). Coalescing sweep (off→8→16→32 MiB): 14.9 → 6.9 → 7.7 → 6.1 s — the win is the off→on step and saturates by ~8 MiB.
    • recommended shape (init prefaulted + last-cycle fetch-only + coalesced, behind the barrier): ~3.5 s resume / ~9 s workload — resume matches the init-only floor (~2.3 s vs ~7.2 s with no prefetch) while the workload is ~19× faster.
    • fetch-only vs prefault (both init-prefaulted): fetch-only 3462 ms / 9.23 s vs prefault 6014 ms / 9.06 s — prefaulting costs ~2.5 s of resume for no workload gain.
    • warm resume (hot cache): proposed config 648 ms / 2.20 s vs init-only baseline 763 ms / 2.29 s — neutral, no regression.
  • Smoke (dev cluster): create → pause → resume → exec, both with the default flag and with source=last-cycle forced — both pass (resume healthy, marker preserved).
  • Volume rationale: last-cycle prefetches only the last cycle's writes; a production dirty-set analysis puts the median at ~15× fewer bytes than prefetching all resident memory.

🤖 Generated with Claude Code

@cla-bot cla-bot Bot added the cla-signed label Jul 10, 2026
@cursor

cursor Bot commented Jul 10, 2026

Copy link
Copy Markdown

PR Summary

Medium Risk
Changes the resume prefetch path and can issue large background memfile reads from object storage when flags are enabled; defaults and explicit caps mitigate rollout risk, and teardown cancellation reduces leak risk on sandbox stop.

Overview
Paused sandboxes can warm the last resume→pause working set on the next resume by deriving block indices from the snapshot’s own memfile BuildMap (no new pause metadata), gated by resume-prefetch-source (init default, last-cycle, both, off). ResumeSandbox may run init prefetch (prefault, as today) then last-cycle prefetch (Prefault = false, cache-only) in sequence, registers an early priority cancel so teardown does not keep pulling multi‑GiB diffs, and records span attributes plus a mapping_blocks histogram by mode. The prefetcher adds optional contiguous fetch coalescing (memory-prefetch-coalesce-max-mb) and resume-last-cycle-prefetch-max-mib truncation, with guards for short reads and base-template vs layered snapshots.

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

@codecov

codecov Bot commented Jul 10, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 55.60538% with 99 lines in your changes missing coverage. Please review.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
packages/orchestrator/pkg/sandbox/sandbox.go 0.00% 72 Missing ⚠️
...chestrator/pkg/sandbox/uffd/prefetch/prefetcher.go 73.56% 18 Missing and 5 partials ⚠️
...ckages/orchestrator/pkg/sandbox/resume_prefetch.go 93.75% 2 Missing and 2 partials ⚠️

📢 Thoughts on this report? Let us know!

@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 introduces resume prefetching capabilities for sandboxes, allowing the prefetching of either the build-time init trace, the full-life diff (pages written during the last resume-pause cycle), or both. It adds support for coalescing contiguous blocks into larger extents to optimize reads and introduces a fetch-only mode to warm the cache without blocking the critical path. The review feedback highlights three key areas for improvement: addressing a potential resource leak in fetch-only mode by using a cancelable context tied to sandbox cleanup, preventing a runtime panic by verifying the length of the slice returned from the source, and avoiding potential integer overflow by using uint64 arithmetic instead of casting offsets to int64.

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 on lines +821 to +843
telemetry.ReportEvent(ctx, "starting prefetcher")
l := logger.L().With(logger.WithSandboxID(runtime.SandboxID), logger.WithTemplateID(runtime.TemplateID), logger.WithTeamID(runtime.TeamID))

go func() {
// Init trace first, prefaulted (prod behavior). Start blocks until
// its fetch+copy complete, so it acts as a barrier before the
// full-life fetch begins.
if initMapping != nil {
p := prefetch.New(l, memfile, fcUffd, initMapping, f.featureFlags)
if err := p.Start(execCtx); err != nil {
l.Error(ctx, "failed to start init prefetcher", zap.Error(err))
}
}

// Full-life diff, fetch-only.
if fullLifeMapping != nil {
p := prefetch.New(l, memfile, fcUffd, fullLifeMapping, f.featureFlags)
p.Prefault = false
if err := p.Start(execCtx); err != nil {
l.Error(ctx, "failed to start full-life prefetcher", zap.Error(err))
}
}
}()

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.

high

The background prefetcher is started using execCtx, which is a non-cancelable context (context.WithoutCancel). If p.Prefault is false (fetch-only mode), there are no copy workers to detect userfaultfd.ErrClosed and trigger cancelRun(). Consequently, if the sandbox is stopped or destroyed while prefetching is active, the fetch workers will continue fetching all remaining extents from object storage to completion, causing a resource and cloud I/O leak. To prevent this, create a cancelable context derived from execCtx and register its cancellation with the sandbox's cleanup manager.

		telemetry.ReportEvent(ctx, "starting prefetcher")
		l := logger.L().With(logger.WithSandboxID(runtime.SandboxID), logger.WithTemplateID(runtime.TemplateID), logger.WithTeamID(runtime.TeamID))

		prefetchCtx, cancelPrefetch := context.WithCancel(execCtx)
		cleanup.Add(ctx, func(ctx context.Context) error {
			cancelPrefetch()
			return nil
		})

		go func() {
			// Init trace first, prefaulted (prod behavior). Start blocks until
			// its fetch+copy complete, so it acts as a barrier before the
			// full-life fetch begins.
			if initMapping != nil {
				p := prefetch.New(l, memfile, fcUffd, initMapping, f.featureFlags)
				if err := p.Start(prefetchCtx); err != nil {
					l.Error(ctx, "failed to start init prefetcher", zap.Error(err))
				}
			}

			// Full-life diff, fetch-only.
			if fullLifeMapping != nil {
				p := prefetch.New(l, memfile, fcUffd, fullLifeMapping, f.featureFlags)
				p.Prefault = false
				if err := p.Start(prefetchCtx); err != nil {
					l.Error(ctx, "failed to start full-life prefetcher", zap.Error(err))
				}
			}
		}()

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good catch — fixed. execCtx is indeed non-cancelable (context.WithoutCancel), and the fetch-only full-life path has no copy worker to observe the uffd close and fire cancelRun. Derived a cancelable prefetchCtx from execCtx and registered its cancel with the cleanup manager (which runs on teardown), so stopping the sandbox mid-fetch now aborts the in-flight fetch instead of draining the whole diff. Done in the full-life commit (ffbd17384).

Comment on lines +316 to +328
data, err := p.source.Slice(ctx, baseOffset, blockSize*int64(e.blocks))
if err != nil {
p.logger.Debug(ctx, "prefetch: failed to fetch page",
zap.Int64("offset", offset),
p.logger.Debug(ctx, "prefetch: failed to fetch extent",
zap.Int64("offset", baseOffset),
zap.Int("blocks", e.blocks),
zap.Error(err),
)
skippedCount.Add(1)
skippedCount.Add(uint64(e.blocks))

continue
}

fetchedCount.Add(1)
fetchedCount.Add(uint64(e.blocks))

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.

high

If p.source.Slice returns a slice that is shorter than the requested blockSize * int64(e.blocks) (e.g., due to a partial read or truncated file), slicing data inside the loop at line 338 will cause a runtime panic (slice bounds out of range), crashing the entire orchestrator process. To ensure robustness, add a check to verify that the returned slice has at least the expected length before proceeding.

			data, err := p.source.Slice(ctx, baseOffset, blockSize*int64(e.blocks))
			if err != nil {
				p.logger.Debug(ctx, "prefetch: failed to fetch extent",
					zap.Int64("offset", baseOffset),
					zap.Int("blocks", e.blocks),
					zap.Error(err),
				)
				skippedCount.Add(uint64(e.blocks))

				continue
			}

			if int64(len(data)) < blockSize*int64(e.blocks) {
				p.logger.Debug(ctx, "prefetch: short read from source",
					zap.Int64("offset", baseOffset),
					zap.Int("expected_blocks", e.blocks),
					zap.Int("actual_bytes", len(data)),
				)
				skippedCount.Add(uint64(e.blocks))

				continue
			}

			fetchedCount.Add(uint64(e.blocks))

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed — added a guard before the per-page split: if len(data) < blockSize*e.blocks (short read with a nil error), log and skip the extent instead of slicing, so a truncated read can no longer panic the process. Added a regression test (TestFetchWorkerShortReadDoesNotPanic). In ffbd17384 (fix) + the test commit.

Comment on lines +95 to +97
for off := int64(bm.Offset); off < int64(bm.Offset+bm.Length); off += blockSize {
seen[uint64(off/blockSize)] = struct{}{}
}

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.

medium

Casting bm.Offset and bm.Length from uint64 to int64 can lead to integer overflow if the offset exceeds math.MaxInt64. Since all offsets and block sizes are logically non-negative and represented as uint64 in the mapping, it is safer and cleaner to perform the loop using uint64 arithmetic.

		uBlockSize := uint64(blockSize)
		for off := bm.Offset; off < bm.Offset+bm.Length; off += uBlockSize {
			seen[off/uBlockSize] = struct{}{}
		}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done — the loop now uses uint64 (uBlockSize), dropping the int64 casts and matching the uint64 map key. In practice offsets are memfile-scale so the overflow was not reachable, but the uint64 form is cleaner. In ffbd17384.

@linear-code

linear-code Bot commented Jul 10, 2026

Copy link
Copy Markdown

EN-1676

@kalyazin kalyazin force-pushed the kalyazin/resume-fulllife-prefetch branch from 9492a80 to b0b2bcc Compare July 10, 2026 20:07
@kalyazin kalyazin marked this pull request as ready for review July 10, 2026 20:36
Comment on lines +96 to +99

for off := bm.Offset; off < bm.Offset+bm.Length; off += uBlockSize {
seen[off/uBlockSize] = struct{}{}
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔴 The full-life prefetch mapping's inner loop walks each own-build BuildMap entry by += uBlockSize from the (possibly unaligned) bm.Offset, so a merged/dedup entry whose Offset isn't aligned to Metadata.BlockSize AND extends into another block only records its first block — the tail block is silently missed and will demand-fault cold on resume, defeating full-life coverage for that fraction of the diff (reachable with HugePages + memfile-diff-dedup on and resume-prefetch-source=fulllife/both). Fix by iterating by block index the same way template/header_metrics.go's maxEntriesPerBlock already does: firstBlock := bm.Offset/uBlockSize; lastBlock := (bm.Offset+bm.Length-1)/uBlockSize; for idx := firstBlock; idx <= lastBlock; idx++ { seen[idx] = struct{}{} }.

Extended reasoning...

The bug

buildDiffMemoryPrefetchMapping (packages/orchestrator/pkg/sandbox/resume_prefetch.go:97-99) enumerates the blocks a BuildMap entry covers by walking bytes from bm.Offset in steps of uBlockSize:

for off := bm.Offset; off < bm.Offset+bm.Length; off += uBlockSize {
    seen[off/uBlockSize] = struct{}{}
}

If bm.Offset is not aligned to uBlockSize and the entry extends into the next block, only floor(Length / uBlockSize) + 1 steps fire — landing uBlockSize apart from the unaligned start — so a block whose byte range is covered but not fully spanned by one uBlockSize step is skipped.

Reachability (the trigger chain)

All three conditions are reachable via existing feature-flag rollouts on this PR:

  1. Metadata.BlockSize = HugepageSize (2 MiB). With config.HugePages on (config/config.go MemfilePageSize(true) = HugepageSize), NewEmpty stores this into Metadata.BlockSize, and Metadata.NextGeneration preserves it across pauses.
  2. Own-build entries can be PageSize (4 KiB) aligned, not HugepageSize aligned. When MemfileDiffDedupFlag.enabled=true, block/memfd.go:304 sets DiffMetadata.BlockSize = header.PageSize; CreateMapping (mapping.go) then emits entries at PageSize. Mapping.Validate is explicitly called with PageSize (metadata.go:176, // Dedup emits PageSize-granular mappings; validate at PageSize.), so PageSize-aligned entries in a HugepageSize header are valid.
  3. NormalizeMappings merges adjacent same-build entries without any block-size alignment constraint, so a merged own-build entry can straddle a HugepageSize boundary.

Step-by-step proof

Take dirty pages 511 and 512 (page 512 * 4096 = 2097152 = HugepageSize, so this run straddles the block 0/1 boundary). CreateMapping emits one entry {Offset: 511*4096=2093056, Length: 2*4096=8192, BuildId: own}, and the loop with uBlockSize=2097152 does:

  • iter 1: off = 2093056; 2093056 < 2101248 → true; seen[2093056/2097152] = seen[0].
  • step: off += 2097152 → 4190208.
  • iter 2 check: 4190208 < 2101248? false → exit.

Only seen[0] is added. Block 1 (bytes 2097152–2101247), which the entry does cover (4 KiB of dirty data lives there), is silently missed. It will demand-fault cold on resume, defeating full-life prefetch for that block.

Why existing safeguards don't prevent it

The codebase already knows this pattern — packages/orchestrator/pkg/sandbox/template/header_metrics.go:78-79 (maxEntriesPerBlock) walks the same merged mapping and uses:

start := int64(bm.Offset) / header.HugepageSize
end   := int64(bm.Offset+bm.Length-1) / header.HugepageSize

with an explicit end > start branch for runs spilling past their first block. Every other consumer that iterates the merged mapping at HugepageSize granularity uses this form. The new buildDiffMemoryPrefetchMapping is the only walker that makes the (broken) assumption that own-build entries are block-aligned. Mapping.Validate only enforces PageSize alignment for merged headers, so the header itself is valid — the new loop's assumption is what breaks.

Impact

No crash, no data corruption, no correctness-of-guest-state issue: the guest still faults the missed blocks and reads them cold from object storage on demand. But the whole point of buildDiffMemoryPrefetchMapping is to enumerate every block the last cycle wrote so the full-life prefetcher can warm them; silently dropping the tail of every cross-boundary run partially defeats the feature under the intended HugePages + memfile-diff-dedup + resume-prefetch-source=fulllife/both combination — which is a plausible near-term rollout combination.

Fix

Iterate by block index instead of by unaligned byte offset — the one-line pattern already used in header_metrics.go:

firstBlock := bm.Offset / uBlockSize
lastBlock  := (bm.Offset + bm.Length - 1) / uBlockSize
for idx := firstBlock; idx <= lastBlock; idx++ {
    seen[idx] = struct{}{}
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed. buildDiffMemoryPrefetchMapping now enumerates by block index instead of byte-stepping from a possibly-unaligned bm.OffsetfirstBlock = Offset/blockSize, lastBlock = (Offset+Length-1)/blockSize, iterate inclusive — so a merged/dedup entry that straddles a block boundary contributes every block it covers, not just the first. (Also guards Length == 0.) Added a regression test (TestBuildDiffMemoryPrefetchMappingUnalignedEntry) with the exact 2 MiB-block / 4 KiB-aligned straddling case. Folded into the last-cycle feat commit (7bef957). Thanks for the detailed repro.

kalyazin added 2 commits July 10, 2026 22:38
Restructure the memory prefetcher's fetch loop around an extent{startIdx,
blocks} unit instead of a bare block offset. Every extent queued today
still covers exactly one block, so this is a pure structural change with
no behavior difference: fetchWorker issues the same one-block
source.Slice call and queues the same single page for copy.

This sets up the fetch path to coalesce contiguous block indices into
larger sequential reads (a following commit), while keeping the copy
phase strictly per-page, since UFFDIO_COPY requires page-sized data.

Signed-off-by: Nikita Kalyazin <nikita.kalyazin@e2b.dev>
Merge runs of contiguous block indices into a single, larger
source.Slice fetch (coalesceIndices), instead of issuing one fetch per
block. Fewer, larger sequential reads can beat many small reads spread
across parallel fetch workers, at the cost of losing per-block fetch
granularity when one block in the run is slow or fails.

The extent size is controlled by the new memory-prefetch-coalesce-max-mb
LaunchDarkly int flag (MemoryPrefetchCoalesceMaxMB), read the same way
the existing fetch/copy worker-count flags are: at Start() time, so no
call site wiring is needed. Default is 0 (off), reproducing today's
one-block-per-fetch behavior exactly.

The copy phase is untouched and stays strictly per-page: a coalesced
extent is split back into page-sized sub-slices before being queued for
copy, because Userfaultfd.Prefault (UFFDIO_COPY) rejects any data whose
length isn't exactly one page.

Signed-off-by: Nikita Kalyazin <nikita.kalyazin@e2b.dev>
@kalyazin kalyazin force-pushed the kalyazin/resume-fulllife-prefetch branch from b0b2bcc to d754afc Compare July 10, 2026 21:39
@kalyazin kalyazin changed the title feat(orch): full-life memory prefetch on resume feat(orch): last-cycle memory prefetch on resume Jul 10, 2026

@chatgpt-codex-connector chatgpt-codex-connector 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.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d754afc743

ℹ️ 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".

l := logger.L().With(logger.WithSandboxID(runtime.SandboxID), logger.WithTemplateID(runtime.TemplateID), logger.WithTeamID(runtime.TeamID))
var lastCycleMapping *metadata.MemoryPrefetchMapping
if useLastCycle {
lastCycleMapping = buildDiffMemoryPrefetchMapping(memfile.Header())

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Avoid treating base templates as last-cycle diffs

When resume-prefetch-source includes last-cycle for a normal sandbox create/build resume, this calls the diff mapper on a base template header as well as on paused snapshots. Base headers created without an explicit diff map the entire memfile to their own BuildId, so buildDiffMemoryPrefetchMapping will return every memory block and the new fetch-only path will read the whole template image from storage instead of only a paused snapshot's last diff. This makes enabling the flag globally turn regular starts into multi-GiB prefetches.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good catch — fixed. buildDiffMemoryPrefetchMapping now returns nil unless the header has a distinct base layer (some block owned by a BuildId other than the snapshot's own, ignoring uuid.Nil holes). A base/build template maps its whole memfile to its own BuildId → no base layer → skipped, so a create/build resume with source=last-cycle/both no longer prefetches the entire template image; only a genuine pause diff (layered on its parent) is treated as last-cycle. Added a full image with no base layer … returns nil test. In 28f7f1a.

// the whole (potentially multi-GiB) diff from object storage for a dead
// sandbox. Cancel via the cleanup manager so teardown aborts it.
prefetchCtx, cancelPrefetch := context.WithCancel(execCtx)
cleanup.Add(ctx, func(context.Context) error {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Register prefetch cancellation before it can race cleanup

Because this cleanup callback is registered from the background prefetch goroutine, it can race with Sandbox.Close or the error cleanup path. Cleanup.Add checks hasRun before taking its mutex, so if Run starts between that check and the append, this cancel callback is appended after cleanup has drained and never runs; in that teardown window the fetch-only last-cycle prefetch can continue draining a large diff for a dead sandbox.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed. Moved the context.WithCancel(execCtx) + cleanup.Add(cancel) registration out of the background goroutine to the synchronous ResumeSandbox body, before the prefetch goroutine is started — so the cancel is registered before any teardown (Close / deferred error cleanup) can run, closing the Cleanup.Add hasRun-check-then-append race. The goroutine just captures the already-registered prefetchCtx. In 28f7f1a.

@kalyazin kalyazin force-pushed the kalyazin/resume-fulllife-prefetch branch from d754afc to 7ca1476 Compare July 10, 2026 22:05

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

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Want fixes drafted automatically? Bugbot Autofix can create code changes for findings. A team admin can enable Autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 7ca1476. Configure here.

Comment thread packages/orchestrator/pkg/sandbox/sandbox.go
kalyazin added 3 commits July 13, 2026 19:35
Pause/resume carries no prefetch mapping today: SameVersionTemplate drops the
build-time one (create-from-template / checkpoint read-hot trace) on a resumed
snapshot's metadata, so every resume demand-faults its working set cold from
object storage.

The pages a resume->pause cycle writes are already recorded: they ARE the
pause diff, present in the merged memfile header as the BuildMap entries whose
BuildId equals the header's own build ID. This "last-cycle" working set -- a
good predictor of what the next cycle touches -- needs no separate
dirty-bitmap capture at pause. buildDiffMemoryPrefetchMapping derives it from
the header at resume, enumerating own-BuildId blocks by block index (dedup
emits PageSize-granular entries that can straddle a block boundary, so it
walks first..last block rather than byte-stepping from a possibly-unaligned
offset), deduped and offset-sorted.

Replay it FETCH-ONLY, not prefault: the prefetcher gains a Prefault field
(default true, so existing callers are unchanged) that, when false, warms the
shared chunk cache and lets the guest fault the now-warm pages itself.
Prefaulting the multi-GiB diff loads that much UFFDIO_COPY onto the
resume-critical path (~+2.5 s resume) for no workload gain and regresses warm
resumes; fetch-only gets the workload win at a fraction of the resume cost.

Select what runs via the resume-prefetch-source LaunchDarkly flag: "init"
(default, replay only the init trace, prefaulted -- today's behavior),
"last-cycle" (only the header-derived diff, fetch-only), "both" (init first,
then last-cycle behind a barrier so the large fetch stays off the
resume-critical path), or "off". Coalescing (memory-prefetch-coalesce-max-mb)
applies to the fetch. Default "init" keeps this strictly additive.

Record per-resume observability: the chosen source and resolved mapping sizes
as resume span attributes, and a mapping-size histogram
(orchestrator.sandbox.uffd.prefetch.mapping_blocks, tagged by prefault/fetch
mode) for rollout dashboards.

Signed-off-by: Nikita Kalyazin <nikita.kalyazin@e2b.dev>
Add resume-last-cycle-prefetch-max-mib (default -1 = uncapped, negative means
no limit per the codebase convention) to bound how much of the last-cycle diff
a single resume prefetches. The recorded diff is small by construction, so
uncapped is the expected steady state; the cap throttles the heavy-churn tail
(GiBs) against the shared object-store pool without a redeploy.
capResumePrefetch keeps the first N MiB of blocks in offset order and leaves
the rest to demand-fault; it applies only to the last-cycle mapping (the init
trace is small and stays uncapped), which is why the flag names last-cycle
explicitly.

Signed-off-by: Nikita Kalyazin <nikita.kalyazin@e2b.dev>
…scing

Table-driven tests for buildDiffMemoryPrefetchMapping (own-build selection,
other BuildIds excluded, cross-layer dedup, an unaligned entry straddling a
block boundary, nil/empty/zero-blocksize returns nil), selectResumePrefetch
(off/init/last-cycle/both plus unknown -> init), and capResumePrefetch
(uncapped and oversized pass through unchanged, truncation keeps the earliest
blocks in offset order without mutating the input, zero cap empties the set,
nil and zero-blocksize edge cases).

Prefetcher-level tests: coalesceIndices grouping; a coalesced multi-block
fetch still yields one page-sized copy per block; a short read is skipped, not
sliced into a panic; and Start() with Prefault off never calls Prefault (the
fetch-only guarantee at the integration seam) while still fetching every block
to warm the cache.

Signed-off-by: Nikita Kalyazin <nikita.kalyazin@e2b.dev>
@kalyazin kalyazin force-pushed the kalyazin/resume-fulllife-prefetch branch from 7ca1476 to 6ad33d6 Compare July 13, 2026 18:35
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