Display error/reason to user, if a transport had to be restarted by msirringhaus · Pull Request #231 · linux-credentials/credentialsd

msirringhaus · 2026-09-09T14:40:16Z

Note: Based on #230 , and needs to be rebased if/when this lands.

Only send events, if the device was actively used by the user (Hybrid: QR scanned, USB: initial touch done, etc.).

Extended the Restarting-events introduced in #230 with an error reason. Display it at the bottom of the start page. Removed those error codes that trigger restarts from the terminal error code list.

Last commit updates the lang-files.

iinuwa

haven't run this yet, but a couple of questions

iinuwa · 2026-09-29T12:12:55Z

+                        } else {
+                            // Pre-active: the QR was never consumed or the BLE channel
+                            // failed before the phone responded. Reissue silently.
+                            tracing::debug!(?err, "Hybrid pre-active error, reissuing QR silently");


I'm reading this on the train, so just wanted to double-check: can this get us in a restart loop? Is there a point where we eventually stop retrying a transport (without failing the ceremony)?

Yes, it could potentially end in a loop. USB has a somewhat similar problem, actually.

I actually have to think about this a bit more. If and how to display this to the user, for example (do we want something like "we removed hybrid from the list, because it failed too often" in the UI?), and how to structure the error handling to deal with this.
It's somewhat hard to distinguish between "this will never work"-errors and those who might be recoverable.
I need a clearer structure for stopping transports (e.g. probably makes sense to stop it on Internal-errors)

Or we never stop/remove a transport, but only throttle it with delays between retries?

iinuwa · 2026-09-29T12:25:05Z

+/// as invalid by `TryFrom`.
+#[repr(u8)]
+#[derive(Debug, Clone, Copy, PartialEq, Eq)]
+pub enum TransportRestartReason {


We're currently using PascalCase strings to communicate enums everywhere else in the API. I find that easier to read in D-Bus output while debugging, and the extra 10 or so bytes in the message doesn't hurt.

Do you have opinions either way? If not, I'd recommend to make this a string for consistency.

msirringhaus · 2026-10-08T14:57:00Z

Ok, I added a way to break the restart loop now. Sorry, this has gotten really big. Please let me know, if you want me to somehow look into splitting this into multiple PRs (although I'm not yet sure, how to easily do that without breaking the functionality. But I'm sure, I can come up with something).

This now basically does what we did for USB already for all transports: Count 5 restarts before user interaction and then stop the transport. For this, we send an update to the UI, telling it that a certain transport has become unavailable.

In doing so, I also changed the initial detection of available transactions a bit. We had duplicates of this. I removed those, and now we have one source of truth for available transports (which now also deactivates NFC, instead of commenting out the handler).

In CreateSession, we have sent a list of fake devices to the UI, where we only ever used the transport. I took the liberty to change CreateSession such, that we only send available transports now instead. Subsequent events then can remove a transport from the list the UI has, causing it to display an error message and not list that section in the start-page anymore. This is a breaking change, obviously.

iinuwa · 2026-10-10T04:03:54Z

This API is getting a little gnarly, and I think it's going to be too hard for other backend implementations to bite off. I think we need to try another direction.

I wonder if we've been thinking about this backwards: what if we send the errors to the UI as normal, and then the UI can send a trigger to restart the request or not? Then a simpler UI implementation can just bail and leave the user to restart the request if they want, and a more complex UI can show the error with "try again"/"cancel" options, and if they select try again, trigger the restart and re-do the flow. I think that leaves us with flat errors while still giving the ability to give the user more context.

I like:

deferring send of events until user interaction,
limits on the transport errors before user interaction, and
the ability to drop transports in the middle of the request if they're being flaky.

Maybe we need to model the transports as a D-Bus property that can be subscribed to instead?

iinuwa

Made some comments on the API, but still need to dig into the credential_service changes. I'll try to get to those soon.

Overall, I'm feeling OK about the direction, but have some suggestions.

I'll also note that we removed devices in favor of transports, but I think that we'll need to add something like that back, just not fake, for #256

iinuwa · 2026-11-10T13:18:00Z

-    async fn get_available_public_key_devices(&self) -> Result<Vec<Device>, ()>;
+    /// Detects the transports usable for the current request and remembers them,
+    /// so `start_discovery` starts exactly what was reported to the UI.
+    async fn detect_transports(&self) -> Vec<Transport>;


I think we should return this from init_request() instead and remove this method altogether.

Yes, makes sense. I still need an accessor-function, though

iinuwa · 2026-11-10T13:19:38Z

                response_channel: tx,
                request_id,
                cancellation: cancellation.clone(),
+                transports: Vec::new(),


yeah, let's make get_transports() private and call it here.

iinuwa · 2026-11-10T13:23:58Z

+
+/// Machine-readable reason for a transport restart. Shared across all transports.
+/// Human-readable strings are built by the UI (gettext).
+///
+/// The `#[repr(u8)]` layout doubles as the D-Bus wire format via `From<Self> for u8`
+/// and `TryFrom<u8> for Self`. Discriminants must remain stable across releases.
+/// Discriminant 0 is deliberately unused so that an all-zeroed message is recognisable
+/// as invalid by `TryFrom`.
+#[repr(u8)]
+#[derive(Debug, Clone, Copy, PartialEq, Eq)]
+pub enum TransportRestartReason {


Can we move this enum down so that members are ordered alphabetically?

iinuwa · 2026-11-10T13:36:07Z

    }

+    /// Stops offering transports that gave up for this request.
+    async fn drop_transports(&mut self, gone: &[Transport]) {


The way this is modeled requires the UI to keep state of all the transports it has seen and dropped. I think we should instead expose a Transports property and update that before we send TransportRestartReason::TransportUnavailable, so that the UI can either use that error as as a signal to check the Transports property or it can subscribe to Transports property changes directly. (this could also allow us to add transports during the ceremony if we want to.) This means that the UI and daemon always agree on what Transports are.

If you agree with this, I think it'd be good to do before the next release, but it can be a follow-up PR.

One major concern with this PR (rightfully so), was the complexity of the API. Introducing two steps instead of one (receiving TransportRestartReason::TransportUnavailable and then checking Transports) seems counterproductive?

iinuwa · 2026-11-10T13:40:43Z

 - daemon: Fix all subsequent requests being rejected after the UI failed to launch.
 - ui: Add Bulgarian translation (Thank you, @salif!)
 - webext: Try to read app ID from associated desktop entry (Thank you, @michaelbeaumont!)
 - ui: Basic recovery from failed attempts to use devices (e.g. flaky bluetooth)


we'll need to move these up to the unreleased section

They are already in that section?

ah, ignore that. After merging with main you mean. Yes, moved them.

msirringhaus requested a review from iinuwa September 9, 2026 14:45

msirringhaus force-pushed the cancellation_part4 branch from b2209ab to 852e47b Compare September 29, 2026 11:50

iinuwa reviewed Sep 29, 2026

View reviewed changes

msirringhaus added 7 commits October 30, 2026 11:35

Only send restart-signals to the UI, if the device was actively used

c90b7bc

Display restart reason as an error message to the user

33f8423

Update language files

dabc442

Make hybrid also use run_until_cancelled

c91b453

Break restart loop by disabling misbehaving transports after 5 tries

f50ebda

Update translation-files again

67ac426

Make cargo fmt happy

ba9fdb4

msirringhaus mentioned this pull request Nov 10, 2026

daemon: Offer devices based on libwebauthn's available transports #253

Merged

iinuwa requested changes Nov 10, 2026

View reviewed changes

kpanuragh mentioned this pull request Nov 11, 2026

Make security key selection more flexible #256

Open

msirringhaus added 3 commits November 11, 2026 16:34

Remove detect_transtports and only detect them once at init

29bf30b

Make TransportRestartReason a string instead of an u8

04043f7

Merge branch 'main' into cancellation_part4

4718250

msirringhaus force-pushed the cancellation_part4 branch from 32e1e76 to 4718250 Compare November 11, 2026 16:23

Conversation

msirringhaus commented Sep 9, 2026

Uh oh!

iinuwa left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

msirringhaus commented Oct 8, 2026

Uh oh!

iinuwa commented Oct 10, 2026

Uh oh!

iinuwa left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants