diff --git a/CHANGELOG.md b/CHANGELOG.md index 5f4968d..0217bb6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -66,6 +66,20 @@ notes for each version. `forbidden`, `insufficient_credits`) keep their existing classes, which catch on both surfaces. `Retry-After` pacing is keyed on the status and survives unchanged. +- A `409` whose body names no error code at all now raises a plain + `ComfyError` carrying `http_status == 409`, instead of `HashMismatch`. The + status table that decoded it is consulted only when the response named no + code of its own, so it never sees the compliant envelope surface — it sees + Router-shaped `{detail, error_type}` bodies and intermediaries, which can + answer a `409` for anything, and the contract itself already spells `409` + two ways (`hash_mismatch` on `POST /assets`, `asset_in_use` on + `DELETE /assets/{id}`). Guessing `HashMismatch` told those callers to + re-upload bytes over a conflict that was never about bytes. Enveloped + responses are unaffected: an `error.code` of `hash_mismatch` still raises + `HashMismatch`, as does the `409` `POST /assets` documents, and a `409` + carrying a Router bucket still keeps that bucket. Any `Retry-After` on the + response still reaches the caller on `.retry_after`. `422` and `429` keep + their status-derived codes. - A Router `409` now keeps the bucket the contract names instead of decoding to `HashMismatch` off the status table. The synced contract declares two `409`s on the run route — `invalid_input` for a key that cannot serve this diff --git a/src/comfy_low/errors.py b/src/comfy_low/errors.py index 3cac931..355bb73 100644 --- a/src/comfy_low/errors.py +++ b/src/comfy_low/errors.py @@ -180,11 +180,14 @@ def error_from_envelope( ``invalid_input`` into ``invalid_workflow`` (failing every reachability probe), and its ``403`` ``not_enabled`` into ``forbidden`` (so ``except NotEnabled`` — the one handler every pre-launch caller writes — never - fired). Retyping only happens on responses that carry a bucket, which only - Router sends, and there the bucket IS the truth; a v2 envelope carries - ``error.code`` and no top-level ``error_type``, and an intermediary's - reject carries neither, so both keep exactly the classes integrators - already catch. + fired). ``409`` has since been dropped from the table outright (see the + admission rule on :data:`_CODE_BY_STATUS`), so the first of those three no + longer needs this ordering as its second line of defence; the ordering + still decides the other two. Retyping only happens on responses that carry + a bucket, which only Router sends, and there the bucket IS the truth; a v2 + envelope carries ``error.code`` and no top-level ``error_type``, and an + intermediary's reject carries neither, so both keep exactly the classes + integrators already catch. """ err = (body or {}).get("error") if isinstance(body, dict) else None code = (err or {}).get("code") if isinstance(err, dict) else None @@ -216,12 +219,37 @@ def error_from_envelope( ) +#: The last resort: a code guessed from the status, consulted only when the +#: response named none of its own — no envelope ``error.code``, no Router +#: bucket. That is exactly the population this table must be sized for, and it +#: is not the compliant surface: the producers of a code-less body are Router +#: (whose error body is ``{detail, error_type}``, with no ``error.code``) and +#: the intermediaries between the caller and either surface, and neither is +#: bound by what any route documents for the status. +#: +#: **Admission rule for a new status: only a status with ONE meaning across +#: every documented surface gets a typed guess.** A status the contract itself +#: spells two ways cannot be guessed, because the guess is not "unknown, but +#: roughly this" — it is a class the caller catches and acts on. +#: +#: ``409`` is the worked example of exclusion, and it was removed from this +#: table for that reason: the contract uses it for both ``hash_mismatch`` +#: (``POST /assets``) and ``asset_in_use`` (``DELETE /assets/{id}``), so even +#: on the compliant surface the status alone does not say which. A code-less +#: ``409`` therefore stays a bare ``ApiError`` carrying the real status, and +#: surfaces to the caller as a plain ``ComfyError``. A real hash mismatch is +#: unaffected: it always arrives enveloped, and ``error.code`` wins outright. +#: +#: ``422`` and ``429`` are kept deliberately, and the rule is what keeps them: +#: their possible misreadings stay inside the right *action class*. A ``422`` +#: read as ``invalid_workflow`` is still a terminal refusal to fix the request; +#: a ``429`` read as ``queue_full`` is still back-off-and-retry. ``HashMismatch`` +#: is the one that failed the rule — it tells the caller to re-upload bytes. _CODE_BY_STATUS: dict[int, str] = { 401: "unauthorized", 402: "insufficient_credits", 403: "forbidden", 404: "not_found", - 409: "hash_mismatch", 422: "invalid_workflow", 429: "queue_full", } diff --git a/tests/test_error_mapping.py b/tests/test_error_mapping.py index fd9ddd8..69a58db 100644 --- a/tests/test_error_mapping.py +++ b/tests/test_error_mapping.py @@ -1,15 +1,23 @@ """How an error response on the wire becomes a typed exception. -`to_sdk_error` mapping the server's 404 codes to the typed `NotFound`, and -`error_from_envelope` reading the two body shapes this API answers in. +`to_sdk_error` mapping the server's 404 codes to the typed `NotFound`, +`error_from_envelope` reading the two body shapes this API answers in, and +which statuses may be decoded to a typed code when the response named none. """ from __future__ import annotations import pytest -from comfy_low.errors import ApiError, QueueFull, error_from_envelope -from comfy_sdk.exceptions import NotFound, to_sdk_error +from comfy_low.errors import ( + ApiError, + HashMismatch, + QueueFull, + Unauthorized, + error_from_envelope, +) +from comfy_sdk.exceptions import ComfyError, NotFound, to_sdk_error +from comfy_sdk.exceptions import HashMismatch as SdkHashMismatch @pytest.mark.parametrize("code", ["not_found", "job_not_found", "asset_not_found"]) @@ -96,6 +104,55 @@ def test_a_bucketless_429_still_means_queue_full() -> None: assert err.retry_after == 3 +# --- which statuses the status table may decode, and which it may not --- +# +# The table is consulted only for a response that named no code of its own, so +# it never sees the compliant envelope surface -- it sees Router-shaped bodies +# and intermediaries, which can answer a status for anything. A typed guess is +# therefore admissible only for a status with ONE meaning across every +# documented surface. 409 is not one: the contract itself spells it both +# `hash_mismatch` (POST /assets) and `asset_in_use` (DELETE /assets/{id}). + + +def test_a_bucketless_409_is_not_guessed_to_be_a_hash_mismatch() -> None: + err = error_from_envelope(409, None) + assert type(err) is ApiError + assert not isinstance(err, HashMismatch) + assert err.code == "error" + assert err.http_status == 409 + # `HashMismatch` tells the caller to re-upload bytes, which is why this + # status cannot be guessed: it is a distinct action, not a vaguer wording + # of the same one. + sdk_err = to_sdk_error(err) + assert type(sdk_err) is ComfyError + assert not isinstance(sdk_err, SdkHashMismatch) + assert sdk_err.http_status == 409 + + +def test_a_bucketless_409_still_carries_the_pace_the_server_named() -> None: + # Dropping the code must not drop the header: a conflict that named a + # `Retry-After` is still telling the caller when to ask again. + err = error_from_envelope(409, None, retry_after=7) + assert err.retry_after == 7 + assert to_sdk_error(err).retry_after == 7 + + +def test_an_enveloped_409_is_still_a_hash_mismatch() -> None: + # The assets path is unaffected: a real hash mismatch always arrives + # enveloped, and `error.code` wins outright. + err = error_from_envelope(409, {"error": {"code": "hash_mismatch", "message": "m"}}) + assert type(err) is HashMismatch + assert err.code == "hash_mismatch" + assert type(to_sdk_error(err)) is SdkHashMismatch + + +def test_a_bodyless_401_still_maps_to_unauthorized() -> None: + # Only 409 was dropped -- the rest of the table decodes exactly as before. + err = error_from_envelope(401, None) + assert err.code == "unauthorized" + assert isinstance(err, Unauthorized) + + def test_a_router_validation_body_degrades_rather_than_coercing_its_detail() -> None: # Router's per-field validation body is the FastAPI `detail[]` shape. A # list is not a message: stringifying it would put a Python repr in front of