Skip to content

Retry bt-agent after bluetooth.service instead of skipping - #13258

Open
Bartok9 wants to merge 4 commits into
omacom:quattrofrom
Bartok9:bartok9/bt-agent-retry-after-bluez
Open

Bartok9 wants to merge 4 commits into
omacom:quattrofrom
Bartok9:bartok9/bt-agent-retry-after-bluez

Conversation

@Bartok9

@Bartok9 Bartok9 commented Sep 25, 2026 •

Copy link
Copy Markdown
Contributor

Problem

bt-agent.service used ExecCondition=systemctl is-active bluetooth.service. When that condition fails, systemd skips the unit. A skip is terminal for that activation — Restart=on-failure does not try again — so a race with bluez at login (or after systemctl restart bluetooth) leaves the session with no pairing agent. The radio still scans; pairing never completes.

Reported in #13247 with journal evidence (Skipped due to 'exec-condition' until bluetoothd is already up).

Change

  • Wait for bluetooth.service inside ExecStart, then exec bt-agent, so a late bluez is no longer a terminal skip and the Type=simple start job does not hold graphical-session.target.
  • Add After=graphical-session.target (same pattern as migrate-notify) so the unit cannot pin the session target open.
  • Do not add user-manager After=/Wants= on system bluetooth.service (those deps are inert in the user manager).
  • Keep ConditionPathIsDirectory=/sys/class/bluetooth so machines with no Bluetooth class still skip cleanly.
  • Update systemd-test.sh with directive-anchored assertions for the wait + ordering.

Test

  • ROOT=... bash test/shell.d/systemd-test.sh — bt-agent assertions pass.

Closes #13247

Fixes #8962

ExecCondition skip is terminal for the session, so a race with bluez at
login (or after restarting bluetooth) leaves no pairing agent. Wait in
ExecStartPre and order After=bluetooth.service so Restart=on-failure can
recover.

Closes omacom#13247
@llstrk

llstrk commented Sep 27, 2026

Copy link
Copy Markdown

Automated AI review

Community review: Independent automated community review, unaffiliated with the Omarchy team, intended to help prepare PRs for their review.

Verified: the core change fixes the reported failure. A failed ExecCondition ends as a skip, and systemd never restarts a skip, even with Restart=always. A failed ExecStartPre is an ordinary failure, so Restart=on-failure retries it after RestartSec=2. A late bluez at login and a mid-session systemctl restart bluetooth.service both recover now. The second case works because bluez-tools bt-agent exits 1 after bluez releases its agent.

Case Base (ExecCondition) Head (ExecStartPre wait)
bluez active within 30 s agent skipped if not yet active, never retried agent starts at the first active check
bluez still inactive after 30 s at login agent stays dead for the session fail, restart after 2 s, wait again
bluez restarted mid-session one retry after 2 s; skipped for the session if bluez is still down fail, restart after 2 s, wait again
Bluetooth kernel module not loaded skipped by ConditionPathIsDirectory skipped by ConditionPathIsDirectory (unchanged)

Verified: the ExecStartPre command line is parsed as intended. It contains no % specifiers, $( passes through systemd's variable expansion unchanged, and systemctl without --user queries the system manager. The 30-second wait stays within the default 90-second start timeout, and a roughly 32-second retry cycle never reaches the default start limit (5 starts in 10 s). The updated systemd-test.sh passes at head and fails against the base unit with the intended message.

Moving the wait into the start job has two side effects that the PR description does not mention.

The wait holds graphical-session.target for up to 30 seconds

bt-agent.service is WantedBy=graphical-session.target and has no After=graphical-session.target. Under systemd's default target dependencies, the target is therefore implicitly ordered After=bt-agent.service (systemd.target(5), "Default Dependencies"). A Type=simple start job does not finish until ExecStartPre has finished, so while bluez is inactive the target's start job waits:

login, bluez not yet active
  bt-agent.service      start job running: ExecStartPre loop, up to 30 s
  graphical-session.target   start job waiting (implicit After=bt-agent.service)
    omarchy-fcitx5, omarchy-crash-watch, omarchy-migrate-notify   waiting
    UWSM wayland-wm-app-daemon.service, app-graphical.slice   waiting if started in this window

At login, uwsm-app restarts the app daemon synchronously and fails if that restart blocks for more than 10 seconds. Omarchy uses it for the omarchy-hyprland-monitor-watch and udiskie autostarts (default/hypr/autostart.lua) and for launch keybindings. With a stubbed systemctl that blocked the daemon restart for 15 s, UWSM 0.27.0's uwsm-app.sh exited 141 with Timed out waiting for pipes!, sent a critical notification and did not start the app. With a 3 s block it launched normally.

Impact: on any login where bluez is late (the scenario this PR targets), the session target is delayed by bluez's lateness, up to 30 s. Launches in that window can fail if the delay exceeds about 10 s. The repository already guards against this pattern: omarchy-migrate-notify.service orders itself after the target "so this oneshot cannot hold the target open", and systemd-test.sh has a matching assertion.

Suggested change: add After=graphical-session.target to the unit, or wait inside ExecStart instead of ExecStartPre so the start job completes immediately (sketch in the next section).

This chain comes from systemd v262 and UWSM 0.27.0 source plus the shell-level uwsm-app.sh run above. It was not reproduced in a live session, and the UWSM version Omarchy installs was not checked. To confirm it, disable or mask bluetooth.service on a machine with an adapter, log in, and run systemctl --user list-jobs within 30 s.

Bluetooth module loaded but bluetooth.service off: clean skip becomes an endless retry

ConditionPathIsDirectory=/sys/class/bluetooth only tells whether the kernel's Bluetooth core is loaded: the bluetooth sysfs class is registered at module init, not per adapter (net/bluetooth/hci_sysfs.c, bt_sysfs_init()). When the module is loaded but bluetooth.service is disabled, masked or failed, the base unit skipped cleanly. The unit comment added with ExecCondition described its purpose as avoiding "entering a restart loop". The head repeats this cycle for the whole session:

ExecStartPre: 30 x (systemctl is-active; sleep 1)  -> exit 1
Failed with result 'exit-code'
Scheduled restart job (after RestartSec=2)          -> repeat, about every 32 s

Impact: about 110 cycles an hour, each starting roughly 60 short processes and writing several user-journal lines, plus the 30 s target hold above at every login. Omarchy's own Bluetooth toggle uses rfkill and leaves bluetoothd running, so it does not trigger this. The affected machines are those where the user has turned the service off, plus a failed bluetoothd.

First-run is affected by the same wait. install/user/first-run/enable-user-units.sh runs systemctl --user enable --now bt-agent.service ... under set -e. That call now waits up to 30 s, and it fails if bluez stays inactive (the old skip counted as success). In that case the script exits before the owe theme hook install, and first-run records the step as failed and runs the whole first-run sequence again at the next login.

Suggested change: one option is to keep Type=simple and wait inside ExecStart, then exec the agent. The start job then finishes at once and addresses both side effects. A machine with bluez off keeps a single polling shell instead of a restart loop, and Restart=on-failure still covers a bluez restart because bt-agent exits 1 when released:

ExecStart=/usr/bin/bash -c 'until systemctl is-active --quiet bluetooth.service; do sleep 2; done; exec /usr/bin/bt-agent -c NoInputNoOutput'

gdbus wait --system org.bluez from GLib could replace the polling. Neither variant was run in a live session.

Inert bluetooth.service ordering: the After= entry and Wants= line have no effect in a user unit. A user manager loads units only from user search paths, so it cannot see the system bluetooth.service. Wants= on a missing unit is dropped silently, and After=bluetooth.service orders against nothing (After=dbus.socket still applies). The new test's After=dbus.socket bluetooth.service assertion therefore pins an inert addition. Suggest keeping After=dbus.socket, dropping bluetooth.service from After= and the Wants=bluetooth.service line, and adjusting that assertion, or adding a comment that they are inert.

Optional test improvement: the updated test checks only text. A disposable copy with ExecStartPre=/usr/bin/true still passes it. A check that the unit's wait command calls is-active on bluetooth.service (wherever the wait ends up) would at least pin the wait.

Related open PRs: #9131 and #11937 change the same unit and test. #9131 uses the same ExecStartPre wait with explicit start limits; #11937 waits inside ExecStart through a supervisor script. Their tests conflict with this one, so one approach needs to be chosen for the unit and its test together.


Review information

Test scope: source review of the pinned head against systemd v262, UWSM 0.27.0, bluez-tools and Linux Bluetooth sources. Sandboxed runs covered systemd-test.sh at head and base, bluetooth-test.sh at head, a mutant unit, the ExecStartPre loop with a stubbed systemctl, and UWSM's uwsm-app.sh with a stubbed blocking daemon restart. No real systemd user manager, Bluetooth hardware or desktop session was used, so the target hold and launch failures are inferred from source and the shell-level run, not observed live.

AI process: Opus 5.5 Medium coordination and synthesis, Opus 5.5 Xhigh technical review and final fact check, GPT 6 Sol Xhigh search for related issues, Opus 5.5 Medium editorial check.

Opt out: To stop receiving these reviews, reply to this comment saying so.

@Bartok9

Bartok9 commented Sep 27, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the thorough review — agreed on the side effects.

Pushed follow-ups that:

  • Move the bluez wait into ExecStart (until is-active; do sleep 2; done; exec bt-agent) so the start job completes immediately and does not hold graphical-session.target / uwsm-app.
  • Add After=graphical-session.target (same pattern as migrate-notify).
  • Drop inert user-manager After=/Wants= on system bluetooth.service.
  • Tighten systemd-test.sh accordingly (no ExecStartPre, no system-unit deps, wait + bt-agent still on ExecStart).

Core fix vs ExecCondition skip remains: readiness failure is no longer a terminal skip for the session.

@llstrk

llstrk commented Sep 28, 2026

Copy link
Copy Markdown

Automated AI review

Community review: Independent automated community review, unaffiliated with the Omarchy team, intended to help prepare PRs for their review.

Outcome: The PR's systemd-test.sh fails at 3549e46.

Earlier review:

systemd-test.sh fails

grep -F 'After=graphical-session.target' misses After=dbus.socket graphical-session.target, so it exits 1. Two later checks also match comments: the unit's line 6 trips one, and a comment satisfies Restart=on-failure. Impact: the file's later checks never run.

Suggested change: anchor each pattern to a directive line (block in the reproducer).

Reproducer (fails at 3549e46)

Reproducer: systemd-test.sh fails at 3549e46

From a checkout of the PR head (3549e46), repository root:

$ bash test/shell.d/systemd-test.sh; echo "exit=$?"
not ok - bt-agent must not hold graphical-session.target open
exit=1

The unit's only After= line is After=dbus.socket graphical-session.target, so the fixed-string pattern After=graphical-session.target never matches:

$ grep -nF 'After=graphical-session.target' default/systemd/user/bt-agent.service; echo "exit=$?"
exit=1

If that line is fixed on its own, the next assertion fails too, because the new comment on line 6 matches the forbidden pattern:

$ grep -nE 'Wants=.*bluetooth\.service|After=.*bluetooth\.service' default/systemd/user/bt-agent.service
6:# Do not After=/Wants= system bluetooth.service: the user manager cannot

(Checked with a disposable copy of the unit where After= is split into After=dbus.socket and After=graphical-session.target: the test then stops with not ok - bt-agent must not depend on inert system bluetooth.service from the user manager.)

The Restart=on-failure check now uses grep -F (it was grep -Fx), and a comment line also contains that text, so the check still passes when the directive is deleted:

$ grep -nF 'Restart=on-failure' default/systemd/user/bt-agent.service
20:# is off while /sys/class/bluetooth still exists; Restart=on-failure still
28:Restart=on-failure

(Checked with a disposable copy that also split After=, reworded the line 6 comment and removed line 28: the whole file passed.)

Because a shell test file exits at its first failed assertion, every later check in systemd-test.sh (sleep lock, migration notifier, fcitx5, oomd) is skipped while this one fails.

Suggested replacement for the bt-agent block (lines 7 to 17)

This anchors each pattern to the start of a directive line, so comments cannot satisfy or trip it:

service="$ROOT/default/systemd/user/bt-agent.service"
grep -E '^ExecCondition=' "$service" >/dev/null && fail "bt-agent ExecCondition skip is terminal across a bluez race"
grep -E '^ExecStartPre=' "$service" >/dev/null && fail "bt-agent ExecStartPre wait would hold graphical-session.target"
grep -E '^After=.*graphical-session\.target' "$service" >/dev/null || fail "bt-agent must not hold graphical-session.target open"
grep -E '^(Wants|After)=.*bluetooth\.service' "$service" >/dev/null && fail "bt-agent must not depend on inert system bluetooth.service from the user manager"
grep -E '^ExecStart=.*is-active --quiet bluetooth\.service.*exec /usr/bin/bt-agent ' "$service" >/dev/null || fail "bt-agent must wait for bluetooth.service inside ExecStart, then exec bt-agent"
pass "bt-agent waits for bluetooth.service without holding the session target"

grep -Fx 'Restart=on-failure' "$service" >/dev/null || fail "bt-agent lost Restart=on-failure"
pass "bt-agent still restarts after runtime failures"

Results of the full systemd-test.sh with this block, against the unchanged 3549e46 unit and disposable variants of it:

Unit Exit Output
3549e46 unit, unchanged 0 ok - bt-agent waits for bluetooth.service without holding the session target, ok - bt-agent still restarts after runtime failures, then the rest of the file passes
base unit (7b336b1) 1 not ok - bt-agent ExecCondition skip is terminal across a bluez race
earlier PR unit (ec28d53) 1 not ok - bt-agent ExecStartPre wait would hold graphical-session.target
Restart=on-failure line deleted 1 not ok - bt-agent lost Restart=on-failure
graphical-session.target dropped from After= 1 not ok - bt-agent must not hold graphical-session.target open
Wants=bluetooth.service added 1 not ok - bt-agent must not depend on inert system bluetooth.service from the user manager
ExecStartPre=/usr/bin/true added 1 not ok - bt-agent ExecStartPre wait would hold graphical-session.target
ExecStart=/usr/bin/bt-agent -c NoInputNoOutput (no wait) 1 not ok - bt-agent must wait for bluetooth.service inside ExecStart, then exec bt-agent

Safety comment lost pairable: true

Line 24 now reads # is only when the user .... Impact: the comment no longer states the condition it gives for auto-accept being safe.

Suggested change: restore `pairable: true`.

Reproducer

Reproducer: the auto-accept safety comment lost its key phrase

At 3549e46 (the change came in 0ab5f5d):

$ grep -n 'is only' default/systemd/user/bt-agent.service
24:# is only  when the user explicitly opens the omarchy

At the base (7b336b1):

$ grep -n 'is only' default/systemd/user/bt-agent.service
15:# is only `pairable: true` when the user explicitly opens the omarchy

At the earlier PR head (ec28d53):

$ grep -n 'is only' default/systemd/user/bt-agent.service
17:# is only `pairable: true` when the user explicitly opens the omarchy

The diff of 0ab5f5d shows the change on that line:

-# is only `pairable: true` when the user explicitly opens the omarchy
+# is only  when the user explicitly opens the omarchy

The backtick-quoted pairable: true was dropped, which leaves a double space and a sentence that no longer says when auto-accepting is safe.

Details

Verified (sandbox runs of the unit's ExecStart script with stubbed systemctl and bt-agent, plus systemd, bluez and bluez-tools source):

  • The wait runs in ExecStart, so the Type=simple start job completes at fork. After=graphical-session.target removes the target's implicit ordering after the unit without creating a cycle, the same pattern as omarchy-migrate-notify.service.
  • bluez inactive, then active: the shell polls every 2 s, then execs bt-agent -c NoInputNoOutput in the same PID, and bt-agent's exit status passes through to systemd.
  • bluez off: one shell calls systemctl is-active every 2 s, with no restarts and no journal lines. SIGTERM ends the waiting shell at once. enable --now in first-run no longer waits for bluez.
  • A mid-session systemctl restart bluetooth.service still recovers: after Release, bt-agent's UnregisterAgent call fails and it exits 1, so Restart=on-failure fires (matching the journal in bt-agent.service is permanently skipped when its ExecCondition races bluetooth.service, leaving the session with no Bluetooth pairing agent #13247).
  • The test's ExecCondition and ExecStartPre checks fail with their intended messages against the base unit and the ec28d53 unit, respectively.

PR description: it still describes After=/Wants=bluetooth.service, a bounded ExecStartPre wait and passing bt-agent assertions, which no longer match the head.

Related open PRs: #9131 and #11937 still conflict with this PR in both files. #13620 (another fix for #13247) also conflicts in both files, and #8929, #12897 and #9965 edit the same ExecStart block.


Review information

Test scope: Source and sandbox tests only at 3549e46; no real systemd user session or bluez.

AI process: Opus 5.5 Medium coordination and synthesis, Opus 5.5 Xhigh technical review and final fact check, GPT 6 Sol Xhigh search for related issues, Opus 5.5 Medium editorial check.

Opt out: To stop receiving these reviews, reply to this comment saying so.

Anchor systemd-test greps to directive lines so combined After=
and comments cannot false-fail/false-pass. Restore pairable: true
in the safety comment.
@Bartok9

Bartok9 commented Sep 28, 2026

Copy link
Copy Markdown
Contributor Author

Thanks — fixed on head:

  • Anchored the bt-agent block in systemd-test.sh to directive lines (^After=.*graphical-session.target, ^(Wants|After)=…, ^ExecStart=…is-active…exec bt-agent, grep -Fx Restart=on-failure) so combined After= and comments no longer false-fail/false-pass.
  • Restored `pairable: true` in the safety comment and reworded the inert-deps comment so it does not trip the bluetooth.service dependency check.
  • PR description updated to match the ExecStart wait (no ExecStartPre / no system-unit Wants).

systemd-test.sh passes locally at the new head.

@omarchybot omarchybot added verified Omarchy Triage has verified that this issue is ready for final review ready Good to merge labels Oct 3, 2026
@omarchybot

Copy link
Copy Markdown
Collaborator

Reviewed at ae62e23 and reproduced on a disposable Omarchy VM (systemd 261.2, bluez 5.87, bluez-tools 0.2.0). It fixes the bug, and nothing was found that needs changing.

Reproduction. With bluetooth.service masked to keep it down and then restored, the base unit was skipped for the rest of the session in all three cases: starting while bluez was down, a mid-session bluez restart (same journal as #13247: Agent released, exit 1, Skipped due to 'exec-condition'), and first-run's systemctl --user enable --now. At this head the unit stays active as one waiting shell, enable --now returns at once, and bt-agent registers within one poll of bluez coming back, each time. No ordering cycle from After=graphical-session.target.

Tests. On the same VM, systemd-test.sh, bluetooth-test.sh and config-test.sh pass, and the new systemd-test.sh assertions fail against the base unit. ./test/cli stops at vscode generated theme references current theme file, and it fails the same way at the base commit, so that isn't from this PR. The PR reports no CI checks.

Second opinion. Codex Medium reviewed the diff and found no defect that changes behaviour, which agrees with my review (its independence isn't guaranteed). It raised two smaller points that I verified. Neither blocks the PR, and I've left both alone. First, the new assertions match directive text, so a broken loop (while for until) would still pass. The reproduction above is what shows the behaviour. Second, the comment saying an ExecStartPre wait "would hold graphical-session.target" isn't true once the unit has After=graphical-session.target.

Competing fixes. #9131, #11937 and #13620 change the same unit for the same bug. Codex was asked to compare all four without being told my answer, and it chose this one, as I did. #9131 (bounded ExecStartPre wait) makes first-run's enable --now wait up to 30 s and fail under set -e when bluez is off, then retries about every 32 s. #13620 fails and restarts about every 2 s while bluez is off, and its After=bluetooth.service has no effect in the user manager. #11937 adds a permanent supervisor polling busctl every 2 s even when healthy, which is more machinery than this failure needs. #11936 (an agent left stale rather than exiting) may still be a separate case.

I've linked #8962 as well, since it's the same login race. This now waits on the maintainer.

@Bartok9

Bartok9 commented Oct 3, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for reproducing this on a real Omarchy VM and comparing the competing fixes.

Glad the three failure cases (start while bluez is down, mid-session bluez restart, first-run enable --now) recover at head, and that the ./test/cli theme failure is pre-existing. Noted the two non-blocking nits (directive-text assertions wouldn't catch while vs until; the ExecStartPre comment is slightly overstated once After=graphical-session.target is set). Leaving both as-is unless a maintainer wants them tightened.

Happy to rebase or adjust if anything else comes up. Waiting on maintainer review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working ready Good to merge verified Omarchy Triage has verified that this issue is ready for final review

Projects

None yet

3 participants