carlos / platform Public

host: a named-unit restart that failed must not stamp a version

Review IMPORTANT 3. The named-unit arm swallowed systemctl's exit status, so
a well-formed but WRONG unit name — exit 5, "not loaded" — was
indistinguishable from a restart that worked: adoptHost stamped Version, the
edge served the new X-Carlos-Version, and `carlos deploy` reported live over
the old process. Worse than the bug this branch fixes, because the stamp was
then actively lying rather than merely unattended.

The arm now returns the failure, and adoptHost's existing error contract does
the rest: poison the version for the pass, roll the symlink back to the
version that IS running, skip SetVersions, print the failure — the next pass
retries and the header stays honest on the old version throughout.

Arms 2 and 3 keep fire-and-forget byte-identically, and the asymmetry is the
point. try-restart of a carlos-app@<host> unit CANNOT distinguish failure
from the ordinary case: a sleeping exec child reaches that arm under
RestartHibernating precisely because there is nothing there to cycle. A named
unit can — try-restart of a loaded, stopped unit exits 0 — so a non-zero exit
there means the stamp is wrong or the service will not come back.

Also corrects RunAdopt's new comment: appconfig.Sync skips every route whose
Backing is not BackingUnit before restarting anything, and a unit-backed route
is what `carlos route -unit` refuses to stamp, so config sync cannot reach
the named-unit arm at all. It shares the callback so there is one dispatch on
this pass, not two that could drift.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Paul Campbell 2 months ago 4100c3c58c3e355a2055660efa1a7a0618ae9d17 parent afc0c72

2 files changed, +147 −23

internal/host/agent.go +50 −15
internal/host/agent_test.go +97 −8

diff --git a/internal/host/agent.go b/internal/host/agent.go

index 1a281e02a..8b6928fb7 100644

--- a/internal/host/agent.go

+++ b/internal/host/agent.go

@@ -1541,17 +1541,34 @@ func statusTick(ctx context.Context, s release.Store, reg *registry.Registry, p

// which is why it is try-restart and not restart (a console an operator has

// deliberately stopped stays stopped).

-// Errors are logged, never returned: a failed restart must not poison the

-// adopted version for the other hosts in the pass (release/adopt.go's rule).

-func makeRestartRoute(reg *registry.Registry, tryRestart func(host string), tryRestartUnit func(unit string), bounce func(host string) bool) func(host string) error {

+// ERRORS split by arm, and the split is the point:

+//

+// Arms 2 and 3 log and return nil, unchanged: a failed try-restart of a

+// carlos-app@<host> unit must not poison the adopted version for the other

+// hosts in the pass (release/adopt.go's rule), and it cannot be told apart

+// from the ordinary case anyway — a sleeping exec child reaches arm 2 under

+// RestartHibernating precisely because there is nothing there to cycle.

+// Fire-and-forget is what makes that safe.

+//

+// Arm 1 RETURNS the failure, because for a named unit the ordinary case is

+// not indistinguishable: try-restart of a unit that is loaded and stopped

+// exits 0, so a non-zero exit means the name is wrong (exit 5, "not loaded")

+// or the service would not come back. Both are "the new build is NOT

+// running", and swallowing them would hand adoptHost a success it did not

+// have: it would stamp Version, the edge would serve the new

+// X-Carlos-Version, and `carlos deploy` would report live over the old

+// process — issue #110 again, now with the stamp actively lying rather than

+// merely unattended. Returned, adoptHost rolls the symlink back to the

+// version that IS running, skips SetVersions, prints the failure, and the

+// next pass retries; the header stays honest on the old version throughout.

+func makeRestartRoute(reg *registry.Registry, tryRestart func(host string), tryRestartUnit func(unit string) error, bounce func(host string) bool) func(host string) error {

return func(host string) error {

rt, err := reg.Get(host)

if err != nil {

log.Printf("adopt: registry lookup %s: %v (restarting as a unit)", host, err)

}

if err == nil && rt != nil && rt.Unit != "" {

- tryRestartUnit(rt.Unit)

- return nil

+ return tryRestartUnit(rt.Unit)

}

if err == nil && rt != nil && rt.Backing != registry.BackingUnit {

if bounce != nil && bounce(host) {

@@ -1581,20 +1598,30 @@ func systemctlTryRestart(host string) {

}

// systemctlTryRestartUnit is makeRestartRoute's NAMED-unit path: cycle the

-// unit the route itself names, iff it's running. Same best-effort, same

-// try-restart-not-restart reasoning as systemctlTryRestart above — a service

-// an operator has stopped stays stopped — and the same swallow-and-log, since

-// one route's failed restart must not poison the pass for the others.

+// unit the route itself names, iff it's running. Same try-restart-not-restart

+// reasoning as systemctlTryRestart above — a service an operator has stopped

+// stays stopped, and try-restart on a loaded-but-stopped unit exits 0.

+//

+// It LOGS AND RETURNS, where its carlos-app@ sibling only logs, and the

+// asymmetry is deliberate: this name came from an operator rather than from

+// activator.UnitName, so a non-zero exit here means the stamp is wrong or the

+// service will not come back — never the routine "nothing was running"

+// systemctlTryRestart has to tolerate. The journal line is kept as well as

+// the error because the error's own destination is a pass that prints one

+// summary line per host; the exit status and systemctl's own words belong in

+// the journal where an operator greps for the unit name.

// The unit reaches exec.Command as ONE argv element, so there is no shell for

// a name to escape into; the shape rail lives at the write (registry's

// ValidUnitName), where a typo can still be reported to the person who typed

// it.

-func systemctlTryRestartUnit(unit string) {

+func systemctlTryRestartUnit(unit string) error {

out, err := exec.Command("systemctl", "try-restart", unit).CombinedOutput()

if err != nil {

- log.Printf("adopt: try-restart %s: %v: %s (continuing)", unit, err, strings.TrimSpace(string(out)))

+ log.Printf("adopt: try-restart %s: %v: %s", unit, err, strings.TrimSpace(string(out)))

+ return fmt.Errorf("try-restart %s: %w: %s", unit, err, strings.TrimSpace(string(out)))

}

+ return nil

}

// rootRestartRoute is the root adopt pass's restart callback (RunAdopt, out of

@@ -1677,10 +1704,18 @@ func RunAdopt(args []string) {

if err != nil {

log.Fatalf("adopt: %v", err)

}

- // One callback for both halves below (config sync's restarts and the

- // release pass's), built once here now that it needs the registry — it

- // reads each route's `unit` column to decide what to cycle. See

- // rootRestartRoute for why the named-unit arm has to exist on THIS pass.

+ // One callback for both halves below, built once here now that it needs

+ // the registry — it reads each route's `unit` column to decide what to

+ // cycle. See rootRestartRoute for why the named-unit arm has to exist on

+ // THIS pass.

+ //

+ // Only the RELEASE pass can actually reach that arm. appconfig.Sync skips

+ // every route whose Backing is not BackingUnit before it restarts anything

+ // (an exec-backed route's config is delivered by the agent's in-process

+ // loop instead), and a unit-backed route is exactly what `carlos route

+ // -unit` refuses to stamp — so config sync sees no unit-named route by

+ // construction. It is handed the same callback anyway because there should

+ // be ONE restart dispatch on this pass, not two that could drift.

restart := rootRestartRoute(reg)

// Parsed here, before config sync, exactly as the full pass always has

// (a box with an unusable release key must die before it converges

diff --git a/internal/host/agent_test.go b/internal/host/agent_test.go

index a44b45427..00917f7a4 100644

--- a/internal/host/agent_test.go

+++ b/internal/host/agent_test.go

@@ -1,15 +1,18 @@

package host

import (

+ "bytes"

"context"

"crypto/ed25519"

"encoding/base64"

"errors"

+ "log"

"os"

"path/filepath"

"runtime"

"slices"

"sort"

+ "strings"

"testing"

"time"

@@ -518,7 +521,10 @@ func TestMakeRestartRouteDispatchesOnBacking(t *testing.T) {

var restarted, bounced []string

restart := makeRestartRoute(reg,

func(h string) { restarted = append(restarted, h) },

- func(u string) { t.Fatalf("named-unit arm fired for %q on a route that names no unit", u) },

+ func(u string) error {

+ t.Fatalf("named-unit arm fired for %q on a route that names no unit", u)

+ return nil

+ },

func(h string) bool { bounced = append(bounced, h); return true })

if err := restart("unit.test"); err != nil {

t.Fatalf("restart unit.test: %v", err)

@@ -541,7 +547,10 @@ func TestMakeRestartRouteUnknownHostFallsBackToUnit(t *testing.T) {

reg := restartTestRegistry(t)

var restarted []string

restart := makeRestartRoute(reg, func(h string) { restarted = append(restarted, h) },

- func(u string) { t.Fatalf("named-unit arm fired for %q on a host the registry cannot answer for", u) }, nil)

+ func(u string) error {

+ t.Fatalf("named-unit arm fired for %q on a host the registry cannot answer for", u)

+ return nil

+ }, nil)

if err := restart("ghost.test"); err != nil {

t.Fatalf("restart ghost.test: %v", err)

}

@@ -577,7 +586,10 @@ func TestMakeRestartRouteExecFallsThroughWhenNothingBounced(t *testing.T) {

}

var restarted []string

restart := makeRestartRoute(reg, func(h string) { restarted = append(restarted, h) },

- func(u string) { t.Fatalf("named-unit arm fired for %q on a route that names no unit", u) }, tc.bounce)

+ func(u string) error {

+ t.Fatalf("named-unit arm fired for %q on a route that names no unit", u)

+ return nil

+ }, tc.bounce)

if err := restart("exec.test"); err != nil {

t.Fatalf("restart exec.test: %v", err)

}

@@ -605,7 +617,7 @@ func TestMakeRestartRouteRestartsTheUnitARouteNames(t *testing.T) {

var units, restarted, bounced []string

restart := makeRestartRoute(reg,

func(h string) { restarted = append(restarted, h) },

- func(u string) { units = append(units, u) },

+ func(u string) error { units = append(units, u); return nil },

func(h string) bool { bounced = append(bounced, h); return true })

if err := restart("console.carloku.com"); err != nil {

t.Fatalf("restart console.carloku.com: %v", err)

@@ -637,7 +649,7 @@ func TestMakeRestartRouteUnitNameBeatsBacking(t *testing.T) {

var units, restarted []string

restart := makeRestartRoute(reg,

func(h string) { restarted = append(restarted, h) },

- func(u string) { units = append(units, u) },

+ func(u string) error { units = append(units, u); return nil },

nil)

if err := restart("odd.test"); err != nil {

t.Fatalf("restart odd.test: %v", err)

@@ -720,7 +732,7 @@ func TestAdoptPassRestartsANamedUnitOnAdoption(t *testing.T) {

var units []string

restart := makeRestartRoute(reg,

func(h string) { t.Fatalf("carlos-app@ arm fired for %q — the console has no such unit", h) },

- func(u string) { units = append(units, u) },

+ func(u string) error { units = append(units, u); return nil },

func(h string) bool {

t.Fatalf("bounce arm fired for %q — the activator owns no console child", h)

return false

@@ -737,6 +749,77 @@ func TestAdoptPassRestartsANamedUnitOnAdoption(t *testing.T) {

}

+// TestAdoptPassRefusesToStampAVersionAWrongUnitDidNotStart is the honesty

+// half of issue #110, and the reason the named-unit arm returns its error

+// where the other two swallow theirs. A well-formed but WRONG unit name

+// (`systemctl try-restart` exits 5, "not loaded") restarted nothing — exactly

+// the state the column exists to end — and if that were swallowed adoption

+// would stamp Version, the edge would serve the new X-Carlos-Version, and

+// `carlos deploy` would report live over the old process.

+//

+// So: no Version stamp, and the failure is in the journal under the unit name

+// an operator would grep for. The next pass retries.

+func TestAdoptPassRefusesToStampAVersionAWrongUnitDidNotStart(t *testing.T) {

+ dir := t.TempDir()

+ optDir := t.TempDir()

+ s := release.NewDirStore(dir)

+ ctx := context.Background()

+ plat := runtime.GOOS + "-" + runtime.GOARCH

+ pub, priv, err := ed25519.GenerateKey(nil)

+ if err != nil {

+ t.Fatalf("generate key: %v", err)

+ }

+ reg := restartTestRegistry(t)

+ // The stamp is well-formed — ValidUnitName cannot tell a real unit from an

+ // absent one — but no such unit is loaded on the box.

+ if err := reg.Upsert(registry.Route{

+ Host: "console.test", Kind: "instance", App: "console", Channel: "canary/console",

+ Addr: "127.0.0.1:8090", Status: "ready", Unit: "consoel.service",

+ }); err != nil {

+ t.Fatalf("upsert console.test: %v", err)

+ }

+ m := release.Manifest{

+ App: "console", Version: "v1", Kind: "binary",

+ Artifacts: map[string]release.Artifact{plat: {}},

+ }

+ if err := release.Ship(ctx, s, "bab", m, map[string][]byte{plat: []byte("build one")}); err != nil {

+ t.Fatalf("Ship: %v", err)

+ }

+ if err := release.Promote(ctx, s, priv, "bab", "console", "canary/console", "v1"); err != nil {

+ t.Fatalf("Promote: %v", err)

+ }

+ // A fake systemctl that answers the way the real one does for a unit that

+ // is not loaded: exit 5, with systemd's own words on stdout.

+ bin := t.TempDir()

+ if err := os.WriteFile(filepath.Join(bin, "systemctl"),

+ []byte("#!/bin/sh\necho \"Failed to try-restart $2: Unit $2 not found.\"\nexit 5\n"), 0o755); err != nil {

+ t.Fatal(err)

+ }

+ t.Setenv("PATH", bin+string(os.PathListSeparator)+os.Getenv("PATH"))

+ var journal bytes.Buffer

+ log.SetOutput(&journal)

+ t.Cleanup(func() { log.SetOutput(os.Stderr) })

+ restart := rootRestartRoute(reg)

+ if err := release.Pass(ctx, reg, s, pub, optDir, restart); err != nil {

+ t.Fatalf("Pass: %v", err)

+ }

+ rt, err := reg.Get("console.test")

+ if err != nil || rt == nil {

+ t.Fatalf("get console.test: %+v, %v", rt, err)

+ }

+ if rt.Version != "" {

+ t.Fatalf("Version = %q after a restart that started nothing — the header would claim a build the console is not running", rt.Version)

+ }

+ if !strings.Contains(journal.String(), "consoel.service") {

+ t.Fatalf("journal = %q, want the failed try-restart logged under the unit name", journal.String())

+ }

// TestAdoptPassBouncesAWarmHibernatingInstance is the end-to-end this task

// exists for, through the real release.Pass with the real closure: an exec

// instance minted the way provision does it (Hibernate: true, Backing "") is

@@ -785,7 +868,10 @@ func TestAdoptPassBouncesAWarmHibernatingInstance(t *testing.T) {

// it never starts anything.

restart := makeRestartRoute(reg,

func(h string) { tried = append(tried, h) },

- func(u string) { t.Fatalf("named-unit arm fired for %q on a route that names no unit", u) },

+ func(u string) error {

+ t.Fatalf("named-unit arm fired for %q on a route that names no unit", u)

+ return nil

+ },

func(h string) bool {

bounced = append(bounced, h)

return h == "warm.test"

@@ -1091,7 +1177,10 @@ func TestFastSweepBouncesAWarmHibernatingCanary(t *testing.T) {

var bounced []string

restart := makeRestartRoute(env.reg,

func(string) {},

- func(u string) { t.Fatalf("named-unit arm fired for %q on a route that names no unit", u) },

+ func(u string) error {

+ t.Fatalf("named-unit arm fired for %q on a route that names no unit", u)

+ return nil

+ },

func(h string) bool { bounced = append(bounced, h); return true })

d := env.deps(nil)

d.restart = restart