amadan
appconfig: narrow three doc-comment claims the final review found overclaiming
- instances.go: LoadMergedHost's doc comment claimed its equivalence with the sweeps' own resolution "is pinned by a test rather than by hope", but the test only covers a valid host. The two actually disagree for a host that fails ValidInstanceHost: LoadMergedHost errors, while instanceOverlayLoader (what the sweeps use) treats it as "no overlay" and the route converges on its group's merge. Narrowed the claim and named the disagreement. - configack.go: InstanceConfigDelivery tested a.Schema < ConfigAckSchema for the unknown-agent verdict. When ConfigAckSchema climbs for some unrelated reason, every schema-1 ack -- which carries a perfectly good InstanceConfigETag -- would read as "cannot answer" for a question it can answer. Introduced instanceConfigETagSchema, the schema at which this specific field became trustworthy, and compare against that instead: it happens to equal ConfigAckSchema today but answers a different question, and the two must not be allowed to drift together silently on the next bump. - syncexec.go: the mass-removal guard's `inst == nil` clause (this branch) is narrower than the guard used to be, and is backstopped by the #344 account-mismatch guard everywhere that narrowing matters -- except for a custom-domain route, since release.HostAccountLabel can only derive an account label from a platform-domain hostname. Recorded that residual, accepted gap in the comment so it isn't rediscovered as a surprise later. Final review, plan 4 (tenant email senders, per-instance config layer).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
3 files changed,
+53
−2
internal/appconfig/configack.go+21 −1internal/appconfig/instances.go+14 −1internal/appconfig/syncexec.go+18 −0
diff --git a/internal/appconfig/configack.go b/internal/appconfig/configack.go| index 6914a73da..36ccdc14d 100644 |
| --- a/internal/appconfig/configack.go |
| +++ b/internal/appconfig/configack.go |
| @@ -43,6 +43,22 @@ import ( |
| // agent starts stating something a reader may need to depend on. |
| const ConfigAckSchema = 1 |
| +// instanceConfigETagSchema is the ack schema at which InstanceConfigETag |
| +// first became a field an agent could write — 1, the schema this plan |
| +// introduced it at. InstanceConfigDelivery compares against THIS constant, |
| +// not against ConfigAckSchema: the two happen to be equal today, but they |
| +// answer different questions, and conflating them is a trap the next schema |
| +// bump would fall into silently. ConfigAckSchema is "what schema does this |
| +// BINARY write" and is meant to climb every time the ack states something |
| +// new; instanceConfigETagSchema is "at what schema did THIS field become |
| +// trustworthy" and has no reason to climb again unless the field itself is |
| +// ever redefined. A bump to ConfigAckSchema for some unrelated addition |
| +// (say, schema 2 adds a field about something else entirely) must not |
| +// demote every schema-1 ack — which carries a perfectly good |
| +// InstanceConfigETag — to InstanceConfigUnknownAgent for a question it can |
| +// already answer. |
| +const instanceConfigETagSchema = 1 |
| + |
| // ConfigAck is one route's config-adoption acknowledgement, written after |
| // config sync converges its env/config file — see ConfigAckKey for where. |
| type ConfigAck struct { |
| @@ -132,11 +148,15 @@ const ( |
| // etags are decidable: an agent too old to read the layer and a new agent |
| // rendering an overlay whose vars the reader does not recognise deliver the |
| // same set of names (design §6.5). |
| +// |
| +// The schema test below is against instanceConfigETagSchema, deliberately |
| +// NOT ConfigAckSchema — see that constant's own doc comment for the hazard |
| +// a future schema bump would otherwise walk into. |
| func InstanceConfigDelivery(a *ConfigAck, want string) string { |
| switch { |
| case a == nil: |
| return InstanceConfigNotYetDelivered |
| - case a.Schema < ConfigAckSchema: |
| + case a.Schema < instanceConfigETagSchema: |
| return InstanceConfigUnknownAgent |
| case a.InstanceConfigETag == want: |
| return InstanceConfigDelivered |
diff --git a/internal/appconfig/instances.go b/internal/appconfig/instances.go| index 301aad326..686c4b29e 100644 |
| --- a/internal/appconfig/instances.go |
| +++ b/internal/appconfig/instances.go |
| @@ -185,7 +185,20 @@ func DeleteInstanceConfig(ctx context.Context, s release.Store, acct, app, host |
| // one group base for every route that shares it and would re-fetch |
| // config.json per route if they came through here. They reach the same |
| // answer through instanceOverlays and applyInstanceOverlay instead, and the |
| -// equivalence is pinned by a test rather than by hope. |
| +// equivalence for a VALID host is pinned by a test rather than by hope — |
| +// TestSyncRendersTheInstanceOverlayForItsOwnRouteOnly compares a sweep's |
| +// rendered file against this function's own answer for the same host. |
| +// |
| +// That equivalence does NOT extend to a host that fails ValidInstanceHost. |
| +// This function refuses one outright (see the guard below): a hostname a |
| +// caller cannot even ask about is an error here. The sweeps never make that |
| +// call at all — instanceOverlayLoader treats such a host as "not named by |
| +// the listing", which reads as "no overlay", and the route converges on its |
| +// group's merge same as any other unconfigured instance. A caller that |
| +// wants the sweep's own answer for a host it cannot yet prove valid (step |
| +// 5's write path, before ApplyInstance's own ValidInstanceHost gate runs) |
| +// must not substitute this function for that reason: the two disagree here, |
| +// on purpose, and only one of them is what a box will actually deliver. |
| func LoadMergedHost(ctx context.Context, s release.Store, acct, app, envName, host string) (*Manifest, string, error) { |
| if !ValidInstanceHost(host) { |
| return nil, "", fmt.Errorf("config: invalid instance host %q", host) |
diff --git a/internal/appconfig/syncexec.go b/internal/appconfig/syncexec.go| index c3a2a2dd4..834a10f46 100644 |
| --- a/internal/appconfig/syncexec.go |
| +++ b/internal/appconfig/syncexec.go |
| @@ -468,6 +468,24 @@ func SyncExec(ctx context.Context, reg *registry.Registry, s release.Store, priv |
| // An overlay is configuration, so a route that has one is never |
| // "absent" for the MASS-REMOVAL guard below, however its group base |
| // resolved: writing a file a bundle asked for is not a removal. |
| + // |
| + // This narrowing (the `inst == nil` clause, added by this branch) |
| + // makes the guard itself strictly narrower than it was before: a |
| + // route with an overlay now sails through where it used to be held |
| + // with everything else. That is backstopped, everywhere this |
| + // narrowing actually matters (r.m == nil, i.e. a mangled account |
| + // column made the group base read absent), by the #344 guard just |
| + // below — EXCEPT for a custom-domain route. release.HostAccountLabel |
| + // only derives a <sqid> to check from a PLATFORM-domain hostname; a |
| + // custom-domain host's label is "", the #344 check's `sq != ""` test |
| + // is false, and the guard simply does not fire. So a custom-domain |
| + // route with an overlay, on a box whose account column is mangled in |
| + // the exact way #344 exists to catch, has no backstop here at all — |
| + // it sails through this guard AND past #344, and the wholesale |
| + // overlay-alone replacement both guards were meant to prevent for it |
| + // can still happen. Known and accepted (final review, plan 4): the |
| + // fix would need a host-independent way to validate the account |
| + // column, which #344 does not have either. |
| if r.m == nil && inst == nil && skipRemovals { |
| continue |
| } |