Skip to content

feat(brokers): give the port an idempotency key so a placement retry is safe - #419

Merged
eaitbrahim merged 1 commit into
mainfrom
feat/port-idempotency-key
Aug 19, 2026
Merged

feat(brokers): give the port an idempotency key so a placement retry is safe#419
eaitbrahim merged 1 commit into
mainfrom
feat/port-idempotency-key

Conversation

@eaitbrahim

Copy link
Copy Markdown
Contributor

Closes #409.

The defect

Every adapter minted a fresh uuid4 client_order_id per place_order call, so no retry was ever deduplicated. A caller retrying after a timeout — exactly when the first request may already have reached the venue — placed a second live order, because the retry carried a different id and the venue had nothing to match it against.

Robinhood and Alpaca documented the hazard. Coinbase's docstring asserted the opposite:

Place a live order. A fresh client_order_id per call gives Coinbase idempotency.

That had it backwards. A per-attempt id is precisely what withholds idempotency — it leaves the venue nothing to deduplicate on.

Why it stayed a comment

The port could not express the difference. place_order(spec) cannot tell a retry of one intent from two orders a strategy genuinely meant to place. So the port now carries it:

def place_order(self, spec: OrderSpec, *, idempotency_key: str | None = None) -> PlaceResult
  • None mints per attempt — the unchanged default. The opposite default is not safer: an id derived from the order would collapse two deliberate orders into one, and a position at half the intended size is as wrong as one at twice it.
  • A key resolves every attempt under it to one venue-facing id.

The derivation, and why it hashes

keel_broker_api.port.resolve_client_order_id owns it, and turns the key into a uuid5 rather than passing it through:

resolve_client_order_id(None)                      # -> a fresh uuid4, one per attempt
resolve_client_order_id("cycle-7:pos-3:exit")      # -> the same uuid5, every time

The venues do not agree on what a client order id may be — Robinhood's is a UUID, Alpaca's a string of up to 128 characters. Hashing lands on the intersection, so a caller uses whatever natural key it has (a cycle id, a position id, a leg name) without knowing which venue the order will be routed to. One derivation in one place: an adapter inventing its own rule would make the same key mean different orders at different venues.

uuid5 is deterministic across processes, which is the case that matters. A retry issued by a new process — what a crash produces — derives the same id as the attempt that crashed. An in-memory table of "ids I already sent" cannot cover that.

The namespace is pinned by a test that writes the expected UUID out rather than recomputing it from the implementation. Recomputing would pass just as happily if the namespace changed, and a changed namespace silently makes every previously-derived id unreachable — which is the deduplication this mechanism exists to provide.

Threading

adapter
robinhood key → to_order_body(client_order_id=...)
coinbase key → create_order(client_order_id=...), backwards docstring corrected
alpaca key → to_order_body(client_order_id=...)
fake accepts and deliberately ignores — it has no client_order_id to carry it into, and a dedup table there would make the stand-in behave better than the venues it stands in for

Conformance

Two new contract tests. They assert acceptance, and that a repeat under one key still reaches the adapter — not deduplication, which is the venue's behaviour and cannot honestly be claimed by a suite running on canned transports. The second one also pins that an adapter must not remember keys locally and refuse the repeat: deduplication belongs to the venue, the only party that knows whether the first attempt actually landed.

Scope

This does not add retries anywhere. It makes one possible. #411's create_order backoff was blocked on exactly this — retrying a POST without an idempotency key doubles live orders — and is now unblocked.

Full suite: 3684 passed, 3 skipped. ruff check and mypy (189 files) clean.

🤖 Generated with Claude Code

…is safe

Every adapter minted a fresh uuid4 `client_order_id` per place_order CALL,
so no retry was ever deduplicated. A caller retrying after a timeout --
exactly when the first request may already have reached the venue -- placed
a SECOND live order, because the retry carried a different id and the venue
had nothing to match it against. Robinhood and Alpaca documented the hazard;
Coinbase's docstring asserted the opposite ("a fresh client_order_id per
call gives Coinbase idempotency"), which had it backwards -- a per-attempt
id is precisely what WITHHOLDS idempotency.

The port could not express the difference, which is why this was an adapter
comment rather than a fix: `place_order(spec)` cannot tell a retry of one
intent from two orders a strategy genuinely meant to place. So the port now
carries it:

    place_order(spec, *, idempotency_key: str | None = None)

`None` mints per ATTEMPT and is the unchanged default, because the opposite
default is not safer -- an id derived from the order would collapse two
deliberate orders into one, and a position at half the intended size is as
wrong as one at twice it. A key resolves every attempt under it to one
venue-facing id.

`resolve_client_order_id` in `keel_broker_api.port` owns the derivation, and
hashes the key to a uuid5 rather than passing it through. That is not
cosmetic: the venues do not agree on what a client order id may be --
Robinhood's is a UUID, Alpaca's a string of up to 128 characters -- so
hashing lands on the intersection and a caller can use whatever natural key
it has without knowing where the order will be routed. One derivation in one
place; an adapter inventing its own rule would make the same key mean
different orders at different venues.

uuid5 is deterministic across processes, which is the case that matters: a
retry issued by a NEW process, which is what a crash produces, derives the
same id as the attempt that crashed. An in-memory table of "ids I already
sent" could not cover that.

The namespace is pinned by a test that writes the expected UUID out rather
than recomputing it, because recomputing would pass just as happily if the
namespace changed -- and a changed namespace silently makes every
previously-derived id unreachable.

Threaded through all four adapters. The fake ACCEPTS the parameter and
deliberately ignores it: it has no client_order_id to carry it into, and a
dedup table there would make the stand-in behave better than the venues it
stands in for.

The conformance suite asserts ACCEPTANCE and that a repeat under one key
still reaches the adapter -- not deduplication, which is the venue's
behaviour and cannot honestly be claimed by a suite running on canned
transports.

This does not add retries anywhere. It makes one possible.

Closes #409.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@eaitbrahim
eaitbrahim merged commit f39d815 into main Aug 19, 2026
5 checks passed
@eaitbrahim
eaitbrahim deleted the feat/port-idempotency-key branch August 19, 2026 22:03
eaitbrahim added a commit that referenced this pull request Aug 20, 2026
Minor, not patch. Three things since v0.9.3 change what an implementor or a
deployment can rely on:

* A NEW DISTRIBUTION. `keel-broker-alpaca` (#382, #384) plus the
  paper-equities profile that selects it (#386), so a deployment can now be
  US equities via the broker port rather than crypto only.
* THE PORT CONTRACT MOVED TWICE. `market_clock`/`market_schedule` made
  venues session-aware (#385), and `place_order` gained `idempotency_key`
  (#419). Both carry defaults so no CALLER breaks, but a third-party adapter
  that does not accept them is no longer a `Broker` -- the conformance suite
  now says so. That is exactly the kind of change a patch bump must not
  hide.
* THE OPERATOR CONSOLE. The TUI became keel's console across #399-#408, and
  `keel update` (#415/#417) makes a deployment self-updating.

Every pinned sibling moves with it. The four production distributions are
required `==` at this exact version (`RELEASING.md`, "Release assets"), so a
bump that missed one would install a mixed set -- the `keel-trader 0.5.7`
against `keel-core 0.5.5` failure `keel versions` exists to catch, and which
`~/keel` actually ran across two releases.

Also in this window, on the Robinhood adapter: the best_bid_ask fixture
corrected against the live venue (#414), a credential guard that catches the
error it only claimed to (#416), pre-flight sizing reported on the preview
(#418), transport backoff (#420), and the fenced one-order probe (#421).

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Broker port: place_order has no idempotency key, so any retry places a second live order

1 participant