feat(launching): detect and surface post-spawn launch failures - #340
feat(launching): detect and surface post-spawn launch failures#340bobtista wants to merge 6 commits into
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Code Review SummaryStatus: 4 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
WARNING
Files Reviewed (11 files)
Note: The previous review SHA was rebased away, so this pass re-reviewed the full PR diff against current HEAD ( Fix these issues in Kilo Cloud Previous Review Summary (commit 163343e)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 163343e)Status: 3 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
Files Reviewed (11 files)
Note: All identified issues have already been reported by Greptile inline comments. No duplicate issues found. The PR makes significant improvements to post-spawn failure detection but has critical threading and error handling issues that need to be addressed. Reviewed by glm-5.2 · Input: 106.7K · Output: 24.5K · Cached: 2M |
…e and sentinel matching
163343e to
f2eba92
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
The base branch was changed.
|
Rebased onto
Instead the six commits unique to this branch were replayed onto Verified after rebase: full core suite 1,553 passed, 0 failed. No approvals existed, so nothing was dismissed by the force-push. |
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| /// PIDs are recycled: an event held indefinitely could be applied to an unrelated | ||
| /// later launch that happened to receive the same PID. | ||
| /// </remarks> | ||
| private static readonly TimeSpan PendingExitRetention = TimeSpan.FromSeconds(30); |
There was a problem hiding this comment.
WARNING: Pending-exit retention is far larger than the race it covers, so a buffered exit can be applied to a later, unrelated launch that recycles the PID.
PendingExitRetention is 30 s, but the gap it buffers (between StartProcessAsync returning and the launcher updating the placeholder entry with the real PID) is described above as milliseconds. Buffered exits are keyed by PID, and this PR explicitly targets Windows, where PIDs recycle. If a later launch is handed a PID that was recycled within the 30 s window, RegisterLaunchAsync drains the stale exit via TryRemove at line 118 and calls ApplyProcessExit, marking the unrelated (possibly still-running) launch as terminated/failed using the dead process exit code and stderr. A second consequence: a terminated launch is never removed from _activeLaunches, so its lingering PID makes OnProcessExited match the recycled-PID event against it (idempotency then drops it) instead of buffering it for the new launch. Tightening this to ~1-2 s still covers the millisecond race with a large margin while shrinking the recycling exposure by an order of magnitude.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
1 participant
Summary
Detect engine initialization failures both during startup and after a launch has been reported as running, preserving stderr evidence so users receive actionable errors.
Changes
[ggc]mount-failure sentinels and name affected archives.Testing
dotnet test GenHub/GenHub.sln -c Release— 1,461 tests passed.Risks and rollback
The Windows system-modal crash dialog remains an engine-side limitation because the process does not exit while the dialog is open. A crash occurring immediately before a requested Stop may be classified as intentional termination; this tradeoff is documented. Reverting this PR restores the previous fixed-window behavior.
Related issues
Fixes #334
Greptile Summary
The PR expands launch-failure detection and propagation:
Confidence Score: 4/5
The PR does not yet appear safe to merge because cancellation can orphan a spawned process, failed termination can suppress a later crash, and late exits update Avalonia-bound state from a worker thread.
A process started before the new cancellable settling wait is neither registered nor terminated when that wait is cancelled; requested-termination markers are not rolled back when killing fails; and the process-exit callback directly mutates observable UI state without dispatching to Avalonia's UI thread.
Files Needing Attention: GenHub/GenHub/Features/GameProfiles/Infrastructure/GameProcessManager.cs; GenHub/GenHub/Features/GameProfiles/ViewModels/GameProfileLauncherViewModel.cs
Important Files Changed
Sequence Diagram
sequenceDiagram participant UI as Launcher ViewModel participant Launcher as GameLauncher participant PM as GameProcessManager participant Registry as LaunchRegistry participant OS as Game Process Launcher->>Registry: Register placeholder launch (PID -1) Launcher->>PM: StartProcessAsync PM->>OS: Spawn process PM->>PM: Capture bounded stderr alt Exit during startup window OS-->>PM: Exit PM-->>Launcher: Failed start with exit details else Process outlives window PM-->>Launcher: Successful start with real PID Launcher->>Registry: Replace placeholder with real PID OS-->>PM: Late exit PM-->>Registry: ProcessExited PM-->>UI: ProcessExited Registry->>Registry: Record termination and failure UI->>UI: Clear running state and surface failure endReviews (3): Last reviewed commit: "fix(launching): classify requested termi..." | Re-trigger Greptile
Context used: