Skip to content

Address PR #10 review feedback#12

Merged
GordonBeeming merged 1 commit intomainfrom
gb/fix-agent-memory-leak-review-feedback
Mar 26, 2026
Merged

Address PR #10 review feedback#12
GordonBeeming merged 1 commit intomainfrom
gb/fix-agent-memory-leak-review-feedback

Conversation

@GordonBeeming
Copy link
Copy Markdown
Owner

Summary

  • Catch OperationCanceledException explicitly in the SignalR reconnection loop so shutdown logs cleanly instead of appearing as a reconnection failure
  • Remove Process.Dispose() from HealthCheckAsync crash detection to avoid racing with SpawnProcessAsync/MonitorAdoptedProcessAsync that may still hold the Process handle

Test plan

  • dotnet build -- builds clean
  • dotnet test -- all tests pass
  • Verify agent shuts down cleanly without noisy reconnection warnings in logs

🤖 Generated with Claude Code

- 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>
@GordonBeeming GordonBeeming marked this pull request as ready for review March 26, 2026 06:33
@GordonBeeming GordonBeeming merged commit 8f6be4c into main Mar 26, 2026
2 checks passed
@GordonBeeming GordonBeeming deleted the gb/fix-agent-memory-leak-review-feedback branch March 26, 2026 06:33
Copilot AI review requested due to automatic review settings March 26, 2026 06:33
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 OperationCanceledException during the SignalR manual reconnection loop to avoid logging shutdown as a reconnection failure.
  • Stop disposing the Process handle inside SessionManager.HealthCheckAsync crash detection to avoid races with code awaiting WaitForExitAsync().

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.

Comment on lines +203 to +206
// 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.
Copy link

Copilot AI Mar 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Suggested change
// 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.

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants