Repository navigation
Expose internal scheduling metadata - #2873
Merged
Merged
Conversation
Return compact build-chain metadata from snapshot creation so API placement can later use real build ancestry for resume affinity.
PR SummaryMedium Risk Overview Reviewed by Cursor Bugbot for commit e46ed48. Bugbot is set up for automated code reviews on this repo. Configure here. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
❌ 3 Tests Failed:
View the full list of 3 ❄️ 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}
Contributor
There was a problem hiding this comment.
Code Review
In update_snapshot_scheduling_metadata.sql, using to_jsonb on a text parameter stores it as an escaped string scalar rather than a JSONB object, so the parameter should be cast directly to jsonb. In sandboxes.go, accessing res.SchedulingMetadata will cause a compilation error because the snapshotResult struct lacks this field. Additionally, NewSnapshotSchedulingMetadata in snapshot.go is missing nil checks for the Mapping fields of the headers, which can lead to a nil pointer dereference panic.
Populate pause/checkpoint responses from snapshot results and store scheduling metadata as JSONB.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Autofix Details
Bugbot Autofix prepared fixes for both issues found in the latest run.
- ✅ Fixed: Pause fails after orchestrator pause
- Added failSnapshotBuild call when updateSnapshotSchedulingMetadata fails after successful pause RPC to properly mark build status as failed.
- ✅ Fixed: Metadata error finishes snapshot success
- Added finish(err) call before returning when updateSnapshotSchedulingMetadata fails to prevent deferred finish(nil) from incorrectly marking snapshot as success.
Or push these changes by commenting:
@cursor push caa2284235
Preview (caa2284235)
diff --git a/packages/api/internal/orchestrator/pause_instance.go b/packages/api/internal/orchestrator/pause_instance.go
--- a/packages/api/internal/orchestrator/pause_instance.go
+++ b/packages/api/internal/orchestrator/pause_instance.go
@@ -52,6 +52,8 @@
}
if schedulingMetadata != nil {
if err := o.updateSnapshotSchedulingMetadata(ctx, sbx.SandboxID, schedulingMetadata); err != nil {
+ o.failSnapshotBuild(ctx, result.BuildID, err)
+
return err
}
}
diff --git a/packages/api/internal/orchestrator/snapshot_template.go b/packages/api/internal/orchestrator/snapshot_template.go
--- a/packages/api/internal/orchestrator/snapshot_template.go
+++ b/packages/api/internal/orchestrator/snapshot_template.go
@@ -106,6 +106,8 @@
if err := o.updateSnapshotSchedulingMetadata(ctx, sbx.SandboxID, metadata); err != nil {
o.failSnapshotBuild(ctx, upsertResult.BuildID, err)
+ finish(err)
+
return SnapshotTemplateResult{}, err
}
}You can send follow-ups to the cloud agent here.
Do not fail a successful pause or checkpoint if persisting optional scheduling metadata fails.
This comment has been minimized.
This comment has been minimized.
Use slices.SortFunc and remove an unnecessary conversion in metadata generation.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Limit one drops parent build
- Added condition to prevent base build from overwriting parent build when chain limit is 1 and last element is the parent.
Or push these changes by commenting:
@cursor push 527fa53cf5
Preview (527fa53cf5)
diff --git a/packages/orchestrator/pkg/sandbox/snapshot.go b/packages/orchestrator/pkg/sandbox/snapshot.go
--- a/packages/orchestrator/pkg/sandbox/snapshot.go
+++ b/packages/orchestrator/pkg/sandbox/snapshot.go
@@ -139,7 +139,7 @@
break
}
}
- if !hasBase {
+ if !hasBase && chain[len(chain)-1].buildID != parentBuildID {
if c, ok := contributions[baseBuildID]; ok {
chain[len(chain)-1] = *c
} else {You can send follow-ups to the cloud agent here.
Avoid replacing the only chain entry with the base build because base build is already stored separately.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Autofix Details
Bugbot Autofix prepared fixes for both issues found in the latest run.
- ✅ Fixed: Nested metadata breaks JSONBStringMap scans
- ✅ Fixed: Upsert clears scheduling metadata key
- Changed create_new_snapshot.sql conflict handler to use snapshots.metadata || excluded.metadata instead of excluded.metadata, preserving existing metadata keys including snapshot_scheduling_metadata during upsert.
Or push these changes by commenting:
@cursor push c71543e849
Preview (c71543e849)
diff --git a/packages/db/queries/create_new_snapshot.sql.go b/packages/db/queries/create_new_snapshot.sql.go
--- a/packages/db/queries/create_new_snapshot.sql.go
+++ b/packages/db/queries/create_new_snapshot.sql.go
@@ -55,7 +55,7 @@
now()
)
ON CONFLICT (sandbox_id) DO UPDATE SET
- metadata = excluded.metadata,
+ metadata = snapshots.metadata || excluded.metadata,
sandbox_started_at = excluded.sandbox_started_at,
allow_internet_access = COALESCE(excluded.allow_internet_access, snapshots.allow_internet_access),
origin_node_id = excluded.origin_node_id,
diff --git a/packages/db/queries/snapshots/create_new_snapshot.sql b/packages/db/queries/snapshots/create_new_snapshot.sql
--- a/packages/db/queries/snapshots/create_new_snapshot.sql
+++ b/packages/db/queries/snapshots/create_new_snapshot.sql
@@ -41,7 +41,7 @@
now()
)
ON CONFLICT (sandbox_id) DO UPDATE SET
- metadata = excluded.metadata,
+ metadata = snapshots.metadata || excluded.metadata,
sandbox_started_at = excluded.sandbox_started_at,
allow_internet_access = COALESCE(excluded.allow_internet_access, snapshots.allow_internet_access),
origin_node_id = excluded.origin_node_id,
diff --git a/packages/db/queries/snapshots/update_snapshot_scheduling_metadata.sql b/packages/db/queries/snapshots/update_snapshot_scheduling_metadata.sql
--- a/packages/db/queries/snapshots/update_snapshot_scheduling_metadata.sql
+++ b/packages/db/queries/snapshots/update_snapshot_scheduling_metadata.sql
@@ -3,7 +3,7 @@
SET metadata = jsonb_set(
metadata,
'{snapshot_scheduling_metadata}',
- (@metadata::text)::jsonb,
+ to_jsonb(@metadata::text),
true
)
WHERE sandbox_id = @sandbox_id;
diff --git a/packages/db/queries/update_snapshot_scheduling_metadata.sql.go b/packages/db/queries/update_snapshot_scheduling_metadata.sql.go
--- a/packages/db/queries/update_snapshot_scheduling_metadata.sql.go
+++ b/packages/db/queries/update_snapshot_scheduling_metadata.sql.go
@@ -1,6 +1,6 @@
// Code generated by sqlc. DO NOT EDIT.
// versions:
-// sqlc v1.31.1
+// sqlc v1.29.0
// source: update_snapshot_scheduling_metadata.sql
package queries
@@ -14,7 +14,7 @@
SET metadata = jsonb_set(
metadata,
'{snapshot_scheduling_metadata}',
- ($1::text)::jsonb,
+ to_jsonb($1::text),
true
)
WHERE sandbox_id = $2You can send follow-ups to the cloud agent here.
Store scheduling metadata as a string value in the existing metadata map and keep it across snapshot upserts until refreshed.
ValentaTomas
marked this pull request as ready for review
May 30, 2026 21:58
dobrac
previously requested changes
May 30, 2026
Comment on lines
+99
to
+102
| baseBuildID := memfileHeader.Metadata.BaseBuildId | ||
| if baseBuildID == uuid.Nil { | ||
| baseBuildID = rootfsHeader.Metadata.BaseBuildId | ||
| } |
Contributor
There was a problem hiding this comment.
could you explain this please? Should we have two different baseBuildIDs?
Member
Author
There was a problem hiding this comment.
There is one scheduling base in the persisted metadata. We prefer the memfile base and fall back to rootfs only if the memfile base is nil; this avoids adding two base IDs to the scheduling contract in this PR.
Contributor
There was a problem hiding this comment.
why don't we have two baseBuildIDs if effectively there are two?
ValentaTomas
changed the title
![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: 055b4e0d2a
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Generation incremented twice
- Removed the duplicate generation increment from FromHeaders and moved it to only apply in NewSnapshotSchedulingMetadata for pre-snapshot headers.
Or push these changes by commenting:
@cursor push 9d1cfd4f72
Preview (9d1cfd4f72)
diff --git a/packages/orchestrator/pkg/sandbox/snapshot.go b/packages/orchestrator/pkg/sandbox/snapshot.go
--- a/packages/orchestrator/pkg/sandbox/snapshot.go
+++ b/packages/orchestrator/pkg/sandbox/snapshot.go
@@ -58,5 +58,9 @@
}
func NewSnapshotSchedulingMetadata(memfileHeader, rootfsHeader *header.Header) *orchestrator.SchedulingMetadata {
- return scheduling.FromHeaders(memfileHeader, rootfsHeader)
+ metadata := scheduling.FromHeaders(memfileHeader, rootfsHeader)
+ if metadata != nil {
+ metadata.Generation++
+ }
+ return metadata
}
diff --git a/packages/orchestrator/pkg/scheduling/metadata.go b/packages/orchestrator/pkg/scheduling/metadata.go
--- a/packages/orchestrator/pkg/scheduling/metadata.go
+++ b/packages/orchestrator/pkg/scheduling/metadata.go
@@ -93,7 +93,7 @@
res := &orchestrator.SchedulingMetadata{
BaseBuildId: baseBuildID.String(),
- Generation: max(memfileHeader.Metadata.Generation, rootfsHeader.Metadata.Generation) + 1,
+ Generation: max(memfileHeader.Metadata.Generation, rootfsHeader.Metadata.Generation),
ChainBuildIds: make([]string, 0, len(chain)),
MemfileLogicalBytes: make([]uint64, 0, len(chain)),
MemfileChunkCounts: make([]uint32, 0, len(chain)),You can send follow-ups to the cloud agent here.
|
Bugbot Autofix prepared fixes for both issues found in the latest run.
Or push these changes by commenting: Preview (526c21400a)diff --git a/packages/orchestrator/pkg/sandbox/sandbox.go b/packages/orchestrator/pkg/sandbox/sandbox.go
--- a/packages/orchestrator/pkg/sandbox/sandbox.go
+++ b/packages/orchestrator/pkg/sandbox/sandbox.go
@@ -1185,7 +1185,7 @@
}
cleanup.AddNoContext(ctx, rootfsDiff.Close)
- schedulingMetadata := NewSnapshotSchedulingMetadata(originalMemfile.Header(), originalRootfs.Header())
+ schedulingMetadata := NewSnapshotSchedulingMetadata(ctx, memfileDiffHeader, NewResolvedDiffHeader(rootfsHeader))
metadataFileLink := template.NewLocalFileLink(cachePaths.CacheMetadata())
cleanup.AddNoContext(ctx, metadataFileLink.Close)
diff --git a/packages/orchestrator/pkg/sandbox/snapshot.go b/packages/orchestrator/pkg/sandbox/snapshot.go
--- a/packages/orchestrator/pkg/sandbox/snapshot.go
+++ b/packages/orchestrator/pkg/sandbox/snapshot.go
@@ -57,6 +57,12 @@
return nil
}
-func NewSnapshotSchedulingMetadata(memfileHeader, rootfsHeader *header.Header) *orchestrator.SchedulingMetadata {
+func NewSnapshotSchedulingMetadata(ctx context.Context, memfileDiffHeader, rootfsDiffHeader *DiffHeader) *orchestrator.SchedulingMetadata {
+ memfileHeader, memfileErr := memfileDiffHeader.WaitWithContext(ctx)
+ rootfsHeader, rootfsErr := rootfsDiffHeader.WaitWithContext(ctx)
+ if memfileErr != nil || rootfsErr != nil {
+ return nil
+ }
+
return scheduling.FromHeaders(memfileHeader, rootfsHeader)
}
diff --git a/packages/orchestrator/pkg/sandbox/template/storage_template.go b/packages/orchestrator/pkg/sandbox/template/storage_template.go
--- a/packages/orchestrator/pkg/sandbox/template/storage_template.go
+++ b/packages/orchestrator/pkg/sandbox/template/storage_template.go
@@ -272,9 +272,9 @@
return t.paths
}
-func (t *storageTemplate) SchedulingMetadata() *orchestrator.SchedulingMetadata {
- memfileHeader, memfileErr := t.memfileHeader.Result()
- rootfsHeader, rootfsErr := t.rootfsHeader.Result()
+func (t *storageTemplate) SchedulingMetadata(ctx context.Context) *orchestrator.SchedulingMetadata {
+ memfileHeader, memfileErr := t.memfileHeader.WaitWithContext(ctx)
+ rootfsHeader, rootfsErr := t.rootfsHeader.WaitWithContext(ctx)
if memfileErr != nil || rootfsErr != nil {
return nil
}
diff --git a/packages/orchestrator/pkg/server/template_cache.go b/packages/orchestrator/pkg/server/template_cache.go
--- a/packages/orchestrator/pkg/server/template_cache.go
+++ b/packages/orchestrator/pkg/server/template_cache.go
@@ -20,9 +20,9 @@
for key, item := range s.templateCache.Items() {
var metadata *orchestrator.SchedulingMetadata
if provider, ok := item.Value().(interface {
- SchedulingMetadata() *orchestrator.SchedulingMetadata
+ SchedulingMetadata(context.Context) *orchestrator.SchedulingMetadata
}); ok {
- metadata = provider.SchedulingMetadata()
+ metadata = provider.SchedulingMetadata(ctx)
}
builds = append(builds, &orchestrator.CachedBuildInfo{
BuildId: key,
diff --git a/packages/orchestrator/pkg/template/build/builder.go b/packages/orchestrator/pkg/template/build/builder.go
--- a/packages/orchestrator/pkg/template/build/builder.go
+++ b/packages/orchestrator/pkg/template/build/builder.go
@@ -390,23 +390,23 @@
KernelVersion: bc.Config.KernelVersion,
FirecrackerVersion: bc.Config.FirecrackerVersion,
RootfsSizeMB: units.BytesToMB(int64(rootfsSize)),
- SchedulingMetadata: templateSchedulingMetadata(builder.templateCache, lastLayerResult.Metadata.Template.BuildID),
+ SchedulingMetadata: templateSchedulingMetadata(ctx, builder.templateCache, lastLayerResult.Metadata.Template.BuildID),
}, nil
}
-func templateSchedulingMetadata(cache *sbxtemplate.Cache, buildID string) *templatemanager.TemplateSchedulingMetadata {
+func templateSchedulingMetadata(ctx context.Context, cache *sbxtemplate.Cache, buildID string) *templatemanager.TemplateSchedulingMetadata {
t, ok := cache.GetCachedTemplate(buildID)
if !ok {
return nil
}
provider, ok := t.(interface {
- SchedulingMetadata() *orchestratorgrpc.SchedulingMetadata
+ SchedulingMetadata(context.Context) *orchestratorgrpc.SchedulingMetadata
})
if !ok {
return nil
}
- return toTemplateSchedulingMetadata(provider.SchedulingMetadata())
+ return toTemplateSchedulingMetadata(provider.SchedulingMetadata(ctx))
}
func toTemplateSchedulingMetadata(m *orchestratorgrpc.SchedulingMetadata) *templatemanager.TemplateSchedulingMetadata {You can send follow-ups to the cloud agent here. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d21605098a
ℹ️ 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".
ValentaTomas
marked this pull request as draft
June 1, 2026 02:32
Derive scheduling metadata from resolved device/diff headers so pause, create, cached-build listing and template builds report the actual build, its own diff contributions, and the on-disk generation. Report logical vs on-disk byte stats and merge template cache entries into ListCachedBuilds.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Autofix Details
Bugbot Autofix prepared fixes for both issues found in the latest run.
- ✅ Fixed: Uninitialized storage diff stats panic
- Added nil check in StorageDiff.FileSize to return error instead of dereferencing nil chunker when called on uninitialized diffs.
- ✅ Fixed: Pause hides header wait failures
- Changed NewSnapshotSchedulingMetadata to return error when header wait fails, causing pause/checkpoint operations to fail instead of silently omitting metadata.
Or push these changes by commenting:
@cursor push 5691fa6c77
Preview (5691fa6c77)
diff --git a/packages/orchestrator/pkg/sandbox/build/storage_diff.go b/packages/orchestrator/pkg/sandbox/build/storage_diff.go
--- a/packages/orchestrator/pkg/sandbox/build/storage_diff.go
+++ b/packages/orchestrator/pkg/sandbox/build/storage_diff.go
@@ -128,6 +128,10 @@
}
func (b *StorageDiff) FileSize(ctx context.Context) (int64, error) {
+ if b.chunker == nil {
+ return 0, fmt.Errorf("chunker not initialized")
+ }
+
return b.chunker.FileSize(ctx)
}
diff --git a/packages/orchestrator/pkg/sandbox/sandbox.go b/packages/orchestrator/pkg/sandbox/sandbox.go
--- a/packages/orchestrator/pkg/sandbox/sandbox.go
+++ b/packages/orchestrator/pkg/sandbox/sandbox.go
@@ -1186,7 +1186,10 @@
cleanup.AddNoContext(ctx, rootfsDiff.Close)
rootfsDiffHeader := NewResolvedDiffHeader(rootfsHeader)
- schedulingMetadata := NewSnapshotSchedulingMetadata(ctx, memfileDiffHeader, rootfsDiffHeader)
+ schedulingMetadata, err := NewSnapshotSchedulingMetadata(ctx, memfileDiffHeader, rootfsDiffHeader)
+ if err != nil {
+ return nil, fmt.Errorf("failed to resolve scheduling metadata: %w", err)
+ }
metadataFileLink := template.NewLocalFileLink(cachePaths.CacheMetadata())
cleanup.AddNoContext(ctx, metadataFileLink.Close)
diff --git a/packages/orchestrator/pkg/sandbox/snapshot.go b/packages/orchestrator/pkg/sandbox/snapshot.go
--- a/packages/orchestrator/pkg/sandbox/snapshot.go
+++ b/packages/orchestrator/pkg/sandbox/snapshot.go
@@ -59,12 +59,16 @@
// NewSnapshotSchedulingMetadata derives the metadata from the new snapshot's
// diff headers so the freshly created build and its contributions are included.
-func NewSnapshotSchedulingMetadata(ctx context.Context, memfileDiffHeader, rootfsDiffHeader *DiffHeader) *orchestrator.SchedulingMetadata {
+func NewSnapshotSchedulingMetadata(ctx context.Context, memfileDiffHeader, rootfsDiffHeader *DiffHeader) (*orchestrator.SchedulingMetadata, error) {
memfileHeader, memfileErr := memfileDiffHeader.WaitWithContext(ctx)
+ if memfileErr != nil {
+ return nil, fmt.Errorf("memfile header wait failed: %w", memfileErr)
+ }
+
rootfsHeader, rootfsErr := rootfsDiffHeader.WaitWithContext(ctx)
- if memfileErr != nil || rootfsErr != nil {
- return nil
+ if rootfsErr != nil {
+ return nil, fmt.Errorf("rootfs header wait failed: %w", rootfsErr)
}
- return scheduling.FromHeaders(memfileHeader, rootfsHeader)
+ return scheduling.FromHeaders(memfileHeader, rootfsHeader), nil
}You can send follow-ups to the cloud agent here.
…adata FileSize panicked on diffs cached before Init (nil chunker), matching the nil guards already on Size/IsCached/Close. NewSnapshotSchedulingMetadata now warns when a diff header fails to resolve instead of silently returning nil.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Autofix Details
Bugbot Autofix prepared fixes for both issues found in the latest run.
- ✅ Fixed: Pause waits on memfile header
- Moved scheduling metadata computation from synchronous snapshot creation to checkpoint handler, allowing Pause to return before memfile-dedup header resolution completes.
- ✅ Fixed: Chain cap is thirty two
- Increased chainLimit constant from 32 to 128 to match the intended hard maximum for build chain metadata truncation.
Or push these changes by commenting:
@cursor push 7d7801b4ce
Preview (7d7801b4ce)
diff --git a/packages/orchestrator/pkg/sandbox/sandbox.go b/packages/orchestrator/pkg/sandbox/sandbox.go
--- a/packages/orchestrator/pkg/sandbox/sandbox.go
+++ b/packages/orchestrator/pkg/sandbox/sandbox.go
@@ -1186,7 +1186,6 @@
cleanup.AddNoContext(ctx, rootfsDiff.Close)
rootfsDiffHeader := NewResolvedDiffHeader(rootfsHeader)
- schedulingMetadata := NewSnapshotSchedulingMetadata(ctx, memfileDiffHeader, rootfsDiffHeader)
metadataFileLink := template.NewLocalFileLink(cachePaths.CacheMetadata())
cleanup.AddNoContext(ctx, metadataFileLink.Close)
@@ -1203,7 +1202,7 @@
MemfileDiffHeader: memfileDiffHeader,
RootfsDiff: rootfsDiff,
RootfsDiffHeader: rootfsDiffHeader,
- SchedulingMetadata: schedulingMetadata,
+ SchedulingMetadata: nil,
MemfileBlockSize: originalMemfile.Header().Metadata.BlockSize,
RootfsBlockSize: originalRootfs.Header().Metadata.BlockSize,
diff --git a/packages/orchestrator/pkg/scheduling/metadata.go b/packages/orchestrator/pkg/scheduling/metadata.go
--- a/packages/orchestrator/pkg/scheduling/metadata.go
+++ b/packages/orchestrator/pkg/scheduling/metadata.go
@@ -10,7 +10,7 @@
"github.com/e2b-dev/infra/packages/shared/pkg/storage/header"
)
-const chainLimit = 32
+const chainLimit = 128
type buildContribution struct {
buildID uuid.UUID
diff --git a/packages/orchestrator/pkg/server/sandboxes.go b/packages/orchestrator/pkg/server/sandboxes.go
--- a/packages/orchestrator/pkg/server/sandboxes.go
+++ b/packages/orchestrator/pkg/server/sandboxes.go
@@ -873,9 +873,14 @@
}
}
+ // Compute scheduling metadata after snapshot creation so Pause returns
+ // without waiting on memfile-dedup header resolution. The background
+ // goroutine in pauseProcessMemory will have had time to finish by now.
+ schedulingMetadata := sandbox.NewSnapshotSchedulingMetadata(ctx, snapshot.MemfileDiffHeader, snapshot.RootfsDiffHeader)
+
return &snapshotResult{
meta: meta,
- schedulingMetadata: snapshot.SchedulingMetadata,
+ schedulingMetadata: schedulingMetadata,
upload: upload,
completeUpload: completeUpload,
}, nilYou can send follow-ups to the cloud agent here.
Drop per-build byte/mapping-count arrays and overall sizes; report only base build, generation, and the deduplicated union of referenced build IDs (the resume-affinity signal). Compute it synchronously on pause from the resolved parent memfile header plus the new rootfs header, so pause no longer blocks on the async memfile-dedup header. Generation is max() of the two headers; chain cap raised to 64.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Chain cap drops scheduling build IDs
- Removed the arbitrary 64-entry cap that silently truncated build IDs, allowing all referenced builds to be included in scheduling metadata.
Or push these changes by commenting:
@cursor push ebf40fc783
Preview (ebf40fc783)
diff --git a/packages/orchestrator/pkg/scheduling/metadata.go b/packages/orchestrator/pkg/scheduling/metadata.go
--- a/packages/orchestrator/pkg/scheduling/metadata.go
+++ b/packages/orchestrator/pkg/scheduling/metadata.go
@@ -9,9 +9,6 @@
"github.com/e2b-dev/infra/packages/shared/pkg/storage/header"
)
-// chainLimit caps how many build IDs are reported in scheduling metadata.
-const chainLimit = 64
-
// FromHeaders builds scheduling metadata for resume affinity. ChainBuildIds is
// the deduplicated union of build IDs referenced by the memfile and rootfs
// mappings, sorted for determinism (a membership set, not lineage order).
@@ -35,9 +32,6 @@
chain = append(chain, id.String())
}
slices.Sort(chain)
- if len(chain) > chainLimit {
- chain = chain[:chainLimit]
- }
baseBuildID := memfileHeader.Metadata.BaseBuildId
if baseBuildID == uuid.Nil {You can send follow-ups to the cloud agent here.
Report per-artifact (memfile, rootfs) referenced build IDs instead of a single union chain, and drop base/generation. Save paths inject the new build (last layer) and derive the rest from resolved parent headers, so pause stays synchronous even if the rootfs copy later becomes async. Sets are sorted; order is not significant for affinity matching.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Base build omitted from ID lists
- Added h.Metadata.BaseBuildId to the buildIDs function to ensure base build IDs appear in the repeated lists as required by the proto contract.
Or push these changes by commenting:
@cursor push 759e604362
Preview (759e604362)
diff --git a/packages/orchestrator/pkg/scheduling/metadata.go b/packages/orchestrator/pkg/scheduling/metadata.go
--- a/packages/orchestrator/pkg/scheduling/metadata.go
+++ b/packages/orchestrator/pkg/scheduling/metadata.go
@@ -38,6 +38,9 @@
func buildIDs(h *header.Header, extra uuid.UUID) []string {
seen := make(map[uuid.UUID]struct{})
+ if h.Metadata.BaseBuildId != uuid.Nil {
+ seen[h.Metadata.BaseBuildId] = struct{}{}
+ }
for _, m := range h.Mapping.All() {
if m.BuildId != uuid.Nil {
seen[m.BuildId] = struct{}{}You can send follow-ups to the cloud agent here.
Reviewed by Cursor Bugbot for commit 31c1d2b. Configure here.
Import the orchestrator SchedulingMetadata into template-manager instead of duplicating it as TemplateSchedulingMetadata, so the template build result and the create/resume/list responses use the identical type and the API can match them directly. Drops the duplicate message and its mapper.
The per-build uncompressed size is already carried in the header (Builds[id].Size) and captured into StorageDiff.uncompressedSize, so return that directly and drop the redundant Chunker.Size accessor.
Add Mapping.BytesByBuild which scans the length/buildIdx columns directly instead of materializing a BuildMap and hashing a uuid per entry, and use it for the scheduling metadata per-layer byte sums.
ListCachedBuilds reported per-build cached/total bytes from the diff store via FileSize, which is the wrong source: per-layer referenced bytes already come from the mapping in SchedulingMetadata. Remove the CachedBuildStats machinery and the CachedBuildInfo byte fields; ListCachedBuilds keeps build id, expiration, and scheduling metadata. Reverts StorageDiff.Size to FileSize.
ValentaTomas
marked this pull request as ready for review
June 1, 2026 05:13
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Comment on lines
19
to
30
| for key, item := range s.templateCache.Items() { | ||
| builds = append(builds, &orchestrator.CachedBuildInfo{ | ||
| info := &orchestrator.CachedBuildInfo{ | ||
| BuildId: key, | ||
| ExpirationTime: timestamppb.New(item.ExpiresAt()), | ||
| }) | ||
| } | ||
| if provider, ok := item.Value().(interface { | ||
| SchedulingMetadata(ctx context.Context) *orchestrator.SchedulingMetadata | ||
| }); ok { | ||
| info.SchedulingMetadata = provider.SchedulingMetadata(ctx) | ||
| } | ||
| builds = append(builds, info) | ||
| } |
There was a problem hiding this comment.
🔴 ListCachedBuilds now calls SchedulingMetadata(ctx) for every cached template, which blocks on memfile/rootfs WaitWithContext until the background Fetch finishes (cache.go:340-344 inserts entries before Fetch resolves the SetOnce headers). This converts a previously O(1)-per-entry RPC into one bounded by the slowest in-progress GCS fetch, multiplied across every still-loading cache entry because the loop is serial. Suggested fix: use a non-blocking IsSet check or a short per-call timeout, returning nil scheduling_metadata when the headers aren't yet resolved.
Extended reasoning...
Bug: ListCachedBuilds at packages/orchestrator/pkg/server/template_cache.go:19-30 iterates every cached template and synchronously calls provider.SchedulingMetadata(ctx). The storageTemplate.SchedulingMetadata implementation at packages/orchestrator/pkg/sandbox/template/storage_template.go:278-282 blocks on t.memfile.WaitWithContext(ctx) and t.rootfs.WaitWithContext(ctx). Per packages/shared/pkg/utils/set_once.go:104-113, WaitWithContext blocks until SetValue/SetError is called or ctx is cancelled.\n\nHow entries get into the cache before resolving: getTemplateWithFetch at packages/orchestrator/pkg/sandbox/template/cache.go:340-344 inserts a fresh storageTemplate into the TTL cache via c.cache.GetOrSet(key, tmpl, ...) and then launches go tmpl.Fetch(context.WithoutCancel(ctx), c.buildStore) in a background goroutine. Fetch is what eventually calls SetValue on t.memfile and t.rootfs (storage_template.go:209, 246), but only after NewStorage downloads diff headers from GCS. During that window the entry is enumerated by s.templateCache.Items() with unresolved SetOnces.\n\nWhy existing code does not prevent it: Items() returns all live cache entries, not just resolved ones, and the new code path has no IsSet / Result fast path — every entry is awaited. The loop is serial, so a single slow fetch stalls every subsequent entry too. Pre-PR the same loop only read item.ExpiresAt() (an O(1) field read) and returned in microseconds.\n\nImpact: ListCachedBuilds becomes O(slowest-in-flight-fetch * N) in the worst case, bounded only by the gRPC client deadline. It will not deadlock the orchestrator (WaitWithContext respects ctx.Done()), but the RPC's purpose — surfacing locally cached builds for placement decisions — is defeated when callers time out instead of getting fast partial answers. The Cursor bot raised this exact regression in inline-comment 3331665823 ("ListCachedBuilds waits on fetch", Medium Severity) on commit 936aef4; the comment was marked resolved on GitHub but the code at HEAD still calls WaitWithContext.\n\nStep-by-step proof:\n1. Orchestrator boots; templateCache is empty.\n2. SandboxCreate is called for two templates whose builds are not yet local. Each getTemplateWithFetch (cache.go:340) inserts a new storageTemplate into the cache and then runs go tmpl.Fetch(...). Both Fetch goroutines block on GCS for, say, 4 s each.\n3. ~50 ms later, an operator calls ListCachedBuilds. The handler ranges over s.templateCache.Items() (template_cache.go:19) and hits entry A first.\n4. provider.SchedulingMetadata(ctx) calls t.memfile.WaitWithContext(ctx) (storage_template.go:279). t.memfile is unset because A's Fetch hasn't finished, so the call blocks for the remaining ~4 s of A's GCS download.\n5. The loop moves to entry B and repeats: WaitWithContext again blocks for B's remaining download time.\n6. The RPC returns after ~8 s instead of microseconds, even though the question — which builds are cached and when do they expire — does not require resolved headers.\n\nSuggested fix: prefer a non-blocking variant — check memfile.IsSet() / rootfs.IsSet() (or use Result() which returns immediately) and return scheduling_metadata: nil when either is not yet resolved. This keeps the RPC O(1) per entry and matches the author's stated "best-effort enrichment" intent for scheduling metadata. Alternatively, dispatch the providers concurrently with a small per-call timeout (e.g. via errgroup + context.WithTimeout).
🔬 also observed by verify-runtime
Keep scheduling metadata only on lifecycle responses (create/resume, pause, checkpoint, build) and stop exposing it on the cached-builds listing/polling path.
jakubno
approved these changes
Jun 1, 2026
jakubno
dismissed
dobrac’s stale review
June 1, 2026 13:15
looks like not high prio, let's do a follow up if needed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Expose compact scheduling metadata from orchestrator and template-manager without DB or public API changes.