feat(attachments): offline attachment pipeline with on-device media cache - #107
Conversation
The offline attachment consumer (AttachmentPipeline + PushEngine) already shipped, but nothing produced its inputs: PendingAttachmentDao.enqueue had zero callers and no pending:<id> marker was ever written, so an offline pick silently failed. Wire the producer + preview: - Pick time: copy the picked file to a durable app dir (mform_attachments/) first (survives cache reclaim / camera-process kill), then upload inline when online, else keep the local path for save-time queueing (resolvePickedAttachment). Threaded via isOnline. - Save time: LocalWriter.writeParentInTxn scans Attach/Attach Image/Image fields on the parent and every child row, enqueues a pending_attachments row with correct coords (child field -> parentUuid=childUuid, topParentUuid=parent), and rewrites the stored value to pending:<id>. Idempotent per (parent_uuid, parent_fieldname). - Preview: attachmentDisplaySource resolves pending:<id> -> its durable local file (display only; stored marker unchanged) via a per-doc id->path map loaded in FormScreen on load/reopen/resume/after-save. Server URLs and post-sync file_urls render unchanged via Image.network. - preserveChildIdentity carries child mobile_uuid/name across the child edit sheet so queued child attachments don't orphan on re-save. - Delete the durable copy after a successful upload. isLocalAttachmentPath excludes pending: (re-save safety). Threads isOnline + pendingAttachmentPaths through FormScreen -> FormBuilder -> FieldFactory -> Attach/Image fields (parent and nested child-table builders). Full SDK suite green (1745 tests); producer verified on-device. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Catch-up merge: this branch carried one commit from 24 Jul, om/bug_fix had 93 commits on top of the same base. Two conflicts, both in the attachment field widgets, and both the same shape — om/bug_fix hardened the pick handlers while this branch replaced their core with the offline-aware resolvePickedAttachment(). Kept both halves in each case. * attach_field.dart / image_field.dart — took om/bug_fix's structure (try/catch around the picker, which throws on a denied storage permission; messenger captured before the await; sdkLog plus a release-visible SnackBar) and dropped resolvePickedAttachment() in place of its inline uploadFile! flow. Imports and widget fields were a union: isOnline / pendingAttachmentPaths from here, httpClient from om/bug_fix. This deliberately reverses one of om/bug_fix's rules. It stored NOTHING on a failed or empty upload — "never store the local path, the server expects a file_url" — and warned the user. Under the offline pipeline a durable local path IS a valid stored value: the save-time producer queues it and rewrites the field once the upload lands. So the null-uploader, failed-upload and empty-URL branches all now fall back to the durable copy and none of them notifies. The warning survives only where the durable copy itself failed, which is the one case with nothing to queue. Both replaced comments are quoted at the new call sites so the reversal is legible rather than looking like a dropped fix. * _AttachViewButton.fileName now resolves from displaySource rather than the raw field value, so a `pending:<id>` marker shows the real filename. Auto-merged but checked by hand, since a stale call would have compiled clean: form_builder.dart still passes parentData: and defaultOnError: to DependsOnEvaluator.evaluate (the merged evaluator is om/bug_fix's interpreter, whose new params are both optional — taking this branch's older two-arg call would silently have reinstated the permanently-mandatory bug). field_factory.dart carries both sides' new state: capDataLength / errorTextResolver from om/bug_fix, isOnline / pendingAttachmentPaths from here, still forwarded to the attachment fields. Verified: flutter test 2117 passing, against 1745 on this branch before the merge and 2090 on om/bug_fix — the +27 is this branch's attachment suite, so nothing was dropped from either side. flutter analyze clean over lib and test. No lockfile churn beyond what om/bug_fix itself carries.
…line media store
A `pending:<id>` marker could be written into Frappe verbatim as an
attach-field value. The marker lived in docs__<doctype> and nothing ever
rewrote that column, while PayloadAssembler rebuilds the payload from
docs__ on every dispatch — so resolution depended entirely on the pending
query returning the row, and that query selected only pending/uploading.
Once a row left those states the marker resolved to nothing and was sent
as-is. Three ways in, all reproduced against the real save -> upload ->
assemble path:
1. upload succeeded, parent POST failed, user retried
2. upload terminally refused, row blocked, user retried — this path
reported SUCCESS
3. any TimestampMismatchError, via _autoMergeAndRetry, no user action
A server_file_url committed to SQLite is now the correctness boundary. The
resolved url is written back into docs__ in the same transaction as the
state transition, so assembly reads an ordinary file_url. Upload and
doc-push became independent commitments: once committed the file is
uploaded permanently, and no later attempt re-uploads it. A document may
only push when every attachment is done; anything else throws
BlockedByUpstream naming the file and field. inlinePayload throws on an
unresolved marker so a regression is loud rather than silent.
Also fixed:
- Staged files leaked on all three document-removal paths and on re-pick;
nothing else reclaims outbox/, so a discarded survey lost its photos to
dead disk permanently.
- upload_file was called with dt/dn/filename. Frappe reads
doctype/docname/file_name — verified against 16.25.0, 16.26.3 and
17.0.0-dev. Unreachable in-tree, but a trap for the next caller.
- Every upload landed server-side named after an internal uuid. Staging now
keeps the user's filename with uniqueness in the parent directory, and
the multipart part name carries it — that part name, not
form_dict.file_name, is what Frappe actually stores.
- The media store survived logout, leaving one user's private survey photos
readable on a shared device after the next sign-in.
- pending_attachments.size_bytes and .mime_type were never populated.
- onCreate wrote a hardcoded schema_version that had drifted from _version.
Added: a media_cache content store keyed by file_url (Frappe dedupes by
content hash, so per-document keying would double-store shared bytes and
make deletion a refcount problem); offline previews, with uploads becoming
their own cache entry and pulled media cached on first view; a rejected
state so a permanently unuploadable file blocks with a reason instead of
retrying forever; a pick-time size guard; mediaStoreSize/clearMediaCache.
Breaking: uploadPendingForTopParent -> resolveForTopParent,
AttachmentPipeline requires db, findPendingForTopParent ->
findUnresolvedForTopParent, resolvePickedAttachment rethrows terminal
failures, schema v6 -> v7 (automatic).
Not verified on a device: the platform pickers, camera lost-capture
recovery, OpenFilex handoff, and the real logout wipe. Eviction is not in
this release, so media grows until logout or an explicit clear.
2209 tests pass; analyzer clean.
Adds doc/release-2.0/attachments.md covering the full lifecycle from a camera tap to a linked Frappe File record and back onto the screen, with six Mermaid diagrams: the end-to-end sequence, the resolution mechanism, the attachment state machine, the stock relink decision tree, the parent/child File-linking topology, and the preview resolution path. The relinking section is the substantive part, because the upload happens BEFORE the document exists. It traces all three things that have to converge — the bytes (a File record), the reference (a docfield value) and the ownership link (File.attached_to_*) — and why the SDK uploads with no doctype/docname at all. Grounded in the real Frappe source rather than described loosely, which surfaced three details worth writing down: - attach_files_to_document iterates Attach and Attach Image ONLY, so a url stored directly in an Image field gets no stock relink. - is_private is recomputed from the url prefix during relink and overrides what was uploaded — the url wins. - when the unattached row was already claimed, Frappe inserts a SECOND File row for the same file_url; bytes stay deduped, records do not. Also documents why top_parent_uuid is what makes parent and child-row attachments one operation, and states plainly what the SDK guarantees versus what depends on mobile_control's relink_mobile_files being present. Indexed from the release README's TL;DR, folder tree and reading table.
Covers N parent attach fields plus X child rows carrying M each — the realistic survey shape, and the case the suite had no coverage for. Uses distinct bytes per field so a mis-keyed writeback surfaces as the wrong url in a slot instead of passing by coincidence. Pins the behaviour that was previously only implied by the code: - One top-parent query finds all N + (X*M) rows at any nesting depth, while each writeback still targets its own row — a child's field updates docs__order_item WHERE mobile_uuid = <child>, not the parent's table. - Uploads are serial, so the first failure aborts the pass. Everything before it stays done with its column written back (progress is not thrown away), everything after stays pending and untouched. - The next dispatch finishes the set and re-uploads nothing that already landed, so total uploads are attempts-on-the-failure plus one per attachment that had not yet succeeded — never one per attachment per dispatch. - One terminal rejection out of the set blocks every later dispatch, even with a working uploader. doc/release-2.0/attachments.md gains the matching section, including the identity split between parent_uuid and top_parent_uuid, the serial-failure diagram, the cost characteristics, and what happens when the same bytes are attached to several fields.
The doc named 3 of the 17 test files that guard this pipeline, so a reader changing any of it had no way to know what would catch them. Adds a table of all of them with what each one is protecting. Also states plainly what has NO coverage in this repo and therefore has to be checked on hardware: the platform pickers, camera lost-capture recovery, OpenFilex handoff of a cached file, real path_provider paths, and whether mform_attachments/ is actually gone after a logout. The Frappe-side relink contract is verified against three checkouts' source, not a running site. Every file path in the doc is verified to exist.
…source The previous version described relinking from the SDK's own code comments. Reading mobile_control/attachment_relink.py and Frappe's attach_files_to_document directly corrected two claims and added the part that actually answers "how is each field identified". file_url is the correlation token, not any device identifier. parent_uuid and top_parent_uuid route the local writeback only and never cross the wire for relink purposes; the server re-derives every coordinate by walking the saved document and reading what each attach field holds. Three properties follow, and they are why N parent fields and X child rows with M each all land correctly: child identity never has to survive the round trip (Frappe assigns child names at insert, and the hook reads them after), order and reordering are irrelevant because matching is by value, and shared bytes still resolve one-to-one because each upload produces its own File row and the hook claims one per field. Corrections: - Stock inserts a NEW File when no unattached row remains, and recomputes is_private from the url prefix. mobile_control's child hook does neither — it skips, leaving the row unlinked, and leaves is_private alone. Both differences are now tabulated. - The earlier "one File record per field because stock inserts a fresh one for the rest" was wrong about the mechanism. One upload always produces one File row even when bytes are deduped, which is precisely what lets each field claim its own. Also documents the failure arithmetic: the hook claims one File per attach field it walks, so fewer unattached rows than referencing fields leaves the surplus unlinked — which is what the push gate exists to prevent.
… keys on the uuid triple, not file_url The previous text led with "file_url is the correlation token, not the device's uuids" under a heading about how fields are identified. That is wrong for the push pipeline and collapsed two separate stages into one key. Identification happens twice: Stage 1, on device, is entirely the uuid triple plus the fieldname, and file_url is the VALUE written rather than any kind of key: top_parent_uuid -> the set of attachments for one outbox row parent_doctype -> which table (docs__order vs docs__order_item) parent_uuid -> which row, matched against mobile_uuid parent_fieldname -> which column Marker resolution in inlinePayload is likewise by row id, not by url. Also worth stating: parent_uuid matches mobile_uuid, not `parent` or a server name, which is what lets the writeback run before the document has ever reached the server. Stage 2 is the server's post-save relink hook. None of those four columns cross the wire, so the hook re-derives the coordinate by walking the saved document and matching each field's value against File.file_url. That is the only place file_url is a matching key. Adds a per-field diagram of the stage-1 fan-out for N parent fields and X child rows with M each.
…rences A full-flow verification pass turned up one inaccurate claim and two dangling references. No behaviour changes. kDefaultMaxAttachmentBytes was documented as "host-overridable". It is not: it is the default for resolvePickedAttachment's maxBytes, and the only callers are this SDK's own AttachField / ImageField, neither of which exposes an override. A deployment whose System Settings max_file_size differs cannot change it without threading a parameter through FormScreen -> FormBuilder -> FieldFactory. Both the dartdoc and the release doc's limitations section now say so, including what the consequence is in each direction — refusing files the server would accept, or accepting ones it rejects, which the pipeline already handles as a terminal rejection. Stale references cleared: a dartdoc pointing at the renamed findPendingForTopParent, and a test name still saying uploadPendingForTopParent.
…che claim Two problems, both framing rather than behaviour. 1. The doc read as though this were a sync/push feature. It is not — the work spans pick, save, push, read/preview, delete/discard and logout, and only one of those is the push pipeline. Adds a table of all six plus the corollary that matters: for a fully-ONLINE user the push pipeline's attachment half is inert. An online pick stores a real file_url, isLocalAttachmentPath excludes /files/, so no pending row is ever created and the gate passes with nothing to resolve. The corruption this release fixes could therefore only ever reach an attachment picked offline or one whose inline upload failed transiently — which is also why it survived so long. The read path is the mirror image: independent of push, and it runs for every document including pulled ones this device never created. 2. The CHANGELOG claimed "media this device uploaded becomes its own cache entry". That is true of the PUSH path only. resolvePickedAttachment deletes the staged copy after a successful inline upload instead of moving it into cache/, so a photo taken while online is uploaded and its local bytes discarded — previewing it later costs a download. Verified by driving resolvePickedAttachment with injected fakes and asserting the delete. Corrected in the CHANGELOG and recorded in the doc's limitations, where it is noted as a small change in the pick path rather than a design change: the bytes, the url and the store are all already in hand there.
…ectivity A pick was decided by connectivity alone, so with offline-first mode ON and the device connected the attachment uploaded inline during data entry. Two problems with that. It breaks the offline-first promise: the enumerator waits for a round trip before the field populates, which is the experience offline mode exists to prevent. Worse, it put the attachment OUTSIDE the offline pipeline entirely. An inline upload stores a real file_url, isLocalAttachmentPath excludes /files/, so queueIfLocalAttachment never fires and no pending_attachments row is created — no push gate, no rejected state, no cache entry, and no record anywhere that the upload happened. A draft discarded afterwards left an orphaned File on the server that the SDK could not even name, which is strictly worse than the delete/re-pick orphans, where at least a row exists. The document was queued while its attachment was already on the wire. resolvePickedAttachment gains offlineModeEnabled, and AttachField / ImageField / FrappeFormBuilder / FieldFactory gain an isOfflineMode callback, read live off the repository so a mid-session toggle flip takes effect on the next pick. Both default to "not offline mode", so a host that wires neither keeps the previous inline-upload behaviour. Under offline mode the document and its attachments now travel together, which is the property the push gate depends on. The size guard still runs before staging in both modes. Covered by five pick-path tests for the mode/connectivity matrix plus an end-to-end one asserting the row lands in `pending`, carries the top parent uuid, the original filename and the size, and that the column holds a marker — i.e. the attachment is genuinely back under the gate.
MediaResolver treated any non-marker value as a server reference. Between pick and save the field holds a raw staged path, so resolving it prepended the base url and issued a guaranteed 404 before falling back. The preview still rendered, because the null result falls through to the value itself, so this was invisible — but it meant a wasted request per un-saved pick, and in offline mode it was exactly the network call the mode exists to avoid. Deferring the upload at pick and then hitting the network at preview would have left the offline-first promise half kept. isLocalAttachmentPath already draws this line for the save path — it excludes /files/, /private/files/, /api/method/, http(s):// and pending: — so the resolver now uses the same classifier and returns a device path as-is when the file exists, null when it does not, and never fetches either way. A cache path is local by the same test and is likewise returned directly. Four tests, including one asserting the check does not over-match: a real /files/ url must still be fetched.
… losing them Two problems, one of them data loss already on this branch. clearMediaCache() delegated to a whole-store wipe, so a host wiring an obvious "Clear cached media" button silently destroyed every staged file — the only copy of an attachment that had not uploaded yet. The storage split makes the correct behaviour unambiguous: outbox/ is correctness storage, cache/ is a performance copy of server media that is always re-fetchable. It now clears cache/ and the media_cache index only and cannot touch outbox/ or pending_attachments. clearAll is unchanged and remains correct for logout and wipe, where clearing staged files is intended — it is now that path's only caller. Separately, staged files leaked whenever a pick never reached a save. Staging happens at pick time, but the only thing that deleted a staged file was keyed to a pending_attachments row, which is not created until save. So picking twice without saving left both files on disk, and abandoning or crashing out of a form stranded one — in outbox/, which is deliberately never evicted, so nothing short of a logout ever reclaimed them. The orphan rule is exact in all three clauses: inside outbox/, no pending_attachments row, and not in the session live-set. The first stops the SDK deleting a host-supplied gallery path, the second protects queued work, and the third protects a pick that is live in an open form — which has no row yet and would otherwise look exactly like an orphan. There is no age heuristic anywhere, so nothing is ever deleted on a guess. Two mechanisms use it. Replacing a field value deletes the file it replaces, immediately. sweepOrphanedMedia() reclaims what nothing can catch at the time: abandoned and crashed forms. mediaStoreUsage() reports staged, cached and reclaimable bytes so a host can show "1.4 GB — 240 MB reclaimable" rather than offering to free an unknown amount; orphanBytes is a subset of outboxBytes and excluded from totalBytes. Both reclaim APIs are now non-destructive by construction. The sweep never throws, and deletes nothing when the referenced-set query fails, because an empty result must never be mistaken for "everything is an orphan". isStagedPath uses canonical containment rather than a string prefix: a prefix match admits ../ escapes and a sibling outbox_old/, and this guard is the only thing standing between the SDK and a user's gallery. mediaStoreSize() is replaced by mediaStoreUsage(). Both are part of this same unreleased release, so nothing has shipped against the old name. Known and documented rather than silently left: cache orphans (a file in cache/ with no index row, from a crash between writing and indexing) are reclaimed by neither the sweep nor eviction, and are folded into Phase 2. The sweep is host-triggered only; nothing runs on a timer or at startup.
An attachment could previously only be replaced, never removed — even on an optional field. Both attach fields now show a remove control whenever there is a value and the field is editable. A mandatory field can still be cleared; requiredValidator catches it at save, which is the right place, and blocking the clear would trap a user who wants to replace via discard-then-pick. Adding that surfaced three defects the UI alone could not have fixed. The writeback was unconditional on the column's current value, so an upload already in flight when the user discarded would complete and rewrite its url into the field afterwards — restoring a removed attachment, or letting an old upload claim a new pick's slot. It is now conditional on the column still holding that row's pending:<id> marker. Same class of defect as the original marker bug: a write that assumes nothing changed underneath it. Clearing a field left its queued row alive with the column emptied, and the push gate would then block that document indefinitely on an attachment nothing referenced. LocalWriter now drops the row and its staged file when an attach field arrives empty, scoped to null/empty only — a synced field holds /files/... and its done row is the writeback backstop. And making an explicit clear representable broke a value arriving AFTER the first build: initialValue applies once and a field's key is stable, so an asynchronously loaded document left the field blank. hasInteractedByUser is the only signal that separates "the user cleared this" from "never touched"; both cases are now pinned by tests, because fixing either one naively re-breaks the other. The field also clears BEFORE the bytes are reclaimed. A failed reclaim leaves an orphan the sweep collects; a failure aborting the clear would leave the attachment in place while the user believes it is gone. ImagePickSource lets the host offer gallery, camera or both. Global across doctypes and fields and read live, so it can be flipped from a setting without rebuilding; null means both, so existing hosts are unaffected. camera removes the gallery route entirely, which is the point when a fresh capture is required. Attach is unaffected — the system file picker has no gallery/camera distinction. onDiscarded was dropped before it shipped: it was never threaded to any host, and onChanged(null) already carries the same signal.
23 commits: OAuth refresh classification, depends_on onError threading and the extractEvalDocField fieldname gate, permission-denial query batching, the format-check CI fix, and 2.0.0-beta.2. Four conflicts, each resolved on its merits rather than by picking a side: - attachment_pipeline.dart — kept this branch. Their only change to the file was a dart-format rewrap of a return statement; the rewritten pipeline supersedes it and nothing of theirs is lost. - attachment_pipeline_test.dart — kept this branch. `db:` is a required parameter of the rewritten pipeline, so dropping it would not compile. - frappe_sdk.dart — took THEIRS. Their change is a real doc comment explaining why sessionHealth is deliberately not disposed; this branch had only a format rewrap there. - CHANGELOG.md — kept both. Each side appended to the same sections. 2315 tests pass, analyzer clean.
…eBytes The pick-source hook added in 63592f1 could not be wired by any host. `FormScreen.imagePickSource` takes a callback that must PRODUCE an `ImagePickSource`, and neither the enum nor its allowsGallery/allowsCamera extension was exported from the public entry point — so a host had no way to name the value it was being asked to supply, short of an `implementation_imports` violation. Every layer inside the package was wired correctly (FormScreen -> FrappeFormBuilder -> FieldFactory -> ImageField, on both the parent and child-table paths); the feature was complete internally and unreachable externally, which is the same shape as an implementation with no callers. `MediaStoreUsage`, the return type of `FrappeSDK.mediaStoreUsage()`, was in the same position but milder: inference still permitted `usage.orphanBytes`, but the type could not be written in a field, a return type, or a variable declaration. No existing test could have caught either, because every one of them imports `src/` directly — exactly what a host may not do. `public_api_surface_test.dart` imports ONLY the public barrel and fails to compile if either export is dropped. Also here, all found while verifying the above: - `test/ui/form/child_uuid_roundtrip_test.dart` failed the format gate. It came from df19398, while the sweep in 73a6c3c ran on om/bug_fix where that file did not exist — so the merge joined a now-real gate to a file the sweep never saw. One 81-column line. - `MediaStore.storeSizeBytes()` lost its last caller in the mediaStoreSize -> mediaStoreUsage rename. `MediaStore.usage().totalBytes` is the same number by definition (outboxBytes + cacheBytes). Deleted; its tests go through usage(). - `mediaStoreUsage()` carried the replaced method's docstring above its own, the two contradicting each other. - `RestHelper.uploadFile`'s comment still claimed the staged basename is a generated uuid. This branch changed exactly that: staging now keeps the user's filename and puts uniqueness in the generated parent directory. - `late_pending_paths_test.dart` pins the ordering the offline preview depends on. FormScreen loads the marker->path map AFTER the first build, and MediaResolveBuilder memoises its future and does not restart on a pendingPaths change, so nothing recovers the preview except the synchronously recomputed attachmentDisplaySource fallback. Correct on both fields, and until now untested — removing that fallback would have broken offline preview silently. flutter analyze clean, dart format gate clean, flutter test 2319/2319 pass.
- Introduced `resolvePulledMobileUuid` function to manage the precedence of local and incoming `mobile_uuid` values during data synchronization. - Updated the `PullApply` class to utilize the new function for determining the correct `mobile_uuid` for both parent and child rows. - Added comprehensive tests to ensure that non-empty incoming `mobile_uuid` values are adopted correctly, while empty or absent values result in new UUIDs being minted. - Ensured that existing local rows retain their UUIDs during re-pulls, maintaining data integrity across sync operations.
Omprakash-48
changed the title
CI's `dart format --set-exit-if-changed .` gate failed on this file only. Formatting-only change; `flutter test` still 2338 passing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…claims Three statements in CHANGELOG contradicted the branch they ship with: - "the attachment work ... is verified by tests and source review only — not on a device" predates the measured hardware run in attachments.md §16, and its test count (2209) predates the current 2338. - the Added entry for clearMediaCache still warned that it is destructive to staged picks; the fix in this same release scoped it to cache/ and the media_cache index, so only logout(clearDatabase: true) clears staged files. - two upgrader notes still named mediaStoreSize(), which the Changed section records as replaced by mediaStoreUsage(). Docs only. flutter test 2338 passing; dart format clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
deepak-dhwani
left a comment
deepak-dhwani
left a comment
There was a problem hiding this comment.
Round 1 — e665ddf · Request changes
The attachment pipeline is well-built: the commitment model is right, the crash-point ordering is
explicit, and the storage-lifetime rules are the most careful part of the change. The blocker is not
in the attachments — it is the mobile_uuid adoption that rides along with them.
Scope of this review, stated so it isn't overread. Static only — no Dart toolchain on my side, so
the reported 2338 passing tests are not reproduced. I read attachment_pipeline.dart,
push_engine.dart (the gate + writeback), pull_apply.dart, media_store.dart,
media_resolver.dart, attachment_paths.dart, attachment_error_classifier.dart,
pending_attachment_dao.dart, local_writer.dart's producer, the schema/migration, and the barrel.
I did not review image_field.dart (527 lines, the largest single file), attach_field.dart,
form_screen.dart, child_table_field.dart, the 38 test files, or attachments.md. The UI layer is
unreviewed and should not be read as cleared.
🔴 H1 — resolvePulledMobileUuid adopts an unvalidated server string into the PRIMARY KEY
The precedence (local → incoming → mint) is the right fix for the problem described. But mobile_uuid
is TEXT PRIMARY KEY on both parent and child mirrors (parent_schema.dart:23,
child_schema.dart:19), and adoption is unconditional on anything except emptiness:
// pull_apply.dart:64
String resolvePulledMobileUuid({...}) {
if (localUuid != null && localUuid.isNotEmpty) return localUuid;
final adopted = incoming?.toString();
if (adopted != null && adopted.isNotEmpty) return adopted; // ← no shape, no uniqueness
return uuidGen.v4();
}So a server-supplied value now becomes the local primary key. Two things follow, and they are
independent.
(a) Two server documents with the same mobile_uuid — different outcomes per path, one of them silent
The four write sites don't agree on conflict policy:
| Site | Path | Insert | Outcome on a duplicate adopted uuid |
|---|---|---|---|
:420 parent |
sequential | :462 batch.insert (default abort) |
batch.commit (:559, no continueOnError) throws — page apply fails |
:610 parent |
bulk | :645 ConflictAlgorithm.**replace** |
second row silently overwrites the first |
:530 child |
sequential | :555 default abort |
commit throws |
:667 child |
bulk (localUuid: null) |
:693 default abort |
commit throws (:698) |
The bulk row is the bad one. mobile_uuid is the PK and the two rows carry different server_names,
so replace means one server document's local mirror is destroyed and replaced by another's, with
no error anywhere. The document simply isn't on the device. Before this PR that was impossible by
construction — every pulled row minted its own v4.
The sequential and child sites fail loudly instead, but they fail permanently: the page throws on
every retry for as long as the server data stays that way.
The trigger is not exotic. Frappe's Desk Duplicate and Amend copy custom field values,
mobile_uuid included — so duplicating a mobile-created document in Desk produces a second document
with the same parent and child mobile_uuids. A Data Import with a mobile_uuid column does the
same. Then any fresh-install / post-logout(clearDatabase: true) / new-device pull — precisely the
scenario this fix targets, since that is when adoption fires at all — hits it.
existingUuids cannot catch this. It is keyed server_name IN (this page's names)
(:587-592), so it answers "what uuid does this server_name already have", never "is this uuid
already taken by a different one".
(b) It breaks an invariant the codebase states in prose and relies on for identity
uuid_rewriter.dart:73-80 says it outright:
Frappe server names are never UUID-shaped, and SDK
mobile_uuids always are — so it catches
local refs populated by paths that never fireonIsLocalChanged(fetch_from, defaults, programmatic
prefill, back-reference Links). Relying on the flag alone let those raw UUIDs reach the server,
which rejects them as unknown names.
Adopt HSFM-2026-00042 and looksLikeMobileUuid returns false, so the rewrite falls back to
__is_local alone — which that same comment documents as insufficient for four specific paths, with
the consequence spelled out. form_screen.dart and link_field.dart gate on the same proxy.
Fix
Gate adoption on shape and uniqueness — both are cheap, and either alone leaves half the hole:
String resolvePulledMobileUuid({
required String? localUuid,
required Object? incoming,
required Uuid uuidGen,
required bool Function(String candidate) isFree, // new
}) {
if (localUuid != null && localUuid.isNotEmpty) return localUuid;
final adopted = incoming?.toString().trim();
if (adopted != null &&
adopted.isNotEmpty &&
looksLikeMobileUuid(adopted) && // restores the uuid_rewriter invariant
isFree(adopted)) { // no OTHER local row holds it
return adopted;
}
return uuidGen.v4();
}isFree wants a real query — SELECT 1 FROM <table> WHERE mobile_uuid = ? excluding this
server_name — not a reverse scan of existingUuids, for the reason above. In the bulk path it can
be batched once per page against the incoming uuid set.
Separately: please make the two parent paths agree on conflict policy. ConflictAlgorithm.replace on
a primary key the server can now influence is a silent-overwrite primitive, and having one path abort
and the other replace means the same server data corrupts differently depending on which code path
ran.
Worth adding to pull_apply_mobile_uuid_adoption_test.dart: two server rows sharing a non-empty
mobile_uuid, asserted through both _applyPageInTxnBulk and _applyPageInTxnSequential, plus a
non-UUID-shaped incoming value asserted to mint rather than adopt.
🔴 H2 — ResolveMediaFn should be exported in this PR, not after it
The description already makes the case, better than I would: FieldFactory is exported bare
(frappe_mobile_sdk.dart:109) and documented as overridable, createField takes
ResolveMediaFn? mediaResolver (field_factory.dart:119), the typedef lives in
src/services/media_resolver.dart, Dart does not re-export a library's imports, and a real consumer
app that subclasses FieldFactory cannot compile without an implementation_imports violation.
Confirmed all of that.
What doesn't follow is leaving it. This PR adds two barrel exports for exactly this reason, in a
single hunk, and the missing line goes two lines below them:
export 'src/models/image_pick_source.dart' show ImagePickSource, ImagePickSourceHelpers;
export 'src/models/media_store_usage.dart' show MediaStoreUsage;
+ export 'src/services/media_resolver.dart' show ResolveMediaFn; // ← this PRThe description says "2.0.0 should not ship claiming that type is host-facing until it lands" — so
this is a known-broken public API being deferred one line short of fixed, in the file already being
edited. Please add it here and let public_api_surface_test.dart start guarding it now.
🟡 M1 — cachePathFor is called with and without sourcePath, so the index can name a file that was never written
// media_store.dart:140 — inside moveToCache, WITH the source
final dest = await cachePathFor(fileUrl, sourcePath: stagedPath);
// attachment_pipeline.dart:205 — recomputed, WITHOUT it
if (moved) cachedPath = await MediaStore.cachePathFor(fileUrl);cachePathFor borrows the extension from sourcePath when the url has none — its own docstring names
the case ("a download_file proxy url that carries the name in its query") and
media_store_test.dart:79 pins the branch. For such a url the bytes land at <digest>.jpg while
media_cache.local_path records <digest>.
Consequences: MediaResolver finds the row, File(hit.localPath).exists() is false, treats it as a
miss and re-downloads — every view, forever. And the uploaded original at <digest>.jpg becomes
unreachable bytes that no API reclaims: sweepOrphans walks outbox/ only (media_store.dart:296),
so clearCache() is the only thing that will ever remove it. media_resolver.dart:95 is
self-consistent, so this is the one divergent call site.
Fix: pass sourcePath: p.localPath at attachment_pipeline.dart:205 — or better, have
moveToCache return the destination it actually used, so the two can't drift again.
🟡 M2 — every 417 is terminal, which inverts the classifier's own stated policy
isTerminalAttachmentError sets out the principle and then contradicts it:
Defaults to FALSE for anything unrecognised. A wrongly-transient error costs one retry; a
wrongly-terminal one strands the user's file with no automatic recovery. Fail toward retrying.
if (error is ValidationException) return true; // ← every 417417 is not the oversized-file status; it is where every frappe.throw lands. A site storage-quota
throw, a custom File hook, any transient server-side business rule — all become rejected, which is
never auto-retried and blocks the parent document until the user re-picks the file. That is the exact
cost the docstring says to avoid, applied to the broadest error class Frappe has.
_terminalMarkers already names the two genuinely terminal cases, but it is only consulted on the
flattened-exception path at the bottom — after this branch has returned.
Fix: for ValidationException, consult _terminalMarkers against the message and default to
transient. That reuses the list that already exists and makes the behaviour match the doc.
🟡 M3 — the marker prefix has four spellings, two of them literals
kPendingMarkerPrefix exists (attachment_paths.dart:30) and is used at two of the four sites that
must agree byte-for-byte:
| Site | Spelling |
|---|---|
local_writer.dart:307 — the producer |
'pending:$id' literal |
attachment_pipeline.dart:319-320 — inlinePayload, the "loud not silent" backstop |
'pending:' literal |
attachment_paths.dart:19 — isLocalAttachmentPath's nonLocalPrefixes |
'pending:' literal |
attachment_pipeline.dart:251 — the writeback WHERE |
kPendingMarkerPrefix ✅ |
media_resolver.dart:62 — display resolution |
kPendingMarkerPrefix ✅ |
Change the constant and the two halves break in opposite directions, both quietly. The writeback
WHERE starts matching nothing. And MediaResolver stops recognising the marker → falls through to
isLocalAttachmentPath, whose literal still matches → returns not-local → the marker goes to the
cache/network path, and a local id is issued as an HTTP request — the one thing that function's
comment says must never happen ("Falling through to the network for a malformed marker would turn a
local id into a request path").
Nothing is wrong today. But this PR exists because markers reached the server, and the invariant that
prevents it is currently spread across four uncoordinated string literals. Using the constant at all
four sites costs nothing and makes the compiler enforce it.
🔵 Low
L1 — kDefaultSyncBackoff means a different attempt count in each of its two consumers.
attachment_pipeline.dart:154 is attempt < backoff.length → 3 attempts, with delays only after
attempts 0 and 1, so the third entry (10s) is never used as a delay. push_engine.dart:568 is
attempt <= networkBackoff.length → 4 attempts. The docstring says "3 attempts at 2s, 5s, 10s",
which is accurate for neither. Trimming the list to [2s, 5s] would silently drop the pipeline to two
attempts, because there the list length is the attempt count.
L2 — downloads are unbounded while picks are capped. A pick is size-guarded at 10 MB before
staging, but MediaFetchFn is Future<List<int>?> and MediaResolver.resolve buffers the whole body
before writeAsBytes. Nothing caps a Desk-uploaded attachment, and the verified device is Android
8.0.0 / API 26. Worth a cap on the resolve path, or a streaming fetch.
L3 — three copies of the fetch-URL logic. frappeFileFetchUrl's own NOTE records that
AttachField._fullFileUrl and ImageField._fullImageUrl still hold private copies, deferred to keep
the change small. Reasonable at the time, but /private/files/ has to route through download_file
to carry auth, so a drift between the three is a private-file 404. Now that the shared helper exists,
worth collapsing onto it.
✅ What holds up
- The commitment model. "A
server_file_urlcommitted to SQLite is the correctness boundary",
with the four steps numbered by crash point and the explicit note that the pre-commit window
degrades to a re-upload that Frappe's content-hash dedup absorbs. Upload and document push as
independent commitments is the right decomposition. - The conditional writeback.
WHERE mobile_uuid = ? AND "<field>" = 'pending:<id>'with the
reasoning written down — an unconditional update is the same mistake that produced the original
bug, and matching nothing when the user re-picked or discarded is the correct outcome. This is the
best single decision in the PR. isStagedPathuses canonical containment, not a prefix match.p.canonicalize+p.isWithin
rejectsoutbox_old/and resolves.., and the comment says exactly why it matters: callers use it
to decide whether to delete, and a host may point a field at/sdcard/DCIM/holiday.jpg. Getting
this wrong is unrecoverable; it is right, and it is right for a stated reason.- Register-before-create in
stageToOutbox. Adding to_stagedThisSessionbefore the file
exists closes a window where a sweep could see a file with neither a row nor a live-set entry. Also
the note that the set is empty after restart and those files are then genuinely orphaned — so they
become sweepable exactly when they should. deleteOutboxCopy's dirname guard. Only prunes a directory sitting directly underoutbox/, so
an arbitrary path can't delete something else.- Sweep guards are exact, not heuristic. No age rule, so a pick sitting in an open form is never at
risk; and the explicit contract that the caller must not pass an emptyreferencedPathswhen the
query failed, honoured insweepOrphanedMedia. - The
clearCache/clearAllboundary, and the logout fix behind it — the store surviving logout
and leaving one user's photos readable on a shared device is a real security bug, and the fix puts
the destructive path where it belongs. - Deadlock notes at both pre-resolve sites (
attachment_pipeline.dart:218,
local_writer.dart:232): querying the outerDatabasewhile holding a txn hangs on sqflite's
non-reentrant lock, and it's called out as a Dart deadlock rather thanSQLITE_BUSY. That
distinction saves the next person an afternoon. public_api_surface_test.dart— a test that imports only the public entry point, so a dropped
export fails to compile. That is the right shape of guard, and the observation that every other test
importssrc/directly is exactly why the missing exports were invisible.- The migration. Properly versioned v6→v7,
IF NOT EXISTSthroughout,ALTERwrapped for
re-entry,schema_versionwritten in the same transaction, and a re-entrancy test. - The verification section names what was not exercised on hardware — offline capture, offline
preview fromoutbox/,BlockedByUpstream, thefailed/rejectedstates, reclaim, camera
lost-capture,OpenFilexhandoff. And §16.2's EXIF-strip note (device cache holds the pre-strip
original, server holds the re-encode) is the kind of thing most PRs discover in production.
Gate
| Area | Status |
|---|---|
| Attachment commitment model | ✅ |
| Push gate / marker containment | ✅ |
| Writeback atomicity + conditionality | ✅ |
| Staged-file lifetime & reclaim | ✅ |
| Logout / wipe boundary | ✅ |
| Schema migration v6→v7 | ✅ |
| Cache index ↔ filesystem agreement | 🟡 M1 |
| Terminal/transient classification | 🟡 M2 |
| Marker constant discipline | 🟡 M3 |
mobile_uuid adoption |
🔴 H1 |
| Public API surface | 🔴 H2 |
UI layer (image_field, attach_field, form_screen) |
⚪ not reviewed |
Requesting changes on H1 and H2. H2 is one line in a file this PR already edits. H1 is the real
one, and I'd genuinely consider splitting it out: the attachment pipeline is independently verified,
well-tested and ready, while the mobile_uuid adoption is a change to primary-key identity that
deserves its own diff, its own collision tests, and its own bench run. It is already flagged as "also
in this PR (not attachments)" — that instinct was right, and the reason it should be separate is that
it is the only part of this change that can silently lose a document.
M1–M3 and L1–L3 are all small and can land in the same round.
Nine fixes from the round-1 review. Kept as one commit because attachment_pipeline.dart and CHANGELOG.md each carry several of them and non-interactive hunk splitting would risk mis-staging. Public API - Export ResolveMediaFn. FieldFactory is exported bare and documented "Override this method to customize field creation", and createField takes a ResolveMediaFn? — but the typedef lived in src/ and Dart does not re-export a library's imports, so overriding it required an implementation_imports violation. Same defect class as the ImagePickSource / MediaStoreUsage gap, missed by that fix. public_api_surface_test now fails to compile if the export is dropped. mobile_uuid identity - Gate PARENT adoption on UUID shape. looksLikeMobileUuid is what UuidRewriter calls "the complete detector" for a local Link reference, and PushEngine's dependency scan uses the same predicate to tier a row behind the row it points at; a non-UUID primary key is invisible to both. Children deliberately keep the pre-existing emptiness-only test — child adoption is not new and is load-bearing, since a Link can reference a child by mobile_uuid and gating it re-breaks the orphan-Link regression pinned by pull_apply_test. Uniqueness is not re-checked client-side: the column carries a real UNIQUE index server-side (a duplicate write fails with MariaDB 1062 on both parent and child tables). - Send each child's mobile_uuid in the push payload. The parent's was re-added after the system-column strip; children were stripped and never restored, so the server stored NULL and children had no identity across a round trip. Sent on INSERT and UPDATE; the child's own uuid, never the parent's (which would collide on that UNIQUE index); omitted when blank, because MariaDB permits many NULLs but not many empty strings. Verified end to end against a live v16 bench. Rows written before this keep NULL and match by position until their parent is next pushed. Media cache - Record the cache path moveToCache actually used. cachePathFor borrows the extension from the source when the url carries none, so recomputing without it named <digest> while the bytes landed at <digest><ext> — a media_cache row pointing at a file that was never written, which reads as a permanent cache miss and strands the bytes where sweepOrphans (outbox/ only) cannot reclaim them. moveToCache now returns String? so the two cannot drift. - Bound the download path. Picks were capped at 10 MB but downloads were unbounded on an API 26 floor. FormScreen's fetcher now streams and refuses on Content-Length before reading a chunk, re-checking mid-stream for chunked responses; MediaResolver.maxFetchBytes additionally keeps an oversized body out of the cache. The download ceiling is deliberately higher than the pick ceiling. Error classification - Classify 417 by the exception Frappe names, not by the status. Every ValidationException was terminal, which inverted this module's own "fail toward retrying" policy across Frappe's broadest error class — 417 is where all frappe.throw lands. The stated justification was also wrong: an oversized upload never reaches 417, because Frappe sets werkzeug's max_content_length to the same get_max_file_size() (app.py:194) and the HTTP layer rejects with 413 first (confirmed on a live bench with a 30 MB upload). exc_type is set unconditionally on a v1 error body and RestHelper hands the whole body to ValidationException.errors, so this is structured rather than a substring guess; the v2 errors[].type shape is read too. Consistency - Use kPendingMarkerPrefix at all five marker sites. Three were literals, so changing the constant would have broken the writeback and the display resolver in opposite directions, the latter issuing a marker as an HTTP request. - Collapse AttachField._fullFileUrl and ImageField._fullImageUrl onto frappeFileFetchUrl after verifying the three were logically identical. /private/files/ must route through download_file to carry auth, so a drift between copies was a private-file 404. The shared helper had no direct coverage; every branch is now pinned. - Correct kDefaultSyncBackoff's docstring, which claimed an attempt count neither consumer had. The loops differ legitimately and are left alone — making them agree would change push retry counts. Also retire a stale ThreeWayMerge note claiming mobile_control still had to ship mobile_uuid onto child doctypes; it already did. Full suite: 2364 passing. analyze clean.
|
Round 1 addressed — 2677711 All nine findings actioned. Suite: 2,364 passing, analyze clean. H1 — took the shape gate, skipped the uniqueness gate. mobile_uuid is unique: 1 with a real enforced index (a duplicate write fails with MariaDB ERROR 1062, parent and child), so two server docs can't share one and the replace path isn't reachable from adoption. Parent adoption now requires looksLikeMobileUuid — H1(b) was the real defect. Children deliberately excluded: pull_apply_test.dart's orphan-Link regression test uses a non-v4 uuid and asserts adoption, and children have always adopted any non-empty value. Gating them would re-break that. H2 — done. One line. Test fails to compile without it; the consumer app now builds against the barrel alone. M1 — done. moveToCache returns the destination it used, so the caller can't recompute it wrong. M2 — done, structurally rather than by substring. exc_type is available (response.py:52 sets it unconditionally; RestHelper passes the whole body through). Also: oversized never reaches 417 — werkzeug's max_content_length is the same get_max_file_size() (app.py:194), so a 30 MB upload returns 413, already terminal via the 4xx rule. Dropping the blanket rule can't weaken it. M3 — done. All five sites on the constant. L1 — docstring only. Both loops are correct for their own purpose; only the shared comment fit neither. Left the loops alone. L2 — bounded in two places. You're right a cap in resolve() doesn't fix the OOM, so the fetcher now streams and refuses on Content-Length. The resolver cap keeps oversized bodies out of the cache. L3 — collapsed, after diffing all three and pinning every branch of frappeFileFetchUrl first (it had no coverage). Also here: child mobile_uuid now ships on the wire, closing the round trip. Verified against a live bench. Two heads-ups: L2/L3 touch form_screen.dart / attach_field.dart / image_field.dart — the layer you didn't review. And H2 must land with the consumer app change; it currently resolves the SDK from a different ref. |
`dart format --output=none --set-exit-if-changed .` failed CI on the nine files touched by 2677711. Formatting only — no behaviour change, and the suite still reports 2364 passing.
deepak-dhwani
left a comment
deepak-dhwani
left a comment
There was a problem hiding this comment.
Two new commits (2677711, 982a5f7). All nine round-1 items verified fixed in code.
grep -rn "'pending:" lib/ now returns exactly one hit — the constant's own definition.
Two corrections to my round 1
M2 — you corrected my premise. I accepted the docstring's claim that 417 is how an oversized upload arrives and only argued about breadth. You checked: Frappe sets werkzeug's max_content_length to the same get_max_file_size() (app.py:194), so the HTTP layer returns 413 first — verified with a 30 MB upload. The justification was wrong, not merely over-wide, and classifying on exc_type is better than the narrowing I asked for.
H1 — I was wrong to frame the uniqueness fix as "just add the check." The bulk path has no existing.isEmpty branch: ConflictAlgorithm.replace at :688-691 is its upsert, so it can't become abort, and replace can't distinguish a legitimate same-uuid update from a pathological collision. The only client-side discriminator is the per-row query you declined — so relying on the server-side index is the pragmatic call. My Duplicate/Amend trigger doesn't survive it either: with the index in place, Frappe's own Duplicate fails 1062 first.
The child asymmetry I was going to raise — withdrawn. Both child paths set server_name from cr['name'] (:582, :719), so the reachability mitigation extends to children; and the preservation fixture uses a non-v4 uuid because deployments exist with that shape, so gating would break real installs.
Your find beats mine
Child mobile_uuid never reaching the wire is bigger than anything I raised, and it retro-corrects my H1: with children round-tripping as NULL, the two child write sites in my table were inert in practice, not live. My finding was right about the parent paths and wrong to weight the child ones equally.
One residual — a sentence, not code
The CHANGELOG states the precondition (server-side UNIQUE index) but not the consequence if unmet: on the bulk path replace destroys one document's local mirror silently, while the sequential path aborts loudly. Since this SDK ships to pub.dev standalone and the guarantee is owned by mobile_control, a site provisioning mobile_uuid without unique: 1 gets silent single-document loss. Worth one line beside the existing note.
Why this isn't an approve yet
image_field.dart (527 lines) is still unread, along with child_table_field.dart, the 38 test files, and attachments.md. That's the pick / discard / re-pick / preview lifecycle and the hasInteractedByUser change — the part most likely to hold a widget-lifecycle bug and the part with the least review.
…bsent Round-2 review residual: both notes stated the server-side UNIQUE index as a precondition but not the consequence if it is unmet. The index is owned by `mobile_control` (its `_MOBILE_CUSTOM_FIELDS` declares `mobile_uuid` as `Data` with `"unique": 1`, for workspace doctypes and their children) while this is a standalone package dependency carrying no `publish_to: none` — so nothing here can enforce it, and a site provisioning the field without `unique: 1` needs to know what it gets. Verified against the real `PullApply.applyPage` with two rows of distinct `name` and one shared `mobile_uuid`, not reasoned from the source. Three outcomes, exactly one silent: * Bulk parent path (initial sync only) — silent. `ConflictAlgorithm.replace` drops the first document's mirror with no log and no error. The bulk safety pre-check does not prevent it: it counts locally-dirty and local-only rows, so a clean `synced` row is replaced without objection. Afterwards the two documents share one row and swap on every page carrying either — those swaps DO log, via the cross-device-clash tripwire, because the `mobile_uuid` fallback query now matches. The bulk page is the only unannounced event. * Sequential, both rows in one page — loud. The plain `INSERT`s are queued in `txn.batch()` and so invisible to that fallback query; the second raises `UNIQUE constraint failed: <table>.mobile_uuid` at `batch.commit`, which has no `continueOnError`. The page rolls back and both pull paths stop the doctype without advancing its cursor — `PullEngine` reports it failed mid-pull, `SyncService` raises a `SyncError` — so it re-fails every cycle. Reported, not lost. * Sequential, rows in different pages — neither. The fallback matches the first row, the tripwire logs, and the second document overwrites it in place. Children always take the loud path: the bulk child insert is a plain `INSERT`. Applied in both places because `resolvePulledMobileUuid`'s docstring carried the same unqualified "cannot present the same value" claim at the write site, and is what the next maintainer reads there. Comments only — no behaviour change. Full suite 2364/2364, analyze and format clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Commit ca69c95 → omprakash/feat/offline-attachment-pipeline (PR 107's head). Two files, comments only, no behaviour change: CHANGELOG.md:30 and resolvePulledMobileUuid's docstring in lib/src/sync/pull_apply.dart. Both now state what happens if the server-side mobile_uuid UNIQUE index is missing. What the verification changed — I probed the real code instead of reasoning about it, which corrected the reviewer once and me three times:
|
Round 3 —
|
| ID | Finding | Fix | |
|---|---|---|---|
| H1 | Unvalidated server string into the PK | parent shape gate; uniqueness declined with evidence | ✅ |
| H2 | ResolveMediaFn unexported |
barrel :38 + compile-time guard |
✅ |
| M1 | cachePathFor called with and without sourcePath |
moveToCache returns String? |
✅ |
| M2 | Every 417 terminal | classified on exc_type, not status |
✅ better |
| M3 | Four spellings of the marker prefix | constant at all five sites | ✅ |
| L1 | kDefaultSyncBackoff attempt count |
per-consumer table in the docstring | ✅ |
| L2 | Downloads unbounded | streamed, bounded twice | ✅ |
| L3 | Three copies of the fetch-URL logic | both private copies delegate | ✅ |
| R2 | UNIQUE-index precondition not stated | three outcomes enumerated and verified | ✅ better |
H2 — the guard test is the right shape: it annotates rather than infers, then assigns a
conforming closure, so it breaks both if the export is dropped and if the typedef drifts.
M1 — returning the destination beats threading sourcePath through. The two paths can no longer
disagree, which was the actual defect.
M3 — grep -rn "'pending:" lib/ now returns exactly one hit: the constant's own definition.
L1 — better than making the loops agree, for the reason you give: forcing agreement would change
push retry counts nobody asked for. The list genuinely is two contracts; documenting both is the fix.
L2 — Content-Length refusal before the first chunk, mid-stream re-check for chunked responses
that under-report, maxFetchBytes keeping an oversized body out of the cache, and
client?.close() in a finally. The docstring is honest that the second stage bounds the cache,
not the allocation. The 25 MB download ceiling being deliberately above the 10 MB pick ceiling is
right and justified where a reader will find it.
The round-2 residual — closed, and more thoroughly than asked
I asked for one sentence on the consequence of the UNIQUE precondition being unmet. What landed
enumerates three outcomes and marks which is silent, verified rather than reasoned. I checked the two
load-bearing claims:
- "the pre-check counts locally-dirty and local-only rows, so a clean
syncedrow is replaced
without objection" — confirmed. The bulk pre-check is a singleCOUNTover
sync_status IN (_locallyDirtyStatuses + 'deleted'), so asyncedrow is not counted. - "both pull paths stop the doctype without advancing its cursor … reported, not lost" —
confirmed atpull_engine.dart:423: "Mid-pull failure: do NOT persist cursor", and the
setLastOkCursorwrite at:406sits before it on the success path only.
Narrowing the silent window to the bulk page itself — because every later flip does log through the
clash tripwire once the fallback query starts matching — is a sharper statement than my finding was,
and it is the difference between "silently wrong forever" and "one unannounced event". Noting that
children always take the loud path, and closing with "do not relax the server-side index expecting a
client-side net", puts the constraint where the next person will hit it.
I also accept that I framed H1's fix wrongly. ConflictAlgorithm.replace at the bulk parent insert
is that path's upsert — it has no existing.isEmpty branch — so it cannot simply become abort,
and replace cannot distinguish a legitimate same-uuid update from a pathological
same-uuid/different-server_name collision. The only client-side discriminator is the per-row query
you declined, so relying on the server-side index is the pragmatic call rather than a shortcut. My
Duplicate/Amend trigger doesn't survive it either: with the index in place, Frappe's own Duplicate
fails 1062 before a second such document can exist.
The child asymmetry — objection withdrawn. I was going to raise that this PR makes child adoption
newly live (it does, since children previously round-tripped as NULL) and that leaving it ungated keeps
the blind spot open for the case you cite as load-bearing. Two things you documented close it: both
child write paths set server_name from cr['name'] (:582, :719), so the reachability mitigation
extends to children; and the preservation fixture uses a deliberately non-v4 uuid because deployments
exist with that shape, so gating would break real installs. History over taste, as you put it.
Two corrections to my round 1
M2 — you corrected my premise, and the fix is better for it. I accepted the docstring's claim that
417 is how an oversized upload arrives and only argued about breadth. You checked: Frappe sets
werkzeug's max_content_length to the same get_max_file_size() (app.py:194), so the HTTP layer
returns 413 before MaxFileSizeReachedError can be raised — confirmed with a 30 MB upload on a
live bench. The branch's stated justification was wrong, not merely over-wide. Classifying on
exc_type from the decoded body (v2's errors[].type read too, substring check demoted to a fallback
for a body carrying no type) is structured rather than a guess, and 413 lands on the 4xx rule where it
belongs.
Child mobile_uuid never reaching the wire is a better catch than anything in my round 1 — and it
retro-corrects my H1. PayloadAssembler re-added the parent's uuid after the system-column strip and
the child loop never put it back, so, as your CHANGELOG puts it, "the pull path's child adopt branch
could only ever fire for a value someone had set in Desk." My "four write sites" table therefore
overstated the child half: those two sites were inert in practice, not live. The finding was right
about the parent paths and wrong to weight the child ones equally.
The three deliberate details all check out: the child's own uuid rather than row.mobileUuid (which
would collide on the second child), omitted-when-blank rather than '' (MariaDB permits many NULLs
but not many empty strings in a unique index), and the set_only_once check confirming re-sends are
safe. No backfill, self-healing on next push, is the right migration posture. Retiring the stale
ThreeWayMerge note rather than leaving a comment claiming work already done is the kind of cleanup
that usually gets skipped.
Why this stays CHANGES_REQUESTED — and it is not a finding
Nothing I have reviewed is unresolved. The gate is held for one reason only: a section of this PR
that I have not read.
Unchanged from round 1:
image_field.dart— 527 changed lines, the largest single file in the diff, still unread.attach_field.dart,child_table_field.dart— read only where later rounds touched them
(thefrappeFileFetchUrldelegation).form_screen.dart— read only_fetchMediaBytes.- The 38 test files and
attachments.md(803 lines).
That covers the pick / discard / re-pick / preview lifecycle and the hasInteractedByUser change —
the part of this PR most likely to hold a widget-lifecycle or state bug, and the part with the least
review. It is also the part your own verification section lists as unit-tested only: offline capture,
offline preview from outbox/, re-pick/discard reclaim, camera lost-capture recovery, OpenFilex
handoff.
So: no changes are being requested of you. Either I do that pass next, or you tell me to approve on
the strength of the device run and the 2,364-test suite and I'll do that with the gap stated in the
approval. Your call — please don't sit blocked waiting on me if you'd rather ship it.
Gate
| Area | R1 | R2 | R3 |
|---|---|---|---|
| Attachment commitment model | ✅ | ✅ | ✅ |
| Push gate / marker containment | ✅ | ✅ | ✅ |
| Cache index ↔ filesystem | 🟡 M1 | ✅ | ✅ |
| Terminal / transient classification | 🟡 M2 | ✅ | ✅ |
| Download bounds | 🟡 L2 | ✅ | ✅ |
| Fetch-URL duplication | 🟡 L3 | ✅ | ✅ |
mobile_uuid shape invariant |
🔴 H1 | ✅ | ✅ |
mobile_uuid uniqueness |
🔴 H1 | 🟡 undocumented | ✅ three outcomes stated |
Child mobile_uuid round trip |
⚪ missed | ✅ your find | ✅ |
| Public API surface | 🔴 H2 | ✅ | ✅ |
| UI layer | ⚪ not reviewed | ⚪ | ⚪ still not reviewed |
Worth keeping from these three rounds: every round-1 finding was two call sites that had to agree and
didn't — a path computed with and without an argument, a prefix spelled two ways, a backoff list read
as two contracts, three copies of a URL builder, two conflict policies. Four of the fixes deleted the
second copy rather than syncing it (moveToCache returning its own destination, the constant, the two
delegations), and the one that couldn't be deduplicated got its asymmetry written down with the
failure mode enumerated. That is the right disposition of the class, and it is why I would not expect
it to recur here.
Discarding or replacing an attachment after saving destroyed the only copy
of bytes a committed pending_attachments row owned, and blocked the
document on a file that no longer existed.
MediaStore.discardReplacedValue deletes any path inside outbox/ and skips
the database on the reasoning that a saved attachment's column holds
pending:<id>, never a path. That is true of the COLUMN, which LocalWriter
rewrites inside the save transaction. It is not true of the value a form
hands over: the field pins what the user picked (hasInteractedByUser beats
a later widget value, deliberately, so an explicit clear survives an async
document load) and nothing writes the marker back into an open form. So
after a save the field still offers the raw staged path while the column
and the queue row have moved on.
Verified rather than reasoned: after didChange('/.../outbox/u1/photo.jpg'),
a rebuild supplying 'pending:42' leaves fieldState.value at the raw path.
The consequence was not a leak. AttachmentPipeline uploads
fileFromPath(localPath); a missing file is not a terminal error, so it
exhausted the backoff, markFailed blocked the parent document, and push
does not auto-retry. A later save repaired it (LocalWriter drops the row
when the field arrives empty); leaving the form without saving, or a push
landing in the gap, did not.
The fix
- ReclaimAttachmentFn, defaulting to MediaStore.discardValue so a host with
no queue to consult is unchanged.
- OfflineRepository.reclaimDiscardedAttachment consults
PendingAttachmentDao.referencedLocalPaths() and refuses a referenced
file. Canonicalised on both sides: a `..` or double-slash difference
reading as "not referenced" is the verdict that deletes the file.
- Both reclaim sites in both fields call the hook instead of MediaStore.
Wired at every hop: FormScreen and FrappeFormRenderer.renderForm ->
FrappeFormBuilder -> FieldFactory -> fields, plus BOTH nested
child-table FrappeFormBuilders. A child row picks and discards on the
same path, so a hook stopping at the parent leaves half the fix unwired
and silent.
- Refusing leaves a staged file until the next save or the orphan sweep
reclaims it. That is the recoverable side of the trade; the other side
is a queued upload with no file.
FieldFactory carries the hook as INSTANCE STATE, not as a createField
parameter. Dart requires an override to redeclare every named parameter of
the method it overrides, so a new one breaks every existing subclass at
compile time and a default value does not save it — the reason
capDataLength and errorTextResolver already give for the same choice.
field_factory_override_compat_test pins it with a subclass carrying the
prior signature verbatim; it stops compiling the moment that signature
grows.
Tests, each watched failing first
- offline_repository_attachment_reclaim_test: refuses a referenced file,
reclaims an unreferenced one.
- attachment_discard_after_save_test: the whole sequence on the real save
path — stage, saveDocument, assert the column became pending:<id> while
the queue row kept the raw path, discard the raw path, file survives.
Run against the pre-fix MediaStore.discardValue it fails with the file
deleted, so it discriminates the bug rather than the code.
- attachment_reclaim_hook_test: both fields route discard through the hook.
- form_builder_reclaim_wiring_test: the hook reaches parent AND child-row
fields in BOTH form modes, and a host-supplied customFieldFactory. Each
hop was mutation-tested — removing any one turns the matching case red.
- form_screen_reclaim_wiring_test: FormScreen supplies the repository's
reclaim, not the default. Its red read "Actual: discardValue: static".
- public_api_surface_test: ReclaimAttachmentFn is nameable from the barrel.
Docs
- attachments.md: the discard section encoded the same wrong premise —
its table promised "the save drops the row" for a value the field never
holds after an in-session save. Rewritten, with the new outcome split
out. Also retired the stale "ResolveMediaFn is not exported" limitation
and the §13 pointer beside it; H2 closed that earlier in this branch.
- CHANGELOG: Fixed entry for the defect, Changed entry for the typedef.
|
Did the UI pass myself rather than have you approve blind — it turned up one real Discard/re-pick after a save deleted the bytes behind a committed queue row. Fixed with a Also reproduced what you couldn't: New finding, raising not fixing: this branch added five named params to |
deepak-dhwani
left a comment
deepak-dhwani
left a comment
There was a problem hiding this comment.
First round with a toolchain. I installed Flutter 3.35.7 (the pubspec floor) and actually ran the suite — rounds 1–3 were all static:
flutter test → 2376 passed, exit 0 (reproduces the +2376 claim exactly)
flutter analyze → clean for lib/ (2 pre-existing, neither from this PR)
Every finding below was then confirmed by running probe code, not by reading.
What I found
🔴 H1 — Attach fields download the whole file on render, not on view. MediaResolveBuilder._start() runs from initState, and MediaResolver.resolve fetches on a cache miss. Probe: an AttachField holding /private/files/big-survey-report.pdf issued the fetch with zero taps. For ImageField that's fine (Image.network fetched anyway); for a PDF there's no render-time need at all. Opening a form now pulls every attachment on it, up to 25 MB each — on Android 8 / metered rural connections. And attachments.md:28 bills the read path as "lazy download" while :657 says "No background prefetch — cached on first view." Rendering a form isn't viewing a PDF.
🔴 H2 — Omprakash's own find, confirmed. createField gained five named params on this branch, so every FieldFactory subclass breaks at compile time. CHANGELOG:240 says the signature "should be treated as frozen"; form_builder.dart:431-442 repeats it verbatim in the same file whose :1409 and :2267 call sites pass the new arguments. One thing that strengthens his case: field_factory_override_compat_test cannot catch this — its "prior signature" baseline is ca69c95, four commits after the five params landed.
🟡 M1 — cachePathFor builds the cache filename from an unsanitised URL. p.extension takes the query string too. Probe, with the multi_cloud_storage proxy URL shape the code names by vendor:
basename = .abcdefghijklmnop
with a 300-char signed key → errno 63, File name too long
The catch-all swallows it → no media_cache row → silent permanent cache miss, re-download forever. attach_field.dart:82 already has the guard (_safeExtension) 400 lines away.
🟡 M2 — the pick path reclaims fieldState.value, the discard path reclaims the rendered value; probe shows them diverging (null vs the staged path) on an async document load. 🟡 M3 — form_builder.dart:451 clobbers a host factory's reclaimAttachment with the unsafe default on every build, while :560 uses ??= for the sibling hooks. 🟡 M4 — frappe_attachments/ temp cache is outside clearAll, sweepOrphanedMedia, mediaStoreUsage and logout(clearDatabase: true), which matters because this PR sells the logout wipe as a security fix. 🟡 M5 — _fetchMediaBytes has a connect timeout but no stall timeout; its sibling in attach_field.dart:578 does. Plus three lows.
93cfb7a itself is correct — I have nothing to add to it. Nothing here touches the pipeline; rounds 1–3 held up under an actual suite run.
… wipe paths H2 — `createField` gained five named parameters, which breaks every existing `FieldFactory` subclass at compile time: Dart requires an override to redeclare every named parameter of the method it overrides, and a default value does not help. Confirmed against a real consumer, whose override fails with `invalid_override` before this change and analyzes clean after. The five are now instance state alongside `capDataLength` / `errorTextResolver` / `reclaimAttachment`. `field_factory_override_compat_test` now pins the **published** v2.0.0-beta.2 signature; it previously pinned this branch's own, five parameters included, which is exactly why it passed while the break was live. They shipped in no tag, so removing them breaks no published API, and an override may keep declaring them — only dropping one the base declares is illegal. M3 — `_configureFieldFactoryForMeta` assigned all six capabilities unconditionally, so a host that configured its own `customFieldFactory` and did not repeat the values had them overwritten on every build: `reclaimAttachment` with `MediaStore.discardValue`, which deletes a staged file a queued `pending_attachments` row still owns — the data loss 93cfb7a fixed, reintroduced through the wiring — and the other five with null. Now assigned only when supplied, and `FrappeFormBuilder.reclaimAttachment` is nullable so "not supplied" is distinguishable from "supplied the destructive default". A plain `??=` was rejected: it also refuses the update case, and `pendingAttachmentPaths` must keep changing as picks complete. H1 — `AttachField` resolved media from `initState`, and `MediaResolver.resolve` downloads on a cache miss, so opening a form fetched every attachment on it (up to 25 MB each) before the user asked for any. The widget renders only a button; resolution moved into its tap handler, which keeps the memoisation property a builder existed to provide. `ImageField` still resolves eagerly — its preview paints from the local file. M1 — `cachePathFor` took `p.extension` of a URL, query string included. For the cloud-proxy shape a 300-character signed key produced a 373-character filename against a 255-byte NAME_MAX; the write threw, `resolve`'s catch-all swallowed it, no `media_cache` row was written, and the attachment re-downloaded on every view forever. Extensions are now accepted only as a dot plus 1-10 alphanumerics, and rejected rather than truncated. M2 — the pick path reclaimed `fieldState.value` while discard reclaimed the `hasInteractedByUser`-aware value. They diverge before the first interaction, so replacing an attachment supplied by an async document load reclaimed null and leaked the staged file it displaced. Both paths in both widgets now share `liveAttachmentValue`, so they cannot drift again. M4 — the viewer's temp cache and the camera-capture marker live under the OS temp root, outside `MediaStore`'s root, so `clearAll`, `sweepOrphans` and `usage` could never reach them: decrypted copies of private attachments survived `logout(clearDatabase: true)`, whose entire purpose is that they are gone. `MediaStore` now owns both. `clearCache` reclaims the temp cache as well, because it counts toward `MediaStoreUsage.totalBytes` and would otherwise be a reported figure with no reclaim path. It stays out of `sweepOrphans`: nothing references those files, so including them would make a reference-based sweep a blanket delete. The marker's path is deliberately unchanged so an upgrade orphans nothing. M5 — `_fetchMediaBytes` bounded `client.send` but not the body, so a connection that went quiet mid-download left the resolve future permanently unresolved. Adds a per-chunk stall timeout (not a total deadline, so a slow large download still completes) and extracts the capped read to `readCappedMediaBody`, since the method builds its own client and left no seam for a hung-server test. Also closes three separate "two constructions of one path" divergences — the class of bug behind the earlier `moveToCache` / `resolve` cache-path fix. `getTemporaryDirectory()` is now resolved in exactly one place in `lib/`, and `path_provider` became an unused import in both field widgets as a result. Behaviour changes that need sign-off rather than just review: H1 alters user-visible fetch timing (a form no longer warms the cache, so a first tap while offline misses — which is what doc/release-2.0/attachments.md already promised, and its contradictory summary line is corrected here), and `MediaStoreUsage.totalBytes` now includes `viewerTempBytes`, so a host showing that figure will display a larger number than before. Verified: flutter analyze clean, 2413 tests pass, dart format clean, example/ clean. Both consumer apps analyze with 0 errors and produce test results identical to their own pinned SDK.
|
Fixed in 1ee7dfa. All findings confirmed by running probe code first. H2 — createField break. Confirmed. The five params are now instance state. H1 — eager download. Confirmed, and you were more precise than me. I assumed M1 — cache filename. Confirmed: 309-char extension → 373-char filename vs M2 — reclaim mismatch. Confirmed. Both paths now share one M3 — clobbered host factory. Confirmed, and worse than reported: my own H2 M4 — temp cache. Confirmed, and not a subdir issue: it's under M5 — no stall timeout. Confirmed. Per-chunk bound, so slow large downloads Found while auditing: three cases of two independent constructions of the Needs sign-off (not just review):
Not covered by tests: H1's miss fallback, M2's pick call site Upgrade note: run analyze clean · 2413 tests pass · format clean · example/ clean. Both consumer |
b617791 to
1ee7dfa
Compare
deepak-dhwani
left a comment
deepak-dhwani
left a comment
There was a problem hiding this comment.
Done, and it's ready to merge
2 participants
Attachments picked on a device that is offline (or in offline-first mode) are now staged locally, queued, uploaded by the sync pass, and relinked to the real record and field on the server — with the uploaded bytes kept on device as the preview cache, so an attachment stays viewable without connectivity. Previously an attach field decided by connectivity alone: an online device uploaded inline, which put the file outside the offline pipeline entirely, and an offline pick could write the literal
pending:<id>marker into Frappe as a field value.Full design, case matrix and internals:
doc/release-2.0/attachments.md— new, 803 lines.Size: 71 files, +8,744 / −447 — 30 in
lib/, 38 tests (28 new), 3 docs. Base branchdevelop.What changed
Pipeline and push gate
server_file_urlcommitted to SQLite is now the correctness boundary: the url is written back intodocs__<doctype>in the same transaction as the state transition, so payload assembly reads an ordinaryfile_url. Upload and document push are independent commitments — an uploaded file is never re-uploaded, whether or not the parent ever syncs.done; otherwise it throwsBlockedByUpstreamnaming the file and field.inlinePayloadnow throws on an unresolved marker instead of shipping it.AttachmentState.rejected(oversized, wrong type, not permitted) is never auto-retried;failedstays transient and is re-armed next dispatch. Unrecognised errors default to transient.pending:<id>marker reached Frappe verbatim (upload OK → parent POST failed → retry; terminal refusal →blocked→ retry, which reported success; anyTimestampMismatchErrorvia auto-merge recursion, with no user action).AttachField/ImageField/FrappeFormBuilder/FieldFactorygain anisOfflineModecallback;resolvePickedAttachmentgainsofflineModeEnabled. Both default to previous behaviour.Media cache and offline previews (schema v6 → v7)
media_cachetable +MediaStore, keyed by serverfile_url(Frappe dedupes by content hash, so per-document caching would double-store and turn deletion into reference counting). Push-uploaded bytes move into the cache; pulled media is cached on first online view. Attach / Attach Image / Image render from disk on a hit; non-image attachments are handed to the external app from the cache. Cache state is non-authoritative — a vanished file is a miss, never an error.mform_attachments/outbox/<id>/<original filename>, uploaded bytes atmform_attachments/cache/<sha256(file_url)>. Staging is never evicted (it is the only copy); cache always is (re-fetchable).Frappe-side correctness
upload_filewas sentdt/dn/filename;frappe/handler.pyreadsdoctype/docname/file_name(checked against 16.25.0, 16.26.3, 17.0.0-dev). The old branch had no in-tree caller, so no released behaviour changed.RestHelper.uploadFile(load-bearing: Frappe reassignsfilenamefrom the part whenever a file part is present).UI
ImagePickSourcehost hook — gallery, camera, or both; read live, global,nullmeans both. Attach Image / Image only.max_file_size), enforced before the durable copy; widgets surface the real limit.hasInteractedByUserdistinguishes "user cleared this" from "never touched").Storage APIs and leak fixes
FrappeSDK.mediaStoreUsage()(staged / cached / reclaimable-now) andsweepOrphanedMedia()(exact guards only: unreferenced and not staged this session; never throws; deletes nothing if the reference query fails).clearMediaCache()no longer wipes staged files — it clearscache/and the index only.logout(clearDatabase: true)is now the only path that clears staged files, where that is the intended security behaviour (previously the store survived logout, leaving one user's photos readable on a shared device).hardDeleteLocalMirror, cancelled-INSERT, tombstone → all route throughdeleteForTopParent), on re-pick, and on a pick that never reached a save.pending_attachments.size_bytes/.mime_typewere never populated; a fresh install wrotesdk_meta.schema_versionfrom a drifted literal.Also in this PR (not attachments)
mobile_uuidadoption on pull —pull_apply.dart. A pulled document was stored under a freshly mintedmobile_uuideven when the server round-tripped this device's own value: the server value was read as a matching key only, never stored, and both parent write paths minted unconditionally. Invisible in normal operation (a present local row preserved its uuid), it surfaced whenever the row was gone —logout(clearDatabase: true), reinstall, fresh-device onboarding — giving the same server records brand-new local identities and defeating theuniquemobile_uuidfallback that exists to reconcile an interruptedserver_namewriteback. Precedence is now local → incoming → mint through oneresolvePulledMobileUuidhelper used by all four write sites; empty/absent still mints (Desk-origin rows return null/'', and it is the local PK). Child behaviour is unchanged, only deduplicated onto the helper.ImagePickSource/MediaStoreUsage, which 2.0.0-beta.2 documented as host-facing but never exported, so the pick-source hook could not be wired at all.attachments.mdandCHANGELOG.md(stale "not verified on a device" claims; server-side image shrinkage attributed to the wrong mechanism).Breaking / migration
Hosts that only call
FrappeSDK/SyncServiceare unaffected —PushEnginewires the pipeline internally.media_cache. Automatic, backfills nothing (an empty cache is just misses); existingpending_attachmentsrows keep working — they store absolute paths.FrappeSDK.mediaStoreSize()→mediaStoreUsage()(a single number cannot say how much is reclaimable). Both are unreleased, so nothing shipped against the old name.AttachmentPipeline.uploadPendingForTopParent→resolveForTopParent; the constructor now requiresdb;resolutionMapForis new.PendingAttachmentDao.findPendingForTopParent→findUnresolvedForTopParent, plusfindAllForTopParentandmarkRejected.AttachmentPipeline.inlinePayloadthrows on an unresolved marker (was a silent passthrough).resolvePickedAttachmentgainedmaxBytesand now rethrows a terminal upload failure and discards the staged copy; a transient failure still falls back to staged queueing.Verification
flutter teston this branch: 2338 tests pass (exit 0, Flutter 3.44.2). 28 new test files, 10 updated.test/public_api_surface_test.dartimports only the public entry point, so it fails to compile if an export is dropped — every other test importssrc/directly, which is why the missing exports were invisible.attachments.md§16): Android 8.0.0 / API 26 debug build against a Frappe v16 bench, offline mode on, two documents × two Attach Image fields (camera + gallery). Confirmed: staging keeps the picked filename, zeroupload_filecalls before sync, markers never crossed between docs or fields, all four →donewith real/private/files/…urls, a server-sideLIKE 'pending%'query on both fields returning empty,Filerelink correct on all four (reconstructed server-side fromfile_urlalone), staged bytes moved intocache/, andlogout(clearDatabase: true)removingmform_attachments/entirely along with everydocs__*table and all credentials.mobile_uuidfix was reproduced and re-verified on a live bench (bulk pull path): divergence before, byte-for-byte adoption after, a Desk-created record with a null server uuid correctly still minting, children parented to the adopted uuid with zero orphans, 603/603 HTTP 200.outbox/,BlockedByUpstreamon a gated document, thefailedandrejectedstates, re-pick/discard reclaim, camera lost-capture recovery,OpenFilexhandoff.strip_exif_metadata_from_uploaded_imagesenabled, Frappe re-encodes JPEGs through PIL — lossy, GPS EXIF destroyed, dimensions unchanged — so the device cache holds the pre-strip original while the server holds the re-encode. Benign, because the cache is non-authoritative, but a cache hit and a miss can render slightly different bytes for the same url. PNG is untouched (the strip is JPEG-gated). This is a site setting, not SDK behaviour or a Frappe default.Known issue (not fixed here)
ResolveMediaFnis still not exported, so a host cannot write the signature of aFieldFactory.createFieldoverride —FieldFactoryis exported bare and documented as overridable, but the typedef lives insrc/services/media_resolver.dart, and Dart does not re-export a library's imports. Confirmed against a real consumer app that subclassesFieldFactory; it cannot compile without animplementation_importsviolation. The fix is one export line. 2.0.0 should not ship claiming that type is host-facing until it lands —public_api_surface_test.dartwill guard it afterwards.Review pointers
lib/src/sync/attachment_pipeline.dart,lib/src/sync/push_engine.dart(the gate + writeback transaction).lib/src/sync/pull_apply.dart(resolvePulledMobileUuidprecedence).lib/src/utils/media_store.dart,lib/src/utils/attachment_paths.dart.test/sync/marker_resolution_regression_test.dart,test/sync/attachment_multi_field_test.dart,test/sync/pull_apply_mobile_uuid_adoption_test.dart.