Skip to content

Commit 70af97d

Browse files
Make trust_server_heartbeat read-timeout widening actually flow through to operation deadline
``DqliteProtocol._operation_deadline()`` keyed off ``self._timeout`` (the un-widened write-path budget) while ``_read_data`` then capped each chunk read at ``min(remaining, self._read_timeout)``. Since the per-read cap was always ``>= remaining`` after the deadline calculation, the heartbeat-trust widening of ``_read_timeout`` had NO effect on the actual deadline budget — the documented ``trust_server_heartbeat=True`` opt-in was dead code on the read deadline path. Point ``_operation_deadline()`` at ``self._read_timeout`` so the widening flows through. Write-path ``_send`` continues to use ``self._timeout`` directly — only the read side gets heartbeat extension. Add a positive test pinning the now-real widening: a read that takes longer than ``_timeout`` but less than the widened ``_read_timeout`` completes cleanly. Updates the sibling deadline test to set both ``_timeout`` and ``_read_timeout`` so the deadline reflects the test's intended budget. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 991d02d commit 70af97d

2 files changed

Lines changed: 60 additions & 2 deletions

File tree

src/dqliteclient/protocol.py

Lines changed: 18 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -739,7 +739,23 @@ def _failure_text(self, response: FailureResponse) -> str:
739739
return _failure_message(response.message, self._addr_suffix())
740740

741741
def _operation_deadline(self) -> float:
742-
"""Deadline (monotonic seconds) for a single protocol operation.
742+
"""Deadline (monotonic seconds) for a single read-side protocol
743+
operation.
744+
745+
Uses ``self._read_timeout`` (NOT ``self._timeout``) so the
746+
``trust_server_heartbeat`` widening — which raises
747+
``_read_timeout`` up to the server-advertised heartbeat
748+
capped at ``_HEARTBEAT_READ_TIMEOUT_CAP_SECONDS`` — actually
749+
flows through to the per-read deadline budget. Without this,
750+
the widening was dead code: ``_read_data``'s
751+
``min(remaining, self._read_timeout)`` always selected
752+
``remaining`` because the deadline came from
753+
``self._timeout`` (un-widened), so the user-configured
754+
opt-in had no effect.
755+
756+
Write-path ``_send`` continues to use ``self._timeout``
757+
directly (line ~652) — that's intentional: only the read
758+
side gets the heartbeat widening.
743759
744760
Note on multi-phase RPCs: ``timeout`` bounds each phase
745761
independently — a phase-1 ``_send`` followed by a phase-2
@@ -753,7 +769,7 @@ def _operation_deadline(self) -> float:
753769
outer call in ``asyncio.timeout`` / ``asyncio.wait_for``.
754770
This matches go-dqlite's per-phase budgeting.
755771
"""
756-
return asyncio.get_running_loop().time() + self._timeout
772+
return asyncio.get_running_loop().time() + self._read_timeout
757773

758774
async def _read_continuation(self, deadline: float | None = None) -> RowsResponse:
759775
"""Read and decode a ROWS continuation frame.

tests/test_protocol.py

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -139,7 +139,12 @@ async def test_read_response_enforces_operation_deadline(
139139
import asyncio
140140
import time
141141

142+
# Set both the write-path timeout AND the read-path timeout
143+
# so the operation deadline (which keys off ``_read_timeout``
144+
# so the heartbeat widening flows through) reflects the
145+
# tightened budget.
142146
protocol._timeout = 0.2
147+
protocol._read_timeout = 0.2
143148

144149
async def drip_forever(_n: int) -> bytes:
145150
await asyncio.sleep(0.1)
@@ -155,6 +160,43 @@ async def drip_forever(_n: int) -> bytes:
155160
f"_read_response must bail at the operation deadline; took {elapsed:.3f}s"
156161
)
157162

163+
async def test_read_timeout_widening_actually_extends_operation_deadline(
164+
self,
165+
protocol: DqliteProtocol,
166+
mock_reader: AsyncMock,
167+
) -> None:
168+
"""Pin: setting ``_read_timeout`` (e.g. via the
169+
``trust_server_heartbeat`` opt-in widening) genuinely extends
170+
the per-operation read deadline; the widening is not dead
171+
code. A read that takes longer than ``_timeout`` but less
172+
than the widened ``_read_timeout`` must complete cleanly.
173+
"""
174+
import asyncio
175+
176+
from dqlitewire.messages import EmptyResponse
177+
178+
protocol._timeout = 0.05 # tight write-side
179+
protocol._read_timeout = 5.0 # widened read-side
180+
body = EmptyResponse().encode()
181+
182+
sent = [False]
183+
184+
async def slow_first_read(_n: int) -> bytes:
185+
# First call takes longer than _timeout but well within
186+
# _read_timeout; deliver the full response so the
187+
# decoder completes.
188+
if not sent[0]:
189+
sent[0] = True
190+
await asyncio.sleep(0.2)
191+
return body
192+
return b""
193+
194+
mock_reader.read.side_effect = slow_first_read
195+
196+
# Must NOT raise: the widening flowed through into the
197+
# deadline budget. Pre-fix this raised TimeoutError at 0.05s.
198+
await protocol._read_response()
199+
158200
async def test_read_data_deadline_already_in_past_raises_immediately(
159201
self,
160202
protocol: DqliteProtocol,

0 commit comments

Comments
 (0)