fix(models,transport): read the Idempotent-Replayed header and lift the run timeout above Router's deadline - #168
mattmillerai wants to merge 3 commits into
Conversation
…declares RouterRunResult.replayed read X-Comfy-Idempotent-Replayed, but Router sends Idempotent-Replayed (no X-Comfy- prefix), as spec/router-openapi.yaml declares on every replayable response. The names could never match, so replayed was False on every response including a genuine replay — the exact signal a caller uses to avoid double-charging, wrong 100% of the time and silently, since False is also correct for the common case. Fix the header name (and the field docstring, which carried the same wrong prefix). Add an end-to-end test through the fake server, which sends the real Idempotent-Replayed header, and re-point the unit test at the contract name rather than a literal copied from the implementation.
MODEL_RUN_TIMEOUT was 600s, exactly Router's DefaultDeadline (600s). At equality the client can abort at the same instant Router is writing its 504 deadline_exceeded, so the caller gets a bare client-side timeout with no X-Comfy-Request-Id, no Retry-After and no error body — the generation billed and still running, the id needed to reason about it gone. Derive the timeout from a named ROUTER_DEADLINE constant plus a minute of headroom, and document the coupling at the constant so the next person changing either number sees it.
|
Warning Review paused — included plan limit reachedKeep your review moving with free on-demand reviews.
On-demand reviews are free for the next 21 days.
Reviews can continue after your included limit without a manual trigger. An admin must approve usage-based billing. Promotion and pricing detailsOn-demand reviews are free for the next 21 days. After that, they cost $0.25 per reviewed file. Review limit detailsOr wait 2 minutes for your next included review. Limit details: You’ve used the included review currently available. Your 138 included PR review attempts over the past 7 days set your current allowance at 1 review per hour. Review configuration: ⚙️ Run configurationConfiguration used: Repository: Comfy-Org/comfy-python-sdk/.coderabbit.yaml Review profile: ASSERTIVE Plan: Team Run ID: 📒 Files selected for processing (7)
Comment |
mattmillerai
added
agent-coded
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔍 Cursor Review — Consolidated panel
Triggered by @mattmillerai.
Found 5 finding(s).
| Severity | Count |
|---|---|
| 🟡 Medium | 1 |
| 🟢 Low | 3 |
| ⚪ Nit | 1 |
Panel: 6/6 reviewers contributed findings.
mattmillerai
added
the
full-autonomy
robinjhuang
left a comment
robinjhuang
left a comment
There was a problem hiding this comment.
Auto-approved under the full-autonomy policy.
Gates verified at 5ec2ad10b41b32869893cab6ad5c51aaf1316608:
full-autonomylabel present- assigned to, or review requested from, @robinjhuang
- not a draft
- 8 required check(s) green — none failing, none pending
Issued by full-autonomy-approve.yml (run). This approval attests
that the machine gates above passed at this commit. It does not attest that a
human read the diff.
…ed names, sane write bound Addresses the cursor-review panel on #168. - `_run_result` normalizes through `httpx.Headers` before every lookup. It is annotated `Mapping[str, str]` and all four names it reads are mixed-case, so a plain mapping (`dict(resp.headers)`, which httpx lowercases) missed all of them silently -- `replayed=False`, `request_id=None`, no error. That is the same shape of silent miss the wrong header name in this PR produced. - The four header names are named constants pinned against `spec/router-openapi.yaml` by a new parametrized test in test_router_spec_contract.py, one-way (everything read is declared; the 200 also carries headers the SDK does not surface). A wrong name cannot fail at runtime, so only a comparison against the contract catches it -- this is the check that was missing when `Idempotent-Replayed` was read prefixed. - `MODEL_RUN_TIMEOUT` names all four bounds explicitly. A single positional argument set `write` and `pool` to the generation wait as well; `write` bounds pushing the request body up and has no business getting eleven minutes, so it is now 120s -- still generous for a multi-megabyte inline base64 image. `pool` stays generation-scale deliberately (every pooled connection is held for a whole generation and `Comfy` exposes no `limits=`, so shortening it would turn a legitimate fan-out past httpx's 100 connections into a wall of PoolTimeout); it is now stated and asserted as a decision rather than inherited. - Docs that asserted the old `MODEL_RUN_TIMEOUT == Router's deadline` equality now point at `ROUTER_DEADLINE`, which is the constant that is actually ten minutes and the one `collect_max_elapsed`'s "two windows" derivation depends on: retry.py's module docstring and `collect_max_elapsed`, models.py's `run` docstring, and README.md. Both new guards were falsified by reverting their fix and confirming they fail. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
🤖 The reviews loop filed Linear follow-up ticket(s) for review thread(s) deferred as out of scope for this PR:
The following carry
|
robinjhuang
left a comment
robinjhuang
left a comment
There was a problem hiding this comment.
Auto-approved under the full-autonomy policy.
Gates verified at f6865acedfd8edadba322979274b40ed710abb92:
full-autonomylabel present- assigned to, or review requested from, @robinjhuang
- not a draft
- 8 required check(s) green — none failing, none pending
Issued by full-autonomy-approve.yml (run). This approval attests
that the machine gates above passed at this commit. It does not attest that a
human read the diff.
2 participants
ELI-5
Two small money bugs in the model-run path. First, the SDK asked the response for a header spelled
X-Comfy-Idempotent-Replayed, but the server actually sendsIdempotent-Replayed(noX-Comfy-prefix) — soRouterRunResult.replayedcame backFalseon every response, including a genuine replay, which is exactly the "you already paid for this, don't charge again" signal. Second, the client's model-run timeout was 600s and Router's own deadline is also 600s, so the client could hang up at the same instant the server was sending its504— losing the request id andRetry-Aftera caller needs to reason about a generation that is billed and still running. This fixes the header name and lifts the client timeout to sit one minute above Router's deadline.What changed
1.
replayedreads the header the contract declares.RouterRunResult._run_resultnow readsIdempotent-Replayedinstead ofX-Comfy-Idempotent-Replayed. The vendored contract confirms the name:spec/router-openapi.yamldefines theRouterIdempotentReplayedHeadercomponent and references it asIdempotent-Replayedon every replayable response (the200, and the recorded4xx/5xxshapes). The field's own docstring carried the same wrong prefix and is corrected too. Headers arrive ashttpx.Headers(case-insensitive), so the defect was purely the wrong name, not casing.Idempotent-Replayed) rather than a literal copied from the implementation — a literal repeated from the code is what let the bug pass review in the first place. A new end-to-end test (test_run_detailed_reports_a_replay_end_to_end_against_the_server) drives a real replay through the fake server, which replies with the actualIdempotent-Replayed: trueheader, and assertsrun_detailed(...).replayed is True. Reverting only the code line makes both of these fail (confirmed locally), so they genuinely guard the fix rather than restating it.2. Client run timeout now sits above Router's deadline.
MODEL_RUN_TIMEOUTis no longer a bare600.0. A namedROUTER_DEADLINE = 600.0constant mirrors Router's server-sideDefaultDeadline, and the client read timeout is derived asROUTER_DEADLINE + 60sof headroom — enough to receive the504and its headers instead of racing it. The coupling is documented at the constant ("if you raise Router's deadline, raise this too").connectstays 10s.test_the_collect_budget_outlasts_a_server_deadline_windowpreviously usedMODEL_RUN_TIMEOUT.readas its stand-in for "the server's deadline window" — that only worked because the two values were mistakenly equal, which is defect 2 itself. It now sources the window fromROUTER_DEADLINEand additionally asserts the new invariantMODEL_RUN_TIMEOUT.read > ROUTER_DEADLINE. The other timeout tests (>= 120sread,<= 30sconnect; the literal600.0server-deadline advances in the fake-clock tests) remain correct because Router's deadline itself is unchanged.Scope swept
Both defects are fully addressed in this repo. Sweep of the portion not being changed:
grepacrosssrc/found exactly three occurrences of the replayed-header spelling — the code read (fixed), the field docstring (fixed), and one prose reference atmodels.py:436that was already correct — and no other reader of the header anywhere insrc/. AllMODEL_RUN_TIMEOUTconsumers were checked; none assume the literal600except the one retry test corrected here.Residual
X-Comfy-Credits-UsedonRouterRunResultand edits this same header-reading block in_run_result. It has not landed onmain(verified: noCredits-Used/credits_usedinsrc/at this base), so there is no conflict today, but whichever lands second will need a trivial rebase of that block. That change's author should also confirm the credits header name againstspec/router-openapi.yamldirectly, given this defect was a wrong prefix on a sibling header — I could not verify the credits header name here because that code is not in this branch.router-reference.mdx(the header docs) androuterdeadline/routerdeadline.go(DefaultDeadline = 10 * time.Minute). I could not open either. I verified the header name against this repo's vendoredspec/router-openapi.yaml(synced from that service) instead, and encoded the 10-minute deadline asROUTER_DEADLINE = 600.0with a comment pointing at the Go constant; if Router's deadline is ever raised, this default must be raised to match.COMFY_ROUTER_DEADLINEin the client. The report floated deriving the timeout from that env var. I did not, because it configures the Router deployment's environment, not the caller's — a same-named client-side variable would be a misleading second source of truth that silently disagrees with the server it is meant to track. The client-side mitigation is the named-and-documentedROUTER_DEADLINEconstant plus headroom, which is the "at minimum, comment the relationship" path.Review round — cursor-review panel
Five threads, all resolved. Three fixes, one partial, one answered without a code change.
Fixed —
_run_resultreads headers case-insensitively. It is annotatedMapping[str, str]and all four names it reads are mixed-case, so it worked only becausepost_model_runhappens to hand it anhttpx.Headers. Any plain mapping —dict(resp.headers), which httpx lowercases — would have missed all four and yieldedreplayed=Falseandrequest_id=Nonewithout raising, which is the same silent shape as the wrong header name this PR fixes. It now normalizes throughhttpx.Headers(headers)first.Fixed — the header names are pinned to the contract. The four names are now constants (
_RUN_RESULT_HEADERS) andtests/test_router_spec_contract.py::test_the_headers_a_run_result_reads_are_the_spec_spins each againstspec/router-openapi.yaml'srunRouterModel200, parametrized per name, reached by searching for theoperationIdrather than looking up the path. The assertion is one-way (everything read is declared) because the 200 also carries headersRouterRunResultdoes not surface. Plustest_the_replayed_header_carries_no_x_comfy_prefixas a named regression case. This is the check whose absence let the prefixed name through.Fixed — docs that asserted the old equality. Decoupling the client timeout from Router's deadline falsified prose saying the
504arrives at "the same ten minutes asMODEL_RUN_TIMEOUT".retry.py's module docstring andcollect_max_elapseddoc,models.py'srundocstring, andREADME.mdnow point atROUTER_DEADLINE— the constant that is actually ten minutes and the onecollect_max_elapsed's "two windows" derivation depends on.Partial — the positional
httpx.Timeoutargument. Correct that one positional value setwriteandpoolas well asread.writeis fixed: pushing a request body has no business inheriting an eleven-minute generation wait, so it is now 120s (still generous for a multi-megabyte inline base64 image, which httpx hands to the transport as a single write).poolis deliberately kept generation-scale, because on this route the pool is full of generations: a run is awaited server-side, so all 100 of httpx's default connections are held for up toROUTER_DEADLINE, and the 101st caller is queued behind a generation rather than a handshake. A short pool bound would convert a legitimate fan-out from a queue that drains into a wall ofPoolTimeout— 150 concurrent 2-minute runs succeed today and would fail inside ~70s under a 10s bound plus the 60smax_elapsed. The accepted cost is written on the constant: in the pathological case where the pool never frees, the caller waits eleven minutes andPoolTimeoutsurfaces without a retry, its fast-class budget long spent. All four bounds are now named explicitly so none is inherited again, andtest_every_bound_of_the_run_timeout_is_chosen_rather_than_inheritedasserts the trade.Answered, no code change — a client-side
COMFY_ROUTER_DEADLINE. Already covered under Residual below; it configures Router's environment, not the caller's, and a same-named client variable would be a second source of truth that can silently disagree with the server it tracks. Additionally, this repo's env idiom is per-construction (_resolve_router_base_url()atclient.py:250/:414), not import-time, so doing it properly means threading a resolved value through everytimeout=MODEL_RUN_TIMEOUTdefault — a design change, not an override. The supported lever for a longer-deadline deployment is the publicrun(..., timeout=...).Deferred to a tracked follow-up.
Comfy/AsyncComfyexpose nolimits=, so a caller cannot raisemax_connections— the real lever for a fan-out that should not queue at all. New public API on both client surfaces, out of scope for a defect fix; proposed as a follow-up rather than dropped.Provenance
uv run pytest: 907 passed, 9 skipped;ruff check .: clean;ruff format --check .: 55 files already formatted;mypy src: no issues in 20 files;scripts/check_drift.py: in sync;scripts/check_public_repo_hygiene.py: no internal-only references. Falsified the two new guards by reverting each fix — removing thehttpx.Headersnormalization failstest_run_detailed_reads_the_headers_case_insensitively, and reinstating theX-Comfy-prefix on the header constant fails both spec-contract cases. The originalreplayedtests were falsified the same way in the first round.MODEL_RUN_TIMEOUT.poolis kept generation-scale rather than shortened as the panel suggested, because shortening it breaks fan-out past 100 concurrent runs; andCOMFY_ROUTER_DEADLINEis still not read client-side, per the Residual note below. One finding was deferred to a follow-up (limits=passthrough on the clients) rather than fixed here.