From 3a117c2d2bdd506dadbe0c97cab6b8c53e9fead9 Mon Sep 17 00:00:00 2001 From: jochen Date: Wed, 7 Oct 2026 02:13:17 +0200 Subject: [PATCH] Keep a replaced build reachable, and delete it only once no process runs from it (hq issue 289) The witness moved a running controller's build into a 0700 directory and deleted it on proof, while the old process could still be serving. Its directories are now 0711, and a build without a reader is retired and swept once /proc shows nothing runs from it. --- internal/witness/draining.go | 153 +++++++++++++++++++++++++++ internal/witness/draining_test.go | 169 ++++++++++++++++++++++++++++++ internal/witness/kept.go | 28 ++--- internal/witness/watch.go | 12 ++- 4 files changed, 348 insertions(+), 14 deletions(-) create mode 100644 internal/witness/draining.go create mode 100644 internal/witness/draining_test.go diff --git a/internal/witness/draining.go b/internal/witness/draining.go new file mode 100644 index 0000000..744b6c5 --- /dev/null +++ b/internal/witness/draining.go @@ -0,0 +1,153 @@ +package witness + +import ( + "fmt" + "os" + "path/filepath" + "sort" + "strconv" + "strings" + "time" +) + +// **A build is never deleted while a process runs from it** (novox/hq issue 289). +// +// The order of a witnessed update is: the running build moved aside as the previous one, the new one +// unpacked in its place, the unit restarted — the supervisor stops the old process, which drains, then +// starts the new — and the build before it retired once the new one is proved. Between the move and the +// stop the old process still serves, and a process may start another from its own image (the +// controller runs every verb as a command of its own binary). So, while that can happen: +// +// - the old build stays **reachable by the user it runs as**: the directories the engine keeps beside +// a process are 0711 — enterable by name, listable by root alone — never 0700, so a build moved +// aside is still at a path its process may open; +// - a build that has no reader any more is **retired**, not deleted: renamed under +// `.witness//retired/`, which leaves every process running from it untouched, and deleted +// only when no process on the machine runs from it. Which processes run from it is asked of the +// kernel — every process's executable — so the one it ran as is found by its PID, and so are the +// commands it started, wherever its unit is in stopping. +// +// The controller does its half too: it runs its verbs from its own running image rather than the path +// it started from, and refuses as a handover what it cannot run (mesh-controller, issue 289). + +// reachable is the mode of every directory the engine keeps beside a process: enterable by the +// process's own user, listable by nobody but root. +const reachable = 0o711 + +func retiredDir(root, name string) string { return filepath.Join(Dir(root, name), "retired") } + +// keep makes the directories the engine keeps about a process — made 0700 before issue 289 — reachable. +func keep(root, name string) error { + for _, dir := range []string{filepath.Join(root, ".witness"), Dir(root, name)} { + if err := os.MkdirAll(dir, reachable); err != nil { + return err + } + if err := os.Chmod(dir, reachable); err != nil { + return err + } + } + return nil +} + +// retire takes a build that has no reader any more out of the way: renamed under retired/, and deleted +// at once if nothing runs from it, else at a later sweep. A path that is not there is nothing to do. +func retire(root, name, dir string, now time.Time) error { + if _, err := os.Lstat(dir); os.IsNotExist(err) { + return nil + } + if err := keep(root, name); err != nil { + return err + } + into := retiredDir(root, name) + if err := os.MkdirAll(into, reachable); err != nil { + return err + } + if err := os.Chmod(into, reachable); err != nil { + return err + } + at := filepath.Join(into, fmt.Sprintf("%s-%d", filepath.Base(dir), now.UnixNano())) + if err := os.Rename(dir, at); err != nil { + return fmt.Errorf("cannot retire %s's build at %s: %w", name, dir, err) + } + _, err := Sweep(root, name) + return err +} + +// Sweep deletes every retired build of a process that no process runs from any more, and says which +// are kept, with the processes still running from each. +func Sweep(root, name string) ([]string, error) { + into := retiredDir(root, name) + entries, err := os.ReadDir(into) + if os.IsNotExist(err) { + return nil, nil + } + if err != nil { + return nil, err + } + var draining []string + for _, e := range entries { + dir := filepath.Join(into, e.Name()) + if pids := RunningFrom(dir); len(pids) > 0 { + draining = append(draining, fmt.Sprintf("%s (still run by PID %s)", dir, joinPIDs(pids))) + continue + } + if err := os.RemoveAll(dir); err != nil { + return draining, err + } + } + return draining, nil +} + +// RunningFrom is every process whose executable is a file under dir — renamed there, or deleted from +// there since it started. A variable so a test can name the machine's processes. +var RunningFrom = func(dir string) []int { + return runningFromProc("/proc", dir) +} + +func runningFromProc(proc, dir string) []int { + entries, err := os.ReadDir(proc) + if err != nil { + return nil + } + dir = filepath.Clean(dir) + string(filepath.Separator) + var pids []int + for _, e := range entries { + pid, err := strconv.Atoi(e.Name()) + if err != nil { + continue + } + exe, err := os.Readlink(filepath.Join(proc, e.Name(), "exe")) + if err != nil { + // Gone between the listing and the read, a kernel thread, or another user's process this + // engine may not read — it runs as root, so in practice the first two. + continue + } + if strings.HasPrefix(strings.TrimSuffix(exe, " (deleted)"), dir) { + pids = append(pids, pid) + } + } + sort.Ints(pids) + return pids +} + +func joinPIDs(pids []int) string { + words := make([]string, len(pids)) + for i, p := range pids { + words[i] = strconv.Itoa(p) + } + return strings.Join(words, ", ") +} + +// SweepAll is Sweep for every process the engine keeps anything about: what the witness does on every +// look, so a build retired while its process drained goes once the process has. +func SweepAll(root string) []string { + var draining []string + for _, s := range all(root) { + kept, err := Sweep(root, s.Process) + if err != nil { + draining = append(draining, fmt.Sprintf("%s: %v", s.Process, err)) + } + draining = append(draining, kept...) + } + return draining +} diff --git a/internal/witness/draining_test.go b/internal/witness/draining_test.go new file mode 100644 index 0000000..6488631 --- /dev/null +++ b/internal/witness/draining_test.go @@ -0,0 +1,169 @@ +package witness + +import ( + "io" + "os" + "os/exec" + "path/filepath" + "runtime" + "slices" + "testing" + "time" +) + +// A real process running from a build, as a controller runs from its own: a copy of sleep, started +// from the directory the build is unpacked in. +func runFrom(t *testing.T, dir string) *exec.Cmd { + t.Helper() + if runtime.GOOS != "linux" { + t.Skip("which process runs from a build is read from /proc") + } + sleep, err := exec.LookPath("sleep") + if err != nil { + t.Skip("no sleep to run") + } + if sleep, err = filepath.EvalSymlinks(sleep); err != nil { + t.Fatal(err) + } + in, err := os.Open(sleep) + if err != nil { + t.Fatal(err) + } + defer in.Close() + if err := os.MkdirAll(dir, 0o755); err != nil { + t.Fatal(err) + } + image := filepath.Join(dir, "sleep") + out, err := os.OpenFile(image, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, 0o755) + if err != nil { + t.Fatal(err) + } + if _, err := io.Copy(out, in); err != nil { + t.Fatal(err) + } + if err := out.Close(); err != nil { + t.Fatal(err) + } + cmd := exec.Command(image, "60") + if err := cmd.Start(); err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = cmd.Process.Kill(); _ = cmd.Wait() }) + return cmd +} + +func retiredBuilds(t *testing.T, root, name string) []string { + t.Helper() + entries, err := os.ReadDir(retiredDir(root, name)) + if err != nil && !os.IsNotExist(err) { + t.Fatal(err) + } + var out []string + for _, e := range entries { + out = append(out, e.Name()) + } + return out +} + +// **A build moved aside while its process runs stays reachable by that process's user, and is deleted +// only once no process runs from it** (novox/hq issue 289). On 2026-10-07 the controller still serving +// after its build was moved into a 0700 directory, and then deleted, could not run its own verbs. +func TestABuildIsKeptReachableAndUndeletedWhileAProcessRunsFromIt(t *testing.T) { + root := t.TempDir() + name := "mesh-controller" + start := time.Date(2026, 10, 7, 2, 0, 0, 0, time.UTC) + + // Build A placed and running. + p, err := Place(root, name, ByLease, buildA, "", "", start) + if err != nil { + t.Fatal(err) + } + old := runFrom(t, filepath.Join(root, name)) + if err := p.Commit(); err != nil { + t.Fatal(err) + } + + // Build B placed while A still runs: A is moved aside, and reachable at its new path. + place(t, root, name, ByLease, buildB, "", "", start.Add(time.Minute)) + for _, dir := range []string{filepath.Join(root, ".witness"), Dir(root, name)} { + info, err := os.Stat(dir) + if err != nil { + t.Fatal(err) + } + if info.Mode().Perm() != reachable { + t.Fatalf("%s is %v: the user the moved build runs as cannot reach it", dir, info.Mode().Perm()) + } + } + if pids := RunningFrom(previousDir(root, name)); !slices.Contains(pids, old.Process.Pid) { + t.Fatalf("the process running from the moved build (PID %d) is not found there: %v", old.Process.Pid, pids) + } + + // B proved while A has not stopped yet: A is retired, not deleted. + if err := Proved(root, name); err != nil { + t.Fatal(err) + } + if kept(root, name) { + t.Fatal("the build before the proved one is still kept to go back to") + } + if got := retiredBuilds(t, root, name); len(got) != 1 { + t.Fatalf("the build a process still runs from was deleted: retired %v", got) + } + draining, err := Sweep(root, name) + if err != nil || len(draining) != 1 { + t.Fatalf("a sweep while it runs: %v %v", draining, err) + } + + // A stops: the next sweep deletes it. + _ = old.Process.Kill() + _ = old.Wait() + if draining, err := Sweep(root, name); err != nil || len(draining) != 0 { + t.Fatalf("a sweep after it stopped: %v %v", draining, err) + } + if got := retiredBuilds(t, root, name); len(got) != 0 { + t.Fatalf("a build nothing runs from was kept: %v", got) + } +} + +// A process whose image was deleted from under it still counts as running from where it was. +func TestAProcessRunsFromABuildDeletedUnderIt(t *testing.T) { + dir := filepath.Join(t.TempDir(), "build") + proc := runFrom(t, dir) + if err := os.Remove(filepath.Join(dir, "sleep")); err != nil { + t.Fatal(err) + } + if pids := RunningFrom(dir); !slices.Contains(pids, proc.Process.Pid) { + t.Fatalf("PID %d runs a deleted image from %s and was not found: %v", proc.Process.Pid, dir, pids) + } + if pids := RunningFrom(filepath.Join(t.TempDir(), "elsewhere")); slices.Contains(pids, proc.Process.Pid) { + t.Fatal("found running from a directory it never ran from") + } +} + +// A build on trial replaced by another is retired too: it runs until the restart stops it. +func TestABuildOnTrialReplacedIsRetiredNotDeletedUnderItsProcess(t *testing.T) { + root := t.TempDir() + name := "node-tools" + start := time.Date(2026, 10, 7, 2, 0, 0, 0, time.UTC) + place(t, root, name, ByPing, buildA, "", "", start) + p, err := Place(root, name, ByPing, buildB, "", "", start.Add(time.Minute)) + if err != nil { + t.Fatal(err) + } + trial := runFrom(t, filepath.Join(root, name)) + if err := p.Commit(); err != nil { + t.Fatal(err) + } + place(t, root, name, ByPing, buildC, "", "", start.Add(2*time.Minute)) + if got := retiredBuilds(t, root, name); len(got) != 1 { + t.Fatalf("the build on trial was deleted while it ran: %v", got) + } + _ = trial.Process.Kill() + _ = trial.Wait() + SweepAll(root) + if got := retiredBuilds(t, root, name); len(got) != 0 { + t.Fatalf("kept after its process stopped: %v", got) + } + if running(t, root, name) != buildC || !kept(root, name) { + t.Fatal("C should run with A kept to go back to") + } +} diff --git a/internal/witness/kept.go b/internal/witness/kept.go index dd5c3a0..58db6a3 100644 --- a/internal/witness/kept.go +++ b/internal/witness/kept.go @@ -20,8 +20,9 @@ import ( // /.witness//previous/ the build before it, kept until the running one is proved // /.witness//state.json State // -// **Never deleted before the new build is proved**, and deleted once it is: the previous build has -// exactly one reader — a restoration — and none once the build after it has been seen healthy. +// **Never deleted before the new build is proved**, and retired once it is: the previous build has +// exactly one reader — a restoration — and none once the build after it has been seen healthy. Retired, +// not deleted: a build is deleted only once no process runs from it (draining.go, issue 289). // The running build's directory never moves while it is judged, so its unit is the same unit // throughout, and restoring is two renames and a restart. @@ -99,7 +100,7 @@ func Load(root, name string) (State, error) { // Save keeps it, whole or not at all. func Save(root string, s State) error { dir := Dir(root, s.Process) - if err := os.MkdirAll(dir, 0o700); err != nil { + if err := keep(root, s.Process); err != nil { return err } body, err := json.MarshalIndent(s, "", " ") @@ -197,15 +198,18 @@ func Place(root, name, by, declared, recorded, notReversible string, now time.Ti s = State{Process: name, Witness: by, Running: declared, Proven: true, Refused: s.Refused} return Placing{Unpack: true, Commit: func() error { return Save(root, s) }}, nil case s.OnTrial(): - // The running build has not been seen healthy: it is not what anybody goes back to. - if err := os.RemoveAll(at); err != nil { + // The running build has not been seen healthy: it is not what anybody goes back to. Retired, + // since it still runs until the restart (issue 289). + if err := retire(root, name, at, now); err != nil { return Placing{}, err } default: - if err := os.RemoveAll(previousDir(root, name)); err != nil { + if err := retire(root, name, previousDir(root, name), now); err != nil { return Placing{}, err } - if err := os.MkdirAll(Dir(root, name), 0o700); err != nil { + // Moved while its process still runs, until the restart stops it: reachable at its new path + // by the user it runs as (issue 289). + if err := keep(root, name); err != nil { return Placing{}, err } if err := os.Rename(at, previousDir(root, name)); err != nil { @@ -223,14 +227,14 @@ func Place(root, name, by, declared, recorded, notReversible string, now time.Ti return Placing{Unpack: true, Commit: func() error { return Save(root, s) }}, nil } -// Proved is the running build seen healthy: the build before it is deleted — it has no reader now — -// and what was concluded and refused before it ends. +// Proved is the running build seen healthy: the build before it is retired — it has no reader now, +// and is deleted once no process runs from it — and what was concluded and refused before it ends. func Proved(root, name string) error { s, err := Load(root, name) if err != nil || s.Process == "" { return err } - if err := os.RemoveAll(previousDir(root, name)); err != nil { + if err := retire(root, name, previousDir(root, name), time.Now()); err != nil { return err } s.Proven, s.Previous, s.Verdicts, s.Refused = true, "", nil, nil @@ -277,7 +281,7 @@ func Restore(ctx context.Context, root, name, why string, run Runner, now time.T _, _ = run(ctx, "systemctl", "stop", unit) at := filepath.Join(root, name) failed := filepath.Join(Dir(root, name), "failed") - if err := os.RemoveAll(failed); err != nil { + if err := retire(root, name, failed, now); err != nil { return restoreFailed(root, s, v, err) } if err := os.Rename(at, failed); err != nil && !errors.Is(err, os.ErrNotExist) { @@ -288,7 +292,7 @@ func Restore(ctx context.Context, root, name, why string, run Runner, now time.T _ = os.Rename(failed, at) return restoreFailed(root, s, v, err) } - _ = os.RemoveAll(failed) + _ = retire(root, name, failed, now) v.To, v.Outcome = s.Previous, link.RolledBack s.Refused = append(s.Refused, s.Running) diff --git a/internal/witness/watch.go b/internal/witness/watch.go index d52ac4f..2fe5d2f 100644 --- a/internal/witness/watch.go +++ b/internal/witness/watch.go @@ -4,6 +4,7 @@ import ( "context" "errors" "fmt" + "strings" "time" "github.com/novox/mesh-host/internal/link" @@ -54,11 +55,12 @@ func (w *Watcher) Watch(ctx context.Context) { } } -// Look judges every build on trial once. +// Look judges every build on trial once, and deletes every retired build no process runs from now. func (w *Watcher) Look(ctx context.Context) { for _, s := range Trials(w.Root) { w.look(ctx, s) } + w.hold(func() { SweepAll(w.Root) }) } func (w *Watcher) look(ctx context.Context, s State) { @@ -92,7 +94,13 @@ func (w *Watcher) look(ctx context.Context, s State) { } case healthy: if Proved(w.Root, s.Process) == nil { - w.say(fmt.Sprintf("%s %s is healthy: %s; the build before it is deleted", s.Process, short(s.Running), why)) + said := fmt.Sprintf("%s %s is healthy: %s; the build before it is retired", s.Process, short(s.Running), why) + if draining, _ := Sweep(w.Root, s.Process); len(draining) > 0 { + said += ", and deleted once nothing runs from it: " + strings.Join(draining, "; ") + } else { + said += " and deleted" + } + w.say(said) } default: current.Watched += every