Skip to content

[fm] Introduce SitrepGuardedInsert.#10532

Open
mergeconflict wants to merge 1 commit into
mainfrom
mergeconflict/fm-sitrepguardedinsert
Open

[fm] Introduce SitrepGuardedInsert.#10532
mergeconflict wants to merge 1 commit into
mainfrom
mergeconflict/fm-sitrepguardedinsert

Conversation

@mergeconflict
Copy link
Copy Markdown
Contributor

@mergeconflict mergeconflict commented Jun 2, 2026

Add the SitrepGuardedInsert Diesel combinator and the SitrepGuardedResource trait: a generic primitive for FM rendezvous to insert a resource row idempotently and guarded against stale-sitrep execution.

The combinator wraps a caller-supplied resource INSERT in a single CTE statement that:

  • aborts (StaleSitrep) unless the executor's expected generation still equals the latest sitrep's generation column;
  • short-circuits (AlreadyExists) if a creation marker already exists for the resource id;
  • on a successful insert, atomically writes a creation marker.

The result is surfaced as a SitrepGuardedInsertOutcome of Created / AlreadyExists / StaleSitrep.

Context: #10248. This is used in #10533 and #10535 which are split out in hopes of making the review somewhat less miserable.

@mergeconflict mergeconflict self-assigned this Jun 2, 2026
@mergeconflict mergeconflict added the fault-management Everything related to the fault-management initiative (RFD480 and others) label Jun 2, 2026
Add the `SitrepGuardedInsert` Diesel combinator and the
`SitrepGuardedResource` trait: a generic primitive for FM rendezvous to
insert a resource row idempotently and guarded against stale-sitrep
execution.

The combinator wraps a caller-supplied resource INSERT in a single CTE
statement that:

  - aborts (StaleSitrep) unless the executor's expected generation still
    equals the latest sitrep's generation column;
  - short-circuits (AlreadyExists) if a creation marker already exists for
    the resource id;
  - on a successful insert, atomically writes a creation marker, gated by
    `WHERE EXISTS (SELECT 1 FROM new_resource)` so a marker is never
    fabricated for a pre-existing row.

All spliced SQL identifiers come from the trait's `&'static str` consts, so
the query is injection-safe. The result is surfaced as a
`SitrepGuardedInsertOutcome` of Created / AlreadyExists / StaleSitrep.
@mergeconflict mergeconflict force-pushed the mergeconflict/fm-sitrepguardedinsert branch from 6663e70 to c6e5904 Compare June 2, 2026 19:49
Comment on lines +315 to +326
Err(e) => {
if matches_sentinel(&e, &[STALE_GENERATION_SENTINEL]).is_some()
{
Ok(SitrepGuardedInsertOutcome::StaleSitrep)
} else if matches_sentinel(&e, &[ALREADY_EXISTS_SENTINEL])
.is_some()
{
Ok(SitrepGuardedInsertOutcome::AlreadyExists)
} else {
Err(e)
}
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style nit, feel free to ignore me: personally, I prefer to stuff the if conditions into the match arms so it feels less nested, like:

Suggested change
Err(e) => {
if matches_sentinel(&e, &[STALE_GENERATION_SENTINEL]).is_some()
{
Ok(SitrepGuardedInsertOutcome::StaleSitrep)
} else if matches_sentinel(&e, &[ALREADY_EXISTS_SENTINEL])
.is_some()
{
Ok(SitrepGuardedInsertOutcome::AlreadyExists)
} else {
Err(e)
}
}
Err(e) if matches_sentinel(&e, &[STALE_GENERATION_SENTINEL]).is_some() => {
Ok(SitrepGuardedInsertOutcome::StaleSitrep)
}
Err(e) if matches_sentinel(&e, &[ALREADY_EXISTS_SENTINEL]).is_some() => {
Ok(SitrepGuardedInsertOutcome::AlreadyExists)
},
Err(e) => Err(e),

/// 5. `new_marker`: the marker INSERT, emitted inline by the
/// combinator using R's `&'static str` consts. This only runs when
/// `new_resource` actually produced a row.
fn walk_ast<'b>(&'b self, mut out: AstPass<'_, 'b, Pg>) -> QueryResult<()> {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice to have an expectorate test (and maybe also an EXPLAIN test) for the kind of queries that this generates.

Comment on lines +61 to +66
/// Column on `fm_sitrep` carrying this resource's generation counter.
const GENERATION_COLUMN: &'static str;
/// Creation table tracked alongside the resource.
const MARKER_TABLE: &'static str;
/// Resource-id column in the marker table.
const MARKER_ID_COLUMN: &'static str;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't love that these are all just &'static strs that have to be the name of the table/column, which you could easily imagine someone typoing and silently breaking something. I wonder if we can instead use the generated diesel schema types, so that you could instead write something like

impl SitrepGuardedResource for nexus_db_model::Alert {
    type MarkerTable = schema::rendezvous_alert_created::table;
    type MarkerIdColumn = schema::rendezvous_alert_created::dsl::alert_id;
    type GenerationColumn = schema::fm_sitrep::dsl::alert_generation;
}

This way, you get the actual table name from the schema and can't mistype it --- and we can probably leverage diesel to enforce that MarkerIdColumn comes from the same table as MarkerTable?

I notice that this is what the DatastoreCollectionConfig trait does:

/// Trait to be implemented by any structs representing a collection.
/// For example, since Silos have a one-to-many relationship with
/// Projects, the Silo datatype should implement this trait.
/// ```
/// # use diesel::prelude::*;
/// # use nexus_db_model::DatastoreCollectionConfig;
/// # use nexus_db_model::Generation;
/// #
/// # table! {
/// # test_schema.silo (id) {
/// # id -> Uuid,
/// # time_deleted -> Nullable<Timestamptz>,
/// # rcgen -> Int8,
/// # }
/// # }
/// #
/// # table! {
/// # test_schema.project (id) {
/// # id -> Uuid,
/// # time_deleted -> Nullable<Timestamptz>,
/// # silo_id -> Uuid,
/// # }
/// # }
///
/// #[derive(Queryable, Insertable, Debug, Selectable)]
/// #[diesel(table_name = project)]
/// struct Project {
/// pub id: uuid::Uuid,
/// pub time_deleted: Option<chrono::DateTime<chrono::Utc>>,
/// pub silo_id: uuid::Uuid,
/// }
///
/// #[derive(Queryable, Insertable, Debug, Selectable)]
/// #[diesel(table_name = silo)]
/// struct Silo {
/// pub id: uuid::Uuid,
/// pub time_deleted: Option<chrono::DateTime<chrono::Utc>>,
/// pub rcgen: Generation,
/// }
///
/// impl DatastoreCollectionConfig<Project> for Silo {
/// // Type of Silo::identity::id and Project::silo_id
/// type CollectionId = uuid::Uuid;
///
/// type GenerationNumberColumn = silo::dsl::rcgen;
/// type CollectionTimeDeletedColumn = silo::dsl::time_deleted;
///
/// type CollectionIdColumn = project::dsl::silo_id;
/// }
/// ```
pub trait DatastoreCollectionConfig<ResourceType> {
/// The Rust type of the collection id (typically Uuid for us)
type CollectionId: Copy + Debug;
/// The column in the CollectionTable that acts as a generation number.
/// This is the "child-resource-generation-number" in RFD 192.
type GenerationNumberColumn: Column + Default;
/// The time deleted column in the CollectionTable
// We enforce that this column comes from the same table as
// GenerationNumberColumn when defining insert_resource() below.
type CollectionTimeDeletedColumn: Column + Default;
/// The column in the ResourceTable that acts as a foreign key into
/// the CollectionTable
type CollectionIdColumn: Column;
}

/// turn the "row already exists" outcome into a hard error.
pub fn new(
resource_id: Uuid,
expected_generation: i64,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i feel like this should probably be a Generation?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fault-management Everything related to the fault-management initiative (RFD480 and others)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants