From 071400b6c8957e71e74a0247621163bdd31bb1b6 Mon Sep 17 00:00:00 2001 From: Andrew Nesbitt Date: Wed, 1 Apr 2026 22:12:09 +0100 Subject: [PATCH 1/2] Fix startup message and add connectivity check for S3 storage When S3 storage is configured, the startup log incorrectly showed the default local path (./cache/artifacts) instead of the actual S3 URL. This also adds a lightweight connectivity check at startup so bad credentials or endpoints fail immediately rather than on first request. Add URL() and Close() to the Storage interface so all backends report their URL and can be cleaned up properly. Rename the stats JSON field from storage_path to storage_url. Close storage in error paths and during graceful shutdown. Fixes #49 --- README.md | 2 +- docs/swagger/docs.go | 2 +- docs/swagger/swagger.json | 2 +- internal/handler/handler_test.go | 4 +++ internal/server/server.go | 22 ++++++++++++-- internal/server/server_test.go | 52 ++++++++++++++++++++++++++++++++ internal/storage/filesystem.go | 8 +++++ internal/storage/storage.go | 6 ++++ 8 files changed, 92 insertions(+), 6 deletions(-) diff --git a/README.md b/README.md index 0590838..93ec8ba 100644 --- a/README.md +++ b/README.md @@ -688,7 +688,7 @@ Response: "cached_artifacts": 142, "total_size_bytes": 523456789, "total_size": "499.2 MB", - "storage_path": "./cache/artifacts", + "storage_url": "file:///path/to/cache/artifacts", "database_path": "./cache/proxy.db" } ``` diff --git a/docs/swagger/docs.go b/docs/swagger/docs.go index 2343f32..76d835d 100644 --- a/docs/swagger/docs.go +++ b/docs/swagger/docs.go @@ -939,7 +939,7 @@ const docTemplate = `{ "database_path": { "type": "string" }, - "storage_path": { + "storage_url": { "type": "string" }, "total_size": { diff --git a/docs/swagger/swagger.json b/docs/swagger/swagger.json index 0f2e364..8f0edb9 100644 --- a/docs/swagger/swagger.json +++ b/docs/swagger/swagger.json @@ -932,7 +932,7 @@ "database_path": { "type": "string" }, - "storage_path": { + "storage_url": { "type": "string" }, "total_size": { diff --git a/internal/handler/handler_test.go b/internal/handler/handler_test.go index 5c433d6..4c71319 100644 --- a/internal/handler/handler_test.go +++ b/internal/handler/handler_test.go @@ -79,6 +79,10 @@ func (s *mockStorage) UsedSpace(_ context.Context) (int64, error) { return total, nil } +func (s *mockStorage) URL() string { return "mem://" } + +func (s *mockStorage) Close() error { return nil } + // mockFetcher implements fetch.FetcherInterface for testing. type mockFetcher struct { artifact *fetch.Artifact diff --git a/internal/server/server.go b/internal/server/server.go index 8e6b588..bc3187b 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -112,9 +112,19 @@ func New(cfg *config.Config, logger *slog.Logger) (*Server, error) { return nil, fmt.Errorf("initializing storage: %w", err) } + // Verify storage is accessible (catches bad S3 credentials/endpoints early). + // Exists returns (false, nil) for a missing key, so only real connectivity + // or permission errors surface here. + if _, err := store.Exists(context.Background(), ".health-check"); err != nil { + _ = store.Close() + _ = db.Close() + return nil, fmt.Errorf("verifying storage connectivity: %w", err) + } + // Load templates templates, err := NewTemplates() if err != nil { + _ = store.Close() _ = db.Close() return nil, fmt.Errorf("loading templates: %w", err) } @@ -244,7 +254,7 @@ func (s *Server) Start() error { s.logger.Info("starting server", "listen", s.cfg.Listen, "base_url", s.cfg.BaseURL, - "storage", s.cfg.Storage.Path, //nolint:staticcheck // backwards compat + "storage", s.storage.URL(), "database", s.cfg.Database.Path) // Start background goroutine to update cache stats metrics @@ -287,6 +297,12 @@ func (s *Server) Shutdown(ctx context.Context) error { } } + if s.storage != nil { + if err := s.storage.Close(); err != nil { + errs = append(errs, fmt.Errorf("storage close: %w", err)) + } + } + if s.db != nil { if err := s.db.Close(); err != nil { errs = append(errs, fmt.Errorf("database close: %w", err)) @@ -707,7 +723,7 @@ type StatsResponse struct { CachedArtifacts int64 `json:"cached_artifacts"` TotalSize int64 `json:"total_size_bytes"` TotalSizeHuman string `json:"total_size"` - StoragePath string `json:"storage_path"` + StorageURL string `json:"storage_url"` DatabasePath string `json:"database_path"` } @@ -739,7 +755,7 @@ func (s *Server) handleStats(w http.ResponseWriter, r *http.Request) { CachedArtifacts: count, TotalSize: size, TotalSizeHuman: formatSize(size), - StoragePath: s.cfg.Storage.Path, //nolint:staticcheck // backwards compat + StorageURL: s.storage.URL(), DatabasePath: s.cfg.Database.Path, } diff --git a/internal/server/server_test.go b/internal/server/server_test.go index 69f36e8..f075019 100644 --- a/internal/server/server_test.go +++ b/internal/server/server_test.go @@ -223,6 +223,10 @@ func TestStatsEndpoint(t *testing.T) { if stats.CachedArtifacts != 0 { t.Errorf("expected 0 cached artifacts, got %d", stats.CachedArtifacts) } + + if !strings.HasPrefix(stats.StorageURL, "file://") { + t.Errorf("expected storage_url to start with file://, got %q", stats.StorageURL) + } } func TestDashboard(t *testing.T) { @@ -867,3 +871,51 @@ func TestHandlePackagesListPage(t *testing.T) { t.Error("expected packages list to contain seeded package") } } + +func TestNewServer_StorageConnectivityCheck(t *testing.T) { + tempDir := t.TempDir() + dbPath := filepath.Join(tempDir, "test.db") + storagePath := filepath.Join(tempDir, "artifacts") + + cfg := &config.Config{ + Listen: ":0", + BaseURL: "http://localhost:8080", + Storage: config.StorageConfig{URL: "file://" + storagePath}, + Database: config.DatabaseConfig{Path: dbPath}, + } + + logger := slog.New(slog.NewTextHandler(io.Discard, nil)) + + srv, err := New(cfg, logger) + if err != nil { + t.Fatalf("New() failed: %v", err) + } + + if srv.storage.URL() != "file://"+filepath.ToSlash(storagePath) { + t.Errorf("expected storage URL file://%s, got %s", storagePath, srv.storage.URL()) + } + + _ = srv.db.Close() +} + +func TestStatsEndpoint_StorageURL(t *testing.T) { + ts := newTestServer(t) + defer ts.close() + + req := httptest.NewRequest("GET", "/stats", nil) + w := httptest.NewRecorder() + ts.handler.ServeHTTP(w, req) + + if w.Code != http.StatusOK { + t.Fatalf("expected status 200, got %d", w.Code) + } + + // Verify the JSON response uses storage_url (not storage_path) + body := w.Body.String() + if !strings.Contains(body, `"storage_url"`) { + t.Errorf("expected JSON key storage_url in response, got: %s", body) + } + if strings.Contains(body, `"storage_path"`) { + t.Errorf("unexpected JSON key storage_path in response (should be storage_url)") + } +} diff --git a/internal/storage/filesystem.go b/internal/storage/filesystem.go index 8dec48b..cf6a1fe 100644 --- a/internal/storage/filesystem.go +++ b/internal/storage/filesystem.go @@ -172,3 +172,11 @@ func (fs *Filesystem) Root() string { func (fs *Filesystem) FullPath(path string) string { return fs.fullPath(path) } + +func (fs *Filesystem) URL() string { + return "file://" + filepath.ToSlash(fs.root) +} + +func (fs *Filesystem) Close() error { + return nil +} diff --git a/internal/storage/storage.go b/internal/storage/storage.go index 93053ca..8a9026c 100644 --- a/internal/storage/storage.go +++ b/internal/storage/storage.go @@ -47,6 +47,12 @@ type Storage interface { // UsedSpace returns the total bytes used by all stored content. UsedSpace(ctx context.Context) (int64, error) + + // URL returns the storage backend URL (e.g. "file:///path" or "s3://bucket"). + URL() string + + // Close releases any resources held by the storage backend. + Close() error } // ArtifactPath builds a storage path for an artifact. From 24702e2947beef9f32675a6600727b29a24e369b Mon Sep 17 00:00:00 2001 From: Andrew Nesbitt Date: Fri, 3 Apr 2026 13:11:17 +0100 Subject: [PATCH 2/2] Fix Windows test assertion for file:// URL format OpenBucket normalizes Windows paths to file:///C:/path (three slashes) but the test expected file://C:/path (two slashes). --- internal/server/server_test.go | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/internal/server/server_test.go b/internal/server/server_test.go index f075019..7e56f2c 100644 --- a/internal/server/server_test.go +++ b/internal/server/server_test.go @@ -891,8 +891,13 @@ func TestNewServer_StorageConnectivityCheck(t *testing.T) { t.Fatalf("New() failed: %v", err) } - if srv.storage.URL() != "file://"+filepath.ToSlash(storagePath) { - t.Errorf("expected storage URL file://%s, got %s", storagePath, srv.storage.URL()) + // On Windows, OpenBucket normalises to file:///C:/path; on Unix the + // absolute path already starts with /, so file:// + /path == file:///path. + wantPrefix := "file://" + wantSuffix := filepath.ToSlash(storagePath) + got := srv.storage.URL() + if !strings.HasPrefix(got, wantPrefix) || !strings.HasSuffix(got, wantSuffix) { + t.Errorf("expected storage URL ending with %s, got %s", wantSuffix, got) } _ = srv.db.Close()