Merged
Conversation
Summary by CodeRabbit릴리스 노트
Walkthrough결제 상태 모델 확장, 웹훅 검증 강화(외부 결제 조회 추가), 중복 결제 방지 로직 및 스케줄러 기반 결제 복구 워크플로우가 도입되었습니다. Changes
Sequence Diagram(s)sequenceDiagram
participant Scheduler as PaymentRecoveryScheduler
participant Service as TosspaymentsPaymentService
participant Repo as TosspaymentsPaymentRepository
participant Recovery as PaymentRecoveryService
participant Client as TosspaymentsClient
participant Token as TokenFacadeService
Scheduler->>Service: findStalePayments(states, threshold, limit)
Service->>Repo: findStalePaymentsByStates(...)
Repo-->>Service: List<TosspaymentsPayment>
Service-->>Scheduler: stale payments
loop for each stale payment
Scheduler->>Scheduler: attemptLock(lockKey)
alt lock acquired
Scheduler->>Recovery: processRecovery(paymentKey)
Recovery->>Client: getPayment(paymentKey)
Client-->>Recovery: TosspaymentsPaymentResponse
alt response.status == DONE
Recovery->>Token: grantPurchasedTokens(tokenPurchase)
Token-->>Recovery: success
Recovery->>Repo: updateState(COMPLETED)
else response.status == CANCELED or PARTIAL_CANCELED
Recovery->>Repo: updateState(CANCELED)
else response.status == EXPIRED or ABORTED
Recovery->>Repo: updateState(NOT_NEED_CANCEL)
else
Recovery-->>Recovery: log and defer
end
end
Scheduler->>Scheduler: releaseLock(lockKey)
end
sequenceDiagram
participant Controller as WebhookController
participant Service as WebhookService
participant Repo as TosspaymentsPaymentRepository
participant Client as TosspaymentsClient
participant Facade as PaymentFacadeService
participant Token as TokenFacadeService
Controller->>Service: handleWebhook(payload)
Service->>Repo: findByPaymentKey(paymentKey)
Repo-->>Service: payment or null
alt payment not found or terminal
Service-->>Controller: log and return
else
Service->>Client: getPayment(paymentKey)
Client-->>Service: TosspaymentsPaymentResponse
alt canCompleteByWebhook && status == DONE
Service->>Token: existsByPaymentKey(paymentKey)?
alt not exists
Service->>Token: grantPurchasedTokens(...)
Token-->>Service: success
Service->>Facade: completePayment(paymentKey)
else exists
Service-->>Service: log duplicate and skip
end
else canCancelByWebhook && status == CANCELED
Service->>Repo: updateState(CANCELED)
else canResolveAsNotNeeded && status in (EXPIRED,ABORTED)
Service->>Repo: updateState(NOT_NEED_CANCEL)
else
Service-->>Service: log ignored action
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 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 unit tests (beta)
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 |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! 이 PR은 Toss Payments 연동 결제 및 환불 시스템의 안정성과 견고성을 대폭 향상시키는 데 중점을 둡니다. 중간 상태에 머물러 있는 결제를 자동으로 복구하는 스케줄러를 도입하고, 웹훅 처리 시 외부 결제 시스템의 실제 상태를 검증하는 단계를 추가하여 데이터 일관성을 보장합니다. 또한, 결제 상태 관리 로직을 세분화하고 토큰 지급 및 결제 취소 과정에서의 잠재적 문제를 방지하는 개선 사항들을 포함하여, 시스템의 전반적인 신뢰도를 높였습니다. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. 결제 상태 흐름, 웹훅 춤추고 복구하며, 토큰 이제 안전. Footnotes
|
Test Results 48 files 48 suites 1m 26s ⏱️ Results for commit 9ee17d5. ♻️ This comment has been updated with latest results. |
![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 PR significantly improves the payment and refund logic by refining PaymentState with an APPROVED status to separate payment approval from business logic, enhancing security by directly verifying payment status via the Toss Payments API during webhook processing, and improving system robustness with PaymentRecoveryScheduler and PaymentRecoveryService for stalled payments. A unique constraint on payment_key also prevents duplicate token issuance. Despite these improvements, several critical and high-severity security issues were identified. The most critical issue is the lack of payment amount verification in the webhook and recovery logic, which could allow attackers to bypass prices. Additionally, the refund logic lacks atomicity between external API calls and internal database updates, potentially leading to financial loss. Finally, sensitive payment identifiers are logged in plain text and should be masked. Beyond these security concerns, additional review comments address potential bugs affecting code execution and data consistency issues that require attention.
Comment on lines
+55
to
+58
| TosspaymentsStatus verifiedStatus = tossResponse.status(); | ||
|
|
||
| switch (verifiedStatus) { | ||
| case DONE -> handleDone(payment, tossResponse); |
There was a problem hiding this comment.
The WebhookService and PaymentRecoveryService do not verify that the payment details (specifically the totalAmount and orderId) returned by the Toss Payments API match the details stored in the application's database before granting tokens.
An attacker can exploit this by initiating a payment for a small amount (e.g., 1,000 KRW), completing it at Toss, and then sending a purchaseTokens request to the application with the same paymentKey but a much larger amount (e.g., 100,000 KRW). While the initial confirmPayment call will fail due to the amount mismatch, the subsequent webhook or recovery task will see the payment as DONE at Toss and grant tokens based on the fraudulent amount stored in the database.
To remediate this, call payment.validateTosspaymentsResult(tossResponse.paymentKey(), tossResponse.orderId(), tossResponse.totalAmount()) before proceeding to grant tokens.
Comment on lines
+16
to
+27
| @Query(""" | ||
| SELECT p FROM TosspaymentsPayment p | ||
| WHERE p.state IN :states | ||
| AND p.updatedAt < :threshold | ||
| ORDER BY p.updatedAt ASC | ||
| LIMIT :limit | ||
| """) | ||
| List<TosspaymentsPayment> findStalePaymentsByStates( | ||
| @Param("states") List<PaymentState> states, | ||
| @Param("threshold") LocalDateTime threshold, | ||
| @Param("limit") int limit | ||
| ); |
There was a problem hiding this comment.
JPQL 쿼리 내에 LIMIT 절을 사용하고 있습니다. LIMIT는 JPQL 표준이 아니므로 애플리케이션 실행 시 QuerySyntaxException이 발생할 수 있습니다. 이는 심각한 버그입니다.
데이터베이스에 독립적인 방식으로 페이징을 처리하려면 Spring Data JPA의 Pageable을 사용해야 합니다. @Query를 제거하고 메서드 시그니처를 다음과 같이 변경하여 수정할 수 있습니다. 이 변경은 TosspaymentsPaymentService의 호출 코드 수정도 필요합니다.
Page<TosspaymentsPayment> findByStateInAndUpdatedAtBefore(
@Param("states") List<PaymentState> states,
@Param("threshold") LocalDateTime threshold,
Pageable pageable
);
Comment on lines
+49
to
+52
| case CANCELED, PARTIAL_CANCELED -> { | ||
| payment.updateState(PaymentState.CANCELED); | ||
| log.info("복구 완료(취소) - paymentKey={}", paymentKey); | ||
| } |
There was a problem hiding this comment.
토스페이먼츠의 PARTIAL_CANCELED 상태를 내부적으로 CANCELED 상태로 처리하고 있습니다. 이는 데이터 불일치를 유발할 수 있습니다. PARTIAL_CANCELED는 결제 금액의 일부만 취소되었음을 의미하는데, 시스템에서는 전체가 취소된 것으로 기록됩니다.
만약 부분 환불 기능을 지원하지 않는다면, PARTIAL_CANCELED 상태는 예상치 못한 상태이므로 에러로 로깅하고 수동으로 확인하도록 처리하는 것이 더 안전합니다. 현재 로직은 사용자가 지불한 금액과 시스템에 기록된 토큰 상태 간의 불일치를 야기할 수 있습니다.
| private void handleDone(TosspaymentsPayment payment, WebhookPaymentData data) { | ||
| if (payment.getState().canCompleteByWebhook()) { | ||
| private void handleDone(TosspaymentsPayment payment, TosspaymentsPaymentResponse tossResponse) { | ||
| if (payment.canCompleteByWebhook()) { |
There was a problem hiding this comment.
웹훅을 통한 결제 완료 처리 시, 토큰이 이미 지급되었는지 확인하는 로직이 없습니다. API를 통한 결제 승인 흐름이나 복구 스케줄러에서는 existsByPaymentKey를 통해 중복 지급을 방지하고 있습니다. 웹훅 핸들러에서도 동일한 확인 로직을 추가하여 멱등성을 보장하고 불필요한 데이터베이스 예외 발생을 방지하는 것이 좋습니다.
Suggested change
| if (payment.canCompleteByWebhook()) { | |
| if (tokenFacadeService.existsByPaymentKey(payment.getPaymentKey())) { | |
| payment.updateState(PaymentState.COMPLETED); | |
| log.info("웹훅 스킵 - 이미 토큰이 지급된 결제: paymentKey={}", payment.getPaymentKey()); | |
| return; | |
| } | |
| if (payment.canCompleteByWebhook()) { |
Comment on lines
+29
to
+31
| log.info("웹훅 수신 - transmissionId: {}, retriedCount: {}, transmissionTime: {}, state: {}, paymentKey: {}", | ||
| transmissionId, retriedCount, | ||
| transmissionTime, payload.data().status(), payload.data().paymentKey()); |
There was a problem hiding this comment.
The application logs the raw paymentKey in multiple locations. The paymentKey is a sensitive identifier used by Toss Payments to manage transactions. Logging it in plain text increases the risk of exposure if log files are compromised. It is recommended to mask the paymentKey in log statements, showing only the first few characters.
Comment on lines
+19
to
+21
| public boolean canCancelByWebhook() { | ||
| return this == NEED_APPROVE || this == APPROVED || this == NEED_CANCEL || this == CONNECTION_TIMEOUT; | ||
| } |
There was a problem hiding this comment.
canCancelByWebhook 메서드의 내용이 canCompleteByWebhook 메서드와 완전히 동일합니다. 두 메서드의 의도가 같다면, 하나의 메서드로 통합하여 중복을 제거하는 것을 고려해 보세요. 예를 들어 isProcessableByWebhook() 같은 이름의 메서드를 만들 수 있습니다.
만약 미래에 두 메서드의 로직이 달라질 가능성이 있어 의도적으로 분리한 것이라면, 해당 의도를 주석으로 남겨두는 것이 다른 개발자가 코드를 이해하는 데 도움이 될 것입니다.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
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/samhap/kokomen/payment/service/WebhookService.java (1)
65-89:⚠️ Potential issue | 🔴 Critical
DONE웹훅 처리에 토큰 지급 idempotency 가드가 필요합니다.현재는 상태 조건만 맞으면 바로
grantPurchasedTokens를 호출합니다. 동시 처리 구간에서 동일 paymentKey로 중복 지급 시도가 발생할 수 있습니다.🧩 제안 수정
private void handleDone(TosspaymentsPayment payment, TosspaymentsPaymentResponse tossResponse) { - if (payment.canCompleteByWebhook()) { - PurchaseMetadata metadata = parseMetadata(payment.getMetadata()); - Long memberId = payment.getMemberId(); - int tokenCount = metadata.count(); + if (!payment.canCompleteByWebhook()) { + log.info("웹훅 완료 무시 - 완료 불가 상태: paymentKey={}, state={}", + payment.getPaymentKey(), payment.getState()); + return; + } - payment.updateState(PaymentState.COMPLETED); + if (tokenFacadeService.existsByPaymentKey(payment.getPaymentKey())) { + payment.updateState(PaymentState.COMPLETED); + log.info("웹훅 완료 스킵 - 이미 토큰 지급됨: paymentKey={}", payment.getPaymentKey()); + return; + } + + PurchaseMetadata metadata = parseMetadata(payment.getMetadata()); + Long memberId = payment.getMemberId(); + int tokenCount = metadata.count(); + + payment.updateState(PaymentState.COMPLETED); - TokenPurchase tokenPurchase = TokenPurchase.builder() - .memberId(memberId) - .paymentKey(payment.getPaymentKey()) - .orderId(payment.getOrderId()) - .totalAmount(payment.getTotalAmount()) - .orderName(payment.getOrderName()) - .productName(metadata.productName()) - .purchaseCount(tokenCount) - .unitPrice(metadata.unitPrice()) - .paymentMethod(tossResponse.method()) - .easyPayProvider(tossResponse.easyPay() != null ? tossResponse.easyPay().provider() : null) - .build(); - tokenFacadeService.grantPurchasedTokens(tokenPurchase, tokenCount); + TokenPurchase tokenPurchase = TokenPurchase.builder() + .memberId(memberId) + .paymentKey(payment.getPaymentKey()) + .orderId(payment.getOrderId()) + .totalAmount(payment.getTotalAmount()) + .orderName(payment.getOrderName()) + .productName(metadata.productName()) + .purchaseCount(tokenCount) + .unitPrice(metadata.unitPrice()) + .paymentMethod(tossResponse.method()) + .easyPayProvider(tossResponse.easyPay() != null ? tossResponse.easyPay().provider() : null) + .build(); + tokenFacadeService.grantPurchasedTokens(tokenPurchase, tokenCount); - log.info("웹훅으로 토큰 지급 완료 - memberId: {}, paymentKey: {}, tokenCount: {}", - memberId, payment.getPaymentKey(), tokenCount); - } + log.info("웹훅으로 토큰 지급 완료 - memberId: {}, paymentKey: {}, tokenCount: {}", + memberId, payment.getPaymentKey(), tokenCount); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/samhap/kokomen/payment/service/WebhookService.java` around lines 65 - 89, handleDone currently calls tokenFacadeService.grantPurchasedTokens whenever payment.canCompleteByWebhook() is true, which allows race conditions to double-grant tokens for the same paymentKey; add an idempotency guard around the token grant using the paymentKey (or payment id) as the unique key: before calling grantPurchasedTokens, check a durable marker (e.g., a TokenGrant record or a boolean column on TosspaymentsPayment) inside the same transactional/locked scope and only proceed if no grant exists, create the marker atomically (or have grantPurchasedTokens itself perform an upsert/unique-constraint-protected insert keyed by payment.getPaymentKey()), and ensure payment.updateState(PaymentState.COMPLETED) is applied only once after a successful, idempotent grant to prevent duplicate grants in concurrent handleDone executions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/com/samhap/kokomen/payment/controller/WebhookController.java`:
- Around line 29-31: The webhook log currently prints the raw paymentKey; add a
masking helper (e.g., a private method maskPaymentKey(String key) in
WebhookController) that returns "***" for null/short keys and otherwise returns
the first 8 chars plus "***", and change the log.info call to pass
maskPaymentKey(payload.data().paymentKey()) instead of the raw key to avoid
logging the full identifier.
In `@src/main/java/com/samhap/kokomen/payment/domain/PaymentState.java`:
- Around line 15-21: The two methods canCompleteByWebhook() and
canCancelByWebhook() return identical state sets; either consolidate them by
extracting a single helper like isWebhookProcessable() (used by both
canCompleteByWebhook and canCancelByWebhook) or, if their parity is intentional,
add a clear comment above both methods explaining they intentionally match and
may diverge later; update PaymentState accordingly so intent is explicit and
duplication is removed or documented.
In `@src/main/java/com/samhap/kokomen/payment/service/PaymentFacadeService.java`:
- Around line 145-153: The completePayment method can suffer race conditions;
annotate and protect it with the cross-cutting concerns and re-check state under
a distributed lock: add `@DistributedLock`(key = "#paymentKey"), `@ExecutionTimer`
and `@RedisExceptionWrapper` to the completePayment method in
PaymentFacadeService, acquire the lock using paymentKey, then inside the locked
section re-read TosspaymentsPayment via
tosspaymentsPaymentService.readByPaymentKey(paymentKey), verify
payment.isApproved() and that current state is not already CANCELED before
calling payment.updateState(PaymentState.COMPLETED); ensure the lock scope
covers the state re-read and update to avoid concurrent webhooks/recovery
overwriting final states.
In
`@src/main/java/com/samhap/kokomen/payment/service/PaymentRecoveryScheduler.java`:
- Line 25: Duplicate hard-coded lock key "lock:payment:" in
PaymentRecoveryScheduler should be centralized; move the LOCK_KEY_PREFIX value
out of PaymentRecoveryScheduler into a shared constant (e.g., a new or existing
LockKeys or Constants class) and replace the private static final String
LOCK_KEY_PREFIX in PaymentRecoveryScheduler with a reference to that shared
constant; update any other classes/AOP lock rule to use the same shared constant
to avoid drift and ensure compile-time usage via the new constant (refer to
LOCK_KEY_PREFIX and PaymentRecoveryScheduler when making the change).
In
`@src/main/java/com/samhap/kokomen/payment/service/PaymentRecoveryService.java`:
- Around line 70-74: The metadata parsing step in PaymentRecoveryService
(parseMetadata and PurchaseMetadata.count) can throw and leaves the payment in a
retryable state; wrap the parseMetadata call in a try/catch that on parse
failure sets the payment to a terminal failure state via
payment.updateState(PaymentState.FAILED_OR_INVALID) (or the appropriate terminal
enum), persists that state, and then throw the project-specific custom exception
(e.g., BadRequestException or a MetadataParseException) instead of
IllegalStateException; apply the same pattern to the other parsing block
referenced around lines 93-98 so failed parses do not cause perpetual scheduler
retries.
In `@src/main/java/com/samhap/kokomen/token/service/TokenFacadeService.java`:
- Around line 51-60: Wrap the critical section that creates/persists
TokenPurchase and grants tokens in a try-catch for
org.springframework.dao.DataIntegrityViolationException so a unique-constraint
race doesn’t return a 500; specifically, around the TokenPurchase creation/use
and the call to grantPurchasedTokens(tokenPurchase, tokenCount) (and the
subsequent paymentFacadeService.completePayment(request.paymentKey())), catch
DataIntegrityViolationException, log a clear message including memberId and
request.paymentKey() that the token was already granted (duplicate paymentKey),
and return gracefully instead of propagating the exception; keep the initial
soft-check using tokenPurchaseService.existsByPaymentKey(...) but treat the DB
unique-constraint violation as the definitive “already processed” case.
In `@src/main/resources/application.yml`:
- Around line 16-24: Increase the scheduler shutdown wait windows to avoid
interrupting long-running batch/IO work: raise
task.scheduling.shutdown.await-termination-period and align
lifecycle.timeout-per-shutdown-phase to the same larger value (e.g., several
minutes) so PaymentRecoveryScheduler and InterviewSchedulerService have enough
time to finish Redis scans, DB batches, external API/lock operations; update the
YAML keys lifecycle.timeout-per-shutdown-phase and
task.scheduling.shutdown.await-termination-period accordingly and keep
await-termination: true.
---
Outside diff comments:
In `@src/main/java/com/samhap/kokomen/payment/service/WebhookService.java`:
- Around line 65-89: handleDone currently calls
tokenFacadeService.grantPurchasedTokens whenever payment.canCompleteByWebhook()
is true, which allows race conditions to double-grant tokens for the same
paymentKey; add an idempotency guard around the token grant using the paymentKey
(or payment id) as the unique key: before calling grantPurchasedTokens, check a
durable marker (e.g., a TokenGrant record or a boolean column on
TosspaymentsPayment) inside the same transactional/locked scope and only proceed
if no grant exists, create the marker atomically (or have grantPurchasedTokens
itself perform an upsert/unique-constraint-protected insert keyed by
payment.getPaymentKey()), and ensure payment.updateState(PaymentState.COMPLETED)
is applied only once after a successful, idempotent grant to prevent duplicate
grants in concurrent handleDone executions.
ℹ️ Review info
Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5f177a60-a7c0-4a89-b399-19830a20d05d
📒 Files selected for processing (17)
src/main/java/com/samhap/kokomen/payment/controller/WebhookController.javasrc/main/java/com/samhap/kokomen/payment/domain/PaymentState.javasrc/main/java/com/samhap/kokomen/payment/domain/TosspaymentsPayment.javasrc/main/java/com/samhap/kokomen/payment/external/TosspaymentsClient.javasrc/main/java/com/samhap/kokomen/payment/repository/TosspaymentsPaymentRepository.javasrc/main/java/com/samhap/kokomen/payment/service/PaymentFacadeService.javasrc/main/java/com/samhap/kokomen/payment/service/PaymentRecoveryScheduler.javasrc/main/java/com/samhap/kokomen/payment/service/PaymentRecoveryService.javasrc/main/java/com/samhap/kokomen/payment/service/TosspaymentsPaymentService.javasrc/main/java/com/samhap/kokomen/payment/service/WebhookService.javasrc/main/java/com/samhap/kokomen/token/domain/TokenPurchase.javasrc/main/java/com/samhap/kokomen/token/repository/TokenPurchaseRepository.javasrc/main/java/com/samhap/kokomen/token/service/TokenFacadeService.javasrc/main/java/com/samhap/kokomen/token/service/TokenPurchaseService.javasrc/main/resources/application.ymlsrc/test/java/com/samhap/kokomen/payment/service/PaymentFacadeServiceTest.javasrc/test/java/com/samhap/kokomen/payment/service/WebhookServiceTest.java
src/main/java/com/samhap/kokomen/payment/controller/WebhookController.java
Outdated
Show resolved
Hide resolved
Comment on lines
15
to
+21
| public boolean canCompleteByWebhook() { | ||
| return this == NEED_APPROVE || this == APPROVED || this == NEED_CANCEL || this == CONNECTION_TIMEOUT; | ||
| } | ||
|
|
||
| public boolean canCancelByWebhook() { | ||
| return this == NEED_APPROVE || this == APPROVED || this == NEED_CANCEL || this == CONNECTION_TIMEOUT; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
canCompleteByWebhook()와 canCancelByWebhook()가 동일한 구현
두 메서드가 동일한 상태 집합을 반환하고 있습니다. 의미적 명확성을 위한 의도적인 설계라면 괜찮지만, 향후 분기될 가능성이 없다면 하나의 메서드로 통합하거나 주석으로 동일한 이유를 명시하는 것을 고려해 주세요.
♻️ 선택적 리팩토링: 공통 메서드 추출 또는 주석 추가
+ // 웹훅에서 완료 또는 취소 처리가 가능한 상태
+ // 완료와 취소 모두 동일한 비종료(non-terminal) 상태에서 가능
public boolean canCompleteByWebhook() {
return this == NEED_APPROVE || this == APPROVED || this == NEED_CANCEL || this == CONNECTION_TIMEOUT;
}
public boolean canCancelByWebhook() {
- return this == NEED_APPROVE || this == APPROVED || this == NEED_CANCEL || this == CONNECTION_TIMEOUT;
+ return canCompleteByWebhook(); // 현재 완료 가능 상태와 동일
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/com/samhap/kokomen/payment/domain/PaymentState.java` around
lines 15 - 21, The two methods canCompleteByWebhook() and canCancelByWebhook()
return identical state sets; either consolidate them by extracting a single
helper like isWebhookProcessable() (used by both canCompleteByWebhook and
canCancelByWebhook) or, if their parity is intentional, add a clear comment
above both methods explaining they intentionally match and may diverge later;
update PaymentState accordingly so intent is explicit and duplication is removed
or documented.
Comment on lines
+145
to
+153
| @Transactional | ||
| public void completePayment(String paymentKey) { | ||
| TosspaymentsPayment payment = tosspaymentsPaymentService.readByPaymentKey(paymentKey); | ||
| if (!payment.isApproved()) { | ||
| log.warn("결제 완료 처리 불가 - paymentKey: {}, 현재 상태: {}", paymentKey, payment.getState()); | ||
| return; | ||
| } | ||
| payment.updateState(PaymentState.COMPLETED); | ||
| } |
There was a problem hiding this comment.
completePayment가 동시성 경합에서 상태를 잘못 덮어쓸 수 있습니다.
현재 구현은 락 없이 상태를 읽고 갱신합니다. 동시 웹훅/복구 처리에서 CANCELED 전이를 COMPLETED로 역전시킬 수 있어 상태 정합성이 깨집니다.
🔒 제안 수정
- `@Transactional`
+ `@DistributedLock`(prefix = "payment", key = "#paymentKey")
+ `@Transactional`
public void completePayment(String paymentKey) {
TosspaymentsPayment payment = tosspaymentsPaymentService.readByPaymentKey(paymentKey);
if (!payment.isApproved()) {
log.warn("결제 완료 처리 불가 - paymentKey: {}, 현재 상태: {}", paymentKey, payment.getState());
return;
}
payment.updateState(PaymentState.COMPLETED);
}As per coding guidelines, "Use @DistributedLock, @ExecutionTimer, @RedisExceptionWrapper custom annotations for cross-cutting concerns".
📝 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
| @Transactional | |
| public void completePayment(String paymentKey) { | |
| TosspaymentsPayment payment = tosspaymentsPaymentService.readByPaymentKey(paymentKey); | |
| if (!payment.isApproved()) { | |
| log.warn("결제 완료 처리 불가 - paymentKey: {}, 현재 상태: {}", paymentKey, payment.getState()); | |
| return; | |
| } | |
| payment.updateState(PaymentState.COMPLETED); | |
| } | |
| `@DistributedLock`(prefix = "payment", key = "#paymentKey") | |
| `@Transactional` | |
| public void completePayment(String paymentKey) { | |
| TosspaymentsPayment payment = tosspaymentsPaymentService.readByPaymentKey(paymentKey); | |
| if (!payment.isApproved()) { | |
| log.warn("결제 완료 처리 불가 - paymentKey: {}, 현재 상태: {}", paymentKey, payment.getState()); | |
| return; | |
| } | |
| payment.updateState(PaymentState.COMPLETED); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/com/samhap/kokomen/payment/service/PaymentFacadeService.java`
around lines 145 - 153, The completePayment method can suffer race conditions;
annotate and protect it with the cross-cutting concerns and re-check state under
a distributed lock: add `@DistributedLock`(key = "#paymentKey"), `@ExecutionTimer`
and `@RedisExceptionWrapper` to the completePayment method in
PaymentFacadeService, acquire the lock using paymentKey, then inside the locked
section re-read TosspaymentsPayment via
tosspaymentsPaymentService.readByPaymentKey(paymentKey), verify
payment.isApproved() and that current state is not already CANCELED before
calling payment.updateState(PaymentState.COMPLETED); ensure the lock scope
covers the state re-read and update to avoid concurrent webhooks/recovery
overwriting final states.
| ); | ||
| private static final int STALE_THRESHOLD_MINUTES = 10; | ||
| private static final int MAX_RECOVERY_COUNT = 10; | ||
| private static final String LOCK_KEY_PREFIX = "lock:payment:"; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
락 키 포맷 상수 중복은 공통화해 두는 편이 안전합니다.
"lock:payment:" 하드코딩은 AOP 락 키 규칙과 드리프트가 생길 수 있습니다. 공통 유틸/상수로 한 곳에서 관리하는 것을 권장합니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/main/java/com/samhap/kokomen/payment/service/PaymentRecoveryScheduler.java`
at line 25, Duplicate hard-coded lock key "lock:payment:" in
PaymentRecoveryScheduler should be centralized; move the LOCK_KEY_PREFIX value
out of PaymentRecoveryScheduler into a shared constant (e.g., a new or existing
LockKeys or Constants class) and replace the private static final String
LOCK_KEY_PREFIX in PaymentRecoveryScheduler with a reference to that shared
constant; update any other classes/AOP lock rule to use the same shared constant
to avoid drift and ensure compile-time usage via the new constant (refer to
LOCK_KEY_PREFIX and PaymentRecoveryScheduler when making the change).
src/main/java/com/samhap/kokomen/payment/service/PaymentRecoveryService.java
Outdated
Show resolved
Hide resolved
Comment on lines
+16
to
+24
| lifecycle: | ||
| timeout-per-shutdown-phase: 60s | ||
| task: | ||
| scheduling: | ||
| pool: | ||
| size: 3 | ||
| shutdown: | ||
| await-termination: true | ||
| await-termination-period: 30s |
There was a problem hiding this comment.
스케줄러 종료 대기 시간이 짧아 종료 시 배치 중단 위험이 있습니다.
Line 23-24의 await-termination-period: 30s는 PaymentRecoveryScheduler(외부 API/락/반복), InterviewSchedulerService(Redis scan + DB batch) 작업 특성상 부족할 수 있습니다. 종료 중 작업이 끊기면 결제 복구/정합성 목표에 직접적인 리스크가 생깁니다. Line 17의 shutdown phase timeout도 함께 같은 수준으로 맞추는 게 안전합니다.
🔧 제안 설정 (종료 대기 시간 정렬)
lifecycle:
- timeout-per-shutdown-phase: 60s
+ timeout-per-shutdown-phase: ${SPRING_SHUTDOWN_PHASE_TIMEOUT:10m}
task:
scheduling:
pool:
size: 3
shutdown:
await-termination: true
- await-termination-period: 30s
+ await-termination-period: ${SPRING_SCHEDULER_AWAIT_TERMINATION:10m}📝 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
| lifecycle: | |
| timeout-per-shutdown-phase: 60s | |
| task: | |
| scheduling: | |
| pool: | |
| size: 3 | |
| shutdown: | |
| await-termination: true | |
| await-termination-period: 30s | |
| lifecycle: | |
| timeout-per-shutdown-phase: ${SPRING_SHUTDOWN_PHASE_TIMEOUT:10m} | |
| task: | |
| scheduling: | |
| pool: | |
| size: 3 | |
| shutdown: | |
| await-termination: true | |
| await-termination-period: ${SPRING_SCHEDULER_AWAIT_TERMINATION:10m} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/resources/application.yml` around lines 16 - 24, Increase the
scheduler shutdown wait windows to avoid interrupting long-running batch/IO
work: raise task.scheduling.shutdown.await-termination-period and align
lifecycle.timeout-per-shutdown-phase to the same larger value (e.g., several
minutes) so PaymentRecoveryScheduler and InterviewSchedulerService have enough
time to finish Redis scans, DB batches, external API/lock operations; update the
YAML keys lifecycle.timeout-per-shutdown-phase and
task.scheduling.shutdown.await-termination-period accordingly and keep
await-termination: true.
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/test/java/com/samhap/kokomen/payment/service/WebhookServiceTest.java (1)
53-225: 🧹 Nitpick | 🔵 Trivial토스 조회 실패 예외 경로 테스트를 1개 추가해 주세요.
getPayment예외 시 조기 반환하는 분기가 핵심 안정성 경로라, “토큰 미지급 + 상태 불변(또는 기대 상태)” 검증 케이스가 있으면 회귀 방지에 도움이 됩니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/com/samhap/kokomen/payment/service/WebhookServiceTest.java` around lines 53 - 225, Add a new unit test that simulates tosspaymentsClient.getPayment throwing an exception and asserts that webhookService.handlePaymentStatusChanged returns/aborts without granting tokens or changing payment state: create and save a TosspaymentsPayment (use TosspaymentsPaymentFixtureBuilder) in a pre-existing state (e.g., NEED_CANCEL or CONNECTION_TIMEOUT), stub tosspaymentsClient.getPayment to throw (for that paymentKey), call webhookService.handlePaymentStatusChanged with a matching WebhookPayload, then verify tosspaymentsPaymentRepository.findByPaymentKey still reports the original PaymentState, tokenRepository.findByMemberIdAndType shows no token count change, and tokenPurchaseRepository.findAll() remains empty. Ensure the test references the exact paymentKey used and covers the exception branch of getPayment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/main/java/com/samhap/kokomen/payment/service/PaymentRecoveryService.java`:
- Around line 33-34: Replace plain paymentKey usage in the logging calls inside
PaymentRecoveryService with masked values by calling
PaymentKeyMasker.mask(paymentKey); specifically update any
log.info/log.warn/log.error invocations that format paymentKey (e.g., the lines
that currently log "복구 스킵 - 이미 종료 상태: paymentKey={}, state={}" and similar
messages referencing paymentKey and payment.getState()) to pass
PaymentKeyMasker.mask(paymentKey) instead of paymentKey so all occurrences
(including the other spots mentioned) uniformly redact the key.
- Around line 37-43: The current catch in PaymentRecoveryService around
tosspaymentsClient.getPayment(paymentKey) treats all exceptions as retryable;
update the try-catch to specifically catch HttpClientErrorException (4xx) and
handle it as a terminal CLIENT_BAD_REQUEST recovery state (e.g., mark the
recovery/result as CLIENT_BAD_REQUEST and do not return for retry), while
keeping a separate catch for other exceptions (5xx/network) that log and allow
retry as before; reference tosspaymentsClient.getPayment, the try-catch block in
PaymentRecoveryService, and ensure any existing flow that records recovery
status is set to CLIENT_BAD_REQUEST when catching HttpClientErrorException.
In `@src/main/java/com/samhap/kokomen/payment/service/WebhookService.java`:
- Around line 71-99: The grantPurchasedTokens call in WebhookService is
vulnerable to a race between existsByPaymentKey and the actual token grant; wrap
the call to tokenFacadeService.grantPurchasedTokens(...) in a try/catch that
catches DataIntegrityViolationException and treats it as an idempotent success
(log the paymentKey masked and return) following the same pattern used in
TokenFacadeService.purchaseTokens (lines ~60-65); ensure the catch behavior
mirrors PaymentRecoveryService's handling so duplicate grants do not throw or
retry incorrectly while preserving normal error propagation for other
exceptions.
In `@src/main/java/com/samhap/kokomen/token/service/TokenFacadeService.java`:
- Around line 53-55: The log statements in TokenFacadeService that print the raw
paymentKey when tokenPurchaseService.existsByPaymentKey(request.paymentKey())
returns true (and the similar log at the later check around the other return)
should use the masking utility; replace direct use of request.paymentKey() in
the log calls with PaymentKeyMasker.mask(request.paymentKey()) so all
occurrences consistently mask the payment key before logging.
- Around line 60-64: The catch block in TokenFacadeService around
grantPurchasedTokens currently treats any DataIntegrityViolationException as a
duplicate payment; change it to detect whether the exception is caused by the
unique constraint on payment_key (uk_token_purchase_payment_key) and only
swallow/log as idempotent in that specific case, while rethrowing or propagating
other DataIntegrityViolationException causes (such as FK or NOT NULL
violations); update the catch to inspect the root cause/SQLException SQLState or
constraint name, keep the existing log message for the unique-key path and
include the exception details, and rethrow (or throw a wrapped runtime
exception) for non-unique-constraint violations so real errors are not hidden.
---
Outside diff comments:
In `@src/test/java/com/samhap/kokomen/payment/service/WebhookServiceTest.java`:
- Around line 53-225: Add a new unit test that simulates
tosspaymentsClient.getPayment throwing an exception and asserts that
webhookService.handlePaymentStatusChanged returns/aborts without granting tokens
or changing payment state: create and save a TosspaymentsPayment (use
TosspaymentsPaymentFixtureBuilder) in a pre-existing state (e.g., NEED_CANCEL or
CONNECTION_TIMEOUT), stub tosspaymentsClient.getPayment to throw (for that
paymentKey), call webhookService.handlePaymentStatusChanged with a matching
WebhookPayload, then verify tosspaymentsPaymentRepository.findByPaymentKey still
reports the original PaymentState, tokenRepository.findByMemberIdAndType shows
no token count change, and tokenPurchaseRepository.findAll() remains empty.
Ensure the test references the exact paymentKey used and covers the exception
branch of getPayment.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d012b02b-0e3a-4b06-9ec9-2cd3909ffb61
📒 Files selected for processing (7)
src/main/java/com/samhap/kokomen/payment/controller/WebhookController.javasrc/main/java/com/samhap/kokomen/payment/domain/TosspaymentsPayment.javasrc/main/java/com/samhap/kokomen/payment/service/PaymentRecoveryService.javasrc/main/java/com/samhap/kokomen/payment/service/WebhookService.javasrc/main/java/com/samhap/kokomen/payment/tool/PaymentKeyMasker.javasrc/main/java/com/samhap/kokomen/token/service/TokenFacadeService.javasrc/test/java/com/samhap/kokomen/payment/service/WebhookServiceTest.java
Comment on lines
+33
to
+34
| log.info("복구 스킵 - 이미 종료 상태: paymentKey={}, state={}", paymentKey, payment.getState()); | ||
| return; |
There was a problem hiding this comment.
복구 로그에 paymentKey 평문 노출이 남아 있습니다.
이 PR의 다른 결제 경로와 동일하게 PaymentKeyMasker.mask(...)를 사용해 주세요.
🔐 제안 수정
+import com.samhap.kokomen.payment.tool.PaymentKeyMasker;
@@
- if (payment.isTerminal()) {
- log.info("복구 스킵 - 이미 종료 상태: paymentKey={}, state={}", paymentKey, payment.getState());
+ String maskedPaymentKey = PaymentKeyMasker.mask(paymentKey);
+ if (payment.isTerminal()) {
+ log.info("복구 스킵 - 이미 종료 상태: paymentKey={}, state={}", maskedPaymentKey, payment.getState());
return;
}
@@
- log.warn("토스 결제 조회 실패 - paymentKey={}, 다음 주기에 재시도", paymentKey, e);
+ log.warn("토스 결제 조회 실패 - paymentKey={}, 다음 주기에 재시도", maskedPaymentKey, e);
@@
- log.info("복구 진행 - paymentKey={}, 내부상태={}, 토스상태={}", paymentKey, payment.getState(), tossStatus);
+ log.info("복구 진행 - paymentKey={}, 내부상태={}, 토스상태={}", maskedPaymentKey, payment.getState(), tossStatus);Also applies to: 41-42, 46-47, 52-58, 67-68, 79-80, 100-101
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/com/samhap/kokomen/payment/service/PaymentRecoveryService.java`
around lines 33 - 34, Replace plain paymentKey usage in the logging calls inside
PaymentRecoveryService with masked values by calling
PaymentKeyMasker.mask(paymentKey); specifically update any
log.info/log.warn/log.error invocations that format paymentKey (e.g., the lines
that currently log "복구 스킵 - 이미 종료 상태: paymentKey={}, state={}" and similar
messages referencing paymentKey and payment.getState()) to pass
PaymentKeyMasker.mask(paymentKey) instead of paymentKey so all occurrences
(including the other spots mentioned) uniformly redact the key.
Comment on lines
+37
to
+43
| TosspaymentsPaymentResponse tossResponse; | ||
| try { | ||
| tossResponse = tosspaymentsClient.getPayment(paymentKey); | ||
| } catch (Exception e) { | ||
| log.warn("토스 결제 조회 실패 - paymentKey={}, 다음 주기에 재시도", paymentKey, e); | ||
| return; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect TosspaymentsClient to understand exception handling
cat -n src/main/java/com/samhap/kokomen/payment/external/TosspaymentsClient.java
# Also check PaymentRecoveryService around the mentioned lines
head -n 60 src/main/java/com/samhap/kokomen/payment/service/PaymentRecoveryService.javaRepository: samhap-soft/kokomen-backend
Length of output: 4715
🏁 Script executed:
#!/bin/bash
# Check PaymentState enum
rg -A 30 "enum PaymentState|class PaymentState" src/main/java/com/samhap/kokomen/payment/domain/
# Check if PaymentKeyMasker exists
find src -name "*Masker*" -o -name "*paymentKey*" | head -20
rg -l "PaymentKeyMasker"
# Check payment object type and its methods
rg -A 5 "class TosspaymentsPayment|void updateState"Repository: samhap-soft/kokomen-backend
Length of output: 11480
🏁 Script executed:
#!/bin/bash
# Check PaymentKeyMasker implementation
cat -n src/main/java/com/samhap/kokomen/payment/tool/PaymentKeyMasker.java
# Check if there are other places handling TossPayments exceptions
rg -B 2 -A 5 "HttpClientErrorException|RestClientException" src/main/java/com/samhap/kokomen/payment/
# Check if there's logic to distinguish between CLIENT_BAD_REQUEST and SERVER_BAD_REQUEST
rg -B 3 -A 3 "CLIENT_BAD_REQUEST|SERVER_BAD_REQUEST"Repository: samhap-soft/kokomen-backend
Length of output: 14512
🏁 Script executed:
#!/bin/bash
# Verify the exception type hierarchy and confirm RestClient behavior
# Check imports in PaymentFacadeService to confirm exception types
head -n 30 src/main/java/com/samhap/kokomen/payment/service/PaymentFacadeService.java
# Check how payment.updateState is being called in the recovery service context
rg -B 2 -A 2 "payment.updateState" src/main/java/com/samhap/kokomen/payment/service/PaymentRecoveryService.java
# Verify PaymentRecoveryService around the area to understand context better
sed -n '30,60p' src/main/java/com/samhap/kokomen/payment/service/PaymentRecoveryService.javaRepository: samhap-soft/kokomen-backend
Length of output: 4146
4xx 응답은 재시도 불가능한 종결 상태로 처리해야 합니다.
현재 코드가 모든 예외를 동일하게 처리하면서 404(결제 미존재), 400(유효하지 않은 결제키) 등 4xx 에러도 다음 주기에 무한 재시도하게 됩니다. HttpClientErrorException으로 4xx를 구분하여 CLIENT_BAD_REQUEST 종결 상태로 전환하고, 5xx/네트워크 오류만 재시도 대상으로 분리해 주세요.
🔧 제안 수정
+import org.springframework.web.client.HttpClientErrorException;
+import org.springframework.web.client.RestClientException;
@@
TosspaymentsPaymentResponse tossResponse;
try {
tossResponse = tosspaymentsClient.getPayment(paymentKey);
- } catch (Exception e) {
- log.warn("토스 결제 조회 실패 - paymentKey={}, 다음 주기에 재시도", paymentKey, e);
+ } catch (HttpClientErrorException e) {
+ payment.updateState(PaymentState.CLIENT_BAD_REQUEST);
+ log.warn("토스 결제 조회 실패(4xx, 종결) - paymentKey={}, status={}",
+ PaymentKeyMasker.mask(paymentKey), e.getStatusCode(), e);
+ return;
+ } catch (RestClientException e) {
+ log.warn("토스 결제 조회 실패(재시도) - paymentKey={}", PaymentKeyMasker.mask(paymentKey), e);
return;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/com/samhap/kokomen/payment/service/PaymentRecoveryService.java`
around lines 37 - 43, The current catch in PaymentRecoveryService around
tosspaymentsClient.getPayment(paymentKey) treats all exceptions as retryable;
update the try-catch to specifically catch HttpClientErrorException (4xx) and
handle it as a terminal CLIENT_BAD_REQUEST recovery state (e.g., mark the
recovery/result as CLIENT_BAD_REQUEST and do not return for retry), while
keeping a separate catch for other exceptions (5xx/network) that log and allow
retry as before; reference tosspaymentsClient.getPayment, the try-catch block in
PaymentRecoveryService, and ensure any existing flow that records recovery
status is set to CLIENT_BAD_REQUEST when catching HttpClientErrorException.
Comment on lines
+71
to
+99
| if (tokenFacadeService.existsByPaymentKey(payment.getPaymentKey())) { | ||
| payment.updateState(PaymentState.COMPLETED); | ||
|
|
||
| TokenPurchase tokenPurchase = TokenPurchase.builder() | ||
| .memberId(memberId) | ||
| .paymentKey(payment.getPaymentKey()) | ||
| .orderId(payment.getOrderId()) | ||
| .totalAmount(payment.getTotalAmount()) | ||
| .orderName(payment.getOrderName()) | ||
| .productName(metadata.productName()) | ||
| .purchaseCount(tokenCount) | ||
| .unitPrice(metadata.unitPrice()) | ||
| .paymentMethod(data.method()) | ||
| .easyPayProvider(data.easyPay() != null ? data.easyPay().provider() : null) | ||
| .build(); | ||
| tokenFacadeService.grantPurchasedTokens(tokenPurchase, tokenCount); | ||
|
|
||
| log.info("웹훅으로 토큰 지급 완료 - memberId: {}, paymentKey: {}, tokenCount: {}", | ||
| memberId, payment.getPaymentKey(), tokenCount); | ||
| log.info("웹훅 스킵 - 이미 토큰 지급됨: paymentKey={}", PaymentKeyMasker.mask(payment.getPaymentKey())); | ||
| return; | ||
| } | ||
|
|
||
| payment.validateTosspaymentsResult( | ||
| tossResponse.paymentKey(), tossResponse.orderId(), tossResponse.totalAmount()); | ||
|
|
||
| PurchaseMetadata metadata = parseMetadata(payment.getMetadata()); | ||
| Long memberId = payment.getMemberId(); | ||
| int tokenCount = metadata.count(); | ||
|
|
||
| payment.updateState(PaymentState.COMPLETED); | ||
|
|
||
| TokenPurchase tokenPurchase = TokenPurchase.builder() | ||
| .memberId(memberId) | ||
| .paymentKey(payment.getPaymentKey()) | ||
| .orderId(payment.getOrderId()) | ||
| .totalAmount(payment.getTotalAmount()) | ||
| .orderName(payment.getOrderName()) | ||
| .productName(metadata.productName()) | ||
| .purchaseCount(tokenCount) | ||
| .unitPrice(metadata.unitPrice()) | ||
| .paymentMethod(tossResponse.method()) | ||
| .easyPayProvider(tossResponse.easyPay() != null ? tossResponse.easyPay().provider() : null) | ||
| .build(); | ||
| tokenFacadeService.grantPurchasedTokens(tokenPurchase, tokenCount); | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for grantPurchasedTokens calls and existing exception handling
rg -n "grantPurchasedTokens\(" src/main/java/com/samhap/kokomen/ -A 2 -B 2 || echo "No matches found"Repository: samhap-soft/kokomen-backend
Length of output: 2709
🏁 Script executed:
#!/bin/bash
# Check for DataIntegrityViolationException handling in the codebase
rg -n "DataIntegrityViolationException" src/main/java/com/samhap/kokomen/ || echo "No DataIntegrityViolationException usage found"Repository: samhap-soft/kokomen-backend
Length of output: 745
🏁 Script executed:
#!/bin/bash
# Get the structure and current state of WebhookService.java around the lines in question
cat -n src/main/java/com/samhap/kokomen/payment/service/WebhookService.java | sed -n '65,105p' || echo "File not found"Repository: samhap-soft/kokomen-backend
Length of output: 2138
🏁 Script executed:
#!/bin/bash
# Check if TokenFacadeService has any unique constraint related code or documentation
fd -t f "TokenFacadeService.java" src/ && cat -n src/main/java/com/samhap/kokomen/token/service/TokenFacadeService.java | head -100 || echo "File not found"Repository: samhap-soft/kokomen-backend
Length of output: 5300
🏁 Script executed:
#!/bin/bash
# Check PaymentRecoveryService for similar patterns
fd -t f "PaymentRecoveryService.java" src/ && rg -n "grantPurchasedTokens" src/main/java/com/samhap/kokomen/payment/service/PaymentRecoveryService.java -A 5 -B 5 || echo "File not found"Repository: samhap-soft/kokomen-backend
Length of output: 689
grantPurchasedTokens 호출에 race condition 방어 로직이 필요합니다.
existsByPaymentKey는 체크 시점과 실제 지급 시점 사이의 race condition을 막지 못합니다. WebhookService와 PaymentRecoveryService에서 grantPurchasedTokens를 호출할 때 DataIntegrityViolationException을 포착하여 멱등하게 처리해야 합니다. TokenFacadeService의 purchaseTokens 메서드(60-65줄)에서 이미 구현된 패턴을 따르세요.
WebhookService.java (98줄)
+import org.springframework.dao.DataIntegrityViolationException;
@@
- tokenFacadeService.grantPurchasedTokens(tokenPurchase, tokenCount);
+ try {
+ tokenFacadeService.grantPurchasedTokens(tokenPurchase, tokenCount);
+ } catch (DataIntegrityViolationException e) {
+ log.info("웹훅 스킵 - 중복 지급 감지: paymentKey={}",
+ PaymentKeyMasker.mask(payment.getPaymentKey()));
+ return;
+ }PaymentRecoveryService.java (98줄)
+import org.springframework.dao.DataIntegrityViolationException;
@@
- tokenFacadeService.grantPurchasedTokens(tokenPurchase, tokenCount);
+ try {
+ tokenFacadeService.grantPurchasedTokens(tokenPurchase, tokenCount);
+ } catch (DataIntegrityViolationException e) {
+ log.info("복구 스킵 - 중복 지급 감지: paymentKey={}", payment.getPaymentKey());
+ return;
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/com/samhap/kokomen/payment/service/WebhookService.java` around
lines 71 - 99, The grantPurchasedTokens call in WebhookService is vulnerable to
a race between existsByPaymentKey and the actual token grant; wrap the call to
tokenFacadeService.grantPurchasedTokens(...) in a try/catch that catches
DataIntegrityViolationException and treats it as an idempotent success (log the
paymentKey masked and return) following the same pattern used in
TokenFacadeService.purchaseTokens (lines ~60-65); ensure the catch behavior
mirrors PaymentRecoveryService's handling so duplicate grants do not throw or
retry incorrectly while preserving normal error propagation for other
exceptions.
Comment on lines
+53
to
+55
| if (tokenPurchaseService.existsByPaymentKey(request.paymentKey())) { | ||
| log.info("토큰이 이미 지급됨 (웹훅 처리) - memberId: {}, paymentKey: {}", memberId, request.paymentKey()); | ||
| return; |
There was a problem hiding this comment.
신규 로그에서 paymentKey가 평문으로 노출됩니다.
동일 PR에서 마스킹 유틸을 도입했으므로, 이 구간도 PaymentKeyMasker.mask(...)로 통일해 주세요.
🔐 제안 수정
+import com.samhap.kokomen.payment.tool.PaymentKeyMasker;
@@
- log.info("토큰이 이미 지급됨 (웹훅 처리) - memberId: {}, paymentKey: {}", memberId, request.paymentKey());
+ log.info("토큰이 이미 지급됨 (웹훅 처리) - memberId: {}, paymentKey: {}",
+ memberId, PaymentKeyMasker.mask(request.paymentKey()));
@@
- log.info("토큰이 이미 지급됨 (race condition) - memberId: {}, paymentKey: {}", memberId, request.paymentKey());
+ log.info("토큰이 이미 지급됨 (race condition) - memberId: {}, paymentKey: {}",
+ memberId, PaymentKeyMasker.mask(request.paymentKey()));Also applies to: 63-64
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/com/samhap/kokomen/token/service/TokenFacadeService.java`
around lines 53 - 55, The log statements in TokenFacadeService that print the
raw paymentKey when
tokenPurchaseService.existsByPaymentKey(request.paymentKey()) returns true (and
the similar log at the later check around the other return) should use the
masking utility; replace direct use of request.paymentKey() in the log calls
with PaymentKeyMasker.mask(request.paymentKey()) so all occurrences consistently
mask the payment key before logging.
Comment on lines
+60
to
+64
| try { | ||
| grantPurchasedTokens(tokenPurchase, tokenCount); | ||
| } catch (DataIntegrityViolationException e) { | ||
| log.info("토큰이 이미 지급됨 (race condition) - memberId: {}, paymentKey: {}", memberId, request.paymentKey()); | ||
| return; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find src/main/resources/db/migration -name "*.sql" | head -20Repository: samhap-soft/kokomen-backend
Length of output: 1445
🏁 Script executed:
cat src/main/java/com/samhap/kokomen/token/service/TokenFacadeService.java | head -100Repository: samhap-soft/kokomen-backend
Length of output: 4529
🏁 Script executed:
rg -n "token_purchase" src/main/resources/db/migration -A 5 -B 2Repository: samhap-soft/kokomen-backend
Length of output: 2770
🏁 Script executed:
rg -n "DataIntegrityViolationException" src/main/java/com/samhap/kokomen/token/service/ -A 5 -B 2Repository: samhap-soft/kokomen-backend
Length of output: 2089
DataIntegrityViolationException 전체를 중복 지급으로 간주하면 실제 장애를 숨길 수 있습니다.
token_purchase 테이블은 payment_key 유니크 제약(uk_token_purchase_payment_key) 외에도 member_id FK, NOT NULL 제약 등 여러 제약 조건을 가지고 있습니다. 현재 구현은 모든 DataIntegrityViolationException을 중복 지급으로 처리하므로, FK 위반이나 다른 제약 위반으로 인한 실제 오류까지 숨길 수 있습니다.
payment_key 유니크 제약 위반만 멱등 케이스로 처리하고, 나머지는 재던지도록 분기해 주세요.
🔧 제안 수정
try {
grantPurchasedTokens(tokenPurchase, tokenCount);
} catch (DataIntegrityViolationException e) {
- log.info("토큰이 이미 지급됨 (race condition) - memberId: {}, paymentKey: {}", memberId, request.paymentKey());
- return;
+ if (isDuplicatePaymentKeyViolation(e)) {
+ log.info("토큰이 이미 지급됨 (race condition) - memberId: {}, paymentKey: {}", memberId, request.paymentKey());
+ return;
+ }
+ throw e;
}
+
+private boolean isDuplicatePaymentKeyViolation(DataIntegrityViolationException e) {
+ Throwable root = org.springframework.core.NestedExceptionUtils.getMostSpecificCause(e);
+ return root != null && root.getMessage() != null
+ && root.getMessage().contains("uk_token_purchase_payment_key");
+}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/com/samhap/kokomen/token/service/TokenFacadeService.java`
around lines 60 - 64, The catch block in TokenFacadeService around
grantPurchasedTokens currently treats any DataIntegrityViolationException as a
duplicate payment; change it to detect whether the exception is caused by the
unique constraint on payment_key (uk_token_purchase_payment_key) and only
swallow/log as idempotent in that specific case, while rethrowing or propagating
other DataIntegrityViolationException causes (such as FK or NOT NULL
violations); update the catch to inspect the root cause/SQLException SQLState or
constraint name, keep the existing log message for the unique-key path and
include the exception details, and rethrow (or throw a wrapped runtime
exception) for non-unique-constraint violations so real errors are not hidden.
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.
closed #343
closed #340
closed #338
작업 내용
스크린샷
참고 사항