fix(desktop): macOS/Linux pre-CEF stale-lock reap + installer pre-install kill (#4395)#4405
Conversation
…tall kill (tinyhumansai#4395) Follow-up to tinyhumansai#3605; the Windows-only runtime reap landed in tinyhumansai#3793. macOS and Linux had no pre-CEF single-instance guard, so a process holding the CEF SingletonLock past the cache-wait budget could be a healthy running primary, not a wedged one — the reason the earlier macOS SIGKILL escalation was reverted. Part 1: add `cef_stale_reap` (macOS/Linux), a pre-CEF reap of a wedged prior instance gated on a post-update relaunch marker (the staleness signal from tinyhumansai#4395). The updater writes the marker before `app.restart()`; the relaunched process consumes it (once, deleted, age-bounded). Only a fresh marker + a live lock holder triggers a reap. Guards: skip self, skip cross-host holders, and SIGTERM → grace → re-validate the same pid still owns the lock → SIGKILL (closes the PID-reuse window, honors kill_pid_force's contract). Never reaps a healthy instance. Wired into run() before cef_preflight::wait_for_cache_release. Part 2: add an NSIS NSIS_HOOK_PREINSTALL that terminates openhuman-core.exe and the main binary tree before any file copy, so a stale prior-version process can't survive an update. Adds the nix `hostname` feature for gethostname. Pure decision logic is unit tested on any host; the cfg-gated runtime/installer paths need verification on real macOS, Linux, and Windows(installer) hosts.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 27 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a macOS/Linux-only pre-CEF stale lock reap mechanism (marker-based staleness detection, ownership revalidation, SIGTERM/SIGKILL escalation) wired into update install and startup flow, enables the nix hostname feature, and adds a Windows NSIS pre-install hook to kill prior OpenHuman processes, wired via tauri.conf.json. ChangesmacOS/Linux stale CEF lock reap
Windows installer pre-install process kill
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant AppStartup as run()
participant Reap as cef_stale_reap
participant LockFile as SingletonLock
participant Marker as relaunch marker
participant PriorProc as prior process
AppStartup->>Reap: reap_stale_cef_lock_holder()
Reap->>LockFile: read symlink for (host, pid)
Reap->>Marker: consume_marker()
Reap->>Reap: decide_reap(holder, marker, self, host)
alt should reap
Reap->>PriorProc: SIGTERM
Reap->>Reap: wait TERM_GRACE
Reap->>LockFile: holder_still_owns_lock()
Reap->>PriorProc: SIGKILL
else skip
Reap-->>AppStartup: defer to cef_preflight
end
AppStartup->>AppStartup: wait_for_cache_release()
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
coderabbitai
Bot
added
bug
rust-core
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3450adc113
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| #[cfg(any(target_os = "macos", target_os = "linux"))] | ||
| cef_stale_reap::reap_stale_cef_lock_holder(); |
There was a problem hiding this comment.
On macOS/Linux post-update relaunches where app.restart() starts the new process while the old CEF runtime is still tearing down, a fresh marker plus a live SingletonLock holder is an expected transient state—the existing preflight comment above calls this the common sequential relaunch race and waits for it to clear. Running the marker-gated reaper before that wait means an otherwise healthy prior instance gets SIGTERM/SIGKILL as soon as the new process reaches startup, rather than only when it is actually wedged, reintroducing forced termination during ordinary updates.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
app/src-tauri/nsis-hooks.nsh (1)
19-29: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider a brief wait after force-kill to avoid handle-release races.
taskkill /Freturns once termination is requested, not necessarily once the OS has fully released open handles/DLL locks. Given the whole point of this hook is to prevent a "stale process still holding files" scenario, a shortSleep(e.g. 500ms–1s) after the two taskkill calls would reduce the residual race window before file copy begins.💡 Optional hardening
nsExec::Exec 'taskkill /F /IM ${MAINBINARYNAME}.exe /T' Pop $0 + ; Give the OS a moment to fully release file handles after force-kill. + Sleep 500 !macroend🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src-tauri/nsis-hooks.nsh` around lines 19 - 29, The NSIS preinstall hook in NSIS_HOOK_PREINSTALL should add a short delay after force-killing OpenHuman processes to avoid handle-release races before file replacement starts. Update the macro so that after the two nsExec::Exec taskkill calls (for openhuman-core.exe and ${MAINBINARYNAME}.exe), it pauses briefly with Sleep to give the OS time to fully release locks and handles. Keep the change localized to NSIS_HOOK_PREINSTALL and preserve the existing process-kill sequence.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/src-tauri/src/cef_stale_reap.rs`:
- Around line 151-152: The stale-reap flow currently trusts marker freshness
without tying it to the current lock owner, so update the logic around
consume_marker(), SingletonLock handling, and the marker write/read path to
require the parsed marker PID to match the live lock-holder PID before any
reaping occurs. In the code that builds the marker body and in the later reap
checks, propagate and compare the marker’s PID against the PID associated with
the active SingletonLock, and only proceed when both freshness and PID identity
match.
- Around line 101-105: The stale-lock reap logic in cef_stale_reap.rs should
fail closed when host identity cannot be confirmed and should revalidate the
full lock owner instead of only the PID. Update the host check around the
local_host/host comparison to skip reaping whenever the local hostname is
unavailable, and make the SIGKILL revalidation and any lock-owner checks compare
both host and pid as a pair. Apply the same host+pid validation consistently in
the related reap decision paths (including the helpers around the other marked
sections) so a reused PID on another host cannot be mistaken for the same lock
owner.
---
Nitpick comments:
In `@app/src-tauri/nsis-hooks.nsh`:
- Around line 19-29: The NSIS preinstall hook in NSIS_HOOK_PREINSTALL should add
a short delay after force-killing OpenHuman processes to avoid handle-release
races before file replacement starts. Update the macro so that after the two
nsExec::Exec taskkill calls (for openhuman-core.exe and ${MAINBINARYNAME}.exe),
it pauses briefly with Sleep to give the OS time to fully release locks and
handles. Keep the change localized to NSIS_HOOK_PREINSTALL and preserve the
existing process-kill sequence.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e50de52b-a491-44d2-b315-2a93baf73034
📒 Files selected for processing (5)
app/src-tauri/Cargo.tomlapp/src-tauri/nsis-hooks.nshapp/src-tauri/src/cef_stale_reap.rsapp/src-tauri/src/lib.rsapp/src-tauri/tauri.conf.json
|
Fixed the PR Submission Checklist lane (the only red check). The parser (
Verified locally: |
…ansai#4395 review) Codex + CodeRabbit review — three safety tightenings so the pre-CEF reaper can only ever signal a provably-wedged pre-update instance: - Fail closed on host identity. An unresolved local hostname previously still permitted a reap; now it skips (can't prove the SingletonLock holder is same-host). Revalidate the full (host, pid) owner — not pid alone — in holder_still_owns_lock, so a reused numeric pid on another machine (shared/NFS cache path) can't be mistaken for the holder we decided to reap. - Bind the marker to the lock holder. consume_marker now returns the pid the marker recorded (not just freshness); decide_reap requires marker_pid == the live holder pid, so a leaked-but-fresh marker can't authorize killing an unrelated same-host process that happens to own the lock now. - Don't reap before the normal relaunch teardown. A fresh marker + live holder is the expected transient on an ordinary update (the old runtime is still tearing down). reap_pid now waits a bounded RELAUNCH_TEARDOWN_GRACE and re-checks ownership before SIGTERM, so only a still-wedged holder is signalled — a healthy instance is never force-terminated during a routine update. Tests: (host, pid) revalidation incl. wrong-host, marker-pid mismatch skip, fail-closed-on-unresolved-host skip, consume_marker returns the recorded pid. 15 cef_stale_reap tests pass.
2 participants
Summary
cef_stale_reap) that unblocks an updated app when a wedged prior instance keeps holding the CEFSingletonLock— the Background processes from old app versions break updates — critical #3605 symptom, previously fixed on Windows only (fix(tauri): proactively reap wedged stale process on Windows startup (#3605) #3793).kill_pid_force's contract).NSIS_HOOK_PREINSTALLthat terminatesopenhuman-core.exeand the main binary tree before any file copy, so a stale prior-version process can't survive the install/update.Problem
PR #3793 fixed the "wedged stale process blocks the update" bug (#3605) on Windows only; the cross-platform pieces were deferred here (#4395) because they are unsafe without extra guards.
On Windows a Win32 named mutex early in
run()forces any concurrent peer toexit(0)before the reap, so a survivor holding the CEF cache lock is provably wedged. macOS/Linux have no equivalent pre-CEF guard (tauri_plugin_single_instanceregisters later, after CEF init), so a process holding the CEFSingletonLockpast the cache-wait budget may be a healthy running primary — a live app holds its lock for its whole lifetime. Reaping it purely on "held N seconds" could SIGKILL a healthy instance and lose user data (exactly why the earlier macOS SIGKILL was reverted, flagged by @oxoxDev + Codex P1).Solution
cef_stale_reap(macOS/Linux), new module. The updater writes a relaunch marker immediately beforeapp.restart(); the freshly launched process consumes it (once, deleted, age-bounded to 5 min). Only a fresh marker + a live CEF-lock holder is treated as stale — precisely the post-update wedge, never a normal launch of a healthy app. Extra guards: never reap self; skip when the lock's<hostname>-<pid>host ≠ ours (NFS/networked-home pid-collision safety); SIGTERM, grace, then re-read the lock to confirm the same pid still owns it and is alive before SIGKILL.run()immediately beforecef_preflight::wait_for_cache_release()forcfg(macos, linux). Complementary toprocess_recovery::reap_stale_openhuman_processes, which deliberately skips when a live CEF-lock holder exists.bundle.windows.nsis.installerHooks→nsis-hooks.nshwithNSIS_HOOK_PREINSTALLrunningtaskkill /F /Tonopenhuman-core.exeand the main binary before files are copied.hostnamefeature to the existingnixunix dependency (forgethostname); no new crates.Submission Checklist
Rust Tauri Coverage (cargo-llvm-cov)job passed (green) on this PR, which builds + testsapp/src-tauriand enforces the ≥ 80% changed-line coverage gate. (Thereap_pidSIGKILL branch itself is a thin OS-kill wrapper; the decision matrix that drives it is fully unit-tested.)N/A: no matrix feature row for this platform-lifecycle guard (behaviour is startup-recovery internal, not a user-facing feature ID).## Related—N/A: no feature IDs affected.N/A: no smoke-doc change; verification is the real-host matrix noted under Impact.Closes #NNNin the## RelatedsectionImpact
cfg-gated) + Windows (NSIS installer hook). No mobile/web/CLI behaviour change.cfg(...)-gated runtime paths and the NSIS hook must be verified on real macOS, Linux, and Windows(installer) hosts — they cannot be exercised cross-platform on a single dev machine.Related
AI Authored PR Metadata (required for Codex/Linear PRs)
Linear Issue
Commit & Branch
fix/GH-4395-crossplatform-single-instance-guardValidation Run
pnpm --filter openhuman-app format:check—N/A: frontend (app/src) untouched; the diff isapp/src-taurionly (theFrontend Checkslane is correctly skipped for this PR).pnpm typecheck—N/A: frontend untouched (same reason as above).cef_stale_reapunit tests compiled and ran green in CI: the passingRust Tauri Coverage (cargo-llvm-cov)job builds and executes theapp/src-tauritest suite.N/A: root core crate (src/) unchanged; theRust Quality (fmt, clippy)lane passed on this PR.cargo checkforapp/src-tauricompiles clean, verified green in CI by the passingRust Tauri Coveragejob that builds this crate.Validation Blocked
command:real-host runtime verification of thecfg-gated reap (macOS/Linux) and the NSISNSIS_HOOK_PREINSTALL(Windows installer).status:theapp/src-tauribuild, unit tests, and diff-coverage gate are now verified green in CI (Rust Tauri Coverage); what remains is the platform-gated runtime behaviour, which cannot be exercised cross-platform on a single dev host.impact:cfg(windows)/cfg(macos,linux)runtime paths and the NSIS hook are platform-gated and require real macOS/Linux/Windows(installer) hosts to observe end-to-end.Behavior Changes
Parity Contract
cef_preflight::wait_for_cache_releaseremains the fallback.Duplicate / Superseded PR Handling
Summary by CodeRabbit
New Features
Bug Fixes