Address PR #10 review feedback#12
Conversation
- Catch OperationCanceledException explicitly in the SignalR reconnection loop so shutdown cancellation logs cleanly instead of appearing as a reconnection failure with noisy backoff warnings. - Remove Process.Dispose() from HealthCheckAsync crash detection to avoid racing with SpawnProcessAsync/MonitorAdoptedProcessAsync that may still hold the Process handle. The owning code path remains responsible for disposal; the 1-hour cleanup catches any stragglers. Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Co-authored-by: GitButler <gitbutler@gitbutler.com>
There was a problem hiding this comment.
Pull request overview
This PR addresses prior review feedback by making agent shutdown/reconnect behavior cleaner and avoiding a potential race when detecting crashed sessions.
Changes:
- Catch
OperationCanceledExceptionduring the SignalR manual reconnection loop to avoid logging shutdown as a reconnection failure. - Stop disposing the
Processhandle insideSessionManager.HealthCheckAsynccrash detection to avoid races with code awaitingWaitForExitAsync().
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/ClaudeNest.Agent/Services/SignalRConnectionManager.cs | Adds explicit cancellation handling in the reconnection loop for cleaner shutdown logging. |
| src/ClaudeNest.Agent/Services/SessionManager.cs | Removes Process.Dispose() (and related behavior) from health-check crash detection to avoid handle races. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Process no longer exists — mark session as crashed and clear process reference. | ||
| // Don't dispose Process here as SpawnProcessAsync/MonitorAdoptedProcessAsync may | ||
| // still be awaiting WaitForExitAsync() on the same instance. Let the owning code | ||
| // path handle disposal; the 1-hour cleanup will catch any stragglers. |
There was a problem hiding this comment.
The new comment says HealthCheckAsync will "clear process reference", but the code no longer clears session.Process (and likely must not, since SpawnProcessAsync/MonitorAdoptedProcessAsync may still use that instance). Please update the comment to accurately describe the behavior (e.g., that we intentionally keep the Process reference and avoid disposing it here, letting the owning path/cleanup handle disposal).
| // Process no longer exists — mark session as crashed and clear process reference. | |
| // Don't dispose Process here as SpawnProcessAsync/MonitorAdoptedProcessAsync may | |
| // still be awaiting WaitForExitAsync() on the same instance. Let the owning code | |
| // path handle disposal; the 1-hour cleanup will catch any stragglers. | |
| // Process no longer exists — mark session as crashed. | |
| // Intentionally keep the existing Process reference and avoid disposing it here, | |
| // as SpawnProcessAsync/MonitorAdoptedProcessAsync may still be awaiting | |
| // WaitForExitAsync() on the same instance. Let the owning code path handle | |
| // disposal; the 1-hour cleanup below will remove the session and dispose Process. |
Summary
OperationCanceledExceptionexplicitly in the SignalR reconnection loop so shutdown logs cleanly instead of appearing as a reconnection failureProcess.Dispose()fromHealthCheckAsynccrash detection to avoid racing withSpawnProcessAsync/MonitorAdoptedProcessAsyncthat may still hold theProcesshandleTest plan
dotnet build-- builds cleandotnet test-- all tests pass🤖 Generated with Claude Code