amadan
restart: complete a pre-narrowing baseline, and stop the scope door widening
Review fixes on top of the narrowed-restart branch. The design is unchanged:
narrowed requests still go to a sibling restart-scope.json through a separate
console door, because boxes act on restart-gen.json's etag MOVING and never
read its content.
1. A silent drop on exactly the shape the feature was built for.
Every box that has run the app-wide restart already has a restart-marks.json,
written before narrowing existed and so carrying no `scopes` key. The agent's
seed is create-only and will not touch it, so the ROOT pass had no narrowed
position — and a missing position reads as a first sight, which records the
newest request and cycles nothing. Exec-shaped routes survived (the agent
bounces its own children out of memory); unit-shaped ones — the relay and the
router these flags exist for — were dropped silently, having answered
"restart requested", until the next root pass, worst case the hourly timer.
seedRestartMarks now REPORTS an incomplete baseline and restartSeed asks for
one root pass. It writes nothing new: the pass it starts is the ordinary one,
under the same lock, against the same marks file and the same seed rules, so
it changes when that pass runs and never what it decides. It cannot replay
(dispatch is decided by the per-app marks, which nothing here writes) and it
cannot drop (it shrinks the window from an hour to the moment the agent arms
— the same floor seedRestartMarks already buys the app-wide half). Merging
mem.scope in from the agent was the alternative and was rejected: the agent
cannot take the root pass's lock (root creates that file, and opening it is
the ROOT pass's writability test), and a lock-free read-modify-write would
race a running pass and revert the advance it had just made.
TestRestartPassReadsAMarksFileThatPredatesNarrowing pinned the drop as
correct and is gone. In its place: the whole handover walked end to end —
arm over a pre-narrowing file, let the pass it asks for run, then make the
request that used to vanish — plus the genuinely-first-sight case (no marks
file at all) and the marker touch through restartSeed itself.
2. The scope door could still widen. A non-dry-run POST whose selector fields
were absent — a bare {}, or a misspelled key like "hosts" — fell through to
RestartScopeApp and cycled the whole app through the very door that exists
because the other door widens. Both halves are closed: the body is decoded
with DisallowUnknownFields (readJSONStrictLimit), so an unknown key is a 400
naming the field rather than a silently dropped selector; and an app-wide
scope is refused unless dry_run is set, which is the one form of app-wide
this door is for. writeRestartScope refuses it too, as belt to those braces.
3. docs/site/cli.md overclaimed fleet reach: resolveRestartScope refuses a
--host absent from the local registry and an --environment no local route
reads, so on a multi-box fleet those cannot be requested at all. Says so now,
in the console file's own words.
4. The scope door's 404 gate now asserts the body is byte-identical to the
app-wide door's, read from that door rather than spelled out, and covers an
app-scoped service credential reaching past its own app — with the other half
pinned too, so that 404 cannot quietly become "services are turned away".
5. A direct agent-side test for restartScopePass's lost-position re-baseline,
which was only reachable through the root pass.
Gate: go build ./..., go vet ./..., go test ./... -count=1, gofmt -l . all clean.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XxxmnFjdNxLc3M16ooFG95
7 files changed,
+498
−74
docs/site/cli.md+10 −7internal/console/apirestartscope.go+58 −15internal/console/apirestartscope_test.go+98 −15internal/console/authcli.go+34 −1internal/host/restart.go+105 −13internal/host/restart_test.go+15 −4internal/host/restartscope_test.go+178 −19
CI passed — run details
diff --git a/docs/site/cli.md b/docs/site/cli.md| index c0f4dc233..3b017d0d6 100644 |
| --- a/docs/site/cli.md |
| +++ b/docs/site/cli.md |
| @@ -286,13 +286,16 @@ this is not a verb to guess on. A selector that matches nothing is refused the |
| same way, and the refusal lists the hosts or environments the app actually |
| has. Nothing is ever widened back to the whole app. |
| -Two limits worth knowing. The console resolves and lists routes from its own |
| -box's registry, so on a multi-box fleet the printed set is what that box can |
| -see — the request itself reaches every box, and each applies the same selector |
| -to its own routes. And a console or a box that predates these flags does not |
| -understand a narrowed request: the console answers that it has no such door, |
| -and a box that has not been rolled yet simply does nothing. Neither one turns |
| -it into a restart of everything. |
| +Two limits worth knowing. The console resolves the selector against its own |
| +box's registry, and it refuses a selector nothing there matches — so on a |
| +multi-box fleet you can only ask for a `--host` or an `--environment` that at |
| +least one route on the console's own box reads, and the printed set is what |
| +that box can see. A request it does accept is a bucket object every box |
| +converges on, and each box applies the same selector to its own routes; those |
| +other routes are cycled without appearing in the list. And a console or a box |
| +that predates these flags does not understand a narrowed request: the console |
| +answers that it has no such door, and a box that has not been rolled yet simply |
| +does nothing. Neither one turns it into a restart of everything. |
| ### carlos schedule |
diff --git a/internal/console/apirestartscope.go b/internal/console/apirestartscope.go| index b4939ec07..6ce6f3fb2 100644 |
| --- a/internal/console/apirestartscope.go |
| +++ b/internal/console/apirestartscope.go |
| @@ -16,6 +16,10 @@ |
| // |
| // token — cliUser's one 401, before any name is resolved. |
| // account+app 404 — apiAccountApp, so probing teaches a stranger nothing. |
| +// body shape — 400 for a field this door does not declare. A renamed |
| +// or misspelled key that was merely IGNORED would leave a |
| +// body with no selector in it, and no selector means the |
| +// whole app. |
| // selector shape — 400 for a malformed host or environment, before the |
| // registry is opened. |
| // registry — 503 when this console serves none. A narrowed request |
| @@ -26,6 +30,11 @@ |
| // app actually has. A typo in --environment that fell |
| // through to an app-wide restart would be a worse version |
| // of the bug being fixed. |
| +// narrowing — 400 for a WRITE with no selector at all. The app-wide |
| +// case is answered here only as a --dry-run, which is how |
| +// somebody sees the blast radius before typing the real |
| +// verb; performing it belongs to the app-wide door, whose |
| +// object every box in the fleet already reads. |
| // rate limit — the shared `write` budget, keyed on the address, and |
| // only for a request that WRITES. A dry run is a read of |
| // this box's own registry, the same read `carlos routes` |
| @@ -35,13 +44,18 @@ |
| // |
| // SCOPE: like handleAPIRoutes and the config door's restart notice, this |
| // console reads its OWN box's registry, so in a multi-box fleet the other |
| -// boxes' routes are not represented in what it resolves or reports. The |
| -// REQUEST still reaches them — it is a bucket object every box converges on — |
| -// and each box matches it against its own routes, which is what makes |
| -// `--environment relay` mean "everything reading that bundle" fleet-wide. What |
| -// this console cannot do is LIST what it cannot see, so the answer says which |
| -// registry it looked at rather than letting a short list read as a complete |
| -// one. |
| +// boxes' routes are not represented in what it resolves or reports. Once a |
| +// request is accepted it does reach them — it is a bucket object every box |
| +// converges on — and each box matches it against its own routes, which is what |
| +// makes `--environment relay` mean "everything reading that bundle" |
| +// fleet-wide. |
| +// |
| +// The limit that cuts the other way, and is worth stating plainly because the |
| +// resolution gate above is what causes it: a selector NO route on this box |
| +// matches is refused, so a --host or an --environment that only another box in |
| +// the fleet knows about cannot be requested here at all. That direction is |
| +// safe — a refusal, never a wider restart — but it is a real limit, not just a |
| +// reporting one, and docs/site/cli.md says so in the same words. |
| package console |
| import ( |
| @@ -90,7 +104,12 @@ func (s *Server) handleAPIRestartScope(w http.ResponseWriter, r *http.Request) { |
| Environment string `json:"environment"` |
| DryRun bool `json:"dry_run"` |
| } |
| - if !readJSONLimit(w, r, &req, restartScopeMaxBytes) { |
| + // STRICT, and that is a safety property rather than pedantry: a key this |
| + // door does not declare — `"hosts"` for `"host"`, a field a future client |
| + // renamed — would otherwise be dropped, leaving a body with no selector in |
| + // it, which resolves to the whole app. A door that exists because the other |
| + // door widens must not widen on a typo of its own. |
| + if !readJSONStrictLimit(w, r, &req, restartScopeMaxBytes) { |
| return |
| } |
| host := strings.TrimSpace(req.Host) |
| @@ -150,6 +169,19 @@ func (s *Server) handleAPIRestartScope(w http.ResponseWriter, r *http.Request) { |
| jsonError(w, http.StatusBadRequest, refusal) |
| return |
| } |
| + // An app-wide request may be DESCRIBED here and never PERFORMED here. The |
| + // dry run is the whole reason the app-wide case reaches this door at all — |
| + // it is how somebody sees the blast radius before typing the real thing — |
| + // but a write with no selector in it would cycle every route the app owns |
| + // through the very door that exists because the other one widens. The |
| + // app-wide restart has its own door, which every box in the fleet |
| + // understands; this one only ever narrows. |
| + if scope.Kind == release.RestartScopeApp && !req.DryRun { |
| + jsonError(w, http.StatusBadRequest, |
| + "this door only narrows a restart: name --host or --environment, or add --dry-run to see what a whole-app "+ |
| + "restart would cycle. To actually cycle the whole app, run: carlos restart --app "+app) |
| + return |
| + } |
| answer := map[string]any{ |
| "account": acct.Sqid, "app": app, |
| "scope": map[string]string{"kind": scope.Kind, "value": scope.Value}, |
| @@ -197,6 +229,11 @@ func (s *Server) handleAPIRestartScope(w http.ResponseWriter, r *http.Request) { |
| // the resolved hosts: the row is a record of what somebody asked for, and the |
| // set it resolved to is a property of one box's registry at one moment, which |
| // would read as a claim about the fleet the moment a route moved. |
| +// |
| +// The app-wide branch is kept although the handler no longer lets an app-wide |
| +// WRITE reach the audit at all: if a later edit ever restores one, the row it |
| +// writes must read as the app-wide door's own row rather than as |
| +// "requested a restart of app (cli)". |
| func restartAuditWords(scope release.RestartScope) string { |
| if scope.Kind == release.RestartScopeApp { |
| return "requested a restart (cli)" // the app-wide door's own wording |
| @@ -204,14 +241,20 @@ func restartAuditWords(scope release.RestartScope) string { |
| return fmt.Sprintf("requested a restart of %s %s (cli)", scope.Kind, scope.Value) |
| } |
| -// writeRestartScope puts the request where the boxes will find it. An app-wide |
| -// request goes to the OLD object, unchanged and for the old reason: every box |
| -// in the fleet already reads it, including ones that predate narrowing. The |
| -// narrowed kinds go to the log only new boxes read, which is what keeps an old |
| -// box from widening a request it cannot understand into a whole-app restart. |
| +// writeRestartScope puts the request where the boxes will find it: the log |
| +// only new boxes read, which is what keeps an old box from widening a request |
| +// it cannot understand into a whole-app restart. |
| +// |
| +// An app-wide scope is REFUSED here rather than forwarded to the old object, |
| +// which is belt to the handler's braces. The handler has already turned that |
| +// body away, and this is the check that keeps the refusal true if a later edit |
| +// moves or loses it: an app-wide restart is the other door's write, and a |
| +// second way to reach it through the narrowing door is exactly the accident |
| +// this file was built to prevent. The caller answers a store error with "could |
| +// not request the restart — nothing was cycled", which is the honest outcome. |
| func (s *Server) writeRestartScope(ctx context.Context, acct, app string, scope release.RestartScope) error { |
| - if scope.Kind == release.RestartScopeApp { |
| - return release.TouchRestartGen(ctx, s.cfg.Releases, acct, app) |
| + if scope.Kind != release.RestartScopeHost && scope.Kind != release.RestartScopeEnvironment { |
| + return fmt.Errorf("restart scope: %q is not a narrowing — an app-wide restart belongs to the app-wide door", scope.Kind) |
| } |
| _, err := release.AppendRestartScope(ctx, s.cfg.Releases, acct, app, scope) |
| return err |
diff --git a/internal/console/apirestartscope_test.go b/internal/console/apirestartscope_test.go| index a9f72e8f5..d6f31486e 100644 |
| --- a/internal/console/apirestartscope_test.go |
| +++ b/internal/console/apirestartscope_test.go |
| @@ -18,6 +18,7 @@ import ( |
| "github.com/carlosframework/platform/internal/registry" |
| "github.com/carlosframework/platform/internal/release" |
| + "github.com/carlosframework/platform/internal/svcrole" |
| ) |
| func scopeURL(fx *consoleFixture, app string) string { |
| @@ -276,35 +277,117 @@ func TestAPIRestartScopeDryRunOfTheWholeApp(t *testing.T) { |
| } |
| } |
| -// TestAPIRestartScopeAppWideUsesTheOldObject: an app-wide request through this |
| -// door writes the generation EVERY box reads, old ones included — narrowing is |
| -// what the new log is for, and putting an app-wide request there would hide it |
| -// from the fleet that most needs to see it. |
| -func TestAPIRestartScopeAppWideUsesTheOldObject(t *testing.T) { |
| +// TestAPIRestartScopeRefusesAWriteWithNoSelector is the door's own |
| +// never-widen rule, turned on itself. |
| +// |
| +// This path exists BECAUSE the app-wide door widens a request it cannot read. |
| +// A body with no selector in it — a bare `{}`, or a client that renamed a |
| +// field — resolves to the whole app, and performing that here would be the |
| +// nineteen-route accident committed through the door built to prevent it. So |
| +// the app-wide case is answered only as a --dry-run (which is how somebody |
| +// sees the blast radius before typing the real verb), and a WRITE with no |
| +// narrowing is refused, naming the door that does own it. |
| +func TestAPIRestartScopeRefusesAWriteWithNoSelector(t *testing.T) { |
| fx, token := scopeFixture(t, "code-scope-appwide") |
| - if status, _, out := postScope(t, fx, token, `{}`); status != http.StatusOK { |
| - t.Fatalf("app-wide scope = %d %s, want 200", status, out) |
| + status, _, out := postScope(t, fx, token, `{}`) |
| + if status != http.StatusBadRequest { |
| + t.Fatalf("an app-wide write through the scope door = %d %s, want 400", status, out) |
| + } |
| + if !strings.Contains(string(out), "carlos restart --app") { |
| + t.Fatalf("refusal %s does not name the door that cycles the whole app", out) |
| } |
| - if restartGenEtag(t, fx, "titogo") == "" { |
| - t.Fatal("an app-wide request through the scope door did not move the object every box watches") |
| + if got := restartGenEtag(t, fx, "titogo"); got != "" { |
| + t.Fatalf("a refused app-wide write still moved the object every box watches (%q) — that is the whole app cycled", got) |
| } |
| if len(scopeLog(t, fx, "titogo").Requests) != 0 { |
| - t.Fatal("an app-wide request was written to the narrowed log, where old boxes cannot see it") |
| + t.Fatal("a refused app-wide write still appended to the narrowed log") |
| + } |
| + if got := serial(t, fx); got != 0 { |
| + t.Fatalf("serial = %d after a refusal, want it untouched", got) |
| + } |
| + // The dry run of the same body is still answered: that is the one form of |
| + // app-wide this door is for. |
| + if status, res, out := postScope(t, fx, token, `{"dry_run":true}`); status != http.StatusOK || res.Requested { |
| + t.Fatalf("app-wide dry run = %d %s (%+v), want 200 and nothing requested", status, out, res) |
| + } |
| +} |
| + |
| +// TestAPIRestartScopeRefusesAFieldItDoesNotKnow is the same rule one layer |
| +// out. A misspelled or renamed selector key that was merely IGNORED would |
| +// leave a body with no selector in it, which is a whole-app restart requested |
| +// by a client that asked for one route. The refusal names the field, because |
| +// the fix is a spelling. |
| +func TestAPIRestartScopeRefusesAFieldItDoesNotKnow(t *testing.T) { |
| + fx, token := scopeFixture(t, "code-scope-unknown-field") |
| + status, _, out := postScope(t, fx, token, `{"hosts":"relay2.go.tito.io"}`) |
| + if status != http.StatusBadRequest { |
| + t.Fatalf(`{"hosts":...} = %d %s, want 400 — an ignored key here is an app-wide restart`, status, out) |
| + } |
| + if !strings.Contains(string(out), "hosts") { |
| + t.Fatalf("refusal %s does not name the field, so nobody can find the typo", out) |
| + } |
| + if restartGenEtag(t, fx, "titogo") != "" || len(scopeLog(t, fx, "titogo").Requests) != 0 { |
| + t.Fatal("a body this door refused still wrote a request") |
| } |
| } |
| // TestAPIRestartScopeGateIsTheFamilys404: an app the account never claimed, a |
| -// stranger's token and an unknown account are one answer, so probing this door |
| -// teaches nobody anything. |
| +// stranger's token and a credential bound to another app are ONE answer, byte |
| +// for byte the answer the app-wide door gives — this repo's rule for these |
| +// gates is byte-identical, not merely same-status, because a differently |
| +// shaped 404 is an oracle that says "this door exists for you, that app does |
| +// not". |
| func TestAPIRestartScopeGateIsTheFamilys404(t *testing.T) { |
| fx, _ := scopeFixture(t, "code-scope-gate") |
| + // The old door's refusal, read from the old door rather than spelled out |
| + // here, so the two cannot drift apart without this failing. |
| eve := cliToken(t, fx, "eve@keymail.dev", "code-scope-gate-eve") |
| - if status, _ := postBody(t, scopeURL(fx, "titogo"), eve, `{"environment":"relay"}`); status != http.StatusNotFound { |
| - t.Fatalf("a stranger got %d, want 404", status) |
| + oldStatus, oldBody := postBody(t, fx.ts.URL+"/api/cli/apps/bab/titogo/restart", eve, "") |
| + if oldStatus != http.StatusNotFound { |
| + t.Fatalf("the app-wide door answered a stranger %d, want 404", oldStatus) |
| + } |
| + for _, c := range []struct { |
| + name string |
| + url string |
| + token string |
| + }{ |
| + {"a stranger", scopeURL(fx, "titogo"), eve}, |
| + // An app-scoped service credential reaching PAST its own app. The |
| + // binding is checked before the app is even looked up, so "not your |
| + // app" and "no such app" stay the same answer. |
| + {"a credential bound to another app", scopeURL(fx, "titogo"), |
| + serviceToken(t, fx.st, OpsAccountID, "kassbot", "kass", svcrole.Operate)}, |
| + {"an app this account never claimed", scopeURL(fx, "nosuchapp"), |
| + cliToken(t, fx, "alice@keymail.dev", "code-scope-gate-alice")}, |
| + } { |
| + status, body := postBody(t, c.url, c.token, `{"environment":"relay"}`) |
| + if status != oldStatus || string(body) != string(oldBody) { |
| + t.Fatalf("%s got %d %q, want the app-wide door's own %d %q", |
| + c.name, status, body, oldStatus, oldBody) |
| + } |
| } |
| - if len(scopeLog(t, fx, "titogo").Requests) != 0 { |
| + if len(scopeLog(t, fx, "titogo").Requests) != 0 || restartGenEtag(t, fx, "titogo") != "" { |
| t.Fatal("a refused caller still wrote a request") |
| } |
| + if got := serial(t, fx); got != 0 { |
| + t.Fatalf("serial = %d after refusals, want it untouched", got) |
| + } |
| +} |
| + |
| +// TestAPIRestartScopeServiceCredentialReachesItsOwnApp is the other half of |
| +// the case above: the 404 there is about the app binding, not about services |
| +// being turned away from this door. An Operate credential bound to titogo |
| +// narrows a restart of titogo exactly as a member does. |
| +func TestAPIRestartScopeServiceCredentialReachesItsOwnApp(t *testing.T) { |
| + fx, _ := scopeFixture(t, "code-scope-svc-own") |
| + tok := serviceToken(t, fx.st, OpsAccountID, "titogobot", "titogo", svcrole.Operate) |
| + status, res, out := postScope(t, fx, tok, `{"host":"bill.go.tito.io"}`) |
| + if status != http.StatusOK { |
| + t.Fatalf("a credential bound to titogo = %d %s, want 200", status, out) |
| + } |
| + if !res.Requested || res.Scope.Kind != "host" || res.Scope.Value != "bill.go.tito.io" { |
| + t.Fatalf("answer = %+v, want the narrowed request", res) |
| + } |
| } |
| // TestAPIRestartScopeNeedsARegistry: a console that cannot see routes cannot |
diff --git a/internal/console/authcli.go b/internal/console/authcli.go| index 90daa8932..c44a82887 100644 |
| --- a/internal/console/authcli.go |
| +++ b/internal/console/authcli.go |
| @@ -829,18 +829,51 @@ func readJSON(w http.ResponseWriter, r *http.Request, out any) bool { |
| // its own 413 rather than hiding inside "malformed JSON body" — the caller's |
| // fix (split the request, shorten the value) is nothing like a JSON fix. |
| func readJSONLimit(w http.ResponseWriter, r *http.Request, out any, limit int64) bool { |
| + return decodeJSONBody(w, r, out, limit, false) |
| +} |
| + |
| +// readJSONStrictLimit is readJSONLimit that also refuses a field the struct |
| +// does not declare, with a 400 that NAMES it. |
| +// |
| +// It is for the doors where an unrecognised key would otherwise be dropped and |
| +// the request acted on as if it had not been sent — which, at a door whose job |
| +// is to NARROW something, silently widens it. The narrowed restart door |
| +// (apirestartscope.go) is the case it was written for: a client that renames |
| +// `host` to `hosts` would, without this, POST a body with no selector in it |
| +// and be answered by a restart of every route the app owns. A door that exists |
| +// because another door widens must not itself widen on a typo. |
| +// |
| +// The unknown-field check is a prefix match on encoding/json's own message |
| +// because the package gives no typed error for it; the message is stable and |
| +// already names the field, which is the whole thing the caller needs. |
| +func readJSONStrictLimit(w http.ResponseWriter, r *http.Request, out any, limit int64) bool { |
| + return decodeJSONBody(w, r, out, limit, true) |
| +} |
| + |
| +// decodeJSONBody is the shared body of the two above: content type, byte cap, |
| +// decode, and one refusal per failure shape. |
| +func decodeJSONBody(w http.ResponseWriter, r *http.Request, out any, limit int64, strict bool) bool { |
| ct, _, _ := strings.Cut(r.Header.Get("Content-Type"), ";") |
| if strings.TrimSpace(strings.ToLower(ct)) != "application/json" { |
| jsonError(w, http.StatusUnsupportedMediaType, "send a JSON body with Content-Type: application/json") |
| return false |
| } |
| r.Body = http.MaxBytesReader(w, r.Body, limit) |
| - if err := json.NewDecoder(r.Body).Decode(out); err != nil { |
| + dec := json.NewDecoder(r.Body) |
| + if strict { |
| + dec.DisallowUnknownFields() |
| + } |
| + if err := dec.Decode(out); err != nil { |
| var tooLarge *http.MaxBytesError |
| if errors.As(err, &tooLarge) { |
| jsonError(w, http.StatusRequestEntityTooLarge, "request body too large") |
| return false |
| } |
| + if strict && strings.HasPrefix(err.Error(), "json: unknown field ") { |
| + jsonError(w, http.StatusBadRequest, err.Error()+ |
| + " — this door refuses a body it does not fully understand rather than acting on the part it does") |
| + return false |
| + } |
| jsonError(w, http.StatusBadRequest, "malformed JSON body") |
| return false |
| } |
diff --git a/internal/host/restart.go b/internal/host/restart.go| index e4e0f5c58..757f7b740 100644 |
| --- a/internal/host/restart.go |
| +++ b/internal/host/restart.go |
| @@ -275,9 +275,15 @@ type restartMarks struct { |
| // two binaries that roll at different times. A binary that predates the |
| // field ignores it on read (encoding/json drops unknown keys) and DROPS |
| // it on write — which costs a downgraded box exactly one re-baseline, |
| - // after which it cycles nothing it was not asked to. The other direction |
| - // is free: a marks file with no `scopes` key is a box that has never |
| - // looked at the log, which is a first sight, which is a seed. |
| + // after which it cycles nothing it was not asked to. |
| + // |
| + // The other direction is free of REPLAYS but not of drops, and the |
| + // difference matters: a marks file with no `scopes` key is a box that has |
| + // never looked at the log, which is a first sight, which is a seed — safe, |
| + // and exactly wrong for a narrowed request that lands before that seed is |
| + // written. Since EVERY box that ran the app-wide feature has such a file, |
| + // that window is closed from the agent's side rather than left to the |
| + // hourly floor; see seedRestartMarks' needRootPass. |
| Scopes map[string]string `json:"scopes,omitempty"` |
| } |
| @@ -367,7 +373,9 @@ func loadRestartMarks(path string) (marks *restartMarks, known bool) { |
| } |
| // A file written before narrowed restarts existed carries no `scopes` key |
| // at all; that is a first sight of every app's log, not an empty memory of |
| - // one, and the seed rules below turn it into exactly that. |
| + // one, and the seed rules below turn it into exactly that. Which is why |
| + // the agent asks for a pass that performs that seed as it arms, instead of |
| + // leaving a request typed in the meantime to meet it — scopeBaselineIncomplete. |
| if m.Scopes == nil { |
| m.Scopes = map[string]string{} |
| } |
| @@ -439,14 +447,40 @@ func (m *restartMarks) save(path string) error { |
| // Absent objects ARE recorded, with restartAbsent: "looked, nothing there" is |
| // the fact that makes an app's first ever request movement rather than a first |
| // sight. |
| -func seedRestartMarks(path string, mem *restartMemory) error { |
| +// |
| +// needRootPass is the SECOND half of the handover, and it exists because |
| +// create-only cannot repair a baseline that is merely INCOMPLETE. Every box |
| +// that ran the app-wide feature already has a marks file, written before |
| +// narrowed restarts existed and so carrying no `scopes` key at all. This |
| +// function will not touch it — that is the whole create-only rule — so the |
| +// narrowed mark stays missing until a ROOT pass records it, and until then |
| +// restartScopeLeg reads every app as a first sight and cycles nothing for a |
| +// request the CLI has already answered "restart requested". Exec-shaped routes |
| +// survive that (the agent bounces its own children from mem.scope), but |
| +// unit-shaped ones — Tito's relay and router, the shape these flags exist for |
| +// — are dropped silently, for as long as it takes the next root pass to run: |
| +// worst case the hourly timer. |
| +// |
| +// So when the baseline on disk is missing a narrowed mark for an app this box |
| +// serves, say so, and let restartSeed ask for ONE root pass. That pass is the |
| +// same pass carlos-adopt.timer runs anyway, under the same lock, against the |
| +// same marks file and the same seed rules — this changes WHEN it runs, never |
| +// what it decides — so it can neither replay a request (it dispatches on the |
| +// per-app marks, which nothing here writes) nor drop one (it strictly shrinks |
| +// the window in which a narrowed request meets a pass with no narrowed |
| +// baseline, from an hour to the moment the agent arms, which is the same floor |
| +// seedRestartMarks already buys the app-wide half). |
| +func seedRestartMarks(path string, mem *restartMemory) (needRootPass bool, err error) { |
| if mem == nil || len(mem.gen) == 0 { |
| - return nil // nothing was seen; nothing may be claimed |
| + return false, nil // nothing was seen; nothing may be claimed |
| } |
| if _, err := os.Stat(path); err == nil { |
| - return nil // the root pass (or an earlier agent) already has a baseline |
| + // The root pass (or an earlier agent) already has a baseline — but it |
| + // may predate narrowing, and a half-baseline is what silently eats a |
| + // narrowed request. |
| + return scopeBaselineIncomplete(path, mem), nil |
| } else if !errors.Is(err, os.ErrNotExist) { |
| - return err |
| + return false, err |
| } |
| m := newRestartMarks() |
| for k, etag := range mem.gen { |
| @@ -461,21 +495,58 @@ func seedRestartMarks(path string, mem *restartMemory) error { |
| } |
| raw, err := json.MarshalIndent(m, "", " ") |
| if err != nil { |
| - return err |
| + return false, err |
| } |
| raw = append(raw, '\n') |
| f, err := os.OpenFile(path, os.O_WRONLY|os.O_CREATE|os.O_EXCL, 0o644) |
| if err != nil { |
| if errors.Is(err, os.ErrExist) { |
| - return nil // lost the race to the root pass; its mark is the better one |
| + // Lost the race to the root pass; its mark is the better one — but |
| + // a pass running an OLD binary writes no scopes key, so the same |
| + // question has to be asked of what actually landed. |
| + return scopeBaselineIncomplete(path, mem), nil |
| } |
| - return err |
| + return false, err |
| } |
| _, writeErr := f.Write(raw) |
| if err := f.Close(); err != nil && writeErr == nil { |
| writeErr = err |
| } |
| - return writeErr |
| + // The file this call just wrote carries both memories, so there is nothing |
| + // left for a root pass to record. |
| + return false, writeErr |
| +} |
| + |
| +// scopeBaselineIncomplete reports whether the marks file on disk leaves the |
| +// ROOT pass with no narrowed-restart position for an app this box serves. |
| +// |
| +// Two shapes count, and they are the same fault: |
| +// |
| +// - the file does not load (missing, unreadable, unparseable), so the next |
| +// pass re-baselines everything — which is right, and worth doing NOW |
| +// rather than at the hourly floor, since a re-baselining pass dispatches |
| +// nothing; |
| +// - the file loads but has no `scopes` entry for some app in this sweep, |
| +// which is exactly what a marks file written before narrowing existed |
| +// looks like. |
| +// |
| +// It reads and never writes. The agent cannot take restartPass's lock — that |
| +// lock file is created by root and doubles as the ROOT pass's writability |
| +// test — and a lock-free read-modify-write here would race a running pass and |
| +// revert the advance it had just made, which is the one accident the lock |
| +// exists to prevent. Reporting the gap and letting the pass that owns the file |
| +// fill it in is the whole fix. |
| +func scopeBaselineIncomplete(path string, mem *restartMemory) bool { |
| + marks, known := loadRestartMarks(path) |
| + if !known { |
| + return true |
| + } |
| + for k := range mem.scope { |
| + if _, seen := marks.Scopes[restartMarkKey(k[0], k[1])]; !seen { |
| + return true |
| + } |
| + } |
| + return false |
| } |
| // restartPass is the ROOT half: for every app on this box whose restart |
| @@ -685,6 +756,14 @@ func restartRequestLeg(ctx context.Context, routes []registry.Route, store relea |
| // sight — safe) and blocks the marks seed entirely, for the reason |
| // seedRestartMarks and restartPass both give: a baseline with holes in it is |
| // the thing that manufactures restarts. |
| +// |
| +// The one thing it may ask for is a root pass, and only when the baseline on |
| +// disk is INCOMPLETE — see seedRestartMarks' needRootPass. That is the case of |
| +// every box already carrying a pre-narrowing marks file, and asking is what |
| +// keeps the first narrowed request after a roll from being answered "restart |
| +// requested" and then cycling nothing unit-shaped. It is one marker touch, |
| +// once, at arming: the pass it starts is the ordinary one, and once that pass |
| +// has recorded the narrowed marks no later agent start asks again. |
| func restartSeed(ctx context.Context, reg *registry.Registry, store release.Store, dd string, mem *restartMemory) { |
| routes, err := reg.List() |
| if err != nil { |
| @@ -698,7 +777,20 @@ func restartSeed(ctx context.Context, reg *registry.Registry, store release.Stor |
| log.Printf("agent: restart baseline: %d apps unreadable: %v", len(readErrs), readErrs[0]) |
| return |
| } |
| - if err := seedRestartMarks(restartMarksPath(dd), mem); err != nil { |
| + needRootPass, err := seedRestartMarks(restartMarksPath(dd), mem) |
| + if err != nil { |
| log.Printf("agent: seed restart marks: %v", err) |
| + return |
| + } |
| + if !needRootPass { |
| + return |
| + } |
| + if err := requestRootRestart(dd); err != nil { |
| + // Latency, not correctness: carlos-adopt.timer's hourly pass records |
| + // the same marks, it just leaves the window open for longer. |
| + log.Printf("agent: ask for a baselining root restart pass: %v", err) |
| + return |
| } |
| + log.Printf("agent: restart baseline: the root pass has no narrowed-restart mark for at least one app on this box " + |
| + "(a marks file written before narrowed restarts existed) — asked for one root pass to record it") |
| } |
diff --git a/internal/host/restart_test.go b/internal/host/restart_test.go| index e00997eca..bf302619a 100644 |
| --- a/internal/host/restart_test.go |
| +++ b/internal/host/restart_test.go |
| @@ -647,9 +647,15 @@ func TestSeedRestartMarksIsCreateOnly(t *testing.T) { |
| mem.scope = map[[2]string]string{ |
| {"bab", "eleven"}: release.RestartScopeNone, |
| } |
| - if err := seedRestartMarks(path, mem); err != nil { |
| + need, err := seedRestartMarks(path, mem) |
| + if err != nil { |
| t.Fatalf("seed: %v", err) |
| } |
| + if need { |
| + // The file this seed just wrote carries both memories, so there is |
| + // nothing for a root pass to fill in. |
| + t.Fatal("a seed that wrote the whole baseline still asked for a root pass") |
| + } |
| m := marksOf(t, path) |
| if m.Apps[restartMarkKey("bab", "eleven")] != "etag-1" || |
| m.Apps[restartMarkKey("bab", "titogo")] != restartAbsent { |
| @@ -663,8 +669,10 @@ func TestSeedRestartMarksIsCreateOnly(t *testing.T) { |
| } |
| mem.gen[[2]string{"bab", "eleven"}] = "etag-2" |
| - if err := seedRestartMarks(path, mem); err != nil { |
| + if need, err := seedRestartMarks(path, mem); err != nil { |
| t.Fatalf("second seed: %v", err) |
| + } else if need { |
| + t.Fatal("a complete baseline on disk still asked for a root pass — every agent start would ask") |
| } |
| if got := marksOf(t, path).Apps[restartMarkKey("bab", "eleven")]; got != "etag-1" { |
| t.Fatalf("mark = %q after a second seed, want the original %q — seeding must never overwrite", got, "etag-1") |
| @@ -677,8 +685,10 @@ func TestSeedRestartMarksIsCreateOnly(t *testing.T) { |
| // the box later gains as movement. |
| func TestSeedRestartMarksRefusesAnEmptyBaseline(t *testing.T) { |
| path := filepath.Join(t.TempDir(), "restart-marks.json") |
| - if err := seedRestartMarks(path, newRestartMemory()); err != nil { |
| + if need, err := seedRestartMarks(path, newRestartMemory()); err != nil { |
| t.Fatalf("seed: %v", err) |
| + } else if need { |
| + t.Fatal("a sweep that saw nothing asked for a root pass; it may claim nothing at all") |
| } |
| if _, err := os.Stat(path); !errors.Is(err, os.ErrNotExist) { |
| t.Fatalf("an empty sweep wrote %s (err %v), want no file at all", path, err) |
| @@ -699,7 +709,8 @@ func TestSeededBaselineIsWhatTheRootPassTrusts(t *testing.T) { |
| // The agent's first tick: baseline in memory, baseline on disk. |
| mem := newRestartMemory() |
| restartGenPass(ctx, routes, s, mem.gen) |
| - if err := seedRestartMarks(restartMarksPath(dd), mem); err != nil { |
| + restartScopePass(ctx, routes, s, mem.scope) |
| + if _, err := seedRestartMarks(restartMarksPath(dd), mem); err != nil { |
| t.Fatalf("seed: %v", err) |
| } |
diff --git a/internal/host/restartscope_test.go b/internal/host/restartscope_test.go| index 37db219a0..2e54ecd6d 100644 |
| --- a/internal/host/restartscope_test.go |
| +++ b/internal/host/restartscope_test.go |
| @@ -8,6 +8,7 @@ package host |
| import ( |
| "context" |
| "encoding/json" |
| + "errors" |
| "os" |
| "path/filepath" |
| "slices" |
| @@ -92,6 +93,38 @@ func TestRestartScopePassCarriesEveryRequestInAWindow(t *testing.T) { |
| } |
| } |
| +// TestRestartScopePassReBaselinesALostPosition is the agent's own half of the |
| +// rolled-off-mark rule, exercised directly rather than through the root pass: |
| +// a box away long enough for its position to leave the log has no honest |
| +// answer to "what did I miss", so it records where the log now stands and asks |
| +// for nothing. Replaying instead would cycle routes for requests served days |
| +// ago; a dropped one is recovered by typing the verb again. |
| +func TestRestartScopePassReBaselinesALostPosition(t *testing.T) { |
| + ctx, s := genKit(t) |
| + routes := titogoRoutes() |
| + req := askScoped(t, ctx, s, "bab", "titogo", release.RestartScopeEnvironment, "relay") |
| + |
| + // This box acted up to a request that has since been trimmed off the front |
| + // of the log. |
| + cache := map[[2]string]string{{"bab", "titogo"}: "rolled-off"} |
| + work, errs := restartScopePass(ctx, routes, s, cache) |
| + if len(errs) != 0 { |
| + t.Fatalf("unexpected read errors: %v", errs) |
| + } |
| + if len(work) != 0 { |
| + t.Fatalf("a lost position asked for %+v — it must re-baseline instead of replaying the log", work) |
| + } |
| + if got := cache[[2]string{"bab", "titogo"}]; got != req.ID { |
| + t.Fatalf("cache = %q, want the re-baseline to the log's newest %q", got, req.ID) |
| + } |
| + // And the NEXT request is acted on: re-baselining is not a box giving up. |
| + next := askScoped(t, ctx, s, "bab", "titogo", release.RestartScopeHost, "bill.example.com") |
| + work, _ = restartScopePass(ctx, routes, s, cache) |
| + if len(work) != 1 || len(work[0].requests) != 1 || work[0].requests[0].ID != next.ID { |
| + t.Fatalf("work = %+v, want the one request made after the re-baseline", work) |
| + } |
| +} |
| + |
| // TestScopedHostsNarrowsToTheSelector is the whole complaint, tested: one |
| // credential delivered to one bundle must cycle the routes reading that |
| // bundle, not every route the app owns. |
| @@ -350,36 +383,162 @@ func TestRestartPassReBaselinesALostPosition(t *testing.T) { |
| } |
| } |
| -// TestRestartPassReadsAMarksFileThatPredatesNarrowing is the on-disk half of |
| -// the additive rule. A marks file written by the binary before this feature |
| -// has no `scopes` key at all: that must read as "this box has never looked at |
| -// the log" — a first sight, so a seed — and never as an empty memory that |
| -// makes every request in the log look new. |
| -func TestRestartPassReadsAMarksFileThatPredatesNarrowing(t *testing.T) { |
| +// ---- the handover on a box that predates narrowing --------------------------- |
| + |
| +// writePreNarrowingMarks puts on disk exactly what the binary before this |
| +// feature wrote: schema, apps, and no `scopes` key at all. Every box that has |
| +// ever run the app-wide restart has one of these. |
| +func writePreNarrowingMarks(t *testing.T, path string, apps map[string]string) { |
| + t.Helper() |
| + raw, err := json.Marshal(map[string]any{"schema": 1, "apps": apps}) |
| + if err != nil { |
| + t.Fatalf("marshal old marks: %v", err) |
| + } |
| + if err := os.WriteFile(path, raw, 0o644); err != nil { |
| + t.Fatalf("write old marks: %v", err) |
| + } |
| +} |
| + |
| +// armAgent is one agent start: the two silent sweeps that fill its memory, and |
| +// the seed that either writes the root pass's baseline or reports that the one |
| +// already on disk is missing a narrowed mark. |
| +func armAgent(t *testing.T, ctx context.Context, routes []registry.Route, s release.Store, dd string) (*restartMemory, bool) { |
| + t.Helper() |
| + mem := newRestartMemory() |
| + if _, errs := restartGenPass(ctx, routes, s, mem.gen); len(errs) != 0 { |
| + t.Fatalf("gen sweep: %v", errs) |
| + } |
| + if _, errs := restartScopePass(ctx, routes, s, mem.scope); len(errs) != 0 { |
| + t.Fatalf("scope sweep: %v", errs) |
| + } |
| + need, err := seedRestartMarks(restartMarksPath(dd), mem) |
| + if err != nil { |
| + t.Fatalf("seed: %v", err) |
| + } |
| + return mem, need |
| +} |
| + |
| +// TestAPreNarrowingMarksFileIsCompletedBeforeTheNextRequestIsDropped is the |
| +// shape of every production box on the day this rolls, and the drop it used to |
| +// cause. |
| +// |
| +// The marks file was written before narrowed restarts existed, so it has |
| +// `apps` and no `scopes`. The agent's create-only seed will not touch it, |
| +// which leaves the ROOT pass with no narrowed position — and a missing |
| +// position reads as a FIRST SIGHT, so that pass records the log's newest id |
| +// and cycles nothing. An operator who ran `carlos restart --host relay2` in |
| +// that window was told "restart requested" and got no restart: the exec-shaped |
| +// routes survive because the agent bounces its own children out of memory, and |
| +// the unit-shaped ones — the relay and the router these flags exist for — are |
| +// the ones that vanish. |
| +// |
| +// The fix is the seed REPORTING the gap so that one root pass records the |
| +// marks. This walks the whole handover: arm, let that pass run, then ask. |
| +func TestAPreNarrowingMarksFileIsCompletedBeforeTheNextRequestIsDropped(t *testing.T) { |
| ctx, s := genKit(t) |
| dd := t.TempDir() |
| - routes := titogoRoutes() |
| + routes := titogoRoutes() // unit-shaped: only the root pass can cycle these |
| marks := restartMarksPath(dd) |
| - askScoped(t, ctx, s, "bab", "titogo", release.RestartScopeEnvironment, "relay") |
| - |
| - // Exactly what the old binary wrote: schema, apps, nothing else. |
| - old := map[string]any{"schema": 1, "apps": map[string]string{ |
| + // A request from before the roll, already in the log. Nothing here may |
| + // replay it. |
| + askScoped(t, ctx, s, "bab", "titogo", release.RestartScopeEnvironment, "router") |
| + writePreNarrowingMarks(t, marks, map[string]string{ |
| restartMarkKey("bab", "titogo"): genAbsent, // no app-wide object; the old binary recorded the absence |
| - }} |
| - raw, err := json.Marshal(old) |
| - if err != nil { |
| - t.Fatalf("marshal: %v", err) |
| + }) |
| + |
| + if _, need := armAgent(t, ctx, routes, s, dd); !need { |
| + t.Fatal("the agent saw a marks file with no narrowed mark and asked for nothing — " + |
| + `the next narrowed request would be answered "requested" and cycle nothing unit-shaped`) |
| } |
| - if err := os.WriteFile(marks, raw, 0o644); err != nil { |
| - t.Fatalf("write old marks: %v", err) |
| + if got := marksOf(t, marks).Scopes; len(got) != 0 { |
| + t.Fatalf("the seed wrote %v into an existing marks file — it must only report, never race the root pass's own write", got) |
| } |
| + |
| + // The root pass that touch asks for. It records and cycles nothing: the |
| + // request already in the log is this box's history, not work it owes. |
| rec := &recordingRestart{} |
| restartPass(ctx, routes, s, marks, rec.fn) |
| if len(rec.hosts) != 0 { |
| - t.Fatalf("an upgraded box cycled %v on its first read of the log — that is a replay", rec.hosts) |
| + t.Fatalf("the baselining pass cycled %v — a box completing its baseline must replay nothing", rec.hosts) |
| } |
| if marksOf(t, marks).Scopes[restartMarkKey("bab", "titogo")] == "" { |
| - t.Fatal("the upgraded box recorded no scope mark, so it would re-seed forever") |
| + t.Fatal("the pass recorded no scope mark, so the box would re-seed forever") |
| + } |
| + |
| + // And NOW the operator types the verb. This is the request that used to be |
| + // dropped. |
| + askScoped(t, ctx, s, "bab", "titogo", release.RestartScopeHost, "relay2.example.com") |
| + rec = &recordingRestart{} |
| + restartPass(ctx, routes, s, marks, rec.fn) |
| + if !slices.Equal(rec.hosts, []string{"relay2.example.com"}) { |
| + t.Fatalf("cycled %v, want the one route the operator named", rec.hosts) |
| + } |
| + |
| + // A second agent start over a now-complete baseline asks for nothing: the |
| + // repair is one pass per box, not one per agent start. |
| + if _, need := armAgent(t, ctx, routes, s, dd); need { |
| + t.Fatal("an agent restarting over a complete baseline asked for another root pass") |
| + } |
| +} |
| + |
| +// TestSeedOnAFreshBoxAsksForNoRootPass is the genuinely-first-sight case, kept |
| +// separate because it is the one the create-only seed already handles: with no |
| +// marks file at all the agent writes BOTH memories itself, so there is nothing |
| +// left for a root pass to fill in — and the next narrowed request is acted on. |
| +func TestSeedOnAFreshBoxAsksForNoRootPass(t *testing.T) { |
| + ctx, s := genKit(t) |
| + dd := t.TempDir() |
| + routes := titogoRoutes() |
| + marks := restartMarksPath(dd) |
| + askScoped(t, ctx, s, "bab", "titogo", release.RestartScopeEnvironment, "router") // history |
| + |
| + if _, need := armAgent(t, ctx, routes, s, dd); need { |
| + t.Fatal("a seed that wrote the whole baseline itself still asked for a root pass") |
| + } |
| + if marksOf(t, marks).Scopes[restartMarkKey("bab", "titogo")] == "" { |
| + t.Fatal("the fresh seed recorded no scope mark") |
| + } |
| + |
| + askScoped(t, ctx, s, "bab", "titogo", release.RestartScopeHost, "bill.example.com") |
| + rec := &recordingRestart{} |
| + restartPass(ctx, routes, s, marks, rec.fn) |
| + if !slices.Equal(rec.hosts, []string{"bill.example.com"}) { |
| + t.Fatalf("cycled %v, want the named route — the seeded baseline is what the root pass trusts", rec.hosts) |
| + } |
| +} |
| + |
| +// TestRestartSeedAsksTheRootPassThroughTheMarker is the same repair through |
| +// the real entry point, so the marker file — the only way this unprivileged |
| +// process can reach a root pass at all — is pinned rather than assumed. |
| +func TestRestartSeedAsksTheRootPassThroughTheMarker(t *testing.T) { |
| + ctx, s := genKit(t) |
| + reg, err := registry.Open(filepath.Join(t.TempDir(), "registry.db")) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + defer reg.Close() |
| + for _, rt := range titogoRoutes() { |
| + if err := reg.Upsert(rt); err != nil { |
| + t.Fatal(err) |
| + } |
| + } |
| + |
| + dd := t.TempDir() |
| + writePreNarrowingMarks(t, restartMarksPath(dd), map[string]string{ |
| + restartMarkKey("bab", "titogo"): genAbsent, |
| + }) |
| + restartSeed(ctx, reg, s, dd, newRestartMemory()) |
| + if _, err := os.Stat(filepath.Join(dd, restartRequestMarker)); err != nil { |
| + t.Fatalf("no marker after arming over a pre-narrowing baseline: %v — "+ |
| + "the root pass would record no narrowed mark until the hourly floor", err) |
| + } |
| + |
| + // A baseline this agent wrote itself leaves the marker alone: an agent that |
| + // touched it on every start would start a root pass on every start. |
| + fresh := t.TempDir() |
| + restartSeed(ctx, reg, s, fresh, newRestartMemory()) |
| + if _, err := os.Stat(filepath.Join(fresh, restartRequestMarker)); !errors.Is(err, os.ErrNotExist) { |
| + t.Fatalf("the fresh seed touched the marker (err %v), want no request at all", err) |
| } |
| } |