Skip to content

fix(export): preserve existing preset and carousel files - #258

Merged
johnny4young merged 9 commits into
mainfrom
fix/gui-batch-preserve-outputs
Oct 8, 2026
Merged

johnny4young merged 9 commits into
mainfrom
fix/gui-batch-preserve-outputs

Conversation

@johnny4young

@johnny4young johnny4young commented Oct 5, 2026 •

Copy link
Copy Markdown
Owner

Summary

  • Plan every preset/carousel image and requested sidecar name together, allocating numeric suffixes around occupied or duplicated names.
  • Stage complete bytes privately and publish with exclusive rename; destinations created after planning, including links, are never replaced.
  • Report requested nonempty sidecar failures as incomplete exports, preserving completed images and prior files. Stop new publication on cancellation.
  • Add disposable-filesystem fault/interleaving tests plus actual renderer tests for preset/carousel preservation and visible sidecar failure. Update English/Spanish failure copy and document the publication policy.

What changed

  • New Vitrine/Export/BatchOutputFiles.swift:
    • plan reserves image and sidecar names together, with case-folded, NFC keys and -2, -3, … suffixes.
    • publish stages in a private mkdtemp sibling and commits with renamex_np(RENAME_EXCL).
  • ExportManager+Batch.swift:
    • Multi-size and carousel exports use the plan and an injectable @Sendable writer.
    • Cancellation is checked before each commit.
    • A failed sidecar that was requested marks the item as failed.
  • Localized failure copy: "Some export files couldn't be written…" (en/es).
  • Tests: Tests/BatchOutputFilesTests.swift, Tests/MultiSizeExportTests.swift. Docs: docs/ARCHITECTURE.md, CHANGELOG.md.

Why

GUI batch exports used to overwrite existing vitrine-<preset>.png, carousel-NN.png and .txt files in the chosen folder, and a sidecar failure was silently ignored.

Review follow-ups

  • 6fe4ab1 fix(export): keep batch publication working on volumes without exclusive rename.
    • Some network and FAT-family volumes answer RENAME_EXCL with ENOTSUP/EOPNOTSUPP, which would have failed every file exported there.
    • Those volumes now fall back to an O_CREAT | O_EXCL | O_NOFOLLOW create. It still never replaces or follows an existing name, and on a failed write it removes only the file it created.
    • errno is now captured inside the rename call, so the reported error is the rename's own.
    • New test: exclusiveCreateFallbackNeverReplacesOrFollowsAName.

How it was tested

  • Original head 5fc63bf: complete CI passed (Linux static checks; Sequoia/Tahoe formatting, Debug and universal Release, coverage, performance, goldens; both UI and strict screenshot-tour lanes). All eight new regressions executed on each SDK.
  • Follow-up 6fe4ab1: swiftc -parse passes on the changed files. BatchOutputFiles.swift was also type-checked and executed on Linux with Swift 6.0.3 in Docker, with a Glibc shim and a stub renamex_np that forces ENOTSUP. That run checked:
    • the fallback publishes the bytes;
    • a second publish to the same name fails with EEXIST and keeps the original;
    • a dangling symlink is refused and its target is not created;
    • the private stage folder is removed;
    • plan produces image-2/-3 with matching sidecars.
  • The Swift test file itself was not compiled locally (no Xcode), so it relies on macOS CI for the new head.

Decisions taken

  • Publication is atomic per file, not a whole-batch transaction. If a later image or sidecar fails, completed files stay and the sheet reports an incomplete export.
  • The fallback runs only when the volume refuses exclusive rename. It keeps the non-replacing guarantee and gives up only atomic visibility while the file is written.
  • No overwrite option was added.

Known limitations and follow-ups

  • If the app crashes between staging and cleanup, a hidden .vitrine-export-XXXXXX folder can remain in the chosen folder. It is harmless. No automatic sweep was added, to avoid deleting anything in a user folder.
  • Items skipped after cancellation count as failed. There is no Cancel button today, so this only happens on teardown.

Scope

Independent main-based PR. No merge, release or deployment.

@johnny4young
johnny4young marked this pull request as ready for review October 6, 2026 03:50

@johnny4young johnny4young left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: preserve existing preset and carousel files

What it does: GUI multi-size and carousel exports now plan every image and sidecar name up front. Names are case-folded and NFC-normalized, and occupied or duplicate names get -2, -3, … suffixes. Each file is staged in a private mkdtemp sibling and published with renamex_np(RENAME_EXCL), so neither existing files nor files created after planning are ever replaced. A failed sidecar that was requested is now reported, and cancellation stops publication.

Verdict: ready. No blocking defects found.

What I checked

  • BatchOutputFiles.plan: it reserves the image and the sidecar together, so vitrine-x-2.png never pairs with someone else's vitrine-x-2.txt. Duplicates within the batch are handled through the shared occupied set. Invalid names (/, NUL, ., ..) are rejected.
  • publish: the mkdtemp template is built from utf8CString and decoded back correctly. The defer removes only the private stage, never a published file. A race-created destination (including a symlink) makes RENAME_EXCL fail with EEXIST and is preserved, as the test checks.
  • Ownership: writePreset now takes an injected @Sendable writer. Cancellation is checked before each image and sidecar commit.
  • The .txt sidecar name matches the old deletingPathExtension().appendingPathExtension("txt") derivation.
  • CI was green on 5fc63bf (all 5 required jobs).

Questions and nits (not blocking)

  • 🟡 Non-APFS destinations: renamex_np(RENAME_EXCL) can return ENOTSUP on some filesystems (certain SMB/AFP shares, FAT/exFAT USB sticks, depending on the macOS VFS support). If that happens, every file in the batch fails where a plain write used to work. DocumentationPackageWriter already relies on the same call, so this matches existing practice. Still, a manual test against an exFAT stick or SMB share before release would be worthwhile. A fallback could be open(O_CREAT|O_EXCL) on the destination plus a copy from the stage.
  • 🟡 If the app crashes between mkdtemp and cleanup, a hidden .vitrine-export-XXXXXX folder is left in the user's chosen folder. That's harmless, but a sweep of stale .vitrine-export-* folders older than a day, run on the next export into the same folder, would tidy it up.
  • 🟡 Items skipped after cancellation count as failed, so a cancelled batch reads "Some export files couldn't be written". No cancel button exists today, so this only happens on teardown. Worth revisiting if a Cancel button is added.

Generated by Claude Code

claude added 3 commits October 6, 2026 15:48
…ive rename

Some network and FAT-family volumes answer renamex_np(RENAME_EXCL) with
ENOTSUP, which failed every preset and carousel file there. Fall back to
an O_CREAT|O_EXCL|O_NOFOLLOW create that still never replaces or follows
an existing name and removes only its own file on a failed write. Read
errno inside the call so the reported error is the rename's own.
…e-outputs

# Conflicts:
#	CHANGELOG.md
#	Vitrine/Export/ExportManager+Batch.swift
…paths

- Never close a fallback descriptor a second time after a failed close, since a
  concurrent export may already reuse that descriptor number.
- Build the staging template with the same file-system representation used by
  the commit rename, so both name identical bytes for non-ASCII folders.
- Create fallback files with 0o666 so the umask yields the same mode as the
  rename path.
@johnny4young
johnny4young merged commit a9796ab into main Oct 8, 2026
6 checks passed
@johnny4young
johnny4young deleted the fix/gui-batch-preserve-outputs branch October 8, 2026 15:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants