Skip to content

Conversation

@jhrozek
Copy link
Contributor

@jhrozek jhrozek commented Nov 5, 2025

Resolves shutdown hang and "use of closed network connection" errors by: removing redundant listener.Close() call since httpServer.Shutdown() already handles listener cleanup internally.

Previously you would have seen:

5:21PM  ERROR   Errors during shutdown: [failed to close listener: close
tcp 127.0.0.1:4483: use of closed network connection]
Error: failed to close listener: close tcp 127.0.0.1:4483: use of closed
network connection
5:21PM  ERROR   Error executing command: failed to close listener: close
tcp 127.0.0.1:4483: use of closed network connection

Resolves shutdown hang and "use of closed network connection" errors by:
removing redundant listener.Close() call since httpServer.Shutdown()
already handles listener cleanup internally.

Previously you would have seen:
```
5:21PM  ERROR   Errors during shutdown: [failed to close listener: close
tcp 127.0.0.1:4483: use of closed network connection]
Error: failed to close listener: close tcp 127.0.0.1:4483: use of closed
network connection
5:21PM  ERROR   Error executing command: failed to close listener: close
tcp 127.0.0.1:4483: use of closed network connection
```
@codecov
Copy link

codecov bot commented Nov 5, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 55.40%. Comparing base (e667810) to head (f4f3212).
⚠️ Report is 4 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2472      +/-   ##
==========================================
- Coverage   55.46%   55.40%   -0.06%     
==========================================
  Files         284      284              
  Lines       26720    26716       -4     
==========================================
- Hits        14820    14803      -17     
- Misses      10537    10553      +16     
+ Partials     1363     1360       -3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@JAORMX
Copy link
Collaborator

JAORMX commented Nov 6, 2025

@claude please review this

@claude
Copy link
Contributor

claude bot commented Nov 6, 2025

Claude finished @JAORMX's task —— View job


Code Review

Tasks

  • Read the changed file to understand the fix
  • Analyze the code changes
  • Provide comprehensive review feedback

Review Summary

Verdict: Approved

This is an excellent fix that correctly resolves the "use of closed network connection" error during graceful shutdown.

Analysis

The Problem:
The original code attempted to manually close the TCP listener after calling httpServer.Shutdown(), but Go's http.Server.Shutdown() already closes the listener internally as part of its graceful shutdown process. This resulted in a double-close error: "use of closed network connection".

The Solution:
The fix removes the redundant listener.Close() call and only clears the listener reference, allowing httpServer.Shutdown() to handle the listener cleanup correctly.

Technical Review

What Changed (pkg/vmcp/server/server.go:306-333):

  1. Comment clarification (line 312): Updated comment to explicitly state that httpServer.Shutdown() handles listener closure internally
  2. Removed redundant close logic (lines 323-328): Eliminated the manual listener.Close() call that was causing the error
  3. Simplified cleanup (lines 323-326): Now only clears the listener reference under mutex lock

Why This is Correct:

According to Go's net/http documentation, Server.Shutdown():

closes all active net.Listeners and any connections in state StateNew, StateActive, or StateIdle

This means the listener is already closed when Shutdown() completes successfully. The subsequent listener.Close() was attempting to close an already-closed listener, which returns the error we were seeing.

Code Quality:

  • Proper mutex usage maintained for thread safety
  • Comments accurately describe the behavior
  • Error handling remains comprehensive
  • Cleaner, more maintainable code with less redundancy

Additional Observations

Strengths:

  1. The fix targets the root cause rather than masking the symptom
  2. Updated comments provide clear documentation for future maintainers
  3. The change aligns with Go standard library best practices
  4. Test coverage remains at 100% for modified lines

No issues found:

  • No race conditions introduced
  • Shutdown ordering is correct (HTTP server → listener cleanup → session manager)
  • Error aggregation logic is still sound
  • Thread safety preserved with proper mutex usage

Recommendation

This PR should be merged. It's a clean, well-documented fix that eliminates an error condition during graceful shutdown without introducing any new risks.


| Branch: vmcp-server-shutdown-error

@JAORMX JAORMX merged commit b68f85d into main Nov 6, 2025
29 checks passed
@JAORMX JAORMX deleted the vmcp-server-shutdown-error branch November 6, 2025 05:59
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.

3 participants