From f1ad012024c4184913d732488d823990178db1b6 Mon Sep 17 00:00:00 2001 From: rabbitblood Date: Sun, 26 Apr 2026 06:40:15 -0700 Subject: [PATCH] refactor(handlers): apply simplify findings on PR #2094 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Extract walkTemplateConfigs(configsDir, fn) shared helper. Both templates.List and loadRuntimeProvisionTimeouts walked configsDir + parsed config.yaml — same boilerplate twice. Now centralised so a future template-discovery rule (subdir naming, README sentinel, etc.) lands in one place. - templates.List uses the walker — net -10 lines. - loadRuntimeProvisionTimeouts uses the walker — net -10 lines. - Document runtimeProvisionTimeoutsCache as 'NOT SAFE for package-level reuse' so a future change doesn't accidentally promote it to a singleton (sync.Once can't be reset → tests would lock out other fixtures). Skipped (review finding): atomic.Pointer[map[string]int] for future hot-reload. The doc comment already documents the limitation; YAGNI-promoting the primitive now would buy a not-yet-built feature at the cost of more code today. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../handlers/runtime_provision_timeouts.go | 73 ++++++++++++------- .../internal/handlers/templates.go | 30 ++------ 2 files changed, 53 insertions(+), 50 deletions(-) diff --git a/workspace-server/internal/handlers/runtime_provision_timeouts.go b/workspace-server/internal/handlers/runtime_provision_timeouts.go index d27b1975..9e922fb9 100644 --- a/workspace-server/internal/handlers/runtime_provision_timeouts.go +++ b/workspace-server/internal/handlers/runtime_provision_timeouts.go @@ -9,16 +9,21 @@ import ( "gopkg.in/yaml.v3" ) -// runtimeProvisionTimeouts caches the per-runtime provision-timeout values -// declared in template config.yaml manifests (#2054 phase 2). Lazy-init so -// the first workspace API request after process start pays the read cost -// (a few KB of yaml across ~50 templates) and every subsequent one is a -// map lookup. +// runtimeProvisionTimeoutsCache holds the per-runtime provision-timeout +// values declared in template config.yaml manifests (#2054 phase 2). +// Lazy-init so the first workspace API request after process start pays +// the read cost (a few KB of yaml across ~50 templates) and every +// subsequent one is a map lookup. // // Cache lifetime = process lifetime. Templates only change on container // rebuild + workspace-server restart, which already invalidates the // in-memory state. A future template-hot-reload feature would need to // refresh this cache; today there is no such hook. +// +// NOT SAFE for package-level reuse — sync.Once cannot be reset, so a +// shared singleton would lock tests out of trying multiple template +// fixtures. Tests construct fresh struct values; production code holds +// one per WorkspaceHandler instance. type runtimeProvisionTimeoutsCache struct { once sync.Once m map[string]int // runtime → seconds @@ -31,9 +36,38 @@ func (c *runtimeProvisionTimeoutsCache) get(configsDir string, runtime string) i return c.m[runtime] } -// loadRuntimeProvisionTimeouts walks `configsDir`, parses every immediate -// subdirectory's `config.yaml`, and returns a map of runtime → seconds -// for templates that declared `runtime_config.provision_timeout_seconds`. +// walkTemplateConfigs invokes fn(id, configBytes) for every immediate +// subdirectory of configsDir that has a readable config.yaml. Bad reads +// and empty dirs are silently skipped — the caller decides whether +// missing fields warrant the slot. A bad configsDir logs once and +// returns without invoking fn, matching the "start clean with no +// templates" semantics. +// +// Shared between the templates.List handler (which decodes the full +// templateSummary) and loadRuntimeProvisionTimeouts (which decodes the +// narrow runtime-timeout subset). Centralising the walk means a future +// template-discovery rule (subdir naming convention, README sentinel, +// etc.) lands in one place. +func walkTemplateConfigs(configsDir string, fn func(id string, configBytes []byte)) { + entries, err := os.ReadDir(configsDir) + if err != nil { + log.Printf("walkTemplateConfigs: read configsDir %s: %v", configsDir, err) + return + } + for _, e := range entries { + if !e.IsDir() { + continue + } + data, err := os.ReadFile(filepath.Join(configsDir, e.Name(), "config.yaml")) + if err != nil { + continue + } + fn(e.Name(), data) + } +} + +// loadRuntimeProvisionTimeouts returns a map of runtime → seconds for +// templates that declared `runtime_config.provision_timeout_seconds`. // // Templates without the field aren't represented (lookup returns zero, // which the caller treats as "fall through to canvas runtime profile"). @@ -46,22 +80,7 @@ func (c *runtimeProvisionTimeoutsCache) get(configsDir string, runtime string) i // safer default. func loadRuntimeProvisionTimeouts(configsDir string) map[string]int { out := map[string]int{} - entries, err := os.ReadDir(configsDir) - if err != nil { - // Logged but not fatal — workspace-server starts cleanly with - // no templates (dev / fresh-clone). The result is an empty map - // so every runtime falls through to canvas's profile default. - log.Printf("loadRuntimeProvisionTimeouts: read configsDir %s: %v", configsDir, err) - return out - } - for _, e := range entries { - if !e.IsDir() { - continue - } - data, err := os.ReadFile(filepath.Join(configsDir, e.Name(), "config.yaml")) - if err != nil { - continue - } + walkTemplateConfigs(configsDir, func(_ string, data []byte) { var raw struct { Runtime string `yaml:"runtime"` RuntimeConfig struct { @@ -69,15 +88,15 @@ func loadRuntimeProvisionTimeouts(configsDir string) map[string]int { } `yaml:"runtime_config"` } if err := yaml.Unmarshal(data, &raw); err != nil { - continue + return } secs := raw.RuntimeConfig.ProvisionTimeoutSeconds if secs <= 0 || raw.Runtime == "" { - continue + return } if existing, ok := out[raw.Runtime]; !ok || secs > existing { out[raw.Runtime] = secs } - } + }) return out } diff --git a/workspace-server/internal/handlers/templates.go b/workspace-server/internal/handlers/templates.go index 6c5f42f3..e33c06d6 100644 --- a/workspace-server/internal/handlers/templates.go +++ b/workspace-server/internal/handlers/templates.go @@ -87,24 +87,8 @@ func (h *TemplatesHandler) resolveTemplateDir(wsName string) string { // List handles GET /templates func (h *TemplatesHandler) List(c *gin.Context) { - entries, err := os.ReadDir(h.configsDir) - if err != nil { - c.JSON(http.StatusOK, []templateSummary{}) - return - } - templates := make([]templateSummary, 0) - for _, entry := range entries { - if !entry.IsDir() { - continue - } - - configPath := filepath.Join(h.configsDir, entry.Name(), "config.yaml") - data, err := os.ReadFile(configPath) - if err != nil { - continue - } - + walkTemplateConfigs(h.configsDir, func(id string, data []byte) { var raw struct { Name string `yaml:"name"` Description string `yaml:"description"` @@ -113,14 +97,14 @@ func (h *TemplatesHandler) List(c *gin.Context) { Model string `yaml:"model"` Skills []string `yaml:"skills"` RuntimeConfig struct { - Model string `yaml:"model"` - Models []modelSpec `yaml:"models"` - RequiredEnv []string `yaml:"required_env"` + Model string `yaml:"model"` + Models []modelSpec `yaml:"models"` + RequiredEnv []string `yaml:"required_env"` ProvisionTimeoutSeconds int `yaml:"provision_timeout_seconds"` } `yaml:"runtime_config"` } if err := yaml.Unmarshal(data, &raw); err != nil { - continue + return } // Model comes from either top-level (legacy) or runtime_config.model (current). @@ -130,7 +114,7 @@ func (h *TemplatesHandler) List(c *gin.Context) { } templates = append(templates, templateSummary{ - ID: entry.Name(), + ID: id, Name: raw.Name, Description: raw.Description, Tier: raw.Tier, @@ -142,7 +126,7 @@ func (h *TemplatesHandler) List(c *gin.Context) { SkillCount: len(raw.Skills), ProvisionTimeoutSeconds: raw.RuntimeConfig.ProvisionTimeoutSeconds, }) - } + }) c.JSON(http.StatusOK, templates) }