From 0bb57a4fcda379c17a2f897d228ae7753f16e67e Mon Sep 17 00:00:00 2001 From: Jiangzhou He Date: Fri, 24 Jul 2026 07:59:37 -0700 Subject: [PATCH] fix(client): survive handshake from a protocol-incompatible daemon MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes #237. Upgrading 0.2.37 → 0.2.38 bricked every ccc command with "ValidationError: Object missing required field `pid`": 0.2.38 added a required `pid` field to HandshakeResponse, so the reply of a still-running pre-upgrade daemon no longer decoded — before the client could see the `ok=False` version mismatch and restart it. The same uncaught error also broke `ccc daemon stop`, the recovery path. Three layers: - `HandshakeResponse.pid` gets a default (None), with a comment stating the wire-compat rule: handshake fields added after a release must have defaults, since the handshake is the one message exchanged between mismatched versions. - An undecodable handshake reply now raises `DaemonProtocolError` instead of escaping as a raw decode error; `_connect_and_handshake` treats it like a version mismatch (restart on first contact, fail fast once a matching daemon was ensured). This protects against any future wire drift, not just this field. - `stop_daemon` tolerates the decode failure and falls through to its SIGTERM/SIGKILL escalation, so `ccc daemon stop` always works. Co-Authored-By: Claude Fable 5 --- src/cocoindex_code/client.py | 43 ++++++++++++++++-- src/cocoindex_code/protocol.py | 10 +++- tests/test_client.py | 83 ++++++++++++++++++++++++++++++++++ tests/test_protocol.py | 23 ++++++++++ 4 files changed, 155 insertions(+), 4 deletions(-) diff --git a/src/cocoindex_code/client.py b/src/cocoindex_code/client.py index 272eea8..f65dafb 100644 --- a/src/cocoindex_code/client.py +++ b/src/cocoindex_code/client.py @@ -18,6 +18,8 @@ from pathlib import Path from typing import NamedTuple +import msgspec + from ._daemon_paths import ( connection_family, daemon_log_path, @@ -158,6 +160,16 @@ def _connect_and_handshake() -> Connection: if _daemon_ensured and not e.resp.ok: raise stop_daemon() + except DaemonProtocolError: + # The reply didn't even decode — a daemon whose wire protocol + # drifted beyond what the version-mismatch path can express (e.g. + # still running a pre-upgrade binary). Same treatment as a version + # mismatch: restart it below; once a matching daemon was already + # ensured this cannot legitimately happen, so fail fast instead of + # looping on restarts. + if _daemon_ensured: + raise + stop_daemon() except (ConnectionRefusedError, OSError): # No daemon answered. Normal on the first call of this process (start # one below). If we had already ensured one, decide crash vs graceful @@ -238,7 +250,15 @@ def _raw_connect_and_handshake() -> _HandshakeResult: conn.close() raise ConnectionRefusedError(f"Handshake failed: {e}") from e - resp = decode_response(data) + try: + resp = decode_response(data) + except msgspec.DecodeError as e: + # The daemon speaks an incompatible wire protocol — e.g. a stale + # pre-upgrade daemon whose handshake reply no longer matches this + # client's schema. Must not escape as a raw decode error: the caller + # handles DaemonProtocolError by restarting the daemon (issue #237). + conn.close() + raise DaemonProtocolError(f"Undecodable handshake reply from daemon: {e}") from e if isinstance(resp, ErrorResponse): conn.close() raise RuntimeError(f"Daemon error: {resp.message}") @@ -274,6 +294,19 @@ def __init__(self, resp: HandshakeResponse) -> None: super().__init__(message) +class DaemonProtocolError(RuntimeError): + """Raised when the daemon's handshake reply cannot be decoded at all. + + A version-mismatched daemon normally reports itself via ``resp.ok`` being + False (→ ``DaemonVersionError``), but that requires its reply to decode + against this client's schema. When even decoding fails, the daemon is + from a version whose wire protocol drifted too far — handled the same + way: restart on first contact, fail fast once a matching daemon was + already ensured. Subclasses RuntimeError so broad daemon-failure handlers + (e.g. in ``stop_daemon``) cover it. + """ + + class DaemonStartError(RuntimeError): """Raised when the daemon process fails to start. @@ -427,7 +460,7 @@ def send_heartbeat() -> bool: """ try: conn, _resp = _raw_connect_and_handshake() - except (ConnectionRefusedError, DaemonVersionError, OSError): + except (ConnectionRefusedError, DaemonVersionError, DaemonProtocolError, OSError): return False try: conn.send_bytes(encode_request(HeartbeatRequest())) @@ -608,7 +641,11 @@ def stop_daemon() -> None: conn.recv_bytes() finally: conn.close() - except (ConnectionRefusedError, OSError, RuntimeError, DaemonVersionError): + except (ConnectionRefusedError, OSError, RuntimeError, DaemonVersionError, DaemonProtocolError): + # A version-mismatched or protocol-incompatible daemon refuses the + # handshake, so the graceful StopRequest is impossible — fall through + # to the SIGTERM escalation below, which must never be skipped just + # because the daemon speaks a different protocol (issue #237). pass if _wait_for_daemon_exit(timeout=3.0): diff --git a/src/cocoindex_code/protocol.py b/src/cocoindex_code/protocol.py index 164dedf..77625bd 100644 --- a/src/cocoindex_code/protocol.py +++ b/src/cocoindex_code/protocol.py @@ -79,13 +79,21 @@ class HeartbeatRequest(_msgspec.Struct, tag="heartbeat"): class HandshakeResponse(_msgspec.Struct, tag="handshake"): + # The handshake is the one message exchanged between *mismatched* + # client/daemon versions — it is how a mismatch gets detected in the + # first place. Every field added after a release MUST have a default: + # a required field makes an older daemon's reply undecodable, which + # breaks the restart-on-mismatch path and bricks every command + # (issue #237). ok: bool daemon_version: str # The daemon's process id. The client remembers it so that, when the # daemon later vanishes, the graceful-exit marker (written on shutdown # with the same pid) can be matched race-free against the exact process # the client was talking to — distinguishing a graceful exit from a crash. - pid: int + # None only when decoding the reply of a pre-0.2.38 daemon, whose + # handshake never satisfies ``ok`` and version checks anyway. + pid: int | None = None global_settings_mtime_us: int | None = None # Non-fatal daemon-side warnings surfaced to the client on every handshake. # The client dedupes and prints them to stderr (see client._print_handshake_warnings). diff --git a/tests/test_client.py b/tests/test_client.py index 49e0563..5b1f453 100644 --- a/tests/test_client.py +++ b/tests/test_client.py @@ -147,6 +147,89 @@ def fake_raw() -> object: assert started["start"] == 0 # never tried to restart +def test_connect_restarts_daemon_on_undecodable_handshake( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """A handshake reply that does not even decode — a daemon whose wire + protocol drifted beyond the version-mismatch path, e.g. a stale + pre-upgrade daemon (issue #237) — is restarted, not surfaced as a raw + decode error. + """ + monkeypatch.setattr(client, "_daemon_ensured", False) + + sentinel_conn = object() + ok_resp = HandshakeResponse(ok=True, daemon_version="v1", pid=42) + calls = {"raw": 0, "stop": 0, "start": 0} + + def fake_raw() -> client._HandshakeResult: + calls["raw"] += 1 + if calls["raw"] == 1: + raise client.DaemonProtocolError("Undecodable handshake reply from daemon") + return client._HandshakeResult(conn=cast(Connection, sentinel_conn), resp=ok_resp) + + monkeypatch.setattr(client, "_raw_connect_and_handshake", fake_raw) + monkeypatch.setattr(client, "stop_daemon", lambda: calls.update(stop=calls["stop"] + 1)) + monkeypatch.setattr(client, "start_daemon", lambda: calls.update(start=calls["start"] + 1)) + monkeypatch.setattr(client, "_wait_for_daemon", lambda **_kw: None) + monkeypatch.setattr(client, "_is_daemon_supervised", lambda: False) + + conn = client._connect_and_handshake() + + assert conn is sentinel_conn + assert calls["stop"] == 1 # incompatible daemon stopped + assert calls["start"] == 1 # fresh daemon started + assert calls["raw"] == 2 # reconnected after restart + + +def test_connect_fails_fast_on_undecodable_handshake_after_ensured( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """Once a matching daemon was ensured, an undecodable handshake reply + cannot legitimately happen — fail fast, don't loop on restarts. + """ + monkeypatch.setattr(client, "_daemon_ensured", True) + started = {"start": 0} + + def fake_raw() -> client._HandshakeResult: + raise client.DaemonProtocolError("Undecodable handshake reply from daemon") + + monkeypatch.setattr(client, "_raw_connect_and_handshake", fake_raw) + monkeypatch.setattr(client, "stop_daemon", lambda: None) + monkeypatch.setattr(client, "start_daemon", lambda: started.update(start=1)) + monkeypatch.setattr(client, "_wait_for_daemon", lambda **_kw: None) + monkeypatch.setattr(client, "_is_daemon_supervised", lambda: False) + + with pytest.raises(client.DaemonProtocolError): + client._connect_and_handshake() + assert started["start"] == 0 # never tried to restart + + +def test_stop_daemon_escalates_past_undecodable_handshake( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """``ccc daemon stop`` must still work against a protocol-incompatible + daemon (issue #237): the graceful StopRequest is impossible, so it falls + through to the kill escalation instead of crashing. + """ + monkeypatch.setattr(client, "daemon_pid_path", lambda: tmp_path / "daemon.pid") + + def fake_raw() -> client._HandshakeResult: + raise client.DaemonProtocolError("Undecodable handshake reply from daemon") + + monkeypatch.setattr(client, "_raw_connect_and_handshake", fake_raw) + waited = {"n": 0} + + def fake_wait(timeout: float) -> bool: + waited["n"] += 1 + return True + + monkeypatch.setattr(client, "_wait_for_daemon_exit", fake_wait) + + client.stop_daemon() # must not raise + + assert waited["n"] == 1 # reached the escalation ladder + + # --------------------------------------------------------------------------- # Vanished-daemon handling: graceful-exit marker vs crash # --------------------------------------------------------------------------- diff --git a/tests/test_protocol.py b/tests/test_protocol.py index f026764..923cea2 100644 --- a/tests/test_protocol.py +++ b/tests/test_protocol.py @@ -2,6 +2,8 @@ from __future__ import annotations +import msgspec + from cocoindex_code.protocol import ( DaemonEnvRequest, DaemonEnvResponse, @@ -56,6 +58,27 @@ def test_encode_decode_handshake_response_with_pid() -> None: assert decoded.pid == 4242 +def test_decode_handshake_response_from_pre_0_2_38_daemon() -> None: + """A pre-0.2.38 daemon's handshake reply has no ``pid`` field; a newer + client must still decode it to reach the version-mismatch restart path + (issue #237). + """ + old_reply = msgspec.msgpack.encode( + { + "type": "handshake", + "ok": False, + "daemon_version": "0.2.37", + "global_settings_mtime_us": 1234, + "warnings": [], + } + ) + decoded = decode_response(old_reply) + assert isinstance(decoded, HandshakeResponse) + assert decoded.ok is False + assert decoded.pid is None + assert decoded.daemon_version == "0.2.37" + + def test_encode_decode_search_request_with_defaults() -> None: req = SearchRequest(project_root="/tmp", query="test") data = encode_request(req)