spec: add openspec for pod cache and session manager (SANDBOX-1810) - #10
Conversation
Adds design documents, BDD specs, and implementation tasks for the session manager that handles sandbox pod lifecycle — creation, discovery, command proxying, and cleanup. Co-authored-by: Cursor <cursoragent@cursor.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📜 Recent review details🧰 Additional context used🔀 Multi-repo context codeready-toolchain/mcp-server-devsandbox, codeready-toolchain/mcp-common, codeready-toolchain/api, codeready-toolchain/toolchain-commonLinked repositories findingscodeready-toolchain/mcp-server-devsandbox
codeready-toolchain/mcp-common
codeready-toolchain/api
codeready-toolchain/toolchain-common
WalkthroughAdds new openspec documents for a Kubernetes-backed session manager, including metadata, proposal, design, formal specs for ChangesPod Cache Session Manager Openspec
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
coderabbitai
Bot
added
documentation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@openspec/changes/2026-06-22-pod-cache-session-manager/design.md`:
- Around line 11-12: The GetOrCreatePod method needs to be redesigned to handle
concurrent requests safely across stateless replicas. Instead of checking cache
and labels then creating, implement atomic Kubernetes operations that handle the
race condition: use the Kubernetes API's built-in atomicity by attempting to
create the pod and Secret with unique identifiers and owner references, then
gracefully handling the case where another replica already created them
(catching appropriate API conflicts). This ensures that only one set of
resources is created even when multiple replicas process the same X-Session-ID
simultaneously, making the operation truly idempotent across the distributed
system.
- Line 16: The SandboxConfig struct definition is incomplete and missing fields
that are already part of the proposal contract. Add the three missing tunables
to the SandboxConfig struct: namespace, service account, and kubeconfig secret
name. These should be included alongside the existing fields (image, CPU/mem,
idle timeout, HMAC key, warm pool size) to ensure that cluster-specific values
are configurable rather than hard-coded in buildPodSpec, which will enable
proper deployment across different environments.
In `@openspec/changes/2026-06-22-pod-cache-session-manager/proposal.md`:
- Around line 22-23: Update the CleanupStale method to delete the per-session
Secret in addition to deleting the pod, matching the behavior of CleanupSession.
When iterating through sandbox pods to delete those idle longer than
IdleTimeout, ensure that for each pod being deleted, the associated auth Secret
is also removed to prevent orphaned credentials and namespace clutter.
In
`@openspec/changes/2026-06-22-pod-cache-session-manager/specs/session-manager/spec.md`:
- Around line 24-27: The cache miss scenario in the GetOrCreatePod specification
currently references selecting a "Running pod", but the review requires that
only Ready pods be selected to ensure the agent is actually prepared. Update the
scenario to specify selection of a Ready pod instead of a Running pod, add
clarification on how to handle multiple matching Ready pods (either select one
deterministically by a consistent criteria or fail before caching), and ensure
the caching only occurs after a single deterministic pod selection has been
made.
- Around line 95-98: The scenario "Agent unreachable invalidates cache"
currently specifies invalidating cache on any non-200 HTTP status response,
which is too broad and will evict healthy pods when they return legitimate
command-level 4xx/5xx errors. Narrow the cache invalidation trigger by modifying
the WHEN condition to specify only transport-level failures such as connection
errors, timeouts, and unreachable cases, while excluding HTTP status code-based
invalidations from the cache invalidation logic. This ensures that command-level
errors from healthy pods do not trigger unnecessary cache eviction and pod
rediscovery.
- Around line 34-39: The sessionID is being interpolated directly into
Kubernetes resource names (Secret name pattern cli-mcp-sandbox-auth-<sessionID>)
and label values without validation or normalization against Kubernetes naming
requirements. Define a strict session ID format constraint that meets Kubernetes
DNS subdomain validation rules (RFC 1123 compatible) or implement a
normalization function to sanitize sessionID values before they are used in the
Secret name and any label values throughout the specification. Ensure this
constraint is documented and applied consistently wherever sessionID is embedded
into Kubernetes resource identifiers.
- Around line 84-88: The specification in the "Successful command execution"
scenario describes sending the HMAC-token bearer credential over plaintext HTTP
to http://<podIP>:8090/exec, which is a security vulnerability. Either upgrade
the endpoint from http to https (making it https://<podIP>:8090/exec) to encrypt
the bearer token in transit, or alternatively document the trust boundary
assumptions that justify sending credentials over plaintext HTTP (such as if the
pod and requester are on a secured internal network). Choose the approach that
aligns with your security requirements and update the scenario description
accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 74b5555c-6f9d-4096-8d7e-3f934c1cc1eb
📒 Files selected for processing (6)
openspec/changes/2026-06-22-pod-cache-session-manager/.openspec.yamlopenspec/changes/2026-06-22-pod-cache-session-manager/design.mdopenspec/changes/2026-06-22-pod-cache-session-manager/proposal.mdopenspec/changes/2026-06-22-pod-cache-session-manager/specs/pod-cache/spec.mdopenspec/changes/2026-06-22-pod-cache-session-manager/specs/session-manager/spec.mdopenspec/changes/2026-06-22-pod-cache-session-manager/tasks.md
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
codeready-toolchain/mcp-common(manual)codeready-toolchain/mcp-server-devsandbox(manual)codeready-toolchain/api(manual)codeready-toolchain/toolchain-common(manual)codeready-toolchain/host-operator(manual)codeready-toolchain/toolchain-e2e(manual)
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
openspec/changes/2026-06-22-pod-cache-session-manager/tasks.mdopenspec/changes/2026-06-22-pod-cache-session-manager/proposal.mdopenspec/changes/2026-06-22-pod-cache-session-manager/specs/pod-cache/spec.mdopenspec/changes/2026-06-22-pod-cache-session-manager/specs/session-manager/spec.mdopenspec/changes/2026-06-22-pod-cache-session-manager/design.md
🪛 LanguageTool
openspec/changes/2026-06-22-pod-cache-session-manager/proposal.md
[style] ~3-~3: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ... Kubernetes API call for pod discovery. Without cleanup, abandoned pods would accumulat...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🔇 Additional comments (1)
openspec/changes/2026-06-22-pod-cache-session-manager/.openspec.yaml (1)
1-2: LGTM!
- Make GetOrCreatePod idempotent across replicas via K8s AlreadyExists conflict handling (new Decision 2 in design.md) - Add namespace, service account, kubeconfig secret name to SandboxConfig - Require Ready pods (not Running) with deterministic oldest-first selection - Add session ID validation requirement (RFC 1123 DNS label format) - Document HTTP trust boundary for intra-cluster pod communication - Narrow cache invalidation to transport-level failures only - Make CleanupStale explicitly delegate to CleanupSession for Secret cleanup Co-authored-by: Cursor <cursoragent@cursor.com> Signed-off-by: Feny Mehta <fbm3307@gmail.com>
rajivnathan
left a comment
rajivnathan
left a comment
There was a problem hiding this comment.
Is it possible for a sandbox agent session to be invoked more than once in parallel? If so, what is the expected behaviour of the session manager and sandbox agent?
Yes, parallel invocation of the same session is possible. Here's the expected behavior: Session Manager (server side): The MCP server is stateless — any replica can serve any request. Multiple concurrent ExecuteCommand calls for the same session ID all resolve to the same pod (via cache hit) and POST to the agent's /exec endpoint independently. No server-side queuing or serialization. Sandbox Agent (pod side): The BashSession.Execute() method holds a sync.Mutex for the entire command lifecycle (write to stdin → read stdout/stderr → collect exit code). Parallel HTTP requests to /exec are serialized — the second command blocks until the first completes. There is no interleaving or corruption of output. Each queued command gets its full timeout once it starts executing, not from when the HTTP request arrived. In short: Concurrent calls are safe — they queue at the agent and execute sequentially in the single persistent bash process. Good catch that the spec doesn't document this. The concurrent execution semantics are implicit in the current design (stateless server + mutex-protected bash session) but should be made explicit. I'll add a note to the spec. |
- Add scenario for concurrent command serialization at the agent - Add trade-off entry for queued execution in design.md - Fix tasks.md: Running→Ready pod selection, HTTP→transport-level cache invalidation, add session ID validation task Co-authored-by: Cursor <cursoragent@cursor.com> Signed-off-by: Feny Mehta <fbm3307@gmail.com>
rajivnathan
left a comment
rajivnathan
left a comment
There was a problem hiding this comment.
Sounds good in general, just a couple questions
|
|
||
| #### Scenario: Set overwrites existing entries | ||
| - **WHEN** `Set` is called for a session ID that already has a cached entry | ||
| - **THEN** the entry SHALL be replaced with the new podIP, podName, and a fresh TTL |
There was a problem hiding this comment.
Considering the default TTL is 30s, should this be treated as a collision and rejected instead? Or should it only replace if the existing TTL is expired?
There was a problem hiding this comment.
Overwrite is intentional. Set is only called after a cache miss (the cache-hit path returns directly without calling Set), so we're never overwriting a "known good" entry with something worse — we're always writing a freshly-verified result.
The only overwrite scenario is two concurrent goroutines both missing the cache for the same session, both discovering the same pod, and both calling Set. They'd write the same IP; it just refreshes the TTL by a few milliseconds. Harmless.
If we rejected collisions or only replaced on expiry, we'd break the recovery flow: transport error → Invalidate removes entry → rediscovery at new IP → Set must succeed even if a stale concurrent goroutine also tries to Set
| - **WHEN** `GetOrCreatePod` is called for a session with a valid cache entry | ||
| - **THEN** it SHALL return the cached pod IP without querying the K8s API | ||
|
|
||
| #### Scenario: Cache miss triggers label-based discovery |
There was a problem hiding this comment.
What about when there's a cache miss and the pod exists but is not ready?
It could happen when:
- Another replica started creating the pod, wrote it to K8s, but hasn't finished waitForReady yet (or crashed mid-creation)
- A pod is restarting after a crash and briefly loses its Ready condition
There was a problem hiding this comment.
Great catch — this is a spec gap. The current flow would get stuck: discovery finds no Ready pod → create path → AlreadyExists → falls back to discovery → still no Ready pod.
The fix: discovery should find all pods with matching labels (not just Ready), then:
If a Ready pod exists → return its IP (current behavior)
If a non-Ready, non-terminal pod exists (Pending/Running but not yet Ready) → wait for it to become Ready (reuse waitForReady with the 60s deadline)
If no pod exists, or only terminal pods (Failed/Succeeded) → create new
This handles both cases: another replica mid-creation, and a pod restarting after a crash.
i have updated the spec
- Add spec scenario for cache miss with non-Ready pod (wait for Ready instead of looping into AlreadyExists) - Fix tasks.md: GenerateName → deterministic Name (cli-mcp-sandbox-<sessionID>), required for idempotent create-or-get - Update discoverPod task to skip terminal pods (Failed/Succeeded) and wait for non-Ready pods via waitForReady Co-authored-by: Cursor <cursoragent@cursor.com> Signed-off-by: Feny Mehta <fbm3307@gmail.com>
2 participants
Adds design documents, BDD specs, and implementation tasks for the session manager that handles sandbox pod lifecycle — creation, discovery, command proxying, and cleanup.
Assisted by : Cursor
Summary by CodeRabbit
PodCacheand the session manager’s required behaviors (session-id normalization, pod discovery/selection, exec/invalidation rules, and stale eviction).