fix: classify notification reply send failures#1378
Conversation
Return a typed send outcome so the notification worker fails terminal errors immediately and only retries safe connect-phase relay failures within its existing bound. Fixes #1276
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✨ 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 |
|
Ready to review this PR? Stage has broken it down into 4 individual chapters for you:
Chapters generated by Stage for commit d1d8ce2 on Jul 14, 2026 6:11pm UTC. |
Preview APK for PR #1378Home-screen label: 1378 White Noise with the blueprint launcher icon — installs side-by-side with the production and staging apps. Install directly — one tap, no GitHub sign-in (public, content-addressed Every push to this PR rebuilds and updates this comment. |
agent-p1p
left a comment
There was a problem hiding this comment.
Pip adversarial review for #1276 at 85d7b0d6.
BLOCKING:
- none
SUGGESTION:
app/src/main/java/dev/ipf/whitenoise/android/state/AppState.kt:288-304—PERMANENT_FAILUREis broader than its name: the classifier intentionally puts both truly permanent errors and retry-unsafe/ambiguous failures into this bucket. For example,MarmotKitException.StorageBusyis typed as transient by the FFI but can surface after publication during local projection/persistence, where an automatic resend could duplicate the message. The current fail-fast behavior is correct; renaming this outcome toNON_RETRYABLE_FAILUREorFAIL_FASTwould encode the actual invariant and discourage unsafe broadening later.app/src/main/java/dev/ipf/whitenoise/android/notifications/NotificationReplyWorker.kt:64-79— focused tests cover the pure retry predicate, but not thedoWork()outcome-to-Resultwiring. A worker-level test for SENT, permanent/non-retryable, and transient outcomes would guard the cross-layer mapping this issue adds. Non-blocking because the mapping is small and the existing test style uses pure seams.
NITPICK:
- none
SUMMARY: The typed outcome fixes the Boolean collapse. Cancellation still propagates, permanent and retry-unsafe failures fail immediately, and only the existing narrow connect-phase classifier enters the bounded three-attempt WorkManager retry path. The raw notification send does not pass through the foreground controller retry loop, so there is no double retry. Focused verification passed 43/43 tests; all GitHub checks are green. No sensitive paths were touched.
VERDICT: APPROVE
SENSITIVE_PATHS: none
Rename the broad fail-fast outcome to reflect its non-retryable invariant and test the worker's exact success, retry, and failure mapping. Fixes #1276
|
Addressed both adversarial-review suggestions in d1d8ce2: renamed the fail-fast bucket to NON_RETRYABLE_FAILURE and added direct worker result-mapping coverage for sent, non-retryable, retryable, and exhausted-retry outcomes. Local full gates and fresh GitHub CI pass. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
Summary
false.Test plan
./gradlew :app:testDebugUnitTest --tests dev.ipf.whitenoise.android.state.TransientRelaySendErrorTest --tests dev.ipf.whitenoise.android.notifications.NotificationTargetTest./gradlew :app:testDevDebugUnitTest :app:lintDevDebug --no-daemon --stacktrace./gradlew :app:verifyRoborazziDevDebug --no-daemon --stacktrace./gradlew :app:ktlintCheck --no-daemon --stacktraceFixes #1276
Summary by CodeRabbit
Bug Fixes
Tests