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).

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

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

This is maybe nitpicky, but this comment was initially a little bit confusing to me, because we are kind of using "orphaned" to refer to two different things: both sitreps that are orphaned due to not being in fm_sitrep_history, and sitrep child rows that are orphaned due to the sitrep metadata row being deleted. Also, later on you refer to "creating orphans", but that actually means deleting a metadata row, rather than actually creating new records. Anyway I just wanted to write that down for clarity.

And, addressing the actual idea you proposed here, I agree that paginating the whole transaction might be a good idea to limit the duration of the transaction...

I can try to update the comment here to make the orphan terminology more clear. I was kinda using "orphaned" to refer to the top-level sitrep, and "deeply orphaned" to refer to child tables of sitreps which no longer exist.

Adding a TODO for the pagination because it will have subtleties that maybe should be reviewed independently of this PR.

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

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

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?


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

hawkw

This is great, thank you for adding the macro and stuff! I had a bunch of small notes on the comments etc, but it looks great overall!

hawkw · 2026-03-31T16:14:59Z

dev-tools/omdb/src/bin/omdb/nexus.rs

+    // Ensure columns stay aligned even if a child table name is long.
+    let width = child_tables
+        .keys()
+        .map(|name| "orphaned  rows deleted:".len() + name.len() + 1)


turbo nit: maybe worth a comment explaining why there's two spaces in orphaned rows deleted? i very briefly thought it was a typo before i realized what this was doing...

hawkw · 2026-03-31T16:18:16Z

dev-tools/omdb/src/bin/omdb/nexus.rs

+            stats.rows_deleted,
+        );
+        println!("    {:<width$}{:>NUM_WIDTH$}", "  batches:", stats.batches,);
+    }


not important, but it occurs to me that maybe if child_tables is empty, we should complain in some way? that would indicate a bug...

Added a warning

hawkw · 2026-03-31T16:19:00Z

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

+/// metadata. To add a new child table to the sitrep GC, just add a line
+/// here — the GC loop, omdb display, and completeness test all adapt
+/// automatically.


nitpick: technically, you don't add a line here, you do that in the place where we invoke the macro...

also, maybe it's worth briefly describing the syntax so that the person adding a new table is aware of the default column name for the sitrep ID and how it can be overridden (though in practice I think i would complain in a review if someone added a new sitrep child table where it was named anything other than that...)

updated the docs, added an example of the syntax

hawkw · 2026-03-31T16:22:24Z

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;


This is maybe nitpicky, but this comment was initially a little bit confusing to me, because we are kind of using "orphaned" to refer to two different things: both sitreps that are orphaned due to not being in fm_sitrep_history, and sitrep child rows that are orphaned due to the sitrep metadata row being deleted. Also, later on you refer to "creating orphans", but that actually means deleting a metadata row, rather than actually creating new records. Anyway I just wanted to write that down for clarity.

hawkw · 2026-03-31T16:24:03Z

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;


And, addressing the actual idea you proposed here, I agree that paginating the whole transaction might be a good idea to limit the duration of the transaction...

hawkw · 2026-03-31T16:25:31Z

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

+
+    /// Builds a DELETE query that removes orphaned `fm_sitrep` metadata
+    /// rows in a single paginated batch (sitreps not in history whose
+    /// parent is not current). Returns (rows_deleted, next_marker).


turbo nit: perhaps this should have a line saying which test output file contains the text of the query, like you added in #10061?

hawkw · 2026-03-31T16:26:57Z

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

+    /// 1. Finds a batch of distinct `sitrep_id` values in the child table
+    /// 2. Deletes rows whose `sitrep_id` has no corresponding `fm_sitrep`
+    /// 3. Returns (rows_deleted, next_marker) where next_marker is NULL
+    ///    when there are no more pages


nit: similarly, perhaps a "the SQL generated by this query is in ..." comment?

hawkw · 2026-03-31T16:30:40Z

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

+            Ok(result) => {
+                status.orphaned_sitreps_deleted = result.sitreps_deleted;
+                status.sitrep_metadata_batches = result.sitrep_metadata_batches;
+                status.batch_size = result.batch_size;
+                status.child_tables = result
+                    .child_tables


this is a nitpick, but I'd kind of like it if we exhaustively destructured result here, like:

Suggested change

Ok(result) => {

status.orphaned_sitreps_deleted = result.sitreps_deleted;

status.sitrep_metadata_batches = result.sitrep_metadata_batches;

status.batch_size = result.batch_size;

status.child_tables = result

.child_tables

Ok(GcOrphansResult { sitreps_deleted, sitrep_metadata_batches, batch_size, child_tables }) => {

status.orphaned_sitreps_deleted = sitreps_deleted;

status.sitrep_metadata_batches = sitrep_metadata_batches;

status.batch_size = batch_size;

status.child_tables = child_tables

so that we get a compiler error if someone adds additional fields to GcOrphansResult that we're not handling here?

Good catch, done

hawkw · 2026-03-31T16:35:47Z

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

+    // ---------------------------------------------------------------
+    // SitrepChildTableCounts: mirrors BlueprintTableCounts from
+    // deployment.rs to ensure every fm_* child table is covered by
+    // `SitrepChildTable`.
+    // ---------------------------------------------------------------


nit, take it or leave it: i would like it if this comment described what this thing actually does, in addition to saying "it's like that thing in this other file, and it's used for this purpose" --- it counts the number of rows in each sitrep child table, and the list of sitrep child tables is determined using the SitrepChildTable enum above.

Updated the comment

hawkw · 2026-03-31T16:37:57Z

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

+                    "found fm_* child table(s) not covered by \
+                     SitrepChildTable: {}\n\n\
+                     If you added a new fm_* child table, add a variant \
+                     to SitrepChildTable and update the orphan GC code \
+                     in fm_sitrep_gc_orphans.",


turbo nitpick: i might add

Suggested change

"found fm_* child table(s) not covered by \

SitrepChildTable: {}\n\n\

If you added a new fm_* child table, add a variant \

to SitrepChildTable and update the orphan GC code \

in fm_sitrep_gc_orphans.",

"found fm_* child table(s) not covered by \

`SitrepChildTable`: {}\n\n\

If you added a new fm_* child table, add a variant \

to `SitrepChildTable` and update the orphan GC code \

in `fm_sitrep_gc_orphans`.",

also, maybe we should say something about what to do if there's a false positive (i.e. if you add a new fm_foo table that should not be covered by orphan GC) --- something like "consider either dropping the fm_ prefix or adding your table name to tables_ignored"?

Added back-ticks + advice

smklein

Addressed everything but the pagination - agree it's worth doing, but given that it miiiight increase some complexity I'm punting a bit (leaving as a TODO)

smklein · 2026-04-01T22:03:49Z

dev-tools/omdb/src/bin/omdb/nexus.rs

+    // Ensure columns stay aligned even if a child table name is long.
+    let width = child_tables
+        .keys()
+        .map(|name| "orphaned  rows deleted:".len() + name.len() + 1)


smklein · 2026-04-01T22:06:44Z

dev-tools/omdb/src/bin/omdb/nexus.rs

+            stats.rows_deleted,
+        );
+        println!("    {:<width$}{:>NUM_WIDTH$}", "  batches:", stats.batches,);
+    }


Added a warning

smklein · 2026-04-01T22:20:13Z

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

+/// metadata. To add a new child table to the sitrep GC, just add a line
+/// here — the GC loop, omdb display, and completeness test all adapt
+/// automatically.


updated the docs, added an example of the syntax

smklein · 2026-04-01T22:20:23Z

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?


smklein · 2026-04-01T23:05:31Z

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;


I can try to update the comment here to make the orphan terminology more clear. I was kinda using "orphaned" to refer to the top-level sitrep, and "deeply orphaned" to refer to child tables of sitreps which no longer exist.

Adding a TODO for the pagination because it will have subtleties that maybe should be reviewed independently of this PR.

smklein · 2026-04-01T23:06:50Z

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

+
+    /// Builds a DELETE query that removes orphaned `fm_sitrep` metadata
+    /// rows in a single paginated batch (sitreps not in history whose
+    /// parent is not current). Returns (rows_deleted, next_marker).


smklein · 2026-04-01T23:07:01Z

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

+    /// 1. Finds a batch of distinct `sitrep_id` values in the child table
+    /// 2. Deletes rows whose `sitrep_id` has no corresponding `fm_sitrep`
+    /// 3. Returns (rows_deleted, next_marker) where next_marker is NULL
+    ///    when there are no more pages


smklein · 2026-04-01T23:08:51Z

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

+    // ---------------------------------------------------------------
+    // SitrepChildTableCounts: mirrors BlueprintTableCounts from
+    // deployment.rs to ensure every fm_* child table is covered by
+    // `SitrepChildTable`.
+    // ---------------------------------------------------------------


Updated the comment

smklein · 2026-04-01T23:10:03Z

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

+                    "found fm_* child table(s) not covered by \
+                     SitrepChildTable: {}\n\n\
+                     If you added a new fm_* child table, add a variant \
+                     to SitrepChildTable and update the orphan GC code \
+                     in fm_sitrep_gc_orphans.",


Added back-ticks + advice

smklein · 2026-04-01T23:11:01Z

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

+            Ok(result) => {
+                status.orphaned_sitreps_deleted = result.sitreps_deleted;
+                status.sitrep_metadata_batches = result.sitrep_metadata_batches;
+                status.batch_size = result.batch_size;
+                status.child_tables = result
+                    .child_tables


Good catch, done

hawkw

looking great, thank you!!

hawkw · 2026-04-02T16:02:34Z

yay!

Follow-up to #10143 Adds pagination during sitrep garbage collection. While I was there, I realized that we actually don't need transactions on the delete pathway anymore. - The delete operation (within the garbage collection pass) deletes sitreps by deleting out-of-history `fm_sitrep` rows, which immediately orphans all other sub-tables within the sitrep. - All the "concurrent reads" of sitreps go through `fm_sitrep_read_on_conn`, which reads metadata from `fm_sitrep` **last**. If this sitrep can be read: it has not been deleted yet. If this sitrep cannot be read: it has been deleted, and prior reads can be discarded. - All the "concurrent inserts" of sitreps are contingent on the parent sitrep not being stale. So: because sitrep insert writes to the `fm_sitrep` table first, the child rows are "not orphaned" (won't be GC-ed). This protection lasts for the duration of `fm_sitrep_insert`, OR until the parent sitrep is marked stale - at which point insert should fail anyway. All this is to say: the "read" and "insert" pathways function fine if a `fm_sitrep` row is deleted non-atomically before subsequent child rows. Therefore: no transaction is necessary here.

Avoid leaking sitrep rows during deletion

2e57643

smklein commented Mar 24, 2026

View reviewed changes

nexus/db-queries/src/db/datastore/fm.rs Show resolved Hide resolved

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

nexus/db-queries/src/db/datastore/fm.rs Outdated Show resolved Hide resolved

nexus/db-queries/src/db/datastore/fm.rs Outdated Show resolved Hide resolved

hawkw added this to the 20 milestone Mar 26, 2026

smklein added 3 commits March 30, 2026 11:21

merge

e91f27e

strum and macros

5ffc33f

default sitrep_id

6e81e7a

hawkw approved these changes Mar 31, 2026

View reviewed changes

hawkw mentioned this pull request Mar 31, 2026

fm: persist support bundle requests and data selections #10200

Merged

feedback

a8208b2

smklein commented Apr 1, 2026

View reviewed changes

hawkw approved these changes Apr 2, 2026

View reviewed changes

smklein merged commit cfd87ec into main Apr 2, 2026
16 checks passed

smklein deleted the sitrep-orphan-killing-machine branch April 2, 2026 15:59

smklein mentioned this pull request Apr 2, 2026

Delete sitreps non-transactionally with improved pagination #10210

Merged

Conversation

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

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

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!

smklein 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

smklein commented Mar 24, 2026 •

edited

Loading