Remove cancellation and add diagnostics to Cosmos native FFI wrapper - #5309
Arooshi Avasthy (aavasthy) wants to merge 10 commits into
Conversation
|
Azure Pipelines: Successfully started running 1 pipeline(s). 2 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
🟡 Changes recommended
The API metadata is stale and queue-free documentation contradicts the implementation.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Removes drop-based cancellation from the Cosmos native FFI so operations complete with diagnostics while preserving reserved cancellation values.
Changes:
- Removes cancellation APIs, state, and tests.
- Awaits driver operations through normal completion.
- Updates ABI documentation and language examples.
File summaries
| File | Description |
|---|---|
docs/specs/0020-native-async-invocation.md |
Revises async flow documentation. |
docs/specs/0019-native-wrapper.md |
Updates cancellation and shutdown contracts. |
src/submit.rs |
Removes cancellation race logic. |
src/response_header.rs |
Updates completion documentation. |
src/error.rs |
Marks cancellation status reserved. |
src/completion.rs |
Removes cancellation state and FFI API. |
README.md |
Updates binding examples and layouts. |
include/azurecosmosdriver.h |
Updates the public C ABI. |
c_tests/completion_headers_abi.c |
Updates test reference. |
c_tests/cancellation.c |
Removes cancellation harness. |
api/API.md |
Updates exported Rust API surface. |
Review details
- Files reviewed: 11/11 changed files
- Comments generated: 2
- Review effort level: Balanced
| @@ -59,8 +59,6 @@ pub mod completion { | |||
| #[no_mangle] | |||
| pub extern "C" fn cosmos_completion_take_driver(c: *mut CosmosCompletion) -> *mut crate::driver::DriverHandle; | |||
| #[no_mangle] | |||
| pub extern "C" fn cosmos_operation_handle_cancel(op: *mut OperationHandle); | |||
| #[no_mangle] | |||
| pub extern "C" fn cosmos_operation_handle_free(op: *mut OperationHandle); | |||
There was a problem hiding this comment.
the metadata is now regenerated and consistent. API.metadata.yml records apiMdSha256: d58706cc… , which is the exact SHA-256 of the current API.md , so both artifacts are in sync.
| ``` | ||
|
|
||
| **Freeing a queue with operations still in-flight** is a programming error: `cosmos_cq_free` will block until all in-flight submissions targeting that queue have completed (cancelling each one first). Host SDKs that need a non-blocking shutdown must call `cosmos_cq_shutdown` and drain via `cosmos_cq_wait` until `cosmos_cq_state` returns `DRAINED`, then `_free`. See §3.6.4. | ||
| **Freeing a queue with operations still in-flight** is a programming error: `cosmos_cq_free` will block until all in-flight submissions targeting that queue have completed (running each one to its natural completion — there is no cancellation, see §3.6.3). Host SDKs that need a non-blocking shutdown must call `cosmos_cq_shutdown` and drain via `cosmos_cq_wait` until `cosmos_cq_state` returns `DRAINED`, then `_free`. See §3.6.4. |
There was a problem hiding this comment.
cosmos_cq_free just drops the producer handle ( drop_raw ); in-flight tasks hold their own Arc , so it never blocks. Updated the docs to state the actual non-blocking behavior: freeing with ops in-flight is memory-safe but abandons their completions/diagnostics, and hosts that must observe every completion should shutdown + drain to DRAINED first. Fixed in §3.1.2, §3.4, §3.6.4, the header doc, and the 0020 diagram.
d0bdd71 to
3f4130c
Compare
Arooshi Avasthy (aavasthy)
changed the title
|
/azp run rust - cosmos - weekly |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
/azp run rust - cosmos - weekly |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
| JAVA_INT.withName("outcome"), | ||
| JAVA_INT.withName("status"), | ||
| JAVA_LONG.withName("user_data"), | ||
| JAVA_BYTE.withName("was_cancel_requested"), |
There was a problem hiding this comment.
This padding no longer matches the native cosmos_completion_t layout after removing was_cancel_requested. http_status_code occupies bytes 16–17 and is_from_wire is byte 18, so five padding bytes are needed before the 8-byte-aligned message pointer at offset 24. With paddingLayout(3), Java places message at offset 22 and all following pointers are shifted. Could you change this to paddingLayout(5) and add a native sizeof/offsetof assertion for the completion fields so future ABI edits catch this?
There was a problem hiding this comment.
Changed to paddingLayout(5) . Verified offsets by compiling _Static_assert(offsetof(...)) against the generated header: sizeof==112 , http_status_code@16 , is_from_wire@18 , message@24 , diagnostics@80 , backing@104 . Also added those asserts to completion_headers_abi.c so future ABI edits fail at compile time.
Simon Moreno (simorenoh)
left a comment
Simon Moreno (simorenoh)
left a comment
There was a problem hiding this comment.
I think there's some issues still Ananth called out with padding but LGTM for the most part
|
/azp run rust - cosmos - weekly |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
Resolve API.metadata.yml conflict by regenerating the native crate's API artifacts so the recorded apiMdSha256 matches the merged API.md (both main's and this PR's changes). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Resolve api.metadata.yml conflict by regenerating the native crate's API artifacts (main lowercased the api.md/api.metadata.yml filenames); apiMdSha256 now matches the merged api.md containing main's changes plus this PR's diagnostics feature. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This removes on-demand operation cancellation from the Cosmos native FFI wrapper and fills in the previously-reserved diagnostics handle so every completion carries the driver's diagnostics.
Cancellation existed so a caller could stop an in-flight operation early, but the only way the wrapper could do that was to drop the running driver future, which also threw away the diagnostics the driver was building for that request. Since silently losing diagnostics isn't acceptable and the wrapper has no other way to stop a request, cancellation is removed and every operation now runs to completion and always carries diagnostics. Callers who need to bound how long an operation runs use the driver's end-to-end timeout, which returns a normal error with diagnostics. Real on-demand cancellation that preserves diagnostics can only be built inside the driver later, so this change is wrapper-only and keeps the "cancelled" ABI values reserved so it can be added back without breaking compatibility.
The completion previously carried a reserved diagnostics field that was always NULL. This wires that field to a real
cosmos_diagnostics_thandle, populated on success and on errors that carry diagnostics. It exposes NULL-safe accessors for request counts, region names, and per-attempt status, pluscosmos_diagnostics_to_jsonwith a verbosity selector (Default/Summary/Detailed). Validated with clippy (no warnings) and the crate's tests (167 passing); the generated header andapi/API.mdare regenerated and in sync.