carlos / platform Public

appconfig: the key-loss guard covers an instance overlay standing alone (plan 4, task 5)

An unbound route of an app with no default bundle used to resolve to
"never configured" and be left alone. With the instance layer it can
now resolve to an overlay alone, the same wholesale-replacement state
a bound route with no default bundle is in, reached through a
different door. Fetch the overlay before the m == nil skip so that
door is seen, set defaultMissing for it too, and let syncHost's
lostKeys guard name which overlay is standing alone instead of
assuming a bound environment.

Also fixes a stale reference: the syncHost call still passed
r.defaultMissing instead of the new local, which would have left the
guard blind to the new case while still compiling.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Paul Campbell pushed by claude 1 month ago 05b72104fd0ed53582738ff1dc941d7f89f54ce8 parent 77cd1f1

2 files changed, +118 −14

internal/appconfig/instances_test.go +81 −0
internal/appconfig/sync.go +37 −14

diff --git a/internal/appconfig/instances_test.go b/internal/appconfig/instances_test.go

index 9e4420e51..0bf1b7d3d 100644

--- a/internal/appconfig/instances_test.go

+++ b/internal/appconfig/instances_test.go

@@ -576,3 +576,84 @@ func (f *failingListStore) List(ctx context.Context, prefix string) ([]string, e

}

return f.Store.List(ctx, prefix)

}

+// The CRITICAL 1 guard's second door. An app with no config.json, a route

+// bound to nothing, and an overlay: the merged view is that overlay ALONE,

+// which is a wholesale replacement of whatever a hand-staged file holds.

+// Refuse it, loudly, every pass, and name the way out.

+func TestSyncRefusesToDropHandStagedKeysForAnOverlayAlone(t *testing.T) {

+ ctx, s, reg, envDir, priv := syncInstanceKit(t)

+ if _, _, err := ApplyInstance(ctx, s, "bab", "titogo", "book.example.com",

+ map[string]Var{"TENANT": {Kind: "env", Value: "book"}}, nil); err != nil {

+ t.Fatalf("ApplyInstance: %v", err)

+ }

+ unitRoute(t, reg, "book.example.com", "titogo", "bab", "")

+ path := filepath.Join(envDir, "book.example.com.env")

+ staged := "HAND_STAGED=1\nTENANT=old\n"

+ if err := os.WriteFile(path, []byte(staged), 0o600); err != nil {

+ t.Fatalf("stage file: %v", err)

+ }

+ var restarted []string

+ out := captureStdout(t, func() {

+ if err := Sync(ctx, reg, s, priv, envDir, func(h string) error {

+ restarted = append(restarted, h)

+ return nil

+ }, nil); err != nil {

+ t.Fatalf("Sync: %v", err)

+ }

+ })

+ cur, err := os.ReadFile(path)

+ if err != nil || string(cur) != staged {

+ t.Fatalf("a hand-staged file was replaced by an overlay-alone render: %q %v", cur, err)

+ }

+ if len(restarted) != 0 {

+ t.Fatalf("restarted %v for a route this pass refused", restarted)

+ }

+ if !strings.Contains(out, "HAND_STAGED") || !strings.Contains(out, "book.example.com") {

+ t.Fatalf("the refusal does not name the route and the key it would have dropped:\n%s", out)

+ }

+// The same shape, self-healing: nothing is lost, so the route converges.

+// An app with no default bundle and an instance overlay is a legitimate

+// state (step 5 creates exactly that for a fresh instance), and this guard

+// must not freeze it forever.

+func TestSyncConvergesAnOverlayAloneWhenNothingIsLost(t *testing.T) {

+ ctx, s, reg, envDir, priv := syncInstanceKit(t)

+ if _, _, err := ApplyInstance(ctx, s, "bab", "titogo", "book.example.com",

+ map[string]Var{"TENANT": {Kind: "env", Value: "book"}}, nil); err != nil {

+ t.Fatalf("ApplyInstance: %v", err)

+ }

+ unitRoute(t, reg, "book.example.com", "titogo", "bab", "")

+ if err := Sync(ctx, reg, s, priv, envDir, func(string) error { return nil }, nil); err != nil {

+ t.Fatalf("Sync: %v", err)

+ }

+ cur, err := os.ReadFile(filepath.Join(envDir, "book.example.com.env"))

+ if err != nil {

+ t.Fatalf("read env file: %v", err)

+ }

+ if !bytes.Contains(cur, []byte("TENANT=book\n")) {

+ t.Fatalf("the overlay-alone render did not land:\n%s", cur)

+ }

+// And the case that must NOT change: no default bundle, no overlay, no

+// binding is still "never configured at all", and a never-configured app's

+// hand-staged file is not this pass's to touch.

+func TestSyncStillLeavesUnconfiguredAppsAlone(t *testing.T) {

+ ctx, s, reg, envDir, priv := syncInstanceKit(t)

+ unitRoute(t, reg, "book.example.com", "titogo", "bab", "")

+ path := filepath.Join(envDir, "book.example.com.env")

+ staged := "HAND_STAGED=1\n"

+ if err := os.WriteFile(path, []byte(staged), 0o600); err != nil {

+ t.Fatalf("stage file: %v", err)

+ }

+ if err := Sync(ctx, reg, s, priv, envDir, func(string) error { return nil }, nil); err != nil {

+ t.Fatalf("Sync: %v", err)

+ }

+ cur, err := os.ReadFile(path)

+ if err != nil || string(cur) != staged {

+ t.Fatalf("an unconfigured app's file moved: %q %v", cur, err)

+ }

diff --git a/internal/appconfig/sync.go b/internal/appconfig/sync.go

index 830a71566..cd6e533e4 100644

--- a/internal/appconfig/sync.go

+++ b/internal/appconfig/sync.go

@@ -67,6 +67,9 @@ func Sync(ctx context.Context, reg *registry.Registry, s release.Store, priv *ec

// render is not an overlay, it is a wholesale replacement, so

// syncHost refuses to let it drop keys the file on disk already holds

// (see its doc comment; 2026-08-02 review, CRITICAL 1 and round 2).

+ // The materialization loop sets the same flag for an UNBOUND route

+ // whose only config is its instance overlay (senders design §6.2),

+ // which is the same replacement reached by a different door.

defaultMissing bool

}

resolved := map[groupKey]resolution{}

@@ -166,14 +169,11 @@ func Sync(ctx context.Context, reg *registry.Registry, s release.Store, priv *ec

fmt.Printf(" %s: config unresolved (environment %q), env file left as-is: %v\n", rt.Host, env, r.err)

continue

}

- m := r.m

- if m == nil {

- continue

- }

- // The instance overlay wins per key over the group's merge. An

- // unresolved overlay is NOT an absent one: this route keeps its file

- // and the pass says which route and why, exactly as it does for a

- // group whose bundle could not be read.

+ // The instance overlay wins per key over the group's merge, and is

+ // fetched BEFORE the m == nil skip below: an unbound route of an app

+ // with no default bundle used to resolve to "never configured" and

+ // stop here, but with an overlay it now resolves to something (senders

+ // design §6.2), so the skip has to see the overlay first.

inst, _, err := overlayFor(k.acct, k.app, rt.Host)

if err != nil {

// Said ONCE per (account, app), not once per route. The listing

@@ -192,6 +192,18 @@ func Sync(ctx context.Context, reg *registry.Registry, s release.Store, priv *ec

}

continue

}

+ m := r.m

+ if m == nil && inst == nil {

+ continue // never configured at all: this file is not ours to touch

+ }

+ // An overlay with no group base behind it is a wholesale replacement,

+ // not an overlay. it is the same state a bound route with no default

+ // bundle is in, reached without a binding. It gets the same guard:

+ // see syncHost.

+ defaultMissing := r.defaultMissing

+ if m == nil {

+ defaultMissing = true

+ }

m = applyInstanceOverlay(k.app, m, inst)

// Only unit-backed routes read /etc/carlos/apps — an exec-backed

// route's env comes from <DataDir>/<host>.env, which the activator

@@ -205,7 +217,7 @@ func Sync(ctx context.Context, reg *registry.Registry, s release.Store, priv *ec

fmt.Printf(" %s: exec-backed — config delivered by the agent's in-process loop to /data/<host>.config.env, not this pass\n", rt.Host)

continue

}

- syncHost(rt, m, r.defaultMissing, priv, envDir, restart, ack)

+ syncHost(rt, m, defaultMissing, priv, envDir, restart, ack)

}

// An empty route list is never authority to delete anything:

// registry.Open silently creates an empty database when handed a wrong

@@ -282,7 +294,10 @@ func reap(routes []registry.Route, envDir string) {

// render-diff-write-restart path at all). RenderEnv emits exactly one

// KEY=value line per var, so the two key sets are the same by construction —

// except for the agent-owned names applyAgentVars adds to and removes from

-// the rendered file afterward, which is why they are skipped below.

+// the rendered file afterward, which is why they are skipped below. m is the

+// route's FULL merge, all three layers of it (default, environment,

+// instance), because the question is what the file would lose, not which

+// bundle a key came from.

// Blank lines and #-comments are ignored, matching how systemd's

// EnvironmentFile reads these files: they carry no configuration, so their

@@ -420,21 +435,29 @@ func syncHost(rt registry.Route, m *Manifest, defaultMissing bool, priv *ecdh.Pr

// properly, at the cost of changing the rendered file format.

if defaultMissing && existed && len(cur) > 0 {

lost, parsed := lostKeys(cur, m)

+ // Which overlay is standing alone. A bound route names its

+ // environment; an unbound one is here because this instance has its

+ // own config layer (senders design §6.2) and its app has no default

+ // bundle, and naming an empty environment would be a riddle.

+ alone := "this instance's own config layer"

+ if rt.Environment != "" {

+ alone = fmt.Sprintf("environment %q", rt.Environment)

+ }

switch {

case !parsed:

// Unparseable content has unknown keys, so nothing here can prove

// the render drops none of them. Assume the worst — this guard's

// whole job is to be wrong in the safe direction.

- fmt.Printf(" %s: environment %q is bound but %s has no default config bundle, so the merged view is that environment ALONE — "+

+ fmt.Printf(" %s: %s has no default config bundle, so the merged view is %s ALONE — "+

"refusing to replace %s, which this pass cannot parse as KEY=value and so cannot prove is safe to overwrite; "+

"import its config (`carlos env set --app %s KEY=value ...`) or remove the file\n",

- rt.Host, rt.Environment, rt.App, path, rt.App)

+ rt.Host, rt.App, alone, path, rt.App)

return

case len(lost) > 0:

- fmt.Printf(" %s: environment %q is bound but %s has no default config bundle, so the merged view is that environment ALONE — "+

+ fmt.Printf(" %s: %s has no default config bundle, so the merged view is %s ALONE — "+

"refusing to drop %d var(s) already in %s (%s); import them (`carlos env set --app %s KEY=value ...`) "+

"and this route converges on the next pass, or remove the file if it is dead\n",

- rt.Host, rt.Environment, rt.App, len(lost), path, strings.Join(lost, ", "), rt.App)

+ rt.Host, rt.App, alone, len(lost), path, strings.Join(lost, ", "), rt.App)

return

}