From f44942d1742891113c881622734903fc4ba37793 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Karol=20Bary=C5=82a?= <karol.baryla@scylladb.com>
Date: Wed, 9 Oct 2024 11:29:49 +0200
Subject: [PATCH] SE: Fix possible race condition

Consider this scenario:
- `retries_remaining` starts at 1, so we have 1 main execution and
  1 additional speculative execution to spawn.
- Main execution does some retries and exhausts execution plan this way.
- It sends the last attempt to DB and waits for response.
- Now the speculative execution starts.
- The plan is empty, so it immediately finishes and enters `None` block
  of the `match`.
- `async_tasks` is not empty, because main execution is still ongoing,
  so nothing happens.
- Main execution finishes with ignorable error, e.g. broken connection.
- `can_be_ignored` returns true, so we don't return and only assign to
  `last_error`
- Now `async_tasks` is empty, so `async_tasks.select_next_some()` branch
  of `select!` will never be executed again.
- Only `&mut sleep` will be executed (once) and do nothing because
  `retries_remaining == 0`.
- That means we are hanging forever - or until client timeout happens
  if one was configured.

This commit fixes this bug by simplifying the logic.
The new logic always checks if the resolved task is the last one, and
if so it returns, so it's not possible for the tasks to be exhausted
without finishing the `select!`.
---
 scylla/src/transport/speculative_execution.rs | 26 ++++++++-----------
 1 file changed, 11 insertions(+), 15 deletions(-)

diff --git a/scylla/src/transport/speculative_execution.rs b/scylla/src/transport/speculative_execution.rs
index 65389b5eb..ccf4feeaa 100644
--- a/scylla/src/transport/speculative_execution.rs
+++ b/scylla/src/transport/speculative_execution.rs
@@ -127,21 +127,17 @@ where
                 }
             }
             res = async_tasks.select_next_some() => {
-                match res {
-                    Some(r) => {
-                        if !can_be_ignored(&r) {
-                            return r;
-                        } else {
-                            last_error = Some(r)
-                        }
-                    },
-                    None =>  {
-                        if async_tasks.is_empty() && retries_remaining == 0 {
-                            return last_error.unwrap_or({
-                                Err(EMPTY_PLAN_ERROR)
-                            });
-                        }
-                    },
+                if let Some(r) = res {
+                    if !can_be_ignored(&r) {
+                        return r;
+                    } else {
+                        last_error = Some(r)
+                    }
+                }
+                if async_tasks.is_empty() && retries_remaining == 0 {
+                    return last_error.unwrap_or({
+                        Err(EMPTY_PLAN_ERROR)
+                    });
                 }
             }
         }