From 593ed6a78cd38338005d2446d09bf21bba7170e7 Mon Sep 17 00:00:00 2001 From: Revinand Date: Mon, 21 Sep 2026 18:12:32 +0200 Subject: [PATCH 1/3] core: treat every settle() throw as uncertain, not failed (fixes #12) --- docs/security.md | 3 +- src/core/domain/receipt.ts | 25 +++-- src/core/execution/pipeline.ts | 100 +++++++++--------- src/payments/x402/provider.ts | 26 +++-- .../integration/ap2-x402-conformance.test.ts | 29 +++++ .../authorization-ap2/replay-store.test.ts | 18 ++++ .../execution/pipeline-authorization.test.ts | 13 ++- tests/unit/core/execution/pipeline.test.ts | 23 ++-- .../payments-x402/provider-sdk-mocked.test.ts | 25 +++-- 9 files changed, 174 insertions(+), 88 deletions(-) diff --git a/docs/security.md b/docs/security.md index 274ec75..9d1fe03 100644 --- a/docs/security.md +++ b/docs/security.md @@ -341,6 +341,7 @@ rejection outcomes assert that balances did not move. | an unknown key nested under an `additionalProperties` schema | rejected by the closed schema | same | | a hostile or unbounded facilitator rejection string | clamped before it reaches buyer, event or ledger | `tests/unit/payments-x402` | | facilitator timeout | `PAYMENT_PROVIDER_UNAVAILABLE`, settlement treated as *uncertain* | `tests/unit/payments-x402` | +| a settle() throw the provider cannot classify | `PAYMENT_PROVIDER_UNAVAILABLE`, *uncertain* - never a rejection | `tests/unit/payments-x402` | | **facilitator 401 / 5xx** | `PAYMENT_PROVIDER_UNAVAILABLE`, never charged to the buyer | `tests/integration/adversarial-payment.test.ts` | | **malformed facilitator response** | refused; never read as a verdict | same | | backend timeout | `BACKEND_TIMEOUT` | `tests/unit/core/execution` | @@ -375,7 +376,7 @@ rejection outcomes assert that balances did not move. | **a mandate replayed under selective disclosure** (one mandate, many presentation strings) | refused - the replay key is the issuer-signed token, not the presentation | `tests/unit/authorization-ap2` | | **the AP2 replay store unreachable** | `AUTHORIZATION_PROVIDER_UNAVAILABLE`, retryable, never the buyer's fault | `tests/integration/ap2-runtime.test.ts` | | **a payment rejected after a mandate verified** | the reservation is released; a corrected proof reuses the mandate | `tests/integration/ap2-x402-conformance.test.ts` | -| **a settlement broadcast but never confirmed** | the mandate is *not* handed back; marked uncertain for an operator | same | +| **a settlement whose outcome never came back** (timeout, reset, proxy 502, with or without a transaction hash) | the mandate is *not* handed back; marked uncertain for an operator | same, `tests/unit/core/execution` | | **a free resource configured to require a mandate** | refused at config load, and again on the execution path | `tests/unit/config/ap2.test.ts`, `tests/unit/core/execution` | | **an oversized `Agent-Authorization` header** | `AUTHORIZATION_INVALID` before any decode; nothing echoed back | `tests/integration/authorization-carrier.test.ts` | diff --git a/src/core/domain/receipt.ts b/src/core/domain/receipt.ts index ae5bccf..b29ffc4 100644 --- a/src/core/domain/receipt.ts +++ b/src/core/domain/receipt.ts @@ -35,16 +35,23 @@ export interface PaymentAttempt { /** See PaymentResult.replayKey — unique across the store. */ readonly replayKey: string; /** - * Terminal state of the authorisation. + * State of the authorisation, as far as the gateway can prove it. * - * `settlement-uncertain` is deliberately distinct from `failed`: it means the - * settlement transaction was broadcast but its outcome could not be - * confirmed (RPC timeout, dropped connection). "The RPC did not answer" and - * "the transfer did not happen" are different facts, and recording the first - * as the second makes the merchant's reconciliation artefact wrong in exactly - * the case where reconciliation matters. When this status is set, - * `externalReference` carries the broadcast transaction hash so the outcome - * can be resolved later. + * `settlement-uncertain` is deliberately distinct from `failed`: the + * settlement was attempted and no verdict came back (RPC timeout, dropped + * connection, a facilitator that accepted the transfer and then lost the + * response). "The facilitator did not answer" and "the transfer did not + * happen" are different facts, and recording the first as the second makes + * the merchant's reconciliation artefact wrong in exactly the case where + * reconciliation matters. `externalReference` carries the broadcast + * transaction hash when one is known, which is often not: the response + * that would have carried it is usually the thing that went missing. An + * attempt in this state is unresolved rather than terminal, and needs + * evidence from the chain, not a retry. + * + * `failed` is no longer written by the execution pipeline, because a throw + * out of settle() is never proof that nothing moved. It stays in the union + * because existing databases hold rows recorded under the old reading. */ readonly status: | 'reserved' diff --git a/src/core/execution/pipeline.ts b/src/core/execution/pipeline.ts index ea18523..0239300 100644 --- a/src/core/execution/pipeline.ts +++ b/src/core/execution/pipeline.ts @@ -508,33 +508,31 @@ export function createExecutionPipeline( verification, }); } catch (error) { - // / "the RPC did not answer" and "the transfer did not - // happen" are different facts. A provider that broadcast a settlement - // transaction and then failed to confirm it throws - // PAYMENT_PROVIDER_UNAVAILABLE with details.transactionHash attached - // (the hash is known before confirmation). Recording that as plain - // `failed` would tell the merchant's reconciliation artefact a - // falsehood — the buyer's funds may already have moved. The resource - // is still not delivered either way: only what gets *recorded* - // changes, never the fail-closed outcome. - const uncertainTxHash = uncertainSettlementTxHash(error); - // An unconfirmed broadcast may still have moved funds, so the proof is - // not handed back. Only a settlement that provably failed is releasable. - await hold?.finalize(uncertainTxHash !== undefined ? 'markUncertain' : 'release'); + // A verdict arrives as a *returned* PaymentResult, settled or + // rejected. A throw means no verdict was obtained, and no throw on + // this rail can say whether the transfer happened: a facilitator can + // accept a settlement, broadcast it, then lose the response to a + // timeout, a reset or a proxy 502. "No transaction hash" is not + // evidence of "no transfer", only evidence that we never heard one. + // So every throw out of settle() is uncertain: the attempt is + // recorded unresolved and the authorization hold is kept, because a + // released mandate is spendable again with a fresh payment + // authorization against a charge that may already have landed. Only + // a returned `rejected` below - the facilitator's own statement that + // nothing moved - releases it. The resource is not delivered either + // way: only what gets *recorded* changes, never the fail-closed + // outcome. + const txHash = settlementTxHash(error); + await hold?.finalize('markUncertain'); await safePersist( () => - options.store.updatePaymentAttempt( - uncertainTxHash !== undefined - ? { replayKey, status: 'settlement-uncertain', externalReference: uncertainTxHash } - : { - replayKey, - status: 'failed', - ...(error instanceof Error ? { rejectionReason: error.message } : {}), - }, - ), - uncertainTxHash !== undefined - ? 'updatePaymentAttempt(settlement-uncertain)' - : 'updatePaymentAttempt(failed)', + options.store.updatePaymentAttempt({ + replayKey, + status: 'settlement-uncertain', + ...(txHash !== undefined ? { externalReference: txHash } : {}), + ...(error instanceof Error ? { rejectionReason: error.message } : {}), + }), + 'updatePaymentAttempt(settlement-uncertain)', request.requestId, ); await safeEmit( @@ -545,34 +543,32 @@ export function createExecutionPipeline( adapter: request.protocol, paymentProvider: provider.name, status: 'error', - data: - uncertainTxHash !== undefined - ? { reason: 'settlement-uncertain', transactionHash: uncertainTxHash } - : { reason: 'settlement-threw' }, + data: { + reason: 'settlement-uncertain', + ...(txHash !== undefined ? { transactionHash: txHash } : {}), + }, }), ); // The merchant's record now tells the truth (above); the buyer must - // too — they are the party whose funds may have moved, and the + // too - they are the party whose funds may have moved, and the // client-visible error is their only way to find out. Code stays - // PAYMENT_SETTLEMENT_FAILED (not PAYMENT_PROVIDER_UNAVAILABLE, which - // is retryable) — a retry would reuse the already-reserved replay - // key. The transaction hash is safe to disclose: it's the buyer's - // own payment, public on-chain the moment it lands. Nothing else - // from the underlying error travels — only this flag and the hash. - throw new CommerceError( - 'PAYMENT_SETTLEMENT_FAILED', - uncertainTxHash !== undefined - ? 'Settlement could not be confirmed' - : 'Payment settlement failed', - { - requestId: request.requestId, - resourceId: resource.id, - cause: error, - ...(uncertainTxHash !== undefined - ? { details: { settlementUncertain: true, transactionHash: uncertainTxHash } } - : {}), + // PAYMENT_SETTLEMENT_FAILED, not the retryable + // PAYMENT_PROVIDER_UNAVAILABLE: a retry would reuse the + // already-reserved replay key, and paying again is the one thing an + // unresolved settlement must never invite. The correlation id is the + // envelope's requestId; the transaction hash, when there is one, is + // safe to disclose, being the buyer's own payment and public + // on-chain the moment it lands. Nothing else from the underlying + // error travels. + throw new CommerceError('PAYMENT_SETTLEMENT_FAILED', 'Settlement could not be confirmed', { + requestId: request.requestId, + resourceId: resource.id, + cause: error, + details: { + settlementUncertain: true, + ...(txHash !== undefined ? { transactionHash: txHash } : {}), }, - ); + }); } if (settlement.status !== 'settled') { @@ -779,9 +775,11 @@ function backendErrorStatus(error: CommerceError): number { return typeof status === 'number' ? status : 0; } -function uncertainSettlementTxHash(error: unknown): string | undefined { - if (!isCommerceError(error) || error.code !== 'PAYMENT_PROVIDER_UNAVAILABLE') return undefined; - const hash = error.details?.['transactionHash']; +// The broadcast transaction hash a provider attached to a settlement throw, +// when it knows one. Absent far more often than not: the response that would +// have carried it is usually the thing that went missing +function settlementTxHash(error: unknown): string | undefined { + const hash = isCommerceError(error) ? error.details?.['transactionHash'] : undefined; return typeof hash === 'string' ? hash : undefined; } diff --git a/src/payments/x402/provider.ts b/src/payments/x402/provider.ts index 67f81a5..a99db2d 100644 --- a/src/payments/x402/provider.ts +++ b/src/payments/x402/provider.ts @@ -540,14 +540,28 @@ export function createX402PaymentProvider(options: X402ProviderOptions): Payment { cause: err }, ); } - const rejectionReason = isOnChainRevertError(err) - ? 'transaction_reverted' - : 'unexpected_settle_error'; + // A revert is the chain's own statement that the transfer did not + // happen, so it is a real rejection and the pipeline may release what + // it holds. Anything else out of settle() is unclassified: we cannot + // tell whether the transfer was broadcast, so it must not come back as + // a rejection the buyer can be blamed for. It goes back as an + // unavailable provider, and the pipeline records it unresolved. + if (!isOnChainRevertError(err)) { + logger.warn( + { err: describeError(err) }, + 'x402 settle(): settlement failed with an unclassified error; outcome unknown', + ); + throw new CommerceError( + 'PAYMENT_PROVIDER_UNAVAILABLE', + 'x402 provider: settlement failed with an unclassified error; outcome unknown', + { cause: err }, + ); + } logger.warn( - { err: describeError(err), rejectionReason }, - 'x402 settle(): settlement transaction failed', + { err: describeError(err), rejectionReason: 'transaction_reverted' }, + 'x402 settle(): settlement transaction reverted on chain', ); - return rejectedSettlement(rejectionReason); + return rejectedSettlement('transaction_reverted'); } if (!sdkResult.success) { diff --git a/tests/integration/ap2-x402-conformance.test.ts b/tests/integration/ap2-x402-conformance.test.ts index 746207b..c2b1bcb 100644 --- a/tests/integration/ap2-x402-conformance.test.ts +++ b/tests/integration/ap2-x402-conformance.test.ts @@ -601,6 +601,35 @@ describe('AP2 over x402: when settlement goes wrong', () => { expect(counts.backend).toBe(1); }); + it('does not hand the mandate back when the facilitator drops the response', async () => { + // The facilitator took the settlement and then lost the reply, so there + // is no transaction hash to report. That absent hash is what used to make + // this look like a clean failure and hand the mandate back. + const gw = await startGateway( + countingRail({ + settle: async () => { + throw new CommerceError('PAYMENT_PROVIDER_UNAVAILABLE', 'socket hang up'); + }, + }), + ); + const presentation = await mandate(); + + const uncertain = await purchase(presentation, gw); + expect(uncertain.statusCode).toBe(502); + expect(uncertain.body['code']).toBe('PAYMENT_SETTLEMENT_FAILED'); + // No hash to hand over, but the buyer is still told not to pay again + expect(uncertain.body['details']).toEqual({ settlementUncertain: true }); + + // A *different* payment authorization, so nothing here can be refused as + // a payment replay: the rail issues a fresh replayKey per verify, and the + // refusal is still the authorization one. The mandate is what is no + // longer spendable. + const retry = await invoke(gw, { proof: 'x402-proof-2', presentation }); + expect(retry.statusCode).toBe(409); + expect(retry.body['code']).toBe('AUTHORIZATION_REPLAYED'); + expect(counts.backend).toBe(0); + }); + it('does not hand the mandate back when a broadcast settlement was never confirmed', async () => { const gw = await startGateway( countingRail({ diff --git a/tests/unit/authorization-ap2/replay-store.test.ts b/tests/unit/authorization-ap2/replay-store.test.ts index 6c7fc5a..2d75aaa 100644 --- a/tests/unit/authorization-ap2/replay-store.test.ts +++ b/tests/unit/authorization-ap2/replay-store.test.ts @@ -158,6 +158,24 @@ describe('durability', () => { expect(existsSync(path)).toBe(true); }); + it('still refuses an uncertain mandate after the database is reopened', () => { + // The state that carries the money risk: a settlement whose outcome nobody + // learned must not turn spendable again because the gateway restarted + const path = join(scratch, 'uncertain-reopen.sqlite'); + const first = createAp2ReplayStore({ path }); + first.reserve(request()); + first.markUncertain('sha256:AAAA'); + first.close(); + + const second = createAp2ReplayStore({ path }); + expect(second.stateOf('sha256:AAAA')).toBe('uncertain'); + expect(second.reserve(request({ requestId: 'req-2' }))).toEqual({ + kind: 'replayed', + state: 'uncertain', + }); + second.close(); + }); + it('reopens an existing file without re-running the migration', () => { const path = join(scratch, 'migrate-once.sqlite'); const first = createAp2ReplayStore({ path }); diff --git a/tests/unit/core/execution/pipeline-authorization.test.ts b/tests/unit/core/execution/pipeline-authorization.test.ts index 989fa99..3c98c46 100644 --- a/tests/unit/core/execution/pipeline-authorization.test.ts +++ b/tests/unit/core/execution/pipeline-authorization.test.ts @@ -500,10 +500,15 @@ describe('execution pipeline authorization', () => { expect(auth.calls).toEqual(['verifyAndReserve', 'release']); }); - it('releases the reservation when settlement throws without moving funds', async () => { + it('keeps the reservation when settlement throws with no hash and no verdict', async () => { const auth = createFakeAuthorizationProvider(); + let backendCalls = 0; const { pipeline } = buildPipeline({ authorizationProviders: [auth], + backend: createFakeBackendExecutor(async () => { + backendCalls += 1; + return { status: 200, body: {}, headers: {}, durationMs: 0 }; + }), paymentProviders: [ createFakePaymentProvider({ settle: async () => { @@ -516,7 +521,11 @@ describe('execution pipeline authorization', () => { await expect(pipeline.execute(makeRequest())).rejects.toSatisfy( (error: unknown) => codeOf(error) === 'PAYMENT_SETTLEMENT_FAILED', ); - expect(auth.calls).toEqual(['verifyAndReserve', 'release']); + // A throw is not a verdict, hash or no hash. Releasing here would hand + // back a mandate that a fresh payment authorization can spend again, + // against a charge that may already have landed. + expect(auth.calls).toEqual(['verifyAndReserve', 'markUncertain']); + expect(backendCalls).toBe(0); }); it('marks the reservation uncertain when a broadcast settlement was never confirmed', async () => { diff --git a/tests/unit/core/execution/pipeline.test.ts b/tests/unit/core/execution/pipeline.test.ts index bc8cbaa..4fbd5ac 100644 --- a/tests/unit/core/execution/pipeline.test.ts +++ b/tests/unit/core/execution/pipeline.test.ts @@ -1166,7 +1166,7 @@ describe('createExecutionPipeline', () => { ); }); - it('settle() throwing -> PAYMENT_SETTLEMENT_FAILED, attempt marked failed', async () => { + it('settle() throwing -> PAYMENT_SETTLEMENT_FAILED, attempt marked settlement-uncertain', async () => { const resource = makeResource({ id: 'res-1', pricing: { type: 'fixed', amount: '0.01', currency: 'USDC' }, @@ -1200,15 +1200,18 @@ describe('createExecutionPipeline', () => { if (!isCommerceError(thrown)) throw new Error('unreachable'); expect(thrown.code).toBe('PAYMENT_SETTLEMENT_FAILED'); - // Ordinary failure: neither the flag nor a hash — there is no hash to show. - expect(thrown.message).toBe('Payment settlement failed'); - expect(thrown.details).toBeUndefined(); + // No hash to show, but "we never heard back" is not "nothing moved". The + // buyer is told the outcome is unknown, with the requestId to follow up on. + expect(thrown.message).toBe('Settlement could not be confirmed'); + expect(thrown.details).toEqual({ settlementUncertain: true }); const { toErrorEnvelope } = await import('../../../../src/core/domain/wire.js'); const envelope = toErrorEnvelope(thrown); - expect(envelope.details).toBeUndefined(); + expect(envelope.details).toEqual({ settlementUncertain: true }); + expect(envelope.requestId).toBeDefined(); const attempt = [...store.attempts.values()][0]; - expect(attempt?.status).toBe('failed'); + expect(attempt?.status).toBe('settlement-uncertain'); + expect(attempt?.externalReference).toBeUndefined(); }); it('settle() throwing PAYMENT_PROVIDER_UNAVAILABLE with a transactionHash records settlement-uncertain, not failed', async () => { @@ -1264,7 +1267,7 @@ describe('createExecutionPipeline', () => { expect(attempt?.externalReference).toBe('0xdeadbeef'); }); - it('settle() throwing PAYMENT_PROVIDER_UNAVAILABLE with no transactionHash still records failed', async () => { + it('settle() throwing PAYMENT_PROVIDER_UNAVAILABLE with no transactionHash records settlement-uncertain', async () => { const resource = makeResource({ id: 'res-1', pricing: { type: 'fixed', amount: '0.01', currency: 'USDC' }, @@ -1274,7 +1277,9 @@ describe('createExecutionPipeline', () => { const { CommerceError } = await import('../../../../src/core/errors/index.js'); const provider = createFakePaymentProvider({ settle: async () => { - throw new CommerceError('PAYMENT_PROVIDER_UNAVAILABLE', 'RPC unreachable before broadcast'); + // The facilitator took the settlement and then dropped the response, + // so there is no hash. An absent hash is not an absent transfer. + throw new CommerceError('PAYMENT_PROVIDER_UNAVAILABLE', 'connection reset during settle'); }, }); const pipeline = createExecutionPipeline({ @@ -1295,7 +1300,7 @@ describe('createExecutionPipeline', () => { ); const attempt = [...store.attempts.values()][0]; - expect(attempt?.status).toBe('failed'); + expect(attempt?.status).toBe('settlement-uncertain'); }); it('tolerates a non-Error thrown by a persistence call', async () => { diff --git a/tests/unit/payments-x402/provider-sdk-mocked.test.ts b/tests/unit/payments-x402/provider-sdk-mocked.test.ts index d9837ae..f336a6d 100644 --- a/tests/unit/payments-x402/provider-sdk-mocked.test.ts +++ b/tests/unit/payments-x402/provider-sdk-mocked.test.ts @@ -346,7 +346,7 @@ describe('provider — SDK-boundary branches (mocked x402/facilitator)', () => { expect(withoutReason.rejectionReason).toBe('settlement_failed'); }); - it('settle() classifies an on-chain revert distinctly from an unexpected error', async () => { + it('settle() rejects an on-chain revert but reports an unclassified throw as unavailable', async () => { const provider = makeProvider(); const requirement = await provider.createRequirement(paymentContext()); const proof = await createPaymentProof({ @@ -379,16 +379,21 @@ describe('provider — SDK-boundary branches (mocked x402/facilitator)', () => { expect(reverted.asset).toBe(ASSET); expect(reverted.replayKey).toBe('0xreverted'); + // An unclassified throw says nothing about whether the transfer was + // broadcast, so it is not a rejection the buyer can be blamed for. It goes + // back as an unavailable provider, and the pipeline records it unresolved. settleMock.mockRejectedValueOnce(new TypeError('boom')); - const unexpected = await provider.settle({ - requestId: 'req-1', - resource: RESOURCE, - requirement, - submission: { method: 'x402', payload: proof }, - verification: { status: 'verified', provider: 'x402', amount: '0.01', currency: 'USD' }, - }); - expect(unexpected.status).toBe('rejected'); - expect(unexpected.rejectionReason).toBe('unexpected_settle_error'); + await expect( + provider.settle({ + requestId: 'req-1', + resource: RESOURCE, + requirement, + submission: { method: 'x402', payload: proof }, + verification: { status: 'verified', provider: 'x402', amount: '0.01', currency: 'USD' }, + }), + ).rejects.toSatisfy( + (error: unknown) => isCommerceError(error) && error.code === 'PAYMENT_PROVIDER_UNAVAILABLE', + ); }); it('settle() throws PAYMENT_PROVIDER_UNAVAILABLE when the SDK throws a connection-shaped error', async () => { From 293d1d9e5d798ba8339db6fc2c178573022ffeea Mon Sep 17 00:00:00 2001 From: Revinand Date: Mon, 21 Sep 2026 20:23:00 +0200 Subject: [PATCH 2/3] protocol: no duplicate ACP order after an ambiguous merchant reply (fixes #13) --- docs/configuration.md | 15 +- docs/contract-surface.txt | 2 + docs/contracts.md | 1 + docs/protocols.md | 68 +++++-- docs/security.md | 11 +- src/core/domain/request.ts | 7 + src/core/execution/backend-http.ts | 24 +++ src/core/execution/pipeline.ts | 1 + src/core/interfaces/backend.ts | 10 + src/protocols/acp/adapter.ts | 159 ++++++++++++---- src/protocols/acp/checkout-mapping.ts | 9 +- src/protocols/acp/idempotency/fingerprint.ts | 27 ++- src/protocols/acp/idempotency/store.ts | 128 ++++++++++--- src/protocols/acp/index.ts | 2 +- src/protocols/acp/response-mapping.ts | 7 + tests/conformance/acp/checkout.test.ts | 8 +- tests/conformance/acp/errors.test.ts | 12 +- tests/conformance/acp/idempotency.test.ts | 50 ++++- tests/conformance/acp/support/gateway.ts | 8 + .../unit/core/execution/backend-http.test.ts | 65 +++++++ tests/unit/protocols-acp/idempotency.test.ts | 179 ++++++++++++++++-- 21 files changed, 680 insertions(+), 113 deletions(-) diff --git a/docs/configuration.md b/docs/configuration.md index cb82737..d2dfd58 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -204,9 +204,18 @@ before it can fail at request time. A complete, validating configuration is in [examples/acp-checkout](../examples/acp-checkout). `retentionHours` may not go below 24: a shorter window would let a replayed -`Idempotency-Key` past an expired record and run a checkout twice. The -idempotency database is its own file - it never shares a table with receipts or -the x402 replay defence. +`Idempotency-Key` past an expired record and run a checkout twice. It bounds +*completed* records only. One whose merchant outcome was never learned is kept +until an operator clears it, because to the next retry, deleting it looks +exactly like the operation never having happened. The idempotency database is +its own file - it never shares a table with receipts or the x402 replay +defence. + +The gateway forwards an `Idempotency-Key` header to the merchant on every +side-effecting checkout call, derived so that it is stable across retries, a +restart and a token rotation. See +[protocols.md](protocols.md#idempotency) for what the merchant should do with +it. See [protocols.md](protocols.md#acp) for the wire contract. diff --git a/docs/contract-surface.txt b/docs/contract-surface.txt index 1d56cf1..584eec9 100644 --- a/docs/contract-surface.txt +++ b/docs/contract-surface.txt @@ -88,6 +88,7 @@ interface BackendHandler { } interface BackendRequest { + readonly idempotencyKey?: string; readonly input: unknown; readonly requestId: string; readonly resourceId: string; @@ -102,6 +103,7 @@ interface BackendResponse { interface CanonicalRequest { readonly authorization?: AuthorizationSubmission; + readonly idempotencyKey?: string; readonly input: unknown; readonly metadata?: Readonly>; readonly payment?: PaymentSubmission; diff --git a/docs/contracts.md b/docs/contracts.md index f987a70..aa151f9 100644 --- a/docs/contracts.md +++ b/docs/contracts.md @@ -91,6 +91,7 @@ the generated file is right and this table is stale. - **Additive:** `AuthorizationRecord`; optional `CommerceReceipt.authorization`; `AuthorizationProvider` gains `requirement` and `markUncertain`; `AuthorizationVerification` now extends `AuthorizationRecord`; `CommerceEventType` gains `authorization.verified` and `authorization.rejected`. *Use case:* the execution pipeline enforcing authorization, in the order payment verify -> authorize/reserve -> payment replay reserve -> settle -> consume/release/mark-uncertain. *Why `requirement` on the provider:* the 402 challenge has to name what the retry must also carry, and only the provider knows its own spec version and payload profile. *Why `markUncertain` rather than leaving a reservation alone:* a settlement that was broadcast but never confirmed must not hand the proof back, and "we did nothing" is indistinguishable from a path that forgot to finalize. *Why the receipt stores a record and not the verification:* `reservationId` is a live handle, not an audit fact, and a stored proof would be a spendable secret at rest. *Compatibility:* `CommerceReceipt.authorization` is optional and absent for every resource that requires no authorization; the receipt store adds schema version 2 (`ALTER TABLE receipts ADD COLUMN authorization_json`), so an existing database keeps its rows. `AuthorizationProvider` is not yet implemented by anything shipped, so the two new members break no consumer. - **Additive (non-frozen surfaces):** `GatewayOptions.authorizationProviders` (optional) and `ReadinessResult.authorizationProviders`; a new `./ap2` subpath exporting `createAp2AuthorizationProvider` / `ap2`, with `jose`, `@sd-jwt/core` and `canonicalize` as optional peers. *Use case:* running AP2 as a wired subsystem. *Why a subpath:* one entry per distinct peer set, named for the peer - a gateway serving no gated resource should install neither a JOSE stack nor an SD-JWT parser, and the main entry and the CLI import the narrow AP2 modules (`constants.ts`, `types.ts`, `descriptor.ts`) so neither pulls a peer. *Readiness:* an authorization provider reporting `fail` blocks `/ready` on the same threshold as a payment provider - a resource that requires a mandate cannot be served without one, and serving its challenge anyway promises what cannot be honoured. Only the fixed vocabulary token `authorization-provider-unreachable` reaches the client. *Compatibility:* both fields are additive and a deployment configuring no authorization behaves exactly as before. - **Additive (`./ap2` subpath):** `createCheckoutJwt` and `CreateCheckoutJwtOptions`. *Use case:* a merchant has to sign the checkout JWT a Checkout Mandate binds, and the gateway only verifies. *Why it ships:* `input_hash` is an RFC 8785 digest, and a hand-rolled signer reaching for a sorted-key `JSON.stringify` agrees on most inputs and disagrees on floats and non-ASCII keys - producing a mandate refused with a deliberately coarse reason. The helper also refuses a numeric `amount`, the public half of a key pair, a non-P-256 key and a missing field before signing, rather than letting each become that same opaque refusal. *Scope:* signing only. It runs in the merchant's process, never calls the gateway and is never called by it - the mirror of `createPaymentProof`. The Checkout Mandate itself is the buyer's side and nothing here mints one. +- **Additive:** optional `CanonicalRequest.idempotencyKey` and optional `BackendRequest.idempotencyKey`; the HTTP backend executor forwards the latter as the `Idempotency-Key` request header. *Use case:* a merchant that creates an order and then loses the response to a timeout or a 5xx had no way to recognise the retry that followed as the same operation, so one ACP checkout became two orders. *Why not `requestId`:* it is generated per call, so every retry would look like new work; the ACP adapter derives this one from its idempotency scope (deployment, endpoint, caller key), which is identical across a client retry, a reconnect, a gateway restart and a bearer-token rotation. *Why hashed rather than forwarded raw:* the caller's key alone does not name an operation. ACP scopes it per endpoint, so one client may legitimately send the same key to create and to complete, and a merchant keying state on the raw value would read those as one operation; the digest folds deployment, endpoint and key into the single header ACP provides. It does not separate two clients that picked the same key - they share a deployment and an endpoint, so they share a derived key, exactly as they already share a row in the local claim store. *Why a fixed header and not config:* `Idempotency-Key` is the de-facto standard and the same header ACP already mandates inbound, so a merchant speaking ACP needs no second convention; a configurable name can be added if a real backend needs one. A statically configured header of that name is overridden rather than deferred to: a fixed key would make every request after the first look like a retry of the first. *Compatibility:* both fields are optional, every adapter that sets neither behaves exactly as before, and a merchant sees no new header unless one is supplied. - **Additive (gateway wire surface):** `WellKnownDocument.authorizationProviders`, an `AdapterDescriptor[]` that is empty unless a resource requires authorization. *Use case:* the README promises every adapter's `supportedSpec`, `capabilities` and `unsupported` list is checkable at runtime rather than taken on trust, and AP2 was reportable through `doctor` but absent from the document. *Why a separate field and not `paymentProviders`:* an authorization method is not a payment rail and must never be selectable as one - the same reason `AuthorizationMethodName` is neither a `ProtocolName` nor a `PaymentMethodName`. *Compatibility:* additive; the field is always present, and the dashboard's hand-maintained mirror carries only what it renders, as it already does for `protocols.acp`. --- diff --git a/docs/protocols.md b/docs/protocols.md index ee89b9c..9077950 100644 --- a/docs/protocols.md +++ b/docs/protocols.md @@ -205,7 +205,7 @@ The adapter contains **no payment logic** and never calls a merchant backend. | Default mount | `/acp` | | Services | `checkout` only | | Authentication | `Authorization: Bearer `, required on every checkout route | -| Idempotency | `Idempotency-Key` required on every POST, durable, retained >= 24h | +| Idempotency | `Idempotency-Key` required on every POST, durable, retained >= 24h, forwarded to the merchant | The schema is **vendored, not fetched**: the exact released `schema.agentic_checkout.json` sits in the repository with its upstream commit @@ -275,9 +275,12 @@ would be inventing one. `Idempotency-Key` is mandatory on every ACP POST and is checked before the request body is even read. A key is scoped by -`(authenticated identity, concrete endpoint path, key)`, where the identity is a -SHA-256 digest of the bearer token - the token itself never reaches the database, -the logs, or a response. +`(deployment, concrete endpoint path, key)`, where the deployment is the +gateway's own public base URL. Deliberately **not** the bearer token or a +digest of it: a claim must survive a credential rotation, and scoping by the +token meant a rotated one found no row, reserved afresh, and re-ran an +operation whose outcome might already be unknown. The token plays no part in +idempotency and never reaches the database, the logs, or a response. The fingerprint is taken over the **parsed** JSON, so a retry through a different serializer (different key order, `1.0` where it first sent `1`) is a @@ -290,21 +293,58 @@ distinguish requests. | Same key, same body, still running | `409 idempotency_in_flight` with `Retry-After` | | Same key, same body, finished | the stored answer, with `Idempotent-Replayed: true`, no merchant call | | Same key, different body | `422 idempotency_conflict`, no merchant call | -| A `5xx` result | not cached - a clean retry runs again | - -Records are kept for at least 24 hours; the configuration floor is the same 24 -hours, because a shorter window would let a replayed key past an expired record -and run a checkout twice. Cleanup is lazy, inside the same transaction that -claims a key - there is no background worker. +| Merchant answered, even to refuse (`4xx`) | cached; the refusal is the answer to every retry | +| Merchant reached, outcome unknown (timeout, `5xx`, unreadable reply) | `409 idempotency_unresolved`, no `Retry-After`, the merchant is not called again | +| Failed before the merchant was called | not cached - a clean retry runs | + +Completed records are kept for at least 24 hours; the configuration floor is the +same 24 hours, because a shorter window would let a replayed key past an expired +record and run a checkout twice. Cleanup is lazy, inside the same transaction +that claims a key - there is no background worker. + +**An unresolved record never expires.** A timeout or a merchant `5xx` is not +evidence that the merchant did nothing: it may have created the order and lost +the response. Freeing the key would hand the next retry a clean slate and place +the second order, so the claim is kept and the caller is told the outcome is +unknown rather than invited to retry. Sweeping such a row on a timer would +re-create that same bug, so retention only ever deletes a completed one. +Clearing an unresolved record is an operator's decision, taken against the +merchant's own records. + +### What the merchant must implement + +The gateway forwards an **`Idempotency-Key` request header** on every call it +makes for a side-effecting ACP operation. It is not the caller's key but a +SHA-256 digest over `(deployment, endpoint, caller key)`, so it is + +- **identical** across a client retry, a network-level retry, a gateway restart + and a credential rotation, which is what makes it usable as the name of an + operation; +- **different** for the same key used on two endpoints, and for two gateways + fronting the same backend; +- **opaque** - it carries back neither the caller's key nor anything about the + gateway. + +A merchant should key its own record of a side-effecting operation on that +value: if a request arrives under a key it has already completed, return the +original result rather than performing the operation again. It also needs some +way to look an operation up by that key, because that is what an operator uses +to resolve an unresolved record. + +**If the merchant does not implement it**, everything above still holds for +retries the gateway sees - the claim is durable and concurrency-safe, and no +ambiguous failure ever frees one. What weakens is the case the gateway cannot +see: a request that reached the merchant twice by some path outside it, such as +a proxy retry or a duplicate delivery, which only the merchant can collapse. **The limit, stated plainly.** A merchant side effect over HTTP and a local SQLite commit are not one transaction. Ordinary retries and concurrency are protected durably, but if the process dies after the merchant completed an order and before the answer was stored, that key stays claimed and every retry is -answered `409` until it expires - deliberately, because re-running a completion -whose remote state is unknown risks charging a buyer twice. This is not -exactly-once semantics across a remote system, and it is not claimed to be: -merchant-side idempotency on destructive operations is still recommended. +refused - deliberately, because re-running a completion whose remote state is +unknown risks charging a buyer twice. This is not exactly-once semantics across +a remote system, and it is not claimed to be: merchant-side idempotency on +destructive operations is still recommended. ### Errors diff --git a/docs/security.md b/docs/security.md index 9d1fe03..3b2f293 100644 --- a/docs/security.md +++ b/docs/security.md @@ -199,8 +199,11 @@ buffer or parse anything. Discovery at `/.well-known/acp.json` stays public and carries no configuration: no bearer token, no backend URL, no mapped resource ids, no idempotency database -path. Only the digest of the token is ever persisted - the idempotency store -scopes keys by `SHA-256("acp-auth:" + token)`, never by the token itself. +path. The token is not persisted in any form - the idempotency store scopes +keys by the gateway's public base URL, and the token plays no part in it. It +used to scope by `SHA-256("acp-auth:" + token)`, which kept the secret out of +the file but let a rotation free every outstanding claim. A credential must not +be able to do that, so the scope moved off it entirely. `Signature` and `Timestamp` verification are **not implemented**, are not advertised, and a `Signature` header is never accepted in place of the bearer @@ -357,6 +360,10 @@ rejection outcomes assert that balances did not move. | **an ACP request naming an unsupported API version** | 400 naming `supported_versions`; never mapped to the pinned one | same | | **an ACP POST with no or an over-long `Idempotency-Key`** | 400 before the body is read | same | | **an ACP key replayed while the first request is in flight** | 409; the merchant is called exactly once | `tests/conformance/acp/idempotency.test.ts` | +| **an ACP operation the merchant acted on before timing out** | 409 `idempotency_unresolved`; one order, never two | same | +| **an ACP retry after a merchant 5xx** | the claim is held, not freed; the merchant is not called again | same, `tests/conformance/acp/errors.test.ts` | +| **an unresolved ACP record outliving its retention window** | kept; only a completed record expires | `tests/unit/protocols-acp/idempotency.test.ts` | +| **an ACP claim outliving a bearer-token rotation** | kept; the scope is the deployment, never the credential | same | | **an ACP key reused with a different body** | 422; the merchant is called exactly once | same | | **an ACP completion retried after a 5xx** | not cached; the clean retry runs | same | | **a merchant answering an ACP route with a non-ACP document** | refused as `processing_error`; its body never forwarded | `tests/conformance/acp/errors.test.ts` | diff --git a/src/core/domain/request.ts b/src/core/domain/request.ts index 81d22ff..16ec87f 100644 --- a/src/core/domain/request.ts +++ b/src/core/domain/request.ts @@ -26,6 +26,13 @@ export interface CanonicalRequest { */ readonly authorization?: AuthorizationSubmission; readonly receivedAt: IsoTimestamp; + /** + * Stable name for the side-effecting operation this request performs, when + * the protocol has one. Carried through to `BackendRequest` so the merchant + * can recognise a retry as the same operation. Never `requestId`, which is + * new on every call. + */ + readonly idempotencyKey?: string; /** Non-secret transport metadata (client id, user agent, …). */ readonly metadata?: Readonly>; } diff --git a/src/core/execution/backend-http.ts b/src/core/execution/backend-http.ts index 5dab706..c50c6e0 100644 --- a/src/core/execution/backend-http.ts +++ b/src/core/execution/backend-http.ts @@ -20,6 +20,12 @@ import type { BackendExecutor, BackendRequest, BackendResponse } from '../interf import { type Logger, NOOP_LOGGER } from '../interfaces/logger.js'; const MAX_BODY_SNIPPET_LENGTH = 512; +/** + * How a merchant recognises a repeat of an operation it may already have + * performed. The de-facto standard name, and the one ACP already mandates + * inbound, so a merchant speaking ACP needs no second convention. + */ +const IDEMPOTENCY_KEY_HEADER = 'idempotency-key'; /** * The one canonical `{param}` grammar. Extraction, substitution, the config * gate and `doctor`'s probe all go through it. @@ -98,6 +104,15 @@ export class HttpBackendExecutor implements BackendExecutor { } const headers: Record = { ...(handler.headers ?? {}) }; + // Overrides a configured header of the same name rather than deferring to + // it, unlike content-type below. A *static* idempotency key is never a + // deliberate setting: every request after the first would look like a + // retry of the first, and an idempotent merchant would replay one answer + // forever. Only the per-operation value is usable. + if (request.idempotencyKey !== undefined) { + deleteHeader(headers, IDEMPOTENCY_KEY_HEADER); + headers[IDEMPOTENCY_KEY_HEADER] = request.idempotencyKey; + } let body: string | undefined; //.set() REPLACES an existing param, so without this check a caller @@ -450,6 +465,15 @@ function hasHeader(headers: Record, name: string): boolean { return Object.keys(headers).some((key) => key.toLowerCase() === target); } +// Header names are case-insensitive, so replacing one means removing whatever +// casing it was configured under first; plain assignment would leave both +function deleteHeader(headers: Record, name: string): void { + const target = name.toLowerCase(); + for (const key of Object.keys(headers)) { + if (key.toLowerCase() === target) delete headers[key]; + } +} + function headersToRecord(headers: Headers): Record { const record: Record = {}; headers.forEach((value, key) => { diff --git a/src/core/execution/pipeline.ts b/src/core/execution/pipeline.ts index 0239300..3dab51c 100644 --- a/src/core/execution/pipeline.ts +++ b/src/core/execution/pipeline.ts @@ -641,6 +641,7 @@ export function createExecutionPipeline( requestId: request.requestId, resourceId: resource.id, input: validInput, + ...(request.idempotencyKey !== undefined ? { idempotencyKey: request.idempotencyKey } : {}), }); } catch (error) { const commerceError = toCommerceError(error, 'BACKEND_ERROR', 'Backend call failed'); diff --git a/src/core/interfaces/backend.ts b/src/core/interfaces/backend.ts index 8b3ed91..d7ed900 100644 --- a/src/core/interfaces/backend.ts +++ b/src/core/interfaces/backend.ts @@ -12,6 +12,16 @@ export interface BackendRequest { readonly resourceId: string; /** Validated resource input. */ readonly input: unknown; + /** + * Names the *operation*, so the merchant can recognise a repeat of it. + * + * Not `requestId`, which is fresh per call and would make every retry look + * like new work. An adapter sets this only when it can derive a value that + * survives a client retry, a reconnect and a gateway restart; the HTTP + * executor forwards it as the `Idempotency-Key` request header. Absent + * means the protocol has no such notion, and the merchant sees no header. + */ + readonly idempotencyKey?: string; } export interface BackendResponse { diff --git a/src/protocols/acp/adapter.ts b/src/protocols/acp/adapter.ts index ba08bd5..771a623 100644 --- a/src/protocols/acp/adapter.ts +++ b/src/protocols/acp/adapter.ts @@ -39,8 +39,8 @@ import { } from './constants.js'; import { buildDescriptor } from './descriptor.js'; import { type AcpDiscoveryMetadata, buildAcpDiscoveryDocument } from './discovery.js'; -import { acpFailure, writeAcpFailure, writeAcpJson } from './errors.js'; -import { identityHash, requestFingerprint } from './idempotency/fingerprint.js'; +import { type AcpFailure, acpFailure, writeAcpFailure, writeAcpJson } from './errors.js'; +import { operationKey, requestFingerprint } from './idempotency/fingerprint.js'; import { type AcpIdempotencyScope, type AcpIdempotencyStore, @@ -58,6 +58,52 @@ import { validateAcpDocument } from './validation.js'; /** Discovery is stable for the life of the process, so it is safe to cache at the edge. */ const DISCOVERY_CACHE_CONTROL = 'public, max-age=3600'; +/** + * Whether this attempt could have changed anything at the merchant. + * + * `no` has to be a proof, never a guess: it is the only value that frees an + * idempotency key for a retry. The caller treats `unknown` and `yes` alike, + * and they are kept apart only so a log says which it was. + */ +type MerchantReach = 'no' | 'unknown' | 'yes'; + +interface AcpCheckoutAttempt { + readonly response: AcpResponse; + readonly reached: MerchantReach; +} + +/** + * The codes the pipeline raises strictly before it calls the merchant. + * + * An allowlist, so a code added later is ambiguous by default rather than + * silently freeing a key. `STORAGE_ERROR` is deliberately absent: the receipt + * is written *after* delivery, so it means the merchant did act. + */ +const PRE_BACKEND_ERROR_CODES: ReadonlySet = new Set([ + 'CONFIG_INVALID', + 'RESOURCE_NOT_FOUND', + 'INPUT_INVALID', + 'PROTOCOL_UNSUPPORTED', + 'GATEWAY_BUSY', + 'PAYMENT_REQUIRED', + 'PAYMENT_INVALID', + 'PAYMENT_REPLAYED', + 'PAYMENT_PROVIDER_UNAVAILABLE', + 'PAYMENT_SETTLEMENT_FAILED', + 'AUTHORIZATION_REQUIRED', + 'AUTHORIZATION_INVALID', + 'AUTHORIZATION_REPLAYED', + 'AUTHORIZATION_PROVIDER_UNAVAILABLE', +]); + +function reachedFor(code: string): MerchantReach { + return PRE_BACKEND_ERROR_CODES.has(code) ? 'no' : 'unknown'; +} + +function notReached(failure: AcpFailure): AcpCheckoutAttempt { + return { response: asResponse(failure), reached: 'no' }; +} + export interface AcpAdapterOptions { readonly mountPath: string; /** The configured bearer token. Never logged, never echoed, never published. */ @@ -78,8 +124,14 @@ export class AcpProtocolAdapter implements HttpProtocolAdapter { private readonly token: string; private readonly operations: Readonly>; private readonly idempotencyOptions: AcpAdapterOptions['idempotency']; - /** Derived once: the token is fixed, and only its digest may be persisted. */ - private readonly identityHash: string; + /** + * What an idempotency claim is scoped to, from `context.publicBaseUrl`. + * + * Not readonly because, unlike the token, it is known only once the adapter + * starts. Worth that: scoping by the credential meant a rotation freed + * every outstanding claim. + */ + private deployment: string | undefined; private readonly discoveryMetadata: AcpDiscoveryMetadata | undefined; private idempotency: AcpIdempotencyStore | undefined; private resources: ReadonlyMap = new Map(); @@ -96,7 +148,6 @@ export class AcpProtocolAdapter implements HttpProtocolAdapter { this.token = options.token; this.operations = options.operations; this.idempotencyOptions = options.idempotency; - this.identityHash = identityHash(options.token); this.discoveryMetadata = options.discovery; this.descriptor = buildDescriptor(PACKAGE_VERSION); this.additionalHttpRoutes = [ @@ -153,6 +204,7 @@ export class AcpProtocolAdapter implements HttpProtocolAdapter { } this.resources = resolved; + this.deployment = context.publicBaseUrl; this.idempotency = createAcpIdempotencyStore({ path: this.idempotencyOptions.path, retentionHours: this.idempotencyOptions.retentionHours, @@ -249,24 +301,39 @@ export class AcpProtocolAdapter implements HttpProtocolAdapter { */ private async dispatch(request: AcpGuardedRequest): Promise { const store = this.idempotency; + const deployment = this.deployment; const key = request.idempotencyKey; - if (store === undefined || key === undefined) return this.runCheckout(request); + if (store === undefined || deployment === undefined || key === undefined) { + return (await this.runCheckout(request)).response; + } - const scope: AcpIdempotencyScope = { - identityHash: this.identityHash, - endpoint: request.route.path, - key, - }; + const scope: AcpIdempotencyScope = { deployment, endpoint: request.route.path, key }; const claim = store.claim(scope, requestFingerprint(request.body)); switch (claim.kind) { case 'in-flight': + return { + ...asResponse( + acpFailure( + 409, + 'invalid_request', + 'idempotency_in_flight', + 'A request with this Idempotency-Key is still being processed.', + ), + ), + retryAfterSeconds: ACP_IN_FLIGHT_RETRY_AFTER_SECONDS, + }; + case 'unresolved': + // Not "try again": an earlier attempt reached the merchant and nobody + // learned its outcome. Re-running it could order twice, and replaying + // it would invent an answer nobody gave. It may already have + // succeeded, and only the merchant's records can say. return asResponse( acpFailure( 409, - 'invalid_request', - 'idempotency_in_flight', - 'A request with this Idempotency-Key is still being processed.', + 'processing_error', + 'idempotency_unresolved', + 'An earlier request with this Idempotency-Key reached the merchant and its outcome is unknown. Check the operation with the merchant before retrying.', ), ); case 'conflict': @@ -284,22 +351,32 @@ export class AcpProtocolAdapter implements HttpProtocolAdapter { break; } - let response: AcpResponse; + let outcome: AcpCheckoutAttempt; try { - response = await this.runCheckout(request); + outcome = await this.runCheckout(request, operationKey(scope)); } catch (err) { - // The attempt produced no answer worth keeping, so the key is freed for a - // clean retry rather than left claimed by a request that never completed. - store.release(scope); + // A throw here is a bug in this adapter, not a merchant verdict, and it + // says nothing about how far the request had got. The merchant may have + // been called, so the claim is kept. + store.markUnresolved(scope); throw err; } - // 5xx is never cached: a transient failure must not poison the key for the - // whole retention window. 2xx and 4xx are the answer to this request and - // every retry of it. - if (response.status >= 500) store.release(scope); - else store.complete(scope, response); - return response; + // A 2xx or 4xx is the answer to this request and every retry of it: the + // merchant stated an outcome, even a refusing one, so it is cached. + if (outcome.response.status < 500) { + store.complete(scope, outcome.response); + } else if (outcome.reached === 'no') { + // Proven never to have left the gateway, so the key is freed and the + // caller can retry cleanly once the deployment is fixed. + store.release(scope); + } else { + // A timeout, a merchant 5xx, or a reply we could not read. None of them + // say the merchant did nothing, and freeing the key here is how one + // checkout becomes two orders. + store.markUnresolved(scope); + } + return outcome.response; } /** @@ -309,11 +386,14 @@ export class AcpProtocolAdapter implements HttpProtocolAdapter { * merchant backend: the adapter builds a canonical request and reads back * what the pipeline decided. */ - private async runCheckout(request: AcpGuardedRequest): Promise { + private async runCheckout( + request: AcpGuardedRequest, + idempotencyKey?: string, + ): Promise { const context = this.context; const resource = this.resources.get(request.route.operation); if (context === undefined || resource === undefined) { - return asResponse( + return notReached( acpFailure( 503, 'service_unavailable', @@ -328,6 +408,7 @@ export class AcpProtocolAdapter implements HttpProtocolAdapter { resourceId: resource.id, requestId: context.ids.next('acp'), receivedAt: context.clock.nowIso(), + ...(idempotencyKey !== undefined ? { idempotencyKey } : {}), }); try { @@ -341,7 +422,7 @@ export class AcpProtocolAdapter implements HttpProtocolAdapter { { resourceId: resource.id, requestId: canonical.requestId }, 'acp adapter: mapped checkout resource returned payment-required - it must be priced free', ); - return asResponse( + return notReached( acpFailure( 500, 'processing_error', @@ -359,14 +440,25 @@ export class AcpProtocolAdapter implements HttpProtocolAdapter { 'acp adapter: refusing to forward a non-conformant merchant response', ); } - return mapped.response; + // The pipeline delivered, so the merchant ran the operation - including + // where its answer was not ACP and is being refused. The order may well + // exist, and the key must not be freed for a retry. + return { response: mapped.response, reached: 'yes' }; } catch (err) { const error = toCommerceError(err); context.logger.warn( - { resourceId: resource.id, requestId: canonical.requestId, err: error.toInfo() }, + { + resourceId: resource.id, + requestId: canonical.requestId, + reached: reachedFor(error.code), + err: error.toInfo(), + }, 'acp adapter: checkout execution failed', ); - return asResponse(mapCommerceErrorToAcp(error, request.route.operation)); + return { + response: asResponse(mapCommerceErrorToAcp(error, request.route.operation)), + reached: reachedFor(error.code), + }; } } @@ -385,8 +477,8 @@ export class AcpProtocolAdapter implements HttpProtocolAdapter { ? { [ACP_IDEMPOTENCY_KEY_HEADER]: request.idempotencyKey } : {}), ...(replayed ? { [ACP_IDEMPOTENT_REPLAYED_HEADER]: 'true' } : {}), - ...(response?.status === 409 - ? { 'retry-after': String(ACP_IN_FLIGHT_RETRY_AFTER_SECONDS) } + ...(response?.retryAfterSeconds !== undefined + ? { 'retry-after': String(response.retryAfterSeconds) } : {}), }; } @@ -419,6 +511,7 @@ export class AcpProtocolAdapter implements HttpProtocolAdapter { this.resources = new Map(); this.idempotency?.close(); this.idempotency = undefined; + this.deployment = undefined; this.context = undefined; } } diff --git a/src/protocols/acp/checkout-mapping.ts b/src/protocols/acp/checkout-mapping.ts index b0aeaf1..7accd69 100644 --- a/src/protocols/acp/checkout-mapping.ts +++ b/src/protocols/acp/checkout-mapping.ts @@ -20,16 +20,23 @@ export interface AcpCanonicalRequestOptions { readonly resourceId: string; readonly requestId: string; readonly receivedAt: string; + /** + * Names this checkout operation to the merchant. The adapter derives it from + * the idempotency scope, so it is the same value on every retry - unlike + * `requestId`, which is new each time. + */ + readonly idempotencyKey?: string; } export function toCanonicalRequest(options: AcpCanonicalRequestOptions): CanonicalRequest { - const { request, resourceId, requestId, receivedAt } = options; + const { request, resourceId, requestId, receivedAt, idempotencyKey } = options; return { requestId, resourceId, input: canonicalInput(request), protocol: 'acp', receivedAt, + ...(idempotencyKey !== undefined ? { idempotencyKey } : {}), }; } diff --git a/src/protocols/acp/idempotency/fingerprint.ts b/src/protocols/acp/idempotency/fingerprint.ts index 4400f03..88fdc6d 100644 --- a/src/protocols/acp/idempotency/fingerprint.ts +++ b/src/protocols/acp/idempotency/fingerprint.ts @@ -11,13 +11,30 @@ import { createHash } from 'node:crypto'; /** - * The authenticated caller, as a value safe to persist. + * The name the merchant sees for one checkout operation. * - * The bearer token itself never reaches the database, the logs or a response; - * only this digest of it does, and the digest is never returned to a client. + * Derived from the same scope that claims the key locally - deployment, + * endpoint and the caller's key - so it is identical across a client retry, a + * network retry, a gateway restart and a credential rotation. + * + * Hashed rather than forwarded raw because the caller's key alone does not + * name an operation. ACP scopes it per endpoint, so one client may + * legitimately send `Idempotency-Key: 1` to both create and complete, and a + * merchant keying state on the raw value would read those as one operation. + * The digest folds the whole scope into the one header ACP gives us. + * + * What it does *not* do is separate two clients that picked the same key. + * They share a deployment and an endpoint, so they share a derived key, and + * they already share a row in the local claim store for the same reason. ACP + * makes key uniqueness the client's responsibility, and nothing here can fix + * that for them. */ -export function identityHash(token: string): string { - return sha256(`acp-auth:${token}`); +export function operationKey(scope: { + readonly deployment: string; + readonly endpoint: string; + readonly key: string; +}): string { + return sha256(`acp-operation:${scope.deployment}:${scope.endpoint}:${scope.key}`); } /** SHA-256 over the canonical form of a parsed JSON document. */ diff --git a/src/protocols/acp/idempotency/store.ts b/src/protocols/acp/idempotency/store.ts index 26dec53..9648023 100644 --- a/src/protocols/acp/idempotency/store.ts +++ b/src/protocols/acp/idempotency/store.ts @@ -13,23 +13,52 @@ * What this cannot do: a merchant side effect over HTTP and a local SQLite * commit are not one transaction. If the process dies after the merchant * completed an order but before the response was stored, the row stays - * `in_flight` and every retry of that key is answered 409 until it expires - - * deliberately, because re-running a completion whose remote state is unknown - * risks charging a buyer twice. Merchant-side idempotency on destructive - * operations is still strongly recommended; this store does not make the - * merchant's API exactly-once. + * `in_flight` and every retry of that key is refused - deliberately, because + * re-running a completion whose remote state is unknown risks charging a + * buyer twice. The third state, `unresolved`, is the same reasoning applied + * to an attempt that reached the merchant and came back as a timeout, a 5xx + * or a document we could not read. None of those say nothing happened, so + * the claim is kept rather than freed for a retry that could order twice. + * + * Only a `completed` row expires. An unresolved one is the record that a + * duplicate is possible, so a retention sweep deleting it would hand the next + * retry a clean key and re-create the problem the state exists to prevent. + * Clearing one is an operator's decision, taken against the merchant's own + * records. + * + * Merchant-side idempotency on destructive operations is still strongly + * recommended; this store does not make the merchant's API exactly-once. */ import type { Database } from 'better-sqlite3'; import { type Logger, NOOP_LOGGER } from '../../../core/index.js'; import { openSqliteDatabase } from '../../../storage/sqlite.js'; +/** + * v2 replaced the bearer-token digest in the primary key with the deployment's + * public base URL. See `migrate`: the upgrade discards v1 rows rather than + * translating keys it cannot translate. + */ +const SCHEMA_VERSION = 2; + /** Longest `Idempotency-Key` ACP allows. */ export const ACP_MAX_IDEMPOTENCY_KEY_LENGTH = 255; -/** Scope of one claim: who asked, which endpoint, which key. */ +/** Scope of one claim: which deployment, which endpoint, which key. */ export interface AcpIdempotencyScope { - /** Digest of the authenticated bearer token - never the token itself. */ - readonly identityHash: string; + /** + * The gateway's public base URL. + * + * Not a digest of the bearer token, which this was until a rotation turned + * out to free every claim: a new token is a new key, so a retry finds no + * row, reserves a fresh one, and re-runs an operation whose outcome may + * already be unknown. A credential must not be able to do that. The base + * URL survives rotation, is already configuration, and still separates two + * gateways fronting the same merchant backend. + * + * It is not a secret and is stored as-is, so an operator reading an + * unresolved row can see which deployment produced it. + */ + readonly deployment: string; /** The concrete endpoint path, so one key may be reused across operations. */ readonly endpoint: string; readonly key: string; @@ -46,6 +75,8 @@ export type AcpIdempotencyClaim = | { readonly kind: 'reserved' } /** Same scope, same body, still running elsewhere. */ | { readonly kind: 'in-flight' } + /** Same scope, same body, and an earlier attempt's outcome was never learned. */ + | { readonly kind: 'unresolved' } /** Same scope, different body: the key was reused for another request. */ | { readonly kind: 'conflict' } /** Same scope, same body, already answered. */ @@ -56,7 +87,17 @@ export interface AcpIdempotencyStore { claim(scope: AcpIdempotencyScope, fingerprint: string): AcpIdempotencyClaim; /** Store the answer, making later retries a replay. */ complete(scope: AcpIdempotencyScope, response: AcpStoredResponse): void; - /** Drop the claim so a clean retry can run: the attempt produced no answer worth keeping. */ + /** + * Keep the claim, with no answer: the merchant may have acted and we cannot + * say. Later retries are refused rather than replayed or re-run. + */ + markUnresolved(scope: AcpIdempotencyScope): void; + /** + * Drop the claim so a clean retry can run. + * + * Only for an attempt that provably never reached the merchant. Calling it + * after an ambiguous failure is what lets one operation happen twice. + */ release(scope: AcpIdempotencyScope): void; close(): void; } @@ -90,26 +131,35 @@ export function createAcpIdempotencyStore( label: 'ACP idempotency database', logger, }); - migrate(db); + migrate(db, logger); const selectStmt = db.prepare<[string, string, string], ClaimRow>( `SELECT state, fingerprint, status, body_json FROM acp_idempotency - WHERE identity_hash = ? AND endpoint = ? AND idempotency_key = ?`, + WHERE deployment = ? AND endpoint = ? AND idempotency_key = ?`, ); const insertStmt = db.prepare( `INSERT INTO acp_idempotency - (identity_hash, endpoint, idempotency_key, fingerprint, state, created_at, expires_at) - VALUES (@identity_hash, @endpoint, @idempotency_key, @fingerprint, 'in_flight', @created_at, @expires_at)`, + (deployment, endpoint, idempotency_key, fingerprint, state, created_at, expires_at) + VALUES (@deployment, @endpoint, @idempotency_key, @fingerprint, 'in_flight', @created_at, @expires_at)`, ); const completeStmt = db.prepare( `UPDATE acp_idempotency SET state = 'completed', status = @status, body_json = @body_json - WHERE identity_hash = @identity_hash AND endpoint = @endpoint AND idempotency_key = @idempotency_key`, + WHERE deployment = @deployment AND endpoint = @endpoint AND idempotency_key = @idempotency_key`, ); const deleteStmt = db.prepare( `DELETE FROM acp_idempotency - WHERE identity_hash = ? AND endpoint = ? AND idempotency_key = ?`, + WHERE deployment = ? AND endpoint = ? AND idempotency_key = ?`, + ); + const unresolvedStmt = db.prepare( + `UPDATE acp_idempotency SET state = 'unresolved' + WHERE deployment = @deployment AND endpoint = @endpoint AND idempotency_key = @idempotency_key`, + ); + // `state = 'completed'` is the whole point: a row holding an unknown + // merchant outcome outlives retention, because to the next retry, deleting + // it looks exactly like the operation never having happened + const expireStmt = db.prepare( + "DELETE FROM acp_idempotency WHERE expires_at <= ? AND state = 'completed'", ); - const expireStmt = db.prepare('DELETE FROM acp_idempotency WHERE expires_at <= ?'); /** * Lazy cleanup, on the same connection and inside the claim transaction: a @@ -121,10 +171,10 @@ export function createAcpIdempotencyStore( const at = now(); expireStmt.run(new Date(at).toISOString()); - const existing = selectStmt.get(scope.identityHash, scope.endpoint, scope.key); + const existing = selectStmt.get(scope.deployment, scope.endpoint, scope.key); if (existing === undefined) { insertStmt.run({ - identity_hash: scope.identityHash, + deployment: scope.deployment, endpoint: scope.endpoint, idempotency_key: scope.key, fingerprint, @@ -138,6 +188,7 @@ export function createAcpIdempotencyStore( // whether or not the first one has finished. Reporting "in flight" there // would invite a retry that can only ever conflict. if (existing.fingerprint !== fingerprint) return { kind: 'conflict' }; + if (existing.state === 'unresolved') return { kind: 'unresolved' }; if (existing.state !== 'completed') return { kind: 'in-flight' }; return { kind: 'replay', @@ -154,15 +205,22 @@ export function createAcpIdempotencyStore( }, complete(scope, response) { completeStmt.run({ - identity_hash: scope.identityHash, + deployment: scope.deployment, endpoint: scope.endpoint, idempotency_key: scope.key, status: response.status, body_json: JSON.stringify(response.body ?? null), }); }, + markUnresolved(scope) { + unresolvedStmt.run({ + deployment: scope.deployment, + endpoint: scope.endpoint, + idempotency_key: scope.key, + }); + }, release(scope) { - deleteStmt.run(scope.identityHash, scope.endpoint, scope.key); + deleteStmt.run(scope.deployment, scope.endpoint, scope.key); }, close() { if (closed) return; @@ -176,13 +234,33 @@ export function createAcpIdempotencyStore( * `PRAGMA user_version` as the migration marker, so reopening an existing file * is a fast no-op check rather than a re-create. */ -function migrate(db: Database): void { +function migrate(db: Database, logger: Logger): void { const current = db.pragma('user_version', { simple: true }) as number; - if (current >= 1) return; + if (current >= SCHEMA_VERSION) return; + const apply = db.transaction(() => { + if (current === 1) { + // v1 keyed every row by a digest of the bearer token. Those keys cannot + // be translated forward: the digest is one-way, and what it stood for + // was the credential, not the deployment. So the table is rebuilt empty + // - the one time orphaning a claim is right, because leaving rows that + // nothing will ever match again is worse than saying they are gone. + const carried = db + .prepare("SELECT COUNT(*) AS count FROM acp_idempotency WHERE state != 'completed'") + .get() as { count: number }; + if (carried.count > 0) { + logger.warn( + { unresolved: carried.count }, + 'acp idempotency: schema v1 -> v2 discards claims keyed by the old bearer-token digest; ' + + 'reconcile these operations against the merchant before trusting a retry', + ); + } + db.exec('DROP TABLE acp_idempotency'); + } + db.exec(` CREATE TABLE IF NOT EXISTS acp_idempotency ( - identity_hash TEXT NOT NULL, + deployment TEXT NOT NULL, endpoint TEXT NOT NULL, idempotency_key TEXT NOT NULL, fingerprint TEXT NOT NULL, @@ -191,12 +269,12 @@ function migrate(db: Database): void { body_json TEXT, created_at TEXT NOT NULL, expires_at TEXT NOT NULL, - PRIMARY KEY (identity_hash, endpoint, idempotency_key) + PRIMARY KEY (deployment, endpoint, idempotency_key) ); CREATE INDEX IF NOT EXISTS idx_acp_idempotency_expires_at ON acp_idempotency(expires_at); `); - db.pragma('user_version = 1'); + db.pragma(`user_version = ${SCHEMA_VERSION}`); }); apply(); } diff --git a/src/protocols/acp/index.ts b/src/protocols/acp/index.ts index a1e1951..5787223 100644 --- a/src/protocols/acp/index.ts +++ b/src/protocols/acp/index.ts @@ -27,7 +27,7 @@ export { ACP_CAPABILITIES, ACP_UNSUPPORTED } from './descriptor.js'; export type { AcpDiscoveryMetadata } from './discovery.js'; export { buildAcpDiscoveryDocument } from './discovery.js'; export type { AcpError, AcpErrorType, AcpFailure } from './errors.js'; -export { identityHash, requestFingerprint } from './idempotency/fingerprint.js'; +export { operationKey, requestFingerprint } from './idempotency/fingerprint.js'; export { ACP_MAX_IDEMPOTENCY_KEY_LENGTH, type AcpIdempotencyClaim, diff --git a/src/protocols/acp/response-mapping.ts b/src/protocols/acp/response-mapping.ts index 48051d7..2173dc7 100644 --- a/src/protocols/acp/response-mapping.ts +++ b/src/protocols/acp/response-mapping.ts @@ -19,6 +19,13 @@ export interface AcpResponse { readonly body: unknown; /** True when this answer came from the idempotency store rather than work done now. */ readonly replayed?: boolean; + /** + * Seconds to put in `Retry-After`, set only where retrying is the right + * move. Opt-in rather than derived from the status: two of the 409s this + * adapter returns must *not* invite a retry, so a shared status is a poor + * proxy for advice to the caller. + */ + readonly retryAfterSeconds?: number; } /** diff --git a/tests/conformance/acp/checkout.test.ts b/tests/conformance/acp/checkout.test.ts index 7cdc5f6..a100a22 100644 --- a/tests/conformance/acp/checkout.test.ts +++ b/tests/conformance/acp/checkout.test.ts @@ -39,7 +39,7 @@ describe('createCheckoutSession', () => { expect(validateAcpDocument('checkoutSession', result.body)).toBeUndefined(); expect(result.headers.get('content-type')).toContain('application/json'); - expect(stack.calls).toEqual([ + expect(stack.calls).toMatchObject([ { method: 'POST', path: '/checkout_sessions', @@ -58,7 +58,7 @@ describe('updateCheckoutSession', () => { expect(result.status).toBe(200); expect(validateAcpDocument('checkoutSession', result.body)).toBeUndefined(); - expect(stack.calls[0]).toEqual({ + expect(stack.calls[0]).toMatchObject({ method: 'POST', path: '/checkout_sessions/cs_abc123', query: {}, @@ -76,7 +76,7 @@ describe('getCheckoutSession', () => { expect(result.status).toBe(200); expect(validateAcpDocument('checkoutSession', result.body)).toBeUndefined(); - expect(stack.calls[0]).toEqual({ + expect(stack.calls[0]).toMatchObject({ method: 'GET', path: '/checkout_sessions/cs_abc123', query: {}, @@ -117,7 +117,7 @@ describe('cancelCheckoutSession', () => { expect(result.status).toBe(200); expect(validateAcpDocument('checkoutSession', result.body)).toBeUndefined(); expect(result.body['status']).toBe('canceled'); - expect(stack.calls[0]).toEqual({ + expect(stack.calls[0]).toMatchObject({ method: 'POST', path: '/checkout_sessions/cs_abc123/cancel', query: {}, diff --git a/tests/conformance/acp/errors.test.ts b/tests/conformance/acp/errors.test.ts index 2ae30b6..8ec2825 100644 --- a/tests/conformance/acp/errors.test.ts +++ b/tests/conformance/acp/errors.test.ts @@ -130,7 +130,7 @@ describe('merchant failures', () => { expect(JSON.stringify(result.body)).not.toContain('already shipped'); }); - it('does not store a refused answer, so the same key may be retried', async () => { + it('refuses a retry after a merchant 5xx instead of running the operation twice', async () => { stack = await startAcpStack(); stack.nextReply({ status: 500, body: LEAKY_BODY }); const headers = acpHeaders({ 'idempotency-key': 'idem-error-retry' }); @@ -145,7 +145,13 @@ describe('merchant failures', () => { }); expect(failed.status).toBe(502); - expect(retry.status).toBe(200); - expect(stack.calls).toHaveLength(2); + // A merchant 500 is not proof the merchant did nothing: it may have + // recorded the order and failed afterwards. Re-running the completion + // would be the second one. + expect(retry.status).toBe(409); + expect(retry.body['code']).toBe('idempotency_unresolved'); + // No Retry-After: waiting does not resolve this, the merchant's records do + expect(retry.headers.get('retry-after')).toBeNull(); + expect(stack.calls).toHaveLength(1); }); }); diff --git a/tests/conformance/acp/idempotency.test.ts b/tests/conformance/acp/idempotency.test.ts index 526ecb5..c6d4621 100644 --- a/tests/conformance/acp/idempotency.test.ts +++ b/tests/conformance/acp/idempotency.test.ts @@ -8,6 +8,7 @@ */ import { afterEach, beforeEach, describe, expect, it } from 'vitest'; import { + ACP_EXAMPLES, type AcpStack, acpFetch, acpHeaders, @@ -88,15 +89,58 @@ describe('completion replay', () => { expect(stack.calls).toHaveLength(1); }); - it('does not cache a 5xx, so a clean retry reaches the merchant again', async () => { + it('holds the key after a merchant 5xx rather than letting a retry order twice', async () => { stack.nextReply({ status: 500, body: { error: 'merchant exploded' } }); const failed = await complete('idem-complete-5'); const retry = await complete('idem-complete-5'); expect(failed.status).toBe(502); - expect(retry.status).toBe(200); + // The merchant was reached and its outcome is unknown, so the answer is + // neither replayed (we have none to give) nor re-run (it could order twice) + expect(retry.status).toBe(409); + expect(retry.body['code']).toBe('idempotency_unresolved'); expect(retry.headers.get('idempotent-replayed')).toBeNull(); - expect(stack.calls).toHaveLength(2); + expect(stack.calls).toHaveLength(1); + }); + + it('places one order when the merchant acts and the answer arrives too late', async () => { + // The reported case: the merchant records the order, then takes longer + // than our timeout to say so. "No answer" is not "no order", and the + // retry that follows must not place a second one. + const slow = await startAcpStack({ backendTimeoutMs: 150 }); + try { + slow.nextReply({ + status: 200, + body: ACP_EXAMPLES['complete_checkout_session_response'], + delayMs: 500, + }); + const headers = acpHeaders({ 'idempotency-key': 'idem-late-answer' }); + const timedOut = await acpFetch(slow, COMPLETE_PATH, { headers, body: COMPLETE_REQUEST }); + const retry = await acpFetch(slow, COMPLETE_PATH, { headers, body: COMPLETE_REQUEST }); + + expect(timedOut.status).toBe(504); + expect(retry.status).toBe(409); + expect(retry.body['code']).toBe('idempotency_unresolved'); + expect(slow.calls).toHaveLength(1); + } finally { + await slow.close(); + } + }); + + it('gives the merchant one stable key for an operation, across retries', async () => { + const headers = acpHeaders({ 'idempotency-key': 'idem-stable' }); + await acpFetch(stack, COMPLETE_PATH, { headers, body: COMPLETE_REQUEST }); + // A second endpoint, same client key: a merchant keying state on the + // forwarded value must not see two operations as one. + await acpFetch(stack, '/acp/checkout_sessions', { headers, body: CREATE_REQUEST }); + + const [completed, created] = stack.calls; + const forwarded = completed?.headers['idempotency-key']; + expect(forwarded).toBeDefined(); + // Never the caller's own key: it is client-supplied, and it is scoped by a + // digest of the bearer token that must not leave the process. + expect(forwarded).not.toBe('idem-stable'); + expect(created?.headers['idempotency-key']).not.toBe(forwarded); }); it('scopes a key to its endpoint, so the same key may create and complete', async () => { diff --git a/tests/conformance/acp/support/gateway.ts b/tests/conformance/acp/support/gateway.ts index 3c09667..98c7a12 100644 --- a/tests/conformance/acp/support/gateway.ts +++ b/tests/conformance/acp/support/gateway.ts @@ -44,6 +44,8 @@ export interface MerchantCall { readonly path: string; readonly query: Record; readonly body: unknown; + /** Lowercased, as Node delivers them. What the gateway sent, not what the ACP client did. */ + readonly headers: Record; } /** How the merchant should answer the next call, when a test needs something specific. */ @@ -96,6 +98,12 @@ async function startMerchant(state: MerchantState): Promise<{ server: Server; or path: url.pathname, query: Object.fromEntries(url.searchParams), body: raw.length > 0 ? JSON.parse(raw) : undefined, + headers: Object.fromEntries( + Object.entries(req.headers).map(([name, value]) => [ + name, + Array.isArray(value) ? value.join(',') : (value ?? ''), + ]), + ), }); const reply = state.queued ?? defaultReply(req.method ?? '', url.pathname); diff --git a/tests/unit/core/execution/backend-http.test.ts b/tests/unit/core/execution/backend-http.test.ts index 0f96c5c..b0cbae2 100644 --- a/tests/unit/core/execution/backend-http.test.ts +++ b/tests/unit/core/execution/backend-http.test.ts @@ -45,6 +45,71 @@ describe('HttpBackendExecutor', () => { expect(capturedUrl?.searchParams.get('city')).toBeNull(); }); + it('forwards an idempotency key to the merchant as Idempotency-Key', async () => { + let capturedHeaders: Record = {}; + const fetchImpl = vi.fn(async (_input: string | URL | Request, init?: RequestInit) => { + capturedHeaders = Object.fromEntries(new Headers(init?.headers).entries()); + return jsonResponse(201, { created: true }); + }); + const executor = new HttpBackendExecutor({ fetchImpl: fetchImpl as unknown as typeof fetch }); + + const handler: BackendHandler = { + type: 'http', + method: 'POST', + url: 'http://backend.local/api/orders', + }; + await executor.call(handler, { + requestId: 'req-1', + resourceId: 'orders', + input: { sku: 'abc' }, + idempotencyKey: 'op-key-1', + }); + + expect(capturedHeaders['idempotency-key']).toBe('op-key-1'); + }); + + it('sends no Idempotency-Key when the protocol has no notion of one', async () => { + let capturedHeaders: Record = {}; + const fetchImpl = vi.fn(async (_input: string | URL | Request, init?: RequestInit) => { + capturedHeaders = Object.fromEntries(new Headers(init?.headers).entries()); + return jsonResponse(200, { ok: true }); + }); + const executor = new HttpBackendExecutor({ fetchImpl: fetchImpl as unknown as typeof fetch }); + + await executor.call( + { type: 'http', method: 'GET', url: 'http://backend.local/api/weather' }, + { requestId: 'req-1', resourceId: 'weather', input: {} }, + ); + + expect(capturedHeaders['idempotency-key']).toBeUndefined(); + }); + + it('replaces a statically configured idempotency header, whatever its casing', async () => { + let capturedHeaders: Record = {}; + const fetchImpl = vi.fn(async (_input: string | URL | Request, init?: RequestInit) => { + capturedHeaders = Object.fromEntries(new Headers(init?.headers).entries()); + return jsonResponse(201, { created: true }); + }); + const executor = new HttpBackendExecutor({ fetchImpl: fetchImpl as unknown as typeof fetch }); + + const handler: BackendHandler = { + type: 'http', + method: 'POST', + url: 'http://backend.local/api/orders', + headers: { 'Idempotency-Key': 'baked-in-and-never-changing' }, + }; + await executor.call(handler, { + requestId: 'req-1', + resourceId: 'orders', + input: { sku: 'abc' }, + idempotencyKey: 'op-key-1', + }); + + // A fixed key would make every order after the first look like a retry of + // the first, so the per-operation value wins rather than deferring to config + expect(capturedHeaders['idempotency-key']).toBe('op-key-1'); + }); + it('sends remaining input as a JSON body for POST', async () => { let capturedBody: string | undefined; let capturedHeaders: Record = {}; diff --git a/tests/unit/protocols-acp/idempotency.test.ts b/tests/unit/protocols-acp/idempotency.test.ts index 94e0a2f..8f3b7fd 100644 --- a/tests/unit/protocols-acp/idempotency.test.ts +++ b/tests/unit/protocols-acp/idempotency.test.ts @@ -14,7 +14,7 @@ import { describe, expect, it } from 'vitest'; import { createAcpAdapter } from '../../../src/protocols/acp/adapter.js'; import { ACP_SPEC_VERSION } from '../../../src/protocols/acp/constants.js'; import { - identityHash, + operationKey, requestFingerprint, } from '../../../src/protocols/acp/idempotency/fingerprint.js'; import { @@ -23,9 +23,9 @@ import { } from '../../../src/protocols/acp/idempotency/store.js'; import { adapterOptions, deliveredFor, setup } from './fixtures.js'; -const IDENTITY = identityHash('acp-secret-token'); +const DEPLOYMENT = 'https://merchant.example.com'; const SCOPE: AcpIdempotencyScope = { - identityHash: IDENTITY, + deployment: DEPLOYMENT, endpoint: '/acp/checkout_sessions', key: 'idem-1', }; @@ -94,7 +94,7 @@ describe('ACP idempotency store', () => { it.each([ ['a different endpoint', { endpoint: '/acp/checkout_sessions/cs_1/cancel' }], - ['a different caller', { identityHash: identityHash('another-token') }], + ['a different deployment', { deployment: 'https://other-gateway.example.com' }], ])('scopes a key by %s', (_label, override) => { const store = memoryStore(); store.claim(SCOPE, FINGERPRINT); @@ -117,6 +117,91 @@ describe('ACP idempotency store', () => { store.close(); }); + it('refuses a key whose earlier attempt was left unresolved', () => { + const store = memoryStore(); + store.claim(SCOPE, FINGERPRINT); + store.markUnresolved(SCOPE); + + // Neither a replay (there is no answer to give) nor a fresh reservation + // (running it again could repeat a side effect the merchant already took) + expect(store.claim(SCOPE, FINGERPRINT)).toEqual({ kind: 'unresolved' }); + store.close(); + }); + + it('keeps an unresolved record past its retention window', () => { + let clock = Date.parse('2026-01-01T00:00:00.000Z'); + const store = memoryStore(() => clock); + store.claim(SCOPE, FINGERPRINT); + store.markUnresolved(SCOPE); + + // Sweeping this row would hand the next retry a clean key, which is the + // duplicate the state exists to prevent. Only a completed row expires. + clock += 30 * 24 * 60 * 60 * 1000; + expect(store.claim(SCOPE, FINGERPRINT)).toEqual({ kind: 'unresolved' }); + store.close(); + }); + + it('survives a reopen with an unresolved record intact', () => { + const dir = mkdtempSync(join(tmpdir(), 'oac-acp-idem-')); + const path = join(dir, 'acp-idempotency.sqlite'); + + const first = createAcpIdempotencyStore({ path, retentionHours: 24 }); + first.claim(SCOPE, FINGERPRINT); + first.markUnresolved(SCOPE); + first.close(); + + const second = createAcpIdempotencyStore({ path, retentionHours: 24 }); + expect(second.claim(SCOPE, FINGERPRINT)).toEqual({ kind: 'unresolved' }); + second.close(); + + rmSync(dir, { recursive: true, force: true }); + }); + + it('discards a v1 database rather than carrying keys it cannot translate', () => { + const dir = mkdtempSync(join(tmpdir(), 'oac-acp-v1-')); + const path = join(dir, 'acp-idempotency.sqlite'); + + // A v1 file, keyed by the old bearer-token digest, with one claim that was + // deliberately being held. + const legacy = new Database(path); + legacy.exec(` + CREATE TABLE acp_idempotency ( + identity_hash TEXT NOT NULL, + endpoint TEXT NOT NULL, + idempotency_key TEXT NOT NULL, + fingerprint TEXT NOT NULL, + state TEXT NOT NULL, + status INTEGER, + body_json TEXT, + created_at TEXT NOT NULL, + expires_at TEXT NOT NULL, + PRIMARY KEY (identity_hash, endpoint, idempotency_key) + ); + INSERT INTO acp_idempotency VALUES + ('old-digest', '/acp/checkout_sessions', 'idem-1', 'fp', 'unresolved', + NULL, NULL, '2026-01-01T00:00:00.000Z', '2026-01-02T00:00:00.000Z'); + `); + legacy.pragma('user_version = 1'); + legacy.close(); + + const store = createAcpIdempotencyStore({ path, retentionHours: 24 }); + // The old key was a one-way digest of a credential, so nothing can be + // carried forward; the row is gone and the scope is the new one. + expect(store.claim(SCOPE, FINGERPRINT)).toEqual({ kind: 'reserved' }); + store.close(); + + const upgraded = new Database(path, { readonly: true }); + expect(upgraded.pragma('user_version', { simple: true })).toBe(2); + const columns = (upgraded.pragma('table_info(acp_idempotency)') as { name: string }[]).map( + (column) => column.name, + ); + upgraded.close(); + expect(columns).toContain('deployment'); + expect(columns).not.toContain('identity_hash'); + + rmSync(dir, { recursive: true, force: true }); + }); + it('survives a reopen of the database file, and never writes the bearer token', () => { const dir = mkdtempSync(join(tmpdir(), 'oac-acp-idem-')); const path = join(dir, 'acp-idempotency.sqlite'); @@ -130,15 +215,42 @@ describe('ACP idempotency store', () => { expect(second.claim(SCOPE, FINGERPRINT)).toMatchObject({ kind: 'replay', status: 201 }); second.close(); - // Only the digest of the token may be persisted. + // The bearer token is not part of the scope at all any more, so nothing + // derived from it can reach the file either. const bytes = readFileSync(path).toString('binary'); expect(bytes).not.toContain('acp-secret-token'); - expect(bytes).toContain(IDENTITY); + expect(bytes).toContain(DEPLOYMENT); rmSync(dir, { recursive: true, force: true }); }); }); +describe('ACP operation keys', () => { + it('is the same value every time the same operation is presented', () => { + // What makes a retry recognisable to the merchant: the client key is the + // same, so the derived one is too, however many attempts it takes. + expect(operationKey(SCOPE)).toBe(operationKey({ ...SCOPE })); + }); + + it.each([ + ['another deployment', { deployment: 'https://other-gateway.example.com' }], + ['another endpoint', { endpoint: '/acp/checkout_sessions/cs_1/complete' }], + ['another key', { key: 'idem-2' }], + ])('differs for %s', (_label, override) => { + expect(operationKey({ ...SCOPE, ...override })).not.toBe(operationKey(SCOPE)); + }); + + it('discloses neither the caller key nor the deployment it is scoped by', () => { + const derived = operationKey(SCOPE); + + // It goes to the merchant, so it carries nothing back: the caller's key is + // client-supplied, and nothing about the gateway needs to travel with it. + expect(derived).not.toContain(SCOPE.key); + expect(derived).not.toContain(DEPLOYMENT); + expect(derived).toMatch(/^[0-9a-f]{64}$/); + }); +}); + describe('ACP request fingerprints', () => { it.each([ ['object key order', { a: 1, b: 2 }, { b: 2, a: 1 }], @@ -165,9 +277,11 @@ describe('ACP idempotency over the adapter', () => { adapter: ReturnType, key: string, body: unknown, - ): Promise<{ status: number; headers: Record }> { + token = 'acp-secret-token', + ): Promise<{ status: number; headers: Record; body: unknown }> { let status = 0; let headers: Record = {}; + let raw = ''; const res = { headersSent: false, writeHead(code: number, sent?: Record) { @@ -175,7 +289,8 @@ describe('ACP idempotency over the adapter', () => { headers = sent ?? {}; return res; }, - end() { + end(chunk?: string) { + if (chunk !== undefined) raw = chunk; return res; }, }; @@ -188,7 +303,7 @@ describe('ACP idempotency over the adapter', () => { method: 'POST', url: '/acp/checkout_sessions', headers: { - authorization: 'Bearer acp-secret-token', + authorization: `Bearer ${token}`, 'api-version': ACP_SPEC_VERSION, 'content-type': 'application/json', 'idempotency-key': key, @@ -196,7 +311,7 @@ describe('ACP idempotency over the adapter', () => { }, ); await adapter.handleHttp(req as never, res as never); - return { status, headers }; + return { status, headers, body: raw.length > 0 ? JSON.parse(raw) : undefined }; } const CREATE = { line_items: [{ id: 'item_123' }], currency: 'usd', capabilities: {} }; @@ -243,29 +358,55 @@ describe('ACP idempotency over the adapter', () => { // A transient 5xx must not poison the key for the whole retention window: // the second attempt must get to run, not be told the first is in flight. - it('does not cache a 5xx answer', async () => { + it('keeps a claim across a bearer-token rotation', async () => { + const dir = mkdtempSync(join(tmpdir(), 'oac-acp-rotate-')); + const path = join(dir, 'idem.sqlite'); + const idempotency = { path, retentionHours: 24 }; + + // Two adapters over one database, same deployment, different credentials: + // the operator rotated the token between the ambiguous attempt and the retry. + const before = createAcpAdapter(adapterOptions({ token: 'old-token', idempotency })); + await before.start(setup(new Error('backend exploded')).context); + const ambiguous = await post(before, 'idem-rotate', CREATE, 'old-token'); + await before.stop(); + + const after = createAcpAdapter(adapterOptions({ token: 'new-token', idempotency })); + await after.start(setup(deliveredFor('createCheckoutSession')).context); + const retry = await post(after, 'idem-rotate', CREATE, 'new-token'); + await after.stop(); + + expect(ambiguous.status).toBe(500); + // A credential must not be able to free a claim. Scoped by a digest of the + // token, the new one found no row, reserved afresh and re-ran the operation. + expect(retry.status).toBe(409); + expect((retry.body as { code?: string }).code).toBe('idempotency_unresolved'); + + rmSync(dir, { recursive: true, force: true }); + }); + + it('keeps the key claimed when a 5xx leaves the merchant outcome unknown', async () => { const dir = mkdtempSync(join(tmpdir(), 'oac-acp-adapter-')); const path = join(dir, 'idem.sqlite'); const adapter = createAcpAdapter(adapterOptions({ idempotency: { path, retentionHours: 24 } })); await adapter.start(setup(new Error('backend exploded')).context); - // The pipeline throws for this context, so both attempts answer 500. + // A bare Error out of the pipeline says nothing about whether the merchant + // ran, so it is ambiguous and the second attempt must not re-run it. const first = await post(adapter, 'idem-5xx', CREATE); const second = await post(adapter, 'idem-5xx', CREATE); expect(first.status).toBe(500); - expect(second.status).toBe(500); + expect(second.status).toBe(409); + expect((second.body as { code?: string }).code).toBe('idempotency_unresolved'); expect(second.headers['idempotent-replayed']).toBeUndefined(); - // The store really is in the request path, and the released claim left - // nothing behind: a row here would block the key for the whole window. + // The claim survives as a row an operator can find, rather than vanishing + // and handing the next retry a clean key. await adapter.stop(); const db = new Database(path, { readonly: true }); - const rows = db.prepare('SELECT COUNT(*) AS count FROM acp_idempotency').get() as { - count: number; - }; + const rows = db.prepare('SELECT state FROM acp_idempotency').all() as { state: string }[]; db.close(); - expect(rows.count).toBe(0); + expect(rows.map((row) => row.state)).toEqual(['unresolved']); rmSync(dir, { recursive: true, force: true }); }); From d2f60ae011ce2d399de152414ed8c7dc7e1624bf Mon Sep 17 00:00:00 2001 From: Revinand Date: Mon, 21 Sep 2026 20:28:06 +0200 Subject: [PATCH 3/3] docs: let AGENT_COMMERCE_CONFIG override the gateway dev scripts --- README.md | 8 +++++++- package.json | 4 ++-- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/README.md b/README.md index 192c04e..53d0124 100644 --- a/README.md +++ b/README.md @@ -79,9 +79,15 @@ onboarding an existing API is the CLI's main job. import { createGateway, loadConfig, receipts } from '@devlab.group/agent-commerce'; const config = await loadConfig({ path: 'config.yaml' }); +const store = receipts({ path: './receipts.sqlite' }); +// Every ReceiptStore must be initialised before anything else touches it - +// for the SQLite one this is where a schema written by a newer version is +// caught, at boot rather than at the first query +await store.init(); + const gateway = await createGateway({ config, - store: receipts({ path: './receipts.sqlite' }), + store, paymentProviders: [], protocolAdapters: [], }); diff --git a/package.json b/package.json index e92455f..0e668d6 100644 --- a/package.json +++ b/package.json @@ -80,8 +80,8 @@ "chain:deploy": "tsx scripts/chain/deploy.ts", "demo:merchant": "tsx demo/merchant/src/main.ts", "dev:merchant": "tsx watch demo/merchant/src/main.ts", - "demo:gateway": "AGENT_COMMERCE_CONFIG=config-demo.yaml tsx src/gateway/main.ts", - "dev:gateway": "AGENT_COMMERCE_CONFIG=config-demo.yaml tsx watch src/gateway/main.ts", + "demo:gateway": "AGENT_COMMERCE_CONFIG=${AGENT_COMMERCE_CONFIG:-config-demo.yaml} tsx src/gateway/main.ts", + "dev:gateway": "AGENT_COMMERCE_CONFIG=${AGENT_COMMERCE_CONFIG:-config-demo.yaml} tsx watch src/gateway/main.ts", "demo:dashboard": "vite --config demo/dashboard/vite.config.ts", "dev:dashboard": "vite --config demo/dashboard/vite.config.ts", "demo:agent": "tsx demo/agent/src/main.ts",