Conversation
…n of continuation resume
📝 WalkthroughWalkthroughCredential manager executor callbacks now deliver at most one terminal result, while suspend credential APIs avoid resuming cancelled continuations. Manager integrations and regression tests cover callback exceptions, duplicate callbacks, and duplicate coroutine resumption. ChangesCredential completion safety
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant CredentialsManager
participant BaseCredentialsManager
participant Executor
participant Continuation
Caller->>CredentialsManager: request credentials
CredentialsManager->>BaseCredentialsManager: run executor operation
BaseCredentialsManager->>Executor: execute operation
Executor->>CredentialsManager: success or failure callback
CredentialsManager->>Continuation: resume if active
Executor-->>BaseCredentialsManager: optional duplicate callback or exception
BaseCredentialsManager-->>CredentialsManager: suppress duplicate terminal result
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
auth0/src/main/java/com/auth0/android/authentication/storage/BaseCredentialsManager.kt (2)
429-445: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueBroad
catch (t: Throwable)— intentional bulkhead, but verify scope and log-tag consistency.Catching generic
Throwablehere runs counter to the general guideline of using specific exception types, though it's plausibly required as a last-resort bulkhead so any unexpected error from consumer/bridge code still yields a single terminal callback instead of crashing the executor thread. Two small follow-ups:
- Consider a one-line comment explaining why the broad catch is intentional here (helps future maintainers avoid narrowing it by mistake).
- The hardcoded
"BaseCredentialsManager"log tag diverges from thethis::class.java.simpleNamepattern used elsewhere in this same file (e.g.saveDPoPThumbprint,validateDPoPState).As per coding guidelines, "Use specific exception types and typed status/error codes; do not catch generic exceptions or rely on error-message string matching."
♻️ Optional tweak for log tag consistency
- Log.e("BaseCredentialsManager", "Unexpected error in executor block", t) + Log.e(this::class.java.simpleName, "Unexpected error in executor block", 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 429 - 445, Add a concise comment above the broad catch in runCatchingOnExecutor explaining that it intentionally acts as a last-resort bulkhead for unexpected callback or bridge failures while preserving a single terminal callback. Also replace the hardcoded "BaseCredentialsManager" Log.e tag with the class-name pattern already used by nearby methods, without changing the existing fatal-error filtering or callback behavior.Source: Coding guidelines
447-468: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLog dropped duplicate terminal callback calls.
SingleShotCallbackalready guards multiple callbacks, but when a second explicitonSuccessoronFailureis dropped there’s no signal. Add a short log entry on each dropped branch to catch future regressions where an executor calls both terminal callbacks without throwing.🤖 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 447 - 468, Update SingleShotCallback’s onSuccess and onFailure methods to log a brief entry whenever handled.compareAndSet rejects a duplicate terminal callback, while preserving the existing delegate invocation only for the first callback.Source: Coding guidelines
🤖 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.
Nitpick comments:
In
`@auth0/src/main/java/com/auth0/android/authentication/storage/BaseCredentialsManager.kt`:
- Around line 429-445: Add a concise comment above the broad catch in
runCatchingOnExecutor explaining that it intentionally acts as a last-resort
bulkhead for unexpected callback or bridge failures while preserving a single
terminal callback. Also replace the hardcoded "BaseCredentialsManager" Log.e tag
with the class-name pattern already used by nearby methods, without changing the
existing fatal-error filtering or callback behavior.
- Around line 447-468: Update SingleShotCallback’s onSuccess and onFailure
methods to log a brief entry whenever handled.compareAndSet rejects a duplicate
terminal callback, while preserving the existing delegate invocation only for
the first callback.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 981de0dc-1e38-4b49-8bb3-5fe6212fbd64
📒 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
2 participants
Why
Customers report a fatal, uncaught
IllegalStateException: "Already resumed"originating in the credentials managersRoot cause
An executor block can invoke a terminal callback (
onSuccess) and then have aThrowableescape after it — e.g. incontinueGetCredentials,callback.onSuccess(...)fires, and the refresh-branch innercatchonly handlesAuthenticationException, so any otherThrowablepropagates up torunCatchingOnExecutor, which then callscallback.onFailure(...). That is two terminal callbacks for one request.Through the
suspendCancellableCoroutinebridges (awaitCredentials/awaitSsoCredentials/awaitApiCredentials), the first callback resumes the continuation and the second resumes it again. A second resume throws"Already resumed"on the single-threaded serial executor, where nothing catches it → process death.Fix (defense in depth)
Layer A — root cause (
BaseCredentialsManager)runCatchingOnExecutornow wraps the callback in a newSingleShotCallback(backed byAtomicBoolean), so any terminal callback after the first is silently dropped — upholding the fire-once contract ofCallbackeven when a block both invokes the callback and later throws. Fatal errors (VirtualMachineError/ThreadDeath/LinkageError) are still rethrown. All call sites shadow the block's callback parameter so existing innercallback.xxxcalls route through the guard with no body edits.Layer B — bridge safety net (
CredentialsManager,SecureCredentialsManager)Every coroutine bridge now guards its resume with
if (continuation.isActive), so even a double callback reaching a bridge directly can never resume the continuation twice.Tests
Added regression tests to both
CredentialsManagerTestandSecureCredentialsManagerTest:shouldNotInvokeCallbackTwiceWhenConsumerThrowsAfterSuccessOnGetCredentials— covers Layer A via the callback API.shouldNotResumeContinuationTwiceWhenCallbackFiresTwiceOnAwaitCredentials— covers Layer B via the coroutine/awaitAPI.Each test was verified to fail with the respective guard removed (reproducing
IllegalStateException: Already resumed) and pass with the fix.