Skip to content

fix: adopt the handle in IFileStreamFactory.New(SafeFileHandle, ...) - #1087

Closed
Mpdreamz wants to merge 1 commit into
Testably:mainfrom
Mpdreamz:fix/filestream-adopts-handle
Closed

Mpdreamz wants to merge 1 commit into
Testably:mainfrom
Mpdreamz:fix/filestream-adopts-handle

Conversation

@Mpdreamz

@Mpdreamz Mpdreamz commented Sep 20, 2026 •

Copy link
Copy Markdown
Contributor

Fixes #1086.

A real FileStream built from a SafeFileHandle adopts it — the file is not opened again and no new file share is taken — so it can never conflict with whatever already holds the file open. FileStreamFactoryMock resolved the handle to a path and opened that path again with a fresh share, so the mock could throw a sharing violation where the real file system succeeds.

All three handle overloads now go through one helper that opens without taking a share, using the existing FileHandle.Ignore.

I did not use ignoreFileShare for this, although it looks like the obvious lever. It is inert on Windows — FileHandle.GrantAccess only consults it inside if (!_fileSystem.Execute.IsWindows) — and three call sites in InMemoryStorage depend on its current meaning, so widening it seemed like your call rather than a side effect of this fix. Whether that is itself a bug is noted in #1086.

Tests cover all three overloads against a file already held exclusively, plus that isAsync: true really does produce an asynchronous stream. They simulate Windows, since that is where the file share is enforced, so they are behind CAN_SIMULATE_OTHER_OS.

Independent of #1081, #1082 and #1083 — this branches from main. I ran into the divergence there and raised it separately per your note.

Run in Release, which is what puts the real file system into the test matrix: Testably.Abstractions.Tests 12075 on net8.0 and 12584 on net10.0, and Testing.Tests 1313 including the five new ones. All pass.

Happy to rework it however you prefer.

🤖 Generated with Claude Code

Copilot AI lite review requested due to automatic review settings September 20, 2026 13:45

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

Adopted handles need a non-mutating mode to avoid create/truncate behavior, with coverage for those modes.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 High severity

Open (1)
What changed in this PR

Updates FileStreamFactoryMock to adopt SafeFileHandle instances without acquiring a second file share.

Changes:

  • Centralizes handle overloads through one helper.
  • Adds adopted-handle behavior bypassing new share registration.
  • Adds Windows-simulation tests for sharing and async behavior.
File Summary
Tests/​Testably.Abstractions.Testing.Tests/​FileSystem/​FileStreamFactoryMockTests.AdoptHandle.cs Tests handle adoption across overloads, sharing, and async behavior.
Source/​Testably.Abstractions.Testing/​FileSystem/​FileStreamMock.cs Supports streams that do not acquire a new share.
Source/​Testably.Abstractions.Testing/​FileSystem/​FileStreamFactoryMock.cs Centralizes handle-based stream creation.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment on lines +236 to +238
safeFileHandleMock.Mode,
access,
safeFileHandleMock.Share,
A real `FileStream` constructed from a `SafeFileHandle` adopts it: the
file is not opened again, no new file share is taken, and the stream can
never conflict with whatever already holds the file open.

`FileStreamFactoryMock` resolved the handle to a path and opened that
path again, taking a fresh share, so layering a stream on a handle could
fail with a sharing violation where the real file system succeeds.

All three handle overloads now open without taking a share. The existing
`ignoreFileShare` parameter was not used for this: it is inert on Windows
and three call sites in `InMemoryStorage` depend on its current meaning.

Fixes Testably#1086

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@Mpdreamz
Mpdreamz force-pushed the fix/filestream-adopts-handle branch from 8f275a2 to 88f7fd0 Compare September 20, 2026 18:04
@Mpdreamz

Copy link
Copy Markdown
Contributor Author

Trimmed the comments here after your note on #1091: the same explanation appeared twice, once as <remarks> on the private helper and once inline. The helper keeps none and one short line remains where the decision is made — why a stream built on a handle takes no share of its own.

No behaviour change; CI was green before and the diff is comment-only.

@Mpdreamz

Copy link
Copy Markdown
Contributor Author

No code change — correcting the verification note in the description.

It quoted Debug numbers, and Debug leaves the real file system out of the test matrix: FileSystemTestsAttribute only adds it in Release. Re-run in Release: Testably.Abstractions.Tests 12075 on net8.0 and 12584 on net10.0, Testing.Tests 1313 including the five new ones, all passing. It also drops the mention of two macOS access-time failures I had been describing as pre-existing; they are a Debug-only artefact and do not occur in Release.

@Mpdreamz

Copy link
Copy Markdown
Contributor Author

Closing this, since it is covered by what shipped in 7.1.0:

Thanks, @vbreuss.

@Mpdreamz Mpdreamz closed this Sep 30, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

FileStreamFactoryMock re-opens the path behind a SafeFileHandle instead of adopting the handle

2 participants