Bound the browser policy refresh so a wedged browser can't stall an update - #9423
Open
VykosMolt wants to merge 1 commit into
Open
Bound the browser policy refresh so a wedged browser can't stall an update#9423VykosMolt wants to merge 1 commit into
VykosMolt wants to merge 1 commit into
Conversation
…pdate refresh_running_browser ran the browser's --refresh-platform-policy handoff in the foreground with nothing bounding it. A browser that does not return then holds up omarchy-theme-set-browser, the theme change, the migration that called it, and the update running that migration — reported as an hour-long hang at migration 1787481315, and again with Chromium taking 68 seconds at 1787515927. The refresh only asks an already-running browser to re-read the policy it was just handed. The file on disk is what the next launch reads either way, so losing the refresh costs a running browser its new colour until it restarts, while blocking on it costs the whole update. Use `timeout -k`, not a plain timeout. A browser that ignores TERM leaves plain `timeout` waiting exactly as long as the process it was meant to bound: timeout 2s <TERM-ignoring child> returned after 20s timeout -k 1s 2s <same child> returned after 3s The refresh keeps its existing exit contract: it never fed the command's status, which stays owned by the policy write. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
refresh_running_browserinbin/omarchy-theme-set-browserruns the browser's policy handoff in the foreground with nothing bounding it:A browser that does not return therefore holds up
omarchy-theme-set-browser, the theme change, the migration that called it, and the update running that migration. #8212 reports an hour-long hang at migration1787481315; a second report on the same issue has Chromium taking 68 seconds at1787515927.That second case matters for scoping. It was a correctly detected running Chromium, so this is not only the brave/brave-origin detection mix-up — #8258 fixes that detection, and this would still hang. The two are complementary.
The refresh only asks an already-running browser to re-read the policy it was just handed. The file on disk is what the next launch reads either way, so losing the refresh costs a running browser its new colour until it restarts, while blocking on it costs the whole update.
Why
-kand not a plaintimeout. A browser that ignoresSIGTERMleaves plaintimeoutwaiting exactly as long as the process it was meant to bound:Plain
timeoutdoes clean up the process tree when the child cooperates — parent and children all die, since it signals the group — it just cannot help when the child does not.The refresh keeps its existing exit contract. It never fed the command's status, which stays owned by the policy write.
Testing
New
test/shell.d/browser-policy-refresh-test.sh, hermetic: stub browsers on a privatePATH, and a stubpgrepthat answers only for the browsers the fixture declares running, so a browser actually installed on the machine running the suite is never probed or launched. It covers a browser that refreshes cleanly, one that never returns and ignores TERM, that later browsers are still refreshed after an earlier one hangs, that a timed-out refresh does not change the exit status, that no process is left behind, and that a failed policy write still fails the command.Against unpatched
quattroit fails in 30s on "converges when a browser refresh never returns". The harness bounds its own invocation so a regression fails rather than hanging the suite, and its cleanup kills the stub tree so a failure leaves nothing behind../test/shell— 218 of 222 files pass; the four failures (config-test,runtime-smoke-test,snapper-test,unowned-system-paths-test) fail identically on unmodifiedquattroon this machine.shellcheckreports the sameSC1091/SC2086counts before and after — no new findings.bash -nandgit diff --checkclean.Fixes #8212