fix: let resource-doc and swagger caches survive an unreachable Redis - #59
Merged
Merged
Conversation
The product caches in Caching already route through tryGet/trySet, whose comment states the intent plainly: if Redis is unreachable, treat it as a miss and recompute instead of failing the whole request. The resource-doc and swagger caches sit directly above them in the same file and did not -- they called Redis.use bare, and Redis.use throws rather than returning None. A Redis blip therefore turned every /resource-docs, /swagger, OpenAPI and message-docs request into a 500. These are exactly the documents API Explorer and Portal load on startup, so the blast radius is the whole developer-facing surface. Route all four get/set pairs through the same wrappers and give them explicit return types matching the product caches (Option[String] / Unit). Every call site already discarded the set return value, so narrowing it to Unit is source-compatible. Measured by running the suite against a dead Redis port (OBP_CACHE_REDIS_PORT=6399), which makes every Redis call fail: before 3273 tests, 135 failures after 3273 tests, 39 failures The 96 that disappear are the ones served from these caches: ResourceDocsTest (55), V7ResourceDocsAggregationTest (12), SwaggerDocsTest (11), MessageDocsJsonSchemaTest (8), DynamicEndpointsTest (6), ResourceDocsTechnologyTest (2), GetMessageDocsSwaggerTest (1) and Http4sServerIntegrationTest (1). With a healthy Redis the suite is unchanged: 3273 tests, 0 failures. The remaining 39 are other Redis dependencies, untouched here: rate limiting (23), the endpoints that exist to inspect Redis itself (12), and four consent/dauth scenarios that assert on an error message rather than a status code -- those still reject the request correctly, just with different wording. Rate limiting is unaffected in another way worth stating: RateLimitingUtil calls Redis.use directly rather than going through Caching, so the "REDIS_UNAVAILABLE" branch in getCounterState stays dead code. Making it live would mean changing Redis.use itself, which every caller shares.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.



What and why
Cachinghas two groups of cache accessors sitting directly next to each other. The product caches route throughtryGet/trySet, whose comment states the intent plainly:The resource-doc and swagger caches immediately above them did not. They called
Redis.usebare — andRedis.usethrows rather than returningNone. So a Redis blip turned every/resource-docs,/swagger, OpenAPI and message-docs request into a 500. Those are the documents API Explorer and Portal load on startup, so the blast radius is the whole developer-facing surface.Two adjacent groups, one protected and one not, with the explaining comment attached to the protected one: this reads as a missing wrapper rather than a deliberate difference.
Changes
obp-api/src/main/scala/code/api/cache/Caching.scala(only file touched)tryGet/trySetwrappers.Option[String]for gets,Unitfor sets. Every call site already discarded the set return value, so narrowing it is source-compatible (verified by compiling).Test evidence
Measured by running the full suite against a dead Redis port (
OBP_CACHE_REDIS_PORT=6399), which makes every Redis call fail. Both runs on the same base (4d21dc6cf), same machine:The 96 that disappear are exactly the suites served from these caches:
ResourceDocsTestV7ResourceDocsAggregationTestSwaggerDocsTestMessageDocsJsonSchemaTestDynamicEndpointsTestResourceDocsTechnologyTestGetMessageDocsSwaggerTestHttp4sServerIntegrationTestWith a healthy Redis the suite is unchanged: 3273 tests, 0 failures, identical to the pre-change baseline.
CI on this branch: all jobs green.
The remaining 39, classified
These are other Redis dependencies, untouched by this change. Verified by aggregating the
JedisConnectionExceptionstack frames across all four shard logs:RateLimitTest7,RateLimitingTest(v4) 6,RateLimitsTest4,AuthRateLimiterTest3,ConsumerTest2,RateLimitingTest(v5.1) 1RateLimitingUtil.getCounterState,consumerRateLimitState,getCallCounterForPeriod,MappedRateLimitingProvider.createConsumerCallLimits,Caching.invalidateRateLimitCacheLogCacheEndpointTest7,CacheEndpointsTest5ConsentsTest1,ConsentRequestTest2,dauthTest1No resource-doc or swagger call site remains in those stacks.
The third group is worth being precise about, because it is not a 500: the status-code assertion passes and the next line fails. In
ConsentRequestTest.scala:169-170the endpoint still returns 401, but the message is notConsentHeaderValueInvalid. With Redis down these paths still reject the request correctly, just with different wording. Their counts are identical before and after this change.Trade-offs and known limitations
RateLimitingUtilcallsRedis.usedirectly rather than going throughCaching, so this change cannot reach it. In particulargetCounterState'scase None => "REDIS_UNAVAILABLE"fail-open branch remains unreachable, becauseRedis.usethrows instead of returningNone. Making it live means changingRedis.useitself, which every caller shares — a larger change with its own blast radius, deliberately not bundled here.memoize-based caches were already safe and are untouched.Caching.memoizeSyncWithProvider/memoizeWithProvidergo through scalacache, which logsFailed to read from cacheat WARN and continues. They produce a lot of log noise when Redis is down but do not fail requests.Redis.use— both heavier than the property being checked. The before/after numbers are reproducible withOBP_CACHE_REDIS_PORT=<unused port> ./run_tests_parallel.sh.