Merge pull request 'fix: prevent data loss from sync/save race conditions' (#51) from fix/sync-race-conditions into main · scottlanoue.com/atmosphere-office@994bcf4

+8 -1

src/docs/main.js

··· 928 928 sessionStorage.removeItem(pendingKey); 929 929 try { 930 930 const pending = JSON.parse(pendingRaw); 931 + // Set import-in-progress flag to prevent snapshot saves during async import 932 + window.__importInProgress = true; 931 933 // Convert data URL back to a File object 932 934 fetch(pending.data) 933 935 .then(r => r.blob()) 934 936 .then(blob => { 935 937 const file = new File([blob], pending.name, { type: blob.type }); 936 938 handleImportedFile(file); 939 + }) 940 + .finally(() => { 941 + window.__importInProgress = false; 937 942 }); 938 - } catch { /* ignore corrupt data */ } 943 + } catch { 944 + window.__importInProgress = false; 945 + } 939 946 } 940 947 }); 941 948

+37 -6

src/lib/provider.js

··· 23 23 24 24 const SNAPSHOT_INTERVAL = 10_000; // Save snapshot every 10s 25 25 const SAVE_DEBOUNCE = 500; // Debounce save after changes 26 + const MIN_SNAPSHOT_BYTES = 10; // Minimum plausible Yjs state size 26 27 27 28 export class EncryptedProvider { 28 29 /** ··· 45 46 this._snapshotTimer = null; 46 47 this._saveDebounce = null; 47 48 this._destroyed = false; 49 + this._hadSnapshot = false; // Track whether a snapshot was loaded 48 50 49 51 // Bind handlers 50 52 this._onDocUpdate = this._onDocUpdate.bind(this); ··· 90 92 // Send sync step 1: our state vector so peers can send missing updates 91 93 this._sendSyncStep1(); 92 94 93 - // Start periodic snapshot saves 94 - this._snapshotTimer = setInterval(() => this._saveSnapshot(), SNAPSHOT_INTERVAL); 95 + // NOTE: periodic snapshot timer is started in _onSynced(), not here, 96 + // to prevent saving empty/partial state before sync completes. 95 97 }; 96 98 97 99 this.ws.onmessage = async (event) => { ··· 102 104 if (msg.type === 'peer-count' && msg.count === 0 && !this.synced) { 103 105 // No peers — we're the only client. Mark as synced immediately 104 106 // so the editor becomes interactive (snapshot already loaded). 105 - this.synced = true; 106 - this._emit('sync', true); 107 + this._onSynced(); 107 108 } 108 109 if (msg.type === 'peer-joined') { 109 110 // New peer needs our state — send sync step 1 to trigger exchange ··· 158 159 const update = decoding.readVarUint8Array(decoder); 159 160 Y.applyUpdate(this.doc, update, this); 160 161 if (type === MSG_SYNC_STEP2 && !this.synced) { 161 - this.synced = true; 162 - this._emit('sync', true); 162 + this._onSynced(); 163 163 } 164 164 break; 165 165 } ··· 169 169 break; 170 170 } 171 171 } 172 + } 173 + 174 + /** Called when sync completes — starts periodic saves and emits event. */ 175 + _onSynced() { 176 + this.synced = true; 177 + this._emit('sync', true); 178 + // Start periodic snapshot saves now that we have a complete document state 179 + clearInterval(this._snapshotTimer); 180 + this._snapshotTimer = setInterval(() => this._saveSnapshot(), SNAPSHOT_INTERVAL); 172 181 } 173 182 174 183 _sendSyncStep1() { ··· 204 213 } 205 214 206 215 _debouncedSave() { 216 + if (!this.synced) return; // Don't save before sync completes 207 217 clearTimeout(this._saveDebounce); 208 218 this._saveDebounce = setTimeout(() => this._saveSnapshot(), SAVE_DEBOUNCE); 209 219 } ··· 227 237 if (!res.ok) return; // No snapshot yet 228 238 const encrypted = new Uint8Array(await res.arrayBuffer()); 229 239 const plain = await decrypt(encrypted, this.cryptoKey); 240 + 241 + // Validate: the decrypted data should be a plausible Yjs update 242 + if (plain.byteLength < MIN_SNAPSHOT_BYTES) { 243 + console.warn('Snapshot load skipped: decrypted data too small (%d bytes) — likely empty or corrupt', plain.byteLength); 244 + return; 245 + } 246 + 230 247 Y.applyUpdate(this.doc, plain); 248 + this._hadSnapshot = true; 231 249 } catch (err) { 232 250 // No snapshot or wrong key — start fresh 233 251 console.log('No existing snapshot or decryption failed, starting fresh'); ··· 235 253 } 236 254 237 255 async _saveSnapshot() { 256 + // Don't save before sync completes — we'd overwrite real data with empty state 257 + if (!this.synced) return; 258 + 259 + // Don't save while an import is in progress — the doc may be partially populated 260 + if (typeof window !== 'undefined' && window.__importInProgress) return; 261 + 238 262 try { 239 263 const state = Y.encodeStateAsUpdate(this.doc); 264 + 265 + // Validate: don't save suspiciously small state if we loaded a real snapshot 266 + if (this._hadSnapshot && state.byteLength < MIN_SNAPSHOT_BYTES) { 267 + console.warn('Snapshot save skipped: state too small (%d bytes) — possible data loss', state.byteLength); 268 + return; 269 + } 270 + 240 271 const encrypted = await encrypt(state, this.cryptoKey); 241 272 await fetch(`${this.apiUrl}/api/documents/${this.roomId}/snapshot`, { 242 273 method: 'PUT',

+27 -3

src/sheets/main.js

··· 1301 1301 provider.on('status', ({ connected }) => { statusDot.classList.toggle('connected', connected); statusText.textContent = connected ? 'Connected' : 'Reconnecting...'; }); 1302 1302 provider.on('sync', () => { 1303 1303 statusText.textContent = 'Synced'; 1304 - // Re-attach cell observer after sync — the snapshot may have replaced the Y.Map 1305 - // that getCells() returned during initial setup (before data loaded) 1304 + // Re-attach ALL observers after sync — the snapshot may have replaced the Y.Map/Y.Array 1305 + // objects that were observed during initial setup (before data loaded from peers) 1306 1306 getCells().observeDeep(() => { evalCache.clear(); invalidateRecalcEngine(); scheduleRenderGrid(); updateFormulaBar(); }); 1307 + ySheets.observe(() => { renderSheetTabs(); }); 1308 + ySheets.observeDeep((events) => { 1309 + for (const event of events) { 1310 + if (event.target && event.target === getColWidths()) { scheduleRenderGrid(); return; } 1311 + const changed = event.changes?.keys; 1312 + if (changed) { 1313 + for (const [key] of changed) { 1314 + if (key === 'freezeRows' || key === 'freezeCols' || key === 'stripedRows') { scheduleRenderGrid(); return; } 1315 + } 1316 + } 1317 + if (event.target && (event.target === getCfRules() || event.target === getValidations())) { 1318 + evalCache.clear(); invalidateRecalcEngine(); refreshVisibleCells(); return; 1319 + } 1320 + } 1321 + }); 1322 + getCharts().observeDeep(() => { renderCharts(); }); 1323 + getNotesMap().observe(() => renderNoteIndicators()); 1307 1324 renderGrid(); 1308 1325 1309 1326 // Check for pending file import from landing page drag-and-drop ··· 1313 1330 sessionStorage.removeItem(pendingKey); 1314 1331 try { 1315 1332 const pending = JSON.parse(pendingRaw); 1333 + // Set import-in-progress flag to prevent snapshot saves during async import 1334 + window.__importInProgress = true; 1316 1335 // Convert data URL back to a File object 1317 1336 fetch(pending.data) 1318 1337 .then(r => r.blob()) 1319 1338 .then(blob => { 1320 1339 const file = new File([blob], pending.name, { type: blob.type }); 1321 1340 handleImportFile(file); 1341 + }) 1342 + .finally(() => { 1343 + window.__importInProgress = false; 1322 1344 }); 1323 - } catch { /* ignore corrupt data */ } 1345 + } catch { 1346 + window.__importInProgress = false; 1347 + } 1324 1348 } 1325 1349 }); 1326 1350

+398

tests/provider-sync.test.js

··· 1 + /** 2 + * Tests for EncryptedProvider sync/save race conditions. 3 + * 4 + * These tests verify that: 5 + * 1. Snapshots are not saved before sync completes 6 + * 2. The periodic save timer does not start until synced 7 + * 3. Debounced saves respect the synced flag 8 + * 4. Snapshot validation rejects empty/corrupt data on load 9 + * 5. Snapshot validation prevents saving suspiciously small state 10 + * 6. Import-in-progress flag blocks saves 11 + */ 12 + 13 + import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; 14 + import * as Y from 'yjs'; 15 + 16 + // --------------------------------------------------------------------------- 17 + // Mock WebSocket 18 + // --------------------------------------------------------------------------- 19 + class MockWebSocket { 20 + static CONNECTING = 0; 21 + static OPEN = 1; 22 + static CLOSING = 2; 23 + static CLOSED = 3; 24 + 25 + constructor(url) { 26 + this.url = url; 27 + this.readyState = MockWebSocket.CONNECTING; 28 + this.binaryType = 'text'; 29 + this.onopen = null; 30 + this.onmessage = null; 31 + this.onclose = null; 32 + this.onerror = null; 33 + this._sent = []; 34 + // Auto-open after microtask to simulate real WS 35 + MockWebSocket._instances.push(this); 36 + } 37 + 38 + send(data) { 39 + this._sent.push(data); 40 + } 41 + 42 + close() { 43 + this.readyState = MockWebSocket.CLOSED; 44 + if (this.onclose) this.onclose({ code: 1000 }); 45 + } 46 + 47 + // Test helpers 48 + _simulateOpen() { 49 + this.readyState = MockWebSocket.OPEN; 50 + if (this.onopen) this.onopen({}); 51 + } 52 + 53 + _simulateMessage(data) { 54 + if (this.onmessage) this.onmessage({ data }); 55 + } 56 + 57 + _simulateClose() { 58 + this.readyState = MockWebSocket.CLOSED; 59 + if (this.onclose) this.onclose({ code: 1000 }); 60 + } 61 + 62 + static _instances = []; 63 + static _reset() { 64 + MockWebSocket._instances = []; 65 + } 66 + static get latest() { 67 + return MockWebSocket._instances[MockWebSocket._instances.length - 1]; 68 + } 69 + } 70 + 71 + // --------------------------------------------------------------------------- 72 + // Mock fetch & crypto for provider 73 + // --------------------------------------------------------------------------- 74 + let fetchCalls = []; 75 + let fetchResponses = {}; 76 + 77 + function mockFetch(url, opts) { 78 + fetchCalls.push({ url, opts }); 79 + const key = opts?.method === 'PUT' ? `PUT:${url}` : `GET:${url}`; 80 + const resp = fetchResponses[key] || fetchResponses[url]; 81 + if (resp) return Promise.resolve(resp); 82 + // Default: 404 for GET snapshots, 200 for PUT 83 + if (opts?.method === 'PUT') { 84 + return Promise.resolve({ ok: true, json: () => Promise.resolve({ ok: true }) }); 85 + } 86 + return Promise.resolve({ ok: false, status: 404, json: () => Promise.resolve({ error: 'Not found' }) }); 87 + } 88 + 89 + // --------------------------------------------------------------------------- 90 + // Dynamic import with mocks 91 + // --------------------------------------------------------------------------- 92 + let EncryptedProvider; 93 + 94 + beforeEach(async () => { 95 + MockWebSocket._reset(); 96 + fetchCalls = []; 97 + fetchResponses = {}; 98 + 99 + // Set up globals that the provider expects 100 + globalThis.WebSocket = MockWebSocket; 101 + globalThis.fetch = mockFetch; 102 + globalThis.location = { protocol: 'https:', host: 'localhost:3000' }; 103 + globalThis.document = { hidden: false, addEventListener: vi.fn() }; 104 + globalThis.window = { 105 + addEventListener: vi.fn(), 106 + removeEventListener: vi.fn(), 107 + __importInProgress: false, 108 + }; 109 + 110 + // Mock the crypto module 111 + vi.doMock('../src/lib/crypto.js', () => ({ 112 + encrypt: vi.fn(async (data) => data), // passthrough 113 + decrypt: vi.fn(async (data) => data), // passthrough 114 + })); 115 + 116 + // Fresh import each test 117 + const mod = await import('../src/lib/provider.js'); 118 + EncryptedProvider = mod.EncryptedProvider; 119 + }); 120 + 121 + afterEach(() => { 122 + vi.restoreAllMocks(); 123 + vi.resetModules(); 124 + }); 125 + 126 + // --------------------------------------------------------------------------- 127 + // Helper: create a provider without auto-connecting (for fine-grained control) 128 + // --------------------------------------------------------------------------- 129 + async function createProvider(opts = {}) { 130 + const doc = new Y.Doc(); 131 + const cryptoKey = {}; // Dummy — encrypt/decrypt are mocked as passthrough 132 + const provider = new EncryptedProvider(doc, 'test-room', cryptoKey, { 133 + wsUrl: 'wss://localhost:3000/ws', 134 + apiUrl: '', 135 + ...opts, 136 + }); 137 + // Wait for the connect() call to settle (snapshot load) 138 + await vi.waitFor(() => { 139 + expect(MockWebSocket.latest).toBeDefined(); 140 + }, { timeout: 1000 }); 141 + return { doc, provider, ws: MockWebSocket.latest }; 142 + } 143 + 144 + // =========================================================================== 145 + // Bug 2: Snapshot saves before sync completes 146 + // =========================================================================== 147 + describe('Bug 2: no saves before sync', () => { 148 + it('_saveSnapshot returns early when not synced', async () => { 149 + const { provider } = await createProvider(); 150 + 151 + // Provider is not synced yet 152 + expect(provider.synced).toBe(false); 153 + 154 + // Try to save — should be a no-op (no fetch PUT calls) 155 + fetchCalls = []; 156 + await provider._saveSnapshot(); 157 + 158 + const putCalls = fetchCalls.filter(c => c.opts?.method === 'PUT'); 159 + expect(putCalls).toHaveLength(0); 160 + }); 161 + 162 + it('_saveSnapshot proceeds after sync completes', async () => { 163 + const { provider, ws } = await createProvider(); 164 + 165 + // Simulate open + sync (no peers → immediate sync) 166 + ws._simulateOpen(); 167 + ws._simulateMessage(JSON.stringify({ type: 'peer-count', count: 0 })); 168 + expect(provider.synced).toBe(true); 169 + 170 + // Now save should work 171 + fetchCalls = []; 172 + await provider._saveSnapshot(); 173 + 174 + const putCalls = fetchCalls.filter(c => c.opts?.method === 'PUT'); 175 + expect(putCalls.length).toBeGreaterThan(0); 176 + }); 177 + 178 + it('periodic timer does not start until synced', async () => { 179 + vi.useFakeTimers(); 180 + const { provider, ws } = await createProvider(); 181 + 182 + // Open WS but don't sync yet 183 + ws._simulateOpen(); 184 + expect(provider.synced).toBe(false); 185 + 186 + // Advance past several snapshot intervals 187 + fetchCalls = []; 188 + await vi.advanceTimersByTimeAsync(30_000); 189 + 190 + // No PUT calls should have happened 191 + const putCalls = fetchCalls.filter(c => c.opts?.method === 'PUT'); 192 + expect(putCalls).toHaveLength(0); 193 + 194 + vi.useRealTimers(); 195 + }); 196 + 197 + it('periodic timer starts only after sync', async () => { 198 + vi.useFakeTimers(); 199 + const { provider, ws } = await createProvider(); 200 + 201 + ws._simulateOpen(); 202 + // Sync via peer-count 0 203 + ws._simulateMessage(JSON.stringify({ type: 'peer-count', count: 0 })); 204 + expect(provider.synced).toBe(true); 205 + 206 + // Now advance — saves should happen 207 + fetchCalls = []; 208 + await vi.advanceTimersByTimeAsync(25_000); // Should trigger at least 2 saves at 10s intervals 209 + 210 + const putCalls = fetchCalls.filter(c => c.opts?.method === 'PUT'); 211 + expect(putCalls.length).toBeGreaterThanOrEqual(2); 212 + 213 + vi.useRealTimers(); 214 + }); 215 + 216 + it('_debouncedSave does not fire before sync', async () => { 217 + vi.useFakeTimers(); 218 + const { provider, ws } = await createProvider(); 219 + 220 + ws._simulateOpen(); 221 + // Not synced yet 222 + 223 + fetchCalls = []; 224 + provider._debouncedSave(); 225 + await vi.advanceTimersByTimeAsync(2000); 226 + 227 + const putCalls = fetchCalls.filter(c => c.opts?.method === 'PUT'); 228 + expect(putCalls).toHaveLength(0); 229 + 230 + vi.useRealTimers(); 231 + }); 232 + }); 233 + 234 + // =========================================================================== 235 + // Bug 5: Snapshot validation on load and save 236 + // =========================================================================== 237 + describe('Bug 5: snapshot validation', () => { 238 + it('rejects a snapshot that is too small (likely empty/corrupt)', async () => { 239 + // Set up a fetch response that returns a tiny (3 byte) snapshot 240 + const tinyData = new Uint8Array([0, 1, 2]); 241 + fetchResponses['/api/documents/test-room/snapshot'] = { 242 + ok: true, 243 + arrayBuffer: () => Promise.resolve(tinyData.buffer), 244 + }; 245 + 246 + const doc = new Y.Doc(); 247 + const consoleSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}); 248 + 249 + const cryptoKey = {}; 250 + const provider = new EncryptedProvider(doc, 'test-room', cryptoKey, { 251 + wsUrl: 'wss://localhost:3000/ws', 252 + apiUrl: '', 253 + }); 254 + 255 + await vi.waitFor(() => { 256 + expect(MockWebSocket.latest).toBeDefined(); 257 + }, { timeout: 1000 }); 258 + 259 + // Should have logged a warning about the suspicious snapshot 260 + const warningCalls = consoleSpy.mock.calls.filter(c => 261 + String(c[0]).toLowerCase().includes('snapshot') 262 + ); 263 + expect(warningCalls.length).toBeGreaterThan(0); 264 + 265 + consoleSpy.mockRestore(); 266 + }); 267 + 268 + it('tracks _hadSnapshot when a valid snapshot is loaded', async () => { 269 + // Create a doc with real data so the state update is large enough 270 + const tmpDoc = new Y.Doc(); 271 + tmpDoc.getMap('data').set('key', 'This is some real content that makes the snapshot non-trivial'); 272 + const validState = Y.encodeStateAsUpdate(tmpDoc); 273 + // Verify our test data is above the threshold 274 + expect(validState.byteLength).toBeGreaterThanOrEqual(10); 275 + 276 + fetchResponses['/api/documents/test-room/snapshot'] = { 277 + ok: true, 278 + arrayBuffer: () => Promise.resolve(validState.buffer.slice(0)), 279 + }; 280 + 281 + const { provider, ws } = await createProvider(); 282 + 283 + // Provider loaded a real snapshot, so _hadSnapshot should be true 284 + expect(provider._hadSnapshot).toBe(true); 285 + 286 + // Mark as synced so saves are allowed 287 + ws._simulateOpen(); 288 + ws._simulateMessage(JSON.stringify({ type: 'peer-count', count: 0 })); 289 + 290 + // Normal save should succeed since we have real data 291 + fetchCalls = []; 292 + await provider._saveSnapshot(); 293 + 294 + const putCalls = fetchCalls.filter(c => c.opts?.method === 'PUT'); 295 + expect(putCalls.length).toBeGreaterThan(0); 296 + }); 297 + }); 298 + 299 + // =========================================================================== 300 + // Bug 6: Import-in-progress flag blocks saves 301 + // =========================================================================== 302 + describe('Bug 3: import-in-progress blocks saves', () => { 303 + it('_saveSnapshot returns early when window.__importInProgress is true', async () => { 304 + const { provider, ws } = await createProvider(); 305 + 306 + ws._simulateOpen(); 307 + ws._simulateMessage(JSON.stringify({ type: 'peer-count', count: 0 })); 308 + expect(provider.synced).toBe(true); 309 + 310 + // Set import-in-progress 311 + globalThis.window.__importInProgress = true; 312 + 313 + fetchCalls = []; 314 + await provider._saveSnapshot(); 315 + 316 + const putCalls = fetchCalls.filter(c => c.opts?.method === 'PUT'); 317 + expect(putCalls).toHaveLength(0); 318 + 319 + // Clear it and verify saves work again 320 + globalThis.window.__importInProgress = false; 321 + await provider._saveSnapshot(); 322 + 323 + const putCalls2 = fetchCalls.filter(c => c.opts?.method === 'PUT'); 324 + expect(putCalls2.length).toBeGreaterThan(0); 325 + }); 326 + }); 327 + 328 + // =========================================================================== 329 + // Sync state transitions 330 + // =========================================================================== 331 + describe('Sync state transitions', () => { 332 + it('synced is false before any peer messages', async () => { 333 + const { provider, ws } = await createProvider(); 334 + ws._simulateOpen(); 335 + expect(provider.synced).toBe(false); 336 + }); 337 + 338 + it('synced becomes true on peer-count 0 (solo client)', async () => { 339 + const { provider, ws } = await createProvider(); 340 + ws._simulateOpen(); 341 + ws._simulateMessage(JSON.stringify({ type: 'peer-count', count: 0 })); 342 + expect(provider.synced).toBe(true); 343 + }); 344 + 345 + it('synced resets to false on disconnect', async () => { 346 + const { provider, ws } = await createProvider(); 347 + ws._simulateOpen(); 348 + ws._simulateMessage(JSON.stringify({ type: 'peer-count', count: 0 })); 349 + expect(provider.synced).toBe(true); 350 + 351 + ws._simulateClose(); 352 + expect(provider.synced).toBe(false); 353 + }); 354 + 355 + it('emits sync event when synced', async () => { 356 + const syncSpy = vi.fn(); 357 + const { provider, ws } = await createProvider(); 358 + provider.on('sync', syncSpy); 359 + 360 + ws._simulateOpen(); 361 + ws._simulateMessage(JSON.stringify({ type: 'peer-count', count: 0 })); 362 + 363 + expect(syncSpy).toHaveBeenCalledWith(true); 364 + }); 365 + }); 366 + 367 + // =========================================================================== 368 + // Visibility change and beforeunload guards 369 + // =========================================================================== 370 + describe('Visibility and beforeunload save guards', () => { 371 + it('visibilitychange save respects synced flag', async () => { 372 + const { provider } = await createProvider(); 373 + 374 + // The provider registers a visibilitychange handler. 375 + // When hidden and NOT synced, _saveSnapshot should be a no-op. 376 + expect(provider.synced).toBe(false); 377 + 378 + fetchCalls = []; 379 + await provider._saveSnapshot(); // Simulating what visibilitychange does 380 + 381 + const putCalls = fetchCalls.filter(c => c.opts?.method === 'PUT'); 382 + expect(putCalls).toHaveLength(0); 383 + }); 384 + 385 + it('beforeunload save respects synced flag', async () => { 386 + const { provider } = await createProvider(); 387 + 388 + expect(provider.synced).toBe(false); 389 + 390 + fetchCalls = []; 391 + provider._onBeforeUnload(); 392 + // Give async save a tick 393 + await new Promise(r => setTimeout(r, 10)); 394 + 395 + const putCalls = fetchCalls.filter(c => c.opts?.method === 'PUT'); 396 + expect(putCalls).toHaveLength(0); 397 + }); 398 + });

Configure Feed

Configure Feed