fix(app): allow Limitless offload during uploads - #12307
Conversation
Failure-Class: none
Git-on-my-level
left a comment
There was a problem hiding this comment.
Thanks for the excellent groundwork — the #12265 writeup is one of the clearest issue reports I've seen on this surface, and extracting a pure presentation view-model with dedicated unit tests is exactly the right shape. I verified the core isolation claim and it holds; requesting changes for one concurrency gap the new parallel-lane design opens, plus one semantics question.
Blocking: the flash drain lane is now reachable concurrently with itself
Verified good: WalSyncs.offloadFlashPages() (app/lib/services/wals/wal_syncs.dart:144) ends after _flashPageSync.syncAll() and never reaches Phase 2, so the offload genuinely never enters the cloud-upload lane — and SyncProvider.offloadLimitlessFlash() never enters _performSync, so isSyncing stays false (your test asserts exactly this).
The problem: every previous entry into the flash drain sat under the isSyncing/isProcessing umbrella, which this PR deliberately removes — but the remaining guards were never taught about the flash lane:
SyncProvider._drainEligibleWals()(app/lib/providers/sync_provider.dart:438) returnscontendedforisStorageSyncing || isSdCardSyncing, but notisFlashPageSyncing._isTransferSeamBusy()(app/lib/providers/sync_provider.dart:600) has the same omission, so a single-WAL sync from the Auto Sync page can also enter during an offload.FlashPageWalSyncImpl.syncAll()/syncWal()(app/lib/services/wals/flash_page_wal_sync.dart:222/296) have no entry guard;_isSyncingis only set inside_syncWal().
Concrete sequence on this PR's headline path: the user starts an offload during an upload (your main scenario); the upload finishes; any later coordinator wake — userRetry (which bypasses the auto-upload gate entirely), connectivityRestored after a network blip, or a cooldownElapsed retry, which is plausible precisely in the slow/failing-upload situation #12265 describes — runs _drainEligibleWals, sees isProcessing == false, and Phase 1b re-enters _flashPageSync.syncAll() while the first drain is mid-stream. The second _syncWal() calls clearBuffer() on the shared connection (flash_page_wal_sync.dart:339), dropping frames the first drain has received but not yet extracted, and the two loops then interleave extractFramesWithSessionInfo() and ACKs. Depending on where the stall lands, pages can be skipped on resume (storageOffset = lastProcessedIndex + 1) — permanent audio loss on exactly the flow this PR enables. The reverse window also exists: an offload started while a coordinator pass is still in Phase -1 (live-capture upload) can begin draining before that pass's Phase 1b arrives minutes later.
Suggested fix (small): include the flash lane in both seam checks — the provider's isFlashPageSyncing getter already ORs _isOffloadingLimitlessFlash, so it covers the pre-_syncWal window too — and/or add a re-entry guard at the top of FlashPageWalSyncImpl.syncAll()/syncWal(). A test that drives _drainEligibleWals mid-offload would lock the contract in; your offloadCompleter fixture is a perfect fit for it.
Semantics: "Sync Now" now means "copy to phone" in all states, not just during uploads
canOffloadDevice is true whenever flash pages are pending and no drain is running — including when the cloud lane is idle. Pre-PR, the button ran confirmSyncForCustomStt + syncWals() (drain and upload); now limitless_sync_widget.dart:79 wires it to offload only. Two consequences:
offloadLimitlessFlash()'sfinallyrefreshes WALs but never wakes the transfer coordinator, so for auto-sync users the offloaded local files sit unuploaded until the next wake trigger (foreground/connectivity/cooldown/restart) — the card disappears once the flash WAL is marked synced, and the user reasonably reads that as "uploaded".- For custom-STT users the consent story is actually fine — I verified the coordinator's
autoUploadEnabledexcludesuseCustomStt, and the confirm dialog still gates both sync pages, so nothing reaches Omi's servers without consent. But the button gives no hint that a separate upload step is still needed, and the label still says "Sync Now".
Please either restore the full-sync behavior when the cloud lane is idle, or keep offload-only deliberately and (a) wake the coordinator after a successful offload when auto-upload is enabled, and (b) make the label/state say what it does. If offload-only-everywhere is the intended direction, a one-line note in the PR description would help the maintainer confirm that product call.
Per-file notes
- app/lib/pages/capture/widgets/limitless_sync_presentation.dart — clean pure view-model;
canOffloadDevice: hasPendingFlashPages && !isFlashDrainingis the right predicate, and the three unit tests pin the presentation states down. - app/lib/pages/capture/widgets/limitless_sync_widget.dart — the layout correctly lets the upload percent, the drain spinner, and the offload button coexist. Two nits: the hardcoded
0xFF16A34Agreen clashes with the app's deepPurple accents (the progress bar a few lines below is still deepPurple), and during a drain-only offload the title still reads "Sync your recordings" (showsCloudProgress == false) — a "Copying…"-style variant would read better next to the spinner. - app/lib/providers/sync_provider.dart —
_isOffloadingLimitlessFlashsingle-flight with dispose handling and therefreshWals()infinallyare solid; the missing pieces are the coordinator wake and the seam checks above. - app/lib/services/wals/wal_syncs.dart — the
missing.where((wal) => wal.status == WalStatus.miss)filter correctly mirrors Phase 1b; needs the re-entry guard discussed above. - app/test/providers/sync_provider_flash_stall_test.dart — the Completer-based single-flight test is exactly the right technique; please extend it to the coordinator seam as suggested.
- app/test/unit/limitless_sync_presentation_test.dart — covers the three states well; consider adding the transient
hasPendingFlashPages: false, isFlashDraining: truetail state if it is reachable in practice.
The issue diagnosis, the state isolation, and the honesty about not having exercised real hardware are all appreciated. Once the flash-lane guards land, the remaining open item for a maintainer is on-device verification of the drain-during-upload path, since the BLE stall/resume behavior can't be covered by these tests.
by AI on behalf of David — if you need David’s attention urgently, please @Git-on-my-level and escalate with need human response.
Git-on-my-level
left a comment
There was a problem hiding this comment.
Thanks — both follow-up commits land exactly the right fixes for the two items from my earlier review, and I've verified them on this head:
SyncProvider._drainEligibleWals()(sync_provider.dart:440) and_isTransferSeamBusy()(:602) now OR inisFlashPageSyncing, and since that getter already ORs_isOffloadingLimitlessFlash— set synchronously before the firstawait— the forward window (a coordinator wake or single-WAL sync entering during a device-only offload) is closed end-to-end. The new "coordinator drain stays contended for the full device-only offload" test pins it.- The button-semantics question is resolved the way I'd hoped:
limitless_sync_widget.dartnow runs the fullconfirmSyncForCustomStt+syncWals()flow when the cloud lane is idle, and only takes the offload-only path while an upload is actually in flight. (The deepPurple restyle also removes the color clash.)
Remaining blocker — the reverse window: a coordinator pass already past the seam checks still re-enters the flash drain mid-offload
The new seam guards only help callers that check them after an offload starts. They do nothing when the coordinator pass is already inside WalSyncs.syncAll() when the user taps offload — which is precisely this PR's headline scenario:
- A coordinator drain is in Phase -1, uploading an earlier recording (
_syncState.isProcessingtrue, flash lane idle). - The user taps offload.
SyncProvider.offloadLimitlessFlash()(sync_provider.dart:555) checks only_isDisposed || isFlashPageSyncing— deliberately notisProcessing— andWalSyncs.offloadFlashPages()(wal_syncs.dart:144) checks only_flashPageSync.isSyncing, which is still false because that flag flips true deep inside_syncWal()(flash_page_wal_sync.dart:343), afterensureConnection/clearBuffer. The offload drain starts. - Phase -1 finishes minutes later and the same pass proceeds to Phase 1b (wal_syncs.dart:339-349):
refreshWalsFromDevice()early-returns on the_isSyncingguard (good — no list swap mid-drain), butgetMissingWals()still sees in-flight pages asmiss, so it calls_flashPageSync.syncAll()with no busy check — concurrent with the offload's ownsyncAll(). - The second pass resets
_cancelRequested, and the two loops interleave_syncWal()on the shared connection: eachclearBuffer()(flash_page_wal_sync.dart:338) drops frames the other drain has received but not yet extracted, ACKs interleave, and resume positions (storageOffset = lastProcessedIndex + 1) can skip pages — permanent audio loss on exactly the flow this PR enables.
Suggested fix: give FlashPageWalSyncImpl.syncAll()/syncWal() a re-entry guard whose flag is set synchronously at entry, before any await — a plain if (_isSyncing) return; at the top would still leave the multi-second ensureConnection window open, since _isSyncing is only assigned after connection acquisition today. Exposing that entry flag through the existing isSyncing getter also makes the guard in WalSyncs.offloadFlashPages() effective for the whole drain, including enumeration. A reverse-direction test — fake syncAll parked on a Completer to hold the coordinator in Phase 1b, then offloadLimitlessFlash() — would lock the contract symmetric to the one you just added.
Per-file notes
app/lib/pages/capture/widgets/limitless_sync_presentation.dart— clean pure view-model; thecanOffloadDevicepredicate is right and the three unit tests pin the states down.app/lib/pages/capture/widgets/limitless_sync_widget.dart— the branch structure reads well. During the transient drain-only state (upload ends mid-drain) the title still reads "Sync your recordings" next to the spinner; a "Copying…" variant would read better, but that's non-blocking.app/lib/providers/sync_provider.dart— single-flight, dispose handling, and therefreshWals()infinallyare solid;drainEligibleWalsForTestingis a reasonable test seam.app/lib/services/wals/wal_syncs.dart— themissing.where(...)filter correctly mirrors Phase 1b; needs the entry-guard alignment above.app/test/providers/sync_provider_flash_stall_test.dart— the Completer-based tests are exactly the right technique; please add the reverse-direction case.app/test/providers/sync_provider_sync_wal_wake_test.dart— interface-conformance fix for the fake; fine.app/test/unit/limitless_sync_presentation_test.dart— covers the three states well; the transienthasPendingFlashPages: false, isFlashDraining: truetail state remains untested if it's reachable in practice.
Non-blocking: after an offload completes, the drained phone-local files are picked up by the in-flight pass's Phase 2 or the next coordinator wake, so auto-upload users are covered, just not instantly.
The state isolation, the forward serialization, and the test discipline here are all in good shape — this is the last gap I can find in the concurrency story.
by AI on behalf of David — once the re-entry guard lands, the drain-during-upload path still needs on-device verification on real Limitless hardware before merge, since the BLE stall/resume behavior can't be exercised by the Dart test suite.
What changed and why
Separate Limitless flash draining from the cloud-upload state machine so users can free device storage while an earlier recording uploads. The card now presents cloud progress and device offload concurrently, and the device lane is immediately single-flight to prevent duplicate BLE drains.
Closes #12265
Product invariants affected
none
How it was verified
flutter test test/providers/sync_provider_flash_stall_test.dart test/unit/limitless_sync_presentation_test.dart— all 9 UI-state, stall, cloud-state isolation, and single-flight tests passed.bash scripts/analyze_ratchet.sh— analyzer ratchet passed.Tests
Failure-Class: none