[PR #151] feat(errors): add a typed AssetInUse so a delete conflict is catchable #149

New issue

Open

opened 2026-10-31 22:01:49 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-31 22:01:49 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/Comfy-Org/comfy-python-sdk/pull/151
Author: @mattmillerai
Created: 9/14/2026
Status: 🔄 Open

Base: main ← Head: matt/be-14168-typed-asset-in-use

📝 Commits (4)

7841863 feat(errors): add a typed AssetInUse so a delete conflict is catchable
c59e375 Merge origin/main into matt/be-14168-typed-asset-in-use
df7c0ef Merge origin/main into matt/be-14168-typed-asset-in-use
870ffb4 test(router): reconcile the credits_used header-lift tripwire

📊 Changes

11 files changed (+180 additions, -10 deletions)

View changed files

📝 CHANGELOG.md (+9 -0)
📝 README.md (+10 -0)
📝 src/comfy_low/__init__.py (+2 -0)
📝 src/comfy_low/errors.py (+14 -0)
📝 src/comfy_sdk/__init__.py (+2 -0)
📝 src/comfy_sdk/exceptions.py (+14 -0)
📝 tests/conftest.py (+5 -0)
📝 tests/test_assets.py (+44 -1)
📝 tests/test_error_mapping.py (+47 -0)
📝 tests/test_exception_modules.py (+18 -0)
📝 tests/test_router_spec_contract.py (+15 -9)

📄 Description

ELI-5

The API says "no" in two different ways when it returns a 409. Uploading bytes that don't match their declared hash is one (hash_mismatch); deleting an asset something still depends on is the other (asset_in_use). Only the first had a name you could except on, so a caller who wanted to handle just the delete case had to write if exc.code == "asset_in_use" — reaching past the typed errors into the wire protocol. This gives the second one a name too.

What changed

comfy_low.errors.AssetInUse(ApiError) with code = "asset_in_use", registered in that module's _BY_CODE beside HashMismatch.
comfy_sdk.exceptions.AssetInUse(ComfyError) with the matching _BY_CODE entry, exported from comfy_sdk (and AssetInUse from comfy_low) the way every other typed error is.
Tests at the mapping level on both layers, plus end-to-end through the stub server's DELETE /api/v2/assets/{id} route (sync + async), and a regression assert that hash_mismatch still maps to HashMismatch and the two classes are siblings rather than one subclassing the other.
CHANGELOG.md under [Unreleased] → Added — the release flow is tag-driven and pyproject.toml's version is a placeholder, so an already-tagged heading would have been wrong.
README: a bullet in the typed-errors list and a paragraph in the asset-deletion section.

Judgment calls

Registering asset_in_use in comfy_sdk's _BY_CODE is the one line with blast radius, because that table is also a filter. _router_only_class() returns None for any code already in _BY_CODE, so adding a key removes that code from Router-bucket retyping. I checked before adding it: asset_in_use appears nowhere in router_exceptions.py or spec/router-openapi.yaml — it is a v2-envelope code only — so nothing was shadowed. The router-spec contract check (scripts/check_drift.py) still reports all 15 buckets covered.

The two 409s are siblings, not a hierarchy. WorkflowFormatUi subclasses InvalidWorkflow because it is a narrower case of the same failure; a delete conflict and an upload hash conflict are unrelated failures that only share a status, so AssetInUse extends the base directly. A test pins that both ways, since making one catch the other later would be a silent behaviour change for existing except HashMismatch handlers.

I added a reject_delete_in_use flag to the stub server rather than mocking httpx, per the repo's rule that the suite drives server.state and has no network dependency.

No capability is being denied by this diff, so the falsification discipline does not apply: it adds a class and a _BY_CODE row, introduces no deny/dead-end path, no "not supported"/"unavailable" string, and flips no test to assert something is impossible. The only "you can't" prose is the README's note that an immediate retry does not clear a platform hold, which restates the spec's own description of the status rather than asserting a missing feature — and it is written so the retry-later path stays visible.

Verification against what the ticket names

Every artifact the ticket points at, exercised read-only:

spec/openapi.yaml — read both 409s: hash_mismatch on POST /api/v2/assets (line 117) and asset_in_use on DELETE /api/v2/assets/{id} (line 278), whose description is the source for "the body deliberately never says which hold applies".
src/comfy_low/errors.py _BY_CODE and src/comfy_sdk/exceptions.py _BY_CODE — both confirmed to have lacked asset_in_use, both now carry it.
The enveloped 409 asset_in_use on both layers — the stated acceptance criterion, covered by test_an_enveloped_delete_conflict_is_typed_at_the_protocol_layer / ..._at_the_sdk_layer plus three end-to-end tests through the real assets.delete() / Asset.delete() / async paths.
PR #132 — was open when this branch was cut; it has since merged and is now in this branch via the main merge. The two changes stayed independent as expected (#132 touches _CODE_BY_STATUS, this touches _BY_CODE), and together they close both halves of the asset_in_use story — see Residual. Based on main, not stacked.
pyproject.toml — confirmed the version is the placeholder the publish workflow overwrites, which is why the changelog entry is under [Unreleased].

Provenance

Authored by: agent-work loop
Verified: re-run on the merged tree (main merged in at c59e375): ruff check .: all checks passed; ruff format --check .: 57 files already formatted; mypy src: no issues in 21 source files; pytest: 982 passed, 9 skipped (skips are the network-gated integration tests, skipped on main too); scripts/check_drift.py: models in sync, all 18 router error types covered, model-run URL as declared; scripts/check_public_repo_hygiene.py: no internal-only references.
Deviations: none — all four stated criteria are met. The README bullets are an addition beyond the stated list, on the reading that "export it from the package surface the way the other typed errors are" includes the place the README lists every other typed error. Two changes beyond the original scope came from the review round: the AssetInUse docstring and the test_error_mapping.py comment now spell the route DELETE /api/v2/assets/{id} (panel nit — the tree reserves bare /v2/... for the Router surface), and AssetInUse was added to comfy_sdk.exceptions.__all__, which the first pass missed while updating both packages' __all__; a guard test now asserts every class in _BY_CODE is in __all__.

Residual

~~A 409 that carries no envelope code is still mistyped as HashMismatch, on delete as well as upload.~~ Closed — #132 has merged and is in this branch. When this was written, _CODE_BY_STATUS still mapped 409 → "hash_mismatch", so a bare 409 from DELETE /api/v2/assets/{id} — one from an intermediary, or a surface answering the status without the envelope — reached the caller as HashMismatch. That row was left untouched here because removing it was the whole subject of the then-open #132, and changing it in two places at once would have conflicted.

#132 has since merged and arrives in this branch with the main merge: it dropped 409 from _CODE_BY_STATUS outright, so a code-less 409 now decodes to http_409 and surfaces as a plain ComfyError carrying the real http_status, the response's message and any Retry-After. The two changes compose as intended — except AssetInUse is reliable for enveloped delete conflicts, and a code-less 409 no longer claims to be an upload failure instead. The cursor-review panel raised this same gap against this PR (Medium); it is resolved by the merge rather than by a change on this branch.

Three of the twelve error codes the v2 spec names on a 4xx/5xx response still have no typed class at either layer, which is the same defect this ticket describes, one artifact over. I ran the same sweep over the remainder rather than only over the code I fixed: of the 12 codes spec/openapi.yaml names on an error response, 9 are now typed at both layers, and these 3 are not:

too_many_streams (GET /jobs/{id}/events, line 579) — the closest sibling of this ticket. It reaches a caller as a bare ComfyError and the correct handling (close a stream, or fall back to polling) is exactly the kind of decision a typed class should carry.
deployment_not_ready (429 on deployment-scoped surfaces, line 447) — less acute, because client.py's submit path already retries 429 + Retry-After generically, so it is usually absorbed before a caller sees it. It is still indistinguishable from queue_full by class.
not_implemented (501 from the events endpoint, line 588) — mostly absorbed too: jobs.py catches http_status == 501 and falls back to polling. A caller reaching a raw transport call still gets a bare error.

None of these were in scope here and none are caused by this change; they are named because the sweep that sized this fix could see them.

Nothing was exercised against a live deployment. Verification is against the repo's stdlib stub server, which is the repo's own mandated approach (AGENTS.md: the suite has no network dependency, drive server.state rather than mocking httpx). tests/integration/test_gateway_e2e.py is gated on COMFY_BASE_URL + COMFY_API_KEY and skipped in this run, so no real 409 asset_in_use from a real surface was observed — the wire shape is taken from spec/openapi.yaml as the contract of record. If a deployment's delete conflict turns out not to carry error.code in the envelope, the code-less gap above is what it would hit.

Summary by CodeRabbit

New Features
- Added a typed AssetInUse error for asset deletion requests refused because the platform still depends on the asset.
- The error is available through both SDK interfaces and clearly distinguishes this conflict from hash mismatches.
Documentation
- Documented the deletion behavior: the asset remains available, its handle stays reusable, and no re-upload is required after the refusal.

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/Comfy-Org/comfy-python-sdk/pull/151 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 9/14/2026 **Status:** 🔄 Open **Base:** `main` ← **Head:** `matt/be-14168-typed-asset-in-use` --- ### 📝 Commits (4) - [`7841863`](https://github.com/Comfy-Org/comfy-python-sdk/commit/7841863f048745de4f7efd88f18a7e87f957153e) feat(errors): add a typed AssetInUse so a delete conflict is catchable - [`c59e375`](https://github.com/Comfy-Org/comfy-python-sdk/commit/c59e375fd5fc7a3ff05a55166f5fb386e6d99c9f) Merge origin/main into matt/be-14168-typed-asset-in-use - [`df7c0ef`](https://github.com/Comfy-Org/comfy-python-sdk/commit/df7c0ef2029ac531a52a3b0cbd5c0cf43ec278fa) Merge origin/main into matt/be-14168-typed-asset-in-use - [`870ffb4`](https://github.com/Comfy-Org/comfy-python-sdk/commit/870ffb43d98fcda3db2ad992498759b2034b6f48) test(router): reconcile the credits_used header-lift tripwire ### 📊 Changes **11 files changed** (+180 additions, -10 deletions) <details> <summary>View changed files</summary> 📝 `CHANGELOG.md` (+9 -0) 📝 `README.md` (+10 -0) 📝 `src/comfy_low/__init__.py` (+2 -0) 📝 `src/comfy_low/errors.py` (+14 -0) 📝 `src/comfy_sdk/__init__.py` (+2 -0) 📝 `src/comfy_sdk/exceptions.py` (+14 -0) 📝 `tests/conftest.py` (+5 -0) 📝 `tests/test_assets.py` (+44 -1) 📝 `tests/test_error_mapping.py` (+47 -0) 📝 `tests/test_exception_modules.py` (+18 -0) 📝 `tests/test_router_spec_contract.py` (+15 -9) </details> ### 📄 Description ## ELI-5 The API says "no" in two different ways when it returns a `409`. Uploading bytes that don't match their declared hash is one (`hash_mismatch`); deleting an asset something still depends on is the other (`asset_in_use`). Only the first had a name you could `except` on, so a caller who wanted to handle just the delete case had to write `if exc.code == "asset_in_use"` — reaching past the typed errors into the wire protocol. This gives the second one a name too. ## What changed - `comfy_low.errors.AssetInUse(ApiError)` with `code = "asset_in_use"`, registered in that module's `_BY_CODE` beside `HashMismatch`. - `comfy_sdk.exceptions.AssetInUse(ComfyError)` with the matching `_BY_CODE` entry, exported from `comfy_sdk` (and `AssetInUse` from `comfy_low`) the way every other typed error is. - Tests at the mapping level on both layers, plus end-to-end through the stub server's `DELETE /api/v2/assets/{id}` route (sync + async), and a regression assert that `hash_mismatch` still maps to `HashMismatch` and the two classes are siblings rather than one subclassing the other. - `CHANGELOG.md` under `[Unreleased] → Added` — the release flow is tag-driven and `pyproject.toml`'s version is a placeholder, so an already-tagged heading would have been wrong. - README: a bullet in the typed-errors list and a paragraph in the asset-deletion section. ## Judgment calls **Registering `asset_in_use` in `comfy_sdk`'s `_BY_CODE` is the one line with blast radius, because that table is also a filter.** `_router_only_class()` returns `None` for any code already in `_BY_CODE`, so adding a key removes that code from Router-bucket retyping. I checked before adding it: `asset_in_use` appears nowhere in `router_exceptions.py` or `spec/router-openapi.yaml` — it is a v2-envelope code only — so nothing was shadowed. The router-spec contract check (`scripts/check_drift.py`) still reports all 15 buckets covered. **The two 409s are siblings, not a hierarchy.** `WorkflowFormatUi` subclasses `InvalidWorkflow` because it is a narrower case of the same failure; a delete conflict and an upload hash conflict are unrelated failures that only share a status, so `AssetInUse` extends the base directly. A test pins that both ways, since making one catch the other later would be a silent behaviour change for existing `except HashMismatch` handlers. **I added a `reject_delete_in_use` flag to the stub server rather than mocking httpx**, per the repo's rule that the suite drives `server.state` and has no network dependency. **No capability is being denied by this diff**, so the falsification discipline does not apply: it adds a class and a `_BY_CODE` row, introduces no deny/dead-end path, no "not supported"/"unavailable" string, and flips no test to assert something is impossible. The only "you can't" prose is the README's note that an immediate retry does not clear a platform hold, which restates the spec's own description of the status rather than asserting a missing feature — and it is written so the retry-later path stays visible. ## Verification against what the ticket names Every artifact the ticket points at, exercised read-only: - `spec/openapi.yaml` — read both 409s: `hash_mismatch` on `POST /api/v2/assets` (line 117) and `asset_in_use` on `DELETE /api/v2/assets/{id}` (line 278), whose description is the source for "the body deliberately never says which hold applies". - `src/comfy_low/errors.py` `_BY_CODE` and `src/comfy_sdk/exceptions.py` `_BY_CODE` — both confirmed to have lacked `asset_in_use`, both now carry it. - The enveloped `409 asset_in_use` on both layers — the stated acceptance criterion, covered by `test_an_enveloped_delete_conflict_is_typed_at_the_protocol_layer` / `..._at_the_sdk_layer` plus three end-to-end tests through the real `assets.delete()` / `Asset.delete()` / async paths. - PR #132 — was open when this branch was cut; it has since merged and is now in this branch via the `main` merge. The two changes stayed independent as expected (#132 touches `_CODE_BY_STATUS`, this touches `_BY_CODE`), and together they close both halves of the `asset_in_use` story — see Residual. Based on `main`, not stacked. - `pyproject.toml` — confirmed the version is the placeholder the publish workflow overwrites, which is why the changelog entry is under `[Unreleased]`. ## Provenance - **Authored by:** agent-work loop - **Verified:** re-run on the merged tree (`main` merged in at `c59e375`): `ruff check .`: all checks passed; `ruff format --check .`: 57 files already formatted; `mypy src`: no issues in 21 source files; `pytest`: 982 passed, 9 skipped (skips are the network-gated integration tests, skipped on `main` too); `scripts/check_drift.py`: models in sync, all 18 router error types covered, model-run URL as declared; `scripts/check_public_repo_hygiene.py`: no internal-only references. - **Deviations:** none — all four stated criteria are met. The README bullets are an addition beyond the stated list, on the reading that "export it from the package surface the way the other typed errors are" includes the place the README lists every other typed error. Two changes beyond the original scope came from the review round: the `AssetInUse` docstring and the `test_error_mapping.py` comment now spell the route `DELETE /api/v2/assets/{id}` (panel nit — the tree reserves bare `/v2/...` for the Router surface), and `AssetInUse` was added to `comfy_sdk.exceptions.__all__`, which the first pass missed while updating both packages' `__all__`; a guard test now asserts every class in `_BY_CODE` is in `__all__`. ## Residual **~~A `409` that carries no envelope code is still mistyped as `HashMismatch`, on delete as well as upload.~~ Closed — #132 has merged and is in this branch.** When this was written, `_CODE_BY_STATUS` still mapped `409 → "hash_mismatch"`, so a bare `409` from `DELETE /api/v2/assets/{id}` — one from an intermediary, or a surface answering the status without the envelope — reached the caller as `HashMismatch`. That row was left untouched here because removing it was the whole subject of the then-open #132, and changing it in two places at once would have conflicted. #132 has since merged and arrives in this branch with the `main` merge: it dropped `409` from `_CODE_BY_STATUS` outright, so a code-less `409` now decodes to `http_409` and surfaces as a plain `ComfyError` carrying the real `http_status`, the response's message and any `Retry-After`. The two changes compose as intended — `except AssetInUse` is reliable for enveloped delete conflicts, and a code-less `409` no longer claims to be an upload failure instead. The cursor-review panel raised this same gap against this PR (Medium); it is resolved by the merge rather than by a change on this branch. **Three of the twelve error codes the v2 spec names on a 4xx/5xx response still have no typed class at either layer**, which is the same defect this ticket describes, one artifact over. I ran the same sweep over the remainder rather than only over the code I fixed: of the 12 codes `spec/openapi.yaml` names on an error response, 9 are now typed at both layers, and these 3 are not: - `too_many_streams` (`GET /jobs/{id}/events`, line 579) — the closest sibling of this ticket. It reaches a caller as a bare `ComfyError` and the correct handling (close a stream, or fall back to polling) is exactly the kind of decision a typed class should carry. - `deployment_not_ready` (429 on deployment-scoped surfaces, line 447) — less acute, because `client.py`'s submit path already retries `429 + Retry-After` generically, so it is usually absorbed before a caller sees it. It is still indistinguishable from `queue_full` by class. - `not_implemented` (501 from the events endpoint, line 588) — mostly absorbed too: `jobs.py` catches `http_status == 501` and falls back to polling. A caller reaching a raw transport call still gets a bare error. None of these were in scope here and none are caused by this change; they are named because the sweep that sized this fix could see them. **Nothing was exercised against a live deployment.** Verification is against the repo's stdlib stub server, which is the repo's own mandated approach (`AGENTS.md`: the suite has no network dependency, drive `server.state` rather than mocking httpx). `tests/integration/test_gateway_e2e.py` is gated on `COMFY_BASE_URL` + `COMFY_API_KEY` and skipped in this run, so no real `409 asset_in_use` from a real surface was observed — the wire shape is taken from `spec/openapi.yaml` as the contract of record. If a deployment's delete conflict turns out not to carry `error.code` in the envelope, the code-less gap above is what it would hit.  ## Summary by CodeRabbit * **New Features** * Added a typed `AssetInUse` error for asset deletion requests refused because the platform still depends on the asset. * The error is available through both SDK interfaces and clearly distinguishes this conflict from hash mismatches. * **Documentation** * Documented the deletion behavior: the asset remains available, its handle stays reusable, and no re-upload is required after the refusal.  --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi added the

pull-request

label

2026-10-31 22:01:49 +00:00

bsakaguchi referenced this issue

2026-10-31 22:01:49 +00:00

[PR #149] [MERGED] chore: sync vendored Comfy Router spec (cloud@f42cd6e) #150