fix: keep a save made while an install is still checking it stays up

cmd/fjordd/address_clash_test.go

@@ -75,6 +75,41 @@ func TestKeepAddressesSkipsATakenAddress(t *testing.T) {
 	}
 }
 
+// The type test on netlab: an install's stream runs on through its stay-up
+// watch, and host networking saved meanwhile was overwritten at its end by
+// the install-time copy, with the address pinned on top.
+func TestKeepAddressesKeepsASaveMadeSince(t *testing.T) {
+	s := clashFixture(t)
+	s.backends["podman"].(liveEngine).held["web"] = map[string]map[string]string{"web": {"vlan5": "192.168.5.30"}}
+	dir := filepath.Join(s.manager.StacksDir, "web")
+	os.MkdirAll(dir, 0o755)
+	os.WriteFile(filepath.Join(dir, "compose.yaml"), []byte("services:\n  web:\n    image: x\n    networks:\n      - vlan5\nnetworks:\n  vlan5:\n    external: true\n"), 0o644)
+	os.WriteFile(filepath.Join(dir, "state.json"), []byte(`{"schema_version":1,"engine":"podman"}`), 0o644)
+	stale, _ := s.manager.Get("web")
+	onVlan := stale.Compose
+
+	saved := "services:\n  web:\n    image: x\n    network_mode: host\n"
+	if err := s.manager.Save(&stack.Stack{Name: "web", Compose: saved, Env: "A=1\n"}); err != nil {
+		t.Fatal(err)
+	}
+	if kept, _ := s.keepAddresses(context.Background(), stale); len(kept) != 0 {
+		t.Errorf("pinned on networks the stack has left: %v", kept)
+	}
+	if after, _ := s.manager.Get("web"); after.Compose != saved || after.Env != "A=1\n" {
+		t.Errorf("the save was overwritten:\n%s\nenv %q", after.Compose, after.Env)
+	}
+
+	// Still on vlan5: the pin lands in the compose as saved, not in the copy.
+	s.manager.Save(&stack.Stack{Name: "web", Compose: strings.Replace(onVlan, "image: x", "image: y", 1)})
+	stale.Compose = onVlan
+	if kept, _ := s.keepAddresses(context.Background(), stale); len(kept) != 1 {
+		t.Errorf("kept: %v", kept)
+	}
+	if after, _ := s.manager.Get("web"); !strings.Contains(after.Compose, "image: y") || !strings.Contains(after.Compose, "ipv4_address: 192.168.5.30") {
+		t.Errorf("pin not on the saved compose:\n%s", after.Compose)
+	}
+}
+
 // A stack pinning what another stack holds, pinned or not, is a clash --
 // what pre-flight and Save refuse.
 func TestClashesPinnedAndLive(t *testing.T) {

cmd/fjordd/keep_address.go

@@ -58,71 +58,80 @@ func (s *server) keepAddresses(ctx context.Context, st *stack.Stack) (kept, skip
 			ids[c.Service] = c.ID
 		}
 	}
-	compose := st.Compose
 	// What other stacks have, worked out once and only if something is to be
 	// pinned: an address another stack has is never pinned here (see
 	// takenAddresses), it is reported.
 	var taken map[string]addrHolder
-	per := composepkg.ServiceAttachments(compose)
-	svcs := make([]string, 0, len(per))
-	for svc := range per {
-		svcs = append(svcs, svc)
-	}
-	sort.Strings(svcs)
-	for _, svc := range svcs {
-		for _, a := range per[svc] {
-			if private[a.Network] {
-				continue
-			}
-			n, ok := hostnet.Get(a.Network)
-			if !ok || n.Static {
-				continue
-			}
-			if n.DHCP {
-				if a.MAC != "" {
+	// Pinned into the compose as it is on disk now, not st's copy: st was
+	// loaded when the action started, and an install's stream runs on through
+	// its stay-up watch. Saving that copy at the end undid a Save made
+	// meanwhile -- host networking set during the watch came back as the old
+	// networks with this MAC pinned (netlab, type test).
+	pin := func(compose string) string {
+		kept, skipped = nil, nil
+		per := composepkg.ServiceAttachments(compose)
+		svcs := make([]string, 0, len(per))
+		for svc := range per {
+			svcs = append(svcs, svc)
+		}
+		sort.Strings(svcs)
+		for _, svc := range svcs {
+			for _, a := range per[svc] {
+				if private[a.Network] {
+					continue
+				}
+				n, ok := hostnet.Get(a.Network)
+				if !ok || n.Static {
 					continue
 				}
-				mac := hostnet.MACOf(a.Network, ids[svc])
-				if mac == "" {
+				if n.DHCP {
+					if a.MAC != "" {
+						continue
+					}
+					mac := hostnet.MACOf(a.Network, ids[svc])
+					if mac == "" {
+						continue
+					}
+					out, err := composepkg.PinMAC(compose, svc, a.Network, mac)
+					if err != nil || out == compose {
+						continue
+					}
+					compose = out
+					kept = append(kept, fmt.Sprintf("kept %s's MAC %s on %s, which its DHCP lease follows", svc, mac, a.Network))
 					continue
 				}
-				out, err := composepkg.PinMAC(compose, svc, a.Network, mac)
+				if a.IP != "" {
+					continue
+				}
+				ip := held[svc][a.Network]
+				if parsed := net.ParseIP(ip); parsed == nil || parsed.To4() == nil {
+					continue
+				}
+				if taken == nil {
+					taken = s.takenAddresses(ctx, st.Name)
+				}
+				if h, ok := taken[a.Network+"|"+ip]; ok {
+					skipped = append(skipped, clashText(svc, ip, a.Network, h, fixOnStackPage))
+					continue
+				}
+				out, err := composepkg.PinAddress(compose, svc, a.Network, ip)
 				if err != nil || out == compose {
 					continue
 				}
 				compose = out
-				kept = append(kept, fmt.Sprintf("kept %s's MAC %s on %s, which its DHCP lease follows", svc, mac, a.Network))
-				continue
-			}
-			if a.IP != "" {
-				continue
-			}
-			ip := held[svc][a.Network]
-			if parsed := net.ParseIP(ip); parsed == nil || parsed.To4() == nil {
-				continue
-			}
-			if taken == nil {
-				taken = s.takenAddresses(ctx, st.Name)
-			}
-			if h, ok := taken[a.Network+"|"+ip]; ok {
-				skipped = append(skipped, clashText(svc, ip, a.Network, h, fixOnStackPage))
-				continue
-			}
-			out, err := composepkg.PinAddress(compose, svc, a.Network, ip)
-			if err != nil || out == compose {
-				continue
+				kept = append(kept, fmt.Sprintf("kept %s on %s (%s)", svc, ip, a.Network))
 			}
-			compose = out
-			kept = append(kept, fmt.Sprintf("kept %s on %s (%s)", svc, ip, a.Network))
 		}
+		return compose
 	}
+	out, err := s.manager.UpdateCompose(st.Name, pin)
+	if err != nil {
+		return []string{"could not keep addresses: " + err.Error()}, skipped
+	}
+	st.Compose = out
 	if len(kept) == 0 {
 		return nil, skipped
 	}
-	st.Compose = compose
-	if err := s.manager.Save(st); err != nil {
-		return []string{"could not keep addresses: " + err.Error()}, skipped
-	}
 	return kept, skipped
 }
 

pkg/stack/stack.go

@@ -181,6 +181,10 @@ type Manager struct {
 	// records its rollback) otherwise both load the old file and the second
 	// save drops the first one's field.
 	locks sync.Map
+	// fileLocks holds a *sync.Mutex per stack, taken across each write of
+	// its files, so UpdateCompose's read-change-write cannot interleave with
+	// a Save.
+	fileLocks sync.Map
 }
 
 // NewManager initializes a new stack manager.
@@ -271,6 +275,7 @@ func (m *Manager) Save(stack *Stack) error {
 	if !ValidName(stack.Name) {
 		return ErrInvalidName
 	}
+	defer lockIn(&m.fileLocks, stack.Name)()
 	dir := filepath.Join(m.StacksDir, stack.Name)
 	if err := os.MkdirAll(dir, 0755); err != nil {
 		return fmt.Errorf("failed to create stack directory: %w", err)
@@ -529,9 +534,34 @@ func (m *Manager) UpdateState(name string, change func(st *State) *State) error
 	return m.saveState(name, st)
 }
 
+// UpdateCompose changes a stack's compose.yaml as it is on disk now, under
+// the lock Save takes: change gets the compose and returns the one to write
+// (the same text writes nothing). For fjord's own edits at the end of a long
+// action -- the copy that action loaded can be minutes old, and saving it
+// whole put back a compose the user had saved over meanwhile.
+func (m *Manager) UpdateCompose(name string, change func(compose string) string) (string, error) {
+	if !ValidName(name) {
+		return "", ErrInvalidName
+	}
+	defer lockIn(&m.fileLocks, name)()
+	path := filepath.Join(m.StacksDir, name, "compose.yaml")
+	old, err := os.ReadFile(path)
+	if err != nil {
+		return "", err
+	}
+	out := change(string(old))
+	if out == string(old) {
+		return out, nil
+	}
+	return out, writeFileAtomic(path, []byte(out), 0644)
+}
+
 // lock takes the stack's state lock and returns its release.
-func (m *Manager) lock(name string) func() {
-	mu, _ := m.locks.LoadOrStore(name, &sync.Mutex{})
+func (m *Manager) lock(name string) func() { return lockIn(&m.locks, name) }
+
+// lockIn takes name's mutex in locks and returns its release.
+func lockIn(locks *sync.Map, name string) func() {
+	mu, _ := locks.LoadOrStore(name, &sync.Mutex{})
 	mu.(*sync.Mutex).Lock()
 	return mu.(*sync.Mutex).Unlock
 }