Skip to content

General: Name the gesture in onboarding and test the dispatch core - #32

Merged
d4rken merged 5 commits into
mainfrom
refactor/dispatch-coordinator
Jul 27, 2026
Merged

General: Name the gesture in onboarding and test the dispatch core#32
d4rken merged 5 commits into
mainfrom
refactor/dispatch-coordinator

Conversation

@d4rken

@d4rken d4rken commented Jul 27, 2026

Copy link
Copy Markdown
Member

What changed

User-visible: one string. The onboarding feature page called the unplug/replug feature an "Optional cable shortcut"; it is now "Optional reconnect gesture", matching what the dashboard, the settings screen and the ongoing notification have called it since #30. That was the last surface still using a different name.

Everything else is internal, with no user-facing behaviour change: the charge-session service's dispatch core is now a separate, unit-testable class, and two new test suites cover behaviour that previously had none.

Technical Context

Dispatch core extraction

ChargeSessionService kept its two unbounded FIFO queues, their single consumers, the one shared lock and the monitoring generation/ready gate as private fields, so none of it could be tested without a device. That machinery is now DispatchCoordinator<C, E> — generic and free of Android types, because the payloads (Intent, elapsedRealtime()) stay in the service. That is what makes it a plain JVM unit.

This is a behaviour-preserving refactor, deliberately mapped 1:1 onto the previous call sites, on a specialUse foreground service that controls real charge hardware. Three properties were treated as load-bearing rather than incidental, because each is easy to "clean up" into a bug:

  • shutdown() closes the queues and does nothing else. It must not cancel the consumers. Closing lets an in-flight handler finish; stopping them is scope.cancel()'s job, and onDestroy still calls it immediately afterwards. Cancelling consumers there would release the shared lock mid-handler and let the recovery-job tail — parked in withExclusive — run continueGestureOrStop() during teardown, reopening monitoring on a dying service.
  • withExclusive is a bare, cancellable mutex.withLock. No NonCancellable, no retry, no swallowed cancellation: ACTION_START and ACTION_SET_PERSISTENT_POLICY cancel-and-join() the recovery job while already holding that lock, and only a cancelled waiter aborting its acquisition prevents deadlock.
  • launch() is single-shot, shutdown() is terminal. A second consumer per queue would race for the lock and silently destroy the FIFO ordering the queues exist to guarantee.

The gating asymmetry is preserved exactly: only the battery receiver gates on isOpen; the 30-second poll and the reconnect-window expiry nudge submit unconditionally, and the expiry nudge carries the generation captured when its window opened rather than the current one. Generation is filtered twice — before taking the lock and again inside it, since a command can restart monitoring in between. The counter moved to an AtomicInteger; that removes a latent non-atomic ++ but is not a substitute for the ordering invariant, which is documented on the class.

Test coverage

DispatchCoordinatorTest (17 cases) pins FIFO per stream, cross-stream exclusion, generation filtering including the post-lock re-check, cancellable acquisition, and the lifecycle rules. Exclusion is proven with handlers suspended on a CompletableDeferred — with non-suspending handlers on one test dispatcher, an "only one ran at a time" assertion holds even with the lock deleted.

Worth knowing for anyone writing coroutine tests here: advanceUntilIdle() only drains foreground events. These consumers are endless for (x in channel) loops, so they must live in runTest's backgroundScope — which made advanceUntilIdle() a no-op that executed zero tasks. runCurrent() has no tier filter and is the correct primitive. The suite was then mutation-tested to confirm it is not vacuous: splitting the shared lock fails 3 cases, removing the post-lock generation check fails 1, and wrapping withExclusive in NonCancellable hangs the cancellation test.

ChargingRepositoryPersistenceTest (Robolectric, real object graph — there is no mocking library here) pins that a temporary override cannot overwrite the saved persistent policy. Its baseline is deliberately Adaptive: protectivePolicyNow() defaults to FixedLimit(80), so applying 80 and asserting 80 would pass even if the repository wrote nothing. DeviceInfo.ACTION_CHARGING_OPTIMIZATION was widened privateinternal so the test references the constant instead of duplicating the action string.

Review guidance

The extraction is the risk surface; the mapping table is the thing to check, particularly the four close() sites, the single closeAndInvalidate(), and the two open() sites. Everything else is tests plus one string.

Verification

760 tests per flavour (739 + 21 new), lint for both flavours in beta and release, and both flavour debug assembles — verified from the test-result XML on disk, not just the task summary.

Device-verified on a Pixel 8 (Android 17): the gesture arms on live hardware evidence, an unplug/replug triggers a session, restore returns the protective policy, a sub-2-second replug is correctly rejected by the debounce floor, and the reconnect-window expiry nudge fires 10.549 s after unplug — far too early for the 30-second poll to account for it, which is what makes it evidence that the captured-generation path still works.

Not verified on hardware: the stale-generation race — a nudge captured for one monitoring run must not act on the next. Reaching it needs a monitoring restart inside a 10.5-second window; UI interaction was too slow, and the service is not exported so it cannot be driven from adb. It is covered by unit tests and by the mutation testing above, but has no on-device confirmation.

d4rken added 5 commits July 27, 2026 07:06
The onboarding feature card called the powered-off/replug shortcut a
"cable shortcut" while every other surface (settings, notifications,
dashboard) calls it the reconnect gesture. Use the same name here.
…ordinator

The charge-session service hand-rolled its dispatch: two unlimited
channels, one shared mutex, a volatile monitoring generation and a
volatile "monitor ready" flag, with the consumer loops, the double
generation check and the per-item error isolation inlined in onCreate.
None of it was reachable from a JVM test.

Move that mechanism into DispatchCoordinator<C, E>, a pure unit with no
Android types: two FIFO queues drained by one consumer each under a
single shared lock, generation stamping on submit and filtering both
before and inside the lock, and per-item error routing. The service
keeps its Intent/SystemClock payloads and maps 1:1 onto the new API —
no ordering, gating or error-handling change.

Three lifecycle rules are load-bearing and now documented at the type:
shutdown() closes the queues and nothing else (cancelling consumers
would let a waiter run during teardown, so the owner must cancel the
scope right after), withExclusive() is a bare cancellable delegation
(the cancel-and-join paths rely on a cancelled waiter aborting its
acquisition), and launch() is single-shot while shutdown() is terminal.

Adds DispatchCoordinatorTest covering FIFO per stream, both generation
checks, the open/closed asymmetry (only the battery receiver gates on
it), mutual exclusion via suspended handlers, cancellable acquisition
and the shutdown semantics.
ChargingPreferencesTest pins the facade's behaviour, but nothing
exercised the repository actually passing the right `persistent` flag
down. A session write mis-recorded as persistent would rewrite the
user's protective baseline and silently disable the any-level basis of
the reconnect gesture, and no test would have noticed.

Adds a Robolectric test over the real object graph — adapter registry
with real adapters, access resolver over the direct WRITE_SECURE_SETTINGS
backend, DataStore-backed preferences — with the Pixel capability gate
satisfied via Build fields, the telephony feature and a resolvable
charging-optimization activity. It asserts that a persistent apply moves
both signals, a temporary apply moves neither, a forced re-apply moves
both, and a write that never landed records nothing at all.

The baseline is Adaptive rather than FixedLimit(80) on purpose: the
protective policy defaults to FixedLimit(80), so an 80 baseline would
pass even if the repository recorded nothing.

DeviceInfo.ACTION_CHARGING_OPTIMIZATION becomes internal so the test
registers the real action instead of a copy that could drift.
Three KDoc blocks in stats/core described the charge-session service's
serialization as `commandMutex`, an identifier that no longer exists
after the dispatch core moved into DispatchCoordinator.
DispatchCoordinatorTest asserted against empty lists in every case that
expected work to have run: 11 of 17 tests failed.

The consumers are endless channel loops, so they have to be launched into
runTest's backgroundScope or the test would never finish. That tags every
task they schedule as background work, and TestCoroutineScheduler's
advanceUntilIdle() is defined as "advance until no FOREGROUND event is
left" (advanceUntilIdleOr { events.none(TestDispatchEvent::isForeground) }).
With the whole coordinator in the background there was never a foreground
event, so each call returned having executed nothing at all.

runCurrent() has no such filter - it drains every task queued at or before
the current virtual time, foreground and background alike. None of these
tests use virtual delays, so that is a full drain.

DispatchCoordinator itself is unchanged, and no assertion was relaxed.
Verified non-vacuous by mutation: dropping the post-lock generation
re-check fails the fast-path test, splitting the shared mutex fails all
three exclusion tests, and wrapping the acquisition in NonCancellable
hangs the cancelled-waiter test out to the runTest timeout.

Also drops two stray HotSpot crash dumps left in app/ by an earlier build.
@d4rken d4rken added enhancement New feature or request ROM: Pixel Google Pixel labels Jul 27, 2026
@d4rken
d4rken merged commit 3cb328c into main Jul 27, 2026
12 checks passed
@d4rken
d4rken deleted the refactor/dispatch-coordinator branch July 27, 2026 10:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request ROM: Pixel Google Pixel

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant