Skip to content

Don't lose the session when an output is missing from the workspace sets - #2839

Open
dev-goalz wants to merge 1 commit into
pop-os:masterfrom
dev-goalz:fix/output-lookups-session-loss
Open

Don't lose the session when an output is missing from the workspace sets#2839
dev-goalz wants to merge 1 commit into
pop-os:masterfrom
dev-goalz:fix/output-lookups-session-loss

Conversation

@dev-goalz

@dev-goalz dev-goalz commented Sep 9, 2026

Copy link
Copy Markdown

Switching a monitor off, or a dock dropping its link, can currently take down the
whole COSMIC session and every open window with it. The worst part is the delay. The
session usually keeps running for minutes or hours after the display change that
broke it, so the crash looks like it was caused by whatever you happened to be doing
at the time. On one machine here the gap between cause and crash reached 23 hours.

What happens is that an output's render thread asks about workspaces for an output
the main thread has just removed. On a laptop the common case is the built in panel
being switched off because the lid is closed, while its own render thread is still
finishing a frame. active_num unwraps that lookup instead of handling the miss, so
the thread panics. The panic poisons a lock on its way out, and the compositor then
aborts at the next display operation, whenever that happens to be.

This makes the three Workspaces lookups total instead of partial, the way active
and active_mut already are. idx_for_handle and len had no backup_set fallback
at all. It also moves one call out from under the layer map guard so that a panic
there cannot poison it, and fixes a double unwrap on the same accessor in the image
capture path.

Two further sites reach the same lookups through a seat rather than through the
render loop, and neither is covered by the backup_set fallback. A seat keeps its
own idea of which output is focused or active, and that field is never checked
against sets. When the last output goes away the fallback stores the workspace set
aside and the seat fix ups are skipped, so every seat still names an output that no
longer exists. update_active then unwraps active_space on it and aborts on the
main thread, with no delay and no poisoned lock, which is a different failure from
the one above but the same root cause.

Rather than harden each consumer, add_output now restores the invariant. When it
takes the stored set back it re points any seat that still names an output which is
no longer present. That covers the sites in this PR and any future caller that
trusts a seat's output, which is what the rest of the code already assumes it can do.

A surface on its way out asking about workspaces that are already gone is now a no-op
instead of a session ending abort. The worst case after this is one frame of layer
shell surfaces tagged with workspace zero, on an output that is being torn down
anyway.

How it was checked

Running on one laptop with an external display on USB-C since 2026-08-31. In that
time the guarded lookup absorbed four faults that would each have aborted the
compositor on an unpatched build, the most recent on 2026-09-07, and the session
survived all of them with the same process still running. Before the patches the
same machine lost its session ten times.

Relates to #2727.


  • I have disclosed use of any AI generated code in my commit messages.
  • I understand these changes in full and will be able to respond to review comments.
  • My change is accurately described in the commit message.
  • My contribution is tested and working as described.
  • I have read the Developer Certificate of Origin and certify my contribution under its conditions.

Written with assistance from Claude Opus 5.0, as disclosed in the commit message.
The changes were reviewed, built and run on a daily driver for five and a half
weeks before submitting, and I can answer review comments on any of it.

@leviport

leviport commented Sep 9, 2026

Copy link
Copy Markdown
Member

The PR template is required. Please add it back and complete the checklist.

@dev-goalz

Copy link
Copy Markdown
Author

Adding the capture behind the "How it was checked" note, since a reasoned argument
and a recorded event are not the same thing.

The build under test carried this patch plus a diagnostic that logs the miss instead
of taking it, so the branch that would have been an abort is observable. Stimulus was
the reproducer: dock attached with the lid open, lid shut, dock unplugged, dock
reconnected with the lid still shut.

19:33:50  kernel  DP-1 disconnected -> connected      dock attached, lid open
19:33:51  comp    add_output eDP-1, add_output DP-1   sets settle to ["eDP-1","DP-1"]
19:35:01  logind  lid closed
19:35:01  comp    remove_output eDP-1                 sets=["eDP-1","DP-1"]
19:35:05  kernel  DP-1 connected -> disconnected      dock unplugged, lid shut
19:35:06  comp    remove_output DP-1, remove_output eDP-1, then add_output eDP-1
19:35:16  kernel  DP-1 disconnected -> connected      dock reconnected, lid shut
19:35:17  comp    add_output eDP-1, add_output DP-1
19:35:17  comp    remove_output eDP-1                 sets=["eDP-1","DP-1"]
19:35:17  comp    the lookup misses

The recorded line:

active_num: output absent from workspace sets (upstream unwrap would abort here)
caller = src/shell/mod.rs:5149:41
output = eDP-1
sets   = ["DP-1"]

That is the unguarded call inside the layer-map guard in take_presentation_feedback.
eDP-1 is out of sets because the lid path removed it, and its own render thread
reaches the lookup before the removal settles. On an unpatched build the unwrap
takes None here and the compositor aborts. With the lookup made total it returns
instead.

Immediately after: same process, every surface thread alive, no panic anywhere in the
log for it, nothing lost. Before the patch the same machine lost its session ten
times under the same class of stimulus, nine of them at this exact line.

It has kept happening since, and kept being absorbed:

2026-08-30  output=eDP-1  sets=["DP-1"]
2026-09-01  output=eDP-1  sets=["DP-1"]
2026-09-02  output=eDP-1  sets=["DP-1"]
2026-09-07  output=eDP-1  sets=["DP-1"]

Four in five and a half weeks of ordinary use, with the session surviving each one.

Two caveats so this is not read as more than it is. It is one machine, and the
diagnostic fires only when the race actually lands, so it says nothing useful about
how often the window is open. And it does not address the device-wide lock poisoning
that follows any other surface-thread panic, which is a separate problem this patch
does not claim to close.

…ace sets

Makes the Workspaces lookups total and re-points seats that name a removed output.

Written with assistance from Claude Opus 5.0. Reviewed, built and tested by the
author, who understands the changes in full.
@dev-goalz
dev-goalz force-pushed the fix/output-lookups-session-loss branch from 048d9b2 to e639bb2 Compare September 9, 2026 17:22
@dev-goalz

Copy link
Copy Markdown
Author

The PR template is required. Please add it back and complete the checklist.

Done

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants