[volume-8] Redis 기반 주문 대기열 구현 - #355
Merged
Merged
Conversation
📝 WalkthroughWalkthroughRedis 기반 대기열 진입 토큰 관문 기능이 추가됐다. WaitingQueue/EntryTokenStore/QueuePosition 도메인 계약, QueueService 핵심 로직, RedisWaitingQueue/RedisEntryTokenStore 구현체, QueueAdmissionScheduler, QueueFacade/QueueV1Controller API, 설정(QueueProperties, application.yml)이 추가됐다. OrderFacade.placeOrder는 entryToken 검증/소비 흐름을 포함하도록 시그니처가 변경됐고, OrderV1Controller는 X-Entry-Token 헤더를 전달한다. 관련 통합/E2E 테스트가 확장됐다. Changes대기열 입장 토큰 관문
Sequence Diagram(s)sequenceDiagram
participant Client
participant OrderV1Controller
participant OrderFacade
participant QueueService
participant EntryTokenStore
Client->>OrderV1Controller: POST /orders (X-Entry-Token)
OrderV1Controller->>OrderFacade: placeOrder(loginId, entryToken, command)
OrderFacade->>QueueService: validateToken(loginId, entryToken)
QueueService->>EntryTokenStore: find(loginId)
EntryTokenStore-->>QueueService: 저장된 토큰
alt 토큰 불일치/없음
QueueService-->>OrderFacade: FORBIDDEN 예외
OrderFacade-->>Client: 403 응답
else 토큰 유효
OrderFacade->>OrderFacade: 주문 생성(재고/쿠폰 차감)
OrderFacade->>QueueService: consumeToken(loginId)
QueueService->>EntryTokenStore: delete(loginId)
OrderFacade-->>Client: 주문 결과
end
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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: 3
🧹 Nitpick comments (7)
apps/commerce-api/src/test/java/com/loopers/domain/queue/QueuePositionTest.java (1)
8-44: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
throughputPerSecond경계값(0, 음수) 테스트가 누락되어 있다.경계값/실패 케이스가 부족하다.
throughputPerSecond = 0인 경우Long.MAX_VALUE가 반환되는 비정상 동작과, 음수 입력 시 음수 대기 시간이 계산되는 케이스가 커버되지 않는다.추가 테스트 권장:
`@DisplayName`("throughputPerSecond가 0 이하면 예외가 발생한다.") `@Test` void waiting_throwsWhenThroughputNonPositive() { assertThatThrownBy(() -> QueuePosition.waiting(1L, 0L)) .isInstanceOf(IllegalArgumentException.class); assertThatThrownBy(() -> QueuePosition.waiting(1L, -1L)) .isInstanceOf(IllegalArgumentException.class); }As per coding guidelines,
**/*Test*.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 `@apps/commerce-api/src/test/java/com/loopers/domain/queue/QueuePositionTest.java` around lines 8 - 44, QueuePositionTest is missing boundary/failure coverage for QueuePosition.waiting when throughputPerSecond is non-positive. Add tests around the existing waiting() behavior to assert that passing 0 or a negative throughput throws IllegalArgumentException, using the QueuePosition.waiting and estimatedWaitSeconds paths as the target for the validation. Keep the existing happy-path tests, and extend the test class with explicit exception assertions for these invalid inputs.Source: Path instructions
apps/commerce-api/src/main/java/com/loopers/support/error/ErrorType.java (1)
14-14: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
FORBIDDEN기본 메시지가 대기열 기능에 특화되어 있다.
"입장 권한이 없습니다."는 대기열 토큰 관문에 특화된 메시지이나,ErrorType은 범용 HTTP 에러 카테고리 enum이다. 다른 기능에서FORBIDDEN을 커스텀 메시지 없이 사용하면 대기열과 무관한 맥락에서도 "입장 권한" 메시지가 노출된다.운영 관점: 향후 권한 제어 기능 추가 시 사용자 혼란 발생 가능.
수정안:
- FORBIDDEN(HttpStatus.FORBIDDEN, HttpStatus.FORBIDDEN.getReasonPhrase(), "입장 권한이 없습니다."), + FORBIDDEN(HttpStatus.FORBIDDEN, HttpStatus.FORBIDDEN.getReasonPhrase(), "접근 권한이 없습니다."),
QueueService.validateToken()에서 이미 커스텀 메시지를 제공하므로 기본 메시지는 범용으로 유지하는 것이 적절하다.🤖 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 `@apps/commerce-api/src/main/java/com/loopers/support/error/ErrorType.java` at line 14, The FORBIDDEN default message in ErrorType is too queue-specific and should be made generic for reuse across features. Update the FORBIDDEN enum entry in ErrorType to use a neutral default message, and keep the queue-specific wording only in QueueService.validateToken where the custom message is already supplied. Ensure other callers of ErrorType.FORBIDDEN without custom text no longer expose the queue-related phrase.apps/commerce-api/src/test/java/com/loopers/interfaces/api/OrderV1ApiE2ETest.java (2)
217-230: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win실패 케이스에서 토큰 잔존 검증 누락
OrderFacade.placeOrderJavadoc에 "실패하면 토큰은 남아 TTL 내 재시도할 수 있다"라고 명시됐으나, 실패 케이스(404, 409, 400) 테스트에서 토큰이 소비되지 않고 남아있는지 검증하지 않는다. 이 계약이 깨지면 사용자가 실패 후 재시도할 수 없게 되어 운영 장애로 이어질 수 있다.수정안: 각 실패 케이스의
then블록에 토큰 잔존 검증을 추가한다.💚 제안: 404 케이스 예시
// then -assertThat(mvcResult.getResponse().getStatus()).isEqualTo(HttpStatus.NOT_FOUND.value()); +assertAll( + () -> assertThat(mvcResult.getResponse().getStatus()).isEqualTo(HttpStatus.NOT_FOUND.value()), + () -> assertThat(entryTokenStore.find(LOGIN_ID)).isPresent() +);추가 테스트: 상기 검증 자체가 실패 시 토큰 잔존 계약에 대한 추가 테스트에 해당한다.
As per coding guidelines,
**/*Test*.java파일은 경계값/실패 케이스/예외 흐름을 포함하는지 점검한다. 토큰 잔존 여부는 실패 시 예외 흐름의 핵심 검증점이다.Also applies to: 241-257, 356-366, 377-390
🤖 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 `@apps/commerce-api/src/test/java/com/loopers/interfaces/api/OrderV1ApiE2ETest.java` around lines 217 - 230, The failure-path order tests in OrderV1ApiE2ETest are missing the contract check that entry tokens remain usable after a failed OrderFacade.placeOrder call. In each 404, 409, and 400 case, extend the then/assertion block to verify the token was not consumed and still exists so retry is possible, using the same entry-token storage/check helper already used around issueEntryToken and the relevant failure-response assertions.Source: Path instructions
305-330: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win토큰 소진 후 재사용 시도 검증 누락
deletesToken_afterSuccessfulOrder는 토큰 삭제를 검증하지만, 삭제된 토큰으로 두 번째 주문 시도 시 403이 반환되는지는 검증하지 않는다. 이 검증이 있어야 토큰 단일 사용 보장이 end-to-end로 완전히 입증된다.수정안: 동일 토큰으로 두 번째 주문 요청 후 403 응답 검증을 추가한다.
💚 제안: 재사용 검증 추가
// then — 주문 성공 후 토큰이 소비되어 사라진다 assertAll( () -> assertThat(mvcResult.getResponse().getStatus()).isEqualTo(HttpStatus.OK.value()), () -> assertThat(entryTokenStore.find(LOGIN_ID)).isEmpty() ); + +// when — 동일 토큰으로 재시도하면 403이 반환된다 +MvcResult retryResult = mockMvc.perform(post(ENDPOINT) + .header(AuthenticatedUserArgumentResolver.HEADER_LOGIN_ID, LOGIN_ID) + .header(AuthenticatedUserArgumentResolver.HEADER_LOGIN_PW, PASSWORD) + .header(HEADER_ENTRY_TOKEN, entryToken) + .contentType(MediaType.APPLICATION_JSON) + .content(objectMapper.writeValueAsString(request))) + .andReturn(); + +// then +assertThat(retryResult.getResponse().getStatus()).isEqualTo(HttpStatus.FORBIDDEN.value());추가 테스트: 상기 검증 자체가 토큰 단일 사용 보장에 대한 추가 테스트에 해당한다.
🤖 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 `@apps/commerce-api/src/test/java/com/loopers/interfaces/api/OrderV1ApiE2ETest.java` around lines 305 - 330, `deletesToken_afterSuccessfulOrder` only checks that the entry token is removed after a successful order, but it does not verify single-use behavior end-to-end. Update this test in `OrderV1ApiE2ETest` to perform a second `mockMvc.perform(post(ENDPOINT)... )` using the same `entryToken` after the first success, and assert that the response status is 403. Keep the existing assertions that the first request succeeds and `entryTokenStore.find(LOGIN_ID)` becomes empty, and use the same request setup symbols (`issueEntryToken`, `HEADER_ENTRY_TOKEN`, `ENDPOINT`) to clearly exercise token reuse.apps/commerce-api/src/main/java/com/loopers/infrastructure/queue/QueueAdmissionScheduler.java (1)
23-26: 🩺 Stability & Availability | 🔵 Trivial다중 인스턴스 환경에서 스케줄러가 중복 실행되어 입장 처리량이 의도치 않게 증가한다.
@Scheduled는 인스턴스마다 독립적으로 실행된다.popMin이 원자적이어서 중복 발급은 없지만, 인스턴스 수만큼 입장 처리량이 증가한다. 3개 인스턴스 기준 540/s로 의도(175/s)를 3배 초과하여 다운스트림 주문 처리에 과부하가 발생할 수 있다.수정안: ShedLock 등 분산 락으로 단일 인스턴스만 스케줄러를 실행하도록 제어하라.
추가 테스트: 다중 스케줄러 동시 실행 시 처리량이 의도 범위 내로 제한되는지 검증하는 테스트가 필요하다.
다중 인스턴스 배포 여부를 확인하라.
🤖 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 `@apps/commerce-api/src/main/java/com/loopers/infrastructure/queue/QueueAdmissionScheduler.java` around lines 23 - 26, The QueueAdmissionScheduler.admit scheduled job is running independently on every app instance, which multiplies admission throughput in multi-instance deployments. Update the admit flow to use a distributed lock such as ShedLock so only one instance executes the scheduler at a time, and wire the lock around QueueAdmissionScheduler.admit while keeping queueService.admitNext unchanged. Add a test around the scheduler behavior to verify concurrent executions across multiple scheduler instances stay within the intended batch rate.apps/commerce-api/src/test/java/com/loopers/domain/queue/EntryTokenStoreIntegrationTest.java (1)
54-67: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Thread.sleep기반 TTL 만료 검증은 CI 환경에서 flaky 할 수 있다.고정 대기(1.2s)는 CI 부하 시 Redis 지연으로 인해 TTL이 만료되지 않았을 때 fail이 발생할 수 있다. 운영 관점에서 flaky 테스트는 CI 신뢰성을 저하시킨다.
수정안: Awaitility로 폴링하여 안정적으로 만료를 검증하라.
추가 테스트: TTL 경계값(만료 직전/직후) 상태를 검증하는 케이스를 추가하라.
♻️ Awaitility 적용 제안
import org.junit.jupiter.api.Test; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.boot.test.context.SpringBootTest; import org.springframework.test.context.TestPropertySource; +import java.time.Duration; import java.util.Optional; +import static org.awaitility.Awaitility.await; import static org.assertj.core.api.Assertions.assertThat;// act — TTL 초과까지 대기 - Thread.sleep(1_200L); + await().atMost(Duration.ofSeconds(3)) + .pollInterval(Duration.ofMillis(200)) + .until(() -> entryTokenStore.find("alice").isEmpty());🤖 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 `@apps/commerce-api/src/test/java/com/loopers/domain/queue/EntryTokenStoreIntegrationTest.java` around lines 54 - 67, The TTL expiration test in EntryTokenStoreIntegrationTest is flaky because token_expiresAfterTtl uses Thread.sleep with a fixed delay; replace that wait in token_expiresAfterTtl with Awaitility-based polling against entryTokenStore.find so the test waits until the token is actually expired. Also add a boundary-focused test around issue/find behavior for EntryTokenStore to cover just-before-expiry and just-after-expiry states, using the existing entryTokenStore methods as the main identifiers to locate the code.apps/commerce-api/src/main/java/com/loopers/infrastructure/queue/RedisWaitingQueue.java (1)
28-31: 🎯 Functional Correctness | 🔵 Trivial다중 인스턴스 환경에서
System.currentTimeMillis()score는 클럭 스큐로 대기 순서 공정성이 깨질 수 있다.인스턴스 간 시간 차이가 발생하면 나중에 진입한 유저가 더 빠른 score를 받아 먼저 입장할 수 있다. NTP 동기화를 하더라도 ms 단위 스큐는 피하기 어려우며, 초당 175명 처리 기준에서 순번 뒤바뀜은 사용자 불만으로 이어진다.
수정안: Redis
TIME명령이나 INCR 카운터를 score로 사용하면 단일 시계 기반으로 공정성을 보장할 수 있다.추가 테스트: 다중 인스턴스 시나리오에서 동시 진입 시 순서 보장을 검증하는 통합 테스트가 필요하다.
다중 인스턴스 배포 여부를 확인하라.
🤖 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 `@apps/commerce-api/src/main/java/com/loopers/infrastructure/queue/RedisWaitingQueue.java` around lines 28 - 31, `RedisWaitingQueue.enter` uses `System.currentTimeMillis()` as the ZSET score, which can break queue fairness across multiple instances due to clock skew. Update the score source to a single Redis-backed time/sequence, such as Redis `TIME` or an `INCR`-based counter, so ordering is consistent regardless of app instance. Also add or update an integration test around `RedisWaitingQueue` to verify concurrent `enter` calls from multiple instances preserve the intended order, and confirm this deployment is multi-instance before applying the change.
🤖 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
`@apps/commerce-api/src/main/java/com/loopers/application/order/OrderFacade.java`:
- Around line 31-39: The token consumption step in OrderFacade.placeOrder is not
atomic with order creation, so a failed queueService.consumeToken after
orderService.createPendingOrder can leave a reusable token and allow duplicate
orders. Update the flow so the entry token is used as an idempotency guard for
createPendingOrder, or at minimum make consumeToken failures non-fatal by
logging them and still returning a normal response; use the existing
OrderFacade.placeOrder, queueService.consumeToken, and
orderService.createPendingOrder call sites to keep the retry behavior explicit.
Also add tests that cover consumeToken failure and repeated use of the same
token.
In `@apps/commerce-api/src/main/java/com/loopers/domain/queue/QueuePosition.java`:
- Around line 15-16: QueuePosition.waiting currently computes estimated wait
time without validating throughputPerSecond, so zero or negative values can
produce invalid results. Add an explicit guard in QueuePosition.waiting before
the division to reject throughputPerSecond <= 0 with a clear exception, and keep
the normal ceil-based calculation only for positive values. Update or add tests
around QueuePosition.waiting to verify that 0 and negative throughputPerSecond
inputs fail fast with an exception.
In `@apps/commerce-api/src/main/java/com/loopers/domain/queue/QueueService.java`:
- Around line 20-24: QueueService.enter has a TOCTOU race because it calls
waitingQueue.enter() and then getPosition() as two separate Redis operations,
allowing admitNext() to remove the user in between and cause a false NOT_FOUND.
Fix this by making the queue insertion/position retrieval atomic: change
WaitingQueue.enter() to return the assigned rank/position directly, or otherwise
fetch position in the same Redis operation rather than calling getPosition()
afterward. Update QueueService.enter() to rely on that single atomic result, and
add a test that simulates admitNext() interleaving during enter().
---
Nitpick comments:
In
`@apps/commerce-api/src/main/java/com/loopers/infrastructure/queue/QueueAdmissionScheduler.java`:
- Around line 23-26: The QueueAdmissionScheduler.admit scheduled job is running
independently on every app instance, which multiplies admission throughput in
multi-instance deployments. Update the admit flow to use a distributed lock such
as ShedLock so only one instance executes the scheduler at a time, and wire the
lock around QueueAdmissionScheduler.admit while keeping queueService.admitNext
unchanged. Add a test around the scheduler behavior to verify concurrent
executions across multiple scheduler instances stay within the intended batch
rate.
In
`@apps/commerce-api/src/main/java/com/loopers/infrastructure/queue/RedisWaitingQueue.java`:
- Around line 28-31: `RedisWaitingQueue.enter` uses `System.currentTimeMillis()`
as the ZSET score, which can break queue fairness across multiple instances due
to clock skew. Update the score source to a single Redis-backed time/sequence,
such as Redis `TIME` or an `INCR`-based counter, so ordering is consistent
regardless of app instance. Also add or update an integration test around
`RedisWaitingQueue` to verify concurrent `enter` calls from multiple instances
preserve the intended order, and confirm this deployment is multi-instance
before applying the change.
In `@apps/commerce-api/src/main/java/com/loopers/support/error/ErrorType.java`:
- Line 14: The FORBIDDEN default message in ErrorType is too queue-specific and
should be made generic for reuse across features. Update the FORBIDDEN enum
entry in ErrorType to use a neutral default message, and keep the queue-specific
wording only in QueueService.validateToken where the custom message is already
supplied. Ensure other callers of ErrorType.FORBIDDEN without custom text no
longer expose the queue-related phrase.
In
`@apps/commerce-api/src/test/java/com/loopers/domain/queue/EntryTokenStoreIntegrationTest.java`:
- Around line 54-67: The TTL expiration test in EntryTokenStoreIntegrationTest
is flaky because token_expiresAfterTtl uses Thread.sleep with a fixed delay;
replace that wait in token_expiresAfterTtl with Awaitility-based polling against
entryTokenStore.find so the test waits until the token is actually expired. Also
add a boundary-focused test around issue/find behavior for EntryTokenStore to
cover just-before-expiry and just-after-expiry states, using the existing
entryTokenStore methods as the main identifiers to locate the code.
In
`@apps/commerce-api/src/test/java/com/loopers/domain/queue/QueuePositionTest.java`:
- Around line 8-44: QueuePositionTest is missing boundary/failure coverage for
QueuePosition.waiting when throughputPerSecond is non-positive. Add tests around
the existing waiting() behavior to assert that passing 0 or a negative
throughput throws IllegalArgumentException, using the QueuePosition.waiting and
estimatedWaitSeconds paths as the target for the validation. Keep the existing
happy-path tests, and extend the test class with explicit exception assertions
for these invalid inputs.
In
`@apps/commerce-api/src/test/java/com/loopers/interfaces/api/OrderV1ApiE2ETest.java`:
- Around line 217-230: The failure-path order tests in OrderV1ApiE2ETest are
missing the contract check that entry tokens remain usable after a failed
OrderFacade.placeOrder call. In each 404, 409, and 400 case, extend the
then/assertion block to verify the token was not consumed and still exists so
retry is possible, using the same entry-token storage/check helper already used
around issueEntryToken and the relevant failure-response assertions.
- Around line 305-330: `deletesToken_afterSuccessfulOrder` only checks that the
entry token is removed after a successful order, but it does not verify
single-use behavior end-to-end. Update this test in `OrderV1ApiE2ETest` to
perform a second `mockMvc.perform(post(ENDPOINT)... )` using the same
`entryToken` after the first success, and assert that the response status is
403. Keep the existing assertions that the first request succeeds and
`entryTokenStore.find(LOGIN_ID)` becomes empty, and use the same request setup
symbols (`issueEntryToken`, `HEADER_ENTRY_TOKEN`, `ENDPOINT`) to clearly
exercise token reuse.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1c273138-1768-44c9-a515-bc04b4a6efb9
📒 Files selected for processing (21)
apps/commerce-api/src/main/java/com/loopers/application/order/OrderFacade.javaapps/commerce-api/src/main/java/com/loopers/application/queue/QueueFacade.javaapps/commerce-api/src/main/java/com/loopers/application/queue/QueuePositionInfo.javaapps/commerce-api/src/main/java/com/loopers/domain/queue/EntryTokenStore.javaapps/commerce-api/src/main/java/com/loopers/domain/queue/QueuePosition.javaapps/commerce-api/src/main/java/com/loopers/domain/queue/QueueService.javaapps/commerce-api/src/main/java/com/loopers/domain/queue/WaitingQueue.javaapps/commerce-api/src/main/java/com/loopers/infrastructure/queue/QueueAdmissionScheduler.javaapps/commerce-api/src/main/java/com/loopers/infrastructure/queue/RedisEntryTokenStore.javaapps/commerce-api/src/main/java/com/loopers/infrastructure/queue/RedisWaitingQueue.javaapps/commerce-api/src/main/java/com/loopers/interfaces/api/order/OrderV1Controller.javaapps/commerce-api/src/main/java/com/loopers/interfaces/api/queue/QueueV1Controller.javaapps/commerce-api/src/main/java/com/loopers/interfaces/api/queue/QueueV1Dto.javaapps/commerce-api/src/main/java/com/loopers/support/config/QueueProperties.javaapps/commerce-api/src/main/java/com/loopers/support/error/ErrorType.javaapps/commerce-api/src/main/resources/application.ymlapps/commerce-api/src/test/java/com/loopers/domain/queue/EntryTokenStoreIntegrationTest.javaapps/commerce-api/src/test/java/com/loopers/domain/queue/QueueAdmissionIntegrationTest.javaapps/commerce-api/src/test/java/com/loopers/domain/queue/QueuePositionTest.javaapps/commerce-api/src/test/java/com/loopers/domain/queue/QueueServiceIntegrationTest.javaapps/commerce-api/src/test/java/com/loopers/interfaces/api/OrderV1ApiE2ETest.java
Comment on lines
+31
to
+39
| public OrderInfo placeOrder(String loginId, String entryToken, OrderCommand command) { | ||
| queueService.validateToken(loginId, entryToken); | ||
|
|
||
| UserModel user = userService.getMyInfo(loginId); | ||
| List<OrderLine> lines = normalize(command.items()); | ||
|
|
||
| OrderModel pendingOrderModel = orderService.createPendingOrder(user.getId(), lines, command.couponId()); | ||
|
|
||
| queueService.consumeToken(loginId); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
토큰 소비 실패 시 중복 주문 발생 가능 (원자성 부재)
createPendingOrder 커밋 후 consumeToken(Redis DELETE)이 실패하면(예: Redis 일시적 장애), 주문은 생성됐지만 토큰이 남아 TTL 만료 전까지 재사용 가능하다. 사용자가 동일 토큰으로 재시도하면 validateToken을 통과하여 중복 주문이 발생하고, 재고가 이중 차감되어 데이터 정합성이 훼손된다. 이는 비멱등 쓰기에 대한 안전장치 없는 재시도에 해당한다.
수정안:
- (권장) 주문 생성 시 엔트리 토큰을 멱등성 키로 저장하여, 동일 토큰으로 두 번째 주문 생성을 차단한다.
- (최소 완화)
consumeToken실패 시 예외를 전파하지 않고 로그만 남겨 사용자에게 정상 응답을 반환한다. 토큰 TTL을 짧게 설정하여 중복 주문 창을 최소화한다.
🛡️ 최소 완화안: consumeToken 실패 시 로깅 only
public OrderInfo placeOrder(String loginId, String entryToken, OrderCommand command) {
queueService.validateToken(loginId, entryToken);
UserModel user = userService.getMyInfo(loginId);
List<OrderLine> lines = normalize(command.items());
OrderModel pendingOrderModel = orderService.createPendingOrder(user.getId(), lines, command.couponId());
- queueService.consumeToken(loginId);
+ try {
+ queueService.consumeToken(loginId);
+ } catch (Exception e) {
+ log.warn("토큰 소비 실패 [loginId={}]: {}", loginId, e.getMessage());
+ }
return OrderInfo.from(pendingOrderModel);
}추가 테스트:
consumeToken실패 시나리오 통합 테스트 (Redis 장애 상황 모킹)- 동일 토큰으로 두 번째 주문 시도 시 403 또는 기존 주문 반환 검증
As per coding guidelines, **/*.java 파일은 예외 흐름이 명확한지 점검한다. 현재 consumeToken 실패 시 주문은 생성됐으나 사용자에게 에러가 반환되는 예외 흐름이 불명확하다.
📝 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 OrderInfo placeOrder(String loginId, String entryToken, OrderCommand command) { | |
| queueService.validateToken(loginId, entryToken); | |
| UserModel user = userService.getMyInfo(loginId); | |
| List<OrderLine> lines = normalize(command.items()); | |
| OrderModel pendingOrderModel = orderService.createPendingOrder(user.getId(), lines, command.couponId()); | |
| queueService.consumeToken(loginId); | |
| public OrderInfo placeOrder(String loginId, String entryToken, OrderCommand command) { | |
| queueService.validateToken(loginId, entryToken); | |
| UserModel user = userService.getMyInfo(loginId); | |
| List<OrderLine> lines = normalize(command.items()); | |
| OrderModel pendingOrderModel = orderService.createPendingOrder(user.getId(), lines, command.couponId()); | |
| try { | |
| queueService.consumeToken(loginId); | |
| } catch (Exception e) { | |
| log.warn("토큰 소비 실패 [loginId={}]: {}", loginId, e.getMessage()); | |
| } |
🤖 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
`@apps/commerce-api/src/main/java/com/loopers/application/order/OrderFacade.java`
around lines 31 - 39, The token consumption step in OrderFacade.placeOrder is
not atomic with order creation, so a failed queueService.consumeToken after
orderService.createPendingOrder can leave a reusable token and allow duplicate
orders. Update the flow so the entry token is used as an idempotency guard for
createPendingOrder, or at minimum make consumeToken failures non-fatal by
logging them and still returning a normal response; use the existing
OrderFacade.placeOrder, queueService.consumeToken, and
orderService.createPendingOrder call sites to keep the retry behavior explicit.
Also add tests that cover consumeToken failure and repeated use of the same
token.
Source: Path instructions
Comment on lines
+15
to
+16
| public static QueuePosition waiting(long position, long throughputPerSecond) { | ||
| long estimatedWaitSeconds = (long) Math.ceil((double) position / throughputPerSecond); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
throughputPerSecond 0 또는 음수에 대한 검증이 필요하다.
throughputPerSecond가 0이면 (double) position / 0 = Infinity → (long) Math.ceil(Infinity) = Long.MAX_VALUE가 반환되어 예상 대기 시간으로 비정상 값이 노출된다. 음수면 음수 대기 시간이 계산된다. QueueProperties 설정 오류 시 즉시 사용자 응답에 잘못된 값이 전파된다.
운영 관점: 설정 변경 한 번으로 전체 대기열 응답이 비정상 동작하며, 원인 파악이 어렵다.
수정안:
public static QueuePosition waiting(long position, long throughputPerSecond) {
+ if (throughputPerSecond <= 0) {
+ throw new IllegalArgumentException("throughputPerSecond must be positive");
+ }
long estimatedWaitSeconds = (long) Math.ceil((double) position / throughputPerSecond);
return new QueuePosition(position, false, null, estimatedWaitSeconds);
}추가 테스트: throughputPerSecond = 0 및 음수 입력 시 예외 발생 검증.
📝 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 static QueuePosition waiting(long position, long throughputPerSecond) { | |
| long estimatedWaitSeconds = (long) Math.ceil((double) position / throughputPerSecond); | |
| public static QueuePosition waiting(long position, long throughputPerSecond) { | |
| if (throughputPerSecond <= 0) { | |
| throw new IllegalArgumentException("throughputPerSecond must be positive"); | |
| } | |
| long estimatedWaitSeconds = (long) Math.ceil((double) position / throughputPerSecond); |
🤖 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 `@apps/commerce-api/src/main/java/com/loopers/domain/queue/QueuePosition.java`
around lines 15 - 16, QueuePosition.waiting currently computes estimated wait
time without validating throughputPerSecond, so zero or negative values can
produce invalid results. Add an explicit guard in QueuePosition.waiting before
the division to reject throughputPerSecond <= 0 with a clear exception, and keep
the normal ceil-based calculation only for positive values. Update or add tests
around QueuePosition.waiting to verify that 0 and negative throughputPerSecond
inputs fail fast with an exception.
Comment on lines
+20
to
+24
| public long enter(String loginId) { | ||
| waitingQueue.enter(loginId); | ||
|
|
||
| return getPosition(loginId); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
enter()에서 waitingQueue.enter()와 getPosition() 사이에 TOCTOU 경쟁 조건이 존재한다.
enter()는 두 개의 분리된 Redis 호출을 순차 실행한다. 사이에 스케줄러의 admitNext()가 개입하면:
waitingQueue.enter(loginId)— ZSET에 사용자 추가- 스케줄러
admitNext(n)— ZPOPMIN으로 해당 사용자 제거 + 토큰 발급 getPosition(loginId)— ZRANK가 nil →NOT_FOUND예외 발생
사용자는 방금 진입했는데 404를 받게 되며, 에러 메시지("대기열에 진입한 이력이 없습니다")도 실제 상황과 불일치한다. 재시도 시 ZADD NX가 다시 성공하여 사용자가 대기열에 재진입하면서 토큰도 보유한 상태 inconsistency가 발생한다.
운영 관점: 고트래픽 환경에서 스케줄러 주기와 겹치면 간헐적 404 발생 → 사용자 불만 및 상태 불일치 가능.
수정안 — enter()에서 rank 조회 후 비어 있으면 토큰 존재 여부 확인:
public long enter(String loginId) {
waitingQueue.enter(loginId);
- return getPosition(loginId);
+ return waitingQueue.rank(loginId)
+ .map(rank -> rank + 1)
+ .orElseGet(() -> {
+ // admitNext 경쟁으로 이미 토큰이 발급된 경우
+ if (entryTokenStore.find(loginId).isPresent()) {
+ return 0L;
+ }
+ throw new CoreException(ErrorType.NOT_FOUND,
+ "[loginId = " + loginId + "] 대기열에 진입한 이력이 없습니다.");
+ });
}근본 해결은 WaitingQueue.enter()가 순번을 반환하도록 인터페이스를 변경하여 단일 Redis 명령으로 원자적 처리하는 것이다.
추가 테스트: enter() 호출 중 admitNext()가 개입하는 경쟁 시나리오 검증.
📝 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 long enter(String loginId) { | |
| waitingQueue.enter(loginId); | |
| return getPosition(loginId); | |
| } | |
| public long enter(String loginId) { | |
| waitingQueue.enter(loginId); | |
| return waitingQueue.rank(loginId) | |
| .map(rank -> rank + 1) | |
| .orElseGet(() -> { | |
| // admitNext 경쟁으로 이미 토큰이 발급된 경우 | |
| if (entryTokenStore.find(loginId).isPresent()) { | |
| return 0L; | |
| } | |
| throw new CoreException(ErrorType.NOT_FOUND, | |
| "[loginId = " + loginId + "] 대기열에 진입한 이력이 없습니다."); | |
| }); | |
| } |
🤖 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 `@apps/commerce-api/src/main/java/com/loopers/domain/queue/QueueService.java`
around lines 20 - 24, QueueService.enter has a TOCTOU race because it calls
waitingQueue.enter() and then getPosition() as two separate Redis operations,
allowing admitNext() to remove the user in between and cause a false NOT_FOUND.
Fix this by making the queue insertion/position retrieval atomic: change
WaitingQueue.enter() to return the assigned rank/position directly, or otherwise
fetch position in the same Redis operation rather than calling getPosition()
afterward. Update QueueService.enter() to rely on that single atomic result, and
add a test that simulates admitNext() interleaving during enter().
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
1 participant
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.
🧭 Context & Decision
1. 문제 정의
POST /api/v1/orders)는 진입 제한 없이 요청을 그대로 받는다. 주문 1건은 DB 커넥션을 약 200ms 점유하고 커넥션 풀은 50개다. 평상시 초당 100건은 문제없지만, 선착순 쿠폰 발급 순간에는 초당 수천~수만 건이 몰리면 커넥션·스레드 풀이 한계를 넘어 주문뿐 아니라 전체 API가 함께 느려지거나 멈춘다.2. 선택지와 결정
[결정 1] 재진입 시 순번 정책 — 최초 순번 보존
ZADD NX/addIfAbsent).[결정 2] 스케줄러 배치 크기·주기 — 100ms마다 18명
처리량 상한= DB pool 50 × (커넥션당 초당 5건, 주문 200ms 기준) = 250 TPS, 여기에 안전마진 70%를 적용해 175 TPS. 이 값이 back-pressure의 상한 수치다.→ Thundering Herd, 순간 스파이크.
fixedDelay드리프트 위험.@ConfigurationProperties로 외부화해 재배포 없이 조정 가능하게 했다.🤔 고민한 점 / 막혔던 부분
예상 대기를 순번/처리량으로 단순 정수 나눗셈하면 순번이 처리량보다 작은 대기자가 0초로 표시된다. 아직 줄 서 있는 유저에게 0초라고 안내하면 오해를 주므로, 올림 처리로 최소 1초를 보장했다. 이 공식은 Redis 없이 도는 순수 단위 테스트로 분리해 빠르게 고정했다.