Skip to content

feat: add active_queries and cancel_active_query for running SQL queries - #90

Merged
lukekim merged 2 commits into
spiceai:trunkfrom
claudespice:feat/active-query-cancellation
Aug 13, 2026
Merged

feat: add active_queries and cancel_active_query for running SQL queries#90
lukekim merged 2 commits into
spiceai:trunkfrom
claudespice:feat/active-query-cancellation

Conversation

@claudespice

@claudespice claudespice commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

What

Adds Client::active_queries() and Client::cancel_active_query(), wrapping the runtime's GET /v1/sql/active and POST /v1/sql/{query_id}/cancel.

Why

Cancelling a running synchronous query — one started by sql(), FlightSQL, /v1/sql, NSQL, or search — was not reachable from this SDK. The existing cancel_query() cancels an async job via /v1/queries/{id}/cancel, which is a different thing and is gated on the runtime running in cluster mode. The two endpoints added here work on a default single-node runtime, so this closes a gap users hit today.

They ship as a pair deliberately: the runtime assigns a query_id to every synchronous query but does not return it to the client that submitted it, so active_queries() is the only way to discover the id that cancellation needs. Shipping cancel alone would leave it unreachable.

Both endpoints are scoped to the caller by the runtime, so a client only ever lists and cancels its own queries — that's reflected in the docs and in the NotFound error text, since an id belonging to another caller is reported as not found rather than cancelled.

Kept in a new active_query module rather than folded into query, so the sync/async distinction stays legible at the type level: ActiveQueryError carries only variants that can actually occur here (no ClusterModeRequired, no Expired).

Part of aligning query cancellation across the SDKs — no SDK had this, so this establishes the shape (active_queries / cancel_active_query, named after the runtime's own operation ids) for the others to mirror.

Verification

  • cargo build
  • cargo test --lib — 196 passed, including 5 new tests for response deserialization and error text
  • cargo test --doc — 29 passed, including the new README example
  • cargo fmt --all --check
  • cargo clippy --all-features — clean
  • Integration tests (cargo test) — not run: they need a live runtime on 127.0.0.1:50051 and no spice CLI is available in this environment. The 7 pre-existing failures there are all connection-refused and are unrelated to this change.

Review gate

Attests the review-fix commit 0206785, not the original feature work.

  • Adversarial review: codex, two passes — 1c6a068 (verdict needs-attention) and the re-run after the approach changed. Findings triaged on the review threads:
    • Accepted: the corrected scope wording still misstated the boundary for mTLS-authenticated clients, and omitted the runtime-instance boundary. Both fixed.
    • Accepted, and the largest change here: the path-hardening covered only cancel_active_query, leaving QueryHttpClient::cancel / get_status / get_query / the chunk readers formatting a caller-supplied id into the path — reproduced as an authenticated POST landing on /v1/datasets/{name}/acceleration/refresh. All query-id routes now build URLs through build_url.
    • Deferred with an issue: cluster-wide active-query lookup is a runtime gap, not an SDK one — Active-query listing and cancellation only reach one runtime instance spiceai#13026.
  • Security review: covered by the above — the diff is documentation plus URL construction, and the URL-construction defect is the security finding, reproduced and regression-tested rather than argued.
  • Local gate: cargo fmt --check, cargo clippy --all-targets, cargo test --lib (206 passed), cargo test --doc (29 passed). The 7 client_test::tests::test_local_* failures need a running local runtime and fail identically with this diff stashed.

The runtime exposes GET /v1/sql/active and POST /v1/sql/{query_id}/cancel
for listing and cancelling running synchronous queries, but neither was
reachable from this SDK.

These ship as a pair because the runtime assigns a query_id to every
synchronous query and does not return it to the submitting client, so
listing is the only way to discover the id that cancellation needs.

Kept separate from the existing async-job cancel_query: async jobs are
gated on cluster mode, whereas these two endpoints work on a default
single-node runtime.
@claudespice claudespice self-assigned this Aug 13, 2026
@claudespice
claudespice requested a balanced review from Copilot August 13, 2026 17:41
@claudespice

Copy link
Copy Markdown
Contributor Author

@copilot review

Copilot AI 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.

Pull request overview

Adds SDK support for listing and cancelling active synchronous queries.

Changes:

  • Adds active-query models and errors.
  • Implements HTTP endpoint wrappers and public client methods.
  • Adds API documentation, examples, and deserialization tests.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
src/active_query.rs Defines active-query types, errors, docs, and tests.
src/query.rs Implements active-query HTTP requests.
src/client.rs Exposes the new client methods.
src/lib.rs Exports the new public API.
README.md Documents listing and cancellation usage.
Suppressed comments (4)

src/active_query.rs:103

  • The runtime sorts this response by started_at_ms ascending (then query ID), so callers receive the oldest query first. This public contract currently promises the opposite; either document the actual order or reverse the response before returning it.
    /// The active queries, most recently started first.

src/active_query.rs:65

  • This variant is also returned when active_queries() receives a 403, but its message says cancellation failed. Use operation-neutral wording so listing failures are not misleading.
        "The configured API key does not allow cancelling queries. Use a key with write access."

src/client.rs:589

  • Cancellation is scoped to the runtime principal, not the SDK client instance. Another client with the same API key—and any unauthenticated client in the shared public scope—can cancel the query, so this isolation guarantee needs qualification.
    /// Cancellation is scoped to this client: an id belonging to another caller is
    /// reported as not found rather than cancelled.

src/active_query.rs:53

  • This message attributes NotFound to a different client, but client instances sharing an API key (or the unauthenticated public scope) are allowed to cancel each other's queries. Describe the actual principal boundary instead.
        "No active query '{query_id}' found. It may have already finished, or it was submitted by a different client."

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/query.rs Outdated
Comment thread src/active_query.rs Outdated
Comment thread README.md Outdated
Comment thread src/client.rs Outdated
Comment thread src/query.rs
Comment thread src/query.rs
…the route

Copilot's review on spiceai#90 named the per-client isolation claim as wrong, and
it is: the runtime scopes listing and cancellation by the request's
cache-namespace storage id (`current_job_owner` in
`crates/runtime/src/jobs/mod.rs`), which resolves to the authenticated
principal — an API key or a client certificate — or to `public` when the
runtime establishes no principal at all. `list_for`/`cancel_owned` then
compare that owner alone, so clients presenting the same credential share
the query set.

Two boundaries the docs never mentioned are now stated: the registry is an
in-memory map owned by one runtime process (the handler lists what is "known
to this runtime"), while a Client configures Flight and HTTP independently;
and no runtime release up to v2.1.5 contains the principal scoping at all —
`v2.1.5` still calls the unscoped `registry.list()`/`cancel()` behind a
write-access check. spiceai/spiceai#13026 tracks the instance boundary.

The bigger finding is a URL-construction defect this SDK had throughout.
Formatting a caller-supplied query id into a path string lets that id pick
the route: the parser resolves `..` away and a `#` truncates the path at the
fragment, so `get_query("../datasets/orders/acceleration/refresh#").cancel()`
sent an authenticated POST to `/v1/datasets/orders/acceleration/refresh` and
started a dataset refresh. Every query-id route now builds its URL through
`QueryHttpClient::build_url`, which pushes each segment (encoding `/`, `?`
and `#`) and refuses `.` and `..`, which encoding leaves alone. The new
`cancel_active_query` additionally rejects a non-UUID id outright, since its
ids only ever come from `active_queries()`.

Adds the Wiremock coverage the endpoints were missing — both active-query
routes, every status mapping, the unconfigured-HTTP-URL wrapper path, the
rejection, and a cross-route test asserting no query id can reach the
dataset-refresh, sync-cancel, or collection routes — and records the four
protocol values the runtime emits.
@lukekim
lukekim merged commit 97add5e into spiceai:trunk Aug 13, 2026
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants