Hoist retry logic

frontend/obsidian-plugin/src/views/settings-tab.ts

@@ -211,6 +211,7 @@ export class SyncSettingsTab extends PluginSettingTab {
 						new Notice(
 							"The changes have been applied successfully!"
 						);
+						await this.statusDescription.updateConnectionState();
 					} else {
 						new Notice("No changes to apply");
 					}

frontend/sync-client/src/services/connection-status.ts

@@ -39,25 +39,16 @@ export class ConnectionStatus {
 		return input.url;
 	}
 
-	public getFetchImplementation(
-		fetch: typeof globalThis.fetch,
-		{ doRetries = true }: { doRetries: boolean } = { doRetries: true }
-	): typeof globalThis.fetch {
-		return doRetries ? this.retriedFetchFactory(this.logger, fetch) : fetch;
-	}
-
 	public reset(): void {
 		this.rejectUntil(new Error("Sync was reset"));
 		[this.until, this.resolveUntil, this.rejectUntil] = createPromise();
 	}
 
-	private retriedFetchFactory(
+	public getFetchImplementation(
 		logger: Logger,
 		fetch: typeof globalThis.fetch = globalThis.fetch
 	): typeof globalThis.fetch {
 		return async (input: RequestInfo | URL): Promise<Response> => {
-			// eslint-disable-next-line @typescript-eslint/no-unnecessary-condition
-			while (true) {
 			while (!this.canFetch) {
 				await this.until;
 			}
@@ -65,8 +56,7 @@ export class ConnectionStatus {
 			try {
 				// https://github.com/jonbern/fetch-retry/blob/8684ef4e688375f623bd76f13add76dbc1d67cfb/index.js#L67C1-L70C21
 				const _input =
-						typeof Request !== "undefined" &&
-						input instanceof Request
+					typeof Request !== "undefined" && input instanceof Request
 						? input.clone()
 						: input;
 
@@ -82,7 +72,7 @@ export class ConnectionStatus {
 
 				if (!fetchResult.ok) {
 					this.logger.warn(
-							`Retrying fetch for ${ConnectionStatus.getUrlFromInput(
+						`Fetch for ${ConnectionStatus.getUrlFromInput(
 							input
 						)}, got status ${fetchResult.status}`
 					);
@@ -91,13 +81,11 @@ export class ConnectionStatus {
 				return fetchResult;
 			} catch (error) {
 				logger.warn(
-						`Retrying fetch for ${ConnectionStatus.getUrlFromInput(
+					`Fetch for ${ConnectionStatus.getUrlFromInput(
 						input
 					)}, got error: ${error}`
 				);
-				}
-
-				await Promise.race([this.until, sleep(1000)]);
+				throw error;
 			}
 		};
 	}

frontend/sync-client/src/services/sync-service.ts

@@ -9,6 +9,7 @@ import type {
 import type { Logger } from "../tracing/logger";
 import type { Settings } from "../persistence/settings";
 import type { ConnectionStatus } from "./connection-status";
+import { sleep } from "../utils/sleep";
 
 export interface CheckConnectionResult {
 	isSuccessful: boolean;
@@ -16,8 +17,8 @@ export interface CheckConnectionResult {
 }
 
 export class SyncService {
-	private client!: Client<paths>;
-	private clientWithoutRetries!: Client<paths>;
+	private client: Client<paths>;
+	private pingClient: Client<paths>;
 	private _fetchImplementation: typeof globalThis.fetch = globalThis.fetch;
 
 	public constructor(
@@ -25,20 +26,26 @@ export class SyncService {
 		private readonly settings: Settings,
 		private readonly logger: Logger
 	) {
-		this.createClient(this.settings.getSettings().remoteUri);
+		[this.client, this.pingClient] = this.createClient(
+			this.settings.getSettings().remoteUri
+		);
 
 		settings.addOnSettingsChangeListener((newSettings, oldSettings) => {
 			if (newSettings.remoteUri === oldSettings.remoteUri) {
 				return;
 			}
 
-			this.createClient(newSettings.remoteUri);
+			[this.client, this.pingClient] = this.createClient(
+				newSettings.remoteUri
+			);
 		});
 	}
 
 	public set fetchImplementation(fetch: typeof globalThis.fetch) {
 		this._fetchImplementation = fetch;
-		this.createClient(this.settings.getSettings().remoteUri);
+		[this.client, this.pingClient] = this.createClient(
+			this.settings.getSettings().remoteUri
+		);
 	}
 
 	private static formatError(
@@ -62,6 +69,7 @@ export class SyncService {
 		relativePath: RelativePath;
 		contentBytes: Uint8Array;
 	}): Promise<components["schemas"]["DocumentVersionWithoutContent"]> {
+		return this.withRetries(async () => {
 			const formData = new FormData();
 			if (documentId !== undefined) {
 				formData.append("document_id", documentId);
@@ -98,6 +106,7 @@ export class SyncService {
 			);
 
 			return response.data;
+		});
 	}
 
 	public async put({
@@ -111,6 +120,7 @@ export class SyncService {
 		relativePath: RelativePath;
 		contentBytes: Uint8Array;
 	}): Promise<components["schemas"]["DocumentUpdateResponse"]> {
+		return this.withRetries(async () => {
 			this.logger.debug(
 				`Updating document ${documentId} with parent version ${parentVersionId} and relative path ${relativePath}`
 			);
@@ -149,6 +159,7 @@ export class SyncService {
 			);
 
 			return response.data;
+		});
 	}
 
 	public async delete({
@@ -158,6 +169,7 @@ export class SyncService {
 		documentId: DocumentId;
 		relativePath: RelativePath;
 	}): Promise<components["schemas"]["DocumentVersionWithoutContent"]> {
+		return this.withRetries(async () => {
 			const response = await this.client.DELETE(
 				"/vaults/{vault_id}/documents/{document_id}",
 				{
@@ -185,6 +197,7 @@ export class SyncService {
 			);
 
 			return response.data;
+		});
 	}
 
 	public async get({
@@ -192,6 +205,7 @@ export class SyncService {
 	}: {
 		documentId: DocumentId;
 	}): Promise<components["schemas"]["DocumentVersion"]> {
+		return this.withRetries(async () => {
 			const response = await this.client.GET(
 				"/vaults/{vault_id}/documents/{document_id}",
 				{
@@ -218,12 +232,16 @@ export class SyncService {
 			);
 
 			return response.data;
+		});
 	}
 
 	public async getAll(
 		since?: VaultUpdateId
 	): Promise<components["schemas"]["FetchLatestDocumentsResponse"]> {
-		const response = await this.client.GET("/vaults/{vault_id}/documents", {
+		return this.withRetries(async () => {
+			const response = await this.client.GET(
+				"/vaults/{vault_id}/documents",
+				{
 					params: {
 						path: {
 							vault_id: this.settings.getSettings().vaultName
@@ -235,7 +253,8 @@ export class SyncService {
 							since_update_id: since
 						}
 					}
-		});
+				}
+			);
 
 			const { error } = response;
 			if (error) {
@@ -249,6 +268,7 @@ export class SyncService {
 			);
 
 			return response.data;
+		});
 	}
 
 	public async checkConnection(): Promise<CheckConnectionResult> {
@@ -273,8 +293,9 @@ export class SyncService {
 		}
 	}
 
+	// No retries
 	private async ping(): Promise<components["schemas"]["PingResponse"]> {
-		const response = await this.clientWithoutRetries.GET("/ping", {
+		const response = await this.pingClient.GET("/ping", {
 			params: {
 				header: {
 					authorization: `Bearer ${this.settings.getSettings().token}`
@@ -293,20 +314,34 @@ export class SyncService {
 		return response.data;
 	}
 
-	private createClient(remoteUri: string): void {
-		this.client = createClient<paths>({
+	/**
+	 * Create a client and a ping client for the given remote URI.
+	 */
+	private createClient(remoteUri: string): [Client<paths>, Client<paths>] {
+		return [
+			createClient<paths>({
 				baseUrl: remoteUri,
 				fetch: this.connectionStatus.getFetchImplementation(
+					this.logger,
 					this._fetchImplementation
 				)
-		});
-
-		this.clientWithoutRetries = createClient<paths>({
+			}),
+			createClient<paths>({
 				baseUrl: remoteUri,
-			fetch: this.connectionStatus.getFetchImplementation(
-				this._fetchImplementation,
-				{ doRetries: false }
-			)
-		});
+				fetch: this._fetchImplementation
+			})
+		];
+	}
+
+	private async withRetries<T>(fn: () => Promise<T>): Promise<T> {
+		// eslint-disable-next-line @typescript-eslint/no-unnecessary-condition
+		while (true) {
+			try {
+				return await fn();
+			} catch (e) {
+				this.logger.error(`Failed network call (${e}), retrying`);
+				await sleep(1000);
+			}
+		}
 	}
 }

frontend/sync-client/src/sync-operations/unrestricted-syncer.ts

@@ -177,7 +177,9 @@ export class UnrestrictedSyncer {
 				}
 
 				if (
-					document.metadata.parentVersionId >= response.vaultUpdateId
+					// `Syncer` creates fake local document metadata for all remote docs with invalid hashes. The parent IDs will likely match
+					// the latest versions so we still need to update the local versions to turn the fakes into real metadata.
+					document.metadata.parentVersionId > response.vaultUpdateId
 				) {
 					this.logger.debug(
 						`Document ${document.relativePath} is already more up to date than the fetched version`
@@ -281,7 +283,7 @@ export class UnrestrictedSyncer {
 						remoteVersion.vaultUpdateId
 					) {
 						this.logger.debug(
-							`Document ${remoteVersion.relativePath} is already more up to date than the fetched version`
+							`Document ${remoteVersion.relativePath} is already at least as up to date as the fetched version`
 						);
 						return;
 					}