amadan
appconfig: task 7 fix round 1 — the #344 hold is not a removal branch
Review of the e273fd57 range found the #344 account-mis-stamp guard's `r.m == nil && inst == nil` narrowing was wrong: that guard is not a removal decision, it is "this account column cannot be trusted, touch nothing," and a mis-stamp that lands the group's default-bundle read under the wrong prefix lands the instance layer's own write under that same wrong prefix too — so an overlay resolving under a mis-stamped account is what a mangled fleet produces, not evidence of safety. Reverted the guard's condition to r.m == nil (its original reach); the mass-removal guard's own narrowing is unaffected, since that branch really is a removal decision. Also: the listing-failure half of the said-once-per-app dedup sentinel (82df5301, mirrored into this sweep as e273fd57) had no positive test in either sweep that put more than one route behind one failed listing, so three mutations (errors.As forced false in each sweep, and dropping instanceOverlayListErr's wrapper) all left the suite green. Added a two-route listing-failure test to each sweep; SyncExec had none at all before this. And the exec path's listing-failure line was missing the "(seen at %s)" host anchor sync.go's carries, so an operator grepping a hostname on that path found nothing — added it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
3 files changed,
+178
−5
internal/appconfig/instances_test.go+86 −0internal/appconfig/syncexec.go+23 −5internal/appconfig/syncexec_test.go+69 −0
diff --git a/internal/appconfig/instances_test.go b/internal/appconfig/instances_test.go| index 3708c290f..3ac51fb54 100644 |
| --- a/internal/appconfig/instances_test.go |
| +++ b/internal/appconfig/instances_test.go |
| @@ -623,6 +623,92 @@ func TestSyncLeavesTheFileAloneWhenTheOverlayListingFails(t *testing.T) { |
| } |
| } |
| +// The dedup sentinel itself, pinned across TWO routes of one app sharing |
| +// one failed LISTING (review finding, plan 4 task 7 fix round 1): every |
| +// other test naming a listing failure uses a single route, so a mutation |
| +// that stopped telling a listing failure apart from a per-host one — |
| +// errors.As(...) always false, or instanceOverlayListErr's wrapper dropped |
| +// entirely — would still leave every one of them green. This asserts the |
| +// dedup's actual effect: one line, not two, with one host anchor naming |
| +// whichever route the pass happened to reach first. |
| +func TestSyncDedupsTheOverlayListingFailureAcrossTwoRoutesOfOneApp(t *testing.T) { |
| + ctx, base, reg, envDir, priv := syncInstanceKit(t) |
| + if err := SetEnv(ctx, base, "bab", "titogo", map[string]string{"APP_WIDE": "yes"}); err != nil { |
| + t.Fatalf("SetEnv: %v", err) |
| + } |
| + unitRoute(t, reg, "book.example.com", "titogo", "bab", "") |
| + unitRoute(t, reg, "cart.example.com", "titogo", "bab", "") |
| + for _, host := range []string{"book.example.com", "cart.example.com"} { |
| + if err := os.WriteFile(filepath.Join(envDir, host+".env"), []byte("APP_WIDE=stale\n"), 0o600); err != nil { |
| + t.Fatalf("stage %s: %v", host, err) |
| + } |
| + } |
| + failing := &failingListStore{Store: base, failPrefix: InstancesConfigPrefix("bab", "titogo"), |
| + err: errors.New("s3: transient 503 SlowDown")} |
| + out := captureStdout(t, func() { |
| + if err := Sync(ctx, reg, failing, priv, envDir, func(string) error { return nil }, nil); err != nil { |
| + t.Fatalf("Sync: %v", err) |
| + } |
| + }) |
| + if n := strings.Count(out, "instance config unresolved"); n != 1 { |
| + t.Fatalf("instance config unresolved lines = %d, want exactly 1 for one shared LISTING failure across two routes, got:\n%s", n, out) |
| + } |
| + if n := strings.Count(out, "(seen at "); n != 1 { |
| + t.Fatalf("(seen at ...) host anchors = %d, want exactly 1, got:\n%s", n, out) |
| + } |
| + for _, host := range []string{"book.example.com", "cart.example.com"} { |
| + cur, err := os.ReadFile(filepath.Join(envDir, host+".env")) |
| + if err != nil || string(cur) != "APP_WIDE=stale\n" { |
| + t.Fatalf("%s moved on a listing failure: %q %v", host, cur, err) |
| + } |
| + } |
| +} |
| + |
| +// The exec mirror: SyncExec had NO listing-failure test at all before this |
| +// (its said-once-per-app contract, stated in the doc comment above the |
| +// overlayFor loop, was pinned nowhere). Same two-route shape as the Sync |
| +// test above, same three mutations it exists to catch. |
| +func TestSyncExecDedupsTheOverlayListingFailureAcrossTwoRoutesOfOneApp(t *testing.T) { |
| + ctx, base, reg, dd, privB64, _ := execKit(t) |
| + priv, err := ParsePrivateKey(privB64, "test") |
| + if err != nil { |
| + t.Fatalf("ParsePrivateKey: %v", err) |
| + } |
| + if err := SetEnv(ctx, base, "bab", "titogo", map[string]string{"APP_WIDE": "yes"}); err != nil { |
| + t.Fatalf("SetEnv: %v", err) |
| + } |
| + for _, host := range []string{"book.example.com", "cart.example.com"} { |
| + if err := reg.Upsert(registry.Route{ |
| + Host: host, Kind: "instance", App: "titogo", Account: "bab", Status: "ready", |
| + Socket: "/run/carlos/" + host + ".sock", |
| + }); err != nil { |
| + t.Fatalf("Upsert %s: %v", host, err) |
| + } |
| + if err := os.WriteFile(filepath.Join(dd, host+".config.env"), []byte("OLD=1\n"), 0o600); err != nil { |
| + t.Fatalf("stage %s: %v", host, err) |
| + } |
| + } |
| + failing := &failingListStore{Store: base, failPrefix: InstancesConfigPrefix("bab", "titogo"), |
| + err: errors.New("s3: transient 503 SlowDown")} |
| + out := captureStdout(t, func() { |
| + if err := SyncExec(ctx, reg, failing, priv, dd, func(string) error { return nil }, "", nil); err != nil { |
| + t.Fatalf("SyncExec: %v", err) |
| + } |
| + }) |
| + if n := strings.Count(out, "instance config unresolved"); n != 1 { |
| + t.Fatalf("instance config unresolved lines = %d, want exactly 1 for one shared LISTING failure across two routes, got:\n%s", n, out) |
| + } |
| + if n := strings.Count(out, "(seen at "); n != 1 { |
| + t.Fatalf("(seen at ...) host anchors = %d, want exactly 1, got:\n%s", n, out) |
| + } |
| + for _, host := range []string{"book.example.com", "cart.example.com"} { |
| + cur, err := os.ReadFile(filepath.Join(dd, host+".config.env")) |
| + if err != nil || string(cur) != "OLD=1\n" { |
| + t.Fatalf("%s moved on a listing failure: %q %v", host, cur, err) |
| + } |
| + } |
| +} |
| + |
| // failingListStore makes List fail for exactly one prefix, the listing |
| // shape of failingKeyStore (syncexec_test.go). |
| type failingListStore struct { |
diff --git a/internal/appconfig/syncexec.go b/internal/appconfig/syncexec.go| index 826d983b3..1bb114138 100644 |
| --- a/internal/appconfig/syncexec.go |
| +++ b/internal/appconfig/syncexec.go |
| @@ -446,7 +446,12 @@ func SyncExec(ctx context.Context, reg *registry.Registry, s release.Store, priv |
| ak := appKey{k.acct, k.app} |
| if !said[ak] { |
| said[ak] = true |
| - fmt.Printf(" %s/%s: instance config unresolved, config files left as-is: %v\n", k.acct, k.app, instErr) |
| + // rt.Host anchors the line to a real route (the first |
| + // one this pass happened to reach), matching sync.go's |
| + // own wording, so an operator scanning the log has a |
| + // hostname to grep for — every other route of this app |
| + // is silently covered by the same line. |
| + fmt.Printf(" %s/%s: instance config unresolved (seen at %s), config files left as-is: %v\n", k.acct, k.app, rt.Host, instErr) |
| } |
| } else { |
| // A per-host Get or parse failure, unlike the listing |
| @@ -461,9 +466,8 @@ func SyncExec(ctx context.Context, reg *registry.Registry, s release.Store, priv |
| continue |
| } |
| // An overlay is configuration, so a route that has one is never |
| - // "absent" however its group base resolved. Both branches below decide |
| - // REMOVAL, and neither may fire for a route whose own layer is |
| - // standing right there. |
| + // "absent" for the MASS-REMOVAL guard below, however its group base |
| + // resolved: writing a file a bundle asked for is not a removal. |
| if r.m == nil && inst == nil && skipRemovals { |
| continue |
| } |
| @@ -474,7 +478,21 @@ func SyncExec(ctx context.Context, reg *registry.Registry, s release.Store, priv |
| // stamps it from bucket listings, and a mis-stamp deleted a live |
| // tenant's delivered secrets on 2026-08-28). Held, loudly, like an |
| // unresolved group — the fix is the row's account, not this file. |
| - if r.m == nil && inst == nil { |
| + // |
| + // UNLIKE the mass-removal guard above, this hold is NOT narrowed by |
| + // an overlay (review finding, plan 4 task 7 fix round 1: the brief's |
| + // own reasoning — "a route with an overlay never reaches a removal |
| + // branch anyway" — does not transfer here, because this is not a |
| + // removal branch. It is "this account column cannot be trusted, |
| + // touch nothing." A mis-stamp that makes the group's default-bundle |
| + // READ land under the wrong prefix makes the instance layer's own |
| + // WRITE (ApplyInstance) land under that same wrong prefix too, so an |
| + // overlay resolving here under the mis-stamped account is exactly |
| + // what a mis-stamped fleet produces — not evidence the account |
| + // column is safe to act on for this route. Wholesale-replacing the |
| + // file with that overlay's contents would be the 2026-08-28 failure |
| + // again, just reached through the overlay instead of the base. |
| + if r.m == nil { |
| if sq := release.HostAccountLabel(rt.Host, appsDomain); sq != "" && sq != k.acct { |
| fmt.Printf(" %s: no config manifest under account %q, but the host names account %q — config file left as-is; fix the route row's account column (#344)\n", |
| rt.Host, k.acct, sq) |
diff --git a/internal/appconfig/syncexec_test.go b/internal/appconfig/syncexec_test.go| index 453569c88..1ee0bfd15 100644 |
| --- a/internal/appconfig/syncexec_test.go |
| +++ b/internal/appconfig/syncexec_test.go |
| @@ -2261,6 +2261,75 @@ func TestSyncExecHoldsConfigWhenHostNamesADifferentAccount(t *testing.T) { |
| } |
| } |
| +// The #344 guard is NOT a removal branch, and an overlay does not clear it |
| +// (task 7 fix round 1 — the brief's own reasoning, "a route with an overlay |
| +// never reaches a removal branch anyway," does not transfer to this guard). |
| +// A mis-stamp that makes the group's default-bundle READ land under the |
| +// wrong account prefix makes the instance layer's own WRITE |
| +// (ApplyInstance) land under that same wrong prefix too, so an overlay |
| +// resolving here under the mis-stamped account is exactly what a |
| +// mis-stamped fleet produces — not evidence the account column is safe to |
| +// act on. Sibling of TestSyncExecHoldsConfigWhenHostNamesADifferentAccount, |
| +// with an overlay added to the held route. |
| +func TestSyncExecHoldsConfigWhenHostNamesADifferentAccountEvenWithAnOverlay(t *testing.T) { |
| + ctx, s, reg, dd, priv, _ := execKit(t) |
| + pk, err := ParsePrivateKey(priv, "test") |
| + if err != nil { |
| + t.Fatalf("ParsePrivateKey: %v", err) |
| + } |
| + if err := reg.Upsert(registry.Route{ |
| + Host: "birthday-alarm.bde.oncarlos.test", Kind: "instance", App: "birthday-alarm", |
| + Account: "bab", // the mis-stamp |
| + Status: "ready", Socket: "/run/carlos/birthday.sock", |
| + }); err != nil { |
| + t.Fatalf("Upsert: %v", err) |
| + } |
| + // The overlay lands at apps/bab/birthday-alarm/instances-config/ — the |
| + // SAME mis-stamped prefix the group's absent default bundle was looked |
| + // up under, which is the whole point: a mangled account column mangles |
| + // both. |
| + if _, _, err := ApplyInstance(ctx, s, "bab", "birthday-alarm", "birthday-alarm.bde.oncarlos.test", |
| + map[string]Var{"TENANT": {Kind: "env", Value: "mis-stamped"}}, nil); err != nil { |
| + t.Fatalf("ApplyInstance: %v", err) |
| + } |
| + // A second, healthy group so the mass-removal guard (which needs EVERY |
| + // group absent) stays out of the way — this guard is per-route. |
| + if err := reg.Upsert(registry.Route{ |
| + Host: "inst.example.com", Kind: "instance", App: "eleven", Account: "bab", |
| + Status: "ready", Socket: "/run/carlos/inst.sock", |
| + }); err != nil { |
| + t.Fatalf("Upsert: %v", err) |
| + } |
| + if err := SetEnv(ctx, s, "bab", "eleven", map[string]string{"A": "1"}); err != nil { |
| + t.Fatalf("SetEnv: %v", err) |
| + } |
| + path := filepath.Join(dd, "birthday-alarm.bde.oncarlos.test.config.env") |
| + if err := os.WriteFile(path, []byte("BIRTHDAYALARM_INSTANCE_KEY=deadbeef\n"), 0o600); err != nil { |
| + t.Fatalf("stage: %v", err) |
| + } |
| + var bounced []string |
| + out := captureStdout(t, func() { |
| + if err := SyncExec(ctx, reg, s, pk, dd, func(h string) error { |
| + bounced = append(bounced, h) |
| + return nil |
| + }, "oncarlos.test", nil); err != nil { |
| + t.Fatalf("SyncExec: %v", err) |
| + } |
| + }) |
| + cur, err := os.ReadFile(path) |
| + if err != nil || string(cur) != "BIRTHDAYALARM_INSTANCE_KEY=deadbeef\n" { |
| + t.Fatalf("delivered config was replaced by the overlay under a mis-stamped account: %q %v", cur, err) |
| + } |
| + for _, h := range bounced { |
| + if h == "birthday-alarm.bde.oncarlos.test" { |
| + t.Fatalf("held route was bounced: %v", bounced) |
| + } |
| + } |
| + if !strings.Contains(out, "birthday-alarm.bde.oncarlos.test") || !strings.Contains(out, "#344") { |
| + t.Fatalf("the #344 hold did not fire for a route that has an overlay:\n%s", out) |
| + } |
| +} |
| + |
| // TestSyncExecWritesConfigAckForSocketExecRoute is the review's finding 1 |
| // (2026-09-07): syncExecHost DOES bounce a socket-exec route (bounce is |
| // non-nil for it, only a tcp-exec-file/addr route gets nil), so its ack |