Repository navigation
Report an actionable error when the PrjFlt driver service is disabled - #2122
Tyrie Vella (tyrielv) wants to merge 1 commit into
Conversation
Nathan Baird (ShiningMassXAcc)
left a comment
There was a problem hiding this comment.
Review pass over this PR. 5 comment(s).
VFS for Git requires the PrjFlt ProjFS minifilter driver on every volume. Its load behavior is governed by its service Start type. When a security baseline, an anti-virus product, Group Policy, or manual troubleshooting disables the service, the driver cannot start. Today ProjFSFilter.TryStartService calls ServiceController.Start() on the disabled service. That fails with ERROR_SERVICE_DISABLED (1058), which is caught by a generic Win32Exception handler and surfaces to the user as the dead-end message "Failed to start prjflt service" with no indication that the driver is disabled and no way to fix it. Nothing else in the product detects this state (the existing AutoLogger Start handling is an unrelated ETW logging key). Detect the disabled state and report it, rather than silently re-enabling a driver that an administrator or security tool may have disabled on purpose. Changes: - TryStartService checks the service start type only when the service is not already running, so an already-running service succeeds regardless of its start type. When the service is not running and its start type is Disabled, it returns a specific error naming the driver and the exact elevated command to re-enable it. It also maps ERROR_SERVICE_DISABLED from a failed start as a fallback, and now reports its error through an out parameter so the caller surfaces the specific message. - Added pure TryGetDisabledStartTypeError and GetStartServiceFailureError helpers so the disabled-driver messaging is unit-tested without a live service. - EnableAndAttachProjFSHandler.TryEnablePrjFlt propagates that message instead of the generic "Failed to start prjflt service", and records the start-failure reason in the TryEnablePrjFlt_Summary health telemetry so a disabled driver is detectable in aggregate. - Added unit tests covering the disabled and enabled start types and the disabled and generic start-failure error codes. Assisted-by: Claude Opus 4.8 Signed-off-by: Tyrie Vella <tyrielv@gmail.com>
d0efb6b to
3187f0f
Compare
Nathan Baird (ShiningMassXAcc)
left a comment
There was a problem hiding this comment.
Re-review of the amended commit. All five of my earlier threads are genuinely addressed — thank you. Two new findings below; the first one I think inverts the PR's premise, so flagging it before this lands.
CI is green across every leg.
| } | ||
| catch (InvalidOperationException e) | ||
| { | ||
| error = $"{ServiceName} service was not found."; |
There was a problem hiding this comment.
High — I think this fallback is unreachable, and that the net effect is a regression on exactly the path the PR exists to improve.
ServiceController.Start() does not surface a bare Win32Exception when StartService fails — .NET catches the native failure and rethrows it as InvalidOperationException with the Win32Exception as InnerException. So control always lands in this handler, never in the catch (Win32Exception) below, and GetStartServiceFailureError(ERROR_SERVICE_DISABLED) never runs.
The consequence is that every start failure — access denied, driver load failure, and the disable-after-check race this PR is meant to catch — now reports PrjFlt service was not found. That is more confidently wrong than the generic message it replaced.
Could this handler unwrap instead, e.g. map e.InnerException as Win32Exception through GetStartServiceFailureError, and reserve the "not found" wording for ERROR_SERVICE_DOES_NOT_EXIST (1060)? A unit test that drives the InvalidOperationException-wrapping shape would pin it.
| { | ||
| error = "Failed to start prjflt service"; | ||
| error = string.IsNullOrEmpty(startServiceError) ? "Failed to start prjflt service" : startServiceError; | ||
| prjFltHealthMetadata.Add("StartServiceError", error); |
There was a problem hiding this comment.
Medium — This records the full three-sentence user-facing prose as a telemetry value, which means any future rewording of the message silently rebuckets the metric and breaks continuity with historical data. The key is also a magic string where the neighbouring metadata adds use nameof(...).
Would a short stable reason code work better here — ServiceDisabled / ServiceNotFound / StartFailed — with the prose kept for the user-facing error only?
Problem and Context
VFS for Git requires the
PrjFltProjFS minifilter driver on every volume. The driver's load behavior is governed by its service Start type. A security-hardening baseline, an anti-virus/EDR product, Group Policy, or manual troubleshooting can set the service to Disabled. When that happens, VFS for Git cannot start the driver.Today
ProjFSFilter.TryStartServicecallsServiceController.Start()on the disabled service. The call fails withERROR_SERVICE_DISABLED(1058), which is caught by a genericWin32Exceptionhandler.EnableAndAttachProjFSHandler.TryEnablePrjFltthen reports the dead-end message "Failed to start prjflt service" — with no indication that the driver is disabled and no hint at the fix. No other code path detects or reports this state. (The existing AutoLoggerStarthandling is an unrelated ETW logging key, not the driver service.)This change detects the disabled state and reports it with an actionable message. It does not re-enable the driver: a disabled driver is often disabled deliberately by an administrator or a security tool, so silently re-enabling it would change the machine's security posture without consent.
Changes
ProjFSFilter.TryStartServicenow checksServiceController.StartType. When the service isDisabled, it returns a specific error that names the driver and gives the exact elevated command to re-enable it (sc.exe config prjflt start= auto). It also mapsERROR_SERVICE_DISABLEDfrom a failedStart()as a fallback for the disable-after-check race.TryStartServicenow reports its failure through anout string errorparameter instead of returning only a bool, so the caller can surface the specific message.EnableAndAttachProjFSHandler.TryEnablePrjFltpropagates that message instead of the generic "Failed to start prjflt service", and records the start-failure reason in theTryEnablePrjFlt_Summaryhealth telemetry so a disabled driver is detectable in aggregate.internal GetStartServiceFailureError(int nativeErrorCode)seam and unit tests inProjFSFilterTestscovering the disabled error code and other start-failure codes.