Conversation
|
Warning Review limit reached
Next review available in: 39 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
Summary by CodeRabbit
WalkthroughSNS 기반 푸시 실패 처리를 SQS와 DLQ 기반 파이프라인으로 변경했습니다. Changes푸시 실패 처리 파이프라인
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (7)
AGENTS.md (2)
7-11: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value새 SQS 경로를 문서에 반영하십시오.
이 PR은
SqsHandler와events/sqs-push-failure.json을 추가합니다. 현재 설명은 SNS 기반 구조만 기술합니다.📝 제안 변경
-- `lambda/`: handlers for API Gateway, EventBridge, and SNS. +- `lambda/`: handlers for API Gateway, EventBridge, and SQS. - `service/`, `repository/`, `client/`: business logic, DynamoDB access, and AWS SDK providers. - `dto/`, `domain/`, `enums/`, `common/`, `util/`, `config/`: request models, persistence models, shared constants, utilities, and wiring. - `src/main/resources/logback.xml`: logging configuration. -- `events/`: SAM payloads for API Gateway, SNS, and EventBridge. +- `events/`: SAM payloads for API Gateway, SNS, SQS, and EventBridge.🤖 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 `@AGENTS.md` around lines 7 - 11, Update the AGENTS.md project structure description to document the new SQS path and events/sqs-push-failure.json payload, alongside the existing lambda handlers and event fixtures. Ensure the description reflects both SNS and SQS-based flows without removing the existing entries.
25-26: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value새 SQS 핸들러 실행 예시를 추가하십시오.
template.yaml의SnsHandlerFunction은 SQSPushFailureQueue로 전달되므로,events/sns-event-single.json예시는 실제 이벤트와 다릅니다.AGENTS.md25-26 번 줄에events/sqs-push-failure.json기반 예시와 SQS 핸들러 테스트 스크립트 경로도 함께 기록하세요. 기존test-sns-handler.sh는 SNS 이벤트 파일만 지원하므로 새 SQS 경로도 별도 스크립트 또는 인수 지원으로 다뤄야 합니다.🤖 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 `@AGENTS.md` around lines 25 - 26, Update the AGENTS.md local testing examples to use events/sqs-push-failure.json for the SQS PushFailureQueue flow, and document the corresponding SQS handler test script path. Add or extend the testing script so it supports invoking the SQS handler with that event, while preserving the existing SNS test example and behavior.template.yaml (3)
228-232: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win논리 ID
SnsHandlerFunction이 실제 SQS 핸들러와 일치하지 않습니다.
FunctionName과Handler는 SQS로 변경했지만, 리소스 논리 ID는SnsHandlerFunction으로 남아 있습니다. 이후 템플릿을 읽는 사람이 트리거 종류를 잘못 이해할 수 있습니다.논리 ID를 변경하면 CloudFormation이 리소스를 교체합니다. 이번 PR에서
FunctionName이 이미 변경되어 교체가 발생하므로, 같은 배포에서 논리 ID도 함께 정리하는 편이 비용이 낮습니다.또한
AGENTS.md25번 줄의sam local invoke SnsHandlerFunction예시와 새 이벤트 파일events/sqs-push-failure.json의 조합도 함께 갱신하십시오.♻️ 제안 변경
- SnsHandlerFunction: + SqsHandlerFunction: Type: AWS::Serverless::Function Properties: FunctionName: !Sub "sopt-push-notification-lambda-sqs-${Stage}" Handler: com.sopt.push.lambda.SqsHandler::handleRequest🤖 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 `@template.yaml` around lines 228 - 232, Rename the CloudFormation logical ID SnsHandlerFunction to reflect the SQS handler while preserving its current FunctionName and Handler settings. Update the AGENTS.md invocation example to use the new logical ID, and ensure it invokes the events/sqs-push-failure.json event fixture.
149-153: 📐 Maintainability & Code Quality | 🔵 TrivialDLQ 알람 추가를 검토하십시오.
PushFailureDeadLetterQueue는maxReceiveCount: 5초과 메시지를 받습니다. 현재 템플릿에는 DLQ 적재를 감지하는 알람이 없습니다. 메시지가 DLQ에 쌓여도 운영자가 인지하지 못합니다.
ApproximateNumberOfMessagesVisible메트릭에 대한 CloudWatch 알람을 추가하십시오. 이 파이프라인의 목적이 실패 추적이므로, DLQ 적재 자체가 중요한 신호입니다.🤖 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 `@template.yaml` around lines 149 - 153, Add a CloudWatch alarm for PushFailureDeadLetterQueue using the ApproximateNumberOfMessagesVisible metric, so any messages accumulating in the DLQ trigger operational notification. Configure the alarm consistently with existing template alarms and reference the queue’s name or dimensions correctly.
33-38: 🔒 Security & Privacy | 🔵 Trivial | ⚖️ Poor tradeoffSlack 웹훅 URL을 Lambda 환경 변수 평문으로 전달합니다.
NoEcho: true는 CloudFormation 스택 파라미터 출력만 가립니다. Lambda 환경 변수 값은 콘솔과lambda:GetFunctionConfiguration응답에서 평문으로 노출됩니다. Slack 인커밍 웹훅 URL은 그 자체가 인증 자격 증명입니다.가능하면 SSM Parameter Store SecureString 또는 Secrets Manager에 저장하고, 환경 변수에는 파라미터 이름만 전달하십시오. 이 경우
EnvConfig는 이름을 읽고SlackAlertService가 값을 조회하도록 변경해야 합니다.즉시 도입이 어려우면 최소한 실행 역할과
lambda:GetFunctionConfiguration권한 범위를 좁게 유지하십시오.위 판단은 "Do not commit AWS credentials, real device tokens, production ARNs, or local
params-dev.jsonfiles containing secrets; keep environment-specific values in AWS/SAM parameters" 가이드라인의 시크릿 취급 원칙에 근거합니다.Also applies to: 55-55
🤖 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 `@template.yaml` around lines 33 - 38, Stop passing SlackFailureWebhookUrl as a plaintext Lambda environment value. Store the webhook URL in SSM Parameter Store SecureString or Secrets Manager, pass only its parameter or secret name through the environment, and update EnvConfig and SlackAlertService to resolve the value at runtime using the Lambda execution role. Remove the direct webhook URL parameter flow while preserving Slack failure alert behavior.Source: Coding guidelines
src/main/java/com/sopt/push/lambda/SqsHandler.java (2)
119-143: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win17개 위치 인자 생성자 호출의 가독성이 낮습니다.
CreateHistoryDto생성자에 13개의null을 위치로 전달합니다. 인자 순서가 하나만 어긋나도 컴파일 오류 없이 잘못된 필드에 값이 저장됩니다.HistoryService.createLog가 이 값을 그대로 DynamoDB에 저장하므로 오류를 발견하기 어렵습니다.
CreateHistoryDto에 빌더 또는 실패 이력 전용 정적 팩터리 메서드를 추가하십시오. 예:CreateHistoryDto.pushFailure(id, userIds, messageIds).🤖 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/sopt/push/lambda/SqsHandler.java` around lines 119 - 143, CreateFailureLog의 위치 인자 기반 CreateHistoryDto 생성자를 제거하고, CreateHistoryDto에 실패 이력 전용 정적 팩터리 메서드(예: pushFailure) 또는 빌더를 추가해 필요한 id, 사용자 ID, 메시지 ID와 실패 상태·푸시 타입만 명시적으로 설정하도록 변경하십시오. createFailLog에서는 해당 명시적 생성 경로를 사용하고 기존 HistoryService.createLog 호출은 유지하십시오.
103-117: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win파싱 불가 메시지가 5회 재시도됩니다.
extractFailureMessage는objectMapper.readTree에서 예외를 던질 수 있습니다.handleRequest는 이 예외를 배치 실패로 보고합니다. 본문 형식이 잘못된 메시지는 재시도해도 절대 성공하지 않습니다.maxReceiveCount: 5에 도달할 때까지 5회 재처리되고, 그때마다notifyProcessingFailure가 Slack 알림을 보냅니다.파싱 오류처럼 재시도가 무의미한 오류는 배치 실패로 보고하지 않고 즉시 DLQ 대상으로 분류하거나, 로그와 1회 알림 후 메시지를 성공 처리하는 방식을 검토하십시오.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/sopt/push/lambda/SqsHandler.java` around lines 103 - 117, Update extractFailureMessage and its handleRequest caller so JSON parsing failures from record bodies are treated as non-retryable: classify the message for DLQ handling or, per the existing design, log and notify once before acknowledging it successfully. Ensure malformed messages do not propagate as batch failures or trigger repeated notifyProcessingFailure calls across retries.
🤖 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 `@events/sqs-push-failure.json`:
- Line 6: Update the sample payload values in the event body: replace the Token
value with an explicitly non-real example such as example-device-token, and
replace the Korean free-text MessageId with an identifier-formatted example
suitable for SqsHandler.extractFailureMessage and createFailLog.messageIds.
In `@src/main/java/com/sopt/push/lambda/SqsHandler.java`:
- Around line 97-101: Prevent duplicate failure histories on SQS retries by
making the failure-log write in processRecord idempotent, using the SQS
messageId or another deterministic identifier instead of UUID.randomUUID() and
Instant.now() as the record key. Update createFailLog and the underlying
HistoryService.createLog flow to reuse that deterministic key, while preserving
the existing cleanup and Slack notification behavior.
In `@src/main/java/com/sopt/push/service/SlackAlertService.java`:
- Around line 34-53: Update notifyPushFailure and notifyProcessingFailure to
build their field maps with LinkedHashMap in the declared insertion order
instead of Map.of, and add the required java.util.LinkedHashMap import. Preserve
the existing field names, values, and ordering shown in each method so
createPayload produces stable Slack field layouts.
- Around line 55-80: Update SlackAlertService.send() and the
SqsHandler.processRecord() alert flow so alerts are aggregated per SQS batch or
dispatched asynchronously instead of performing one blocking httpClient.send()
call per record. Ensure batch processing does not accumulate up to ten HTTP
request timeouts and preserves failure-log handling without duplicate records on
retry.
In `@template.yaml`:
- Around line 155-163: PushFailureQueue의 VisibilityTimeout을 연결된 함수 timeout의 6배
이상으로 조정하십시오. 현재 Function.Timeout이 60초이므로 VisibilityTimeout을 최소 360초로 설정하고, 기존 큐
처리 및 RedrivePolicy 설정은 유지하십시오.
---
Nitpick comments:
In `@AGENTS.md`:
- Around line 7-11: Update the AGENTS.md project structure description to
document the new SQS path and events/sqs-push-failure.json payload, alongside
the existing lambda handlers and event fixtures. Ensure the description reflects
both SNS and SQS-based flows without removing the existing entries.
- Around line 25-26: Update the AGENTS.md local testing examples to use
events/sqs-push-failure.json for the SQS PushFailureQueue flow, and document the
corresponding SQS handler test script path. Add or extend the testing script so
it supports invoking the SQS handler with that event, while preserving the
existing SNS test example and behavior.
In `@src/main/java/com/sopt/push/lambda/SqsHandler.java`:
- Around line 119-143: CreateFailureLog의 위치 인자 기반 CreateHistoryDto 생성자를 제거하고,
CreateHistoryDto에 실패 이력 전용 정적 팩터리 메서드(예: pushFailure) 또는 빌더를 추가해 필요한 id, 사용자 ID,
메시지 ID와 실패 상태·푸시 타입만 명시적으로 설정하도록 변경하십시오. createFailLog에서는 해당 명시적 생성 경로를 사용하고 기존
HistoryService.createLog 호출은 유지하십시오.
- Around line 103-117: Update extractFailureMessage and its handleRequest caller
so JSON parsing failures from record bodies are treated as non-retryable:
classify the message for DLQ handling or, per the existing design, log and
notify once before acknowledging it successfully. Ensure malformed messages do
not propagate as batch failures or trigger repeated notifyProcessingFailure
calls across retries.
In `@template.yaml`:
- Around line 228-232: Rename the CloudFormation logical ID SnsHandlerFunction
to reflect the SQS handler while preserving its current FunctionName and Handler
settings. Update the AGENTS.md invocation example to use the new logical ID, and
ensure it invokes the events/sqs-push-failure.json event fixture.
- Around line 149-153: Add a CloudWatch alarm for PushFailureDeadLetterQueue
using the ApproximateNumberOfMessagesVisible metric, so any messages
accumulating in the DLQ trigger operational notification. Configure the alarm
consistently with existing template alarms and reference the queue’s name or
dimensions correctly.
- Around line 33-38: Stop passing SlackFailureWebhookUrl as a plaintext Lambda
environment value. Store the webhook URL in SSM Parameter Store SecureString or
Secrets Manager, pass only its parameter or secret name through the environment,
and update EnvConfig and SlackAlertService to resolve the value at runtime using
the Lambda execution role. Remove the direct webhook URL parameter flow while
preserving Slack failure alert behavior.
🪄 Autofix
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 Plus
Run ID: fefa7bbd-0815-4112-aa59-9fd1e7932819
📒 Files selected for processing (7)
AGENTS.mdevents/sqs-push-failure.jsonsrc/main/java/com/sopt/push/config/AppFactory.javasrc/main/java/com/sopt/push/config/EnvConfig.javasrc/main/java/com/sopt/push/lambda/SqsHandler.javasrc/main/java/com/sopt/push/service/SlackAlertService.javatemplate.yaml
- cleanup 실패로 SQS 재시도될 때 실패 history가 최대 5번 중복 생성되는 문제를 줄임 - token이 이미 DB에 없는 경우는 userId 없이 실패 이력을 남기도록 처리
1 participant
Related Issue 🚀
Work Description ✏️
기존에 SNS를 통해 실패를 추적하는 구조였지만, record 처리 중 발생한 예외를 내부에서 catch하고 최종적으로 성공 응답을 반환하는 구조여서 개별 실패 이벤트 처리가 실패해도 SNS 입장에서는 Lambda 호출이 성공한 것으로 볼 수 있어, 재시도나 DLQ로 이어지지 않을 수 있었습니다.
따라서 실패 이벤트를 명시적으로 큐에 저장하고, 처리 실패한 메시지만 재시도/DLQ로 이동시키는 구조가 필요하다고 판단하여 SQS와 DLQ를 도입했습니다.
작업 내용
SNS -> Lambda에서SNS -> SQS -> Lambda로 변경기존 구조
변경된 구조