Merge pull request #22 from snegov/feature/deterministic-startup-shutdown

Bound daemon startup and shutdown and exit with a useful code

.github/workflows/docker-publish.yml

@@ -24,7 +24,7 @@ jobs:
     steps:
       - uses: actions/checkout@v7
 
-      - uses: actions/setup-python@v5
+      - uses: actions/setup-python@v7
         with:
           python-version: '3.11'
           cache: pip

IMPROVEMENTS.md

@@ -90,13 +90,14 @@ Acceptance criteria:
 
 ### Make startup and shutdown deterministic
 
-- [ ] Check every important subprocess return code.
-- [ ] Add timeouts to daemon startup, shutdown, and readiness polling.
-  Confirmed: `start_daemon` and `stop_daemon` poll `while True` with no bound,
-  so a daemon that never changes state hangs the container silently.
-- [ ] Handle `SIGTERM` and `SIGINT` and stop the daemon in `finally`.
-- [ ] Return a nonzero container exit code on configuration or daemon failure.
-- [ ] Produce actionable errors instead of waiting forever.
+- [x] Check every important subprocess return code. A failed `seaf-cli sync` is
+  reported per library and does not stop the remaining ones.
+- [x] Add timeouts to daemon startup, shutdown, and readiness polling. See
+  `DAEMON_START_TIMEOUT` and `DAEMON_STOP_TIMEOUT` in `dsc/const.py`; the stop
+  timeout is deliberately below the 10 second `docker stop` grace period.
+- [x] Handle `SIGTERM` and `SIGINT` and stop the daemon in `finally`.
+- [x] Return a nonzero container exit code on configuration or daemon failure.
+- [x] Produce actionable errors instead of waiting forever.
 
 Acceptance criteria:
 
@@ -113,7 +114,9 @@ Acceptance criteria:
 - [ ] Add regression tests for externally reported password and deletion
   threshold issues.
 - [ ] Add a container smoke test for imports, installed CLI version, startup,
-  and shutdown using controlled fakes.
+  and shutdown using controlled fakes. Graceful shutdown was verified by hand
+  in the built image (`docker stop` returns immediately with exit code 0
+  instead of being killed after 10 seconds); it is not yet automated in CI.
 - [x] Keep image construction as a separate CI job so packaging failures are
   distinguishable from application failures. The `unit-test` job does not
   depend on the image build.

README.md

@@ -66,6 +66,11 @@ The subdirectory is named after the library, with whitespace replaced by
  libraries directory (`/dsc/seafile`, or the deprecated `/data` when that
  exists); a library name can never place it elsewhere.
 
+The container stops gracefully on `docker stop`: it shuts the Seafile daemon
+ down and exits with code 0. It exits with a nonzero code and an explanatory
+ message when the daemon fails to start or stop within its timeout, instead of
+ waiting forever.
+
 `hostname` parameter is optional, but it's recommended to set it to some unique
  value, it will be shown in Seafile webUI as client name (`terminal-dsc` in
  given example).

dsc/client.py

@@ -12,6 +12,7 @@ from urllib3.util.retry import Retry
 import seafile
 
 from dsc import const
+from dsc.errors import DaemonError, DaemonTimeout
 from dsc.misc import create_dir, hide_password, user_cmd
 
 _lg = logging.getLogger(__name__)
@@ -59,6 +60,29 @@ class SeafileClient:
     def __gen_cmd(self, argv: list) -> list:
         return user_cmd(argv)
 
+    def __run(self, argv: list, check: bool = True, **kwargs):
+        """Run a seaf-cli command and, unless told otherwise, check its exit
+        code. An unchecked failure here surfaces much later as a daemon that
+        never becomes ready, with nothing pointing at the real cause."""
+        proc = subprocess.run(self.__gen_cmd(argv), **kwargs)
+        if check and proc.returncode != 0:
+            raise DaemonError(
+                f"Command {' '.join(argv)} failed with exit code {proc.returncode}"
+            )
+        return proc
+
+    def __wait_for_daemon(self, ready: bool, timeout: float, action: str):
+        """Wait until the daemon is ready (or stopped), but never forever."""
+        deadline = time.monotonic() + timeout
+        while self.daemon_ready != ready:
+            if time.monotonic() >= deadline:
+                state = "ready" if ready else "stopped"
+                raise DaemonTimeout(
+                    f"Seafile daemon did not become {state} within {timeout}s"
+                    f" while {action}"
+                )
+            time.sleep(const.DAEMON_POLL_PERIOD)
+
     @property
     def token(self):
         if self.__token is None:
@@ -101,32 +125,26 @@ class SeafileClient:
             return
         cmd = ["seaf-cli", "init", "-d", self.app_dir]
         _lg.info("Initializing seafile config: %s", " ".join(cmd))
-        subprocess.run(self.__gen_cmd(cmd))
+        self.__run(cmd)
 
     def start_daemon(self):
         cmd = ["seaf-cli", "start"]
         _lg.info("Starting seafile daemon: %s", " ".join(cmd))
-        subprocess.run(self.__gen_cmd(cmd))
-        _lg.info("Waiting for seafile daemon to start")
-
-        while True:
-            if self.daemon_ready:
-                break
-            time.sleep(5)
-
+        self.__run(cmd)
+        _lg.info("Waiting up to %ss for seafile daemon to start",
+                 const.DAEMON_START_TIMEOUT)
+        self.__wait_for_daemon(True, const.DAEMON_START_TIMEOUT, "starting")
         _lg.info("Seafile daemon is ready")
 
     def stop_daemon(self):
         cmd = ["seaf-cli", "stop"]
         _lg.info("Stopping seafile daemon: %s", " ".join(cmd))
-        subprocess.run(self.__gen_cmd(cmd))
-        _lg.info("Waiting for seafile daemon to stop")
-
-        while True:
-            if not self.daemon_ready:
-                break
-            time.sleep(5)
-
+        # A daemon that is already gone makes "stop" fail; the state below is
+        # what matters, not this exit code.
+        self.__run(cmd, check=False)
+        _lg.info("Waiting up to %ss for seafile daemon to stop",
+                 const.DAEMON_STOP_TIMEOUT)
+        self.__wait_for_daemon(False, const.DAEMON_STOP_TIMEOUT, "stopping")
         _lg.info("Seafile daemon is stopped")
 
     def get_library_id(self, library) -> Optional[str]:
@@ -135,11 +153,14 @@ class SeafileClient:
                 return lib_id
         return None
 
-    def sync_lib(self, lib_id: str, lib_dir: str):
+    def sync_lib(self, lib_id: str, lib_dir: str) -> bool:
         """
         Sync a library into an already resolved directory. The caller decides
         the directory, see dsc.paths.plan_lib_dirs: the name comes from the
         server and must not be turned into a path here.
+
+        Returns whether the library was accepted. One library that cannot be
+        synced must not stop the others, so this reports instead of raising.
         """
         create_dir(lib_dir)
         cmd = [
@@ -155,7 +176,12 @@ class SeafileClient:
             "Syncing library %s into %s: %s", lib_id, lib_dir,
             " ".join(hide_password(cmd, self.password)),
         )
-        subprocess.run(self.__gen_cmd(cmd))
+        proc = self.__run(cmd, check=False)
+        if proc.returncode != 0:
+            _lg.error("Failed to sync library %s into %s: seaf-cli exited with %s",
+                      lib_id, lib_dir, proc.returncode)
+            return False
+        return True
 
     def __print_tx_task(self, tx_task) -> str:
         """ Print transfer task status """
@@ -261,7 +287,7 @@ class SeafileClient:
             # check current value
             cmd = ["seaf-cli", "config", "-k", key]
             _lg.info("Checking seafile client option: %s", " ".join(cmd))
-            proc = subprocess.run(self.__gen_cmd(cmd), stdout=subprocess.PIPE)
+            proc = self.__run(cmd, stdout=subprocess.PIPE)
             # stdout looks like "option = value"
             cur_value = proc.stdout.decode().strip()
             try:
@@ -274,7 +300,7 @@ class SeafileClient:
             # set new value
             cmd = ["seaf-cli", "config", "-k", key, "-v", str(value)]
             _lg.info("Setting seafile client option: %s", " ".join(cmd))
-            subprocess.run(self.__gen_cmd(cmd))
+            self.__run(cmd)
             need_restart = True
 
         if need_restart:

dsc/const.py

@@ -4,6 +4,13 @@ DEPRECATED_LIBS_DIR = "/data"
 DEFAULT_USERNAME = "seafile"
 STATUS_POLL_PERIOD = 1
 
+# Bounds for daemon startup and shutdown, in seconds. The stop timeout has to
+# fit inside the grace period of "docker stop", which sends SIGKILL 10 seconds
+# after SIGTERM by default, otherwise shutdown is never graceful.
+DAEMON_POLL_PERIOD = 1
+DAEMON_START_TIMEOUT = 120
+DAEMON_STOP_TIMEOUT = 8
+
 AVAILABLE_SEAFCLI_OPTIONS = {
     "delete_confirm_threshold",
     "disable_verify_certificate",

dsc/errors.py

@@ -0,0 +1,17 @@
+"""Errors that must end the container with a useful message."""
+
+
+class DscError(Exception):
+    """Base class for failures that stop the client."""
+
+
+class DaemonError(DscError):
+    """A seaf-cli command failed."""
+
+
+class DaemonTimeout(DscError):
+    """The daemon did not reach the expected state in time."""
+
+
+class GracefulShutdown(DscError):
+    """SIGTERM or SIGINT was received; stop the daemon and exit cleanly."""

start.py

@@ -3,15 +3,78 @@
 import argparse
 import logging
 import os
+import signal
 import sys
 
 from dsc import SeafileClient, const
+from dsc.errors import DscError, GracefulShutdown
 from dsc.misc import setup_uid, create_dir
 from dsc.paths import plan_lib_dirs
 
 _lg = logging.getLogger('dsc')
 
 
+def handle_signal(signum, frame):
+    """Turn a stop signal into an exception so the daemon is stopped on the
+    way out instead of the container being killed with the daemon running."""
+    raise GracefulShutdown(signal.Signals(signum).name)
+
+
+def install_signal_handlers():
+    for signum in (signal.SIGTERM, signal.SIGINT):
+        signal.signal(signum, handle_signal)
+
+
+def resolve_libraries(client, requested: str, server: str) -> set:
+    """Map the requested names or IDs to library IDs that are not synced yet."""
+    libs_to_sync = set()
+    for arg_lib in requested.split(sep=":"):
+        lib_id = client.get_library_id(arg_lib)
+        if lib_id:
+            libs_to_sync.add(lib_id)
+        else:
+            _lg.warning("Library %s is not found on server %s", arg_lib, server)
+
+    # don't start to sync libraries already in sync
+    return libs_to_sync - client.get_local_libraries()
+
+
+def run(client, args, libs_dir: str) -> int:
+    """
+    Run the client lifecycle and return the process exit code. The daemon is
+    always stopped on the way out, including on a failure or a stop signal.
+    """
+    rc = 0
+    try:
+        client.init_config()
+        client.start_daemon()
+        client.configure(args, check_for_daemon=False)
+
+        libs_to_sync = resolve_libraries(client, args.libraries, args.server)
+
+        # Library names come from the server, so the directory for each one is
+        # resolved and checked before anything is created or written.
+        lib_dirs = plan_lib_dirs(
+            {lib_id: client.remote_libraries[lib_id] for lib_id in libs_to_sync},
+            libs_dir,
+        )
+        for lib_id, lib_dir in lib_dirs.items():
+            client.sync_lib(lib_id, lib_dir)
+        client.watch_status()
+    except GracefulShutdown as err:
+        _lg.info("Received %s, shutting down", err)
+    except DscError as err:
+        _lg.error("%s", err)
+        rc = 1
+
+    try:
+        client.stop_daemon()
+    except DscError as err:
+        _lg.error("Could not stop seafile daemon cleanly: %s", err)
+        rc = 1
+    return rc
+
+
 def main():
     logging.basicConfig(format="%(asctime)s %(message)s", level=logging.INFO)
 
@@ -49,24 +112,12 @@ def main():
     if not args.libraries:
         parser.error("library is not specified")
 
+    install_signal_handlers()
+
     setup_uid(args.uid, args.gid)
     create_dir(const.DEFAULT_APP_DIR)
 
     client = SeafileClient(args.server, args.username, args.password, const.DEFAULT_APP_DIR)
-    client.init_config()
-    client.start_daemon()
-    client.configure(args, check_for_daemon=False)
-
-    libs_to_sync = set()
-    for arg_lib in args.libraries.split(sep=":"):
-        lib_id = client.get_library_id(arg_lib)
-        if lib_id:
-            libs_to_sync.add(lib_id)
-        else:
-            _lg.warning("Library %s is not found on server %s", arg_lib, args.server)
-
-    # don't start to sync libraries already in sync
-    libs_to_sync -= client.get_local_libraries()
 
     # check for deprecated /data directory
     if os.path.isdir(const.DEPRECATED_LIBS_DIR):
@@ -77,18 +128,7 @@ def main():
     else:
         libs_dir = const.DEFAULT_LIBS_DIR
 
-    # Library names come from the server, so the directory for each one is
-    # resolved and checked before anything is created or written.
-    lib_dirs = plan_lib_dirs(
-        {lib_id: client.remote_libraries[lib_id] for lib_id in libs_to_sync},
-        libs_dir,
-    )
-    for lib_id, lib_dir in lib_dirs.items():
-        client.sync_lib(lib_id, lib_dir)
-    client.watch_status()
-
-    client.stop_daemon()
-    return 0
+    return run(client, args, libs_dir)
 
 
 if __name__ == "__main__":

tests/test_daemon.py

@@ -0,0 +1,184 @@
+"""Startup and shutdown must be bounded, checked, and interruptible."""
+
+import argparse
+import signal
+import subprocess
+import time
+
+import pytest
+
+import start
+from dsc import client as client_module
+from dsc import const
+from dsc.client import SeafileClient
+from dsc.errors import DaemonError, DaemonTimeout, GracefulShutdown
+
+
+@pytest.fixture(autouse=True)
+def fast_polling(monkeypatch):
+    """Keep the bounded waits bounded to milliseconds in tests."""
+    monkeypatch.setattr(const, "DAEMON_POLL_PERIOD", 0)
+    monkeypatch.setattr(const, "DAEMON_START_TIMEOUT", 0.05)
+    monkeypatch.setattr(const, "DAEMON_STOP_TIMEOUT", 0.05)
+
+
+@pytest.fixture
+def client(monkeypatch):
+    monkeypatch.setattr(client_module, "create_dir", lambda path: None)
+    return SeafileClient("seafile.example", "user", "pw", "/dsc")
+
+
+def fake_runner(returncodes, monkeypatch, stdout=b""):
+    """Return exit codes by seaf-cli subcommand; record every call."""
+    calls = []
+
+    def fake_run(argv, **kwargs):
+        calls.append(argv)
+        subcommand = argv[argv.index("seaf-cli") + 1]
+        code = returncodes.get(subcommand, 0)
+        return subprocess.CompletedProcess(argv, code, stdout=stdout)
+
+    monkeypatch.setattr(subprocess, "run", fake_run)
+    return calls
+
+
+def test_start_daemon_fails_when_the_command_fails(client, monkeypatch):
+    fake_runner({"start": 1}, monkeypatch)
+    with pytest.raises(DaemonError):
+        client.start_daemon()
+
+
+def test_init_config_fails_when_the_command_fails(client, monkeypatch):
+    monkeypatch.setattr(type(client), "config_initialized", property(lambda self: False))
+    fake_runner({"init": 2}, monkeypatch)
+    with pytest.raises(DaemonError):
+        client.init_config()
+
+
+def test_start_daemon_gives_up_when_the_daemon_never_becomes_ready(client, monkeypatch):
+    # "start" succeeds but "status" never does: the daemon never comes up.
+    fake_runner({"status": 1}, monkeypatch)
+
+    started = time.monotonic()
+    with pytest.raises(DaemonTimeout) as err:
+        client.start_daemon()
+
+    assert time.monotonic() - started < 5, "the wait must be bounded"
+    assert "ready" in str(err.value).lower()
+
+
+def test_start_daemon_returns_once_ready(client, monkeypatch):
+    fake_runner({}, monkeypatch)  # status returns 0 straight away
+    client.start_daemon()
+
+
+def test_stop_daemon_gives_up_when_the_daemon_never_stops(client, monkeypatch):
+    # "status" keeps succeeding, so the daemon never reports itself stopped.
+    fake_runner({}, monkeypatch)
+
+    started = time.monotonic()
+    with pytest.raises(DaemonTimeout):
+        client.stop_daemon()
+
+    assert time.monotonic() - started < 5, "the wait must be bounded"
+
+
+def test_stop_daemon_succeeds_when_the_daemon_is_already_gone(client, monkeypatch):
+    # A stopped daemon makes both "stop" and "status" fail; that is success.
+    fake_runner({"stop": 1, "status": 1}, monkeypatch)
+    client.stop_daemon()
+
+
+def test_stop_timeout_fits_inside_the_docker_stop_grace_period():
+    """docker stop sends SIGKILL 10s after SIGTERM by default."""
+    assert const.DAEMON_STOP_TIMEOUT < 10
+
+
+def test_configure_fails_when_setting_an_option_fails(client, monkeypatch):
+    fake_runner({"config": 1}, monkeypatch)
+    with pytest.raises(DaemonError):
+        client.configure(argparse.Namespace(upload_limit=100), check_for_daemon=False)
+
+
+def test_sync_lib_reports_a_failure_without_stopping_other_libraries(client, monkeypatch, caplog):
+    fake_runner({"sync": 1}, monkeypatch)
+    with caplog.at_level("ERROR"):
+        assert client.sync_lib("lib-id", "/dsc/seafile/Docs") is False
+    assert "lib-id" in caplog.text
+
+
+def test_sync_lib_reports_success(client, monkeypatch):
+    fake_runner({}, monkeypatch)
+    assert client.sync_lib("lib-id", "/dsc/seafile/Docs") is True
+
+
+class FakeClient:
+    """A client whose lifecycle calls are recorded instead of executed."""
+
+    def __init__(self, fail_on=None, error=None):
+        self.fail_on = fail_on
+        self.error = error or DaemonError("boom")
+        self.calls = []
+        self.remote_libraries = {"lib-id": "Docs"}
+
+    def __getattr__(self, name):
+        def record(*args, **kwargs):
+            self.calls.append(name)
+            if name == self.fail_on:
+                raise self.error
+            return True
+        return record
+
+    def __record(self, name):
+        self.calls.append(name)
+        if name == self.fail_on:
+            raise self.error
+
+    def get_library_id(self, name):
+        self.__record("get_library_id")
+        return "lib-id"
+
+    def get_local_libraries(self):
+        self.__record("get_local_libraries")
+        return set()
+
+
+ARGS = argparse.Namespace(libraries="Docs", server="seafile.example")
+
+
+def test_run_stops_the_daemon_after_a_normal_finish():
+    client = FakeClient()
+    assert start.run(client, ARGS, "/dsc/seafile") == 0
+    assert client.calls[-1] == "stop_daemon"
+
+
+@pytest.mark.parametrize("failing_step", [
+    "init_config", "start_daemon", "configure", "get_local_libraries", "watch_status",
+])
+def test_run_returns_nonzero_and_still_stops_the_daemon_on_failure(failing_step):
+    client = FakeClient(fail_on=failing_step)
+    assert start.run(client, ARGS, "/dsc/seafile") != 0
+    assert client.calls[-1] == "stop_daemon"
+
+
+def test_run_returns_zero_and_stops_the_daemon_on_shutdown_signal():
+    client = FakeClient(fail_on="watch_status", error=GracefulShutdown("SIGTERM"))
+    assert start.run(client, ARGS, "/dsc/seafile") == 0
+    assert client.calls[-1] == "stop_daemon"
+
+
+def test_run_reports_failure_even_if_stopping_also_fails():
+    client = FakeClient(fail_on="stop_daemon", error=DaemonTimeout("stuck"))
+    assert start.run(client, ARGS, "/dsc/seafile") != 0
+
+
+@pytest.mark.parametrize("signum", [signal.SIGTERM, signal.SIGINT])
+def test_signal_handlers_raise_a_graceful_shutdown(signum):
+    previous = signal.getsignal(signum)
+    try:
+        start.install_signal_handlers()
+        handler = signal.getsignal(signum)
+        with pytest.raises(GracefulShutdown):
+            handler(signum, None)
+    finally:
+        signal.signal(signum, previous)