From c9a711c259edaf3e00775deb2741ba89449f55b1 Mon Sep 17 00:00:00 2001 From: jochen Date: Tue, 13 Oct 2026 02:12:06 +0200 Subject: [PATCH] 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 | 11 ++++- .../cmd/betterbird-tools/accounts.go | 15 +++---- .../cmd/betterbird-tools/accounts_test.go | 24 ++++++++++ .../betterbird/cmd/betterbird-tools/derive.go | 19 +++++--- .../cmd/betterbird-tools/derive_test.go | 44 ++++++++++++++++++- 5 files changed, 94 insertions(+), 19 deletions(-) diff --git a/modules/betterbird/README.md b/modules/betterbird/README.md index 96287a22..4e2a2acd 100644 --- a/modules/betterbird/README.md +++ b/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 diff --git a/modules/betterbird/cmd/betterbird-tools/accounts.go b/modules/betterbird/cmd/betterbird-tools/accounts.go index cb718d1e..227af178 100644 --- a/modules/betterbird/cmd/betterbird-tools/accounts.go +++ b/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 diff --git a/modules/betterbird/cmd/betterbird-tools/accounts_test.go b/modules/betterbird/cmd/betterbird-tools/accounts_test.go index 37a0ccc2..0e96ab44 100644 --- a/modules/betterbird/cmd/betterbird-tools/accounts_test.go +++ b/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) + } +} diff --git a/modules/betterbird/cmd/betterbird-tools/derive.go b/modules/betterbird/cmd/betterbird-tools/derive.go index 9efc25d3..6d9b0484 100644 --- a/modules/betterbird/cmd/betterbird-tools/derive.go +++ b/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 } diff --git a/modules/betterbird/cmd/betterbird-tools/derive_test.go b/modules/betterbird/cmd/betterbird-tools/derive_test.go index 6d2cc55d..549e98d9 100644 --- a/modules/betterbird/cmd/betterbird-tools/derive_test.go +++ b/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) + } +}