[fix] 예약/결제 동시성 취약점 수정 (레이스 컨디션 및 오버부킹)#158
Conversation
- BookingScheduler: jakarta → Spring @transactional 교체, 취소 전 PENDING 상태 재확인 추가 - Booking: @Version 추가 (낙관적 락으로 동시 상태 변경 감지) - BookingTable: booking_date/time 컬럼 추가 및 (store_table_id, booking_date, booking_time) 유니크 제약 추가 - BookingRepository: findByIdWithLock() 메서드 추가 (PESSIMISTIC_WRITE) - BookingCommandServiceImpl: confirmPayment/cancelBooking/cancelBookingByOwner에 비관적 락 적용 - PaymentService: booking.confirm() 시 락 재조회 및 CONFIRMED 중복 방지 처리 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
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:
📝 WalkthroughWalkthrough낙관적 락( 동시성 제어 및 오버부킹 방지
Sequence DiagramsequenceDiagram
participant Scheduler
participant CancelExecutor
participant Repository
participant Database
Scheduler->>CancelExecutor: findExpiredPendingIds(threshold)
CancelExecutor-->>Scheduler: [id...]
loop per id
Scheduler->>CancelExecutor: cancelIfPending(id)
CancelExecutor->>Repository: findByIdWithLock(id)
Repository->>Database: SELECT ... FOR UPDATE
Database-->>Repository: locked Booking
CancelExecutor->>CancelExecutor: if PENDING -> booking.cancel(...)
CancelExecutor->>Database: commit cancel
end
Estimated code review effort 🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces concurrency control and data integrity measures for the booking system, including optimistic locking via @Version, pessimistic locking for critical service methods, and a unique constraint to prevent overbooking. While these changes improve consistency, the review identifies a critical issue where the new unique constraint prevents re-booking slots after a cancellation because the mapping records are not removed. Additionally, the payment confirmation logic needs to be more restrictive regarding allowed status transitions, and the booking scheduler's batch transaction approach should be refined to prevent a single locking failure from rolling back all expired booking cancellations.
| uniqueConstraints = @UniqueConstraint( | ||
| name = "uq_booking_table_slot", | ||
| columnNames = {"store_table_id", "booking_date", "booking_time"} | ||
| ) |
There was a problem hiding this comment.
uq_booking_table_slot 유니크 제약 조건은 오버부킹을 방지하는 강력한 수단이지만, 현재 구현상 심각한 부작용이 있습니다. 예약이 취소(CANCELED)되어도 BookingTable 레코드가 삭제되지 않고 유지되기 때문에, 취소된 예약이 점유했던 동일한 시간대에 다른 사용자가 예약을 시도하면 DB 제약 조건 위반으로 인해 재예약이 불가능해집니다. 이를 해결하려면 예약 취소 시 연관된 BookingTable 데이터를 삭제(또는 Soft Delete 처리 후 유니크 인덱스에서 제외)하는 로직이 반드시 추가되어야 합니다.
| if (lockedBooking != null && lockedBooking.getStatus() != BookingStatus.CONFIRMED) { | ||
| lockedBooking.confirm(); | ||
| log.info("Booking confirmed for OrderID: {}", dto.orderId()); | ||
| } |
There was a problem hiding this comment.
lockedBooking.getStatus() != BookingStatus.CONFIRMED 조건은 너무 포괄적입니다. 만약 예약이 이미 COMPLETED(방문 완료) 상태인 경우에도 이 로직이 실행되면 다시 CONFIRMED로 상태가 변경되는 논리적 오류가 발생할 수 있습니다. 결제 확정 처리는 오직 PENDING 상태인 경우에만 수행되도록 조건을 좁히는 것이 안전합니다. 만약 스케줄러에 의해 취소된 예약을 복구하려는 의도가 포함되어 있다면 PENDING 또는 CANCELED 상태인 경우로 한정해야 합니다.
| if (lockedBooking != null && lockedBooking.getStatus() != BookingStatus.CONFIRMED) { | |
| lockedBooking.confirm(); | |
| log.info("Booking confirmed for OrderID: {}", dto.orderId()); | |
| } | |
| if (lockedBooking != null && lockedBooking.getStatus() == BookingStatus.PENDING) { | |
| lockedBooking.confirm(); | |
| log.info("Booking confirmed for OrderID: {}", dto.orderId()); | |
| } |
| if (booking.getStatus() == BookingStatus.PENDING) { | ||
| booking.cancel("결제 시간 초과로 인한 자동 취소"); | ||
| } |
There was a problem hiding this comment.
현재 스케줄러는 단일 @Transactional 내에서 여러 예약을 일괄 취소합니다. 특정 예약에 대해 결제 스레드와 낙관적 락(@Version) 충돌이 발생하여 OptimisticLockingFailureException이 발생하면, 해당 배치의 모든 취소 작업이 함께 롤백되는 문제가 있습니다. 만료 대상 예약이 많을 경우, 개별 예약 단위로 트랜잭션을 분리하거나 예외 처리를 추가하여 일부 실패가 전체 배치에 영향을 주지 않도록 개선하는 것을 권장합니다.
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/main/java/com/eatsfine/domain/booking/entity/Booking.java (1)
107-111:⚠️ Potential issue | 🔴 Critical | ⚡ Quick win
cancel()이bookingTables를 클리어하지 않아 취소 후 동일 슬롯 재예약 시 유니크 제약 위반
BookingTable에 추가된uq_booking_table_slot(store_table_id, booking_date, booking_time)제약은booking상태와 무관하게 모든 행에 적용됩니다.cancel()이bookingTables리스트를 비우지 않으면orphanRemoval = true가 있더라도BookingTable행은 DB에 잔류합니다. 결과적으로 취소된 슬롯에 신규 예약을 시도하면DataIntegrityViolationException이 발생합니다.반면
findReservedTableIds쿼리는status IN ('CONFIRMED', 'PENDING')으로 취소 예약을 제외하므로, 애플리케이션 레벨 중복 체크는 통과하고 DB 제약에서 실패합니다.🐛 제안 수정
public void cancel(String cancelReason) { + this.bookingTables.clear(); // orphanRemoval=true: BookingTable 행 삭제하여 유니크 제약 해제 this.status = BookingStatus.CANCELED; this.cancelReason = cancelReason; }🤖 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/com/eatsfine/domain/booking/entity/Booking.java` around lines 107 - 111, The cancel() method sets status and cancelReason but does not remove associated BookingTable entries, leaving DB rows that violate the unique constraint uq_booking_table_slot; update cancel() in Booking to clear or remove the bookingTables collection (e.g., bookingTables.clear() or remove each BookingTable) so orphanRemoval can delete DB rows when status becomes BookingStatus.CANCELED, and ensure behavior remains consistent with the findReservedTableIds query that filters by status IN ('CONFIRMED','PENDING').src/main/java/com/eatsfine/domain/booking/service/BookingCommandServiceImpl.java (1)
200-203:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift비관적 락 보유 상태에서 외부 Toss API 호출이 발생하여 락 경합 위험
Line 185의
findByIdWithLock으로booking행의FOR UPDATE락을 획득한 뒤, Line 202의paymentService.cancelPayment()가 동일 트랜잭션에 참여합니다(REQUIREDpropagation).cancelPayment내부의tossPaymentService.cancel()은 외부 HTTP 호출이므로, 네트워크 지연이나 Toss 서비스 응답 지연 시 해당booking행의 락이 수 초 이상 유지됩니다.동일한 패턴이
cancelBookingByOwner(Lines 246–251)에도 존재합니다.이로 인해:
- 동일 예약 행을 접근하려는 다른 트랜잭션이 블로킹됨 (커넥션 풀 고갈 위험)
- Toss 취소는 성공했으나 이후 DB 커밋이 실패하면 환불만 처리되고 예약 상태가 일관성을 잃을 수 있음
PR Objectives에서 "Move external Toss API calls out of transactional context"를 개선 과제로 명시하고 있습니다. 이번 릴리즈에서 해결 여부를 팀과 확인하는 것을 권장합니다. 수정 방향은 Toss API 호출을 트랜잭션 외부로 분리하고, 결과에 따라 DB 상태를 업데이트하는 구조로 변경하는 것입니다.
🤖 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/com/eatsfine/domain/booking/service/BookingCommandServiceImpl.java` around lines 200 - 203, The code holds a FOR UPDATE lock from findByIdWithLock while calling paymentService.cancelPayment (which delegates to tossPaymentService.cancel), causing long-held DB locks and inconsistency risk; refactor cancel flow so that external Toss HTTP calls occur outside the transaction: remove paymentService.cancelPayment from inside the transaction scope in methods using findByIdWithLock (e.g., the block around booking.getStatus() == BookingStatus.CONFIRMED and the cancelBookingByOwner flow), instead persist intent/state (e.g., mark booking as CANCELLATION_PENDING or record a compensation record) within the transaction, commit, then invoke tossPaymentService.cancel/paymentService.cancelPayment in a new non-transactional context (or async job) and finally update booking/payments in a short follow-up transaction based on the external call result to ensure locks are not held during network calls and DB consistency is maintained.
🤖 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/com/eatsfine/domain/booking/entity/mapping/BookingTable.java`:
- Around line 16-22: 테이블 매이그레이션과 예외처리를 분리해 해결하세요: BookingTable 엔티티의
booking_date/booking_time을 즉시 NOT NULL로 변경하지 말고 마이그레이션에서(1) 컬럼을 NULL 허용으로
추가하고(2) 기존 Booking 레코드의 값으로 backfill(참조: Booking → booking_date/booking_time
매핑)하며(3) 취소된 예약의 잔여 BookingTable 행은 삭제하도록 Booking.cancel() 로직을 보강해 유니크 충돌을 방지한
뒤(4) 컬럼을 NOT NULL로 변경하고(5) 유니크 제약(uq_booking_table_slot) 추가하세요; 또한
BookingCommandServiceImpl.createBooking 내 bookingRepository.flush() 호출로 발생하는
DataIntegrityViolationException을 글로벌 예외 핸들러에서 잡아 “이미 예약된 슬롯” 오류로 변환 처리하도록 예외 매핑을
추가하세요.
In `@src/main/java/com/eatsfine/domain/booking/repository/BookingRepository.java`:
- Around line 76-78: findByIdWithLock currently uses `@Lock`(PESSIMISTIC_WRITE)
without a lock timeout, which can cause indefinite blocking and exhaustion of DB
connections when callers like cancelBooking or cancelBookingByOwner perform long
external calls; add a pessimistic lock timeout hint to the repository method
(e.g., via `@QueryHints/`@QueryHint with "javax.persistence.lock.timeout" set to a
sensible millisecond value such as 5000) on the findByIdWithLock method so the
DB will fail the wait after the configured timeout instead of blocking forever.
In `@src/main/java/com/eatsfine/domain/booking/service/BookingScheduler.java`:
- Around line 44-48: The in-memory recheck using booking.getStatus() ==
BookingStatus.PENDING after loading expiredBookings from
findAllByStatusAndCreatedAtBefore is ineffective and risks rolling back the
whole `@Transactional` batch on ObjectOptimisticLockingFailureException when
optimistic locking (`@Version`) detects concurrent CONFIRMED updates; change to
process each booking in its own REQUIRES_NEW transaction by reloading with a
pessimistic/optimistic lock (use a method like findByIdWithLock called from a
REQUIRES_NEW service or via self-injection) and then call booking.cancel(...)
there so a single booking’s optimistic lock failure does not roll back other
expirations.
---
Outside diff comments:
In `@src/main/java/com/eatsfine/domain/booking/entity/Booking.java`:
- Around line 107-111: The cancel() method sets status and cancelReason but does
not remove associated BookingTable entries, leaving DB rows that violate the
unique constraint uq_booking_table_slot; update cancel() in Booking to clear or
remove the bookingTables collection (e.g., bookingTables.clear() or remove each
BookingTable) so orphanRemoval can delete DB rows when status becomes
BookingStatus.CANCELED, and ensure behavior remains consistent with the
findReservedTableIds query that filters by status IN ('CONFIRMED','PENDING').
In
`@src/main/java/com/eatsfine/domain/booking/service/BookingCommandServiceImpl.java`:
- Around line 200-203: The code holds a FOR UPDATE lock from findByIdWithLock
while calling paymentService.cancelPayment (which delegates to
tossPaymentService.cancel), causing long-held DB locks and inconsistency risk;
refactor cancel flow so that external Toss HTTP calls occur outside the
transaction: remove paymentService.cancelPayment from inside the transaction
scope in methods using findByIdWithLock (e.g., the block around
booking.getStatus() == BookingStatus.CONFIRMED and the cancelBookingByOwner
flow), instead persist intent/state (e.g., mark booking as CANCELLATION_PENDING
or record a compensation record) within the transaction, commit, then invoke
tossPaymentService.cancel/paymentService.cancelPayment in a new
non-transactional context (or async job) and finally update booking/payments in
a short follow-up transaction based on the external call result to ensure locks
are not held during network calls and DB consistency is maintained.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 992754ff-3cb7-48f5-be69-60ee3d8b3cd1
📒 Files selected for processing (6)
src/main/java/com/eatsfine/domain/booking/entity/Booking.javasrc/main/java/com/eatsfine/domain/booking/entity/mapping/BookingTable.javasrc/main/java/com/eatsfine/domain/booking/repository/BookingRepository.javasrc/main/java/com/eatsfine/domain/booking/service/BookingCommandServiceImpl.javasrc/main/java/com/eatsfine/domain/booking/service/BookingScheduler.javasrc/main/java/com/eatsfine/domain/payment/service/PaymentService.java
| @Table( | ||
| name = "booking_table", | ||
| uniqueConstraints = @UniqueConstraint( | ||
| name = "uq_booking_table_slot", | ||
| columnNames = {"store_table_id", "booking_date", "booking_time"} | ||
| ) | ||
| ) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
기존 테이블에 NOT NULL 컬럼 추가 시 데이터 백필 마이그레이션 필요
booking_date/booking_time 컬럼은 nullable = false로 추가되지만, 기존 booking_table 행에는 해당 값이 없습니다. 마이그레이션 스크립트 없이 적용하면 NOT NULL 제약 위반으로 마이그레이션이 실패합니다.
권장 마이그레이션 순서:
- 컬럼을
NULLABLE로 추가 - 기존 행에 연관
booking의booking_date/booking_time으로 백필 - 취소된 예약의
BookingTable행을 삭제(유니크 제약 충돌 방지,Booking.cancel()수정과 함께) NOT NULL제약 추가- 유니크 제약 추가
또한 BookingCommandServiceImpl.createBooking의 bookingRepository.flush()(Line 130)에서 유니크 제약 위반 시 DataIntegrityViolationException이 발생하므로, 글로벌 예외 핸들러에서 이를 명시적으로 "이미 예약된 슬롯" 오류로 처리해야 합니다.
🤖 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/com/eatsfine/domain/booking/entity/mapping/BookingTable.java`
around lines 16 - 22, 테이블 매이그레이션과 예외처리를 분리해 해결하세요: BookingTable 엔티티의
booking_date/booking_time을 즉시 NOT NULL로 변경하지 말고 마이그레이션에서(1) 컬럼을 NULL 허용으로
추가하고(2) 기존 Booking 레코드의 값으로 backfill(참조: Booking → booking_date/booking_time
매핑)하며(3) 취소된 예약의 잔여 BookingTable 행은 삭제하도록 Booking.cancel() 로직을 보강해 유니크 충돌을 방지한
뒤(4) 컬럼을 NOT NULL로 변경하고(5) 유니크 제약(uq_booking_table_slot) 추가하세요; 또한
BookingCommandServiceImpl.createBooking 내 bookingRepository.flush() 호출로 발생하는
DataIntegrityViolationException을 글로벌 예외 핸들러에서 잡아 “이미 예약된 슬롯” 오류로 변환 처리하도록 예외 매핑을
추가하세요.
| @Lock(LockModeType.PESSIMISTIC_WRITE) | ||
| @Query("SELECT b FROM Booking b WHERE b.id = :id") | ||
| Optional<Booking> findByIdWithLock(@Param("id") Long id); |
There was a problem hiding this comment.
findByIdWithLock에 락 타임아웃이 설정되지 않아 무한 블로킹 위험
@Lock(PESSIMISTIC_WRITE)는 SELECT ... FOR UPDATE를 발행하지만, 타임아웃 힌트가 없으면 락을 보유한 트랜잭션이 느릴 때(예: cancelBooking/cancelBookingByOwner에서 Toss 외부 API 호출 중) 대기 트랜잭션이 무한정 블로킹되어 DB 커넥션 풀이 고갈될 수 있습니다.
🛡️ 제안 수정
+import org.springframework.data.jpa.repository.QueryHints;
+import jakarta.persistence.QueryHint;
...
`@Lock`(LockModeType.PESSIMISTIC_WRITE)
+@QueryHints(`@QueryHint`(name = "jakarta.persistence.lock.timeout", value = "3000"))
`@Query`("SELECT b FROM Booking b WHERE b.id = :id")
Optional<Booking> findByIdWithLock(`@Param`("id") Long id);🤖 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/com/eatsfine/domain/booking/repository/BookingRepository.java`
around lines 76 - 78, findByIdWithLock currently uses `@Lock`(PESSIMISTIC_WRITE)
without a lock timeout, which can cause indefinite blocking and exhaustion of DB
connections when callers like cancelBooking or cancelBookingByOwner perform long
external calls; add a pessimistic lock timeout hint to the repository method
(e.g., via `@QueryHints/`@QueryHint with "javax.persistence.lock.timeout" set to a
sensible millisecond value such as 5000) on the findByIdWithLock method so the
DB will fail the wait after the configured timeout instead of blocking forever.
취소된 예약이 점유하던 BookingTable 레코드를 cancel() 시 제거하여 동일 시간대 재예약이 가능하도록 수정. orphanRemoval=true 활용. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
CONFIRMED/COMPLETED/CANCELED 등 이미 확정된 예약에 다시 CONFIRMED를 덮어쓰는 논리 오류 방지. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
BookingCancelExecutor 추가 (REQUIRES_NEW) — 낙관적 락 충돌 발생 시 해당 예약만 실패하고 나머지 배치는 정상 처리됨. 스케줄러는 트랜잭션 없이 조율만 담당하고 실패는 warn 로그로 기록. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
예약 생성/결제 확인/취소 및 만료 스케줄러 처리에서 동시 요청으로 발생할 수 있는 레이스 컨디션(오버부킹, 상태 덮어쓰기 등)을 줄이기 위해 잠금(낙관/비관)과 DB 제약을 추가한 PR입니다.
Changes:
Booking에 낙관적 락(@Version) 추가 및 상태 변경 경로에서 비관적 락 조회 적용booking_table에 (store_table_id, booking_date, booking_time) 유니크 제약 및 슬롯 비정규화 컬럼 추가- 만료 PENDING 예약 정리를 스케줄러 → 실행기(개별 REQUIRES_NEW 트랜잭션) 구조로 분리
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/main/java/com/eatsfine/domain/payment/service/PaymentService.java | 결제 승인 시 Booking을 비관적 락으로 재조회 후 상태 변경 |
| src/main/java/com/eatsfine/domain/booking/service/BookingScheduler.java | 만료 예약 취소 로직을 executor로 위임하고 배치 처리 방식 변경 |
| src/main/java/com/eatsfine/domain/booking/service/BookingCommandServiceImpl.java | confirm/cancel 경로에서 Booking 비관적 락 조회로 변경 |
| src/main/java/com/eatsfine/domain/booking/service/BookingCancelExecutor.java | 만료 예약 ID 조회 및 REQUIRES_NEW 취소 실행 추가 |
| src/main/java/com/eatsfine/domain/booking/repository/BookingRepository.java | findByIdWithLock(PESSIMISTIC_WRITE) 추가 |
| src/main/java/com/eatsfine/domain/booking/entity/mapping/BookingTable.java | 슬롯 컬럼 추가 및 유니크 제약(uq_booking_table_slot) 설정 |
| src/main/java/com/eatsfine/domain/booking/entity/Booking.java | @Version 추가, BookingTable 생성 시 슬롯값 저장, cancel 시 bookingTables 정리 로직 추가 |
Comments suppressed due to low confidence (2)
src/main/java/com/eatsfine/domain/booking/service/BookingCommandServiceImpl.java:189
findByIdWithLockintroduces a DB write lock for the wholecancelBookingtransaction. This method also calls the external Toss cancel API inside the same transaction; holding a pessimistic lock across a network call can significantly increase lock wait time and risk deadlocks/throughput degradation. Consider restructuring so the external call happens outside the DB lock/transaction (e.g., mark cancel intent + commit, then call provider, then finalize in a separate transaction or via outbox/event).
Booking booking = bookingRepository.findByIdWithLock(bookingId)
.orElseThrow(() -> new BookingException(BookingErrorStatus._BOOKING_NOT_FOUND));
// 본인 예약인지 확인
src/main/java/com/eatsfine/domain/booking/service/BookingCommandServiceImpl.java:227
- Same concern as user cancel: this flow takes a
PESSIMISTIC_WRITElock (findByIdWithLock) and later calls an external refund API within the same transaction. This can hold row locks for the duration of the network call and amplify contention under load. Consider moving the external call out of the locked transaction (or using an async/outbox approach) while keeping the booking state transition consistent.
Booking booking = bookingRepository.findByIdWithLock(bookingId)
.orElseThrow(() -> new BookingException(BookingErrorStatus._BOOKING_NOT_FOUND));
// 2. 데이터 무결성 검증
if (!booking.getStore().getId().equals(storeId)) {
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // 유니크 제약 해제: 취소된 예약의 테이블 점유를 풀어 동일 시간대 재예약 허용 | ||
| // orphanRemoval = true이므로 리스트 비우면 DB에서 자동 삭제됨 | ||
| this.bookingTables.clear(); |
| @Table( | ||
| name = "booking_table", | ||
| uniqueConstraints = @UniqueConstraint( | ||
| name = "uq_booking_table_slot", | ||
| columnNames = {"store_table_id", "booking_date", "booking_time"} |
| @Column(name = "booking_date", nullable = false) | ||
| private LocalDate bookingDate; | ||
|
|
||
| @Column(name = "booking_time", nullable = false) |
| // 비관적 락으로 재조회하여 스케줄러 / 다른 스레드와의 동시 수정 방지 | ||
| Booking lockedBooking = bookingRepository.findByIdWithLock(booking.getId()) | ||
| .orElse(null); | ||
| if (lockedBooking != null && lockedBooking.getStatus() == BookingStatus.PENDING) { | ||
| lockedBooking.confirm(); |
| int successCount = 0; | ||
| for (Long id : expiredIds) { | ||
| try { | ||
| bookingCancelExecutor.cancelIfPending(id); | ||
| successCount++; | ||
| } catch (Exception e) { | ||
| log.warn("예약 ID {} 자동 취소 실패 — 다음 실행에서 재시도: {}", id, e.getMessage()); | ||
| } | ||
| } | ||
|
|
||
| log.info("스케줄러 완료: {}건 성공 / {}건 시도", successCount, expiredIds.size()); |
| @Transactional(readOnly = true) | ||
| public List<Long> findExpiredPendingIds(LocalDateTime threshold) { | ||
| return bookingRepository.findAllByStatusAndCreatedAtBefore(BookingStatus.PENDING, threshold) | ||
| .stream() | ||
| .map(booking -> booking.getId()) |
- BookingTable에 is_active(nullable Boolean) 컬럼 추가 - 유니크 제약을 (store_table_id, booking_date, booking_time, is_active)로 변경 - cancel() 시 bookingTables.clear() 제거 → deactivate()로 is_active=null 처리 - MySQL NULL 유니크 특성으로 취소 슬롯 재예약 허용, 테이블 이력 데이터 보존 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
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/com/eatsfine/domain/payment/service/PaymentService.java (1)
111-128:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift결제 완료(
completePayment)가 예약 락/상태 검증보다 먼저 수행되어 “결제 완료 + 예약 취소” 불일치 발생 가능현재 흐름은 ① Toss 승인 → ②
payment.completePayment(...)→ ③findByIdWithLock로 예약 잠금 → ④PENDING이면confirm()입니다. ③ 시점에 스케줄러(BookingCancelExecutor.cancelIfPending)나 다른 취소 경로가 먼저 락을 잡고 예약을CANCELED로 변경했다면, 결제는 이미COMPLETED로 완료되었지만 예약은 취소된 상태가 됩니다. 사용자는 결제만 되고 예약은 없는 상태가 되며 자동 보상도 없으므로 수동 환불이 필요해집니다.또한
lockedBooking이 없거나PENDING이 아닐 때 무로깅으로 무시되는데, 이는 운영 시 동시성 문제 추적을 어렵게 합니다. 최소한 경고 로그가 필요합니다.권장 보완 방향(택1):
- (선호) Toss 호출 이전에
findByIdWithLock로 예약을 잠그고PENDING검증 → 외부 결제 자체를 차단(fail-fast).- 차선: Toss 승인 후 잠금/검증에서
PENDING이 아니면 즉시tossPaymentService.cancel(...)로 보상 환불을 호출하고payment.failPayment()처리.🛡️ 제안 수정 — Toss 호출 전 예약 잠금/검증 + 미스매치 시 보상 처리 골격
- Booking booking = payment.getBooking(); - if (booking != null) { - // 비관적 락으로 재조회하여 스케줄러 / 다른 스레드와의 동시 수정 방지 - Booking lockedBooking = bookingRepository.findByIdWithLock(booking.getId()) - .orElse(null); - if (lockedBooking != null && lockedBooking.getStatus() == BookingStatus.PENDING) { - lockedBooking.confirm(); - log.info("Booking confirmed for OrderID: {}", dto.orderId()); - } - } + Booking booking = payment.getBooking(); + if (booking != null) { + Booking lockedBooking = bookingRepository.findByIdWithLock(booking.getId()) + .orElse(null); + if (lockedBooking == null || lockedBooking.getStatus() != BookingStatus.PENDING) { + // 예약이 이미 취소/확정된 상태라면 결제도 정합성 맞춰 환불 처리 + log.warn("Booking no longer PENDING (id={}, status={}). Compensating payment.", + booking.getId(), + lockedBooking != null ? lockedBooking.getStatus() : "NOT_FOUND"); + // TODO: tossPaymentService.cancel(...) 호출 및 payment.failPayment() 등 보상 트랜잭션 + throw new PaymentException(PaymentErrorStatus._BOOKING_NOT_FOUND); + } + lockedBooking.confirm(); + log.info("Booking confirmed for OrderID: {}", dto.orderId()); + }추가로, 가능하다면 위 락/검증 블록을
tossPaymentService.confirm(dto)호출 직전으로 옮겨 외부 호출 자체를 차단하는 것이 더 안전합니다.🤖 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/com/eatsfine/domain/payment/service/PaymentService.java` around lines 111 - 128, The current flow calls payment.completePayment(...) before locking and validating the booking (payment.getBooking → bookingRepository.findByIdWithLock(...)), which can yield a “payment completed + booking canceled” mismatch; to fix, prefer moving the findByIdWithLock + BookingStatus.PENDING check (lockedBooking and lockedBooking.confirm()) to run before invoking external Toss (i.e., before tossPaymentService.confirm(dto) / before calling payment.completePayment), failing fast if not PENDING; alternatively, if you must keep Toss call first, then after findByIdWithLock detect non-PENDING or null and immediately call tossPaymentService.cancel(...) and payment.failPayment() and emit a warning log (including dto.orderId and booking id) instead of silently ignoring, ensuring Booking.confirm() only runs when lockedBooking.getStatus()==BookingStatus.PENDING.
🤖 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/com/eatsfine/domain/booking/entity/mapping/BookingTable.java`:
- Around line 46-47: The BookingTable.isActive field is initialized to true but
lacks Lombok's `@Builder.Default`, so using BookingTable.builder() yields isActive
== null and breaks the unique slot constraint; add the `@Builder.Default`
annotation to the isActive field declaration in BookingTable (keeping the = true
initializer) so builders default to true, and verify call sites like
Booking.addBookingTable(StoreTable) rely on the builder-provided default; also
plan a DB migration to fix any existing is_active=NULL rows.
In
`@src/main/java/com/eatsfine/domain/booking/service/BookingCancelExecutor.java`:
- Around line 22-38: Extract the hard-coded cancel reason in cancelIfPending
("결제 시간 초과로 인한 자동 취소") into a constant (e.g., AUTO_CANCEL_REASON or
Booking.CANCEL_REASON_AUTO_TIMEOUT) and use that constant in Booking.cancel call
and any related logs; also change findExpiredPendingIds to avoid loading full
Booking entities by adding an ID-only repository query (e.g., add a method like
findIdsByStatusAndCreatedAtBefore in BookingRepository implemented with
`@Query`("select b.id from Booking b where b.status = ?1 and b.createdAt < ?2"))
and update findExpiredPendingIds to call that new repository method instead of
findAllByStatusAndCreatedAtBefore to reduce memory/ORM overhead.
---
Outside diff comments:
In `@src/main/java/com/eatsfine/domain/payment/service/PaymentService.java`:
- Around line 111-128: The current flow calls payment.completePayment(...)
before locking and validating the booking (payment.getBooking →
bookingRepository.findByIdWithLock(...)), which can yield a “payment completed +
booking canceled” mismatch; to fix, prefer moving the findByIdWithLock +
BookingStatus.PENDING check (lockedBooking and lockedBooking.confirm()) to run
before invoking external Toss (i.e., before tossPaymentService.confirm(dto) /
before calling payment.completePayment), failing fast if not PENDING;
alternatively, if you must keep Toss call first, then after findByIdWithLock
detect non-PENDING or null and immediately call tossPaymentService.cancel(...)
and payment.failPayment() and emit a warning log (including dto.orderId and
booking id) instead of silently ignoring, ensuring Booking.confirm() only runs
when lockedBooking.getStatus()==BookingStatus.PENDING.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 92a02b3a-afa4-4fca-9749-9a88d25d2121
📒 Files selected for processing (5)
src/main/java/com/eatsfine/domain/booking/entity/Booking.javasrc/main/java/com/eatsfine/domain/booking/entity/mapping/BookingTable.javasrc/main/java/com/eatsfine/domain/booking/service/BookingCancelExecutor.javasrc/main/java/com/eatsfine/domain/booking/service/BookingScheduler.javasrc/main/java/com/eatsfine/domain/payment/service/PaymentService.java
| @Column(name = "is_active") | ||
| private Boolean isActive = true; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP --type=java -C3 'BookingTable\s*\.\s*builder\s*\(\s*\)'Repository: Eatsfine/BE
Length of output: 751
🏁 Script executed:
cat -n src/main/java/com/eatsfine/domain/booking/entity/mapping/BookingTable.java | head -70Repository: Eatsfine/BE
Length of output: 1803
🏁 Script executed:
cat -n src/main/java/com/eatsfine/domain/booking/entity/Booking.java | sed -n '85,100p'Repository: Eatsfine/BE
Length of output: 629
🏁 Script executed:
rg -nP --type=java "uq_booking_table_slot|unique.*booking.*table" -iRepository: Eatsfine/BE
Length of output: 173
🏁 Script executed:
# Check for `@Builder` and `@AllArgsConstructor` annotations in BookingTable
rg -nP --type=java "@Builder|@AllArgsConstructor" src/main/java/com/eatsfine/domain/booking/entity/mapping/BookingTable.javaRepository: Eatsfine/BE
Length of output: 90
🏁 Script executed:
# Search for all BookingTable.builder() usages to see the full scope
rg -nP --type=java 'BookingTable\.builder\(\)'Repository: Eatsfine/BE
Length of output: 179
@Builder.Default 누락 — 신규 슬롯이 isActive=null로 생성되어 유니크 제약이 무력화됨
@Builder + @AllArgsConstructor 조합에서는 필드 초기값(= true)이 빌더에 반영되지 않습니다. @Builder.Default가 없으므로 BookingTable.builder()...build() 시 isActive는 true가 아닌 null로 채워집니다.
Booking.addBookingTable(StoreTable) (Booking.java Line 89~97)는 isActive를 명시적으로 설정하지 않으므로, 신규 예약 슬롯도 is_active=NULL로 저장됩니다. MySQL 유니크 인덱스는 NULL을 중복으로 취급하지 않기 때문에 uq_booking_table_slot 제약이 이중 예약을 전혀 차단하지 못합니다 — 이번 PR의 핵심 방어선이 무효화됩니다.
🛡️ 제안 수정 — `@Builder.Default` 추가
// true = 활성 예약 슬롯 (유니크 제약 적용)
// null = 취소된 슬롯 — MySQL은 NULL을 유니크 인덱스에서 중복으로 보지 않으므로 동일 시간대 재예약 허용
+ `@Builder.Default`
`@Column`(name = "is_active")
private Boolean isActive = true;또한 운영 DB에 이미 is_active=NULL로 잘못 적재된 활성 슬롯이 존재할 수 있으니 마이그레이션으로 보정 권장합니다.
🤖 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/com/eatsfine/domain/booking/entity/mapping/BookingTable.java`
around lines 46 - 47, The BookingTable.isActive field is initialized to true but
lacks Lombok's `@Builder.Default`, so using BookingTable.builder() yields isActive
== null and breaks the unique slot constraint; add the `@Builder.Default`
annotation to the isActive field declaration in BookingTable (keeping the = true
initializer) so builders default to true, and verify call sites like
Booking.addBookingTable(StoreTable) rely on the builder-provided default; also
plan a DB migration to fix any existing is_active=NULL rows.
| public List<Long> findExpiredPendingIds(LocalDateTime threshold) { | ||
| return bookingRepository.findAllByStatusAndCreatedAtBefore(BookingStatus.PENDING, threshold) | ||
| .stream() | ||
| .map(booking -> booking.getId()) | ||
| .toList(); | ||
| } | ||
|
|
||
| // REQUIRES_NEW: 호출마다 독립 트랜잭션 — 하나 실패해도 다른 예약에 영향 없음 | ||
| @Transactional(propagation = Propagation.REQUIRES_NEW) | ||
| public void cancelIfPending(Long bookingId) { | ||
| bookingRepository.findByIdWithLock(bookingId).ifPresent(booking -> { | ||
| if (booking.getStatus() == BookingStatus.PENDING) { | ||
| booking.cancel("결제 시간 초과로 인한 자동 취소"); | ||
| log.info("예약 ID {} 자동 취소 완료", bookingId); | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
락/REQUIRES_NEW 적용은 잘 되어 있으며, 매직 문자열 상수화 및 ID 전용 프로젝션 쿼리 권장
전체 구조(락 재조회 + 독립 트랜잭션 + PENDING 재검증)는 적절합니다. 두 가지 사소한 개선 의견입니다.
"결제 시간 초과로 인한 자동 취소"는 매직 문자열입니다. 향후 다른 자동 취소 사유가 추가되거나 다국어화 시 분기점이 되도록 상수로 추출하는 편이 안전합니다. (코드 품질 가이드: 매직 넘버/하드코딩 값 점검)findExpiredPendingIds가 만료 후보Booking엔티티 전체를 로드한 뒤 ID만 추출합니다. 만료 건수가 많아질 경우 메모리/연관 lazy 프록시 비용이 누적되므로, 리포지토리에@Query("select b.id from Booking b where b.status = ?1 and b.createdAt < ?2")형태의 ID 전용 쿼리를 추가하는 편이 효율적입니다. (성능 가이드: 불필요한 DB 쿼리/Stream 효율성)
♻️ 제안 수정 — 상수 추출 및 메소드 레퍼런스
+ private static final String CANCEL_REASON_PAYMENT_TIMEOUT = "결제 시간 초과로 인한 자동 취소";
+
`@Transactional`(readOnly = true)
public List<Long> findExpiredPendingIds(LocalDateTime threshold) {
return bookingRepository.findAllByStatusAndCreatedAtBefore(BookingStatus.PENDING, threshold)
.stream()
- .map(booking -> booking.getId())
+ .map(Booking::getId)
.toList();
}
// REQUIRES_NEW: 호출마다 독립 트랜잭션 — 하나 실패해도 다른 예약에 영향 없음
`@Transactional`(propagation = Propagation.REQUIRES_NEW)
public void cancelIfPending(Long bookingId) {
bookingRepository.findByIdWithLock(bookingId).ifPresent(booking -> {
if (booking.getStatus() == BookingStatus.PENDING) {
- booking.cancel("결제 시간 초과로 인한 자동 취소");
+ booking.cancel(CANCEL_REASON_PAYMENT_TIMEOUT);
log.info("예약 ID {} 자동 취소 완료", bookingId);
}
});
}위 코딩 가이드라인의 “3. 성능 및 효율성: 불필요한 DB 쿼리 호출…”과 “2. 코드 품질 & 가독성: 매직 넘버, 하드코딩된 값이 존재하는지 점검”에 따른 의견입니다.
🤖 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/com/eatsfine/domain/booking/service/BookingCancelExecutor.java`
around lines 22 - 38, Extract the hard-coded cancel reason in cancelIfPending
("결제 시간 초과로 인한 자동 취소") into a constant (e.g., AUTO_CANCEL_REASON or
Booking.CANCEL_REASON_AUTO_TIMEOUT) and use that constant in Booking.cancel call
and any related logs; also change findExpiredPendingIds to avoid loading full
Booking entities by adding an ID-only repository query (e.g., add a method like
findIdsByStatusAndCreatedAtBefore in BookingRepository implemented with
`@Query`("select b.id from Booking b where b.status = ?1 and b.createdAt < ?2"))
and update findExpiredPendingIds to call that new repository method instead of
findAllByStatusAndCreatedAtBefore to reduce memory/ORM overhead.
동시 예약 생성 시 uq_booking_table_slot 제약 위반으로 발생하는 DataIntegrityViolationException을 BookingException(_ALREADY_RESERVED_TABLE)로 변환. 500 대신 409 Conflict 응답을 반환함. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/main/java/com/eatsfine/domain/booking/service/BookingCommandServiceImpl.java (2)
187-271: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
cancelBooking과cancelBookingByOwner의 취소 흐름이 거의 동일합니다 — 공통 로직 추출을 검토해 주세요.권한 검증(본인 vs 사장님)과 사유 문자열을 제외한 핵심 흐름(락 조회 → 취소 가능 상태 확인 → CONFIRMED일 때 결제 취소 →
booking.cancel(reason))이 중복되어 있습니다. 향후 위 비관적 락+외부 API 분리 리팩터를 적용할 때 두 곳을 동시에 수정해야 하는 부담이 큽니다. 공통 로직을 private 메서드(예:executeCancellation(Booking, String))로 추출해 두면 일관성 유지와 향후 변경 비용을 모두 줄일 수 있습니다.As per coding guidelines, "코드 품질 & 가독성 - 중복 코드가 있는 경우, 유틸/공통 컴포넌트로 추출 가능한지" 및 "메소드/클래스가 단일 책임 원칙(SRP)에 맞게 구성되어 있는지".
🤖 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/com/eatsfine/domain/booking/service/BookingCommandServiceImpl.java` around lines 187 - 271, Both cancelBooking and cancelBookingByOwner share the same cancellation steps (locked booking retrieval already done, status checks, calling paymentService.cancelPayment when BookingStatus.CONFIRMED, invoking booking.cancel(reason) and building the response), so extract that core flow into a private helper like executeCancellation(Booking booking, String reason) that: verifies cancelable state (throws same BookingException._ALREADY_CANCELED), if CONFIRMED calls paymentService.cancelPayment(booking.getSuccessPaymentKey(), new PaymentRequestDTO.CancelPaymentDTO(reason)), calls booking.cancel(reason), and returns the common data (booking id, status, refund amount and optionally canceledAt if needed) so that cancelBooking keeps only user ownership check and cancelBookingByOwner keeps owner/table validation and both call executeCancellation to perform the shared work.
206-210:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift비관적 락(PESSIMISTIC_WRITE)을 보유한 채 Toss 외부 API를 호출하고 있습니다.
cancelBooking/cancelBookingByOwner는findByIdWithLock로 행 락을 획득한 트랜잭션 안에서paymentService.cancelPayment(...)(외부 결제 PG 호출)를 수행합니다. 외부 호출이 지연되거나 타임아웃이 발생하는 동안 DB 행 락이 길게 유지되어 다음 문제가 발생합니다.
- 동일 예약/연관 행에 대한 후속 트랜잭션의 락 대기·타임아웃 증가(가용성 저하)
- DB 커넥션 점유 시간 증가에 따른 커넥션 풀 고갈 위험
- PG 응답 지연/실패 시 트랜잭션 전체 롤백되어 결제는 취소되었으나 예약 상태가 갱신되지 않는 정합성 이탈 가능성(반대 케이스 포함)
PR 목적에도 명시된 대로(“Toss 외부 API 호출을 트랜잭션 밖으로 분리”), 외부 API 호출을 트랜잭션 경계 밖으로 분리하고 멱등성·상태 검증을 적용하시길 권장합니다.
권장 패턴:
- 짧은 트랜잭션①: 락으로 상태를
CANCELING(또는 유사) 등 중간 상태로 전이 + 환불 의도 기록.- 트랜잭션 밖: PG 취소 호출(멱등키/이전 결과 조회로 중복 호출 방지).
- 짧은 트랜잭션②: 결과를 반영하여
CANCELED로 확정 또는 보상 처리.원하시면 위 3단계 패턴을 적용한 리팩터 초안(상태 머신·멱등키 설계 포함)을 작성해 드릴까요? 별도 이슈로 분리하는 것도 권장드립니다.
As per coding guidelines, "트랜잭션 관리 - DB 일관성, 롤백 정책이 올바른지 검토" 및 "성능 및 효율성 - 불필요한 DB 쿼리/잠금 시간 고려".
Also applies to: 253-259
🤖 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/com/eatsfine/domain/booking/service/BookingCommandServiceImpl.java` around lines 206 - 210, The code currently holds a PESSIMISTIC_WRITE lock (via findByIdWithLock) while calling external PG API paymentService.cancelPayment inside cancelBooking/cancelBookingByOwner; refactor to a 3-step flow: (1) short transaction using findByIdWithLock to set booking status to an intermediate state (e.g., CANCELING) and record a refund intent/idempotency key, then commit; (2) outside any transaction call paymentService.cancelPayment using the recorded idempotency key and perform result/timeout handling; (3) in a second short transaction load booking by id, verify current status and idempotency/result, and update to CANCELED or perform compensating actions; ensure paymentService.cancelPayment is never invoked while holding the DB lock and add idempotency/state validation to avoid duplicate or inconsistent transitions.
🤖 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/com/eatsfine/domain/booking/service/BookingCommandServiceImpl.java`:
- Around line 130-137: The catch for DataIntegrityViolationException in
BookingCommandServiceImpl around bookingRepository.save blindly maps all
integrity errors to BookingErrorStatus._ALREADY_RESERVED_TABLE; change it to
inspect the root SQLException/constraint name from the
DataIntegrityViolationException (unwrap e.getRootCause() or use ExceptionUtils)
and only convert to BookingException(BookingErrorStatus._ALREADY_RESERVED_TABLE)
when the constraint name equals "uq_booking_table_slot"; for any other
constraint or null/unknown root cause rethrow the original
DataIntegrityViolationException (or wrap it in a BookingException that preserves
the original exception as the cause) so the stack trace and correct error type
are retained. Ensure the thrown BookingException includes the original exception
as its cause when you do map or wrap.
---
Outside diff comments:
In
`@src/main/java/com/eatsfine/domain/booking/service/BookingCommandServiceImpl.java`:
- Around line 187-271: Both cancelBooking and cancelBookingByOwner share the
same cancellation steps (locked booking retrieval already done, status checks,
calling paymentService.cancelPayment when BookingStatus.CONFIRMED, invoking
booking.cancel(reason) and building the response), so extract that core flow
into a private helper like executeCancellation(Booking booking, String reason)
that: verifies cancelable state (throws same
BookingException._ALREADY_CANCELED), if CONFIRMED calls
paymentService.cancelPayment(booking.getSuccessPaymentKey(), new
PaymentRequestDTO.CancelPaymentDTO(reason)), calls booking.cancel(reason), and
returns the common data (booking id, status, refund amount and optionally
canceledAt if needed) so that cancelBooking keeps only user ownership check and
cancelBookingByOwner keeps owner/table validation and both call
executeCancellation to perform the shared work.
- Around line 206-210: The code currently holds a PESSIMISTIC_WRITE lock (via
findByIdWithLock) while calling external PG API paymentService.cancelPayment
inside cancelBooking/cancelBookingByOwner; refactor to a 3-step flow: (1) short
transaction using findByIdWithLock to set booking status to an intermediate
state (e.g., CANCELING) and record a refund intent/idempotency key, then commit;
(2) outside any transaction call paymentService.cancelPayment using the recorded
idempotency key and perform result/timeout handling; (3) in a second short
transaction load booking by id, verify current status and idempotency/result,
and update to CANCELED or perform compensating actions; ensure
paymentService.cancelPayment is never invoked while holding the DB lock and add
idempotency/state validation to avoid duplicate or inconsistent transitions.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 60e60ba0-18bd-42ae-818e-acccacfc2fb3
📒 Files selected for processing (1)
src/main/java/com/eatsfine/domain/booking/service/BookingCommandServiceImpl.java
- flyway-mysql 의존성 추가 - V1 마이그레이션: booking_date/time 백필 → NOT NULL → 유니크 제약 추가 - Booking.version 컬럼 추가 (낙관적 락) - application-local: ddl-auto update→validate, flyway baseline-on-migrate 설정 - application-test: flyway disabled (H2 create-drop 유지) [prod 적용 가이드] application-prod.yml에 아래 설정 추가 필요: spring.jpa.hibernate.ddl-auto: validate spring.flyway.enabled: true spring.flyway.baseline-on-migrate: true spring.flyway.baseline-version: 0 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- GeneralException/BookingException에 cause 생성자 추가 (원본 스택 트레이스 보존) - uq_booking_table_slot 위반만 _ALREADY_RESERVED_TABLE(409)로 변환 - FK/NOT NULL 등 다른 제약 위반은 원본 예외 그대로 전파 (500 처리 유지) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
macOS 대소문자 무시 파일시스템으로 인해 이전 커밋에서 GeneralException.java 변경사항이 누락됨. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- PaymentService.confirmPayment: orElse(null) → orElseThrow로 변경하여 예약 누락 방지 - CANCELED 상태 감지 시 Toss 자동 환불 후 payment.cancelPayment() 처리 (noRollbackFor = GeneralException.class로 환불 상태 커밋 보장) - CONFIRMED 상태는 멱등성 처리 (중복 확정 방지) - BookingCommandServiceImpl.confirmPayment: CANCELED 체크 추가하여 취소된 예약이 CONFIRMED로 잘못 전이되는 상태 머신 위반 방지 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
실제 취소 여부와 무관하게 successCount가 증가하던 문제 수정. 취소 완료/스킵/시도 건수를 분리하여 로그 지표 명확화. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/main/java/com/eatsfine/domain/booking/service/BookingCommandServiceImpl.java (1)
213-217:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift비관적 락 +
@Transactional내에서 외부 환불 API를 호출하고 있습니다.L199에서
findByIdWithLock으로 booking에 PESSIMISTIC_WRITE 락을 획득한 상태로, L216paymentService.cancelPayment(...)가 Toss 외부 API를 호출합니다(PaymentService.cancelPayment도@Transactional이므로 동일 트랜잭션에서 외부 호출 + DB 락이 묶입니다). 네트워크 지연 동안 booking 행 락이 유지되어 다른 동시 요청을 차단하고, connection pool 고갈 위험이 있습니다.PR 본문에서도
Toss 외부 API 호출을 트랜잭션 외부로 분리가 추가 개선 권장사항으로 명시되어 있어, 본 PR에서 반영하거나 후속 이슈로 트래킹할 것을 권장합니다.L265
cancelBookingByOwner의paymentService.cancelPayment(...)호출도 동일한 패턴으로 함께 개선이 필요합니다.As per coding guidelines, 트랜잭션 관리(트랜잭션 범위, 롤백 정책)와 성능(불필요한 락 보유 시간) 검토 항목에 해당합니다.
🤖 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/com/eatsfine/domain/booking/service/BookingCommandServiceImpl.java` around lines 213 - 217, The code currently holds a PESSIMISTIC_WRITE lock obtained by findByIdWithLock inside BookingCommandServiceImpl's transactional method while calling paymentService.cancelPayment (and likewise in cancelBookingByOwner), which performs an external Toss API call; refactor so the external call happens outside the `@Transactional` scope: persist any DB state that indicates a refund is required (e.g., set status/mark refund-intent and flush) inside the transaction and then perform paymentService.cancelPayment after the transaction commits (use TransactionSynchronizationManager.registerSynchronization/afterCommit callback, an application event published afterCommit, or split into two methods where one is `@Transactional` for DB updates and the other calls paymentService). Apply the same pattern to cancelBookingByOwner to avoid holding DB locks during network calls.src/main/java/com/eatsfine/domain/payment/service/PaymentService.java (1)
113-130:⚠️ Potential issue | 🔴 Critical | ⚡ Quick win결제 완료 처리와 상태 전이 검증 로직을 보완해주세요.
payment.completePayment()와payment.cancelPayment()메소드가 현재 상태를 검증하지 않아 멱등하지 않습니다.
completePayment()는 상태를 확인하지 않고 무조건COMPLETED로 변경cancelPayment()는 상태를 확인하지 않고 무조건REFUNDED로 변경따라서
confirmPayment()메소드의 흐름에서:
- L113에서
payment.completePayment()호출 (PENDING → COMPLETED)- 락 획득 후 booking이 CANCELED로 판명되면 L140에서
payment.cancelPayment()호출 (COMPLETED → REFUNDED)이 과정에서
payment.completePayment()가 재호출될 경우나 동시성 상황에서 예상치 못한 상태 전이가 발생할 수 있습니다.해결 방안:
- Payment 엔티티의
completePayment(),cancelPayment()메소드에 상태 검증 로직 추가- 또는
confirmPayment()메소드 내 호출 전 결제 상태 사전 검증참고:
processWebhook()메소드(L283-287)는 올바르게 상태 전이를 검증하고 있으므로, 동일한 패턴을 적용하면 됩니다.🤖 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/com/eatsfine/domain/payment/service/PaymentService.java` around lines 113 - 130, The payment state transitions are not protected: update Payment entity methods completePayment() and cancelPayment() (or alternatively the confirmPayment() flow) must validate current status before changing it to prevent invalid or non-idempotent transitions; update Payment.completePayment() to only set COMPLETED when current state is PENDING (or other allowed predecessors) and throw a PaymentException otherwise, and update Payment.cancelPayment() to only set REFUNDED when current state is COMPLETED (or allowed predecessors); you can mirror the checks used in processWebhook() to implement the same validation pattern and/or add pre-checks in confirmPayment() before calling payment.completePayment() / payment.cancelPayment().
🤖 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/com/eatsfine/domain/payment/service/PaymentService.java`:
- Around line 131-144: The code currently holds a PESSIMISTIC_WRITE lock from
bookingRepository.findByIdWithLock(...) while calling the external
tossPaymentService.cancel(...) and then updates payment.cancelPayment(), which
blocks DB rows and risks inconsistency if the external call fails; refactor so
the locked transaction only validates and persists the booking/payment state
change (e.g., set status to CANCELED or create a CancellationRequest record) and
commit immediately, then perform tossPaymentService.cancel(...) outside the
transaction (via a separate `@Transactional` method, background worker/event
published, or retry queue) using an idempotency key; ensure failures from
tossPaymentService.cancel are not merely logged—push to retry/compensation
queue, emit metrics/alerts, and record the external refund outcome in a
follow-up transactional update method (use symbols payment.cancelPayment(),
BookingException/BookingErrorStatus._ALREADY_CANCELED as before).
In `@src/main/resources/db/migration/V1__add_booking_table_slot_constraints.sql`:
- Around line 17-29: The ALTER TABLE ... MODIFY COLUMN to NOT NULL can fail if
booking_table contains rows without matching booking rows because the UPDATE
uses an INNER JOIN; before making columns NOT NULL either (a) change the
backfill UPDATE (the UPDATE booking_table bt INNER JOIN booking b ...) to a LEFT
JOIN and explicitly set bt.booking_date and bt.booking_time = NULL-safe values
or COALESCE from b (so orphan rows are handled), or (b) add a pre-check and
cleanup step that runs SELECT COUNT(*) FROM booking_table WHERE booking_id IS
NULL OR booking_id NOT IN (SELECT id FROM booking) and aborts/fails the
migration if >0, and ensure a DB-level FK exists for booking_table.booking_id;
update the ALTER TABLE booking_table MODIFY COLUMN booking_date DATE NOT NULL,
MODIFY COLUMN booking_time TIME NOT NULL only after orphan rows are fixed or
default values are assigned.
- Around line 38-40: The migration adds booking.version as NULL which leaves
existing rows with NULL and breaks JPA `@Version` semantics; update the migration
to backfill existing Booking rows to 0 and then alter the booking.version column
to NOT NULL with DEFAULT 0 — specifically: after ADD COLUMN version BIGINT NULL,
run an UPDATE to set version = 0 for all existing rows, then ALTER TABLE booking
ALTER COLUMN version SET DEFAULT 0 and ALTER TABLE booking ALTER COLUMN version
SET NOT NULL (or equivalent for your SQL dialect) so Hibernate sees non-null
version values for preexisting rows.
In `@src/test/resources/application-test.yml`:
- Around line 11-12: 테스트 환경(application-test.yml)에서 flyway.enabled가 false로 되어 있어
마이그레이션 스크립트(V1 및 uq_booking_table_slot 제약 등)가 검증되지 않으니, 마이그레이션 회귀를 방지하려면 통합
테스트(예: Testcontainers MySQL)를 별도로 추가하여 해당 환경에서 flyway.enabled를 true로 켜고 실제 DB에
Flyway를 실행해 V1 마이그레이션과 제약·백필 로직을 검증하도록 설정하십시오.
---
Outside diff comments:
In
`@src/main/java/com/eatsfine/domain/booking/service/BookingCommandServiceImpl.java`:
- Around line 213-217: The code currently holds a PESSIMISTIC_WRITE lock
obtained by findByIdWithLock inside BookingCommandServiceImpl's transactional
method while calling paymentService.cancelPayment (and likewise in
cancelBookingByOwner), which performs an external Toss API call; refactor so the
external call happens outside the `@Transactional` scope: persist any DB state
that indicates a refund is required (e.g., set status/mark refund-intent and
flush) inside the transaction and then perform paymentService.cancelPayment
after the transaction commits (use
TransactionSynchronizationManager.registerSynchronization/afterCommit callback,
an application event published afterCommit, or split into two methods where one
is `@Transactional` for DB updates and the other calls paymentService). Apply the
same pattern to cancelBookingByOwner to avoid holding DB locks during network
calls.
In `@src/main/java/com/eatsfine/domain/payment/service/PaymentService.java`:
- Around line 113-130: The payment state transitions are not protected: update
Payment entity methods completePayment() and cancelPayment() (or alternatively
the confirmPayment() flow) must validate current status before changing it to
prevent invalid or non-idempotent transitions; update Payment.completePayment()
to only set COMPLETED when current state is PENDING (or other allowed
predecessors) and throw a PaymentException otherwise, and update
Payment.cancelPayment() to only set REFUNDED when current state is COMPLETED (or
allowed predecessors); you can mirror the checks used in processWebhook() to
implement the same validation pattern and/or add pre-checks in confirmPayment()
before calling payment.completePayment() / payment.cancelPayment().
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a08696c7-0a2c-415f-b17c-9471d64be62a
📒 Files selected for processing (8)
build.gradlesrc/main/java/com/eatsfine/domain/booking/exception/BookingException.javasrc/main/java/com/eatsfine/domain/booking/service/BookingCommandServiceImpl.javasrc/main/java/com/eatsfine/domain/payment/service/PaymentService.javasrc/main/java/com/eatsfine/global/apipayload/exception/GeneralException.javasrc/main/resources/application-local.ymlsrc/main/resources/db/migration/V1__add_booking_table_slot_constraints.sqlsrc/test/resources/application-test.yml
| } else if (lockedBooking.getStatus() == BookingStatus.CANCELED) { | ||
| // 보상 처리: 스케줄러 등에 의해 예약이 취소됐으나 결제가 완료된 경우 자동 환불 | ||
| log.warn("Booking {} is CANCELED but payment was completed. Triggering compensation refund for OrderID: {}", | ||
| lockedBooking.getId(), dto.orderId()); | ||
| try { | ||
| tossPaymentService.cancel(response.paymentKey(), | ||
| new PaymentRequestDTO.CancelPaymentDTO("예약 취소 상태에서 결제 완료 - 자동 환불")); | ||
| } catch (Exception refundEx) { | ||
| log.error("Compensation refund failed for OrderID: {}. Manual intervention required.", | ||
| dto.orderId(), refundEx); | ||
| } | ||
| payment.cancelPayment(); | ||
| // noRollbackFor = GeneralException.class 이므로 payment.cancelPayment() 변경은 커밋됨 | ||
| throw new BookingException(BookingErrorStatus._ALREADY_CANCELED); |
There was a problem hiding this comment.
비관적 락 + 트랜잭션 내에서 외부 결제 취소 API를 호출하고 있습니다.
bookingRepository.findByIdWithLock(...)로 booking 행에 PESSIMISTIC_WRITE 락을 잡은 상태로 tossPaymentService.cancel(...) 외부 호출을 수행합니다. 네트워크 지연/타임아웃 동안 booking 행 락이 유지되어 다른 트랜잭션이 차단되며, Toss 측 응답 지연 시 connection pool 고갈로 이어질 수 있습니다.
권장 패턴:
- 락 트랜잭션에서는 booking 상태만 검증·기록하고 즉시 커밋
- 외부 환불 호출은 트랜잭션 외부에서 수행 (별도 컴포넌트/이벤트 발행)
- 환불 결과는 멱등 키 기반 후속 트랜잭션으로 반영
또한 try/catch (Exception refundEx)로 환불 실패를 단순 로깅 후 통과시키고 있습니다. 환불 실패 시 payment.cancelPayment()만 호출되어 DB는 취소 상태로 보이지만 Toss 측 자금은 보류된 채 남는 정합성 깨짐이 발생합니다. 최소한 알림/메트릭/재시도 큐 적재로 수동 개입을 보장해야 합니다.
As per coding guidelines, 트랜잭션 관리와 DB 일관성·롤백 정책을 검토해야 합니다.
🤖 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/com/eatsfine/domain/payment/service/PaymentService.java` around
lines 131 - 144, The code currently holds a PESSIMISTIC_WRITE lock from
bookingRepository.findByIdWithLock(...) while calling the external
tossPaymentService.cancel(...) and then updates payment.cancelPayment(), which
blocks DB rows and risks inconsistency if the external call fails; refactor so
the locked transaction only validates and persists the booking/payment state
change (e.g., set status to CANCELED or create a CancellationRequest record) and
commit immediately, then perform tossPaymentService.cancel(...) outside the
transaction (via a separate `@Transactional` method, background worker/event
published, or retry queue) using an idempotency key; ensure failures from
tossPaymentService.cancel are not merely logged—push to retry/compensation
queue, emit metrics/alerts, and record the external refund outcome in a
follow-up transactional update method (use symbols payment.cancelPayment(),
BookingException/BookingErrorStatus._ALREADY_CANCELED as before).
| UPDATE booking_table bt | ||
| INNER JOIN booking b ON bt.booking_id = b.id | ||
| SET bt.booking_date = b.booking_date, | ||
| bt.booking_time = b.booking_time, | ||
| bt.is_active = CASE | ||
| WHEN b.status IN ('CONFIRMED', 'PENDING') THEN 1 | ||
| ELSE NULL | ||
| END; | ||
|
|
||
| -- [Step 3] 백필 완료 후 NOT NULL 적용 | ||
| ALTER TABLE booking_table | ||
| MODIFY COLUMN booking_date DATE NOT NULL, | ||
| MODIFY COLUMN booking_time TIME NOT NULL; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# booking_table.booking_id에 FK 제약이 정의되어 있는지 엔티티/스키마에서 확인
fd -t f 'BookingTable.java' src/main/java | xargs cat
echo "---"
rg -nP --type=sql -C2 'booking_table' src/main/resources/dbRepository: Eatsfine/BE
Length of output: 4978
🏁 Script executed:
cat -n src/main/resources/db/migration/V1__add_booking_table_slot_constraints.sqlRepository: Eatsfine/BE
Length of output: 1838
🏁 Script executed:
# Check if there are any other migration files that might have created the FK initially
fd -t f 'V*.sql' src/main/resources/db/migration | sortRepository: Eatsfine/BE
Length of output: 130
🏁 Script executed:
# Search for FK constraint definitions in migration files
rg -n "FOREIGN KEY|CONSTRAINT.*booking" src/main/resources/db/migration --type=sqlRepository: Eatsfine/BE
Length of output: 174
🏁 Script executed:
fd -t f '\.(sql|ddl)' src/main/resources | head -20Repository: Eatsfine/BE
Length of output: 130
🏁 Script executed:
# Search for any reference to FOREIGN KEY or FK constraints related to booking
rg -n "FOREIGN KEY|booking_id.*booking" src/main/resources --type=sql -B2 -A2Repository: Eatsfine/BE
Length of output: 37
🏁 Script executed:
# Check if there are any database initialization or schema files
find src/main/resources -name "*.sql" -o -name "*.ddl" | xargs ls -la 2>/dev/null | head -20Repository: Eatsfine/BE
Length of output: 179
🏁 Script executed:
fd -t f 'application\.(yml|yaml|properties)' src/main/resourcesRepository: Eatsfine/BE
Length of output: 90
🏁 Script executed:
# Check for any schema.sql or data.sql files
fd -t f '(schema|data)\.sql' src/main/resourcesRepository: Eatsfine/BE
Length of output: 37
🏁 Script executed:
# Look at application configuration
find src/main/resources -name "application*" -exec cat {} \;Repository: Eatsfine/BE
Length of output: 4389
Step 3의 NOT NULL 변경은 기존 데이터 무결성에 따라 실패할 수 있습니다.
Step 2의 INNER JOIN은 booking_table.booking_id가 booking.id와 일치하는 행만 갱신합니다. booking_table에 고아 행(예: booking_id IS NULL 또는 참조할 수 없는 booking_id)이 존재하면 백필이 누락된 상태로 남아 Step 3의 MODIFY COLUMN ... NOT NULL 실행 시 오류가 발생하여 마이그레이션이 중단됩니다. 본 마이그레이션 적용 전 다음을 사전 확인해야 합니다.
booking_table.booking_id에 데이터베이스 레벨의 FK 제약이 존재하는지 확인SELECT COUNT(*) FROM booking_table WHERE booking_id IS NULL OR booking_id NOT IN (SELECT id FROM booking)쿼리 결과가 0인지 검증
🤖 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/db/migration/V1__add_booking_table_slot_constraints.sql`
around lines 17 - 29, The ALTER TABLE ... MODIFY COLUMN to NOT NULL can fail if
booking_table contains rows without matching booking rows because the UPDATE
uses an INNER JOIN; before making columns NOT NULL either (a) change the
backfill UPDATE (the UPDATE booking_table bt INNER JOIN booking b ...) to a LEFT
JOIN and explicitly set bt.booking_date and bt.booking_time = NULL-safe values
or COALESCE from b (so orphan rows are handled), or (b) add a pre-check and
cleanup step that runs SELECT COUNT(*) FROM booking_table WHERE booking_id IS
NULL OR booking_id NOT IN (SELECT id FROM booking) and aborts/fails the
migration if >0, and ensure a DB-level FK exists for booking_table.booking_id;
update the ALTER TABLE booking_table MODIFY COLUMN booking_date DATE NOT NULL,
MODIFY COLUMN booking_time TIME NOT NULL only after orphan rows are fixed or
default values are assigned.
| -- [Step 5] Booking 낙관적 락(@Version) 컬럼 추가 — NULL 허용 (JPA가 첫 write 시 0으로 초기화) | ||
| ALTER TABLE booking | ||
| ADD COLUMN version BIGINT NULL; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
JPA Hibernate @Version column NULL behavior on existing rows
💡 Result:
When adding a @Version column to an existing JPA/Hibernate entity with pre-existing rows where the version column is NULL, Hibernate treats entities loaded from these rows as transient (new) rather than detached/persistent. This is because Hibernate detects a null version as indicating a transient instance, regardless of other unsaved-value strategies [1][2]. Attempting to update such an entity will fail with exceptions like PropertyValueException ("Detached entity with generated id has an uninitialized version value 'null'") [3][4], OptimisticLockException, or NPE when trying to increment null [5][6][7]. For new entities, Hibernate initializes @Version to 0 (for numeric types like int/Long with default 0) on insert [8]. On first update, it uses WHERE version = 0 and increments to 1 [9][8]. However, for existing rows with NULL version, loading sets the Java field to null, triggering transient detection [1]. Recommended handling for existing rows: - Add the column as nullable initially. - Update all rows to a starting value (e.g., UPDATE table SET version = 0) [10]. - Alter to NOT NULL [10][6]. - Use database defaults where supported (e.g., Oracle NOT NULL DEFAULT 0) to auto-set on add [10][6]. The @Version column should ideally be NOT NULL in the database with a default value like 0 or 1 for consistency [11][1]. Using a primitive int (defaults to 0) avoids null in Java but requires DB backfill since primitives can't represent NULL [11][8]. Nullable Integer allows null but risks transient misdetection [1][2]. Vlad Mihalcea recommends primitive short/smallint for efficiency, implying non-null [11]. Official docs note nullable version properties help avoid reattachment issues but confirm null versions signal transient [2].
Citations:
- 1: https://discourse.hibernate.org/t/question-about-optimistic-locking-documentation/7212
- 2: https://docs.hibernate.org/orm/5.2/userguide/html_single/chapters/locking/Locking.html
- 3: https://access.redhat.com/solutions/7041395
- 4: HHH-20320 fix: detached versioned entity parameter triggers auto-flush transient check hibernate/hibernate-orm#12133
- 5: https://forum.hibernate.org/viewtopic.php?p=2492682
- 6: https://stackoverflow.com/questions/2146423/hibernate-automatic-versioning-nullable-column
- 7: https://stackoverflow.com/questions/47590436/optimistic-lock-failure-when-version-is-null
- 8: https://vladmihalcea.com/jpa-entity-version-property-hibernate/
- 9: https://vladmihalcea.com/optimistic-locking-version-property-jpa-hibernate
- 10: https://stackoverflow.com/questions/39509851/how-to-set-default-value-to-version-column-for-existing-data
- 11: https://vladmihalcea.com/best-way-map-entity-version-jpa-hibernate
🏁 Script executed:
# Find the booking entity class
fd -t f "Booking" --type f -e java | head -20Repository: Eatsfine/BE
Length of output: 1398
🏁 Script executed:
# Search for `@Version` annotation in the codebase
rg "@Version" -t java --no-headingRepository: Eatsfine/BE
Length of output: 205
🏁 Script executed:
# Check the complete migration file to see context
cat src/main/resources/db/migration/V1__add_booking_table_slot_constraints.sqlRepository: Eatsfine/BE
Length of output: 1558
🏁 Script executed:
# Check for other migration files that might handle the backfill
ls -la src/main/resources/db/migration/Repository: Eatsfine/BE
Length of output: 258
🏁 Script executed:
# Read the Booking entity around the `@Version` field
cat -n src/main/java/com/eatsfine/domain/booking/entity/Booking.java | sed -n '1,150p'Repository: Eatsfine/BE
Length of output: 5027
🏁 Script executed:
# Search for `@Version` and surrounding context in Booking.java
rg -A 2 -B 2 "@Version" src/main/java/com/eatsfine/domain/booking/entity/Booking.javaRepository: Eatsfine/BE
Length of output: 117
마이그레이션 파일에서 version 컬럼 NULL 초기화 누락 — 기존 Booking 데이터 업데이트 실패 위험
Booking 엔티티가 @Version 낙관적 락을 사용하고 있으나, 마이그레이션에서 version 컬럼을 NULL로만 추가하고 백필 없이 진행됩니다. Hibernate는 NULL version을 transient(새로운) 엔티티로 인식하므로, 마이그레이션 이전의 기존 Booking 행을 수정할 때 WHERE version = NULL 조건이 SQL에서 false로 평가되어 OptimisticLockException이 발생하거나 의도치 않은 거동을 보입니다.
마이그레이션에 다음 단계를 추가하세요:
- 기존 행의 version을 0으로 백필
- 컬럼을 NOT NULL DEFAULT 0으로 변경
수정 예시
ALTER TABLE booking
ADD COLUMN version BIGINT NULL;
+
+-- 기존 행 낙관적 락 초기화
+UPDATE booking SET version = 0 WHERE version IS NULL;
+
+ALTER TABLE booking
+ MODIFY COLUMN version BIGINT NOT NULL DEFAULT 0;🤖 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/db/migration/V1__add_booking_table_slot_constraints.sql`
around lines 38 - 40, The migration adds booking.version as NULL which leaves
existing rows with NULL and breaks JPA `@Version` semantics; update the migration
to backfill existing Booking rows to 0 and then alter the booking.version column
to NOT NULL with DEFAULT 0 — specifically: after ADD COLUMN version BIGINT NULL,
run an UPDATE to set version = 0 for all existing rows, then ALTER TABLE booking
ALTER COLUMN version SET DEFAULT 0 and ALTER TABLE booking ALTER COLUMN version
SET NOT NULL (or equivalent for your SQL dialect) so Hibernate sees non-null
version values for preexisting rows.
| flyway: | ||
| enabled: false # 테스트는 H2 ddl-auto:create-drop으로 스키마 관리 |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚖️ Poor tradeoff
테스트에서 Flyway가 비활성화되어 마이그레이션 스크립트는 검증되지 않습니다.
H2 create-drop로 스키마를 생성하므로 V1 마이그레이션 스크립트 자체의 정합성(SQL 호환성, 백필 로직, uq_booking_table_slot 제약)은 단위 테스트로 검증되지 않습니다. 마이그레이션 회귀 방지를 위해 별도의 통합 테스트(Testcontainers MySQL 등)에서 Flyway를 enabled로 두고 검증하는 것을 권장합니다.
🤖 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/resources/application-test.yml` around lines 11 - 12, 테스트
환경(application-test.yml)에서 flyway.enabled가 false로 되어 있어 마이그레이션 스크립트(V1 및
uq_booking_table_slot 제약 등)가 검증되지 않으니, 마이그레이션 회귀를 방지하려면 통합 테스트(예: Testcontainers
MySQL)를 별도로 추가하여 해당 환경에서 flyway.enabled를 true로 켜고 실제 DB에 Flyway를 실행해 V1 마이그레이션과
제약·백필 로직을 검증하도록 설정하십시오.
전체 Booking 엔티티 로드 후 ID 매핑 → SELECT id만 조회하는 findIdsByStatusAndCreatedAtBefore() 추가로 불필요한 메모리/GC 부하 제거. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/main/java/com/eatsfine/domain/booking/repository/BookingRepository.java (1)
76-78:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
findByIdWithLock에 락 타임아웃을 추가해 무기한 대기를 방지하세요.Line 76-78의
PESSIMISTIC_WRITE조회는 타임아웃 힌트가 없어 경합 시 대기 시간이 과도해질 수 있습니다. 스케줄러/결제 동시 경로에서 커넥션 점유가 길어지는 장애로 이어질 수 있어 timeout을 명시하는 게 안전합니다.🔧 제안 수정
+import jakarta.persistence.QueryHint; import org.springframework.data.jpa.repository.Lock; import org.springframework.data.jpa.repository.Query; +import org.springframework.data.jpa.repository.QueryHints; ... `@Lock`(LockModeType.PESSIMISTIC_WRITE) + `@QueryHints`(`@QueryHint`(name = "jakarta.persistence.lock.timeout", value = "3000")) `@Query`("SELECT b FROM Booking b WHERE b.id = :id") Optional<Booking> findByIdWithLock(`@Param`("id") Long id);#!/bin/bash set -euo pipefail echo "[1] 락 조회 메서드와 timeout 힌트 존재 여부 확인" rg -n -C3 --type java 'findByIdWithLock|PESSIMISTIC_WRITE|QueryHints|lock.timeout|QueryHint' echo echo "[2] 호출부 확인 (락 대기 영향 범위 파악)" rg -n -C4 --type java '\bfindByIdWithLock\s*\('As per coding guidelines, "4. 트랜잭션 관리 - DB 일관성, 롤백 정책이 올바른지 검토."
🤖 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/com/eatsfine/domain/booking/repository/BookingRepository.java` around lines 76 - 78, The PESSIMISTIC_WRITE query method findByIdWithLock in BookingRepository can block indefinitely under contention; add a lock timeout hint to the query to avoid infinite waits. Modify the `@Query/`@Lock declaration for findByIdWithLock to include a QueryHint setting javax.persistence.lock.timeout (or vendor equivalent, e.g., org.hibernate.lock.timeout) to a sensible millisecond value, so the method returns with an exception on timeout instead of waiting forever; ensure the repository method signature and semantics (Optional<Booking> findByIdWithLock) remain unchanged and update callers/transaction handling to handle the timeout exception appropriately.src/main/java/com/eatsfine/domain/booking/service/BookingCancelExecutor.java (1)
33-33: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick win취소 사유 문자열을 상수로 추출해 변경 지점을 단일화하세요.
Line 33의
"결제 시간 초과로 인한 자동 취소"는 하드코딩 문자열이라 추후 사유 확장/문구 변경 시 누락 위험이 있습니다.🔧 제안 수정
public class BookingCancelExecutor { + private static final String AUTO_CANCEL_REASON_PAYMENT_TIMEOUT = "결제 시간 초과로 인한 자동 취소"; private final BookingRepository bookingRepository; ... if (booking.getStatus() == BookingStatus.PENDING) { - booking.cancel("결제 시간 초과로 인한 자동 취소"); + booking.cancel(AUTO_CANCEL_REASON_PAYMENT_TIMEOUT); log.info("예약 ID {} 자동 취소 완료", bookingId); return true; }As per coding guidelines, "2. 코드 품질 & 가독성 - 매직 넘버, 하드코딩된 값이 존재하는지 점검."
🤖 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/com/eatsfine/domain/booking/service/BookingCancelExecutor.java` at line 33, Extract the hardcoded cancellation reason "결제 시간 초과로 인한 자동 취소" into a single constant and use that constant in BookingCancelExecutor where booking.cancel(...) is called; for example, add a constant (either a private static final String PAYMENT_TIMEOUT_REASON in BookingCancelExecutor or a centralized enum/class like BookingCancellationReasons.PAYMENT_TIMEOUT) and replace the literal in booking.cancel(...) with that constant, then update any other places that use the same literal to reference the new constant to centralize and standardize the cancellation reason.
🤖 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/com/eatsfine/domain/booking/service/BookingScheduler.java`:
- Around line 44-49: In BookingScheduler (the block where reservation
auto-cancel is attempted), update the exception logging inside the catch that
currently does log.warn("예약 ID {} 자동 취소 실패 — 다음 실행에서 재시도: {}", id,
e.getMessage()) to include the full exception object (e) so stacktrace and cause
are recorded (e.g., pass e as a parameter to the logger) and ensure you
increment or compute a failure counter (e.g., failedCount) for each caught
exception; then include that failure count alongside canceledCount, skippedCount
and expiredIds.size() in the final log.info summary so the total number of
failures is visible in the scheduler completion message.
---
Duplicate comments:
In `@src/main/java/com/eatsfine/domain/booking/repository/BookingRepository.java`:
- Around line 76-78: The PESSIMISTIC_WRITE query method findByIdWithLock in
BookingRepository can block indefinitely under contention; add a lock timeout
hint to the query to avoid infinite waits. Modify the `@Query/`@Lock declaration
for findByIdWithLock to include a QueryHint setting
javax.persistence.lock.timeout (or vendor equivalent, e.g.,
org.hibernate.lock.timeout) to a sensible millisecond value, so the method
returns with an exception on timeout instead of waiting forever; ensure the
repository method signature and semantics (Optional<Booking> findByIdWithLock)
remain unchanged and update callers/transaction handling to handle the timeout
exception appropriately.
In
`@src/main/java/com/eatsfine/domain/booking/service/BookingCancelExecutor.java`:
- Line 33: Extract the hardcoded cancellation reason "결제 시간 초과로 인한 자동 취소" into a
single constant and use that constant in BookingCancelExecutor where
booking.cancel(...) is called; for example, add a constant (either a private
static final String PAYMENT_TIMEOUT_REASON in BookingCancelExecutor or a
centralized enum/class like BookingCancellationReasons.PAYMENT_TIMEOUT) and
replace the literal in booking.cancel(...) with that constant, then update any
other places that use the same literal to reference the new constant to
centralize and standardize the cancellation reason.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a4f703d8-ecda-4fbf-b23b-0d88a4eb3d04
📒 Files selected for processing (3)
src/main/java/com/eatsfine/domain/booking/repository/BookingRepository.javasrc/main/java/com/eatsfine/domain/booking/service/BookingCancelExecutor.javasrc/main/java/com/eatsfine/domain/booking/service/BookingScheduler.java
| log.warn("예약 ID {} 자동 취소 실패 — 다음 실행에서 재시도: {}", id, e.getMessage()); | ||
| } | ||
| } | ||
|
|
||
| log.info("스케줄러 완료: 취소 {}건 / 스킵 {}건 / 시도 {}건", | ||
| canceledCount, skippedCount, expiredIds.size()); |
There was a problem hiding this comment.
실패 원인 추적을 위해 예외 객체와 실패 건수를 함께 기록하세요.
Line 44는 메시지만 남겨 원인 분석이 어렵고, Line 48-49 요약에도 실패 건수가 빠져 운영 지표 해석이 모호합니다.
🔧 제안 수정
int canceledCount = 0;
int skippedCount = 0;
+ int failedCount = 0;
for (Long id : expiredIds) {
try {
if (bookingCancelExecutor.cancelIfPending(id)) canceledCount++;
else skippedCount++;
} catch (Exception e) {
- log.warn("예약 ID {} 자동 취소 실패 — 다음 실행에서 재시도: {}", id, e.getMessage());
+ failedCount++;
+ log.warn("예약 ID {} 자동 취소 실패 — 다음 실행에서 재시도", id, e);
}
}
- log.info("스케줄러 완료: 취소 {}건 / 스킵 {}건 / 시도 {}건",
- canceledCount, skippedCount, expiredIds.size());
+ log.info("스케줄러 완료: 취소 {}건 / 스킵 {}건 / 실패 {}건 / 시도 {}건",
+ canceledCount, skippedCount, failedCount, expiredIds.size());As per coding guidelines, "1. 예외 처리 - 예외가 적절히 처리되었는지 확인."
🤖 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/com/eatsfine/domain/booking/service/BookingScheduler.java`
around lines 44 - 49, In BookingScheduler (the block where reservation
auto-cancel is attempted), update the exception logging inside the catch that
currently does log.warn("예약 ID {} 자동 취소 실패 — 다음 실행에서 재시도: {}", id,
e.getMessage()) to include the full exception object (e) so stacktrace and cause
are recorded (e.g., pass e as a parameter to the logger) and ensure you
increment or compute a failure counter (e.g., failedCount) for each caught
exception; then include that failure count alongside canceledCount, skippedCount
and expiredIds.size() in the final log.info summary so the total number of
failures is visible in the scheduler completion message.
💡 작업 개요
예약 생성·확인·취소 및 결제 확인 흐름에서 동시 요청이 들어올 때 발생할 수 있는 레이스 컨디션을 수정합니다.
오버부킹(이중 예약), 확정된 예약의 자동 취소, 중복 상태 변경 등의 데이터 정합성 문제를 방지합니다.
✅ 작업 내용
📋 상세 작업 내용
[CRITICAL] 스케줄러 트랜잭션 오용 수정
BookingScheduler:jakarta.transaction.Transactional→ Spring@Transactional교체BookingCancelExecutor로 분리 — 예약별 독립 트랜잭션(REQUIRES_NEW)으로 처리하여 낙관적 락 충돌 시 해당 예약만 실패하고 나머지 배치는 정상 처리[CRITICAL] Booking 낙관적 락 추가
Booking엔티티에@Version private Long version추가booking.confirm()/booking.cancel()을 호출하면 두 번째 커밋 시OptimisticLockingFailureException발생[CRITICAL] BookingTable DB 유니크 제약 추가 (소프트 딜리트 방식)
BookingTable에booking_date,booking_time,is_active(nullable) 컬럼 추가(store_table_id, booking_date, booking_time, is_active)유니크 제약 추가is_active = null로 슬롯 해제 — MySQL NULL 특성으로 취소 슬롯 재예약 허용, 이력 데이터 보존[HIGH] Booking 비관적 락 메서드 추가 및 적용
BookingRepository.findByIdWithLock()추가 (@Lock(PESSIMISTIC_WRITE))confirmPayment(),cancelBooking(),cancelBookingByOwner(),PaymentService.confirmPayment()에 락 적용PaymentService: Booking 상태 전이 조건을PENDING으로 한정 (COMPLETED/CANCELED 덮어쓰기 방지)[MEDIUM] DB 예외 도메인 변환
DataIntegrityViolationException→BookingException(_ALREADY_RESERVED_TABLE)변환[CHORE] Flyway 도입
ddl-auto: update의존에서 벗어나 안전한 단계별 마이그레이션 적용V1: booking_date/time NULL 허용 추가 → 백필 → NOT NULL 변환 → 유니크 제약 추가Flyway V1 마이그레이션이 서버 시작 시 자동 실행됩니다:
booking_table:booking_date,booking_time,is_active컬럼 추가booking_table: 유니크 제약uq_booking_table_slot추가booking:version컬럼 추가prod 환경 적용 전
application-prod.yml에 아래 설정 추가 필요 (담당자 직접 수정):🧪 테스트 내용
./gradlew test전체 통과📝 기타 참고 사항
Summary by CodeRabbit
릴리스 노트
새로운 기능
버그 수정
Chores