From 57bb761deaa0aee3ec6963bacebd757e93876b9f Mon Sep 17 00:00:00 2001 From: Tyrie Vella Date: Fri, 12 Jun 2026 10:08:08 -0700 Subject: [PATCH 1/2] Mount: prevent process crash on unhandled request handler exceptions HandleRequest now catches exceptions from individual pipe request handlers instead of letting them propagate to OnNewConnection, which calls Environment.Exit on any unhandled exception. A single transient error (network timeout, disk I/O failure) in a download handler would crash the entire mount process, breaking all pipe connections. Both catch sites use exception filters to exclude OutOfMemoryException, which indicates a corrupted heap state where continuing is unsafe. StackOverflowException and AccessViolationException are already uncatchable in .NET Core and need no explicit exclusion. HandleDownloadObjectRequest is refactored to isolate the download logic in DownloadObject and wrap it in a try-catch that returns a DownloadFailed response on exception. The read-object hook then receives a proper failure response instead of ERROR_BROKEN_PIPE (109), and git handles the object-not-available error more gracefully. Assisted-by: Claude Opus 4.6 Signed-off-by: Tyrie Vella --- GVFS/GVFS.Mount/InProcessMount.cs | 205 +++++++++++++++++------------- 1 file changed, 118 insertions(+), 87 deletions(-) diff --git a/GVFS/GVFS.Mount/InProcessMount.cs b/GVFS/GVFS.Mount/InProcessMount.cs index 2695113c0..1896d21f6 100644 --- a/GVFS/GVFS.Mount/InProcessMount.cs +++ b/GVFS/GVFS.Mount/InProcessMount.cs @@ -495,68 +495,79 @@ private void HandleRequest(ITracer tracer, string request, NamedPipeServer.Conne return; } - switch (message.Header) + try { - case NamedPipeMessages.GetStatus.Request: - this.HandleGetStatusRequest(connection); - break; + switch (message.Header) + { + case NamedPipeMessages.GetStatus.Request: + this.HandleGetStatusRequest(connection); + break; - case NamedPipeMessages.Unmount.Request: - this.HandleUnmountRequest(connection); - break; + case NamedPipeMessages.Unmount.Request: + this.HandleUnmountRequest(connection); + break; - case NamedPipeMessages.AcquireLock.AcquireRequest: - this.HandleLockRequest(message.Body, connection); - break; + case NamedPipeMessages.AcquireLock.AcquireRequest: + this.HandleLockRequest(message.Body, connection); + break; - case NamedPipeMessages.ReleaseLock.Request: - this.HandleReleaseLockRequest(message.Body, connection); - break; + case NamedPipeMessages.ReleaseLock.Request: + this.HandleReleaseLockRequest(message.Body, connection); + break; - case NamedPipeMessages.DownloadObject.DownloadRequest: - this.HandleDownloadObjectRequest(message, connection); - break; + case NamedPipeMessages.DownloadObject.DownloadRequest: + this.HandleDownloadObjectRequest(message, connection); + break; - case NamedPipeMessages.ModifiedPaths.ListRequest: - this.HandleModifiedPathsListRequest(message, connection); - break; + case NamedPipeMessages.ModifiedPaths.ListRequest: + this.HandleModifiedPathsListRequest(message, connection); + break; - case NamedPipeMessages.PostIndexChanged.NotificationRequest: - this.HandlePostIndexChangedRequest(message, connection); - break; + case NamedPipeMessages.PostIndexChanged.NotificationRequest: + this.HandlePostIndexChangedRequest(message, connection); + break; - case NamedPipeMessages.PrepareForUnstage.Request: - this.HandlePrepareForUnstageRequest(message, connection); - break; + case NamedPipeMessages.PrepareForUnstage.Request: + this.HandlePrepareForUnstageRequest(message, connection); + break; - case NamedPipeMessages.RunPostFetchJob.PostFetchJob: - this.HandlePostFetchJobRequest(message, connection); - break; + case NamedPipeMessages.RunPostFetchJob.PostFetchJob: + this.HandlePostFetchJobRequest(message, connection); + break; - case NamedPipeMessages.DehydrateFolders.Dehydrate: - this.HandleDehydrateFolders(message, connection); - break; + case NamedPipeMessages.DehydrateFolders.Dehydrate: + this.HandleDehydrateFolders(message, connection); + break; - case NamedPipeMessages.PrefetchCommits.Request: - this.HandlePrefetchCommitsRequest(connection); - break; + case NamedPipeMessages.PrefetchCommits.Request: + this.HandlePrefetchCommitsRequest(connection); + break; - case NamedPipeMessages.PrefetchBlobs.RequestHeader: - this.HandlePrefetchBlobsRequest(message, connection); - break; + case NamedPipeMessages.PrefetchBlobs.RequestHeader: + this.HandlePrefetchBlobsRequest(message, connection); + break; - case NamedPipeMessages.HydrationStatus.Request: - this.HandleGetHydrationStatusRequest(connection); - break; + case NamedPipeMessages.HydrationStatus.Request: + this.HandleGetHydrationStatusRequest(connection); + break; - default: - EventMetadata metadata = new EventMetadata(); - metadata.Add("Area", "Mount"); - metadata.Add("Header", message.Header); - this.tracer.RelatedError(metadata, "HandleRequest: Unknown request"); + default: + EventMetadata metadata = new EventMetadata(); + metadata.Add("Area", "Mount"); + metadata.Add("Header", message.Header); + this.tracer.RelatedError(metadata, "HandleRequest: Unknown request"); - connection.TrySendResponse(NamedPipeMessages.UnknownRequest); - break; + connection.TrySendResponse(NamedPipeMessages.UnknownRequest); + break; + } + } + catch (Exception e) when (e is not OutOfMemoryException) + { + EventMetadata metadata = new EventMetadata(); + metadata.Add("Area", "Mount"); + metadata.Add("Header", message.Header); + metadata.Add("Exception", e.ToString()); + this.tracer.RelatedError(metadata, "HandleRequest: Unhandled exception in request handler"); } } @@ -901,56 +912,76 @@ private void HandleDownloadObjectRequest(NamedPipeMessages.Message message, Name } else { - Stopwatch downloadTime = Stopwatch.StartNew(); - - /* If this is the root tree for a commit that was was just downloaded, assume that more - * trees will be needed soon and download them as well by using the download commit API. - * - * Otherwise, or as a fallback if the commit download fails, download the object directly. - */ - if (this.ShouldDownloadCommitPack(objectSha, out string commitSha) - && this.gitObjects.TryDownloadCommit(commitSha)) - { - this.DownloadedCommitPack(commitSha); - response = new NamedPipeMessages.DownloadObject.Response(NamedPipeMessages.DownloadObject.SuccessResult); - // FUTURE: Should the stats be updated to reflect all the trees in the pack? - // FUTURE: Should we try to clean up duplicate trees or increase depth of the commit download? - } - else if (this.gitObjects.TryDownloadAndSaveObject(objectSha, GVFSGitObjects.RequestSource.NamedPipeMessage) == GitObjects.DownloadAndSaveObjectResult.Success) + try { - this.UpdateTreesForDownloadedCommits(objectSha); - response = new NamedPipeMessages.DownloadObject.Response(NamedPipeMessages.DownloadObject.SuccessResult); + response = this.DownloadObject(objectSha); } - else + catch (Exception e) when (e is not OutOfMemoryException) { + EventMetadata metadata = new EventMetadata(); + metadata.Add("Area", "Mount"); + metadata.Add("objectSha", objectSha); + metadata.Add("Exception", e.ToString()); + this.tracer.RelatedWarning(metadata, nameof(this.HandleDownloadObjectRequest) + ": Exception downloading object"); + response = new NamedPipeMessages.DownloadObject.Response(NamedPipeMessages.DownloadObject.DownloadFailed); } + } + } + connection.TrySendResponse(response.CreateMessage()); + } - Native.ObjectTypes? objectType; - this.context.Repository.TryGetObjectType(objectSha, out objectType); - this.context.Repository.GVFSLock.Stats.RecordObjectDownload(objectType == Native.ObjectTypes.Blob, downloadTime.ElapsedMilliseconds); + private NamedPipeMessages.DownloadObject.Response DownloadObject(string objectSha) + { + NamedPipeMessages.DownloadObject.Response response; + Stopwatch downloadTime = Stopwatch.StartNew(); - if (objectType == Native.ObjectTypes.Commit - && !this.context.Repository.CommitAndRootTreeExists(objectSha, out var treeSha) - && !string.IsNullOrEmpty(treeSha)) - { - /* If a commit is downloaded, it wasn't prefetched. - * The trees for the commit may be needed soon depending on the context. - * e.g. git log (without a pathspec) doesn't need trees, but git checkout does. - * - * If any prefetch has been done there is probably a similar commit/tree in the graph, - * but in case there isn't (such as if the cache server repack maintenance job is failing) - * we should still try to avoid downloading an excessive number of loose trees for a commit. - * - * Save the tree/commit so if more trees are requested we can download all the trees for the commit in a batch. - */ - this.missingTreeTracker.AddMissingRootTree(treeSha: treeSha, commitSha: objectSha); - } - } + /* If this is the root tree for a commit that was was just downloaded, assume that more + * trees will be needed soon and download them as well by using the download commit API. + * + * Otherwise, or as a fallback if the commit download fails, download the object directly. + */ + if (this.ShouldDownloadCommitPack(objectSha, out string commitSha) + && this.gitObjects.TryDownloadCommit(commitSha)) + { + this.DownloadedCommitPack(commitSha); + response = new NamedPipeMessages.DownloadObject.Response(NamedPipeMessages.DownloadObject.SuccessResult); + // FUTURE: Should the stats be updated to reflect all the trees in the pack? + // FUTURE: Should we try to clean up duplicate trees or increase depth of the commit download? + } + else if (this.gitObjects.TryDownloadAndSaveObject(objectSha, GVFSGitObjects.RequestSource.NamedPipeMessage) == GitObjects.DownloadAndSaveObjectResult.Success) + { + this.UpdateTreesForDownloadedCommits(objectSha); + response = new NamedPipeMessages.DownloadObject.Response(NamedPipeMessages.DownloadObject.SuccessResult); + } + else + { + response = new NamedPipeMessages.DownloadObject.Response(NamedPipeMessages.DownloadObject.DownloadFailed); } - connection.TrySendResponse(response.CreateMessage()); + Native.ObjectTypes? objectType; + this.context.Repository.TryGetObjectType(objectSha, out objectType); + this.context.Repository.GVFSLock.Stats.RecordObjectDownload(objectType == Native.ObjectTypes.Blob, downloadTime.ElapsedMilliseconds); + + if (objectType == Native.ObjectTypes.Commit + && !this.context.Repository.CommitAndRootTreeExists(objectSha, out var treeSha) + && !string.IsNullOrEmpty(treeSha)) + { + /* If a commit is downloaded, it wasn't prefetched. + * The trees for the commit may be needed soon depending on the context. + * e.g. git log (without a pathspec) doesn't need trees, but git checkout does. + * + * If any prefetch has been done there is probably a similar commit/tree in the graph, + * but in case there isn't (such as if the cache server repack maintenance job is failing) + * we should still try to avoid downloading an excessive number of loose trees for a commit. + * + * Save the tree/commit so if more trees are requested we can download all the trees for the commit in a batch. + */ + this.missingTreeTracker.AddMissingRootTree(treeSha: treeSha, commitSha: objectSha); + } + + return response; } private bool ShouldDownloadCommitPack(string objectSha, out string commitSha) From ab1ab7343cf963a3a8aef97476f98fb6b72e7e32 Mon Sep 17 00:00:00 2001 From: Tyrie Vella Date: Fri, 12 Jun 2026 12:25:51 -0700 Subject: [PATCH 2/2] FunctionalTests: fail fast when shared enlistment mount is dead When GitRepoTests uses a shared enlistment (enlistmentPerTest=false), check IsMounted() in SetupForTest before running git commands. If the mount process crashed during a previous test, all remaining tests in the fixture would fail with the same unhelpful 'does not appear to be mounted' error from the pre-command hook. The early check produces a clear message pointing to the earlier root-cause failure. Assisted-by: Claude Opus 4.6 Signed-off-by: Tyrie Vella --- .../GVFS.FunctionalTests/Tests/GitCommands/GitRepoTests.cs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/GVFS/GVFS.FunctionalTests/Tests/GitCommands/GitRepoTests.cs b/GVFS/GVFS.FunctionalTests/Tests/GitCommands/GitRepoTests.cs index 64aa7a668..626cfaeba 100644 --- a/GVFS/GVFS.FunctionalTests/Tests/GitCommands/GitRepoTests.cs +++ b/GVFS/GVFS.FunctionalTests/Tests/GitCommands/GitRepoTests.cs @@ -137,6 +137,13 @@ public virtual void SetupForTest() { this.CreateEnlistment(); } + else if (!this.Enlistment.IsMounted()) + { + Assert.Fail( + "GVFS mount is not running for the shared enlistment. " + + "A previous test likely caused the mount process to crash. " + + "Check earlier test failures for the root cause."); + } if (this.validateWorkingTree == Settings.ValidateWorkingTreeMode.SparseMode) {