Feat/go sdk pool#1198
Conversation
Implement OSEP-0005 client-side sandbox pool with: - SandboxPool interface + DefaultSandboxPool (acquire/release/resize/shutdown) - PoolStateStore abstraction with InMemory and Redis implementations - Reconcile loop with leader election, exponential backoff, concurrent warmup - Builder pattern with validation and sensible defaults - 48 unit tests (including race-safe concurrency tests) Closes opensandbox-group#1166 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ython - Add missing Renew call in createOneSandbox when using SandboxCreator path, so idle TTL is properly reset after warmup (matches Kotlin/Python) - Kill orphaned sandbox when Renew fails in directCreate, preventing server-side resource leak - Add primaryLockTTL vs warmupReadyTimeout warning on Start (parity with Kotlin/Python safety check) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Align Go SDK sandbox pool with Kotlin and Python reference implementations per OSEP-0005 review (issue opensandbox-group#1166): 1. Shutdown no longer releases idle sandboxes. In distributed mode, killing shared idle entries on one node's shutdown could disrupt other nodes. Users can still call ReleaseAllIdle() explicitly. 2. Add defer sb.Close() after warmup sandbox creation so only the sandbox ID is retained, matching Kotlin's finally { sandbox.close() } and Python's finally: await sandbox.close(). Prevents future resource leaks when transport pooling is added. 3. Add sb.Close() on renew failure in directCreate, matching Kotlin/Python kill-then-close cleanup pattern. 4. Allow Start() after Shutdown() (restart from STOPPED state), matching Kotlin/Python behavior. Previously Go returned PoolNotRunningError; now users can reuse pool instances. Also includes infrastructure improvements: - Structured logger support via PoolLogger builder method - Reconciler accepts logger parameter for structured logging - Atomic RemoveIdle via Lua script in Redis store - InMemoryStore queue compaction to prevent unbounded growth - Reconciler shrink-excess-idle phase with leader lock renewal
…rent lifecycle bugs - Split PoolStateStoreError into PoolStateStoreUnavailableError and PoolStateStoreContentionError to align with OSEP-0005 error matrix and Python/Kotlin SDKs. - Export PrimaryLockKey on RedisPoolStateStore for e2e test access. - Fix reconcile tick using ReconcileInterval as context timeout; use context.Background() instead (tick operations have their own timeouts via WarmupReadyTimeout). - Fix immediate first tick not syncing healthState back to pool struct; extract syncHealthState() helper called by both paths. - Fix concurrent Start/Shutdown panic from double-closing done channel; add doneClosed guard flag. - Fix concurrent Start/Shutdown deadlock from WaitGroup reuse; add shutdownDone channel so Start waits for previous Shutdown to fully complete before creating new goroutines.
Add 17 pool e2e tests (9 single-node InMemory + 8 Redis distributed) covering warmup, acquire policies, stale idle fallback, lifecycle idempotency, concurrent acquire/shutdown, warmup preparer, warmup concurrency, broken connection degradation, cross-node acquire, primary lock failover, shared resize, atomic take contention, idle TTL expiry, and resize jitter convergence. Add pool helper functions to base_e2e_test.go: createTestPool, eventually, cleanupPool, cleanupBorrowed, cleanupTaggedSandboxes, countTaggedSandboxes, brokenConnectionConfig, etc. Redis tests are gated by OPENSANDBOX_TEST_REDIS_URL env var and automatically skipped when not set.
…te policy When the state store is unavailable (e.g., Redis outage), Acquire now falls through to direct sandbox creation under the DirectCreate policy instead of returning a PoolStateStoreUnavailableError. This keeps the pool at least as available as raw SDK usage during store outages, per the OSEP-0005 error-code action matrix. Under FailFast policy, the store error is still propagated immediately. Added two tests: - TestPool_Acquire_StoreError_DirectCreate_FallsThrough - TestPool_Acquire_StoreError_FailFast_ReturnsError
Add client-side pool usage documentation aligned with the existing Python and Kotlin SDK docs pages, covering: - InMemory pool quick start with builder API - Redis distributed deployment example - Pool lifecycle semantics (info box) - Distributed deployment notes (tip box)
jianpingpei
requested review from
Pangjiping,
hittyt,
jwx0925 and
ninan-nn
as code owners
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: 4b5226d705
ℹ️ 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".
| module github.com/alibaba/OpenSandbox/sdks/sandbox/go | ||
|
|
||
| go 1.20 | ||
| go 1.24 |
There was a problem hiding this comment.
The SDK workflow I checked sets up Go 1.20 and runs go vet/go test in sdks/sandbox/go (.github/workflows/sdk-tests.yml:373-396), but this module now declares Go 1.24 and the new pool files also import log/slog, which is unavailable to that Go 1.20 job. In that CI and documented go 1.20+ install context, the Go SDK no longer builds unless the workflow/docs/support policy are updated together or the new code stays within the supported toolchain.
Useful? React with 👍 / 👎.
| return nil, fmt.Errorf("opensandbox: pool direct create: renew failed: %w", err) | ||
| } | ||
| } | ||
| return sb, nil |
There was a problem hiding this comment.
When the pool is empty or the store falls back to DirectCreate, this path returns the sandbox after creation and optional renewal without running p.config.AcquireHealthCheck, even though the idle-acquire path does. For pools that configure an app-specific acquire check and do not set SkipHealthCheck, cold starts or store outages can hand callers a sandbox before that app-specific readiness condition is satisfied; run the same acquire check before returning here.
Useful? React with 👍 / 👎.
| Timeout: p.config.AcquireReadyTimeout, | ||
| PollingInterval: p.config.AcquireHealthCheckPollingInterval, | ||
| } | ||
| sb, err := ConnectSandbox(ctx, p.config.ConnectionConfig, sandboxID, readyOpts) |
There was a problem hiding this comment.
When AcquireOptions.SkipHealthCheck is true for an idle sandbox, this still passes ReadyOptions into ConnectSandbox; ConnectSandbox calls WaitUntilReady whenever options are supplied, so the idle path still performs the execd readiness check while the direct-create path skips it. Callers using SkipHealthCheck to avoid readiness polling can still fail or block on idle acquisition, so skip passing ready options in that scenario.
Useful? React with 👍 / 👎.
|
|
||
| // WarmupConcurrency sets the maximum number of sandboxes created per reconcile tick. | ||
| func (b *SandboxPoolBuilder) WarmupConcurrency(n int) *SandboxPoolBuilder { | ||
| b.config.WarmupConcurrency = n |
There was a problem hiding this comment.
WarmupConcurrency(-1) is accepted by Build because only zero gets defaulted; on a later reconcile with a deficit, toCreate := min(deficit, cfg.WarmupConcurrency) becomes negative and make([]createResult, toCreate) panics, taking down the process. Validate n > 0 or normalize non-positive values before the pool can start.
Useful? React with 👍 / 👎.
| Volumes: spec.Volumes, | ||
| Extensions: spec.Extensions, | ||
| Platform: spec.Platform, | ||
| ManualCleanup: spec.ManualCleanup, |
There was a problem hiding this comment.
If PoolCreationSpec.ManualCleanup is true, CreateSandbox omits the runtime TTL while the pool store still expires the idle record; after the pool is down longer than IdleTimeout or misses the near-expiry reap window, the expired record is dropped without returning an ID to kill, leaving an untracked sandbox running indefinitely. Reject this option for pool-created sandboxes or avoid forwarding it here.
Useful? React with 👍 / 👎.
| if b.config.IdleTimeout == 0 { | ||
| b.config.IdleTimeout = DefaultIdleTimeout |
There was a problem hiding this comment.
Only the zero value is defaulted here, so IdleTimeout(-1 * time.Hour) is accepted and later produces a negative sandbox timeout plus already-expired in-memory idle entries (Redis clamps the store TTL to 1ms). Validate that the post-default IdleTimeout is positive before initializing the store or deriving AcquireMinRemainingTTL.
Useful? React with 👍 / 👎.
| // Do not use ReconcileInterval as context timeout — the interval | ||
| // controls how often ticks fire, not how long each tick may run. | ||
| // Sandbox creation has its own timeouts (WarmupReadyTimeout). | ||
| p.runReconcileTick(context.Background()) |
There was a problem hiding this comment.
Reconcile ticks are run with context.Background(), while Shutdown(false) only closes done and then waits on the worker group; if a warmup create or preparer is blocked until a long WarmupReadyTimeout, non-graceful shutdown still blocks for that work instead of returning promptly. Use a cancellable reconcile context and cancel it from the non-graceful shutdown path.
Useful? React with 👍 / 👎.
| if err != nil { | ||
| logger.Warn("reconcile: lock acquire error", slog.String("pool_name", poolName), slog.Any("error", err)) | ||
| return |
There was a problem hiding this comment.
When the Redis/state store is unavailable during lock acquisition, this path logs and returns without calling state.recordFailure, so Snapshot().HealthState can remain HEALTHY forever even though the pool cannot reconcile or warm any idle sandboxes. Record these store errors as reconcile failures so operators can detect the degraded pool state.
Useful? React with 👍 / 👎.
1 participant
feat(sdk/go): add client-side sandbox pool (OSEP-0005)
Summary
Implement OSEP-0005 client-side sandbox pool for the Go SDK, enabling millisecond-level sandbox acquisition through pre-warmed idle instances. Fully aligned with the existing Python and Kotlin SDK pool implementations.
Features
SandboxPoolinterface withDefaultSandboxPoolimplementationPoolBuilderwith fluent API, validation, and sensible defaultsDirectCreate(default) /FailFastpoliciesInMemoryPoolStateStorefor single-process deploymentsRedisPoolStateStorewith Lua scripts for atomic distributed operationsTryTakeIdleWithMinTTL/ReapExpiredIdleWithMinTTLfor near-expiry-aware idle managementPooledSandboxCreatorandWarmupSandboxPreparercallbacksResize,ReleaseAllIdle,Snapshot,SnapshotIdleEntriesoperationsDirectCreatepolicy falls through to direct create on store outageslog.LoggerCross-language alignment
The implementation is fully aligned with the Python (
pool_async.py,pool_redis.py) and Kotlin (SandboxPool.kt,RedisPoolStateStore.kt) SDKs in:start,acquire,resize,releaseAllIdle,snapshot,shutdown)PoolStateStoreinterface (includingTryTakeIdleWithMinTTL,SetMaxIdle,SetIdleEntryTTL)TakeIdleResultwithDiscardedAliveSandboxIDsfor near-expiry cleanupPutIdlein reconcileRenewto compensate for creation timeReconcileStatewith degraded threshold and exponential backoffPoolEmptyError,PoolAcquireFailedError,PoolNotRunningError,PoolStateStoreUnavailableError,PoolStateStoreContentionError)File structure
Closes #1166
Testing
Test coverage:
pool_test.go)pool_store_memory_test.go)pool_store_redis_test.go,-tags integration)pool_reconciler_test.go)tests/go/pool_e2e_test.go)tests/go/pool_e2e_test.go)All existing Go SDK and e2e tests continue to pass (
go test ./...).Breaking Changes
Pool is a new opt-in API. Existing SDK usage is completely unaffected.
Checklist