diff --git a/frontend/deterministic-tests/src/test-runner.ts b/frontend/deterministic-tests/src/test-runner.ts index 8fdefcbe..ee2534a2 100644 --- a/frontend/deterministic-tests/src/test-runner.ts +++ b/frontend/deterministic-tests/src/test-runner.ts @@ -2,6 +2,7 @@ import type { TestDefinition, TestResult, TestStep } from "./test-definition"; import { DeterministicAgent } from "./deterministic-agent"; import type { ServerControl } from "./server-control"; import type { SyncSettings, Logger } from "sync-client"; +import { CONFLICT_PATH_REGEX } from "sync-client"; import { assert } from "./utils/assert"; import { AssertableState } from "./utils/assertable-state"; import { sleep } from "./utils/sleep"; @@ -326,6 +327,15 @@ export class TestRunner { this.logger.info("✓ All clients are consistent"); + const conflictFiles = referenceFiles.filter((path) => + CONFLICT_PATH_REGEX.test(path) + ); + if (conflictFiles.length > 0) { + throw new Error( + `Found ${conflictFiles.length} conflict file(s) — local displacements indicate a reconciliation regression: [${conflictFiles.join(", ")}]` + ); + } + if (verify) { this.logger.info("Running custom verification..."); try { diff --git a/frontend/sync-client/src/index.ts b/frontend/sync-client/src/index.ts index c79ace63..8958464d 100644 --- a/frontend/sync-client/src/index.ts +++ b/frontend/sync-client/src/index.ts @@ -37,6 +37,7 @@ export type { AuthenticationError } from "./errors/authentication-error"; export type { MaybeOutdatedClientCursors } from "./types/maybe-outdated-client-cursors"; export { DocumentSyncStatus } from "./types/document-sync-status"; export { SyncClient } from "./sync-client"; +export { CONFLICT_PATH_REGEX } from "./sync-operations/conflict-path"; export type { TextWithCursors, CursorPosition } from "reconcile-text"; export const debugging = { diff --git a/frontend/sync-client/src/services/server-config.ts b/frontend/sync-client/src/services/server-config.ts index da804b2f..662304bc 100644 --- a/frontend/sync-client/src/services/server-config.ts +++ b/frontend/sync-client/src/services/server-config.ts @@ -14,15 +14,14 @@ export class ServerConfig { private response: Promise | undefined; private config: ServerConfigData | undefined; - public constructor(private readonly syncService: SyncService) {} + public constructor(private readonly syncService: SyncService) { } private static validateConfig(config: ServerConfigData): void { if (config.supportedApiVersion !== SUPPORTED_API_VERSION) { const shouldUpgradeClient = config.supportedApiVersion > SUPPORTED_API_VERSION; throw new ServerVersionMismatchError( - `Unsupported API version: ${config.supportedApiVersion}. Consider upgrading the ${ - shouldUpgradeClient ? "client" : "sync-server" + `Unsupported API version: ${config.supportedApiVersion}. Consider upgrading the ${shouldUpgradeClient ? "client" : "sync-server" } to ensure compatibility` ); } @@ -41,7 +40,7 @@ export class ServerConfig { try { let { response } = this; if (!response || forceUpdate) { - response = this.response = this.syncService.ping(); + response = this.startPing(); } const result: PingResponse = await response; // it must be defined, otherwise we would have thrown above @@ -68,7 +67,7 @@ export class ServerConfig { public async getConfig(): Promise { if (!this.config) { - this.response ??= this.syncService.ping(); + this.response ??= this.startPing(); this.config = await this.response; } @@ -77,6 +76,17 @@ export class ServerConfig { return this.config; } + private startPing(): Promise { + const pending = this.syncService.ping().catch((e: unknown) => { + if (this.response === pending) { + this.response = undefined; + } + throw e; + }); + this.response = pending; + return pending; + } + public reset(): void { this.response = undefined; this.config = undefined; diff --git a/frontend/sync-client/src/sync-client.ts b/frontend/sync-client/src/sync-client.ts index 99a02a87..1258bba1 100644 --- a/frontend/sync-client/src/sync-client.ts +++ b/frontend/sync-client/src/sync-client.ts @@ -399,7 +399,7 @@ export class SyncClient { return DocumentSyncStatus.SYNCING_IS_DISABLED; } - if (!this.syncer.isFirstSyncStarted || !this.hasFinishedOfflineSync) { + if (!this.hasFinishedOfflineSync) { return DocumentSyncStatus.SYNCING; } @@ -441,20 +441,25 @@ export class SyncClient { } this.isDestroying = true; - await this.pause(); + // Run cleanup in `finally` so a thrown pause() — or anything else + // mid-shutdown — still leaves the client in the disposed state + // instead of bricked with subscribers/telemetry hanging on. + try { + await this.pause(); + } finally { + this.hasBeenDestroyed = true; - this.hasBeenDestroyed = true; + this.resetInMemoryState(); - this.resetInMemoryState(); + this.eventUnsubscribers.forEach((unsubscribe) => { + unsubscribe(); + }); + this.eventUnsubscribers.length = 0; - this.eventUnsubscribers.forEach((unsubscribe) => { - unsubscribe(); - }); - this.eventUnsubscribers.length = 0; + this.logger.info("SyncClient has been successfully disposed"); - this.logger.info("SyncClient has been successfully disposed"); - - this.unloadTelemetry?.(); + this.unloadTelemetry?.(); + } } private async startSyncing(): Promise { diff --git a/frontend/sync-client/src/sync-operations/sync-event-queue.ts b/frontend/sync-client/src/sync-operations/sync-event-queue.ts index 7ad34fdc..cdaa9923 100644 --- a/frontend/sync-client/src/sync-operations/sync-event-queue.ts +++ b/frontend/sync-client/src/sync-operations/sync-event-queue.ts @@ -38,8 +38,16 @@ export class SyncEventQueue { // It maps pending changes onto the local filesystem. private readonly events: SyncEvent[] = []; - // file creations for paths matching any of these patterns will be ignored - private ignorePatterns: RegExp[]; + // file creations for paths matching any of these patterns are ignored + // because the user explicitly told us to ignore them. + private userIgnorePatterns: RegExp[]; + + // Whether `CONFLICT_PATH_REGEX` is applied at enqueue time. Conflict files + // exist because the syncer set them aside; ignoring them at runtime + // prevents resync churn. During an offline scan we DO want to surface them + // so a stranded conflict file (e.g. one this client previously displaced + // and was unable to re-sync) gets picked up as a normal new file. + private ignoreConflictPaths = true; public constructor( private readonly settings: Settings, @@ -47,19 +55,16 @@ export class SyncEventQueue { initialState: Partial | undefined, private readonly saveData: (data: StoredSyncState) => Promise ) { - this.ignorePatterns = [ - CONFLICT_PATH_REGEX, // conflict paths need to be resolved before they can be synced again - ...globsToRegexes( - this.settings.getSettings().ignorePatterns, - this.logger - ) - ]; + this.userIgnorePatterns = globsToRegexes( + this.settings.getSettings().ignorePatterns, + this.logger + ); this.settings.onSettingsChanged.add((newSettings) => { - this.ignorePatterns = [ - CONFLICT_PATH_REGEX, - ...globsToRegexes(newSettings.ignorePatterns, this.logger) - ]; + this.userIgnorePatterns = globsToRegexes( + newSettings.ignorePatterns, + this.logger + ); }); initialState ??= {}; @@ -94,19 +99,35 @@ export class SyncEventQueue { this._lastSeenUpdateId.add(id); } + /** + * Toggle whether `CONFLICT_PATH_REGEX` filters incoming events. The + * offline scan flips this off so a stranded conflict file gets surfaced + * as a regular create; everywhere else conflict files stay ignored. + */ + public setIgnoreConflictPaths(ignore: boolean): void { + this.ignoreConflictPaths = ignore; + } + public async enqueue(input: FileSyncEvent): Promise { const path = input.type === SyncEventType.RemoteChange ? input.remoteVersion.relativePath : input.path; - if (this.ignorePatterns.some((pattern) => pattern.test(path))) { + if (this.userIgnorePatterns.some((pattern) => pattern.test(path))) { this.logger.info( `Ignoring ${input.type} for ${path} as it matches ignore patterns` ); return; } + if (this.ignoreConflictPaths && CONFLICT_PATH_REGEX.test(path)) { + this.logger.info( + `Ignoring ${input.type} for ${path} as it is a conflict path` + ); + return; + } + if (input.type === SyncEventType.RemoteChange) { this.events.push(input); return; @@ -198,11 +219,23 @@ export class SyncEventQueue { /** * Update the settled document map and persist the new document version. + * + * If the document is already tracked under a different path (e.g. after a + * rename) the old entry is removed so the map stays keyed by the latest + * disk path and `getDocumentByDocumentId` can't return a stale match. */ public async setDocument( path: RelativePath, record: DocumentRecord ): Promise { + for (const [existingPath, existingRecord] of this.documents) { + if ( + existingPath !== path && + existingRecord.documentId === record.documentId + ) { + this.documents.delete(existingPath); + } + } this.documents.set(path, record); return this.save(); } diff --git a/frontend/sync-client/src/sync-operations/syncer.ts b/frontend/sync-client/src/sync-operations/syncer.ts index 1bc3bd48..3e454b90 100644 --- a/frontend/sync-client/src/sync-operations/syncer.ts +++ b/frontend/sync-client/src/sync-operations/syncer.ts @@ -45,7 +45,6 @@ export class Syncer { private readonly queue: SyncEventQueue; - private _isFirstSyncStarted = false; private runningScheduleSyncForOfflineChanges: Promise | undefined; private drainPromise: Promise | undefined; private isScanning = false; @@ -75,10 +74,6 @@ export class Syncer { ); } - public get isFirstSyncStarted(): boolean { - return this._isFirstSyncStarted; - } - public syncLocallyCreatedFile(relativePath: RelativePath): void { void this.queue.enqueue({ type: SyncEventType.LocalCreate, @@ -121,8 +116,6 @@ export class Syncer { }); this.ensureDraining(); - - this._isFirstSyncStarted = true; } public async scheduleSyncForOfflineChanges(): Promise { @@ -160,7 +153,6 @@ export class Syncer { } public reset(): void { - this._isFirstSyncStarted = false; this.queue.clearPending(); const current = this.runningScheduleSyncForOfflineChanges; if (current !== undefined) { @@ -184,6 +176,10 @@ export class Syncer { private async internalScheduleSyncForOfflineChanges(): Promise { this.isScanning = true; + // Surface stranded conflict files (e.g. ones we displaced in a prior + // session and never resynced) as regular creates during the scan; the + // queue re-enables conflict filtering when we're done. + this.queue.setIgnoreConflictPaths(false); try { while (this.drainPromise !== undefined) { await this.drainPromise; @@ -203,6 +199,7 @@ export class Syncer { } ); } finally { + this.queue.setIgnoreConflictPaths(true); this.isScanning = false; }