amadan
appconfig: gate the unit pass's instance-overlay Get on Backing == BackingUnit
Sync ran overlayFor for every syncable route and only checked rt.Backing != registry.BackingUnit 53 lines later, so it paid a Get for every exec-backed route with an overlay -- exactly the routes step 5 targets -- and discarded the result. A Get or parse failure on such a route also printed "instance config unresolved, env file left as-is" about a file this pass never writes, which SyncExec already reports correctly on its own. Fix: gate the overlayFor call itself on rt.Backing == registry.BackingUnit, leaving the later Backing check (the "exec-backed" informational print) exactly where it was -- moving that check earlier instead would make it fire for an exec-backed route with no config at all, which it does not do today. Adds tests pinning both halves: an exec-backed route with an overlay costs the unit pass zero Gets, and a Get failure on that overlay prints nothing from the unit pass. Final review, plan 4 (tenant email senders, per-instance config layer).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
2 files changed,
+122
−29
internal/appconfig/instances_test.go+74 −0internal/appconfig/sync.go+48 −29
diff --git a/internal/appconfig/instances_test.go b/internal/appconfig/instances_test.go| index 4d87cafb1..beb862c4d 100644 |
| --- a/internal/appconfig/instances_test.go |
| +++ b/internal/appconfig/instances_test.go |
| @@ -553,6 +553,80 @@ func TestSyncCostsOneListingPerAppAndOneGetPerOverlay(t *testing.T) { |
| } |
| } |
| +// The unit pass never delivers an exec-backed route's config at all |
| +// (TestSyncSkipsExecBackedRoutes) — SyncExec does, and reports its own |
| +// overlay failures on it correctly. Final review, plan 4: the unit pass |
| +// used to fetch the overlay for EVERY syncable route regardless of Backing |
| +// and only checked Backing 53 lines later, so it paid a Get for exactly the |
| +// exec-backed, overlay-bearing routes step 5 targets, and threw the result |
| +// away. This pins the fixed cost: zero Gets against that key from the unit |
| +// pass, whatever the route's overlay actually holds. |
| +func TestSyncPaysNoGetForAnExecBackedRoutesOverlay(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) |
| + } |
| + if _, _, err := ApplyInstance(ctx, base, "bab", "titogo", "book.example.com", |
| + map[string]Var{"TENANT": {Kind: "env", Value: "book"}}, nil); err != nil { |
| + t.Fatalf("ApplyInstance: %v", err) |
| + } |
| + // Exec-backed: Backing left at its zero value, the fleet default (same |
| + // shape as TestSyncSkipsExecBackedRoutes). |
| + if err := reg.Upsert(registry.Route{ |
| + Host: "book.example.com", Kind: "instance", App: "titogo", Account: "bab", Status: "ready", |
| + }); err != nil { |
| + t.Fatalf("Upsert: %v", err) |
| + } |
| + s := newCountingStore(base) |
| + if err := Sync(ctx, reg, s, priv, envDir, func(string) error { return nil }, nil); err != nil { |
| + t.Fatalf("Sync: %v", err) |
| + } |
| + if got := s.n(s.gets, InstanceConfigKey("bab", "titogo", "book.example.com")); got != 0 { |
| + t.Fatalf("unit pass Get'd an exec-backed route's overlay %d time(s), want 0: SyncExec owns delivering it, not Sync", got) |
| + } |
| + if _, err := os.Stat(filepath.Join(envDir, "book.example.com.env")); !os.IsNotExist(err) { |
| + t.Fatalf("expected no env file for an exec-backed route, stat err = %v", err) |
| + } |
| +} |
| + |
| +// A Get failure against an exec-backed route's overlay must be invisible to |
| +// the unit pass: gating the fetch on Backing == BackingUnit (rather than |
| +// merely discarding its result) means the failure never happens here at |
| +// all, so the pass cannot print the misleading "env file left as-is" claim |
| +// about a file it never writes for this route. SyncExec, exercised |
| +// separately, is the one that reports a failure on this exact key. |
| +func TestSyncPrintsNothingForAnExecBackedRoutesOverlayGetFailure(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) |
| + } |
| + if _, _, err := ApplyInstance(ctx, base, "bab", "titogo", "book.example.com", |
| + map[string]Var{"TENANT": {Kind: "env", Value: "book"}}, nil); err != nil { |
| + t.Fatalf("ApplyInstance: %v", err) |
| + } |
| + if err := reg.Upsert(registry.Route{ |
| + Host: "book.example.com", Kind: "instance", App: "titogo", Account: "bab", Status: "ready", |
| + }); err != nil { |
| + t.Fatalf("Upsert: %v", err) |
| + } |
| + failing := &failingKeyStore{Store: base, failKey: InstanceConfigKey("bab", "titogo", "book.example.com"), |
| + 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 strings.Contains(out, "unresolved") { |
| + t.Fatalf("the unit pass printed about an exec-backed route's overlay, which it never delivers or Gets:\n%s", out) |
| + } |
| + if !strings.Contains(out, "exec-backed") { |
| + t.Fatalf("expected the ordinary exec-backed notice (this route still has a default bundle), got:\n%s", out) |
| + } |
| + if _, err := os.Stat(filepath.Join(envDir, "book.example.com.env")); !os.IsNotExist(err) { |
| + t.Fatalf("expected no env file for an exec-backed route, stat err = %v", err) |
| + } |
| +} |
| + |
| // Two independently corrupt overlays are two independent failures, not one. |
| // The listing itself succeeds (both hosts are named), so the "once per app" |
| // dedup for a failed LISTING must not swallow the second route's Get |
diff --git a/internal/appconfig/sync.go b/internal/appconfig/sync.go| index ca6a5a727..5e7135fdc 100644 |
| --- a/internal/appconfig/sync.go |
| +++ b/internal/appconfig/sync.go |
| @@ -181,37 +181,56 @@ func Sync(ctx context.Context, reg *registry.Registry, s release.Store, priv *ec |
| // 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, instETag, err := overlayFor(k.acct, k.app, rt.Host) |
| - if err != nil { |
| - var listErr *instanceOverlayListErr |
| - if errors.As(err, &listErr) { |
| - // Said ONCE per (account, app), not once per route: the |
| - // listing this failed on is cached per app inside the |
| - // loader, so every route of the app, across every |
| - // environment it is bound to, fails identically and for |
| - // the same reason; a line each would bury the cause under |
| - // its own repetitions on a box with many routes. |
| - ak := appKey{k.acct, k.app} |
| - if !said[ak] { |
| - said[ak] = true |
| - // rt.Host anchors the line to a real route (the |
| - // first one this pass happened to reach), so an |
| - // operator scanning the log has something 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), env files left as-is: %v\n", k.acct, k.app, rt.Host, err) |
| + // |
| + // Fetched ONLY for unit-backed routes (final review, plan 4): an |
| + // exec-backed route never reaches syncHost below — SyncExec renders |
| + // its overlay and reports its own listing/Get failures on it — so a |
| + // Get paid here would be charged and discarded for every exec-backed |
| + // route that has one, which is exactly the shape step 5 targets. A |
| + // Get or parse failure paid here for such a route would also print |
| + // "env file left as-is" about a file this pass never writes, a claim |
| + // SyncExec already makes correctly on its own. Deliberately NOT |
| + // hoisting the rt.Backing check up to gate this whole block, though: |
| + // that would make the "exec-backed" print below fire for an |
| + // exec-backed route with no config at all, which it does not do |
| + // today — the check stays exactly where it always was, and only the |
| + // overlay fetch itself is narrowed. |
| + var inst *Manifest |
| + var instETag string |
| + if rt.Backing == registry.BackingUnit { |
| + var err error |
| + inst, instETag, err = overlayFor(k.acct, k.app, rt.Host) |
| + if err != nil { |
| + var listErr *instanceOverlayListErr |
| + if errors.As(err, &listErr) { |
| + // Said ONCE per (account, app), not once per route: the |
| + // listing this failed on is cached per app inside the |
| + // loader, so every route of the app, across every |
| + // environment it is bound to, fails identically and for |
| + // the same reason; a line each would bury the cause under |
| + // its own repetitions on a box with many routes. |
| + ak := appKey{k.acct, k.app} |
| + if !said[ak] { |
| + said[ak] = true |
| + // rt.Host anchors the line to a real route (the |
| + // first one this pass happened to reach), so an |
| + // operator scanning the log has something 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), env files left as-is: %v\n", k.acct, k.app, rt.Host, err) |
| + } |
| + } else { |
| + // A per-host Get or parse failure, unlike the listing |
| + // above, is specific to THIS route: the listing already |
| + // succeeded (it named this host), so another route of |
| + // the same app can have a perfectly good overlay, or a |
| + // failure of its own for a different reason. Folding this |
| + // into the app-level line would leave that route's stale |
| + // credential file invisible, so it is reported every time. |
| + fmt.Printf(" %s: instance config unresolved, env file left as-is: %v\n", rt.Host, err) |
| } |
| - } else { |
| - // A per-host Get or parse failure, unlike the listing |
| - // above, is specific to THIS route: the listing already |
| - // succeeded (it named this host), so another route of |
| - // the same app can have a perfectly good overlay, or a |
| - // failure of its own for a different reason. Folding this |
| - // into the app-level line would leave that route's stale |
| - // credential file invisible, so it is reported every time. |
| - fmt.Printf(" %s: instance config unresolved, env file left as-is: %v\n", rt.Host, err) |
| + continue |
| } |
| - continue |
| } |
| m := r.m |
| if m == nil && inst == nil { |