diff --git a/CHANGELOG.md b/CHANGELOG.md index dbd4442..ea571c1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,23 @@ 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 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 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) 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..fd2ae40 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; 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. | | `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..6ea6c41 100644 --- a/docs/guide/reading-and-writing.md +++ b/docs/guide/reading-and-writing.md @@ -52,14 +52,22 @@ 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`. +- 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): 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/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 new file mode 100644 index 0000000..949c6a1 --- /dev/null +++ b/docs/plans/2026-10-13-2027-1-1.md @@ -0,0 +1,462 @@ +--- +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. 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: 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 + 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`, 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 + 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. +- **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. 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). 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. 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.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..b590c45 100644 --- a/src/WriteableFileProviders.Common/FileAlreadyExistsException.cs +++ b/src/WriteableFileProviders.Common/FileAlreadyExistsException.cs @@ -1,11 +1,11 @@ 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. Physical on +/// 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 { diff --git a/src/WriteableFileProviders.Common/IWriteableFileProvider.cs b/src/WriteableFileProviders.Common/IWriteableFileProvider.cs index 31672f6..e219b44 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,12 @@ 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; or, on Physical on Linux and macOS, + /// an append lost a new name to a file still empty. /// /// - /// 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..6675e5c 100644 --- a/src/WriteableFileProviders.Common/WriteableFileProvider.Core.cs +++ b/src/WriteableFileProviders.Common/WriteableFileProvider.Core.cs @@ -338,13 +338,20 @@ 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 + /// , 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 + /// throws . /// /// The file. /// The content to add. @@ -354,6 +361,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 +391,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 +432,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 +507,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..af8f894 100644 --- a/src/WriteableFileProviders.Physical/README.md +++ b/src/WriteableFileProviders.Physical/README.md @@ -53,7 +53,13 @@ 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 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): 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 645da07..58b2cf4 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,21 +43,30 @@ 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 -/// 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, 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. +/// 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 +/// , 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 @@ -101,6 +111,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 +306,125 @@ 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); + // 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; + } + : 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 +477,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 +815,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 +861,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 +1112,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/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.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/PhysicalLiveTests.cs b/tests/WriteableFileProviders.Physical.Tests/PhysicalLiveTests.cs index 36b6038..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")] @@ -28,7 +30,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 6105f87..080932e 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,247 @@ 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_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] + [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() + { + 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..b237584 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 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;"]; 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]