preemptive eliza fixes based on #10042

Author: david-crespo

.claude/skills/add-background-task/SKILL.md

@@ -25,7 +25,7 @@ Create the task module. The struct holds whatever state it needs (typically `Arc
 
 Logging conventions: `debug` when there's nothing to do, `info` when routine work was done, `warn` when the work done indicates something is wrong (e.g., cleaning up after a crash), `error` on failure.
 
-Include unit tests in the same file using `TestDatabase::new_with_datastore`. Tests call `actually_activate` directly.
+Include a unit test in the same file using `TestDatabase::new_with_datastore` that calls `actually_activate` directly. If the task has a datastore method, a single test exercising the task end-to-end (including the limit/batching behavior) is sufficient — don't add a redundant test for the datastore method separately unless it has complex logic worth testing in isolation.
 
 ### 3. Register the module (`nexus/src/app/background/tasks/mod.rs`)
 
@@ -37,7 +37,7 @@ Add `pub task_<name>: Activator` to the `BackgroundTasks` struct, maintaining al
 
 ### 5. Config (`nexus-config/src/nexus_config.rs`)
 
-Add a config struct (e.g., `YourTaskConfig`) with at minimum `period_secs: Duration` (using `#[serde_as(as = "DurationSeconds<u64>")]`). If the task does bounded work per activation, name the limit field `max_<verb>_per_activation` (e.g., `max_delete_per_activation`, `max_update_per_activation`) to match existing conventions. Add the field to `BackgroundTaskConfig`. Update the test config literal and expected parse output at the bottom of the file.
+Add a config struct (e.g., `YourTaskConfig`) with at minimum `period_secs: Duration` (using `#[serde_as(as = "DurationSeconds<u64>")]`). If the task does bounded work per activation, name the limit field `max_<past_tense_verb>_per_activation` (e.g., `max_deleted_per_activation`, `max_timed_out_per_activation`) to match existing conventions. Add the field to `BackgroundTaskConfig`. Update the test config literal and expected parse output at the bottom of the file.
 
 ### 6. Config files
 
@@ -62,7 +62,7 @@ If the task needs a new index or schema change to support its query, add a migra
 
 ### 9. Datastore method (if needed)
 
-If the task needs a new query, add it in the appropriate `nexus/db-queries/src/db/datastore/` file. Add a test in the same file or in `nexus/db-queries/src/db/datastore/mod.rs`.
+If the task needs a new query, add it in the appropriate `nexus/db-queries/src/db/datastore/` file. Prefer the Diesel typed DSL over raw SQL (`diesel::sql_query`) for queries and test helpers. Only fall back to raw SQL when the DSL genuinely can't express the query.
 
 If the task modifies rows that other code paths also modify, think about races: what happens if both run concurrently on the same row? Both paths should typically guard their writes so only one succeeds.
 

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

@@ -2694,7 +2694,7 @@ fn print_task_audit_log_timeout_incomplete(details: &serde_json::Value) {
         Ok(status) => {
             const TIMED_OUT: &str = "timed_out:";
             const CUTOFF: &str = "cutoff:";
-            const MAX_UPDATE: &str = "max_update_per_activation:";
+            const MAX_UPDATE: &str = "max_timed_out_per_activation:";
             const ERROR: &str = "error:";
             const WIDTH: usize =
                 const_max_len(&[TIMED_OUT, CUTOFF, MAX_UPDATE, ERROR]) + 1;
@@ -2706,7 +2706,7 @@ fn print_task_audit_log_timeout_incomplete(details: &serde_json::Value) {
             );
             println!(
                 "    {MAX_UPDATE:<WIDTH$}{}",
-                status.max_update_per_activation
+                status.max_timed_out_per_activation
             );
             if let Some(error) = &status.error {
                 println!("    {ERROR:<WIDTH$}{error}");

dev-tools/omdb/tests/successes.out

@@ -597,9 +597,9 @@ task: "audit_log_timeout_incomplete"
   configured period: every <REDACTED_DURATION>m
   last completed activation: <REDACTED ITERATIONS>, triggered by <TRIGGERED_BY_REDACTED>
     started at <REDACTED_TIMESTAMP> (<REDACTED DURATION>s ago) and ran for <REDACTED DURATION>ms
-    timed_out:                 0
-    cutoff:                    <REDACTED_TIMESTAMP>
-    max_update_per_activation: 1000
+    timed_out:                    0
+    cutoff:                       <REDACTED_TIMESTAMP>
+    max_timed_out_per_activation: 1000
 
 task: "bfd_manager"
   configured period: every <REDACTED_DURATION>s
@@ -1200,9 +1200,9 @@ task: "audit_log_timeout_incomplete"
   configured period: every <REDACTED_DURATION>m
   last completed activation: <REDACTED ITERATIONS>, triggered by <TRIGGERED_BY_REDACTED>
     started at <REDACTED_TIMESTAMP> (<REDACTED DURATION>s ago) and ran for <REDACTED DURATION>ms
-    timed_out:                 0
-    cutoff:                    <REDACTED_TIMESTAMP>
-    max_update_per_activation: 1000
+    timed_out:                    0
+    cutoff:                       <REDACTED_TIMESTAMP>
+    max_timed_out_per_activation: 1000
 
 task: "bfd_manager"
   configured period: every <REDACTED_DURATION>s

nexus-config/src/nexus_config.rs

@@ -464,7 +464,7 @@ pub struct AuditLogTimeoutIncompleteConfig {
     pub timeout_secs: Duration,
 
     /// max rows per SQL statement
-    pub max_update_per_activation: u32,
+    pub max_timed_out_per_activation: u32,
 }
 
 #[serde_as]
@@ -1286,7 +1286,7 @@ mod test {
             session_cleanup.max_delete_per_activation = 10000
             audit_log_timeout_incomplete.period_secs = 600
             audit_log_timeout_incomplete.timeout_secs = 14400
-            audit_log_timeout_incomplete.max_update_per_activation = 1000
+            audit_log_timeout_incomplete.max_timed_out_per_activation = 1000
             [default_region_allocation_strategy]
             type = "random"
             seed = 0
@@ -1558,7 +1558,7 @@ mod test {
                             AuditLogTimeoutIncompleteConfig {
                                 period_secs: Duration::from_secs(600),
                                 timeout_secs: Duration::from_secs(14400),
-                                max_update_per_activation: 1000,
+                                max_timed_out_per_activation: 1000,
                             },
                     },
                     multicast: MulticastConfig { enabled: false },
@@ -1669,7 +1669,7 @@ mod test {
             session_cleanup.max_delete_per_activation = 10000
             audit_log_timeout_incomplete.period_secs = 600
             audit_log_timeout_incomplete.timeout_secs = 14400
-            audit_log_timeout_incomplete.max_update_per_activation = 1000
+            audit_log_timeout_incomplete.max_timed_out_per_activation = 1000
 
             [default_region_allocation_strategy]
             type = "random"

nexus/db-model/src/audit_log.rs

@@ -224,7 +224,7 @@ impl From<AuditLogEntryInitParams> for AuditLogEntryInit {
 
 /// `audit_log_complete` is a view on `audit_log` filtering for rows with
 /// non-null `time_completed`, not its own table.
-#[derive(Queryable, Selectable, Clone, Debug)]
+#[derive(Queryable, Selectable, Clone, Debug, PartialEq)]
 #[diesel(table_name = audit_log_complete)]
 pub struct AuditLogEntry {
     pub id: Uuid,

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

@@ -450,154 +450,16 @@ mod tests {
         id: Uuid,
         time_started: DateTime<Utc>,
     ) {
-        use diesel::sql_types;
-        diesel::sql_query(
-            "UPDATE omicron.public.audit_log \
-             SET time_started = $1 WHERE id = $2",
-        )
-        .bind::<sql_types::Timestamptz, _>(time_started)
-        .bind::<sql_types::Uuid, _>(id)
-        .execute_async(&*datastore.pool_connection_for_tests().await.unwrap())
-        .await
-        .unwrap();
-    }
-
-    #[tokio::test]
-    async fn test_audit_log_timeout_incomplete() {
-        let logctx = dev::test_setup_log("test_audit_log_timeout_incomplete");
-        let db = TestDatabase::new_with_datastore(&logctx.log).await;
-        let (opctx, datastore) = (db.opctx(), db.datastore());
-
-        let now = Utc::now();
-        let two_hours_ago = now - TimeDelta::try_hours(2).unwrap();
-        let cutoff = now - TimeDelta::try_hours(1).unwrap();
-
-        // Create an incomplete entry with time_started in the past
-        let old_entry = datastore
-            .audit_log_entry_init(opctx, make_entry_params("old-req").into())
-            .await
-            .unwrap();
-        set_time_started(datastore, old_entry.id, two_hours_ago).await;
-
-        // Create an incomplete entry that's recent (should not be timed out)
-        let _recent_entry = datastore
-            .audit_log_entry_init(opctx, make_entry_params("recent-req").into())
-            .await
-            .unwrap();
-
-        // Create an old entry that's already completed (should not be touched)
-        let completed_entry = datastore
-            .audit_log_entry_init(
-                opctx,
-                make_entry_params("completed-req").into(),
-            )
-            .await
-            .unwrap();
-        set_time_started(datastore, completed_entry.id, two_hours_ago).await;
-        datastore
-            .audit_log_entry_complete(
-                opctx,
-                &completed_entry,
-                AuditLogCompletion::Success { http_status_code: 200 }.into(),
+        use diesel::prelude::*;
+        use nexus_db_schema::schema::audit_log;
+        diesel::update(audit_log::table)
+            .filter(audit_log::id.eq(id))
+            .set(audit_log::time_started.eq(time_started))
+            .execute_async(
+                &*datastore.pool_connection_for_tests().await.unwrap(),
             )
             .await
-            .unwrap();
-
-        // Run the timeout
-        let timed_out = datastore
-            .audit_log_timeout_incomplete(opctx, cutoff, 100)
-            .await
-            .unwrap();
-
-        // Only the old incomplete entry should be affected
-        assert_eq!(timed_out, 1);
-
-        // The timed-out entry should now appear in the audit log with
-        // result_kind = timeout
-        let pagparams = DataPageParams {
-            marker: None,
-            limit: NonZeroU32::new(100).unwrap(),
-            direction: dropshot::PaginationOrder::Ascending,
-        };
-        let entries = datastore
-            .audit_log_list(opctx, &pagparams, two_hours_ago, None)
-            .await
-            .unwrap();
-
-        let timed_out_entry =
-            entries.iter().find(|e| e.id == old_entry.id).unwrap();
-        assert_eq!(timed_out_entry.result_kind, AuditLogResultKind::Timeout);
-        assert!(timed_out_entry.http_status_code.is_none());
-        assert!(timed_out_entry.error_code.is_none());
-        assert!(timed_out_entry.error_message.is_none());
-
-        // The completed entry should still be success
-        let completed =
-            entries.iter().find(|e| e.id == completed_entry.id).unwrap();
-        assert_eq!(completed.result_kind, AuditLogResultKind::Success);
-
-        // Running again should find nothing (idempotent)
-        let timed_out = datastore
-            .audit_log_timeout_incomplete(opctx, cutoff, 100)
-            .await
-            .unwrap();
-        assert_eq!(timed_out, 0);
-
-        db.terminate().await;
-        logctx.cleanup_successful();
-    }
-
-    #[tokio::test]
-    async fn test_audit_log_timeout_respects_limit() {
-        let logctx =
-            dev::test_setup_log("test_audit_log_timeout_respects_limit");
-        let db = TestDatabase::new_with_datastore(&logctx.log).await;
-        let (opctx, datastore) = (db.opctx(), db.datastore());
-
-        let now = Utc::now();
-        let two_hours_ago = now - TimeDelta::try_hours(2).unwrap();
-        let cutoff = now - TimeDelta::try_hours(1).unwrap();
-
-        // Create 5 stale incomplete entries
-        for i in 0..5 {
-            let entry = datastore
-                .audit_log_entry_init(
-                    opctx,
-                    make_entry_params(&format!("req-{i}")).into(),
-                )
-                .await
-                .unwrap();
-            set_time_started(datastore, entry.id, two_hours_ago).await;
-        }
-
-        // Timeout with limit of 2
-        let batch1 = datastore
-            .audit_log_timeout_incomplete(opctx, cutoff, 2)
-            .await
-            .unwrap();
-        assert_eq!(batch1, 2);
-
-        let batch2 = datastore
-            .audit_log_timeout_incomplete(opctx, cutoff, 2)
-            .await
-            .unwrap();
-        assert_eq!(batch2, 2);
-
-        let batch3 = datastore
-            .audit_log_timeout_incomplete(opctx, cutoff, 2)
-            .await
-            .unwrap();
-        assert_eq!(batch3, 1);
-
-        // All gone
-        let batch4 = datastore
-            .audit_log_timeout_incomplete(opctx, cutoff, 2)
-            .await
-            .unwrap();
-        assert_eq!(batch4, 0);
-
-        db.terminate().await;
-        logctx.cleanup_successful();
+            .expect("could not set time_started");
     }
 
     /// A late completion of an already-timed-out entry should be a no-op.

nexus/examples/config-second.toml

@@ -193,7 +193,7 @@ session_cleanup.period_secs = 300
 session_cleanup.max_delete_per_activation = 10000
 audit_log_timeout_incomplete.period_secs = 600
 audit_log_timeout_incomplete.timeout_secs = 14400
-audit_log_timeout_incomplete.max_update_per_activation = 1000
+audit_log_timeout_incomplete.max_timed_out_per_activation = 1000
 
 [default_region_allocation_strategy]
 # allocate region on 3 random distinct zpools, on 3 random distinct sleds.

nexus/examples/config.toml

@@ -177,7 +177,7 @@ session_cleanup.period_secs = 300
 session_cleanup.max_delete_per_activation = 10000
 audit_log_timeout_incomplete.period_secs = 600
 audit_log_timeout_incomplete.timeout_secs = 14400
-audit_log_timeout_incomplete.max_update_per_activation = 1000
+audit_log_timeout_incomplete.max_timed_out_per_activation = 1000
 
 [default_region_allocation_strategy]
 # allocate region on 3 random distinct zpools, on 3 random distinct sleds.

nexus/src/app/background/init.rs

@@ -1200,7 +1200,7 @@ impl BackgroundTasksInitializer {
                     config.audit_log_timeout_incomplete.timeout_secs,
                     config
                         .audit_log_timeout_incomplete
-                        .max_update_per_activation,
+                        .max_timed_out_per_activation,
                 ),
             ),
             opctx: opctx.child(BTreeMap::new()),