From 918130befd007086e1ea71b9ef3b35086c72bf3c Mon Sep 17 00:00:00 2001 From: Dario Mirovic Date: Fri, 24 Apr 2026 17:03:34 +0200 Subject: [PATCH 1/2] utils: loading_cache: add insert() that is a no-op when caching is disabled When the cache is constructed with expiry == 0 the underlying storage is never instantiated and get_ptr() asserts via caching_enabled(). This is fine for callers that need a handle into the cache, but it makes get_ptr() unusable for write-only insertions on caches whose expiry is configurable at runtime (e.g. caches driven by a LiveUpdate config option that the operator may set to 0). Add a new insert(k, load) method on loading_cache that returns a future<> and is a no-op when caching is disabled, otherwise forwards to get_ptr(k, load) and discards the resulting handle. This completes the disabled-mode safety contract of the cache for the write side, mirroring the fallback that get() already provides for the read side. Switch authorized_prepared_statements_cache::insert() from get_ptr().discard_result() to the new insert(), which fixes the crash 'Assertion caching_enabled() failed' in authorized_prepared_statements_cache::insert() that occurs when permissions_validity_in_ms is set to 0 and a prepared statement is executed under authentication. Fixes SCYLLADB-1699 --- cql3/authorized_prepared_statements_cache.hh | 4 ++-- utils/loading_cache.hh | 23 ++++++++++++++++++-- 2 files changed, 23 insertions(+), 4 deletions(-) diff --git a/cql3/authorized_prepared_statements_cache.hh b/cql3/authorized_prepared_statements_cache.hh index 392fa4fda8..9810f42c0c 100644 --- a/cql3/authorized_prepared_statements_cache.hh +++ b/cql3/authorized_prepared_statements_cache.hh @@ -136,9 +136,9 @@ public: {} future<> insert(auth::authenticated_user user, cql3::prepared_cache_key_type prep_cache_key, value_type v) noexcept { - return _cache.get_ptr(key_type(std::move(user), std::move(prep_cache_key)), [v = std::move(v)] (const cache_key_type&) mutable { + return _cache.insert(key_type(std::move(user), std::move(prep_cache_key)), [v = std::move(v)] (const cache_key_type&) mutable { return make_ready_future(std::move(v)); - }).discard_result(); + }); } value_ptr find(const auth::authenticated_user& user, const cql3::prepared_cache_key_type& prep_cache_key) { diff --git a/utils/loading_cache.hh b/utils/loading_cache.hh index b4e4983004..ad76e69bb3 100644 --- a/utils/loading_cache.hh +++ b/utils/loading_cache.hh @@ -65,8 +65,9 @@ struct do_nothing_loading_cache_stats { /// The values are going to be evicted from the cache if they are not accessed during the "expiration" period or haven't /// been reloaded even once during the same period. /// -/// If "expiration" is set to zero - the caching is going to be disabled and get_XXX(...) is going to call the "loader" callback -/// every time in order to get the requested value. +/// If "expiration" is set to zero - the caching is going to be disabled and get(...) is going to call the "loader" callback +/// every time in order to get the requested value. insert(...) is going to be a no-op in this mode. get_ptr(...) is not +/// safe to call when caching is disabled (it asserts) since it returns a handle into the cache. /// /// \note In order to avoid the eviction of cached entries due to "aging" of the contained value the user has to choose /// the "expiration" to be at least ("refresh" + "max load latency"). This way the value is going to stay in the cache and is going to be @@ -353,6 +354,24 @@ public: return get_ptr(k, _load); } + /// \brief Insert a value into the cache, loading it via \p load if not already present. + /// + /// Equivalent to get_ptr(k, load).discard_result() when caching is enabled, + /// but is a no-op when caching is disabled (i.e. the cache was constructed + /// with expiry == 0). Use this when you only want the side effect of + /// populating the cache and don't need a handle to the cached value. + /// + /// Unlike get_ptr(), it is safe to call this on a cache configured with + /// caching disabled. + template + requires std::is_invocable_r_v, LoadFunc, const key_type&> + future<> insert(const Key& k, LoadFunc&& load) { + if (!caching_enabled()) { + return make_ready_future<>(); + } + return get_ptr(k, std::forward(load)).discard_result(); + } + future get(const Key& k) { static_assert(ReloadEnabled == loading_cache_reload_enabled::yes, ""); From 3875d79ac6fcfc6bad97f7735093064a2333a94e Mon Sep 17 00:00:00 2001 From: Dario Mirovic Date: Fri, 24 Apr 2026 17:07:33 +0200 Subject: [PATCH 2/2] test: boost: regression test for loading_cache::insert with caching disabled Add two test cases for the new loading_cache::insert() method: * test_loading_cache_insert verifies that insert() populates the cache and invokes the loader exactly once per key when caching is enabled. * test_loading_cache_insert_caching_disabled is a regression test for SCYLLADB-1699: when the cache is constructed with expiry == 0 (caching disabled), insert() must be a no-op rather than asserting in loading_cache::get_ptr() via caching_enabled(). The loader must not be invoked and the cache must remain empty. Refs SCYLLADB-1699 --- test/boost/loading_cache_test.cc | 38 ++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/test/boost/loading_cache_test.cc b/test/boost/loading_cache_test.cc index 7896d5c7d3..1914a22a7c 100644 --- a/test/boost/loading_cache_test.cc +++ b/test/boost/loading_cache_test.cc @@ -823,4 +823,42 @@ SEASTAR_TEST_CASE(test_prepared_statement_small_cache) { }, small_cache_config); } +SEASTAR_THREAD_TEST_CASE(test_loading_cache_insert) { + using namespace std::chrono; + loader loader; + loading_cache_for_test loading_cache(num_loaders, 1h, testlog); + auto stop_cache = seastar::defer([&loading_cache] { loading_cache.stop().get(); }); + + // insert() must populate the cache and invoke the loader exactly once. + loading_cache.insert(0, loader.get()).get(); + BOOST_REQUIRE_EQUAL(loader.load_count(), 1); + BOOST_REQUIRE_EQUAL(loading_cache.size(), 1); + auto vp = loading_cache.find(0); + BOOST_REQUIRE(vp != nullptr); + BOOST_REQUIRE_EQUAL(*vp, test_string); + + // A second insert() for the same key must not re-invoke the loader. + loading_cache.insert(0, loader.get()).get(); + BOOST_REQUIRE_EQUAL(loader.load_count(), 1); + BOOST_REQUIRE_EQUAL(loading_cache.size(), 1); +} + +// Regression test for SCYLLADB-1699: insert() on a cache constructed with +// expiry == 0 (caching disabled) must be a no-op rather than asserting in +// loading_cache::get_ptr(). +SEASTAR_THREAD_TEST_CASE(test_loading_cache_insert_caching_disabled) { + using namespace std::chrono; + loader loader; + loading_cache_for_test loading_cache(num_loaders, 0ms, testlog); + auto stop_cache = seastar::defer([&loading_cache] { loading_cache.stop().get(); }); + + auto f = loading_cache.insert(0, loader.get()); + loading_cache.insert(0, loader.get()).get(); + + // The loader must not have been invoked and the cache must remain empty. + BOOST_REQUIRE_EQUAL(loader.load_count(), 0); + BOOST_REQUIRE_EQUAL(loading_cache.size(), 0); + BOOST_REQUIRE(loading_cache.find(0) == nullptr); +} + BOOST_AUTO_TEST_SUITE_END()