docs(oseps): add OSEP-0016 resilient SDK transport - #1368
Conversation
github-actions
Bot
added
documentation
ccf2c47 to
5dbbc2c
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ccf2c4785e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
5dbbc2c to
599950d
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 599950d456
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
599950d to
1f75b28
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1f75b28bca
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
1f75b28 to
3217cb7
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3217cb7933
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
3217cb7 to
624adf2
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 624adf2239
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
I suggest making status-code retries for non-idempotent methods opt-in rather than enabled by default: The default policy would then be:
A This default keeps the safety boundary simple: the SDK automatically recovers where replay is demonstrably safe, while availability-vs-duplication trade-offs for mutating operations require explicit caller consent. Once a server-side idempotency-key protocol exists, the default non-idempotent retry set can be expanded safely. |
624adf2 to
d91ed9e
Compare
github-actions
Bot
added
size/XXL
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d91ed9ed87
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Propose a client-only resilience layer for all five SDKs (Python, JavaScript, Kotlin, Go, C#) to survive routine infrastructure blips without any server-side change. The proposal defines a shared RetryPolicy contract, a retry decision algorithm asymmetric across idempotent vs. non-idempotent methods, decorrelated jitter, Retry-After handling with a bounded cap, and an extended exception hierarchy with a machine-checkable is_retryable accessor. Non-idempotent methods (POST/PATCH) retry only on locally provable side-effect-free conditions: pre-send transport failures (DNS, TCP connect, TLS handshake, fresh-conn RST), 429, and 502. Idempotent methods retry on the full transient set including 408, 425, 429, 500, 502, 503, 504 plus all transport errors. No spec change, no wire-format change, no server change.
d91ed9e to
8fd7f41
Compare
github-actions
Bot
added
size/XL
Thanks — this is exactly the framing the OSEP needed. Adopted in commit The prior default of Concrete changes in commit Contract:
Retry Decision Algorithm & Matrix:
Motivation / Goals / Non-Goals / Requirements:
Opt-in path (Configuration Surface):
Go migration:
Risks & Alternatives:
Test Plan:
This also happens to resolve a self-contradiction that persisted through earlier review rounds: the OSEP repeatedly claimed "SDK only retries on locally-provable-safe conditions" while carrying Future direction: if operational data ( |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8fd7f4124b
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 260d601c10
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| `Retry-After` handling (when `respect_retry_after=true`): if the | ||
| response carries `Retry-After` (delta-seconds or HTTP-date), the | ||
| next wait is `min(retry_after_value, retry_after_cap)`. Unparseable | ||
| values are ignored and the computed backoff is used. |
There was a problem hiding this comment.
When a server sends an HTTP-date Retry-After that is already in the past (or the client clock is ahead), retry_after_value becomes negative, but the rule passes it directly to min(...). Different SDK runtimes handle negative sleeps differently, so the promised shared behavior can diverge on stale gateway headers; clamp parsed dates to zero or ignore past dates before applying the cap.
AGENTS.md reference: AGENTS.md:L119-L122
Useful? React with 👍 / 👎.
| - Fresh-conn recovery triggers on `ECONNRESET` from a pooled | ||
| connection for `GET` and `POST` (pre-send): attempt count = 2. |
There was a problem hiding this comment.
This vector makes pooled ECONNRESET pre-send recovery stop after 2 attempts for both GET and POST, but the decision algorithm's PreSendFailure branch uses the normal retry budget and the earlier reset-before-first-byte vector expects 4 attempts. A shared retry-vector file cannot encode both outcomes, so SDKs will diverge on the same pooled pre-send reset unless fresh-conn recovery is explicitly a one-shot special case for all affected methods or this vector is removed.
AGENTS.md reference: AGENTS.md:L119-L122
Useful? React with 👍 / 👎.
| - With `{429, 502}`: retry on 429 for `POST`, attempt count = 2; | ||
| retry on 502 for `POST`, attempt count = 4, same body each | ||
| attempt. |
There was a problem hiding this comment.
This shared opt-in test promises the same POST body on every 502 retry, but only the JS/Kotlin sections define replayability. The current Python filesystem upload path can send async-generator/file-stream bodies, and C# maps Stream uploads to StreamContent; a transport-level retry cannot reliably rebuild those consumed bodies, and C# also cannot resend the same HttpRequestMessage. Require buffering/body factories or single-attempt fallback for these SDKs too.
AGENTS.md reference: AGENTS.md:L119-L122
Useful? React with 👍 / 👎.
| `Retry-After` handling (when `respect_retry_after=true`): if the | ||
| response carries `Retry-After` (delta-seconds or HTTP-date), the | ||
| next wait is `min(retry_after_value, retry_after_cap)`. Unparseable | ||
| values are ignored and the computed backoff is used. |
There was a problem hiding this comment.
When a server sends an HTTP-date Retry-After that is already in the past (or the client clock is ahead), retry_after_value becomes negative, but the rule passes it directly to min(...). Different SDK runtimes handle negative sleeps differently, so the promised shared behavior can diverge on stale gateway headers; clamp parsed dates to zero or ignore past dates before applying the cap.
Useful? React with 👍 / 👎.
| - Fresh-conn recovery triggers on `ECONNRESET` from a pooled | ||
| connection for `GET` and `POST` (pre-send): attempt count = 2. |
There was a problem hiding this comment.
This vector makes pooled ECONNRESET pre-send recovery stop after 2 attempts for both GET and POST, but the decision algorithm's PreSendFailure branch uses the normal retry budget and the earlier reset-before-first-byte vector expects 4 attempts. A shared retry-vector file cannot encode both outcomes, so SDKs will diverge on the same pooled pre-send reset unless fresh-conn recovery is explicitly a one-shot special case for all affected methods or this vector is removed.
Useful? React with 👍 / 👎.
| **Kotlin** (`sdks/sandbox/kotlin`). A new `RetryInterceptor` | ||
| implementing `okhttp3.Interceptor` is installed on `httpClient` and | ||
| `authenticatedClient` in `HttpClientProvider`; the SSE `sseClient` | ||
| is deliberately excluded. OkHttp `RequestBody` is re-readable by |
There was a problem hiding this comment.
OkHttp enables retryOnConnectionFailure by default, so a stale pooled connection or alternate-route failure can be retried inside OkHttp before this RetryInterceptor observes a failure. In Kotlin, that bypasses RetryPolicy.disabled(), attempt counts, logs, and the shared status/transport decisions; explicitly set retryOnConnectionFailure(false) on the clients that the SDK retry engine owns.
Useful? React with 👍 / 👎.
| Go SDK's existing `APIError.IsTransient()` is preserved as the | ||
| Go-idiomatic accessor. |
There was a problem hiding this comment.
The current Go SDK exposes APIError.IsTransient() as a status-only public method, but this proposal also requires the retryable accessor to reflect method, configured status sets, retry budget, deadline, and cancellation. Reusing IsTransient() for the new policy-derived value either breaks existing Go callers/tests that construct APIError{StatusCode: 503} or leaves Go inconsistent under disabled()/custom status sets; keep IsTransient() intrinsic and add a separate policy-aware accessor or stored field.
Useful? React with 👍 / 👎.
| identical defaults. `internal/lifecycle_metrics.py` is updated to | ||
| reuse the shared transport rather than constructing its own. |
There was a problem hiding this comment.
If internal/lifecycle_metrics.py reuses the caller's retry-wrapped transport, then a user who opts POST into status retries for business calls also makes the best-effort POST /metrics/events retry on 502/429. A lost response after the server recorded the event would double-count sandbox.create telemetry, so internal metrics reporting should use an unwrapped/no-retry transport regardless of the user's operation policy.
Useful? React with 👍 / 👎.
| - Enable retry by default (currently opt-in). Add | ||
| `per_attempt_timeout` and `overall_deadline`. | ||
| - Remove `withRetry` from `doStreamRequest` |
There was a problem hiding this comment.
Go's net/http.Transport can retry idempotent requests on a previously used connection before the SDK wrapper sees the failure, so a stale pooled GET may be issued twice even when RetryPolicy.disabled() is supposed to restore true fast-fail behavior. The Go landing plan needs to neutralize or account for that built-in path; otherwise disabled policy, attempt counts, and retry observability are not enforceable for Go.
Useful? React with 👍 / 👎.
| - Counter `opensandbox.sdk.retry.attempts` with dimensions | ||
| `{sdk_language, method, endpoint, cause, idempotent}`. |
There was a problem hiding this comment.
The retry metrics require an endpoint dimension but the event contract only carries a raw url, which for execd includes per-sandbox hosts/ports and for several APIs includes path parameters. If SDKs emit those values literally, retry metrics become high-cardinality and conflict with the existing OTel guidance to use route templates; define endpoint as the OpenAPI route/schema path instead.
Useful? React with 👍 / 👎.
4 participants
Summary
Adds OSEP-0016 proposing a client-only resilience layer for all five OpenSandbox SDKs (Python, JavaScript/TypeScript, Kotlin, Go, C#) to absorb routine network infrastructure blips without any server-side change.
Context
An audit of the five SDKs shows that, aside from Go, none has any HTTP-level retry. Even the Go SDK retries
POSTblindly on429/502/503/504, which is unsafe once the network fault produces a "sent request but lost response" race.Proposal in one paragraph
Every SDK ships a shared
RetryPolicycontract with identical semantics across languages, an extended exception hierarchy with a machine-checkableis_retryableaccessor, decorrelated jitter, andRetry-Afterhandling with a bounded cap. The retry engine is injected at the same layer each SDK already uses for transport concerns (httpx transport, openapi-fetch middleware, OkHttpInterceptor,net/http.RoundTripper,DelegatingHandler). Generated OpenAPI code is not touched.Safety invariant
The
POST/PATCHretry set is deliberately narrow: only conditions that can be locally proven side-effect free. Specifically pre-send transport failures (DNS, TCP connect, TLS handshake, fresh-conn RST), plus429and502. Read timeouts,500,503,504are not retried onPOST/PATCH. This preserves at-most-once semantics without requiring any server-side idempotency store.Idempotent methods (
GET/HEAD/PUT/DELETE/OPTIONS) retry on the full transient set{408, 425, 429, 500, 502, 503, 504}plus all transport errors.Zero cross-cutting cost
RetryPolicy.disabled()Behavior change to call out
RetryPolicyis enabled by default. Callers that rely on fast-fail semantics must explicitly opt out viaConnectionConfig(retry_policy=RetryPolicy.disabled()). The Go SDK's existingRetryConfigis preserved as a deprecated alias for one minor version; itsPOSTbehavior on503/504narrows from "retry" to "fail-fast" as a safety fix.What comes next (out of scope for this OSEP)
Last-Event-IDIdempotency-Keyprotocol with a server-side dedup storeThe SSE Resume follow-up is unblocked by this OSEP's requirement that every SDK's SSE parser stop discarding
id:andretry:fields.Files
oseps/0016-resilient-sdk-transport.md— new proposalThe
oseps/README.mdindex entry will be added in a follow-up once the proposal is accepted.Reviewer notes