refactor : Notification Outbox 기반 FCM 발송 안정화#100
Open
Juhye0k wants to merge 33 commits into
Open
Conversation
Main <- Dev 반영
dev 변경사항 적용
main에 반영
Contributor
WalkthroughFCM 알림 전송을 아웃박스 기반으로 바꾸고, 만료된 최대 집중 세션 종료 시 outbox 적재를 연결했습니다. Resilience4j 기반 전송 보호와 재시도 정책이 추가되었고, CI/CD 워크플로우도 조정됐습니다. ChangesFCM 아웃박스 전송 흐름
CI/CD 워크플로우 업데이트
Sequence Diagram(s)sequenceDiagram
participant StudySessionService
participant NotificationOutboxCommandService
participant NotificationOutboxWorkerService
participant FcmSendGuard
participant NotificationRetryPolicy
StudySessionService->>NotificationOutboxCommandService: createMaxFocusOutbox(...)
NotificationOutboxWorkerService->>NotificationOutboxCommandService: markProcessing(outboxId, workerId, now)
NotificationOutboxWorkerService->>FcmSendGuard: send(outbox)
alt 전송 성공
NotificationOutboxWorkerService->>NotificationOutboxCommandService: deleteAfterSendSuccess(outboxId)
else 전송 실패
NotificationOutboxWorkerService->>NotificationRetryPolicy: decide(...)
NotificationOutboxWorkerService->>NotificationOutboxCommandService: applyRetryDecision(outboxId, decision)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (9)
src/main/java/com/gpt/geumpumtabackend/fcm/sender/FcmSendGuard.java (1)
36-42: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value불필요한 재던지기 catch 블록을 정리할 수 있습니다.
catch (FirebaseMessagingException e) { throw e; }와catch (RuntimeException e) { throw e; }는 그대로 다시 던지기만 하므로 동작상 무의미합니다.executeCallable이 던질 수 있는 checked 예외만 래핑하면 되므로 아래처럼 단순화할 수 있습니다.RequestNotPermitted/CallNotPermittedException은RuntimeException이라 그대로 전파되는 동작은 유지됩니다.♻️ 제안 리팩터
try { return circuitBreaker.executeCallable(() -> fcmOutboxSender.send(outbox)); } catch (FirebaseMessagingException e) { throw e; - } catch (RuntimeException e) { - throw e; } catch (Exception e) { throw new IllegalStateException(e); }🤖 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/gpt/geumpumtabackend/fcm/sender/FcmSendGuard.java` around lines 36 - 42, Simplify the exception handling in FcmSendGuard.executeCallable by removing the redundant catch blocks for FirebaseMessagingException and RuntimeException that only rethrow the same exception. Keep only the checked-exception handling path, wrapping other checked exceptions in IllegalStateException while preserving direct propagation of runtime failures such as RequestNotPermitted and CallNotPermittedException.src/main/java/com/gpt/geumpumtabackend/fcm/outbox/NotificationOutboxWorkerService.java (1)
78-85: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
decideUnexpected에 캡처된now대신 새LocalDateTime.now()를 전달합니다.다른 분기(Line 63, 67, 71)는 모두 Line 51에서 캡처한
now를 사용하지만, 여기서만LocalDateTime.now()를 다시 호출합니다(Line 82). 동작 차이는 미미하지만 일관성을 위해 캡처된now를 사용하는 편이 좋습니다.♻️ 제안 리팩터
NotificationRetryDecision decision = notificationRetryPolicy.decideUnexpected( e, outbox.getRetryCount(), - LocalDateTime.now() + now );🤖 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/gpt/geumpumtabackend/fcm/outbox/NotificationOutboxWorkerService.java` around lines 78 - 85, The unexpected-exception branch in NotificationOutboxWorkerService.handleNotificationRetry uses a fresh LocalDateTime.now() when calling notificationRetryPolicy.decideUnexpected, unlike the other retry branches that reuse the captured now value. Update this branch to pass the already captured now variable so all retry decisions in the method use the same timestamp consistently.src/main/java/com/gpt/geumpumtabackend/fcm/domain/NotificationOutboxStatus.java (1)
8-8: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
SENT상태가 실제로 사용되지 않습니다.엔티티에는
markProcessing/scheduleRetry/markDead/markCancelled전이 메서드만 있고SENT로 전이하는 경로가 없으며, 전송 성공 시에는deleteAfterSendSuccess로 행을 삭제합니다. 따라서SENT는 어디에서도 설정되지 않는 죽은 상태값입니다. 감사/추적 목적으로 보존 후SENT전이를 추가할지, 아니면 enum에서 제거할지 의도를 명확히 해주세요.🤖 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/gpt/geumpumtabackend/fcm/domain/NotificationOutboxStatus.java` at line 8, The NotificationOutboxStatus enum contains a dead SENT state that is never reached by the current transition methods. Decide whether NotificationOutboxStatus should support SENT by adding a transition from the outbox entity/service flow that can set it on successful send, or remove SENT from the enum if deleteAfterSendSuccess is the intended success path; update the related transition methods like markProcessing, scheduleRetry, markDead, and markCancelled accordingly so the status model matches actual behavior.src/main/java/com/gpt/geumpumtabackend/fcm/outbox/NotificationOutboxCommandService.java (1)
18-18: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value클래스 레벨은
@Transactional(readOnly = true)로 두고 쓰기 메서드에만@Transactional을 부여하세요.현재 클래스 전체가 쓰기 트랜잭션입니다. 가이드라인에 따라 클래스 레벨은 읽기 전용을 기본으로 하고, 변경 메서드에만 쓰기 트랜잭션을 명시하는 것이 일관적입니다.
As per coding guidelines: "Use class-level
@Transactional(readOnly = true) and add@Transactionalto write methods".🤖 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/gpt/geumpumtabackend/fcm/outbox/NotificationOutboxCommandService.java` at line 18, Change NotificationOutboxCommandService to use class-level `@Transactional`(readOnly = true) as the default, then add `@Transactional` only on the write method(s) in that class. Locate the service by its class name and update the transaction annotation at the class declaration, keeping any mutating operations explicitly annotated for write transactions while leaving non-mutating methods under the read-only default.Source: Coding guidelines
src/main/java/com/gpt/geumpumtabackend/study/service/StudySessionService.java (1)
120-128: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win만료 세션 적재 루프는 적절하나 호출 빈도/예외 전파를 함께 검토하세요.
이 루프 자체는 정상입니다. 다만
createMaxFocusOutbox내부에서 발생할 수 있는 트랜잭션 롤백 표시 문제(해당 파일 코멘트 참조)가 이 배치 전체 커밋에 영향을 줍니다. 또한 스케줄러가 1초 주기(fixedRate = 1000)로findAllByStatusAndStartTimeBefore를 호출하므로 데이터 증가 시 매 초 조회 부하가 누적됩니다. 주기 완화나 인덱스(status,start_time) 점검을 권장합니다.🤖 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/gpt/geumpumtabackend/study/service/StudySessionService.java` around lines 120 - 128, The expired-session loop in StudySessionService is fine, but the surrounding batch flow needs two fixes: handle createMaxFocusOutbox failures so a rollback-only mark in NotificationOutboxCommandService does not poison the whole scheduled commit, and reduce the load from the 1-second fixedRate scan. Review the scheduled method that calls findAllByStatusAndStartTimeBefore and the createMaxFocusOutbox path, then make outbox failures isolated/explicitly handled while keeping the session loop behavior intact. Also relax the scheduler frequency or confirm indexing on status and start_time to avoid growing query cost.src/main/java/com/gpt/geumpumtabackend/study/scheduler/MaxFocusStudyScheduler.java (1)
16-16: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value더 이상 사용되지 않는
studyProperties필드 제거를 검토하세요.리팩터링으로
maxFocusHours조회가StudySessionService로 이동하면서 이 스케줄러에서studyProperties는 더 이상 사용되지 않습니다.♻️ 제안 수정
private final StudySessionService studySessionService; - private final StudyProperties studyProperties;🤖 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/gpt/geumpumtabackend/study/scheduler/MaxFocusStudyScheduler.java` at line 16, `MaxFocusStudyScheduler` contains an unused `studyProperties` field that is no longer needed after `maxFocusHours` lookup moved to `StudySessionService`. Remove the `studyProperties` member from the scheduler class and update any constructor or dependency injection wiring in `MaxFocusStudyScheduler` so it no longer expects that dependency. Keep the scheduler focused on its current responsibilities and verify no remaining references to `studyProperties` exist in this class.build.gradle (1)
82-87: 📐 Maintainability & Code Quality | 🔵 Trivial
resilience4j-spring-boot3의존성만 선언하여 중복 제거하세요.
resilience4j-spring-boot3:2.3.0는 Circuit Breaker, Rate Limiter 및 Micrometer 통합에 필요한 모든 핵심 모듈을 전이적으로 포함하므로, 하기의 명시적 의존성 선언은 불필요하고 중복입니다.Spring Boot 3.5.6 과의 호환성 역시 2.3.0 이 Spring Boot 3.x 를 대상으로 하므로 문제없습니다.
다음 코드를 삭제하여 빌드 속도를 개선하고 의존성 그래프를 단순화하세요:
삭제 대상 코드 (build.gradle 82-87 행)
implementation 'io.github.resilience4j:resilience4j-circuitbreaker:2.3.0' implementation 'io.github.resilience4j:resilience4j-ratelimiter:2.3.0' implementation 'io.github.resilience4j:resilience4j-micrometer:2.3.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 `@build.gradle` around lines 82 - 87, The dependency block in build.gradle is redundant because resilience4j-spring-boot3 already brings in the Circuit Breaker, Rate Limiter, and Micrometer support transitively. Keep only the resilience4j-spring-boot3 declaration and remove the explicit resilience4j-circuitbreaker, resilience4j-ratelimiter, and resilience4j-micrometer entries from the Circuit Breaker section to simplify the dependency graph.src/test/java/com/gpt/geumpumtabackend/unit/fcm/outbox/NotificationRetryPolicyTest.java (1)
120-131: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
decideUnexpected의 maxRetry 도달 시 DEAD 경계 케이스가 누락되었습니다.
decide에 대해서는 maxRetry DEAD 케이스를 검증하지만(maxRetryGoesDead),decideUnexpected의 동일한 경계 분기(currentRetryCount >= maxRetry→dead)는 검증되지 않습니다.🤖 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/java/com/gpt/geumpumtabackend/unit/fcm/outbox/NotificationRetryPolicyTest.java` around lines 120 - 131, `decideUnexpected` is missing coverage for the maxRetry dead-path boundary. Add a test in `NotificationRetryPolicyTest` alongside `unexpectedErrorRetriesWithUnexpectedDelay` that calls `retryPolicy.decideUnexpected(...)` with a current retry count at or beyond maxRetry and asserts it returns `NotificationRetryDecisionType.DEAD` (matching the existing `decide` maxRetry dead-case behavior), using `NotificationRetryPolicy` and `NotificationRetryDecision` to verify the boundary branch.src/test/java/com/gpt/geumpumtabackend/unit/fcm/outbox/NotificationOutboxErrorClassifierTest.java (1)
51-57: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value매핑되지 않은 코드의 default(UNKNOWN) 경계 케이스 추가를 고려해 주세요.
null은 검증하지만 switch의default -> UNKNOWN경로(예: 매핑되지 않은MessagingErrorCode)는 검증되지 않습니다. 경계 케이스 커버리지를 보강하면 좋습니다.🤖 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/java/com/gpt/geumpumtabackend/unit/fcm/outbox/NotificationOutboxErrorClassifierTest.java` around lines 51 - 57, 현재 NotificationOutboxErrorClassifierTest의 nullErrorCode만 검증하고 있어, NotificationErrorClassifier.classify의 switch default -> UNKNOWN 경로가 빠져 있습니다. classifier.classify에 매핑되지 않은 MessagingErrorCode를 넣었을 때 NotificationSendFailureType.UNKNOWN이 반환되는 경계 케이스 테스트를 추가해, nullErrorCode와 함께 default 분기까지 커버하도록 보강하세요.
🤖 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 @.github/workflows/prod-cd.yml:
- Around line 144-145: The diagnostic output in the workflow is leaking
sensitive database credentials because the current grep on SPRING_DATASOURCE
also matches the password variable. Update the runtime datasource debug step in
the prod-cd workflow so it only prints non-sensitive keys such as URL and
username, or explicitly filters out SPRING_DATASOURCE_PASSWORD when using docker
exec env. Keep the existing health-check diagnostics, but make the output safe
by narrowing the pattern or excluding secret-bearing environment variables.
- Around line 81-102: The .env.runtime generation in the prod-cd workflow is
vulnerable to shell/template injection because unquoted heredoc content is
populated directly with `${{ secrets.* }}` and `${{ vars.* }}` values. Fix the
`cat > .env.runtime <<EOF` block by moving these values into workflow `env:`
entries and writing the file with a quoted heredoc in the deployment step so the
shell does not perform command substitution on injected content. Keep the
existing variable names (`APP_IMAGE`, `MYSQL_DATABASE`, `SPRING_DATASOURCE_URL`,
etc.) but ensure the heredoc is treated as literal text.
In `@src/main/java/com/gpt/geumpumtabackend/fcm/domain/NotificationOutbox.java`:
- Around line 15-16: `NotificationOutbox`의 id 생성 전략이 다른 엔티티들과 다르게 생략되어 있어
Hibernate 6에서 매핑 충돌이 날 수 있습니다. `NotificationOutbox` 클래스의 `id` 필드에 대한
`@GeneratedValue`를 `User`, `RefreshToken`, `StudySession`과 동일하게
`GenerationType.IDENTITY`를 명시하도록 수정해 생성 전략을 일관되게 맞추세요.
- Line 11: The NotificationOutbox entity currently exposes a public no-args
constructor via `@NoArgsConstructor`, which violates the entity guideline. Update
the Lombok annotation on NotificationOutbox to use `@NoArgsConstructor`(access =
PROTECTED) so the default constructor is restricted while still allowing the
internal createMaxFocusNotification factory to instantiate it.
In
`@src/main/java/com/gpt/geumpumtabackend/fcm/outbox/NotificationOutboxCommandService.java`:
- Around line 30-35: `createMaxFocusOutbox` 에서
`userSessionService.findActiveSession` 호출 시 발생하는 `BusinessException` 이 현재 트랜잭션을
rollback-only 로 마킹하므로, 단순히 catch 후 return 하지 말고 세션 조회를 예외 비발생 방식으로 바꾸거나 별도
트랜잭션으로 분리하세요. `NotificationOutboxCommandService.createMaxFocusOutbox` 기준으로
`findActiveSession` 을 `Optional<UserSession>` 반환으로 변경하는 방안, 또는
`UserSessionService.findActiveSession` 에 `Propagation.REQUIRES_NEW` 적용, 혹은
`BusinessException` 에 대한 `noRollbackFor` 설정 중 하나로 트랜잭션 커밋 시
`UnexpectedRollbackException` 이 나지 않게 수정하세요.
In
`@src/main/java/com/gpt/geumpumtabackend/fcm/outbox/NotificationOutboxWorkerService.java`:
- Around line 50-88: The current NotificationOutboxWorkerService.processOne flow
can leave records stuck in NotificationOutboxStatus.PROCESSING if the worker
dies or deleteAfterSendSuccess fails, because findDueOutboxes only picks up
PENDING and RETRY_SCHEDULED items. Add a stale-recovery/reaper path that scans
PROCESSING outboxes whose lastAttemptAt exceeds a timeout (for example, 10
minutes) and moves them back to RETRY_SCHEDULED or PENDING using
NotificationOutboxCommandService before normal dispatch continues. Use the
existing markProcessing, lastAttemptAt, lockedBy, and
applyRetryDecision/deleteAfterSendSuccess symbols to wire the recovery into the
outbox lifecycle cleanly.
In
`@src/test/java/com/gpt/geumpumtabackend/unit/fcm/outbox/NotificationOutboxErrorClassifierTest.java`:
- Around line 12-14: The test class NotificationOutboxErrorClassifierTest should
extend BaseUnitTest to match the unit test convention for unit/**/*Test.java.
Update the class declaration so it inherits from BaseUnitTest while keeping the
existing NotificationOutboxErrorClassifier setup and any JUnit 5, Mockito, or
AssertJ usage unchanged.
In
`@src/test/java/com/gpt/geumpumtabackend/unit/fcm/outbox/NotificationRetryPolicyTest.java`:
- Around line 21-23: NotificationRetryPolicyTest is missing the required
BaseUnitTest inheritance. Update the NotificationRetryPolicyTest class
declaration to extend BaseUnitTest so it follows the unit test convention used
in this test path; keep the existing NotificationRetryPolicy setup and test
methods unchanged.
---
Nitpick comments:
In `@build.gradle`:
- Around line 82-87: The dependency block in build.gradle is redundant because
resilience4j-spring-boot3 already brings in the Circuit Breaker, Rate Limiter,
and Micrometer support transitively. Keep only the resilience4j-spring-boot3
declaration and remove the explicit resilience4j-circuitbreaker,
resilience4j-ratelimiter, and resilience4j-micrometer entries from the Circuit
Breaker section to simplify the dependency graph.
In
`@src/main/java/com/gpt/geumpumtabackend/fcm/domain/NotificationOutboxStatus.java`:
- Line 8: The NotificationOutboxStatus enum contains a dead SENT state that is
never reached by the current transition methods. Decide whether
NotificationOutboxStatus should support SENT by adding a transition from the
outbox entity/service flow that can set it on successful send, or remove SENT
from the enum if deleteAfterSendSuccess is the intended success path; update the
related transition methods like markProcessing, scheduleRetry, markDead, and
markCancelled accordingly so the status model matches actual behavior.
In
`@src/main/java/com/gpt/geumpumtabackend/fcm/outbox/NotificationOutboxCommandService.java`:
- Line 18: Change NotificationOutboxCommandService to use class-level
`@Transactional`(readOnly = true) as the default, then add `@Transactional` only on
the write method(s) in that class. Locate the service by its class name and
update the transaction annotation at the class declaration, keeping any mutating
operations explicitly annotated for write transactions while leaving
non-mutating methods under the read-only default.
In
`@src/main/java/com/gpt/geumpumtabackend/fcm/outbox/NotificationOutboxWorkerService.java`:
- Around line 78-85: The unexpected-exception branch in
NotificationOutboxWorkerService.handleNotificationRetry uses a fresh
LocalDateTime.now() when calling notificationRetryPolicy.decideUnexpected,
unlike the other retry branches that reuse the captured now value. Update this
branch to pass the already captured now variable so all retry decisions in the
method use the same timestamp consistently.
In `@src/main/java/com/gpt/geumpumtabackend/fcm/sender/FcmSendGuard.java`:
- Around line 36-42: Simplify the exception handling in
FcmSendGuard.executeCallable by removing the redundant catch blocks for
FirebaseMessagingException and RuntimeException that only rethrow the same
exception. Keep only the checked-exception handling path, wrapping other checked
exceptions in IllegalStateException while preserving direct propagation of
runtime failures such as RequestNotPermitted and CallNotPermittedException.
In
`@src/main/java/com/gpt/geumpumtabackend/study/scheduler/MaxFocusStudyScheduler.java`:
- Line 16: `MaxFocusStudyScheduler` contains an unused `studyProperties` field
that is no longer needed after `maxFocusHours` lookup moved to
`StudySessionService`. Remove the `studyProperties` member from the scheduler
class and update any constructor or dependency injection wiring in
`MaxFocusStudyScheduler` so it no longer expects that dependency. Keep the
scheduler focused on its current responsibilities and verify no remaining
references to `studyProperties` exist in this class.
In
`@src/main/java/com/gpt/geumpumtabackend/study/service/StudySessionService.java`:
- Around line 120-128: The expired-session loop in StudySessionService is fine,
but the surrounding batch flow needs two fixes: handle createMaxFocusOutbox
failures so a rollback-only mark in NotificationOutboxCommandService does not
poison the whole scheduled commit, and reduce the load from the 1-second
fixedRate scan. Review the scheduled method that calls
findAllByStatusAndStartTimeBefore and the createMaxFocusOutbox path, then make
outbox failures isolated/explicitly handled while keeping the session loop
behavior intact. Also relax the scheduler frequency or confirm indexing on
status and start_time to avoid growing query cost.
In
`@src/test/java/com/gpt/geumpumtabackend/unit/fcm/outbox/NotificationOutboxErrorClassifierTest.java`:
- Around line 51-57: 현재 NotificationOutboxErrorClassifierTest의 nullErrorCode만
검증하고 있어, NotificationErrorClassifier.classify의 switch default -> UNKNOWN 경로가 빠져
있습니다. classifier.classify에 매핑되지 않은 MessagingErrorCode를 넣었을 때
NotificationSendFailureType.UNKNOWN이 반환되는 경계 케이스 테스트를 추가해, nullErrorCode와 함께
default 분기까지 커버하도록 보강하세요.
In
`@src/test/java/com/gpt/geumpumtabackend/unit/fcm/outbox/NotificationRetryPolicyTest.java`:
- Around line 120-131: `decideUnexpected` is missing coverage for the maxRetry
dead-path boundary. Add a test in `NotificationRetryPolicyTest` alongside
`unexpectedErrorRetriesWithUnexpectedDelay` that calls
`retryPolicy.decideUnexpected(...)` with a current retry count at or beyond
maxRetry and asserts it returns `NotificationRetryDecisionType.DEAD` (matching
the existing `decide` maxRetry dead-case behavior), using
`NotificationRetryPolicy` and `NotificationRetryDecision` to verify the boundary
branch.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9f27e9d7-173e-4fce-8719-143713d540f6
📒 Files selected for processing (23)
.github/workflows/prod-cd.yml.github/workflows/prod-ci.ymlbuild.gradlesrc/main/java/com/gpt/geumpumtabackend/fcm/domain/NotificationOutbox.javasrc/main/java/com/gpt/geumpumtabackend/fcm/domain/NotificationOutboxStatus.javasrc/main/java/com/gpt/geumpumtabackend/fcm/outbox/NotificationOutboxCommandService.javasrc/main/java/com/gpt/geumpumtabackend/fcm/outbox/NotificationOutboxProperties.javasrc/main/java/com/gpt/geumpumtabackend/fcm/outbox/NotificationOutboxWorkerService.javasrc/main/java/com/gpt/geumpumtabackend/fcm/outbox/retry/NotificationOutboxErrorClassifier.javasrc/main/java/com/gpt/geumpumtabackend/fcm/outbox/retry/NotificationRetryDecision.javasrc/main/java/com/gpt/geumpumtabackend/fcm/outbox/retry/NotificationRetryDecisionType.javasrc/main/java/com/gpt/geumpumtabackend/fcm/outbox/retry/NotificationRetryPolicy.javasrc/main/java/com/gpt/geumpumtabackend/fcm/outbox/retry/NotificationSendFailureType.javasrc/main/java/com/gpt/geumpumtabackend/fcm/repository/NotificationOutboxRepository.javasrc/main/java/com/gpt/geumpumtabackend/fcm/sender/FcmMessageSender.javasrc/main/java/com/gpt/geumpumtabackend/fcm/sender/FcmOutboxSender.javasrc/main/java/com/gpt/geumpumtabackend/fcm/sender/FcmSendGuard.javasrc/main/java/com/gpt/geumpumtabackend/fcm/service/FcmMessageSender.javasrc/main/java/com/gpt/geumpumtabackend/fcm/service/FcmService.javasrc/main/java/com/gpt/geumpumtabackend/study/scheduler/MaxFocusStudyScheduler.javasrc/main/java/com/gpt/geumpumtabackend/study/service/StudySessionService.javasrc/test/java/com/gpt/geumpumtabackend/unit/fcm/outbox/NotificationOutboxErrorClassifierTest.javasrc/test/java/com/gpt/geumpumtabackend/unit/fcm/outbox/NotificationRetryPolicyTest.java
💤 Files with no reviewable changes (2)
- src/main/java/com/gpt/geumpumtabackend/fcm/service/FcmService.java
- src/main/java/com/gpt/geumpumtabackend/fcm/service/FcmMessageSender.java
Comment on lines
81
to
102
| cat > .env.runtime <<EOF | ||
| APP_IMAGE=${{ needs.build.outputs.image-tag }} | ||
|
|
||
| MYSQL_DATABASE=${{ vars.MYSQL_DATABASE }} | ||
| MYSQL_ROOT_PASSWORD=${{ secrets.MYSQL_ROOT_PASSWORD }} | ||
| MYSQL_USER=${{ secrets.MYSQL_USER }} | ||
| MYSQL_PASSWORD=${{ secrets.MYSQL_PASSWORD }} | ||
|
|
||
| SPRING_PROFILES_ACTIVE=${{ vars.SPRING_PROFILES_ACTIVE }} | ||
| TZ=${{ vars.TZ }} | ||
|
|
||
| WEB_PORT=${{ vars.WEB_PORT }} | ||
| MYSQL_PORT=${{ vars.MYSQL_PORT }} | ||
| REDIS_PORT=${{ vars.REDIS_PORT }} | ||
|
|
||
| SPRING_DATASOURCE_URL=${{ vars.SPRING_DATASOURCE_URL }} | ||
| SPRING_DATASOURCE_USERNAME=${{ secrets.MYSQL_USER }} | ||
| SPRING_DATASOURCE_PASSWORD=${{ secrets.MYSQL_PASSWORD }} | ||
|
|
||
| SPRING_DATA_REDIS_HOST=${{ vars.SPRING_DATA_REDIS_HOST }} | ||
| SPRING_DATA_REDIS_PORT=${{ vars.SPRING_DATA_REDIS_PORT }} | ||
| EOF |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
시크릿/변수를 ${{ }}로 직접 heredoc에 보간하면 명령 주입 위험이 있습니다.
<<EOF는 따옴표가 없는 heredoc이므로, Actions가 ${{ secrets.* }}/${{ vars.* }}를 치환한 뒤에도 셸이 결과 문자열에 대해 $(...)·백틱 명령 치환을 수행합니다. 시크릿/변수 값에 $(...)나 백틱이 포함되면 셀프호스티드 러너에서 임의 명령이 실행될 수 있습니다 (zizmor template-injection).
값을 env:로 전달하고 따옴표 heredoc(<<'EOF')으로 작성해 보간/치환을 차단하는 것을 권장합니다.
🔒 제안 패턴
- name: Create runtime env file
working-directory: /home/ubuntu/geumpumta
+ env:
+ APP_IMAGE: ${{ needs.build.outputs.image-tag }}
+ MYSQL_DATABASE: ${{ vars.MYSQL_DATABASE }}
+ MYSQL_ROOT_PASSWORD: ${{ secrets.MYSQL_ROOT_PASSWORD }}
+ MYSQL_USER: ${{ secrets.MYSQL_USER }}
+ MYSQL_PASSWORD: ${{ secrets.MYSQL_PASSWORD }}
+ # ... 나머지 vars/secrets 동일하게 매핑
run: |
- cat > .env.runtime <<EOF
- APP_IMAGE=${{ needs.build.outputs.image-tag }}
-
- MYSQL_DATABASE=${{ vars.MYSQL_DATABASE }}
- ...
- EOF
+ cat > .env.runtime <<'EOF'
+ APP_IMAGE=${APP_IMAGE}
+ MYSQL_DATABASE=${MYSQL_DATABASE}
+ ...
+ EOF참고: 따옴표 heredoc에서는
${VAR}가 셸에 의해 안전하게 확장되며 명령 치환은 발생하지 않습니다.
🧰 Tools
🪛 zizmor (1.26.1)
[info] 82-82: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[info] 84-84: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[info] 89-89: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[info] 90-90: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[info] 92-92: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[info] 93-93: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[info] 94-94: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[info] 96-96: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[info] 100-100: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[info] 101-101: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
🤖 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 @.github/workflows/prod-cd.yml around lines 81 - 102, The .env.runtime
generation in the prod-cd workflow is vulnerable to shell/template injection
because unquoted heredoc content is populated directly with `${{ secrets.* }}`
and `${{ vars.* }}` values. Fix the `cat > .env.runtime <<EOF` block by moving
these values into workflow `env:` entries and writing the file with a quoted
heredoc in the deployment step so the shell does not perform command
substitution on injected content. Keep the existing variable names (`APP_IMAGE`,
`MYSQL_DATABASE`, `SPRING_DATASOURCE_URL`, etc.) but ensure the heredoc is
treated as literal text.
Source: Linters/SAST tools
Comment on lines
+15
to
+16
| @Id @GeneratedValue | ||
| private Long id; |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# 기존 엔티티들의 `@GeneratedValue` 전략 확인
rg -nP -C2 '`@GeneratedValue`' --type=java src/main/javaRepository: Geumpumta/backend
Length of output: 5612
ID 생성 전략을 다른 엔티티와 일관되게 IDENTITY로 명시하세요.
프로젝트의 모든 기존 엔티티(User, RefreshToken, StudySession 등) 가 GenerationType.IDENTITY를 명시적으로 사용하는 반면, NotificationOutbox 는 전략이 생략되어 있습니다. Hibernate 6 환경에서 이는 시퀀스 기반 매핑으로 간주되어 DB 의 AUTO_INCREMENT 설정과 충돌할 수 있으므로, 아래 코드를 수정해 일관성을 맞춰야 합니다.
수정 대상
`@Id`
`@GeneratedValue`(strategy = GenerationType.IDENTITY)
private 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/gpt/geumpumtabackend/fcm/domain/NotificationOutbox.java`
around lines 15 - 16, `NotificationOutbox`의 id 생성 전략이 다른 엔티티들과 다르게 생략되어 있어
Hibernate 6에서 매핑 충돌이 날 수 있습니다. `NotificationOutbox` 클래스의 `id` 필드에 대한
`@GeneratedValue`를 `User`, `RefreshToken`, `StudySession`과 동일하게
`GenerationType.IDENTITY`를 명시하도록 수정해 생성 전략을 일관되게 맞추세요.
Comment on lines
+30
to
+35
| UserSession activeSession; | ||
| try { | ||
| activeSession = userSessionService.findActiveSession(userId); | ||
| } catch(BusinessException e) { | ||
| return; | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# findActiveSession의 트랜잭션/예외 동작 확인
ast-grep run --pattern 'findActiveSession($$$) { $$$ }' --lang java || true
rg -nP -C5 'findActiveSession' --type=java src/main/java/com/gpt/geumpumtabackend/tokenRepository: Geumpumta/backend
Length of output: 1896
🏁 Script executed:
#!/bin/bash
# Check UserSessionService class-level or method-level `@Transactional` annotation
cat -n src/main/java/com/gpt/geumpumtabackend/token/service/UserSessionService.java | head -120Repository: Geumpumta/backend
Length of output: 5307
findActiveSession 예외 처리로 인한 트랜잭션 rollback-only 마킹 및 배치 실패 위험
createMaxFocusOutbox 가 @Transactional 이며 findActiveSession 을 호출할 때, 세션이 없어 BusinessException 이 발생하면 Spring 트랜잭션 관리자는 해당 트랜잭션을 rollback-only 로 마킹합니다. 호출该方法이 예외를 catch 하여 무視하더라도 rollback-only 상태는 해제되지 않아, 최종 커밋 시 UnexpectedRollbackException 이 발생하여 만료 세션 종료 배치 전체가 실패합니다.
UserSessionService 클래스 수준에서 @Transactional(readOnly = true) 가 적용되어 있음에도 findActiveSession 는 별도의 변경 구문이 없어 읽기 전용으로 동작하지만, 런타임 예외 발생 시 트랜잭션 상태 마킹 메커니즘은 동일하게 적용됩니다.
해결 방안:
findActiveSession에서 예외를 발생하지 않고Optional<UserSession>을 반환하도록 변경하거나- 해당 조회 로직을
Propagation.REQUIRES_NEW로 분리하여 독립된 트랜잭션으로 처리하거나 @Transactional(noRollbackFor = BusinessException.class)설정을 검토하세요.
<details>
<summary>위험 코드 섹션</summary>
```java
// src/main/java/com/gpt/geumpumtabackend/fcm/outbox/NotificationOutboxCommandService.java:30-35
try {
activeSession = userSessionService.findActiveSession(userId); // 여기서 BusinessException 발생 시 Transaction 마킹됨
} catch(BusinessException e) {
return; // 마킹된 상태는 해제되지 않음. 이후 commit 시 UnexpectedRollbackException 발생
}🤖 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/gpt/geumpumtabackend/fcm/outbox/NotificationOutboxCommandService.java`
around lines 30 - 35, `createMaxFocusOutbox` 에서
`userSessionService.findActiveSession` 호출 시 발생하는 `BusinessException` 이 현재 트랜잭션을
rollback-only 로 마킹하므로, 단순히 catch 후 return 하지 말고 세션 조회를 예외 비발생 방식으로 바꾸거나 별도
트랜잭션으로 분리하세요. `NotificationOutboxCommandService.createMaxFocusOutbox` 기준으로
`findActiveSession` 을 `Optional<UserSession>` 반환으로 변경하는 방안, 또는
`UserSessionService.findActiveSession` 에 `Propagation.REQUIRES_NEW` 적용, 혹은
`BusinessException` 에 대한 `noRollbackFor` 설정 중 하나로 트랜잭션 커밋 시
`UnexpectedRollbackException` 이 나지 않게 수정하세요.
Comment on lines
+50
to
+88
| private void processOne(Long outboxId) { | ||
| LocalDateTime now = LocalDateTime.now(); | ||
| String workerId = notificationOutboxProperties.getWorker().getWorkerId(); | ||
|
|
||
| Optional<NotificationOutbox> processingOutbox = | ||
| notificationOutboxCommandService.markProcessing(outboxId, workerId, now); | ||
| if(processingOutbox.isEmpty()) { | ||
| return; | ||
| } | ||
| NotificationOutbox outbox = processingOutbox.get(); | ||
| try { | ||
| fcmSendGuard.send(outbox); | ||
| } catch (RequestNotPermitted e) { | ||
| NotificationRetryDecision decision = notificationRetryPolicy.decideRateLimited(now); | ||
| notificationOutboxCommandService.applyRetryDecision(outboxId, decision); | ||
| return; | ||
| } catch(CallNotPermittedException e) { | ||
| NotificationRetryDecision decision = notificationRetryPolicy.decideCircuitOpen(now); | ||
| notificationOutboxCommandService.applyRetryDecision(outboxId, decision); | ||
| return; | ||
| } catch (FirebaseMessagingException e) { | ||
| NotificationRetryDecision decision = notificationRetryPolicy.decide( | ||
| e, | ||
| outbox.getRetryCount(), | ||
| now | ||
| ); | ||
| notificationOutboxCommandService.applyRetryDecision(outboxId, decision); | ||
| return; | ||
| } catch(Exception e) { | ||
| NotificationRetryDecision decision = notificationRetryPolicy.decideUnexpected( | ||
| e, | ||
| outbox.getRetryCount(), | ||
| LocalDateTime.now() | ||
| ); | ||
| notificationOutboxCommandService.applyRetryDecision(outboxId, decision); | ||
| return; | ||
| } | ||
| notificationOutboxCommandService.deleteAfterSendSuccess(outboxId); | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
ast-grep outline src/main/java/com/gpt/geumpumtabackend/fcm/outbox/NotificationOutboxCommandService.java
rg -nP 'lastAttemptAt|lockedBy|PROCESSING' src/main/java/com/gpt/geumpumtabackend/fcm/domain/NotificationOutbox.javaRepository: Geumpumta/backend
Length of output: 707
PROCESSING 상태의 스테일 레코드를 복구하는 타임아웃 기반 회수 로직이 필수적입니다.
markProcessing 실행 시 lastAttemptAt와 lockedBy가 갱신되나, findDueOutboxes는 PENDING 및 RETRY_SCHEDULED 상태만 조회합니다. fcmSendGuard.send 성공 후 deleteAfterSendSuccess 실패 또는 워커 비정상 종료 시, 해당 레코드는 처리되지 않은 채 PROCESSING 상태로 영구히 고립됩니다.
NotificationOutboxStatus.PROCESSING 중 lastAttemptAt가 특정 시간 (예: 10 분) 을 초과한 레코드를 RETRY_SCHEDULED 또는 PENDING으로 복귀시키는 Stale Reaper 로직 추가를 필수적으로 요청합니다. 이는 시스템 안정성과 알림 전달 신뢰도를 위해 반드시 구현되어야 합니다.
🤖 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/gpt/geumpumtabackend/fcm/outbox/NotificationOutboxWorkerService.java`
around lines 50 - 88, The current NotificationOutboxWorkerService.processOne
flow can leave records stuck in NotificationOutboxStatus.PROCESSING if the
worker dies or deleteAfterSendSuccess fails, because findDueOutboxes only picks
up PENDING and RETRY_SCHEDULED items. Add a stale-recovery/reaper path that
scans PROCESSING outboxes whose lastAttemptAt exceeds a timeout (for example, 10
minutes) and moves them back to RETRY_SCHEDULED or PENDING using
NotificationOutboxCommandService before normal dispatch continues. Use the
existing markProcessing, lastAttemptAt, lockedBy, and
applyRetryDecision/deleteAfterSendSuccess symbols to wire the recovery into the
outbox lifecycle cleanly.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…Outbox.java Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…ationOutboxErrorClassifierTest.java Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…ationRetryPolicyTest.java Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/test/java/com/gpt/geumpumtabackend/unit/study/service/StudySessionServiceTest.java (1)
321-350: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win아웃박스 적재 경계 케이스도 같이 고정해 주세요.
지금 테스트는 만료 세션 1건에서
createMaxFocusOutbox(...)가 호출되는 정상 흐름만 검증합니다. 이번 변경의 핵심이 세션 만료 시 enqueue 부수효과라서,findAllByStatusAndStartTimeBefore(...)가 빈 목록을 반환할 때 호출이 없음을 최소 1건은 추가해 두는 편이 안전합니다. 가능하면 2건 이상일 때 세션별로 1회씩 호출되는지도 함께 확인해 주세요. As per coding guidelines,src/test/**/*.java: "verify normal/exception/boundary cases".🤖 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/java/com/gpt/geumpumtabackend/unit/study/service/StudySessionServiceTest.java` around lines 321 - 350, The expired-session test in StudySessionServiceTest only covers the happy path where one session is returned and createMaxFocusOutbox(...) is called once; add boundary coverage around endExpiredMaxFocusSessions() to verify no outbox call happens when findAllByStatusAndStartTimeBefore(...) returns an empty list, and also confirm multiple expired sessions each trigger one notification call per session. Use the existing studySessionService, studySessionRepository, and notificationOutboxCommandService mocks to locate and extend the assertions.Source: Coding guidelines
🤖 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.
Nitpick comments:
In
`@src/test/java/com/gpt/geumpumtabackend/unit/study/service/StudySessionServiceTest.java`:
- Around line 321-350: The expired-session test in StudySessionServiceTest only
covers the happy path where one session is returned and
createMaxFocusOutbox(...) is called once; add boundary coverage around
endExpiredMaxFocusSessions() to verify no outbox call happens when
findAllByStatusAndStartTimeBefore(...) returns an empty list, and also confirm
multiple expired sessions each trigger one notification call per session. Use
the existing studySessionService, studySessionRepository, and
notificationOutboxCommandService mocks to locate and extend the assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0181d8d5-206e-4b52-95ed-db95e469c993
📒 Files selected for processing (3)
.github/workflows/prod-cd.ymlsrc/main/java/com/gpt/geumpumtabackend/fcm/domain/NotificationOutbox.javasrc/test/java/com/gpt/geumpumtabackend/unit/study/service/StudySessionServiceTest.java
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/prod-cd.yml
- src/main/java/com/gpt/geumpumtabackend/fcm/domain/NotificationOutbox.java
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
2 participants
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.
🚀 1. 개요
FCM 알림 발송 로직을 기존 즉시 발송 방식에서 Notification Outbox 기반 비동기 발송 구조로 변경했습니다.
기존에는 최대 집중 시간 초과 세션을 종료하는 스케줄러에서 FCM을 즉시 발송했기 때문에, FCM 장애·쿼터 초과·토큰 오류 등이 발생했을 때 알림 실패 내역을 추적하거나 재시도하기 어려웠습니다.
이번 변경에서는 알림 발송 요청을 먼저
NotificationOutbox테이블에 저장하고, 별도 Worker가 주기적으로 처리하도록 분리했습니다. 또한 Resilience4j 기반 Circuit Breaker와 Rate Limiter를 적용하여 FCM 장애 상황에서도 백엔드 시스템이 과도하게 영향을 받지 않도록 개선했습니다.📝 2. 주요 변경 사항
1. Notification Outbox 도입
NotificationOutbox엔티티 추가알림 발송 대상, FCM 토큰 스냅샷, 제목, 본문, 이벤트 키 저장
eventKey에 unique 제약을 적용하여 동일 이벤트 중복 발송 방지상태값 추가
PENDINGPROCESSINGRETRY_SCHEDULEDCANCELLEDDEAD2. 최대 집중 시간 초과 알림 구조 변경
NotificationOutbox에 알림 이벤트 저장3. FCM 발송 Worker 추가
NotificationOutboxWorkerService추가PENDING,RETRY_SCHEDULED상태 중nextRetryAt이 지난 이벤트만 처리PROCESSING상태로 변경4. 재시도 정책 추가
NotificationRetryPolicy추가Firebase 오류 유형별 재시도 정책 분리
QUOTA_EXCEEDED: 쿼터 초과로 분류 후 지연 재시도UNAVAILABLE,INTERNAL: 일시 장애로 분류 후 재시도SENDER_ID_MISMATCH,THIRD_PARTY_AUTH_ERROR: Provider 설정 문제로 분류 후 긴 지연 재시도UNREGISTERED,INVALID_ARGUMENT: 유효하지 않은 토큰으로 판단하여 DEAD 처리5. Circuit Breaker / Rate Limiter 적용
Summary by CodeRabbit