From d5bdbbe3042de4d872592a66dbcacc14c609eaa0 Mon Sep 17 00:00:00 2001 From: Andras Schmelczer Date: Tue, 29 Sep 2026 21:28:17 +0100 Subject: [PATCH] Clean up file rename --- .../src/file-operations/file-renamer.test.ts | 58 +++++++++++++ .../{file-paths.ts => file-renamer.ts} | 85 ++++++++++--------- .../sync-client/src/types/client-phase.ts | 5 ++ 3 files changed, 108 insertions(+), 40 deletions(-) create mode 100644 frontend/sync-client/src/file-operations/file-renamer.test.ts rename frontend/sync-client/src/file-operations/{file-paths.ts => file-renamer.ts} (63%) create mode 100644 frontend/sync-client/src/types/client-phase.ts diff --git a/frontend/sync-client/src/file-operations/file-renamer.test.ts b/frontend/sync-client/src/file-operations/file-renamer.test.ts new file mode 100644 index 00000000..aaea70ce --- /dev/null +++ b/frontend/sync-client/src/file-operations/file-renamer.test.ts @@ -0,0 +1,58 @@ +/* eslint-disable @typescript-eslint/no-unsafe-type-assertion */ +import assert from "node:assert/strict"; +import { test } from "node:test"; +import { MemoryDisk } from "../../../test-support/storage"; +import { Database, createEmptyDatabase } from "../persistence/database"; +import type { ServerConfig } from "../services/server-config"; +import { LocalChangeCheckpoint } from "../sync-operations/local-change-checkpoint"; +import { hash } from "../utils/hash"; +import { FileOperations } from "./file-operations"; + +const bytes = (text: string): Uint8Array => new TextEncoder().encode(text); + +test("a path swap uses a conflict path only as a temporary destination", async () => { + const disk = new MemoryDisk(); + await disk.userWrite("a.md", bytes("A")); + await disk.userWrite("b.md", bytes("B")); + const initial = createEmptyDatabase("test"); + initial.actualFileManifest = { a: "a.md", b: "b.md" }; + initial.documents = { + a: { observedHash: await hash(bytes("A")) }, + b: { observedHash: await hash(bytes("B")) } + }; + const database = new Database( + initial, + "test", + async () => undefined, + async () => undefined + ); + const files = new FileOperations(disk, database, {} as ServerConfig); + const checkpoint = new LocalChangeCheckpoint({ generation: 0 }); + const renames: string[] = []; + disk.boundary = (label): void => { + if (label.startsWith("visible:rename:")) { + renames.push(label.slice("visible:rename:".length)); + } + }; + + const next = structuredClone(database.state); + next.actualFileManifest = { a: "b.md", b: "a.md" }; + await files.applyChanges(next, checkpoint); + + assert.deepEqual(renames, [ + "b.md->b (conflict b).md", // Free B without losing its contents. + "a.md->b.md", // Move A into B. + "b (conflict b).md->a.md" // Finish moving B into A. + ]); + assert.deepEqual(database.state.actualFileManifest, { + a: "b.md", + b: "a.md" + }); + assert.deepEqual( + disk.userFiles(), + new Map([ + ["a.md", bytes("B")], + ["b.md", bytes("A")] + ]) + ); +}); diff --git a/frontend/sync-client/src/file-operations/file-paths.ts b/frontend/sync-client/src/file-operations/file-renamer.ts similarity index 63% rename from frontend/sync-client/src/file-operations/file-paths.ts rename to frontend/sync-client/src/file-operations/file-renamer.ts index 35b5d72e..f16545cd 100644 --- a/frontend/sync-client/src/file-operations/file-paths.ts +++ b/frontend/sync-client/src/file-operations/file-renamer.ts @@ -1,7 +1,7 @@ import { FileKind } from "./filesystem-operations"; import { v4 as uuid } from "uuid"; import type { FileSystemOperations } from "./filesystem-operations"; -import type { FileApplication } from "./file-application"; +import type { FileChangeSession } from "./file-change-session"; import { posix } from "path"; import type { DocumentId, @@ -10,7 +10,7 @@ import type { } from "../persistence/database"; import { allocatePortablePath, isInternalPath } from "../utils/portable-path"; -interface PathObstruction { +interface PathCollision { path: RelativePath; kind: FileKind; } @@ -20,48 +20,53 @@ interface Occupant { path: RelativePath; } -// Resolve occupied rename destinations without discarding another file. -export class FilePaths { +/** + * A collision-free target manifest can still collide with files on disk: rename swaps + * and cycles leave old paths occupied, and files or directories can block a new + * destination. FileRenamer renames local files while dealing with temporary conflicts. + */ +export class FileRenamer { public constructor( private readonly fs: FileSystemOperations, private readonly isProtectedFile: ( path: RelativePath, size: number ) => boolean - ) {} + ) { } public async prepareDestination( - application: FileApplication, + session: FileChangeSession, id: DocumentId, path: RelativePath ): Promise { - const { current, next } = application; - for (;;) { - const obstruction = await this.findObstruction(path); - if (!obstruction) { + const { current, planned } = session; + for (; ;) { + const collision = await this.findCollision(path); + if (!collision) { break; } const occupants = await this.findMovableOccupants( - obstruction, + collision, current, - next + planned ); if (occupants === undefined) { - path = await this.allocateConflictPath(next, id, path); + // path must be occupied by an ignored file which we can't move + path = await this.allocateConflictPath(planned, id, path); continue; } - await this.relocateOccupants(occupants, application); + await this.relocateOccupants(occupants, session); - if (obstruction.kind === FileKind.Directory) { - application.beginFileChanges(); - await this.fs.deleteDirectory(obstruction.path); + if (collision.kind === FileKind.Directory) { + session.beforeFileMutation(); + await this.fs.deleteDirectory(collision.path); } } - application.assertPlanCurrent(); + session.abortIfStale(); await this.createParentDirectories(path); return path; @@ -85,9 +90,9 @@ export class FilePaths { } } - private async findObstruction( + private async findCollision( path: RelativePath - ): Promise { + ): Promise { const parts = path.split("/"); for (let i = 0; i < parts.length; i++) { const prefix = parts.slice(0, i + 1).join("/"); @@ -103,29 +108,29 @@ export class FilePaths { } private async findMovableOccupants( - obstruction: PathObstruction, + collision: PathCollision, current: StoredDatabase, - next: StoredDatabase + planned: StoredDatabase ): Promise { const children = - obstruction.kind === FileKind.File - ? [obstruction.path] - : await this.fs.listFilesRecursively(obstruction.path); + collision.kind === FileKind.File + ? [collision.path] + : await this.fs.listFilesRecursively(collision.path); const occupants = []; for (const child of children) { - const occupant = Object.keys(current.local).find( + const occupant = Object.keys(current.actualFileManifest).find( (key) => current.documents[key]?.observedHash !== undefined && - current.local[key] === child + current.actualFileManifest[key] === child ); const info = await this.fs.stat(child); if ( !info || // doesn't exist anymore this.isProtectedFile(child, info.size) || - (obstruction.kind === FileKind.Directory && - (occupant === undefined || next.local[occupant] === child)) + (collision.kind === FileKind.Directory && + (occupant === undefined || planned.actualFileManifest[occupant] === child)) ) { return undefined; } @@ -137,11 +142,11 @@ export class FilePaths { } private async allocateConflictPath( - next: StoredDatabase, + planned: StoredDatabase, id: DocumentId, path: RelativePath ): Promise { - const occupied = Object.entries(next.local).flatMap( + const occupied = Object.entries(planned.actualFileManifest).flatMap( ([other, occupiedPath]) => (other === id ? [] : [occupiedPath]) ); @@ -154,26 +159,26 @@ export class FilePaths { private async relocateOccupants( occupants: Occupant[], - application: FileApplication + session: FileChangeSession ): Promise { - const { current, next } = application; + const { current, planned } = session; for (const occupant of occupants) { const displaced = await this.allocateConflictPath( - next, + planned, occupant.id, posix.basename(occupant.path) ); - application.beginFileChanges(); - current.local[occupant.id] = displaced; + session.beforeFileMutation(); + current.actualFileManifest[occupant.id] = displaced; await this.fs.rename(occupant.path, displaced); if (!current.documents[occupant.id]) { current.documents[occupant.id] = { observedHash: "" }; - next.documents[occupant.id] = { observedHash: "" }; - next.local[occupant.id] = displaced; - } else if (next.local[occupant.id] === occupant.path) { - next.local[occupant.id] = displaced; + planned.documents[occupant.id] = { observedHash: "" }; + planned.actualFileManifest[occupant.id] = displaced; + } else if (planned.actualFileManifest[occupant.id] === occupant.path) { + planned.actualFileManifest[occupant.id] = displaced; } } } diff --git a/frontend/sync-client/src/types/client-phase.ts b/frontend/sync-client/src/types/client-phase.ts new file mode 100644 index 00000000..e555f8a9 --- /dev/null +++ b/frontend/sync-client/src/types/client-phase.ts @@ -0,0 +1,5 @@ +export enum ClientPhase { + Created = "created", + Started = "started", + Destroyed = "destroyed" +}