Repository navigation
feat: persist synchronously-available layer sizes to env_builds via gRPC - #3119
ValentaTomas wants to merge 3 commits into
Conversation
…etadata Attach logical (virtual device) size and non-zero mapped size as custom metadata on the memfile/rootfs data objects at upload time, alongside the existing uncompressed-size, so the storage index can read layer sizes without parsing the binary header.
PR SummaryMedium Risk Overview Reviewed by Cursor Bugbot for commit 0cf3173. Bugbot is set up for automated code reviews on this repo. Configure here. |
There was a problem hiding this comment.
Code Review
If the memory snapshot or rootfs diff headers resolve to nil, the respective upload goroutines will still proceed to upload the data files, resulting in orphaned uploads. To prevent this, check if the resolved headers are nil and return early before initiating the uploads.
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.
| h, err := u.snap.MemorySnapshot.DiffHeader.WaitWithContext(egCtx) | ||
| if err != nil { | ||
| return fmt.Errorf("wait memfile diff header: %w", err) | ||
| } |
There was a problem hiding this comment.
If the memory snapshot diff header resolves to nil, the header upload goroutine returns early, but this goroutine will still proceed to upload the memfile. To prevent uploading an orphan data file without its corresponding header, check if the header is nil and return early.
h, err := u.snap.MemorySnapshot.DiffHeader.WaitWithContext(egCtx)
if err != nil {
return fmt.Errorf("wait memfile diff header: %w", err)
}
if h == nil {
return nil
}| h, err := u.snap.RootfsDiffHeader.WaitWithContext(egCtx) | ||
| if err != nil { | ||
| return fmt.Errorf("wait rootfs diff header: %w", err) | ||
| } |
There was a problem hiding this comment.
If the rootfs diff header resolves to nil, the header upload goroutine returns early, but this goroutine will still proceed to upload the rootfs. To prevent uploading an orphan data file without its corresponding header, check if the header is nil and return early.
h, err := u.snap.RootfsDiffHeader.WaitWithContext(egCtx)
if err != nil {
return fmt.Errorf("wait rootfs diff header: %w", err)
}
if h == nil {
return nil
}
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Replace the object-metadata approach with returning per-artifact layer sizes over gRPC (pause/checkpoint responses and TemplateBuildMetadata) and persisting them on the env_builds row. Only sizes available without blocking on the async memfile dedup header are included: rootfs mapped/diff and memfile logical (rootfs logical is already stored as total_disk_size_mb). The async memfile mapped/diff sizes are handled separately.
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: Layer sizes fail marks build failed
- Modified SetFinished to log but not return errors from SetEnvBuildLayerSizes, preventing layer-size persistence failures from marking successful builds as failed.
Or push these changes by commenting:
@cursor push ffd0512efc
Preview (ffd0512efc)
diff --git a/packages/api/internal/template-manager/template_status.go b/packages/api/internal/template-manager/template_status.go
--- a/packages/api/internal/template-manager/template_status.go
+++ b/packages/api/internal/template-manager/template_status.go
@@ -321,13 +321,17 @@
FirecrackerVersion: firecrackerVersion,
BuildID: buildID,
})
- if err == nil {
- err = tm.sqlcDB.SetEnvBuildLayerSizes(ctx, apidb.LayerSizesParams(buildID, layerSizes))
+ if err != nil {
+ return err
}
+ if layerSizeErr := tm.sqlcDB.SetEnvBuildLayerSizes(ctx, apidb.LayerSizesParams(buildID, layerSizes)); layerSizeErr != nil {
+ logger.L().Error(ctx, "failed to persist layer sizes, continuing with successful build", zap.Error(layerSizeErr), logger.WithBuildID(buildID.String()))
+ }
+
tm.buildCache.Invalidate(ctx, buildID)
- return err
+ return nil
}
// buildStatus maps a status group to a default build status for the database.You can send follow-ups to the cloud agent here.
Reviewed by Cursor Bugbot for commit a131641. Configure here.
| }) | ||
| if err == nil { | ||
| err = tm.sqlcDB.SetEnvBuildLayerSizes(ctx, apidb.LayerSizesParams(buildID, layerSizes)) | ||
| } |
There was a problem hiding this comment.
Layer sizes fail marks build failed
High Severity
SetFinished runs FinishTemplateBuild first, then SetEnvBuildLayerSizes. If layer-size persistence fails, the whole call errors even though the build is already uploaded. Poll treats that as unrecoverable and calls SetStatus with failed, clobbering a successful template build. Pause/checkpoint log layer-size errors and do not fail the operation.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit a131641. Configure here.
…sizes Move the rootfs mapped/diff sizes out of the gRPC/DB path so each size category lives in one place: logical sizes go via gRPC into env_builds (memfile logical; rootfs logical is already total_disk_size_mb), while mapped and diff sizes for both artifacts are written to the data objects' metadata (see the memfile-layer -metadata PR). Drops the rootfs_*_size_bytes columns and the now-unused Mapping.MappedBytes helper.
|
Superseded by #3122: per the latest direction, all layer sizes (logical/mapped/diff, both artifacts) go via GCS object metadata and the gRPC/env_builds path is dropped for now. |
… metadata (#3122) Writes each layer's `logical-size`, `mapped-size`, and `diff-size` (all uncompressed, decimal bytes) as custom metadata on the memfile and rootfs data objects, derived from the resolved diff header during the background upload. Sizes live on the data object rather than the build row because the memfile values depend on the async dedup header, which resolves after the build row is finalized. Deriving them from the header adds no latency to the pause/checkpoint response. Supersedes #3119 (the gRPC-to-env_builds approach), which is closed.
Persists per-artifact layer sizes on the
env_buildsrow, returned over gRPC from the build/pause/snapshot flows (no new GCS object metadata).Only sizes available without blocking on the async memfile dedup header are included here (that header can take 30s+ under background-copy):
rootfs_mapped_size_bytes,rootfs_diff_size_bytes(rootfs header is built synchronously)memfile_logical_size_bytes(from the base memfile header)rootfs logical size is already stored as
total_disk_size_mb. The truly-async memfile mapped/diff sizes are handled in a follow-up PR (written to the memfile data object metadata during upload).Wiring:
LayerSizesmessage onSandboxPauseResponse,SandboxCheckpointResponse, andTemplateBuildMetadataSnapshot.SyncLayerSizes,getLayerSizesfor template builds)SetEnvBuildLayerSizesquery (pause/snapshot are best-effort, non-fatal)Most of the diff is generated (proto
.pb.go, sqlc.sql.go).