Stats: Keep the current charging session across app restarts - #22
Merged
Conversation
A process death while plugged in restarted the live card's "Since …" at process-launch time: startup repair sealed every open row unconditionally, so the next tick opened a fresh session stamped with the current time and history gained a duplicate row for one physical charge. Android exposes no plug-in timestamp, but the row was already persisted. Startup repair now probes current battery state (via the existing BatteryReader sticky read) and reattaches the newest open row when the evidence is consistent with the same plug event, else seals as before. Continuity is inferred, not observed, so the guards are best-effort and biased toward merging: a replug at an unchanged level during the gap is indistinguishable from an uninterrupted plug. Two things keep that honest — a resumed row is always flagged partial, and its last power/temperature readings are dropped so the unobserved gap is never credited to the aggregates. A wrong merge costs an over-long duration, never invented averages. The probe runs synchronously in startup repair rather than on the first watcher tick: a row held open awaiting a tick renders as a live session and suppresses the "couldn't start capture" retry when the foreground service fails to start. Also fixed, all reachable before this change: - BootIdSource's -1 sentinel compares equal across two different boots, so an unknown boot id is now explicitly disqualifying rather than merely degraded — otherwise a resume could splice two boots' elapsed-realtime readings into one bogus duration. - ChargeStatsRepository.currentSession() keyed distinctUntilChangedBy on the row id alone while capturing the row inside flatMapLatest, so a partial flip on the same row was suppressed for the lifetime of the subscription. - StatsCardPresentation preferred Live whenever a row existed. A resumed row stays open by design, so a failed service start would show a frozen live card and hide the retry. A failed start now wins. - The lastCapture stamp is written after the session row is committed, so a death in between left an open row that startup repair skipped entirely, stranding it and letting the next tick open a second row. ChargeStatsRecorder gains an injected dispatcher (@StatsDispatcher) so the new Robolectric test can drive its command loop. The stats test classes now use per-test temp-file DataStores instead of sharing the app's real path. Verified on a Pixel 8 (resume across force-stop, unplugged reject, level-drop reject, and BOOT_MISMATCH plus REBOOT seal across a real reboot).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changed
If Amply's process was killed while your phone was still on the charger — a force stop, an app
update, or an aggressive OEM task killer — the "current charging session" card lost track of when
the charge actually began. On reopening the app, it showed "Since <the moment you reopened it>"
and the elapsed time started from zero, and the charge was split into two entries in the history.
The session now survives that. When the app comes back and the evidence says the same charge is
still running, it picks up the existing session: the real plug-in time, the starting percentage, and
one history entry for one charge.
Where it can't tell, it doesn't guess. If the charger was pulled in the meantime, or the battery
level dropped, or the phone rebooted, the old session is closed and a new one starts — the same as
before.
One honest limitation: if you unplug and replug at the same battery level while the app isn't
running, nothing observed the gap, so the two charges are counted as one. Sessions restored this way
are marked as partial, because the recorded curve has a hole in it.
Technical Context
Root cause. Startup repair sealed every open session row unconditionally, with an explicit
"never resumes" rationale. The next battery tick then had no open session and opened a fresh one
stamped with the current wall time — hence the wrong "Since" and the duplicate row.
There is no platform source for the plug-in timestamp (
ACTION_BATTERY_CHANGEDcarries none, theBatteryManagerproperties are all instantaneous, no portable sysfs node).dumpsys batterystatshistory has
+pluggedentries, but it needsDUMP, its format is device- and version-dependent, andit is reset by the platform — and routing it through the Shizuku service would mean widening a typed
boundary that today only does settings get/put. Not worth it: the data was already in
stats.db.Why the probe runs in startup repair. The decision needs current battery state, which the first
watcher tick would also provide — but a row held open while waiting for that tick renders as a live
session, and the card prefers
Liveover the "couldn't start capture" retry. A failed foregroundstart would then show a frozen card with no way out. Resolving synchronously costs one sticky
broadcast read and leaves the sampling path untouched.
Why a wrong merge is cheap. Resuming nulls the last power/temperature readings, so the interval
crediting skips the unobserved gap entirely instead of extrapolating up to 10 minutes of pre-death
values across it. The cost of a false merge is bounded to an over-long duration, never fabricated
averages.
Also fixed here, all reachable before this change:
-1, for devices that don't reportBOOT_COUNT) compares equal acrosstwo different boots. Left as a plain id it would have allowed a resume across a reboot, splicing
two boots'
elapsedRealtimereadings into one nonsensical duration. It is now explicitlydisqualifying.
distinctUntilChangedByon the row id alone while capturing the rowinside
flatMapLatest, so a field change on the same row was suppressed for the lifetime of thesubscription — the resumed-session flag would never have reached the card.
Since resumed rows stay open by design, that would have meant a permanently frozen card.
the two left an open row that startup repair skipped entirely — stranding it and letting the next
tick open a second row alongside it.
Review guidance. The decision itself is a pure function (
evaluateResume) with a sealedresult, unit-tested against each rejection reason; the orchestration around it is covered by a new
Robolectric test over in-memory Room.
ChargeStatsRecordergains an injected dispatcher — the firstin this codebase, deliberately qualified and scoped to the stats feature rather than introduced as an
app-wide binding — purely so that test can drive its command loop.
Unrelated but worth knowing: every stats test class was building a DataStore over the app's real file
path, so adding a class made pre-existing tests fail in a full-suite run. They now use per-test
temp-file DataStores, matching what the other DataStore tests already did.
Testing
585 unit tests across both flavors,
lintVitalFossRelease, and both debug assembles are green.Device pass on a Pixel 8 (Android API 37), driving plug state with
dumpsys battery:Two paths were not exercised on device — capture being disabled at startup, and a refused foreground
start — because neither race can be staged through adb. Both are covered by unit tests.