fix(provider): reclaim refused warm reservations

CHANGELOG.md

@@ -2,6 +2,13 @@
 
 ## Unreleased
 
+- Release a warm worker's exact memory reservation when placement refuses
+  creation. Successful capacity deferrals previously skipped cleanup and
+  retained uncreated workers until the admission lease expired. Early
+  argument failures now preserve the cleanup identity, cleanup failures are
+  reported, and ambiguous create responses retain their reservation for
+  authoritative reconciliation.
+
 - Classify warm-preemption and instance-delete waits as capacity, not
   timeout. A create that reclaims a warm slot and hits `context deadline
   exceeded` or Incus `instance is busy running a start` used to page

config/example-runner-1.yaml

@@ -9,7 +9,7 @@ control_plane:
   manager_version: v0.2.1-nddev.89
   scheduling_mode: scale-set
   provider: incus
-  provider_version: v0.1.5-nddev.124
+  provider_version: v0.1.5-nddev.125
   provider_interface: v0.1.0
   worker_kind: incus-container
   runner: actions/runner

config/example-runner-2.yaml

@@ -9,7 +9,7 @@ control_plane:
   manager_version: v0.2.1-nddev.89
   scheduling_mode: scale-set
   provider: incus
-  provider_version: v0.1.5-nddev.124
+  provider_version: v0.1.5-nddev.125
   provider_interface: v0.1.0
   worker_kind: incus-container
   runner: actions/runner

config/example-runner-3.yaml

@@ -9,7 +9,7 @@ control_plane:
   manager_version: v0.2.1-nddev.89
   scheduling_mode: scale-set
   provider: incus
-  provider_version: v0.1.5-nddev.124
+  provider_version: v0.1.5-nddev.125
   provider_interface: v0.1.0
   worker_kind: incus-container
   runner: actions/runner

config/example-runner-4.yaml

@@ -9,7 +9,7 @@ control_plane:
   manager_version: v0.2.1-nddev.89
   scheduling_mode: scale-set
   provider: incus
-  provider_version: v0.1.5-nddev.124
+  provider_version: v0.1.5-nddev.125
   provider_interface: v0.1.0
   worker_kind: incus-container
   runner: actions/runner

config/example-services.yaml

@@ -27,7 +27,7 @@ control_plane:
   manager_version: v0.2.1-nddev.89
   scheduling_mode: scale-set
   provider: incus
-  provider_version: v0.1.5-nddev.124
+  provider_version: v0.1.5-nddev.125
   provider_interface: v0.1.0
   worker_kind: incus-container
   runner: actions/runner

config/provider-derivative.yaml

@@ -16,7 +16,7 @@ artifact: garm-provider-incus
 # state all move together, because all three derive from here. A provider change
 # that does not bump it ships under the previous version, which is exactly how
 # runner-1 and runner-2 diverged.
-derivative_version: v0.1.5-nddev.124
+derivative_version: v0.1.5-nddev.125
 
 # The external-provider protocol GARM speaks to this binary. It moves on its own
 # schedule -- a provider release does not imply an interface release -- so it is
@@ -37,8 +37,8 @@ runtime:
   queue_intent_schema_version: 6
 
 build:
-  source_commit: 28bea87a35a1f4e80ba6f18fb922829c7afa834d
-  binary_sha256: 9ad4bae4b9b8f2625fee5baf1f31f2a05085ebbecb5b2aba6741a52f961b328f
+  source_commit: e38153e6e9ca42cdeb74d95c812bad981101db47
+  binary_sha256: a381f312e29f1407f6fd678b2b75989b156b99a268376978e5911e40c534d2dc
   go_version: go1.26.7
   cgo_enabled: false
   target_os: linux

internal/garmproviderincus/provider/warm.go

@@ -288,33 +288,42 @@ func (l *Incus) createWarm(ctx context.Context, flavor string) (name string, con
 	if !decision.Admitted {
 		return "", false, &decision, nil
 	}
+	reservedName := name
 	created := false
+	launchAttempted := false
 	defer func() {
-		if err == nil {
+		if err == nil && deferred == nil {
+			return
+		}
+		// An uncertain create can still complete in Incus. Its reservation
+		// belongs to inventory reconciliation until existence is established.
+		if launchAttempted && !created && deferred == nil {
 			return
 		}
 		cleanupContext, cancel := context.WithTimeout(context.Background(), 2*time.Minute)
 		defer cancel()
 		var cleanupErr error
 		if created {
-			cleanupErr = l.DeleteInstance(cleanupContext, name)
+			cleanupErr = l.DeleteInstance(cleanupContext, reservedName)
 		} else {
-			cleanupErr = l.admission.Release(cleanupContext, name)
+			cleanupErr = l.admission.Release(cleanupContext, reservedName)
 		}
 		if cleanupErr != nil {
-			err = errors.Wrapf(err, "cleaning failed warm instance %q: %v", name, cleanupErr)
+			err = errors2.Join(err, errors.Wrapf(cleanupErr, "cleaning failed warm instance %q", reservedName))
 		}
 	}()
 	args, err := l.getWarmCreateArgs(ctx, flavor, name)
 	if err != nil {
 		return "", false, nil, err
 	}
+	launchAttempted = true
 	if err = l.launchInstance(ctx, args); err != nil {
 		if isPlacementRefusal(err) {
 			// The scriptlet is the per-member truth and fleet-level admission
 			// is the ledger; when they disagree under load, the member that
 			// had ledger room lacked live room. That is a capacity deferral,
-			// not a failed reconcile.
+			// not a failed reconcile. Cleanup must still release the exact
+			// reservation: named return values clear both name and err here.
 			return "", false, &admission.Decision{Admitted: false, Reason: admission.ReasonPlacementRefused, Pool: flavor}, nil
 		}
 		return "", false, nil, errors.Wrap(err, "launching warm instance")

internal/garmproviderincus/provider/warm_reservation_test.go

@@ -0,0 +1,108 @@
+package provider
+
+import (
+	"context"
+	"errors"
+	"testing"
+
+	"github.com/NDDev-OpenNetwork/github-actions/internal/admission"
+	"github.com/stretchr/testify/mock"
+	"github.com/stretchr/testify/require"
+)
+
+type trackedWarmAdmission struct {
+	allowAllAdmission
+	reserved   map[string]bool
+	released   []string
+	releaseErr error
+}
+
+func (a *trackedWarmAdmission) AdmitWarm(_ context.Context, _ InstanceServerInterface, _, name string) (admission.Decision, error) {
+	if a.reserved == nil {
+		a.reserved = make(map[string]bool)
+	}
+	a.reserved[name] = true
+	return admission.Decision{Admitted: true}, nil
+}
+
+func (a *trackedWarmAdmission) Release(ctx context.Context, name string) error {
+	if err := ctx.Err(); err != nil {
+		return err
+	}
+	a.released = append(a.released, name)
+	if a.releaseErr != nil {
+		return a.releaseErr
+	}
+	delete(a.reserved, name)
+	return nil
+}
+
+func TestWarmPlacementDeferralReleasesReservationBeforeNextAttempt(t *testing.T) {
+	cli := new(MockIncusServer)
+	p := newTestProvider(cli)
+	a := &trackedWarmAdmission{}
+	p.admission = a
+	prepareCreateMocks(cli, testImageDigest)
+	cli.On("CreateInstance", mock.Anything).Return(new(MockOperation), errors.New(
+		"Failed instance placement scriptlet: insufficient-memory: no fleet member has room for this worker",
+	))
+	for range 3 {
+		name, consumed, deferred, err := p.createWarm(t.Context(), "nddev-linux-standard")
+		require.NoError(t, err)
+		require.Empty(t, name)
+		require.False(t, consumed)
+		require.NotNil(t, deferred)
+		require.Equal(t, admission.ReasonPlacementRefused, deferred.Reason)
+		require.Empty(t, a.reserved, "a refused warm create must not consume the next job's memory")
+	}
+	require.Len(t, a.released, 3)
+	for _, name := range a.released {
+		require.Contains(t, name, "warm-standard-")
+	}
+	cli.AssertNotCalled(t, "DeleteInstance", mock.Anything)
+}
+
+func TestWarmPlacementDeferralReportsReleaseFailure(t *testing.T) {
+	cli := new(MockIncusServer)
+	p := newTestProvider(cli)
+	a := &trackedWarmAdmission{releaseErr: errors.New("journal unavailable")}
+	p.admission = a
+	prepareCreateMocks(cli, testImageDigest)
+	ctx, cancel := context.WithCancel(t.Context())
+	defer cancel()
+	cli.On("CreateInstance", mock.Anything).Run(func(mock.Arguments) { cancel() }).Return(new(MockOperation), errors.New(
+		"Failed instance placement scriptlet: insufficient-memory: no fleet member has room for this worker",
+	))
+	_, _, _, err := p.createWarm(ctx, "nddev-linux-standard")
+	require.ErrorContains(t, err, "journal unavailable")
+	require.Len(t, a.released, 1, "cleanup must run even when the create context expired")
+	require.Len(t, a.reserved, 1)
+}
+
+func TestWarmArgumentsFailureReleasesExactReservedIdentity(t *testing.T) {
+	cli := new(MockIncusServer)
+	p := newTestProvider(cli)
+	a := &trackedWarmAdmission{}
+	p.admission = a
+	cli.On("GetProfileNames").Return([]string{}, errors.New("profile lookup unavailable"))
+	_, _, _, err := p.createWarm(t.Context(), "nddev-linux-standard")
+	require.Error(t, err)
+	require.Empty(t, a.reserved)
+	require.Len(t, a.released, 1)
+	require.Contains(t, a.released[0], "warm-standard-")
+	cli.AssertNotCalled(t, "CreateInstance", mock.Anything)
+}
+
+func TestWarmAmbiguousCreatePreservesReservationForReconciliation(t *testing.T) {
+	cli := new(MockIncusServer)
+	p := newTestProvider(cli)
+	a := &trackedWarmAdmission{}
+	p.admission = a
+	prepareCreateMocks(cli, testImageDigest)
+	cli.On("CreateInstance", mock.Anything).Return(new(MockOperation), errors.New("connection reset after request"))
+	_, _, _, err := p.createWarm(t.Context(), "nddev-linux-standard")
+	require.Error(t, err)
+	require.Len(t, a.reserved, 1, "an ambiguous create may still allocate a worker")
+	require.Empty(t, a.released)
+	cli.AssertNotCalled(t, "DeleteInstance", mock.Anything)
+}