fix(delete): don't brick local teardown when the revoke isn't a 403

internal/cli/delete.go

@@ -193,21 +193,57 @@ func runDelete(ctx context.Context, p *ui.Printer, pr prompter, o deleteOpts) er
 
 	// 1. Revoke the machine credential server-side (POST /edge-device/<id>/revoke,
 	// §7.10 / C.6). This kills the credential without deleting the row — the
-	// retained history in scope 2 stays intact. A 403 → ask-an-admin.
+	// retained history in scope 2 stays intact.
+	revoked := true
 	if rerr := client.RevokeClient(ctx, id); rerr != nil {
+ revoked = false
+ // A 426 (CLI too old) won't recover by continuing — the whole offboard talks
+ // to the same backend — so fail fast with the upgrade message rather than
+ // tear the machine down against a backend that can't process the revoke.
+ // Mirrors the pre-offboard guard above (which treats 426 as terminal).
+ var ue *api.UpgradeRequiredError
+ if errors.As(rerr, &ue) {
+ return &exitError{code: 1, err: rerr}
+ }
  var ae *api.APIError
- if errors.As(rerr, &ae) && ae.StatusCode == http.StatusForbidden {
- return askAnAdmin(ctx, p, client, "offboard this machine", "offboarding")
+ if errors.As(rerr, &ae) {
+ switch ae.StatusCode {
+ case http.StatusForbidden:
+ // A 403 is a genuine authorization decision — you can't revoke a
+ // client you don't manage; route to ask-an-admin (unchanged).
+ return askAnAdmin(ctx, p, client, "offboard this machine", "offboarding")
+ case http.StatusUnauthorized:
+ // A 401 means the signed-in session is expired/revoked. With --force
+ // the online-guard above is skipped, so DON'T silently tear the machine
+ // down while a live credential remains — fail fast and point at sign-in,
+ // mirroring the pre-offboard guard's 401 handling.
+ return &exitError{code: 1, err: errors.New(
+ "tracebloc rejected your credentials — run `tracebloc login`, then retry `tracebloc delete`")}
+ }
  }
- return &exitError{code: 1, err: fmt.Errorf("revoking the machine credential: %w", rerr)}
+ // Any OTHER revoke failure must NOT block the local teardown. Removing
+ // tracebloc from THIS machine (helm uninstall, cluster teardown, on-host
+ // data + config wipe, self-remove) is offline-capable and is the command's
+ // primary job — it can't be held hostage to a best-effort remote call. This
+ // hits on a 404 (a stale/wrong-account active-client pointer, or a backend
+ // predating the /revoke route), a transient network/5xx error, etc. Warn and
+ // continue, mirroring the online-guard above. The credential may remain live
+ // server-side (revoked stays false), so the closing summary says so: the user
+ // can revoke it from the dashboard, and the orphan reaper (backend#970) sweeps
+ // a never-torn-down record later.
+ p.Hintf("Couldn't revoke the credential server-side (%v) — continuing with local teardown. "+
+ "The credential may still be live on tracebloc; revoke it from the dashboard if needed.", rerr)
+	} else {
+ p.Successf("Revoked this machine's credential (client %q kept on tracebloc as a record).", name)
 	}
-	p.Successf("Revoked this machine's credential (client %q kept on tracebloc as a record).", name)
 
-	// The teardown steps below are best-effort (the credential is already revoked),
-	// but a step that leaves real state behind — a live release, the local cluster,
-	// or on-host data — must NOT be papered over by the final success line. Track it
-	// so the closing message tells the truth (image reclaim is pure disk cleanup, so
-	// it's intentionally excluded — its own warning already surfaces it).
+	// The teardown steps below are best-effort. (The credential is revoked when the
+	// server-side revoke above succeeded; on a best-effort revoke failure it may
+	// still be live — the closing summary reports which.) A step that leaves real
+	// state behind — a live release, the local cluster, or on-host data — must NOT be
+	// papered over by the final success line. Track it so the closing message tells
+	// the truth (image reclaim is pure disk cleanup, so it's intentionally excluded —
+	// its own warning already surfaces it).
 	degraded := false
 
 	// Clear the local enrollment pointer and persist it IMMEDIATELY — before the
@@ -283,14 +319,23 @@ func runDelete(ctx context.Context, p *ui.Printer, pr prompter, o deleteOpts) er
 	}
 
 	p.Newline()
-	// The credential is revoked either way, so the machine can no longer connect —
-	// but only claim a clean offboard when the teardown actually completed. If a
-	// step left real state behind, say so instead of printing an unqualified success.
-	if degraded {
+	// Be honest on BOTH axes: whether the server-side revoke succeeded (revoked) and
+	// whether the local teardown completed (!degraded). Neither is guaranteed — the
+	// revoke is best-effort on a non-terminal failure, and the teardown steps are
+	// best-effort — so only claim "revoked / no longer connected" when it's true.
+	switch {
+	case revoked && !degraded:
+ p.Successf("Offboarded %q. This machine is no longer connected to tracebloc.", name)
+	case revoked && degraded:
  p.Warnf("Offboarded %q: the machine credential is revoked, so it can no longer connect to tracebloc — "+
  "but some cleanup above didn't complete. Finish the flagged steps by hand.", name)
-	} else {
- p.Successf("Offboarded %q. This machine is no longer connected to tracebloc.", name)
+	case !revoked && !degraded:
+ p.Warnf("Tore down %q on this machine. The server-side revoke didn't complete, so the credential may "+
+ "still be live on tracebloc — revoke it from the dashboard if needed (the orphan reaper sweeps it otherwise).", name)
+	default: // !revoked && degraded
+ p.Warnf("Tore down %q on this machine, but some cleanup above didn't complete and the server-side revoke "+
+ "didn't complete — the credential may still be live on tracebloc (revoke it from the dashboard). "+
+ "Finish the flagged steps by hand.", name)
 	}
 	return nil
 }

internal/cli/delete_test.go

@@ -193,6 +193,138 @@ func TestDelete_Yes_FullSequence(t *testing.T) {
 	assertRemoved(t, fn, filepath.Join(filepath.Dir(exe), "tb"))
 }
 
+// (b') A non-403 revoke failure (a 404 from a stale/wrong-account pointer or a
+// backend predating /revoke, a transient network/5xx error, …) must NOT brick the
+// offboard: local teardown is the command's real job and runs anyway. Only a 403
+// (a genuine authz denial) aborts to ask-an-admin (covered separately).
+func TestDelete_RevokeNon403_ContinuesTeardown(t *testing.T) {
+	withClientBackend(t, func(w http.ResponseWriter, r *http.Request) {
+ switch {
+ case r.Method == http.MethodGet && r.URL.Path == "/edge-device/":
+ _, _ = w.Write([]byte(`[{"id":5,"first_name":"gpu-box-01","namespace":"gpu-box-01","status":0}]`))
+ case r.Method == http.MethodPost && strings.Contains(r.URL.Path, "/revoke"):
+ w.WriteHeader(http.StatusNotFound) // 404: stale pointer / backend predates /revoke
+ default:
+ t.Errorf("unexpected %s %s", r.Method, r.URL.Path)
+ }
+	})
+	setActiveForDelete(t, "5", "gpu-box-01", "gpu-box-01")
+	exe := writeBinaryWithTBAlias(t)
+	fn := &fakeNodeboot{executable: exe}
+	fn.install(t)
+
+	var out bytes.Buffer
+	if err := runDelete(context.Background(), ui.New(&out), nil, deleteOpts{yes: true}); err != nil {
+ t.Fatalf("a non-403 revoke error must not abort the offboard, got: %v", err)
+	}
+	// Teardown still ran despite the failed revoke.
+	for _, want := range []string{"uninstall:gpu-box-01", "teardown:" + nodeboot.ClusterName, "prune"} {
+ found := false
+ for _, c := range fn.calls {
+ if c == want {
+ found = true
+ break
+ }
+ }
+ if !found {
+ t.Errorf("teardown step %q must run even when revoke fails; calls: %v", want, fn.calls)
+ }
+	}
+	// Honest messaging: warn about the failed revoke, and do NOT claim success.
+	s := out.String()
+	if !strings.Contains(s, "Couldn't revoke the credential server-side") {
+ t.Errorf("want a warning about the failed revoke, got:\n%s", s)
+	}
+	if strings.Contains(s, "Revoked this machine's credential") {
+ t.Errorf("must NOT claim the credential was revoked when it wasn't:\n%s", s)
+	}
+}
+
+// (b”) A non-403 revoke failure that ALSO hits a degraded teardown step (here: no
+// namespace → the uninstall is skipped) must tell the truth on BOTH axes — it must
+// NOT claim the credential was revoked or the machine disconnected. Regression for
+// the overclaim: the old closing hardcoded "the credential is revoked, so it can no
+// longer connect" even when the revoke had failed.
+func TestDelete_RevokeNon403_Degraded_HonestClosing(t *testing.T) {
+	withClientBackend(t, func(w http.ResponseWriter, r *http.Request) {
+ switch {
+ case r.Method == http.MethodGet && r.URL.Path == "/edge-device/":
+ _, _ = w.Write([]byte(`[{"id":5,"first_name":"gpu-box-01","namespace":"gpu-box-01","status":0}]`))
+ case r.Method == http.MethodPost && strings.Contains(r.URL.Path, "/revoke"):
+ w.WriteHeader(http.StatusNotFound) // 404: best-effort revoke fails
+ default:
+ t.Errorf("unexpected %s %s", r.Method, r.URL.Path)
+ }
+	})
+	setActiveForDelete(t, "5", "gpu-box-01", "") // no namespace → uninstall skipped → degraded
+	fn := &fakeNodeboot{executable: filepath.Join(t.TempDir(), "tracebloc")}
+	fn.install(t)
+
+	var out bytes.Buffer
+	if err := runDelete(context.Background(), ui.New(&out), nil, deleteOpts{yes: true}); err != nil {
+ t.Fatalf("a best-effort revoke failure must not abort the offboard, got: %v", err)
+	}
+	s := out.String()
+	if strings.Contains(s, "no longer connect") || strings.Contains(s, "credential is revoked") {
+ t.Errorf("closing must NOT claim the credential was revoked / the machine disconnected when revoke failed:\n%s", s)
+	}
+	if !strings.Contains(s, "revoke didn't complete") {
+ t.Errorf("closing should say the server-side revoke didn't complete:\n%s", s)
+	}
+	if !strings.Contains(s, "still be live") {
+ t.Errorf("closing should point at the possibly-live credential:\n%s", s)
+	}
+}
+
+// A 426 from the REVOKE call (reached under --force, which skips the online guard)
+// must fail fast with the upgrade message rather than warn-and-continue into a
+// teardown against a backend that can't process the revoke — matching the guard.
+func TestDelete_RevokeUpgradeRequired_FailsFast(t *testing.T) {
+	withClientBackend(t, func(w http.ResponseWriter, r *http.Request) {
+ if r.Method == http.MethodPost && strings.Contains(r.URL.Path, "/revoke") {
+ w.WriteHeader(http.StatusUpgradeRequired) // 426
+ _, _ = w.Write([]byte(`{"error":"upgrade_required","min_version":"1.2.3"}`))
+ }
+	})
+	setActiveForDelete(t, "5", "gpu-box-01", "gpu-box-01")
+	fn := &fakeNodeboot{executable: filepath.Join(t.TempDir(), "tracebloc")}
+	fn.install(t)
+
+	var out bytes.Buffer
+	err := runDelete(context.Background(), ui.New(&out), nil, deleteOpts{yes: true, force: true})
+	if err == nil || !strings.Contains(err.Error(), "too old") {
+ t.Fatalf("a 426 revoke must fail fast with the upgrade message, got: %v", err)
+	}
+	if len(fn.calls) != 0 {
+ t.Errorf("no teardown after a 426 revoke, got: %v", fn.calls)
+	}
+}
+
+// A 401 from the REVOKE call (reached under --force) means the session is expired/
+// revoked; fail fast and point at re-login rather than tear the machine down while a
+// live credential remains. Without this, --force + an expired token silently wipes
+// the machine and leaves the credential alive.
+func TestDelete_RevokeUnauthorized_FailsFast(t *testing.T) {
+	withClientBackend(t, func(w http.ResponseWriter, r *http.Request) {
+ if r.Method == http.MethodPost && strings.Contains(r.URL.Path, "/revoke") {
+ w.WriteHeader(http.StatusUnauthorized) // 401
+ _, _ = w.Write([]byte(`{"detail":"invalid token"}`))
+ }
+	})
+	setActiveForDelete(t, "5", "gpu-box-01", "gpu-box-01")
+	fn := &fakeNodeboot{executable: filepath.Join(t.TempDir(), "tracebloc")}
+	fn.install(t)
+
+	var out bytes.Buffer
+	err := runDelete(context.Background(), ui.New(&out), nil, deleteOpts{yes: true, force: true})
+	if err == nil || !strings.Contains(err.Error(), "tracebloc login") {
+ t.Fatalf("a 401 revoke must fail fast with a sign-in hint, got: %v", err)
+	}
+	if len(fn.calls) != 0 {
+ t.Errorf("no teardown after a 401 revoke, got: %v", fn.calls)
+	}
+}
+
 // (c) --keep-data spares ~/.tracebloc but still uninstalls + removes the binary.
 func TestDelete_KeepData_SparesDataDir(t *testing.T) {
 	withClientBackend(t, func(w http.ResponseWriter, r *http.Request) {