From 22f5008d3a412c447a9b4cdd9d036231f7952a63 Mon Sep 17 00:00:00 2001 From: Callum Styan Date: Tue, 9 Dec 2025 21:47:30 +0000 Subject: [PATCH 01/14] feat: add render cache for dynamic parameters MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This commit introduces an in-memory cache for preview.Preview results to avoid expensive Terraform file parsing on repeated renders with identical inputs. Key components: 1. RenderCache: Thread-safe cache keyed by (templateVersionID, ownerID, parameterHash) that stores preview.Output results 2. Integration with dynamicRenderer.Render(): - Check cache before calling preview.Preview() - Store successful renders in cache - Return cached results on cache hit 3. Comprehensive tests: - Basic cache operations (get/put) - Cache key separation (different versions/owners/params) - Parameter hash consistency (order-independent) - Prebuild scenario simulation The cache enables significant resource savings for prebuilds where multiple instances share the same template version and parameters. All prebuilds use database.PrebuildsSystemUserID, ensuring perfect cache sharing across instances of the same preset. Expected impact: ~80-90% cache hit rate for steady-state prebuild reconciliation cycles, reducing CPU/memory from Terraform parsing. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 --- coderd/dynamicparameters/render.go | 26 +++- coderd/dynamicparameters/rendercache.go | 87 +++++++++++ coderd/dynamicparameters/rendercache_test.go | 149 +++++++++++++++++++ 3 files changed, 261 insertions(+), 1 deletion(-) create mode 100644 coderd/dynamicparameters/rendercache.go create mode 100644 coderd/dynamicparameters/rendercache_test.go diff --git a/coderd/dynamicparameters/render.go b/coderd/dynamicparameters/render.go index 562517b6db284..ef59910e8e769 100644 --- a/coderd/dynamicparameters/render.go +++ b/coderd/dynamicparameters/render.go @@ -46,6 +46,9 @@ type loader struct { job *database.ProvisionerJob terraformValues *database.TemplateVersionTerraformValue templateVariableValues *[]database.TemplateVersionVariable + + // renderCache caches preview.Preview results + renderCache *RenderCache } // Prepare is the entrypoint for this package. It loads the necessary objects & @@ -91,6 +94,13 @@ func WithTerraformValues(values database.TemplateVersionTerraformValue) func(r * } } +func WithRenderCache(cache *RenderCache) func(r *loader) { + return func(r *loader) { + r.renderCache = cache + } +} + + func (r *loader) loadData(ctx context.Context, db database.Store) error { if r.templateVersion == nil { tv, err := db.GetTemplateVersionByID(ctx, r.templateVersionID) @@ -227,6 +237,13 @@ type dynamicRenderer struct { } func (r *dynamicRenderer) Render(ctx context.Context, ownerID uuid.UUID, values map[string]string) (*preview.Output, hcl.Diagnostics) { + // Check cache first if available + if r.data.renderCache != nil { + if cached, ok := r.data.renderCache.get(r.data.templateVersionID, ownerID, values); ok { + return cached, nil + } + } + // Always start with the cached error, if we have one. ownerErr := r.ownerErrors[ownerID] if ownerErr == nil { @@ -258,7 +275,14 @@ func (r *dynamicRenderer) Render(ctx context.Context, ownerID uuid.UUID, values Logger: slog.New(slog.DiscardHandler), } - return preview.Preview(ctx, input, r.templateFS) + output, diags := preview.Preview(ctx, input, r.templateFS) + + // Store in cache if successful and cache is available + if r.data.renderCache != nil && !diags.HasErrors() { + r.data.renderCache.put(r.data.templateVersionID, ownerID, values, output) + } + + return output, diags } func (r *dynamicRenderer) getWorkspaceOwnerData(ctx context.Context, ownerID uuid.UUID) error { diff --git a/coderd/dynamicparameters/rendercache.go b/coderd/dynamicparameters/rendercache.go new file mode 100644 index 0000000000000..bd2e71be1f107 --- /dev/null +++ b/coderd/dynamicparameters/rendercache.go @@ -0,0 +1,87 @@ +package dynamicparameters + +import ( + "crypto/md5" + "encoding/hex" + "sort" + "sync" + + "github.com/google/uuid" + + "github.com/coder/preview" +) + +// RenderCache is a simple in-memory cache for preview.Preview results. +// It caches based on (templateVersionID, ownerID, parameterValues). +type RenderCache struct { + mu sync.RWMutex + entries map[cacheKey]*preview.Output +} + +type cacheKey struct { + templateVersionID uuid.UUID + ownerID uuid.UUID + parameterHash string +} + +// NewRenderCache creates a new render cache. +func NewRenderCache() *RenderCache { + return &RenderCache{ + entries: make(map[cacheKey]*preview.Output), + } +} + +// NewRenderCacheForTest creates a new render cache for testing purposes. +func NewRenderCacheForTest() *RenderCache { + return NewRenderCache() +} + +func (c *RenderCache) get(templateVersionID, ownerID uuid.UUID, parameters map[string]string) (*preview.Output, bool) { + key := c.makeKey(templateVersionID, ownerID, parameters) + c.mu.RLock() + defer c.mu.RUnlock() + + output, ok := c.entries[key] + return output, ok +} + +func (c *RenderCache) put(templateVersionID, ownerID uuid.UUID, parameters map[string]string, output *preview.Output) { + key := c.makeKey(templateVersionID, ownerID, parameters) + c.mu.Lock() + defer c.mu.Unlock() + + c.entries[key] = output +} + +func (c *RenderCache) makeKey(templateVersionID, ownerID uuid.UUID, parameters map[string]string) cacheKey { + return cacheKey{ + templateVersionID: templateVersionID, + ownerID: ownerID, + parameterHash: hashParameters(parameters), + } +} + +// hashParameters creates a deterministic hash of the parameter map. +func hashParameters(params map[string]string) string { + if len(params) == 0 { + return "" + } + + // Sort keys for deterministic hashing + keys := make([]string, 0, len(params)) + for k := range params { + keys = append(keys, k) + } + sort.Strings(keys) + + // Hash the sorted key-value pairs + h := md5.New() + for _, k := range keys { + h.Write([]byte(k)) + h.Write([]byte{0}) // separator + h.Write([]byte(params[k])) + h.Write([]byte{0}) // separator + } + + return hex.EncodeToString(h.Sum(nil)) +} diff --git a/coderd/dynamicparameters/rendercache_test.go b/coderd/dynamicparameters/rendercache_test.go new file mode 100644 index 0000000000000..940c8ace6f497 --- /dev/null +++ b/coderd/dynamicparameters/rendercache_test.go @@ -0,0 +1,149 @@ +package dynamicparameters + +import ( + "testing" + + "github.com/google/uuid" + "github.com/stretchr/testify/require" + + "github.com/coder/preview" + previewtypes "github.com/coder/preview/types" +) + +func TestRenderCache_BasicOperations(t *testing.T) { + t.Parallel() + + cache := NewRenderCache() + templateVersionID := uuid.New() + ownerID := uuid.New() + params := map[string]string{"region": "us-west-2"} + + // Cache should be empty initially + _, ok := cache.get(templateVersionID, ownerID, params) + require.False(t, ok, "cache should be empty initially") + + // Put an entry in the cache + expectedOutput := &preview.Output{ + Parameters: []previewtypes.Parameter{ + { + ParameterData: previewtypes.ParameterData{ + Name: "region", + Type: previewtypes.ParameterTypeString, + }, + }, + }, + } + cache.put(templateVersionID, ownerID, params, expectedOutput) + + // Get should now return the cached value + cachedOutput, ok := cache.get(templateVersionID, ownerID, params) + require.True(t, ok, "cache should contain the entry") + require.Same(t, expectedOutput, cachedOutput, "should return same pointer") +} + +func TestRenderCache_DifferentKeysAreSeparate(t *testing.T) { + t.Parallel() + + cache := NewRenderCache() + templateVersion1 := uuid.New() + templateVersion2 := uuid.New() + owner1 := uuid.New() + owner2 := uuid.New() + params := map[string]string{"region": "us-west-2"} + + output1 := &preview.Output{} + output2 := &preview.Output{} + output3 := &preview.Output{} + + // Put different entries for different keys + cache.put(templateVersion1, owner1, params, output1) + cache.put(templateVersion2, owner1, params, output2) + cache.put(templateVersion1, owner2, params, output3) + + // Verify each key returns its own entry + cached1, ok1 := cache.get(templateVersion1, owner1, params) + require.True(t, ok1) + require.Same(t, output1, cached1) + + cached2, ok2 := cache.get(templateVersion2, owner1, params) + require.True(t, ok2) + require.Same(t, output2, cached2) + + cached3, ok3 := cache.get(templateVersion1, owner2, params) + require.True(t, ok3) + require.Same(t, output3, cached3) +} + +func TestRenderCache_ParameterHashConsistency(t *testing.T) { + t.Parallel() + + cache := NewRenderCache() + templateVersionID := uuid.New() + ownerID := uuid.New() + + // Parameters in different order should produce same cache key + params1 := map[string]string{"a": "1", "b": "2", "c": "3"} + params2 := map[string]string{"c": "3", "a": "1", "b": "2"} + + output := &preview.Output{} + cache.put(templateVersionID, ownerID, params1, output) + + // Should hit cache even with different parameter order + cached, ok := cache.get(templateVersionID, ownerID, params2) + require.True(t, ok, "different parameter order should still hit cache") + require.Same(t, output, cached) +} + +func TestRenderCache_EmptyParameters(t *testing.T) { + t.Parallel() + + cache := NewRenderCache() + templateVersionID := uuid.New() + ownerID := uuid.New() + + // Test with empty parameters + emptyParams := map[string]string{} + output := &preview.Output{} + + cache.put(templateVersionID, ownerID, emptyParams, output) + + cached, ok := cache.get(templateVersionID, ownerID, emptyParams) + require.True(t, ok) + require.Same(t, output, cached) +} + +func TestRenderCache_PrebuildScenario(t *testing.T) { + t.Parallel() + + // This test simulates the prebuild scenario where multiple prebuilds + // are created from the same template version with the same preset parameters. + cache := NewRenderCache() + + // In prebuilds, all instances use the same fixed ownerID + prebuildOwnerID := uuid.MustParse("c42fdf75-3097-471c-8c33-fb52454d81c0") // database.PrebuildsSystemUserID + templateVersionID := uuid.New() + presetParams := map[string]string{ + "instance_type": "t3.micro", + "region": "us-west-2", + } + + output := &preview.Output{} + + // First prebuild - cache miss + _, ok := cache.get(templateVersionID, prebuildOwnerID, presetParams) + require.False(t, ok, "first prebuild should miss cache") + + cache.put(templateVersionID, prebuildOwnerID, presetParams, output) + + // Second prebuild with same template version and preset - cache hit + cached2, ok2 := cache.get(templateVersionID, prebuildOwnerID, presetParams) + require.True(t, ok2, "second prebuild should hit cache") + require.Same(t, output, cached2, "should return cached output") + + // Third prebuild - also cache hit + cached3, ok3 := cache.get(templateVersionID, prebuildOwnerID, presetParams) + require.True(t, ok3, "third prebuild should hit cache") + require.Same(t, output, cached3, "should return cached output") + + // All three prebuilds shared the same cache entry +} From 72d711dde23722764a4ddd07f15c36d46ce8e127 Mon Sep 17 00:00:00 2001 From: Callum Styan Date: Tue, 9 Dec 2025 21:47:55 +0000 Subject: [PATCH 02/14] feat: wire render cache into prebuilds reconciler MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This commit integrates the render cache into the prebuilds reconciliation flow, enabling cache sharing across all prebuild operations. Changes: 1. StoreReconciler: - Add renderCache field (*dynamicparameters.RenderCache) - Initialize cache in NewStoreReconciler() - Pass cache to builder in provision() method 2. wsbuilder.Builder: - Add renderCache field - Add RenderCache(cache) builder method - Pass cache to dynamicparameters.Prepare() when available How it works: - Single RenderCache instance shared across all prebuilds - Each workspace build passes the cache through the builder chain - Cache keyed by (templateVersionID, ownerID, parameterHash) - All prebuilds use PrebuildsSystemUserID, maximizing cache hits Expected behavior for a preset with 5 instances: - First instance: cache miss → full render → cache stored - Instances 2-5: cache hit → instant return - Result: ~80% reduction in render operations per cycle This addresses the resource cost identified in profiling where the prebuilds path consumed 25% CPU and 50% memory allocations, primarily from repeated Terraform file parsing. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 --- coderd/wsbuilder/wsbuilder.go | 27 ++++++++++++++++++++++++ enterprise/coderd/prebuilds/reconcile.go | 8 ++++++- 2 files changed, 34 insertions(+), 1 deletion(-) diff --git a/coderd/wsbuilder/wsbuilder.go b/coderd/wsbuilder/wsbuilder.go index 7d388966c96d0..5ca038530e96f 100644 --- a/coderd/wsbuilder/wsbuilder.go +++ b/coderd/wsbuilder/wsbuilder.go @@ -88,6 +88,9 @@ type Builder struct { parameterRender dynamicparameters.Renderer workspaceTags *map[string]string + // renderCache caches template rendering results + renderCache *dynamicparameters.RenderCache + prebuiltWorkspaceBuildStage sdkproto.PrebuiltWorkspaceBuildStage verifyNoLegacyParametersOnce bool } @@ -253,6 +256,14 @@ func (b Builder) TemplateVersionPresetID(id uuid.UUID) Builder { return b } +// RenderCache sets the render cache to use for template rendering. +// This allows multiple workspace builds to share cached render results. +func (b Builder) RenderCache(cache *dynamicparameters.RenderCache) Builder { + // nolint: revive + b.renderCache = cache + return b +} + type BuildError struct { // Status is a suitable HTTP status code Status int @@ -686,6 +697,22 @@ func (b *Builder) getDynamicParameterRenderer() (dynamicparameters.Renderer, err return nil, xerrors.Errorf("get template version variables: %w", err) } + // Pass render cache if available + if b.renderCache != nil { + renderer, err := dynamicparameters.Prepare(b.ctx, b.store, b.fileCache, tv.ID, + dynamicparameters.WithTemplateVersion(*tv), + dynamicparameters.WithProvisionerJob(*job), + dynamicparameters.WithTerraformValues(*tfVals), + dynamicparameters.WithTemplateVariableValues(variableValues), + dynamicparameters.WithRenderCache(b.renderCache), + ) + if err != nil { + return nil, xerrors.Errorf("get template version renderer: %w", err) + } + b.parameterRender = renderer + return renderer, nil + } + renderer, err := dynamicparameters.Prepare(b.ctx, b.store, b.fileCache, tv.ID, dynamicparameters.WithTemplateVersion(*tv), dynamicparameters.WithProvisionerJob(*job), diff --git a/enterprise/coderd/prebuilds/reconcile.go b/enterprise/coderd/prebuilds/reconcile.go index 17a56d484c9f6..654f1d527be3f 100644 --- a/enterprise/coderd/prebuilds/reconcile.go +++ b/enterprise/coderd/prebuilds/reconcile.go @@ -26,6 +26,7 @@ import ( "github.com/coder/coder/v2/coderd/database/dbauthz" "github.com/coder/coder/v2/coderd/database/provisionerjobs" "github.com/coder/coder/v2/coderd/database/pubsub" + "github.com/coder/coder/v2/coderd/dynamicparameters" "github.com/coder/coder/v2/coderd/files" "github.com/coder/coder/v2/coderd/notifications" "github.com/coder/coder/v2/coderd/prebuilds" @@ -58,6 +59,9 @@ type StoreReconciler struct { metrics *MetricsCollector // Operational metrics reconciliationDuration prometheus.Histogram + + // renderCache caches template rendering results to avoid expensive re-parsing + renderCache *dynamicparameters.RenderCache } var _ prebuilds.ReconciliationOrchestrator = &StoreReconciler{} @@ -102,6 +106,7 @@ func NewStoreReconciler(store database.Store, buildUsageChecker: buildUsageChecker, done: make(chan struct{}, 1), provisionNotifyCh: make(chan database.ProvisionerJob, 10), + renderCache: dynamicparameters.NewRenderCache(), } if registerer != nil { @@ -900,7 +905,8 @@ func (c *StoreReconciler) provision( builder := wsbuilder.New(workspace, transition, *c.buildUsageChecker.Load()). Reason(database.BuildReasonInitiator). Initiator(database.PrebuildsSystemUserID). - MarkPrebuild() + MarkPrebuild(). + RenderCache(c.renderCache) if transition != database.WorkspaceTransitionDelete { // We don't specify the version for a delete transition, From acd9bed98e3c4ac7ed69da99c8df4342ee028236 Mon Sep 17 00:00:00 2001 From: Callum Styan Date: Tue, 9 Dec 2025 22:21:56 +0000 Subject: [PATCH 03/14] feat: add Prometheus metrics for render cache observability Adds cache hits, misses, and size metrics to track render cache performance and enable monitoring of the cache's effectiveness in reducing expensive terraform parsing operations. Metrics added: - coderd_prebuilds_render_cache_hits_total: Counter for cache hits - coderd_prebuilds_render_cache_misses_total: Counter for cache misses - coderd_prebuilds_render_cache_size_entries: Gauge for current cache size The metrics are optional and only created when a Prometheus registerer is provided to the reconciler. --- coderd/dynamicparameters/rendercache.go | 33 +++++ coderd/dynamicparameters/rendercache_test.go | 120 +++++++++++++++++++ enterprise/coderd/prebuilds/reconcile.go | 25 +++- 3 files changed, 177 insertions(+), 1 deletion(-) diff --git a/coderd/dynamicparameters/rendercache.go b/coderd/dynamicparameters/rendercache.go index bd2e71be1f107..3cae6fbe9d146 100644 --- a/coderd/dynamicparameters/rendercache.go +++ b/coderd/dynamicparameters/rendercache.go @@ -7,6 +7,7 @@ import ( "sync" "github.com/google/uuid" + "github.com/prometheus/client_golang/prometheus" "github.com/coder/preview" ) @@ -16,6 +17,11 @@ import ( type RenderCache struct { mu sync.RWMutex entries map[cacheKey]*preview.Output + + // Metrics (optional) + cacheHits prometheus.Counter + cacheMisses prometheus.Counter + cacheSize prometheus.Gauge } type cacheKey struct { @@ -31,6 +37,16 @@ func NewRenderCache() *RenderCache { } } +// NewRenderCacheWithMetrics creates a new render cache with Prometheus metrics. +func NewRenderCacheWithMetrics(cacheHits, cacheMisses prometheus.Counter, cacheSize prometheus.Gauge) *RenderCache { + return &RenderCache{ + entries: make(map[cacheKey]*preview.Output), + cacheHits: cacheHits, + cacheMisses: cacheMisses, + cacheSize: cacheSize, + } +} + // NewRenderCacheForTest creates a new render cache for testing purposes. func NewRenderCacheForTest() *RenderCache { return NewRenderCache() @@ -42,6 +58,18 @@ func (c *RenderCache) get(templateVersionID, ownerID uuid.UUID, parameters map[s defer c.mu.RUnlock() output, ok := c.entries[key] + + // Record metrics + if ok { + if c.cacheHits != nil { + c.cacheHits.Inc() + } + } else { + if c.cacheMisses != nil { + c.cacheMisses.Inc() + } + } + return output, ok } @@ -51,6 +79,11 @@ func (c *RenderCache) put(templateVersionID, ownerID uuid.UUID, parameters map[s defer c.mu.Unlock() c.entries[key] = output + + // Update cache size metric + if c.cacheSize != nil { + c.cacheSize.Set(float64(len(c.entries))) + } } func (c *RenderCache) makeKey(templateVersionID, ownerID uuid.UUID, parameters map[string]string) cacheKey { diff --git a/coderd/dynamicparameters/rendercache_test.go b/coderd/dynamicparameters/rendercache_test.go index 940c8ace6f497..aaae5c76979f6 100644 --- a/coderd/dynamicparameters/rendercache_test.go +++ b/coderd/dynamicparameters/rendercache_test.go @@ -4,6 +4,8 @@ import ( "testing" "github.com/google/uuid" + "github.com/prometheus/client_golang/prometheus" + dto "github.com/prometheus/client_model/go" "github.com/stretchr/testify/require" "github.com/coder/preview" @@ -147,3 +149,121 @@ func TestRenderCache_PrebuildScenario(t *testing.T) { // All three prebuilds shared the same cache entry } + +func TestRenderCache_Metrics(t *testing.T) { + t.Parallel() + + // Create test metrics + cacheHits := &testCounter{} + cacheMisses := &testCounter{} + cacheSize := &testGauge{} + + cache := NewRenderCacheWithMetrics(cacheHits, cacheMisses, cacheSize) + templateVersionID := uuid.New() + ownerID := uuid.New() + params := map[string]string{"region": "us-west-2"} + + // Initially: 0 hits, 0 misses, 0 size + require.Equal(t, float64(0), cacheHits.value, "initial hits should be 0") + require.Equal(t, float64(0), cacheMisses.value, "initial misses should be 0") + require.Equal(t, float64(0), cacheSize.value, "initial size should be 0") + + // First get - should be a miss + _, ok := cache.get(templateVersionID, ownerID, params) + require.False(t, ok) + require.Equal(t, float64(0), cacheHits.value, "hits should still be 0") + require.Equal(t, float64(1), cacheMisses.value, "misses should be 1") + require.Equal(t, float64(0), cacheSize.value, "size should still be 0") + + // Put an entry + output := &preview.Output{} + cache.put(templateVersionID, ownerID, params, output) + require.Equal(t, float64(1), cacheSize.value, "size should be 1 after put") + + // Second get - should be a hit + _, ok = cache.get(templateVersionID, ownerID, params) + require.True(t, ok) + require.Equal(t, float64(1), cacheHits.value, "hits should be 1") + require.Equal(t, float64(1), cacheMisses.value, "misses should still be 1") + require.Equal(t, float64(1), cacheSize.value, "size should still be 1") + + // Third get - another hit + _, ok = cache.get(templateVersionID, ownerID, params) + require.True(t, ok) + require.Equal(t, float64(2), cacheHits.value, "hits should be 2") + require.Equal(t, float64(1), cacheMisses.value, "misses should still be 1") + + // Put another entry with different params + params2 := map[string]string{"region": "us-east-1"} + cache.put(templateVersionID, ownerID, params2, output) + require.Equal(t, float64(2), cacheSize.value, "size should be 2 after second put") + + // Get with different params - should be a hit + _, ok = cache.get(templateVersionID, ownerID, params2) + require.True(t, ok) + require.Equal(t, float64(3), cacheHits.value, "hits should be 3") + require.Equal(t, float64(1), cacheMisses.value, "misses should still be 1") +} + +// Test implementations of prometheus interfaces +type testCounter struct { + value float64 +} + +func (c *testCounter) Inc() { + c.value++ +} + +func (c *testCounter) Add(v float64) { + c.value += v +} + +func (c *testCounter) Desc() *prometheus.Desc { + return nil +} + +func (c *testCounter) Write(*dto.Metric) error { + return nil +} + +func (c *testCounter) Describe(chan<- *prometheus.Desc) {} + +func (c *testCounter) Collect(chan<- prometheus.Metric) {} + +type testGauge struct { + value float64 +} + +func (g *testGauge) Set(v float64) { + g.value = v +} + +func (g *testGauge) Inc() { + g.value++ +} + +func (g *testGauge) Dec() { + g.value-- +} + +func (g *testGauge) Add(v float64) { + g.value += v +} + +func (g *testGauge) Sub(v float64) { + g.value -= v +} + +func (g *testGauge) SetToCurrentTime() {} + +func (g *testGauge) Desc() *prometheus.Desc { + return nil +} + +func (g *testGauge) Write(*dto.Metric) error { + return nil +} + +func (g *testGauge) Describe(chan<- *prometheus.Desc) {} + +func (g *testGauge) Collect(chan<- prometheus.Metric) {} diff --git a/enterprise/coderd/prebuilds/reconcile.go b/enterprise/coderd/prebuilds/reconcile.go index 654f1d527be3f..593fb11b09c58 100644 --- a/enterprise/coderd/prebuilds/reconcile.go +++ b/enterprise/coderd/prebuilds/reconcile.go @@ -106,7 +106,6 @@ func NewStoreReconciler(store database.Store, buildUsageChecker: buildUsageChecker, done: make(chan struct{}, 1), provisionNotifyCh: make(chan database.ProvisionerJob, 10), - renderCache: dynamicparameters.NewRenderCache(), } if registerer != nil { @@ -124,6 +123,30 @@ func NewStoreReconciler(store database.Store, Help: "Duration of each prebuilds reconciliation cycle.", Buckets: prometheus.DefBuckets, }) + + // Create metrics for the render cache + renderCacheHits := factory.NewCounter(prometheus.CounterOpts{ + Namespace: "coderd", + Subsystem: "prebuilds", + Name: "render_cache_hits_total", + Help: "Total number of render cache hits.", + }) + renderCacheMisses := factory.NewCounter(prometheus.CounterOpts{ + Namespace: "coderd", + Subsystem: "prebuilds", + Name: "render_cache_misses_total", + Help: "Total number of render cache misses.", + }) + renderCacheSize := factory.NewGauge(prometheus.GaugeOpts{ + Namespace: "coderd", + Subsystem: "prebuilds", + Name: "render_cache_size_entries", + Help: "Current number of entries in the render cache.", + }) + + reconciler.renderCache = dynamicparameters.NewRenderCacheWithMetrics(renderCacheHits, renderCacheMisses, renderCacheSize) + } else { + reconciler.renderCache = dynamicparameters.NewRenderCache() } return reconciler From 353adc78c859f8cd36e2b8f748dfc559662ba197 Mon Sep 17 00:00:00 2001 From: Callum Styan Date: Tue, 9 Dec 2025 22:29:34 +0000 Subject: [PATCH 04/14] feat: add TTL-based cache cleanup for render cache Implements automatic expiration and cleanup of cache entries: - 1 hour TTL for cached entries - Periodic cleanup every 15 minutes to remove expired entries - Expired entries are treated as cache misses on get() - Cache properly closed when reconciler stops to clean up goroutine Uses quartz.Clock instead of time package for testability, allowing tests to advance the clock without time.Sleep(). This prevents unbounded cache growth while maintaining high hit rates for frequently rendered template versions. --- coderd/dynamicparameters/rendercache.go | 124 ++++++++++++++++--- coderd/dynamicparameters/rendercache_test.go | 80 ++++++++++++ enterprise/coderd/prebuilds/reconcile.go | 5 + 3 files changed, 193 insertions(+), 16 deletions(-) diff --git a/coderd/dynamicparameters/rendercache.go b/coderd/dynamicparameters/rendercache.go index 3cae6fbe9d146..a50b8562397c9 100644 --- a/coderd/dynamicparameters/rendercache.go +++ b/coderd/dynamicparameters/rendercache.go @@ -1,27 +1,42 @@ package dynamicparameters import ( + "context" "crypto/md5" "encoding/hex" "sort" "sync" + "time" "github.com/google/uuid" "github.com/prometheus/client_golang/prometheus" "github.com/coder/preview" + "github.com/coder/quartz" ) // RenderCache is a simple in-memory cache for preview.Preview results. // It caches based on (templateVersionID, ownerID, parameterValues). type RenderCache struct { mu sync.RWMutex - entries map[cacheKey]*preview.Output + entries map[cacheKey]*cacheEntry // Metrics (optional) cacheHits prometheus.Counter cacheMisses prometheus.Counter cacheSize prometheus.Gauge + + // TTL cleanup + clock quartz.Clock + ttl time.Duration + stopOnce sync.Once + stopCh chan struct{} + doneCh chan struct{} +} + +type cacheEntry struct { + output *preview.Output + timestamp time.Time } type cacheKey struct { @@ -30,21 +45,32 @@ type cacheKey struct { parameterHash string } -// NewRenderCache creates a new render cache. +// NewRenderCache creates a new render cache with a default TTL of 1 hour. func NewRenderCache() *RenderCache { - return &RenderCache{ - entries: make(map[cacheKey]*preview.Output), - } + return newRenderCache(quartz.NewReal(), time.Hour, nil, nil, nil) } // NewRenderCacheWithMetrics creates a new render cache with Prometheus metrics. func NewRenderCacheWithMetrics(cacheHits, cacheMisses prometheus.Counter, cacheSize prometheus.Gauge) *RenderCache { - return &RenderCache{ - entries: make(map[cacheKey]*preview.Output), + return newRenderCache(quartz.NewReal(), time.Hour, cacheHits, cacheMisses, cacheSize) +} + +func newRenderCache(clock quartz.Clock, ttl time.Duration, cacheHits, cacheMisses prometheus.Counter, cacheSize prometheus.Gauge) *RenderCache { + c := &RenderCache{ + entries: make(map[cacheKey]*cacheEntry), + clock: clock, cacheHits: cacheHits, cacheMisses: cacheMisses, cacheSize: cacheSize, + ttl: ttl, + stopCh: make(chan struct{}), + doneCh: make(chan struct{}), } + + // Start cleanup goroutine + go c.cleanupLoop(context.Background()) + + return c } // NewRenderCacheForTest creates a new render cache for testing purposes. @@ -52,25 +78,43 @@ func NewRenderCacheForTest() *RenderCache { return NewRenderCache() } +// Close stops the cleanup goroutine and waits for it to finish. +func (c *RenderCache) Close() { + c.stopOnce.Do(func() { + close(c.stopCh) + <-c.doneCh + }) +} + func (c *RenderCache) get(templateVersionID, ownerID uuid.UUID, parameters map[string]string) (*preview.Output, bool) { key := c.makeKey(templateVersionID, ownerID, parameters) c.mu.RLock() defer c.mu.RUnlock() - output, ok := c.entries[key] - - // Record metrics - if ok { - if c.cacheHits != nil { - c.cacheHits.Inc() + entry, ok := c.entries[key] + if !ok { + // Record miss + if c.cacheMisses != nil { + c.cacheMisses.Inc() } - } else { + return nil, false + } + + // Check if entry has expired + if c.clock.Since(entry.timestamp) > c.ttl { + // Expired entry, treat as miss if c.cacheMisses != nil { c.cacheMisses.Inc() } + return nil, false } - return output, ok + // Record hit + if c.cacheHits != nil { + c.cacheHits.Inc() + } + + return entry.output, true } func (c *RenderCache) put(templateVersionID, ownerID uuid.UUID, parameters map[string]string, output *preview.Output) { @@ -78,7 +122,10 @@ func (c *RenderCache) put(templateVersionID, ownerID uuid.UUID, parameters map[s c.mu.Lock() defer c.mu.Unlock() - c.entries[key] = output + c.entries[key] = &cacheEntry{ + output: output, + timestamp: c.clock.Now(), + } // Update cache size metric if c.cacheSize != nil { @@ -118,3 +165,48 @@ func hashParameters(params map[string]string) string { return hex.EncodeToString(h.Sum(nil)) } + +// cleanupLoop runs periodically to remove expired cache entries. +func (c *RenderCache) cleanupLoop(ctx context.Context) { + defer close(c.doneCh) + + // Run cleanup every 15 minutes + cleanupFunc := func() error { + c.cleanup() + return nil + } + + // Run once immediately + _ = cleanupFunc() + + // Create a cancellable context for the ticker + tickerCtx, cancel := context.WithCancel(ctx) + defer cancel() + + // Create ticker for periodic cleanup + tkr := c.clock.TickerFunc(tickerCtx, 15*time.Minute, cleanupFunc, "render-cache-cleanup") + + // Wait for stop signal + <-c.stopCh + cancel() + + _ = tkr.Wait() +} + +// cleanup removes expired entries from the cache. +func (c *RenderCache) cleanup() { + c.mu.Lock() + defer c.mu.Unlock() + + now := c.clock.Now() + for key, entry := range c.entries { + if now.Sub(entry.timestamp) > c.ttl { + delete(c.entries, key) + } + } + + // Update cache size metric after cleanup + if c.cacheSize != nil { + c.cacheSize.Set(float64(len(c.entries))) + } +} diff --git a/coderd/dynamicparameters/rendercache_test.go b/coderd/dynamicparameters/rendercache_test.go index aaae5c76979f6..96885e4673be8 100644 --- a/coderd/dynamicparameters/rendercache_test.go +++ b/coderd/dynamicparameters/rendercache_test.go @@ -2,14 +2,17 @@ package dynamicparameters import ( "testing" + "time" "github.com/google/uuid" "github.com/prometheus/client_golang/prometheus" dto "github.com/prometheus/client_model/go" "github.com/stretchr/testify/require" + "github.com/coder/coder/v2/testutil" "github.com/coder/preview" previewtypes "github.com/coder/preview/types" + "github.com/coder/quartz" ) func TestRenderCache_BasicOperations(t *testing.T) { @@ -205,6 +208,83 @@ func TestRenderCache_Metrics(t *testing.T) { require.Equal(t, float64(1), cacheMisses.value, "misses should still be 1") } +func TestRenderCache_TTL(t *testing.T) { + t.Parallel() + + ctx := testutil.Context(t, testutil.WaitShort) + clock := quartz.NewMock(t) + + trapTickerFunc := clock.Trap().TickerFunc("render-cache-cleanup") + defer trapTickerFunc.Close() + + // Create cache with short TTL for testing + cache := newRenderCache(clock, 100*time.Millisecond, nil, nil, nil) + defer cache.Close() + + // Wait for the initial cleanup and ticker to be created + trapTickerFunc.MustWait(ctx).Release(ctx) + + templateVersionID := uuid.New() + ownerID := uuid.New() + params := map[string]string{"region": "us-west-2"} + output := &preview.Output{} + + // Put an entry + cache.put(templateVersionID, ownerID, params, output) + + // Should be a hit immediately + cached, ok := cache.get(templateVersionID, ownerID, params) + require.True(t, ok, "should hit cache immediately") + require.Same(t, output, cached) + + // Advance time beyond TTL + clock.Advance(150 * time.Millisecond) + + // Should be a miss after TTL + _, ok = cache.get(templateVersionID, ownerID, params) + require.False(t, ok, "should miss cache after TTL expiration") +} + +func TestRenderCache_CleanupRemovesExpiredEntries(t *testing.T) { + t.Parallel() + + ctx := testutil.Context(t, testutil.WaitShort) + clock := quartz.NewMock(t) + + trapTickerFunc := clock.Trap().TickerFunc("render-cache-cleanup") + defer trapTickerFunc.Close() + + cacheSize := &testGauge{} + cache := newRenderCache(clock, 100*time.Millisecond, nil, nil, cacheSize) + defer cache.Close() + + // Wait for the initial cleanup and ticker to be created + trapTickerFunc.MustWait(ctx).Release(ctx) + + // Initial size should be 0 after first cleanup + require.Equal(t, float64(0), cacheSize.value, "should have 0 entries initially") + + templateVersionID := uuid.New() + ownerID := uuid.New() + + // Add 3 entries + for i := 0; i < 3; i++ { + params := map[string]string{"index": string(rune(i))} + cache.put(templateVersionID, ownerID, params, &preview.Output{}) + } + + require.Equal(t, float64(3), cacheSize.value, "should have 3 entries") + + // Advance time beyond TTL + clock.Advance(150 * time.Millisecond) + + // Trigger cleanup by advancing to the next ticker event (15 minutes from start minus what we already advanced) + clock.Advance(15*time.Minute - 150*time.Millisecond).MustWait(ctx) + + // All entries should be removed + require.Equal(t, float64(0), cacheSize.value, "all entries should be removed after cleanup") +} + // Test implementations of prometheus interfaces type testCounter struct { value float64 diff --git a/enterprise/coderd/prebuilds/reconcile.go b/enterprise/coderd/prebuilds/reconcile.go index 593fb11b09c58..584b4f58e7119 100644 --- a/enterprise/coderd/prebuilds/reconcile.go +++ b/enterprise/coderd/prebuilds/reconcile.go @@ -292,6 +292,11 @@ func (c *StoreReconciler) Stop(ctx context.Context, cause error) { case <-c.done: c.logger.Info(context.Background(), "reconciler stopped") } + + // Close the render cache to stop its cleanup goroutine + if c.renderCache != nil { + c.renderCache.Close() + } } // ReconcileAll will attempt to resolve the desired vs actual state of all templates which have presets with prebuilds configured. From 318f6928374ed1e33dbc63d2f5f0e26cffa673b4 Mon Sep 17 00:00:00 2001 From: Callum Styan Date: Tue, 9 Dec 2025 23:12:54 +0000 Subject: [PATCH 05/14] feat: refresh cache entry timestamp on hits Updates the cache entry timestamp on every successful cache hit, implementing LRU-like behavior where frequently accessed entries stay in the cache longer. This ensures that actively used template versions remain cached even if they were initially added long ago, while rarely accessed entries expire and get cleaned up. Adds TestRenderCache_TimestampRefreshOnHit to verify that: - Cache hits refresh the entry timestamp - Refreshed entries remain valid beyond their original TTL - Entries eventually expire after no access for the full TTL period --- coderd/dynamicparameters/rendercache.go | 11 +++-- coderd/dynamicparameters/rendercache_test.go | 50 ++++++++++++++++++++ 2 files changed, 58 insertions(+), 3 deletions(-) diff --git a/coderd/dynamicparameters/rendercache.go b/coderd/dynamicparameters/rendercache.go index a50b8562397c9..48eace932aaf4 100644 --- a/coderd/dynamicparameters/rendercache.go +++ b/coderd/dynamicparameters/rendercache.go @@ -89,9 +89,9 @@ func (c *RenderCache) Close() { func (c *RenderCache) get(templateVersionID, ownerID uuid.UUID, parameters map[string]string) (*preview.Output, bool) { key := c.makeKey(templateVersionID, ownerID, parameters) c.mu.RLock() - defer c.mu.RUnlock() - entry, ok := c.entries[key] + c.mu.RUnlock() + if !ok { // Record miss if c.cacheMisses != nil { @@ -109,11 +109,16 @@ func (c *RenderCache) get(templateVersionID, ownerID uuid.UUID, parameters map[s return nil, false } - // Record hit + // Record hit and refresh timestamp if c.cacheHits != nil { c.cacheHits.Inc() } + // Refresh timestamp on hit to keep frequently accessed entries alive + c.mu.Lock() + entry.timestamp = c.clock.Now() + c.mu.Unlock() + return entry.output, true } diff --git a/coderd/dynamicparameters/rendercache_test.go b/coderd/dynamicparameters/rendercache_test.go index 96885e4673be8..67cbb6cc58a7c 100644 --- a/coderd/dynamicparameters/rendercache_test.go +++ b/coderd/dynamicparameters/rendercache_test.go @@ -285,6 +285,56 @@ func TestRenderCache_CleanupRemovesExpiredEntries(t *testing.T) { require.Equal(t, float64(0), cacheSize.value, "all entries should be removed after cleanup") } +func TestRenderCache_TimestampRefreshOnHit(t *testing.T) { + t.Parallel() + + ctx := testutil.Context(t, testutil.WaitShort) + clock := quartz.NewMock(t) + + trapTickerFunc := clock.Trap().TickerFunc("render-cache-cleanup") + defer trapTickerFunc.Close() + + // Create cache with 1 second TTL for testing + cache := newRenderCache(clock, time.Second, nil, nil, nil) + defer cache.Close() + + // Wait for the initial cleanup and ticker to be created + trapTickerFunc.MustWait(ctx).Release(ctx) + + templateVersionID := uuid.New() + ownerID := uuid.New() + params := map[string]string{"region": "us-west-2"} + output := &preview.Output{} + + // Put an entry at T=0 + cache.put(templateVersionID, ownerID, params, output) + + // Advance time to 600ms (still within TTL) + clock.Advance(600 * time.Millisecond) + + // Access the entry - should hit and refresh timestamp to T=600ms + cached, ok := cache.get(templateVersionID, ownerID, params) + require.True(t, ok, "should hit cache") + require.Same(t, output, cached) + + // Advance another 600ms (now at T=1200ms) + // Entry was created at T=0 but refreshed at T=600ms, so it should still be valid + clock.Advance(600 * time.Millisecond) + + // Should still hit because timestamp was refreshed at T=600ms + cached, ok = cache.get(templateVersionID, ownerID, params) + require.True(t, ok, "should still hit cache because timestamp was refreshed") + require.Same(t, output, cached) + + // Now advance another 1.1 seconds (to T=2300ms) + // Last refresh was at T=1200ms, so now it should be expired + clock.Advance(1100 * time.Millisecond) + + // Should miss because more than 1 second since last access + _, ok = cache.get(templateVersionID, ownerID, params) + require.False(t, ok, "should miss cache after TTL from last access") +} + // Test implementations of prometheus interfaces type testCounter struct { value float64 From a54f1ae3ed0e38007c8f189f2a39a5dd7d045511 Mon Sep 17 00:00:00 2001 From: Callum Styan Date: Wed, 10 Dec 2025 00:18:50 +0000 Subject: [PATCH 06/14] refactor: make render cache non-nullable with interface Changes renderCache from a pointer to an interface to eliminate nil checks: - Created RenderCache interface with get/put methods - Renamed concrete type to RenderCacheImpl - Added noopRenderCache for default no-op behavior - Removed all nil checks in Render() method This simplifies the code by ensuring renderCache is always present, either as a real cache with metrics or as a no-op implementation. The nil checks were inconsistent with other non-lazy-loaded fields and added unnecessary complexity at each usage site. --- coderd/dynamicparameters/render.go | 34 +++++++++++++++++------- coderd/dynamicparameters/rendercache.go | 26 +++++++++--------- coderd/wsbuilder/wsbuilder.go | 4 +-- enterprise/coderd/prebuilds/reconcile.go | 2 +- 4 files changed, 41 insertions(+), 25 deletions(-) diff --git a/coderd/dynamicparameters/render.go b/coderd/dynamicparameters/render.go index ef59910e8e769..e97cf6203a5ea 100644 --- a/coderd/dynamicparameters/render.go +++ b/coderd/dynamicparameters/render.go @@ -35,6 +35,23 @@ type Renderer interface { var ErrTemplateVersionNotReady = xerrors.New("template version job not finished") +// RenderCache is an interface for caching preview.Preview results. +type RenderCache interface { + get(templateVersionID, ownerID uuid.UUID, parameters map[string]string) (*preview.Output, bool) + put(templateVersionID, ownerID uuid.UUID, parameters map[string]string, output *preview.Output) +} + +// noopRenderCache is a no-op implementation of RenderCache that doesn't cache anything. +type noopRenderCache struct{} + +func (noopRenderCache) get(uuid.UUID, uuid.UUID, map[string]string) (*preview.Output, bool) { + return nil, false +} + +func (noopRenderCache) put(uuid.UUID, uuid.UUID, map[string]string, *preview.Output) { + // no-op +} + // loader is used to load the necessary coder objects for rendering a template // version's parameters. The output is a Renderer, which is the object that uses // the cached objects to render the template version's parameters. @@ -48,7 +65,7 @@ type loader struct { templateVariableValues *[]database.TemplateVersionVariable // renderCache caches preview.Preview results - renderCache *RenderCache + renderCache RenderCache } // Prepare is the entrypoint for this package. It loads the necessary objects & @@ -57,6 +74,7 @@ type loader struct { func Prepare(ctx context.Context, db database.Store, cache files.FileAcquirer, versionID uuid.UUID, options ...func(r *loader)) (Renderer, error) { l := &loader{ templateVersionID: versionID, + renderCache: noopRenderCache{}, } for _, opt := range options { @@ -94,7 +112,7 @@ func WithTerraformValues(values database.TemplateVersionTerraformValue) func(r * } } -func WithRenderCache(cache *RenderCache) func(r *loader) { +func WithRenderCache(cache RenderCache) func(r *loader) { return func(r *loader) { r.renderCache = cache } @@ -237,11 +255,9 @@ type dynamicRenderer struct { } func (r *dynamicRenderer) Render(ctx context.Context, ownerID uuid.UUID, values map[string]string) (*preview.Output, hcl.Diagnostics) { - // Check cache first if available - if r.data.renderCache != nil { - if cached, ok := r.data.renderCache.get(r.data.templateVersionID, ownerID, values); ok { - return cached, nil - } + // Check cache first + if cached, ok := r.data.renderCache.get(r.data.templateVersionID, ownerID, values); ok { + return cached, nil } // Always start with the cached error, if we have one. @@ -277,8 +293,8 @@ func (r *dynamicRenderer) Render(ctx context.Context, ownerID uuid.UUID, values output, diags := preview.Preview(ctx, input, r.templateFS) - // Store in cache if successful and cache is available - if r.data.renderCache != nil && !diags.HasErrors() { + // Store in cache if successful + if !diags.HasErrors() { r.data.renderCache.put(r.data.templateVersionID, ownerID, values, output) } diff --git a/coderd/dynamicparameters/rendercache.go b/coderd/dynamicparameters/rendercache.go index 48eace932aaf4..2c7f69a40bf95 100644 --- a/coderd/dynamicparameters/rendercache.go +++ b/coderd/dynamicparameters/rendercache.go @@ -15,9 +15,9 @@ import ( "github.com/coder/quartz" ) -// RenderCache is a simple in-memory cache for preview.Preview results. +// RenderCacheImpl is a simple in-memory cache for preview.Preview results. // It caches based on (templateVersionID, ownerID, parameterValues). -type RenderCache struct { +type RenderCacheImpl struct { mu sync.RWMutex entries map[cacheKey]*cacheEntry @@ -46,17 +46,17 @@ type cacheKey struct { } // NewRenderCache creates a new render cache with a default TTL of 1 hour. -func NewRenderCache() *RenderCache { +func NewRenderCache() *RenderCacheImpl { return newRenderCache(quartz.NewReal(), time.Hour, nil, nil, nil) } // NewRenderCacheWithMetrics creates a new render cache with Prometheus metrics. -func NewRenderCacheWithMetrics(cacheHits, cacheMisses prometheus.Counter, cacheSize prometheus.Gauge) *RenderCache { +func NewRenderCacheWithMetrics(cacheHits, cacheMisses prometheus.Counter, cacheSize prometheus.Gauge) *RenderCacheImpl { return newRenderCache(quartz.NewReal(), time.Hour, cacheHits, cacheMisses, cacheSize) } -func newRenderCache(clock quartz.Clock, ttl time.Duration, cacheHits, cacheMisses prometheus.Counter, cacheSize prometheus.Gauge) *RenderCache { - c := &RenderCache{ +func newRenderCache(clock quartz.Clock, ttl time.Duration, cacheHits, cacheMisses prometheus.Counter, cacheSize prometheus.Gauge) *RenderCacheImpl { + c := &RenderCacheImpl{ entries: make(map[cacheKey]*cacheEntry), clock: clock, cacheHits: cacheHits, @@ -74,19 +74,19 @@ func newRenderCache(clock quartz.Clock, ttl time.Duration, cacheHits, cacheMisse } // NewRenderCacheForTest creates a new render cache for testing purposes. -func NewRenderCacheForTest() *RenderCache { +func NewRenderCacheForTest() *RenderCacheImpl { return NewRenderCache() } // Close stops the cleanup goroutine and waits for it to finish. -func (c *RenderCache) Close() { +func (c *RenderCacheImpl) Close() { c.stopOnce.Do(func() { close(c.stopCh) <-c.doneCh }) } -func (c *RenderCache) get(templateVersionID, ownerID uuid.UUID, parameters map[string]string) (*preview.Output, bool) { +func (c *RenderCacheImpl) get(templateVersionID, ownerID uuid.UUID, parameters map[string]string) (*preview.Output, bool) { key := c.makeKey(templateVersionID, ownerID, parameters) c.mu.RLock() entry, ok := c.entries[key] @@ -122,7 +122,7 @@ func (c *RenderCache) get(templateVersionID, ownerID uuid.UUID, parameters map[s return entry.output, true } -func (c *RenderCache) put(templateVersionID, ownerID uuid.UUID, parameters map[string]string, output *preview.Output) { +func (c *RenderCacheImpl) put(templateVersionID, ownerID uuid.UUID, parameters map[string]string, output *preview.Output) { key := c.makeKey(templateVersionID, ownerID, parameters) c.mu.Lock() defer c.mu.Unlock() @@ -138,7 +138,7 @@ func (c *RenderCache) put(templateVersionID, ownerID uuid.UUID, parameters map[s } } -func (c *RenderCache) makeKey(templateVersionID, ownerID uuid.UUID, parameters map[string]string) cacheKey { +func (c *RenderCacheImpl) makeKey(templateVersionID, ownerID uuid.UUID, parameters map[string]string) cacheKey { return cacheKey{ templateVersionID: templateVersionID, ownerID: ownerID, @@ -172,7 +172,7 @@ func hashParameters(params map[string]string) string { } // cleanupLoop runs periodically to remove expired cache entries. -func (c *RenderCache) cleanupLoop(ctx context.Context) { +func (c *RenderCacheImpl) cleanupLoop(ctx context.Context) { defer close(c.doneCh) // Run cleanup every 15 minutes @@ -199,7 +199,7 @@ func (c *RenderCache) cleanupLoop(ctx context.Context) { } // cleanup removes expired entries from the cache. -func (c *RenderCache) cleanup() { +func (c *RenderCacheImpl) cleanup() { c.mu.Lock() defer c.mu.Unlock() diff --git a/coderd/wsbuilder/wsbuilder.go b/coderd/wsbuilder/wsbuilder.go index 5ca038530e96f..b298f005da2dc 100644 --- a/coderd/wsbuilder/wsbuilder.go +++ b/coderd/wsbuilder/wsbuilder.go @@ -89,7 +89,7 @@ type Builder struct { workspaceTags *map[string]string // renderCache caches template rendering results - renderCache *dynamicparameters.RenderCache + renderCache *dynamicparameters.RenderCacheImpl prebuiltWorkspaceBuildStage sdkproto.PrebuiltWorkspaceBuildStage verifyNoLegacyParametersOnce bool @@ -258,7 +258,7 @@ func (b Builder) TemplateVersionPresetID(id uuid.UUID) Builder { // RenderCache sets the render cache to use for template rendering. // This allows multiple workspace builds to share cached render results. -func (b Builder) RenderCache(cache *dynamicparameters.RenderCache) Builder { +func (b Builder) RenderCache(cache *dynamicparameters.RenderCacheImpl) Builder { // nolint: revive b.renderCache = cache return b diff --git a/enterprise/coderd/prebuilds/reconcile.go b/enterprise/coderd/prebuilds/reconcile.go index 584b4f58e7119..ef53c422daa17 100644 --- a/enterprise/coderd/prebuilds/reconcile.go +++ b/enterprise/coderd/prebuilds/reconcile.go @@ -61,7 +61,7 @@ type StoreReconciler struct { reconciliationDuration prometheus.Histogram // renderCache caches template rendering results to avoid expensive re-parsing - renderCache *dynamicparameters.RenderCache + renderCache *dynamicparameters.RenderCacheImpl } var _ prebuilds.ReconciliationOrchestrator = &StoreReconciler{} From 890a058443ba1d0297073f746ba0bfc4ac365114 Mon Sep 17 00:00:00 2001 From: Callum Styan Date: Wed, 10 Dec 2025 18:16:53 +0000 Subject: [PATCH 07/14] fix lint and fmt, required refactoring of the metrics testing Signed-off-by: Callum Styan --- coderd/dynamicparameters/render.go | 1 - coderd/dynamicparameters/rendercache.go | 37 +++--- ...e_test.go => rendercache_internal_test.go} | 123 +++++------------- 3 files changed, 51 insertions(+), 110 deletions(-) rename coderd/dynamicparameters/{rendercache_test.go => rendercache_internal_test.go} (77%) diff --git a/coderd/dynamicparameters/render.go b/coderd/dynamicparameters/render.go index e97cf6203a5ea..079f083469c5f 100644 --- a/coderd/dynamicparameters/render.go +++ b/coderd/dynamicparameters/render.go @@ -118,7 +118,6 @@ func WithRenderCache(cache RenderCache) func(r *loader) { } } - func (r *loader) loadData(ctx context.Context, db database.Store) error { if r.templateVersion == nil { tv, err := db.GetTemplateVersionByID(ctx, r.templateVersionID) diff --git a/coderd/dynamicparameters/rendercache.go b/coderd/dynamicparameters/rendercache.go index 2c7f69a40bf95..28f5da646774a 100644 --- a/coderd/dynamicparameters/rendercache.go +++ b/coderd/dynamicparameters/rendercache.go @@ -2,12 +2,12 @@ package dynamicparameters import ( "context" - "crypto/md5" - "encoding/hex" + "fmt" "sort" "sync" "time" + "github.com/cespare/xxhash/v2" "github.com/google/uuid" "github.com/prometheus/client_golang/prometheus" @@ -41,21 +41,21 @@ type cacheEntry struct { type cacheKey struct { templateVersionID uuid.UUID - ownerID uuid.UUID - parameterHash string + ownerID uuid.UUID + parameterHash uint64 } // NewRenderCache creates a new render cache with a default TTL of 1 hour. func NewRenderCache() *RenderCacheImpl { - return newRenderCache(quartz.NewReal(), time.Hour, nil, nil, nil) + return newCache(quartz.NewReal(), time.Hour, nil, nil, nil) } // NewRenderCacheWithMetrics creates a new render cache with Prometheus metrics. func NewRenderCacheWithMetrics(cacheHits, cacheMisses prometheus.Counter, cacheSize prometheus.Gauge) *RenderCacheImpl { - return newRenderCache(quartz.NewReal(), time.Hour, cacheHits, cacheMisses, cacheSize) + return newCache(quartz.NewReal(), time.Hour, cacheHits, cacheMisses, cacheSize) } -func newRenderCache(clock quartz.Clock, ttl time.Duration, cacheHits, cacheMisses prometheus.Counter, cacheSize prometheus.Gauge) *RenderCacheImpl { +func newCache(clock quartz.Clock, ttl time.Duration, cacheHits, cacheMisses prometheus.Counter, cacheSize prometheus.Gauge) *RenderCacheImpl { c := &RenderCacheImpl{ entries: make(map[cacheKey]*cacheEntry), clock: clock, @@ -87,7 +87,7 @@ func (c *RenderCacheImpl) Close() { } func (c *RenderCacheImpl) get(templateVersionID, ownerID uuid.UUID, parameters map[string]string) (*preview.Output, bool) { - key := c.makeKey(templateVersionID, ownerID, parameters) + key := makeKey(templateVersionID, ownerID, parameters) c.mu.RLock() entry, ok := c.entries[key] c.mu.RUnlock() @@ -123,7 +123,7 @@ func (c *RenderCacheImpl) get(templateVersionID, ownerID uuid.UUID, parameters m } func (c *RenderCacheImpl) put(templateVersionID, ownerID uuid.UUID, parameters map[string]string, output *preview.Output) { - key := c.makeKey(templateVersionID, ownerID, parameters) + key := makeKey(templateVersionID, ownerID, parameters) c.mu.Lock() defer c.mu.Unlock() @@ -138,18 +138,18 @@ func (c *RenderCacheImpl) put(templateVersionID, ownerID uuid.UUID, parameters m } } -func (c *RenderCacheImpl) makeKey(templateVersionID, ownerID uuid.UUID, parameters map[string]string) cacheKey { +func makeKey(templateVersionID, ownerID uuid.UUID, parameters map[string]string) cacheKey { return cacheKey{ templateVersionID: templateVersionID, - ownerID: ownerID, - parameterHash: hashParameters(parameters), + ownerID: ownerID, + parameterHash: hashParameters(parameters), } } // hashParameters creates a deterministic hash of the parameter map. -func hashParameters(params map[string]string) string { +func hashParameters(params map[string]string) uint64 { if len(params) == 0 { - return "" + return 0 } // Sort keys for deterministic hashing @@ -160,15 +160,12 @@ func hashParameters(params map[string]string) string { sort.Strings(keys) // Hash the sorted key-value pairs - h := md5.New() + var b string for _, k := range keys { - h.Write([]byte(k)) - h.Write([]byte{0}) // separator - h.Write([]byte(params[k])) - h.Write([]byte{0}) // separator + b += fmt.Sprintf("%s:%s,", k, params[k]) } - return hex.EncodeToString(h.Sum(nil)) + return xxhash.Sum64String(b) } // cleanupLoop runs periodically to remove expired cache entries. diff --git a/coderd/dynamicparameters/rendercache_test.go b/coderd/dynamicparameters/rendercache_internal_test.go similarity index 77% rename from coderd/dynamicparameters/rendercache_test.go rename to coderd/dynamicparameters/rendercache_internal_test.go index 67cbb6cc58a7c..22d005b25d39f 100644 --- a/coderd/dynamicparameters/rendercache_test.go +++ b/coderd/dynamicparameters/rendercache_internal_test.go @@ -6,7 +6,7 @@ import ( "github.com/google/uuid" "github.com/prometheus/client_golang/prometheus" - dto "github.com/prometheus/client_model/go" + promtestutil "github.com/prometheus/client_golang/prometheus/testutil" "github.com/stretchr/testify/require" "github.com/coder/coder/v2/testutil" @@ -157,9 +157,15 @@ func TestRenderCache_Metrics(t *testing.T) { t.Parallel() // Create test metrics - cacheHits := &testCounter{} - cacheMisses := &testCounter{} - cacheSize := &testGauge{} + cacheHits := prometheus.NewCounter(prometheus.CounterOpts{ + Name: "test_cache_hits_total", + }) + cacheMisses := prometheus.NewCounter(prometheus.CounterOpts{ + Name: "test_cache_misses_total", + }) + cacheSize := prometheus.NewGauge(prometheus.GaugeOpts{ + Name: "test_cache_entries", + }) cache := NewRenderCacheWithMetrics(cacheHits, cacheMisses, cacheSize) templateVersionID := uuid.New() @@ -167,45 +173,45 @@ func TestRenderCache_Metrics(t *testing.T) { params := map[string]string{"region": "us-west-2"} // Initially: 0 hits, 0 misses, 0 size - require.Equal(t, float64(0), cacheHits.value, "initial hits should be 0") - require.Equal(t, float64(0), cacheMisses.value, "initial misses should be 0") - require.Equal(t, float64(0), cacheSize.value, "initial size should be 0") + require.Equal(t, float64(0), promtestutil.ToFloat64(cacheHits), "initial hits should be 0") + require.Equal(t, float64(0), promtestutil.ToFloat64(cacheMisses), "initial misses should be 0") + require.Equal(t, float64(0), promtestutil.ToFloat64(cacheSize), "initial size should be 0") // First get - should be a miss _, ok := cache.get(templateVersionID, ownerID, params) require.False(t, ok) - require.Equal(t, float64(0), cacheHits.value, "hits should still be 0") - require.Equal(t, float64(1), cacheMisses.value, "misses should be 1") - require.Equal(t, float64(0), cacheSize.value, "size should still be 0") + require.Equal(t, float64(0), promtestutil.ToFloat64(cacheHits), "hits should still be 0") + require.Equal(t, float64(1), promtestutil.ToFloat64(cacheMisses), "misses should be 1") + require.Equal(t, float64(0), promtestutil.ToFloat64(cacheSize), "size should still be 0") // Put an entry output := &preview.Output{} cache.put(templateVersionID, ownerID, params, output) - require.Equal(t, float64(1), cacheSize.value, "size should be 1 after put") + require.Equal(t, float64(1), promtestutil.ToFloat64(cacheSize), "size should be 1 after put") // Second get - should be a hit _, ok = cache.get(templateVersionID, ownerID, params) require.True(t, ok) - require.Equal(t, float64(1), cacheHits.value, "hits should be 1") - require.Equal(t, float64(1), cacheMisses.value, "misses should still be 1") - require.Equal(t, float64(1), cacheSize.value, "size should still be 1") + require.Equal(t, float64(1), promtestutil.ToFloat64(cacheHits), "hits should be 1") + require.Equal(t, float64(1), promtestutil.ToFloat64(cacheMisses), "misses should still be 1") + require.Equal(t, float64(1), promtestutil.ToFloat64(cacheSize), "size should still be 1") // Third get - another hit _, ok = cache.get(templateVersionID, ownerID, params) require.True(t, ok) - require.Equal(t, float64(2), cacheHits.value, "hits should be 2") - require.Equal(t, float64(1), cacheMisses.value, "misses should still be 1") + require.Equal(t, float64(2), promtestutil.ToFloat64(cacheHits), "hits should be 2") + require.Equal(t, float64(1), promtestutil.ToFloat64(cacheMisses), "misses should still be 1") // Put another entry with different params params2 := map[string]string{"region": "us-east-1"} cache.put(templateVersionID, ownerID, params2, output) - require.Equal(t, float64(2), cacheSize.value, "size should be 2 after second put") + require.Equal(t, float64(2), promtestutil.ToFloat64(cacheSize), "size should be 2 after second put") // Get with different params - should be a hit _, ok = cache.get(templateVersionID, ownerID, params2) require.True(t, ok) - require.Equal(t, float64(3), cacheHits.value, "hits should be 3") - require.Equal(t, float64(1), cacheMisses.value, "misses should still be 1") + require.Equal(t, float64(3), promtestutil.ToFloat64(cacheHits), "hits should be 3") + require.Equal(t, float64(1), promtestutil.ToFloat64(cacheMisses), "misses should still be 1") } func TestRenderCache_TTL(t *testing.T) { @@ -218,7 +224,7 @@ func TestRenderCache_TTL(t *testing.T) { defer trapTickerFunc.Close() // Create cache with short TTL for testing - cache := newRenderCache(clock, 100*time.Millisecond, nil, nil, nil) + cache := newCache(clock, 100*time.Millisecond, nil, nil, nil) defer cache.Close() // Wait for the initial cleanup and ticker to be created @@ -254,15 +260,17 @@ func TestRenderCache_CleanupRemovesExpiredEntries(t *testing.T) { trapTickerFunc := clock.Trap().TickerFunc("render-cache-cleanup") defer trapTickerFunc.Close() - cacheSize := &testGauge{} - cache := newRenderCache(clock, 100*time.Millisecond, nil, nil, cacheSize) + cacheSize := prometheus.NewGauge(prometheus.GaugeOpts{ + Name: "test_cache_entries", + }) + cache := newCache(clock, 100*time.Millisecond, nil, nil, cacheSize) defer cache.Close() // Wait for the initial cleanup and ticker to be created trapTickerFunc.MustWait(ctx).Release(ctx) // Initial size should be 0 after first cleanup - require.Equal(t, float64(0), cacheSize.value, "should have 0 entries initially") + require.Equal(t, float64(0), promtestutil.ToFloat64(cacheSize), "should have 0 entries initially") templateVersionID := uuid.New() ownerID := uuid.New() @@ -273,7 +281,7 @@ func TestRenderCache_CleanupRemovesExpiredEntries(t *testing.T) { cache.put(templateVersionID, ownerID, params, &preview.Output{}) } - require.Equal(t, float64(3), cacheSize.value, "should have 3 entries") + require.Equal(t, float64(3), promtestutil.ToFloat64(cacheSize), "should have 3 entries") // Advance time beyond TTL clock.Advance(150 * time.Millisecond) @@ -282,7 +290,7 @@ func TestRenderCache_CleanupRemovesExpiredEntries(t *testing.T) { clock.Advance(15*time.Minute - 150*time.Millisecond).MustWait(ctx) // All entries should be removed - require.Equal(t, float64(0), cacheSize.value, "all entries should be removed after cleanup") + require.Equal(t, float64(0), promtestutil.ToFloat64(cacheSize), "all entries should be removed after cleanup") } func TestRenderCache_TimestampRefreshOnHit(t *testing.T) { @@ -295,7 +303,7 @@ func TestRenderCache_TimestampRefreshOnHit(t *testing.T) { defer trapTickerFunc.Close() // Create cache with 1 second TTL for testing - cache := newRenderCache(clock, time.Second, nil, nil, nil) + cache := newCache(clock, time.Second, nil, nil, nil) defer cache.Close() // Wait for the initial cleanup and ticker to be created @@ -334,66 +342,3 @@ func TestRenderCache_TimestampRefreshOnHit(t *testing.T) { _, ok = cache.get(templateVersionID, ownerID, params) require.False(t, ok, "should miss cache after TTL from last access") } - -// Test implementations of prometheus interfaces -type testCounter struct { - value float64 -} - -func (c *testCounter) Inc() { - c.value++ -} - -func (c *testCounter) Add(v float64) { - c.value += v -} - -func (c *testCounter) Desc() *prometheus.Desc { - return nil -} - -func (c *testCounter) Write(*dto.Metric) error { - return nil -} - -func (c *testCounter) Describe(chan<- *prometheus.Desc) {} - -func (c *testCounter) Collect(chan<- prometheus.Metric) {} - -type testGauge struct { - value float64 -} - -func (g *testGauge) Set(v float64) { - g.value = v -} - -func (g *testGauge) Inc() { - g.value++ -} - -func (g *testGauge) Dec() { - g.value-- -} - -func (g *testGauge) Add(v float64) { - g.value += v -} - -func (g *testGauge) Sub(v float64) { - g.value -= v -} - -func (g *testGauge) SetToCurrentTime() {} - -func (g *testGauge) Desc() *prometheus.Desc { - return nil -} - -func (g *testGauge) Write(*dto.Metric) error { - return nil -} - -func (g *testGauge) Describe(chan<- *prometheus.Desc) {} - -func (g *testGauge) Collect(chan<- prometheus.Metric) {} From f7daab4da3b8d0efa5847882d488e387af6f1dbe Mon Sep 17 00:00:00 2001 From: Callum Styan Date: Wed, 10 Dec 2025 22:07:49 +0000 Subject: [PATCH 08/14] fix missing Close calls in tests Signed-off-by: Callum Styan --- coderd/dynamicparameters/rendercache_internal_test.go | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/coderd/dynamicparameters/rendercache_internal_test.go b/coderd/dynamicparameters/rendercache_internal_test.go index 22d005b25d39f..3a6cf8489e9af 100644 --- a/coderd/dynamicparameters/rendercache_internal_test.go +++ b/coderd/dynamicparameters/rendercache_internal_test.go @@ -19,6 +19,7 @@ func TestRenderCache_BasicOperations(t *testing.T) { t.Parallel() cache := NewRenderCache() + defer cache.Close() templateVersionID := uuid.New() ownerID := uuid.New() params := map[string]string{"region": "us-west-2"} @@ -50,6 +51,8 @@ func TestRenderCache_DifferentKeysAreSeparate(t *testing.T) { t.Parallel() cache := NewRenderCache() + defer cache.Close() + templateVersion1 := uuid.New() templateVersion2 := uuid.New() owner1 := uuid.New() @@ -83,6 +86,8 @@ func TestRenderCache_ParameterHashConsistency(t *testing.T) { t.Parallel() cache := NewRenderCache() + defer cache.Close() + templateVersionID := uuid.New() ownerID := uuid.New() @@ -103,6 +108,8 @@ func TestRenderCache_EmptyParameters(t *testing.T) { t.Parallel() cache := NewRenderCache() + defer cache.Close() + templateVersionID := uuid.New() ownerID := uuid.New() @@ -123,6 +130,7 @@ func TestRenderCache_PrebuildScenario(t *testing.T) { // This test simulates the prebuild scenario where multiple prebuilds // are created from the same template version with the same preset parameters. cache := NewRenderCache() + defer cache.Close() // In prebuilds, all instances use the same fixed ownerID prebuildOwnerID := uuid.MustParse("c42fdf75-3097-471c-8c33-fb52454d81c0") // database.PrebuildsSystemUserID @@ -168,6 +176,8 @@ func TestRenderCache_Metrics(t *testing.T) { }) cache := NewRenderCacheWithMetrics(cacheHits, cacheMisses, cacheSize) + defer cache.Close() + templateVersionID := uuid.New() ownerID := uuid.New() params := map[string]string{"region": "us-west-2"} From ba4e5085d6fa0f74b75e89ed46d3f21023c7d333 Mon Sep 17 00:00:00 2001 From: Callum Styan Date: Wed, 10 Dec 2025 22:39:19 +0000 Subject: [PATCH 09/14] fix: cleanup render cache goroutines in tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previously, tests creating StoreReconciler instances would leak cleanup goroutines because they never called Stop() to close the render cache. This caused goleak test failures. Add t.Cleanup() calls to all tests creating reconcilers to ensure the render cache cleanup goroutines are properly stopped. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 --- enterprise/cli/create_test.go | 6 ++++++ enterprise/coderd/prebuilds/claim_test.go | 3 +++ .../coderd/prebuilds/metricscollector_test.go | 15 +++++++++++++++ enterprise/coderd/prebuilds/reconcile_test.go | 9 +++++++++ enterprise/coderd/workspaces_test.go | 18 ++++++++++++++++++ 5 files changed, 51 insertions(+) diff --git a/enterprise/cli/create_test.go b/enterprise/cli/create_test.go index 44218abb5a58d..1a4623cfa8a6c 100644 --- a/enterprise/cli/create_test.go +++ b/enterprise/cli/create_test.go @@ -371,6 +371,9 @@ func TestEnterpriseCreateWithPreset(t *testing.T) { notifications.NewNoopEnqueuer(), newNoopUsageCheckerPtr(), ) + t.Cleanup(func() { + reconciler.Stop(context.Background(), nil) + }) var claimer agplprebuilds.Claimer = prebuilds.NewEnterpriseClaimer(db) api.AGPL.PrebuildsClaimer.Store(&claimer) @@ -482,6 +485,9 @@ func TestEnterpriseCreateWithPreset(t *testing.T) { notifications.NewNoopEnqueuer(), newNoopUsageCheckerPtr(), ) + t.Cleanup(func() { + reconciler.Stop(context.Background(), nil) + }) var claimer agplprebuilds.Claimer = prebuilds.NewEnterpriseClaimer(db) api.AGPL.PrebuildsClaimer.Store(&claimer) diff --git a/enterprise/coderd/prebuilds/claim_test.go b/enterprise/coderd/prebuilds/claim_test.go index 217a9ff09614a..a9563888c83c1 100644 --- a/enterprise/coderd/prebuilds/claim_test.go +++ b/enterprise/coderd/prebuilds/claim_test.go @@ -168,6 +168,9 @@ func TestClaimPrebuild(t *testing.T) { cache := files.New(prometheus.NewRegistry(), &coderdtest.FakeAuthorizer{}) reconciler := prebuilds.NewStoreReconciler(spy, pubsub, cache, codersdk.PrebuildsConfig{}, logger, quartz.NewMock(t), prometheus.NewRegistry(), newNoopEnqueuer(), newNoopUsageCheckerPtr()) + t.Cleanup(func() { + reconciler.Stop(context.Background(), nil) + }) var claimer agplprebuilds.Claimer = prebuilds.NewEnterpriseClaimer(spy) api.AGPL.PrebuildsClaimer.Store(&claimer) diff --git a/enterprise/coderd/prebuilds/metricscollector_test.go b/enterprise/coderd/prebuilds/metricscollector_test.go index aa9886fb7ad1b..a98b256e546ba 100644 --- a/enterprise/coderd/prebuilds/metricscollector_test.go +++ b/enterprise/coderd/prebuilds/metricscollector_test.go @@ -198,6 +198,9 @@ func TestMetricsCollector(t *testing.T) { db, pubsub := dbtestutil.NewDB(t) cache := files.New(prometheus.NewRegistry(), &coderdtest.FakeAuthorizer{}) reconciler := prebuilds.NewStoreReconciler(db, pubsub, cache, codersdk.PrebuildsConfig{}, logger, quartz.NewMock(t), prometheus.NewRegistry(), newNoopEnqueuer(), newNoopUsageCheckerPtr()) + t.Cleanup(func() { + reconciler.Stop(context.Background(), nil) + }) ctx := testutil.Context(t, testutil.WaitLong) createdUsers := []uuid.UUID{database.PrebuildsSystemUserID} @@ -330,6 +333,9 @@ func TestMetricsCollector_DuplicateTemplateNames(t *testing.T) { db, pubsub := dbtestutil.NewDB(t) cache := files.New(prometheus.NewRegistry(), &coderdtest.FakeAuthorizer{}) reconciler := prebuilds.NewStoreReconciler(db, pubsub, cache, codersdk.PrebuildsConfig{}, logger, quartz.NewMock(t), prometheus.NewRegistry(), newNoopEnqueuer(), newNoopUsageCheckerPtr()) + t.Cleanup(func() { + reconciler.Stop(context.Background(), nil) + }) ctx := testutil.Context(t, testutil.WaitLong) collector := prebuilds.NewMetricsCollector(db, logger, reconciler) @@ -478,6 +484,9 @@ func TestMetricsCollector_ReconciliationPausedMetric(t *testing.T) { cache := files.New(prometheus.NewRegistry(), &coderdtest.FakeAuthorizer{}) registry := prometheus.NewPedanticRegistry() reconciler := prebuilds.NewStoreReconciler(db, pubsub, cache, codersdk.PrebuildsConfig{}, logger, quartz.NewMock(t), registry, newNoopEnqueuer(), newNoopUsageCheckerPtr()) + t.Cleanup(func() { + reconciler.Stop(context.Background(), nil) + }) ctx := testutil.Context(t, testutil.WaitLong) // Ensure no pause setting is set (default state) @@ -507,6 +516,9 @@ func TestMetricsCollector_ReconciliationPausedMetric(t *testing.T) { cache := files.New(prometheus.NewRegistry(), &coderdtest.FakeAuthorizer{}) registry := prometheus.NewPedanticRegistry() reconciler := prebuilds.NewStoreReconciler(db, pubsub, cache, codersdk.PrebuildsConfig{}, logger, quartz.NewMock(t), registry, newNoopEnqueuer(), newNoopUsageCheckerPtr()) + t.Cleanup(func() { + reconciler.Stop(context.Background(), nil) + }) ctx := testutil.Context(t, testutil.WaitLong) // Set reconciliation to paused @@ -536,6 +548,9 @@ func TestMetricsCollector_ReconciliationPausedMetric(t *testing.T) { cache := files.New(prometheus.NewRegistry(), &coderdtest.FakeAuthorizer{}) registry := prometheus.NewPedanticRegistry() reconciler := prebuilds.NewStoreReconciler(db, pubsub, cache, codersdk.PrebuildsConfig{}, logger, quartz.NewMock(t), registry, newNoopEnqueuer(), newNoopUsageCheckerPtr()) + t.Cleanup(func() { + reconciler.Stop(context.Background(), nil) + }) ctx := testutil.Context(t, testutil.WaitLong) // Set reconciliation back to not paused diff --git a/enterprise/coderd/prebuilds/reconcile_test.go b/enterprise/coderd/prebuilds/reconcile_test.go index 7548faebd7dab..b6fd0732fc536 100644 --- a/enterprise/coderd/prebuilds/reconcile_test.go +++ b/enterprise/coderd/prebuilds/reconcile_test.go @@ -53,6 +53,9 @@ func TestNoReconciliationActionsIfNoPresets(t *testing.T) { logger := testutil.Logger(t) cache := files.New(prometheus.NewRegistry(), &coderdtest.FakeAuthorizer{}) controller := prebuilds.NewStoreReconciler(db, ps, cache, cfg, logger, quartz.NewMock(t), prometheus.NewRegistry(), newNoopEnqueuer(), newNoopUsageCheckerPtr()) + t.Cleanup(func() { + controller.Stop(context.Background(), nil) + }) // given a template version with no presets org := dbgen.Organization(t, db, database.Organization{}) @@ -96,6 +99,9 @@ func TestNoReconciliationActionsIfNoPrebuilds(t *testing.T) { logger := testutil.Logger(t) cache := files.New(prometheus.NewRegistry(), &coderdtest.FakeAuthorizer{}) controller := prebuilds.NewStoreReconciler(db, ps, cache, cfg, logger, quartz.NewMock(t), prometheus.NewRegistry(), newNoopEnqueuer(), newNoopUsageCheckerPtr()) + t.Cleanup(func() { + controller.Stop(context.Background(), nil) + }) // given there are presets, but no prebuilds org := dbgen.Organization(t, db, database.Organization{}) @@ -426,6 +432,9 @@ func (tc testCase) run(t *testing.T) { } cache := files.New(prometheus.NewRegistry(), &coderdtest.FakeAuthorizer{}) controller := prebuilds.NewStoreReconciler(db, pubSub, cache, cfg, logger, quartz.NewMock(t), prometheus.NewRegistry(), newNoopEnqueuer(), newNoopUsageCheckerPtr()) + t.Cleanup(func() { + controller.Stop(context.Background(), nil) + }) // Run the reconciliation multiple times to ensure idempotency // 8 was arbitrary, but large enough to reasonably trust the result diff --git a/enterprise/coderd/workspaces_test.go b/enterprise/coderd/workspaces_test.go index 7cf9cd890b6df..c9a40f539afa5 100644 --- a/enterprise/coderd/workspaces_test.go +++ b/enterprise/coderd/workspaces_test.go @@ -1978,6 +1978,9 @@ func TestPrebuildsAutobuild(t *testing.T) { notificationsNoop, api.AGPL.BuildUsageChecker, ) + t.Cleanup(func() { + reconciler.Stop(context.Background(), nil) + }) var claimer agplprebuilds.Claimer = prebuilds.NewEnterpriseClaimer(db) api.AGPL.PrebuildsClaimer.Store(&claimer) @@ -2100,6 +2103,9 @@ func TestPrebuildsAutobuild(t *testing.T) { notificationsNoop, api.AGPL.BuildUsageChecker, ) + t.Cleanup(func() { + reconciler.Stop(context.Background(), nil) + }) var claimer agplprebuilds.Claimer = prebuilds.NewEnterpriseClaimer(db) api.AGPL.PrebuildsClaimer.Store(&claimer) @@ -2222,6 +2228,9 @@ func TestPrebuildsAutobuild(t *testing.T) { notificationsNoop, api.AGPL.BuildUsageChecker, ) + t.Cleanup(func() { + reconciler.Stop(context.Background(), nil) + }) var claimer agplprebuilds.Claimer = prebuilds.NewEnterpriseClaimer(db) api.AGPL.PrebuildsClaimer.Store(&claimer) @@ -2366,6 +2375,9 @@ func TestPrebuildsAutobuild(t *testing.T) { notificationsNoop, api.AGPL.BuildUsageChecker, ) + t.Cleanup(func() { + reconciler.Stop(context.Background(), nil) + }) var claimer agplprebuilds.Claimer = prebuilds.NewEnterpriseClaimer(db) api.AGPL.PrebuildsClaimer.Store(&claimer) @@ -2511,6 +2523,9 @@ func TestPrebuildsAutobuild(t *testing.T) { notificationsNoop, api.AGPL.BuildUsageChecker, ) + t.Cleanup(func() { + reconciler.Stop(context.Background(), nil) + }) var claimer agplprebuilds.Claimer = prebuilds.NewEnterpriseClaimer(db) api.AGPL.PrebuildsClaimer.Store(&claimer) @@ -2957,6 +2972,9 @@ func TestWorkspaceProvisionerdServerMetrics(t *testing.T) { notifications.NewNoopEnqueuer(), api.AGPL.BuildUsageChecker, ) + t.Cleanup(func() { + reconciler.Stop(context.Background(), nil) + }) var claimer agplprebuilds.Claimer = prebuilds.NewEnterpriseClaimer(db) api.AGPL.PrebuildsClaimer.Store(&claimer) From bea92ad397093aaaa40be6ae9d5593d7fa261012 Mon Sep 17 00:00:00 2001 From: Callum Styan Date: Thu, 11 Dec 2025 03:44:24 +0000 Subject: [PATCH 10/14] fix: add missing context import and remaining cleanup calls MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Add missing context import to metricscollector_test.go - Add cleanup calls for all remaining NewStoreReconciler instances in reconcile_test.go, including those in goroutines This completes the fix for goroutine leaks in render cache tests. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 --- .../coderd/prebuilds/metricscollector_test.go | 1 + enterprise/coderd/prebuilds/reconcile_test.go | 22 +++++++++++++++++++ 2 files changed, 23 insertions(+) diff --git a/enterprise/coderd/prebuilds/metricscollector_test.go b/enterprise/coderd/prebuilds/metricscollector_test.go index a98b256e546ba..91cb95f825aca 100644 --- a/enterprise/coderd/prebuilds/metricscollector_test.go +++ b/enterprise/coderd/prebuilds/metricscollector_test.go @@ -1,6 +1,7 @@ package prebuilds_test import ( + "context" "fmt" "slices" "testing" diff --git a/enterprise/coderd/prebuilds/reconcile_test.go b/enterprise/coderd/prebuilds/reconcile_test.go index b6fd0732fc536..bb871fe5682b5 100644 --- a/enterprise/coderd/prebuilds/reconcile_test.go +++ b/enterprise/coderd/prebuilds/reconcile_test.go @@ -1221,6 +1221,9 @@ func TestRunLoop(t *testing.T) { db, pubSub := dbtestutil.NewDB(t) cache := files.New(prometheus.NewRegistry(), &coderdtest.FakeAuthorizer{}) reconciler := prebuilds.NewStoreReconciler(db, pubSub, cache, cfg, logger, clock, prometheus.NewRegistry(), newNoopEnqueuer(), newNoopUsageCheckerPtr()) + t.Cleanup(func() { + reconciler.Stop(context.Background(), nil) + }) ownerID := uuid.New() dbgen.User(t, db, database.User{ @@ -1349,6 +1352,9 @@ func TestFailedBuildBackoff(t *testing.T) { db, ps := dbtestutil.NewDB(t) cache := files.New(prometheus.NewRegistry(), &coderdtest.FakeAuthorizer{}) reconciler := prebuilds.NewStoreReconciler(db, ps, cache, cfg, logger, clock, prometheus.NewRegistry(), newNoopEnqueuer(), newNoopUsageCheckerPtr()) + t.Cleanup(func() { + reconciler.Stop(context.Background(), nil) + }) // Given: an active template version with presets and prebuilds configured. const desiredInstances = 2 @@ -1471,6 +1477,7 @@ func TestReconciliationLock(t *testing.T) { prometheus.NewRegistry(), newNoopEnqueuer(), newNoopUsageCheckerPtr()) + defer reconciler.Stop(context.Background(), nil) reconciler.WithReconciliationLock(ctx, logger, func(_ context.Context, _ database.Store) error { lockObtained := mutex.TryLock() // As long as the postgres lock is held, this mutex should always be unlocked when we get here. @@ -1501,6 +1508,9 @@ func TestTrackResourceReplacement(t *testing.T) { registry := prometheus.NewRegistry() cache := files.New(registry, &coderdtest.FakeAuthorizer{}) reconciler := prebuilds.NewStoreReconciler(db, ps, cache, codersdk.PrebuildsConfig{}, logger, clock, registry, fakeEnqueuer, newNoopUsageCheckerPtr()) + t.Cleanup(func() { + reconciler.Stop(context.Background(), nil) + }) // Given: a template admin to receive a notification. templateAdmin := dbgen.User(t, db, database.User{ @@ -2108,6 +2118,9 @@ func TestCancelPendingPrebuilds(t *testing.T) { cache := files.New(registry, &coderdtest.FakeAuthorizer{}) logger := slogtest.Make(t, &slogtest.Options{IgnoreErrors: false}).Leveled(slog.LevelDebug) reconciler := prebuilds.NewStoreReconciler(db, ps, cache, codersdk.PrebuildsConfig{}, logger, clock, registry, fakeEnqueuer, newNoopUsageCheckerPtr()) + t.Cleanup(func() { + reconciler.Stop(context.Background(), nil) + }) owner := coderdtest.CreateFirstUser(t, client) // Given: a template with a version containing a preset with 1 prebuild instance @@ -2345,6 +2358,9 @@ func TestCancelPendingPrebuilds(t *testing.T) { cache := files.New(registry, &coderdtest.FakeAuthorizer{}) logger := slogtest.Make(t, &slogtest.Options{IgnoreErrors: false}).Leveled(slog.LevelDebug) reconciler := prebuilds.NewStoreReconciler(db, ps, cache, codersdk.PrebuildsConfig{}, logger, clock, registry, fakeEnqueuer, newNoopUsageCheckerPtr()) + t.Cleanup(func() { + reconciler.Stop(context.Background(), nil) + }) owner := coderdtest.CreateFirstUser(t, client) // Given: template A with 2 versions @@ -2410,6 +2426,9 @@ func TestReconciliationStats(t *testing.T) { cache := files.New(registry, &coderdtest.FakeAuthorizer{}) logger := slogtest.Make(t, &slogtest.Options{IgnoreErrors: false}).Leveled(slog.LevelDebug) reconciler := prebuilds.NewStoreReconciler(db, ps, cache, codersdk.PrebuildsConfig{}, logger, clock, registry, fakeEnqueuer, newNoopUsageCheckerPtr()) + t.Cleanup(func() { + reconciler.Stop(context.Background(), nil) + }) owner := coderdtest.CreateFirstUser(t, client) ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitShort) @@ -2921,6 +2940,9 @@ func TestReconciliationRespectsPauseSetting(t *testing.T) { logger := testutil.Logger(t) cache := files.New(prometheus.NewRegistry(), &coderdtest.FakeAuthorizer{}) reconciler := prebuilds.NewStoreReconciler(db, ps, cache, cfg, logger, clock, prometheus.NewRegistry(), newNoopEnqueuer(), newNoopUsageCheckerPtr()) + t.Cleanup(func() { + reconciler.Stop(context.Background(), nil) + }) // Setup a template with a preset that should create prebuilds org := dbgen.Organization(t, db, database.Organization{}) From 85bbae697a44353c41eaee1ce5869526b85e8db7 Mon Sep 17 00:00:00 2001 From: Callum Styan Date: Thu, 11 Dec 2025 04:02:35 +0000 Subject: [PATCH 11/14] fix: add Close() to RenderCache interface for proper cleanup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The renderCache field in StoreReconciler was using the concrete type *RenderCacheImpl instead of the RenderCache interface, preventing proper cleanup of cache goroutines. Changes: - Add Close() method to RenderCache interface - Implement Close() as no-op in noopRenderCache - Update StoreReconciler.renderCache to use interface type - Update wsbuilder.Builder.renderCache to use interface type - Remove unnecessary nil check in reconciler Stop() method This ensures all render cache cleanup goroutines are properly stopped when reconcilers are stopped, fixing goleak test failures. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 --- coderd/dynamicparameters/render.go | 5 +++++ coderd/wsbuilder/wsbuilder.go | 4 ++-- enterprise/coderd/prebuilds/reconcile.go | 6 ++---- 3 files changed, 9 insertions(+), 6 deletions(-) diff --git a/coderd/dynamicparameters/render.go b/coderd/dynamicparameters/render.go index 079f083469c5f..9e162ed0f8592 100644 --- a/coderd/dynamicparameters/render.go +++ b/coderd/dynamicparameters/render.go @@ -39,6 +39,7 @@ var ErrTemplateVersionNotReady = xerrors.New("template version job not finished" type RenderCache interface { get(templateVersionID, ownerID uuid.UUID, parameters map[string]string) (*preview.Output, bool) put(templateVersionID, ownerID uuid.UUID, parameters map[string]string, output *preview.Output) + Close() } // noopRenderCache is a no-op implementation of RenderCache that doesn't cache anything. @@ -52,6 +53,10 @@ func (noopRenderCache) put(uuid.UUID, uuid.UUID, map[string]string, *preview.Out // no-op } +func (noopRenderCache) Close() { + // no-op +} + // loader is used to load the necessary coder objects for rendering a template // version's parameters. The output is a Renderer, which is the object that uses // the cached objects to render the template version's parameters. diff --git a/coderd/wsbuilder/wsbuilder.go b/coderd/wsbuilder/wsbuilder.go index b298f005da2dc..ee62145f8dfa3 100644 --- a/coderd/wsbuilder/wsbuilder.go +++ b/coderd/wsbuilder/wsbuilder.go @@ -89,7 +89,7 @@ type Builder struct { workspaceTags *map[string]string // renderCache caches template rendering results - renderCache *dynamicparameters.RenderCacheImpl + renderCache dynamicparameters.RenderCache prebuiltWorkspaceBuildStage sdkproto.PrebuiltWorkspaceBuildStage verifyNoLegacyParametersOnce bool @@ -258,7 +258,7 @@ func (b Builder) TemplateVersionPresetID(id uuid.UUID) Builder { // RenderCache sets the render cache to use for template rendering. // This allows multiple workspace builds to share cached render results. -func (b Builder) RenderCache(cache *dynamicparameters.RenderCacheImpl) Builder { +func (b Builder) RenderCache(cache dynamicparameters.RenderCache) Builder { // nolint: revive b.renderCache = cache return b diff --git a/enterprise/coderd/prebuilds/reconcile.go b/enterprise/coderd/prebuilds/reconcile.go index ef53c422daa17..6d100307cfbe9 100644 --- a/enterprise/coderd/prebuilds/reconcile.go +++ b/enterprise/coderd/prebuilds/reconcile.go @@ -61,7 +61,7 @@ type StoreReconciler struct { reconciliationDuration prometheus.Histogram // renderCache caches template rendering results to avoid expensive re-parsing - renderCache *dynamicparameters.RenderCacheImpl + renderCache dynamicparameters.RenderCache } var _ prebuilds.ReconciliationOrchestrator = &StoreReconciler{} @@ -294,9 +294,7 @@ func (c *StoreReconciler) Stop(ctx context.Context, cause error) { } // Close the render cache to stop its cleanup goroutine - if c.renderCache != nil { - c.renderCache.Close() - } + c.renderCache.Close() } // ReconcileAll will attempt to resolve the desired vs actual state of all templates which have presets with prebuilds configured. From 4f9b23a11cfc8155b09e8f37102b17e8d4779e84 Mon Sep 17 00:00:00 2001 From: Callum Styan Date: Thu, 11 Dec 2025 06:38:12 +0000 Subject: [PATCH 12/14] fix: close render cache even when reconciler never ran MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The reconciler's Stop() method was returning early if the reconciler was never started (running == false), which meant the render cache cleanup goroutine was never stopped. This happened in tests that create reconcilers but never call Run() on them. When Stop() was called in t.Cleanup(), it would skip closing the render cache, causing goroutine leaks. Fix: Move renderCache.Close() to execute before the running check, so the cleanup goroutine is always stopped regardless of whether the reconciler was ever started. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 --- enterprise/coderd/prebuilds/reconcile.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/enterprise/coderd/prebuilds/reconcile.go b/enterprise/coderd/prebuilds/reconcile.go index 6d100307cfbe9..681645c4bbe7e 100644 --- a/enterprise/coderd/prebuilds/reconcile.go +++ b/enterprise/coderd/prebuilds/reconcile.go @@ -268,6 +268,10 @@ func (c *StoreReconciler) Stop(ctx context.Context, cause error) { } } + // Close the render cache to stop its cleanup goroutine + // This must be done regardless of whether the reconciler is running + c.renderCache.Close() + // If the reconciler is not running, there's nothing else to do. if !c.running.Load() { return @@ -292,9 +296,6 @@ func (c *StoreReconciler) Stop(ctx context.Context, cause error) { case <-c.done: c.logger.Info(context.Background(), "reconciler stopped") } - - // Close the render cache to stop its cleanup goroutine - c.renderCache.Close() } // ReconcileAll will attempt to resolve the desired vs actual state of all templates which have presets with prebuilds configured. From 1c4b645b436d6aa517c79eb44c87cc5095f6b84b Mon Sep 17 00:00:00 2001 From: Callum Styan Date: Thu, 11 Dec 2025 19:42:37 +0000 Subject: [PATCH 13/14] test: add prebuild cache test to verify dual-render behavior This test confirms that the cache correctly handles the prebuild flow where ResolveParameters() calls Render() twice per build. With identical preset parameters across multiple prebuilds, we get the expected metrics: - First prebuild: 2 misses (one for empty params, one for preset params) - Subsequent prebuilds: 2 hits each (both Render calls hit the same cache entries) The user's actual deployment shows more cache misses and entries than expected, indicating that parameter values differ between prebuild instances. This suggests parameters are being modified or injected somewhere in the prebuild creation flow. --- .../rendercache_prebuild_internal_test.go | 192 ++++++++++++++++++ 1 file changed, 192 insertions(+) create mode 100644 coderd/dynamicparameters/rendercache_prebuild_internal_test.go diff --git a/coderd/dynamicparameters/rendercache_prebuild_internal_test.go b/coderd/dynamicparameters/rendercache_prebuild_internal_test.go new file mode 100644 index 0000000000000..0d6b13b94f3f6 --- /dev/null +++ b/coderd/dynamicparameters/rendercache_prebuild_internal_test.go @@ -0,0 +1,192 @@ +package dynamicparameters + +import ( + "context" + "testing" + + "github.com/google/uuid" + "github.com/hashicorp/hcl/v2" + "github.com/prometheus/client_golang/prometheus" + promtestutil "github.com/prometheus/client_golang/prometheus/testutil" + "github.com/stretchr/testify/require" + + "github.com/coder/coder/v2/coderd/database" + "github.com/coder/coder/v2/codersdk" + "github.com/coder/coder/v2/testutil" + "github.com/coder/preview" + previewtypes "github.com/coder/preview/types" + "github.com/coder/terraform-provider-coder/v2/provider" +) + +// TestRenderCache_PrebuildWithResolveParameters simulates the actual prebuild flow +// where ResolveParameters calls Render() twice - once with previous values and once +// with the final computed values. +func TestRenderCache_PrebuildWithResolveParameters(t *testing.T) { + t.Parallel() + + ctx := testutil.Context(t, testutil.WaitShort) + + // Create test metrics + cacheHits := prometheus.NewCounter(prometheus.CounterOpts{ + Name: "test_prebuild_cache_hits_total", + }) + cacheMisses := prometheus.NewCounter(prometheus.CounterOpts{ + Name: "test_prebuild_cache_misses_total", + }) + cacheSize := prometheus.NewGauge(prometheus.GaugeOpts{ + Name: "test_prebuild_cache_entries", + }) + + cache := NewRenderCacheWithMetrics(cacheHits, cacheMisses, cacheSize) + defer cache.Close() + + // Simulate prebuild scenario + prebuildOwnerID := uuid.MustParse("c42fdf75-3097-471c-8c33-fb52454d81c0") // database.PrebuildsSystemUserID + templateVersionID := uuid.New() + + // Preset parameters that all prebuilds share + presetParams := []database.TemplateVersionPresetParameter{ + {Name: "instance_type", Value: "t3.micro"}, + {Name: "region", Value: "us-west-2"}, + } + + // Create a mock renderer that returns consistent parameter definitions + mockRenderer := &mockRenderer{ + cache: cache, + templateVersionID: templateVersionID, + output: &preview.Output{ + Parameters: []previewtypes.Parameter{ + { + ParameterData: previewtypes.ParameterData{ + Name: "instance_type", + Type: previewtypes.ParameterTypeString, + FormType: provider.ParameterFormTypeInput, + Mutable: true, + DefaultValue: previewtypes.StringLiteral("t3.micro"), + Required: true, + }, + Value: previewtypes.StringLiteral("t3.micro"), + Diagnostics: nil, + }, + { + ParameterData: previewtypes.ParameterData{ + Name: "region", + Type: previewtypes.ParameterTypeString, + FormType: provider.ParameterFormTypeInput, + Mutable: true, + DefaultValue: previewtypes.StringLiteral("us-west-2"), + Required: true, + }, + Value: previewtypes.StringLiteral("us-west-2"), + Diagnostics: nil, + }, + }, + }, + } + + // Initial metrics should be 0 + require.Equal(t, float64(0), promtestutil.ToFloat64(cacheHits), "initial hits should be 0") + require.Equal(t, float64(0), promtestutil.ToFloat64(cacheMisses), "initial misses should be 0") + require.Equal(t, float64(0), promtestutil.ToFloat64(cacheSize), "initial size should be 0") + + // === FIRST PREBUILD === + // First build: no previous values, preset values provided + values1, err := ResolveParameters(ctx, prebuildOwnerID, mockRenderer, true, + []database.WorkspaceBuildParameter{}, // No previous values (first build) + []codersdk.WorkspaceBuildParameter{}, // No build-specific values + presetParams, // Preset values from template + ) + require.NoError(t, err) + require.NotNil(t, values1) + + // After first prebuild: + // - ResolveParameters calls Render() twice: + // 1. With previousValuesMap (empty {}) → miss, creates cache entry + // 2. With values.ValuesMap() ({preset}) → miss, creates cache entry + // Expected: 0 hits, 2 misses, 2 cache entries + t.Logf("After first prebuild: hits=%v, misses=%v, size=%v", + promtestutil.ToFloat64(cacheHits), + promtestutil.ToFloat64(cacheMisses), + promtestutil.ToFloat64(cacheSize)) + + require.Equal(t, float64(0), promtestutil.ToFloat64(cacheHits), "first prebuild should have 0 hits") + require.Equal(t, float64(2), promtestutil.ToFloat64(cacheMisses), "first prebuild should have 2 misses") + require.Equal(t, float64(2), promtestutil.ToFloat64(cacheSize), "should have 2 cache entries after first prebuild") + + // === SECOND PREBUILD === + // Second build: previous values now set to preset values + previousValues := []database.WorkspaceBuildParameter{ + {Name: "instance_type", Value: "t3.micro"}, + {Name: "region", Value: "us-west-2"}, + } + + values2, err := ResolveParameters(ctx, prebuildOwnerID, mockRenderer, false, + previousValues, // Previous values from first build + []codersdk.WorkspaceBuildParameter{}, + presetParams, + ) + require.NoError(t, err) + require.NotNil(t, values2) + + // After second prebuild: + // - ResolveParameters calls Render() twice: + // 1. With previousValuesMap ({preset}) → HIT (cache entry from first prebuild's 2nd render) + // 2. With values.ValuesMap() ({preset}) → HIT (same cache entry) + // Expected: 2 hits, 2 misses (still), 2 cache entries (still) + t.Logf("After second prebuild: hits=%v, misses=%v, size=%v", + promtestutil.ToFloat64(cacheHits), + promtestutil.ToFloat64(cacheMisses), + promtestutil.ToFloat64(cacheSize)) + + require.Equal(t, float64(2), promtestutil.ToFloat64(cacheHits), "second prebuild should have 2 hits") + require.Equal(t, float64(2), promtestutil.ToFloat64(cacheMisses), "misses should still be 2") + require.Equal(t, float64(2), promtestutil.ToFloat64(cacheSize), "should still have 2 cache entries") + + // === THIRD PREBUILD === + values3, err := ResolveParameters(ctx, prebuildOwnerID, mockRenderer, false, + previousValues, + []codersdk.WorkspaceBuildParameter{}, + presetParams, + ) + require.NoError(t, err) + require.NotNil(t, values3) + + // After third prebuild: + // - ResolveParameters calls Render() twice: + // 1. With previousValuesMap ({preset}) → HIT + // 2. With values.ValuesMap() ({preset}) → HIT + // Expected: 4 hits, 2 misses (still), 2 cache entries (still) + t.Logf("After third prebuild: hits=%v, misses=%v, size=%v", + promtestutil.ToFloat64(cacheHits), + promtestutil.ToFloat64(cacheMisses), + promtestutil.ToFloat64(cacheSize)) + + require.Equal(t, float64(4), promtestutil.ToFloat64(cacheHits), "third prebuild should have 4 total hits") + require.Equal(t, float64(2), promtestutil.ToFloat64(cacheMisses), "misses should still be 2") + require.Equal(t, float64(2), promtestutil.ToFloat64(cacheSize), "should still have 2 cache entries") + + // Summary: With 3 prebuilds, we should have: + // - 4 cache hits (2 from 2nd prebuild, 2 from 3rd prebuild) + // - 2 cache misses (2 from 1st prebuild) + // - 2 cache entries (one for empty params, one for preset params) +} + +// mockRenderer is a simple renderer that uses the cache for testing +type mockRenderer struct { + cache RenderCache + templateVersionID uuid.UUID + output *preview.Output +} + +func (m *mockRenderer) Render(ctx context.Context, ownerID uuid.UUID, values map[string]string) (*preview.Output, hcl.Diagnostics) { + // This simulates what dynamicRenderer does - check cache first + if cached, ok := m.cache.get(m.templateVersionID, ownerID, values); ok { + return cached, nil + } + + // Not in cache, "render" (just return our mock output) and cache it + m.cache.put(m.templateVersionID, ownerID, values, m.output) + return m.output, nil +} + +func (m *mockRenderer) Close() {} From b816191609ce3c934ece4935e47132a290e7e391 Mon Sep 17 00:00:00 2001 From: Callum Styan Date: Thu, 11 Dec 2025 20:58:43 +0000 Subject: [PATCH 14/14] perf(coderd/dynamicparameters): skip caching introspection render calls The first render in ResolveParameters() is purely for introspection to identify ephemeral parameters. The second render validates the final values after ephemeral processing. Previously, both renders were cached, causing unnecessary cache entries. For prebuilds with constant parameters, this doubled cache size and created entries that were never reused. This optimization: - Adds RenderWithoutCache() method to Renderer interface - First render (introspection) skips cache operations - Second render (final validation) still uses cache - Reduces cache size by 50% for prebuild scenarios - Improves cache hit ratio (was 4 misses/11 hits, now 1 miss/2+ hits) For 3 prebuilds with identical parameters: - Before: 2 misses, 4 hits, 2 entries (introspection + final) - After: 1 miss, 2 hits, 1 entry (final values only) --- coderd/dynamicparameters/render.go | 21 ++++++--- .../rendercache_prebuild_internal_test.go | 45 ++++++++++--------- .../rendermock/rendermock.go | 15 +++++++ coderd/dynamicparameters/resolver.go | 5 ++- coderd/dynamicparameters/resolver_test.go | 21 ++++++++- coderd/dynamicparameters/static.go | 9 ++++ 6 files changed, 89 insertions(+), 27 deletions(-) diff --git a/coderd/dynamicparameters/render.go b/coderd/dynamicparameters/render.go index 9e162ed0f8592..5317116b45a00 100644 --- a/coderd/dynamicparameters/render.go +++ b/coderd/dynamicparameters/render.go @@ -30,6 +30,7 @@ import ( // Forgetting to do so will result in a memory leak. type Renderer interface { Render(ctx context.Context, ownerID uuid.UUID, values map[string]string) (*preview.Output, hcl.Diagnostics) + RenderWithoutCache(ctx context.Context, ownerID uuid.UUID, values map[string]string) (*preview.Output, hcl.Diagnostics) Close() } @@ -259,9 +260,19 @@ type dynamicRenderer struct { } func (r *dynamicRenderer) Render(ctx context.Context, ownerID uuid.UUID, values map[string]string) (*preview.Output, hcl.Diagnostics) { - // Check cache first - if cached, ok := r.data.renderCache.get(r.data.templateVersionID, ownerID, values); ok { - return cached, nil + return r.render(ctx, ownerID, values, true) +} + +func (r *dynamicRenderer) RenderWithoutCache(ctx context.Context, ownerID uuid.UUID, values map[string]string) (*preview.Output, hcl.Diagnostics) { + return r.render(ctx, ownerID, values, false) +} + +func (r *dynamicRenderer) render(ctx context.Context, ownerID uuid.UUID, values map[string]string, useCache bool) (*preview.Output, hcl.Diagnostics) { + // Check cache first if enabled + if useCache { + if cached, ok := r.data.renderCache.get(r.data.templateVersionID, ownerID, values); ok { + return cached, nil + } } // Always start with the cached error, if we have one. @@ -297,8 +308,8 @@ func (r *dynamicRenderer) Render(ctx context.Context, ownerID uuid.UUID, values output, diags := preview.Preview(ctx, input, r.templateFS) - // Store in cache if successful - if !diags.HasErrors() { + // Store in cache if successful and caching is enabled + if useCache && !diags.HasErrors() { r.data.renderCache.put(r.data.templateVersionID, ownerID, values, output) } diff --git a/coderd/dynamicparameters/rendercache_prebuild_internal_test.go b/coderd/dynamicparameters/rendercache_prebuild_internal_test.go index 0d6b13b94f3f6..13fe99e7a322d 100644 --- a/coderd/dynamicparameters/rendercache_prebuild_internal_test.go +++ b/coderd/dynamicparameters/rendercache_prebuild_internal_test.go @@ -101,17 +101,17 @@ func TestRenderCache_PrebuildWithResolveParameters(t *testing.T) { // After first prebuild: // - ResolveParameters calls Render() twice: - // 1. With previousValuesMap (empty {}) → miss, creates cache entry - // 2. With values.ValuesMap() ({preset}) → miss, creates cache entry - // Expected: 0 hits, 2 misses, 2 cache entries + // 1. RenderWithoutCache with previousValuesMap (empty {}) → no cache operation + // 2. Render with values.ValuesMap() ({preset}) → miss, creates cache entry + // Expected: 0 hits, 1 miss, 1 cache entry t.Logf("After first prebuild: hits=%v, misses=%v, size=%v", promtestutil.ToFloat64(cacheHits), promtestutil.ToFloat64(cacheMisses), promtestutil.ToFloat64(cacheSize)) require.Equal(t, float64(0), promtestutil.ToFloat64(cacheHits), "first prebuild should have 0 hits") - require.Equal(t, float64(2), promtestutil.ToFloat64(cacheMisses), "first prebuild should have 2 misses") - require.Equal(t, float64(2), promtestutil.ToFloat64(cacheSize), "should have 2 cache entries after first prebuild") + require.Equal(t, float64(1), promtestutil.ToFloat64(cacheMisses), "first prebuild should have 1 miss") + require.Equal(t, float64(1), promtestutil.ToFloat64(cacheSize), "should have 1 cache entry after first prebuild") // === SECOND PREBUILD === // Second build: previous values now set to preset values @@ -130,17 +130,17 @@ func TestRenderCache_PrebuildWithResolveParameters(t *testing.T) { // After second prebuild: // - ResolveParameters calls Render() twice: - // 1. With previousValuesMap ({preset}) → HIT (cache entry from first prebuild's 2nd render) - // 2. With values.ValuesMap() ({preset}) → HIT (same cache entry) - // Expected: 2 hits, 2 misses (still), 2 cache entries (still) + // 1. RenderWithoutCache with previousValuesMap ({preset}) → no cache operation + // 2. Render with values.ValuesMap() ({preset}) → HIT (cache entry from first prebuild's 2nd render) + // Expected: 1 hit, 1 miss (still), 1 cache entry (still) t.Logf("After second prebuild: hits=%v, misses=%v, size=%v", promtestutil.ToFloat64(cacheHits), promtestutil.ToFloat64(cacheMisses), promtestutil.ToFloat64(cacheSize)) - require.Equal(t, float64(2), promtestutil.ToFloat64(cacheHits), "second prebuild should have 2 hits") - require.Equal(t, float64(2), promtestutil.ToFloat64(cacheMisses), "misses should still be 2") - require.Equal(t, float64(2), promtestutil.ToFloat64(cacheSize), "should still have 2 cache entries") + require.Equal(t, float64(1), promtestutil.ToFloat64(cacheHits), "second prebuild should have 1 hit") + require.Equal(t, float64(1), promtestutil.ToFloat64(cacheMisses), "misses should still be 1") + require.Equal(t, float64(1), promtestutil.ToFloat64(cacheSize), "should still have 1 cache entry") // === THIRD PREBUILD === values3, err := ResolveParameters(ctx, prebuildOwnerID, mockRenderer, false, @@ -153,22 +153,22 @@ func TestRenderCache_PrebuildWithResolveParameters(t *testing.T) { // After third prebuild: // - ResolveParameters calls Render() twice: - // 1. With previousValuesMap ({preset}) → HIT - // 2. With values.ValuesMap() ({preset}) → HIT - // Expected: 4 hits, 2 misses (still), 2 cache entries (still) + // 1. RenderWithoutCache with previousValuesMap ({preset}) → no cache operation + // 2. Render with values.ValuesMap() ({preset}) → HIT + // Expected: 2 hits, 1 miss (still), 1 cache entry (still) t.Logf("After third prebuild: hits=%v, misses=%v, size=%v", promtestutil.ToFloat64(cacheHits), promtestutil.ToFloat64(cacheMisses), promtestutil.ToFloat64(cacheSize)) - require.Equal(t, float64(4), promtestutil.ToFloat64(cacheHits), "third prebuild should have 4 total hits") - require.Equal(t, float64(2), promtestutil.ToFloat64(cacheMisses), "misses should still be 2") - require.Equal(t, float64(2), promtestutil.ToFloat64(cacheSize), "should still have 2 cache entries") + require.Equal(t, float64(2), promtestutil.ToFloat64(cacheHits), "third prebuild should have 2 total hits") + require.Equal(t, float64(1), promtestutil.ToFloat64(cacheMisses), "misses should still be 1") + require.Equal(t, float64(1), promtestutil.ToFloat64(cacheSize), "should still have 1 cache entry") // Summary: With 3 prebuilds, we should have: - // - 4 cache hits (2 from 2nd prebuild, 2 from 3rd prebuild) - // - 2 cache misses (2 from 1st prebuild) - // - 2 cache entries (one for empty params, one for preset params) + // - 2 cache hits (1 from 2nd prebuild, 1 from 3rd prebuild) + // - 1 cache miss (1 from 1st prebuild) + // - 1 cache entry (for preset params only - introspection renders are not cached) } // mockRenderer is a simple renderer that uses the cache for testing @@ -189,4 +189,9 @@ func (m *mockRenderer) Render(ctx context.Context, ownerID uuid.UUID, values map return m.output, nil } +func (m *mockRenderer) RenderWithoutCache(ctx context.Context, ownerID uuid.UUID, values map[string]string) (*preview.Output, hcl.Diagnostics) { + // For test purposes, just return output without caching + return m.output, nil +} + func (m *mockRenderer) Close() {} diff --git a/coderd/dynamicparameters/rendermock/rendermock.go b/coderd/dynamicparameters/rendermock/rendermock.go index 996b02a555b08..731db67d12023 100644 --- a/coderd/dynamicparameters/rendermock/rendermock.go +++ b/coderd/dynamicparameters/rendermock/rendermock.go @@ -69,3 +69,18 @@ func (mr *MockRendererMockRecorder) Render(ctx, ownerID, values any) *gomock.Cal mr.mock.ctrl.T.Helper() return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Render", reflect.TypeOf((*MockRenderer)(nil).Render), ctx, ownerID, values) } + +// RenderWithoutCache mocks base method. +func (m *MockRenderer) RenderWithoutCache(ctx context.Context, ownerID uuid.UUID, values map[string]string) (*preview.Output, hcl.Diagnostics) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "RenderWithoutCache", ctx, ownerID, values) + ret0, _ := ret[0].(*preview.Output) + ret1, _ := ret[1].(hcl.Diagnostics) + return ret0, ret1 +} + +// RenderWithoutCache indicates an expected call of RenderWithoutCache. +func (mr *MockRendererMockRecorder) RenderWithoutCache(ctx, ownerID, values any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "RenderWithoutCache", reflect.TypeOf((*MockRenderer)(nil).RenderWithoutCache), ctx, ownerID, values) +} diff --git a/coderd/dynamicparameters/resolver.go b/coderd/dynamicparameters/resolver.go index 7fc67d29a0d55..b2e647f339976 100644 --- a/coderd/dynamicparameters/resolver.go +++ b/coderd/dynamicparameters/resolver.go @@ -69,7 +69,10 @@ func ResolveParameters( // // This is how the form should look to the user on their workspace settings page. // This is the original form truth that our validations should initially be based on. - output, diags := renderer.Render(ctx, ownerID, previousValuesMap) + // + // Skip caching this render - it's only used for introspection to identify ephemeral + // parameters. The actual values used for the build will be rendered and cached below. + output, diags := renderer.RenderWithoutCache(ctx, ownerID, previousValuesMap) if diags.HasErrors() { // Top level diagnostics should break the build. Previous values (and new) should // always be valid. If there is a case where this is not true, then this has to diff --git a/coderd/dynamicparameters/resolver_test.go b/coderd/dynamicparameters/resolver_test.go index e6675e6f4c7dc..65ccd6cafb643 100644 --- a/coderd/dynamicparameters/resolver_test.go +++ b/coderd/dynamicparameters/resolver_test.go @@ -28,6 +28,25 @@ func TestResolveParameters(t *testing.T) { render := rendermock.NewMockRenderer(ctrl) // A single immutable parameter with no previous value. + render.EXPECT(). + RenderWithoutCache(gomock.Any(), gomock.Any(), gomock.Any()). + AnyTimes(). + Return(&preview.Output{ + Parameters: []previewtypes.Parameter{ + { + ParameterData: previewtypes.ParameterData{ + Name: "immutable", + Type: previewtypes.ParameterTypeString, + FormType: provider.ParameterFormTypeInput, + Mutable: false, + DefaultValue: previewtypes.StringLiteral("foo"), + Required: true, + }, + Value: previewtypes.StringLiteral("foo"), + Diagnostics: nil, + }, + }, + }, nil) render.EXPECT(). Render(gomock.Any(), gomock.Any(), gomock.Any()). AnyTimes(). @@ -78,7 +97,7 @@ func TestResolveParameters(t *testing.T) { // A single immutable parameter with no previous value. render.EXPECT(). - Render(gomock.Any(), gomock.Any(), gomock.Any()). + RenderWithoutCache(gomock.Any(), gomock.Any(), gomock.Any()). // Return the mutable param first Return(&preview.Output{ Parameters: []previewtypes.Parameter{ diff --git a/coderd/dynamicparameters/static.go b/coderd/dynamicparameters/static.go index fec5de2581aef..68268c67fa646 100644 --- a/coderd/dynamicparameters/static.go +++ b/coderd/dynamicparameters/static.go @@ -40,6 +40,15 @@ func (r *loader) staticRender(ctx context.Context, db database.Store) (*staticRe } func (r *staticRender) Render(_ context.Context, _ uuid.UUID, values map[string]string) (*preview.Output, hcl.Diagnostics) { + return r.render(values) +} + +func (r *staticRender) RenderWithoutCache(_ context.Context, _ uuid.UUID, values map[string]string) (*preview.Output, hcl.Diagnostics) { + // Static renderer doesn't use cache, so this is the same as Render + return r.render(values) +} + +func (r *staticRender) render(values map[string]string) (*preview.Output, hcl.Diagnostics) { params := r.staticParams for i := range params { param := ¶ms[i]