amadan
zdd plan 2 final fix: finalise generation files once termination is proven
Generation numbers are never reused, so leftovers accumulated: - A generation that exits on its own, or one restarted outside the activator and stopped, is finalised by observe once its cgroup is empty: launch.gen.<n>.json here, and the control service's drop-in and exe layout through StopGen on the inactive unit, unless a stop of ours already removed them (genFilesGone). The foreign-invocation branch now removes its launch file too. - /sweep recovers inactive leftovers: every carlos-instance-gen@*.d under DropInDir whose unit systemd reports inactive or failed and whose cgroup is empty loses its files. A unit the sweep could not stop, or one a start is preparing (genStarting, checked and removed under one lock), keeps them. No daemon-reload, as with stopGen. - A start refused before it was sent removes its launch file (the L1 change; covered here). Nothing is removed while termination is unproven. Final review finding L3 (triage T9 natural exit, T11 foreign launch).
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
5 files changed,
+241
−3
internal/activator/unitproc.go+37 −3internal/activator/unitproc_gen_test.go+74 −0internal/instancectl/gen.go+72 −0internal/instancectl/server.go+6 −0internal/instancectl/server_gen_test.go+52 −0
diff --git a/internal/activator/unitproc.go b/internal/activator/unitproc.go| index 4fa0fcf41..e8b6de72b 100644 |
| --- a/internal/activator/unitproc.go |
| +++ b/internal/activator/unitproc.go |
| @@ -57,6 +57,10 @@ type unitProc struct { |
| // stop on its own. It is instead the proof that our own invocation is |
| // already over (see observe). |
| stopRequested atomic.Bool |
| + // genFilesGone is set once a stop of this generation succeeded: the |
| + // control service removed the unit's drop-in and exe layout with it, |
| + // so finaliseGen need not ask again. |
| + genFilesGone atomic.Bool |
| } |
| func (b *ExecBackend) newUnitProc(rt registry.Route, role instancectl.Role, p instancePaths, inv string, ctl *edge.ControlListener, logs *logListener) *unitProc { |
| @@ -106,7 +110,7 @@ func (p *unitProc) Stop(ctx context.Context) error { |
| // still fire its own foreign-invocation stop for the same unit. |
| p.stopRequested.Store(true) |
| sctx, cancel := context.WithTimeout(ctx, instancectl.DeadlineControl) |
| - err := p.b.stopRole(sctx, p.rt.Host, p.role, p.gen) |
| + err := p.stopUnit(sctx) |
| cancel() |
| if err != nil { |
| log.Printf("activator: stop %s %s: %v", p.rt.Host, p.role, err) |
| @@ -134,6 +138,36 @@ func (p *unitProc) Stop(ctx context.Context) error { |
| } |
| } |
| +// stopUnit asks the control service to stop this proc's unit. For a |
| +// generation, a stop that succeeded also removed the unit's files there. |
| +func (p *unitProc) stopUnit(ctx context.Context) error { |
| + err := p.b.stopRole(ctx, p.rt.Host, p.role, p.gen) |
| + if err == nil && p.gen != 0 { |
| + p.genFilesGone.Store(true) |
| + } |
| + return err |
| +} |
| + |
| +// finaliseGen removes a generation's files once observe has proven it |
| +// gone (generation numbers are never reused, so nothing else would): its |
| +// launch file here, and the control service's drop-in and exe layout |
| +// through a stop of the inactive unit, unless a stop of ours already |
| +// removed them. Never called while termination is unproven. |
| +func (p *unitProc) finaliseGen() { |
| + if p.gen == 0 { |
| + return |
| + } |
| + p.removeLaunch() |
| + if p.genFilesGone.Load() { |
| + return |
| + } |
| + ctx, cancel := context.WithTimeout(context.Background(), instancectl.DeadlineControl) |
| + defer cancel() |
| + if err := p.stopUnit(ctx); err != nil { |
| + log.Printf("activator: %s: remove the files of %s: %v", p.rt.Host, p.unit, err) |
| + } |
| +} |
| + |
| func (p *unitProc) closeListeners() { |
| if p.closeOnce.Swap(true) { |
| return |
| @@ -202,7 +236,7 @@ func (b *ExecBackend) observe(p *unitProc) { |
| if foreign && !foreignStopped && !p.stopRequested.Load() { |
| log.Printf("activator: %s: unit restarted outside the activator (invocation %s, ours %s); stopping it", p.rt.Host, inv, p.invocation) |
| sctx, scancel := context.WithTimeout(context.Background(), instancectl.DeadlineControl) |
| - if err := b.stopRole(sctx, p.rt.Host, p.role, p.gen); err != nil { |
| + if err := p.stopUnit(sctx); err != nil { |
| log.Printf("activator: %s: stop foreign invocation: %v", p.rt.Host, err) |
| } |
| scancel() |
| @@ -236,10 +270,10 @@ func (b *ExecBackend) observe(p *unitProc) { |
| // clears records left over from EARLIER invocations; |
| // this one is this invocation's own to remove). |
| p.removeRecord() |
| - p.removeLaunch() |
| } else { |
| emitEvent(b.Logs, p.rt, "warn", "instance exited (status unknown: restarted outside the activator)") |
| } |
| + p.finaliseGen() |
| p.exited.Store(true) |
| // Stop closes the listeners when it observes done; a crash |
| // nobody is stopping yet leaves them for the sweeper's |
diff --git a/internal/activator/unitproc_gen_test.go b/internal/activator/unitproc_gen_test.go| index 286ff5903..552a09c7b 100644 |
| --- a/internal/activator/unitproc_gen_test.go |
| +++ b/internal/activator/unitproc_gen_test.go |
| @@ -270,3 +270,77 @@ func TestStartGenWithNoInvocationKillsTheGeneration(t *testing.T) { |
| t.Errorf("launch file left after a proven stop: %v", err) |
| } |
| } |
| + |
| +// genProcFixture is a generation-7 unitProc over fakes, with its launch |
| +// file on disk. |
| +func genProcFixture(t *testing.T) (*fakeControl, *fakeUnit, *ExecBackend, registry.Route, string) { |
| + t.Helper() |
| + ctl, unit := &fakeControl{}, &fakeUnit{} |
| + b, rt := nestedBackend(t, ctl, unit) |
| + b.GenContainment = true |
| + b.Control = fakeGenControl{ctl} |
| + p := b.pathsFor(rt) |
| + if err := os.MkdirAll(p.dataDir, 0o750); err != nil { |
| + t.Fatal(err) |
| + } |
| + launch := filepath.Join(p.dataDir, instancectl.GenLaunchFileName(7)) |
| + if err := os.WriteFile(launch, []byte("{}"), 0o600); err != nil { |
| + t.Fatal(err) |
| + } |
| + return ctl, unit, b, rt, launch |
| +} |
| + |
| +// A generation that exits on its own is finalised once observe proves it |
| +// gone: its launch file here, and the control service's drop-in and exe |
| +// layout through a stop of the inactive unit. A later Stop sends nothing. |
| +func TestGenerationNaturalExitFinalisesItsFiles(t *testing.T) { |
| + ctl, unit, b, rt, launch := genProcFixture(t) |
| + unit.set("active", "inv7", true, true) |
| + up := b.newUnitProcFor(rt, instancectl.RoleRun, 7, b.pathsFor(rt), "inv7", nil, nil) |
| + unit.set("inactive", "inv7", false, false) |
| + waitFor(t, "the generation to exit", up.Exited) |
| + if want := "stopgen h.example.com 7 kill=false"; ctl.joined() != want { |
| + t.Errorf("calls =\n%s\nwant\n%s", ctl.joined(), want) |
| + } |
| + if _, err := os.Stat(launch); !os.IsNotExist(err) { |
| + t.Errorf("launch file left after a natural exit: %v", err) |
| + } |
| + if err := up.Stop(context.Background()); err != nil { |
| + t.Fatal(err) |
| + } |
| + if want := "stopgen h.example.com 7 kill=false"; ctl.joined() != want { |
| + t.Errorf("a stop after the exit sent more:\n%s", ctl.joined()) |
| + } |
| +} |
| + |
| +// A generation restarted outside the activator is stopped, and once that |
| +// is proven its files go too. The stop that proved it already removed the |
| +// control service's files, so it is not asked twice. |
| +func TestGenerationForeignInvocationExitFinalisesItsFiles(t *testing.T) { |
| + ctl, unit, b, rt, launch := genProcFixture(t) |
| + ctl.stopHook = func(string, instancectl.Role) { unit.set("inactive", "", false, false) } |
| + unit.set("active", "inv7", true, true) |
| + up := b.newUnitProcFor(rt, instancectl.RoleRun, 7, b.pathsFor(rt), "inv7", nil, nil) |
| + unit.set("active", "someone-else", true, true) |
| + waitFor(t, "the generation to exit", up.Exited) |
| + if want := "stopgen h.example.com 7 kill=false"; ctl.joined() != want { |
| + t.Errorf("calls =\n%s\nwant\n%s", ctl.joined(), want) |
| + } |
| + if _, err := os.Stat(launch); !os.IsNotExist(err) { |
| + t.Errorf("launch file left after a foreign invocation was stopped: %v", err) |
| + } |
| +} |
| + |
| +// With no generation channel StartGen is never sent, so nothing ran: the |
| +// launch file written for it goes. |
| +func TestStartGenRefusedBeforeSendingRemovesTheLaunchFile(t *testing.T) { |
| + ctl := &fakeControl{inv: "inv7"} |
| + b, rt, launch, _ := genStartFixture(t, ctl) |
| + b.Control = ctl // a Control without the generation verbs |
| + if _, _, err := b.StartGen(context.Background(), rt, 7, false); !errors.Is(err, errNoGenControl) { |
| + t.Fatalf("StartGen = %v, want errNoGenControl", err) |
| + } |
| + if _, err := os.Stat(launch); !os.IsNotExist(err) { |
| + t.Errorf("launch file left by a start never sent: %v", err) |
| + } |
| +} |
diff --git a/internal/instancectl/gen.go b/internal/instancectl/gen.go| index 11c0b0e36..f06f188c3 100644 |
| --- a/internal/instancectl/gen.go |
| +++ b/internal/instancectl/gen.go |
| @@ -120,6 +120,8 @@ func (s *Server) startGen(w http.ResponseWriter, r *http.Request, req request) { |
| http.Error(w, "not starting "+unit+": generation units are not configured on this service", http.StatusNotImplemented) |
| return |
| } |
| + s.markGenStarting(unit, true) |
| + defer s.markGenStarting(unit, false) |
| // A refusal for the disk writes nothing. An exe layout that fails |
| // half-way may have made its directory, so that failure cleans up too. |
| if s.refuseStart(w, req.host, unit, func() error { |
| @@ -226,6 +228,76 @@ func (s *Server) stopGen(w http.ResponseWriter, r *http.Request, req request) { |
| w.WriteHeader(http.StatusNoContent) |
| } |
| +func (s *Server) markGenStarting(unit string, on bool) { |
| + s.genMu.Lock() |
| + defer s.genMu.Unlock() |
| + if !on { |
| + delete(s.genStarting, unit) |
| + return |
| + } |
| + if s.genStarting == nil { |
| + s.genStarting = map[string]bool{} |
| + } |
| + s.genStarting[unit] = true |
| +} |
| + |
| +// sweepGenLeftovers is the sweep's recovery for generation units that are |
| +// gone but kept their files: their stop's answer was lost, or the agent |
| +// that would have finalised them restarted. Such a unit is inactive, so |
| +// the sweep had nothing to stop. Each drop-in directory under DropInDir is |
| +// checked: a unit the sweep could not stop is skipped, as is one a start |
| +// is preparing. The rest lose their files only when systemd reports them |
| +// inactive or failed AND their cgroup is empty. No daemon-reload follows: |
| +// stopGen removes files the same way, and a generation's name is never |
| +// started again. |
| +func (s *Server) sweepGenLeftovers(ctx context.Context, failed map[string]string) { |
| + if s.DropInDir == "" { |
| + return |
| + } |
| + entries, err := os.ReadDir(s.DropInDir) |
| + if err != nil { |
| + if !errors.Is(err, os.ErrNotExist) { |
| + s.logf("instance-control: sweep: %s: %v", s.DropInDir, err) |
| + } |
| + return |
| + } |
| + for _, e := range entries { |
| + unit, ok := strings.CutSuffix(e.Name(), ".d") |
| + if !ok || !e.IsDir() { |
| + continue |
| + } |
| + if _, _, ok := ParseGenUnitName(unit); !ok { |
| + continue |
| + } |
| + if _, stuck := failed[unit]; stuck { |
| + continue |
| + } |
| + s.sweepGenLeftover(ctx, unit) |
| + } |
| +} |
| + |
| +func (s *Server) sweepGenLeftover(ctx context.Context, unit string) { |
| + // Held across the check and the removal, so a start of the same unit |
| + // cannot write fresh files in between. |
| + s.genMu.Lock() |
| + defer s.genMu.Unlock() |
| + if s.genStarting[unit] { |
| + return |
| + } |
| + if state := s.showValue(ctx, "ActiveState", unit); state != "inactive" && state != "failed" { |
| + return |
| + } |
| + _, esc, _ := ParseUnitName(unit) |
| + // One look at the cgroup, no wait: a populated one is not swept. |
| + wctx, cancel := context.WithTimeout(ctx, time.Millisecond) |
| + defer cancel() |
| + if err := WaitCgroupEmpty(wctx, s.CgroupRoot, "carlos-instances-"+esc+".slice", unit, time.Millisecond); err != nil { |
| + return |
| + } |
| + s.removeGenFiles(unit) |
| + s.logf("instance-control: sweep: removed the files of %s", unit) |
| +} |
| + |
| // handleParent sets the host slice's memory: the generation parent's |
| // backstop (spec §7.3). The swap raises it before the successor starts and |
| // restores it once the retiring generation has stopped. CPU and IO shares |
diff --git a/internal/instancectl/server.go b/internal/instancectl/server.go| index ac0b38f29..9d91bffcf 100644 |
| --- a/internal/instancectl/server.go |
| +++ b/internal/instancectl/server.go |
| @@ -99,6 +99,11 @@ type Server struct { |
| // cgroupWaitBudget bounds a wait for an empty cgroup; tests shorten it. |
| cgroupWaitBudget time.Duration |
| + // genMu guards genStarting, the generation units a start is preparing |
| + // now. The sweep never removes the files of one of them. |
| + genMu sync.Mutex |
| + genStarting map[string]bool |
| + |
| mu sync.Mutex |
| report ProbeReport |
| probed bool |
| @@ -615,6 +620,7 @@ func (s *Server) handleSweep(w http.ResponseWriter, r *http.Request) { |
| stopped = append(stopped, unit) |
| s.removeGenFiles(unit) |
| } |
| + s.sweepGenLeftovers(ctx, failed) |
| if len(failed) > 0 { |
| writeJSON(w, http.StatusBadGateway, SweepResponse{Units: stopped, Failed: failed}) |
| return |
diff --git a/internal/instancectl/server_gen_test.go b/internal/instancectl/server_gen_test.go| index 7b5346649..88317469f 100644 |
| --- a/internal/instancectl/server_gen_test.go |
| +++ b/internal/instancectl/server_gen_test.go |
| @@ -398,3 +398,55 @@ func TestGenStartDaemonReloadFailureRemovesTheFiles(t *testing.T) { |
| t.Errorf("files left behind: drop-in %v exe %v", dropIn, exe) |
| } |
| } |
| + |
| +// A generation that stopped while nobody removed its files (its stop's |
| +// answer was lost, the agent restarted) is inactive, so the sweep has no |
| +// unit to stop. Its leftovers still go once systemd reports it inactive |
| +// and its cgroup is empty. |
| +func TestSweepRemovesTheFilesOfAnInactiveGeneration(t *testing.T) { |
| + ctl := genCtl() |
| + s, sock, dropins := genServer(t, ctl) |
| + if err := s.writeGenDropIn(genHost, 7, cgroup.TierDefault); err != nil { |
| + t.Fatal(err) |
| + } |
| + if err := s.ensureGenExe(genHost, 7); err != nil { |
| + t.Fatal(err) |
| + } |
| + out, err := NewClient(sock).Sweep(context.Background()) |
| + if err != nil || len(out.Units) != 0 { |
| + t.Fatalf("sweep = %+v, %v", out, err) |
| + } |
| + if !strings.Contains(ctl.joined(), "show -p ActiveState --value "+genUnit) { |
| + t.Errorf("the leftover unit's state was not checked:\n%s", ctl.joined()) |
| + } |
| + if dropIn, exe := genFilesPresent(t, s, dropins); dropIn || exe { |
| + t.Errorf("leftovers of an inactive generation survived the sweep: drop-in %v exe %v", dropIn, exe) |
| + } |
| +} |
| + |
| +// Leftovers whose unit is not proven gone stay: one systemd reports |
| +// active, and one whose cgroup still holds processes. |
| +func TestSweepKeepsTheFilesOfAGenerationNotProvenGone(t *testing.T) { |
| + for name, setup := range map[string]func(*fakeCtl, *Server){ |
| + "active": func(c *fakeCtl, _ *Server) { c.answers["show -p ActiveState --value"] = "active\n" }, |
| + "populated": func(_ *fakeCtl, s *Server) { populateGenCgroup(t, s) }, |
| + } { |
| + t.Run(name, func(t *testing.T) { |
| + ctl := genCtl() |
| + s, sock, dropins := genServer(t, ctl) |
| + setup(ctl, s) |
| + if err := s.writeGenDropIn(genHost, 7, cgroup.TierDefault); err != nil { |
| + t.Fatal(err) |
| + } |
| + if err := s.ensureGenExe(genHost, 7); err != nil { |
| + t.Fatal(err) |
| + } |
| + if _, err := NewClient(sock).Sweep(context.Background()); err != nil { |
| + t.Fatal(err) |
| + } |
| + if dropIn, exe := genFilesPresent(t, s, dropins); !dropIn || !exe { |
| + t.Errorf("files of a generation not proven gone were removed: drop-in %v exe %v", dropIn, exe) |
| + } |
| + }) |
| + } |
| +} |