[saga-recovery] Mark sagas with non-transient errors as abandoned by karencfv · Pull Request #10602 · oxidecomputer/omicron

karencfv · 2026-06-11T07:50:38Z

karencfv · 2026-07-01T09:22:45Z

+                // We can't abandon a saga whose current SEC is unknown (there's no row to
+                // conditionally update on), so a permanent failure with no `current_sec` is
+                // left as a recovery candidate.


This kind of sucks? I've left it like this due to the comment in the saga_update_state() method

/// It's conceivable that multiple SECs do try to udpate the same saga /// concurrently. That would be a bug. This is noticed and prevented by /// making this query conditional on current_sec and failing with a conflict /// if the current SEC has changed.

For what it's worth, I believe this cannot be None. I think the schema allowed for that because we thought we might use it to deal with failover, but I don't think we ever did.

I don't think you should do anything different here, except maybe make it an error instead of a warn and add that to the comment.

karencfv · 2026-07-01T09:29:07Z

To avoid annoying merge conflicts due to versioning, I'm not merging with main until the PR is approved.

davepacheco

Thanks!

davepacheco · 2026-07-08T18:11:12Z

+                // We can't abandon a saga whose current SEC is unknown (there's no row to
+                // conditionally update on), so a permanent failure with no `current_sec` is
+                // left as a recovery candidate.


For what it's worth, I believe this cannot be None. I think the schema allowed for that because we thought we might use it to deal with failover, but I don't think we ever did.

I don't think you should do anything different here, except maybe make it an error instead of a warn and add that to the comment.

davepacheco · 2026-07-08T18:36:24Z

(Feel free to ignore this.)

I tried the Claude skill from #10125 on it and got the following report. One of these (the full-struct destructuring one) is totally bogus, one is meaningless, and the other two are valid ones that I mentioned above. I did like the way it phrased it as: we made the write side strongly type-safe but the read side isn't yet.

Type Safety Review

Mode

Branch diff from merge-base 13937a1 (main) to HEAD (mark-failed-sagas-as-abandoned).

Files reviewed:

dev-tools/omdb/src/bin/omdb/db/saga.rs
nexus/db-model/src/saga_types.rs
nexus/db-queries/src/db/datastore/saga.rs
nexus/saga-recovery/src/recovery.rs
nexus/saga-recovery/src/status.rs
nexus/src/app/background/tasks/saga_recovery.rs

Overall, this branch uses the type system well. SagaStateTransition is a thoughtful design: it encodes at the type level the invariant that abandonment metadata must be supplied whenever a saga transitions to Abandoned. RecoveryFailureKind::classify uses an exhaustive match with no wildcard, so a new Error variant forces a compile-time decision. Destructuring is used to catch new struct fields in the omdb From<Saga> for SagaRow impl and in abandon_non_transient_failed_saga. A few things could be tightened.

Findings

SUGGESTION — Multiple representations / sentinel values — nexus/db-queries/src/db/datastore/saga.rs:31 and dev-tools/omdb/src/bin/omdb/db/saga.rs:441

Problem: SagaStateUpdate has all public fields and can be constructed directly, side-stepping the type-safe SagaStateTransition enum that exists precisely to enforce the invariant "abandonment metadata is present iff saga_state

== Abandoned". The omdb cmd_sagas_abandon path exploits this by constructing a SagaStateUpdate inline:

let new_state = SagaStateUpdate {
    saga_state: SagaState::Abandoned,
    abandon_reason: Some(nexus_db_model::SagaReasonAbandoned::Omdb),
    abandon_information: Some(information),
    abandon_time: Some(Utc::now()),
};

Nothing in the type system prevents someone from writing, for example, saga_state: SagaState::Running here while still supplying abandon_* fields, or vice versa. Today the DB CHECK constraint would reject the write, but that is a runtime backstop for something the type system could enforce statically. The concern is not hypothetical: SagaStateTransition was introduced specifically to close this gap on the saga-recovery path, and the omdb path recreates it.

Fix: Make SagaStateUpdate's fields non-pub (or make the whole struct pub(crate)) and have SagaStateTransition::into_update be the only way to build one. Promote into_update from private to pub. Then rewrite the omdb path to go through the enum:

let new_state = SagaStateTransition::Abandoned {
    reason: SagaReasonAbandoned::Omdb,
    information,
}
.into_update();

diesel::update(dsl::saga)
    .filter(dsl::id.eq(saga_id))
    .set(new_state)
    .execute_async(&*conn)
    .await?;

Now every path that produces a SagaStateUpdate goes through the invariant-preserving conversion, and the DB CHECK constraint becomes a defense-in-depth backstop rather than the primary enforcement.

SUGGESTION — Missing full-struct destructuring in serialization — nexus/db-queries/src/db/datastore/saga.rs:31

Problem: SagaStateUpdate derives AsChangeset. Diesel's derive is not a manual serialization site, so this is not a direct instance of category 8, but the conversion function that populates it (SagaStateTransition::into_update) is a hand-written mapping that would silently fail to propagate a new field if one were added to SagaStateUpdate. Today the four fields happen to align, but if a future column such as abandon_generation is added to the changeset struct, into_update will still compile while writing whatever the field's Default produces (or failing at the derive if there is no default). Because there is no destructuring of SagaStateUpdate inside into_update, no compile error will point at the mapping.

Fix: Not strictly necessary today, but consider defining SagaStateUpdate via a constructor that takes a SagaStateTransition directly (rather than exposing struct-literal construction), or add an explicit destructuring assertion in into_update to prove the field set is understood. The category-1 fix above accomplishes this incidentally.

SUGGESTION — Multiple representations / sentinel values — nexus/db-model/src/saga_types.rs:231-238

Problem: The Saga model exposes the abandonment tuple as three independent Option<T> fields whose "must be all-Some or all-None, and only Some when saga_state == Abandoned" invariant lives entirely in the comment and the DB CHECK constraint:

pub abandon_time: Option<chrono::DateTime<chrono::Utc>>,
pub abandon_reason: Option<SagaReasonAbandoned>,
pub abandon_information: Option<String>,

Every read-side consumer that wants to describe the abandonment must independently pattern-match three Options and either assume they agree or defensively handle mismatches. If the invariant is ever violated in the DB (bad migration, direct SQL update), consumers cannot express "this row is malformed" without extra plumbing.

The fix on the write path (SagaStateTransition) is only half the story: the read side has the same shape it always did.

Fix: Consider adding a read-side accessor that groups the tuple:

pub struct AbandonInfo {
    pub time: DateTime<Utc>,
    pub reason: SagaReasonAbandoned,
    pub information: String,
}

impl Saga {
    /// Returns abandonment metadata if the saga is abandoned.
    pub fn abandon_info(&self) -> Option<AbandonInfo> {
        match (self.abandon_time, self.abandon_reason, self.abandon_information.as_ref()) {
            (Some(time), Some(reason), Some(information)) => {
                Some(AbandonInfo { time, reason, information: information.clone() })
            }
            (None, None, None) => None,
            _ => None, // DB CHECK should prevent this; consider logging or returning Result
        }
    }
}

The three raw fields can stay for diesel, but consumers should be encouraged (or required, if the fields were made pub(crate)) to use the accessor. This mirrors the category-1 fix on the write side.

This is a lower-priority suggestion than the first because the Saga struct is essentially the row shape and the DB CHECK constraint does enforce consistency at the boundary. Worth noting but not blocking.

SUGGESTION — Weak enum / bool usage — nexus/db-queries/src/db/datastore/saga.rs:50-61

Problem: In impl From<SagaStateTransition> for SagaState, the abandoned arm uses reason: _, information: _:

SagaStateTransition::Abandoned { reason: _, information: _ } => {
    SagaState::Abandoned
}

That is already explicit and will error if a new field is added to Abandoned, which is good. The into_update function, by contrast, does not fully destructure the Abandoned variant either — it names reason and information but that is because it consumes them. This is fine as written.

Not a finding — noted because it's easy to accidentally break by using .. in future edits. Prefer keeping the explicit field: _ form so a new field forces a compile error.

No issues found in:

Stringly-typed values: SagaReasonAbandoned is a proper enum with the diesel impl_enum_type! machinery. The free-form abandon_information: String is genuinely free-form context and correctly typed as String.
Display / FromStr as footguns: No new Display or FromStr impls on domain types.
Magic literals / missing constants: SAGA_ABANDON_TIMEOUT: Duration = Duration::from_secs(15) is a named constant with a well-written docstring explaining the value's role. Other literals are one-off defaults (e.g., "manually abandoned") that do not need naming.
Missing newtypes for domain values: SagaId, SecId, SagaReasonAbandoned are all strong types. No bare Uuid fields introduced. SecId gained Serialize/Deserialize, which is fine.
Implicit runtime panics: No new production unwrap/expect calls. Test code uses .expect(...) and Arc::try_unwrap(...).unwrap(), which is acceptable in tests.
Vec when uniqueness matters: The new failures: &[RecoveryFailure] parameter is a sequence of events, not a set — duplicates are semantically meaningful (each is a separate observation), so &[] is correct.
Exhaustive match / no wildcards: RecoveryFailureKind::classify matches every Error variant explicitly with no _. impl From<SagaStateTransition> for SagaState, impl From<steno::SagaCachedState> for SagaStateTransition, and the match kind { Transient | Permanent } in abandon_non_transient_failed_saga all avoid wildcards. Excellent.
Full-struct destructuring on read: impl From<Saga> for SagaRow in omdb destructures all three new abandon_* fields; let RecoveryFailure { time, saga_id, current_sec, kind, message } = failure in abandon_non_transient_failed_saga destructures the full struct; SagaAbandonArgs { saga_id, information, bypass_sec_check } is destructured in the fn signature. If any of these structs grows a field, the compiler will find these sites.

Summary

0 blocking issues, 3 suggestions (plus one non-finding note).

The strongest opportunity is closing the SagaStateUpdate back door in the omdb command by routing it through SagaStateTransition::into_update. That would make the "abandonment metadata is present iff saga_state == Abandoned" invariant type-enforced at every write site, with the DB CHECK constraint as a backstop rather than the primary defense.

karencfv

Thanks for the review @davepacheco! I think I've addressed all of your comments. The new SagaRow implementation feels a little wonky, but does the job! Let me know what you think

smklein · 2026-07-14T16:42:08Z

+    abandon_reason omicron.public.saga_abandon_reason,
+    abandon_comment TEXT,


Thanks for adding these. Either in #10592 or a follow-up, I can propagate this info to the FM system, which can highlight this info for operators.

Sweet! Does this format work for you? Or is there a better way to represent these for FM?

This format is fine; we're scooping stuff up now based on the saga_state, and it's easy to grab an auxiliary data hanging off the rest of the row here too

davepacheco · 2026-07-15T16:57:09Z

+            let row = dsl::saga
                .filter(dsl::id.eq(args.saga_id))
+                .select(nexus_db_model::SagaRow::as_select())
                .first_async(&*conn)
-                .await?
+                .await?;
+            Saga::try_from(row)?


I didn't realize the degree to which we already had code outside of the DataStore that was querying for sagas directly with diesel. In an ideal world, we wouldn't expose SagaRow at all outside the datastore and callers wouldn't have to think about this conversion.

This makes me think it might be worth looking into a better way to encapsulate the model type at the diesel layer (e.g., is there some trait we could impl like Selectable that would let us just do this select as-is and do the (fallible) load that validates the metadata?). But that's probably beyond the scope of this PR.

Yeah, it was really annoying when I discovered that as well :( I can write up a follow up issue though!

davepacheco · 2026-07-15T17:00:59Z

    prompt.read_and_validate("y/N", "y")?;
    drop(prompt);

+    let new_state = SagaStateDbFields {


It would seem a bit safer to use datastore.saga_update_state() here. I think that would let you make SagaStateDbFields non-pub.

I tried doing this before, but it's not really possible to do this and keep the same functionality of the command. I would have to completely modify the logic around finding the current_sec (bypass_sec_check flag functionality), and this seems out of scope for this PR. I can write up an issue for a follow up PR though!

Semi-unrelated, how disruptive do you think it would be to make the current_sec field not be an option (not as part of this PR of course!)?

Semi-unrelated, how disruptive do you think it would be to make the current_sec field not be an option (not as part of this PR of course!)?

I'd like to do that, too, and I think it ought to be fine, but we don't really have a way of dealing with migrations like that that fail. e.g., if there was some saga out there with current_sec IS NULL, the obvious schema migration would just fail and I think the update would get stuck in a bad place. Make we could do something where we install a sentinel value for any such rows?

davepacheco · 2026-07-15T18:14:26Z

Thanks, this is getting there!

karencfv · 2026-07-16T04:13:04Z

Thanks for the thorough review @davepacheco! I think this should be ready to go now 🤞

I'll merge main and fix merge conflicts once I get the ✅ to avoid fixing merge conflicts more than once

davepacheco

Thanks for the changes. Just a few very small nits here that don't need another round of review.

davepacheco · 2026-07-16T21:24:59Z

-/// read via [`Saga::abandon_metadata`]. Reads go through `TryFrom<SagaRow>`,
-/// which rejects rows whose metadata is inconsistent with `saga_state` with an
-/// [`Error::internal_error`].
+/// into a single private `abandon` field, kept all or none. Reads go through


Suggested change

/// into a single private `abandon` field, kept all or none. Reads go through

/// into a single `abandon` field, kept all or none. Reads go through

davepacheco · 2026-07-16T21:27:22Z

-                if saga_state == SagaState::Abandoned {
-                    Some(abandon_metadata.clone())
-                } else {
+            _ => {


nit: I think we generally prefer avoiding these wildcards (listing out the specific variants instead)

davepacheco · 2026-07-16T21:29:49Z

    prompt.read_and_validate("y/N", "y")?;
    drop(prompt);

+    let new_state = SagaStateDbFields {


Semi-unrelated, how disruptive do you think it would be to make the current_sec field not be an option (not as part of this PR of course!)?

I'd like to do that, too, and I think it ought to be fine, but we don't really have a way of dealing with migrations like that that fail. e.g., if there was some saga out there with current_sec IS NULL, the obvious schema migration would just fail and I think the update would get stuck in a bad place. Make we could do something where we install a sentinel value for any such rows?

davepacheco · 2026-07-16T21:30:27Z

+            match Saga::try_from(row.clone()) {
+                Ok(saga_row) => sagas.push(saga_row),
+                Err(e) => {
+                    println!("WARNING: Skipping saga with id {}: {e}", row.id())


Suggested change

println!("WARNING: Skipping saga with id {}: {e}", row.id())

eprintln!("WARNING: Skipping saga with id {}: {e}", row.id())

Main's saga-abandonment-metadata migration (#10602) records when, why, and with what comment a saga was abandoned. Thread that metadata through the SagaSummary projection and ObservedSagaState::Abandoned so the Abandoned fact's case comment can say who abandoned the saga (saga recovery vs an operator via omdb) and why, instead of the now-wrong "abandoned by Nexus". Also fix the semantic fallout of merging main into this branch, which a textual merge could not catch (and which broke CI): - SitrepBuilder lost its public log field; use the debug-log API - open_case now takes the case comment directly - test_util's input_builder must pass observed_sagas to Input::builder - the unfinished-sagas test must insert SagaRow, not the validated Saga

karencfv added 16 commits June 11, 2026 19:49

[saga-recovery] Mark sagas with non-transient errors as abandoned

bfc7913

clean up

262c202

modify crdb saga table

91ab273

add constraint for abandoned entries

1e0de41

add some todos

751042b

fix tests

d337525

make it impossible to update state to abandoned without metadata

ef5ccd6

abandon failed sagas

6c6aa4a

mark errors as transient or not

18149b2

clean up

51b54e7

set timeout for DB write

028aaf5

add some tests

7664a59

fix column names and omdb command

6ca4fcb

fix omdb command

ec811f6

clean up

2c6ce6f

clean up

74cecfd

karencfv commented Jul 1, 2026

View reviewed changes

karencfv requested review from davepacheco and jgallagher July 1, 2026 09:29

karencfv marked this pull request as ready for review July 1, 2026 09:29

karencfv mentioned this pull request Jul 1, 2026

[update-status] Re-enable stuck saga reporting #10725

Open

davepacheco reviewed Jul 8, 2026

View reviewed changes

karencfv added 6 commits July 14, 2026 14:04

address comments

b76dbe0

address comments

e1ff220

remove tiemout

33c351b

change DB field

a8f7f5c

restrict module

23571f7

enforce type safety

60a6f17

new SagaRow type for raw saga information

3db2f42

karencfv commented Jul 14, 2026

View reviewed changes

smklein reviewed Jul 14, 2026

View reviewed changes

karencfv commented Jul 15, 2026

View reviewed changes

Comment thread nexus/db-model/src/saga_types.rs Outdated

karencfv added 2 commits July 15, 2026 14:41

clean up decision matrix

8f6a7b0

clean up

e52b15f

davepacheco reviewed Jul 15, 2026

View reviewed changes

karencfv added 4 commits July 16, 2026 12:14

address comments

6aefad8

type name change and log failed saga row validation

88acaf1

add second constraint

928b85e

simplify Saga try_from

d2a8e4a

fix docs

916f2ff

davepacheco approved these changes Jul 16, 2026

View reviewed changes

This was referenced Jul 16, 2026

current_sec should not be Option<T> for a saga #10848

Open

Clean up saga querying outside of Datastore #10849

Open

karencfv added 2 commits July 17, 2026 10:40

clean up

202efed

merge main

1a03bc6

karencfv enabled auto-merge (squash) July 16, 2026 23:25

karencfv disabled auto-merge July 17, 2026 00:08

merge main

062543b

karencfv enabled auto-merge (squash) July 17, 2026 00:26

karencfv merged commit a396ca6 into oxidecomputer:main Jul 17, 2026
19 checks passed

karencfv deleted the mark-failed-sagas-as-abandoned branch July 17, 2026 10:06

karencfv mentioned this pull request Jul 22, 2026

[update-status] enable stuck saga reporting #10891

Open

		abandon_reason omicron.public.saga_abandon_reason,
		abandon_comment TEXT,

	/// into a single private `abandon` field, kept all or none. Reads go through
	/// into a single `abandon` field, kept all or none. Reads go through

	println!("WARNING: Skipping saga with id {}: {e}", row.id())
	eprintln!("WARNING: Skipping saga with id {}: {e}", row.id())

Uh oh!

Conversation

karencfv commented Jun 11, 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!

karencfv commented Jul 1, 2026

Uh oh!

davepacheco left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

davepacheco commented Jul 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Type Safety Review

Mode

Findings

SUGGESTION — Multiple representations / sentinel values — nexus/db-queries/src/db/datastore/saga.rs:31 and dev-tools/omdb/src/bin/omdb/db/saga.rs:441

SUGGESTION — Missing full-struct destructuring in serialization — nexus/db-queries/src/db/datastore/saga.rs:31

SUGGESTION — Multiple representations / sentinel values — nexus/db-model/src/saga_types.rs:231-238

SUGGESTION — Weak enum / bool usage — nexus/db-queries/src/db/datastore/saga.rs:50-61

No issues found in:

Summary

Uh oh!

karencfv 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!

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!

karencfv Jul 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

davepacheco commented Jul 15, 2026

Uh oh!

karencfv commented Jul 16, 2026

Uh oh!

davepacheco 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!

karencfv commented Jun 11, 2026 •

edited

Loading

davepacheco commented Jul 8, 2026 •

edited

Loading

karencfv Jul 15, 2026 •

edited

Loading