fix(payments): add local idempotency gate for Lightning recording
The Fava-side duplicate checks (add_entry_idempotent, journal-link scan) are read-then-write races: on restart with a persisted invoice queue, or webhook + poller firing together, both callers pass the "not present" check and both insert. New processed_payments table (m005) keyed on payment_hash; exactly one claimant wins the INSERT ... ON CONFLICT DO NOTHING. Lifecycle: 'processing' while the write is in flight, 'done' after; failed recordings release the claim so redelivery retries, and 'processing' rows from a crashed process are cleared at listener startup. Also wraps the invoice-listener loop body in try/except so one poison payment can't kill payment recording for the process lifetime. Addresses CODE-REVIEW-2026-06 findings #4 and #9. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
parent
c50455d5f6
commit
4fdb358bb0
4 changed files with 137 additions and 1 deletions
70
crud.py
70
crud.py
|
|
@ -1696,3 +1696,73 @@ async def check_user_has_role_permission(
|
||||||
return True
|
return True
|
||||||
|
|
||||||
return False
|
return False
|
||||||
|
|
||||||
|
|
||||||
|
# =============================================================================
|
||||||
|
# PROCESSED PAYMENTS (Lightning payment idempotency gate)
|
||||||
|
# =============================================================================
|
||||||
|
# The Fava-side duplicate checks are read-then-write races; this table's
|
||||||
|
# primary key on payment_hash makes exactly one claimant win. Shared by the
|
||||||
|
# background invoice listener (tasks.on_invoice_paid) and the client-driven
|
||||||
|
# /record-payment endpoint.
|
||||||
|
|
||||||
|
|
||||||
|
async def claim_payment(payment_hash: str) -> bool:
|
||||||
|
"""Atomically claim a Lightning payment for recording.
|
||||||
|
|
||||||
|
Returns True when this caller owns the claim; False when the payment
|
||||||
|
is already recorded or another coroutine is recording it right now.
|
||||||
|
"""
|
||||||
|
result = await db.execute(
|
||||||
|
"""
|
||||||
|
INSERT INTO processed_payments (payment_hash, status)
|
||||||
|
VALUES (:payment_hash, 'processing')
|
||||||
|
ON CONFLICT (payment_hash) DO NOTHING
|
||||||
|
""",
|
||||||
|
{"payment_hash": payment_hash},
|
||||||
|
)
|
||||||
|
return result.rowcount == 1
|
||||||
|
|
||||||
|
|
||||||
|
async def get_processed_payment(payment_hash: str) -> Optional[dict]:
|
||||||
|
row = await db.fetchone(
|
||||||
|
"SELECT payment_hash, status, entry_id FROM processed_payments"
|
||||||
|
" WHERE payment_hash = :payment_hash",
|
||||||
|
{"payment_hash": payment_hash},
|
||||||
|
)
|
||||||
|
return dict(row) if row else None
|
||||||
|
|
||||||
|
|
||||||
|
async def mark_payment_done(payment_hash: str, entry_id: Optional[str] = None) -> None:
|
||||||
|
await db.execute(
|
||||||
|
"""
|
||||||
|
UPDATE processed_payments SET status = 'done', entry_id = :entry_id
|
||||||
|
WHERE payment_hash = :payment_hash
|
||||||
|
""",
|
||||||
|
{"payment_hash": payment_hash, "entry_id": entry_id},
|
||||||
|
)
|
||||||
|
|
||||||
|
|
||||||
|
async def release_payment_claim(payment_hash: str) -> None:
|
||||||
|
"""Compensating delete after a failed recording, so redelivery retries.
|
||||||
|
|
||||||
|
Only removes an in-flight claim — a 'done' row is permanent.
|
||||||
|
"""
|
||||||
|
await db.execute(
|
||||||
|
"DELETE FROM processed_payments"
|
||||||
|
" WHERE payment_hash = :payment_hash AND status = 'processing'",
|
||||||
|
{"payment_hash": payment_hash},
|
||||||
|
)
|
||||||
|
|
||||||
|
|
||||||
|
async def clear_stale_payment_claims() -> int:
|
||||||
|
"""Drop 'processing' claims left behind by a previous process life.
|
||||||
|
|
||||||
|
A live claim only exists inside a running coroutine, so anything
|
||||||
|
still 'processing' at listener startup belongs to a crashed or
|
||||||
|
restarted process and would otherwise block that payment forever.
|
||||||
|
"""
|
||||||
|
result = await db.execute(
|
||||||
|
"DELETE FROM processed_payments WHERE status = 'processing'"
|
||||||
|
)
|
||||||
|
return result.rowcount
|
||||||
|
|
|
||||||
|
|
@ -624,3 +624,30 @@ async def m004_add_rbac_tables(db):
|
||||||
"created_by": "system", # System-created default roles
|
"created_by": "system", # System-created default roles
|
||||||
},
|
},
|
||||||
)
|
)
|
||||||
|
|
||||||
|
|
||||||
|
async def m005_add_processed_payments(db):
|
||||||
|
"""
|
||||||
|
Local idempotency gate for Lightning payment recording.
|
||||||
|
|
||||||
|
The Fava-side duplicate check (`add_entry_idempotent`, journal-link
|
||||||
|
scan) is a read-then-write race: the background invoice listener and
|
||||||
|
the client-driven /record-payment endpoint can both pass the "not
|
||||||
|
present" check for the same payment_hash and both insert. The
|
||||||
|
primary key on payment_hash makes exactly one claimant win.
|
||||||
|
|
||||||
|
status lifecycle: 'processing' (claimed, write in flight) → 'done'
|
||||||
|
(entry recorded). Failed claims are deleted so redelivery retries;
|
||||||
|
'processing' rows from a crashed process are cleared at listener
|
||||||
|
startup.
|
||||||
|
"""
|
||||||
|
await db.execute(
|
||||||
|
f"""
|
||||||
|
CREATE TABLE IF NOT EXISTS processed_payments (
|
||||||
|
payment_hash TEXT PRIMARY KEY,
|
||||||
|
status TEXT NOT NULL DEFAULT 'processing',
|
||||||
|
entry_id TEXT,
|
||||||
|
created_at TIMESTAMP NOT NULL DEFAULT {db.timestamp_now}
|
||||||
|
);
|
||||||
|
"""
|
||||||
|
)
|
||||||
|
|
|
||||||
40
tasks.py
40
tasks.py
|
|
@ -179,12 +179,31 @@ async def wait_for_paid_invoices():
|
||||||
This ensures payments are recorded even if the user closes their browser
|
This ensures payments are recorded even if the user closes their browser
|
||||||
before the payment is detected by client-side polling.
|
before the payment is detected by client-side polling.
|
||||||
"""
|
"""
|
||||||
|
from .crud import clear_stale_payment_claims
|
||||||
|
|
||||||
invoice_queue = Queue()
|
invoice_queue = Queue()
|
||||||
register_invoice_listener(invoice_queue, "ext_libra")
|
register_invoice_listener(invoice_queue, "ext_libra")
|
||||||
|
|
||||||
|
# Claims from a previous process life can't be live anymore — clear
|
||||||
|
# them so those payments aren't blocked forever.
|
||||||
|
cleared = await clear_stale_payment_claims()
|
||||||
|
if cleared:
|
||||||
|
logger.warning(
|
||||||
|
f"[LIBRA] Cleared {cleared} stale in-flight payment claim(s) "
|
||||||
|
"from a previous run"
|
||||||
|
)
|
||||||
|
|
||||||
while True:
|
while True:
|
||||||
payment = await invoice_queue.get()
|
payment = await invoice_queue.get()
|
||||||
await on_invoice_paid(payment)
|
try:
|
||||||
|
await on_invoice_paid(payment)
|
||||||
|
except Exception:
|
||||||
|
# One bad payment must not kill the listener for the rest of
|
||||||
|
# the process lifetime; its claim was released, so redelivery
|
||||||
|
# can retry it.
|
||||||
|
logger.exception(
|
||||||
|
f"[LIBRA] Failed to record payment {payment.payment_hash}"
|
||||||
|
)
|
||||||
|
|
||||||
|
|
||||||
async def on_invoice_paid(payment: Payment) -> None:
|
async def on_invoice_paid(payment: Payment) -> None:
|
||||||
|
|
@ -210,8 +229,20 @@ async def on_invoice_paid(payment: Payment) -> None:
|
||||||
logger.warning(f"Libra invoice {payment.payment_hash} missing user_id in metadata")
|
logger.warning(f"Libra invoice {payment.payment_hash} missing user_id in metadata")
|
||||||
return
|
return
|
||||||
|
|
||||||
|
from .crud import claim_payment, mark_payment_done, release_payment_claim
|
||||||
from .fava_client import get_fava_client
|
from .fava_client import get_fava_client
|
||||||
|
|
||||||
|
# Local idempotency gate: exactly one claimant (this listener or the
|
||||||
|
# /record-payment endpoint) gets to record a given payment_hash. The
|
||||||
|
# Fava-side idempotent write below stays as a second layer for
|
||||||
|
# entries recorded before this table existed.
|
||||||
|
if not await claim_payment(payment.payment_hash):
|
||||||
|
logger.info(
|
||||||
|
f"Payment {payment.payment_hash} already recorded or being "
|
||||||
|
"recorded; skipping"
|
||||||
|
)
|
||||||
|
return
|
||||||
|
|
||||||
fava = get_fava_client()
|
fava = get_fava_client()
|
||||||
|
|
||||||
# Use idempotency key based on payment hash - this ensures duplicate
|
# Use idempotency key based on payment hash - this ensures duplicate
|
||||||
|
|
@ -245,6 +276,7 @@ async def on_invoice_paid(payment: Payment) -> None:
|
||||||
|
|
||||||
if not fiat_currency or not fiat_amount:
|
if not fiat_currency or not fiat_amount:
|
||||||
logger.error(f"Payment {payment.payment_hash} missing fiat currency/amount metadata")
|
logger.error(f"Payment {payment.payment_hash} missing fiat currency/amount metadata")
|
||||||
|
await release_payment_claim(payment.payment_hash)
|
||||||
return
|
return
|
||||||
|
|
||||||
# Get user's current balance to determine receivables and payables
|
# Get user's current balance to determine receivables and payables
|
||||||
|
|
@ -276,6 +308,7 @@ async def on_invoice_paid(payment: Payment) -> None:
|
||||||
lightning_account = await get_account_by_name("Assets:Bitcoin:Lightning")
|
lightning_account = await get_account_by_name("Assets:Bitcoin:Lightning")
|
||||||
if not lightning_account:
|
if not lightning_account:
|
||||||
logger.error("Lightning account 'Assets:Bitcoin:Lightning' not found")
|
logger.error("Lightning account 'Assets:Bitcoin:Lightning' not found")
|
||||||
|
await release_payment_claim(payment.payment_hash)
|
||||||
return
|
return
|
||||||
|
|
||||||
# Query for unsettled entries to link this settlement back to them
|
# Query for unsettled entries to link this settlement back to them
|
||||||
|
|
@ -324,6 +357,11 @@ async def on_invoice_paid(payment: Payment) -> None:
|
||||||
f"{result.get('data', 'Unknown')}"
|
f"{result.get('data', 'Unknown')}"
|
||||||
)
|
)
|
||||||
|
|
||||||
|
await mark_payment_done(payment.payment_hash, idempotency_key)
|
||||||
|
|
||||||
except Exception as e:
|
except Exception as e:
|
||||||
logger.error(f"Error recording Libra payment {payment.payment_hash}: {e}")
|
logger.error(f"Error recording Libra payment {payment.payment_hash}: {e}")
|
||||||
|
# Release the claim so redelivery (or the /record-payment
|
||||||
|
# endpoint) can retry this payment.
|
||||||
|
await release_payment_claim(payment.payment_hash)
|
||||||
raise
|
raise
|
||||||
|
|
|
||||||
|
|
@ -55,6 +55,7 @@ EXPECTED_TABLES = [
|
||||||
"roles",
|
"roles",
|
||||||
"role_permissions",
|
"role_permissions",
|
||||||
"user_roles",
|
"user_roles",
|
||||||
|
"processed_payments",
|
||||||
]
|
]
|
||||||
|
|
||||||
|
|
||||||
|
|
|
||||||
Loading…
Add table
Add a link
Reference in a new issue