From a435c90d15b49f32f4cbfb8492e291afc6897202 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 31 Oct 2026 10:25:08 +0000 Subject: [PATCH 1/6] 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 2/6] 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 801a766840b5fefee82195331d6ae257e2b0f092 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 31 Oct 2026 10:50:45 +0000 Subject: [PATCH 3/6] 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 19894eceec95aac78083d4569730b2c6f86b5924 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 31 Oct 2026 10:52:04 +0000 Subject: [PATCH 4/6] 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 5/6] 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 6/6] 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; }