Skip to content

fix(sync): recover journal metadata updates raised while the sync runtime is down - #970

Merged
h4yfans merged 2 commits into
mainfrom
journal-offline-clock-fallback
Aug 5, 2026
Merged

fix(sync): recover journal metadata updates raised while the sync runtime is down#970
h4yfans merged 2 commits into
mainfrom
journal-offline-clock-fallback

Conversation

@h4yfans

@h4yfans h4yfans commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator

Closes #965

The hole

Journals had the same fallback-less local adapter notes had before #963getJournalSyncService()?.enqueueUpdate(...), optional chaining, no else — and were additionally excluded from the sweep that fixed notes. recoverDirtyNotes filters journalDate IS NULL by construction.

So a journal metadata update raised while the sync runtime was down (quit, vault switch, re-auth) had nothing left to re-push it: no queue row, no dirty marker, no sweep. Real callers that hit this are notes/entity-properties.ts (journal properties) and ipc/journal-handlers.ts (tags/frontmatter).

Nothing compensated — verified in source, not assumed

  • journalHandler.seedUnclocked only selects clock IS NULL, so it owns first pushes and never sees a journal the server already knows.
  • seedUnclockedNotes excludes journals outright (journalDate IS NULL).
  • checkManifestIntegrity re-enqueues only items absent from the server manifest (if (!serverRef)). It does no version comparison, so a journal that exists on the server at a stale version is invisible to it.

The fix

1. Offline fallback on the journal adapter's enqueueUpdate (local-mutations.ts), reusing incrementNoteClockOffline. Journals are note_metadata rows and share the exact columns that helper touches (clock, syncedAt, localOnly), so no second helper was warranted — its doc comment now states it serves both.

2. A recoverDirtyJournals arm (dirty-recovery.ts) selecting journalDate IS NOT NULL, routed through the journal sync service, adding journals to RecoveryResult.

3. enqueueRecoveredUpdate forwards extra args (sync-core, content-sync-base.ts) so the journal recovery can hand over the entry's date.

Decisions, and why

Separate recoverDirtyJournals, not a branch in recoverDirtyNotes. The two route to different sync services and the journal payload builder takes an argument the note one does not. Folding them together would push a service switch and a date through a query that needs neither, and would edit the shipped note query — which I wanted left byte-for-byte alone. The doc comment that declared the ownership split is updated rather than contradicted.

The recovery arm must pass the date — this is the non-obvious part. enqueueRecoveredUpdate(itemId) calls enqueueForPush(itemId, 'update', ...getFallbackArgs()), and getFallbackArgs() returns []. For journals TArgs = [string], so the date reaches JournalSyncService.buildSnapshotPayload as undefined. That builder calls getJournalPath(date) outside its own try/catch, and formatJournalFilename does isoDate.split('-') — so a naive copy of the note arm throws a TypeError out of the recovery loop and takes the whole sweep down with it, tasks and projects included. Hence change (3).

I considered instead defaulting to cached.journalDate inside the payload builder (one file, no shared-package change). Rejected: it forces the builder to fabricate a payload for the "journal row with no date" case it cannot skip, and the frozen bad payload would still reach the wire via resolvePushPayload's fallback. Passing the date keeps the builder's date: string contract intact and makes the omission a compile error at the ContentSyncService boundary.

The two fixed constraints

Clock is advanced at write time. The fallback bumps before returning; recovery deliberately does not bump (content-sync-base.ts re-sends the stored clock). Pinned by journal-sync.test.ts, which asserts the recovered push carries the stored clock and the row is not re-bumped, and by the dirty-recovery test asserting { 'device-A': 3 } after the fallback.

No _offline key. incrementNoteClockOffline bumps under sync_devices.is_current_device and no-ops when no device is registered — matching handleMissingDevice on the online path. ContentSyncService has no rebinding hook, so an _offline tick would reach peers as a device id two machines could both claim. Already covered by the note tests; journals inherit it by using the same helper.

Backward compatibility

No schema change, no migration, no contract or payload change. clock and syncedAt are existing columns; syncedAt is already nullable and already means "never confirmed synced". Nothing new goes on the wire — the recovered push is an ordinary journal update, and journalHandler.buildPushPayload rebuilds it from the row at push time exactly as before. Peers on older builds are unaffected.

enqueueRecoveredUpdate(itemId, ...extra) is additive: every existing caller passes no extra args and takes the identical getFallbackArgs() path.

Tests

pnpm --filter @memry/desktop exec vitest run --config config/vitest.config.ts --project main src/main/sync

PASS (1654) FAIL (0) skipped (3)

packages/sync-core: PASS (6) FAIL (0). pnpm typecheck green. ESLint 0 errors on all changed files. docs:impact --strict passes, docs:build green.

New coverage:

  • dirty-recovery.test.ts — real in-memory data DB and the real recovery predicate: a diverged journal is recovered; clean, local-only, clock-less journals and plain notes are left alone; and end-to-end, a server-confirmed journal is not recovered until the fallback runs, then is re-pushed at an advanced clock.
  • journal-sync.test.ts — real RecordSyncController + real queue: a recovered update carries the date it was given and re-sends the stored clock without bumping it.
  • local-mutations.test.ts — the journal adapter reaches the fallback when the service is null, and does not when it is up.

Mutation check

Each half reverted separately, red observed, restored.

  1. Adapter fallback (incrementNoteClockOffline → no-op):
1. local-mutations falls back to the offline clock bump when the journal sync service is down
   AssertionError: expected "vi.fn()" to be called with arguments: [ {}, 'journal-1' ]
   Number of calls: 0
  1. Recovery arm (recoverDirtyJournals call removed):
1. dirty-recovery journals recovers a synced journal whose local change never reached the server
   AssertionError: expected +0 to be 1
2. dirty-recovery journals re-pushes a journal whose update was enqueued while the sync service was down
   AssertionError: expected +0 to be 1
  1. Date pass-through (extra ignored in enqueueRecoveredUpdate):
1. JournalSyncService push re-pushes a recovered update at the stored clock, carrying the date it was given
   AssertionError: expected undefined to be '2026-05-10'

Restored: PASS (1654) FAIL (0).

Deliberately out of scope

  • enqueueCreate / enqueueDelete on journals keep the no-op. Confirmed in source rather than carried over from the note conclusion: journalHandler.seedUnclocked sweeps clock IS NULL AND journalDate IS NOT NULL into a create at the next runtime start, so a journal that has never been pushed is already owned.
  • checkManifestIntegrity doing version comparison. It would also cover this class of drift, but it is a 30-minute periodic network check with a much wider blast radius than the issue calls for.
  • CRDT body content. This is the metadata push only; journal bodies ride the CRDT path and are unaffected.

h4yfans added 2 commits August 5, 2026 20:58
…is down

Journals had the same fallback-less local adapter notes had before #963, and
were additionally excluded from the recovery sweep that fixed notes
(`recoverDirtyNotes` filters `journalDate IS NULL`). A journal metadata update
raised while the sync runtime was down had nothing to re-push it: no queue row,
no dirty marker, no sweep.

- journal adapter `enqueueUpdate` falls back to `incrementNoteClockOffline`,
  which bumps under the real device id and clears `syncedAt`.
- new `recoverDirtyJournals` arm re-pushes those rows at the next runtime start,
  routed through the journal sync service and passing the row's date.
- `enqueueRecoveredUpdate` forwards extra args so the journal payload builder
  gets its date instead of throwing on `undefined`.
Copilot AI lite review requested due to automatic review settings August 5, 2026 18:00

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@github-actions github-actions Bot added bug Something isn't working documentation Improvements or additions to documentation test labels Aug 5, 2026
@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown

React Doctor found no new issues. 🎉

Reviewed by React Doctor for commit d9fa1f1.

@codecov

codecov Bot commented Aug 5, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@h4yfans
h4yfans marked this pull request as ready for review August 5, 2026 18:28
@h4yfans
h4yfans merged commit 810c127 into main Aug 5, 2026
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working documentation Improvements or additions to documentation test

Projects

None yet

Development

Successfully merging this pull request may close these issues.

sync: journal metadata updates are lost when the sync runtime is down (no offline fallback, outside dirty recovery)

2 participants