Merge pull request 'fix(service): start the Windows service after granting it its config (D157)' (#31) from fix/windows-start-after-grant into edge

Reviewed-on: #31

src/install.rs

@@ -576,8 +576,9 @@ fn install_sidecar(
         layout.relocated,
     )?;
 
-    let outcome = service::register(&prepared, layout, action.writes())?;
+    let mut outcome = service::register(&prepared, layout, action.writes())?;
     service::grant_service_access(&config_path, &layout.data_dir, &outcome)?;
+    service::start_registered(&mut outcome, &layout.sidecar_bin, &config_path)?;
     let service_record = match &outcome {
         service::Outcome::Registered {
             kind,

src/rustgame/install.rs

@@ -624,7 +624,7 @@ fn deploy_instance(
         prepared.user.as_deref(),
         layout.relocated,
     )?;
-    let outcome = service::register_rust(
+    let mut outcome = service::register_rust(
         prepared,
         layout,
         &plan.id,
@@ -632,6 +632,7 @@ fn deploy_instance(
         binary_changed,
     )?;
     service::grant_service_access(&plan.config_path, &data_dir, &outcome)?;
+    service::start_registered(&mut outcome, &layout.rust_sidecar_bin, &plan.config_path)?;
     let service_record = match &outcome {
         service::Outcome::Registered {
             kind,

src/service.rs

@@ -399,22 +399,15 @@ pub fn windows_start_failure_for(
 
 #[cfg(windows)]
 fn register_windows(binary: &Path, config: &Path, restart: bool) -> Result<Outcome> {
-    register_windows_named(
-        WINDOWS_SERVICE,
-        DISPLAY_NAME,
-        MIN_SERVICE_SIDECAR,
-        binary,
-        config,
-        restart,
-    )
+    register_windows_named(WINDOWS_SERVICE, DISPLAY_NAME, binary, config, restart)
 }
 
-/// Registers (or reconfigures), sets the restart policy of, and starts one SCM service.
+/// Registers (or reconfigures) one SCM service and sets its restart policy. It does not start it:
+/// [`start_registered`] does, after [`grant_service_access`].
 #[cfg(windows)]
 fn register_windows_named(
     name: &str,
     display: &str,
-    min_sidecar: &str,
     binary: &Path,
     config: &Path,
     restart: bool,
@@ -473,17 +466,11 @@ fn register_windows_named(
     if restart && windows_service_state(name).contains("RUNNING") {
         stop_windows_service(name)?;
     }
-    // 1056 is ERROR_SERVICE_ALREADY_RUNNING, which is the desired end state, not a failure.
-    let start = run("sc.exe", &["start", name])?;
-    if !start.status.success() && start.status.code() != Some(1056) {
-        anyhow::bail!(windows_start_failure_for(
-            name,
-            min_sidecar,
-            start.status.code().unwrap_or(-1),
-            binary,
-            config
-        ));
-    }
+    // NOT started here: the virtual account `sc create` just made cannot read the config yet —
+    // `protect_config` locked it to SYSTEM and Administrators, and `grant_service_access` is what
+    // lets the account in. Started first, the sidecar died on `Access is denied` and the service
+    // sat STOPPED; the restart policy never fires for a clean exit with an error code. The phase 18
+    // walk found it (D157). The caller grants, then calls `start_registered`.
 
     Ok(Outcome::Registered {
         kind: "windows-scm",
@@ -659,7 +646,6 @@ pub fn register_rust(
         Manager::WindowsScm => register_windows_named(
             &rust_windows_service(server_id),
             &format!("Runic Gateway rust-link sidecar ({server_id})"),
-            MIN_RUST_SERVICE_SIDECAR,
             binary,
             config,
             restart,
@@ -1180,6 +1166,47 @@ pub fn protect_config(
     protect_config_platform(config, data_dir, user, relocated)
 }
 
+/// Starts a service that [`register`] or [`register_rust`] has just registered, once
+/// [`grant_service_access`] has let its account read the config.
+///
+/// A no-op on Linux, where registration starts the unit itself: the service user was known before
+/// the config existed, so `protect_config` had already handed the file to it. On Windows the order
+/// is the whole point — see `register_windows_named`. The outcome's `state` is refreshed so the
+/// run reports what the service is doing now, not what it was doing before it was started.
+pub fn start_registered(outcome: &mut Outcome, binary: &Path, config: &Path) -> Result<()> {
+    start_registered_platform(outcome, binary, config)
+}
+
+#[cfg(unix)]
+fn start_registered_platform(_outcome: &mut Outcome, _binary: &Path, _config: &Path) -> Result<()> {
+    Ok(())
+}
+
+#[cfg(windows)]
+fn start_registered_platform(outcome: &mut Outcome, binary: &Path, config: &Path) -> Result<()> {
+    let Outcome::Registered { name, state, .. } = outcome else {
+        return Ok(());
+    };
+    let min_sidecar = if name.as_str() == WINDOWS_SERVICE {
+        MIN_SERVICE_SIDECAR
+    } else {
+        MIN_RUST_SERVICE_SIDECAR
+    };
+    // 1056 is ERROR_SERVICE_ALREADY_RUNNING, which is the desired end state, not a failure.
+    let start = run("sc.exe", &["start", name])?;
+    if !start.status.success() && start.status.code() != Some(1056) {
+        anyhow::bail!(windows_start_failure_for(
+            name,
+            min_sidecar,
+            start.status.code().unwrap_or(-1),
+            binary,
+            config
+        ));
+    }
+    *state = format!("{}, automatic start", windows_service_state(name));
+    Ok(())
+}
+
 /// Grants the registered service account access to what it must read and write.
 ///
 /// A no-op on Linux, where the account was known before the config existed and `protect_config`