Skip to content

fix(dbapi): fail pending queries on SQL connection loss - #74

Open
rbavery wants to merge 5 commits into
wherobots:mainfrom
rbavery:codex/wbc-1051-disconnect
Open

rbavery wants to merge 5 commits into
wherobots:mainfrom
rbavery:codex/wbc-1051-disconnect

Conversation

@rbavery

@rbavery rbavery commented Sep 5, 2026

Copy link
Copy Markdown
Member

What/Why

Fix WBC-1051: when the SQL WebSocket disappears, pending cursors must receive an error instead of waiting indefinitely. A stalled send must not hold the query-state lock needed for result delivery and shutdown. Connection loss leaves server-side write outcomes uncertain; no SQL is automatically retried.

How

Keep admission and terminal-result ownership under a short state lock; serialize requests before registration and use a separate gate for all driver sends. Shutdown stops admission, shuts down/closes the owned socket without waiting for the WebSocket protocol's send mutex, then claims pending queries and delivers errors. This intentionally aborts the transport; it does not promise rollback or a graceful WebSocket handshake. Already-delivered results remain intact.

Retry ordinary receive timeouts, handle confirmed transport failures separately from local serialization/API errors, and clear stale cursor execution IDs after rejected submissions. Explicit close waits up to one second for reader cleanup and avoids self-join. Session/execution IDs remain in errors, with normalized session-ID extraction.

Complete query-local decoding and result-request failures with one OperationalError for the affected cursor, without closing a healthy connection. Malformed result objects/state fields and unsupported formats cannot silently strand the identified execution. Errors include correlation IDs but not payload contents or arbitrary decoder exception text; normal completion and concurrent shutdown still compete through the same terminal-ownership claim.

HTTP diagnostics are removed from this PR and isolated in the stacked diagnostics draft. This core fix has no dependency on the companion backend rollout. The follow-up targets upstream main and depends on this PR; its diff currently includes these prerequisite commits.

Verified

  • The disconnect suite passes locally against the minimum supported websockets==13.0; the same transport design was checked against 16.0 and 17.0. Tests include real local socket backpressure and TLS shutdown, alongside deterministic result/close/send races. These are controlled local fixtures, not a staging or production workload validation.

Co-authored with Codex.

@rbavery

rbavery commented Sep 21, 2026

Copy link
Copy Markdown
Member Author

@salty-hambot review

@salty-hambot salty-hambot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed by Salty Hambot 🤖🧂 — rubric mode

Verdict: ✅ pass

Dimension Verdict Notes
correctness ✅ pass Decode-failure orphan fixed; state transitions and terminal delivery are sound across the changed paths.
security ✅ pass SQL redaction preserved and error text reports only exception type, so raw payloads/SQL don't leak into surfaced errors.
privacy ✅ pass Tests assert 'private' payloads never appear in error text or caplog on decode/malformed paths.
reliability ✅ pass Transport loss, idle timeouts, decode failures, and close races all deliver exactly one terminal outcome per cursor.
scalability ✅ pass Queries are popped on terminal delivery so handlers/results aren't retained for the connection's lifetime (WBC-922).
observability ✅ pass Transport failures and undeliverable handlers log via logging.exception; query-local failures log with session/execution context.
clarity/maintainability ✅ pass Clear locking rationale, separated __receive_loop/__fail_pending, well-documented close() semantics.
test quality ✅ pass New tests cover the decode-failing cursor itself receiving an error and the close/decode race delivering only one outcome.

Clean landing — the Thread 6 decode-orphan fang got pulled and now has real regression teeth (the bad cursor actually gets its OperationalError). All six prior threads resolved; nothing new to bite.

Prior findings: ✅ 6 resolved

0 finding(s) posted.
💰 Review cost: $1.2556 · 276.2k in / 12.7k out tokens · ⏱️ 2m19.9s
💬 To request a re-review, comment @salty-hambot review

Comment thread wherobots/db/connection.py
Comment thread wherobots/db/connection.py Outdated
Comment thread wherobots/db/connection.py
Comment thread wherobots/db/connection.py Outdated
@rbavery

rbavery commented Sep 21, 2026

Copy link
Copy Markdown
Member Author

@salty-hambot review

Comment thread wherobots/db/connection.py

@sfishel18 sfishel18 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

recommend splitting the enrichment changes into their own pr. since they need to be sequenced after the studio-backend changes or they're just a wasted request. but the other changes here are immediately beneficial

Comment thread wherobots/db/connection.py Outdated
Comment thread wherobots/db/connection.py Outdated
Comment thread wherobots/db/connection.py Outdated
Comment thread tests/test_disconnect.py Outdated
Comment thread wherobots/db/driver.py Outdated
Comment thread wherobots/db/driver.py Outdated
Comment thread wherobots/db/connection.py
self.__shutdown_owner = threading.get_ident()
try:
abort_connection(self.__ws)
except OSError:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this only catches OSError, and the try/finally that sets __shutdown_done starts below it — so any other exception escaping abort_connection skips the delivery loop entirely. and because __closed was already latched above, every later __fail_pending early-returns. the connection is then wedged permanently with its queries never failed.

test_abort_closes_socket_even_if_shutdown_errors covers OSError(EIO) and that path is genuinely safe, because you catch it right here. it's only the untested exception types that wedge.

except Exception here would cover it, though moving the finally that sets __shutdown_done up to just after the latch seems more robust: as written, any failure between latching __closed and reaching delivery has this shape, and the latch is what turns it from retryable into permanent.

decoder or callback can outlive the bounded reader join.
"""
deadline = time.monotonic() + 1.0
self.__fail_pending()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

following this down: close() now always routes through __fail_pending into abort_connection, and ws.close() isn't called anywhere in the package any more. so every close is shutdown(SHUT_RDWR) + socket.close() with no websocket close frame — including a completely healthy with connect(...) exit where nothing is stalled. that means the session can't distinguish a graceful client exit from a crashed client and would fall back to idle timeouts.

i see it's tricky because ws.close() takes the protocol mutex a stalled sendall may hold, which is the entire reason the adapter exists. but that's only true when a send is actually in flight, and you already have a way to ask: if self.__send_lock.acquire(blocking=False) succeeds there's no sender to strand, so a graceful ws.close() is safe there and the abort stays as the fallback

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants