Skip to content

feat(client,transport): expose httpx connection-pool limits on Comfy/AsyncComfy - #173

Open
mattmillerai wants to merge 1 commit into
mainfrom
matt/be-15952-httpx-limits
Open

mattmillerai wants to merge 1 commit into
mainfrom
matt/be-15952-httpx-limits

Conversation

@mattmillerai

Copy link
Copy Markdown
Contributor

ELI-5

models.run is awaited server-side: the server holds the connection open until the generation finishes. That connection comes out of the client's httpx pool, and the pool has always been httpx's default — max_connections=100 — with no way to change it from the public surface.

So a caller fanning out more than 100 concurrent runs did not get 101 runs. It got 100 runs and one call sitting in httpx's pool queue, waiting for a generation to finish. If nothing freed up, that call waited out the run timeout and raised httpx.PoolTimeout — a ten-minute stall ending in an error that comfy_sdk.retry's fast class (60s elapsed budget) will not retry.

The lever is httpx.Limits, and now it is reachable:

client = Comfy(limits=httpx.Limits(max_connections=250))

What changed

  • Comfy / AsyncComfy take a keyword-only limits: httpx.Limits | None = None, positioned after timeout, and pass it down to ComfyLow / AsyncComfyLow. Both class docstrings say what it is for and why a models.run fan-out is the case that needs it.
  • ComfyLow / AsyncComfyLow take the same keyword and hand it to the httpx.Client / httpx.AsyncClient they build.
  • None is dropped rather than forwarded. httpx.Client types limits as a Limits (default DEFAULT_LIMITS) and does not accept None, so passing it through would break the default path. The constructor builds a kwargs dict and only sets limits when one was given — which keeps the default exactly httpx's own, rather than importing the private httpx._config.DEFAULT_LIMITS to restate it.
  • An injected client= still owns its pool. limits is ignored on that path, exactly as timeout already is, and both __init__ docstrings say so.
  • README gains a paragraph directly after the run-timeout paragraph, and CHANGELOG an Unreleased / Added entry.

Tests

Five new cases in tests/test_models_namespace.py, next to the existing timeout view tests — no httpx mocks, they construct real clients against the server fixture stub:

  • Comfy(limits=httpx.Limits(max_connections=250)) and the AsyncComfy equivalent build a transport whose pool reports 250.
  • Comfy() and AsyncComfy() still report 100, which pins "None keeps httpx's default" rather than leaving it as an assumption about a code path nothing exercises.
  • ComfyLow(base_url, key, client=httpx.Client(), limits=httpx.Limits(max_connections=5)) keeps the injected client's pool — the transport object is the injected one and its pool still reports 100, not 5.

These read client._low._client._transport._pool._max_connections, which is httpcore internals. That is deliberate and it is the judgment call in this PR: httpx exposes no public getter for the limits a client was built with, so the alternative is asserting nothing about whether limits= arrived at all. The comment above the block says as much. A read-only limits view on the transport would remove the need, but it would mean caching the constructor value, and it is out of scope here.

Notes for the reviewer

  • The public API grows by one optional keyword on four constructors and nothing else changes. Both clients gained it, so test_paired_methods_take_the_same_parameters stays green; the default path is byte-identical in behaviour to before.
  • The README quotes ten minutes deliberately. On this base the pool timeout is MODEL_RUN_TIMEOUT's 600s, because the single positional argument to httpx.Timeout(600.0, connect=10.0) sets pool along with read and write. The open PR that lifts that run timeout to 660s would move this figure to eleven minutes — and the paragraph immediately above mine, which already says "10-minute timeout", moves with it. They are adjacent on purpose so a future edit catches both.
  • This is additive only. It denies no capability, adds no refusal path and flips no test to assert a dead end, so the negative-claim falsification check does not apply; there was no capability to go looking for before shipping a denial.
  • Several open PRs also touch README.md. This paragraph is additive and sits in the models.run section; whichever lands first, the other rebases cleanly or needs a one-hunk resolution.

Provenance

  • Authored by: agent-work loop
  • Verified: uv run --extra dev pytest: 980 passed, 9 skipped, 0 failed (the 9 skips are the network-gated e2e module); ruff check .: all checks passed; ruff format --check .: 57 files already formatted; mypy src: no issues in 21 source files; python3 scripts/check_public_repo_hygiene.py: no internal-only references. scripts/check_drift.py was not run — no generated model, spec file or Router error type is touched by this diff.
  • Deviations: none — every acceptance criterion is met. See Residual for what was deliberately left out of scope and what could not be exercised here.

Residual

Not fixed by this PR, each of them named as out of scope by the work that prompted it:

  • No read-only limits view on ComfyLow / AsyncComfyLow or client.models. The other transport settings (base_url, timeout, authenticated) have read-through properties and this one does not, which is an asymmetry a reader will notice. httpx exposes no public getter for a client's pool limits, so a view means caching the constructor value — a second source of truth for something the injected-client path deliberately does not control. Left out until a reviewer asks for it. Its direct cost is visible in this PR: the tests have to reach into httpcore internals for the assertion.
  • assets.py's per-call httpx.AsyncClient in from_url is untouched. It builds its own client per call and so never uses the shared pool; limits does not reach it. If a large concurrent from_url fan-out ever matters, that is a separate change (it should probably use the shared transport rather than gain its own limits).
  • MODEL_RUN_TIMEOUT.pool is unchanged. Whether the pool-acquisition wait should be generation-scale at all — rather than short, so a saturated pool fails fast inside the retry budget instead of stalling for minutes — is the open question this parameter routes around rather than answers. It is settled as a deliberate trade elsewhere and is being changed in another open PR; this PR does not touch it.

Unexercised artifacts:

  • No live fan-out was run. The claim that >100 concurrent models.run calls queue and then raise PoolTimeout is not reproduced here: the suite is network-free by design (a stdlib stub server, and this repo's guidance forbids httpx mocks), and a real reproduction needs 100+ concurrent generations against Router — a costly, account-billing action, so it was not run. What is verified is the mechanism: the pool the client builds reports the ceiling it was given, and the default is still 100. The failure mode itself is carried over from the review thread that raised it, not re-measured.
  • The originating investigation's findings were not read directly. This PR was built from the plan handed to it; the underlying research notes are on a tracker this branch has no access to.

`models.run` is awaited server-side, so each concurrent run holds its pooled
connection for the whole generation. The pool was always httpx's default
(`max_connections=100`) and nothing on the public surface could change it, so a
caller fanning out past 100 concurrent runs had its 101st call queue behind a
generation and — if nothing freed up — end in `httpx.PoolTimeout` after the run
timeout rather than in a result.

`Comfy`/`AsyncComfy` (and `ComfyLow`/`AsyncComfyLow` underneath) now take a
keyword-only `limits: httpx.Limits | None = None`. `None` is dropped rather
than forwarded — httpx types the parameter as a `Limits` and does not accept
`None` — so the default path stays httpx's own default exactly as before. An
injected `client=` owns its pool and ignores `limits`, as it already does
`timeout`.
@mattmillerai mattmillerai added the agent-coded Authored by the agent-work loop label Sep 19, 2026
@mattmillerai
mattmillerai marked this pull request as ready for review September 19, 2026 08:19
@mattmillerai
mattmillerai requested review from a team as code owners September 19, 2026 08:19
@coderabbitai

coderabbitai Bot commented Sep 19, 2026

Copy link
Copy Markdown

Warning

Review paused — included plan limit reached

Keep your review moving with free on-demand reviews.

  • Run this review for free

On-demand reviews are free for the next 21 days.

  • Ask an admin to make reviews automatic

Open in CodeRabbit

Reviews can continue after your included limit without a manual trigger. An admin must approve usage-based billing.

Promotion and pricing details

On-demand reviews are free for the next 21 days. After that, they cost $0.25 per reviewed file.

Review limit details

Or wait 30 minutes for your next included review.

Check out review usage here.

Limit details: You’ve used the included review currently available. Your 139 included PR review attempts over the past 7 days set your current allowance at 1 review per hour.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Repository: Comfy-Org/comfy-python-sdk/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Team

Run ID: 10c43eb9-7338-40ca-b5dd-792fa0fc5a55

📥 Commits

Reviewing files that changed from the base of the PR and between e4773c7 and 2bf66df.

📒 Files selected for processing (5)
  • CHANGELOG.md
  • README.md
  • src/comfy_low/transport.py
  • src/comfy_sdk/client.py
  • tests/test_models_namespace.py

Comment @coderabbitai help to get the list of available commands.

@mattmillerai mattmillerai added the cursor-review Request an automated Cursor review label Sep 19, 2026

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 Cursor Review — Consolidated panel

Triggered by @mattmillerai.

⚠️ Panel did not produce any findings.

Every reviewer in the matrix failed to contribute — see the panel summary for which cells errored, and the run logs for the underlying cause.

Panel: 0/6 reviewers contributed findings.

Reviewers that did not contribute: claude-opus-5-thinking-max:adversarial (error), gpt-5.6-sol-max:adversarial (error), kimi-k3-high:adversarial (error), claude-opus-5-thinking-max:edge-case (error), gpt-5.6-sol-max:edge-case (error), kimi-k3-high:edge-case (error)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agent-coded Authored by the agent-work loop cursor-review Request an automated Cursor review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant