diff --git a/.github/workflows/docker-publish.yml b/.github/workflows/docker-publish.yml index 47feafc..b62c574 100644 --- a/.github/workflows/docker-publish.yml +++ b/.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 diff --git a/IMPROVEMENTS.md b/IMPROVEMENTS.md index fe9ab92..f4e827a 100644 --- a/IMPROVEMENTS.md +++ b/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. diff --git a/README.md b/README.md index d74553b..5f00eb2 100644 --- a/README.md +++ b/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). diff --git a/dsc/client.py b/dsc/client.py index 0ef5a66..6e29f37 100644 --- a/dsc/client.py +++ b/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: diff --git a/dsc/const.py b/dsc/const.py index 15b800f..b7f623e 100644 --- a/dsc/const.py +++ b/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", diff --git a/dsc/errors.py b/dsc/errors.py new file mode 100644 index 0000000..3b188d3 --- /dev/null +++ b/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.""" diff --git a/start.py b/start.py index a71312c..5733e23 100755 --- a/start.py +++ b/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__": diff --git a/tests/test_daemon.py b/tests/test_daemon.py new file mode 100644 index 0000000..d182a49 --- /dev/null +++ b/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)