Avoid leaking sitrep rows during deletion by smklein · Pull Request #10143 · oxidecomputer/omicron

smklein · 2026-03-24T19:56:40Z

In an attempt to avoid using transactions during the INSERT pathway (see: #10133)
this PR instead tries to alter how DELETE works for sitreps.

Rather than using a two-step GC process (listing orphaned sitreps and deleting
them - even while concurrent INSERT operations could be underway) this PR
also enables the deletion of "rows within a sitrep, for which the sitrep metadata
record no longer exists".

This allows us to delete rows that were previously leaked, and is also now the
primary mechanism by which rows are removed: we delete the high-level
sitrep first, then remove all these newly-orphaned rows. This is still performed
within a transaction on the DELETE pathway to avoid reading torn sitreps
during the loading phase (as originally appeared in #9594).

smklein · 2026-03-24T19:58:02Z

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

+    /// parent is not current). Returns (rows_deleted, next_marker).
+    fn delete_orphaned_sitrep_metadata_query(
+        marker: Uuid,
+        batch_size: std::num::NonZeroU32,


If we don't paginate, this incurs a full table scan. So: We have to paginate.

nit, take it or leave it: it might be kinda nice if the background task's status thingy was able to indicate how many of these it had to do (and also the batch size?)

Done (plumbed batch info through bg task, out to omdb) in 076920b

smklein · 2026-03-24T20:25:43Z

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

+            .transaction(&conn, |conn| {
+                async move {
+                    // Step 1: Delete orphaned fm_sitrep metadata rows.
+                    let mut sitreps_deleted = 0usize;


So, the structure here is basically:

Delete sitrep metadata rows (make orphaned rows)

Clean up orphaned rows (which maybe we just made, or maybe got created by an errant INSERT operation that bailed out)

We could make this transaction smaller by "limiting how many sitrep metadata rows we create at once".

For example, we could:

"Only create up to 64 orphans in a txn"

... then clean up all their data, within this transaction (to avoid torn reads)

... then, COMMIT the transaction, and do another batch, until we stop cleaning stuff up / hit some arbitrary limit / etc

hawkw · 2026-03-24T23:40:52Z

In an attempt to avoid using transactions during the INSERT pathway (see: #10133) this PR instead tries to alter how DELETE works for sitreps.

Thank you for your support in this matter :D

hawkw

this code is kinda wild, i'm impressed --- thank you for humoring me

hawkw · 2026-03-24T23:44:20Z

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

+    pub const fn table_name(&self) -> &'static str {
+        match self {
+            Self::CaseEreport => "fm_ereport_in_case",
+            Self::AlertRequest => "fm_alert_request",
+            Self::Case => "fm_case",
+        }
+    }
+
+    const fn sitrep_id_column(&self) -> &'static str {
+        match self {
+            Self::CaseEreport => "sitrep_id",
+            Self::AlertRequest => "sitrep_id",
+            Self::Case => "sitrep_id",
+        }
+    }


...this is how you know the queries you're about to read are gonna be Really Cool, huh?

So the one really big concern here is that if you want to have your new sitrep child table --- such as (for example) the alert requests that @mergeconflict is working on --- you gotta remember to add it here or else your new thing won't be GCed. Which, once I actually think about it at all, I realize was already the case, since you would have to write queries for deleting your new thing, so this doesn't actually make the status quo any worse, and in fact, kinda makes it easier, since all you have to do is add your new table here.

With that said, I wonder if we ought to have some kinda test that asks CRDB for all the table/column names and does a big panic!("HEY DID YOU FORGET TO ADD YOUR NEW TABLE TO THE SITREP GC CODE?") if it encounters a table with a name like fm_* with a sitrep_id column, or something...?

We have almost an identical problem in reconfigurator-land for blueprints. And there is a test to catch this! I'll see if I can pull it over and make it work for sitreps too -- we did prefix matching there.

Added a test to catch these cases in a88fd11

hawkw · 2026-03-24T23:51:04Z

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

+                    // Step 2: For each child table, paginate through and
+                    // delete rows whose sitrep_id no longer exists in
+                    // fm_sitrep.
+                    let mut cases_deleted = 0usize;
+                    let mut case_ereports_deleted = 0usize;
+                    let mut alert_requests_deleted = 0usize;
+
+                    for (table, counter) in [
+                        (
+                            SitrepChildTable::CaseEreport,
+                            &mut case_ereports_deleted,
+                        ),
+                        (
+                            SitrepChildTable::AlertRequest,
+                            &mut alert_requests_deleted,
+                        ),
+                        (SitrepChildTable::Case, &mut cases_deleted),
+                    ] {


it occurs to me that, if this gets somewhat unwieldy as we add more tables, this could maybe just be a map of (SitrepChildTable, usize). Although I do kinda not love allocating a whole hashmap for that... 🤷‍♀️

hawkw · 2026-03-24T23:55:01Z

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

+                            let result =
+                                Self::delete_orphaned_sitrep_metadata_query(
+                                    marker,
+                                    SQL_BATCH_SIZE,
+                                )
+                                .get_result_async::<(i64, Option<Uuid>)>(&conn)
+                                .await?;
+
+                            let (deleted, next_marker) = result;


nit: mayhaps

Suggested change

let result =

Self::delete_orphaned_sitrep_metadata_query(

marker,

SQL_BATCH_SIZE,

)

.get_result_async::<(i64, Option<Uuid>)>(&conn)

.await?;

let (deleted, next_marker) = result;

let (deleted, next_marker) =

Self::delete_orphaned_sitrep_metadata_query(

marker,

SQL_BATCH_SIZE,

)

.get_result_async::<(i64, Option<Uuid>)>(&conn)

.await?;

Fixed in 8362260

hawkw · 2026-03-24T23:56:07Z

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

@@ -998,6 +913,244 @@ impl DataStore {
        Ok(sitreps_deleted)
    }


do we still like, need this?

there are some tests which still use it, and transitioning them over is still a bit hairy - I can mark it as cfg(test) though and make it non-pub?

Reduced scope in d183381

hawkw · 2026-03-24T23:57:51Z

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

+    /// parent is not current). Returns (rows_deleted, next_marker).
+    fn delete_orphaned_sitrep_metadata_query(
+        marker: Uuid,
+        batch_size: std::num::NonZeroU32,


nit, take it or leave it: it might be kinda nice if the background task's status thingy was able to indicate how many of these it had to do (and also the batch size?)

hawkw · 2026-03-24T23:58:05Z

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

+    ) -> Result<GcOrphansResult, Error> {
+        let conn = self.pool_connection_authorized(opctx).await?;
+
+        // TODO(eliza): there should probably be an authz object for the fm sitrep?


hawkw · 2026-03-25T23:49:35Z

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

+    /// All child tables. Used by the GC loop and by tests to ensure
+    /// completeness. If you add a new variant, add it here — the GC
+    /// loop will fail to compile until you handle it.
+    pub const ALL: &[SitrepChildTable] =
+        &[Self::CaseEreport, Self::AlertRequest, Self::Case];


can this be accomplished using #[derive(strum::VariantArray)] or whatever? or is it specifically intended to be the thing that catches variants you forgot to add?

hawkw · 2026-03-25T23:51:06Z

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

+/// Identifies a child table of `fm_sitrep` for use in orphan-deletion
+/// queries. Using an enum (rather than raw strings) ensures the table and
+/// column names are known at compile time, preventing SQL injection.
+#[derive(Clone, Copy, Debug, PartialEq, Eq, PartialOrd, Ord)]
+pub enum SitrepChildTable {
+    CaseEreport,
+    AlertRequest,
+    Case,
+}
+
+impl SitrepChildTable {
+    /// All child tables. Used by the GC loop and by tests to ensure
+    /// completeness. If you add a new variant, add it here — the GC
+    /// loop will fail to compile until you handle it.
+    pub const ALL: &[SitrepChildTable] =
+        &[Self::CaseEreport, Self::AlertRequest, Self::Case];
+}
+
+impl SitrepChildTable {
+    pub const fn table_name(&self) -> &'static str {
+        match self {
+            Self::CaseEreport => "fm_ereport_in_case",
+            Self::AlertRequest => "fm_alert_request",
+            Self::Case => "fm_case",
+        }
+    }
+
+    pub(crate) const fn sitrep_id_column(&self) -> &'static str {
+        match self {
+            Self::CaseEreport => "sitrep_id",
+            Self::AlertRequest => "sitrep_id",
+            Self::Case => "sitrep_id",
+        }
+    }


don't feel like you have to figure this out now, but i kinda wonder how easy it would be to make the developer experience of adding a new table to the GC targets be just adding one line like

// ... MyCoolTable, "fm_my_cool_table", "sitrep_id", // ...

to something? i bet we could rig it up so that there was a macro that basically just generated everything else, since it's so repetitive...

Avoid leaking sitrep rows during deletion

2e57643

smklein commented Mar 24, 2026

View reviewed changes

smklein added 4 commits March 24, 2026 13:04

stronger UUID types

1d0c779

cope with a batch size of 1

134911d

if you've been scanning for full table scans, you can keep scanning

86a56f0

fmt

a2d5bae

smklein commented Mar 24, 2026

View reviewed changes

hawkw reviewed Mar 24, 2026

View reviewed changes

hawkw added the fault-management Everything related to the fault-management initiative (RFD480 and others) label Mar 25, 2026

smklein added 4 commits March 25, 2026 15:39

Background task talks about batches now

076920b

Test to check for partially deleted sitreps

a88fd11

results

8362260

cfg test

d183381

smklein marked this pull request as ready for review March 25, 2026 23:38

cleanup warnings

0cc8fa3

hawkw reviewed Mar 25, 2026

View reviewed changes

hawkw added this to the 20 milestone Mar 26, 2026

Conversation

smklein commented Mar 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hawkw commented Mar 24, 2026

Uh oh!

hawkw left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

smklein commented Mar 24, 2026 •

edited

Loading