diff --git a/server/internal/cli/errors_test.go b/server/internal/cli/errors_test.go index ed68527a07..980c335705 100644 --- a/server/internal/cli/errors_test.go +++ b/server/internal/cli/errors_test.go @@ -211,6 +211,29 @@ func TestExtractServerMessagePrefersProseOverMachineCode(t *testing.T) { } } +// TestFormatErrorValidationPrefersProseOverMachineCode pins an intentional +// behavior change that rides along with the conflict fix: the prose preference +// lives in the shared extractor, so a 400/422 whose "error" holds a machine code +// now shows its "message" instead of the code. Only the issue-table endpoints +// are shaped this way and none of them is reachable from the CLI today, but the +// change is deliberate and should fail loudly if someone reverts it by accident. +func TestFormatErrorValidationPrefersProseOverMachineCode(t *testing.T) { + withLang(t, "en_US.UTF-8") + got := FormatError(&HTTPError{ + StatusCode: 422, + Body: `{"error":"unsupported_group","code":"group_kind_unsupported","message":"This group type is not supported."}`, + }, false) + if !strings.Contains(got, "This group type is not supported.") { + t.Errorf("expected the prose message, got %q", got) + } + + // A validation body carrying only a code is unchanged. + only := FormatError(&HTTPError{StatusCode: 422, Body: `{"error":"title_is_required"}`}, false) + if !strings.Contains(only, "title_is_required") { + t.Errorf("code-only validation body should still surface the code, got %q", only) + } +} + func TestFormatErrorDebugIncludesRawChain(t *testing.T) { withLang(t, "en_US.UTF-8") httpErr := &HTTPError{Method: "GET", Path: "/api/issues/abc", StatusCode: 404, Body: `{"error":"not found"}`} diff --git a/server/internal/handler/runtime_update.go b/server/internal/handler/runtime_update.go index 493b699854..2a30b11f38 100644 --- a/server/internal/handler/runtime_update.go +++ b/server/internal/handler/runtime_update.go @@ -3,6 +3,7 @@ package handler import ( "context" "encoding/json" + "errors" "log/slog" "net/http" "sync" @@ -249,7 +250,19 @@ func (h *Handler) InitiateUpdate(w http.ResponseWriter, r *http.Request) { uuidToString(member.UserID), ) if err != nil { - writeError(w, http.StatusConflict, err.Error()) + // Only the in-progress rejection is a conflict the caller can act on. + // Every other Create failure is infrastructure — the Redis store wraps + // connection failures as "reserve active update: ..." / "persist update + // request: ..." — and echoing it back would both leak internals and + // label an outage as a user-fixable conflict. That was survivable while + // the CLI hid 409 bodies; it no longer is, now that they are shown by + // default. + if errors.Is(err, errUpdateInProgress) { + writeError(w, http.StatusConflict, err.Error()) + return + } + slog.Error("UpdateStore Create failed", "error", err, "runtime_id", uuidToString(rt.ID)) + writeError(w, http.StatusInternalServerError, "failed to start the update") return } diff --git a/server/internal/handler/runtime_update_error_classification_test.go b/server/internal/handler/runtime_update_error_classification_test.go new file mode 100644 index 0000000000..6b5548eee3 --- /dev/null +++ b/server/internal/handler/runtime_update_error_classification_test.go @@ -0,0 +1,83 @@ +package handler + +import ( + "context" + "errors" + "net/http" + "net/http/httptest" + "strings" + "testing" +) + +// failingUpdateStore returns a chosen error from Create. The embedded interface +// supplies the rest of the method set; InitiateUpdate only reaches Create, so +// anything else calling through would panic loudly rather than pass silently. +type failingUpdateStore struct { + UpdateStore + createErr error +} + +func (s *failingUpdateStore) Create(context.Context, string, string, string) (*UpdateRequest, error) { + return nil, s.createErr +} + +// TestInitiateUpdate_InfrastructureErrorIsNotAConflict pins the classification +// split this PR adds alongside GH #6264. InitiateUpdate used to answer every +// UpdateStore.Create failure with a 409 carrying err.Error(). That was survivable +// while the CLI hid conflict bodies; now that they print by default, a Redis +// outage would show its dial address to the user and read as a conflict they +// could fix by retrying. +func TestInitiateUpdate_InfrastructureErrorIsNotAConflict(t *testing.T) { + if testHandler == nil || testPool == nil { + t.Skip("database not available") + } + + // Shape mirrors RedisUpdateStore.Create wrapping a dial failure. + infraErr := errors.New("reserve active update: dial tcp 10.1.2.3:6379: connect: connection refused") + + original := testHandler.UpdateStore + testHandler.UpdateStore = &failingUpdateStore{createErr: infraErr} + t.Cleanup(func() { testHandler.UpdateStore = original }) + + w := httptest.NewRecorder() + r := newRequest("POST", "/api/runtimes/"+testRuntimeID+"/update", map[string]any{"target_version": "v1.2.3"}) + r = withURLParams(r, "runtimeId", testRuntimeID) + + testHandler.InitiateUpdate(w, r) + + if w.Code != http.StatusInternalServerError { + t.Fatalf("infrastructure failure: expected 500, got %d: %s", w.Code, w.Body.String()) + } + body := w.Body.String() + for _, leak := range []string{"10.1.2.3:6379", "connection refused", "reserve active update"} { + if strings.Contains(body, leak) { + t.Fatalf("response leaked internal detail %q: %s", leak, body) + } + } +} + +// TestInitiateUpdate_InProgressStillConflicts is the positive half: the one +// Create failure a caller can actually act on keeps its 409 and its actionable +// wording, which is exactly what the CLI now surfaces by default. +func TestInitiateUpdate_InProgressStillConflicts(t *testing.T) { + if testHandler == nil || testPool == nil { + t.Skip("database not available") + } + + original := testHandler.UpdateStore + testHandler.UpdateStore = &failingUpdateStore{createErr: errUpdateInProgress} + t.Cleanup(func() { testHandler.UpdateStore = original }) + + w := httptest.NewRecorder() + r := newRequest("POST", "/api/runtimes/"+testRuntimeID+"/update", map[string]any{"target_version": "v1.2.3"}) + r = withURLParams(r, "runtimeId", testRuntimeID) + + testHandler.InitiateUpdate(w, r) + + if w.Code != http.StatusConflict { + t.Fatalf("update-in-progress: expected 409, got %d: %s", w.Code, w.Body.String()) + } + if !strings.Contains(w.Body.String(), "already in progress") { + t.Fatalf("409 should keep its actionable message, got %s", w.Body.String()) + } +}