fix: improve port discovery resilience and cleanup

[nicksenap]

• Continue scanning on any port error, not just EADDRINUSE
• Wait for failed WebSocketServer to close before rejecting

[claude]

PR Review: fix: improve port discovery resilience and cleanup

Summary

This PR makes two focused improvements to the WebSocket server port discovery logic:

1. Continues scanning on any port error (not just EADDRINUSE)
2. Waits for the WebSocketServer to fully close before rejecting in tryPort

✅ Code Quality & Best Practices

Excellent changes overall. The code follows project conventions:

• Proper error handling with typed error codes
• Clear console logging for debugging
• Consistent with the feature-based organization and error patterns in the codebase

Minor suggestion: Consider extracting the error code for better readability:

const code = (err as NodeJS.ErrnoException).code;

This is already done in the PR—great! ✓

✅ Correctness & Bug Fixes

Fix #1: Continue on all port errors

• Before: Only EADDRINUSE was handled, other errors would throw and halt discovery
• After: All errors continue scanning, with appropriate logging
• Impact: More resilient against transient errors (e.g., EACCES, EADDRNOTAVAIL)
• Assessment: ✅ Correct improvement. Port scanning should be robust.

Fix #2: Callback-based close in tryPort

wss.close(() => reject(err));

• Before: Called reject(err) immediately after wss.close()
• After: Waits for close to complete via callback
• Rationale: ws library's close() is asynchronous. Rejecting immediately could leave resources in an inconsistent state.
• Assessment: ✅ Correct. Prevents potential race conditions and ensures cleanup completes.

✅ Performance Considerations

No performance concerns. Changes are minimal and the async close adds negligible overhead while improving reliability.

✅ Security Concerns

No security issues. The changes improve error handling resilience without introducing vulnerabilities.

🧪 Test Coverage Assessment

Current test coverage is strong (packages/mcp-server/src/extension-bridge/websocket-server.test.ts):

• ✅ Port fallback when first port is taken
• ✅ Multiple bridge instances
• ✅ Exhausted port range throwing error

Gap identified: The new behavior (continuing on non-EADDRINUSE errors) is not explicitly tested.

Recommendation: Add a test case like:

it('should continue scanning on non-EADDRINUSE errors', async () => {
  // Simulate EACCES or other error on first port
  // Verify it continues to next port instead of throwing
});

This would be valuable but not blocking—the existing port discovery tests provide good confidence.

📋 Summary

Aspect	Status	Notes
Code Quality	✅	Clean, follows conventions
Bug Fixes	✅	Both changes are correct improvements
Performance	✅	No concerns
Security	✅	No concerns
Test Coverage	⚠️	Strong overall; minor gap for new error handling

Recommendation

Approve with optional enhancement. The changes are solid and improve resilience. Consider adding a test for the new error-handling behavior in a follow-up if desired, but not required for merge.

Great work on improving the multi-instance support! 🎉

[claude]

Code Review: PR #6 - Improve port discovery resilience and cleanup

Summary

This PR improves the robustness of the WebSocket server's port discovery mechanism by handling all port binding errors (not just EADDRINUSE) and ensures proper cleanup when port binding fails.

✅ Strengths

1. Improved Resilience
The change from handling only EADDRINUSE to handling all port errors is a solid improvement. This addresses real-world scenarios where ports may fail for reasons other than being in use (e.g., EACCES, EADDRNOTAVAIL).

2. Better Error Cleanup (websocket-server.ts:65)
The change from wss.close(); reject(err); to wss.close(() => reject(err)); ensures the WebSocket server is fully closed before rejecting the promise. This prevents potential race conditions or resource leaks.

3. Comprehensive Test Coverage
The new test case at websocket-server.test.ts:188-212 properly validates the new behavior with a mock that simulates EACCES errors. Good use of Vitest's spying capabilities.

4. Consistent Logging
Both EADDRINUSE and other errors now get logged, making debugging easier in production.

🔍 Code Quality Observations

Well-Structured Changes

• The logic flow in the start() method is cleaner now with the error code extraction
• The distinction between EADDRINUSE (expected) and other errors is preserved in logging
• Follows ESM and TypeScript conventions per CLAUDE.md

Test Quality

• The test properly isolates the behavior by using a fresh port range (TEST_PORT + 100)
• Spy is restored after use
• Assertions verify both the behavior (port selection) and implementation detail (call count)

💭 Minor Considerations

1. Test Brittleness (websocket-server.test.ts:199)
The cast to as never when spying on the private method tryPort works but is a bit fragile. If the method signature changes, TypeScript won't catch it. However, this is acceptable for testing private methods.

2. Error Code Undefined (websocket-server.ts:83)
The code assumes err.code exists, but it could be undefined for non-Node.js errors. Currently this would log "Port X failed (undefined), trying next...". Consider adding a nullish coalescing operator for better logging. This is a minor enhancement, not a blocker.

3. Continue Statement Behavior (websocket-server.ts:89)
The logic now continues on all errors, which is the desired behavior. The current structure is perfectly readable.

🔒 Security

No security concerns. The changes are defensive and improve error handling.

⚡ Performance

No performance impact. Error handling in the startup path is not performance-critical.

🧪 Test Coverage

Excellent coverage for the new behavior:

• ✅ Non-EADDRINUSE errors trigger port fallback
• ✅ Server successfully starts on next available port after error
• ✅ Spy verification confirms error path is taken

The existing tests for EADDRINUSE (line 151-172) continue to validate the original behavior.

📋 Suggestions (Non-Blocking)

1. Consider adding a test for undefined error code: While unlikely, testing the edge case where err.code is undefined would be thorough.

2. Error propagation: All errors in the port range trigger a generic "No available port" message. This is fine, but consider logging the specific errors that occurred for debugging purposes. Not a requirement, just a nice-to-have.

✨ Verdict

LGTM ✅

This is a clean, well-tested improvement that makes the port discovery more robust. The changes are minimal, focused, and follow the project's conventions. The test coverage properly validates the new behavior.

Recommendation: Merge with confidence.

---

Review generated by Claude Code following CLAUDE.md conventions

[claude]

Pull Request Review: Port Discovery Resilience

Summary

This PR improves the WebSocket server's port discovery by making it more resilient to various error conditions beyond just EADDRINUSE. The changes are well-focused and include appropriate test coverage.

✅ Strengths

1. Good Problem Identification: The change recognizes that port binding can fail for reasons other than the port being in use (e.g., EACCES for permission errors, EADDRNOTAVAIL for invalid addresses).

2. Improved User Experience: The updated error logging differentiates between EADDRINUSE and other errors with appropriate messages.

3. Test Coverage: The new test case should skip port on non-EADDRINUSE errors and continue scanning properly validates the new behavior using a spy to simulate permission errors.

4. Proper Cleanup: The change to wss.close(() => reject(err)) in tryPort (line 65) ensures the WebSocket server properly closes before rejecting, preventing resource leaks.

🔍 Code Quality

websocket-server.ts (lines 82-90)

Good practices:

• Extracting the error code to a variable (const code = ...) improves readability
• The continue statement now applies to all errors, which is the intended behavior
• Logging distinguishes between error types

Minor consideration:
The error code might be undefined for non-Node.js errors. Consider defensive handling:

```typescript
const code = (err as NodeJS.ErrnoException).code;
if (code === 'EADDRINUSE') {
console.error(`[Paparazzi] Port ${port} is already in use, trying next...`);
} else if (code) {
console.error(`[Paparazzi] Port ${port} failed (${code}), trying next...`);
} else {
console.error(`[Paparazzi] Port ${port} failed, trying next...`);
}
```

However, this is very minor since WebSocket server errors typically include error codes.

websocket-server.test.ts (lines 188-211)

Test design:

• ✅ Proper isolation with a fresh port range (TEST_PORT + 100)
• ✅ Correct use of vi.spyOn() to mock the private tryPort method
• ✅ Proper cleanup with mockRestore()
• ✅ Validates both the outcome (port selection) and the behavior (spy call count)

Type safety:
The // eslint-disable-next-line @typescript-eslint/no-explicit-any comment is necessary for spying on private methods. This is acceptable for testing purposes.

🔒 Security Considerations

No security concerns identified. The changes:

• Don't introduce new attack surfaces
• Maintain the same port exhaustion protection
• Don't expose sensitive error details to clients (only to stderr logs)

⚡ Performance Considerations

Performance impact is negligible:

• The error handling path is unchanged in complexity (still O(1) per port attempt)
• The change only affects the error handling branch, which is already exceptional
• Proper cleanup with wss.close(() => reject(err)) prevents resource accumulation

🧪 Test Coverage

Coverage is good:

• ✅ New behavior is tested (non-EADDRINUSE error handling)
• ✅ Existing test cases remain unchanged and passing
• ✅ Edge cases covered (port exhaustion still works as expected)

Could be enhanced (optional):
Consider testing with different error codes (e.g., EADDRNOTAVAIL, EACCES) explicitly, though the current spy-based test is sufficient.

📋 Suggestions

1. Consider logging the full error for debugging

In the catch block, you might want to log more details for unexpected errors:

```typescript
} else {
console.error(`[Paparazzi] Port ${port} failed (${code}): ${(err as Error).message}`);
}
```

2. Documentation update

Consider adding a comment in the code explaining why all errors result in continuing to the next port:

```typescript
// Try all ports in range, skipping any that fail for any reason
// (EADDRINUSE = in use, EACCES = no permission, etc.)
```

✅ Verdict

This PR is ready to merge. The changes are:

• Focused and well-scoped
• Properly tested
• Improve resilience without introducing risks
• Follow the project's conventions (based on CLAUDE.md)

The suggestions above are optional enhancements and should not block merging.

---

Alignment with CLAUDE.md conventions:

• ✅ ESM-only patterns maintained
• ✅ Feature-based organization preserved
• ✅ Tests colocated with source
• ✅ camelCase for functions, PascalCase for types
• ✅ Proper error handling patterns