fix(plugins): cancel every in-flight request in the D1, libSQL and Trino HTTP clients - #2736
Merged
Merged
Conversation
…ino HTTP clients Claude-Session: https://claude.ai/code/session_01JKFSBk6YwDemnkbQnyc2xz
Signed-off-by: Ngô Quốc Đạt <datlechin@gmail.com>
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
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.
Stop on a Cloudflare D1, libSQL (Hrana over HTTP) or Trino connection cancelled whichever HTTP request started last. That was often a sidebar or autocomplete read, not the query the user stopped. Cancelling the Swift task that awaited a request did not reach the request either. The Cloudflare R2 SQL rewrite in #2030 found this same defect in its own transport. This PR fixes the three clients that shared it.
Cause
currentTaskslot, and every send overwrote it.cancelQuerycancelled only the latest request. A request that finished cleared the slot even when it held someone else's task, so the next Stop cancelled nothing. ThewithTaskCancellationHandlercancelled the slot as well, so cancelling one Task could cancel a different request.dataTaskand a continuation with no cancellation handler, so Task cancellation never reached the socket. The single slot was inTrinoStatementClient(_cancelled/_currentNextUri). Every new statement reset both fields, so a sidebar read that started mid-query wiped the query's cancel flag and itsnextUri. When that read finished, the nextcancel()sent no DELETE at all.Fix
All three transports now use
URLSession.data(for:delegate:). A per-requestURLSessionTaskDelegateregisters its task inurlSession(_:didCreateTask:), and a lock-protected in-flight set holds every request until itsdeferremoves it.cancelAll()cancels each one. Cancelling the awaiting Task cancels its own URL task, whichdata(for:)does natively.cancelCurrentTask()becomescancelAll(), and each driver'scancelQuerycalls it. Disconnect still cancels everything throughinvalidateAndCancel(). A cancelled request now throwsCancellationError()instead of a rawURLError -999, whichDatabaseCancellationDiagnosis.isCancellationalready recognises. So a sidebar read that Stop cancels reads as a cancellation, not a failure. libSQL's localsqlite3_interruptpath is untouched.cancelAll()is added to theTrinoTransportprotocol. Adeinitnow invalidates the session, which was leaked on every connect before. A DELETE is never tracked, because it is how a statement tells Trino to stop, and a second Stop must not cancel the first Stop's DELETE.nextUriand a claim-once release.cancel()flags every running statement, callstransport.cancelAll(), and sends one DELETE per statement. A statement that ends by cancellation, throughcancel()or through its Task, sends its own DELETE, becausedata(for:)now throws before the loop's cancellation check would run.Cancelling the initial POST is safe. Trino's
QueuedStatementResourceonly dispatches a query on the first GET of itsnextUri(submitIfNeeded), and purges one that is never polled. So a POST cancelled mid-flight never starts on the server.Timeouts (
HttpQueryTimeoutBox), auth headers, HTTP error mapping, Trino's retry and backoff, session headers and the paging loop are unchanged.Verification
TrinoURLSessionTransportTests(new, 6 cases against aURLProtocolstub):cancelAllcancelling two concurrent sends;cancelAllsparing a DELETE;cancel()stopping two running statements while a third starts and finishes in between, with one DELETE for each of the two;testCancelStopsPollingalso checks thatcancelAllwas called.TrinoStatementClientbehind the new tests, the concurrent-statements test fails (requests run to the 60 s timeout, wrong DELETEs), and so does the Task-cancel test (no DELETE sent).D1HttpClientCancellationTestsandHranaHttpClientCancellationTests(new, 5 cases each): round trip with token and timeout,cancelAllcancelling two concurrent requests, a finished request leaving the others cancellable, Task cancellation, and disconnect. Both client files compile on their own, soproject.ymladds them to the TableProTests sources. 36 of 36 pass with the existing D1 suites.CloudflareD1DriverPlugin,LibSQLDriverPluginandTrinoDriverPlugineach build with no warnings in the touched files. TheAllPluginsaggregate was not run: it fails locally in the third-partyOracleNIOcheckout under the Xcode beta toolchain, in code this PR does not touch.swiftlint --stricton the 11 changed Swift files: 0 violations. That includes four findings that were already on untouched lines of edited files: two implicit returns, one unusedenumerated()and one import order.Not covered by tests: TLS client certificates and CA pinning. The per-request delegate implements no challenge methods, so challenges still go to
TrinoTLSDelegate, but theURLProtocolstub cannot exercise that. A response that lands after a DELETE can still surface as Trino'sUSER_CANCELEDquery error rather than.cancelled, as before.No UI automation: registry plugins never load in the UI test host, and there is no D1, Turso or Trino server to talk to.
After merge, these need plugin releases:
plugin-cloudflare-d1,plugin-libsql,plugin-trino.https://claude.ai/code/session_01JKFSBk6YwDemnkbQnyc2xz