T10/T11: location seam and recording pipeline; fix a native crash-recovery bug
T10 -- LocationSource abstraction plus FakeLocationSource, so the riskiest code in the app is testable with no device. Also found that geolocator ships its own Android foreground service with enableWakeLock, which could drop flutter_foreground_task and both its deprecation paths; deferred to T12 since it costs notification actions. T11 -- the pipeline. Kotlin's unbounded Channel plus blocking-receive writer becomes a List buffer plus a periodic Timer; Dart's event loop makes a blocking receive unnecessary and the guarantee is unchanged, since the fix callback only appends and returns. 24 tests. Found a real bug in the native app while porting. restoreAfterProcessDeath says it resumes into a new segment because the dead time is a real gap, but it calls resumeTrip, which adopts the segment a crash left open. After a pause that is right; after a crash nothing closed it. Points either side of the dead time then share a segment, and computeSummary -- the authoritative pass that overwrites the live estimate on completion -- measures straight through the gap. Reverting the fix and running the guard shows 111,217 m of phantom distance. Fixed via TripRepository.resumeIntoNewSegment, which closes the stale segment at its last recorded point rather than at now, so the dead time is not billed as ride time either. A deliberate, documented departure from parity: the code contradicted its own comment and produced silently wrong data. The first version of that guard passed with the bug still present -- the fixture used pause(), which closes the segment and stops reproducing the crash. Caught only by reverting the fix and checking the test failed. 145 tests passing, analyze clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
@@ -368,3 +368,111 @@ statistics and export layers above it are proven equivalent to the Kotlin origin
|
||||
Next: **Phase 3, the recording engine** — the risky phase. T10's `LocationSource` seam
|
||||
first, then the pipeline, then the two platform liveness stories. The
|
||||
`flutter_foreground_task` deprecation warnings recorded under T01 become relevant at T12.
|
||||
|
||||
---
|
||||
|
||||
## T10 — Location source seam · **complete**
|
||||
|
||||
`lib/src/recording/location_source.dart`: `LocationFix`, `LocationSource`,
|
||||
`LocationException`, and `FakeLocationSource`.
|
||||
|
||||
`LocationFix` is deliberately **not** `TrackPoint` — a fix has no trip or segment
|
||||
identity. Those ids are stamped on by the engine at creation, which is the whole basis of
|
||||
the pause guarantee.
|
||||
|
||||
### `geolocator` can replace `flutter_foreground_task` entirely
|
||||
|
||||
`geolocator_android` ships `ForegroundNotificationConfig`, which raises a foreground
|
||||
service with `foregroundServiceType=location` for as long as the position stream is
|
||||
active, and exposes `enableWakeLock` and `setOngoing`. That is **everything**
|
||||
`TrackingService` used a foreground service and a `PARTIAL_WAKE_LOCK` for.
|
||||
|
||||
Dropping `flutter_foreground_task` would remove both deprecation paths recorded under
|
||||
T01 (no Swift Package Manager on iOS; applies KGP on Android) at no cost to the design.
|
||||
|
||||
**One parity casualty:** geolocator's notification config has no support for *actions*,
|
||||
so the notification would be display-only — the native app's Pause/Resume buttons in the
|
||||
shade would be lost. Decision deferred to T12, where it actually bites. The seam means
|
||||
neither choice touches the engine.
|
||||
|
||||
---
|
||||
|
||||
## T11 — Recording pipeline · **complete**
|
||||
|
||||
`lib/src/recording/recording_engine.dart`. **24 tests.**
|
||||
|
||||
The Kotlin `Channel(UNLIMITED)` + blocking-`receive` writer coroutine becomes a plain
|
||||
`List` buffer plus a periodic `Timer`. Dart has no blocking receive and its single
|
||||
threaded event loop makes one unnecessary — the guarantee is unchanged: the fix callback
|
||||
only appends and returns, so disk latency can never stall GPS. `Mutex` becomes
|
||||
`synchronized`'s `Lock`, serialising the periodic flush against explicit drains at pause,
|
||||
stop and discard.
|
||||
|
||||
All five actions ported: start (adopting), pause, resume (via start), stop, discard, plus
|
||||
`restoreAfterProcessDeath`.
|
||||
|
||||
---
|
||||
|
||||
## 🐞 A real bug found in the native app
|
||||
|
||||
**`TrackingService.restoreAfterProcessDeath` does not do what its comment says.**
|
||||
|
||||
```kotlin
|
||||
// Resume into a *new* segment: the time the process was dead is a real
|
||||
// gap in the recording and should render as one.
|
||||
trips.resumeTrip(System.currentTimeMillis())?.let { handle ->
|
||||
```
|
||||
|
||||
`resumeTrip` calls `adoptOrOpenSegment`, which returns the **existing open segment** if
|
||||
there is one. After a *pause* that is correct, because pausing closes the segment first.
|
||||
After a *crash* nothing closed it — so the adopt branch wins and the stated intent is
|
||||
silently not met.
|
||||
|
||||
### The consequence is data corruption, not cosmetics
|
||||
|
||||
Points either side of the dead time land in one segment. `RideStatistics.compute` groups
|
||||
by segment, and it is the **authoritative** pass that overwrites the live estimate when
|
||||
the trip completes. So the gap gets measured as if it had been ridden.
|
||||
|
||||
Measured, by reverting the fix and running the guard:
|
||||
|
||||
```
|
||||
the dead time leaked into distance: 111217.31924957958 m
|
||||
```
|
||||
|
||||
**111 km of phantom distance** added to a ride because the process died and the rider
|
||||
relaunched somewhere else. The map would also draw a straight line across roads never
|
||||
ridden — exactly the artefact segments exist to prevent.
|
||||
|
||||
The live accumulator gets this right (it is re-seeded with a null anchor). The
|
||||
authoritative recomputation then overwrites the correct figure with the wrong one.
|
||||
|
||||
### The fix
|
||||
|
||||
`TripRepository.resumeIntoNewSegment` closes the stale segment and opens a fresh one.
|
||||
The stale segment is closed **at its last recorded point**, not at `now` — recording
|
||||
genuinely stopped when the process died, and `computeSummary` sums closed segment spans
|
||||
for elapsed time, so closing at `now` would bill the dead time as ride time.
|
||||
|
||||
This is a deliberate, documented **departure from parity**. The plan says port bugs
|
||||
faithfully, and that holds for the elevation drift where a "fix" would make differential
|
||||
testing ambiguous. It does not hold here: the code contradicts its own stated intent and
|
||||
the result is silently wrong data.
|
||||
|
||||
> Worth carrying back to the native app if it is ever revived. Second finding of its kind
|
||||
> after the seed-lucky elevation bound.
|
||||
|
||||
### And a near-miss worth recording
|
||||
|
||||
The first version of the guard **passed with the bug still present**. The fixture called
|
||||
`pause()` to flush points to disk — but pausing *closes* the segment, so the crash state
|
||||
was never reproduced. It was only caught by deliberately reverting the fix and checking
|
||||
the test failed.
|
||||
|
||||
The rewritten fixture writes points through the repository directly, leaving the segment
|
||||
open exactly as a crash does. It now fails at 111 km with the native behaviour and passes
|
||||
with the fix.
|
||||
|
||||
**A regression test nobody has watched fail is not yet a regression test.**
|
||||
|
||||
**145 tests passing, analyze clean.**
|
||||
|
||||
Reference in New Issue
Block a user