Skip to content

fix: retry attachment uploads and heal content missing from storage - #131

Open
petergaultney wants to merge 1 commit into
No-Instructions:mainfrom
TrilliantHealth:upstream/attachment-upload-reliability
Open

fix: retry attachment uploads and heal content missing from storage#131
petergaultney wants to merge 1 commit into
No-Instructions:mainfrom
TrilliantHealth:upstream/attachment-upload-reliability

Conversation

@petergaultney

Copy link
Copy Markdown
Contributor

🍋:

An attachment whose upload PUT dies without an HTTP response is abandoned permanently, while its
metadata syncs to everyone — so every client and any git-sync ends up chasing a content hash that
exists nowhere in storage. We found six of these in one vault; the oldest had been broken for three
weeks and had never been retried across many restarts.

Why it is permanent

An attachment syncs in two decoupled halves. The metadata (path + sha256) rides the folder doc over
CRDT sync and always converges. The content is a client PUT to storage. When that PUT dies with no
response at all (network drop, CORS, connection reset), S3Error has no code or status to match, so
it falls back to the generic "Attachment storage request failed." and the failure is classified
terminal. The background queue only re-drives retryable failures, so nothing ever tries again —
not on the next sync, not after a restart. The metadata is already published, so the reference is
fleet-wide and permanent.

Changes

  • CAS.writeFile gains the withTransientRetry wrapper readFile already had.
  • Any non-abort transfer failure with no HTTP response is retryable
    (s3TransferFailureFromUnknown). DOMException AbortError and abort-like messages stay terminal.
  • Existing holes heal. Each attachment version is HEAD-verified against storage at most once
    per machine, bounded by a durable guid -> hash ledger (casVerified); successful pushes and
    pulls record for free. A machine whose local content is the missing version re-uploads it; a
    machine holding a different version surfaces an error instead of clobbering the claim, since the
    real bytes may still exist on the author's machine.
  • sweepUnverifiedCasContent enqueues that verification after each syncFileTree. Without it the
    verify never runs for files at rest: the tree sync noops any file whose local state matches
    committed metadata, and SyncFile.sync() otherwise only fires on local modify events and
    in-memory retry queues.
  • 404 now reads "Attachment content not found in storage" instead of the generic fallback.

On deleting enableVerifyUploads

Worth a maintainer's judgement, so stating the reasoning rather than burying it. The flag is deleted
rather than defaulted on, because a flag default cannot ship a fix to existing installs: the plugin
materializes every flag into data.json on save, so an install that has ever saved settings never
consults the schema default. The flag reads false on every such machine (it defaults false
upstream), and gating the heal on it makes the fix inert for exactly the people who need it. We
learned this the hard way — our first build gated on a default-true flag and did nothing.

Separately, the path that flag guarded called push() without force, whose hash-unchanged guard
skipped the upload. So the verify feature was a no-op precisely for the author-machine case it
exists for. That is now push(true).

Verification

Manufactured a hole against real storage (uploaded an attachment, deleted the object, left the
metadata), then reloaded: the sweep found and re-uploaded it within ~30s with no user action. Then
ran it for real — the two authoring machines of the six broken attachments upgraded, and all six
missing versions reappeared in storage within minutes; every client can download them again.

11 tests in tests-th/. They pass on our fork but cannot run in a clean upstream checkout:
jest loads __tests__/mocks/globalStubs.ts, which is git-crypt ciphertext without the key
(TS1490: File appears to be binary), so every suite fails to start regardless of content. That is
also why they are not under __tests__/ — a file there would be unreadable in the diff. Please move
them wherever you prefer.

Running as 0.8.9-th.5/th.6 on our internal fork across ~35 machines.

An attachment syncs in two decoupled halves: the metadata (path + sha256)
rides the folder doc over CRDT sync and always converges, while the content
is a client PUT to storage. When that PUT died with no HTTP response, the
classifier had no code or status to match, fell back to the generic message,
and treated the failure as terminal - the background queue only re-drives
retryable failures, so the upload was abandoned permanently. The metadata
landed anyway, leaving every other client and any git-sync chasing a hash
that exists nowhere. We found six such attachments in one vault, the oldest
three weeks old and never retried across many restarts.

- CAS.writeFile gains the withTransientRetry wrapper readFile already had.
- Any non-abort transfer failure with no HTTP response is retryable
  (s3TransferFailureFromUnknown); DOMException AbortError and abort-like
  messages stay terminal.
- Existing holes heal: each attachment version is HEAD-verified against
  storage once per machine, bounded by a durable guid -> hash ledger
  (casVerified), and a machine whose local content IS the missing version
  re-uploads it. A machine holding a different version surfaces an error
  instead of clobbering the claim - the bytes may still exist on the author's
  machine. Successful pushes and pulls record for free.
- sweepUnverifiedCasContent enqueues the verification after each
  syncFileTree. Without it the verify never runs for files at rest: the tree
  sync noops any file whose local state matches committed metadata, and
  SyncFile.sync() otherwise only fires on local modify events and in-memory
  retry queues.
- 404 now reads 'Attachment content not found in storage' rather than the
  generic 'Attachment storage request failed.'

enableVerifyUploads is deleted rather than defaulted on. The plugin
materializes every flag into data.json on save, so an existing install never
consults the schema default - the flag reads false on every machine that has
saved settings, and a fix gated on it does nothing for them. The verify path
it guarded also called push() without force, whose hash-unchanged guard
skipped the upload, so the feature was a no-op for exactly the author-machine
case it existed for; that is now push(true).
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