From 0fda95ff8efcaf3bccf27414b50b969bdd7721e0 Mon Sep 17 00:00:00 2001 From: Andras Schmelczer Date: Fri, 8 May 2026 21:36:54 +0100 Subject: [PATCH] split: sync-client file-operations + persistence Rewrite file-operations and safe-filesystem-operations (and their tests), update filesystem-operations. Drop persistence/database.ts (in-memory record store moved into sync-event-queue). Update persistence/settings.ts. --- .../file-operations/file-operations.test.ts | 232 ++++------- .../src/file-operations/file-operations.ts | 290 +++++++------- .../file-operations/filesystem-operations.ts | 2 +- .../safe-filesystem-operations.ts | 92 +---- .../sync-client/src/persistence/database.ts | 374 ------------------ .../sync-client/src/persistence/settings.ts | 10 +- 6 files changed, 251 insertions(+), 749 deletions(-) delete mode 100644 frontend/sync-client/src/persistence/database.ts diff --git a/frontend/sync-client/src/file-operations/file-operations.test.ts b/frontend/sync-client/src/file-operations/file-operations.test.ts index 998e47ec..7916ab57 100644 --- a/frontend/sync-client/src/file-operations/file-operations.test.ts +++ b/frontend/sync-client/src/file-operations/file-operations.test.ts @@ -1,15 +1,14 @@ import { describe, it } from "node:test"; -import type { - Database, - DocumentRecord, - RelativePath -} from "../persistence/database"; +import assert from "node:assert/strict"; +import type { RelativePath } from "../sync-operations/types"; import { FileOperations } from "./file-operations"; import { Logger } from "../tracing/logger"; import { assertSetContainsExactly } from "../utils/assert-set-contains-exactly"; import type { FileSystemOperations } from "./filesystem-operations"; import type { TextWithCursors } from "reconcile-text"; import type { ServerConfig, ServerConfigData } from "../services/server-config"; +import { ExpectedFsEvents } from "../sync-operations/expected-fs-events"; +import { FileAlreadyExistsError } from "../errors/file-already-exists-error"; class MockServerConfig implements Pick { public async getConfig(): Promise { @@ -21,29 +20,13 @@ class MockServerConfig implements Pick { } } -class MockDatabase implements Partial { - public getLatestDocumentByRelativePath( - _find: RelativePath - ): DocumentRecord | undefined { - // no-op - return undefined; - } - - public move( - _oldRelativePath: RelativePath, - _newRelativePath: RelativePath - ): void { - // no-op - } -} - class FakeFileSystemOperations implements FileSystemOperations { public readonly names = new Set(); public async listFilesRecursively( _root: RelativePath | undefined ): Promise { - return ["file.md"]; + return Array.from(this.names); } public async read(_path: RelativePath): Promise { throw new Error("Method not implemented."); @@ -63,17 +46,14 @@ class FakeFileSystemOperations implements FileSystemOperations { public async getFileSize(_path: RelativePath): Promise { throw new Error("Method not implemented."); } - public async getModificationTime(_path: RelativePath): Promise { - throw new Error("Method not implemented."); - } public async exists(path: RelativePath): Promise { return this.names.has(path); } public async createDirectory(_path: RelativePath): Promise { - // this is called but irrelevant for this mock + // no-op for the in-memory fake; we only track files } - public async delete(_path: RelativePath): Promise { - throw new Error("Method not implemented."); + public async delete(path: RelativePath): Promise { + this.names.delete(path); } public async rename( oldPath: RelativePath, @@ -84,152 +64,92 @@ class FakeFileSystemOperations implements FileSystemOperations { } } +function makeOps(): { + fs: FakeFileSystemOperations; + ops: FileOperations; +} { + const fs = new FakeFileSystemOperations(); + const ops = new FileOperations( + new Logger(), + fs, + new MockServerConfig() as ServerConfig, // eslint-disable-line @typescript-eslint/no-unsafe-type-assertion + new ExpectedFsEvents() + ); + return { fs, ops }; +} + describe("File operations", () => { - it("should deconflict renames", async () => { - const fileSystemOperations = new FakeFileSystemOperations(); - const fileOperations = new FileOperations( - new Logger(), - new MockDatabase() as Database, // eslint-disable-line @typescript-eslint/no-unsafe-type-assertion - fileSystemOperations, - new MockServerConfig() as ServerConfig // eslint-disable-line @typescript-eslint/no-unsafe-type-assertion - ); + it("create writes the file at the requested path", async () => { + const { fs, ops } = makeOps(); - await fileOperations.create("a", new Uint8Array()); - assertSetContainsExactly(fileSystemOperations.names, "a"); - await fileOperations.move("a", "b"); - assertSetContainsExactly(fileSystemOperations.names, "b"); + const result = await ops.create("a", new Uint8Array()); - await fileOperations.create("c", new Uint8Array()); - assertSetContainsExactly(fileSystemOperations.names, "b", "c"); - - await fileOperations.move("c", "b"); - assertSetContainsExactly(fileSystemOperations.names, "b", "b (1)"); - - await fileOperations.create("c", new Uint8Array()); - await fileOperations.move("c", "b"); - assertSetContainsExactly( - fileSystemOperations.names, - "b", - "b (1)", - "b (2)" - ); + assertSetContainsExactly(fs.names, "a"); + assert.equal(result.actualPath, "a"); }); - it("should deconflict renames with file extension", async () => { - const fileSystemOperations = new FakeFileSystemOperations(); - const fileOperations = new FileOperations( - new Logger(), - new MockDatabase() as Database, // eslint-disable-line @typescript-eslint/no-unsafe-type-assertion - fileSystemOperations, - new MockServerConfig() as ServerConfig // eslint-disable-line @typescript-eslint/no-unsafe-type-assertion + it("create throws FileAlreadyExistsError when the path is occupied", async () => { + const { fs, ops } = makeOps(); + + await ops.create("note.md", new Uint8Array()); + await assert.rejects( + ops.create("note.md", new Uint8Array()), + FileAlreadyExistsError ); - await fileOperations.create("b.md", new Uint8Array()); - await fileOperations.create("c.md", new Uint8Array()); - await fileOperations.move("c.md", "b.md"); - assertSetContainsExactly( - fileSystemOperations.names, - "b.md", - "b (1).md" - ); - - await fileOperations.create("d.md", new Uint8Array()); - await fileOperations.move("d.md", "b.md"); - assertSetContainsExactly( - fileSystemOperations.names, - "b.md", - "b (1).md", - "b (2).md" - ); - - await fileOperations.create("file-23.md", new Uint8Array()); - await fileOperations.create("file-23 (1).md", new Uint8Array()); - await fileOperations.move("file-23.md", "file-23 (1).md"); - assertSetContainsExactly( - fileSystemOperations.names, - "b.md", - "b (1).md", - "b (2).md", - "file-23 (1).md", - "file-23 (2).md" - ); + // The original file is left intact and no other entries appeared. + assertSetContainsExactly(fs.names, "note.md"); }); - it("should deconflict renames with paths", async () => { - const fileSystemOperations = new FakeFileSystemOperations(); - const fileOperations = new FileOperations( - new Logger(), - new MockDatabase() as Database, // eslint-disable-line @typescript-eslint/no-unsafe-type-assertion - fileSystemOperations, - new MockServerConfig() as ServerConfig // eslint-disable-line @typescript-eslint/no-unsafe-type-assertion - ); + it("move to an empty target just renames the file", async () => { + const { fs, ops } = makeOps(); - await fileOperations.create("a/b.c/d", new Uint8Array()); - await fileOperations.create("a/b.c/e", new Uint8Array()); - await fileOperations.move("a/b.c/d", "a/b.c/e"); - assertSetContainsExactly( - fileSystemOperations.names, - "a/b.c/e", - "a/b.c/e (1)" - ); + await ops.create("a", new Uint8Array()); + assertSetContainsExactly(fs.names, "a"); + + const result = await ops.move("a", "b"); + assertSetContainsExactly(fs.names, "b"); + assert.equal(result.actualPath, "b"); }); - it("should continue deconfliction from existing number in filename", async () => { - const fileSystemOperations = new FakeFileSystemOperations(); - const fileOperations = new FileOperations( - new Logger(), - new MockDatabase() as Database, // eslint-disable-line @typescript-eslint/no-unsafe-type-assertion - fileSystemOperations, - new MockServerConfig() as ServerConfig // eslint-disable-line @typescript-eslint/no-unsafe-type-assertion - ); + it("move with same source and target is a no-op", async () => { + const { fs, ops } = makeOps(); - await fileOperations.create("document (5).md", new Uint8Array()); - await fileOperations.create("other.md", new Uint8Array()); + await ops.create("a", new Uint8Array()); + const result = await ops.move("a", "a"); - await fileOperations.move("other.md", "document (5).md"); - assertSetContainsExactly( - fileSystemOperations.names, - "document (5).md", - "document (6).md" - ); - - await fileOperations.create("another.md", new Uint8Array()); - await fileOperations.move("another.md", "document (5).md"); - assertSetContainsExactly( - fileSystemOperations.names, - "document (5).md", - "document (6).md", - "document (7).md" - ); + assertSetContainsExactly(fs.names, "a"); + assert.equal(result.actualPath, "a"); }); - it("should handle dotfiles correctly", async () => { - const fileSystemOperations = new FakeFileSystemOperations(); - const fileOperations = new FileOperations( - new Logger(), - new MockDatabase() as Database, // eslint-disable-line @typescript-eslint/no-unsafe-type-assertion - fileSystemOperations, - new MockServerConfig() as ServerConfig // eslint-disable-line @typescript-eslint/no-unsafe-type-assertion + it("move throws FileAlreadyExistsError when the target is occupied", async () => { + const { fs, ops } = makeOps(); + + await ops.create("source.md", new Uint8Array()); + await ops.create("dest.md", new Uint8Array()); + + await assert.rejects( + ops.move("source.md", "dest.md"), + FileAlreadyExistsError ); - await fileOperations.create(".gitignore", new Uint8Array()); - await fileOperations.create("temp", new Uint8Array()); - await fileOperations.move("temp", ".gitignore"); - assertSetContainsExactly( - fileSystemOperations.names, - ".gitignore", - ".gitignore (1)" - ); + // Both files are left intact — no displacement happens. + assertSetContainsExactly(fs.names, "source.md", "dest.md"); + }); - await fileOperations.create(".config.json", new Uint8Array()); - await fileOperations.create("temp2", new Uint8Array()); - await fileOperations.move("temp2", ".config.json"); - assertSetContainsExactly( - fileSystemOperations.names, - ".gitignore", - ".gitignore (1)", - ".config.json", - ".config (1).json" - ); + it("create works for nested paths (parent-directory creation)", async () => { + const { fs, ops } = makeOps(); + + await ops.create("a/b.c/d", new Uint8Array()); + assertSetContainsExactly(fs.names, "a/b.c/d"); + }); + + it("move works for nested target paths (parent-directory creation)", async () => { + const { fs, ops } = makeOps(); + + await ops.create("source", new Uint8Array()); + await ops.move("source", "a/b.c/dest"); + + assertSetContainsExactly(fs.names, "a/b.c/dest"); }); }); diff --git a/frontend/sync-client/src/file-operations/file-operations.ts b/frontend/sync-client/src/file-operations/file-operations.ts index 2864bd20..17a2c655 100644 --- a/frontend/sync-client/src/file-operations/file-operations.ts +++ b/frontend/sync-client/src/file-operations/file-operations.ts @@ -1,28 +1,40 @@ import type { Logger } from "../tracing/logger"; import type { FileSystemOperations } from "./filesystem-operations"; -import type { Database, RelativePath } from "../persistence/database"; +import type { RelativePath } from "../sync-operations/types"; import { SafeFileSystemOperations } from "./safe-filesystem-operations"; import type { TextWithCursors } from "reconcile-text"; import { reconcile } from "reconcile-text"; import { isFileTypeMergable } from "../utils/is-file-type-mergable"; import { isBinary } from "../utils/is-binary"; import type { ServerConfig } from "../services/server-config"; +import { FileNotFoundError } from "../errors/file-not-found-error"; +import { FileAlreadyExistsError } from "../errors/file-already-exists-error"; +import type { ExpectedFsEvents } from "../sync-operations/expected-fs-events"; + +/** + * Outcome of a `move`/`create`. `actualPath` is where the file ended up; + * with the conflict-path machinery removed it is always equal to the + * requested path. The shape is preserved so callers don't all need to + * change. + */ +export interface FileOpResult { + actualPath: RelativePath; +} export class FileOperations { - private static readonly PARENTHESES_REGEX = / \((?\d+)\)$/; private readonly fs: SafeFileSystemOperations; public constructor( private readonly logger: Logger, - private readonly database: Database, fs: FileSystemOperations, private readonly serverConfig: ServerConfig, + private readonly expectedFsEvents: ExpectedFsEvents, private readonly nativeLineEndings = "\n" ) { this.fs = new SafeFileSystemOperations(fs, logger); } - private static getParentDirAndFile( + private static getParentDirAndFileName( path: RelativePath ): [RelativePath, RelativePath] { const pathParts = path.split("/"); @@ -45,43 +57,42 @@ export class FileOperations { } /** - * Create a file at the specified path. - * - * If a file with the same name already exists, it is moved before creating the new one. - * Parent directories are created if necessary. - */ + * Create a file at the specified path. + * + * Throws `FileAlreadyExistsError` if a file already lives at `path`. + * Parent directories are created if necessary. The reconciler is the + * only caller that places files now and pre-checks for conflicts; + * the throw guards against a TOCTOU race rather than being a normal + * code path. + */ public async create( path: RelativePath, newContent: Uint8Array - ): Promise { - await this.ensureClearPath(path); - return this.fs.write(path, this.toNativeLineEndings(newContent)); - } - - public async ensureClearPath(path: RelativePath): Promise { + ): Promise { if (await this.fs.exists(path)) { - const deconflictedPath = await this.deconflictPath(path); - try { - this.logger.debug( - `Didn't expect ${path} to exist, deconflicting by moving it to '${deconflictedPath}'` - ); - - this.database.move(path, deconflictedPath); - await this.fs.rename(path, deconflictedPath, true); - } finally { - this.fs.unlock(deconflictedPath); - } - } else { - await this.createParentDirectories(path); + throw new FileAlreadyExistsError( + `Refusing to create '${path}': file already exists`, + path + ); } + await this.createParentDirectories(path); + + this.expectedFsEvents.expectCreate(path); + try { + await this.fs.write(path, this.toNativeLineEndings(newContent)); + } catch (e) { + this.expectedFsEvents.unexpectCreate(path); + throw e; + } + return { actualPath: path }; } /** - * Update the file at the given path. - * - * Performs a 3-way merge before writing if the file's content differs from `expectedContent`. - * Does not recreate the file if it no longer exists, returning an empty array instead. - */ + * Update the file at the given path. + * + * Performs a 3-way merge before writing if the file's content differs from `expectedContent`. + * Does not recreate the file if it no longer exists, returning an empty array instead. + */ public async write( path: RelativePath, expectedContent: Uint8Array, @@ -94,58 +105,96 @@ export class FileOperations { return; } - if ( - !isFileTypeMergable( - path, - (await this.serverConfig.getConfig()).mergeableFileExtensions - ) || - isBinary(expectedContent) || - isBinary(newContent) - ) { - this.logger.debug( - `The expected content is not mergable, so we won't perform a 3-way merge, just overwrite it` - ); - await this.fs.write( - path, - // `newContent` might not be binary so we still have to ensure the line endings are correct - this.toNativeLineEndings(newContent) - ); - return; - } - - const expectedText = new TextDecoder().decode(expectedContent); // this comes from a previous read which must only have \n line endings - const newText = new TextDecoder().decode(newContent); // this comes from the server which stores text with \n line endings - - await this.fs.atomicUpdateText( - path, - ({ text, cursors }: TextWithCursors): TextWithCursors => { + // Single-source the expectation registration: register exactly once + // per call, and unexpect from the catch if the underlying fs op + // throws (FileNotFoundError or otherwise). The previous shape + // registered inside each branch and let the catch swallow + // FileNotFoundError, leaking the expectation into the map. + this.expectedFsEvents.expectUpdate(path); + try { + if ( + !isFileTypeMergable( + path, + (await this.serverConfig.getConfig()) + .mergeableFileExtensions + ) || + isBinary(expectedContent) || + isBinary(newContent) + ) { this.logger.debug( - `Performing a 3-way merge for ${path} with the expected content` + `The expected content is not mergable, so we won't perform a 3-way merge, just overwrite it` ); - - text = text.replaceAll(this.nativeLineEndings, "\n"); - const merged = reconcile( - expectedText, - { text, cursors }, - newText + await this.fs.write( + path, + // `newContent` might not be binary so we still have to ensure the line endings are correct + this.toNativeLineEndings(newContent) ); - - const resultText = merged.text.replaceAll( - "\n", - this.nativeLineEndings - ); - - return { - text: resultText, - cursors: merged.cursors - }; + return; } - ); + + let expectedText = ""; + let newText = ""; + try { + expectedText = new TextDecoder("utf-8", { fatal: true }).decode( + expectedContent + ); // this comes from a previous read which must only have \n line endings + newText = new TextDecoder("utf-8", { fatal: true }).decode( + newContent + ); // this comes from the server which stores text with \n line endings + } catch (decodeError) { + this.logger.warn( + `3-way merge aborted for ${path}: one of expected/new is not valid UTF-8 (${decodeError}); falling back to overwrite` + ); + await this.fs.write(path, this.toNativeLineEndings(newContent)); + return; + } + + await this.fs.atomicUpdateText( + path, + ({ text, cursors }: TextWithCursors): TextWithCursors => { + this.logger.debug( + `Performing a 3-way merge for ${path} with the expected content` + ); + + text = text.replaceAll(this.nativeLineEndings, "\n"); + const merged = reconcile( + expectedText, + { text, cursors }, + newText + ); + + const resultText = merged.text.replaceAll( + "\n", + this.nativeLineEndings + ); + + return { + text: resultText, + cursors: merged.cursors + }; + } + ); + } catch (e) { + this.expectedFsEvents.unexpectUpdate(path); + if (e instanceof FileNotFoundError) { + this.logger.debug( + `File ${path} disappeared during write; not recreating` + ); + return; + } + throw e; + } } public async delete(path: RelativePath): Promise { if (await this.exists(path)) { - await this.fs.delete(path); + this.expectedFsEvents.expectDelete(path); + try { + await this.fs.delete(path); + } catch (e) { + this.expectedFsEvents.unexpectDelete(path); + throw e; + } await this.deletingEmptyParentDirectoriesOfDeletedFile(path); } else { this.logger.debug(`No need to delete '${path}', it doesn't exist`); @@ -160,23 +209,39 @@ export class FileOperations { return this.fs.exists(path); } + /** + * Move the file at `oldPath` to `newPath`. + * + * Throws `FileAlreadyExistsError` if a file already lives at `newPath` + * (and `oldPath !== newPath`). The reconciler is the only caller that + * relocates tracked records and pre-checks for conflicts; the throw + * guards against a TOCTOU race. + */ public async move( oldPath: RelativePath, newPath: RelativePath - ): Promise { + ): Promise { if (oldPath === newPath) { - return; + return { actualPath: oldPath }; } - await this.ensureClearPath(newPath); + if (await this.fs.exists(newPath)) { + throw new FileAlreadyExistsError( + `Refusing to move '${oldPath}' onto '${newPath}': target already exists`, + newPath + ); + } + await this.createParentDirectories(newPath); - this.database.move(oldPath, newPath); - await this.fs.rename(oldPath, newPath); + this.expectedFsEvents.expectRename(oldPath, newPath); + try { + await this.fs.rename(oldPath, newPath); + } catch (e) { + this.expectedFsEvents.unexpectRename(oldPath, newPath); + throw e; + } await this.deletingEmptyParentDirectoriesOfDeletedFile(oldPath); - } - - public reset(): void { - this.fs.reset(); + return { actualPath: newPath }; } private async deletingEmptyParentDirectoriesOfDeletedFile( @@ -185,7 +250,7 @@ export class FileOperations { let directory = path; // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition while (true) { - [directory] = FileOperations.getParentDirAndFile(directory); + [directory] = FileOperations.getParentDirAndFileName(directory); if (directory.length === 0) { break; } @@ -237,55 +302,4 @@ export class FileOperations { } } } - - /** - * Deconflicts the given path by appending (1), (2), etc. before the file extension until a non-existent path is found. - * The returned path has a lock acquired on it; it must be released by the caller when no longer needed. - * - * @param path The starting path to deconflict - * @returns a non-existent path with a lock acquired on it - */ - private async deconflictPath(path: RelativePath): Promise { - // eslint-disable-next-line prefer-const - let [directory, fileName] = FileOperations.getParentDirAndFile(path); - - if (directory) { - directory += "/"; - } - - const nameParts = fileName.split("."); - // Handle dotfiles: ".gitignore" should have no extension, ".config.json" should have ".json" - const isDotfile = fileName.startsWith(".") && nameParts[0] === ""; - const extension = - nameParts.length > 1 && !(isDotfile && nameParts.length === 2) - ? "." + nameParts[nameParts.length - 1] - : ""; - let stem = extension ? nameParts.slice(0, -1).join(".") : fileName; - let currentCount = Number.parseInt( - FileOperations.PARENTHESES_REGEX.exec(stem)?.groups?.count ?? "0" - ); - stem = stem.replace(FileOperations.PARENTHESES_REGEX, ""); - - let newName = path; - - // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition - while (true) { - currentCount++; - newName = `${directory}${stem} (${currentCount})${extension}`; - - // Avoid multiple deconflictPath calls returning the same path - if (this.fs.tryLock(newName)) { - const newDocument = - this.database.getLatestDocumentByRelativePath(newName); - if ( - newDocument?.isDeleted === false || // the document might have been confirmed by the server at a new path but haven't yet moved there locally - (await this.fs.exists(newName, true)) - ) { - this.fs.unlock(newName); - } else { - return newName; - } - } - } - } } diff --git a/frontend/sync-client/src/file-operations/filesystem-operations.ts b/frontend/sync-client/src/file-operations/filesystem-operations.ts index 36dddfe6..a5fb006b 100644 --- a/frontend/sync-client/src/file-operations/filesystem-operations.ts +++ b/frontend/sync-client/src/file-operations/filesystem-operations.ts @@ -1,4 +1,4 @@ -import type { RelativePath } from "../persistence/database"; +import type { RelativePath } from "../sync-operations/types"; import type { TextWithCursors } from "reconcile-text"; diff --git a/frontend/sync-client/src/file-operations/safe-filesystem-operations.ts b/frontend/sync-client/src/file-operations/safe-filesystem-operations.ts index 904bf805..89b5008c 100644 --- a/frontend/sync-client/src/file-operations/safe-filesystem-operations.ts +++ b/frontend/sync-client/src/file-operations/safe-filesystem-operations.ts @@ -1,24 +1,18 @@ -import type { RelativePath } from "../persistence/database"; +import type { RelativePath } from "../sync-operations/types"; import type { FileSystemOperations } from "./filesystem-operations"; import type { Logger } from "../tracing/logger"; -import { Locks } from "../utils/data-structures/locks"; -import { FileNotFoundError } from "./file-not-found-error"; +import { FileNotFoundError } from "../errors/file-not-found-error"; import type { TextWithCursors } from "reconcile-text"; /** * Decorates `FileSystemOperations` to replace errors with `FileNotFoundError` - * if the accessed file doesn't exist. It also ensures that there's at most a - * single request in-flight for any one file through the use of locks. + * if the accessed file doesn't exist. */ export class SafeFileSystemOperations implements FileSystemOperations { - private readonly locks: Locks; - public constructor( private readonly fs: FileSystemOperations, private readonly logger: Logger - ) { - this.locks = new Locks(logger); - } + ) {} public async listFilesRecursively( root: RelativePath | undefined @@ -31,19 +25,12 @@ export class SafeFileSystemOperations implements FileSystemOperations { public async read(path: RelativePath): Promise { this.logger.debug(`Reading file '${path}'`); - return this.safeOperation( - path, - async () => - this.locks.withLock(path, async () => this.fs.read(path)), - "read" - ); + return this.safeOperation(path, async () => this.fs.read(path), "read"); } public async write(path: RelativePath, content: Uint8Array): Promise { this.logger.debug(`Writing to file '${path}'`); - return this.locks.withLock(path, async () => - this.fs.write(path, content) - ); + return this.fs.write(path, content); } public async atomicUpdateText( @@ -53,10 +40,7 @@ export class SafeFileSystemOperations implements FileSystemOperations { this.logger.debug(`Atomically updating file '${path}'`); return this.safeOperation( path, - async () => - this.locks.withLock(path, async () => - this.fs.atomicUpdateText(path, updater) - ), + async () => this.fs.atomicUpdateText(path, updater), "atomicUpdateText" ); } @@ -65,80 +49,43 @@ export class SafeFileSystemOperations implements FileSystemOperations { // Logging this would be too noisy return this.safeOperation( path, - async () => - this.locks.withLock(path, async () => - this.fs.getFileSize(path) - ), + async () => this.fs.getFileSize(path), "getFileSize" ); } - public async exists( - path: RelativePath, - skipLock = false - ): Promise { + public async exists(path: RelativePath): Promise { this.logger.debug(`Checking if file '${path}' exists`); - if (skipLock) { - return this.fs.exists(path); - } else { - return this.locks.withLock(path, async () => this.fs.exists(path)); - } + return this.fs.exists(path); } public async createDirectory(path: RelativePath): Promise { this.logger.debug(`Creating directory '${path}'`); - return this.locks.withLock(path, async () => - this.fs.createDirectory(path) - ); + return this.fs.createDirectory(path); } public async delete(path: RelativePath): Promise { this.logger.debug(`Deleting file '${path}'`); - return this.locks.withLock(path, async () => this.fs.delete(path)); + return this.fs.delete(path); } public async rename( oldPath: RelativePath, - newPath: RelativePath, - skipLock = false + newPath: RelativePath ): Promise { this.logger.debug(`Renaming file '${oldPath}' to '${newPath}'`); return this.safeOperation( oldPath, - async () => { - if (skipLock) { - return this.fs.rename(oldPath, newPath); - } else { - return this.locks.withLock([oldPath, newPath], async () => - this.fs.rename(oldPath, newPath) - ); - } - }, + async () => this.fs.rename(oldPath, newPath), "rename" ); } - public tryLock(path: RelativePath): boolean { - return this.locks.tryLock(path); - } - - public async waitForLock(path: RelativePath): Promise { - return this.locks.waitForLock(path); - } - - public unlock(path: RelativePath): void { - this.locks.unlock(path); - } - - public reset(): void { - this.locks.reset(); - } - /** - * Decorate an operation to ensure that the file exists before running it. - * If the operation fails, it will check if the file still exists and throw - * a FileNotFoundError if it doesn't. - */ + * Decorate an operation to ensure that the file exists before running it. + * If the operation fails, it will check if the file still exists and throw + * a FileNotFoundError if it doesn't. + */ private async safeOperation( path: RelativePath, operation: () => Promise, @@ -154,9 +101,6 @@ export class SafeFileSystemOperations implements FileSystemOperations { try { return await operation(); } catch (error) { - // Without locking the file, this isn't atomic, however, it's good enough in practice. - // This will only break if the file exists, gets deleted and then immediately - // recreated while `operation` is running. if (await this.fs.exists(path)) { throw error; } else { diff --git a/frontend/sync-client/src/persistence/database.ts b/frontend/sync-client/src/persistence/database.ts deleted file mode 100644 index 86b2845c..00000000 --- a/frontend/sync-client/src/persistence/database.ts +++ /dev/null @@ -1,374 +0,0 @@ -import type { Logger } from "../tracing/logger"; -import { EMPTY_HASH } from "../utils/hash"; -import { CoveredValues } from "../utils/data-structures/min-covered"; -import { awaitAll } from "../utils/await-all"; -import { removeFromArray } from "../utils/remove-from-array"; - -export type VaultUpdateId = number; -export type DocumentId = string; -export type RelativePath = string; - -export interface DocumentMetadata { - parentVersionId: VaultUpdateId; - hash: string; - remoteRelativePath?: RelativePath; -} - -export interface StoredDocumentMetadata { - relativePath: RelativePath; - documentId: DocumentId; - parentVersionId: VaultUpdateId; - remoteRelativePath?: RelativePath; - hash: string; -} - -export interface StoredDatabase { - documents: StoredDocumentMetadata[]; - lastSeenUpdateId: VaultUpdateId | undefined; - hasInitialSyncCompleted: boolean; -} - -/** - * Represents a document in the database. - * - * It is mutable and its content should always represent the latest - * state of the document on disk based on the update events we have seen. - */ -export interface DocumentRecord { - relativePath: RelativePath; - documentId: DocumentId; - metadata: DocumentMetadata | undefined; - isDeleted: boolean; - updates: Promise[]; - parallelVersion: number; -} - -export class Database { - private documents: DocumentRecord[]; - private lastSeenUpdateIds: CoveredValues; - private hasInitialSyncCompleted: boolean; - - public constructor( - private readonly logger: Logger, - initialState: Partial | undefined, - private readonly saveData: (data: StoredDatabase) => Promise - ) { - initialState ??= {}; - - this.documents = - initialState.documents?.map( - ({ relativePath, documentId, ...metadata }) => ({ - relativePath, - documentId, - metadata, - isDeleted: false, - updates: [], - parallelVersion: 0 - }) - ) ?? []; - - this.ensureConsistency(); - this.logger.debug(`Loaded ${this.documents.length} documents`); - - const { lastSeenUpdateId } = initialState; - this.logger.debug(`Loaded last seen update id: ${lastSeenUpdateId}`); - this.lastSeenUpdateIds = new CoveredValues( - Math.max(0, lastSeenUpdateId ?? 0) // the first updateId will be 1 which is the first integer after -1 - ); - - this.documents.forEach((doc) => { - this.lastSeenUpdateIds.add(doc.metadata?.parentVersionId); - }); - - this.hasInitialSyncCompleted = - initialState.hasInitialSyncCompleted ?? false; - this.logger.debug( - `Loaded hasInitialSyncCompleted: ${this.hasInitialSyncCompleted}` - ); - } - - public get length(): number { - return this.documents.length; - } - - public get resolvedDocuments(): DocumentRecord[] { - const paths = new Map(); - this.documents - // eslint-disable-next-line no-restricted-syntax -- Type narrowing, not removing a specific item - .filter(({ metadata }) => metadata !== undefined) - .forEach((record) => - paths.set(record.relativePath, [ - record, - ...(paths.get(record.relativePath) ?? []) - ]) - ); - - return Array.from(paths.values()).map((records) => { - records.sort( - (a, b) => b.parallelVersion - a.parallelVersion // descending - ); - - if ( - records.length > 1 && - records.some((current, i) => - i === 0 - ? false - : records[i - 1].parallelVersion === - current.parallelVersion - ) - ) { - throw new Error( - `Multiple documents with the same parallel version and path at ${records[0].relativePath}` - ); - } - return records[0]; - }); - } - - public updateDocumentMetadata( - metadata: { - parentVersionId: VaultUpdateId; - hash: string; - remoteRelativePath: RelativePath; - }, - toUpdate: DocumentRecord - ): void { - if (!this.documents.includes(toUpdate)) { - throw new Error("Document not found in database"); - } - - toUpdate.metadata = metadata; - - this.saveInTheBackground(); - } - - public removeDocumentPromise(promise: Promise): void { - const entry = this.documents.find(({ updates }) => - updates.includes(promise) - ); - - if (entry === undefined) { - // This method should be idempotent and tolerant of - // stragglers calling it after the databse has been reset. - return; - } - - removeFromArray(entry.updates, promise); - // No need to save as Promises don't get serialized - } - - public removeDocument(find: DocumentRecord): void { - removeFromArray(this.documents, find); - this.saveInTheBackground(); - } - - public getLatestDocumentByRelativePath( - find: RelativePath - ): DocumentRecord | undefined { - const candidates = this.documents.filter( - ({ relativePath }) => relativePath === find - ); - candidates.sort((a, b) => b.parallelVersion - a.parallelVersion); // descending - return candidates[0]; - } - - public async getResolvedDocumentByRelativePath( - relativePath: RelativePath, - promise: Promise - ): Promise { - const entry = this.getLatestDocumentByRelativePath(relativePath); - - if (entry === undefined) { - throw new Error( - `Document not found by relative path: ${relativePath}, ${JSON.stringify( - this.documents, - null, - 2 - )}` - ); - } - - const currentPromises = entry.updates; - entry.updates = [...currentPromises, promise]; - await awaitAll(currentPromises); - - return entry; - } - - public createNewPendingDocument( - documentId: DocumentId, - relativePath: RelativePath, - promise: Promise - ): DocumentRecord { - this.logger.debug( - `Creating new pending document: ${relativePath} (${documentId})` - ); - const previousEntry = - this.getLatestDocumentByRelativePath(relativePath); - - const entry = { - relativePath, - documentId, - metadata: undefined, - isDeleted: false, - updates: [promise], - parallelVersion: - previousEntry?.parallelVersion === undefined - ? 0 - : previousEntry.parallelVersion + 1 - }; - - this.documents.push(entry); - this.saveInTheBackground(); - - return entry; - } - - public createNewEmptyDocument( - documentId: DocumentId, - parentVersionId: VaultUpdateId, - relativePath: RelativePath - ): DocumentRecord { - const entry = { - relativePath, - documentId, - metadata: { - parentVersionId, - hash: EMPTY_HASH, - remoteRelativePath: relativePath - }, - isDeleted: false, - updates: [], - parallelVersion: 0 - }; - - this.documents.push(entry); - this.saveInTheBackground(); - - return entry; - } - - public getDocumentByDocumentId( - find: DocumentId - ): DocumentRecord | undefined { - return this.documents.find(({ documentId }) => documentId === find); - } - - public move( - oldRelativePath: RelativePath, - newRelativePath: RelativePath - ): void { - const oldDocument = - this.getLatestDocumentByRelativePath(oldRelativePath); - - if (oldDocument === undefined) { - return; - } - - const newDocument = - this.getLatestDocumentByRelativePath(newRelativePath); - if (newDocument?.isDeleted === false) { - throw new Error( - `Document already exists at new location: ${newRelativePath}` - ); - } - - oldDocument.relativePath = newRelativePath; - // We're in a strange state where the target of the move has just got deleted, - // however, its metadata might already have a bunch of updates queued up for - // the document at the new location. We need to keep these updates. - oldDocument.parallelVersion = - newDocument !== undefined ? newDocument.parallelVersion + 1 : 0; - - this.saveInTheBackground(); - } - - public delete(relativePath: RelativePath): void { - const candidate = this.getLatestDocumentByRelativePath(relativePath); - if (candidate === undefined) { - throw new Error( - `Document not found by relative path: ${relativePath}` - ); - } - candidate.isDeleted = true; - } - - public getHasInitialSyncCompleted(): boolean { - return this.hasInitialSyncCompleted; - } - - public setHasInitialSyncCompleted(value: boolean): void { - this.hasInitialSyncCompleted = value; - this.saveInTheBackground(); - } - - public getLastSeenUpdateId(): VaultUpdateId { - return this.lastSeenUpdateIds.min; - } - - public addSeenUpdateId(value: number): void { - const previousMin = this.lastSeenUpdateIds.min; - this.lastSeenUpdateIds.add(value); - if (previousMin !== this.lastSeenUpdateIds.min) { - this.saveInTheBackground(); - } - } - - public setLastSeenUpdateId(value: number): void { - this.lastSeenUpdateIds.min = value; - this.saveInTheBackground(); - } - - public reset(): void { - this.documents = []; - this.lastSeenUpdateIds = new CoveredValues( - 0 // the first updateId will be 1 which is the first integer after -1 - ); - this.hasInitialSyncCompleted = false; - this.saveInTheBackground(); - } - - public async save(): Promise { - return this.saveData({ - documents: this.resolvedDocuments.map( - ({ relativePath, documentId, metadata }) => ({ - documentId, - relativePath, - // eslint-disable-next-line @typescript-eslint/no-non-null-assertion - ...metadata! // `resolvedDocuments` only returns docs with metadata set - }) - ), - lastSeenUpdateId: this.lastSeenUpdateIds.min, - hasInitialSyncCompleted: this.hasInitialSyncCompleted - }); - } - - private ensureConsistency(): void { - const idToPath = new Map(); - - this.resolvedDocuments.forEach(({ relativePath, documentId }) => { - idToPath.set(documentId, [ - ...(idToPath.get(documentId) ?? []), - relativePath - ]); - }); - - const duplicates = Array.from(idToPath.entries()) - .filter(([_, paths]) => paths.length > 1) - .map(([id, paths]) => `${id} (${paths.join(", ")})`); - - if (duplicates.length > 0) { - throw new Error( - "Document IDs are not unique, found duplicates: " + - duplicates.join("; ") - ); - } - } - - private saveInTheBackground(): void { - this.ensureConsistency(); - void this.save().catch((error: unknown) => { - this.logger.error(`Error saving data: ${error}`); - }); - } -} diff --git a/frontend/sync-client/src/persistence/settings.ts b/frontend/sync-client/src/persistence/settings.ts index d78170e6..c954134f 100644 --- a/frontend/sync-client/src/persistence/settings.ts +++ b/frontend/sync-client/src/persistence/settings.ts @@ -6,7 +6,6 @@ export interface SyncSettings { remoteUri: string; token: string; vaultName: string; - syncConcurrency: number; isSyncEnabled: boolean; maxFileSizeMB: number; ignorePatterns: string[]; @@ -14,22 +13,19 @@ export interface SyncSettings { diffCacheSizeMB: number; enableTelemetry: boolean; networkRetryIntervalMs: number; - minimumSaveIntervalMs: number; } export const DEFAULT_SETTINGS: SyncSettings = { remoteUri: "", token: "", vaultName: "default", - syncConcurrency: 1, isSyncEnabled: false, maxFileSizeMB: 10, ignorePatterns: [], webSocketRetryIntervalMs: 3500, diffCacheSizeMB: 4, enableTelemetry: false, - networkRetryIntervalMs: 1000, - minimumSaveIntervalMs: 1000 + networkRetryIntervalMs: 1000 }; export class Settings { @@ -38,7 +34,7 @@ export class Settings { >(); private settings: SyncSettings; - private readonly lock: Lock = new Lock(); + private readonly lock: Lock; public constructor( private readonly logger: Logger, @@ -50,6 +46,8 @@ export class Settings { ...(initialState ?? {}) }; + this.lock = new Lock(Settings.name, this.logger); + this.logger.debug( `Loaded settings: ${JSON.stringify(this.settings, null, 2)}` );