From 0e9ad384c49c3e8daa6e94e82b778482de0fffa0 Mon Sep 17 00:00:00 2001 From: Osvay Soma Date: Tue, 13 Oct 2026 09:35:11 +0200 Subject: [PATCH 1/6] Plan: ship 2027.1.1 with appends that end alike on every provider and a Physical folder delete no link can steer The two follow-ups 2027.1.0 left open, as a patch: an append with no condition retries after a concurrent change on every provider, as Azure Blob does, and Physical deletes a folder through handles, so no link swapped in can steer it outside the root. Two CI jobs run Physical's tests on macOS and on Linux on Arm first. Reviewed in six rounds and approved by the maintainer. Refs #65, #73, #70 --- docs/plans/2026-10-13-2027-1-1.md | 438 ++++++++++++++++++++++++++++++ 1 file changed, 438 insertions(+) create mode 100644 docs/plans/2026-10-13-2027-1-1.md diff --git a/docs/plans/2026-10-13-2027-1-1.md b/docs/plans/2026-10-13-2027-1-1.md new file mode 100644 index 0000000..836aa38 --- /dev/null +++ b/docs/plans/2026-10-13-2027-1-1.md @@ -0,0 +1,438 @@ +--- +title: Ship 2027.1.1 with appends that end alike on every provider and a Physical folder delete no link can steer +issue: "#65, #73, #70" +status: approved +approved_by: serionist +approved_at: 2026-10-13 +--- + +# Ship 2027.1.1 with appends that end alike on every provider and a Physical folder delete no link can steer + +2027.1.0 left two follow-ups open on purpose: #65, an unconditional append that ends one way on Azure Blob and another +everywhere else when another writer gets there first, and #70, a Physical folder delete that a link swapped in while it +runs can steer outside the root. Both are fixes, so they ship as a patch, 2027.1.1 +([versioning](../guide/versioning.md)). This plan spans four PRs, one per step; after its first merge each step's PR +updates its progress log and its decisions ([docs/plans/README.md](README.md)). + +## Goal + +2027.1.1 of every package is on nuget.org with: + +1. **#65:** an append with no condition of the caller's ends the same way on every provider when another writer changes + or creates the file between the provider's read and its write: the provider reads the file again and appends after + what is there now, up to five attempts in all, then throws `PreconditionFailedException`. Azure Blob does this today; + Physical and the providers that append by rewriting (`AppendByRewriteAsync`: S3, MinIO, Google Cloud Storage, Box, + SharePoint, the in-memory provider, and any third-party provider that calls it) throw at the first conflict. +2. **#70:** Physical deletes a folder through handles: it never goes into a link inside the folder, even one swapped in + while it runs, and with `RefuseLinks` it also walks down from the root without following one. On Linux and macOS + through `openat` and `unlinkat`, on Windows through `NtCreateFile` relative to the folder's handle and a delete by + handle. + +**In scope:** the two issues, their tests and docs, two CI jobs that run Physical's tests on macOS and on Linux on Arm +(D6), the changelog, and the release. + +**Out of scope:** + +- A race test of appends to an existing file. The staged conformance test (`Write_ConcurrentAppendIsNeverLost`) + exercises the retry on every provider that can detect the staged write; a real race would need a new capability for + the stores whose `IfMatch` is only checked (Physical, Box as it ships, S3Mock). +- Making `IfMatch` atomic where it is checked (Physical, Box as it ships, SharePoint, MinIO releases without conditional + writes): an append there can still lose a write that lands between the check and the rewrite, as today. +- Telling a lost answer from a real conflict (Risks, P10): no store gives a way that doesn't also drop one of two + identical appends. +- Making Physical's other members atomic with the `RefuseLinks` check. A write, a read, a move, a copy or a file delete + still checks the path just before it acts, as the 2027.1 docs say; #70 is about the folder delete, which walked a whole + tree. +- Listing a folder through its handle (D4). +- A delay between attempts. Azure retries at once; so do the others. +- The live suites. Their accounts come later. Each step's PR says they didn't run. + +## Decisions + +Decided while planning. The maintainer's approval of this plan approves them. + +- **D1. An append with no condition retries, on every provider (#65; the maintainer's choice, 2026-10-13).** After a + concurrent change or create, the provider reads the file again and appends after what is there now: up to five + attempts in all (Azure's `MaxAttempts`), then `PreconditionFailedException`, the base type whatever the last conflict + was, with Azure's message ("kept changing while it was appended to; nothing was appended after 5 attempts") and the + last conflict inside. A caller's own `IfMatch` or `IfNotExists` still fails at once, as on Azure: the caller asked for + that version, or for nothing to be there. A file deleted between the read and the write counts as a change too: the + next attempt creates it, as Azure does. A folder that takes the name gets the store's answer, unretried. Only the + provider's own conflict signals go round again (P3): an exception from the caller's stream, whatever its type (a + pinned read from another provider throws `PreconditionFailedException`), ends the append at once. + - *Rejected: retry only the create race.* An existing file's conflict would still end differently (Azure retries, + the others throw at once), so "one meaning" would hold for one case only. + - *Rejected: Azure refuses.* Azure's appends would get less robust, and a caller that gave no condition would get a + create-only error. +- **D2. The caller's stream is read once, and a retry costs nothing until it happens (P4).** + - **`AppendByRewriteAsync`** reads the file, then the caller's stream, into its temp file, as today, and remembers + where the caller's bytes start. A retry makes a new temp file of the file as it is now, then copies the previous + temp file from that offset, or the caller's stream while no temp file holds its bytes: no spool, no extra copy + without a conflict, and nothing in the system temp folder that isn't there today. Reading the file before the caller's stream keeps the order the staged tests rely on (the write + they stage lands after the read, so the retry is what they exercise). + - **Physical** deletes its commit temp file when the commit fails, so it copies the caller's stream into a spool of + its own once the append's early refusals have passed and before the file is opened (N2): a hidden + `.wfp-.tmp` in the target's folder, deleted when the append ends; on Linux and macOS private to the process's + user (`0600`, set as it is created, as `CommitAsync` sets its temp file's mode, U1); on Windows with the folder's permissions, as the commit's temp file has there. Listings hide it and + `DeleteTempFilesAsync` sweeps one a crash leaves, as for every temp file of a write, so an append needs nothing + outside the root (N8). Each attempt rewinds the spool and copies it after the file. The appended bytes are written + twice locally (Risks). +- **D3. A folder delete acts only through handles (#70).** The walk opens each entry relative to its folder's handle + without following a link, and removes it relative to that handle, or through its own handle on Windows: + - **Linux:** `openat(folder, name, O_PATH | O_DIRECTORY | O_NOFOLLOW | O_CLOEXEC)` succeeds only on a real folder. + `ENOTDIR` (or `ELOOP`) means a file or a link, which `readlinkat(folder, name, buffer, 1)` tells apart (a link + answers; anything else is `EINVAL`), with no structure layouts (P6). A file or a link is removed with + `unlinkat(folder, name, 0)`; a folder is emptied, then removed with `unlinkat(folder, name, AT_REMOVEDIR)`. `O_PATH` + needs no read permission, as `unlinkat` doesn't. + - **macOS:** the same with `O_RDONLY` for `O_PATH`, which macOS lacks, and the same `readlinkat` after either error, + since which of the two a link gives there is not something this plan relies on (N5). A folder on the way must be + readable (Risks). + - **Windows:** `NtCreateFile` with the folder's handle as its root, `FILE_OPEN_REPARSE_POINT`, + `FILE_SYNCHRONOUS_IO_NONALERT` and `FILE_OPEN_FOR_BACKUP_INTENT`, for `DELETE | FILE_READ_ATTRIBUTES | SYNCHRONIZE` + (the attributes query needs `FILE_READ_ATTRIBUTES`; synchronous I/O needs `SYNCHRONIZE`), sharing read, write and + delete, as .NET's relative opens and `DeleteFile` do (P7). Only the entries the walk removes ask for `DELETE`: the + folder the walk starts from and, with `RefuseLinks`, each folder on the way are opened for + `FILE_READ_ATTRIBUTES | SYNCHRONIZE`, so the delete needs no more right on them than today's path delete (R3). `GetFileInformationByHandleEx(FileAttributeTagInfo)` + gives the attributes and the reparse tag. Each entry is deleted through its own handle with `FileDispositionInfoEx` + (`FILE_DISPOSITION_FLAG_DELETE | FILE_DISPOSITION_FLAG_POSIX_SEMANTICS`), and where the file system answers + `ERROR_INVALID_PARAMETER`, `ERROR_NOT_SUPPORTED` or `ERROR_INVALID_FUNCTION`, with `FileDispositionInfo`, as Rust's + `remove_dir_all` does; any other error is the delete's failure. + - **What an entry is** is decided in shared code from those raw facts (D7): a folder (the directory attribute, and no + name-surrogate reparse tag), a link (a name surrogate on Windows; a `readlinkat` answer on Unix), + something else, or nothing (gone). A link is removed as a link, never entered, as .NET's recursive delete does; on + Windows that includes junctions, which .NET 10 fails on today (D5). + - **The start:** without `RefuseLinks`, the folder that holds the target is opened by its path, following links as + every member does; with it, the walk starts at the root (opened by its path, since the root may be a link) and opens + each folder on the way without following one. A link found there, swapped in after the check, is refused with the + same `ArgumentException` as the check's; a file or nothing there means the folder is gone, and the delete returns + false (P12). The target itself is opened without following: a link there is removed as a link, as today (with + `RefuseLinks`, refused); a file there, put in its place since the probe, is deleted, since the member deletes what + the path names (N6); nothing there returns false. `Opening` fires for each folder on the way and for every entry. + - **The file delete** is unchanged (`File.Delete` on the path), as is what `DeleteAsync` returns: the probe that + decides between a file, a folder and nothing stays, so a link to nothing still returns false. + - *Rejected: holding each folder open by path on Windows (`CreateFileW`) instead of `NtCreateFile`.* NTFS refuses to + rename a folder while a handle below it is open (checked on this machine), but the safety would then rest on that + rule of one file system; the relative open rests on nothing. .NET opens folders relative to a handle the same way + in its recursive enumeration, and Rust's `remove_dir_all` deletes this way since CVE-2022-21658. +- **D4. Folders are listed by their path; only listing does.** The names come from `Directory` enumeration of the + folder's path, and every open and remove goes through handles. A listing that a rename or a link swapped in made read + another folder can only make the delete miss names, which then fails (the folder isn't empty: `IOException`), never + delete outside the folder. + - *Rejected: listing through the handle.* `readdir`'s `dirent` differs between glibc, musl and macOS (with + `$INODE64` symbols on Intel Macs), and Windows would need `FILE_FULL_DIR_INFO` parsing: more native code for + completeness under attack, not for safety. +- **D5. Errors as .NET's recursive delete gives them.** The walk goes on past an entry it can't delete and throws the + first failure at the end; an entry gone meanwhile is skipped; access refused is `UnauthorizedAccessException`, + anything else `IOException`, naming the provider path. Two differences, both fixes: on Windows a folder holding a + junction is deleted (today `UnauthorizedAccessException`, from .NET's `DeleteVolumeMountPoint` on the junction), and + where a folder still has entries after the walk (a file another process holds open on a file system without POSIX + deletes, such as FAT or a network share) the delete throws `IOException` instead of returning true with the folder + still there. +- **D6. The native constants per platform, and CI on each.** Picked once from `OperatingSystem` and + `RuntimeInformation.ProcessArchitecture` (an x64 process emulated on Arm needs the x64 values, P7), as a table: + + | | Linux x64 | Linux Arm, Arm64 | macOS x64, Arm64 | + |---|---|---|---| + | `O_DIRECTORY` | 0x10000 | 0x4000 | 0x100000 | + | `O_NOFOLLOW` | 0x20000 | 0x8000 | 0x100 | + | `O_PATH` | 0x200000 | 0x200000 | none (`O_RDONLY`, 0) | + | `O_CLOEXEC` | 0x80000 | 0x80000 | 0x1000000 | + | `AT_REMOVEDIR` | 0x200 | 0x200 | 0x80 | + | `ENOENT`, `EACCES`, `EPERM`, `ENOTDIR`, `EINVAL` | 2, 13, 1, 20, 22 | same | same | + | `ELOOP`, `ENOTEMPTY` | 40, 39 | 40, 39 | 62, 66 | + + What each error means to the walk (N5): `ENOENT`, gone (skipped, or false at the start); `EACCES` and `EPERM`, refused + (`UnauthorizedAccessException`); `ENOTDIR` and `ELOOP` from an open, not a folder (then `readlinkat`); `EINVAL` from + `readlinkat`, not a link; `ENOTEMPTY`, not empty (`IOException`); anything else, `IOException` with the system's + message. The Linux values were run on x64 and on Arm64 under emulation here. `open` and `openat` are declared with + their fixed arguments only (two and three); `O_CREAT` is never set, so the variadic mode is never read. Anywhere else (another Linux + architecture, another system) the delete is `Directory.Delete`, as today, and the docs say so. CI gains two jobs that + build and run Physical's tests: macOS (`macos-latest`, Arm64) and Linux on Arm (`ubuntu-24.04-arm`). They are not + required checks in the ruleset, but they are in `ci.yml`, whose success `release.yml` requires on the tagged commit: + a red one blocks a release, as the Windows job does (P8). +- **D7. Only Windows' raw calls are left out of the coverage measure (P5).** CI measures coverage on Linux only, where + Windows' calls can't run. So the seam between the walk and the system returns raw facts (a handle; the attributes and + reparse tag; the system's error number), and offers both of Windows' deletes; everything that decides is shared code, + run and covered on Linux: what an entry is, which error means what, when to fall back from the POSIX delete, and the + table of D6 (a unit test pins which column each system and architecture picks, on any system), and the `\\?\` form + of a path, for a drive, a drive's root and a share (N3). What stays unmeasured is `WindowsFolders`, the class that + holds the `LibraryImport` declarations and the wrappers that marshal their arguments, about forty lines with no branch + but the success test of each call, marked `[ExcludeFromCodeCoverage]` with the reason (`tests/coverage.runsettings` + already honours it). The Windows job runs every line of it: a test switch makes + Physical use the classic delete, which NTFS would otherwise never need. + - *Rejected: measuring coverage on Windows too and merging line by line.* The gate keeps each assembly's best figure + across reports; merging lines would change what the gate means for every assembly (Common's lines run by provider + tests would count), and move it out of the Linux job and `release.yml`. +- **D8. Release 2027.1.1 (the maintainer's choice, 2026-10-13).** A patch: both changes are fixes. An rc first, as for + 2027.1.0, from the same commit. The approval of this plan covers approving the `nuget` environment for both tags. + +## Design + +### Appends (#65) + +**Common, `AppendByRewriteAsync`.** A loop of at most five attempts: + +1. Read the file's info; check the caller's conditions against it, as today. +2. Make the attempt's temp file: the file's content when it is one (read with `ReadFileCoreAsync`), then the caller's + bytes: from the caller's stream while no temp file holds them, else from the previous temp file at the offset where + they start (N1). Only a temp file that holds them replaces the previous one, which is then disposed, and its offset + is remembered; one whose read of the file failed is disposed and the previous kept. +3. Rewrite from position 0, with `IfMatch` on the version read, or with `IfNotExists` where there was no file. + +An unconditional append goes round again on `PreconditionFailedException` (the subtype included) from the rewrite, and +on `FileNotFoundException`, or an exception `IsNotFound` recognises (P2), from reading a file deleted since its info was +read. Nothing else is caught: an exception from the caller's stream, read in step 2 outside those catches, ends the +append. On the fifth conflict it throws D1's exception. The XML docs say so. + +**Physical.** `UpdateFileCoreAsync`'s append runs the same loop around today's two paths. Each attempt looks at the path +again, as today's single attempt does: a folder there, whether a file is there, then `IsReplaceable` and the mode the +new file takes (R2). Its refusals come in today's order: a folder; the caller's `IfNotExists`; a file `IsReplaceable` +refuses; the caller's `IfMatch`, on that look's version (R1). The first attempt to pass them spools the caller's stream +(D2; a spool it may not create is reported for the file, as `CommitAsync` reports its temp file), before the file is +opened (S1); an existing file is then opened and the caller's `IfMatch` checked again on the opened handle's version, as +today. A refusal on any attempt ends the append; `IsReplaceable`'s not-found is no refusal but a retry signal (S1). The +loop retries only on Physical's own signals, raised as a private marker that it unwraps (P3): + +- on an existing file, the version check before the rename (`Changed`) and a file deleted since the probe (`OpenRead`'s + or `IsReplaceable`'s `FileNotFoundException` or `DirectoryNotFoundException`, N7, R2); +- on a missing file, the name taken meanwhile (`AlreadyExists` through `taken`). + +The commit's write copies the rewound spool where it copies `contents` today, after the file's handle is read and +closed, so Windows still renames over a file nobody holds open. A folder that takes the name stays `IOException`. The +spool is the one temp file of the append that `TempCreated` doesn't report, so the tests stage their write from +`TempCreated`, which fires for each attempt's commit, and from a new internal hook, `Probed`, called after each +attempt's look at the path (a folder, a file) and before `IsReplaceable` (R2, S1). + +**Azure Blob** is unchanged; its docs already say this. **The other providers** call `AppendByRewriteAsync`, so they +change with it; their docs and tests follow. + +### The folder delete (#70) + +`DeleteCoreAsync` keeps its probe: a file is deleted by path as today; a folder goes to `DeleteFolder`; nothing there +returns false. Where the walk can't run (D6), `DeleteFolder` is today's `Directory.Delete(path, recursive: true)`. + +- **`FolderDelete`** (new file, internal, shared): the start (D3), the walk, what an entry is, the errors (D5), the + fallback between Windows' deletes, the test hook `Opening`, and the seam to the system: + - open a folder by its path, following links: a handle, or an error number; + - open an entry in a folder without following a link: a handle, the facts of D3 (Windows: attributes and tag; Unix: + whether it is a folder, and for a non-folder whether it is a link), or an error number; + - remove an entry (Unix: by folder and name, as a folder or not; Windows: through its handle, with the POSIX + delete or the classic one): 0 or an error number. +- **`UnixFolders`:** the seam through `open`, `openat`, `readlinkat`, `unlinkat` (`LibraryImport` from `libc`, + `SetLastError`, as `link` is), with D6's table. An fd is a `SafeFileHandle`, which closes it. +- **`WindowsFolders`** (`[ExcludeFromCodeCoverage]`, D7): `CreateFileW` with `FILE_FLAG_BACKUP_SEMANTICS` for a folder by + its path, given in the `\\?\` form that shared code builds (N3), so long paths and any name work; `NtCreateFile` + relative to a folder's handle, with `RtlNtStatusToDosError` for its status; + `GetFileInformationByHandleEx(FileAttributeTagInfo)`; `SetFileInformationByHandle` with either disposition class. Checked on this machine before planning: a junction is + removed by handle and its target kept; a file open for reading with delete sharing loses its name at once; a + read-only file is refused with access denied, as `DeleteFile` refuses it. +- **Seams for the tests:** `DeletesByHandle` (true where D6 says the walk runs; a test sets false to cover the + fallback), `Opening`, the classic-delete switch (D7), and the seam itself, which a fake implements to drive what a + disk can't stage: an entry that turns from a folder into a file between the listing and the open, a removal refused, + a folder not empty, each Windows error that falls back and one that doesn't. + +### CI + +Physical's tests have never run on macOS, so the two jobs come before #70, in their own PR (step 2, #73), which fixes or +marks inconclusive, with the reason, whatever they find before the walk changes anything (P9, N9). Each is a checkout like the +others (`persist-credentials: false`, actions pinned by SHA), the SDK from `global.json`, a locked restore of +`tests/WriteableFileProviders.Physical.Tests`, and its tests (`TestCategory!=Live`): `macos-latest` and +`ubuntu-24.04-arm`, 20 minutes each. + +### The release + +As for 2027.1.0 ([releasing](../releasing.md)): a PR dates the `2027.1.1` section; `v2027.1.1-rc.1`, then `v2027.1.1`, on +its merge commit; the `nuget` environment approved for each; the packages checked on nuget.org; #65 and #70 closed by +their PRs, with the version noted on each. + +## Compatibility + +No public or protected member is added, removed or renamed, and the configuration shape and the gateway are untouched. +Package validation against 2027.0.0 stays green. + +- **#65, behaviour:** on Physical, S3, MinIO, Google Cloud Storage, Box, SharePoint and a third-party provider that + calls `AppendByRewriteAsync`, an unconditional append that met a concurrent change or create now succeeds, after the + other writer's content, where it threw `PreconditionFailedException` or `FileAlreadyExistsException`. It throws + `PreconditionFailedException` only after five attempts. A caller that caught the exception to retry keeps working, and + now rarely gets there; a caller that relied on an unconditional append failing to learn that someone else created the + file should pass `IfNotExists`, as the docs say. +- **The Common API door (P1):** `AppendByRewriteAsync` is a protected helper of the base class, so it is inside the + door. Its signature and its promise (a concurrent write is never lost) stay; what changes is that it retries where it + threw. This plan reads that as a fix toward the door's own rule, one meaning across providers, which the difference + broke, and so as a patch. The maintainer's agreement is posted on #65 with this plan's approval, as it was on #59 for + 2027.1; step 1's PR says so in its title ("one-way door: the Common API") and links it under One-way doors (N4). +- **#70, behaviour:** a Physical folder delete never goes into a link, as .NET intends today, now also when one is swapped + in. Visible differences: on Windows, a folder holding a junction is deleted where it threw (D5); a folder that still + has entries after the walk throws `IOException` where it returned true (D5); on macOS, the folder that holds the + deleted folder, and with `RefuseLinks` every folder on the way, must be readable (Risks). The file delete is unchanged. + +## Steps + +Each step is one PR from a branch `-` off `origin/main`, through `review-code` and `finish-work`. Each adds +its changelog lines under `## 2027.1.1` and its progress log entry here. Step 1's branch carries this plan as its first +commit. + +| # | Issue | What | The tests that prove it | +|---|---|---|---| +| 1 | #65 | D1 and D2 in `AppendByRewriteAsync` and Physical; the docs that describe appends; the changelog. | Common (`BaseClassTests`; the stub provider gains a scripted store, per-call infos and failures, P13): a create that loses once to another writer appends after that writer's content; a rewrite that fails once on an existing file is read again and appended after the new content, and the caller's stream is read once; a rewrite that always fails throws exactly `PreconditionFailedException` after five attempts, the last failure inside; a file deleted between its info and its read is created, whether its read throws `FileNotFoundException` or the stub's own not-found; a caller's stream that throws `PreconditionFailedException` ends the append after one attempt; a conflict on the first attempt, then a not-found read on the second, still appends the caller's bytes once (N1); with the caller's `IfMatch` or `IfNotExists`, one attempt and the caller's failure. Conformance: `Write_AppendRaceToAMissingFileKeepsBoth` expects both appends to succeed and the file to be both parts, in either order; `Write_ConcurrentAppendIsNeverLost` expects the append to succeed and the file to be the other writer's content, then the append. Physical: the two staged tests (`Append_FailsWhenTheFileChangesWhileItCopies`, `Append_ToAMissingFileFailsWhenOneAppearsMeanwhile`, renamed for what they now prove) stage their write from `TempCreated` and expect the append after the other content, over their rows; a file changed on every attempt gives the exception after five; a caller's `IfMatch` with a staged change fails at once; a caller's stream that throws `PreconditionFailedException` is not retried; a refused append (a caller's `IfMatch` on a missing file in a missing folder) leaves no folder behind (N2); a file deleted between the look at the path and `IsReplaceable` (through `Probed`) is created with the caller's bytes (N7, S1); after a lost create, the next attempt takes the existing file's path, checks it with `IsReplaceable` and gives the new file its mode (R2); a file changed between the probe and the open under a caller's `IfMatch` is refused (R1); the spool is hidden from listings, gone afterwards, and swept when left; on Linux and macOS the spool, found from `TempCreated` beside the commit's temp file, is `0600` (T1); `PhysicalConformanceTests`' comment on `DetectsConcurrentAppends` says what is now true (N2). The provider tests that expected an append to throw on a staged change (GCS, SharePoint, and Azure's where they differ) expect the retry. Emulators: Azurite, S3Mock, MinIO, fake-gcs-server; FakeBox and GraphFake offline. | +| 2 | #73 | The two CI jobs (D6, P9), and whatever the macOS and Arm runs of today's Physical tests find; `AGENTS.md`, `REVIEW.md`, `CONTRIBUTING.md` and `release.yml`'s comments on what CI runs. | The jobs themselves, green on the PR. | +| 3 | #70 | D3 to D7: `FolderDelete`, `UnixFolders`, `WindowsFolders`; the Physical README, the `RefuseLinks` and class remarks, `AGENTS.md`; the changelog. Its `review-code` runs at the deep tier (the maintainer's choice). | Physical, on Linux (x64 in CI and in a container here, Arm64), Windows and macOS: through `Opening`, a subfolder swapped for a link to a folder outside the root while its parent is deleted: the link goes, the outside folder keeps everything, the delete returns true; with `RefuseLinks`, a folder on the way swapped for a link after the check gives `ArgumentException` naming `RefuseLinks` and the outside folder is untouched, and one swapped for a file returns false; a target folder replaced by a file since the probe is deleted; a link at the target (to a folder inside and outside the root) is removed and its target kept, as today; a folder holding a link to a file, a link to nothing and (Windows) a junction is deleted, the targets kept; a file open for reading with delete sharing inside a deleted folder; a folder the process may not empty (a read-only file on Windows, a folder without write permission on Linux, not as root) throws `UnauthorizedAccessException` and deletes the rest; the folder that holds the target, and with `RefuseLinks` each folder on the way, held open by another handle that doesn't share delete (Windows), doesn't stop the delete (R3); `DeletesByHandle` false keeps today's delete; the classic-delete switch deletes the same tree (Windows). Through the fake: an entry that turns from a folder into a file, an entry gone between the listing and the open, a removal refused, a folder not empty, each fallback error and one that isn't. D6's table: which column each system and architecture picks, on any system; the `\\?\` form of a drive path, a drive's root and a share. The conformance suite's delete tests run through the walk on every system. | +| 4 | #65, #70 | Release: date the changelog, tag `v2027.1.1-rc.1`, then `v2027.1.1`, and approve the `nuget` environment for each ([releasing](../releasing.md)). | `release.yml`: tests, coverage gate, package check, CI on the tag (the new jobs included), trusted publishing. A restore of 2027.1.1 from nuget.org. | + +## Tests + +- **Offline,** on every PR: the Common, AspNetCore, Box and SharePoint suites, and the offline S3, MinIO, Azure and GCS + tests. +- **Emulators** in CI on Linux: Azurite, S3Mock, MinIO and fake-gcs-server, which run the append tests on every store. +- **Physical** on Linux (x64 in the main job, Arm64 in the new one), Windows and macOS. Here, Physical's tests also run + on Linux in a container before each push, as for 2027.1. +- **Coverage:** the gate keeps every floor (D7 for Windows' raw calls), and a floor that rises is raised in the PR that + earns it. +- **Live:** none run (their accounts come later). + +## Docs + +- **XML docs:** `AppendByRewriteAsync`; Physical's class remarks (appends and their spool, the folder delete), + `RefuseLinks`' remarks (the recursive delete's caveat goes, the new rule comes); Azure's remarks are already right. +- **The guide:** + - `reading-and-writing.md`: Append: the retry, that a caller's condition fails at once, that an append whose answer + was lost and was sent again can land twice (P10), and that on Windows Physical's appends to one file can still + collide (S2); + - `providers.md`: the paragraph on how an append to a missing file ends goes, but its caveat stays, that where + `IfNotExists` is only checked a file created in between can be replaced (P11); the Append column's note; + - `errors.md`: `FileAlreadyExistsException` no longer comes from an append without `IfNotExists`; + `PreconditionFailedException` from an append only after five attempts; + - `writing-a-provider.md`: `AppendByRewriteAsync` retries. +- **`docs/migration-2027.md`:** the rows "A racing append lost a write" and "A racing append or move went through" say + the append is now read again and retried (P11). +- **The conformance suite:** `DetectsConcurrentAppends`' doc says the append retries (P11). +- **Package READMEs:** Physical (appends and their spool; the folder delete and where it runs; `RefuseLinks`), Amazon S3, + MinIO, Google Cloud Storage, Box and SharePoint (appends retry), Azure Blob as needed. +- **What CI runs** (step 2, P8): `AGENTS.md` (the CI and Release bullets, the Physical tests line), `REVIEW.md` (Build + and tests), `CONTRIBUTING.md`, and `release.yml`'s comments ("the Windows build and tests"). +- **`CHANGELOG.md`:** a `2027.1.1` section, dated at release. + +## Risks + +| Risk | Status | +|---|---| +| A wrong flag for some platform: `O_NOFOLLOW` or `O_DIRECTORY` wrong would follow links again (with `O_PATH` the kernel ignores the flags it doesn't know). | Mitigated: the values come from the kernel's and Darwin's headers, were run on Linux x64 and Arm64 (emulated) here, are picked by `ProcessArchitecture`, and CI runs Linux x64 and Arm64, macOS Arm64 and Windows, where the link tests would catch a wrong value; a unit test pins which column each platform picks. Linux Arm (32-bit) and Intel Macs share those values and run nowhere in CI. Any other platform keeps today's delete. | +| The macOS and Arm jobs find failures in today's Physical tests that have nothing to do with #70. | Mitigated: they land first, in their own PR (step 2), which fixes them or marks them inconclusive with the reason. | +| The listing reads another folder after a rename or a swapped link (D4). | Accepted: the delete can then miss names and fail with `IOException`; it never acts outside the folder it opened. | +| On macOS, a folder on the way that the process may write but not read now refuses a folder delete (`UnauthorizedAccessException`), where a path delete only needed search permission. | Accepted and documented: Linux opens without reading (`O_PATH`); macOS has no equivalent we can count on. | +| Windows' raw calls are measured by no coverage report (D7). | Accepted, with the maintainer's yes: they hold no decision, the Windows job runs every line, and the reason is on the attribute. | +| A volume mounted in a folder inside a deleted folder on Windows (only an administrator can mount one). | Accepted, unverified: it is a name surrogate, so its folder is removed by handle like a junction, never entered; doing that needs elevation, so no test stages it. Where Windows refuses, the delete throws `IOException`. | +| `NtCreateFile` is an ntdll export, not a Win32 API. | Accepted: documented in the Windows driver docs, stable since NT, and what .NET calls to open folders relative to a handle in its own enumeration. | +| A store applies a rewrite, its answer is lost, and the SDK sends it again (AWS and Azure SDK retries, Kiota's 503/504, Box's 5xx, an NFS retransmit for Physical): the resend fails on the provider's own write, the loop reads that as a conflict, and the append lands twice (a create by append doubles its content). | Accepted, with the maintainer's yes (P10): Azure does this today, and a caller who retries as the 2027.1 docs say does it too; the guide says so. Telling the two apart by comparing content would drop one of two identical appends, which is worse. Google Cloud Storage's resumable upload is not exposed. | +| An append under heavy contention rewrites the file up to five times (each attempt uploads the whole file on the rewrite providers). | Accepted: a write that would have thrown now usually succeeds; the cap bounds the cost; Azure does the same. | +| Physical writes the appended bytes twice locally (D2): a create by append writes its content twice. | Accepted: an append already copies the whole file; appends are small (logs) in practice. | +| On Windows, which doesn't rename over a file that is open, two appends to one existing file can still collide: one's rename meets the other's read handle and throws `UnauthorizedAccessException`, unretried, where Azure would land both (S2). | Accepted, as today and documented in Physical's remarks: the refusal can't be told from a real access refusal. The Append docs keep the Windows caveat. A rename with POSIX semantics might lift it later (unverified). | +| A retried append is a different write from a caller's point of view: two appenders' bytes now both land where one used to fail. | Wanted (D1); the changelog says so. | +| A third-party provider's own tests expected `AppendByRewriteAsync` to throw at the first conflict. | Accepted: the changelog says what changed. | +| nuget.org refuses a package. | Mitigated: the package check and the rc first. | + +## Plan review + +**Round 1:** standard tier, Opus, 2026-10-13. One reviewer applied every lens and returned 13 findings, verdict +needs-fixes, and suggested a targeted deep pass on the #70 half after the edits. Each major or sensitive finding (P1, +P3, P4, P5, P6, P7, P8, P10) then met its own refuter: P1, P3, P5, P6 and P8 confirmed; P4, P7 and P10 confirmed in +part, with corrections folded in below. The maintainer answered the three questions (P4, P5, P10) as proposed, and +chose to spend the deep tier on step 3's code review rather than on this plan. + +| # | Severity | Section | Finding | Edit | Status | +|---|---|---|---|---|---| +| P1 | major, sensitive (door) | Compatibility | The plan said the maintainer's decision was recorded on #65; #65 has no comment. `AppendByRewriteAsync` is a protected helper, inside the Common API door, and its documented meaning changes (Core.cs:342-345). AGENTS.md asks for the agreement in the issue; #59 set the precedent. | The agreement is posted on #65 once the plan is approved, and step 1's PR links it. | fixed | +| P2 | minor | Design, Common | The loop caught only `FileNotFoundException` from the read; a third-party provider's read may throw its store's own not-found, which `IsNotFound` recognises only at the boundary (the stub throws `KeyNotFoundException`). | Also what `IsNotFound` recognises; a test with the stub's own not-found. | fixed | +| P3 | minor | D1, Design, Physical | Physical made its spool inside the commit, so a caller's stream that throws `PreconditionFailedException` (a pinned read from another provider) would be caught as a conflict and read again; and no section said the spool is rewound before each attempt (S3 reads from the position). Common's half refuted: its catches wrap only the rewrite and the file's read. | Physical spools first and retries only on its own signals, through a private marker; each attempt rewinds; tests for both. | fixed | +| P4 | minor, sensitive (data), owner call | D2, Risks | The spool in the system temp folder put Physical's appended bytes outside the root, beyond the sweep after a crash, and made Physical's appends need a writable system temp folder (a read-only container); the rewrite providers would also copy the appended bytes twice. | Common reuses the previous attempt's temp file from the offset of the caller's bytes (no spool); Physical spools to a hidden `.wfp-.tmp` in the target's folder. | fixed (the maintainer's yes, 2026-10-13) | +| P5 | major, sensitive (harness) | D7, Design | D7 excluded `WindowsFolders` as holding no decisions, but the seam returned classified results, so it would decide what is a link, map the errors and choose the fallback, which never runs on NTFS; and the macOS and Arm constant branches never run on Linux x64 (branch floor 93). | The seam returns raw facts and offers both deletes; shared code decides, covered on Linux; a switch makes the Windows job run the classic delete; a unit test checks the table; D7 names what stays unmeasured. Needs the maintainer's yes (the coverage check). | fixed (the maintainer's yes, 2026-10-13: the raw calls only) | +| P6 | minor, sensitive (security) | D3, Design | On Linux, `O_PATH \| O_DIRECTORY \| O_NOFOLLOW` gives `ENOTDIR` for a file and a link alike, so "a link" and the `RefuseLinks` refusal on the way couldn't be told from a file. Fails closed either way. | `readlinkat(folder, name, buffer, 1)` after `ENOTDIR`. | fixed | +| P7 | minor, sensitive | D3, D6, Risks | Unstated native details: the access mask (the tag query needs `FILE_READ_ATTRIBUTES`), the share modes, the fallback codes, macOS's `O_CLOEXEC` and error numbers, `ProcessArchitecture`, the three-argument `open`. Refuted in part: the variadic declaration was already right, and .NET does call `NtCreateFile` relative to a handle (its enumeration), though not for deletes. | All written into D3 and D6; the Risks line reworded, with Rust's `remove_dir_all` as the delete's precedent. | fixed | +| P8 | minor, sensitive (harness) | D6, Docs | The new jobs aren't required in the ruleset, but `release.yml` requires the whole `ci.yml` run to succeed; and `AGENTS.md`, `REVIEW.md`, `CONTRIBUTING.md` and `release.yml`'s comments describe what CI runs. | D6 says a red job blocks a release; those docs are listed; the jobs copy the others' checkout. | fixed | +| P9 | minor | Steps, Risks | Physical's suite has never run on macOS, so unrelated failures would land in #70's PR. | The jobs come first, in their own PR (step 2). | fixed | +| P10 | minor, sensitive (data) | Risks | A rewrite whose answer is lost and which the SDK resends fails on its own write; the loop then appends the same bytes again. Confirmed for S3, MinIO, Azure (today), SharePoint's simple upload, Box's create and Physical over NFS; refuted for GCS's resumable upload. A content compare would drop one of two identical appends. | A Risks row and a line in the guide; the maintainer accepts it by name. | fixed (accepted by the maintainer, 2026-10-13) | +| P11 | nit | Docs | `providers.md`'s checked-`IfNotExists` caveat must stay; `DetectsConcurrentAppends`' doc and `migration-2027.md`'s racing-append rows change. | Listed in Docs. | fixed | +| P12 | nit | D3 | The plan didn't say what the delete returns when the target, or a folder on the way, is gone or a file by the time the walk opens it; nor that `Opening` fires for the folders on the way. | False for gone or a file; `Opening` fires for both. | fixed | +| P13 | nit | Steps | The Common tests need a store that changes between attempts; the stub is static. | The stub gains a scripted store. | fixed | + +**Round 2:** standard tier, Opus, 2026-10-13, on the edited sections. P1 to P13 verified in the text, P12 in part +(the target itself, N6). No blocker or major. It noted that P1, P6, P7 and P8 touch sensitive areas and were marked fixed +without a recorded yes; they are asked with the approval. + +| # | Severity | Section | Finding | Edit | Status | +|---|---|---|---|---|---| +| N1 | minor | Design, Common | A not-found read on the first attempt leaves no temp file holding the caller's bytes, and on a later one the previous temp file was disposed too early. | The caller's bytes come from the stream while no temp file holds them; a temp file replaces the previous only once it holds them; a test of a conflict, then a not-found read. | fixed | +| N2 | minor | D2, Design, Physical | Spooling before the probe would create the folder chain for an append then refused (a caller's `IfMatch` on a missing file), read the caller's stream for nothing, name the spool in an access error, and make the fixture's comment wrong. | The spool comes after the early refusals and before the file is opened; its access error names the file; tests; the comment. | fixed | +| N3 | minor, sensitive (harness) | D7, Design | Building the `\\?\` form (a drive or a share) is a branch inside the class D7 leaves unmeasured. | Shared code builds it, with unit tests for a drive, a drive's root and a share. | fixed | +| N4 | nit | Compatibility | AGENTS.md asks a door's PR to say so in its title; the agreement should come with the approval. | Posted with the approval; step 1's title says so. | fixed | +| N5 | nit | D3, D6, Risks | `open` has two fixed arguments; macOS's error for a link under `O_DIRECTORY` was never run; the table didn't say what each error means; a unit test can only pin which column is picked. | Fixed arguments "two and three"; `readlinkat` after either error on both systems; what each error means; `EROFS` dropped (it is "anything else"); the Risks row reworded. | fixed | +| N6 | nit | D3 | A target folder replaced by a file since the probe was unstated. | It is deleted; a test. | fixed | +| N7 | nit | Design, Physical | `IsReplaceable`'s not-found was not a retry signal. | Added, with a test. | fixed | +| N8 | nit | D2 | "Nothing but its root" was too broad (local streams use the system temp folder); `0600` holds on Unix only. | "An append needs nothing outside the root"; Windows' permissions said. | fixed | +| N9 | nit | Steps, CI | Step 2 had no issue; "first" was wrong. | #73 opened and named; "before #70". | fixed | + +Refuted in round 2: the rewrite closing the temp file between attempts (`UpdateFileCoreAsync` leaves it open); the sweep +removing an open spool (an open fd keeps it on Unix, an open handle refuses the delete on Windows); the rename-over-open +rule (the spool is another file); the error numbers (all right; `EISDIR`, `EBUSY` and `EEXIST` are "anything else"); +whether the staged conformance test still exercises the retry (yes, where it runs); SharePoint's `If-Match` on a deleted +file (retried); the Windows access, share and fallback text. + +**Round 3:** standard tier, Opus, 2026-10-13, on N1 to N9's edits. N1, N3, N4, N5, N6, N8 and N9 verified; N2 and N7 +in part (R1, R2). No blocker or major. + +| # | Severity | Section | Finding | Edit | Status | +|---|---|---|---|---|---| +| R1 | minor, sensitive (conditions) | D2, Design, Physical | The plan put the caller's `IfMatch` among the refusals before the spool, but today an existing file's `IfMatch` is checked on the opened handle, after `IsReplaceable` and the open; dropping that check would let a change between the probe and the open through. | `IfMatch` checked on the probe's version before the spool, and still on the handle; `IsReplaceable` before both; a test. | fixed | +| R2 | minor, sensitive (data) | Design, Physical | The probe and `IsReplaceable` sat outside the loop, so a retry after a lost create would repeat the missing file's path, skip `IsReplaceable` and keep the old mode; N7's test had no hook; `IsReplaceable` can also give `DirectoryNotFoundException`. | Each attempt probes again; the spool once, in the first; a refusal later ends the append; the `Probed` hook; both not-founds. | fixed | +| R3 | minor | D3, Windows | Folders on the way and the start folder were opened for `DELETE`, which they don't need: it asks for the delete right on each and fails while another process holds one without sharing delete (a shell's current folder). | Only removed entries ask for `DELETE`; the others `FILE_READ_ATTRIBUTES \| SYNCHRONIZE`; a test. | fixed | +| R4 | nit | D2 | "Copies the previous temp file" missed N1's case; "as the commit's temp file has" read as covering `0600`. | Reworded. | fixed | + +Refuted in round 3: roots in the `\\?\` form (the constructor stores the plain form); a staged change retried under a +caller's `IfMatch` (only unconditional appends retry, and a test says so); the subtype (`FileAlreadyExistsException` +derives from `PreconditionFailedException`); "every line runs" (the wrappers' only branches are their success tests). + +**Round 4:** standard tier, Opus, 2026-10-13, on R1 to R4's edits. All four verified. No blocker or major. + +| # | Severity | Section | Finding | Edit | Status | +|---|---|---|---|---|---| +| S1 | minor, sensitive (data) | Design, Physical | "The first attempt spools" left an attempt that retried on `IsReplaceable`'s not-found before any spool with nothing to copy; `Probed` was "after the probe", which includes `IsReplaceable`; `IsReplaceable` was both a refusal and a retry signal. | "The first attempt to pass them spools"; `Probed` after the look at the path; its not-found a retry signal; N7's test checks the caller's bytes. | fixed | +| S2 | minor, sensitive (one meaning) | Risks, Docs | On Windows two appends to one file can still collide (a rename over the other's open read handle), unretried, where Azure lands both. | A Risks row, accepted; the Append docs keep the Windows caveat. | fixed | + +Refuted in round 4: "a refusal on a later attempt" against D1 (only unconditional appends retry, and a folder or a file +`IsReplaceable` refuses ends them, as D1 says); a folder swapped in before the open (`OpenRead` reports it as not found, +so one more attempt, then the folder's answer); the spool before the open on both paths; `Probed` being enough for N7 +and R1. + +**Round 5:** standard tier, Opus, 2026-10-13, on S1 and S2's edits. Both verified. No blocker or major. + +| # | Severity | Section | Finding | Edit | Status | +|---|---|---|---|---|---| +| T1 | minor, sensitive (security) | D2, Steps | No test pinned the spool's `0600` on Linux and macOS; without it a private file's appended bytes could sit readable by others in its folder. | A step 1 test finds the spool from `TempCreated` and checks its mode. | fixed | + +Refuted in round 5: "keep the Windows caveat" (the Docs bullet adds it to the guide; Physical's README and remarks +have it); S2 against Goal 1 (the other appender only reads); S1 against D1 and D2. + +**Round 6:** standard tier, Opus, 2026-10-13, on T1's edit. Verified. One nit, U1 (sensitive: security): a test that +reads the spool's mode after it is written would pass code that narrows the mode after creating the file, when another +user could open it; D2 now says the mode is set as the spool is created (`UnixCreateMode`). Refuted: macOS has no CI +until step 2 (the job runs the whole suite from then on). + +**Verdict:** approved. The maintainer said yes to the edits in sensitive areas (P1, P6, P7, P8, N3, R1, R2, R3, S1, +S2, T1, U1) with the plan's approval. Six rounds; no pass after the first added a blocker or a major, and the last added one nit, applied. +The plan schedules its tests and docs, and each step ends with `review-code` (step 3's at the deep tier). + +**Refuted in round 1:** patch rather than minor (versioning.md: fixes are patches; the change restores one meaning); the file delete +still going by path under `RefuseLinks` (out of scope, stated); the plan's approval covering the `nuget` approvals (the +2027.1 precedent); the path listing steering a delete outside the root (names only; every act is through a handle); a +held folder renamed out of the root mid-walk (the walk acts only inside folders it holds); "Azure Blob does this today" +(Writes.cs:218-241 matches D1, but for the inner exception); S3's and SharePoint's 409s (both retried, a folder's +`IOException` not); D5's junction claim (matches .NET's `RemoveDirectoryRecursive`); the two-writer race's new +expectation (the winner is done before the loser retries); the locked restore of one project and the runner labels; +`SafeFileHandle` owning an fd; D7 as a silent conflict with the rules (it is stated; P5 is its factual premise). + +## Progress log + +- **2026-10-13:** the plan is written for #65 and #70, after the maintainer chose D1 and D8. The Windows and Linux + calls of D3 were tried on this machine and in Linux containers (x64, and Arm64 under emulation) before planning. + Reviewed in six rounds; #73 opened for the CI jobs (P9); approved by @serionist, whose agreement on the Common API + door is posted on #65. From 71969621b2a0c9a23b1d551c9c1e8c457aec7fb2 Mon Sep 17 00:00:00 2001 From: Osvay Soma Date: Tue, 13 Oct 2026 09:53:32 +0200 Subject: [PATCH 2/6] Retry an unconditional append after a concurrent change on every provider An append with no condition ended one way on Azure Blob, which read the file again and appended after another writer's change or create, and another on every other provider, which threw at the first conflict. AppendByRewriteAsync and Physical now do what Azure does: up to five attempts, then PreconditionFailedException with the last conflict inside. A caller's IfMatch or IfNotExists still fails at once. The caller's stream is read once: the rewrite keeps its bytes in the latest attempt's temp file, and Physical copies them to a hidden temp file beside the target, after the refusals that need none, and retries only on its own signals of a change. Refs #65 --- CHANGELOG.md | 14 ++ docs/guide/errors.md | 4 +- docs/guide/providers.md | 8 +- docs/guide/reading-and-writing.md | 15 +- docs/guide/writing-a-provider.md | 3 +- docs/migration-2027.md | 7 +- docs/plans/2026-10-13-2027-1-1.md | 9 + src/WriteableFileProviders.AmazonS3/README.md | 4 +- src/WriteableFileProviders.Box/README.md | 3 +- .../FileAlreadyExistsException.cs | 8 +- .../IWriteableFileProvider.cs | 10 +- .../WriteableFileProvider.Core.cs | 110 +++++++- .../README.md | 3 +- src/WriteableFileProviders.Minio/README.md | 3 +- src/WriteableFileProviders.Physical/README.md | 6 +- .../WriteablePhysicalFileProvider.cs | 236 ++++++++++++++---- .../README.md | 2 + .../BaseClassTests.cs | 127 ++++++++++ .../ScriptedStoreProvider.cs | 79 ++++++ .../GcsProviderTests.cs | 7 +- .../PhysicalConformanceTests.cs | 5 +- .../PhysicalProviderTests.cs | 218 +++++++++++++++- .../SharePointProviderTests.cs | 7 +- .../ProviderConformanceTests.cs | 32 +-- 24 files changed, 789 insertions(+), 131 deletions(-) create mode 100644 tests/WriteableFileProviders.Common.Tests/ScriptedStoreProvider.cs diff --git a/CHANGELOG.md b/CHANGELOG.md index dbd4442..f2bfa97 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,20 @@ Every package releases together, at one version that changes something consumers notice adds a line under the next version's heading. `release.yml` publishes a version's section as its GitHub release notes. +## 2027.1.1 + +### Fixed + +- **An append ends the same way on every provider** (Physical, Amazon S3, MinIO, Google Cloud Storage, Box, + SharePoint, and a provider of your own built on `AppendByRewriteAsync`): an append with no condition of yours that + meets another writer's change or create, or a file deleted since it was read, reads the file again and appends after + what is there now, up to five attempts in all, then throws `PreconditionFailedException`, as Azure Blob's append always + did. These providers threw at the first conflict: `PreconditionFailedException`, or `FileAlreadyExistsException` for a + file created meanwhile. With your own `IfMatch` or `IfNotExists` the first conflict still throws. Where a store's + answer to a write is lost and its SDK sends the write again, the append can meet its own write and append its bytes a + second time, as a retry by hand would. Physical copies your stream to a hidden `.wfp-.tmp` beside the file + first; on Windows, one of two appends to one file at once can still fail with `UnauthorizedAccessException`. + ## 2027.1.0 (2026-10-13) Additions and fixes, nothing that breaks: a create-only write's own exception, an injectable clock, Physical's link diff --git a/docs/guide/errors.md b/docs/guide/errors.md index 7182eea..9273da3 100644 --- a/docs/guide/errors.md +++ b/docs/guide/errors.md @@ -8,8 +8,8 @@ for when you need its status or error code. | `ArgumentException` (and `ArgumentNullException`, `ArgumentOutOfRangeException`) | A path or an option the provider refuses. | | `FileNotFoundException` | The file doesn't exist, or is a folder. | | `FileExistsException` | The destination of a copy or a move exists. | -| `FileAlreadyExistsException` | A write with `IfNotExists` found a file at its path; or, where the provider refuses it, an append to a missing file that another writer created meanwhile ([providers compared](providers.md#reads-and-writes)). It is a `PreconditionFailedException`. | -| `PreconditionFailedException` | `IfMatch` didn't hold, a pinned read's file changed, an append found its file changed, a copy's or a move's source changed while it was copied, or the store reported a conflict with a concurrent operation (the write can be retried); and, as its subtype above, `IfNotExists` didn't hold. | +| `FileAlreadyExistsException` | A write with `IfNotExists` found a file at its path. It is a `PreconditionFailedException`. | +| `PreconditionFailedException` | `IfMatch` didn't hold, a pinned read's file changed, an append with no condition found its file changed five times in a row ([appends](reading-and-writing.md#append)), a copy's or a move's source changed while it was copied, or the store reported a conflict with a concurrent operation (the write can be retried); and, as its subtype above, `IfNotExists` didn't hold. | | `NotSupportedException` | The provider or its store can't do it. | | `InvalidOperationException` | The store isn't set up as configured (a missing container, bucket, library or root folder, or no credentials to find), or you tried to modify the root. | | `UnauthorizedAccessException` | The store refused the credential or the permission: HTTP 401 or 403, or a refused sign-in. | diff --git a/docs/guide/providers.md b/docs/guide/providers.md index 0be260d..f95320c 100644 --- a/docs/guide/providers.md +++ b/docs/guide/providers.md @@ -43,10 +43,10 @@ whose condition fails. **Checked** means the provider compares just before writi `IfNotExists` that finds a file throws `FileAlreadyExistsException` on every provider. Only a file counts: where a folder has the name, the write gets the answer it gets without conditions. -An append to a missing file that another writer creates at the same moment ends differently: Azure Blob appends after -the new content, and every other provider refuses with `FileAlreadyExistsException`. Where the store enforces the -condition (**Atomic** in the `IfNotExists` column), the other writer's content is kept either way; where it is only -checked, a file created in between can be replaced. +An append with no condition of yours that meets another writer's change or create reads the file again and appends +after it, on every provider, up to five attempts in all. Where the store enforces the condition (**Atomic** in the +`IfMatch` and `IfNotExists` columns), the other writer's content is kept; where it is only checked, a write that lands +between the check and the rewrite can be replaced. Every provider reads byte ranges natively; Box and SharePoint read them from the file's download URL. diff --git a/docs/guide/reading-and-writing.md b/docs/guide/reading-and-writing.md index c243172..8690ddd 100644 --- a/docs/guide/reading-and-writing.md +++ b/docs/guide/reading-and-writing.md @@ -52,14 +52,19 @@ append and a local stream's commit keep the file's type. ## Append Where the store has no native append, the provider rewrites the file on condition that it is still the version it read. -If another write landed in between, the append throws `PreconditionFailedException`: retry it. An append to a missing -file that another writer creates at the same moment ends differently by provider: most refuse it with -`FileAlreadyExistsException`, and Azure Blob appends after the new content. Where the store enforces `IfNotExists`, -the other writer's content is kept either way ([providers compared](providers.md#reads-and-writes)). +If another writer changed or created the file in between, or deleted it, the append reads it again and appends after +what is there now, on every provider, as Azure Blob's native append does: up to five attempts in all, then +`PreconditionFailedException`. With your own `IfMatch` or `IfNotExists`, the first conflict throws instead, since you +asked for that version, or for no file at all. - The condition is atomic on Azure Blob, Google Cloud Storage and S3 on AWS. -- Elsewhere it is checked, so a write that lands between the check and the rewrite can be lost. +- Elsewhere it is checked, so a write that lands between the check and the rewrite can be lost + ([providers compared](providers.md#reads-and-writes)). - An append by rewrite needs the file's `ETag`; a file without one throws `NotSupportedException`. +- A write whose answer was lost, and which the store's SDK sent again, can meet itself: the append then reads its own + write as another writer's and appends its bytes a second time, as a caller who retried by hand would. +- On Windows, which doesn't rename over a file that is open, Physical can't replace a file another append is reading: of + two appends to one file at once, one can fail with `UnauthorizedAccessException`. ## Write only if unchanged, or only if absent diff --git a/docs/guide/writing-a-provider.md b/docs/guide/writing-a-provider.md index 2f5bcbe..cc06e5e 100644 --- a/docs/guide/writing-a-provider.md +++ b/docs/guide/writing-a-provider.md @@ -76,7 +76,8 @@ Declare what the store can do. Until you do, the base refuses or weakens the ope ## Use the helpers -- `AppendByRewriteAsync`: an append for a store without one, conditional on the version read. +- `AppendByRewriteAsync`: an append for a store without one, conditional on the version read, which reads the file + again after a concurrent change, as every provider's append does when the caller gave no condition. - `SpoolToTempFileAsync`: copies a stream to a local temp file, for an SDK that needs a length or a seekable stream. - `NotFound(path)`: the `FileNotFoundException` for a missing file, naming the path under the root. - `ReadWithContractAsync`: a read by a public member of your own, under the same error rules as `ReadFileAsync`. diff --git a/docs/migration-2027.md b/docs/migration-2027.md index a4d0109..d55107a 100644 --- a/docs/migration-2027.md +++ b/docs/migration-2027.md @@ -239,7 +239,8 @@ Azure.Storage.Blobs moves from 12.23 to 12.30. | A URL lived as asked | Short-lived credentials need `AllowShorterValidity` | Use long-term keys, or set it. | | Any endpoint scheme | Plain `http` to a non-loopback host: `ArgumentException` | Use `https`, or set `AllowInsecureHttp`. | | Writes disposed your stream | Left open | Dispose it yourself. | -| A racing append or move went through | `PreconditionFailedException` | Retry. | +| A racing append lost a write | The append reads the file again and lands after the other write; `PreconditionFailedException` after five attempts (2027.1.1) | Nothing; retry the rare failure. | +| A racing move went through | `PreconditionFailedException` | Retry. | - **`ReadFileAsync(versionId, "a", "b.txt")` still compiles**: it reads the path `/a/b.txt`. - `AllowShorterValidity` is needed for a credential that expires first, a session token, or a client you pass in. @@ -271,7 +272,7 @@ Azure.Storage.Blobs moves from 12.23 to 12.30. | `GetFileInfo` read one object | It also lists | Grant `storage.objects.list` (`roles/storage.objectViewer`). | | `ReadFile` returned a seekable temp file | A forward-only stream | Use `OpenReadSeekableAsync` or `ReadRangeAsync`. | | The SDK retried a failed copy | Not retried | Check the destination before you retry. | -| A racing append lost a write | `PreconditionFailedException` | Retry. | +| A racing append lost a write | The append reads the file again and lands after the other write; `PreconditionFailedException` after five attempts (2027.1.1) | Nothing; retry the rare failure. | | Writes disposed your stream | Left open | Dispose it yourself. | | Over 7 days: `ArgumentOutOfRangeException`; can't sign: `PlatformNotSupportedException` | `NotSupportedException` | Catch it, or use the gateway. | @@ -325,7 +326,7 @@ now place files elsewhere: | Any Graph error read as "missing" | Only 404; 401, 403: `UnauthorizedAccessException`; else `IOException` | Catch those too. | | `ReadFile` returned a seekable temp file | A forward-only download | Use `OpenReadSeekableAsync` to seek. | | Writes rewound your stream, some disposed it | Read from its position, leave it open | Rewind and dispose it yourself. | -| A racing append lost a write | `PreconditionFailedException` | Retry. | +| A racing append lost a write | The append reads the file again and lands after the other write; `PreconditionFailedException` after five attempts (2027.1.1) | Nothing; retry the rare failure. | | `a%41` in a path reached the service as `aA` | Names `a%41` | Read 2026's files by the decoded name. | | Download URLs ignored the validity | 1 hour to 7 days, with `AllowShorterValidity` | Set it, or use the gateway. | diff --git a/docs/plans/2026-10-13-2027-1-1.md b/docs/plans/2026-10-13-2027-1-1.md index 836aa38..a7ee9ca 100644 --- a/docs/plans/2026-10-13-2027-1-1.md +++ b/docs/plans/2026-10-13-2027-1-1.md @@ -436,3 +436,12 @@ expectation (the winner is done before the loser retries); the locked restore of calls of D3 were tried on this machine and in Linux containers (x64, and Arm64 under emulation) before planning. Reviewed in six rounds; #73 opened for the CI jobs (P9); approved by @serionist, whose agreement on the Common API door is posted on #65. +- **2026-10-13, step 1 (#65):** `AppendByRewriteAsync` retries as D1 and D2 say, holding the caller's bytes in the + latest attempt's temp file; Physical looks at the path again on each attempt, spools beside the target, and retries on + its own signals. As built: the "private marker" is the exception's own identity, recorded as Physical throws it (the + version check through a new `changed` argument of `CommitAsync`, the taken name and the two not-founds), and the loop + retries only the exception it recorded, with no wrapper type; the scripted store is a provider of its own beside the + stub (`ScriptedStoreProvider`), not a mode of the stub; Physical's look reads whether a file is there once, before + `Probed`, so a file deleted at that hook reaches `IsReplaceable`. The conformance and provider tests that expected an + append to throw on a staged change (the two conformance tests, GCS's and SharePoint's) now expect the append after the + change; nothing else moved. diff --git a/src/WriteableFileProviders.AmazonS3/README.md b/src/WriteableFileProviders.AmazonS3/README.md index 5218599..1398351 100644 --- a/src/WriteableFileProviders.AmazonS3/README.md +++ b/src/WriteableFileProviders.AmazonS3/README.md @@ -68,8 +68,8 @@ pass your own `IAmazonS3`. Nothing touches the network until the first call. - **Writes** up to 8 MiB are one `PUT`, larger ones a multipart upload. A failed write leaves the old content. - **Conditions** (`IfMatch`, `IfNotExists`) go to S3 with the write, which checks them atomically. Toward a `ServiceUrl`, the provider also checks them first: not atomic, since the server may ignore them. -- **Appends** rewrite the file with `If-Match` on the version read: a concurrent write makes the append throw - `PreconditionFailedException`. +- **Appends** rewrite the file with `If-Match` on the version read. After a concurrent write the append reads the file + again and appends after it, up to five attempts in all, then throws `PreconditionFailedException`. - **Copies and moves** run on the server, files over 5 GiB in parts of 512 MiB or more. A move deletes the source with `If-Match` and undoes the copy if the source changed meanwhile. - **Listings** carry no content type; `GetFileInfoAsync` does. Requester-pays buckets work on AWS. diff --git a/src/WriteableFileProviders.Box/README.md b/src/WriteableFileProviders.Box/README.md index 6d9231f..174d9d1 100644 --- a/src/WriteableFileProviders.Box/README.md +++ b/src/WriteableFileProviders.Box/README.md @@ -71,7 +71,8 @@ happens on the first call; a sign-in Box refuses throws `UnauthorizedAccessExcep - **Real folders, case ignored.** Writing `A.TXT` over `a.txt` keeps the stored name. Listings skip web links. - **Every call walks its path** from `RootFolderId`, one listing per folder; nothing is cached. Deep paths cost more. - **Writes** go through a local temp file, then up in one request below 20 MiB, or a chunked upload session from - 20 MiB. A failed write leaves the old content. Appends rewrite the file. Deletes go to the Box trash. + 20 MiB. A failed write leaves the old content. Appends rewrite the file, and after a concurrent write read it again + and append after it, up to five attempts in all. Deletes go to the Box trash. - **No content types.** `UpdateFileOptions.ContentType` is ignored; `ContentType` is null. - **ETags** are Box file version ids. `IfNotExists` is atomic; `IfMatch` is checked just before the write: not atomic. - **Errors** carry no credentials, unless you pass an `IBoxClient` other than a plain `BoxClient`. diff --git a/src/WriteableFileProviders.Common/FileAlreadyExistsException.cs b/src/WriteableFileProviders.Common/FileAlreadyExistsException.cs index 63c0432..db47831 100644 --- a/src/WriteableFileProviders.Common/FileAlreadyExistsException.cs +++ b/src/WriteableFileProviders.Common/FileAlreadyExistsException.cs @@ -1,11 +1,9 @@ namespace WriteableFileProviders; /// -/// A write with found a file at its path; nothing was written. Most -/// providers throw it too for an append to a missing file that another writer created meanwhile; Azure Blob appends -/// after the new content instead. It is a , so a catch of that type -/// catches it. Not to be confused with , which a copy or a move throws when its -/// destination exists. +/// A write with found a file at its path; nothing was written. It is a +/// , so a catch of that type catches it. Not to be confused with +/// , which a copy or a move throws when its destination exists. /// public class FileAlreadyExistsException : PreconditionFailedException { diff --git a/src/WriteableFileProviders.Common/IWriteableFileProvider.cs b/src/WriteableFileProviders.Common/IWriteableFileProvider.cs index 31672f6..9aab718 100644 --- a/src/WriteableFileProviders.Common/IWriteableFileProvider.cs +++ b/src/WriteableFileProviders.Common/IWriteableFileProvider.cs @@ -91,7 +91,8 @@ public interface IWriteableFileProvider : IFileProvider, IDisposable, IAsyncDisp /// /// Writes a file: overwrites or appends, atomically, so a write that fails leaves the old content. Missing parent - /// folders are created. + /// folders are created. An append with no condition that meets another writer's change, create or delete reads the + /// file again and appends after what is there now, up to five attempts in all. /// /// The file. /// @@ -100,12 +101,11 @@ public interface IWriteableFileProvider : IFileProvider, IDisposable, IAsyncDisp /// How to write; overwrite with no conditions by default. /// Cancels the call. /// - /// is set and a file is at the path; or, on a provider that refuses it, - /// an append to a missing file that another writer created meanwhile. + /// is set and a file is at the path. /// /// - /// did not hold, an append found the file changed since it read it, or the - /// store reported a conflict with a concurrent operation and the write can be retried. + /// did not hold, an append with no condition found the file changed on each + /// of its five attempts, or the store reported a conflict with a concurrent operation and the write can be retried. /// /// Both and are set. /// The path is the provider root. diff --git a/src/WriteableFileProviders.Common/WriteableFileProvider.Core.cs b/src/WriteableFileProviders.Common/WriteableFileProvider.Core.cs index 862fbff..6630573 100644 --- a/src/WriteableFileProviders.Common/WriteableFileProvider.Core.cs +++ b/src/WriteableFileProviders.Common/WriteableFileProvider.Core.cs @@ -338,13 +338,19 @@ protected virtual ValueTask DisposeAsyncCore() /// /// Appends by rewriting, for a store with no native append: the existing content and - /// are spooled to a temp file, which then replaces the file with on the - /// version that was read, so a concurrent change throws instead of being - /// lost. Where no file was read (a missing path, or a folder), it writes with - /// , so a file created meanwhile throws - /// , and a folder gets the store's answer. It keeps the file's content type - /// unless the caller gives one. It needs the file's : an existing file without - /// one throws . + /// are copied to a temp file, which then replaces the file with on the + /// version that was read, so a concurrent change is never lost. Where no file was read (a missing path, or a + /// folder), it writes with , and a folder gets the store's answer. When + /// the caller gave no condition, a concurrent change or create, or a file deleted since it was read, makes it read + /// the file again and append after what is there now, up to five attempts in all, as Azure Blob's native append + /// does; then it throws with the last conflict inside. With the caller's + /// or , the first conflict + /// throws: for a file created meanwhile under + /// , otherwise. + /// is read once: a retry copies the caller's bytes from the previous attempt's temp file, + /// and a failure of the caller's stream ends the append, whatever its type. It keeps the file's content type unless + /// the caller gives one. It needs the file's : an existing file without one + /// throws . /// /// The file. /// The content to add. @@ -354,6 +360,26 @@ protected async Task AppendByRewriteAsync(ResolvedPath path, Stream contents, Up { ArgumentNullException.ThrowIfNull(contents); ArgumentNullException.ThrowIfNull(options); + // A caller's condition names one version, or none: its failure is the caller's answer, never read past. + var retries = !options.IfNotExists && options.IfMatch is null; + var appended = new AppendedBytes(); + await using (appended.ConfigureAwait(false)) + { + for (var attempt = 1; ; attempt++) + { + var conflict = await TryAppendByRewriteAsync(path, contents, options, appended, retries, cancellationToken).ConfigureAwait(false); + if (conflict is null) + return; + if (attempt == AppendAttempts) + throw new PreconditionFailedException($"The file '{path}' kept changing while it was appended to; nothing was appended after {AppendAttempts} attempts.", conflict); + } + } + } + + // One attempt of an append by rewrite: null once the file is written, or the concurrent change the next attempt reads + // past. Only an append that retries returns one; any other failure is thrown. + private async Task TryAppendByRewriteAsync(ResolvedPath path, Stream contents, UpdateFileOptions options, AppendedBytes appended, bool retries, CancellationToken cancellationToken) + { var info = await GetFileInfoCoreAsync(path, cancellationToken).ConfigureAwait(false); // Only a file is appended to; a folder at the path gets the store's own answer to the rewrite below. var isFile = info.Exists && !info.IsDirectory; @@ -364,15 +390,36 @@ protected async Task AppendByRewriteAsync(ResolvedPath path, Stream contents, Up if (isFile && info.ETag is null) throw new NotSupportedException($"{GetType().Name} keeps no version of '{path}', so an append could lose a concurrent write."); var temp = StreamUtilities.CreateTempFile(); - await using (temp.ConfigureAwait(false)) + var held = false; + try { if (isFile) { - var existing = await ReadFileCoreAsync(path, cancellationToken).ConfigureAwait(false); - await using (existing.ConfigureAwait(false)) - await existing.CopyToAsync(temp, cancellationToken).ConfigureAwait(false); + try + { + var existing = await ReadFileCoreAsync(path, cancellationToken).ConfigureAwait(false); + await using (existing.ConfigureAwait(false)) + await existing.CopyToAsync(temp, cancellationToken).ConfigureAwait(false); + } + catch (Exception e) when (retries && (e is FileNotFoundException || Recognises(IsNotFound, e))) + { + // Deleted since its info was read: the next attempt creates it. + return e; + } + } + // The caller's bytes: from the caller's stream the first time, from the temp file that holds them after. + var start = temp.Position; + if (appended.File is { } previous) + { + previous.Position = appended.Offset; + await previous.CopyToAsync(temp, cancellationToken).ConfigureAwait(false); } - await contents.CopyToAsync(temp, cancellationToken).ConfigureAwait(false); + else + { + await contents.CopyToAsync(temp, cancellationToken).ConfigureAwait(false); + } + await appended.HoldAsync(temp, start).ConfigureAwait(false); + held = true; temp.Position = 0; // The rewrite replaces exactly the file that was read, or creates one where there was none: a file written // meanwhile makes it fail rather than be lost. @@ -384,7 +431,20 @@ protected async Task AppendByRewriteAsync(ResolvedPath path, Stream contents, Up // An append does not change what the file is. ContentType = options.ContentType ?? info.ContentType, }; - await UpdateWithPreconditionsAsync(path, temp, rewrite, cancellationToken).ConfigureAwait(false); + try + { + await UpdateWithPreconditionsAsync(path, temp, rewrite, cancellationToken).ConfigureAwait(false); + return null; + } + catch (PreconditionFailedException e) when (retries) + { + return e; + } + } + finally + { + if (!held) + await temp.DisposeAsync().ConfigureAwait(false); } } @@ -446,4 +506,28 @@ private async Task UpdateWithPreconditionsAsync(ResolvedPath path, Stream conten } await UpdateFileCoreAsync(path, contents, options, cancellationToken).ConfigureAwait(false); } + + // How many attempts an append by rewrite with no condition of the caller's makes in all: as many as Azure Blob's + // native append. + private const int AppendAttempts = 5; + + // The caller's bytes of an append by rewrite, held in the latest attempt's temp file from Offset on, so that a retry + // never reads the caller's stream again. + private sealed class AppendedBytes : IAsyncDisposable + { + public FileStream? File { get; private set; } + + public long Offset { get; private set; } + + // Takes the temp file that now holds the bytes, from 'offset' on, and lets the previous one go. + public async ValueTask HoldAsync(FileStream file, long offset) + { + if (File is { } previous) + await previous.DisposeAsync().ConfigureAwait(false); + File = file; + Offset = offset; + } + + public ValueTask DisposeAsync() => File?.DisposeAsync() ?? default; + } } diff --git a/src/WriteableFileProviders.GoogleCloudStorage/README.md b/src/WriteableFileProviders.GoogleCloudStorage/README.md index 49ff36c..c9428fe 100644 --- a/src/WriteableFileProviders.GoogleCloudStorage/README.md +++ b/src/WriteableFileProviders.GoogleCloudStorage/README.md @@ -66,7 +66,8 @@ missing file. `ReadFileAsync` and `ReadRangeAsync` of a missing file still throw ## What's different - **ETags are generations.** `IfMatch` and `IfNotExists` are generation preconditions, atomic. Appends rewrite the - object on the generation read, so they never lose a concurrent write. + object on the generation read, so they never lose a concurrent write; after one they read the object again and append + after it, up to five attempts in all. - **Writes are resumable uploads.** The object appears only when every byte has arrived. - **A `dir/` object** (the console's "create folder") is an empty folder. - **Reads** aren't checked against the object's hash. diff --git a/src/WriteableFileProviders.Minio/README.md b/src/WriteableFileProviders.Minio/README.md index f992b00..f10b9e8 100644 --- a/src/WriteableFileProviders.Minio/README.md +++ b/src/WriteableFileProviders.Minio/README.md @@ -70,7 +70,8 @@ A MinIO key pair, or none for anonymous access, which can't presign URLs. For an - **A file and a folder with the same name:** MinIO lists only the file, and hides what's under the folder until the file is deleted. - **Writes** up to 8 MiB are one request, larger ones a multipart upload (10,000 parts at most). A failed write leaves - the old content. Appends rewrite the file with `If-Match` on the version read. + the old content. Appends rewrite the file with `If-Match` on the version read, and after a concurrent write read it + again and append after it, up to five attempts in all. - **Listings** carry no content type; `GetFileInfoAsync` does. - **No storage class** is sent: MinIO refuses the ones it doesn't know. - **Signed URLs** are presigned read and write URLs, up to 7 days, over the scheme `UseSSL` picks. Create URLs come diff --git a/src/WriteableFileProviders.Physical/README.md b/src/WriteableFileProviders.Physical/README.md index 8d8478a..e0bdf3a 100644 --- a/src/WriteableFileProviders.Physical/README.md +++ b/src/WriteableFileProviders.Physical/README.md @@ -53,7 +53,11 @@ None. The provider reads and writes with the permissions of the process. write, plus any lag of a file server's clock: a write whose temp file goes fails, and its file keeps its content. It doesn't walk into linked folders, and skips a folder below the root that it may not list; a root it can't read throws. -- **An append copies the whole file**, so frequent appends to a large file (a log) are expensive. +- **An append copies the whole file**, so frequent appends to a large file (a log) are expensive. It first copies your + stream to a hidden `.wfp-.tmp` beside the file (on Linux and macOS readable by the process's user only), so that + after a concurrent write it can read the file again and append after it, up to five attempts in all. On Windows, which + doesn't rename over a file that is open, one of two appends to one file at once can fail with + `UnauthorizedAccessException`. - **A write puts a new file at the path.** Other hard links keep the old content, and a symbolic link at the path becomes a file. On Windows the file takes the folder's permissions; on Linux and macOS, the old file's mode bits. ACLs and extended attributes are not kept. diff --git a/src/WriteableFileProviders.Physical/WriteablePhysicalFileProvider.cs b/src/WriteableFileProviders.Physical/WriteablePhysicalFileProvider.cs index 645da07..ff74906 100644 --- a/src/WriteableFileProviders.Physical/WriteablePhysicalFileProvider.cs +++ b/src/WriteableFileProviders.Physical/WriteablePhysicalFileProvider.cs @@ -16,7 +16,8 @@ namespace WriteableFileProviders.Physical; /// .wfp-<guid>.tmp (32 lowercase hex digits) in the target's folder, then rename it over the target: the /// file has its old content or its new content, never a part. A write that fails removes its temp file; listings never /// show one, and show every other name, .wfp-notes.tmp included. An append copies the file and the new bytes -/// into the temp file. +/// into the temp file; it first copies the caller's stream into a second temp file named the same way beside it, which +/// only the process's user may read on Linux and macOS, and which goes when the append ends. /// /// /// Replacing by rename makes the new content a new file at the path, so a write is refused, the file keeping its @@ -42,13 +43,18 @@ namespace WriteableFileProviders.Physical; /// than that resolution step it further each time, so the time can run ahead of the clock without bound; bounding it /// would let an ETag repeat. A change by another program that keeps the length within the file system's time resolution /// may not change the ETag. is checked just before the rename, which is not -/// atomic: another process can change the file between the check and the rename. An append that finds the file changed -/// since it read it throws , with the same window. +/// atomic: another process can change the file between the check and the rename. An append with no condition of the +/// caller's that finds the file changed since it read it, created, or deleted, reads it again and appends after what is +/// there now, up to five attempts in all, then throws ; its check has the same +/// window. With the caller's or , the +/// first change throws. On Windows a write cannot replace a file another write is reading, so one of two appends to one +/// file at once can fail with (see Reads). /// /// /// New names are taken atomically: , an append to a missing file, and -/// the destination of a move or a copy never replace a file that is there; a write that finds one throws -/// , a move or a copy . Windows renames +/// the destination of a move or a copy never replace a file that is there; a write with +/// that finds one throws , an append +/// appends after it, a move or a copy throws . Windows renames /// without replacing. On Linux and macOS a write or a copy gives its temp file the name as a second hard link, which /// fails when anything has the name, then drops the temp name: the name never holds a part of the file, and a process /// that dies in between leaves the file in place and a hidden temp name. Where the file system refuses hard links (FAT @@ -56,7 +62,7 @@ namespace WriteableFileProviders.Physical; /// exclusive create and the content renamed onto it, so the file exists empty for that moment: a watcher sees it, and a /// process that dies there leaves an empty file. On a network file system that may answer a resent request again (NFS), /// a write can find its own new name taken and throw , a copy -/// , with its content in place. +/// , with its content in place; an append then appends its bytes a second time. /// /// /// Reads share the file with writers and deleters, and keep the content they opened. A delete goes ahead while a @@ -101,6 +107,9 @@ public sealed class WriteablePhysicalFileProvider : WriteableFileProvider? TempCreated { get; init; } + // Called with the target each time a write has looked at it (a folder, a file there), before IsReplaceable opens it: + // the tests change the file there. + internal Action? Probed { get; init; } + /// True on Windows and macOS, whose file systems match names without regard to case by default. protected override bool IsCaseInsensitive => IgnoresCase; @@ -289,41 +302,121 @@ protected override async Task UpdateFileCoreAsync(ResolvedPath path, Stream cont ArgumentNullException.ThrowIfNull(options); cancellationToken.ThrowIfCancellationRequested(); var target = FullPath(path); + if (options.Mode != UpdateFileMode.Append) + { + var (replaced, like) = Look(path, target, options); + CheckPreconditions(path, options, VersionOf(replaced)); + // A folder that takes the name meanwhile is what a write without conditions finds there, not a file. + Func? taken = options.IfNotExists ? e => Directory.Exists(target) ? IsAFolder(path, e) : AlreadyExists(path, e) : null; + await CommitAsync(path, target, options.IfMatch, changed: null, taken, like, contents.CopyToAsync, cancellationToken).ConfigureAwait(false); + return; + } + // An append with no condition of the caller's reads the file again after a concurrent change and appends after + // what is there now, as the base class's append by rewrite does. Only this provider's own signals of a change + // count, each recorded as it is thrown: a failure of the caller's stream, whatever its type, ends the append. + var retries = !options.IfNotExists && options.IfMatch is null; + var spool = new AppendSpool(); + await using (spool.ConfigureAwait(false)) + { + for (var attempt = 1; ; attempt++) + { + IOException? signal = null; + try + { + await AppendOnceAsync(path, target, contents, options, spool, e => signal = e, cancellationToken).ConfigureAwait(false); + return; + } + catch (IOException e) when (retries && ReferenceEquals(e, signal)) + { + if (attempt == AppendAttempts) + throw new PreconditionFailedException($"The file '{path}' kept changing while it was appended to; nothing was appended after {AppendAttempts} attempts.", e); + } + } + } + } + + // One attempt of an append, which looks at the path afresh; 'signal' receives the exception that says the file changed + // since it was looked at, just before it is thrown. + private async Task AppendOnceAsync(ResolvedPath path, string target, Stream contents, UpdateFileOptions options, AppendSpool spool, + Action signal, CancellationToken cancellationToken) + { + FileInfo replaced; + FileInfo? like; + try + { + (replaced, like) = Look(path, target, options); + } + catch (IOException e) when (e is FileNotFoundException or DirectoryNotFoundException) + { + // Deleted between the look and IsReplaceable's open. + signal(e); + throw; + } + CheckPreconditions(path, options, VersionOf(replaced)); + // The caller's bytes are read here once, before the file is opened, and only after the refusals that need none. + await spool.FillAsync(path, target, contents, cancellationToken).ConfigureAwait(false); + if (like is null) + { + // An append to a missing file creates it, and fails rather than replace one that appears meanwhile; over a link + // to nothing, it replaces the link, as an overwrite does. + Func? taken = options.IfNotExists || !replaced.Exists + ? e => + { + // A folder that takes the name meanwhile is what a write without conditions finds there, not a file. + if (Directory.Exists(target)) + return IsAFolder(path, e); + var exists = AlreadyExists(path, e); + signal(exists); + return exists; + } + : null; + await CommitAsync(path, target, options.IfMatch, signal, taken, like, spool.CopyFromStartAsync, cancellationToken).ConfigureAwait(false); + return; + } + SafeFileHandle existing; + try + { + existing = OpenRead(path, target); + } + catch (FileNotFoundException e) + { + // Deleted since the look. + signal(e); + throw; + } + // The file as this handle opened it, then the new bytes; the rename happens only if the file is still that version. + var source = new FileStream(existing, FileAccess.Read, BufferSize, isAsync: true); + await using (source.ConfigureAwait(false)) + { + var version = VersionOf(existing); + CheckPreconditions(path, options, version); + await CommitAsync(path, target, version, signal, taken: null, like, async (output, token) => + { + await source.CopyToAsync(output, token).ConfigureAwait(false); + // Closed before the rename: Windows does not rename over a file that is open, by this handle or any. + await source.DisposeAsync().ConfigureAwait(false); + await spool.CopyFromStartAsync(output, token).ConfigureAwait(false); + }, cancellationToken).ConfigureAwait(false); + } + } + + // The path as a write finds it: refuses a folder there, and a file under IfNotExists; gives the file the write + // replaces (which may not exist) and the file whose permission bits the new one takes (null for none, or for a link + // to nothing). + private (FileInfo Replaced, FileInfo? Like) Look(ResolvedPath path, string target, UpdateFileOptions options) + { if (Directory.Exists(target)) throw new IOException($"The path '{path}' is a folder."); var replaced = new FileInfo(target); - if (options.IfNotExists && replaced.Exists) + // Read once: what the write found there, even if it goes before IsReplaceable opens it. + var exists = replaced.Exists; + if (options.IfNotExists && exists) throw AlreadyExists(path); + Probed?.Invoke(target); // Before anything else opens it: a file the write must not replace is refused, and the new file takes the mode of // the one it replaces. - var like = replaced.Exists && IsReplaceable(path, target, replaced) ? replaced : null; - if (options.Mode == UpdateFileMode.Append && like is not null) - { - // A file deleted since the probe is reported as missing. - var existing = OpenRead(path, target); - // The file as this handle opened it, then the new bytes; the rename happens only if the file is still that version. - var source = new FileStream(existing, FileAccess.Read, BufferSize, isAsync: true); - await using (source.ConfigureAwait(false)) - { - var version = VersionOf(existing); - CheckPreconditions(path, options, version); - await CommitAsync(path, target, version, taken: null, like, async (output, token) => - { - await source.CopyToAsync(output, token).ConfigureAwait(false); - // Closed before the rename: Windows does not rename over a file that is open, by this handle or any. - await source.DisposeAsync().ConfigureAwait(false); - await contents.CopyToAsync(output, token).ConfigureAwait(false); - }, cancellationToken).ConfigureAwait(false); - } - return; - } - CheckPreconditions(path, options, VersionOf(replaced)); - // An append to a missing file creates it, and fails rather than replace one that appears meanwhile; over a link to - // nothing, it replaces the link, as an overwrite does. - var mustBeNew = options.IfNotExists || (options.Mode == UpdateFileMode.Append && !replaced.Exists); - // A folder that takes the name meanwhile is what a write without conditions finds there, not a file. - Func? taken = mustBeNew ? e => Directory.Exists(target) ? IsAFolder(path, e) : AlreadyExists(path, e) : null; - await CommitAsync(path, target, options.IfMatch, taken, like, contents.CopyToAsync, cancellationToken).ConfigureAwait(false); + var like = exists && IsReplaceable(path, target, replaced) ? replaced : null; + return (replaced, like); } /// @@ -376,7 +469,7 @@ protected override async Task CopyCoreAsync(ResolvedPath source, ResolvedPath de await using (input.ConfigureAwait(false)) { // Whatever took the destination meanwhile, a file or a folder, the copy's answer is that it exists. - await CommitAsync(destination, FullPath(destination), expected: null, + await CommitAsync(destination, FullPath(destination), expected: null, changed: null, e => new FileExistsException($"The destination '{destination}' already exists.", e), new FileInfo(from), input.CopyToAsync, cancellationToken).ConfigureAwait(false); } @@ -714,11 +807,12 @@ private static bool IsLink(FileSystemInfo info) => (info.Attributes & FileAttributes.ReparsePoint) != 0 && info.LinkTarget is not null; // Writes the content to a temp file next to the target, then renames it over the target. 'expected' is the version the - // target must still have just before the rename (checked, not atomic); 'taken', when set, renames only onto a free - // name (atomic), and gives the exception for a name that something took; the new file takes the permission bits of - // 'like', the file it replaces or copies, else the default mode. - private async Task CommitAsync(ResolvedPath path, string target, string? expected, Func? taken, FileInfo? like, - Func write, CancellationToken cancellationToken) + // target must still have just before the rename (checked, not atomic), and 'changed' receives the exception thrown + // when it doesn't; 'taken', when set, renames only onto a free name (atomic), and gives the exception for a name that + // something took; the new file takes the permission bits of 'like', the file it replaces or copies, else the default + // mode. + private async Task CommitAsync(ResolvedPath path, string target, string? expected, Action? changed, Func? taken, + FileInfo? like, Func write, CancellationToken cancellationToken) { var folder = Path.GetDirectoryName(target)!; Directory.CreateDirectory(folder); @@ -759,7 +853,11 @@ private async Task CommitAsync(ResolvedPath path, string target, string? expecte written = true; var current = new FileInfo(target); if (expected is not null && VersionOf(current) != expected) - throw Changed(path, expected); + { + var exception = Changed(path, expected); + changed?.Invoke(exception); + throw exception; + } Stamp(Described(current).LastWriteTimeUtc, TimeProvider.GetUtcNow().UtcDateTime, time => { File.SetLastWriteTimeUtc(temp, time); @@ -1006,6 +1104,60 @@ private static void DeleteQuietly(string fullPath) private static IOException IsAFolder(ResolvedPath path, Exception innerException) => new($"The path '{path}' is a folder.", innerException); + // The caller's bytes of an append, copied once into a hidden temp file next to the target, before the file is opened: + // a retry appends them again without reading the caller's stream, they never leave the root, and a crash leaves a + // temp file DeleteTempFilesAsync finds. On Linux and macOS only the process's user may read it, from its creation. + private sealed class AppendSpool : IAsyncDisposable + { + private FileStream? _file; + + public async Task FillAsync(ResolvedPath path, string target, Stream contents, CancellationToken cancellationToken) + { + if (_file is not null) + return; + var folder = Path.GetDirectoryName(target)!; + Directory.CreateDirectory(folder); + var create = new FileStreamOptions + { + Mode = FileMode.CreateNew, + Access = FileAccess.ReadWrite, + Share = FileShare.None, + BufferSize = BufferSize, + Options = FileOptions.Asynchronous | FileOptions.DeleteOnClose, + }; + if (!OperatingSystem.IsWindows()) + create.UnixCreateMode = UnixFileMode.UserRead | UnixFileMode.UserWrite; + FileStream file; + try + { + file = new FileStream(Path.Join(folder, TempPrefix + Guid.NewGuid().ToString("N") + TempSuffix), create); + } + catch (UnauthorizedAccessException e) + { + // The message would name the temp file. + throw new UnauthorizedAccessException($"The file '{path}' cannot be written: the process may not create files in its folder.", e); + } + try + { + await contents.CopyToAsync(file, cancellationToken).ConfigureAwait(false); + } + catch + { + await file.DisposeAsync().ConfigureAwait(false); + throw; + } + _file = file; + } + + public async Task CopyFromStartAsync(Stream output, CancellationToken cancellationToken) + { + _file!.Position = 0; + await _file.CopyToAsync(output, cancellationToken).ConfigureAwait(false); + } + + public ValueTask DisposeAsync() => _file?.DisposeAsync() ?? default; + } + private static FileAlreadyExistsException AlreadyExists(ResolvedPath path, Exception? innerException = null) => new($"The file '{path}' already exists.", innerException); diff --git a/src/WriteableFileProviders.SharePoint/README.md b/src/WriteableFileProviders.SharePoint/README.md index e3839b9..74a9bf6 100644 --- a/src/WriteableFileProviders.SharePoint/README.md +++ b/src/WriteableFileProviders.SharePoint/README.md @@ -71,6 +71,8 @@ write access to the site. Building the provider sends nothing; the site and libr - **Writes** go through a local temp file, then up in one request up to 4 MiB, or an upload session in 5 MiB fragments. A fragment that hits a server or network error is sent once more. A failed write leaves the old content. - **Conditions aren't atomic.** `IfMatch` and `IfNotExists` are checked before the write and also sent to Graph. + Appends rewrite the file with `If-Match` on the version read, and after a concurrent write read it again and append + after it, up to five attempts in all. - **Content type** comes from the file name. `UpdateFileOptions.ContentType` is ignored. - **Lengths are real.** A file whose size Graph doesn't report costs one extra ranged request to measure. - **Copies** run on the server and are polled for about 10 minutes, then throw `IOException`; the copy may still diff --git a/tests/WriteableFileProviders.Common.Tests/BaseClassTests.cs b/tests/WriteableFileProviders.Common.Tests/BaseClassTests.cs index 1681727..600e195 100644 --- a/tests/WriteableFileProviders.Common.Tests/BaseClassTests.cs +++ b/tests/WriteableFileProviders.Common.Tests/BaseClassTests.cs @@ -169,6 +169,133 @@ public async Task FilesWithoutAnETag_RefusePinnedReadsAndAppendsByRewrite() Assert.AreEqual("e1", provider.LastUpdate!.IfMatch); } + [TestMethod] + public async Task AppendByRewrite_AfterACreateLostToAnotherWriter_AppendsAfterIt() + { + using var store = new ScriptedStoreProvider(); + store.BeforeWrite = (_, write) => + { + if (write == 1) + store.Put("f.txt", "theirs;"); + }; + await store.Append("f.txt", TestStreams.Text("mine;")); + Assert.AreEqual("theirs;mine;", store.Text("f.txt")); + Assert.IsTrue(store.Writes[0].IfNotExists, "The first attempt creates the file"); + Assert.IsNotNull(store.Writes[1].IfMatch, "The second appends to the file it read"); + } + + [TestMethod] + public async Task AppendByRewrite_AfterAChangeMeanwhile_AppendsToTheNewVersion() + { + using var store = new ScriptedStoreProvider(); + store.Put("f.txt", "a;"); + store.BeforeWrite = (_, write) => + { + if (write == 1) + store.Put("f.txt", "a;b;"); + }; + // The caller's stream is at its end after the first attempt: the second appends its bytes from the first's temp file. + await store.Append("f.txt", TestStreams.Text("c;")); + Assert.AreEqual("a;b;c;", store.Text("f.txt")); + Assert.HasCount(2, store.Writes); + } + + [TestMethod] + public async Task AppendByRewrite_ThatKeepsMeetingChanges_ThrowsAfterFiveAttempts() + { + using var store = new ScriptedStoreProvider(); + // Every attempt loses: a create to another writer's create, an append to another writer's delete. + store.BeforeWrite = (options, _) => + { + if (options.IfNotExists) + store.Put("f.txt", "theirs;"); + else + store.Remove("f.txt"); + }; + var e = await Assert.ThrowsExactlyAsync(() => store.Append("f.txt", TestStreams.Text("mine;"))); + Assert.HasCount(5, store.Writes); + Assert.IsInstanceOfType(e.InnerException, "The last conflict, a lost create, is inside; the append itself asked for no create"); + StringAssert.Contains(e.Message, "5 attempts"); + } + + [TestMethod] + [DataRow(true)] + [DataRow(false)] + public async Task AppendByRewrite_ToAFileDeletedBeforeItIsRead_CreatesIt(bool storesOwnNotFound) + { + using var store = new ScriptedStoreProvider(); + store.Put("f.txt", "old;"); + store.BeforeRead = read => + { + if (read != 1) + return; + store.Remove("f.txt"); + // The store's own "not found" (which IsNotFound recognises), or the contract's. + if (!storesOwnNotFound) + throw new FileNotFoundException("The file is gone."); + }; + await store.Append("f.txt", TestStreams.Text("mine;")); + Assert.AreEqual("mine;", store.Text("f.txt")); + Assert.HasCount(1, store.Writes); + Assert.IsTrue(store.Writes[0].IfNotExists); + } + + [TestMethod] + public async Task AppendByRewrite_AfterAChange_ThenAFileDeletedBeforeItIsRead_WritesTheCallersBytesOnce() + { + using var store = new ScriptedStoreProvider(); + store.Put("f.txt", "a;"); + store.BeforeWrite = (_, write) => + { + if (write == 1) + store.Put("f.txt", "a;b;"); + }; + store.BeforeRead = read => + { + if (read == 2) + store.Remove("f.txt"); + }; + await store.Append("f.txt", TestStreams.Text("c;")); + Assert.AreEqual("c;", store.Text("f.txt")); + Assert.HasCount(2, store.Writes, "The attempt whose read failed wrote nothing"); + } + + [TestMethod] + public async Task AppendByRewrite_WhoseStreamFails_EndsWithItsFailure() + { + using var store = new ScriptedStoreProvider(); + store.Put("f.txt", "a;"); + // A pinned read of a file elsewhere throws the base type when that file changes: it is not this file's change. + var contents = new InterceptingStream("b;"u8.ToArray(), () => throw new PreconditionFailedException("The source changed.")); + var e = await Assert.ThrowsExactlyAsync(() => store.Append("f.txt", contents)); + Assert.AreEqual("The source changed.", e.Message); + Assert.IsEmpty(store.Writes); + Assert.AreEqual("a;", store.Text("f.txt")); + } + + [TestMethod] + public async Task AppendByRewrite_WithTheCallersCondition_FailsAtTheFirstConflict() + { + using var store = new ScriptedStoreProvider(); + store.Put("f.txt", "a;"); + store.BeforeWrite = (_, _) => store.Put("f.txt", "theirs;"); + var etag = store.ETagOf("f.txt"); + var changed = await Assert.ThrowsExactlyAsync(() => store.Append("f.txt", TestStreams.Text("b;"), UpdateFileOptions.Append with { IfMatch = etag })); + Assert.DoesNotContain("attempts", changed.Message); + Assert.HasCount(1, store.Writes); + + store.Remove("f.txt"); + store.Writes.Clear(); + await Assert.ThrowsExactlyAsync(() => store.Append("f.txt", TestStreams.Text("b;"), UpdateFileOptions.Append with { IfNotExists = true })); + Assert.HasCount(1, store.Writes); + + // A file deleted before its read fails a caller's version as it did. + store.BeforeWrite = null; + store.Put("f.txt", "a;"); + store.BeforeRead = _ => store.Remove("f.txt"); + await Assert.ThrowsExactlyAsync(() => store.Append("f.txt", TestStreams.Text("b;"), UpdateFileOptions.Append with { IfMatch = store.ETagOf("f.txt") })); + } + [TestMethod] public async Task Disposed_RefusesEveryCall() { diff --git a/tests/WriteableFileProviders.Common.Tests/ScriptedStoreProvider.cs b/tests/WriteableFileProviders.Common.Tests/ScriptedStoreProvider.cs new file mode 100644 index 0000000..6d21d82 --- /dev/null +++ b/tests/WriteableFileProviders.Common.Tests/ScriptedStoreProvider.cs @@ -0,0 +1,79 @@ +using System.Globalization; +using System.Runtime.CompilerServices; +using System.Text; + +namespace WriteableFileProviders.Common.Tests; + +/// +/// A store of text files whose reads and writes a test scripts, attempt by attempt, for the base class's append by +/// rewrite: a hook before each write sees its options and may change the store, as a concurrent writer would; a hook +/// before each read of a file may do the same, or throw. The store checks the conditions as it writes (atomically), and +/// its own "not found" is , which recognises. +/// +public sealed class ScriptedStoreProvider() : WriteableFileProvider(new StubOptions()) +{ + private readonly Dictionary _files = new(StringComparer.Ordinal); + private int _version; + + /// The options of every write that reached the store, in order. + public List Writes { get; } = []; + + /// Called with the write's options and its number (from 1) before the store checks them. + public Action? BeforeWrite { get; set; } + + /// Called with the read's number (from 1) before the store reads the file. + public Action? BeforeRead { get; set; } + + private int Reads { get; set; } + + protected override bool SupportsAtomicPreconditions => true; + + public void Put(string name, string text) => + _files[name] = (text, "v" + (++_version).ToString(CultureInfo.InvariantCulture)); + + public void Remove(string name) => _files.Remove(name); + + public string? Text(string name) => _files.TryGetValue(name, out var file) ? file.Text : null; + + public string? ETagOf(string name) => _files.TryGetValue(name, out var file) ? file.ETag : null; + + public Task Append(string path, Stream contents, UpdateFileOptions? options = null) => + AppendByRewriteAsync(Resolve(path), contents, options ?? UpdateFileOptions.Append, default); + + protected override Task GetFileInfoCoreAsync(ResolvedPath path, CancellationToken cancellationToken) => + Task.FromResult(_files.TryGetValue(path.Name, out var file) + ? new WriteableFileInfo(this, path.Path, true, false, file.Text.Length, DateTimeOffset.UtcNow, etag: file.ETag) + : WriteableFileInfo.Missing(this, path.Path)); + + protected override async IAsyncEnumerable EnumerateCoreAsync(ResolvedPath path, [EnumeratorCancellation] CancellationToken cancellationToken) + { + await Task.Yield(); + yield break; + } + + protected override Task ReadFileCoreAsync(ResolvedPath path, CancellationToken cancellationToken) + { + BeforeRead?.Invoke(++Reads); + return _files.TryGetValue(path.Name, out var file) + ? Task.FromResult(new MemoryStream(Encoding.UTF8.GetBytes(file.Text))) + : throw new KeyNotFoundException("The store has no such key."); + } + + protected override async Task UpdateFileCoreAsync(ResolvedPath path, Stream contents, UpdateFileOptions options, CancellationToken cancellationToken) + { + Writes.Add(options); + BeforeWrite?.Invoke(options, Writes.Count); + var exists = _files.TryGetValue(path.Name, out var current); + if (options.IfNotExists && exists) + throw new FileAlreadyExistsException($"The file '{path}' already exists."); + if (options.IfMatch is not null && (!exists || current.ETag != options.IfMatch)) + throw new PreconditionFailedException($"The file '{path}' changed."); + using var reader = new StreamReader(contents, leaveOpen: true); + Put(path.Name, await reader.ReadToEndAsync(cancellationToken)); + } + + protected override Task DeleteCoreAsync(ResolvedPath path, CancellationToken cancellationToken) => + Task.FromResult(_files.Remove(path.Name)); + + protected override bool IsNotFound(Exception exception) => exception is KeyNotFoundException; +} diff --git a/tests/WriteableFileProviders.GoogleCloudStorage.Tests/GcsProviderTests.cs b/tests/WriteableFileProviders.GoogleCloudStorage.Tests/GcsProviderTests.cs index ca13bfc..c484acc 100644 --- a/tests/WriteableFileProviders.GoogleCloudStorage.Tests/GcsProviderTests.cs +++ b/tests/WriteableFileProviders.GoogleCloudStorage.Tests/GcsProviderTests.cs @@ -748,7 +748,7 @@ public async Task SignedUrls_UploadAndDownloadThroughTheEmulator() // ----- Writes through append and local streams keep their guarantees on big files ----- [TestMethod] - public async Task Append_ToAFileWrittenMeanwhileThrowsAndKeepsTheWrite() + public async Task Append_ToAFileWrittenMeanwhile_KeepsTheWriteAndAppendsAfterIt() { var http = new FaultInjection(); var provider = Provider(http); @@ -762,7 +762,8 @@ public async Task Append_ToAFileWrittenMeanwhileThrowsAndKeepsTheWrite() } return null; }; - await Assert.ThrowsExactlyAsync(() => provider.UpdateFileAsync("f.txt", TestStreams.Text("+mine"), UpdateFileOptions.Append)); - Assert.AreEqual("theirs", await ReadAll(await provider.ReadFileAsync("f.txt"))); + // The upload on the generation read is refused; the append reads the file again and appends after it. + await provider.UpdateFileAsync("f.txt", TestStreams.Text("+mine"), UpdateFileOptions.Append); + Assert.AreEqual("theirs+mine", await ReadAll(await provider.ReadFileAsync("f.txt"))); } } diff --git a/tests/WriteableFileProviders.Physical.Tests/PhysicalConformanceTests.cs b/tests/WriteableFileProviders.Physical.Tests/PhysicalConformanceTests.cs index 867b24b..b167110 100644 --- a/tests/WriteableFileProviders.Physical.Tests/PhysicalConformanceTests.cs +++ b/tests/WriteableFileProviders.Physical.Tests/PhysicalConformanceTests.cs @@ -21,8 +21,9 @@ public sealed class PhysicalConformanceTests : ProviderConformanceTests // The root is a folder of the disk, which the system resolves (dot segments included); paths under it stay inside it. protected override bool ChecksItsRoot => false; - // The version check before the rename is not atomic: PhysicalProviderTests pins that it catches a change - // made while the append copies, which is what the shared test races. + // The shared test writes from the appended stream, which Physical copies before it opens the file, so the write lands + // before the append reads the file, not between the read and the rename; PhysicalProviderTests stages that change + // from TempCreated, and pins that the check before the rename (not atomic) catches it and the append reads again. protected override bool DetectsConcurrentAppends => false; protected override string? LocalTempFolder => _base; diff --git a/tests/WriteableFileProviders.Physical.Tests/PhysicalProviderTests.cs b/tests/WriteableFileProviders.Physical.Tests/PhysicalProviderTests.cs index 6105f87..30b678b 100644 --- a/tests/WriteableFileProviders.Physical.Tests/PhysicalProviderTests.cs +++ b/tests/WriteableFileProviders.Physical.Tests/PhysicalProviderTests.cs @@ -27,13 +27,27 @@ public void Start() // Runs the test on the way new names are taken on Windows (a rename that does not replace) or on Linux and macOS (a // hard link, or where the system has none, as on Windows, an exclusive create, then a rename), whichever system // runs it. With linkRefused, the system refuses the hard link (as a file system without them does), so Linux and - // macOS take the name the way they fall back to. - private void TakeNewNames(bool byRenameWithoutReplacing, bool linkRefused = false) + // macOS take the name the way they fall back to. 'tempCreated' is the provider's TempCreated hook. + private void TakeNewNames(bool byRenameWithoutReplacing, bool linkRefused = false, Action? tempCreated = null) { _provider.Dispose(); _provider = linkRefused - ? new WriteablePhysicalFileProvider(_root) { RenamesWithoutReplacing = byRenameWithoutReplacing, Link = (_, _) => 1 } - : new WriteablePhysicalFileProvider(_root) { RenamesWithoutReplacing = byRenameWithoutReplacing }; + ? new WriteablePhysicalFileProvider(_root) { RenamesWithoutReplacing = byRenameWithoutReplacing, Link = (_, _) => 1, TempCreated = tempCreated } + : new WriteablePhysicalFileProvider(_root) { RenamesWithoutReplacing = byRenameWithoutReplacing, TempCreated = tempCreated }; + } + + // A hook that runs 'change' the first time only: another writer, once. + private static Action Once(Action change) + { + var done = false; + return _ => + { + if (!done) + { + done = true; + change(); + } + }; } // Whether the process can be refused access to its own files: not when it runs as root. @@ -259,6 +273,10 @@ public async Task Write_InAFolderWhereTheProcessMayNotCreateFilesIsRefusedNaming var e = await Assert.ThrowsExactlyAsync(() => Write("f.txt", "new")); StringAssert.Contains(e.Message, "'f.txt'"); Assert.DoesNotContain(".wfp-", e.Message, "The message names the file, not the provider's temp file"); + // An append's first file there is the copy of the caller's bytes. + var appended = await Assert.ThrowsExactlyAsync(() => Write("f.txt", "+more", UpdateFileOptions.Append)); + StringAssert.Contains(appended.Message, "'f.txt'"); + Assert.DoesNotContain(".wfp-", appended.Message, "The message names the file, not the append's spool"); } finally { @@ -284,6 +302,10 @@ public async Task Write_InAFolderTheProcessMayNotWriteIsRefusedNamingTheFile() var e = await Assert.ThrowsExactlyAsync(() => Write("f.txt", "new")); StringAssert.Contains(e.Message, "'f.txt'"); Assert.DoesNotContain(".wfp-", e.Message, "The message names the file, not the provider's temp file"); + // An append's first file there is the copy of the caller's bytes. + var appended = await Assert.ThrowsExactlyAsync(() => Write("f.txt", "+more", UpdateFileOptions.Append)); + StringAssert.Contains(appended.Message, "'f.txt'"); + Assert.DoesNotContain(".wfp-", appended.Message, "The message names the file, not the append's spool"); } finally { @@ -759,13 +781,15 @@ public async Task IfNotExists_WhenAFolderTakesTheNameDuringTheWrite_IsAFolder(bo } [TestMethod] - public async Task Append_FailsWhenTheFileChangesWhileItCopies() + public async Task Append_WhenTheFileChangesWhileItCopies_AppendsToTheNewVersion() { - // On Physical, the check before the rename catches a change made while the append reads, but is not atomic. + // On Physical, the check before the rename catches a change made while the append copies, but is not atomic; the + // append then reads the file again. await Write("f.txt", "start"); - var contents = new InterceptingStream(Encoding.UTF8.GetBytes("+mine"), () => File.WriteAllText(Disk("f.txt"), "theirs")); - await Assert.ThrowsExactlyAsync(() => _provider.UpdateFileAsync("f.txt", contents, UpdateFileOptions.Append)); - Assert.AreEqual("theirs", await Read("f.txt")); + _provider.Dispose(); + _provider = new WriteablePhysicalFileProvider(_root) { TempCreated = Once(() => File.WriteAllText(Disk("f.txt"), "theirs")) }; + await Write("f.txt", "+mine", UpdateFileOptions.Append); + Assert.AreEqual("theirs+mine", await Read("f.txt")); Assert.IsEmpty(TempFiles()); } @@ -773,15 +797,183 @@ public async Task Append_FailsWhenTheFileChangesWhileItCopies() [DataRow(true, false)] [DataRow(false, false)] [DataRow(false, true)] - public async Task Append_ToAMissingFileFailsWhenOneAppearsMeanwhile(bool byRenameWithoutReplacing, bool linkRefused) + public async Task Append_ToAMissingFile_AppendsToOneThatAppearsMeanwhile(bool byRenameWithoutReplacing, bool linkRefused) { - TakeNewNames(byRenameWithoutReplacing, linkRefused); - var contents = new InterceptingStream(Encoding.UTF8.GetBytes("mine"), () => File.WriteAllText(Disk("f.txt"), "theirs")); - await Assert.ThrowsExactlyAsync(() => _provider.UpdateFileAsync("f.txt", contents, UpdateFileOptions.Append)); + TakeNewNames(byRenameWithoutReplacing, linkRefused, Once(() => File.WriteAllText(Disk("f.txt"), "theirs"))); + await Write("f.txt", "mine", UpdateFileOptions.Append); + Assert.AreEqual("theirsmine", await Read("f.txt")); + Assert.IsEmpty(TempFiles()); + } + + [TestMethod] + public async Task Append_ToAFileThatChangesEveryTime_ThrowsAfterFiveAttempts() + { + await Write("f.txt", "start"); + var attempts = 0; + _provider.Dispose(); + _provider = new WriteablePhysicalFileProvider(_root) { TempCreated = _ => File.WriteAllText(Disk("f.txt"), new string('x', ++attempts)) }; + var e = await Assert.ThrowsExactlyAsync(() => Write("f.txt", "+mine", UpdateFileOptions.Append)); + Assert.AreEqual(5, attempts); + StringAssert.Contains(e.Message, "5 attempts"); + Assert.IsInstanceOfType(e.InnerException); + Assert.AreEqual("xxxxx", await Read("f.txt"), "Every other write is kept, and nothing of the append"); + Assert.IsEmpty(TempFiles()); + } + + [TestMethod] + public async Task Append_WithTheCallersVersion_FailsAtTheFirstChange() + { + await Write("f.txt", "start"); + var etag = (await _provider.GetFileInfoAsync("f.txt")).ETag; + var attempts = 0; + _provider.Dispose(); + _provider = new WriteablePhysicalFileProvider(_root) + { + TempCreated = _ => + { + attempts++; + File.WriteAllText(Disk("f.txt"), "theirs"); + }, + }; + var e = await Assert.ThrowsExactlyAsync(() => Write("f.txt", "+mine", UpdateFileOptions.Append with { IfMatch = etag })); + Assert.AreEqual(1, attempts); + Assert.DoesNotContain("attempts", e.Message); Assert.AreEqual("theirs", await Read("f.txt")); Assert.IsEmpty(TempFiles()); } + [TestMethod] + public async Task Append_WithTheCallersVersion_RefusesAChangeBetweenTheLookAndTheOpen() + { + await Write("f.txt", "start"); + var etag = (await _provider.GetFileInfoAsync("f.txt")).ETag; + _provider.Dispose(); + _provider = new WriteablePhysicalFileProvider(_root) { Probed = target => File.WriteAllText(target, "theirs") }; + await Assert.ThrowsExactlyAsync(() => Write("f.txt", "+mine", UpdateFileOptions.Append with { IfMatch = etag })); + Assert.AreEqual("theirs", await Read("f.txt")); + Assert.IsEmpty(TempFiles()); + } + + [TestMethod] + public async Task Append_WhoseStreamFails_EndsWithItsFailure() + { + // A pinned read of a file elsewhere throws the base type when that file changes: it is not this file's change. + await Write("f.txt", "start"); + var contents = new InterceptingStream("+mine"u8.ToArray(), () => throw new PreconditionFailedException("The source changed.")); + var e = await Assert.ThrowsExactlyAsync(() => _provider.UpdateFileAsync("f.txt", contents, UpdateFileOptions.Append)); + Assert.AreEqual("The source changed.", e.Message); + Assert.AreEqual("start", await Read("f.txt")); + Assert.IsEmpty(TempFiles()); + } + + [TestMethod] + public async Task Append_ToAFileDeletedBeforeItIsOpened_CreatesIt() + { + // The caller's stream is copied between the look at the path and the file's open: the file goes there. + await Write("f.txt", "old"); + var contents = new InterceptingStream("mine"u8.ToArray(), () => File.Delete(Disk("f.txt"))); + await _provider.UpdateFileAsync("f.txt", contents, UpdateFileOptions.Append); + Assert.AreEqual("mine", await Read("f.txt")); + Assert.IsEmpty(TempFiles()); + } + + [TestMethod] + [DataRow(true, false)] + [DataRow(false, false)] + [DataRow(false, true)] + public async Task Append_ToAMissingFile_WhenAFolderTakesTheName_IsAFolder(bool byRenameWithoutReplacing, bool linkRefused) + { + // A folder is what a write without conditions finds there: the append ends, unretried. + if (!byRenameWithoutReplacing && OperatingSystem.IsWindows()) + Assert.Inconclusive("Windows refuses an exclusive create over a folder as access denied; Linux and macOS, which take names this way, answer that the name exists."); + var attempts = 0; + TakeNewNames(byRenameWithoutReplacing, linkRefused, _ => + { + attempts++; + Directory.CreateDirectory(Disk("f.txt")); + }); + var e = await Assert.ThrowsExactlyAsync(() => Write("f.txt", "mine", UpdateFileOptions.Append)); + StringAssert.Contains(e.Message, "is a folder"); + Assert.AreEqual(1, attempts); + Assert.IsTrue(Directory.Exists(Disk("f.txt")), "The folder is left as it was"); + Assert.IsEmpty(TempFiles()); + } + + [TestMethod] + public async Task Append_RefusedBeforeItReadsTheStream_LeavesNoFolder() + { + await Assert.ThrowsExactlyAsync(() => Write("new/f.txt", "x", UpdateFileOptions.Append with { IfMatch = "1-1" })); + Assert.IsFalse(Directory.Exists(Disk("new")), "The caller's stream is copied only once the append's refusals have passed"); + } + + [TestMethod] + public async Task Append_ToAFileDeletedAsItIsLookedAt_CreatesIt() + { + await Write("f.txt", "old"); + _provider.Dispose(); + _provider = new WriteablePhysicalFileProvider(_root) { Probed = Once(() => File.Delete(Disk("f.txt"))) }; + await Write("f.txt", "mine", UpdateFileOptions.Append); + Assert.AreEqual("mine", await Read("f.txt"), "Created with the caller's bytes"); + Assert.IsEmpty(TempFiles()); + } + + [TestMethod] + public async Task Append_AfterALostCreate_ChecksTheFileItFindsAndTakesItsMode() + { + // The second attempt looks at the path again: the file another writer made is the one it replaces. + TakeNewNames(byRenameWithoutReplacing: OperatingSystem.IsWindows(), tempCreated: Once(() => + { + File.WriteAllText(Disk("f.txt"), "theirs"); + if (!OperatingSystem.IsWindows()) + File.SetUnixFileMode(Disk("f.txt"), UnixFileMode.UserRead | UnixFileMode.UserWrite); + })); + await Write("f.txt", "+mine", UpdateFileOptions.Append); + Assert.AreEqual("theirs+mine", await Read("f.txt")); + if (!OperatingSystem.IsWindows()) + Assert.AreEqual(UnixFileMode.UserRead | UnixFileMode.UserWrite, File.GetUnixFileMode(Disk("f.txt")), "The new file takes the bits of the one it replaces"); + } + + [TestMethod] + public async Task Append_SpoolsTheCallersBytesBesideTheFile_HiddenAndGoneAfterwards() + { + await Write("dir/f.txt", "old"); + var spools = new List(); + var names = new List(); + _provider.Dispose(); + _provider = new WriteablePhysicalFileProvider(_root) + { + TempCreated = temp => + { + spools.AddRange(Directory.GetFiles(Path.GetDirectoryName(temp)!, ".wfp-*.tmp").Where(file => file != temp).Select(file => Path.GetFileName(file))); + names.AddRange(_provider.EnumerateAsync("dir").ToBlockingEnumerable().Select(entry => entry.Name)); + }, + }; + await Write("dir/f.txt", "+more", UpdateFileOptions.Append); + Assert.HasCount(1, spools, "One spool, in the file's folder"); + Assert.IsTrue(WriteablePhysicalFileProvider.IsTempFile(spools[0]), "Named as a write's temp file, which the sweep takes when a crash leaves it"); + CollectionAssert.AreEqual(new[] { "f.txt" }, names, "Listings hide it"); + Assert.IsEmpty(TempFiles(), "Gone once the append ends"); + Assert.AreEqual("old+more", await Read("dir/f.txt")); + } + + [TestMethod] + [OSCondition(OperatingSystems.Linux | OperatingSystems.OSX)] + [UnsupportedOSPlatform("windows")] + public async Task Append_CreatesItsSpoolReadableOnlyByItsUser() + { + await Write("f.txt", "old"); + File.SetUnixFileMode(Disk("f.txt"), File.GetUnixFileMode(Disk("f.txt")) | UnixFileMode.GroupRead | UnixFileMode.OtherRead); + var modes = new List(); + _provider.Dispose(); + _provider = new WriteablePhysicalFileProvider(_root) + { + TempCreated = temp => modes.AddRange(Directory.GetFiles(Path.GetDirectoryName(temp)!, ".wfp-*.tmp").Where(file => file != temp).Select(File.GetUnixFileMode)), + }; + await Write("f.txt", "+more", UpdateFileOptions.Append); + Assert.HasCount(1, modes); + Assert.AreEqual(UnixFileMode.UserRead | UnixFileMode.UserWrite, modes[0], "Only the process's user may read the caller's bytes, whatever the file's mode"); + } + [TestMethod] public async Task Append_ChecksTheConditionsOnTheVersionItOpens() { diff --git a/tests/WriteableFileProviders.SharePoint.Tests/SharePointProviderTests.cs b/tests/WriteableFileProviders.SharePoint.Tests/SharePointProviderTests.cs index 8c34f93..7e95f8e 100644 --- a/tests/WriteableFileProviders.SharePoint.Tests/SharePointProviderTests.cs +++ b/tests/WriteableFileProviders.SharePoint.Tests/SharePointProviderTests.cs @@ -226,7 +226,7 @@ public async Task IfNotExists_IsSentWithASimpleUpload_AndAFileCreatedAfterTheChe } [TestMethod] - public async Task Append_RacedAfterItsCheck_IsRefusedByTheIfMatchItSends() + public async Task Append_RacedAfterItsCheck_IsRefusedByTheIfMatchItSends_ThenAppendsAfterTheRace() { var (provider, fake) = Create(); await Write(provider, "f.txt", "start"); @@ -238,8 +238,9 @@ public async Task Append_RacedAfterItsCheck_IsRefusedByTheIfMatchItSends() fake.SetFile("r/f.txt", Encoding.UTF8.GetBytes("theirs")); } }; - await Assert.ThrowsExactlyAsync(() => Write(provider, "f.txt", "+mine", UpdateFileOptions.Append)); - Assert.AreEqual("theirs", Text(fake.GetFile("r/f.txt")), "A service that honours If-Match keeps the concurrent write"); + await Write(provider, "f.txt", "+mine", UpdateFileOptions.Append); + Assert.AreEqual("theirs+mine", Text(fake.GetFile("r/f.txt")), "A service that honours If-Match keeps the concurrent write; the append reads it again"); + Assert.HasCount(3, Sent(fake, "PUT"), "The first write, the refused append, the append after the race"); } [TestMethod] diff --git a/tests/WriteableFileProviders.Testing/ProviderConformanceTests.cs b/tests/WriteableFileProviders.Testing/ProviderConformanceTests.cs index bb1fe2e..c7634a4 100644 --- a/tests/WriteableFileProviders.Testing/ProviderConformanceTests.cs +++ b/tests/WriteableFileProviders.Testing/ProviderConformanceTests.cs @@ -53,8 +53,8 @@ public abstract class ProviderConformanceTests protected virtual bool KeepsContentType => false; /// - /// Whether an append detects the concurrent write this suite stages (while the appended stream is read) and throws - /// instead of losing it; false where the staged race cannot be detected. + /// Whether an append detects the concurrent write this suite stages (while the appended stream is read), reads the + /// file again and appends after it, instead of losing it; false where the staged race cannot be detected. /// protected virtual bool DetectsConcurrentAppends => true; @@ -745,23 +745,15 @@ public async Task Write_IfNotExistsRaceHasOneWinner_Large() [TestMethod] public async Task Write_AppendRaceToAMissingFileKeepsBoth() { - // Providers end this race differently (one refuses the second create, one appends after it); every way keeps - // the writes that succeeded. Two writers, because with more, appenders that each found the file race each - // other's version too. + // The append that loses the create reads the file the other made and appends after it. Two writers: the winner + // is done before the loser tries again, so this holds where IfMatch is only checked, too. if (!RefusesConcurrentCreates) Assert.Inconclusive("The store this fixture runs on cannot settle a race of creates."); string[] parts = ["first;", "second;"]; var outcomes = await RaceAsync(parts.Length, writer => Provider.UpdateFileAsync("f.txt", TestStreams.Text(parts[writer]), UpdateFileOptions.Append)); - Assert.IsTrue(outcomes.Any(o => o is null), "At least one append succeeds"); foreach (var failure in outcomes.OfType()) - Assert.AreEqual(typeof(FileAlreadyExistsException), failure.GetType(), failure.ToString()); - var content = await Read("f.txt"); - for (var writer = 0; writer < parts.Length; writer++) - { - if (outcomes[writer] is null) - StringAssert.Contains(content, parts[writer], "A successful append is kept"); - } - Assert.AreEqual(parts.Where((_, writer) => outcomes[writer] is null).Sum(part => part.Length), content.Length, "Nothing else is in the file"); + Assert.Fail("Every append succeeds: " + failure); + CollectionAssert.Contains(new[] { "first;second;", "second;first;" }, await Read("f.txt"), "Both appends, one after the other"); } // Writers race to create "race.bin" with IfNotExists: exactly one wins, every other throws exactly @@ -829,16 +821,8 @@ public async Task Write_ConcurrentAppendIsNeverLost() await Write("f.txt", "start"); var racing = new InterceptingStream(Encoding.UTF8.GetBytes("+mine"), () => Provider.UpdateFileAsync("f.txt", TestStreams.Text("theirs")).GetAwaiter().GetResult()); - try - { - await Provider.UpdateFileAsync("f.txt", racing, UpdateFileOptions.Append); - } - catch (PreconditionFailedException) - { - Assert.AreEqual("theirs", await Read("f.txt"), "The concurrent write is kept"); - return; - } - StringAssert.Contains(await Read("f.txt"), "theirs", "The concurrent write is kept"); + await Provider.UpdateFileAsync("f.txt", racing, UpdateFileOptions.Append); + Assert.AreEqual("theirs+mine", await Read("f.txt"), "The concurrent write is kept, and the append lands after it"); } [TestMethod] From c8c0ccf897f2e0b89d7baada0147f7478fb259e8 Mon Sep 17 00:00:00 2001 From: Osvay Soma Date: Tue, 13 Oct 2026 10:33:57 +0200 Subject: [PATCH 3/6] Read past a lost create only once the other writer's file has content On Physical, where links are refused, a new name is taken with an empty file that the winner's content is renamed onto afterwards. An append that lost the name and read past that empty file appended to it, and the winner's rename then replaced the append, which had reported success. A lost create is now read past only once the file has content; while it is empty the append throws FileAlreadyExistsException, as before. Also: SharePoint's remarks on appends, what a caller's IfMatch gets for a file deleted before its read, and two comments. Refs #65 --- docs/plans/2026-10-13-2027-1-1.md | 7 ++- .../WriteableFileProvider.Core.cs | 3 +- src/WriteableFileProviders.Physical/README.md | 4 +- .../WriteablePhysicalFileProvider.cs | 13 ++++-- .../WriteableSharePointFileProvider.cs | 7 +-- .../PhysicalLiveTests.cs | 3 +- .../PhysicalProviderTests.cs | 43 +++++++++++++++++++ .../ProviderConformanceTests.cs | 4 +- 8 files changed, 72 insertions(+), 12 deletions(-) diff --git a/docs/plans/2026-10-13-2027-1-1.md b/docs/plans/2026-10-13-2027-1-1.md index a7ee9ca..44ef447 100644 --- a/docs/plans/2026-10-13-2027-1-1.md +++ b/docs/plans/2026-10-13-2027-1-1.md @@ -444,4 +444,9 @@ expectation (the winner is done before the loser retries); the locked restore of stub (`ScriptedStoreProvider`), not a mode of the stub; Physical's look reads whether a file is there once, before `Probed`, so a file deleted at that hook reaches `IsReplaceable`. The conformance and provider tests that expected an append to throw on a staged change (the two conformance tests, GCS's and SharePoint's) now expect the append after the - change; nothing else moved. + change; nothing else moved. Code review round 1 (standard, Opus): no blocker or major. Fixed: SharePoint's class + remarks (F1); on Physical's exclusive-create fallback, where links are refused, a retried append could land on the + winner's empty placeholder and be replaced by its rename while reporting success, reproduced by the refuter: an + append that loses a new name now reads past the file only once it has content, else throws + `FileAlreadyExistsException` as before (F2, the maintainer's choice), with a regression test that fails without it; + two doc nits (F3, F4). diff --git a/src/WriteableFileProviders.Common/WriteableFileProvider.Core.cs b/src/WriteableFileProviders.Common/WriteableFileProvider.Core.cs index 6630573..6675e5c 100644 --- a/src/WriteableFileProviders.Common/WriteableFileProvider.Core.cs +++ b/src/WriteableFileProviders.Common/WriteableFileProvider.Core.cs @@ -346,7 +346,8 @@ protected virtual ValueTask DisposeAsyncCore() /// does; then it throws with the last conflict inside. With the caller's /// or , the first conflict /// throws: for a file created meanwhile under - /// , otherwise. + /// , the store's "not found" ( to the + /// caller) for a file deleted before its read, otherwise. /// is read once: a retry copies the caller's bytes from the previous attempt's temp file, /// and a failure of the caller's stream ends the append, whatever its type. It keeps the file's content type unless /// the caller gives one. It needs the file's : an existing file without one diff --git a/src/WriteableFileProviders.Physical/README.md b/src/WriteableFileProviders.Physical/README.md index e0bdf3a..185ef10 100644 --- a/src/WriteableFileProviders.Physical/README.md +++ b/src/WriteableFileProviders.Physical/README.md @@ -55,7 +55,9 @@ None. The provider reads and writes with the permissions of the process. throws. - **An append copies the whole file**, so frequent appends to a large file (a log) are expensive. It first copies your stream to a hidden `.wfp-.tmp` beside the file (on Linux and macOS readable by the process's user only), so that - after a concurrent write it can read the file again and append after it, up to five attempts in all. On Windows, which + after a concurrent write it can read the file again and append after it, up to five attempts in all. Where the file + system has no hard links, an append that loses a new name to another writer still throws `FileAlreadyExistsException` + while that writer's file is empty. On Windows, which doesn't rename over a file that is open, one of two appends to one file at once can fail with `UnauthorizedAccessException`. - **A write puts a new file at the path.** Other hard links keep the old content, and a symbolic link at the path diff --git a/src/WriteableFileProviders.Physical/WriteablePhysicalFileProvider.cs b/src/WriteableFileProviders.Physical/WriteablePhysicalFileProvider.cs index ff74906..c5a5052 100644 --- a/src/WriteableFileProviders.Physical/WriteablePhysicalFileProvider.cs +++ b/src/WriteableFileProviders.Physical/WriteablePhysicalFileProvider.cs @@ -59,8 +59,11 @@ namespace WriteableFileProviders.Physical; /// fails when anything has the name, then drops the temp name: the name never holds a part of the file, and a process /// that dies in between leaves the file in place and a hidden temp name. Where the file system refuses hard links (FAT /// and exFAT, some network and FUSE file systems), and for the destination of a move, the name is taken with an -/// exclusive create and the content renamed onto it, so the file exists empty for that moment: a watcher sees it, and a -/// process that dies there leaves an empty file. On a network file system that may answer a resent request again (NFS), +/// exclusive create and the content renamed onto it, so the file exists empty for that moment: a watcher sees it, a +/// process that dies there leaves an empty file, and another writer that appends to it then has its append replaced by +/// that rename. An append that loses a new name reads past the file that took it only once that file has content, so it +/// never appends to such an empty file: while it is empty (or the other writer wrote nothing) it throws +/// . On a network file system that may answer a resent request again (NFS), /// a write can find its own new name taken and throw , a copy /// , with its content in place; an append then appends its bytes a second time. /// @@ -366,7 +369,11 @@ private async Task AppendOnceAsync(ResolvedPath path, string target, Stream cont if (Directory.Exists(target)) return IsAFolder(path, e); var exists = AlreadyExists(path, e); - signal(exists); + // Read past only once the other writer is done: where a name is taken with an exclusive create, it + // holds an empty file until that writer's content is renamed onto it, which would replace an append + // made to it meanwhile. + if (new FileInfo(target) is { Exists: true, Length: > 0 }) + signal(exists); return exists; } : null; diff --git a/src/WriteableFileProviders.SharePoint/WriteableSharePointFileProvider.cs b/src/WriteableFileProviders.SharePoint/WriteableSharePointFileProvider.cs index 2b5d9be..75321d8 100644 --- a/src/WriteableFileProviders.SharePoint/WriteableSharePointFileProvider.cs +++ b/src/WriteableFileProviders.SharePoint/WriteableSharePointFileProvider.cs @@ -29,9 +29,10 @@ namespace WriteableFileProviders.SharePoint; /// Preconditions are checked, not atomic: and /// are checked just before the write, and also sent to the service /// (If-Match, and the conflict behaviour fail), which the service enforces on uploads it documents them for. -/// An append rewrites the file with If-Match on the version it read, so a concurrent change it sees throws -/// . Reads pinned to a version check the ETag before each fetch, which is not -/// atomic either. +/// An append rewrites the file with If-Match on the version it read, so a concurrent change it sees is never +/// lost: with no condition of the caller's, it reads the file again and appends after it, up to five attempts in all, +/// then throws . Reads pinned to a version check the ETag before each fetch, +/// which is not atomic either. /// /// /// Signed URLs are read-only: the service's own pre-authenticated download URLs, which live up to an hour. A validity diff --git a/tests/WriteableFileProviders.Physical.Tests/PhysicalLiveTests.cs b/tests/WriteableFileProviders.Physical.Tests/PhysicalLiveTests.cs index 36b6038..fcda7ed 100644 --- a/tests/WriteableFileProviders.Physical.Tests/PhysicalLiveTests.cs +++ b/tests/WriteableFileProviders.Physical.Tests/PhysicalLiveTests.cs @@ -28,7 +28,8 @@ public sealed class PhysicalLiveTests : ProviderConformanceTests // The root is a folder of the disk, which the system resolves (dot segments included); paths under it stay inside it. protected override bool ChecksItsRoot => false; - // The version check before the rename is not atomic. + // The shared test writes from the appended stream, which Physical copies before it opens the file, so the write lands + // before the append reads the file, not between the read and the rename. protected override bool DetectsConcurrentAppends => false; protected override IWriteableFileProvider CreateProvider(string? root) diff --git a/tests/WriteableFileProviders.Physical.Tests/PhysicalProviderTests.cs b/tests/WriteableFileProviders.Physical.Tests/PhysicalProviderTests.cs index 30b678b..a92fa01 100644 --- a/tests/WriteableFileProviders.Physical.Tests/PhysicalProviderTests.cs +++ b/tests/WriteableFileProviders.Physical.Tests/PhysicalProviderTests.cs @@ -805,6 +805,49 @@ public async Task Append_ToAMissingFile_AppendsToOneThatAppearsMeanwhile(bool by Assert.IsEmpty(TempFiles()); } + [TestMethod] + public async Task Append_ThatLosesANameTakenByAnExclusiveCreate_NeverAppendsToItsEmptyFile() + { + // Where links are refused, a name is taken with an empty file, which the winner's content is renamed onto + // afterwards: an append made to that empty file would be replaced. The winner (A) takes the name, then waits; the + // loser (B) must not read past A's empty file. + using var aHoldsTheName = new ManualResetEventSlim(); + using var bIsDone = new ManualResetEventSlim(); + using var a = new WriteablePhysicalFileProvider(_root) + { + RenamesWithoutReplacing = false, + Link = (_, _) => 1, + NameTaken = _ => + { + aHoldsTheName.Set(); + bIsDone.Wait(TimeSpan.FromSeconds(30)); + }, + }; + Task? aWrites = null; + _provider.Dispose(); + _provider = new WriteablePhysicalFileProvider(_root) + { + RenamesWithoutReplacing = false, + Link = (_, _) => 1, + TempCreated = Once(() => + { + aWrites = Task.Run(() => a.UpdateFileAsync("f.txt", TestStreams.Text("first;"), UpdateFileOptions.Append)); + Assert.IsTrue(aHoldsTheName.Wait(TimeSpan.FromSeconds(30)), "The other writer took the name"); + }), + }; + try + { + await Assert.ThrowsExactlyAsync(() => Write("f.txt", "second;", UpdateFileOptions.Append)); + } + finally + { + bIsDone.Set(); + } + await aWrites!; + Assert.AreEqual("first;", await Read("f.txt")); + Assert.IsEmpty(TempFiles()); + } + [TestMethod] public async Task Append_ToAFileThatChangesEveryTime_ThrowsAfterFiveAttempts() { diff --git a/tests/WriteableFileProviders.Testing/ProviderConformanceTests.cs b/tests/WriteableFileProviders.Testing/ProviderConformanceTests.cs index c7634a4..b237584 100644 --- a/tests/WriteableFileProviders.Testing/ProviderConformanceTests.cs +++ b/tests/WriteableFileProviders.Testing/ProviderConformanceTests.cs @@ -745,8 +745,8 @@ public async Task Write_IfNotExistsRaceHasOneWinner_Large() [TestMethod] public async Task Write_AppendRaceToAMissingFileKeepsBoth() { - // The append that loses the create reads the file the other made and appends after it. Two writers: the winner - // is done before the loser tries again, so this holds where IfMatch is only checked, too. + // The append that loses the create reads the file the other made and appends after it. Two writers: the loser + // tries again only once the winner is done, so this holds where IfMatch is only checked, too. if (!RefusesConcurrentCreates) Assert.Inconclusive("The store this fixture runs on cannot settle a race of creates."); string[] parts = ["first;", "second;"]; From fc77a8cd81a397d61d193a0289f65496cfc1c269 Mon Sep 17 00:00:00 2001 From: Osvay Soma Date: Tue, 13 Oct 2026 11:23:27 +0200 Subject: [PATCH 4/6] Keep Windows reading past a lost create, and name Physical's corner where it can't Windows takes a new name with its content already in it, so an append that lost the name has no empty file to fear there: it reads past it as the other providers do. On Linux and macOS a lost create is read past unless the file there is still empty (a move's destination, a file system without hard links, or a file another writer left empty), and a name gone by then is read past again. The remarks, README, guide, errors.md, the exception's docs and the changelog say so. Refs #65 --- CHANGELOG.md | 5 +++- docs/guide/errors.md | 2 +- docs/guide/reading-and-writing.md | 3 +++ docs/plans/2026-10-13-2027-1-1.md | 13 ++++++++--- .../FileAlreadyExistsException.cs | 8 ++++--- .../IWriteableFileProvider.cs | 3 ++- src/WriteableFileProviders.Physical/README.md | 10 ++++---- .../WriteablePhysicalFileProvider.cs | 23 ++++++++++--------- .../PhysicalLiveTests.cs | 4 +++- 9 files changed, 45 insertions(+), 26 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f2bfa97..3375d8b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,7 +17,10 @@ version's section as its GitHub release notes. file created meanwhile. With your own `IfMatch` or `IfNotExists` the first conflict still throws. Where a store's answer to a write is lost and its SDK sends the write again, the append can meet its own write and append its bytes a second time, as a retry by hand would. Physical copies your stream to a hidden `.wfp-.tmp` beside the file - first; on Windows, one of two appends to one file at once can still fail with `UnauthorizedAccessException`. + first. On Linux and macOS, Physical still throws `FileAlreadyExistsException` for an append that loses a new name + while the file there is empty (a move's destination, a file system without hard links, or a file another writer left + empty), since the rename that fills such a file would undo the append; on Windows, one of two appends to one file at + once can still fail with `UnauthorizedAccessException`. ## 2027.1.0 (2026-10-13) diff --git a/docs/guide/errors.md b/docs/guide/errors.md index 9273da3..fd2ae40 100644 --- a/docs/guide/errors.md +++ b/docs/guide/errors.md @@ -8,7 +8,7 @@ for when you need its status or error code. | `ArgumentException` (and `ArgumentNullException`, `ArgumentOutOfRangeException`) | A path or an option the provider refuses. | | `FileNotFoundException` | The file doesn't exist, or is a folder. | | `FileExistsException` | The destination of a copy or a move exists. | -| `FileAlreadyExistsException` | A write with `IfNotExists` found a file at its path. It is a `PreconditionFailedException`. | +| `FileAlreadyExistsException` | A write with `IfNotExists` found a file at its path; or, on Physical on Linux and macOS, an append lost a new name to a file still empty ([appends](reading-and-writing.md#append)). It is a `PreconditionFailedException`. | | `PreconditionFailedException` | `IfMatch` didn't hold, a pinned read's file changed, an append with no condition found its file changed five times in a row ([appends](reading-and-writing.md#append)), a copy's or a move's source changed while it was copied, or the store reported a conflict with a concurrent operation (the write can be retried); and, as its subtype above, `IfNotExists` didn't hold. | | `NotSupportedException` | The provider or its store can't do it. | | `InvalidOperationException` | The store isn't set up as configured (a missing container, bucket, library or root folder, or no credentials to find), or you tried to modify the root. | diff --git a/docs/guide/reading-and-writing.md b/docs/guide/reading-and-writing.md index 8690ddd..9af10e7 100644 --- a/docs/guide/reading-and-writing.md +++ b/docs/guide/reading-and-writing.md @@ -65,6 +65,9 @@ asked for that version, or for no file at all. write as another writer's and appends its bytes a second time, as a caller who retried by hand would. - On Windows, which doesn't rename over a file that is open, Physical can't replace a file another append is reading: of two appends to one file at once, one can fail with `UnauthorizedAccessException`. +- On Linux and macOS, Physical's append that loses a new name throws `FileAlreadyExistsException` while the file there is + still empty (a move's destination, a file system without hard links, or a file another writer left empty): the rename + that fills such a file would undo an append made to it. ## Write only if unchanged, or only if absent diff --git a/docs/plans/2026-10-13-2027-1-1.md b/docs/plans/2026-10-13-2027-1-1.md index 44ef447..82cbd07 100644 --- a/docs/plans/2026-10-13-2027-1-1.md +++ b/docs/plans/2026-10-13-2027-1-1.md @@ -255,7 +255,9 @@ Package validation against 2027.0.0 stays green. other writer's content, where it threw `PreconditionFailedException` or `FileAlreadyExistsException`. It throws `PreconditionFailedException` only after five attempts. A caller that caught the exception to retry keeps working, and now rarely gets there; a caller that relied on an unconditional append failing to learn that someone else created the - file should pass `IfNotExists`, as the docs say. + file should pass `IfNotExists`, as the docs say. One corner keeps 2027.1.0's answer (code review F2, G1): on Linux and + macOS, Physical's append that loses a new name throws `FileAlreadyExistsException` while the file there is still + empty, since the rename that fills it would undo an append made to it. - **The Common API door (P1):** `AppendByRewriteAsync` is a protected helper of the base class, so it is inside the door. Its signature and its promise (a concurrent write is never lost) stay; what changes is that it retries where it threw. This plan reads that as a fix toward the door's own rule, one meaning across providers, which the difference @@ -300,7 +302,8 @@ commit. collide (S2); - `providers.md`: the paragraph on how an append to a missing file ends goes, but its caveat stays, that where `IfNotExists` is only checked a file created in between can be replaced (P11); the Append column's note; - - `errors.md`: `FileAlreadyExistsException` no longer comes from an append without `IfNotExists`; + - `errors.md`: `FileAlreadyExistsException` no longer comes from an append without `IfNotExists`, but for Physical's + corner on Linux and macOS; `PreconditionFailedException` from an append only after five attempts; - `writing-a-provider.md`: `AppendByRewriteAsync` retries. - **`docs/migration-2027.md`:** the rows "A racing append lost a write" and "A racing append or move went through" say @@ -449,4 +452,8 @@ expectation (the winner is done before the loser retries); the locked restore of winner's empty placeholder and be replaced by its rename while reporting success, reproduced by the refuter: an append that loses a new name now reads past the file only once it has content, else throws `FileAlreadyExistsException` as before (F2, the maintainer's choice), with a regression test that fails without it; - two doc nits (F3, F4). + two doc nits (F3, F4). Round 2: F1 to F4 verified; G1, the guard reached further than the placeholder (a writer that + wrote nothing, on any system; a move's destination on Linux and macOS): on the maintainer's choice it is skipped on + Windows, which never takes a name with an empty file, and the corner left on Linux and macOS is in the remarks, the + README, the guide, `errors.md`, the exception's docs and the changelog; G2, a name gone by the time the guard looks + is read past again; G3, the live fixture says the race test can fail by design without hard links. diff --git a/src/WriteableFileProviders.Common/FileAlreadyExistsException.cs b/src/WriteableFileProviders.Common/FileAlreadyExistsException.cs index db47831..b3a59d2 100644 --- a/src/WriteableFileProviders.Common/FileAlreadyExistsException.cs +++ b/src/WriteableFileProviders.Common/FileAlreadyExistsException.cs @@ -1,9 +1,11 @@ namespace WriteableFileProviders; /// -/// A write with found a file at its path; nothing was written. It is a -/// , so a catch of that type catches it. Not to be confused with -/// , which a copy or a move throws when its destination exists. +/// A write with found a file at its path; nothing was written. Physical on +/// Linux and macOS also throws it for an append that lost a new name to a file still empty, since the rename that fills +/// such a file would undo an append made to it. It is a , so a catch of +/// that type catches it. Not to be confused with , which a copy or a move throws when +/// its destination exists. /// public class FileAlreadyExistsException : PreconditionFailedException { diff --git a/src/WriteableFileProviders.Common/IWriteableFileProvider.cs b/src/WriteableFileProviders.Common/IWriteableFileProvider.cs index 9aab718..b8ce5b9 100644 --- a/src/WriteableFileProviders.Common/IWriteableFileProvider.cs +++ b/src/WriteableFileProviders.Common/IWriteableFileProvider.cs @@ -101,7 +101,8 @@ public interface IWriteableFileProvider : IFileProvider, IDisposable, IAsyncDisp /// How to write; overwrite with no conditions by default. /// Cancels the call. /// - /// is set and a file is at the path. + /// is set and a file is at the path; or, on a provider whose new files + /// are empty for a moment (Physical on Linux and macOS), an append lost a new name to such a file. /// /// /// did not hold, an append with no condition found the file changed on each diff --git a/src/WriteableFileProviders.Physical/README.md b/src/WriteableFileProviders.Physical/README.md index 185ef10..8bdd3e6 100644 --- a/src/WriteableFileProviders.Physical/README.md +++ b/src/WriteableFileProviders.Physical/README.md @@ -55,11 +55,11 @@ None. The provider reads and writes with the permissions of the process. throws. - **An append copies the whole file**, so frequent appends to a large file (a log) are expensive. It first copies your stream to a hidden `.wfp-.tmp` beside the file (on Linux and macOS readable by the process's user only), so that - after a concurrent write it can read the file again and append after it, up to five attempts in all. Where the file - system has no hard links, an append that loses a new name to another writer still throws `FileAlreadyExistsException` - while that writer's file is empty. On Windows, which - doesn't rename over a file that is open, one of two appends to one file at once can fail with - `UnauthorizedAccessException`. + after a concurrent write it can read the file again and append after it, up to five attempts in all. On Linux and + macOS, an append that loses a new name to another writer still throws `FileAlreadyExistsException` while the file + there is empty (a move's destination, a file system without hard links, or a file another writer left empty): the + rename that fills it would undo the append. On Windows, which doesn't rename over a file that is open, one of two + appends to one file at once can fail with `UnauthorizedAccessException`. - **A write puts a new file at the path.** Other hard links keep the old content, and a symbolic link at the path becomes a file. On Windows the file takes the folder's permissions; on Linux and macOS, the old file's mode bits. ACLs and extended attributes are not kept. diff --git a/src/WriteableFileProviders.Physical/WriteablePhysicalFileProvider.cs b/src/WriteableFileProviders.Physical/WriteablePhysicalFileProvider.cs index c5a5052..6d09009 100644 --- a/src/WriteableFileProviders.Physical/WriteablePhysicalFileProvider.cs +++ b/src/WriteableFileProviders.Physical/WriteablePhysicalFileProvider.cs @@ -54,18 +54,19 @@ namespace WriteableFileProviders.Physical; /// New names are taken atomically: , an append to a missing file, and /// the destination of a move or a copy never replace a file that is there; a write with /// that finds one throws , an append -/// appends after it, a move or a copy throws . Windows renames -/// without replacing. On Linux and macOS a write or a copy gives its temp file the name as a second hard link, which +/// appends after it, a move or a copy throws . Windows renames without replacing. On +/// Linux and macOS a write or a copy gives its temp file the name as a second hard link, which /// fails when anything has the name, then drops the temp name: the name never holds a part of the file, and a process /// that dies in between leaves the file in place and a hidden temp name. Where the file system refuses hard links (FAT /// and exFAT, some network and FUSE file systems), and for the destination of a move, the name is taken with an /// exclusive create and the content renamed onto it, so the file exists empty for that moment: a watcher sees it, a /// process that dies there leaves an empty file, and another writer that appends to it then has its append replaced by -/// that rename. An append that loses a new name reads past the file that took it only once that file has content, so it -/// never appends to such an empty file: while it is empty (or the other writer wrote nothing) it throws -/// . On a network file system that may answer a resent request again (NFS), -/// a write can find its own new name taken and throw , a copy -/// , with its content in place; an append then appends its bytes a second time. +/// that rename. On Linux and macOS, an append that loses a new name reads past the file that took it only once that file +/// has content, so it never appends to such an empty file: while it is empty (a move's destination, a file system +/// without hard links, or a file another writer left empty) it throws . On a +/// network file system that may answer a resent request again (NFS), a write can find its own new name taken and throw +/// , a copy , with its content in place; an +/// append then appends its bytes a second time. /// /// /// Reads share the file with writers and deleters, and keep the content they opened. A delete goes ahead while a @@ -369,10 +370,10 @@ private async Task AppendOnceAsync(ResolvedPath path, string target, Stream cont if (Directory.Exists(target)) return IsAFolder(path, e); var exists = AlreadyExists(path, e); - // Read past only once the other writer is done: where a name is taken with an exclusive create, it - // holds an empty file until that writer's content is renamed onto it, which would replace an append - // made to it meanwhile. - if (new FileInfo(target) is { Exists: true, Length: > 0 }) + // Read past only once the other writer is done: on Linux and macOS a name taken with an exclusive + // create holds an empty file until that writer's content is renamed onto it, which would replace an + // append made to it meanwhile. Windows takes a name with its content in it. + if (RenamesWithoutReplacing || new FileInfo(target) is not { Exists: true, Length: 0 }) signal(exists); return exists; } diff --git a/tests/WriteableFileProviders.Physical.Tests/PhysicalLiveTests.cs b/tests/WriteableFileProviders.Physical.Tests/PhysicalLiveTests.cs index fcda7ed..ba4bef2 100644 --- a/tests/WriteableFileProviders.Physical.Tests/PhysicalLiveTests.cs +++ b/tests/WriteableFileProviders.Physical.Tests/PhysicalLiveTests.cs @@ -9,7 +9,9 @@ namespace WriteableFileProviders.Physical.Tests; /// WFP_LIVE=1 it is inconclusive; with it, the folder comes from the WFP_PHYSICAL_ROOT environment /// variable, or PhysicalFileProvider_RootPath in an optional testsettings.json next to this project (see /// testsettings.sample.json), and without either every test fails. Each test works in a folder of its own under it and -/// deletes it; the folder itself is left as it was. +/// deletes it; the folder itself is left as it was. On Linux and macOS over a file system without hard links, +/// Write_AppendRaceToAMissingFileKeepsBoth can fail by design: the append that loses the name meets the winner's +/// file while it is still empty, and throws . /// [TestClass] [TestCategory("Live")] From e5d94cfeaa2f6c8dd042ecbeca882e83149c45f4 Mon Sep 17 00:00:00 2001 From: Osvay Soma Date: Tue, 13 Oct 2026 11:28:54 +0200 Subject: [PATCH 5/6] Pin the Windows read-past with an empty winner, and give the corner one reason A test now has another writer leave an empty file: where names are taken without replacing (Windows) the append reads past it, elsewhere it throws FileAlreadyExistsException. The docs give the reason that holds for all three cases (the provider can't tell an empty file from one a rename is about to fill), the interface's exception doc says what the others say, and the remarks' paragraph is reflowed. Refs #65 --- CHANGELOG.md | 4 ++-- docs/guide/reading-and-writing.md | 4 ++-- docs/plans/2026-10-13-2027-1-1.md | 2 +- .../FileAlreadyExistsException.cs | 8 +++---- .../IWriteableFileProvider.cs | 4 ++-- src/WriteableFileProviders.Physical/README.md | 6 +++--- .../WriteablePhysicalFileProvider.cs | 20 +++++++++--------- .../PhysicalProviderTests.cs | 21 +++++++++++++++++++ 8 files changed, 45 insertions(+), 24 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3375d8b..ea571c1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,8 +19,8 @@ version's section as its GitHub release notes. second time, as a retry by hand would. Physical copies your stream to a hidden `.wfp-.tmp` beside the file first. On Linux and macOS, Physical still throws `FileAlreadyExistsException` for an append that loses a new name while the file there is empty (a move's destination, a file system without hard links, or a file another writer left - empty), since the rename that fills such a file would undo the append; on Windows, one of two appends to one file at - once can still fail with `UnauthorizedAccessException`. + empty), since it can't tell such a file from one a rename is about to fill, which would undo the append; on Windows, + one of two appends to one file at once can still fail with `UnauthorizedAccessException`. ## 2027.1.0 (2026-10-13) diff --git a/docs/guide/reading-and-writing.md b/docs/guide/reading-and-writing.md index 9af10e7..6ea6c41 100644 --- a/docs/guide/reading-and-writing.md +++ b/docs/guide/reading-and-writing.md @@ -66,8 +66,8 @@ asked for that version, or for no file at all. - On Windows, which doesn't rename over a file that is open, Physical can't replace a file another append is reading: of two appends to one file at once, one can fail with `UnauthorizedAccessException`. - On Linux and macOS, Physical's append that loses a new name throws `FileAlreadyExistsException` while the file there is - still empty (a move's destination, a file system without hard links, or a file another writer left empty): the rename - that fills such a file would undo an append made to it. + still empty (a move's destination, a file system without hard links, or a file another writer left empty): it can't + tell such a file from one a rename is about to fill, which would undo an append made to it. ## Write only if unchanged, or only if absent diff --git a/docs/plans/2026-10-13-2027-1-1.md b/docs/plans/2026-10-13-2027-1-1.md index 82cbd07..e949c0b 100644 --- a/docs/plans/2026-10-13-2027-1-1.md +++ b/docs/plans/2026-10-13-2027-1-1.md @@ -257,7 +257,7 @@ Package validation against 2027.0.0 stays green. now rarely gets there; a caller that relied on an unconditional append failing to learn that someone else created the file should pass `IfNotExists`, as the docs say. One corner keeps 2027.1.0's answer (code review F2, G1): on Linux and macOS, Physical's append that loses a new name throws `FileAlreadyExistsException` while the file there is still - empty, since the rename that fills it would undo an append made to it. + empty, which it can't tell from one a rename is about to fill and would undo an append made to. - **The Common API door (P1):** `AppendByRewriteAsync` is a protected helper of the base class, so it is inside the door. Its signature and its promise (a concurrent write is never lost) stay; what changes is that it retries where it threw. This plan reads that as a fix toward the door's own rule, one meaning across providers, which the difference diff --git a/src/WriteableFileProviders.Common/FileAlreadyExistsException.cs b/src/WriteableFileProviders.Common/FileAlreadyExistsException.cs index b3a59d2..6fe9ee1 100644 --- a/src/WriteableFileProviders.Common/FileAlreadyExistsException.cs +++ b/src/WriteableFileProviders.Common/FileAlreadyExistsException.cs @@ -2,10 +2,10 @@ namespace WriteableFileProviders; /// /// A write with found a file at its path; nothing was written. Physical on -/// Linux and macOS also throws it for an append that lost a new name to a file still empty, since the rename that fills -/// such a file would undo an append made to it. It is a , so a catch of -/// that type catches it. Not to be confused with , which a copy or a move throws when -/// its destination exists. +/// Linux and macOS also throws it for an append that lost a new name to a file still empty, which it can't tell from one +/// a rename is about to fill. It is a , so a catch of that type catches +/// it. Not to be confused with , which a copy or a move throws when its destination +/// exists. /// public class FileAlreadyExistsException : PreconditionFailedException { diff --git a/src/WriteableFileProviders.Common/IWriteableFileProvider.cs b/src/WriteableFileProviders.Common/IWriteableFileProvider.cs index b8ce5b9..e219b44 100644 --- a/src/WriteableFileProviders.Common/IWriteableFileProvider.cs +++ b/src/WriteableFileProviders.Common/IWriteableFileProvider.cs @@ -101,8 +101,8 @@ public interface IWriteableFileProvider : IFileProvider, IDisposable, IAsyncDisp /// How to write; overwrite with no conditions by default. /// Cancels the call. /// - /// is set and a file is at the path; or, on a provider whose new files - /// are empty for a moment (Physical on Linux and macOS), an append lost a new name to such a file. + /// is set and a file is at the path; or, on Physical on Linux and macOS, + /// an append lost a new name to a file still empty. /// /// /// did not hold, an append with no condition found the file changed on each diff --git a/src/WriteableFileProviders.Physical/README.md b/src/WriteableFileProviders.Physical/README.md index 8bdd3e6..af8f894 100644 --- a/src/WriteableFileProviders.Physical/README.md +++ b/src/WriteableFileProviders.Physical/README.md @@ -57,9 +57,9 @@ None. The provider reads and writes with the permissions of the process. stream to a hidden `.wfp-.tmp` beside the file (on Linux and macOS readable by the process's user only), so that after a concurrent write it can read the file again and append after it, up to five attempts in all. On Linux and macOS, an append that loses a new name to another writer still throws `FileAlreadyExistsException` while the file - there is empty (a move's destination, a file system without hard links, or a file another writer left empty): the - rename that fills it would undo the append. On Windows, which doesn't rename over a file that is open, one of two - appends to one file at once can fail with `UnauthorizedAccessException`. + there is empty (a move's destination, a file system without hard links, or a file another writer left empty): it + can't tell such a file from one a rename is about to fill, which would undo the append. On Windows, which doesn't + rename over a file that is open, one of two appends to one file at once can fail with `UnauthorizedAccessException`. - **A write puts a new file at the path.** Other hard links keep the old content, and a symbolic link at the path becomes a file. On Windows the file takes the folder's permissions; on Linux and macOS, the old file's mode bits. ACLs and extended attributes are not kept. diff --git a/src/WriteableFileProviders.Physical/WriteablePhysicalFileProvider.cs b/src/WriteableFileProviders.Physical/WriteablePhysicalFileProvider.cs index 6d09009..58b2cf4 100644 --- a/src/WriteableFileProviders.Physical/WriteablePhysicalFileProvider.cs +++ b/src/WriteableFileProviders.Physical/WriteablePhysicalFileProvider.cs @@ -55,16 +55,16 @@ namespace WriteableFileProviders.Physical; /// the destination of a move or a copy never replace a file that is there; a write with /// that finds one throws , an append /// appends after it, a move or a copy throws . Windows renames without replacing. On -/// Linux and macOS a write or a copy gives its temp file the name as a second hard link, which -/// fails when anything has the name, then drops the temp name: the name never holds a part of the file, and a process -/// that dies in between leaves the file in place and a hidden temp name. Where the file system refuses hard links (FAT -/// and exFAT, some network and FUSE file systems), and for the destination of a move, the name is taken with an -/// exclusive create and the content renamed onto it, so the file exists empty for that moment: a watcher sees it, a -/// process that dies there leaves an empty file, and another writer that appends to it then has its append replaced by -/// that rename. On Linux and macOS, an append that loses a new name reads past the file that took it only once that file -/// has content, so it never appends to such an empty file: while it is empty (a move's destination, a file system -/// without hard links, or a file another writer left empty) it throws . On a -/// network file system that may answer a resent request again (NFS), a write can find its own new name taken and throw +/// Linux and macOS a write or a copy gives its temp file the name as a second hard link, which fails when anything has +/// the name, then drops the temp name: the name never holds a part of the file, and a process that dies in between +/// leaves the file in place and a hidden temp name. Where the file system refuses hard links (FAT and exFAT, some +/// network and FUSE file systems), and for the destination of a move, the name is taken with an exclusive create and the +/// content renamed onto it, so the file exists empty for that moment: a watcher sees it, a process that dies there +/// leaves an empty file, and another writer that appends to it then has its append replaced by that rename. On Linux +/// and macOS, an append that loses a new name reads past the file that took it only once that file has content, so it +/// never appends to such an empty file: while it is empty (a move's destination, a file system without hard links, or a +/// file another writer left empty) it throws . On a network file system that +/// may answer a resent request again (NFS), a write can find its own new name taken and throw /// , a copy , with its content in place; an /// append then appends its bytes a second time. /// diff --git a/tests/WriteableFileProviders.Physical.Tests/PhysicalProviderTests.cs b/tests/WriteableFileProviders.Physical.Tests/PhysicalProviderTests.cs index a92fa01..080932e 100644 --- a/tests/WriteableFileProviders.Physical.Tests/PhysicalProviderTests.cs +++ b/tests/WriteableFileProviders.Physical.Tests/PhysicalProviderTests.cs @@ -848,6 +848,27 @@ public async Task Append_ThatLosesANameTakenByAnExclusiveCreate_NeverAppendsToIt Assert.IsEmpty(TempFiles()); } + [TestMethod] + [DataRow(true)] + [DataRow(false)] + public async Task Append_ThatLosesANameToAnEmptyFile_ReadsPastIt_OnlyWhereNamesNeverHoldAnEmptyFile(bool byRenameWithoutReplacing) + { + // Windows takes a name with its content in it, so an empty file there is the other writer's own. Linux and macOS + // can't tell an empty file from one a rename is about to fill (a move's destination; the fallback without links). + TakeNewNames(byRenameWithoutReplacing, tempCreated: Once(() => File.WriteAllText(Disk("f.txt"), ""))); + if (byRenameWithoutReplacing) + { + await Write("f.txt", "mine", UpdateFileOptions.Append); + Assert.AreEqual("mine", await Read("f.txt")); + } + else + { + await Assert.ThrowsExactlyAsync(() => Write("f.txt", "mine", UpdateFileOptions.Append)); + Assert.AreEqual("", await Read("f.txt")); + } + Assert.IsEmpty(TempFiles()); + } + [TestMethod] public async Task Append_ToAFileThatChangesEveryTime_ThrowsAfterFiveAttempts() { From c441a3442917ed0db4c62d25ce2c0d5864943735 Mon Sep 17 00:00:00 2001 From: Osvay Soma Date: Tue, 13 Oct 2026 11:32:49 +0200 Subject: [PATCH 6/6] Word the corner's reason the same way in the exception's docs and the plan Refs #65 --- docs/plans/2026-10-13-2027-1-1.md | 7 +++++-- .../FileAlreadyExistsException.cs | 8 ++++---- 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/docs/plans/2026-10-13-2027-1-1.md b/docs/plans/2026-10-13-2027-1-1.md index e949c0b..949c6a1 100644 --- a/docs/plans/2026-10-13-2027-1-1.md +++ b/docs/plans/2026-10-13-2027-1-1.md @@ -257,7 +257,7 @@ Package validation against 2027.0.0 stays green. now rarely gets there; a caller that relied on an unconditional append failing to learn that someone else created the file should pass `IfNotExists`, as the docs say. One corner keeps 2027.1.0's answer (code review F2, G1): on Linux and macOS, Physical's append that loses a new name throws `FileAlreadyExistsException` while the file there is still - empty, which it can't tell from one a rename is about to fill and would undo an append made to. + empty: it can't tell such a file from one a rename is about to fill, which would undo an append made to it. - **The Common API door (P1):** `AppendByRewriteAsync` is a protected helper of the base class, so it is inside the door. Its signature and its promise (a concurrent write is never lost) stay; what changes is that it retries where it threw. This plan reads that as a fix toward the door's own rule, one meaning across providers, which the difference @@ -456,4 +456,7 @@ expectation (the winner is done before the loser retries); the locked restore of wrote nothing, on any system; a move's destination on Linux and macOS): on the maintainer's choice it is skipped on Windows, which never takes a name with an empty file, and the corner left on Linux and macOS is in the remarks, the README, the guide, `errors.md`, the exception's docs and the changelog; G2, a name gone by the time the guard looks - is read past again; G3, the live fixture says the race test can fail by design without hard links. + is read past again; G3, the live fixture says the race test can fail by design without hard links. Round 3: G1 to G3 + verified; H1, the interface's exception doc now says what errors.md says; H2, a test pins the Windows read-past + with an empty winner and the refusal elsewhere; H3 and H4, a reflow and one reason for the corner. Round 4: H1 to H4 + verified, clean; three wording nits applied. diff --git a/src/WriteableFileProviders.Common/FileAlreadyExistsException.cs b/src/WriteableFileProviders.Common/FileAlreadyExistsException.cs index 6fe9ee1..b590c45 100644 --- a/src/WriteableFileProviders.Common/FileAlreadyExistsException.cs +++ b/src/WriteableFileProviders.Common/FileAlreadyExistsException.cs @@ -2,10 +2,10 @@ namespace WriteableFileProviders; /// /// A write with found a file at its path; nothing was written. Physical on -/// Linux and macOS also throws it for an append that lost a new name to a file still empty, which it can't tell from one -/// a rename is about to fill. It is a , so a catch of that type catches -/// it. Not to be confused with , which a copy or a move throws when its destination -/// exists. +/// Linux and macOS also throws it for an append that lost a new name to a file still empty: Physical can't tell such a +/// file from one a rename is about to fill, which would undo the append. It is a +/// , so a catch of that type catches it. Not to be confused with +/// , which a copy or a move throws when its destination exists. /// public class FileAlreadyExistsException : PreconditionFailedException {