Lack of atomicity in some fog db operations

[cbeck88]

I noticed randomly a defect in the following code:

https://github.com/mobilecoinfoundation/mobilecoin/blob/6825f2296b246cf7051e6c9750d61ac559e75663/fog/sql_recovery_db/src/lib.rs#L161

    /// Mark a given ingest invocation as decommissioned.
    fn decommission_ingest_invocation_impl(
        &self,
        conn: &PgConnection,
        ingest_invocation_id: &IngestInvocationId,
    ) -> Result<(), Error> {
        // Mark the ingest invocation as decommissioned.
        diesel::update(
            schema::ingest_invocations::dsl::ingest_invocations
                .filter(schema::ingest_invocations::dsl::id.eq(**ingest_invocation_id)),
        )
        .set((
            schema::ingest_invocations::dsl::decommissioned.eq(true),
            schema::ingest_invocations::dsl::last_active_at.eq(diesel::expression::dsl::now),
        ))
        .execute(conn)?;

        // Write a user event.
        let new_event =
            models::NewUserEvent::decommission_ingest_invocation(**ingest_invocation_id);

        diesel::insert_into(schema::user_events::table)
            .values(&new_event)
            .execute(conn)?;

        Ok(())
    }

This code marks an ingest invocation as decommissioned, and updates its "last active at" timestamp. But this happens in a separate diesel action from the one which adds the decommissioned event to the user events table.

If we lost connection to the DB in between, then the user event would never be written, which defeats the point of decommissioning this ingest invocation, since the users would still keep querying it, not knowing that it is decommissioned.

Ideally this would all be executed in one operation.

Separarely, I'm not sure it's necessary to change the "last active at" time, since marking something decommissioning may not really represent "activity".

We should check if there are other similar improvements that could be made in the fog db diesel code.

[eranrund]

This is meant to be called from inside a transaction, e.g. https://github.com/mobilecoinfoundation/mobilecoin/blob/6825f2296b246cf7051e6c9750d61ac559e75663/fog/sql_recovery_db/src/lib.rs#L435

(and it does look like it is being called from inside one)

[cbeck88]

I guess you're right, it is being used correctly.

I might leave this open because I'm not sure it makes sense to update the last active at time when decommissioning

thanks for clearing this up