betterbird: write the account lists only while the profile is closed, and never propose over an unreadable layer (hq issue 586, review 121)

writeAccountLists checked only for a running Betterbird and wrote without a sync;
it now waits for the profile's lock too and writes atomically. readLayer read any
answer that was not JSON as an empty layer, and a proposal replaces the layer
whole, so a changed wording would have dropped accounts; it now refuses. The
README says the rollout order that leaves no node on Betterbird's defaults.

modules/betterbird/README.md

@@ -99,7 +99,14 @@ overrides one preference, or takes one out with `null`. The operator's setup app
 density, 14-point text, the vertical view with multi-line cards and threads, no new-mail pop-up, sound or tray
 icon, the calendar's day from 09:00 to 20:00, reminders missed while asleep shown, chat off, remote content
 blocked, Do Not Track; 21 preferences) moved from the catalogue's defaults file into the mesh-wide layer of
-`prefs`, proposed with `betterbird_derive` and set on the operator's Approve.
+`prefs`, set on the operator's Approve.
+
+**The order that leaves no node on Betterbird's own defaults** (issue 586): merge this build (the `record`
+upgrade policy holds it back from the nodes); propose the mesh-wide layer through the controller's `settings`
+verb with `propose`, carrying the current `accounts` verbatim and `prefs` with the 21 values; the operator
+approves; only then push the desktop nodes. Caveat: between that approval and a node's push, any other send to a
+desktop node writes the map's text into the old build's user.js, and `betterbird_check` there says the user.js is
+refused until the push.
 
 **Locked** (the policy): Betterbird's updater off (`DisableAppUpdate`); telemetry off (`DisableTelemetry`);
 add-ons change only through the mesh's pins (`ExtensionUpdate` false); and three preferences locked off:
@@ -327,7 +334,7 @@ outside the mesh. With this version, a push to a node (each waits for the operat
 | `/usr/local` | the start, the launcher entry, the tools' program |
 | `~/.config/betterbird/` | `prefs.json`, `accounts.json`, `configuration.json`, `tags.json` (each empty until its setting is approved) |
 | the profile | untouched until the next start from the launcher, `betterbird_user_js` with apply, or `betterbird_restart` |
-| the configuration | the policy's locks at Betterbird's next start; the settings in the user.js from the next start after they are approved. Until the operator approves the mesh-wide layer of `prefs`, Betterbird runs with its own defaults where the module's defaults file held the operator's 21 preferences before issue 586 |
+| the configuration | the policy's locks at Betterbird's next start; the settings in the user.js from the next start. Pushed in the order above, the operator's 21 preferences are in `prefs` before the build arrives; pushed before that layer is approved, Betterbird runs with its own defaults for them until it is |
 
 ## Relies on
 

modules/betterbird/cmd/betterbird-tools/accounts.go

@@ -398,10 +398,6 @@ func unquote(v string) string {
 // (§12): the declared accounts' keys added at the end, the module's keys of accounts no longer declared taken
 // out, every other key and line kept as it was. Only while Betterbird is not running. Says what changed.
 func (m *Machine) writeAccountLists(profile string, resolved []resolvedAccount) (string, error) {
-	if ps := m.mainProcs(); len(ps) > 0 {
-		return "", fmt.Errorf("Betterbird is running (pid %d), and it rewrites prefs.js when it quits: the account "+
-			"list is written at its next start from the launcher, or by betterbird_restart", ps[0].PID)
-	}
 	path := filepath.Join(profile, "prefs.js")
 	raw, err := readBounded(path)
 	if err != nil && !errors.Is(err, os.ErrNotExist) {
@@ -418,16 +414,15 @@ func (m *Machine) writeAccountLists(profile string, resolved []resolvedAccount)
 	if !changed {
 		return "unchanged", nil
 	}
+	// Only while Betterbird is closed: no main process and no lock on the profile (novox/hq ADR 0316 §12, ADR 0329 §9).
+	if why := m.closedFor(profile); why != "" {
+		return "", errors.New(why + ": the account list is written at its next start from the launcher, or by betterbird_restart")
+	}
 	mode := os.FileMode(0o600)
 	if st, err := os.Stat(path); err == nil {
 		mode = st.Mode().Perm()
 	}
-	tmp := path + ".mesh-new"
-	if err := os.WriteFile(tmp, []byte(out), mode); err != nil {
-		return "", err
-	}
-	if err := os.Rename(tmp, path); err != nil {
-		os.Remove(tmp)
+	if err := writeAtomic(path, []byte(out), mode); err != nil {
 		return "", err
 	}
 	return fmt.Sprintf("written: %d account(s) the module created in the account list", len(wantAccounts)), nil

modules/betterbird/cmd/betterbird-tools/accounts_test.go

@@ -541,3 +541,27 @@ func TestAnAccountsOwnAliasesAndNonMailIdentitiesDoNotShareItsOutgoingServer(t *
 		t.Fatalf("%v", err)
 	}
 }
+
+// Review 121: the account lists wait for Betterbird to close by its profile's lock too, not only by its process.
+func TestTheAccountListsWaitWhileTheProfileIsLocked(t *testing.T) {
+	f := newFake(t)
+	f.profileWithAccounts()
+	f.noPackage()
+	links(t, nil)
+	f.declare("// none")
+	f.declareAccounts(personal)
+	stopBetterbird(t, f)
+	f.write(profileDir+"/lock", "")
+	before := f.read(profileDir + "/prefs.js")
+	u, err := f.userJS(true)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if !strings.HasPrefix(u.Lists, "not written") || !strings.Contains(u.Lists, "locked") || f.read(profileDir+"/prefs.js") != before {
+		t.Fatalf("with the profile locked and no Betterbird running, prefs.js is unchanged: %q", u.Lists)
+	}
+	os.Remove(filepath.Join(f.Root, profileDir, "lock"))
+	if u, err = f.userJS(true); err != nil || !strings.HasPrefix(u.Lists, "written") {
+		t.Fatalf("once unlocked the list is written: %q %v", u.Lists, err)
+	}
+}

modules/betterbird/cmd/betterbird-tools/derive.go

@@ -567,14 +567,21 @@ func readLayer(node string) (map[string]any, error) {
 	if !ok {
 		return nil, fmt.Errorf("the controller did not answer the layer: %s", said)
 	}
-	out := map[string]any{}
+	// A missing layer is said in exactly these words; anything else must read as the layer's JSON. A proposal
+	// replaces the layer whole, so an answer read wrongly as empty would drop what the layer holds: refused, loudly.
 	text := strings.TrimSpace(said)
-	if !strings.HasPrefix(text, "{") {
-		return out, nil
+	where := "the whole mesh"
+	if node != "" {
+		where = node
 	}
-	dec := json.NewDecoder(strings.NewReader(text))
-	if err := dec.Decode(&out); err != nil {
-		return nil, fmt.Errorf("the layer the controller answered cannot be read: %v", err)
+	if strings.HasPrefix(text, "betterbird on "+where+": no layer") {
+		return map[string]any{}, nil
+	}
+	out := map[string]any{}
+	if err := json.NewDecoder(strings.NewReader(text)).Decode(&out); err != nil || !strings.HasPrefix(text, "{") {
+		first, _, _ := strings.Cut(text, "\n")
+		return nil, fmt.Errorf("the controller's answer for the layer of betterbird on %s cannot be read as the layer "+
+			"(it begins %q), so nothing is proposed: a layer is replaced whole and would lose what it holds", where, first)
 	}
 	return out, nil
 }

modules/betterbird/cmd/betterbird-tools/derive_test.go

@@ -22,6 +22,8 @@ type derivingMesh struct {
 	proposed []map[string]any
 	asked    []string
 	asleep   bool
+	// garbled answers the layer in words that are not its JSON.
+	garbled bool
 }
 
 func (d *derivingMesh) ask(key string, body any) (json.RawMessage, error) {
@@ -60,9 +62,16 @@ func (d *derivingMesh) ask(key string, body any) (json.RawMessage, error) {
 		return answer(true, "proposal sab12: Set the settings of betterbird?\n  fingerprint ab12")
 	case key == "seat:mesh-controller.settings":
 		node, _ := args["node"].(string)
+		if d.garbled {
+			return answer(true, "Settings of betterbird on "+node+":\n  accounts = {…}\n")
+		}
 		layer := d.layers[node]
 		if layer == nil {
-			return answer(true, "betterbird on "+node+": no layer — the module's definition says\nbetterbird on "+node+", every value")
+			where := node
+			if where == "" {
+				where = "the whole mesh"
+			}
+			return answer(true, "betterbird on "+where+": no layer — the module's definition says\nbetterbird on "+where+", every value")
 		}
 		b, _ := json.MarshalIndent(layer, "", "  ")
 		return answer(true, string(b)+"\n  approved by the operator\nbetterbird on the whole mesh, every value and where it comes from:\n")
@@ -219,3 +228,36 @@ func TestDeriveLaysEachEntryOutPerLayerAndProposesOneLayerAtATime(t *testing.T)
 		t.Fatalf("%v", err)
 	}
 }
+
+// Review 121: a layer the controller answers in any other words than its JSON or "no layer" refuses the proposal,
+// because a proposal replaces the layer whole and would drop accounts.
+func TestALayerThatCannotBeReadRefusesTheProposal(t *testing.T) {
+	f := newFake(t)
+	f.profileWithAccounts()
+	f.profileWithConfiguration()
+	f.declareAccounts(adoptable)
+	links(t, nil)
+	f2 := newFake(t)
+	f2.profileWithAccounts()
+	f2.declareAccounts(adoptable)
+	mesh := &derivingMesh{t: t, shanks: f2, garbled: true}
+	oldNode, oldAsk := nodeName, asker
+	t.Cleanup(func() { nodeName, asker = oldNode, oldAsk })
+	nodeName = func() string { return "g14" }
+	asker = mesh.ask
+	if _, err := f.Derive(nil, false, true, nil, ""); err == nil || !strings.Contains(err.Error(), "cannot be read as the layer") ||
+		len(mesh.proposed) != 0 {
+		t.Fatalf("the derive refuses: %v %v", err, mesh.proposed)
+	}
+	if _, err := f.ProfileAccounts(true); err == nil || !strings.Contains(err.Error(), "cannot be read as the layer") ||
+		len(mesh.proposed) != 0 {
+		t.Fatalf("betterbird_accounts refuses: %v %v", err, mesh.proposed)
+	}
+	if _, err := readLayer(""); err == nil {
+		t.Fatal("an unreadable layer is an error, never an empty layer")
+	}
+	mesh.garbled = false
+	if got, err := readLayer("shanks"); err != nil || len(got) != 0 {
+		t.Fatalf("no layer, in the controller's words, is an empty layer: %v %v", got, err)
+	}
+}