Conversation
Member
Author
|
@salty-hambot review |
![salty-hambot[bot]](https://avatars.githubusercontent.com/in/2952321?s=60&v=4){kind=small}
There was a problem hiding this comment.
Reviewed by Salty Hambot 🤖🧂 — rubric mode
Verdict: ✅ pass
| Dimension | Verdict | Notes |
|---|---|---|
| correctness | ✅ pass | Decode/interpret/retrieve failures are query-local; the send-gate race Reviewer B flagged is self-healing and doesn't strand queries. |
| security | ✅ pass | SQL redaction preserved and error text reports only exception type, so raw payloads/SQL don't leak. |
| privacy | ✅ pass | Tests assert 'private' payloads never surface in error text or caplog across decode/malformed paths. |
| reliability | ✅ pass | Transport loss, idle timeouts, decode failures, and close races each 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 | Locking rationale is well-documented and __receive_loop/__fail_pending/__terminate_transport are cleanly separated. |
| test quality | ✅ pass | New tests cover decode/malformed/invalid-state failures, send-failure untracking, and the close/decode race delivering only one outcome. |
Sixth time's the charm — all six prior threads verifiably resolved, Reviewer A came up empty, and Reviewer B's lone 'airlock slip' is a benign self-healing race where the escapee just fails on the same dead socket. Ship it.
0 finding(s) posted · 1 filtered as false positives.
💰 Review cost: $1.8769 · 505.7k in / 21.1k out tokens · ⏱️ 3m57.8s
💬 To request a re-review, comment @salty-hambot review
Member
Author
|
@salty-hambot review |
sfishel18
reviewed
Sep 21, 2026
sfishel18
left a comment
sfishel18
left a comment
Contributor
There was a problem hiding this comment.
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
This was referenced Sep 21, 2026
Closed
rbavery
marked this pull request as ready for review
September 21, 2026 20:57
sfishel18
reviewed
Sep 21, 2026
…ails __fail_pending latched __closed before calling abort_connection, but only OSError was caught and the try/finally that sets __shutdown_done started below it. Any other exception from the abort skipped the delivery loop, and since __closed is a one-way latch every later __fail_pending early-returned: pending queries were never failed, __shutdown_done never set, and fetchall() blocked forever -- the hang this PR exists to remove. Run everything after the latch under one try/finally, and make transport termination catch every exception so delivery is never skipped.
Every close() routed through abort_connection, which shuts the socket down without sending a close frame -- even on a healthy `with connect()` exit. The server could not distinguish a client exit from a crash and had to fall back to its idle timeout to reclaim the session. The unconditional abort exists because ws.close() takes the library's protocol mutex, which a stalled sendall() may hold. That is only true when a send is actually in flight, and __send_lock tells us: a non-blocking acquire succeeds only when no sender is active, in which case close() performs the handshake; otherwise, and on reader-side failures where the transport is already broken, we abort as before. The handshake is bounded by close_timeout on the connection.
Contributor
|
@salty-hambot review |
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
2 participants
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.
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
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.