Merge 'row_cache: abort on exteral_updater::execute errors' from Benn…

…y Halevy

Currently the cache updaters aren't exception safe
yet they are intended to be.

Instead of allowing exceptions from
`external_updater::execute` escape `row_cache::update`,
abort using `on_fatal_internal_error`.

Future changes should harden all `execute` implementations
to effectively make them `noexcept`, then the pure virtual
definition can be made `noexcept` to cement that.

Fixes scylladb#15576

Closes scylladb#15577

* github.com:scylladb/scylladb:
  row_cache: abort on exteral_updater::execute errors
  row_cache: do_update: simplify _prev_snapshot_pos setup

replica/table.cc

@@ -752,6 +752,7 @@ table::update_cache(compaction_group& cg, lw_shared_ptr<memtable> m, std::vector
         ms_opt = make_combined_mutation_source(std::move(sources));
     }
     auto adder = row_cache::external_updater([this, m, ssts = std::move(ssts), new_ssts_ms = std::move(*ms_opt), &cg] () mutable {
+        // FIXME: the following isn't exception safe.
         for (auto& sst : ssts) {
             add_sstable(cg, sst);
             update_stats_for_new_sstable(sst);
@@ -1261,6 +1262,7 @@ compaction_group::update_sstable_lists_on_off_strategy_completion(sstables::comp
         virtual void execute() override {
             _cg.set_main_sstables(std::move(_new_main_list));
             _cg.set_maintenance_sstables(std::move(_new_maintenance_list));
+            // FIXME: the following is not exception safe
             _t.refresh_compound_sstable_set();
             // Input sstables aren't not removed from backlog tracker because they come from the maintenance set.
             _cg.backlog_tracker_adjust_charges({}, _new_main);
@@ -1347,6 +1349,7 @@ compaction_group::update_main_sstable_list_on_compaction_completion(sstables::co
         }
         virtual void execute() override {
             _cg.set_main_sstables(std::move(_new_sstables));
+            // FIXME: the following is not exception safe
             _t.refresh_compound_sstable_set();
             _cg.backlog_tracker_adjust_charges(_desc.old_sstables, _desc.new_sstables);
         }
@@ -2046,6 +2049,7 @@ future<db::replay_position> table::discard_sstables(db_clock::time_point truncat
     };
     auto p = make_lw_shared<pruner>(*this);
     co_await _cache.invalidate(row_cache::external_updater([this, p, truncated_at] {
+        // FIXME: the following isn't exception safe.
         for (const compaction_group_ptr& cg : compaction_groups()) {
             p->prune(*cg, truncated_at);
         }

row_cache.cc

@@ -1440,10 +1440,15 @@ future<> row_cache::do_update(row_cache::external_updater eu, row_cache::interna
         return get_units(_update_sem, 1);
     }).then([this, &eu, &iu] (auto permit) mutable {
       return eu.prepare().then([this, &eu, &iu, permit = std::move(permit)] () mutable {
-        auto pos = dht::ring_position::min();
-        eu.execute();
+        try {
+            eu.execute();
+        } catch (...) {
+            // Any error from execute is considered fatal
+            // to enforce exception safety.
+            on_fatal_internal_error(clogger, fmt::format("Fatal error during cache update: {}", std::current_exception()));
+        }
         [&] () noexcept {
-            _prev_snapshot_pos = std::move(pos);
+            _prev_snapshot_pos = dht::ring_position::min();
             _prev_snapshot = std::exchange(_underlying, _snapshot_source());
             ++_underlying_phase;
         }();

row_cache.hh

@@ -176,9 +176,19 @@ public:
     class external_updater_impl {
     public:
         virtual ~external_updater_impl() {}
+        // Prepare may return an exceptional future
+        // and the error is propagated to the row_cache::update caller.
+        // Hence, it must provide strong exception safety guarantees.
+        //
+        // Typically, `prepare` creates only temporary state
+        // to be atomically applied by `execute`, or, alternatively
+        // it must undo any side-effects on failure.
         virtual future<> prepare() { return make_ready_future<>(); }
         // FIXME: make execute() noexcept, that will require every updater to make execution exception safe,
         // also change function signature.
+        // See https://github.com/scylladb/scylladb/issues/15576
+        //
+        // For now, scylla aborts on any exception from `execute` 
         virtual void execute() = 0;
     };