[FEAT/#245] SSE BackOff 적용 및 연결 상태 관리 리팩 - #312
Merged
Merged
Conversation
- `AuthManager`에 유저 로그인 상태(`isUserLoggedIn`) 및 강제 로그아웃 이벤트(`forceLogoutEvent`) 추가 - `AuthManagerImpl`에서 `AtomicBoolean`을 사용하여 중복 인증 실패 처리 방지 가드 구현 - `SseManager`가 `AuthManager`의 로그인 상태를 구독하여 연결을 자동으로 관리하도록 수정 (기존 ViewModel에서의 직접 호출 제거) - `TokenAuthenticator` 및 각 ViewModel(`Login`, `SignUp`, `Splash`, `MyPage`, `Withdraw`)에서 `SseManager` 의존성을 `AuthManager`로 교체 및 관련 메서드 호출 수정 - `MainActivity`에서 `authEvent` 대신 `forceLogoutEvent`를 구독하도록 변경
- [FEAT] Mutex를 활용하여 동시 토큰 재발급 요청을 처리하는 `TokenReissueUseCase` 추가 - [REFACTOR] `SplashViewModel`의 토큰 재발급 로직을 `TokenReissueUseCase`로 이관 - [REFACTOR] `TokenAuthenticator`에서 중복되는 재발급 로직을 `TokenReissueUseCase` 사용으로 수정하여 단순화
- SSE 연결 오류 시 HTTP 상태 코드를 전달할 수 있도록 `SseConnectionState.Error` 필드 추가 - `SseManager` 내 401 에러 감지 시 `TokenReissueUseCase`를 통한 토큰 재발급 처리 추가 - 포그라운드/백그라운드 및 로그인 상태를 고려한 SSE 연결 관리 로직 최적화 (Mutex 활용) - `MainActivity`의 생명주기 메서드명을 `onAppForegrounded`, `onAppBackgrounded`로 변경하여 의도 명확화 - `EventRepository` 및 `EventRemoteDataSource` 인터페이스에 `connectionState` 노출 관점 추가
- SSE 연결 상태(Error, Disconnected)에 따른 재연결 스케줄링 추가 - Exponential Backoff 및 Full Jitter 알고리즘을 적용한 지연 시간 계산 로직 도입 - 최대 재연결 시도 횟수(30회) 및 최대 지연 시간(120초) 제한 설정 - 토큰 재발급 또는 앱 상태(Background)에 따른 연결 관리 로직 보완 및 Coroutine Job 제어 추가
Walkthrough토큰 재발급 책임을 Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 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 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 |
seungjunGong
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
app/src/main/java/com/smashing/app/presentation/main/MainActivity.kt (1)
68-73:onDestroy()에서onAppBackgrounded()호출이 중복될 수 있습니다.
onStop()은 항상onDestroy()이전에 호출되므로,onDestroy()에서 다시onAppBackgrounded()를 호출하면 중복 실행됩니다.SseManager의 mutex 보호로 인해 기능상 문제는 없지만, 불필요한 코루틴 실행과 락 획득이 발생합니다.
onDestroy()의onAppBackgrounded()호출을 제거하는 것을 권장합니다.♻️ 중복 호출 제거 제안
override fun onDestroy() { super.onDestroy() - if (!isChangingConfigurations) { - sseManager.onAppBackgrounded() - } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/smashing/app/presentation/main/MainActivity.kt` around lines 68 - 73, onDestroy currently calls sseManager.onAppBackgrounded() guarded by isChangingConfigurations, but onStop already calls sseManager.onAppBackgrounded(), causing duplicate execution; remove the sseManager.onAppBackgrounded() call (and its isChangingConfigurations guard) from MainActivity.onDestroy() so the backgrounding logic only runs from onStop(), leaving onStop(), sseManager.onAppBackgrounded(), and isChangingConfigurations elsewhere unchanged.app/src/main/java/com/smashing/app/data/remote/datasource/impl/EventRemoteDataSourceImpl.kt (1)
85-90:onClosed에서 상태 업데이트 순서를 검토해 주세요.현재
eventSource를null로 설정한 후synchronized블록 밖에서_connectionState.value를 업데이트합니다.connect()가 동시에 호출되면Disconnected상태 설정 전에 새 연결이 시작될 수 있습니다.일관성을 위해
onFailure와 동일한 패턴(상태 업데이트 후synchronized블록 내에서 null 처리)을 고려해 보세요.♻️ 순서 일관성 제안
override fun onClosed(eventSource: EventSource) { + _connectionState.value = SseConnectionState.Disconnected synchronized(eventSourceLock) { this@EventRemoteDataSourceImpl.eventSource = null } - _connectionState.value = SseConnectionState.Disconnected }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/smashing/app/data/remote/datasource/impl/EventRemoteDataSourceImpl.kt` around lines 85 - 90, onClosed currently nulls this@EventRemoteDataSourceImpl.eventSource inside the synchronized(eventSourceLock) block and then updates _connectionState.value to SseConnectionState.Disconnected outside it, which allows connect() to race in and replace the source before the state flips; change the sequence to match onFailure: first set _connectionState.value = SseConnectionState.Disconnected, then inside synchronized(eventSourceLock) set this@EventRemoteDataSourceImpl.eventSource = null so the state and eventSource are updated atomically relative to connect()—update the onClosed implementation accordingly, referencing onClosed, eventSourceLock, this@EventRemoteDataSourceImpl.eventSource, _connectionState, onFailure, and connect().app/src/main/java/com/smashing/app/core/network/sse/SseManager.kt (2)
240-244: 타입 일관성 개선 고려
MAX_RECONNECT_DELAY_MS가Double(120_000.0)로 선언되어 있고min()결과를toLong()으로 변환하고 있습니다. 타입 일관성을 위해Long타입을 사용하고coerceAtMost를 활용하는 것이 더 명확합니다.♻️ 제안된 수정
private fun calculateReconnectDelayMs(attempt: Int): Long { - val exponentialDelay = MIN_RECONNECT_DELAY_MS * 2.0.pow(attempt.toDouble()) - val cappedDelay = min(MAX_RECONNECT_DELAY_MS, exponentialDelay).toLong() + val exponentialDelay = MIN_RECONNECT_DELAY_MS shl attempt + val cappedDelay = exponentialDelay.coerceAtMost(MAX_RECONNECT_DELAY_MS) return Random.nextLong(0L, cappedDelay + 1L) } companion object { private const val TAG = "SSE LOG" private const val AUTH_FAILURE_CODE = 401 private const val MIN_RECONNECT_DELAY_MS = 1_000L - private const val MAX_RECONNECT_DELAY_MS = 120_000.0 + private const val MAX_RECONNECT_DELAY_MS = 120_000L private const val MAX_RECONNECT_ATTEMPTS = 30 }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/smashing/app/core/network/sse/SseManager.kt` around lines 240 - 244, The function calculateReconnectDelayMs uses Double math and min() then toLong(), causing type inconsistency; change MAX_RECONNECT_DELAY_MS (and any related constants) to Long (e.g., 120_000L), compute exponentialDelay as Long (use shifting or Long-based calculation), then replace min(...) with kotlin's coerceAtMost on Long values (e.g., cappedDelay = exponentialDelay.coerceAtMost(MAX_RECONNECT_DELAY_MS)) and return Random.nextLong(0L, cappedDelay + 1L); update MIN_RECONNECT_DELAY_MS / MAX_RECONNECT_DELAY_MS types and any uses accordingly to maintain Long consistency.
111-128:tokenReissueUseCase호출 시previousAccessToken전달 고려
tokenReissueUseCase()를 파라미터 없이 호출하고 있습니다.TokenReissueUseCase의 double-check 최적화는previousAccessToken을 사용하여 이미 재발급된 토큰을 감지합니다. 현재 액세스 토큰을 전달하면TokenAuthenticator와의 동시 호출 시 불필요한 재발급 요청을 방지할 수 있습니다.♻️ 제안된 수정
private fun handleSseAuthFailure() { scope.launch { if (!tryStartReissue()) return@launch - val reissueResult = tokenReissueUseCase() + val currentAccessToken = tokenDataSource.getAccessToken() + val reissueResult = tokenReissueUseCase(previousAccessToken = currentAccessToken) mutex.withLock {이를 위해
LocalTokenDataSource를 생성자에 주입해야 합니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/smashing/app/core/network/sse/SseManager.kt` around lines 111 - 128, In handleSseAuthFailure(), pass the current stored access token into tokenReissueUseCase to enable its double-check optimization: inject LocalTokenDataSource into SseManager (add it to the constructor and store it) and call tokenReissueUseCase(previousAccessToken = localTokenDataSource.getAccessToken()) instead of tokenReissueUseCase() inside the launched coroutine; keep existing synchronization around tryStartReissue, mutex.withLock, isReissuingToken toggling, and the reconnect logic (reconnectAttempt reset, cancel reconnectJob, eventRepository.connect()) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/main/java/com/smashing/app/core/network/token/AuthManagerImpl.kt`:
- Around line 28-43: The transitions between onUserLoggedIn, onUserLoggedOut and
onAuthFailure are not atomic and a late onAuthFailure can flip _isUserLoggedIn
and emit _forceLogoutEvent for a new session; fix by introducing a single shared
lock/mutex and perform the state changes for authFailureHandled, _isUserLoggedIn
and the _forceLogoutEvent emission inside that lock in onUserLoggedIn,
onUserLoggedOut and onAuthFailure so the three methods are synchronized;
specifically ensure onAuthFailure uses the same lock when calling
authFailureHandled.compareAndSet(...), setting _isUserLoggedIn.value = false and
tryEmit(Unit), and that onUserLoggedIn sets authFailureHandled and
_isUserLoggedIn under the same lock to prevent races.
In
`@app/src/main/java/com/smashing/app/domain/usecase/auth/TokenReissueUseCase.kt`:
- Around line 37-49: The null-previousAccessToken path bypasses the
duplicate-reissue guard because isAlreadyReissued treats null/blank as "not
reissued"; update the logic so getReissuedAccessToken(previousAccessToken:
String?) (and/or isAlreadyReissued) first checks the
cached/lastReissuedAccessToken regardless of whether previousAccessToken is null
and returns that cached token when present, so concurrent invoke() calls
(including the SplashViewModel path) reuse an in-flight or already produced
token; ensure the mutex.withLock block still guards postTokenReissue() and that
postTokenReissue() sets the same cached lastReissuedAccessToken that
getReissuedAccessToken reads.
---
Nitpick comments:
In `@app/src/main/java/com/smashing/app/core/network/sse/SseManager.kt`:
- Around line 240-244: The function calculateReconnectDelayMs uses Double math
and min() then toLong(), causing type inconsistency; change
MAX_RECONNECT_DELAY_MS (and any related constants) to Long (e.g., 120_000L),
compute exponentialDelay as Long (use shifting or Long-based calculation), then
replace min(...) with kotlin's coerceAtMost on Long values (e.g., cappedDelay =
exponentialDelay.coerceAtMost(MAX_RECONNECT_DELAY_MS)) and return
Random.nextLong(0L, cappedDelay + 1L); update MIN_RECONNECT_DELAY_MS /
MAX_RECONNECT_DELAY_MS types and any uses accordingly to maintain Long
consistency.
- Around line 111-128: In handleSseAuthFailure(), pass the current stored access
token into tokenReissueUseCase to enable its double-check optimization: inject
LocalTokenDataSource into SseManager (add it to the constructor and store it)
and call tokenReissueUseCase(previousAccessToken =
localTokenDataSource.getAccessToken()) instead of tokenReissueUseCase() inside
the launched coroutine; keep existing synchronization around tryStartReissue,
mutex.withLock, isReissuingToken toggling, and the reconnect logic
(reconnectAttempt reset, cancel reconnectJob, eventRepository.connect())
unchanged.
In
`@app/src/main/java/com/smashing/app/data/remote/datasource/impl/EventRemoteDataSourceImpl.kt`:
- Around line 85-90: onClosed currently nulls
this@EventRemoteDataSourceImpl.eventSource inside the
synchronized(eventSourceLock) block and then updates _connectionState.value to
SseConnectionState.Disconnected outside it, which allows connect() to race in
and replace the source before the state flips; change the sequence to match
onFailure: first set _connectionState.value = SseConnectionState.Disconnected,
then inside synchronized(eventSourceLock) set
this@EventRemoteDataSourceImpl.eventSource = null so the state and eventSource
are updated atomically relative to connect()—update the onClosed implementation
accordingly, referencing onClosed, eventSourceLock,
this@EventRemoteDataSourceImpl.eventSource, _connectionState, onFailure, and
connect().
In `@app/src/main/java/com/smashing/app/presentation/main/MainActivity.kt`:
- Around line 68-73: onDestroy currently calls sseManager.onAppBackgrounded()
guarded by isChangingConfigurations, but onStop already calls
sseManager.onAppBackgrounded(), causing duplicate execution; remove the
sseManager.onAppBackgrounded() call (and its isChangingConfigurations guard)
from MainActivity.onDestroy() so the backgrounding logic only runs from
onStop(), leaving onStop(), sseManager.onAppBackgrounded(), and
isChangingConfigurations elsewhere unchanged.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 21166fe5-7eaa-4c79-aebc-8253785572b5
📒 Files selected for processing (18)
app/src/main/java/com/smashing/app/core/network/TokenAuthenticator.ktapp/src/main/java/com/smashing/app/core/network/sse/SseConnectionState.ktapp/src/main/java/com/smashing/app/core/network/sse/SseManager.ktapp/src/main/java/com/smashing/app/core/network/token/AuthManager.ktapp/src/main/java/com/smashing/app/core/network/token/AuthManagerImpl.ktapp/src/main/java/com/smashing/app/data/remote/datasource/api/EventRemoteDatasource.ktapp/src/main/java/com/smashing/app/data/remote/datasource/impl/EventRemoteDataSourceImpl.ktapp/src/main/java/com/smashing/app/data/repository/api/EventRepository.ktapp/src/main/java/com/smashing/app/data/repository/impl/AuthRepositoryImpl.ktapp/src/main/java/com/smashing/app/data/repository/impl/EventRepositoryImpl.ktapp/src/main/java/com/smashing/app/domain/usecase/auth/TokenReissueUseCase.ktapp/src/main/java/com/smashing/app/presentation/login/LoginViewModel.ktapp/src/main/java/com/smashing/app/presentation/main/MainActivity.ktapp/src/main/java/com/smashing/app/presentation/mypage/MyPageViewModel.ktapp/src/main/java/com/smashing/app/presentation/signup/SignUpViewModel.ktapp/src/main/java/com/smashing/app/presentation/splash/SplashViewModel.ktapp/src/main/java/com/smashing/app/presentation/withdraw/WithdrawViewModel.ktfastlane/notes/qa/debug.txt
- `handleRawEvent`, `parseEvent` 메서드 추출을 통한 관심사 분리 - `_events`를 `asSharedFlow()`로 캡슐화 - `@param:ApplicationScope` 어노테이션 타겟 명시 수정 - 불필요한 null 처리 및 흐름 제어 로직 간소화
- [feat] SSE용 OkHttpClient에 `TokenAuthenticator`를 추가하여 401 에러 시 자동 토큰 재발급 연동 - [refactor] `TokenReissueUseCase`의 중복 재발급 방지 로직을 단순화하고 `mutex` 기반 동기화로 변경 - [refactor] `SseManager` 내 직접적인 토큰 재발급 처리 로직을 제거하고, `Authenticator`를 통한 인증 흐름으로 통합 - [refactor] `EventRemoteDataSourceImpl`의 SSE 리스너를 내부 클래스로 분리하고, 현재 연결 소스(EventSource) 검증 로직 추가 - [style] `SseConnectionState` 모델의 프로퍼티 구성을 재연결 시도 횟수 및 지연 시간 중심으로 변경 - [chore] 재연결 관련 백오프 계산식의 타입 캐스팅 및 상수 값 정리
Member
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
app/src/main/java/com/smashing/app/domain/usecase/auth/TokenReissueUseCase.kt (1)
33-45: Mutex가 동시 실행은 막지만, 대기 중인 호출의 중복 재발급은 방지하지 못합니다.현재 구현에서 여러 호출이 동시에
invoke()를 호출하면:
- 첫 번째 호출이 락을 획득하고 재발급 성공
- 대기 중이던 두 번째 호출이 락을 획득하고 또다시 재발급 수행
Refresh token rotation을 사용하는 서버에서는 불필요한 API 호출과 토큰 충돌이 발생할 수 있습니다. 성공한 재발급 결과를 짧은 TTL로 캐싱하거나,
Deferred를 공유하여 대기 중인 호출이 동일 결과를 재사용하도록 개선을 권장합니다.♻️ Deferred 공유 방식 예시
`@Singleton` class TokenReissueUseCase `@Inject` constructor( private val authRepository: AuthRepository, private val tokenDataSource: LocalTokenDataSource, private val authManager: AuthManager, ) { private val mutex = Mutex() + private var inFlightReissue: Deferred<Result<String>>? = null suspend operator fun invoke(): Result<String> = mutex.withLock { + // 진행 중인 재발급이 있으면 결과 재사용 + inFlightReissue?.let { deferred -> + if (deferred.isActive) { + return@withLock deferred.await() + } + } + val refreshToken = tokenDataSource.getRefreshToken() ?: return handleFailure(IllegalStateException("Refresh token is null")) - return authRepository.postTokenReissue(PostTokenReissueRequest(refreshToken)) + val deferred = scope.async { + authRepository.postTokenReissue(PostTokenReissueRequest(refreshToken)) + .map { it.accessToken } + } + inFlightReissue = deferred + + return deferred.await() .onSuccess { Timber.tag(TAG).d("토큰 재발급 성공") } - .map { it.accessToken } .onFailure { error -> handleFailure(error) + }.also { + inFlightReissue = null } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/smashing/app/domain/usecase/auth/TokenReissueUseCase.kt` around lines 33 - 45, The invoke() in TokenReissueUseCase currently uses mutex.withLock but still allows queued callers to perform duplicate reissue calls; fix this by introducing a shared in-flight marker (e.g., a nullable Deferred<Result<String>> field like ongoingReissue) that you read/write under the existing mutex: if ongoingReissue exists return/await it, otherwise create a new async Deferred that executes authRepository.postTokenReissue(PostTokenReissueRequest(refreshToken)) (and maps to accessToken), store it in ongoingReissue, await its result for callers, and clear ongoingReissue on completion/failure; ensure tokenDataSource.getRefreshToken and error handling remain unchanged and access to ongoingReissue is protected by the same mutex.app/src/main/java/com/smashing/app/core/network/sse/SseManager.kt (1)
140-143: 최대 재시도 도달 시 사용자 알림이 누락되어 있습니다.PR 설명에서 "retry-limit user notification" UX 정책이 미결로 언급되어 있습니다. 현재는 로그만 남기고 재시도를 중단하는데, 사용자에게 연결 실패 상태를 알리는 이벤트 emit을 추가하는 것을 권장합니다.
이 기능을 추적할 새 이슈를 생성해 드릴까요?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/smashing/app/core/network/sse/SseManager.kt` around lines 140 - 143, When reconnectAttempt >= MAX_RECONNECT_ATTEMPTS in SseManager you currently only log and return; add a user-facing notification by emitting a connection-failure event before returning. Update the block that contains reconnectAttempt, MAX_RECONNECT_ATTEMPTS and Timber.tag(TAG).e to call the manager's event emitter (or add a new emitConnectionFailure/notifyConnectionFailed method on SseManager) and include contextual details (reconnectAttempt and MAX_RECONNECT_ATTEMPTS) in the event payload so listeners/UI can show a retry-limit notification to the user.app/src/main/java/com/smashing/app/data/remote/datasource/impl/EventRemoteDataSourceImpl.kt (1)
63-72:onEvent에서 lock 없이isCurrentSource를 호출하는 점이 다른 콜백과 불일치합니다.
onOpen,onFailure,onClosed는 모두synchronized(eventSourceLock)블록 내에서isCurrentSource체크를 수행하지만,onEvent는 락 없이 호출합니다.@Volatile덕분에 가시성은 보장되지만, 체크와 emit 사이에disconnect()가 호출되면 이미 해제된 연결의 이벤트가 emit될 수 있습니다.실제 영향은 미미하나(이미 수신된 이벤트의 benign emit), 일관성을 위해 동기화를 추가하거나 의도적 설계라면 주석으로 명시하는 것을 권장합니다.
♻️ 일관성을 위한 선택적 수정
override fun onEvent( eventSource: EventSource, id: String?, type: String?, data: String, ) { - if (!isCurrentSource(eventSource)) return - val eventName = type ?: return - _rawEvents.tryEmit(RawEventResponse(eventName = eventName, data = data)) + synchronized(eventSourceLock) { + if (!isCurrentSource(eventSource)) return + val eventName = type ?: return + _rawEvents.tryEmit(RawEventResponse(eventName = eventName, data = data)) + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/smashing/app/data/remote/datasource/impl/EventRemoteDataSourceImpl.kt` around lines 63 - 72, onEvent currently calls isCurrentSource without holding eventSourceLock, unlike onOpen/onFailure/onClosed; wrap the isCurrentSource check and the subsequent _rawEvents.tryEmit(RawEventResponse(...)) inside a synchronized(eventSourceLock) block to prevent emitting events for a source that may be disconnected concurrently (or if you intend no lock, add a clear comment explaining the race is benign). Update the onEvent implementation to use synchronized(eventSourceLock) around the isCurrentSource check and emit, referencing the onEvent method, isCurrentSource(), eventSourceLock, _rawEvents.tryEmit(...), and disconnect() for context.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/main/java/com/smashing/app/core/network/di/NetworkModule.kt`:
- Around line 140-145: AuthInterceptor currently uses addHeader(AUTHORIZATION,
...) which can create duplicate Authorization headers versus
TokenAuthenticator's header(AUTHORIZATION, ...); update AuthInterceptor (the
class/method that sets the AUTHORIZATION header) to use header(AUTHORIZATION,
...) instead of addHeader so the Authorization header is replaced rather than
appended, keeping behavior consistent with TokenAuthenticator and avoiding
duplicate auth headers.
In `@app/src/main/java/com/smashing/app/core/network/sse/SseManager.kt`:
- Around line 64-80: In observeConnectionState(), change the Error branch to
inspect SseConnectionState.Error.statusCode: if the error is a 401, do not call
scheduleReconnectLocked; instead invoke the token-refresh flow via the existing
TokenAuthenticator or AuthManager (e.g., call TokenAuthenticator.refreshToken()
or AuthManager.handle401()) inside the same mutex.withLock scope (or ensure
mutual exclusion) and only schedule a reconnect if the refresh fails or for
non-401 errors; preserve the existing resetReconnectLocked behavior for
Connected states and keep scheduleReconnectLocked for Disconnected and non-401
Error cases.
---
Nitpick comments:
In `@app/src/main/java/com/smashing/app/core/network/sse/SseManager.kt`:
- Around line 140-143: When reconnectAttempt >= MAX_RECONNECT_ATTEMPTS in
SseManager you currently only log and return; add a user-facing notification by
emitting a connection-failure event before returning. Update the block that
contains reconnectAttempt, MAX_RECONNECT_ATTEMPTS and Timber.tag(TAG).e to call
the manager's event emitter (or add a new
emitConnectionFailure/notifyConnectionFailed method on SseManager) and include
contextual details (reconnectAttempt and MAX_RECONNECT_ATTEMPTS) in the event
payload so listeners/UI can show a retry-limit notification to the user.
In
`@app/src/main/java/com/smashing/app/data/remote/datasource/impl/EventRemoteDataSourceImpl.kt`:
- Around line 63-72: onEvent currently calls isCurrentSource without holding
eventSourceLock, unlike onOpen/onFailure/onClosed; wrap the isCurrentSource
check and the subsequent _rawEvents.tryEmit(RawEventResponse(...)) inside a
synchronized(eventSourceLock) block to prevent emitting events for a source that
may be disconnected concurrently (or if you intend no lock, add a clear comment
explaining the race is benign). Update the onEvent implementation to use
synchronized(eventSourceLock) around the isCurrentSource check and emit,
referencing the onEvent method, isCurrentSource(), eventSourceLock,
_rawEvents.tryEmit(...), and disconnect() for context.
In
`@app/src/main/java/com/smashing/app/domain/usecase/auth/TokenReissueUseCase.kt`:
- Around line 33-45: The invoke() in TokenReissueUseCase currently uses
mutex.withLock but still allows queued callers to perform duplicate reissue
calls; fix this by introducing a shared in-flight marker (e.g., a nullable
Deferred<Result<String>> field like ongoingReissue) that you read/write under
the existing mutex: if ongoingReissue exists return/await it, otherwise create a
new async Deferred that executes
authRepository.postTokenReissue(PostTokenReissueRequest(refreshToken)) (and maps
to accessToken), store it in ongoingReissue, await its result for callers, and
clear ongoingReissue on completion/failure; ensure
tokenDataSource.getRefreshToken and error handling remain unchanged and access
to ongoingReissue is protected by the same mutex.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 250bfeb3-a412-4f71-8b92-3f8894ecdcd8
📒 Files selected for processing (9)
app/src/main/java/com/smashing/app/core/network/TokenAuthenticator.ktapp/src/main/java/com/smashing/app/core/network/di/NetworkModule.ktapp/src/main/java/com/smashing/app/core/network/sse/SseConnectionState.ktapp/src/main/java/com/smashing/app/core/network/sse/SseManager.ktapp/src/main/java/com/smashing/app/core/network/token/AuthManagerImpl.ktapp/src/main/java/com/smashing/app/data/remote/datasource/impl/EventRemoteDataSourceImpl.ktapp/src/main/java/com/smashing/app/data/repository/impl/EventRepositoryImpl.ktapp/src/main/java/com/smashing/app/domain/usecase/auth/TokenReissueUseCase.ktapp/src/main/java/com/smashing/app/presentation/main/MainActivity.kt
🚧 Files skipped from review as they are similar to previous changes (4)
- app/src/main/java/com/smashing/app/presentation/main/MainActivity.kt
- app/src/main/java/com/smashing/app/core/network/TokenAuthenticator.kt
- app/src/main/java/com/smashing/app/core/network/sse/SseConnectionState.kt
- app/src/main/java/com/smashing/app/data/repository/impl/EventRepositoryImpl.kt
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
app/src/main/java/com/smashing/app/core/network/sse/SseManager.kt (1)
67-73:⚠️ Potential issue | 🟠 Major401은 재연결이 아니라 재발급 경로로 보내야 합니다.
Line 72는
Error와Disconnected를 같은 분기로 묶어서, 만료 토큰도 일반 transport 오류처럼 최대 30회 backoff retry 하게 만듭니다. PR 목표대로라면statusCode == 401은 기존 토큰 재발급 경로로 분기하고, 그 실패 때만 재연결 또는 로그아웃을 결정해야 합니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/smashing/app/core/network/sse/SseManager.kt` around lines 67 - 73, The current when(state) groups SseConnectionState.Error and Disconnected together causing 401 errors to be treated as transport retries; change the branch so SseConnectionState.Error checks the underlying HTTP/status code (e.g., error.statusCode == 401) and route that case to the token re-issuance flow (call the existing refresh token function—e.g., refreshAuthToken() or the token reissue handler); only if the token refresh fails should you fall back to scheduleReconnectLocked() or trigger logout, and for non-401 errors continue to call mutex.withLock { scheduleReconnectLocked() } as before; keep resetReconnectLocked() for SseConnectionState.Connected.
🧹 Nitpick comments (1)
app/src/main/java/com/smashing/app/data/remote/datasource/impl/EventRemoteDataSourceImpl.kt (1)
63-72:onEvent()에서 동기화 누락 확인 필요다른 콜백(
onOpen,onFailure,onClosed)과 달리onEvent()는synchronized(eventSourceLock)블록 없이isCurrentSource()를 호출합니다._rawEvents.tryEmit()이 스레드 세이프하므로 심각한 문제는 아니지만, 일관성을 위해 동기화 추가를 고려해 보세요.♻️ 일관성을 위한 동기화 추가
override fun onEvent( eventSource: EventSource, id: String?, type: String?, data: String, ) { + synchronized(eventSourceLock) { if (!isCurrentSource(eventSource)) return val eventName = type ?: return _rawEvents.tryEmit(RawEventResponse(eventName = eventName, data = data)) + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/smashing/app/data/remote/datasource/impl/EventRemoteDataSourceImpl.kt` around lines 63 - 72, onEvent currently calls isCurrentSource and _rawEvents.tryEmit without the synchronized(eventSourceLock) used by onOpen/onFailure/onClosed; wrap the body of onEvent in a synchronized(eventSourceLock) block so the eventSource identity check (isCurrentSource(eventSource)) and the emit (_rawEvents.tryEmit(RawEventResponse(...))) execute under the same lock to maintain consistency with other callbacks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@app/src/main/java/com/smashing/app/data/repository/impl/AuthRepositoryImpl.kt`:
- Around line 79-84: 현재 response.toTokenReissueModel()로 만든 tokenReissueModel을 검증
없이 tokenDataStore.setTokens(...)로 바로 저장하면 빈 문자열(또는 null) 응답 시 기존 유효 토큰이 덮어써져 인증
실패 루프가 발생할 수 있습니다; 따라서 tokenReissueModel.accessToken과
tokenReissueModel.refreshToken이 비어있지 않은지(또는 null이 아닌지) 검사한 뒤 유효한 값이 하나라도 있으면 해당
토큰만 업데이트하거나(예: access가 유효하면 access만, refresh가 유효하면 refresh만) 둘 다 비어있다면
setTokens를 호출하지 않고 오류를 기록하거나 예외를 던져 기존 토큰을 보존하도록 수정하세요; 관련 식별자:
response.toTokenReissueModel(), tokenReissueModel.accessToken,
tokenReissueModel.refreshToken, tokenDataStore.setTokens().
In
`@app/src/main/java/com/smashing/app/data/repository/impl/EventRepositoryImpl.kt`:
- Around line 49-53: The code silently drops unsupported event names and always
logs "event emitted" even if emission fails; update the handling around
SseEventType.fromEventName, parseEvent, and _events.tryEmit so that when
SseEventType.fromEventName(raw.eventName) returns null you log a clear warning
including the raw.eventName (and optionally raw.data) indicating
unknown/unsupported event, and after calling _events.tryEmit(event) check its
Boolean/Result return value and log success vs failure (including event details
and reason/context) instead of unconditionally logging "event emitted" via
Timber.tag(SSE_LOG_TAG).d; ensure parseEvent failures are also logged similarly
before returning.
In `@fastlane/notes/qa/debug.txt`:
- Around line 1-3: The release note is too generic and misleading (mentions "서버
명세 수정" while the PR mainly changes client SSE reconnection), so update the QA
release note (the "1차 스프린트 QA 용 배포 26.04.03" entry) to list concrete changes and
tester checks: add bullets for SSE exponential backoff behavior (1s–120s, up to
30 retries), the 401 token refresh flow trigger and expected outcome, and SSE
connection management across app foreground/background and login state, and
remove or correct the "서버 명세 수정" line to reflect client-side SSE reconnection
improvements.
---
Duplicate comments:
In `@app/src/main/java/com/smashing/app/core/network/sse/SseManager.kt`:
- Around line 67-73: The current when(state) groups SseConnectionState.Error and
Disconnected together causing 401 errors to be treated as transport retries;
change the branch so SseConnectionState.Error checks the underlying HTTP/status
code (e.g., error.statusCode == 401) and route that case to the token
re-issuance flow (call the existing refresh token function—e.g.,
refreshAuthToken() or the token reissue handler); only if the token refresh
fails should you fall back to scheduleReconnectLocked() or trigger logout, and
for non-401 errors continue to call mutex.withLock { scheduleReconnectLocked() }
as before; keep resetReconnectLocked() for SseConnectionState.Connected.
---
Nitpick comments:
In
`@app/src/main/java/com/smashing/app/data/remote/datasource/impl/EventRemoteDataSourceImpl.kt`:
- Around line 63-72: onEvent currently calls isCurrentSource and
_rawEvents.tryEmit without the synchronized(eventSourceLock) used by
onOpen/onFailure/onClosed; wrap the body of onEvent in a
synchronized(eventSourceLock) block so the eventSource identity check
(isCurrentSource(eventSource)) and the emit
(_rawEvents.tryEmit(RawEventResponse(...))) execute under the same lock to
maintain consistency with other callbacks.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 9db226d8-37e9-4c14-8e1f-1e90e27d8482
📒 Files selected for processing (19)
app/src/main/java/com/smashing/app/core/network/TokenAuthenticator.ktapp/src/main/java/com/smashing/app/core/network/di/NetworkModule.ktapp/src/main/java/com/smashing/app/core/network/sse/SseConnectionState.ktapp/src/main/java/com/smashing/app/core/network/sse/SseManager.ktapp/src/main/java/com/smashing/app/core/network/token/AuthManager.ktapp/src/main/java/com/smashing/app/core/network/token/AuthManagerImpl.ktapp/src/main/java/com/smashing/app/data/remote/datasource/api/EventRemoteDatasource.ktapp/src/main/java/com/smashing/app/data/remote/datasource/impl/EventRemoteDataSourceImpl.ktapp/src/main/java/com/smashing/app/data/repository/api/EventRepository.ktapp/src/main/java/com/smashing/app/data/repository/impl/AuthRepositoryImpl.ktapp/src/main/java/com/smashing/app/data/repository/impl/EventRepositoryImpl.ktapp/src/main/java/com/smashing/app/domain/usecase/auth/TokenReissueUseCase.ktapp/src/main/java/com/smashing/app/presentation/login/LoginViewModel.ktapp/src/main/java/com/smashing/app/presentation/main/MainActivity.ktapp/src/main/java/com/smashing/app/presentation/mypage/MyPageViewModel.ktapp/src/main/java/com/smashing/app/presentation/signup/SignUpViewModel.ktapp/src/main/java/com/smashing/app/presentation/splash/SplashViewModel.ktapp/src/main/java/com/smashing/app/presentation/withdraw/WithdrawViewModel.ktfastlane/notes/qa/debug.txt
Comment on lines
+79
to
+84
| val tokenReissueModel = response.toTokenReissueModel() | ||
|
|
||
| response.toTokenReissueModel() | ||
| tokenDataStore.setTokens( | ||
| accessToken = tokenReissueModel.accessToken, | ||
| refreshToken = tokenReissueModel.refreshToken, | ||
| ) |
There was a problem hiding this comment.
재발급 토큰 공백값 저장을 막아주세요.
Line 81-Line 84에서 토큰 유효성 검증 없이 저장하면, 비정상 응답(빈 토큰) 시 기존 유효 토큰을 덮어써 인증 실패 루프가 발생할 수 있습니다.
🔧 제안 수정안
val tokenReissueModel = response.toTokenReissueModel()
+ require(tokenReissueModel.accessToken.isNotBlank() && tokenReissueModel.refreshToken.isNotBlank()) {
+ "Reissued token is blank"
+ }
tokenDataStore.setTokens(
accessToken = tokenReissueModel.accessToken,
refreshToken = tokenReissueModel.refreshToken,
)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@app/src/main/java/com/smashing/app/data/repository/impl/AuthRepositoryImpl.kt`
around lines 79 - 84, 현재 response.toTokenReissueModel()로 만든 tokenReissueModel을
검증 없이 tokenDataStore.setTokens(...)로 바로 저장하면 빈 문자열(또는 null) 응답 시 기존 유효 토큰이 덮어써져
인증 실패 루프가 발생할 수 있습니다; 따라서 tokenReissueModel.accessToken과
tokenReissueModel.refreshToken이 비어있지 않은지(또는 null이 아닌지) 검사한 뒤 유효한 값이 하나라도 있으면 해당
토큰만 업데이트하거나(예: access가 유효하면 access만, refresh가 유효하면 refresh만) 둘 다 비어있다면
setTokens를 호출하지 않고 오류를 기록하거나 예외를 던져 기존 토큰을 보존하도록 수정하세요; 관련 식별자:
response.toTokenReissueModel(), tokenReissueModel.accessToken,
tokenReissueModel.refreshToken, tokenDataStore.setTokens().
oilbeaneda
approved these changes
Apr 18, 2026
oilbeaneda
left a comment
oilbeaneda
left a comment
Collaborator
There was a problem hiding this comment.
와우띵가링가링 .. 어려운 sse 보고 더 공부해보겠슨니다 수고많으셨습니다!
ShinHyeongcheol
approved these changes
Apr 20, 2026
ShinHyeongcheol
left a comment
ShinHyeongcheol
left a comment
Collaborator
There was a problem hiding this comment.
호오 한동안 틈틈히 봤는데, 흠 잡을 곳을 찾을 수가 없다!!
코드 리뷰 보다 synchronized(stateLock)를 처음 알게 되어서 관련해서 궁금한 점 남겨두었습니다!!
| * 로그아웃 전환(forceLogoutEvent emit)은 최초 1회만 허용한다. | ||
| * 사용자가 다시 로그인하면 false로 리셋한다. | ||
| */ | ||
| private val stateLock = Any() |
Collaborator
There was a problem hiding this comment.
p3: 요거 synchronized(stateLock)을 사용한 부분 처음 알게 된 내용이라 인상적이었어요!!
궁금한 점은 stateLock을 사용할 때, Any()를 사용해주는 이유는 무엇일까요?? 다른 값으로 초기화 해주면 안될까요?
이전 커밋을 보면 AtomicBoolean을 사용해서 관리하는 것으로 보았고, 두 방식의 차이점은 하나의 값을 다룰지와 여러 값을 관리할지 일 것 같은데 AtomicBoolean을 여러개 사용하는 방식과 비교했을 때 현 방법의 장점은 무엇이라고 생각하나요??
Member
Author
There was a problem hiding this comment.
Any()를 사용한 이유는 this나 다른 공유 객체를 lock으로 사용할 경우, 예상치 못한 곳에서 동일한 lock을 잡아 문제가 생길 수 있기 때문이에요. Effective Kotlin에서도 Any를 사용하는 예제가 있어서 이를 사용했고 private으로 숨긴 전용 lock 객체 생성했다에 초점을 맞추면 좋을거 같아요.
AtomicBoolean은 말씀하신 것처럼 단일 값의 원자적 변경에는 매우 적합해요.
다만 관리해야 할 상태가 2~3개 이상이고 이를 일관되게 변경해야 하는 경우에는 atomic 변수 여러 개만으로는 한 값만 먼저 바뀐 중간 상태가 외부에 노출될 수 있어요.
반면 synchronized를 사용하면 관련 상태 변경을 하나의 트랜잭션처럼 묶어 처리할 수 있어서, 순서와 일관성을 보장하기 쉬워요.
정리하면 장점은 다음과 같아요.
- 여러 상태의 불변식 유지가 쉬워요.
- 읽기/쓰기 경계가 명확해 코드 의도가 잘 드러나요.
- 이후 상태가 늘어나도 동기화 전략을 크게 바꾸지 않아도 돼요.
vahkjsdf
approved these changes
Apr 23, 2026
vahkjsdf
left a comment
vahkjsdf
left a comment
Collaborator
There was a problem hiding this comment.
우와 수정하시느라 수고 많으셨어요! 이것저것 공부할게 생겼네용 열심히 따라가보겠습니다!!
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
4 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.
Related issue 🛠
Work Description ✏️
TokenReissueUseCase기반으로 재발급 경로 통합SseManager임시 관리 방식에서AuthManager기반 상태 관리로 정리Screenshot 📸
Uncompleted Tasks 😅
To Reviewers 📢
기존의 Token 재발급 로직을 SSE 에 붙이는 과정에서 TokenUseCase 를 만들어서 통합하고-> 중복 재호출 가능성을 줄이고자 Authenticator 를 사용해서 단일진입점을 만들었습니다.
로그인 상태 관리를 위해 임시로 SseManger 에서 임시로 관리하던 로그인 상태 로직을 AuthManger 로 이동시켰습니다.
작업하다보니 수정 사항이 많아졌네요..😅 양해부탁드립니다.
Summary by CodeRabbit
버그 수정
리팩토링