Guard against all uncaught exceptions in the serial executor block#997
Guard against all uncaught exceptions in the serial executor block#997pmathew92 wants to merge 1 commit into
Conversation
|
Warning Review limit reached
Next review available in: 3 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughAdds a ChangesrunCatchingOnExecutor refactor
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant CredentialsManager
participant runCatchingOnExecutor
participant Storage
participant Callback
Caller->>CredentialsManager: getCredentials(...)
CredentialsManager->>runCatchingOnExecutor: execute block
runCatchingOnExecutor->>Storage: retrieveString(...)
alt storage throws unexpected exception
Storage-->>runCatchingOnExecutor: Throwable
runCatchingOnExecutor->>Callback: onFailure(UNKNOWN_ERROR, cause)
else success
Storage-->>runCatchingOnExecutor: stored value
runCatchingOnExecutor-->>Callback: onSuccess(credentials)
end
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| tokenType.orEmpty(), | ||
| refreshToken, | ||
| Date(expiresAt), | ||
| storedScope |
| val expiresAt = fresh.expiresAt.time | ||
| val willAccessTokenExpire = willExpire(expiresAt, minTtl.toLong()) | ||
| if (willAccessTokenExpire) { | ||
| val tokenLifetime = (expiresAt - currentTimeInMillis - minTtl * 1000) / -1000 |
| val expiresAt = newCredentials.expiresAt.time | ||
| val willAccessTokenExpire = willExpire(expiresAt, minTtl.toLong()) | ||
| if (willAccessTokenExpire) { | ||
| val tokenLifetime = (expiresAt - currentTimeInMillis - minTtl * 1000) / -1000 |
| val expiresAt = fresh.expiresAt.time | ||
| val willAccessTokenExpire = willExpire(expiresAt, minTtl.toLong()) | ||
| if (willAccessTokenExpire) { | ||
| val tokenLifetime = (expiresAt - currentTimeInMillis - minTtl * 1000) / -1000 |
| val expiresAt = newCredentials.expiresAt.time | ||
| val willAccessTokenExpire = willExpire(expiresAt, minTtl.toLong()) | ||
| if (willAccessTokenExpire) { | ||
| val tokenLifetime = (expiresAt - currentTimeInMillis - minTtl * 1000) / -1000 |
✅ Action performedFull review finished. |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
![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 (1)
auth0/src/test/java/com/auth0/android/authentication/storage/SecureCredentialsManagerTest.kt (1)
4215-4239: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing test for the
STORE_FAILEDalignment mentioned in the PR objectives.These new tests only cover the outer
runCatchingOnExecutorsafety net (UNKNOWN_ERROR). The PR description statescontinueGetApiCredentialswas changed to map save failures toSTORE_FAILEDinstead of falling through toUNKNOWN_ERROR, but no test in this diff exercises that specific path (e.g.,crypto.encrypt/storage save throwing during API credential renewal).Consider adding a test that triggers a save failure after a successful
request.execute()incontinueGetApiCredentialsand assertsCredentialsManagerException.Code.STORE_FAILED.🤖 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 `@auth0/src/test/java/com/auth0/android/authentication/storage/SecureCredentialsManagerTest.kt` around lines 4215 - 4239, The new test in SecureCredentialsManagerTest only validates the outer runCatchingOnExecutor fallback to UNKNOWN_ERROR, but it does not cover the STORE_FAILED mapping added in continueGetApiCredentials. Add a test around continueGetApiCredentials that makes request.execute() succeed and then forces the credential persistence step to fail (for example via crypto.encrypt or the storage save path), and assert that the callback receives CredentialsManagerException.Code.STORE_FAILED rather than UNKNOWN_ERROR. Use the existing shouldCatchUnexpectedExceptionDuringApiCredentialRenewal setup style and the apiCredentialsCallback/exceptionCaptor assertions to keep it aligned with the surrounding tests.
🤖 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
`@auth0/src/main/java/com/auth0/android/authentication/storage/BaseCredentialsManager.kt`:
- Around line 335-339: The catch block in BaseCredentialsManager’s executor
handling is swallowing fatal JVM errors by treating every Throwable as an
UNKNOWN_ERROR callback failure. Update the Throwable handling in this block to
rethrow VirtualMachineError, ThreadDeath, and LinkageError before the Log.e and
callback.onFailure path, so only non-fatal exceptions are mapped to
CredentialsManagerException.Code.UNKNOWN_ERROR.
In
`@auth0/src/main/java/com/auth0/android/authentication/storage/SecureCredentialsManager.kt`:
- Line 757: The public getCredentials flow still performs a storage pre-check
via hasValidCredentials(minTtl.toLong()) outside the runCatchingOnExecutor
protection, so storage failures can bypass UNKNOWN_ERROR. Update
SecureCredentialsManager.getCredentials so that the pre-check is wrapped in the
same guarded error handling as continueGetCredentials, or move the check under
that wrapper, while keeping the biometric prompt behavior unchanged.
---
Nitpick comments:
In
`@auth0/src/test/java/com/auth0/android/authentication/storage/SecureCredentialsManagerTest.kt`:
- Around line 4215-4239: The new test in SecureCredentialsManagerTest only
validates the outer runCatchingOnExecutor fallback to UNKNOWN_ERROR, but it does
not cover the STORE_FAILED mapping added in continueGetApiCredentials. Add a
test around continueGetApiCredentials that makes request.execute() succeed and
then forces the credential persistence step to fail (for example via
crypto.encrypt or the storage save path), and assert that the callback receives
CredentialsManagerException.Code.STORE_FAILED rather than UNKNOWN_ERROR. Use the
existing shouldCatchUnexpectedExceptionDuringApiCredentialRenewal setup style
and the apiCredentialsCallback/exceptionCaptor assertions to keep it aligned
with the surrounding tests.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c55bef49-7298-4726-ab4e-5867e3f972e8
📒 Files selected for processing (5)
auth0/src/main/java/com/auth0/android/authentication/storage/BaseCredentialsManager.ktauth0/src/main/java/com/auth0/android/authentication/storage/CredentialsManager.ktauth0/src/main/java/com/auth0/android/authentication/storage/SecureCredentialsManager.ktauth0/src/test/java/com/auth0/android/authentication/storage/CredentialsManagerTest.ktauth0/src/test/java/com/auth0/android/authentication/storage/SecureCredentialsManagerTest.kt
| } catch (t: Throwable) { | ||
| Log.e("BaseCredentialsManager", "Unexpected error in executor block", t) | ||
| callback.onFailure( | ||
| CredentialsManagerException(CredentialsManagerException.Code.UNKNOWN_ERROR, t) | ||
| ) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant file and inspect the target region.
ast-grep outline auth0/src/main/java/com/auth0/android/authentication/storage/BaseCredentialsManager.kt --view expanded || true
wc -l auth0/src/main/java/com/auth0/android/authentication/storage/BaseCredentialsManager.kt
sed -n '300,360p' auth0/src/main/java/com/auth0/android/authentication/storage/BaseCredentialsManager.ktRepository: auth0/Auth0.Android
Length of output: 3597
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check whether similar Throwable handling elsewhere in the repo already excludes fatal errors.
rg -n "catch \(t: Throwable\)|catch \([a-zA-Z_]+: Throwable\)|VirtualMachineError|ThreadDeath|LinkageError" auth0/src/main/java/com/auth0/android -SRepository: auth0/Auth0.Android
Length of output: 281
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Probe Kotlin/JVM semantics for catching fatal errors is not needed; instead inspect API contract/docs if any mention this executor helper.
rg -n "runCatchingOnExecutor|Unexpected error in executor block|UNKNOWN_ERROR|CredentialsManagerException.Code.UNKNOWN_ERROR" auth0/src/main/java/com/auth0/android auth0/README.md auth0/EXAMPLES.md -SRepository: auth0/Auth0.Android
Length of output: 5250
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the exact helper and nearby implementations to see whether fatal errors are intentionally converted into callback failures.
sed -n '320,350p' auth0/src/main/java/com/auth0/android/authentication/storage/BaseCredentialsManager.kt
printf '\n---\n'
rg -n "catch \(t: Throwable\)|catch \(e: Exception\)|runCatchingOnExecutor|UNKNOWN_ERROR" auth0/src/main/java/com/auth0/android/authentication/storage -SRepository: auth0/Auth0.Android
Length of output: 2792
Rethrow fatal JVM errors before mapping Throwable to UNKNOWN_ERROR. VirtualMachineError, ThreadDeath, and LinkageError should escape here instead of being converted into a recoverable callback failure.
Suggested guard
- } catch (t: Throwable) {
+ } catch (t: Throwable) {
+ if (t is VirtualMachineError || t is ThreadDeath || t is LinkageError) {
+ throw t
+ }
Log.e("BaseCredentialsManager", "Unexpected error in executor block", t)
callback.onFailure(
CredentialsManagerException(CredentialsManagerException.Code.UNKNOWN_ERROR, t)
)📝 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.
| } catch (t: Throwable) { | |
| Log.e("BaseCredentialsManager", "Unexpected error in executor block", t) | |
| callback.onFailure( | |
| CredentialsManagerException(CredentialsManagerException.Code.UNKNOWN_ERROR, t) | |
| ) | |
| } catch (t: Throwable) { | |
| if (t is VirtualMachineError || t is ThreadDeath || t is LinkageError) { | |
| throw t | |
| } | |
| Log.e("BaseCredentialsManager", "Unexpected error in executor block", t) | |
| callback.onFailure( | |
| CredentialsManagerException(CredentialsManagerException.Code.UNKNOWN_ERROR, t) | |
| ) |
🤖 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
`@auth0/src/main/java/com/auth0/android/authentication/storage/BaseCredentialsManager.kt`
around lines 335 - 339, The catch block in BaseCredentialsManager’s executor
handling is swallowing fatal JVM errors by treating every Throwable as an
UNKNOWN_ERROR callback failure. Update the Throwable handling in this block to
rethrow VirtualMachineError, ThreadDeath, and LinkageError before the Log.e and
callback.onFailure path, so only non-fatal exceptions are mapped to
CredentialsManagerException.Code.UNKNOWN_ERROR.
| validateDPoPState(tokenType)?.let { dpopError -> | ||
| callback.onFailure(dpopError) | ||
| return@execute | ||
| } |
There was a problem hiding this comment.
Wrapping the whole block means the fast-path callback.onSuccess(...) now runs inside the catch (Throwable). If a consumer's onSuccess throws, it gets caught and turned into a second onFailure(UNKNOWN_ERROR) — a double callback. On awaitCredentials, that's a resume after resume → IllegalStateException: Already resumed. Might be cleaner to call onSuccess outside the guarded block. Minor/edge-case, and it's inherited from #970
4 participants
Summary
Ports the serial-executor exception guard (#970) onto the
v4_developmentbranch as its own feature.runCatchingOnExecutorhelper toBaseCredentialsManager, wrapping eachserialExecutor.execute { }body in acatch (Throwable) -> onFailure(UNKNOWN_ERROR)safety net so unexpected errors surface as a failure callback instead of being swallowed on the executor thread.CredentialsManagerand all 3SecureCredentialsManagerexecute blocks; removes the now-redundant trailingcatch (RuntimeException)blocks while keeping the domain-specific catches (AuthenticationException,CredentialsManagerException,IncompatibleDeviceException,CryptoException).SecureCredentialsManager.continueGetApiCredentialsto parity with the other two blocks by mapping a save failure toSTORE_FAILED(rather than letting it fall through to the newUNKNOWN_ERRORnet).This is the v4 port of the change merged to
mainin #970; reconciled onto v4's restructured credentials managers and test imports (org.mockito.kotlin.*).Test plan
./gradlew :auth0:compileDebugKotlin./gradlew :auth0:testDebugUnitTest— full suite greenCredentialsManagerTest(98 total), 4 new inSecureCredentialsManagerTest(131 total), 0 failuresSummary by CodeRabbit
Bug Fixes
Tests