Fail HeartbeatWorker cleanly when session is inactive#3552
Fail HeartbeatWorker cleanly when session is inactive#3552
Conversation
📝 WalkthroughWalkthroughThe change refactors the session validation logic in HeartbeatWorker.kt by replacing a combined condition with an early guard pattern. The new structure immediately returns failure if the session is inactive, then separately evaluates whether to proceed with a heartbeat request. An import for SessionUnavailableException was added. Exception handling remains unchanged in scope and behavior. Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 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: 1
🤖 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/org/commcare/heartbeat/HeartbeatWorker.kt`:
- Line 8: The imported but unused SessionUnavailableException indicates we need
to handle a race where isSessionActive() can be true but accessing session
throws SessionUnavailableException; update the when block that handles
exceptions in HeartbeatWorker (the same block that currently checks
isSessionActive() and accesses .session) to add an explicit case for
SessionUnavailableException and handle it the same way as the session-not-active
path (avoid logging the misleading "unexpected exception" message and stop the
heartbeat thread gracefully), referencing isSessionActive(), session, and
SessionUnavailableException so the exception is caught and logged/handled
appropriately.
| import androidx.work.WorkerParameters | ||
| import org.commcare.CommCareApplication | ||
| import org.commcare.util.LogTypes | ||
| import org.commcare.utils.SessionUnavailableException |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate SessionUnavailableException and check its definition
fd -type f \( -name "*.kt" -o -name "*.java" \) | xargs grep -l "class SessionUnavailableException" | head -5Repository: dimagi/commcare-android
Length of output: 237
🏁 Script executed:
#!/bin/bash
# Read the HeartbeatWorker.kt file to inspect the full catch block and imports
cat -n app/src/org/commcare/heartbeat/HeartbeatWorker.ktRepository: dimagi/commcare-android
Length of output: 1946
🏁 Script executed:
#!/bin/bash
# Find SessionUnavailableException definition
find . -name "*.kt" -o -name "*.java" | xargs grep -l "class SessionUnavailableException" 2>/dev/nullRepository: dimagi/commcare-android
Length of output: 128
🏁 Script executed:
#!/bin/bash
# Search for SessionUnavailableException references in the codebase
rg "SessionUnavailableException" -A 2 -B 2Repository: dimagi/commcare-android
Length of output: 50379
🏁 Script executed:
#!/bin/bash
# Verify SessionUnavailableException is only in import and nowhere else in HeartbeatWorker.kt
grep -n "SessionUnavailableException" app/src/org/commcare/heartbeat/HeartbeatWorker.ktRepository: dimagi/commcare-android
Length of output: 122
Add SessionUnavailableException to the catch block to prevent misleading logging
The import on line 8 is unused and signals incomplete implementation. Between the isSessionActive() check (line 22) and the .session access (line 25), a race condition can occur: if the session expires in that window, .session throws SessionUnavailableException. Since SessionUnavailableException is not listed in the when block's cases, it falls through to the else arm and logs:
"Encountered unexpected exception during heartbeat communications, stopping the heartbeat thread."
This is exactly the misleading noise the PR aims to eliminate. Add an explicit case to handle it:
Proposed fix
} catch (e: Exception) {
// Encountered an unexpected issue, should just bail on this thread
return when (e) {
is UnknownHostException, is IllegalStateException, is IOException -> {
Result.retry()
}
+ is SessionUnavailableException -> {
+ // Session expired between the active-check and the session access; expected, no logging needed.
+ Result.failure()
+ }
else -> {
Logger.exception(
"Encountered unexpected exception during heartbeat communications, stopping the heartbeat thread.",
e
)
Result.failure()
}
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import org.commcare.utils.SessionUnavailableException | |
| } catch (e: Exception) { | |
| // Encountered an unexpected issue, should just bail on this thread | |
| return when (e) { | |
| is UnknownHostException, is IllegalStateException, is IOException -> { | |
| Result.retry() | |
| } | |
| is SessionUnavailableException -> { | |
| // Session expired between the active-check and the session access; expected, no logging needed. | |
| Result.failure() | |
| } | |
| else -> { | |
| Logger.exception( | |
| "Encountered unexpected exception during heartbeat communications, stopping the heartbeat thread.", | |
| e | |
| ) | |
| Result.failure() | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/org/commcare/heartbeat/HeartbeatWorker.kt` at line 8, The imported
but unused SessionUnavailableException indicates we need to handle a race where
isSessionActive() can be true but accessing session throws
SessionUnavailableException; update the when block that handles exceptions in
HeartbeatWorker (the same block that currently checks isSessionActive() and
accesses .session) to add an explicit case for SessionUnavailableException and
handle it the same way as the session-not-active path (avoid logging the
misleading "unexpected exception" message and stop the heartbeat thread
gracefully), referencing isSessionActive(), session, and
SessionUnavailableException so the exception is caught and logged/handled
appropriately.
conroy-ricketts
left a comment
conroy-ricketts
left a comment
There was a problem hiding this comment.
Looks like we can delete an unused import as well?
3 participants
Product Description
This PR addresses the most prominent non-fatal issue reported in Crashlytics. It's caused when the user's session expires and the
HeartbeatWorkertries to accessCommCareApplication.instance().sessionto verify if the session is active. This method throws aSessionUnavailableExceptionexception when there is no active session, which is caught by the generic exception handler and logged as an "unexpected exception during heartbeat communications". Session expiry is expected behavior, and heartbeat requests are not relevant to CommCare when there is no active user session.Crashlytics report. This is
Technical Summary
isSessionActive()check inHeartbeatWorker.doWork()that returnsResult.failure()when the session has expired.Feature Flag
N/A
Safety Assurance
Safety story
This change is a small guard clause. Existing tests continue to pass.
Labels and Review