Skip to content

[Feature] Redis 기반 주문 저장소 및 주문 취소 구현#5

Open
seminchoi wants to merge 20 commits intomainfrom
feature/issue-3-1
Open

[Feature] Redis 기반 주문 저장소 및 주문 취소 구현#5
seminchoi wants to merge 20 commits intomainfrom
feature/issue-3-1

Conversation

@seminchoi
Copy link
Contributor

@seminchoi seminchoi commented Feb 16, 2026

구현 내용

1. Redis 기반 OrderRepository

  • ZSET + HASH 이중 구조로 주문 저장소 구현
    • ZSET: 주문 우선순위 정렬 (score = scaledPrice, member = {createdAt epoch millis}|{orderId})
    • HASH: 주문 상세 정보 저장 (orderId 기준)

2. 주문 취소 API

  • DELETE /internal/orders/{orderId} 엔드포인트 추가
  • OrderRepository.remove(orderId) 호출 → 실패 시 ORDER_CANCEL_FAILED 예외 반환

Key Points

1. 주문 체결과 취소의 동시성 고려 (CAS)

  • 문제: 체결(Worker 스레드)과 취소(API 스레드)가 동시에 같은 주문을 삭제할 수 있음
  • Hash DEL을 CAS로 활용: Hash 삭제 성공 여부를 기반으로 체결/주문 취소 결정
    • Hash DEL 성공 → ZSET 삭제 진행 → 취소/체결 확정
    • Hash DEL 실패 → 이미 상대방이 처리함 → 중복 처리 방지
  • 별도 분산 락 없이 Redis의 단일 명령 원자성만으로 해결

2. 가격 Scale 정책

  • Redis ZSET score는 double로 저장되므로 주문가격이 소수점일 때, 정렬 순서 오차 발생 가능
  • price * 10,000 (소수 4자리 정밀도)으로 정수 변환 후 score로 사용
  • 주가 자리수 가정 (추가 논의 필요):
    • 주가의 정수부는 최대 10자리 이하, 소수부는 4자리 이하로 가정 (수백만 원대 주식까지 커버)
    • double은 정수를 약 15자리 까지 오차 없이 표현

3. 주문 RANGE 조회 채택

  • 기존 peek/poll 방식: 최우선 주문 1건씩 꺼내서 매칭 시도
  • 문제 — 시간 제약에 의한 선두 주문 막힘
    호가 발생 시간이 T, 최우선 호가가 900원 이라고 가정할 때,
    ZSET (매수):
      Order A: price=1000, createdAt=T+1  ← 아직 체결 불가 (주문 시점이 호가보다 미래)
      Order B: price=900,  createdAt=T-1  ← 체결 가능하지만 도달 불가
    
    • 공정성을 위해 order.createdAt <= orderbook.timestamp인 주문만 체결 허용
    • peek은 항상 선두(Order A)만 반환 → A가 시간 제약에 걸리면 뒤의 체결 가능한 주문(B)까지 전부 차단
    • poll로 넘어가는 방식은 체결되지 않은 주문을 메모리에 보관 후 재삽입해야 하므로 관리 오버헤드 발생, 서버 재시작 시 휘발 위험
  • 해결 — ZRANGEBYSCORE 일괄 조회:
    • 가격 범위 내 전체 주문을 한 번에 조회
    • 시간 제약에 걸린 주문은 SKIP하고 다음 주문으로 계속 매칭

@seminchoi seminchoi self-assigned this Feb 16, 2026
@seminchoi seminchoi changed the title Redis 기반 주문 저장소 및 주문 취소 구현 [Feature] Redis 기반 주문 저장소 및 주문 취소 구현 Feb 16, 2026
@seminchoi seminchoi changed the base branch from feature/issue-3 to main February 17, 2026 04:04
@jjh5887
Copy link

jjh5887 commented Feb 19, 2026

/gemini review

@jjh5887
Copy link

jjh5887 commented Feb 19, 2026

/gemini review

안대네요....

Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This PR significantly enhances system scalability and stability by migrating the order repository from an in-memory queue to a Redis-based solution, leveraging ZSET and HASH for order management. It also improves reactive processing via ReactiveSymbolMatchingWorker and refines StockSessionManager's sequential message handling. However, several security flaws were identified in the new API endpoints. Specifically, the lack of ownership checks and server-side ID generation could enable unauthorized order modification and cancellation. Additionally, missing input validation on order prices might lead to logic errors in the matching engine. Beyond security, a critical race condition exists in the order cancellation logic, potentially causing data inconsistency. Blocking code in the service layer also risks undermining the reactive architecture's benefits. Further improvements are suggested for overall stability and code cleanliness. Overall, this is an excellent PR that makes significant advancements to the matching engine, and addressing these points will result in a more robust implementation.

Comment on lines +27 to +29
public ResponseEntity<Void> createOrder(@Valid @RequestBody OrderRequest request) {
orderService.addOrder(request.toOrder());
return ResponseEntity.ok().build();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

security-critical critical

The createOrder endpoint accepts a user-provided id in the OrderRequest. This id is used directly as the Redis key for storing order details in OrderHashClient.save. An attacker who can reach this endpoint can overwrite any existing order by providing its id, leading to unauthorized data modification and potential market disruption. It is highly recommended to generate unique order IDs server-side or verify that the provided ID does not already exist and belongs to the requester.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

그러네요 이거 id값 까지 request에 있으면 안돼지 않나영?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Match API는 내부 요청만 받는다는 가정이라 어뷰징은 배제한다는 생각이었습니다..!
공격 가능성을 고려해야할까요?!

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Match API는 내부 요청만 받는다는 가정이라 어뷰징은 배제한다는 생각이었습니다..! 공격 가능성을 고려해야할까요?!

요게 공격이라는 좀 강한 표현이긴 한데 match engine을 사용하는 쪽에서 실수로 요청이 잘못 들어오거나 하는 경우도 있을 수 있어서 최소한의 방어로직으로는 있어야 할것 같아요!

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

정확히는 불필요한 데이터 까지 굳이 받을 수 있게? 한다는 느낌이 드네여

Comment on lines +68 to +79
@Override
public Mono<Boolean> remove(Long orderId) {
return hashClient.findById(orderId)
.flatMap(order -> hashClient.delete(orderId)
.filter(deleted -> deleted)
.flatMap(deleted -> {
String member = toMember(order);
return zsetClient.remove(sideKey(order.side()), order.symbol(), member);
})
.hasElement())
.defaultIfEmpty(false);
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

remove 메소드에 심각한 경쟁 조건(race condition)이 존재합니다. 이 메소드는 findById 호출과 delete 호출이 원자적으로 실행되지 않아, 부분 체결(updateQuantity)과 주문 취소/완전 체결(remove)이 동시에 발생할 때 데이터 불일치를 유발할 수 있습니다.

시나리오 예시:

  1. 부분 체결 스레드: 주문 A에 대해 부분 체결을 계산합니다.
  2. 취소 스레드: remove(A)를 호출하여 findById로 주문 A의 현재 상태(예: 수량 10)를 읽습니다.
  3. 부분 체결 스레드: updateQuantity(A, 5)를 호출하여 Redis HASH의 수량을 5로 변경합니다.
  4. 취소 스레드: delete(A)를 호출하여 HASH를 삭제합니다. 이로 인해 수량이 5로 변경된 업데이트가 유실됩니다.

결과적으로 주문은 취소 처리되지만, 이미 처리된 부분 체결 내역이 반영되지 않아 시스템 상태가 불일치하게 됩니다.

해결 제안:
이 문제를 해결하려면 remove 연산을 원자적으로 만들어야 합니다. Redis Lua 스크립트를 사용하여 조건부 삭제를 구현하는 것이 표준적인 방법입니다. 스크립트는 HASH와 ZSET에서의 삭제를 하나의 원자적 단위로 묶어 처리해야 합니다.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

주문 취소 이전에 부분 체결이 완료되면, 해당 주문은 부분체결에 대한 웹훅 이벤트는 전송하고 남은 수량에 대해 취소하는 시나리오를 생각했습니다.
취소 건에 대해 취소 수량을 Match API 서버가 책임지고 반환해야 한다면, Lua 스크립트를 작성하는 것이 맞을 것 같아요.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

역시 제미니 행님
redis 원자성 해결은 lua로 많이들 하니 이참에 한번 적용해 보시져!

Comment on lines +36 to +38
public ResponseEntity<Void> cancelOrder(@PathVariable Long orderId) {
orderService.cancelOrder(orderId);
return ResponseEntity.ok().build();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

security-high high

The cancelOrder endpoint allows cancelling an order using only the orderId provided in the path. There is no verification to ensure that the authenticated user or the calling service has the authority to cancel that specific order. This is a classic Insecure Direct Object Reference (IDOR) vulnerability. An attacker could potentially cancel any active order in the system by iterating through order IDs.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

요 내용도 고려해 주셔야 할듯 하네요
물론 앞단 gateway에서 기본적인 인증은 해주겠지만... 인증단 다른 회원이 주문을 취소 하려는 경우도 있을 수 있기 때문에 userId 까지 같이 받아서 검증을 한번 해주셔야 할 것 같아요.
(memberId는 header에 넘기고 있음)

Comment on lines +26 to +27
orderRepository.addOrder(order)
.block();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

addOrder 메소드에서 .block()을 사용하여 리액티브 스트림을 블로킹하고 있습니다. 리액티브 스택(Project Reactor, Netty)을 사용하는 애플리케이션에서 요청 처리 스레드를 블로킹하는 것은 성능 저하의 주요 원인이 될 수 있으며 리액티브 프로그래밍의 이점을 상쇄시키는 안티패턴입니다.

해결 제안:
OrderControllerOrderService를 포함한 전체 요청 처리 과정을 논블로킹(non-blocking) 방식으로 변경하는 것을 강력히 권장합니다. 이를 위해 OrderService의 메소드들이 Mono<Void>를 반환하고, OrderControllerMono<ResponseEntity<Void>>를 반환하도록 수정해야 합니다.

        orderRepository.addOrder(order);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

고민했던 포인트인데, 프로젝트 전체적으로 Webflux를 적용하는게 좋을까요?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@seminchoi
프로젝트 자체가 webflux라면 (맞죠..?) 동기 라이브러리를 섞어 쓰기 보단 reactive 라이브러리를 쓰는게 좋을것 같네요.
하고 webflux 환경에선 block 자체가 어떻게 보면 안티패턴이라 가능한한 안하는게 좋습니다!

Controller 단까지 Mono나 Flux를 그대로 반환하는 방식을 권장합니다!
자세한 내용은 찾아보기 -> 왜 webflux에서 block을 하지 않는가

Comment on lines +31 to +33
boolean removed = orderRepository.remove(orderId)
.blockOptional()
.orElse(false);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

cancelOrder 메소드에서 .blockOptional()을 사용하여 리액티브 스트림을 블로킹하고 있습니다. 이는 addOrder 메소드의 .block() 호출과 마찬가지로 리액티브 애플리케이션의 성능을 저하시킬 수 있는 안티패턴입니다.

해결 제안:
OrderController까지 이어지는 전체 호출 스택을 리액티브하게 변경하여 블로킹 호출을 제거하는 것이 좋습니다. cancelOrder 메소드는 Mono<Void>를 반환하고, 실패 시 Mono.error()를 통해 예외를 전달하도록 수정할 수 있습니다.

// In OrderService
public Mono<Void> cancelOrder(Long orderId) {
    return orderRepository.remove(orderId)
        .filter(removed -> removed)
        .switchIfEmpty(Mono.error(new ApiException(MatchErrorCode.ORDER_CANCEL_FAILED, Map.of("orderId", orderId))))
        .then();
}

// In OrderController
@DeleteMapping("/{orderId}")
public Mono<ResponseEntity<Void>> cancelOrder(@PathVariable Long orderId) {
    return orderService.cancelOrder(orderId)
        .then(Mono.just(ResponseEntity.ok().build()));
}
        orderRepository.remove(orderId)
                .filter(removed -> removed)
                .switchIfEmpty(Mono.error(new ApiException(MatchErrorCode.ORDER_CANCEL_FAILED,
                        Map.of("orderId", orderId))))
                .block();

@NotNull TradeSide side,
@NotNull OrderType type,
@NotNull @Positive BigDecimal quantity,
BigDecimal price, // LIMIT 주문일 때만 필수 (Order 생성자에서 검증)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

security-medium medium

The price field in OrderRequest lacks validation to ensure it is a positive value. While the Order constructor checks for nullity in LIMIT orders, it does not enforce a positive price. A negative price would result in incorrect score calculation in RedisOrderRepository.toScore, potentially placing the order at an incorrect priority level and causing unintended matches in the engine.

Suggested change
BigDecimal price, // LIMIT 주문일 때만 필수 (Order 생성자에서 검증)
@Positive BigDecimal price, // LIMIT 주문일 때만 필수 (Order 생성자에서 검증)

Comment on lines +1 to +5
package com.stocat.match.api.engine.event;

public sealed interface MatchingEvent permits OrderAddedEvent, OrderbookEvent {
String symbol();
} No newline at end of file

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

MatchingEvent sealed interface와 이를 구현하는 OrderAddedEvent, OrderbookEvent 클래스가 새로 추가되었지만, 현재 코드베이스에서 사용되는 곳이 없는 것으로 보입니다. 사용되지 않는 코드는 혼란을 유발하고 유지보수 비용을 증가시킬 수 있으므로, 의도된 기능이 아니라면 제거하는 것이 좋습니다.

.retryWhen(Retry.backoff(3, Duration.ofMillis(100))
.maxBackoff(Duration.ofSeconds(2)))
.doOnError(e -> log.error("체결 전송 최종 실패: fill={}, error={}", fill, e.getMessage()))
.subscribe();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

subscribe()를 인수 없이 호출하는 것은 "fire-and-forget" 방식이지만, 스트림에서 처리되지 않은 오류가 발생할 경우 전역 에러 핸들러로 전파될 수 있어 잠재적인 위험이 있습니다. doOnError가 대부분의 오류를 로깅하지만, 구독 시점에 발생하는 예외는 잡지 못할 수 있습니다.
해결 제안:
안정성을 높이기 위해 subscribe 호출 시 에러 핸들러 람다를 명시적으로 제공하는 것이 좋습니다.

Suggested change
.subscribe();
.subscribe(null, e -> log.error("체결 전송 구독 중 에러 발생: fill={}, error={}", fill, e.getMessage()));

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants