builds: recover crashed builds by cleaning up leftover source/config volumes

lib/builds/manager.go

@@ -5,7 +5,9 @@ import (
 	"context"
 	_ "embed"
 	"encoding/json"
+	"errors"
 	"fmt"
+	"io"
 	"log/slog"
 	"net"
 	"os"
@@ -678,13 +680,8 @@ func (m *manager) executeBuild(ctx context.Context, id string, req CreateBuildRe
 	defer sourceFile.Close()
 
 	// Create volume with source (using the volume manager's archive import)
-	_, err = m.volumeManager.CreateVolumeFromArchive(ctx, volumes.CreateVolumeFromArchiveRequest{
-		Id:     &sourceVolID,
-		Name:   sourceVolID,
-		SizeGb: 10, // 10GB should be enough for most source bundles
-	}, sourceFile)
-	if err != nil {
-		return nil, fmt.Errorf("create source volume: %w", err)
+	if err := m.createBuildSourceVolume(ctx, id, sourceVolID, sourceFile); err != nil {
+		return nil, err
 	}
 	defer m.volumeManager.DeleteVolume(context.Background(), sourceVolID)
 
@@ -697,25 +694,8 @@ func (m *manager) executeBuild(ctx context.Context, id string, req CreateBuildRe
 	defer os.Remove(configVolPath) // Clean up the config disk file
 
 	// Register the config volume with the volume manager
-	_, err = m.volumeManager.CreateVolume(ctx, volumes.CreateVolumeRequest{
-		Id:     &configVolID,
-		Name:   configVolID,
-		SizeGb: 1,
-	})
-	if err != nil {
-		// If volume creation fails, try to use the disk file directly
-		// by copying it to the expected location
-		volPath := m.paths.VolumeData(configVolID)
-		if copyErr := copyFile(configVolPath, volPath); copyErr != nil {
-			return nil, fmt.Errorf("setup config volume: %w", copyErr)
-		}
-	} else {
-		// Copy our config disk over the empty volume
-		volPath := m.paths.VolumeData(configVolID)
-		if err := copyFile(configVolPath, volPath); err != nil {
-			m.volumeManager.DeleteVolume(context.Background(), configVolID)
-			return nil, fmt.Errorf("write config to volume: %w", err)
-		}
+	if err := m.registerBuildConfigVolume(ctx, id, configVolID, configVolPath); err != nil {
+		return nil, err
 	}
 	defer m.volumeManager.DeleteVolume(context.Background(), configVolID)
 
@@ -767,6 +747,96 @@ func (m *manager) executeBuild(ctx context.Context, id string, req CreateBuildRe
 	return result, nil
 }
 
+// createBuildSourceVolume creates the source volume for a build. Build volume
+// names are deterministic (build-source-<id>), so a re-run of the same build
+// after a crash (e.g. via RecoverPendingBuilds) can hit a leftover volume from
+// the interrupted attempt. Tolerate that case: remove the leftover and retry
+// the create once.
+func (m *manager) createBuildSourceVolume(ctx context.Context, buildID, volID string, source io.Reader) error {
+	req := volumes.CreateVolumeFromArchiveRequest{
+		Id:     &volID,
+		Name:   volID,
+		SizeGb: 10, // 10GB should be enough for most source bundles
+	}
+	_, err := m.volumeManager.CreateVolumeFromArchive(ctx, req, source)
+	if errors.Is(err, volumes.ErrAlreadyExists) {
+		m.logger.Info("removing leftover source volume from crashed build attempt", "build_id", buildID, "volume", volID)
+		if delErr := m.deleteLeftoverBuildVolume(ctx, buildID, volID); delErr != nil {
+			return fmt.Errorf("remove leftover source volume: %w", delErr)
+		}
+		_, err = m.volumeManager.CreateVolumeFromArchive(ctx, req, source)
+	}
+	if err != nil {
+		return fmt.Errorf("create source volume: %w", err)
+	}
+	return nil
+}
+
+// registerBuildConfigVolume registers the config disk as a volume and copies
+// the config data onto it. Like the source volume, the config volume has a
+// deterministic name (build-config-<id>), so a re-run of the same build after
+// a crash can hit a leftover from the interrupted attempt; remove it and retry
+// the create once rather than silently copying over the stale volume.
+func (m *manager) registerBuildConfigVolume(ctx context.Context, buildID, volID, configDiskPath string) error {
+	req := volumes.CreateVolumeRequest{
+		Id:     &volID,
+		Name:   volID,
+		SizeGb: 1,
+	}
+	_, err := m.volumeManager.CreateVolume(ctx, req)
+	if errors.Is(err, volumes.ErrAlreadyExists) {
+		m.logger.Info("removing leftover config volume from crashed build attempt", "build_id", buildID, "volume", volID)
+		if delErr := m.deleteLeftoverBuildVolume(ctx, buildID, volID); delErr != nil {
+			return fmt.Errorf("remove leftover config volume: %w", delErr)
+		}
+		_, err = m.volumeManager.CreateVolume(ctx, req)
+	}
+	if err != nil {
+		// If volume creation fails, try to use the disk file directly
+		// by copying it to the expected location
+		volPath := m.paths.VolumeData(volID)
+		if copyErr := copyFile(configDiskPath, volPath); copyErr != nil {
+			return fmt.Errorf("setup config volume: %w", copyErr)
+		}
+		return nil
+	}
+	// Copy our config disk over the empty volume
+	volPath := m.paths.VolumeData(volID)
+	if err := copyFile(configDiskPath, volPath); err != nil {
+		m.volumeManager.DeleteVolume(context.Background(), volID)
+		return fmt.Errorf("write config to volume: %w", err)
+	}
+	return nil
+}
+
+// deleteLeftoverBuildVolume removes a build volume left behind by a crashed
+// prior attempt of the same build. If the volume is still attached, it is
+// almost certainly attached to the crashed attempt's stale builder instance
+// (builder-<id>); delete that instance (which detaches all of its volumes) and
+// retry the volume delete. A volume attached to an unknown instance is never
+// force-deleted; an error is returned instead.
+func (m *manager) deleteLeftoverBuildVolume(ctx context.Context, buildID, volID string) error {
+	err := m.volumeManager.DeleteVolume(ctx, volID)
+	if !errors.Is(err, volumes.ErrInUse) {
+		return err
+	}
+
+	builderName := fmt.Sprintf("builder-%s", buildID)
+	inst, getErr := m.instanceManager.GetInstance(ctx, builderName)
+	if getErr != nil {
+		if errors.Is(getErr, instances.ErrNotFound) {
+			return fmt.Errorf("volume %s is in use but stale builder instance %q was not found; refusing to force-delete a volume attached to an unknown instance", volID, builderName)
+		}
+		return fmt.Errorf("look up stale builder instance %q: %w", builderName, getErr)
+	}
+
+	m.logger.Info("deleting stale builder instance from crashed build attempt", "build_id", buildID, "instance", inst.Id, "volume", volID)
+	if delErr := m.instanceManager.DeleteInstance(ctx, inst.Id); delErr != nil {
+		return fmt.Errorf("delete stale builder instance %s: %w", inst.Id, delErr)
+	}
+	return m.volumeManager.DeleteVolume(ctx, volID)
+}
+
 // waitForResult waits for the build result from the builder agent via vsock
 func (m *manager) waitForResult(ctx context.Context, buildID string, inst *instances.Instance) (*BuildResult, error) {
 	// Wait a bit for the VM to start and the builder agent to listen on vsock

lib/builds/manager_test.go

@@ -1067,6 +1067,187 @@ eventLoop:
 	}
 }
 
+// setupBuildInputs writes the on-disk source tarball and build config that
+// executeBuild expects for a build.
+func setupBuildInputs(t *testing.T, mgr *manager, buildID string) {
+	t.Helper()
+	sourceDir := mgr.paths.BuildSourceDir(buildID)
+	require.NoError(t, os.MkdirAll(sourceDir, 0755))
+	require.NoError(t, os.WriteFile(filepath.Join(sourceDir, "source.tar.gz"), []byte("fake-tarball-data"), 0644))
+	require.NoError(t, os.MkdirAll(filepath.Dir(mgr.paths.BuildConfig(buildID)), 0755))
+	require.NoError(t, os.WriteFile(mgr.paths.BuildConfig(buildID), []byte(`{"job_id":"`+buildID+`"}`), 0644))
+}
+
+// TestExecuteBuild_SourceVolumeAlreadyExists verifies that a leftover source
+// volume from a crashed prior attempt of the same build is deleted and
+// recreated, allowing the re-run to proceed past source volume creation.
+func TestExecuteBuild_SourceVolumeAlreadyExists(t *testing.T) {
+	mgr, instanceMgr, volumeMgr, tempDir := setupTestManager(t)
+	defer os.RemoveAll(tempDir)
+
+	buildID := "build-crash-src"
+	setupBuildInputs(t, mgr, buildID)
+
+	var archiveCalls int
+	var deleted []string
+	volumeMgr.createFromArchiveFunc = func(ctx context.Context, req volumes.CreateVolumeFromArchiveRequest, archive io.Reader) (*volumes.Volume, error) {
+		archiveCalls++
+		if archiveCalls == 1 {
+			// Leftover from the crashed prior attempt
+			return nil, volumes.ErrAlreadyExists
+		}
+		vol := &volumes.Volume{Id: *req.Id, Name: req.Name}
+		volumeMgr.volumes[vol.Id] = vol
+		return vol, nil
+	}
+	volumeMgr.deleteFunc = func(ctx context.Context, id string) error {
+		deleted = append(deleted, id)
+		delete(volumeMgr.volumes, id)
+		return nil
+	}
+	// Short-circuit once volume setup is done so the test doesn't enter the
+	// vsock wait loop.
+	instanceMgr.createFunc = func(ctx context.Context, req instances.CreateInstanceRequest) (*instances.Instance, error) {
+		return nil, fmt.Errorf("stop after volume setup")
+	}
+
+	_, err := mgr.executeBuild(context.Background(), buildID, CreateBuildRequest{}, &BuildPolicy{})
+
+	// The build must get past source volume creation (it may fail later, e.g.
+	// at config disk creation or instance launch in the test environment).
+	require.Error(t, err)
+	assert.NotContains(t, err.Error(), "create source volume")
+	assert.Equal(t, 2, archiveCalls, "expected delete + retry of source volume creation")
+	require.NotEmpty(t, deleted)
+	assert.Equal(t, "build-source-"+buildID, deleted[0])
+}
+
+// TestExecuteBuild_SourceVolumeInUse_StaleBuilder verifies that when the
+// leftover source volume is still attached to the crashed attempt's stale
+// builder instance, the builder is deleted (detaching the volume) before the
+// volume is deleted and recreated.
+func TestExecuteBuild_SourceVolumeInUse_StaleBuilder(t *testing.T) {
+	mgr, instanceMgr, volumeMgr, tempDir := setupTestManager(t)
+	defer os.RemoveAll(tempDir)
+
+	buildID := "build-crash-inuse"
+	setupBuildInputs(t, mgr, buildID)
+
+	// Seed the stale builder instance from the crashed attempt
+	builderName := "builder-" + buildID
+	instanceMgr.instances[builderName] = &instances.Instance{
+		StoredMetadata: instances.StoredMetadata{Id: builderName, Name: builderName},
+		State:          instances.StateRunning,
+	}
+
+	var archiveCalls int
+	var deleteCalls int
+	volumeMgr.createFromArchiveFunc = func(ctx context.Context, req volumes.CreateVolumeFromArchiveRequest, archive io.Reader) (*volumes.Volume, error) {
+		archiveCalls++
+		if archiveCalls == 1 {
+			return nil, volumes.ErrAlreadyExists
+		}
+		vol := &volumes.Volume{Id: *req.Id, Name: req.Name}
+		volumeMgr.volumes[vol.Id] = vol
+		return vol, nil
+	}
+	volumeMgr.deleteFunc = func(ctx context.Context, id string) error {
+		deleteCalls++
+		if deleteCalls == 1 {
+			// Still attached to the stale builder
+			return volumes.ErrInUse
+		}
+		delete(volumeMgr.volumes, id)
+		return nil
+	}
+	// Short-circuit once volume setup is done so the test doesn't enter the
+	// vsock wait loop.
+	instanceMgr.createFunc = func(ctx context.Context, req instances.CreateInstanceRequest) (*instances.Instance, error) {
+		return nil, fmt.Errorf("stop after volume setup")
+	}
+
+	_, err := mgr.executeBuild(context.Background(), buildID, CreateBuildRequest{}, &BuildPolicy{})
+
+	require.Error(t, err)
+	assert.NotContains(t, err.Error(), "create source volume")
+	assert.Equal(t, 2, archiveCalls, "expected delete + retry of source volume creation")
+	assert.Equal(t, 1, instanceMgr.deleteCallCount, "expected stale builder instance to be deleted")
+	_, getErr := instanceMgr.GetInstance(context.Background(), builderName)
+	assert.ErrorIs(t, getErr, instances.ErrNotFound, "stale builder instance should be gone")
+}
+
+// TestExecuteBuild_SourceVolumeInUse_NoStaleBuilder verifies that when the
+// leftover source volume is in use but the crashed attempt's builder instance
+// is gone, the build fails with a clear error and nothing is force-deleted.
+func TestExecuteBuild_SourceVolumeInUse_NoStaleBuilder(t *testing.T) {
+	mgr, instanceMgr, volumeMgr, tempDir := setupTestManager(t)
+	defer os.RemoveAll(tempDir)
+
+	buildID := "build-crash-orphan"
+	setupBuildInputs(t, mgr, buildID)
+
+	var archiveCalls int
+	volumeMgr.createFromArchiveFunc = func(ctx context.Context, req volumes.CreateVolumeFromArchiveRequest, archive io.Reader) (*volumes.Volume, error) {
+		archiveCalls++
+		return nil, volumes.ErrAlreadyExists
+	}
+	volumeMgr.deleteFunc = func(ctx context.Context, id string) error {
+		return volumes.ErrInUse
+	}
+
+	_, err := mgr.executeBuild(context.Background(), buildID, CreateBuildRequest{}, &BuildPolicy{})
+
+	require.Error(t, err)
+	assert.Contains(t, err.Error(), "refusing to force-delete")
+	assert.Equal(t, 1, archiveCalls, "create must not be retried when the leftover cannot be removed")
+	assert.Equal(t, 0, instanceMgr.deleteCallCount, "no instance should be deleted")
+}
+
+// TestRegisterBuildConfigVolume_AlreadyExists verifies that a leftover config
+// volume from a crashed prior attempt of the same build is explicitly deleted
+// and recreated rather than silently masked by the copy-over fallback.
+func TestRegisterBuildConfigVolume_AlreadyExists(t *testing.T) {
+	mgr, _, volumeMgr, tempDir := setupTestManager(t)
+	defer os.RemoveAll(tempDir)
+
+	buildID := "build-crash-config"
+	configVolID := "build-config-" + buildID
+
+	// Dummy config disk to copy over the recreated volume
+	configData := []byte("fake-ext4-config-disk")
+	configDiskPath := filepath.Join(tempDir, "config.ext4")
+	require.NoError(t, os.WriteFile(configDiskPath, configData, 0644))
+
+	var createCalls int
+	var deleted []string
+	volumeMgr.createFunc = func(ctx context.Context, req volumes.CreateVolumeRequest) (*volumes.Volume, error) {
+		createCalls++
+		if createCalls == 1 {
+			// Leftover from the crashed prior attempt
+			return nil, volumes.ErrAlreadyExists
+		}
+		vol := &volumes.Volume{Id: *req.Id, Name: req.Name}
+		volumeMgr.volumes[vol.Id] = vol
+		return vol, nil
+	}
+	volumeMgr.deleteFunc = func(ctx context.Context, id string) error {
+		deleted = append(deleted, id)
+		delete(volumeMgr.volumes, id)
+		return nil
+	}
+
+	err := mgr.registerBuildConfigVolume(context.Background(), buildID, configVolID, configDiskPath)
+
+	require.NoError(t, err)
+	assert.Equal(t, 2, createCalls, "expected delete + retry of config volume creation")
+	assert.Equal(t, []string{configVolID}, deleted)
+	assert.Contains(t, volumeMgr.volumes, configVolID, "config volume should be recreated")
+	// The config data must have been copied onto the recreated volume
+	copied, readErr := os.ReadFile(mgr.paths.VolumeData(configVolID))
+	require.NoError(t, readErr)
+	assert.Equal(t, configData, copied)
+}
+
 func TestExtractInternalBaseImageRepos(t *testing.T) {
 	registryURL := "http://10.102.0.1:8085"