Give screen recording stop more than five seconds to mux

[Bartok9]

What

stop_screenrecording() used to SIGINT gpu-screen-recorder and then SIGKILL after a flat 5s. On a loaded or thermally throttled machine the muxer cannot write the MP4 trailer in that window, so the file is truncated (issue #12469).

Wait 30s by default (still a hard cap), overridable with OMARCHY_SCREENRECORD_STOP_TIMEOUT. Invalid values fall back to 30: non-digits, empty, and digit strings outside 1..5 length; accepted values are forced base-10 and clamped to 1..600.

Why

Reproduce without needing a slow machine: start a recording, pkill -STOP the recorder, then --stop-recording. Five seconds later the force-kill toast fires and the file is cut. Raising the grace period is the smallest change that matches the reported trailer-write failure mode.

Mitigates #12469 (longer post-SIGINT grace). Does not restore video that was already behind the container timeline while recording under -fm cfr; that path needs separate diagnosis with OMARCHY_SCREENRECORD_DEBUG=true / packet endpoints.

Test

CLI-only; no VM screenshots. ./test/cli / ./test/shell not run here (no local Omarchy tree on this host). The wait loop is the same shape as before; timeout parsing forces base-10 ASCII digits only (1..5 chars), clamps 1..600, and falls back to 30 for non-ASCII digits or longer digit strings.

[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 longer grace period works as described for recorders that need between 5 and 30 seconds to exit after SIGINT. With a stubbed recorder that needs 8 s, the base force-kills it at about 5.4 s with the "force-killed" toast, while this head waits about 8.1 s, runs the normal finalize step and shows the "saved" toast. A recorder that exits promptly is unaffected, because the loop ends at the first pgrep miss. Non-numeric values fall back to 30 s, and the script still exits 0 on both the saved and force-killed paths, so the ALT+PRINT || menu fallback is not triggered.

Two issues remain in the new code path, plus a note on the scope of "Closes #12469".

Leading-zero and very large timeout values bypass the fallback

^[0-9]+$ accepts values that Bash arithmetic then reads as octal or overflows:

OMARCHY_SCREENRECORD_STOP_TIMEOUT	Observed on this head (stubbed recorder)
unset, abc, -5, 2.5	30 s wait (intended fallback)
0	clamped to 1 s
010	8 s wait (octal), not 10 s
08, 09	value too great for base: the script exits 1 right after SIGINT, with no wait, no finalize, no toast, and the recording state file left behind; from ALT+PRINT the `
1000000000000000000	timeout_s * 10 overflows to a negative bound, so the recorder is SIGKILLed immediately

Impact: a user who writes a padded value such as 08 loses finalization entirely. The PR description's "Invalid values fall back to 30" holds only for non-digit input.

Suggested change: force base 10 and cap the value before multiplying, for example (untested sketch):

[[ $timeout_s =~ ^[0-9]{1,5}$ ]] || timeout_s=30
timeout_s=$((10#$timeout_s))
((timeout_s < 1)) && timeout_s=1

A second stop during the longer wait runs a concurrent finalize

While stop_screenrecording() waits, the recorder is still visible to pgrep, the bar indicator still shows recording (it is refreshed only after the loop) and no notification is shown. Any stop entry point (ALT+PRINT, the bar indicator, the menu) therefore starts a second, independent stop:

t=0s  stop #1: SIGINT, waits          recorder needs 8 s (stub)
t=1s  stop #2: SIGINT again, waits    (no lock, no "finishing" notice)
t=8s  recorder exits
      stop #1 + stop #2: both finalize the same file
      -> two concurrent ffmpeg writers to the same *-processed.mp4
      -> one mv fails, two "Screen recording saved" toasts

The race already exists on the base (there it produces a "force-killed" error next to a "saved" toast with an empty path), but the window grows from 5 s to 30 s, and it opens in exactly the slow case this PR targets. Whether real ffmpeg output is corrupted by the two writers was not tested; the duplicate concurrent processing was observed with stubs.

Impact: a user who presses stop again because nothing seems to happen gets duplicate processing of the same recording and contradictory or duplicate notifications.

Suggested change: serialize the stop path (for example flock on a lock file in the runtime directory) and show a short "Finishing screen recording…" notification once the wait passes a second or two, as issue #12469 also suggested.

Scope of "Closes #12469"

In gpu-screen-recorder 6.1.0 (source read at the tag), SIGINT only clears a running flag, and the main loop checks that flag between iterations. With Omarchy's -fm cfr, one iteration encodes one frame for every missed frame slot without rechecking the flag (src/recorder/recorder.c, main loop and recorder_capture_and_encode_frame). The issue's files have video ending at 202.0 s and 188.0 s against container durations of 286.8 s and 234.2 s. That is a gap in the media timeline, meaning video was behind the rest of the file, rather than a measurement of how long trailer writing takes. Waiting longer after SIGINT cannot restore video that was already missing when stop was pressed, and nothing in the issue shows that 30 s covers the reporter's shutdown time.

Suggested change: describe the PR as mitigating #12469 rather than closing it, or confirm on the affected machine first. OMARCHY_SCREENRECORD_DEBUG=true logs gpu-screen-recorder's periodic update fps lines, and those, together with the file's per-stream packet endpoints, would show whether the video path was falling behind during recording. The issue's SIGSTOP reproduction also still ends in SIGKILL on this head (after 30 s instead of 5 s), so it demonstrates the failure rather than the fix.

Documentation note: OMARCHY_SCREENRECORD_STOP_TIMEOUT is only mentioned in a comment inside the function. It could go in the script's # Env: header next to OMARCHY_SCREENRECORD_USE_PORTAL and OMARCHY_SCREENRECORD_DEBUG, and in the uwsm env.d guidance, since the keybinding and the bar/menu stop paths can inherit different environments.

---

Review information

Test scope: Source review of the pinned head against its merge-base, including callers (keybinding, menu, bar indicator, webcam wrapper) and gpu-screen-recorder 6.1.0 source. Stop-path behavior was exercised in isolated sandboxes with stubbed pgrep, pkill, ffmpeg, notifications and a fake recorder; timings above come from those stubs. No real gpu-screen-recorder, GPU, display, audio or ffmpeg run, and the repository test suite was not run (its screen recording test covers only the start path). The explanation of the #12469 files is source analysis, not a reproduction on the reporter's hardware.

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 on the gpu-screen-recorder stop path, Opus 5.5 Medium editorial check.

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

[Bartok9]

Thanks for the careful sandbox review — appreciated.

Pushed a follow-up that hardens OMARCHY_SCREENRECORD_STOP_TIMEOUT parsing: ^[0-9]{1,5}$, 10# base-10 force, and clamp to 1..600 before the * 10 loop bound, plus an # Env: header note. That covers the leading-zero / overflow cases you listed.

On the rest:

• Concurrent second stop: agree the race is real and the longer window makes it more visible. Left out of this PR on purpose (pre-existing on base; wants a stop lock + “Finishing…” toast as its own change). Happy to take that next if maintainers want it bundled.
• Closes screenrecording: 5s SIGINT grace before SIGKILL truncates recordings on slow or loaded systems #12469: updated the description to Mitigates screenrecording: 5s SIGINT grace before SIGKILL truncates recordings on slow or loaded systems #12469 — longer grace helps trailer write under load, but cannot restore video that was already behind under -fm cfr when stop was pressed. The SIGSTOP reproduction still ends in SIGKILL after the grace period by design.

Happy to adjust further if maintainers prefer different bounds or want the flock path in this PR.

[llstrk]

Automated AI review

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

Follow-up to the earlier review and the author's reply, checked at head 2477ae0.

Outcome: the timeout parsing issue, the "Closes #12469" scope note and the documentation note are resolved. The concurrent second stop is still present, and the author has deferred it to a separate change. One newly found edge remains in the parsing: timeout values written with non-ASCII digits abort the stop in some glibc UTF-8 locales such as en_US.UTF-8.

Earlier finding	Status on 2477ae0
Leading-zero and very large timeout values bypass the fallback	Resolved
A second stop during the longer wait runs a concurrent finalize	Still present (deferred by the author)
Scope of "Closes #12469"	Resolved: the description now says "Mitigates #12469", and GitHub lists no closing-issue reference
Documentation of OMARCHY_SCREENRECORD_STOP_TIMEOUT	Resolved in the script's # Env: header. The uwsm env.d part was not adopted, which matches OMARCHY_SCREENRECORD_USE_PORTAL and OMARCHY_SCREENRECORD_DEBUG next to it in the header; neither is documented there

Timeout parsing

Verified: every value from the earlier table now either parses as decimal or falls back to 30 s, and stays within 1 to 600 s. The recorder stub keeps running after SIGINT, so each wait below is the loop bound (loop iterations × 0.1 s, not wall-clock time):

OMARCHY_SCREENRECORD_STOP_TIMEOUT	db9bc49 (earlier review)	2477ae0
010	8 s (octal)	10 s
08, 09	exit 1, value too great for base, no finalize	8 s, 9 s, then the force-kill toast, exit 0
0600	384 s (octal)	600 s
1000000000000000000	immediate SIGKILL (overflow)	30 s (fallback)
1000, 99999	no upper cap (the stub recorder exited at its 700 s safety limit first)	600 s (clamp)

The earlier Verified results still hold on this head:

• A recorder that needs 8 s is saved at about 8.0 s, while base force-kills it at about 5.3 s (real-time runs).
• A recorder that exits promptly adds no wait.
• Non-numeric ASCII values fall back to 30 s.
• The saved and force-killed paths both exit 0.

The keybinding, menu, bar indicator and webcam wrapper all reach the same stop branch, so they get the same exit status and toasts. For ASCII input, results were identical in the C, C.UTF-8 and en_US.UTF-8 locales.

Documentation note: the header's "oversized digit strings" could say "more than five digits", because 99999 is clamped to 600 s while 100000 falls back to 30 s. The description's Test section says the parsing "rejects octal/08/09", but those values are now accepted and read as decimal.

Non-ASCII digits pass the check and abort the stop (newly found)

In a glibc UTF-8 locale such as en_US.UTF-8, a [0-9] range inside Bash =~ follows the locale's collation order, so it also matches many non-ASCII digits. A full scan of non-ASCII code points found 1,040 matches, among them fullwidth ３ and Arabic-Indic ٣. C and C.UTF-8 gave none. Such a value passes the check, and $((10#$timeout_s)) then fails:

LC_ALL=en_US.UTF-8  OMARCHY_SCREENRECORD_STOP_TIMEOUT=３０  (stubbed recorder)
  SIGINT sent
  line 233: 10#: invalid integer constant
  exit 1: no wait, no indicator refresh, no finalize, no toast, recording state file left behind

The same class of failure was already present in db9bc49 (^[0-9]+$), and the earlier review's own suggested sketch kept the same [0-9] range.

Impact: only a value written with non-ASCII digits (for example, typed with a CJK input method) triggers it. Every stop then skips post-processing and the toast, and ALT+PRINT falls through to the capture menu. The recorder has already received SIGINT, so a real gpu-screen-recorder probably still writes its file. That part was not tested.

Suggested change: use ^[[:digit:]]{1,5}$ or ^[0123456789]{1,5}$. In a separate scan of the same code points, neither matched any non-ASCII character, and both still accept every value from 0 to 99999.

Second stop during the wait (still present)

The stop path after the parsing block is unchanged. On this head, with stubs, the recorder needed 8 s and a second stop started 1 s after the first. Both stops then finalized the same recording: two ffmpeg writers overlapped on the same *-processed.mp4, one mv failed, and two "Screen recording saved" toasts appeared. The window now lasts up to the configured timeout (at most 600 s), but only while the recorder is still running. A second SIGINT does not affect gpu-screen-recorder 6.1.3 itself, because its handler only clears the running flag.

Impact: the same as in the earlier review: pressing stop again during a slow shutdown leads to duplicate processing and duplicate toasts.

Suggested change: a separate change, as the author proposes, is a reasonable scope. Open #11520 already adds a non-blocking flock guard to the stop path, so the follow-up could build on it or be coordinated with it.

Related open PRs: #13362 and #10397 also rewrite this stop loop. #13362 replaces the pgrep check with its own process helper but keeps count < 50. Resolving the conflict needs this PR's max together with that helper, or the 5 s bound returns. #10397 stops the recorder with gsr-cli ... stop instead of SIGINT and polling, so this timeout would not apply there.

---

Review information

Test scope: Source review of head 2477ae0 against the previously reviewed db9bc49 and the base, covering the stop callers and gpu-screen-recorder 6.1.3 source. The stop path was exercised in isolated sandboxes on all three revisions. pgrep, pkill, ffmpeg and the notification helpers were stubbed, and so was sleep in the parsing matrix. Runs used Bash 5.3 and glibc 2.44 in the C, C.UTF-8 and en_US.UTF-8 locales, and the parsing table's waits are loop-bound counts, not wall-clock measurements. No real recorder, GPU, display or ffmpeg was used, and the repository test suite was not run. Statements about #11520, #13362 and #10397 come from reading their diffs; none of them was merged or run.

AI process: Opus 5.5 Medium coordination and synthesis, Opus 5.5 Xhigh technical follow-up review and final fact check, GPT 6 Sol Xhigh search for related issues, Opus 5.5 Medium editorial check.

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

[Bartok9]

Thanks for the follow-up — good catch on locale [0-9].

Pushed a fix that matches explicit ASCII ^[0123456789]{1,5}$ before the 10# coerce, so fullwidth/Arabic-Indic digits fall back to 30 instead of aborting the stop path under glibc UTF-8 locales. Also tightened the # Env: wording (more-than-five-digit vs oversized) and the PR Test blurb so it no longer claims 08/09 are rejected (they are accepted as decimal).

Still leaving the concurrent second-stop / flock + “Finishing…” toast to a separate change (and happy to coordinate with #11520 if maintainers prefer). Related stop rewrites in #13362 / #10397 noted — will rebase as needed when those land.

[llstrk]

Automated AI review

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

Follow-up to the previous review and the author's reply, checked at head 7ce02b9.

Outcome: Verified on 7ce02b9: the non-ASCII digit abort and both documentation notes from the previous review are resolved, and no regression was found in the tested scope. The concurrent second stop is unchanged and remains deferred by the author.

Finding	Status on 7ce02b9
Non-ASCII digits pass the check and abort the stop	Resolved
Header wording "oversized digit strings"	Resolved: now "more-than-five-digit strings"
Description Test section said 08/09 are rejected	Resolved: now describes base-10 parsing, clamping and the fallback
A second stop during the longer wait runs a concurrent finalize	Still present (deferred by the author; stop code after the parse line unchanged)
Leading-zero and very large values, "Closes #12469" scope, # Env: header	Still resolved, rechecked on this head

Timeout parsing

Verified: ^[0123456789]{1,5}$ matched none of the non-ASCII Unicode code points in C, C.UTF-8 or en_US.UTF-8. The previous ^[0-9]{1,5}$ matched 1,040 in en_US.UTF-8, including characters that are not decimal digits, such as ² and ½. Every ASCII digit string of 1 to 5 characters is still accepted and parsed as the same decimal value.

Running the script's stop path with stubs in en_US.UTF-8:

OMARCHY_SCREENRECORD_STOP_TIMEOUT	2477ae0	7ce02b9
３０, ٣٠, 3٠	exit 1 right after SIGINT: no wait, no toast, no finalize	exit 0, 30 s fallback
３０, stub recorder exits after 80 polls (8 s)	(same abort)	exit 0, "Screen recording saved"

For all ASCII values, including 08, 010, 0600, 99999, 100000 and non-numeric input, results were identical on both heads in all three locales. Waits are loop-bound counts from a stubbed sleep (iterations × 0.1 s), not wall-clock times. The author's reply matches these results.

Related open PRs

The previous note on #11520, #13362 and #10397 still applies. Three details for whoever resolves the overlap:

• Detect and stop Omarchy screen recording via gsr-cli IPC #10397: it calls gsr-cli ... stop, which sets no receive timeout for the reply (gpu-screen-recorder 6.1.3, tools/gsr-cli/main.c). The recorder sends that reply only once it has stopped recording and written the file (src/cli/main.c). If that approach replaces this stop loop, the hard cap and the force-kill path go away with it. The combined version then needs to decide whether a cap is still wanted.
• Converge omarchy-mac and omarchy-mx-mac into upstream Omarchy #13362: besides the loop line, it also conflicts in the script's # Env: header block, so the new OMARCHY_SCREENRECORD_STOP_TIMEOUT lines need keeping as well as max.
• fix(capture): single-flight screen recording stop and skip empty (#11508) #11520: if the second-stop fix builds on it, note that its lock is a fixed name in /tmp (/tmp/omarchy-screenrecord.lock). This script's own comment explains why its other state files were moved out of fixed /tmp names into $RUNTIME_DIR. Putting the lock there too would be consistent. Only fix(capture): single-flight screen recording stop and skip empty (#11508) #11520's patch text was checked; the effect of a pre-created lock file was not tested.

---

Review information

Test scope: Source review of head 7ce02b9 against the previously reviewed 2477ae0 and the base. The script's stop path and its timeout parsing ran in isolated sandboxes on those revisions, with pgrep, pkill, sleep, ffmpeg and the notification helpers stubbed. Runs used Bash 5.3 and glibc 2.44 in the C, C.UTF-8 and en_US.UTF-8 locales. Other locales rely on reading glibc's regex source, not execution. No real recorder, GPU, display or ffmpeg was used, and the repository test suite was not run (its screen recording test exercises only this script's start path). The concurrent second stop was checked by source comparison only this round. Statements about #10397, #11520 and #13362 come from reading their diffs, a textual merge simulation and gpu-screen-recorder 6.1.3 source; none of the merged results was run.

AI process: Opus 5.5 Medium coordination and synthesis, Opus 5.5 Xhigh technical follow-up review and final fact check, GPT 6 Sol Xhigh search for related issues, Opus 5.5 Medium editorial check.

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

[Bartok9]

Thanks for re-verifying on 7ce02b9 — glad the non-ASCII digit abort and docs notes are clean.

Still holding the concurrent second-stop / flock + “Finishing…” toast for a follow-up (and will align with #11520 / rebase against #13362 or #10397 if those land first). No further code changes planned on this PR unless maintainers want that bundled.