From 3f1c6c878a68491a68b01dd9175872385b1bd9b5 Mon Sep 17 00:00:00 2001 From: Lukas Wuttke Date: Wed, 8 Jul 2026 17:38:16 +0200 Subject: [PATCH] =?UTF-8?q?fix(client=20create):=20honest=20conflict=20mes?= =?UTF-8?q?sage=20=E2=80=94=20name=20the=20owner,=20handle=20cluster=5Fin?= =?UTF-8?q?=5Fuse?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The CLI mapped every provisioning 409 to a static "registered to a different tracebloc account — sign in to that account, or ask your admin" — which was often FALSE (the same-account phantom case) and a dead end. With the backend's fix #2 (backend#1021) the 409 body now distinguishes: • cluster_conflict — genuinely another account; body carries owner_email; • cluster_in_use — a same-account client is live on this cluster. New conflictMessage() parses the 409 body and picks the right guidance: • cross-account → "registered to another tracebloc account () — ask them to release it, or sign in as that account" (contact-the-owner, never "delete the cluster" — it isn't ours to wipe; names the owner when supplied); • cluster_in_use → "another tracebloc client () in your account is already live on this cluster — offboard it first with `tracebloc delete`, or provision on a separate machine". Degrades gracefully against a backend without fix #2 (empty/unparseable body → the generic cross-account text). The client-side not-owned refusal (no HTTP body) keeps the generic message. Reworded crossAccountConflictMsg to match. Companion to backend#1021 (fix #2) and cli#190 (fix #1) of the phantom-client migration. Tests: owner_email surfaced; cluster_in_use names the live client and does NOT read as cross-account; existing cross-account + client-side-refusal messages updated to the new wording. Full suite green; gofmt -s / errcheck / ineffassign / misspell clean. Co-Authored-By: Claude Opus 4.8 (1M context) --- internal/cli/client.go | 62 +++++++++++++++++++++++++++++++------ internal/cli/client_test.go | 50 ++++++++++++++++++++++++++++-- 2 files changed, 101 insertions(+), 11 deletions(-) diff --git a/internal/cli/client.go b/internal/cli/client.go index 4277bb0..c15836d 100644 --- a/internal/cli/client.go +++ b/internal/cli/client.go @@ -4,6 +4,7 @@ import ( "context" "crypto/rand" "encoding/hex" + "encoding/json" "errors" "fmt" "net/http" @@ -356,9 +357,10 @@ func runClientCreate(ctx context.Context, p *ui.Printer, pr prompter, opts clien case http.StatusForbidden: return askAnAdmin(ctx, p, client, "provision a client", "provisioning") case http.StatusConflict: - // Per RFC C.3 the only 409 on POST /edge-device/ is cluster_conflict - // (R6): this cluster_id is bound to another account. - return &exitError{code: 1, err: errors.New(crossAccountConflictMsg)} + // A 409 on POST /edge-device/ is a cross-account cluster_conflict + // (R6) or a same-account cluster_in_use; conflictMessage picks the + // right guidance (and names the owner when the backend supplies it). + return &exitError{code: 1, err: errors.New(conflictMessage(ae))} } } return &exitError{code: 1, err: cerr} @@ -437,10 +439,49 @@ func runClientCreate(ctx context.Context, p *ui.Printer, pr prompter, opts clien } // crossAccountConflictMsg is the guidance shown when this cluster — or the client -// already live on it — belongs to a different tracebloc account. Shared by the -// create 409 (R6) and the R7 not-owned / anchor-taken refusals so they read alike. -const crossAccountConflictMsg = "this cluster is already registered to a different tracebloc account — " + - "sign in to that account, or ask your admin (cluster_conflict)" +// already live on it — belongs to another tracebloc account, and we don't have the +// owner's contact (a client-side refusal, or a backend without the owner_email in +// its 409 body). Contact-the-owner first (never "delete the cluster" — it isn't +// ours to wipe). conflictMessage() enriches this with the owner's email when the +// backend supplies it. +const crossAccountConflictMsg = "this cluster is already registered to another tracebloc account — " + + "ask its owner to release it, or sign in as that account (cluster_conflict)" + +// conflictMessage turns a provisioning 409 body into user guidance. Fix #2 has the +// backend distinguish a genuine cross-account conflict (cluster_conflict, now +// carrying the owner's contact email so the user knows who to ask) from a +// same-account live sibling (cluster_in_use — two clients fighting over one +// cluster). Degrades to the generic cross-account text when the body isn't +// parseable (a backend predating fix #2) or carries no owner. +func conflictMessage(ae *api.APIError) string { + var body struct { + Error string `json:"error"` + OwnerEmail string `json:"owner_email"` + HolderName string `json:"holder_name"` + } + if ae != nil { + _ = json.Unmarshal([]byte(ae.Body), &body) + } + switch body.Error { + case "cluster_in_use": + who := body.HolderName + if who == "" { + who = "another of your clients" + } + return fmt.Sprintf( + "another tracebloc client (%s) in your account is already live on this cluster — "+ + "offboard it first with `tracebloc delete`, or provision on a separate machine (cluster_in_use)", + who) + default: // cluster_conflict, or an unrecognized / empty body + if body.OwnerEmail != "" { + return fmt.Sprintf( + "this cluster is already registered to another tracebloc account (%s) — "+ + "ask them to release it, or sign in as that account (cluster_conflict)", + body.OwnerEmail) + } + return crossAccountConflictMsg + } +} // adoptLiveInClusterClient implements the RFC-0001 §7.2 / R7 adopt-backfill. It // discovers a tracebloc client already live on the target cluster and, when the @@ -518,8 +559,11 @@ func adoptLiveInClusterClient( var ae *api.APIError switch { case errors.As(perr, &ae) && ae.StatusCode == http.StatusConflict: - // Anchor already taken (write-once / bound elsewhere — R6). - return nil, false, &exitError{code: 1, err: errors.New(crossAccountConflictMsg)} + // Anchor held by another client: cross-account (cluster_conflict, now + // naming the owner) or a same-account live sibling (cluster_in_use). + // Fix #2's same-account reclaim means this no longer fires for a stale + // same-account holder — that path now succeeds (200). + return nil, false, &exitError{code: 1, err: errors.New(conflictMessage(ae))} case errors.As(perr, &ae) && ae.StatusCode == http.StatusForbidden: return nil, false, askAnAdmin(ctx, p, apiClient, "provision a client", "provisioning") } diff --git a/internal/cli/client_test.go b/internal/cli/client_test.go index a721d84..eae9e07 100644 --- a/internal/cli/client_test.go +++ b/internal/cli/client_test.go @@ -283,7 +283,7 @@ func TestClientCreate_R7_CrossAccountRefuse(t *testing.T) { err := runClientCreate(context.Background(), ui.New(&bytes.Buffer{}), nil, clientCreateOpts{name: "box", location: "DE", yes: true}) - if err == nil || !strings.Contains(err.Error(), "different tracebloc account") { + if err == nil || !strings.Contains(err.Error(), "registered to another tracebloc account") { t.Errorf("want cross-account refusal, got %v", err) } } @@ -658,11 +658,57 @@ func TestClientCreate_ClusterConflict(t *testing.T) { }) stubClusterID(t, "uid-1", nil) err := runClientCreate(context.Background(), ui.New(&bytes.Buffer{}), nil, clientCreateOpts{name: "c", location: "DE", yes: true}) - if err == nil || !strings.Contains(err.Error(), "different tracebloc account") { + if err == nil || !strings.Contains(err.Error(), "registered to another tracebloc account") { t.Errorf("want a cluster_conflict error, got %v", err) } } +// TestClientCreate_ClusterConflict_RevealsOwnerEmail: fix #2 has the backend put the +// owning account's contact email in the cross-account 409 body; the CLI surfaces it +// so the user knows who to ask to release the cluster. +func TestClientCreate_ClusterConflict_RevealsOwnerEmail(t *testing.T) { + withClientBackend(t, func(w http.ResponseWriter, r *http.Request) { + switch { + case r.Method == http.MethodGet: + _, _ = w.Write([]byte(`[]`)) + case r.Method == http.MethodPost: + w.WriteHeader(http.StatusConflict) + _, _ = w.Write([]byte(`{"error":"cluster_conflict","cluster_id":"uid-1","owner_email":"owner@other.test"}`)) + } + }) + stubClusterID(t, "uid-1", nil) + err := runClientCreate(context.Background(), ui.New(&bytes.Buffer{}), nil, clientCreateOpts{name: "c", location: "DE", yes: true}) + if err == nil || !strings.Contains(err.Error(), "owner@other.test") { + t.Errorf("want the owner email surfaced, got %v", err) + } + if !strings.Contains(err.Error(), "ask them to release it") { + t.Errorf("want contact-the-owner guidance, got %v", err) + } +} + +// TestClientCreate_ClusterInUse: a same-account live sibling already holds the +// anchor (fix #2's cluster_in_use). The message names the live client and points at +// offboarding it — NOT a cross-account "different account" message. +func TestClientCreate_ClusterInUse(t *testing.T) { + withClientBackend(t, func(w http.ResponseWriter, r *http.Request) { + switch { + case r.Method == http.MethodGet: + _, _ = w.Write([]byte(`[]`)) + case r.Method == http.MethodPost: + w.WriteHeader(http.StatusConflict) + _, _ = w.Write([]byte(`{"error":"cluster_in_use","cluster_id":"uid-1","holder_client_id":42,"holder_name":"other-box"}`)) + } + }) + stubClusterID(t, "uid-1", nil) + err := runClientCreate(context.Background(), ui.New(&bytes.Buffer{}), nil, clientCreateOpts{name: "c", location: "DE", yes: true}) + if err == nil || !strings.Contains(err.Error(), "other-box") || !strings.Contains(err.Error(), "cluster_in_use") { + t.Errorf("want a cluster_in_use message naming the live client, got %v", err) + } + if strings.Contains(err.Error(), "another tracebloc account") { + t.Errorf("cluster_in_use must NOT read as a cross-account conflict, got %v", err) + } +} + func TestClientCreate_NoClusterAnchorWarns(t *testing.T) { var body api.CreateClientRequest withClientBackend(t, func(w http.ResponseWriter, r *http.Request) {