Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds end-to-end support for cancelling an in-progress submission: it introduces a cancelled submission state persisted in a new submissions_cancelled table, exposes a new producer HTTP endpoint + Rust client method, and wires the feature through the Python bindings and tests.
Changes:
- Add
SubmissionStatus::Cancelled/SubmissionCancelledand a DB cancel path that moves submissions intosubmissions_cancelled. - Expose
POST /producer/submissions/cancel/{submission_id}and a matchingClient::cancel_submissionmethod. - Add Python API + exceptions for cancellation and add roundtrip tests for cancel/not-found/not-cancellable cases.
Reviewed changes
Copilot reviewed 14 out of 16 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| opsqueue/src/prometheus.rs | Adds metric names for cancelled submissions. |
| opsqueue/src/producer/server.rs | Adds new HTTP route/handler for cancelling submissions. |
| opsqueue/src/producer/client.rs | Adds client method to call the cancel endpoint and decode errors. |
| opsqueue/src/common/submission.rs | Adds cancelled submission status, DB queries, cancel mutation, and cleanup logic. |
| opsqueue/src/common/errors.rs | Adds serializable cancellation-related error types. |
| opsqueue/opsqueue_example_database_schema.db | Adds an example SQLite schema DB file reflecting new tables. |
| opsqueue/migrations/20260225164600_submissions_cancelled.sql | Adds migration creating submissions_cancelled. |
| libs/opsqueue_python/tests/test_roundtrip.py | Adds Python integration tests for cancel behavior and exceptions. |
| libs/opsqueue_python/src/producer.rs | Exposes cancel_submission from Rust -> Python client. |
| libs/opsqueue_python/src/lib.rs | Registers new Python-exposed types (but see review comments). |
| libs/opsqueue_python/src/errors.rs | Maps new cancellation errors into Python exceptions. |
| libs/opsqueue_python/src/common.rs | Adds Python types for cancelled status and not-cancellable details. |
| libs/opsqueue_python/python/opsqueue/producer.py | Adds Python ProducerClient.cancel_submission wrapper + docs. |
| libs/opsqueue_python/python/opsqueue/exceptions.py | Adds Python SubmissionNotCancellableError and enriches SubmissionNotFoundError. |
| Cargo.toml | Bumps workspace version to 0.35.0. |
| Cargo.lock | Updates locked versions for opsqueue and opsqueue_python. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 16 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| describe_counter!( | ||
| SUBMISSIONS_CANCELLED_COUNTER, | ||
| Unit::Count, | ||
| "Number of submissions cancelled permanently" |
There was a problem hiding this comment.
Nit: Good to indicate here what the difference between cancelled and failed is, so people using the Prometheus metrics in Grafana will understand without having to look it up.
Qqwy
left a comment
Qqwy
left a comment
There was a problem hiding this comment.
Very nice work Jeremy! The main thing that's still missing (if I read it correctly) is a test case for the happy path. Besides that, I have a couple of smaller nits/remarks, but nothing big.
| // 200 if the submission was successfully cancelled. | ||
| // 404 if the submission could not be found. | ||
| // 409 if the submission could not be cancelled. | ||
| // 500 if a DatabaseError occurred. |
There was a problem hiding this comment.
Nit: If you turn this into /// comments, it will become part of the documentation (both for LSPs and if someone runs cargo doc)
As discussed face to face: It would be cleaner to encode these errors inside the data rather than in the transport layer, but since we control both the server and the client here this can be considered overkill. So let's Keep It Simple as it is now 👍.
| E< | ||
| errors::SubmissionNotFound, | ||
| E<errors::SubmissionNotCancellable, InternalProducerClientError>, | ||
| >, |
There was a problem hiding this comment.
Nit: You can use the E![...] macro to make the return type a bit more readable here. See for example
opsqueue/libs/opsqueue_python/src/producer.rs
Line 209 in 3367641
| query!( | ||
| "DELETE FROM submissions_metadata | ||
| WHERE submission_id = ( | ||
| WHERE submission_id IN ( |
There was a problem hiding this comment.
Thanks! Pretty sure it was (only...) the leniency that SQLite has in certain places that made this work before.
| submission = producer_client.get_submission_status(submission_id) | ||
| assert submission is not None | ||
| assert_submission_failed_has_metadata(submission.submission) | ||
|
|
There was a problem hiding this comment.
Am I overlooking something, or is there no test for the 'happy path' yet?
(Love the error case tests though! 👍)
| .await | ||
| .map_err(|e| CError(R(e))) | ||
| }) | ||
| // TODO ? |
There was a problem hiding this comment.
Nit: Left commented code with TODO
This PR adds a new method
cancel_submissiontoProducerClient. This method takes aSubmissionIdand returnsNoneif successful. If the submission could not be found aSubmissionNotFoundErroris raised. If the submission was already completed, failed or cancelled then aSubmissionNotCancellableErroris raised which contains the specific reason.