Skip to content

fix(app): allow Limitless offload during uploads - #12307

Open
arhxam wants to merge 3 commits into
BasedHardware:mainfrom
arhxam:fix/limitless-offload-during-upload
Open

fix(app): allow Limitless offload during uploads#12307
arhxam wants to merge 3 commits into
BasedHardware:mainfrom
arhxam:fix/limitless-offload-during-upload

Conversation

@arhxam

@arhxam arhxam commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

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.
  • The BLE drain was not exercised on physical Limitless hardware in this environment.

Tests

  • Added presentation coverage for simultaneous cloud upload and device offload.
  • Added provider coverage proving the offload never enters cloud sync state and rejects a second concurrent request.
  • Preserved existing flash-stall regression coverage.
  • Physical Limitless flash drain during an upload (hardware unavailable).

Failure-Class: none

Review in cubic

@arhxam
arhxam requested a review from mdmohsin7 as a code owner August 27, 2026 17:55

@Git-on-my-level Git-on-my-level left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) returns contended for isStorageSyncing || isSdCardSyncing, but not isFlashPageSyncing.
  • _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; _isSyncing is 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:

  1. offloadLimitlessFlash()'s finally refreshes 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".
  2. For custom-STT users the consent story is actually fine — I verified the coordinator's autoUploadEnabled excludes useCustomStt, 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 && !isFlashDraining is 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 0xFF16A34A green 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 — _isOffloadingLimitlessFlash single-flight with dispose handling and the refreshWals() in finally are 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: true tail 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 Git-on-my-level added the flutter flutter work label Aug 27, 2026

@Git-on-my-level Git-on-my-level left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 in isFlashPageSyncing, and since that getter already ORs _isOffloadingLimitlessFlash — set synchronously before the first await — 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.dart now runs the full confirmSyncForCustomStt + 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:

  1. A coordinator drain is in Phase -1, uploading an earlier recording (_syncState.isProcessing true, flash lane idle).
  2. The user taps offload. SyncProvider.offloadLimitlessFlash() (sync_provider.dart:555) checks only _isDisposed || isFlashPageSyncing — deliberately not isProcessing — and WalSyncs.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), after ensureConnection/clearBuffer. The offload drain starts.
  3. Phase -1 finishes minutes later and the same pass proceeds to Phase 1b (wal_syncs.dart:339-349): refreshWalsFromDevice() early-returns on the _isSyncing guard (good — no list swap mid-drain), but getMissingWals() still sees in-flight pages as miss, so it calls _flashPageSync.syncAll() with no busy check — concurrent with the offload's own syncAll().
  4. The second pass resets _cancelRequested, and the two loops interleave _syncWal() on the shared connection: each clearBuffer() (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; the canOffloadDevice predicate 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 the refreshWals() in finally are solid; drainEligibleWalsForTesting is a reasonable test seam.
  • app/lib/services/wals/wal_syncs.dart — the missing.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 transient hasPendingFlashPages: false, isFlashDraining: true tail 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

flutter flutter work

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Limitless: no UI pathway to drain device flash while cloud upload is in progress

2 participants