Skip to content

PaymentOrchestrator mints a new idempotency key per call and cannot accept one — two identical calls are two payments, while the response reports a key as if protected (service.py:47) #72

Description

@aurumflux20

Summary

PaymentOrchestrator.pay() mints a fresh idempotency key on every invocation and has no way to accept one from the caller, so two identical calls are two payments — while the response reports a key, which makes it read as though idempotency were in force.

# mcp_server/app/payments/service.py:47
idempotency_key = str(uuid.uuid4())
...
payment = await self.client.execute_payment(..., idempotency_key=idempotency_key)
return {"status": "success", "idempotency_key": idempotency_key, "payment": payment}

The schema it validates against has no key field:

# service.py:16-22
class PaymentRequest(BaseModel):
    wallet_id: str
    recipient: str
    amount: str

pay() takes request_data: dict, constructs PaymentRequest(**request_data), and Pydantic drops anything not declared. So an idempotency_key supplied by a caller is discarded silently — no error, no warning — and replaced at line 47.

Why the shape of this matters more than a missing key

An absent idempotency key is a gap a reviewer can see. This is worse in one specific way: the key exists, is sent downstream to execute_payment, and is echoed back in the success payload. Every log line and every response shows an idempotency key present and correct. It simply differs on every attempt, so it protects nothing across them — which is the one job it has.

This matters at the retry boundary. mcp_server/app/mcp/fastmcp_server.py:217 defaults the execution strategy to "retry_then_fail", and the same file exposes an idempotency_key parameter to MCP callers at lines 206 and 298. So the surface tells a caller that (a) retries are the default behaviour and (b) they may supply a key to make those retries safe. Anything reaching this orchestrator gets neither.

The simulate-then-execute sequence does not close it either: simulate_payment succeeding says nothing about whether a previous execute_payment already landed.

What I checked

  • service.py:47 — unconditional uuid.uuid4(), not a fallback for a missing value.
  • service.py:16-32PaymentRequest declares three fields; no key, and no model_config permitting extras, so a caller-supplied key is dropped rather than rejected.
  • service.py:74 — the minted key is returned as idempotency_key in the success response.
  • fastmcp_server.py:206, 217, 298 — MCP tools accept an optional idempotency_key and default execution_strategy to retry_then_fail.

Limits, stated plainly

This is a code read. I have not run the server and observed two payments from two identical calls, which is the experiment that turns a mechanism into a measured rate.

The class is docstringed "Legacy compatibility orchestrator", so it may be superseded by a path I have not traced, and the newer path may handle this correctly — in which case the honest finding is narrower: a live legacy entry point that silently discards a caller's idempotency key. I could not determine from the repository what still routes here, and I would rather say that than assume the worse reading. If it is dead code, saying so publicly is a perfectly good outcome and I will record it.

If execute_payment deduplicates on (wallet_id, recipient, amount) independently of the key, this reduces to a documentation gap and I will publish that correction as prominently as the claim.

On the fix

The fix is contained — it does not require restructuring the orchestrator. I am not posting the patch.

That is a deliberate change and I would rather be straight about it: I have filed seven of these in three weeks, each with the full remedy and a failing test attached, free. Seven teams shipped fixes. It built a real public record and it has not been a business. So the finding is free and the fix is the work now.

If useful: I read one money path end to end and return every finding tied to your own file and line numbers, each with its patch and a failing test in your own harness. Five working days, written only, no call. $1,200, no invoice if the path is clean. The deliverable is specified up front: https://github.com/aurumflux20/seal/blob/main/docs/REVIEW-DELIVERABLE.md

If you would rather just have it, say so on this thread and I will post the fix here for nothing — I will not withhold a payment-safety fix on a public repo. But it is the thing I sell, and asking costs you nothing.

Context: of ten agent-payment money paths read in three days, seven could charge a payer twice on an ambiguous outcome. Public record, evidence per row, nobody named while their finding is open: https://aurumflux.co/retry-safety/

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions