amadan
appconfig: the restart notice knows an instance overlay shadows a bundle write (plan 4, task 8)
AwaitingRestart and allShadowed answer "did a write land a value nothing will read" and were written when there were two layers above a write rather than three. allShadowed is now variadic over every layer above the bundle a write touched (named bundle, instance overlay, or both), so a write to the default OR the named bundle can be recognised as shadowed by an instance overlay — closing the carve-out that only covered default-bundle writes to bound routes. The instance layer is consulted through the same instanceOverlayLoader the sync sweeps use, so the notice and the pass that writes the files never disagree about what sits above a bundle. Consulting it is gated on keys being non-empty: an empty keys means there is nothing to shadow (a write with no vars, e.g. flipping exec_delivery on), so a transient overlay-listing blip must not downgrade a route this call was certain about into Unresolved over a question it was never going to ask. Reporting an INSTANCE-layer write itself stays out of scope, per the plan's ruling: that is step 5's job, once something writes one.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
2 files changed,
+154
−19
internal/appconfig/awaitingrestart_test.go+100 −0internal/appconfig/syncexec.go+54 −19
diff --git a/internal/appconfig/awaitingrestart_test.go b/internal/appconfig/awaitingrestart_test.go| index 501ed1d92..ea3e750d0 100644 |
| --- a/internal/appconfig/awaitingrestart_test.go |
| +++ b/internal/appconfig/awaitingrestart_test.go |
| @@ -237,6 +237,106 @@ func TestAwaitingRestartUnknownExecDeliveryIsOff(t *testing.T) { |
| } |
| } |
| +// TestAwaitingRestartInstanceOverlayShadowsADefaultWrite: the third layer |
| +// shadows a default-bundle write the same way a named bundle does, and for |
| +// the same reason. The merged manifest does not move, so syncExecHost's |
| +// bytes.Equal writes nothing and there is no file for anyone to restart over. |
| +func TestAwaitingRestartInstanceOverlayShadowsADefaultWrite(t *testing.T) { |
| + ctx, s, _, _, _, _ := execKit(t) |
| + if err := SetExecDelivery(ctx, s, "bab", "relay", ExecDeliveryFile); err != nil { |
| + t.Fatalf("SetExecDelivery: %v", err) |
| + } |
| + if _, _, err := ApplyInstance(ctx, s, "bab", "relay", "connect.example.com", |
| + map[string]Var{"OWN_KEY": {Kind: "env", Value: "instance"}}, nil); err != nil { |
| + t.Fatalf("ApplyInstance: %v", err) |
| + } |
| + routes := []registry.Route{addrRoute("connect.example.com", "relay", "bab", "")} |
| + |
| + if got := AwaitingRestart(ctx, s, routes, "bab", "relay", "", []string{"OWN_KEY"}); !got.Empty() { |
| + t.Fatalf("got %+v, want nothing: OWN_KEY is shadowed by this instance's own layer", got) |
| + } |
| + got := AwaitingRestart(ctx, s, routes, "bab", "relay", "", []string{"OTHER_KEY"}) |
| + if len(got.Hosts) != 1 || got.Hosts[0].Host != "connect.example.com" { |
| + t.Fatalf("Hosts = %+v, want the route for a key its overlay does not state", got.Hosts) |
| + } |
| + got = AwaitingRestart(ctx, s, routes, "bab", "relay", "", []string{"OWN_KEY", "OTHER_KEY"}) |
| + if len(got.Hosts) != 1 { |
| + t.Fatalf("Hosts = %+v, want the route when any key is unshadowed", got.Hosts) |
| + } |
| +} |
| + |
| +// TestAwaitingRestartInstanceOverlayShadowsANamedBundleWrite is the case |
| +// that did not exist before the third layer: a write to the route's OWN |
| +// bound bundle used to be unshadowable, because nothing sat above it. |
| +// Something does now. |
| +func TestAwaitingRestartInstanceOverlayShadowsANamedBundleWrite(t *testing.T) { |
| + ctx, s, _, _, _, _ := execKit(t) |
| + if err := SetExecDelivery(ctx, s, "bab", "relay", ExecDeliveryFile); err != nil { |
| + t.Fatalf("SetExecDelivery: %v", err) |
| + } |
| + if _, err := ApplyEnvironment(ctx, s, "bab", "relay", "prod", |
| + map[string]Var{"K": {Kind: "env", Value: "environment"}}, nil); err != nil { |
| + t.Fatalf("ApplyEnvironment: %v", err) |
| + } |
| + if _, _, err := ApplyInstance(ctx, s, "bab", "relay", "connect.example.com", |
| + map[string]Var{"K": {Kind: "env", Value: "instance"}}, nil); err != nil { |
| + t.Fatalf("ApplyInstance: %v", err) |
| + } |
| + routes := []registry.Route{addrRoute("connect.example.com", "relay", "bab", "prod")} |
| + |
| + if got := AwaitingRestart(ctx, s, routes, "bab", "relay", "prod", []string{"K"}); !got.Empty() { |
| + t.Fatalf("got %+v, want nothing: K is shadowed by this instance's own layer", got) |
| + } |
| + if got := AwaitingRestart(ctx, s, routes, "bab", "relay", "prod", []string{"OTHER"}); len(got.Hosts) != 1 { |
| + t.Fatalf("Hosts = %+v, want the route for an unshadowed key", got.Hosts) |
| + } |
| +} |
| + |
| +// TestAwaitingRestartUnresolvedWhenTheOverlayCannotBeRead: a read this |
| +// function could not make is not a determination, on this layer as on the |
| +// other two — the route is named as unresolved rather than reported either |
| +// way. |
| +func TestAwaitingRestartUnresolvedWhenTheOverlayCannotBeRead(t *testing.T) { |
| + ctx, s, _, _, _, _ := execKit(t) |
| + if err := SetExecDelivery(ctx, s, "bab", "relay", ExecDeliveryFile); err != nil { |
| + t.Fatalf("SetExecDelivery: %v", err) |
| + } |
| + if _, _, err := ApplyInstance(ctx, s, "bab", "relay", "connect.example.com", |
| + map[string]Var{"OWN_KEY": {Kind: "env", Value: "instance"}}, nil); err != nil { |
| + t.Fatalf("ApplyInstance: %v", err) |
| + } |
| + blip := &failingKeyStore{Store: s, failKey: InstanceConfigKey("bab", "relay", "connect.example.com"), |
| + err: errors.New("s3: transient 503 SlowDown")} |
| + routes := []registry.Route{addrRoute("connect.example.com", "relay", "bab", "")} |
| + got := AwaitingRestart(ctx, blip, routes, "bab", "relay", "", []string{"OWN_KEY"}) |
| + if len(got.Hosts) != 0 || len(got.Unresolved) != 1 || got.Unresolved[0] != "connect.example.com" { |
| + t.Fatalf("got %+v, want the host as unresolved and nothing claimed either way", got) |
| + } |
| +} |
| + |
| +// TestAwaitingRestartEmptyKeysNeverConsultsTheOverlay pins that an empty |
| +// keys (a write with no vars, e.g. flipping exec_delivery on) never asks the |
| +// instance layer anything: there is nothing for it to shadow, per keys' own |
| +// doc comment, so a blip reading it must not downgrade a route this call was |
| +// certain about into Unresolved over a question it was never going to ask. |
| +func TestAwaitingRestartEmptyKeysNeverConsultsTheOverlay(t *testing.T) { |
| + ctx, s, _, _, _, _ := execKit(t) |
| + if err := SetExecDelivery(ctx, s, "bab", "relay", ExecDeliveryFile); err != nil { |
| + t.Fatalf("SetExecDelivery: %v", err) |
| + } |
| + if _, _, err := ApplyInstance(ctx, s, "bab", "relay", "connect.example.com", |
| + map[string]Var{"OWN_KEY": {Kind: "env", Value: "instance"}}, nil); err != nil { |
| + t.Fatalf("ApplyInstance: %v", err) |
| + } |
| + blip := &failingKeyStore{Store: s, failKey: InstanceConfigKey("bab", "relay", "connect.example.com"), |
| + err: errors.New("s3: transient 503 SlowDown")} |
| + routes := []registry.Route{addrRoute("connect.example.com", "relay", "bab", "")} |
| + got := AwaitingRestart(ctx, blip, routes, "bab", "relay", "", nil) |
| + if len(got.Hosts) != 1 || got.Hosts[0].Host != "connect.example.com" || len(got.Unresolved) != 0 { |
| + t.Fatalf("got %+v, want the route named in Hosts: an unreadable overlay is irrelevant when keys is empty", got) |
| + } |
| +} |
| + |
| // TestAwaitingRestartSorted pins determinism: registry order is not stable, |
| // and a notice whose lines reshuffle between identical runs is one people |
| // stop reading. |
diff --git a/internal/appconfig/syncexec.go b/internal/appconfig/syncexec.go| index 1bb114138..7618d719a 100644 |
| --- a/internal/appconfig/syncexec.go |
| +++ b/internal/appconfig/syncexec.go |
| @@ -855,6 +855,11 @@ func (r RestartPending) Empty() bool { return len(r.Hosts) == 0 && len(r.Unresol |
| // short list read as a complete one. |
| func AwaitingRestart(ctx context.Context, s release.Store, routes []registry.Route, acct, app, envName string, keys []string) RestartPending { |
| var out RestartPending |
| + // The instance layer, through the same loader the sweeps use, so this |
| + // notice and the pass that writes the files agree about what sits above |
| + // a bundle. One listing for the app, then a Get per route that has an |
| + // overlay; both live for one call, like the caches below. |
| + overlayFor := instanceOverlayLoader(ctx, s) |
| // Both caches exist for the same reason SyncExec's do: a box with many |
| // TCP-addr routes on one bundle must not cost more than one read for |
| // it. Unlike SyncExec's, they live for one call. |
| @@ -926,19 +931,33 @@ func AwaitingRestart(ctx context.Context, s release.Store, routes []registry.Rou |
| if !v.On { |
| continue |
| } |
| - // The shadowing test, and the reason keys is a parameter: a write |
| - // to the DEFAULT bundle changes what a bound route resolves to |
| - // only for keys that route's own bundle does not state. Where every |
| + // The shadowing test, and the reason keys is a parameter: where every |
| // touched key is shadowed, the merged manifest is byte-identical, |
| // syncExecHost's bytes.Equal writes nothing, and naming the route |
| // would send someone to restart a process for a value it was never |
| - // going to see. |
| - // |
| - // Only for a default-bundle write reaching a BOUND route: a write |
| - // to the route's own bundle is never shadowed, and an unbound |
| - // route has nothing above it. |
| - if envName == "" && rt.Environment != "" && len(keys) > 0 && allShadowed(keys, eb.m) { |
| - continue |
| + // going to see. An empty keys disables this whole test (nothing to |
| + // shadow — see keys' own doc comment), so the instance layer is |
| + // consulted only when it could actually change the verdict: a route |
| + // whose overlay is unreadable must not be downgraded to Unresolved |
| + // over a call that was never going to ask it anything. |
| + if len(keys) > 0 { |
| + // Which layers sit ABOVE the bundle that was written, for this |
| + // route. A write cannot change what a route resolves to for a |
| + // key some higher layer states, and the third layer sits above |
| + // both of the other two. |
| + inst, _, instErr := overlayFor(acct, app, rt.Host) |
| + if instErr != nil { |
| + out.Unresolved = append(out.Unresolved, rt.Host) |
| + continue |
| + } |
| + var above []*Manifest |
| + if envName == "" && rt.Environment != "" { |
| + above = append(above, eb.m) |
| + } |
| + above = append(above, inst) |
| + if allShadowed(keys, above...) { |
| + continue |
| + } |
| } |
| out.Hosts = append(out.Hosts, RestartHost{Host: rt.Host, Unit: rt.Unit}) |
| } |
| @@ -950,16 +969,32 @@ func AwaitingRestart(ctx context.Context, s release.Store, routes []registry.Rou |
| return out |
| } |
| -// allShadowed reports whether every one of keys is STATED by m — the test |
| -// that decides whether a default-bundle write can have changed what a route |
| -// bound to m actually resolves to. An unset key counts the same as a set |
| -// one: removing a key the overlay shadows changes the merge by nothing. |
| -func allShadowed(keys []string, m *Manifest) bool { |
| - if m == nil { |
| - return false |
| - } |
| +// allShadowed reports whether every one of keys is STATED by at least one |
| +// of the layers above the bundle a write touched — the test that decides |
| +// whether that write can have changed what a route actually resolves to. An |
| +// unset key counts the same as a set one: removing a key a higher layer |
| +// shadows changes the merge by nothing. |
| +// |
| +// above is variadic because how many layers sit over a write depends on |
| +// which bundle was written: a default-bundle write to a bound route is |
| +// under both the named bundle and the instance overlay, a named-bundle |
| +// write is under the overlay alone, and a write to the overlay itself has |
| +// nothing over it. A nil layer (no bundle of that kind for this route) is |
| +// skipped rather than treated as stating nothing, so an empty list answers |
| +// false and the route is named. |
| +func allShadowed(keys []string, above ...*Manifest) bool { |
| for _, k := range keys { |
| - if _, ok := m.Vars[k]; !ok { |
| + stated := false |
| + for _, m := range above { |
| + if m == nil { |
| + continue |
| + } |
| + if _, ok := m.Vars[k]; ok { |
| + stated = true |
| + break |
| + } |
| + } |
| + if !stated { |
| return false |
| } |
| } |