diff --git a/src/etl/LoadBalancer.cpp b/src/etl/LoadBalancer.cpp index 3f9c784e9..9bb14b777 100644 --- a/src/etl/LoadBalancer.cpp +++ b/src/etl/LoadBalancer.cpp @@ -277,7 +277,7 @@ LoadBalancer::forwardToRippled( auto const cmd = boost::json::value_to(request.at("command")); if (shouldUseCache(isAdmin)) { // NOLINTNEXTLINE(bugprone-unchecked-optional-access) - if (auto cachedResponse = forwardingCache_->get(cmd); cachedResponse) { + if (auto cachedResponse = forwardingCache_->get(cmd, request); cachedResponse) { forwardingCounters_.cacheHit.get() += 1; return *std::move(cachedResponse); } @@ -311,8 +311,10 @@ LoadBalancer::forwardToRippled( } if (response) { - if (shouldUseCache(isAdmin) and not response->contains("error")) - forwardingCache_->put(cmd, *response); // NOLINT(bugprone-unchecked-optional-access) + if (shouldUseCache(isAdmin) and not response->contains("error")) { + // NOLINTNEXTLINE(bugprone-unchecked-optional-access) + forwardingCache_->put(cmd, request, *response); + } return *std::move(response); } diff --git a/src/rpc/RPCEngine.hpp b/src/rpc/RPCEngine.hpp index bb9ee6779..c6380ba9f 100644 --- a/src/rpc/RPCEngine.hpp +++ b/src/rpc/RPCEngine.hpp @@ -141,7 +141,7 @@ public: } if (not ctx.isAdmin and responseCache_) { - if (auto res = responseCache_->get(ctx.method); res.has_value()) + if (auto res = responseCache_->get(ctx.method, ctx.params); res.has_value()) return Result{*std::move(res)}; } @@ -174,7 +174,7 @@ public: if (not v) { notifyErrored(ctx.method); } else if (not ctx.isAdmin and responseCache_) { - responseCache_->put(ctx.method, v.result->as_object()); + responseCache_->put(ctx.method, ctx.params, v.result->as_object()); } return Result{std::move(v)}; diff --git a/src/util/ResponseExpirationCache.cpp b/src/util/ResponseExpirationCache.cpp index c1e5c3ba4..e43e53afc 100644 --- a/src/util/ResponseExpirationCache.cpp +++ b/src/util/ResponseExpirationCache.cpp @@ -4,11 +4,14 @@ #include +#include +#include #include #include #include #include #include +#include #include namespace util { @@ -44,9 +47,37 @@ ResponseExpirationCache::shouldCache(std::string const& cmd) return cache_.contains(cmd); } -std::optional -ResponseExpirationCache::get(std::string const& cmd) const +bool +ResponseExpirationCache::isBareRequest(boost::json::object const& request) { + // Keys that identify the command or are safe to ignore for caching purposes. + // + // "command" and "method" merely name the RPC command — they do not affect response content. + // + // "id" is safe to ignore because the web layer re-applies the current request's id to the + // response AFTER the cache lookup (see src/web/RPCServerHandler.hpp), so a cached body never + // leaks a stale id back to the client. Ignoring "id" is also necessary for WebSocket + // requests, which almost always carry an id and would otherwise never benefit from caching. + // + // "api_version" is intentionally NOT in this ignore-set: it can change the response body + // content (not just an echoed field), so requests carrying it must bypass the cache entirely. + static constexpr auto kIgnoredKeys = + std::to_array({"command", "method", "id"}); + + for (auto const& kv : request) { + std::string_view const key{kv.key()}; + if (not std::ranges::contains(kIgnoredKeys, key)) + return false; + } + return true; +} + +std::optional +ResponseExpirationCache::get(std::string const& cmd, boost::json::object const& request) const +{ + if (not isBareRequest(request)) + return std::nullopt; + auto it = cache_.find(cmd); if (it == cache_.end()) return std::nullopt; @@ -59,8 +90,15 @@ ResponseExpirationCache::get(std::string const& cmd) const } void -ResponseExpirationCache::put(std::string const& cmd, boost::json::object const& response) +ResponseExpirationCache::put( + std::string const& cmd, + boost::json::object const& request, + boost::json::object const& response +) { + if (not isBareRequest(request)) + return; + if (not shouldCache(cmd)) return; diff --git a/src/util/ResponseExpirationCache.hpp b/src/util/ResponseExpirationCache.hpp index 083a2daea..582f6faf6 100644 --- a/src/util/ResponseExpirationCache.hpp +++ b/src/util/ResponseExpirationCache.hpp @@ -62,6 +62,19 @@ class ResponseExpirationCache { bool shouldCache(std::string const& cmd); + /** + * @brief Check whether a request is "bare" (carries no response-customizing params). + * + * A request is bare iff every key it contains is one of: "command", "method", "id". + * Any other key (e.g. "api_version", "hash", "counters", "ledger_index") makes the request + * non-bare, and the cache must be bypassed for it. + * + * @param request The request object to inspect + * @return true if the request is bare; false otherwise + */ + static bool + isBareRequest(boost::json::object const& request); + public: /** * @brief Construct a new Cache object @@ -83,20 +96,30 @@ public: /** * @brief Get a response from the cache * + * Only bare requests (those carrying no response-customizing params beyond "command", "method", + * and "id") are served from the cache. Non-bare requests always return std::nullopt. + * * @param cmd The command to get the response for - * @return The response if it exists or std::nullopt otherwise + * @param request The full request object; used to determine whether the request is bare + * @return The cached response if available and not expired, or std::nullopt otherwise */ [[nodiscard]] std::optional - get(std::string const& cmd) const; + get(std::string const& cmd, boost::json::object const& request) const; /** * @brief Put a response into the cache if the request should be cached * + * Only bare requests (those carrying no response-customizing params beyond "command", "method", + * and "id") are stored in the cache. Non-bare requests are silently ignored. + * * @param cmd The command to store the response for + * @param request The full request object; used to determine whether the request is bare * @param response The response to store */ void - put(std::string const& cmd, boost::json::object const& response); + put(std::string const& cmd, + boost::json::object const& request, + boost::json::object const& response); /** * @brief Invalidate all entries in the cache diff --git a/tests/unit/etl/LoadBalancerTests.cpp b/tests/unit/etl/LoadBalancerTests.cpp index c331fd1ac..243c27616 100644 --- a/tests/unit/etl/LoadBalancerTests.cpp +++ b/tests/unit/etl/LoadBalancerTests.cpp @@ -762,6 +762,42 @@ TEST_F(LoadBalancerForwardToRippledPrometheusTests, forwardingCacheEnabled) }); } +TEST_F(LoadBalancerForwardToRippledPrometheusTests, forwardingCacheBypassedForNonBareRequest) +{ + configJson_.as_object()["forwarding"] = boost::json::object{{"cache_timeout", 10.}}; + EXPECT_CALL(sourceFactory_, makeSource).Times(2); + auto loadBalancer = makeLoadBalancer(); + + auto const nonBareRequest = boost::json::object{{"command", "server_info"}, {"counters", true}}; + + auto& cacheHitCounter = makeMock("forwarding_cache_hit_counter", ""); + auto& cacheMissCounter = makeMock("forwarding_cache_miss_counter", ""); + auto& successDurationCounter = + makeMock("forwarding_duration_milliseconds_counter", "{status=\"success\"}"); + + EXPECT_CALL(cacheMissCounter, add(1)).Times(2); + EXPECT_CALL(cacheHitCounter, add(testing::_)).Times(0); + EXPECT_CALL(successDurationCounter, add(testing::_)).Times(2); + + EXPECT_CALL( + sourceFactory_.sourceAt(0), + forwardToRippled( + nonBareRequest, clientIP_, LoadBalancer::kUserForwardingXUserValue, testing::_ + ) + ) + .Times(2) + .WillRepeatedly(Return(response_)); + + runSpawn([&](boost::asio::yield_context yield) { + EXPECT_EQ( + loadBalancer->forwardToRippled(nonBareRequest, clientIP_, false, yield), response_ + ); + EXPECT_EQ( + loadBalancer->forwardToRippled(nonBareRequest, clientIP_, false, yield), response_ + ); + }); +} + TEST_F(LoadBalancerForwardToRippledPrometheusTests, source0Fails) { EXPECT_CALL(sourceFactory_, makeSource).Times(2); diff --git a/tests/unit/rpc/RPCEngineTests.cpp b/tests/unit/rpc/RPCEngineTests.cpp index 8b7733c14..57ffaa0d8 100644 --- a/tests/unit/rpc/RPCEngineTests.cpp +++ b/tests/unit/rpc/RPCEngineTests.cpp @@ -417,17 +417,19 @@ TEST_P(RPCEngineCacheParameterTest, Test) handlerProvider ); int callTime = 2; + auto const bareParams = boost::json::object{}; EXPECT_CALL(*handlerProvider, isClioOnly).Times(callTime).WillRepeatedly(Return(false)); if (testParam.expectedCacheEnabled) { + // Cache hit on second call: handler only invoked once. EXPECT_CALL(*backend_, isTooBusy).WillOnce(Return(false)); EXPECT_CALL(*handlerProvider, getHandler) - .WillOnce(Return(AnyHandler{tests::common::HandlerFake{}})); + .WillOnce(Return(AnyHandler{tests::common::NoInputHandlerFake{}})); } else { EXPECT_CALL(*backend_, isTooBusy).Times(callTime).WillRepeatedly(Return(false)); EXPECT_CALL(*handlerProvider, getHandler) .Times(callTime) - .WillRepeatedly(Return(AnyHandler{tests::common::HandlerFake{}})); + .WillRepeatedly(Return(AnyHandler{tests::common::NoInputHandlerFake{}})); } while (callTime-- != 0) { @@ -436,7 +438,7 @@ TEST_P(RPCEngineCacheParameterTest, Test) yield, method, 1, - boost::json::parse(R"JSON({"hello": "world", "limit": 50})JSON").as_object(), + bareParams, nullptr, tagFactory, LedgerRange{.minSequence = 0, .maxSequence = 30}, @@ -448,7 +450,63 @@ TEST_P(RPCEngineCacheParameterTest, Test) ASSERT_TRUE(res.response.has_value()); EXPECT_EQ( res.response.value(), - boost::json::parse(R"JSON({ "computed": "world_50"})JSON").as_object() + boost::json::parse(R"JSON({"computed": "test"})JSON").as_object() + ); + }); + } +} + +TEST_F(RPCEngineTest, NonBareRequestBypassesCache) +{ + auto const cfgCache = ClioConfigDefinition{ + {"server.max_queue_size", ConfigValue{ConfigType::Integer}.defaultValue(2)}, + {"workers", + ConfigValue{ConfigType::Integer}.defaultValue(4).withConstraint(gValidateUint16)}, + {"rpc.cache_timeout", + ConfigValue{ConfigType::Double}.defaultValue(10.0).withConstraint(gValidatePositiveDouble)} + }; + + auto const notAdmin = false; + auto const method = "server_info"; + std::shared_ptr> engine = RPCEngine::makeRPCEngine( + cfgCache, + backend_, + mockLoadBalancerPtr_, + dosGuard, + queue, + *mockCountersPtr_, + handlerProvider + ); + + auto const nonBareParams = + boost::json::parse(R"JSON({"hello": "world", "limit": 50})JSON").as_object(); + + int callTime = 2; + EXPECT_CALL(*handlerProvider, isClioOnly).Times(callTime).WillRepeatedly(Return(false)); + EXPECT_CALL(*backend_, isTooBusy).Times(callTime).WillRepeatedly(Return(false)); + EXPECT_CALL(*handlerProvider, getHandler) + .Times(callTime) + .WillRepeatedly(Return(AnyHandler{tests::common::HandlerFake{}})); + + while (callTime-- != 0) { + runSpawn([&](auto yield) { + auto const ctx = web::Context( + yield, + method, + 1, + nonBareParams, + nullptr, + tagFactory, + LedgerRange{.minSequence = 0, .maxSequence = 30}, + "127.0.0.2", + notAdmin + ); + + auto const res = engine->buildResponse(ctx); + ASSERT_TRUE(res.response.has_value()); + EXPECT_EQ( + res.response.value(), + boost::json::parse(R"JSON({"computed": "world_50"})JSON").as_object() ); }); } diff --git a/tests/unit/util/ResponseExpirationCacheTests.cpp b/tests/unit/util/ResponseExpirationCacheTests.cpp index 23a187574..9ba3fbdd6 100644 --- a/tests/unit/util/ResponseExpirationCacheTests.cpp +++ b/tests/unit/util/ResponseExpirationCacheTests.cpp @@ -11,40 +11,104 @@ using namespace util; struct ResponseExpirationCacheTests : public ::testing::Test { protected: ResponseExpirationCache cache_{std::chrono::seconds{100}, {"key"}}; + boost::json::object bareRequest_{{"command", "key"}}; boost::json::object object_{{"key", "value"}}; }; TEST_F(ResponseExpirationCacheTests, PutAndGetNotExpired) { - EXPECT_FALSE(cache_.get("key").has_value()); + EXPECT_FALSE(cache_.get("key", bareRequest_).has_value()); - cache_.put("key", object_); - auto result = cache_.get("key"); + cache_.put("key", bareRequest_, object_); + auto result = cache_.get("key", bareRequest_); ASSERT_TRUE(result.has_value()); EXPECT_EQ(*result, object_); // NOLINT(bugprone-unchecked-optional-access) - result = cache_.get("key2"); + result = cache_.get("key2", bareRequest_); ASSERT_FALSE(result.has_value()); - cache_.put("key2", object_); - result = cache_.get("key2"); + cache_.put("key2", bareRequest_, object_); + result = cache_.get("key2", bareRequest_); ASSERT_FALSE(result.has_value()); } TEST_F(ResponseExpirationCacheTests, Invalidate) { - cache_.put("key", object_); + cache_.put("key", bareRequest_, object_); cache_.invalidate(); - EXPECT_FALSE(cache_.get("key").has_value()); + EXPECT_FALSE(cache_.get("key", bareRequest_).has_value()); } TEST_F(ResponseExpirationCacheTests, GetExpired) { ResponseExpirationCache cache{std::chrono::milliseconds{1}, {"key"}}; auto const response = boost::json::object{{"key", "value"}}; + auto const req = boost::json::object{{"command", "key"}}; - cache.put("key", response); + cache.put("key", req, response); std::this_thread::sleep_for(std::chrono::milliseconds{2}); - auto const result = cache.get("key"); + auto const result = cache.get("key", req); EXPECT_FALSE(result); } + +TEST_F(ResponseExpirationCacheTests, BareRequestCommandOnlyIsCached) +{ + boost::json::object const req{{"command", "key"}}; + cache_.put("key", req, object_); + auto const result = cache_.get("key", req); + ASSERT_TRUE(result.has_value()); + EXPECT_EQ(*result, object_); // NOLINT(bugprone-unchecked-optional-access) +} + +TEST_F(ResponseExpirationCacheTests, BareRequestWithIdIsCached) +{ + boost::json::object const req{{"command", "key"}, {"id", 42}}; + cache_.put("key", req, object_); + auto const result = cache_.get("key", req); + ASSERT_TRUE(result.has_value()); + EXPECT_EQ(*result, object_); // NOLINT(bugprone-unchecked-optional-access) +} + +TEST_F(ResponseExpirationCacheTests, BareRequestMethodAndIdIsCached) +{ + boost::json::object const req{{"method", "key"}, {"id", "req-1"}}; + cache_.put("key", req, object_); + auto const result = cache_.get("key", req); + ASSERT_TRUE(result.has_value()); + EXPECT_EQ(*result, object_); // NOLINT(bugprone-unchecked-optional-access) +} + +TEST_F(ResponseExpirationCacheTests, NonBareGetReturnNulloptAfterBarePut) +{ + boost::json::object const bareReq{{"command", "key"}}; + boost::json::object const nonBareReq{{"command", "key"}, {"limit", 50}}; + + cache_.put("key", bareReq, object_); + ASSERT_TRUE(cache_.get("key", bareReq).has_value()); + EXPECT_FALSE(cache_.get("key", nonBareReq).has_value()); +} + +TEST_F(ResponseExpirationCacheTests, NonBarePutDoesNotStore) +{ + boost::json::object const nonBareReq{{"command", "key"}, {"limit", 50}}; + boost::json::object const bareReq{{"command", "key"}}; + + cache_.put("key", nonBareReq, object_); + EXPECT_FALSE(cache_.get("key", bareReq).has_value()); +} + +TEST_F(ResponseExpirationCacheTests, ApiVersionMakesRequestNonBare) +{ + boost::json::object const req{{"command", "key"}, {"api_version", 1}}; + cache_.put("key", req, object_); + EXPECT_FALSE(cache_.get("key", req).has_value()); +} + +TEST_F(ResponseExpirationCacheTests, BareRequestIdOnlyIsCached) +{ + boost::json::object const req{{"id", 7}}; + cache_.put("key", req, object_); + auto const result = cache_.get("key", req); + ASSERT_TRUE(result.has_value()); + EXPECT_EQ(*result, object_); // NOLINT(bugprone-unchecked-optional-access) +}