fix(server): removeUnusableWorkspace reports a failed delete as kept (BUG-3479) (#1870)

Its contract is nil when the workspace is gone, an error when it was kept,
but the branch where its own soft-delete FAILED returned nil. Every caller
then reported a removal that had not happened while a live, ownerless
workspace held its slug.

- That branch now returns an error naming the failed removal.
- Cloud auto-create (both arms) logs that the workspace could NOT be removed
  instead of leaving 'is being removed' as the last word.
- Create workspace's seed failure says in its 500 that the half-made
  workspace survived and holds its name.
- addOwnerOrCompensate's ABSENT arm now returns the KEPT error, which its
  callers already treat as a failure; the bundle keep path reads the row back
  (BUG-2709) and the interrupted-import rollback does too (BUG-3475).

A test per caller with a SQLite trigger refusing the soft-delete; each fails
on the old nil.

Claude-Session: https://claude.ai/code/session_01Q9WssCjFuHjSfLABoW4tmS

internal/server/bug3479_remove_unusable_truth_test.go

@@ -0,0 +1,150 @@
+package server
+
+import (
+	"database/sql"
+	"net/http"
+	"strings"
+	"testing"
+
+	"github.com/PerpetualSoftware/pad/internal/models"
+	"github.com/PerpetualSoftware/pad/internal/store"
+)
+
+// BUG-3479: removeUnusableWorkspace returned nil when its own soft-delete
+// FAILED, and its contract says nil means "the workspace is gone". Every
+// caller then reported a removal that had not happened, while a live,
+// ownerless workspace kept its slug. A trigger refuses the soft-delete so
+// each caller can be driven into exactly that state (SQLite).
+
+func refuseSoftDelete(t *testing.T, srv *Server) {
+	t.Helper()
+	if _, err := srv.store.DB().Exec(`CREATE TRIGGER t3479_no_soft_delete BEFORE UPDATE OF deleted_at ON workspaces
+		BEGIN SELECT RAISE(ABORT, 'soft-delete refused (BUG-3479 test)'); END`); err != nil {
+		t.Fatalf("create trigger: %v", err)
+	}
+}
+
+func liveBySlug(t *testing.T, srv *Server, slug string) *models.Workspace {
+	t.Helper()
+	ws, err := srv.store.GetWorkspaceBySlug(slug)
+	if err != nil {
+		t.Fatalf("GetWorkspaceBySlug(%q): %v", slug, err)
+	}
+	return ws
+}
+
+// The helper itself: a failed delete is an ERROR, and the workspace is still
+// there; the control (no trigger) is nil and gone.
+func TestRemoveUnusableWorkspace_BUG3479_FailedDeleteIsAnError(t *testing.T) {
+	srv := attachmentsServerOn(t, store.DriverSQLite)
+
+	ctl, err := srv.store.CreateWorkspace(models.WorkspaceCreate{Name: "Control 3479"})
+	if err != nil {
+		t.Fatal(err)
+	}
+	if err := srv.removeUnusableWorkspace("test", ctl.ID, ctl.Slug, "", errSimSeedFailure); err != nil {
+		t.Fatalf("control: a removal that succeeded returned %v", err)
+	}
+	if liveBySlug(t, srv, ctl.Slug) != nil {
+		t.Fatal("control: the workspace is still live after a successful removal")
+	}
+
+	ws, err := srv.store.CreateWorkspace(models.WorkspaceCreate{Name: "Stuck 3479"})
+	if err != nil {
+		t.Fatal(err)
+	}
+	refuseSoftDelete(t, srv)
+	err = srv.removeUnusableWorkspace("test", ws.ID, ws.Slug, "", errSimSeedFailure)
+	if err == nil {
+		t.Fatal("a removal whose delete failed returned nil, which callers read as 'gone'")
+	}
+	if !strings.Contains(err.Error(), "could not be removed") {
+		t.Errorf("the error should say the workspace could not be removed: %v", err)
+	}
+	if liveBySlug(t, srv, ws.Slug) == nil {
+		t.Fatal("precondition: the trigger should have kept the workspace live")
+	}
+}
+
+// addOwnerOrCompensate's ABSENT arm: the owner row is genuinely absent, the
+// removal's delete fails, and the error must be the KEPT one, not the
+// "removed" one its callers read as gone.
+func TestAddOwnerOrCompensate_BUG3479_FailedRemovalIsReportedAsKept(t *testing.T) {
+	srv := ackLossCloudServer(t, store.DriverSQLite)
+	user := realUser(t, srv, "absent3479@example.com")
+	ws, err := srv.store.CreateWorkspace(models.WorkspaceCreate{Name: "Absent 3479"})
+	if err != nil {
+		t.Fatal(err)
+	}
+	restore := srv.store.SetAddWorkspaceMemberCommitHookForTesting(func(tx *sql.Tx) error {
+		_ = tx.Rollback()
+		return errSimMemberAckLoss
+	})
+	t.Cleanup(restore)
+	refuseSoftDelete(t, srv)
+
+	err = srv.addOwnerOrCompensate("test", ws.ID, ws.Slug, user.ID)
+	if err == nil || !strings.Contains(err.Error(), "could not be removed") {
+		t.Fatalf("want the kept error naming the failed removal, got %v", err)
+	}
+	if liveBySlug(t, srv, ws.Slug) == nil {
+		t.Fatal("precondition: the workspace should still be live")
+	}
+}
+
+// Cloud auto-create, both arms: the log must not end on "being removed" when
+// the workspace survived.
+func TestAutoCreateWorkspace_BUG3479_FailedRemovalIsLogged(t *testing.T) {
+	t.Run("seed failure", func(t *testing.T) {
+		srv := ackLossCloudServer(t, store.DriverSQLite)
+		user := realUser(t, srv, "seed3479@example.com")
+		logs := captureLogs(t)
+		restore := srv.store.SetSeedCollectionsFailureHookForTesting(func(string, string) error { return errSimSeedFailure })
+		t.Cleanup(restore)
+		refuseSoftDelete(t, srv)
+
+		srv.autoCreateWorkspace(user)
+
+		if !strings.Contains(logs.String(), "could NOT be removed and is still live") {
+			t.Errorf("a failed removal was not logged as one: %s", logs.String())
+		}
+	})
+	t.Run("owner never written", func(t *testing.T) {
+		srv := ackLossCloudServer(t, store.DriverSQLite)
+		user := realUser(t, srv, "absent3479b@example.com")
+		logs := captureLogs(t)
+		restore := srv.store.SetAddWorkspaceMemberCommitHookForTesting(func(tx *sql.Tx) error {
+			_ = tx.Rollback()
+			return errSimMemberAckLoss
+		})
+		t.Cleanup(restore)
+		refuseSoftDelete(t, srv)
+
+		srv.autoCreateWorkspace(user)
+
+		if !strings.Contains(logs.String(), "could NOT be removed and is still live") {
+			t.Errorf("a failed removal was not logged as one: %s", logs.String())
+		}
+	})
+}
+
+// Create workspace, seed failure: the 500 says the half-made workspace
+// survived and holds its name, instead of reading as "nothing was created".
+func TestCreateWorkspace_BUG3479_SeedFailureThatCannotBeRemovedSaysSo(t *testing.T) {
+	srv := attachmentsServerOn(t, store.DriverSQLite)
+	_, tok := memberImporter(t, srv)
+	restore := srv.store.SetSeedCollectionsFailureHookForTesting(func(string, string) error { return errSimSeedFailure })
+	t.Cleanup(restore)
+	refuseSoftDelete(t, srv)
+
+	rr := doRequestWithCookie(srv, "POST", "/api/v1/workspaces", map[string]any{"name": "Seedless", "template": "startup"}, tok)
+	if rr.Code != http.StatusInternalServerError {
+		t.Fatalf("want 500, got %d: %s", rr.Code, rr.Body.String())
+	}
+	if !strings.Contains(rr.Body.String(), "could not be removed and still holds its name") {
+		t.Errorf("the 500 must say the workspace survived: %s", rr.Body.String())
+	}
+	if liveBySlug(t, srv, "seedless") == nil {
+		t.Fatal("precondition: the trigger should have kept the workspace live")
+	}
+}

internal/server/handlers_cloud.go

@@ -1751,7 +1751,11 @@ func (s *Server) autoCreateWorkspace(user *models.User) {
 	if err := s.store.SeedCollectionsFromTemplate(ws.ID, "startup"); err != nil {
 		slog.Error("auto-create workspace: failed to seed collections; the workspace is unusable and is being removed",
 			"workspace_id", ws.ID, "workspace_slug", ws.Slug, "user_id", user.ID, "error", err)
-		_ = s.removeUnusableWorkspace("cloud auto-create", ws.ID, ws.Slug, user.ID, err)
+		if rerr := s.removeUnusableWorkspace("cloud auto-create", ws.ID, ws.Slug, user.ID, err); rerr != nil {
+			// BUG-3479: say it, rather than leave "is being removed" as the last word.
+			slog.Error("auto-create workspace: the unusable workspace could NOT be removed and is still live",
+				"workspace_id", ws.ID, "workspace_slug", ws.Slug, "user_id", user.ID, "error", rerr)
+		}
 		return
 	}
 
@@ -1816,7 +1820,10 @@ func (s *Server) autoCreateWorkspace(user *models.User) {
 				// other members, which this arm never checked.
 				slog.Error("auto-create workspace: add owner member failed after retry; the workspace is unusable and is being removed",
 					"workspace_id", ws.ID, "workspace_slug", ws.Slug, "user_id", user.ID, "error", err)
-				_ = s.removeUnusableWorkspace("cloud auto-create", ws.ID, ws.Slug, user.ID, err)
+				if rerr := s.removeUnusableWorkspace("cloud auto-create", ws.ID, ws.Slug, user.ID, err); rerr != nil {
+					slog.Error("auto-create workspace: the unusable workspace could NOT be removed and is still live",
+						"workspace_id", ws.ID, "workspace_slug", ws.Slug, "user_id", user.ID, "error", rerr)
+				}
 				return
 			// The literal matches the AddWorkspaceMember call above and every other
 			// role comparison in this package; there is no shared constant.

internal/server/handlers_workspaces.go

@@ -482,11 +482,15 @@ func (s *Server) handleCreateWorkspace(w http.ResponseWriter, r *http.Request) {
 		// could use or remove. Compensating only the owner step would have left
 		// the invariant true of one failure point and false of its neighbour.
 		userID := currentUserID(r)
+		msg := "Failed to seed collections: " + err.Error()
 		if rerr := s.removeUnusableWorkspace("create workspace (seed)", ws.ID, ws.Slug, userID, err); rerr != nil {
 			slog.Error("workspace seeding failed and the workspace could not be removed",
 				"workspace_id", ws.ID, "workspace_slug", ws.Slug, "error", rerr)
+			// BUG-3479: the caller is told the half-made workspace survived,
+			// since it holds the name and a retry will land on name-2.
+			msg += fmt.Sprintf("; the unusable workspace %q could not be removed and still holds its name (see the server log)", ws.Slug)
 		}
-		writeError(w, http.StatusInternalServerError, "internal_error", "Failed to seed collections: "+err.Error())
+		writeError(w, http.StatusInternalServerError, "internal_error", msg)
 		return
 	}
 

internal/server/workspace_owner_compensation.go

@@ -202,7 +202,10 @@ func (s *Server) removeUnusableWorkspace(door, workspaceID, workspaceSlug, userI
 	if delErr := s.store.DeleteWorkspace(workspaceSlug); delErr != nil {
 		slog.Error(door+": failed to soft-delete the workspace; manual intervention required",
 			"workspace_id", workspaceID, "workspace_slug", workspaceSlug, "user_id", userID, "error", delErr)
-		return nil
+		// An ERROR, per this function's contract (BUG-3479): the workspace is
+		// still live and still holds its slug. This returned nil, so every
+		// caller reported a removal that had not happened.
+		return fmt.Errorf("%w (the workspace could not be removed: %v)", cause, delErr)
 	}
 	// TASK-3365: nobody should hold a live connection to a workspace minted
 	// moments ago, but if anyone does it re-checks now.