Send heartbeats while the caller consumes fetched rows - #632
Open
hashhar wants to merge 5 commits into
Open
Conversation
hashhar
force-pushed
the
hashhar/463-query-stuck-finishing
branch
from
August 19, 2026 21:40
2cd7679 to
904d56e
Compare
hashhar
changed the title
hashhar
force-pushed
the
hashhar/463-query-stuck-finishing
branch
from
August 20, 2026 13:27
459267d to
46d44b8
Compare
Before this change the client made no requests between `fetch()` calls. A caller holding a batch of rows for longer than `query.client.timeout` got the query abandoned by the coordinator. A heartbeat existed but it ran only while a spooled segment was being downloaded. `TrinoQuery` now runs the heartbeat between `fetch()` calls. That covers segment downloads too, so the per-download heartbeat is removed. The heartbeat stops on completion and on `cancel()`. Its loop also exits through a weakref check once the `TrinoQuery` is garbage collected - to handle a caller that abandons an unclosed cursor. `_RequestHeartbeat` exposes `start()` and `stop()` in place of the context manager now. `TrinoRequest.heartbeat()` is now the only place that constructs one so `SegmentIterator` no longer takes `request` and `heartbeat_interval`.
The tests started a heartbeat, slept for a fixed 0.1s and then asserted how many HEAD requests a mock had recorded. The counts depend on how the machine schedules the heartbeat thread within that window so a loaded machine can fail them. Serve scripted HEAD responses from a recording request instead. Each response releases a semaphore so a test waits for the heartbeats it needs and then joins the thread. This also removes the fixture that patched MAX_FAILURES, the scripted responses set up each case directly.
The tests exercise `_RequestHeartbeat` directly and touch nothing related to spooling. They lived in `test_client_spooling.py` because the heartbeat used to run only during spooled segment downloads.
Before this change `stop()` set the stop event and returned. The loop reads that event only in its `wait()` call so the following interleaving was possible: - the heartbeat thread returns from `_stop_event.wait()`, event unset - `fetch()` calls `stop()`, which sets the event and returns - the heartbeat thread sends its HEAD, it does not read the event again - `fetch()` sends its GET The HEAD and the GET then run at once on one `requests.Session` which is not thread safe. `stop()` now joins the heartbeat thread. The race is older than the query level heartbeat. The context manager that `_RequestHeartbeat` used to expose did not join either so a spooled segment download could overlap the `fetch()` that followed it.
hashhar
force-pushed
the
hashhar/463-query-stuck-finishing
branch
from
August 20, 2026 21:46
46d44b8 to
fb4e162
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Fixes #463.
The client makes no requests between
fetch()calls. A caller that holds a batch of rows for longer thanquery.client.timeoutgets the query abandoned by the coordinator and the next fetch fails.TrinoQuerynow runs a heartbeat betweenfetch()calls.fetch()stops it on entry and starts it again before returning so a heartbeat never overlaps a request on the same HTTP session.execute()starts one as well when the first response already carries rows and no fetch has run yet. The heartbeat stops on completion and oncancel(). Its loop also exits through a weakref check once theTrinoQueryis garbage collected so a caller that abandons a cursor without closing it does not leak the thread.heartbeat_interval(default 30s) controls the interval andNonedisables it. Servers older than Trino 475 do not support HEAD on the statement endpoint so they keep the old abandonment behavior.A heartbeat already ran while a spooled segment was downloading. The query-level one covers that too so the per-download wiring is gone and
SegmentIteratorno longer takesrequestandheartbeat_interval. Stopping a heartbeat now joins its thread which closes a pre-existing race where an in-flight HEAD could overlap the request that followed it.Non-technical explanation
The client pings the coordinator between fetches, so slow consumers no longer fail with "Query was abandoned by the client".
Release notes
( ) This is not user-visible or docs only and no release notes are required.
(x) Release notes are required, please propose a release note for me.
( ) Release notes are required, with the following suggested text: