Fix #228: Replace hardcoded port 1108 with dynamic port allocation, s… - #239
Conversation
|
@microsoft-github-policy-service agree |
Renato L. de F. Cunha (renatolfc)
left a comment
Renato L. de F. Cunha (renatolfc)
left a comment
There was a problem hiding this comment.
Thanks for addressing the hard-coded webhook port. We cannot merge this as-is: the ngrok tunnel and agent lifecycle are not cleaned up, pyngrok still uses shared mutable configuration across runs, and choosing a free port before binding it retains a TOCTOU race. Please rebase onto current main, use per-run scoped configuration, bind the port atomically, and guarantee cleanup on success and failure. Note that #199 is a separate COMET API v2.5 compatibility issue.
…cation, scope ngrok cleanup - comet_server.py: Replace HTTP_SERVER_PORT=1108 with find_free_port() that binds to port 0 to get an OS-assigned available port - comet_server.py: Use self.port instance variable for per-server port - comet_requester.py: Remove global ngrok.kill() from finally block; server.shutdown() already calls ngrok.disconnect() for current tunnel - whatif_comet_local.py: Remove global ngrok.kill() from __del__; remove unused pyngrok import Co-authored-by: nik464 <nikhil18chaudhary@gmail.com>
c1bf42e to
4544fa2
Compare
|
Implemented requested changes and rebased onto current main.
Keeping this PR scoped to issue #228. |
|
Renato L. de F. Cunha (@renatolfc) pushed requested fixes to this branch (atomic bind, per-run ngrok config, scoped lifecycle cleanup). Could you please re-review when you have time? |
There was a problem hiding this comment.
Pull request overview
Adds per-request HTTP/ngrok resources to support concurrent Carbon Sequestration runs.
Changes:
- Uses OS-assigned HTTP ports.
- Introduces instance-specific ngrok configuration and cleanup.
- Removes global ngrok cleanup calls.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
comet_server.py |
Adds dynamic ports and scoped ngrok lifecycle. |
comet_requester.py |
Delegates cleanup to the server. |
whatif_comet_local.py |
Removes global cleanup and uses a placeholder webhook. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
- Point PyngrokConfig.config_path at the instance temp dir so set_auth_token never races on the shared user-wide ngrok config - Replace ngrok.disconnect()/ngrok.kill() with a direct kill of this instance's ngrok process, avoiding pyngrok's shared tunnel registry during concurrent shutdowns - Drop the obsolete HTTPServer.server_bind patches that suppressed server_port assignment and broke test_whatif_request* under the atomic port-0 bind, and add a test asserting ephemeral port allocation
Summary
Fixes #228
Problem
The Carbon Sequestration notebook fails when run more
than once simultaneously because:
crashes with "port already in use"
belonging to other running instances
Fix
find_free_port() that binds to port 0 to get an
OS-assigned available port
for per-server port isolation
finally block — server.shutdown() already calls
ngrok.disconnect() for current tunnel only
from del and remove unused pyngrok import
Files Changed
Validation
Co-authors
Co-authored-by: nik464 nikhil18chaudhary@gmail.com