Fix recording indicator stuck 'active' after a force-killed stop

[ReneXiong]

Summary

Stopping a recording could leave the bar showing "Stop recording" after the recorder had exited. The stop script refreshed during the five-second SIGINT grace period's fallback decision, before sending SIGKILL. A later click with no recorder running exited without refreshing, so the stale indicator persisted.

This PR refreshes before post-processing on a graceful stop and after observing recorder exit on a force-kill. After SIGKILL, it polls for up to one second in the foreground. If the recorder remains visible, a background waiter checks once per second and refreshes when it disappears, allowing the error notification and stop command to complete. A nonblocking flock in the private runtime directory keeps one waiter even if stop is pressed repeatedly. A redundant --stop-recording also refreshes before exiting 1.

This addresses the script-side state synchronization problem associated with #9205. Related PR #9296 proposes periodic QML polling and a probe watchdog; it remains open and unmerged. If a recorder exits outside this stop flow, clicking the stale indicator resynchronizes it. Automatic recovery after an independent recorder exit is separate work discussed in #9296.

Validation

• bash test/shell.d/screenrecording-test.sh at 594a75a: 35 assertions pass, covering delayed exit after SIGKILL, exit after the foreground wait expires, force-kill notification, repeated-stop waiter deduplication, redundant-stop resync, and graceful refresh before finalizing.
• The delayed-exit regression fails against the previous PR head, c9e86db, because the indicator's probe still reads recording.
• Live desktop: a short real recording saves a video readable by ffprobe and clears the indicator on normal stop. A real process named gpu-screen-recorder that ignores SIGINT reaches the force-kill path and clears the actual bar indicator after exit. Active/idle screenshots were inspected for both paths. Uninterruptible I/O was simulated in the command tests, not reproduced on the live desktop.
• bash -n for the changed script and test, and git diff --check.
• ./test/cli passes at 594a75a.
• Previous ./test/all run at 192c626: CLI passes; 235 of 244 shell test files pass. Eight unrelated failures also occur against c9e86db: three require the absent sibling omarchy-pkgs checkout, plus factory-reset accounts, About animation, network QR, kernel migration, and guarded pacman invocation. The ninth, locate, scans ignored Python bytecode in bin/__pycache__; copying that local cache into the baseline reproduces its UTF-8 decoding failure, and the clean baseline passes it.

Fixes #9205

[llstrk]

Automated AI review

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

Verified: the force-kill ordering fix and the redundant --stop-recording resync both work as described. On the force-kill path the indicator refresh now runs after pkill -9 and the error notification, and a redundant --stop-recording requests a refresh before exit 1, so a stale "active" icon recovers on the next click. Each of the four new assertions fails when its matching hunk is removed and passes at the PR head. The existing 26 assertions still pass.

One side effect remains on the normal (graceful) stop path.

Normal stop now keeps the indicator "active" through post-processing

The single refresh moved to the end of stop_screenrecording(), after both branches. When the recorder exits within the 5 s grace period, the bar keeps showing the recording icon ("Stop recording") until finalize_recording (ffprobe and the ffmpeg trim, loudnorm or libx264 re-encode), the preview thumbnail and the "saved" notification have all finished. The base refreshed as soon as the recorder was gone.

Event order on a graceful stop, from a stubbed run of the real script with the finalize ffmpeg call simulated as a 1.5 s step:

                       base          PR head       per-branch (suggested)
recorder seen gone     t=0           t=0           t=0
indicator refresh      +0.002 s      +1.528 s      +0.007 s
                       (before       (last event,  (before
                        finalize)     after notify) finalize)

Impact: the icon is misleading, with no data loss. How long it stays wrong depends on how long post-processing takes. Timing the head's own finalize commands on synthetic 1080p60 media on a fast 16-CPU host gave about 1.3 s per recorded minute with audio (loudnorm) and about 8 s per recorded minute on the libx264 re-encode path. Slower machines, like the throttled laptop that motivated this PR, will take longer. A click during that window still recovers because of this PR's redundant-stop refresh. History: aabaef83 moved the refresh ahead of post-processing, and this PR restores the earlier end-of-function placement.

Suggested change: refresh inside each branch, so the force-kill path keeps the new ordering and the graceful path refreshes before post-processing. This variant passes the PR's test file unchanged.

  if pgrep -f "^gpu-screen-recorder" >/dev/null; then
    pkill -9 -f "^gpu-screen-recorder"
    omarchy-notification-send -u critical -t 5000 "Screen recording error" "Recording process had to be force-killed. Video may be corrupted."
    toggle_screenrecording_indicator
  else
    toggle_screenrecording_indicator
    finalize_recording
    ...

Optional test improvement: the new tests do not exercise the graceful stop path, so the head's end-of-function placement passes them. An assertion that the refresh precedes the finalize ffmpeg call would catch this, using a pgrep stub that reports the recorder alive for the first calls and gone afterwards, plus logging ffprobe/ffmpeg stubs.

Optional: SIGKILL delivery is asynchronous, and the new ordering test only checks that pkill -9 was invoked first (its pgrep stub stays "alive" throughout). A recorder stuck in uninterruptible I/O could in principle still be visible to the indicator probe right after pkill -9. This was not reproduced, and the redundant-stop refresh now recovers it on the next click. A short bounded wait until pgrep stops matching would remove the dependency. The original indicator code had a 0.2 s wait for the same reason.

Description note: #9296 is still open and unmerged, so "the QML watchdog fixed in #9296" overstates its status. The redundant-stop resync also relies on the QML status probe finishing, because refresh() in ScreenRecording.qml returns early while statusProc is running. A stuck probe remains that PR's scope.

---

Review information

Test scope: source review of the script, the test file, the bar indicator QML and the omarchy-shell IPC path at head 3cef217f, plus the gpu-screen-recorder 6.1.0 stop handling. Isolated command-only runs covered test/shell.d/screenrecording-test.sh against the head, the base and single-hunk variants, a stubbed graceful-stop sequence, and timing of the finalize ffmpeg commands on synthetic media. No live recorder or desktop run was performed. QML probe timing after a real SIGKILL was assessed from source only. ./test/all and the live reproduction were not rerun.

AI process: Opus 5.5 Medium coordination and synthesis, independent Opus 5.5 Xhigh and GPT 6 Sol Xhigh technical assessments, Opus 5.5 Medium editorial check.

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

[omarchybot]

Reviewed and verified. I pushed one fix to your branch, c9e86db, which changes the graceful stop path.

The bug is real. I tested it on a disposable Omarchy VM with a real process named gpu-screen-recorder that ignores SIGINT, driven through the actual --stop-recording path. At the moment of the refresh, a stub omarchy-shell ran the same pgrep probe the bar's ScreenRecording.qml runs. On quattro the refresh happens while the recorder is still alive, so the probe reads "recording" and the recorder is then SIGKILLed with no further refresh. With this PR the refresh comes after the kill and reads idle. Without a recorder running, a redundant --stop-recording now refreshes before exit 1, where quattro exited without refreshing.

What I changed. Moving the single refresh to the end of stop_screenrecording also delayed it on the normal stop, where the recorder exits within the grace period. The bar then kept showing "Stop recording" through the whole ffprobe/ffmpeg finalize, the thumbnail and the saved notification, which can take many seconds on a long recording. @llstrk's review above found the same thing. c9e86db refreshes inside each branch: right after pkill -9 on the force-kill path, and before finalize_recording on the normal one. It also adds a test that the normal stop refreshes before finalizing. Your two changes and your tests are otherwise unchanged.

Tests, all on the VM: test/shell.d/screenrecording-test.sh passes 31 of 31, and ./test/cli is green. The force-kill test fails on quattro's script. The new graceful-stop test fails with your end-of-function placement, and also with the finalize step removed. CI has not run on this pull request yet.

Second opinion: Codex Medium reviewed it in two rounds. It agreed the bug is real and the change is proportionate; independence from my review is not guaranteed. It also found that the first version of my graceful-stop test could pass on the thumbnail's ffmpeg call even with finalizing removed, which is fixed in c9e86db. Its last round, on c9e86db, found nothing open. It noted two existing limits this pull request does not change, neither a regression. First, SIGKILL is asynchronous, so a recorder stuck in uninterruptible I/O could still be visible to the probe. Second, refresh() in ScreenRecording.qml returns early while a probe is already running. That second one is #9296's scope, so this does not close #9205 on its own.

Waiting on CI and then on the maintainer.

[greptile-apps]

Confidence Score: 5/5

[Medium impact] The PR appears safe to merge; no new actionable failure was established.

Summary

The PR refreshes the recording indicator after a graceful stop or confirmed force-killed exit, uses one background waiter when exit takes longer, and resynchronizes the indicator on a redundant stop. The merge also adapts the recording flow and its tests to PID-based recorder selection and --status.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart TD
  A[Stop requested] --> B[Send SIGINT and wait]
  B --> C{Recorder still visible?}
  C -->|No| D[Refresh indicator before finalizing]
  C -->|Yes| E[Send SIGKILL and wait briefly]
  E --> F{Recorder still visible?}
  F -->|No| G[Refresh indicator]
  F -->|Yes| H[One background waiter]
  H --> I[Recorder exits]
  I --> G

Loading

_{Reviews (4) · Last reviewed commit: "Merge quattro into fix-screenrecord-indi..." · Reviewed by Greptile}

[omarchybot]

Reviewed your new commit, 192c626, and verified it. I pushed one fix on top of it, cf67a23, plus two commits that tighten its test.

192c626 fixes a real case. I reproduced it on a disposable Omarchy VM with a process named gpu-screen-recorder blocked in uninterruptible I/O on a frozen filesystem, so it ignores SIGINT and stays visible after SIGKILL until the filesystem thaws. A stub omarchy-shell ran the bar's own pgrep probe at each refresh. On quattro, and on the branch as it stood at c9e86db, the only refresh came while the recorder was still visible, so the indicator stayed on "recording". With your commit it reads idle in both cases. When the recorder exits within the one-second wait, the refresh comes then. When it stays stuck past that, --stop-recording still returns after 6.4s with the error notification, and the background watcher refreshes once the recorder is gone.

What I changed. Every --stop-recording against a recorder that stayed stuck started another watcher, and none of them exited until the recorder did. Each click on the stale indicator added a process running pgrep once a second, and they all refreshed when it finally exited. cf67a23 has the watcher take flock -n on a lock in the private runtime directory, the same idiom omarchy-hyprland-monitor-watch uses, so the first watcher runs and later ones exit at once. The first watcher still does the refresh, so nothing is lost. A new test stops twice while the recorder is held and checks that exactly one watcher is left before releasing it. With your script at 192c626 that test fails. On the VM with the frozen filesystem, two stops gave two refreshes before the fix and one after. c2abe79 and 594a75a make that test deterministic: it waits for the second watcher to try the lock rather than relying on a timing window, and it fails if the second stop times out.

Tests, all on the VM at 594a75a: test/shell.d/screenrecording-test.sh passes 35 of 35 on several runs, and ./test/cli is green. Your delayed-exit test fails with your commit reverted.

Second opinion: Codex Medium reviewed it in four rounds. On 192c626 it found the piling-up watchers, which Greptile also flagged. In the next two rounds it found the two weaknesses in my test. It found nothing open on 594a75a. It agreed that the bounded wait plus an eventual refresh fits the problem, but independence from my review is not guaranteed. It also checked that the detached watcher cannot hang a caller that captures the script's output, because all three of its streams are redirected.

Waiting on CI, then on the maintainer.

[omarchybot]

This branch had stopped merging after #14431 reworked the recorder script on quattro, so I merged quattro into it and ported the fix: 0511ab4. Nothing about what the fix does has changed.

Why it still matters on quattro. quattro now finds and signals the recorder through omarchy-capture-screenrecording-process, by the pid it saved, and the bar asks --status. But stop_screenrecording still refreshes the indicator before the SIGKILL, and a redundant --stop-recording still exits without refreshing. Both bugs this PR fixes are still there.

The port. The force-kill wait and the background watcher now probe the recorder that stop selected, through the new helper, rather than pgrep. So the watcher keeps watching the same process after stop has removed the pid file. The graceful stop still refreshes before post-processing, and the redundant-stop refresh sits inside quattro's new lock. The new tests stub the helper in place of pgrep and pkill. They count watchers by the watch lock they hold, because an earlier stop's preview cleanup shares the script's command line. They also wait for the late refresh to read idle rather than for its file to appear.

Tests, on a disposable Omarchy VM:

• End to end, with the real helper and the bar's own --status probe at each refresh, against a process named gpu-screen-recorder that ignores SIGINT. On quattro the indicator reads "recording" after the force-kill, and a redundant stop does not refresh. At 0511ab4 both read idle.
• screenrecording-test.sh passes 40 of 40, three runs in a row. On quattro with the same tests, the force-kill ordering test fails. Removing the watcher lock, or the redundant-stop refresh, fails its own test.
• The other five screenrecording-* tests and ./test/cli pass.

Second opinion. Codex Medium reviewed the port in two rounds. It found nothing wrong in the script, and agreed the watcher no longer holds the toggle lock and still selects the right recorder. Its independence is not guaranteed. It also found the race in the late-refresh wait, which is fixed above.

Related. I linked #9205, which this fixes. #9296 addresses the same issue with a bar-side watchdog and a 2s poll. Asked separately, Codex Medium also preferred this PR: #9296 does not touch the refresh ordering, and against quattro it puts the bar back on a pgrep probe that misses wf-recorder. What only #9296 offers is that poll: it would clear the icon when the recorder dies by itself, where this PR needs one click. Whether the bar should poll is the maintainer's call.

This now waits on the maintainer. The repository's Tests workflow does not run on pull requests from forks, so there is no CI result here.

[jandrusk]

Still needed after #14431's recorder rework — indicator refresh must wait until the process is actually gone. omarchybot ported/verified. Related grace-window issue #12469 tracks #15122. Ready.