Keep a pairing's unconfirmed long-term PSK out of the record saves behind it

docs/conventions.md

@@ -47,7 +47,7 @@ The design standards that code in this repository is held to. This document is n
 
 - **Held in wiping types, wiped after use.** A private key or PSK lives in a type that wipes it when destroyed (`Identity`, `HandshakePsk`, `ClientRecord`, the ephemeral key a `NoiseInitiator` keeps), and a buffer that held one (a blob loaded or encoded for the provider) is wiped with `secure_zero()` once used. A secret is never formatted into a string.
 - **Pairing secrets the same way.** A pairing code or token from the host, the CPace run and the values it unwraps are held in wiping types (`PairingAttempt`, `PairingPskToken`, the code's buffer on its way to core), and the buffers a pairing message was read from are wiped once it is parsed, whether or not it passed validation, spare capacity included, as are the decrypted message's buffer and the reassembly buffer when a message fails to decrypt, a fragment sequence fails, the session ends or either is destroyed; either grows through `secure_reserve()`, which wipes the block it leaves. The message trace never sees one: `redact_pairing_secrets()` replaces each secret field a client sends before the plaintext is traced.
-- **Persisted before use.** A credential the server will reference in later handshakes (a long-term PSK) is referenced only once the provider reports it durable; until then the record as it was applies (`ServerCore::store_long_term_psk()`).
+- **Persisted before use.** A credential the server will reference in later handshakes (a long-term PSK) is referenced only once the provider reports it durable; until then the record as it was applies (`ServerCore::store_long_term_psk()`). Only the credential's own save writes it: a save of the record made meanwhile writes the record without it, and the credential is durable only once a save of it lands with none made meanwhile.
 - **Removed at once, written until durable.** A removed long-term PSK or approval (an unpairing, a revocation) stops applying at once, and a record write no caller waits on that the provider fails is written again until it lands or the server stops (`ServerCore::FailedWrites`), so a restart does not restore it once the provider recovers before the stop.
 
 ## Logging

src/server_core.cpp

@@ -909,8 +909,25 @@ std::string ServerCore::record_owner(const std::string& client_id) const {
     return "a client not connected";
 }
 
-void ServerCore::save_record(const std::string& client_id, std::function<void(bool)> done) {
+void ServerCore::save_record(const std::string& client_id) {
     const ClientRecord* record = this->records_.find(client_id);
+    const auto unsaved = this->unsaved_records_.find(client_id);
+    if (record == nullptr || unsaved == this->unsaved_records_.end()) {
+        this->save_record(client_id, record, nullptr);
+        return;
+    }
+    // pairing.md "server/pair-finalize": only its own save persists a long-term PSK, so a save
+    // behind it writes the record as put_back() would leave it, and a change whose save lands is
+    // saved again
+    unsaved->second.saved_meanwhile = true;
+    // A record the change created and put_back() removes is not written
+    if (const std::optional<ClientRecord> kept = without_change(record, unsaved->second)) {
+        this->save_record(client_id, &*kept, nullptr);
+    }
+}
+
+void ServerCore::save_record(const std::string& client_id, const ClientRecord* record,
+                             std::function<void(bool)> done) {
     const std::optional<size_t> slot = this->records_.slot_of(client_id);
     auto blob = std::make_shared<RecordBlob>();
     const bool encoded = record != nullptr && encode_client_record(*record, blob->bytes);
@@ -1083,8 +1100,19 @@ void ServerCore::store_long_term_psk(const std::string& client_id, const Psk& ps
     this->unsaved_records_.emplace(client_id,
                                    UnsavedRecord{std::move(before), existing != nullptr, false});
     this->records_.set(record);
-    this->save_record(client_id, [this, id = client_id, done = std::move(done)](bool saved) {
+    this->save_change(client_id, std::move(done));
+}
+
+void ServerCore::save_change(const std::string& client_id,
+                             std::function<void(bool, const UnsavedRecord&)> done) {
+    auto on_saved = [this, id = client_id, done = std::move(done)](bool saved) mutable {
         const auto found = this->unsaved_records_.find(id);
+        // A save of the record was made meanwhile, so the record in memory is saved again
+        if (saved && found->second.saved_meanwhile) {
+            found->second.saved_meanwhile = false;
+            this->save_change(id, std::move(done));
+            return;
+        }
         const UnsavedRecord unsaved = std::move(found->second);
         this->unsaved_records_.erase(found);
         if (saved) {
@@ -1109,24 +1137,31 @@ void ServerCore::store_long_term_psk(const std::string& client_id, const Psk& ps
         for (Client* client : connected) {
             this->reactivate(*client);
         }
-    });
+    };
+    this->save_record(client_id, this->records_.find(client_id), std::move(on_saved));
 }
 
-void ServerCore::put_back(const std::string& client_id, const UnsavedRecord& unsaved) {
-    const ClientRecord* current = this->records_.find(client_id);
+std::optional<ClientRecord> ServerCore::without_change(const ClientRecord* current,
+                                                       const UnsavedRecord& unsaved) {
     if (!unsaved.existed && !unsaved.approval_set) {
-        if (const std::optional<size_t> slot = this->records_.remove(client_id)) {
-            this->erase_record(client_id, *slot);
-        }
-    } else {
-        // A name saved meanwhile stays
-        ClientRecord restored = current != nullptr ? *current : unsaved.before;
-        restored.long_term_psk = unsaved.before.long_term_psk;
-        if (!unsaved.approval_set) {
-            restored.approval = unsaved.before.approval;
-        }
-        this->records_.set(restored);
+        return std::nullopt;
+    }
+    // A name saved meanwhile stays
+    ClientRecord restored = current != nullptr ? *current : unsaved.before;
+    restored.long_term_psk = unsaved.before.long_term_psk;
+    if (!unsaved.approval_set) {
+        restored.approval = unsaved.before.approval;
+    }
+    return restored;
+}
+
+void ServerCore::put_back(const std::string& client_id, const UnsavedRecord& unsaved) {
+    if (const std::optional<ClientRecord> restored =
+            without_change(this->records_.find(client_id), unsaved)) {
+        this->records_.set(*restored);
         this->save_record(client_id);
+    } else if (const std::optional<size_t> slot = this->records_.remove(client_id)) {
+        this->erase_record(client_id, *slot);
     }
     const bool paired = this->long_term_psk(client_id) != nullptr;
     this->for_each_connection(client_id, [this, &client_id, paired](Client& client) {

src/server_core.h

@@ -227,17 +227,22 @@ class ServerCore {
         bool existed{false};
         /// set_approval() ran meanwhile, so putting the change back keeps the approval it set
         bool approval_set{false};
+        /// A save of the record was made meanwhile, so the record in memory is saved again before
+        /// the change is durable
+        bool saved_meanwhile{false};
     };
 
     /// @brief A completed pairing's record (pairing.md "Pairing Records"): sets client_id's
     /// long-term PSK and discards its approval (pairing.md "Unpaired Access"), then saves the
     /// record and calls done on the core thread with whether it is durable and the change's
     /// UnsavedRecord. Until then handshakes reference the PSK of the record as it was, and a
     /// failed save puts the change back (put_back()), so nothing uses the new PSK before it is
-    /// persisted (pairing.md "server/pair-finalize"). After done, every connection of the client
-    /// is activated again under its record. A second store for a client while one is in flight
-    /// gets done(false) at once. done is dropped once the core stops, and shutdown() puts the
-    /// change back on disk.
+    /// persisted (pairing.md "server/pair-finalize"). A save of the record made meanwhile leaves
+    /// the change out (save_record()), so the change is durable only once a save of it lands with
+    /// none made meanwhile (save_change()). After done, every connection of the client is
+    /// activated again under its record. A second store for a client while one is in flight gets
+    /// done(false) at once. done is dropped once the core stops, and shutdown() puts the change
+    /// back on disk.
     void store_long_term_psk(const std::string& client_id, const Psk& psk,
                              std::function<void(bool, const UnsavedRecord&)> done);
 
@@ -605,13 +610,29 @@ class ServerCore {
     /// @brief Re-handshakes every activated connection of client_id to psk (connection.md
     /// "Re-handshake")
     void rekey(const std::string& client_id, const HandshakePsk& psk);
-    /// @brief Saves client_id's record through the provider on the dispatch thread, reporting a
-    /// failure with on_error(); then, unless the core stopped, calls done on the core thread with
-    /// whether it is durable. A failed save without done is written again (FailedWrites). Without
-    /// a provider nothing is saved and done gets true. The save may finish after the core is
-    /// destroyed: it reaches the core only through loop_, which outlives the dispatch thread, and
-    /// a handler posted after the loop ended never runs.
-    void save_record(const std::string& client_id, std::function<void(bool)> done = nullptr);
+    /// @brief Saves client_id's record in memory, as the other overload describes, without done.
+    /// While a store_long_term_psk() change is being saved, the record saved is without_change()'s,
+    /// and none when that is nullopt.
+    void save_record(const std::string& client_id);
+    /// @brief Saves record (nullptr: none, which fails) as client_id's through the provider on the
+    /// dispatch thread, reporting a failure with on_error(); then, unless the core stopped, calls
+    /// done on the core thread with whether it is durable. A failed save without done is written
+    /// again (FailedWrites). Without a provider nothing is saved and done gets true. The save may
+    /// finish after the core is destroyed: it reaches the core only through loop_, which outlives
+    /// the dispatch thread, and a handler posted after the loop ended never runs.
+    void save_record(const std::string& client_id, const ClientRecord* record,
+                     std::function<void(bool)> done);
+    /// @brief current, client_id's record in memory, as put_back() leaves it: the long-term PSK
+    /// and, unless set_approval() ran meanwhile, the approval of unsaved's record before, with
+    /// current's other fields; nullopt when the change created the record and no approval was set
+    static std::optional<ClientRecord> without_change(const ClientRecord* current,
+                                                      const UnsavedRecord& unsaved);
+
+    /// @brief Saves the record in memory, which holds client_id's store_long_term_psk() change,
+    /// again when a save made meanwhile left the change out, then ends the change as
+    /// store_long_term_psk() describes
+    void save_change(const std::string& client_id,
+                     std::function<void(bool, const UnsavedRecord&)> done);
     /// @brief Saves an all-zero blob, a free slot, in slot, once client_id's record is removed
     void erase_record(const std::string& client_id, size_t slot);
     /// @brief A record's blob, wiped when the last holder releases it