Add review context comments

Author: mergeconflict

nexus/db-model/src/fm.rs

@@ -12,6 +12,14 @@
 //! These types are used when inserting and reading sitreps in CRDB; when in
 //! use, the sitrep is represented as a [`nexus_types::fm::Sitrep`]. See the
 //! documentation in [`nexus_types::fm`] for more information.
+//!
+//! # Conventions
+//!
+//! Fields in `Queryable`/`Insertable` structs should be ordered to match the
+//! column order in `dbinit.sql`. Diesel maps `Queryable` fields positionally,
+//! so matching the schema column order prevents subtle bugs if a query ever
+//! omits `Selectable`, and keeps the Rust types easy to cross-reference with
+//! the schema definition.
 
 use crate::SqlU32;
 use crate::typed_uuid::DbTypedUuid;

nexus/db-queries/src/db/datastore/fm.rs

@@ -7,6 +7,17 @@
 //!
 //! See [RFD 603](https://rfd.shared.oxide.computer/rfd/0603) for details on the
 //! fault management sitrep.
+//!
+//! # Conventions
+//!
+//! Methods with an `_on_conn` suffix take an explicit database connection
+//! parameter. In other parts of the codebase this suffix sometimes implies the
+//! method is called within a transaction, but that isn't always the case here.
+//! In the FM datastore, `_on_conn` methods exist primarily so that multiple
+//! queries can share the same connection for consistency (e.g. loading a sitrep
+//! reads child records first and metadata last on the same connection, to
+//! detect concurrent deletes without requiring a transaction, see
+//! [`DataStore::fm_sitrep_read_on_conn`] and issue #9594).
 
 use super::DataStore;
 use crate::authz;
@@ -503,6 +514,13 @@ impl DataStore {
         // rather than doing smaller ones for each case in the sitrep. This uses
         // more memory in Nexus but reduces the number of small db queries we
         // perform.
+        //
+        // The ordering of inserts among case child records (ereports, alert
+        // requests) and case metadata doesn't matter: there are no foreign key
+        // constraints between these tables, and garbage collection is keyed on
+        // sitrep_id (which is inserted first above). If we crash partway
+        // through, orphaned child records will be cleaned up when the orphaned
+        // sitrep is garbage collected.
         let mut cases = Vec::with_capacity(sitrep.cases.len());
         let mut alerts_requested = Vec::new();
         let mut case_ereports = Vec::new();

nexus/src/app/background/tasks/fm_rendezvous.rs

@@ -82,7 +82,19 @@ impl FmRendezvous {
 
         // XXX(eliza): is it better to allocate all of these into a big array
         // and do a single `INSERT INTO` query, or iterate over them one by one
-        // (not allocating) but insert one at a time?
+        // (not allocating) but insert one at a time? Note that a batched insert
+        // would need to use `ON CONFLICT DO NOTHING` rather than checking for
+        // `Conflict` errors from individual inserts, since multiple Nexus
+        // instances may run this task concurrently.
+        //
+        // Currently, these `alert_create` calls have no guard against a stale
+        // Nexus inserting alerts from an outdated sitrep. This is fine for now
+        // because alert requests are carried forward into newer sitreps, so a
+        // stale insert is redundant rather than incorrect. However, if alerts
+        // are ever hard-deleted (e.g. when a case is closed), a lagging Nexus
+        // could re-create "zombie" alert records after deletion. At that point,
+        // the INSERT should be guarded by a CTE that checks the sitrep
+        // generation matches the current one.
         for (case_id, req) in sitrep.alerts_requested() {
             let &AlertRequest { id, class, requested_sitrep_id, .. } = req;
             status.total_alerts_requested += 1;
@@ -97,7 +109,9 @@ impl FmRendezvous {
                 )
                 .await
             {
-                // Alert already exists, that's fine.
+                // Alert already exists --- this is expected, since multiple
+                // Nexus instances may run this task concurrently for the same
+                // sitrep, or a previous activation may have partially completed.
                 Err(Error::Conflict { .. }) => {}
                 Err(e) => {
                     slog::warn!(