mirror of
https://github.com/johannesjo/super-productivity.git
synced 2026-01-23 10:45:57 +00:00
fix(sync): detect server migration for file-based providers
File-based providers (Dropbox, WebDAV, LocalFile) don't set the gapDetected flag when connecting to an empty server, which caused server migration detection to fail. This resulted in "Already in Sync" with 0 ops uploaded when switching from SuperSync to Dropbox. Add alternative migration detection in OperationLogDownloadService: - Check when server is empty (finalLatestSeq === 0) regardless of lastServerSeq - Query hasSyncedOps() to detect prior sync history from another provider - Trigger needsFullStateUpload when client has ops but server is empty Key insight: lastServerSeq might be non-zero from a previous sync with the same provider (e.g., user previously synced with Dropbox, cleared folder, switching back). The migration should trigger based on server state, not local tracking state. Also remove unnecessary file-based provider check for IN_SYNC status - this was a workaround that's no longer needed now that migration detection works correctly.
This commit is contained in:
parent
3461cf8ac5
commit
2ed3800a9d
3 changed files with 194 additions and 10 deletions
|
|
@ -55,7 +55,6 @@ import { IS_ELECTRON } from '../../app.constants';
|
|||
import { OperationLogStoreService } from '../../op-log/persistence/operation-log-store.service';
|
||||
import { OperationLogSyncService } from '../../op-log/sync/operation-log-sync.service';
|
||||
import { WrappedProviderService } from '../../op-log/sync-providers/wrapped-provider.service';
|
||||
import { isFileBasedProvider } from '../../op-log/sync/operation-sync.util';
|
||||
|
||||
/**
|
||||
* Converts LegacySyncProvider to SyncProviderId.
|
||||
|
|
@ -237,15 +236,10 @@ export class SyncWrapperService {
|
|||
await this._opLogSyncService.uploadPendingOps(syncCapableProvider);
|
||||
}
|
||||
|
||||
// Mark as in-sync - only for SuperSync (API-based sync)
|
||||
// File-based providers (Dropbox, WebDAV, LocalFile) don't get IN_SYNC status
|
||||
// because we can't confirm real-time sync without server confirmation.
|
||||
if (rawProvider && !isFileBasedProvider(rawProvider)) {
|
||||
this._providerManager.setSyncStatus('IN_SYNC');
|
||||
SyncLog.log('SyncWrapperService: Sync complete, status=IN_SYNC');
|
||||
} else {
|
||||
SyncLog.log('SyncWrapperService: Sync complete (file-based provider)');
|
||||
}
|
||||
// Mark as in-sync for all providers after successful sync
|
||||
// This indicates the sync operation completed successfully
|
||||
this._providerManager.setSyncStatus('IN_SYNC');
|
||||
SyncLog.log('SyncWrapperService: Sync complete, status=IN_SYNC');
|
||||
return SyncStatus.InSync;
|
||||
} catch (error) {
|
||||
SyncLog.err(error);
|
||||
|
|
|
|||
|
|
@ -22,6 +22,7 @@ describe('OperationLogDownloadService', () => {
|
|||
beforeEach(() => {
|
||||
mockOpLogStore = jasmine.createSpyObj('OperationLogStoreService', [
|
||||
'getAppliedOpIds',
|
||||
'hasSyncedOps',
|
||||
]);
|
||||
mockLockService = jasmine.createSpyObj('LockService', ['request']);
|
||||
mockSnackService = jasmine.createSpyObj('SnackService', ['open']);
|
||||
|
|
@ -36,6 +37,7 @@ describe('OperationLogDownloadService', () => {
|
|||
},
|
||||
);
|
||||
mockOpLogStore.getAppliedOpIds.and.returnValue(Promise.resolve(new Set<string>()));
|
||||
mockOpLogStore.hasSyncedOps.and.returnValue(Promise.resolve(false));
|
||||
|
||||
TestBed.configureTestingModule({
|
||||
providers: [
|
||||
|
|
@ -1091,6 +1093,166 @@ describe('OperationLogDownloadService', () => {
|
|||
expect(mockApiProvider.setLastServerSeq).not.toHaveBeenCalled();
|
||||
});
|
||||
});
|
||||
|
||||
describe('server migration detection for file-based providers', () => {
|
||||
it('should set needsFullStateUpload when empty server AND first connect AND has synced ops', async () => {
|
||||
// Scenario: User switches from SuperSync to Dropbox
|
||||
// - Server is empty (no sync-data.json file)
|
||||
// - First time connecting (lastServerSeq === 0)
|
||||
// - Client has previously synced ops (from SuperSync)
|
||||
mockApiProvider.getLastServerSeq.and.returnValue(Promise.resolve(0));
|
||||
mockApiProvider.downloadOps.and.returnValue(
|
||||
Promise.resolve({
|
||||
ops: [],
|
||||
hasMore: false,
|
||||
latestSeq: 0, // Empty server
|
||||
}),
|
||||
);
|
||||
// Client has synced ops from previous provider
|
||||
mockOpLogStore.hasSyncedOps.and.returnValue(Promise.resolve(true));
|
||||
|
||||
const result = await service.downloadRemoteOps(mockApiProvider);
|
||||
|
||||
expect(result.needsFullStateUpload).toBeTrue();
|
||||
expect(OpLog.warn).toHaveBeenCalledWith(
|
||||
jasmine.stringContaining(
|
||||
'Server migration detected - empty server with synced ops',
|
||||
),
|
||||
);
|
||||
});
|
||||
|
||||
it('should NOT set needsFullStateUpload when empty server AND first connect AND NO synced ops', async () => {
|
||||
// Scenario: Fresh client connecting for the first time
|
||||
// - Server is empty
|
||||
// - First time connecting
|
||||
// - No previous sync history (fresh client)
|
||||
mockApiProvider.getLastServerSeq.and.returnValue(Promise.resolve(0));
|
||||
mockApiProvider.downloadOps.and.returnValue(
|
||||
Promise.resolve({
|
||||
ops: [],
|
||||
hasMore: false,
|
||||
latestSeq: 0,
|
||||
}),
|
||||
);
|
||||
// Client has NO synced ops (fresh client)
|
||||
mockOpLogStore.hasSyncedOps.and.returnValue(Promise.resolve(false));
|
||||
|
||||
const result = await service.downloadRemoteOps(mockApiProvider);
|
||||
|
||||
expect(result.needsFullStateUpload).toBeFalsy();
|
||||
});
|
||||
|
||||
it('should NOT set needsFullStateUpload when server has data', async () => {
|
||||
// Scenario: Joining existing sync group
|
||||
// - Server has ops
|
||||
// - Client may or may not have synced ops - irrelevant
|
||||
mockApiProvider.getLastServerSeq.and.returnValue(Promise.resolve(0));
|
||||
mockApiProvider.downloadOps.and.returnValue(
|
||||
Promise.resolve({
|
||||
ops: [
|
||||
{
|
||||
serverSeq: 1,
|
||||
receivedAt: Date.now(),
|
||||
op: {
|
||||
id: 'op-1',
|
||||
clientId: 'c1',
|
||||
actionType: '[Task] Add' as ActionType,
|
||||
opType: OpType.Create,
|
||||
entityType: 'TASK',
|
||||
payload: {},
|
||||
vectorClock: {},
|
||||
timestamp: Date.now(),
|
||||
schemaVersion: 1,
|
||||
},
|
||||
},
|
||||
],
|
||||
hasMore: false,
|
||||
latestSeq: 1,
|
||||
}),
|
||||
);
|
||||
mockOpLogStore.hasSyncedOps.and.returnValue(Promise.resolve(true));
|
||||
|
||||
const result = await service.downloadRemoteOps(mockApiProvider);
|
||||
|
||||
expect(result.needsFullStateUpload).toBeFalsy();
|
||||
});
|
||||
|
||||
it('should NOT set needsFullStateUpload when already synced with this server (server has data)', async () => {
|
||||
// Scenario: Regular sync with already-connected server
|
||||
// - lastServerSeq > 0 means we've synced before
|
||||
// - Server has data (latestSeq > 0)
|
||||
mockApiProvider.getLastServerSeq.and.returnValue(Promise.resolve(50)); // Already synced
|
||||
mockApiProvider.downloadOps.and.returnValue(
|
||||
Promise.resolve({
|
||||
ops: [],
|
||||
hasMore: false,
|
||||
latestSeq: 50, // Server has data
|
||||
}),
|
||||
);
|
||||
mockOpLogStore.hasSyncedOps.and.returnValue(Promise.resolve(true));
|
||||
|
||||
const result = await service.downloadRemoteOps(mockApiProvider);
|
||||
|
||||
expect(result.needsFullStateUpload).toBeFalsy();
|
||||
});
|
||||
|
||||
it('should set needsFullStateUpload when server was reset (previously synced, now empty)', async () => {
|
||||
// Scenario: User previously synced with Dropbox, then cleared the Dropbox folder
|
||||
// - lastServerSeq > 0 (had previous sync)
|
||||
// - Server is now empty (user deleted sync-data.json)
|
||||
// - Client has synced ops
|
||||
// This is a server reset scenario - need full state upload
|
||||
mockApiProvider.getLastServerSeq.and.returnValue(Promise.resolve(100)); // Had previous sync
|
||||
mockApiProvider.downloadOps.and.returnValue(
|
||||
Promise.resolve({
|
||||
ops: [],
|
||||
hasMore: false,
|
||||
latestSeq: 0, // Server is empty now
|
||||
}),
|
||||
);
|
||||
mockOpLogStore.hasSyncedOps.and.returnValue(Promise.resolve(true));
|
||||
|
||||
const result = await service.downloadRemoteOps(mockApiProvider);
|
||||
|
||||
expect(result.needsFullStateUpload).toBeTrue();
|
||||
expect(OpLog.warn).toHaveBeenCalledWith(
|
||||
jasmine.stringContaining(
|
||||
'Server migration detected - empty server with synced ops',
|
||||
),
|
||||
);
|
||||
});
|
||||
|
||||
it('should NOT set needsFullStateUpload if gapDetected migration already triggered', async () => {
|
||||
// Scenario: API-based provider with gap detection
|
||||
// The gapDetected path should take precedence
|
||||
mockApiProvider.getLastServerSeq.and.returnValue(Promise.resolve(100));
|
||||
mockApiProvider.downloadOps.and.returnValues(
|
||||
// Gap detected on empty server - triggers migration via hasResetForGap path
|
||||
Promise.resolve({
|
||||
ops: [],
|
||||
hasMore: false,
|
||||
latestSeq: 0,
|
||||
gapDetected: true,
|
||||
}),
|
||||
// After reset, still empty
|
||||
Promise.resolve({
|
||||
ops: [],
|
||||
hasMore: false,
|
||||
latestSeq: 0,
|
||||
}),
|
||||
);
|
||||
mockOpLogStore.hasSyncedOps.and.returnValue(Promise.resolve(true));
|
||||
|
||||
const result = await service.downloadRemoteOps(mockApiProvider);
|
||||
|
||||
// Should have been triggered by the gapDetected path, not the alternative path
|
||||
expect(result.needsFullStateUpload).toBeTrue();
|
||||
// The warning should mention the gap-based detection
|
||||
expect(OpLog.warn).toHaveBeenCalledWith(
|
||||
jasmine.stringContaining('gap on empty server'),
|
||||
);
|
||||
});
|
||||
});
|
||||
});
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -265,6 +265,34 @@ export class OperationLogDownloadService {
|
|||
);
|
||||
}
|
||||
|
||||
// Alternative migration detection for file-based providers:
|
||||
// When connecting to an empty server, check if the client has previously synced ops
|
||||
// (from another provider like SuperSync OR from a previous sync with this provider).
|
||||
// This handles:
|
||||
// 1. Provider switch scenario (e.g., SuperSync → Dropbox)
|
||||
// 2. Server reset scenario (e.g., user deleted sync-data.json in Dropbox)
|
||||
// File-based providers don't return gapDetected, so we need this alternative check.
|
||||
// NOTE: We check regardless of lastServerSeq because:
|
||||
// - lastServerSeq might be non-zero from a previous sync with this provider
|
||||
// - If server is empty but client has ops, we need to migrate regardless
|
||||
OpLog.verbose(
|
||||
`OperationLogDownloadService: Migration check - needsFullStateUpload=${needsFullStateUpload}, ` +
|
||||
`allNewOps=${allNewOps.length}, finalLatestSeq=${finalLatestSeq}, lastServerSeq=${lastServerSeq}`,
|
||||
);
|
||||
if (!needsFullStateUpload && allNewOps.length === 0 && finalLatestSeq === 0) {
|
||||
const hasSyncedOps = await this.opLogStore.hasSyncedOps();
|
||||
OpLog.verbose(
|
||||
`OperationLogDownloadService: Empty server detected, hasSyncedOps=${hasSyncedOps}`,
|
||||
);
|
||||
if (hasSyncedOps) {
|
||||
needsFullStateUpload = true;
|
||||
OpLog.warn(
|
||||
'OperationLogDownloadService: Server migration detected - empty server with synced ops. ' +
|
||||
'Full state upload will be required.',
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
OpLog.normal(
|
||||
`OperationLogDownloadService: Downloaded ${allNewOps.length} new operations via API.`,
|
||||
);
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue