feat(perps): retry transient order cancellations - #290
Conversation
…-and-add-bounded-retry-handling
brunson-bot
left a comment
There was a problem hiding this comment.
The retry classification matches the engine's documented semantics: order_in_flight is the one transient cancel rejection ("retry shortly"), and order_not_pending_engine / order_unknown are correctly left terminal (the docs explicitly say to wait for the first cancel's order update rather than retry). Backoff, deadline, and abort plumbing read cleanly, no orphaned timers or listeners, and reusing the existing PerpsCommandExecutor seam keeps the new tests off fake clients.
One blocking item: the closed rejection enum doesn't match the wire contract, and a code outside it takes down the whole batch response rather than the one item. Details inline.
| }), | ||
| z.object({ | ||
| status: z.literal('err'), | ||
| error: PerpsCancelOrderErrorCodeSchema, |
There was a problem hiding this comment.
[blocking] Closing this field to five values turns any other rejection identifier into a thrown error that discards the rest of the batch.
The engine renders a code its build predates as a literal unknown_error_code_<n> string — Error::Unknown(u16) carries #[strum(to_string = "unknown_error_code_{0}")] (perpetuals engine/platform/src/errors.rs:207-209) — and the cancel ack goes straight through it via cancel_rejected_err(err) -> cancel_rejected(err.to_string(), ..) (apps/gateway/common/result.rs), whose own comment says why: "to_string() (not as_str()) so an unrecognized wire code surfaces as unknown_error_code_<n>". docs/getting-started/errors.mdx also documents a WS request-level rejection of cancel-orders / cancel-orders-coid as [{ "status": "err", "error": "invalid_request" }], and lists cancel-reachable identifiers outside this set (account_not_found, proxy_expired, account_liquidating).
The damage isn't confined to the offending item. cancelPerpsOrders parses with z.array(PerpsCancelOrderResultSchema), so one non-member fails the whole array, and PerpsSession.#handleResponse (session.ts:1075-1083) routes a schema failure through errorAckFrom, which recurses into the array and rejects the command with RequestRejectedError(<first err string>). So
[{ "status": "ok", "oid": 1 }, { "status": "err", "error": "unknown_error_code_18", "oid": 2 }]used to return two results and now throws — the caller can't tell that order 1 was cancelled. That is the worst information to lose on a cancel path.
The repo already has the pattern for this: PerpsWithdrawalStatus (perps/common.ts:224-239) pairs a PerpsKnown* enum with a Known | (string & {}) alias and z.string().transform(...), documenting that the service evolves the set independently of released clients. The same shape here keeps result.error === PerpsCancelOrderErrorCode.OrderInFlight working in retryPerpsOrderCancellations while letting new codes through.
orders.test.ts:192 codifies the strict behavior, so I read this as deliberate rather than an oversight — but I don't think the wire contract supports it.
There was a problem hiding this comment.
Already addressed in 8cd56f7. PerpsCancelOrderErrorCode = PerpsKnownCancelOrderErrorCode | (string & {}) with z.string().min(1) matches the PerpsWithdrawalStatus precedent exactly, and orders.test.ts now pins unknown_error_code_18 flowing through with its oid. A mixed [ok, unknown_code] array no longer collapses the whole command.
| const remainingMs = retryDeadline - Date.now(); | ||
| const delayMs = perpsCancelRetryDelayMs(attempts); | ||
| if (remainingMs <= 0 || delayMs >= remainingMs) break; | ||
| await waitForPerpsCancelRetry(delayMs, options.signal); |
There was a problem hiding this comment.
[issue] Aborting during backoff throws away results the SDK already holds. By the time this line is reachable every identifier has an entry in finalResults from the previous attempt, and the rejection discards all of them — cancel 3 orders, get 2 terminal results plus 1 order_in_flight, abort, and you lose the fact that 2 were cancelled.
Resolving with the collected results (transient entries keeping their last order_in_flight rejection) is what retry: false and an exhausted budget already do. The assertPerpsCancelNotAborted call before the first attempt is the one spot where throwing is unambiguous, since nothing has been sent yet.
There was a problem hiding this comment.
Already addressed in 8cd56f7. The abort path now breaks out of the loop, and since finalResults[target.resultIndex] is written for every result on every attempt, all identifiers have a terminal or last-seen entry by the time abort is reachable. returns collected results when retry backoff is aborted pins it.
| const attemptResults = await execute( | ||
| pending.map(({ identifier }) => identifier), | ||
| ); | ||
| if (attemptResults.length !== pending.length) { |
There was a problem hiding this comment.
[issue] Worth deciding explicitly what a request-level rejection means here. docs/getting-started/errors.mdx says a request-level rejection of cancel-orders / cancel-orders-coid returns a one-element array regardless of batch size, and "must not be mapped to the first submitted order". This check covers pending.length > 1, but pending.length === 1 — every single-order cancelOrder, and the tail of every retry chain — is indistinguishable, so invalid_request / ip_rate_limited / action_rate_limited / internal_error would be recorded as that order's cancel outcome. Retrying is also what makes action_rate_limited likelier. Most of those strings currently fail the schema instead (see the enum comment); if that widens, they start surfacing as per-order results.
There was a problem hiding this comment.
Addressed in 694e37f, and the disambiguation matches the wire contract. Verified against the engine rather than the docs alone:
- Request-level rejection is a one-element array with no identifier —
errors.mdx:188-195, and recommended client behavior refactor: consolidate public error unions, standardize @throws TSDoc, and remove CancelError #2 says to treat it as applying to the whole batch. That is whatorderId === undefined && clientOrderId === undefinednow encodes. - Per-order rejections do carry an identifier. On the oid path
cancel_rejected_err(err, oid_response_field(oid), None)(handler.rs:8204,8212) andforeign_err(handler.rs:9526) both pass the requested id. On the coid path the unresolved-coid slot passescoid(handler.rs:9696), andCancelRejectedCoiduses the same^[0-9a-f]{32}$pattern asPerpsClientOrderIdSchema, so it always re-parses.
Two residuals, neither worth blocking:
oid_response_fieldis(order_id != 0).then_some(order_id), andPerpsOrderIdSchemais.nonnegative(), socancelPerpsOrders({ orderIds: [0] })gets a per-order rejection with nooidand is now thrown as a request-level rejection. Order id0is the engine's "no order" sentinel, so.positive()would be the more accurate bound — separate from this PR.handler.rs:8216(_ => (0, cancel_rejected_err(err, None, None))) is a defensive catch-all on an unexpected cmd type; same shape, shouldn't be reachable on a cancel route.
| * @experimental This API may change in a breaking way in any release, including patch releases. | ||
| */ | ||
| export type PerpsCancelOptions = { | ||
| /** Optional command expiration timestamp in milliseconds. */ |
There was a problem hiding this comment.
[nit] expiresAt now does double duty — retryPerpsOrderCancellations caps retryDeadline at it (line 1008), so a near-term expiresAt silently shortens the retry budget below maxElapsedMs. Sensible behavior, but neither this line nor maxElapsedMs on line 855 mentions the coupling.
There was a problem hiding this comment.
Already addressed in 2393b48. maxElapsedMs now says retries "also stop at expiresAt, whichever limit is reached first", and expiresAt documents the same coupling from its side.
brunson-bot
left a comment
There was a problem hiding this comment.
All four round-2 findings are addressed. The open-string error field now follows the PerpsWithdrawalStatus pattern exactly, abort during backoff returns collected results instead of discarding them, and the expiresAt/maxElapsedMs interaction is documented on both option fields.
I checked the request-level rejection fix against the engine and it targets a real wire shape — PostOp::CancelOrders/CancelOrdersCoid are in has_batch_result_array() (ws/pubs.rs:144-149), so batch_error_response wraps request failures in a singleton array (pubs.rs:332-334) rather than the bare object the session already handles. One problem with how it discriminates, though.
[issue] trading.ts:814-819 — internal_error is also a per-order cancel rejection, so a single-order cancel throws away a real result
internal_cancel_response (engine/platform/src/apps/gateway/common/handler.rs:7172-7175) is the per-slot fill for any requested order whose engine response never came back. It builds cancel_rejected("internal_error", oid_response_field(order_id), coid) — a per-order rejection carrying oid — and it is used at four fill sites (:8229, :8305, :8423, :8454), all returned inside response::ok(&results).
So cancelPerpsOrders(client, { orderIds: [1] }) can legitimately receive [{ status: 'err', error: 'internal_error', oid: 1 }]. perpsCancelRequestRejectionFrom sees length === 1 and a code in the set, and throws — discarding a per-order result that carries its own oid. That is the same class of bug this commit set out to fix, mirrored.
It also fires on retry attempts, not just the first, so it is reachable from multi-order calls: cancel [1, 2], order 1 returns ok and order 2 returns order_in_flight, the retry narrows to a batch of one, and an internal_error on that retry throws away order 1's confirmed cancellation too.
The code list is also incomplete in the other direction — message_rate_limited (pubs.rs:233), open_orders_limit, service_unavailable, not_found, and request_body_timeout (common/response.rs:29-71) are request-level too and still get attributed to the pending order with orderId: undefined.
There is a structural discriminator that needs no maintained list: generic_rejected emits only { status, error } (pubs.rs:332-334), while every per-order result carries oid or coid. So:
results.length === 1 &&
result?.status === 'err' &&
result.orderId === undefined &&
result.clientOrderId === undefinedThat is complete for the codes above, and drops PERPS_CANCEL_REQUEST_REJECTION_CODES along with the duplicated backend knowledge.
[nit] orders.ts:110 — PerpsCancelOrderErrorCode value alias
The PerpsWithdrawalStatus pattern this models is type-only (perps/common.ts:239) — there is no companion export const. The enum is not on main, so nothing has consumed PerpsCancelOrderErrorCode as a value yet, but packages/client/src/index.ts now exports two runtime names for the same enum object and the only value uses are in perps.test-d.ts. Dropping the const would match the precedent.
brunson-bot
left a comment
There was a problem hiding this comment.
One new finding: a request-level rejection or transport failure on a retry attempt discards the per-order results already collected, so the default retry can return less information than retry: false. Everything else checks out — I verified the five rejection identifiers, the order_in_flight transient classification, and the request-level-singleton contract against the perpetuals engine (engine/platform/src/errors.rs, apps/gateway/common/handler.rs) and docs/getting-started/errors.mdx, and all four of my round-1 findings are resolved at 694e37f.
Validation steps
- Verified PR #290 is OPEN at head
694e37f; local worktreegit rev-parse HEADmatches, so local reads are at the PR head - Read the full diff (12 files, +736/-65)
- Read
.brunson/review.mdand applied it withAGENTS.md; skippeddocs/sdk-direction.md(no new surface shape beyond the options object) - Read full context for
packages/client/src/websockets/perps/actions/trading.ts(retry loop, option schemas, both cancel actions),session.ts(executeCommand,#sendRequest,#handleResponse,errorAckFrom,isRejectedPerpsAck,PerpsSessionTradingError),packages/bindings/src/perps/orders.ts, and both test files - Confirmed
isRejectedPerpsAckdeliberately returnsfalsefor arrays ("Array schemas own per-item error policy"), so per-item classification is the action's job — the new heuristic sits in the right layer - Verified the request-level contract in
Polymarket/perpetualsdocs/getting-started/errors.mdx:186-212: batch ops return an array, a request-level rejection is a one-element array with no identifier and "must not be mapped positionally to one submitted order" - Verified per-order rejections do carry an identifier:
handler.rs:8204,8212,9526passoid_response_field(oid),handler.rs:9696passescoid, andCancelRejectedCoidshares^[0-9a-f]{32}$withPerpsClientOrderIdSchema - Verified all five enum members exist as wire strings in
engine/platform/src/errors.rs:936-939plusError::OrderNotFound => "order_not_found"inresult.rs, and thatorder_in_flightis documented Transient/"retry shortly" (errors.mdx:113-116) whileorder_not_in_orderbook/order_not_pending_engine/order_unknownare documented terminal — the retry scoping matches the docs - Checked backoff arithmetic:
attemptsincrements after eachexecute, soperpsCancelRetryDelayMs(1..3)gives 100/200/400ms caps under full jitter and total attempts equalsmaxAttempts - Traced the abort path — listener removed on both timer-fire and abort, no dangling handle;
clearTimeoutonsetNonBlockingTimeoutmatches existing precedent atsession.ts:1031-1041 - Confirmed
finalResultsis fully populated on every loop exit (deadline, max attempts, abort), soexpectPresentat line 1073 cannot fire - Verified
perpsAckErroris still used byPerpsAckErrorSchema/PerpsPostOrderAckSchema— no dead code from the transform change - Verified export chains:
PerpsKnownCancelOrderErrorCodefromclient/src/index.ts,PerpsCancelOrderErrorCode/PerpsCancelOrderResultviaexport type * from '@polymarket/bindings/perps', andPerpsCancelOptions/PerpsCancelRetryOptionsre-exported by bothactions/perps.tsanddecorators/perps.tsper the decorator rule - Confirmed
OperationAbortedErrorandUnexpectedResponseErrorare both inPerpsSessionTradingError, andOperationAbortedErroris public viaexport * from './errors' - Confirmed every new public surface carries
@experimental - Confirmed the
Known* enum + (string & {})forward-compat shape matches thePerpsWithdrawalStatusprecedent atperps/common.ts:224-239 - Checked CI at head: Verify, Tests, Changeset, CodeQL, dependency-review all passing
- Replied in-thread to all four round-1 findings with resolution verdicts
| const requestRejection = perpsCancelRequestRejectionFrom(attemptResults); | ||
| if (requestRejection !== undefined) { | ||
| throw new RequestRejectedError(requestRejection, { status: 200 }); | ||
| } | ||
| if (attemptResults.length !== pending.length) { | ||
| throw new UnexpectedResponseError( | ||
| 'Perps cancel response did not include one result per requested order.', | ||
| ); | ||
| } |
There was a problem hiding this comment.
[issue] A failure on a retry attempt throws away the results already collected on earlier attempts — the same class of loss as the abort path, arriving through a different door.
By the time attempt 2 runs, finalResults holds a terminal entry for every identifier in the batch (line 1060 writes one per result, every attempt). Both throws here, and any throw out of execute itself, abandon that array.
Concretely — cancelPerpsOrders({ orderIds: [1, 2, 3] }) under the default retry:
- attempt 1 →
[order_in_flight, ok, ok];pending = [1] - attempt 2 sends
['cancelOrders', [1]]and the gateway returns[{ status: 'err', error: 'action_rate_limited' }] perpsCancelRequestRejectionFromcorrectly classifies it, andcancelPerpsOrdersthrows
The caller loses the fact that orders 2 and 3 were cancelled. With retry: false they would have gotten all three results back. So turning on the default makes the outcome strictly less informative in this case, and retrying is itself what makes action_rate_limited likelier — errors.mdx:213 lists it alongside ip_rate_limited / internal_error / service_unavailable as request-level and retryable. The same applies to the UnexpectedResponseError below and to a TransportError timing out attempt 2.
Two defensible shapes:
- Only throw while
attempts === 0; on a later attempt stop retrying and returnfinalResults, matching whatretry: falsewould have produced. - Keep throwing, but carry the collected results on the error so the caller can still reconcile.
Either way the invariant worth holding is that enabling retries never returns less information than disabling them.
Types Perps cancel rejection codes and retries only transient order_in_flight results with bounded configurable backoff. Includes focused batch, deadline, and abort coverage.
Note
Medium Risk
Changes default Perps cancel behavior (extra cancel commands on transient rejections) on a trading-critical path, though retries are bounded and scoped to one error code.
Overview
Adds typed Perps cancel rejection codes in bindings (
PerpsKnownCancelOrderErrorCode/ forward-compatiblePerpsCancelOrderErrorCode) and tightens cancel result parsing soerrresponses must carry a non-empty error identifier.cancelPerpsOrder/cancelPerpsOrdersnow run through a retry loop that only re-issues cancels fororder_in_flight, keeping per-order result order and retrying smaller batches. Defaults are bounded (4 attempts, 2s elapsed, exponential backoff with jitter); callers can passretry: false, tunemaxAttempts/maxElapsedMs, honorexpiresAt, or use anAbortSignal(newOperationAbortedError). Whole-request failures (invalid_request, rate limits,internal_error) still throw instead of appearing as per-order results.Public surface exports the new cancel option types and error codes; session docs describe the new default retry behavior.
Reviewed by Cursor Bugbot for commit 8cd56f7. Bugbot is set up for automated code reviews on this repo. Configure here.