Repository navigation
fix(local): report a taken host port instead of a raw APIError - #9294
Conversation
![aws-sam-tooling-bot[bot]](https://avatars.githubusercontent.com/b/1290?s=60&v=4){kind=small}
| PORT_TAKEN_WORDINGS = ( | ||
| "Ports are not available", # Docker Desktop | ||
| "port is already allocated", # Linux daemon, port held by another container | ||
| "bind: address already in use", # Linux daemon, port held by a non-Docker process |
There was a problem hiding this comment.
[GENERAL] The three wordings are Docker-only, but Container.start can be talking to Finch. self.docker_client resolves through utils.get_validated_container_client(), which can return FinchContainerClient (samcli/local/docker/container_client.py:477), and nerdctl does not phrase a publish failure the way moby's port allocator or the userland proxy does. On that backend a taken host port still escapes as a raw APIError — the exact failure this PR removes for Docker.
The codebase already treats backend wording differences as a real concern rather than an edge case: is_dockerfile_error is implemented separately on DockerContainerClient ("Cannot locate specified Dockerfile") and FinchContainerClient ("no such file or directory") for the same reason.
The cheapest fix is to drop the layer-specific prefix so the match is not tied to the userland proxy's phrasing:
PORT_TAKEN_WORDINGS = (
"Ports are not available", # Docker Desktop
"port is already allocated", # Linux daemon, port held by another container
"address already in use", # bind refused: port held by a non-Docker process
)"address already in use" is the OS-level text every backend surfaces, and a container-start APIError carrying it is a publish conflict in practice, so widening it does not realistically misclassify anything. The same applies to HOST_INTERFACE_MISSING_WORDING and to the message it produces, which names Docker explicitly.
There was a problem hiding this comment.
Good instinct — backend wording drift is real, and is_dockerfile_error is the right precedent to cite. But I measured this against Finch rather than reasoning about it, and the conclusion doesn't hold: Finch never reaches this handler.
I ran the Docker-compatible socket FinchContainerClient actually connects to (unix:///var/run/finch.sock, platform_config.py:130) with Finch 1.17.2 / nerdctl v2.3.5 / containerd 2.2.7, doing create then start with a published port, once per case:
| case | Docker | Finch |
|---|---|---|
| ordinary process holds the port | start(): bind: address already in use |
create(): bind for 0.0.0.0:3101 failed: port is already allocated |
| another container holds the port | start(): port is already allocated |
create(): bind for 0.0.0.0:3102 failed: port is already allocated |
| address absent on the host | start(): bind: cannot assign requested address |
no error — the publish is accepted, exit 0 |
Three things follow:
- Finch fails at
create, notstart. nerdctl reserves host ports while parsing the port mapping, so theAPIErroris raised out ofContainer.create()and this handler is never entered. Widening its wordings cannot affect Finch either way. - Its wording is
port is already allocated— already matched here. So even on thestartpath there would be nothing to add. - The third case has no Finch analogue. Finch happily publishes on an address that doesn't exist on the host, so there is no error to translate.
On the specific suggestion: dropping the prefix to "address already in use" would widen the match without buying any Finch coverage, because Finch never emits that string. I'd rather keep bind: address already in use, which is the exact text I observed, than loosen a substring match on speculation. Happy to revisit if someone sees a real backend emitting the bare form.
What the measurement did turn up is a genuine defect, just not in this handler. Because Finch raises at create, a taken port on Finch today produces:
Container creation failed: failed to parse port mapping "0.0.0.0:3001:80/tcp":
bind for 0.0.0.0:3001 failed: port is already allocated, check template for
potential issue
That's Container.create's blanket DockerAPIError → DockerContainerCreationFailedException (container.py:305-308). It's a UserException, so no traceback — but it tells the user to check their template for what is a port conflict, and it's the one place PortAlreadyInUse should be raised for Finch users. I'll open that separately rather than grow this PR; it needs the same create-path treatment for both backends, and the taxonomy question there is worth its own review.
github-actions
Bot
added
area/local/start-api
Container.start recognised one daemon's wording for a port collision and let the others through as a bare docker.errors.APIError. None of the local command handlers convert that type, so the user gets a traceback instead of a message. A daemon refuses a publish in three different ways, depending on which layer says no. Verified against Docker 25 on Linux by driving Container.start at a port held by another container, a port held by an ordinary process, and an address that does not exist on the daemon host: another container holds the port port is already allocated a local process holds the port bind: address already in use the interface does not exist bind: cannot assign requested address Only the first was matched, alongside Docker Desktop's "Ports are not available". The second now maps to PortAlreadyInUse as well. The third is not a port conflict and must not be reported as one, but it should not be a traceback either: it is what a stale or mistyped --container-host-interface produces. It now raises ContainerNotStartableException, which the local commands already convert, with text naming that flag. Also fixes the explanation decode in the same handler. docker-py has handed back a str since 7.0, so the existing .decode() would itself have raised AttributeError on the one path it did match; bytes are still tolerated for older versions.
_initialize_all_functions_containers allowlists PortAlreadyInUse and funnels
everything else into ContainersInitializationException("Lambda functions
containers initialization failed"). The ContainerNotStartableException added in
this PR was hitting that wrapper, so with --warm-containers EAGER and a stale
--container-host-interface the user got the generic message and the guidance
survived only in an ERROR log line. The same masking applied to the pre-existing
NoFreePortsError -> ContainerNotStartableException path in container.py.
Both exceptions name the flag the user has to change, so both are now re-raised
as themselves; all three local commands already convert them.
The allowlisted branch also has to clean up. It runs from __enter__, so a raise
there means __exit__ never runs, and the existing PortAlreadyInUse branch was
leaking every container created before the failure -- the two branches around it
both call _clean_running_containers_and_related_resources.
Container.start reaches whichever engine get_validated_container_client returned, which is Docker or Finch, so matching on one engine's whole sentence misses the other. Docker's userland proxy prefixes the bind errno with "bind: "; nerdctl surfaces the bare text and the nerdctl binary carries no "bind: "-prefixed form of either message. So "bind: address already in use" never matched a Finch bind-level refusal, and neither did "bind: cannot assign requested address" - both escaped as a raw docker.errors.APIError, which is the failure this handler exists to remove. Both matches are now prefix-free. "port is already allocated" already covered Finch: it reports "bind for 127.0.0.1:38199 failed: port is already allocated" (lowercase "bind for", against Docker's "Bind for"), which the fragment match catches. Measured against Finch 1.17.2 / nerdctl 2.3.5 and Docker 25.0.16. The ContainerNotStartableException message no longer says "Docker host" or "Docker daemon", since either engine can be the one refusing the publish, and a test asserts the message names no single engine.
e0cf14e to
9fc415e
Compare
🧪 Hands-on test: No problems observed in 7 scenariosCommit Scenarios
Runner's notes in full
🤖 Automated hands-on test — not a review approval · model claude-opus-5.5 · Kiro kiro-cli 2.33.1 · build History (4)
|
The interpolated detail does not end in punctuation, so the message read "...cannot assign requested address Pass --container-host-interface with...". Parenthesising the detail fixes it without having to normalise punctuation that comes from the engine. Flagged by the hands-on runner on 9fc415e.
|
Thanks — this run is more useful than its green verdict suggests, because S2 independently shows the prefix-free match matters on Docker too, not only on Finch. That scenario held the port with an ordinary host process on Docker 29.9.2, and the daemon said: No Also fixed the cosmetic defect from S6, in One limitation of the run worth stating for the record, since it is the thing this finding was about: per S1's note there is no Finch engine on the build host, so the Finch wordings are covered by unit-test strings taken from a live Finch measurement (Finch 1.17.2 / nerdctl 2.3.5, driven through |
| # The engine's own detail is parenthesised: it does not end in punctuation, | ||
| # so interpolating it bare ran it into the next sentence ("...requested | ||
| # address Pass --container-host-interface..."). |
There was a problem hiding this comment.
This comment does not make sense to me, could you either revise it or explain to me?
There was a problem hiding this comment.
You are right, and thanks — that comment was explaining how the message used to be wrong rather than what the code is doing, which is no use to a reader who never saw the bug. Revised in 99bfb072c:
# detail is the engine's own text and has no trailing period, so it is wrapped
# in parentheses to keep it from running into the sentence that follows.For the record, the thing it is guarding against: detail comes from the engine, e.g. failed to bind host port 192.0.2.1:6706/tcp: cannot assign requested address, with no trailing punctuation. Interpolating it bare produced ...requested address Pass --container-host-interface with... in one run-on sentence. Parenthesising it avoids having to normalise punctuation that is not ours.
The comment explained how the message used to be wrong rather than what a reader needs to know about it now. Reviewer could not follow it.
|
@reedham-aws — both fair, and the second one is a good question with a non-obvious answer. Does this close #9295?No. Your reasoning is right as far as the wording goes — That list is only consulted in So a Finch user whose host port is taken is still told to check their template for a port conflict. This PR does not touch Measured, driving each engine's own socket:
#9295 is the The #9290 relationshipAgreed that was hard to follow — it was three facts in one paragraph. I have rewritten that section of the description as a table. Short version:
The first two rows are why this PR stands on its own. The third row is the only coupling: #9290 stops pre-checking the port locally, which is what makes that case reachable at all. The practical consequence, and the only thing you need from it at merge time: merge this one before #9290. #9290 alone turns that third case from a clean I also revised the code comment you flagged — it was describing a bug I had already fixed rather than the rule, which is why it did not parse. |
| @staticmethod | ||
| def _api_error_detail(ex: docker.errors.APIError) -> str: | ||
| """ | ||
| Returns the daemon's own explanation for an APIError, falling back to the full message. | ||
|
|
||
| docker-py hands back a str today and bytes in older versions. | ||
| """ | ||
| explanation = ex.explanation | ||
| if isinstance(explanation, bytes): | ||
| explanation = explanation.decode() | ||
| return explanation or str(ex) |
There was a problem hiding this comment.
Sorry I didn't comment on this earlier, but I don't see why wee need this method to decode bytes. Either we have the newer version of docker-py or we don't?
There was a problem hiding this comment.
You are right — dropped in 9dc069352, and the helper went with it since it had one call site.
Checked rather than argued: the pin is docker>=7.1,<7.3 (resolving to 7.2.0), and in that range docker/errors.py sets explanation from response.json()["message"] or (response.text or "").strip(), both str on every path. So the bytes branch could not run.
Where it came from, since it was my mistake and not inherited blindly: the line before this PR was
raise PortAlreadyInUse(ex.explanation.decode()) from exwhich assumed the opposite — and would raise AttributeError on a str, on the one wording it did match. I kept that assumption while fixing the crash instead of checking which type the pin actually gives. Your question is what settled it.
What is left is one line:
detail = ex.explanation or messageThe or is load-bearing rather than defensive: APIError.__init__ defaults explanation=None, and the response.text path can yield "", so without it the reported reason comes out blank. I swapped the bytes test for one covering that fallback and mutation-checked it — removing the or message turns exactly that test red.
Net −11 lines. make pr green: 9763 passed, 94.08% coverage, ruff/black/mypy clean.
Sorry about the approval — the push dismissed it, since develop has dismiss_stale_reviews on. Only the two files above changed from the revision you read.
…ng bytes The pin is docker>=7.1,<7.3, and in that range docker/errors.py sets explanation from response.json()["message"] or (response.text or "").strip(), both str on every path. So the bytes branch could not run, and the helper that held it had one call site. The pre-existing code was ex.explanation.decode(), which assumed the opposite and would raise AttributeError on a str. This change keeps the fix for that and drops the speculative half: explanation is read directly, with a fallback to the full message because APIError.__init__ defaults explanation to None and the response.text path can yield "". The test that asserted the bytes path is replaced by one covering the fallback, which is the branch that can actually be reached.
Which issue(s) does this change fix?
no linked issue: split out of #9290 during review, where the reviewer showed the handler covered only one of the daemon's three wordings. Related: #2837.
Why is this change necessary?
Container.starttranslates a port collision intoPortAlreadyInUse, but only for one daemon's wording:Ports are not availableis Docker Desktop's phrasing. A daemon refuses a publish in three different ways depending on which layer says no, and the other two fall through toraise ex. None of the local command handlers listdocker.errors.APIError—samcli/commands/local/invoke/cli.pyconvertsPortAlreadyInUse,ContainerNotStartableExceptionandDockerContainerCreationFailedException— so the user gets a traceback instead of a message.Container.startreaches whichever engineget_validated_container_clientreturned, so the match has to hold for Docker and Finch. I drove it at a real engine for each case rather than pattern-matching on documentation:Bind for ...: port is already allocatedbind for ...: port is already allocatedAPIErrorbind: address already in useaddress already in use(bare)APIErrorbind: cannot assign requested addresscannot assign requested address(bare)APIErrorThe
bind:prefix is Docker's userland-proxy formatting; nerdctl surfaces Go's bare errno text and its binary contains nobind:-prefixed form of either message. So the matches are deliberately prefix-free — a prefixed match covers Docker only. Measured against Docker 25.0.16 and Finch 1.17.2 / nerdctl 2.3.5 on Linux; the Docker Desktop wording (Ports are not available) is as reported.The prefix turned out to matter on Docker as well, not only on Finch: the hands-on runner reproduced the process-held-port case on Docker 29.9.2, where the daemon reports
failed to bind host port 127.0.0.1:5891/tcp: address already in usewith nobind:prefix at all. A prefixed match misses that too, so matching the errno fragment rather than any one engine's sentence is what makes this hold across both engines and across Docker versions.The second is the likely shape whenever the daemon is not on the developer's own machine, because there the port's holder is usually not a container.
There is also a latent bug in the same three lines:
ex.explanationhas been astrsince docker-py 7.0, so.decode()would raiseAttributeErroron the one path the condition did match. No test covered it.How does it address the issue?
Both port-taken wordings now raise
PortAlreadyInUse, listed as named constants beside the class rather than inline literals.The third case is not a port conflict and is not reported as one — but it should not be a traceback either. It is what a stale or mistyped
--container-host-interfaceproduces, so it raisesContainerNotStartableException(already converted by the local commands) with text naming that flag:The engine's detail is parenthesised because it does not end in punctuation, so interpolating it bare ran it into the next sentence.
The message says "container engine" rather than Docker because either engine can be the one refusing the publish; a test asserts it names no single engine.
explanationis read directly, with a fallback to the full message:APIError.__init__defaults it toNoneand theresponse.textpath can yield"", so without the fallback the reported reason would be blank. It is never decoded — the pin isdocker>=7.1,<7.3, and in that rangedocker/errors.pybuildsexplanationfromresponse.json()["message"]or(response.text or "").strip(), bothstr, so the.decode()above was unreachable as well as wrong.Raising the exception is not enough on the eager warm-container path, so
samcli/commands/local/cli_common/invoke_context.pychanges too._initialize_all_functions_containersallowlisted onlyPortAlreadyInUseand funnelled everything else intoContainersInitializationException("Lambda functions containers initialization failed"), so with--warm-containers EAGERthe guidance above survived only in anERRORlog line. Both exceptions now re-raise as themselves. That branch also has to release what it created: it runs from__enter__, so a raise there means__exit__never runs, and thePortAlreadyInUsebranch was leaking every container started before the failure while the branches on either side of it both clean up. This also un-masks the pre-existingNoFreePortsError→ContainerNotStartableExceptionpath atcontainer.py:175.Verified end to end by driving
Container.startagainst the daemon once per case:What side effects does this change have?
Two failures that used to be tracebacks become
UserExceptions. Nothing that already produced a clean error changes: the Docker Desktop path behaves exactly as before.Finch needed the match widened, and also has a separate defect. My first pass tested Finch through its socket on Linux with rootful nerdctl, where a taken port is refused by the port allocator and reads
bind for ...: port is already allocated— already matched, so I concluded Finch was covered. That configuration never exercised a bind-level refusal from nerdctl, which carries the bare errno text; the reviewer was right that the prefixed matches missed it, and the same defect was inHOST_INTERFACE_MISSING_WORDING. Both are prefix-free now, with test rows that go red if the prefix returns. Separately, Finch reserves host ports while parsing the port mapping, so the common conflict is raised out ofContainer.create()and never reaches this handler at all — that is Bug: a taken host port is reported as a template problem on Finch, because Finch fails at container create rather than start #9295, fixed on its own branch rather than grown into this PR.Both PRs now touch
invoke_context.py. The hunks are far apart (imports and_initialize_all_functions_containershere,__enter__and a new helper in fix(local): make port publishing usable when SAM CLI runs in a container #9290) and do not conflict.Relationship to fix(local): make port publishing usable when SAM CLI runs in a container #9290. Three cases, two PRs. This PR turns the engine's refusal into a clean error. fix(local): make port publishing usable when SAM CLI runs in a container #9290 changes which refusals can happen.
developtodayThe first two are worth fixing on their own. The third only becomes possible once fix(local): make port publishing usable when SAM CLI runs in a container #9290 stops pre-checking the port locally, which is why I verified it with both branches stacked.
Merge this one first. On its own, fix(local): make port publishing usable when SAM CLI runs in a container #9290 replaces a clean
No free ports on the host machinemessage with a rawdocker.errors.APIErrortraceback for the third case, because the fix for it lives here. The hands-on runner reproduced that on fix(local): make port publishing usable when SAM CLI runs in a container #9290 independently, 2 of 2 runs.Wordings are daemon strings, so they can drift. They are matched as substrings and collected in one place; a future daemon rephrasing degrades to today's behaviour (a raw
APIError) rather than a wrong error.Mandatory Checklist
make prpassesmake update-reproducible-reqsif dependencies were changed — no dependency changesBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.