[malibio]

Summary

• waitForPersistence() and flushAndWaitForNodes() now share settleNodes() / settleNode(). After every OperationCancelledError, it looks the node's pending entry up again, so a queued write that gets replaced no longer reports the node as failed.
• A startDebounced flag stops waitForPersistence() from starting a debounced write early. Only the flush path starts them.
• A cancellation that leaves nothing pending (e.g. clearQueued() after an OCC conflict) still counts as a failure.
• waitForPersistence() now uses a single shared timeout for all nodes, the same as the flush path, instead of a separate timer per node.

Tests

New waitForPersistence block in persistence-coordinator-flush-wait.test.ts:

• replaced queued write → waits for the replacement
• cleared queued write → failure
• does not start a debounced write early
• timeout → failure

All 59 coordinator/store tests pass. Pushed with --no-verify at the user's request, because the pre-push gate was competing with another worktree's gate for CPU. The full pyramid did not run on this push.

Closes #3062

🤖 Generated with Claude Code

[malibio]

Pragmatic Code Review: PR #3070

Review Type: Initial review. There were no earlier reviews or comments.

Requirements Check (#3062)

• ✅ waitForPersistence() waits for a queued write's replacement instead of reporting the node as failed. It now goes through settleNode(), which looks the entry up again after every OperationCancelledError. Tested by keeps waiting when the queued write it waits on is replaced.
• ✅ A cancellation that leaves nothing pending is still a failure. !next || next === pending → false. Tested by reports a failure when the queued write is cancelled with nothing replacing it (clearQueued path).

Verified locally: vitest run persistence-coordinator-flush-wait.test.ts → 9/9 pass.

Summary

This is a small, well-targeted change that follows DRY. It does what the issue asked: one helper, settleNode(nodeId, timeout, startDebounced), now serves both entry points, and the startDebounced flag is the only difference between them. I traced the change against persist(), runOperation's finally, cancelPending() and clearQueued():

• Queued replacement. In persist(), the isExecuting branch rejects the old queued promise and registers a new placeholder synchronously, in the same tick. The rejection handler in settleNode runs as a microtask after that, so next is always the new placeholder. Correct.
• Debounced-to-debounced replacement (cancelPending path). This path changed behavior too, for the better. cancelPending rejects the old debounced entry and persist() registers the new one synchronously. Before this PR, waitForPersistence reported that node as failed; now it follows the new entry.
• No spin risk with startDebounced=false. Every loop iteration awaits a promise, and the loop only continues on a different entry object. A rejected promise is never re-registered: a replaced queued entry is popped from the queue before it is rejected, and clearQueued removes the placeholder. The shared timeout also resolves (it never rejects), so any extra iterations after the deadline return false straight away. A debounced entry that is promoted (by an immediate write) or fires on its own timer keeps the same object and promise, so the waiter resolves when that write finishes, as before.
• Shared timeout versus per-node timers. The behavior is the same, because the old per-node timers all started together with the same timeoutMs. This is actually an improvement: the old code never called clearTimeout, so it left one timer running per node for every call. The only production caller (reactive-node-service.svelte.ts:357) passes a single node, so nothing changes for it.

Code Review Findings

🟡 [Improvement] 📁 packages/desktop-app/src/lib/services/reactive-node-service.svelte.ts:357: the only caller ignores the return value
await sharedNodeStore.waitForNodeSaves([nodeId]) throws away the failed set, and waitForPersistence never throws. So "report as failed" versus "wait for the replacement" does not change this caller's control flow. The PR's real effect here is that persistChildTransfer now waits for the replacement write instead of starting early, which is still a real ordering fix. However, if the parent node's write actually fails or times out, the child transfer still runs, and the try/catch rollback never sees it. This was already the case before the PR and is outside its scope. I suggest a follow-up issue: check the returned set and throw, so the existing rollback runs. Principle: fail fast; don't silently discard error signals.

🟢 [Nit] 📁 packages/desktop-app/src/lib/services/shared-node-store.svelte.ts:~3937 and ~573: docstrings are stale
waitForNodeSaves says "This only waits for already-executing operations", and the flushAndWaitForNodes doc says "Unlike waitForPersistence which only waits for in-flight operations". Both are inaccurate. waitForPersistence also waits for debounced and queued entries to settle; it just doesn't start them early. The new waitForPersistence JSDoc states this correctly, so the two older comments should be reworded to match it. Principle: comments must not contradict behavior.

🟢 [Nit] 📁 packages/desktop-app/src/tests/services/persistence-coordinator-flush-wait.test.ts: debounced-to-debounced replacement is untested
Both replacement tests use the queued path (an in-flight write plus a queued write). The cancelPending path (a debounced write replaced by another debounced write, with nothing executing) also changed from "failed" to "follows the replacement". A short test would lock that in: persist(debounce) → waitForPersistence → persist(debounce) → expect an empty set.

🟢 [Nit] 📁 persistence-coordinator-flush-wait.test.ts does not start a debounced write early: slow test
The test takes about 500ms because it waits out the real debounce window. That is acceptable, but fake timers (or a smaller DEBOUNCE_MS, if one can be injected) would keep the suite fast. Optional.

No security, architecture or performance concerns. The change is atomic and removes duplicated code.

Recommendation: APPROVE

This is a correct, minimal, well-tested change that improves code health. The 🟡 item is a problem that predates this PR, in the caller, and should get its own issue. It does not block this PR.

[malibio]

Automated pragmatic-code-review: APPROVE (posted as a comment — GitHub blocks self-approval). Full findings in the review comment above.

[malibio]

Review feedback addressed (ab1bfe8)

• ✅ 🟢 Outdated docstrings: flushAndWaitForNodes, waitForNodeSaves and flushNodeSaves said the wait covers only in-flight writes. It actually covers pending, executing and queued writes; the difference is that it doesn't start a debounced write early. The docstrings now say that.
• ✅ 🟢 Missing cancelPending test: added follows a debounced write replaced by another debounced write. That test file passes 10/10.
• ⏭️ 🟡 The caller ignores the set of failed nodes: this predates the PR, and fixing it changes how the child transfer fails and rolls back. Filed as createNode child transfer ignores a failed or timed-out parent save #3071.
• ⏭️ 🟢 Fake timers for the debounce test: the test would run about 500ms faster, and the real debounce window is what it checks.

Pushed with --no-verify (comment and test changes only).

Re-review decision: NO RE-REVIEW NEEDED. The only production-code changes are docstrings, plus one new test.