Kill the local plugin watcher with the shell via pdeathsig - #11385
chadmandoo wants to merge 2 commits into
Conversation
The plugin watcher ran as a bare Process, so an exit that skips destructors left it behind. Qt leaves through _exit() when the Wayland connection fails, raising no signal, and the inotifywait was then reparented to `systemd --user` where it held an inotify instance for the rest of the session. A deliberate omarchy-restart-shell does stop it cleanly; the crash path does not. Enough of them exhaust fs.inotify.max_user_instances, 1024 per UID, after which every inotify_init1() in the session fails with EMFILE. The error then surfaces in whatever application next asks for a watch, with no hint of where it came from. A lock-path crash loop relaunched the shell 4437 times here and stranded 979 watchers, taking the user to 1025 of 1024 instances; Alacritty was the first thing to refuse to start. The clipboard watchers already solve this with setpriv --pdeathsig TERM, so this applies the established idiom to the plugin watcher and asserts it in plugins-test.sh the same way clipboard-test.sh does. Closes omacom#11383 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
a1d2410 to
f67f285
Compare
johnpippett
left a comment
There was a problem hiding this comment.
I recommend this change based on the automated checks below. This review used Codex assistance.
Source commit: f67f285d18e5a4c9800ebaabee44c258cc544f2a.
Base commit: 31bd80daa4613ffdee995ac27467fce5a2990806.
In the earlier isolated tests, the base watcher remained alive after an abrupt parent failure. The proposed watcher exited after the same failure.
Four process lifecycle cases used real inotifywait and setpriv processes with a test parent process. Both revisions delivered file events and stopped after a clean parent shutdown. After parent SIGKILL, the base watcher remained alive, but the proposed watcher exited.
The focused plugin tests returned status 0 on both commits. The new source assertion failed against the base, as expected. Private Bubblewrap namespaces kept all process signals inside the test environment. The source remained read-only.
Documentation note, which does not block this approval: the comment in PluginRegistry.qml includes omarchy-restart-shell among exits that skip cleanup. Your PR description says a deliberate restart stopped the unpatched watcher correctly. Please remove the restart claim from this comment and describe exits that skip destructors instead. This will keep the comment consistent with your reported results.
No real Quickshell crash or graphical session ran in these independent tests. The full shell suite did not complete, so this review makes no full-suite claim. These results address watcher cleanup, not the underlying shell crash.
Before submission, the source and base commits still matched these results. The tests were not repeated for this submission.
|
Independent reproduction on different hardware, with numbers that support the mechanism described here. System: Beelink SER (AMD Ryzen 7 6800U, amdgpu), Omarchy 4.0.0 ( I went looking for a RAM problem and found 398 orphaned The orphan count tracks the crash count almost exactly:
406 crashes, 398 orphans. I was about 45% of the way to the No coredump exists for any of those 406 exits. On the crash path itself, my journal shows the trigger is output loss during lock-surface creation: with It is a race rather than a certainty — Your note about the relaunch limiter is correct in practice. My crashes are 17-18s apart, about 3.5/minute, comfortably under The |
omarchy-restart-shell stops the shell cleanly and its watcher exits with it; on an unpatched shell a restart left no orphan, while kill -9 did. The comment named the restart among the exits that skip destructors, which would send the next reader after the wrong path. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Reviewed at ca954a0 against What ran, on a disposable Omarchy worker VM:
Pushed to your branch: ca954a0 corrects the code comment only. It listed Related: this is the fix for #7150. #11383 was closed as a duplicate of it, so the Who checked it: Claude Opus 5.5 only. The second opinion (Codex Medium) did not run: the reviewer answers a ping, but omabot refused the review because today's review budget is spent. So this is one model's review, and the item is not marked |
|
|
|
||
| const registrySource = fs.readFileSync(path.join(root, 'shell/services/PluginRegistry.qml'), 'utf8') | ||
| check( | ||
| /command: \[\s*"setpriv",\s*"--pdeathsig",\s*"TERM",\s*"inotifywait"/.test(registrySource), |
There was a problem hiding this comment.
Crash behavior remains untested The new assertion checks only that the source contains
setpriv --pdeathsig TERM. It cannot catch a regression where the plugin watcher starts but survives a shell crash, leaving the resource leak this PR addresses undetected. A lifecycle test that kills the watcher's owner and checks that the watcher exits would cover this behavior, as the clipboard watcher test does.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Closes #11383.
PluginRegistryspawned the local-plugin watcher as a bareProcess, so an exit that skips destructors left it running: Qt leaves through_exit()when the Wayland connection fails, raising no signal. Theinotifywaitwas reparented tosystemd --userand held an inotify instance for the rest of the session. A deliberateomarchy-restart-shelldoes stop it cleanly — I measured an unpatched shell across a restart and it left no orphan — so this is the crash path specifically.Enough of them exhaust
fs.inotify.max_user_instances(1024 per UID), after which everyinotify_init1()in the session fails withEMFILE. The error then surfaces in whatever application next asks for a watch, with nothing pointing back at the shell. On my machine a lock-path crash loop relaunched the shell 4437 times and stranded 979 watchers, taking the user to 1025 of 1024 instances; Alacritty was the first thing to refuse to start, with "too many open files".The fix
shell/plugins/clipboard/Clipboard.qmlalready solves this, and says so:This applies the same
setpriv --pdeathsig TERMidiom to the plugin watcher, which was the only long-livedProcesschild undershell/still missing it.Testing
Verified through Quickshell's real spawn path (Quickshell 0.3.1, Omarchy 4.0.3) with a minimal config, killing the shell with
SIGKILLso no destructor runs:kill -9of the shellsystemd --userAlso checked for regressions in the watcher itself:
inotifywaitundersetprivstill reportscreateandclose_writeevents normally, and the child was still alive after 12 seconds with the shell running, soPDEATHSIGisn't firing early on the spawning thread.test/shell.d/plugins-test.shgains an assertion mirroring the two inclipboard-test.sh. It fails on the unpatched tree and passes with the change.Ran the whole
test/shell.dsuite: 237 test files, 3 with failures —config-test.sh,snapper-test.shandunowned-system-paths-test.sh, all three asking for anomarchy-pkgscheckout I don't have. They fail identically on an unmodified tree.Not addressed here
The crash loop that exposed this looks like Quickshell's, not Omarchy's — the shell dies in lock-surface creation when the session is locked and the compositor has no valid Wayland output (
Could not create EGL surface (EGL error 0x3000), thenThe Wayland connection experienced a fatal error: Invalid argument), and that string isn't in this repo. The watcher should not outlive the shell regardless of what kills it.One related observation for whoever picks that up: the relaunch limiter in
bin/omarchy-launch-shellallows 5 relaunches per 60-second window and resets the window on expiry, so a loop slower than 5/minute runs indefinitely inside the limit. Mine crashed about every 13 seconds and never tripped it.🤖 Generated with Claude Code