feat(sdk): add LRU+TTL endpoint cache with inflight dedup across all SDKs#1133
feat(sdk): add LRU+TTL endpoint cache with inflight dedup across all SDKs#1133Pangjiping wants to merge 1 commit into
Conversation
…SDKs Cache (sandbox_id, port, use_server_proxy) → Endpoint to avoid repeated HTTP roundtrips. Default on: TTL=600s, MaxSize=1024, configurable via ConnectionConfig fields. kill() actively invalidates cached entries. Implements: #582 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| foreach (var kvp in _entries) | ||
| { | ||
| if (kvp.Key.SandboxId == sandboxId) | ||
| toRemove.Add(kvp.Value); | ||
| } |
| import threading | ||
| import time | ||
| from collections import OrderedDict | ||
| from typing import Any, Awaitable, Callable, TypeVar |
![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: 4a46e45bf0
ℹ️ 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.25.0 |
There was a problem hiding this comment.
The Go SDK workflow still sets up Go 1.20 in .github/workflows/sdk-tests.yml before running go vet ./... and go test ./..., and the SDK README still advertises go 1.20+. Bumping this module directive to go 1.25.0 makes the module unusable for that CI job and for documented Go 1.20 consumers before the tests can run, so either the workflow/docs need to move together or this should stay compatible with Go 1.20.
Useful? React with 👍 / 👎.
| this.endpointCacheTtlMs = opts.endpointCacheTtlMs ?? 600_000; | ||
| this.endpointCacheSize = opts.endpointCacheSize ?? 1024; | ||
| this.endpointCacheDisabled = !!opts.endpointCacheDisabled; |
There was a problem hiding this comment.
These new options are lost whenever high-level JS entry points call ConnectionConfig.withTransportIfMissing(): that method constructs a clone but does not pass endpointCacheTtlMs, endpointCacheSize, or endpointCacheDisabled. In the common case where a user supplies new ConnectionConfig({ endpointCacheDisabled: true }) without pre-initialized transport, the cloned config reverts to defaults and caching is still enabled.
Useful? React with 👍 / 👎.
| return cached | ||
|
|
||
| if key in self._inflight: | ||
| return await self._inflight[key] |
There was a problem hiding this comment.
When a second async caller joins an inflight endpoint lookup and that caller is cancelled, for example by asyncio.timeout, awaiting the bare shared Future propagates cancellation to the shared future itself. The owner fetch can then succeed but fail on future.set_result(...), and other waiters see cancellation/invalid-state behavior instead of the resolved endpoint; use asyncio.shield or per-waiter futures for joined callers.
Useful? React with 👍 / 👎.
| return c.cache.GetOrFetch(key, func() (*Endpoint, error) { | ||
| return c.getEndpointFromServer(ctx, sandboxID, port, useServerProxy) | ||
| }) |
There was a problem hiding this comment.
For a long-lived LifecycleClient, resolving an endpoint before PauseSandbox/ResumeSandbox leaves this cache entry live, and a later GetEndpoint after resume returns the old address for up to 10 minutes. The high-level resume flow documents that endpoints may change across pause/resume, so the low-level lifecycle mutators that can change endpoint routing need to clear c.cache for that sandbox before cached lookups are reused.
Useful? React with 👍 / 👎.
| /// <summary> | ||
| /// Removes all cached endpoints for a sandbox. No-op if caching is disabled. | ||
| /// </summary> | ||
| void InvalidateEndpointCache(string sandboxId); |
There was a problem hiding this comment.
ISandboxes is a public SDK interface, so adding a new required method is a source-breaking change for any consumer or test adapter that implements the existing lifecycle contract; those implementations now fail to compile even though endpoint-cache invalidation is an internal adapter concern. Keep this cache hook internal to the concrete adapter or make the extension non-breaking before exposing it through the public interface.
Useful? React with 👍 / 👎.
1 participant
Summary
Cache (sandbox_id, port, use_server_proxy) → Endpoint to avoid repeated HTTP roundtrips. Default on: TTL=600s, MaxSize=1024, configurable via ConnectionConfig fields. kill() actively invalidates cached entries.
closes #582
Testing
Breaking Changes
Checklist