waitForPersistence follows a replaced queued write to its replacement (closes #3062)

packages/desktop-app/src/lib/services/shared-node-store.svelte.ts

@@ -556,32 +556,22 @@ export class SimplePersistenceCoordinator {
     await this.flushAndWaitForNodes(Array.from(this.pendingOperations.keys()));
   }
 
+  /**
+   * Wait for the given nodes' pending writes to settle without starting any
+   * debounced write early. See `settleNode` for how a replaced queued write
+   * is followed to its replacement.
+   *
+   * @returns Set of node IDs that failed to persist or timed out
+   */
   async waitForPersistence(nodeIds: string[], timeoutMs = 5000): Promise<Set<string>> {
-    const failed = new Set<string>();
-    const promises = nodeIds.map(async (nodeId) => {
-      const pending = this.pendingOperations.get(nodeId);
-      if (pending) {
-        try {
-          await Promise.race([
-            pending.promise,
-            new Promise<void>((_, reject) =>
-              setTimeout(() => reject(new Error('Timeout')), timeoutMs)
-            )
-          ]);
-        } catch {
-          failed.add(nodeId);
-        }
-      }
-    });
-    await Promise.all(promises);
-    return failed;
+    return this.settleNodes(nodeIds, timeoutMs, false);
   }
 
   /**
    * Flush specific pending operations immediately and wait for completion.
    *
-   * Unlike waitForPersistence which only waits for in-flight operations,
-   * this method also triggers debounced operations that haven't started yet.
+   * Unlike waitForPersistence, which waits out a debounced write's timer,
+   * this method starts debounced operations that haven't started yet.
    *
    * Use this when you need to ensure specific nodes are fully persisted
    * before performing dependent operations (e.g., moveNode that references them).
@@ -591,6 +581,14 @@ export class SimplePersistenceCoordinator {
    * @returns Set of node IDs that failed to persist
    */
   async flushAndWaitForNodes(nodeIds: string[], timeoutMs = 5000): Promise<Set<string>> {
+    return this.settleNodes(nodeIds, timeoutMs, true);
+  }
+
+  private async settleNodes(
+    nodeIds: string[],
+    timeoutMs: number,
+    startDebounced: boolean
+  ): Promise<Set<string>> {
     const failed = new Set<string>();
     let timeoutId: ReturnType<typeof setTimeout> | undefined;
     const timeout = new Promise<'timeout'>((resolve) => {
@@ -599,21 +597,22 @@ export class SimplePersistenceCoordinator {
 
     await Promise.all(
       nodeIds.map(async (nodeId) => {
-        if (!(await this.flushNode(nodeId, timeout))) failed.add(nodeId);
+        if (!(await this.settleNode(nodeId, timeout, startDebounced))) failed.add(nodeId);
       })
     );
     clearTimeout(timeoutId);
     return failed;
   }
 
   /**
-   * Start `nodeId`'s debounced write if it is still waiting, then wait for the
-   * node's pending entry to settle. Returns false on failure or timeout.
+   * Wait for `nodeId`'s pending entry to settle, first starting its debounced
+   * write if `startDebounced` is set and it is still waiting. Returns false on
+   * failure or timeout.
    *
    * A queued write replaced during the wait rejects with
    * `OperationCancelledError` while the node's in-flight write and the write
    * that replaced it are still running. The entry is looked up again after
-   * every cancellation, so the flush waits on the replacement instead of
+   * every cancellation, so the wait follows the replacement instead of
    * resolving early. A cancellation that leaves nothing pending dropped the
    * write outright (e.g. `clearQueued()` after an OCC conflict), so it counts
    * as a failure.
@@ -625,10 +624,14 @@ export class SimplePersistenceCoordinator {
    * `runOperation`'s `finally` owns that, and a delete from here could remove
    * the placeholder it registers for the next queued write.
    */
-  private async flushNode(nodeId: string, timeout: Promise<'timeout'>): Promise<boolean> {
+  private async settleNode(
+    nodeId: string,
+    timeout: Promise<'timeout'>,
+    startDebounced: boolean
+  ): Promise<boolean> {
     let pending = this.pendingOperations.get(nodeId);
     while (pending) {
-      if (pending.debounced && !this.executingOperations.has(nodeId)) {
+      if (startDebounced && pending.debounced && !this.executingOperations.has(nodeId)) {
         clearTimeout(pending.timeoutId);
         pending.debounced = false;
         void pending.operation();
@@ -646,7 +649,7 @@ export class SimplePersistenceCoordinator {
         pending = next;
       }
     }
-    // Nothing was pending, so there was nothing to flush.
+    // Nothing was pending, so there was nothing to wait for.
     return true;
   }
 
@@ -3932,8 +3935,9 @@ export class SharedNodeStore {
    * Wait for pending node saves to complete with timeout
    * Delegates to PersistenceCoordinator
    *
-   * NOTE: This only waits for already-executing operations. It does NOT trigger
-   * debounced operations that haven't started yet. For that, use flushNodeSaves().
+   * NOTE: This waits for pending, executing and queued writes, but does NOT
+   * start a debounced write early; it settles once the debounce timer fires
+   * and the write lands. To start it immediately, use flushNodeSaves().
    *
    * @param nodeIds - Array of node IDs to wait for
    * @param timeoutMs - Timeout in milliseconds (default 5000)
@@ -3946,8 +3950,8 @@ export class SharedNodeStore {
   /**
    * Flush specific pending node saves immediately and wait for completion.
    *
-   * Unlike waitForNodeSaves which only waits for in-flight operations,
-   * this method also TRIGGERS debounced operations that haven't started yet.
+   * Unlike waitForNodeSaves, which waits out a debounced write's timer,
+   * this method STARTS debounced operations that haven't started yet.
    *
    * Use this when you need to ensure specific nodes are fully persisted
    * before performing dependent operations (e.g., moveNode that references them).

packages/desktop-app/src/tests/services/persistence-coordinator-flush-wait.test.ts

@@ -8,6 +8,9 @@
  * must also never delete a pending entry it did not start, such as the
  * placeholder registered for a write queued behind the one it started.
  *
+ * waitForPersistence() follows a replaced queued write the same way, but never
+ * starts a debounced write early.
+ *
  * Exercises the coordinator directly, like
  * persistence-coordinator-promotion.test.ts.
  */
@@ -143,3 +146,111 @@ describe('SimplePersistenceCoordinator - flushAndWaitForNodes', () => {
     stuck.release();
   }, 3000);
 });
+
+describe('SimplePersistenceCoordinator - waitForPersistence', () => {
+  let coordinator: SimplePersistenceCoordinator;
+
+  beforeEach(() => {
+    SimplePersistenceCoordinator.resetInstance();
+    coordinator = SimplePersistenceCoordinator.getInstance();
+  });
+
+  afterEach(() => {
+    SimplePersistenceCoordinator.resetInstance();
+  });
+
+  it('keeps waiting when the queued write it waits on is replaced', async () => {
+    const nodeId = 'wait-replaced';
+    const done: string[] = [];
+    const inFlight = blockedWrite(done, 'in-flight');
+    const replacement = blockedWrite(done, 'replacement');
+
+    coordinator.persist(nodeId, inFlight.operation, { mode: 'immediate' });
+    const replaced = coordinator.persist(nodeId, async () => {}, { mode: 'debounce' });
+    replaced.promise.catch(() => {});
+
+    let settled = false;
+    const waited = coordinator.waitForPersistence([nodeId], 2000).then((failed) => {
+      settled = true;
+      return failed;
+    });
+
+    // Replace the queued write the wait is waiting on.
+    coordinator.persist(nodeId, replacement.operation, { mode: 'debounce' });
+    await yieldToTimers();
+    expect(settled).toBe(false);
+
+    inFlight.release();
+    await yieldToTimers();
+    expect(settled).toBe(false);
+
+    replacement.release();
+    expect(await waited).toEqual(new Set());
+    expect(done).toEqual(['in-flight', 'replacement']);
+  }, 3000);
+
+  it('reports a failure when the queued write is cancelled with nothing replacing it', async () => {
+    const nodeId = 'wait-cleared';
+    const inFlight = blockedWrite([], 'in-flight');
+
+    coordinator.persist(nodeId, inFlight.operation, { mode: 'immediate' });
+    const queued = coordinator.persist(nodeId, async () => {}, { mode: 'debounce' });
+    queued.promise.catch(() => {});
+
+    const waited = coordinator.waitForPersistence([nodeId], 2000);
+    coordinator.clearQueued(nodeId);
+    inFlight.release();
+
+    expect(await waited).toEqual(new Set([nodeId]));
+  }, 3000);
+
+  it('follows a debounced write replaced by another debounced write', async () => {
+    const nodeId = 'wait-debounce-replaced';
+    const done: string[] = [];
+
+    const replaced = coordinator.persist(nodeId, async () => {}, { mode: 'debounce' });
+    replaced.promise.catch(() => {});
+    const waited = coordinator.waitForPersistence([nodeId], 2000);
+
+    // Nothing is executing, so this replaces the debounced entry outright.
+    coordinator.persist(
+      nodeId,
+      async () => {
+        done.push('replacement');
+      },
+      { mode: 'debounce' }
+    );
+
+    expect(await waited).toEqual(new Set());
+    expect(done).toEqual(['replacement']);
+  }, 3000);
+
+  it('does not start a debounced write early', async () => {
+    const nodeId = 'wait-debounced';
+    const done: string[] = [];
+
+    coordinator.persist(
+      nodeId,
+      async () => {
+        done.push('debounced');
+      },
+      { mode: 'debounce' }
+    );
+    const waited = coordinator.waitForPersistence([nodeId], 2000);
+    await yieldToTimers();
+
+    // Still inside the debounce window: the wait must not have run the write.
+    expect(done).toEqual([]);
+    expect(await waited).toEqual(new Set());
+    expect(done).toEqual(['debounced']);
+  }, 3000);
+
+  it('reports a timeout as a failure', async () => {
+    const nodeId = 'wait-timeout';
+    const stuck = blockedWrite([], 'stuck');
+
+    coordinator.persist(nodeId, stuck.operation, { mode: 'immediate' });
+    expect(await coordinator.waitForPersistence([nodeId], 20)).toEqual(new Set([nodeId]));
+    stuck.release();
+  }, 3000);
+});