carlos / platform Public

host: settle waits out the exec window, retries a failed show, leaves a stopped unit alone

Final review F1, F2, F5 (Plan 1 D1).

F1: an exe outside the new version's directory is not-ready, not a failure. For Type=simple systemd reports the invocation active before it execs ExecStart, so /proc/<pid>/exe can name systemd-executor on the first poll. Only a deadline that still sees the wrong exe fails, naming it. Applies to the rollback settle too.

F2: settleUnit reads SubState and returns errUnitStopped for inactive/dead (an operator's stop, before or during the settle). checkedUnitRestart and rollbackUnitRestart log it and return nil: no rollback, no refusal, no restart.

F5: a systemctl show error inside the loop is retried until the deadline, which fails with 'cannot observe <unit>: <err>'.

Tests: the exec window, the executor at the deadline, a transient show error, show failing to the deadline, stop during settle (settleUnit, checkedUnitRestart, rollbackUnitRestart), failed still fails, and the production wiring settling a healthy forward restart.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

claude 2 weeks ago 44e9fbbeba37631f531309bdd76389d56ae94e63 parent ffe4ec7

3 files changed, +230 −12

internal/host/agent.go +16 −2
internal/host/unitsettle.go +43 −6
internal/host/unitsettle_test.go +171 −4

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

index 131cb5d0c..db6bc1eff 100644

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

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

@@ -3245,6 +3245,13 @@ func checkedUnitRestart(sys unitSystem, optDir string, getenv func(string) strin

Timeout: timeout, Poll: 500 * time.Millisecond, Hold: 5 * time.Second,

WantDir: filepath.Dir(want), Ready: routeReady(rt, 2*time.Second),

Now: time.Now, Sleep: time.Sleep})

+ if errors.Is(err, errUnitStopped) {

+ // An operator stopped it between the read above and now. Not a

+ // verdict on the build, and a rollback restart would undo their

+ // stop: same as a unit that was stopped before the restart.

+ log.Printf("adopt: %s: %s was stopped during settle; not judged, not restarted", rt.Host, rt.Unit)

+ return nil

+ }

if err == nil {

log.Printf("adopt: %s: %s settled on %s", rt.Host, rt.Unit, filepath.Dir(want))

}

@@ -3273,10 +3280,17 @@ func rollbackUnitRestart(reg *registry.Registry, sys unitSystem, optDir string,

if err != nil {

return err

}

- if err := settleUnit(sys, rt.Unit, before.InvocationID, settleOpts{

+ err = settleUnit(sys, rt.Unit, before.InvocationID, settleOpts{

Timeout: unitSettleTimeout(getenv), Poll: 500 * time.Millisecond, Hold: 5 * time.Second,

WantDir: filepath.Dir(want), Ready: routeReady(*rt, 2*time.Second),

- Now: time.Now, Sleep: time.Sleep}); err != nil {

+ Now: time.Now, Sleep: time.Sleep})

+ if errors.Is(err, errUnitStopped) {

+ // The restored build is what the link names; a stop an operator

+ // made while it settled is theirs to keep, not a failed recovery.

+ log.Printf("adopt: %s: %s was stopped during the rollback's settle; not judged, not restarted", host, rt.Unit)

+ return nil

+ }

+ if err != nil {

return fmt.Errorf("rollback did not come up either: %w", err)

}

return nil

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

index adb3cbe5b..15e15d842 100644

--- a/internal/host/unitsettle.go

+++ b/internal/host/unitsettle.go

@@ -33,6 +33,7 @@ import (

type unitProps struct {

ActiveState string

+ SubState string // "dead" with ActiveState inactive is a stop, not a crash

InvocationID string

MainPID int

NRestarts int

@@ -52,7 +53,7 @@ type systemdUnits struct{}

func (systemdUnits) Show(unit string) (unitProps, error) {

out, err := exec.Command("systemctl", "show", unit,

- "-p", "ActiveState", "-p", "InvocationID", "-p", "MainPID", "-p", "NRestarts", "-p", "ExecStart").Output()

+ "-p", "ActiveState", "-p", "SubState", "-p", "InvocationID", "-p", "MainPID", "-p", "NRestarts", "-p", "ExecStart").Output()

if err != nil {

return unitProps{}, fmt.Errorf("systemctl show %s: %w", unit, err)

}

@@ -92,6 +93,8 @@ func parseShow(out string) unitProps {

switch k {

case "ActiveState":

p.ActiveState = v

+ case "SubState":

+ p.SubState = v

case "InvocationID":

p.InvocationID = v

case "MainPID":

@@ -117,15 +120,29 @@ type settleOpts struct {

Sleep func(time.Duration)

}

+// errUnitStopped is settleUnit's answer for a unit that was stopped while it

+// was being watched (inactive, SubState dead): an operator's stop, not a

+// crash. It is neither a healthy build nor a failed one, so the caller judges

+// nothing and restarts nothing.

+var errUnitStopped = errors.New("unit was stopped during settle")

// settleUnit watches unit after a try-restart. beforeInvocation is the

// InvocationID before the restart; "" means the unit was not running, and an

-// inactive unit is then "nothing started", not a failure.

+// inactive unit is then "nothing started", not a failure. Otherwise a unit

+// that goes inactive/dead returns errUnitStopped.

func settleUnit(sys unitSystem, unit, beforeInvocation string, o settleOpts) error {

deadline := o.Now().Add(o.Timeout)

var inv string // the new invocation, once seen active

var activeSince time.Time

var lastReady error

var lastExeErr error

+ // wrongExe is the executable MainPID ran on this poll when it was not

+ // under WantDir. Not a failure on its own: for Type=simple systemd reports

+ // the new invocation active as soon as it has forked, BEFORE it execs the

+ // ExecStart binary, so /proc/<pid>/exe can briefly name systemd's own

+ // executor (systemd-executor, or systemd itself on older releases). It is

+ // treated like not-ready, and only a deadline that still sees it fails.

+ var wrongExe string

var binCheckSkipped bool // permission error on Exe; skip binary check for rest of settle

notUp := func(reason string) error {

return &release.UnitNotUpError{Unit: unit, Reason: reason, Journal: sys.JournalTail(unit, 20)}

@@ -133,7 +150,23 @@ func settleUnit(sys unitSystem, unit, beforeInvocation string, o settleOpts) err

for {

p, err := sys.Show(unit)

if err != nil {

- return notUp(err.Error())

+ // One failed `systemctl show` says nothing about the build (a

+ // busy bus, a timed-out D-Bus call). Retried until the deadline,

+ // and only a deadline that still cannot observe the unit fails.

+ if !o.Now().Before(deadline) {

+ return notUp(fmt.Sprintf("cannot observe %s: %v", unit, err))

+ }

+ o.Sleep(o.Poll)

+ continue

+ }

+ // Inactive with SubState dead and nothing pending is a STOP: a crash

+ // reads failed, and a crash systemd will restart reads activating

+ // (auto-restart). try-restart blocks until its start job is done, so

+ // this is not the gap between the stop and start halves of the

+ // restart either. Somebody stopped the unit on purpose, before or

+ // during this check; the caller must neither judge nor restart it.

+ if beforeInvocation != "" && p.ActiveState == "inactive" && p.SubState == "dead" {

+ return errUnitStopped

}

if beforeInvocation == "" && p.ActiveState == "inactive" {

return nil

@@ -152,6 +185,7 @@ func settleUnit(sys unitSystem, unit, beforeInvocation string, o settleOpts) err

case inv == "" && p.ActiveState == "active" && p.InvocationID != beforeInvocation:

inv, activeSince = p.InvocationID, o.Now()

}

+ wrongExe = ""

if inv != "" && p.ActiveState == "active" {

// Check the binary only if we haven't hit a permission error

if !binCheckSkipped {

@@ -167,11 +201,11 @@ func settleUnit(sys unitSystem, unit, beforeInvocation string, o settleOpts) err

// Continue to next poll without calling Ready/Hold

}

} else if o.WantDir != "" && !strings.HasPrefix(exe, strings.TrimSuffix(o.WantDir, "/")+"/") {

- return notUp(fmt.Sprintf("running %s, not the new build in %s", exe, o.WantDir))

+ wrongExe = exe // not yet the new build: not ready, poll again

}

- // Only call Ready/Hold if there was no exe error on this poll

- if lastExeErr == nil {

+ // Only call Ready/Hold once this poll saw the new build running

+ if lastExeErr == nil && wrongExe == "" {

if o.Ready != nil {

if lastReady = o.Ready(); lastReady == nil {

return nil

@@ -182,6 +216,9 @@ func settleUnit(sys unitSystem, unit, beforeInvocation string, o settleOpts) err

}

if !o.Now().Before(deadline) {

+ if wrongExe != "" {

+ return notUp(fmt.Sprintf("running %s, not the new build in %s", wrongExe, o.WantDir))

+ }

if lastExeErr != nil {

return notUp(fmt.Sprintf("exe unreadable: %v", lastExeErr))

}

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

index e2ff0a97d..c249b9060 100644

--- a/internal/host/unitsettle_test.go

+++ b/internal/host/unitsettle_test.go

@@ -22,11 +22,18 @@ type fakeUnits struct {

n int

exe map[int]string

exeErr map[int]error // optional: errors to return from Exe

+ exeSeq []string // optional: Exe's answers in order, whatever the pid; the last repeats

+ exeN int

+ showErr map[int]error // optional: the Nth Show call (0-based) fails with this

journal string

restarts []string

}

func (f *fakeUnits) Show(string) (unitProps, error) {

+ if err, ok := f.showErr[f.n]; ok {

+ f.n++

+ return unitProps{}, err

+ }

p := f.shows[min(f.n, len(f.shows)-1)]

f.n++

return p, nil

@@ -48,6 +55,11 @@ func (f *fakeUnits) Exe(pid int) (string, error) {

if err, ok := f.exeErr[pid]; ok {

return "", err

}

+ if len(f.exeSeq) > 0 {

+ e := f.exeSeq[min(f.exeN, len(f.exeSeq)-1)]

+ f.exeN++

+ return e, nil

+ }

if e, ok := f.exe[pid]; ok {

return e, nil

}

@@ -68,9 +80,9 @@ func opts(ready func() error) settleOpts {

}

func TestParseShow(t *testing.T) {

- p := parseShow("ActiveState=active\nInvocationID=abc\nMainPID=42\nNRestarts=0\n" +

+ p := parseShow("ActiveState=active\nSubState=running\nInvocationID=abc\nMainPID=42\nNRestarts=0\n" +

"ExecStart={ path=/opt/carlos/live/platform.example.com ; argv[]=/opt/carlos/live/platform.example.com serve ; ignore_errors=no }\n")

- if p.ActiveState != "active" || p.InvocationID != "abc" || p.MainPID != 42 ||

+ if p.ActiveState != "active" || p.SubState != "running" || p.InvocationID != "abc" || p.MainPID != 42 ||

p.ExecPath != "/opt/carlos/live/platform.example.com" {

t.Fatalf("parseShow = %+v", p)

}

@@ -131,8 +143,67 @@ func TestSettleRejectsTheOldBinary(t *testing.T) {

f := &fakeUnits{shows: []unitProps{{ActiveState: "active", InvocationID: "new", MainPID: 7}},

exe: map[int]string{7: "/opt/carlos/versions/old/console"}}

var nu *release.UnitNotUpError

- if err := settleUnit(f, "console.service", "old", opts(func() error { return nil })); !errors.As(err, &nu) {

- t.Fatalf("settle = %v, want not-up: the process runs the old build", err)

+ if err := settleUnit(f, "console.service", "old", opts(func() error { return nil })); !errors.As(err, &nu) ||

+ !strings.Contains(nu.Reason, "running /opt/carlos/versions/old/console, not the new build in /opt/carlos/versions/new") {

+ t.Fatalf("settle = %v, want not-up at the deadline naming the old build's exe", err)

+ }

+// Final review F1: for Type=simple, systemd reports the new invocation active

+// once it has forked and before it execs ExecStart, so the first poll can see

+// systemd's executor as MainPID's exe. That is not-ready, not a failure.

+func TestSettleWaitsOutTheExecWindow(t *testing.T) {

+ f := &fakeUnits{shows: []unitProps{{ActiveState: "active", InvocationID: "new", MainPID: 7}},

+ exeSeq: []string{"/usr/lib/systemd/systemd-executor", "/opt/carlos/versions/new/console"}}

+ readies := 0

+ err := settleUnit(f, "console.service", "old", opts(func() error { readies++; return nil }))

+ if err != nil {

+ t.Fatalf("settle = %v, want healthy once the new binary is exec'd", err)

+ }

+ if f.exeN != 2 || readies != 1 {

+ t.Fatalf("exe reads %d, ready calls %d; want 2 and 1 (no readiness judged on the executor)", f.exeN, readies)

+ }

+func TestSettleExecutorAtTheDeadlineNamesTheExe(t *testing.T) {

+ f := &fakeUnits{shows: []unitProps{{ActiveState: "active", InvocationID: "new", MainPID: 7}},

+ exeSeq: []string{"/usr/lib/systemd/systemd-executor"}}

+ var nu *release.UnitNotUpError

+ err := settleUnit(f, "console.service", "old", opts(func() error { return nil }))

+ if !errors.As(err, &nu) ||

+ !strings.Contains(nu.Reason, "running /usr/lib/systemd/systemd-executor, not the new build in /opt/carlos/versions/new") {

+ t.Fatalf("settle = %v, want a deadline UnitNotUpError naming the exe seen", err)

+ }

+// Final review F5: one failed `systemctl show` is retried, not a verdict.

+func TestSettleRetriesATransientShowError(t *testing.T) {

+ f := &fakeUnits{shows: []unitProps{{ActiveState: "active", InvocationID: "new", MainPID: 7}},

+ exe: map[int]string{7: "/opt/carlos/versions/new/console"},

+ showErr: map[int]error{0: errors.New("Failed to get properties: Connection timed out")}}

+ if err := settleUnit(f, "console.service", "old", opts(func() error { return nil })); err != nil {

+ t.Fatalf("settle = %v, want healthy after one show error", err)

+ }

+func TestSettleShowErrorAtTheDeadline(t *testing.T) {

+ sys := &showFails{fakeUnits: &fakeUnits{}}

+ var nu *release.UnitNotUpError

+ err := settleUnit(sys, "console.service", "old", opts(func() error { return nil }))

+ if !errors.As(err, &nu) || !strings.Contains(nu.Reason, "cannot observe console.service: Access denied") {

+ t.Fatalf("settle = %v, want a deadline UnitNotUpError saying the unit could not be observed", err)

+ }

+// Final review F2: inactive/dead after the restart is an operator's stop.

+func TestSettleStoppedUnitIsErrUnitStopped(t *testing.T) {

+ f := &fakeUnits{shows: []unitProps{

+ {ActiveState: "active", InvocationID: "new", MainPID: 7},

+ {ActiveState: "inactive", SubState: "dead", InvocationID: "new"},

+ }, exe: map[int]string{7: "/opt/carlos/versions/new/console"}}

+ err := settleUnit(f, "console.service", "old", opts(func() error { return errors.New("refused") }))

+ if !errors.Is(err, errUnitStopped) {

+ t.Fatalf("settle = %v, want errUnitStopped", err)

}

@@ -370,3 +441,99 @@ func TestRollbackRestartResetsThenRestarts(t *testing.T) {

t.Fatalf("restarts = %v, want reset-failed then restart (never start)", f.restarts)

}

+// newBuildDir is where checkedUnitRestart expects the new process to run

+// from: the directory of the live link's resolved target, the same

+// EvalSymlinks the production code does (t.TempDir can sit under a symlink).

+func newBuildDir(t *testing.T, opt, host string) string {

+ t.Helper()

+ want, err := filepath.EvalSymlinks(filepath.Join(opt, "live", host))

+ if err != nil {

+ t.Fatal(err)

+ }

+ return filepath.Dir(want)

+// The production wiring, end to end: checkedUnitRestart reads the unit,

+// try-restarts it, and settles a healthy forward restart on the new build

+// (ledger Task 6 minor). Real clock; the first poll is healthy.

+func TestNamedRestartSettlesAHealthyForwardRestart(t *testing.T) {

+ opt := t.TempDir()

+ liveTo(t, opt, "platform.example.com", "new")

+ ln, err := net.Listen("tcp", "127.0.0.1:0")

+ if err != nil {

+ t.Fatal(err)

+ }

+ t.Cleanup(func() { ln.Close() })

+ rt := boundRoute()

+ rt.Addr = ln.Addr().String()

+ f := &fakeUnits{shows: []unitProps{

+ {ActiveState: "active", SubState: "running", InvocationID: "old", MainPID: 6,

+ ExecPath: filepath.Join(opt, "live", "platform.example.com")},

+ {ActiveState: "active", SubState: "running", InvocationID: "new", MainPID: 7},

+ }, exe: map[int]string{7: filepath.Join(newBuildDir(t, opt, "platform.example.com"), "console")}}

+ if err := checkedUnitRestart(f, opt, func(string) string { return "" })(rt); err != nil {

+ t.Fatalf("forward restart = %v, want settled", err)

+ }

+ if strings.Join(f.restarts, ",") != "try-restart console.service" {

+ t.Fatalf("restarts = %v, want exactly the try-restart", f.restarts)

+ }

+// Final review F2: the unit read active, then an operator stopped it (before

+// or during the settle). checkedUnitRestart reports nil, so adoptHost does

+// not roll back, refuse or restart it, and the only systemctl verb issued is

+// the try-restart itself.

+func TestNamedRestartStopDuringSettleIsNotJudged(t *testing.T) {

+ opt := t.TempDir()

+ liveTo(t, opt, "platform.example.com", "new")

+ f := &fakeUnits{shows: []unitProps{

+ {ActiveState: "active", SubState: "running", InvocationID: "old", MainPID: 6,

+ ExecPath: filepath.Join(opt, "live", "platform.example.com")},

+ {ActiveState: "inactive", SubState: "dead", InvocationID: "old"},

+ }}

+ if err := checkedUnitRestart(f, opt, func(string) string { return "" })(boundRoute()); err != nil {

+ t.Fatalf("err = %v, want nil: a stop is not a verdict on the build", err)

+ }

+ if strings.Join(f.restarts, ",") != "try-restart console.service" {

+ t.Fatalf("restarts = %v, want only the try-restart (no restart, no reset-failed)", f.restarts)

+ }

+// The other side of F2: failed is a crash, never a stop.

+func TestNamedRestartFailedStillFails(t *testing.T) {

+ opt := t.TempDir()

+ liveTo(t, opt, "platform.example.com", "new")

+ f := &fakeUnits{shows: []unitProps{

+ {ActiveState: "active", SubState: "running", InvocationID: "old", MainPID: 6,

+ ExecPath: filepath.Join(opt, "live", "platform.example.com")},

+ {ActiveState: "failed", SubState: "failed", InvocationID: "new"},

+ }}

+ var nu *release.UnitNotUpError

+ if err := checkedUnitRestart(f, opt, func(string) string { return "" })(boundRoute()); !errors.As(err, &nu) {

+ t.Fatalf("err = %v, want UnitNotUpError for a failed unit", err)

+ }

+// A stop during the rollback's own settle is the operator's too: the link

+// names the restored build, and nothing restarts it a second time.

+func TestRollbackRestartStopDuringSettleIsNotJudged(t *testing.T) {

+ opt := t.TempDir()

+ liveTo(t, opt, "platform.example.com", "old")

+ reg := restartTestRegistry(t)

+ r := boundRoute()

+ r.Addr = ""

+ if err := reg.Upsert(r); err != nil {

+ t.Fatal(err)

+ }

+ f := &fakeUnits{shows: []unitProps{

+ {ActiveState: "active", InvocationID: "bad"},

+ {ActiveState: "inactive", SubState: "dead", InvocationID: "bad"},

+ }}

+ if err := rollbackUnitRestart(reg, f, opt, func(string) string { return "" })("platform.example.com"); err != nil {

+ t.Fatalf("rollback = %v, want nil for a stop during its settle", err)

+ }

+ if strings.Join(f.restarts, ",") != "reset-failed console.service,restart console.service" {

+ t.Fatalf("restarts = %v, want the one reset-failed and restart only", f.restarts)

+ }