MOB-1832: Apply server switch hysteresis to automatic server selection

[nesence-m]

Summary

App half of the server-switch hysteresis for automatic server selection, tracked in MOB-1832 and mirroring iOS (zodl-ios #2049). Companion SDK PR: zodl-inc/zodl-android-wallet-sdk#14 (this branch pins it via SDK_COMMIT_PIN).

• AutomaticServerRepositoryImpl.init() is now a single lane: every foreground edge (including the first real foreground on launch) calls the SDK's evaluateServerSwitch(current, allBundledEndpoints, 5 s, 1 block); a new edge cancels an in-flight benchmark (mapLatest). A non-null answer is re-validated (automatic still on, no transaction in flight, candidate still bundled) and applied through the existing updateWalletEndpoint pipeline. A null answer changes nothing.
• Previously any benchmark winner that differed from the persisted endpoint triggered a Synchronizer rebuild, even one measured 5 ms faster. Now a switch needs ≥ 200 ms and ≥ 25 % improvement, or an unhealthy current server (policy and thresholds live in the SDK).
• The settings-screen benchmark and the manual "Automatic" save path (plain winner, no hysteresis) are unchanged.

Merged with maint/v3.10.x after #2489 (MOB-1723) landed: its balance-snapshot gate now lives inside the hysteresis pipeline, so a switch candidate waits for the first local balance snapshot (inside the cancellable mapLatest block) before it is applied; resolveAutomaticServerCandidate went away with the fastestEndpoints lane it gated. Shipping on 3.10.x requires an SDK release from maint/v3.1.x first, then clearing SDK_COMMIT_PIN and bumping ZCASH_SDK_VERSION.

Test plan

• AutomaticServerRepositoryTest (12 tests): automatic off → no SDK call; in-transaction state → no SDK call; SDK null/endpoint passed through; SDK called with current endpoint, every bundled candidate, 5 s and 1 block; candidate waits for / is released by the local balance snapshot
• ./gradlew :zashi-android:ktlint :zashi-android:detektAll clean (from the task-workspace root)
• ./gradlew :zashi-android:app:assembleZcashmainnetStoreDebug against the pinned SDK branch
• Device check: background/foreground the app, confirm one Server Switch: log line per foreground and no wallet reconnect when the decision is "stay"

Note
This code review checklist is intended to serve as a starting point for the author and reviewer, although it may not be appropriate for all types of changes (e.g. fixing a spelling typo in documentation). For more in-depth discussion of how we think about code review, please see Code Review Guidelines.

Author

• Self-review your own code in GitHub's web interface¹
• Add automated tests as appropriate
• Update the manual tests² as appropriate
• Check the code coverage³ report for the automated tests
• Update documentation as appropriate (e.g README.md, Architecture.md, CHANGELOG.md, etc.)
• Run the app and try the changes
• Pull in the latest changes from the main branch and squash your commits before assigning a reviewer⁴

Note
It is good practice to provide before and after UI screenshots in the description of this PR. This is only applicable for changes that modify the UI.

Reviewer

• Check the code with the Code Review Guidelines checklist
• Perform an ad hoc review⁵
• Review the automated tests
• Review the manual tests
• Review the documentation, README.md, Architecture.md, etc. as appropriate
• Run the app and try the changes⁶

https://claude.ai/code/session_01PYn1vxkynZJHRJQWVSkvtE

Footnotes

1. Code often looks different when reviewing the diff in a browser, making it easier to spot potential bugs. ↩

2. While we aim for automated testing of the application, some aspects require manual testing. If you had to manually test something during development of this pull request, write those steps down. ↩

3. While we are not looking for perfect coverage, the tool can point out potential cases that have been missed. Code coverage can be generated with: ./gradlew check for Kotlin modules and ./gradlew connectedCheck -PIS_ANDROID_INSTRUMENTATION_TEST_COVERAGE_ENABLED=true for Android modules. ↩

4. Having your code up to date and squashed will make it easier for others to review. Use best judgement when squashing commits, as some changes (such as refactoring) might be easier to review as a separate commit. ↩

5. In addition to a first pass using the code review guidelines, do a second pass using your best judgement and experience which may identify additional questions or comments. Research shows that code review is most effective when done in multiple passes, where reviewers look for different things through each pass. ↩

6. While the CI server runs the app to look for build failures or crashes, humans running the app are more likely to notice unexpected log messages, UI inconsistencies, or bad output data. Perform this step last, after verifying the code changes are safe to run locally.
   🤖 Generated with Claude Code ↩

[linear]

MOB-1832

[nesence-m]

This app PR builds against the SDK companion via SDK_COMMIT_PIN pointing at zodl-inc/zodl-android-wallet-sdk#14, and must land only after the SDK side ships.

[LukasKorba]

Reviewed together with zodl-inc/zodl-android-wallet-sdk#14 — most of the substance is over there (the failure path has no hysteresis, so A→B→A flapping is still reachable; plus two gRPC leak paths). This side is smaller, but one blocking item and a bad merge artifact.

Blocking — the only lane has no error handling

AutomaticServerRepository.kt:104-111:

applicationStateProvider.observeOnForeground()
    .mapLatest { resolveSwitchCandidate() }
    .filterNotNull()
    .onEach { applyServerSwitch(it) }
    .launchIn(scope)

No .catch, no runCatching, no retry. resolveSwitchCandidate (:117-129) reaches a DataStore read (isServerAutomatic()), an encrypted-prefs read (getPersistableWallet()), and a multi-endpoint network benchmark. Any throw kills the flow — a StatusRuntimeException escaping the un-runCatching'd getLatestBlockHeight at FastestServerFetcher.kt:291, an IOException from prefs, or the IllegalArgumentException from the SDK's require at :96.

init() is called once, and scope is CoroutineScope(Dispatchers.IO + SupervisorJob()) (:48) with no CoroutineExceptionHandler — so the failure goes to the global handler and then the thread's uncaught handler. Likely a crash; at best automatic server selection is permanently dead until process restart, silently. Wrap resolveSwitchCandidate() in runCatching and log.

Should-fix

gradle.properties:112 — the merge reverted this comment to the wrong upstream repo. Base maint/v3.10.x reads # Optional commit hash on zodl-inc/zodl-android-wallet-sdk.; this PR changes it to zcash/zcash-android-wallet-sdk. Nothing in the description mentions it and neither commit needed it — looks like a bad conflict resolution in merge commit 69f48b5. The pinned SHA ba6d49dd… doesn't exist in zcash/zcash-android-wallet-sdk, so the comment now points a reader at a repo where the pin is unresolvable. Revert that line.

The fake Synchronizer can't fail, so the blocking item above is invisible to the suite — every stub in AutomaticServerRepositoryTest.kt is returns null or returns known.last(), no throws case, and nothing tests that a cancelled mapLatest leaves nothing applied. The 12 tests do faithfully pin the arguments (sdkIsCalledWithTheCurrentEndpointAndEveryKnownCandidate — current, all known, 5.seconds, blocksToFetch = 1) and the balance-snapshot gate, which is the right thing to pin.

Merge order — yes, this needs unpinning before it goes anywhere

1. SDK #14 lands on maint/v3.1.x first — this can't compile without it, since Synchronizer gains an abstract evaluateServerSwitch (breaking for every implementer; the CHANGELOG flags it correctly and SlipstreamSynchronizer.kt:933-944 delegates properly).
2. Cut an SDK release from maint/v3.1.x.
3. Immediately after #14 merges: repoint SDK_COMMIT_PIN — gradle.properties:127 currently pins ba6d49dd…, which is the SDK PR head, i.e. a feature-branch commit that stops being fetchable once GitHub deletes the branch. That violates the constraint written a few lines above it in the same file ("must be reachable from something more durable than the SDK's own PR branch"). Same for the base branch's stale 0865690d… if it's in the same state.
4. Before this reaches release/* or candidate/*: clear SDK_COMMIT_PIN entirely and bump ZCASH_SDK_VERSION from 3.1.1-SNAPSHOT to the released version. Per the file's own comment CI hard-fails on a non-blank pin there, so it's a gate, not a nicety.

Base branches pair correctly (SDK maint/v3.1.x ↔ app maint/v3.10.x at 3.1.1-SNAPSHOT), and the call site itself is clean — named arguments throughout (:123-128), Duration makes a ms/s mixup impossible, the explicit 5.seconds/1 match the SDK defaults, nullable return handled. The one real integration gap is the unenforced "candidates must include current" invariant, which I've written up on the SDK PR — worth fixing on whichever side you prefer, but not leaving to a doc comment.

[nesence-m]

Thanks — addressed in d080105c58b2618c8cdf9495908d15151801713c, with the SDK half in zodl-inc/zodl-android-wallet-sdk#14 at 6a30e60f3fb39c435893c00ef25e2334d1657110. Item → change below.

Blocking — the only lane has no error handling

Fixed. init() now launches observeSwitchCandidates(...), and both halves of the lane run inside a guarded(...) helper: runCatching, rethrow CancellationException so mapLatest keeps its semantics, Twig.error, emit null. I guarded applyServerSwitch as well as resolveSwitchCandidate — it reaches updateWalletEndpoint, so leaving it bare would have left the same hole one operator downstream.

The lane moved into observeSwitchCandidates(foregroundEdges: Flow<Unit>) — the flow init() launches — purely so this is unit-testable without injecting the repository's internal Dispatchers.IO scope. (A constructor seam would have meant dropping singleOf(::AutomaticServerRepositoryImpl) in RepositoryModule, since singleOf doesn't honour defaulted params.) That retires the "exercised by the instrumentation test rather than here" caveat in the test KDoc.

New theLaneSurvivesAFailedEvaluationAndKeepsServingTheNextEdge: the fake Synchronizer.evaluateServerSwitch throws on the first foreground edge, nothing is applied, and the lane still applies the switch on a subsequent edge. I checked it isn't vacuous — remove the runCatching and it fails. theLaneSurvivesAFailureToApplyTheSwitch does the same for the apply side.

Should-fix

Minimum interval between evaluations. Added, since observeOnForeground() has no throttle and a full evaluation opens a connection to every bundled host. New internal EvaluationInterval (10 minutes, TimeSource-injectable) filters the foreground edges. markCompleted() fires only after the SDK benchmark returns normally, so a cancelled evaluation doesn't burn the window and a throwing one is retried on the next edge rather than being silently rate-limited. Covered by aSecondForegroundEdgeInsideTheEvaluationIntervalIsNotEvaluated plus four direct EvaluationIntervalTest cases with a TestTimeSource for both directions.

gradle.properties:112. Reverted to the base's # Optional commit hash on zodl-inc/zodl-android-wallet-sdk. — good catch, that was a bad conflict resolution in 69f48b5, and you're right that the pinned sha doesn't exist in zcash/zcash-android-wallet-sdk.

The fake Synchronizer can't fail. Covered by the two new lane tests above.

The unenforced "candidates must include current" invariant. Fixed on the SDK side rather than here, so the app can't get it wrong: evaluateServerSwitch now unions current into the benchmarked set and returns a distinct CurrentNotOffered outcome, so a host dropped from the bundled list is measured instead of force-switched. Details on the SDK PR.

Merge order — acknowledged, exactly as you laid it out

1. MOB-1832: Server switch hysteresis (evaluateServerSwitch) and real block-stream benchmarking zodl-android-wallet-sdk#14 lands on maint/v3.1.x first; this can't compile without it.
2. SDK release cut from maint/v3.1.x.
3. Immediately after Add Splash Screen #14 merges: SDK_COMMIT_PIN gets repointed to the merged sha on origin/maint/v3.1.x. Right now it's at 6a30e60f…, still the SDK PR head — same feature-branch-commit problem you flagged, and I'm leaving it that way only because CI's companion build needs something resolvable while Add Splash Screen #14 is open. That repoint will be a separate follow-up commit on this branch once Add Splash Screen #14 merges, not folded into this one, so it's an obvious one-line diff to re-review. Same for the base branch's stale 0865690d….
4. Before this reaches release/* or candidate/*: SDK_COMMIT_PIN cleared entirely and ZCASH_SDK_VERSION bumped off 3.1.1-SNAPSHOT to the released version. Understood that CI hard-fails on a non-blank pin there — it's a gate, not a nicety.

Gates: ./gradlew ktlintFormat detektAll green; :ui-lib:testZcashmainnetStoreDebugUnitTest green, built against the SDK worktree at the pinned commit via ORG_GRADLE_PROJECT_SDK_INCLUDED_BUILD_PATH.

[LukasKorba]

Lane protection and the comment revert are both properly fixed. The substantive findings this round are cross-repo, so they're written up on zodl-inc/zodl-android-wallet-sdk#14 — the short version is that the SDK's 10-minute cooldown can never fire on this app's path, because evaluationInterval.markCompleted() (:158) runs after the SDK has already stamped lastSwitchAt, and both windows are 10 minutes. K=2 is the only hysteresis actually running. Two more over there: markCompleted() only fires on success, so a user toggling foreground faster than an evaluation completes gets an unthrottled full benchmark every edge; and TimeSource.Monotonic at :177 doesn't advance in deep sleep, so an overnight sleep can filter out the morning foreground.

A1 fixed properly. :121-128 — guarded(...) wraps both resolveSwitchCandidate() and applyServerSwitch(candidate), and guarded (:130-139) rethrows CancellationException at :136 before logging, so mapLatest semantics are preserved. Failures return null and get dropped by filterNotNull(), lane stays alive for the next edge. The mockk tests with explicit throws (:207-227, :230-244) are the right shape, and asserting that the next edge still applies a switch is the assertion that matters.

Two leftovers: observeOnForeground() itself is still uncaught, so a throw from the upstream flow rather than the transform still kills the lane at :106 (low). And there's still no test that a cancelled mapLatest leaves nothing applied — theLaneSurvivesAFailureToApplyTheSwitch (:230) asserts one call happened, not that the lane survived, because the 10-minute interval makes a second edge unobservable in that test.

A2 fixed — gradle.properties:112 reads zodl-inc/zodl-android-wallet-sdk again.

A4 — not fixed, and it can't be yet. To be clear, I don't read this as a misrepresentation: your commit message says "repoints SDK_COMMIT_PIN at the companion SDK commit", which is exactly what happened. But gradle.properties:127 is 6a30e60f…, and I resolved it against the SDK repo:

• branches-where-head → feature/mob-1832-server-switch-hysteresis, and nothing else
• compare maint/v3.1.x...6a30e60f → ahead_by: 4, behind_by: 0, so it is not an ancestor of the maint branch

It's still a feature-branch commit that stops being fetchable when the branch is deleted, which is what the constraint at gradle.properties:123-126 exists to prevent. Unchanged sequence: merge #14 → repoint to the merged SHA on origin/maint/v3.1.x → blank the pin and bump ZCASH_SDK_VERSION off 3.1.1-SNAPSHOT (:206) before this touches release/* or candidate/*.

[nesence-m]

Thanks — second round addressed in ea9142e9. The cross-repo half is in zodl-inc/zodl-android-wallet-sdk#14 (ed11a76d); this side does four things.

markCompleted() only on success → renamed to markAttempted() and moved into a finally around the SDK benchmark call. An evaluation that throws, or that mapLatest cancels on the next foreground edge, did the network work all the same and now restarts the interval. The early returns before the benchmark (manual selection, no wallet, no candidates, no Synchronizer) deliberately do not mark — nothing was benchmarked there.

Wrong monotonic clock → EvaluationInterval now measures with SystemClock.elapsedRealtime() through a small ElapsedRealtimeTimeSource : TimeSource, so deep sleep counts and an overnight sleep no longer filters out the morning foreground edge. It is a TimeSource, so the tests keep using TestTimeSource and stay deterministic; the interval is injected (registered in RepositoryModule) rather than constructed inline, which is what lets a test advance past it.

Unbounded balance-snapshot wait → withTimeoutOrNull(30.seconds) { walletBalances.filterNotNull().first() }, and on timeout the switch is applied anyway. That is the deliberate choice for exactly the case you named: after two failed evaluations the current server is plausibly why there is no snapshot, so the failover that matters most cannot be gated forever on the sync it is meant to repair — a snapshot that is merely late costs the UI a redraw, a failover that never happens costs the user their sync. Which path was taken is logged (warn on timeout, info on a real snapshot). Both paths tested: switchCandidateWaitsForTheLocalBalanceSnapshot and switchIsAppliedAnywayWhenTheLocalBalanceSnapshotNeverArrives.

Confirming the switch → applyServerSwitch calls synchronizer.confirmServerSwitch(candidate) after updateWalletEndpoint has actually written the endpoint, so a declined switch (automatic/transaction re-check, membership) never confirms. The Synchronizer reference is taken before the write, since the write tears it down and rebuilds it; which instance receives the confirmation does not matter, the state it updates is process-wide. Tests: anAppliedSwitchIsConfirmedToTheSdk and aDeclinedSwitchIsNotConfirmedToTheSdk.

Leftovers from your review:

• observeOnForeground() itself is now covered — .catch { Twig.error(...) } sits above the filter, so a throw from upstream of the transform logs and does not take the lane down.
• The missing cancellation test exists: aCancelledEvaluationAppliesNothing hangs the SDK call with awaitCancellation(), cancels the lane, and asserts neither updateWalletEndpoint nor confirmServerSwitch ran. aFailedEvaluationStillCountsAgainstTheEvaluationInterval pins the finally behaviour.

A4 — agreed and unchanged: SDK_COMMIT_PIN is repointed to the new SDK head ed11a76d1ff064950689a483f6f16881e8d4c396, still a feature-branch commit. The durable repoint to the merged sha on origin/maint/v3.1.x stays a post-merge follow-up, and blanking the pin plus the ZCASH_SDK_VERSION bump stays the gate before this touches release/* or candidate/*.

Gates: ktlintFormat detektAll, :ui-lib:testZcashmainnetStoreDebugUnitTest — AutomaticServerRepositoryTest 20 tests and EvaluationIntervalTest 4, all green, built against the SDK worktree at ed11a76d.

[LukasKorba]

All three app findings fixed correctly, and the confirm handshake is called on exactly the right paths. Good from my side — the remaining work is on zodl-inc/zodl-android-wallet-sdk#14 (the SDK cooldown still uses TimeSource.Monotonic, which your own KDoc at :227-230 explains is the wrong clock — so a phone that sleeps overnight can have the app's interval elapse while the SDK's cooldown hasn't, deferring the morning failover off a dead server for hours).

N2 — markAttempted() in the finally at :180-182, and it's non-suspending so it runs on the cancellation path too. I checked the starvation risk I warned about and it isn't reachable: a user toggling every 30s gets edges dropped for 10 minutes, then the next window opens an attempt again — it retries once per 10 min indefinitely and never latches off. Early returns at :168-172 sit before the try and so don't mark, which is right: no network work was done.

N3 — ElapsedRealtimeTimeSource at :232-240, wired as the default at :215. The rationale comment is the clearest thing in the diff.

N7 — bounded at 30s (:247), and you made the right call on semantics: the switch still applies on timeout and which path was taken is logged (:152-164). That's correct for the CurrentUnhealthy case, which is exactly why the unbounded wait was the wrong gate. Both ends pinned.

The handshake. Taking the synchronizer before the write (:197 vs :199) is load-bearing and the comment at :185-192 gets it right — getSynchronizerOrNull() suspends on filterNotNull().first(), so calling it after would block until the rebuild finished. I traced the decline, throw and process-death paths and they all preserve the count correctly.

Two lows, neither blocking:

• The cancellation gap at :199-200 is closed by accident, not by design. markAttempted() runs in evaluateServerSwitch's finally, before the balance wait and before applyServerSwitch, so by the time you reach the write lastAttemptAt is ~30s old and the filter at :128 drops every edge for ~9.5 min — meaning nothing can cancel you between the write and the confirm. That holds today, but it depends on MINIMUM_EVALUATION_INTERVAL staying long and the filter staying where it is. If a confirm is ever missed after a successful write and the new server also fails to measure, the stale count of 2 increments to 3, already past threshold, with no cooldown — a switch every 10 minutes walking the host list. Cheap insurance: withContext(NonCancellable) { updateWalletEndpoint(candidate); synchronizer?.confirmServerSwitch(candidate) }.
• confirmServerSwitch fires even when updateWalletEndpoint no-ops. WalletRepository.kt:218-223 returns silently on a null wallet or an unchanged endpoint, and :200 confirms unconditionally — starting a 30-minute cooldown for a switch that never happened, which is the hazard your own SDK doc at Synchronizer.kt:285-286 warns about. Narrow reachability, but returning a Boolean from updateWalletEndpoint and gating the confirm on it would be exact.

Nit: EvaluationInterval.lastAttemptAt (:217) is a non-volatile var written from the mapLatest child and read from the collector in filter, on a shared Koin single. Worst case is one extra benchmark and dispatch makes a stale read unlikely, but @Volatile is free.

Pin — correctly repointed to ed11a76, the new SDK head. Still a feature-branch SHA, as expected; same sequence stands: merge #14 → repoint to the merged SHA on origin/maint/v3.1.x → blank the pin and bump ZCASH_SDK_VERSION before release/* or candidate/*.

CI: Keystone is red on every completed run of e2e-smoke.yml across six unrelated branches over the last three days — repo-wide, not this PR. test_android_modules_emulator (app) shows AdbCommandRejectedException: device offline and "Expected 12 tests, received 0", i.e. the emulator died rather than an assertion failing; the prior head passed the same job. Worth one rerun, but the signature is infra.