Refactor: 성능 개선, 로직 수정 - #15
Merged
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRefactors slot acquisition to return SlotAcquire(status, enteredAt), rewrites Redis Lua to return [status, enteredAt], switches SSE sending to direct payloads (no ObjectMapper), adds an emitter reverse index, introduces Feign-based match-admin authorization, and updates Docker/config/logging/security/async settings. ChangesQueue Acquisition and SSE Flow Refactoring
Sequence DiagramsequenceDiagram
participant Client
participant QueueService
participant QueueRedisSubscriber
participant RedisLua as Redis(Lua)
participant SseEmitter
Client->>QueueService: SSE subscribe(matchId,userId)
QueueService->>QueueRedisSubscriber: pushStatus(matchId,userId,rank,total)
QueueRedisSubscriber->>RedisLua: ACQUIRE_SLOT_AND_TOKEN_SCRIPT(KEYS..., enteredAtKey, ARGV...)
RedisLua-->>QueueRedisSubscriber: [status, enteredAt] or [-N]
QueueRedisSubscriber->>QueueService: issue token / send status / complete emitter
QueueService->>SseEmitter: SseEmitter.event().data(UserStatusResponse)
SseEmitter-->>Client: SSE message
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (1 warning, 2 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/org/ticketing/queue/application/service/QueueService.java (1)
184-211:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse a dedicated
TaskExecutorfor the per-user SSE push work instead ofparallelStream().The per-user work inside the stream—
queueRedisRepository.acquireSlotAndToken()(Redis Lua eval) andemitter.send()(blocking I/O)—is not suitable forparallelStream(). It dispatches to the JVM-wideForkJoinPool.commonPool()(default parallelism = cores − 1), which is sized for CPU-bound work, not blocking I/O. Under load this starves otherparallelStreamconsumers in the same JVM and caps throughput well below the number of concurrent users.Additionally, operations like
sseEmitterRepository.find()(line 194) andemitter.complete()(line 199) outside the exception-handling block inpushStatus()can throw uncaught exceptions and short-circuit the batch.Inject a
TaskExecutororThreadPoolTaskExecutorwith a bounded pool sized for I/O, and wrap each user's work in a try-catch to isolate failures:♻️ Sketch — bounded I/O executor + per-user isolation
- // parallel stream으로 변경 - userIds.parallelStream().forEach(userId -> { - SseEmitter emitter = sseEmitterRepository.find(matchId, userId); - if (emitter == null) return; - - // rank == null, 이미 exit() 됐는데 emitter만 남은 경우 → 정리 - Long rank = ranks.get(userId); - if (rank == null) { - emitter.complete(); // onCompletion → remove() - return; - } - - // 무조건 pushStatus → Lua 스크립트가 슬롯 판단 - queueRedisSubscriber.pushStatus(matchId, userId, emitter, rank, totalCount); - }); + for (UUID userId : userIds) { + sseTaskExecutor.execute(() -> { + try { + SseEmitter emitter = sseEmitterRepository.find(matchId, userId); + if (emitter == null) return; + + Long rank = ranks.get(userId); + if (rank == null) { + emitter.complete(); // onCompletion → remove() + return; + } + queueRedisSubscriber.pushStatus(matchId, userId, emitter, rank, totalCount); + } catch (Exception e) { + log.warn("[SSE] pushStatus 실패. matchId={}, userId={}", matchId, userId, e); + } + }); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/org/ticketing/queue/application/service/QueueService.java` around lines 184 - 211, The pushStatusToAll method must stop using userIds.parallelStream() and instead dispatch per-user work to a dedicated I/O-oriented TaskExecutor (e.g., a ThreadPoolTaskExecutor bean) sized/bounded for blocking operations; inject the TaskExecutor into QueueService and for each userId submit a Runnable that performs sseEmitterRepository.find(matchId, userId), checks emitter null, reads rank from ranks, and calls queueRedisSubscriber.pushStatus(matchId, userId, emitter, rank, totalCount). Wrap the entire per-user Runnable in a try-catch to isolate exceptions (so exceptions from sseEmitterRepository.find, emitter.complete, or pushStatus don't short-circuit other tasks), ensure emitter.complete() is called only inside the try-catch cleanup when rank==null, and do not use ForkJoinPool.commonPool()/parallelStream so Redis Lua eval (queueRedisRepository.acquireSlotAndToken) and blocking emitter.send() run on the dedicated pool.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.gitignore:
- Around line 2-4: Remove the README.md and .gitignore entries from the
.gitignore file so those files are tracked; specifically delete the lines
containing "README.md" and ".gitignore" (leave or optionally remove ".git" since
it's redundant), then commit the updated .gitignore so documentation and ignore
rules are version-controlled.
In `@Dockerfile`:
- Around line 17-23: The Dockerfile currently runs the image as root; create a
non-root user and group (e.g., appuser/appgroup) in the Dockerfile, chown the
application files (the copied app.jar) and working directory (WORKDIR /app) to
that user, and then set USER to that non-root user before the ENTRYPOINT; update
any file permissions as needed so the JVM can read/execute app.jar and the
process uses the non-root account rather than root.
In `@src/main/java/org/ticketing/queue/application/service/QueueService.java`:
- Around line 156-161: In subscribe (QueueService.subscribe) after calling
queueRedisRepository.getRank(matchId, userId) and getTotalCount, add the same
null-guard used in pushStatusToAll: if rank == null then complete the emitter
and return instead of forwarding to queueRedisSubscriber.pushStatus; this
ensures the TOCTOU case (getPassToken → getRank) won't pass a null rank into
pushStatus or the Lua slot path—locate getRank/getTotalCount calls and the
queueRedisSubscriber.pushStatus invocation and mirror pushStatusToAll's
null-check and emitter completion behavior.
- Around line 163-175: The catch block that handles Exception e in QueueService
should always finalize and remove the SSE emitter: after attempting
emitter.send(...) (which may throw IOException), ensure you call
emitter.completeWithError(e) and sseEmitterRepository.remove(matchId, userId)
regardless of whether the send succeeded; rename the inner catch variable from
ignored to ioe and, if send throws, log ioe along with the original exception e
so you don't lose error context. Target the catch surrounding emitter.send in
QueueService where matchId, userId, emitter and sseEmitterRepository are used
and make the cleanup unconditional while preserving/error-logging both
exceptions.
In
`@src/main/java/org/ticketing/queue/infrastructure/config/QueueSecurityConfig.java`:
- Around line 14-29: The SecurityFilterChain bean queueFilterChain in
QueueSecurityConfig currently exposes a global permitAll for "/**" at highest
precedence (`@Order`(1)); restrict this test-only chain so it does not load in
production by marking it with a test-only activation (e.g., annotate the bean
method or the QueueSecurityConfig class with `@Profile`("test") or an equivalent
test-only conditional) and ensure production profiles do not include that
profile; keep the bean name queueFilterChain, Order(1) and
securityMatcher("/**") unchanged but gated behind the test profile.
In
`@src/main/java/org/ticketing/queue/infrastructure/persistence/QueueRedisRepositoryImpl.java`:
- Around line 82-90: The validation reads ticketOpenAt (openAtValue) as seconds
but compares it to Instant.now().toEpochMilli(), causing a units mismatch in
validateTicketOpenAt(); fix by using consistent epoch units: either parse
openAtValue as seconds and compare to Instant.now().getEpochSecond(), or convert
openAtEpoch to milliseconds (openAtEpoch * 1000) before comparing to nowEpoch;
update the same logic in the other occurrence (the block around the second
validation at lines 167-175) and keep references to initSlots(), ticketOpenAt,
validateTicketOpenAt(), openAtValue, openAtEpoch, and nowEpoch to locate and
change the comparisons accordingly.
In
`@src/main/java/org/ticketing/queue/infrastructure/persistence/SseEmitterRepository.java`:
- Around line 29-37: The remove method currently removes the key from emitters
then mutates matchUserIndex's Set and calls matchUserIndex.remove(matchId,
userIds), which can race and delete a newly-repopulated mapping; make the
reverse-index pruning atomic by using an atomic map operation on matchUserIndex
(e.g., compute/computeIfPresent) inside remove(UUID matchId, UUID userId): first
remove the emitter via emitters.remove(buildKey(matchId, userId)) then call
matchUserIndex.compute(matchId, (k, users) -> { if users==null -> return null;
users.remove(userId); return users.isEmpty() ? null : users; }) so the removal
only happens if the same value is still present and concurrent saves cannot be
lost.
In `@src/main/java/org/ticketing/queue/infrastructure/util/RuaScript.java`:
- Around line 44-45: The Lua script in RuaScript.java currently authorizes users
based on HGET from enteredAtKey (local enteredAtKey = KEYS[3]) which can race
with exit()/refreshQueue() that remove ZSET membership; change the script to use
the sorted-set membership check (ZSCORE on the queue key) as the authoritative
check before granting a slot and treat enteredAtKey as metadata only. Update all
occurrences (the blocks around the lines with enteredAtKey and userId, and the
similar logic at lines 59-63 and 76-79) to expect queueKey as an extra KEYS
parameter, call ZSCORE(queueKey, userId) and only proceed if ZSCORE is
non-nil/valid, while leaving HGET on enteredAtKey solely for metadata reads;
also update the caller(s) to pass queueKey in KEYS when invoking the script.
In `@src/main/resources/application-docker.yml`:
- Around line 29-42: The DB schema defaults are inconsistent: the JDBC URL uses
${DB_SCHEMA:queue_service} while Hibernate uses
hibernate.properties.hibernate.default_schema: ${DB_SCHEMA:queue}, causing
operations to target different schemas; fix by unifying the DB_SCHEMA default so
both use the same placeholder value (e.g., change
hibernate.properties.hibernate.default_schema to ${DB_SCHEMA:queue_service} or
change the URL to ${DB_SCHEMA:queue}) so that the url property and
hibernate.properties.hibernate.default_schema reference the identical DB_SCHEMA
default.
In `@src/main/resources/application.yml`:
- Line 6: The import line "import: optional:configserver:http://localhost:10002"
hardcodes a local Config Server URL; change it to use a configurable property or
environment variable instead (e.g., replace the literal URL with a placeholder
like ${CONFIG_SERVER_URL} or a Spring property such as
${spring.cloud.config.uri}) so the Config Server host/port can be provided per
environment; update the "import: optional:configserver:..." entry accordingly
and ensure default/overrides are documented or provided via
environment/configuration.
---
Outside diff comments:
In `@src/main/java/org/ticketing/queue/application/service/QueueService.java`:
- Around line 184-211: The pushStatusToAll method must stop using
userIds.parallelStream() and instead dispatch per-user work to a dedicated
I/O-oriented TaskExecutor (e.g., a ThreadPoolTaskExecutor bean) sized/bounded
for blocking operations; inject the TaskExecutor into QueueService and for each
userId submit a Runnable that performs sseEmitterRepository.find(matchId,
userId), checks emitter null, reads rank from ranks, and calls
queueRedisSubscriber.pushStatus(matchId, userId, emitter, rank, totalCount).
Wrap the entire per-user Runnable in a try-catch to isolate exceptions (so
exceptions from sseEmitterRepository.find, emitter.complete, or pushStatus don't
short-circuit other tasks), ensure emitter.complete() is called only inside the
try-catch cleanup when rank==null, and do not use
ForkJoinPool.commonPool()/parallelStream so Redis Lua eval
(queueRedisRepository.acquireSlotAndToken) and blocking emitter.send() run on
the dedicated pool.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 82df3a02-fa11-45e9-9ddb-c0298f10cfe2
📒 Files selected for processing (16)
.gitignoreDockerfilepostgres/init.sqlsrc/main/java/org/ticketing/queue/application/service/QueueService.javasrc/main/java/org/ticketing/queue/domain/model/AcquireResult.javasrc/main/java/org/ticketing/queue/domain/model/SlotAcquire.javasrc/main/java/org/ticketing/queue/domain/repository/QueueRedisRepository.javasrc/main/java/org/ticketing/queue/infrastructure/config/QueueSecurityConfig.javasrc/main/java/org/ticketing/queue/infrastructure/config/SseConfig.javasrc/main/java/org/ticketing/queue/infrastructure/persistence/QueueRedisRepositoryImpl.javasrc/main/java/org/ticketing/queue/infrastructure/persistence/SseEmitterRepository.javasrc/main/java/org/ticketing/queue/infrastructure/redis/pubsub/QueueRedisSubscriber.javasrc/main/java/org/ticketing/queue/infrastructure/util/RuaScript.javasrc/main/resources/application-docker.ymlsrc/main/resources/application.ymlsrc/main/resources/logback.xml
Comment on lines
+44
to
45
| local enteredAtKey = KEYS[3] | ||
| local userId = ARGV[1] |
There was a problem hiding this comment.
Use the sorted set as the authoritative queue-membership check.
This now grants success based on HGET enteredAtKey, but exit()/refreshQueue() remove the ZSET membership and the enteredAt hash in separate Redis calls. That leaves a race where a user already removed from the queue can still pass this check and receive a slot/token. Check ZSCORE on the queue key before success, and treat enteredAt as metadata only.
Suggested fix
- local enteredAtKey = KEYS[3]
+ local enteredAtKey = KEYS[3]
+ local queueKey = KEYS[4]
@@
- local enteredAt = redis.call('HGET', enteredAtKey, userId)
- if not enteredAt then
+ if not redis.call('ZSCORE', queueKey, userId) then
return {-5} -- USER_NOT_IN_QUEUE: ban/refresh/rollback 등으로 이미 제거됨
end
+ local enteredAt = redis.call('HGET', enteredAtKey, userId)
+ if not enteredAt then
+ return {-5}
+ endThe caller should then pass queueKey as an additional script key.
Also applies to: 59-63, 76-79
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/java/org/ticketing/queue/infrastructure/util/RuaScript.java` around
lines 44 - 45, The Lua script in RuaScript.java currently authorizes users based
on HGET from enteredAtKey (local enteredAtKey = KEYS[3]) which can race with
exit()/refreshQueue() that remove ZSET membership; change the script to use the
sorted-set membership check (ZSCORE on the queue key) as the authoritative check
before granting a slot and treat enteredAtKey as metadata only. Update all
occurrences (the blocks around the lines with enteredAtKey and userId, and the
similar logic at lines 59-63 and 76-79) to expect queueKey as an extra KEYS
parameter, call ZSCORE(queueKey, userId) and only proceed if ZSCORE is
non-nil/valid, while leaving HGET on enteredAtKey solely for metadata reads;
also update the caller(s) to pass queueKey in KEYS when invoking the script.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
src/main/resources/application-docker.yml (1)
34-34:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSchema default mismatch with
init.sql—verify against actual schema name.Both the datasource
currentSchema(Line 34) and Hibernatedefault_schema(Line 47) default toqueue, which is now internally consistent. However, according to the AI summary,postgres/init.sqlcreates aqueue_serviceschema.If the actual database schema is
queue_service, the application will fail to find tables at runtime. Ensure theDB_SCHEMAdefault matches the schema name created by your initialization scripts.🔧 Proposed fix (if init.sql creates queue_service)
- url: jdbc:postgresql://${DB_HOST:ticketing-postgres}:${DB_PORT:15432}/${DB_NAME:ticketing}?currentSchema=${DB_SCHEMA:queue} + url: jdbc:postgresql://${DB_HOST:ticketing-postgres}:${DB_PORT:15432}/${DB_NAME:ticketing}?currentSchema=${DB_SCHEMA:queue_service}- default_schema: ${DB_SCHEMA:queue} + default_schema: ${DB_SCHEMA:queue_service}Also applies to: 47-47
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/resources/application-docker.yml` at line 34, Datasource `currentSchema` and Hibernate `default_schema` defaults use "queue" but init.sql creates "queue_service"; update the defaults to match the actual schema or change init.sql. Modify the DB_SCHEMA default value used in the URL (the ${DB_SCHEMA:queue} default) and the Hibernate property referenced as default_schema to use "queue_service" (or alternately rename the schema in postgres/init.sql to "queue") so both currentSchema and default_schema match the schema created by postgres/init.sql.
🧹 Nitpick comments (1)
src/test/java/org/ticketing/queue/application/service/QueueServiceTest.java (1)
248-250: ⚡ Quick winStrengthen
pushStatusverification to assert expected values.Using
any()for all parameters makes this test permissive and can hide regressions in rank/total or ID propagation. Assert concrete arguments like in the immediate-token test.Suggested test assertion update
- verify(queueRedisSubscriber) - .pushStatus(any(), any(), any(), any(), any()); + verify(queueRedisSubscriber) + .pushStatus(eq(matchId), eq(userId), any(SseEmitter.class), eq(10L), eq(100L));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/java/org/ticketing/queue/application/service/QueueServiceTest.java` around lines 248 - 250, The test uses overly-permissive any() matchers for queueRedisSubscriber.pushStatus; tighten it by asserting the concrete expected arguments (e.g., the specific message ID, rank, total, and any other expected fields) similar to the immediate-token test pattern. Update the verification call in QueueServiceTest so verify(queueRedisSubscriber).pushStatus(...) uses exact matchers or eq(...) for the expected ID, rank and total values produced by the unit under test (and only keep any() for parameters that truly vary), ensuring pushStatus receives the precise values you expect.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/resources/application-docker.yml`:
- Around line 1-8: Current Tomcat thread pool (tomcat.threads.max = 1000) far
exceeds the DB connection pool; update configuration so thread count and DB pool
are balanced: either reduce tomcat.threads.max to a realistic 200–300 (and
adjust min-spare accordingly) or increase hikari.maximum-pool-size to ~100–150
so application threads won’t block waiting for connections; ensure the chosen
value respects PostgreSQL’s max_connections across all app instances and keep
tomcat.threads.min-spare consistent with the new max.
- Around line 74-76: The queue token secret (queue.token.secret) is required but
has no default, causing startup failure if QUEUE_TOKEN_SECRET isn't set; fix by
either adding a safe dev default in application-docker.yml (e.g. use the
Spring-style fallback syntax ${QUEUE_TOKEN_SECRET:your-dev-default}) or
explicitly document that QUEUE_TOKEN_SECRET is mandatory for production and must
be supplied in deployment docs/config; update the file to include the fallback
or update README/ops docs to mark QUEUE_TOKEN_SECRET as required and describe
how to provision it.
- Line 52: The bootstrap-servers entry currently relies solely on
SPRING_KAFKA_BOOTSTRAP_SERVERS which will cause startup failure if the env var
is missing; update the application-docker.yml to provide a sensible default for
bootstrap-servers (for example use
${SPRING_KAFKA_BOOTSTRAP_SERVERS:localhost:9092} or a Docker service name) or
add a clear comment above the bootstrap-servers property marking
SPRING_KAFKA_BOOTSTRAP_SERVERS as required and documenting the expected format;
target the bootstrap-servers property and SPRING_KAFKA_BOOTSTRAP_SERVERS
variable in your change.
- Around line 28-31: The Docker profile currently sets spring.sql.init.mode
(sql.init.mode) to "always" which will re-run classpath:db/init-schema.sql on
every container restart; change sql.init.mode to "never" for Docker deployments
so initialization is not auto-run, or alternatively make the referenced init
script (classpath:db/init-schema.sql) fully idempotent (use IF NOT EXISTS for
CREATE statements and safe DDL) so repeated runs are safe; update the
application-docker.yml entry for sql.init.mode and/or refactor the
init-schema.sql accordingly.
---
Duplicate comments:
In `@src/main/resources/application-docker.yml`:
- Line 34: Datasource `currentSchema` and Hibernate `default_schema` defaults
use "queue" but init.sql creates "queue_service"; update the defaults to match
the actual schema or change init.sql. Modify the DB_SCHEMA default value used in
the URL (the ${DB_SCHEMA:queue} default) and the Hibernate property referenced
as default_schema to use "queue_service" (or alternately rename the schema in
postgres/init.sql to "queue") so both currentSchema and default_schema match the
schema created by postgres/init.sql.
---
Nitpick comments:
In `@src/test/java/org/ticketing/queue/application/service/QueueServiceTest.java`:
- Around line 248-250: The test uses overly-permissive any() matchers for
queueRedisSubscriber.pushStatus; tighten it by asserting the concrete expected
arguments (e.g., the specific message ID, rank, total, and any other expected
fields) similar to the immediate-token test pattern. Update the verification
call in QueueServiceTest so verify(queueRedisSubscriber).pushStatus(...) uses
exact matchers or eq(...) for the expected ID, rank and total values produced by
the unit under test (and only keep any() for parameters that truly vary),
ensuring pushStatus receives the precise values you expect.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5d17edd0-bdc0-4745-a874-5eb75ce13afc
📒 Files selected for processing (2)
src/main/resources/application-docker.ymlsrc/test/java/org/ticketing/queue/application/service/QueueServiceTest.java
Comment on lines
+1
to
+8
| server: | ||
| tomcat: | ||
| threads: | ||
| max: 1000 | ||
| min-spare: 100 | ||
| accept-count: 1000 | ||
| max-connections: 10000 | ||
|
|
There was a problem hiding this comment.
Thread pool far exceeds DB connection pool—threads will starve.
The configuration creates 1000 Tomcat threads (Line 4) but only 30 database connections (Line 39), a ratio of ~33:1. Most threads will block waiting for a DB connection, causing:
- Increased latency and timeouts
- Thread starvation
- Wasted memory (idle threads)
This contradicts the PR's performance improvement goals. Either:
- Reduce
tomcat.threads.maxto a realistic value (e.g., 200-300), or - Increase
hikari.maximum-pool-sizeproportionally (e.g., 100-150)
Consider that PostgreSQL's max_connections also limits total connections across all app instances.
🔧 Recommended fix (balanced approach)
tomcat:
threads:
- max: 1000
- min-spare: 100
+ max: 300
+ min-spare: 30
accept-count: 1000
- max-connections: 10000
+ max-connections: 3000 hikari:
- maximum-pool-size: 30
- minimum-idle: 10
+ maximum-pool-size: 100
+ minimum-idle: 30Also applies to: 39-40
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/resources/application-docker.yml` around lines 1 - 8, Current Tomcat
thread pool (tomcat.threads.max = 1000) far exceeds the DB connection pool;
update configuration so thread count and DB pool are balanced: either reduce
tomcat.threads.max to a realistic 200–300 (and adjust min-spare accordingly) or
increase hikari.maximum-pool-size to ~100–150 so application threads won’t block
waiting for connections; ensure the chosen value respects PostgreSQL’s
max_connections across all app instances and keep tomcat.threads.min-spare
consistent with the new max.
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (4)
src/main/resources/application-docker.yml (4)
1-8:⚠️ Potential issue | 🟠 MajorThread pool far exceeds DB connection pool—threads will starve.
This issue was previously flagged and remains unaddressed. With 1000 Tomcat threads but only 30 database connections, most threads will block waiting for a connection, causing increased latency, timeouts, and wasted memory.
Also applies to: 34-35
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/resources/application-docker.yml` around lines 1 - 8, The Tomcat thread pool configured under server.tomcat (keys server.tomcat.threads.max, server.tomcat.threads.min-spare, accept-count, max-connections) is set far higher than the database connection pool, causing threads to block and starve; fix this by aligning Tomcat max/min threads with your DB pool or increasing the DB pool size: either reduce server.tomcat.threads.max/min-spare and accept-count to a value close to your DB connection pool, or raise the DB pool (e.g., spring.datasource.* / HikariCP maximum-pool-size) so the number of DB connections matches expected concurrent servlet threads, and ensure related entries (the duplicate settings referenced on lines 34-35) are updated consistently.
47-47:⚠️ Potential issue | 🔴 CriticalProvide a default for
SPRING_KAFKA_BOOTSTRAP_SERVERSor document it as required.This issue was previously flagged and remains unaddressed. The application will fail to start if the environment variable is not set.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/resources/application-docker.yml` at line 47, The kafka bootstrap property is using an unset env var and can cause startup failure; update the application-docker.yml entry for bootstrap-servers (SPRING_KAFKA_BOOTSTRAP_SERVERS) to supply a safe default using Spring env syntax (e.g. bootstrap-servers: ${SPRING_KAFKA_BOOTSTRAP_SERVERS:localhost:9092}) or alternatively document SPRING_KAFKA_BOOTSTRAP_SERVERS as REQUIRED in the deployment README / .env.sample so operators know to set it.
29-29:⚠️ Potential issue | 🟠 MajorUnify schema defaults to avoid cross-schema behavior.
This issue was previously flagged as addressed but the mismatch persists. Line 29 defaults
DB_SCHEMAtoqueue_service, while Line 42 defaults toqueue. Runtime reads/writes and DDL will target different schemas.Also applies to: 42-42
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/resources/application-docker.yml` at line 29, The DB_SCHEMA default is inconsistent: the JDBC URL line uses ${DB_SCHEMA:queue_service} while another entry uses ${DB_SCHEMA:queue}; update both occurrences of DB_SCHEMA (the JDBC url configuration and the other DB_SCHEMA reference) to use the same default value so runtime reads/writes and DDL target the same schema (choose and apply either "queue" or "queue_service" consistently across the DB_SCHEMA substitutions).
94-96:⚠️ Potential issue | 🔴 CriticalProvide a default for
QUEUE_TOKEN_SECRETor document it as required.This issue was previously flagged and remains unaddressed. The application will fail to start if the environment variable is not set.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/resources/application-docker.yml` around lines 94 - 96, The queue.token.secret property uses an unset environment variable QUEUE_TOKEN_SECRET which will break startup; either supply a safe default inline (e.g. change queue.token.secret to use the Spring placeholder with a fallback like ${QUEUE_TOKEN_SECRET:your-default-or-empty}) or add a clear comment/README entry marking QUEUE_TOKEN_SECRET as required for docker deployments and instruct operators how to set it; update the application-docker.yml entry for queue.token.secret (and any deployment manifest) accordingly so the app never relies on an undefined env var at runtime.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/main/java/org/ticketing/queue/application/service/MatchAuthorizationService.java`:
- Around line 29-37: The code is creating blocking Feign calls with
CompletableFuture.supplyAsync(...) without a dedicated executor and is using
join() which wraps domain exceptions in CompletionException; fix by supplying a
dedicated Executor (e.g., an injected or created ExecutorService) to
CompletableFuture.supplyAsync(...) when calling clubFeignClient.getClub(...,
SERVICE_NAME) for both home and away, and replace the direct use of
homeFuture.join() / awayFuture.join() with an unwrap pattern that catches
CompletionException and rethrows its cause (preserving
NotFoundClubMatchException and other domain exceptions) or a small helper that
returns future.join() but unwraps CompletionException.getCause() before
propagating.
In
`@src/main/java/org/ticketing/queue/infrastructure/feign/ClubFeignClientFallback.java`:
- Around line 15-18: The ClubFeignClientFallback.getClub currently maps every
failure to NotFoundClubMatchException (404); change this to use a
FallbackFactory for the Feign client so you can inspect the cause
(exception/response) and only throw NotFoundClubMatchException when the upstream
response truly indicates 404, otherwise throw a service-unavailable style
exception (e.g., ServiceUnavailableException or a custom upstream error) for
timeouts/network/5xx; follow the same pattern used in MatchFeignClientFallback
to access the Throwable cause, check for FeignException and its status(), and
map to the appropriate exception instead of always throwing
NotFoundClubMatchException.
In
`@src/main/java/org/ticketing/queue/presentation/controller/QueueController.java`:
- Around line 138-142: The banUser endpoint is validating the wrong user: add a
RequestHeader parameter for the requesting user's ID (annotate a UUID parameter
with `@RequestHeader`("X-User-Id") requestingUserId) in the
QueueController.banUser method and change the call to
matchAuthorizationService.validateClubAdmin(...) to pass requestingUserId
instead of the banned userId so the club-admin check verifies the requester, not
the target.
- Around line 139-140: Validate and normalize the incoming roles header before
creating roleList: ensure the variable roles (in QueueController) is checked for
null or blank and return/handle unauthorized if missing; when present, split on
commas, trim each entry and filter out empty strings (e.g. map(String::trim) and
filter(s -> !s.isEmpty())) before collecting to roleList, then use that
normalized list for contains("CLUB_ADMIN") checks to avoid NPEs and incorrect
matches.
- Line 138: The banUser method currently reads an untrusted X-User-Roles header;
remove that header parameter from the banUser(`@PathVariable`("matchId") UUID
matchId, `@PathVariable` UUID userId, `@RequestHeader`("X-User-Roles") String roles)
signature and instead obtain the caller's roles from the authenticated principal
via the Spring Security context (e.g. SecurityContextHolder/Authentication or a
`@AuthenticationPrincipal` parameter) and use those authorities for
authorization/@PreAuthorize checks; also update your security configuration
(SecurityConfig) to enforce authentication/authorization at the framework level
and to strip/reject externally supplied role headers so requests cannot spoof
roles.
In `@src/main/resources/application-docker.yml`:
- Around line 83-84: The YAML currently records java.lang.Exception for circuit
breaker/retry (the record-exceptions entries), which is too broad; narrow these
to only transient failure types by replacing java.lang.Exception with specific
transient exception classes (e.g., java.io.IOException,
java.net.SocketTimeoutException, java.util.concurrent.TimeoutException,
org.springframework.web.client.ResourceAccessException and server 5xx related
exceptions such as org.springframework.web.client.HttpServerErrorException) so
that validation/business/4xx errors are not retried or tripped; update both
record-exceptions blocks (the one shown and the other occurrence at lines 91-92)
to the selected transient exceptions.
---
Duplicate comments:
In `@src/main/resources/application-docker.yml`:
- Around line 1-8: The Tomcat thread pool configured under server.tomcat (keys
server.tomcat.threads.max, server.tomcat.threads.min-spare, accept-count,
max-connections) is set far higher than the database connection pool, causing
threads to block and starve; fix this by aligning Tomcat max/min threads with
your DB pool or increasing the DB pool size: either reduce
server.tomcat.threads.max/min-spare and accept-count to a value close to your DB
connection pool, or raise the DB pool (e.g., spring.datasource.* / HikariCP
maximum-pool-size) so the number of DB connections matches expected concurrent
servlet threads, and ensure related entries (the duplicate settings referenced
on lines 34-35) are updated consistently.
- Line 47: The kafka bootstrap property is using an unset env var and can cause
startup failure; update the application-docker.yml entry for bootstrap-servers
(SPRING_KAFKA_BOOTSTRAP_SERVERS) to supply a safe default using Spring env
syntax (e.g. bootstrap-servers:
${SPRING_KAFKA_BOOTSTRAP_SERVERS:localhost:9092}) or alternatively document
SPRING_KAFKA_BOOTSTRAP_SERVERS as REQUIRED in the deployment README /
.env.sample so operators know to set it.
- Line 29: The DB_SCHEMA default is inconsistent: the JDBC URL line uses
${DB_SCHEMA:queue_service} while another entry uses ${DB_SCHEMA:queue}; update
both occurrences of DB_SCHEMA (the JDBC url configuration and the other
DB_SCHEMA reference) to use the same default value so runtime reads/writes and
DDL target the same schema (choose and apply either "queue" or "queue_service"
consistently across the DB_SCHEMA substitutions).
- Around line 94-96: The queue.token.secret property uses an unset environment
variable QUEUE_TOKEN_SECRET which will break startup; either supply a safe
default inline (e.g. change queue.token.secret to use the Spring placeholder
with a fallback like ${QUEUE_TOKEN_SECRET:your-default-or-empty}) or add a clear
comment/README entry marking QUEUE_TOKEN_SECRET as required for docker
deployments and instruct operators how to set it; update the
application-docker.yml entry for queue.token.secret (and any deployment
manifest) accordingly so the app never relies on an undefined env var at
runtime.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a1c892b9-45ea-46f9-ac51-75cfb4f6ec4b
📒 Files selected for processing (11)
src/main/java/org/ticketing/queue/application/service/MatchAuthorizationService.javasrc/main/java/org/ticketing/queue/domain/exception/NotFoundClubMatchException.javasrc/main/java/org/ticketing/queue/domain/exception/UnauthorizedClubAdminException.javasrc/main/java/org/ticketing/queue/infrastructure/feign/ClubFeignClient.javasrc/main/java/org/ticketing/queue/infrastructure/feign/ClubFeignClientFallback.javasrc/main/java/org/ticketing/queue/infrastructure/feign/MatchFeignClient.javasrc/main/java/org/ticketing/queue/infrastructure/feign/MatchFeignClientFallback.javasrc/main/java/org/ticketing/queue/infrastructure/feign/response/ClubResponse.javasrc/main/java/org/ticketing/queue/infrastructure/feign/response/MatchResponse.javasrc/main/java/org/ticketing/queue/presentation/controller/QueueController.javasrc/main/resources/application-docker.yml
Comment on lines
+29
to
+37
| CompletableFuture<UUID> homeFuture = CompletableFuture.supplyAsync(() -> | ||
| clubFeignClient.getClub(match.homeClubId(), SERVICE_NAME).adminId() | ||
| ); | ||
| CompletableFuture<UUID> awayFuture = CompletableFuture.supplyAsync(() -> | ||
| clubFeignClient.getClub(match.awayClubId(), SERVICE_NAME).adminId() | ||
| ); | ||
|
|
||
| UUID homeAdminId = homeFuture.join(); | ||
| UUID awayAdminId = awayFuture.join(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "MatchAuthorizationService.java" | head -20Repository: 3s-ticketing/queue-service
Length of output: 157
🏁 Script executed:
# Get the file size first to ensure we can read it
wc -l src/main/java/org/ticketing/queue/application/service/MatchAuthorizationService.java 2>/dev/null || find . -type f -name "MatchAuthorizationService.java" -exec wc -l {} \;Repository: 3s-ticketing/queue-service
Length of output: 158
🏁 Script executed:
# Read the entire file to see the context
cat -n src/main/java/org/ticketing/queue/application/service/MatchAuthorizationService.java 2>/dev/null || find . -type f -name "MatchAuthorizationService.java" -exec cat -n {} \;Repository: 3s-ticketing/queue-service
Length of output: 2317
🏁 Script executed:
# Search for global exception handlers
find . -type f -name "*ExceptionHandler*.java" -o -name "*ControllerAdvice*.java" | head -20Repository: 3s-ticketing/queue-service
Length of output: 52
🏁 Script executed:
# Search for exception handlers in the codebase
rg -t java "ControllerAdvice|ExceptionHandler|RestControllerAdvice" -A 3 | head -100Repository: 3s-ticketing/queue-service
Length of output: 52
🏁 Script executed:
# Search for executor beans
rg -t java "Executor|ThreadPoolTaskExecutor|ExecutorService" | grep -i bean | head -20Repository: 3s-ticketing/queue-service
Length of output: 52
🏁 Script executed:
# Search for CompletionException handling
rg -t java "CompletionException" | head -20Repository: 3s-ticketing/queue-service
Length of output: 52
🏁 Script executed:
# Search for exception handling in main directory structure
find . -type f \( -name "*.java" -o -name "*.xml" -o -name "*.yml" -o -name "*.yaml" \) | grep -E "(config|exception|handler)" | head -20Repository: 3s-ticketing/queue-service
Length of output: 1270
🏁 Script executed:
# Check for Spring configuration classes
rg -t java "@Configuration|@Bean" | grep -i "exception|error|handler" | head -20Repository: 3s-ticketing/queue-service
Length of output: 52
🏁 Script executed:
# Look for where validateClubAdmin is called
rg "validateClubAdmin" -B 2 -A 2Repository: 3s-ticketing/queue-service
Length of output: 1374
🏁 Script executed:
# Check project structure for any config files
ls -la src/main/resources/ 2>/dev/null || find . -path "*/main/resources/*" -type f | head -20Repository: 3s-ticketing/queue-service
Length of output: 380
🏁 Script executed:
# Read the QueueController to see the context where validateClubAdmin is called
cat -n src/main/java/org/ticketing/queue/presentation/controller/QueueController.java | head -100Repository: 3s-ticketing/queue-service
Length of output: 4263
🏁 Script executed:
# Check for exception handling in the controller class
rg -t java "QueueController" -A 50 | grep -A 50 "validateClubAdmin" | head -40Repository: 3s-ticketing/queue-service
Length of output: 52
🏁 Script executed:
# Check application.yml for error handling
cat src/main/resources/application.ymlRepository: 3s-ticketing/queue-service
Length of output: 1013
🏁 Script executed:
# Check for Feign client and exception handling
cat -n src/main/java/org/ticketing/queue/infrastructure/feign/ClubFeignClient.javaRepository: 3s-ticketing/queue-service
Length of output: 882
🏁 Script executed:
# Look for any exception handler configuration in infrastructure
find . -type f -name "*.java" -path "*/infrastructure/*" | xargs grep -l "ExceptionHandler\|GlobalExceptionHandler" | head -10Repository: 3s-ticketing/queue-service
Length of output: 52
🏁 Script executed:
# Search for Feign exception handling
rg -t java "FeignException|feign" -A 3 | head -50Repository: 3s-ticketing/queue-service
Length of output: 5010
🏁 Script executed:
# Check the ClubFeignClientFallback implementation
cat -n src/main/java/org/ticketing/queue/infrastructure/feign/ClubFeignClientFallback.javaRepository: 3s-ticketing/queue-service
Length of output: 849
🏁 Script executed:
# Check the MatchFeignClientFallback implementation
cat -n src/main/java/org/ticketing/queue/infrastructure/feign/MatchFeignClientFallback.javaRepository: 3s-ticketing/queue-service
Length of output: 858
🏁 Script executed:
# Look for `@ExceptionHandler` anywhere in the codebase
rg -t java "@ExceptionHandler" -B 3 -A 5Repository: 3s-ticketing/queue-service
Length of output: 52
🏁 Script executed:
# Check if NotFoundClubMatchException is a RuntimeException
cat -n src/main/java/org/ticketing/queue/domain/exception/NotFoundClubMatchException.javaRepository: 3s-ticketing/queue-service
Length of output: 584
🏁 Script executed:
# Search for authorizationExecutor or any executor bean that might handle this
rg -t java "authorizationExecutor"Repository: 3s-ticketing/queue-service
Length of output: 52
🏁 Script executed:
# Search for any TaskExecutor or ExecutorService beans
rg -t java "@Bean.*[Ee]xecutor|@Bean.*[Ee]xecutor" -A 2Repository: 3s-ticketing/queue-service
Length of output: 52
Use dedicated executor and unwrap exceptions from CompletableFuture operations for blocking Feign calls.
The current code uses CompletableFuture.supplyAsync() without an executor, which defaults to the common ForkJoinPool for blocking HTTP calls. This creates thread pool contention under load. Additionally, join() wraps all exceptions—including NotFoundClubMatchException—in CompletionException, masking the original domain exception in error responses.
Configure a dedicated executor and unwrap completion exceptions to preserve proper error handling:
Suggested approach
- CompletableFuture<UUID> homeFuture = CompletableFuture.supplyAsync(() ->
+ CompletableFuture<UUID> homeFuture = CompletableFuture.supplyAsync(() ->
clubFeignClient.getClub(match.homeClubId(), SERVICE_NAME).adminId()
- );
+ , authorizationExecutor);
- CompletableFuture<UUID> awayFuture = CompletableFuture.supplyAsync(() ->
+ CompletableFuture<UUID> awayFuture = CompletableFuture.supplyAsync(() ->
clubFeignClient.getClub(match.awayClubId(), SERVICE_NAME).adminId()
- );
+ , authorizationExecutor);
- UUID homeAdminId = homeFuture.join();
- UUID awayAdminId = awayFuture.join();
+ UUID homeAdminId;
+ UUID awayAdminId;
+ try {
+ homeAdminId = homeFuture.join();
+ awayAdminId = awayFuture.join();
+ } catch (CompletionException e) {
+ throw (e.getCause() instanceof RuntimeException re) ? re : e;
+ }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@src/main/java/org/ticketing/queue/application/service/MatchAuthorizationService.java`
around lines 29 - 37, The code is creating blocking Feign calls with
CompletableFuture.supplyAsync(...) without a dedicated executor and is using
join() which wraps domain exceptions in CompletionException; fix by supplying a
dedicated Executor (e.g., an injected or created ExecutorService) to
CompletableFuture.supplyAsync(...) when calling clubFeignClient.getClub(...,
SERVICE_NAME) for both home and away, and replace the direct use of
homeFuture.join() / awayFuture.join() with an unwrap pattern that catches
CompletionException and rethrows its cause (preserving
NotFoundClubMatchException and other domain exceptions) or a small helper that
returns future.join() but unwraps CompletionException.getCause() before
propagating.
Comment on lines
+15
to
+18
| public ClubResponse getClub(UUID clubId, String internalService) { | ||
| log.error("[Feign Fallback] club-service 호출 실패. clubId={}, service={}", clubId, internalService); | ||
| throw new NotFoundClubMatchException(null, clubId); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate and examine the target file
find . -type f -name "ClubFeignClientFallback.java" | head -20Repository: 3s-ticketing/queue-service
Length of output: 156
🏁 Script executed:
# Also search for related files and exception definitions
fd -t f -e java | grep -i "feign\|fallback\|exception" | head -30Repository: 3s-ticketing/queue-service
Length of output: 1429
🏁 Script executed:
# Search for the exception class definitions
rg "class NotFoundClubMatchException" -t javaRepository: 3s-ticketing/queue-service
Length of output: 219
🏁 Script executed:
# Look for any ServiceUnavailable or similar exceptions in the codebase
rg "ServiceUnavailable|ServiceException|FallbackException" -t javaRepository: 3s-ticketing/queue-service
Length of output: 52
🏁 Script executed:
# Check how Feign is configured in the project
rg "@FeignClient" -t java -A 5 | head -50Repository: 3s-ticketing/queue-service
Length of output: 1612
🏁 Script executed:
# Search for other fallback implementations to compare patterns
rg "class.*Fallback.*implements" -t javaRepository: 3s-ticketing/queue-service
Length of output: 373
🏁 Script executed:
# Read the ClubFeignClientFallback.java file
cat -n ./src/main/java/org/ticketing/queue/infrastructure/feign/ClubFeignClientFallback.javaRepository: 3s-ticketing/queue-service
Length of output: 849
🏁 Script executed:
# Read the NotFoundClubMatchException definition
cat -n ./src/main/java/org/ticketing/queue/domain/exception/NotFoundClubMatchException.javaRepository: 3s-ticketing/queue-service
Length of output: 584
🏁 Script executed:
# Check the similar fallback pattern for Match
cat -n ./src/main/java/org/ticketing/queue/infrastructure/feign/MatchFeignClientFallback.javaRepository: 3s-ticketing/queue-service
Length of output: 858
🏁 Script executed:
# Look for all exception classes that might be relevant
cat -n ./src/main/java/org/ticketing/queue/domain/exception/*.java | grep -A 3 "class.*Exception"Repository: 3s-ticketing/queue-service
Length of output: 2723
🏁 Script executed:
# Check if there's a Feign configuration file that specifies error decoding or fallback factory
find . -type f \( -name "*.java" -o -name "*.yml" -o -name "*.yaml" -o -name "*.properties" \) | xargs rg -l "FeignClientConfiguration|ErrorDecoder|fallback" | head -20Repository: 3s-ticketing/queue-service
Length of output: 227
🏁 Script executed:
# Check the CustomException to understand the exception hierarchy
rg "class CustomException" -A 10 -t javaRepository: 3s-ticketing/queue-service
Length of output: 52
Fallback incorrectly maps all upstream failures to 404.
The fallback throws NotFoundClubMatchException (HTTP 404) for all failures—timeouts, network errors, 5xx responses—not just missing resources. This breaks error semantics and prevents clients from distinguishing temporary outages from actual not-found responses. Use a FallbackFactory to access the cause, and throw a service-unavailable exception for non-404 failures. (Same pattern exists in MatchFeignClientFallback.java.)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@src/main/java/org/ticketing/queue/infrastructure/feign/ClubFeignClientFallback.java`
around lines 15 - 18, The ClubFeignClientFallback.getClub currently maps every
failure to NotFoundClubMatchException (404); change this to use a
FallbackFactory for the Feign client so you can inspect the cause
(exception/response) and only throw NotFoundClubMatchException when the upstream
response truly indicates 404, otherwise throw a service-unavailable style
exception (e.g., ServiceUnavailableException or a custom upstream error) for
timeouts/network/5xx; follow the same pattern used in MatchFeignClientFallback
to access the Throwable cause, check for FeignException and its status(), and
map to the appropriate exception instead of always throwing
NotFoundClubMatchException.
Comment on lines
+138
to
+142
| public void banUser(@PathVariable("matchId") UUID matchId, @PathVariable UUID userId, @RequestHeader("X-User-Roles") String roles) { | ||
| List<String> roleList = Arrays.asList(roles.split(",")); | ||
| if (roleList.contains("CLUB_ADMIN")) { | ||
| matchAuthorizationService.validateClubAdmin(matchId, userId); | ||
| } |
There was a problem hiding this comment.
Critical logic error: Validating the wrong user.
The userId path variable represents the user being banned, but line 141 validates whether that banned user is a club admin. This is backwards!
According to the PR objective ("사용자 차단 시 CLUB_ADMIN 권한이면 해당 경기 클럽의 관리자 여부 확인"), when the requesting user has CLUB_ADMIN role, you should verify that the requesting user is an admin of that match's club—not the user being banned.
You need to:
- Add
@RequestHeader("X-User-Id") UUID requestingUserIdparameter to capture the requesting user - Pass
requestingUserId(notuserId) tovalidateClubAdmin
🐛 Proposed fix
- public void banUser(`@PathVariable`("matchId") UUID matchId, `@PathVariable` UUID userId, `@RequestHeader`("X-User-Roles") String roles) {
+ public void banUser(`@PathVariable`("matchId") UUID matchId, `@PathVariable` UUID userId,
+ `@RequestHeader`("X-User-Id") UUID requestingUserId, `@RequestHeader`("X-User-Roles") String roles) {
List<String> roleList = Arrays.asList(roles.split(","));
if (roleList.contains("CLUB_ADMIN")) {
- matchAuthorizationService.validateClubAdmin(matchId, userId);
+ matchAuthorizationService.validateClubAdmin(matchId, requestingUserId);
}
queueService.banUser(matchId, userId);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| public void banUser(@PathVariable("matchId") UUID matchId, @PathVariable UUID userId, @RequestHeader("X-User-Roles") String roles) { | |
| List<String> roleList = Arrays.asList(roles.split(",")); | |
| if (roleList.contains("CLUB_ADMIN")) { | |
| matchAuthorizationService.validateClubAdmin(matchId, userId); | |
| } | |
| public void banUser(`@PathVariable`("matchId") UUID matchId, `@PathVariable` UUID userId, | |
| `@RequestHeader`("X-User-Id") UUID requestingUserId, `@RequestHeader`("X-User-Roles") String roles) { | |
| List<String> roleList = Arrays.asList(roles.split(",")); | |
| if (roleList.contains("CLUB_ADMIN")) { | |
| matchAuthorizationService.validateClubAdmin(matchId, requestingUserId); | |
| } | |
| queueService.banUser(matchId, userId); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@src/main/java/org/ticketing/queue/presentation/controller/QueueController.java`
around lines 138 - 142, The banUser endpoint is validating the wrong user: add a
RequestHeader parameter for the requesting user's ID (annotate a UUID parameter
with `@RequestHeader`("X-User-Id") requestingUserId) in the
QueueController.banUser method and change the call to
matchAuthorizationService.validateClubAdmin(...) to pass requestingUserId
instead of the banned userId so the club-admin check verifies the requester, not
the target.
| @PreAuthorize("hasAnyRole('ADMIN','CLUB_ADMIN')") | ||
| @PostMapping("/{matchId}/{userId}/banned") | ||
| public void banUser(@PathVariable("matchId") UUID matchId, @PathVariable UUID userId) { | ||
| public void banUser(@PathVariable("matchId") UUID matchId, @PathVariable UUID userId, @RequestHeader("X-User-Roles") String roles) { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check if there's a security filter or gateway configuration that validates/sets X-User-Roles
# Search for security configurations that might validate or set role headers
rg -n -C3 'X-User-Roles' --type=java
# Search for gateway or filter configurations
fd -e yml -e yaml -e properties | xargs rg -l 'gateway|filter|security'Repository: 3s-ticketing/queue-service
Length of output: 1085
🏁 Script executed:
# Search for any filter or interceptor that might handle X-User-Roles header
rg -i 'filter|interceptor|X-User-Roles' --type=java -A 5 -B 5
# Look for Spring Security configuration
fd -e java | xargs rg -l 'SecurityConfig|WebSecurityConfig|SecurityFilterChain'
# Check for application properties/yml files
find . -type f \( -name "application*.yml" -o -name "application*.yaml" -o -name "application*.properties" \)Repository: 3s-ticketing/queue-service
Length of output: 5575
🏁 Script executed:
# Search for any header manipulation or validation logic
rg -i 'header|@RequestHeader' --type=java | head -20
# Look for any proxy or gateway configuration
fd -type f \( -name "*gateway*" -o -name "*proxy*" \) | head -20Repository: 3s-ticketing/queue-service
Length of output: 3543
🏁 Script executed:
# Check the broader context of the QueueController class
cat -n src/main/java/org/ticketing/queue/presentation/controller/QueueController.java | head -50Repository: 3s-ticketing/queue-service
Length of output: 2256
🏁 Script executed:
# Read the full QueueSecurityConfig
cat src/main/java/org/ticketing/queue/infrastructure/config/QueueSecurityConfig.javaRepository: 3s-ticketing/queue-service
Length of output: 1330
🏁 Script executed:
# Search for any custom filters or interceptors
fd -name '*Filter*.java' -o -name '*Interceptor*.java' | xargs cat 2>/dev/null | head -100Repository: 3s-ticketing/queue-service
Length of output: 301
🏁 Script executed:
# Check the full banUser method to see the complete implementation
sed -n '135,145p' src/main/java/org/ticketing/queue/presentation/controller/QueueController.javaRepository: 3s-ticketing/queue-service
Length of output: 566
🏁 Script executed:
# Search for any header stripping or validation logic
rg -i 'header|removeHeader|strip' src/main/java/org/ticketing/queue/infrastructure/config/ -A 2Repository: 3s-ticketing/queue-service
Length of output: 52
Critical: Remove reliance on X-User-Roles header or implement proper header validation.
The security configuration explicitly permits all requests (.anyRequest().permitAll()), rendering @PreAuthorize ineffective. Additionally, the banUser method reads the X-User-Roles header directly from the request without any validation of its source. Since no upstream infrastructure validates or sets this header, external clients can arbitrarily spoof roles to bypass authorization checks.
The temporary MVP security configuration must be replaced with a production-ready configuration that:
- Enforces authentication and authorization at the framework level (not just method annotations)
- Implements header stripping to reject externally-supplied role headers
- Derives user roles from authenticated identity (JWT, OAuth, etc.) rather than from request headers
Alternatively, remove the header parameter entirely and obtain roles from the authenticated principal if @PreAuthorize authorization is sufficient for your use case.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@src/main/java/org/ticketing/queue/presentation/controller/QueueController.java`
at line 138, The banUser method currently reads an untrusted X-User-Roles
header; remove that header parameter from the banUser(`@PathVariable`("matchId")
UUID matchId, `@PathVariable` UUID userId, `@RequestHeader`("X-User-Roles") String
roles) signature and instead obtain the caller's roles from the authenticated
principal via the Spring Security context (e.g.
SecurityContextHolder/Authentication or a `@AuthenticationPrincipal` parameter)
and use those authorities for authorization/@PreAuthorize checks; also update
your security configuration (SecurityConfig) to enforce
authentication/authorization at the framework level and to strip/reject
externally supplied role headers so requests cannot spoof roles.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/main/resources/application-docker.yml (1)
18-18: ⚡ Quick winRedis timeout of 10 seconds may cause extended thread blocking.
The
timeout: 10000mssetting means operations can block for up to 10 seconds. Combined with the high thread count (1000) and limited connection pools, this could significantly amplify latency spikes and thread starvation under load.Consider reducing to 3-5 seconds to fail faster and free threads for other work.
♻️ Proposed adjustment
- timeout: 10000ms + timeout: 5000ms🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/resources/application-docker.yml` at line 18, The Redis client timeout currently set as `timeout: 10000ms` in application-docker.yml is too long and can cause thread blocking under load; change the `timeout` value to a shorter duration (recommend 3000ms–5000ms) to fail faster and free threads, then redeploy and monitor for connection/timeouts alongside your thread pool settings (e.g., the 1000 thread config) to ensure the reduced timeout does not surface new errors.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/main/resources/application-docker.yml`:
- Line 18: The Redis client timeout currently set as `timeout: 10000ms` in
application-docker.yml is too long and can cause thread blocking under load;
change the `timeout` value to a shorter duration (recommend 3000ms–5000ms) to
fail faster and free threads, then redeploy and monitor for connection/timeouts
alongside your thread pool settings (e.g., the 1000 thread config) to ensure the
reduced timeout does not surface new errors.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c1824273-9ced-477d-b2c0-ba1f6914c34f
📒 Files selected for processing (1)
src/main/resources/application-docker.yml
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/main/java/org/ticketing/queue/infrastructure/persistence/SseEmitterRepository.java (1)
22-22:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake reverse-index updates atomic across
save()andremove().Line 22 performs
.add(userId)outside acompute*block. Undersave()/remove()concurrency,remove()can drop thematchIdentry before that add runs, leaving active emitters unreachable viafindUserIdsByMatchId().Suggested fix
public void save(UUID matchId, UUID userId, SseEmitter emitter) { emitters.put(buildKey(matchId, userId), emitter); - matchUserIndex.computeIfAbsent(matchId, k -> ConcurrentHashMap.newKeySet()).add(userId); + matchUserIndex.compute(matchId, (id, userIds) -> { + Set<UUID> ids = (userIds == null) ? ConcurrentHashMap.newKeySet() : userIds; + ids.add(userId); + return ids; + }); }#!/bin/bash # Verify current non-atomic update shape in save/remove. rg -n -C3 'computeIfAbsent\(matchId,.*newKeySet\(\)\)\.add\(userId\)' --type=java rg -n -C5 'computeIfPresent\(matchId, \(id, userIds\) ->' --type=javaAlso applies to: 31-34
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/org/ticketing/queue/infrastructure/persistence/SseEmitterRepository.java` at line 22, The reverse-index update for matchUserIndex must be made atomic: in SseEmitterRepository change save() so instead of computeIfAbsent(...).add(userId) call compute(matchId, (k, set) -> { if (set==null) set = ConcurrentHashMap.newKeySet(); set.add(userId); return set; }) and change remove() to use computeIfPresent/compute(matchId, (k, set) -> { if (set!=null) { set.remove(userId); return set.isEmpty() ? null : set; } return null; }) so both add and remove mutate the key’s Set inside the map’s atomic compute callback and avoid the race where remove drops the entry before add runs; reference matchUserIndex, save(), and remove() in SseEmitterRepository.
🧹 Nitpick comments (1)
src/main/java/org/ticketing/queue/infrastructure/persistence/SseEmitterRepository.java (1)
45-46: ⚡ Quick winReturn a defensive snapshot from
getAllMatchIds().Line 46 returns a live
keySet()view. Callers can mutate repository state through methods likeremove()andclear(), which could desynchronize index behavior fromemitters. The codebase already uses this pattern elsewhere (e.g.,List.copyOf()on line 42), so adopt it here for consistency.Suggested fix
public Set<UUID> getAllMatchIds() { - return matchUserIndex.keySet(); + return Set.copyOf(matchUserIndex.keySet()); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/org/ticketing/queue/infrastructure/persistence/SseEmitterRepository.java` around lines 45 - 46, getAllMatchIds currently returns the live view matchUserIndex.keySet() which lets callers mutate repository state; change getAllMatchIds to return a defensive snapshot (an immutable or new Set copy) of matchUserIndex.keySet() so callers cannot modify the repository via the returned set, matching the existing List.copyOf() pattern used elsewhere.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In
`@src/main/java/org/ticketing/queue/infrastructure/persistence/SseEmitterRepository.java`:
- Line 22: The reverse-index update for matchUserIndex must be made atomic: in
SseEmitterRepository change save() so instead of
computeIfAbsent(...).add(userId) call compute(matchId, (k, set) -> { if
(set==null) set = ConcurrentHashMap.newKeySet(); set.add(userId); return set; })
and change remove() to use computeIfPresent/compute(matchId, (k, set) -> { if
(set!=null) { set.remove(userId); return set.isEmpty() ? null : set; } return
null; }) so both add and remove mutate the key’s Set inside the map’s atomic
compute callback and avoid the race where remove drops the entry before add
runs; reference matchUserIndex, save(), and remove() in SseEmitterRepository.
---
Nitpick comments:
In
`@src/main/java/org/ticketing/queue/infrastructure/persistence/SseEmitterRepository.java`:
- Around line 45-46: getAllMatchIds currently returns the live view
matchUserIndex.keySet() which lets callers mutate repository state; change
getAllMatchIds to return a defensive snapshot (an immutable or new Set copy) of
matchUserIndex.keySet() so callers cannot modify the repository via the returned
set, matching the existing List.copyOf() pattern used elsewhere.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1fc9b293-a20f-4c0e-be3d-3e0c0dade13d
📒 Files selected for processing (4)
src/main/java/org/ticketing/queue/application/service/QueueService.javasrc/main/java/org/ticketing/queue/infrastructure/persistence/SseEmitterRepository.javasrc/main/java/org/ticketing/queue/presentation/controller/QueueController.javasrc/main/resources/application-docker.yml
🚧 Files skipped from review as they are similar to previous changes (3)
- src/main/java/org/ticketing/queue/presentation/controller/QueueController.java
- src/main/resources/application-docker.yml
- src/main/java/org/ticketing/queue/application/service/QueueService.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
📎 관련 이슈
📌 작업 내용
✨ 변경 사항
📝 리뷰 포인트 (선택)
🧠 기타 참고 사항
(참고 문서, 스크린샷 등)
Summary by CodeRabbit
New Features
Bug Fixes
Chores
Tests