Promote dev → prod: background attachment sync + audit gate - #320
Merged
Merged
Conversation
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
… background (beta) fix(sync): bank each attachment as it arrives, not after the whole batch
Brings sharp 0.35.4 / js-yaml 4.3.2 (#319) onto dev so the next promote does not reintroduce the failing npm audit gate. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Y9tnFMz9LARZ2rc6yy2EuG
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Promote of
devto production, requested by Jon 13/09.What ships:
1c1972efix(sync): bank each attachment as it arrives, not after the whole batch5f19443fix(sync): stream attachments in the background so a big vault never times out698861aMerge fix(sync): bank each attachment as it arrives, not after the whole batch #317 (beta) — the two abovedd7b86achore: sync main into dev (sharp 0.35.4 / js-yaml 4.3.2 + dependabot bumps already on main)devCI is green ondd7b86a(Lint · Typecheck · Test · Build, Collab server, E2E).npm audit --audit-level=highexits 0.Merge commit, not squash, so
devandmainstay in sync afterwards.🤖 Generated with Claude Code
https://claude.ai/code/session_01Y9tnFMz9LARZ2rc6yy2EuG