fix(linux): keep the generated systemd unit out of the user's tier - #796
Open
AalmanSadath wants to merge 5 commits into
Open
fix(linux): keep the generated systemd unit out of the user's tier#796AalmanSadath wants to merge 5 commits into
AalmanSadath wants to merge 5 commits into
Conversation
`launch_at_login` wrote its unit to `$XDG_CONFIG_HOME/systemd/user`, the highest-precedence tier in systemd's user load path and the one reserved for units the user authors. Three consequences followed. A hand-edited unit could not survive: `reconcile` runs on every agent start and on every config reload, and rewrote the file whenever its content differed, so an added `Environment=` or a changed `RestartSec` was reverted within one restart, silently. Nothing marked the file as generated, so a deliberate edit and a stale artifact were indistinguishable. A packaged unit was shadowed permanently. For a package install the generated copy is byte-identical to `/usr/lib/systemd/user/openlogi-agent.service` — same directives, same `ExecStart` — so it added no capability, only precedence. Later changes to the packaged unit would never reach anyone who used the toggle, and `uninstall.sh` could not remove the copy, since no package script can reach a home directory. `ExecStart` also tracked whichever binary started last, so running a build from source once pinned autostart to a path inside the build tree, which a later `cargo clean` turned into a unit that fails at every login while the installed binary sits unused. Now: - A packaged unit that already launches this binary is simply enabled; nothing is generated. Ownership stays with the package manager, and removing the package removes the unit. - Otherwise the unit is generated under `$XDG_DATA_HOME/systemd/user`, the tier systemd reserves for units installed on a user's behalf. It still outranks the packaged unit but sits below the user's own, so a hand-written unit wins without any provenance check. - The file earlier versions left in the config tier is removed, but only when it is provably ours: one template has ever shipped, parameterised solely by `ExecStart`, so splicing a file's own `ExecStart` line back into that template reproduces it byte for byte if and only if we rendered it. Anything else is left in place and keeps winning. `render_unit` is split so the provenance check can splice an `ExecStart` line in verbatim — re-running the escaper over an escaped value would double `%%` into `%%%%` and stop a unit we wrote from matching itself. The test modules move to sibling files under `launch_agent/`; those lines are relocated unchanged, and the split keeps the module near the workspace's file-size guideline. Verified against the built agent with the XDG bases redirected: a packaged unit naming this binary generates nothing and enables the packaged one; a redundant generated unit beside it is removed; a packaged unit naming a different binary still generates one for the running path; a stale unit in the config tier is removed; and a hand-edited one is left byte-identical. Refs AprilNEA#763.
Greptile SummaryThe PR moves generated Linux systemd units from the user configuration tier to the XDG data tier while preserving packaged and user-authored units.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| crates/openlogi-agent/src/launch_agent.rs | Reworks Linux autostart reconciliation around systemd load-tier ownership, packaged-unit detection, provenance checks, and enablement tracking. |
| crates/openlogi-agent/src/launch_agent/linux_tests.rs | Adds focused coverage for generated-unit provenance, systemd escaping, executable comparison, and XDG path placement. |
| crates/openlogi-core/src/paths.rs | Adds a raw XDG data-home resolver parallel to the existing config-home resolver. |
| docs/INSTALL-linux.md | Documents generated and user-authored unit locations and persistent systemd customization. |
| packaging/linux/systemd/openlogi-agent.service | Updates packaged-unit guidance to reflect the new single-owner tiering model. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Reconcile launch_at_login] --> B{Legacy config-tier unit generated by OpenLogi?}
B -->|Yes| C[Remove legacy unit and reload systemd]
B -->|No| D[Preserve config-tier unit]
C --> E{Setting enabled?}
D --> E
E -->|No| F[Disable only when ownership marker exists]
E -->|Yes| G{Matching packaged unit exists?}
G -->|Yes| H[Remove redundant generated unit]
H --> I[Enable packaged unit]
G -->|No| J[Write or retain data-tier generated unit]
J --> K[Enable service]
Reviews (5): Last reviewed commit: "fix(linux): claim the autostart enableme..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Review catch on AprilNEA#796. With `launch_at_login` off and nothing of ours on disk, reconcile called `systemctl --user disable openlogi-agent.service` unconditionally. `disable` acts on the unit *name*, resolving to whichever file wins the load path — so a unit the user wrote in their own config tier, and enabled themselves, was silently un-enabled on every agent start. That contradicts the change it shipped alongside, which exists to hand that tier back to the user. Deleting the call is not the fix: the packaged-unit branch writes nothing, so without it, turning the toggle off would leave a packaged unit enabled and autostart would survive a setting that says it should not. Withdraw an enablement only where this app could have made one — a packaged unit for this binary, and only while the user has not authored their own. `user_authored_unit_present` reuses the round-trip provenance check already used by the migration, so this adds a caller rather than a mechanism. The enable path stays unconditional on purpose, and the asymmetry is recorded in a comment: enabling is what the user just asked for, whereas the enablement withdrawn here may be one they made outside OpenLogi. Verified with a `systemctl` shim recording invocations, agent run against redirected XDG bases: packaged user-authored toggle disable -- yes off not called yes yes off not called yes -- off called -- -- off not called Reverting just this predicate reproduces the reported behaviour: the same user-authored scenario calls `systemctl --user disable`. Refs AprilNEA#763.
…unit Second review round on AprilNEA#796, covering two ways the previous commit still reached past what this app owns. `systemctl --user enable` records that a unit is enabled, never who asked, and the unit name is shared with whatever a package installs. The previous gate inferred ownership from surrounding state, which cannot distinguish the two causes of an identical situation. Following the documented install — install the package, run `systemctl --user enable --now openlogi-agent.service`, never touch the GUI toggle — the agent reconciled the default disabled setting and disabled its own autostart on first run. `disable` does not stop a running unit, so nothing looked wrong until the next login. Enablement is now recorded explicitly: a marker beside the config notes an enablement this app made, and `disable` runs only against one. Losing the marker fails safe, leaving autostart working. The data tier is also not exclusively this app's. Both deletes there ran unconditionally, so a unit another tool or the user installed under the same name was removed — the config-tier round-trip check guarded the migration but not the deletes one tier down, which moved the squatting rather than ending it. Every delete is now gated on the same check, and a unit that does not round-trip is left alone entirely, with the setting honoured through enablement alone. `user_authored_unit_present` is gone: the marker subsumes it, and the rule is now uniform across both user-writable tiers rather than special-cased per tier. Verified with a `systemctl` shim recording invocations, across nine combinations of packaged unit, data-tier file, marker, and setting. Reverting either guard reproduces the corresponding failure: the documented install disables itself, and a user's data-tier unit is deleted. Refs AprilNEA#763.
…rms it Review catch on AprilNEA#796. `run_systemctl` swallowed failures into a warning and returned nothing, so the ownership marker was written even when `enable` never happened — no session bus, unit not found, systemctl missing. A later reconcile then read that marker as proof and disabled an enablement this app never made, which is the failure the marker exists to prevent. `run_systemctl` now reports whether the command succeeded. The marker is written only after `enable` confirms, and is deliberately kept when `disable` fails, so the next reconcile retries rather than stranding an enablement this app is still responsible for. Claiming an enablement the user made by hand stays as it is, and the reasoning is now recorded on `enable_unit`: reaching the toggle is an explicit request for this app to manage autostart, so the setting has to mean what it says when switched back off. Declining there would leave the agent starting at login while the GUI reports otherwise — the same lie about system state this branch set out to remove. Verified with a systemctl shim failing a chosen verb: a failed enable leaves no marker, a failed disable keeps it, and both success paths are unchanged. Refs AprilNEA#763.
Review catch on AprilNEA#796. `enable_unit` enabled the unit first and recorded the claim afterwards, so a marker write that failed — read-only data directory, full disk — left autostart running with nothing to withdraw it. A later reconcile of a disabled setting would find no marker, skip `systemctl disable`, and leave the service starting at login while the GUI reports it off. The module already treats a read-only autostart directory as an expected condition, so this is not a remote failure. Ordering is what makes it safe. The claim is recorded first; if it cannot be recorded, autostart is left alone rather than turned on untrackably. If `systemctl` then fails, the claim is dropped, which keeps the previous round's property that no marker survives an enablement that never happened. The trade is deliberate: on a read-only data directory the toggle now does nothing and warns, where before it enabled autostart that nothing could turn off. An enablement the app cannot withdraw is the worse of the two, since the setting is what the GUI reports. Verified with a systemctl shim and a read-only data directory: `enable` is not issued when the marker cannot be written, a failed enable leaves no marker, a failed disable keeps it for retry, and both success paths are unchanged. Refs AprilNEA#763.
davidbudnick
added
type: bug
AprilNEA
requested changes
Aug 22, 2026
AprilNEA
left a comment
AprilNEA
left a comment
Owner
There was a problem hiding this comment.
Two Linux autostart ownership issues still need to be addressed before merge: preserve an existing ownership marker across a transient re-enable failure, and resolve the effective highest-precedence system unit rather than any lower matching entry. Please add automated regression coverage for both state transitions; the current helper tests do not exercise them.
| return; | ||
| } | ||
| if !run_systemctl(&["enable", UNIT_NAME]) { | ||
| clear_enablement_marker(); |
Owner
There was a problem hiding this comment.
Preserve an existing claim when a redundant enable fails. record_enablement() returns success both when this call creates the marker and when a marker from an earlier successful enable already exists. In the latter case, a transient failure from this repeated systemctl enable clears valid ownership here even though the previously created enable symlink may remain. A later disabled reconcile then sees no marker, skips disable, and leaves autostart active while the setting reads off. Please distinguish a newly created marker from a pre-existing one and only roll back the former; add a regression for existing marker + enable failure.
| SYSTEM_UNIT_DIRS | ||
| .iter() | ||
| .map(|dir| Path::new(dir).join(UNIT_NAME)) | ||
| .find(|path| { |
Owner
There was a problem hiding this comment.
Check the highest-precedence existing unit, not the first unit whose executable matches. This predicate skips an existing higher-tier unit when its ExecStart differs and can then accept a lower matching unit. For example, if /usr/local/share/systemd/user/openlogi-agent.service names an old binary while /usr/lib/systemd/user/openlogi-agent.service names the running one, this returns the /usr/lib file and removes the generated data-tier override, but systemd actually loads the higher /usr/local/share file. The probe should resolve the effective first existing unit in the real user load path and only reuse it when that unit matches. The hard-coded path set also omits $XDG_CONFIG_DIRS/systemd/user, /run/systemd/user, and custom $XDG_DATA_DIRS, so please cover the actual precedence rather than a partial static list.
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
3 participants
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.
Summary
The third item in #763, and the only behavioural one:
launch_at_loginwrote its unit to$XDG_CONFIG_HOME/systemd/user— the highest-precedence tier in systemd's user load path, and the one reserved for units the user writes themselves.Three consequences followed.
A hand-edited unit could not survive.
reconcileruns on every agent start and every config reload, rewriting the file whenever its content differed. An addedEnvironment=, a changedRestartSec, aWantedBy=default.target— reverted within one restart, with no message. Nothing marked the file as generated, so a deliberate edit and a stale artifact were indistinguishable.A packaged unit was shadowed permanently. For a package install the generated copy is byte-identical to
/usr/lib/systemd/user/openlogi-agent.service— same directives, sameExecStart. It added no capability, only precedence. Later changes to the packaged unit would never reach anyone who used the toggle, anduninstall.shcannot remove the copy, because no package script can reach a home directory.ExecStarttracked whichever binary started last. Running a build from source once pinned autostart to a path inside the build tree; a latercargo cleanleft a unit that fails at every login while the installed binary sits unused.Changes
openlogi-agent—launch_agent.rsExecStartagainst the running executable, resolved through symlinks.$XDG_DATA_HOME/systemd/user— the tier systemd reserves for units installed on a user's behalf. It still outranks the packaged unit, but sits below the user's own, so a hand-written unit wins with no provenance check needed.ExecStart, so splicing a file's ownExecStartline back into that template reproduces it byte for byte if and only if we rendered it. Anything carrying another directive is the user's: left in place, still winning.render_unitis split fromrender_unit_with_execso that check can splice a line in verbatim. Re-running the escaper over an already-escaped value would double%%into%%%%and stop a unit we wrote from matching itself.openlogi-core—xdg_data_home(), the counterpart to the existingxdg_config_home().Docs —
INSTALL-linux.mddescribes both paths and states that~/.config/systemd/userstays the user's, pointing atsystemctl --user editfor a drop-in that survives upgrades. The packaged unit's header comment no longer advertises the two copies as coexisting.The test modules move to sibling files under
launch_agent/; those lines are relocated unchanged, and the split keeps the module near the workspace's file-size guideline.Testing
tests (macos)did not run — this is a Linux host. The diff is cfg-gated, so that is worth stating plainly: Linux is covered natively (both host and target of every new function) and the Windows proxy passed, but macOS is unverified here. Every new function is#[cfg(target_os = "linux")]and no macOS path changed. rustdoc was also run directly againstopenlogi-agent, which CI's rustdoc job excludes.12 new unit tests cover the provenance check: a rendered unit is recognised for any executable path, including ones needing
%,$, space and quote escaping; an added, changed, or dropped directive is not ours; twoExecStartlines is not a shape the renderer can emit; escaping round-trips; symlinked paths compare equal.Runtime-verified against the built agent with the XDG bases redirected to a scratch tree, on Fedora 44 / systemd 259:
Environment=RUST_LOG=debug)The packaged cases were exercised by placing a unit at
/usr/lib/systemd/user/openlogi-agent.service; the load-path precedence was confirmed withsystemd-analyze --user unit-pathsandsystemctl --user show -p FragmentPath.Notes
The template has never changed since Linux autostart shipped in v0.6.8 — byte-identical across every commit that touched it — so the provenance check needs no template history. The one intermediate rendering that differed (
$escaping) existed for 71 minutes on the #172 branch and never shipped.One gap remains, unchanged by this PR:
uninstall.shcannot clean a home directory, so a config-tier unit from an older install survives if a user upgrades and uninstalls without ever launching the agent. Package scripts have never been able to reach$HOME.Fixes #763.