Move doc comments to crate/module level per review feedback

Move DB model conventions (field ordering, Diesel derives) from
nexus/db-model/src/fm.rs to the nexus-db-model crate-level docs in
lib.rs, with a vmm example showing schema-to-model type mapping.

Move _on_conn/_in_txn naming conventions from the FM datastore module
to nexus/db-queries/src/db/datastore/mod.rs. Fix the framing: connection
sharing is primarily about pool efficiency, not consistency. Keep the
FM-specific correctness detail (concurrent delete detection, #9594)
inline with a pointer to the general docs.

Author: mergeconflict

nexus/db-model/src/fm.rs

@@ -11,15 +11,8 @@
 //!
 //! 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.
+//! documentation in [`nexus_types::fm`] for more information, and the
+//! [crate-level documentation](crate) for general conventions.
 
 use crate::SqlU32;
 use crate::typed_uuid::DbTypedUuid;

nexus/db-model/src/lib.rs

@@ -2,7 +2,78 @@
 // License, v. 2.0. If a copy of the MPL was not distributed with this
 // file, You can obtain one at https://mozilla.org/MPL/2.0/.
 
-//! Structures stored to the database.
+//! Rust types that represent rows in the Omicron database (CockroachDB).
+//!
+//! Each struct in this crate maps to a database table. The table's columns and
+//! SQL types are declared in [`nexus_db_schema`] via Diesel's
+//! [`table!`](diesel::table) macro; the struct here provides the corresponding
+//! Rust types. [`nexus_db_queries`] then uses both to build and execute
+//! queries in its [`DataStore`](nexus_db_queries::db::DataStore) methods.
+//!
+//! # How the mapping works
+//!
+//! Consider the `vmm` table. The schema declares columns and SQL types:
+//!
+//! ```text
+//! // in nexus-db-schema
+//! table! {
+//!     vmm (id) {
+//!         id -> Uuid,
+//!         sled_id -> Uuid,
+//!         state_generation -> Int8,
+//!         state -> VmmStateEnum,
+//!         ...
+//!     }
+//! }
+//! ```
+//!
+//! The model struct provides the Rust representation of a row:
+//!
+//! ```text
+//! // in nexus-db-model
+//! #[derive(Queryable, Insertable, Selectable)]
+//! #[diesel(table_name = vmm)]
+//! pub struct Vmm {
+//!     pub id: Uuid,
+//!     pub sled_id: DbTypedUuid<SledKind>,
+//!     #[diesel(column_name = state_generation)]
+//!     pub generation: Generation,
+//!     pub state: VmmState,
+//!     ...
+//! }
+//! ```
+//!
+//! A few things to note:
+//!
+//! - **Type translation.** Each field's Rust type must implement Diesel's
+//!   [`FromSql`](diesel::deserialize::FromSql) and
+//!   [`ToSql`](diesel::serialize::ToSql) traits for the corresponding column's
+//!   SQL type. Diesel provides these impls for common types (`Uuid`,
+//!   `DateTime<Utc>`, `String`, etc.); this crate defines wrapper types like
+//!   [`DbTypedUuid`](typed_uuid::DbTypedUuid) for domain-specific conversions
+//!   (e.g. distinguishing a sled UUID from any other UUID).
+//! - **Column renaming.** Field names must match column names by default, but
+//!   `#[diesel(column_name = ...)]` allows the Rust name to differ (e.g.
+//!   `generation` for the `state_generation` column).
+//! - **Positional mapping.** [`diesel::Queryable`] maps SQL result columns to
+//!   struct fields by position, not by name. If the field order doesn't match
+//!   the column order in the query, fields will silently receive wrong values.
+//!   Deriving [`diesel::Selectable`] mitigates this by generating an explicit
+//!   column list, so the result columns are always in the order `Queryable`
+//!   expects. **Always derive both.**
+//!
+//! # Field ordering convention
+//!
+//! Fields in `Queryable`/`Insertable` structs should be ordered to match the
+//! column order in `dbinit.sql`. This keeps the Rust types easy to
+//! cross-reference with the schema definition and prevents subtle bugs if a
+//! query ever omits `Selectable`.
+//!
+//! # Representing enums
+//!
+//! For types that map to database enum columns, see the [`impl_enum_type!`]
+//! and [`impl_enum_wrapper!`] macros defined below. For string-backed enums,
+//! see the [`DatabaseString`] trait.
 
 #[macro_use]
 extern crate diesel;

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

@@ -6,18 +6,8 @@
 //! reports (sitreps).
 //!
 //! 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).
+//! fault management sitrep, and the [datastore module documentation](super) for
+//! general conventions.
 
 use super::DataStore;
 use crate::authz;

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

@@ -3,6 +3,19 @@
 // file, You can obtain one at https://mozilla.org/MPL/2.0/.
 
 //! Primary control plane interface for database read and write operations
+//!
+//! # Method naming conventions
+//!
+//! Many methods in this module have a public entry point that acquires a
+//! database connection from the pool, paired with a private `_on_conn` (or
+//! `_on_connection`) helper that takes an explicit connection parameter. The
+//! helper exists so that multiple queries can share the same connection without
+//! re-acquiring one from the pool for each step of a multi-query operation.
+//!
+//! A related convention is `_in_txn` (or `_in_transaction`), for helpers that
+//! run inside an explicit database transaction. These typically take an
+//! [`OptionalError`](crate::transaction_retry::OptionalError) so they can bail
+//! out of the entire transaction on application-level errors.
 
 // TODO-scalability review all queries for use of indexes (may need
 // "time_deleted IS NOT NULL" conditions) Figure out how to automate this.

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

@@ -87,14 +87,14 @@ impl FmRendezvous {
         // `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.
+        // TODO(#9592) 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;