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 <maxim.shulga@alludo.com>

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.
 

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)
+            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:

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
 

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