Skip to content

Commit 67d3a9c

Browse files
authored
Merge pull request #56 from runcycles/claude/optimize-cycle-performance-hwNX9
Optimize Lua script execution and add API key caching
2 parents f1f9706 + aa29b54 commit 67d3a9c

18 files changed

Lines changed: 1361 additions & 320 deletions

File tree

AUDIT.md

Lines changed: 55 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -26,8 +26,9 @@
2626
| Grace Period Handling || 0 |
2727
| Test Coverage || 0 |
2828
| Tenant Default Config || 0 |
29+
| Performance Optimizations | 7/7 | 0 |
2930

30-
**All previously identified issues have been fixed. No remaining spec violations found.**
31+
**All previously identified issues have been fixed. No remaining spec violations found. Performance optimized and benchmarked.**
3132

3233
---
3334

@@ -140,27 +141,33 @@ Two-pass audit covering:
140141
- `GET /v1/reservations/{id}` returns `ReservationDetail` with `status: EXPIRED` for expired reservations
141142
- All mutation responses (reserve, commit, release, extend, event) populate optional `balances` field
142143

143-
### Lua Atomicity (correct)
144+
### Lua Atomicity (correct, optimized)
144145
- `ALLOW_IF_AVAILABLE` in commit.lua uses fail-fast pattern (all checks before mutations)
145-
- `ALLOW_WITH_OVERDRAFT` in commit.lua uses fail-fast pattern across all scopes
146+
- `ALLOW_WITH_OVERDRAFT` in commit.lua uses fail-fast pattern with cached scope values (eliminates redundant Redis reads in mutation loop)
146147
- Event.lua uses same fail-fast atomicity patterns
147-
- Reserve.lua atomically checks and deducts across all derived scopes
148+
- Reserve.lua atomically checks and deducts across all derived scopes using HMGET (1 call per scope instead of 5)
148149
- All Lua scripts leverage Redis single-threaded execution for atomicity
150+
- Balance snapshots returned atomically from Lua scripts (reserve, commit, release) — read consistency guaranteed within the same atomic operation
151+
- Commit/release idempotency-hit paths return `estimate_amount`/`estimate_unit` for correct `released` calculation
149152

150-
### Test Coverage (above 98%)
153+
### Test Coverage (above 95%)
151154
- JaCoCo line coverage threshold raised from 90% to **95%** (enforced in parent pom.xml)
152-
- **API module**: **100%** line coverage (270 tests across 14 test classes)
153-
- **Data module**: **98.5%+** line coverage, 76%+ branch coverage (215 tests across 7 test classes)
155+
- **API module**: **100%** line coverage (278 tests across 15 test classes)
156+
- **Data module**: **95%+** line coverage, 76%+ branch coverage (235 tests across 8 test classes)
154157
- Branch gaps: defensive null-check ternaries and unreachable `&&` short-circuit branches in `RedisReservationRepository`
155158
- Tenant default resolution: 10 unit tests covering all fallback paths, TTL capping, max extensions, malformed JSON recovery
159+
- LuaScriptRegistry: 5 tests (startup load, EVALSHA, NOSCRIPT fallback, no-SHA fallback, startup failure)
160+
- Idempotency replay: 2 tests (commit replay returns released amount, release replay returns released amount)
161+
- API key cache: 1 test (verifies cache hit avoids second Redis call)
156162
- **Model module**: Coverage skipped (POJOs only, no business logic) — 9 tests
157-
- Total test count: 270 (API) + 215 (Data) + 9 (Model) = **494 tests** across 22 test classes
163+
- Total test count: 278 (API) + 235 (Data) + 9 (Model) = **522 tests** across 24 test classes
158164
- **Integration tests**: 27+ nested test classes covering all 9 endpoints, including:
159165
- Tenant Defaults (9 tests): overage policy resolution (ALLOW_IF_AVAILABLE, ALLOW_WITH_OVERDRAFT, REJECT), explicit override vs tenant default, TTL capping, max extensions enforcement, default TTL usage, no-tenant-record fallback
160166
- Expiry Sweep (7 tests): end-to-end expire.lua execution, grace period skip, orphan TTL cleanup, multi-scope budget release, already-finalized skip
161167
- Budget Status (2 tests): BUDGET_FROZEN and BUDGET_CLOSED enforcement on reserve
162168
- ALLOW_IF_AVAILABLE commit overage (integration coverage)
163-
- All unit tests pass without Docker/Testcontainers (integration tests excluded by default)
169+
- **Performance benchmarks**: 8 tests (6 individual operations + 2 composite lifecycles), tagged `@Tag("benchmark")`
170+
- All unit tests pass without Docker/Testcontainers (integration and benchmark tests excluded by default)
164171

165172
### Tenant Default Configuration (correct)
166173
- `default_commit_overage_policy`: resolved at reservation/event creation time
@@ -185,6 +192,44 @@ Two-pass audit covering:
185192
- Concurrent commit/event behavior matches spec (per-operation check, not cross-operation atomic)
186193
- Event overage_policy defaults to REJECT, supports all three policies
187194

195+
### Performance Optimizations (all applied)
196+
197+
Seven optimizations applied to the reserve/commit/release hot path, preserving all protocol correctness guarantees (atomicity, idempotency, ledger invariant `remaining = allocated - spent - reserved - debt`).
198+
199+
| # | Optimization | Impact | Location |
200+
|---|-------------|--------|----------|
201+
| 1 | **BCrypt API key cache** — ConcurrentHashMap keyed by SHA-256(key), 60s TTL | Eliminates ~100ms+ BCrypt per request on cache hit | `ApiKeyRepository.java` |
202+
| 2 | **EVALSHA** — Script SHA loaded at startup, 40-char hash sent instead of full script | Saves ~1-5KB network per call; auto-fallback to EVAL on NOSCRIPT | `LuaScriptRegistry.java` (new), all `eval()` callers |
203+
| 3 | **Pipelined balance fetch** — N HGETALL calls batched into single round-trip | Saves (N-1) round-trips for fallback paths (extend, events, idempotency hits) | `RedisReservationRepository.fetchBalancesForScopes()` |
204+
| 4 | **Lua returns balances** — Balance snapshots collected atomically at end of reserve/commit/release scripts | Eliminates post-operation Java balance fetch entirely for primary paths | `reserve.lua`, `commit.lua`, `release.lua` |
205+
| 5 | **Lua HMGET** — Single HMGET per scope instead of EXISTS + 4 HGET; cached fail-fast values in ALLOW_WITH_OVERDRAFT; TIME reuse | Fewer Redis commands inside Lua scripts | `reserve.lua`, `commit.lua`, `release.lua` |
206+
| 6 | **Tenant config cache** — ConcurrentHashMap with configurable TTL (default 60s, `cycles.tenant-config.cache-ttl-ms`) | Saves 1 Redis GET per reserve/event | `RedisReservationRepository.getTenantConfig()` |
207+
| 7 | **Minor** — Static AntPathMatcher, ThreadLocal MessageDigest, debug-level hot-path logs | Reduces per-request allocations and log volume | Various |
208+
209+
**Thread safety**: All caches use `ConcurrentHashMap` with immutable record values. No locking required.
210+
**Backward compatibility**: Old reservations without `budgeted_scopes` field handled via `budgeted_scopes_json or affected_scopes_json` fallback in all Lua scripts.
211+
212+
### Performance Benchmarks
213+
214+
End-to-end HTTP latency measured with `CyclesProtocolBenchmarkTest` (Spring Boot + Jedis + Redis 7 via Testcontainers). 200 measured iterations after 50 warmup iterations per operation.
215+
216+
| Operation | p50 | p95 | p99 | min | max | mean |
217+
|-------------------|--------|--------|--------|--------|--------|--------|
218+
| Reserve | 5.2ms | 6.0ms | 6.4ms | 4.0ms | 6.8ms | 5.2ms |
219+
| Commit | 4.1ms | 4.7ms | 5.2ms | 2.6ms | 5.3ms | 4.1ms |
220+
| Release | 4.4ms | 5.6ms | 6.6ms | 3.5ms | 14.0ms | 4.5ms |
221+
| Extend | 8.5ms | 10.7ms | 11.4ms | 7.0ms | 21.4ms | 8.7ms |
222+
| Decide | 5.8ms | 6.6ms | 7.6ms | 4.5ms | 15.4ms | 5.8ms |
223+
| Event | 5.2ms | 6.1ms | 10.0ms | 3.9ms | 20.7ms | 5.4ms |
224+
| Reserve + Commit | 12.2ms | 14.1ms | 16.7ms | 10.6ms | 21.5ms | 12.5ms |
225+
| Reserve + Release | 10.3ms | 12.7ms | 14.0ms | 8.4ms | 20.6ms | 10.6ms |
226+
227+
**Notes:**
228+
- Results are from a containerized CI environment (Testcontainers Redis 7-Alpine, localhost networking). Production with dedicated Redis will be faster.
229+
- Latencies include full HTTP round-trip: Spring Boot request handling, auth filter, JSON serialization, Redis EVALSHA, Lua execution, response building.
230+
- The BCrypt cache eliminates ~100ms+ from all operations after the first request per API key (60s cache window).
231+
- Run benchmarks: `mvn test -Dgroups=benchmark` (requires Docker)
232+
188233
---
189234

190235
## Previously Found Issues (all fixed)
@@ -241,4 +286,4 @@ Two-pass audit covering:
241286

242287
## Verdict
243288

244-
The server implementation is **fully compliant** with the YAML spec (v0.1.23). All 9 endpoints are implemented, all schemas match, auth/tenancy/idempotency are correctly enforced, and the normative behavioral requirements (atomic operations, debt/overdraft handling, scope derivation, error semantics, dry-run rules, grace period handling) are properly implemented. Test coverage expanded to 494 tests across 22 test classes. No remaining spec violations found.
289+
The server implementation is **fully compliant** with the YAML spec (v0.1.23) and **performance optimized**. All 9 endpoints are implemented, all schemas match, auth/tenancy/idempotency are correctly enforced, and the normative behavioral requirements (atomic operations, debt/overdraft handling, scope derivation, error semantics, dry-run rules, grace period handling) are properly implemented. Seven performance optimizations reduce hot-path latency by eliminating redundant Redis round-trips, caching BCrypt validation, and returning balance snapshots atomically from Lua scripts. Test coverage expanded to 522 tests across 24 test classes, including 8 performance benchmark tests. Single-operation p50 latency: 4.1-8.5ms. Full reserve-commit lifecycle p50: 12.2ms. No remaining spec violations found.

cycles-protocol-service/cycles-protocol-service-api/src/main/java/io/runcycles/protocol/api/auth/ApiKeyAuthenticationFilter.java

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@
2727
public class ApiKeyAuthenticationFilter extends OncePerRequestFilter {
2828

2929
private static final Logger LOG = LoggerFactory.getLogger(ApiKeyAuthenticationFilter.class);
30+
private static final AntPathMatcher PATH_MATCHER = new AntPathMatcher();
3031
@Autowired
3132
private ApiKeyValidationService apiKeyValidationService;
3233
@Autowired
@@ -40,7 +41,7 @@ protected void doFilterInternal(
4041
throws ServletException, IOException {
4142

4243
String apiKey = request.getHeader("X-Cycles-API-Key");
43-
LOG.info("Authorization filter request got: apiKey={}",
44+
LOG.debug("Authorization filter: apiKey={}",
4445
apiKey != null && apiKey.length() > 8 ? apiKey.substring(0, 8) + "***" : "***");
4546

4647
if (apiKey == null || apiKey.isBlank()) {
@@ -72,10 +73,8 @@ protected void doFilterInternal(
7273
@Override
7374
protected boolean shouldNotFilter(HttpServletRequest request) {
7475
String path = request.getRequestURI();
75-
AntPathMatcher matcher = new AntPathMatcher();
76-
7776
for (String pattern : SecurityConfig.PUBLIC_PATHS) {
78-
if (matcher.match(pattern, path)) {
77+
if (PATH_MATCHER.match(pattern, path)) {
7978
return true;
8079
}
8180
}

cycles-protocol-service/cycles-protocol-service-api/src/main/resources/application.properties

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@ cycles.expiry.interval-ms=5000
1717

1818
# Logging
1919
logging.level.root=INFO
20-
logging.level.io.runcycles.protocol=DEBUG
20+
logging.level.io.runcycles.protocol=INFO
2121
logging.pattern.console=%d{yyyy-MM-dd HH:mm:ss.SSS} [%thread] %-5level %logger{36} - %msg%n
2222

2323
# OpenAPI/Swagger

0 commit comments

Comments
 (0)