Refactor/ashell services crate

[MalpenZibo]

No description provided.

[romanstingler]

One blocker: the Bluetooth stream can die for good

changes() now runs on every InterfacesAdded/Removed, and it calls bluetooth.devices() just to get the device paths. devices() reads the properties of every device. If one of those devices disappears between GetManagedObjects and its property read, BlueZ answers UnknownObject, the ? fails the whole changes(), updates() hits the return at mod.rs:93, and the stream ends. After that no power, connect or device change ever reaches the UI until ashell restarts.

I reproduced it on real hardware. Discovery was on, with a couple of unpaired devices nearby. Whenever one of them showed up, I removed it again with RemoveDevice, which is what BlueZ does itself when a scanned device goes stale (TemporaryTimeout, 30s by default). The branch's code died every time, within the first 1 to 3 removals:

ERROR ashell_services::bluetooth] Failed to listen for bluetooth events: org.freedesktop.DBus.Error.UnknownObject: Method "GetAll" with signature "s" on interface "org.freedesktop.DBus.Properties" doesn't exist
[client] updates() stream ENDED

code	runs	result
this branch	3	stream ended in all 3, after 1 to 3 removals
main (old listener)	2	still alive after 39 and 51 removals in 60s

So this one is new in this PR. The old code built the event stream once at startup, so it only hit that path once. I also reproduced it deterministically with a mock BlueZ, and there the same race inside read_data() turns out to be fine: you get a warning, then the vanished device's InterfacesRemoved triggers a rebuild and it recovers. It's only the changes() path that's fatal.

What I'd suggest:

• in changes(), take the paths (and whether each device has Battery1) straight from GetManagedObjects instead of calling devices(). I counted the calls with dbus-monitor: with 4 devices a rebuild is 22 calls to BlueZ, and 9 of them are that devices() call plus Battery1 GetAll on devices that don't have a battery
• if one device proxy fails, skip that device instead of failing everything
• don't just retry on errors. When BlueZ isn't on the bus, every call to org.bluez makes the bus try to start it and log it (checked on a private dbus-daemon), so a retry loop would do that every time. The error name isn't reliable either: I got ServiceUnknown with no service file and Spawn.ChildExited when activation fails. I think it's simpler to check whether org.bluez has an owner and wait for NameOwnerChanged if it doesn't.

Smaller stuff

• nix-ci.yml is missing crates/** in the paths filter (ci.yml has it). All the Nix steps are gated on that filter, so a PR that only touches the crate skips the Nix build
• docs still show the old lint commands in code-style.md, building.md, ci-pipeline.md and common-tasks.md. project-layout.md still shows services/bluetooth/ with mod.rs and dbus.rs, and service-traits.md documents type Command: Send + 'static, but the trait in src/services/mod.rs doesn't have that bound
• Bluetooth::data() and the full feature aren't used anywhere
• every helper that execute calls builds a new BluetoothDbus, which means a full GetManagedObjects. I counted: Toggle does 2, Connect and Disconnect do 1 each. Building it once in execute and matching on the command would also get rid of the 7 pub wrapper methods
• info!("Listening for bluetooth events") fires on every rebuild, so on every InterfacesAdded/Removed. Probably should be debug!

[romanstingler]

I had a look, it is already coming up nicely.

My question is how separately used do you want to have it,
and by this I am asking about your opinion about possible need of documentation inside the crate folder?

Another thing is that we never run any tests in the CI.
Do we want to put that at least in the make check if you don't want to enable tests?

pre-release.yml runs cargo set-version without --workspace , do you want to keep it like that?

From my perspective it already looks fine. I ran multiple LLMs on it and they found

--- LLM (my opinion yes theoretically, practically very unlikely under normal circumstances)

One blocker: a signal flood during a read deadlocks the service for good

zbus queues at most 64 messages per match rule, and when a queue is full its socket reader waits (broadcast_direct(..).await in socket_reader.rs, no overflow mode). In updates(), the events stream isn't polled while read_data awaits the GetManagedObjects reply. If more than 64 matching signals arrive before that reply, the reader blocks on signal 65, the reply behind it is never read, and since there's no method timeout by default, nothing ever wakes up again. That includes NameOwnerChanged, so even a bluez restart doesn't recover it. Every execute() on the same handle hangs too, because it shares the connection.

The mock emits N Device1.RSSI PropertiesChanged inside its GetManagedObjects handler, then replies. A shown change triggers the read, and Powered is flipped 3s later:

N	snapshot after the read	Powered flip seen	execute(ConnectDevice)
30, 63, 64	yes	yes	ok
65, 100, 200	never	never	hangs (the mock answered, the client never read the reply)

main survives the same RSSI floods (N up to 1000).

The single PropertiesChanged rule is what makes this reachable. It has no sender, so it takes PropertiesChanged under /org/bluez from any peer, and the default system bus policy allows any local process to broadcast signals (<allow send_type="signal"/>). With a separate connection that does not own org.bluez sending 3 rounds of 1 spoofed Connected plus 100 RSSI, this branch triggered a read and then deadlocked. main ignored those signals. With real bluez alone you need more than 64 signals within one GetManagedObjects round trip. On my machine a 15s scan peaked at 2 signals in any 5ms window, so it needs a crowded room or a noisy media player. But the spoofed case is deterministic.

The general pattern isn't new: flooding InterfacesAdded inside the handler deadlocks main at 65 as well. This PR just widens it to every PropertiesChanged in the bluez tree from any sender.

Fix I verified against the same scenarios: keep consuming events while the read is in flight, re-read if anything arrived meanwhile, and add .sender(BLUEZ) to the rule. dbus-daemon filters a well-known sender at routing time, and zbus passes it client side (match_rule/mod.rs: "We can't match against a well-known name"), so the rule still matches bluez after restarts.

-            while let Some(event) = events.next().await {
+            // Something changed while the last read was in flight: read again
+            let mut stale = false;
+
+            loop {
+                if !stale {
+                    match events.next().await {
+                        Some(Event::Bluez(running)) => bluez_running = running,
+                        Some(Event::Changed) => {}
+                        None => return,
+                    }
+                }
+                stale = false;
                 // One action fires a burst of signals (pairing: Paired, Connected,
                 // Battery1, Percentage): read once for all those already queued.
-                let mut event = Some(event);
-                while let Some(current) = event {
-                    if let Event::Bluez(running) = current {
+                while let Some(event) = events.next().now_or_never().flatten() {
+                    if let Event::Bluez(running) = event {
                         bluez_running = running;
                     }
-                    event = events.next().now_or_never().flatten();
                 }
 
                 // Reading bluez while it isn't running would activate it
                 let data = if bluez_running {
-                    match Self::read_data(&conn).await {
+                    // Keep consuming events while waiting for the reply: once a zbus
+                    // signal queue is full, the connection stops reading replies too.
+                    let mut read = pin!(Self::read_data(&conn));
+                    let result = loop {
+                        match select(read.as_mut(), events.next()).await {
+                            Either::Left((result, _)) => break result,
+                            Either::Right((Some(event), _)) => {
+                                if let Event::Bluez(running) = event {
+                                    bluez_running = running;
+                                }
+                                stale = true;
+                            }
+                            Either::Right((None, _)) => return,
+                        }
+                    };
+                    match result {

         let rule = MatchRule::builder()
             .msg_type(Type::Signal)
+            .sender(BLUEZ)?
             .interface("org.freedesktop.DBus.Properties")?

With that patch: N=65 and N=200 both produce the snapshot and the Powered flip, and ConnectDevice returns. The spoofed flood causes zero reads. The restart, removal-storm and coalescing scenarios give the same results as before (same number of GetManagedObjects calls, same final state). I also ran the patched client against real bluez on the system bus, next to an unpatched one: power off, power on and a 15s scan produced the same 6 updates, at the same timestamps, in both. The window that remains is output.send(data).await blocking, but no reply is pending there, so it only delays things.

Smaller things worth fixing in this PR

• events() now fails the whole service if the rfkill watch can't be added for any reason other than NotFound (e.g. Inotify::init hitting max_user_instances). Before, Init was sent first, so the bar at least showed the state. Now the Bluetooth button never appears. Treating any watch error like NotFound (warn, then pending()) would keep the service usable.
• execute() activates bluez when it isn't running. It starts with BluetoothDbus::new(), a GetManagedObjects on org.bluez with auto-start, before looking at the command. On the private bus, Toggle, StartDiscovery and ConnectDevice with no bluez each produced one activation attempt (Spawn.ChildExited), including Toggle, which would then do nothing for Unavailable. ashell's UI hides the toggle when Unavailable, so it's mostly unreachable from the bar, but the crate API promises the opposite. zbus has #[zbus(no_autostart)] for proxy methods, so putting it on get_managed_objects (and the other bluez calls) would make "never activate bluez" hold by construction, and the owner check in updates() would become defense in depth.
• Devices with the same alias produce spurious updates. sort_by compares only name, over HashMap order, so two "JBL Flip" swap places between snapshots. The last != data dedupe sees a difference, and the rows jump around in the menu. With 20 changes that don't affect what's shown, I got 8 to 9 updates (two runs) where the only difference was the order. A tie-break fixes it: a.name.cmp(&b.name).then_with(|| a.path.cmp(&b.path)).

--- /LLM

[MalpenZibo]

About the separation: it makes sense in the long run to have a dedicated documentation that maybe could be linked in the website. For now, we do not have any real external consumers of these services, so it's not crucial. I think that we could open dedicated issues/prs about the documentation without slowing down this migration.

I will check for the CI issue