fix: isolate cursor results per execution and stop cancelling completed queries (WBC-922)

[james-willis]

Fixes WBC-922

Problem

The cursor violates PEP 249 in two ways when execute() is called again before the previous statement's results are fetched:

1. Result misalignment. Results arrive asynchronously into a single shared queue.Queue, __get_results() pops blindly with no correlation to the current execution, and execute() never drains stale entries. A completed-but-unfetched result therefore gets handed to the next statement's fetchall(), and every subsequent result set comes back shifted by one query. PEP 249 binds fetch methods to the most recent .execute*() call, and no mainstream driver requires draining before re-execute.

2. Silent cancellation of completed/in-flight statements. execute() (and close()) unconditionally cancelled the previous execution — including DML that had already completed, and worse, in-flight writes. An unfetched MERGE followed by another execute() sends a cancel for the write; it only survives if it happened to commit before the cancel lands.

Found while bug bashing the S3 Tables integration: a MERGE INTO executed without a follow-up fetch caused the next SELECT to return the MERGE's empty result, the query after that to return the SELECT's rows, and the MERGE itself to be cancelled mid-flight.

Fix

• Fresh queue per execution: execute() now creates a new queue.Queue and passes queue.put (closing over that specific queue) as the result handler. Late results from a superseded execution land in the orphaned queue and can never be observed by fetches of the current one. This makes correlation structural — no execution-id plumbing needed in ExecutionResult.
• Cancel only in-flight executions: execute() and close() now only send a cancel when the previous execution's result has not yet arrived (__in_flight_execution_id()), preserving supersede semantics for genuinely running queries while never cancelling completed statements.

Tests

Eight new unit tests in tests/test_cursor.py using an async-delivery mock that mimics the connection's callbacks:

• unfetched results don't leak into the next execute
• late results from a superseded execution are ignored
• repeated fetches still return memoized results
• execute cancels an in-flight previous query, but not a completed one
• close cancels an in-flight query, but not a completed one (nor when nothing ran)

uv run pytest tests/ → 111 passed. pre-commit clean.

[salty-hambot]

Reviewed by Salty Hambot 🤖🧂 — rubric mode

Verdict: ❌ fail

📋 Requested evidence

• Confirm the threading model: are execute()/close() and the result-delivery handler guaranteed to run on the same thread/turn, or can the handler put() interleave with the cancellation check?

Dimension	Verdict	Notes
correctness	❌ fail	__results is None misclassifies completed store/empty-result executions as in-flight, reintroducing the very cancellation bug the PR fixes.
security	✅ pass	SQL literal quoting/escaping in _quote_value is sound; no injection surface added.
privacy	✅ pass	No PII handling or logging changes in the diff.
reliability	❌ fail	Non-atomic queue.empty()/cancel across the background reader thread leaves a race that can cancel a just-completed query.
scalability	✅ pass	Per-execution queue allocation is negligible; no scaling concern.
observability	✅ pass	No logging change needed here; connection-level logging is untouched.
clarity/maintainability	⚠️ concerns	Overloading __results is None as the completion signal is subtle and mis-documented; an explicit fetched flag would read clearer.
test quality	⚠️ concerns	Strong DML/SELECT matrix, but no store-result completion test covers the path that actually breaks.

The fresh-queue-per-execution trick is clean, but the completion signal results is None springs a leak on the store-export path — cancels completed queries all over again — plus an unlocked empty()/cancel race across threads. Two blocking issues and a missing store-path test stand between this and the WBC-922 fix it promises.

_{3 finding(s) posted.
💰 Review cost: $0.9300 · 259.0k in / 8.3k out tokens · ⏱️ 1m46s
💬 To request a re-review, comment @salty-hambot review}

[james-willis]

@peterfoldes plz look

[salty-hambot]

Reviewed by Salty Hambot 🤖🧂 — rubric mode

Verdict: ⚠️ concerns

📋 Requested evidence

• Confirm whether completed queries are ever removed from Connection.__queries on the success paths (194/204/220), or share the connection-teardown path that reclaims them — the diff shows a pop only on the CANCELLED branch.

Dimension	Verdict	Notes
correctness	✅ pass	The __complete flag now records all terminal outcomes (rows/store/empty/error), retiring the __results-is-None misclassification that reintroduced the cancel-on-completed bug.
security	✅ pass	No injection surface touched; _quote_value escaping is unchanged and sound.
privacy	✅ pass	No PII handling or logging changes in the diff.
reliability	⚠️ concerns	The empty()/cancel race across the reader thread remains unsynchronized — documented as benign but not prevented (prior thread 2, still open).
scalability	⚠️ concerns	Superseded result queues are retained via handlers pinned in Connection.__queries on success paths, leaking DataFrames for the connection's lifetime under repeated unfetched re-execution.
observability	✅ pass	Connection-level logging is untouched; no observability gap introduced here.
clarity/maintainability	✅ pass	Explicit __complete flag reads far clearer than the overloaded __results-is-None signal it replaces, with a solid docstring on the in-flight predicate.
test quality	✅ pass	New TestCursorCancellation/ResultIsolation cover store, empty, error, and re-execute/close paths — the store-completion gap from review 1 is filled.

The __complete flag lands the WBC-922 fix cleanly and the store/empty-path tests fill the gap — but the fresh-queue-per-execute trick has a hangover: superseded queues stay pinned through their retained handlers in __queries, so unfetched completed queries quietly hoard their result sets. One blocking leak between this and done.

Prior findings: ✅ 2 resolved · ⚠️ 1 still open

_{1 finding(s) posted.
💰 Review cost: $1.2871 · 297.2k in / 6.6k out tokens · ⏱️ 1m38.8s
💬 To request a re-review, comment @salty-hambot review}

[james-willis]

@salty-hambot review

All three findings addressed in d94e192: the __results is None completion signal is replaced with an explicit __complete flag (fixes the store/empty-result cancellation leak + two pre-existing fetch deadlocks), store/empty/error completion paths now have cancellation-gate tests, and the empty()/cancel race is documented with the analysis of why it's benign rather than locked (see inline reply).

[salty-hambot]

Reviewed by Salty Hambot 🤖🧂 — rubric mode

Verdict: ⚠️ concerns

Dimension	Verdict	Notes
correctness	✅ pass	The __complete flag records all terminal outcomes, retiring the __results-is-None misclassification that had reintroduced the cancel-on-completed bug.
security	✅ pass	No injection surface touched; _quote_value escaping is unchanged and sound.
privacy	✅ pass	No PII handling or logging changes in the diff.
reliability	⚠️ concerns	The empty()/cancel race across the reader thread remains unsynchronized — documented as benign but not prevented (prior thread 2, still open).
scalability	⚠️ concerns	Completed queries are never popped from Connection.__queries on success paths, so superseded result queues stay pinned via retained handlers, leaking DataFrames for the connection's lifetime (prior thread 4, still open).
observability	✅ pass	Connection-level logging is untouched; no observability gap introduced.
clarity/maintainability	✅ pass	Explicit __complete flag with its in-flight-predicate docstring reads far clearer than the overloaded __results-is-None signal it replaced.
test quality	✅ pass	TestCursorCancellation/ResultIsolation now cover store, empty, error, and re-execute/close paths — the store-completion gap from review 1 is filled.

Same ship, same course — the __complete fix and store-path tests are still holding, and no new leaks aboard. Threads 2 and 4 remain the only barnacles: the benign empty()/cancel race and the unpopped __queries leak on success paths, both already on the PR and awaiting the author's call.

Prior findings: ✅ 2 resolved · ⚠️ 2 still open

_{0 finding(s) posted.
💰 Review cost: $0.7548 · 208.6k in / 4.3k out tokens · ⏱️ 57.3s
💬 To request a re-review, comment @salty-hambot review}

[james-willis]

@salty-hambot review

Thread 4 (__queries leak) fixed in beddbb1 — terminal deliveries now pop the query from tracking on every path, with connection-level tests for each. That also makes cancel-of-completed a client-side no-op, further narrowing thread 2's race window.

[salty-hambot]

Reviewed by Salty Hambot 🤖🧂 — rubric mode

Verdict: ✅ pass

Dimension	Verdict	Notes
correctness	✅ pass	__complete flag and complete_query helper handle every terminal shape; the cancel-of-completed bug is fully retired.
security	✅ pass	No injection surface touched; _quote_value escaping unchanged and sound.
privacy	✅ pass	No PII handling or logging changes in the diff.
reliability	✅ pass	empty()/cancel race is now a client-side no-op since completed queries are popped before delivery; documented invariant holds.
scalability	✅ pass	Superseded queues are released on every terminal path — the prior DataFrame-hoarding leak is closed.
observability	✅ pass	Connection-level logging untouched; no gap introduced.
clarity/maintainability	✅ pass	Single complete_query helper unifies the pop-before-dispatch logic with a clear WBC-922 rationale.
test quality	✅ pass	New test_connection_query_tracking.py covers all terminal paths plus cancel-of-untracked; cursor suite covers store/empty/error/re-execute.

Clean sweep — the complete_query helper pops before dispatch on every terminal path, closing the WBC-922 leak, and the new tracking suite nails down each path. The benign empty()/cancel race is now genuinely benign (cancel-of-popped is a no-op). Ship it.

_{0 finding(s) posted.
💰 Review cost: $0.8059 · 270.7k in / 6.3k out tokens · ⏱️ 1m24.2s
💬 To request a re-review, comment @salty-hambot review}

[salty-hambot]

Reviewed by Salty Hambot 🤖🧂 — rubric mode

Verdict: ✅ pass

Dimension	Verdict	Notes
correctness	✅ pass	__complete flag plus complete_query helper cover every terminal shape; the cancel-of-completed bug is fully retired.
security	✅ pass	No injection surface touched; _quote_value quoting/escaping unchanged and sound.
privacy	✅ pass	No PII handling or logging changes in the diff; SQL redaction paths untouched.
reliability	✅ pass	empty()/cancel race is now a client-side no-op since completed queries are popped before delivery; invariant documented.
scalability	✅ pass	Superseded queues released on every terminal path via pop-before-dispatch — the prior DataFrame-hoarding leak is closed.
observability	✅ pass	Connection-level logging untouched; no gap introduced.
clarity/maintainability	✅ pass	Single complete_query helper unifies pop-before-dispatch with a clear WBC-922 rationale and docstring.
test quality	✅ pass	New test_connection_query_tracking.py covers all terminal paths plus cancel-of-untracked; cursor suite covers store/empty/error/re-execute.

Clean sweep — both reviewers found nothing new, all four prior threads landed on beddbb1, and the complete_query pop-before-dispatch helper closes the WBC-922 leak with a dedicated tracking suite to prove it. Ship it.

Prior findings: ✅ 4 resolved

_{0 finding(s) posted.
💰 Review cost: $0.5000 · 132.2k in / 2.8k out tokens · ⏱️ 41.2s
💬 To request a re-review, comment @salty-hambot review}

[james-willis]

@sfishel18 both comments addressed in f906da8 (succeeded-path tests with real JSON/Arrow payloads through the CBOR decode path, and the failed-state-stays-tracked-until-error case) — threads resolved, ready for another look. 🙏

[james-willis]

@sfishel18 I can't merge. can you merge this?