Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion CubeAPI/src/cubemaster/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1397,7 +1397,8 @@ pub struct SandboxUpdateRequest {
/// "pause" | "resume"
#[serde(rename = "action")]
pub action: String,
/// TTL in seconds (for resume; 0 = keep original). Optional for pause.
/// Idle timeout in seconds for resume (-1 = never, 0 = immediate).
/// Omitted for pause or when the caller keeps the current timeout.
#[serde(skip_serializing_if = "Option::is_none")]
pub timeout: Option<i32>,
}
Expand Down
8 changes: 8 additions & 0 deletions CubeAPI/src/handlers/sandboxes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -300,6 +300,7 @@ pub async fn pause_sandbox(
request_body = ResumedSandbox,
responses(
(status = 201, description = "Sandbox resumed", body = Sandbox),
(status = 400, description = "Invalid timeout value", body = ApiError),
(status = 404, description = "Sandbox not found", body = ApiError),
(status = 409, description = "Sandbox is already running", body = ApiError),
(status = 500, description = "Unexpected backend error", body = ApiError)
Expand All @@ -310,6 +311,9 @@ pub async fn resume_sandbox(
Path(sandbox_id): Path<String>,
Json(body): Json<ResumedSandbox>,
) -> AppResult<impl IntoResponse> {
body.validate()
.map_err(|e| AppError::BadRequest(e.to_string()))?;

state
.logger
.log(
Expand Down Expand Up @@ -346,6 +350,7 @@ pub async fn resume_sandbox(
request_body = ConnectSandbox,
responses(
(status = 200, description = "Sandbox connection info", body = Sandbox),
(status = 400, description = "Invalid timeout value", body = ApiError),
(status = 404, description = "Sandbox not found", body = ApiError),
(status = 500, description = "Unexpected backend error", body = ApiError)
)
Expand All @@ -355,6 +360,9 @@ pub async fn connect_sandbox(
Path(sandbox_id): Path<String>,
Json(body): Json<ConnectSandbox>,
) -> AppResult<impl IntoResponse> {
body.validate()
.map_err(|e| AppError::BadRequest(e.to_string()))?;

state
.logger
.log(
Expand Down
31 changes: 28 additions & 3 deletions CubeAPI/src/models/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -359,11 +359,12 @@ pub struct SandboxDetail {
// ─── Sandbox — pause/resume/connect/snapshot ──────────────────────────────

/// Request body for POST /sandboxes/{id}/resume (deprecated).
#[derive(Debug, Deserialize, ToSchema)]
#[derive(Debug, Deserialize, Validate, ToSchema)]
#[allow(dead_code)]
pub struct ResumedSandbox {
/// Idle timeout in seconds; None when the client did not send one.
#[serde(default, skip_serializing_if = "Option::is_none")]
#[validate(custom(function = "validate_timeout_value"))]
pub timeout: Option<i32>,
#[serde(rename = "autoPause", default)]
pub auto_pause: bool,
Expand All @@ -374,6 +375,7 @@ pub struct ResumedSandbox {
pub struct ConnectSandbox {
/// Idle timeout in seconds; None when the client did not send one.
#[serde(default, skip_serializing_if = "Option::is_none")]
#[validate(custom(function = "validate_timeout_value"))]
pub timeout: Option<i32>,
}

Expand Down Expand Up @@ -576,8 +578,8 @@ fn default_page_limit() -> i32 {
#[cfg(test)]
mod tests {
use super::{
CreateTemplateRequest, NewSandbox, SandboxNetworkConfig, SetTimeoutRequest,
TemplateAliasLookupResponse,
ConnectSandbox, CreateTemplateRequest, NewSandbox, ResumedSandbox, SandboxNetworkConfig,
SetTimeoutRequest, TemplateAliasLookupResponse,
};
use validator::Validate;

Expand Down Expand Up @@ -609,6 +611,29 @@ mod tests {
}
}

#[test]
fn resume_and_connect_use_timeout_value_semantics() {
for timeout in [None, Some(-1), Some(0), Some(60)] {
ConnectSandbox { timeout }
.validate()
.unwrap_or_else(|e| panic!("connect timeout={timeout:?} should be valid: {e}"));
ResumedSandbox {
timeout,
auto_pause: false,
}
.validate()
.unwrap_or_else(|e| panic!("resume timeout={timeout:?} should be valid: {e}"));
}

assert!(ConnectSandbox { timeout: Some(-2) }.validate().is_err());
assert!(ResumedSandbox {
timeout: Some(-2),
auto_pause: false,
}
.validate()
.is_err());
}

#[test]
fn sandbox_network_config_accepts_snake_case_policy_fields() {
let cfg: SandboxNetworkConfig = serde_json::from_value(serde_json::json!({
Expand Down
17 changes: 17 additions & 0 deletions CubeAPI/src/routes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -350,6 +350,23 @@ mod tests {
);
}

#[tokio::test]
async fn resume_and_connect_reject_invalid_timeout_before_cubemaster() {
let server = test_server().await;

for path in ["/sandboxes/sb-1/resume", "/sandboxes/sb-1/connect"] {
let response = server
.post(path)
.json(&serde_json::json!({ "timeout": -2 }))
.await;
assert_eq!(
response.status_code(),
StatusCode::BAD_REQUEST,
"path={path}"
);
}
}

#[tokio::test]
async fn template_alias_route_is_mounted_before_template_id_route() {
let server = test_server().await;
Expand Down
149 changes: 148 additions & 1 deletion CubeAPI/src/services/sandboxes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -381,6 +381,13 @@ impl SandboxService {
)?;

d = self.fetch_sandbox_detail(sandbox_id).await?;
} else if d.status == SandboxStatus::Running {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The else if only handles Paused and Running. Any other status — most notably SandboxStatus::Unknown (CubeMaster status 0 = CONTAINER_CREATED, which is what a freshly created sandbox reports until it reaches running) as well as Pausing/Stopping/Error — falls through with no timeout applied, silently dropping the explicit value. That is the exact class of silent-drop this PR is meant to fix. Concretely, Sandbox.connect(id, timeout=300) issued immediately after create races the status transition and can return success while the sandbox keeps its old lifecycle policy. Consider applying the timeout for any status that is not Paused (the resume-with-timeout path), or at minimum rejecting/warning when the requested timeout cannot be applied.

// E2B connect semantics reset an already-running sandbox's
// timeout too. Paused sandboxes apply the same value atomically
// with the successful resume in CubeMaster.
if let Some(timeout) = timeout {
self.set_timeout(sandbox_id, timeout).await?;
Comment on lines +388 to +389

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

self.set_timeout(...).await? propagates any failure and fails the entire Connect request, even though the sandbox is running and the connection info could still be returned. The timeout is an optional side effect here, so a transient error (e.g. NotFound while CubeMaster's localcache is cold after a restart, or a backend error) turns a previously-always-successful "connect to running sandbox" into a hard failure. Consider treating the timeout refresh as best-effort (log and continue) to match the E2B semantics the PR claims, or at least document that Connect now fails when the timeout cannot be applied.

}
}

let envd_version = envd_version_from_annotations(&d.annotations);
Expand Down Expand Up @@ -1131,12 +1138,152 @@ mod tests {
extract::State,
http::{header::RETRY_AFTER, StatusCode},
response::IntoResponse,
routing::{delete, post},
routing::{delete, get, post},
Json, Router,
};
use serde_json::Value;
use tokio::sync::Mutex;

#[tokio::test]
async fn connect_running_sandbox_applies_explicit_timeout() {
#[derive(Clone, Default)]
struct Capture {
timeout_body: Arc<Mutex<Option<Value>>>,
}

async fn info_handler() -> Json<Value> {
Json(serde_json::json!({
"requestID": "req-info",
"ret": { "ret_code": 0, "ret_msg": "ok" },
"data": [{
"sandbox_id": "sb-running",
"host_id": "host-1",
"template_id": "tpl-1",
"status": 1,
"annotations": {}
}]
}))
}

async fn timeout_handler(
State(capture): State<Capture>,
Json(body): Json<Value>,
) -> Json<Value> {
*capture.timeout_body.lock().await = Some(body);
Json(serde_json::json!({
"requestID": "req-timeout",
"sandboxID": "sb-running",
"ret": { "ret_code": 0, "ret_msg": "ok" }
}))
}

let listener = tokio::net::TcpListener::bind("127.0.0.1:0")
.await
.expect("listener should bind");
let address = listener.local_addr().expect("listener address");
let capture = Capture::default();
let app = Router::new()
.route("/cube/sandbox/info", get(info_handler))
.route("/cube/sandbox/timeout", post(timeout_handler))
.with_state(capture.clone());
tokio::spawn(async move {
axum::serve(listener, app).await.expect("server should run");
});

let service = SandboxService::new(
CubeMasterClient::new(format!("http://{address}"), reqwest::Client::new()),
"cubebox".to_string(),
"cube.app".to_string(),
);

service
.connect_sandbox("sb-running", Some(120))
.await
.expect("connect should succeed");

let body = capture
.timeout_body
.lock()
.await
.clone()
.expect("running connect should reset the explicit timeout");
assert_eq!(body["sandboxID"], "sb-running");
assert_eq!(body["timeout"], 120);
}

#[tokio::test]
async fn connect_paused_sandbox_applies_timeout_with_resume_only() {
#[derive(Clone, Default)]
struct Capture {
update_bodies: Arc<Mutex<Vec<Value>>>,
timeout_calls: Arc<Mutex<usize>>,
}

async fn info_handler() -> Json<Value> {
Json(serde_json::json!({
"requestID": "req-info",
"ret": { "ret_code": 0, "ret_msg": "ok" },
"data": [{
"sandbox_id": "sb-paused",
"host_id": "host-1",
"template_id": "tpl-1",
"status": 5,
"annotations": {}
}]
}))
}

async fn update_handler(
State(capture): State<Capture>,
Json(body): Json<Value>,
) -> Json<Value> {
capture.update_bodies.lock().await.push(body);
Json(serde_json::json!({
"ret": { "ret_code": 0, "ret_msg": "ok" }
}))
}

async fn timeout_handler(State(capture): State<Capture>) -> Json<Value> {
*capture.timeout_calls.lock().await += 1;
Json(serde_json::json!({
"requestID": "req-timeout",
"sandboxID": "sb-paused",
"ret": { "ret_code": 0, "ret_msg": "ok" }
}))
}

let listener = tokio::net::TcpListener::bind("127.0.0.1:0")
.await
.expect("listener should bind");
let address = listener.local_addr().expect("listener address");
let capture = Capture::default();
let app = Router::new()
.route("/cube/sandbox/info", get(info_handler))
.route("/cube/sandbox/update", post(update_handler))
.route("/cube/sandbox/timeout", post(timeout_handler))
.with_state(capture.clone());
tokio::spawn(async move {
axum::serve(listener, app).await.expect("server should run");
});

let service = SandboxService::new(
CubeMasterClient::new(format!("http://{address}"), reqwest::Client::new()),
"cubebox".to_string(),
"cube.app".to_string(),
);

service
.connect_sandbox("sb-paused", Some(120))
.await
.expect("connect should succeed");

let update_bodies = capture.update_bodies.lock().await;
assert_eq!(update_bodies.len(), 1);
assert_eq!(update_bodies[0]["action"], "resume");
assert_eq!(update_bodies[0]["timeout"], 120);
assert_eq!(*capture.timeout_calls.lock().await, 0);
}

#[test]
fn map_volume_mounts_returns_none_for_empty_input() {
assert!(map_volume_mounts(&[]).is_none());
Expand Down
53 changes: 53 additions & 0 deletions CubeMaster/pkg/lifecycle/store_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import (
"reflect"
"sync"
"testing"
"time"
)

// recordedCall captures one Do invocation for later assertion.
Expand All @@ -22,6 +23,7 @@ type recordedCall struct {
type fakeRedis struct {
mu sync.Mutex
calls []recordedCall
hget interface{}
// errOn maps command name -> error to return on the Nth call (counter-based).
failHSET bool
failHDEL bool
Expand All @@ -33,6 +35,11 @@ func (f *fakeRedis) Do(cmd string, args ...interface{}) (interface{}, error) {
defer f.mu.Unlock()
f.calls = append(f.calls, recordedCall{cmd: cmd, args: args})
switch cmd {
case "HGET":
if f.hget != nil {
return f.hget, nil
}
return nil, nil
case "HSET":
if f.failHSET {
return nil, errors.New("HSET boom")
Expand All @@ -49,6 +56,52 @@ func (f *fakeRedis) Do(cmd string, args ...interface{}) (interface{}, error) {
return "OK", nil
}

func TestStoreTimeoutProviderRefreshesOnlyMutableTimeoutFields(t *testing.T) {
timeout := 60
createdAt := int64(1700000000000)
meta := SandboxLifecycleMeta{
SandboxID: "sbx-refresh",
TemplateID: "tpl-1",
HostID: "host-1",
HostIP: "10.0.0.1",
InstanceType: "cubebox",
TimeoutSeconds: &timeout,
AutoPause: true,
AutoResume: true,
CreatedAt: createdAt,
}
raw, err := json.Marshal(meta)
if err != nil {
t.Fatalf("marshal seed meta: %v", err)
}
r := &fakeRedis{hget: raw}
provider := &storeTimeoutProvider{store: NewStore(r)}

endAt, err := provider.RefreshTimeout(context.Background(), meta.SandboxID, 120)
if err != nil {
t.Fatalf("refresh timeout: %v", err)
}
if endAt <= time.Now().UnixMilli() || endAt > time.Now().Add(121*time.Second).UnixMilli() {
t.Fatalf("endAt=%d is not a fresh 120-second deadline", endAt)
}

calls := r.snapshot()
if len(calls) != 3 || calls[0].cmd != "HGET" || calls[1].cmd != "HSET" || calls[2].cmd != "XADD" {
t.Fatalf("unexpected refresh calls: %+v", calls)
}
updated, ok := calls[1].args[2].([]byte)
if !ok {
t.Fatalf("updated meta payload type=%T", calls[1].args[2])
}
var got SandboxLifecycleMeta
if err := json.Unmarshal(updated, &got); err != nil {
t.Fatalf("unmarshal updated meta: %v", err)
}
if got.TimeoutSeconds == nil || *got.TimeoutSeconds != 120 || !got.AutoPause || !got.AutoResume || got.TemplateID != meta.TemplateID {
t.Fatalf("refresh did not preserve policy/identity fields: %+v", got)
}
}

func (f *fakeRedis) snapshot() []recordedCall {
f.mu.Lock()
defer f.mu.Unlock()
Expand Down
Loading
Loading