From 3721247990eddb911c124a4db6cc978ba9deb13e Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 20 Jun 2025 10:23:46 -0500 Subject: [PATCH 1/4] chore: add cacheCloser to cleanup all opened files --- coderd/dynamicparameters/render.go | 34 ++++++++++--------- coderd/files/cache.go | 4 +++ coderd/files/closer.go | 54 ++++++++++++++++++++++++++++++ 3 files changed, 76 insertions(+), 16 deletions(-) create mode 100644 coderd/files/closer.go diff --git a/coderd/dynamicparameters/render.go b/coderd/dynamicparameters/render.go index 9c4c73f87e5bc..b6a77c4704225 100644 --- a/coderd/dynamicparameters/render.go +++ b/coderd/dynamicparameters/render.go @@ -131,46 +131,48 @@ func (r *loader) Renderer(ctx context.Context, db database.Store, cache *files.C return r.staticRender(ctx, db) } - return r.dynamicRenderer(ctx, db, cache) + return r.dynamicRenderer(ctx, db, files.NewCacheCloser(cache)) } // Renderer caches all the necessary files when rendering a template version's // parameters. It must be closed after use to release the cached files. -func (r *loader) dynamicRenderer(ctx context.Context, db database.Store, cache *files.Cache) (*dynamicRenderer, error) { +func (r *loader) dynamicRenderer(ctx context.Context, db database.Store, cache *files.CacheCloser) (*dynamicRenderer, error) { + closeFiles := true // If the function returns with no error, this will toggle to false. + defer func() { + if closeFiles { + cache.Close() + } + }() + // If they can read the template version, then they can read the file for // parameter loading purposes. //nolint:gocritic fileCtx := dbauthz.AsFileReader(ctx) - templateFS, err := cache.Acquire(fileCtx, r.job.FileID) + + var templateFS fs.FS + var err error + + templateFS, err = cache.Acquire(fileCtx, r.job.FileID) if err != nil { return nil, xerrors.Errorf("acquire template file: %w", err) } - var terraformFS fs.FS = templateFS var moduleFilesFS *files.CloseFS if r.terraformValues.CachedModuleFiles.Valid { moduleFilesFS, err = cache.Acquire(fileCtx, r.terraformValues.CachedModuleFiles.UUID) if err != nil { - templateFS.Close() return nil, xerrors.Errorf("acquire module files: %w", err) } - terraformFS = files.NewOverlayFS(templateFS, []files.Overlay{{Path: ".terraform/modules", FS: moduleFilesFS}}) + templateFS = files.NewOverlayFS(templateFS, []files.Overlay{{Path: ".terraform/modules", FS: moduleFilesFS}}) } + closeFiles = false // Caller will have to call close return &dynamicRenderer{ data: r, - templateFS: terraformFS, + templateFS: templateFS, db: db, ownerErrors: make(map[uuid.UUID]error), - close: func() { - // Up to 2 files are cached, and must be released when rendering is complete. - // TODO: Might be smart to always call release when the context is - // canceled. - templateFS.Close() - if moduleFilesFS != nil { - moduleFilesFS.Close() - } - }, + close: cache.Close, }, nil } diff --git a/coderd/files/cache.go b/coderd/files/cache.go index 6e4dc9383b6f1..170abb10b1ff7 100644 --- a/coderd/files/cache.go +++ b/coderd/files/cache.go @@ -19,6 +19,10 @@ import ( "github.com/coder/coder/v2/coderd/util/lazy" ) +type FileAcquirer interface { + Acquire(ctx context.Context, fileID uuid.UUID) (*CloseFS, error) +} + // NewFromStore returns a file cache that will fetch files from the provided // database. func NewFromStore(store database.Store, registerer prometheus.Registerer, authz rbac.Authorizer) *Cache { diff --git a/coderd/files/closer.go b/coderd/files/closer.go new file mode 100644 index 0000000000000..cb6050d8a1bd4 --- /dev/null +++ b/coderd/files/closer.go @@ -0,0 +1,54 @@ +package files + +import ( + "context" + "sync" + + "github.com/google/uuid" + "golang.org/x/xerrors" +) + +// CacheCloser is a cache wrapper used to close all acquired files. +// This is a more simple interface to use if opening multiple files at once. +type CacheCloser struct { + cache FileAcquirer + + close []*CloseFS + mu sync.Mutex + closed bool +} + +func NewCacheCloser(cache FileAcquirer) *CacheCloser { + return &CacheCloser{ + cache: cache, + close: make([]*CloseFS, 0), + } +} + +func (c *CacheCloser) Close() { + c.mu.Lock() + defer c.mu.Unlock() + + for _, fs := range c.close { + fs.Close() + } + c.closed = true +} + +func (c *CacheCloser) Acquire(ctx context.Context, fileID uuid.UUID) (*CloseFS, error) { + c.mu.Lock() + defer c.mu.Unlock() + + if c.closed { + return nil, xerrors.New("cache is closed, and cannot acquire new files") + } + + f, err := c.cache.Acquire(ctx, fileID) + if err != nil { + return nil, err + } + + c.close = append(c.close, f) + + return f, nil +} From be1247afd0520d2b211443617c6699a7941c5b0b Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 20 Jun 2025 10:26:28 -0500 Subject: [PATCH 2/4] remove some references --- coderd/files/closer.go | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/coderd/files/closer.go b/coderd/files/closer.go index cb6050d8a1bd4..fcccb42969ef9 100644 --- a/coderd/files/closer.go +++ b/coderd/files/closer.go @@ -13,15 +13,15 @@ import ( type CacheCloser struct { cache FileAcquirer - close []*CloseFS - mu sync.Mutex - closed bool + closers []func() + mu sync.Mutex + closed bool } func NewCacheCloser(cache FileAcquirer) *CacheCloser { return &CacheCloser{ - cache: cache, - close: make([]*CloseFS, 0), + cache: cache, + closers: make([]func(), 0), } } @@ -29,10 +29,13 @@ func (c *CacheCloser) Close() { c.mu.Lock() defer c.mu.Unlock() - for _, fs := range c.close { - fs.Close() + for _, doClose := range c.closers { + doClose() } c.closed = true + + // Remove any references + c.closers = make([]func(), 0) } func (c *CacheCloser) Acquire(ctx context.Context, fileID uuid.UUID) (*CloseFS, error) { @@ -48,7 +51,7 @@ func (c *CacheCloser) Acquire(ctx context.Context, fileID uuid.UUID) (*CloseFS, return nil, err } - c.close = append(c.close, f) + c.closers = append(c.closers, f.close) return f, nil } From 0315359f2a3ef77cf0fe0071468165c9e3f93a99 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 20 Jun 2025 10:56:53 -0500 Subject: [PATCH 3/4] set cache to nil to signal closed --- coderd/files/closer.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/coderd/files/closer.go b/coderd/files/closer.go index fcccb42969ef9..1a1297ad72d10 100644 --- a/coderd/files/closer.go +++ b/coderd/files/closer.go @@ -15,7 +15,6 @@ type CacheCloser struct { closers []func() mu sync.Mutex - closed bool } func NewCacheCloser(cache FileAcquirer) *CacheCloser { @@ -32,8 +31,9 @@ func (c *CacheCloser) Close() { for _, doClose := range c.closers { doClose() } - c.closed = true + // Prevent further acquisitions + c.cache = nil // Remove any references c.closers = make([]func(), 0) } @@ -42,7 +42,7 @@ func (c *CacheCloser) Acquire(ctx context.Context, fileID uuid.UUID) (*CloseFS, c.mu.Lock() defer c.mu.Unlock() - if c.closed { + if c.cache == nil { return nil, xerrors.New("cache is closed, and cannot acquire new files") } From 616f4e3419b234d520a5573a1f3128d963ca2cc4 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 20 Jun 2025 10:57:21 -0500 Subject: [PATCH 4/4] nil out the closers too --- coderd/files/closer.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/coderd/files/closer.go b/coderd/files/closer.go index 1a1297ad72d10..9bd98fdd60caf 100644 --- a/coderd/files/closer.go +++ b/coderd/files/closer.go @@ -35,7 +35,7 @@ func (c *CacheCloser) Close() { // Prevent further acquisitions c.cache = nil // Remove any references - c.closers = make([]func(), 0) + c.closers = nil } func (c *CacheCloser) Acquire(ctx context.Context, fileID uuid.UUID) (*CloseFS, error) {