Races in `DataStore::project_delete`

Background

DataStore::project_delete is initially given a db_project:

https://github.com/oxidecomputer/omicron/blob/8c0e4f75083dd5d88093a36478ccdab75487028a/nexus/db-queries/src/db/datastore/project.rs#L232-L237

This project has a generation number (db_project.rcgen). First, we make sure there are no child resources in the project:

https://github.com/oxidecomputer/omicron/blob/8c0e4f75083dd5d88093a36478ccdab75487028a/nexus/db-queries/src/db/datastore/project.rs#L240-L249

... And then we tombstone the project in an optimistic txn if its rcgen hasn't changed concurrently.

https://github.com/oxidecomputer/omicron/blob/8c0e4f75083dd5d88093a36478ccdab75487028a/nexus/db-queries/src/db/datastore/project.rs#L261-L267

This relies on all possible concurrent changes to the child resources correctly updating the project's rcgen. This is done mostly correctly, but there are a couple cases (at least?) that we seem to have missed.

DataStore::allocate_floating_ip

The first one is floating IP creation. We start by constructing an IncompleteExternalIp model with a project_id, and passing that into allocate_external_ip:

https://github.com/oxidecomputer/omicron/blob/8c0e4f75083dd5d88093a36478ccdab75487028a/nexus/db-queries/src/db/datastore/external_ip.rs#L330-L349

Looking down the call stack, that gets us into allocate_external_ip_on_connection, which boils down to:

https://github.com/oxidecomputer/omicron/blob/8c0e4f75083dd5d88093a36478ccdab75487028a/nexus/db-queries/src/db/datastore/external_ip.rs#L374

... with a bunch of error mapping tacked on. That get_result_async method is in the blanket impl:

https://github.com/oxidecomputer/omicron/blob/8c0e4f75083dd5d88093a36478ccdab75487028a/nexus/db-queries/src/db/queries/external_ip.rs#L877

The RunQueryDsl trait comes from the async_bb8_diesel which isn't doing any rcgen magic for us. And then looking backwards up the call stack from allocate_floating_ip, nothing at the nexus app / api layer does it for us either.

So, the race here in a nutshell is:

• Nexus A calls ensure_no_floating_ips_in_project, which succeeds.
• Nexus B calls allocate_floating_ip, which succeeds and doesn't bump the project's rcgen.
• Nexus A tombstones the project.

I'm not sure what the best fix for this would be. A few options:

• Add a subquery to NextExternalIp to bump the project's generation number. Trouble is that this CTE is used in other contexts where there isn't a project, so it would need to be conditional.
• In allocate_floating_ip, bump the generation in a transaction together with allocating the external IP.
• In project_delete, do all the ensure_no_* checks together in a transaction with the tombstone update.

---

By the way, I think there's another related race. allocate_floating_ip is called from floating_ip_create. It first does an authz lookup for the project:

https://github.com/oxidecomputer/omicron/blob/8c0e4f75083dd5d88093a36478ccdab75487028a/nexus/src/app/external_ip.rs#L114-L121

and then passes the project ID into allocate_floating_ip:

https://github.com/oxidecomputer/omicron/blob/8c0e4f75083dd5d88093a36478ccdab75487028a/nexus/src/app/external_ip.rs#L153-L160

So we could have this interleaving:

• Nexus A calls project_lookup.lookup_for and successfully gets the project.
• Nexus B calls project_delete and successfully tombstones the project.
• Nexus A calls allocate_floating_ip and successfully allocates the IP to the deleted project.

DataStore::silo_image_demote

When we demote an image from a silo to a specific project, we tack on a project id to the existing image row:

https://github.com/oxidecomputer/omicron/blob/8c0e4f75083dd5d88093a36478ccdab75487028a/nexus/db-queries/src/db/datastore/image.rs#L214-L220

We neglect to bump the project's rcgen here. This is a much simpler query than the IP allocation case, but it's the same sort of race:

• Nexus A calls ensure_no_project_images_in_project, which succeeds.
• Nexus B calls silo_image_demote, which succeeds and doesn't bump the project's rcgen.
• Nexus A tombstones the project.

Possible fixes:

• In silo_image_demote, bump the generation in a transaction together with setting the image's project id.
• Or do them together in a CTE.

---

And, again, by the way, we also have another race:

https://github.com/oxidecomputer/omicron/blob/8c0e4f75083dd5d88093a36478ccdab75487028a/nexus/src/app/image.rs#L211-L231

• Nexus A calls project_lookup.lookup_for and successfully gets the project.
• Nexus B calls project_delete and successfully tombstones the project.
• Nexus A calls silo_image_demote and successfully assigns the image to the deleted project.

The moral of the story, maybe?

My personal opinion: this optimistic dance we do with generation IDs is dangerously brittle in general because it requires whole-program analysis for correctness. In order for a mutating operation that's gated on an rcgen to be correct, it requires that all other relevant mutating operations must update rcgen.

In the case of projects, we use our DatastoreCollectionConfig machinery for most of the children (including Image!), like:

https://github.com/oxidecomputer/omicron/blob/8c0e4f75083dd5d88093a36478ccdab75487028a/nexus/db-model/src/project.rs#L98-L103

This helps us ensure we're doing the right thing with rcgen for insert operations, but:

• The silo_image_demote operation is entirely reasonable SQL, but it's not an insert so DatastoreCollectionConfig doesn't help there.
• There's no DatastoreCollectionConfig<ExternalIp> or DatastoreCollectionConfig<FloatingIp> for projects.
• Even if there were, we wouldn't insert floating IPs using Project::insert_resource taking advantage of DatastoreCollectionConfig; we have a very complex CTE that deals with IP address allocation.

In other words, our rcgen dance is a convention, or a sort of protocol. Every participant must uniformly follow it, enforced by code review. If we used transactions I think I'd argue that the db is guaranteeing correctness as long as we read the right things (for example, our project_delete transaction would still need to query all children to ensure none reference the project).

There's a tradeoff of course. As RFD 192 notes:

Applications use a variety of patterns to synchronize database updates, including both explicit and implicit locks in the database as well as optimistic concurrency control (OCC). These patterns can have drastically different latency (and correctness) impacts under different kinds of load. Workloads that run well at small scale can see faster-than-linear increase in latency as the system gets busier due to queueing delays waiting for locks. The suggestions here seek to minimize the use of explicit locks, minimize the hold time of all locks, and minimize the need for retry loops.

I want to acknowledge that, and I don't mean to minimize the issues it notes. In pessimistic concurrency, latency caused by lock contention can be a significant issue in some situations. In optimistic concurrency, I've seen cases where a "thundering herd" of racing db clients will attempt a transaction, only one of N will win, and all N-1 will fruitlessly retry. When done poorly this can be an O(n^2) waste of time.

I think the right solution is very situation dependent.

Lastly ...

If I understand our setup correctly, we are actually using pessimistic concurrency in lots of places where we might expect optimistic concurrency from eyeballing the code.

CockroachDB since v20.1 implicitly uses SELECT ... FOR UPDATE in the read phase of an UPDATE/UPSERT statement (see blog post, documentation). This effectively upgrades (or downgrades, depending on your perspective) what would have been an optimistic transaction to a pessimistic one. The idea as I understand it is to try to minimize client-side retries to improve overall throughput. If we wanted it not to do that, we'd have to set enable_implicit_select_for_update = false, but I don't think we are.

So, I suspect we have a ton of transaction_retry_wrapper loops that effectively only ever iterate once because CockroachDB is locking for us under the covers.

I think that after confirming and fixing the races here, we should probably:

• Audit unsafe uses of generation numbers.
• Find pessimistic transactions that could be optimistic.
• Take another pass over RFD0192 to see if we need to revise our recommendations.

Start by tracing project_delete in nexus/db-queries/src/db/datastore/project.rs alongside allocate_floating_ip in external_ip.rs and silo_image_demote in image.rs. Review the linked app-layer entry points and confirm the CockroachDB transaction behavior described in the issue. Done means the identified project-deletion races are covered by a decided concurrency strategy and regression tests.