Conversation
|
@salty-hambot review |
There was a problem hiding this comment.
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
|
@salty-hambot review |
sfishel18
left a comment
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
| self.__shutdown_owner = threading.get_ident() | ||
| try: | ||
| abort_connection(self.__ws) | ||
| except OSError: |
There was a problem hiding this comment.
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() |
There was a problem hiding this comment.
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
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.