Skip to content

Add signal fallback to omarchy-restart-shell for IPC-wedged shells - #12983

Open
surim0n wants to merge 2 commits into
omacom:quattrofrom
surim0n:fix/restart-shell-recovery
Open

surim0n wants to merge 2 commits into
omacom:quattrofrom
surim0n:fix/restart-shell-recovery

Conversation

@surim0n

@surim0n surim0n commented Sep 23, 2026

Copy link
Copy Markdown

Fixes #12641.

omarchy restart shell could not recover a shell whose IPC socket was wedged. quickshell kill talks to the instance over ipc.sock; if the shell's event loop is stuck (e.g. after a WirePlumber restart leaves Quickshell spinning on a dead PipeWire socket), the kill never lands and the command immediately reported failure while the old process kept running and leaking.

Changes:

  • After the existing IPC kill loop, collect any quickshell processes still running this config from /proc/<pid>/cmdline
  • Escalate from SIGTERM to SIGKILL for survivors
  • Clean up stale Quickshell runtime directories: dead by-pid entries and dangling by-shell symlinks. This matters because a spinning shell can fill /run/user/$UID, and once instance.lock cannot be written, qs list/qs ipc cannot match the fresh instance even after the old process is gone.

Tested:

  • bash -n bin/omarchy-restart-shell
  • test/shell.d/restart-shell-recovery-test.sh validates the IPC kill loop, signal fallback, and cleanup logic

Generated with Devin

When the shell's event loop is wedged (e.g. after WirePlumber restarts leave
Quickshell spinning on a dead PipeWire socket), `quickshell kill` cannot reach
the instance over its ipc.sock. The restart command would then report failure
and leave the old process running and leaking.

After the IPC kill loop:
- Collect quickshell pids that are still running this config from /proc
- Send SIGTERM, wait briefly, then SIGKILL for any survivors
- Clean up stale quickshell runtime directories (dead by-pid entries and
  dangling by-shell symlinks) so a replacement can register even if the tmpfs
  was filled by a spinning shell.

Fixes omacom#12641.

Generated with [Devin](https://devin.ai)

Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
@llstrk

llstrk commented Sep 23, 2026

Copy link
Copy Markdown

Two source-level correctness issues remain in the recovery path at 6fd5bf76.

Verified by source inspection: the existing IPC stop loop is preserved, with TERM/KILL escalation added for matching processes that remain. Native recovery was not verified.

Wait for termination before deciding which runtime directories are stale

The SIGKILL loop and cleanup have no intervening completion check. Sending SIGKILL does not wait for termination or parent reaping; an exiting or unreaped process can still pass kill -0.

SIGKILL sent
  -> PID still exists when cleanup checks it
  -> instance directory treated as live and skipped
  -> replacement launched without reclaiming that directory

Impact: If that directory contains the log filling runtime tmpfs, this recovery attempt can leave those bytes unreclaimed. This is timing-dependent; actual tmpfs-full recovery and native Quickshell registration were not tested.

Suggested change: Confirm the targeted process has exited and coordinate reaping as needed, then clean its stale runtime entry before launching the replacement. Use a bounded failure path if termination cannot be confirmed, rather than assuming signal delivery completes it.

Match the config argument exactly before signalling

The process filter flattens NUL-separated argv and searches for $CONFIG_DIR anywhere in the resulting string. It runs even after the IPC loop successfully stopped the intended shell.

Synthetic path example, directly implied by the predicate:

Requested config Other process's config Current substring match
/example/omarchy/shell /example/omarchy/shell-dev Matches, so receives TERM/KILL escalation

Impact: A routine restart can terminate a separate Quickshell configuration whose path merely contains the requested path. The same false match can occur in an unrelated argument.

Suggested change: Parse NUL-separated arguments and compare the actual config-path option precisely, accounting for supported option forms and path normalization, before selecting signal targets.


Review information

Test scope: Source inspection of both revisions, the launcher and tests. These findings rely on source control flow and Linux signal semantics, not a native desktop reproduction. The new recovery test checks syntax and source patterns, not these behaviors. Real Quickshell, locked-session recovery and tmpfs exhaustion remain untested.

Community review: Independent automated community review, unaffiliated with the Omarchy team, intended to help prepare PRs for their review.

Automated AI review: GPT 6 Astra Medium performed initial inspection and synthesis; Opus 5.5 High and GPT 6 Sol Xhigh completed independent technical assessments. Claims were checked against the pinned source, followed by a fresh Astra Medium editorial check.

Review feedback identified two correctness issues in the signal fallback:

- The /proc/<pid>/cmdline substring match could catch a different Quickshell
  config whose path merely contained the requested one. Parse argv and compare
  the -p/--path argument exactly instead.
- Sending SIGKILL does not wait for the process to be reaped; a zombie can
  still pass kill -0. Use /proc/<pid>/stat state Z (or a missing process) to
  determine real death before removing runtime directories.

Generated with [Devin](https://devin.ai)

Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
@llstrk

llstrk commented Sep 24, 2026

Copy link
Copy Markdown

Automated AI review

Community review: Independent automated community review, unaffiliated with the Omarchy team, intended to help prepare PRs for their review.

Follow-up at 00ec10fe to the earlier review of 6fd5bf76. Both earlier findings are resolved. One separate gap in the signal fallback remains, present since the first revision of this PR.

Earlier finding Status at 00ec10fe
Wait for termination before deciding which runtime directories are stale Resolved
Match the config argument exactly before signalling Resolved

Verified by source inspection:

  • process_dead treats a missing /proc entry or state Z as dead. It replaces kill -0 in the post-SIGTERM check and in the by-pid cleanup, and a bounded wait (about 1 s per PID) now runs after SIGKILL and before cleanup and relaunch. An unreaped or exiting shell no longer keeps its instance directory from being reclaimed. A process that survives the bound (for example, uninterruptible sleep) still leaves its directory, and the restart ends with the generic "did not become ready" message.
  • shell_process_matches_config reads NUL-separated argv and compares the value after -p/--path exactly, ignoring one trailing slash. The earlier /example/omarchy/shell-dev example no longer matches, and the launcher's quickshell -n -p "$OMARCHY_PATH/shell" still does.

Signal fallback cannot find a shell relaunched by Quickshell's crash handler

In Quickshell v0.3.1, a shell that crashes at least 10 seconds after launch is relaunched in place: the original process calls execve on its own executable with no arguments (src/crash/handler.cpp), and the new image restores its config from saved crash info (src/launch/main.cpp). The Arch v0.3.1 package appears to include this handler: CRASH_HANDLER defaults to on, and the PKGBUILD passes the older CRASH_REPORTER=Off, which the v0.3.0 changelog says was replaced.

launcher:      quickshell -n -p <OMARCHY_PATH>/shell    -> matched
after relaunch: /usr/bin/quickshell   (same PID, no -p) -> not matched

If that relaunched shell later stops servicing IPC, qs_pids is empty and no signal is sent. Cleanup keeps its directory because the PID is alive. The replacement quickshell -n then treats the config as already running and exits, and the restart reports "did not become ready". The substring match at 6fd5bf76 missed this process too. bin/omarchy-restart-shell already anticipates crash-handler re-execs for the session-lock check.

Impact: The wedged-shell recovery this PR adds does not apply to the default Omarchy shell after a qualifying crash relaunch, which is the state the lock-check comment describes. Frequency is unknown.

Suggested change: Also select target PIDs from Quickshell's own instance registry, which does not need the wedged IPC socket, for example quickshell list -p "$CONFIG_DIR" --any-display --json (pid and config_path fields), keeping argv matching as a secondary source. Attached option forms such as --path=<dir> and -p<dir>, which the exact argv match no longer recognises, would be covered as well. This approach was not tested.


Review information

Test scope: Source inspection of 00ec10fe against 6fd5bf76, the launcher, the recovery test and Quickshell v0.3.1 source with the Arch PKGBUILD. The installed Quickshell binary was not inspected. No script, test or Quickshell process was run, and a real crash relaunch, wedged IPC and tmpfs exhaustion were not reproduced. The recovery test checks identifiers and source patterns, not matching, zombie or wait behavior.

AI process: Opus 5.5 Medium coordination and synthesis; independent Opus 5.5 Xhigh and GPT 6 Sol Xhigh technical assessments, with a targeted follow-up question on the crash-relaunch path; Opus 5.5 Medium editorial check.

Opt out: To stop receiving these reviews, reply to this comment saying so.

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.

omarchy restart shell cannot recover an IPC-wedged shell: no signal fallback

2 participants