amadan

carlos / platform Public

Merge branch 'fix-system-apply-dirs' into main

claude 5a409bdd260d1edd39e2e66c417aaa7585bc45e1 parent e6773f5parent a193165
4 files changed, +277 −14
  • internal/host/bootstrap.go +55 −8
  • internal/host/host_test.go +81 −0
  • internal/host/systemapply.go +41 −6
  • internal/host/systemapply_test.go +100 −0

CI passed — run details

diff --git a/internal/host/bootstrap.go b/internal/host/bootstrap.go
index 39aead0ff..89d232ffc 100644
--- a/internal/host/bootstrap.go
+++ b/internal/host/bootstrap.go
@@ -74,6 +74,15 @@ const tmpfilesConf = "d /run/carlos 2750 " + serviceUser + " " + socketsGroup +
// line just set. Both are 0700 so re-runs converge instead of racing.
"d /data/instances 0700 " + serviceUser + " " + serviceUser + " -\n" +
"d /data/layout 0700 " + serviceUser + " " + serviceUser + " -\n" +
+ // The relay's one writable state dir (carlos-relay.service's
+ // ReadWritePaths). It is in bootstrap's dirs list too, but that list
+ // only ever runs against a FRESH box or a staging root — a self-update
+ // apply creates no directories, it re-runs tmpfiles. Without this line
+ // a box bootstrapped before the relay existed gets the unit, the
+ // binary and the master file and still fails to start with "Failed to
+ // set up mount namespacing: /data/relay: No such file or directory"
+ // (discussion #118, hit on the Ireland box 2026-09-07).
+ "d /data/relay 0750 " + serviceUser + " " + serviceUser + " -\n" +
"d /run/carlos/.instances 2750 " + serviceUser + " " + socketsGroup + " -\n" +
"d /run/carlos/instances 2750 " + serviceUser + " " + socketsGroup + " -\n" +
// The readiness probe's own instance directory (design §8.5): a name
@@ -161,13 +170,50 @@ var inertUnits = []string{"carlos-restore-verify.timer", "carlos-hibernate-verif
// bootstrapPlan is what `bootstrap -root <dir> -plan-json` prints: the
// arming list of THIS binary, so a self-update apply arms what the new
// binary's bootstrap would arm and never a list an older binary remembers.
+//
+// Armed is the other half of that promise, and it was missing until
+// discussion #118. Enabled alone says which units to `enable`; WHICH of them
+// also get a `start` was decided by the applier's own systemctlPlan — code
+// from the OLD binary, which cannot know about a unit the new build just
+// introduced. That is how the relay landed on the Ireland box enabled but
+// never started on 2026-09-07: the applying build predated
+// carlos-relay.service, so its systemctlPlan had no reason to start it.
+// Carrying the list the TARGET build would start closes that gap for every
+// future unit, not just the relay.
+//
+// Additive on purpose: a plan.json written by a build older than this field
+// (a rollback stages carlos.prev's plan) carries no "armed" key, and arm()
+// then decides locally exactly as it always did. The key is NOT omitempty:
+// nil ("this build is too old to say") and empty ("start nothing") are
+// different answers, and omitempty would collapse them into the same absent
+// key, making "start nothing" impossible for a build to express.
type bootstrapPlan struct {
Enabled []string `json:"enabled"`
Inert []string `json:"inert"`
+ Armed []string `json:"armed"`
}
func currentBootstrapPlan() bootstrapPlan {
- return bootstrapPlan{Enabled: append([]string(nil), enabledUnits...), Inert: append([]string(nil), inertUnits...)}
+ return bootstrapPlan{
+ Enabled: append([]string(nil), enabledUnits...),
+ Inert: append([]string(nil), inertUnits...),
+ Armed: armedUnits(enabledUnits),
+ }
+}
+
+// armedUnits is the "and start these too" rule in one place: the unit types
+// whose enable-without-start is a silent no-op until the next boot, plus
+// carlos-relay.service, whose ConditionPathExists makes starting it safe on a
+// box with no region master. See systemctlPlan for the long version.
+func armedUnits(units []string) []string {
+ var armed []string
+ for _, u := range units {
+ if strings.HasSuffix(u, ".timer") || strings.HasSuffix(u, ".path") ||
+ strings.HasSuffix(u, ".socket") || strings.HasSuffix(u, ".mount") || u == relayUnitName {
+ armed = append(armed, u)
+ }
+ }
+ return armed
}
// systemctlPlan is the exact systemctl sequence a live bootstrap runs — and
@@ -206,17 +252,18 @@ func currentBootstrapPlan() bootstrapPlan {
// tree with no protection, until the next reboot. Starting either is
// idempotent.
func systemctlPlan(units []string) [][]string {
+ return systemctlPlanArming(units, armedUnits(units))
+}
+
+// systemctlPlanArming is systemctlPlan with the start list supplied rather
+// than derived — the shape a self-update apply needs, where the units come
+// from the target build's plan.json and so does the decision about which of
+// them to start (discussion #118).
+func systemctlPlanArming(units, armed []string) [][]string {
plan := [][]string{
{"daemon-reload"},
append([]string{"enable"}, units...),
}
- var armed []string
- for _, u := range units {
- if strings.HasSuffix(u, ".timer") || strings.HasSuffix(u, ".path") ||
- strings.HasSuffix(u, ".socket") || strings.HasSuffix(u, ".mount") || u == relayUnitName {
- armed = append(armed, u)
- }
- }
if len(armed) > 0 {
plan = append(plan, append([]string{"start"}, armed...))
}
diff --git a/internal/host/host_test.go b/internal/host/host_test.go
index 38b6ebd3e..77d9f305d 100644
--- a/internal/host/host_test.go
+++ b/internal/host/host_test.go
@@ -6,6 +6,7 @@ import (
"crypto/ed25519"
"encoding/base64"
"encoding/json"
+ "fmt"
"log"
"net"
"os"
@@ -1675,3 +1676,83 @@ func TestBootstrapWritesJournaldCap(t *testing.T) {
t.Errorf("drop-in gone after a re-run: %v", err)
}
}
+
+// TestTmpfilesCarriesRelayStateDir is discussion #118 as a test. The relay's
+// unit declares ReadWritePaths=/data/relay, and systemd refuses to start a
+// unit whose ReadWritePaths names a directory that is not there — "Failed to
+// set up mount namespacing: /data/relay: No such file or directory", which is
+// what the Ireland box answered on 2026-09-07.
+//
+// bootstrap's own dirs list creates it, and that is not enough: the dirs list
+// runs on a FRESH box or against a staging root, never against the live root
+// of a self-update apply. What an apply re-runs is systemd-tmpfiles. So the
+// directory has to be in tmpfilesConf, and this test is what keeps a future
+// unit's writable state dir from being added to one list and not the other.
+func TestTmpfilesCarriesRelayStateDir(t *testing.T) {
+ b, err := unitsFS.ReadFile("units/" + relayUnitName)
+ if err != nil {
+ t.Fatal(err)
+ }
+ var rw string
+ for _, line := range strings.Split(string(b), "\n") {
+ if v, ok := strings.CutPrefix(line, "ReadWritePaths="); ok {
+ rw = v
+ break
+ }
+ }
+ if rw == "" {
+ t.Fatal("carlos-relay.service declares no ReadWritePaths")
+ }
+ for _, p := range strings.Fields(rw) {
+ if !strings.HasPrefix(p, "/data/") {
+ continue // /run/carlos is tmpfs and already has its own line
+ }
+ want := "d " + p + " "
+ if !strings.Contains(tmpfilesConf, want) {
+ t.Errorf("tmpfilesConf has no %q line, so an apply onto a box that predates the unit leaves it unable to start:\n%s", want, tmpfilesConf)
+ }
+ }
+ // The mode must match bootstrap's dirs list, or a re-run of one flips
+ // what the other just set (the /data/instances lesson, one line up).
+ // Read the mode bootstrap actually uses rather than restating it: a
+ // hardcoded "0750" here passes happily while the two lists disagree.
+ root := t.TempDir()
+ if err := bootstrapWith(root, nil, false); err != nil {
+ t.Fatal(err)
+ }
+ fi, err := os.Stat(filepath.Join(root, "data", "relay"))
+ if err != nil {
+ t.Fatalf("bootstrap did not create /data/relay: %v", err)
+ }
+ want := fmt.Sprintf("d /data/relay %04o ", fi.Mode().Perm())
+ if !strings.Contains(tmpfilesConf, want) {
+ t.Errorf("tmpfilesConf must carry %q to match the mode bootstrap's dirs list creates:\n%s", want, tmpfilesConf)
+ }
+}
+
+// TestPlanCarriesArmedList pins the other half of discussion #118: the plan a
+// build prints says which units to START, not just which to enable, so an
+// applier running an OLDER binary arms what the new build wants rather than
+// what its own systemctlPlan happens to know about.
+func TestPlanCarriesArmedList(t *testing.T) {
+ plan := currentBootstrapPlan()
+ if len(plan.Armed) == 0 {
+ t.Fatal("plan carries no armed list")
+ }
+ if !slices.Contains(plan.Armed, relayUnitName) {
+ t.Errorf("armed list omits the relay: %v", plan.Armed)
+ }
+ if slices.Contains(plan.Armed, "carlos-edge.service") {
+ t.Errorf("edge must stay start-by-hand: %v", plan.Armed)
+ }
+ for _, u := range plan.Armed {
+ if !slices.Contains(plan.Enabled, u) {
+ t.Errorf("armed unit %s is not in the enabled list", u)
+ }
+ }
+ // The armed list and the local rule must agree for THIS build; they
+ // diverge only across builds, which is the point of carrying it.
+ if got, want := plan.Armed, armedUnits(enabledUnits); !slices.Equal(got, want) {
+ t.Errorf("plan.Armed = %v, systemctlPlan would start %v", got, want)
+ }
+}
diff --git a/internal/host/systemapply.go b/internal/host/systemapply.go
index 5ad62425e..198a3a074 100644
--- a/internal/host/systemapply.go
+++ b/internal/host/systemapply.go
@@ -710,13 +710,48 @@ func (a *systemApplier) gateOnce(ctx context.Context, target string) error {
return nil
}
-// arm is systemctlPlan verbatim: enable all, start timers and paths, and of
-// the services only carlos-relay, whose ConditionPathExists makes the start
-// a no-op on a box with no region master (relay spec §3). It starts nothing
-// else — in particular not the edge, which is running this very code — which
-// is why apply can call it from inside itself.
+// arm enables every unit in the plan and starts the ones the plan says to
+// start: the unit types whose enable-without-start is a silent no-op until
+// the next boot, and of the services only carlos-relay, whose
+// ConditionPathExists makes the start a no-op on a box with no region master
+// (relay spec §3).
+//
+// It never starts the edge, which is running this very code — that is why
+// apply can call it from inside itself. Before #118 that was structural: the
+// applier derived the start list itself and could not name a service other
+// than the relay. Now the list comes from the target build, so the guarantee
+// is enforced here instead, explicitly.
func (a *systemApplier) arm(plan bootstrapPlan) error {
- for _, args := range systemctlPlan(plan.Enabled) {
+ // plan.Armed is the TARGET build's answer to "and which of these get a
+ // start" (discussion #118). Preferring it is what lets a build introduce
+ // a unit that must be started at apply time rather than at the next
+ // boot; falling back to the local rule keeps a plan.json from an older
+ // build — which is what a rollback stages — working unchanged.
+ armed := plan.Armed
+ if armed == nil {
+ armed = armedUnits(plan.Enabled)
+ }
+ // Two guards on a list that now comes from another build. The edge is
+ // never started here, whatever the plan says. And a unit that is not in
+ // Enabled is dropped rather than passed to systemctl: `start` is ONE
+ // batched call, so a single name this box has no unit file for would
+ // fail the whole call and leave nothing armed at all.
+ enabled := make(map[string]bool, len(plan.Enabled))
+ for _, u := range plan.Enabled {
+ enabled[u] = true
+ }
+ kept := armed[:0:0]
+ for _, u := range armed {
+ switch {
+ case u == systemEdgeUnit || u == systemEdgeUnit+".service":
+ a.ops.logf("system apply: arm: refusing to start %s, which is running this code", u)
+ case !enabled[u]:
+ a.ops.logf("system apply: arm: %s is armed by the plan but not enabled by it; skipping", u)
+ default:
+ kept = append(kept, u)
+ }
+ }
+ for _, args := range systemctlPlanArming(plan.Enabled, kept) {
if _, err := a.ops.systemctl(args...); err != nil {
return fmt.Errorf("systemctl %s: %w", strings.Join(args, " "), err)
}
diff --git a/internal/host/systemapply_test.go b/internal/host/systemapply_test.go
index bf642a8a0..baf7b426a 100644
--- a/internal/host/systemapply_test.go
+++ b/internal/host/systemapply_test.go
@@ -5,9 +5,11 @@ import (
"crypto/ed25519"
"crypto/sha256"
"encoding/hex"
+ "encoding/json"
"errors"
"os"
"path/filepath"
+ "slices"
"strings"
"testing"
"time"
@@ -1253,3 +1255,101 @@ func TestRecoveryStillHandlesALegacyBackingUpJournal(t *testing.T) {
t.Fatalf("result = %+v, want the run to carry on to its request", r)
}
}
+
+// TestApplyArmsTheTargetBuildsList is the forward-compatibility half of
+// discussion #118. The applier is the OLD binary: when the target build
+// introduces a unit that must be started at apply time rather than at the
+// next boot, only the target build's own plan knows that. So arm() starts
+// what plan.Armed names, even a unit this binary's own rule would skip.
+//
+// Without this, a unit new to the box is enabled and left inactive — which
+// is exactly how carlos-relay.service reached the Ireland box on 2026-09-07
+// enabled, gated, armed with a masters file, and not running.
+func TestApplyArmsTheTargetBuildsList(t *testing.T) {
+ _, started := armCalls(t, bootstrapPlan{
+ Enabled: []string{"carlos-edge.service", "carlos-future.service"},
+ // A service this binary's armedUnits would never pick.
+ Armed: []string{"carlos-future.service"},
+ })
+ if !slices.Contains(started, "carlos-future.service") {
+ t.Errorf("apply did not start the unit the target build asked for: started %v", started)
+ }
+ if slices.Contains(started, "carlos-edge.service") {
+ t.Errorf("apply started the edge, which runs this very code: started %v", started)
+ }
+}
+
+// The plan's armed list comes from another build, so arm() does not take it
+// on trust: the edge is never started whatever the plan says, and a unit the
+// plan arms but does not enable is dropped rather than passed to systemctl,
+// because one bad name in the single batched `start` would lose every unit
+// in it.
+func TestApplyRefusesToArmTheEdgeOrAnUnenabledUnit(t *testing.T) {
+ _, started := armCalls(t, bootstrapPlan{
+ Enabled: []string{"carlos-adopt.timer", "carlos-edge.service"},
+ Armed: []string{"carlos-adopt.timer", "carlos-edge.service", "carlos-never-installed.service"},
+ })
+ if slices.Contains(started, "carlos-edge.service") {
+ t.Errorf("the edge was started from the plan's armed list: started %v", started)
+ }
+ if slices.Contains(started, "carlos-never-installed.service") {
+ t.Errorf("a unit the plan never enabled was handed to systemctl: started %v", started)
+ }
+ if !slices.Contains(started, "carlos-adopt.timer") {
+ t.Errorf("the good unit was lost along with the bad ones: started %v", started)
+ }
+}
+
+// A plan.json from a build older than the Armed field has no armed key. That
+// is what a rollback stages (carlos.prev's plan), so the fallback is not a
+// theoretical path — arm() must decide locally exactly as it did before.
+func TestApplyArmsLocallyWhenPlanPredatesArmed(t *testing.T) {
+ var plan bootstrapPlan
+ if err := json.Unmarshal([]byte(`{"enabled":["carlos-adopt.timer","carlos-edge.service","carlos-relay.service"]}`), &plan); err != nil {
+ t.Fatal(err)
+ }
+ if plan.Armed != nil {
+ t.Fatalf("armed should be absent, got %v", plan.Armed)
+ }
+ _, started := armCalls(t, plan)
+ // Membership, not substring: the relay is batched onto the same start
+ // line as the timer, and its bare name also appears on the enable line.
+ // A substring check is how the first version of this test passed
+ // against a fallback changed to drop the relay — the exact #118 failure
+ // mode it exists to catch.
+ for _, want := range []string{"carlos-adopt.timer", "carlos-relay.service"} {
+ if !slices.Contains(started, want) {
+ t.Errorf("fallback did not arm %s: started %v", want, started)
+ }
+ }
+ if slices.Contains(started, "carlos-edge.service") {
+ t.Errorf("fallback started the edge: started %v", started)
+ }
+}
+
+// armCalls runs arm(plan) against a recording systemd and returns the units
+// it enabled and the units it started. Both are read as SETS out of the two
+// batched systemctl lines: arm puts every unit on one `enable` and every
+// started unit on one `start`, so a substring check over the joined log
+// cannot tell "start carlos-relay.service" from the same name appearing in
+// the enable line, and cannot see a name that is merely batched beside
+// another. The first version of these tests got both wrong.
+func armCalls(t *testing.T, plan bootstrapPlan) (enabled, started []string) {
+ t.Helper()
+ a := &systemApplier{ops: sysOps{
+ systemctl: func(args ...string) (string, error) {
+ switch args[0] {
+ case "enable":
+ enabled = append(enabled, args[1:]...)
+ case "start":
+ started = append(started, args[1:]...)
+ }
+ return "", nil
+ },
+ logf: func(string, ...any) {},
+ }}
+ if err := a.arm(plan); err != nil {
+ t.Fatal(err)
+ }
+ return enabled, started
+}