mirror of
https://github.com/simstudioai/sim.git
synced 2026-09-24 15:45:35 +08:00
fix(v2): tell a caller when to come back on every failure meant to be retried (#6625)
* fix(v2): tell a caller when to come back on every failure meant to be retried Three related gaps in retry signalling, found auditing the v2 surface against RFC 9110/6585 and against how Stripe, GitHub and Google's AIPs handle the same problems. **No 503 carried `Retry-After`.** Every one of them — the three route builders' `unhandledErrorResponse`, the execute and resume routes, and `serviceFailureResponse` — funnels through `v2Error`, so the default lands there, 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. A caller that supplies its own value still wins. RFC 9110 §15.6.4 makes this a `MAY` rather than a `SHOULD`, so it is a deliberate improvement, not a conformance fix: without it a client's only defensible policy on a 503 is an immediate retry, and Sim raises 503 exactly when a dependency is too degraded to absorb one. **A 429 that already knew its wait threw it away.** The admission descriptors declare `retryAfterSeconds` per denial, but mapping a descriptor onto a preprocess error copied only `statusCode`, `code` and `retryable`. A concurrency denial therefore reached the client as a bare 429 with no `Retry-After` despite the policy layer having named the wait five seconds earlier. The value now travels `descriptor.retryAfterSeconds` → `PreprocessExecutionError.retryAfterMs` → `ExecuteWorkflowServiceFailure.retryAfterMs` → `serviceFailureResponse`, so the transport reads a number the policy owns instead of re-guessing one. The 503 default is now only the floor for paths with no policy signal. **One failure must not advise a retry at all.** `ASYNC_ENQUEUE_AMBIGUOUS` is a 503 whose enqueue may have succeeded — it deliberately retains its execution-ID claim because a job may already exist. Telling that caller to come back in five seconds invites a client with no `X-Run-Id` to start, and bill, a second run of the same workflow. It opts out via `omitRetryAfter` and returns the run id so the caller reconciles instead. `ADMISSION_RETRY_AFTER_SECONDS` is reused rather than restated, so the execute route's capacity 429 and every other surface's 503 cannot drift apart. Also records the audit in `.agents/skills/v2-api-conventions/SKILL.md`: the retry rule, the cursor-tampering invariants, and reasoned rejections of RFC 9457 problem+json, the `RateLimit-*` draft fields, renaming `X-RateLimit-*` under RFC 6648, 422-for-semantic-validation, `Location` on 201, ETag/`If-Match`, and `merge-patch+json` — each with the spec text and the industry evidence, so they are not re-litigated. `Deprecation`/`Sunset` on v1 is left open pending a retirement date, which is a product decision. * docs(v2): name the one 503 that omits Retry-After in the shared contract The shared ServiceUnavailable description claimed every 503 carries the header, which the ASYNC_ENQUEUE_AMBIGUOUS response deliberately does not. It now says the header is normally present and names that exception, so the published contract matches the runtime behaviour for all 128 operations.
This commit is contained in:
@@ -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/<domain>.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.
|
||||
|
||||
@@ -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/<domain>.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.
|
||||
|
||||
@@ -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/<domain>.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.
|
||||
|
||||
@@ -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": {
|
||||
|
||||
@@ -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": {
|
||||
|
||||
@@ -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": {
|
||||
|
||||
@@ -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": {
|
||||
|
||||
@@ -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": {
|
||||
|
||||
@@ -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": {
|
||||
|
||||
@@ -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": {
|
||||
|
||||
@@ -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()
|
||||
})
|
||||
})
|
||||
@@ -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<Record<number, V2ErrorCode>> = 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<Record<number, number>> = {
|
||||
503: ADMISSION_RETRY_AFTER_SECONDS,
|
||||
}
|
||||
|
||||
type RateLimitHeaderSource = Pick<RateLimitResult, 'limit' | 'remaining' | 'resetAt'>
|
||||
|
||||
export function rateLimitHeaders(rateLimit?: RateLimitHeaderSource): Record<string, string> {
|
||||
@@ -104,6 +143,19 @@ interface V2ErrorOptions {
|
||||
status?: number
|
||||
details?: unknown
|
||||
headers?: Record<string, string>
|
||||
/**
|
||||
* 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,
|
||||
},
|
||||
}
|
||||
)
|
||||
}
|
||||
|
||||
@@ -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([
|
||||
|
||||
@@ -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
|
||||
? {
|
||||
|
||||
@@ -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<Record<string, OpenApiErrorResponse>>
|
||||
|
||||
@@ -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': {
|
||||
|
||||
@@ -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)
|
||||
|
||||
@@ -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' },
|
||||
},
|
||||
|
||||
@@ -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<string, unknown>
|
||||
}
|
||||
|
||||
/**
|
||||
* 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,
|
||||
},
|
||||
|
||||
Reference in New Issue
Block a user