feat(errors): add a typed AssetInUse so a delete conflict is catchable

[mattmillerai]

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 fix(errors): drop 409 from the status->code fallback so a code-less 409 raises ComfyError #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 (fix(errors): drop 409 from the status->code fallback so a code-less 409 raises ComfyError #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.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

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

Review profile: ASSERTIVE

Plan: Team

Run ID: 7cdb7a02-88b0-4925-804e-0967968e745f

📥 Commits

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

📒 Files selected for processing (10)

• CHANGELOG.md
• README.md
• src/comfy_low/__init__.py
• src/comfy_low/errors.py
• src/comfy_sdk/__init__.py
• src/comfy_sdk/exceptions.py
• tests/conftest.py
• tests/test_assets.py
• tests/test_error_mapping.py
• tests/test_exception_modules.py

Included review availability: 0 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 1 review per hour.

---

📝 Walkthrough

Walkthrough

The SDKs add AssetInUse for HTTP 409 asset deletion conflicts. Both SDK layers map and export the exception. Tests cover sync, async, mapping, and retained asset IDs. Documentation describes retry behavior.

Changes

Asset deletion conflict handling

Layer / File(s)	Summary
Exception contract and exports src/comfy_low/errors.py, src/comfy_low/__init__.py, src/comfy_sdk/exceptions.py, src/comfy_sdk/__init__.py	Adds AssetInUse, maps the asset_in_use code, and exports the exception from both SDK layers.
Deletion rejection and validation tests/conftest.py, tests/test_assets.py, tests/test_error_mapping.py, tests/test_exception_modules.py	Simulates HTTP 409 deletion responses and verifies typed exceptions, preserved asset IDs, sync and async behavior, error mapping, and exports.
Behavior documentation README.md, CHANGELOG.md	Documents platform-held asset dependencies, retained handles, and the new typed error.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Feature

Sequence Diagram(s)

sequenceDiagram
  participant AssetDeletion as Asset deletion
  participant TestServer as Test server
  participant ComfyLow as comfy_low error mapper
  participant ComfySDK as comfy_sdk error mapper
  AssetDeletion->>TestServer: DELETE /assets/{id}
  TestServer-->>AssetDeletion: HTTP 409 asset_in_use
  AssetDeletion->>ComfyLow: Map asset_in_use response
  ComfyLow-->>AssetDeletion: AssetInUse
  AssetDeletion->>ComfySDK: Convert transport error
  ComfySDK-->>AssetDeletion: AssetInUse

Loading

Suggested reviewers: wei-hai

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 7.14% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 14 functions across 8 files. (2 skipped: 2…	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main change: adding a typed AssetInUse error for catchable delete conflicts.

Full details: Docstring Coverage

Explanation

Docstring coverage is 7.14% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 14 functions across 8 files. (2 skipped: 2 unsupported.)

• Fix all pre-merge checks with AI

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

• Commit to this branch
• Create a new PR

🧪 Generate unit tests (beta)

• Commit to this branch
• Create a new PR

---

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

[github-actions]

🔍 Cursor Review — Consolidated panel

Triggered by @mattmillerai.

Found 2 finding(s).

Severity	Count
🟡 Medium	1
⚪ Nit	1

Panel: 6/6 reviewers contributed findings.