Reset error state before calling Process32Next (#7328)

* Convenience script to install daemon

* Use -p when re-launching with runas to keep window open

* Align new script with PEP 8

* Add more args for flexibility

* Add task to reinstall Windows daemon

* Fixed mistake in continue message

* Close task window after running script

* Reset error state before calling Process32Next

* Update CL

* Increase Windows CI timeout

* Bump timeout again due to choco

.github/workflows/job-test-windows.yml

@@ -13,7 +13,7 @@ concurrency:
 jobs:
   test-windows:
     runs-on: windows-latest
-    timeout-minutes: 10
+    timeout-minutes: 20
 
     env:
       GIT_COMMIT: ${{ github.sha }}

.vscode/tasks.json

@@ -8,6 +8,12 @@
       "targets": ["all"],
       "preset": "${command:cmake.activeBuildPresetName}",
       "group": "build"
+    },
+    {
+      "label": "reinstall windows daemon",
+      "type": "shell",
+      "command": "python scripts/windows_daemon.py",
+      "presentation": { "close": true }
     }
   ]
 }

ChangeLog

@@ -26,6 +26,7 @@ Tasks:
 - #7325 Add timeout to all GitHub workflows
 - #7326 Restore lpDesktop assignment in Windows daemon
 - #7327 Only use Ninja to build on Windows
+- #7328 Reset error state before calling Process32Next
 
 # 1.14.6
 

scripts/windows_daemon.py

@@ -0,0 +1,93 @@
+import os
+import subprocess
+import ctypes
+import sys
+import argparse
+import glob
+
+BIN_NAME = 'synergyd'
+SOURCE_BIN_DIR = os.path.join('build', 'bin')
+TARGET_BIN_DIR = 'bin'
+SERVICE_NOT_RUNNING_ERROR = 2
+
+def main():
+  """Entry point for the script."""
+
+  parser = argparse.ArgumentParser()
+  parser.add_argument('--pause-on-exit', action='store_true')
+  parser.add_argument('--source-bin-dir', default=SOURCE_BIN_DIR)
+  parser.add_argument('--target-bin-dir', default=TARGET_BIN_DIR)
+  parser.add_argument('--source-bin-name', default=BIN_NAME)
+  parser.add_argument('--target-bin-name', default=BIN_NAME)
+  args = parser.parse_args()
+
+  if not is_admin():
+    print('Re-launching script as admin')
+    args = ' '.join(sys.argv[1:])
+    command = f'{__file__} --pause-on-exit {args}'
+    ctypes.windll.shell32.ShellExecuteW(None, 'runas', sys.executable, command, None, 1)
+    sys.exit()
+
+  try:
+    reinstall(args.source_bin_dir, args.target_bin_dir, args.source_bin_name, args.target_bin_name)
+  except Exception as e:
+    print(f'Error: {e}')
+  
+  if (args.pause_on_exit):
+    input('Press enter to continue...')
+
+def reinstall(source_bin_dir, target_bin_dir, source_bin_name, target_bin_name):
+  """Stops the running daemon service, copies files, and reinstalls."""
+  
+  print('Stopping daemon service')
+  try:
+    subprocess.run(['net', 'stop', 'synergy'], shell=True, check=True)
+  except subprocess.CalledProcessError as e:
+    if (e.returncode == SERVICE_NOT_RUNNING_ERROR):
+      print('Daemon service not running')
+    else:
+      raise e
+
+  copy_bin_files(source_bin_dir, target_bin_dir, source_bin_name, target_bin_name)
+
+  target_bin_file = f'{os.path.join(target_bin_dir, target_bin_name)}.exe'
+
+  print('Removing old daemon service')
+  subprocess.run([target_bin_file, '/uninstall'], shell=True, check=True)
+
+  print('Installing daemon service')
+  subprocess.run([target_bin_file, '/install'], shell=True, check=True)
+
+def copy_bin_files(source_bin_dir, target_bin_dir, source_bin_name, target_bin_name):
+
+  if not os.path.isdir(source_bin_dir):
+    raise Exception(f'Invalid source bin dir: {source_bin_dir}')
+
+  print(f'Persisting dir: {target_bin_dir}')
+  os.makedirs(target_bin_dir, exist_ok=True)
+
+  source_bin_glob = f'{source_bin_name}*'
+  source_files = glob.glob(os.path.join(source_bin_dir, source_bin_glob))
+
+  if not source_files:
+    raise Exception(f'No files found in {source_bin_dir} matching {source_bin_glob}')
+
+  for source_file in source_files:
+    base_name = os.path.basename(source_file)
+    base_name = base_name.replace(source_bin_name, target_bin_name)
+    target_file = os.path.join(target_bin_dir, base_name)
+    print(f'Copying {source_file} to {target_file}')
+  # use the copy command; shutil.copy gives us a permission denied error.
+    try:
+      subprocess.run(['copy', source_file, target_file], shell=True, check=True)
+    except subprocess.CalledProcessError as e:
+      print(f'Copy failed: {e}')
+
+def is_admin():
+    """Returns True if the current process has admin privileges."""
+    try:
+        return ctypes.windll.shell32.IsUserAnAdmin()
+    except ctypes.WinError:
+        return False
+
+main()

src/lib/platform/MSWindowsSession.cpp

@@ -158,13 +158,20 @@ MSWindowsSession::updateActiveSession()
 BOOL
 MSWindowsSession::nextProcessEntry(HANDLE snapshot, LPPROCESSENTRY32 entry)
 {
+    // TODO: issue S3-2021
+    // resetting the error state here is acceptable, but having to do so indicates that a
+    // different win32 function call has failed beforehand. we should always check for errors 
+    // after each win32 function call.
+    SetLastError(0);
+
     BOOL gotEntry = Process32Next(snapshot, entry);
     if (!gotEntry) {
 
         DWORD err = GetLastError();
-        if (err != ERROR_NO_MORE_FILES) {
 
-            // only worry about error if it's not the end of the snapshot
+        // only throw if it's not the end of the snapshot, if not the 'no more files' error
+        // then it's probably something serious.
+        if (err != ERROR_NO_MORE_FILES) {
             LOG((CLOG_ERR "could not get next process entry"));
             throw XArch(new XArchEvalWindows());
         }