fix: fix for prebuilds claiming and deletion (coder#17624)

evgeniy-scherbina · web-flow · commit 98e5611e164c · 2025-05-01T08:52:23.000Z
PR contains:
- fix for claiming &amp; deleting prebuilds with immutable params
- unit test for claiming scenario
- unit test for deletion scenario

The parameter resolver was failing when deleting/claiming prebuilds
because a value for a previously-used parameter was provided to the
resolver, but since the value was unchanged (it's coming from the
preset) it failed in the resolver. The resolver was missing a check to
see if the old value != new value; if the values match then there's no
mutation of an immutable parameter.

---------

Signed-off-by: Danny Kopping &lt;dannykopping@gmail.com&gt;
diff --git a/codersdk/richparameters.go b/codersdk/richparameters.go
@@ -164,7 +164,7 @@ type ParameterResolver struct {
 // resolves the correct value.  It returns the value of the parameter, if valid, and an error if invalid.
 func (r *ParameterResolver) ValidateResolve(p TemplateVersionParameter, v *WorkspaceBuildParameter) (value string, err error) {
 	prevV := r.findLastValue(p)
-	if !p.Mutable && v != nil && prevV != nil {
+	if !p.Mutable && v != nil && prevV != nil && v.Value != prevV.Value {
 		return "", xerrors.Errorf("Parameter %q is not mutable, so it can't be updated after creating a workspace.", p.Name)
 	}
 	if p.Required && v == nil && prevV == nil {
diff --git a/codersdk/richparameters_test.go b/codersdk/richparameters_test.go
@@ -1,6 +1,7 @@
 package codersdk_test
 
 import (
+	"fmt"
 	"testing"
 
 	"github.com/stretchr/testify/require"
@@ -121,20 +122,60 @@ func TestParameterResolver_ValidateResolve_NewOverridesOld(t *testing.T) {
 func TestParameterResolver_ValidateResolve_Immutable(t *testing.T) {
 	t.Parallel()
 	uut := codersdk.ParameterResolver{
-		Rich: []codersdk.WorkspaceBuildParameter{{Name: "n", Value: "5"}},
+		Rich: []codersdk.WorkspaceBuildParameter{{Name: "n", Value: "old"}},
 	}
 	p := codersdk.TemplateVersionParameter{
 		Name:     "n",
-		Type:     "number",
+		Type:     "string",
 		Required: true,
 		Mutable:  false,
 	}
-	v, err := uut.ValidateResolve(p, &codersdk.WorkspaceBuildParameter{
-		Name:  "n",
-		Value: "6",
-	})
-	require.Error(t, err)
-	require.Equal(t, "", v)
+
+	cases := []struct {
+		name        string
+		newValue    string
+		expectedErr string
+	}{
+		{
+			name:        "mutation",
+			newValue:    "new", // "new" != "old"
+			expectedErr: fmt.Sprintf("Parameter %q is not mutable", p.Name),
+		},
+		{
+			// Values are case-sensitive.
+			name:        "case change",
+			newValue:    "Old", // "Old" != "old"
+			expectedErr: fmt.Sprintf("Parameter %q is not mutable", p.Name),
+		},
+		{
+			name:        "default",
+			newValue:    "", // "" != "old"
+			expectedErr: fmt.Sprintf("Parameter %q is not mutable", p.Name),
+		},
+		{
+			name:     "no change",
+			newValue: "old", // "old" == "old"
+		},
+	}
+
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			t.Parallel()
+
+			v, err := uut.ValidateResolve(p, &codersdk.WorkspaceBuildParameter{
+				Name:  "n",
+				Value: tc.newValue,
+			})
+
+			if tc.expectedErr == "" {
+				require.NoError(t, err)
+				require.Equal(t, tc.newValue, v)
+			} else {
+				require.ErrorContains(t, err, tc.expectedErr)
+				require.Equal(t, "", v)
+			}
+		})
+	}
 }
 
 func TestRichParameterValidation(t *testing.T) {
diff --git a/enterprise/coderd/prebuilds/claim_test.go b/enterprise/coderd/prebuilds/claim_test.go
@@ -329,9 +329,8 @@ func TestClaimPrebuild(t *testing.T) {
 			require.NoError(t, err)
 
 			stopBuild, err := userClient.CreateWorkspaceBuild(ctx, workspace.ID, codersdk.CreateWorkspaceBuildRequest{
-				TemplateVersionID:   version.ID,
-				Transition:          codersdk.WorkspaceTransitionStop,
-				RichParameterValues: wp,
+				TemplateVersionID: version.ID,
+				Transition:        codersdk.WorkspaceTransitionStop,
 			})
 			require.NoError(t, err)
 			coderdtest.AwaitWorkspaceBuildJobCompleted(t, userClient, stopBuild.ID)
@@ -369,6 +368,17 @@ func templateWithAgentAndPresetsWithPrebuilds(desiredInstances int32) *echo.Resp
 								},
 							},
 						},
+						// Make sure immutable params don't break claiming logic
+						Parameters: []*proto.RichParameter{
+							{
+								Name:         "k1",
+								Description:  "immutable param",
+								Type:         "string",
+								DefaultValue: "",
+								Required:     false,
+								Mutable:      false,
+							},
+						},
 						Presets: []*proto.Preset{
 							{
 								Name: "preset-a",
diff --git a/enterprise/coderd/prebuilds/reconcile_test.go b/enterprise/coderd/prebuilds/reconcile_test.go
@@ -901,6 +901,16 @@ func setupTestDBTemplateVersion(
 		ID:              templateID,
 		ActiveVersionID: templateVersion.ID,
 	}))
+	// Make sure immutable params don't break prebuilt workspace deletion logic
+	dbgen.TemplateVersionParameter(t, db, database.TemplateVersionParameter{
+		TemplateVersionID: templateVersion.ID,
+		Name:              "test",
+		Description:       "required & immutable param",
+		Type:              "string",
+		DefaultValue:      "",
+		Required:          true,
+		Mutable:           false,
+	})
 	return templateVersion.ID
 }
 
@@ -999,7 +1009,7 @@ func setupTestDBWorkspace(
 		OrganizationID: orgID,
 		Error:          buildError,
 	})
-	dbgen.WorkspaceBuild(t, db, database.WorkspaceBuild{
+	workspaceBuild := dbgen.WorkspaceBuild(t, db, database.WorkspaceBuild{
 		WorkspaceID:             workspace.ID,
 		InitiatorID:             initiatorID,
 		TemplateVersionID:       templateVersionID,
@@ -1008,6 +1018,13 @@ func setupTestDBWorkspace(
 		Transition:              transition,
 		CreatedAt:               clock.Now(),
 	})
+	dbgen.WorkspaceBuildParameters(t, db, []database.WorkspaceBuildParameter{
+		{
+			WorkspaceBuildID: workspaceBuild.ID,
+			Name:             "test",
+			Value:            "test",
+		},
+	})
 
 	return workspace
 }

Original file line number	Diff line number	Diff line change
`@@ -164,7 +164,7 @@ type ParameterResolver struct {`
`164`	`164`	`// resolves the correct value. It returns the value of the parameter, if valid, and an error if invalid.`
`165`	`165`	`func (r ParameterResolver) ValidateResolve(p TemplateVersionParameter, v WorkspaceBuildParameter) (value string, err error) {`
`166`	`166`	`prevV := r.findLastValue(p)`
`167`		`- if !p.Mutable && v != nil && prevV != nil {`
	`167`	`+ if !p.Mutable && v != nil && prevV != nil && v.Value != prevV.Value {`
`168`	`168`	`return "", xerrors.Errorf("Parameter %q is not mutable, so it can't be updated after creating a workspace.", p.Name)`
`169`	`169`	`}`
`170`	`170`	`if p.Required && v == nil && prevV == nil {`