Implement first step of cancellation: Stop all handler when one completes/fails - #204
Open
msirringhaus wants to merge 3 commits into
Open
Implement first step of cancellation: Stop all handler when one completes/fails#204msirringhaus wants to merge 3 commits into
msirringhaus wants to merge 3 commits into
Conversation
msirringhaus
force-pushed
the
cancellation
branch
from
August 19, 2026 07:01
10fb382 to
e2c3acf
Compare
There was a problem hiding this comment.
Pull request overview
Introduces shared cancellation across credential transports so remaining handlers stop when one completes, fails, or is cancelled.
Changes:
- Propagates cancellation tokens through USB, NFC, hybrid, and UI handlers.
- Completes requests on transport success or failure.
- Adds cancellation-focused tests and dependencies.
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
credentialsd/src/dbus/flow_control.rs |
Stops UI event handling on cancellation. |
credentialsd/src/credential_service/mod.rs |
Coordinates cancellation and request completion. |
credentialsd/src/credential_service/usb.rs |
Adds USB cancellation handling. |
credentialsd/src/credential_service/nfc.rs |
Adds NFC cancellation handling. |
credentialsd/src/credential_service/hybrid.rs |
Adds hybrid cancellation handling. |
credentialsd/Cargo.toml |
Adds cancellation and test dependencies. |
Cargo.lock |
Locks the added dependencies. |
Suppressed comments (1)
credentialsd/src/credential_service/mod.rs:1021
- This assertion is probabilistic: two independent random
u32request IDs can legally be equal, making the test flaky. Either remove this test or inject a deterministic ID generator and test an actual uniqueness contract.
// IDs should be different (random)
assert_ne!(
request_id_1, request_id_2,
"Sequential requests should (almost certainly) have different IDs"
);
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+166
to
169
| _ = cancellation.cancelled() => { | ||
| tracing::debug!("Hybrid handler cancelled, stopping processing"); | ||
| break Err(Error::Internal("Request cancelled".to_string())); | ||
| } |
Comment on lines
+76
to
+78
| _ = cancellation.cancelled() => { | ||
| tracing::debug!("USB idle polling cancelled"); | ||
| Err(Error::Internal("Request cancelled".to_string())) |
Collaborator
Author
There was a problem hiding this comment.
This is true, but shouldn't cause any harm, other than some spurious logging. Probably the best solution would be to introduce a Cancelled-error variant and use that everywhere.
This might get a bit more involved, though. @iinuwa do you have any preference as to implementing this right here, or in a follow-up PR?
Comment on lines
+69
to
+71
| _ = cancellation.cancelled() => { | ||
| tracing::debug!("NFC idle polling cancelled"); | ||
| Err(Error::Internal("Request cancelled".to_string())) |
Comment on lines
+227
to
+229
| _ = cancellation.cancelled() => { | ||
| tracing::debug!("NFC handler cancelled, stopping processing"); | ||
| break Err(Error::Internal("Request cancelled".to_string())); |
| } | ||
| } => { | ||
| state = next_usb_state.unwrap_or_else(UsbStateInternal::Failed); | ||
| // Usually, comparing the Discrimimant is enough, but PinNotSet/NeedsPin |
msirringhaus
force-pushed
the
cancellation
branch
from
August 19, 2026 14:35
e2c3acf to
d5e4220
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Again, based on #135 . Only the last commit is relevant. Will be rebased, once the other PR lands.
Next step would be cancellation events from the UI.