Merge pull request #1961 from tyrielv/tyrielv/streamlined-upgrade-fixes

Fix staged upgrade races and installer error handling

Author: tyrielv

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

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);
+                }
             }
         }
     }