Fix disk-root mount validation and stale volume recovery

Author: cursoragent

lib/builds/disk_root_test.go

@@ -120,6 +120,60 @@ func TestSetupDiskRootVolume_LeftoverDeleteError(t *testing.T) {
 	assert.Empty(t, volID)
 }
 
+func TestSetupDiskRootVolume_LeftoverInUseCleansStaleBuilder(t *testing.T) {
+	mgr, instanceMgr, volumeMgr, tempDir := setupTestManager(t)
+	defer os.RemoveAll(tempDir)
+	mgr.config.DiskRootEnabled = true
+
+	buildID := "build-1"
+	staleBuilderID := "inst-builder-build-1"
+	meta := &buildMetadata{
+		ID:              buildID,
+		Status:          StatusBuilding,
+		Request:         &CreateBuildRequest{Dockerfile: "FROM alpine"},
+		CreatedAt:       time.Now(),
+		BuilderInstance: &staleBuilderID,
+	}
+	require.NoError(t, writeMetadata(mgr.paths, meta))
+	instanceMgr.instances[staleBuilderID] = &instances.Instance{
+		StoredMetadata: instances.StoredMetadata{
+			Id:   staleBuilderID,
+			Name: "builder-build-1",
+		},
+		State: instances.StateRunning,
+	}
+
+	created := false
+	volumeMgr.createFunc = func(ctx context.Context, req volumes.CreateVolumeRequest) (*volumes.Volume, error) {
+		if !created {
+			created = true
+			return nil, volumes.ErrAlreadyExists
+		}
+		return &volumes.Volume{Id: *req.Id, Name: req.Name, SizeGb: req.SizeGb}, nil
+	}
+
+	deleteAttempts := 0
+	volumeMgr.deleteFunc = func(ctx context.Context, id string) error {
+		deleteAttempts++
+		if deleteAttempts == 1 {
+			return volumes.ErrInUse
+		}
+		return nil
+	}
+
+	volID, err := mgr.setupDiskRootVolume(context.Background(), buildID)
+
+	require.NoError(t, err)
+	assert.Equal(t, "build-disk-build-1", volID)
+	assert.Equal(t, 1, instanceMgr.deleteCallCount)
+	assert.Equal(t, 2, deleteAttempts)
+	assert.Equal(t, 2, volumeMgr.createCallCount)
+
+	metaAfter, err := readMetadata(mgr.paths, buildID)
+	require.NoError(t, err)
+	assert.Nil(t, metaAfter.BuilderInstance)
+}
+
 func TestBuilderVolumeAttachments(t *testing.T) {
 	attachments := builderVolumeAttachments("src-vol", "cfg-vol", "")
 	require.Len(t, attachments, 2)

lib/builds/manager.go

@@ -803,7 +803,16 @@ func (m *manager) setupDiskRootVolume(ctx context.Context, buildID string) (stri
 	if errors.Is(err, volumes.ErrAlreadyExists) {
 		// A previous attempt at this build crashed before cleanup. Delete
 		// the leftover volume and start fresh.
-		if delErr := m.volumeManager.DeleteVolume(ctx, volID); delErr != nil {
+		delErr := m.volumeManager.DeleteVolume(ctx, volID)
+		if errors.Is(delErr, volumes.ErrInUse) {
+			// The leftover volume may still be attached to a stale builder
+			// instance from a crashed process. Remove it and retry.
+			if cleanupErr := m.cleanupStaleBuilderInstance(ctx, buildID); cleanupErr != nil {
+				return "", fmt.Errorf("cleanup stale builder instance: %w", cleanupErr)
+			}
+			delErr = m.volumeManager.DeleteVolume(ctx, volID)
+		}
+		if delErr != nil {
 			return "", fmt.Errorf("delete leftover buildkit root volume: %w", delErr)
 		}
 		_, err = m.volumeManager.CreateVolume(ctx, volumes.CreateVolumeRequest{
@@ -818,6 +827,28 @@ func (m *manager) setupDiskRootVolume(ctx context.Context, buildID string) (stri
 	return volID, nil
 }
 
+func (m *manager) cleanupStaleBuilderInstance(ctx context.Context, buildID string) error {
+	meta, err := readMetadata(m.paths, buildID)
+	if err != nil {
+		return fmt.Errorf("read build metadata: %w", err)
+	}
+	if meta.BuilderInstance == nil || *meta.BuilderInstance == "" {
+		return nil
+	}
+
+	builderInstanceID := *meta.BuilderInstance
+	if err := m.instanceManager.DeleteInstance(ctx, builderInstanceID); err != nil && !errors.Is(err, instances.ErrNotFound) {
+		return fmt.Errorf("delete stale builder instance %s: %w", builderInstanceID, err)
+	}
+
+	meta.BuilderInstance = nil
+	if err := writeMetadata(m.paths, meta); err != nil {
+		return fmt.Errorf("clear stale builder instance metadata: %w", err)
+	}
+
+	return nil
+}
+
 // builderVolumeAttachments returns the volume attachments for a builder VM.
 // diskRootVolID is empty when the disk root feature is disabled.
 func builderVolumeAttachments(sourceVolID, configVolID, diskRootVolID string) []instances.VolumeAttachment {

lib/instances/create.go

@@ -51,6 +51,12 @@ var systemDirectories = []string{
 	"/var",
 }
 
+// allowedSystemMountPaths are explicit exceptions under system directories that
+// are required for internal platform workloads.
+var allowedSystemMountPaths = map[string]struct{}{
+	"/var/lib/buildkit": {},
+}
+
 // generateVsockCID converts first 8 chars of instance ID to a unique CID
 // CIDs 0-2 are reserved (hypervisor, loopback, host)
 // Returns value in range 3 to 4294967295
@@ -665,7 +671,7 @@ func validateVolumeAttachments(volumes []VolumeAttachment) error {
 		cleanPath := filepath.Clean(vol.MountPath)
 
 		// Check for system directories
-		if isSystemDirectory(cleanPath) {
+		if isSystemDirectory(cleanPath) && !isAllowedSystemMountPath(cleanPath) {
 			return fmt.Errorf("volume %s: cannot mount to system directory %q", vol.VolumeID, cleanPath)
 		}
 
@@ -704,6 +710,11 @@ func isSystemDirectory(path string) bool {
 	return false
 }
 
+func isAllowedSystemMountPath(path string) bool {
+	_, ok := allowedSystemMountPaths[filepath.Clean(path)]
+	return ok
+}
+
 // startAndBootVM starts the VMM and boots the VM
 func (m *manager) startAndBootVM(
 	ctx context.Context,

lib/instances/resource_limits_test.go

@@ -44,6 +44,29 @@ func TestValidateVolumeAttachments_SystemDirectory(t *testing.T) {
 	assert.Contains(t, err.Error(), "system directory")
 }
 
+func TestValidateVolumeAttachments_BuildkitRootSystemPathAllowed(t *testing.T) {
+	t.Parallel()
+	volumes := []VolumeAttachment{{
+		VolumeID:  "vol-1",
+		MountPath: "/var/lib/buildkit",
+	}}
+
+	err := validateVolumeAttachments(volumes)
+	assert.NoError(t, err)
+}
+
+func TestValidateVolumeAttachments_VarSubdirectoryStillBlocked(t *testing.T) {
+	t.Parallel()
+	volumes := []VolumeAttachment{{
+		VolumeID:  "vol-1",
+		MountPath: "/var/lib/other",
+	}}
+
+	err := validateVolumeAttachments(volumes)
+	assert.Error(t, err)
+	assert.Contains(t, err.Error(), "system directory")
+}
+
 func TestValidateVolumeAttachments_DuplicatePaths(t *testing.T) {
 	t.Parallel()
 	volumes := []VolumeAttachment{