test: add unit test for closing files

coder · Emyrk · May 16, 2025 · May 14, 2025 · May 14, 2025 · May 14, 2025
commit 3cb39eaad2a19637d263cc296e49fa2522cb531f
@@ -16,7 +16,7 @@ import (
 
 // NewFromStore returns a file cache that will fetch files from the provided
 // database.
-func NewFromStore(store database.Store) Cache {
+func NewFromStore(store database.Store) *Cache {
 	fetcher := func(ctx context.Context, fileID uuid.UUID) (fs.FS, error) {
 		file, err := store.GetFileByID(ctx, fileID)
 		if err != nil {
@@ -27,7 +27,7 @@ func NewFromStore(store database.Store) Cache {
 		return archivefs.FromTarReader(content), nil
 	}
 
-	return Cache{
+	return &Cache{
 		lock:    sync.Mutex{},
 		data:    make(map[uuid.UUID]*cacheEntry),
 		fetcher: fetcher,

diff --git a/coderd/parameters.go b/coderd/parameters.go
@@ -71,11 +71,23 @@
 		return
 	}
 
+	// staticDiagnostics is a set of diagnostics to be applied to all rendered results.
 	staticDiagnostics := parameterProvisionerVersionDiagnostic(tf)
 
+	// render is the function that given a set of input values, will return the
+	// parameter state. There is 2 rendering functions.
+	//
+	// prepareStaticPreview uses the static set of parameters saved from the template
+	// import. These parameters are returned on every request, and have no dynamic
+	// functionality. This exists for backwards compatibility with older template versions
+	// which have not uploaded their plan & module files.
+	//
+	// prepareDynamicPreview uses the dynamic preview engine.
 	var render previewFunction
 	major, minor, err := apiversion.Parse(tf.ProvisionerdVersion)
 	if err != nil || major < 1 || (major == 1 && minor < 5) {
+		// Versions < 1.5 do not upload the required files.
+		// Versions == "" are < 1.5, but we don't know the exact version.
 		staticRender, err := prepareStaticPreview(ctx, api.Database, templateVersion.ID)
 		if err != nil {
 			httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
@@ -113,7 +125,7 @@
 	// Send an initial form state, computed without any user input.
 	result, diagnostics := render(ctx, map[string]string{})
 	response := codersdk.DynamicParametersResponse{
-		ID:          -1,
+		ID:          -1, // Always start with -1.
 		Diagnostics: previewtypes.Diagnostics(diagnostics.Extend(staticDiagnostics)),
 	}
 	if result != nil {
@@ -138,6 +150,7 @@
 				// The connection has been closed, so there is no one to write to
 				return
 			}
+
 			result, diagnostics := render(ctx, update.Inputs)
 			response := codersdk.DynamicParametersResponse{
 				ID:          update.ID,
@@ -158,12 +171,16 @@
 type previewFunction func(ctx context.Context, values map[string]string) (*preview.Output, hcl.Diagnostics)
 
 func prepareDynamicPreview(ctx context.Context, rw http.ResponseWriter, db database.Store, fc *files.Cache, tf database.TemplateVersionTerraformValue, templateVersion database.TemplateVersion, user database.User) (render previewFunction, closer func(), success bool) {
+	// keep track of all files opened
 	openFiles := make([]uuid.UUID, 0)
 	closeFiles := func() {
 		for _, it := range openFiles {
 			fc.Release(it)
 		}
 	}
+
+	// This defer will close the files if the function exits early without success.
+	// Closing the files is important to avoid having a memory leak.
 	defer func() {
 		if !success {
 			closeFiles()
@@ -182,6 +199,8 @@
 		return nil, nil, false
 	}
 
+	// Add the file first. Calling `Release` if it fails is a no-op, so this is safe.
+	openFiles = append(openFiles, fileID)
 	templateFS, err := fc.Acquire(fileCtx, fileID)
 	if err != nil {
 		httpapi.Write(ctx, rw, http.StatusNotFound, codersdk.Response{
@@ -190,14 +209,14 @@
 		})
 		return nil, nil, false
 	}
-	openFiles = append(openFiles, fileID)
 
 	// Having the Terraform plan available for the evaluation engine is helpful
 	// for populating values from data blocks, but isn't strictly required. If
 	// we don't have a cached plan available, we just use an empty one instead.
 	plan := json.RawMessage("{}")
 	plan = tf.CachedPlan
 
+	openFiles = append(openFiles, tf.CachedModuleFiles.UUID)
 	if tf.CachedModuleFiles.Valid {
 		moduleFilesFS, err := fc.Acquire(fileCtx, tf.CachedModuleFiles.UUID)
 		if err != nil {
@@ -207,7 +226,6 @@
 			})
 			return nil, nil, false
 		}
-		openFiles = append(openFiles, tf.CachedModuleFiles.UUID)
 
 		templateFS = files.NewOverlayFS(templateFS, []files.Overlay{{Path: ".terraform/modules", FS: moduleFilesFS}})
 	}
@@ -227,7 +245,7 @@
 		Owner:           owner,
 	}

 	return func(ctx context.Context, values map[string]string) (*preview.Output, hcl.Diagnostics) {
 		return preview.Preview(ctx, input, templateFS)
 	}, closeFiles, true
 }
@@ -313,14 +331,14 @@
 		params = append(params, param)
 	}

 	return func(ctx context.Context, values map[string]string) (*preview.Output, hcl.Diagnostics) {
 		for i := range params {
 			param := &params[i]
 			paramValue, ok := values[param.Name]
 			if ok {
 				param.Value = previewtypes.StringLiteral(paramValue)
 			} else {
 				paramValue = param.DefaultValue.AsString()
 			}
 			param.Diagnostics = previewtypes.Diagnostics(param.Valid(param.Value))
 		}
@@ -371,7 +389,10 @@
 
 	var publicKey string
 	g.Go(func() error {
-		key, err := db.GetGitSSHKey(ctx, user.ID)
+		// The correct public key has to be sent. This will not be leaked
+		// unless the template leaks it.
+		// nolint:gocritic
+		key, err := db.GetGitSSHKey(dbauthz.AsSystemRestricted(ctx), user.ID)
 		if err != nil {
 			return err
 		}
@@ -381,7 +402,11 @@
 
 	var groupNames []string
 	g.Go(func() error {
-		groups, err := db.GetGroups(ctx, database.GetGroupsParams{
+		// The groups need to be sent to preview. These groups are not exposed to the
+		// user, unless the template does it through the parameters. Regardless, we need
+		// the correct groups, and a user might not have read access.
+		// nolint:gocritic
+		groups, err := db.GetGroups(dbauthz.AsSystemRestricted(ctx), database.GetGroupsParams{
 			OrganizationID: organizationID,
 			HasMemberID:    user.ID,
 		})

diff --git a/coderd/parameters_test.go b/coderd/parameters_test.go
@@ -1,13 +1,19 @@
 package coderd_test
 
 import (
+	"context"
 	"os"
 	"testing"
 
+	"github.com/google/uuid"
 	"github.com/stretchr/testify/require"
+	"golang.org/x/xerrors"
 
 	"github.com/coder/coder/v2/coderd"
 	"github.com/coder/coder/v2/coderd/coderdtest"
+	"github.com/coder/coder/v2/coderd/database"
+	"github.com/coder/coder/v2/coderd/database/dbtestutil"
+	"github.com/coder/coder/v2/coderd/database/pubsub"
 	"github.com/coder/coder/v2/coderd/rbac"
 	"github.com/coder/coder/v2/codersdk"
 	"github.com/coder/coder/v2/codersdk/wsjson"
@@ -141,6 +147,8 @@
 	t.Parallel()
 
 	t.Run("OK_Modules", func(t *testing.T) {
+		t.Parallel()
+
 		dynamicParametersTerraformSource, err := os.ReadFile("testdata/parameters/modules/main.tf")
 		require.NoError(t, err)
 
@@ -171,7 +179,9 @@
 	})
 
 	// OldProvisioners use the static parameters in the dynamic param flow
 	t.Run("OldProvisioner", func(t *testing.T) {
+		t.Parallel()
+
 		setup := setupDynamicParamsTest(t, setupDynamicParamsTestParams{
 			provisionerDaemonVersion: "1.4",
 			mainTF:                   nil,
@@ -244,15 +254,42 @@
 		}
 
 	})
+
+	t.Run("FileError", func(t *testing.T) {
+		// Verify files close even if the websocket terminates from an error
+		t.Parallel()
+
+		db, ps := dbtestutil.NewDB(t)
+		dynamicParametersTerraformSource, err := os.ReadFile("testdata/parameters/modules/main.tf")
+		require.NoError(t, err)
+
+		modulesArchive, err := terraform.GetModulesArchive(os.DirFS("testdata/parameters/modules"))
+		require.NoError(t, err)
+
+		setup := setupDynamicParamsTest(t, setupDynamicParamsTestParams{
+			db:                       &dbRejectGitSSHKey{Store: db},
+			ps:                       ps,
+			provisionerDaemonVersion: provProto.CurrentVersion.String(),
+			mainTF:                   dynamicParametersTerraformSource,
+			modulesArchive:           modulesArchive,
+			expectWebsocketError:     true,
+		})
+		// This is checked in setupDynamicParamsTest. Just doing this in the
+		// test to make it obvious what this test is doing.
+		require.Zero(t, setup.api.FileCache.Count())
+	})
 }
 
 type setupDynamicParamsTestParams struct {
+	db                       database.Store
+	ps                       pubsub.Pubsub
 	provisionerDaemonVersion string
 	mainTF                   []byte
 	modulesArchive           []byte
 	plan                     []byte
 
-	static []*proto.RichParameter
+	static               []*proto.RichParameter
+	expectWebsocketError bool
 }
 
 type dynamicParamsTest struct {
@@ -265,6 +302,8 @@
 	cfg := coderdtest.DeploymentValues(t)
 	cfg.Experiments = []string{string(codersdk.ExperimentDynamicParameters)}
 	ownerClient, _, api := coderdtest.NewWithAPI(t, &coderdtest.Options{
+		Database:                 args.db,
+		Pubsub:                   args.ps,
 		IncludeProvisionerDaemon: true,
 		ProvisionerDaemonVersion: args.provisionerDaemonVersion,
 		DeploymentValues:         cfg,
@@ -292,10 +331,16 @@
 
 	ctx := testutil.Context(t, testutil.WaitShort)
 	stream, err := templateAdmin.TemplateVersionDynamicParameters(ctx, templateAdminUser.ID, version.ID)
-	require.NoError(t, err)
+	if args.expectWebsocketError {
+		require.Errorf(t, err, "expected error forming websocket")
+	} else {
+		require.NoError(t, err)
+	}
 
 	t.Cleanup(func() {
-		_ = stream.Close(websocket.StatusGoingAway)
+		if stream != nil {
+			_ = stream.Close(websocket.StatusGoingAway)
+		}
 		// Cache should always have 0 files when the only stream is closed
 		require.Eventually(t, func() bool {
 			return api.FileCache.Count() == 0
@@ -308,3 +353,13 @@
 		api:    api,
 	}
 }
+
+// dbRejectGitSSHKey is a cheeky way to force an error to occur in a place
+// that is generally impossible to force an error.
+type dbRejectGitSSHKey struct {
+	database.Store
+}
+
+func (d *dbRejectGitSSHKey) GetGitSSHKey(_ context.Context, _ uuid.UUID) (database.GitSSHKey, error) {
+	return database.GitSSHKey{}, xerrors.New("forcing a fake error")
+}