Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b1d95bad61
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # own xdg-desktop-portal-singularity.service among them) launch at all. | ||
| install_data( | ||
| 'config/systemd-user/singularity-session.target', | ||
| install_dir: get_option('prefix') / 'lib' / 'systemd' / 'user', |
There was a problem hiding this comment.
For the advertised /opt/local installation, this places the target in /opt/local/lib/systemd/user, so the new systemctl call cannot find it and silently continues without activating the graphical services. Checked with systemd-analyze --user unit-paths: the standard paths include /usr/local/lib/systemd/user and /usr/lib/systemd/user, but not arbitrary ${prefix}/lib/systemd/user; use the configured systemd user-unit directory or otherwise install the file into a searched location.
Useful? React with 👍 / 👎.
| # the same role: BindsTo=graphical-session.target, | ||
| # Wants=graphical-session-pre.target, so starting it gets the pre-target | ||
| # sequencing for free too. | ||
| systemctl --user --no-block start singularity-session.target 2>/dev/null || true |
There was a problem hiding this comment.
When the user manager survives logout—such as with lingering enabled or another concurrent login—this start has no matching stop, so singularity-session.target remains active after labwc terminates. Because it keeps the StopWhenUnneeded=yes graphical target needed, graphical-session services such as the portal backend and foot server continue running outside the desktop session; add teardown tied to the launcher/compositor lifetime.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in aec8a46.
The pid-file cleanup that was already on trap ... EXIT becomes _session_cleanup(), and the matching systemctl --user --no-block stop singularity-session.target goes in there alongside it.
EXIT only, deliberately. bash runs the EXIT trap for an untrapped fatal signal as well, whereas an explicit trap ... TERM is deferred until the foreground command returns — and on the logout path the supervisor is blocked in "$_DESKTOP", so it never would. Checked on bash 5.3.9 (and 3.2.57): SIGTERM to the launcher while it supervised a long-lived fake shell issued the stop and removed the pid file.
Added tests/session_target_lifecycle_test.sh (wired into meson as session-target-lifecycle). It drives the launcher to its crash budget behind stub systemctl/pkill/gsettings/xdg-user-dirs-update and asserts the start, the stop, their ordering, and the pid-file removal. Against the pre-fix script it fails with:
launcher exited without stopping singularity-session.target
--user --no-block start singularity-session.target
--user restart --no-block xdg-desktop-portal
Both tests green on aarch64: session-safe-mode OK 4.17s, session-target-lifecycle OK 4.34s.
b1d95ba to
cab7acb
Compare
|
Fixed in cab7acb: the target install now asks systemd for its actual configured user-unit directory ( |
mirkobrombin
left a comment
mirkobrombin
left a comment
There was a problem hiding this comment.
The implementation now targets systemd correctly, but please reduce the added Meson and launcher comment blocks to the few non-obvious lifecycle facts.
|
Trimmed both blocks in dc14e66. meson.build — 7 lines down to 3 for the unit-dir lookup (systemd's user-unit search path does not follow our src/singularity-desktop-session — 21 lines down to 4 above the Comment-only, no functional change. Rebuilt and |
5962d54 to
dc14e66
Compare
mirkobrombin
left a comment
mirkobrombin
left a comment
There was a problem hiding this comment.
The EXIT trap stops the single user-wide target even when another concurrent login is still using it, so please make this lifecycle per-session or reference-counted instead of stopping it unconditionally.
… actually activates graphical-session.target is systemd-hardened against direct manual start (RefuseManualStart) -- it needs a unit that BindsTo= it, which is what labwc's own docs/autostart recommends (start labwc-session.target). That unit is not installed by this build, so add our own with the same shape: BindsTo=graphical-session.target, Wants=graphical-session-pre.target. Without something starting it, every systemd --user unit declaring WantedBy=graphical-session.target -- our xdg-desktop-portal-singularity service among them, plus foot-server -- sits enabled but never runs. Root-caused on real O6N 2026-08-27: xdg-desktop-portal correctly routed Screenshot requests to the singularity backend, but the backend process itself never started because nothing had ever activated graphical-session.target on this system. Confirmed fixed end-to-end after a clean session restart (with the matching cix-installer bridge that puts this target where systemd --user actually looks) -- both the portal-mediated Screenshot call and the native singularity-screenshot binary produced real PNGs.
Derive the install path for singularity-session.target from
`dependency('systemd').get_variable(pkgconfig: 'systemduserunitdir')`
instead of hardcoding it relative to our own --prefix.
systemd's user-unit search path doesn't move just because this
project is installed under a non-standard prefix (e.g. /opt/local),
so deriving it from get_option('prefix') silently placed the unit
somewhere systemd never looks, and the systemctl --user start call
in singularity-desktop-session would just quietly fail to find it.
Falls back to the previous prefix-relative path if systemd's
pkg-config data isn't available to query (e.g. a minimal sysroot).
Addresses Mirko's review on this PR asking for the target to be
installed into systemd's configured user unit directory rather than
a path derived from our own prefix.
The test hardcoded tests/../src/singularity-desktop-session, which is the .in template's directory without the .in suffix -- that path never exists; only the meson-generated copy in the build dir does. The sibling session-safe-mode test already solved this via SINGULARITY_TEST_DESKTOP_SESSION (set in meson.build, falls back to the .in template outside meson); apply the same pattern here so the test actually runs instead of failing with "No such file or directory" before it ever exercises the launcher. Assisted-by: Claude Code:claude-sonnet-5 AI-Scope: Diagnosed and fixed the test's launcher-path resolution while landing PR singularityos-lab#6 (session target start/stop); mirrored the existing fix pattern from session_safe_mode_test.sh.
aec8a46 to
1d2750a
Compare
Per review: the Meson and launcher comment blocks added by this PR restated the code and narrated the investigation. Cut both to the two facts that are not visible from the code -- graphical-session.target sets RefuseManualStart so it can only be reached through a unit that BindsTo= it, and systemd's user-unit search path does not follow our --prefix, which is why the dir is queried from systemd.pc with a prefix-relative fallback. Comment-only; no functional change. Rebuilt and `meson test` green on aarch64 (1/1), and a DESTDIR install still places the unit in systemd's own user dir rather than under the project prefix. Assisted-by: Claude Code:claude-opus-5 AI-Scope: Trimmed the two over-verbose comment blocks in meson.build and src/singularity-desktop-session down to the non-obvious lifecycle facts, per maintainer review, with no functional change. Signed-off-by: Jason Perlow <jperlow@gmail.com>
The start added in 8f3b9a1 had no matching stop. graphical-session.target is StopWhenUnneeded=yes, so it only goes away once nothing binds it -- and a user manager that survives logout (lingering enabled, or a second concurrent login) keeps singularity-session.target active after the compositor exits, leaving the portal backend and foot server running outside any desktop session. Move the existing pid-file cleanup into _session_cleanup() and stop the target there. EXIT only, deliberately: bash runs the EXIT trap for an untrapped fatal signal too, whereas an explicit TERM trap is deferred until the supervisor's foreground child returns, which on the logout path it never does. Confirmed on bash 5.3.9 by SIGTERMing the launcher while it supervised a long-lived fake shell -- the stop was issued and the pid file removed. tests/session_target_lifecycle_test.sh drives the launcher to its crash budget behind stub systemctl/pkill/gsettings and asserts start, stop, their ordering, and the pid-file removal. Against the pre-fix script it fails with "launcher exited without stopping singularity-session.target". Assisted-by: Claude Code:claude-opus-5 AI-Scope: Authored the cleanup trap, the lifecycle test, and the pre/post-fix verification runs on the arm64 build host.
The test hardcoded tests/../src/singularity-desktop-session, which is the .in template's directory without the .in suffix -- that path never exists; only the meson-generated copy in the build dir does. The sibling session-safe-mode test already solved this via SINGULARITY_TEST_DESKTOP_SESSION (set in meson.build, falls back to the .in template outside meson); apply the same pattern here so the test actually runs instead of failing with "No such file or directory" before it ever exercises the launcher. Assisted-by: Claude Code:claude-sonnet-5 AI-Scope: Diagnosed and fixed the test's launcher-path resolution while landing PR singularityos-lab#6 (session target start/stop); mirrored the existing fix pattern from session_safe_mode_test.sh.
1d2750a to
702451d
Compare
Mirko (PR singularityos-lab#6 review, on aec8a46): the EXIT trap stopped singularity-session.target unconditionally, but one systemd --user manager is shared by every login of the same UID -- a second concurrent login (a second seat, or console + SSH) runs its own copy of this launcher under the same manager and the same target, and would have its portal/audio-autoswitch/etc torn down the moment a sibling login exited first. Fix: each running instance drops a marker file under $XDG_RUNTIME_DIR/singularity-session.d/$$ on start and removes only its own marker on exit; the target is stopped only when no marker remains. Verified via a new scenario in tests/session_target_lifecycle_test.sh: with a sibling marker present, a full launcher run starts the target but does NOT stop it, and leaves the sibling's marker untouched. Assisted-by: Claude Code:claude-sonnet-5 AI-Scope: Implemented the reference-counted stop and the corresponding test scenario in response to Mirko's PR singularityos-lab#6 review comment on commit aec8a46.
|
Addressed all three review threads on this branch (now at 57fe5c2):
Assisted-by: Claude Code:claude-sonnet-5 |
2 participants
graphical-session.target refuses direct manual start (systemd hardening) — it needs a bound unit to pull it in. Nothing did, so every WantedBy=graphical-session.target unit (xdg-desktop-portal-singularity, foot-server) was enabled but never ran.
Adds singularity-session.target (BindsTo=graphical-session.target, matching what labwc's own docs recommend starting) and starts it from the session launcher.
Validated on real O6N: root-caused live, fixed, then confirmed clean from a full session restart — both the portal-mediated Screenshot call and the native singularity-screenshot binary produced real PNGs.
AI assistance: disclosed