3e24809fe · carlos/platform

diff --git a/internal/host/buildstate.go b/internal/host/buildstate.go

index f81f46d92..a1c56d2fa 100644

--- a/internal/host/buildstate.go

+++ b/internal/host/buildstate.go

@@ -519,8 +519,15 @@ func (b *buildStates) beforeExec(ctx context.Context, host, build string) error

}

// The held target is revalidated against the state read under the lease

// (Astra round 1 finding 1): a wake that decided on a stale copy is

- // refused here, before anything is written or run.

- rt, _ := b.reg.Get(host)

+ // refused here, before anything is written or run. A route that does

+ // not read refuses the exec before any write (Astra r4 finding 3): the

+ // tenant check below needs it, and an unread tenant must not merge a

+ // hostname-keyed file into the state. A route that is absent (nil, no

+ // error) is read as before.

+ rt, err := b.reg.Get(host)

+ if err != nil {

+ return fmt.Errorf("read the route of %s: %w", host, err)

+ }

// A file root wrote for another tenant of this hostname carries nothing

// into this tenant's state (ruling R26). An untagged one is read as the

// route's own, as before.

@@ -661,15 +668,17 @@ func (b *buildStates) heldTarget(host string) activator.HeldWake {

// heldWakeInput reads DecideHeldWake's input for rt. state, when set, is

// the build state just read under the lease (beforeExec's revalidation), in

// place of the registry copy. A consumed file or refusal record that does

-// not read is no word from root (nil), and so is a consumed file not

-// decided for rt's tenant (Consumed.DecidedFor); a copy that does not parse

-// is unread.

+// not read is no word from root (nil), and so is a consumed file tagged for

+// another tenant (Consumed.ForeignTo); a copy that does not parse is unread.

func (b *buildStates) heldWakeInput(rt registry.Route, state *release.BuildState) release.HeldWakeInput {

in := release.HeldWakeInput{Settings: rt.UpdateSettings}

- // Only a decision root made for this route's tenant counts (ruling R26):

- // another tenant's file, or an untagged one from before ruling R22, is

- // no word from root, so a wake that could be held holds and asks root.

- if c, _ := release.ReadConsumed(b.optDir, rt.Host); c.DecidedFor(rt) {

+ // Another tenant's file is no word from root for this route (ruling

+ // R26), so a wake that could be held holds and asks root. An untagged

+ // file, from before ruling R22, is read as before, as every other read

+ // of it is (ruling R28): ignoring its failed decision would let a wake

+ // whose target is already live run a build root reports failed, as

+ // awaitRootDecision does not wait when the live link names the target.

+ if c, _ := release.ReadConsumed(b.optDir, rt.Host); !c.ForeignTo(rt) {

in.Consumed = c

}

in.Refusal, _ = release.ReadRefusal(b.bakeDir, rt.Host)

diff --git a/internal/host/buildstate_astra_test.go b/internal/host/buildstate_astra_test.go

index 5b5416890..2750dca19 100644

--- a/internal/host/buildstate_astra_test.go

+++ b/internal/host/buildstate_astra_test.go

@@ -83,19 +83,34 @@ func TestAMajorHoldIsNotBypassedBeforeRootDecidesForTheSettings(t *testing.T) {

// build state read and absent, T installed, and root's decision that T

// failed. The installed-build fallback is failure-checked like

// last_executed: held on T and failed, never "not held" (which ran T).

+//

+// It runs twice: with root's file tagged for the route's tenant, as root

+// writes it now, and with the file left untagged, as a settings-4 build

+// wrote it (ruling R28: an untagged file is read as before task 8).

func TestAFailedInstalledBuildWithNoBuildStateIsHeldFailed(t *testing.T) {

- b, _, reg := testBuildStates(t)

- installLive(t, b, "T")

- setSettings(t, reg, release.UpdateSettings{Updates: release.UpdatesManual, Channel: "stable"})

- setCopy(t, reg, release.BuildStateCopy{Known: true, Replica: true})

- decided(t, b, release.Consumed{Reason: string(release.ReasonFailed), Detail: "T"})

- if h := b.heldTarget(astraHost); !h.Held || !h.Failed || h.Target != "T" || h.Staged != nil {

- t.Fatalf("heldTarget = %+v; want T held and failed", h)

- }

- // Not failed: held on the installed build, as root's Target holds it.

- decided(t, b, release.Consumed{Reason: string(release.ReasonManual), Desired: "T", Detail: "T"})

- if h := b.heldTarget(astraHost); !h.Held || h.Failed || h.Target != "T" {

- t.Fatalf("heldTarget = %+v; want held on the installed T", h)

+ for _, untagged := range []bool{false, true} {

+ b, _, reg := testBuildStates(t)

+ installLive(t, b, "T")

+ setSettings(t, reg, release.UpdateSettings{Updates: release.UpdatesManual, Channel: "stable"})

+ setCopy(t, reg, release.BuildStateCopy{Known: true, Replica: true})

+ write := func(c release.Consumed) {

+ if !untagged {

+ decided(t, b, c)

+ return

+ }

+ rt, _ := reg.Get(astraHost)

+ c.Settings = release.SettingsDigest(rt.UpdateSettings)

+ writeConsumedAsIs(t, b, c)

+ }

+ write(release.Consumed{Reason: string(release.ReasonFailed), Detail: "T"})

+ if h := b.heldTarget(astraHost); !h.Held || !h.Failed || h.Target != "T" || h.Staged != nil {

+ t.Fatalf("untagged %v: heldTarget = %+v; want T held and failed", untagged, h)

+ }

+ // Not failed: held on the installed build, as root's Target holds it.

+ write(release.Consumed{Reason: string(release.ReasonManual), Desired: "T", Detail: "T"})

+ if h := b.heldTarget(astraHost); !h.Held || h.Failed || h.Target != "T" {

+ t.Fatalf("untagged %v: heldTarget = %+v; want held on the installed T", untagged, h)

+ }

}

diff --git a/internal/host/buildstate_test.go b/internal/host/buildstate_test.go

index 5eaeab15f..d1fbbe225 100644

--- a/internal/host/buildstate_test.go

+++ b/internal/host/buildstate_test.go

@@ -663,8 +663,7 @@ func TestClaimsOnRegistersOnlyTheClaimBucket(t *testing.T) {

// writeConsumed writes root's consumed file for the test host. An untagged

// c is tagged with the route's tenant, as root writes every file since

-// ruling R22; the held wake check reads no other (ruling R26). Tests of an

-// untagged or foreign file use writeConsumedAsIs.

+// ruling R22. Tests of an untagged or foreign file use writeConsumedAsIs.

func writeConsumed(t *testing.T, b *buildStates, c release.Consumed) {

t.Helper()

c.Host = "a.example.com"

diff --git a/internal/host/tenantchange_test.go b/internal/host/tenantchange_test.go

index edc505e79..8d2047088 100644

--- a/internal/host/tenantchange_test.go

+++ b/internal/host/tenantchange_test.go

@@ -2,8 +2,13 @@ package host

import (

"context"

+ "net/http"

+ "sync/atomic"

"testing"

+ "time"

+ "github.com/carlosframework/platform/internal/activator"

+ "github.com/carlosframework/platform/internal/registry"

"github.com/carlosframework/platform/internal/release"

)

@@ -11,8 +16,8 @@ import (

// keyed by hostname, and a hostname can move to another tenant on one box.

// A file tagged for another tenant is absent to the pre-exec write and to a

// move's carry; an untagged file is read there as today. The held wake check

-// trusts only a file decided for the route (DecidedFor): with a foreign or an

-// untagged file it holds and asks root.

+// reads the file by the same rule (ruling R28): with a foreign file it holds

+// and asks root, and an untagged file is read as today.

// foreignRecord is tenant A's (cdf/blog) record for the test host: a

// resolved request, a failed completion watermark and a manual decision.

@@ -61,6 +66,33 @@ func TestBeforeExecIgnoresAnotherTenantsFile(t *testing.T) {

}

+// Astra r4 finding 3: the pre-exec write learns the route's tenant from the

+// registry. A read that fails is no answer: B owns the host, A's file is

+// still on disk, and an unread route cannot tell them apart. beforeExec

+// returns the error before any build-state write, so the durable object is

+// left exactly as it was. (A route that is absent, read without error, is

+// today's behaviour and is not this case.)

+func TestBeforeExecFailsClosedOnARegistryReadError(t *testing.T) {

+ b, s, reg := testBuildStates(t)

+ ctx := context.Background()

+ if err := put(t, b, ctx, astraHost, release.BuildState{LastExecuted: "B1", Completed: 1}); err != nil {

+ t.Fatal(err)

+ }

+ key := release.BuildStateKey("inst/" + astraHost)

+ etag := mustEtag(t, s, key)

+ writeConsumedAsIs(t, b, foreignRecord())

+ if err := reg.Close(); err != nil { // every Get now fails

+ t.Fatal(err)

+ }

+ if err := b.beforeExec(ctx, astraHost, "v1"); err == nil {

+ t.Fatal("beforeExec with an unreadable route returned nil")

+ }

+ if got := mustEtag(t, s, key); got != etag {

+ st, _, _ := getState(ctx, s, key, astraHost)

+ t.Fatalf("the durable build state was written with the route unread: %+v (resolved %+v)", st, st.Resolved)

+ }

+}

+

func TestACarryIgnoresAnotherTenantsFile(t *testing.T) {

b, s, reg := testBuildStates(t)

ctx := context.Background()

@@ -105,7 +137,6 @@ func TestTheHeldWakeTrustsOnlyAFileDecidedForTheRoute(t *testing.T) {

for _, tc := range []struct{ name, account, app string }{

{"another tenant's file", "cdf", "blog"},

{"another app's file on the same account", release.OpsAccount, "blog"},

- {"an untagged file", "", ""},

} {

c := record

c.Account, c.App = tc.account, tc.app

@@ -115,11 +146,71 @@ func TestTheHeldWakeTrustsOnlyAFileDecidedForTheRoute(t *testing.T) {

}

- // The route's own file is root's word: the request's T.

- c := record

- c.Account, c.App = release.OpsAccount, "km"

- writeConsumedAsIs(t, b, c)

- if h := b.heldTarget(astraHost); !h.Held || h.AskRoot || h.Target != "v4" {

- t.Fatalf("the route's own file: heldTarget = %+v; want the request's v4", h)

+ // The route's own file is root's word: the request's T. An untagged file

+ // is read as before task 8 (ruling R28), as the route's own.

+ for _, tc := range []struct{ name, account, app string }{

+ {"the route's own file", release.OpsAccount, "km"},

+ {"an untagged file", "", ""},

+ } {

+ c := record

+ c.Account, c.App = tc.account, tc.app

+ writeConsumedAsIs(t, b, c)

+ if h := b.heldTarget(astraHost); !h.Held || h.AskRoot || h.Target != "v4" {

+ t.Fatalf("%s: heldTarget = %+v; want the request's v4", tc.name, h)

+ }

+}

+

+// identityRecorder is plannedBackend with a live link it can capture: it

+// counts the captures and answers with T's identity, so a wake that gets as

+// far as running the live link records it.

+type identityRecorder struct {

+ *plannedBackend

+ captured atomic.Int32

+}

+

+func (r *identityRecorder) LiveIdentity(context.Context, registry.Route) (release.StagedIdentity, error) {

+ r.captured.Add(1)

+ return release.StagedIdentity{Version: r.live}, nil

+}

+

+// Astra r4 finding 1 (ruling R28), through the real wake: a same-tenant

+// manual instance with T installed, its build state read and absent, and an

+// UNTAGGED consumed file (a settings-4 build's) recording that T failed.

+// There is no refusal record and no planner. The activator's held wake check

+// must refuse the wake as failed: it never captures the live link, never

+// records T, never runs anything.

+func TestAnUntaggedFailedDecisionRefusesTheWake(t *testing.T) {

+ b, s, reg := testBuildStates(t)

+ b.prefix = func(h string) string { return activator.HibernatePrefix("", h) }

+ installLive(t, b, "T")

+ setSettings(t, reg, release.UpdateSettings{Updates: release.UpdatesManual, Channel: "stable"})

+ setCopy(t, reg, release.BuildStateCopy{Known: true, Replica: true})

+ rt, _ := reg.Get(astraHost)

+ writeConsumedAsIs(t, b, release.Consumed{Reason: string(release.ReasonFailed), Detail: "T",

+ Settings: release.SettingsDigest(rt.UpdateSettings)})

+ if r, _ := release.ReadRefusal(b.bakeDir, astraHost); r != nil {

+ t.Fatalf("a refusal record exists: %+v", r)

+ }

+

+ be := &identityRecorder{plannedBackend: &plannedBackend{live: "T"}}

+ svc := activator.New(activator.Config{Bucket: "hib", Idle: time.Hour}, be, reg)

+ svc.OnGen = func(activator.GenEvent) {}

+ svc.BeforeExec = b.beforeExec

+ svc.HeldTarget = b.heldTarget

+ t.Cleanup(func() { svc.Shutdown(context.Background()) })

+

+ rec := serve(t, svc, reg)

+ if rec.Code == http.StatusOK {

+ t.Fatalf("the wake ran T, which root reports failed: %q", rec.Body.String())

+ }

+ if n := be.captured.Load(); n != 0 {

+ t.Fatalf("the wake captured the live link %d times; want it refused before", n)

+ }

+ if got := be.builds(); len(got) != 0 {

+ t.Fatalf("generations %v, want none", got)

+ }

+ if st := durable(t, b, s); st != nil {

+ t.Fatalf("durable state %+v; want none (T never recorded)", st)

}

diff --git a/internal/registry/registry.go b/internal/registry/registry.go

index 3867c14ab..3dafe927e 100644

--- a/internal/registry/registry.go

+++ b/internal/registry/registry.go

@@ -22,6 +22,8 @@ import (

"time"

"modernc.org/sqlite"

+

+ "github.com/carlosframework/platform/internal/controlname"

)

type Registry struct {

@@ -1085,17 +1087,51 @@ func (r *Registry) Delete(host string) error {

// Route53 latency records — a real incident this repo caught in review before

// it shipped, not a hypothetical.

// `update_settings` and `build_state` keep an existing value only while the

-// row's tenant (account, app) stays the same. An Upsert that moves the

-// hostname to another tenant takes the incoming values, empty included:

+// row's tenant (effective account, app) stays the same. An Upsert that moves

+// the hostname to another tenant takes the incoming values, empty included:

// one tenant's manual or major hold, or its build state, is never another

-// tenant's (settings-5 task 8). SQLite evaluates every SET expression

-// against the row as it was, so `routes.account` and `routes.app` there are

-// still the old tenant's.

+// tenant's (settings-5 task 8). The account compared is the effective one

+// (EffectiveAccount, as release.RouteAccount reads it), so a legacy row's

+// empty account re-added as the ops account is the same tenant and keeps

+// both (Astra r4 finding 2). SQLite evaluates every SET expression against

+// the row as it was, so `routes.account` and `routes.app` there are still

+// the old tenant's.

// `good_version`, `good_digest`, `good_demand` and `serving_version`

// (zero-downtime spec §8.5) are absent from the ON CONFLICT clause on

// purpose: an existing row keeps them whatever the incoming Route carries.

// They have one writer each (SetGoodBuild, SetServingVersion), and no

// full-row caller knows what an instance last served.

+// opsAccount is the ops account's sqid: account #1, pinned by migrate. It is

+// release.OpsAccount (release cannot be imported here; a test in release

+// pins the two together through EffectiveAccount).

+const opsAccount = "bab"

+

+// EffectiveAccount is the account a route's stored account names: itself

+// when it is a valid account sqid (controlname.ValidAccount), else the ops

+// account. It is release.RouteAccount on the account alone, which root and

+// the agent use for a route's tenant; Upsert compares tenants by it.

+func EffectiveAccount(account string) string {

+ if controlname.ValidAccount(account) {

+ return account

+ }

+ return opsAccount

+}

+

+// effectiveAccountSQL is EffectiveAccount as a SQLite expression over col.

+// controlname.ValidAccount is ^[a-z]{3,16}$: three to sixteen characters,

+// none outside a-z. TestEffectiveAccountSQLIsEffectiveAccount pins the two

+// together.

+func effectiveAccountSQL(col string) string {

+ return `(CASE WHEN length(` + col + `) BETWEEN 3 AND 16 AND ` + col + ` NOT GLOB '*[^a-z]*' THEN ` + col +

+ ` ELSE '` + opsAccount + `' END)`

+}

+

+// tenantChangedSQL is true in Upsert's ON CONFLICT clause when the incoming

+// row names another tenant than the stored one: another effective account

+// or another app.

+var tenantChangedSQL = effectiveAccountSQL("routes.account") + ` != ` + effectiveAccountSQL("excluded.account") +

+ ` OR routes.app != excluded.app`

+

func (r *Registry) Upsert(t Route) error {

if r.remote != nil {

_, err := r.remote.call(nil, remoteRequest{Method: "Upsert", TValue: t}, nil)

@@ -1166,9 +1202,9 @@ ON CONFLICT(host) DO UPDATE SET

args=CASE WHEN excluded.args = '' THEN routes.args ELSE excluded.args END,

idle_seconds=CASE WHEN excluded.idle_seconds = 0 THEN routes.idle_seconds ELSE excluded.idle_seconds END,

max_run_seconds=CASE WHEN excluded.max_run_seconds = 0 THEN routes.max_run_seconds ELSE excluded.max_run_seconds END,

- update_settings=CASE WHEN routes.account != excluded.account OR routes.app != excluded.app THEN excluded.update_settings

+ update_settings=CASE WHEN `+tenantChangedSQL+` THEN excluded.update_settings

WHEN excluded.update_settings = '' THEN routes.update_settings ELSE excluded.update_settings END,

- build_state=CASE WHEN routes.account != excluded.account OR routes.app != excluded.app THEN excluded.build_state

+ build_state=CASE WHEN `+tenantChangedSQL+` THEN excluded.build_state

WHEN excluded.build_state = '' THEN routes.build_state ELSE excluded.build_state END

WHERE routes.claim_incarnation=excluded.claim_incarnation AND routes.claim_stamp=excluded.claim_stamp

AND routes.claim_owner=excluded.claim_owner AND routes.claim_row_id=excluded.claim_row_id`,

diff --git a/internal/registry/updatesettings_test.go b/internal/registry/updatesettings_test.go

index c164f368d..17fd5ad82 100644

--- a/internal/registry/updatesettings_test.go

+++ b/internal/registry/updatesettings_test.go

@@ -103,6 +103,67 @@ func TestATenantChangeUpsertTakesTheNewTenantsSettings(t *testing.T) {

}

+// Astra r4 finding 2: the tenant is the EFFECTIVE account, as

+// release.RouteAccount reads it (an empty or invalid account is the ops

+// account, "bab"). A legacy row with account "" re-added as "bab" (or the

+// reverse, or an invalid spelling) is the same tenant, so its columns are

+// kept as today. A genuine account change with empty incoming columns

+// clears both.

+func TestAnEquivalentAccountSpellingIsTheSameTenant(t *testing.T) {

+ reg, err := Open(filepath.Join(t.TempDir(), "r.db"))

+ if err != nil {

+ t.Fatal(err)

+ }

+ const host = "a.example.com"

+ settings := `{"updates":"manual","channel":"stable"}`

+ state := `{"known":true,"replica":true}`

+ for _, tc := range []struct{ from, to string }{

+ {"", "bab"}, {"bab", ""}, {"", ""}, {"NOT-VALID", "bab"}, {"bab", "x"},

+ } {

+ if err := reg.Upsert(Route{Host: host, Kind: "instance", Account: tc.from, App: "km", UpdateSettings: settings, BuildState: state}); err != nil {

+ t.Fatal(err)

+ }

+ if err := reg.Upsert(Route{Host: host, Kind: "instance", Account: tc.to, App: "km"}); err != nil {

+ t.Fatal(err)

+ }

+ if rt, _ := reg.Get(host); rt.Account != tc.to || rt.UpdateSettings != settings || rt.BuildState != state {

+ t.Fatalf("%q -> %q (one effective account): account %q, settings %q, build state %q; want both columns kept",

+ tc.from, tc.to, rt.Account, rt.UpdateSettings, rt.BuildState)

+ }

+ for _, tc := range []struct{ from, to string }{{"", "bes"}, {"bes", ""}, {"bes", "cdf"}} {

+ if err := reg.Upsert(Route{Host: host, Kind: "instance", Account: tc.from, App: "km", UpdateSettings: settings, BuildState: state}); err != nil {

+ t.Fatal(err)

+ }

+ if err := reg.Upsert(Route{Host: host, Kind: "instance", Account: tc.to, App: "km"}); err != nil {

+ t.Fatal(err)

+ }

+ if rt, _ := reg.Get(host); rt.UpdateSettings != "" || rt.BuildState != "" {

+ t.Fatalf("%q -> %q (another account): settings %q, build state %q; want both cleared",

+ tc.from, tc.to, rt.UpdateSettings, rt.BuildState)

+ }

+}

+

+// Upsert's tenant compare runs in SQLite (effectiveAccountSQL); it must

+// say exactly what EffectiveAccount (controlname.ValidAccount) says.

+func TestEffectiveAccountSQLIsEffectiveAccount(t *testing.T) {

+ reg, err := Open(filepath.Join(t.TempDir(), "r.db"))

+ if err != nil {

+ t.Fatal(err)

+ }

+ for _, a := range []string{"", "bab", "bes", "abc", "ab", "a", "abcdefghijklmnop", "abcdefghijklmnopq",

+ "Abc", "ABC", "ab1", "a-b", "a_b", "a b", " abc", "abc ", "abc\n", "\u00e1bc", "ab\u00e1", "ab]", "ab^", "ab*", "ab?", "ab["} {

+ var got string

+ if err := reg.sql.QueryRow(`SELECT `+effectiveAccountSQL("?1"), a).Scan(&got); err != nil {

+ t.Fatal(err)

+ }

+ if want := EffectiveAccount(a); got != want {

+ t.Fatalf("account %q: SQL says %q, EffectiveAccount says %q", a, got, want)

+ }

+}

+

func TestUpdateSettingsMovesSnapshotFingerprint(t *testing.T) {

reg, err := Open(filepath.Join(t.TempDir(), "r.db"))

if err != nil {

diff --git a/internal/release/buildstate.go b/internal/release/buildstate.go

index c280b9a56..b729a594d 100644

--- a/internal/release/buildstate.go

+++ b/internal/release/buildstate.go

@@ -128,10 +128,11 @@ func (c *Consumed) DecidedFor(rt registry.Route) bool {

// rt.App). A hostname can move between tenants on one box, and the file is

// keyed by hostname, so root and the agent treat a foreign file as absent

// wherever they decide from it (ruling R26): the adoption read, NextConsumed,

-// the pre-exec write and a move's carry. An untagged file (from before

-// ruling R22) is not foreign: those readers take it as the route's own, so a

-// completion or resolution only it holds still counts. The held wake check

-// and the update report are stricter and trust only DecidedFor.

+// the pre-exec write, a move's carry and the held wake check. An untagged

+// file (from before ruling R22) is not foreign: those readers take it as the

+// route's own, so a completion, resolution or failure only it holds still

+// counts (ruling R28). The update report is stricter and shows only a file

+// DecidedFor the route.

func (c *Consumed) ForeignTo(rt registry.Route) bool {

return c != nil && (c.Account != "" || c.App != "") && (c.Account != RouteAccount(rt) || c.App != rt.App)

}

diff --git a/internal/release/tenantchange_test.go b/internal/release/tenantchange_test.go

index f1bcdcf62..fc7446160 100644

--- a/internal/release/tenantchange_test.go

+++ b/internal/release/tenantchange_test.go

@@ -207,3 +207,15 @@ func TestConsumedForeignTo(t *testing.T) {

t.Error("the ops account's own file is foreign to its route")

}

+

+// The registry's Upsert compares tenants by registry.EffectiveAccount (Astra

+// r4 finding 2). It must be RouteAccount on the account, or the registry

+// keeps one tenant's settings columns for a route root reads as another's,

+// or drops them for one root reads as the same.

+func TestRegistryEffectiveAccountIsRouteAccount(t *testing.T) {

+ for _, a := range []string{"", OpsAccount, "bes", "cdf", "ab", "ABC", "ab1", "abcdefghijklmnopq", "abcdefghijklmnop"} {

+ if got, want := registry.EffectiveAccount(a), RouteAccount(registry.Route{Account: a}); got != want {

+ t.Fatalf("account %q: registry.EffectiveAccount %q, RouteAccount %q", a, got, want)

+ }

+}

carlos / platform Public

settings-5 task 8: fix round 1 (held wake reads untagged decisions, effective-account compare, fail closed on registry errors)