From 6d0575f3f725b9bd6c2b786dd3ca7f71dab66fdd Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 31 Oct 2026 09:54:32 +0000 Subject: [PATCH 01/13] Add Redis cache for SDLC systems and SDLC project lookups - Spring Cache on Spring Data Redis (Lettuce), values as JSON with type information limited to this application's and the JDK's classes - sdlc-systems (10m TTL): SDLC systems by id, read on every create, update and SDLC call - external-projects (5m TTL): GET /projects/{id}/external answers per SDLC system and external id; "unknown" answers are not cached; a background sync bypasses the cache and refreshes the entry (ExternalProjectService) - Redis failures are logged and skipped (LoggingCacheErrorHandler), with 500ms Redis timeouts instead of Lettuce's 60s default, so an outage slows requests down instead of failing them - The custom RedisCacheConfiguration applies Boot's spring.cache.redis.* settings (key prefix etc.), which defining the bean would otherwise drop - compose.yaml + spring-boot-docker-compose start Redis for spring-boot:run - Tests use a real Redis (Testcontainers, one container per run, a key prefix per context): served from cache, unknown not cached, sync refreshes, entities/records survive JSON, keys prefixed with TTL, Redis down still works (error handler, sync bypass and prefix each verified to fail without their fix) Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_012MLyPXGHeCc1fvTeB8JnT2 --- README.md | 19 ++ compose.yaml | 7 + docs/docker.md | 5 +- pom.xml | 22 +++ .../resourceapi/config/CacheConfig.java | 71 +++++++ .../config/CacheTtlProperties.java | 17 ++ .../controllers/ProjectRestController.java | 6 +- .../services/ExternalProjectService.java | 32 ++++ .../services/ProjectSyncService.java | 6 +- .../services/impl/SdlcSystemServiceImpl.java | 5 + src/main/resources/application.properties | 8 + .../RedisTestContainerInitializer.java | 42 +++++ .../cache/CacheIntegrationTest.java | 178 ++++++++++++++++++ .../cache/CacheUnavailableTest.java | 72 +++++++ .../ProjectRestControllerConflictTest.java | 4 +- .../ProjectRestControllerSdlcFailureTest.java | 10 +- src/test/resources/META-INF/spring.factories | 2 + 17 files changed, 492 insertions(+), 14 deletions(-) create mode 100644 compose.yaml create mode 100644 src/main/java/com/company/resourceapi/config/CacheConfig.java create mode 100644 src/main/java/com/company/resourceapi/config/CacheTtlProperties.java create mode 100644 src/main/java/com/company/resourceapi/services/ExternalProjectService.java create mode 100644 src/test/java/com/company/resourceapi/RedisTestContainerInitializer.java create mode 100644 src/test/java/com/company/resourceapi/cache/CacheIntegrationTest.java create mode 100644 src/test/java/com/company/resourceapi/cache/CacheUnavailableTest.java create mode 100644 src/test/resources/META-INF/spring.factories diff --git a/README.md b/README.md index 7efb8ef..ce69c34 100644 --- a/README.md +++ b/README.md @@ -71,6 +71,25 @@ The OpenAPI document is served at `/v3/api-docs` and Swagger UI at `/swagger-ui. annotations. Only `/api/**` is documented. To turn both off (e.g. in production), set `springdoc.api-docs.enabled=false` and `springdoc.swagger-ui.enabled=false`. +## Redis +The application uses Redis (`spring.data.redis.host`/`port`, default `localhost:6379`). + +**Cache** (Spring Cache on Spring Data Redis, values stored as JSON): + +| Cache | What | TTL | +|---|---|---| +| `sdlc-systems` | SDLC systems by id, read on every create, update and SDLC call | `resourceapi.cache.sdlc-systems-ttl` (10m) | +| `external-projects` | `GET /api/v2/projects/{id}/external` answers per SDLC system and external id; "unknown" answers are not cached | `resourceapi.cache.external-projects-ttl` (5m) | + +A background sync always asks the SDLC system and refreshes the cached answer. SDLC systems are not changed +through this API; one changed directly in the database is picked up when its entry expires. The cache is an +optimization: if Redis is unreachable, cache reads and writes are logged and skipped (each waits at most the +500 ms Redis timeouts) and requests are served without it. + +**Local development**: `./mvnw spring-boot:run` starts Redis from `compose.yaml` automatically +(`spring-boot-docker-compose`); with `java -jar`, start it yourself with `docker compose up -d` and point +`spring.data.redis.port` at it. Tests start their own Redis with Testcontainers, so they need Docker. + ## Tracing Every request gets a trace id ([Micrometer Tracing](https://micrometer.io/docs/tracing) with Brave, W3C trace context): diff --git a/compose.yaml b/compose.yaml new file mode 100644 index 0000000..1824de0 --- /dev/null +++ b/compose.yaml @@ -0,0 +1,7 @@ +# Local development services. `./mvnw spring-boot:run` starts them automatically +# (spring-boot-docker-compose); or run `docker compose up -d` yourself. +services: + redis: + image: redis:8-alpine + ports: + - "6379" diff --git a/docs/docker.md b/docs/docker.md index 09e4700..0805423 100644 --- a/docs/docker.md +++ b/docs/docker.md @@ -125,9 +125,12 @@ the build. The executable is built for the architecture of the machine (or `--pl ## Configuration -All three images are configured like the application itself, e.g. with environment variables: +All three images are configured like the application itself, e.g. with environment variables. The +application uses Redis (see the README); point it at one with `SPRING_DATA_REDIS_HOST`/`SPRING_DATA_REDIS_PORT` +(without Redis it still works, uncached): ```sh +docker run --rm -p 8080:8080 -e SPRING_DATA_REDIS_HOST=redis --network my-network resourceapi docker run --rm -p 9090:9090 -e SERVER_PORT=9090 resourceapi docker run --rm -p 8080:8080 -e SPRING_HTTP_CLIENTS_READ_TIMEOUT=10s resourceapi:aot ``` diff --git a/pom.xml b/pom.xml index b1c7804..0105df1 100644 --- a/pom.xml +++ b/pom.xml @@ -49,6 +49,15 @@ io.micrometer micrometer-tracing-bridge-brave + + + org.springframework.boot + spring-boot-starter-data-redis + + + org.springframework.boot + spring-boot-starter-cache + org.apache.httpcomponents.client5 @@ -59,6 +68,13 @@ spring-boot-h2console + + + org.springframework.boot + spring-boot-docker-compose + runtime + true + org.springframework.boot spring-boot-devtools @@ -87,6 +103,12 @@ spring-boot-starter-webmvc-test test + + + org.springframework.boot + spring-boot-testcontainers + test + org.wiremock diff --git a/src/main/java/com/company/resourceapi/config/CacheConfig.java b/src/main/java/com/company/resourceapi/config/CacheConfig.java new file mode 100644 index 0000000..2e622cc --- /dev/null +++ b/src/main/java/com/company/resourceapi/config/CacheConfig.java @@ -0,0 +1,71 @@ +package com.company.resourceapi.config; + +import org.springframework.boot.cache.autoconfigure.CacheProperties; +import org.springframework.boot.cache.autoconfigure.RedisCacheManagerBuilderCustomizer; +import org.springframework.boot.context.properties.EnableConfigurationProperties; +import org.springframework.cache.annotation.CachingConfigurer; +import org.springframework.cache.annotation.EnableCaching; +import org.springframework.cache.interceptor.CacheErrorHandler; +import org.springframework.cache.interceptor.LoggingCacheErrorHandler; +import org.springframework.context.annotation.Bean; +import org.springframework.context.annotation.Configuration; +import org.springframework.data.redis.cache.RedisCacheConfiguration; +import org.springframework.data.redis.serializer.GenericJacksonJsonRedisSerializer; +import org.springframework.data.redis.serializer.RedisSerializationContext.SerializationPair; +import tools.jackson.databind.jsontype.BasicPolymorphicTypeValidator; + +/** + * Redis-backed Spring Cache. Values are stored as JSON with their type, limited to this + * application's and the JDK's classes. A cache that can't be reached is logged and skipped, so + * Redis being down makes requests slower, not fail. + */ +@Configuration +@EnableCaching +@EnableConfigurationProperties({CacheProperties.class, CacheTtlProperties.class}) +public class CacheConfig implements CachingConfigurer { + + public static final String SDLC_SYSTEMS = "sdlc-systems"; + public static final String EXTERNAL_PROJECTS = "external-projects"; + + /** + * Defining this bean replaces Boot's own, so Boot's spring.cache.redis.* settings are applied + * here (otherwise e.g. the key prefix would silently be ignored). + */ + @Bean + RedisCacheConfiguration redisCacheConfiguration(CacheProperties cacheProperties) { + var typeValidator = BasicPolymorphicTypeValidator.builder() + .allowIfSubType("com.company.resourceapi.") + .allowIfSubType("java.") + .build(); + var serializer = GenericJacksonJsonRedisSerializer.builder() + .enableDefaultTyping(typeValidator) + .build(); + var config = RedisCacheConfiguration.defaultCacheConfig() + .serializeValuesWith(SerializationPair.fromSerializer(serializer)) + .disableCachingNullValues(); + + var redis = cacheProperties.getRedis(); + if (redis.getTimeToLive() != null) { + config = config.entryTtl(redis.getTimeToLive()); + } + if (redis.getKeyPrefix() != null) { + config = config.prefixCacheNameWith(redis.getKeyPrefix()); + } + if (!redis.isUseKeyPrefix()) { + config = config.disableKeyPrefix(); + } + return config; + } + + @Bean + RedisCacheManagerBuilderCustomizer cacheTimeToLive(RedisCacheConfiguration defaults, CacheTtlProperties ttl) { + return builder -> builder + .withCacheConfiguration(SDLC_SYSTEMS, defaults.entryTtl(ttl.sdlcSystemsTtl())) + .withCacheConfiguration(EXTERNAL_PROJECTS, defaults.entryTtl(ttl.externalProjectsTtl())); + } + + @Override + public CacheErrorHandler errorHandler() { + return new LoggingCacheErrorHandler(); + } +} diff --git a/src/main/java/com/company/resourceapi/config/CacheTtlProperties.java b/src/main/java/com/company/resourceapi/config/CacheTtlProperties.java new file mode 100644 index 0000000..e1399f2 --- /dev/null +++ b/src/main/java/com/company/resourceapi/config/CacheTtlProperties.java @@ -0,0 +1,17 @@ +package com.company.resourceapi.config; + +import java.time.Duration; +import org.springframework.boot.context.properties.ConfigurationProperties; +import org.springframework.boot.context.properties.bind.DefaultValue; + +/** + * Time to live of the Redis caches ({@code resourceapi.cache.*}). + * + * @param sdlcSystemsTtl SDLC systems by id; they change rarely and only outside this API + * @param externalProjectsTtl projects as described by their SDLC system ({@code GET .../external}) + */ +@ConfigurationProperties("resourceapi.cache") +public record CacheTtlProperties( + @DefaultValue("10m") Duration sdlcSystemsTtl, + @DefaultValue("5m") Duration externalProjectsTtl) { +} diff --git a/src/main/java/com/company/resourceapi/controllers/ProjectRestController.java b/src/main/java/com/company/resourceapi/controllers/ProjectRestController.java index eae8e54..ea6f9a4 100755 --- a/src/main/java/com/company/resourceapi/controllers/ProjectRestController.java +++ b/src/main/java/com/company/resourceapi/controllers/ProjectRestController.java @@ -9,11 +9,11 @@ import static org.springframework.http.HttpStatus.SERVICE_UNAVAILABLE; import com.company.resourceapi.clients.ExternalProject; -import com.company.resourceapi.clients.SdlcProjectClient; import com.company.resourceapi.entities.Project; import com.company.resourceapi.entities.SdlcSystem; import com.company.resourceapi.exceptions.ConflictException; import com.company.resourceapi.exceptions.NotFoundException; +import com.company.resourceapi.services.ExternalProjectService; import com.company.resourceapi.services.ProjectService; import com.company.resourceapi.services.SdlcSystemService; import io.swagger.v3.oas.annotations.Operation; @@ -58,7 +58,7 @@ public class ProjectRestController { private final ProjectService projectService; private final SdlcSystemService sdlcSystemService; - private final SdlcProjectClient sdlcProjectClient; + private final ExternalProjectService externalProjectService; @Operation(summary = "Get a Project") @GetMapping(ENDPOINT_ID) @@ -74,7 +74,7 @@ public ResponseEntity getExternalProject( @Parameter(name = PATH_VARIABLE_ID, description = API_PARAM_ID, required = true) @PathVariable(PATH_VARIABLE_ID) final long projectId) throws NotFoundException { var project = projectService.getProject(projectId); - return sdlcProjectClient.fetchProject(project.getSdlcSystem(), project.getExternalId()) + return externalProjectService.findProject(project.getSdlcSystem(), project.getExternalId()) .map(ResponseEntity::ok) .orElseGet(() -> ResponseEntity.status(NOT_FOUND).build()); } diff --git a/src/main/java/com/company/resourceapi/services/ExternalProjectService.java b/src/main/java/com/company/resourceapi/services/ExternalProjectService.java new file mode 100644 index 0000000..41ed2c5 --- /dev/null +++ b/src/main/java/com/company/resourceapi/services/ExternalProjectService.java @@ -0,0 +1,32 @@ +package com.company.resourceapi.services; + +import static com.company.resourceapi.config.CacheConfig.EXTERNAL_PROJECTS; + +import com.company.resourceapi.clients.ExternalProject; +import com.company.resourceapi.clients.SdlcProjectClient; +import com.company.resourceapi.entities.SdlcSystem; +import java.util.Optional; +import lombok.RequiredArgsConstructor; +import org.springframework.cache.annotation.CachePut; +import org.springframework.cache.annotation.Cacheable; +import org.springframework.stereotype.Service; + +/** Projects as described by their SDLC system, cached per SDLC system and external id. */ +@Service +@RequiredArgsConstructor +public class ExternalProjectService { + + private final SdlcProjectClient sdlcProjectClient; + + /** Cached; a project unknown to the SDLC system isn't cached, so it is found once it exists. */ + @Cacheable(cacheNames = EXTERNAL_PROJECTS, key = "#sdlcSystem.id + ':' + #externalId", unless = "#result == null") + public Optional findProject(SdlcSystem sdlcSystem, String externalId) { + return sdlcProjectClient.fetchProject(sdlcSystem, externalId); + } + + /** Always asks the SDLC system, and puts the fresh answer in the cache. */ + @CachePut(cacheNames = EXTERNAL_PROJECTS, key = "#sdlcSystem.id + ':' + #externalId", unless = "#result == null") + public Optional refreshProject(SdlcSystem sdlcSystem, String externalId) { + return sdlcProjectClient.fetchProject(sdlcSystem, externalId); + } +} diff --git a/src/main/java/com/company/resourceapi/services/ProjectSyncService.java b/src/main/java/com/company/resourceapi/services/ProjectSyncService.java index d6c5d40..f0e5880 100644 --- a/src/main/java/com/company/resourceapi/services/ProjectSyncService.java +++ b/src/main/java/com/company/resourceapi/services/ProjectSyncService.java @@ -2,7 +2,6 @@ import static java.util.concurrent.CompletableFuture.completedFuture; -import com.company.resourceapi.clients.SdlcProjectClient; import java.util.concurrent.CompletableFuture; import java.util.concurrent.atomic.AtomicBoolean; import lombok.RequiredArgsConstructor; @@ -18,7 +17,8 @@ public class ProjectSyncService { private final ProjectService projectService; - private final SdlcProjectClient sdlcProjectClient; + /** Bypasses the cache (a sync must see the current state) and refreshes it. */ + private final ExternalProjectService externalProjectService; /** * Runs on a virtual thread of the application task executor, in the caller's trace. Failures are @@ -33,7 +33,7 @@ public CompletableFuture syncName(long projectId) { var externalId = project.getExternalId(); var sdlcSystemId = project.getSdlcSystem().getId(); - var externalProject = sdlcProjectClient.fetchProject(project.getSdlcSystem(), externalId); + var externalProject = externalProjectService.refreshProject(project.getSdlcSystem(), externalId); if (externalProject.isEmpty()) { log.warn("Project {} ({}) is unknown to its SDLC system", projectId, externalId); return completedFuture(null); diff --git a/src/main/java/com/company/resourceapi/services/impl/SdlcSystemServiceImpl.java b/src/main/java/com/company/resourceapi/services/impl/SdlcSystemServiceImpl.java index a9b60ec..95f9746 100755 --- a/src/main/java/com/company/resourceapi/services/impl/SdlcSystemServiceImpl.java +++ b/src/main/java/com/company/resourceapi/services/impl/SdlcSystemServiceImpl.java @@ -1,10 +1,13 @@ package com.company.resourceapi.services.impl; +import static com.company.resourceapi.config.CacheConfig.SDLC_SYSTEMS; + import com.company.resourceapi.entities.SdlcSystem; import com.company.resourceapi.exceptions.NotFoundException; import com.company.resourceapi.repositories.SdlcSystemRepository; import com.company.resourceapi.services.SdlcSystemService; import lombok.RequiredArgsConstructor; +import org.springframework.cache.annotation.Cacheable; import org.springframework.stereotype.Service; @Service @@ -13,6 +16,8 @@ public class SdlcSystemServiceImpl implements SdlcSystemService { private final SdlcSystemRepository sdlcSystemRepository; + /** Cached: SDLC systems are read on every create, update and SDLC call, and rarely change. */ + @Cacheable(SDLC_SYSTEMS) public SdlcSystem getSdlcSystem(long id) throws NotFoundException { return sdlcSystemRepository.findById(id) .orElseThrow(() -> new NotFoundException(SdlcSystem.class, id)); diff --git a/src/main/resources/application.properties b/src/main/resources/application.properties index 81ccaf5..1d9cde7 100755 --- a/src/main/resources/application.properties +++ b/src/main/resources/application.properties @@ -12,6 +12,14 @@ spring.task.execution.simple.reject-tasks-when-limit-reached=true # on shutdown, let running background tasks finish (up to 10s) spring.task.execution.shutdown.await-termination=true spring.task.execution.shutdown.await-termination-period=10s +# redis (cache); spring.data.redis.host/port default to localhost:6379. Short timeouts: the cache is an +# optimization, and with Redis unreachable each cache read/write waits this long before it is skipped +# (Lettuce's default command timeout is 60s) +spring.data.redis.connect-timeout=500ms +spring.data.redis.timeout=500ms +spring.cache.redis.key-prefix=resourceapi:: +resourceapi.cache.sdlc-systems-ttl=10m +resourceapi.cache.external-projects-ttl=5m # datasource spring.datasource.url=jdbc:h2:mem:test;MODE=MySQL;DATABASE_TO_LOWER=TRUE spring.sql.init.platform=h2 diff --git a/src/test/java/com/company/resourceapi/RedisTestContainerInitializer.java b/src/test/java/com/company/resourceapi/RedisTestContainerInitializer.java new file mode 100644 index 0000000..ec1a96b --- /dev/null +++ b/src/test/java/com/company/resourceapi/RedisTestContainerInitializer.java @@ -0,0 +1,42 @@ +package com.company.resourceapi; + +import java.util.Map; +import java.util.UUID; +import org.springframework.context.ApplicationContextInitializer; +import org.springframework.context.ConfigurableApplicationContext; +import org.springframework.core.env.MapPropertySource; +import org.testcontainers.containers.GenericContainer; +import org.testcontainers.utility.DockerImageName; + +/** + * Gives every test application context a real Redis: one container, started on first use and + * shared by all contexts of the test run (registered in META-INF/spring.factories). + * + *

Each context gets its own key prefix: contexts have their own in-memory database, so the same + * id can mean different rows in two contexts and they must not see each other's cache entries. + * A test can still point at another Redis with its own {@code spring.data.redis.*} properties. + */ +public class RedisTestContainerInitializer implements ApplicationContextInitializer { + + private static final GenericContainer REDIS = + new GenericContainer<>(DockerImageName.parse("redis:8-alpine")).withExposedPorts(6379); + + @Override + public void initialize(ConfigurableApplicationContext context) { + startOnce(); + var propertySources = context.getEnvironment().getPropertySources(); + // defaults: a test's own spring.data.redis.* properties win + propertySources.addLast(new MapPropertySource("redisTestContainer", Map.of( + "spring.data.redis.host", REDIS.getHost(), + "spring.data.redis.port", REDIS.getMappedPort(6379)))); + // overrides application.properties + propertySources.addFirst(new MapPropertySource("redisTestContainerKeyPrefix", Map.of( + "spring.cache.redis.key-prefix", "test-" + UUID.randomUUID() + "::"))); + } + + private static synchronized void startOnce() { + if (!REDIS.isRunning()) { + REDIS.start(); + } + } +} diff --git a/src/test/java/com/company/resourceapi/cache/CacheIntegrationTest.java b/src/test/java/com/company/resourceapi/cache/CacheIntegrationTest.java new file mode 100644 index 0000000..72c91df --- /dev/null +++ b/src/test/java/com/company/resourceapi/cache/CacheIntegrationTest.java @@ -0,0 +1,178 @@ +package com.company.resourceapi.cache; + +import static com.company.resourceapi.config.CacheConfig.EXTERNAL_PROJECTS; +import static com.company.resourceapi.config.CacheConfig.SDLC_SYSTEMS; +import static com.github.tomakehurst.wiremock.client.WireMock.getRequestedFor; +import static com.github.tomakehurst.wiremock.client.WireMock.notFound; +import static com.github.tomakehurst.wiremock.client.WireMock.okJson; +import static com.github.tomakehurst.wiremock.client.WireMock.urlEqualTo; +import static com.github.tomakehurst.wiremock.core.WireMockConfiguration.wireMockConfig; +import static org.assertj.core.api.Assertions.assertThat; +import static org.hamcrest.Matchers.equalTo; +import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get; +import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.post; +import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.jsonPath; +import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status; + +import com.company.resourceapi.clients.ExternalProject; +import com.company.resourceapi.controllers.ProjectRestController; +import com.company.resourceapi.entities.Project; +import com.company.resourceapi.entities.SdlcSystem; +import com.company.resourceapi.repositories.ProjectRepository; +import com.company.resourceapi.repositories.SdlcSystemRepository; +import com.company.resourceapi.services.SdlcSystemService; +import com.github.tomakehurst.wiremock.client.WireMock; +import com.github.tomakehurst.wiremock.junit5.WireMockExtension; +import java.time.Duration; +import java.time.Instant; +import java.util.function.BooleanSupplier; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.RegisterExtension; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.boot.test.context.SpringBootTest; +import org.springframework.boot.webmvc.test.autoconfigure.AutoConfigureMockMvc; +import org.springframework.beans.factory.annotation.Value; +import org.springframework.cache.CacheManager; +import org.springframework.data.redis.core.StringRedisTemplate; +import org.springframework.test.web.servlet.MockMvc; + +/** Redis cache in front of the SDLC systems, against a real Redis (RedisTestContainerInitializer). */ +@SpringBootTest +@AutoConfigureMockMvc +class CacheIntegrationTest { + + private static final String EXTERNAL_ID = "CACHED"; + private static final String SDLC_PROJECT_URL = "/rest/api/2/project/" + EXTERNAL_ID; + + @RegisterExtension + static WireMockExtension sdlcSystemServer = WireMockExtension.newInstance() + .options(wireMockConfig().dynamicPort()) + .build(); + + @Autowired + private MockMvc mockMvc; + + @Autowired + private SdlcSystemRepository sdlcSystemRepository; + + @Autowired + private ProjectRepository projectRepository; + + @Autowired + private SdlcSystemService sdlcSystemService; + + @Autowired + private CacheManager cacheManager; + + @Autowired + private StringRedisTemplate redisTemplate; + + @Value("${spring.cache.redis.key-prefix}") + private String keyPrefix; + + private SdlcSystem sdlcSystem; + private long projectId; + + @BeforeEach + void setup() { + sdlcSystem = new SdlcSystem(); + sdlcSystem.setBaseUrl(sdlcSystemServer.baseUrl()); + sdlcSystem = sdlcSystemRepository.save(sdlcSystem); + + var project = new Project(); + project.setExternalId(EXTERNAL_ID); + project.setName("Local name"); + project.setSdlcSystem(sdlcSystem); + projectId = projectRepository.save(project).getId(); + } + + @Test + void givenRepeatedLookupsThenSdlcSystemIsAskedOnce() throws Exception { + sdlcSystemServer.stubFor(WireMock.get(urlEqualTo(SDLC_PROJECT_URL)) + .willReturn(okJson("{\"key\": \"CACHED\", \"name\": \"From SDLC\"}"))); + + for (int i = 0; i < 3; i++) { + mockMvc.perform(get(externalUrl())) + .andExpect(status().isOk()) + .andExpect(jsonPath("$.name", equalTo("From SDLC"))); + } + + sdlcSystemServer.verify(1, getRequestedFor(urlEqualTo(SDLC_PROJECT_URL))); + // stored as JSON in Redis and read back as the record, not a map + assertThat(cacheManager.getCache(EXTERNAL_PROJECTS).get(externalKey(), ExternalProject.class)) + .isEqualTo(new ExternalProject("CACHED", "From SDLC", null)); + } + + @Test + void givenProjectUnknownToSdlcSystemThenItIsNotCached() throws Exception { + sdlcSystemServer.stubFor(WireMock.get(urlEqualTo(SDLC_PROJECT_URL)) + .willReturn(notFound().withHeader("Content-Type", "application/json").withBody("{}"))); + + mockMvc.perform(get(externalUrl())).andExpect(status().isNotFound()); + mockMvc.perform(get(externalUrl())).andExpect(status().isNotFound()); + + // asked again, so the project is found as soon as it exists in the SDLC system + sdlcSystemServer.verify(2, getRequestedFor(urlEqualTo(SDLC_PROJECT_URL))); + assertThat(cacheManager.getCache(EXTERNAL_PROJECTS).get(externalKey())).isNull(); + } + + @Test + void givenSyncThenItBypassesAndRefreshesTheCache() throws Exception { + sdlcSystemServer.stubFor(WireMock.get(urlEqualTo(SDLC_PROJECT_URL)) + .willReturn(okJson("{\"key\": \"CACHED\", \"name\": \"Old name\"}"))); + mockMvc.perform(get(externalUrl())).andExpect(jsonPath("$.name", equalTo("Old name"))); + + // the SDLC system renames the project; a sync must not use the cached answer + sdlcSystemServer.stubFor(WireMock.get(urlEqualTo(SDLC_PROJECT_URL)) + .willReturn(okJson("{\"key\": \"CACHED\", \"name\": \"New name\"}"))); + mockMvc.perform(post(ProjectRestController.ENDPOINT + "/" + projectId + "/sync")) + .andExpect(status().isAccepted()); + await(() -> projectRepository.findById(projectId).orElseThrow().getName().equals("New name")); + + // ... and the cache now has the fresh answer, without another call + mockMvc.perform(get(externalUrl())).andExpect(jsonPath("$.name", equalTo("New name"))); + sdlcSystemServer.verify(2, getRequestedFor(urlEqualTo(SDLC_PROJECT_URL))); + } + + @Test + void givenSdlcSystemLookupThenItIsServedFromTheCache() throws Exception { + var first = sdlcSystemService.getSdlcSystem(sdlcSystem.getId()); + + // changed behind the API's back: the cached value is used until it expires + sdlcSystem.setDescription("Changed in the database"); + sdlcSystemRepository.save(sdlcSystem); + var second = sdlcSystemService.getSdlcSystem(sdlcSystem.getId()); + + assertThat(second).isNotSameAs(first); + assertThat(second.getBaseUrl()).isEqualTo(sdlcSystemServer.baseUrl()); + assertThat(second.getDescription()).isNull(); + assertThat(cacheManager.getCache(SDLC_SYSTEMS).get(sdlcSystem.getId(), SdlcSystem.class)).isNotNull(); + } + + private String externalUrl() { + return ProjectRestController.ENDPOINT + "/" + projectId + "/external"; + } + + private String externalKey() { + return sdlcSystem.getId() + ":" + EXTERNAL_ID; + } + + private static void await(BooleanSupplier condition) throws InterruptedException { + var deadline = Instant.now().plus(Duration.ofSeconds(5)); + while (!condition.getAsBoolean()) { + assertThat(Instant.now()).as("condition not met within 5s").isBefore(deadline); + Thread.sleep(50); + } + } + + @Test + void givenCachedValuesThenRedisKeysCarryTheConfiguredPrefixAndTtl() throws Exception { + sdlcSystemService.getSdlcSystem(sdlcSystem.getId()); + + var keys = redisTemplate.keys("*" + SDLC_SYSTEMS + "::" + sdlcSystem.getId()); + // this context's prefix (RedisTestContainerInitializer), so contexts don't share entries + assertThat(keys).singleElement().asString().startsWith(keyPrefix + SDLC_SYSTEMS + "::"); + assertThat(redisTemplate.getExpire(keys.iterator().next())).isBetween(1L, 600L); + } +} diff --git a/src/test/java/com/company/resourceapi/cache/CacheUnavailableTest.java b/src/test/java/com/company/resourceapi/cache/CacheUnavailableTest.java new file mode 100644 index 0000000..34e6225 --- /dev/null +++ b/src/test/java/com/company/resourceapi/cache/CacheUnavailableTest.java @@ -0,0 +1,72 @@ +package com.company.resourceapi.cache; + +import static com.github.tomakehurst.wiremock.client.WireMock.okJson; +import static com.github.tomakehurst.wiremock.client.WireMock.urlEqualTo; +import static com.github.tomakehurst.wiremock.core.WireMockConfiguration.wireMockConfig; +import static org.assertj.core.api.Assertions.assertThat; +import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get; +import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.post; +import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status; + +import com.company.resourceapi.controllers.ProjectRestController; +import com.company.resourceapi.entities.Project; +import com.company.resourceapi.entities.SdlcSystem; +import com.company.resourceapi.repositories.ProjectRepository; +import com.company.resourceapi.repositories.SdlcSystemRepository; +import com.github.tomakehurst.wiremock.client.WireMock; +import com.github.tomakehurst.wiremock.junit5.WireMockExtension; +import java.time.Duration; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.RegisterExtension; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.boot.test.context.SpringBootTest; +import org.springframework.boot.webmvc.test.autoconfigure.AutoConfigureMockMvc; +import org.springframework.http.MediaType; +import org.springframework.test.web.servlet.MockMvc; + +/** + * The cache is an optimization: with Redis unreachable the API keeps working, just uncached, and + * requests stay fast thanks to the short Redis timeouts in application.properties. + */ +@SpringBootTest(properties = "spring.data.redis.port=1") +@AutoConfigureMockMvc +class CacheUnavailableTest { + + @RegisterExtension + static WireMockExtension sdlcSystemServer = WireMockExtension.newInstance() + .options(wireMockConfig().dynamicPort()) + .build(); + + @Autowired + private MockMvc mockMvc; + + @Autowired + private SdlcSystemRepository sdlcSystemRepository; + + @Autowired + private ProjectRepository projectRepository; + + @Test + void givenRedisDownWhenUsingCachedEndpointsThenTheyStillWork() throws Exception { + var sdlcSystem = new SdlcSystem(); + sdlcSystem.setBaseUrl(sdlcSystemServer.baseUrl()); + sdlcSystem = sdlcSystemRepository.save(sdlcSystem); + var project = new Project(); + project.setExternalId("NOREDIS"); + project.setSdlcSystem(sdlcSystem); + long projectId = projectRepository.save(project).getId(); + sdlcSystemServer.stubFor(WireMock.get(urlEqualTo("/rest/api/2/project/NOREDIS")) + .willReturn(okJson("{\"key\": \"NOREDIS\"}"))); + + var start = System.nanoTime(); + mockMvc.perform(get(ProjectRestController.ENDPOINT + "/" + projectId + "/external")) + .andExpect(status().isOk()); + mockMvc.perform(post(ProjectRestController.ENDPOINT) + .contentType(MediaType.APPLICATION_JSON) + .content("{\"externalId\": \"NOREDIS-2\", \"sdlcSystem\": {\"id\": " + sdlcSystem.getId() + "}}")) + .andExpect(status().isCreated()); + + // each skipped cache read/write is bounded by the Redis timeouts, not Lettuce's 60s default + assertThat(Duration.ofNanos(System.nanoTime() - start)).isLessThan(Duration.ofSeconds(10)); + } +} diff --git a/src/test/java/com/company/resourceapi/controllers/ProjectRestControllerConflictTest.java b/src/test/java/com/company/resourceapi/controllers/ProjectRestControllerConflictTest.java index 3e01367..464dd07 100644 --- a/src/test/java/com/company/resourceapi/controllers/ProjectRestControllerConflictTest.java +++ b/src/test/java/com/company/resourceapi/controllers/ProjectRestControllerConflictTest.java @@ -7,8 +7,8 @@ import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.post; import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status; -import com.company.resourceapi.clients.SdlcProjectClient; import com.company.resourceapi.entities.SdlcSystem; +import com.company.resourceapi.services.ExternalProjectService; import com.company.resourceapi.services.ProjectService; import com.company.resourceapi.services.SdlcSystemService; import java.sql.SQLException; @@ -36,7 +36,7 @@ class ProjectRestControllerConflictTest { private SdlcSystemService sdlcSystemService; @MockitoBean - private SdlcProjectClient sdlcProjectClient; + private ExternalProjectService externalProjectService; @Test void givenConcurrentDuplicateWhenPostThenReturnStatusConflict() throws Exception { diff --git a/src/test/java/com/company/resourceapi/controllers/ProjectRestControllerSdlcFailureTest.java b/src/test/java/com/company/resourceapi/controllers/ProjectRestControllerSdlcFailureTest.java index f6a7c5e..ebedcc2 100644 --- a/src/test/java/com/company/resourceapi/controllers/ProjectRestControllerSdlcFailureTest.java +++ b/src/test/java/com/company/resourceapi/controllers/ProjectRestControllerSdlcFailureTest.java @@ -7,9 +7,9 @@ import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.header; import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status; -import com.company.resourceapi.clients.SdlcProjectClient; import com.company.resourceapi.entities.Project; import com.company.resourceapi.entities.SdlcSystem; +import com.company.resourceapi.services.ExternalProjectService; import com.company.resourceapi.services.ProjectService; import com.company.resourceapi.services.SdlcSystemService; import java.util.Optional; @@ -35,7 +35,7 @@ class ProjectRestControllerSdlcFailureTest { private SdlcSystemService sdlcSystemService; @MockitoBean - private SdlcProjectClient sdlcProjectClient; + private ExternalProjectService externalProjectService; @BeforeEach void setup() throws Exception { @@ -47,7 +47,7 @@ void setup() throws Exception { @Test void givenExhaustedConnectionPoolWhenGetExternalThenReturnStatusServiceUnavailable() throws Exception { - when(sdlcProjectClient.fetchProject(any(), anyString())).thenThrow(new ResourceAccessException( + when(externalProjectService.findProject(any(), anyString())).thenThrow(new ResourceAccessException( "I/O error", new ConnectionRequestTimeoutException("Timeout deadline: 2000 MILLISECONDS"))); mockMvc.perform(get(ProjectRestController.ENDPOINT + "/1/external")) @@ -58,7 +58,7 @@ void givenExhaustedConnectionPoolWhenGetExternalThenReturnStatusServiceUnavailab @Test void givenNoTracerWhenHandledThenNoTraceIdHeader() throws Exception { // this web slice loads no tracing: the filter must not send an empty trace id - when(sdlcProjectClient.fetchProject(any(), anyString())).thenReturn(Optional.empty()); + when(externalProjectService.findProject(any(), anyString())).thenReturn(Optional.empty()); mockMvc.perform(get(ProjectRestController.ENDPOINT + "/1/external")) .andExpect(header().doesNotExist("X-Trace-Id")); @@ -66,7 +66,7 @@ void givenNoTracerWhenHandledThenNoTraceIdHeader() throws Exception { @Test void givenUnreachableSdlcSystemWhenGetExternalThenReturnStatusBadGateway() throws Exception { - when(sdlcProjectClient.fetchProject(any(), anyString())) + when(externalProjectService.findProject(any(), anyString())) .thenThrow(new ResourceAccessException("I/O error", new java.net.ConnectException("refused"))); mockMvc.perform(get(ProjectRestController.ENDPOINT + "/1/external")) diff --git a/src/test/resources/META-INF/spring.factories b/src/test/resources/META-INF/spring.factories new file mode 100644 index 0000000..5256c02 --- /dev/null +++ b/src/test/resources/META-INF/spring.factories @@ -0,0 +1,2 @@ +org.springframework.context.ApplicationContextInitializer=\ +com.company.resourceapi.RedisTestContainerInitializer From 46c3b26e81aff71b3be426fe0990c8f98e093481 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 31 Oct 2026 10:01:58 +0000 Subject: [PATCH 02/13] Address review: evict on unknown, keep Boot's cache defaults, narrower typing - A sync that finds the project gone from its SDLC system evicts the cached answer (late @CacheEvict sees the raw Optional as #result) - Start from Boot's cache defaults (spring.cache.redis.*) and only replace the value serializer, instead of re-implementing them in a RedisCacheConfiguration bean; null values disabled by property - Type information in cached JSON limited to this application's classes, java.lang and java.time (not all of java.*) - sdlc-systems uses sync=true, so concurrent misses do one database read - compose.yaml publishes Redis on localhost:6379 for java -jar - Test: sync of a project gone upstream removes the entry; the key-prefix test matches this context's prefix exactly Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_012MLyPXGHeCc1fvTeB8JnT2 --- README.md | 7 +-- compose.yaml | 2 +- .../resourceapi/config/CacheConfig.java | 47 ++++++------------- .../services/ExternalProjectService.java | 14 +++++- .../services/impl/SdlcSystemServiceImpl.java | 7 ++- src/main/resources/application.properties | 1 + .../cache/CacheIntegrationTest.java | 18 ++++++- 7 files changed, 55 insertions(+), 41 deletions(-) diff --git a/README.md b/README.md index ce69c34..c714d08 100644 --- a/README.md +++ b/README.md @@ -81,14 +81,15 @@ The application uses Redis (`spring.data.redis.host`/`port`, default `localhost: | `sdlc-systems` | SDLC systems by id, read on every create, update and SDLC call | `resourceapi.cache.sdlc-systems-ttl` (10m) | | `external-projects` | `GET /api/v2/projects/{id}/external` answers per SDLC system and external id; "unknown" answers are not cached | `resourceapi.cache.external-projects-ttl` (5m) | -A background sync always asks the SDLC system and refreshes the cached answer. SDLC systems are not changed +A background sync always asks the SDLC system and refreshes the cached answer (or removes it when the SDLC +system no longer knows the project). SDLC systems are not changed through this API; one changed directly in the database is picked up when its entry expires. The cache is an optimization: if Redis is unreachable, cache reads and writes are logged and skipped (each waits at most the 500 ms Redis timeouts) and requests are served without it. **Local development**: `./mvnw spring-boot:run` starts Redis from `compose.yaml` automatically -(`spring-boot-docker-compose`); with `java -jar`, start it yourself with `docker compose up -d` and point -`spring.data.redis.port` at it. Tests start their own Redis with Testcontainers, so they need Docker. +(`spring-boot-docker-compose`); with `java -jar`, start it yourself with `docker compose up -d` (it listens on +`localhost:6379`). Tests start their own Redis with Testcontainers, so they need Docker. ## Tracing Every request gets a trace id ([Micrometer Tracing](https://micrometer.io/docs/tracing) with Brave, W3C trace context): diff --git a/compose.yaml b/compose.yaml index 1824de0..942b34f 100644 --- a/compose.yaml +++ b/compose.yaml @@ -4,4 +4,4 @@ services: redis: image: redis:8-alpine ports: - - "6379" + - "6379:6379" diff --git a/src/main/java/com/company/resourceapi/config/CacheConfig.java b/src/main/java/com/company/resourceapi/config/CacheConfig.java index 2e622cc..e4a80d4 100644 --- a/src/main/java/com/company/resourceapi/config/CacheConfig.java +++ b/src/main/java/com/company/resourceapi/config/CacheConfig.java @@ -1,6 +1,5 @@ package com.company.resourceapi.config; -import org.springframework.boot.cache.autoconfigure.CacheProperties; import org.springframework.boot.cache.autoconfigure.RedisCacheManagerBuilderCustomizer; import org.springframework.boot.context.properties.EnableConfigurationProperties; import org.springframework.cache.annotation.CachingConfigurer; @@ -9,59 +8,43 @@ import org.springframework.cache.interceptor.LoggingCacheErrorHandler; import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.Configuration; -import org.springframework.data.redis.cache.RedisCacheConfiguration; import org.springframework.data.redis.serializer.GenericJacksonJsonRedisSerializer; import org.springframework.data.redis.serializer.RedisSerializationContext.SerializationPair; import tools.jackson.databind.jsontype.BasicPolymorphicTypeValidator; /** - * Redis-backed Spring Cache. Values are stored as JSON with their type, limited to this - * application's and the JDK's classes. A cache that can't be reached is logged and skipped, so - * Redis being down makes requests slower, not fail. + * Redis-backed Spring Cache. Values are stored as JSON with their type, limited to the classes the + * cached values consist of. A cache that can't be reached is logged and skipped, so Redis being down + * makes requests slower, not fail. */ @Configuration @EnableCaching -@EnableConfigurationProperties({CacheProperties.class, CacheTtlProperties.class}) +@EnableConfigurationProperties(CacheTtlProperties.class) public class CacheConfig implements CachingConfigurer { public static final String SDLC_SYSTEMS = "sdlc-systems"; public static final String EXTERNAL_PROJECTS = "external-projects"; /** - * Defining this bean replaces Boot's own, so Boot's spring.cache.redis.* settings are applied - * here (otherwise e.g. the key prefix would silently be ignored). + * Starts from Boot's defaults (spring.cache.redis.*: key prefix, null values, ...) and only + * replaces the value serializer and sets each cache's time to live. */ @Bean - RedisCacheConfiguration redisCacheConfiguration(CacheProperties cacheProperties) { + RedisCacheManagerBuilderCustomizer redisCacheCustomizer(CacheTtlProperties ttl) { var typeValidator = BasicPolymorphicTypeValidator.builder() .allowIfSubType("com.company.resourceapi.") - .allowIfSubType("java.") + .allowIfSubType("java.lang.") + .allowIfSubType("java.time.") .build(); var serializer = GenericJacksonJsonRedisSerializer.builder() .enableDefaultTyping(typeValidator) .build(); - var config = RedisCacheConfiguration.defaultCacheConfig() - .serializeValuesWith(SerializationPair.fromSerializer(serializer)) - .disableCachingNullValues(); - - var redis = cacheProperties.getRedis(); - if (redis.getTimeToLive() != null) { - config = config.entryTtl(redis.getTimeToLive()); - } - if (redis.getKeyPrefix() != null) { - config = config.prefixCacheNameWith(redis.getKeyPrefix()); - } - if (!redis.isUseKeyPrefix()) { - config = config.disableKeyPrefix(); - } - return config; - } - - @Bean - RedisCacheManagerBuilderCustomizer cacheTimeToLive(RedisCacheConfiguration defaults, CacheTtlProperties ttl) { - return builder -> builder - .withCacheConfiguration(SDLC_SYSTEMS, defaults.entryTtl(ttl.sdlcSystemsTtl())) - .withCacheConfiguration(EXTERNAL_PROJECTS, defaults.entryTtl(ttl.externalProjectsTtl())); + return builder -> { + var defaults = builder.cacheDefaults().serializeValuesWith(SerializationPair.fromSerializer(serializer)); + builder.cacheDefaults(defaults) + .withCacheConfiguration(SDLC_SYSTEMS, defaults.entryTtl(ttl.sdlcSystemsTtl())) + .withCacheConfiguration(EXTERNAL_PROJECTS, defaults.entryTtl(ttl.externalProjectsTtl())); + }; } @Override diff --git a/src/main/java/com/company/resourceapi/services/ExternalProjectService.java b/src/main/java/com/company/resourceapi/services/ExternalProjectService.java index 41ed2c5..415c1fc 100644 --- a/src/main/java/com/company/resourceapi/services/ExternalProjectService.java +++ b/src/main/java/com/company/resourceapi/services/ExternalProjectService.java @@ -7,8 +7,10 @@ import com.company.resourceapi.entities.SdlcSystem; import java.util.Optional; import lombok.RequiredArgsConstructor; +import org.springframework.cache.annotation.CacheEvict; import org.springframework.cache.annotation.CachePut; import org.springframework.cache.annotation.Cacheable; +import org.springframework.cache.annotation.Caching; import org.springframework.stereotype.Service; /** Projects as described by their SDLC system, cached per SDLC system and external id. */ @@ -24,8 +26,16 @@ public Optional findProject(SdlcSystem sdlcSystem, String exter return sdlcProjectClient.fetchProject(sdlcSystem, externalId); } - /** Always asks the SDLC system, and puts the fresh answer in the cache. */ - @CachePut(cacheNames = EXTERNAL_PROJECTS, key = "#sdlcSystem.id + ':' + #externalId", unless = "#result == null") + /** + * Always asks the SDLC system and updates the cache with the answer: a found project replaces the + * cached one, a project the SDLC system no longer knows removes it. + */ + @Caching( + put = @CachePut(cacheNames = EXTERNAL_PROJECTS, key = "#sdlcSystem.id + ':' + #externalId", + unless = "#result == null"), + // unlike for @CachePut, #result is the Optional itself here (late evictions get the raw return value) + evict = @CacheEvict(cacheNames = EXTERNAL_PROJECTS, key = "#sdlcSystem.id + ':' + #externalId", + condition = "#result == null or #result.isEmpty()")) public Optional refreshProject(SdlcSystem sdlcSystem, String externalId) { return sdlcProjectClient.fetchProject(sdlcSystem, externalId); } diff --git a/src/main/java/com/company/resourceapi/services/impl/SdlcSystemServiceImpl.java b/src/main/java/com/company/resourceapi/services/impl/SdlcSystemServiceImpl.java index 95f9746..7b783ae 100755 --- a/src/main/java/com/company/resourceapi/services/impl/SdlcSystemServiceImpl.java +++ b/src/main/java/com/company/resourceapi/services/impl/SdlcSystemServiceImpl.java @@ -16,8 +16,11 @@ public class SdlcSystemServiceImpl implements SdlcSystemService { private final SdlcSystemRepository sdlcSystemRepository; - /** Cached: SDLC systems are read on every create, update and SDLC call, and rarely change. */ - @Cacheable(SDLC_SYSTEMS) + /** + * Cached: SDLC systems are read on every create, update and SDLC call, and rarely change. When an + * entry expires, concurrent callers wait for one database read instead of each doing their own. + */ + @Cacheable(cacheNames = SDLC_SYSTEMS, sync = true) public SdlcSystem getSdlcSystem(long id) throws NotFoundException { return sdlcSystemRepository.findById(id) .orElseThrow(() -> new NotFoundException(SdlcSystem.class, id)); diff --git a/src/main/resources/application.properties b/src/main/resources/application.properties index 1d9cde7..150cb61 100755 --- a/src/main/resources/application.properties +++ b/src/main/resources/application.properties @@ -18,6 +18,7 @@ spring.task.execution.shutdown.await-termination-period=10s spring.data.redis.connect-timeout=500ms spring.data.redis.timeout=500ms spring.cache.redis.key-prefix=resourceapi:: +spring.cache.redis.cache-null-values=false resourceapi.cache.sdlc-systems-ttl=10m resourceapi.cache.external-projects-ttl=5m # datasource diff --git a/src/test/java/com/company/resourceapi/cache/CacheIntegrationTest.java b/src/test/java/com/company/resourceapi/cache/CacheIntegrationTest.java index 72c91df..7fa5fff 100644 --- a/src/test/java/com/company/resourceapi/cache/CacheIntegrationTest.java +++ b/src/test/java/com/company/resourceapi/cache/CacheIntegrationTest.java @@ -170,9 +170,25 @@ private static void await(BooleanSupplier condition) throws InterruptedException void givenCachedValuesThenRedisKeysCarryTheConfiguredPrefixAndTtl() throws Exception { sdlcSystemService.getSdlcSystem(sdlcSystem.getId()); - var keys = redisTemplate.keys("*" + SDLC_SYSTEMS + "::" + sdlcSystem.getId()); + var keys = redisTemplate.keys(keyPrefix + SDLC_SYSTEMS + "::" + sdlcSystem.getId()); // this context's prefix (RedisTestContainerInitializer), so contexts don't share entries assertThat(keys).singleElement().asString().startsWith(keyPrefix + SDLC_SYSTEMS + "::"); assertThat(redisTemplate.getExpire(keys.iterator().next())).isBetween(1L, 600L); } + + @Test + void givenProjectGoneFromSdlcSystemWhenSyncedThenCachedEntryIsRemoved() throws Exception { + sdlcSystemServer.stubFor(WireMock.get(urlEqualTo(SDLC_PROJECT_URL)) + .willReturn(okJson("{\"key\": \"CACHED\", \"name\": \"Soon gone\"}"))); + mockMvc.perform(get(externalUrl())).andExpect(status().isOk()); + + // deleted in the SDLC system; the sync finds out + sdlcSystemServer.stubFor(WireMock.get(urlEqualTo(SDLC_PROJECT_URL)) + .willReturn(notFound().withHeader("Content-Type", "application/json").withBody("{}"))); + mockMvc.perform(post(ProjectRestController.ENDPOINT + "/" + projectId + "/sync")) + .andExpect(status().isAccepted()); + await(() -> cacheManager.getCache(EXTERNAL_PROJECTS).get(externalKey()) == null); + + mockMvc.perform(get(externalUrl())).andExpect(status().isNotFound()); + } } From 762ceeb63f40283e0a2c3c08c485a40d7956fe34 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 31 Oct 2026 10:11:17 +0000 Subject: [PATCH 03/13] Add distributed lock: one sync per project across instances (Redisson) - Redisson client (plain library; its Boot starter targets Boot 3) on the same Redis connection details as the cache, connecting lazily so the app starts without Redis; object names prefixed (resourceapi.redisson.key-prefix) - ProjectSyncLock: per-project RLock taken by POST /sync and released when the background sync ends (held by a random owner id, as it is released on another thread); expires after resourceapi.sync.lock-lease-time (60s); a concurrent sync gets 409; if Redis is unreachable the sync runs unlocked - spring.cache.type=redis: Redisson brings a JCache provider that Boot would otherwise pick over Redis - Tests (real Redis): lock held elsewhere -> 409 and no SDLC call; slow sync -> second refused, then allowed once the first released (verified to fail without the release); abandoned lock expires; keys carry the context's prefix; Redis down -> sync still runs (verified to fail without fail-open) - Backpressure test uses two projects (the lock refuses the same one first) Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_012MLyPXGHeCc1fvTeB8JnT2 --- README.md | 8 + pom.xml | 7 + .../resourceapi/config/RedissonConfig.java | 63 ++++++++ .../controllers/ProjectSyncController.java | 19 ++- .../exceptions/SyncInProgressException.java | 8 + .../resourceapi/services/ProjectSyncLock.java | 68 ++++++++ .../resourceapi/web/ApiExceptionHandler.java | 9 ++ src/main/resources/application.properties | 6 + .../RedisTestContainerInitializer.java | 4 +- .../cache/CacheUnavailableTest.java | 22 +++ .../lock/ProjectSyncLockIntegrationTest.java | 153 ++++++++++++++++++ .../services/ProjectSyncBackpressureTest.java | 21 ++- 12 files changed, 378 insertions(+), 10 deletions(-) create mode 100644 src/main/java/com/company/resourceapi/config/RedissonConfig.java create mode 100644 src/main/java/com/company/resourceapi/exceptions/SyncInProgressException.java create mode 100644 src/main/java/com/company/resourceapi/services/ProjectSyncLock.java create mode 100644 src/test/java/com/company/resourceapi/lock/ProjectSyncLockIntegrationTest.java diff --git a/README.md b/README.md index c714d08..3012c81 100644 --- a/README.md +++ b/README.md @@ -87,6 +87,14 @@ through this API; one changed directly in the database is picked up when its ent optimization: if Redis is unreachable, cache reads and writes are logged and skipped (each waits at most the 500 ms Redis timeouts) and requests are served without it. +**Lock** ([Redisson](https://redisson.org/) `RLock`): at most one `POST /api/v2/projects/{id}/sync` per project +runs at a time, across all instances; a second one gets `409 Conflict` until the first ends. The lock is +released when the background sync ends and expires after `resourceapi.sync.lock-lease-time` (60s) in case +an instance dies mid-sync. If Redis can't be reached, the sync runs without the lock (a duplicate sync only +costs an extra SDLC call). Redisson connects lazily and uses the same Redis connection settings; its keys +are prefixed with `resourceapi.redisson.key-prefix`. (Redisson's Spring Boot starter isn't used: it targets +Spring Boot 3. Redisson brings a JCache provider, so `spring.cache.type=redis` is set explicitly.) + **Local development**: `./mvnw spring-boot:run` starts Redis from `compose.yaml` automatically (`spring-boot-docker-compose`); with `java -jar`, start it yourself with `docker compose up -d` (it listens on `localhost:6379`). Tests start their own Redis with Testcontainers, so they need Docker. diff --git a/pom.xml b/pom.xml index 0105df1..98f9ef3 100644 --- a/pom.xml +++ b/pom.xml @@ -17,6 +17,7 @@ 25 3.2.0 + 4.8.0 3.13.2 0.8.16 @@ -58,6 +59,12 @@ org.springframework.boot spring-boot-starter-cache + + + org.redisson + redisson + ${redisson.version} + org.apache.httpcomponents.client5 diff --git a/src/main/java/com/company/resourceapi/config/RedissonConfig.java b/src/main/java/com/company/resourceapi/config/RedissonConfig.java new file mode 100644 index 0000000..ac22be6 --- /dev/null +++ b/src/main/java/com/company/resourceapi/config/RedissonConfig.java @@ -0,0 +1,63 @@ +package com.company.resourceapi.config; + +import java.time.Duration; +import org.redisson.Redisson; +import org.redisson.api.RedissonClient; +import org.redisson.config.Config; +import org.redisson.config.NameMapper; +import org.springframework.beans.factory.annotation.Value; +import org.springframework.boot.data.redis.autoconfigure.DataRedisConnectionDetails; +import org.springframework.boot.data.redis.autoconfigure.DataRedisProperties; +import org.springframework.context.annotation.Bean; +import org.springframework.context.annotation.Configuration; + +/** + * Redisson client for distributed locks and rate limiters, connected to the same Redis as the cache + * (Boot's connection details, so Docker Compose and Testcontainers connections work too). + * + *

It connects lazily, on first use: the application starts without Redis (for example the AOT + * training run while building an image), and Redis being down only affects the calls that need it. + */ +@Configuration +public class RedissonConfig { + + @Bean(destroyMethod = "shutdown") + RedissonClient redissonClient(DataRedisConnectionDetails connection, DataRedisProperties redis, + @Value("${resourceapi.redisson.key-prefix}") String keyPrefix) { + var standalone = connection.getStandalone(); + if (standalone == null) { + throw new IllegalStateException("Only a standalone Redis is supported for Redisson"); + } + var config = new Config().setLazyInitialization(true); + config.useSingleServer() + .setAddress((connection.getSslBundle() != null ? "rediss://" : "redis://") + + standalone.getHost() + ":" + standalone.getPort()) + .setDatabase(standalone.getDatabase()) + .setUsername(connection.getUsername()) + .setPassword(connection.getPassword()) + .setConnectTimeout(millis(redis.getConnectTimeout(), Duration.ofSeconds(10))) + .setTimeout(millis(redis.getTimeout(), Duration.ofSeconds(3))) + // fail fast like the cache: callers decide what to do without Redis + .setRetryAttempts(1) + .setNameMapper(new KeyPrefixNameMapper(keyPrefix)); + return Redisson.create(config); + } + + private static int millis(Duration value, Duration fallback) { + return (int) (value != null ? value : fallback).toMillis(); + } + + /** Prefixes every Redisson object name (locks, rate limiters) in Redis. */ + record KeyPrefixNameMapper(String prefix) implements NameMapper { + + @Override + public String map(String name) { + return prefix + name; + } + + @Override + public String unmap(String name) { + return name.startsWith(prefix) ? name.substring(prefix.length()) : name; + } + } +} diff --git a/src/main/java/com/company/resourceapi/controllers/ProjectSyncController.java b/src/main/java/com/company/resourceapi/controllers/ProjectSyncController.java index e49b860..38f6672 100644 --- a/src/main/java/com/company/resourceapi/controllers/ProjectSyncController.java +++ b/src/main/java/com/company/resourceapi/controllers/ProjectSyncController.java @@ -5,7 +5,9 @@ import static com.company.resourceapi.controllers.ProjectRestController.PATH_VARIABLE_ID; import com.company.resourceapi.exceptions.NotFoundException; +import com.company.resourceapi.exceptions.SyncInProgressException; import com.company.resourceapi.services.ProjectService; +import com.company.resourceapi.services.ProjectSyncLock; import com.company.resourceapi.services.ProjectSyncService; import io.swagger.v3.oas.annotations.Operation; import io.swagger.v3.oas.annotations.Parameter; @@ -25,16 +27,27 @@ public class ProjectSyncController { private final ProjectService projectService; private final ProjectSyncService projectSyncService; + private final ProjectSyncLock projectSyncLock; @Operation(summary = "Refresh a Project's name from its SDLC system, in the background") @PostMapping(ENDPOINT_ID + "/sync") public ResponseEntity syncProject( @Parameter(name = PATH_VARIABLE_ID, description = "ID", required = true) @PathVariable(PATH_VARIABLE_ID) final long projectId) throws NotFoundException { - // an unknown project is reported now, not only in the background (404 via ApiExceptionHandler); - // a full task executor rejects the sync (503 via ApiExceptionHandler) + // an unknown project is reported now, not only in the background (404 via ApiExceptionHandler) projectService.getProject(projectId); - projectSyncService.syncName(projectId); + + // one sync per project at a time, across instances (409 via ApiExceptionHandler); the background + // task releases the lock when it ends + var lease = projectSyncLock.tryAcquire(projectId) + .orElseThrow(() -> new SyncInProgressException(projectId)); + try { + projectSyncService.syncName(projectId).whenComplete((result, failure) -> lease.release()); + } catch (RuntimeException e) { + // not started, e.g. a full task executor (503 via ApiExceptionHandler) + lease.release(); + throw e; + } return ResponseEntity.accepted().build(); } } diff --git a/src/main/java/com/company/resourceapi/exceptions/SyncInProgressException.java b/src/main/java/com/company/resourceapi/exceptions/SyncInProgressException.java new file mode 100644 index 0000000..d715b7c --- /dev/null +++ b/src/main/java/com/company/resourceapi/exceptions/SyncInProgressException.java @@ -0,0 +1,8 @@ +package com.company.resourceapi.exceptions; + +public class SyncInProgressException extends RuntimeException { + + public SyncInProgressException(long projectId) { + super("A sync of project %d is already running.".formatted(projectId)); + } +} diff --git a/src/main/java/com/company/resourceapi/services/ProjectSyncLock.java b/src/main/java/com/company/resourceapi/services/ProjectSyncLock.java new file mode 100644 index 0000000..820d721 --- /dev/null +++ b/src/main/java/com/company/resourceapi/services/ProjectSyncLock.java @@ -0,0 +1,68 @@ +package com.company.resourceapi.services; + +import java.time.Duration; +import java.util.Optional; +import java.util.concurrent.ThreadLocalRandom; +import java.util.concurrent.TimeUnit; +import lombok.extern.slf4j.Slf4j; +import org.redisson.api.RLock; +import org.redisson.api.RedissonClient; +import org.springframework.beans.factory.annotation.Value; +import org.springframework.stereotype.Component; + +/** + * At most one sync per project at a time, across all instances (a Redisson lock per project). + * + *

The lock is taken by the request and released by the background task, on another thread, so it + * is held by a random owner id instead of a thread. It expires after the lease time, so an instance + * that dies mid-sync doesn't block the project forever. If Redis can't be reached the sync runs + * unlocked: a duplicate sync only costs an extra call, while refusing it would stop syncs entirely. + */ +@Component +@Slf4j +public class ProjectSyncLock { + + private final RedissonClient redisson; + private final Duration leaseTime; + + public ProjectSyncLock(RedissonClient redisson, @Value("${resourceapi.sync.lock-lease-time}") Duration leaseTime) { + this.redisson = redisson; + this.leaseTime = leaseTime; + } + + /** @return the lease to release when the sync ends, or empty if a sync of the project is running */ + public Optional tryAcquire(long projectId) { + var lock = lock(projectId); + long ownerId = ThreadLocalRandom.current().nextLong(); + try { + boolean acquired = lock.tryLockAsync(0, leaseTime.toMillis(), TimeUnit.MILLISECONDS, ownerId) + .toCompletableFuture().join(); + return acquired ? Optional.of(new Lease(lock, ownerId)) : Optional.empty(); + } catch (RuntimeException e) { + log.warn("Sync lock for project {} unavailable, syncing without it: {}", projectId, e.getMessage()); + return Optional.of(Lease.NONE); + } + } + + private RLock lock(long projectId) { + return redisson.getLock("project-sync:" + projectId); + } + + /** A held lock; releasing it twice, or after it expired, is harmless. */ + public record Lease(RLock lock, long ownerId) { + + static final Lease NONE = new Lease(null, 0); + + public void release() { + if (lock == null) { + return; + } + try { + lock.unlockAsync(ownerId).toCompletableFuture().join(); + } catch (RuntimeException e) { + // expired after its lease time (and maybe taken by another sync), or Redis unreachable + log.warn("Sync lock {} was not released: {}", lock.getName(), e.getMessage()); + } + } + } +} diff --git a/src/main/java/com/company/resourceapi/web/ApiExceptionHandler.java b/src/main/java/com/company/resourceapi/web/ApiExceptionHandler.java index fa78cc8..3630122 100644 --- a/src/main/java/com/company/resourceapi/web/ApiExceptionHandler.java +++ b/src/main/java/com/company/resourceapi/web/ApiExceptionHandler.java @@ -1,10 +1,12 @@ package com.company.resourceapi.web; import static org.springframework.http.HttpHeaders.RETRY_AFTER; +import static org.springframework.http.HttpStatus.CONFLICT; import static org.springframework.http.HttpStatus.NOT_FOUND; import static org.springframework.http.HttpStatus.SERVICE_UNAVAILABLE; import com.company.resourceapi.exceptions.NotFoundException; +import com.company.resourceapi.exceptions.SyncInProgressException; import lombok.extern.slf4j.Slf4j; import org.springframework.core.task.TaskRejectedException; import org.springframework.http.ResponseEntity; @@ -22,6 +24,13 @@ ResponseEntity handleNotFound(NotFoundException e) { return ResponseEntity.status(NOT_FOUND).build(); } + /** Another request already started a sync of this project, which is still running. */ + @ExceptionHandler(SyncInProgressException.class) + ResponseEntity handleSyncInProgress(SyncInProgressException e) { + log.debug(e.getMessage()); + return ResponseEntity.status(CONFLICT).build(); + } + /** The background task executor is at its concurrency limit; the client can retry shortly. */ @ExceptionHandler(TaskRejectedException.class) ResponseEntity handleTaskRejected(TaskRejectedException e) { diff --git a/src/main/resources/application.properties b/src/main/resources/application.properties index 150cb61..dc86186 100755 --- a/src/main/resources/application.properties +++ b/src/main/resources/application.properties @@ -17,10 +17,16 @@ spring.task.execution.shutdown.await-termination-period=10s # (Lettuce's default command timeout is 60s) spring.data.redis.connect-timeout=500ms spring.data.redis.timeout=500ms +# explicit: Redisson brings a JCache provider, which Boot would otherwise prefer over Redis +spring.cache.type=redis spring.cache.redis.key-prefix=resourceapi:: spring.cache.redis.cache-null-values=false resourceapi.cache.sdlc-systems-ttl=10m resourceapi.cache.external-projects-ttl=5m +# redisson (locks, rate limiters) uses the same Redis; its object names get this prefix +resourceapi.redisson.key-prefix=resourceapi: +# a project's sync lock expires after this even if never released (e.g. the instance died) +resourceapi.sync.lock-lease-time=60s # datasource spring.datasource.url=jdbc:h2:mem:test;MODE=MySQL;DATABASE_TO_LOWER=TRUE spring.sql.init.platform=h2 diff --git a/src/test/java/com/company/resourceapi/RedisTestContainerInitializer.java b/src/test/java/com/company/resourceapi/RedisTestContainerInitializer.java index ec1a96b..1a42868 100644 --- a/src/test/java/com/company/resourceapi/RedisTestContainerInitializer.java +++ b/src/test/java/com/company/resourceapi/RedisTestContainerInitializer.java @@ -30,8 +30,10 @@ public void initialize(ConfigurableApplicationContext context) { "spring.data.redis.host", REDIS.getHost(), "spring.data.redis.port", REDIS.getMappedPort(6379)))); // overrides application.properties + var contextId = "test-" + UUID.randomUUID(); propertySources.addFirst(new MapPropertySource("redisTestContainerKeyPrefix", Map.of( - "spring.cache.redis.key-prefix", "test-" + UUID.randomUUID() + "::"))); + "spring.cache.redis.key-prefix", contextId + "::", + "resourceapi.redisson.key-prefix", contextId + ":"))); } private static synchronized void startOnce() { diff --git a/src/test/java/com/company/resourceapi/cache/CacheUnavailableTest.java b/src/test/java/com/company/resourceapi/cache/CacheUnavailableTest.java index 34e6225..a4aaa3e 100644 --- a/src/test/java/com/company/resourceapi/cache/CacheUnavailableTest.java +++ b/src/test/java/com/company/resourceapi/cache/CacheUnavailableTest.java @@ -69,4 +69,26 @@ void givenRedisDownWhenUsingCachedEndpointsThenTheyStillWork() throws Exception // each skipped cache read/write is bounded by the Redis timeouts, not Lettuce's 60s default assertThat(Duration.ofNanos(System.nanoTime() - start)).isLessThan(Duration.ofSeconds(10)); } + + @Test + void givenRedisDownWhenSyncThenItRunsWithoutTheLock() throws Exception { + var sdlcSystem = new SdlcSystem(); + sdlcSystem.setBaseUrl(sdlcSystemServer.baseUrl()); + sdlcSystem = sdlcSystemRepository.save(sdlcSystem); + var project = new Project(); + project.setExternalId("NOLOCK"); + project.setSdlcSystem(sdlcSystem); + long projectId = projectRepository.save(project).getId(); + sdlcSystemServer.stubFor(WireMock.get(urlEqualTo("/rest/api/2/project/NOLOCK")) + .willReturn(okJson("{\"key\": \"NOLOCK\", \"name\": \"Synced without Redis\"}"))); + + mockMvc.perform(post(ProjectRestController.ENDPOINT + "/" + projectId + "/sync")) + .andExpect(status().isAccepted()); + + var deadline = System.nanoTime() + Duration.ofSeconds(10).toNanos(); + while (!"Synced without Redis".equals(projectRepository.findById(projectId).orElseThrow().getName())) { + assertThat(System.nanoTime()).as("sync not done within 10s").isLessThan(deadline); + Thread.sleep(50); + } + } } diff --git a/src/test/java/com/company/resourceapi/lock/ProjectSyncLockIntegrationTest.java b/src/test/java/com/company/resourceapi/lock/ProjectSyncLockIntegrationTest.java new file mode 100644 index 0000000..fe14c51 --- /dev/null +++ b/src/test/java/com/company/resourceapi/lock/ProjectSyncLockIntegrationTest.java @@ -0,0 +1,153 @@ +package com.company.resourceapi.lock; + +import static com.github.tomakehurst.wiremock.client.WireMock.anyRequestedFor; +import static com.github.tomakehurst.wiremock.client.WireMock.anyUrl; +import static com.github.tomakehurst.wiremock.client.WireMock.getRequestedFor; +import static com.github.tomakehurst.wiremock.client.WireMock.okJson; +import static com.github.tomakehurst.wiremock.client.WireMock.urlEqualTo; +import static com.github.tomakehurst.wiremock.core.WireMockConfiguration.wireMockConfig; +import static org.assertj.core.api.Assertions.assertThat; +import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.post; +import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status; + +import com.company.resourceapi.controllers.ProjectRestController; +import com.company.resourceapi.entities.Project; +import com.company.resourceapi.entities.SdlcSystem; +import com.company.resourceapi.repositories.ProjectRepository; +import com.company.resourceapi.repositories.SdlcSystemRepository; +import com.github.tomakehurst.wiremock.client.WireMock; +import com.github.tomakehurst.wiremock.junit5.WireMockExtension; +import java.time.Duration; +import java.time.Instant; +import java.util.concurrent.TimeUnit; +import java.util.function.BooleanSupplier; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.RegisterExtension; +import org.redisson.api.RedissonClient; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.beans.factory.annotation.Value; +import org.springframework.boot.test.context.SpringBootTest; +import org.springframework.boot.webmvc.test.autoconfigure.AutoConfigureMockMvc; +import org.springframework.data.redis.core.StringRedisTemplate; +import org.springframework.test.web.servlet.MockMvc; + +/** + * One sync per project at a time, against a real Redis; "another instance" holds the lock directly. + * The application's lease is long, so a lock that is free again was released, not expired. + */ +@SpringBootTest(properties = "resourceapi.sync.lock-lease-time=30s") +@AutoConfigureMockMvc +class ProjectSyncLockIntegrationTest { + + private static final String EXTERNAL_ID = "LOCKED"; + private static final String SDLC_PROJECT_URL = "/rest/api/2/project/" + EXTERNAL_ID; + + @RegisterExtension + static WireMockExtension sdlcSystemServer = WireMockExtension.newInstance() + .options(wireMockConfig().dynamicPort()) + .build(); + + @Autowired + private MockMvc mockMvc; + + @Autowired + private SdlcSystemRepository sdlcSystemRepository; + + @Autowired + private ProjectRepository projectRepository; + + @Autowired + private RedissonClient redisson; + + @Autowired + private StringRedisTemplate redisTemplate; + + @Value("${resourceapi.redisson.key-prefix}") + private String keyPrefix; + + private long projectId; + private String syncUrl; + + @BeforeEach + void setup() { + var sdlcSystem = new SdlcSystem(); + sdlcSystem.setBaseUrl(sdlcSystemServer.baseUrl()); + sdlcSystem = sdlcSystemRepository.save(sdlcSystem); + var project = new Project(); + project.setExternalId(EXTERNAL_ID); + project.setName("Old name"); + project.setSdlcSystem(sdlcSystem); + projectId = projectRepository.save(project).getId(); + syncUrl = ProjectRestController.ENDPOINT + "/" + projectId + "/sync"; + sdlcSystemServer.stubFor(WireMock.get(urlEqualTo(SDLC_PROJECT_URL)) + .willReturn(okJson("{\"key\": \"LOCKED\", \"name\": \"New name\"}"))); + } + + @Test + void givenSyncRunningElsewhereWhenSyncThenReturnStatusConflict() throws Exception { + // another instance holds the project's lock (same Redis, different owner) + var otherInstanceLock = redisson.getLock("project-sync:" + projectId); + assertThat(otherInstanceLock.tryLockAsync(0, 10, TimeUnit.SECONDS, 4711L).toCompletableFuture().join()).isTrue(); + try { + mockMvc.perform(post(syncUrl)).andExpect(status().isConflict()); + + Thread.sleep(200); + sdlcSystemServer.verify(0, anyRequestedFor(anyUrl())); + } finally { + otherInstanceLock.unlockAsync(4711L).toCompletableFuture().join(); + } + + // released: syncing works again + mockMvc.perform(post(syncUrl)).andExpect(status().isAccepted()); + await(() -> projectRepository.findById(projectId).orElseThrow().getName().equals("New name")); + } + + @Test + void givenSlowSyncWhenSyncedTwiceThenSecondIsRefusedUntilFirstEnds() throws Exception { + sdlcSystemServer.stubFor(WireMock.get(urlEqualTo(SDLC_PROJECT_URL)) + .willReturn(okJson("{\"key\": \"LOCKED\", \"name\": \"New name\"}").withFixedDelay(800))); + + mockMvc.perform(post(syncUrl)).andExpect(status().isAccepted()); + mockMvc.perform(post(syncUrl)).andExpect(status().isConflict()); + + // the first sync releases the lock when it ends, long before its 30s lease would expire + await(() -> projectRepository.findById(projectId).orElseThrow().getName().equals("New name")); + await(() -> !redisson.getLock("project-sync:" + projectId).isLocked(), Duration.ofMillis(500)); + mockMvc.perform(post(syncUrl)).andExpect(status().isAccepted()); + sdlcSystemServer.verify(1, getRequestedFor(urlEqualTo(SDLC_PROJECT_URL))); + } + + @Test + void givenLockNeverReleasedThenItExpiresAfterItsLeaseTime() throws Exception { + // an instance took the lock and died: nobody releases it + var abandonedLock = redisson.getLock("project-sync:" + projectId); + abandonedLock.tryLockAsync(0, 2, TimeUnit.SECONDS, 4712L).toCompletableFuture().join(); + mockMvc.perform(post(syncUrl)).andExpect(status().isConflict()); + + await(() -> !abandonedLock.isLocked()); + mockMvc.perform(post(syncUrl)).andExpect(status().isAccepted()); + } + + @Test + void givenLockNameThenItCarriesThisContextsPrefix() { + redisson.getLock("project-sync:" + projectId).tryLockAsync(0, 2, TimeUnit.SECONDS, 4713L) + .toCompletableFuture().join(); + + // raw Redis keys (Redisson's own key API would apply the prefix to the pattern too): + // RedisTestContainerInitializer gives each test context its own prefix + assertThat(redisTemplate.keys(keyPrefix + "project-sync:" + projectId)).hasSize(1); + } + + private static void await(BooleanSupplier condition) throws InterruptedException { + await(condition, Duration.ofSeconds(5)); + } + + private static void await(BooleanSupplier condition, Duration timeout) throws InterruptedException { + var deadline = Instant.now().plus(timeout); + while (!condition.getAsBoolean()) { + assertThat(Instant.now()).as("condition not met within " + timeout).isBefore(deadline); + Thread.sleep(50); + } + } +} diff --git a/src/test/java/com/company/resourceapi/services/ProjectSyncBackpressureTest.java b/src/test/java/com/company/resourceapi/services/ProjectSyncBackpressureTest.java index 8a80a55..235ebd7 100644 --- a/src/test/java/com/company/resourceapi/services/ProjectSyncBackpressureTest.java +++ b/src/test/java/com/company/resourceapi/services/ProjectSyncBackpressureTest.java @@ -46,17 +46,26 @@ void givenBusyTaskExecutorWhenSyncThenReturnStatusServiceUnavailable() throws Ex var sdlcSystem = new SdlcSystem(); sdlcSystem.setBaseUrl(sdlcSystemServer.baseUrl()); sdlcSystem = sdlcSystemRepository.save(sdlcSystem); - var project = new Project(); - project.setExternalId("BUSY"); - project.setSdlcSystem(sdlcSystem); - var syncUrl = ProjectRestController.ENDPOINT + "/" + projectRepository.save(project).getId() + "/sync"; + var syncUrl = syncUrl(sdlcSystem, "BUSY"); + var otherSyncUrl = syncUrl(sdlcSystem, "OTHER"); sdlcSystemServer.stubFor(get(urlPathMatching("/rest/api/2/project/.*")) .willReturn(okJson("{\"key\": \"BUSY\", \"name\": \"Busy\"}").withFixedDelay(1_000))); - // when / then + // when / then: another project's sync (the same project's would be refused by its lock) mockMvc.perform(post(syncUrl)).andExpect(status().isAccepted()); - mockMvc.perform(post(syncUrl)) + mockMvc.perform(post(otherSyncUrl)) .andExpect(status().isServiceUnavailable()) .andExpect(header().string("Retry-After", "1")); + + // the rejected sync released its lock: it can be started once a slot is free + Thread.sleep(1_500); + mockMvc.perform(post(otherSyncUrl)).andExpect(status().isAccepted()); + } + + private String syncUrl(SdlcSystem sdlcSystem, String externalId) { + var project = new Project(); + project.setExternalId(externalId); + project.setSdlcSystem(sdlcSystem); + return ProjectRestController.ENDPOINT + "/" + projectRepository.save(project).getId() + "/sync"; } } From b7819030dc5739ec6fbb9ecf63e9dea077b9aadd Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 31 Oct 2026 10:17:11 +0000 Subject: [PATCH 04/13] Address review: lock in the service layer, no orphaned or blocking locks - ProjectSyncService.requestSync takes and releases the lock; the async work moves to ProjectSyncWorker, so no caller can start a sync unlocked - A lock request that fails (e.g. times out after Redis applied it) keeps its lease, so the sync's end still tries to release it - Redisson is only created for a standalone Redis; with Sentinel/Cluster the app starts and syncs run unlocked (was: startup failure) - The task executor depends on Redisson, so on shutdown running syncs release their locks before Redisson stops - No Redisson retries (a failed request took 2-3s), small connection pool (defaults were 24 idle / 64 max) - Tests: Sentinel config starts and syncs unlocked; executor shutdown order; SDLC call count checked before the next sync can call; slow-sync test waits for its last sync; backpressure test polls instead of sleeping Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_012MLyPXGHeCc1fvTeB8JnT2 --- README.md | 6 +- .../resourceapi/config/RedissonConfig.java | 38 ++++++++-- .../controllers/ProjectSyncController.java | 21 +----- .../resourceapi/services/ProjectSyncLock.java | 33 +++++---- .../services/ProjectSyncService.java | 73 ++++++------------ .../services/ProjectSyncWorker.java | 73 ++++++++++++++++++ .../lock/NonStandaloneRedisTest.java | 74 +++++++++++++++++++ .../lock/ProjectSyncLockIntegrationTest.java | 17 ++++- .../services/ProjectSyncBackpressureTest.java | 13 +++- 9 files changed, 253 insertions(+), 95 deletions(-) create mode 100644 src/main/java/com/company/resourceapi/services/ProjectSyncWorker.java create mode 100644 src/test/java/com/company/resourceapi/lock/NonStandaloneRedisTest.java diff --git a/README.md b/README.md index 3012c81..09267f9 100644 --- a/README.md +++ b/README.md @@ -91,8 +91,10 @@ optimization: if Redis is unreachable, cache reads and writes are logged and ski runs at a time, across all instances; a second one gets `409 Conflict` until the first ends. The lock is released when the background sync ends and expires after `resourceapi.sync.lock-lease-time` (60s) in case an instance dies mid-sync. If Redis can't be reached, the sync runs without the lock (a duplicate sync only -costs an extra SDLC call). Redisson connects lazily and uses the same Redis connection settings; its keys -are prefixed with `resourceapi.redisson.key-prefix`. (Redisson's Spring Boot starter isn't used: it targets +costs an extra SDLC call; the failed lock request takes about a second). Redisson connects lazily and uses the +same Redis connection settings; its keys are prefixed with `resourceapi.redisson.key-prefix`. Only a +standalone Redis is supported for the lock: with Sentinel or Cluster the application still starts and syncs +run unlocked; TLS uses the JVM's trust store (Boot SSL bundles aren't passed to Redisson). (Redisson's Spring Boot starter isn't used: it targets Spring Boot 3. Redisson brings a JCache provider, so `spring.cache.type=redis` is set explicitly.) **Local development**: `./mvnw spring-boot:run` starts Redis from `compose.yaml` automatically diff --git a/src/main/java/com/company/resourceapi/config/RedissonConfig.java b/src/main/java/com/company/resourceapi/config/RedissonConfig.java index ac22be6..4d08765 100644 --- a/src/main/java/com/company/resourceapi/config/RedissonConfig.java +++ b/src/main/java/com/company/resourceapi/config/RedissonConfig.java @@ -1,11 +1,15 @@ package com.company.resourceapi.config; import java.time.Duration; +import java.util.Arrays; +import java.util.stream.Stream; import org.redisson.Redisson; import org.redisson.api.RedissonClient; import org.redisson.config.Config; import org.redisson.config.NameMapper; +import org.springframework.beans.factory.config.BeanFactoryPostProcessor; import org.springframework.beans.factory.annotation.Value; +import org.springframework.boot.autoconfigure.condition.ConditionalOnExpression; import org.springframework.boot.data.redis.autoconfigure.DataRedisConnectionDetails; import org.springframework.boot.data.redis.autoconfigure.DataRedisProperties; import org.springframework.context.annotation.Bean; @@ -17,17 +21,20 @@ * *

It connects lazily, on first use: the application starts without Redis (for example the AOT * training run while building an image), and Redis being down only affects the calls that need it. + * Only a standalone Redis is supported; with Sentinel or Cluster there is no client and its users fall + * back to running without it. TLS uses the JVM's trust store (Boot SSL bundles aren't passed on). */ @Configuration public class RedissonConfig { - @Bean(destroyMethod = "shutdown") + static final String REDISSON_CLIENT = "redissonClient"; + static final String TASK_EXECUTOR = "applicationTaskExecutor"; + + @Bean(name = REDISSON_CLIENT, destroyMethod = "shutdown") + @ConditionalOnExpression("'${spring.data.redis.sentinel.master:}' == '' and '${spring.data.redis.cluster.nodes:}' == ''") RedissonClient redissonClient(DataRedisConnectionDetails connection, DataRedisProperties redis, @Value("${resourceapi.redisson.key-prefix}") String keyPrefix) { var standalone = connection.getStandalone(); - if (standalone == null) { - throw new IllegalStateException("Only a standalone Redis is supported for Redisson"); - } var config = new Config().setLazyInitialization(true); config.useSingleServer() .setAddress((connection.getSslBundle() != null ? "rediss://" : "redis://") @@ -38,11 +45,32 @@ RedissonClient redissonClient(DataRedisConnectionDetails connection, DataRedisPr .setConnectTimeout(millis(redis.getConnectTimeout(), Duration.ofSeconds(10))) .setTimeout(millis(redis.getTimeout(), Duration.ofSeconds(3))) // fail fast like the cache: callers decide what to do without Redis - .setRetryAttempts(1) + .setRetryAttempts(0) + // a lock or rate limit check now and then needs few connections (defaults: 24 idle, 64 max) + .setConnectionMinimumIdleSize(1) + .setConnectionPoolSize(8) + .setSubscriptionConnectionMinimumIdleSize(1) + .setSubscriptionConnectionPoolSize(4) .setNameMapper(new KeyPrefixNameMapper(keyPrefix)); return Redisson.create(config); } + /** + * On shutdown, the task executor waits for running syncs, which release their locks when they end; + * Redisson must still be up then, so the executor depends on it and is destroyed first. + */ + @Bean + static BeanFactoryPostProcessor taskExecutorShutsDownBeforeRedisson() { + return beanFactory -> { + if (beanFactory.containsBeanDefinition(TASK_EXECUTOR) && beanFactory.containsBeanDefinition(REDISSON_CLIENT)) { + var executor = beanFactory.getBeanDefinition(TASK_EXECUTOR); + var dependsOn = executor.getDependsOn() == null ? new String[0] : executor.getDependsOn(); + executor.setDependsOn(Stream.concat(Arrays.stream(dependsOn), Stream.of(REDISSON_CLIENT)) + .distinct().toArray(String[]::new)); + } + }; + } + private static int millis(Duration value, Duration fallback) { return (int) (value != null ? value : fallback).toMillis(); } diff --git a/src/main/java/com/company/resourceapi/controllers/ProjectSyncController.java b/src/main/java/com/company/resourceapi/controllers/ProjectSyncController.java index 38f6672..11d12fe 100644 --- a/src/main/java/com/company/resourceapi/controllers/ProjectSyncController.java +++ b/src/main/java/com/company/resourceapi/controllers/ProjectSyncController.java @@ -5,9 +5,6 @@ import static com.company.resourceapi.controllers.ProjectRestController.PATH_VARIABLE_ID; import com.company.resourceapi.exceptions.NotFoundException; -import com.company.resourceapi.exceptions.SyncInProgressException; -import com.company.resourceapi.services.ProjectService; -import com.company.resourceapi.services.ProjectSyncLock; import com.company.resourceapi.services.ProjectSyncService; import io.swagger.v3.oas.annotations.Operation; import io.swagger.v3.oas.annotations.Parameter; @@ -25,29 +22,15 @@ @RequiredArgsConstructor public class ProjectSyncController { - private final ProjectService projectService; private final ProjectSyncService projectSyncService; - private final ProjectSyncLock projectSyncLock; @Operation(summary = "Refresh a Project's name from its SDLC system, in the background") @PostMapping(ENDPOINT_ID + "/sync") public ResponseEntity syncProject( @Parameter(name = PATH_VARIABLE_ID, description = "ID", required = true) @PathVariable(PATH_VARIABLE_ID) final long projectId) throws NotFoundException { - // an unknown project is reported now, not only in the background (404 via ApiExceptionHandler) - projectService.getProject(projectId); - - // one sync per project at a time, across instances (409 via ApiExceptionHandler); the background - // task releases the lock when it ends - var lease = projectSyncLock.tryAcquire(projectId) - .orElseThrow(() -> new SyncInProgressException(projectId)); - try { - projectSyncService.syncName(projectId).whenComplete((result, failure) -> lease.release()); - } catch (RuntimeException e) { - // not started, e.g. a full task executor (503 via ApiExceptionHandler) - lease.release(); - throw e; - } + // unknown project: 404, sync already running: 409, no background slot: 503 (ApiExceptionHandler) + projectSyncService.requestSync(projectId); return ResponseEntity.accepted().build(); } } diff --git a/src/main/java/com/company/resourceapi/services/ProjectSyncLock.java b/src/main/java/com/company/resourceapi/services/ProjectSyncLock.java index 820d721..76c58f2 100644 --- a/src/main/java/com/company/resourceapi/services/ProjectSyncLock.java +++ b/src/main/java/com/company/resourceapi/services/ProjectSyncLock.java @@ -7,6 +7,7 @@ import lombok.extern.slf4j.Slf4j; import org.redisson.api.RLock; import org.redisson.api.RedissonClient; +import org.springframework.beans.factory.ObjectProvider; import org.springframework.beans.factory.annotation.Value; import org.springframework.stereotype.Component; @@ -15,8 +16,9 @@ * *

The lock is taken by the request and released by the background task, on another thread, so it * is held by a random owner id instead of a thread. It expires after the lease time, so an instance - * that dies mid-sync doesn't block the project forever. If Redis can't be reached the sync runs - * unlocked: a duplicate sync only costs an extra call, while refusing it would stop syncs entirely. + * that dies mid-sync doesn't block the project forever. Without Redis (unreachable, or no Redisson + * client for a non-standalone Redis) the sync runs unlocked: a duplicate sync only costs an extra + * call, while refusing it would stop syncs entirely. */ @Component @Slf4j @@ -25,14 +27,21 @@ public class ProjectSyncLock { private final RedissonClient redisson; private final Duration leaseTime; - public ProjectSyncLock(RedissonClient redisson, @Value("${resourceapi.sync.lock-lease-time}") Duration leaseTime) { - this.redisson = redisson; + public ProjectSyncLock(ObjectProvider redisson, + @Value("${resourceapi.sync.lock-lease-time}") Duration leaseTime) { + this.redisson = redisson.getIfAvailable(); this.leaseTime = leaseTime; + if (this.redisson == null) { + log.warn("No Redisson client (only a standalone Redis is supported): syncs run without the per-project lock"); + } } /** @return the lease to release when the sync ends, or empty if a sync of the project is running */ public Optional tryAcquire(long projectId) { - var lock = lock(projectId); + if (redisson == null) { + return Optional.of(Lease.NONE); + } + var lock = redisson.getLock("project-sync:" + projectId); long ownerId = ThreadLocalRandom.current().nextLong(); try { boolean acquired = lock.tryLockAsync(0, leaseTime.toMillis(), TimeUnit.MILLISECONDS, ownerId) @@ -40,15 +49,13 @@ public Optional tryAcquire(long projectId) { return acquired ? Optional.of(new Lease(lock, ownerId)) : Optional.empty(); } catch (RuntimeException e) { log.warn("Sync lock for project {} unavailable, syncing without it: {}", projectId, e.getMessage()); - return Optional.of(Lease.NONE); + // a timed-out request may still have taken the lock in Redis: keep the lease, so the sync's + // end tries to release it instead of leaving it held until it expires + return Optional.of(new Lease(lock, ownerId)); } } - private RLock lock(long projectId) { - return redisson.getLock("project-sync:" + projectId); - } - - /** A held lock; releasing it twice, or after it expired, is harmless. */ + /** A lock that may be held; releasing one that isn't (anymore) only logs. */ public record Lease(RLock lock, long ownerId) { static final Lease NONE = new Lease(null, 0); @@ -60,8 +67,8 @@ public void release() { try { lock.unlockAsync(ownerId).toCompletableFuture().join(); } catch (RuntimeException e) { - // expired after its lease time (and maybe taken by another sync), or Redis unreachable - log.warn("Sync lock {} was not released: {}", lock.getName(), e.getMessage()); + // not held: never taken, expired after its lease time, or Redis unreachable + log.debug("Sync lock {} not released: {}", lock.getName(), e.getMessage()); } } } diff --git a/src/main/java/com/company/resourceapi/services/ProjectSyncService.java b/src/main/java/com/company/resourceapi/services/ProjectSyncService.java index f0e5880..5dea543 100644 --- a/src/main/java/com/company/resourceapi/services/ProjectSyncService.java +++ b/src/main/java/com/company/resourceapi/services/ProjectSyncService.java @@ -1,70 +1,39 @@ package com.company.resourceapi.services; -import static java.util.concurrent.CompletableFuture.completedFuture; - -import java.util.concurrent.CompletableFuture; -import java.util.concurrent.atomic.AtomicBoolean; +import com.company.resourceapi.exceptions.NotFoundException; +import com.company.resourceapi.exceptions.SyncInProgressException; import lombok.RequiredArgsConstructor; -import lombok.extern.slf4j.Slf4j; -import org.springframework.scheduling.annotation.Async; import org.springframework.stereotype.Service; -import org.springframework.web.client.RestClientResponseException; -/** Example of background work: refreshes a project's name from its SDLC system. */ +/** Starts background syncs, at most one per project at a time across all instances. */ @Service @RequiredArgsConstructor -@Slf4j public class ProjectSyncService { private final ProjectService projectService; - /** Bypasses the cache (a sync must see the current state) and refreshes it. */ - private final ExternalProjectService externalProjectService; + private final ProjectSyncLock projectSyncLock; + private final ProjectSyncWorker projectSyncWorker; /** - * Runs on a virtual thread of the application task executor, in the caller's trace. Failures are - * logged here, since nobody waits for the result of a fire-and-forget call. + * Starts refreshing the project from its SDLC system in the background. * - * @return completes when the sync has finished, exceptionally if it failed + * @throws NotFoundException if the project doesn't exist + * @throws SyncInProgressException if a sync of the project is still running + * @throws org.springframework.core.task.TaskRejectedException if no background slot is free */ - @Async - public CompletableFuture syncName(long projectId) { - try { - var project = projectService.getProject(projectId); - var externalId = project.getExternalId(); - var sdlcSystemId = project.getSdlcSystem().getId(); + public void requestSync(long projectId) throws NotFoundException { + // an unknown project is reported now, not only in the background + projectService.getProject(projectId); - var externalProject = externalProjectService.refreshProject(project.getSdlcSystem(), externalId); - if (externalProject.isEmpty()) { - log.warn("Project {} ({}) is unknown to its SDLC system", projectId, externalId); - return completedFuture(null); - } - var externalName = externalProject.get().name(); - if (externalName == null || externalName.equals(project.getName())) { - log.info("Project {} name is unchanged in its SDLC system", projectId); - return completedFuture(null); - } - - var applied = new AtomicBoolean(); - projectService.updateProject(projectId, stored -> { - // the project may have been re-keyed while its SDLC system was being asked - if (stored.getExternalId().equals(externalId) && stored.getSdlcSystem().getId() == sdlcSystemId) { - stored.setName(externalName); - applied.set(true); - } - }); - if (applied.get()) { - log.info("Synced project {} name from its SDLC system", projectId); - } else { - log.info("Project {} changed while syncing; SDLC system name not applied", projectId); - } - return completedFuture(null); - } catch (Exception e) { - // the status only: an error response's message would include the upstream body - var reason = e instanceof RestClientResponseException response - ? "HTTP " + response.getStatusCode().value() - : e.getClass().getSimpleName() + ": " + e.getMessage(); - log.warn("Syncing project {} from its SDLC system failed: {}", projectId, reason); - return CompletableFuture.failedFuture(e); + var lease = projectSyncLock.tryAcquire(projectId) + .orElseThrow(() -> new SyncInProgressException(projectId)); + try { + // the background task releases the lock when it ends, however it ends + projectSyncWorker.syncName(projectId).whenComplete((result, failure) -> lease.release()); + } catch (RuntimeException e) { + // not started, e.g. a full task executor + lease.release(); + throw e; } } } diff --git a/src/main/java/com/company/resourceapi/services/ProjectSyncWorker.java b/src/main/java/com/company/resourceapi/services/ProjectSyncWorker.java new file mode 100644 index 0000000..ddab6f5 --- /dev/null +++ b/src/main/java/com/company/resourceapi/services/ProjectSyncWorker.java @@ -0,0 +1,73 @@ +package com.company.resourceapi.services; + +import static java.util.concurrent.CompletableFuture.completedFuture; + +import java.util.concurrent.CompletableFuture; +import java.util.concurrent.atomic.AtomicBoolean; +import lombok.RequiredArgsConstructor; +import lombok.extern.slf4j.Slf4j; +import org.springframework.scheduling.annotation.Async; +import org.springframework.stereotype.Service; +import org.springframework.web.client.RestClientResponseException; + +/** + * Example of background work: refreshes a project's name from its SDLC system. Started through + * {@link ProjectSyncService#requestSync}, which makes sure only one runs per project. + */ +@Service +@RequiredArgsConstructor +@Slf4j +public class ProjectSyncWorker { + + private final ProjectService projectService; + /** Bypasses the cache (a sync must see the current state) and refreshes it. */ + private final ExternalProjectService externalProjectService; + + /** + * Runs on a virtual thread of the application task executor, in the caller's trace. Failures are + * logged here, since nobody waits for the result of a fire-and-forget call. + * + * @return completes when the sync has finished, exceptionally if it failed + */ + @Async + public CompletableFuture syncName(long projectId) { + try { + var project = projectService.getProject(projectId); + var externalId = project.getExternalId(); + var sdlcSystemId = project.getSdlcSystem().getId(); + + var externalProject = externalProjectService.refreshProject(project.getSdlcSystem(), externalId); + if (externalProject.isEmpty()) { + log.warn("Project {} ({}) is unknown to its SDLC system", projectId, externalId); + return completedFuture(null); + } + var externalName = externalProject.get().name(); + if (externalName == null || externalName.equals(project.getName())) { + log.info("Project {} name is unchanged in its SDLC system", projectId); + return completedFuture(null); + } + + var applied = new AtomicBoolean(); + projectService.updateProject(projectId, stored -> { + // the project may have been re-keyed while its SDLC system was being asked + if (stored.getExternalId().equals(externalId) && stored.getSdlcSystem().getId() == sdlcSystemId) { + stored.setName(externalName); + applied.set(true); + } + }); + if (applied.get()) { + log.info("Synced project {} name from its SDLC system", projectId); + } else { + log.info("Project {} changed while syncing; SDLC system name not applied", projectId); + } + return completedFuture(null); + } catch (Exception e) { + // the status only: an error response's message would include the upstream body + var reason = e instanceof RestClientResponseException response + ? "HTTP " + response.getStatusCode().value() + : e.getClass().getSimpleName() + ": " + e.getMessage(); + log.warn("Syncing project {} from its SDLC system failed: {}", projectId, reason); + return CompletableFuture.failedFuture(e); + } + } +} diff --git a/src/test/java/com/company/resourceapi/lock/NonStandaloneRedisTest.java b/src/test/java/com/company/resourceapi/lock/NonStandaloneRedisTest.java new file mode 100644 index 0000000..88fc296 --- /dev/null +++ b/src/test/java/com/company/resourceapi/lock/NonStandaloneRedisTest.java @@ -0,0 +1,74 @@ +package com.company.resourceapi.lock; + +import static com.github.tomakehurst.wiremock.client.WireMock.okJson; +import static com.github.tomakehurst.wiremock.client.WireMock.urlEqualTo; +import static com.github.tomakehurst.wiremock.core.WireMockConfiguration.wireMockConfig; +import static org.assertj.core.api.Assertions.assertThat; +import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.post; +import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status; + +import com.company.resourceapi.controllers.ProjectRestController; +import com.company.resourceapi.entities.Project; +import com.company.resourceapi.entities.SdlcSystem; +import com.company.resourceapi.repositories.ProjectRepository; +import com.company.resourceapi.repositories.SdlcSystemRepository; +import com.github.tomakehurst.wiremock.client.WireMock; +import com.github.tomakehurst.wiremock.junit5.WireMockExtension; +import java.time.Duration; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.RegisterExtension; +import org.redisson.api.RedissonClient; +import org.springframework.beans.factory.ObjectProvider; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.boot.test.context.SpringBootTest; +import org.springframework.boot.webmvc.test.autoconfigure.AutoConfigureMockMvc; +import org.springframework.test.web.servlet.MockMvc; + +/** A Sentinel (or Cluster) Redis doesn't stop the application: syncs run without the lock. */ +@SpringBootTest(properties = { + "spring.data.redis.sentinel.master=mymaster", + "spring.data.redis.sentinel.nodes=localhost:1"}) +@AutoConfigureMockMvc +class NonStandaloneRedisTest { + + @RegisterExtension + static WireMockExtension sdlcSystemServer = WireMockExtension.newInstance() + .options(wireMockConfig().dynamicPort()) + .build(); + + @Autowired + private MockMvc mockMvc; + + @Autowired + private SdlcSystemRepository sdlcSystemRepository; + + @Autowired + private ProjectRepository projectRepository; + + @Autowired + private ObjectProvider redisson; + + @Test + void givenSentinelRedisThenApplicationStartsAndSyncsRunUnlocked() throws Exception { + assertThat(redisson.getIfAvailable()).isNull(); + + var sdlcSystem = new SdlcSystem(); + sdlcSystem.setBaseUrl(sdlcSystemServer.baseUrl()); + sdlcSystem = sdlcSystemRepository.save(sdlcSystem); + var project = new Project(); + project.setExternalId("SENTINEL"); + project.setSdlcSystem(sdlcSystem); + long projectId = projectRepository.save(project).getId(); + sdlcSystemServer.stubFor(WireMock.get(urlEqualTo("/rest/api/2/project/SENTINEL")) + .willReturn(okJson("{\"key\": \"SENTINEL\", \"name\": \"Synced unlocked\"}"))); + + mockMvc.perform(post(ProjectRestController.ENDPOINT + "/" + projectId + "/sync")) + .andExpect(status().isAccepted()); + + var deadline = System.nanoTime() + Duration.ofSeconds(15).toNanos(); + while (!"Synced unlocked".equals(projectRepository.findById(projectId).orElseThrow().getName())) { + assertThat(System.nanoTime()).as("sync not done within 15s").isLessThan(deadline); + Thread.sleep(50); + } + } +} diff --git a/src/test/java/com/company/resourceapi/lock/ProjectSyncLockIntegrationTest.java b/src/test/java/com/company/resourceapi/lock/ProjectSyncLockIntegrationTest.java index fe14c51..60a6593 100644 --- a/src/test/java/com/company/resourceapi/lock/ProjectSyncLockIntegrationTest.java +++ b/src/test/java/com/company/resourceapi/lock/ProjectSyncLockIntegrationTest.java @@ -27,6 +27,7 @@ import org.redisson.api.RedissonClient; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.beans.factory.annotation.Value; +import org.springframework.beans.factory.config.ConfigurableListableBeanFactory; import org.springframework.boot.test.context.SpringBootTest; import org.springframework.boot.webmvc.test.autoconfigure.AutoConfigureMockMvc; import org.springframework.data.redis.core.StringRedisTemplate; @@ -63,6 +64,9 @@ class ProjectSyncLockIntegrationTest { @Autowired private StringRedisTemplate redisTemplate; + @Autowired + private ConfigurableListableBeanFactory beanFactory; + @Value("${resourceapi.redisson.key-prefix}") private String keyPrefix; @@ -114,8 +118,12 @@ void givenSlowSyncWhenSyncedTwiceThenSecondIsRefusedUntilFirstEnds() throws Exce // the first sync releases the lock when it ends, long before its 30s lease would expire await(() -> projectRepository.findById(projectId).orElseThrow().getName().equals("New name")); await(() -> !redisson.getLock("project-sync:" + projectId).isLocked(), Duration.ofMillis(500)); - mockMvc.perform(post(syncUrl)).andExpect(status().isAccepted()); + // only the first sync called the SDLC system (checked before the next sync can call it) sdlcSystemServer.verify(1, getRequestedFor(urlEqualTo(SDLC_PROJECT_URL))); + mockMvc.perform(post(syncUrl)).andExpect(status().isAccepted()); + + // let that sync finish here, not during the next test + await(() -> !redisson.getLock("project-sync:" + projectId).isLocked()); } @Test @@ -150,4 +158,11 @@ private static void await(BooleanSupplier condition, Duration timeout) throws In Thread.sleep(50); } } + + @Test + void givenShutdownThenTaskExecutorStopsBeforeRedisson() { + // running syncs release their locks while the executor waits for them on shutdown + assertThat(beanFactory.getBeanDefinition("applicationTaskExecutor").getDependsOn()) + .contains("redissonClient"); + } } diff --git a/src/test/java/com/company/resourceapi/services/ProjectSyncBackpressureTest.java b/src/test/java/com/company/resourceapi/services/ProjectSyncBackpressureTest.java index 235ebd7..0781f63 100644 --- a/src/test/java/com/company/resourceapi/services/ProjectSyncBackpressureTest.java +++ b/src/test/java/com/company/resourceapi/services/ProjectSyncBackpressureTest.java @@ -4,6 +4,7 @@ import static com.github.tomakehurst.wiremock.client.WireMock.okJson; import static com.github.tomakehurst.wiremock.client.WireMock.urlPathMatching; import static com.github.tomakehurst.wiremock.core.WireMockConfiguration.wireMockConfig; +import static org.assertj.core.api.Assertions.assertThat; import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.post; import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.header; import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status; @@ -14,6 +15,7 @@ import com.company.resourceapi.repositories.ProjectRepository; import com.company.resourceapi.repositories.SdlcSystemRepository; import com.github.tomakehurst.wiremock.junit5.WireMockExtension; +import java.time.Duration; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.RegisterExtension; import org.springframework.beans.factory.annotation.Autowired; @@ -57,9 +59,14 @@ void givenBusyTaskExecutorWhenSyncThenReturnStatusServiceUnavailable() throws Ex .andExpect(status().isServiceUnavailable()) .andExpect(header().string("Retry-After", "1")); - // the rejected sync released its lock: it can be started once a slot is free - Thread.sleep(1_500); - mockMvc.perform(post(otherSyncUrl)).andExpect(status().isAccepted()); + // the rejected sync released its lock: it can be started once the first sync freed the slot + var deadline = System.nanoTime() + Duration.ofSeconds(10).toNanos(); + int status; + while ((status = mockMvc.perform(post(otherSyncUrl)).andReturn().getResponse().getStatus()) == 503) { + assertThat(System.nanoTime()).as("no free slot within 10s").isLessThan(deadline); + Thread.sleep(100); + } + assertThat(status).isEqualTo(202); } private String syncUrl(SdlcSystem sdlcSystem, String externalId) { From a435c90d15b49f32f4cbfb8492e291afc6897202 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 31 Oct 2026 10:25:08 +0000 Subject: [PATCH 05/13] Add distributed rate limits for SDLC calls and sync requests (Redisson) - RateLimiters: Redisson RRateLimiter token buckets shared by all instances; created with the configured rate, updated when the configuration changed, expiring after an hour unused; requests are allowed when Redis is unreachable - Calls to one SDLC system (resourceapi.rate-limit.sdlc.*, 10/s, waiting up to 1s): only real calls count, not cache hits; exceeded -> 503 + Retry-After (our own limit), logged by a background sync - POST /sync per project (resourceapi.rate-limit.sync.*, 6/min): checked after the 404 and lock checks, so refused requests don't use a permit; exceeded -> 429 + Retry-After - Tests (real Redis): third uncached call refused and not sent, permits shared with another instance and refilled, cache hits don't count, changed rate applied (verified to fail without the update), sync 429 with Retry-After, 409s don't use permits; Redis down still allows (verified to fail without fail-open) Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_012MLyPXGHeCc1fvTeB8JnT2 --- README.md | 11 ++ .../config/RateLimitProperties.java | 27 +++ .../resourceapi/config/RedissonConfig.java | 2 + .../RateLimitExceededException.java | 20 ++ .../services/ExternalProjectService.java | 22 ++- .../services/ProjectSyncService.java | 15 ++ .../resourceapi/services/RateLimiters.java | 64 +++++++ .../resourceapi/web/ApiExceptionHandler.java | 13 ++ src/main/resources/application.properties | 8 + .../ratelimit/RateLimitIntegrationTest.java | 180 ++++++++++++++++++ .../services/ProjectSyncBackpressureTest.java | 5 +- 11 files changed, 364 insertions(+), 3 deletions(-) create mode 100644 src/main/java/com/company/resourceapi/config/RateLimitProperties.java create mode 100644 src/main/java/com/company/resourceapi/exceptions/RateLimitExceededException.java create mode 100644 src/main/java/com/company/resourceapi/services/RateLimiters.java create mode 100644 src/test/java/com/company/resourceapi/ratelimit/RateLimitIntegrationTest.java diff --git a/README.md b/README.md index 09267f9..0c11806 100644 --- a/README.md +++ b/README.md @@ -97,6 +97,17 @@ standalone Redis is supported for the lock: with Sentinel or Cluster the applica run unlocked; TLS uses the JVM's trust store (Boot SSL bundles aren't passed to Redisson). (Redisson's Spring Boot starter isn't used: it targets Spring Boot 3. Redisson brings a JCache provider, so `spring.cache.type=redis` is set explicitly.) +**Rate limits** (Redisson `RRateLimiter`, token buckets shared by all instances): + +| Limit | Default | When exceeded | +|---|---|---| +| Calls to one SDLC system (`resourceapi.rate-limit.sdlc.*`), protecting its API quota; cache hits don't count | 10 per 1s, waiting up to 1s for a permit | `503` + `Retry-After` (our own limit, not the SDLC system's failure); a background sync logs it | +| `POST /api/v2/projects/{id}/sync` per project (`resourceapi.rate-limit.sync.*`); requests refused anyway (unknown project, sync already running) don't count | 6 per minute | `429` + `Retry-After` | + +A changed rate is applied to the existing limiter when an instance first uses it; unused limiters expire from +Redis after an hour. If Redis can't be reached, requests are allowed (limiting protects, losing it shouldn't +stop the application). + **Local development**: `./mvnw spring-boot:run` starts Redis from `compose.yaml` automatically (`spring-boot-docker-compose`); with `java -jar`, start it yourself with `docker compose up -d` (it listens on `localhost:6379`). Tests start their own Redis with Testcontainers, so they need Docker. diff --git a/src/main/java/com/company/resourceapi/config/RateLimitProperties.java b/src/main/java/com/company/resourceapi/config/RateLimitProperties.java new file mode 100644 index 0000000..7d1f95a --- /dev/null +++ b/src/main/java/com/company/resourceapi/config/RateLimitProperties.java @@ -0,0 +1,27 @@ +package com.company.resourceapi.config; + +import java.time.Duration; +import org.springframework.boot.context.properties.ConfigurationProperties; + +/** + * Distributed rate limits ({@code resourceapi.rate-limit.*}), shared by all instances through Redis. + * + * @param sdlc calls to one SDLC system (protects its API quota) + * @param sync sync requests for one project + */ +@ConfigurationProperties("resourceapi.rate-limit") +public record RateLimitProperties(Limit sdlc, Limit sync) { + + /** + * @param permits how many per period + * @param period the period the permits refill over + * @param maxWait how long a caller may wait for a permit before being refused + */ + public record Limit(long permits, Duration period, Duration maxWait) { + + /** Roughly when the next permit is available, for a Retry-After header. */ + public long retryAfterSeconds() { + return Math.max(1, (long) Math.ceil(period.toMillis() / 1000.0 / permits)); + } + } +} diff --git a/src/main/java/com/company/resourceapi/config/RedissonConfig.java b/src/main/java/com/company/resourceapi/config/RedissonConfig.java index 4d08765..1e086bf 100644 --- a/src/main/java/com/company/resourceapi/config/RedissonConfig.java +++ b/src/main/java/com/company/resourceapi/config/RedissonConfig.java @@ -10,6 +10,7 @@ import org.springframework.beans.factory.config.BeanFactoryPostProcessor; import org.springframework.beans.factory.annotation.Value; import org.springframework.boot.autoconfigure.condition.ConditionalOnExpression; +import org.springframework.boot.context.properties.EnableConfigurationProperties; import org.springframework.boot.data.redis.autoconfigure.DataRedisConnectionDetails; import org.springframework.boot.data.redis.autoconfigure.DataRedisProperties; import org.springframework.context.annotation.Bean; @@ -25,6 +26,7 @@ * back to running without it. TLS uses the JVM's trust store (Boot SSL bundles aren't passed on). */ @Configuration +@EnableConfigurationProperties(RateLimitProperties.class) public class RedissonConfig { static final String REDISSON_CLIENT = "redissonClient"; diff --git a/src/main/java/com/company/resourceapi/exceptions/RateLimitExceededException.java b/src/main/java/com/company/resourceapi/exceptions/RateLimitExceededException.java new file mode 100644 index 0000000..1163ea0 --- /dev/null +++ b/src/main/java/com/company/resourceapi/exceptions/RateLimitExceededException.java @@ -0,0 +1,20 @@ +package com.company.resourceapi.exceptions; + +import lombok.Getter; + +/** One of our own rate limits refused the request; it can be retried after {@link #getRetryAfterSeconds()}. */ +@Getter +public class RateLimitExceededException extends RuntimeException { + + /** Whose limit: the caller's own requests (429) or the application's calls to a system (503). */ + public enum Scope { CALLER, UPSTREAM } + + private final Scope scope; + private final long retryAfterSeconds; + + public RateLimitExceededException(Scope scope, String message, long retryAfterSeconds) { + super(message); + this.scope = scope; + this.retryAfterSeconds = retryAfterSeconds; + } +} diff --git a/src/main/java/com/company/resourceapi/services/ExternalProjectService.java b/src/main/java/com/company/resourceapi/services/ExternalProjectService.java index 415c1fc..f1d428e 100644 --- a/src/main/java/com/company/resourceapi/services/ExternalProjectService.java +++ b/src/main/java/com/company/resourceapi/services/ExternalProjectService.java @@ -4,7 +4,10 @@ import com.company.resourceapi.clients.ExternalProject; import com.company.resourceapi.clients.SdlcProjectClient; +import com.company.resourceapi.config.RateLimitProperties; import com.company.resourceapi.entities.SdlcSystem; +import com.company.resourceapi.exceptions.RateLimitExceededException; +import com.company.resourceapi.exceptions.RateLimitExceededException.Scope; import java.util.Optional; import lombok.RequiredArgsConstructor; import org.springframework.cache.annotation.CacheEvict; @@ -13,17 +16,22 @@ import org.springframework.cache.annotation.Caching; import org.springframework.stereotype.Service; -/** Projects as described by their SDLC system, cached per SDLC system and external id. */ +/** + * Projects as described by their SDLC system, cached per SDLC system and external id, and rate limited + * per SDLC system across all instances. + */ @Service @RequiredArgsConstructor public class ExternalProjectService { private final SdlcProjectClient sdlcProjectClient; + private final RateLimiters rateLimiters; + private final RateLimitProperties rateLimits; /** Cached; a project unknown to the SDLC system isn't cached, so it is found once it exists. */ @Cacheable(cacheNames = EXTERNAL_PROJECTS, key = "#sdlcSystem.id + ':' + #externalId", unless = "#result == null") public Optional findProject(SdlcSystem sdlcSystem, String externalId) { - return sdlcProjectClient.fetchProject(sdlcSystem, externalId); + return fetch(sdlcSystem, externalId); } /** @@ -37,6 +45,16 @@ public Optional findProject(SdlcSystem sdlcSystem, String exter evict = @CacheEvict(cacheNames = EXTERNAL_PROJECTS, key = "#sdlcSystem.id + ':' + #externalId", condition = "#result == null or #result.isEmpty()")) public Optional refreshProject(SdlcSystem sdlcSystem, String externalId) { + return fetch(sdlcSystem, externalId); + } + + /** Only real calls count against the SDLC system's rate limit, not cache hits. */ + private Optional fetch(SdlcSystem sdlcSystem, String externalId) { + var limit = rateLimits.sdlc(); + if (!rateLimiters.tryAcquire("sdlc-rate:" + sdlcSystem.getId(), limit)) { + throw new RateLimitExceededException(Scope.UPSTREAM, + "Rate limit for SdlcSystem %d reached.".formatted(sdlcSystem.getId()), limit.retryAfterSeconds()); + } return sdlcProjectClient.fetchProject(sdlcSystem, externalId); } } diff --git a/src/main/java/com/company/resourceapi/services/ProjectSyncService.java b/src/main/java/com/company/resourceapi/services/ProjectSyncService.java index 5dea543..06ab4b0 100644 --- a/src/main/java/com/company/resourceapi/services/ProjectSyncService.java +++ b/src/main/java/com/company/resourceapi/services/ProjectSyncService.java @@ -1,6 +1,9 @@ package com.company.resourceapi.services; +import com.company.resourceapi.config.RateLimitProperties; import com.company.resourceapi.exceptions.NotFoundException; +import com.company.resourceapi.exceptions.RateLimitExceededException; +import com.company.resourceapi.exceptions.RateLimitExceededException.Scope; import com.company.resourceapi.exceptions.SyncInProgressException; import lombok.RequiredArgsConstructor; import org.springframework.stereotype.Service; @@ -13,11 +16,14 @@ public class ProjectSyncService { private final ProjectService projectService; private final ProjectSyncLock projectSyncLock; private final ProjectSyncWorker projectSyncWorker; + private final RateLimiters rateLimiters; + private final RateLimitProperties rateLimits; /** * Starts refreshing the project from its SDLC system in the background. * * @throws NotFoundException if the project doesn't exist + * @throws RateLimitExceededException if the project was synced too often recently * @throws SyncInProgressException if a sync of the project is still running * @throws org.springframework.core.task.TaskRejectedException if no background slot is free */ @@ -27,6 +33,15 @@ public void requestSync(long projectId) throws NotFoundException { var lease = projectSyncLock.tryAcquire(projectId) .orElseThrow(() -> new SyncInProgressException(projectId)); + + // counted only for syncs that could run: an unknown project or a running sync doesn't use a permit + var limit = rateLimits.sync(); + if (!rateLimiters.tryAcquire("sync-rate:" + projectId, limit)) { + lease.release(); + throw new RateLimitExceededException(Scope.CALLER, + "Too many sync requests for project %d.".formatted(projectId), limit.retryAfterSeconds()); + } + try { // the background task releases the lock when it ends, however it ends projectSyncWorker.syncName(projectId).whenComplete((result, failure) -> lease.release()); diff --git a/src/main/java/com/company/resourceapi/services/RateLimiters.java b/src/main/java/com/company/resourceapi/services/RateLimiters.java new file mode 100644 index 0000000..be7399e --- /dev/null +++ b/src/main/java/com/company/resourceapi/services/RateLimiters.java @@ -0,0 +1,64 @@ +package com.company.resourceapi.services; + +import com.company.resourceapi.config.RateLimitProperties; +import java.time.Duration; +import java.util.Set; +import java.util.concurrent.ConcurrentHashMap; +import lombok.extern.slf4j.Slf4j; +import org.redisson.api.RRateLimiter; +import org.redisson.api.RateType; +import org.redisson.api.RedissonClient; +import org.springframework.beans.factory.ObjectProvider; +import org.springframework.stereotype.Component; + +/** + * Token-bucket rate limiters shared by all instances (Redisson {@link RRateLimiter}), one per name. + * + *

A limiter is created with the configured rate on first use, and updated when the configuration + * changed; its state expires after a while unused. Without Redis (unreachable, or no Redisson + * client) requests are allowed: limiting is a protection, losing it shouldn't stop the application. + */ +@Component +@Slf4j +public class RateLimiters { + + /** Unused limiters are removed from Redis after this. */ + private static final Duration KEEP_ALIVE = Duration.ofHours(1); + + private final RedissonClient redisson; + private final Set checkedConfigs = ConcurrentHashMap.newKeySet(); + + public RateLimiters(ObjectProvider redisson) { + this.redisson = redisson.getIfAvailable(); + } + + /** @return whether a permit was obtained, waiting at most the limit's max wait */ + public boolean tryAcquire(String name, RateLimitProperties.Limit limit) { + if (redisson == null) { + return true; + } + try { + var limiter = redisson.getRateLimiter(name); + ensureRate(name, limiter, limit); + return limiter.tryAcquireAsync(1, limit.maxWait()).toCompletableFuture().join(); + } catch (RuntimeException e) { + log.warn("Rate limiter {} unavailable, allowing the request: {}", name, e.getMessage()); + return true; + } + } + + private void ensureRate(String name, RRateLimiter limiter, RateLimitProperties.Limit limit) { + long keepAliveMillis = Math.max(KEEP_ALIVE.toMillis(), limit.period().toMillis()); + boolean created = limiter.trySetRateAsync(RateType.OVERALL, limit.permits(), limit.period(), + Duration.ofMillis(keepAliveMillis)).toCompletableFuture().join(); + // once per instance and limiter: an existing limiter keeps its old rate unless it is replaced + if (!created && checkedConfigs.add(name)) { + var config = limiter.getConfigAsync().toCompletableFuture().join(); + if (config.getRate() != limit.permits() || config.getRateInterval() != limit.period().toMillis()) { + log.info("Updating rate limiter {} to {} per {}", name, limit.permits(), limit.period()); + limiter.setRateAsync(RateType.OVERALL, limit.permits(), limit.period(), Duration.ofMillis(keepAliveMillis)) + .toCompletableFuture().join(); + } + } + } +} diff --git a/src/main/java/com/company/resourceapi/web/ApiExceptionHandler.java b/src/main/java/com/company/resourceapi/web/ApiExceptionHandler.java index 3630122..50ed5a8 100644 --- a/src/main/java/com/company/resourceapi/web/ApiExceptionHandler.java +++ b/src/main/java/com/company/resourceapi/web/ApiExceptionHandler.java @@ -4,8 +4,10 @@ import static org.springframework.http.HttpStatus.CONFLICT; import static org.springframework.http.HttpStatus.NOT_FOUND; import static org.springframework.http.HttpStatus.SERVICE_UNAVAILABLE; +import static org.springframework.http.HttpStatus.TOO_MANY_REQUESTS; import com.company.resourceapi.exceptions.NotFoundException; +import com.company.resourceapi.exceptions.RateLimitExceededException; import com.company.resourceapi.exceptions.SyncInProgressException; import lombok.extern.slf4j.Slf4j; import org.springframework.core.task.TaskRejectedException; @@ -31,6 +33,17 @@ ResponseEntity handleSyncInProgress(SyncInProgressException e) { return ResponseEntity.status(CONFLICT).build(); } + /** + * One of our rate limits: the caller's own requests are 429; our calls to an SDLC system being + * limited is a temporary unavailability on our side, 503. + */ + @ExceptionHandler(RateLimitExceededException.class) + ResponseEntity handleRateLimitExceeded(RateLimitExceededException e) { + log.info(e.getMessage()); + var status = e.getScope() == RateLimitExceededException.Scope.CALLER ? TOO_MANY_REQUESTS : SERVICE_UNAVAILABLE; + return ResponseEntity.status(status).header(RETRY_AFTER, String.valueOf(e.getRetryAfterSeconds())).build(); + } + /** The background task executor is at its concurrency limit; the client can retry shortly. */ @ExceptionHandler(TaskRejectedException.class) ResponseEntity handleTaskRejected(TaskRejectedException e) { diff --git a/src/main/resources/application.properties b/src/main/resources/application.properties index dc86186..bcf9293 100755 --- a/src/main/resources/application.properties +++ b/src/main/resources/application.properties @@ -27,6 +27,14 @@ resourceapi.cache.external-projects-ttl=5m resourceapi.redisson.key-prefix=resourceapi: # a project's sync lock expires after this even if never released (e.g. the instance died) resourceapi.sync.lock-lease-time=60s +# distributed rate limits (Redisson): calls to one SDLC system (503 + Retry-After when exceeded after +# waiting max-wait), and sync requests per project (429 + Retry-After) +resourceapi.rate-limit.sdlc.permits=10 +resourceapi.rate-limit.sdlc.period=1s +resourceapi.rate-limit.sdlc.max-wait=1s +resourceapi.rate-limit.sync.permits=6 +resourceapi.rate-limit.sync.period=1m +resourceapi.rate-limit.sync.max-wait=0s # datasource spring.datasource.url=jdbc:h2:mem:test;MODE=MySQL;DATABASE_TO_LOWER=TRUE spring.sql.init.platform=h2 diff --git a/src/test/java/com/company/resourceapi/ratelimit/RateLimitIntegrationTest.java b/src/test/java/com/company/resourceapi/ratelimit/RateLimitIntegrationTest.java new file mode 100644 index 0000000..b8610fd --- /dev/null +++ b/src/test/java/com/company/resourceapi/ratelimit/RateLimitIntegrationTest.java @@ -0,0 +1,180 @@ +package com.company.resourceapi.ratelimit; + +import static com.github.tomakehurst.wiremock.client.WireMock.getRequestedFor; +import static com.github.tomakehurst.wiremock.client.WireMock.okJson; +import static com.github.tomakehurst.wiremock.client.WireMock.urlPathMatching; +import static com.github.tomakehurst.wiremock.core.WireMockConfiguration.wireMockConfig; +import static org.assertj.core.api.Assertions.assertThat; +import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get; +import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.post; +import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.header; +import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status; + +import com.company.resourceapi.controllers.ProjectRestController; +import com.company.resourceapi.entities.Project; +import com.company.resourceapi.entities.SdlcSystem; +import com.company.resourceapi.repositories.ProjectRepository; +import com.company.resourceapi.repositories.SdlcSystemRepository; +import com.github.tomakehurst.wiremock.client.WireMock; +import com.github.tomakehurst.wiremock.junit5.WireMockExtension; +import java.time.Duration; +import java.time.Instant; +import java.util.concurrent.TimeUnit; +import java.util.function.BooleanSupplier; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.RegisterExtension; +import org.redisson.api.RateType; +import org.redisson.api.RedissonClient; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.boot.test.context.SpringBootTest; +import org.springframework.boot.webmvc.test.autoconfigure.AutoConfigureMockMvc; +import org.springframework.test.web.servlet.MockMvc; + +/** Distributed rate limits against a real Redis; "another instance" uses the same limiter directly. */ +@SpringBootTest(properties = { + "resourceapi.rate-limit.sdlc.permits=2", + "resourceapi.rate-limit.sdlc.period=2s", + "resourceapi.rate-limit.sdlc.max-wait=0s", + "resourceapi.rate-limit.sync.permits=2", + "resourceapi.rate-limit.sync.period=1m"}) +@AutoConfigureMockMvc +class RateLimitIntegrationTest { + + @RegisterExtension + static WireMockExtension sdlcSystemServer = WireMockExtension.newInstance() + .options(wireMockConfig().dynamicPort()) + .build(); + + @Autowired + private MockMvc mockMvc; + + @Autowired + private SdlcSystemRepository sdlcSystemRepository; + + @Autowired + private ProjectRepository projectRepository; + + @Autowired + private RedissonClient redisson; + + private SdlcSystem sdlcSystem; + + @BeforeEach + void setup() { + sdlcSystem = new SdlcSystem(); + sdlcSystem.setBaseUrl(sdlcSystemServer.baseUrl()); + sdlcSystem = sdlcSystemRepository.save(sdlcSystem); + sdlcSystemServer.stubFor(WireMock.get(urlPathMatching("/rest/api/2/project/.*")) + .willReturn(okJson("{\"key\": \"ANY\", \"name\": \"Synced\"}"))); + } + + @Test + void givenTooManyCallsToOneSdlcSystemThenFurtherCallsAreRefused() throws Exception { + mockMvc.perform(get(externalUrl(project("A")))).andExpect(status().isOk()); + mockMvc.perform(get(externalUrl(project("B")))).andExpect(status().isOk()); + mockMvc.perform(get(externalUrl(project("C")))) + .andExpect(status().isServiceUnavailable()) + .andExpect(header().string("Retry-After", "1")); + + sdlcSystemServer.verify(2, getRequestedFor(urlPathMatching("/rest/api/2/project/.*"))); + } + + @Test + void givenPermitsUsedByAnotherInstanceThenThisInstanceIsLimitedToo() throws Exception { + var shared = redisson.getRateLimiter("sdlc-rate:" + sdlcSystem.getId()); + shared.trySetRate(RateType.OVERALL, 2, Duration.ofSeconds(2), Duration.ofHours(1)); + assertThat(shared.tryAcquire(2)).isTrue(); + + mockMvc.perform(get(externalUrl(project("A")))).andExpect(status().isServiceUnavailable()); + + // the permits refill after the period + var url = externalUrl(project("B")); + await(() -> statusOf(url) == 200, Duration.ofSeconds(5)); + } + + @Test + void givenCachedAnswersThenOnlyRealCallsCount() throws Exception { + var url = externalUrl(project("A")); + for (int i = 0; i < 5; i++) { + mockMvc.perform(get(url)).andExpect(status().isOk()); + } + + sdlcSystemServer.verify(1, getRequestedFor(urlPathMatching("/rest/api/2/project/.*"))); + mockMvc.perform(get(externalUrl(project("B")))).andExpect(status().isOk()); + } + + @Test + void givenChangedConfigurationThenExistingLimiterIsUpdated() throws Exception { + var limiter = redisson.getRateLimiter("sdlc-rate:" + sdlcSystem.getId()); + limiter.trySetRate(RateType.OVERALL, 100, Duration.ofSeconds(1), Duration.ofHours(1)); + + mockMvc.perform(get(externalUrl(project("A")))).andExpect(status().isOk()); + + var config = limiter.getConfig(); + assertThat(config.getRate()).isEqualTo(2); + assertThat(config.getRateInterval()).isEqualTo(Duration.ofSeconds(2).toMillis()); + } + + @Test + void givenTooManySyncRequestsForOneProjectThenTheyAreRefused() throws Exception { + long projectId = project("SYNCED"); + var syncUrl = ProjectRestController.ENDPOINT + "/" + projectId + "/sync"; + + for (int i = 0; i < 2; i++) { + mockMvc.perform(post(syncUrl)).andExpect(status().isAccepted()); + // let it finish, so the lock doesn't refuse the next one + await(() -> !redisson.getLock("project-sync:" + projectId).isLocked(), Duration.ofSeconds(5)); + } + mockMvc.perform(post(syncUrl)) + .andExpect(status().isTooManyRequests()) + .andExpect(header().string("Retry-After", "30")); + } + + private long project(String externalId) { + var project = new Project(); + project.setExternalId(externalId); + project.setSdlcSystem(sdlcSystem); + return projectRepository.save(project).getId(); + } + + private static String externalUrl(long projectId) { + return ProjectRestController.ENDPOINT + "/" + projectId + "/external"; + } + + private int statusOf(String url) { + try { + return mockMvc.perform(get(url)).andReturn().getResponse().getStatus(); + } catch (Exception e) { + throw new IllegalStateException(e); + } + } + + private static void await(BooleanSupplier condition, Duration timeout) throws InterruptedException { + var deadline = Instant.now().plus(timeout); + while (!condition.getAsBoolean()) { + assertThat(Instant.now()).as("condition not met within " + timeout).isBefore(deadline); + Thread.sleep(100); + } + } + + @Test + void givenSyncAlreadyRunningThenRefusedRequestDoesNotUseAPermit() throws Exception { + long projectId = project("BUSY"); + var syncUrl = ProjectRestController.ENDPOINT + "/" + projectId + "/sync"; + var otherInstanceLock = redisson.getLock("project-sync:" + projectId); + otherInstanceLock.tryLockAsync(0, 10, TimeUnit.SECONDS, 4714L).toCompletableFuture().join(); + try { + for (int i = 0; i < 3; i++) { + mockMvc.perform(post(syncUrl)).andExpect(status().isConflict()); + } + } finally { + otherInstanceLock.unlockAsync(4714L).toCompletableFuture().join(); + } + + // both permits are still there + mockMvc.perform(post(syncUrl)).andExpect(status().isAccepted()); + // let that sync finish here, not during the next test + await(() -> !otherInstanceLock.isLocked(), Duration.ofSeconds(5)); + } +} diff --git a/src/test/java/com/company/resourceapi/services/ProjectSyncBackpressureTest.java b/src/test/java/com/company/resourceapi/services/ProjectSyncBackpressureTest.java index 0781f63..947f5fa 100644 --- a/src/test/java/com/company/resourceapi/services/ProjectSyncBackpressureTest.java +++ b/src/test/java/com/company/resourceapi/services/ProjectSyncBackpressureTest.java @@ -24,7 +24,10 @@ import org.springframework.test.web.servlet.MockMvc; /** Background syncs are bounded: beyond the executor's limit they are rejected, not queued. */ -@SpringBootTest(properties = "spring.task.execution.simple.concurrency-limit=1") +@SpringBootTest(properties = { + "spring.task.execution.simple.concurrency-limit=1", + // this test retries refused syncs; the per-project sync rate limit isn't what it is about + "resourceapi.rate-limit.sync.permits=1000"}) @AutoConfigureMockMvc class ProjectSyncBackpressureTest { From 3d34afc3831cf708ac93c75295e77063933fd9c9 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 31 Oct 2026 10:31:42 +0000 Subject: [PATCH 06/13] Address review: sliding-window Retry-After, fewer Redis calls, validated limits - Redisson's RRateLimiter is a sliding window (a used permit returns one period later): Retry-After is now the full period (was period/permits, so clients retried into another 429); README says sliding window - A limiter is set up once per instance (trySetRate no longer on every call), and again only when Redis dropped it after expiring; it is marked set up only after that succeeded, so a failed update is retried - The set of set-up limiters is bounded (LRU, 10,000) - Rate limit properties are validated at startup (@Positive permits, required period and max-wait) - README: a sync refused for a full executor (503) does use a permit - Tests: removed/expired limiter is set up again (verified to fail without it); invalid limits rejected; Retry-After is one full window Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_012MLyPXGHeCc1fvTeB8JnT2 --- README.md | 7 +- .../config/RateLimitProperties.java | 19 ++++-- .../resourceapi/services/RateLimiters.java | 67 ++++++++++++++----- .../ratelimit/RateLimitIntegrationTest.java | 19 +++++- .../ratelimit/RateLimitPropertiesTest.java | 31 +++++++++ 5 files changed, 117 insertions(+), 26 deletions(-) create mode 100644 src/test/java/com/company/resourceapi/ratelimit/RateLimitPropertiesTest.java diff --git a/README.md b/README.md index 0c11806..3b7c054 100644 --- a/README.md +++ b/README.md @@ -97,15 +97,16 @@ standalone Redis is supported for the lock: with Sentinel or Cluster the applica run unlocked; TLS uses the JVM's trust store (Boot SSL bundles aren't passed to Redisson). (Redisson's Spring Boot starter isn't used: it targets Spring Boot 3. Redisson brings a JCache provider, so `spring.cache.type=redis` is set explicitly.) -**Rate limits** (Redisson `RRateLimiter`, token buckets shared by all instances): +**Rate limits** (Redisson `RRateLimiter`, sliding windows shared by all instances: a used permit becomes +available again one period later, which is also what `Retry-After` says): | Limit | Default | When exceeded | |---|---|---| | Calls to one SDLC system (`resourceapi.rate-limit.sdlc.*`), protecting its API quota; cache hits don't count | 10 per 1s, waiting up to 1s for a permit | `503` + `Retry-After` (our own limit, not the SDLC system's failure); a background sync logs it | -| `POST /api/v2/projects/{id}/sync` per project (`resourceapi.rate-limit.sync.*`); requests refused anyway (unknown project, sync already running) don't count | 6 per minute | `429` + `Retry-After` | +| `POST /api/v2/projects/{id}/sync` per project (`resourceapi.rate-limit.sync.*`); requests refused anyway (unknown project, sync already running) don't count, one refused because no background slot is free (`503`) does | 6 per minute | `429` + `Retry-After` | A changed rate is applied to the existing limiter when an instance first uses it; unused limiters expire from -Redis after an hour. If Redis can't be reached, requests are allowed (limiting protects, losing it shouldn't +Redis after an hour. Invalid limits (e.g. `permits=0`) stop the application at startup. If Redis can't be reached, requests are allowed (limiting protects, losing it shouldn't stop the application). **Local development**: `./mvnw spring-boot:run` starts Redis from `compose.yaml` automatically diff --git a/src/main/java/com/company/resourceapi/config/RateLimitProperties.java b/src/main/java/com/company/resourceapi/config/RateLimitProperties.java index 7d1f95a..37e3cba 100644 --- a/src/main/java/com/company/resourceapi/config/RateLimitProperties.java +++ b/src/main/java/com/company/resourceapi/config/RateLimitProperties.java @@ -1,27 +1,36 @@ package com.company.resourceapi.config; +import jakarta.validation.Valid; +import jakarta.validation.constraints.NotNull; +import jakarta.validation.constraints.Positive; import java.time.Duration; import org.springframework.boot.context.properties.ConfigurationProperties; +import org.springframework.validation.annotation.Validated; /** * Distributed rate limits ({@code resourceapi.rate-limit.*}), shared by all instances through Redis. + * Validated at startup: a broken limit would otherwise fail, or silently stop limiting, at runtime. * * @param sdlc calls to one SDLC system (protects its API quota) * @param sync sync requests for one project */ +@Validated @ConfigurationProperties("resourceapi.rate-limit") -public record RateLimitProperties(Limit sdlc, Limit sync) { +public record RateLimitProperties(@NotNull @Valid Limit sdlc, @NotNull @Valid Limit sync) { /** + * At most {@code permits} per sliding window of {@code period}: each permit becomes available again + * one period after it was used. + * * @param permits how many per period - * @param period the period the permits refill over + * @param period the sliding window * @param maxWait how long a caller may wait for a permit before being refused */ - public record Limit(long permits, Duration period, Duration maxWait) { + public record Limit(@Positive long permits, @NotNull Duration period, @NotNull Duration maxWait) { - /** Roughly when the next permit is available, for a Retry-After header. */ + /** When a permit is surely available again: a used permit only comes back after a full period. */ public long retryAfterSeconds() { - return Math.max(1, (long) Math.ceil(period.toMillis() / 1000.0 / permits)); + return Math.max(1, (long) Math.ceil(period.toMillis() / 1000.0)); } } } diff --git a/src/main/java/com/company/resourceapi/services/RateLimiters.java b/src/main/java/com/company/resourceapi/services/RateLimiters.java index be7399e..f5bcefa 100644 --- a/src/main/java/com/company/resourceapi/services/RateLimiters.java +++ b/src/main/java/com/company/resourceapi/services/RateLimiters.java @@ -2,8 +2,9 @@ import com.company.resourceapi.config.RateLimitProperties; import java.time.Duration; -import java.util.Set; -import java.util.concurrent.ConcurrentHashMap; +import java.util.Collections; +import java.util.LinkedHashMap; +import java.util.Map; import lombok.extern.slf4j.Slf4j; import org.redisson.api.RRateLimiter; import org.redisson.api.RateType; @@ -12,11 +13,12 @@ import org.springframework.stereotype.Component; /** - * Token-bucket rate limiters shared by all instances (Redisson {@link RRateLimiter}), one per name. + * Sliding-window rate limiters shared by all instances (Redisson {@link RRateLimiter}), one per name. * - *

A limiter is created with the configured rate on first use, and updated when the configuration - * changed; its state expires after a while unused. Without Redis (unreachable, or no Redisson - * client) requests are allowed: limiting is a protection, losing it shouldn't stop the application. + *

A limiter is set up with the configured rate the first time this instance uses it (updating a + * limiter created with another rate), and again if Redis dropped it after it expired from disuse. + * Without Redis (unreachable, or no Redisson client) requests are allowed: limiting is a protection, + * losing it shouldn't stop the application. */ @Component @Slf4j @@ -24,9 +26,16 @@ public class RateLimiters { /** Unused limiters are removed from Redis after this. */ private static final Duration KEEP_ALIVE = Duration.ofHours(1); + /** Limiters this instance has set up, most recently used kept (bounded: one per project). */ + private static final int MAX_REMEMBERED = 10_000; private final RedissonClient redisson; - private final Set checkedConfigs = ConcurrentHashMap.newKeySet(); + private final Map setUp = Collections.synchronizedMap(new LinkedHashMap<>(16, 0.75f, true) { + @Override + protected boolean removeEldestEntry(Map.Entry eldest) { + return size() > MAX_REMEMBERED; + } + }); public RateLimiters(ObjectProvider redisson) { this.redisson = redisson.getIfAvailable(); @@ -39,26 +48,52 @@ public boolean tryAcquire(String name, RateLimitProperties.Limit limit) { } try { var limiter = redisson.getRateLimiter(name); - ensureRate(name, limiter, limit); - return limiter.tryAcquireAsync(1, limit.maxWait()).toCompletableFuture().join(); + if (!setUp.containsKey(name)) { + setUp(name, limiter, limit); + } + try { + return acquire(limiter, limit); + } catch (RuntimeException e) { + if (!isNotInitialized(e)) { + throw e; + } + // expired from Redis since this instance set it up + setUp(name, limiter, limit); + return acquire(limiter, limit); + } } catch (RuntimeException e) { log.warn("Rate limiter {} unavailable, allowing the request: {}", name, e.getMessage()); return true; } } - private void ensureRate(String name, RRateLimiter limiter, RateLimitProperties.Limit limit) { - long keepAliveMillis = Math.max(KEEP_ALIVE.toMillis(), limit.period().toMillis()); - boolean created = limiter.trySetRateAsync(RateType.OVERALL, limit.permits(), limit.period(), - Duration.ofMillis(keepAliveMillis)).toCompletableFuture().join(); - // once per instance and limiter: an existing limiter keeps its old rate unless it is replaced - if (!created && checkedConfigs.add(name)) { + private static boolean acquire(RRateLimiter limiter, RateLimitProperties.Limit limit) { + return limiter.tryAcquireAsync(1, limit.maxWait()).toCompletableFuture().join(); + } + + private void setUp(String name, RRateLimiter limiter, RateLimitProperties.Limit limit) { + var keepAlive = Duration.ofMillis(Math.max(KEEP_ALIVE.toMillis(), limit.period().toMillis())); + boolean created = limiter.trySetRateAsync(RateType.OVERALL, limit.permits(), limit.period(), keepAlive) + .toCompletableFuture().join(); + if (!created) { + // an existing limiter keeps its rate unless replaced, e.g. after the configuration changed var config = limiter.getConfigAsync().toCompletableFuture().join(); if (config.getRate() != limit.permits() || config.getRateInterval() != limit.period().toMillis()) { log.info("Updating rate limiter {} to {} per {}", name, limit.permits(), limit.period()); - limiter.setRateAsync(RateType.OVERALL, limit.permits(), limit.period(), Duration.ofMillis(keepAliveMillis)) + limiter.setRateAsync(RateType.OVERALL, limit.permits(), limit.period(), keepAlive) .toCompletableFuture().join(); } } + // only once it is set up: a failure above is retried on the next call + setUp.put(name, Boolean.TRUE); + } + + private static boolean isNotInitialized(Throwable e) { + for (Throwable cause = e; cause != null; cause = cause.getCause()) { + if (cause.getMessage() != null && cause.getMessage().contains("not initialized")) { + return true; + } + } + return false; } } diff --git a/src/test/java/com/company/resourceapi/ratelimit/RateLimitIntegrationTest.java b/src/test/java/com/company/resourceapi/ratelimit/RateLimitIntegrationTest.java index b8610fd..0c18a57 100644 --- a/src/test/java/com/company/resourceapi/ratelimit/RateLimitIntegrationTest.java +++ b/src/test/java/com/company/resourceapi/ratelimit/RateLimitIntegrationTest.java @@ -75,7 +75,7 @@ void givenTooManyCallsToOneSdlcSystemThenFurtherCallsAreRefused() throws Excepti mockMvc.perform(get(externalUrl(project("B")))).andExpect(status().isOk()); mockMvc.perform(get(externalUrl(project("C")))) .andExpect(status().isServiceUnavailable()) - .andExpect(header().string("Retry-After", "1")); + .andExpect(header().string("Retry-After", "2")); sdlcSystemServer.verify(2, getRequestedFor(urlPathMatching("/rest/api/2/project/.*"))); } @@ -128,7 +128,7 @@ void givenTooManySyncRequestsForOneProjectThenTheyAreRefused() throws Exception } mockMvc.perform(post(syncUrl)) .andExpect(status().isTooManyRequests()) - .andExpect(header().string("Retry-After", "30")); + .andExpect(header().string("Retry-After", "60")); } private long project(String externalId) { @@ -177,4 +177,19 @@ void givenSyncAlreadyRunningThenRefusedRequestDoesNotUseAPermit() throws Excepti // let that sync finish here, not during the next test await(() -> !otherInstanceLock.isLocked(), Duration.ofSeconds(5)); } + + @Test + void givenLimiterRemovedFromRedisThenItIsSetUpAgain() throws Exception { + mockMvc.perform(get(externalUrl(project("A")))).andExpect(status().isOk()); + var limiter = redisson.getRateLimiter("sdlc-rate:" + sdlcSystem.getId()); + // expired from disuse (or flushed) + assertThat(limiter.delete()).isTrue(); + + mockMvc.perform(get(externalUrl(project("B")))).andExpect(status().isOk()); + + assertThat(limiter.getConfig().getRate()).isEqualTo(2); + // a fresh window: B used one permit, C uses the second, D is refused + mockMvc.perform(get(externalUrl(project("C")))).andExpect(status().isOk()); + mockMvc.perform(get(externalUrl(project("D")))).andExpect(status().isServiceUnavailable()); + } } diff --git a/src/test/java/com/company/resourceapi/ratelimit/RateLimitPropertiesTest.java b/src/test/java/com/company/resourceapi/ratelimit/RateLimitPropertiesTest.java new file mode 100644 index 0000000..b6a8b81 --- /dev/null +++ b/src/test/java/com/company/resourceapi/ratelimit/RateLimitPropertiesTest.java @@ -0,0 +1,31 @@ +package com.company.resourceapi.ratelimit; + +import static org.assertj.core.api.Assertions.assertThat; + +import com.company.resourceapi.config.RateLimitProperties; +import com.company.resourceapi.config.RateLimitProperties.Limit; +import jakarta.validation.Validation; +import java.time.Duration; +import org.junit.jupiter.api.Test; + +class RateLimitPropertiesTest { + + private final jakarta.validation.Validator validator = Validation.buildDefaultValidatorFactory().getValidator(); + + @Test + void givenInvalidLimitsThenTheyAreRejected() { + var noPermits = new Limit(0, Duration.ofSeconds(1), Duration.ZERO); + var noPeriod = new Limit(5, null, Duration.ZERO); + var valid = new Limit(5, Duration.ofSeconds(1), Duration.ZERO); + + assertThat(validator.validate(new RateLimitProperties(noPermits, valid))).hasSize(1); + assertThat(validator.validate(new RateLimitProperties(valid, noPeriod))).hasSize(1); + assertThat(validator.validate(new RateLimitProperties(valid, valid))).isEmpty(); + } + + @Test + void givenLimitThenRetryAfterIsOneFullWindow() { + assertThat(new Limit(6, Duration.ofMinutes(1), Duration.ZERO).retryAfterSeconds()).isEqualTo(60); + assertThat(new Limit(10, Duration.ofMillis(500), Duration.ZERO).retryAfterSeconds()).isEqualTo(1); + } +} From 1bcec63fd1c836c0c07dfa0e23fef228eeb6c3a9 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 31 Oct 2026 10:34:53 +0000 Subject: [PATCH 07/13] Quieter startup: infrastructure-role cache error handler, no Redis repositories - The CachingConfigurer (error handler) is its own infrastructure-role configuration, so its early creation by the caching infrastructure no longer logs 'not eligible for getting processed by all BeanPostProcessors' - spring.data.redis.repositories.enabled=false: Redis is only a cache, so the JPA repositories aren't inspected as Redis repository candidates Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_012MLyPXGHeCc1fvTeB8JnT2 --- .../resourceapi/config/CacheConfig.java | 19 +++++++++++++++---- src/main/resources/application.properties | 2 ++ 2 files changed, 17 insertions(+), 4 deletions(-) diff --git a/src/main/java/com/company/resourceapi/config/CacheConfig.java b/src/main/java/com/company/resourceapi/config/CacheConfig.java index e4a80d4..b70a71d 100644 --- a/src/main/java/com/company/resourceapi/config/CacheConfig.java +++ b/src/main/java/com/company/resourceapi/config/CacheConfig.java @@ -1,5 +1,6 @@ package com.company.resourceapi.config; +import org.springframework.beans.factory.config.BeanDefinition; import org.springframework.boot.cache.autoconfigure.RedisCacheManagerBuilderCustomizer; import org.springframework.boot.context.properties.EnableConfigurationProperties; import org.springframework.cache.annotation.CachingConfigurer; @@ -8,6 +9,7 @@ import org.springframework.cache.interceptor.LoggingCacheErrorHandler; import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.Configuration; +import org.springframework.context.annotation.Role; import org.springframework.data.redis.serializer.GenericJacksonJsonRedisSerializer; import org.springframework.data.redis.serializer.RedisSerializationContext.SerializationPair; import tools.jackson.databind.jsontype.BasicPolymorphicTypeValidator; @@ -20,7 +22,7 @@ @Configuration @EnableCaching @EnableConfigurationProperties(CacheTtlProperties.class) -public class CacheConfig implements CachingConfigurer { +public class CacheConfig { public static final String SDLC_SYSTEMS = "sdlc-systems"; public static final String EXTERNAL_PROJECTS = "external-projects"; @@ -47,8 +49,17 @@ RedisCacheManagerBuilderCustomizer redisCacheCustomizer(CacheTtlProperties ttl) }; } - @Override - public CacheErrorHandler errorHandler() { - return new LoggingCacheErrorHandler(); + /** + * Caching infrastructure reads this before regular beans exist; as an infrastructure bean of its + * own, without dependencies, that early creation is expected and not reported as a problem. + */ + @Configuration + @Role(BeanDefinition.ROLE_INFRASTRUCTURE) + static class CacheErrorHandling implements CachingConfigurer { + + @Override + public CacheErrorHandler errorHandler() { + return new LoggingCacheErrorHandler(); + } } } diff --git a/src/main/resources/application.properties b/src/main/resources/application.properties index 150cb61..590a7ff 100755 --- a/src/main/resources/application.properties +++ b/src/main/resources/application.properties @@ -17,6 +17,8 @@ spring.task.execution.shutdown.await-termination-period=10s # (Lettuce's default command timeout is 60s) spring.data.redis.connect-timeout=500ms spring.data.redis.timeout=500ms +# Redis is used for caching only, not as a Spring Data repository store +spring.data.redis.repositories.enabled=false spring.cache.redis.key-prefix=resourceapi:: spring.cache.redis.cache-null-values=false resourceapi.cache.sdlc-systems-ttl=10m From 5c1f8a051b1c109eb0d7965af15a6bbac11338a9 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 31 Oct 2026 10:37:01 +0000 Subject: [PATCH 08/13] Redisson: set username, password and name mapper on Config They are deprecated on the server config (logged as warnings at startup). Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_012MLyPXGHeCc1fvTeB8JnT2 --- .../company/resourceapi/config/RedissonConfig.java | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/main/java/com/company/resourceapi/config/RedissonConfig.java b/src/main/java/com/company/resourceapi/config/RedissonConfig.java index 4d08765..96bf712 100644 --- a/src/main/java/com/company/resourceapi/config/RedissonConfig.java +++ b/src/main/java/com/company/resourceapi/config/RedissonConfig.java @@ -35,13 +35,15 @@ public class RedissonConfig { RedissonClient redissonClient(DataRedisConnectionDetails connection, DataRedisProperties redis, @Value("${resourceapi.redisson.key-prefix}") String keyPrefix) { var standalone = connection.getStandalone(); - var config = new Config().setLazyInitialization(true); + var config = new Config() + .setLazyInitialization(true) + .setUsername(connection.getUsername()) + .setPassword(connection.getPassword()) + .setNameMapper(new KeyPrefixNameMapper(keyPrefix)); config.useSingleServer() .setAddress((connection.getSslBundle() != null ? "rediss://" : "redis://") + standalone.getHost() + ":" + standalone.getPort()) .setDatabase(standalone.getDatabase()) - .setUsername(connection.getUsername()) - .setPassword(connection.getPassword()) .setConnectTimeout(millis(redis.getConnectTimeout(), Duration.ofSeconds(10))) .setTimeout(millis(redis.getTimeout(), Duration.ofSeconds(3))) // fail fast like the cache: callers decide what to do without Redis @@ -50,8 +52,7 @@ RedissonClient redissonClient(DataRedisConnectionDetails connection, DataRedisPr .setConnectionMinimumIdleSize(1) .setConnectionPoolSize(8) .setSubscriptionConnectionMinimumIdleSize(1) - .setSubscriptionConnectionPoolSize(4) - .setNameMapper(new KeyPrefixNameMapper(keyPrefix)); + .setSubscriptionConnectionPoolSize(4); return Redisson.create(config); } From 801a766840b5fefee82195331d6ae257e2b0f092 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 31 Oct 2026 10:50:45 +0000 Subject: [PATCH 09/13] Remember unknown SDLC projects briefly; rate limit API requests per client - external-projects-missing cache (30s): a project the SDLC system doesn't know is answered from the cache instead of asking again each time, and found soon after it is created there; a sync replaces it with the project. Used directly through the CacheManager, so failures are caught and logged like the cache error handler does - ClientRateLimitFilter on /api/* (FilterRegistrationBean, after the trace filter): requests per client IP across all instances (100/s, 429 + Retry-After); server.forward-headers-strategy=native takes the client IP from X-Forwarded-For only from internal proxy addresses - Tests: unknown answer remembered then found after expiry, sync clears it; per-client 429 with Retry-After and X-Trace-Id, separate per client, non-API paths unlimited; the repeated-lookup test counts calls after the first lookup, as the HTTP client may retry a GET on a stale connection Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_012MLyPXGHeCc1fvTeB8JnT2 --- README.md | 7 ++- .../resourceapi/config/CacheConfig.java | 4 +- .../config/CacheTtlProperties.java | 5 +- .../config/RateLimitProperties.java | 7 ++- .../services/ExternalProjectService.java | 55 ++++++++++++++++++- .../web/ClientRateLimitConfig.java | 23 ++++++++ .../web/ClientRateLimitFilter.java | 44 +++++++++++++++ src/main/resources/application.properties | 7 +++ .../cache/CacheIntegrationTest.java | 52 ++++++++++++++++-- .../ClientRateLimitIntegrationTest.java | 55 +++++++++++++++++++ .../ratelimit/RateLimitPropertiesTest.java | 6 +- .../resources/config/application.properties | 2 + 12 files changed, 247 insertions(+), 20 deletions(-) create mode 100644 src/main/java/com/company/resourceapi/web/ClientRateLimitConfig.java create mode 100644 src/main/java/com/company/resourceapi/web/ClientRateLimitFilter.java create mode 100644 src/test/java/com/company/resourceapi/ratelimit/ClientRateLimitIntegrationTest.java diff --git a/README.md b/README.md index 3b7c054..f7249d9 100644 --- a/README.md +++ b/README.md @@ -79,10 +79,10 @@ The application uses Redis (`spring.data.redis.host`/`port`, default `localhost: | Cache | What | TTL | |---|---|---| | `sdlc-systems` | SDLC systems by id, read on every create, update and SDLC call | `resourceapi.cache.sdlc-systems-ttl` (10m) | -| `external-projects` | `GET /api/v2/projects/{id}/external` answers per SDLC system and external id; "unknown" answers are not cached | `resourceapi.cache.external-projects-ttl` (5m) | +| `external-projects` | `GET /api/v2/projects/{id}/external` answers per SDLC system and external id | `resourceapi.cache.external-projects-ttl` (5m) | +| `external-projects-missing` | "the SDLC system doesn't know this project" answers, kept briefly so a project created there is found soon | `resourceapi.cache.missing-external-projects-ttl` (30s) | -A background sync always asks the SDLC system and refreshes the cached answer (or removes it when the SDLC -system no longer knows the project). SDLC systems are not changed +A background sync always asks the SDLC system and refreshes both caches with its answer. SDLC systems are not changed through this API; one changed directly in the database is picked up when its entry expires. The cache is an optimization: if Redis is unreachable, cache reads and writes are logged and skipped (each waits at most the 500 ms Redis timeouts) and requests are served without it. @@ -104,6 +104,7 @@ available again one period later, which is also what `Retry-After` says): |---|---|---| | Calls to one SDLC system (`resourceapi.rate-limit.sdlc.*`), protecting its API quota; cache hits don't count | 10 per 1s, waiting up to 1s for a permit | `503` + `Retry-After` (our own limit, not the SDLC system's failure); a background sync logs it | | `POST /api/v2/projects/{id}/sync` per project (`resourceapi.rate-limit.sync.*`); requests refused anyway (unknown project, sync already running) don't count, one refused because no background slot is free (`503`) does | 6 per minute | `429` + `Retry-After` | +| All `/api/**` requests per client IP (`resourceapi.rate-limit.client.*`); behind a reverse proxy the IP comes from `X-Forwarded-For`, trusted only from internal proxy addresses (`server.forward-headers-strategy=native`) | 100 per second | `429` + `Retry-After` | A changed rate is applied to the existing limiter when an instance first uses it; unused limiters expire from Redis after an hour. Invalid limits (e.g. `permits=0`) stop the application at startup. If Redis can't be reached, requests are allowed (limiting protects, losing it shouldn't diff --git a/src/main/java/com/company/resourceapi/config/CacheConfig.java b/src/main/java/com/company/resourceapi/config/CacheConfig.java index b70a71d..0131fbd 100644 --- a/src/main/java/com/company/resourceapi/config/CacheConfig.java +++ b/src/main/java/com/company/resourceapi/config/CacheConfig.java @@ -26,6 +26,7 @@ public class CacheConfig { public static final String SDLC_SYSTEMS = "sdlc-systems"; public static final String EXTERNAL_PROJECTS = "external-projects"; + public static final String MISSING_EXTERNAL_PROJECTS = "external-projects-missing"; /** * Starts from Boot's defaults (spring.cache.redis.*: key prefix, null values, ...) and only @@ -45,7 +46,8 @@ RedisCacheManagerBuilderCustomizer redisCacheCustomizer(CacheTtlProperties ttl) var defaults = builder.cacheDefaults().serializeValuesWith(SerializationPair.fromSerializer(serializer)); builder.cacheDefaults(defaults) .withCacheConfiguration(SDLC_SYSTEMS, defaults.entryTtl(ttl.sdlcSystemsTtl())) - .withCacheConfiguration(EXTERNAL_PROJECTS, defaults.entryTtl(ttl.externalProjectsTtl())); + .withCacheConfiguration(EXTERNAL_PROJECTS, defaults.entryTtl(ttl.externalProjectsTtl())) + .withCacheConfiguration(MISSING_EXTERNAL_PROJECTS, defaults.entryTtl(ttl.missingExternalProjectsTtl())); }; } diff --git a/src/main/java/com/company/resourceapi/config/CacheTtlProperties.java b/src/main/java/com/company/resourceapi/config/CacheTtlProperties.java index e1399f2..597d037 100644 --- a/src/main/java/com/company/resourceapi/config/CacheTtlProperties.java +++ b/src/main/java/com/company/resourceapi/config/CacheTtlProperties.java @@ -9,9 +9,12 @@ * * @param sdlcSystemsTtl SDLC systems by id; they change rarely and only outside this API * @param externalProjectsTtl projects as described by their SDLC system ({@code GET .../external}) + * @param missingExternalProjectsTtl "the SDLC system doesn't know this project" answers; short, so a + * project created there is found soon */ @ConfigurationProperties("resourceapi.cache") public record CacheTtlProperties( @DefaultValue("10m") Duration sdlcSystemsTtl, - @DefaultValue("5m") Duration externalProjectsTtl) { + @DefaultValue("5m") Duration externalProjectsTtl, + @DefaultValue("30s") Duration missingExternalProjectsTtl) { } diff --git a/src/main/java/com/company/resourceapi/config/RateLimitProperties.java b/src/main/java/com/company/resourceapi/config/RateLimitProperties.java index 37e3cba..2ed983f 100644 --- a/src/main/java/com/company/resourceapi/config/RateLimitProperties.java +++ b/src/main/java/com/company/resourceapi/config/RateLimitProperties.java @@ -11,12 +11,13 @@ * Distributed rate limits ({@code resourceapi.rate-limit.*}), shared by all instances through Redis. * Validated at startup: a broken limit would otherwise fail, or silently stop limiting, at runtime. * - * @param sdlc calls to one SDLC system (protects its API quota) - * @param sync sync requests for one project + * @param sdlc calls to one SDLC system (protects its API quota) + * @param sync sync requests for one project + * @param client API requests from one client (IP address) */ @Validated @ConfigurationProperties("resourceapi.rate-limit") -public record RateLimitProperties(@NotNull @Valid Limit sdlc, @NotNull @Valid Limit sync) { +public record RateLimitProperties(@NotNull @Valid Limit sdlc, @NotNull @Valid Limit sync, @NotNull @Valid Limit client) { /** * At most {@code permits} per sliding window of {@code period}: each permit becomes available again diff --git a/src/main/java/com/company/resourceapi/services/ExternalProjectService.java b/src/main/java/com/company/resourceapi/services/ExternalProjectService.java index f1d428e..7b064e1 100644 --- a/src/main/java/com/company/resourceapi/services/ExternalProjectService.java +++ b/src/main/java/com/company/resourceapi/services/ExternalProjectService.java @@ -1,6 +1,7 @@ package com.company.resourceapi.services; import static com.company.resourceapi.config.CacheConfig.EXTERNAL_PROJECTS; +import static com.company.resourceapi.config.CacheConfig.MISSING_EXTERNAL_PROJECTS; import com.company.resourceapi.clients.ExternalProject; import com.company.resourceapi.clients.SdlcProjectClient; @@ -10,6 +11,9 @@ import com.company.resourceapi.exceptions.RateLimitExceededException.Scope; import java.util.Optional; import lombok.RequiredArgsConstructor; +import lombok.extern.slf4j.Slf4j; +import org.springframework.cache.Cache; +import org.springframework.cache.CacheManager; import org.springframework.cache.annotation.CacheEvict; import org.springframework.cache.annotation.CachePut; import org.springframework.cache.annotation.Cacheable; @@ -22,16 +26,27 @@ */ @Service @RequiredArgsConstructor +@Slf4j public class ExternalProjectService { private final SdlcProjectClient sdlcProjectClient; private final RateLimiters rateLimiters; private final RateLimitProperties rateLimits; + private final CacheManager cacheManager; - /** Cached; a project unknown to the SDLC system isn't cached, so it is found once it exists. */ + /** + * Cached. A project unknown to the SDLC system is remembered separately and briefly, so repeated + * lookups don't each call the SDLC system, while a project created there is still found soon. + */ @Cacheable(cacheNames = EXTERNAL_PROJECTS, key = "#sdlcSystem.id + ':' + #externalId", unless = "#result == null") public Optional findProject(SdlcSystem sdlcSystem, String externalId) { - return fetch(sdlcSystem, externalId); + var key = key(sdlcSystem, externalId); + if (isKnownMissing(key)) { + return Optional.empty(); + } + var project = fetch(sdlcSystem, externalId); + rememberMissing(key, project.isEmpty()); + return project; } /** @@ -45,7 +60,41 @@ public Optional findProject(SdlcSystem sdlcSystem, String exter evict = @CacheEvict(cacheNames = EXTERNAL_PROJECTS, key = "#sdlcSystem.id + ':' + #externalId", condition = "#result == null or #result.isEmpty()")) public Optional refreshProject(SdlcSystem sdlcSystem, String externalId) { - return fetch(sdlcSystem, externalId); + var project = fetch(sdlcSystem, externalId); + rememberMissing(key(sdlcSystem, externalId), project.isEmpty()); + return project; + } + + // The "missing" cache is used directly, which bypasses the cache error handler: failures are + // handled here the same way, by working without it. + + private boolean isKnownMissing(String key) { + try { + return missingCache().get(key) != null; + } catch (RuntimeException e) { + log.warn("Cache '{}' failed to get entry with key '{}': {}", MISSING_EXTERNAL_PROJECTS, key, e.getMessage()); + return false; + } + } + + private void rememberMissing(String key, boolean missing) { + try { + if (missing) { + missingCache().put(key, Boolean.TRUE); + } else { + missingCache().evict(key); + } + } catch (RuntimeException e) { + log.warn("Cache '{}' failed to update entry with key '{}': {}", MISSING_EXTERNAL_PROJECTS, key, e.getMessage()); + } + } + + private Cache missingCache() { + return cacheManager.getCache(MISSING_EXTERNAL_PROJECTS); + } + + private static String key(SdlcSystem sdlcSystem, String externalId) { + return sdlcSystem.getId() + ":" + externalId; } /** Only real calls count against the SDLC system's rate limit, not cache hits. */ diff --git a/src/main/java/com/company/resourceapi/web/ClientRateLimitConfig.java b/src/main/java/com/company/resourceapi/web/ClientRateLimitConfig.java new file mode 100644 index 0000000..5cb3f82 --- /dev/null +++ b/src/main/java/com/company/resourceapi/web/ClientRateLimitConfig.java @@ -0,0 +1,23 @@ +package com.company.resourceapi.web; + +import com.company.resourceapi.config.RateLimitProperties; +import com.company.resourceapi.services.RateLimiters; +import org.springframework.boot.web.servlet.FilterRegistrationBean; +import org.springframework.context.annotation.Bean; +import org.springframework.context.annotation.Configuration; +import org.springframework.core.Ordered; + +/** Registers {@link ClientRateLimitFilter} for the API only (not Swagger UI or the OpenAPI document). */ +@Configuration +public class ClientRateLimitConfig { + + @Bean + FilterRegistrationBean clientRateLimitFilter(RateLimiters rateLimiters, + RateLimitProperties rateLimits) { + var registration = new FilterRegistrationBean<>(new ClientRateLimitFilter(rateLimiters, rateLimits)); + registration.addUrlPatterns("/api/*"); + // after the server observation (+1) and trace id (+2) filters + registration.setOrder(Ordered.HIGHEST_PRECEDENCE + 3); + return registration; + } +} diff --git a/src/main/java/com/company/resourceapi/web/ClientRateLimitFilter.java b/src/main/java/com/company/resourceapi/web/ClientRateLimitFilter.java new file mode 100644 index 0000000..e0637d8 --- /dev/null +++ b/src/main/java/com/company/resourceapi/web/ClientRateLimitFilter.java @@ -0,0 +1,44 @@ +package com.company.resourceapi.web; + +import static org.springframework.http.HttpHeaders.RETRY_AFTER; + +import com.company.resourceapi.config.RateLimitProperties; +import com.company.resourceapi.services.RateLimiters; +import jakarta.servlet.FilterChain; +import jakarta.servlet.ServletException; +import jakarta.servlet.http.HttpServletRequest; +import jakarta.servlet.http.HttpServletResponse; +import java.io.IOException; +import lombok.RequiredArgsConstructor; +import lombok.extern.slf4j.Slf4j; +import org.springframework.http.HttpStatus; +import org.springframework.web.filter.OncePerRequestFilter; + +/** + * Limits API requests per client IP address, across all instances (429 + Retry-After). Behind a + * reverse proxy the address comes from X-Forwarded-For, which Tomcat only trusts from internal proxy + * addresses ({@code server.forward-headers-strategy=native}), so clients can't pick their own key. + * + *

Registered for {@code /api/*} by {@link ClientRateLimitConfig}, after the trace id filter, so a + * refused request still gets its X-Trace-Id. + */ +@RequiredArgsConstructor +@Slf4j +public class ClientRateLimitFilter extends OncePerRequestFilter { + + private final RateLimiters rateLimiters; + private final RateLimitProperties rateLimits; + + @Override + protected void doFilterInternal(HttpServletRequest request, HttpServletResponse response, + FilterChain filterChain) throws ServletException, IOException { + var limit = rateLimits.client(); + if (!rateLimiters.tryAcquire("client-rate:" + request.getRemoteAddr(), limit)) { + log.info("Too many requests from {}", request.getRemoteAddr()); + response.setStatus(HttpStatus.TOO_MANY_REQUESTS.value()); + response.setHeader(RETRY_AFTER, String.valueOf(limit.retryAfterSeconds())); + return; + } + filterChain.doFilter(request, response); + } +} diff --git a/src/main/resources/application.properties b/src/main/resources/application.properties index 1cd9fae..877aed1 100755 --- a/src/main/resources/application.properties +++ b/src/main/resources/application.properties @@ -25,6 +25,7 @@ spring.cache.redis.key-prefix=resourceapi:: spring.cache.redis.cache-null-values=false resourceapi.cache.sdlc-systems-ttl=10m resourceapi.cache.external-projects-ttl=5m +resourceapi.cache.missing-external-projects-ttl=30s # redisson (locks, rate limiters) uses the same Redis; its object names get this prefix resourceapi.redisson.key-prefix=resourceapi: # a project's sync lock expires after this even if never released (e.g. the instance died) @@ -37,6 +38,12 @@ resourceapi.rate-limit.sdlc.max-wait=1s resourceapi.rate-limit.sync.permits=6 resourceapi.rate-limit.sync.period=1m resourceapi.rate-limit.sync.max-wait=0s +# API requests per client IP (429 + Retry-After); behind a reverse proxy the client IP comes from +# X-Forwarded-For, trusted only from internal proxy addresses (Tomcat's RemoteIpValve) +resourceapi.rate-limit.client.permits=100 +resourceapi.rate-limit.client.period=1s +resourceapi.rate-limit.client.max-wait=0s +server.forward-headers-strategy=native # datasource spring.datasource.url=jdbc:h2:mem:test;MODE=MySQL;DATABASE_TO_LOWER=TRUE spring.sql.init.platform=h2 diff --git a/src/test/java/com/company/resourceapi/cache/CacheIntegrationTest.java b/src/test/java/com/company/resourceapi/cache/CacheIntegrationTest.java index 7fa5fff..3e81b5f 100644 --- a/src/test/java/com/company/resourceapi/cache/CacheIntegrationTest.java +++ b/src/test/java/com/company/resourceapi/cache/CacheIntegrationTest.java @@ -1,6 +1,7 @@ package com.company.resourceapi.cache; import static com.company.resourceapi.config.CacheConfig.EXTERNAL_PROJECTS; +import static com.company.resourceapi.config.CacheConfig.MISSING_EXTERNAL_PROJECTS; import static com.company.resourceapi.config.CacheConfig.SDLC_SYSTEMS; import static com.github.tomakehurst.wiremock.client.WireMock.getRequestedFor; import static com.github.tomakehurst.wiremock.client.WireMock.notFound; @@ -38,7 +39,7 @@ import org.springframework.test.web.servlet.MockMvc; /** Redis cache in front of the SDLC systems, against a real Redis (RedisTestContainerInitializer). */ -@SpringBootTest +@SpringBootTest(properties = "resourceapi.cache.missing-external-projects-ttl=1s") @AutoConfigureMockMvc class CacheIntegrationTest { @@ -92,29 +93,60 @@ void givenRepeatedLookupsThenSdlcSystemIsAskedOnce() throws Exception { sdlcSystemServer.stubFor(WireMock.get(urlEqualTo(SDLC_PROJECT_URL)) .willReturn(okJson("{\"key\": \"CACHED\", \"name\": \"From SDLC\"}"))); - for (int i = 0; i < 3; i++) { + mockMvc.perform(get(externalUrl())).andExpect(status().isOk()); + // counted after the first lookup: the HTTP client may retry a GET on a stale pooled connection + int callsForFirstLookup = sdlcSystemServer.findAll(getRequestedFor(urlEqualTo(SDLC_PROJECT_URL))).size(); + + for (int i = 0; i < 2; i++) { mockMvc.perform(get(externalUrl())) .andExpect(status().isOk()) .andExpect(jsonPath("$.name", equalTo("From SDLC"))); } - sdlcSystemServer.verify(1, getRequestedFor(urlEqualTo(SDLC_PROJECT_URL))); + // the next lookups are served from the cache + sdlcSystemServer.verify(callsForFirstLookup, getRequestedFor(urlEqualTo(SDLC_PROJECT_URL))); // stored as JSON in Redis and read back as the record, not a map assertThat(cacheManager.getCache(EXTERNAL_PROJECTS).get(externalKey(), ExternalProject.class)) .isEqualTo(new ExternalProject("CACHED", "From SDLC", null)); } @Test - void givenProjectUnknownToSdlcSystemThenItIsNotCached() throws Exception { + void givenProjectUnknownToSdlcSystemThenThatIsRememberedBriefly() throws Exception { sdlcSystemServer.stubFor(WireMock.get(urlEqualTo(SDLC_PROJECT_URL)) .willReturn(notFound().withHeader("Content-Type", "application/json").withBody("{}"))); mockMvc.perform(get(externalUrl())).andExpect(status().isNotFound()); mockMvc.perform(get(externalUrl())).andExpect(status().isNotFound()); - // asked again, so the project is found as soon as it exists in the SDLC system - sdlcSystemServer.verify(2, getRequestedFor(urlEqualTo(SDLC_PROJECT_URL))); + // one call: the "unknown" answer is remembered, in its own short-lived cache + sdlcSystemServer.verify(1, getRequestedFor(urlEqualTo(SDLC_PROJECT_URL))); + assertThat(cacheManager.getCache(MISSING_EXTERNAL_PROJECTS).get(externalKey())).isNotNull(); assertThat(cacheManager.getCache(EXTERNAL_PROJECTS).get(externalKey())).isNull(); + + // the project is created in the SDLC system: found once the "unknown" answer expired (1s here) + sdlcSystemServer.stubFor(WireMock.get(urlEqualTo(SDLC_PROJECT_URL)) + .willReturn(okJson("{\"key\": \"CACHED\", \"name\": \"Created later\"}"))); + mockMvc.perform(get(externalUrl())).andExpect(status().isNotFound()); + await(() -> statusOf(externalUrl()) == 200); + assertThat(cacheManager.getCache(MISSING_EXTERNAL_PROJECTS).get(externalKey())).isNull(); + } + + @Test + void givenProjectRememberedAsUnknownWhenSyncedThenItIsFoundRightAway() throws Exception { + sdlcSystemServer.stubFor(WireMock.get(urlEqualTo(SDLC_PROJECT_URL)) + .willReturn(notFound().withHeader("Content-Type", "application/json").withBody("{}"))); + mockMvc.perform(get(externalUrl())).andExpect(status().isNotFound()); + + sdlcSystemServer.stubFor(WireMock.get(urlEqualTo(SDLC_PROJECT_URL)) + .willReturn(okJson("{\"key\": \"CACHED\", \"name\": \"Created later\"}"))); + mockMvc.perform(post(ProjectRestController.ENDPOINT + "/" + projectId + "/sync")) + .andExpect(status().isAccepted()); + await(() -> projectRepository.findById(projectId).orElseThrow().getName().equals("Created later")); + + // the sync replaced the "unknown" answer with the project + assertThat(cacheManager.getCache(MISSING_EXTERNAL_PROJECTS).get(externalKey())).isNull(); + mockMvc.perform(get(externalUrl())).andExpect(status().isOk()); + sdlcSystemServer.verify(2, getRequestedFor(urlEqualTo(SDLC_PROJECT_URL))); } @Test @@ -191,4 +223,12 @@ void givenProjectGoneFromSdlcSystemWhenSyncedThenCachedEntryIsRemoved() throws E mockMvc.perform(get(externalUrl())).andExpect(status().isNotFound()); } + + private int statusOf(String url) { + try { + return mockMvc.perform(get(url)).andReturn().getResponse().getStatus(); + } catch (Exception e) { + throw new IllegalStateException(e); + } + } } diff --git a/src/test/java/com/company/resourceapi/ratelimit/ClientRateLimitIntegrationTest.java b/src/test/java/com/company/resourceapi/ratelimit/ClientRateLimitIntegrationTest.java new file mode 100644 index 0000000..5de44fd --- /dev/null +++ b/src/test/java/com/company/resourceapi/ratelimit/ClientRateLimitIntegrationTest.java @@ -0,0 +1,55 @@ +package com.company.resourceapi.ratelimit; + +import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get; +import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.header; +import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status; + +import com.company.resourceapi.controllers.ProjectRestController; +import org.junit.jupiter.api.Test; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.boot.test.context.SpringBootTest; +import org.springframework.boot.webmvc.test.autoconfigure.AutoConfigureMockMvc; +import org.springframework.test.web.servlet.MockMvc; +import org.springframework.test.web.servlet.request.RequestPostProcessor; + +/** Requests per client address, shared by all instances through Redis. */ +@SpringBootTest(properties = { + "resourceapi.rate-limit.client.permits=3", + "resourceapi.rate-limit.client.period=2s"}) +@AutoConfigureMockMvc +class ClientRateLimitIntegrationTest { + + private static final String PROJECT_URL = ProjectRestController.ENDPOINT + "/1"; + + @Autowired + private MockMvc mockMvc; + + @Test + void givenTooManyRequestsFromOneClientThenFurtherOnesAreRefused() throws Exception { + for (int i = 0; i < 3; i++) { + mockMvc.perform(get(PROJECT_URL).with(from("10.0.0.1"))).andExpect(status().isOk()); + } + mockMvc.perform(get(PROJECT_URL).with(from("10.0.0.1"))) + .andExpect(status().isTooManyRequests()) + .andExpect(header().string("Retry-After", "2")) + .andExpect(header().exists("X-Trace-Id")); + + // another client has its own limit + mockMvc.perform(get(PROJECT_URL).with(from("10.0.0.2"))).andExpect(status().isOk()); + } + + @Test + void givenNonApiPathsThenTheyAreNotLimited() throws Exception { + for (int i = 0; i < 5; i++) { + mockMvc.perform(get("/v3/api-docs").with(from("10.0.0.3"))).andExpect(status().isOk()); + } + mockMvc.perform(get(PROJECT_URL).with(from("10.0.0.3"))).andExpect(status().isOk()); + } + + private static RequestPostProcessor from(String address) { + return request -> { + request.setRemoteAddr(address); + return request; + }; + } +} diff --git a/src/test/java/com/company/resourceapi/ratelimit/RateLimitPropertiesTest.java b/src/test/java/com/company/resourceapi/ratelimit/RateLimitPropertiesTest.java index b6a8b81..3540100 100644 --- a/src/test/java/com/company/resourceapi/ratelimit/RateLimitPropertiesTest.java +++ b/src/test/java/com/company/resourceapi/ratelimit/RateLimitPropertiesTest.java @@ -18,9 +18,9 @@ void givenInvalidLimitsThenTheyAreRejected() { var noPeriod = new Limit(5, null, Duration.ZERO); var valid = new Limit(5, Duration.ofSeconds(1), Duration.ZERO); - assertThat(validator.validate(new RateLimitProperties(noPermits, valid))).hasSize(1); - assertThat(validator.validate(new RateLimitProperties(valid, noPeriod))).hasSize(1); - assertThat(validator.validate(new RateLimitProperties(valid, valid))).isEmpty(); + assertThat(validator.validate(new RateLimitProperties(noPermits, valid, valid))).hasSize(1); + assertThat(validator.validate(new RateLimitProperties(valid, noPeriod, valid))).hasSize(1); + assertThat(validator.validate(new RateLimitProperties(valid, valid, valid))).isEmpty(); } @Test diff --git a/src/test/resources/config/application.properties b/src/test/resources/config/application.properties index c2a9ab1..67b2d67 100644 --- a/src/test/resources/config/application.properties +++ b/src/test/resources/config/application.properties @@ -1,2 +1,4 @@ # Each test application context gets its own in-memory database so schema.sql/data.sql run on a clean slate spring.datasource.url=jdbc:h2:mem:test-${random.uuid};MODE=MySQL;DATABASE_TO_LOWER=TRUE +# tests send many requests from one address; ClientRateLimitIntegrationTest sets its own limit +resourceapi.rate-limit.client.permits=100000 From 575fd6ca22ee82f9d714bee8e3bbd9655371a6ce Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 31 Oct 2026 10:51:55 +0000 Subject: [PATCH 10/13] Native image: register cached value types for reflection Values in the Redis cache are rebuilt from JSON by their type name, which a native image only allows for types registered for reflection. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_012MLyPXGHeCc1fvTeB8JnT2 --- .../java/com/company/resourceapi/config/CacheConfig.java | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/main/java/com/company/resourceapi/config/CacheConfig.java b/src/main/java/com/company/resourceapi/config/CacheConfig.java index b70a71d..85554ee 100644 --- a/src/main/java/com/company/resourceapi/config/CacheConfig.java +++ b/src/main/java/com/company/resourceapi/config/CacheConfig.java @@ -1,5 +1,8 @@ package com.company.resourceapi.config; +import com.company.resourceapi.clients.ExternalProject; +import com.company.resourceapi.entities.SdlcSystem; +import org.springframework.aot.hint.annotation.RegisterReflectionForBinding; import org.springframework.beans.factory.config.BeanDefinition; import org.springframework.boot.cache.autoconfigure.RedisCacheManagerBuilderCustomizer; import org.springframework.boot.context.properties.EnableConfigurationProperties; @@ -22,6 +25,8 @@ @Configuration @EnableCaching @EnableConfigurationProperties(CacheTtlProperties.class) +// cached values are rebuilt from JSON by type name: reflection a native image must know about +@RegisterReflectionForBinding({SdlcSystem.class, ExternalProject.class}) public class CacheConfig { public static final String SDLC_SYSTEMS = "sdlc-systems"; From 19894eceec95aac78083d4569730b2c6f86b5924 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 31 Oct 2026 10:52:04 +0000 Subject: [PATCH 11/13] Native image: register rate limit properties for validation Startup validation reads the record fields reflectively; the native image failed to start with MissingReflectionRegistrationError on Limit.permits. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_012MLyPXGHeCc1fvTeB8JnT2 --- .../com/company/resourceapi/config/RateLimitProperties.java | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/main/java/com/company/resourceapi/config/RateLimitProperties.java b/src/main/java/com/company/resourceapi/config/RateLimitProperties.java index 37e3cba..451a044 100644 --- a/src/main/java/com/company/resourceapi/config/RateLimitProperties.java +++ b/src/main/java/com/company/resourceapi/config/RateLimitProperties.java @@ -4,6 +4,8 @@ import jakarta.validation.constraints.NotNull; import jakarta.validation.constraints.Positive; import java.time.Duration; +import org.springframework.aot.hint.MemberCategory; +import org.springframework.aot.hint.annotation.RegisterReflection; import org.springframework.boot.context.properties.ConfigurationProperties; import org.springframework.validation.annotation.Validated; @@ -16,6 +18,9 @@ */ @Validated @ConfigurationProperties("resourceapi.rate-limit") +// validation reads the record fields reflectively, which a native image must know about +@RegisterReflection(classes = {RateLimitProperties.class, RateLimitProperties.Limit.class}, + memberCategories = {MemberCategory.ACCESS_DECLARED_FIELDS, MemberCategory.INVOKE_DECLARED_METHODS}) public record RateLimitProperties(@NotNull @Valid Limit sdlc, @NotNull @Valid Limit sync) { /** From 41981eafb797c9d495080dbf608103a8ec6c0764 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 31 Oct 2026 11:05:33 +0000 Subject: [PATCH 12/13] Address review: one external-projects cache, Redis outage pause, shared 429 - "Unknown project" is cached as Spring's null value in the external-projects cache with a per-entry TTL (30s unknown, 5m found), replacing the separate cache and the hand-written lookups; a sync simply @CachePut-replaces it. - RateLimiters stops asking Redis for 10s after a failure, so an outage doesn't add a timeout to every request. - Limiters expire 10 periods after last use (at least a minute) instead of an hour, so per-IP limiters don't pile up. - The per-client filter answers through the shared exception handler; refused callers are logged at DEBUG. - README: server.tomcat.remoteip.internal-proxies for the per-IP limit. - Tests: longer negative-cache TTL, no timing-sensitive 404 assertion, delta-based call counts, unit test for the outage pause. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_012MLyPXGHeCc1fvTeB8JnT2 --- README.md | 20 ++++-- .../resourceapi/config/CacheConfig.java | 14 ++-- .../services/ExternalProjectService.java | 70 ++----------------- .../resourceapi/services/RateLimiters.java | 37 ++++++++-- .../resourceapi/web/ApiExceptionHandler.java | 10 ++- .../web/ClientRateLimitConfig.java | 8 ++- .../web/ClientRateLimitFilter.java | 18 ++--- src/main/resources/application.properties | 1 - .../cache/CacheIntegrationTest.java | 32 +++++---- .../ratelimit/RateLimitIntegrationTest.java | 9 +++ .../ratelimit/RateLimitersTest.java | 49 +++++++++++++ 11 files changed, 163 insertions(+), 105 deletions(-) create mode 100644 src/test/java/com/company/resourceapi/ratelimit/RateLimitersTest.java diff --git a/README.md b/README.md index f7249d9..3629212 100644 --- a/README.md +++ b/README.md @@ -79,10 +79,9 @@ The application uses Redis (`spring.data.redis.host`/`port`, default `localhost: | Cache | What | TTL | |---|---|---| | `sdlc-systems` | SDLC systems by id, read on every create, update and SDLC call | `resourceapi.cache.sdlc-systems-ttl` (10m) | -| `external-projects` | `GET /api/v2/projects/{id}/external` answers per SDLC system and external id | `resourceapi.cache.external-projects-ttl` (5m) | -| `external-projects-missing` | "the SDLC system doesn't know this project" answers, kept briefly so a project created there is found soon | `resourceapi.cache.missing-external-projects-ttl` (30s) | +| `external-projects` | `GET /api/v2/projects/{id}/external` answers per SDLC system and external id | `resourceapi.cache.external-projects-ttl` (5m); "the SDLC system doesn't know this project" (stored as a cached null) only `resourceapi.cache.missing-external-projects-ttl` (30s), so a project created there is found soon | -A background sync always asks the SDLC system and refreshes both caches with its answer. SDLC systems are not changed +A background sync always asks the SDLC system and replaces the cached answer with its own. SDLC systems are not changed through this API; one changed directly in the database is picked up when its entry expires. The cache is an optimization: if Redis is unreachable, cache reads and writes are logged and skipped (each waits at most the 500 ms Redis timeouts) and requests are served without it. @@ -104,11 +103,20 @@ available again one period later, which is also what `Retry-After` says): |---|---|---| | Calls to one SDLC system (`resourceapi.rate-limit.sdlc.*`), protecting its API quota; cache hits don't count | 10 per 1s, waiting up to 1s for a permit | `503` + `Retry-After` (our own limit, not the SDLC system's failure); a background sync logs it | | `POST /api/v2/projects/{id}/sync` per project (`resourceapi.rate-limit.sync.*`); requests refused anyway (unknown project, sync already running) don't count, one refused because no background slot is free (`503`) does | 6 per minute | `429` + `Retry-After` | -| All `/api/**` requests per client IP (`resourceapi.rate-limit.client.*`); behind a reverse proxy the IP comes from `X-Forwarded-For`, trusted only from internal proxy addresses (`server.forward-headers-strategy=native`) | 100 per second | `429` + `Retry-After` | +| All `/api/**` requests per client IP (`resourceapi.rate-limit.client.*`); behind a reverse proxy the IP comes from `X-Forwarded-For`, trusted only from internal proxy addresses (`server.forward-headers-strategy=native`, see below) | 100 per second | `429` + `Retry-After` | A changed rate is applied to the existing limiter when an instance first uses it; unused limiters expire from -Redis after an hour. Invalid limits (e.g. `permits=0`) stop the application at startup. If Redis can't be reached, requests are allowed (limiting protects, losing it shouldn't -stop the application). +Redis after 10 periods (at least a minute), so per-IP limiters don't pile up. Invalid limits (e.g. +`permits=0`) stop the application at startup. If Redis can't be reached, requests are allowed (limiting +protects, losing it shouldn't stop the application), and Redis is not asked again for 10 seconds, so an +outage doesn't add a timeout to every request. + +The per-IP limit only means something if the application sees the client's address. Tomcat takes it from +`X-Forwarded-For` when the request comes from an address matching `server.tomcat.remoteip.internal-proxies` +(by default 10/8, 192.168/16, 172.16/12, 169.254/16, 127/8 and IPv6 local addresses). If the reverse proxy or +load balancer has another address, set that property to match it; otherwise every request appears to come from +the proxy and all clients share one limit. Never match addresses clients can connect from directly, or they +can choose their own key by sending `X-Forwarded-For`. **Local development**: `./mvnw spring-boot:run` starts Redis from `compose.yaml` automatically (`spring-boot-docker-compose`); with `java -jar`, start it yourself with `docker compose up -d` (it listens on diff --git a/src/main/java/com/company/resourceapi/config/CacheConfig.java b/src/main/java/com/company/resourceapi/config/CacheConfig.java index bd7f328..c89783e 100644 --- a/src/main/java/com/company/resourceapi/config/CacheConfig.java +++ b/src/main/java/com/company/resourceapi/config/CacheConfig.java @@ -10,6 +10,7 @@ import org.springframework.cache.annotation.EnableCaching; import org.springframework.cache.interceptor.CacheErrorHandler; import org.springframework.cache.interceptor.LoggingCacheErrorHandler; +import org.springframework.cache.support.NullValue; import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.Configuration; import org.springframework.context.annotation.Role; @@ -26,12 +27,11 @@ @EnableCaching @EnableConfigurationProperties(CacheTtlProperties.class) // cached values are rebuilt from JSON by type name: reflection a native image must know about -@RegisterReflectionForBinding({SdlcSystem.class, ExternalProject.class}) +@RegisterReflectionForBinding({SdlcSystem.class, ExternalProject.class, NullValue.class}) public class CacheConfig { public static final String SDLC_SYSTEMS = "sdlc-systems"; public static final String EXTERNAL_PROJECTS = "external-projects"; - public static final String MISSING_EXTERNAL_PROJECTS = "external-projects-missing"; /** * Starts from Boot's defaults (spring.cache.redis.*: key prefix, null values, ...) and only @@ -43,16 +43,22 @@ RedisCacheManagerBuilderCustomizer redisCacheCustomizer(CacheTtlProperties ttl) .allowIfSubType("com.company.resourceapi.") .allowIfSubType("java.lang.") .allowIfSubType("java.time.") + // Spring's marker for a cached null ("the SDLC system doesn't know this project") + .allowIfSubType(NullValue.class) .build(); var serializer = GenericJacksonJsonRedisSerializer.builder() .enableDefaultTyping(typeValidator) + .enableSpringCacheNullValueSupport() .build(); return builder -> { var defaults = builder.cacheDefaults().serializeValuesWith(SerializationPair.fromSerializer(serializer)); builder.cacheDefaults(defaults) .withCacheConfiguration(SDLC_SYSTEMS, defaults.entryTtl(ttl.sdlcSystemsTtl())) - .withCacheConfiguration(EXTERNAL_PROJECTS, defaults.entryTtl(ttl.externalProjectsTtl())) - .withCacheConfiguration(MISSING_EXTERNAL_PROJECTS, defaults.entryTtl(ttl.missingExternalProjectsTtl())); + // a found project is kept longer than "unknown", so a project created there is found soon + .withCacheConfiguration(EXTERNAL_PROJECTS, defaults.entryTtl((key, value) -> + value == null || value instanceof NullValue + ? ttl.missingExternalProjectsTtl() + : ttl.externalProjectsTtl())); }; } diff --git a/src/main/java/com/company/resourceapi/services/ExternalProjectService.java b/src/main/java/com/company/resourceapi/services/ExternalProjectService.java index 7b064e1..88d39e2 100644 --- a/src/main/java/com/company/resourceapi/services/ExternalProjectService.java +++ b/src/main/java/com/company/resourceapi/services/ExternalProjectService.java @@ -1,7 +1,6 @@ package com.company.resourceapi.services; import static com.company.resourceapi.config.CacheConfig.EXTERNAL_PROJECTS; -import static com.company.resourceapi.config.CacheConfig.MISSING_EXTERNAL_PROJECTS; import com.company.resourceapi.clients.ExternalProject; import com.company.resourceapi.clients.SdlcProjectClient; @@ -11,13 +10,8 @@ import com.company.resourceapi.exceptions.RateLimitExceededException.Scope; import java.util.Optional; import lombok.RequiredArgsConstructor; -import lombok.extern.slf4j.Slf4j; -import org.springframework.cache.Cache; -import org.springframework.cache.CacheManager; -import org.springframework.cache.annotation.CacheEvict; import org.springframework.cache.annotation.CachePut; import org.springframework.cache.annotation.Cacheable; -import org.springframework.cache.annotation.Caching; import org.springframework.stereotype.Service; /** @@ -26,75 +20,25 @@ */ @Service @RequiredArgsConstructor -@Slf4j public class ExternalProjectService { private final SdlcProjectClient sdlcProjectClient; private final RateLimiters rateLimiters; private final RateLimitProperties rateLimits; - private final CacheManager cacheManager; /** - * Cached. A project unknown to the SDLC system is remembered separately and briefly, so repeated - * lookups don't each call the SDLC system, while a project created there is still found soon. + * Cached, including the answer "the SDLC system doesn't know this project" (cached as null), which + * is kept only briefly so a project created there is found soon (see CacheConfig). */ - @Cacheable(cacheNames = EXTERNAL_PROJECTS, key = "#sdlcSystem.id + ':' + #externalId", unless = "#result == null") + @Cacheable(cacheNames = EXTERNAL_PROJECTS, key = "#sdlcSystem.id + ':' + #externalId") public Optional findProject(SdlcSystem sdlcSystem, String externalId) { - var key = key(sdlcSystem, externalId); - if (isKnownMissing(key)) { - return Optional.empty(); - } - var project = fetch(sdlcSystem, externalId); - rememberMissing(key, project.isEmpty()); - return project; + return fetch(sdlcSystem, externalId); } - /** - * Always asks the SDLC system and updates the cache with the answer: a found project replaces the - * cached one, a project the SDLC system no longer knows removes it. - */ - @Caching( - put = @CachePut(cacheNames = EXTERNAL_PROJECTS, key = "#sdlcSystem.id + ':' + #externalId", - unless = "#result == null"), - // unlike for @CachePut, #result is the Optional itself here (late evictions get the raw return value) - evict = @CacheEvict(cacheNames = EXTERNAL_PROJECTS, key = "#sdlcSystem.id + ':' + #externalId", - condition = "#result == null or #result.isEmpty()")) + /** Always asks the SDLC system and caches its answer, found or unknown, replacing the cached one. */ + @CachePut(cacheNames = EXTERNAL_PROJECTS, key = "#sdlcSystem.id + ':' + #externalId") public Optional refreshProject(SdlcSystem sdlcSystem, String externalId) { - var project = fetch(sdlcSystem, externalId); - rememberMissing(key(sdlcSystem, externalId), project.isEmpty()); - return project; - } - - // The "missing" cache is used directly, which bypasses the cache error handler: failures are - // handled here the same way, by working without it. - - private boolean isKnownMissing(String key) { - try { - return missingCache().get(key) != null; - } catch (RuntimeException e) { - log.warn("Cache '{}' failed to get entry with key '{}': {}", MISSING_EXTERNAL_PROJECTS, key, e.getMessage()); - return false; - } - } - - private void rememberMissing(String key, boolean missing) { - try { - if (missing) { - missingCache().put(key, Boolean.TRUE); - } else { - missingCache().evict(key); - } - } catch (RuntimeException e) { - log.warn("Cache '{}' failed to update entry with key '{}': {}", MISSING_EXTERNAL_PROJECTS, key, e.getMessage()); - } - } - - private Cache missingCache() { - return cacheManager.getCache(MISSING_EXTERNAL_PROJECTS); - } - - private static String key(SdlcSystem sdlcSystem, String externalId) { - return sdlcSystem.getId() + ":" + externalId; + return fetch(sdlcSystem, externalId); } /** Only real calls count against the SDLC system's rate limit, not cache hits. */ diff --git a/src/main/java/com/company/resourceapi/services/RateLimiters.java b/src/main/java/com/company/resourceapi/services/RateLimiters.java index f5bcefa..75d1218 100644 --- a/src/main/java/com/company/resourceapi/services/RateLimiters.java +++ b/src/main/java/com/company/resourceapi/services/RateLimiters.java @@ -18,18 +18,24 @@ *

A limiter is set up with the configured rate the first time this instance uses it (updating a * limiter created with another rate), and again if Redis dropped it after it expired from disuse. * Without Redis (unreachable, or no Redisson client) requests are allowed: limiting is a protection, - * losing it shouldn't stop the application. + * losing it shouldn't stop the application. After a failure Redis is left alone for a short while, + * so an outage doesn't add a Redis timeout to every request. */ @Component @Slf4j public class RateLimiters { - /** Unused limiters are removed from Redis after this. */ - private static final Duration KEEP_ALIVE = Duration.ofHours(1); + /** Unused limiters are removed from Redis after this many periods, but not sooner than MIN_KEEP_ALIVE. */ + private static final int KEEP_ALIVE_PERIODS = 10; + private static final Duration MIN_KEEP_ALIVE = Duration.ofMinutes(1); + /** How long requests are allowed without asking Redis after it failed. */ + static final Duration PAUSE_AFTER_FAILURE = Duration.ofSeconds(10); /** Limiters this instance has set up, most recently used kept (bounded: one per project). */ private static final int MAX_REMEMBERED = 10_000; private final RedissonClient redisson; + /** System.nanoTime() until which Redis isn't asked; 0 while it works. */ + private volatile long pausedUntil; private final Map setUp = Collections.synchronizedMap(new LinkedHashMap<>(16, 0.75f, true) { @Override protected boolean removeEldestEntry(Map.Entry eldest) { @@ -43,7 +49,7 @@ public RateLimiters(ObjectProvider redisson) { /** @return whether a permit was obtained, waiting at most the limit's max wait */ public boolean tryAcquire(String name, RateLimitProperties.Limit limit) { - if (redisson == null) { + if (redisson == null || isPaused()) { return true; } try { @@ -62,17 +68,36 @@ public boolean tryAcquire(String name, RateLimitProperties.Limit limit) { return acquire(limiter, limit); } } catch (RuntimeException e) { - log.warn("Rate limiter {} unavailable, allowing the request: {}", name, e.getMessage()); + pausedUntil = System.nanoTime() + PAUSE_AFTER_FAILURE.toNanos(); + log.warn("Rate limiter {} unavailable, allowing requests without rate limits for {}: {}", + name, PAUSE_AFTER_FAILURE, e.getMessage()); return true; } } + private boolean isPaused() { + long until = pausedUntil; + if (until == 0) { + return false; + } + if (System.nanoTime() - until < 0) { + return true; + } + pausedUntil = 0; + return false; + } + + static Duration keepAlive(RateLimitProperties.Limit limit) { + var periods = limit.period().multipliedBy(KEEP_ALIVE_PERIODS); + return periods.compareTo(MIN_KEEP_ALIVE) > 0 ? periods : MIN_KEEP_ALIVE; + } + private static boolean acquire(RRateLimiter limiter, RateLimitProperties.Limit limit) { return limiter.tryAcquireAsync(1, limit.maxWait()).toCompletableFuture().join(); } private void setUp(String name, RRateLimiter limiter, RateLimitProperties.Limit limit) { - var keepAlive = Duration.ofMillis(Math.max(KEEP_ALIVE.toMillis(), limit.period().toMillis())); + var keepAlive = keepAlive(limit); boolean created = limiter.trySetRateAsync(RateType.OVERALL, limit.permits(), limit.period(), keepAlive) .toCompletableFuture().join(); if (!created) { diff --git a/src/main/java/com/company/resourceapi/web/ApiExceptionHandler.java b/src/main/java/com/company/resourceapi/web/ApiExceptionHandler.java index 50ed5a8..cdb0b5d 100644 --- a/src/main/java/com/company/resourceapi/web/ApiExceptionHandler.java +++ b/src/main/java/com/company/resourceapi/web/ApiExceptionHandler.java @@ -39,8 +39,14 @@ ResponseEntity handleSyncInProgress(SyncInProgressException e) { */ @ExceptionHandler(RateLimitExceededException.class) ResponseEntity handleRateLimitExceeded(RateLimitExceededException e) { - log.info(e.getMessage()); - var status = e.getScope() == RateLimitExceededException.Scope.CALLER ? TOO_MANY_REQUESTS : SERVICE_UNAVAILABLE; + var caller = e.getScope() == RateLimitExceededException.Scope.CALLER; + // a refused caller is expected, possibly many times a second; holding back our own calls is worth seeing + if (caller) { + log.debug(e.getMessage()); + } else { + log.info(e.getMessage()); + } + var status = caller ? TOO_MANY_REQUESTS : SERVICE_UNAVAILABLE; return ResponseEntity.status(status).header(RETRY_AFTER, String.valueOf(e.getRetryAfterSeconds())).build(); } diff --git a/src/main/java/com/company/resourceapi/web/ClientRateLimitConfig.java b/src/main/java/com/company/resourceapi/web/ClientRateLimitConfig.java index 5cb3f82..d552051 100644 --- a/src/main/java/com/company/resourceapi/web/ClientRateLimitConfig.java +++ b/src/main/java/com/company/resourceapi/web/ClientRateLimitConfig.java @@ -2,10 +2,12 @@ import com.company.resourceapi.config.RateLimitProperties; import com.company.resourceapi.services.RateLimiters; +import org.springframework.beans.factory.annotation.Qualifier; import org.springframework.boot.web.servlet.FilterRegistrationBean; import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.Configuration; import org.springframework.core.Ordered; +import org.springframework.web.servlet.HandlerExceptionResolver; /** Registers {@link ClientRateLimitFilter} for the API only (not Swagger UI or the OpenAPI document). */ @Configuration @@ -13,8 +15,10 @@ public class ClientRateLimitConfig { @Bean FilterRegistrationBean clientRateLimitFilter(RateLimiters rateLimiters, - RateLimitProperties rateLimits) { - var registration = new FilterRegistrationBean<>(new ClientRateLimitFilter(rateLimiters, rateLimits)); + RateLimitProperties rateLimits, + @Qualifier("handlerExceptionResolver") HandlerExceptionResolver exceptionResolver) { + var registration = new FilterRegistrationBean<>( + new ClientRateLimitFilter(rateLimiters, rateLimits, exceptionResolver)); registration.addUrlPatterns("/api/*"); // after the server observation (+1) and trace id (+2) filters registration.setOrder(Ordered.HIGHEST_PRECEDENCE + 3); diff --git a/src/main/java/com/company/resourceapi/web/ClientRateLimitFilter.java b/src/main/java/com/company/resourceapi/web/ClientRateLimitFilter.java index e0637d8..570a96a 100644 --- a/src/main/java/com/company/resourceapi/web/ClientRateLimitFilter.java +++ b/src/main/java/com/company/resourceapi/web/ClientRateLimitFilter.java @@ -1,8 +1,8 @@ package com.company.resourceapi.web; -import static org.springframework.http.HttpHeaders.RETRY_AFTER; - import com.company.resourceapi.config.RateLimitProperties; +import com.company.resourceapi.exceptions.RateLimitExceededException; +import com.company.resourceapi.exceptions.RateLimitExceededException.Scope; import com.company.resourceapi.services.RateLimiters; import jakarta.servlet.FilterChain; import jakarta.servlet.ServletException; @@ -10,9 +10,8 @@ import jakarta.servlet.http.HttpServletResponse; import java.io.IOException; import lombok.RequiredArgsConstructor; -import lombok.extern.slf4j.Slf4j; -import org.springframework.http.HttpStatus; import org.springframework.web.filter.OncePerRequestFilter; +import org.springframework.web.servlet.HandlerExceptionResolver; /** * Limits API requests per client IP address, across all instances (429 + Retry-After). Behind a @@ -20,23 +19,24 @@ * addresses ({@code server.forward-headers-strategy=native}), so clients can't pick their own key. * *

Registered for {@code /api/*} by {@link ClientRateLimitConfig}, after the trace id filter, so a - * refused request still gets its X-Trace-Id. + * refused request still gets its X-Trace-Id. The refusal is answered like any other + * {@link RateLimitExceededException}, by the application's exception handling. */ @RequiredArgsConstructor -@Slf4j public class ClientRateLimitFilter extends OncePerRequestFilter { private final RateLimiters rateLimiters; private final RateLimitProperties rateLimits; + private final HandlerExceptionResolver exceptionResolver; @Override protected void doFilterInternal(HttpServletRequest request, HttpServletResponse response, FilterChain filterChain) throws ServletException, IOException { var limit = rateLimits.client(); if (!rateLimiters.tryAcquire("client-rate:" + request.getRemoteAddr(), limit)) { - log.info("Too many requests from {}", request.getRemoteAddr()); - response.setStatus(HttpStatus.TOO_MANY_REQUESTS.value()); - response.setHeader(RETRY_AFTER, String.valueOf(limit.retryAfterSeconds())); + var refusal = new RateLimitExceededException(Scope.CALLER, + "Too many requests from " + request.getRemoteAddr(), limit.retryAfterSeconds()); + exceptionResolver.resolveException(request, response, null, refusal); return; } filterChain.doFilter(request, response); diff --git a/src/main/resources/application.properties b/src/main/resources/application.properties index 877aed1..ade8268 100755 --- a/src/main/resources/application.properties +++ b/src/main/resources/application.properties @@ -22,7 +22,6 @@ spring.data.redis.repositories.enabled=false # explicit: Redisson brings a JCache provider, which Boot would otherwise prefer over Redis spring.cache.type=redis spring.cache.redis.key-prefix=resourceapi:: -spring.cache.redis.cache-null-values=false resourceapi.cache.sdlc-systems-ttl=10m resourceapi.cache.external-projects-ttl=5m resourceapi.cache.missing-external-projects-ttl=30s diff --git a/src/test/java/com/company/resourceapi/cache/CacheIntegrationTest.java b/src/test/java/com/company/resourceapi/cache/CacheIntegrationTest.java index 3e81b5f..d15b903 100644 --- a/src/test/java/com/company/resourceapi/cache/CacheIntegrationTest.java +++ b/src/test/java/com/company/resourceapi/cache/CacheIntegrationTest.java @@ -1,7 +1,6 @@ package com.company.resourceapi.cache; import static com.company.resourceapi.config.CacheConfig.EXTERNAL_PROJECTS; -import static com.company.resourceapi.config.CacheConfig.MISSING_EXTERNAL_PROJECTS; import static com.company.resourceapi.config.CacheConfig.SDLC_SYSTEMS; import static com.github.tomakehurst.wiremock.client.WireMock.getRequestedFor; import static com.github.tomakehurst.wiremock.client.WireMock.notFound; @@ -39,7 +38,7 @@ import org.springframework.test.web.servlet.MockMvc; /** Redis cache in front of the SDLC systems, against a real Redis (RedisTestContainerInitializer). */ -@SpringBootTest(properties = "resourceapi.cache.missing-external-projects-ttl=1s") +@SpringBootTest(properties = "resourceapi.cache.missing-external-projects-ttl=3s") @AutoConfigureMockMvc class CacheIntegrationTest { @@ -116,19 +115,22 @@ void givenProjectUnknownToSdlcSystemThenThatIsRememberedBriefly() throws Excepti .willReturn(notFound().withHeader("Content-Type", "application/json").withBody("{}"))); mockMvc.perform(get(externalUrl())).andExpect(status().isNotFound()); + int callsForFirstLookup = sdlcSystemServer.findAll(getRequestedFor(urlEqualTo(SDLC_PROJECT_URL))).size(); mockMvc.perform(get(externalUrl())).andExpect(status().isNotFound()); - // one call: the "unknown" answer is remembered, in its own short-lived cache - sdlcSystemServer.verify(1, getRequestedFor(urlEqualTo(SDLC_PROJECT_URL))); - assertThat(cacheManager.getCache(MISSING_EXTERNAL_PROJECTS).get(externalKey())).isNotNull(); - assertThat(cacheManager.getCache(EXTERNAL_PROJECTS).get(externalKey())).isNull(); + // the "unknown" answer is remembered (as a cached null), with the short time to live (3s here) + sdlcSystemServer.verify(callsForFirstLookup, getRequestedFor(urlEqualTo(SDLC_PROJECT_URL))); + var cached = cacheManager.getCache(EXTERNAL_PROJECTS).get(externalKey()); + assertThat(cached).isNotNull(); + assertThat(cached.get()).isNull(); + assertThat(redisTemplate.getExpire(redisKey())).isBetween(1L, 3L); - // the project is created in the SDLC system: found once the "unknown" answer expired (1s here) + // the project is created in the SDLC system: found once the "unknown" answer expired sdlcSystemServer.stubFor(WireMock.get(urlEqualTo(SDLC_PROJECT_URL)) .willReturn(okJson("{\"key\": \"CACHED\", \"name\": \"Created later\"}"))); - mockMvc.perform(get(externalUrl())).andExpect(status().isNotFound()); await(() -> statusOf(externalUrl()) == 200); - assertThat(cacheManager.getCache(MISSING_EXTERNAL_PROJECTS).get(externalKey())).isNull(); + // ... and kept with the long time to live (5m) + assertThat(redisTemplate.getExpire(redisKey())).isGreaterThan(60L); } @Test @@ -142,11 +144,13 @@ void givenProjectRememberedAsUnknownWhenSyncedThenItIsFoundRightAway() throws Ex mockMvc.perform(post(ProjectRestController.ENDPOINT + "/" + projectId + "/sync")) .andExpect(status().isAccepted()); await(() -> projectRepository.findById(projectId).orElseThrow().getName().equals("Created later")); + int callsUntilSynced = sdlcSystemServer.findAll(getRequestedFor(urlEqualTo(SDLC_PROJECT_URL))).size(); - // the sync replaced the "unknown" answer with the project - assertThat(cacheManager.getCache(MISSING_EXTERNAL_PROJECTS).get(externalKey())).isNull(); + // the sync replaced the "unknown" answer with the project, well within its 3s + assertThat(cacheManager.getCache(EXTERNAL_PROJECTS).get(externalKey(), ExternalProject.class)) + .isEqualTo(new ExternalProject("CACHED", "Created later", null)); mockMvc.perform(get(externalUrl())).andExpect(status().isOk()); - sdlcSystemServer.verify(2, getRequestedFor(urlEqualTo(SDLC_PROJECT_URL))); + sdlcSystemServer.verify(callsUntilSynced, getRequestedFor(urlEqualTo(SDLC_PROJECT_URL))); } @Test @@ -190,6 +194,10 @@ private String externalKey() { return sdlcSystem.getId() + ":" + EXTERNAL_ID; } + private String redisKey() { + return keyPrefix + EXTERNAL_PROJECTS + "::" + externalKey(); + } + private static void await(BooleanSupplier condition) throws InterruptedException { var deadline = Instant.now().plus(Duration.ofSeconds(5)); while (!condition.getAsBoolean()) { diff --git a/src/test/java/com/company/resourceapi/ratelimit/RateLimitIntegrationTest.java b/src/test/java/com/company/resourceapi/ratelimit/RateLimitIntegrationTest.java index 0c18a57..28884ed 100644 --- a/src/test/java/com/company/resourceapi/ratelimit/RateLimitIntegrationTest.java +++ b/src/test/java/com/company/resourceapi/ratelimit/RateLimitIntegrationTest.java @@ -178,6 +178,15 @@ void givenSyncAlreadyRunningThenRefusedRequestDoesNotUseAPermit() throws Excepti await(() -> !otherInstanceLock.isLocked(), Duration.ofSeconds(5)); } + @Test + void givenLimiterThenItExpiresFromRedisSoonAfterItsLastUse() throws Exception { + mockMvc.perform(get(externalUrl(project("A")))).andExpect(status().isOk()); + + // 10 periods of 2s is less than the 1-minute minimum + long ttl = redisson.getRateLimiter("sdlc-rate:" + sdlcSystem.getId()).remainTimeToLive(); + assertThat(ttl).isBetween(Duration.ofSeconds(50).toMillis(), Duration.ofMinutes(1).toMillis()); + } + @Test void givenLimiterRemovedFromRedisThenItIsSetUpAgain() throws Exception { mockMvc.perform(get(externalUrl(project("A")))).andExpect(status().isOk()); diff --git a/src/test/java/com/company/resourceapi/ratelimit/RateLimitersTest.java b/src/test/java/com/company/resourceapi/ratelimit/RateLimitersTest.java new file mode 100644 index 0000000..6b6f7aa --- /dev/null +++ b/src/test/java/com/company/resourceapi/ratelimit/RateLimitersTest.java @@ -0,0 +1,49 @@ +package com.company.resourceapi.ratelimit; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +import com.company.resourceapi.config.RateLimitProperties.Limit; +import com.company.resourceapi.services.RateLimiters; +import java.time.Duration; +import org.junit.jupiter.api.Test; +import org.redisson.api.RedissonClient; +import org.springframework.beans.factory.ObjectProvider; + +/** Behavior without a working Redis; with one, see RateLimitIntegrationTest. */ +class RateLimitersTest { + + private static final Limit LIMIT = new Limit(1, Duration.ofSeconds(1), Duration.ZERO); + + @Test + void givenRedisFailsThenRequestsAreAllowedWithoutAskingItAgainForAWhile() { + var redisson = mock(RedissonClient.class); + when(redisson.getRateLimiter(anyString())).thenThrow(new IllegalStateException("Redis is down")); + var rateLimiters = new RateLimiters(providerOf(redisson)); + + for (int i = 0; i < 5; i++) { + assertThat(rateLimiters.tryAcquire("test", LIMIT)).isTrue(); + } + + // one failed attempt, then Redis is left alone instead of each request waiting for its timeout + verify(redisson, times(1)).getRateLimiter(anyString()); + } + + @Test + void givenNoRedissonClientThenRequestsAreAllowed() { + var rateLimiters = new RateLimiters(providerOf(null)); + + assertThat(rateLimiters.tryAcquire("test", LIMIT)).isTrue(); + } + + @SuppressWarnings("unchecked") + private static ObjectProvider providerOf(RedissonClient redisson) { + ObjectProvider provider = mock(ObjectProvider.class); + when(provider.getIfAvailable()).thenReturn(redisson); + return provider; + } +} From 39f3d560d9de9f312292c16b1ad0682780b7015b Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 31 Oct 2026 11:06:48 +0000 Subject: [PATCH 13/13] Tighten RateLimiters visibility; note cached-null dependency Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_012MLyPXGHeCc1fvTeB8JnT2 --- src/main/java/com/company/resourceapi/config/CacheConfig.java | 3 ++- .../java/com/company/resourceapi/services/RateLimiters.java | 4 ++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/src/main/java/com/company/resourceapi/config/CacheConfig.java b/src/main/java/com/company/resourceapi/config/CacheConfig.java index c89783e..8b0533d 100644 --- a/src/main/java/com/company/resourceapi/config/CacheConfig.java +++ b/src/main/java/com/company/resourceapi/config/CacheConfig.java @@ -35,7 +35,8 @@ public class CacheConfig { /** * Starts from Boot's defaults (spring.cache.redis.*: key prefix, null values, ...) and only - * replaces the value serializer and sets each cache's time to live. + * replaces the value serializer and sets each cache's time to live. External projects rely on + * null values being cached (spring.cache.redis.cache-null-values, true by default). */ @Bean RedisCacheManagerBuilderCustomizer redisCacheCustomizer(CacheTtlProperties ttl) { diff --git a/src/main/java/com/company/resourceapi/services/RateLimiters.java b/src/main/java/com/company/resourceapi/services/RateLimiters.java index 75d1218..787c239 100644 --- a/src/main/java/com/company/resourceapi/services/RateLimiters.java +++ b/src/main/java/com/company/resourceapi/services/RateLimiters.java @@ -29,7 +29,7 @@ public class RateLimiters { private static final int KEEP_ALIVE_PERIODS = 10; private static final Duration MIN_KEEP_ALIVE = Duration.ofMinutes(1); /** How long requests are allowed without asking Redis after it failed. */ - static final Duration PAUSE_AFTER_FAILURE = Duration.ofSeconds(10); + private static final Duration PAUSE_AFTER_FAILURE = Duration.ofSeconds(10); /** Limiters this instance has set up, most recently used kept (bounded: one per project). */ private static final int MAX_REMEMBERED = 10_000; @@ -87,7 +87,7 @@ private boolean isPaused() { return false; } - static Duration keepAlive(RateLimitProperties.Limit limit) { + private static Duration keepAlive(RateLimitProperties.Limit limit) { var periods = limit.period().multipliedBy(KEEP_ALIVE_PERIODS); return periods.compareTo(MIN_KEEP_ALIVE) > 0 ? periods : MIN_KEEP_ALIVE; }