amadan
console: attach core — alias-branch side effects pinned, idempotent adoption narrowed
Re-review round, two items: - The alias attach branch had no test for its audit row or serial bump; both attach branches and detach now pin them. - The idempotent record-conflict branch accepted any readable record at the key. An app with a custom manifest Domain can have carlos instances create write an INSTANCE record there, and adopting one would let a later detach retire a tenant instance this door never created. Adoption is now limited to the fixed shape a first-route attach writes (static, no slug, shared pool, unconstrained region); anything else keeps the 422 and the claim rollback, with the record untouched.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
2 files changed,
+63
−6
internal/console/domainattach.go+20 −6internal/console/domainattach_test.go+43 −0
diff --git a/internal/console/domainattach.go b/internal/console/domainattach.go| index 5249a5c91..7807bcf1a 100644 |
| --- a/internal/console/domainattach.go |
| +++ b/internal/console/domainattach.go |
| @@ -127,16 +127,17 @@ func (s *Server) attachDomain(ctx context.Context, acct *Account, app, host, kin |
| // not the one this call would have written. |
| if errors.Is(err, release.ErrCASConflict) { |
| existing, gerr := provision.GetRecord(ctx, s.cfg.Releases, acct.Sqid, app, host) |
| - if gerr == nil { |
| + if gerr == nil && attachWrote(*existing) { |
| s.audit(acct, app, principal, "domain-attach", host+" (first route, "+existing.Channel+", record already present)") |
| s.bumpSerial(ctx) |
| return attachResult{Claim: claim, FirstRoute: true, Channel: existing.Channel}, nil |
| } |
| - // Unreadable: malformed, or a body that lies about its identity. |
| - // Not ours to delete and not safe to write over, so the claim |
| - // goes back and the member is told what stands in the way — the |
| - // fact of a record, and nothing about who wrote it. |
| - log.Printf("console: attach %s: existing record unreadable: %v", host, gerr) |
| + // Unreadable (malformed, or a body that lies about its |
| + // identity) or written by a door other than this one. Not ours |
| + // to delete and not safe to write over, so the claim goes back |
| + // and the member is told what stands in the way — the fact of a |
| + // record, and nothing about who wrote it. |
| + log.Printf("console: attach %s: existing record is not this door's to adopt (read: %v)", host, gerr) |
| s.rollbackAttach(ctx, acct.Sqid, app, host, false) |
| return attachResult{}, &doorRefusal{http.StatusUnprocessableEntity, |
| "a route record already exists for this host — detach it or pick another hostname"} |
| @@ -315,6 +316,19 @@ func (s *Server) attachChannel(ctx context.Context, sqid, app, channelFlag, self |
| ") — say which one this domain serves"} |
| } |
| +// attachWrote reports whether rec is a record THIS door could have written: |
| +// the fixed shape of a first-route attach — kind static, no slug, and the |
| +// shared pool and unconstrained region (§3). It is the guard on the |
| +// idempotent conflict branch, and it is narrow on purpose. The record key |
| +// is host-scoped within an app, and an app with a custom manifest Domain |
| +// can have `carlos instances create` write an INSTANCE record at the very |
| +// same key; adopting one of those as "an attach that already happened" |
| +// would hand detach the authority to retire a tenant instance this door |
| +// never created. Anything that fails this test is left alone and refused. |
| +func attachWrote(rec provision.Record) bool { |
| + return rec.Kind == "static" && rec.Slug == "" && rec.Region == "" && rec.Pool == "" |
| +} |
| + |
| // bucketRefusal is the "this console cannot answer at all" check both verbs |
| // open with: a deployment with no bucket has nowhere to put a claim or a |
| // record. Same 503 and the same sentence the instance API's gate uses |
diff --git a/internal/console/domainattach_test.go b/internal/console/domainattach_test.go| index 5e3c8bc93..a9ab27bcd 100644 |
| --- a/internal/console/domainattach_test.go |
| +++ b/internal/console/domainattach_test.go |
| @@ -155,6 +155,8 @@ func TestAttachAliasWhenAppHasReadyRoute(t *testing.T) { |
| Account: fx.sqid, Channel: "canary/live", Status: "ready"}, |
| ) |
| const host = "story.carlosframework.com" |
| + ops := &opStore{Store: fx.rel} |
| + fx.srv.cfg.Releases = ops |
| res, ref := fx.srv.attachDomain(context.Background(), acct, "story", host, "", "", "paul@keymail.dev") |
| if ref != nil { |
| t.Fatalf("attach: refused %+v", ref) |
| @@ -183,6 +185,14 @@ func TestAttachAliasWhenAppHasReadyRoute(t *testing.T) { |
| if len(keys) != 0 { |
| t.Errorf("records after an alias attach = %v, want none (the alias is derived box-side)", keys) |
| } |
| + // The alias branch owes the same two side effects the first-route |
| + // branch does: both doors get them from here, or neither does. |
| + auditedOnce(t, fx, acct, "story", "domain-attach", host) |
| + claimPut := ops.opIndex("put " + domainclaim.Key(host)) |
| + serial := ops.opIndex("put " + release.SerialKey) |
| + if claimPut < 0 || serial < claimPut { |
| + t.Errorf("ops %v: want the serial bumped after the claim it announces (claim=%d serial=%d)", ops.ops, claimPut, serial) |
| + } |
| } |
| // TestAttachNeverAliasesAHostToItself is the operator-added custom domain |
| @@ -364,6 +374,39 @@ func TestAttachUnreadableExistingRecordRefuses422(t *testing.T) { |
| claimAbsent(t, fx, host) |
| } |
| +// TestAttachInstanceRecordAtTheKeyIsNotAdopted is the narrow half of the |
| +// idempotent branch: an app with a custom manifest Domain can have `carlos |
| +// instances create` write an INSTANCE record at this very key. Treating one |
| +// of those as "an attach that already happened" would hand detach the |
| +// authority to retire a tenant instance this door never created. |
| +func TestAttachInstanceRecordAtTheKeyIsNotAdopted(t *testing.T) { |
| + fx, acct := attachFixture(t) |
| + shipVersion(t, fx, "story", "v1") |
| + seedChannel(t, fx, "story", "canary/live", "v1") |
| + const host = "story.carlosframework.com" |
| + // Kind "" is an instance record — what reconcile minted before Kind |
| + // existed, and what the instance door writes today. |
| + instance := provision.Record{ |
| + Schema: 1, Host: host, App: "story", Account: fx.sqid, |
| + Slug: "story", Channel: "stable", |
| + } |
| + if err := provision.CreateRecord(context.Background(), fx.rel, instance); err != nil { |
| + t.Fatal(err) |
| + } |
| + _, ref := fx.srv.attachDomain(context.Background(), acct, "story", host, "", "", "paul@keymail.dev") |
| + if ref == nil || ref.Status != 422 { |
| + t.Fatalf("refusal = %+v, want 422 — an instance record is not an attach to adopt", ref) |
| + } |
| + claimAbsent(t, fx, host) |
| + got, err := provision.GetRecord(context.Background(), fx.rel, fx.sqid, "story", host) |
| + if err != nil { |
| + t.Fatalf("record: %v, want the instance record untouched", err) |
| + } |
| + if got.Kind != "" || got.Slug != "story" || got.Channel != "stable" { |
| + t.Errorf("record = %+v, want the instance record exactly as it was", *got) |
| + } |
| +} |
| + |
| // TestAttachAmbiguousRecordFailureUnwindsBoth: a write that LANDS and then |
| // reports failure (a timeout) must not leave the record behind the claim it |
| // rolls back — that record would materialize a route for a host anyone may |