Skip to content

Replay a sync delta on the local history before asking the server (#1905) - #1920

Closed
michielbdejong wants to merge 2 commits into
developfrom
claude/sync-delta-local-base
Closed

michielbdejong wants to merge 2 commits into
developfrom
claude/sync-delta-local-base

Conversation

@michielbdejong

Copy link
Copy Markdown
Contributor

Closes #1905. Follow-up to #1846 / #1904: this is why "+ Add view" still lacked the app in the atomic-plugins money e2e after #1904.

Root cause

A local query returns JSON-AD, not snapshots. So every resource it hydrates (the table page's app query, the sidebar, and so on) gets a Loro doc rebuilt from values. That doc has new ops, by a new peer, and none of the history. Two things went wrong from there:

  1. addResource wrote that rebuilt doc back to the local database, replacing the real history the query had just read the values from. putResourceWithSnapshot stores the snapshot as given. The new lib test shows the write: a history-less, single-peer snapshot.
  2. The drive sync after a reload sends deltas from the version vectors the local database reported. None of them can apply to a rebuilt doc. So each one parked as incomplete Loro import … missing base state and waited on a full snapshot from the server, with the resource marked as recovering.

Evidence from the traces (atomic-plugins run 36616868387, candidate16 3830675 with #1904)

  • The page loads onto the table at t=126.04 s, the ClientDb is ready at about 127.2 s, and "+ Add view" opens at about 127.6 s.
  • At 128.49 s, five ws-sync-push imports come in incomplete. They are the app's entry point (HTGzQ…), its agent, its own row class (HVD4W…), its ontology (QQ-gR…), and one more (KIpnk…). These are exactly the resources the previous page created or edited.
  • No repair lands in the 9 s that follow: no failed to recover warning appears, and "New app" never appears in the menu. This happened on the first try and again on the retry.
  • The page before the reload shows the same pattern. About 45 incomplete imports arrive at 103.7 s, and the App iframe the test waits on takes 20 s to show.

Ruled out

  • The hook missing the landing. I wrote a vitest with the real store and real Loro: menu open, the app query-hydrated from stale JSON-AD, an incomplete delta, and the server snapshot landing later. It passes on Ask the drive for its apps again when a view menu opens (#1846) #1904's hook. When a repair does land, ResourceUpdated fires and the hook folds the app in.
  • The menu failing to re-render. DropdownMenu filters its items prop on every render.

I could not see why the server repair was slow in CI, because Playwright traces don't record WebSocket frames. This PR takes that round trip off the path instead of tuning it.

Fix (browser/lib/src/store.ts)

  • The store records which subjects have a doc rebuilt from JSON-AD with no history (_withoutLoroHistory). It sets this in hydrateOfflineReplay when there is no snapshot, and clears it when a full snapshot arrives from the server, when an HTTP fetch returns, or when the resource is evicted.
  • writeToClientDb does not write such a doc back to the local database unless it carries an unsaved edit. The local history survives.
  • recoverFromIncompleteImport first tries replayOnLocalBase:
    • It reads the subject's snapshot from the local database, which is by construction the base the server diffed against.
    • It replaces a history-less doc with that snapshot, or merges when the doc holds an edit that hasn't reached the server.
    • It then replays every update that could not apply.
    • It asks the server only when that fails. With no ClientDb, the behavior is unchanged: the server fetch still fires synchronously.

Tests

  • browser/lib/src/sync-delta-local-base.test.ts, 5 cases:

    • no write-back of a rebuilt doc;
    • a delta applies on the local base with no server call, both when the resource was query-hydrated and when it is new to the page;
    • the repaired doc is written with its real history only;
    • the store still asks the server when there is no local base.

    4 of the 5 fail on develop.

  • browser/data-browser/src/chunks/AppPage/useDriveApps.localBase.test.tsx covers the exact order from the traces:

    1. The menu is open and the app is known for its own rows only.
    2. The sync delta that adds the table's class arrives and can't apply.
    3. The server never answers.
    4. The app is offered on the table without reopening the menu.

    It fails on develop's hook and on Ask the drive for its apps again when a view menu opens (#1846) #1904's hook without this fix, and passes with it on both.

  • Lib vitest: 1085 passed. data-browser vitest: 1450 passed. Lib typecheck and lint are clean. data-browser tsc reports one error in AICreditsService.tsx, which this PR doesn't touch.

Cherry-pick onto candidate16 / candidate17

Three conflicts in store.ts need the same hand edits on both. The candidates predate writeToClientDb, isPersistableState, lastPersisted and outbox.mayHavePending:

  1. In hydrateOfflineReplay, keep the candidate's ): Resource { return type and add const existing = this.getResolved(subject);.
  2. In addResource's inline persist condition, append && !(this._withoutLoroHistory.has(this.normalizeSubject(emitResource.subject)) && !emitResource.hasUnsavedChanges()).
  3. In evictResource, keep this.lastPersistedStamp.delete(resolved); and add this._withoutLoroHistory.delete(resolved);.
  4. In replayOnLocalBase, this.outbox.mayHavePending becomes this.outbox.hasPending.

With those edits, on both candidates the lib vitest passes (1156 on candidate16 and 1170 on candidate17), and so do AppPage and TablePage, including the new hook test. TableViewTabs.tsx is not touched.

Not covered

Editing a resource that is still without its history saves from the rebuilt doc, as before. TESTING_COVERAGE.md notes this. Loading the local snapshot before the first edit would close that gap and belongs in its own change.

🤖 Generated with Claude Code

)

A local query returns JSON-AD without snapshots, so the resources it hydrates
get a Loro doc rebuilt from values: new ops by a new peer, none of the
history. Two things followed.

- `addResource` wrote that doc back to the local database, replacing the
  history the query had just read its values from.
- The drive sync after a reload sends deltas from the version vectors the
  local database reported. None of them can apply to a rebuilt doc, so each
  parked as an incomplete import and waited on a full snapshot from the
  server. In the atomic-plugins money e2e the app a table offers sat under
  that repair past the ten seconds the test waited for it in "+ Add view"
  (#1846).

Now the store records which docs were rebuilt without history, does not
write them back, and replays an unappliable delta on the local database's
snapshot, which is by construction the base the server diffed against. The
server is asked only when that fails.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@michielbdejong michielbdejong added bug Something isn't working @tomic/lib @tomic/lib labels Sep 29, 2026
michielbdejong pushed a commit that referenced this pull request Sep 29, 2026
CI run 36636664834 on candidate17b failed the delete resource @smoke
e2e with an unexpected browser warning:

  [Store] failed to recover missing state for <child>: Resource not found

After the reload, a sync delta for the nested folder parks as incomplete
and the store repairs it. With #1920 the repair first waits on the local
database, then asks the server; by then the test has deleted the parent,
the server cascaded to the child, and the server's not-found is correct.
The store warned and called failResource, which puts an errored copy of
a deleted resource back. Locally: 1 of 3 runs failed with #1920, 0 of 6
without it, 0 of 6 with this fix.

The repair now checks whether the subject was destroyed (or has a
destroy in flight): it skips the server fetch then, and a failure after
the delete is logged as info, without failResource. Unit tests in
sync-delta-local-base.test.ts cover both.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
CI run 36636664834 on candidate17b failed the delete resource @smoke
e2e with an unexpected browser warning:

  [Store] failed to recover missing state for <child>: Resource not found

After the reload, a sync delta for the nested folder parks as incomplete
and the store repairs it. With #1920 the repair first waits on the local
database, then asks the server; by then the test has deleted the parent,
the server cascaded to the child, and the server's not-found is correct.
The store warned and called failResource, which puts an errored copy of
a deleted resource back. Locally: 1 of 3 runs failed with #1920, 0 of 6
without it, 0 of 6 with this fix.

The repair now checks whether the subject was destroyed (or has a
destroy in flight): it skips the server fetch then, and a failure after
the delete is logged as info, without failResource. Unit tests in
sync-delta-local-base.test.ts cover both.

(cherry picked from commit 06c6e1b)

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@michielbdejong

Copy link
Copy Markdown
Contributor Author

Pushed ef90a13 ("Store: a resource deleted during gap recovery is not a failure"). It's cherry-picked from candidate17b (06c6e1b) and applied cleanly on this branch. sync-delta-local-base.test.ts passes 7 of 7, and the browser/lib suite passes 1087 tests.

Why: with this PR on candidate17b, the delete resource @smoke e2e failed in CI (run 36636664834) on an unexpected browser warning:

[Store] failed to recover missing state for <nested folder>: Resource not found. DID Resource … not found locally

The Playwright trace shows this sequence:

  1. After the reload, a ws-sync-push delta for the nested folder can't apply, so recoverFromIncompleteImport starts.
  2. replayOnLocalBase waits for the local database first. It has no base for this subject, so the repair falls back to the server.
  3. Meanwhile the test deletes the parent, and the server cascades the delete to the child. The server's "not found" is therefore correct.
  4. The store logged that as a warning and called failResource, which puts an errored copy of a deleted resource back into the store.

Before this PR, the repair went to the server straight away and won the race. Waiting on the local database first widened the window. Locally, the test failed 1 run in 3 with this PR, and passed 6 of 6 without it.

Fix:

  • The repair now checks isDestroyed(subject) || hasPendingDestroy(subject) and skips the server fetch when either is true.
  • A failure that comes back after the delete is logged at info level, without failResource.

Two unit tests cover it in sync-delta-local-base.test.ts: a delete during the repair, and a resource already deleted before the repair. The first test fails without the fix. With the fix, the e2e passes 6 of 6 locally, all 19 @smoke tests pass, and candidate17b's CI is green (run 36644412818).

joepio commented Sep 30, 2026

Copy link
Copy Markdown
Member

@michielbdejong CI run 4835 on ef90a13 (Mancave, full e2e) is red on two tests that develop (run 4829) passes: saved-drives.spec.ts:81 ("a second drive, made after a reload, is listed too") and search.spec.ts:130 ("scoped search"). One flaky: website-errors.spec.ts:4. Could you take a look? Both touch drive listing and search after a reload, which is where the local-base change reads from.


Generated by Claude Code

joepio added a commit that referenced this pull request Oct 6, 2026
joepio added a commit that referenced this pull request Oct 6, 2026
One tested commit for the open PRs of 5 October: link device to account (#1948), one profile form (#1953), faster branch runs (#1956), end-to-end encrypted conversations (#1971), passkey ceremony header (#1972), Flutter account package (#2035), cheap canvas undo (#2053), atomic.place tab title (#2056), table resource cell (#2057), small table fixes (#2058), storage map (#2061), Sync page cloud ladder (#2072), several served-domain suffixes (#2073), one sign-in (#2074), smaller chat messages (#2077), bring-along e2e fix (#2078), inline row data loss (#2079), Retry-After (#2080), #2070, #2071, #2066, #2067, #2068, #1969, #1887, #1897 and #1920.

Compressed stored values from #2077 stay readable, but a downgrade cannot read compressed rows.

Co-authored-by: Joep Meindertsma <joep@ontola.io>
@joepio joepio closed this Oct 6, 2026

joepio commented Oct 6, 2026

Copy link
Copy Markdown
Member

Merged into develop through #2085 (batch of 5 October), so this PR is closed. Your commits are in the squashed history.


Generated by Claude Code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working @tomic/lib @tomic/lib

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Store: sync updates for resources without local history trigger 'incomplete Loro import … fetching a full snapshot'

2 participants