Vantage/vantage-app
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix: Harden instance rename against interleaving and lost unwinds

Browse Source
This commit is contained in:
mrhid6
2026-08-12 11:08:36 +00:00
parent f534b74066
commit cdc50b7aaf
10 changed files with 220 additions and 64 deletions
Download Patch File Download Diff File Expand all files Collapse all files
admin/internal/api/customer.go
+91 -13
View File
@@ -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,
admin/internal/api/staff.go
+36 -8
View File
@@ -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})
}
admin/internal/cloudprov/cloudprov.go
+4 -1
View File
@@ -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)
}
admin/internal/models/models.go
+2 -2
View File
@@ -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
adminsite/app/(customer)/instances/[id]/page.tsx
+8
View File
@@ -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.
</p>
{/*
* 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.
*/}
<RenamePanel
key={instance.instance_id}
movesHost
currentName={instance.name}
currentSlug={instance.slug ?? ""}
onRename={async (name) => {
adminsite/app/(staff)/staff/instances/[id]/page.tsx
+7
View File
@@ -87,7 +87,14 @@ export default function StaffInstancePage() {
* hours.
*/}
<Panel title="Name" meta={data.instance.deployment === "cloud" ? "Moves the address" : "Label only"}>
{/*
* 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.
*/}
<RenamePanel
key={data.instance.instance_id}
movesHost={data.instance.deployment === "cloud" && !data.instance.placeholder}
currentName={data.instance.name}
currentSlug={data.instance.slug ?? ""}
onRename={async (name) => {
adminsite/components/RenamePanel.tsx
+40 -20
View File
@@ -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<RenameResult>;
}) {
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 (
<Note tone="warn">
<span className="grid gap-2">
<span>
This instance is now <strong>{done.name}</strong>, at{" "}
<span className="font-mono">{hostFor(done.slug)}</span>. The old address has stopped working, and your
sign-in does not follow it — you will need to sign in again there.
</span>
<a href={host} className="justify-self-start font-mono text-[0.78rem] text-accent underline">
Open {hostFor(done.slug)} &rarr;
</a>
</span>
</Note>
);
}
// 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 (
<div className="grid gap-3">
{done &&
(movesHost ? (
<Note tone="warn">
<span className="grid gap-2">
<span>
This instance is now <strong>{done.name}</strong>, at{" "}
<span className="font-mono">{hostFor(done.slug)}</span>. The old address has stopped working, and
your sign-in does not follow it — you will need to sign in again there.
</span>
<a
href={done.login_url || `https://${hostFor(done.slug)}`}
className="justify-self-start font-mono text-[0.78rem] text-accent underline"
>
Open {hostFor(done.slug)} &rarr;
</a>
</span>
</Note>
) : (
<Note tone="warn">
This instance is now <strong>{done.name}</strong>.
</Note>
))}
{open && (
<Field
label="Instance name"
@@ -79,7 +99,7 @@ export function RenamePanel({
onChange={(e) => setValue(e.target.value)}
error={error ?? (name ? invalid : undefined)}
hint={
name && !invalid ? (
movesHost && name && !invalid ? (
<>
Moves to <span className="font-mono">{hostFor(derived)}</span>
{derived === currentSlug && " — the address does not change"}
@@ -99,7 +119,7 @@ export function RenamePanel({
>
{busy ? "Renaming…" : "Rename instance"}
</Button>
{open && (
{open && movesHost && (
<span className="text-[0.82rem] text-ink-3">
Anyone signed in will need to sign in again at the new address, and links to the old one stop working.
</span>
adminsite/lib/api.ts
+2
View File
@@ -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[];
docs/superpowers/specs/2026-08-12-instance-rename-design.md
+9 -6
View File
@@ -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
shared/provision/instance.go
+21 -14
View File
@@ -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