feat(api-server): Add DELETE endpoint to cancel query jobs; Treat Cancelling as a terminal state when polling results.
#1964
Conversation
…ling` as a terminal state when polling for query results.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a search-job cancellation flow: new Client method sets a job's status to Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant "API Endpoint\n(cancel_query)" as API
participant "Client\n(cancel_search_job)" as ClientLib
participant Database
User->>API: DELETE /query/{search_job_id}
API->>ClientLib: cancel_search_job(search_job_id)
activate ClientLib
ClientLib->>Database: UPDATE search_jobs SET status='Cancelling'\nWHERE id = ? AND status IN ('Pending','Running')
Database-->>ClientLib: rows_updated (n) or 0 / SQL error
alt rows_updated >= 1
ClientLib-->>API: Ok(())
else rows_updated == 0
ClientLib-->>API: Err(SearchJobNotFound(search_job_id))
else SQL error
ClientLib-->>API: Err(ClientError::SqlError)
end
deactivate ClientLib
API-->>User: 200 OK or 404 / 500
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@components/api-server/src/client.rs`:
- Around line 236-248: The cancel_search_job function currently returns Ok(())
even if no row was updated; change it to inspect the execute() result's
rows_affected() and return a meaningful error when it is 0. Specifically, after
the UPDATE in cancel_search_job, check result.rows_affected(): if >0 return
Ok(()); if 0, run a follow-up SELECT of the status for the given search_job_id
(e.g. sqlx::query_scalar to read QueryJobStatus) to determine if the job doesn't
exist (return a ClientError::NotFound) or exists but is already terminal (map to
ClientError::Conflict or a new variant); update the route handler to map those
ClientError variants to 404/409 as suggested. Ensure you reference
QueryJobStatus and the same QUERY_JOBS_TABLE_NAME in the SELECT so behavior
matches the UPDATE.
| pub async fn cancel_search_job(&self, search_job_id: u64) -> Result<(), ClientError> { | ||
| sqlx::query(&format!( | ||
| "UPDATE `{QUERY_JOBS_TABLE_NAME}` SET status = ? WHERE id = ? AND status IN (?, ?)" | ||
| )) | ||
| .bind::<i32>(QueryJobStatus::Cancelling.into()) | ||
| .bind(search_job_id) | ||
| .bind::<i32>(QueryJobStatus::Pending.into()) | ||
| .bind::<i32>(QueryJobStatus::Running.into()) | ||
| .execute(&self.sql_pool) | ||
| .await?; | ||
|
|
||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
Silent success for non-existent or already-terminal jobs may mislead API consumers.
cancel_search_job returns Ok(()) regardless of whether any row was actually updated. This means the DELETE endpoint will return 200 OK even when:
- The
search_job_iddoesn't exist at all. - The job is already in a terminal state (
Succeeded,Failed,Cancelled,Killed).
For a REST API, a 404 for a non-existent job (or a 409 for an already-terminal job) would give callers much better signal. Consider inspecting rows_affected() on the query result and differentiating the response.
Suggested approach
pub async fn cancel_search_job(&self, search_job_id: u64) -> Result<(), ClientError> {
- sqlx::query(&format!(
+ let result = sqlx::query(&format!(
"UPDATE `{QUERY_JOBS_TABLE_NAME}` SET status = ? WHERE id = ? AND status IN (?, ?)"
))
.bind::<i32>(QueryJobStatus::Cancelling.into())
.bind(search_job_id)
.bind::<i32>(QueryJobStatus::Pending.into())
.bind::<i32>(QueryJobStatus::Running.into())
.execute(&self.sql_pool)
.await?;
- Ok(())
+ if result.rows_affected() == 0 {
+ // Distinguish between "not found" and "not cancellable" if needed,
+ // e.g., by querying the current status.
+ return Err(ClientError::QueryNotFound); // or a new variant
+ }
+ Ok(())
}The route handler can then map distinct error variants to 404 / 409 as appropriate.
🤖 Prompt for AI Agents
In `@components/api-server/src/client.rs` around lines 236 - 248, The
cancel_search_job function currently returns Ok(()) even if no row was updated;
change it to inspect the execute() result's rows_affected() and return a
meaningful error when it is 0. Specifically, after the UPDATE in
cancel_search_job, check result.rows_affected(): if >0 return Ok(()); if 0, run
a follow-up SELECT of the status for the given search_job_id (e.g.
sqlx::query_scalar to read QueryJobStatus) to determine if the job doesn't exist
(return a ClientError::NotFound) or exists but is already terminal (map to
ClientError::Conflict or a new variant); update the route handler to map those
ClientError variants to 404/409 as suggested. Ensure you reference
QueryJobStatus and the same QUERY_JOBS_TABLE_NAME in the SELECT so behavior
matches the UPDATE.
LinZhihao-723
left a comment
LinZhihao-723
left a comment
There was a problem hiding this comment.
One issue is, if we cancel a search job that doesn't exist, it won't complain and return on success. Actually, it's the same when attempting to retrieve results from a nonexistent query. Shall we improve this as well (in a future PR)?
| /// * Forwards [`sqlx::query::Query::execute`]'s return values on failure. | ||
| pub async fn cancel_search_job(&self, search_job_id: u64) -> Result<(), ClientError> { | ||
| sqlx::query(&format!( | ||
| "UPDATE `{QUERY_JOBS_TABLE_NAME}` SET status = ? WHERE id = ? AND status IN (?, ?)" |
There was a problem hiding this comment.
Just realized this: I think we can leverage const_format to format SQL at compile time. If this sounds good to u, we can ask coderabbitai to create an issue for it.
There was a problem hiding this comment.
🧩 Analysis chain
@hoophalab, I'll create a GitHub issue for using const_format for compile-time SQL formatting.
🏁 Script executed:
#!/bin/bash
# Create a GitHub issue for the const_format suggestion
gh issue create \
--repo y-scope/clp \
--title "Use const_format for compile-time SQL formatting" \
--body "## Context
This issue was created as a follow-up to a suggestion in PR `#1964`.
**Suggested by:** `@LinZhihao-723`
**Requested by:** `@hoophalab`
**PR:** https://github.com/y-scope/clp/pull/1964
**Comment:** https://github.com/y-scope/clp/pull/1964#discussion_r2784365172
## Description
Currently, SQL queries in the codebase are formatted at runtime using the \`format!\` macro. We could leverage the \`const_format\` crate to perform SQL formatting at compile time instead, which could provide performance benefits by moving this work from runtime to compile time.
## Scope
This suggestion applies to SQL query formatting throughout the codebase, particularly in the API server components where database queries are constructed." \
--assignee hoophalab
Length of output: 42
@hoophalab, I've created the issue for you!
Issue #1967: Use const_format for compile-time SQL formatting
URL: #1967
The issue has been assigned to you and includes links back to this PR and the comment thread for context.
🧬 Code Graph Analysis Results
[files_relevant_snippets]
-
File: components/clp-rust-utils/src/database/mysql.rs
Lines 21-37pub async fn create_clp_db_mysql_pool( config: &DatabaseConfig, credentials: &DatabaseCredentials, max_connections: u32, ) -> Result<sqlx::MySqlPool, crate::Error> { let mysql_options = sqlx::mysql::MySqlConnectOptions::new() .host(&config.host) .port(config.port) .database(&config.names.clp) .username(&credentials.user) .password(credentials.password.expose_secret()); Ok(sqlx::mysql::MySqlPoolOptions::new() .max_connections(max_connections) .connect_with(mysql_options) .await?) } -
File: components/clp-rust-utils/src/clp_config/package/config.rs
Lines 103-110fn default() -> Self { Self { host: "localhost".to_owned(), port: 3001, query_job_polling: QueryJobPollingConfig::default(), default_max_num_query_results: 1000, } } -
File: components/clp-rust-utils/src/s3/client.rs
Lines 19-41pub async fn create_new_client( access_key_id: &str, secret_access_key: &str, region_id: &str, endpoint: Option<&NonEmptyString>, ) -> Client { let credentials = Credentials::new( access_key_id, secret_access_key, None, None, "clp-credentials-provider", ); let base_config = aws_config::defaults(BehaviorVersion::latest()) .credentials_provider(credentials) .region(Region::new(region_id.to_string())) .load() .await; let mut config_builder = Builder::from(&base_config); config_builder.set_endpoint_url(endpoint.map(std::string::ToString::to_string)); Client::from_conf(config_builder.build()) } -
File: components/api-server/src/error.rs
Lines 41-43fn from(_value: T) -> Self { Self::MalformedData }Lines 47-51
fn from(value: SdkError<AwsSdkErrorType>) -> Self { Self::Aws { description: value.to_string(), } }Lines 55-59
fn from(value: ByteStreamError) -> Self { Self::Aws { description: value.to_string(), } }Lines 63-71
fn from(value: clp_rust_utils::Error) -> Self { match value { clp_rust_utils::Error::MsgpackEncode(_) | clp_rust_utils::Error::SerdeYaml(_) => { Self::MalformedData } clp_rust_utils::Error::Io(error) => error.into(), clp_rust_utils::Error::Sqlx(error) => error.into(), } } -
File: components/api-server/src/routes.rs
Lines 105-124async fn query( State(client): State<Client>, Json(query_config): Json<QueryConfig>, ) -> Result<Json<QueryResultsUri>, HandlerError> { tracing::info!("Submitting query: {:?}", query_config); let search_job_id = match client.submit_query(query_config).await { Ok(id) => { tracing::info!("Submitted query with search_id: {}", id); id } Err(err) => { tracing::error!("Failed to submit query: {:?}", err); return Err(err.into()); } }; let uri = format!("/query_results/{search_job_id}"); Ok(Json(QueryResultsUri { query_results_uri: uri, })) } -
File: components/api-server/src/client.rs
Lines 164-174
(Note: If you intended to review how the client handles job submission and configuration, the submit_query behavior in this file is relevant.)pub async fn submit_query(&self, query_config: QueryConfig) -> Result<u64, ClientError> { let mut search_job_config: SearchJobConfig = query_config.into(); if search_job_config.dataset.is_none() { search_job_config.dataset = match self.config.package.storage_engine { StorageEngine::Clp => None, StorageEngine::ClpS => Some("default".to_owned()), } } if search_job_config.max_num_results == 0 { search_job_config.max_num_results = self.get_api_server_config().default_max_num_query_results; } let query_job_type_i32: i32 = QueryJobType::SearchOrAggregation.into(); let query_result = sqlx::query(&format!( "INSERT INTO `{QUERY_JOBS_TABLE_NAME}` (`job_config`, `type`) VALUES (?, ?)" )) .bind(rmp_serde::to_vec_named(&search_job_config)?) .bind(query_job_type_i32) .execute(&self.sql_pool) .await?; let search_job_id = query_result.last_insert_id(); Ok(search_job_id) }
🧠 Learnings used
Learnt from: junhaoliao
Repo: y-scope/clp PR: 939
File: components/package-template/src/etc/clp-config.yml:64-64
Timestamp: 2025-06-24T20:13:46.758Z
Learning: When users ask CodeRabbit to create an issue after providing suggestions, they want a GitHub issue created with the high-level requirements and context, not specific code implementations.
Confirmed with @kirkrodrigues that we should probably try to fix this. I think the fix should be just adding a SQL query to check whether the job exists. |
Co-authored-by: Lin Zhihao <59785146+LinZhihao-723@users.noreply.github.com>
Co-authored-by: Lin Zhihao <59785146+LinZhihao-723@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@components/api-server/src/routes.rs`:
- Around line 184-195: The OpenAPI attribute on the delete route for
"/query/{search_job_id}" (the #[utoipa::path(...)] above the cancel handler)
does not explicitly declare the search_job_id path parameter; update that
attribute to include a params(...) entry that names "search_job_id", marks it as
a Path parameter of type u64, and adds a short description (e.g., "The ID of the
search job to cancel") so generated docs include the parameter type and
description; keep the rest of the responses/description the same.
- Around line 184-218: Update the cancel_query endpoint to return 202 Accepted
for successful cancellation submissions instead of 200 OK: change the success
StatusCode in the function body (the Ok branch in match
client.cancel_search_job) to StatusCode::ACCEPTED and update the utoipa
attribute on the cancel_query handler (the responses tuple and any description
referencing OK) to document status = ACCEPTED so the OpenAPI contract reflects
the async acceptance semantics.
| #[utoipa::path( | ||
| delete, | ||
| path = "/query/{search_job_id}", | ||
| description = "Cancels a previously submitted query job.", | ||
| responses( | ||
| ( | ||
| status = OK, | ||
| description = "The cancellation request was submitted successfully." | ||
| ), | ||
| (status = INTERNAL_SERVER_ERROR) | ||
| ) | ||
| )] |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
OpenAPI spec doesn't declare the search_job_id path parameter explicitly.
The query_results endpoint similarly omits an explicit params(...) attribute, so this is consistent with the existing codebase. However, without it, generated OpenAPI documentation may lack the parameter description and type info. Consider adding it for a better developer experience in the API docs:
#[utoipa::path(
delete,
path = "/query/{search_job_id}",
description = "Cancels a previously submitted query job.",
params(
("search_job_id" = u64, Path, description = "The ID of the search job to cancel.")
),
...
)]This is optional since utoipa-axum's routes! macro may infer it from Path<u64>.
🤖 Prompt for AI Agents
In `@components/api-server/src/routes.rs` around lines 184 - 195, The OpenAPI
attribute on the delete route for "/query/{search_job_id}" (the
#[utoipa::path(...)] above the cancel handler) does not explicitly declare the
search_job_id path parameter; update that attribute to include a params(...)
entry that names "search_job_id", marks it as a Path parameter of type u64, and
adds a short description (e.g., "The ID of the search job to cancel") so
generated docs include the parameter type and description; keep the rest of the
responses/description the same.
| #[utoipa::path( | ||
| delete, | ||
| path = "/query/{search_job_id}", | ||
| description = "Cancels a previously submitted query job.", | ||
| responses( | ||
| ( | ||
| status = OK, | ||
| description = "The cancellation request was submitted successfully." | ||
| ), | ||
| (status = INTERNAL_SERVER_ERROR) | ||
| ) | ||
| )] | ||
| async fn cancel_query( | ||
| State(client): State<Client>, | ||
| Path(search_job_id): Path<u64>, | ||
| ) -> Result<StatusCode, HandlerError> { | ||
| tracing::info!("Cancelling search job ID: {}", search_job_id); | ||
| match client.cancel_search_job(search_job_id).await { | ||
| Ok(()) => { | ||
| tracing::info!( | ||
| "Successfully submitted cancellation request for search job ID: {}", | ||
| search_job_id | ||
| ); | ||
| Ok(StatusCode::OK) | ||
| } | ||
| Err(err) => { | ||
| tracing::error!( | ||
| "Failed to cancel search job ID {}: {:?}", | ||
| search_job_id, | ||
| err | ||
| ); | ||
| Err(err.into()) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider 202 Accepted instead of 200 OK for the asynchronous cancellation.
The endpoint submits a cancellation request (transitions the job to Cancelling), but the job isn't actually cancelled until a downstream component processes it. 202 Accepted better communicates that the request has been accepted for processing but is not yet complete — this is the standard REST semantic for asynchronous operations.
Suggested diff
responses(
(
- status = OK,
- description = "The cancellation request was submitted successfully."
+ status = ACCEPTED,
+ description = "The cancellation request was submitted successfully."
),
(status = INTERNAL_SERVER_ERROR)
)
)]
async fn cancel_query(
State(client): State<Client>,
Path(search_job_id): Path<u64>,
-) -> Result<StatusCode, HandlerError> {
+) -> Result<StatusCode, HandlerError> {
tracing::info!("Cancelling search job ID: {}", search_job_id);
match client.cancel_search_job(search_job_id).await {
Ok(()) => {
tracing::info!(
"Successfully submitted cancellation request for search job ID: {}",
search_job_id
);
- Ok(StatusCode::OK)
+ Ok(StatusCode::ACCEPTED)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #[utoipa::path( | |
| delete, | |
| path = "/query/{search_job_id}", | |
| description = "Cancels a previously submitted query job.", | |
| responses( | |
| ( | |
| status = OK, | |
| description = "The cancellation request was submitted successfully." | |
| ), | |
| (status = INTERNAL_SERVER_ERROR) | |
| ) | |
| )] | |
| async fn cancel_query( | |
| State(client): State<Client>, | |
| Path(search_job_id): Path<u64>, | |
| ) -> Result<StatusCode, HandlerError> { | |
| tracing::info!("Cancelling search job ID: {}", search_job_id); | |
| match client.cancel_search_job(search_job_id).await { | |
| Ok(()) => { | |
| tracing::info!( | |
| "Successfully submitted cancellation request for search job ID: {}", | |
| search_job_id | |
| ); | |
| Ok(StatusCode::OK) | |
| } | |
| Err(err) => { | |
| tracing::error!( | |
| "Failed to cancel search job ID {}: {:?}", | |
| search_job_id, | |
| err | |
| ); | |
| Err(err.into()) | |
| } | |
| } | |
| } | |
| #[utoipa::path( | |
| delete, | |
| path = "/query/{search_job_id}", | |
| description = "Cancels a previously submitted query job.", | |
| responses( | |
| ( | |
| status = ACCEPTED, | |
| description = "The cancellation request was submitted successfully." | |
| ), | |
| (status = INTERNAL_SERVER_ERROR) | |
| ) | |
| )] | |
| async fn cancel_query( | |
| State(client): State<Client>, | |
| Path(search_job_id): Path<u64>, | |
| ) -> Result<StatusCode, HandlerError> { | |
| tracing::info!("Cancelling search job ID: {}", search_job_id); | |
| match client.cancel_search_job(search_job_id).await { | |
| Ok(()) => { | |
| tracing::info!( | |
| "Successfully submitted cancellation request for search job ID: {}", | |
| search_job_id | |
| ); | |
| Ok(StatusCode::ACCEPTED) | |
| } | |
| Err(err) => { | |
| tracing::error!( | |
| "Failed to cancel search job ID {}: {:?}", | |
| search_job_id, | |
| err | |
| ); | |
| Err(err.into()) | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@components/api-server/src/routes.rs` around lines 184 - 218, Update the
cancel_query endpoint to return 202 Accepted for successful cancellation
submissions instead of 200 OK: change the success StatusCode in the function
body (the Ok branch in match client.cancel_search_job) to StatusCode::ACCEPTED
and update the utoipa attribute on the cancel_query handler (the responses tuple
and any description referencing OK) to document status = ACCEPTED so the OpenAPI
contract reflects the async acceptance semantics.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@components/api-server/src/routes.rs`:
- Around line 239-246: The global impl From<ClientError> for HandlerError maps
ClientError::SearchJobNotFound to HandlerError::NotFound which can
unintentionally return 404s from any handler (query, query_results,
cancel_query) if SearchJobNotFound is reused; remove or narrow this global
conversion and instead perform an explicit match-to-HandlerError conversion in
the specific handlers (e.g., in cancel_query, query, query_results) where a
SearchJobNotFound should be treated as NotFound, or alternatively introduce a
more specific ClientError variant name (e.g., SearchJobNotFoundForQuery) and
keep the global impl; reference ClientError, HandlerError, and the
SearchJobNotFound variant when making the change.
| impl From<ClientError> for HandlerError { | ||
| fn from(err: ClientError) -> Self { | ||
| match err { | ||
| ClientError::SearchJobNotFound => Self::NotFound, | ||
| _ => Self::InternalServer, | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Verify that the From<ClientError> mapping doesn't unintentionally surface 404 from other endpoints.
The catch-all _ => Self::InternalServer is correct for non-cancellation code paths today, but this From impl is shared across all handlers (query, query_results, cancel_query). Currently no other code path produces SearchJobNotFound, so this is safe. Worth keeping in mind if SearchJobNotFound is reused in future client methods — a 404 might surface unexpectedly from a different endpoint.
🤖 Prompt for AI Agents
In `@components/api-server/src/routes.rs` around lines 239 - 246, The global impl
From<ClientError> for HandlerError maps ClientError::SearchJobNotFound to
HandlerError::NotFound which can unintentionally return 404s from any handler
(query, query_results, cancel_query) if SearchJobNotFound is reused; remove or
narrow this global conversion and instead perform an explicit
match-to-HandlerError conversion in the specific handlers (e.g., in
cancel_query, query, query_results) where a SearchJobNotFound should be treated
as NotFound, or alternatively introduce a more specific ClientError variant name
(e.g., SearchJobNotFoundForQuery) and keep the global impl; reference
ClientError, HandlerError, and the SearchJobNotFound variant when making the
change.
LinZhihao-723
left a comment
LinZhihao-723
left a comment
There was a problem hiding this comment.
I guess we need to update get_status to return SearchJobNotFound as well, but presumably in another PR.
components/api-server/src/error.rs
Outdated
| #[error("No cancellable search job found")] | ||
| SearchJobNotFound, |
There was a problem hiding this comment.
| #[error("No cancellable search job found")] | |
| SearchJobNotFound, | |
| #[error("Search job ID found: {0}")] | |
| SearchJobIdNotFound(u64), |
How about:
- Update the description accordingly, since it might not just be used for cancellation. This should make the error message general, and it's reusable in result retrieval.
- Also store the ID of the search job.
There was a problem hiding this comment.
I update the doc and add the id, but keep the name. SearchJobNotFound sounds more concise.
Co-authored-by: Lin Zhihao <59785146+LinZhihao-723@users.noreply.github.com>
@coderabbitai Can you create an issue? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain
🏁 Script executed: Length of output: 42
🧠 Learnings used |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@components/api-server/src/routes.rs`:
- Around line 213-219: The error branch that logs cancellation failures should
treat SearchJobNotFound as a client-side condition and log it at warn (or info)
instead of error: in the Err(err) handling for the cancel flow (where
search_job_id is referenced), match or downcast the error to detect the
SearchJobNotFound variant and call tracing::warn! with a message including
search_job_id; for all other errors keep tracing::error! and return
Err(err.into()) as before.
| Err(err) => { | ||
| tracing::error!( | ||
| "Failed to cancel search job ID {}: {:?}", | ||
| search_job_id, | ||
| err | ||
| ); | ||
| Err(err.into()) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider logging SearchJobNotFound at warn level instead of error.
A 404 from SearchJobNotFound is a client-side issue (non-existent or already-terminal job), not a server error. Logging it at error level can create noise in production, especially if clients retry cancellation. A warn or info level would be more appropriate for expected client mistakes, reserving error for actual server failures.
This is consistent with the existing pattern in other handlers, so it's optional to change here.
Suggested approach
Err(err) => {
- tracing::error!(
- "Failed to cancel search job ID {}: {:?}",
- search_job_id,
- err
- );
+ match &err {
+ ClientError::SearchJobNotFound(_) => {
+ tracing::warn!(
+ "No cancellable search job found for ID {}: {:?}",
+ search_job_id,
+ err
+ );
+ }
+ _ => {
+ tracing::error!(
+ "Failed to cancel search job ID {}: {:?}",
+ search_job_id,
+ err
+ );
+ }
+ }
Err(err.into())
}🤖 Prompt for AI Agents
In `@components/api-server/src/routes.rs` around lines 213 - 219, The error branch
that logs cancellation failures should treat SearchJobNotFound as a client-side
condition and log it at warn (or info) instead of error: in the Err(err)
handling for the cancel flow (where search_job_id is referenced), match or
downcast the error to detect the SearchJobNotFound variant and call
tracing::warn! with a message including search_job_id; for all other errors keep
tracing::error! and return Err(err.into()) as before.
LinZhihao-723
left a comment
LinZhihao-723
left a comment
There was a problem hiding this comment.
For the PR title, how about:
feat(api-server): Add DELETE endpoint to cancel query jobs; Treat `Cancelling` as a terminal state when polling results.
hoophalab
changed the title
Cancelling as a terminal state when polling for query results.Cancelling as a terminal state when polling results.
2 participants
Description
This PR adds the ability to cancel previously submitted query jobs through the API server.
DELETE /query/{search_job_id}endpoint that submits a cancellation request for a searchjob by updating its status to
Cancellingif the job is currentlyPendingorRunning.Cancellingas a terminal state in the query results polling loop so that the client stopswaiting for results from a job that is being cancelled.
utoipa.Checklist
breaking change.
Validation performed
DELETE http://localhost:3001/query/104then5inclp-db.query_jobstableSummary by CodeRabbit
New Features
Behavior Changes
Error Handling
Documentation