Skip to content

Develop - #39

Merged
cfcromn merged 7 commits into
mainfrom
develop
Jun 7, 2026
Merged

cfcromn merged 7 commits into
mainfrom
develop

Conversation

@cfcromn

@cfcromn cfcromn commented Jun 7, 2026

Copy link
Copy Markdown
Contributor

✨ 작업 내용

이번 PR에서 어떤 작업을 했는지 간단히 요약해주세요.


🔍 리뷰 시 참고사항

  • 리뷰어가 알면 좋은 변경 이유, 배경, 고려했던 점 등을 적어주세요.

✅ 체크리스트

  • 문서(README, .env.example 등) 변경이 필요한 경우 작성 또는 수정했나요?
  • 작업한 코드가 정상적으로 동작하는 것을 직접 확인했나요?
  • 필요한 경우 테스트 코드를 작성하거나 수정했나요?
  • Merge 대상 브랜치를 올바르게 설정했나요?
  • PR에 관련 없는 작업이 포함되지 않았나요?
  • 적절한 라벨과 리뷰어를 설정했나요?

📎 관련 이슈(선택)

  • Close #

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request refactors the SSE subscription logic to use an event-driven approach by introducing AvatarSseSubscribedEvent and publishing it asynchronously upon subscription. It also simplifies connection tracking and cleanup logic, and reduces the heartbeat interval. The review feedback highlights a potential connection leak in the asynchronous onAvatarSseSubscribed event listener if a database error occurs, and suggests wrapping the logic in a runCatching block to guarantee proper cleanup.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment on lines 51 to +56
@Async
@TransactionalEventListener(phase = TransactionPhase.AFTER_COMMIT)
@EventListener
@Transactional(readOnly = true)
fun onAvatarSseSubscribed(event: AvatarSseSubscribedEvent) {
send(event.emitter, findAvatarChanges())
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

In onAvatarSseSubscribed, if findAvatarChanges() throws an exception (e.g., due to a database connection timeout or query failure), the exception will propagate out of the @Async method. Because send is never called, the cleanup method is never invoked, leaving the SseEmitter in the emitters list indefinitely (until it eventually times out, which is set to 30 minutes). This can leak connection slots and quickly exhaust maxConnections under database error conditions.

We should wrap the execution in a runCatching block and ensure cleanup is called with complete = true if an exception occurs.

    @Async
    @EventListener
    @Transactional(readOnly = true)
    fun onAvatarSseSubscribed(event: AvatarSseSubscribedEvent) {
        runCatching { findAvatarChanges() }
            .onSuccess { send(event.emitter, it) }
            .onFailure { e ->
                logger.error("Failed to send initial avatar changes", e)
                cleanup(event.emitter, "initial send failed", complete = true)
            }
    }

@cfcromn
cfcromn merged commit 63b7323 into main Jun 7, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant