Skip to content

Coalesce concurrent shell restarts - #10634

Open
chivopic wants to merge 1 commit into
omacom:quattrofrom
chivopic:fix/restart-shell-concurrency
Open

Coalesce concurrent shell restarts#10634
chivopic wants to merge 1 commit into
omacom:quattrofrom
chivopic:fix/restart-shell-concurrency

Conversation

@chivopic

@chivopic chivopic commented Sep 7, 2026

Copy link
Copy Markdown

Summary

Fixes #10098.

omarchy-restart-shell can be invoked twice in quick succession, for example by double-clicking the Dictation indicator. The first invocation may already have killed the old shell and started its replacement when the second invocation enters the kill loop, allowing the second restart to terminate the new Quickshell instance while it is still loading.

Coalesce overlapping restart requests with a non-blocking flock in the user runtime directory. The in-flight restart already satisfies the overlapping request, so later concurrent invocations exit successfully instead of entering the kill/relaunch sequence again.

This follows the existing Omarchy pattern used for other duplicate event-driven actions such as display brightness handling.

Testing

  • Added test/shell.d/restart-shell-concurrency-test.sh.
  • The regression holds the restart lock, invokes omarchy-restart-shell, and asserts the overlapping invocation exits successfully before reaching lock inspection, Quickshell kill, or Hyprland dispatch.
  • Verified the focused lock-behavior harness locally.

@chivopic
chivopic requested a review from dhh as a code owner September 7, 2026 12:09
Copilot AI lite review requested due to automatic review settings September 7, 2026 12:09

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@sayem314

sayem314 commented Sep 7, 2026

Copy link
Copy Markdown

@robosayem review this

@robosayem robosayem left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verified on head d7f1f10 — approve.

  • restart-shell-concurrency-test passes, and the existing restart-shell-test still passes with the lock in place (8 assertions including locked-session preservation and crash recovery, which exercise the script far past the new guard)
  • The guard sits after the CONFIG_DIR existence check, so a misconfigured invocation still fails fast with its real error instead of being silently coalesced
  • Lock-hold duration is bounded by construction: the relock loop runs to a SECONDS deadline, the duplicate-instance kill loop is wrapped in timeout 5, and the new-shell ping loop is capped at 20 attempts — so a wedged restart releases the lock eventually and cannot permanently suppress future restarts
  • The idiom matches existing Omarchy code exactly (omarchy-brightness-display and omarchy-system-lock both use flock -n || exit 0 on an XDG_RUNTIME_DIR-based lock), so this is the established coalescing pattern, not a new invention

One tradeoff worth naming for the record: exit 0 on contention means an overlapping invocation also swallows the first attempt's failure — if the in-flight restart dies mid-flight, a user's immediate retry no-ops. The kernel releases the lock when the holder exits, so a later retry always works; acceptable, just noting the semantics.

Also for whoever merges: this touches the same file as PR #9469 (insert at ~line 14 vs #9469's additions further down), no textual conflict, but rebase order matters for both.

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.

Double-click on the Dictation indicator runs omarchy-restart-shell twice and segfaults quickshell in IpcHandler::updateRegistration

4 participants