Bound daemon startup and shutdown and exit with a useful code

start_daemon and stop_daemon polled "while True" with no bound, and no
seaf-cli exit code was checked. A daemon that failed to start left the
container spinning forever with nothing pointing at the cause, and there was
no signal handling, so "docker stop" waited the full 10 second grace period
and then SIGKILLed the process with the daemon still running.

Readiness polling is now bounded by DAEMON_START_TIMEOUT and
DAEMON_STOP_TIMEOUT. The stop timeout is 8 seconds so a graceful shutdown
fits inside the docker stop grace period, and a test asserts that relation.

SIGTERM and SIGINT raise GracefulShutdown, and the lifecycle moved into
start.run(), which always stops the daemon on the way out and returns the
process exit code: 0 on a clean finish or a stop signal, 1 on a daemon or
configuration failure.

seaf-cli exit codes are checked. A failed sync is reported per library and
does not stop the remaining libraries, since one bad library should not take
the container down.

Verified in the built image: "docker stop" now returns in 0s with exit code 0
and the daemon stopped, against 10s and exit 137 before. A daemon that cannot
start exits 1 with the failing command and its exit code in the log.

Also bumps actions/setup-python to v7, which clears the last Node 20
deprecation warning; v7 only drops the unused pip-install input.

.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.
+- [x] Check every important subprocess return code. A failed `seaf-cli sync` is
-- [ ] Add timeouts to daemon startup, shutdown, and readiness polling.
+  reported per library and does not stop the remaining ones.
-  Confirmed: `start_daemon` and `stop_daemon` poll `while True` with no bound,
+- [x] Add timeouts to daemon startup, shutdown, and readiness polling. See
-  so a daemon that never changes state hangs the container silently.
+  `DAEMON_START_TIMEOUT` and `DAEMON_STOP_TIMEOUT` in `dsc/const.py`; the stop
-- [ ] Handle `SIGTERM` and `SIGINT` and stop the daemon in `finally`.
+  timeout is deliberately below the 10 second `docker stop` grace period.
-- [ ] Return a nonzero container exit code on configuration or daemon failure.
+- [x] Handle `SIGTERM` and `SIGINT` and stop the daemon in `finally`.
-- [ ] Produce actionable errors instead of waiting forever.
+- [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))
+        self.__run(cmd)
-        _lg.info("Waiting for seafile daemon to start")
+        _lg.info("Waiting up to %ss for seafile daemon to start",
-
+                 const.DAEMON_START_TIMEOUT)
-        while True:
+        self.__wait_for_daemon(True, const.DAEMON_START_TIMEOUT, "starting")
-            if self.daemon_ready:
-                break
-            time.sleep(5)
-
         _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))
+        # A daemon that is already gone makes "stop" fail; the state below is
-        _lg.info("Waiting for seafile daemon to stop")
+        # what matters, not this exit code.
-
+        self.__run(cmd, check=False)
-        while True:
+        _lg.info("Waiting up to %ss for seafile daemon to stop",
-            if not self.daemon_ready:
+                 const.DAEMON_STOP_TIMEOUT)
-                break
+        self.__wait_for_daemon(False, const.DAEMON_STOP_TIMEOUT, "stopping")
-            time.sleep(5)
-
         _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
+    return run(client, args, libs_dir)
-    # 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
 
 
 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)