amadan
appconfig: gate the no-default guard on key loss, not file non-emptiness
Round-2 review. CRITICAL 1's guard used "a non-empty file exists" as a proxy for "a human staged this". After Sync's own first write the file is non-empty, so the second pass tripped the same guard: an app with no default bundle converged once and then froze, refusing to replace content Sync itself authored. Bundle edits never landed, and the refusal's "converges on the next pass" was false. The test is now key loss — refuse only when the file on disk holds keys the merged render would drop, which is the actual truncation hazard. Changed values and added keys converge normally, and the refusal self-heals: once the missing keys exist in a bundle nothing is lost and the route converges with no further intervention. The round-1 truncation case is unchanged (three keys on disk, one in the render, still refused) and its test is untouched. Keys are compared against m.Vars rather than rendered bytes so the decision still happens BEFORE the render-diff-write-restart path, which a refused route must never enter. An unparseable file has unknown keys, so nothing can prove the render drops none of them: that is treated as loss and refused, with its own message. Blank lines and #-comments are ignored, matching how systemd reads these files. Known limitation, named in the refusal and pinned by a test: while an app has no default bundle, REMOVING a key from its environment bundle is indistinguishable from truncation and is refused until the config is imported. Also names an escape in the Platform+Canary -environment refusal, which previously offered none: a binding made before that rail existed survives re-mints and cannot be cleared by this verb, so the way out is `carlos remove <host>` and letting the next reconcile pass re-mint it clean.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
3 files changed,
+253
−13
cli.go+3 −1internal/appconfig/sync.go+77 −12internal/appconfig/sync_test.go+173 −0
diff --git a/cli.go b/cli.go| index 3f46ea3cb..91be37344 100644 |
| --- a/cli.go |
| +++ b/cli.go |
| @@ -495,7 +495,9 @@ func runRoute(args []string) { |
| // half-owned column would be the worst of both. |
| envVerdicts[i] = envVerdict{rt: rt, from: from, kind: ePlatformOwned, note: "Platform+Canary route — its whole row is rebuilt by the domains\n" + |
| "reconciler's canary-minting pass (internal/domains/reconcile.go) whenever that pass's `followed`\n" + |
| - "check goes false, from values it derives itself. This verb does not own this row"} |
| + "check goes false, from values it derives itself. This verb does not own this row.\n" + |
| + "A binding already on such a route (made before this rail existed) survives re-mints and cannot be\n" + |
| + "cleared here either: `carlos remove " + rt.Host + "` and let the next reconcile pass re-mint it clean"} |
| case envClear: |
| envVerdicts[i] = envVerdict{rt: rt, from: from, kind: eWillWrite} |
| default: |
diff --git a/internal/appconfig/sync.go b/internal/appconfig/sync.go| index ea2f6eadb..7be8313f3 100644 |
| --- a/internal/appconfig/sync.go |
| +++ b/internal/appconfig/sync.go |
| @@ -15,6 +15,7 @@ import ( |
| "fmt" |
| "os" |
| "path/filepath" |
| + "sort" |
| "strings" |
| "github.com/carlosframework/platform/internal/registry" |
| @@ -57,8 +58,8 @@ func Sync(ctx context.Context, reg *registry.Registry, s release.Store, priv *ec |
| // defaultMissing marks a bound route's merged view as the named |
| // bundle ALONE, because the app has no config.json at all. Such a |
| // render is not an overlay, it is a wholesale replacement, so |
| - // syncHost refuses to let it overwrite a non-empty file (see its doc |
| - // comment; 2026-08-02 review, CRITICAL 1). |
| + // 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). |
| defaultMissing bool |
| } |
| resolved := map[groupKey]resolution{} |
| @@ -228,6 +229,42 @@ func reap(routes []registry.Route, envDir string) { |
| } |
| } |
| +// lostKeys reports which var names the file on disk holds that the manifest |
| +// about to replace it does not, sorted; parsed is false if cur is not |
| +// KEY=value throughout, in which case lost is meaningless and the caller must |
| +// treat the file as unsafe to overwrite. |
| +// |
| +// It compares against m.Vars rather than the rendered bytes on purpose: the |
| +// render is what the caller is deciding whether to WRITE, and this decision |
| +// has to be made before rendering (a refused route must never reach the |
| +// 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. |
| +// |
| +// Blank lines and #-comments are ignored, matching how systemd's |
| +// EnvironmentFile reads these files: they carry no configuration, so their |
| +// loss is not the loss this guard exists to prevent. Anything else without an |
| +// "=" and a non-empty name makes the whole file unparseable rather than |
| +// silently skipped — a line this function does not understand may well be a |
| +// var, and guessing it is not is the unsafe direction. |
| +func lostKeys(cur []byte, m *Manifest) (lost []string, parsed bool) { |
| + for _, line := range strings.Split(string(cur), "\n") { |
| + s := strings.TrimSpace(line) |
| + if s == "" || strings.HasPrefix(s, "#") { |
| + continue |
| + } |
| + name, _, ok := strings.Cut(s, "=") |
| + name = strings.TrimSpace(name) |
| + if !ok || name == "" { |
| + return nil, false |
| + } |
| + if _, kept := m.Vars[name]; !kept { |
| + lost = append(lost, name) |
| + } |
| + } |
| + sort.Strings(lost) |
| + return lost, true |
| +} |
| + |
| // syncable mirrors release.adoptable: lifecycle-managed instances of a |
| // known app, excluding platform-minted alias routes (an alias shares its |
| // canonical route's process and therefore its canonical route's config — |
| @@ -269,20 +306,48 @@ func syncHost(rt registry.Route, m *Manifest, defaultMissing bool, priv *ecdh.Pr |
| // render the override bundle ALONE — so the documented migration step |
| // (create a small override, bind the routes) would replace every var a |
| // hand-staged file holds with just the override's, as root, and restart |
| - // the unit. Three states, deliberately: |
| + // the unit. |
| + // |
| + // The test is KEY LOSS, not "a non-empty file exists" (round-2 review): |
| + // after this function's own first write the file is non-empty, so the |
| + // cruder test refused to replace content Sync itself authored and froze |
| + // the route forever. Losing keys is the actual hazard; a changed value or |
| + // an added key is ordinary convergence. It also self-heals — the moment |
| + // the missing keys exist in a bundle, nothing is lost and the route |
| + // converges with no further intervention. |
| // |
| // - default bundle EXISTS (even with zero vars): the operator has |
| // adopted bucket config for this app, the merged render is |
| - // authoritative, nothing below is skipped. |
| - // - default MISSING, non-empty file on disk: refuse this route, loudly, |
| - // every pass. The file is not ours to replace yet. |
| - // - default MISSING, nothing (or an empty file) on disk: nothing to |
| - // lose — a fresh route renders the environment alone. |
| + // authoritative, and none of this applies. |
| + // - default MISSING and the file holds keys the render drops: refuse |
| + // this route, loudly, every pass. Those keys are not ours to delete. |
| + // - default MISSING and nothing is lost (including an empty or absent |
| + // file): converge. |
| + // |
| + // Known limitation, accepted with the ruling: while an app has no default |
| + // bundle, REMOVING a key from its environment bundle is indistinguishable |
| + // from the truncation hazard and is refused. The message names both ways |
| + // out. A "# managed by carlos" marker line would separate the two cases |
| + // properly, at the cost of changing the rendered file format. |
| if defaultMissing && existed && len(cur) > 0 { |
| - fmt.Printf(" %s: environment %q is bound but %s has no default config bundle, so the merged view is that environment ALONE — "+ |
| - "refusing to replace the existing %s with it; import the staged config first (`carlos env set -app %s KEY=value ...`), "+ |
| - "then this route converges on the next pass\n", rt.Host, rt.Environment, rt.App, path, rt.App) |
| - return |
| + lost, parsed := lostKeys(cur, m) |
| + 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 — "+ |
| + "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) |
| + 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 — "+ |
| + "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) |
| + return |
| + } |
| } |
| rendered, err := RenderEnv(m, priv) |
| if err != nil { |
diff --git a/internal/appconfig/sync_test.go b/internal/appconfig/sync_test.go| index 9e42ca295..1fedbc201 100644 |
| --- a/internal/appconfig/sync_test.go |
| +++ b/internal/appconfig/sync_test.go |
| @@ -741,6 +741,179 @@ func TestSyncRefusesToReplaceStagedFileWhenAppHasNoDefaultBundle(t *testing.T) { |
| } |
| } |
| +// Round-2 review, IMPORTANT: the no-default guard must not freeze a route on |
| +// the file SYNC ITSELF wrote. "A non-empty file exists" is not evidence a |
| +// human staged it — after pass 1 it is evidence of pass 1 — so the guard is |
| +// keyed on KEY LOSS instead: it refuses only when the file on disk holds keys |
| +// the merged render would drop. Same keys with different values, and supersets |
| +// of what is on disk, converge freely. |
| +func TestSyncEnvironmentOnlyAppKeepsConvergingAfterItsOwnFirstWrite(t *testing.T) { |
| + s := release.NewDirStore(t.TempDir()) |
| + ctx := context.Background() |
| + reg := testRegistry(t) |
| + envDir := t.TempDir() |
| + priv, _, err := GenerateKey() |
| + if err != nil { |
| + t.Fatalf("GenerateKey: %v", err) |
| + } |
| + privKey, err := ParsePrivateKey(priv, "test") |
| + if err != nil { |
| + t.Fatalf("ParsePrivateKey: %v", err) |
| + } |
| + // No config.json for this app, ever — only a named bundle. |
| + if err := SaveEnvironment(ctx, s, "bab", "keymail", "demo", map[string]Var{ |
| + "LOG_LEVEL": {Kind: "env", Value: "debug"}, |
| + }, nil); err != nil { |
| + t.Fatalf("SaveEnvironment: %v", err) |
| + } |
| + if err := reg.Upsert(registry.Route{ |
| + Host: "fresh.example.com", Kind: "instance", App: "keymail", Account: "bab", Status: "ready", |
| + Backing: registry.BackingUnit, Environment: "demo", |
| + }); err != nil { |
| + t.Fatalf("Upsert: %v", err) |
| + } |
| + var restarted []string |
| + restart := func(h string) error { restarted = append(restarted, h); return nil } |
| + if err := Sync(ctx, reg, s, privKey, envDir, restart); err != nil { |
| + t.Fatalf("Sync (pass 1): %v", err) |
| + } |
| + path := filepath.Join(envDir, "fresh.example.com.env") |
| + if got, err := os.ReadFile(path); err != nil || string(got) != "LOG_LEVEL=debug\n" { |
| + t.Fatalf("pass 1: env = %q, err = %v; want LOG_LEVEL=debug", got, err) |
| + } |
| + |
| + // The operator edits the bundle: one value changed, one key added. |
| + if err := SaveEnvironment(ctx, s, "bab", "keymail", "demo", map[string]Var{ |
| + "LOG_LEVEL": {Kind: "env", Value: "trace"}, |
| + "NEW": {Kind: "env", Value: "1"}, |
| + }, nil); err != nil { |
| + t.Fatalf("SaveEnvironment (edit): %v", err) |
| + } |
| + restarted = nil |
| + if err := Sync(ctx, reg, s, privKey, envDir, restart); err != nil { |
| + t.Fatalf("Sync (pass 2): %v", err) |
| + } |
| + got, err := os.ReadFile(path) |
| + if err != nil { |
| + t.Fatalf("pass 2: read env: %v", err) |
| + } |
| + if string(got) != "LOG_LEVEL=trace\nNEW=1\n" { |
| + t.Fatalf("pass 2: env = %q, want the edited bundle to converge", got) |
| + } |
| + if len(restarted) != 1 || restarted[0] != "fresh.example.com" { |
| + t.Fatalf("pass 2: restarted = %v, want exactly [fresh.example.com]", restarted) |
| + } |
| +} |
| + |
| +// A file this pass cannot parse is content whose keys are unknown, so it is |
| +// impossible to prove nothing would be lost — assume the worst and refuse, |
| +// same direction as the guard's other cases. |
| +func TestSyncRefusesToReplaceAnUnparseableFileWhenAppHasNoDefaultBundle(t *testing.T) { |
| + s := release.NewDirStore(t.TempDir()) |
| + ctx := context.Background() |
| + reg := testRegistry(t) |
| + envDir := t.TempDir() |
| + priv, _, err := GenerateKey() |
| + if err != nil { |
| + t.Fatalf("GenerateKey: %v", err) |
| + } |
| + privKey, err := ParsePrivateKey(priv, "test") |
| + if err != nil { |
| + t.Fatalf("ParsePrivateKey: %v", err) |
| + } |
| + if err := SaveEnvironment(ctx, s, "bab", "keymail", "demo", map[string]Var{ |
| + "LOG_LEVEL": {Kind: "env", Value: "debug"}, |
| + }, nil); err != nil { |
| + t.Fatalf("SaveEnvironment: %v", err) |
| + } |
| + if err := reg.Upsert(registry.Route{ |
| + Host: "odd.example.com", Kind: "instance", App: "keymail", Account: "bab", Status: "ready", |
| + Backing: registry.BackingUnit, Environment: "demo", |
| + }); err != nil { |
| + t.Fatalf("Upsert: %v", err) |
| + } |
| + junk := "this file is not KEY=value at all\n" |
| + path := filepath.Join(envDir, "odd.example.com.env") |
| + if err := os.WriteFile(path, []byte(junk), 0o600); err != nil { |
| + t.Fatalf("stage junk env file: %v", err) |
| + } |
| + var restarted []string |
| + if err := Sync(ctx, reg, s, privKey, envDir, func(h string) error { |
| + restarted = append(restarted, h) |
| + return nil |
| + }); err != nil { |
| + t.Fatalf("Sync: %v", err) |
| + } |
| + got, err := os.ReadFile(path) |
| + if err != nil { |
| + t.Fatalf("read env: %v", err) |
| + } |
| + if string(got) != junk { |
| + t.Fatalf("env = %q, want an unparseable file left untouched", got) |
| + } |
| + if len(restarted) != 0 { |
| + t.Fatalf("restarted = %v, want none", restarted) |
| + } |
| +} |
| + |
| +// Known limitation of the key-loss gate, pinned so it is not a surprise: |
| +// while an app has NO default bundle, REMOVING a key from its environment |
| +// bundle looks exactly like the truncation hazard (a key on disk that the |
| +// render drops) and is refused. The refusal names both escapes — put the |
| +// remaining config in a default bundle, or delete the file. It self-heals the |
| +// moment a default bundle exists, which is the migration this guard is for. |
| +func TestSyncEnvOnlyKeyRemovalIsRefusedUntilADefaultBundleExists(t *testing.T) { |
| + s := release.NewDirStore(t.TempDir()) |
| + ctx := context.Background() |
| + reg := testRegistry(t) |
| + envDir := t.TempDir() |
| + priv, _, err := GenerateKey() |
| + if err != nil { |
| + t.Fatalf("GenerateKey: %v", err) |
| + } |
| + privKey, err := ParsePrivateKey(priv, "test") |
| + if err != nil { |
| + t.Fatalf("ParsePrivateKey: %v", err) |
| + } |
| + if err := SaveEnvironment(ctx, s, "bab", "keymail", "demo", map[string]Var{ |
| + "KEEP": {Kind: "env", Value: "1"}, |
| + "GONE": {Kind: "env", Value: "2"}, |
| + }, nil); err != nil { |
| + t.Fatalf("SaveEnvironment: %v", err) |
| + } |
| + if err := reg.Upsert(registry.Route{ |
| + Host: "shrink.example.com", Kind: "instance", App: "keymail", Account: "bab", Status: "ready", |
| + Backing: registry.BackingUnit, Environment: "demo", |
| + }); err != nil { |
| + t.Fatalf("Upsert: %v", err) |
| + } |
| + if err := Sync(ctx, reg, s, privKey, envDir, func(string) error { return nil }); err != nil { |
| + t.Fatalf("Sync (pass 1): %v", err) |
| + } |
| + path := filepath.Join(envDir, "shrink.example.com.env") |
| + if _, err := UnsetEnvironment(ctx, s, "bab", "keymail", "demo", []string{"GONE"}); err != nil { |
| + t.Fatalf("UnsetEnvironment: %v", err) |
| + } |
| + if err := Sync(ctx, reg, s, privKey, envDir, func(string) error { return nil }); err != nil { |
| + t.Fatalf("Sync (pass 2): %v", err) |
| + } |
| + if got, _ := os.ReadFile(path); string(got) != "GONE=2\nKEEP=1\n" { |
| + t.Fatalf("env = %q, want the removal refused while the app has no default bundle", got) |
| + } |
| + // The moment a default bundle exists the app is bucket-managed and the |
| + // merged render is authoritative — the removal lands, no operator |
| + // intervention beyond the import the refusal asked for. |
| + if err := SetEnv(ctx, s, "bab", "keymail", map[string]string{"KEEP": "1"}); err != nil { |
| + t.Fatalf("SetEnv: %v", err) |
| + } |
| + if err := Sync(ctx, reg, s, privKey, envDir, func(string) error { return nil }); err != nil { |
| + t.Fatalf("Sync (pass 3): %v", err) |
| + } |
| + if got, _ := os.ReadFile(path); string(got) != "KEEP=1\n" { |
| + t.Fatalf("env = %q, want the removal to land once a default bundle exists", got) |
| + } |
| +} |
| + |
| func TestSyncMergedRenderIsAuthoritativeOnceADefaultBundleExists(t *testing.T) { |
| // The other side of the guard above: once the app HAS a config.json — |
| // even one with every var unset — the operator has adopted bucket config |