From 123705aa53be6c8ed244d368f1921bda8363cd02 Mon Sep 17 00:00:00 2001 From: Maksim Shulga Date: Mon, 3 Aug 2026 18:55:34 +0000 Subject: [PATCH] Handle temporary RPC failures and daemon restarts without corrupting state (#35) * Handle temporary RPC failures and daemon restarts without corrupting state watch_status()'s per-iteration RPC read had no error handling: an unexpected failure (the daemon crashing or restarting mid-call, or any other transient RPC hiccup) propagated straight out of the loop and crashed the whole container, indistinguishable from a real fatal error. The read is now wrapped: an unexpected exception logs a warning and retries on the next tick, leaving prev_status untouched so the next successful read still reports every real change. A stop signal or another DscError still ends the loop exactly as before - the wrapper explicitly re-raises DscError first, since GracefulShutdown is one and must not be swallowed by the broader except below it. Verified against the real image: a container survives seaf-daemon being killed outright, logging the failure instead of exiting. * Log a traceback on RPC failure and strengthen the state test Copilot review: - The RPC failure warning logged only the message; add exc_info so a repeated failure in production is actually debuggable. - The 'prior state is not corrupted' test asserted the warning fired but never proved a real change after the failure was still detected and logged, which is the actual property being claimed. --------- Co-authored-by: Maxim Shulga --- IMPROVEMENTS.md | 10 +++++-- dsc/client.py | 18 ++++++++++-- tests/smoke.sh | 46 ++++++++++++++++++++++++++++- tests/test_status.py | 70 ++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 139 insertions(+), 5 deletions(-) create mode 100644 tests/test_status.py diff --git a/IMPROVEMENTS.md b/IMPROVEMENTS.md index 6aa04d1..d2a228a 100644 --- a/IMPROVEMENTS.md +++ b/IMPROVEMENTS.md @@ -214,8 +214,14 @@ Acceptance criteria: Each check is a fresh root-spawned process (unlike the long-lived `start.py`), so it drops to the `seafile` user with `runuser` before touching the RPC socket - the one place that mechanism still belongs. -- [ ] Handle temporary RPC failures and daemon restarts without corrupting - state. +- [x] Handle temporary RPC failures and daemon restarts without corrupting + state. `watch_status()`'s per-iteration RPC read is now wrapped so an + unexpected failure (the daemon crashing or restarting mid-call) logs a + warning and retries on the next tick instead of crashing the container; + a stop signal or another `DscError` still ends the loop as before. Prior + state is left untouched across a failed read, so the next successful one + still reports every real change. Verified against the real image: the + container survives `seaf-daemon` being killed outright. - [ ] Document supported CPU architectures instead of implying multi-platform support. diff --git a/dsc/client.py b/dsc/client.py index 07dcd25..986eab2 100644 --- a/dsc/client.py +++ b/dsc/client.py @@ -13,7 +13,7 @@ from urllib3.util.retry import Retry import seafile from dsc import const -from dsc.errors import ConfigError, DaemonError, DaemonTimeout, NetworkError +from dsc.errors import ConfigError, DaemonError, DaemonTimeout, DscError, NetworkError from dsc.misc import config_value, create_dir, hide_secrets, user_cmd _lg = logging.getLogger(__name__) @@ -302,7 +302,21 @@ class SeafileClient: fmt = "Library {:%ds} {}" % max_name_len while True: time.sleep(const.STATUS_POLL_PERIOD) - cur_status = self.get_status() + try: + cur_status = self.get_status() + except DscError: + # A stop signal or another DscError must still end the loop; + # only an unexpected RPC failure below is worth retrying. + raise + except Exception as err: + # The daemon can restart under us; a status read caught mid + # restart must not take the whole process down with it. + # prev_status is left untouched, so the next successful read + # still reports every real change since it, not just since + # this failure. + _lg.warning("Could not read sync status, retrying: %s", err, + exc_info=err) + continue for library, state in cur_status.items(): if state != prev_status.get(library): if 30 > len(library) > max_name_len: diff --git a/tests/smoke.sh b/tests/smoke.sh index 9ebec88..204d53f 100755 --- a/tests/smoke.sh +++ b/tests/smoke.sh @@ -119,6 +119,49 @@ else fi docker rm -f dsc-smoke >/dev/null +echo "== a killed daemon does not crash the watch loop" +docker rm -f dsc-smoke >/dev/null 2>&1 || true +docker run -d $PLATFORM_ARG --name dsc-smoke "$IMAGE" python3 -c " +import logging +logging.basicConfig(format='%(asctime)s %(message)s', level=logging.INFO) +from dsc.client import SeafileClient + +c = SeafileClient('example.invalid', 'u', 'pw', '/dsc', token='tok') +c.init_config() +c.start_daemon() +c.watch_status() # the real thing, not a stub: this is what is under test +" >/dev/null + +started=0 +for _ in $(seq 1 30); do + logs=$(docker logs dsc-smoke 2>&1) + if printf '%s' "$logs" | grep -q "Seafile daemon is ready"; then started=1; break; fi + sleep 1 +done + +if [ "$started" -eq 0 ]; then + fail "the daemon never became ready, kill scenario not measured" +else + # No procps in the image, so find the PID by hand instead of pkill. + docker exec dsc-smoke sh -c ' + for p in /proc/[0-9]*; do + [ "$(cat "$p/comm" 2>/dev/null)" = "seaf-daemon" ] && kill -9 "${p#/proc/}" + done' >/dev/null 2>&1 + recovered=0 + for _ in $(seq 1 30); do + logs=$(docker logs dsc-smoke 2>&1) + if printf '%s' "$logs" | grep -q "Could not read sync status"; then recovered=1; break; fi + sleep 1 + done + if [ "$recovered" -eq 1 ] && [ "$(docker inspect -f '{{.State.Running}}' dsc-smoke)" = "true" ]; then + pass "the watch loop logged the failure and kept the container running" + else + fail "the container did not survive a killed daemon" + docker logs dsc-smoke 2>&1 | sed 's/^/ /' | tail -20 + fi +fi +docker rm -f dsc-smoke >/dev/null + echo "== the HEALTHCHECK command through the real entrypoint" # Runs through main(), unlike the fixture above: HOME/UID/GID come from the # same drop_privileges() path a real deployment uses, so .ccnet ends up @@ -130,7 +173,8 @@ docker run -d $PLATFORM_ARG --name dsc-smoke \ ready=0 for _ in $(seq 1 30); do - if docker logs dsc-smoke 2>&1 | grep -q "Seafile daemon is ready"; then ready=1; break; fi + logs=$(docker logs dsc-smoke 2>&1) + if printf '%s' "$logs" | grep -q "Seafile daemon is ready"; then ready=1; break; fi sleep 1 done diff --git a/tests/test_status.py b/tests/test_status.py new file mode 100644 index 0000000..c4f0bb7 --- /dev/null +++ b/tests/test_status.py @@ -0,0 +1,70 @@ +"""A transient RPC failure must not take the whole process down with it.""" + +import time + +import pytest + +from dsc.client import SeafileClient +from dsc.errors import GracefulShutdown + + +@pytest.fixture +def client(monkeypatch): + monkeypatch.setattr(time, "sleep", lambda seconds: None) + return SeafileClient("seafile.example", "user", "pw", "/dsc", token="tok-en") + + +def test_watch_status_survives_a_transient_rpc_failure(client, monkeypatch): + """The loop keeps going instead of crashing the container.""" + calls = [] + + def flaky_get_status(): + calls.append(1) + if len(calls) == 1: + raise ConnectionError("daemon restarting") + raise GracefulShutdown("SIGTERM") # ends the otherwise-infinite loop + + monkeypatch.setattr(client, "get_status", flaky_get_status) + + with pytest.raises(GracefulShutdown): + client.watch_status() + + assert len(calls) == 2, "the loop must retry after the first failure" + + +def test_watch_status_still_propagates_a_stop_signal(client, monkeypatch): + """A DscError, such as a stop signal, must still end the loop.""" + monkeypatch.setattr(client, "get_status", + lambda: (_ for _ in ()).throw(GracefulShutdown("SIGINT"))) + + with pytest.raises(GracefulShutdown): + client.watch_status() + + +def test_watch_status_keeps_prior_state_across_a_failed_read(client, monkeypatch, caplog): + """ + A failed read is logged and skipped; it must not corrupt prior state, + so a real change right after the failure is still detected and logged. + """ + calls = [] + + def flaky_get_status(): + calls.append(1) + if len(calls) == 1: + return {"Docs": "syncing"} + if len(calls) == 2: + raise ConnectionError("daemon restarting") + if len(calls) == 3: + return {"Docs": "done"} + raise GracefulShutdown("SIGTERM") + + monkeypatch.setattr(client, "get_status", flaky_get_status) + + with caplog.at_level("INFO"): + with pytest.raises(GracefulShutdown): + client.watch_status() + + assert "Could not read sync status" in caplog.text + assert "syncing" in caplog.text + assert "done" in caplog.text + assert len(calls) == 4