feat(sdk/go): add SandboxPoolManager with fence/tombstone parity - #1525
feat(sdk/go): add SandboxPoolManager with fence/tombstone parity#1525MannXo wants to merge 3 commits into
Conversation
The Go SDK had no way to retire a pool namespace. Python and Kotlin both ship SandboxPoolManager.destroy with a DESTROYING -> DESTROYED protocol, so docs/guides/client-pool.md pointed Go users at a manual sequence that races any surviving peer node. Port that protocol to Go: - PoolDestroyState, PoolDestroyStrategy, PoolDestroyOptions and PoolDestroyResult, plus PoolDestroyedError and PoolDestroyIncompleteError. - Four new PoolStateStore methods: GetDestroyState, BeginDestroy, ClearPoolState and MarkDestroyed, implemented in both InMemoryPoolStateStore and RedisPoolStateStore. - SandboxPoolManager and NewSandboxPoolManagerBuilder, exposing Destroy(ctx, poolName, options). Fence enforcement lives in the store, matching Python: PutIdle, SetMaxIdle and SetIdleEntryTTL return PoolDestroyedError while a namespace is fenced, and no peer can take or renew the primary lock. A running pool therefore stops warming up on its next reconcile tick without any change to DefaultSandboxPool. The Redis store does this inside the existing Lua scripts, so the check is atomic with the write. Adding methods to PoolStateStore is a compile-time break for custom store implementations, matching the route Python and Kotlin took. Fixes opensandbox-group#1353
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: a34138be9c
ℹ️ 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".
Store-level fencing alone does not close the retirement race. A peer that is still RUNNING when the fence lands finds an empty idle buffer, and under DIRECT_CREATE or RETRY_NEXT_IDLE_THEN_CREATE the acquire falls through to direct create, minting a fresh sandbox into the namespace being retired. TryTakeIdle is deliberately unfenced so the manager can drain, so nothing on that path stops it. Add the pool-side checks Python already has: - Start refuses to bind a namespace that is DESTROYING or DESTROYED. - Acquire checks the fence before the take loop, so the direct-create fallthrough cannot run against a retired namespace. - A direct create re-checks afterwards and kills the sandbox if a destroy landed mid-create. - The reconcile tick stops the pool once it observes the fence, instead of retrying a namespace that is going away. Store outages keep the OSEP-0005 acquire semantics: policies that already fall through to direct create on an unreachable store treat the destroy state as unknown and proceed, while FAIL_FAST and RETRY_NEXT_IDLE surface the outage.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f1c62c2d4d
ℹ️ 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".
| if err := p.ensureNamespaceActiveForAcquire(ctx, policy); err != nil { | ||
| return nil, err |
There was a problem hiding this comment.
When a destroy fence lands after this preflight check but before or during tryTakeIdle/connectIdle, both stores still allow the idle take so the manager can drain, and the Go acquire path then returns that sandbox without another fence check. In that race the ID has already been popped from the store, so Destroy cannot drain or kill it and a caller receives a sandbox from the retired namespace; re-check the destroy state after a successful idle connect/renew and kill/close on PoolDestroyedError, matching the other SDKs' behavior.
AGENTS.md reference: sdks/AGENTS.md:L122-L122
Useful? React with 👍 / 👎.
|
|
||
| // GetDestroyState returns the destroy state of the pool namespace. | ||
| // An expired tombstone reads back as ACTIVE. | ||
| GetDestroyState(ctx context.Context, poolName string) (PoolDestroyState, error) |
There was a problem hiding this comment.
Adding destroy-state methods directly to PoolStateStore means any external Go user with a custom store that worked with NewSandboxPoolBuilder().StateStore(...) no longer satisfies the public interface, even if they do not use SandboxPoolManager. Consider keeping the existing pool store interface stable and requiring a narrower destroy-capable extension interface for manager/fence support so non-Redis custom stores continue to compile.
AGENTS.md reference: sdks/AGENTS.md:L126-L130
Useful? React with 👍 / 👎.
The preflight check in Acquire leaves a window: a destroy that lands after it, but before or during the idle take, still yields a sandbox to the caller. TryTakeIdle is deliberately unfenced so the manager can drain, so by that point the ID has been popped from the store and the concurrent Destroy can no longer see it, let alone kill it. Mirror what Python does in the same loop: - After a successful idle connect and renew, re-check the namespace and kill and close the sandbox if it is fenced. - Between retry iterations after a connect failure, re-check and stop retrying instead of popping further IDs out from under the drain. The post-take check is fail-closed even when the direct-create policies would otherwise degrade on a store outage. A sandbox taken from the idle buffer is no longer tracked anywhere, so an unconfirmable namespace has to be treated as retired or the sandbox leaks. Direct create keeps its degradation, since nothing has been removed from the store on that path.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2f4372bfe4
ℹ️ 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".
| } | ||
| } | ||
|
|
||
| if err := m.stateStore.ClearPoolState(ctx, poolName); err != nil { |
There was a problem hiding this comment.
When two managers destroy the same pool concurrently, one can pop an idle ID and block in KillSandbox while the other observes an empty queue, finishes with a short tombstone TTL, and allows a new pool to bind after expiry; the first manager then reaches this unconditional cleanup and deletes the new pool's coordination state before tombstoning it again. Although ownerID is recorded, ClearPoolState and MarkDestroyed do not validate it or a fence generation, so stale destroy operations must be prevented from mutating a rebound namespace.
Useful? React with 👍 / 👎.
| break | ||
| } | ||
| drained++ | ||
| if err := m.manager.KillSandbox(ctx, sandboxID); err != nil { |
There was a problem hiding this comment.
When an idle sandbox's DELETE request is slow and the caller context has a longer deadline, this passes the original context to KillSandbox and checks the drain deadline only after that request returns. For example, a 10 ms DrainTimeout can block for the SDK's 30-second request timeout before reporting incomplete, so the documented drain phase is not actually bounded by DrainTimeout; derive a context from the remaining drain budget for each kill.
AGENTS.md reference: AGENTS.md:L44-L44
Useful? React with 👍 / 👎.
1 participant
Summary
Fixes #1353.
The Go SDK had no way to retire a pool namespace. Python and Kotlin both ship
SandboxPoolManager.destroy(poolName, options)with a fullDESTROYING → DESTROYEDprotocol, sodocs/guides/client-pool.mdhad to point Go users at an operator-driven manual sequence that races any surviving peer node. This ports that protocol to Go.Types and errors (
pool_types.go,pool_errors.go)PoolDestroyState(ACTIVE/DESTROYING/DESTROYED),PoolDestroyStrategy(FORCEonly),PoolDestroyOptions,PoolDestroyResult, plusPoolDestroyedErrorandPoolDestroyIncompleteError.DrainTimeoutandTombstoneTTLare*time.Durationso both "use the default" and the meaningful zero are expressible: nil gives 30s / 7 days, an explicit zero drains without a deadline and writes a tombstone that never expires. That matches Python'sdrain_timeout=0andtombstone_ttl=None, and follows the existing*int/*AcquirePolicyconvention in this package.Store methods (
pool_store.go,pool_store_memory.go,poolredis/store.go)GetDestroyState,BeginDestroy,ClearPoolStateandMarkDestroyed, implemented in both stores. The Redis store addsdestroy:stateanddestroy:ownerunder the existing hash-tagged key prefix, so every script stays single-slot on Redis Cluster.Manager (
pool_manager.go)SandboxPoolManagerandNewSandboxPoolManagerBuilder, exposingDestroy(ctx, poolName, options)and following the same five steps as Python and Kotlin: short-circuit on an existing tombstone, write the fence, drain and best-effort kill idle sandboxes up to the drain timeout, clear persistent state, write the tombstone.Fence enforcement (
pool_store_memory.go,poolredis/store.go,pool.go)Enforcement sits at two levels, matching Python.
In the store:
PutIdle,SetMaxIdleandSetIdleEntryTTLreturnPoolDestroyedErrorwhile a namespace is fenced, and no peer can take or renew the primary lock, so replenishment stops. In Redis this is inside the existing Lua scripts, so the check is atomic with the write rather than a separate round trip.In the pool:
Startrefuses aDESTROYING/DESTROYEDnamespace,Acquirechecks the fence before the take loop and again once it holds a live sandbox, retry iterations re-check between attempts, and the reconcile tick stops the pool once it observes the fence.The pool-side checks are load-bearing, not belt-and-braces, because
TryTakeIdleis deliberately unfenced so the manager can drain. Two distinct races follow from that, both caught by the Codex reviews on earlier commits:RUNNINGwhen the fence lands sees an empty idle buffer and mints a fresh sandbox into the retired namespace through the direct-create fallthrough, without ever touching a fenced store write. Regression test:TestSandboxPoolManager_Destroy_BlocksDirectCreateOnLivePool.Destroycan no longer drain or kill it. Regression test:TestPool_Acquire_KillsIdleSandboxFencedMidAcquire.State-store outages keep the OSEP-0005 acquire semantics:
DIRECT_CREATEandRETRY_NEXT_IDLE_THEN_CREATEtreat an unreachable store as "destroy state unknown" and proceed, mirroring the fallthrough already applied toTryTakeIdle, whileFAIL_FASTandRETRY_NEXT_IDLEsurface the outage.That relaxation deliberately stops at a sandbox already taken from the idle buffer, where the check is fail-closed regardless of policy: nothing is tracking that sandbox any more, so an unconfirmable namespace has to be treated as retired or it leaks. Direct create keeps its degradation, since nothing has left the store on that path.
TestPool_Acquire_IdlePathFenceCheckIsFailClosedpins the asymmetry.TestPool_Shutdown_DoesNotReleaseIdleandTestPool_Shutdown_NonGraceful_DoesNotReleaseIdleare untouched.Testing
sdks/sandbox/go—gofmt -l .clean,go vet ./...clean,go test -count=1 -race ./...passing. Newpool_manager_test.gocovers the destroy protocol table-driven (empty pool, drain and kill, best-effort kill failures, zero drain timeout), idempotency, drain timeout leaving the fence in place for a retry, tombstone TTL expiry, the never-expiring zero TTL, and option validation.Fence observation is covered separately: rejection across every fenced store write, a live pool that stops replenishing and reaches
STOPPEDonce destroyed, a fresh peer that refuses to start against the tombstone, a liveDIRECT_CREATEpeer that creates nothing after a destroy, a sandbox killed when the fence lands mid-create, an idle sandbox killed when the fence lands mid-take, the fail-closed post-take check under a store outage, and the store-outage degradation across all four acquire policies.sdks/sandbox/go/poolredis—go test -tags integration -race ./...passing against Redis 7. Seven new tests mirror the in-memory fence, tombstone and validation cases.I checked the new tests fail without the change. Reverting only the store-level fence fails
TestInMemoryPoolStateStore_FenceRejectsWrites,TestSandboxPoolManager_Destroy_FenceStopsLivePoolandTestSandboxPoolManager_Destroy_TombstoneTTLExpires. Reverting only the pre-acquire and post-create checks failsTestSandboxPoolManager_Destroy_BlocksDirectCreateOnLivePoolandTestPool_Acquire_KillsSandboxFencedMidCreate. Reverting only the post-take check failsTestPool_Acquire_KillsIdleSandboxFencedMidAcquireandTestPool_Acquire_IdlePathFenceCheckIsFailClosed.Breaking Changes
The four new methods go on
PoolStateStoredirectly, which is route (a) in the issue and matches Python and Kotlin. Anyone with a customPoolStateStoregets a compile error until they implementGetDestroyState,BeginDestroy,ClearPoolStateandMarkDestroyed; both bundled stores are updated here. This wants a minor SDK version bump. Happy to move it behind a narrower optionalPoolDestroyStorethatSandboxPoolManagertype-asserts if you would rather keep the interface additive.SetMaxIdleandSetIdleEntryTTLon the Redis store now go through a Lua script instead of a plainSET, so they can refuse a fenced write atomically. Behavior on an unfenced namespace is unchanged.Checklist
Docs: the "Retiring an old pool namespace" section of
docs/guides/client-pool.mdnow describes one shared protocol across all three SDKs, with a Go example, instead of documenting the gap.