Repository navigation
fix(backup): publish complete archives without damaging previous backups - #198
Conversation
johnny4young
left a comment
There was a problem hiding this comment.
Summary: GanchoArchive.export builds the archive in a private sibling stage (.ganchoarchive-stage-<UUID>), re-validates the manifest and every checksum with streamed SHA-256, then publishes it. A new destination is published with renameatx_np(RENAME_EXCL); an existing directory is replaced with RENAME_SWAP, and only under an explicit .replaceExisting. The macOS app no longer deletes the user's destination when an export fails.
Verdict: an improvement in crash consistency. Two should-fix risks need an owner decision, so I did not push anything.
🟠 1. External drives and network shares may stop working for backup
Publication has no fallback when the filesystem does not support renameatx_np flags. RENAME_SWAP (and possibly RENAME_EXCL) depends on volume capabilities (VOL_CAP_INT_RENAME_SWAP / _EXCL). FAT32, exFAT and some SMB volumes, which are common for backup drives, may return ENOTSUP/EINVAL. On main, those exports worked; with this PR, even a backup to a fresh destination fails there. The doc acknowledges it, but please qualify on an exFAT USB drive and an SMB share. Two possible ways forward:
- For a fresh destination only, fall back to
mkdir-exclusive +rename(2)whenRENAME_EXCLreportsENOTSUP. A fresh destination has no previous archive to protect. - Surface a specific message such as "this volume can't receive an atomic backup" instead of the generic failure.
🟠 2. .replaceExisting replaces any directory, and then deletes the old one recursively
backupHistory() in Apps/GanchoMac/SettingsView.swift uses an NSSavePanel with no allowedContentTypes. A user can type the name of an existing ordinary folder, confirm the save panel's "Replace?" prompt, and after the swap the stage defer deletes that folder's entire contents. On main the exporter wrote into the folder, and only deleted it on failure. Suggested cheap guard in AtomicArchivePublication.publish: only swap when the existing directory looks like a Gancho archive (it contains manifest.json) or is empty. Otherwise throw fileWriteFileExists. Declaring an exported UTType for .ganchoarchive and setting allowedContentTypes would also help.
🟡 Nits
GanchoArchive.swift,streamRows: the newtry Task.checkCancellation()runs inside GRDB'swriter.readclosure on GRDB's own queue, where there is no current task, so it is a no-op. Cancellation still works because GRDB's async APIs interrupt the read. Either drop the call or add a comment explaining it.- After a crash, a hidden stage that may contain the previous plaintext archive can remain in the user's folder. This is documented, but consider sweeping
.ganchoarchive-stage-*siblings older than N hours on the next export to the same parent. - The macOS app is unsandboxed today, so creating a sibling of the
NSSavePanelURL works. In a future sandboxed or App Store build, the panel grants only the chosen URL, not its parent, and this design would need to change.
Validation: there is no Swift toolchain in my environment, so this is code reading only. CI is green.
Generated by Claude Code
Replacement swaps the whole destination and then deletes the old one, so a save panel pointed at an ordinary folder would erase it. Only replace a directory that is already a Gancho archive (it has a manifest) or is empty apart from .DS_Store; refuse anything else untouched. exFAT, FAT and some SMB volumes reject renameatx_np flags with ENOTSUP or EINVAL, which made every backup there fail. A new destination now falls back to a plain rename after a non-existence check; replacing an archive on such a volume still fails and keeps the previous backup. The iOS temporary archive path is app-owned, so a manifest-less leftover from an interrupted in-place export by an older build is cleared first. Drop the cancellation check inside GRDB's read closure: it runs on GRDB's queue with no current task, and GRDB already cancels the read itself.
- Require the existing manifest.json to decode as a Gancho manifest before a replacement swaps the folder, so a project folder that happens to hold its own manifest.json is never moved into the stage and deleted - Check that the destination is publishable before streaming the export into a stage that would only be discarded - Clear the app-owned iOS temporary archive before every export again, so a failed export never leaves the previous plaintext archive behind - Write files in place inside the private stage, which is discarded whole on failure - Cover a foreign manifest, stage cleanup after replacement and after a failed export, and keep the unsupported-volume test on the swap path
Summary
GanchoArchive.exportbuilds the whole archive in a private sibling stage, re-validates manifest and checksums, then publishes it with one namespace change (RENAME_EXCLfor a new destination,RENAME_SWAPfor an explicitly authorized replacement). A failed export never damages the previous backup. Details below.Review follow-ups
d5f8e70— fix(backup): guard replacement and publish new archives on plain volumes.replaceExistingnow only swaps a directory that is already a Gancho archive (containsmanifest.json) or is empty apart from.DS_Store. Any other folder (e.g. an ordinary folder named in the save panel, which has noallowedContentTypes) is refused untouched withfileWriteFileExists. Previously the swap succeeded and the stage cleanup then deleted that folder recursively.renameatx_npflags (exFAT, FAT, some SMB →ENOTSUP/EINVAL): a NEW destination falls back to a plain rename after anlstatnon-existence check. Replacing an existing archive there still fails and keeps the previous backup (no remove-then-move).Task.checkCancellation()inside GRDB's read closure (no current task there; GRDB cancels the read itself).AtomicArchivePublication.publishtakes an injectableRenamerso the unsupported-volume path is unit-tested..DS_Store) replaced and restorable;ENOTSUP/EINVALpublish a new archive and refuse both policies over an existing one.docs/ARCHIVE_PUBLICATION.mdupdated.Decisions taken
How it was tested (this round)
swiftc -parse+swift-format lint --strict(Swift 6.0.3 container). Darwinrenameatx_npcode is not compiled locally; relies on macOS CI (see below).Known limitations & follow-ups
.ganchoarchive-stage-*(possibly holding the previous plaintext archive) next to the destination; a later sweep of old stages in the same parent is a possible follow-up.Merge order
Independent; merge before the v0.10.0 release PR #193.
CI: macOS jobs for the new head were queued when this description was written; check the PR checks tab.
Original description (state at
9fceab8; superseded where it conflicts with the sections above):Problem
Export replaced clips.json before all source blobs and the new manifest were ready. A later failure damaged an existing archive; application cleanup could then delete the selected prior backup.
Repair
Build a complete version-1 archive in an owned 0700 sibling stage. Validate its manifest and stream checksum validation with bounded memory. Check cancellation before publication. Use exclusive Darwin rename for a fresh destination and atomic RENAME_SWAP for explicitly authorized existing-directory replacement. Unsupported filesystem operations fail safely with no remove-then-move fallback. Cleanup owns only the private stage, including best-effort cleanup of the old archive after a successful exchange. macOS and iOS no longer delete the selected archive before or after a failed export.
Explicit replacement behavior and call sites
The API now defaults to failIfExists. Existing call syntax still compiles; writing over an existing archive requires explicit replaceExisting. macOS selects that policy after NSSavePanel's normal destination/replacement decision. iOS selects it for its application-owned temporary archive before the system Files exporter. The CLI runExport path exports plain JSON/CSV through exportJSON/exportCSV and does not call GanchoArchive.export. docs/ARCHIVE_PUBLICATION.md records this intentional safer-default semantic change and filesystem limitations.
Regression coverage added
Byte-identical previous archive and unrelated sentinel plus successful restore after injected row/blob/manifest/promotion errors; cancellation; missing source blob; explicit replacement refusal; successful consistent replacement. A native fixture test calls the actual Darwin publication syscall with a missing stage path and requires the old archive and separately recoverable stage bytes to remain intact. This complements, rather than substitutes for, checkpoint failures.
Format compatibility
Version-1 row encoding and restore behavior remain unchanged. Replacement is of the whole archive directory, not a merge of arbitrary files.
Exact-head validation
Published head: 9fceab8
Uploaded/local staged tree: 6b7e5a2a4b2436844880179f81423f06e91550d1 (verified identical before publication)
Combined source review
All five repair trees plus unmerged MCP PR195 at c90e07d combine without textual conflicts in a disposable local tree. Native combined execution remains unverified. MCP optional binary content may become nil after legitimate deletion/GC; the captured row hash is never replaced by a newer body lookup. No stronger availability promise for deleted content or new public-stack dependency is inferred.
Ready for human review only. No merge/release, live Keychain, clipboard, real user library or backup access.
Human-review readiness
Scoped implementation and regression coverage are complete at unchanged head 9fceab8. Fresh verification confirms both required native jobs and GitGuardian successful, the expected non-blocking beta-canary skip, and no pending discussion comments, submitted reviews or unresolved inline review threads. Review readiness does not authorize merge, release or deployment.
🤖 Generated with Claude Code
https://claude.ai/code/session_01ExmnfUngrDYjAheGvYwMnn
Generated by Claude Code