From 2c5636a7d07cf1e7fac0a2d7878f04f335cc69b0 Mon Sep 17 00:00:00 2001 From: Ridanshi Date: Mon, 1 Jun 2026 21:37:11 +0530 Subject: [PATCH] fix(cloud-sync): prevent stale snapshots from overwriting newer account state MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit hydrateFromCloud previously merged accounts as { ...local, ...cloud }, letting the Appwrite snapshot unconditionally overwrite every local field. A device that earned tokens while offline and reconnected before the offline queue was flushed would silently lose those balances. Fix: DB.set now stamps every local account write with _syncTs (epoch ms). sanitizeAccount propagates _syncTs into Appwrite so both sides carry the same timestamp field. hydrateFromCloud reads both timestamps and picks the authoritative side before merging: localTs > cloudTs → local wins; Appwrite is re-pushed to catch up. cloudTs >= localTs → cloud wins (standard cloud-wins merge). Safety net for legacy records without _syncTs: Math.max is applied to tokens and staked so an offline-earned balance is never silently discarded. Covers: offline device recovery, reconnect, multi-device divergence. Tests: scripts/test-cloud-sync-merge.mjs — 22 assertions, all passing. Closes #543 --- scripts/test-cloud-sync-merge.mjs | 280 ++++++++++++++++++++++++++++++ src/app.js | 18 +- src/cloud-sync.js | 63 ++++++- 3 files changed, 354 insertions(+), 7 deletions(-) create mode 100644 scripts/test-cloud-sync-merge.mjs diff --git a/scripts/test-cloud-sync-merge.mjs b/scripts/test-cloud-sync-merge.mjs new file mode 100644 index 0000000..320e2ac --- /dev/null +++ b/scripts/test-cloud-sync-merge.mjs @@ -0,0 +1,280 @@ +/** + * Tests for the CloudSync stale-overwrite fix. + * + * hydrateFromCloud originally merged accounts as { ...local, ...cloud }, + * letting the cloud snapshot unconditionally overwrite every local field. + * A device that earned tokens while offline and reconnected before the + * offline queue was flushed would silently lose those tokens. + * + * The fix introduces _syncTs timestamps on every local account write + * (via DB.set) and in every Appwrite push (via sanitizeAccount). + * hydrateFromCloud now uses those timestamps to decide which side is + * authoritative, with a Math.max safety net for legacy records that + * predate the _syncTs field. + * + * Coverage: + * 1. Original (buggy) merge: local newer tokens overwritten by cloud + * 2. Fixed merge: local newer tokens preserved when localTs > cloudTs + * 3. Fixed merge: cloud wins when cloudTs >= localTs (normal reconnect) + * 4. Fixed merge: same timestamp → cloud wins + * 5. Legacy records (no _syncTs): Math.max preserves higher local tokens + * 6. Legacy records: Math.max preserves higher local staked + * 7. Legacy records: cloud-only fields are still applied + * 8. When local is newer: re-push triggered (cloud brought up to date) + * 9. When cloud is newer: re-push NOT triggered + * 10. Non-financial fields (name, role) follow cloud in cloud-wins path + * 11. Non-financial fields (name, role) follow local in local-wins path + * 12. Local newer: cloud-only fields populated into merged result + * 13. Empty local account treated as fully stale (cloud wins) + * 14. Empty cloud account falls back to local (no merge performed) + * 15. _syncTs is preserved in merged result + * 16. Regression: offline token earn survives reconnect hydration + * 17. Regression: multi-device divergence resolves by timestamp + */ + +let passed = 0; +let failed = 0; + +function assert(condition, label) { + if (condition) { + console.log(` ✓ ${label}`); + passed++; + } else { + console.error(` ✗ ${label}`); + failed++; + } +} + +// ── Core merge logic (mirrors the patched hydrateFromCloud) ─────────────────── + +function mergeAccount(localAcc, cloudAccount) { + if (!cloudAccount) return { merged: localAcc, repushed: false }; + + const local = localAcc || {}; + const localTs = Number(local._syncTs) || 0; + const cloudTs = Number(cloudAccount._syncTs) || 0; + + let merged; + let repushed = false; + + if (localTs > cloudTs) { + // Local is newer — local wins, cloud is brought up to date. + merged = { ...cloudAccount, ...local }; + repushed = true; + } else { + // Cloud is current — standard cloud-wins merge. + merged = { ...local, ...cloudAccount }; + + // Safety net for legacy records without _syncTs. + if (localTs === 0 && cloudTs === 0) { + if (typeof local.tokens === 'number') { + merged.tokens = Math.max(Number(local.tokens), Number(cloudAccount.tokens) || 0); + } + if (typeof local.staked === 'number') { + merged.staked = Math.max(Number(local.staked), Number(cloudAccount.staked) || 0); + } + } + } + + return { merged, repushed }; +} + +// ── Original (buggy) merge for regression comparison ───────────────────────── + +function buggyMergeAccount(localAcc, cloudAccount) { + if (!cloudAccount) return { merged: localAcc || {}, repushed: false }; + const merged = { ...(localAcc || {}), ...cloudAccount }; + return { merged, repushed: false }; +} + +// ───────────────────────────────────────────────────────────────────────────── + +console.log('\nRunning CloudSync stale-overwrite tests…\n'); + +// ── 1. Regression: original buggy merge loses offline tokens ────────────────── + +console.log('1. Regression: original (buggy) merge overwrites local tokens'); +{ + const local = { id: 'u1', tokens: 150, _syncTs: 2000 }; // newer offline earn + const cloud = { id: 'u1', tokens: 100, _syncTs: 1000 }; // stale Appwrite snapshot + + const { merged } = buggyMergeAccount(local, cloud); + assert(merged.tokens === 100, + '[regression] buggy merge: cloud tokens (100) overwrite local tokens (150)'); +} + +// ── 2. Fixed: local newer → local tokens preserved ──────────────────────────── + +console.log('\n2. Fixed merge: local newer → local tokens preserved'); +{ + const local = { id: 'u1', tokens: 150, _syncTs: 2000 }; + const cloud = { id: 'u1', tokens: 100, _syncTs: 1000 }; + + const { merged, repushed } = mergeAccount(local, cloud); + assert(merged.tokens === 150, 'local tokens (150) preserved when local is newer'); + assert(repushed === true, 'push-to-cloud triggered to bring Appwrite up to date'); +} + +// ── 3. Cloud newer → cloud values applied ──────────────────────────────────── + +console.log('\n3. Fixed merge: cloud newer → cloud values applied'); +{ + const local = { id: 'u1', tokens: 100, _syncTs: 1000 }; + const cloud = { id: 'u1', tokens: 200, _syncTs: 2000 }; // Device A earned tokens + + const { merged, repushed } = mergeAccount(local, cloud); + assert(merged.tokens === 200, 'cloud tokens (200) applied when cloud is newer'); + assert(repushed === false, 'no re-push when cloud is already authoritative'); +} + +// ── 4. Same timestamp → cloud wins ─────────────────────────────────────────── + +console.log('\n4. Same timestamp → cloud wins'); +{ + const ts = 1000; + const local = { id: 'u1', tokens: 80, _syncTs: ts }; + const cloud = { id: 'u1', tokens: 120, _syncTs: ts }; + + const { merged } = mergeAccount(local, cloud); + assert(merged.tokens === 120, 'cloud wins on tie'); +} + +// ── 5-6. Legacy records (no _syncTs): Math.max for financial fields ─────────── + +console.log('\n5-6. Legacy records — Math.max safety net'); +{ + const local = { id: 'u1', tokens: 180, staked: 50 }; // no _syncTs + const cloud = { id: 'u1', tokens: 100, staked: 30 }; // no _syncTs + + const { merged } = mergeAccount(local, cloud); + assert(merged.tokens === 180, '5. legacy: higher local tokens preserved'); + assert(merged.staked === 50, '6. legacy: higher local staked preserved'); +} + +// ── 7. Legacy: cloud-only fields still applied ─────────────────────────────── + +console.log('\n7. Legacy: cloud-only fields populated into result'); +{ + const local = { id: 'u1', tokens: 50 }; + const cloud = { id: 'u1', tokens: 40, name: 'Alice Updated', org: 'NewOrg' }; + + const { merged } = mergeAccount(local, cloud); + assert(merged.tokens === 50, '7a. higher local tokens preserved'); + assert(merged.name === 'Alice Updated', '7b. cloud name applied'); + assert(merged.org === 'NewOrg', '7c. cloud org applied'); +} + +// ── 8-9. Re-push behaviour ──────────────────────────────────────────────────── + +console.log('\n8-9. Re-push triggered only when local is newer'); +{ + const { repushed: repush8 } = mergeAccount( + { id: 'u1', tokens: 150, _syncTs: 2000 }, + { id: 'u1', tokens: 100, _syncTs: 1000 } + ); + assert(repush8 === true, '8. re-push when local newer'); + + const { repushed: repush9 } = mergeAccount( + { id: 'u1', tokens: 100, _syncTs: 1000 }, + { id: 'u1', tokens: 200, _syncTs: 2000 } + ); + assert(repush9 === false, '9. no re-push when cloud newer'); +} + +// ── 10-11. Non-financial fields follow the winning side ─────────────────────── + +console.log('\n10-11. Non-financial fields follow the winning side'); +{ + // Cloud wins path + const { merged: m10 } = mergeAccount( + { id: 'u1', tokens: 100, name: 'Old Name', _syncTs: 1000 }, + { id: 'u1', tokens: 200, name: 'New Name', _syncTs: 2000 } + ); + assert(m10.name === 'New Name', '10. cloud-wins: cloud name applied'); + + // Local wins path + const { merged: m11 } = mergeAccount( + { id: 'u1', tokens: 150, name: 'Local Name', _syncTs: 2000 }, + { id: 'u1', tokens: 100, name: 'Cloud Name', _syncTs: 1000 } + ); + assert(m11.name === 'Local Name', '11. local-wins: local name preserved'); +} + +// ── 12. Local newer: cloud-only fields still populated ─────────────────────── + +console.log('\n12. Local newer: cloud-only fields still populated'); +{ + const { merged } = mergeAccount( + { id: 'u1', tokens: 150, _syncTs: 2000 }, + { id: 'u1', tokens: 100, _syncTs: 1000, cloudOnlyField: 'present' } + ); + assert(merged.tokens === 150, '12a. local tokens preserved'); + assert(merged.cloudOnlyField === 'present', '12b. cloud-only field still populated'); +} + +// ── 13. Empty local → cloud wins entirely ──────────────────────────────────── + +console.log('\n13. Empty local account: cloud wins'); +{ + const { merged } = mergeAccount(null, { id: 'u1', tokens: 200, _syncTs: 1000 }); + assert(merged.tokens === 200, 'cloud tokens applied when local is empty'); +} + +// ── 14. No cloud account → no merge, local returned ────────────────────────── + +console.log('\n14. No cloud account: local preserved unchanged'); +{ + const local = { id: 'u1', tokens: 150, _syncTs: 2000 }; + const { merged } = mergeAccount(local, null); + assert(merged.tokens === 150, 'local tokens unchanged when no cloud account'); +} + +// ── 15. _syncTs preserved in merged result ──────────────────────────────────── + +console.log('\n15. _syncTs preserved in merged result'); +{ + const { merged } = mergeAccount( + { id: 'u1', tokens: 150, _syncTs: 2000 }, + { id: 'u1', tokens: 100, _syncTs: 1000 } + ); + assert(typeof merged._syncTs === 'number' && merged._syncTs > 0, + '_syncTs is a positive number in merged result'); +} + +// ── 16. Regression: offline token earn survives reconnect ───────────────────── + +console.log('\n16. Regression: offline token earn survives reconnect hydration'); +{ + // Device B offline state: started at 100, earned 50 tokens → 150 + const deviceBLocal = { id: 'u1', tokens: 150, _syncTs: 5000 }; + + // Appwrite (Device A's last sync): still 100 + const appwrite = { id: 'u1', tokens: 100, _syncTs: 1000 }; + + const { merged } = mergeAccount(deviceBLocal, appwrite); + assert(merged.tokens === 150, '[regression] offline-earned tokens (150) survive reconnect'); +} + +// ── 17. Multi-device divergence resolved by timestamp ──────────────────────── + +console.log('\n17. Multi-device divergence resolved by timestamp'); +{ + // Device A: spent 100 tokens, sync at ts=3000 → cloud=50 + const deviceA_cloud = { id: 'u1', tokens: 50, _syncTs: 3000 }; + + // Device B: independently spent 20 tokens, last local write at ts=2000 → local=80 + const deviceB_local = { id: 'u1', tokens: 80, _syncTs: 2000 }; + + // Device A's cloud is newer (3000 > 2000) → cloud wins + const { merged } = mergeAccount(deviceB_local, deviceA_cloud); + assert(merged.tokens === 50, + 'newer cloud state (Device A) wins over older local state (Device B)'); +} + +// ── Summary ─────────────────────────────────────────────────────────────────── + +console.log('\n──────────────────────────────────────'); +console.log(`Results: ${passed} passed, ${failed} failed`); +console.log('──────────────────────────────────────\n'); + +process.exit(failed > 0 ? 1 : 0); diff --git a/src/app.js b/src/app.js index 5883ebf..a0221ce 100644 --- a/src/app.js +++ b/src/app.js @@ -258,10 +258,18 @@ const DEDUP_WINDOW_MS = 30000; const DB = { get: (key) => { try { const r = window.localStorage.getItem(STORAGE_KEY_PREFIX + key); return r ? JSON.parse(r) : null; } catch { return null; } }, set: (key, val, options = {}) => { try { - window.localStorage.setItem(STORAGE_KEY_PREFIX + key, JSON.stringify(val)); + // Stamp account records with a local-write timestamp so hydrateFromCloud + // can compare freshness with the Appwrite copy and avoid silently + // overwriting offline-earned balances with a stale cloud snapshot. + // A fresh object is used so the live SESSION reference is never mutated + // by the sync machinery. + const stored = (key.startsWith('acc:') && val && typeof val === 'object') + ? { ...val, _syncTs: Date.now() } + : val; + window.localStorage.setItem(STORAGE_KEY_PREFIX + key, JSON.stringify(stored)); if (!options.silent && ReGenXRealtime) { ReGenXRealtime.syncStorageMutation({ - updates: [{ key: STORAGE_KEY_PREFIX + key, value: val, action: 'set' }], + updates: [{ key: STORAGE_KEY_PREFIX + key, value: stored, action: 'set' }], rooms: options.rooms, eventType: options.eventType || 'KPI_UPDATED', meta: options.meta || {} @@ -278,10 +286,10 @@ const DB = { } } else if (key.startsWith('acc:') && window.CloudSync) { if (navigator.onLine && window.CloudSync.isLive) { - window.CloudSync.pushAccount(val) - .catch(() => window.CloudSync.queueOfflineWrite(key, val)); + window.CloudSync.pushAccount(stored) + .catch(() => window.CloudSync.queueOfflineWrite(key, stored)); } else { - window.CloudSync.queueOfflineWrite(key, val); + window.CloudSync.queueOfflineWrite(key, stored); } } } diff --git a/src/cloud-sync.js b/src/cloud-sync.js index 275289b..5c9707a 100644 --- a/src/cloud-sync.js +++ b/src/cloud-sync.js @@ -347,6 +347,9 @@ export const CloudSync = { ['lat', 'lng', 'tokens', 'staked'].forEach(f => { sanitized[f] = account[f] != null ? Number(account[f]) : 0; }); + // Persist the local-write timestamp so hydrateFromCloud can compare + // freshness between the Appwrite snapshot and the local copy. + sanitized._syncTs = account._syncTs != null ? Number(account._syncTs) : 0; return sanitized; }, @@ -497,11 +500,67 @@ export const CloudSync = { CloudSync.fetchOrdersForUser(uid) ]); - // Hydrate account — cloud wins on financial fields + // Hydrate account — freshness-aware merge. + // + // The original code applied `{ ...localAcc, ...cloudAccount }` unconditionally, + // letting the cloud snapshot overwrite every local field regardless of which + // copy was more recent. This silently discarded balances earned offline + // (when the offline queue had not yet been flushed before the hydration ran). + // + // The fix: compare the _syncTs write-timestamp that DB.set now stamps on + // every local account write and that sanitizeAccount includes in every push + // to Appwrite. + // + // localTs > cloudTs → device had offline edits since the last cloud sync; + // local values take precedence and are re-pushed to + // bring Appwrite up to date. + // + // cloudTs >= localTs → cloud is current; standard cloud-wins merge applies. + // + // Safety net (no timestamps): when neither record carries a _syncTs (legacy + // accounts created before this fix) Math.max is applied to token and staked + // balances so an offline-earned balance is never silently discarded. if (cloudAccount) { const localRaw = localStorage.getItem(STORAGE_KEY_PREFIX + 'acc:' + uid); const localAcc = localRaw ? JSON.parse(localRaw) : {}; - const merged = { ...localAcc, ...cloudAccount }; + + const localTs = Number(localAcc._syncTs) || 0; + const cloudTs = Number(cloudAccount._syncTs) || 0; + + let merged; + if (localTs > cloudTs) { + // Local account is newer — the device had offline edits that have + // not yet reached Appwrite. Keep local values as primary and fill + // in fields that exist only on the cloud record. + merged = { ...cloudAccount, ...localAcc }; + // Re-push the fresher local state so Appwrite is brought up to date. + CloudSync.pushAccount(merged).catch(err => + console.warn('[CloudSync] Failed to re-sync newer local account to cloud', err) + ); + } else { + // Cloud is current (same or newer timestamp) — standard cloud-wins merge. + merged = { ...localAcc, ...cloudAccount }; + + // Safety net for records that predate the _syncTs field. + // If neither side has a timestamp, apply Math.max on financial + // fields: a higher local balance may reflect rewards earned offline + // before the queue was flushed, so we protect it from silent loss. + if (localTs === 0 && cloudTs === 0) { + if (typeof localAcc.tokens === 'number') { + merged.tokens = Math.max( + Number(localAcc.tokens), + Number(cloudAccount.tokens) || 0 + ); + } + if (typeof localAcc.staked === 'number') { + merged.staked = Math.max( + Number(localAcc.staked), + Number(cloudAccount.staked) || 0 + ); + } + } + } + localStorage.setItem(STORAGE_KEY_PREFIX + 'acc:' + uid, JSON.stringify(merged)); if (window.SESSION?.id === uid) { Object.assign(window.SESSION, merged);