From 3cdb63f36c72a59741ad961b29f53a0cf4aaa808 Mon Sep 17 00:00:00 2001 From: nocstah Date: Tue, 4 Aug 2026 09:44:51 +0700 Subject: [PATCH 1/5] fix(capture): time out blocking screencopy instead of spinning forever MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit captureOutputBlocking() looped wl_display_roundtrip() with no deadline. When the compositor never delivers ready/failed for a frame (reproduced with lock-screen desktop snapshots on Hyprland — both outputs, every time), the loop spun forever inside LockScreen::lock() on the main loop: the whole shell froze, IPC went dark, and the session lock was never even requested. Bound the wait to 2 seconds, cancel the in-flight capture on timeout (the completion callback holds stack references), and let the caller fall back to the wallpaper background. Co-Authored-By: Claude Fable 5 --- src/capture/screencopy_util.cpp | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/src/capture/screencopy_util.cpp b/src/capture/screencopy_util.cpp index 47269dfa10..263da21c2c 100644 --- a/src/capture/screencopy_util.cpp +++ b/src/capture/screencopy_util.cpp @@ -4,7 +4,9 @@ #include "wayland/wayland_connection.h" #include +#include #include +#include #include #include @@ -219,11 +221,27 @@ namespace screencopy { return false; } + // Bound the wait: a compositor that never delivers ready/failed for this + // capture (seen 2026-08-04 with a lock-screen snapshot on Hyprland) would + // otherwise spin this loop forever — inside the main loop, freezing the + // whole shell. Time out and let the caller fall back to the wallpaper. + const auto deadline = std::chrono::steady_clock::now() + std::chrono::seconds(2); while (!finished && capture.busy()) { if (wl_display_roundtrip(wayland.display()) < 0) { + capture.cancelInFlight(); error = "Wayland roundtrip failed"; return false; } + if (!finished && capture.busy()) { + if (std::chrono::steady_clock::now() >= deadline) { + // Drops the pending frame without firing the completion callback, + // which captures stack references that die when we return. + capture.cancelInFlight(); + error = "screencopy capture timed out"; + return false; + } + std::this_thread::sleep_for(std::chrono::milliseconds(10)); + } } if (!error.empty() || !finished) { From f9d0f6a652dfb4b1704d1d859ab4ab87b04747d5 Mon Sep 17 00:00:00 2001 From: nocstah Date: Tue, 4 Aug 2026 09:44:51 +0700 Subject: [PATCH 2/5] fix(lockscreen): log lock requests ignored because a lock is active lock() returned true silently when isActive(), which made a stale m_locked flag (seen 2026-08-04 after an external locker + compositor lock-restore sequence) look like a successful lock while every request no-op'd. Log the state so the condition is diagnosable; a shell restart clears it. Co-Authored-By: Claude Fable 5 --- src/shell/lockscreen/lock_screen.cpp | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/shell/lockscreen/lock_screen.cpp b/src/shell/lockscreen/lock_screen.cpp index 4d853f869e..6266791dc0 100644 --- a/src/shell/lockscreen/lock_screen.cpp +++ b/src/shell/lockscreen/lock_screen.cpp @@ -110,6 +110,11 @@ bool LockScreen::lock() { return false; } if (isActive()) { + // Seen desynced once (2026-08-04): an external locker + compositor lock + // restore left m_locked stuck true while the compositor was unlocked, and + // every lock request no-op'd silently. Log so that state is diagnosable; + // a shell restart clears it. + kLog.info("lock requested but lock already active (pending={} locked={}); ignoring", m_lockPending, m_locked); return true; } if (!m_wayland->hasSessionLockManager()) { From 940de7df4106740a76540c0fc6b9783e0991955e Mon Sep 17 00:00:00 2001 From: nocstah Date: Mon, 10 Aug 2026 17:50:50 +0700 Subject: [PATCH 3/5] fix(logind): resolve the session via the user's Display when PID lookup fails MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both session lookups assumed the shell runs inside the login session's cgroup. Under the systemd user manager it does not: user@.service sits outside it, so GetSessionByPID answers NoSessionForPID, and XDG_SESSION_ID is not in that manager's environment either. The whole logind integration was therefore dark on this machine: the session lock monitor logged 'disabled: session path unavailable' (so loginctl lock-session never reached the lock screen), the idle-inhibit monitor never armed, and brightness fell back to the 'auto' session path — which resolves against the caller and so is the same dead end. Ask logind for this user's Display session as a last resort. Now logs 'logind session lock monitor active (/org/freedesktop/login1/session/_33)'. Co-Authored-By: Claude Fable 5 --- src/dbus/logind/logind_service.cpp | 38 ++++++++++++++++++++++++++---- src/system/brightness_service.cpp | 33 ++++++++++++++++++++++---- 2 files changed, 61 insertions(+), 10 deletions(-) diff --git a/src/dbus/logind/logind_service.cpp b/src/dbus/logind/logind_service.cpp index 89b8951939..8819fbbdb9 100644 --- a/src/dbus/logind/logind_service.cpp +++ b/src/dbus/logind/logind_service.cpp @@ -52,12 +52,40 @@ namespace { } } - sdbus::ObjectPath sessionPath; - managerProxy->callMethod("GetSessionByPID") + try { + sdbus::ObjectPath sessionPath; + managerProxy->callMethod("GetSessionByPID") + .onInterface(kLogindManagerInterface) + .withArguments(static_cast(::getpid())) + .storeResultsTo(sessionPath); + return sessionPath; + } catch (const sdbus::Error& e) { + kLog.debug("failed to resolve logind session by pid: {}", e.what()); + } + + // Last resort: this user's DISPLAY session. Neither lookup above works + // when the shell is started by the systemd user manager — user@.service + // lives outside the login session's cgroup, so GetSessionByPID answers + // NoSessionForPID, and XDG_SESSION_ID is not in that manager's + // environment either. Without this the whole logind integration stayed + // dark: `loginctl lock-session` never reached the lock screen, and the + // brightness path lost its session too. + sdbus::ObjectPath userPath; + managerProxy->callMethod("GetUser") .onInterface(kLogindManagerInterface) - .withArguments(static_cast(::getpid())) - .storeResultsTo(sessionPath); - return sessionPath; + .withArguments(static_cast(::getuid())) + .storeResultsTo(userPath); + auto userProxy = sdbus::createProxy(connection, kLogindBusName, userPath); + const sdbus::Variant display = userProxy->getProperty("Display").onInterface("org.freedesktop.login1.User"); + // Display is (so): the session id plus its object path. + const auto displaySession = display.get>(); + const sdbus::ObjectPath& displayPath = std::get<1>(displaySession); + if (displayPath.empty()) { + kLog.warn("logind reports no display session for this user"); + return std::nullopt; + } + kLog.debug("resolved logind session via user Display: {}", displayPath); + return displayPath; } catch (const sdbus::Error& e) { kLog.warn("failed to resolve logind session: {}", e.what()); return std::nullopt; diff --git a/src/system/brightness_service.cpp b/src/system/brightness_service.cpp index dcabde34a5..d7a3b5f94a 100644 --- a/src/system/brightness_service.cpp +++ b/src/system/brightness_service.cpp @@ -381,12 +381,35 @@ namespace { } } - sdbus::ObjectPath sessionPath; - managerProxy->callMethod("GetSessionByPID") + try { + sdbus::ObjectPath sessionPath; + managerProxy->callMethod("GetSessionByPID") + .onInterface(kLogindManagerInterface) + .withArguments(static_cast(::getpid())) + .storeResultsTo(sessionPath); + return sessionPath; + } catch (const sdbus::Error& e) { + kLog.debug("failed to resolve logind session by pid: {}", e.what()); + } + + // Same fallback as logind_service.cpp: under the systemd user manager + // this process is outside the login session's cgroup, so the PID lookup + // answers NoSessionForPID and XDG_SESSION_ID is absent. Ask logind for + // this user's display session instead of falling through to the "auto" + // path, which resolves against the CALLER and so is the same dead end. + sdbus::ObjectPath userPath; + managerProxy->callMethod("GetUser") .onInterface(kLogindManagerInterface) - .withArguments(static_cast(::getpid())) - .storeResultsTo(sessionPath); - return sessionPath; + .withArguments(static_cast(::getuid())) + .storeResultsTo(userPath); + auto userProxy = sdbus::createProxy(connection, kLogindBusName, userPath); + const sdbus::Variant display = userProxy->getProperty("Display").onInterface("org.freedesktop.login1.User"); + const auto displaySession = display.get>(); + if (const sdbus::ObjectPath& displayPath = std::get<1>(displaySession); !displayPath.empty()) { + return displayPath; + } + kLog.warn("logind reports no display session for this user"); + return sdbus::ObjectPath{"/org/freedesktop/login1/session/auto"}; } catch (const sdbus::Error& e) { kLog.warn("failed to resolve logind session: {}", e.what()); return sdbus::ObjectPath{"/org/freedesktop/login1/session/auto"}; From e6fdd444e2f0c86fb82cf872463b5bff49c595b3 Mon Sep 17 00:00:00 2001 From: nocstah Date: Fri, 14 Aug 2026 14:16:38 +0700 Subject: [PATCH 4/5] fix(capture): wait on the Wayland fd with a deadline, not blocking roundtrips Review feedback on the first cut: the deadline was only checked *after* wl_display_roundtrip() returned, so a connection that stops making progress blocks inside the roundtrip and the timeout can never fire. Repeated blocking roundtrips with sleep_for() also do not fit the poll-based event loop. Wait on the Wayland fd instead: dispatch what is already queued, prepare_read, flush, poll() for at most the remaining budget, then read and dispatch what arrived. The wait is now bounded by the deadline rather than wrapped in it, and there is no sleep. The capture and wait sides move into screencopy_blocking.{h,cpp} behind small injectable ops so the deadline arithmetic and the cancellation path are testable without a compositor. The completion state also moves off the caller's stack into a shared_ptr: giving up flags it abandoned, so a late completion is a no-op instead of a write through references to `out` and `error` that no longer exist. Covered by tests/screencopy_blocking_test.cpp on a virtual clock: synchronous and pumped completion, timeout with cancellation, no wait longer than the remaining budget, a single wait that consumes the whole budget, dispatch failure, and a completion fired after the call returned leaving the caller's frame and error untouched. Co-Authored-By: Claude Opus 5 (1M context) --- meson.build | 2 + src/capture/screencopy_blocking.cpp | 96 +++++++++++ src/capture/screencopy_blocking.h | 47 ++++++ src/capture/screencopy_util.cpp | 126 ++++++++------ tests/screencopy_blocking_test.cpp | 245 ++++++++++++++++++++++++++++ 5 files changed, 465 insertions(+), 51 deletions(-) create mode 100644 src/capture/screencopy_blocking.cpp create mode 100644 src/capture/screencopy_blocking.h create mode 100644 tests/screencopy_blocking_test.cpp diff --git a/meson.build b/meson.build index 2a7f66da24..b3eb4e3ac0 100644 --- a/meson.build +++ b/meson.build @@ -818,6 +818,7 @@ _noctalia_sources = files( 'src/system/day_night_schedule.cpp', 'src/system/gamma_service.cpp', 'src/system/location_service.cpp', + 'src/capture/screencopy_blocking.cpp', 'src/capture/screencopy_capture.cpp', 'src/capture/screencopy_util.cpp', 'src/capture/screenshot_service.cpp', @@ -1164,6 +1165,7 @@ if build_tests 'plugin_source_paths', 'process', 'scheme', + 'screencopy_blocking', 'secret_store', 'security_primitives', 'state_store', diff --git a/src/capture/screencopy_blocking.cpp b/src/capture/screencopy_blocking.cpp new file mode 100644 index 0000000000..4bc2089763 --- /dev/null +++ b/src/capture/screencopy_blocking.cpp @@ -0,0 +1,96 @@ +#include "capture/screencopy_blocking.h" + +#include +#include +#include + +namespace screencopy { + + WaitOutcome waitForCapture( + const EventWaitOps& ops, const std::function& done, std::chrono::steady_clock::time_point deadline + ) { + while (!done()) { + const auto now = ops.now(); + if (now >= deadline) { + return WaitOutcome::TimedOut; + } + + // Hand the wait what is left of the budget, never more: this is the only + // thing standing between a silent compositor and a frozen main loop. + const auto remaining = + std::max(std::chrono::duration_cast(deadline - now), std::chrono::milliseconds{0}); + if (ops.waitAndDispatch(remaining) < 0) { + // The connection may still have handed us the completion on its way + // out — take it rather than reporting a failure we already recovered. + return done() ? WaitOutcome::Completed : WaitOutcome::Error; + } + } + return WaitOutcome::Completed; + } + + bool runBlockingCapture( + const BlockingCaptureOps& capture, const EventWaitOps& wait, ScreencopyImage& out, std::string& error, + std::chrono::milliseconds timeout + ) { + error.clear(); + + // Heap state, not the caller's stack: once we give up (timeout, dispatch + // error) a completion that still arrives must have nothing of ours left to + // write to. `abandoned` turns that late fire into a no-op instead of a + // use-after-return on `out` and `error`. + struct State { + bool finished = false; + bool abandoned = false; + std::optional image; + std::string error; + }; + const auto state = std::make_shared(); + + capture.start([state](std::optional image, std::string err) { + if (state->abandoned) { + return; + } + state->finished = true; + if (!err.empty() || !image.has_value()) { + state->error = err.empty() ? "screencopy capture failed" : std::move(err); + return; + } + state->image = std::move(image); + }); + + const auto done = [&state, &capture] { return state->finished || !capture.busy(); }; + const auto outcome = waitForCapture(wait, done, wait.now() + timeout); + + switch (outcome) { + case WaitOutcome::Completed: + break; + case WaitOutcome::TimedOut: + state->abandoned = true; + capture.cancel(); + error = "screencopy capture timed out"; + return false; + case WaitOutcome::Error: + state->abandoned = true; + capture.cancel(); + error = "Wayland event dispatch failed"; + return false; + } + + if (!state->error.empty()) { + error = state->error; + return false; + } + if (!state->finished || !state->image.has_value()) { + error = "screencopy capture failed"; + return false; + } + + out = std::move(*state->image); + if (out.width <= 0 || out.height <= 0 || out.rgba.empty()) { + error = "screencopy capture returned an empty frame"; + return false; + } + return true; + } + +} // namespace screencopy diff --git a/src/capture/screencopy_blocking.h b/src/capture/screencopy_blocking.h new file mode 100644 index 0000000000..949922c28e --- /dev/null +++ b/src/capture/screencopy_blocking.h @@ -0,0 +1,47 @@ +#pragma once + +#include "capture/screencopy_capture.h" + +#include +#include +#include + +namespace screencopy { + + // How long a blocking capture waits for the compositor before giving up. + inline constexpr std::chrono::milliseconds kBlockingCaptureTimeout{2000}; + + enum class WaitOutcome { Completed, TimedOut, Error }; + + // The wait side of a blocking capture, kept free of Wayland types so the + // deadline arithmetic is testable without a compositor. + // + // `waitAndDispatch` must block for AT MOST the timeout it is handed — the + // deadline can only be honoured if the wait itself is bounded — and return + // >0 when it dispatched events, 0 when the wait expired (or was interrupted) + // with no progress, <0 on a connection error. + struct EventWaitOps { + std::function waitAndDispatch; + std::function now = [] { return std::chrono::steady_clock::now(); }; + }; + + [[nodiscard]] WaitOutcome waitForCapture( + const EventWaitOps& ops, const std::function& done, std::chrono::steady_clock::time_point deadline + ); + + // The capture side, likewise free of Wayland types. + struct BlockingCaptureOps { + std::function start; + std::function busy; + std::function cancel; + }; + + // Starts a capture and pumps `wait` until it settles or `timeout` elapses. + // On timeout or dispatch error the in-flight capture is cancelled and the + // completion callback is abandoned: a late completion writes nothing. + [[nodiscard]] bool runBlockingCapture( + const BlockingCaptureOps& capture, const EventWaitOps& wait, ScreencopyImage& out, std::string& error, + std::chrono::milliseconds timeout = kBlockingCaptureTimeout + ); + +} // namespace screencopy diff --git a/src/capture/screencopy_util.cpp b/src/capture/screencopy_util.cpp index 263da21c2c..ca4774d454 100644 --- a/src/capture/screencopy_util.cpp +++ b/src/capture/screencopy_util.cpp @@ -1,12 +1,14 @@ #include "capture/screencopy_util.h" +#include "capture/screencopy_blocking.h" #include "capture/screencopy_capture.h" #include "wayland/wayland_connection.h" #include +#include #include #include -#include +#include #include #include @@ -196,67 +198,89 @@ namespace { } } -} // namespace - -namespace screencopy { - - bool captureOutputBlocking( - ScreencopyCapture& capture, WaylandConnection& wayland, wl_output* output, ScreencopyImage& out, - std::string& error, bool overlayCursor - ) { - error.clear(); - bool finished = false; - capture.capture( - output, std::nullopt, overlayCursor, [&](std::optional image, const std::string& err) { - finished = true; - if (!err.empty() || !image.has_value()) { - error = err.empty() ? "screencopy capture failed" : err; - return; - } - out = std::move(*image); - } - ); + // Deadline-aware wait on the Wayland fd. wl_display_roundtrip() cannot be + // used here: it blocks until the server answers, so a compositor that never + // delivers ready/failed for a frame (seen 2026-08-04 with lock-screen + // snapshots on Hyprland) blocks it forever and no deadline checked around it + // can fire. Dispatch what is already queued, then poll the fd for at most + // the remaining budget and dispatch whatever arrived. + // + // Returns >0 when events were dispatched, 0 when the wait expired or was + // interrupted with no progress, <0 on a connection error. + [[nodiscard]] int waitAndDispatchWayland(wl_display* display, std::chrono::milliseconds timeout) { + if (display == nullptr) { + return -1; + } - if (!error.empty()) { - return false; + if (wl_display_dispatch_pending(display) < 0) { + return -1; } - // Bound the wait: a compositor that never delivers ready/failed for this - // capture (seen 2026-08-04 with a lock-screen snapshot on Hyprland) would - // otherwise spin this loop forever — inside the main loop, freezing the - // whole shell. Time out and let the caller fall back to the wallpaper. - const auto deadline = std::chrono::steady_clock::now() + std::chrono::seconds(2); - while (!finished && capture.busy()) { - if (wl_display_roundtrip(wayland.display()) < 0) { - capture.cancelInFlight(); - error = "Wayland roundtrip failed"; - return false; + // prepare_read/read_events rather than wl_display_dispatch(): only the + // latter pair lets us sit in our own poll() with a timeout. + while (wl_display_prepare_read(display) != 0) { + // prepare_read only refuses while the queue still holds events, so drain + // them; any progress goes back to the caller, which re-checks the + // deadline before waiting again. + const int dispatched = wl_display_dispatch_pending(display); + if (dispatched < 0) { + return -1; } - if (!finished && capture.busy()) { - if (std::chrono::steady_clock::now() >= deadline) { - // Drops the pending frame without firing the completion callback, - // which captures stack references that die when we return. - capture.cancelInFlight(); - error = "screencopy capture timed out"; - return false; - } - std::this_thread::sleep_for(std::chrono::milliseconds(10)); + if (dispatched > 0) { + return dispatched; } + // Nothing drained and still cannot prepare: yield instead of spinning. + return 0; } - if (!error.empty() || !finished) { - if (error.empty()) { - error = "screencopy capture failed"; - } - return false; + if (wl_display_flush(display) < 0 && errno != EAGAIN) { + wl_display_cancel_read(display); + return -1; } - if (out.width <= 0 || out.height <= 0 || out.rgba.empty()) { - error = "screencopy capture returned an empty frame"; - return false; + pollfd pollFd{.fd = wl_display_get_fd(display), .events = POLLIN, .revents = 0}; + const int ready = ::poll(&pollFd, 1, static_cast(timeout.count())); + if (ready < 0) { + wl_display_cancel_read(display); + return errno == EINTR ? 0 : -1; + } + if (ready == 0) { + wl_display_cancel_read(display); + return 0; } - return true; + if (wl_display_read_events(display) < 0) { + return -1; + } + return wl_display_dispatch_pending(display); + } + +} // namespace + +namespace screencopy { + + bool captureOutputBlocking( + ScreencopyCapture& capture, WaylandConnection& wayland, wl_output* output, ScreencopyImage& out, + std::string& error, bool overlayCursor + ) { + // The wait is bounded (see runBlockingCapture): a compositor that never + // delivers ready/failed for this capture used to spin here forever — + // inside the main loop, freezing the whole shell. On timeout the caller + // falls back to the wallpaper background. + const BlockingCaptureOps ops{ + .start = [&]( + ScreencopyCapture::CompletionCallback onComplete + ) { capture.capture(output, std::nullopt, overlayCursor, std::move(onComplete)); }, + .busy = [&] { return capture.busy(); }, + .cancel = [&] { capture.cancelInFlight(); }, + }; + const EventWaitOps wait{ + .waitAndDispatch = [&](std::chrono::milliseconds timeout) { + return waitAndDispatchWayland(wayland.display(), timeout); + }, + }; + + return runBlockingCapture(ops, wait, out, error); } bool orientCaptureNative(ScreencopyImage& image, const WaylandConnection& wayland, wl_output* output) { diff --git a/tests/screencopy_blocking_test.cpp b/tests/screencopy_blocking_test.cpp new file mode 100644 index 0000000000..de07149a6f --- /dev/null +++ b/tests/screencopy_blocking_test.cpp @@ -0,0 +1,245 @@ +// Blocking screencopy: the wait is bounded by a deadline the compositor cannot +// outrun, the pending frame is cancelled when we give up, and a completion +// that arrives late has nothing of the caller's left to write to. + +#include "capture/screencopy_blocking.h" + +#include +#include +#include +#include +#include +#include + +namespace { + + using namespace std::chrono_literals; + + int g_failures = 0; + + void expectTrue(const char* what, bool cond) { + if (!cond) { + std::println(stderr, "screencopy_blocking_test: FAIL: {}", what); + ++g_failures; + } + } + + void expectEqual(const char* what, const std::string& actual, const std::string& expected) { + if (actual != expected) { + std::println(stderr, "screencopy_blocking_test: FAIL: {} = \"{}\", expected \"{}\"", what, actual, expected); + ++g_failures; + } + } + + void expectEqual(const char* what, std::size_t actual, std::size_t expected) { + if (actual != expected) { + std::println(stderr, "screencopy_blocking_test: FAIL: {} = {}, expected {}", what, actual, expected); + ++g_failures; + } + } + + [[nodiscard]] ScreencopyImage sampleImage() { + ScreencopyImage image; + image.width = 2; + image.height = 1; + image.rgba.assign(2U * 1U * 4U, 0xFFU); + return image; + } + + // A compositor stand-in driven by a virtual clock: every wait consumes + // exactly the timeout it was handed, so "the deadline is respected" is a + // deterministic assertion rather than a race with wall time. + struct FakeLoop { + std::chrono::steady_clock::time_point clock{}; + std::vector waits; + int pumpsUntilCompletion = -1; // <0: never completes + int failAfterPumps = -1; // >=0: waitAndDispatch reports a connection error + std::function onProgress; + + [[nodiscard]] screencopy::EventWaitOps ops() { + return screencopy::EventWaitOps{ + .waitAndDispatch = + [this](std::chrono::milliseconds timeout) { + waits.push_back(timeout); + clock += timeout; + if (failAfterPumps >= 0 && static_cast(waits.size()) > failAfterPumps) { + return -1; + } + if (pumpsUntilCompletion >= 0 && static_cast(waits.size()) >= pumpsUntilCompletion) { + if (onProgress) { + onProgress(); + } + return 1; + } + return 0; + }, + .now = [this] { return clock; }, + }; + } + }; + + struct FakeCapture { + ScreencopyCapture::CompletionCallback callback; + bool busy = false; + int cancels = 0; + bool completeOnStart = false; + std::string startError; + + [[nodiscard]] screencopy::BlockingCaptureOps ops() { + return screencopy::BlockingCaptureOps{ + .start = + [this](ScreencopyCapture::CompletionCallback onComplete) { + callback = std::move(onComplete); + busy = true; + if (!startError.empty()) { + busy = false; + callback(std::nullopt, startError); + return; + } + if (completeOnStart) { + busy = false; + callback(sampleImage(), {}); + } + }, + .busy = [this] { return busy; }, + .cancel = + [this] { + ++cancels; + busy = false; + }, + }; + } + + void complete() { + busy = false; + callback(sampleImage(), {}); + } + }; + +} // namespace + +int main() { + // A capture that completes on the first pump succeeds and yields its frame. + { + FakeCapture capture; + FakeLoop loop; + loop.pumpsUntilCompletion = 1; + loop.onProgress = [&capture] { capture.complete(); }; + + ScreencopyImage out; + std::string error; + const bool ok = screencopy::runBlockingCapture(capture.ops(), loop.ops(), out, error, 2000ms); + + expectTrue("completed capture returns true", ok); + expectEqual("completed capture leaves no error", error, ""); + expectTrue("completed capture fills the frame", out.width == 2 && out.height == 1 && !out.rgba.empty()); + expectEqual("completed capture does not cancel", static_cast(capture.cancels), 0U); + } + + // A completion that fires synchronously from start() needs no pumping. + { + FakeCapture capture; + capture.completeOnStart = true; + FakeLoop loop; + + ScreencopyImage out; + std::string error; + const bool ok = screencopy::runBlockingCapture(capture.ops(), loop.ops(), out, error, 2000ms); + + expectTrue("synchronous completion returns true", ok); + expectEqual("synchronous completion never waits", loop.waits.size(), 0U); + } + + // The reported bug: the compositor never answers. The wait must end at the + // deadline, cancel the frame, and never ask to wait past the budget. + { + FakeCapture capture; + FakeLoop loop; // never completes + + ScreencopyImage out; + std::string error; + const bool ok = screencopy::runBlockingCapture(capture.ops(), loop.ops(), out, error, 2000ms); + + expectTrue("silent compositor returns false", !ok); + expectEqual("silent compositor reports a timeout", error, "screencopy capture timed out"); + expectEqual("silent compositor cancels the frame", static_cast(capture.cancels), 1U); + expectTrue("the wait ends at the deadline", loop.clock <= std::chrono::steady_clock::time_point{} + 2000ms); + + std::chrono::milliseconds total{0}; + for (const auto wait : loop.waits) { + total += wait; + } + expectTrue("no wait exceeds the remaining budget", total <= 2000ms); + expectTrue("the deadline is reached, not merely approached", total == 2000ms); + } + + // A wait that blocks for the whole budget in one go still terminates: the + // deadline is re-checked after the wait returns, not only between pumps. + { + FakeCapture capture; + FakeLoop loop; + + ScreencopyImage out; + std::string error; + const bool ok = screencopy::runBlockingCapture(capture.ops(), loop.ops(), out, error, 50ms); + + expectTrue("single long wait returns false", !ok); + expectEqual("single long wait times out", error, "screencopy capture timed out"); + expectEqual("single long wait pumps exactly once", loop.waits.size(), 1U); + expectTrue("single long wait is bounded by the timeout", loop.waits.front() == 50ms); + } + + // A dead Wayland connection is an error, not a hang. + { + FakeCapture capture; + FakeLoop loop; + loop.failAfterPumps = 0; + + ScreencopyImage out; + std::string error; + const bool ok = screencopy::runBlockingCapture(capture.ops(), loop.ops(), out, error, 2000ms); + + expectTrue("dispatch failure returns false", !ok); + expectEqual("dispatch failure is reported", error, "Wayland event dispatch failed"); + expectEqual("dispatch failure cancels the frame", static_cast(capture.cancels), 1U); + } + + // The capture failing outright surfaces the compositor's message. + { + FakeCapture capture; + capture.startError = "screencopy unavailable"; + FakeLoop loop; + + ScreencopyImage out; + std::string error; + const bool ok = screencopy::runBlockingCapture(capture.ops(), loop.ops(), out, error, 2000ms); + + expectTrue("failed capture returns false", !ok); + expectEqual("failed capture keeps the reason", error, "screencopy unavailable"); + } + + // What the timeout is really guarding: a completion that arrives after we + // gave up. The callback must touch nothing the caller owns — before the fix + // it held references to `out` and `error` and wrote through them. + { + FakeCapture capture; + FakeLoop loop; + + ScreencopyImage out; + std::string error; + const bool ok = screencopy::runBlockingCapture(capture.ops(), loop.ops(), out, error, 2000ms); + expectTrue("abandoned capture times out", !ok && error == "screencopy capture timed out"); + + // The compositor finally answers, long after runBlockingCapture returned. + capture.callback(sampleImage(), {}); + + expectEqual("late completion does not overwrite the caller's error", error, "screencopy capture timed out"); + expectTrue("late completion does not fill the caller's frame", out.width == 0 && out.rgba.empty()); + } + + if (g_failures > 0) { + std::println(stderr, "screencopy_blocking_test: {} failure(s)", g_failures); + return 1; + } + return 0; +} From 380c6cf19ab7a80f6ab82b2100f2a5a316f2e822 Mon Sep 17 00:00:00 2001 From: nocstah Date: Fri, 14 Aug 2026 14:16:50 +0700 Subject: [PATCH 5/5] refactor(logind): share session resolution, make the Display fallback explicit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review feedback: the fallback added to LogindService was copy-pasted into BrightnessService. Both now call logind::resolveSession() from logind-owned code (src/dbus/logind/logind_session.{h,cpp}), which returns the session path together with the lookup that answered — XDG_SESSION_ID, pid, or the user's Display session. Both call sites log that source, so which path a session came from is visible in the log rather than inferred. The Display fallback is best-effort by nature: logind picks it per user, not per caller, so a user with several concurrent sessions can be handed one this process is not running in. That is now explicit — the resolver reads the user's full Sessions list and warns, naming the session it guessed, instead of returning it silently. BrightnessService keeps its "auto" path for the case where nothing resolves at all. The lookups are injectable, so tests/logind_session_test.cpp covers the order (id wins, no fallback consulted), a stale XDG_SESSION_ID falling through to the pid, the systemd-user-manager case that motivated the fallback, the multi-session guess, a user with no display session, and every lookup dark. Co-Authored-By: Claude Opus 5 (1M context) --- meson.build | 2 + src/dbus/logind/logind_service.cpp | 69 ++-------- src/dbus/logind/logind_session.cpp | 159 ++++++++++++++++++++++ src/dbus/logind/logind_session.h | 65 +++++++++ src/system/brightness_service.cpp | 59 ++------ tests/logind_session_test.cpp | 210 +++++++++++++++++++++++++++++ 6 files changed, 455 insertions(+), 109 deletions(-) create mode 100644 src/dbus/logind/logind_session.cpp create mode 100644 src/dbus/logind/logind_session.h create mode 100644 tests/logind_session_test.cpp diff --git a/meson.build b/meson.build index b3eb4e3ac0..7441cc919e 100644 --- a/meson.build +++ b/meson.build @@ -464,6 +464,7 @@ _noctalia_sources = files( 'src/dbus/bluetooth/bluetooth_service.cpp', 'src/dbus/idle/screensaver_service.cpp', 'src/dbus/logind/logind_service.cpp', + 'src/dbus/logind/logind_session.cpp', 'src/dbus/mpris/mpris_art.cpp', 'src/dbus/mpris/mpris_service.cpp', 'src/dbus/network/external_ip_service.cpp', @@ -1149,6 +1150,7 @@ if build_tests 'kde_color_scheme', 'log', 'location_service', + 'logind_session', 'math_provider', 'monitor_selector', 'notification_filter', diff --git a/src/dbus/logind/logind_service.cpp b/src/dbus/logind/logind_service.cpp index 8819fbbdb9..066b338025 100644 --- a/src/dbus/logind/logind_service.cpp +++ b/src/dbus/logind/logind_service.cpp @@ -1,9 +1,9 @@ #include "dbus/logind/logind_service.h" #include "core/log.h" +#include "dbus/logind/logind_session.h" #include "dbus/system_bus.h" -#include #include #include #include @@ -35,62 +35,6 @@ namespace { return ::fcntl(fd, F_SETFD, flags | FD_CLOEXEC) == 0; } - [[nodiscard]] std::optional resolveSessionPath(sdbus::IConnection& connection) { - try { - auto managerProxy = sdbus::createProxy(connection, kLogindBusName, kLogindObjectPath); - - if (const char* sessionId = std::getenv("XDG_SESSION_ID"); sessionId != nullptr && sessionId[0] != '\0') { - try { - sdbus::ObjectPath sessionPath; - managerProxy->callMethod("GetSession") - .onInterface(kLogindManagerInterface) - .withArguments(std::string(sessionId)) - .storeResultsTo(sessionPath); - return sessionPath; - } catch (const sdbus::Error& e) { - kLog.debug("failed to resolve logind session via XDG_SESSION_ID={}: {}", sessionId, e.what()); - } - } - - try { - sdbus::ObjectPath sessionPath; - managerProxy->callMethod("GetSessionByPID") - .onInterface(kLogindManagerInterface) - .withArguments(static_cast(::getpid())) - .storeResultsTo(sessionPath); - return sessionPath; - } catch (const sdbus::Error& e) { - kLog.debug("failed to resolve logind session by pid: {}", e.what()); - } - - // Last resort: this user's DISPLAY session. Neither lookup above works - // when the shell is started by the systemd user manager — user@.service - // lives outside the login session's cgroup, so GetSessionByPID answers - // NoSessionForPID, and XDG_SESSION_ID is not in that manager's - // environment either. Without this the whole logind integration stayed - // dark: `loginctl lock-session` never reached the lock screen, and the - // brightness path lost its session too. - sdbus::ObjectPath userPath; - managerProxy->callMethod("GetUser") - .onInterface(kLogindManagerInterface) - .withArguments(static_cast(::getuid())) - .storeResultsTo(userPath); - auto userProxy = sdbus::createProxy(connection, kLogindBusName, userPath); - const sdbus::Variant display = userProxy->getProperty("Display").onInterface("org.freedesktop.login1.User"); - // Display is (so): the session id plus its object path. - const auto displaySession = display.get>(); - const sdbus::ObjectPath& displayPath = std::get<1>(displaySession); - if (displayPath.empty()) { - kLog.warn("logind reports no display session for this user"); - return std::nullopt; - } - kLog.debug("resolved logind session via user Display: {}", displayPath); - return displayPath; - } catch (const sdbus::Error& e) { - kLog.warn("failed to resolve logind session: {}", e.what()); - return std::nullopt; - } - } } // namespace LogindService::LogindService(SystemBus& bus) : m_bus(bus) { @@ -112,13 +56,13 @@ void LogindService::ensureSessionLockMonitor() { return; } - const auto sessionPath = resolveSessionPath(m_bus.connection()); - if (!sessionPath.has_value()) { + const auto session = logind::resolveSession(m_bus.connection()); + if (!session.has_value()) { kLog.warn("logind session lock monitor disabled: session path unavailable"); return; } - m_sessionProxy = sdbus::createProxy(m_bus.connection(), kLogindBusName, *sessionPath); + m_sessionProxy = sdbus::createProxy(m_bus.connection(), kLogindBusName, session->path); m_sessionProxy->uponSignal("Lock").onInterface(kLogindSessionInterface).call([this]() { if (m_lockCallback) { m_lockCallback(); @@ -129,7 +73,10 @@ void LogindService::ensureSessionLockMonitor() { m_unlockCallback(); } }); - kLog.info("logind session lock monitor active ({})", std::string(sessionPath->c_str())); + kLog.info( + "logind session lock monitor active ({}, resolved via {})", std::string(session->path.c_str()), + logind::describe(session->source) + ); } void LogindService::setSessionLockIntegrationEnabled(bool enabled) { diff --git a/src/dbus/logind/logind_session.cpp b/src/dbus/logind/logind_session.cpp new file mode 100644 index 0000000000..0bb790133a --- /dev/null +++ b/src/dbus/logind/logind_session.cpp @@ -0,0 +1,159 @@ +#include "dbus/logind/logind_session.h" + +#include "core/log.h" + +#include +#include +#include +#include +#include +#include + +namespace { + constexpr Logger kLog("logind"); + + const sdbus::ServiceName kLogindBusName{"org.freedesktop.login1"}; + const sdbus::ObjectPath kLogindObjectPath{"/org/freedesktop/login1"}; + constexpr auto kLogindManagerInterface = "org.freedesktop.login1.Manager"; + constexpr auto kLogindUserInterface = "org.freedesktop.login1.User"; +} // namespace + +namespace logind { + + std::string_view describe(SessionSource source) { + switch (source) { + case SessionSource::XdgSessionId: + return "XDG_SESSION_ID"; + case SessionSource::ProcessId: + return "pid"; + case SessionSource::UserDisplay: + return "user Display session"; + } + return "unknown"; + } + + bool displaySessionIsAmbiguous(const UserSessions& sessions) { return sessions.all.size() > 1; } + + std::optional resolveSession(const SessionLookups& lookups) { + if (lookups.xdgSessionId && lookups.sessionById) { + if (const auto sessionId = lookups.xdgSessionId(); sessionId.has_value() && !sessionId->empty()) { + if (auto path = lookups.sessionById(*sessionId); path.has_value() && !path->empty()) { + return ResolvedSession{.path = std::move(*path), .source = SessionSource::XdgSessionId}; + } + } + } + + if (lookups.sessionByProcessId) { + if (auto path = lookups.sessionByProcessId(); path.has_value() && !path->empty()) { + return ResolvedSession{.path = std::move(*path), .source = SessionSource::ProcessId}; + } + } + + if (!lookups.userSessions) { + return std::nullopt; + } + const auto sessions = lookups.userSessions(); + if (!sessions.has_value()) { + return std::nullopt; + } + if (sessions->displayPath.empty()) { + kLog.warn("logind reports no display session for this user"); + return std::nullopt; + } + + // Deliberate best-effort: with several concurrent sessions this can pick a + // session other than the one this process runs in. Say so out loud rather + // than have a lock request quietly land on the wrong seat. + if (displaySessionIsAmbiguous(*sessions)) { + kLog.warn( + "logind: {} concurrent sessions for this user, falling back to the Display session {} ({}) — it may not be " + "the session this process runs in", + sessions->all.size(), sessions->displayId, static_cast(sessions->displayPath) + ); + } else { + kLog.debug("resolved logind session via user Display: {}", static_cast(sessions->displayPath)); + } + return ResolvedSession{.path = sessions->displayPath, .source = SessionSource::UserDisplay}; + } + + std::optional resolveSession(sdbus::IConnection& connection) { + try { + auto managerProxy = sdbus::createProxy(connection, kLogindBusName, kLogindObjectPath); + + const SessionLookups lookups{ + .xdgSessionId = []() -> std::optional { + const char* sessionId = std::getenv("XDG_SESSION_ID"); + if (sessionId == nullptr || sessionId[0] == '\0') { + return std::nullopt; + } + return std::string(sessionId); + }, + .sessionById = [&](const std::string& id) -> std::optional { + try { + sdbus::ObjectPath sessionPath; + managerProxy->callMethod("GetSession") + .onInterface(kLogindManagerInterface) + .withArguments(id) + .storeResultsTo(sessionPath); + return sessionPath; + } catch (const sdbus::Error& e) { + kLog.debug("failed to resolve logind session via XDG_SESSION_ID={}: {}", id, e.what()); + return std::nullopt; + } + }, + .sessionByProcessId = [&]() -> std::optional { + try { + sdbus::ObjectPath sessionPath; + managerProxy->callMethod("GetSessionByPID") + .onInterface(kLogindManagerInterface) + .withArguments(static_cast(::getpid())) + .storeResultsTo(sessionPath); + return sessionPath; + } catch (const sdbus::Error& e) { + kLog.debug("failed to resolve logind session by pid: {}", e.what()); + return std::nullopt; + } + }, + .userSessions = [&]() -> std::optional { + try { + sdbus::ObjectPath userPath; + managerProxy->callMethod("GetUser") + .onInterface(kLogindManagerInterface) + .withArguments(static_cast(::getuid())) + .storeResultsTo(userPath); + auto userProxy = sdbus::createProxy(connection, kLogindBusName, userPath); + + UserSessions sessions; + // Display is (so): the session id plus its object path. + const sdbus::Variant display = userProxy->getProperty("Display").onInterface(kLogindUserInterface); + const auto displaySession = display.get>(); + sessions.displayId = std::get<0>(displaySession); + sessions.displayPath = std::get<1>(displaySession); + + // Sessions is a(so) — only needed to tell an unambiguous pick + // from a guess, so a failure here downgrades the log, not the + // lookup. + try { + const sdbus::Variant all = userProxy->getProperty("Sessions").onInterface(kLogindUserInterface); + for (const auto& entry : all.get>>()) { + sessions.all.push_back(UserSession{.id = std::get<0>(entry), .path = std::get<1>(entry)}); + } + } catch (const sdbus::Error& e) { + kLog.debug("failed to list this user's logind sessions: {}", e.what()); + } + return sessions; + } catch (const sdbus::Error& e) { + kLog.debug("failed to resolve logind session via the user's Display: {}", e.what()); + return std::nullopt; + } + }, + }; + + return resolveSession(lookups); + } catch (const sdbus::Error& e) { + kLog.warn("failed to resolve logind session: {}", e.what()); + return std::nullopt; + } + } + +} // namespace logind diff --git a/src/dbus/logind/logind_session.h b/src/dbus/logind/logind_session.h new file mode 100644 index 0000000000..217434e7f7 --- /dev/null +++ b/src/dbus/logind/logind_session.h @@ -0,0 +1,65 @@ +#pragma once + +#include +#include +#include +#include +#include +#include + +namespace sdbus { + class IConnection; +} // namespace sdbus + +namespace logind { + + // Which lookup answered. Only XdgSessionId and ProcessId identify THIS + // process' session; UserDisplay is a best-effort guess (see below), so + // callers log the source rather than treating them as equivalent. + enum class SessionSource { XdgSessionId, ProcessId, UserDisplay }; + + [[nodiscard]] std::string_view describe(SessionSource source); + + struct ResolvedSession { + sdbus::ObjectPath path; + SessionSource source = SessionSource::XdgSessionId; + }; + + struct UserSession { + std::string id; + sdbus::ObjectPath path; + }; + + // org.freedesktop.login1.User: the Display session plus every session this + // user owns. + struct UserSessions { + std::string displayId; + sdbus::ObjectPath displayPath; + std::vector all; + }; + + // True when the user has more than one session, i.e. the Display session is + // a guess: logind picks it per user, not per caller, so a shell started by + // the systemd user manager can be handed a session it is not running in. + [[nodiscard]] bool displaySessionIsAmbiguous(const UserSessions& sessions); + + // The three lookups, injectable so the fallback order and the Display choice + // can be tested without a bus. Each returns nullopt when it does not answer. + struct SessionLookups { + std::function()> xdgSessionId; + std::function(const std::string& id)> sessionById; + std::function()> sessionByProcessId; + std::function()> userSessions; + }; + + // XDG_SESSION_ID, then GetSessionByPID, then this user's Display session. + // The last one exists because neither of the first two works when the shell + // is started by the systemd user manager: user@.service lives outside the + // login session's cgroup, so GetSessionByPID answers NoSessionForPID, and + // XDG_SESSION_ID is not in that manager's environment either. + [[nodiscard]] std::optional resolveSession(const SessionLookups& lookups); + + // Same resolution, driven against logind on `connection`. + [[nodiscard]] std::optional resolveSession(sdbus::IConnection& connection); + +} // namespace logind diff --git a/src/system/brightness_service.cpp b/src/system/brightness_service.cpp index d7a3b5f94a..49321c05b2 100644 --- a/src/system/brightness_service.cpp +++ b/src/system/brightness_service.cpp @@ -4,6 +4,7 @@ #include "config/config_types.h" #include "core/log.h" #include "core/process/process.h" +#include "dbus/logind/logind_session.h" #include "dbus/system_bus.h" #include "ipc/ipc_arg_parse.h" #include "ipc/ipc_service.h" @@ -118,7 +119,6 @@ namespace { }; const sdbus::ServiceName kLogindBusName{"org.freedesktop.login1"}; - constexpr auto kLogindManagerInterface = "org.freedesktop.login1.Manager"; constexpr auto kLogindSessionInterface = "org.freedesktop.login1.Session"; std::string joinBrightnessDisplayIds(const BrightnessService& service) { @@ -365,55 +365,18 @@ namespace { } sdbus::ObjectPath resolveSessionPath(sdbus::IConnection& connection) { - try { - auto managerProxy = sdbus::createProxy(connection, kLogindBusName, sdbus::ObjectPath{"/org/freedesktop/login1"}); - - if (const char* sessionId = std::getenv("XDG_SESSION_ID"); sessionId != nullptr && sessionId[0] != '\0') { - try { - sdbus::ObjectPath sessionPath; - managerProxy->callMethod("GetSession") - .onInterface(kLogindManagerInterface) - .withArguments(std::string(sessionId)) - .storeResultsTo(sessionPath); - return sessionPath; - } catch (const sdbus::Error& e) { - kLog.debug("failed to resolve logind session via XDG_SESSION_ID={}: {}", sessionId, e.what()); - } - } - - try { - sdbus::ObjectPath sessionPath; - managerProxy->callMethod("GetSessionByPID") - .onInterface(kLogindManagerInterface) - .withArguments(static_cast(::getpid())) - .storeResultsTo(sessionPath); - return sessionPath; - } catch (const sdbus::Error& e) { - kLog.debug("failed to resolve logind session by pid: {}", e.what()); - } - - // Same fallback as logind_service.cpp: under the systemd user manager - // this process is outside the login session's cgroup, so the PID lookup - // answers NoSessionForPID and XDG_SESSION_ID is absent. Ask logind for - // this user's display session instead of falling through to the "auto" - // path, which resolves against the CALLER and so is the same dead end. - sdbus::ObjectPath userPath; - managerProxy->callMethod("GetUser") - .onInterface(kLogindManagerInterface) - .withArguments(static_cast(::getuid())) - .storeResultsTo(userPath); - auto userProxy = sdbus::createProxy(connection, kLogindBusName, userPath); - const sdbus::Variant display = userProxy->getProperty("Display").onInterface("org.freedesktop.login1.User"); - const auto displaySession = display.get>(); - if (const sdbus::ObjectPath& displayPath = std::get<1>(displaySession); !displayPath.empty()) { - return displayPath; - } - kLog.warn("logind reports no display session for this user"); - return sdbus::ObjectPath{"/org/freedesktop/login1/session/auto"}; - } catch (const sdbus::Error& e) { - kLog.warn("failed to resolve logind session: {}", e.what()); + // Shared with the lock monitor (logind_session.h) — the fallbacks matter + // here too: "auto" resolves against the CALLER, so under the systemd user + // manager it is the same dead end as the PID lookup. + const auto session = logind::resolveSession(connection); + if (!session.has_value()) { return sdbus::ObjectPath{"/org/freedesktop/login1/session/auto"}; } + kLog.debug( + "using logind session {} (resolved via {})", std::string(session->path.c_str()), + logind::describe(session->source) + ); + return session->path; } std::optional parseTrailingInteger(std::string_view input) { diff --git a/tests/logind_session_test.cpp b/tests/logind_session_test.cpp new file mode 100644 index 0000000000..c4692949fd --- /dev/null +++ b/tests/logind_session_test.cpp @@ -0,0 +1,210 @@ +// Shared logind session resolution: the fallback order (XDG_SESSION_ID, then +// GetSessionByPID, then the user's Display session) and the best-effort nature +// of that last step, which can name a session this process is not running in. + +#include "dbus/logind/logind_session.h" + +#include +#include +#include +#include + +namespace { + + int g_failures = 0; + + void expectTrue(const char* what, bool cond) { + if (!cond) { + std::println(stderr, "logind_session_test: FAIL: {}", what); + ++g_failures; + } + } + + void expectPath(const char* what, const std::optional& actual, const std::string& expected) { + if (!actual.has_value()) { + std::println(stderr, "logind_session_test: FAIL: {} = unresolved, expected \"{}\"", what, expected); + ++g_failures; + return; + } + if (std::string(actual->path.c_str()) != expected) { + std::println( + stderr, "logind_session_test: FAIL: {} = \"{}\", expected \"{}\"", what, std::string(actual->path.c_str()), + expected + ); + ++g_failures; + } + } + + void + expectSource(const char* what, const std::optional& actual, logind::SessionSource expected) { + if (!actual.has_value() || actual->source != expected) { + std::println( + stderr, "logind_session_test: FAIL: {} resolved via {}, expected {}", what, + actual.has_value() ? logind::describe(actual->source) : "nothing", logind::describe(expected) + ); + ++g_failures; + } + } + + struct Calls { + int byId = 0; + int byPid = 0; + int userSessions = 0; + }; + +} // namespace + +int main() { + const auto never = [](const std::string&) -> std::optional { return std::nullopt; }; + + // XDG_SESSION_ID wins when logind knows the id: it names THIS session. + { + Calls calls; + const logind::SessionLookups lookups{ + .xdgSessionId = [] { return std::optional{"3"}; }, + .sessionById = [&](const std::string& id) -> std::optional { + ++calls.byId; + return id == "3" ? std::optional{sdbus::ObjectPath{"/org/freedesktop/login1/session/_33"}} : std::nullopt; + }, + .sessionByProcessId = [&]() -> std::optional { + ++calls.byPid; + return sdbus::ObjectPath{"/org/freedesktop/login1/session/pid"}; + }, + .userSessions = [&]() -> std::optional { + ++calls.userSessions; + return std::nullopt; + }, + }; + + const auto session = logind::resolveSession(lookups); + expectPath("XDG_SESSION_ID session", session, "/org/freedesktop/login1/session/_33"); + expectSource("XDG_SESSION_ID session", session, logind::SessionSource::XdgSessionId); + expectTrue("no fallback is consulted once the id resolves", calls.byPid == 0 && calls.userSessions == 0); + } + + // A stale XDG_SESSION_ID (logind no longer knows it) falls through to the pid. + { + Calls calls; + const logind::SessionLookups lookups{ + .xdgSessionId = [] { return std::optional{"9"}; }, + .sessionById = [&](const std::string&) -> std::optional { + ++calls.byId; + return std::nullopt; + }, + .sessionByProcessId = [] { return std::optional{sdbus::ObjectPath{"/org/freedesktop/login1/session/_31"}}; }, + .userSessions = [&]() -> std::optional { + ++calls.userSessions; + return std::nullopt; + }, + }; + + const auto session = logind::resolveSession(lookups); + expectPath("stale id falls back to pid", session, "/org/freedesktop/login1/session/_31"); + expectSource("stale id falls back to pid", session, logind::SessionSource::ProcessId); + expectTrue("the stale id was tried first", calls.byId == 1); + expectTrue("the Display session is not needed", calls.userSessions == 0); + } + + // The reported bug: started by the systemd user manager, so there is no + // XDG_SESSION_ID and GetSessionByPID answers NoSessionForPID. The user's + // Display session is the only thing left. + { + const logind::SessionLookups lookups{ + .xdgSessionId = []() -> std::optional { return std::nullopt; }, + .sessionById = never, + .sessionByProcessId = []() -> std::optional { return std::nullopt; }, + .userSessions = + [] { + return std::optional{logind::UserSessions{ + .displayId = "3", + .displayPath = sdbus::ObjectPath{"/org/freedesktop/login1/session/_33"}, + .all = {{.id = "3", .path = sdbus::ObjectPath{"/org/freedesktop/login1/session/_33"}}}, + }}; + }, + }; + + const auto session = logind::resolveSession(lookups); + expectPath("user manager falls back to Display", session, "/org/freedesktop/login1/session/_33"); + expectSource("user manager falls back to Display", session, logind::SessionSource::UserDisplay); + } + + // Explicit best-effort: with several concurrent sessions the Display session + // is still used, but it is flagged as a guess (and logged as one) because + // logind picks it per user, not per caller. + { + const logind::UserSessions sessions{ + .displayId = "3", + .displayPath = sdbus::ObjectPath{"/org/freedesktop/login1/session/_33"}, + .all = { + {.id = "3", .path = sdbus::ObjectPath{"/org/freedesktop/login1/session/_33"}}, + {.id = "5", .path = sdbus::ObjectPath{"/org/freedesktop/login1/session/_35"}}, + }, + }; + expectTrue( + "one session is unambiguous", + !logind::displaySessionIsAmbiguous( + logind::UserSessions{ + .displayId = sessions.displayId, + .displayPath = sessions.displayPath, + .all = {sessions.all.front()}, + } + ) + ); + expectTrue("concurrent sessions are ambiguous", logind::displaySessionIsAmbiguous(sessions)); + + const logind::SessionLookups lookups{ + .xdgSessionId = []() -> std::optional { return std::nullopt; }, + .sessionById = never, + .sessionByProcessId = []() -> std::optional { return std::nullopt; }, + .userSessions = [&] { return std::optional{sessions}; }, + }; + + const auto session = logind::resolveSession(lookups); + expectPath("ambiguous Display session is still used", session, "/org/freedesktop/login1/session/_33"); + expectSource("ambiguous Display session is still used", session, logind::SessionSource::UserDisplay); + } + + // A user with no graphical session at all: nothing to guess with. + { + const logind::SessionLookups lookups{ + .xdgSessionId = []() -> std::optional { return std::nullopt; }, + .sessionById = never, + .sessionByProcessId = []() -> std::optional { return std::nullopt; }, + .userSessions = + [] { + return std::optional{logind::UserSessions{ + .displayId = {}, + .displayPath = sdbus::ObjectPath{}, + .all = {{.id = "7", .path = sdbus::ObjectPath{"/org/freedesktop/login1/session/_37"}}}, + }}; + }, + }; + + expectTrue("no Display session resolves to nothing", !logind::resolveSession(lookups).has_value()); + } + + // Every lookup dark (no bus, no user object) resolves to nothing rather than + // to a path the caller would then talk to. + { + const logind::SessionLookups lookups{ + .xdgSessionId = []() -> std::optional { return std::nullopt; }, + .sessionById = never, + .sessionByProcessId = []() -> std::optional { return std::nullopt; }, + .userSessions = []() -> std::optional { return std::nullopt; }, + }; + + expectTrue("all lookups failing resolves to nothing", !logind::resolveSession(lookups).has_value()); + } + + // Missing lookups must not be called blindly. + { + const logind::SessionLookups lookups{}; + expectTrue("empty lookups resolve to nothing", !logind::resolveSession(lookups).has_value()); + } + + if (g_failures > 0) { + std::println(stderr, "logind_session_test: {} failure(s)", g_failures); + return 1; + } + return 0; +}