diff --git a/.agents/skills/v2-api-conventions/SKILL.md b/.agents/skills/v2-api-conventions/SKILL.md index 52ab14450ad..37276518085 100644 --- a/.agents/skills/v2-api-conventions/SKILL.md +++ b/.agents/skills/v2-api-conventions/SKILL.md @@ -110,6 +110,62 @@ Order matters because each layer is checked against the one before it. 3. **Route** with `defineV2JsonRoute`, declaring `contract`, `auth: v2ApiKeyAuth`, `operation`, `rateLimit`, `errorPolicy`, `mapInput`, `useCase`, `present`. Auth and rate limiting run before parsing. 4. **OpenAPI description** in `lib/api/contracts/v2/openapi/.ts`, then `bun run generate:openapi`. A description that claims behaviour the route does not have is the same class of bug as a wrong schema. +## Rule 6 — a transient failure says when to come back + +A response the caller is *expected* to retry must say how long to wait. Two statuses qualify, and both are wired: + +| Status | Source of the value | Where | +|---|---|---| +| 429 | The caller's own token bucket (`retryAfterMs`, else `resetAt - now`) | `v2RateLimitError` | +| 503 | A fixed floor, `RETRY_AFTER_SECONDS_BY_STATUS` | `v2Error`, applied automatically | + +The 503 default is applied by `v2Error` keyed on the response *status* — `Retry-After` is defined against the status, and the status is the only half of the code/status pair a client sees — so every 503 the surface can emit carries it — the three route builders' `unhandledErrorResponse`, the execute and resume routes, and `serviceFailureResponse`'s `infra` failures. A route with a better number passes `headers: { 'Retry-After': … }` and wins. + +Do not add a default for any other code. 400/403/404/409 are not fixed by waiting, and 402 (`USAGE_LIMIT_EXCEEDED`) is resolved by a billing change, not by time. + +**Where a policy already knows the wait, carry it — do not re-guess it at the transport.** The admission descriptors in `lib/core/admission/transient-failure` declare `retryAfterSeconds` per denial. That value used to be dropped when the descriptor was mapped onto a preprocess error, so a concurrency denial arrived as a bare 429 with no `Retry-After` even though the policy had named the wait. It now travels `descriptor.retryAfterSeconds → PreprocessExecutionError.retryAfterMs → ExecuteWorkflowServiceFailure.retryAfterMs → serviceFailureResponse`. The `v2Error` default is the floor for paths with *no* policy signal, not the source of truth. + +**A failure whose outcome is unknown must not advise a retry.** `ASYNC_ENQUEUE_AMBIGUOUS` is a 503 whose enqueue may have succeeded — it deliberately retains its execution-ID claim. Telling that caller to come back in 5 seconds invites a client with no `X-Run-Id` to start and bill a second run. It passes `omitRetryAfter: true` and returns the run id so the caller reconciles instead. Any future "we don't know if it happened" failure does the same. + +RFC 9110 §10.2.3 gives 503 this field's clearest meaning — "how long the service is expected to be unavailable to the client". Note the requirement level is only `MAY`, on 503 (§15.6.4) and, via RFC 6585 §4, on 429. It is `SHOULD` on exactly one status, 413, and only when the condition is temporary; Sim's 413 is a fixed byte ceiling, so it correctly sends none. + +## Deliberate non-adoptions + +Audited against the primary specs and against Stripe, GitHub, and Google's AIPs. Each is a considered "no", not an oversight. Re-open one only with new evidence. + +| Practice | Verdict | Why | +|---|---|---| +| **RFC 9457 `application/problem+json`** | No | 9457 §4 steers APIs with an existing format toward keeping it: "Problem details are intended to avoid the necessity of establishing new 'fault' or 'error' document formats, **not to replace existing domain-specific formats**." Nothing in it is a `MUST` to adopt, and none of Stripe, GitHub, or Google use it. Our envelope is load-bearing for every client. **The default error shape does not change.** | +| **`RateLimit`/`RateLimit-Policy` (IETF draft)** | No | Still an unpublished draft (`-11`, May 2026), returned "Not ready" at HTTPDIR review, and on its **third mutually incompatible wire format** — anything built against `-07` or earlier is already broken. None of the three surveyed APIs emit it; GitHub uses `x-ratelimit-*`, as we do. | +| **Renaming `X-RateLimit-*` per RFC 6648** | No | 6648 is a `SHOULD NOT` binding *creators of new* parameters, and §1 item 4 "**makes no recommendation as to whether existing 'X-' parameters ought to remain in use or be migrated**". Appendix B argues the migration is itself the interoperability harm. A rename is a client-visible break bought with nothing. | +| **`X-RateLimit-Reset` as delta-seconds** | No | It is an absolute ISO 8601 timestamp, so it is clock-skew sensitive — but the response where timing actually decides behaviour (429) also carries `Retry-After`, which is skew-free. The absolute value stays useful for scheduling. | +| **422 for semantic validation** | No | RFC 9110 §15.5.21 defines 422, but Appendix B.3 records that 9110 **deleted** RFC 4918's clause saying 400 was inappropriate. 400 covers "cannot or will not process… perceived to be a client error". The split is convention, not requirement — GitHub splits, Stripe and Google do not. Our machine-readable `error.code` already carries the distinction, and restatusing now breaks clients. | +| **`Location` on 201** | No | §9.3.3 makes this a `SHOULD` **for POST**; the status code itself (§15.3.2) requires nothing and defines the fallback — absent `Location`, the target URI identifies the resource. Declined knowingly: several 201 responses (signed upload sessions, table exports, knowledge folders) have no canonical single-resource GET, so a `Location` would 404, and adopting it on some of the 19 is worse for a client than on none. Every 201 returns the full representation including its `id`. Revisit per-route if one gains a canonical GET. | +| **ETag / `If-None-Match` / `If-Match`** | No | Every v2 response is `Cache-Control: private, no-store` per-caller data, so `If-None-Match` buys nothing. For writes, `If-Match` needs a **strong** validator: §8.8.3.2's strong comparison fails if *either* tag is weak, so a weak ETag silently makes every `If-Match` fail. None of the three surveyed APIs does HTTP optimistic concurrency — Google does the semantics via a resource `etag` **field** (AIP-154), deliberately not the header. If Sim needs optimistic concurrency, do it that way. | +| **`Deprecation` / `Sunset` on v1** | Not yet | RFC 9745 (Standards Track) and RFC 8594 (Informational) both apply, and GitHub emits both. But `Sunset` is a timestamp and 9745 §4 makes `Sunset >= Deprecation` a `MUST`, so emitting either commits Sim to a v1 retirement date — a product decision, not an engineering one. When that date exists: `Deprecation` is an RFC 9651 Structured Field **Date** (`@1688169599`); `Sunset` is an **HTTP-date** (`Sat, 31 Dec 2033 23:59:59 GMT`). Two encodings in one response — the most common implementation error here. | +| **`application/merge-patch+json`** | No | v2 PATCH bodies are merge-patch *shaped* — absent means unchanged, `null` clears — but they are `.strict()`, so unknown members are rejected where RFC 7396 §2 would merge them, and nested objects are replaced wholesale rather than merged. Advertising the media type would over-claim. Document the semantics per contract instead. | + +## Idempotency: at-most-once, not replay + +`POST /workflows/{id}/execute` accepts `X-Run-Id`, a caller-supplied run identifier claimed through the `idempotency_key` table (`execution-id-claim.ts`). It is a **uniqueness claim, not an idempotency key**, and the distinction is deliberate and already published in the operation description: + +- First use wins and runs. +- Any reuse returns **409** with `error.details.code: "RUN_ID_CONFLICT"`, the run id in `error.details.runId`, and an `X-Run-Id` response header. It never replays the earlier run's result — the client recovers it by polling the runs resource. +- Claims are durable tombstones, so deleting execution logs cannot make an id reusable. + +That makes the money path safe against double-execution **for callers that opt in**. What it is not: a Stripe-style `Idempotency-Key` that stores and replays the original status and body. Building that means a request fingerprint, a retention window, an in-flight-vs-completed distinction (the expired IETF draft would have these be 422 and 409 respectively), and somewhere to put a large synchronous execution body. It is a designed piece of work, not an increment — do not half-build it by aliasing the header name, which would invite clients written against Stripe semantics to treat our 409 as a hard failure. + +## Cursors are opaque, not trusted + +The base64-JSON cursor is **not signed**, and does not need to be. Tampering is bounded by construction, and that is a property to preserve: + +- Every key value is re-validated by its `KeysetKey.bind`, which returns `null` for a wrong-typed or unparseable value and becomes a 400. A forged cursor cannot reach SQL as `NaN` or an `Invalid Date`. +- The sort is stamped into the cursor and re-checked (`decodeSortedCursor`), so a cursor from a differently-sorted query is a 400, not a silently skipped page. +- The offset codec rejects anything that is not a non-negative integer. +- Authorization is **never** carried in the cursor. Every list re-derives its workspace scope from the authenticated principal, so a cursor lifted from another query — or another tenant — can only move the caller within their own authorized result set. + +The consequence to keep true: **never put a resource id, filter, or scope into a cursor and then trust it on the way back.** A cursor is a position hint, never an input to an access decision. + ## Checklist Run this against any new or changed v2 endpoint. @@ -122,7 +178,8 @@ Run this against any new or changed v2 endpoint. - [ ] If the response carries `nextCursor`, the query accepts `limit` + `cursor` and the query actually applies them. - [ ] Keyset sorts end in a unique `id` key. - [ ] The list is classified in `list-pagination.test.ts`. -- [ ] Cross-tenant access answers 404, never 403. +- [ ] Cross-tenant access answers 404, never 403 — and carries `Cache-Control: private, no-store`, because RFC 9110 §15.5.5 makes 404 heuristically cacheable and an authorization-dependent 404 must never be stored. `v2Error` sets this unconditionally; do not build a v2 response any other way. +- [ ] A retryable failure says when: 429 and 503 carry `Retry-After`. No other status invents one. - [ ] 403s carry a machine-readable `details.code`. - [ ] Validation messages name the field and echo the valid set. - [ ] Response schema matches every field the route actually emits. diff --git a/.claude/commands/v2-api-conventions.md b/.claude/commands/v2-api-conventions.md index c7740489850..62f45651b14 100644 --- a/.claude/commands/v2-api-conventions.md +++ b/.claude/commands/v2-api-conventions.md @@ -109,6 +109,62 @@ Order matters because each layer is checked against the one before it. 3. **Route** with `defineV2JsonRoute`, declaring `contract`, `auth: v2ApiKeyAuth`, `operation`, `rateLimit`, `errorPolicy`, `mapInput`, `useCase`, `present`. Auth and rate limiting run before parsing. 4. **OpenAPI description** in `lib/api/contracts/v2/openapi/.ts`, then `bun run generate:openapi`. A description that claims behaviour the route does not have is the same class of bug as a wrong schema. +## Rule 6 — a transient failure says when to come back + +A response the caller is *expected* to retry must say how long to wait. Two statuses qualify, and both are wired: + +| Status | Source of the value | Where | +|---|---|---| +| 429 | The caller's own token bucket (`retryAfterMs`, else `resetAt - now`) | `v2RateLimitError` | +| 503 | A fixed floor, `RETRY_AFTER_SECONDS_BY_STATUS` | `v2Error`, applied automatically | + +The 503 default is applied by `v2Error` keyed on the response *status* — `Retry-After` is defined against the status, and the status is the only half of the code/status pair a client sees — so every 503 the surface can emit carries it — the three route builders' `unhandledErrorResponse`, the execute and resume routes, and `serviceFailureResponse`'s `infra` failures. A route with a better number passes `headers: { 'Retry-After': … }` and wins. + +Do not add a default for any other code. 400/403/404/409 are not fixed by waiting, and 402 (`USAGE_LIMIT_EXCEEDED`) is resolved by a billing change, not by time. + +**Where a policy already knows the wait, carry it — do not re-guess it at the transport.** The admission descriptors in `lib/core/admission/transient-failure` declare `retryAfterSeconds` per denial. That value used to be dropped when the descriptor was mapped onto a preprocess error, so a concurrency denial arrived as a bare 429 with no `Retry-After` even though the policy had named the wait. It now travels `descriptor.retryAfterSeconds → PreprocessExecutionError.retryAfterMs → ExecuteWorkflowServiceFailure.retryAfterMs → serviceFailureResponse`. The `v2Error` default is the floor for paths with *no* policy signal, not the source of truth. + +**A failure whose outcome is unknown must not advise a retry.** `ASYNC_ENQUEUE_AMBIGUOUS` is a 503 whose enqueue may have succeeded — it deliberately retains its execution-ID claim. Telling that caller to come back in 5 seconds invites a client with no `X-Run-Id` to start and bill a second run. It passes `omitRetryAfter: true` and returns the run id so the caller reconciles instead. Any future "we don't know if it happened" failure does the same. + +RFC 9110 §10.2.3 gives 503 this field's clearest meaning — "how long the service is expected to be unavailable to the client". Note the requirement level is only `MAY`, on 503 (§15.6.4) and, via RFC 6585 §4, on 429. It is `SHOULD` on exactly one status, 413, and only when the condition is temporary; Sim's 413 is a fixed byte ceiling, so it correctly sends none. + +## Deliberate non-adoptions + +Audited against the primary specs and against Stripe, GitHub, and Google's AIPs. Each is a considered "no", not an oversight. Re-open one only with new evidence. + +| Practice | Verdict | Why | +|---|---|---| +| **RFC 9457 `application/problem+json`** | No | 9457 §4 steers APIs with an existing format toward keeping it: "Problem details are intended to avoid the necessity of establishing new 'fault' or 'error' document formats, **not to replace existing domain-specific formats**." Nothing in it is a `MUST` to adopt, and none of Stripe, GitHub, or Google use it. Our envelope is load-bearing for every client. **The default error shape does not change.** | +| **`RateLimit`/`RateLimit-Policy` (IETF draft)** | No | Still an unpublished draft (`-11`, May 2026), returned "Not ready" at HTTPDIR review, and on its **third mutually incompatible wire format** — anything built against `-07` or earlier is already broken. None of the three surveyed APIs emit it; GitHub uses `x-ratelimit-*`, as we do. | +| **Renaming `X-RateLimit-*` per RFC 6648** | No | 6648 is a `SHOULD NOT` binding *creators of new* parameters, and §1 item 4 "**makes no recommendation as to whether existing 'X-' parameters ought to remain in use or be migrated**". Appendix B argues the migration is itself the interoperability harm. A rename is a client-visible break bought with nothing. | +| **`X-RateLimit-Reset` as delta-seconds** | No | It is an absolute ISO 8601 timestamp, so it is clock-skew sensitive — but the response where timing actually decides behaviour (429) also carries `Retry-After`, which is skew-free. The absolute value stays useful for scheduling. | +| **422 for semantic validation** | No | RFC 9110 §15.5.21 defines 422, but Appendix B.3 records that 9110 **deleted** RFC 4918's clause saying 400 was inappropriate. 400 covers "cannot or will not process… perceived to be a client error". The split is convention, not requirement — GitHub splits, Stripe and Google do not. Our machine-readable `error.code` already carries the distinction, and restatusing now breaks clients. | +| **`Location` on 201** | No | §9.3.3 makes this a `SHOULD` **for POST**; the status code itself (§15.3.2) requires nothing and defines the fallback — absent `Location`, the target URI identifies the resource. Declined knowingly: several 201 responses (signed upload sessions, table exports, knowledge folders) have no canonical single-resource GET, so a `Location` would 404, and adopting it on some of the 19 is worse for a client than on none. Every 201 returns the full representation including its `id`. Revisit per-route if one gains a canonical GET. | +| **ETag / `If-None-Match` / `If-Match`** | No | Every v2 response is `Cache-Control: private, no-store` per-caller data, so `If-None-Match` buys nothing. For writes, `If-Match` needs a **strong** validator: §8.8.3.2's strong comparison fails if *either* tag is weak, so a weak ETag silently makes every `If-Match` fail. None of the three surveyed APIs does HTTP optimistic concurrency — Google does the semantics via a resource `etag` **field** (AIP-154), deliberately not the header. If Sim needs optimistic concurrency, do it that way. | +| **`Deprecation` / `Sunset` on v1** | Not yet | RFC 9745 (Standards Track) and RFC 8594 (Informational) both apply, and GitHub emits both. But `Sunset` is a timestamp and 9745 §4 makes `Sunset >= Deprecation` a `MUST`, so emitting either commits Sim to a v1 retirement date — a product decision, not an engineering one. When that date exists: `Deprecation` is an RFC 9651 Structured Field **Date** (`@1688169599`); `Sunset` is an **HTTP-date** (`Sat, 31 Dec 2033 23:59:59 GMT`). Two encodings in one response — the most common implementation error here. | +| **`application/merge-patch+json`** | No | v2 PATCH bodies are merge-patch *shaped* — absent means unchanged, `null` clears — but they are `.strict()`, so unknown members are rejected where RFC 7396 §2 would merge them, and nested objects are replaced wholesale rather than merged. Advertising the media type would over-claim. Document the semantics per contract instead. | + +## Idempotency: at-most-once, not replay + +`POST /workflows/{id}/execute` accepts `X-Run-Id`, a caller-supplied run identifier claimed through the `idempotency_key` table (`execution-id-claim.ts`). It is a **uniqueness claim, not an idempotency key**, and the distinction is deliberate and already published in the operation description: + +- First use wins and runs. +- Any reuse returns **409** with `error.details.code: "RUN_ID_CONFLICT"`, the run id in `error.details.runId`, and an `X-Run-Id` response header. It never replays the earlier run's result — the client recovers it by polling the runs resource. +- Claims are durable tombstones, so deleting execution logs cannot make an id reusable. + +That makes the money path safe against double-execution **for callers that opt in**. What it is not: a Stripe-style `Idempotency-Key` that stores and replays the original status and body. Building that means a request fingerprint, a retention window, an in-flight-vs-completed distinction (the expired IETF draft would have these be 422 and 409 respectively), and somewhere to put a large synchronous execution body. It is a designed piece of work, not an increment — do not half-build it by aliasing the header name, which would invite clients written against Stripe semantics to treat our 409 as a hard failure. + +## Cursors are opaque, not trusted + +The base64-JSON cursor is **not signed**, and does not need to be. Tampering is bounded by construction, and that is a property to preserve: + +- Every key value is re-validated by its `KeysetKey.bind`, which returns `null` for a wrong-typed or unparseable value and becomes a 400. A forged cursor cannot reach SQL as `NaN` or an `Invalid Date`. +- The sort is stamped into the cursor and re-checked (`decodeSortedCursor`), so a cursor from a differently-sorted query is a 400, not a silently skipped page. +- The offset codec rejects anything that is not a non-negative integer. +- Authorization is **never** carried in the cursor. Every list re-derives its workspace scope from the authenticated principal, so a cursor lifted from another query — or another tenant — can only move the caller within their own authorized result set. + +The consequence to keep true: **never put a resource id, filter, or scope into a cursor and then trust it on the way back.** A cursor is a position hint, never an input to an access decision. + ## Checklist Run this against any new or changed v2 endpoint. @@ -121,7 +177,8 @@ Run this against any new or changed v2 endpoint. - [ ] If the response carries `nextCursor`, the query accepts `limit` + `cursor` and the query actually applies them. - [ ] Keyset sorts end in a unique `id` key. - [ ] The list is classified in `list-pagination.test.ts`. -- [ ] Cross-tenant access answers 404, never 403. +- [ ] Cross-tenant access answers 404, never 403 — and carries `Cache-Control: private, no-store`, because RFC 9110 §15.5.5 makes 404 heuristically cacheable and an authorization-dependent 404 must never be stored. `v2Error` sets this unconditionally; do not build a v2 response any other way. +- [ ] A retryable failure says when: 429 and 503 carry `Retry-After`. No other status invents one. - [ ] 403s carry a machine-readable `details.code`. - [ ] Validation messages name the field and echo the valid set. - [ ] Response schema matches every field the route actually emits. diff --git a/.cursor/commands/v2-api-conventions.md b/.cursor/commands/v2-api-conventions.md index a19ab57e6aa..febdb915345 100644 --- a/.cursor/commands/v2-api-conventions.md +++ b/.cursor/commands/v2-api-conventions.md @@ -104,6 +104,62 @@ Order matters because each layer is checked against the one before it. 3. **Route** with `defineV2JsonRoute`, declaring `contract`, `auth: v2ApiKeyAuth`, `operation`, `rateLimit`, `errorPolicy`, `mapInput`, `useCase`, `present`. Auth and rate limiting run before parsing. 4. **OpenAPI description** in `lib/api/contracts/v2/openapi/.ts`, then `bun run generate:openapi`. A description that claims behaviour the route does not have is the same class of bug as a wrong schema. +## Rule 6 — a transient failure says when to come back + +A response the caller is *expected* to retry must say how long to wait. Two statuses qualify, and both are wired: + +| Status | Source of the value | Where | +|---|---|---| +| 429 | The caller's own token bucket (`retryAfterMs`, else `resetAt - now`) | `v2RateLimitError` | +| 503 | A fixed floor, `RETRY_AFTER_SECONDS_BY_STATUS` | `v2Error`, applied automatically | + +The 503 default is applied by `v2Error` keyed on the response *status* — `Retry-After` is defined against the status, and the status is the only half of the code/status pair a client sees — so every 503 the surface can emit carries it — the three route builders' `unhandledErrorResponse`, the execute and resume routes, and `serviceFailureResponse`'s `infra` failures. A route with a better number passes `headers: { 'Retry-After': … }` and wins. + +Do not add a default for any other code. 400/403/404/409 are not fixed by waiting, and 402 (`USAGE_LIMIT_EXCEEDED`) is resolved by a billing change, not by time. + +**Where a policy already knows the wait, carry it — do not re-guess it at the transport.** The admission descriptors in `lib/core/admission/transient-failure` declare `retryAfterSeconds` per denial. That value used to be dropped when the descriptor was mapped onto a preprocess error, so a concurrency denial arrived as a bare 429 with no `Retry-After` even though the policy had named the wait. It now travels `descriptor.retryAfterSeconds → PreprocessExecutionError.retryAfterMs → ExecuteWorkflowServiceFailure.retryAfterMs → serviceFailureResponse`. The `v2Error` default is the floor for paths with *no* policy signal, not the source of truth. + +**A failure whose outcome is unknown must not advise a retry.** `ASYNC_ENQUEUE_AMBIGUOUS` is a 503 whose enqueue may have succeeded — it deliberately retains its execution-ID claim. Telling that caller to come back in 5 seconds invites a client with no `X-Run-Id` to start and bill a second run. It passes `omitRetryAfter: true` and returns the run id so the caller reconciles instead. Any future "we don't know if it happened" failure does the same. + +RFC 9110 §10.2.3 gives 503 this field's clearest meaning — "how long the service is expected to be unavailable to the client". Note the requirement level is only `MAY`, on 503 (§15.6.4) and, via RFC 6585 §4, on 429. It is `SHOULD` on exactly one status, 413, and only when the condition is temporary; Sim's 413 is a fixed byte ceiling, so it correctly sends none. + +## Deliberate non-adoptions + +Audited against the primary specs and against Stripe, GitHub, and Google's AIPs. Each is a considered "no", not an oversight. Re-open one only with new evidence. + +| Practice | Verdict | Why | +|---|---|---| +| **RFC 9457 `application/problem+json`** | No | 9457 §4 steers APIs with an existing format toward keeping it: "Problem details are intended to avoid the necessity of establishing new 'fault' or 'error' document formats, **not to replace existing domain-specific formats**." Nothing in it is a `MUST` to adopt, and none of Stripe, GitHub, or Google use it. Our envelope is load-bearing for every client. **The default error shape does not change.** | +| **`RateLimit`/`RateLimit-Policy` (IETF draft)** | No | Still an unpublished draft (`-11`, May 2026), returned "Not ready" at HTTPDIR review, and on its **third mutually incompatible wire format** — anything built against `-07` or earlier is already broken. None of the three surveyed APIs emit it; GitHub uses `x-ratelimit-*`, as we do. | +| **Renaming `X-RateLimit-*` per RFC 6648** | No | 6648 is a `SHOULD NOT` binding *creators of new* parameters, and §1 item 4 "**makes no recommendation as to whether existing 'X-' parameters ought to remain in use or be migrated**". Appendix B argues the migration is itself the interoperability harm. A rename is a client-visible break bought with nothing. | +| **`X-RateLimit-Reset` as delta-seconds** | No | It is an absolute ISO 8601 timestamp, so it is clock-skew sensitive — but the response where timing actually decides behaviour (429) also carries `Retry-After`, which is skew-free. The absolute value stays useful for scheduling. | +| **422 for semantic validation** | No | RFC 9110 §15.5.21 defines 422, but Appendix B.3 records that 9110 **deleted** RFC 4918's clause saying 400 was inappropriate. 400 covers "cannot or will not process… perceived to be a client error". The split is convention, not requirement — GitHub splits, Stripe and Google do not. Our machine-readable `error.code` already carries the distinction, and restatusing now breaks clients. | +| **`Location` on 201** | No | §9.3.3 makes this a `SHOULD` **for POST**; the status code itself (§15.3.2) requires nothing and defines the fallback — absent `Location`, the target URI identifies the resource. Declined knowingly: several 201 responses (signed upload sessions, table exports, knowledge folders) have no canonical single-resource GET, so a `Location` would 404, and adopting it on some of the 19 is worse for a client than on none. Every 201 returns the full representation including its `id`. Revisit per-route if one gains a canonical GET. | +| **ETag / `If-None-Match` / `If-Match`** | No | Every v2 response is `Cache-Control: private, no-store` per-caller data, so `If-None-Match` buys nothing. For writes, `If-Match` needs a **strong** validator: §8.8.3.2's strong comparison fails if *either* tag is weak, so a weak ETag silently makes every `If-Match` fail. None of the three surveyed APIs does HTTP optimistic concurrency — Google does the semantics via a resource `etag` **field** (AIP-154), deliberately not the header. If Sim needs optimistic concurrency, do it that way. | +| **`Deprecation` / `Sunset` on v1** | Not yet | RFC 9745 (Standards Track) and RFC 8594 (Informational) both apply, and GitHub emits both. But `Sunset` is a timestamp and 9745 §4 makes `Sunset >= Deprecation` a `MUST`, so emitting either commits Sim to a v1 retirement date — a product decision, not an engineering one. When that date exists: `Deprecation` is an RFC 9651 Structured Field **Date** (`@1688169599`); `Sunset` is an **HTTP-date** (`Sat, 31 Dec 2033 23:59:59 GMT`). Two encodings in one response — the most common implementation error here. | +| **`application/merge-patch+json`** | No | v2 PATCH bodies are merge-patch *shaped* — absent means unchanged, `null` clears — but they are `.strict()`, so unknown members are rejected where RFC 7396 §2 would merge them, and nested objects are replaced wholesale rather than merged. Advertising the media type would over-claim. Document the semantics per contract instead. | + +## Idempotency: at-most-once, not replay + +`POST /workflows/{id}/execute` accepts `X-Run-Id`, a caller-supplied run identifier claimed through the `idempotency_key` table (`execution-id-claim.ts`). It is a **uniqueness claim, not an idempotency key**, and the distinction is deliberate and already published in the operation description: + +- First use wins and runs. +- Any reuse returns **409** with `error.details.code: "RUN_ID_CONFLICT"`, the run id in `error.details.runId`, and an `X-Run-Id` response header. It never replays the earlier run's result — the client recovers it by polling the runs resource. +- Claims are durable tombstones, so deleting execution logs cannot make an id reusable. + +That makes the money path safe against double-execution **for callers that opt in**. What it is not: a Stripe-style `Idempotency-Key` that stores and replays the original status and body. Building that means a request fingerprint, a retention window, an in-flight-vs-completed distinction (the expired IETF draft would have these be 422 and 409 respectively), and somewhere to put a large synchronous execution body. It is a designed piece of work, not an increment — do not half-build it by aliasing the header name, which would invite clients written against Stripe semantics to treat our 409 as a hard failure. + +## Cursors are opaque, not trusted + +The base64-JSON cursor is **not signed**, and does not need to be. Tampering is bounded by construction, and that is a property to preserve: + +- Every key value is re-validated by its `KeysetKey.bind`, which returns `null` for a wrong-typed or unparseable value and becomes a 400. A forged cursor cannot reach SQL as `NaN` or an `Invalid Date`. +- The sort is stamped into the cursor and re-checked (`decodeSortedCursor`), so a cursor from a differently-sorted query is a 400, not a silently skipped page. +- The offset codec rejects anything that is not a non-negative integer. +- Authorization is **never** carried in the cursor. Every list re-derives its workspace scope from the authenticated principal, so a cursor lifted from another query — or another tenant — can only move the caller within their own authorized result set. + +The consequence to keep true: **never put a resource id, filter, or scope into a cursor and then trust it on the way back.** A cursor is a position hint, never an input to an access decision. + ## Checklist Run this against any new or changed v2 endpoint. @@ -116,7 +172,8 @@ Run this against any new or changed v2 endpoint. - [ ] If the response carries `nextCursor`, the query accepts `limit` + `cursor` and the query actually applies them. - [ ] Keyset sorts end in a unique `id` key. - [ ] The list is classified in `list-pagination.test.ts`. -- [ ] Cross-tenant access answers 404, never 403. +- [ ] Cross-tenant access answers 404, never 403 — and carries `Cache-Control: private, no-store`, because RFC 9110 §15.5.5 makes 404 heuristically cacheable and an authorization-dependent 404 must never be stored. `v2Error` sets this unconditionally; do not build a v2 response any other way. +- [ ] A retryable failure says when: 429 and 503 carry `Retry-After`. No other status invents one. - [ ] 403s carry a machine-readable `details.code`. - [ ] Validation messages name the field and echo the valid set. - [ ] Response schema matches every field the route actually emits. diff --git a/apps/docs/openapi-v2-billing.json b/apps/docs/openapi-v2-billing.json index a03fc6a80eb..13ef0c04962 100644 --- a/apps/docs/openapi-v2-billing.json +++ b/apps/docs/openapi-v2-billing.json @@ -283,13 +283,13 @@ } }, "Retry-After": { - "description": "Seconds to wait before retrying a rate-limited request.", + "description": "Seconds to wait before retrying. Sent on `429` (derived from the caller rate-limit window) and on `503` (a fixed transient-failure floor). Add jitter rather than retrying at exactly this offset.", "schema": { "type": "integer", "minimum": 0, "maximum": 9007199254740991, "title": "Retry after", - "description": "Seconds to wait before retrying a rate-limited request." + "description": "Seconds to wait before retrying. Sent on `429` (derived from the caller rate-limit window) and on `503` (a fixed transient-failure floor). Add jitter rather than retrying at exactly this offset." } }, "X-Run-Id": { @@ -454,7 +454,12 @@ } }, "ServiceUnavailable": { - "description": "A required service is temporarily unavailable.", + "description": "A required service is temporarily unavailable. The condition is transient, so the response normally carries `Retry-After` with the number of seconds to wait; treat that value as a floor and add jitter before retrying. One case deliberately omits the header: when `error.details.code` is `ASYNC_ENQUEUE_AMBIGUOUS`, the run may already have started, so retrying could start and bill a second run. Reconcile against the returned run id instead of retrying.", + "headers": { + "Retry-After": { + "$ref": "#/components/headers/Retry-After" + } + }, "content": { "application/json": { "schema": { diff --git a/apps/docs/openapi-v2-files-audit.json b/apps/docs/openapi-v2-files-audit.json index 702da1a9924..971cfd08b59 100644 --- a/apps/docs/openapi-v2-files-audit.json +++ b/apps/docs/openapi-v2-files-audit.json @@ -1912,13 +1912,13 @@ } }, "Retry-After": { - "description": "Seconds to wait before retrying a rate-limited request.", + "description": "Seconds to wait before retrying. Sent on `429` (derived from the caller rate-limit window) and on `503` (a fixed transient-failure floor). Add jitter rather than retrying at exactly this offset.", "schema": { "type": "integer", "minimum": 0, "maximum": 9007199254740991, "title": "Retry after", - "description": "Seconds to wait before retrying a rate-limited request." + "description": "Seconds to wait before retrying. Sent on `429` (derived from the caller rate-limit window) and on `503` (a fixed transient-failure floor). Add jitter rather than retrying at exactly this offset." } }, "X-Run-Id": { @@ -2083,7 +2083,12 @@ } }, "ServiceUnavailable": { - "description": "A required service is temporarily unavailable.", + "description": "A required service is temporarily unavailable. The condition is transient, so the response normally carries `Retry-After` with the number of seconds to wait; treat that value as a floor and add jitter before retrying. One case deliberately omits the header: when `error.details.code` is `ASYNC_ENQUEUE_AMBIGUOUS`, the run may already have started, so retrying could start and bill a second run. Reconcile against the returned run id instead of retrying.", + "headers": { + "Retry-After": { + "$ref": "#/components/headers/Retry-After" + } + }, "content": { "application/json": { "schema": { diff --git a/apps/docs/openapi-v2-knowledge.json b/apps/docs/openapi-v2-knowledge.json index 2ec9e730198..ee3973a7e7a 100644 --- a/apps/docs/openapi-v2-knowledge.json +++ b/apps/docs/openapi-v2-knowledge.json @@ -1732,13 +1732,13 @@ } }, "Retry-After": { - "description": "Seconds to wait before retrying a rate-limited request.", + "description": "Seconds to wait before retrying. Sent on `429` (derived from the caller rate-limit window) and on `503` (a fixed transient-failure floor). Add jitter rather than retrying at exactly this offset.", "schema": { "type": "integer", "minimum": 0, "maximum": 9007199254740991, "title": "Retry after", - "description": "Seconds to wait before retrying a rate-limited request." + "description": "Seconds to wait before retrying. Sent on `429` (derived from the caller rate-limit window) and on `503` (a fixed transient-failure floor). Add jitter rather than retrying at exactly this offset." } }, "X-Run-Id": { @@ -1903,7 +1903,12 @@ } }, "ServiceUnavailable": { - "description": "A required service is temporarily unavailable.", + "description": "A required service is temporarily unavailable. The condition is transient, so the response normally carries `Retry-After` with the number of seconds to wait; treat that value as a floor and add jitter before retrying. One case deliberately omits the header: when `error.details.code` is `ASYNC_ENQUEUE_AMBIGUOUS`, the run may already have started, so retrying could start and bill a second run. Reconcile against the returned run id instead of retrying.", + "headers": { + "Retry-After": { + "$ref": "#/components/headers/Retry-After" + } + }, "content": { "application/json": { "schema": { diff --git a/apps/docs/openapi-v2-logs.json b/apps/docs/openapi-v2-logs.json index e38fe2e6026..7907c60e5ea 100644 --- a/apps/docs/openapi-v2-logs.json +++ b/apps/docs/openapi-v2-logs.json @@ -394,13 +394,13 @@ } }, "Retry-After": { - "description": "Seconds to wait before retrying a rate-limited request.", + "description": "Seconds to wait before retrying. Sent on `429` (derived from the caller rate-limit window) and on `503` (a fixed transient-failure floor). Add jitter rather than retrying at exactly this offset.", "schema": { "type": "integer", "minimum": 0, "maximum": 9007199254740991, "title": "Retry after", - "description": "Seconds to wait before retrying a rate-limited request." + "description": "Seconds to wait before retrying. Sent on `429` (derived from the caller rate-limit window) and on `503` (a fixed transient-failure floor). Add jitter rather than retrying at exactly this offset." } }, "X-Run-Id": { @@ -565,7 +565,12 @@ } }, "ServiceUnavailable": { - "description": "A required service is temporarily unavailable.", + "description": "A required service is temporarily unavailable. The condition is transient, so the response normally carries `Retry-After` with the number of seconds to wait; treat that value as a floor and add jitter before retrying. One case deliberately omits the header: when `error.details.code` is `ASYNC_ENQUEUE_AMBIGUOUS`, the run may already have started, so retrying could start and bill a second run. Reconcile against the returned run id instead of retrying.", + "headers": { + "Retry-After": { + "$ref": "#/components/headers/Retry-After" + } + }, "content": { "application/json": { "schema": { diff --git a/apps/docs/openapi-v2-resources.json b/apps/docs/openapi-v2-resources.json index 741b4b84251..a9aa61b7580 100644 --- a/apps/docs/openapi-v2-resources.json +++ b/apps/docs/openapi-v2-resources.json @@ -1938,13 +1938,13 @@ } }, "Retry-After": { - "description": "Seconds to wait before retrying a rate-limited request.", + "description": "Seconds to wait before retrying. Sent on `429` (derived from the caller rate-limit window) and on `503` (a fixed transient-failure floor). Add jitter rather than retrying at exactly this offset.", "schema": { "type": "integer", "minimum": 0, "maximum": 9007199254740991, "title": "Retry after", - "description": "Seconds to wait before retrying a rate-limited request." + "description": "Seconds to wait before retrying. Sent on `429` (derived from the caller rate-limit window) and on `503` (a fixed transient-failure floor). Add jitter rather than retrying at exactly this offset." } }, "X-Run-Id": { @@ -2109,7 +2109,12 @@ } }, "ServiceUnavailable": { - "description": "A required service is temporarily unavailable.", + "description": "A required service is temporarily unavailable. The condition is transient, so the response normally carries `Retry-After` with the number of seconds to wait; treat that value as a floor and add jitter before retrying. One case deliberately omits the header: when `error.details.code` is `ASYNC_ENQUEUE_AMBIGUOUS`, the run may already have started, so retrying could start and bill a second run. Reconcile against the returned run id instead of retrying.", + "headers": { + "Retry-After": { + "$ref": "#/components/headers/Retry-After" + } + }, "content": { "application/json": { "schema": { diff --git a/apps/docs/openapi-v2-tables.json b/apps/docs/openapi-v2-tables.json index 64601b2a7ef..66ef47c1c0c 100644 --- a/apps/docs/openapi-v2-tables.json +++ b/apps/docs/openapi-v2-tables.json @@ -3622,13 +3622,13 @@ } }, "Retry-After": { - "description": "Seconds to wait before retrying a rate-limited request.", + "description": "Seconds to wait before retrying. Sent on `429` (derived from the caller rate-limit window) and on `503` (a fixed transient-failure floor). Add jitter rather than retrying at exactly this offset.", "schema": { "type": "integer", "minimum": 0, "maximum": 9007199254740991, "title": "Retry after", - "description": "Seconds to wait before retrying a rate-limited request." + "description": "Seconds to wait before retrying. Sent on `429` (derived from the caller rate-limit window) and on `503` (a fixed transient-failure floor). Add jitter rather than retrying at exactly this offset." } }, "X-Run-Id": { @@ -3793,7 +3793,12 @@ } }, "ServiceUnavailable": { - "description": "A required service is temporarily unavailable.", + "description": "A required service is temporarily unavailable. The condition is transient, so the response normally carries `Retry-After` with the number of seconds to wait; treat that value as a floor and add jitter before retrying. One case deliberately omits the header: when `error.details.code` is `ASYNC_ENQUEUE_AMBIGUOUS`, the run may already have started, so retrying could start and bill a second run. Reconcile against the returned run id instead of retrying.", + "headers": { + "Retry-After": { + "$ref": "#/components/headers/Retry-After" + } + }, "content": { "application/json": { "schema": { diff --git a/apps/docs/openapi-v2-workflows.json b/apps/docs/openapi-v2-workflows.json index 77cb3151340..65348031bc5 100644 --- a/apps/docs/openapi-v2-workflows.json +++ b/apps/docs/openapi-v2-workflows.json @@ -2018,13 +2018,13 @@ } }, "Retry-After": { - "description": "Seconds to wait before retrying a rate-limited request.", + "description": "Seconds to wait before retrying. Sent on `429` (derived from the caller rate-limit window) and on `503` (a fixed transient-failure floor). Add jitter rather than retrying at exactly this offset.", "schema": { "type": "integer", "minimum": 0, "maximum": 9007199254740991, "title": "Retry after", - "description": "Seconds to wait before retrying a rate-limited request." + "description": "Seconds to wait before retrying. Sent on `429` (derived from the caller rate-limit window) and on `503` (a fixed transient-failure floor). Add jitter rather than retrying at exactly this offset." } }, "X-Run-Id": { @@ -2189,7 +2189,12 @@ } }, "ServiceUnavailable": { - "description": "A required service is temporarily unavailable.", + "description": "A required service is temporarily unavailable. The condition is transient, so the response normally carries `Retry-After` with the number of seconds to wait; treat that value as a floor and add jitter before retrying. One case deliberately omits the header: when `error.details.code` is `ASYNC_ENQUEUE_AMBIGUOUS`, the run may already have started, so retrying could start and bill a second run. Reconcile against the returned run id instead of retrying.", + "headers": { + "Retry-After": { + "$ref": "#/components/headers/Retry-After" + } + }, "content": { "application/json": { "schema": { diff --git a/apps/sim/app/api/v2/lib/response.test.ts b/apps/sim/app/api/v2/lib/response.test.ts new file mode 100644 index 00000000000..f84dd84a203 --- /dev/null +++ b/apps/sim/app/api/v2/lib/response.test.ts @@ -0,0 +1,48 @@ +/** + * @vitest-environment node + */ +import { describe, expect, it } from 'vitest' +import { v2Error } from '@/app/api/v2/lib/response' + +describe('v2Error retry guidance', () => { + it('sends Retry-After on 503 so a client does not retry a degraded dependency immediately', () => { + const response = v2Error('SERVICE_UNAVAILABLE', 'Service temporarily unavailable') + + expect(response.status).toBe(503) + const retryAfter = response.headers.get('Retry-After') + expect(retryAfter).not.toBeNull() + expect(Number(retryAfter)).toBeGreaterThan(0) + expect(Number.isInteger(Number(retryAfter))).toBe(true) + }) + + it('lets a caller-supplied Retry-After win over the default', () => { + const response = v2Error('SERVICE_UNAVAILABLE', 'Service temporarily unavailable', { + headers: { 'Retry-After': '30' }, + }) + + expect(response.headers.get('Retry-After')).toBe('30') + }) + + it('does not invent Retry-After for failures a retry cannot fix', () => { + for (const code of ['BAD_REQUEST', 'NOT_FOUND', 'FORBIDDEN', 'CONFLICT'] as const) { + expect(v2Error(code, 'nope').headers.get('Retry-After')).toBeNull() + } + }) + + it('does not default Retry-After on 429, whose wait comes from the token bucket', () => { + expect(v2Error('RATE_LIMITED', 'API rate limit exceeded').headers.get('Retry-After')).toBeNull() + }) + + it('stays silent on retrying when the outcome is unknown rather than absent', () => { + const response = v2Error( + 'SERVICE_UNAVAILABLE', + 'Async execution queue acceptance unconfirmed', + { + omitRetryAfter: true, + } + ) + + expect(response.status).toBe(503) + expect(response.headers.get('Retry-After')).toBeNull() + }) +}) diff --git a/apps/sim/app/api/v2/lib/response.ts b/apps/sim/app/api/v2/lib/response.ts index 93c983828cb..d619a3c2138 100644 --- a/apps/sim/app/api/v2/lib/response.ts +++ b/apps/sim/app/api/v2/lib/response.ts @@ -2,6 +2,7 @@ import { NextResponse } from 'next/server' import type { ZodError } from 'zod' import { type CursorKey, INVALID_CURSOR_MESSAGE } from '@/lib/api/list-query' import { getValidationErrorMessage, serializeZodIssues } from '@/lib/api/server' +import { ADMISSION_RETRY_AFTER_SECONDS } from '@/lib/core/admission/transient-failure' import { asOrchestrationError, OrchestrationError, @@ -59,6 +60,44 @@ const V2_CODE_BY_HTTP_STATUS: Partial> = Object.from */ const PRIVATE_NO_STORE = { 'Cache-Control': 'private, no-store' } as const +/** + * Seconds a caller should wait before retrying a transient v2 failure, for the + * statuses whose response carries no other timing signal. + * + * Keyed on the response status rather than the v2 error code because + * `Retry-After` is defined against the status, and the status is the only half + * of the pair a client actually sees. `v2Error` lets a caller override the + * status independently of the code, so keying on the code would let the two + * disagree. + * + * RFC 9110 §10.2.3 singles out 503 as the status whose `Retry-After` means "how + * long the service is expected to be unavailable to the client", and §15.6.4 + * permits one. Note the requirement level is `MAY`, so this is a deliberate + * improvement on the baseline rather than a conformance fix: without it a + * client's only defensible policy on a 503 is an immediate retry, which is + * exactly the traffic a degraded dependency cannot absorb. Sim raises 503 when + * the API-key store, the rollout gate, the rate-limit backend, or + * execution-identity allocation is briefly unavailable, and all four are made + * worse by an unthrottled retry storm. + * + * 429 is deliberately absent because every 429 already knows its own wait: the + * throttle path measures it from the caller's token bucket + * ({@link v2RateLimitError}), and an admission denial carries the descriptor's + * declared `retryAfterSeconds` through to the route. Defaulting it here would + * paper over a path that had simply dropped its value — which is exactly the + * bug that used to leave a concurrency denial with no `Retry-After` at all. + * + * The value is Sim's one transient-failure floor, shared with the admission + * descriptors so the execute route's capacity 429 and every other surface's 503 + * cannot drift apart. It is a floor, not a schedule: a fleet that retries at + * exactly this offset re-converges into a single burst, so callers should still + * add jitter — `backoffWithJitter` from `@sim/utils/retry` is what Sim's own + * clients use. + */ +const RETRY_AFTER_SECONDS_BY_STATUS: Partial> = { + 503: ADMISSION_RETRY_AFTER_SECONDS, +} + type RateLimitHeaderSource = Pick export function rateLimitHeaders(rateLimit?: RateLimitHeaderSource): Record { @@ -104,6 +143,19 @@ interface V2ErrorOptions { status?: number details?: unknown headers?: Record + /** + * Suppresses the code's default `Retry-After` for a failure whose outcome is + * *unknown* rather than *absent*. + * + * A 503 normally means the work did not happen, so "come back in 5 seconds" + * is safe advice. The async enqueue that could not be confirmed + * (`ASYNC_ENQUEUE_AMBIGUOUS`) is the exception: it deliberately retains its + * execution-ID claim because a job may already exist. Telling that caller to + * retry invites a client with no `X-Run-Id` to start a second run of the same + * workflow, which bills twice. It must reconcile against the run id the + * response returns instead, so the response stays silent on retrying. + */ + omitRetryAfter?: boolean } /** `{ error: { code, message, details? } }`. */ @@ -114,11 +166,19 @@ export function v2Error( ): NextResponse { const error: { code: V2ErrorCode; message: string; details?: unknown } = { code, message } if (options.details !== undefined) error.details = options.details + const status = options.status ?? STATUS_BY_CODE[code] + const retryAfterSeconds = options.omitRetryAfter + ? undefined + : RETRY_AFTER_SECONDS_BY_STATUS[status] return NextResponse.json( { error }, { - status: options.status ?? STATUS_BY_CODE[code], - headers: { ...PRIVATE_NO_STORE, ...options.headers }, + status, + headers: { + ...PRIVATE_NO_STORE, + ...(retryAfterSeconds === undefined ? {} : { 'Retry-After': retryAfterSeconds.toString() }), + ...options.headers, + }, } ) } diff --git a/apps/sim/app/api/v2/workflows/[id]/execute/route.test.ts b/apps/sim/app/api/v2/workflows/[id]/execute/route.test.ts index 9d5f5aeeb17..8d2abfda872 100644 --- a/apps/sim/app/api/v2/workflows/[id]/execute/route.test.ts +++ b/apps/sim/app/api/v2/workflows/[id]/execute/route.test.ts @@ -536,6 +536,39 @@ describe('POST /api/v2/workflows/[id]/execute', () => { expect((await res.json()).error.code).toBe('RATE_LIMITED') }) + it('tells a client how long to wait when a dependency is briefly unavailable', async () => { + mockPreprocessExecution.mockResolvedValue({ + success: false, + error: { + message: 'Workflow execution identity is temporarily unavailable', + statusCode: 503, + }, + }) + + const res = await callExecute({ input: {} }) + + expect(res.status).toBe(503) + expect(Number(res.headers.get('Retry-After'))).toBeGreaterThan(0) + }) + + it('never advises a retry when an enqueue may already have started a run', async () => { + mockPreprocessExecution.mockResolvedValue({ + success: false, + error: { + message: 'Async execution queue acceptance could not be confirmed', + statusCode: 503, + code: 'ASYNC_ENQUEUE_AMBIGUOUS', + }, + }) + + const res = await callExecute({ input: {} }) + + expect(res.status).toBe(503) + // Retrying without X-Run-Id would start, and bill, a second run of the same workflow. + expect(res.headers.get('Retry-After')).toBeNull() + expect((await res.json()).error.details.code).toBe('ASYNC_ENQUEUE_AMBIGUOUS') + }) + it('runs the anonymous public path sync but refuses async', async () => { dbChainMockFns.limit.mockReset() dbChainMockFns.limit.mockResolvedValueOnce([ diff --git a/apps/sim/app/api/v2/workflows/[id]/execute/route.ts b/apps/sim/app/api/v2/workflows/[id]/execute/route.ts index 95015eb4055..ae363a04dee 100644 --- a/apps/sim/app/api/v2/workflows/[id]/execute/route.ts +++ b/apps/sim/app/api/v2/workflows/[id]/execute/route.ts @@ -83,6 +83,8 @@ function serviceFailureResponse(failure: ExecuteWorkflowServiceFailure) { return v2Error(code, isRunIdConflict ? 'Run ID has already been used' : failure.message, { status: failure.statusCode, headers, + /** An unconfirmed enqueue may already have started a run — reconcile on `runId`, never retry blind. */ + omitRetryAfter: failure.code === 'ASYNC_ENQUEUE_AMBIGUOUS', details: detailCode || failure.executionId ? { diff --git a/apps/sim/lib/api/contracts/v2/openapi/shared.ts b/apps/sim/lib/api/contracts/v2/openapi/shared.ts index 473def99c60..f07a62c99fd 100644 --- a/apps/sim/lib/api/contracts/v2/openapi/shared.ts +++ b/apps/sim/lib/api/contracts/v2/openapi/shared.ts @@ -67,7 +67,9 @@ export const ERROR_RESPONSES = { InternalError: { status: 500, description: 'An unexpected server error occurred.' }, ServiceUnavailable: { status: 503, - description: 'A required service is temporarily unavailable.', + description: + 'A required service is temporarily unavailable. The condition is transient, so the response normally carries `Retry-After` with the number of seconds to wait; treat that value as a floor and add jitter before retrying. One case deliberately omits the header: when `error.details.code` is `ASYNC_ENQUEUE_AMBIGUOUS`, the run may already have started, so retrying could start and bill a second run. Reconcile against the returned run id instead of retrying.', + headers: ['Retry-After'], }, } as const satisfies Readonly> @@ -188,7 +190,8 @@ export const V2_COMMON_HEADERS = { schema: z.number().int().nonnegative().meta({ id: 'RetryAfterHeader', title: 'Retry after', - description: 'Seconds to wait before retrying a rate-limited request.', + description: + 'Seconds to wait before retrying. Sent on `429` (derived from the caller rate-limit window) and on `503` (a fixed transient-failure floor). Add jitter rather than retrying at exactly this offset.', }), }, 'X-Run-Id': { diff --git a/apps/sim/lib/billing/calculations/usage-reservation.ts b/apps/sim/lib/billing/calculations/usage-reservation.ts index e4c4db5fd1a..aa7caf3311b 100644 --- a/apps/sim/lib/billing/calculations/usage-reservation.ts +++ b/apps/sim/lib/billing/calculations/usage-reservation.ts @@ -235,6 +235,8 @@ export class UsageReservationUnavailableError extends Error { readonly code = ADMISSION_ERROR_DESCRIPTOR.RESERVATION_INFRASTRUCTURE.code readonly statusCode = ADMISSION_ERROR_DESCRIPTOR.RESERVATION_INFRASTRUCTURE.statusCode readonly retryable = ADMISSION_ERROR_DESCRIPTOR.RESERVATION_INFRASTRUCTURE.retryable + readonly retryAfterSeconds = + ADMISSION_ERROR_DESCRIPTOR.RESERVATION_INFRASTRUCTURE.retryAfterSeconds constructor(message: string, cause?: unknown) { super(message) diff --git a/apps/sim/lib/execution/preprocessing.test.ts b/apps/sim/lib/execution/preprocessing.test.ts index 1fef3ecbe3a..a39e4b500af 100644 --- a/apps/sim/lib/execution/preprocessing.test.ts +++ b/apps/sim/lib/execution/preprocessing.test.ts @@ -34,6 +34,8 @@ vi.mock('@/lib/billing/calculations/usage-reservation', () => ({ readonly code = 'SERVICE_OVERLOADED' readonly statusCode = 503 readonly retryable = true + /** Mirrors ADMISSION_ERROR_DESCRIPTOR.RESERVATION_INFRASTRUCTURE. */ + readonly retryAfterSeconds = 5 }, })) vi.mock('@/lib/billing/core/billing-attribution', () => ({ @@ -671,6 +673,8 @@ describe('preprocessExecution billing attribution', () => { statusCode: 429, code: ADMISSION_ERROR_CODE.RESERVATION_CONCURRENCY, retryable: true, + /** A retryable denial must carry the descriptor's declared wait to the transport. */ + retryAfterMs: 5000, message: 'Too many concurrent executions', }, { @@ -678,6 +682,8 @@ describe('preprocessExecution billing attribution', () => { statusCode: 402, code: ADMISSION_ERROR_CODE.RESERVATION_PAYER_HEADROOM, retryable: false, + /** Waiting does not fix a billing limit, so no retry pacing is offered. */ + retryAfterMs: undefined, message: 'billing account has no guaranteed base-charge headroom', }, { @@ -685,11 +691,12 @@ describe('preprocessExecution billing attribution', () => { statusCode: 402, code: ADMISSION_ERROR_CODE.RESERVATION_MEMBER_HEADROOM, retryable: false, + retryAfterMs: undefined, message: 'organization member usage limit has no guaranteed base-charge headroom', }, ])( 'maps $reason to stable admission metadata while retaining local wording', - async ({ reason, statusCode, code, retryable, message }) => { + async ({ reason, statusCode, code, retryable, retryAfterMs, message }) => { mockCheckAttributedUsageLimits.mockResolvedValueOnce({ isExceeded: false, payerUsage: { currentUsage: 1, limit: 10 }, @@ -717,6 +724,7 @@ describe('preprocessExecution billing attribution', () => { }) if (result.success) throw new Error('Expected preprocessing to reject the reservation') expect(result.error.message).toContain(message) + expect(result.error.retryAfterMs).toBe(retryAfterMs) } ) @@ -734,6 +742,7 @@ describe('preprocessExecution billing attribution', () => { error: { statusCode: 503, retryable: true, + retryAfterMs: 5000, code: ADMISSION_ERROR_CODE.RESERVATION_INFRASTRUCTURE, cause: { code: 'SERVICE_OVERLOADED' }, }, diff --git a/apps/sim/lib/execution/preprocessing.ts b/apps/sim/lib/execution/preprocessing.ts index d111187d0fa..84f41b41c41 100644 --- a/apps/sim/lib/execution/preprocessing.ts +++ b/apps/sim/lib/execution/preprocessing.ts @@ -16,6 +16,7 @@ import { import type { HighestPrioritySubscription } from '@/lib/billing/core/plan' import { getHighestPrioritySubscription } from '@/lib/billing/core/subscription' import { + type AdmissionErrorDescriptor, getReservationDenialDescriptor, type ReservationDenialReason, } from '@/lib/core/admission/transient-failure' @@ -111,11 +112,30 @@ export interface PreprocessExecutionError { statusCode: number code?: string retryable?: boolean - /** Populated on rate-limit denials so callers can emit `Retry-After`. */ + /** + * How long the caller should wait before retrying, so surfaces can emit + * `Retry-After`. Set by rate-limit denials from the token bucket, and by + * admission denials from their descriptor's declared `retryAfterSeconds`. + */ retryAfterMs?: number cause?: Record } +/** + * Carries an admission descriptor's declared retry pacing to the transport, + * which speaks milliseconds. + * + * The descriptors in `lib/core/admission/transient-failure` already decide how + * long each denial should hold a caller off, but that value used to stop here: + * only `statusCode`, `code`, and `retryable` were copied onto the preprocess + * error, so a concurrency denial reached the client as a bare `429` with no + * `Retry-After` despite the policy layer having named the wait. The transport + * is not the right place to re-guess a number the policy already owns. + */ +function retryAfterMsFrom(retryAfterSeconds: number | undefined): { retryAfterMs?: number } { + return retryAfterSeconds === undefined ? {} : { retryAfterMs: retryAfterSeconds * 1000 } +} + export const WORKFLOW_NOT_DEPLOYED_CODE = 'WORKFLOW_NOT_DEPLOYED' export interface PreprocessExecutionSuccess { @@ -729,7 +749,14 @@ export async function preprocessExecution( }) if (!reservation.reserved) { - const descriptor = getReservationDenialDescriptor(reservation.reason) + /** + * Widened to the declared interface so the optional `retryAfterSeconds` + * is readable: the const descriptors narrow to literal shapes where the + * non-retryable 402 members simply omit the key. + */ + const descriptor: AdmissionErrorDescriptor = getReservationDenialDescriptor( + reservation.reason + ) const message = RESERVATION_DENIAL_MESSAGE[reservation.reason] logger.warn(`[${requestId}] Admission reservation full for user ${actorUserId}`, { workflowId, @@ -756,6 +783,7 @@ export async function preprocessExecution( statusCode: descriptor.statusCode, code: descriptor.code, retryable: descriptor.retryable, + ...retryAfterMsFrom(descriptor.retryAfterSeconds), cause: { code: descriptor.code, constraint: reservation.reason, @@ -782,6 +810,7 @@ export async function preprocessExecution( statusCode: unavailable.statusCode, code: unavailable.code, retryable: unavailable.retryable, + ...retryAfterMsFrom(unavailable.retryAfterSeconds), cause: { code: unavailable.code, },