Skip to content

Commit d2b1ad7

Browse files
Document four structural divergences from go-dqlite
Expand four in-source rationale blocks describing deliberate Python-side deviations from go-dqlite's reference implementation, with quantitative worst-case shapes operators porting code need to know about: ClusterClient.connect docstring: cancel-propagation discipline (Go checks ctx.Done at iteration tops and lets the first attempt always run; Python's asyncio cancel propagates through every await, so an outer asyncio.timeout(0) can land before the first attempt runs) and the double-dial structural cost (Go adopts the probe socket; Python returns an address and re-dials, paying one extra RTT per successful connect on the leader). find_leader docstring: address-only return contrasted with go- dqlite's protocol-handoff design; the per-acquirer cost grows with how often the single-flight slot collapses concurrent callers. DqliteProtocol.handshake: expand the existing client_id divergence comment with the operational trade-offs (mixed-client cluster log filtering, no cross-reconnect correlation, secrets.randbits vs random.getrandbits fork-safety choice). DqliteProtocol._operation_deadline: quantify the per-phase worst- case (10-frame continuation with timeout=10s reaches ~120s end-to-end) and call out the trust_server_heartbeat amplification when the per-phase budget can stretch to the 300s cap. Pin each rationale block via substring-fence tests; pin the asyncio-cancel-propagation contract with a behavioural test that asserts no per-attempt breadcrumb fires under asyncio.timeout(0)-before-first-attempt.
1 parent 7c8188e commit d2b1ad7

4 files changed

Lines changed: 265 additions & 13 deletions

File tree

‎src/dqliteclient/cluster.py‎

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -678,6 +678,19 @@ async def find_leader(
678678
single-flight collapse is a deliberate trade-off: the alternative
679679
(re-poll the node store between probes) costs an extra await
680680
per probe to close a window most callers do not exercise.
681+
682+
Address-only return: go-dqlite's connector adopts the probe
683+
socket on success and returns the open ``*Protocol``;
684+
Python's ``find_leader`` returns just an address string,
685+
and high-level callers (:meth:`connect`,
686+
:meth:`ConnectionPool._create_connection`,
687+
:meth:`open_admin_connection`) re-dial. The cost is one
688+
extra TCP three-way + handshake RTT per success path on
689+
the leader — the leader pays N round-trips for N
690+
concurrent acquirers if no single-flight collapse applies,
691+
and one round-trip for N collapsed acquirers if it does.
692+
Operators sizing ``attempt_timeout`` on high-RTT WAN
693+
deployments should budget for the extra round trip.
681694
"""
682695
self._check_pid()
683696
# Resolve ``policy=None`` to ``self._redirect_policy`` BEFORE
@@ -1608,6 +1621,43 @@ async def connect(
16081621
enable debug logging to diagnose cluster churn instead of
16091622
seeing only the final exception.
16101623
1624+
Comparison to go-dqlite
1625+
-----------------------
1626+
1627+
Two structural divergences from go-dqlite's
1628+
``Connector.Connect`` (``/internal/protocol/connector.go``)
1629+
operators porting code should know about:
1630+
1631+
1. Cancel-propagation discipline. go-dqlite checks
1632+
``ctx.Done()`` only at the TOP of each retry iteration
1633+
(``attempt > 1``), so the FIRST attempt always runs even
1634+
if the context was already cancelled when ``Connect``
1635+
was invoked. Python's asyncio cancel propagates through
1636+
every ``await`` checkpoint, so an outer
1637+
``asyncio.timeout(0)`` wrapping ``cluster.connect()``
1638+
lands the cancel BEFORE the first attempt. The first
1639+
attempt never runs; the caller sees a bare
1640+
``TimeoutError`` with no per-attempt log breadcrumb.
1641+
This is the CORRECT shape for the Python ecosystem
1642+
(asyncio's structured-concurrency cancel contract is the
1643+
source of truth); callers porting from Go that rely on
1644+
"one attempt is always made" semantics must adapt.
1645+
1646+
2. Discovery vs. connection: double-dial cost. go-dqlite's
1647+
``connectAttemptOne`` adopts the probe socket on success
1648+
(``case address:`` arm at ``connector.go:392-409`` sends
1649+
``EncodeClient`` on the OPEN socket). Python's design
1650+
returns an address from ``_query_leader`` and re-dials
1651+
in ``_connect_impl`` — every successful
1652+
``ClusterClient.connect()`` pays one extra TCP three-way
1653+
+ handshake RTT to the leader. The single-flight slot
1654+
amortises the probe sweep across N concurrent
1655+
``connect()`` callers, so the cost grows linearly with
1656+
``max_size`` for pool warm-up but not per-acquire when
1657+
probes overlap. Operators sizing ``attempt_timeout`` on
1658+
high-RTT WAN deployments should budget for the extra
1659+
round trip.
1660+
16111661
Args:
16121662
close_timeout: Budget (seconds) for the transport-drain
16131663
during ``close()``. After ``writer.close()`` the

‎src/dqliteclient/protocol.py‎

Lines changed: 76 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -302,12 +302,49 @@ async def handshake(self, client_id: int | None = None) -> int:
302302
lighter :meth:`negotiate_protocol_only` instead.
303303
"""
304304
if client_id is None:
305-
# Deliberate divergence from go-dqlite: Go leaves the default
306-
# client_id; we randomize so each connection is distinguishable
307-
# in server logs, traces, and per-client metrics. 63 bits
308-
# avoids sign-extension pitfalls if an intermediate layer
309-
# treats the id as int64. The ``or 1`` guards against the
310-
# (astronomically unlikely) all-zero draw.
305+
# Deliberate divergence from go-dqlite. Go declares
306+
# ``Connector.clientID uint64`` (``connector.go:75-83``)
307+
# default-zero and never assigns; every Go connection
308+
# registers with ``id=0``. The server-side
309+
# ``handle_client`` (``gateway.c:300-309``) stores
310+
# ``g->client_id = request.id`` verbatim — used only
311+
# for ``tracef`` records and per-connection
312+
# ``handle_interrupt`` keying, neither of which requires
313+
# a unique id per connection.
314+
#
315+
# Python randomises so each connection is distinguishable
316+
# in server logs, traces, and per-client metrics — useful
317+
# for operators tailing a busy server and filtering by
318+
# connection id. 63 bits avoids sign-extension pitfalls
319+
# if an intermediate layer treats the id as int64. The
320+
# ``or 1`` guards against the astronomically unlikely
321+
# all-zero draw — preserved deliberately so an
322+
# operator's ``client_id != 0`` filter is reliable.
323+
#
324+
# Operational trade-offs:
325+
#
326+
# * Mixed-client clusters: an operator filtering server
327+
# logs by ``client_id`` sees Python connections with
328+
# random 63-bit ids while Go connections all show id 0;
329+
# the same filter cannot apply uniformly across both
330+
# client implementations.
331+
# * No cross-reconnect correlation: a Python client that
332+
# loses connection and reconnects gets a FRESH random
333+
# id, so server-side per-client metrics see one
334+
# connection per attempt rather than one client with
335+
# several attempts. Go's id=0 doesn't solve this either,
336+
# but the current Python design forecloses a future
337+
# operator-supplied stable id without an API change.
338+
# * ``secrets.randbits`` (CSPRNG-backed via
339+
# ``getrandom(2)`` on Linux) is the choice over the
340+
# faster ``random.getrandbits`` because the
341+
# ``random._inst`` PRNG state is process-global and
342+
# not fork-aware (see ``retry._retry_random`` and
343+
# ``cluster._cluster_random`` for the same reasoning
344+
# at sibling sites). The per-handshake cost is
345+
# sub-microsecond and negligible against handshake
346+
# RTT; the fork-safety property is the load-bearing
347+
# reason for this choice.
311348
client_id = secrets.randbits(63) or 1
312349
# Send protocol version + client registration together
313350
request = ClientRequest(client_id=client_id)
@@ -1183,15 +1220,41 @@ def _operation_deadline(self) -> float:
11831220
order of ``2 × timeout`` (send + read+drain), and a fresh
11841221
``connect → query`` flow can stack ``handshake`` +
11851222
``open_database`` + ``query_sql`` for proportionally more.
1186-
Callers needing an absolute end-to-end bound should wrap the
1187-
outer call in ``asyncio.timeout`` / ``asyncio.wait_for``.
1223+
Concrete worst-case quantification: a continuation-paginated
1224+
result with N frames pays N × ``timeout`` because each
1225+
continuation frame consumes its own per-read deadline (no
1226+
cumulative budget bounds the read sequence). With
1227+
``timeout=10s`` and a 10-frame continuation, end-to-end
1228+
wall-clock can reach ~120 s (send + read + 10 × continuation
1229+
read) while every individual phase stayed within the
1230+
operator's configured budget. Callers needing an absolute
1231+
end-to-end bound should wrap the outer call in
1232+
``asyncio.timeout`` / ``asyncio.wait_for``.
1233+
1234+
Server-heartbeat amplification: when
1235+
``trust_server_heartbeat=True`` widens ``_read_timeout`` up
1236+
to the 300 s hard cap, the PER-PHASE budget grows; the
1237+
end-to-end multiplier compounds. A 5-phase RPC with the
1238+
server advertising the cap can stack 5 × 300 s = 1500 s of
1239+
read time even when the operator set ``timeout=10``. The
1240+
cap is per-phase, not cumulative — opting into the widening
1241+
is opting out of the operator's per-RPC SLO at the
1242+
cumulative level. Operators whose ``timeout`` is a
1243+
latency-SLO boundary should NOT opt in (the default is
1244+
``False``).
11881245
11891246
Note: this differs from go-dqlite, which uses an absolute
1190-
per-call deadline (``conn.SetDeadline(ctx.Deadline())``). The
1191-
per-phase shape here is a deliberate Python-side choice — the
1192-
asyncio.wait_for wraps individual sends and reads independently
1193-
— and is documented at :class:`DqliteConnection` for caller
1194-
guidance.
1247+
per-call deadline (``conn.SetDeadline(ctx.Deadline())`` at
1248+
``connector.go:312-333`` — applied to BOTH the
1249+
``conn.Write`` and any subsequent ``conn.Read`` until
1250+
cleared). The per-phase shape here is a deliberate
1251+
Python-side choice — the asyncio.wait_for wraps individual
1252+
sends and reads independently — and is documented at
1253+
:class:`DqliteConnection` for caller guidance. The
1254+
:class:`DqliteConnection` and :class:`ConnectionPool`
1255+
docstrings echo the worst-case multiplier so operators
1256+
sizing ``timeout`` for SLO-bound queries see the same shape
1257+
at every entry point.
11951258
"""
11961259
return asyncio.get_running_loop().time() + self._read_timeout
11971260

Lines changed: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,62 @@
1+
"""Pin: an outer cancel landing BEFORE
2+
``ClusterClient.connect``'s first attempt runs propagates through
3+
asyncio cleanly — no per-attempt log breadcrumb fires (because the
4+
inner ``try_connect`` never ran), the cancel propagates as the
5+
canonical Python idiom.
6+
7+
This is the structural divergence from go-dqlite's
8+
``Connector.Connect`` which checks ``ctx.Done()`` only at
9+
``attempt > 1`` and lets the FIRST attempt always run. Python's
10+
asyncio cancel propagates through every ``await`` checkpoint —
11+
the CORRECT shape for the Python ecosystem.
12+
13+
The pin trips if a future change adds a CancelledError catch in
14+
``try_connect`` to synthesise a breadcrumb (which would violate
15+
structured-concurrency cancel propagation).
16+
"""
17+
18+
from __future__ import annotations
19+
20+
import asyncio
21+
import logging
22+
23+
import pytest
24+
25+
from dqliteclient.cluster import ClusterClient
26+
from dqliteclient.node_store import MemoryNodeStore
27+
28+
29+
@pytest.mark.asyncio
30+
async def test_outer_cancel_before_first_attempt_does_not_log_breadcrumb(
31+
caplog: pytest.LogCaptureFixture,
32+
) -> None:
33+
"""``asyncio.timeout(0)`` wrapping ``cluster.connect()`` lands
34+
the cancel BEFORE any ``try_connect`` invocation. The
35+
per-attempt DEBUG breadcrumb at ``cluster.py`` must NOT fire,
36+
confirming Python's structured-concurrency cancel propagation
37+
is preserved (no defensive CancelledError catch synthesises
38+
a "we tried once" breadcrumb)."""
39+
cluster = ClusterClient(MemoryNodeStore(["10.0.0.1:9001"]))
40+
41+
caplog.set_level(logging.DEBUG, logger="dqliteclient.cluster")
42+
43+
with pytest.raises((TimeoutError, asyncio.CancelledError)):
44+
async with asyncio.timeout(0):
45+
await cluster.connect()
46+
47+
# The per-attempt log emitter at try_connect lives inside the
48+
# attempt body; if the cancel landed before the body ran, the
49+
# log should NOT contain the per-attempt phrase. The exact
50+
# substring "attempt" is too broad (it might appear in other
51+
# cluster-side log messages); pin instead on the cluster.connect
52+
# attempt-failed breadcrumb specifically.
53+
breadcrumb_phrases = ("attempt failed", "Connection attempt")
54+
matching = [
55+
r for r in caplog.records if any(phrase in r.getMessage() for phrase in breadcrumb_phrases)
56+
]
57+
assert not matching, (
58+
"Outer cancel-before-first-attempt produced per-attempt log records "
59+
f"({len(matching)}); a defensive CancelledError catch in try_connect "
60+
"is suppressing the structured-concurrency cancel contract. "
61+
"Records: " + " | ".join(r.getMessage() for r in matching)
62+
)
Lines changed: 77 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,77 @@
1+
"""Pins for permanent documentation notes describing structural
2+
divergences from go-dqlite. Each pin is a substring fence on a
3+
docstring or in-source comment; a future refactor that drops the
4+
note trips the pin so the reviewer keeps the rationale in place.
5+
6+
The pins cover four divergences:
7+
8+
1. ``ClusterClient.connect`` — cancel propagation through asyncio
9+
vs go's ``ctx.Done()`` check at iteration tops.
10+
2. ``ClusterClient.connect`` and ``find_leader`` — double-dial
11+
structural cost (Python returns an address; Go adopts the
12+
probe socket).
13+
3. ``DqliteProtocol.handshake`` — deliberate randomised
14+
``client_id`` divergence from go-dqlite's default-zero.
15+
4. ``DqliteProtocol._operation_deadline`` — per-phase timeout
16+
vs Go's absolute per-call ``conn.SetDeadline`` deadline.
17+
"""
18+
19+
from __future__ import annotations
20+
21+
import inspect
22+
23+
from dqliteclient.cluster import ClusterClient
24+
from dqliteclient.protocol import DqliteProtocol
25+
26+
27+
def test_cluster_connect_documents_cancel_propagation_divergence() -> None:
28+
doc = ClusterClient.connect.__doc__ or ""
29+
assert "Comparison to go-dqlite" in doc
30+
assert "Cancel-propagation discipline" in doc
31+
# The "first attempt always runs" Go behaviour is the load-bearing
32+
# contrast point.
33+
assert "first attempt" in doc.lower()
34+
35+
36+
def test_cluster_connect_documents_double_dial_cost() -> None:
37+
doc = ClusterClient.connect.__doc__ or ""
38+
assert "double-dial" in doc.lower() or "extra TCP three-way" in doc
39+
# Operators reading this note must understand they pay one extra
40+
# RTT per successful connect on the leader.
41+
assert "extra" in doc.lower() and "round trip" in doc.lower()
42+
43+
44+
def test_find_leader_documents_address_only_return() -> None:
45+
"""``find_leader``'s docstring documents that it returns an
46+
address (not an open protocol), so high-level callers re-dial."""
47+
doc = ClusterClient.find_leader.__doc__ or ""
48+
assert "address" in doc.lower()
49+
assert "re-dial" in doc.lower() or "extra TCP three-way" in doc
50+
51+
52+
def test_handshake_documents_client_id_divergence_rationale() -> None:
53+
"""The source comment at the randomisation site documents the
54+
Go-divergence rationale: Go is default-zero, Python randomises
55+
for log distinguishability, the cost / fork-safety trade-off
56+
against ``random.getrandbits`` lives in the comment."""
57+
src = inspect.getsource(DqliteProtocol.handshake)
58+
assert "Deliberate divergence from go-dqlite" in src
59+
# The operational trade-offs the comment must call out:
60+
assert "Mixed-client clusters" in src
61+
assert "secrets.randbits" in src
62+
# Cross-references to sibling SystemRandom sites for symmetry.
63+
assert "_retry_random" in src or "_cluster_random" in src
64+
65+
66+
def test_operation_deadline_documents_per_phase_worst_case_quantified() -> None:
67+
"""The ``_operation_deadline`` docstring documents the per-phase
68+
vs Go-absolute-deadline divergence AND the quantitative
69+
worst-case multiplier."""
70+
src = inspect.getsource(DqliteProtocol._operation_deadline)
71+
# Quantitative example (the 10-frame / 120s shape).
72+
assert "10-frame" in src or "10 × continuation" in src.replace("times", "×")
73+
assert "120" in src
74+
# The trust_server_heartbeat amplification call-out.
75+
assert "300 s" in src or "300s" in src
76+
# The Go contrast remains.
77+
assert "differs from go-dqlite" in src

0 commit comments

Comments
 (0)