Add locking to asset update/delete at API level

[jjnesbitt]

Closes #1469

Opening this as a draft, as the locking has been added, but I haven't figured out a good way to test this, since we'd need to test against a race condition.

This adds a lock at the API level for asset update and delete, as well as updates the asset paths implementation to raise an AssetAlreadyExists error, if the unique-version-path unique constraint fails on asset path creation (i.e., if an asset with a conflicting path is attempted to be added to a version).

Since we can't effectively test the API level race condition, I've done testing locally without the included changes, confirmed I was able to reproduce the error, and then tested locally with the included changes, and confirmed that the API behaves as expected.

[waxlamp]

Can you spam your local instance with O(dozens) of requests to create an asset?

[jjnesbitt]

Can you spam your local instance with O(dozens) of requests to create an asset?

We could do that, but that would:

1. Involve multithreading in a test
2. Make the test stochastic, which is very undesirable.

If you just meant doing locally outside of our testing suite as a sanity check, I'll probably do that yes. I'm not sure if the dev server is multi-threaded or not, but if not I could just run it with gunicorn.

[waxlamp]

If you just meant doing locally outside of our testing suite as a sanity check, I'll probably do that yes. I'm not sure if the dev server is multi-threaded or not, but if not I could just run it with gunicorn.

This is what I meant, yes, but I see you're (rightly) more concerned with automated tests, not just "seeing if it works in dev".

Can you spam your local instance with O(dozens) of requests to create an asset?

We could do that, but that would:

1. Involve multithreading in a test

To make the test deterministic, we would want some way to execute the first request and then suspend it during that critical section where the race condition becomes possible, then execute a second request to completion, and then finally resume the first request. This sounds like an interesting systems programming type challenge but perhaps not easy or natural to accomplish in Python, and probably not worth the effort.

2. Make the test stochastic, which is very undesirable.

Unpopular opinion incoming: if you can virtually guarantee that the race condition can be hit (by spamming an unreasonably large number of requests), and the test passes when the proper 400 error is raised, then I don't think the stochasticity is too bad. It's sort of like how we don't observe quantum effects at macroscopic scales--the effects exist but are virtually undetectable.

But, I agree that going to the trouble of doing this may not be worth it.

Maybe this is a case where we test in dev to make sure this largely works as intended, and then deploy to production to see if the recurring Sentry errors stop. If crafting a test is prohibitively difficult, perhaps that would be good enough at least for now.

[dandibot]

🚀 PR was released in v0.3.16 🚀