From 58a00e8e45e9e05e575fec559c02c038413a5b40 Mon Sep 17 00:00:00 2001 From: Tyrie Vella Date: Thu, 7 May 2026 14:00:23 -0700 Subject: [PATCH] Fix staged upgrade races and installer error handling Address review feedback from #1958: - StopService/StagingUpdateService: check sc.exe exit code separately from Exec launch failure. Previously non-zero sc.exe results were silently ignored. - Timer race: wrap pendingUpgradeTimer create/reset in a lock. Two concurrent unmounts on separate pipe threads could both observe null and create duplicate timers, causing parallel TryApplyPendingUpgrade. - .ready race: move service start from AfterInstall hook to ssPostInstall after .ready marker is written. Previously the service could start its 5s debounce timer before .ready existed, skip the upgrade, and leave staged files until the next service restart. Assisted-by: Claude Opus 4.6 Signed-off-by: Tyrie Vella --- GVFS/GVFS.Installers/Setup.iss | 26 +++++++++++++--- GVFS/GVFS.Service/Handlers/RequestHandler.cs | 32 +++++++++++--------- 2 files changed, 39 insertions(+), 19 deletions(-) diff --git a/GVFS/GVFS.Installers/Setup.iss b/GVFS/GVFS.Installers/Setup.iss index 8ba43a32d..10765dddb 100644 --- a/GVFS/GVFS.Installers/Setup.iss +++ b/GVFS/GVFS.Installers/Setup.iss @@ -66,7 +66,7 @@ DestDir: "{app}"; Flags: ignoreversion; Source:"{#LayoutDir}\GVFS.Service.exe"; ; goes directly to {app} so the restarted service has PendingUpgradeHandler code. ; The service is briefly stopped/restarted (mounts are independent processes). DestDir: "{app}\PendingUpgrade"; Flags: ignoreversion recursesubdirs; Source:"{#LayoutDir}\*"; Check: IsStagingInstall -DestDir: "{app}"; Flags: ignoreversion; Source:"{#LayoutDir}\GVFS.Service.exe"; AfterInstall: StagingUpdateService; Check: IsStagingInstall +DestDir: "{app}"; Flags: ignoreversion; Source:"{#LayoutDir}\GVFS.Service.exe"; Check: IsStagingInstall [Dirs] Name: "{app}\ProgramData\{#ServiceName}"; Permissions: users-readexec @@ -166,11 +166,17 @@ var ResultCode: integer; begin Log('StopService: stopping: ' + ServiceName); - // ErrorCode 1060 means service not installed, 1062 means service not started - if not Exec(ExpandConstant('{sys}\SC.EXE'), 'stop ' + ServiceName, '', SW_HIDE, ewWaitUntilTerminated, ResultCode) and (ResultCode <> 1060) and (ResultCode <> 1062) then + if not Exec(ExpandConstant('{sys}\SC.EXE'), 'stop ' + ServiceName, '', SW_HIDE, ewWaitUntilTerminated, ResultCode) then begin + Log('StopService: Failed to launch sc.exe'); RaiseException('Fatal: Could not stop service: ' + ServiceName); end; + // 1060 = service not installed, 1062 = service not started + if (ResultCode <> 0) and (ResultCode <> 1060) and (ResultCode <> 1062) then + begin + Log('StopService: sc stop returned error code ' + IntToStr(ResultCode)); + RaiseException('Fatal: Could not stop service: ' + ServiceName + ' (exit code ' + IntToStr(ResultCode) + ')'); + end; end; procedure WaitForServiceProcessToExit(ServiceName: string); @@ -328,9 +334,14 @@ begin try Log('StagingUpdateService: Starting service with new binary'); - if not Exec(ExpandConstant('{sys}\SC.EXE'), 'start GVFS.Service', '', SW_HIDE, ewWaitUntilTerminated, ResultCode) then + if Exec(ExpandConstant('{sys}\SC.EXE'), 'start GVFS.Service', '', SW_HIDE, ewWaitUntilTerminated, ResultCode) then + begin + if ResultCode <> 0 then + Log('StagingUpdateService: Warning - sc start returned error code ' + IntToStr(ResultCode)); + end + else begin - Log('StagingUpdateService: Warning - could not start service: ' + SysErrorMessage(ResultCode)); + Log('StagingUpdateService: Warning - could not launch sc.exe'); end; WriteOnDiskVersion16CapableFile(); @@ -721,6 +732,11 @@ begin // complete and safe to apply. SaveStringToFile(ExpandConstant('{app}\PendingUpgrade\.ready'), '', False); Log('CurStepChanged: Wrote PendingUpgrade .ready marker'); + + // Start the service AFTER .ready is written. Previously this + // was an AfterInstall hook on GVFS.Service.exe, but that races: + // the service's debounce timer could fire before .ready exists. + StagingUpdateService(); end; MigrateConfigAndStatusCacheFiles(); if (not KeepMountsRunning) and (ExpandConstant('{param:REMOUNTREPOS|true}') = 'true') then diff --git a/GVFS/GVFS.Service/Handlers/RequestHandler.cs b/GVFS/GVFS.Service/Handlers/RequestHandler.cs index 85f27c4f3..4d665c416 100644 --- a/GVFS/GVFS.Service/Handlers/RequestHandler.cs +++ b/GVFS/GVFS.Service/Handlers/RequestHandler.cs @@ -29,6 +29,7 @@ public class RequestHandler private ITracer tracer; private IRepoRegistry repoRegistry; private Timer pendingUpgradeTimer; + private readonly object pendingUpgradeTimerLock = new object(); public RequestHandler(ITracer tracer, string etwArea, IRepoRegistry repoRegistry) { @@ -146,21 +147,24 @@ private void TryDeferredPendingUpgradeCheck(ITracer tracer) // Debounce: reset the timer on each unmount so the check fires // once after the last unmount settles. If multiple repos unmount // in quick succession, only one upgrade attempt runs. - if (this.pendingUpgradeTimer == null) + lock (this.pendingUpgradeTimerLock) { - this.pendingUpgradeTimer = new Timer( - _ => - { - tracer.RelatedInfo("TryDeferredPendingUpgradeCheck: Checking pending upgrade after unmount"); - PendingUpgradeHandler.TryApplyPendingUpgrade(tracer); - }, - null, - PendingUpgradeDelayMs, - Timeout.Infinite); - } - else - { - this.pendingUpgradeTimer.Change(PendingUpgradeDelayMs, Timeout.Infinite); + if (this.pendingUpgradeTimer == null) + { + this.pendingUpgradeTimer = new Timer( + _ => + { + tracer.RelatedInfo("TryDeferredPendingUpgradeCheck: Checking pending upgrade after unmount"); + PendingUpgradeHandler.TryApplyPendingUpgrade(tracer); + }, + null, + PendingUpgradeDelayMs, + Timeout.Infinite); + } + else + { + this.pendingUpgradeTimer.Change(PendingUpgradeDelayMs, Timeout.Infinite); + } } } }