Fix ensuring snapshot creation and deletion

[jmpesp]

If project_ensure_snapshot is passed a snapshot where the project and name matches a record that exists already, but the ID is different, then

.on_conflict((dsl::project_id, dsl::name))
.filter_target(dsl::time_deleted.is_null())
.do_update()
.set(dsl::time_modified.eq(dsl::time_modified)),

will incorrectly modify the existing record, leading callers of that function to erroneously believe they were the one to create that snapshot record. Change project_ensure_snapshot to check if an existing snapshot record has a matching project and name, and return ObjectAlreadyExists if so.

It's not appropriate to delete a snapshot in any state: if it's in state Creating, then a snapshot_create saga is working on it and concurrently running snapshot_delete will cause a conflict. Add an argument to project_delete_snapshot that describes the states that a snapshot can be deleted in, depending on on the call site.

[smklein]

(Looking at the old Project::insert_resource code) Could we do this without the transaction? diesel::insert_into should return the inserted row, which goes through insert_and_get_result_async.

It seems possible to call either:

• insert_and_get_result_async, and check for "not found" on the result, or to call
• insert_and_get_results_async, and to check for an "empty vec" as the result (aka, nothing was inserted).

Something like:

       let snapshots: Vec<Snapshot> = Project::insert_resource(
            project_id,
            diesel::insert_into(dsl::snapshot)
                .values(snapshot)
                .on_conflict((dsl::project_id, dsl::name))
                .filter_target(dsl::time_deleted.is_null())
                .do_nothing(),
        )
        .insert_and_get_results_async(self.pool_authorized(opctx).await?)
        .await
        .map_err(|e| match e {
            AsyncInsertError::CollectionNotFound => Error::ObjectNotFound {
                type_name: ResourceType::Project,
                lookup_type: LookupType::ById(project_id),
            },
            AsyncInsertError::DatabaseError(e) => {
                public_error_from_diesel_pool(e, ErrorHandler::Server)
            }
        })?;
        let Some(snapshot) = snapshots.get(0) else {
            // Return an error -- we failed to insert the snapshot.
        };

[jmpesp]

I originally tried insert_and_get_result_async with the additional filter (once I got it working), but I saw that it returned CollectionNotFound instead of DatabaseError. I'm trying the second suggestion now.

[smklein]

Do we still want to update the old record in this case?

[jmpesp]

Definitely not, but I'm at a loss as how to do that.

[smklein]

Can we use on_conflict().do_nothing()?

[jmpesp]

If I do

        let mut snapshots: Vec<Snapshot> = Project::insert_resource(
            project_id,
            diesel::insert_into(dsl::snapshot)
                .values(snapshot)
                .on_conflict((dsl::project_id, dsl::name))
                .filter_target(dsl::time_deleted.is_null())
                .do_nothing()
                .filter(dsl::id.eq(snapshot_id)),
        )
        .insert_and_get_results_async(self.pool_authorized(opctx).await?)

then I see a syntax error:

thread 'integration_tests::snapshots::test_create_snapshot_record_idempotent' panicked at 'called `Result::unwrap()` on an `Err` value: InternalError { internal_message: "unexpected database error: at or near \"where\": syntax error" }', nexus/tests/integration_tests/snapshots.rs:996:10

If I remove the .filter(dsl::id.eq(snapshot_id)), on the end of that (so it's just on_conflict + do_nothing, I see test_create_snapshot_record_idempotent failing on the second call here:

    // Test project_ensure_snapshot is idempotent

    let snapshot_created_1 = datastore
        .project_ensure_snapshot(&opctx, &authz_project, snapshot.clone())
        .await
        .unwrap();
                
    let snapshot_created_2 = datastore
        .project_ensure_snapshot(&opctx, &authz_project, snapshot.clone())
        .await
        .unwrap();

saying the snapshot already exists:

thread 'integration_tests::snapshots::test_create_snapshot_record_idempotent' panicked at 'called `Result::unwrap()` on an `Err` value: ObjectAlreadyExists { type_name: Snapshot, object_name: "snapshot" }', nexus/tests/integration_tests/snapshots.rs:1001:10

[jmpesp]

We spoke in DMs, and I reverted to the transaction to avoid modifying the existing snapshot's time_modified column.