carlos / platform Public

host: settle check fixes (review round 1)

Fix three issues from review:

1. NRestarts check on fresh invocation: before the switch in settleUnit, check if NRestarts > 0 on the first poll of a new invocation (when InvocationID changed from beforeInvocation). Add TestSettleCatchesRestartLoopOnFreshInvocation.

2. Exe error handling: distinguish permission errors from other errors. On permission error, log once and skip binary check for the rest of settle. On other errors, remember and skip Ready/Hold on that poll, including error in deadline reason. Add TestSettleExeErrorDeadline and TestSettleExePermissionErrorSkipped.

3. Add binCheckSkipped flag and lastExeErr variable to track state across polls. Extend fakeUnits with exeErr map for custom Exe errors in tests.

Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>

claude 2 weeks ago d2ff49402ca9923a806ae7ff854214b306a17d53 parent 85472d8

2 files changed, +94 −14

internal/host/unitsettle.go +41 −11
internal/host/unitsettle_test.go +53 −3

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

index 69bf568b8..f860258be 100644

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

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

@@ -11,7 +11,9 @@

package host

import (

+ "errors"

"fmt"

+ "log"

"os"

"os/exec"

"regexp"

@@ -50,8 +52,8 @@ func (systemdUnits) Show(unit string) (unitProps, error) {

return parseShow(string(out)), nil

}

-func (systemdUnits) TryRestart(unit string) error { return systemctlTryRestartUnit(unit) }

-func (systemdUnits) Restart(unit string) error { return systemctlVerb("restart", unit) }

+func (systemdUnits) TryRestart(unit string) error { return systemctlTryRestartUnit(unit) }

+func (systemdUnits) Restart(unit string) error { return systemctlVerb("restart", unit) }

func (systemdUnits) ResetFailed(unit string) error { return systemctlVerb("reset-failed", unit) }

func (systemdUnits) JournalTail(unit string, n int) string {

@@ -59,7 +61,9 @@ func (systemdUnits) JournalTail(unit string, n int) string {

return strings.TrimSpace(string(out))

}

-func (systemdUnits) Exe(pid int) (string, error) { return os.Readlink(fmt.Sprintf("/proc/%d/exe", pid)) }

+func (systemdUnits) Exe(pid int) (string, error) {

+ return os.Readlink(fmt.Sprintf("/proc/%d/exe", pid))

func systemctlVerb(verb, unit string) error {

out, err := exec.Command("systemctl", verb, unit).CombinedOutput()

@@ -111,9 +115,11 @@ type settleOpts struct {

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

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 inv string // the new invocation, once seen active

var activeSince time.Time

var lastReady error

+ var lastExeErr error

+ 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)}

}

@@ -125,6 +131,10 @@ func settleUnit(sys unitSystem, unit, beforeInvocation string, o settleOpts) err

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

return nil

}

+ // NRestarts check on fresh invocation, before the switch

+ if p.InvocationID != beforeInvocation && p.InvocationID != "" && p.NRestarts > 0 {

+ return notUp(fmt.Sprintf("systemd restarted it %d time(s)", p.NRestarts))

+ }

switch {

case p.ActiveState == "failed":

return notUp("unit entered failed state")

@@ -136,24 +146,44 @@ func settleUnit(sys unitSystem, unit, beforeInvocation string, o settleOpts) err

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

}

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

- exe, err := sys.Exe(p.MainPID)

- if err == nil && o.WantDir != "" && !strings.HasPrefix(exe, strings.TrimSuffix(o.WantDir, "/")+"/") {

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

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

+ if !binCheckSkipped {

+ exe, err := sys.Exe(p.MainPID)

+ if err != nil {

+ if errors.Is(err, os.ErrPermission) {

+ // Permission error; log once and skip binary check for rest

+ log.Printf("adopt: %s: cannot read /proc/%d/exe (%v); binary check skipped", unit, p.MainPID, err)

+ binCheckSkipped = true

+ } else {

+ // Other error; remember it and skip Ready/Hold on this poll

+ lastExeErr = 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))

+ }

}

- if o.Ready != nil {

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

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

+ if lastExeErr == nil {

+ if o.Ready != nil {

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

+ return nil

+ }

+ } else if o.Now().Sub(activeSince) >= o.Hold {

return nil

}

- } else if o.Now().Sub(activeSince) >= o.Hold {

- return nil

}

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

+ if lastExeErr != nil {

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

+ }

if lastReady != nil {

return notUp(fmt.Sprintf("not ready after %s: %v", o.Timeout, lastReady))

}

return notUp(fmt.Sprintf("not ready after %s", o.Timeout))

}

+ lastExeErr = nil // reset exe error for next poll

o.Sleep(o.Poll)

}

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

index fb92cb4ae..36f62d42b 100644

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

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

@@ -2,7 +2,9 @@ package host

import (

"errors"

+ "io/fs"

"strings"

+ "syscall"

"testing"

"time"

@@ -13,6 +15,7 @@ type fakeUnits struct {

shows []unitProps // returned in order; the last repeats

n int

exe map[int]string

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

journal string

restarts []string

}

@@ -22,11 +25,23 @@ func (f *fakeUnits) Show(string) (unitProps, error) {

f.n++

return p, nil

}

-func (f *fakeUnits) TryRestart(u string) error { f.restarts = append(f.restarts, "try-restart "+u); return nil }

-func (f *fakeUnits) Restart(u string) error { f.restarts = append(f.restarts, "restart "+u); return nil }

-func (f *fakeUnits) ResetFailed(u string) error { f.restarts = append(f.restarts, "reset-failed "+u); return nil }

+func (f *fakeUnits) TryRestart(u string) error {

+ f.restarts = append(f.restarts, "try-restart "+u)

+ return nil

+func (f *fakeUnits) Restart(u string) error {

+ f.restarts = append(f.restarts, "restart "+u)

+ return nil

+func (f *fakeUnits) ResetFailed(u string) error {

+ f.restarts = append(f.restarts, "reset-failed "+u)

+ return nil

func (f *fakeUnits) JournalTail(string, int) string { return f.journal }

func (f *fakeUnits) Exe(pid int) (string, error) {

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

+ return "", err

+ }

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

return e, nil

}

@@ -139,6 +154,41 @@ func TestSettleSkipsAUnitThatWasNotRunning(t *testing.T) {

}

+// Fix round 1, issue 1: NRestarts unchecked on the poll that fixes the new invocation.

+func TestSettleCatchesRestartLoopOnFreshInvocation(t *testing.T) {

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

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

+ }, exe: map[int]string{7: "/opt/carlos/versions/new/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 UnitNotUpError for restart loop on fresh invocation", err)

+ }

+// Fix round 1, issue 2a: Exe error (not permission) should be remembered and deadline should mention it.

+func TestSettleExeErrorDeadline(t *testing.T) {

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

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

+ }, exe: map[int]string{}}

+ var nu *release.UnitNotUpError

+ if err := settleUnit(f, "console.service", "old", opts(nil)); !errors.As(err, &nu) || !strings.Contains(nu.Reason, "exe unreadable") {

+ t.Fatalf("settle = %v, want UnitNotUpError with 'exe unreadable' in reason", err)

+ }

+// Fix round 1, issue 2b: Permission error on Exe should be logged once and skipped, allowing settle to succeed.

+func TestSettleExePermissionErrorSkipped(t *testing.T) {

+ f := &fakeUnits{

+ shows: []unitProps{

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

+ },

+ exeErr: map[int]error{7: &fs.PathError{Op: "readlink", Path: "/proc/7/exe", Err: syscall.EACCES}},

+ }

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

+ t.Fatalf("settle = %v, want nil (permission error on Exe should be skipped)", err)

+ }

func TestUnitSettleTimeoutFloor(t *testing.T) {

env := func(v string) func(string) string { return func(string) string { return v } }

for in, want := range map[string]time.Duration{"": 30 * time.Second, "1s": 5 * time.Second,