wrappers/internal: Sanitize systemd user services#16686
wrappers/internal: Sanitize systemd user services#16686sergio-costas wants to merge 1 commit intocanonical:masterfrom
Conversation
|
Fri Feb 27 09:43:28 UTC 2026 No spread failures reported |
In some cases, snapd insists on enabling user services not only globally, as expected (thus, adding a symlink at `/etc/systemd/user/XXXXX.target.wants`), but also locally (thus, at $HOME/.config/systemd/user/XXXX.target.wants`). This means that if an user service is migrated from `default.target` to `graphical-session.target` (or vice-versa), the soft link at the user's HOME folder will be in the wrong subfolder. Unfortunately, the main snapd service seems to not have the capability of accessing and managing the local user services, having to rely on the user's local daemon (`snap userd`), which runs as an user's service. So a change in that daemon (which has its source code at `usersession/userd` folder) is required to check the locally enabled user services and ensure that their soft link is in the right place. This commit does this by checking the links in the user folder every time user session daemon is launched, and runs as the user a `systemctl --user reenable SNAP-SERVICE-NAME` to rebuild the wrong softlink.
2da0b29 to
1d2a0c0
Compare
There was a problem hiding this comment.
Pull request overview
Updates the user-session daemon (snap userd) to sanitize locally-enabled systemd user services under $HOME/.config/systemd/user/*.target.wants, correcting stale/misplaced symlinks when a snap user service’s WantedBy target changes (e.g. default.target ↔ graphical-session.target).
Changes:
- Add startup-time scan of
$HOME/.config/systemd/user/*.target.wants/snap.*and re-enable services whoseWantedByno longer matches the.target.wantsdirectory. - Remove broken symlinks for snap user services found under local
*.target.wantsdirectories. - Introduce
gopkg.in/ini.v1dependency to parseWantedByfrom unit files.
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| usersession/userd/userd.go | Implements the local user-service sanitation logic at userd startup (scan, broken-link cleanup, and reenable). |
| go.mod | Adds gopkg.in/ini.v1 module requirement for unit file parsing. |
| go.sum | Records checksum entries for the newly introduced dependency chain. |
| wantedBy := "" | ||
| if section := targetIni.Section("Install"); section != nil { | ||
| wantedBy = section.Key("WantedBy").String() | ||
| } | ||
| if wantedBy != targetName { | ||
| // the symlink is in the wrong folder. Re-enable the service | ||
| // to change it. | ||
| err := reenableUserService(path.Base(snapTarget)) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| } |
There was a problem hiding this comment.
WantedBy in a unit file can contain multiple targets and often includes trailing whitespace/newlines. Comparing section.Key("WantedBy").String() directly against targetName can trigger unnecessary reenable calls. Consider trimming whitespace and checking whether targetName is one of the WantedBy entries (e.g. split with strings.Fields or read repeated keys) before deciding the symlink is misplaced.
| } | ||
| } | ||
|
|
||
| sanitizeUserServices() |
There was a problem hiding this comment.
sanitizeUserServices() returns an error but Init() ignores it. If the intent is best-effort cleanup, at least log the returned error so failures are diagnosable; otherwise, propagate the error to avoid silently leaving incorrect symlinks behind.
| sanitizeUserServices() | |
| if err := sanitizeUserServices(); err != nil { | |
| logger.Noticef("cannot sanitize user services: %v", err) | |
| } |
| github.com/rogpeppe/go-internal v1.6.1 // indirect | ||
| golang.org/x/exp v0.0.0-20240416160154-fe59bbe5cc7f // indirect | ||
| golang.org/x/term v0.20.0 // indirect | ||
| gopkg.in/ini.v1 v1.67.1 // indirect |
There was a problem hiding this comment.
userd.go imports gopkg.in/ini.v1, so this dependency is direct. In go.mod it’s currently listed in the indirect require block with // indirect, which will cause go mod tidy churn. Move it to the main require block (or drop the // indirect marker) so module metadata matches actual usage.
| func sanitizeUserServices() error { | ||
| // ensure that the user services enabled in the $HOME folder are | ||
| // correctly placed in the right .target.wants folder. This placement | ||
| // will be wrong if a service is moved from default.target to | ||
| // graphical-session.target or vice-versa, so this clean up is required. | ||
| // | ||
| // The change can happen in two cases: | ||
| // | ||
| // * when migrating from an old version of snapd without "graphical-session.target" | ||
| // support, to a new version that supports it: all the user daemons' WantedBy entry | ||
| // will be updated, and so these entries will have to be updated too. | ||
| // * when a snap adds or removes the `desktop` plug in a daemon | ||
|
|
||
| userSystemdQuery := path.Join(os.Getenv("HOME"), ".config", "systemd", "user", "*.target.wants") | ||
| entries, err := filepath.Glob(userSystemdQuery) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| for _, targetWantPath := range entries { | ||
| snapTargetPaths, err := filepath.Glob(path.Join(targetWantPath, "snap.*")) | ||
| if err != nil { | ||
| logger.Noticef("cannot get the user service files at %s: %s", targetWantPath, err.Error()) | ||
| continue | ||
| } | ||
| for _, snapTarget := range snapTargetPaths { | ||
| // Remove any broken link (that's a service that was removed) | ||
| if err = clearBrokenLink(snapTarget); err != nil { | ||
| logger.Noticef("cannot remove the broken link %s: %s", snapTarget, err.Error()) | ||
| continue | ||
| } | ||
| // Check that any snap service is in the right .target.wants | ||
| targetName := strings.TrimSuffix(targetWantPath, ".wants") | ||
| if err := checkServicePlacement(path.Base(targetName), snapTarget); err != nil { | ||
| logger.Noticef("cannot process user service at %s for %s: %s", snapTarget, targetName, err.Error()) | ||
| continue | ||
| } | ||
| } | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
New startup behavior (sanitizeUserServices, checkServicePlacement, and the systemctl reenable path) isn’t covered by unit tests, but this package already has a substantial test suite. Adding tests for at least: (1) broken symlink cleanup, and (2) wrong .target.wants placement triggering a single systemctl --user reenable call (using systemd.MockSystemctl) would help prevent regressions.
| func reenableUserService(serviceName string) error { | ||
| logger.Noticef("re-enabling user service %s", serviceName) | ||
| sysd := systemd.New(systemd.UserMode, nil) | ||
| return sysd.DaemonReEnable([]string{serviceName}) | ||
| } |
There was a problem hiding this comment.
systemd.Systemd does not define DaemonReEnable anywhere in this repo, so sysd.DaemonReEnable(...) will not compile. Either add a corresponding method to the systemd package (wrapping systemctl --user reenable ...) or invoke systemctl --user reenable via an existing systemd API (e.g. adding a small helper in systemd and calling that here).
| // will be updated, and so these entries will have to be updated too. | ||
| // * when a snap adds or removes the `desktop` plug in a daemon | ||
|
|
||
| userSystemdQuery := path.Join(os.Getenv("HOME"), ".config", "systemd", "user", "*.target.wants") |
There was a problem hiding this comment.
sanitizeUserServices builds its glob using os.Getenv("HOME") without checking for empty/undefined HOME. If HOME is empty, the glob becomes relative (e.g. .config/systemd/user/*.target.wants) and userd may scan/remove links under the current working directory. Use os.UserHomeDir() (or validate HOME is non-empty) and return early when no home dir is available.
| userSystemdQuery := path.Join(os.Getenv("HOME"), ".config", "systemd", "user", "*.target.wants") | |
| homeDir, err := os.UserHomeDir() | |
| if err != nil || homeDir == "" { | |
| logger.Noticef("cannot determine user home directory, skipping user service sanitization: %v", err) | |
| return nil | |
| } | |
| userSystemdQuery := path.Join(homeDir, ".config", "systemd", "user", "*.target.wants") |
| func clearBrokenLink(targetPath string) error { | ||
| _, err := filepath.EvalSymlinks(targetPath) | ||
| if err != nil { | ||
| switch err.(type) { | ||
| case *fs.PathError: | ||
| logger.Noticef("deleting broken link in snap service %s", targetPath) | ||
| // it's a broken link; delete it | ||
| os.Remove(targetPath) | ||
| default: |
There was a problem hiding this comment.
clearBrokenLink ignores the return value of os.Remove(targetPath), so a failure to delete the broken symlink (e.g. permissions, races) will be silently swallowed and subsequent code will proceed as if cleanup succeeded. Capture and return/log the os.Remove error, and consider only treating os.IsNotExist/broken-symlink cases as removable instead of any *fs.PathError.
This is the second of three PRs to replace #16601 It requires #16685
In some cases, snapd insists on enabling user services not only globally, as expected (thus, adding a symlink at
/etc/systemd/user/XXXXX.target.wants), but also locally (thus, at $HOME/.config/systemd/user/XXXX.target.wants`).This means that if an user service is migrated from
default.targettographical-session.target(or vice-versa), the soft link at the user's HOME folder will be in the wrong subfolder.Unfortunately, the main snapd service seems to not have the capability of accessing and managing the local user services, having to rely on the user's local daemon (
snap userd), which runs as an user's service. So a change in that daemon (which has its source code atusersession/userdfolder) is required to check the locally enabled user services and ensure that their soft link is in the right place.This commit does this by checking the links in the user folder every time user session daemon is launched, and runs as the user a
systemctl --user reenable SNAP-SERVICE-NAMEto rebuild the wrong softlink.Thanks for helping us make a better snapd!
Have you signed the license agreement and read the contribution guide?