fix(drive): skip re-downloading flat package bundles every sync#473
Open
epheterson wants to merge 12 commits into
Open
fix(drive): skip re-downloading flat package bundles every sync#473epheterson wants to merge 12 commits into
epheterson wants to merge 12 commits into
Conversation
iCloud Drive serves Apple iWork (.key, .pages, .numbers), JMG (.jmb),
and other "package" types as opaque archives. libmagic detects only
zip/gzip; everything else reports as application/octet-stream and the
old code path returned None from process_package(), which the caller
in drive_file_download.py treated as a hard download failure — even
though the bytes had ALREADY been written to disk and were perfectly
usable.
Result on Eric's library: every sync cycle re-downloaded ~300 MB of
.key + .jmb files, logged as "0 successful, 23 failed" — alarming
but not actually a data-loss bug, just wasted bandwidth + misleading
logs.
Fix:
- process_package() now returns the local_file path (truthy) when the
mime type isn't a recognised archive. The file is on disk in a
usable form; that IS the canonical local representation for these
bundle types. Logged at INFO ("Package format not recognised for
unpacking; keeping as single-file bundle") instead of ERROR.
- drive_file_download.py treats only ``None`` return as failure (was
treating any falsy value as failure).
Tests:
- Renames test_process_package_invalid_package_type →
test_process_package_unrecognised_mime_preserves_file with the new
contract.
- Adds test_download_file_keeps_unrecognised_package_as_single_file to
exercise the success path through download_file.
- Fixes the patch target in test_download_file_returns_none_on_processing_failure
(was patching the wrong module-level binding; happened to pass under
the old contract by coincidence).
NOT fixed in this PR: the next-sync dedup still triggers a re-download
because file_exists() compares getsize(local_file) to item.size, and
for flat bundles those differ (item.size is the unpacked contents
total). That requires either a sidecar metadata file or relaxing the
size comparator to fall back on mtime; out of scope for this focused
fix. Documented as a follow-up.
Co-Authored-By: Claude <noreply@anthropic.com>
Extends PR 11 with a fix for the FileExistsError seen when two iCloud
Drive packages share generic internal entry names (Apple iWork
.numbers / .pages / .key — all rooted at "Data/Document.iwa",
"Metadata/...", etc).
## The bug
``_process_zip_package`` called ``extractall(path=parent_dir)``, which
worked fine for genuine bundle zips like GarageBand's .band whose
entries are self-prefixed ("Project.band/...") but silently collided
for bare-rooted zips: two .numbers in the same iCloud folder both
extract "Data/Document.iwa" into the parent dir, and the second one
raises FileExistsError at the rename/extract step.
Real-world report: on a 0.7.3 deploy, mandarons logged ``Failed to
download Untitled 6.numbers: [Errno 17] File exists:
Untitled.numbers`` — the second .numbers couldn't land because the
first had already extracted bare entries into the shared parent.
## Fix
``_zip_entries_self_prefixed`` heuristic picks the right extraction
target:
- All entries under ``<bundle_basename>/`` → extract into parent
(preserves historical behaviour for .band-style bundles).
- Otherwise → extract into a bundle-named subdirectory of its own
(iWork-style bare-rooted zips, no sibling collisions).
## Plus a config knob: ``drive.flatten_packages``
For backup-style deployments (NAS, cold storage) where bundle-
directory semantics aren't valuable, ``drive.flatten_packages: true``
skips the unpack step entirely and keeps the package on disk as the
downloaded single binary file:
- One mtime+size per package → simpler dedup, easier round-trip
back to iCloud.
- No expansion to hundreds of internal files per bundle → way lower
inode footprint on NAS-style filesystems.
- No cross-bundle collisions period — the whole class of bugs goes
away regardless of internal zip layout.
Default False so existing installs are unaffected. Threaded through
``download_file → process_package`` via ``flatten_packages`` kwarg +
``flatten`` flag.
## Tests (10 new in test_drive_package_bundle_layout.py)
- Self-prefixed zip extracts into parent (no regression for .band).
- Bare-rooted zip extracts into bundle subdir.
- ``test_two_sibling_iwork_zips_dont_collide`` is the headline case —
reproduces Eric's exact error, confirms the fix.
- ``flatten=True`` preserves zip-detected packages as single files
(byte-identical, no extraction).
- ``flatten=True`` is a no-op for octet-stream packages too.
- Five config-getter cases (default, drive-absent, true, false,
None-config).
All 24 existing package + download tests still pass.
Co-Authored-By: Claude <noreply@anthropic.com>
…l too Gzip-wrapped bundles can produce inner ZIPs whose entries are prefixed with ../<basename>/ — Python zipfile sanitises the .. so the on-disk end-state is identical to a plain self-prefixed entry. The new heuristic was treating these as bare-rooted and extracting into a bundle subdir, breaking 4 existing test_sync_drive cases. Recognise both forms; extract into parent for either; keep the bare-rooted iWork path going to its own subdir. 581/581 tests pass after this fix. Co-Authored-By: Claude <noreply@anthropic.com>
…heck Upstream CI runs ruff. Combined the two startswith() calls in _zip_entries_self_prefixed into a single startswith((prefix, traversal_prefix)) per PIE810. Identical behaviour, cleaner code. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…100%) The one uncovered line on this branch was the `if not names: return False` guard in _zip_entries_self_prefixed — exercised when a zip's only entry is the bare bundle directory itself with no files inside. Covers it with a tiny in-memory zip and asserts False. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ck user-zip invariant
Addresses all 3 Copilot review points on this PR, plus an explicit
defence-in-depth check Eric flagged ("are we unzipping only Apple
packages or any zip file?"):
1. **Zip Slip (CWE-22) defence.** ``_safe_extractall`` wraps
``ZipFile.extractall`` with a per-member ``os.path.realpath`` +
``os.path.commonpath`` check. Any entry whose final target
resolves outside the configured safety boundary is refused and
logged at WARNING; the rest of the zip still extracts. CloudKit-
served bytes are technically untrusted -- an attacker-controlled
shared package, or a future CloudKit compromise, could otherwise
write outside the destination via absolute paths or unbounded
``..`` traversal.
The safety boundary is layout-specific:
- ``self`` (entries are ``<bundle>/...``): extract_dir = parent_dir; boundary = parent_dir.
- ``traversal`` (entries are ``../<bundle>/...``): extract_dir = parent_dir; boundary = grandparent_dir (the dir ``..`` lands in).
- ``None`` (bare-rooted): extract_dir = bundle subdir; boundary = bundle subdir.
``_zip_entries_self_prefixed`` now returns ``"self" | "traversal" | None``
so ``_process_zip_package`` can pick the right boundary.
2. **Flatten mode skips libmagic entirely.** Moved the ``flatten``
early-return ABOVE the ``magic.from_file`` call. Flatten doesn't
need MIME detection, and libmagic can fail on truncated or unusual
downloads where we still want to keep the bytes on disk.
3. **Direct call instead of ``getattr(..., lambda...)``.** The
``get_drive_flatten_packages`` config getter is unconditionally
present once this PR lands, so the defensive ``getattr`` wrapper
was just noise.
4. **User-zip invariant test.** Eric's question -- "are we unzipping
only Apple packages, or any zip file?" -- pointed at a subtle
safety property. The architecture: Apple's CloudKit returns
``data_token`` URLs for regular files (any ``.zip`` a user uploads)
and ``package_token`` URLs for Apple bundle formats only
(``.key``, ``.pages``, ``.numbers``, ``.band``, etc -- files that
look like single files in Finder but are actually directories).
The gate in ``drive_file_download.py`` ONLY routes to
``process_package`` when ``/packageDownload?`` is in
``response.url``, so user-uploaded zips are never touched.
``TestProcessPackageNeverTouchesRegularUserZips`` audits the call
graph: ``process_package`` has exactly one external caller and
that caller has the URL-substring gate. If a future refactor
removes the gate, this test fails.
Tests:
- TestZipSlipDefence (3 tests) -- absolute-path entry blocked,
traversal escape blocked, benign traversal at widened boundary
not blocked.
- TestZipEntriesSelfPrefixedEdgeCases expanded to cover the new
``self`` / ``traversal`` / ``None`` return values explicitly.
- TestProcessPackageNeverTouchesRegularUserZips -- 1 audit test.
Verified on python:3.10 docker mirroring CI: ruff clean, 454 passed,
100.00% coverage.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
A package kept on disk as a flat single-file bundle (unrecognised-mime package, or flatten_packages=true) has an on-disk byte count equal to the package-download archive size, which never equals item.size (the package's logical size iCloud reports). file_exists() compares against item.size, sees a spurious size mismatch, and re-downloads the bundle on EVERY sync -- confirmed live on a real library (a 103 MB .key re-downloaded across consecutive syncs). Add package_bundle_unchanged(): when the item is a package and the on-disk bundle's mtime already matches the remote date_modified (which download_file stamps via os.utime, and iCloud bumps on any content change), the bytes are unchanged -- skip the re-download. Wired into both collect_file_for_download (active parallel path) and process_file (legacy). is_package() is already called in the outdated-file branch, so this adds no extra network round-trip. Completes the single-file-bundle handling from the package PR (mandarons#461 silenced the error log but left the re-download). 9 regression tests; 100% coverage. Co-Authored-By: Claude <noreply@anthropic.com>
…d side Code review flagged a latent timezone bug: download_file stamped mtime via time.mktime(item.date_modified.timetuple()) (misreads a tz-aware datetime as local time), while file_exists()/package_bundle_unchanged() read via .timestamp() (true epoch). They agree for the naive datetimes iCloudPy returns today (numerically identical -> existing files unaffected), but would desync for tz-aware datetimes and silently break dedup. Switch the write to .timestamp() so write and read use the same clock; drop the now-unused 'import time'. Add a tz-aware regression test. Co-Authored-By: Claude <noreply@anthropic.com>
Contributor
|
Hmmm, wonder if we have overlapping solutions? #451 |
…heck read side" This reverts commit 32ef043.
Contributor
Author
|
Good catch — yes, #451 is the right fix for the mtime/timezone half, and it predates mine. I've dropped the mtime change from this PR (reverted) and deferred to yours. They're complementary though: #451 makes mtimes TZ-invariant, but flat single-file package bundles still re-download because their on-disk size never equals |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR improves iCloud Drive package handling to avoid redundant re-downloads of “flat” (single-file) package bundles whose on-disk byte size can never match iCloud’s logical item.size. It also expands package processing to support an opt-in drive.flatten_packages mode and strengthens ZIP extraction behavior and safety.
Changes:
- Add an mtime-based “unchanged flat bundle” guard (
package_bundle_unchanged) and wire it into both the parallel and legacy file-processing paths to prevent per-sync re-download loops. - Add
drive.flatten_packagesconfiguration + plumbing so package downloads can be kept as a single file (skip unpacking). - Update package processing contract (unrecognised mime types are preserved) and add extensive regression/security/unit tests around package layouts and extraction safety.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
src/sync_drive.py |
Uses the new flat-bundle guard to skip wasteful re-downloads in the legacy path. |
src/drive_parallel_download.py |
Adds flat-bundle skip logic in the main parallel path and passes flatten_packages through to downloads. |
src/drive_file_existence.py |
Introduces package_bundle_unchanged() for mtime-only freshness checks on flat bundles. |
src/drive_file_download.py |
Adds flatten_packages option and updates package processing handling + mtime stamping. |
src/drive_package_processing.py |
Adds flatten option, improves ZIP layout handling, and adds Zip Slip defenses. |
src/config_parser.py |
Adds get_drive_flatten_packages() config accessor. |
tests/test_sync_drive.py |
Formatting changes plus test adjustments related to updated package-processing behavior. |
tests/test_drive_redownload_loop.py |
New regression tests ensuring flat bundles don’t re-download every sync. |
tests/test_drive_package_bundle_layout.py |
New tests for extraction layout rules, flatten mode, and ZIP slip safety/invariants. |
Comment on lines
+118
to
+120
| if not (item and local_file and os.path.isfile(local_file)): | ||
| return False | ||
| return int(os.path.getmtime(local_file)) == int(item.date_modified.timestamp()) |
Comment on lines
+128
to
+132
| if all(n.startswith(prefix) for n in names): | ||
| return "self" | ||
| if all(n.startswith((prefix, traversal_prefix)) for n in names): | ||
| return "traversal" | ||
| return None |
Comment on lines
+268
to
+270
| if __name__ == "__main__": | ||
| unittest.main() | ||
|
|
Comment on lines
+45
to
+46
| mtime = int(item.date_modified.timestamp()) | ||
| os.utime(path, (mtime, mtime)) |
Comment on lines
+70
to
+71
| newer = int(item.date_modified.timestamp()) + 5 | ||
| os.utime(path, (newer, newer)) |
Comment on lines
+127
to
+128
| newer = int(item.date_modified.timestamp()) + 99 | ||
| os.utime(path, (newer, newer)) # remote changed → mtime differs |
…convention mandarons#451 (now merged to main) made file_exists/package_exists read mtimes as naive-UTC via .replace(tzinfo=timezone.utc).timestamp(). package_bundle_unchanged was the lone naive holdout — switch it to the same convention so flat-bundle dedup is TZ-invariant too (otherwise it'd break under a non-UTC TZ, the exact bug mandarons#451 fixes). Test stamps mtime the same way. Co-Authored-By: Claude <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
A package kept on disk as a flat single-file bundle — an unrecognised-mime package kept as-is (#461), or any package downloaded with
flatten_packages: true— has an on-disk byte count equal to the package-download archive size, which never equalsitem.size(the package's logical size iCloud reports).file_exists()compares the local size toitem.size, sees a spurious mismatch every run, and re-downloads the bundle on every sync — pure wasted bandwidth.Confirmed live on a ~100k-photo / large-Drive install: a 103 MB
.key(plus.key-tef,.playgroundbook,.abbu,.pvt) re-downloaded on every consecutive sync, and bloated each drive cycle.Fix
Add
package_bundle_unchanged(item, local_file)(indrive_file_existence.py): when the item is a package and the on-disk bundle's mtime already matches the remotedate_modified, the bytes are unchanged — skip the re-download. mtime is the reliable signal here becausedownload_filestamps it fromitem.date_modifiedand iCloud bumpsdate_modifiedon any content change; the size comparison is unusable for flat bundles.Wired into both
collect_file_for_download(the active parallel path) andprocess_file(legacy).is_package()is already called in the outdated-file branch, so the skip adds no extra network round-trip.Also (review-driven):
download_filestamped the file mtime viatime.mktime(item.date_modified.timetuple()), which misreads a timezone-aware datetime as local time, while the freshness checks read it back with.timestamp(). They agree for the naive datetimes icloudpy returns today (numerically identical → existing files unaffected) but would desync for tz-aware datetimes and silently break dedup for all files. Switched the write to.timestamp()so write and read use the same clock.Validation
10 tests in
tests/test_drive_redownload_loop.py(mtime match/mismatch/missing/dir/no-item, package-skip in both paths, non-package still re-downloads, changed-package re-downloads, tz-aware round-trip). 100% coverage maintained. Verified in production: after deploy,.key/.key-tef/.playgroundbook/.abbure-downloads dropped to zero.