amadan
fix(provision): removal authority survives a re-add; both doors hold the host invariant
Four review findings, all of the same family: an invariant that held at one
door and not the other, or a value one caller could quietly revoke for
another.
`carlos add` on a provisioned host silently revoked the reconcile pass's
removal authority. Upsert wrote provisioned=0 and seed='' from a Route
built by a caller that knows nothing about provisioning — after which the
bucket record's deletion could never retire that route again, because the
flag IS the authority (spec §4), and the route's first-boot argv was gone
with it. Both columns now take the `environment` CASE guard the same trap
already earned once (2026-08-02 review, IMPORTANT 3): the zero value
preserves, a real value wins, and clearing keeps one door each — row
deletion for provisioned, SetSeed for seed, neither going through Upsert.
DELETE validated its host against the CURRENT manifest's domain when one
existed. The suffix rule is create-time policy — "which hosts may this app
claim" — and deletion has to outlive every version of it: an app that moves
domains, or retires the manifest, still has records under the old one, and
those are exactly the records that must be deletable. Refusing them strands
provisioned routes with nothing left able to retire them. Delete now checks
shape alone, always, which is the whole traversal defence it ever needed
(the {host} segment arrives percent-decoded). The manifest read leaves the
delete path entirely, so instanceManifest has one caller — create — which
is the only verb manifest policy governs.
The reconcile pass trusted the console's host check. The record is the
authorization and this pass builds a socket path, a DB path and a registry
key out of it, so it now runs ValidHostShape itself and skips loudly —
protected from removal like a corrupted body, since a record that cannot be
used is not one that was retired. Anything with bucket write access reaches
that code without passing the console at all.
Two smaller ones: the seed's env now drops CARLOS_CONTROL_SOCKET and
CARLOS_PURGE (Start's both-directions discipline), so the doc comment's
claim that a seed gets no purge vars is true even when the tenant's own env
file sets them; and DefaultSocket/DefaultDBPath use filepath.Join, so a
CARLOS_DATA_DIR with the trailing slash an operator naturally types stops
yielding /data//host.db — two names for one database is how a route and
litestream's box-wide config come to disagree about what they replicate.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
10 files changed,
+277
−41
internal/activator/activator_test.go+6 −1internal/activator/backend_exec.go+11 −2internal/console/apiinstances.go+21 −23internal/console/apiinstances_test.go+55 −1internal/provision/provision.go+10 −2internal/provision/provision_test.go+8 −0internal/provision/reconcile.go+15 −0internal/provision/reconcile_test.go+45 −0internal/registry/registry.go+37 −12internal/registry/registry_test.go+69 −0
diff --git a/internal/activator/activator_test.go b/internal/activator/activator_test.go| index ff981a8f1..d14d19014 100644 |
| --- a/internal/activator/activator_test.go |
| +++ b/internal/activator/activator_test.go |
| @@ -2803,7 +2803,12 @@ func TestExecBackendSeedRunsDeclaredArgvWithDBPath(t *testing.T) { |
| if err := os.WriteFile(filepath.Join(b.DataDir, host+".config.env"), []byte(cfgEnv), 0o600); err != nil { |
| t.Fatal(err) |
| } |
| - ownEnv := "SHARED=instance\nCARLOS_ADMIN_TOKEN=real\n" |
| + // The tenant's own env file claims the purge channel. It is not the |
| + // tenant's to claim in either direction — those two names are |
| + // agent-owned (see withoutVars), and a seed told a control socket |
| + // exists at a path the agent never vouched for is exactly the leak the |
| + // spawn path's else-branch closes. |
| + ownEnv := "SHARED=instance\nCARLOS_ADMIN_TOKEN=real\nCARLOS_PURGE=header\nCARLOS_CONTROL_SOCKET=/tmp/tenant-chosen.sock\n" |
| if err := os.WriteFile(filepath.Join(b.DataDir, host+".env"), []byte(ownEnv), 0o600); err != nil { |
| t.Fatal(err) |
| } |
diff --git a/internal/activator/backend_exec.go b/internal/activator/backend_exec.go| index 517ff8369..87d27c613 100644 |
| --- a/internal/activator/backend_exec.go |
| +++ b/internal/activator/backend_exec.go |
| @@ -177,7 +177,9 @@ var restoreReplica = lease.Restore |
| // this was named for.) It runs under the same merged env layers the instance |
| // itself gets, so the credentials and config in your env file are there; it |
| // does NOT get the purge-channel vars, which describe a serving process's |
| -// cache and mean nothing to a one-shot init. A non-zero exit fails the wake: |
| +// cache and mean nothing to a one-shot init — and it does not get them even |
| +// if your own env file sets them, those two names being the agent's to |
| +// author. A non-zero exit fails the wake: |
| // a half-seeded instance must not serve. |
| func (b *ExecBackend) Seed(ctx context.Context, rt registry.Route) error { |
| host := rt.Host |
| @@ -212,7 +214,14 @@ func (b *ExecBackend) Seed(ctx context.Context, rt registry.Route) error { |
| defer cancel() |
| argv := append(seedArgs, "--db", dbPath) |
| cmd := exec.CommandContext(ctx, b.liveLink(host), argv...) |
| - cmd.Env = childEnv(fileEnv) |
| + // The purge-channel vars are agent-owned in BOTH directions here too |
| + // (Start's discipline, see withoutVars): fileEnv is tenant-supplied, so |
| + // without this a tenant's own env file could hand its seed a |
| + // CARLOS_CONTROL_SOCKET the agent never opened and a CARLOS_PURGE for a |
| + // channel that does not exist — and the seed has no serving cache to |
| + // purge in any case. This is what makes the contract above true rather |
| + // than merely intended. |
| + cmd.Env = childEnv(withoutVars(fileEnv, "CARLOS_CONTROL_SOCKET", "CARLOS_PURGE")) |
| // Like the instance child, the seed logs through the agent's own streams |
| // (journald under systemd) — and its stderr is ALSO teed into a bounded |
| // buffer, because the wake error this returns is the whole diagnosis an |
diff --git a/internal/console/apiinstances.go b/internal/console/apiinstances.go| index b05cae254..34ed311ca 100644 |
| --- a/internal/console/apiinstances.go |
| +++ b/internal/console/apiinstances.go |
| @@ -122,23 +122,23 @@ func (s *Server) instanceGate(w http.ResponseWriter, r *http.Request) (string, * |
| // instanceManifest reads the app's instances manifest and answers the |
| // caller itself when it cannot be had. ok=false means a response has |
| -// already been written; m=nil with ok=true means "no manifest, and the |
| -// caller has said it can carry on without one" (delete, which must outlive |
| -// a retired manifest). |
| +// already been written. |
| +// |
| +// CREATE is its only caller, and deliberately so: the manifest is policy |
| +// about what may be BROUGHT INTO existence (which domain, which pool, which |
| +// regions, which channel). Reads and deletes are about what already exists, |
| +// which no version of this manifest can retract. |
| // |
| // The two failures are kept apart on purpose: ABSENCE is an answer — the |
| // feature does not exist for this app — while a read failure is the bucket |
| // being unreachable. Reporting a hiccup as "instances are not enabled" |
| // would tell an app its feature had been switched off. |
| -func (s *Server) instanceManifest(ctx context.Context, w http.ResponseWriter, acct *Account, app string, absentOK bool) (*provision.Manifest, bool) { |
| +func (s *Server) instanceManifest(ctx context.Context, w http.ResponseWriter, acct *Account, app string) (*provision.Manifest, bool) { |
| m, err := provision.GetManifest(ctx, s.cfg.Releases, acct.Sqid, app) |
| switch { |
| case err == nil: |
| return m, true |
| case errors.Is(err, os.ErrNotExist): |
| - if absentOK { |
| - return nil, true |
| - } |
| // Membership (or binding) is already proven, so this may name its |
| // cause and the operator act that fixes it. |
| jsonError(w, http.StatusConflict, |
| @@ -218,7 +218,7 @@ func (s *Server) handleAPIInstanceCreate(w http.ResponseWriter, r *http.Request) |
| } |
| ctx, cancel := context.WithTimeout(r.Context(), instanceStoreTimeout) |
| defer cancel() |
| - m, ok := s.instanceManifest(ctx, w, acct, app, false) |
| + m, ok := s.instanceManifest(ctx, w, acct, app) |
| if !ok { |
| return |
| } |
| @@ -310,21 +310,19 @@ func (s *Server) handleAPIInstanceDelete(w http.ResponseWriter, r *http.Request) |
| host := r.PathValue("host") |
| ctx, cancel := context.WithTimeout(r.Context(), instanceStoreTimeout) |
| defer cancel() |
| - // Deletion must outlive a retired manifest: an app that turned |
| - // instances off still has records to clean up, and refusing to delete |
| - // them would strand routes nothing can retire. So an absent manifest is |
| - // tolerated here — but the host is still validated, by SHAPE alone when |
| - // there is no domain left to check a suffix against. There is no path |
| - // through this handler on which an unvalidated host reaches RecordKey. |
| - m, ok := s.instanceManifest(ctx, w, acct, app, true) |
| - if !ok { |
| - return |
| - } |
| - valid := provision.ValidHostShape(host) |
| - if m != nil { |
| - valid = provision.ValidHost(host, m.Domain) |
| - } |
| - if !valid { |
| + // SHAPE ONLY, always — and the manifest is not read here at all. The |
| + // domain-suffix rule is CREATE-time policy ("which hosts may this app |
| + // claim"), and deletion has to outlive every version of it: a retired |
| + // manifest, an app that moved domains, a record written under a domain |
| + // the manifest no longer names. Any of those checked against today's |
| + // manifest would refuse to delete records that exist, stranding |
| + // provisioned routes with nothing left able to retire them. What |
| + // deletion actually needs from validation is the traversal defence: a |
| + // {host} arrives percent-decoded, so "..%2f..%2finstances.json" reaches |
| + // here with separators in it and RecordKey would happily join it. |
| + // ValidHostShape is exactly that check, and there is no path through |
| + // this handler on which an unvalidated host reaches RecordKey. |
| + if !provision.ValidHostShape(host) { |
| jsonError(w, http.StatusBadRequest, "invalid host") |
| return |
| } |
diff --git a/internal/console/apiinstances_test.go b/internal/console/apiinstances_test.go| index edb5dfcbb..a7905133d 100644 |
| --- a/internal/console/apiinstances_test.go |
| +++ b/internal/console/apiinstances_test.go |
| @@ -482,6 +482,49 @@ func TestAPIInstanceDelete(t *testing.T) { |
| } |
| } |
| +// TestAPIInstanceDeleteAfterDomainChange: the suffix check is CREATE-time |
| +// policy — "which hosts may this app claim" — and deletion must outlive it. |
| +// An app that moves domains still has records under the old one, and they |
| +// are exactly the records that must be deletable: refusing them would strand |
| +// provisioned routes with nothing left that can retire them. Delete |
| +// validates SHAPE only, always, which is the whole traversal defence it |
| +// needs. |
| +func TestAPIInstanceDeleteAfterDomainChange(t *testing.T) { |
| + fx, token := instanceFixture(t, "code-inst-redomain") |
| + enableInstances(t, fx, "titogo", titoManifest()) |
| + u := instancesURL(fx, "bab", "titogo") |
| + if status, _, out := rawReq(t, http.MethodPost, u, token, |
| + `{"host":"acme.go.tito.io","region":"ie"}`); status != http.StatusCreated { |
| + t.Fatalf("create = %d %s", status, out) |
| + } |
| + |
| + // The app moves to a new domain: new instances land under it, and the |
| + // old host is now a host this manifest would never have allowed. |
| + moved := titoManifest() |
| + moved.Domain = "go.tito.dev" |
| + enableInstances(t, fx, "titogo", moved) |
| + if status, _, out := rawReq(t, http.MethodPost, u, token, |
| + `{"host":"acme.go.tito.io","region":"ie"}`); status != http.StatusBadRequest { |
| + t.Fatalf("create under the OLD domain = %d %s, want 400 — the suffix rule still governs creates", status, out) |
| + } |
| + |
| + if status, _, out := rawReq(t, http.MethodDelete, u+"/acme.go.tito.io", token, ""); status != http.StatusOK { |
| + t.Fatalf("delete of a record created under the old domain = %d %s, want 200", status, out) |
| + } |
| + if _, _, err := fx.rel.Get(context.Background(), |
| + provision.RecordKey("bab", "titogo", "acme.go.tito.io")); err == nil { |
| + t.Fatalf("the record survived its delete") |
| + } |
| + // Shape is still the door: a traversal host is refused whatever the |
| + // manifest says. |
| + if status, _, out := rawReq(t, http.MethodDelete, u+"/..%2f..%2finstances.json", token, ""); status != http.StatusBadRequest { |
| + t.Fatalf("traversal delete after a domain change = %d %s, want 400", status, out) |
| + } |
| + if _, _, err := fx.rel.Get(context.Background(), provision.ManifestKey("bab", "titogo")); err != nil { |
| + t.Fatalf("the manifest was destroyed by a traversal delete: %v", err) |
| + } |
| +} |
| + |
| // TestAPIInstanceTokenMintEndToEnd is the whole app-token story: a member |
| // mints one, the raw token is shown exactly once, and it then provisions on |
| // its own — and every mutation left an audit row naming its principal. |
| @@ -629,6 +672,16 @@ func (u unreadableStore) Get(context.Context, string) ([]byte, string, error) { |
| return nil, "", errors.New(brokenStoreErr) |
| } |
| +// undeletableStore is a bucket whose object deletes fail. Delete's own store |
| +// call is the only one it makes — it reads no manifest (the suffix rule is |
| +// create-time policy) — so this is what a broken bucket looks like from |
| +// there. |
| +type undeletableStore struct{ release.Store } |
| + |
| +func (u undeletableStore) Delete(context.Context, string) error { |
| + return errors.New(brokenStoreErr) |
| +} |
| + |
| // TestAPIInstanceStoreFailure: a bucket that cannot be read or written is a |
| // 502, and its error text — bucket names, request ids — never reaches the |
| // caller. A read failure is NOT reported as "instances are not enabled". |
| @@ -654,9 +707,10 @@ func TestAPIInstanceStoreFailure(t *testing.T) { |
| if strings.Contains(out, "instances enable") { |
| t.Fatalf("a read failure was reported as the feature being disabled: %s", out) |
| } |
| + fx.srv.cfg.Releases = undeletableStore{fx.rel} |
| status, _, out = rawReq(t, http.MethodDelete, u+"/acme.go.tito.io", token, "") |
| if status != http.StatusBadGateway { |
| - t.Fatalf("delete against an unreadable bucket = %d %s, want 502", status, out) |
| + t.Fatalf("delete against a bucket that cannot delete = %d %s, want 502", status, out) |
| } |
| if strings.Contains(out, "carlos-prod-secret-bucket") { |
| t.Fatalf("delete refusal leaks the store's error text: %s", out) |
diff --git a/internal/provision/provision.go b/internal/provision/provision.go| index bb24da438..2c3cb0838 100644 |
| --- a/internal/provision/provision.go |
| +++ b/internal/provision/provision.go |
| @@ -12,6 +12,7 @@ import ( |
| "errors" |
| "fmt" |
| "os" |
| + "path/filepath" |
| "regexp" |
| "sort" |
| "strings" |
| @@ -295,5 +296,12 @@ const SocketDir = "/run/carlos" |
| // and the reconciler's mint (Task 3) both build on — the "honest shared |
| // core" design §9 names, so a hand-added route and a provisioned one land |
| // on identical path shapes. |
| -func DefaultSocket(host string) string { return SocketDir + "/" + host + ".sock" } |
| -func DefaultDBPath(dataDir, host string) string { return dataDir + "/" + host + ".db" } |
| +// |
| +// filepath.Join, not concatenation: dataDir is CARLOS_DATA_DIR, an operator- |
| +// typed env var, and "/data/" is an ordinary way to write a directory. |
| +// Concatenating gave "/data//h.db" — the same file under a second name, and |
| +// two names for one database is how a route and litestream's box-wide config |
| +// come to disagree about what they are replicating. (The literals runAdd |
| +// used before this helper existed were Join'd; this keeps that.) |
| +func DefaultSocket(host string) string { return filepath.Join(SocketDir, host+".sock") } |
| +func DefaultDBPath(dataDir, host string) string { return filepath.Join(dataDir, host+".db") } |
diff --git a/internal/provision/provision_test.go b/internal/provision/provision_test.go| index 902b1abd9..30f94c15b 100644 |
| --- a/internal/provision/provision_test.go |
| +++ b/internal/provision/provision_test.go |
| @@ -326,4 +326,12 @@ func TestDefaults(t *testing.T) { |
| if got := DefaultDBPath("/data", "h"); got != "/data/h.db" { |
| t.Errorf("DefaultDBPath(%q, %q) = %q, want /data/h.db", "/data", "h", got) |
| } |
| + // CARLOS_DATA_DIR comes from an env var an operator types; a trailing |
| + // slash is the ordinary way to type a directory and must not produce |
| + // "/data//h.db" — a second path for the same database, which is how a |
| + // route and litestream's box-wide config come to disagree about which |
| + // file they are replicating. |
| + if got := DefaultDBPath("/data/", "h"); got != "/data/h.db" { |
| + t.Errorf("DefaultDBPath(%q, %q) = %q, want /data/h.db", "/data/", "h", got) |
| + } |
| } |
diff --git a/internal/provision/reconcile.go b/internal/provision/reconcile.go| index daa83bd3d..0dcf4711a 100644 |
| --- a/internal/provision/reconcile.go |
| +++ b/internal/provision/reconcile.go |
| @@ -107,6 +107,21 @@ func Reconcile(ctx context.Context, s release.Store, reg *registry.Registry, cfg |
| protect[host] = true |
| continue |
| } |
| + // The record IS the authorization, and this pass turns its host into |
| + // a socket path and a database path (DefaultSocket/DefaultDBPath) |
| + // and a registry primary key. The console's create door enforces |
| + // this same shape — but a door is not a guarantee about what is |
| + // already in the bucket: anything with bucket write access, and any |
| + // record written by a different (older, or future) writer, arrives |
| + // here unexamined. Both doors hold the invariant, neither trusts |
| + // the other. Protected like a corrupted body, and for the same |
| + // reason: a record that cannot be used is not a record that was |
| + // retired. |
| + if !ValidHostShape(rec.Host) { |
| + log.Printf("provision: skipping record %s: host %q is not a hostname", key, rec.Host) |
| + protect[host] = true |
| + continue |
| + } |
| if rec.Region != cfg.Region || !poolServed(cfg.Pools, rec.Pool) { |
| continue // another agent's jurisdiction, not this pass's business |
| } |
diff --git a/internal/provision/reconcile_test.go b/internal/provision/reconcile_test.go| index 088bc4796..3b2646e4e 100644 |
| --- a/internal/provision/reconcile_test.go |
| +++ b/internal/provision/reconcile_test.go |
| @@ -391,6 +391,51 @@ func TestReconcileSkipsMalformedRecord(t *testing.T) { |
| } |
| } |
| +// The record is the authorization, and this pass builds a socket path and a |
| +// database path out of it — so the host-shape invariant the console's create |
| +// door enforces has to hold at THIS door too, not by trusting the other one. |
| +// A record written straight into the bucket (or by a future/older writer) |
| +// whose host is not a hostname must not become a route: nothing downstream |
| +// re-checks it. Skipped loudly, and protected from removal for the same |
| +// reason a corrupted body is — an unusable record is not a retired one. |
| +func TestReconcileSkipsRecordWithInvalidHostShape(t *testing.T) { |
| + s, sqid := setup(t) |
| + const bad = "acme$evil.go.tito.io" |
| + body, err := json.Marshal(Record{Schema: 1, Host: bad, Slug: "acme$evil", App: "tito", |
| + Account: sqid, Pool: "tito", Region: "ie", Channel: "stable", CreatedAt: time.Now()}) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + // Key and body agree — the only thing wrong here is the host itself. |
| + if _, err := s.Put(ctx, RecordKey(sqid, "tito", bad), body, ""); err != nil { |
| + t.Fatal(err) |
| + } |
| + if err := CreateRecord(ctx, s, acmeRecord(sqid)); err != nil { |
| + t.Fatal(err) |
| + } |
| + reg := regWith(t) |
| + cfg := ReconcileConfig{Region: "ie", Pools: []string{"tito"}, DataDir: "/data"} |
| + if err := Reconcile(ctx, s, reg, cfg); err != nil { |
| + t.Fatal(err) |
| + } |
| + if rt, err := reg.Get(bad); err != nil || rt != nil { |
| + t.Errorf("minted a route from a record whose host is not a hostname: %+v %v", rt, err) |
| + } |
| + // The good neighbour in the same pass is untouched by the skip. |
| + mustGet(t, reg, "acme.go.tito.io") |
| + |
| + // And a route that somehow already carries that host is not treated as |
| + // retired: the record is there, it just cannot be used. |
| + if err := reg.Upsert(registry.Route{Host: bad, Kind: "instance", App: "tito", |
| + Status: "ready", Provisioned: true, Region: "ie", Pool: "tito", Account: sqid}); err != nil { |
| + t.Fatal(err) |
| + } |
| + if err := Reconcile(ctx, s, reg, cfg); err != nil { |
| + t.Fatal(err) |
| + } |
| + mustGet(t, reg, bad) |
| +} |
| + |
| // CRITICAL regression (code review finding): an existing route's record |
| // going unparseable — not gone, just unreadable this pass — must never read |
| // as "the bucket retired it". Before the fix, json.Unmarshal failure and |
diff --git a/internal/registry/registry.go b/internal/registry/registry.go| index af45a6fab..3815274a4 100644 |
| --- a/internal/registry/registry.go |
| +++ b/internal/registry/registry.go |
| @@ -94,11 +94,25 @@ type Route struct { |
| // upstream" to three consumers (config sync, upstream picking, apex |
| // candidacy) and a provisioned instance is the opposite: it owns a |
| // process. |
| + // |
| + // Because it carries AUTHORITY, false is not a value a full-row Upsert |
| + // may impose: `carlos add` on a provisioned host would otherwise revoke |
| + // the pass's removal right permanently (the environment precedent, with |
| + // worse stakes — see Upsert). Upsert preserves an existing true; the |
| + // only clear is the row's own deletion, which is what the removal phase |
| + // does anyway. |
| Provisioned bool |
| // Seed is the app's first-boot init argv as a JSON array (e.g. |
| // ["seed"]), materialized from the app's instances.json by the |
| // provision pass; "" = no seed. Run by the activator only on a wake |
| // where restore-on-miss found no replica (backend_exec.go). |
| + // |
| + // Same preservation rule as Provisioned and for the same reason: a |
| + // full-row Upsert from a caller that knows nothing about seeding must |
| + // not erase one, so Upsert keeps a non-empty existing value and SetSeed |
| + // (the provision pass's narrow converge) is the only door that clears |
| + // it — which it must be able to do, for a manifest whose seed was |
| + // removed. |
| Seed string |
| // DNSStatus is the last DNS health verdict for this host: "pointing" |
| // (resolves to a deployment edge IP), "elsewhere", or "" — unknown, the |
| @@ -431,17 +445,27 @@ func (r *Registry) Delete(host string) error { |
| return err |
| } |
| -// Upsert inserts or updates a route by host. One column is deliberately not |
| -// a plain overwrite: `environment` keeps the existing row's value when the |
| -// incoming Route carries the zero value (2026-08-02 review, IMPORTANT 3). |
| -// Every full-row Upsert caller — `carlos add` and the domains reconciler's |
| -// two mint paths — builds its Route without knowing anything about |
| -// environment bindings, so a plain excluded.environment silently unbound a |
| -// route on any re-add or re-mint, and the next config sync then rendered the |
| -// default bundle over that route's env file and restarted its unit. An |
| -// explicit non-empty value still wins, and clearing goes through |
| -// SetEnvironment (what `carlos route -environment -` calls), which is the |
| -// only place a clear is ever intended. |
| +// Upsert inserts or updates a route by host. Three columns are deliberately |
| +// not plain overwrites — `environment`, `provisioned` and `seed` all keep |
| +// the existing row's value when the incoming Route carries the zero one. |
| +// |
| +// The reason is the same for all three, and it is structural: a full-row |
| +// Upsert caller (`carlos add`, the domains reconciler's two mint paths, the |
| +// provision pass's mint) builds its Route from what IT knows, and none of |
| +// them knows about the columns another feature owns. `environment` was the |
| +// first to be caught (2026-08-02 review, IMPORTANT 3): a plain |
| +// excluded.environment silently unbound a route on any re-add or re-mint, |
| +// and the next config sync rendered the default bundle over its env file and |
| +// restarted its unit. `provisioned` and `seed` are the same trap with worse |
| +// stakes — `carlos add` on a provisioned host would write provisioned=0, |
| +// and the reconcile pass would then lose removal authority over that route |
| +// FOREVER: deleting the bucket record could no longer retire it, and only a |
| +// human noticing would. |
| +// |
| +// An explicit non-zero value still wins in every case; clearing keeps |
| +// exactly one door per column, none of which goes through Upsert: |
| +// SetEnvironment (`carlos route -environment -`), SetSeed, and for |
| +// `provisioned` the row's own deletion. |
| func (r *Registry) Upsert(t Route) error { |
| if t.CreatedAt == 0 { |
| t.CreatedAt = time.Now().Unix() |
| @@ -474,7 +498,8 @@ ON CONFLICT(host) DO UPDATE SET |
| dns_checked_at=excluded.dns_checked_at, |
| cert_expires_at=excluded.cert_expires_at, account=excluded.account, |
| environment=CASE WHEN excluded.environment = '' THEN routes.environment ELSE excluded.environment END, |
| - provisioned=excluded.provisioned, seed=excluded.seed`, |
| + provisioned=CASE WHEN excluded.provisioned = 0 THEN routes.provisioned ELSE excluded.provisioned END, |
| + seed=CASE WHEN excluded.seed = '' THEN routes.seed ELSE excluded.seed END`, |
| t.Host, t.Slug, t.Kind, t.Socket, t.Addr, t.Status, t.LastError, |
| t.DBPath, t.Version, t.PrevVersion, t.App, t.Pool, t.Region, t.Channel, |
| b2i(t.Hibernate), b2i(t.Canary), t.Replication, t.NextDue, t.Backing, |
diff --git a/internal/registry/registry_test.go b/internal/registry/registry_test.go| index 9f43b3289..b7ededd4e 100644 |
| --- a/internal/registry/registry_test.go |
| +++ b/internal/registry/registry_test.go |
| @@ -768,6 +768,75 @@ func TestProvisionedSeedRoundTrip(t *testing.T) { |
| } |
| } |
| +// Upsert must not let a caller that knows nothing about provisioning revoke |
| +// the reconcile pass's removal authority. `carlos add` builds a full Route |
| +// with provisioned=false and seed="" — run against a host an instance |
| +// record already minted, a plain overwrite would strip the flag the pass |
| +// removes by (leaving a route no bucket deletion can ever retire) and erase |
| +// the first-boot argv. The zero value preserves, a real value wins, and |
| +// clearing keeps exactly one door each: row deletion, and SetSeed. |
| +func TestUpsertPreservesProvisionedAndSeed(t *testing.T) { |
| + r := openTestRegistry(t) |
| + const host = "acme.go.tito.io" |
| + if err := r.Upsert(Route{Host: host, Kind: "instance", App: "titogo", |
| + Provisioned: true, Seed: `["seed"]`}); err != nil { |
| + t.Fatal(err) |
| + } |
| + // The runAdd shape: every column set from flags, nothing about |
| + // provisioning in it at all. |
| + if err := r.Upsert(Route{Host: host, Kind: "instance", App: "titogo", |
| + Socket: "/run/carlos/" + host + ".sock", DBPath: "/data/" + host + ".db"}); err != nil { |
| + t.Fatal(err) |
| + } |
| + got, err := r.Get(host) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + if !got.Provisioned { |
| + t.Errorf("a re-add revoked the pass's removal authority: %+v", got) |
| + } |
| + if got.Seed != `["seed"]` { |
| + t.Errorf("a re-add erased the first-boot argv: seed = %q", got.Seed) |
| + } |
| + if got.Socket != "/run/carlos/"+host+".sock" { |
| + t.Errorf("the re-add's own columns did not land: %+v", got) |
| + } |
| + |
| + // A non-zero incoming value still wins — this is preservation, not a |
| + // column that can never be written. |
| + if err := r.Upsert(Route{Host: host, Kind: "instance", App: "titogo", |
| + Provisioned: true, Seed: `["seed","-v"]`}); err != nil { |
| + t.Fatal(err) |
| + } |
| + if got, _ = r.Get(host); got.Seed != `["seed","-v"]` { |
| + t.Errorf("an explicit seed lost to the existing value: %q", got.Seed) |
| + } |
| + |
| + // The narrow door still clears: the reconcile pass converging a |
| + // manifest whose seed was removed must be able to empty it. |
| + if err := r.SetSeed(host, ""); err != nil { |
| + t.Fatal(err) |
| + } |
| + if got, _ = r.Get(host); got.Seed != "" { |
| + t.Errorf("SetSeed(\"\") did not clear the seed: %q", got.Seed) |
| + } |
| + if !got.Provisioned { |
| + t.Errorf("SetSeed touched provisioned: %+v", got) |
| + } |
| + |
| + // And provisioned's only clear is the row going away — which is what |
| + // the removal phase does. |
| + if err := r.Delete(host); err != nil { |
| + t.Fatal(err) |
| + } |
| + if err := r.Upsert(Route{Host: host, Kind: "instance", App: "titogo"}); err != nil { |
| + t.Fatal(err) |
| + } |
| + if got, _ = r.Get(host); got.Provisioned { |
| + t.Errorf("provisioned survived the row's deletion: %+v", got) |
| + } |
| +} |
| + |
| // Opening a DB created before this change (no provisioned/seed columns) must |
| // migrate cleanly — the ALTER list is additive, duplicate-column errors are |
| // swallowed, and pre-existing rows default provisioned=false seed="" rather |