diff --git a/internal/server/bug3479_remove_unusable_truth_test.go b/internal/server/bug3479_remove_unusable_truth_test.go new file mode 100644 index 000000000..74245d3b3 --- /dev/null +++ b/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") + } +} diff --git a/internal/server/handlers_cloud.go b/internal/server/handlers_cloud.go index cf3d20de4..7bea2b80f 100644 --- a/internal/server/handlers_cloud.go +++ b/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. diff --git a/internal/server/handlers_workspaces.go b/internal/server/handlers_workspaces.go index ca797960d..414b038d5 100644 --- a/internal/server/handlers_workspaces.go +++ b/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 } diff --git a/internal/server/workspace_owner_compensation.go b/internal/server/workspace_owner_compensation.go index f33b04841..48248fea5 100644 --- a/internal/server/workspace_owner_compensation.go +++ b/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.