feat(sdks/pool): add RETRY_NEXT_IDLE acquire policies - #1347
Conversation
Adds two new AcquirePolicy values across Kotlin, Go, and Python SDKs so acquire can skip a stale idle candidate and retry the next one, addressing the case where the pool contains a mix of healthy and unreachable idles (custom templates with long cold-start; leftover unhealthy idles after a network flap): - RETRY_NEXT_IDLE: try up to maxAcquireRetries idle candidates; if all fail, raise PoolAcquireFailedException (fail-fast semantics). - RETRY_NEXT_IDLE_THEN_CREATE: same retry loop, but fall through to direct-create instead of raising. FAIL_FAST and DIRECT_CREATE keep their single-shot semantics unchanged. Retry bound is a new maxAcquireRetries config field (default 3, must be >= 1); each attempt still pays up to acquireReadyTimeout so the bound is required to cap worst-case latency. Implementation notes: - Pool store contract unchanged. tryTakeIdle is atomic pop, so the loop advances via repeated take calls; no new store API required. - Discarded-alive sandbox ids are accumulated across retry iterations and cleaned up asynchronously in a single batch after acquire returns/raises. - Loop re-checks pool lifecycle between iterations so an in-flight shutdown / namespace destroy short-circuits instead of paying extra ready-timeouts. - Go acquire additionally honours ctx cancellation between iterations. Tests: 6 Kotlin + 9 Go + 14 Python (sync 7 / async 7) new tests covering empty idle, all-stale, drained-mid-loop, then_create fallthrough, first- healthy short-circuit, config validation. Existing pool tests unchanged. Docs updated for all three SDKs with an AcquirePolicy comparison table.
github-actions
Bot
added
documentation
![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: 4d19e92f38
ℹ️ 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".
Codex review (PR #1347) pointed out that renew failures were being caught by the same handler as connect/readiness failures. When sandboxTimeout is set, acquire calls sandbox.renew(...) after a successful connect; if the lifecycle API rejects the renew (429, 5xx, transient), the RETRY_NEXT_IDLE* policies were removing the just-connected healthy sandbox, best-effort killing it, and looping through up to maxAcquireRetries more healthy idles that could not succeed for the same reason. This drained healthy pool entries and masked the real (server-side) failure as PoolAcquireFailedError. Fix: split connect+readiness from renew across all three SDKs. - Connect / readiness / health-check failure => idle is stale, remove + best-effort kill + continue loop (unchanged behavior). - Renew failure (against an already-connected sandbox) => close the just- connected sandbox best-effort and surface the raw renew error. Do NOT removeIdle, do NOT kill, do NOT continue. Also renames the Go internal helper connectAndRenew to connectIdle since it no longer performs the renew. Tests: 1 new Go regression test (renewFailingLifecycleServer returns 500 on POST /renew-expiration; asserts single renew attempt, healthy idles untouched, error not wrapped as PoolAcquireFailedError). 2 new Python regressions (sync + async) using existing FakeSandbox.fail_renew=True. Kotlin behavior is verified by inspection since Sandbox.connector() has no injection point in existing tests; Go + Python cover the invariant.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b967369802
ℹ️ 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".
… retry loop Two follow-ups from Codex review round 2 (PR #1347): 1. Renew-failure path leaked the remote sandbox (all three SDKs). tryTakeIdle atomically pops the id out of the idle store; then the just-connected sandbox failed renew. The previous fix only called Close() / close(), which releases local HTTP resources but leaves the remote sandbox alive on the server, untracked, until its TTL expires. Fix: kill the remote sandbox best-effort before closing the local handle and re-raising. Kotlin / Python call sandbox.kill(); Go schedules killSandboxBestEffort (matches how stale-idle failures already kill). 2. Go retry loop did not re-check p.lifecycleState between iterations. Kotlin and Python already do this so a Shutdown(graceful=true) in another goroutine short-circuits the RETRY_NEXT_IDLE loop instead of paying AcquireReadyTimeout per remaining retry (Shutdown uses its own context and does not cancel the caller's acquire ctx). Added an equivalent lifecycle re-check after each candidate failure. Tests updated: - Go regression renamed to RenewFailure_KillsRemoteAndDoesNotRetry; renewFailingLifecycleServer now records DELETE calls and the test asserts exactly one healthy-* DELETE (via short poll for async kill). - Python sync + async regressions now assert connected[0].killed == True by capturing the connected sandbox via a TrackingSandbox subclass (FakeSandbox.connect() had no last_created hook).
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?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: bc7c3545e9
ℹ️ 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".
…_THEN_CREATE Codex review round 3 pointed out that PoolStateStoreUnavailableException raised by try_take_idle was propagating unguarded in Python (sync + async) and Kotlin, aborting acquire before the fallthrough branch could create a sandbox. OSEP-0005 and the Go implementation already degrade to direct-create under fallthrough policies during store outages, so the new RETRY_NEXT_IDLE_THEN_CREATE (and existing DIRECT_CREATE) were less available than documented in Python/Kotlin. Fix: wrap the store take in try/except (Python) / try/catch (Kotlin) and: - Under DIRECT_CREATE / RETRY_NEXT_IDLE_THEN_CREATE: log the outage and break out of the retry loop so control falls through to directCreate(). - Under FAIL_FAST / RETRY_NEXT_IDLE: schedule the pending discarded-alive cleanup and re-raise so callers observe the outage. Also extracts a Kotlin policyFallsThroughToDirectCreate() helper so all three SDKs share one fallthrough classification, and replaces the Python except Exception: pass at the renew-failure close() site with a debug-logged handler (github-code-quality[bot] lint feedback). Tests: 2 Kotlin + 2 Python sync + 2 Python async new regressions using an OutageStore fake that raises PoolStateStoreUnavailableException from both try_take_idle variants. Coverage: THEN_CREATE falls through, non-fallthrough policies re-raise.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 159d2e8882
ℹ️ 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".
Codex review round 4: the awaited stale-candidate kill was blocking the RETRY_NEXT_IDLE* retry loop in Python (sync + async) and Kotlin. When a lifecycle DELETE takes near the default request timeout (30s), a pool ordered [stale, healthy] still waits on the stale kill before returning the healthy sandbox, and each retry adds another DELETE timeout. Go already fires kill in the background (`go p.killSandboxBestEffort`), so this brings the other three implementations in line. Fix: route the stale-candidate kill through the existing warmup executor via _schedule_kill_discarded_alive / scheduleKillDiscardedAlive (which already falls back to inline execution mid-shutdown so cleanup is never silently dropped). Uses a 1-tuple / singleton list to reuse the existing fire-and-forget path. Tests: - Two existing stale-kill tests (Python sync and async test_*acquire_fail_fast_stale_idle_raises_and_kills_candidate, test_*acquire_retry_next_idle_all_stale_bounds_retries_and_raises) now use _eventually(...) to poll for the background kill; they used to assert synchronously and broke with the fire-and-forget change. - Two new positive regressions (test_*acquire_retry_next_idle_does_not_block_on_slow_stale_kill, sync + async) inject a SlowKillManager that blocks kill_sandbox for 2s and assert acquire returns in <2s with the healthy candidate. This directly proves the invariant Codex flagged: retry latency is bounded by connect/readiness, not by cleanup.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1652b0a789
ℹ️ 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".
|
Non-blocking suggestion: the new retry policies are covered well by SDK unit tests, but this PR does not appear to add e2e/manual coverage for the new behavior. It would be useful to add or document an e2e/manual scenario with a real pool containing stale idle IDs ahead of a healthy idle, verifying that RETRY_NEXT_IDLE skips stale candidates and returns the healthy sandbox, and that RETRY_NEXT_IDLE_THEN_CREATE falls through to direct create when the retry budget is exhausted. This would give confidence that lifecycle connect/readiness failures and remote cleanup behave correctly outside the fake/mock store setup. |
…o/Kotlin Adds three e2e scenarios exercising the new RETRY_NEXT_IDLE and RETRY_NEXT_IDLE_THEN_CREATE acquire policies against a real sandbox backend. All three languages cover the same behaviors: - Skip a stale idle candidate and return the next healthy warm sandbox (mixed idle queue: stale ahead of healthy). - RETRY_NEXT_IDLE_THEN_CREATE falls through to direct-create after the bounded retry budget exhausts on an all-stale idle queue. - RETRY_NEXT_IDLE respects maxAcquireRetries and raises PoolAcquireFailedException without falling through to direct-create, so no tagged sandbox is created. Construction of the "mixed" scenario is the same across languages: put a nonexistent sandbox id into the state store before pool.start() so FIFO idle ordering guarantees the stale entry sits ahead of the real idle the reconciler warms to reach maxIdle. Once idle_count == maxIdle the reconciler holds steady (no shrink, no rewarm) and acquire under RETRY_NEXT_IDLE pops the stale, fails its ready-check, and returns the healthy warm sandbox behind it. Test-only changes to fixture helpers: - tests/go/base_e2e_test.go: poolCreateOpts.maxAcquireRetries. - tests/python/tests/test_sandbox_pool_e2e_async.py: _create_pool gains max_acquire_retries kwarg (default 3, matches SDK). - Java tests inline SandboxPool.builder().maxAcquireRetries(...). Verified: ruff+pyright (Python), gofmt+go vet+go build (Go), compileTestJava+spotlessCheck (Java). Not run against a live sandbox backend; requires an OpenSandbox server to actually exercise.
github-actions
Bot
added
the
size/XL
…aces
Follow-up on the previous commit. Two silent-false-green risks in the new
e2e tests, spotted during a hindsight review:
1. mixed test (test_async_retry_next_idle_skips_stale_and_returns_healthy_warm
and its Go/Kotlin counterparts) assumed FIFO ordering put the pre-injected
stale id ahead of the reconciler-created healthy sandbox. FIFO is in fact
guaranteed across all three InMemory stores (verified via source), but if a
future store change ever reordered, the test would silently take the
"healthy first" happy path and no longer cover RETRY. Added an explicit
snapshotIdleEntries head-of-queue assertion before acquire.
2. all-stale + THEN_CREATE test used default reconcile_interval (Python/Go 1s;
Java already 5min). With a 1s tick and per-candidate ready_timeout of 3s,
the reconciler could shrink stale ids concurrently with the retry loop,
causing the loop to see an empty queue mid-iteration and take the
"idle drained" fallthrough branch instead of the "loop_exhausted"
fallthrough branch. Both branches return a healthy sandbox and pass the
existing asserts, but only loop_exhausted actually exercises the RETRY
semantics under test.
Fixed by:
- Setting reconcile_interval to 5 minutes on the Python/Go pools (Java
already had this).
- Asserting after acquire that all pre-injected stale ids have been
removed from the store. Under the long reconcile_interval, only the
acquire retry loop can drain the queue, so this proves the loop
actually iterated over every stale candidate.
Also verified (no code change, just recorded):
- Go/Kotlin/Python InMemoryPoolStateStore are all FIFO
(idleQueue[0]/idleQueue[1:], state.queue.poll()/add, popleft/append).
- Reconciler in all three SDKs does not proactively ready-check idle
entries; it only removes them when idle_count > max_idle (shrink) or
when the store-side TTL expires (reap). Since our injected stale
entries live under the default 24h idle TTL, they are stable in the
queue for the duration of the test.
- Kotlin acquire path raises PoolAcquireFailedException directly (not
SandboxException with a code), so the Java test's assertThrows type
is correct.
- SDK unit tests exercising the same RETRY paths pass locally
(sdks/sandbox/python tests/test_pool_async.py -k retry_next_idle: 10
passed; sdks/sandbox/go TestPool_Acquire_Retry*: passed).
Still NOT run against a live sandbox backend; requires an OpenSandbox
server. The hardening above is what can be verified statically.
|
@ninan-nn Addressed in 246bf61 + 5682aa6 — added e2e coverage across Python (async), Go, and Kotlin/Java for both new policies: Python
Go
Java (Kotlin SDK)
How the mixed [stale, healthy] scenario is constructed (real pool, no mocks):
All-stale fallthrough test (test 2) has a subtle timing consideration: with the default 1s reconcile interval, the reconciler can concurrently shrink stale ids during the acquire loop, causing the loop to see "idle drained mid-loop" instead of "loop_exhausted" — both fallthrough to direct-create and pass the naive assertion, but only one actually exercises the retry semantics. Fixed in 5682aa6 by (a) setting Verified: |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5682aa6b1b
ℹ️ 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".
… outage Codex review round 5 (P2) pointed out that the state-store-outage fallthrough added in commit 159d2e8 was incomplete: the acquire path has two additional state-store touchpoints outside `try_take_idle`, and both were unguarded in Python (sync + async) and Kotlin. When the whole state store (e.g. Redis) is down — not just `try_take_idle*` — full acquire under RETRY_NEXT_IDLE_THEN_CREATE still aborted before reaching the fallthrough branch, making the policy strictly less available than OSEP-0005 documents. The Round 3 outage tests missed this because their fake store only overrode `try_take_idle*` and left `get_destroy_state` returning ACTIVE via the default. The two unguarded touchpoints in acquire were: 1. Pre-loop `_ensure_pool_namespace_active()` call, which reads `get_destroy_state` — Redis down here aborts before the retry loop. 2. Post-create `_ensure_pool_namespace_active_after_create()` inside `_direct_create()` — Redis down here kills the freshly-created sandbox and re-raises, even under a fallthrough policy. Fix (Python async, Python sync, Kotlin; three languages kept in sync): - New `_ensure_pool_namespace_active_for_acquire(policy)` / `ensurePoolNamespaceActiveForAcquire(policy)` helper. If the state store is unavailable (`PoolStateStoreUnavailableException`) and the policy falls through to direct-create, we treat destroy-state as *unknown* and proceed. Non-fallthrough policies (FAIL_FAST / RETRY_NEXT_IDLE) keep fail-closed behavior — the outage is surfaced as-is so callers can react. Used at acquire's pre-loop check and inside `_direct_create()`. - `_ensure_pool_namespace_active_after_create(sandbox, policy=...)` / `ensurePoolNamespaceActiveOrDispose(sandbox, policy=...)` now takes an optional policy. On store outage under a fallthrough policy, the freshly-created sandbox is kept (matches the "at least as available as raw SDK usage" guarantee); on `PoolDestroyedException` or under a non-fallthrough policy, the original fail-closed cleanup + rethrow path runs. Backwards-compatible: existing callers passing no policy default to fail-closed. Go is unaffected: Go's Acquire does not have a pre-loop or post-create `get_destroy_state` check, so the Round 3 fallthrough already covered it. Tests: 2 Python async + 2 Python sync + 2 Kotlin new regressions. Each uses an `OutageStore(WithNamespaceFailure)` fake that starts healthy so `pool.start()` succeeds (mirroring a Redis that crashes after warmup), then flips to full outage mode where every store method — including `get_destroy_state` — raises `PoolStateStoreUnavailableException`. - Fallthrough coverage: acquire under RETRY_NEXT_IDLE_THEN_CREATE still returns a direct-created sandbox (was aborting with the outage exception before this fix). - Fail-closed coverage: acquire under RETRY_NEXT_IDLE still surfaces PoolStateStoreUnavailableException (behavior preserved). All existing pool unit tests continue to pass: - sdks/sandbox/python: 64 pool tests - sdks/sandbox/kotlin: SandboxPoolTest full suite
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: 0587392d44
ℹ️ 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".
2 participants
Summary
Adds two new `AcquirePolicy` values across Kotlin, Go, and Python SDKs so `acquire` can skip a stale idle candidate and retry the next one. This addresses the case where the pool contains a mix of healthy and unreachable idles — custom templates with long cold-start, or leftover unhealthy idles after a network flap — where a single bad idle used to force either `FAIL_FAST` or a slow `DIRECT_CREATE`.
New policies:
`FAIL_FAST` / `DIRECT_CREATE` keep single-shot semantics unchanged.
New config field: `maxAcquireRetries` / `MaxAcquireRetries` / `max_acquire_retries` (default 3, must be `>= 1`). Only takes effect under the retry policies. Bound is required because each failed candidate still pays up to `acquireReadyTimeout`; without a cap, mass-stale idle would freeze all acquires.
Implementation notes
Design decisions
Not in scope (possible follow-ups)
Testing
Docs: VitePress build passes (`pnpm docs:build`).
Breaking Changes
Checklist