Fix builder release and idle reaper retry behavior

Author: cursoragent

lib/builders/manager.go

@@ -337,19 +337,24 @@ func (m *manager) ReleaseBuild(ctx context.Context, id string, buildID string) e
 	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 @@ func (m *manager) reapIdle(ctx context.Context) {
 		if err != nil {
 			continue
 		}
-		if meta.Status != StatusReady {
-			continue
-		}
-		if _, held := m.acquired[id]; held {
-			continue
-		}
-		lastActivity := meta.CreatedAt
-		if meta.LastUsedAt != nil {
-			lastActivity = *meta.LastUsedAt
-		}
-		if lastActivity.After(cutoff) {
+		if meta.Status != StatusReady && meta.Status != StatusDeleting {
 			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
-		}
+		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
+			}
 
-		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
+			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 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

lib/builders/manager_test.go

@@ -24,6 +24,11 @@ type mockInstanceChecker struct {
 	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 @@ func (m *mockInstanceChecker) DeleteInstance(ctx context.Context, id string) err
 	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 @@ func TestReleaseBuild_StampsLastUsed(t *testing.T) {
 	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 @@ func TestIdleReaper(t *testing.T) {
 	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"))