diff --git a/web/handlers_settings.go b/web/handlers_settings.go index 02576edc2c07696053b36d3fc1eccbb71e9f2334..44eb2d3436def60b444c4f5f71c20496be66dffb 100644 --- a/web/handlers_settings.go +++ b/web/handlers_settings.go @@ -2,12 +2,15 @@ package web import ( "errors" + "log/slog" "net/http" "net/url" "strconv" "strings" "github.com/go-chi/chi/v5" + + "go.bigb.es/auxilia/scribe" "sourcecraft.dev/bigbes/sr-ht-ecore/chrome" "sourcecraft.dev/bigbes/sr-ht-ecore/pages" @@ -198,12 +201,22 @@ http.Error(w, "failed to delete database", http.StatusInternalServerError) return } if err := a.cfg.Stores.DeleteStore(r.Context(), a.cfg.ReposRoot, repo.Path); err != nil { - http.Error(w, "database record removed but store deletion failed: "+err.Error(), + // The store-layer error names the on-disk path, which nothing else on + // this surface discloses. The reader keeps the fact that matters to + // them — the record is gone but the store may still be on disk — and + // the detail goes to the log against the database id. + slog.Error("deleting a database's on-disk store failed after the record was removed", + "component", "web", "database", repo.ID, scribe.Err(err)) + http.Error(w, "The database record was removed, but the on-disk store could not be deleted. Contact support.", http.StatusInternalServerError) return } if err := a.cfg.Stores.Evict(repo.Path); err != nil { - http.Error(w, "store deleted but cache eviction failed: "+err.Error(), + // Same split: the store is gone, but the cached handle may survive it + // — a different fact from the one above, kept distinct on purpose. + slog.Error("evicting a database's cached store handle failed after the store was deleted", + "component", "web", "database", repo.ID, scribe.Err(err)) + http.Error(w, "The on-disk store was deleted, but the cached handle could not be evicted. Contact support.", http.StatusInternalServerError) return } diff --git a/web/web_test.go b/web/web_test.go index b2dca419d64ca4a09f2992009a74cada9888b37c..31aace8ead67460affec018f4fc4fcce0e2ddffb 100644 --- a/web/web_test.go +++ b/web/web_test.go @@ -247,6 +247,8 @@ } type fakeStoreManager struct { initErr error + deleteErr error + evictErr error initCalls []string deleteCalls []string evictCalls []string @@ -258,11 +260,11 @@ return m.initErr } func (m *fakeStoreManager) DeleteStore(_ context.Context, _, absPath string) error { m.deleteCalls = append(m.deleteCalls, absPath) - return nil + return m.deleteErr } func (m *fakeStoreManager) Evict(diskPath string) error { m.evictCalls = append(m.evictCalls, diskPath) - return nil + return m.evictErr } type fakeSession struct { @@ -849,6 +851,58 @@ } if len(h.stores.deleteCalls) != 1 || len(h.stores.evictCalls) != 1 { t.Fatalf("store delete/evict not called: del=%v evict=%v", h.stores.deleteCalls, h.stores.evictCalls) } +} + +// storeDetail is a store-layer failure of the shape DeleteStore/Evict really +// produce: the on-disk path underneath the store. +const storeDetail = "unlink /var/lib/dolt/~owner/db/.dolt/noms/oldgen: permission denied" + +// The settings delete path used to render the store layer's own error text +// into the response, under "database record removed but store deletion +// failed: " and "store deleted but cache eviction failed: " — both carrying +// the store's path on disk, which nothing else on this surface discloses. +// +// The two failures are still told apart: one means the row is gone but the +// store may still be on disk, the other that the store is gone but a cached +// handle may survive it. The path just no longer rides along. +func TestSettingsDeleteDoesNotPrintTheStoreError(t *testing.T) { + newDeleteHarness := func(t *testing.T) (*harness, *auth.AuthContext) { + t.Helper() + h := newHarness(t) + h.store.add(&core.Repo{Name: "db", OwnerID: 10, OwnerName: "owner", + Path: "/var/lib/dolt/~owner/db", Visibility: core.VisibilityPublic}) + return h, testCaller(10, "owner") + } + assertHidden := func(t *testing.T, body string) { + t.Helper() + assert.NotContains(t, body, storeDetail) + assert.NotContains(t, body, "/var/lib/dolt", "the store's path must not reach the reader") + assert.NotContains(t, body, "permission denied", "the underlying OS error must not reach the reader") + } + + t.Run("store deletion fails", func(t *testing.T) { + h, owner := newDeleteHarness(t) + h.stores.deleteErr = errors.New(storeDetail) + + rec := h.do("POST", "/~owner/db/settings", owner, url.Values{"action": {"delete"}, "confirm_name": {"db"}}) + require.Equal(t, http.StatusInternalServerError, rec.Code) + body := rec.Body.String() + assertHidden(t, body) + assert.Contains(t, body, "The database record was removed, but the on-disk store could not be deleted.") + // Eviction must not be attempted once the store deletion itself failed. + assert.Empty(t, h.stores.evictCalls) + }) + + t.Run("cache eviction fails", func(t *testing.T) { + h, owner := newDeleteHarness(t) + h.stores.evictErr = errors.New(storeDetail) + + rec := h.do("POST", "/~owner/db/settings", owner, url.Values{"action": {"delete"}, "confirm_name": {"db"}}) + require.Equal(t, http.StatusInternalServerError, rec.Code) + body := rec.Body.String() + assertHidden(t, body) + assert.Contains(t, body, "The on-disk store was deleted, but the cached handle could not be evicted.") + }) } func TestSettingsACLAddRemove(t *testing.T) {