Fix: DockerDeployment.start() hangs on startup timeout instead of reporting it - #301
Open
lavneethora wants to merge 2 commits into
Open
lavneethora wants to merge 2 commits into
lavneethora wants to merge 2 commits into
Conversation
DockerDeployment captured the container process on pipes that nothing ever drained. Reading a pipe blocks until the writer exits, so the startup-timeout handler blocked forever on a still-running container, taking the event loop and every concurrent deployment down with it and leaking the container because cleanup never ran. A full 64kiB pipe also silently discarded the rest of the output, which is exactly the output the handler wanted to report. Capture to temporary files instead, so reads never block and nothing is lost. Guard the cleanup in the handler so a failing stop() cannot replace the TimeoutError the caller needs to see. Also run the blocking container runtime calls in start() and stop() via asyncio.to_thread. They stalled the event loop for every deployment started alongside them, while the startup deadline is wall clock.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #261.
What happens
DockerDeploymentlaunches the container withPopen(cmds, stdout=PIPE, stderr=PIPE)and never drains either pipe. When
start()hits itsstartup_timeout, the handler does:.read()on a pipe blocks until the writer exits. The container is still running at thatpoint, so this never returns.
start()does not raise the timeout, it hangs on it, andbecause the read is synchronous it hangs the event loop, so every deployment started
alongside it in
asyncio.gatherfreezes too. Theawait self.stop()on the next linenever runs, so the container is leaked as well.
That is why the reporter only sees this in parallel: one deployment reaching its timeout
is enough to wedge the whole program.
Two further problems on the same path:
docker runblocks writing and everything afterthat is lost, so the output the handler exists to report is truncated. (The container
itself keeps running; the daemon buffers its output.)
await self.stop()raisesClientOSErroragainst theunreachable runtime, and that replaces the
TimeoutError. Callers get a connectionerror instead of the timeout they configured.
Separately,
start()andstop()areasync defbut do their work with blocking calls(
docker inspect,docker pull,docker build,find_free_port,docker kill, and await(timeout=5)loop). None of it is offloaded, so concurrent deployments stall eachother's event loop while
_wait_until_alive's deadline runs on a wall clock and eachis_aliveprobe gets only_runtime_timeout = 0.15s.What changed
src/swerex/deployment/docker.pyonly:tempfile.TemporaryFile()rather than pipes. Readsnever block and nothing is discarded. Closed in
stop()._read_container_output()helper replaces the four blocking.read()calls, andthe dead
assertthat sat after the reads it was meant to guard is gone.TimeoutError.asyncio.to_thread. The kill andimage removal blocks moved into
_kill_container()/_remove_container_image()sostop()stays readable.No public API or config change.
Verification
Before,
test_docker_deployment_startup_timeouthangs indefinitely (killed at 180s,leaving the container running). After, it raises
TimeoutErrorin ~6s and cleans up.Output capture, same container emitting 1MB: 1,000,272 bytes captured, where a pipe
caps at 65,536.
Full suite (
pytest -k "not cloud") goes from 76 passed to 78 passed. The 5 failures intest_execution.py,test_get_deployment.pyandtest_server.pyare identical beforeand after and unrelated to this change.
Notes
docker pullswhere they previously serialised. Docker deduplicates layer pulls, so this is safe.
AbstractDeployment.__del__while a loop is already shutting down ismarginally more best-effort than before, since
stop()now needs one more loopiteration to resume from its thread. That path was already best-effort.
swerex/utils/free_port.pyalso blocks the loop withtime.sleepand releases itssocket before docker binds the port. Left alone: it is shared with other deployments
and the test fixtures, and I have no evidence it contributes to this issue. Happy to
follow up separately.