Use shutdown-first detached reboot wrappers

* Use shutdown-first detached reboot wrappers

* Share detached reboot wrapper

* Simplify shutdown reboot wrapper

* Address reboot wrapper review comments

* Share reboot progress message

* Remove redundant shutdown reboot wrapper

Author: jamesyc

src/timecapsulesmb/cli/flows.py

@@ -7,7 +7,7 @@
 from timecapsulesmb.cli.runtime import LogCallback, emit_progress
 from timecapsulesmb.core.net import extract_host
 from timecapsulesmb.core.errors import system_exit_message
-from timecapsulesmb.deploy.executor import remote_request_reboot, remote_request_shutdown_reboot
+from timecapsulesmb.deploy.executor import remote_request_reboot
 from timecapsulesmb.deploy.verify import (
     managed_runtime_ready,
     render_managed_runtime_verification,
@@ -26,6 +26,7 @@
 
 REBOOT_UP_TIMEOUT_MESSAGE = "Timed out waiting for SSH after reboot."
 ACP_REBOOT_REQUEST_TIMEOUT_SECONDS = 10
+SSH_SHUTDOWN_REBOOT_PROGRESS_MESSAGE = "SSH: /bin/sync; /sbin/shutdown -r now (fallback /sbin/reboot)"
 
 
 def wait_for_tcp_port_state(
@@ -167,8 +168,8 @@ def _request_reboot_via_ssh_shutdown(
         connection,
         command_context,
         log=log,
-        request_reboot=remote_request_shutdown_reboot,
-        progress_message="SSH: /sbin/shutdown -r now (fallback /sbin/reboot)",
+        request_reboot=remote_request_reboot,
+        progress_message=SSH_SHUTDOWN_REBOOT_PROGRESS_MESSAGE,
     )
 
 
@@ -178,7 +179,7 @@ def _request_reboot_via_ssh(
     *,
     log: LogCallback = None,
     request_reboot: Callable[[SshConnection], None] | None = None,
-    progress_message: str = "SSH: /sbin/reboot",
+    progress_message: str = SSH_SHUTDOWN_REBOOT_PROGRESS_MESSAGE,
 ) -> None:
     command_context.add_debug_fields(ssh_reboot_attempted=True)
     emit_progress(log, progress_message)

src/timecapsulesmb/cli/fsck.py

@@ -8,6 +8,7 @@
 from timecapsulesmb.cli.context import CommandContext
 from timecapsulesmb.cli.flows import observe_reboot_cycle
 from timecapsulesmb.cli.runtime import add_config_argument, load_env_config
+from timecapsulesmb.deploy.executor import DETACHED_SHUTDOWN_REBOOT_COMMAND
 from timecapsulesmb.deploy.planner import DEFAULT_APPLE_MOUNT_WAIT_SECONDS
 from timecapsulesmb.device.processes import render_direct_pkill9_by_ucomm, render_direct_pkill9_watchdog
 from timecapsulesmb.identity import ensure_install_id
@@ -89,7 +90,7 @@ def build_remote_fsck_script(device: str, mountpoint: str, *, reboot: bool) -> s
         lines.extend(
             [
                 "echo '--- reboot ---'",
-                "/sbin/reboot >/dev/null 2>&1 || true",
+                DETACHED_SHUTDOWN_REBOOT_COMMAND,
             ]
         )
     return "\n".join(lines)

src/timecapsulesmb/deploy/executor.py

@@ -10,11 +10,10 @@
 from timecapsulesmb.transport.ssh import SshConnection, run_scp, run_ssh
 
 
-DETACHED_REBOOT_COMMAND = "/bin/sh -c 'exec </dev/null >/dev/null 2>&1; (/bin/sleep 1; /sbin/reboot) & exit 0'"
 DETACHED_SHUTDOWN_REBOOT_COMMAND = (
     "/bin/sh -c 'exec </dev/null >/dev/null 2>&1; "
-    "(/bin/sleep 1; "
-    "if [ -x /sbin/shutdown ]; then /sbin/shutdown -r now || /sbin/reboot; else /sbin/reboot; fi"
+    "(/bin/sync; /bin/sleep 1; "
+    "/sbin/shutdown -r now || /sbin/reboot"
     ") & exit 0'"
 )
 REBOOT_REQUEST_TIMEOUT_SECONDS = 30
@@ -120,10 +119,6 @@ def run_remote_actions(connection: SshConnection, actions: Iterable[RemoteAction
 
 
 def remote_request_reboot(connection: SshConnection) -> None:
-    run_ssh(connection, DETACHED_REBOOT_COMMAND, check=False, timeout=REBOOT_REQUEST_TIMEOUT_SECONDS)
-
-
-def remote_request_shutdown_reboot(connection: SshConnection) -> None:
     run_ssh(connection, DETACHED_SHUTDOWN_REBOOT_COMMAND, check=False, timeout=REBOOT_REQUEST_TIMEOUT_SECONDS)
 
 

tests/test_cli.py

@@ -902,14 +902,8 @@ def run_deploy_cli(
                 mocks.verify_managed_runtime = stack.enter_context(
                     mock.patch("timecapsulesmb.cli.flows.verify_managed_runtime", return_value=verify_runtime)
                 )
-            mocks.remote_request_shutdown_reboot = stack.enter_context(
-                mock.patch("timecapsulesmb.cli.flows.remote_request_shutdown_reboot", side_effect=reboot_side_effect)
-            )
             mocks.remote_request_reboot = stack.enter_context(
-                mock.patch(
-                    "timecapsulesmb.cli.flows.remote_request_reboot",
-                    side_effect=AssertionError("deploy should not request legacy SSH reboot directly"),
-                )
+                mock.patch("timecapsulesmb.cli.flows.remote_request_reboot", side_effect=reboot_side_effect)
             )
             mocks.acp_reboot = stack.enter_context(
                 mock.patch(
@@ -4529,7 +4523,6 @@ def test_deploy_no_reboot_stops_after_upload_phase(self) -> None:
         )
 
         self.assertEqual(result.rc, 0)
-        result.mocks.remote_request_shutdown_reboot.assert_not_called()
         result.mocks.remote_request_reboot.assert_not_called()
         self.assertEqual(result.mocks.verify_payload_home_conn.call_count, 2)
         result.mocks.flush_remote_filesystem_writes.assert_called_once()
@@ -4546,7 +4539,6 @@ def test_deploy_payload_verification_failure_aborts_before_reboot(self) -> None:
         )
 
         self.assertEqual(str(result.exception), "managed payload verification failed at /Volumes/dk2/.samba4: missing smbd")
-        result.mocks.remote_request_shutdown_reboot.assert_not_called()
         result.mocks.remote_request_reboot.assert_not_called()
         result.mocks.verify_payload_home_conn.assert_called_once()
         result.mocks.flush_remote_filesystem_writes.assert_not_called()
@@ -4572,7 +4564,6 @@ def test_deploy_post_sync_payload_verification_failure_aborts_before_reboot(self
             "managed payload verification failed at /Volumes/dk2/.samba4: missing payload directory",
         )
         result.mocks.flush_remote_filesystem_writes.assert_called_once()
-        result.mocks.remote_request_shutdown_reboot.assert_not_called()
         result.mocks.remote_request_reboot.assert_not_called()
         self.assertEqual(result.mocks.verify_payload_home_conn.call_count, 2)
         telemetry_error = self.telemetry_payload("deploy_finished")["error"]
@@ -4591,7 +4582,6 @@ def test_deploy_declined_reboot_returns_without_rebooting(self) -> None:
 
         self.assertEqual(result.rc, 0)
         self.assertIn("Deployment complete without reboot.", result.text)
-        result.mocks.remote_request_shutdown_reboot.assert_not_called()
         result.mocks.remote_request_reboot.assert_not_called()
         self.assertEqual(result.mocks.verify_payload_home_conn.call_count, 2)
         result.mocks.flush_remote_filesystem_writes.assert_called_once()
@@ -4610,8 +4600,7 @@ def test_deploy_reboot_timeout_returns_failure(self) -> None:
         self.assertEqual(result.rc, 1)
         self.assertIn("SSH reboot request timed out; checking whether the device is rebooting...", result.text)
         self.assertIn(deploy.REBOOT_NO_DOWN_MESSAGE, result.text)
-        result.mocks.remote_request_shutdown_reboot.assert_called_once()
-        result.mocks.remote_request_reboot.assert_not_called()
+        result.mocks.remote_request_reboot.assert_called_once()
         result.mocks.acp_reboot.assert_not_called()
         result.mocks.verify_managed_runtime.assert_not_called()
 
@@ -4673,7 +4662,6 @@ def test_deploy_netbsd4_yes_runs_activation_and_skips_reboot(self) -> None:
         self.assertEqual(result.mocks.run_remote_actions.call_count, 3)
         self.assertEqual(result.mocks.verify_payload_home_conn.call_count, 2)
         result.mocks.flush_remote_filesystem_writes.assert_called_once()
-        result.mocks.remote_request_shutdown_reboot.assert_not_called()
         result.mocks.remote_request_reboot.assert_not_called()
         self.assertIn("Activating NetBSD4 payload without reboot.", result.text)
         self.assertIn("NetBSD4 activation complete.", result.text)
@@ -7324,7 +7312,11 @@ def test_fsck_yes_reboots_and_waits_by_default(self) -> None:
         self.assertIn("^wcifsfs$", remote_cmd)
         self.assertIn("umount -f /Volumes/dk2", remote_cmd)
         self.assertIn("fsck_hfs -fy /dev/dk2", remote_cmd)
+        self.assertIn("exec </dev/null >/dev/null 2>&1", remote_cmd)
+        self.assertIn("/bin/sync; /bin/sleep 1;", remote_cmd)
+        self.assertIn("/sbin/shutdown -r now", remote_cmd)
         self.assertIn("/sbin/reboot", remote_cmd)
+        self.assertIn(") & exit 0", remote_cmd)
         self.assertEqual(wait_mock.call_args_list[0].kwargs, {"expected_up": False, "timeout_seconds": 90})
         self.assertEqual(wait_mock.call_args_list[1].kwargs, {"expected_up": True, "timeout_seconds": 420})
         text = output.getvalue()

tests/test_cli_flows.py

@@ -16,6 +16,7 @@
 from timecapsulesmb.cli.flows import (
     ACP_REBOOT_REQUEST_TIMEOUT_SECONDS,
     REBOOT_UP_TIMEOUT_MESSAGE,
+    SSH_SHUTDOWN_REBOOT_PROGRESS_MESSAGE,
     observe_reboot_cycle,
     request_deploy_reboot_and_wait,
     request_reboot_and_wait,
@@ -152,10 +153,10 @@ def test_request_reboot_and_wait_succeeds_after_acp_reboot_request(self) -> None
         self.assertIn("ACP reboot requested.", output.getvalue())
         self.assertIn("Waiting for the device to go down...", output.getvalue())
 
-    def test_request_deploy_reboot_and_wait_uses_ssh_shutdown_request(self) -> None:
+    def test_request_deploy_reboot_and_wait_uses_ssh_reboot_request(self) -> None:
         command_context = FakeCommandContext()
         output = io.StringIO()
-        with mock.patch("timecapsulesmb.cli.flows.remote_request_shutdown_reboot") as shutdown_reboot_mock:
+        with mock.patch("timecapsulesmb.cli.flows.remote_request_reboot") as reboot_mock:
             with mock.patch("timecapsulesmb.cli.flows.acp_reboot", side_effect=AssertionError("deploy should not use ACP reboot")):
                 with mock.patch("timecapsulesmb.cli.flows.wait_for_ssh_state_conn", side_effect=[True, True]) as wait_mock:
                     with redirect_stdout(output):
@@ -166,7 +167,7 @@ def test_request_deploy_reboot_and_wait_uses_ssh_shutdown_request(self) -> None:
                         )
 
         self.assertTrue(ok)
-        shutdown_reboot_mock.assert_called_once()
+        reboot_mock.assert_called_once()
         self.assertEqual(wait_mock.call_args_list[0].kwargs, {"expected_up": False, "timeout_seconds": 60})
         self.assertEqual(wait_mock.call_args_list[1].kwargs, {"expected_up": True, "timeout_seconds": 240})
         self.assertEqual(command_context.finish_fields["reboot_was_attempted"], True)
@@ -268,7 +269,7 @@ def test_request_ssh_reboot_uses_ssh_only_strategy_and_progress_log(self) -> Non
         self.assertEqual(command_context.debug_fields["reboot_request_strategy"], "ssh")
         self.assertEqual(command_context.debug_fields["ssh_reboot_attempted"], True)
         self.assertEqual(command_context.debug_fields["ssh_reboot_succeeded"], True)
-        self.assertEqual(messages, ["SSH: /sbin/reboot"])
+        self.assertEqual(messages, [SSH_SHUTDOWN_REBOOT_PROGRESS_MESSAGE])
         self.assertIn("SSH reboot requested.", output.getvalue())
 
     def test_request_ssh_reboot_records_timeout_without_raising(self) -> None:

tests/test_deploy_modules.py

@@ -40,14 +40,12 @@
 )
 from timecapsulesmb.deploy.dry_run import format_deployment_plan
 from timecapsulesmb.deploy.executor import (
-    DETACHED_REBOOT_COMMAND,
     DETACHED_SHUTDOWN_REBOOT_COMMAND,
     FLUSH_REMOTE_FILESYSTEMS_COMMAND,
     FLUSH_REMOTE_FILESYSTEMS_TIMEOUT_SECONDS,
     REBOOT_REQUEST_TIMEOUT_SECONDS,
     flush_remote_filesystem_writes,
     remote_request_reboot,
-    remote_request_shutdown_reboot,
     remote_uninstall_payload,
     upload_deployment_payload,
     upload_flash_file,
@@ -297,25 +295,18 @@ def test_remote_request_reboot_uses_explicit_reboot_timeout(self) -> None:
         connection = SshConnection("root@10.0.0.2", "pw", "-o foo")
         with mock.patch("timecapsulesmb.deploy.executor.run_ssh") as run_ssh_mock:
             remote_request_reboot(connection)
-        run_ssh_mock.assert_called_once_with(
-            connection,
-            DETACHED_REBOOT_COMMAND,
-            check=False,
-            timeout=REBOOT_REQUEST_TIMEOUT_SECONDS,
-        )
-
-    def test_remote_request_shutdown_reboot_uses_shutdown_with_reboot_fallback(self) -> None:
-        connection = SshConnection("root@10.0.0.2", "pw", "-o foo")
-        with mock.patch("timecapsulesmb.deploy.executor.run_ssh") as run_ssh_mock:
-            remote_request_shutdown_reboot(connection)
         run_ssh_mock.assert_called_once_with(
             connection,
             DETACHED_SHUTDOWN_REBOOT_COMMAND,
             check=False,
             timeout=REBOOT_REQUEST_TIMEOUT_SECONDS,
         )
+        self.assertIn("exec </dev/null >/dev/null 2>&1", DETACHED_SHUTDOWN_REBOOT_COMMAND)
+        self.assertIn("/bin/sync; /bin/sleep 1;", DETACHED_SHUTDOWN_REBOOT_COMMAND)
         self.assertIn("/sbin/shutdown -r now", DETACHED_SHUTDOWN_REBOOT_COMMAND)
         self.assertIn("|| /sbin/reboot", DETACHED_SHUTDOWN_REBOOT_COMMAND)
+        self.assertNotIn("[ -x /sbin/shutdown ]", DETACHED_SHUTDOWN_REBOOT_COMMAND)
+        self.assertIn(") & exit 0", DETACHED_SHUTDOWN_REBOOT_COMMAND)
 
     def test_flush_remote_filesystem_writes_syncs_and_waits(self) -> None:
         connection = SshConnection("root@10.0.0.2", "pw", "-o foo")