carlos / platform Public

zdd plan 2 final fix: an uncertain generation start is killed before release

A generation start can fail after systemctl start succeeded: the
invocation lookup fails, or the answer is lost. The process then ran on
while the server dropped it and the activator closed its listeners.

Server: once start has been issued, any failure kills the unit, stops
it and waits for its cgroup to empty (killGen, shared with /stop kill=1)
on a context the client's hang-up cannot cancel. Only that proof removes
the drop-in and exe layout. Otherwise the files stay and the answer is
statusGenUnproven (500), which Client.StartGen maps to ErrGenUnproven.
The start-error branch no longer removes files after a stop that failed.

Activator: a failed StartGen, or one with no invocation, sends StopGen
kill=1 on its own budget. Only a nil answer closes the listeners and
removes launch.gen.<n>.json; otherwise the error wraps errGenUnproven
and nothing is released. Flag-off starts are unchanged.

Tests: failed show then proven stop, unproven stop keeps the files,
failed start whose stop fails (server); proven, unproven and empty
invocation (activator); drop-in write and daemon-reload failures.

Final review finding L1 (triage T9 items).

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

claude 2 weeks ago ba6e6d6a9416546877b705b8938395c453f610b4 parent 7f889d9

8 files changed, +285 −26

internal/activator/backend_exec.go +30 −0
internal/activator/containment.go +5 −0
internal/activator/containment_test.go +1 −1
internal/activator/unitproc_gen_test.go +72 −0
internal/activator/unitproc_test.go +4 −2
internal/instancectl/client.go +6 −2
internal/instancectl/gen.go +56 −21
internal/instancectl/server_gen_test.go +111 −0

diff --git a/internal/activator/backend_exec.go b/internal/activator/backend_exec.go

index d0292183f..fa22b4cf1 100644

--- a/internal/activator/backend_exec.go

+++ b/internal/activator/backend_exec.go

@@ -1540,6 +1540,17 @@ func (b *ExecBackend) startUnitAt(ctx context.Context, rt registry.Route, sock s

// older version (one extra convergence cycle), never the newer.

spawnVer, _ := b.LiveVersion(rt.Host)

inv, err := b.startRole(ctx, rt, instancectl.RoleRun, gen)

+ if gen != 0 && (err != nil || inv == "") {

+ if err == nil {

+ err = fmt.Errorf("%s exited before it was observed", unitName(instancectl.RoleRun, rt.Host, gen))

+ }

+ if kerr := b.killFailedGen(ctx, rt, gen); kerr != nil {

+ // Nothing is released: the generation may still be running.

+ return nil, "", fmt.Errorf("%s: start unit: %w; %w: %v", rt.Host, err, errGenUnproven, kerr)

+ }

+ _ = os.Remove(filepath.Join(p.dataDir, instancectl.GenLaunchFileName(gen)))

+ return fail(fmt.Errorf("%s: start unit: %w", rt.Host, err))

+ }

if err != nil {

return fail(fmt.Errorf("%s: start unit: %w", rt.Host, err))

}

@@ -1583,6 +1594,25 @@ func (b *ExecBackend) startUnitAt(ctx context.Context, rt registry.Route, sock s

return up, spawnVer, nil

}

+// killFailedGen ends a generation whose start failed or answered no

+// invocation. The start may have succeeded with its answer lost, so the

+// generation is killed and proven empty (StopGen with kill) on a budget the

+// caller's context cannot cancel. nil is that proof. With no generation

+// channel StartGen was never sent, so nothing was started.

+func (b *ExecBackend) killFailedGen(ctx context.Context, rt registry.Route, gen uint64) error {

+ gc, err := b.genControl()

+ if err != nil {

+ return nil

+ }

+ kctx, cancel := context.WithTimeout(context.WithoutCancel(ctx), instancectl.DeadlineControl)

+ defer cancel()

+ if err := gc.StopGen(kctx, rt.Host, gen, true); err != nil {

+ log.Printf("activator: %s: kill %s after a failed start: %v", rt.Host, unitName(instancectl.RoleRun, rt.Host, gen), err)

+ return err

+ }

+ return nil

// nestedGate is the pre-flight check every nested spawn path runs first: no

// root instance-control channel configured, or the box itself is not fit

// to run an isolated instance (NestedOK's own mounts and /ready check),

diff --git a/internal/activator/containment.go b/internal/activator/containment.go

index 621d2f3a6..42e4e3809 100644

--- a/internal/activator/containment.go

+++ b/internal/activator/containment.go

@@ -14,6 +14,11 @@ import (

var (

errNoGenContainment = errors.New("activator: generation containment is off")

errNoGenControl = errors.New("activator: the instance control channel cannot run generation units")

+ // errGenUnproven is a generation start that failed and whose kill did

+ // not prove the generation gone. It may still be running: its listeners

+ // and launch file are kept, and a caller must keep whatever it owns for

+ // the host (a later plan's swap keeps the instance's ownership).

+ errGenUnproven = errors.New("activator: generation start failed and its termination is unproven")

)

// spawnKind is which child shape a spawn is, for placeFor.

diff --git a/internal/activator/containment_test.go b/internal/activator/containment_test.go

index 37c15e8a4..8885b523a 100644

--- a/internal/activator/containment_test.go

+++ b/internal/activator/containment_test.go

@@ -29,7 +29,7 @@ func (f fakeGenControl) StopGen(_ context.Context, host string, n uint64, kill b

if f.stopHook != nil {

f.stopHook(host, instancectl.RoleRun)

}

- return nil

+ return f.stopGenErr

}

func (f fakeGenControl) ResetGen(_ context.Context, host string, n uint64) error {

f.rec(fmt.Sprintf("resetgen %s %d", host, n))

diff --git a/internal/activator/unitproc_gen_test.go b/internal/activator/unitproc_gen_test.go

index 20e4f16c3..286ff5903 100644

--- a/internal/activator/unitproc_gen_test.go

+++ b/internal/activator/unitproc_gen_test.go

@@ -3,6 +3,7 @@ package activator

import (

"context"

"encoding/json"

+ "errors"

"os"

"path/filepath"

"strings"

@@ -12,6 +13,7 @@ import (

"github.com/carlosframework/platform/internal/instancectl"

"github.com/carlosframework/platform/internal/provision"

+ "github.com/carlosframework/platform/internal/registry"

)

func TestPrepareAtAGenerationKeysItsFilesByGeneration(t *testing.T) {

@@ -198,3 +200,73 @@ func TestStartUnitWithContainmentOffKeepsTheOriginalTemplate(t *testing.T) {

unit.set("inactive", "inv1", false, false)

waitFor(t, "the unit to exit", p.Exited)

}

+// genStartFixture is a nested backend with containment on, ready for a

+// generation-7 start whose answer the test chooses.

+func genStartFixture(t *testing.T, ctl *fakeControl) (*ExecBackend, registry.Route, string, string) {

+ t.Helper()

+ b, rt := nestedBackend(t, ctl, &fakeUnit{})

+ b.GenContainment = true

+ b.Control = fakeGenControl{ctl}

+ launch := filepath.Join(b.pathsFor(rt).dataDir, instancectl.GenLaunchFileName(7))

+ return b, rt, launch, b.logSocketFor(rt.Host, instancectl.RoleRun, 7)

+// A generation start that failed may have started the unit (its answer

+// was lost): the backend kills it through StopGen on its own budget, and

+// only then closes its listeners and removes its launch file.

+func TestStartGenFailureKillsTheGenerationBeforeReleasingIt(t *testing.T) {

+ ctl := &fakeControl{startErr: errors.New("instance control /start: 502: show: bus timeout")}

+ b, rt, launch, logSock := genStartFixture(t, ctl)

+ var launchAtStop bool

+ ctl.stopHook = func(string, instancectl.Role) { _, err := os.Stat(launch); launchAtStop = err == nil }

+ _, _, err := b.StartGen(context.Background(), rt, 7, false)

+ if err == nil || errors.Is(err, errGenUnproven) {

+ t.Fatalf("StartGen = %v, want an ordinary start failure", err)

+ }

+ if want := "startgen h.example.com 7 536870912\nstopgen h.example.com 7 kill=true"; ctl.joined() != want {

+ t.Errorf("calls =\n%s\nwant\n%s", ctl.joined(), want)

+ }

+ if !launchAtStop {

+ t.Error("the launch file was gone before the kill")

+ }

+ if _, err := os.Stat(launch); !os.IsNotExist(err) {

+ t.Errorf("launch file left after a proven stop: %v", err)

+ }

+ if _, err := os.Stat(logSock); !os.IsNotExist(err) {

+ t.Errorf("log listener still bound after a proven stop: %v", err)

+ }

+// The kill that follows a failed start fails too: termination is

+// unproven, so the generation's files stay and the error says so.

+func TestStartGenFailureWithAnUnprovenKillKeepsTheGeneration(t *testing.T) {

+ ctl := &fakeControl{startErr: errors.New("instance control socket: context deadline exceeded"), stopGenErr: errors.New("instance control /stop: 502: stop: processes remain")}

+ b, rt, launch, logSock := genStartFixture(t, ctl)

+ _, _, err := b.StartGen(context.Background(), rt, 7, false)

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

+ t.Fatalf("StartGen = %v, want errGenUnproven", err)

+ }

+ if _, err := os.Stat(launch); err != nil {

+ t.Errorf("launch file removed while termination is unproven: %v", err)

+ }

+ if _, err := os.Stat(logSock); err != nil {

+ t.Errorf("log listener released while termination is unproven: %v", err)

+ }

+// A start that answered no invocation is just as uncertain for a

+// generation: it is killed and proven gone before its files go.

+func TestStartGenWithNoInvocationKillsTheGeneration(t *testing.T) {

+ ctl := &fakeControl{}

+ b, rt, launch, _ := genStartFixture(t, ctl)

+ if _, _, err := b.StartGen(context.Background(), rt, 7, false); err == nil {

+ t.Fatal("a start with no invocation succeeded")

+ }

+ if !strings.Contains(ctl.joined(), "stopgen h.example.com 7 kill=true") {

+ t.Errorf("calls =\n%s", ctl.joined())

+ }

+ if _, err := os.Stat(launch); !os.IsNotExist(err) {

+ t.Errorf("launch file left after a proven stop: %v", err)

+ }

diff --git a/internal/activator/unitproc_test.go b/internal/activator/unitproc_test.go

index 7b867e7c5..11f55352b 100644

--- a/internal/activator/unitproc_test.go

+++ b/internal/activator/unitproc_test.go

@@ -27,8 +27,10 @@ type fakeControl struct {

inv string

seed instancectl.SeedResponse

startErr error

- stopHook func(host string, role instancectl.Role)

- readyErr error

+ // stopGenErr is what fakeGenControl.StopGen answers.

+ stopGenErr error

+ stopHook func(host string, role instancectl.Role)

+ readyErr error

// onStart runs inside Start, in the fake control's own goroutine, the

// same place the real control service's trampoline would exec the app

// and the app would bind its socket: prepare's stale-socket removal

diff --git a/internal/instancectl/client.go b/internal/instancectl/client.go

index 0b7029001..b954b7acd 100644

--- a/internal/instancectl/client.go

+++ b/internal/instancectl/client.go

@@ -178,12 +178,16 @@ func genValues(host string, n uint64) url.Values {

}

// StartGen starts generation n's unit (zero-downtime spec §7.6) with its

-// tier and returns the invocation id the server read after the start.

+// tier and returns the invocation id the server read after the start. A

+// failure whose cleanup could not prove the unit gone wraps ErrGenUnproven.

func (c *Client) StartGen(ctx context.Context, host string, n uint64, memoryMax int64) (string, error) {

q := genValues(host, n)

q.Set("memory_max", strconv.FormatInt(memoryMax, 10))

var out StartResponse

- if _, err := c.do(ctx, DeadlineControl, http.MethodPost, "/start", q, &out); err != nil {

+ if status, err := c.do(ctx, DeadlineControl, http.MethodPost, "/start", q, &out); err != nil {

+ if status == statusGenUnproven {

+ return "", fmt.Errorf("%w: %w", ErrGenUnproven, err)

+ }

return "", err

}

return out.InvocationID, nil

diff --git a/internal/instancectl/gen.go b/internal/instancectl/gen.go

index 277aab297..11c0b0e36 100644

--- a/internal/instancectl/gen.go

+++ b/internal/instancectl/gen.go

@@ -21,6 +21,16 @@ import (

// everything the template leaves out, reloads, checks that systemd took

// it, and only then starts the unit.

+// ErrGenUnproven is StartGen's answer when the start failed after systemd

+// may have started the unit and the stop that followed did not prove the

+// unit's cgroup empty. The generation may still be running: its files are

+// kept, and the caller must not release anything it owns for it.

+var ErrGenUnproven = errors.New("instance control: generation termination unproven")

+// statusGenUnproven is the status that carries ErrGenUnproven. No other

+// verb answers it, so the client can tell it from an ordinary 502.

+const statusGenUnproven = http.StatusInternalServerError

// DropInName is the drop-in's file name inside <unit>.d.

const DropInName = "50-carlos-generation.conf"

@@ -143,26 +153,59 @@ func (s *Server) startGen(w http.ResponseWriter, r *http.Request, req request) {

return

}

+ // From the start on, systemd may have started the unit, whatever the

+ // answer says. A failure past this point kills it and proves it empty

+ // before its files go; one that cannot prove that keeps the files and

+ // says so (statusGenUnproven).

if out, err := s.Systemctl(ctx, "start", unit); err != nil {

- // A start that failed leaves a failed or half-started unit and the

- // files it was given. Stop it (on a context the client's hang-up

- // cannot cancel) and take the files away, so the next attempt

- // begins from nothing.

- dctx, cancel := context.WithTimeout(context.WithoutCancel(ctx), 30*time.Second)

- defer cancel()

- s.stopUnit(dctx, unit)

- fail(http.StatusBadGateway, "start: "+strings.TrimSpace(out+" "+err.Error()))

+ s.abandonGen(ctx, w, req.host, unit, "start: "+strings.TrimSpace(out+" "+err.Error()))

return

}

inv, err := s.Systemctl(ctx, "show", "-p", "InvocationID", "--value", unit)

if err != nil {

- http.Error(w, "show: "+err.Error(), http.StatusBadGateway)

+ s.abandonGen(ctx, w, req.host, unit, "show: "+err.Error())

return

}

s.logf("instance-control: %s: started %s memory.max=%d", req.host, unit, req.tier.MemoryMax)

writeJSON(w, http.StatusOK, StartResponse{InvocationID: strings.TrimSpace(inv)})

}

+// abandonGen answers a generation start that failed after systemctl start

+// was issued. The unit may be running, so it is killed and waited on, on a

+// context the client's hang-up cannot cancel. Only a stop that proved the

+// unit's cgroup empty removes its files; otherwise they stay, and the

+// answer is statusGenUnproven so the client knows the generation may live.

+func (s *Server) abandonGen(ctx context.Context, w http.ResponseWriter, host, unit, msg string) {

+ dctx, cancel := context.WithTimeout(context.WithoutCancel(ctx), DeadlineControl+s.waitBudget())

+ defer cancel()

+ if err := s.killGen(dctx, host, unit); err != nil {

+ s.logf("instance-control: %s: abandon %s: termination unproven, files kept: %v", host, unit, err)

+ http.Error(w, msg+"; termination unproven: "+err.Error(), statusGenUnproven)

+ return

+ }

+ s.removeGenFiles(unit)

+ http.Error(w, msg, http.StatusBadGateway)

+// killGen kills a generation unit outright, stops it, and waits until its

+// cgroup is empty (spec §8.2 step 1). nil is the proof that nothing of it

+// runs. A kill that finds nothing to kill is not a failure: the stop and

+// the wait are the proof.

+func (s *Server) killGen(ctx context.Context, host, unit string) error {

+ if out, err := s.Systemctl(ctx, "kill", "--signal=SIGKILL", unit); err != nil && !strings.Contains(out+err.Error(), "not loaded") {

+ s.logf("instance-control: %s: kill %s: %v", host, unit, err)

+ }

+ if out, err := s.Systemctl(ctx, "stop", unit); err != nil {

+ return errors.New("stop: " + strings.TrimSpace(out+" "+err.Error()))

+ }

+ wctx, cancel := context.WithTimeout(ctx, s.waitBudget())

+ defer cancel()

+ if err := WaitCgroupEmpty(wctx, s.CgroupRoot, SliceName(host), unit, 50*time.Millisecond); err != nil {

+ return errors.New("stop: " + err.Error())

+ }

+ return nil

// stopGen is /stop with a generation. With kill=1 the generation is killed

// outright (spec §8.2 step 1) and waited on until its cgroup is empty.

// Either way the stop acts on that generation's unit only, and its files

@@ -171,22 +214,14 @@ func (s *Server) stopGen(w http.ResponseWriter, r *http.Request, req request) {

ctx := r.Context()

unit := GenUnitName(req.host, req.gen)

if req.kill {

- if out, err := s.Systemctl(ctx, "kill", "--signal=SIGKILL", unit); err != nil && !strings.Contains(out+err.Error(), "not loaded") {

- s.logf("instance-control: %s: kill %s: %v", req.host, unit, err)

+ if err := s.killGen(ctx, req.host, unit); err != nil {

+ http.Error(w, err.Error(), http.StatusBadGateway)

+ return

}

- }

- if out, err := s.Systemctl(ctx, "stop", unit); err != nil {

+ } else if out, err := s.Systemctl(ctx, "stop", unit); err != nil {

http.Error(w, "stop: "+strings.TrimSpace(out+" "+err.Error()), http.StatusBadGateway)

return

}

- if req.kill {

- wctx, cancel := context.WithTimeout(ctx, s.waitBudget())

- defer cancel()

- if err := WaitCgroupEmpty(wctx, s.CgroupRoot, SliceName(req.host), unit, 50*time.Millisecond); err != nil {

- http.Error(w, "stop: "+err.Error(), http.StatusBadGateway)

- return

- }

s.removeGenFiles(unit)

w.WriteHeader(http.StatusNoContent)

}

diff --git a/internal/instancectl/server_gen_test.go b/internal/instancectl/server_gen_test.go

index 0f93b13fb..7b5346649 100644

--- a/internal/instancectl/server_gen_test.go

+++ b/internal/instancectl/server_gen_test.go

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

"path/filepath"

"strings"

"testing"

+ "time"

"github.com/carlosframework/platform/internal/cgroup"

)

@@ -287,3 +288,113 @@ func TestGenAndParentInputsAreValidated(t *testing.T) {

t.Errorf("systemctl was called:\n%s", ctl.joined())

}

+// genFilesPresent reports whether the generation's drop-in directory and

+// its executable layout are on disk.

+func genFilesPresent(t *testing.T, s *Server, dropins string) (dropIn, exe bool) {

+ t.Helper()

+ _, err1 := os.Stat(filepath.Join(dropins, genUnit+".d"))

+ _, err2 := os.Stat(filepath.Join(s.ExeRoot, GenKey(genHost, 7)))

+ return err1 == nil, err2 == nil

+// populateGenCgroup makes the generation unit's cgroup report processes,

+// so no stop can prove it empty.

+func populateGenCgroup(t *testing.T, s *Server) {

+ t.Helper()

+ events := filepath.Join(s.CgroupRoot, parentSlices, SliceName(genHost), genUnit, "cgroup.events")

+ if err := os.MkdirAll(filepath.Dir(events), 0o755); err != nil {

+ t.Fatal(err)

+ }

+ if err := os.WriteFile(events, []byte("populated 1\nfrozen 0\n"), 0o644); err != nil {

+ t.Fatal(err)

+ }

+// systemctl start succeeded, then the invocation lookup failed: the unit

+// may be running. It is killed and proven empty before its files go, and

+// the answer is an ordinary start failure.

+func TestGenStartWhoseInvocationLookupFailsIsKilledBeforeItsFilesGo(t *testing.T) {

+ ctl := genCtl()

+ ctl.fail = map[string]error{"show -p InvocationID": errors.New("bus timeout")}

+ s, sock, dropins := genServer(t, ctl)

+ _, err := NewClient(sock).StartGen(context.Background(), genHost, 7, cgroup.TierDefault.MemoryMax)

+ if err == nil || errors.Is(err, ErrGenUnproven) {

+ t.Fatalf("StartGen = %v, want an ordinary start failure", err)

+ }

+ if want := "start " + genUnit + "\nshow -p InvocationID --value " + genUnit + "\nkill --signal=SIGKILL " + genUnit + "\nstop " + genUnit; !strings.HasSuffix(ctl.joined(), want) {

+ t.Errorf("calls =\n%s\nwant them to end\n%s", ctl.joined(), want)

+ }

+ if dropIn, exe := genFilesPresent(t, s, dropins); dropIn || exe {

+ t.Errorf("files left after a proven stop: drop-in %v exe %v", dropIn, exe)

+ }

+// The same, but the generation's cgroup never empties: termination is

+// unproven, so the files stay and the answer says so.

+func TestGenStartWhoseCleanupIsUnprovenKeepsTheFiles(t *testing.T) {

+ ctl := genCtl()

+ ctl.fail = map[string]error{"show -p InvocationID": errors.New("bus timeout")}

+ s, sock, dropins := genServer(t, ctl)

+ s.cgroupWaitBudget = 100 * time.Millisecond

+ populateGenCgroup(t, s)

+ _, err := NewClient(sock).StartGen(context.Background(), genHost, 7, cgroup.TierDefault.MemoryMax)

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

+ t.Fatalf("StartGen = %v, want ErrGenUnproven", err)

+ }

+ if dropIn, exe := genFilesPresent(t, s, dropins); !dropIn || !exe {

+ t.Errorf("files removed while termination is unproven: drop-in %v exe %v", dropIn, exe)

+ }

+// A failed systemctl start whose cleanup stop also fails: nothing proves

+// the unit gone, so the files stay.

+func TestGenStartFailureWhoseStopFailsKeepsTheFiles(t *testing.T) {

+ ctl := genCtl()

+ ctl.fail = map[string]error{"start ": errors.New("boom"), "stop ": errors.New("stop timed out")}

+ s, sock, dropins := genServer(t, ctl)

+ _, err := NewClient(sock).StartGen(context.Background(), genHost, 7, cgroup.TierDefault.MemoryMax)

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

+ t.Fatalf("StartGen = %v, want ErrGenUnproven", err)

+ }

+ if dropIn, exe := genFilesPresent(t, s, dropins); !dropIn || !exe {

+ t.Errorf("files removed after a failed stop: drop-in %v exe %v", dropIn, exe)

+ }

+// Nothing was started when the drop-in cannot be written: the exe layout

+// already made goes, and systemctl is never asked to start.

+func TestGenStartDropInWriteFailureRemovesTheFiles(t *testing.T) {

+ ctl := genCtl()

+ s, sock, dropins := genServer(t, ctl)

+ // A file where the unit's drop-in directory goes: MkdirAll fails.

+ if err := os.WriteFile(filepath.Join(dropins, genUnit+".d"), nil, 0o644); err != nil {

+ t.Fatal(err)

+ }

+ _, err := NewClient(sock).StartGen(context.Background(), genHost, 7, cgroup.TierDefault.MemoryMax)

+ if err == nil || errors.Is(err, ErrGenUnproven) {

+ t.Fatalf("StartGen = %v, want an ordinary failure", err)

+ }

+ if ctl.joined() != "" {

+ t.Errorf("systemctl was called:\n%s", ctl.joined())

+ }

+ if _, err := os.Stat(filepath.Join(s.ExeRoot, GenKey(genHost, 7))); !os.IsNotExist(err) {

+ t.Errorf("exe dir left behind: %v", err)

+ }

+func TestGenStartDaemonReloadFailureRemovesTheFiles(t *testing.T) {

+ ctl := genCtl()

+ ctl.fail = map[string]error{"daemon-reload": errors.New("bus gone")}

+ s, sock, dropins := genServer(t, ctl)

+ _, err := NewClient(sock).StartGen(context.Background(), genHost, 7, cgroup.TierDefault.MemoryMax)

+ if err == nil || errors.Is(err, ErrGenUnproven) {

+ t.Fatalf("StartGen = %v, want an ordinary failure", err)

+ }

+ if strings.Contains(ctl.joined(), "start "+genUnit) {

+ t.Errorf("the unit was started:\n%s", ctl.joined())

+ }

+ if dropIn, exe := genFilesPresent(t, s, dropins); dropIn || exe {

+ t.Errorf("files left behind: drop-in %v exe %v", dropIn, exe)

+ }