Repository navigation
Conversation
The driver trusts a single pushed state_updated per query. When that frame is lost on a healthy socket, the query waits forever (2026-09-17 staging MCP canary: the session logged success, the driver never saw it). Add a bounded watchdog on the reader thread. Any inbound event for an execution, progress included, resets that query's clock. After N seconds of silence it re-sends retrieve_results as a probe, backing off N, 2N, 4N, then 8N. Replies to a retrieve reset the clock but not the backoff, so a long, silent query isn't re-asked every N seconds. The check runs on every reader-loop iteration (gated to at most once per min(1s, N)), so steady traffic for one query can't starve another's watchdog. When the watchdog is on, recv() is capped at that check interval, so a read_timeout of None or a long one can't park the reader past a due probe. Probes never wait for the send lock: if a user thread is mid-send, maybe stalled, the probe is skipped until the next check instead of stopping recv() for every query on the connection. Probed states: EXECUTION_REQUESTED, PENDING, RUNNING. A query whose results were already requested gets one probe, after 4N: the session re-serializes and re-sends the full result for every retrieve, so duplicates are costly. Duplicate replies are dropped by the existing claim in complete_query. Add the session's PENDING state, so a probe reply can never fail a healthy query as an unknown state. sql-session's execution cache is a small LRU that can evict a query that is still running; its terminal state_updated still arrives. So a probe's "Execution not found" doesn't fail the query: it stops probing it and keeps waiting. After the normal retrieve, not-found still fails it. New additive kwarg stale_query_probe_seconds (default 30.0) on connect(), connect_direct() and Connection. None disables the watchdog; an invalid value disables it with a warning and never raises.
…-1179) A store-only execution has no cached Arrow table, so the session answers retrieve_results for it with succeeded and no results. Sessions with the probe contract (sql-session step 16) also echo the top-level result_uri and size, as state_updated carries them. On an execution_result with a result_uri, complete with that StoreResult, as the state_updated path does. The normal path never requests results for a store-configured query, so such a reply can only answer a probe. Succeeded without a result_uri on a store query is an older session or an empty store result, which can't be told apart. Log a warning and keep waiting rather than complete empty, which would silently drop an old session's real result, and stop probing it. Known limitation: an empty store result whose terminal event was lost waits as it does today.
…(WBC-1179) The probe log reported silence since the last probe, not the last event: each probe resets last_activity to space the next one, so once probes go unanswered (the case worth diagnosing) it under-reported, e.g. 60s for a real 90s. Track the last inbound event separately in _Watch.last_event (set on registration and inbound messages only) and log that, noting when the previous probe went unanswered. Probe timing is unchanged. The invalid stale_query_probe_seconds warning logged only the type name, so 0 read as "(int)". Log a truncated repr, falling back to the type name if repr raises, since this runs in the constructor. Reword the eviction-guard comment: the terminal event still arrives only if the query is still running; if it was already lost, nothing more can be done.
Contributor
Author
|
@salty-hambot review |
![salty-hambot[bot]](https://avatars.githubusercontent.com/in/2952321?s=60&v=4){kind=small}
A probe for an evicted queued or running query can be followed by a genuine state_updated: running before the probe's "Execution not found" reply. Clearing probe_outstanding on that update made the reply look like an answer to a normal retrieve, so it bypassed the eviction guard and failed a query that could still complete. Only replies to a retrieve (execution_result/error) clear the flag now. The not-found cases that must still fail are gated by results_requested, which the normal retrieve sets after a terminal success.
A probe runs on the reader thread. Its non-blocking acquire of the send lock only avoids waiting behind another sender; its own ws.send() can still block when the socket's send buffer is full, e.g. a peer that keeps sending but stops reading. The reader then stops processing every query, and keepalive can't help: the stalled sendall() holds websockets' protocol mutex, which pings need too. On the non-blocking path, check the socket is writable first and treat a full one like a busy lock: skip, and retry at the next check. The check uses poll (or a selector), so fds above FD_SETSIZE work, and never raises: anything unusable, including test fakes, counts as writable.
Contributor
Author
|
@salty-hambot review |
sfishel18
marked this pull request as ready for review
October 9, 2026 19:20
This branch has not been deployed
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.
Summary
On 2026-09-17 the staging MCP canary waited out its full 900s budget even though the SQL session had finished the query. The session logged success and sent the terminal
state_updated, but the driver never received it, on a healthy socket. The driver trusts that one pushed event and never asks again, so the cursor waited until the caller gave up.This adds a watchdog. When a query goes
stale_query_probe_seconds(default 30s) without any message from the session, the driver sends aretrieve_resultsprobe. Repeat probes back off to 8× the interval. The reply recovers the query: results for a normal query, or the result location (result_uri) for a store query. PassNoneto disable it.Behaviour worth knowing:
result_uriecho in the probe reply ships in 1.9.3 (wherobots/sql-session#208). Against an older session, a store query's probe reply carries no location, so the watchdog keeps waiting rather than complete the query with no results. That is the same as today, never worse. An empty store result whose terminal event was lost looks identical, so it waits too.Execution not found. The driver treats this as "can't probe this one", not as a failure, and stops probing it. If the query is still running, its terminal event still arrives as normal.stale_query_probe_secondskwarg onconnect()/connect_direct(), andExecutionState.PENDING.Related Issues
Relates to WBC-1179 (canary run https://github.com/wherobots/studio-backend/actions/runs/35281752422). Builds on #74. Server side: wherobots/sql-session#208.
Requester Checklist
Complete these before marking Ready for Review
Visual Proof
uv run pytest -q: 225 passed.pre-commit run --all-files: all hooks pass. The watchdog test file passed 5/5 consecutive runs.The new tests were checked to fail against
main, and the key ones were also checked by breaking the behaviour they cover:result_uri→ the keep-waiting test fails;Size Justification (if L/XL)
The size comes mostly from tests: source and docs are +308/−30, tests are +608/−2. Nearly all the source change is in
wherobots/db/connection.py. The PR is split into two commits that review independently:result_uri.Reviewer Checklist
If these are not met, close the tab — this PR is not ready for review