Don't allow two timeline_delete operations to run concurrently.

[hlinnaka]

If the timeline is already being deleted, return an error. We used to notice the duplicate request and error out in
persist_index_part_with_deleted_flag(), but it's better to detect it earlier. Add an explicit flag for the deletion.

Note: This doesn't do anything about the async cancellation problem (github issue #3478): if the original HTTP request dropped, because the client disconnected, the timeline deletion stops half-way through the operation. That needs to be fixed, too, but that's a separate story.

(This is a simpler replacement for PR #4194. I'm also working on the cancellation shielding, I'll open separate PR for that.)

[github-actions]

1004 tests run: 962 passed, 0 failed, 42 skipped (full report)

---

Flaky tests (1)

Postgres 15

• test_broken_timeline: ✅ debug

_{The comment gets automatically updated with the latest test results
f54eb39 at 2023-05-27T10:01:43.370Z :recycle:}

[koivunej]

I think this is fine, because the tests asserted the response status code but I'll let Christian approve.

[problame]

Some comments in delete_timeline are now stale, please go through them and adjust

Example

        // NB: If you call delete_timeline multiple times concurrently, they will
        // all go through the motions here. Make sure the code here is idempotent,
        // and don't error out if some of the shutdown tasks have already been
        // completed!

Other than that, I don't see why we need a mutex instead of an AtomicBool for this.

But I'm Ok with using the tokio mutex.

We should really fix the #3478 issue, though.

Giving approval because I'm on vacation next week, but please fix those comments.

[hlinnaka]

Some comments in delete_timeline are now stale, please go through them and adjust

Example

        // NB: If you call delete_timeline multiple times concurrently, they will
        // all go through the motions here. Make sure the code here is idempotent,
        // and don't error out if some of the shutdown tasks have already been
        // completed!

Updated that and a few other comments. Also, this race cannot happen any more:

-        let removed_timeline = timelines.remove(&timeline_id);
-        if removed_timeline.is_none() {
-            // This can legitimately happen if there's a concurrent call to this function.
-            //   T1                                             T2
-            //   lock
-            //   unlock
-            //                                                  lock
-            //                                                  unlock
-            //                                                  remove files
-            //                                                  lock
-            //                                                  remove from map
-            //                                                  unlock
-            //                                                  return
-            //   remove files
-            //   lock
-            //   remove from map observes empty map
-            //   unlock
-            //   return
-            debug!("concurrent call to this function won the race");

so I turned that into a panic instead.

Other than that, I don't see why we need a mutex instead of an AtomicBool for this.

But I'm Ok with using the tokio mutex.

The Mutex allows you to detect if another task is currently performing the deletion. With an AtomicBool, the boolean would need to indicate "is deletion in progress", and you'd need to be careful to set it back to false if the deletion fails half-way through. I think you'd need another bool for "did it already finish", or make it an atomic with three states:

1. deletion not in progress
2. deletion in progress
3. deletion completed

Currently, this uses try_lock to check if another deletion is in progress, but originally I actually actually wanted to wait for it to finish, like #4194 did. That would require pulling in the test changes from that PR, however, and because we still don't handle async cancellations gracefully, those tests would need some further tweaking to make them pass. After we fix #3478, we might want to do that, but for now I wanted to keep the behaviour and tests unchanged.

We should really fix the #3478 issue, though.

Yes, I'm doing that in #4314.