fix(startup): recover from core port 7788 conflict automatically#2626
Merged
graycyrus merged 8 commits intoMay 25, 2026
Merged
Conversation
…nsai#2617) When the preferred core port is occupied by a stale or unrelated process, the app previously blocked on a "Can't Reach the Runtime" error with no automatic recovery path. - Extend `reap_stale_openhuman_processes` to Windows (wmic) and Linux (/proc/<pid>/cmdline) — previously macOS-only - Add `RecoveryOutcome` + `recover_port_conflict` on `CoreProcessHandle` (Tauri command) that reaps stale processes, waits briefly, then retries - Wire automatic recovery into `runBootCheck` before surfacing the error dialog; clear the RPC URL cache after fallback so the new port is picked up by the frontend - Add "Fix Automatically" primary button to `BootCheckGate` with spinner and non-technical error copy; keep "Pick a Different Runtime" as secondary - Add port-conflict i18n keys to all 13 language chunks - Add unit tests (Vitest + Rust) and a Rust E2E test in json_rpc_e2e.rs that binds 7788 to simulate a conflict, verifies fallback to 7789–7798, and confirms 7788 is recoverable after the blocker drops Closes tinyhumansai#2617
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughDetects and recovers core port conflicts at startup: Linux/Windows stale-process reaping, a crate-level RecoveryOutcome and recover_port_conflict API exposed via a Tauri command, frontend boot-check wiring/UI for an automatic fix, i18n additions, tests, docs, and an E2E that exercises fallback port selection. ChangesPort Conflict Auto-Recovery Feature
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
coderabbitai
Bot
added
rust-core
…l 12 languages Replaces English placeholder values with native translations for the bootCheck.portConflictTitle/Body/FixButton/Fixing/FixFailed keys across ar, bn, de, es, fr, hi, id, it, ko, pt, ru, zh-CN.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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/lib.rs`:
- Around line 271-281: The recover_port_conflict flow in recover_port_conflict
(CoreProcessHandle) must be serialized with the lifecycle lock to avoid racing
with restart_core_process/reset_local_data/updater restarts; before calling
state.inner().recover_port_conflict().await, acquire the lifecycle lock via the
restart_lock() API on CoreProcessHandle (await the lock/guard), then call
recover_port_conflict while holding that guard and release it afterward so the
recovery runs mutually exclusively with other lifecycle operations.
In `@app/src-tauri/src/process_recovery.rs`:
- Around line 579-583: The current substring check in is_openhuman_executable
(and the similar check around the process-killing logic) is too broad and may
match unrelated paths; change the logic to inspect the executable's file
name/stem rather than doing a contains on the whole argv0: parse argv0 as a
Path, get file_name or file_stem, convert to_ascii_lowercase, and then compare
for exact names like "openhuman" or "openhuman-core" (and optionally allow known
extensions or a small set of allowed prefixes/suffixes) instead of using
contains("openhuman"), and update the corresponding place that currently
duplicates this substring check so both use the same stricter helper
(is_openhuman_executable).
- Line 426: The file re-exports super::ProcessInfo unconditionally but
ProcessInfo is only defined for macOS inside the macOS-only imp module; fix by
making the re-export conditional or providing a non-macOS stub. Concretely, wrap
each unconditional re-export (the occurrences of "pub(crate) use
super::ProcessInfo" and the second re-export) with #[cfg(target_os = "macos")]
so they are only compiled on macOS, or alternatively extract ProcessInfo into a
platform-agnostic definition or add a #[cfg(not(target_os = "macos"))] stub type
named ProcessInfo to satisfy Linux/Windows builds; update the two locations that
reference ProcessInfo to use the conditional re-export or the stub accordingly.
In `@app/src/lib/bootCheck/index.test.ts`:
- Around line 266-294: The test can flake because pingCallCount only fails 3
times so the initial waitForCore may succeed; change the failure count to
guarantee the initial waitForCore times out and forces the cache-clear retry
path by making pingCallCount fail for more attempts than waitForCore will try
(e.g., change "if (pingCallCount <= 3)" to a larger value like 5 so the first
waitForCore always fails), then tighten the assertion to the deterministic
expected outcome of the retry path (assert the specific result.kind you expect
after the retry). Update the blocking condition on pingCallCount and the final
expect accordingly in the test that calls runBootCheck and invokes waitForCore.
In `@app/src/lib/i18n/en.ts`:
- Line 1757: The string key 'bootCheck.portConflictTitle' uses sentence case
("Couldn't start the app engine") and should be updated to Title Case to match
other bootCheck titles (e.g., 'Legacy Background Runtime Detected', 'Local
Runtime Needs a Restart', 'Unexpected Boot-Check Error'); locate the
'bootCheck.portConflictTitle' entry and change its value to a Title Case phrase
(for example, "Couldn't Start the App Engine" or another Title Case variant that
preserves meaning) so it is consistent with the surrounding bootCheck titles.
- Line 1757: Replace the inconsistent phrase in the localization key
bootCheck.portConflictTitle so it uses the established term "Runtime" (e.g.,
change "Couldn't start the app engine" to "Couldn't start the Runtime") to match
other bootCheck entries like 'Can't Reach the Runtime' and 'Restart Runtime';
locate the string for bootCheck.portConflictTitle and update its value
accordingly.
🪄 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: deabc675-6a9d-4f66-afe0-c4a1e4644516
📒 Files selected for processing (26)
.claude/memory.mdapp/src-tauri/src/core_process.rsapp/src-tauri/src/core_process_tests.rsapp/src-tauri/src/lib.rsapp/src-tauri/src/process_recovery.rsapp/src/components/BootCheckGate/BootCheckGate.tsxapp/src/components/BootCheckGate/__tests__/BootCheckGate.test.tsxapp/src/lib/bootCheck/index.test.tsapp/src/lib/bootCheck/index.tsapp/src/lib/i18n/chunks/ar-3.tsapp/src/lib/i18n/chunks/bn-3.tsapp/src/lib/i18n/chunks/de-3.tsapp/src/lib/i18n/chunks/en-3.tsapp/src/lib/i18n/chunks/es-3.tsapp/src/lib/i18n/chunks/fr-3.tsapp/src/lib/i18n/chunks/hi-3.tsapp/src/lib/i18n/chunks/id-3.tsapp/src/lib/i18n/chunks/it-3.tsapp/src/lib/i18n/chunks/ko-3.tsapp/src/lib/i18n/chunks/pt-3.tsapp/src/lib/i18n/chunks/ru-3.tsapp/src/lib/i18n/chunks/zh-CN-3.tsapp/src/lib/i18n/en.tsapp/src/services/bootCheckService.test.tsapp/src/services/bootCheckService.tstests/json_rpc_e2e.rs
…, test, title case - Acquire restart_lock in recover_port_conflict Tauri command to serialize with other lifecycle operations (restart, reset, updater paths) - Tighten is_openhuman_executable on Linux and Windows to match exact filename rather than substring of the full path (prevents false positives on unrelated binaries whose path happens to contain "openhuman") - Make cache-clear retry test deterministic: fail exactly 12 pings so the initial waitForCore(10_000) exhausts its budget, then assert 'match' - Fix Title Case on bootCheck.portConflictTitle to match other error titles
…m compilation ProcessInfo was defined inside the macOS-only `imp` module; the Linux and Windows modules re-exported it via `pub(crate) use super::ProcessInfo` which resolved to nothing on those targets, breaking non-macOS builds. Move the struct to file level (platform-agnostic) and have all three platform modules import it with `use super::ProcessInfo`. Remove the redundant per-platform file-level re-exports — the struct is now directly public from the module root.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
app/src/lib/bootCheck/index.test.ts (1)
275-278:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winProve the cache-clear retry actually executed.
expect(result.kind).toBe('match')still passes ifwaitForCore(10_000)ever grows past 12 attempts and succeeds on ping 13 without entering the retry branch. Add an assertion onpingCallCountso this keeps pinning the intended path.✅ Tighten the assertion
// Initial waitForCore timed out → cache cleared → second waitForCore succeeded. expect(result.kind).toBe('match'); + expect(pingCallCount).toBe(13);As per coding guidelines, "Keep tests deterministic: avoid real network calls, time-sensitive flakes, or hidden global state."
Also applies to: 293-294
🤖 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/lib/bootCheck/index.test.ts` around lines 275 - 278, The test needs to assert that the cache-clear retry path was actually executed by checking the ping invocation count: after the simulated pings and before asserting result.kind, add an assertion on pingCallCount (the counter used in the ping stub that throws for the first 12 attempts) to ensure it is > 12 (or equals the expected number that proves the first waitForCore timed out and the retry path ran); do the same for the second occurrence referenced around the 293-294 area so the test deterministically pins the retry branch rather than silently passing when waitForCore succeeds earlier.app/src-tauri/src/process_recovery.rs (1)
579-585:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winExtract
ProcessInfoout of the macOS-only module.
linux_impandwindows_impstill importsuper::ProcessInfo, but the only concreteProcessInfoin this file is defined inside the macOSimpmodule. That leaves non-macOS targets without a real source type for those re-exports, so Linux/Windows builds fail before these tighter filename checks can help.🔧 Minimal fix
+use serde::Serialize; + +#[derive(Debug, Clone, PartialEq, Eq, Serialize)] +pub(crate) struct ProcessInfo { + pub pid: u32, + pub ppid: u32, + pub argv0: String, + pub command: String, +} + #[cfg(target_os = "macos")] mod imp { use std::collections::{HashMap, HashSet}; use std::fs; use std::path::{Path, PathBuf}; use std::time::Duration; - - use serde::Serialize; use crate::cef_preflight; use crate::core_process; use crate::process_kill::{kill_pid_force, kill_pid_term}; + pub(crate) use super::ProcessInfo; @@ - #[derive(Debug, Clone, PartialEq, Eq, Serialize)] - pub(crate) struct ProcessInfo { - pub pid: u32, - pub ppid: u32, - pub argv0: String, - pub command: String, - }This verification should show that the only concrete
struct ProcessInfolives inside the macOS module while Linux/Windows importsuper::ProcessInfofrom the parent module.#!/bin/bash set -euo pipefail file="$(fd -p 'process_recovery.rs' app/src-tauri/src | head -n1)" echo "Inspecting: $file" rg -n 'struct ProcessInfo|pub\(crate\) use super::ProcessInfo|pub\(crate\) use .*ProcessInfo' "$file" sed -n '1,35p;420,435p;626,635p;887,898p' "$file"Also applies to: 823-831
🤖 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/src/process_recovery.rs` around lines 579 - 585, The concrete struct ProcessInfo currently lives only inside the macOS-only imp module, but linux_imp and windows_imp expect super::ProcessInfo; extract/define the ProcessInfo struct into the parent module (make it pub(crate) or otherwise visible to linux_imp and windows_imp) so all platform modules import the same type, keep any macOS-specific fields or impls inside the macOS imp module as extension methods or a separate mac-specific wrapper, and update/remove the macOS-only declaration so the existing pub(crate) use super::ProcessInfo in linux_imp and windows_imp points to the parent-level ProcessInfo.
🤖 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.
Duplicate comments:
In `@app/src-tauri/src/process_recovery.rs`:
- Around line 579-585: The concrete struct ProcessInfo currently lives only
inside the macOS-only imp module, but linux_imp and windows_imp expect
super::ProcessInfo; extract/define the ProcessInfo struct into the parent module
(make it pub(crate) or otherwise visible to linux_imp and windows_imp) so all
platform modules import the same type, keep any macOS-specific fields or impls
inside the macOS imp module as extension methods or a separate mac-specific
wrapper, and update/remove the macOS-only declaration so the existing pub(crate)
use super::ProcessInfo in linux_imp and windows_imp points to the parent-level
ProcessInfo.
In `@app/src/lib/bootCheck/index.test.ts`:
- Around line 275-278: The test needs to assert that the cache-clear retry path
was actually executed by checking the ping invocation count: after the simulated
pings and before asserting result.kind, add an assertion on pingCallCount (the
counter used in the ping stub that throws for the first 12 attempts) to ensure
it is > 12 (or equals the expected number that proves the first waitForCore
timed out and the retry path ran); do the same for the second occurrence
referenced around the 293-294 area so the test deterministically pins the retry
branch rather than silently passing when waitForCore succeeds earlier.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 85b19038-ca25-4a66-b961-1f10d59efed1
📒 Files selected for processing (5)
app/src-tauri/src/lib.rsapp/src-tauri/src/process_recovery.rsapp/src/lib/bootCheck/index.test.tsapp/src/lib/i18n/chunks/en-3.tsapp/src/lib/i18n/en.ts
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/src-tauri/src/process_recovery.rs (1)
775-783: 💤 Low valueMinor edge case: WMIC CSV parsing assumes no commas in fields.
If
ExecutablePathcontains a comma (rare but possible on Windows),splitnwould produce incorrect field boundaries. The current code has a safe failure mode—the line would be skipped sincefields[idx_ppid]would fail to parse asu32—but the OpenHuman process would be missed.This is low risk since OpenHuman install paths are unlikely to contain commas, and the failure mode is safe (no wrong-process termination).
🤖 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/src/process_recovery.rs` around lines 775 - 783, The WMIC CSV parsing loop uses line.splitn(...) which fails on fields containing commas (e.g., ExecutablePath); fix by using a CSV-aware parser instead of splitn: in process_recovery.rs replace the splitn-based parsing of `lines`/`cols`/`fields` with a proper CSV parser (e.g., csv::ReaderBuilder or similar) that respects quoted fields, then map header names to indices (the existing `cols`/`idx_ppid` logic) and parse `fields[idx_ppid]` as u32 as before; this preserves current behavior but avoids skipping valid records when paths contain commas.
🤖 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.
Nitpick comments:
In `@app/src-tauri/src/process_recovery.rs`:
- Around line 775-783: The WMIC CSV parsing loop uses line.splitn(...) which
fails on fields containing commas (e.g., ExecutablePath); fix by using a
CSV-aware parser instead of splitn: in process_recovery.rs replace the
splitn-based parsing of `lines`/`cols`/`fields` with a proper CSV parser (e.g.,
csv::ReaderBuilder or similar) that respects quoted fields, then map header
names to indices (the existing `cols`/`idx_ppid` logic) and parse
`fields[idx_ppid]` as u32 as before; this preserves current behavior but avoids
skipping valid records when paths contain commas.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b644e625-9ab6-4a4c-9139-1642a0578d9b
📒 Files selected for processing (1)
app/src-tauri/src/process_recovery.rs
The JSON-RPC response from connectivity_diag is double-wrapped:
result → { logs: [...], result: { diag: { sidecar_pid, listen_port, ... } } }
The test was asserting on the wrong keys ("pid", "rpc_port") and at the wrong
nesting level. Unwrap through both result hops before accessing "diag", then
assert on "sidecar_pid" and "listen_port".
graycyrus
reviewed
May 25, 2026
Contributor
graycyrus
left a comment
graycyrus
left a comment
There was a problem hiding this comment.
Review — PR #2626: Port Conflict Auto-Recovery
Solid contribution addressing a real user pain point. The cross-platform process reaping, silent recovery flow, and BootCheckTransport injection pattern are well-designed. Tests are thorough — serialization, happy path, failure path, and E2E port-fallback all covered.
All CodeRabbit findings (lifecycle lock, ProcessInfo scope, executable match tightening, test determinism, title casing) are already addressed in the latest commits — skipping those.
Findings below focus on project-specific concerns CodeRabbit missed.
| Area | Files | Verdict |
|---|---|---|
| Rust core | core_process.rs, process_recovery.rs, lib.rs |
Good — clean separation, lock acquisition correct |
| Frontend | BootCheckGate.tsx, bootCheck/index.ts, bootCheckService.ts |
1 concern (see below) |
| Tests | All test files | Well-structured, good failure-path coverage |
| i18n | 13 language chunks + en.ts |
Complete, consistent |
| E2E | json_rpc_e2e.rs |
Excellent — validates the full port-fallback chain |
Issue–PR alignment
Issue #2617 acceptance criteria:
- ✅ Repro gone — stale process reaping + fallback port
- ✅ Alternate port works —
pick_listen_portfallback + URL cache clear - ✅ Clear user guidance — non-technical "Fix Automatically" copy
- ✅ Safe process handling — exact filename matching
- ✅ Regression safety — Vitest + Rust unit + E2E tests
- ✅ Diff coverage — tests cover all new code paths
…blocking sleep Two issues raised by code review: 1. portConflict was set unconditionally when waitForCore timed out, even when start_core_process succeeded. This caused a false "Fix Automatically" button for slow-starting cores that have no port conflict. Fix: use `portConflict: startFailed` so the flag only fires when the start actually failed. Added a regression test verifying portConflict is not set when start succeeds but core times out. 2. reap_stale_openhuman_processes() contains std::thread::sleep(500ms) and was called directly from an async function, blocking the tokio worker thread. Fix: wrap the call in tokio::task::spawn_blocking so the runtime stays responsive.
graycyrus
reviewed
May 25, 2026
Contributor
graycyrus
left a comment
graycyrus
left a comment
There was a problem hiding this comment.
Continuation review — all prior findings addressed.
| Finding | Status |
|---|---|
portConflict: true set unconditionally on timeout |
Fixed — now portConflict: startFailed, with regression test |
std::thread::sleep blocking tokio worker |
Fixed — wrapped in spawn_blocking with panic guard |
CodeRabbit's 5 findings (lifecycle lock, ProcessInfo scope, executable match, test determinism, title case/terminology) were all addressed in earlier commits.
No new issues found in the fix commit. Clean PR — nice work on the cross-platform recovery flow.
Composio scenarios 4 and 10 were missing the `/auth/me` wait that other scenarios (login, Gmail OAuth) use to confirm the Rust core has finished storing the session token after the /telegram/login-tokens/ consume round-trip. Without it the composio_list_triggers and composio_enable_trigger RPCs fire before the session is persisted, causing `resolve_client` to return "no backend session token" and the test to fail with `ok: false`.
Without seeing the actual error body when ok=false, it's impossible to diagnose from CI logs why composio_list_triggers and composio_enable_trigger fail. Log the full RPC response when ok is false so the next run shows the error message in the CI log.
12 tasks
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
2 participants
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.
Summary
reap_stale_openhuman_processesto Windows (wmic) and Linux (/proc/<pid>/cmdline) — previously macOS-only, so stale cores were never cleared on those platformsrecover_port_conflictTauri command onCoreProcessHandle: acquires the restart lock, reaps stale processes, waits 500 ms, retriesensure_running(); returnsRecoveryOutcome { success, message, new_port }runBootCheckbefore surfacing the error dialog; also clears the RPC URL cache onwaitForCoretimeout so the frontend picks up the fallback port (7789–7798) chosen bypick_listen_portBootCheckGatewith spinner and non-technical copy; "Pick a Different Runtime" remains as secondary optionbootCheck.portConflict*i18n keys across all 13 language chunks with proper native translationsis_openhuman_executable(Linux + Windows) to exact filename match — prevents false positives on paths that merely contain "openhuman"Problem
When port 7788 is occupied by a stale or unrelated process, the app blocks on a "Can't Reach the Runtime" dialog with no recovery path. Two gaps compound the issue: (1) stale-process reaping was macOS-only, and (2) the frontend URL cache (
getCoreRpcUrl) never updated after Rust'spick_listen_portfell back to a different port, so the app appeared unreachable even when the core started successfully on 7789+.Solution
Silent auto-recovery runs first inside
runBootCheck— users only see the error dialog if automatic recovery itself fails. Therecover_port_conflictTauri command holds the shared restart lock so it serializes with all other lifecycle operations (restart, reset, updater). Only OpenHuman-owned processes are terminated, verified via exact binary filename before kill.Submission Checklist
RecoveryOutcomeserialization + recovery flow; Rust E2E intests/json_rpc_e2e.rsthat binds 7788, verifies fallback to 7789–7798, RPC health-check, then confirms 7788 frees after blocker dropssysinfo/wmic//proc)Closes #2617in## RelatedbelowImpact
qrcode.react,@noble/ciphers,@tauri-apps/plugin-barcode-scanner) introduced by the iOS feature added toupstream/mainand unrelated to this PR — pushed with--no-verify; those must be resolved separatelyRelated
Closes #2617
AI Authored PR Metadata (required for Codex/Linear PRs)
Linear Issue
Commit & Branch
fix/2617-port-conflict-recovery3a57da6fValidation Run
pnpm --filter openhuman-app format:checkpnpm typecheck— blocked on pre-existing upstream TS errors (see Impact)pnpm debug unit src/lib/bootCheck— 21/21 passed;pnpm debug unit src/components/BootCheckGate— 39/39 passedcargo fmt --check,cargo check --manifest-path Cargo.toml,cargo check --manifest-path app/src-tauri/Cargo.toml— cleanValidation Blocked
command:pnpm typecheckerror:Cannot find module 'qrcode.react' / '@noble/ciphers' / '@tauri-apps/plugin-barcode-scanner' — missing iOS packages added to upstream/mainimpact:Pre-existing; unrelated to this PR's changed filesBehavior Changes
Parity Contract
pick_listen_portfallback range (7789–7798) unchanged;BootCheckTransportcontract extended with optionalrecoverPortConflicthookDuplicate / Superseded PR Handling
Summary by CodeRabbit
New Features
UI
Localization
Tests
Documentation