andras/vault-link
1
0
Fork 0
Code Issues Pull requests 18 Projects Releases 55 Packages Wiki Activity Actions 1

Revie ai fixes

Browse source
This commit is contained in:
Andras Schmelczer 2026-04-26 12:29:02 +01:00
parent fe2b4751bd
commit 8eae770621
12 changed files with 287 additions and 121 deletions
Download patch file Download diff file Expand all files Collapse all files

7
frontend/deterministic-tests/src/tests/17-create-merge-preserves-renamed-update.test.ts
View file

@ -15,6 +15,13 @@ export const createMergePreservesRenamedUpdateTest: TestDefinition = {
{ type: "enable-sync", client: 1 },
{ type: "barrier" },
{
type: "assert-consistent",
verify: (state: AssertableState): void => {
state.assertContains("doc.md", "alpha", "beta");
}
},
{ type: "disable-sync", client: 1 },
{

2
frontend/deterministic-tests/src/tests/7-concurrent-rename-and-create-at-target.test.ts
View file

@ -13,6 +13,8 @@ export const concurrentRenameAndCreateAtTargetTest: TestDefinition = {
path: "X.md",
content: "original file X"
},
{ type: "enable-sync", client: 0 },
{ type: "enable-sync", client: 1 },
{ type: "barrier" },
{ type: "disable-sync", client: 0 },

5
frontend/deterministic-tests/src/tests/online-create-update-while-other-creates-same-path.test.ts
View file

@ -39,8 +39,9 @@ export const onlineCreateUpdateWhileOtherCreatesSamePathTest: TestDefinition = {
verify: (state: AssertableState): void => {
state
.assertFileCount(2)
.assertContains("data.bin", "content-v2")
.assertContains("data (1).bin", "other-content");
.assertNoFileContains("content-v1")
.assertAnyFileContains("content-v2")
.assertAnyFileContains("other-content");
}
}
]

6
frontend/deterministic-tests/src/tests/reset-clears-recently-deleted-resurrection.test.ts
View file

@ -16,13 +16,9 @@ export const resetClearsRecentlyDeletedResurrectionTest: TestDefinition = {
},
{ type: "enable-sync", client: 0 },
{ type: "enable-sync", client: 1 },
{ type: "sync" },
{ type: "barrier" },
{ type: "delete", client: 0, path: "ghost.md" },
{ type: "sync", client: 0 },
{ type: "sync" },
{ type: "barrier" },
{
@ -34,7 +30,7 @@ export const resetClearsRecentlyDeletedResurrectionTest: TestDefinition = {
{ type: "disable-sync", client: 1 },
{ type: "enable-sync", client: 1 },
{ type: "sync" },
{ type: "barrier" },
{

2
frontend/deterministic-tests/src/tests/server-pause-update-and-create.test.ts
View file

@ -14,7 +14,6 @@ export const serverPauseUpdateAndCreateTest: TestDefinition = {
path: "shared.md",
content: "initial content"
},
{ type: "sync" },
{ type: "barrier" },
{
type: "assert-consistent",
@ -40,7 +39,6 @@ export const serverPauseUpdateAndCreateTest: TestDefinition = {
{ type: "resume-server" },
{ type: "sync" },
{ type: "barrier" },
{

20
frontend/deterministic-tests/src/utils/assertable-state.ts
View file

@ -86,6 +86,26 @@ export class AssertableState {
return this;
}
public assertNoFileContains(...substrings: string[]): this {
const offenders: { path: string; substring: string }[] = [];
for (const [path, content] of this.files) {
for (const s of substrings) {
if (content.includes(s)) {
offenders.push({ path, substring: s });
}
}
}
if (offenders.length > 0) {
const dump = Array.from(this.files.entries())
.map(([k, v]) => ` ${k}: "${v}"`)
.join("\n");
throw new Error(
`Expected no file to contain ${substrings.map((s) => `"${s}"`).join(", ")}, but found ${offenders.map((o) => `"${o.substring}" in "${o.path}"`).join(", ")}.\nFiles:\n${dump}`
);
}
return this;
}
public assertSubstringCount(
path: string,
substring: string,

15
frontend/sync-client/src/file-operations/file-operations.ts
View file

@ -29,7 +29,7 @@ export class FileOperations {
this.fs = new SafeFileSystemOperations(fs, logger);
}
private static getParentDirAndFile(
private static getParentDirAndFileName(
path: RelativePath
): [RelativePath, RelativePath] {
const pathParts = path.split("/");
@ -47,7 +47,7 @@ export class FileOperations {
* statistically impossible, so no disk probe / lock dance is needed.
*/
private static buildConflictPath(path: RelativePath): RelativePath {
const [directory, fileName] = FileOperations.getParentDirAndFile(path);
const [directory, fileName] = FileOperations.getParentDirAndFileName(path);
const conflictName = buildConflictFileName(fileName);
return directory ? `${directory}/${conflictName}` : conflictName;
}
@ -202,6 +202,8 @@ export class FileOperations {
return this.fs.exists(path);
}
// Returns the actual path the file got moved to.
public async move(
oldPath: RelativePath,
@ -234,7 +236,12 @@ export class FileOperations {
`Displacing existing file at ${path} to '${conflictPath}' to make room`
);
this.expectedFsEvents.expectRename(path, conflictPath);
// Intentionally NOT calling `expectRename` here: the displaced
// file may be a tracked document (its `queue.documents` entry
// still points at `path`), and we need the watcher's
// `syncLocallyUpdatedFile` to flow into `queue.enqueue`'s
// path-update branch so the doc's map key follows its file
// to `conflictPath` and gets resynced
await this.fs.rename(path, conflictPath);
return path;
}
@ -252,7 +259,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;
}

15
frontend/sync-client/src/sync-client.ts
View file

@ -435,7 +435,18 @@ export class SyncClient {
}
public async waitUntilFinished(): Promise<void> {
this.checkIfDestroyed("waitUntilIdle");
this.checkIfDestroyed("waitUntilFinished");
await this.waitUntilFinishedInternal();
}
/**
* The actual drain — separated from `waitUntilFinished` so internal
* shutdown paths (`pause` / `destroy`) can wait for in-flight work
* without tripping the public `checkIfDestroyed` guard, which exists
* only to keep external callers from continuing to use a disposed
* client.
*/
private async waitUntilFinishedInternal(): Promise<void> {
await this.syncer.waitUntilFinished();
await this.webSocketManager.waitUntilFinished();
await this.syncEventQueue.save();
@ -502,7 +513,7 @@ export class SyncClient {
// the rest of the client is winding down.
this.syncService.stop();
await this.webSocketManager.stop();
await this.waitUntilFinished();
await this.waitUntilFinishedInternal();
// Clear the offline-scan gate so a subsequent `startSyncing()`
// re-runs the scan; otherwise any local changes made while sync was
// paused (offline edits, deletes, renames) wouldn't be detected, and

10
frontend/sync-client/src/sync-operations/offline-change-detector.ts
View file

@ -73,17 +73,25 @@ export async function scheduleOfflineChanges(
for (const path of locallyPossibleCreatedFiles) {
if (renamedPaths.has(path)) continue;
logger.debug(
logger.info(
`File ${path} was created while offline, scheduling sync to create it`
);
enqueueCreate(path);
}
for (const item of locallyPossiblyDeletedFiles) {
logger.info(
`File ${item.path} was deleted while offline, scheduling sync to delete it`
);
enqueueDelete(item.path);
}
for (const path of syncedLocalFiles) {
logger.info(
`File ${path} may have been updated while offline, scheduling sync to update it`
);
enqueueUpdate({ relativePath: path });
}
}

97
frontend/sync-client/src/sync-operations/sync-event-queue.ts
View file

@ -38,6 +38,14 @@ export class SyncEventQueue {
// It maps pending changes onto the local filesystem.
private readonly events: SyncEvent[] = [];
// Tombstones: documents we deleted along with the vaultUpdateId at
// which the delete committed. After we delete, the server may still
// send us older broadcasts for that document (e.g. a backlog update
// committed before the delete from another client). Without these
// entries, the syncer would resurrect the doc by treating an old
// update as a brand-new create.
private readonly deletedDocuments = new Map<DocumentId, VaultUpdateId>();
// file creations for paths matching any of these patterns are ignored
// because the user explicitly told us to ignore them.
private userIgnorePatterns: RegExp[];
@ -121,15 +129,29 @@ export class SyncEventQueue {
return;
}
if (this.ignoreConflictPaths && CONFLICT_PATH_REGEX.test(path)) {
this.logger.info(
`Ignoring ${input.type} for ${path} as it is a conflict path`
);
if (input.type === SyncEventType.RemoteChange) {
this.events.push(input);
return;
}
if (input.type === SyncEventType.RemoteChange) {
this.events.push(input);
// Drop bare LocalCreate events for conflict paths. Those are
// produced by the watcher when the syncer's own write to a
// displacement path slips past the `ExpectedFsEvents` filter
// (e.g. a sync race where the watcher fires before
// `expectCreate` was registered). Re-uploading them as new docs
// would invent duplicates on the server. The legitimate way a
// conflict-path doc enters the queue is via the displacement
// rename's `LocalUpdate` (with `oldPath`) — that branch is
// allowed through below so the tracked document's path follows
// its file.
if (
this.ignoreConflictPaths &&
CONFLICT_PATH_REGEX.test(path) &&
input.type === SyncEventType.LocalCreate
) {
this.logger.info(
`Ignoring local-create for ${path} as it is a conflict path`
);
return;
}
@ -215,6 +237,31 @@ export class SyncEventQueue {
return this.events.shift();
}
/**
* Return the next event without removing it. Drain uses this so the
* event stays visible in the queue while it is being processed —
* critical for `findLatestCreateForPath` to update an in-flight
* `LocalCreate`'s path when a rename arrives mid-process. Also marks
* the event as in-flight so dedup checks in `enqueue` know not to
* fold a fresh content change into an event whose disk read already
* happened.
*/
public peekFront(): SyncEvent | undefined {
return this.events[0];
}
/**
* Remove a specific event after `peekFront`-based processing is done.
* Idempotent — safe to call when the event was already taken out by
* `resolveCreate` (which clears a same-path pending create that a
* remote-create handler just absorbed).
*/
public consumeEvent(event: SyncEvent): void {
removeFromArray(this.events, event);
}
/**
* Call once a create has been acknowledged by the server.
*/
@ -255,6 +302,42 @@ export class SyncEventQueue {
return this.save();
}
/**
* Mark a document as deleted at a given vault-update version. Used by
* the syncer after a successful local or remote delete so future
* obsolete broadcasts for that doc (older parents that arrive late)
* don't resurrect it as a brand-new create.
*/
public recordDeletion(
documentId: DocumentId,
deletedAtVaultUpdateId: VaultUpdateId
): void {
const existing = this.deletedDocuments.get(documentId);
if (existing !== undefined && existing >= deletedAtVaultUpdateId) {
return;
}
this.deletedDocuments.set(documentId, deletedAtVaultUpdateId);
}
/**
* Returns the vault-update version at which we last saw this document
* deleted, or `undefined` if we have no record of its deletion.
*/
public getDeletionVersion(
documentId: DocumentId
): VaultUpdateId | undefined {
return this.deletedDocuments.get(documentId);
}
/**
* Forget a doc's tombstone — used when a doc with the same id is
* re-introduced (e.g. via a remote create whose server-side state
* surpasses the previous delete).
*/
public clearDeletion(documentId: DocumentId): void {
this.deletedDocuments.delete(documentId);
}
public getDocumentByDocumentId(
target: DocumentId
): DocumentWithPath | undefined {
@ -327,6 +410,8 @@ export class SyncEventQueue {
);
}
public async clearAllState(): Promise<void> {
this.clearPending();
this.documents.clear();

202
frontend/sync-client/src/sync-operations/syncer.ts
View file

@ -128,7 +128,7 @@ export class Syncer {
this.runningScheduleSyncForOfflineChanges =
this.internalScheduleSyncForOfflineChanges();
await this.runningScheduleSyncForOfflineChanges;
this.logger.info(`All local changes have been applied remotely`);
this.logger.info(`All local changes have been queued`);
} catch (e) {
if (e instanceof SyncResetError) {
this.logger.info(
@ -192,9 +192,8 @@ export class Syncer {
// queue re-enables conflict filtering when we're done.
this.queue.setIgnoreConflictPaths(false);
try {
while (this.drainPromise !== undefined) {
await this.drainPromise;
}
this.queue.clearPending(); // can't have conflicts between the offline scan and ongoing operations created during the preceeding pause
await scheduleOfflineChanges(
this.logger,
this.operations,
@ -226,8 +225,21 @@ export class Syncer {
}
private async drain(): Promise<void> {
let event = await this.queue.next();
while (event !== undefined) {
// Peek then remove-after-processing (instead of shift-then-process):
// the event must remain reachable through `findLatestCreateForPath`
// while it is in flight, so a rename event arriving mid-process can
// call `updatePendingCreatePath` to retarget this create's path.
while (true) {
if (!this.settings.getSettings().isSyncEnabled) {
this.logger.debug(
"Drain pausing because sync is disabled; events stay queued"
);
return;
}
const event = this.queue.peekFront();
if (event === undefined) { break; }
try {
await this.processEvent(event);
} catch (e) {
@ -239,19 +251,12 @@ export class Syncer {
`Failed to process sync event ${event.type}: ${e}`
);
}
this.queue.consumeEvent(event);
this.notifyRemainingOperationsChanged();
event = await this.queue.next();
}
}
private async processEvent(event: SyncEvent): Promise<void> {
if (!this.settings.getSettings().isSyncEnabled) {
this.logger.info(
`Skipping sync operation because sync is disabled`
);
return;
}
try {
if (await this.skipIfOversized(event)) {
return;
@ -272,22 +277,27 @@ export class Syncer {
break;
}
} catch (e) {
// The currently-processed event was already shifted off the queue
// by drain() before processEvent ran. If it's a LocalCreate, any
// queued Delete/Update events whose `documentId` is this Create's
// resolvers.promise would `await` it forever once we return — so
// settle the resolvers on every failure path before
// dispatching/re-throwing. clearPending()'s rejectAllPendingCreates
// walks the queue and so cannot reach this in-flight event.
// Re-rejecting an already-resolved promise is a no-op, so it's
// safe to call this unconditionally on the LocalCreate branch.
if (event.type === SyncEventType.LocalCreate) {
// If a LocalCreate fails terminally, queued LocalDelete /
// LocalUpdate events whose `documentId` is this Create's
// `resolvers.promise` would `await` it forever — reject the
// resolver so they fail-fast with the same error class and
// hit their matching skip/log branch below.
//
// Only do this for terminal errors. `SyncResetError` is
// transient: drain returns without consuming the event, so
// the next drain retries the same Create. Rejecting the
// resolver now would permanently poison it, and the eventual
// `resolveCreate(...resolve)` after the retry succeeds is a
// no-op on an already-settled promise — leaving every
// dependent event stuck failing on `await event.documentId`.
if (
event.type === SyncEventType.LocalCreate &&
!(e instanceof SyncResetError)
) {
event.resolvers.promise.catch(() => {
/* suppressed */
});
event.resolvers.reject(
new Error(`Create was cancelled: ${e}`)
);
event.resolvers.reject(e);
}
if (e instanceof FileNotFoundError) {
@ -364,8 +374,7 @@ export class Syncer {
private async processCreate(
event: Extract<SyncEvent, { type: SyncEventType.LocalCreate }>
): Promise<void> {
const effectivePath = event.path;
const contentBytes = await this.operations.read(effectivePath);
const contentBytes = await this.operations.read(event.path);
const contentHash = await hash(contentBytes);
const response = await this.syncService.create({
@ -375,7 +384,7 @@ export class Syncer {
});
await this.handleMaybeMergingResponse({
path: effectivePath,
path: event.path,
response,
contentHash,
originalContentBytes: contentBytes,
@ -384,7 +393,7 @@ export class Syncer {
this.history.addHistoryEntry({
status: SyncStatus.SUCCESS,
details: { type: SyncType.CREATE, relativePath: effectivePath },
details: { type: SyncType.CREATE, relativePath: event.path },
message:
response.type === "MergingUpdate"
? "Created file and merged with existing remote version"
@ -399,7 +408,15 @@ export class Syncer {
): Promise<void> {
const documentId = await event.documentId;
const doc = this.queue.getDocumentByDocumentIdOrFail(documentId);
const doc = this.queue.getDocumentByDocumentId(documentId);
if (doc === undefined) {
// Already deleted (e.g. a remote delete drained ahead of
// this redundant local one). Nothing to do.
this.logger.debug(
`Skipping local-delete for ${documentId} — doc no longer tracked`
);
return;
}
const relativePath = doc.path;
const response = await this.syncService.delete({
@ -408,6 +425,7 @@ export class Syncer {
});
await this.queue.removeDocument(doc.path);
this.queue.recordDeletion(documentId, response.vaultUpdateId);
this.queue.lastSeenUpdateId = response.vaultUpdateId;
this.history.addHistoryEntry({
@ -426,8 +444,17 @@ export class Syncer {
): Promise<void> {
const documentId = await event.documentId;
const { path: diskPath, record } =
this.queue.getDocumentByDocumentIdOrFail(documentId);
const tracked = this.queue.getDocumentByDocumentId(documentId);
if (tracked === undefined) {
// The doc was deleted between this event being queued and
// drained — skip silently. Common when a LocalDelete drains
// ahead of a LocalUpdate that was already in the queue.
this.logger.debug(
`Skipping local-update for ${documentId} — doc no longer tracked (deleted)`
);
return;
}
const { path: diskPath, record } = tracked;
const contentBytes = await this.operations.read(diskPath);
const contentHash = await hash(contentBytes);
@ -518,6 +545,28 @@ export class Syncer {
}
if (createEvent === undefined) {
// The disk path captured at the start of `processLocalUpdate`
// can be stale: the user may have renamed the file during the
// server roundtrip, in which case `queue.documents` already
// points at the new path and a follow-up rename's LocalUpdate
// is queued behind us. If we forced the disk back to
// `response.relativePath` here we'd undo the user's intent;
// worse, `setDocument`'s same-docId cleanup would clobber the
// map entry that was tracking the latest disk path, leaving
// future LocalUpdates for this doc reading from a vacated
// slot and getting skipped as `FileNotFoundError`. Refresh
// the latest tracked path and only touch disk when it still
// matches the captured one.
const tracked = this.queue.getDocumentByDocumentId(
response.documentId
);
if (tracked === undefined) {
this.logger.debug(
`Document ${response.documentId} is no longer tracked after update; cannot reconcile potential rename`
);
} else {
const currentPath = tracked.path ?? path;
if (currentPath === path) {
// a http response will always be more up-to-date than any queued remote update
// move will always move to the relative path when MoveOnConflict.EXISTING is given
await this.operations.move(
@ -530,6 +579,16 @@ export class Syncer {
...record,
remoteHash
});
} else {
// User renamed during the roundtrip. Leave the disk file
// at `currentPath`; the queued rename's LocalUpdate will
// reconcile the server on its next drain.
await this.queue.setDocument(currentPath, {
...record,
remoteHash
});
}
}
} else {
// The server may have deconflicted the path on create (e.g.
// another client raced us to the same path and won). Move the
@ -574,6 +633,24 @@ export class Syncer {
);
}
// The doc was deleted at-or-after the version this broadcast
// describes (e.g. another client's update committed before our
// local delete; the server's backlog is replaying it now). Apply
// would resurrect a doc we deliberately removed.
const deletedAt = this.queue.getDeletionVersion(
remoteVersion.documentId
);
if (
deletedAt !== undefined &&
deletedAt >= remoteVersion.vaultUpdateId
) {
this.queue.lastSeenUpdateId = remoteVersion.vaultUpdateId;
this.logger.debug(
`Skipping obsolete remote update for already-deleted document ${remoteVersion.documentId} (V=${remoteVersion.vaultUpdateId} <= deleted V=${deletedAt})`
);
return;
}
if (
(documentWithPath?.record.parentVersionId ?? 0) >=
remoteVersion.vaultUpdateId
@ -598,14 +675,7 @@ export class Syncer {
remoteVersion.relativePath
);
if (pendingCreate === undefined) {
return this.processRemoteCreateForNewDocument(remoteVersion);
} else {
return this.processRemoteCreateForPendingDocument(
remoteVersion,
pendingCreate
);
}
}
private async processRemoteDelete(
@ -614,6 +684,10 @@ export class Syncer {
): Promise<void> {
await this.operations.delete(path);
await this.queue.removeDocument(path);
this.queue.recordDeletion(
remoteVersion.documentId,
remoteVersion.vaultUpdateId
);
this.queue.lastSeenUpdateId = remoteVersion.vaultUpdateId;
@ -770,53 +844,7 @@ export class Syncer {
});
}
// A remote create landed at a path where we have an unsynced local
// create. This might be becuase there's another sync client running.
// We must avoid duplicating files.
private async processRemoteCreateForPendingDocument(
remoteVersion: DocumentVersionWithoutContent,
pendingCreateEvent: Extract<
SyncEvent,
{ type: SyncEventType.LocalCreate }
>
): Promise<void> {
const remoteContent = await this.syncService.getDocumentVersionContent({
documentId: remoteVersion.documentId,
vaultUpdateId: remoteVersion.vaultUpdateId
});
const remoteHash = await hash(remoteContent);
const path = remoteVersion.relativePath;
const currentContent = await this.operations.read(
pendingCreateEvent.path
);
await this.operations.write(path, currentContent, remoteContent);
await this.updateCache(
remoteVersion.vaultUpdateId,
remoteContent,
path
);
await this.queue.resolveCreate(pendingCreateEvent, {
documentId: remoteVersion.documentId,
parentVersionId: remoteVersion.vaultUpdateId,
remoteHash,
remoteRelativePath: path
});
this.queue.lastSeenUpdateId = remoteVersion.vaultUpdateId;
this.history.addHistoryEntry({
status: SyncStatus.SUCCESS,
details: {
type: SyncType.UPDATE,
relativePath: path
},
message: `Adopted remote create at ${path}`,
author: remoteVersion.userId,
timestamp: new Date(remoteVersion.updatedDate)
});
}
private async sendUpdate({
record,

5
sync-server/src/server/create_document.rs
View file

@ -73,7 +73,10 @@ pub async fn create_document(
// but client 2 moves it to P2 while client 1 creates a new document at P2,
// then client 1 would merge its new document with the moved version of A at P2
// that client 2 resulting in two files (P1 and P2) with the same doc id (A).
if latest_version.creation_vault_update_id > request.last_seen_vault_update_id {
if latest_version.creation_vault_update_id > request.last_seen_vault_update_id
&& latest_version.creation_vault_update_id == latest_version.vault_update_id
// can't allow merging with a moved document as that could create a cycle
{
let is_mergeable_text = is_file_type_mergable(
&sanitized_relative_path,
&state.config.server.mergeable_file_extensions,