builders: add Builder resource domain with persistent cache disks

[rgarcia]

Introduce lib/builders: a Builder is a first-class resource and the unit
of build-cache isolation. Each builder owns one persistent ext4
sparse-file volume (builder-disk-, tagged hypeman.system/managed-by=
builder) provisioned eagerly at create and later attached by disposable
builder VMs at /var/lib/buildkit.

The manager provides Create/Get/List/Delete, an internal
AcquireForBuild/ReleaseBuild seam (one build at a time per builder), and
ResetDisk for pruning. Status (ready, pruning, deleting, error) is
persisted before side effects so interrupted deletes and prunes are
resumed by startup reconciliation, which also recreates missing disks
(best-effort, empty) and clears stale disk attachments: records whose
instance is gone are detached, records backed by a surviving stale VM are
cleared by deleting that VM and re-fetching the volume. Delete and prune
return ErrInUse while a builder is acquired, attached, or mid-transition.
AcquireForBuild clears stale disk attachments itself: an attachment on an
unheld, ready builder can only be a leaked record or a stale VM from a
crashed build, so the next build self-heals instead of failing until a
restart runs reconciliation. Config covers max_count, default/max disk
size, and an idle TTL reaper that is destructive and disabled by default.

Public volume APIs now reject caller-supplied IDs with the reserved
builder-disk- prefix and tags in the hypeman.system/ namespace on both
create paths, reject deletion of reserved disks, and instance creation
rejects attaching reserved volumes unless the internal
AllowSystemVolumeMounts opt-in is set (never populated from API
requests). Add builder:read/write/delete scope constants and docs.

Tests cover CRUD, quotas and disk-size validation, failed-create cleanup,
restart loading, corrupt metadata reporting, exclusivity, last_used stamping,
lazy disk recreation, deterministic prune acceptance, reconciliation of
missing disks/orphan attachments/stale instances/interrupted transitions,
stale-attachment clearing at acquire (including loud failure), and the idle
reaper through the shared delete path.

Follow-up (not blocking)

Individual create, acquire, delete, and reset transitions still hold the single manager mutex across volume I/O to avoid ownership races. Idle reaping no longer holds it across the full scan and delegates deletion to the shared crash-safe path. Follow-up: replace the global transition lock with per-builder locks if cross-builder disk I/O becomes a throughput bottleneck.

---

_{Stack created with GitHub Stacks CLI • Give Feedback 💬}

---

Note

Medium Risk
Large new domain with reconciliation that can delete stale instances and irreversibly reap idle builders; volume/instance guardrails reduce spoofing risk but internal attach paths must stay API-inaccessible.

Overview
Introduces Builder as a first-class resource in lib/builders: each builder gets an eagerly provisioned persistent disk (builder-disk-<id>), CRUD plus AcquireForBuild / ReleaseBuild (one build at a time), ResetDisk (async prune), startup reconciliation for interrupted deletes/prunes/missing disks, stale attachment cleanup (including deleting stale builder VMs), and an optional idle TTL reaper. Status transitions are persisted before side effects so crashes are recoverable.

Public API hardening: volume create (including from archive) rejects reserved builder-disk- IDs and hypeman.system/ tag keys; delete rejects reserved disks. Instance volume validation blocks attaching reserved volumes unless internal AllowSystemVolumeMounts / SystemVolumeMountPaths are set (not exposed on the API). Adds builder:read|write|delete scopes and builder metadata paths under dataDir/builders/.

^{Reviewed by Cursor Bugbot for commit 3ae855e. Bugbot is set up for automated code reviews on this repo. Configure here.}

[cursor]

Cursor Bugbot has reviewed your changes using high effort and found 2 potential issues.

Autofix Details

Bugbot Autofix prepared fixes for both issues found in the latest run.

• ✅ Fixed: Idle reaper wedges deleting builders
  • Updated reapIdle to also process builders already in deleting status so failed volume deletes are retried on later reaper ticks.
• ✅ Fixed: Release clears hold before finish
  • ReleaseBuild now clears the in-memory acquisition only after last_used_at is persisted (or the builder is already gone), preserving retryable release semantics.

Or push these changes by commenting:

@cursor push 1f157c52f3

Preview (1f157c52f3)

diff --git a/lib/builders/manager.go b/lib/builders/manager.go
--- a/lib/builders/manager.go
+++ b/lib/builders/manager.go
@@ -337,19 +337,24 @@
 	if holder != buildID {
 		return fmt.Errorf("builder %s is acquired by build %s, not %s", id, holder, buildID)
 	}
-	delete(m.acquired, id)
 
 	// Record usage even for failed builds: last_used_at drives idle TTL.
 	meta, err := loadMetadata(m.paths, id)
 	if err != nil {
 		if errors.Is(err, ErrNotFound) {
+			delete(m.acquired, id)
 			return nil // builder was deleted while held
 		}
 		return err
 	}
 	now := time.Now()
 	meta.LastUsedAt = &now
-	return saveMetadata(m.paths, meta)
+	if err := saveMetadata(m.paths, meta); err != nil {
+		return err
+	}
+
+	delete(m.acquired, id)
+	return nil
 }
 
 // ResetDisk resets a builder's cache by recreating its disk asynchronously
@@ -586,35 +591,41 @@
 		if err != nil {
 			continue
 		}
-		if meta.Status != StatusReady {
+		if meta.Status != StatusReady && meta.Status != StatusDeleting {
 			continue
 		}
-		if _, held := m.acquired[id]; held {
-			continue
-		}
-		lastActivity := meta.CreatedAt
-		if meta.LastUsedAt != nil {
-			lastActivity = *meta.LastUsedAt
-		}
-		if lastActivity.After(cutoff) {
-			continue
-		}
 
-		attached, err := m.diskAttached(ctx, meta.DiskVolumeID)
-		if err != nil {
-			m.logger.Error("idle reaper failed to check builder disk", "id", id, "error", err)
-			continue
+		if meta.Status == StatusReady {
+			if _, held := m.acquired[id]; held {
+				continue
+			}
+			lastActivity := meta.CreatedAt
+			if meta.LastUsedAt != nil {
+				lastActivity = *meta.LastUsedAt
+			}
+			if lastActivity.After(cutoff) {
+				continue
+			}
+
+			attached, err := m.diskAttached(ctx, meta.DiskVolumeID)
+			if err != nil {
+				m.logger.Error("idle reaper failed to check builder disk", "id", id, "error", err)
+				continue
+			}
+			if attached {
+				continue
+			}
+
+			m.logger.Info("deleting idle builder", "id", id, "last_activity", lastActivity)
+			meta.Status = StatusDeleting
+			if err := saveMetadata(m.paths, meta); err != nil {
+				m.logger.Error("idle reaper failed to mark builder deleting", "id", id, "error", err)
+				continue
+			}
+		} else {
+			m.logger.Info("resuming idle builder delete", "id", id)
 		}
-		if attached {
-			continue
-		}
 
-		m.logger.Info("deleting idle builder", "id", id, "last_activity", lastActivity)
-		meta.Status = StatusDeleting
-		if err := saveMetadata(m.paths, meta); err != nil {
-			m.logger.Error("idle reaper failed to mark builder deleting", "id", id, "error", err)
-			continue
-		}
 		if err := m.volumeManager.DeleteVolume(ctx, meta.DiskVolumeID); err != nil && !errors.Is(err, volumes.ErrNotFound) {
 			m.logger.Error("idle reaper failed to delete builder disk", "id", id, "error", err)
 			continue

diff --git a/lib/builders/manager_test.go b/lib/builders/manager_test.go
--- a/lib/builders/manager_test.go
+++ b/lib/builders/manager_test.go
@@ -24,6 +24,11 @@
 	deleted    []string
 }
 
+type flakyDeleteVolumeManager struct {
+	volumes.Manager
+	failDelete map[string]int
+}
+
 func (m *mockInstanceChecker) GetInstance(ctx context.Context, idOrName string) (*instances.Instance, error) {
 	if m.getErr != nil {
 		return nil, m.getErr
@@ -43,6 +48,14 @@
 	return nil
 }
 
+func (m *flakyDeleteVolumeManager) DeleteVolume(ctx context.Context, id string) error {
+	if remaining := m.failDelete[id]; remaining > 0 {
+		m.failDelete[id] = remaining - 1
+		return errors.New("transient delete failure")
+	}
+	return m.Manager.DeleteVolume(ctx, id)
+}
+
 func setupTestManager(t *testing.T, cfg Config) (*manager, volumes.Manager, *mockInstanceChecker, *paths.Paths) {
 	t.Helper()
 	p := paths.New(t.TempDir())
@@ -241,6 +254,33 @@
 	assert.WithinDuration(t, time.Now(), *got.LastUsedAt, time.Minute)
 }
 
+func TestReleaseBuild_KeepsHoldWhenPersistFails(t *testing.T) {
+	mgr, _, _, p := setupTestManager(t, Config{})
+
+	b, err := mgr.CreateBuilder(context.Background(), CreateBuilderRequest{})
+	require.NoError(t, err)
+
+	_, err = mgr.AcquireForBuild(context.Background(), b.ID, "build-1")
+	require.NoError(t, err)
+
+	require.NoError(t, os.Chmod(p.BuilderDir(b.ID), 0555))
+	t.Cleanup(func() {
+		_ = os.Chmod(p.BuilderDir(b.ID), 0755)
+	})
+
+	err = mgr.ReleaseBuild(context.Background(), b.ID, "build-1")
+	require.Error(t, err)
+
+	_, err = mgr.AcquireForBuild(context.Background(), b.ID, "build-2")
+	assert.ErrorIs(t, err, ErrInUse)
+
+	require.NoError(t, os.Chmod(p.BuilderDir(b.ID), 0755))
+	require.NoError(t, mgr.ReleaseBuild(context.Background(), b.ID, "build-1"))
+
+	_, err = mgr.AcquireForBuild(context.Background(), b.ID, "build-2")
+	require.NoError(t, err)
+}
+
 func TestAcquireForBuild_RecreatesMissingDisk(t *testing.T) {
 	mgr, volumeMgr, _, _ := setupTestManager(t, Config{})
 
@@ -501,6 +541,37 @@
 	assert.NoError(t, err, "acquired builder must not be reaped")
 }
 
+func TestIdleReaper_RetriesDeletingBuilder(t *testing.T) {
+	m, volumeMgr, _, p := setupTestManager(t, Config{IdleTTL: time.Hour})
+
+	b, err := m.CreateBuilder(context.Background(), CreateBuilderRequest{})
+	require.NoError(t, err)
+
+	meta, err := loadMetadata(p, b.ID)
+	require.NoError(t, err)
+	old := time.Now().Add(-2 * time.Hour)
+	meta.LastUsedAt = &old
+	require.NoError(t, saveMetadata(p, meta))
+
+	m.volumeManager = &flakyDeleteVolumeManager{
+		Manager:    volumeMgr,
+		failDelete: map[string]int{b.DiskVolumeID: 1},
+	}
+
+	m.reapIdle(context.Background())
+
+	got, err := m.GetBuilder(context.Background(), b.ID)
+	require.NoError(t, err)
+	assert.Equal(t, StatusDeleting, got.Status)
+
+	m.reapIdle(context.Background())
+
+	_, err = m.GetBuilder(context.Background(), b.ID)
+	assert.ErrorIs(t, err, ErrNotFound)
+	_, err = volumeMgr.GetVolume(context.Background(), b.DiskVolumeID)
+	assert.ErrorIs(t, err, volumes.ErrNotFound)
+}
+
 func TestValidateBuilderID(t *testing.T) {
 	assert.NoError(t, ValidateBuilderID("abc"))
 	assert.NoError(t, ValidateBuilderID("team-cache_1"))

_{You can send follow-ups to the cloud agent here.}

[cursor]

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

Autofix Details

Bugbot Autofix prepared a fix for the issue found in the latest run.

• ✅ Fixed: System path blocks builder mounts
  • Updated volume-attachment validation so internal requests with AllowSystemVolumeMounts only bypass system-directory checks for reserved volume IDs, allowing /var/lib/buildkit builder disk mounts while keeping non-reserved mounts blocked.

Or push these changes by commenting:

@cursor push ee257f475b

Preview (ee257f475b)

diff --git a/lib/instances/create.go b/lib/instances/create.go
--- a/lib/instances/create.go
+++ b/lib/instances/create.go
@@ -658,6 +658,9 @@
 
 	seenPaths := make(map[string]bool)
 	for _, vol := range attachments {
+		reservedPrefix := volumes.ReservedVolumeIDPrefix(vol.VolumeID)
+		isReservedVolume := reservedPrefix != ""
+
 		// Validate mount path is absolute
 		if !filepath.IsAbs(vol.MountPath) {
 			return fmt.Errorf("volume %s: mount path %q must be absolute", vol.VolumeID, vol.MountPath)
@@ -666,16 +669,15 @@
 		// Clean the path to normalize it
 		cleanPath := filepath.Clean(vol.MountPath)
 
-		// Check for system directories
-		if isSystemDirectory(cleanPath) {
+		// Check for system directories. Internal instances may only bypass this
+		// restriction when attaching reserved internal volumes.
+		if isSystemDirectory(cleanPath) && !(allowSystemVolumes && isReservedVolume) {
 			return fmt.Errorf("volume %s: cannot mount to system directory %q", vol.VolumeID, cleanPath)
 		}
 
 		// Reserved internal volume IDs are attachable only by internal instances
-		if !allowSystemVolumes {
-			if prefix := volumes.ReservedVolumeIDPrefix(vol.VolumeID); prefix != "" {
-				return fmt.Errorf("volume %s: volume IDs with the prefix %q are reserved for internal use", vol.VolumeID, prefix)
-			}
+		if !allowSystemVolumes && isReservedVolume {
+			return fmt.Errorf("volume %s: volume IDs with the prefix %q are reserved for internal use", vol.VolumeID, reservedPrefix)
 		}
 
 		// Check for duplicate mount paths

diff --git a/lib/instances/resource_limits_test.go b/lib/instances/resource_limits_test.go
--- a/lib/instances/resource_limits_test.go
+++ b/lib/instances/resource_limits_test.go
@@ -251,4 +251,19 @@
 		MountPath: "/mnt/data",
 	}}, false)
 	assert.NoError(t, err)
+
+	// Internal instances may mount reserved internal volumes under system paths.
+	err = validateVolumeAttachments([]VolumeAttachment{{
+		VolumeID:  "builder-disk-abc123",
+		MountPath: "/var/lib/buildkit",
+	}}, true)
+	assert.NoError(t, err)
+
+	// The system-path bypass does not apply to non-reserved volume IDs.
+	err = validateVolumeAttachments([]VolumeAttachment{{
+		VolumeID:  "vol-1",
+		MountPath: "/var/lib/buildkit",
+	}}, true)
+	assert.Error(t, err)
+	assert.Contains(t, err.Error(), "system directory")
 }

_{You can send follow-ups to the cloud agent here.}

[sjmiller609]

bugs

• lib/builders/manager.go:159 — oversize disk_size_gb is an untyped error → handler 500s on user input

findings

• One thing I’m looking at is if we could introduce another way to do builds later and this is the only thing that hard assumes buildkit. There’s only a couple places so might as well keep it abstract
  • lib/builders/types.go:2-3,35 — package + struct doc comments define Builder as a BuildKit cache disk at /var/lib/buildkit (docs only, no code)
  • lib/instances/create.go — allowedSystemMountPaths hardcodes /var/lib/buildkit in the instances domain
• lib/builders/manager.go:203 — rollback on disk-create failure can orphan a reserved volume invisible to reconcile; same-ID retry then 500s forever
• lib/builders/manager.go:619-640 — reapIdle re-implements delete with different crash semantics, could extract shared
• lock hygiene — reapIdle:580 (not sure, but take a look) holds m.mu across full scan + volume I/O; resetDisk:410 across disk delete+create; AcquireForBuild:313 can delete an instance under the lock
• lib/builders/storage.go:96 — stat/load errors silently hide a builder from list/reconcile/reaper while its disk persists
• the "hands off builder-disk-* volumes" rule is repeated in each API handler instead of enforced once in the volume manager.
• lib/builders/metrics.go:29 — add “status” label

nits

• storage.go:27 — DiskVolumeID persisted but derivable; two sources of truth.
• manager.go:207 — create-duration metric only records success (add label for this?)
• manager.go:173 — deleting/error builders count toward MaxCount (I think this makes sense but double checking)
• inconsistent status codes for the same offense: creating a volume with a reserved ID (builder-disk-*) returns 400 bad request, but deleting one returns 409 conflict. Should it be the same error?
• builder_instance_id vs builder_id is a bit confusing of naming. I’m wondering if a builder should be called a build cache or something - subjective just to think about

[rgarcia]

@sjmiller609 addressed the actionable domain feedback in 732065c:

• keeps failed-create metadata until any partially-created reserved disk is cleaned up, so reconciliation and same-ID recovery cannot lose ownership
• routes idle reaping through DeleteBuilder and no longer holds the manager lock across the full scan
• reports metadata/stat/reconciliation failures instead of silently hiding builders; Start now returns reconciliation failures
• derives the disk volume ID instead of persisting a second source of truth
• records create failures and labels the builder-count gauge by lifecycle status
• removes BuildKit-specific package/instance-domain coupling; internal callers now provide their exact allowed system mount paths
• retains the existing typed oversized-disk error and adds typed negative-size validation

I kept the global lock around individual ownership transitions and volume I/O because releasing it there creates acquisition/delete TOCTOU races; moving to per-builder locks is a separate concurrency change. I also kept reserved-volume enforcement at the public API boundary because the same volume manager is intentionally used by trusted internal managers; central enforcement needs a distinct privileged volume interface rather than a prefix check that would block Builder cleanup. Error/deleting builders continue counting toward quota because they still own metadata or disk state. Creating a reserved ID is invalid input (400), while deleting an existing internally-owned resource is a state conflict (409).

[cursor]

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

Bugbot Autofix prepared a fix for the issue found in the latest run.

• ✅ Fixed: Reaper skips stale attached builders
  • Idle reaping and interrupted-delete reconciliation now clear stale disk attachments for unheld builders before retrying delete, so stale attachments no longer block deletion.

Or push these changes by commenting:

@cursor push 18d842f823

Preview (18d842f823)

diff --git a/lib/builders/manager.go b/lib/builders/manager.go
--- a/lib/builders/manager.go
+++ b/lib/builders/manager.go
@@ -502,6 +502,12 @@
 		switch meta.Status {
 		case StatusDeleting:
 			m.logger.Info("resuming interrupted builder delete", "id", id)
+			if err := m.clearStaleAttachmentsIfUnheld(ctx, id, StatusDeleting); err != nil {
+				if !errors.Is(err, ErrInUse) && !errors.Is(err, ErrNotFound) {
+					reconcileErrs = append(reconcileErrs, fmt.Errorf("clear builder %s stale attachments before resumed delete: %w", id, err))
+				}
+				continue
+			}
 			if err := m.DeleteBuilder(ctx, id); err != nil {
 				reconcileErrs = append(reconcileErrs, fmt.Errorf("resume builder %s delete: %w", id, err))
 			}
@@ -579,6 +585,25 @@
 	return nil
 }
 
+// clearStaleAttachmentsIfUnheld only clears attachments when the builder is
+// still in the expected status and no build currently holds it.
+func (m *manager) clearStaleAttachmentsIfUnheld(ctx context.Context, id string, expectedStatus string) error {
+	m.mu.Lock()
+	defer m.mu.Unlock()
+
+	meta, err := loadMetadata(m.paths, id)
+	if err != nil {
+		return err
+	}
+	if meta.Status != expectedStatus {
+		return ErrInUse
+	}
+	if _, held := m.acquired[id]; held {
+		return ErrInUse
+	}
+	return m.clearStaleAttachments(ctx, meta.diskVolumeID())
+}
+
 // runIdleReaper periodically deletes builders idle past the configured TTL.
 // Deletion is total and irreversible; the reaper only runs when IdleTTL > 0.
 func (m *manager) runIdleReaper(ctx context.Context) {
@@ -619,6 +644,12 @@
 			continue
 		}
 		if meta.Status == StatusDeleting {
+			if err := m.clearStaleAttachmentsIfUnheld(ctx, id, StatusDeleting); err != nil {
+				if !errors.Is(err, ErrInUse) && !errors.Is(err, ErrNotFound) {
+					m.logger.Error("idle reaper failed to clear stale attachments for deleting builder", "id", id, "error", err)
+				}
+				continue
+			}
 			if err := m.DeleteBuilder(ctx, id); err != nil && !errors.Is(err, ErrInUse) && !errors.Is(err, ErrNotFound) {
 				m.logger.Error("idle reaper failed to resume builder delete", "id", id, "error", err)
 			}
@@ -634,6 +665,12 @@
 		if lastActivity.After(cutoff) {
 			continue
 		}
+		if err := m.clearStaleAttachmentsIfUnheld(ctx, id, StatusReady); err != nil {
+			if !errors.Is(err, ErrInUse) && !errors.Is(err, ErrNotFound) {
+				m.logger.Error("idle reaper failed to clear stale attachments for ready builder", "id", id, "error", err)
+			}
+			continue
+		}
 
 		m.logger.Info("deleting idle builder", "id", id, "last_activity", lastActivity)
 		if err := m.DeleteBuilder(ctx, id); err != nil && !errors.Is(err, ErrInUse) && !errors.Is(err, ErrNotFound) {

diff --git a/lib/builders/manager_test.go b/lib/builders/manager_test.go
--- a/lib/builders/manager_test.go
+++ b/lib/builders/manager_test.go
@@ -488,6 +488,9 @@
 
 	b, err := mgr.CreateBuilder(context.Background(), CreateBuilderRequest{})
 	require.NoError(t, err)
+	require.NoError(t, volumeMgr.AttachVolume(context.Background(), b.DiskVolumeID, volumes.AttachVolumeRequest{
+		InstanceID: "inst-gone", MountPath: "/var/lib/buildkit",
+	}))
 
 	// Crash mid-delete: status persisted as deleting, disk and metadata remain.
 	meta, err := loadMetadata(p, b.ID)
@@ -529,6 +532,29 @@
 	assert.NoError(t, err, "interrupted prune must finish recreating the disk")
 }
 
+func TestIdleReaper_ClearsStaleAttachmentsBeforeDelete(t *testing.T) {
+	m, volumeMgr, _, p := setupTestManager(t, Config{IdleTTL: time.Hour})
+
+	b, err := m.CreateBuilder(context.Background(), CreateBuilderRequest{})
+	require.NoError(t, err)
+
+	meta, err := loadMetadata(p, b.ID)
+	require.NoError(t, err)
+	old := time.Now().Add(-2 * time.Hour)
+	meta.LastUsedAt = &old
+	require.NoError(t, saveMetadata(p, meta))
+	require.NoError(t, volumeMgr.AttachVolume(context.Background(), b.DiskVolumeID, volumes.AttachVolumeRequest{
+		InstanceID: "inst-gone", MountPath: "/var/lib/buildkit",
+	}))
+
+	m.reapIdle(context.Background())
+
+	_, err = m.GetBuilder(context.Background(), b.ID)
+	assert.ErrorIs(t, err, ErrNotFound)
+	_, err = volumeMgr.GetVolume(context.Background(), b.DiskVolumeID)
+	assert.ErrorIs(t, err, volumes.ErrNotFound)
+}
+
 func TestIdleReaper(t *testing.T) {
 	m, volumeMgr, _, p := setupTestManager(t, Config{IdleTTL: time.Hour})

_{You can send follow-ups to the cloud agent here.}

^{Reviewed by Cursor Bugbot for commit 732065c. Configure here.}