Skip to content

Detect and stop Omarchy screen recording via gsr-cli IPC - #10397

Open
calledtoconstruct wants to merge 10 commits into
omacom:quattrofrom
calledtoconstruct:fix/screenrecording-pgrep-name
Open

calledtoconstruct wants to merge 10 commits into
omacom:quattrofrom
calledtoconstruct:fix/screenrecording-pgrep-name

Conversation

@calledtoconstruct

@calledtoconstruct calledtoconstruct commented Sep 5, 2026 •

Copy link
Copy Markdown

The bar indicator used pgrep -f, which stays true on leftover command-line matches after the recorder has exited and also matches unrelated processes whose args mention gpu-screen-recorder. pgrep -x cannot see gpu-screen-recorder at all: /proc/pid/comm is 15 characters and the name is 19. pidof treats any process by that name as an Omarchy recording, including a systemd replay-buffer unit whose argv0 is an absolute path. Stop still used pgrep/pkill -f "^gpu-screen-recorder", which misses that argv0, so Stop can claim a save with an empty filename.

Omarchy now starts gpu-screen-recorder with -ipc "${XDG_RUNTIME_DIR:-/tmp}/omarchy-gsr.sock". The capture helper, bar indicator, and menu when: all use gsr-cli -ipc <socket> status (exit 0 if that instance is running). Stop uses gsr-cli -ipc <socket> stop, so only the instance Omarchy started is gated and stopped. If that stop fails, SIGINT is sent only to the PID recorded when this capture started — never every gpu-screen-recorder on the machine.

@omarchybot

Copy link
Copy Markdown
Collaborator

Reviewed on a disposable worker (Arch, procps-ng 4.0.7, gpu-screen-recorder 6.0.1): the diff read, the three substitutions exercised against real processes, ./test/cli and test/shell.d/{menu,menu-guards,bar,screenrecording-pgrep}-test.sh run there, and the indicator watched live in a two-monitor Hyprland session. Second reviewer: Codex at xhigh reasoning.

The core change holds up, and it is worth saying why explicitly. /proc/<pid>/comm for the recorder is gpu-screen-reco — 15 bytes — and pgrep -x gpu-screen-recorder refuses the pattern outright: pattern that searches for process name longer than 15 characters will result in zero matches, exit 1. pidof compares against argv[0], its basename, and /proc/<pid>/exe, so the 19-character name costs it nothing: it matched the real binary started as /usr/bin/gpu-screen-recorder, a copy invoked bare through PATH, and one launched with exec -a under an unrelated argv[0]. Live with two monitors and both bars constructed in the same shell start: nothing recording → the indicator stays hidden; a process named gpu-screen-recorder plus an indicator refresh → the glyph appears on both bars; the process gone → it clears, and the bar crop is pixel-identical to the idle one.

Two things worth changing before this lands.

1. The gate now sees recorders the stop path cannot kill. screenrecording_active() matches on the process name, but stop_screenrecording() still matches on the command line (bin/omarchy-capture-screenrecording:205,209,217,218). Those disagree for any recorder whose argv[0] is a path. Measured on the worker, with a recorder started as an absolute path: pidof -q gpu-screen-recorder → 0, pgrep -f "^gpu-screen-recorder" → 1, and pkill -SIGINT -f "^gpu-screen-recorder" left it running. This is not hypothetical: the gpu-screen-recorder package ships gpu-screen-recorder.service (the replay-buffer service, disabled by default) with ExecStart=gpu-screen-recorder ..., and systemd resolves a bare ExecStart name to an absolute path and passes that as argv[0] — I confirmed that with a transient unit, whose argv[] came out as the resolved absolute path. So on a machine where that service is enabled, after this change: the bar shows a recording permanently; the menu offers Stop; and omarchy-capture-screenrecording — with or without --stop-recording — takes the stop branch at line 282, kills nothing, skips the wait, skips the force-kill branch, and ends at a "Screen recording saved" notification with an empty filename while the recorder keeps running. It also becomes impossible to start an Omarchy recording, since the toggle always lands on the stop path.

Either half fixes it, and which one is a call for the maintainer rather than for me: widen the killer (pkill -x is not available — same 15-byte limit — so kill -INT $(pidof gpu-screen-recorder)), or narrow the gate to the recording Omarchy actually started, which $RECORDING_FILE already implies is being tracked.

2. "Fixes #10311" is not established. The mechanism the body describes is not the one that issue documents, and I could not reproduce the issue in either direction. On the worker, with two monitors and both ScreenRecording instances starting together, the stock pgrep --quiet -f "^gpu-screen-recorder" gate returned 1 and the indicator stayed hidden — the false positive never appeared, so there was nothing here for pidof to fix. #10311's own evidence is pgrep exiting 0 while printing no matching line, which is an exit code arriving without a match; swapping the command cannot correct a wrong exit code. Codex added the detail that closes this off: nothing re-polls. The indicator refreshes only on Component.onCompleted, onBarChanged and indicatorHost.refreshRequested, and clicking a falsely-active indicator runs --stop-recording, which exits at line 285 without asking for a refresh — so a false positive, however it arises, stays on screen exactly as the reporter describes. Worth dropping the Fixes keyword unless you can reproduce it and show it gone, otherwise merging this closes an issue that may still be live.

One smaller thing, and the reason the body's first sentence does not quite land: a command line cannot be "left over" after the recorder exits — the /proc entry goes with it, and both gates return 1 the moment the process is gone (checked). What pgrep -f really costs you is the opposite pair: it matches an unrelated command line that happens to start with the name, and it misses a recorder invoked by absolute path. That is the honest case for this change, and it is a good one.

3. (low) The new test. test/shell.d/screenrecording-pgrep-test.sh:23-24 installs the EXIT trap before recorder_pid exists, so under set -u a failing cp or chmod makes the trap die on the unset variable instead of removing $tmp_dir. Codex also spotted a race I had missed: the background copy of sleep is checked with no synchronisation, so pidof can run in the window between fork and exec, when neither argv[0] nor the executable name is in place yet — an intermittent failure. Asserting on pidof gpu-screen-recorder containing $recorder_pid, after waiting for the child to exec, would fix both and would also stop the test passing on somebody else's recorder.

Nothing was pushed to the branch: the first item is a design choice, not a defect with one correct repair. The tests above all passed as they stand (menu-test.sh 121, menu-guards-test.sh 18, bar-test.sh 88, the new file 2, ./test/cli clean). Where Codex agreed with conclusions already reached its independence is not currently guaranteed; the re-poll gap and the fork/exec race are its own contributions, and both check out against the source. This is waiting on you.

For context, since two similar PRs are open at once: #10260 makes the same kind of change for the screensaver with pgrep -x omarchy-screensaver, and that one cannot work — omarchy-screensaver is 19 characters and pgrep -x rejects the pattern. This PR avoids that trap by not using pgrep -x at all.

@dec05eba

dec05eba commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

There is a better way to do this now. gpu-screen-recorder can be launched with the -ipc option (for example -ipc /tmp/gsr.sock. Another process can then do gsr-cli -ipc /tmp/gsr.sock status which exits with 0 if gpu-screen-recorder is running. That doesn't rely on looking at processes list and command line to find the process.

@calledtoconstruct

Copy link
Copy Markdown
Author

bcfc882

@dec05eba gpu-screen-recorder is now launched with -ipc "${XDG_RUNTIME_DIR:-/tmp}/omarchy-gsr.sock". Status and stop go through gsr-cli -ipc <socket> status|stop in the capture helper, the bar indicator, and the menu when:.

The gate and stop now share that socket, so a systemd replay-buffer unit with absolute argv0 is no longer treated as an Omarchy recording.

Fixes #10311 was dropped from the PR body.

The test no longer races on pidof and no longer forks a fake gpu-screen-recorder.

@dec05eba

dec05eba commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

Since you stop it with gsr-cli, why keep RECORDING_PID_FILE and do kill? the gsr-cli stop is enough

gsr-cli stop already saves and waits. A pid file and SIGINT/SIGKILL fallback is not needed.
@calledtoconstruct

Copy link
Copy Markdown
Author

9669588

RECORDING_PID_FILE and the SIGINT/SIGKILL fallback are gone. Stop is gsr-cli -ipc "$GSR_SOCKET" stop; it already saves and waits.

@calledtoconstruct calledtoconstruct changed the title Detect screen recording by process name, not command line Detect and stop Omarchy screen recording via gsr-cli IPC Sep 11, 2026
@omarchybot omarchybot added the bug Something isn't working label Sep 27, 2026
omarchybot and others added 4 commits October 2, 2026 07:08
quattro moved the recorder's state out of /tmp into a private runtime dir (omacom#8374). The gsr socket follows it there, and the bar indicator and menu guard resolve the same fallback, so all three name one socket and none of them falls back to a fixed /tmp name.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
refresh() reads Quickshell.env, but the file imported only Quickshell.Io, so every refresh threw "ReferenceError: Quickshell is not defined" before the status check ran and the indicator never turned on.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
gsr-cli refuses a second stop while the first is still saving ("GPU Screen Recorder is already stopping"). A second press of Stop during the save then deleted the filename file before the first stop read it, so the recording was never finalized and the saved notification named an empty file. Only the stop that succeeded clears it now.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Codex Medium <noreply@openai.com>
The status and stop calls also contain -ipc "$GSR_SOCKET", so the old assertion still passed with the flag dropped from the launch.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Codex Medium <noreply@openai.com>
@greptile-apps

greptile-apps Bot commented Oct 2, 2026 •

Copy link
Copy Markdown
Contributor

RetriggerConfidence Score: 5/5

[Medium risk] Replaces process-based screen recording detection with IPC socket.

The PR appears safe to merge; no outstanding findings remain.

Summary

The PR moves screen-recording detection and stopping to an Omarchy-specific gsr-cli socket. Since the previous review, the test has added a pkill stub to prevent webcam cleanup from signalling a live desktop process and tightened its stop-call assertion.

Diagram
%%{init: {'theme': 'neutral'}}%%
flowchart LR
  A[Capture helper starts recorder] --> B[Omarchy IPC socket]
  B --> C[Bar and menu query status]
  B --> D[Capture helper requests stop]
  D --> E{Stop succeeded?}
  E -->|Yes| F[Finalize recording and clear filename]
  E -->|No| G[Report failure and retain filename]
Loading

Reviews (4) · Last reviewed commit: "Assert the stop went to the Omarchy sock..."

Comment thread bin/omarchy-capture-screenrecording
Comment thread bin/omarchy-capture-screenrecording
Comment thread test/shell.d/screenrecording-ipc-test.sh
@omarchybot

Copy link
Copy Markdown
Collaborator

Re-reviewed at the new head on a disposable worker (Omarchy ISO, gpu-screen-recorder 6.1.0, a real Hyprland session running the shell from this branch), with Codex at medium effort as the second reviewer. Four commits are now on top of yours.

The bug is real, and the switch to -ipc fixes it. On quattro, with a process named gpu-screen-recorder-gtk running (the name of the GTK frontend), omarchy-capture-screenrecording --fullscreen took the stop path: it sent SIGINT and then SIGKILL to that unrelated process, and no recording started. On this branch the same process is left alone. The menu's Stop guard is false while nothing answers on the socket and true once something does. The bar indicator now appears and clears with the socket; the two idle crops are pixel-identical.

Pushed to your branch:

  • 72579f537 merges quattro, which conflicted. quattro has since moved the recorder's state out of /tmp into a private runtime directory ([Security] Keep screen-recording state out of world-writable /tmp #8374), so the socket now lives there too, as $RUNTIME_DIR/omarchy-gsr.sock. The bar indicator and the menu guard resolve the same fallback ($XDG_STATE_HOME/omarchy, then ~/.local/state/omarchy), so all three name one socket and none falls back to a fixed name in /tmp.
  • 6bee14430 adds import Quickshell to ScreenRecording.qml. refresh() calls Quickshell.env, but the file only imported Quickshell.Io. Every refresh threw ReferenceError: Quickshell is not defined before the status check ran, so with this PR the recording indicator could never turn on; the shell log on the worker showed it on every refresh. The test now checks for the import.
  • edb30553b deletes the filename file only when this stop actually succeeded. gsr-cli refuses a second stop while the first is still saving ("GPU Screen Recorder is already stopping"). A second press of Stop during a save therefore deleted the filename before the first stop read it, so the recording was never finalized and the "saved" toast named an empty file. Codex found this in gpu-screen-recorder's IPC source, and I confirmed it there.
  • 5ce5cd135 makes the test look for -ipc "$GSR_SOCKET" on the launch line. The status and stop calls contain the same string, so the old check passed with the flag removed from the launch (also Codex's catch).

What was not run: an actual recording. The worker has no GPU, and gpu-screen-recorder refuses to record on llvmpipe, so gsr-cli stop saving a real file was not exercised. The suite passed on the worker: screenrecording-ipc-test.sh 1/1, screenrecording-test.sh 26/26, menu-test.sh 127/127, menu-guards-test.sh 18/18, bar-test.sh 88/88. ./test/cli stops at vscode generated theme references current theme file, which fails the same way on quattro and is unrelated.

@calledtoconstruct, could you confirm on your own machine at 5ce5cd13543386d4aa987d89cb8077b1e1a01d1e? From a checkout of the branch, omarchy dev link it (or run bin/omarchy-capture-screenrecording from the checkout), then:

  • start a recording and check that the bar indicator shows;
  • stop it and check that the file in ~/Videos plays and the toast names it;
  • start one more and press Stop twice quickly, and check that the first save still comes through.

Two things are for the maintainer to decide, not defects with one correct repair, so nothing was pushed for them:

  • A recording started before the update is stranded. It was launched without -ipc, so after updating, the indicator, the menu and --stop-recording cannot see it, and the toggle starts a second recording. It ends at logout or with a manual pkill -INT gpu-screen-recorder.
  • Stop no longer has a timeout. gsr-cli waits for stop with no reply timeout, so a wedged recorder now hangs the stop where quattro force-killed it after five seconds. The flip side is screenrecording: 5s SIGINT grace before SIGKILL truncates recordings on slow or loaded systems #12469: five seconds truncates long recordings on slow machines, and this PR removes that limit.

Related: #11520, #12559 and #12471 also rewrite stop_screenrecording and will conflict with this one. #12559 (more than five seconds to mux, for #12469) becomes moot if this lands. Codex's second round found the pushed commits clean and nothing else open. Where it agreed with conclusions already written down, its independence is not guaranteed. The stop-twice defect and the test gap were its own findings, and both check out against the source.

This waits on your confirmation, then on the maintainer.

A successful notification was the last command, so --stop-recording
exited 0 after a failed stop. The stop test now runs that path.
Comment thread test/shell.d/screenrecording-ipc-test.sh
calledtoconstruct and others added 2 commits October 2, 2026 07:48
cleanup_webcam runs pkill -f WebcamOverlay. The stop test now stubs
pkill and checks that the cleanup hits the stub.
The status call logs the same socket, so checking for the socket and for stop separately passed with stop sent elsewhere.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Codex Medium <noreply@openai.com>
@omarchybot omarchybot added the verified Omarchy Triage has verified that this issue is ready for final review label Oct 2, 2026
@omarchybot

Copy link
Copy Markdown
Collaborator

Re-reviewed at the new head on a disposable worker (Omarchy ISO, gpu-screen-recorder 6.1.0), with Codex at medium effort as the second reviewer over three rounds.

return 1 on a failed stop is right. It keeps the filename, skips finalizing, and makes --stop-recording exit non-zero, and the new test now runs the stop path both ways. The pkill stub in b7fe0a2 does what it says: on the worker, the same test without that stub killed a live process named WebcamOverlay, and with it the process survived.

The fix still holds at this head. With an unrelated process named gpu-screen-recorder-gtk running, --stop-recording on quattro took it for a recording, killed it and reported a force-kill. On this branch, that process is left alone and the command exits 1. Putting the old pgrep -f gate back into this branch's script makes it take the stop path again.

Pushed to your branch: ad542085d makes the stop test assert the exact line -ipc <runtime>/omarchy-gsr.sock stop. It used to check for stop and for the socket separately, and the status call logs the same socket, so a stop sent to the wrong socket still passed. Codex found this, and its last round on this head found nothing else open.

Tests run on the worker at ad542085d: screenrecording-ipc-test.sh 4/4, screenrecording-test.sh 26/26, menu-test.sh 127/127, menu-guards-test.sh 18/18, bar-test.sh 88/88. ./test/cli fails only vscode generated theme references current theme file, and that test fails the same way on quattro. This push touched only the test, so the indicator was not watched again; it was checked live at 5ce5cd135.

Still not run: a real recording, because the worker has no GPU. @calledtoconstruct, the request from the last comment still stands, now at ad542085d893733ae8b1fbcc79273b6a91779e82: start a recording and check that the bar indicator shows; stop it and check that the file in ~/Videos plays and the toast names it; then start one more and press Stop twice quickly, and check that the first save still comes through.

This waits on that confirmation, then on the maintainer.

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

Labels

bug Something isn't working verified Omarchy Triage has verified that this issue is ready for final review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants