fix(server): retry egress sidecar with new ports on Docker Desktop/Windows port conflicts - #1706
Conversation
…ndows port conflicts When docker start fails with "ports are not available" or "bind: forbidden" (e.g. on Docker Desktop for Windows where OS-reserved port ranges are invisible from the container-side probe), retry once with freshly-allocated ports instead of failing immediately with a generic 500. Also surfaces the original Docker error in the failure message when the retry also fails. A shallow-copy bug in build_sidecar_host_config meant the retry used stale port_bindings from base_sidecar_host_config_kwargs; the fix rebuilds that dict before the retry create call. Closes opensandbox-group#1702
github-actions
Bot
added
component/server
size/L
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a9bb4d510b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| retry_api_host_port = next( | ||
| (b[1] for b in sidecar_port_bindings.values()), | ||
| egress_api_host_port, | ||
| ) |
There was a problem hiding this comment.
When the retry path is reached from create_sandbox, sidecar_port_bindings is ordered as 44772, 8080, then 18080, so this next() always selects the new execd host port rather than the egress API port. The readiness probe then requests /healthz from execd instead of the egress server on container port 18080, causing every otherwise-successful port-conflict retry to wait until the readiness timeout and fail sandbox creation. Select sidecar_port_bindings["18080"][1] instead.
Useful? React with 👍 / 👎.
| retry_bindings = port_allocator( | ||
| list(sidecar_port_bindings.keys()), | ||
| min_port=self.app_config.docker.port_range_min, | ||
| max_port=self.app_config.docker.port_range_max, | ||
| ) | ||
| sidecar_port_bindings = retry_bindings |
There was a problem hiding this comment.
A successful retry only replaces this method's local sidecar_port_bindings; the caller still writes its original host_execd_port and host_http_port into the main container labels in docker_service.py:840-841. Endpoint resolution reads those labels, so after a successful retry clients are directed to the rejected old ports rather than the sidecar's newly published execd and HTTP ports. Propagate the refreshed bindings (or refreshed two host ports) back to the caller before it creates the main container.
Useful? React with 👍 / 👎.
| if ( | ||
| last_port_error is not None | ||
| and port_allocator is not None | ||
| and not _is_port_publish_error(last_port_error) | ||
| ): | ||
| raise |
There was a problem hiding this comment.
The production caller now always supplies port_allocator, so any sidecar start() failure that is not recognized as a port-publish error takes this bare raise. That exits the outer handler before its cleanup at lines 662-680; the caller has not received a sidecar_container return value either, so it cannot remove the already-created sidecar. This regresses ordinary Docker start failures into orphaned sidecars and uncaught DockerExceptions instead of the prior cleanup and normalized HTTP error.
Useful? React with 👍 / 👎.
| if _is_port_publish_error(retry_exc): | ||
| raise HTTPException( | ||
| status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, | ||
| detail={ | ||
| "code": SandboxErrorCodes.CONTAINER_START_FAILED, | ||
| "message": ( | ||
| f"Egress sidecar container failed to start: " | ||
| f"{retry_exc}" | ||
| ), | ||
| }, | ||
| ) from retry_exc |
There was a problem hiding this comment.
If the second start() also gets a port-publish error, this immediately raises an HTTPException from inside the outer exception handler, bypassing the normal cleanup below. The first failed sidecar was removed before retrying, but the newly created second container remains orphaned because create_sandbox never receives a returned sidecar to clean up. Route this failure through cleanup before surfacing the HTTP error.
Useful? React with 👍 / 👎.
1 participant
Summary
When creating sandboxes with an egress sidecar on Docker Desktop for Windows (or other environments with OS-reserved excluded port ranges),
docker startcan fail with `"ports are not available: ... bind: ... forbidden"` even though the container-side port probe succeeded. The server currently raises a generic HTTP 500 without retrying — this PR adds a one-shot retry with freshly-allocated ports.Root Cause
allocate_host_port()inport_allocator.pyprobes availability withsocket.bind()inside the server container (Linux network stack). On Docker Desktop/Windows the actual bind happens on the Windows host, where OS-reserved excluded port ranges (e.g. Hyper-V/WinNAT) make the bind fail withWSAEACCES— the container-side probe cannot see this._start_egress_sidecar()innetworking.pyonly retried on the IPv6-sysctl rejection; any otherstart()failure (including"ports are not available") cleaned up and raised HTTP 500 without re-allocating a new host port.Fix
_is_port_publish_error()helper that detects"ports are not available"and"bind:" + "forbidden"patterns in Docker errors.start()fails with a port-publish error and aport_allocatoris wired in, re-allocate fresh ports, clean up the failed container, and retry create+start once.build_sidecar_host_config()doesdict(base_sidecar_host_config_kwargs), which preserves the oldport_bindingsdict whensidecar_port_bindingsis reassigned. The fix rebuildsbase_sidecar_host_config_kwargsfrom scratch before the retry.Testing
test_egress_sidecar_retries_on_port_publish_error: verifies that on a port-publish failure the sidecar is recreated with different ports and starts successfully.test_egress_sidecar_raises_on_second_port_failure: verifies that when both attempts fail, an HTTPException is raised with the real Docker error.test_docker_service.pypass.ruff checkpasses on all changed files.Files Changed
server/opensandbox_server/services/docker/networking.py— retry logic +_is_port_publish_errorserver/opensandbox_server/services/docker/docker_service.py— wireport_allocatorat the call siteserver/tests/test_docker_service.py— two regression testsCloses #1702