fix(crashlytics): close the four top 0.9.19/0.9.20 production issues - #163
Conversation
Each of these was a real defect that a foreground assumption or an
unbounded read made invisible until it hit a user's device.
1. IMPORT/EXPORT OOM. `importFromDb` ran `src.query(t)` per table and
held the whole result live for the insert loop. sqflite materialises
an entire result set as Java objects BEFORE any of it crosses the
channel, so importing another device's `decoded_onehz` (86,400 rows
per day) exhausted the 256 MB Dalvik heap — the production
`java.lang.OutOfMemoryError` blamed on a BLE binder callback, which
only ever surfaced there because that was the next thread to
allocate. Both paths now page on rowid (keyset, not OFFSET, which
would re-scan the prefix quadratically), one transaction per page.
Each decoded_onehz row's orphan guard still rides in the SAME
transaction as the row it guards — that invariant is per-row, not
per-table — and every write stays INSERT OR REPLACE keyed on the
row's own identity, so an interrupted import converges on re-run.
2. UNBOUNDED RR FALLBACK. `decodedRrByCounterRange`'s degraded branch
(endpoint row pruned) treats its arguments as a bare counter span,
and the strap's counter resets on reboot — a reboot-straddling page
degenerated to `0 .. ~1200000`, i.e. the whole table, on the same
heap. Capped, with `decodedRrFallbackTruncations` counting any short
read so truncated HRV input can never pass as a complete one.
3. BACKGROUND DERIVE TIMEOUTS. `day_blocks_failed` ("timed out after
0:01:30") came only from `IosBgTask._run`. Three concurrent day-lanes
win when there are spare cores; a throttled background slot has none,
so the lanes divided one budget three ways and a wall-clock timeout
turned "3 days derived" into "3 days timed out". Headless entries now
take one lane and a 4-minute budget via a pure `DerivePacing`; serial
lanes also cap peak memory at one day's substrate.
4. JANK FALSE POSITIVES. The watchdog gated on `FrameTiming.totalSpan`,
which spans vsync-start to raster-end and therefore includes the gap
across an app resume — producing "Slow frame: 16358ms (build=0
raster=8)", a 16-second stutter in which the engine did 8 ms of work.
That single class became the top issue by impacted users and buried
real signal. `JankPolicy` gates on build+raster, the cost the app
actually controls and the only one a fix would move; totalSpan is
still attached as context, with idle_ms, so a scheduling-delay
pattern stays visible.
Also fixes the ImportScreen double-tap crash: `_busy` only went true
once an import was RUNNING, leaving the option cards live while the
native sheet was still opening, so a second tap threw
`PlatformException(already_active)` from an unawaited context. Guarded
by a `_picking` flag plus a catch that refuses to let a picker failure
be fatal.
The three tuning/verdict decisions live in pure classes (DerivePacing,
JankPolicy) so they are unit-testable without an isolate, a background
slot, a SchedulerBinding or a Firebase app. 989 tests pass; analyze
clean. No analytics output changed, so kAlgoVersion stays at 50.
PR Reviewer Guide 🔍(Review updated until commit 8994285)Here are some key observations to aid the review process:
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe PR adds background-aware derivation pacing, paged database import/export, work-based jank detection, guarded file-picker state, and App Store Connect export settings. It also adds regression and integration tests for pacing, database transfers, and telemetry behavior. ChangesDerivation pacing
Paged database transfer
Work-based jank telemetry
Import picker state
App Store Connect export
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)Background derivation pacingsequenceDiagram
participant BackgroundTask
participant DerivationEngine
participant DerivePacing
BackgroundTask->>DerivationEngine: construct with background true
DerivationEngine->>DerivePacing: calculate timeout and concurrency
DerivePacing-->>DerivationEngine: return single-lane, extended pacing
DerivationEngine-->>BackgroundTask: run derivation
Paged database transfersequenceDiagram
participant LocalDb
participant SourceDatabase
participant DestinationDatabase
LocalDb->>SourceDatabase: fetch rowid page
LocalDb->>DestinationDatabase: insert filtered page in transaction
DestinationDatabase-->>LocalDb: commit page
LocalDb->>SourceDatabase: fetch next page
Work-based jank reportingsequenceDiagram
participant FrameTiming
participant JankPolicy
participant TelemetryService
FrameTiming->>JankPolicy: provide build, raster, and total durations
JankPolicy-->>TelemetryService: return report verdict and timing breakdown
TelemetryService->>TelemetryService: emit breadcrumb and non-fatal event when reporting
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
PR Code Suggestions ✨Latest suggestions up to 8994285
Previous suggestionsSuggestions up to commit cacf029
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/data/db.dart`:
- Around line 3016-3051: The copyPaged method currently pages filtered tables by
rowid, causing repeated scans of the remaining table. Update the day-ranged
export flow to use keyset pagination on the filtered indexed column: for
decoded_onehz use rec_ts as the cursor with rec_ts > lastRecTs and ORDER BY
rec_ts ASC, and for ts/start_ts tables use the appropriate (timestamp, rowid)
composite cursor. Preserve filtering, insertion, callbacks, and short-page
termination behavior.
- Around line 3321-3325: Update the first-page fetch error handling in the
import loop around nextPage() so only the specific missing-table
DatabaseException is treated as an absent source table and continues. Let
corruption, malformed data, I/O, and other read failures propagate, ensuring
counts[t] is not silently omitted and importEdgeBackup reports failure rather
than partial success.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 20b7c1a6-030c-4583-8109-6f634c92be66
📒 Files selected for processing (13)
ios/ExportOptions/AppStoreConnect.plistlib/compute/background_derivation.dartlib/compute/derivation_engine.dartlib/compute/derive_pacing.dartlib/data/db.dartlib/sync/background_sync.dartlib/sync/ios_bg_task.dartlib/telemetry/jank_policy.dartlib/telemetry/telemetry_service.dartlib/ui/import/import_screen.darttest/db_paged_import_export_test.darttest/derive_pacing_test.darttest/jank_policy_test.dart
| Future<void> copyPaged( | ||
| String table, { | ||
| String? where, | ||
| List<Object?> whereArgs = const [], | ||
| Future<void> Function(List<Map<String, Object?>> page)? onPage, | ||
| }) async { | ||
| final rows = await src.query(table, where: where, whereArgs: whereArgs); | ||
| if (rows.isEmpty) return; | ||
| await out.transaction((txn) async { | ||
| final batch = txn.batch(); | ||
| for (final row in rows) { | ||
| batch.insert( | ||
| table, | ||
| Map<String, Object?>.from(row), | ||
| conflictAlgorithm: ConflictAlgorithm.replace, | ||
| ); | ||
| } | ||
| await batch.commit(noResult: true); | ||
| }); | ||
| } | ||
|
|
||
| Future<void> copyRawRange(int startSec, int endSec) async { | ||
| final decoded = await src.query( | ||
| 'decoded_onehz', | ||
| where: 'rec_ts >= ? AND rec_ts < ?', | ||
| whereArgs: [startSec, endSec], | ||
| ); | ||
| if (decoded.isNotEmpty) { | ||
| var lastRowid = 0; | ||
| while (true) { | ||
| final clause = where == null ? '' : 'AND ($where) '; | ||
| final page = await src.rawQuery( | ||
| 'SELECT rowid AS $rowidKey, * FROM $table ' | ||
| 'WHERE rowid > ? $clause' | ||
| 'ORDER BY rowid ASC LIMIT ?', | ||
| [lastRowid, ...whereArgs, exportPageSize], | ||
| ); | ||
| if (page.isEmpty) return; | ||
| await out.transaction((txn) async { | ||
| final batch = txn.batch(); | ||
| for (final row in decoded) { | ||
| for (final row in page) { | ||
| final clean = <String, Object?>{ | ||
| for (final e in row.entries) | ||
| if (e.key != rowidKey) e.key: e.value, | ||
| }; | ||
| batch.insert( | ||
| 'decoded_onehz', | ||
| Map<String, Object?>.from(row), | ||
| table, | ||
| clean, | ||
| conflictAlgorithm: ConflictAlgorithm.replace, | ||
| ); | ||
| } | ||
| await batch.commit(noResult: true); | ||
| }); | ||
| final counters = <Object?>[ | ||
| for (final row in decoded) | ||
| if (row['counter'] != null) row['counter'], | ||
| ]; | ||
| // CHUNKED `IN (…)`: a full day is 86 400 counters, two orders of | ||
| // magnitude past SQLITE_MAX_VARIABLE_NUMBER — one giant statement can | ||
| // never bind. (This never surfaced only because the missing `version:` | ||
| // above aborted the export earlier.) | ||
| for (final chunk in _sqlVarChunks(counters)) { | ||
| final placeholders = List.filled(chunk.length, '?').join(','); | ||
| final rr = await src.rawQuery( | ||
| 'SELECT * FROM decoded_rr WHERE counter IN ($placeholders)', | ||
| chunk, | ||
| ); | ||
| if (rr.isEmpty) continue; | ||
| await out.transaction((txn) async { | ||
| final batch = txn.batch(); | ||
| for (final row in rr) { | ||
| batch.insert( | ||
| 'decoded_rr', | ||
| Map<String, Object?>.from(row), | ||
| conflictAlgorithm: ConflictAlgorithm.replace, | ||
| ); | ||
| } | ||
| await batch.commit(noResult: true); | ||
| }); | ||
| } | ||
| if (onPage != null) await onPage(page); | ||
| lastRowid = (page.last[rowidKey] as num).toInt(); | ||
| if (page.length < exportPageSize) return; | ||
| } | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Filtered rowid paging re-scans the table on every page.
WHERE rowid > ? AND (rec_ts >= ? AND rec_ts < ?) ORDER BY rowid ASC forces a rowid-ordered scan, so the idx_decoded_onehz_rects range index can't drive the query: each page walks the remaining decoded_onehz rows from lastRowid to the end of the table to collect its 2000 matches (and the last page always scans to the end). On a multi-year ledger that is a full-table scan per page, per exported day. Correctness is fine (a short page really does mean exhaustion), but the export cost is now O(days × pages × table).
For the day-ranged copies, keyset on the filtered column instead — WHERE rec_ts >= ? AND rec_ts < ? AND rec_ts > lastRecTs ORDER BY rec_ts ASC uses the existing index and stays linear. decoded_onehz is UNIQUE(rec_ts), so rec_ts is a valid unique cursor; the same shape works for the ts/start_ts-filtered tables with a (ts, rowid) composite cursor.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/data/db.dart` around lines 3016 - 3051, The copyPaged method currently
pages filtered tables by rowid, causing repeated scans of the remaining table.
Update the day-ranged export flow to use keyset pagination on the filtered
indexed column: for decoded_onehz use rec_ts as the cursor with rec_ts >
lastRecTs and ORDER BY rec_ts ASC, and for ts/start_ts tables use the
appropriate (timestamp, rowid) composite cursor. Preserve filtering, insertion,
callbacks, and short-page termination behavior.
Addresses bot review on #163. The import's first-page read caught EVERYTHING and treated it as "this export doesn't carry that table" — so corruption, a truncated or malformed source file, or an I/O error all skipped the table silently, `counts[t]` was never set, and the summed total then reported a PARTIAL import as a success. Silent partial success on someone's health history is the worst available outcome. Now only `isNoSuchTableError()` skips; everything else propagates. Narrowing this had a real regression risk worth a test: the fixture only creates decoded_onehz + decoded_rr, so every other table in the import list already hits the missing-table path on every import test. Had `isNoSuchTableError()` not matched sqflite's actual exception, importing a partial export would have started THROWING instead of skipping — and that would have ridden in on tests that appear to be about paging. Now asserted by name. (Used sqflite's own predicate rather than matching 'no such table' in a `toString()`, which is not stable across platforms.) Also, from the same review: - `copyPaged` hands [onPage] rows with the `_rowid` cursor column already stripped, so a callback can insert what it is given. Today's only callback reads `row['counter']`, but the raw-row API was a standing invitation to a "no such column: _rowid" bug. Drops a redundant per-row map rebuild too. - Documented why the static truncation counter is sound at this call site (every sqflite call needs the root isolate's platform channel, and the only caller reads on the main isolate and ships pages to the worker via `worker.send`) and what breaks it, because the previous comment implied the opposite and led a reviewer to a wrong conclusion. - Documented the paging-column tradeoff where it lives. Paging on rowid rather than the filtered column means a filtered page walks the rowid chain instead of driving off the rec_ts index. It stays cheap because both factors are bounded — decoded_onehz is pruned to rawRetentionDays behind the data edge, and the never-pruned per-day tables are hundreds of rows. Measured ~10 ms for the worst case (the exhaustion page that scans to the end of the table) on a real 435k-row ledger. 990 tests pass; analyze clean.
|
Fixed (8994285)
Declining, with measurements
990 tests pass, |
|
Persistent review updated to latest commit 8994285 |
1 participant
User description
Four production defects from the 0.9.19/0.9.20 Crashlytics dashboard, each one a foreground assumption or an unbounded read applied to a context that breaks it. No analytics output changes, so
kAlgoVersionstays at 50.1. Import/export OOM —
java.lang.OutOfMemoryErrorimportFromDbransrc.query(t)per table and then held the entire result live for the duration of the insert loop. sqflite serialises a whole result set into Java objects on the platform side before any of it crosses the channel, so importing another device'sdecoded_onehz(86,400 rows per day of history) materialised the whole table on the 256 MB Dalvik heap at once (target footprint 268435456, growth limit 268435456). The OOM surfaced on whichever thread allocated next — which is why it was reported against a BLE binder callback rather than the import.Both the import and export paths now page on
rowid:LIMIT/OFFSET— OFFSET re-scans the skipped prefix on every page, which is quadratic over a full history.decoded_onehzrow's guard is still queued in the same transaction as the row it guards. The invariant is per-row, not per-table.INSERT OR REPLACEkeyed on the row's own identity, so a re-run converges to the same state.decoded_onehzplus its beats — not a whole day of both.2. Unbounded RR fallback read
decodedRrByCounterRangehas a degraded branch for when an endpoint row isn't indecoded_onehz(a prune, or an import's REPLACE + orphan-guard DELETE landing between the page read and the call). The caller's counters are then just a span — and because the strap's counter resets on reboot, a reboot-straddling page degenerates to0 .. ~1200000, i.e. effectively the whole table, on the same heap that OOMed above.Now capped. A page is 2000 frames and a second rarely carries more than a handful of beats, so the cap sits orders of magnitude above any legitimate page — reaching it means the degraded path is serving a range it was never meant to. Because a short read here shows up downstream as understated HRV rather than as an error, it is never silent:
decodedRrFallbackTruncationscounts it for the Diagnostics screen. (db.darttakes no telemetry dependency — it has to run inside compute isolates.)3. Background derive timeouts —
day_blocks_failedTimeoutException: day-blocks computation timed out after 0:01:30, reported only from insideIosBgTask._run— i.e. only ever in background. Two foreground assumptions were being applied to a context that breaks both:Paired with a wall-clock timeout, the old tuning converted "3 days derived" into "3 days timed out". Serial lanes also cap peak memory at one day's substrate instead of three.
background: trueis set at construction on all four headless entries (iOS BGProcessingTask + BGAppRefreshTask, Android WorkManager, post-drain background sync) so a long-lived foreground engine can never inherit background tuning by accident.4. Jank watchdog false positives
The watchdog gated on
FrameTiming.totalSpan— vsync-start to raster-end, which includes time the engine did nothing, above all the gap across an app resume where a "frame" straddles however long the app sat in the background. The result:A 16-second stutter in which the app did 8 ms of work. That single false-positive class became the top issue by impacted-user count on both platforms, burying real signal underneath it.
JankPolicygates on build + raster — the two costs the app controls and the two a fix would actually move. The 700 ms threshold is unchanged but now means "the engine burned 700 ms on one frame" rather than "700 ms elapsed", which is orders of magnitude rarer and always actionable.totalSpanis still attached as context alongside a derivedidle_ms, so a genuine scheduling-delay/thermal pattern stays visible — it just isn't a jank report.Also: ImportScreen double-tap crash
_busyonly went true once an import was already running, i.e. after the picker returned — so while the native sheet was still opening the three option cards stayed live. The sheet takes a beat to appear on a cold platform channel, so users do tap twice; the second tap calledpickFilesagain and file_picker threwPlatformException(already_active)out of an unawaited context, crashing the app. Guarded twice over: a_pickingflag makes the cards inert for the whole picker lifetime, and the catch refuses to let any picker platform failure become fatal (already_activeitself is silent — the first picker is still up and will deliver the user's choice).Testing
flutter test --concurrency=1→ 989 passed (up from 914; three new suites:db_paged_import_export_test,derive_pacing_test,jank_policy_test).flutter analyze→ clean, no issues.DerivePacing,JankPolicy) so they are unit-testable without a database, an isolate, a real background slot, aSchedulerBinding, or a Firebase app.pubspec.yamlrefs matchpubspec.lockresolved-refs (protocola98cd70, analyticsf5ccae6), git provenance intact.Not yet exercised on real hardware — items 3 and 4 are background/timing behaviours that only prove out on a device over a few OS-granted slots, so the Crashlytics rates for
day_blocks_failedandjank_watchdogare what should confirm this after release.🤖 Generated with Claude Code
PR Type
Bug fix, Tests
Description
Fix import/export OOM: replace unbounded
SELECT *with keyset-paged reads (2000 rows/page, one transaction per page) on both import and export pathsFix background derivation timeouts: introduce
DerivePacingso background slots use 1 lane and a 4-minute per-day timeout instead of 3 lanes and 90 sFix false-positive jank reports: gate the watchdog on build+raster work time, not
totalSpan(which includes app-resume idle gaps)Fix double-tap
already_activecrash in ImportScreen: add_pickingguard around the native file picker callDiagram Walkthrough
File Walkthrough
8 files
Keyset-paged import/export; bounded RR fallback readWire background flag into concurrency and per-day timeoutPass background=true to DerivationEngine in WorkManager entryPass background=true to post-drain derive passPass background=true to both iOS BGTask derive enginesNew pure class: jank verdict based on engine work, not totalSpanReplace totalSpan jank trigger with JankPolicy build+raster checkGuard file picker against double-tap already_active crash1 files
New pure class: foreground vs background lane/timeout policy3 files
Tests: paged import/export correctness across page boundariesTests: background vs foreground pacing concurrency and timeoutTests: jank policy suppresses idle-span false positives1 files
Add App Store Connect export options plist for iOS archiveSummary by CodeRabbit
New Features
Bug Fixes
Tests