Skip to content

app(sync): keep backendBusy cooldown in-memory only#7527

Open
mdmohsin7 wants to merge 1 commit into
mainfrom
caleb/sync-busy-no-persist
Open

app(sync): keep backendBusy cooldown in-memory only#7527
mdmohsin7 wants to merge 1 commit into
mainfrom
caleb/sync-busy-no-persist

Conversation

@mdmohsin7
Copy link
Copy Markdown
Member

Summary

  • SyncRateLimiter now persists only the rateLimit cooldown. backendBusy cooldowns live in an in-memory field that clears on app restart.
  • Constructor clears any pre-existing persisted backendBusy entry from older app versions, so users coming from a healthy server get unstuck on next launch.

Why

The "Omi servers are busy" banner was surviving app restarts even when the backend had long since recovered. The cooldown was saved to SharedPreferences for both rateLimit (HTTP 429) and backendBusy (stale-guard) cases.

rateLimit cooldowns SHOULD persist — they mirror server-side fair-use enforcement that is itself sticky across restarts (30-day restrict window). But backendBusy reflects transient backend pipeline pressure. If a user restarts the app to retry, the cooldown should clear and the next sync should re-probe the backend, not trust a local timer set during an earlier rough patch.

Backend data (last 7 days): zero stale-guard fires globally, yet some users still report seeing the banner. Their app state is stuck from an earlier server hiccup that has long since resolved.

Behavior

State Before After
Server returns 429 (rateLimit) 30 min cooldown, persists across restart unchanged — still persists
Reconciler sees stale-guard (backendBusy) 10 min cooldown, persists across restart 10 min cooldown, in-memory only
App restart while in backendBusy banner persists until 10 min wall-clock elapses banner clears immediately, next sync re-probes
User upgrades from old version with stuck backendBusy banner persists one-time migration clears it on first launch

Test plan

  • Manual: mark backendBusy via debug hook (or hit the trigger condition), kill app, relaunch — banner gone
  • Manual: mark rateLimit (e.g. fair-use 429), kill app, relaunch — banner still there, cooldown still ticking
  • Manual: install build over an older one that had persisted backendBusy state — banner gone after launch
  • No regression in successful-sync flow (banner clears on success)

The "Omi servers are busy" message persisted across app restarts because
the SyncRateLimiter saved its until/reason to SharedPreferences for both
rate-limit (HTTP 429) and backendBusy (stale-guard) cases.

backendBusy reflects transient backend pipeline pressure. If a user
restarts the app to retry, the cooldown should clear and the next sync
should re-probe the backend instead of trusting a local timer set
during an earlier rough patch.

Keep persistence for rateLimit (mirrors server-side fair-use enforcement
which IS sticky across restarts) and move backendBusy to an in-memory
field. On construction, clear any pre-existing persisted backendBusy
entry so users coming from older app versions get unstuck on next
launch.
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented May 28, 2026

Greptile Summary

This PR changes SyncRateLimiter so that backendBusy cooldowns live only in an in-memory field (_backendBusyUntilMs) and are not written to SharedPreferences, while rateLimit (HTTP 429) cooldowns continue to be persisted as before. A one-time migration in the constructor clears any previously-persisted backendBusy entry so users upgrading from older builds are immediately unstuck.

  • In-memory backendBusy cooldown: markLimited now branches on reason, writing either to _backendBusyUntilMs or to SharedPreferences. isLimited, until, and reason getters all consult both sources.
  • Migration: the private constructor reads _prefKeyReason; if it equals \"backendBusy\" it zeroes out both persisted keys, running exactly once per app install of the new version.
  • Inconsistency in reason/until pairing: when both a backendBusy and a rateLimit cooldown are active simultaneously, until returns the later (max) expiry while reason always surfaces backendBusy first — so the displayed reason and deadline can refer to two different cooldowns.

Confidence Score: 3/5

Safe to merge for the common case (single cooldown active), but the dual-cooldown scenario produces a misleading UI state.

The core improvement — clearing backendBusy on restart and migrating stuck state from older installs — is sound and well-reasoned. The concern is in the reason and until getters: when both a backendBusy and a rateLimit cooldown are simultaneously active, until returns the later (max) expiry while reason always surfaces backendBusy first. The user would see "Omi servers are busy — wait until [rateLimit deadline]" even though the backendBusy ends sooner and the displayed deadline belongs to the rateLimit — then the message silently flips to "Fair-use limit" at backendBusy expiry with no change to the timer.

app/lib/services/wals/sync_rate_limiter.dart — specifically the reason and until getters

Important Files Changed

Filename Overview
app/lib/services/wals/sync_rate_limiter.dart Splits cooldown storage into in-memory (backendBusy) and persisted (rateLimit), with a one-time migration to clear stuck state. Logic bug: reason and until getters use different priority rules, producing an inconsistent pair when both cooldowns are simultaneously active.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[markLimited called] --> B{reason == backendBusy?}
    B -- Yes --> C[_backendBusyUntilMs = untilMs in-memory only]
    B -- No --> D[SharedPreferences persisted]
    C --> E[notifyListeners]
    D --> E
    F[isLimited getter] --> G{_backendBusyUntilMs > now?}
    G -- Yes --> H[return true]
    G -- No --> I{persisted until > now?}
    I -- Yes --> H
    I -- No --> J[return false]
    K[until getter] --> L[return max of active candidates]
    N[reason getter] --> O{_backendBusyUntilMs > now?}
    O -- Yes --> P[return backendBusy]
    O -- No --> Q{persisted > now?}
    Q -- Yes --> R[return persisted reason]
    Q -- No --> S[return null]
Loading

Reviews (1): Last reviewed commit: "app(sync): keep backendBusy cooldown in-..." | Re-trigger Greptile

Comment on lines 57 to +74
DateTime? get until {
final ms = SharedPreferencesUtil().getInt(_prefKeyUntil);
return ms > 0 ? DateTime.fromMillisecondsSinceEpoch(ms) : null;
final now = DateTime.now().millisecondsSinceEpoch;
final persisted = SharedPreferencesUtil().getInt(_prefKeyUntil);
final inMemory = _backendBusyUntilMs;
final candidates = <int>[if (inMemory > now) inMemory, if (persisted > now) persisted];
if (candidates.isEmpty) return null;
return DateTime.fromMillisecondsSinceEpoch(candidates.reduce((a, b) => a > b ? a : b));
}

RateLimitReason? get reason {
if (!isLimited) return null;
final name = SharedPreferencesUtil().getString(_prefKeyReason);
return RateLimitReason.values.asNameMap()[name] ?? RateLimitReason.rateLimit;
final now = DateTime.now().millisecondsSinceEpoch;
if (_backendBusyUntilMs > now) return RateLimitReason.backendBusy;
final persisted = SharedPreferencesUtil().getInt(_prefKeyUntil);
if (persisted > now) {
final name = SharedPreferencesUtil().getString(_prefKeyReason);
return RateLimitReason.values.asNameMap()[name] ?? RateLimitReason.rateLimit;
}
return null;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P1 reason and until can report an inconsistent pair when both cooldowns are active

When a backendBusy in-memory cooldown and a persisted rateLimit cooldown are both active (e.g., a stale-guard fired during a 429 window), until returns the max expiry (likely the rateLimit end time), while reason returns backendBusy because _backendBusyUntilMs > now takes priority. The UI would then show "Omi servers are busy — wait until [rateLimit deadline]", which is wrong on both counts: the labelled reason expires sooner than displayed, and the displayed deadline belongs to a different reason. After _backendBusyUntilMs expires, the reason silently flips to rateLimit with no change in until, but until that flip the user sees an incorrect message. The reason getter should return the reason whose expiry matches the value returned by until (i.e., the reason with the later deadline), consistent with until's max-based logic.

Copy link
Copy Markdown
Collaborator

@kodjima33 kodjima33 left a comment

Choose a reason for hiding this comment

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

thanks — keeping backendBusy cooldown in-memory makes sense

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants