Skip to content

fix: make concurrent file writes race-safe#342

Merged
pepicrft merged 1 commit intomainfrom
feat/idempotent-make-directory
May 7, 2026
Merged

fix: make concurrent file writes race-safe#342
pepicrft merged 1 commit intomainfrom
feat/idempotent-make-directory

Conversation

@pepicrft
Copy link
Copy Markdown
Contributor

@pepicrft pepicrft commented May 7, 2026

Summary

writeText, writeAsPlist, and writeAsJSON previously did the following when .overwrite was set:

if options.contains(.overwrite), try await exists(path) {
    try await remove(path)
}
// then write

The pre-check is both redundant and racy. The underlying writes (atomic write on POSIX via File.System.Write.Atomic.write with .replaceExisting, Data.write on Windows) already replace existing files atomically. Two concurrent writers could both pass the exists check, and the second remove would then fail.

Drop the dance. Behavior is preserved (writes still overwrite), the race is gone. The .overwrite enum cases are kept so the public API is unchanged.

Also extends the doc comment on MakeDirectoryOptions.createTargetParentDirectories to make full mkdir -p semantics explicit — including idempotency for an existing target directory, which the swift-file-system POSIX backend already provides — and adds a regression test pinning that contract under 50 parallel tasks racing to create the same target.

Test plan

  • New concurrency test covering writeText (mixed .overwrite/no-options) under 50 parallel writers.
  • New concurrency test covering writeAsJSON under 50 parallel writers.
  • New regression test for makeDirectory(...,[.createTargetParentDirectories]) under 50 parallel tasks racing to create the same target path.
  • swift test — 91 tests, 0 failures (macOS).

🤖 Generated with Claude Code

@pepicrft pepicrft self-assigned this May 7, 2026
@pepicrft pepicrft requested a review from fortmarek May 7, 2026 08:38
Comment thread Sources/FileSystem/FileSystem.swift Outdated
Comment on lines +74 to +80
/// When passed, it creates the parent directories if needed.
case createTargetParentDirectories
/// When passed, the call returns silently if a directory already exists at the target path.
/// This makes the operation safe to call concurrently from multiple processes or tasks
/// without check-then-act races. If a non-directory file exists at the target path the
/// call still throws.
case ignoreIfExists
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I wonder if we should align with mkdir -p where -p:

     -p            
Create intermediate directories as required.  If this option is not specified, the full path prefix of each operand must already exist.  On the other hand, with this option specified, no error will be reported if a directory given as an operand already exists.  Intermediate directories are created with permission bits of “rwxrwxrwx” (0777) as modified by the current umask, plus write and search permission for the owner.

In our case, the equivalent flag would be createTargetParentDirectories (would have personally preferred for this case to be aligned with mkdir, too, and be called createIntermediateDirectories`).

I don't feel strongly about it, though, and we can keep the ignoreIfExists flag separate.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Took your suggestion and dropped the new option. As you noted, mkdir -p semantics already include idempotency for the target dir, and the swift-file-system POSIX backend already implements that under createTargetParentDirectories. Updated the doc comment to make that contract explicit and added a regression test pinning it under parallel creation, but no new flag.

Agree on createIntermediateDirectories being a clearer name. Happy to do that rename as a separate deprecation if you want.

Comment thread Sources/FileSystem/FileSystem.swift Outdated
Comment on lines +662 to +669
if ignoreIfExists {
var isDirectory: ObjCBool = false
if FileManager.default.fileExists(atPath: at.pathString, isDirectory: &isDirectory),
isDirectory.boolValue
{
return
}
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

does it make sense to add an extra operation to check whether a file exists vs. just catching the error. We're patching the same thing twice. I'd say that in most cases, we should expect the file/directory not to exist, so would optimize for that scenario instead of eagerly checking for the file's existence.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good call, also resolved by the same change. Dropping the new option deletes this whole branch, so the extra fileExists call goes away. The remaining createDirectory flow on Windows is unchanged from before.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Pushed a follow-up that removes FileManager from makeDirectory entirely. The Windows branch was redundant: File.System.Create.Directory.create already implements both mkdir and mkdir -p on Windows (in File.System.Create.Directory+Windows.swift), and maps parentDirectoryNotFound to the same logical error we were synthesizing via FileManager.default.fileExists. Collapsing to a single platform-agnostic block.

There are still a handful of FileManager call sites elsewhere in the file (move, replace, attributes, symbolic links). Worth migrating those too, but I'd do that as a separate PR to keep this one focused.

@pepicrft pepicrft force-pushed the feat/idempotent-make-directory branch from fc8c67d to 091b7a2 Compare May 7, 2026 08:51
@pepicrft pepicrft changed the title fix: make concurrent directory and file writes race-safe fix: make concurrent file writes race-safe May 7, 2026
@pepicrft pepicrft force-pushed the feat/idempotent-make-directory branch from 091b7a2 to 5bfe077 Compare May 7, 2026 08:55
@pepicrft pepicrft changed the title fix: make concurrent file writes race-safe fix: make concurrent file writes race-safe and drop FileManager from makeDirectory May 7, 2026
@pepicrft pepicrft force-pushed the feat/idempotent-make-directory branch from 5bfe077 to b957e3a Compare May 7, 2026 09:07
`writeText`, `writeAsPlist`, and `writeAsJSON` previously did `exists`
+ `remove` + write when `.overwrite` was set. The pre-check was both
redundant and racy: the underlying writes (atomic write on POSIX,
`Data.write` on Windows) already replace existing files atomically, and
two concurrent writers could both pass the `exists` check causing the
second `remove` to fail.

Drop the dance. Behavior is preserved (writes still overwrite); the
race is gone. The `.overwrite` enum cases are kept so the public API is
unchanged.

Also extends the doc comment on `MakeDirectoryOptions.createTargetParentDirectories`
to make `mkdir -p` semantics explicit (idempotent for an existing
target directory) and adds a regression test pinning that contract
under parallel creation of the same path.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@pepicrft pepicrft force-pushed the feat/idempotent-make-directory branch from b957e3a to 73876e4 Compare May 7, 2026 09:07
@pepicrft pepicrft changed the title fix: make concurrent file writes race-safe and drop FileManager from makeDirectory fix: make concurrent file writes race-safe May 7, 2026
@pepicrft pepicrft merged commit f7d5930 into main May 7, 2026
11 checks passed
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