[fix] Transactional Outbox 패턴 적용해서 알림 로직 수정#85
Conversation
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은 TodaySound 서비스의 핵심 기능인 피드 알림의 신뢰성 문제를 해결하기 위해 Transactional Outbox 패턴을 적용합니다. 기존에는 알림 전송(FCM)과 DB 저장 로직이 분리되어 있어 알림 유실, 중복, 또는 팬텀 알림과 같은 문제가 발생했습니다. 이제 Highlights
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 Results24 tests 12 ✅ 0s ⏱️ For more details on these failures, see this check. Results for commit aeec702. ♻️ 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 pull request is an excellent improvement that enhances the reliability of the notification system by applying the Transactional Outbox pattern. However, the implementation introduces a significant Denial of Service (DoS) risk in the scheduler and lacks necessary authentication for the internal alert endpoint. The scheduler performs synchronous network calls within a long-running database transaction, which can lead to connection pool exhaustion. Additionally, the internal endpoint for receiving alerts from the crawler does not verify the identity of the caller, potentially allowing unauthorized users to send notifications or create data. I have also included some suggestions for performance, readability, and consistency improvements in the review.
| @Transactional | ||
| public void processOutbox() { | ||
| List<NotificationOutbox> pending = | ||
| outboxRepository.findByStatusOrderByCreatedAtAsc(OutboxStatus.PENDING); | ||
|
|
||
| if (pending.isEmpty()) { | ||
| return; | ||
| } | ||
|
|
||
| log.info(BUSINESS, "Outbox 처리 시작 {}", kv("pendingCount", pending.size())); | ||
|
|
||
| for (NotificationOutbox outbox : pending) { | ||
| try { | ||
| User user = userRepository.findById(outbox.getUserId()).orElse(null); | ||
|
|
||
| if (user == null) { | ||
| // 탈퇴한 사용자: 재시도 없이 즉시 FAILED 처리 | ||
| log.warn(BUSINESS, "Outbox 전송 대상 사용자 없음 (탈퇴) {} {}", | ||
| kv("outboxId", outbox.getId()), | ||
| kv("userId", outbox.getUserId())); | ||
| outbox.incrementRetry(1); | ||
| continue; | ||
| } | ||
|
|
||
| fcmService.sendNotificationToUser(user, outbox.getTitle(), outbox.getBody()); |
There was a problem hiding this comment.
The processOutbox method is annotated with @Transactional and performs synchronous network calls to the FCM service within a loop. This is a major anti-pattern that can lead to database connection pool exhaustion, causing a Denial of Service (DoS) for the entire application. To remediate this, decouple the database transaction from the network call. Each outbox item should be processed in its own short-lived transaction, and the network call should occur outside of that transaction. Additionally, within this loop, there is an N+1 query issue with userRepository.findById() for each outbox item, which can further degrade performance when many notifications are processed. Consider fetching all necessary users in a single query using findAllById before the loop and storing them in a Map for efficient lookup.
| @PostMapping("/alerts") | ||
| @Transactional | ||
| public void createAlert(@RequestBody InternalAlertRequest request) { |
There was a problem hiding this comment.
The /internal/alerts endpoint lacks any visible authentication or authorization mechanism. While there is a consistency check between subscriptionId and userId, it does not verify that the caller is an authorized internal service (e.g., the crawler). If this endpoint is exposed, an attacker could send arbitrary push notifications to any user or create unauthorized summaries. Implement a secure authentication mechanism, such as an internal API key or mutual TLS, and ensure the endpoint is protected at both the infrastructure and application levels.
| @Column(nullable = false) | ||
| private LocalDateTime createdAt; |
There was a problem hiding this comment.
createdAt 필드를 LocalDateTime.now()로 수동 설정하고 있습니다. 프로젝트 내 다른 엔티티(User, Subscription 등)에서는 @CreationTimestamp나 @CreatedDate 같은 어노테이션을 사용하여 생성 시각을 자동으로 관리하고 있습니다.
코드의 일관성을 유지하고 프레임워크의 기능을 활용하기 위해, 이 엔티티에서도 어노테이션을 사용하여 createdAt을 자동으로 설정하는 것을 권장합니다. 이렇게 변경하면 create 팩토리 메서드에서 관련 코드를 제거할 수 있습니다.
| @Column(nullable = false) | |
| private LocalDateTime createdAt; | |
| @org.hibernate.annotations.CreationTimestamp | |
| @Column(nullable = false, updatable = false) | |
| private LocalDateTime createdAt; |
| outbox.body = body; | ||
| outbox.status = OutboxStatus.PENDING; | ||
| outbox.retryCount = 0; | ||
| outbox.createdAt = LocalDateTime.now(); |
There was a problem hiding this comment.
앞선 제안에 따라 createdAt 필드에 @CreationTimestamp 어노테이션을 적용하면, JPA가 엔티티를 저장할 때 자동으로 생성 시각을 주입해주므로 이 라인은 더 이상 필요하지 않습니다.
| log.warn(BUSINESS, "Outbox 전송 대상 사용자 없음 (탈퇴) {} {}", | ||
| kv("outboxId", outbox.getId()), | ||
| kv("userId", outbox.getUserId())); | ||
| outbox.incrementRetry(1); |
There was a problem hiding this comment.
outbox.incrementRetry(1) 호출은 "즉시 실패 처리"라는 의도를 코드만으로 파악하기 어렵게 만듭니다. 이 로직은 의도대로 동작하지만 가독성과 유지보수성을 위해 개선할 수 있습니다.
NotificationOutbox에 상태를 직접 FAILED로 설정하는 markAsFailed() 메서드를 추가하고, 여기서는 그 메서드를 호출하는 것이 더 명확합니다.
이를 위해서는 NotificationOutbox 엔티티에 다음과 같은 메서드 추가가 필요합니다.
// In NotificationOutbox.java
public void markAsFailed() {
this.status = OutboxStatus.FAILED;
}
There was a problem hiding this comment.
TableNameExtractorImpl.java을 추가한 이유가 뭘까요?
DatabaseManager에서 TableNameExtractor tableNameExtractor이 추가될 때 TableNameExtractor에 어떤걸 상속받을 지 몰라서 에러가 나는 것 같아요!!
2 participants
Related issue 🛠
어떤 변경사항이 있었나요?
Transactional Outbox 패턴
TodaySound 서비스는 "피드에 새 글이 올라오면 사용자 폰에 알림을 보내주는 것"이 핵심 기능임. 그런데 운영하다 보니 같은 피드를 구독한 사용자인데 어떤 사람은 알림이 오고 어떤 사람은 안 오는 현상이 발생함.
왜 이런 일이 벌어졌는가
기존 코드의 실행 순서가 이랬음
그리고 @transactional이 없었음. 즉 ①과 ②는 아무 관계 없는 독립된 두 작업임.
크롤러가 구독자 10명에 대해 이 API를 10번 호출하는데, 각 요청이 독립적으로 다른 지점에서 실패할 수 있음
이게 뒤섞여서 "어떤 핸드폰은 오고 어떤 핸드폰은 안 온다"는 현상이 나온 것임.
해결 과정
시도 1: 순서만 바꾸기 → 불완전
@transactional이 없으니 ①은 즉시 auto-commit됨. ②에서 FCM이 실패하면? → DB에는 데이터가 있는데 알림은 영영 안 감. 재시도 수단이 없음.
시도 2: @transactional + afterCommit() → 거의 해결, 하지만 한계
이러면 DB 커밋이 실패하면 FCM이 아예 실행 안 됨 → Phantom 문제 해결됨.
하지만: afterCommit() 실행 중에 서버가 죽거나 Firebase가 일시 장애이면? → 그 알림은 영구 유실됨. afterCommit()은 메모리에만 있는 콜백이라 서버가 재시작되면 사라짐.
시도 3: CDC (Debezium) → 검토했지만 부적합
CDC는 DB의 binlog를 읽어서 변경을 감지하는 방식인데, 상황에 부적합함
최종 해결: Transactional Outbox 패턴
핵심 아이디어:
왜 이게 모든 문제를 해결하는지 케이스별로 보면:
트레이드오프: 뭘 포기하고 뭘 얻었는가
포기한 것: 알림의 즉각성. 기존에는 요청 처리 중에 바로 FCM을 쐈는데, 이제는 스케줄러 주기(최대 5초) 만큼 지연이 생김.
얻은 것: 알림의 신뢰성. 모든 사용자에게 eventually 반드시 전달된다는 보장.
이건 의도된 트레이드오프. TodaySound는 실시간 메신저가 아니라 피드 알림 서비스. 5초 늦게 오는 게 문제가 아니라, 안 오는 게 문제임.
To Reviewers 📢
"알림 전송과 데이터 저장의 원자성을 DB 트랜잭션으로 보장하고, 제어할 수 없는 외부 시스템(Firebase)의 일시적 장애에 대한 복구 책임을 애플리케이션 내부로 가져오기 위해서 수정함."