Skip to content

fix(sync): bank each attachment as it arrives, not after the whole batch - #317

Merged
thetechjon merged 2 commits into
devfrom
fix/sync-attachment-bank-as-fetched
Sep 8, 2026
Merged

fix(sync): bank each attachment as it arrives, not after the whole batch#317
thetechjon merged 2 commits into
devfrom
fix/sync-attachment-bank-as-fetched

Conversation

@thetechjon

Copy link
Copy Markdown
Collaborator

Jon's vault sync hung on «Syncing…» and then «Sync timed out — check your connection and retry.» (08/09/2026).

Root cause (measured): the vault's Files/ holds 175 images, 83.3 MiB, and the default attachmentsFolder is Files, so every pull classifies all of them as attachmentCreated. syncApply fetched the whole batch (base64 blob API, ~111 MB on the wire) and wrote to IndexedDB only after the batch settled; the 45 s whole-sync watchdog aborted mid-fetch, nothing was banked, and every retry restarted from zero.

Fix: persist each attachment inside the mapper as its blob lands, so a timeout leaves the fetched ones banked and the next pull only sees the rest (converges over a few syncs). getBlobBytes now asks for application/vnd.github.raw (bytes, not base64) with a JSON fallback. SYNC_WATCHDOG_MS and the Files default are untouched (product decisions); the user-side workaround today is the gitignore overlay Files/.

Test: syncApply.test.ts — an AbortError on the 2nd of 3 attachments keeps the 1st persisted and propagates. Fails on the old code ({created: 2, failed: 1} resolved instead of rejected), passes now. Lint, typecheck, 3336 tests, build: green.

Follow-up (separate): thread the watchdog's controller.signal into runApplygetBlobBytes so the abort reaches in-flight blob fetches.

🤖 Generated with Claude Code

https://claude.ai/code/session_01Ndqko5Lm9khVfibZdxue5K

A vault with a large `Files/` folder could never finish its first sync.
Measured on ipapakonstantinou/obsidian: `Files/` = 175 images, 83.3 MiB.
`settingsStore.ts` defaults `attachmentsFolder: 'Files'`, so syncPull
classifies all 175 as `attachmentCreated`, and applyAttachmentClassifications
fetched them all (mapWithConcurrency, DEFAULT_CONCURRENCY = 8, getBlobBytes =
GET /git/blobs/{sha}, base64 => ~111 MB on the wire) and only wrote them to
IndexedDB AFTER the entire batch resolved.

SYNC_WATCHDOG_MS (45s) wraps the whole runSync, so the watchdog fired
mid-fetch, zero attachments were banked, the next pull re-classified all 175
as created, and every retry restarted from nothing: permanent "Syncing..."
then "Sync timed out - check your connection and retry."

The apply was all-or-nothing; now it is incremental.

- syncApply.ts: `putAttachmentAtPath` moved into the per-item mapper, so each
  blob is persisted the moment it lands. Concurrency is unchanged. A partially
  applied batch is consistent by construction: syncPull classifies attachments
  straight off IDB (`listAttachmentPaths` + `getAttachmentGitSha`, which
  recomputes the sha from the stored bytes), so there is no manifest or tree
  snapshot to keep in step - the next run's `attachmentCreated` set simply
  shrinks by whatever was banked, and repeated retries converge instead of
  looping. Write order is no longer input order; nothing depends on it (one
  write per distinct path).
- syncApply.ts: an AbortError now rejects the batch instead of being swallowed.
  A caller abort means the sync is over; grinding through the remaining blobs
  and reporting the cancellation as N per-file failures was both wasted network
  and a wrong count. A genuine per-file failure is still logged and counted as
  `failed` without aborting the batch, as before.
- github.ts `getBlobBytes`: request `Accept: application/vnd.github.raw` and
  read `res.arrayBuffer()`. githubFetch returns a raw Response, so this is
  contained to this one caller. Drops the base64 inflation (111 MB -> 83 MiB,
  ~33%) plus the decode pass. Falls back to the base64 JSON shape if GitHub
  ignores the Accept header.

SYNC_WATCHDOG_MS and the default attachments folder are deliberately untouched
(product decisions). The user-side workaround remains the gitignore overlay:
add `Files/` so the attachments are not pulled at all.

Follow-up not taken here: the watchdog's `controller.signal` is still not
threaded through runApply into getBlobBytes, so today the abort never actually
reaches an in-flight blob fetch. The new AbortError branch is the correct
handling once it is wired (and for any page-level abort); wiring it is a
separate change.

Test: syncApply.test.ts - 3 attachments where the 2nd fetch rejects with an
AbortError; asserts the 1st is already persisted and the error propagates.
Against the pre-change code it fails with "Received promise resolved instead
of rejected. Resolved to value: {created: 2, failed: 1, updated: 0}" - the old
code swallowed the cancellation and only wrote anything after the batch
settled.

Gate: npm run lint, npm run typecheck, npm test (262 suites, 3336 passed),
npm run build - all green.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Ndqko5Lm9khVfibZdxue5K
…times out

Commit 1 made the attachment apply resumable; it did not make it fit. Jon
rejected the gitignore workaround ("de thelo gitignore"), so all 83.3 MiB /
175 images of `Files/` must sync AND the sync must not time out.

Attachments were applied inside the watchdog-wrapped runSync, so the one part
of an apply that scales with vault size gated the parts that don't: notes,
conflicts and push all died with it when the 45s watchdog fired. They are now
fire-and-forget, the same shape the pull already uses for note shells.

- backgroundFill.ts: new `fillAttachmentsInBackground(classifications, onPhase)`
  next to `fillShellsInBackground`, same fire-and-forget contract - never
  rethrows, so a background fill can never fail the sync that started it.
  Resume across reloads needs no new state: syncPull classifies attachments off
  IDB (listAttachmentPaths + getAttachmentGitSha), so whatever the last fill
  banked is simply absent from the next pull's `attachmentCreated` set. The
  startup auto-pull is the resume, as the startup fillShellsInBackground
  kick-off is for note bodies.
- useGitHubSync.ts: `runApply` no longer awaits the attachment apply; it kicks
  the background fill and returns the queued count. Kicked from runApply rather
  than from each of the four call sites so no path can forget it. The sync
  (tree, notes, conflicts, push) again finishes inside SYNC_WATCHDOG_MS exactly
  as it did before attachments existed.
- useGitHubSync.ts: one `backgroundPhase` callback now shared by both fills
  (it replaces two copies of the same inline arrow). It only writes when the
  status is idle, so the status bar reads "Synced" for the notes while the
  images are still coming down and a background fill can never overwrite a real
  status or an error.
- syncApply.ts: `applyAttachmentClassifications` takes `{ signal, onPhase }`.
  Progress is reported per banked item as "Downloading images... (37 / 175)".
- github.ts: `getBlobBytes` takes an optional `signal`, threaded into
  githubFetch, which already tells a caller abort from its own timeout and
  propagates the former as an AbortError without retrying. This makes commit
  1's AbortError branch live: a newer pull aborts the fill in flight instead of
  racing it for the same paths, and what it already banked stays banked.
- The counts in the status line are now "queued" rather than "applied"
  (`downarrow 175 images`), since the fill outlives the message. The per-item
  progress line reports the real completion.

Push safety (verified by reading syncPush 3b, then asserted): a banked image
has localSha === the remote blob sha it was fetched from, so
`if (plan.remoteSha === plan.localSha) continue` skips it - a fill can never
cause a re-upload. An image the fill has not reached yet is simply absent from
IDB, and absence is never a delete (only an explicit tombstone is, section 3c).

SYNC_WATCHDOG_MS and the `Files` attachments default remain untouched.

Tests (4 new, all fail against the pre-change code for the right reason):
- useGitHubSync.test.ts "resolves to a terminal sync state while the attachment
  fill is still running" - the attachment apply never settles; the sync must
  still reach `ok`. This is the regression itself at the hook level.
- syncApply.test.ts "an aborted signal stops the batch and keeps what was
  banked" - also asserts the signal reached every getBlobBytes call.
- syncApply.test.ts "reports progress as it banks each image".
- attachmentSyncTimeoutRetry.test.ts "a background-fetched attachment is not
  re-uploaded, while a genuinely local one still is".

Gate: npm run lint, npm run typecheck, npm test (262 suites, 3340 passed),
npm run build - all green.

Known gap, needs a product call: with `autoSyncOnStart` off (default is on), a
half-filled attachment set resumes only on the user's next manual sync, whereas
note shells resume on boot unconditionally. Closing it needs either the
outstanding classification list persisted or an unconditional tree fetch on
boot for a user who turned auto-sync off.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Ndqko5Lm9khVfibZdxue5K
@thetechjon

Copy link
Copy Markdown
Collaborator Author

Second commit pushed: 5f19443 — the gitignore workaround is out, so this makes the sync fit instead of shrinking the work.

What changed. Attachments were applied inside the watchdog-wrapped runSync, so the one part of an apply that scales with vault size gated the parts that don't — notes, conflicts and push all died with it when the 45s watchdog fired. They now stream in behind the sync, reusing the fire-and-forget shape the pull already had for note shells:

  • backgroundFill.ts — new fillAttachmentsInBackground() beside fillShellsInBackground. Never rethrows, so a fill can't fail the sync that started it. Resume across reloads needs no new state: syncPull classifies attachments off IDB, so whatever the last fill banked is simply absent from the next pull's attachmentCreated set — the startup auto-pull is the resume.
  • useGitHubSync.tsrunApply kicks the fill and returns the queued count instead of awaiting it. Kicked from runApply rather than from each of the four call sites so no path can forget it. One shared backgroundPhase callback replaces two copies of the same inline arrow; it only writes when the status is idle, so the bar reads "Synced" for the notes while the images are still coming down.
  • syncApply.ts / github.ts{ signal, onPhase } threaded down into getBlobBytes. Progress reports per banked item as Downloading images… (37 / 175). This also makes commit 1's AbortError branch live: a newer pull aborts the fill in flight instead of racing it for the same paths, and what it already banked stays banked.

SYNC_WATCHDOG_MS and the Files default are untouched.

Push safety — verified by reading syncPush 3b, then asserted. A banked image has localSha === the remote blob sha it came from, so if (plan.remoteSha === plan.localSha) continue skips it: a fill can never cause a re-upload. An image the fill hasn't reached yet is just absent from IDB, and absence is never a delete (only an explicit tombstone is, 3c).

Tests (4 new, all red against the pre-change code for the right reason):

  • useGitHubSync.test.tsresolves to a terminal sync state while the attachment fill is still running (the attachment apply never settles; the sync must still reach ok). The regression itself, at the hook level.
  • syncApply.test.tsan aborted signal stops the batch and keeps what was banked (also asserts the signal reached every getBlobBytes call).
  • syncApply.test.tsreports progress as it banks each image.
  • attachmentSyncTimeoutRetry.test.tsa background-fetched attachment is not re-uploaded, while a genuinely local one still is.

Gate green: lint, typecheck, npm test (262 suites, 3340 passed), build.

One gap that needs your call: with autoSyncOnStart off (default is on), a half-filled attachment set resumes only on the next manual sync, whereas note shells resume on boot unconditionally. Closing it needs either the outstanding classification list persisted, or an unconditional tree fetch on boot for a user who deliberately turned auto-sync off — that's a product decision, so I left it.

@thetechjon
thetechjon merged commit 698861a into dev Sep 8, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant