From 55dfb22a7b8e480778d249d7c15c8642c4ace118 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 17 Jun 2025 10:21:33 -0500 Subject: [PATCH 1/7] chore: file cache Release tied 1:1 with an acquire Made idempotent --- coderd/files/cache.go | 39 ++++++++++++++++++++++++++++++++------ coderd/files/cache_test.go | 13 ++++++++----- coderd/parameters.go | 11 +++++++---- 3 files changed, 48 insertions(+), 15 deletions(-) diff --git a/coderd/files/cache.go b/coderd/files/cache.go index 484507d2ac5b0..9ac80b94e7e60 100644 --- a/coderd/files/cache.go +++ b/coderd/files/cache.go @@ -3,6 +3,7 @@ package files import ( "bytes" "context" + "io" "io/fs" "sync" @@ -140,20 +141,34 @@ type cacheEntry struct { type fetcher func(context.Context, uuid.UUID) (CacheEntryValue, error) +var _ fs.FS = (*CloseFS)(nil) +var _ io.Closer = (*CloseFS)(nil) + +// CloseFS is a wrapper around fs.FS that implements io.Closer. The Close() +// method tells the cache to release the fileID. Once all open references are +// closed, the file is removed from the cache. +type CloseFS struct { + fs.FS + + close func() error +} + +func (f *CloseFS) Close() error { return f.close() } + // Acquire will load the fs.FS for the given file. It guarantees that parallel // calls for the same fileID will only result in one fetch, and that parallel // calls for distinct fileIDs will fetch in parallel. // // Safety: Every call to Acquire that does not return an error must have a // matching call to Release. -func (c *Cache) Acquire(ctx context.Context, fileID uuid.UUID) (fs.FS, error) { +func (c *Cache) Acquire(ctx context.Context, fileID uuid.UUID) (*CloseFS, error) { // It's important that this `Load` call occurs outside of `prepare`, after the // mutex has been released, or we would continue to hold the lock until the // entire file has been fetched, which may be slow, and would prevent other // files from being fetched in parallel. it, err := c.prepare(ctx, fileID).Load() if err != nil { - c.Release(fileID) + c.release(fileID) return nil, err } @@ -163,11 +178,20 @@ func (c *Cache) Acquire(ctx context.Context, fileID uuid.UUID) (fs.FS, error) { } // Always check the caller can actually read the file. if err := c.authz.Authorize(ctx, subject, policy.ActionRead, it.Object); err != nil { - c.Release(fileID) + c.release(fileID) return nil, err } - return it.FS, err + var once sync.Once + return &CloseFS{ + FS: it.FS, + close: func() error { + // sync.Once makes the Close() idempotent, so we can call it + // multiple times without worrying about double-releasing. + once.Do(func() { c.release(fileID) }) + return nil + }, + }, nil } func (c *Cache) prepare(ctx context.Context, fileID uuid.UUID) *lazy.ValueWithError[CacheEntryValue] { @@ -203,9 +227,12 @@ func (c *Cache) prepare(ctx context.Context, fileID uuid.UUID) *lazy.ValueWithEr return entry.value } -// Release decrements the reference count for the given fileID, and frees the +// release decrements the reference count for the given fileID, and frees the // backing data if there are no further references being held. -func (c *Cache) Release(fileID uuid.UUID) { +// +// release should only be called after a successful call to Acquire using the Release() +// method on the returned *CloseFS. +func (c *Cache) release(fileID uuid.UUID) { c.lock.Lock() defer c.lock.Unlock() diff --git a/coderd/files/cache_test.go b/coderd/files/cache_test.go index 469520b4139fe..9b4912dff6314 100644 --- a/coderd/files/cache_test.go +++ b/coderd/files/cache_test.go @@ -75,7 +75,7 @@ func TestCacheRBAC(t *testing.T) { require.Equal(t, 0, cache.Count()) // Read the file with a file reader to put it into the cache. - _, err := cache.Acquire(cacheReader, file.ID) + a, err := cache.Acquire(cacheReader, file.ID) require.NoError(t, err) require.Equal(t, 1, cache.Count()) @@ -86,12 +86,12 @@ func TestCacheRBAC(t *testing.T) { require.Equal(t, 1, cache.Count()) // UserReader can - _, err = cache.Acquire(userReader, file.ID) + b, err := cache.Acquire(userReader, file.ID) require.NoError(t, err) require.Equal(t, 1, cache.Count()) - cache.Release(file.ID) - cache.Release(file.ID) + _ = a.Close() + _ = b.Close() require.Equal(t, 0, cache.Count()) rec.AssertActorID(t, nobodyID.String(), rec.Pair(policy.ActionRead, file)) @@ -179,6 +179,7 @@ func TestRelease(t *testing.T) { ids = append(ids, uuid.New()) } + releases := make(map[uuid.UUID][]func() error, 0) // Acquire a bunch of references batchSize := 10 for openedIdx, id := range ids { @@ -186,6 +187,7 @@ func TestRelease(t *testing.T) { it, err := c.Acquire(ctx, id) require.NoError(t, err) require.Equal(t, emptyFS, it) + releases[id] = append(releases[id], it.Close) // Each time a new file is opened, the metrics should be updated as so: opened := openedIdx + 1 @@ -206,7 +208,8 @@ func TestRelease(t *testing.T) { for closedIdx, id := range ids { stillOpen := len(ids) - closedIdx for closingIdx := range batchSize { - c.Release(id) + _ = releases[id][0]() + releases[id] = releases[id][1:] // Each time a file is released, the metrics should decrement the file refs require.Equal(t, (stillOpen*batchSize)-(closingIdx+1), promhelp.GaugeValue(t, reg, cachePromMetricName("open_file_refs_current"), nil)) diff --git a/coderd/parameters.go b/coderd/parameters.go index c88199956392d..dac6fe8baced7 100644 --- a/coderd/parameters.go +++ b/coderd/parameters.go @@ -4,6 +4,7 @@ import ( "context" "database/sql" "encoding/json" + "io/fs" "net/http" "time" @@ -144,7 +145,8 @@ func (api *API) handleDynamicParameters(listen bool, rw http.ResponseWriter, r * } // Add the file first. Calling `Release` if it fails is a no-op, so this is safe. - templateFS, err := api.FileCache.Acquire(fileCtx, fileID) + var templateFS fs.FS + closeableTemplateFS, err := api.FileCache.Acquire(fileCtx, fileID) if err != nil { httpapi.Write(ctx, rw, http.StatusNotFound, codersdk.Response{ Message: "Internal error fetching template version Terraform.", @@ -152,7 +154,8 @@ func (api *API) handleDynamicParameters(listen bool, rw http.ResponseWriter, r * }) return } - defer api.FileCache.Release(fileID) + defer closeableTemplateFS.Close() + templateFS = closeableTemplateFS // Having the Terraform plan available for the evaluation engine is helpful // for populating values from data blocks, but isn't strictly required. If @@ -171,9 +174,9 @@ func (api *API) handleDynamicParameters(listen bool, rw http.ResponseWriter, r * }) return } - defer api.FileCache.Release(tf.CachedModuleFiles.UUID) + defer moduleFilesFS.Close() - templateFS = files.NewOverlayFS(templateFS, []files.Overlay{{Path: ".terraform/modules", FS: moduleFilesFS}}) + templateFS = files.NewOverlayFS(closeableTemplateFS, []files.Overlay{{Path: ".terraform/modules", FS: moduleFilesFS}}) } owner, err := getWorkspaceOwnerData(ctx, api.Database, apikey.UserID, templateVersion.OrganizationID) From cfc36e98ae773aeb65dec8a8a4d3e0dacc5d3680 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 17 Jun 2025 10:27:46 -0500 Subject: [PATCH 2/7] fmt --- coderd/files/cache.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/coderd/files/cache.go b/coderd/files/cache.go index 9ac80b94e7e60..b72c5220557f2 100644 --- a/coderd/files/cache.go +++ b/coderd/files/cache.go @@ -141,8 +141,10 @@ type cacheEntry struct { type fetcher func(context.Context, uuid.UUID) (CacheEntryValue, error) -var _ fs.FS = (*CloseFS)(nil) -var _ io.Closer = (*CloseFS)(nil) +var ( + _ fs.FS = (*CloseFS)(nil) + _ io.Closer = (*CloseFS)(nil) +) // CloseFS is a wrapper around fs.FS that implements io.Closer. The Close() // method tells the cache to release the fileID. Once all open references are From 59a19f3fc987c1e28930fda7799ac2f85fddfae1 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 17 Jun 2025 16:27:40 -0500 Subject: [PATCH 3/7] fix test assert --- coderd/files/cache_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/coderd/files/cache_test.go b/coderd/files/cache_test.go index 9b4912dff6314..1d0ac8a13d348 100644 --- a/coderd/files/cache_test.go +++ b/coderd/files/cache_test.go @@ -186,7 +186,7 @@ func TestRelease(t *testing.T) { for batchIdx := range batchSize { it, err := c.Acquire(ctx, id) require.NoError(t, err) - require.Equal(t, emptyFS, it) + require.Equal(t, emptyFS, it.FS) releases[id] = append(releases[id], it.Close) // Each time a new file is opened, the metrics should be updated as so: From 9a3d811293259357d65033c76a185fd301a14a70 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 18 Jun 2025 12:59:57 -0500 Subject: [PATCH 4/7] add some comments --- coderd/parameters.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/coderd/parameters.go b/coderd/parameters.go index dac6fe8baced7..dacd8de812ab8 100644 --- a/coderd/parameters.go +++ b/coderd/parameters.go @@ -155,6 +155,8 @@ func (api *API) handleDynamicParameters(listen bool, rw http.ResponseWriter, r * return } defer closeableTemplateFS.Close() + // templateFS does not implement the Close method. For it to be later merged with + // the module files, we need to convert it to an OverlayFS. templateFS = closeableTemplateFS // Having the Terraform plan available for the evaluation engine is helpful From 93170203b2bda7e96f5b62029f9e3cd7905a6658 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 18 Jun 2025 13:01:49 -0500 Subject: [PATCH 5/7] close to not return an error --- coderd/files/cache.go | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/coderd/files/cache.go b/coderd/files/cache.go index b72c5220557f2..cbdca75ab8016 100644 --- a/coderd/files/cache.go +++ b/coderd/files/cache.go @@ -3,7 +3,6 @@ package files import ( "bytes" "context" - "io" "io/fs" "sync" @@ -142,8 +141,7 @@ type cacheEntry struct { type fetcher func(context.Context, uuid.UUID) (CacheEntryValue, error) var ( - _ fs.FS = (*CloseFS)(nil) - _ io.Closer = (*CloseFS)(nil) + _ fs.FS = (*CloseFS)(nil) ) // CloseFS is a wrapper around fs.FS that implements io.Closer. The Close() @@ -152,10 +150,10 @@ var ( type CloseFS struct { fs.FS - close func() error + close func() } -func (f *CloseFS) Close() error { return f.close() } +func (f *CloseFS) Close() { f.close() } // Acquire will load the fs.FS for the given file. It guarantees that parallel // calls for the same fileID will only result in one fetch, and that parallel @@ -187,11 +185,10 @@ func (c *Cache) Acquire(ctx context.Context, fileID uuid.UUID) (*CloseFS, error) var once sync.Once return &CloseFS{ FS: it.FS, - close: func() error { + close: func() { // sync.Once makes the Close() idempotent, so we can call it // multiple times without worrying about double-releasing. once.Do(func() { c.release(fileID) }) - return nil }, }, nil } From 9a21fee1f1b3ab92cc3d00763806c8d6016fe037 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 18 Jun 2025 13:27:28 -0500 Subject: [PATCH 6/7] fmt --- coderd/files/cache.go | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/coderd/files/cache.go b/coderd/files/cache.go index cbdca75ab8016..8127c3989129b 100644 --- a/coderd/files/cache.go +++ b/coderd/files/cache.go @@ -140,9 +140,7 @@ type cacheEntry struct { type fetcher func(context.Context, uuid.UUID) (CacheEntryValue, error) -var ( - _ fs.FS = (*CloseFS)(nil) -) +var _ fs.FS = (*CloseFS)(nil) // CloseFS is a wrapper around fs.FS that implements io.Closer. The Close() // method tells the cache to release the fileID. Once all open references are @@ -162,7 +160,7 @@ func (f *CloseFS) Close() { f.close() } // Safety: Every call to Acquire that does not return an error must have a // matching call to Release. func (c *Cache) Acquire(ctx context.Context, fileID uuid.UUID) (*CloseFS, error) { - // It's important that this `Load` call occurs outside of `prepare`, after the + // It's important that this `Load` call occurs outside `prepare`, after the // mutex has been released, or we would continue to hold the lock until the // entire file has been fetched, which may be slow, and would prevent other // files from being fetched in parallel. From e640600223b206b792d4143be926d00b2318d3d8 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 18 Jun 2025 13:28:08 -0500 Subject: [PATCH 7/7] fmt --- coderd/files/cache_test.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/coderd/files/cache_test.go b/coderd/files/cache_test.go index 1d0ac8a13d348..5efb4ba19be28 100644 --- a/coderd/files/cache_test.go +++ b/coderd/files/cache_test.go @@ -90,8 +90,8 @@ func TestCacheRBAC(t *testing.T) { require.NoError(t, err) require.Equal(t, 1, cache.Count()) - _ = a.Close() - _ = b.Close() + a.Close() + b.Close() require.Equal(t, 0, cache.Count()) rec.AssertActorID(t, nobodyID.String(), rec.Pair(policy.ActionRead, file)) @@ -179,7 +179,7 @@ func TestRelease(t *testing.T) { ids = append(ids, uuid.New()) } - releases := make(map[uuid.UUID][]func() error, 0) + releases := make(map[uuid.UUID][]func(), 0) // Acquire a bunch of references batchSize := 10 for openedIdx, id := range ids { @@ -208,7 +208,7 @@ func TestRelease(t *testing.T) { for closedIdx, id := range ids { stillOpen := len(ids) - closedIdx for closingIdx := range batchSize { - _ = releases[id][0]() + releases[id][0]() releases[id] = releases[id][1:] // Each time a file is released, the metrics should decrement the file refs