amadan
appconfig: dedup the instance-overlay listing failure, not the per-host one
Review of plan 4 task 4 found said folding a per-host Get/parse error into the same one-line-per-app dedup meant for the cached listing failure: two routes of one app with two separately corrupt overlay objects produced one log line naming only the first, leaving the second route's stale file invisible. instanceOverlayLoader now wraps only its listing failure in instanceOverlayListErr; sync.go dedups on that via errors.As and reports a per-host failure every time instead. said is also narrowed from groupKey to appKey, matching the loader's own per-(account, app) cache and dropping a same-cause duplicate line per extra environment. countingStore.puts, dead in the original commit, is now asserted (Sync never writes the release store). instances.go's instanceOverlayLoader comment confirms SyncExec (task 6) does share it and notes why its unlocked index map is safe only because each pass builds its own closure.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
3 files changed,
+130
−26
internal/appconfig/instances.go+32 −10internal/appconfig/instances_test.go+61 −0internal/appconfig/sync.go+37 −16
diff --git a/internal/appconfig/instances.go b/internal/appconfig/instances.go| index 88ac66fab..301aad326 100644 |
| --- a/internal/appconfig/instances.go |
| +++ b/internal/appconfig/instances.go |
| @@ -262,21 +262,43 @@ func instanceOverlays(ctx context.Context, s release.Store, acct, app string) (m |
| return out, nil |
| } |
| +// instanceOverlayListErr marks an error as coming from instanceOverlayLoader's |
| +// cached LISTING (instanceOverlays) rather than from a per-host Get. The |
| +// listing is the one call every route of an app shares, so every route |
| +// fails identically and for the same reason when it errors — a caller may |
| +// dedup on that. A per-host Get or parse failure carries no such guarantee: |
| +// two routes can have two separately corrupt overlay objects, each failing |
| +// for its own reason, and folding both into one line would hide the second |
| +// one's failure entirely. errors.As is how a caller tells the two apart. |
| +type instanceOverlayListErr struct{ error } |
| + |
| +// Unwrap lets errors.Is/errors.As see through the wrapper to the listing |
| +// failure itself, the same courtesy every %w wrap in this package gives. |
| +func (e *instanceOverlayListErr) Unwrap() error { return e.error } |
| + |
| // instanceOverlayLoader is one pass's instance-layer reader, shared by both |
| -// sweeps so neither can drift from the other about what a host gets or what |
| -// it costs. One List per (account, app), cached for the pass including its |
| -// failure, then a Get only for a host that listing named. |
| +// sweeps (Sync and SyncExec) so neither can drift from the other about what |
| +// a host gets or what it costs. One List per (account, app), cached for the |
| +// pass including its failure, then a Get only for a host that listing |
| +// named. |
| // |
| -// A listing FAILURE is returned to the caller, never folded into "no |
| -// overlay". The two sweeps treat it the way they treat a bundle fetch |
| -// failure: leave the file exactly as it is, say so per route, retry next |
| -// pass. Folding it away would silently deliver the app-wide merge to an |
| -// instance that is meant to have its own credential, which is the one |
| -// outcome this layer exists to prevent. |
| +// A listing FAILURE is returned to the caller wrapped in |
| +// instanceOverlayListErr, never folded into "no overlay". The two sweeps |
| +// treat it the way they treat a bundle fetch failure: leave the file |
| +// exactly as it is, say so, retry next pass. Folding it away would silently |
| +// deliver the app-wide merge to an instance that is meant to have its own |
| +// credential, which is the one outcome this layer exists to prevent. |
| // |
| // A host the listing named but whose object is gone by the time of the Get |
| // is simply "no overlay": a delete racing the pass converges the route back |
| // to its group's merge, which is what the delete asked for. |
| +// |
| +// The index map below has no lock. That's safe only because each call to |
| +// this function returns a fresh closure over its own map, and both Sync and |
| +// SyncExec build one loader per pass and drive it from their own single |
| +// route loop — never two goroutines sharing one closure. A caller that ever |
| +// shared one instanceOverlayLoader value across concurrently running sweeps |
| +// would race on this map; don't. |
| func instanceOverlayLoader(ctx context.Context, s release.Store) func(acct, app, host string) (*Manifest, string, error) { |
| type appKey struct{ acct, app string } |
| type indexRes struct { |
| @@ -290,7 +312,7 @@ func instanceOverlayLoader(ctx context.Context, s release.Store) func(acct, app, |
| if !done { |
| hosts, err := instanceOverlays(ctx, s, acct, app) |
| if err != nil { |
| - err = fmt.Errorf("%s/%s: instance config listing failed: %w", acct, app, err) |
| + err = &instanceOverlayListErr{fmt.Errorf("%s/%s: instance config listing failed: %w", acct, app, err)} |
| } |
| idx = indexRes{hosts: hosts, err: err} |
| index[ak] = idx |
diff --git a/internal/appconfig/instances_test.go b/internal/appconfig/instances_test.go| index 76265ecf1..94f488a19 100644 |
| --- a/internal/appconfig/instances_test.go |
| +++ b/internal/appconfig/instances_test.go |
| @@ -426,6 +426,19 @@ func (c *countingStore) n(m map[string]int, k string) int { |
| return m[k] |
| } |
| +// total sums every count in m, so a test can assert "this pass wrote |
| +// nothing to the release store at all" without naming every key it might |
| +// have touched. |
| +func (c *countingStore) total(m map[string]int) int { |
| + c.mu.Lock() |
| + defer c.mu.Unlock() |
| + n := 0 |
| + for _, v := range m { |
| + n += v |
| + } |
| + return n |
| +} |
| + |
| // syncInstanceKit is the unit-path fixture these tests share: a store, a |
| // registry, an env dir and a parsed key pair. |
| func syncInstanceKit(t *testing.T) (context.Context, release.Store, *registry.Registry, string, *ecdh.PrivateKey) { |
| @@ -532,6 +545,54 @@ func TestSyncCostsOneListingPerAppAndOneGetPerOverlay(t *testing.T) { |
| t.Fatalf("overlay Gets for %s = %d, want none: the listing already said it has no overlay", host, got) |
| } |
| } |
| + // A convergence pass reads the release store, it never writes to it — |
| + // every write this test triggers (SetEnv, ApplyInstance) went through |
| + // the unwrapped base store during setup, before s ever saw a call. |
| + if got := s.total(s.puts); got != 0 { |
| + t.Fatalf("Sync put %d object(s) to the release store, want 0: this pass only writes local env files", got) |
| + } |
| +} |
| + |
| +// 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 |
| +// failure — that would leave one of two stale credential files invisible, |
| +// which is exactly what this layer exists to surface. |
| +func TestSyncReportsEachRoutesOverlayGetFailureSeparately(t *testing.T) { |
| + ctx, s, reg, envDir, priv := syncInstanceKit(t) |
| + if err := SetEnv(ctx, s, "bab", "titogo", map[string]string{"APP_WIDE": "yes"}); err != nil { |
| + t.Fatalf("SetEnv: %v", err) |
| + } |
| + // Schema 2 is refused by getIfExistsAtETag ("newer than this binary |
| + // understands"), which is a Get-time failure specific to the object |
| + // read, not a listing failure — instanceOverlays only reads keys, never |
| + // their bodies, so the listing itself succeeds for both hosts. |
| + putInstanceRaw(t, ctx, s, "bab", "titogo", "book.example.com", &Manifest{Schema: 2, App: "titogo"}) |
| + putInstanceRaw(t, ctx, s, "bab", "titogo", "cart.example.com", &Manifest{Schema: 2, App: "titogo"}) |
| + unitRoute(t, reg, "book.example.com", "titogo", "bab", "") |
| + unitRoute(t, reg, "cart.example.com", "titogo", "bab", "") |
| + |
| + out := captureStdout(t, func() { |
| + if err := Sync(ctx, reg, s, priv, envDir, func(string) error { return nil }, nil); err != nil { |
| + t.Fatalf("Sync: %v", err) |
| + } |
| + }) |
| + for _, host := range []string{"book.example.com", "cart.example.com"} { |
| + if !strings.Contains(out, host) { |
| + t.Fatalf("no line for %s's overlay failure, one route's stale file went unreported:\n%s", host, out) |
| + } |
| + } |
| + if n := strings.Count(out, "instance config unresolved"); n != 2 { |
| + t.Fatalf("instance config unresolved lines = %d, want 2 (one per corrupt overlay), got:\n%s", n, out) |
| + } |
| + // Neither route had a file before this pass, and a Get failure must |
| + // leave that exactly as it is: no env file materializes from a resolved |
| + // group merge that an unresolved overlay was supposed to sit on top of. |
| + for _, host := range []string{"book.example.com", "cart.example.com"} { |
| + if _, err := os.Stat(filepath.Join(envDir, host+".env")); !os.IsNotExist(err) { |
| + t.Fatalf("%s.env exists after an unresolved overlay: %v", host, err) |
| + } |
| + } |
| } |
| // A listing that fails is not a determination. The route keeps its file and |
diff --git a/internal/appconfig/sync.go b/internal/appconfig/sync.go| index cd6e533e4..e473a89fb 100644 |
| --- a/internal/appconfig/sync.go |
| +++ b/internal/appconfig/sync.go |
| @@ -12,6 +12,7 @@ import ( |
| "bytes" |
| "context" |
| "crypto/ecdh" |
| + "errors" |
| "fmt" |
| "os" |
| "path/filepath" |
| @@ -149,9 +150,15 @@ func Sync(ctx context.Context, reg *registry.Registry, s release.Store, priv *ec |
| // per host, and it costs one listing per app plus a Get for each |
| // instance that actually has an overlay. |
| overlayFor := instanceOverlayLoader(ctx, s) |
| - // One line per app when its listing fails, not one per route: every |
| - // route of that app fails identically and for the same reason. |
| - said := map[groupKey]bool{} |
| + // One line per app when the LISTING fails, not one per route: every |
| + // route of that app fails identically and for the same reason, since |
| + // instanceOverlayLoader caches that failure per (account, app) — |
| + // independent of Environment, which is why this is keyed on appKey and |
| + // not groupKey. A per-host Get or parse failure is a DIFFERENT claim — |
| + // two routes can have two separately corrupt overlay objects, each |
| + // wrong for its own reason — so that case is never folded in here; see |
| + // the errors.As branch below. |
| + said := map[appKey]bool{} |
| for _, rt := range routes { |
| if !syncable(rt) { |
| continue |
| @@ -176,19 +183,33 @@ func Sync(ctx context.Context, reg *registry.Registry, s release.Store, priv *ec |
| // 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 |
| - // this failed on is per app, so every route of that app fails |
| - // identically and for the same reason; a line each would bury the |
| - // cause under its own repetitions on a box with many routes. The |
| - // loader already holds the error, so said is only about output. |
| - if !said[k] { |
| - said[k] = 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 |
| - // (account, app, environment) failed for the identical |
| - // reason and 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) |
| + 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) |
| } |
| continue |
| } |