diff --git a/admin/internal/api/customer.go b/admin/internal/api/customer.go index e199043..6c6df86 100644 --- a/admin/internal/api/customer.go +++ b/admin/internal/api/customer.go @@ -1,6 +1,7 @@ package api import ( + "context" "errors" "fmt" "log" @@ -23,6 +24,7 @@ import ( "github.com/gin-gonic/gin" "github.com/google/uuid" "go.mongodb.org/mongo-driver/v2/bson" + "go.mongodb.org/mongo-driver/v2/mongo" ) // ownedInstance resolves an instance and confirms the session's account owns it. @@ -576,50 +578,126 @@ func renameInstance(c *gin.Context) { return } - if inst.RenamedAt != nil { - if until := inst.RenamedAt.Add(models.RenameCooldown); time.Now().UTC().Before(until) { + ctx := c.Request.Context() + + // The unwind and the audit write run on a context detached from the request. + // The commonest reason the admin-side write fails at all is the caller + // walking away, and an unwind sharing that context fails with it — leaving + // the control plane renamed and admin's row not, which is the exact + // divergence this handler is arranged to prevent. + bgCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), 5*time.Second) + defer cancel() + + // Claim the cooldown atomically BEFORE the control-plane call. Checking it + // and then acting lets two parallel PUTs both pass the check and then + // interleave their two-database writes, which ends with the two databases + // disagreeing about the host — a worse outcome than either rename losing. + // The conditional update IS the cooldown; there is no second reading of it. + now := time.Now().UTC() + var claimed models.Instance + err := db.Admin("admin_instances").FindOneAndUpdate(ctx, + bson.M{ + "instance_id": inst.InstanceID, + "account_id": inst.AccountID, + "$or": []bson.M{ + {"renamed_at": bson.M{"$exists": false}}, + {"renamed_at": bson.M{"$lte": now.Add(-models.RenameCooldown)}}, + }, + }, + bson.M{"$set": bson.M{"renamed_at": now}}).Decode(&claimed) + if err != nil { + if !errors.Is(err, mongo.ErrNoDocuments) { + log.Printf("renameInstance: claiming the cooldown on %s: %v", inst.InstanceID, err) + c.JSON(http.StatusInternalServerError, gin.H{"error": "could not rename the instance"}) + return + } + // No match means the cooldown is live or the row has gone; only a + // re-read tells those apart, and they are different answers. + var cur models.Instance + if err := db.Admin("admin_instances").FindOne(ctx, + bson.M{"instance_id": inst.InstanceID, "account_id": inst.AccountID}).Decode(&cur); err != nil { + c.JSON(http.StatusNotFound, gin.H{"error": "not found"}) + return + } + if cur.RenamedAt != nil { + until := cur.RenamedAt.Add(models.RenameCooldown) c.JSON(http.StatusTooManyRequests, gin.H{ "error": fmt.Sprintf("this instance was renamed recently; it can be renamed again after %s UTC", until.Format("2 Jan 2006 15:04")), "retry_after": until, }) return } + // The row is here and its cooldown is spent, yet the claim matched + // nothing: it changed under us. Nothing has been written, so refuse + // rather than guess which way. + log.Printf("renameInstance: cooldown claim on %s matched nothing against an eligible row", inst.InstanceID) + c.JSON(http.StatusInternalServerError, gin.H{"error": "could not rename the instance"}) + return } - ctx := c.Request.Context() - renamed, err := cloudprov.RenameInstance(ctx, inst.InstanceID, name) + // releaseClaim puts renamed_at back to whatever the claim overwrote — the + // previous instant, or absent when there was none. Every failure past the + // claim owes the customer their rename back. + releaseClaim := func(after string) { + undo := bson.M{"$unset": bson.M{"renamed_at": ""}} + if claimed.RenamedAt != nil { + undo = bson.M{"$set": bson.M{"renamed_at": *claimed.RenamedAt}} + } + if _, err := db.Admin("admin_instances").UpdateOne(bgCtx, + bson.M{"instance_id": inst.InstanceID}, undo); err != nil { + log.Printf("renameInstance: releasing the cooldown claim on %s after %s: %v", inst.InstanceID, after, err) + } + } + + renamed, prevName, prevSlug, err := cloudprov.RenameInstance(ctx, inst.InstanceID, name) switch { case errors.Is(err, provision.ErrSlugTaken): + releaseClaim("a taken slug") c.JSON(http.StatusConflict, gin.H{"error": "that name is already in use — try another"}) return case errors.Is(err, provision.ErrNameRejected): + releaseClaim("a rejected name") c.JSON(http.StatusUnprocessableEntity, gin.H{"error": err.Error()}) return case err != nil: + releaseClaim("a failed control-plane rename") log.Printf("renameInstance: control plane rename of %s: %v", inst.InstanceID, err) c.JSON(http.StatusInternalServerError, gin.H{"error": "could not rename the instance"}) return } - if _, err := db.Admin("admin_instances").UpdateOne(ctx, + // A matched count of zero is a silent version of the same failure: the + // control plane moved and admin's row did not. + res, err := db.Admin("admin_instances").UpdateOne(ctx, bson.M{"instance_id": inst.InstanceID}, - bson.M{"$set": bson.M{ - "name": renamed.Name, - "slug": renamed.Slug, - "renamed_at": time.Now().UTC(), - }}); err != nil { - if rbErr := cloudprov.RestoreInstanceIdentity(ctx, inst.InstanceID, inst.Name, inst.Slug); rbErr != nil { + bson.M{"$set": bson.M{"name": renamed.Name, "slug": renamed.Slug}}) + if err == nil && res.MatchedCount == 0 { + err = errors.New("admin_instances row matched nothing") + } + if err != nil { + // The control plane's own previous values, not admin's copy: admin's may + // be stale, and its slug is omitempty. + if rbErr := cloudprov.RestoreInstanceIdentity(bgCtx, inst.InstanceID, prevName, prevSlug); rbErr != nil { log.Printf("renameInstance: rollback of %s failed: %v", inst.InstanceID, rbErr) } + releaseClaim("a failed record write") log.Printf("renameInstance: record rename of %s: %v", inst.InstanceID, err) c.JSON(http.StatusInternalServerError, gin.H{"error": "could not rename the instance"}) return } + if renamed.Slug == prevSlug { + // The cooldown exists because a rename moves the DNS host; a cosmetic + // edit that derives to the same slug moves nothing, so it should not + // spend one. The claim is already written by this point — releasing it + // is how that is expressed now the check is atomic. + releaseClaim("a rename that did not move the host") + } + s := auth.Current(c) - audit.Write(ctx, models.AuditEntry{ + audit.Write(bgCtx, models.AuditEntry{ Actor: s.Email, Action: "instance.renamed", AccountID: s.AccountID, - Target: inst.InstanceID, Detail: inst.Slug + " -> " + renamed.Slug, IP: c.ClientIP()}) + Target: inst.InstanceID, Detail: prevSlug + " -> " + renamed.Slug, IP: c.ClientIP()}) c.JSON(http.StatusOK, gin.H{ "instance_id": inst.InstanceID, diff --git a/admin/internal/api/staff.go b/admin/internal/api/staff.go index 13a2d83..f5f726b 100644 --- a/admin/internal/api/staff.go +++ b/admin/internal/api/staff.go @@ -1,6 +1,7 @@ package api import ( + "context" "errors" "fmt" "log" @@ -631,6 +632,11 @@ func staffCreateAccountUser(c *gin.Context) { // On self-hosted it changes admin's label only. There is no control-plane row to // write — the install is the customer's — and no slug, because self-hosted has // no tenant subdomain. +// +// A cloud placeholder is refused outright rather than relabelled: it has no +// control-plane row yet, so a label-only rename here would be a name that the +// instance never gets when provisioning finally derives its slug from the +// checkout's name. The customer endpoint refuses it for the same reason. func staffRenameInstance(c *gin.Context) { var body struct { Name string `json:"name"` @@ -652,12 +658,27 @@ func staffRenameInstance(c *gin.Context) { c.JSON(http.StatusNotFound, gin.H{"error": "not found"}) return } + if inst.Deployment == license.DeploymentCloud && inst.Placeholder { + c.JSON(http.StatusConflict, gin.H{"error": "this instance is not provisioned yet"}) + return + } + + // The unwind and the audit write must survive the request being cancelled: + // an unwind on a dead context leaves the two databases disagreeing, which is + // the failure the unwind exists for. + bgCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), 5*time.Second) + defer cancel() set := bson.M{"name": name} slug := inst.Slug + cloud := inst.Deployment == license.DeploymentCloud + // The control plane's own previous values, not admin's copy: admin's may be + // stale, and its slug is omitempty, so unwinding from it can write an empty + // slug into instances. + prevName, prevSlug := inst.Name, inst.Slug - if inst.Deployment == license.DeploymentCloud && !inst.Placeholder { - renamed, err := cloudprov.RenameInstance(ctx, inst.InstanceID, name) + if cloud { + renamed, pName, pSlug, err := cloudprov.RenameInstance(ctx, inst.InstanceID, name) switch { case errors.Is(err, provision.ErrSlugTaken): c.JSON(http.StatusConflict, gin.H{"error": "that name is already in use"}) @@ -669,14 +690,21 @@ func staffRenameInstance(c *gin.Context) { c.JSON(http.StatusInternalServerError, gin.H{"error": err.Error()}) return } + prevName, prevSlug = pName, pSlug slug = renamed.Slug set["slug"] = renamed.Slug } - if _, err := db.Admin("admin_instances").UpdateOne(ctx, - bson.M{"instance_id": inst.InstanceID}, bson.M{"$set": set}); err != nil { - if inst.Deployment == license.DeploymentCloud && !inst.Placeholder { - if rbErr := cloudprov.RestoreInstanceIdentity(ctx, inst.InstanceID, inst.Name, inst.Slug); rbErr != nil { + // A matched count of zero is the same failure quietly: the control plane + // moved and admin's row did not. + res, err := db.Admin("admin_instances").UpdateOne(ctx, + bson.M{"instance_id": inst.InstanceID}, bson.M{"$set": set}) + if err == nil && res.MatchedCount == 0 { + err = errors.New("admin_instances row matched nothing") + } + if err != nil { + if cloud { + if rbErr := cloudprov.RestoreInstanceIdentity(bgCtx, inst.InstanceID, prevName, prevSlug); rbErr != nil { log.Printf("staffRenameInstance: rollback of %s failed: %v", inst.InstanceID, rbErr) } } @@ -684,9 +712,9 @@ func staffRenameInstance(c *gin.Context) { return } - audit.Write(ctx, models.AuditEntry{ + audit.Write(bgCtx, models.AuditEntry{ Actor: auth.Current(c).Email, Action: "instance.renamed", AccountID: inst.AccountID, - Target: inst.InstanceID, Detail: inst.Slug + " -> " + slug, IP: c.ClientIP()}) + Target: inst.InstanceID, Detail: prevSlug + " -> " + slug, IP: c.ClientIP()}) c.JSON(http.StatusOK, gin.H{"instance_id": inst.InstanceID, "name": name, "slug": slug}) } diff --git a/admin/internal/cloudprov/cloudprov.go b/admin/internal/cloudprov/cloudprov.go index 9824aeb..ed7c4a3 100644 --- a/admin/internal/cloudprov/cloudprov.go +++ b/admin/internal/cloudprov/cloudprov.go @@ -209,7 +209,10 @@ func ProjectedUsers(ctx context.Context, hqUserID string) ([]sharedmodels.User, // It writes `instances` and nothing else, so admin's control-plane write // boundary is unchanged. It issues no licence: a licence binds the instance // UUID, which a rename never touches. -func RenameInstance(ctx context.Context, instanceID, name string) (*sharedmodels.Instance, error) { +// +// The previous name and slug come back with the result because they are what an +// unwind must restore — admin's own copy can be stale, or slugless. +func RenameInstance(ctx context.Context, instanceID, name string) (inst *sharedmodels.Instance, prevName, prevSlug string, err error) { return provision.RenameInstance(ctx, db.ControlDB(), instanceID, name) } diff --git a/admin/internal/models/models.go b/admin/internal/models/models.go index c12bcfb..32aea0c 100644 --- a/admin/internal/models/models.go +++ b/admin/internal/models/models.go @@ -150,8 +150,8 @@ type Instance struct { // rename cooldown. It is a pointer because absent means "never renamed"; a // zero time.Time would read as year 1 — an inert cooldown, but only by // accident. Staff renames deliberately leave it alone. - RenamedAt *time.Time `bson:"renamed_at,omitempty" json:"renamed_at,omitempty"` - InjectFailedAt *time.Time `bson:"inject_failed_at,omitempty" json:"inject_failed_at,omitempty"` + RenamedAt *time.Time `bson:"renamed_at,omitempty" json:"renamed_at,omitempty"` + InjectFailedAt *time.Time `bson:"inject_failed_at,omitempty" json:"inject_failed_at,omitempty"` // NoticesSent holds the lifecycle notice keys already emailed for the // CURRENT term ("expiring", "expired", "delete_7", "delete_1"). Renewal // clears it, so the next term starts the sequence again. It is what stops a diff --git a/adminsite/app/(customer)/instances/[id]/page.tsx b/adminsite/app/(customer)/instances/[id]/page.tsx index e110eec..9c63b51 100644 --- a/adminsite/app/(customer)/instances/[id]/page.tsx +++ b/adminsite/app/(customer)/instances/[id]/page.tsx @@ -213,7 +213,15 @@ export default function InstancePage() { The instance name is where its address comes from. Renaming moves it to a new address and releases the old one, so saved links and bookmarks to it stop working.

+ {/* + * Keyed on the instance: this element stays mounted + * across a navigation between two instance pages, so + * without a key the success note and the typed name + * from one instance surface on the next. + */} { diff --git a/adminsite/app/(staff)/staff/instances/[id]/page.tsx b/adminsite/app/(staff)/staff/instances/[id]/page.tsx index 7090430..b56f2f6 100644 --- a/adminsite/app/(staff)/staff/instances/[id]/page.tsx +++ b/adminsite/app/(staff)/staff/instances/[id]/page.tsx @@ -87,7 +87,14 @@ export default function StaffInstancePage() { * hours. */} + {/* + * Keyed on the instance so a success note cannot follow staff + * from one instance page to the next — the element stays + * mounted across that navigation. + */} { diff --git a/adminsite/components/RenamePanel.tsx b/adminsite/components/RenamePanel.tsx index bf406b6..555c991 100644 --- a/adminsite/components/RenamePanel.tsx +++ b/adminsite/components/RenamePanel.tsx @@ -14,14 +14,22 @@ import { baseSlug, hostFor, slugError } from "@/lib/slug"; * * The host preview is drawn from lib/slug.ts, a mirror of the Go rules. It can * disagree with the server; the 409 that comes back is the answer that counts. + * + * movesHost is what separates a rename that moves a DNS host from one that only + * changes a label. Self-hosted instances and unprovisioned cloud placeholders + * have no address, so every word about old links breaking and signing in again + * is false for them — and a preview host they will never live at is worse than + * no preview at all. */ export function RenamePanel({ currentName, currentSlug, + movesHost, onRename, }: { currentName: string; currentSlug: string; + movesHost: boolean; onRename: (name: string) => Promise; }) { const [open, setOpen] = useState(false); @@ -45,6 +53,10 @@ export function RenamePanel({ const res = await onRename(name); setDone(res); setOpen(false); + // The input is prefilled with the current name, and the current name + // is now this one. Leaving the old text in would make the next open + // look like an edit already in progress. + setValue(res.name); } catch (err) { setError(err instanceof ApiError ? err.message : "Rename failed. Try again."); } finally { @@ -52,26 +64,34 @@ export function RenamePanel({ } } - if (done) { - const host = done.login_url || `https://${hostFor(done.slug)}`; - return ( - - - - This instance is now {done.name}, at{" "} - {hostFor(done.slug)}. The old address has stopped working, and your - sign-in does not follow it — you will need to sign in again there. - - - Open {hostFor(done.slug)} → - - - - ); - } - + // The note sits ABOVE the control rather than replacing it. A rename is not + // a one-shot action — a customer who mistypes the new name needs the panel + // back, and returning early here left them with a success message and no way + // to correct it short of a reload. return (
+ {done && + (movesHost ? ( + + + + This instance is now {done.name}, at{" "} + {hostFor(done.slug)}. The old address has stopped working, and + your sign-in does not follow it — you will need to sign in again there. + + + Open {hostFor(done.slug)} → + + + + ) : ( + + This instance is now {done.name}. + + ))} {open && ( setValue(e.target.value)} error={error ?? (name ? invalid : undefined)} hint={ - name && !invalid ? ( + movesHost && name && !invalid ? ( <> Moves to {hostFor(derived)} {derived === currentSlug && " — the address does not change"} @@ -99,7 +119,7 @@ export function RenamePanel({ > {busy ? "Renaming…" : "Rename instance"} - {open && ( + {open && movesHost && ( Anyone signed in will need to sign in again at the new address, and links to the old one stop working. diff --git a/adminsite/lib/api.ts b/adminsite/lib/api.ts index c1d3909..60e07e5 100644 --- a/adminsite/lib/api.ts +++ b/adminsite/lib/api.ts @@ -129,6 +129,8 @@ export interface Instance { status: InstanceStatus; current_license?: string; relink_count: number; + /** Cloud only, and only until the paid checkout provisions the real row. */ + placeholder?: boolean; renamed_at?: string; inject_failed_at?: string | null; notices_sent?: string[]; diff --git a/docs/superpowers/specs/2026-08-12-instance-rename-design.md b/docs/superpowers/specs/2026-08-12-instance-rename-design.md index 671e2e3..fc1ac47 100644 --- a/docs/superpowers/specs/2026-08-12-instance-rename-design.md +++ b/docs/superpowers/specs/2026-08-12-instance-rename-design.md @@ -39,10 +39,13 @@ that tempts someone to touch the licence from this path. are unaffected — they dial `GRPC_HOST`, which is not per-tenant. - **The old host keeps working for up to 60 seconds.** `server/internal/auth/instancehost.go` caches slug-to-instance lookups for 60s, and admin has no path to invalidate - another process's memory. This is a lag, not a leak: the stale entry maps the - old slug to the same instance, so nothing is exposed that was not exposed a - minute earlier. Adding a cross-service invalidation channel for a 60-second - window is not worth the coupling. + another process's memory. The released slug can be claimed by another account + inside that window, so for up to a minute a replica still maps that host to the + previous tenant. No data is exposed — the host/session guard rejects a session + belonging to a different instance — but the new owner's users can briefly reach + the old tenant's instance on their own host, and see its login page rather than + theirs. Adding a cross-service invalidation channel for a 60-second window is + not worth the coupling. - **The customer must sign in again.** `km_session` is set with no `Domain` attribute, so it is host-only and does not follow the instance to its new subdomain. The UI says so rather than letting the customer discover it. @@ -52,7 +55,7 @@ that tempts someone to touch the licence from this path. | | Customer (owner or admin) | Staff | |---|---|---| | Cloud instance | rename, 24h cooldown | rename, no cooldown | -| Self-hosted instance | refused, 409 | name only; there is no slug | +| Self-hosted instance | refused, 400 | name only; there is no slug | | Cloud placeholder | refused, 409 | refused, 409 | Self-hosted is refused on the customer side for the same reason the member @@ -219,7 +222,7 @@ exercise, matching existing practice: - A second rename within 24 hours answers 429. - A rename onto an occupied slug answers 409 and changes nothing. - A rename attempt on a self-hosted instance from the customer portal answers - 409. + 400, the same status and constant the member endpoints already answer with. - The audit log carries `instance.renamed` with both slugs. ## Out of scope diff --git a/shared/provision/instance.go b/shared/provision/instance.go index 2ba4d30..d4c6d8f 100644 --- a/shared/provision/instance.go +++ b/shared/provision/instance.go @@ -107,34 +107,41 @@ func RenameSlug(name, currentSlug string) (string, error) { // RenameInstance changes an instance's name and re-derives its slug from it. // +// It returns the name and slug the control plane held BEFORE the write, and +// those are the only correct values to unwind with. The caller's own copy of the +// instance may be stale, and admin's copy stores slug with `omitempty`, so an +// unwind driven from there can write an empty slug — which either mis-restores +// the tenant host or trips the unique index against every other slugless row. +// // The count-then-update is racy on its own, and is safe for the same reason // CreateInstanceWithID's loop is: instances.slug carries a unique index, so a // lost race surfaces as a duplicate-key error. Unlike creation there is nothing // to retry with — the caller asked for one specific name — so it becomes // ErrSlugTaken. Do not remove the duplicate-key branch, and do not remove the // index. -func RenameInstance(ctx context.Context, db *mongo.Database, instanceID, name string) (*models.Instance, error) { - var inst models.Instance +func RenameInstance(ctx context.Context, db *mongo.Database, instanceID, name string) (inst *models.Instance, prevName, prevSlug string, err error) { + var cur models.Instance if err := db.Collection("instances").FindOne(ctx, - bson.M{"instance_id": instanceID}).Decode(&inst); err != nil { - return nil, err + bson.M{"instance_id": instanceID}).Decode(&cur); err != nil { + return nil, "", "", err } + prevName, prevSlug = cur.Name, cur.Slug - slug, err := RenameSlug(name, inst.Slug) + slug, err := RenameSlug(name, cur.Slug) if err != nil { - return nil, err + return nil, prevName, prevSlug, err } - if slug != inst.Slug { + if slug != cur.Slug { n, err := db.Collection("instances").CountDocuments(ctx, bson.M{ "slug": slug, "instance_id": bson.M{"$ne": instanceID}, }) if err != nil { - return nil, err + return nil, prevName, prevSlug, err } if n > 0 { - return nil, fmt.Errorf("%w: %s", ErrSlugTaken, slug) + return nil, prevName, prevSlug, fmt.Errorf("%w: %s", ErrSlugTaken, slug) } } @@ -142,14 +149,14 @@ func RenameInstance(ctx context.Context, db *mongo.Database, instanceID, name st bson.M{"instance_id": instanceID}, bson.M{"$set": bson.M{"name": name, "slug": slug}}); err != nil { if mongo.IsDuplicateKeyError(err) { - return nil, fmt.Errorf("%w: %s", ErrSlugTaken, slug) + return nil, prevName, prevSlug, fmt.Errorf("%w: %s", ErrSlugTaken, slug) } - return nil, err + return nil, prevName, prevSlug, err } - inst.Name = name - inst.Slug = slug - return &inst, nil + cur.Name = name + cur.Slug = slug + return &cur, prevName, prevSlug, nil } // RestoreInstanceIdentity writes an exact name and slug back, unwinding a rename