From 68284b2a60040b1fd39d03f7b14527af139592fb Mon Sep 17 00:00:00 2001 From: Lukas Wuttke Date: Thu, 9 Jul 2026 20:06:07 +0200 Subject: [PATCH 1/2] fix(delete): don't brick local teardown when the server-side revoke isn't a 403 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit runDelete treated the credential revoke as a hard gate: only HTTP 403 was special-cased (ask-an-admin); every OTHER RevokeClient error returned exitError{code:1} and short-circuited the entire offboard — helm uninstall, k3d teardown, image prune, ~/.tracebloc wipe, and self-remove never ran. So a stale/ wrong-account active-client pointer (account-scoped 404), a backend predating the /edge-device//revoke route (404), or a transient network/5xx blip left the machine fully installed with no escape (--force only skips the online guard). That contradicts the same function's online-guard, which already treats 5xx/429/network as "warn and continue — the teardown is the real gate," and its own comment anticipating a 404 revoke. Fix: non-403 revoke failures now warn and continue into the (offline-capable) local teardown, mirroring the online guard. 403 still routes to ask-an-admin (unchanged, still tested). "Revoked …" is printed only on actual success; the warn path says the credential may still be live (revoke from the dashboard; the orphan reaper backend#970 sweeps a never-torn-down record later). Test: TestDelete_RevokeNon403_ContinuesTeardown (404 revoke → teardown still runs, honest warning, no false "revoked" claim). Existing TestDelete_RevokeForbidden (403 → ask-an-admin) unchanged. Full suite green; gofmt -s / errcheck / ineffassign clean. Co-Authored-By: Claude Opus 4.8 (1M context) --- internal/cli/delete.go | 19 ++++++++++++--- internal/cli/delete_test.go | 47 +++++++++++++++++++++++++++++++++++++ 2 files changed, 63 insertions(+), 3 deletions(-) diff --git a/internal/cli/delete.go b/internal/cli/delete.go index 4d85e85b..d4ba2102 100644 --- a/internal/cli/delete.go +++ b/internal/cli/delete.go @@ -193,15 +193,28 @@ func runDelete(ctx context.Context, p *ui.Printer, pr prompter, o deleteOpts) er // 1. Revoke the machine credential server-side (POST /edge-device//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. if rerr := client.RevokeClient(ctx, id); rerr != nil { var ae *api.APIError if errors.As(rerr, &ae) && ae.StatusCode == 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") } - 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, so say 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, diff --git a/internal/cli/delete_test.go b/internal/cli/delete_test.go index fbfab450..4b9bb212 100644 --- a/internal/cli/delete_test.go +++ b/internal/cli/delete_test.go @@ -193,6 +193,53 @@ 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) + } +} + // (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) { From ec884f9dfc357e4bfca56af60f2e4d3fc36db304 Mon Sep 17 00:00:00 2001 From: Lukas Wuttke Date: Fri, 10 Jul 2026 12:25:52 +0200 Subject: [PATCH 2/2] fix(delete): honest offboard summary + treat 426/401 revoke as terminal Addresses @saadqbal's #205 review: - 426 revoke now fails fast with the upgrade prompt (mirrors the pre-offboard guard) instead of warn-and-continue into a teardown the backend can't process. - 401 revoke now fails fast to re-login. The --force footgun: with --force the online guard is skipped, so an expired session used to silently tear the machine down while leaving a live credential; now it aborts to sign-in. - Track whether the server-side revoke actually succeeded; the closing summary is honest on BOTH axes (revoke status x teardown status) and no longer claims the credential is revoked / the machine disconnected when the revoke failed. - Fix the now-false "credential is already revoked" comment on the teardown block. - Tests: 404 + no-namespace honest closing (the degraded+revoke-failed case the old test never reached), plus 426 and 401 revoke fail-fast. Co-Authored-By: Claude Opus 4.8 --- internal/cli/delete.go | 66 ++++++++++++++++++++-------- internal/cli/delete_test.go | 85 +++++++++++++++++++++++++++++++++++++ 2 files changed, 134 insertions(+), 17 deletions(-) diff --git a/internal/cli/delete.go b/internal/cli/delete.go index d4ba2102..754b7b11 100644 --- a/internal/cli/delete.go +++ b/internal/cli/delete.go @@ -194,12 +194,32 @@ func runDelete(ctx context.Context, p *ui.Printer, pr prompter, o deleteOpts) er // 1. Revoke the machine credential server-side (POST /edge-device//revoke, // §7.10 / C.6). This kills the credential without deleting the row — the // 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 { - // 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") + 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`")} + } } // Any OTHER revoke failure must NOT block the local teardown. Removing // tracebloc from THIS machine (helm uninstall, cluster teardown, on-host @@ -208,19 +228,22 @@ func runDelete(ctx context.Context, p *ui.Printer, pr prompter, o deleteOpts) er // 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, so say so: the user can revoke it from the dashboard, and the - // orphan reaper (backend#970) sweeps a never-torn-down record later. + // 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) } - // 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 @@ -296,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 } diff --git a/internal/cli/delete_test.go b/internal/cli/delete_test.go index 4b9bb212..f70bb56c 100644 --- a/internal/cli/delete_test.go +++ b/internal/cli/delete_test.go @@ -240,6 +240,91 @@ func TestDelete_RevokeNon403_ContinuesTeardown(t *testing.T) { } } +// (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) {