Report the real reason a mount fails instead of a broken pipe - #2105
Draft
tyrielv wants to merge 4 commits into
Draft
Report the real reason a mount fails instead of a broken pipe#2105tyrielv wants to merge 4 commits into
tyrielv wants to merge 4 commits into
Conversation
tyrielv
force-pushed
the
tyrielv/fix-mount-failure-reporting
branch
from
September 1, 2026 17:59
93cdb36 to
4e4ec42
Compare
When GVFS.Mount failed to mount, the client printed a transport error:
Could not connect to GVFS.Mount: BrokenPipeException: Unable to send: GetStatus
The real cause was written only to the mount process log. A user hitting a
ProjFS attach failure on a Dev Drive got no cause and no remedy, although
the mount process had already diagnosed it.
Two defects combined to lose the reason.
First, FailMountAndExit called fileSystemCallbacks.Dispose(), which reached
GitIndexProjection.Dispose(bool) and called Task.Dispose() on the still
running index-parsing thread. Shutdown() had not run, so the task was not in
a completion state and Task.Dispose threw InvalidOperationException. That
secondary exception killed the process before it could serve a GetStatus
response.
Second, even without the exception, FailMountAndExit called Environment.Exit
immediately after setting MountState.MountFailed. The client polls GetStatus
every 100ms, so the process usually died before the client observed the
state. MountState.MountFailed and the GetStatus.MountFailed response already
existed but were never seen.
Changes:
- GitIndexProjection.Dispose(bool) only disposes the index-parsing task once
it has completed. A Task holds no unmanaged resources, so skipping the
call is safe, and it no longer throws during a failed mount.
- FailMountAndExit keeps the named pipe answering GetStatus after a failure,
and exits once a client has read MountFailed. Callbacks are disposed after
the failure is reported, and disposal exceptions are logged instead of
terminating the process. A latch makes the first failing thread the only
one that tears down and exits.
The wait is deliberately short. Until the process exits it still holds the
mount lock and its named pipe, and a "gvfs mount" retry inside that window
hits IsExistingPipeListening and is told "The repo at ... is already
mounted" with exit code Success. So the wait is capped at 2 seconds, or
500ms when no client has polled GetStatus at all, and normally ends on the
client's next poll, about 100ms. Failures raised before the pipe server
starts do not wait; MountVerb already detects those through the mount
process exit code.
- GetStatus.Response carries a MountError field, so WaitUntilMounted prints
the mount process's own message rather than "Failed to mount at <path>".
- StartVirtualizing failures with 0x800701DE
(ERROR_FILE_SYSTEM_VIRTUALIZATION_NOT_AVAILABLE) name the cause and the
remedy: PrjFlt cannot attach to the volume, and on a Dev Drive it must be
allowed with "fsutil devdrv setfiltersallowed PrjFlt".
Tests: disposing a projection whose parsing thread is still running must not
throw, and WaitUntilMounted must surface MountError when the mount process
sends one, falling back to the generic message when it does not. Both fail
against the unfixed code.
Assisted-by: Claude Opus 5
Signed-off-by: Tyrie Vella <tyrielv@gmail.com>
tyrielv
force-pushed
the
tyrielv/fix-mount-failure-reporting
branch
from
September 1, 2026 18:05
4e4ec42 to
9ba27f1
Compare
Verified the original fix end-to-end on a real ReFS Dev Drive with PrjFlt not on the allowed-filter list. The baseline reproduced the reported BrokenPipeException exactly, and this branch now prints the real cause. That run, plus a multi-perspective self-review, turned up several defects in the first version of this change. Fix a second copy of the disposal bug. The repro showed the InvalidOperationException still being thrown after the GitIndexProjection fix, now from BackgroundFileSystemTaskRunner.Dispose, which disposes its own still-running Task. Only the new try/catch kept the report alive. Both classes now drop the Task.Dispose call outright rather than guarding it: a Task holds no unmanaged resources and Task.Dispose is legacy IAsyncResult cleanup. Correct the Dev Drive remedy, which was wrong and destructive. It advised "fsutil devdrv setfiltersallowed PrjFlt". Without /volume that applies to every developer volume on the machine, and setFiltersAllowed replaces the list rather than appending to it, so following the advice would drop filters the machine already requires. The message now names the enlistment's own volume, tells the user to read the current list first, and says the list is replaced. Do not let message formatting hang the mount process. FailMountAndExit formatted the message twice, and the second call was unprotected. A malformed format string threw there, and an outer catch block then re-entered FailMountAndExit on the same thread, where the latch parked it forever holding the mount lock and the pipe. Format once, and make the latch re-entrant for the thread that already owns it. Other fixes from the review: - Only count a client that actually received a status. clientPolledStatus was set before the send succeeded, so a client that connected and disconnected still extended the wait. - Raise the drain above the client's 100ms poll interval. Any client can satisfy the wait, so a concurrent "gvfs status" could consume the signal while the mounting client was still sleeping. - Trace the report timeout and the swallowed disposal exception with Keywords.Telemetry. Without it, a failure that used to be a process-fatal crash became invisible in the field. - Shorten the test's server-task wait so a failing assertion is not delayed. Adds a regression test for the BackgroundFileSystemTaskRunner disposal. Both disposal tests are mutation-checked. Assisted-by: Claude Opus 5 Signed-off-by: Tyrie Vella <tyrielv@gmail.com>
The failure-reporting change had grown to three constants, four fields and two methods inside InProcessMount, a 2158-line class that already carried 74 private fields. The state was cohesive but scattered, and it was reachable from only five places, so it moves out cleanly. MountFailureReporter now owns the handshake: the ownership latch, the published failure message, the pipe-ready and client-received flags, the bounded wait, and the safe message formatting. InProcessMount keeps the policy that belongs to it -- the MountFailed state transition, tracing, disposal, and process exit. Ownership is expressed as a MountFailureOwnership enum (Owner, Reentrant, NotOwner) rather than a bare interlocked int, so the re-entrant case is named instead of implied by a thread-id comparison at the call site. The class lives in GVFS.Common/NamedPipes beside the protocol it implements. The client half of the same handshake, GVFSEnlistment.WaitUntilMounted, is already in GVFS.Common, so both halves now sit together. Placing it there also makes the state machine testable. GVFS.UnitTests references GVFS.Common but not GVFS.Mount, so while this logic lived in InProcessMount none of it could be covered, and CI runs only GVFS.UnitTests. Adds 11 tests for ownership and re-entrancy, the wait and its two timeouts, the distinction between a status that carried the failure and one that did not, and the formatting fallback. Two are mutation-checked. No behavior change. Verified on the same ReFS Dev Drive repro: the mount still prints the real ProjFS cause, and the mount log has no disposal exception and no report timeout. Assisted-by: Claude Opus 5 Signed-off-by: Tyrie Vella <tyrielv@gmail.com>
A second self-review pass over the extraction. No defects were found -- every finding was a judgement call, and the security, async and rollout lenses were clean -- but three were worth acting on. Narrow the public surface. The three Default* timeout constants are read nowhere outside the class, so they are now internal. GVFS.Common already grants InternalsVisibleTo to GVFS.UnitTests, so the tests still see them. The class and the ownership enum stay public because GVFS.Mount consumes them across an assembly boundary. Cover the public constructor. Every test built the reporter through the internal test constructor, so nothing exercised the public one or its forwarding of the Default* constants. Swapping DefaultReportTimeoutMs and DefaultNoClientTimeoutMs would have shipped unnoticed. Two tests now close that: one drives the public constructor down the no-client path, and one asserts the ordering invariants the constants have to satisfy. Mutation-checked by swapping the two values. Cut the fixture's wall-clock cost. One test has to wait out the long timeout to prove the long timeout was selected, and the fixture's own constants made that 2000ms. They are arbitrary -- they only have to be far enough apart to tell the two paths apart -- so the long one drops to 600ms. The assertion now checks against the short timeout rather than a fixed margin below the long one, which is what actually proves the selection. Fixture time falls from ~2.9s to ~2.0s. Also: document why InProcessMount deliberately never disposes the reporter, and stop discarding the result of the stand-in server's Wait so a hung server task is reported instead of silently faulting. Measured on this machine: 977 tests pass, and the timing assertions survived six consecutive runs with all twelve cores saturated. Assisted-by: Claude Opus 5 Signed-off-by: Tyrie Vella <tyrielv@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
gvfs mountreports a transport error instead of the real failure:The message names no cause and no remedy.
GVFS.Mounthad already diagnosed thereal problem and written it to its own log:
0x800701DEisERROR_FILE_SYSTEM_VIRTUALIZATION_NOT_AVAILABLE— "The filesystem minifilter cannot attach to the developer volume." PrjFlt was not on the
Dev Drive filter allow-list. The user had to read the mount log to find that out.
This was a regression. 1.0 attached ProjFS in the client process, so an attach
failure printed
Attaching ProjFS to volume...Failedon the console the user waslooking at. 2.0 delegates ProjFS setup to
GVFS.Mountand depends on the namedpipe to carry the error back — and that did not happen.
Why the reason was lost
Two defects combined.
1. A secondary exception killed the process. After
StartVirtualizingfailed,
FailMountAndExitcalledfileSystemCallbacks.Dispose(), which reachedGitIndexProjection.Dispose(bool)and calledTask.Dispose()on the stillrunning
indexParsingThread.Shutdown()had never run, so the task was not ina completion state:
That exception escaped before the process could serve a
GetStatusresponse.2. The process exited before the client could poll. Even without the
exception,
FailMountAndExitcalledEnvironment.Exitimmediately after settingMountState.MountFailed.WaitUntilMountedpollsGetStatusevery 100ms, sothe process usually died first.
MountState.MountFailedand theGetStatus.MountFailedresponse already existed and were already polled for —they were simply never observed.
Changes
GitIndexProjection.Dispose(bool)disposes the index-parsing task onlyonce it has completed. A
Taskholds no unmanaged resources, so skipping thecall is safe, and disposal no longer throws during a failed mount.
InProcessMount.FailMountAndExitkeeps the named pipe answeringGetStatusafter a failure, and exits once a client has readMountFailed.The wait is deliberately short — until the process exits it still holds the
mount lock and its named pipe, and a
gvfs mountretry inside that window hitsIsExistingPipeListeningand is told "The repo at ... is already mounted"with exit code
Success. So the wait is capped at 2 seconds, or 500ms when noclient has polled
GetStatusat all, and normally ends on the client's nextpoll (~100ms). Failures raised before the pipe server starts do not wait —
MountVerbalready detects those through the mount process exit code.Callbacks are disposed after the failure is reported, and disposal exceptions
are logged instead of terminating the process. A latch makes the first failing
thread the only one that tears down and exits.
GetStatus.Responsecarries aMountErrorfield, soWaitUntilMountedprints the mount process's own message instead of the generic
Failed to mount at <path>.0x800701DEis translated into an actionable message.StartVirtualizingfailures with
ERROR_FILE_SYSTEM_VIRTUALIZATION_NOT_AVAILABLEnow name thecause and the remedy, including the enlistment's own volume. The message
deliberately tells the user to read the current allowed-filter list first,
because
fsutil devdrv setFiltersAllowedreplaces the list rather thanappending to it — and without
/volumeit applies to every developer volumeon the machine.
Automatically enabling the ProjFS filter on a Dev Drive is out of scope here.
This PR changes error reporting only.
Tests
Two new unit test fixtures:
GitIndexProjectionDisposeTests— disposing a projection whose parsing threadis still running must not throw, and a completed task must still be disposed.
WaitUntilMountedFailureReportingTests—WaitUntilMountedmust surfaceMountErrorwhen the mount process sends one, and fall back to the genericmessage when it does not. Drives a real named pipe.
Both fixtures were mutation-checked: reverting either production fix makes the
corresponding test fail.
Manual verification
Verified end-to-end on a real ReFS Dev Drive (
E:, ARM64, PrjFlt not on theallowed-filter list), cloning and mounting
ForTestsunelevated.Baseline — installed 2.0.26229.1, the exact version from the report:
with the real cause visible only in the mount log, along with the secondary exception:
With this change:
The mount log for that run contains no
InvalidOperationExceptionand no disposalwarning. A back-to-back
gvfs mountretry printed the real error both times — nospurious "already mounted".
A second copy of the disposal bug, found only by the repro
The first round of fixes was incomplete. Re-running the repro showed the
InvalidOperationExceptionstill being thrown — this time fromBackgroundFileSystemTaskRunner.Dispose, which disposes its own still-runningTaskonthe same
FileSystemCallbacks.Dispose()path. The report survived only because of thenew defensive
try/catch. Both classes now drop theTask.Dispose()call outright: aTaskholds no unmanaged resources andTask.Disposeis legacyIAsyncResultwait-handle cleanup. Both have mutation-checked regression tests.
On the post-failure pipe lifetime
The mount process stays alive briefly after a failure so the client can read the reason.
This does not meaningfully extend the window in which
IsExistingPipeListeningreports "already mounted":
StartNamedPipe()runs early — before network auth, cacheresolution, hooks, and virtualization — so the pipe is already listening for the entire
mount attempt. Measured on the baseline run, the pipe was open 1,147 ms before the
failure even occurred, on a tiny test repo; on a real enlistment it is far longer. This
change appends at most 2s, and typically ~150ms, to a pre-existing and larger window.
Branch target
master. This restores an error message that 1.0 produced and 2.0 lost, so it isa 2.0 regression fix and in scope for stabilization.