From b4632db79c67fe2c5eee832354f2d079660f612d Mon Sep 17 00:00:00 2001 From: redshift <213178690+1ftredsh@users.noreply.github.com> Date: Sun, 4 Oct 2026 15:29:21 +0800 Subject: [PATCH] fix(refund): give refund refusals machine-readable error codes /v1/wallet/refund answered three distinct refusals with a bare `detail` string: "No balance to refund", "Balance too small to refund" and "Cannot refund key. There are ongoing requests for this api key." They are indistinguishable to a client, and the consequences differ: the first proves the key holds nothing and its stored copy can be dropped, the second is dust no retry can pay out, and the third is a transient race whose balance is still on the key and must be kept. The SDK tried to tell them apart by matching the whole error string, but the refund error it compares against is built as "API key refund failed: ", so its no-balance branch never matched. Every dead key stayed in storage and was re-swept every five minutes, forever. All three now carry the structured envelope the other refund errors already use ({"error": {"type", "code", "message"}}), with codes `no_balance_to_refund`, `balance_too_small_to_refund` and `refund_ongoing_requests`. Messages and status codes are unchanged, so message-matching clients (including SDK <= 0.4.8) behave exactly as before. --- routstr/balance.py | 14 +++-- routstr/refund.py | 32 ++++++++++++ .../integration/test_database_consistency.py | 4 +- tests/integration/test_wallet_refund.py | 9 +++- tests/unit/test_refund_failure_error.py | 51 +++++++++++++++++++ tests/unit/test_stale_reservations.py | 4 +- 6 files changed, 105 insertions(+), 9 deletions(-) create mode 100644 tests/unit/test_refund_failure_error.py diff --git a/routstr/balance.py b/routstr/balance.py index f887f5ae..33760ceb 100644 --- a/routstr/balance.py +++ b/routstr/balance.py @@ -384,9 +384,9 @@ async def refund_wallet_endpoint( ) await session.refresh(key) if key.reserved_balance > 0: - raise HTTPException( - status_code=400, - detail="Cannot refund key. There are ongoing requests for this api key.", + raise refund.refund_failure_error( + "Cannot refund key. There are ongoing requests for this api key.", + refund.REFUND_ONGOING_REQUESTS, ) logger.warning( "refund_wallet_endpoint: released stale reservation before refund", @@ -401,9 +401,13 @@ async def refund_wallet_endpoint( remaining_balance = refund.amount_in_unit(remaining_balance_msats, unit) if remaining_balance_msats > 0 and remaining_balance <= 0: - raise HTTPException(status_code=400, detail="Balance too small to refund") + raise refund.refund_failure_error( + "Balance too small to refund", refund.REFUND_BALANCE_TOO_SMALL + ) elif remaining_balance <= 0: - raise HTTPException(status_code=400, detail="No balance to refund") + raise refund.refund_failure_error( + "No balance to refund", refund.REFUND_NO_BALANCE + ) requested = refund_request.lightning_address if refund_request else None destination = requested or key.refund_address diff --git a/routstr/refund.py b/routstr/refund.py index 2aee20e8..babe146d 100644 --- a/routstr/refund.py +++ b/routstr/refund.py @@ -252,6 +252,38 @@ async def hold( await session.commit() +# Machine-readable reasons a refund cannot proceed. Clients act on these +# codes, so they must not be reworded: `no_balance_to_refund` proves the key +# holds nothing and its local copy can be dropped, while +# `refund_ongoing_requests` is a transient race whose balance is still there. +REFUND_NO_BALANCE = "no_balance_to_refund" +REFUND_BALANCE_TOO_SMALL = "balance_too_small_to_refund" +REFUND_ONGOING_REQUESTS = "refund_ongoing_requests" + + +def refund_failure_error( + message: str, code: str, status_code: int = 400 +) -> HTTPException: + """A refund refusal carrying a structured `code`, not a bare `detail` string. + + These used to be plain-string 400s, which are indistinguishable to a + client: the SDK could not tell "No balance to refund" (the key is dead, + drop it) from "Cannot refund key. There are ongoing requests for this api + key." (a transient race, keep the key). It kept every key and re-swept it + forever, re-reading the same 400 every five minutes. + """ + return HTTPException( + status_code=status_code, + detail={ + "error": { + "message": message, + "type": "invalid_request_error", + "code": code, + } + }, + ) + + def refund_in_progress_error(refund: Refund | None = None) -> HTTPException: """The 409 raised when a key already has an unresolved refund claim.""" stuck = refund is not None and refund.status == "stuck" diff --git a/tests/integration/test_database_consistency.py b/tests/integration/test_database_consistency.py index 5c2bbe68..3ab9aa19 100644 --- a/tests/integration/test_database_consistency.py +++ b/tests/integration/test_database_consistency.py @@ -409,7 +409,9 @@ class TestDataIntegrity: # Should fail assert response.status_code == 400 - assert "Balance too small to refund" in response.json()["detail"] + error = response.json()["detail"]["error"] + assert error["code"] == "balance_too_small_to_refund" + assert "Balance too small to refund" in error["message"] # Verify balance unchanged await integration_session.refresh(api_key) diff --git a/tests/integration/test_wallet_refund.py b/tests/integration/test_wallet_refund.py index 1029d073..7aeacf36 100644 --- a/tests/integration/test_wallet_refund.py +++ b/tests/integration/test_wallet_refund.py @@ -134,7 +134,10 @@ async def test_zero_balance_refund_handling( response = await integration_client.post("/v1/wallet/refund") assert response.status_code == 400 - assert response.json()["detail"] == "No balance to refund" + error = response.json()["detail"]["error"] + assert error["code"] == "no_balance_to_refund" + assert error["message"] == "No balance to refund" + assert error["type"] == "invalid_request_error" # Key should still exist result = await integration_session.execute( @@ -585,7 +588,9 @@ async def test_refund_error_handling( response = await integration_client.post("/v1/wallet/refund") assert response.status_code == 400 - assert response.json()["detail"] == "No balance to refund" + error = response.json()["detail"]["error"] + assert error["code"] == "no_balance_to_refund" + assert error["message"] == "No balance to refund" @pytest.mark.integration diff --git a/tests/unit/test_refund_failure_error.py b/tests/unit/test_refund_failure_error.py new file mode 100644 index 00000000..357f0d99 --- /dev/null +++ b/tests/unit/test_refund_failure_error.py @@ -0,0 +1,51 @@ +"""The refund endpoint's refusal reasons must stay machine-readable. + +Clients act on these codes. ``no_balance_to_refund`` proves the key holds +nothing and its stored copy can be dropped; ``refund_ongoing_requests`` is a +transient race whose balance is still on the key and must be kept; +``balance_too_small_to_refund`` is dust that no retry can fix. They used to be +bare ``detail`` strings, which are indistinguishable to a client, so the SDK +kept every dead key and re-swept it forever. +""" + +import pytest +from fastapi import HTTPException + +from routstr import refund + + +@pytest.mark.parametrize( + ("code", "message"), + [ + (refund.REFUND_NO_BALANCE, "No balance to refund"), + (refund.REFUND_BALANCE_TOO_SMALL, "Balance too small to refund"), + ( + refund.REFUND_ONGOING_REQUESTS, + "Cannot refund key. There are ongoing requests for this api key.", + ), + ], +) +def test_refund_failure_error_carries_its_code(code: str, message: str) -> None: + error = refund.refund_failure_error(message, code) + + assert isinstance(error, HTTPException) + assert error.status_code == 400 + assert error.detail == { + "error": { + "message": message, + "type": "invalid_request_error", + "code": code, + } + } + + +def test_refund_failure_codes_are_distinct_lowercase_slugs() -> None: + codes = { + refund.REFUND_NO_BALANCE, + refund.REFUND_BALANCE_TOO_SMALL, + refund.REFUND_ONGOING_REQUESTS, + } + + assert len(codes) == 3 + assert all(code == code.lower() for code in codes) + assert all(code.strip() == code for code in codes) diff --git a/tests/unit/test_stale_reservations.py b/tests/unit/test_stale_reservations.py index 61a3da5a..eedffada 100644 --- a/tests/unit/test_stale_reservations.py +++ b/tests/unit/test_stale_reservations.py @@ -377,7 +377,9 @@ async def test_refund_rejects_recent_reservation(session: AsyncSession) -> None: ) assert exc_info.value.status_code == 400 - assert "ongoing requests" in exc_info.value.detail + error = exc_info.value.detail["error"] + assert error["code"] == "refund_ongoing_requests" + assert "ongoing requests" in error["message"] @pytest.mark.asyncio