From aa6712dbf0b180e40f827cdc348b24b40deff960 Mon Sep 17 00:00:00 2001 From: Ed Zynda Date: Mon, 29 Jun 2026 17:48:23 +0300 Subject: [PATCH 1/2] fix(core): honor resources.disk_gb on sandbox create (#1) - thread DiskGB through CreateParams and the create handler so the requested writable-overlay size is no longer silently dropped - map disk_gb (GiB) to the SDK's WithOCIUpperSize (MiB) with unit conversion; only applied when > 0 so the image default still wins - extract the CreateParams -> SandboxOption mapping into a pure buildCreateOptions helper and cover it with unit tests - clarify the disk_gb description in openapi.yaml to match behavior Fixes #1 --- internal/api/handlers.go | 1 + internal/core/service.go | 102 +++++++++++++++++++--------------- internal/core/service_test.go | 55 ++++++++++++++++++ openapi.yaml | 2 +- 4 files changed, 115 insertions(+), 45 deletions(-) create mode 100644 internal/core/service_test.go diff --git a/internal/api/handlers.go b/internal/api/handlers.go index 29440aa..8fed759 100644 --- a/internal/api/handlers.go +++ b/internal/api/handlers.go @@ -46,6 +46,7 @@ func (s *Server) handleCreate(w http.ResponseWriter, r *http.Request) { Image: req.Image, CPU: req.Resources.CPU, MemoryMB: req.Resources.MemoryMB, + DiskGB: req.Resources.DiskGB, AutoStopSecs: req.AutoStopSecs, Env: req.Env, Labels: req.Labels, diff --git a/internal/core/service.go b/internal/core/service.go index 4df5948..5d8e006 100644 --- a/internal/core/service.go +++ b/internal/core/service.go @@ -60,6 +60,7 @@ type CreateParams struct { Image string CPU float64 MemoryMB int + DiskGB int // writable overlay upper size, in GiB (OCI images) AutoStopSecs int Env map[string]string Labels map[string]string @@ -124,6 +125,58 @@ func (s *Service) Create(ctx context.Context, p CreateParams) (*Instance, error) name := newName() workdir := strings.TrimSpace(p.Workdir) + opts := buildCreateOptions(p, image) + + cctx, cancel := context.WithTimeout(ctx, s.createTO) + defer cancel() + + sb, err := msb.CreateSandbox(cctx, name, opts...) + if err != nil { + return nil, fmt.Errorf("create sandbox: %w", err) + } + s.reg.cache(name, sb) + + // Resolve the box's REAL working directory. + // + // 1. Caller pinned a workdir: ensure it exists in the guest (mkdir -p), + // then use it — the dir gets created if the image didn't ship it, + // rather than the SDK refusing to boot. + // 2. No workdir pinned: trust the image's own WORKDIR by asking the guest + // with `pwd`. + // + // Best-effort: fall back to defaultWorkdir on any error. + resolved := workdir + if resolved != "" { + quoted := shellQuote(resolved) + if _, perr := runShell(cctx, sb, ExecParams{Cmd: "mkdir -p " + quoted}); perr != nil { + // Don't fail Create on mkdir error — fall back to the image's WORKDIR. + resolved = "" + } + } + if resolved == "" { + if out, perr := runShell(cctx, sb, ExecParams{Cmd: "pwd"}); perr == nil { + if wd := strings.TrimSpace(out.Stdout); strings.HasPrefix(wd, "/") { + resolved = wd + } + } + } + resolved = defaultWorkdir(resolved) + s.reg.setWorkdir(name, resolved) + + return &Instance{ + ID: name, + Image: image, + State: StateRunning, + Workdir: resolved, + Labels: p.Labels, + }, nil +} + +// buildCreateOptions translates the provider-neutral CreateParams into the +// microsandbox SDK option slice. It is deterministic and free of side effects +// (no SDK calls beyond constructing options), which keeps the CreateParams -> +// SandboxOption mapping unit-testable without booting a microVM. +func buildCreateOptions(p CreateParams, image string) []msb.SandboxOption { opts := []msb.SandboxOption{ msb.WithImage(image), msb.WithDetached(), // survive msbd restart @@ -134,6 +187,10 @@ func (s *Service) Create(ctx context.Context, p CreateParams) (*Instance, error) if p.CPU > 0 { opts = append(opts, msb.WithCPUs(uint8(p.CPU))) } + if p.DiskGB > 0 { + // API field is GiB; SDK option takes MiB. + opts = append(opts, msb.WithOCIUpperSize(uint32(p.DiskGB)*1024)) + } if len(p.Env) > 0 { opts = append(opts, msb.WithEnv(p.Env)) } @@ -188,50 +245,7 @@ func (s *Service) Create(ctx context.Context, p CreateParams) (*Instance, error) if p.AutoStopSecs > 0 { opts = append(opts, msb.WithIdleTimeout(time.Duration(p.AutoStopSecs)*time.Second)) } - - cctx, cancel := context.WithTimeout(ctx, s.createTO) - defer cancel() - - sb, err := msb.CreateSandbox(cctx, name, opts...) - if err != nil { - return nil, fmt.Errorf("create sandbox: %w", err) - } - s.reg.cache(name, sb) - - // Resolve the box's REAL working directory. - // - // 1. Caller pinned a workdir: ensure it exists in the guest (mkdir -p), - // then use it — the dir gets created if the image didn't ship it, - // rather than the SDK refusing to boot. - // 2. No workdir pinned: trust the image's own WORKDIR by asking the guest - // with `pwd`. - // - // Best-effort: fall back to defaultWorkdir on any error. - resolved := workdir - if resolved != "" { - quoted := shellQuote(resolved) - if _, perr := runShell(cctx, sb, ExecParams{Cmd: "mkdir -p " + quoted}); perr != nil { - // Don't fail Create on mkdir error — fall back to the image's WORKDIR. - resolved = "" - } - } - if resolved == "" { - if out, perr := runShell(cctx, sb, ExecParams{Cmd: "pwd"}); perr == nil { - if wd := strings.TrimSpace(out.Stdout); strings.HasPrefix(wd, "/") { - resolved = wd - } - } - } - resolved = defaultWorkdir(resolved) - s.reg.setWorkdir(name, resolved) - - return &Instance{ - ID: name, - Image: image, - State: StateRunning, - Workdir: resolved, - Labels: p.Labels, - }, nil + return opts } func (s *Service) Get(ctx context.Context, id string) (*Instance, error) { diff --git a/internal/core/service_test.go b/internal/core/service_test.go new file mode 100644 index 0000000..d418ce1 --- /dev/null +++ b/internal/core/service_test.go @@ -0,0 +1,55 @@ +package core + +import ( + "testing" + + msb "github.com/superradcompany/microsandbox/sdk/go" +) + +// applyOptions folds a slice of SandboxOption into a fresh SandboxConfig so +// tests can assert what buildCreateOptions actually wires through to the SDK. +func applyOptions(opts []msb.SandboxOption) msb.SandboxConfig { + var cfg msb.SandboxConfig + for _, o := range opts { + o(&cfg) + } + return cfg +} + +// TestBuildCreateOptionsDiskGB is the regression test for issue #1: +// resources.disk_gb must be honored and converted from GiB to MiB before being +// handed to the SDK's WithOCIUpperSize option. +func TestBuildCreateOptionsDiskGB(t *testing.T) { + cfg := applyOptions(buildCreateOptions(CreateParams{DiskGB: 32}, "microsandbox/python")) + if got, want := cfg.OCIUpperSizeMiB, uint32(32*1024); got != want { + t.Fatalf("OCIUpperSizeMiB = %d, want %d (32 GiB -> MiB)", got, want) + } +} + +// TestBuildCreateOptionsNoDiskGB confirms that omitting disk_gb leaves the +// overlay size at the SDK/image default (zero, i.e. unset) rather than forcing +// a value. +func TestBuildCreateOptionsNoDiskGB(t *testing.T) { + cfg := applyOptions(buildCreateOptions(CreateParams{}, "microsandbox/python")) + if cfg.OCIUpperSizeMiB != 0 { + t.Fatalf("OCIUpperSizeMiB = %d, want 0 when disk_gb is unset", cfg.OCIUpperSizeMiB) + } +} + +// TestBuildCreateOptionsResources covers the CPU/memory mapping alongside disk +// so the conversion test doesn't silently regress the neighbouring knobs. +func TestBuildCreateOptionsResources(t *testing.T) { + cfg := applyOptions(buildCreateOptions(CreateParams{CPU: 2, MemoryMB: 1024, DiskGB: 8}, "img")) + if cfg.CPUs != 2 { + t.Errorf("CPUs = %d, want 2", cfg.CPUs) + } + if cfg.MemoryMiB != 1024 { + t.Errorf("MemoryMiB = %d, want 1024", cfg.MemoryMiB) + } + if cfg.OCIUpperSizeMiB != 8*1024 { + t.Errorf("OCIUpperSizeMiB = %d, want %d", cfg.OCIUpperSizeMiB, 8*1024) + } + if cfg.Image != "img" { + t.Errorf("Image = %q, want %q", cfg.Image, "img") + } +} diff --git a/openapi.yaml b/openapi.yaml index 957d2f5..f969697 100644 --- a/openapi.yaml +++ b/openapi.yaml @@ -883,7 +883,7 @@ components: properties: cpu: { type: number, description: vCPUs } memory_mb: { type: integer, description: Memory in MiB } - disk_gb: { type: integer, description: Disk in GiB } + disk_gb: { type: integer, description: Writable overlay upper size in GiB (OCI images). Zero uses the image default. } CreateRequest: type: object From 86e125ac943cf5fcbfe647c575a15b2c22f0970c Mon Sep 17 00:00:00 2001 From: Ed Zynda Date: Mon, 29 Jun 2026 18:02:43 +0300 Subject: [PATCH 2/2] fix(core): validate disk_gb to prevent overflow and negatives - reject negative disk_gb instead of silently skipping it - add an upper bound (maxDiskGB) so the GiB->MiB conversion can't overflow the uint32 WithOCIUpperSize takes - return an error from buildCreateOptions and propagate it from Create so invalid input fails fast rather than booting a misconfigured box - cover the negative, overflow, and max in-range cases with unit tests --- internal/core/service.go | 28 +++++++++++++++----- internal/core/service_test.go | 49 ++++++++++++++++++++++++++++++++--- 2 files changed, 68 insertions(+), 9 deletions(-) diff --git a/internal/core/service.go b/internal/core/service.go index 5d8e006..cab2aa3 100644 --- a/internal/core/service.go +++ b/internal/core/service.go @@ -55,6 +55,10 @@ func (s *Service) Reconcile(ctx context.Context) (int, error) { return s.reg.Rec // Lifecycle // --------------------------------------------------------------------------- +// maxDiskGB bounds CreateParams.DiskGB so the GiB->MiB conversion stays within +// the uint32 the SDK's WithOCIUpperSize option accepts (math.MaxUint32 / 1024). +const maxDiskGB = 4194303 + // CreateParams is the provider-neutral create input. type CreateParams struct { Image string @@ -125,7 +129,10 @@ func (s *Service) Create(ctx context.Context, p CreateParams) (*Instance, error) name := newName() workdir := strings.TrimSpace(p.Workdir) - opts := buildCreateOptions(p, image) + opts, err := buildCreateOptions(p, image) + if err != nil { + return nil, err + } cctx, cancel := context.WithTimeout(ctx, s.createTO) defer cancel() @@ -175,8 +182,10 @@ func (s *Service) Create(ctx context.Context, p CreateParams) (*Instance, error) // buildCreateOptions translates the provider-neutral CreateParams into the // microsandbox SDK option slice. It is deterministic and free of side effects // (no SDK calls beyond constructing options), which keeps the CreateParams -> -// SandboxOption mapping unit-testable without booting a microVM. -func buildCreateOptions(p CreateParams, image string) []msb.SandboxOption { +// SandboxOption mapping unit-testable without booting a microVM. It returns an +// error for inputs that can't be represented safely (e.g. an out-of-range +// disk size) rather than silently dropping or wrapping them. +func buildCreateOptions(p CreateParams, image string) ([]msb.SandboxOption, error) { opts := []msb.SandboxOption{ msb.WithImage(image), msb.WithDetached(), // survive msbd restart @@ -187,8 +196,15 @@ func buildCreateOptions(p CreateParams, image string) []msb.SandboxOption { if p.CPU > 0 { opts = append(opts, msb.WithCPUs(uint8(p.CPU))) } - if p.DiskGB > 0 { - // API field is GiB; SDK option takes MiB. + if p.DiskGB != 0 { + // API field is GiB; SDK option takes MiB (uint32). Reject negatives and + // values that would overflow the MiB conversion instead of wrapping. + if p.DiskGB < 0 { + return nil, fmt.Errorf("invalid disk_gb: %d (must be non-negative)", p.DiskGB) + } + if p.DiskGB > maxDiskGB { + return nil, fmt.Errorf("invalid disk_gb: %d (exceeds maximum of %d GiB)", p.DiskGB, maxDiskGB) + } opts = append(opts, msb.WithOCIUpperSize(uint32(p.DiskGB)*1024)) } if len(p.Env) > 0 { @@ -245,7 +261,7 @@ func buildCreateOptions(p CreateParams, image string) []msb.SandboxOption { if p.AutoStopSecs > 0 { opts = append(opts, msb.WithIdleTimeout(time.Duration(p.AutoStopSecs)*time.Second)) } - return opts + return opts, nil } func (s *Service) Get(ctx context.Context, id string) (*Instance, error) { diff --git a/internal/core/service_test.go b/internal/core/service_test.go index d418ce1..2d5ba50 100644 --- a/internal/core/service_test.go +++ b/internal/core/service_test.go @@ -20,7 +20,11 @@ func applyOptions(opts []msb.SandboxOption) msb.SandboxConfig { // resources.disk_gb must be honored and converted from GiB to MiB before being // handed to the SDK's WithOCIUpperSize option. func TestBuildCreateOptionsDiskGB(t *testing.T) { - cfg := applyOptions(buildCreateOptions(CreateParams{DiskGB: 32}, "microsandbox/python")) + opts, err := buildCreateOptions(CreateParams{DiskGB: 32}, "microsandbox/python") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + cfg := applyOptions(opts) if got, want := cfg.OCIUpperSizeMiB, uint32(32*1024); got != want { t.Fatalf("OCIUpperSizeMiB = %d, want %d (32 GiB -> MiB)", got, want) } @@ -30,16 +34,55 @@ func TestBuildCreateOptionsDiskGB(t *testing.T) { // overlay size at the SDK/image default (zero, i.e. unset) rather than forcing // a value. func TestBuildCreateOptionsNoDiskGB(t *testing.T) { - cfg := applyOptions(buildCreateOptions(CreateParams{}, "microsandbox/python")) + opts, err := buildCreateOptions(CreateParams{}, "microsandbox/python") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + cfg := applyOptions(opts) if cfg.OCIUpperSizeMiB != 0 { t.Fatalf("OCIUpperSizeMiB = %d, want 0 when disk_gb is unset", cfg.OCIUpperSizeMiB) } } +// TestBuildCreateOptionsDiskGBInvalid rejects negative and out-of-range disk_gb +// instead of silently skipping or wrapping the uint32 MiB conversion. +func TestBuildCreateOptionsDiskGBInvalid(t *testing.T) { + for _, tc := range []struct { + name string + diskGB int + }{ + {"negative", -1}, + {"overflow", maxDiskGB + 1}, + } { + t.Run(tc.name, func(t *testing.T) { + if _, err := buildCreateOptions(CreateParams{DiskGB: tc.diskGB}, "img"); err == nil { + t.Fatalf("expected error for disk_gb=%d, got nil", tc.diskGB) + } + }) + } +} + +// TestBuildCreateOptionsDiskGBMax accepts the largest in-range disk_gb and +// converts it without overflowing uint32. +func TestBuildCreateOptionsDiskGBMax(t *testing.T) { + opts, err := buildCreateOptions(CreateParams{DiskGB: maxDiskGB}, "img") + if err != nil { + t.Fatalf("unexpected error at max disk_gb: %v", err) + } + cfg := applyOptions(opts) + if got, want := cfg.OCIUpperSizeMiB, uint32(maxDiskGB)*1024; got != want { + t.Fatalf("OCIUpperSizeMiB = %d, want %d", got, want) + } +} + // TestBuildCreateOptionsResources covers the CPU/memory mapping alongside disk // so the conversion test doesn't silently regress the neighbouring knobs. func TestBuildCreateOptionsResources(t *testing.T) { - cfg := applyOptions(buildCreateOptions(CreateParams{CPU: 2, MemoryMB: 1024, DiskGB: 8}, "img")) + opts, err := buildCreateOptions(CreateParams{CPU: 2, MemoryMB: 1024, DiskGB: 8}, "img") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + cfg := applyOptions(opts) if cfg.CPUs != 2 { t.Errorf("CPUs = %d, want 2", cfg.CPUs) }